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.

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 merkle proof module #1101

Merged

Conversation

ericnordelo
Copy link
Member

@ericnordelo ericnordelo commented Aug 13, 2024

Fixes #936

  • Tests
  • Documentation
  • Added entry to CHANGELOG.md
  • Tried the feature on a public network

@ericnordelo ericnordelo mentioned this pull request Aug 13, 2024
4 tasks
@ericnordelo ericnordelo marked this pull request as ready for review August 16, 2024 13:21
packages/utils/src/cryptography/merkle_proof.cairo Outdated Show resolved Hide resolved
packages/utils/src/cryptography/merkle_proof.cairo Outdated Show resolved Hide resolved
// Check proof validity.
if (leaves_len + proof.len() != proof_flags_len + 1) {
panic!("MerkleProof: invalid multi proof");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like a perfect case to use assert_eq

Copy link
Member Author

@ericnordelo ericnordelo Aug 16, 2024

Choose a reason for hiding this comment

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

Sadly assert_eq only works on tests.

@ericnordelo
Copy link
Member Author

We can use this for testing (generating trees and proofs)

https://github.com/ericnordelo/strk-merkle-tree

@ericnordelo ericnordelo requested a review from immrsd August 22, 2024 12:28
Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

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

Very nice PR, Eric! The code and tests look great. I left some questions and feedback mostly on docs and comments

CHANGELOG.md Show resolved Hide resolved
packages/utils/src/lib.cairo Outdated Show resolved Hide resolved
Scarb.toml Outdated Show resolved Hide resolved
packages/merkle_tree/src/hashes.cairo Outdated Show resolved Hide resolved

/// These functions deal with verification of Merkle Tree proofs.
///
/// WARNING: You should avoid using leaf values that are 62 bytes long prior to
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the recommendation be to avoid leaf values that are 63 bytes long (31.5 * 2) or can we confirm this may apply to 62 bytes?

Copy link
Member Author

Choose a reason for hiding this comment

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

What about two felt252 long?

Copy link
Collaborator

Choose a reason for hiding this comment

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

"two felt252 long" works! TBH I think specifying the bytes length offers more precision, but I'm okay with how it is now

docs/modules/ROOT/pages/api/merkle-tree.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/merkle-tree.adoc Outdated Show resolved Hide resolved
packages/merkle_tree/src/merkle_proof.cairo Outdated Show resolved Hide resolved
packages/merkle_tree/src/merkle_proof.cairo Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/merkle-tree.adoc Outdated Show resolved Hide resolved
ericnordelo and others added 4 commits August 23, 2024 12:13
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
…elo/cairo-contracts into feat/merkle-proof-verifier-#936
Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

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

I left a few final suggestions, but LGTM!

ericnordelo and others added 3 commits August 27, 2024 13:23
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
….cairo

Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
….cairo

Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
Copy link
Collaborator

@immrsd immrsd left a comment

Choose a reason for hiding this comment

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

Great job on the module, Eric! Everything's looking good, I left several minor comments

docs/modules/ROOT/pages/api/merkle-tree.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/merkle-tree.adoc Outdated Show resolved Hide resolved
let proof = [
0x044fdc540a81d0189ed30b49d64136f9e8bd499c942ba170404ef0b9406e524c,
0x05fb6a626bb2c1e12fc2d6fa7f218ec06928ba5febf4d5677c2c5060827e383b
].span();
Copy link
Collaborator

Choose a reason for hiding this comment

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

The same root and proof values are used in almost all the test cases. Should we consider them constants and place them in the common module alongside with LEAVES?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, but common module is for things that are common among different modules. This can be defined as local test constants.

let proof = [
0x044fdc540a81d0189ed30b49d64136f9e8bd499c942ba170404ef0b9406e524c,
0x05fb6a626bb2c1e12fc2d6fa7f218ec06928ba5febf4d5677c2c5060827e383b
].span();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thing here, moving Poseidon root and proof values to common would help avoid code repetition

ericnordelo and others added 5 commits August 28, 2024 21:32
Co-authored-by: immrsd <103599616+immrsd@users.noreply.github.com>
Co-authored-by: immrsd <103599616+immrsd@users.noreply.github.com>
…elo/cairo-contracts into feat/merkle-proof-verifier-#936
@ericnordelo ericnordelo merged commit d4cca89 into OpenZeppelin:main Aug 29, 2024
6 checks passed
@ericnordelo ericnordelo deleted the feat/merkle-proof-verifier-#936 branch August 29, 2024 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merkle proof verifier
3 participants