Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Add Merkle tree libraries #32

Merged
merged 13 commits into from
Mar 7, 2022
Merged

feat: Add Merkle tree libraries #32

merged 13 commits into from
Mar 7, 2022

Conversation

adlerjohn
Copy link
Member

@adlerjohn adlerjohn commented Jan 14, 2022 •
edited
Loading

Fixes #6.

  • Binary Merkle tree
    • tests
  • Namespace Merkle tree
    • tests

Tests are being added in #51

@adlerjohn adlerjohn self-assigned this Jan 14, 2022
@adlerjohn adlerjohn added the enhancement New feature or request label Jan 14, 2022
@adlerjohn adlerjohn mentioned this pull request Jan 16, 2022
Merged
@adlerjohn adlerjohn marked this pull request as ready for review January 25, 2022 22:24
liamsi
liamsi reviewed Jan 29, 2022
View reviewed changes
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

I think this is all correct but it's hard to tell without any tests. I do think it would be better to add tests and then merge it instead of adding in the tests later.

ethereum/solidity/contracts/lib/tree/binary/BinaryMerkleProof.sol Show resolved Hide resolved
ethereum/solidity/contracts/lib/tree/binary/BinaryMerkleProof.sol Show resolved Hide resolved
@adlerjohn adlerjohn marked this pull request as draft January 29, 2022 01:35
@adlerjohn
Copy link
Member Author

Converting to draft to add tests.

@adlerjohn adlerjohn changed the title Add Merkle tree libraries feat: Add Merkle tree libraries Jan 29, 2022
@musalbas musalbas removed their request for review February 23, 2022 15:47
@adlerjohn adlerjohn marked this pull request as ready for review February 25, 2022 21:14
@adlerjohn
Copy link
Member Author

Tests are implemented in #51, so either this branch can merge into master first, then #51 rebased, or #51 can be merged into here then this merged into master.

rach-id
rach-id approved these changes Mar 7, 2022
View reviewed changes
Copy link
Member

@rach-id rach-id left a comment

Choose a reason for hiding this comment

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

Awesome job 👍 Only small suggestions that i think make sense.

ethereum/solidity/contracts/lib/tree/namespace/TreeHasher.sol Show resolved Hide resolved
ethereum/solidity/contracts/lib/tree/namespace/TreeHasher.sol
Comment on lines +44 to +47
if (l.min == Constants.PARITY_SHARE_NAMESPACE_ID) {
max = Constants.PARITY_SHARE_NAMESPACE_ID;
} else if (r.min == Constants.PARITY_SHARE_NAMESPACE_ID) {
max = r.max;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (l.min == Constants.PARITY_SHARE_NAMESPACE_ID) {
max = Constants.PARITY_SHARE_NAMESPACE_ID;
} else if (r.min == Constants.PARITY_SHARE_NAMESPACE_ID) {
max = r.max;
if (l.min == Constants.PARITY_SHARE_NAMESPACE_ID || r.min == Constants.PARITY_SHARE_NAMESPACE_ID) {
max = Constants.PARITY_SHARE_NAMESPACE_ID;

If I understand right, node.min <= node.max. And since the leaves are ordered. Then, if we have one left leaf/node having a min == PARITY_SHARE_NAMESPACE_ID. Then, all the rest will have {min,max} == PARITY_SHARE_NAMESPACE_ID.
Correct me If I am missing something.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's the reasoning!

Copy link
Member Author

Choose a reason for hiding this comment

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

Your suggested fix isn't correct thought because if the NMT contains non-parity shares, then max should be the maximum non-parity namespace ID. (I copied the logic in the contract directly from the specs for this reason.)

ethereum/solidity/contracts/lib/tree/binary/BinaryMerkleTree.sol Show resolved Hide resolved
@adlerjohn adlerjohn requested a review from rach-id March 7, 2022 18:05
@adlerjohn @rach-id
feat: Add oracle interface (#33)
34364f1 
* Update docs.

* Clean up doc.

* Refactor out message tuple, and add oracle interface.

* Improve docs.

* More docs, fix type.

* Refactor and add oracle iterface.

* Fix incorrect check.

* Add more documentation.

* Fix order of parameters.

* Rename.

* Change to data root tuple.

* Rename file.

* Use data root instead of message.

* Clean up.

* Update readme.

* Update README.md

Co-authored-by: CHAMI Rachid <chamirachid1@gmail.com>

Co-authored-by: CHAMI Rachid <chamirachid1@gmail.com>
@adlerjohn
Copy link
Member Author

Ahhh I accidentally merged #33 into here... @sweexordious any outstanding comments on this one?

@adlerjohn adlerjohn merged commit 5ce2be6 into master Mar 7, 2022
@adlerjohn adlerjohn deleted the adlerjohn/libraries branch March 7, 2022 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@musalbas musalbas musalbas left review comments

@rach-id rach-id rach-id approved these changes

@liamsi liamsi Awaiting requested review from liamsi

@evan-forbes evan-forbes Awaiting requested review from evan-forbes

Assignees

@adlerjohn adlerjohn

Labels
enhancement New feature or request
Projects
No open projects
Celestia Node
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Merkle proof verification for Solidity contracts
4 participants
@adlerjohn @musalbas @liamsi @rach-id