fix: remove minimum rent exempt check for SPL token withdrawals #3374
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 modifications to the SPL token withdrawal validation process across multiple files. The changes primarily focus on enhancing the validation logic for cross-chain transactions by incorporating a new coin type parameter. The modifications affect the withdrawal validation mechanism, specifically removing the minimum rent exempt check for SPL token withdrawals and updating the validation methods to support more granular coin-type specific checks. Changes
Sequence DiagramsequenceDiagram
participant Validator
participant WithdrawalMethod
participant CoinTypeChecker
Validator->>WithdrawalMethod: Initiate Withdrawal
WithdrawalMethod->>CoinTypeChecker: Validate Coin Type
CoinTypeChecker-->>WithdrawalMethod: Validation Result
WithdrawalMethod-->>Validator: Withdrawal 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
|
ws4charlie
added
chain:solana
SOLANA_TESTS
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3374 +/- ##
========================================
Coverage 63.22% 63.22%
========================================
Files 436 436
Lines 30636 30639 +3
========================================
+ Hits 19369 19372 +3
Misses 10432 10432
Partials 835 835
|
Yeah. I'm modifying that and running the E2E test in local. It's now reduced in 594449e |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
x/crosschain/keeper/evm_hooks_test.go (1)
175-187: Test case needs additional edge cases for SOL withdrawals.While the test covers the basic rent exempt case, consider adding tests for:
- Values slightly below and above the rent exempt amount
- Maximum value limits
- Zero value transfers
t.Run("successfully validate a valid SOL withdrawal event", func(t *testing.T) { k, ctx, _, _ := keepertest.CrosschainKeeper(t) // Test cases + testCases := []struct { + name string + value *big.Int + coinType coin.CoinType + expectErr bool + }{ + {"exactly rent exempt", big.NewInt(constant.SolanaWalletRentExempt), coin.CoinType_Gas, false}, + {"slightly below rent exempt", big.NewInt(constant.SolanaWalletRentExempt - 1), coin.CoinType_Gas, true}, + {"slightly above rent exempt", big.NewInt(constant.SolanaWalletRentExempt + 1), coin.CoinType_Gas, false}, + {"zero value", big.NewInt(0), coin.CoinType_Gas, true}, + {"max uint64", new(big.Int).SetUint64(math.MaxUint64), coin.CoinType_Gas, false}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + chainID := chains.SolanaMainnet.ChainId + to := []byte(sample.SolanaAddress(t)) + solWithdrawalEvent := sample.ZRC20Withdrawal(to, tc.value) + + err := k.ValidateZRC20WithdrawEvent(ctx, solWithdrawalEvent, chainID, tc.coinType) + if tc.expectErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
changelog.md(1 hunks)e2e/e2etests/e2etests.go(1 hunks)x/crosschain/keeper/cctx_orchestrator_validate_outbound.go(1 hunks)x/crosschain/keeper/evm_hooks.go(3 hunks)x/crosschain/keeper/evm_hooks_test.go(6 hunks)x/crosschain/keeper/v2_zevm_inbound.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
x/crosschain/keeper/v2_zevm_inbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/keeper/evm_hooks.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.
x/crosschain/keeper/evm_hooks_test.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.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: start-solana-test / e2e
- GitHub Check: start-e2e-test / e2e
🔇 Additional comments (7)
x/crosschain/keeper/v2_zevm_inbound.go (1)
78-78: LGTM! Correctly passes coin type for withdrawal validation.The change properly integrates the new coin type parameter into the withdrawal validation flow.
x/crosschain/keeper/evm_hooks.go (2)
308-315: LGTM! Function signature updated correctly.The ValidateZRC20WithdrawEvent function properly integrates the new coinType parameter.
Line range hint
320-358: Verify the security implications of removing rent exempt check for non-gas tokens.The validation logic now only enforces the rent exempt check for SOL gas token withdrawals. While this aligns with the PR objective, we should verify:
- That SPL token withdrawals inherently require sufficient SOL for ATA creation
- That removing this check doesn't introduce security vulnerabilities
Run this script to check for any existing security measures or validations for SPL token withdrawals:
✅ Verification successful
Removal of rent exempt check for SPL tokens is architecturally sound
The validation now correctly enforces rent exempt check only for SOL withdrawals, which is the proper architectural approach since:
- SOL minimum is critical for wallet creation
- ATA requirements for SPL tokens are enforced at the Solana runtime level
- SPL token amounts don't affect rent calculations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for SPL token withdrawal validations and ATA creation requirements rg -A 5 "Associated Token Account|ATA creation|rent exempt|minimum balance"Length of output: 3849
x/crosschain/keeper/cctx_orchestrator_validate_outbound.go (1)
201-201: LGTM! Consistent with the updated validation flow.The change correctly integrates the coin type parameter from the inbound parameters.
x/crosschain/keeper/evm_hooks_test.go (1)
189-199: LGTM! SPL token withdrawal test case is comprehensive.The test correctly verifies that small amounts can be withdrawn for SPL tokens, which is the key change in this PR.
e2e/e2etests/e2etests.go (1)
456-456: LGTM! Test amount adjustment aligns with PR objectives.The reduction in default test amount from 1,000,000 to 100,000 SPL tokens appropriately tests the removal of minimum rent exempt requirement.
changelog.md (1)
11-14: LGTM! Clear and well-structured changelog entry.The entry properly documents the fix under the correct section and includes the PR reference.
Description
The minimum rent exempt
1_000_000 lamportswas intended to avoid SOL token withdraw failure on gateway program when a Solana receiver account doesn't have enough lamports.This rent exempt restriction should not be applied on SPL token because withdrawing SPL token already needs a non trivial amount of SOL for potential ATA creation so we can skip the minimal withdraw amount checking. Its already anti spam.
Changes:
validateZRC20Withdrawalfor non-Gas token.1_000_000 -> 100_000in SPL withdraw E2E test.How Has This Been Tested?
Summary by CodeRabbit
Release Notes
Fixes
Testing
Improvements