feat: support advanced abort workflow (onAbort)#3414
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 extensive changes focused on enhancing abort handling and error management across cross‐chain transactions. Notable updates include the introduction of a new Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant FK as Fungible Keeper (ProcessDeposit)
participant GC as Gateway Contract (CallExecuteAbort)
participant TA as TestAbort Contract
U->>+FK: Initiate deposit/withdraw with revert options
FK->>GC: Process deposit and check for revert conditions
alt Revert condition detected
GC->>TA: Invoke executeAbort (with AbortContext via onAbort)
TA-->>GC: Respond with abort confirmation
GC-->>FK: Return aborted status with error message
else Successful deposit
GC-->>FK: Return success response
end
FK-->>U: Deliver final transaction result
sequenceDiagram
participant C as CCTX Gateway Observer
participant K as CrosschainKeeper
participant F as FungibleKeeper (ProcessAbort)
participant S as System (Status Update)
C->>K: Detect error during outbound processing
K->>F: Call ProcessAbort with context and status messages
F-->>K: Return abort result
K->>S: Update CCTX status to Aborted
S-->>C: Provide updated transaction status
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3414 +/- ##
===========================================
- Coverage 64.83% 64.68% -0.16%
===========================================
Files 454 453 -1
Lines 31154 31310 +156
===========================================
+ Hits 20199 20252 +53
- Misses 10090 10189 +99
- Partials 865 869 +4
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
testutil/keeper/crosschain.go (1)
307-334: 🛠️ Refactor suggestionUpdate mock function name to match implementation.
The function name
MockProcessV2RevertDepositshould be updated toMockProcessRevertto match the implementation changes. This ensures consistency with the new method names in the codebase.Apply this diff:
-func MockProcessV2RevertDeposit( +func MockProcessRevert(
🧹 Nitpick comments (32)
x/fungible/keeper/deposits_legacy.go (5)
16-20: Validateamountinput.Consider adding robust checks to ensure that
amountis non-nil and non-negative. This will help prevent potential runtime issues and guarantee safe minting operations.
22-24: Add argument validation foramount.Similar to
DepositCoinZeta, you could validate thatamountis non-nil and non-negative here as well, returning a typed error if the input is invalid.
26-82: Ensure fallback for unsupported protocol versions.Currently, the logic branches to
ProcessDepositwhen the protocol version isV2and otherwise proceeds with a legacy flow. Consider adding an explicit fallback for unsupported versions to avoid silent acceptance or partial handling.
26-82: Log errors on message/contract mismatch.When the address is an externally owned account (line 75) but a non-empty message is provided, returning an error is correct. However, logging additional context (e.g., the message or address) could be beneficial for debugging.
84-135: Refine foreign coin checks.This function performs multiple checks (paused coin, cap reached, etc.). You may want to reduce branching by extracting certain logic (e.g., pause checks, cap checks) into smaller functions. This can simplify unit testing and maintenance.
x/fungible/keeper/deposits.go (4)
16-53: Use a switch statement for coin type checks.
ProcessDepositbranches oncoinTypeandisCrossChainCall. Adopting a switch-by-coin-type approach may improve clarity, reduce nested conditionals, and make future additions for more coin types easier to maintain.
16-53: Clarify ZETA asset support.Returning an error for
coinType == coin.CoinType_Zetais correct for now, but consider adding an inline comment or TODO if you plan to support ZETA deposits in the future.
55-113: Consider unifying revert logic.
ProcessReverthas a fair amount of branching forCoinType_NoAssetCall,CoinType_ERC20, etc. Keeping each path distinct is fine, but you could factor out common operations (like retrievingzrc20Addror deposit calls) for clarity and testability.
115-165: Reduce code duplication inProcessAbort.Similar to
ProcessRevert, you might reduce repeated logic by extracting shared steps (e.g., deposit calls, contract introspection). This will streamline maintenance and testing.x/fungible/keeper/deposits_test.go (2)
19-51: Return explicit typed value for index.
getTestDAppNoMessageIndexcurrently returns a string for the index. If this index is intended to be numeric in the contract, consider converging on a numeric type to better match the underlying data and avoid conversions.
120-272: Test structure is well-organized.The sub-tests in
TestKeeper_ProcessDepositare logically grouped and thoroughly test the deposit scenarios (no-call, message ignored, deposit with call). Suggest verifying edge cases, such as zero amounts or excessively large amounts, to bolster confidence in deposit flow reliability.x/crosschain/keeper/abort.go (2)
26-32: Clarify fallback for non-v2 CCTX and invalid abort requests.
This block precludes processing the abort if the CCTX isn't v2 or the abort has no address or refund has already occurred. To avoid confusion with legacy flows, consider logging or commenting on how older (v1) CCTXs should be handled (if at all).
136-136: Typographical fix in function comment.
The comment contains “Refund the the amount…” – please remove the duplicate “the” for clarity.- // Refund the the amount was previously + // Refund the amount previouslyx/crosschain/keeper/cctx_orchestrator_validate_outbound.go (4)
41-60: Ensure no error is blindly swallowed.
This logic sets the CCTX to Aborted or PendingRevert but never returns an error to the caller. Consider whether a caller-side indication is beneficial for telemetry or upstream error handling, or if silent state updates suffice.
87-95: Return an error if encountering an unexpected status.
Currently, this switch justreturns if the status isn't PendingRevert or PendingOutbound. Consider logging or faulting if a status is unexpected, to help trace potential state inconsistencies.
167-168: Expand the revert failure context.
The error handling message “revert failed to be processed on connected chain” might benefit from additional context (e.g., chain ID, cctx index). This can simplify debugging when multiple concurrent reverts fail.
185-190: Default case handling in ballot status.
The switch overobservertypes.BallotStatusonly covers Success/Failure but lacks a default branch. If new ballot states are introduced, the function might silently ignore them. Adding a default guard could improve robustness.e2e/e2etests/test_eth_withdraw_revert_and_abort.go (1)
17-61: Consider adding more test cases for edge scenarios.While the test is well-implemented, consider adding test cases for:
- Different revert message lengths
- Zero amount withdrawals
- Maximum gas limit scenarios
LGTM! Comprehensive test implementation.
The test thoroughly validates the abort workflow:
- Proper setup of test contract
- Verification of abort status
- Validation of token balances
e2e/e2etests/test_erc20_deposit_revert_and_abort.go (2)
30-31: Consider adding test cases for edge cases.The test uses a non-existing address for deposit, but it would be valuable to test other edge cases such as zero amount deposits or deposits to contracts that don't accept ERC20 tokens.
56-58: Add balance check before the deposit.To make the test more robust, consider checking the initial balance of the abort contract before the deposit to ensure the balance increase is exactly the deposited amount.
+ // check initial balance + initialBalance, err := r.ERC20ZRC20.BalanceOf(&bind.CallOpts{}, testAbortAddr) + require.NoError(r, err) + // check abort contract received the tokens balance, err := r.ERC20ZRC20.BalanceOf(&bind.CallOpts{}, testAbortAddr) require.NoError(r, err) - require.True(r, balance.Uint64() > 0) + require.Equal(r, amount.Uint64(), balance.Sub(balance, initialBalance).Uint64())e2e/e2etests/test_erc20_withdraw_revert_and_abort.go (2)
22-23: Consider consolidating token approvals.The test approves both ERC20ZRC20 and ETHZRC20, but only ERC20 withdrawal is being tested. Consider removing the unnecessary ETHZRC20 approval.
r.ApproveERC20ZRC20(r.GatewayZEVMAddr) - r.ApproveETHZRC20(r.GatewayZEVMAddr)
59-61: Enhance balance verification.Similar to the deposit test, consider checking the exact balance change instead of just verifying it's greater than zero.
+ // check initial balance + initialBalance, err := r.ERC20ZRC20.BalanceOf(&bind.CallOpts{}, testAbortAddr) + require.NoError(r, err) + // check abort contract received the tokens balance, err := r.ERC20ZRC20.BalanceOf(&bind.CallOpts{}, testAbortAddr) require.NoError(r, err) - require.True(r, balance.Uint64() > 0) + require.Equal(r, amount.Uint64(), balance.Sub(balance, initialBalance).Uint64())x/crosschain/keeper/cctx_gateway_zevm.go (1)
32-37: Consider adding error context to ProcessAbort.The error message could be more descriptive by including the actual fee amount that was insufficient.
c.crosschainKeeper.ProcessAbort(ctx, config.CCTX, types.StatusMessages{ StatusMessage: fmt.Sprintf( - "observation failed, reason %s", + "observation failed, reason %s, required fee: %s", types.InboundStatus_INSUFFICIENT_DEPOSITOR_FEE.String(), + config.CCTX.InboundParams.RequiredFee, ), })e2e/e2etests/test_eth_deposit_revert_and_abort.go (3)
62-63: Fix typo in comment.There's a typo in the comment for Test 2.
- // Test 2: no contract ofr abort + // Test 2: no contract for abort
67-78: Consider extracting common test parameters.The test parameters are duplicated between the two test cases. Consider extracting them into a helper function or test parameters struct.
type testParams struct { amount *big.Int revertAddress common.Address revertMessage []byte gasLimit *big.Int abortAddress common.Address } func (r *E2ERunner) performETHDepositTest(params testParams) { return r.ETHDepositAndCall( r.TestDAppV2ZEVMAddr, params.amount, params.revertMessage, gatewayevm.RevertOptions{ RevertAddress: params.revertAddress, CallOnRevert: true, RevertMessage: params.revertMessage, OnRevertGasLimit: params.gasLimit, AbortAddress: params.abortAddress, }, ) }
85-88: Enhance balance verification.Similar to previous suggestions, consider checking the exact balance change instead of just verifying it's greater than zero.
+ // check initial balance + initialBalance, err := r.ETHZRC20.BalanceOf(&bind.CallOpts{}, abortAddressNoContract) + require.NoError(r, err) + // check abort contract received the tokens balance, err = r.ETHZRC20.BalanceOf(&bind.CallOpts{}, abortAddressNoContract) require.NoError(r, err) - require.True(r, balance.Uint64() > 0) + require.Equal(r, amount.Uint64(), balance.Sub(balance, initialBalance).Uint64())x/crosschain/keeper/msg_server_vote_outbound_tx.go (1)
127-133: Clarify "Should not happen" comment.The comment "Should not happen" should be expanded to explain why this error condition is unexpected and what preconditions prevent it.
x/crosschain/types/cctx.go (1)
27-28: Consider enhancing the comment about additional chains.The comment could be more descriptive about why additional chains are empty and the implications.
- // Note: additional chains argument is empty, all ZetaChain IDs are hardcoded in the codebase. + // Note: additional chains argument is empty since all ZetaChain IDs are hardcoded in the codebase. + // This means we only need to check against the built-in ZetaChain IDs.x/fungible/keeper/deposits_legacy_test.go (2)
23-412: Enhance test assertions inTestKeeper_ZRC20DepositAndCallContract.Consider adding error message validation in failure test cases and more descriptive assertions in success cases. For example:
require.ErrorIs(t, err, types.ErrCallNonContract) +require.Contains(t, err.Error(), "cannot call contract with data to EOA") require.NoError(t, err) require.False(t, contractCall) +require.Equal(t, big.NewInt(42), balance, "expected balance to be 42 after deposit")
414-449: Add edge cases toTestKeeper_DepositCoinZeta.The test function could benefit from additional test cases:
- Test with maximum uint256 amount
- Test with invalid address format
- Test deposit with zero amount
x/crosschain/keeper/abort_test.go (1)
23-149: Enhance error message validation in abort tests.The test cases should validate error messages more thoroughly. For example:
require.Equal(t, types.CctxStatus_Aborted, cctx.CctxStatus.Status) +require.Contains(t, cctx.CctxStatus.ErrorMessageAbort, "process abort failed", "error message should indicate process abort failure")docs/openapi/openapi.swagger.yaml (1)
60438-60440: Enhance the OpenAPI documentation for error_message_abort.While the property definition is correct, consider enhancing it to match the documentation style of
error_message_revertand include additional OpenAPI metadata.Apply this diff to improve the documentation:
error_message_abort: type: string - title: error_message_abort carries information when aborting the CCTX fails + title: |- + error_message_abort carries information about the abort operation, + which is created when aborting the CCTX fails + description: Detailed error message explaining why the CCTX abort operation failed + example: "Failed to abort CCTX: insufficient gas" + nullable: true
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
e2e/contracts/testabort/TestAbort.binis excluded by!**/*.bingo.sumis excluded by!**/*.sumpkg/contracts/testdappv2/TestDAppV2.binis excluded by!**/*.bintypescript/zetachain/zetacore/crosschain/cross_chain_tx_pb.d.tsis excluded by!**/*_pb.d.tsx/crosschain/types/cross_chain_tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (44)
changelog.md(1 hunks)cmd/zetae2e/local/evm.go(2 hunks)docs/openapi/openapi.swagger.yaml(1 hunks)e2e/contracts/testabort/TestAbort.abi(1 hunks)e2e/contracts/testabort/TestAbort.go(1 hunks)e2e/contracts/testabort/TestAbort.json(1 hunks)e2e/contracts/testabort/TestAbort.sol(1 hunks)e2e/contracts/testabort/bindings.go(1 hunks)e2e/e2etests/e2etests.go(5 hunks)e2e/e2etests/test_erc20_deposit_revert_and_abort.go(1 hunks)e2e/e2etests/test_erc20_withdraw_revert_and_abort.go(1 hunks)e2e/e2etests/test_eth_deposit_revert_and_abort.go(1 hunks)e2e/e2etests/test_eth_withdraw_revert_and_abort.go(1 hunks)go.mod(1 hunks)pkg/contracts/testdappv2/TestDAppV2.go(1 hunks)pkg/contracts/testdappv2/TestDAppV2.json(1 hunks)pkg/contracts/testdappv2/TestDAppV2.sol(1 hunks)proto/zetachain/zetacore/crosschain/cross_chain_tx.proto(1 hunks)testutil/keeper/crosschain.go(1 hunks)testutil/keeper/mocks/crosschain/fungible.go(1 hunks)x/crosschain/keeper/abort.go(1 hunks)x/crosschain/keeper/abort_test.go(23 hunks)x/crosschain/keeper/cctx_gateway_observers.go(1 hunks)x/crosschain/keeper/cctx_gateway_zevm.go(2 hunks)x/crosschain/keeper/cctx_gateways.go(1 hunks)x/crosschain/keeper/cctx_orchestrator_validate_outbound.go(6 hunks)x/crosschain/keeper/cctx_orchestrator_validate_outbound_test.go(19 hunks)x/crosschain/keeper/msg_server_abort_stuck_cctx.go(1 hunks)x/crosschain/keeper/msg_server_refund_aborted_tx.go(1 hunks)x/crosschain/keeper/msg_server_vote_outbound_tx.go(2 hunks)x/crosschain/keeper/msg_server_vote_outbound_tx_test.go(0 hunks)x/crosschain/keeper/refund.go(0 hunks)x/crosschain/types/cctx.go(1 hunks)x/crosschain/types/cctx_test.go(1 hunks)x/crosschain/types/expected_keepers.go(2 hunks)x/crosschain/types/status.go(2 hunks)x/crosschain/types/status_test.go(1 hunks)x/fungible/keeper/deposits.go(1 hunks)x/fungible/keeper/deposits_legacy.go(1 hunks)x/fungible/keeper/deposits_legacy_test.go(1 hunks)x/fungible/keeper/deposits_test.go(1 hunks)x/fungible/keeper/evm_gateway.go(1 hunks)x/fungible/keeper/v2_deposits.go(0 hunks)x/fungible/keeper/v2_deposits_test.go(0 hunks)
💤 Files with no reviewable changes (4)
- x/crosschain/keeper/msg_server_vote_outbound_tx_test.go
- x/fungible/keeper/v2_deposits_test.go
- x/fungible/keeper/v2_deposits.go
- x/crosschain/keeper/refund.go
✅ Files skipped from review due to trivial changes (3)
- pkg/contracts/testdappv2/TestDAppV2.json
- pkg/contracts/testdappv2/TestDAppV2.go
- x/crosschain/keeper/cctx_gateways.go
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.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/evm.gox/crosschain/types/status_test.gox/crosschain/keeper/msg_server_abort_stuck_cctx.gox/crosschain/keeper/cctx_orchestrator_validate_outbound_test.gox/crosschain/types/status.gox/crosschain/keeper/msg_server_refund_aborted_tx.goe2e/contracts/testabort/bindings.gox/crosschain/types/cctx.gox/crosschain/keeper/cctx_gateway_zevm.gotestutil/keeper/crosschain.gox/fungible/keeper/evm_gateway.gox/crosschain/keeper/msg_server_vote_outbound_tx.goe2e/e2etests/test_eth_withdraw_revert_and_abort.goe2e/e2etests/e2etests.goe2e/e2etests/test_erc20_deposit_revert_and_abort.gox/crosschain/types/expected_keepers.goe2e/e2etests/test_eth_deposit_revert_and_abort.gox/crosschain/keeper/abort.gox/crosschain/types/cctx_test.gox/fungible/keeper/deposits_test.gotestutil/keeper/mocks/crosschain/fungible.goe2e/e2etests/test_erc20_withdraw_revert_and_abort.gox/crosschain/keeper/cctx_gateway_observers.gox/fungible/keeper/deposits_legacy.gox/crosschain/keeper/abort_test.gox/fungible/keeper/deposits_legacy_test.gox/fungible/keeper/deposits.gox/crosschain/keeper/cctx_orchestrator_validate_outbound.goe2e/contracts/testabort/TestAbort.go
`**/*.proto`: Review the Protobuf definitions, point out iss...
**/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.
proto/zetachain/zetacore/crosschain/cross_chain_tx.proto
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: start-upgrade-test / e2e
- GitHub Check: start-solana-test / e2e
- GitHub Check: start-legacy-test / e2e
- GitHub Check: start-e2e-test / e2e
- GitHub Check: build-and-test
- GitHub Check: Analyze (go)
🔇 Additional comments (34)
x/fungible/keeper/deposits_test.go (2)
53-61: Deployment tests look good.
No major issues observed. The checks for contract deployment correctness usingassertContractDeploymentare sufficient for ensuring the contract is created as expected.
63-118: Good test coverage and clear assertion flows.
assertTestDAppV2MessageAndAmountthoroughly checks the contract state by verifying both message and amount. The usage of require statements to validate outputs is appropriate.x/crosschain/keeper/abort.go (2)
44-46: Validate temporary context usage.
A temporary context is used to avoid partial commits if an error occurs during log processing. This is prudent, but ensure any subsequent logic (outside these lines) also follows the same pattern, lest the code inadvertently commits a partial update.
61-94: Double-check handling of non-revert contract errors.
IfevmTxResponseis non-nil and not marked as reverted, yeterris present, the code proceeds with committing state changes but setsErrorMessageAbort. Confirm this behavior is intentional to avoid partially committed abort flows when the contract call failed for reasons other than an on-chain revert.x/crosschain/keeper/cctx_orchestrator_validate_outbound.go (1)
222-235: Verify consistent error messages and resource cleanup.
When a V2 reversion fails, the code aborts the CCTX with a broad error message. Ensure that any partial allocations, locks, or ephemeral states in the halfway-lifecycle revert attempt are properly rolled back or invalidated outside the ephemeral context.e2e/contracts/testabort/TestAbort.go (1)
1-435: Auto-generated code looks consistent.
This generated code follows standard Go binding patterns for Ethereum contracts. No changes are recommended at this time.e2e/contracts/testabort/bindings.go (1)
1-4: LGTM! Good choice of EVM version.The generate directives are well-structured and use appropriate tools. The explicit use of Paris EVM version ensures compatibility with the current Ethereum mainnet.
e2e/contracts/testabort/TestAbort.sol (1)
4-38: LGTM! Well-structured contract implementation.The contract follows best practices:
- Secure use of keccak256 for mapping keys
- Clear separation of concerns between internal and external functions
- Proper event handling with payable fallback and receive functions
x/crosschain/keeper/msg_server_abort_stuck_cctx.go (1)
44-46: LGTM! Good refactoring to use ProcessAbort.The change from direct SetAbort to ProcessAbort improves the code by centralizing abort logic and maintaining consistency across the codebase.
x/crosschain/types/status.go (3)
12-12: LGTM! Addition of ErrorMessageAbort field.The new field enhances error handling capabilities for abort scenarios, maintaining consistency with existing error message fields.
15-18: LGTM! Method rename to SetAbortRefunded.The rename from
AbortRefundedtoSetAbortRefundedfollows better naming conventions for setter methods.
50-52: LGTM! ErrorMessageAbort handling.The implementation follows the same pattern as other error message handling, maintaining code consistency.
x/crosschain/keeper/cctx_gateway_observers.go (1)
78-82: LGTM! Enhanced abort handling.The change improves error handling by centralizing abort processing through
ProcessAbortand providing clear error messages.e2e/contracts/testabort/TestAbort.abi (1)
1-176: LGTM! Well-structured TestAbort contract ABI.The ABI provides comprehensive abort handling capabilities with:
- Clear view functions for abort status
- Structured AbortContext for detailed abort information
- Proper function modifiers (view, nonpayable, payable)
x/crosschain/keeper/msg_server_refund_aborted_tx.go (2)
68-68: Document the use of legacy refund method.The change to
LegacyRefundAbortedAmountOnZetaChainsuggests a transition period, but there's no documentation explaining why the legacy method is used or when it will be deprecated.Please add a comment explaining:
- Why the legacy method is being used
- When it will be deprecated
- What will replace it
75-75: LGTM! Updated to use SetAbortRefunded.The change aligns with the renamed setter method in the Status type.
cmd/zetae2e/local/evm.go (1)
60-60: LGTM! Test coverage enhanced for abort workflow.The new test cases for ETH and ERC20 deposit/withdraw abort scenarios are well-organized within their respective test groups.
Also applies to: 63-63, 84-84, 87-87
x/fungible/keeper/evm_gateway.go (2)
283-284: Track non-EVM chain sender implementation.The TODO comment indicates a potential issue with non-EVM chains that needs to be addressed.
Please track this issue at #3532 to ensure it's resolved.
259-305: LGTM! Well-structured abort execution implementation.The
CallExecuteAbortmethod follows the established pattern of other gateway contract interaction methods, with proper error handling and parameter validation.x/crosschain/types/status_test.go (1)
20-20: LGTM! Method name improved for clarity.The change from
AbortRefunded()toSetAbortRefunded()better reflects the method's purpose as a setter.x/crosschain/keeper/msg_server_vote_outbound_tx.go (1)
193-197: LGTM! Clear documentation of SaveOutbound responsibilities.The documentation clearly lists all the operations performed by the SaveOutbound method, making its responsibilities explicit.
pkg/contracts/testdappv2/TestDAppV2.sol (1)
170-170: LGTM! Important validation check added.The added validation prevents recursive revert scenarios by checking if the revert message itself is a revert message.
x/crosschain/types/expected_keepers.go (1)
154-175: LGTM! Well-structured interface methods for handling revert and abort scenarios.The new methods provide a clear separation of concerns between revert and abort processing, with comprehensive parameters and consistent types.
x/crosschain/types/cctx.go (1)
17-21: LGTM! Well-documented and clear method signature.The method provides clear documentation of its purpose and return values.
x/crosschain/types/cctx_test.go (1)
15-77: LGTM! Comprehensive test coverage.The test suite thoroughly covers:
- Error cases for nil parameters
- Edge cases for empty and nil outbound params
- Success cases for both outgoing and incoming transactions
x/crosschain/keeper/cctx_orchestrator_validate_outbound_test.go (1)
63-262: LGTM! Improved error handling in validation tests.The test cases now correctly validate that failed scenarios result in aborted status rather than errors, which aligns with the new error handling approach. The assertions are clear and verify both the status transition and error messages.
testutil/keeper/mocks/crosschain/fungible.go (2)
423-451: LGTM! New ProcessAbort mock method follows best practices.The implementation correctly handles return values and error cases, following the standard mock pattern.
453-481: LGTM! ProcessRevert mock method is properly implemented.The implementation maintains consistency with other mock methods in error handling and return value management.
e2e/e2etests/e2etests.go (2)
19-20: LGTM! Test constants follow naming conventions.The new test constants for abort scenarios maintain consistency with existing naming patterns.
Also applies to: 28-29, 37-38, 45-46
256-263: LGTM! Test registrations are well-structured.The new test registrations:
- Include clear descriptions
- Provide sensible default values
- Follow the existing pattern for test definitions
Also applies to: 320-327, 385-392, 441-448
proto/zetachain/zetacore/crosschain/cross_chain_tx.proto (1)
144-145: LGTM! Well-structured error handling enhancement.The addition of
error_message_abortfield is well-documented and follows proper protobuf field numbering conventions.e2e/contracts/testabort/TestAbort.json (1)
1-179: LGTM! Well-structured contract ABI for abort testing.The contract interface provides comprehensive functionality for testing abort scenarios with proper function visibility and state mutability specifications.
go.mod (1)
53-53:Details
❓ Verification inconclusive
Verify compatibility with the new protocol-contracts version.
Please ensure that the new version of protocol-contracts (v1.0.2-athens3.0.20250124151021-87b63e845f1c) is compatible with the abort workflow changes.
🌐 Web query:
What are the breaking changes in zeta-chain/protocol-contracts between version v1.0.2-athens3.0.20250115133723 and v1.0.2-athens3.0.20250124151021?💡 Result:
The breaking changes between these versions of
zeta-chain/protocol-contractsprimarily relate to cross-chain transaction handling and contract workflows, as inferred from recent protocol updates:
Key Breaking Changes
Universal Contract Calls from Bitcoin/Solana
- Now follow the Protocol Contract V2 workflow instead of legacy implementations[10].
- Requires updating interaction logic for:
depositAndCalltransactions- Direct contract calls via gateway methods
V2 Contracts Access Control
- Added stricter permissioning for critical functions like:
- Connector contract interactions[5]
- ZRC-20 token minting/burning
ZETA Token Handling
- Modified flow for native/non-native ZETA transfers across chains[5]:
// Previous function lock(address recipient) external payable; // Current (V2) function mint(address recipient, uint256 amount) external;Revert Mechanism Changes
- Updated
onRevertlogic requires explicit handling of:
- Cross-chain gas fee reversions[5]
- Contextual revert messages in failed withdrawals
Required Developer Actions
To migrate between these versions:
- Update all references to legacy Connector/TSS contracts with new V2 addresses[2][5].
- Refactor cross-chain calls involving Bitcoin/Solana to use updated gateway methods[10].
- Implement access control checks for protocol contract interactions[5].
For detailed migration steps, refer to ZetaChain's Athens-3 documentation[7][10].
Citations:
- 1: https://www.zetachain.com/docs/developers/tutorials/swap/
- 2: https://www.zetachain.com/blog/athens-testnet-and-beyond
- 3: https://github.com/zeta-chain/protocol-contracts
- 4: https://github.com/zeta-chain/zetachain/releases
- 5: https://github.com/zeta-chain/protocol-contracts/releases
- 6: https://www.zetachain.com
- 7: https://www.zetachain.com/docs/reference/network/contracts/
- 8: https://www.coinbase.com/developer-platform/discover/protocol-guides/guide-to-zetachain
- 9: https://www.zetachain.com/docs/reference/learn/faq/
- 10: https://newreleases.io/project/github/zeta-chain/node/release/v27.0.1
Compatibility Check: Ensure Alignment with Protocol-Contracts V2 Changes
After reviewing the recent protocol-contracts release notes, the new version (v1.0.2-athens3.0.20250124151021-87b63e845f1c) introduces breaking changes primarily in cross-chain transaction handling and contract workflows. Notably:
- Universal Contract Calls & Workflow Updates: Legacy interactions (e.g.,
depositAndCalland direct gateway calls) now follow the Protocol Contract V2 workflow.- Access Control Enhancements: Stricter permissions have been added for critical functions, which might require adjustments in our abort workflow.
- Token Handling Changes: The previous
lockfunction has been replaced bymintfor native/non-native ZETA transfers, necessitating a review of token management logic.- Revert Mechanism Overhaul: The updated
onReverthandling, especially for cross-chain gas fee reversions and contextual messaging, may impact our existing abort workflow.Action Items:
- Verify that any cross-chain transactions (including
depositAndCallcalls and direct gateway interactions) have been updated to reflect the new V2 workflow.- Confirm that all access control critical functions are properly integrated with the revised permissioning model.
- Check and, if necessary, refactor token transfer logic to align with the new
mintfunction.- Ensure that the abort workflow correctly processes the revised revert mechanisms.
Please assess these changes to ensure full compatibility with our abort workflow adjustments.
changelog.md (1)
7-8: LGTM! Well-documented changelog entries.The changes are properly documented with clear descriptions and PR references.
skosito
left a comment
skosito
left a comment
There was a problem hiding this comment.
looks good, just minor comments
Co-authored-by: Dmitry S <11892559+swift1337@users.noreply.github.com>
Co-authored-by: skosito <skostic9242@gmail.com>
Co-authored-by: skosito <skostic9242@gmail.com>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
| if coinType == coin.CoinType_ERC20 || coinType == coin.CoinType_Gas { | ||
| // simply deposit back to the revert address | ||
| // if the deposit fails, processing the abort entirely fails | ||
| // MsgRefundAbort can still be used to retry the operation later on | ||
| if _, err := k.DepositZRC20(ctx, zrc20Addr, abortAddress, amount); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
This comments seems incorrect. Keeper::ProcessAbort() sets cctx.CctxStatus.IsAbortRefunded = true
node/x/crosschain/keeper/abort.go
Lines 101 to 103 in 0a4acde
which is checked in the RefundAbortedCCTX message handler.
Similarly, if the getAndCheckZRC20() above errors, e.g., if the zrc20 is paused, the cctx will be marked as aborted without refunding or calling the contract.
There was a problem hiding this comment.
Yes comment is incorrect, it is expected to set IsAbortRefunded = true if the funds have been deposited even if the onAbort reverted
There was a problem hiding this comment.
Yes comment is incorrect, it is expected to set IsAbortRefunded = true if the funds have been deposited even if the onAbort reverted
That makes sense in case onAbort reverts.
But I was rather talking about DepositZRC20(..) erroring, or getAndCheckZRC20(..). In those situations, ProcessAbort(..) returns nil, err, which leads the caller to skip the if block in
node/x/crosschain/keeper/abort.go
Line 64 in f104014
and subsequently mark the cctx as refunded
node/x/crosschain/keeper/abort.go
Lines 98 to 103 in f104014
| types.ModuleAddressEVM, | ||
| gatewayAddr, | ||
| BigIntZero, | ||
| gatewayGasLimit, |
There was a problem hiding this comment.
Is the gas that is used for the abort call charged at any point to the user?
In general, is the gas used during the abort process charged? (e.g., DepositZRC20() call, ...)?
There was a problem hiding this comment.
It is inferred by the protocoll
There was a problem hiding this comment.
I'm not 100% sure about this.
Consider an outbound cctx, where ZetaChain is the sender and the receiver is some external chain E.
The cctx is initiated by calling withdrawAndCall(..) on the ZEVM gateway contract. It is possible to set callOptions.gasLimit to 1 (bypassing the zero check). This means that the gas fee that's paid is mainly consisting of the fixed protocol fee.
If the outbound cctx fails on the receiver chain E, a revert on ZetaChain is attempted.
If cctx.ProtocolContractVersion == types.ProtocolContractVersion_V2, it calls processFailedOutboundV2(..). It will then end up calling one of the revert functions on the gateway contract (e.g., executeRevert), with a fixed gas limit gatewayGasLimit. But IMO the user did not pay for this gas.
Additionally, should the revert call also error, it will abort the cctx
node/x/crosschain/keeper/cctx_orchestrator_validate_outbound.go
Lines 225 to 234 in f104014
Again, by using a fixed gas limit of gatewayGasLimit = big.NewInt(1_500_000)
This makes it seem that a user is able to severely underpay gas, which would be required to compensate for the revert + abort call, each consuming max. 1.5M gas, in total 3M gas.
Would be great if you or any other engineer could double-check this to make sure this is not prematurely discarded as a false positive, to be on the safe side.
There was a problem hiding this comment.
I meant it's fully paid by the protocol, like depositAndCall. The reasoning is that the fee to be paid on the external chain already limit spam but there are chains that are cheap to send transaction and it is true that it can be underpaid by the user.
I think with this added hook we should start to look more closely into introducing additional fees on the target chain, in particular if onRevert or onAbort are used
There was a problem hiding this comment.
Agree, the user should always cover for any costs caused, preferably a bit more than actually needed to have some safety buffer against exploitation.
Yes, both the onRevert and onAbort calls should be somehow paid for by the user.
| // do not commit anything here as the CCTX should be aborted, and use ctx for processing the abort | ||
| c.crosschainKeeper.ProcessAbort(ctx, config.CCTX, types.StatusMessages{ |
There was a problem hiding this comment.
Via this code path it seems possible to cause an (infinite) loop of aborts by withdrawing from the ZEVM within onAbort(..) with an outbound cctx that ends up in lines 77-84 in InitiateOutbound(), aborting the new cctx.
However, given that the ZEVM withdrawal requires a ZRC-20 gas payment I'm not sure how likely this is to happen. Also, lines 77-84 in InitiateOutbound() are not that easy to reach on purpose given that config.ShouldPayGas = false when processing ZEVM logs/events
node/x/crosschain/keeper/v2_zevm_inbound.go
Line 127 in 0a4acde
There was a problem hiding this comment.
Yes imo the gas payment to initiate a new cctx is already a protection against doing a loop of onAbort calls
There was a problem hiding this comment.
Is it possible that an outbound cctx, that is initiated on ZetaChain mainnet by a ZEVM gateway contract call (e.g. withdraw), results in ResolveCCTXGateway to resolve the gateway to the ZEVM gateway? E.g., having the zeta testnet, devnet or privnet as a receiver chain?
If so, this would maybe also make it possible to cause this infinite loop of aborts
There was a problem hiding this comment.
Is it possible that an outbound cctx, that is initiated on ZetaChain mainnet by a ZEVM gateway contract call (e.g. withdraw), results in ResolveCCTXGateway to resolve the gateway to the ZEVM gateway? E.g., having the zeta testnet, devnet or privnet as a receiver chain?
No it is not possible, the parsed withdraw events are routed to the observer gateway
4 participants
Description
Add onAbort support with E2E tests for:
Some refactoring in the cctx processing logic was necessary to support the feature
No asset call to be supported in a future PR
Some unit tests are missing in the fungible module, this represents files with already lacking unit testing, to be addressed in #3533
Closes #3168
Summary by CodeRabbit