Skip to content

Cancel eth tests#136

Merged
pepebndc merged 15 commits into
mainfrom
cancel-eth-tests
Aug 5, 2025
Merged

Cancel eth tests#136
pepebndc merged 15 commits into
mainfrom
cancel-eth-tests

Conversation

@LeoPatOZ

@LeoPatOZ LeoPatOZ commented Jul 11, 2025

Copy link
Copy Markdown
Contributor
  • Updated offchain scripts to account for canceler
  • Added cancellable scenario tests
  • Changed already claimed error to already processed to be more consistent

Summary by CodeRabbit

  • New Features

    • Added support for specifying a canceller address in deposit operations, enabling deposits to be made cancelable.
    • Introduced new error handling for zero receiver addresses in deposits.
    • Expanded deposit proof samples to include cancelable deposits.
    • Added comprehensive test suites covering cancelable and non-cancelable deposit scenarios, including contract and EOA recipients.
  • Bug Fixes

    • Improved error messages for already processed deposits.
  • Tests

    • Added tests to verify deposit cancellation functionality, event emission, ETH transfers on cancellation, and correct error handling for unauthorized cancellations.
    • Introduced new test contracts for various deposit scenarios, including cancelable deposits and contract call validity.
  • Chores

    • Updated variable names and data structures to reflect the new canceler logic and improve clarity.

@coderabbitai

coderabbitai Bot commented Jul 11, 2025

Copy link
Copy Markdown

Walkthrough

This update introduces support for a canceler address in deposit proof generation, propagation, and bridge contract logic. It refactors deposit-related functions and tests to incorporate the canceler field, adds new error types, and expands the test suite with various scenarios for cancelable and non-cancelable deposits, including contract and EOA recipients.

Changes

File(s) Change Summary
Offchain deposit proof generation
offchain/sample_deposit_proof.rs
Adds canceler parameter to deposit proof generation, updates function signatures, and propagates canceler in output.
Bridge contract and interface
src/protocol/ETHBridge.sol, src/protocol/IETHBridge.sol
Adds zero-address check for deposit receiver, renames error AlreadyClaimed to AlreadyProcessed, adds ZeroReceiver error.
Test state and variables
test/ETHBridge/CrossChainDepositExists.t.sol, test/ETHBridge/InitialState.t.sol
Adds cancellationRecipient state variable; splits nonCancellableAddress into cancelerAddress and zeroCanceler.
Test scenarios and contracts
test/ETHBridge/CancelableScenarios.t.sol, test/ETHBridge/EnumeratedScenarios.t.sol, test/ETHBridge/SampleDepositScenarios.t.sol, test/ETHBridge/ContractCallValidityScenarios.t.sol
Adds multiple new contracts testing cancelable and non-cancelable deposits, contract and EOA recipients, and cancellation behavior.
Sample deposit proofs
test/ETHBridge/SampleDepositProof.t.sol
Updates sample proofs to include canceler field; adds two new proofs with non-zero canceler; updates proof data and constants.
Universal deposit tests
test/ETHBridge/UniversalTest.t.sol
Replaces nonCancellableAddress with zeroCanceler in all deposit calls.
Deposit cancellation test update
test/ETHBridge/CancelDepositTest.t.sol
Updates revert error expectation from AlreadyClaimed to AlreadyProcessed when cancelling an already processed deposit.
Recipient contract constants
test/ETHBridge/RecipientScenarios.t.sol
Moves constants REQUIRED_INPUT and RETURN_VALUE outside contract to file-level scope.
Gas report formatting
gas-reports/minimal_inbox_publish.json
Removes quotes from JSON keys and adjusts formatting.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant ProofGen as SampleDepositProofGen
    participant Bridge as ETHBridge
    participant Canceler
    participant Recipient

    User->>ProofGen: Generate deposit proof (with canceler)
    ProofGen-->>User: Deposit proof (includes canceler address)
    User->>Bridge: Submit deposit (with canceler)
    Bridge-->>User: Emit Deposit event

    Canceler->>Bridge: Cancel deposit
    alt Canceler is authorized
        Bridge-->>Canceler: Emit DepositCancelled event
        Bridge-->>Recipient: (No ETH sent)
        Bridge-->>Canceler: ETH sent to cancellationRecipient
    else Unauthorized
        Bridge-->>Canceler: Revert with OnlyCanceler error
    end

    User->>Bridge: Claim deposit
    alt Deposit is cancelled or already claimed
        Bridge-->>User: Revert with AlreadyProcessed error
    else
        Bridge-->>Recipient: ETH sent
        Bridge-->>User: Emit DepositClaimed event
    end
Loading

Possibly related PRs

  • OpenZeppelin/minimal-rollup#120: Introduces the canceler field and cancellation logic in the bridge contract and interface, directly related to the changes in deposit proof generation and testing in this PR.

Suggested reviewers

  • nikeshnazareth

Poem

In fields of code where deposits grow,
A canceler hops in, just so you know!
Proofs now richer, tests abound,
With errors and checks all tightly wound.
Rabbits rejoice, the bridge is strong—
Cancel or claim, you can't go wrong!
🐇✨

Note

⚡️ Unit Test Generation is now available in beta!

Learn more here, or try it out under "Finishing Touches" below.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch cancel-eth-tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
test/ETHBridge/InitialState.t.sol (1)

10-10: Remove unused console import.

The console import appears to be unused and was likely added for debugging purposes.

-import {console} from "forge-std/console.sol";
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 294045c and 42de4fb.

📒 Files selected for processing (10)
  • offchain/deposit_signal_proof.rs (1 hunks)
  • offchain/sample_deposit_proof.rs (6 hunks)
  • src/protocol/ETHBridge.sol (1 hunks)
  • src/protocol/IETHBridge.sol (1 hunks)
  • test/ETHBridge/CancellableScenarios.t.sol (1 hunks)
  • test/ETHBridge/CrossChainDepositExists.t.sol (1 hunks)
  • test/ETHBridge/EnumeratedScenarios.t.sol (2 hunks)
  • test/ETHBridge/InitialState.t.sol (2 hunks)
  • test/ETHBridge/SampleDepositProof.t.sol (2 hunks)
  • test/ETHBridge/UniversalTest.t.sol (9 hunks)
🔇 Additional comments (14)
test/ETHBridge/CrossChainDepositExists.t.sol (1)

18-19: LGTM! Clean addition of cancellation recipient for testing.

The addition of cancellationRecipient follows the established pattern and provides the necessary infrastructure for testing deposit cancellation scenarios.

src/protocol/ETHBridge.sol (1)

91-91: LGTM! Error name accurately reflects the broader processing semantics.

The change from AlreadyClaimed() to AlreadyProcessed() correctly reflects that deposits can now be either claimed or cancelled, making the error message more semantically accurate.

src/protocol/IETHBridge.sol (1)

45-46: LGTM! Interface change properly reflects expanded processing semantics.

The error renaming from AlreadyClaimed to AlreadyProcessed with updated documentation correctly captures that deposits can now be either claimed or cancelled.

offchain/deposit_signal_proof.rs (1)

40-43: LGTM! Correctly adds canceler parameter to deposit call.

The addition of the zero_canceler parameter properly aligns with the updated deposit function signature. Using Address::ZERO appropriately makes the deposit non-cancellable for this proof generation scenario.

test/ETHBridge/UniversalTest.t.sol (1)

26-26: LGTM: Systematic replacement of canceler address parameter.

The consistent replacement of nonCancellableAddress with cancellerAddress across all deposit calls properly aligns with the introduction of the cancellation feature. The test logic remains unchanged, ensuring existing behavior is preserved while adding cancellation capability.

Also applies to: 31-31, 40-40, 45-45, 55-55, 59-59, 69-69, 73-73, 83-83, 87-87, 96-96, 104-105, 118-118, 123-123, 136-136, 143-143, 148-148, 155-155, 162-162, 174-174

offchain/sample_deposit_proof.rs (4)

23-32: LGTM: Proper addition of canceler parameter to function signature.

The create_deposit_call function correctly accepts the new canceler parameter and incorporates it into the generated deposit call code. The parameter is properly positioned in the function signature and used consistently.

Also applies to: 54-54


59-65: LGTM: Appropriate struct extension for canceler field.

The DepositSpecification struct is properly extended with the canceler field, and all deposit specifications are updated to include the canceler address. The use of a fixed non-zero address is appropriate for testing purposes.

Also applies to: 92-92


81-82: LGTM: Proper use of fixed canceler address for testing.

The use of a fixed canceler address (0xf9f5C5411F0bEf1880cE3B051BD14196479764D2) generated from _randomAddress("canceler") is appropriate for testing and ensures consistency across test scenarios.


151-151: LGTM: Correct propagation of canceler address to deposit calls.

The canceler address is properly converted to string and passed to the create_deposit_call function, ensuring the canceler information is correctly included in the generated deposit proof code.

test/ETHBridge/EnumeratedScenarios.t.sol (2)

4-4: LGTM: Proper import of cancellation test scenario.

The import statement correctly brings in the DepositIsCancellable contract from the cancellation scenarios, enabling its integration into the enumerated test framework.


45-54: LGTM: Well-structured test contract for cancellable deposits.

The CancelDepositToEOA contract properly inherits from multiple scenario contracts including the new DepositIsCancellable, creating a comprehensive test case for cancellable deposits to EOAs. The setUp function correctly overrides and calls parent setup methods.

test/ETHBridge/SampleDepositProof.t.sol (2)

41-41: LGTM: Consistent addition of canceler field to all sample deposits.

All sample deposits now correctly include the canceler field set to the fixed address 0xf9f5C5411F0bEf1880cE3B051BD14196479764D2, ensuring consistency across all test scenarios. Since this is an auto-generated file, the systematic inclusion of the canceler field indicates proper tooling integration.

Also applies to: 68-68, 95-95, 122-122, 149-149, 176-176, 203-203, 230-230


252-252: LGTM: Updated state root and block hash constants.

The getStateRoot() and getBlockHash() functions return updated constant values that correspond to the new proof data structure including the canceler field. This ensures the sample proofs remain valid and consistent.

Also applies to: 257-257

test/ETHBridge/CancellableScenarios.t.sol (1)

1-77: LGTM: Comprehensive test suite for deposit cancellation functionality.

The DepositIsCancellable contract provides excellent test coverage for the cancellation feature, including:

  • Successful cancellation by authorized canceller
  • Proper state management (setting processed flag)
  • Event emission verification
  • ETH transfer validation
  • Access control enforcement
  • Integration with claim functionality

The test structure follows best practices with proper use of test infrastructure (vm.prank, vm.expectRevert, vm.expectEmit) and comprehensive assertions. The tests ensure both positive scenarios (successful cancellation) and negative scenarios (unauthorized access, double-processing) are properly covered.

Comment thread test/ETHBridge/InitialState.t.sol Outdated
@OpenZeppelin OpenZeppelin deleted a comment from coderabbitai Bot Jul 11, 2025
@OpenZeppelin OpenZeppelin deleted a comment from coderabbitai Bot Jul 11, 2025

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
test/ETHBridge/CancelableScenarios.t.sol (1)

90-113: Remove commented out code.

The commented code appears to be unrelated to the cancellation functionality and should be removed to keep the file clean.

-// // This contract describes behaviours that should be valid when the deposit is a valid call
-// // to a recipient contract, and all other validity conditions are met.
-// abstract contract DepositIsValidContractCall is
-//     RecipientIsAContract,
-//     BridgeSufficientlyCapitalized,
-//     DepositIsClaimable
-// {
-//     function setUp()
-//         public
-//         virtual
-//         override(CrossChainDepositExists, RecipientIsAContract, BridgeSufficientlyCapitalized)
-//     {
-//         super.setUp();
-//     }
-//
-//     function test_claimDeposit_shouldInvokeRecipientContract() public {
-//         IETHBridge.ETHDeposit memory deposit = sampleDepositProof.getEthDeposit(_depositIdx());
-//         bytes memory proof = abi.encode(sampleDepositProof.getDepositSignalProof(_depositIdx()));
-//
-//         vm.expectEmit();
-//         emit TransferRecipient.FunctionCalled();
-//         bridge.claimDeposit(deposit, HEIGHT, proof);
-//     }
-// }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 615cd12 and ceae146.

📒 Files selected for processing (8)
  • offchain/deposit_signal_proof.rs (2 hunks)
  • offchain/sample_deposit_proof.rs (6 hunks)
  • test/ETHBridge/CancelableScenarios.t.sol (1 hunks)
  • test/ETHBridge/EnumeratedScenarios.t.sol (3 hunks)
  • test/ETHBridge/InitialState.t.sol (1 hunks)
  • test/ETHBridge/SampleDepositProof.t.sol (12 hunks)
  • test/ETHBridge/SampleDepositScenarios.t.sol (1 hunks)
  • test/ETHBridge/UniversalTest.t.sol (9 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
  • offchain/deposit_signal_proof.rs
  • test/ETHBridge/UniversalTest.t.sol
  • test/ETHBridge/InitialState.t.sol
  • offchain/sample_deposit_proof.rs
  • test/ETHBridge/EnumeratedScenarios.t.sol
  • test/ETHBridge/SampleDepositProof.t.sol
🔇 Additional comments (5)
test/ETHBridge/SampleDepositScenarios.t.sol (1)

56-66: LGTM! Clean implementation of cancellable deposit scenarios.

The new abstract contracts follow the established pattern and provide the necessary deposit indices for cancellable scenarios. The implementation is consistent with the existing codebase structure.

test/ETHBridge/CancelableScenarios.t.sol (4)

8-77: Comprehensive test coverage for cancellable deposits.

The DepositIsCancelable contract provides excellent test coverage for the cancellation functionality:

  • Authorization checks (only canceller can cancel)
  • State changes (processed flag is set)
  • Event emission (DepositCancelled)
  • ETH transfer to cancellation recipient
  • Prevention of double processing (claim after cancel fails)

The test implementations are well-structured and follow Solidity testing best practices.


79-88: Good coverage for non-cancellable deposits.

The DepositIsNotCancelable contract correctly tests that cancellation attempts fail when no canceller is set, ensuring proper authorization controls.


40-57: Excellent balance verification in ETH transfer test.

The test correctly verifies that:

  • ETH is transferred from bridge to cancellation recipient
  • Original recipient balance remains unchanged
  • Bridge balance is reduced by the deposit amount

This provides solid assurance that the cancellation mechanism works as intended.


59-67: Good test coverage for preventing double processing.

The test correctly verifies that claiming a deposit after cancellation fails with AlreadyProcessed error, ensuring deposits cannot be processed twice.

@nikeshnazareth

Copy link
Copy Markdown
Contributor

Looks good. A couple of thoughts:

  • In CancelableScenarios, there is some commented out code at the bottom. What's that about?
  • EnumeratedScenarios imports NonzeroETH_ValidCallToPayableFn_IsCancellable but doesn't use it. Were you expecting to enumerate those cases as well?

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
test/ETHBridge/ContractCallValidityScenarios.t.sol (1)

53-54: Fix misleading comment.

The comment states "behaviours that should be valid" but the test actually verifies failure behavior (cancellation should fail with FailedClaim). This is misleading.

-// This contract describes behaviours that should be valid when the deposit is a valid call
-// to a recipient contract, and all other validity conditions are met.
+// This contract describes behaviours when attempting to cancel a deposit that is a valid call
+// to a recipient contract. Such cancellation attempts should fail.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0bb60c7 and 23b951b.

📒 Files selected for processing (6)
  • src/protocol/ETHBridge.sol (2 hunks)
  • src/protocol/IETHBridge.sol (1 hunks)
  • test/ETHBridge/CancelableScenarios.t.sol (1 hunks)
  • test/ETHBridge/ContractCallValidityScenarios.t.sol (2 hunks)
  • test/ETHBridge/EnumeratedScenarios.t.sol (3 hunks)
  • test/ETHBridge/SampleDepositScenarios.t.sol (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
  • src/protocol/ETHBridge.sol
  • src/protocol/IETHBridge.sol
  • test/ETHBridge/SampleDepositScenarios.t.sol
  • test/ETHBridge/CancelableScenarios.t.sol
  • test/ETHBridge/EnumeratedScenarios.t.sol
🔇 Additional comments (3)
test/ETHBridge/ContractCallValidityScenarios.t.sol (3)

4-4: LGTM: Import statement is correct.

The import for DepositIsCancelable is properly structured and aligns with the new cancellation functionality being added.


55-66: LGTM: Contract structure follows established pattern.

The inheritance chain and setUp() function override pattern is consistent with other test contracts in the file.


68-75: No change required: FailedClaim is the correct revert for cancelDeposit

The cancelDeposit flow delegates to _sendETH, which does:

(bool success, ) = to.call{value: value}(data);
require(success, FailedClaim());

Any recipient revert (as in this test) bubbles up as FailedClaim. Expecting IETHBridge.FailedClaim.selector is therefore correct.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
test/ETHBridge/EnumeratedScenarios.t.sol (3)

72-81: Fix naming inconsistency.

The contract name uses lowercase "with" while the previous contract uses "WithNoCalldata" with uppercase "W". Consider maintaining consistency.

-contract CancelDeposit_withCalldata_ToEOACancelRecipient is
+contract CancelDeposit_WithCalldata_ToEOACancelRecipient is

83-83: Fix incorrect comment.

The comment describes a scenario about bridge having no ETH, but the actual contract SimpleDepositToEOA_NoCancelerIsSet inherits from BridgeSufficientlyCapitalized and tests non-cancellable deposits.

-// Same transfer as above, but the bridge does not have ETH. It should fail.
+// Same transfer as above, but no canceler is set. Cancellation attempts should fail.

116-116: Fix grammatical error in comment.

The comment has a grammatical error and unclear wording.

-// Not be able to claim a deposit if the recipient to a cancelable deposit is a contract
+// Should not be able to claim a deposit if the recipient of a cancelable deposit is a contract
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 23b951b and 2732d1d.

📒 Files selected for processing (2)
  • test/ETHBridge/ContractCallValidityScenarios.t.sol (2 hunks)
  • test/ETHBridge/EnumeratedScenarios.t.sol (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/ETHBridge/ContractCallValidityScenarios.t.sol
🔇 Additional comments (6)
test/ETHBridge/EnumeratedScenarios.t.sol (6)

4-4: LGTM: Proper import of cancellable scenario traits.

The import correctly brings in the necessary traits for testing cancellable and non-cancellable deposits.


7-11: LGTM: Comprehensive import of contract call validity scenarios.

The imports correctly include the new CancelableDepositIsValidContractCall trait alongside existing contract validity traits.


17-17: LGTM: Proper import of cancellable deposit scenarios.

The new cancellable deposit scenarios are correctly imported for use in the test contracts.

Also applies to: 20-20


59-69: LGTM: Well-structured test contract for cancellable deposits.

The test contract properly inherits from the necessary traits and follows the established pattern for test setup.


84-93: LGTM: Proper test contract for non-cancellable deposits.

The test contract correctly inherits from the necessary traits to test the scenario where no canceler is set.


117-124: LGTM: Proper test contract for cancellable deposits to contracts.

The test contract correctly inherits from the necessary traits to test cancellation scenarios with contract recipients.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
test/ETHBridge/ContractCallValidityScenarios.t.sol (3)

4-4: Remove duplicate import.

The TransferRecipient import on line 9 is redundant since it's already imported on line 8.

 import {REQUIRED_INPUT, RecipientIsAContract, TransferRecipient} from "./RecipientScenarios.t.sol";
-import {TransferRecipient} from "./RecipientScenarios.t.sol";

Also applies to: 8-9


53-53: Fix typo in comment.

"secnario" should be "scenario".

-// This contract describes the secnario where a cancelable deposit is made but the canceler
+// This contract describes the scenario where a cancelable deposit is made but the canceler

79-86: Consider clarifying the test assertion.

The test name suggests verifying that the recipient contract is not invoked, but the test only checks for a revert. Consider either:

  1. Renaming to test_cancelDeposit_shouldRevertWithEmptyCalldata() to match what's actually tested, or
  2. Adding explicit verification that no contract call occurs before the revert

The current logic is correct - when calldata is empty, the operation should revert with FailedClaim.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3406250 and 64b2d8b.

📒 Files selected for processing (6)
  • src/protocol/ETHBridge.sol (2 hunks)
  • src/protocol/IETHBridge.sol (1 hunks)
  • test/ETHBridge/CancelDepositTest.t.sol (1 hunks)
  • test/ETHBridge/CancelableScenarios.t.sol (1 hunks)
  • test/ETHBridge/ContractCallValidityScenarios.t.sol (2 hunks)
  • test/ETHBridge/RecipientScenarios.t.sol (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • test/ETHBridge/RecipientScenarios.t.sol
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/protocol/ETHBridge.sol
  • src/protocol/IETHBridge.sol
  • test/ETHBridge/CancelableScenarios.t.sol
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: in foundry testing, the syntax `vm.expectrevert(errorselector, 0)` is valid and means "expect zero r...
Learnt from: nikeshnazareth
PR: OpenZeppelin/minimal-rollup#134
File: test/ProverManager/CurrentPeriodIsOpenTest.t.sol:0-0
Timestamp: 2025-06-26T01:42:08.393Z
Learning: In Foundry testing, the syntax `vm.expectRevert(ErrorSelector, 0)` is valid and means "expect zero reverts with that specific error message." This is used to assert that a particular error does NOT occur during function execution, while potentially allowing other errors. This is useful for testing that specific validation checks are bypassed in certain conditions.

Applied to files:

  • test/ETHBridge/CancelDepositTest.t.sol
🔇 Additional comments (4)
test/ETHBridge/CancelDepositTest.t.sol (2)

168-168: LGTM! Error selector update aligns with improved error semantics.

The change from AlreadyClaimed.selector to AlreadyProcessed.selector correctly reflects the updated error naming that improves consistency across the codebase. The new error name "AlreadyProcessed" is more accurate since both claiming and canceling deposits result in the same processed state.


1-220: Excellent test coverage for cancel deposit functionality.

The test suite is well-structured and comprehensive, covering:

  • Basic cancellation scenarios with different data payloads
  • Authorization validation (only canceler can cancel)
  • Event emission verification
  • Double-cancellation prevention
  • Contract interaction testing with MockReceiver

The tests properly use Foundry utilities and follow good testing practices with clear setup, execution, and verification phases.

test/ETHBridge/ContractCallValidityScenarios.t.sol (2)

55-66: LGTM! Contract structure follows established patterns.

The inheritance structure correctly combines cancelable deposit functionality with contract recipient scenarios, and the setUp function properly handles multiple inheritance.


68-77: LGTM! Test logic is sound and well-structured.

The test correctly verifies that when a canceler provides non-empty calldata, the recipient contract's function is invoked and the expected event is emitted.

@github-actions

github-actions Bot commented Aug 1, 2025

Copy link
Copy Markdown

Changes to gas cost

Generated at commit: 1d3d9df8147cd1b8e83d2ed778353f6acdf1bdfe, compared to commit: 68939035ee947a13390e74732e2d329145ee7d30

🧾 Summary (10% most significant diffs)

Contract Method Avg (+/-) %
ETHBridge cancelDeposit
claimDeposit
+207,182 ❌
+9,690 ❌
+256.62%
+2.55%

Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
ETHBridge 1,062,000 (0) cancelDeposit
claimDeposit
deposit
getDepositId
30,333 (0)
331,849 (+551)
64,953 (0)
4,933 (0)
0.00%
+0.17%
0.00%
0.00%
287,916 (+207,182)
389,098 (+9,690)
81,217 (-14)
4,953 (-1)
+256.62%
+2.55%
-0.02%
-0.02%
360,856 (+264,753)
386,756 (+6,770)
82,053 (0)
4,969 (0)
+275.49%
+1.78%
0.00%
0.00%
408,805 (+292,362)
410,318 (+565)
82,869 (0)
4,969 (0)
+251.08%
+0.14%
0.00%
0.00%
21 (+14)
26 (0)
272 (+76)
158 (+66)

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 64b2d8b and 62935cb.

📒 Files selected for processing (10)
  • gas-reports/minimal_inbox_publish.json (1 hunks)
  • offchain/sample_deposit_proof.rs (6 hunks)
  • src/protocol/ETHBridge.sol (1 hunks)
  • src/protocol/IETHBridge.sol (1 hunks)
  • test/ETHBridge/CancelDepositTest.t.sol (1 hunks)
  • test/ETHBridge/CancelableScenarios.t.sol (1 hunks)
  • test/ETHBridge/CrossChainDepositExists.t.sol (1 hunks)
  • test/ETHBridge/InitialState.t.sol (1 hunks)
  • test/ETHBridge/SampleDepositProof.t.sol (14 hunks)
  • test/ETHBridge/UniversalTest.t.sol (9 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
  • test/ETHBridge/CancelDepositTest.t.sol
  • src/protocol/ETHBridge.sol
  • test/ETHBridge/UniversalTest.t.sol
  • test/ETHBridge/InitialState.t.sol
  • offchain/sample_deposit_proof.rs
  • test/ETHBridge/CancelableScenarios.t.sol
  • test/ETHBridge/CrossChainDepositExists.t.sol
  • src/protocol/IETHBridge.sol
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: in erc20bridge and similar bridge contracts, when deploying bridged tokens, it's important to check ...
Learnt from: nikeshnazareth
PR: OpenZeppelin/minimal-rollup#146
File: src/protocol/ERC20Bridge.sol:129-135
Timestamp: 2025-08-01T09:46:40.439Z
Learning: In ERC20Bridge and similar bridge contracts, when deploying bridged tokens, it's important to check both _processed[id] for the specific initialization and _deployedTokens[key] for the original token address. The _processed check alone is insufficient because token upgrades that change metadata (name, symbol, decimals) would generate different initialization IDs, potentially allowing multiple bridged tokens for the same original token address.

Applied to files:

  • test/ETHBridge/SampleDepositProof.t.sol
📚 Learning: in provermanager invariant tests, it's acceptable to rely on solidity mapping behavior that returns ...
Learnt from: nikeshnazareth
PR: OpenZeppelin/minimal-rollup#134
File: test/ProverManager/InvariantTest.t.sol:10-20
Timestamp: 2025-06-24T01:29:22.272Z
Learning: In ProverManager invariant tests, it's acceptable to rely on Solidity mapping behavior that returns zero values for non-existent keys when testing that periods are unused. The zero values serve as a proxy to confirm that a period is not initialized, which is the intended behavior for the invariant tests.

Applied to files:

  • test/ETHBridge/SampleDepositProof.t.sol
🪛 Biome (2.1.2)
gas-reports/minimal_inbox_publish.json

[error] 1-1: Property key must be double quoted

(parse)


[error] 1-2: Property key must be double quoted

(parse)

🔇 Additional comments (4)
test/ETHBridge/SampleDepositProof.t.sol (4)

43-43: Canceler field properly initialized for existing proofs.

The systematic addition of deposit.canceler = address(0x0000000000000000000000000000000000000000); across all existing proofs (0-7) correctly maintains backward compatibility by making these deposits non-cancelable.

Also applies to: 72-72, 99-99, 128-128, 157-157, 186-186, 215-215, 242-242


251-276: New proof 8 correctly implements cancelable deposit.

The addition of proof 8 with deposit.canceler = address(0x738b9be4596e37015bA15F17116c9B2eE971c238) properly demonstrates a cancelable deposit scenario. The proof structure follows the established pattern and enables testing of cancellation functionality.


278-305: New proof 9 enables cancelable contract call testing.

The addition of proof 9 with the same canceler address as proof 8 but including contract call data (deposit.data = bytes(hex"9b28f6fb00000000000000000000000000000000000000000000000000000000000004d2")) enables comprehensive testing of cancellation scenarios involving contract interactions.


320-320: Updated constants align with new deposit structure.

The updated getStateRoot() and getBlockHash() return values correctly reflect the blockchain state that includes the new canceler field in deposit structures.

Also applies to: 325-325

Comment on lines +1 to +2
{num_publications:20,
average_gas_used_publish: 44563} No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix invalid JSON format.

The JSON file has unquoted property keys which violates JSON specification and will cause parsing errors. Property keys must be enclosed in double quotes.

Apply this diff to fix the JSON formatting:

-{num_publications:20,
-average_gas_used_publish: 44563}
+{
+  "num_publications": 20,
+  "average_gas_used_publish": 44563
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{num_publications:20,
average_gas_used_publish: 44563}
{
"num_publications": 20,
"average_gas_used_publish": 44563
}
🧰 Tools
🪛 Biome (2.1.2)

[error] 1-1: Property key must be double quoted

(parse)


[error] 1-2: Property key must be double quoted

(parse)

🤖 Prompt for AI Agents
In gas-reports/minimal_inbox_publish.json at lines 1 to 2, the JSON object keys
are not enclosed in double quotes, which is invalid JSON syntax. To fix this,
enclose all property keys such as num_publications and average_gas_used_publish
in double quotes to comply with JSON standards and avoid parsing errors.

@pepebndc
pepebndc merged commit b72fff2 into main Aug 5, 2025
6 checks passed
@pepebndc
pepebndc deleted the cancel-eth-tests branch August 5, 2025 07:20
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.

3 participants