feat!(ethexe): transition of value claims to merkle root#5415
feat!(ethexe): transition of value claims to merkle root#5415StackOverflowExcept1on wants to merge 3 commits intomasterfrom
Conversation
StackOverflowExcept1on
added
scope: vara.eth
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request transitions the value claim mechanism in the ethexe system from using arrays to Merkle roots. This change optimizes the data structure for value claims, allowing for more efficient verification via Merkle proofs. Please note that this update introduces a breaking change for existing program states on Ethereum, though it does not affect the underlying database state. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request transitions the ethexe value claim mechanism from a push-based model to a pull-based model using Merkle proofs. It introduces a BinaryMerkleTree Solidity library, updates the Mirror contract to store Merkle roots of claims, and adds a claimValue function for users to claim funds individually. Feedback was provided to ensure that failed Ether transfers in claimValue revert the transaction to allow for retries and prevent permanent fund locking. Additionally, a comment in the BinaryMerkleTree library was flagged as misleading, noting that commutative hashing would break compatibility with the Rust-side root calculation.
| _processedValueClaims[_claim.messageId] = true; | ||
|
|
||
| bool success = _transferEther(_claim.destination, _claim.value); | ||
| if (success) { | ||
| emit ValueClaimed(_claim.messageId, _claim.value); | ||
| } else { | ||
| emit ValueClaimFailed(_claim.messageId, _claim.value); | ||
| } |
There was a problem hiding this comment.
In the new pull-based claimValue function, the claim is marked as processed (_processedValueClaims[_claim.messageId] = true) even if the ether transfer fails. Since the user initiates this call, they should be able to retry if the transfer fails (e.g., if the destination is a contract that temporarily reverts or runs out of gas). Marking it as processed on failure permanently locks the funds in the contract. It is recommended to use require(success, ...) to revert the transaction on failure, which ensures the state is not updated and allows for retries.
_processedValueClaims[_claim.messageId] = true;
bool success = _transferEther(_claim.destination, _claim.value);
require(success, "Transfer failed");
emit ValueClaimed(_claim.messageId, _claim.value);
1 participant
Related to #5406
Summary
Transition to merkle roots, but only for value claims for now
TODO
How to test
See tests that use merkle proofs to claim value
Notes
This will break state of programs on Ethereum, but will not break them in database
Checklist
type(scope): description)