test(e2e): add more test cases for onAbort workflow
#3543
Conversation
📝 WalkthroughWalkthroughThe pull request updates the TestAbort contract across multiple files by modifying its compiled bytecode and consolidating metadata. The Go and JSON files now reference the updated binary and ABI values from a central metadata structure. The Solidity contract is enhanced with a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Contract as TestAbort Contract
participant ZRC20 as IZRC20 Interface
participant Gateway as IGatewayZEVM Interface
User->>Contract: Trigger abort with message "withdraw"
Contract->>Contract: isWithdrawMessage("withdraw")
Contract->>ZRC20: Call withdrawGasFee(...)
ZRC20-->>Contract: Return fee amount
Contract->>Contract: Validate fee and compute withdrawAmount
Contract->>Gateway: Call withdraw(RevertOptions)
Gateway-->>Contract: Confirm withdrawal processed
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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
|
lumtis
changed the title
e2e): add more test cases for onAbort
lumtis
changed the title
e2e): add more test cases for onAborte2e): add more test cases for onAbort workflow
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3543 +/- ##
===========================================
- Coverage 64.68% 64.54% -0.15%
===========================================
Files 453 456 +3
Lines 31310 31747 +437
===========================================
+ Hits 20252 20490 +238
- Misses 10189 10353 +164
- Partials 869 904 +35 |
![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 (4)
e2e/contracts/testabort/TestAbort.sol (4)
13-19: Consider documenting the new struct for clarity.The
RevertOptionsstruct is well-defined, yet it lacks comments or documentation. Adding docstrings describing each field (e.g.,revertAddress,callOnRevert, etc.) can help future readers understand the intended usage and constraints.
21-28: Add high-level comments or events to the interface.The
IGatewayZEVMinterface’swithdrawfunction does not emit events or include comments describing its parameters or expected behavior. To improve maintainability, consider documenting the interface with a brief explanation of how the withdrawal mechanism is meant to operate.
56-72: Review approval logic for potential over-allowance.When calling
approve(msg.sender, abortContext.amount)before callingwithdraw, you are approving the entireabortContext.amountrather than justwithdrawAmount. While this may be intended (e.g., themsg.senderis a trusted gateway), it could present a risk of over-allowance in other scenarios. If more fine-grained control is desired, consider approving onlywithdrawAmountinstead.
75-77: Use caution with string-based message identification.Implementing
isWithdrawMessagevia a hash check is straightforward, but be aware of potential scenarios where a message with extra whitespace or unexpected casing might not match. If the system demands flexible matching, consider normalizing or parsing the message. Otherwise, this strict check should suffice.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
e2e/contracts/testabort/TestAbort.binis excluded by!**/*.bin
📒 Files selected for processing (7)
e2e/contracts/testabort/TestAbort.go(1 hunks)e2e/contracts/testabort/TestAbort.json(1 hunks)e2e/contracts/testabort/TestAbort.sol(2 hunks)e2e/e2etests/e2etests.go(2 hunks)e2e/e2etests/test_erc20_deposit_revert_and_abort.go(2 hunks)e2e/e2etests/test_eth_deposit_and_call_revert_with_call.go(1 hunks)e2e/e2etests/test_eth_withdraw_revert_and_abort.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- e2e/contracts/testabort/TestAbort.json
🧰 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.
e2e/e2etests/test_eth_deposit_and_call_revert_with_call.goe2e/e2etests/e2etests.goe2e/e2etests/test_eth_withdraw_revert_and_abort.goe2e/e2etests/test_erc20_deposit_revert_and_abort.goe2e/contracts/testabort/TestAbort.go
🔇 Additional comments (12)
e2e/contracts/testabort/TestAbort.sol (1)
30-33: Verify usage ofwithdrawGasFeeacross implementations.The
IZRC20interface is concise, which is good. However, ensure thatwithdrawGasFee()consistently returns the correct token and fee across all implementations to prevent unexpected revert scenarios.e2e/e2etests/test_eth_deposit_and_call_revert_with_call.go (1)
27-34: Good clarity and structure in parameter formatting.The updated formatting for
ETHDepositAndCallwithRevertOptionson separate lines makes the code more readable and maintains logical clarity. The approach looks well-structured.e2e/e2etests/test_erc20_deposit_revert_and_abort.go (4)
18-18: Ensure the test can adapt to different scenarios.
require.Len(r, args, 0)mandates no additional arguments. If new scenarios arise requiring dynamic inputs, this restriction may need revisiting.
29-31: Fixed deposit amount helps demonstrate insufficient fee logic.Using a hardcoded amount of
1effectively forces the test to trigger an abort for insufficient fees. This is clear and intentional, though do note that it limits scenario flexibility.
34-34: AligningRevertAddresswith a valid contract address.Switching from a non-existing address to
r.TestDAppV2EVMAddris sensible, ensuring theonRevertcall can succeed if needed.
39-40: Confirm thatAbortAddresssupports the same interface requirements.
AbortAddressis now set to the newly deployedtestAbortcontract. Verify that this address implements any expected interface/logic for an abort flow, especially if other tests rely on it.e2e/e2etests/test_eth_withdraw_revert_and_abort.go (3)
34-38: LGTM! Clear and purposeful configuration of RevertOptions.The RevertOptions configuration is well-structured, with the revert message set to "withdraw" to create a withdrawal during onAbort execution.
55-57: LGTM! Consistent message verification.The abort context verification is properly aligned with the new revert message "withdraw".
59-69: LGTM! Comprehensive withdrawal verification.The test now properly verifies that:
- The withdrawal CCTX is mined
- The final status is OutboundMined
e2e/contracts/testabort/TestAbort.go (1)
45-45: LGTM! Updated contract bytecode.This is an auto-generated file, and the bytecode update reflects the changes in the contract's implementation.
e2e/e2etests/e2etests.go (2)
323-325: LGTM! Enhanced test description and parameters.The changes improve test clarity by:
- Adding explicit mention of CCTX creation verification
- Increasing the default amount to a more realistic value (1 ETH)
389-390: LGTM! Improved test configuration.The test description now clearly indicates the abort condition (revert fee cannot be paid), and the removal of explicit arguments suggests the test uses internally defined values for better control.
Description
Closes #3531
Add some more case for onAbort E2E tests:
Summary by CodeRabbit
New Features
Refactor
Tests