fix: abort cctx on dust amount in revert outbound (Bitcoin and Solana) #3149
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 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several significant updates, including the addition of features for whitelisting SPL tokens on Solana and enhancements to the testing framework. Key modifications involve refactoring of existing code, such as removing the HSM signer from the zetaclient and improving error handling in outbound transaction processes. New test cases are added to better cover scenarios involving deposits and reverts, particularly focusing on dust amounts. The changelog has been updated to reflect these changes across multiple versions, detailing improvements in functionality and robustness. Changes
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
|
ws4charlie
requested review from
brewmaster012,
kingpinXD,
lumtis,
skosito,
swift1337 and
fbac
as code owners
ws4charlie
added
SOLANA_TESTS
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3149 +/- ##
========================================
Coverage 62.64% 62.64%
========================================
Files 424 424
Lines 30115 30124 +9
========================================
+ Hits 18866 18872 +6
- Misses 10408 10410 +2
- Partials 841 842 +1
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (13)
e2e/e2etests/test_solana_deposit_and_call_revert_with_dust.go (2)
14-15: Enhance test documentation for better clarityWhile the current comment describes the test's purpose, it could be more detailed to help other developers understand the test scenario better.
Consider expanding the documentation like this:
-// TestSolanaDepositAndCallRevertWithDust tests Solana deposit and call that reverts with a dust amount in the revert outbound. +// TestSolanaDepositAndCallRevertWithDust verifies that a Solana deposit and call transaction +// is properly aborted when the revert outbound would result in a dust amount. +// Test scenario: +// 1. Deposits exactly the rent-exempt amount +// 2. Attempts a call to a non-existent receiver +// 3. Verifies that the CCTX is aborted due to dust amount in revert
19-19: Consider using constants for test dataThe test uses string literals and direct values that could be defined as constants for better maintainability and reusability.
Consider defining test constants:
+const ( + testDepositDustMessage = "dust lamports should abort cctx" +) -depositAmount := big.NewInt(constant.SolanaWalletRentExempt) +depositAmount := new(big.Int).SetInt64(constant.SolanaWalletRentExempt) -data := []byte("dust lamports should abort cctx") +data := []byte(testDepositDustMessage)Also applies to: 24-25
e2e/e2etests/test_solana_deposit_refund.go (3)
12-13: Enhance function documentation for better clarity.Consider expanding the documentation to include:
- Expected parameters and their format
- Success criteria for the test
- Description of the revert scenario being tested
-// TestSolanaDepositAndCallRevert tests deposit of lamports calling a example contract that reverts. +// TestSolanaDepositAndCallRevert tests the handling of reverted deposits on Solana. +// Parameters: +// - args[0]: Deposit amount in lamports +// The test verifies that: +// - The deposit transaction is processed correctly +// - The CCTX status is set to Reverted +// - Appropriate error messages are present in the CCTX status
Line range hint
19-23: Address TODO: Optimize contract deployment strategy.The repeated deployment of the reverter contract in each test could be optimized by:
- Deploying once during test suite initialization
- Reusing the deployed contract across tests
Would you like me to help create a shared contract deployment mechanism or open an issue to track this optimization?
Line range hint
34-40: Refactor error handling for better maintainability.The current error checking logic checks two different fields for the error message. Consider:
- Standardizing error message location to a single field
- Creating a dedicated error validation helper
+func requireRevertError(r *runner.E2ERunner, cctx *crosschaintypes.CrossChainTx) { + msg := cctx.CctxStatus.ErrorMessage + if msg == "" { + msg = cctx.CctxStatus.StatusMessage + } + require.Contains(r, msg, "revert executed") +} + // Check the error carries the revert executed. -// tolerate the error in both the ErrorMessage field and the StatusMessage field -if cctx.CctxStatus.ErrorMessage != "" { - require.Contains(r, cctx.CctxStatus.ErrorMessage, "revert executed") -} else { - require.Contains(r, cctx.CctxStatus.StatusMessage, utils.ErrHashRevertFoo) -} +requireRevertError(r, cctx)e2e/utils/zetacore.go (1)
Line range hint
12-19: Consider documenting timeout increase rationaleThe timeout was increased from 6 to 8 minutes, but the comment still references an increase from 4 to 6 minutes. Consider updating the comment to reflect the current change and document why 8 minutes was chosen.
// The timeout was increased from 4 to 6 , which allows for a higher success in test runs +// Further increased to 8 minutes to accommodate longer processing times in dust amount scenarios // However this needs to be researched as to why the increase in timeout was needed. // https://github.com/zeta-chain/node/issues/2690cmd/zetae2e/local/local.go (1)
Line range hint
432-445: Consider reorganizing test cases for better maintainability.The Solana test cases could be organized into logical groups (e.g., basic operations, error cases, restricted operations) for better maintainability.
Consider refactoring the test organization:
- solanaTests := []string{ - e2etests.TestSolanaDepositName, - e2etests.TestSolanaWithdrawName, - e2etests.TestSolanaDepositAndCallName, - e2etests.TestSolanaDepositAndCallRevertName, - e2etests.TestSolanaDepositAndCallRevertWithDustName, - e2etests.TestSolanaDepositRestrictedName, - e2etests.TestSolanaWithdrawRestrictedName, - e2etests.TestSPLDepositName, - e2etests.TestSPLDepositAndCallName, - e2etests.TestSPLWithdrawName, - e2etests.TestSPLWithdrawAndCreateReceiverAtaName, - e2etests.TestSolanaWhitelistSPLName, - } + // Basic Solana operations + solanaBasicTests := []string{ + e2etests.TestSolanaDepositName, + e2etests.TestSolanaWithdrawName, + e2etests.TestSolanaDepositAndCallName, + } + + // Error handling cases + solanaErrorTests := []string{ + e2etests.TestSolanaDepositAndCallRevertName, + e2etests.TestSolanaDepositAndCallRevertWithDustName, + } + + // Restricted operations + solanaRestrictedTests := []string{ + e2etests.TestSolanaDepositRestrictedName, + e2etests.TestSolanaWithdrawRestrictedName, + } + + // SPL token operations + solanaTokenTests := []string{ + e2etests.TestSPLDepositName, + e2etests.TestSPLDepositAndCallName, + e2etests.TestSPLWithdrawName, + e2etests.TestSPLWithdrawAndCreateReceiverAtaName, + e2etests.TestSolanaWhitelistSPLName, + } + + solanaTests := append( + solanaBasicTests, + append( + solanaErrorTests, + append( + solanaRestrictedTests, + solanaTokenTests..., + )..., + )..., + )changelog.md (1)
27-27: Fix typographical error in changelog entry.The word "dected" appears to be misspelled.
-* [3149](https://github.com/zeta-chain/node/pull/3149) - abort the cctx if dust amount is dected in the revert outbound +* [3149](https://github.com/zeta-chain/node/pull/3149) - abort the cctx if dust amount is detected in the revert outboundx/crosschain/keeper/cctx_orchestrator_validate_outbound.go (5)
197-207: Enhance Error Context invalidateZRC20WithdrawalWhile the addition of
validateZRC20Withdrawalstrengthens the validation process for revert transactions, improving the error handling can provide clearer diagnostics. Consider wrapping the error with additional context.Apply the following diff to enhance error messages:
if err != nil { - return err + return fmt.Errorf("validation failed in validateZRC20Withdrawal: %w", err) }This modification ensures that any failure in the validation process is accompanied by a descriptive message, aiding in debugging and maintenance.
196-207: Avoid Redundant Validation CallsThe call to
validateZRC20Withdrawalimmediately after paying the gas fee may introduce redundancy if similar validations have already been performed earlier in the process. Evaluate whether this validation is necessary at this point or if it can be consolidated to prevent unnecessary overhead.If the validation is essential here, no action is needed. Otherwise, consider removing or consolidating it to streamline the code execution path.
Line range hint
289-294: Consolidate Logic inprocessFailedOutboundV2The TODO comment indicates an opportunity to consolidate logic with other functions, as referenced in issue #2627. Refactoring the code can enhance maintainability and reduce duplication.
Consider abstracting common functionalities into shared helper methods or interfaces. This approach will:
- Simplify the codebase by removing redundant code.
- Make future updates more manageable.
- Improve readability for other developers.
Line range hint
297-304: Handle Potential Inconsistency in Sender Chain ValidationThe sender chain validation checks if
cctx.InboundParams.SenderChainIdmatcheszetaChain.ChainId. If this condition fails, an error is returned. Ensure that this validation logic is consistent across all relevant functions to prevent unintended behaviors.Consider adding unit tests to verify that transactions with mismatched sender chain IDs are appropriately handled and that the error messages are clear and informative.
Line range hint
318-329: Check for Error Handling in ZEVM Revert ProcessingIn the call to
k.fungibleKeeper.ProcessV2RevertDeposit, errors are wrapped but not differentiated. To facilitate better debugging, specify the nature of potential errors.Modify the error wrapping to include more context:
if err := k.fungibleKeeper.ProcessV2RevertDeposit( // parameters ); err != nil { - return errors.Wrap(err, "failed ProcessV2RevertDeposit") + return fmt.Errorf("failed to process V2 revert deposit for CCTX %s: %w", cctx.Index, err) }This change provides clearer information about which transaction failed and why.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
changelog.md(1 hunks)cmd/zetae2e/local/local.go(1 hunks)e2e/e2etests/e2etests.go(2 hunks)e2e/e2etests/test_bitcoin_deposit_and_call_revert_with_dust.go(1 hunks)e2e/e2etests/test_solana_deposit_and_call_revert_with_dust.go(1 hunks)e2e/e2etests/test_solana_deposit_refund.go(1 hunks)e2e/utils/zetacore.go(1 hunks)x/crosschain/keeper/cctx_orchestrator_validate_outbound.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
cmd/zetae2e/local/local.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/e2etests.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_bitcoin_deposit_and_call_revert_with_dust.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_solana_deposit_and_call_revert_with_dust.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_solana_deposit_refund.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/utils/zetacore.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/keeper/cctx_orchestrator_validate_outbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (8)
e2e/e2etests/test_solana_deposit_and_call_revert_with_dust.go (1)
1-12: LGTM: Clean and well-organized imports
The imports are relevant and properly organized, with standard library imports separated from external dependencies.
e2e/e2etests/test_bitcoin_deposit_and_call_revert_with_dust.go (2)
39-40: LGTM! Clear and accurate variable naming.
The rename from badMemo to anyMemo better reflects the variable's purpose, as the memo itself isn't inherently bad but rather arbitrary. The memo construction is clear and follows good practices.
44-46: LGTM! Clean and direct test assertion.
The simplified test assertion using WaitCctxAbortedByInboundHash is a more elegant approach compared to the previous implementation. It directly verifies the expected outcome (CCTX abortion) without unnecessary intermediate checks.
e2e/utils/zetacore.go (1)
230-242: LGTM! The implementation is clean and follows established patterns.
The new function WaitCctxAbortedByInboundHash is well-implemented and consistent with the existing codebase patterns, particularly mirroring the structure of WaitCctxRevertedByInboundHash. The function correctly uses the MatchStatus option with CctxStatus_Aborted to wait for aborted transactions.
cmd/zetae2e/local/local.go (1)
436-437: LGTM! Test cases align with PR objectives.
The new test cases appropriately cover the dust amount handling in Solana transactions.
Let's verify the test implementations:
✅ Verification successful
Verified: Dust handling is adequately covered by the new test cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test implementations for the new Solana test cases
# Expected: Find the test implementations that handle dust amounts
# Search for the test implementations
ast-grep --pattern 'func TestSolanaDepositAndCallRevert($$$) {
$$$
}'
# Search for dust amount validation logic
rg -A 5 "validateZRC20Withdrawal.*dust"
Length of output: 2656
Script:
#!/bin/bash
# Description: Find the implementation of validateZRC20Withdrawal and check for dust handling
# Step 1: Locate the definition of validateZRC20Withdrawal
ast-grep --pattern 'func validateZRC20Withdrawal($$$) { $$$ }'
# Step 2: Search for dust-related logic in the codebase
rg -A 5 "dust" --glob '*.go'
Length of output: 8459
e2e/e2etests/e2etests.go (1)
58-68: LGTM! Well-structured Solana test definitions.
The Solana test constants follow the established naming convention and provide clear, descriptive names for each test case.
x/crosschain/keeper/cctx_orchestrator_validate_outbound.go (2)
Line range hint 185-190: Verify Gas Payment Sufficiency for Revert Transactions
The newly added code segment pays the gas fee for the revert outbound transaction using inputAmount. Please ensure that:
inputAmountaccurately reflects the required gas fee for the revert transaction.- There are sufficient funds to cover this gas payment to prevent potential failure of the revert operation.
Line range hint 331-339: Ensure Transaction Hash Assignment is Accurate
When assigning the transaction hash and observed external height:
- Confirm that
ctx.TxBytes()reliably returns the correct transaction bytes in all execution contexts. - Validate that the conversion to an Ethereum-compatible hash via
ethcommon.BytesToHashis appropriate.
Run the following script to check the consistency of transaction hashes:
gartnera
added
the
CONSENSUS_BREAKING_ACK
Co-authored-by: Tanmay <[email protected]>
Description
validateZRC20Withdrawalto abort the CCTX if dust amount is detected in revert outbound.TestBitcoinDepositAndCallRevertWithDustand add Solana E2E testTestSolanaDepositAndCallRevertWithDustCloses: #3135
How Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation