feat: revert restricted cctx for evm chain, bitcoin and solana#3906
feat: revert restricted cctx for evm chain, bitcoin and solana#3906ws4charlie merged 9 commits intodevelopfrom
Conversation
… won't lose their funds
|
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 change refactors and extends the handling of restricted cross-chain transactions (CCTX) and revert logic across EVM, Bitcoin, Solana, and Sui chains. It introduces explicit revert options and addresses in withdrawal flows, updates compliance checks, refines test argument structures, removes redundant helpers, and clarifies status handling for cancelled transactions. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as E2E Test
participant Runner as E2ERunner
participant Gateway as GatewayZEVM
participant BTC as Bitcoin Chain
participant ZetaCore as Zetacore
Test->>Runner: WithdrawBTCAndWaitCCTX(to, amount, revertOptions, status)
Runner->>Runner: WithdrawBTC(to, amount, revertOptions, approve)
Runner->>Gateway: Withdraw(to, amount, revertOptions)
Gateway-->>Runner: ETH Transaction
Runner->>BTC: Initiate BTC Withdrawal
BTC-->>ZetaCore: Outbound Transaction
ZetaCore-->>Runner: CCTX Status Update
Runner->>BTC: Fetch Outbound BTC Tx by Hash
BTC-->>Runner: Raw BTC Tx
Runner-->>Test: BTC Tx Result
Possibly related PRs
Suggested labels
Suggested reviewers
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
ws4charlie
added
chain:evm
chain:bitcoin
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3906 +/- ##
===========================================
+ Coverage 64.87% 64.98% +0.11%
===========================================
Files 470 470
Lines 34400 34343 -57
===========================================
+ Hits 22316 22318 +2
+ Misses 11051 10993 -58
+ Partials 1033 1032 -1
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
zetaclient/chains/bitcoin/signer/outbound_data_test.go (1)
186-188: Verify completeness of restricted CCTX test coverage.While the test case correctly sets
restrictedCCTX: truefor the restricted scenario, consider ensuring that test coverage is comprehensive for both restricted and non-restricted cases. The current test validates the cancellation behavior, but additional edge cases might be valuable.Consider adding test cases that verify:
- Behavior when
restrictedCCTXis false with valid amounts- Interaction between
restrictedCCTXand dust amount conditions- Error scenarios with restricted CCTXs
{ name: "should not cancel non-restricted CCTX with valid amount", cctx: sample.CrossChainTx(t, "0x123"), cctxModifier: func(cctx *crosschaintypes.CrossChainTx) { cctx.InboundParams.CoinType = coin.CoinType_Gas cctx.GetCurrentOutboundParam().Receiver = receiver.String() cctx.GetCurrentOutboundParam().ReceiverChainId = chain.ChainId cctx.GetCurrentOutboundParam().Amount = sdkmath.NewUint(1e7) cctx.GetCurrentOutboundParam().GasPrice = "8" cctx.GetCurrentOutboundParam().TssNonce = 1 }, height: 101, minRelayFee: 0.00001, restrictedCCTX: false, // explicitly test non-restricted case expected: &OutboundData{ to: receiver, amount: 0.1, amountSats: 10000000, feeRate: 8, nonce: 1, minRelayFee: 0.00001, height: 101, cancelTx: false, }, },zetaclient/chains/solana/signer/signer.go (2)
141-148: Well-organized variable declarations with centralized compliance logic.The grouped variable declarations improve readability and the centralized
cancelTxflag using!signer.PassesCompliance(cctx)effectively consolidates compliance checking.However, there's a duplicate
isRevertvariable declaration:var ( chainID = signer.Chain().ChainId nonce = params.TssNonce coinType = cctx.InboundParams.CoinType isRevert = (cctx.CctxStatus.Status == types.CctxStatus_PendingRevert && cctx.RevertOptions.CallOnRevert) cancelTx = !signer.PassesCompliance(cctx) outboundGetter outboundGetter )Note that
isRevertis redeclared at line 161. Consider removing the duplicate declaration.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 141-148: zetaclient/chains/solana/signer/signer.go#L141-L148
Added lines #L141 - L148 were not covered by tests
161-161: Remove duplicate isRevert variable declaration.The
isRevertvariable is already declared in the grouped variables section (line 145). This redeclaration is unnecessary and should be removed for clarity.- isRevert := (cctx.CctxStatus.Status == types.CctxStatus_PendingRevert && cctx.RevertOptions.CallOnRevert)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
changelog.md(1 hunks)e2e/e2etests/e2etests.go(4 hunks)e2e/e2etests/helpers.go(0 hunks)e2e/e2etests/legacy/test_erc20_multiple_withdraws.go(1 hunks)e2e/e2etests/legacy/test_erc20_withdraw.go(1 hunks)e2e/e2etests/test_bitcoin_withdraw_legacy.go(2 hunks)e2e/e2etests/test_bitcoin_withdraw_multiple.go(2 hunks)e2e/e2etests/test_bitcoin_withdraw_p2sh.go(2 hunks)e2e/e2etests/test_bitcoin_withdraw_p2wsh.go(2 hunks)e2e/e2etests/test_bitcoin_withdraw_rbf.go(3 hunks)e2e/e2etests/test_bitcoin_withdraw_restricted_address.go(1 hunks)e2e/e2etests/test_bitcoin_withdraw_segwit.go(2 hunks)e2e/e2etests/test_bitcoin_withdraw_taproot.go(2 hunks)e2e/e2etests/test_eth_withdraw_restricted_address.go(2 hunks)e2e/e2etests/test_solana_withdraw.go(2 hunks)e2e/e2etests/test_solana_withdraw_restricted_address.go(3 hunks)e2e/e2etests/test_solana_withdraw_revert_executable_receiver.go(2 hunks)e2e/e2etests/test_sui_withdraw_restricted_address.go(1 hunks)e2e/runner/bitcoin.go(3 hunks)e2e/runner/solana.go(2 hunks)e2e/runner/verify.go(2 hunks)testutil/sample/zetaclient.go(1 hunks)zetaclient/chains/base/signer.go(2 hunks)zetaclient/chains/bitcoin/observer/outbound.go(1 hunks)zetaclient/chains/bitcoin/signer/outbound_data.go(1 hunks)zetaclient/chains/bitcoin/signer/outbound_data_test.go(3 hunks)zetaclient/chains/bitcoin/signer/sign_rbf_test.go(1 hunks)zetaclient/chains/bitcoin/signer/sign_test.go(1 hunks)zetaclient/chains/bitcoin/signer/signer.go(1 hunks)zetaclient/chains/evm/observer/outbound.go(1 hunks)zetaclient/chains/evm/signer/signer.go(1 hunks)zetaclient/chains/solana/observer/outbound.go(1 hunks)zetaclient/chains/solana/signer/execute.go(1 hunks)zetaclient/chains/solana/signer/execute_spl.go(1 hunks)zetaclient/chains/solana/signer/signer.go(4 hunks)zetaclient/chains/solana/signer/withdraw.go(1 hunks)zetaclient/chains/solana/signer/withdraw_spl.go(1 hunks)zetaclient/chains/sui/signer/signer.go(1 hunks)
💤 Files with no reviewable changes (1)
- e2e/e2etests/helpers.go
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/legacy/test_erc20_multiple_withdraws.goe2e/e2etests/test_sui_withdraw_restricted_address.gozetaclient/chains/bitcoin/signer/sign_test.gozetaclient/chains/bitcoin/observer/outbound.gotestutil/sample/zetaclient.gozetaclient/chains/bitcoin/signer/outbound_data.gozetaclient/chains/base/signer.goe2e/e2etests/test_bitcoin_withdraw_taproot.gozetaclient/chains/evm/signer/signer.gozetaclient/chains/solana/signer/execute_spl.gozetaclient/chains/bitcoin/signer/sign_rbf_test.goe2e/e2etests/legacy/test_erc20_withdraw.goe2e/e2etests/test_bitcoin_withdraw_legacy.goe2e/e2etests/test_bitcoin_withdraw_p2sh.goe2e/e2etests/test_bitcoin_withdraw_p2wsh.gozetaclient/chains/bitcoin/signer/signer.goe2e/e2etests/test_solana_withdraw_restricted_address.gozetaclient/chains/sui/signer/signer.gozetaclient/chains/solana/observer/outbound.goe2e/runner/solana.goe2e/e2etests/test_bitcoin_withdraw_rbf.gozetaclient/chains/bitcoin/signer/outbound_data_test.gozetaclient/chains/solana/signer/signer.gozetaclient/chains/solana/signer/execute.goe2e/runner/bitcoin.goe2e/e2etests/test_bitcoin_withdraw_multiple.gozetaclient/chains/solana/signer/withdraw.goe2e/runner/verify.goe2e/e2etests/test_solana_withdraw.goe2e/e2etests/e2etests.goe2e/e2etests/test_bitcoin_withdraw_restricted_address.gozetaclient/chains/solana/signer/withdraw_spl.goe2e/e2etests/test_solana_withdraw_revert_executable_receiver.gozetaclient/chains/evm/observer/outbound.goe2e/e2etests/test_eth_withdraw_restricted_address.goe2e/e2etests/test_bitcoin_withdraw_segwit.go
🧬 Code Graph Analysis (12)
zetaclient/chains/bitcoin/signer/sign_test.go (1)
zetaclient/chains/bitcoin/signer/outbound_data.go (1)
NewOutboundData(53-145)
zetaclient/chains/bitcoin/observer/outbound.go (3)
zetaclient/compliance/compliance.go (1)
IsCCTXRestricted(13-18)zetaclient/chains/interfaces/interfaces.go (1)
ZetacoreClient(58-114)x/crosschain/types/message_vote_outbound.go (1)
NewMsgVoteOutbound(17-47)
zetaclient/chains/bitcoin/signer/sign_rbf_test.go (1)
zetaclient/chains/bitcoin/signer/outbound_data.go (1)
NewOutboundData(53-145)
zetaclient/chains/solana/observer/outbound.go (2)
zetaclient/compliance/compliance.go (1)
IsCCTXRestricted(13-18)e2e/runner/accounting.go (1)
Amount(27-30)
e2e/runner/solana.go (1)
pkg/contracts/solana/instruction.go (1)
RevertOptions(117-132)
zetaclient/chains/bitcoin/signer/outbound_data_test.go (1)
zetaclient/chains/bitcoin/signer/outbound_data.go (2)
OutboundData(19-50)NewOutboundData(53-145)
zetaclient/chains/solana/signer/signer.go (3)
zetaclient/context/chain.go (1)
Chain(31-38)testutil/sample/crosschain.go (2)
InboundParams(176-191)Status(248-260)pkg/contracts/solana/instruction.go (1)
RevertOptions(117-132)
e2e/runner/verify.go (1)
e2e/runner/runner.go (1)
E2ERunner(80-210)
e2e/e2etests/test_solana_withdraw.go (1)
pkg/contracts/solana/instruction.go (1)
RevertOptions(117-132)
e2e/e2etests/e2etests.go (1)
testutil/sample/zetaclient.go (4)
RevertAddressZEVM(15-15)RestrictedSuiAddressTest(14-14)RestrictedBtcAddressTest(12-12)RestrictedEVMAddressTest(11-11)
e2e/e2etests/test_solana_withdraw_revert_executable_receiver.go (1)
e2e/runner/solana.go (1)
ConnectedProgramID(24-24)
zetaclient/chains/evm/observer/outbound.go (2)
testutil/sample/crosschain.go (1)
InboundParams(176-191)zetaclient/compliance/compliance.go (1)
IsCCTXRestricted(13-18)
🪛 GitHub Check: codecov/patch
zetaclient/chains/bitcoin/observer/outbound.go
[warning] 168-179: zetaclient/chains/bitcoin/observer/outbound.go#L168-L179
Added lines #L168 - L179 were not covered by tests
[warning] 193-194: zetaclient/chains/bitcoin/observer/outbound.go#L193-L194
Added lines #L193 - L194 were not covered by tests
[warning] 197-197: zetaclient/chains/bitcoin/observer/outbound.go#L197
Added line #L197 was not covered by tests
zetaclient/chains/base/signer.go
[warning] 159-163: zetaclient/chains/base/signer.go#L159-L163
Added lines #L159 - L163 were not covered by tests
[warning] 165-178: zetaclient/chains/base/signer.go#L165-L178
Added lines #L165 - L178 were not covered by tests
zetaclient/chains/bitcoin/signer/signer.go
[warning] 130-131: zetaclient/chains/bitcoin/signer/signer.go#L130-L131
Added lines #L130 - L131 were not covered by tests
[warning] 133-133: zetaclient/chains/bitcoin/signer/signer.go#L133
Added line #L133 was not covered by tests
zetaclient/chains/solana/observer/outbound.go
[warning] 148-148: zetaclient/chains/solana/observer/outbound.go#L148
Added line #L148 was not covered by tests
zetaclient/chains/solana/signer/signer.go
[warning] 141-148: zetaclient/chains/solana/signer/signer.go#L141-L148
Added lines #L141 - L148 were not covered by tests
[warning] 163-163: zetaclient/chains/solana/signer/signer.go#L163
Added line #L163 was not covered by tests
[warning] 171-171: zetaclient/chains/solana/signer/signer.go#L171
Added line #L171 was not covered by tests
[warning] 182-182: zetaclient/chains/solana/signer/signer.go#L182
Added line #L182 was not covered by tests
[warning] 190-190: zetaclient/chains/solana/signer/signer.go#L190
Added line #L190 was not covered by tests
[warning] 199-199: zetaclient/chains/solana/signer/signer.go#L199
Added line #L199 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: start-solana-test / e2e
- GitHub Check: start-e2e-test / e2e
🔇 Additional comments (68)
testutil/sample/zetaclient.go (1)
15-15: LGTM: Well-formed test constant addition.The new
RevertAddressZEVMconstant follows established naming conventions and provides a properly formatted Ethereum address for test scenarios involving revert functionality.changelog.md (1)
31-31: LGTM: Proper changelog documentation.The changelog entry accurately documents the revert of restricted CCTX functionality and follows the established format and structure.
zetaclient/chains/bitcoin/signer/sign_test.go (1)
49-49: LGTM: Test signature correctly updated.The test helper function properly adapts to the new
NewOutboundDatasignature by providing the requiredrestrictedCCTXboolean parameter. Usingfalseis appropriate for non-restricted test scenarios.zetaclient/chains/bitcoin/signer/signer.go (1)
129-131: LGTM: Centralized compliance check implementation.The compliance check correctly utilizes the centralized
PassesCompliancemethod and properly negates the result to determine restricted status. This aligns with the PR objective of centralizing compliance logic across chain signers.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 130-131: zetaclient/chains/bitcoin/signer/signer.go#L130-L131
Added lines #L130 - L131 were not covered by testse2e/e2etests/legacy/test_erc20_multiple_withdraws.go (1)
87-87: LGTM! Verification method refactoring improves modularity.The change from
VerifyTransferAmountFromCCTXtoEVMVerifyOutboundTransferAmountwith explicit hash and amount parameters enhances code clarity and decouples verification logic from the full CCTX object structure.e2e/e2etests/legacy/test_erc20_withdraw.go (1)
32-32: LGTM! Consistent verification method refactoring.The change maintains consistency with the broader refactor pattern, replacing
VerifyTransferAmountFromCCTXwith the more explicitEVMVerifyOutboundTransferAmountmethod call.zetaclient/chains/evm/signer/signer.go (1)
343-343:Details
✅ Verification successful
Verify compliance failure logging is preserved.
The centralization of compliance checking to
signer.PassesCompliance(cctx)improves maintainability and consistency across chain signers. However, ensure that appropriate logging for compliance failures is maintained within thePassesCompliancemethod to preserve observability.
🏁 Script executed:
#!/bin/bash # Description: Verify that PassesCompliance method includes appropriate logging for compliance failures # Expected: Find logging statements within the PassesCompliance method implementation ast-grep --pattern $'func ($$$) PassesCompliance($$$) { $$$ }'Length of output: 65
🏁 Script executed:
#!/bin/bash # Locate the PassesCompliance method and inspect its implementation for logging # 1. Find the file and line where PassesCompliance is defined, with surrounding context echo "=== PassesCompliance definition ===" rg -n -C3 "func .*PassesCompliance" -R # 2. Capture the file path for the first occurrence file=$(rg -l "func .*PassesCompliance" -R | head -n1) if [ -z "$file" ]; then echo "Error: PassesCompliance definition not found." exit 1 fi # 3. Search within that file for any logging statements (log., logger., cctx.Logger) echo "=== Logging statements inside $file ===" rg -n -C2 "log\\.|logger\\.|cctx\\.Logger" "$file" || echo "No logging statements found in PassesCompliance."Length of output: 1115
🏁 Script executed:
#!/bin/bash # Locate PassesCompliance and verify that it emits logs on failure echo "=== Searching for PassesCompliance definitions ===" rg -n -C3 'func .*PassesCompliance' . # Capture the first occurrence file=$(rg -l 'func .*PassesCompliance' . | head -n1) if [[ -z "$file" ]]; then echo "Error: PassesCompliance definition not found." exit 1 fi echo "=== Inspecting $file for logging statements ===" rg -n -C2 'log\.|logger\.|cctx\.Logger' "$file" \ || echo "No logging statements found in PassesCompliance."Length of output: 2086
🏁 Script executed:
#!/bin/bash # Display extended context around PassesCompliance to verify logging echo "=== Extended context for PassesCompliance ===" rg -n -C10 'func (s .*PassesCompliance' zetaclient/chains/base/signer.goLength of output: 258
🏁 Script executed:
#!/bin/bash # Correctly display the full body of PassesCompliance to verify logging echo "=== Full PassesCompliance implementation ===" sed -n '150,200p' zetaclient/chains/base/signer.goLength of output: 1244
Compliance failure logging is preserved in PassesCompliance.
The centralized compliance check still emits logs on failure via
compliance.PrintComplianceLog, passing bothStdandComplianceloggers.•
zetaclient/chains/base/signer.go:159–167–PassesCompliancecallscompliance.PrintComplianceLog( s.Logger().Std, s.Logger().Compliance, true, s.Chain().ChainId, cctx.Index, cctx.InboundParams.Sender, params.Receiver, params.CoinType.String(), )to record restricted transactions.
No further changes required.
e2e/e2etests/test_bitcoin_withdraw_legacy.go (2)
4-4: LGTM! Necessary imports for enhanced withdrawal method.The added imports support the new explicit withdrawal method call with revert options and CCTX status handling.
Also applies to: 8-8, 12-12
25-30: Excellent refactoring to explicit withdrawal method.The replacement of the helper function with
WithdrawBTCAndWaitCCTXsignificantly improves test clarity and robustness by:
- Making revert options explicit with
gatewayzevm.RevertOptions{OnRevertGasLimit: big.NewInt(0)}- Specifying expected CCTX status (
crosschaintypes.CctxStatus_OutboundMined)- Eliminating hidden behavior from the removed helper function
This change enhances test maintainability and makes the expected behavior more transparent.
zetaclient/chains/bitcoin/signer/sign_rbf_test.go (1)
240-240: Test parameter update aligns with API refactor.The update correctly passes
falsefor the newrestrictedCCTXparameter, indicating this test case handles non-restricted transactions. This change properly aligns with theNewOutboundDatasignature refactor that externalized compliance checks.zetaclient/chains/solana/observer/outbound.go (1)
143-149: Correctly implements revert behavior for restricted CCTXs.The implementation properly handles cancelled transactions by:
- Setting the outbound amount to the CCTX amount to bypass zetacore's amount validation
- Explicitly setting the status to
failedto trigger the revert process in zetacore- Adding clear documentation explaining the dual-purpose logic
This change successfully unifies the behavior with SUI chain handling and aligns with the PR objective to enable refunds for restricted transactions.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 148-148: zetaclient/chains/solana/observer/outbound.go#L148
Added line #L148 was not covered by testse2e/e2etests/test_bitcoin_withdraw_p2wsh.go (2)
4-4: Import additions support enhanced test functionality.The new imports correctly support the updated test pattern:
math/bigforbig.NewInt(0)in RevertOptionsgatewayzevm.solfor RevertOptions structcrosschaintypesfor CctxStatus enumAlso applies to: 8-8, 12-12
24-29: Test refactor enhances clarity and control.The replacement of the helper function with direct method invocation improves the test by:
- Explicitly specifying revert options (
OnRevertGasLimit: big.NewInt(0))- Clearly stating the expected transaction status (
CctxStatus_OutboundMined)- Providing better test readability and maintainability
This pattern aligns with the broader test refactor across Bitcoin withdrawal tests and supports the enhanced revert handling functionality.
e2e/e2etests/test_solana_withdraw_revert_executable_receiver.go (2)
9-9: Import addition aligns with new revert options functionality.The import of
gatewayzevm.solpackage is correctly added to support the new revert options parameter in the withdrawal call.
40-45:Details
✅ Verification successful
Verify OnRevertGasLimit value is appropriate for test scenario.
The addition of explicit revert options to the withdrawal call is well-aligned with the PR objective to unify revert behavior across chains. However, please confirm that setting
OnRevertGasLimitto zero is the intended behavior for this test case, as this could affect gas handling during revert operations.
🏁 Script executed:
#!/bin/bash # Description: Verify how OnRevertGasLimit is used in other test files to ensure consistency # Search for other uses of OnRevertGasLimit in test files rg "OnRevertGasLimit" --type go -A 2 -B 2Length of output: 33589
🏁 Script executed:
#!/bin/bash # Search for WithdrawSOLZRC20 definition in e2e/runner rg "func .*WithdrawSOLZRC20" -n e2e/runnerLength of output: 107
🏁 Script executed:
#!/bin/bash # Display the context of the revert executable receiver test sed -n '1,200p' e2e/e2etests/test_solana_withdraw_revert_executable_receiver.goLength of output: 1739
Confirmed:
OnRevertGasLimit=0 is correct and consistentVerified that in Solana withdraw tests (including
test_solana_withdraw_restricted_address.go) and most EVM/SPL withdraw tests without an on-revert hook,OnRevertGasLimitis explicitly set tobig.NewInt(0). Only tests invoking a call-on-revert or abort logic use non-zero limits (e.g. 200 000, 500 000). Keeping zero here aligns with the intended revert-only behavior—no changes required.e2e/e2etests/test_bitcoin_withdraw_multiple.go (2)
4-5: Import additions support the refactored withdrawal method.The new imports for
math/big,gatewayzevm.sol, andcrosschaintypesare correctly added to support the enhanced withdrawal method signature with revert options and status parameters.Also applies to: 7-7, 12-12
32-37: Refactor from helper function to direct runner method improves encapsulation.The replacement of
withdrawBTCZRC20helper function withWithdrawBTCAndWaitCCTXmethod call is a positive change that centralizes the withdrawal logic. The addition of explicit revert options and expected CCTX status parameters enhances test reliability and aligns with the PR's objective to unify revert behavior across chains.Note: The consistent use of
OnRevertGasLimit: big.NewInt(0)across test files suggests this is intentional for testing scenarios.e2e/e2etests/test_bitcoin_withdraw_taproot.go (2)
4-5: Import additions support enhanced withdrawal functionality.The import additions are consistent with the refactoring pattern across Bitcoin withdrawal tests and properly support the new method signature requirements.
Also applies to: 8-8, 12-12
24-29: Consistent refactoring maintains test integrity while adding revert support.The replacement of
withdrawBTCZRC20withWithdrawBTCAndWaitCCTXfollows the same pattern as other Bitcoin withdrawal tests, ensuring consistency across the test suite. The taproot-specific address validation logic is preserved, and the addition of revert options supports the unified revert behavior objective.e2e/e2etests/test_solana_withdraw.go (2)
9-9: Import addition supports revert options functionality.The addition of
gatewayzevm.solimport is correctly placed and necessary for the revert options parameter in the withdrawal call.
42-47: Withdrawal method updated with explicit revert options.The addition of
gatewayzevm.RevertOptions{OnRevertGasLimit: big.NewInt(0)}parameter to theWithdrawSOLZRC20call is consistent with the pattern across other Solana withdrawal tests and supports the PR's objective to unify revert behavior across chains. The test logic remains sound and continues to verify successful withdrawal scenarios.zetaclient/chains/solana/signer/execute.go (1)
18-24: Well-structured refactor for compliance check externalization.The method signature change to accept an explicit
cancelTxparameter effectively separates compliance concerns from transaction preparation logic. This approach improves modularity and makes the cancellation behavior explicit rather than buried within the method implementation.e2e/e2etests/test_bitcoin_withdraw_p2sh.go (2)
3-12: Proper imports for enhanced withdrawal testing.The updated imports correctly support the new withdrawal method signature with
math/bigfor amount handling,gatewayzevmfor revert options, andcrosschaintypesfor transaction status verification.
25-30: Excellent refactor to explicit withdrawal method.Replacing the helper function with a direct call to
WithdrawBTCAndWaitCCTXprovides several benefits:
- Explicit specification of revert options rather than hidden defaults
- Clear expectation of the final transaction status (
CctxStatus_OutboundMined)- Improved test readability and maintainability
- Alignment with the broader pattern across Bitcoin withdrawal tests
e2e/e2etests/test_sui_withdraw_restricted_address.go (2)
7-7: Appropriate import for address handling.The addition of
ethcommonimport correctly supports the conversion of the revert address parameter to Ethereum common address type.
19-25: Excellent parameterization enhancement.The updated test signature and argument parsing significantly improves test flexibility:
- Accepts receiver address, withdrawal amount, and revert address as parameters
- Eliminates hardcoded values, making the test more reusable
- Properly converts the revert address to the required type using
ethcommon.HexToAddress- Aligns with the consistent pattern across restricted withdrawal tests
This change enhances test maintainability and allows for more comprehensive test coverage scenarios.
e2e/runner/solana.go (2)
580-580: Enhanced API flexibility with revert options parameter.The addition of the
revertOptionsparameter to theWithdrawSOLZRC20method signature provides excellent configurability for withdrawal behavior, allowing callers to specify custom revert handling rather than relying on hardcoded values.
594-594: Proper utilization of configurable revert options.The method correctly passes the
revertOptionsparameter to the underlyingGatewayZEVM.Withdrawcall, ensuring that the caller's specified revert behavior is properly applied. This change aligns perfectly with the broader pattern of making revert options configurable across all chain withdrawal methods.zetaclient/chains/bitcoin/signer/outbound_data_test.go (2)
31-38: Test structure properly extended for new parameter.The addition of the
restrictedCCTXfield to the test struct correctly supports the updatedNewOutboundDatafunction signature. The test structure is well-organized and maintains consistency.
233-233: Function call correctly updated for new signature.The
NewOutboundDatacall properly includes the newrestrictedCCTXparameter, maintaining consistency with the updated function signature.e2e/e2etests/test_bitcoin_withdraw_segwit.go (2)
4-12: Imports properly added for new functionality.The addition of
math/big,gatewayzevm.sol, andcrosschaintypesimports correctly supports the new method signature and revert options functionality.
24-29: Direct method call improves explicitness and aligns with revert functionality.The replacement of the
withdrawBTCZRC20helper with a direct call toWithdrawBTCAndWaitCCTXenhances code clarity by making revert options and expected status explicit. The parameters are appropriate for the test scenario:
gatewayzevm.RevertOptions{OnRevertGasLimit: big.NewInt(0)}indicates no gas limit for revert operationscrosschaintypes.CctxStatus_OutboundMinedexpects successful miningHowever, consider adding a comment explaining the revert options parameters for future maintainability:
r.WithdrawBTCAndWaitCCTX( receiver, amount, // No gas limit for revert operations in this test scenario gatewayzevm.RevertOptions{OnRevertGasLimit: big.NewInt(0)}, crosschaintypes.CctxStatus_OutboundMined, )zetaclient/chains/solana/signer/withdraw.go (1)
18-24: Method signature improvement centralizes compliance responsibility.The addition of the
cancelTx boolparameter and removal of internal compliance checking represents a good separation of concerns. This change follows the single responsibility principle by making the method focused solely on transaction preparation while delegating compliance decisions to the caller.The method name and parameter clearly communicate the intent, and the implementation correctly uses the
cancelTxflag in thecreateMsgWithdrawcall on line 28.zetaclient/chains/sui/signer/signer.go (1)
111-111: Method call correctly updated to use centralized compliance logic.The change from
s.passesCompliance(cctx)tos.PassesCompliance(cctx)properly utilizes the centralized compliance checking mechanism. This standardization across chain signers improves code maintainability and ensures consistent compliance behavior.The capitalized method name follows Go conventions for public methods and indicates the compliance logic is now accessible across different signer implementations, promoting code reuse and consistency.
e2e/e2etests/test_solana_withdraw_restricted_address.go (5)
21-21: LGTM: Argument count correctly updated.The test now properly expects 3 arguments to support the new revert address functionality.
23-37: Well-structured test setup with clear variable naming.The test correctly parses the receiver address, withdrawal amount, and revert address. The validation logic for withdrawal amount against approved amount is appropriate.
39-44: Proper balance tracking implementation.The test correctly captures both receiver and revert address balances before the transaction. Using
rpc.CommitmentFinalizedensures consistent balance reads.
47-55: Excellent use of RevertOptions for restricted withdrawal testing.The withdrawal call properly includes revert options with zero gas limit, which is appropriate for testing scenarios. The structure aligns well with the broader revert functionality implementation.
57-74: Comprehensive verification of revert behavior.The test correctly verifies that:
- The CCTX status is reverted
- The outbound transaction is cancelled with zero value
- The receiver balance remains unchanged
- The revert address receives the withdrawn amount
This provides excellent coverage of the revert functionality.
e2e/e2etests/test_bitcoin_withdraw_rbf.go (3)
4-4: Appropriate imports for the new RevertOptions functionality.The addition of
math/bigandgatewayzevmimports aligns with the updated withdrawal method signature.Also applies to: 10-10
35-36: Correctly updated withdrawal method call.The change from
WithdrawBTCto includeRevertOptions{OnRevertGasLimit: big.NewInt(0)}and the boolean flag properly aligns with the new API. The return type change from receipt to transaction is handled correctly.
61-61: Consistent hash access pattern.The update to use
tx.Hash().Hex()instead ofreceipt.TxHash.Hex()is consistent with the return type change from the withdrawal method.zetaclient/chains/solana/signer/execute_spl.go (2)
22-22: Good parameter addition for centralized compliance handling.Adding the
cancelTx boolparameter effectively communicates the compliance decision made externally, eliminating the need for internal compliance checks and reducing code duplication.
28-28: Consistent parameter usage in method delegation.The
cancelTxparameter is correctly passed tocreateMsgExecuteSPL, maintaining the flow of compliance decisions through the call chain.zetaclient/chains/solana/signer/signer.go (1)
163-163: Consistent parameter updates across all transaction preparation methods.All method calls to
prepareExecuteTx,prepareWithdrawTx,prepareExecuteSPLTx, andprepareWithdrawSPLTxare correctly updated to include thecancelTxparameter, ensuring uniform compliance handling across different transaction types.Also applies to: 171-171, 182-182, 190-190, 199-199
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 163-163: zetaclient/chains/solana/signer/signer.go#L163
Added line #L163 was not covered by testszetaclient/chains/evm/observer/outbound.go (2)
185-189: Well-organized variable declarations.The grouping of variable declarations in a single
varblock improves code readability and follows Go best practices for related variable declarations.
191-199: Correct implementation of restricted CCTX handling.The logic properly handles restricted transactions by:
- Setting
receiveValueto the CCTX amount to bypass amount validation in zetacore- Setting
receiveStatusto failed to trigger the revert process- Posting the vote immediately and returning to prevent further processing
This ensures consistent revert behavior for compliance-restricted transactions.
zetaclient/chains/bitcoin/observer/outbound.go (3)
168-173: Improved variable organization and coin type handling.The grouped variable declarations enhance readability, and using
cctx.InboundParams.CoinTypeinstead of a hardcoded value ensures the correct coin type is propagated in the vote message.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 168-179: zetaclient/chains/bitcoin/observer/outbound.go#L168-L179
Added lines #L168 - L179 were not covered by tests
175-179: Consistent restricted CCTX handling across chains.The implementation correctly handles restricted transactions by setting
receiveStatusto failed, which will trigger the revert process in zetacore. This maintains consistency with the EVM observer implementation.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 168-179: zetaclient/chains/bitcoin/observer/outbound.go#L168-L179
Added lines #L168 - L179 were not covered by tests
193-194: Dynamic parameter usage improves flexibility.Using the dynamically determined
receiveValue,receiveStatus, andcointypevariables instead of hardcoded values makes the vote message construction more flexible and accurate.Also applies to: 197-197
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 193-194: zetaclient/chains/bitcoin/observer/outbound.go#L193-L194
Added lines #L193 - L194 were not covered by testszetaclient/chains/bitcoin/signer/outbound_data.go (1)
57-58: Excellent separation of concerns in function signature.The refactored signature improves the function by:
- Removing internal compliance checking and delegating it to the caller
- Reducing parameter complexity by removing the compliance logger
- Making the function more testable and focused on data preparation
This follows the single responsibility principle and improves maintainability.
e2e/e2etests/test_eth_withdraw_restricted_address.go (3)
18-30: Well-structured test setup with comprehensive balance tracking.The test properly initializes all required parameters and captures baseline balances for both receiver and revert addresses, enabling accurate verification of the revert behavior.
37-44: Correct usage of revert options for restricted withdrawals.The withdrawal call properly specifies the revert address and zero gas limit on revert, which aligns with the expected behavior for restricted transactions.
54-72: Comprehensive assertions verify complete revert behavior.The test thoroughly validates:
- CCTX status is correctly set to
Reverted- Outbound transfer amount is zero (cancelled transaction)
- Receiver balance remains unchanged (funds not delivered)
- Revert address receives the full refunded amount
This ensures the restricted address handling works as expected.
e2e/e2etests/test_bitcoin_withdraw_restricted_address.go (3)
18-27: Well-structured test setup with proper argument validation.The test function correctly validates the number of arguments and properly parses the restricted address, amount, and revert address parameters.
34-43: Correct implementation of withdrawal with revert options.The test properly executes the withdrawal with revert options and expects the transaction to be reverted, which aligns with the PR's objective of implementing revert mechanisms for restricted CCTXs.
46-52: Comprehensive verification of cancelled transaction structure.The test correctly verifies that cancelled transactions have only 2 outputs (nonce and change) instead of 3, and confirms no funds were sent to the restricted address.
zetaclient/chains/solana/signer/withdraw_spl.go (2)
17-35: Clean separation of compliance decision from transaction preparation.The refactor correctly externalizes the compliance check by accepting a
cancelTxparameter, improving modularity and testability of the withdrawal preparation logic.
65-68: Proper handling of cancelled transactions.The logic correctly zeros out the withdrawal amount for cancelled transactions while maintaining a valid transaction structure, ensuring proper revert handling for restricted addresses.
e2e/runner/verify.go (2)
12-27: Improved modularity in EVM transfer verification.The refactor enhances reusability by accepting transaction hash and amount directly, eliminating the dependency on the CCTX type.
29-52: Consistent refactoring of Solana withdrawal verification.The method follows the same pattern as the EVM verification refactor, improving consistency and modularity across chain-specific verification methods.
e2e/runner/bitcoin.go (3)
188-224: Proper integration with gateway contract for Bitcoin withdrawals.The refactor correctly implements withdrawal through the gateway contract with revert options support, following the standard pattern of approving the gateway address rather than direct contract interaction.
226-242: Comprehensive withdrawal and verification workflow.The new method provides a complete testing workflow that handles withdrawal, block mining for local networks, and CCTX status verification, improving test reliability and maintainability.
244-273: Correct handling of outbound hash retrieval for different CCTX statuses.The implementation properly distinguishes between reverted and normal CCTX statuses when retrieving the outbound hash. For reverted transactions, it correctly retrieves the cancel transaction hash from the first outbound parameter. The detailed transaction logging provides excellent debugging capabilities.
e2e/e2etests/e2etests.go (4)
754-754: LGTM: Proper revert address integration for Solana restricted withdrawals.The addition of the revert address argument with the appropriate default value correctly implements the revert mechanism for restricted CCTX scenarios on Solana chain.
992-994: LGTM: Consistent argument structure for Sui restricted withdrawals.The revert address argument addition maintains consistency with other chain implementations and uses the appropriate sample addresses for testing restricted withdrawal scenarios.
1175-1177: LGTM: Complete argument structure for Bitcoin restricted withdrawals.The addition of both receiver and revert address arguments properly aligns the Bitcoin restricted withdrawal test with the unified revert mechanism implementation across all supported chains.
1638-1640: LGTM: Unified revert mechanism implementation for Ether restricted withdrawals.The argument additions complete the consistent implementation of revert mechanisms across EVM, Bitcoin, Solana, and Sui chains, properly addressing the PR objectives for handling restricted cross-chain transactions.
lumtis
left a comment
lumtis
left a comment
There was a problem hiding this comment.
Thanks for the PR, though we need to check if it's not the normal behavior for Eth and co and we should change it to Sui
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
ws4charlie
added
the
CONSENSUS_BREAKING_ACK
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
3 participants
Description
The
EVM, Bitcoin, and Solanazetaclients will vote withReceiveStatus_successfor restricted (censored) outbounds. This results in the user losing funds as they will not get the funds refunded on ZetaChain due to the revert process not being triggered in this case.The
SUIzetaclient behaves differently and will vote withReceiveStatus_failed, thus refunding the funds on ZetaChain. This is a better way to deal with restricted CCTX in ZetaChain.This PR
EVM, Bitcoin, Solana and SUIchains by reverting restricted CCTXs.PassesComplianceto base signer struct, so it can be reused by all chains.How Has This Been Tested?