-
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
Partial Merkle tree implementation #156
Conversation
ee270c1
to
10b0532
Compare
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! This is not a full review yet - but I left some comments which will probably lead to some significant changes - so, I can review other parts once these are implemented.
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 left some more comments inline.
a2ee4db
to
69df6ed
Compare
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 left some more comments inline.
9197420
to
6f0c749
Compare
c439bcc
to
218a64b
Compare
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! I left a few more comments inline.
e7f63bd
to
766702e
Compare
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 left a few more comments inline. Also, a couple of general comments about tests:
- When a test has several things that it is doing, it would be good to have a brief comment explaining what is being tested.
- We should probably add tests for error conditions. For example, adding two Merkle paths which resolve to different roots should be an error.
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.
All looks good! Thank you!
For the next PR, let's add some additional methods (e.g., inner_nodes()
) and anything else that we'd need to make this a drop-in replacement for MerklePathSet
.
Implementation of basic functionality for Partial Merkle tree similar to Sparse Merkle tree.
(De)serialization of this structure will be implemented in future PR.