-
Notifications
You must be signed in to change notification settings - Fork 35
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
Add missing methods to Smt
#268
Conversation
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.
Thank you! Looks good! I think the approach works but I left some comments about reducing potential cloning/allocations.
src/merkle/smt/full/mod.rs
Outdated
for (key_in_leaf, value_in_leaf) in self.entries() { | ||
if key == key_in_leaf { | ||
return *value_in_leaf; | ||
} | ||
} | ||
|
||
EMPTY_WORD |
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.
Maybe the compiler can optimize this away, but I'd prefer to handle it using a match statement to avoid potential unnecessary allocations.
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.
fixed
src/merkle/smt/full/mod.rs
Outdated
let leaf = self.get_leaf(key); | ||
|
||
leaf.get_value(key) |
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.
Similar comment as above: this may result in closing of the leaf first and then getting a value from it. May be better to make sure we don't rely on the compiler for optimizing away unnecessary cloning/allocations.
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.
fixed
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
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!
Closes #250