Skip to content

Rust#39

Closed
tiziano88 wants to merge 7 commits intobenlaurie:masterfrom
tiziano88:rust
Closed

Rust#39
tiziano88 wants to merge 7 commits intobenlaurie:masterfrom
tiziano88:rust

Conversation

@tiziano88
Copy link

No description provided.

@tarcieri
Copy link
Contributor

See #38

@tiziano88
Copy link
Author

Thanks @tarcieri I guess I should have checked before doing this

I note though that we have taken different approaches: you have a generic and extensible mechanism for hashing arbitrary Rust types (anything that implements the ObjectHash trait), and new types can easily be added to the set of hashed types.

In my version instead I tried to limit as much as possible the core data structures, as my intent was actually to try and formalise objecthash to a bare minimum set of primitives, since AFAICT there is no formal specification for how the hashing is performed, apart from the existing implementations in various languages.

I was then expecting other libraries to convert higher-level constructs into low-level building blocks, and rely on the hashing function of my lib.

In a way they could coexist (though I'm not saying that's necessarily a good idea): e.g. have my core library as a minimal specification, and then your library implementing the necessary conversions to it. Happy to discuss anyway, also we should hear what @benlaurie thinks about all this 😄

@tarcieri
Copy link
Contributor

For what it's worth, I already have a published crate (that I'm using in another project):

https://crates.io/crates/objecthash

rust/Cargo.toml Outdated
authors = ["Tiziano Santoro <tiziano88@gmail.com>"]

[dependencies]
rust-crypto = "^0.2"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh please no, it's completely unmaintained 😢

Use this instead: https://github.com/RustCrypto/hashes

@tiziano88
Copy link
Author

Thanks for the comments, I'll update the dependency shortly. I am considering renaming my crate to something like objecthash-core or similar, and then perhaps have yours depending on mine for the core serialisation part. I was also thinking it would be nice to write a serde serialiser that emits the hash (one way only, so no deserialiser of course). Anyway I'm also fine dropping my PR in favour of yours if we all agree that's the better thing to do.

@tarcieri
Copy link
Contributor

tarcieri commented Nov 25, 2017

I am considering renaming my crate to something like objecthash-core or similar, and then perhaps have yours depending on mine for the core serialisation part

I'm not sure that makes sense. It would involve building an intermediate graph of Node objects to compute a hash whereas my ObjectHash trait can effectively do it in-place. Even if you were able to do all of this on the stack (it looks like you have heap allocations all over the place due to copious use of vec!) it would still effectively be a 2-pass algorithm versus the 1-pass algorithm my code presently implements.

Really I think I'd prefer to see the "core" behavior implemented as macros, which is the road I've started going down.

I was also thinking it would be nice to write a serde serialiser that emits the hash (one way only, so no deserialiser of course).

Per my comments on #38 I was thinking of going the other direction and leaning on deserializer traits. Specifically I think it could be possible to lean on serde::de::Visitor as the basis of a generalized serde-powered ObjectHash (i.e. impl<T: serde::de::Visitor> ObjectHash for T).

@tiziano88 tiziano88 closed this Oct 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants