feat: support Bitcoin for onAbort and add e2e test cases for no asset calls #3552
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces several new end-to-end test cases covering cross-chain interactions between ZEVM, EVM, and Bitcoin. The tests focus on scenarios involving transaction reversion and abort, adding cases for both Bitcoin deposits and cross-chain calls. Additionally, the observer module in the Bitcoin chain has been updated to include an abort address in the revert options of vote messages. Existing control flows remain unchanged while expanding test coverage and error handling validation. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Test Runner
participant BNode as Bitcoin Node
participant Abort as Abort Contract
Runner->>BNode: Initiate Bitcoin deposit test with minimal satoshi deposit
BNode->>Runner: Mine blocks and process the deposit transaction
Runner->>Abort: Trigger onAbort callback due to revert/abort condition
Abort-->>Runner: Confirm call execution and token balance update
Runner->>Runner: Assert transaction status is aborted
sequenceDiagram
participant Runner as Test Runner
participant Abort as Test Abort Contract
participant EVM as EVM Contract
Runner->>Abort: Deploy test abort contract
Runner->>EVM: Initiate cross-chain call (ZEVM/EVM) with revert options
EVM-->>Runner: Process cross-chain call and return status (Reverted/Aborted)
Runner->>Abort: Trigger onAbort callback based on execution outcome
Runner->>Runner: Verify transaction status and onAbort execution
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3552 +/- ##
===========================================
+ Coverage 64.61% 64.70% +0.08%
===========================================
Files 458 460 +2
Lines 31825 31900 +75
===========================================
+ Hits 20565 20640 +75
Misses 10355 10355
Partials 905 905
|
lumtis
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
e2e/e2etests/test_evm_to_zevm_call_abort.go (1)
17-43: Consider adding test cleanup and using constants.The test structure is clear, but consider these improvements:
- Add cleanup for the deployed contract
- Extract magic values like "revert" into constants
+const ( + testRevertMessage = "revert" +) func TestEVMToZEVMCallAbort(r *runner.E2ERunner, args []string) { require.Len(r, args, 0) + defer func() { + // Cleanup deployed contract + _ = r.ZEVMClient.Close() + }() // ... rest of the test - []byte("revert"), + []byte(testRevertMessage),e2e/e2etests/test_zevm_to_evm_call_revert.go (1)
16-49: Add documentation and extract magic numbers.The test would benefit from:
- Documentation explaining the test's purpose and expected behavior
- Extracting the hardcoded gas limit into a constant
- Ensuring randomPayload helper is properly defined
+// TestZEVMToEVMCallRevert verifies that a cross-chain call from ZEVM to EVM +// properly reverts when calling a non-existing address, and the revert message +// is correctly propagated back to the sender. + +const ( + defaultRevertGasLimit = 200000 +) func TestZEVMToEVMCallRevert(r *runner.E2ERunner, args []string) { // ... rest of the test - OnRevertGasLimit: big.NewInt(200000), + OnRevertGasLimit: big.NewInt(defaultRevertGasLimit),e2e/e2etests/test_zevm_to_evm_call_revert_and_abort.go (1)
17-54: Validate gas limit and reuse constants.Consider these improvements:
- Validate that the gas limit is sufficient
- Reuse the revert message constant
- Add validation for the abort context values
+const ( + minRevertGasLimit = 100000 + defaultRevertGasLimit = 200000 + revertMessage = "revert" +) func TestZEVMToEVMCallRevertAndAbort(r *runner.E2ERunner, args []string) { require.Len(r, args, 0) + + // Validate gas limit + gasLimit := big.NewInt(defaultRevertGasLimit) + require.True(r, gasLimit.Cmp(big.NewInt(minRevertGasLimit)) >= 0, + "Gas limit must be at least %d", minRevertGasLimit) // ... rest of the test - []byte("revert"), + []byte(revertMessage),e2e/e2etests/test_bitcoin_std_deposit_and_call_revert_and_abort.go (2)
15-62: Document memo fields and extract constants.The test has good structure but could benefit from:
- Documentation for memo fields and their purpose
- Constants for the deposit amount
- Validation of the balance check
+const ( + // oneStaoshi is intentionally low to trigger insufficient gas error + oneSatoshi = 0.00000001 +) func TestBitcoinStdMemoDepositAndCallRevertAndAbort(r *runner.E2ERunner, args []string) { // Start mining blocks stop := r.MineBlocksIfLocalBitcoin() defer stop() require.Len(r, args, 0) - amount := 0.00000001 // 1 satoshi so revert fails because of insufficient gas + amount := oneSatoshi // ... rest of the test // check abort contract received the tokens balance, err := r.BTCZRC20.BalanceOf(&bind.CallOpts{}, testAbortAddr) require.NoError(r, err) - require.True(r, balance.Uint64() > 0) + require.True(r, balance.Uint64() > 0, + "Expected non-zero balance, got %d", balance.Uint64())
28-41: Add documentation for memo fields.The memo structure would benefit from documentation explaining the purpose of each field.
+ // Create a memo to call non-existing contract with the following fields: + // - Version: 0 (current version) + // - EncodingFmt: Compact short format for efficiency + // - OpCode: Deposit and call operation + // - Receiver: Non-existing contract to trigger revert + // - RevertOptions: Configuration for abort handling inboundMemo := &memo.InboundMemo{ Header: memo.Header{ Version: 0,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/zetae2e/local/bitcoin.go(1 hunks)cmd/zetae2e/local/evm.go(1 hunks)e2e/e2etests/e2etests.go(5 hunks)e2e/e2etests/test_bitcoin_std_deposit_and_call_revert_and_abort.go(1 hunks)e2e/e2etests/test_evm_to_zevm_call_abort.go(1 hunks)e2e/e2etests/test_zevm_to_evm_call_revert.go(1 hunks)e2e/e2etests/test_zevm_to_evm_call_revert_and_abort.go(1 hunks)zetaclient/chains/bitcoin/observer/event.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to ...
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetae2e/local/bitcoin.gozetaclient/chains/bitcoin/observer/event.gocmd/zetae2e/local/evm.goe2e/e2etests/test_evm_to_zevm_call_abort.goe2e/e2etests/test_zevm_to_evm_call_revert_and_abort.goe2e/e2etests/test_bitcoin_std_deposit_and_call_revert_and_abort.goe2e/e2etests/test_zevm_to_evm_call_revert.goe2e/e2etests/e2etests.go
🔇 Additional comments (9)
cmd/zetae2e/local/evm.go (1)
66-68: LGTM! The new test cases enhance error handling coverage.The added test cases provide comprehensive coverage for cross-chain call scenarios involving reverts and aborts between ZEVM and EVM.
cmd/zetae2e/local/bitcoin.go (1)
35-35: LGTM! The new test case enhances Bitcoin error handling coverage.The added test case provides coverage for Bitcoin deposit and call scenarios involving both revert and abort conditions.
zetaclient/chains/bitcoin/observer/event.go (1)
220-220:✅ Verification successful
LGTM! The abort address support enhances error handling for Bitcoin transactions.
The addition of
AbortAddresstoRevertOptionsenables proper handling of onAbort functionality for Bitcoin transactions.Run the following script to verify that the abort address is properly handled in other Bitcoin-related code:
🏁 Script executed:
#!/bin/bash # Description: Verify abort address handling in Bitcoin-related code. # Test: Search for other occurrences of AbortAddress to ensure consistent handling. rg -A 5 $'AbortAddress' # Test: Search for potential missing abort address handling in Bitcoin-related code. rg -g 'bitcoin/**' -A 5 $'RevertOptions'Length of output: 31575
Approval: Abort Address Enhancement Verified
The integration of the
AbortAddressfield intoRevertOptionsin the Bitcoin observer is consistent with its usage throughout the codebase and documentation. The search results confirm that this new field is referenced appropriately in tests, contract integrations, and the standard memo format. Note that the secondary search forRevertOptionswithin the Bitcoin directory did not yield output due to filtering limitations; however, manual inspection ofzetaclient/chains/bitcoin/observer/event.goshows the correct incorporation ofAbortAddress.
- Confirmed usage in documentation (RFC) and in multiple test files.
- Consistent implementation alongside related fields (e.g.,
RevertMessage) in standard memo handling.- Suggest a manual re-check of Bitcoin modules if extra caution is needed during regression testing.
LGTM.
e2e/e2etests/e2etests.go (6)
49-50: LGTM! The new test case constants enhance test coverage for ZEVM to EVM calls.The added test case constants follow the established naming convention and provide clear descriptions of their purpose.
53-53: LGTM! The new test case constant enhances test coverage for EVM to ZEVM calls.The added test case constant follows the established naming convention and provides a clear description of its purpose.
99-99: LGTM! The new test case constant enhances test coverage for Bitcoin transactions.The added test case constant follows the established naming convention and provides a clear description of its purpose.
471-482: LGTM! The new test definitions enhance test coverage for ZEVM to EVM calls.The added test definitions include appropriate descriptions and follow the established pattern for test registration.
495-500: LGTM! The new test definition enhances test coverage for EVM to ZEVM calls.The added test definition includes an appropriate description and follows the established pattern for test registration.
740-745: LGTM! The new test definition enhances test coverage for Bitcoin transactions.The added test definition includes an appropriate description and follows the established pattern for test registration.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Description
Closes #3530 #3532
Finally seems it's already supported so this PR just add the test
no asset call evm -> zevm aborts
no asset call zevm -> evm reverts (was actually missing in the first place)
no asset call zevm -> evm aborts
Summary by CodeRabbit