-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Made Hash store a byte array, so Node can store a Hash, rather than a byte slice #67
base: master
Are you sure you want to change the base?
Made Hash store a byte array, so Node can store a Hash, rather than a byte slice #67
Conversation
src/crypto/hash.rs
Outdated
@@ -112,6 +140,12 @@ impl DerefMut for Hash { | |||
} | |||
} | |||
|
|||
impl fmt::Display for Hash { | |||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | |||
write!(f, "{:x?}", self.hash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for compat with the JS version we should probably use pretty-fmt
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I thought there was somewhere I'd missed that!
Related question: are we aiming for full storage API compatibility with the js version, as well as network API compatibility? If so are the JS maintainers aware that any braking change they make will also break the rust version?
Thanks heaps for this PR! Unfortunately in its current state we can't accept it, because it opens us up to timing attacks. Keeping I do really like some of the ergonomics improvements introduced here; typing Thanks again so much; overall I really like this direction! |
Ahh, I should have asked before I picked a It looks like like |
053a054
to
19dbe3d
Compare
Opening an issue is a good way to go about this! Otherwise I'm also around on Freenode and Discord for synchronous chat.
Possibly! But looking at this patch I'm thinking that adding more conversion traits on the existing codebase could probably get us the closest to our intended goal with the fewest amount of changes required. |
I've switched
Do you mean implementing |
@yoshuawuyts sorry to bother, but I'm wondering how do you feel in regards of the new constant hash verification in this current PR. |
🙋 feature
Key Changes:
crypto::hash::Hash
to contain a[u8; 32]
, rather thanBlake2bResult
storage::node::Node.hash
to be aHash
, rather than aVec<u8>