feat(AggLayer): implement verify_leaf_bridge#2288
feat(AggLayer): implement verify_leaf_bridge#2288mmagician merged 21 commits intoagglayer-fixed-2from
verify_leaf_bridge#2288Conversation
mmagician
changed the title
verify_leaf_bridgeverify_leaf_bridge
mmagician
added
the
no changelog
|
Added a :no changelog" label (as this PR is included in the changelog entry from #2295) |
mmagician
added
the
agglayer
* feat: verify_leaf stubbed feat: fill some TODOs * chore: test global index processing * chore: update comments * self review * changelog
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left some comments inline - most should be pretty easy to address.
| #! Outputs: [LEAF_VALUE] | ||
| #! Outputs: [LEAF_VALUE[8]] | ||
| #! |
There was a problem hiding this comment.
Any reason not to keep the TODO here?
There was a problem hiding this comment.
Not for this PR, but I wonder if a better name for this module is global_exit_tree.masm. It seems like most (if not all) functionality here is about global exit trees. And the naming would also be consistent with local_exit_tree.masm module we already have.
| #! Outputs: [leaf_index] | ||
| #! | ||
| #! Invocation: exec | ||
| pub proc process_global_index_mainnet |
There was a problem hiding this comment.
Having this procedure be pub makes it a part of the account interface. AFAICT, this is an internal procedure, and so, should probably not be pub.
There was a problem hiding this comment.
This has unit tests, IIRC making the procedure public was necessary for that?
Addresses PR feedback: clarify that mainnet exit root is at exit_roots_ptr and rollup exit root is at exit_roots_ptr + 8. Co-authored-by: marti <marti@hungrycats.studio>
This procedure has been replaced by verify_merkle_proof and is no longer used. Co-authored-by: marti <marti@hungrycats.studio>
Addresses PR feedback: organize the file with clear section headers for: - Constants (storage slots and memory layout) - Errors - Public interface procedures - Helper procedures Co-authored-by: marti <marti@hungrycats.studio>
mmagician
force-pushed
the
mmagician-verify-leaf-bridge
branch
from
b68528b to
86d7153
Compare
mmagician
force-pushed
the
mmagician-verify-leaf-bridge
branch
from
31aff4e to
1b3acaf
Compare
|
I know this has been approved already, but I've done a rather large rebase (on top of |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you. I left some comments inline - mostly about adding clarity and improving code organization.
| #! Verifies a Merkle proof for a leaf value against a root. | ||
| #! | ||
| #! Inputs: [smt_proof_ptr, leaf_index, LEAF_VALUE[8], root_ptr] | ||
| #! Outputs: [] | ||
| #! | ||
| pub proc verify_merkle_proof |
There was a problem hiding this comment.
I'm assuming this procedure will panic if the proof cannot be verified, right? If so, would be good to say this explicitly.
Also, I'm assuming we are panicking rather than returning a boolean too keep the interfaces consistent with Solidity code?
There was a problem hiding this comment.
the proc here is just a stub, and the panic docs are already addressed by Andrew in #2361
Also, I'm assuming we are panicking rather than returning a boolean too keep the interfaces consistent with Solidity code?
indeed
closes #2276