-
Notifications
You must be signed in to change notification settings - Fork 33
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
Rpo256: Add RpoDigest conversions #311
Conversation
@@ -287,8 +287,7 @@ fn test_empty_leaf_hash() { | |||
#[test] | |||
fn test_smt_get_value() { | |||
let key_1: RpoDigest = RpoDigest::from([ONE, ONE, ONE, ONE]); | |||
let key_2: RpoDigest = | |||
RpoDigest::from([2_u32.into(), 2_u32.into(), 2_u32.into(), 2_u32.into()]); |
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.
Here is an example were backwards compatibility is broken. The compiler doesn't know the type .into()
should go to, because there are new valid targets (bool
, u8
, u16
, u32
).
The fix is to remove the .into()
. IMO the result is more readable.
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.
Looks good! Thank you! One comment: we should probably make similar changes for RpxDigest
as we try to keep them equivalent.
b53e4b7
to
977ebc4
Compare
Quality Gate passedIssues Measures |
synchronized the files |
#[derive(Copy, Clone, Debug)] | ||
pub enum RpxDigestError { | ||
InvalidInteger, | ||
} |
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.
One thing I noticed: I don't think this or the RpoDigestError
are exported from the crate. This probably doesn't cause any immediate issues, but may be a good idea to export them.
Describe your changes
While writing the issues for the proposed trait implementations I noticed some missing conversions. This adds a bunch.
Note: This is a backwards incompatible change, because now there are multiple conversions for arrays. However, I think the new code is actually shorter and easier to use.
Checklist before requesting a review
next
according to naming convention.