feat: integrate withdraw SPL #3134
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📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request includes extensive updates across various components of the ZetaChain project, primarily focusing on enhancements related to SPL token management on the Solana blockchain. Key changes include the addition of new test cases for SPL withdrawals, the introduction of a rent payer mechanism, and new message structures for SPL token transactions. Additionally, the changelog has been updated to reflect these changes, alongside improvements in dependency management and error handling throughout the system. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SolanaGateway
participant SPLToken
participant RentPayer
User->>SolanaGateway: Initiate SPL Withdrawal
SolanaGateway->>SPLToken: Validate Withdrawal Amount
SPLToken-->>SolanaGateway: Confirm Sufficient Balance
SolanaGateway->>RentPayer: Initialize Rent Payer
RentPayer-->>SolanaGateway: Confirm Rent Payer Initialized
SolanaGateway->>SPLToken: Execute Withdrawal
SPLToken-->>SolanaGateway: Confirm Withdrawal
SolanaGateway-->>User: Withdrawal Successful
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
|
|
!!!WARNING!!! Be very careful about using Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203 Pay extra attention to the way |
skosito
changed the title
skosito
changed the title
skosito
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
swift1337 and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (34)
contrib/localnet/solana/start-solana.sh (1)
Line range hint
1-19: Enhance script robustness and maintainability.The script lacks proper error handling, logging, and cleanup mechanisms.
Here's a more robust implementation:
#!/bin/bash +set -euo pipefail + +# Configuration +VALIDATOR_STARTUP_WAIT=5 +CLEANUP_WAIT=1 +LOG_FILE="/tmp/solana-validator.log" + +# Cleanup function +cleanup() { + echo "Shutting down Solana validator..." + pkill solana-test-validator || true + sleep "$CLEANUP_WAIT" +} + +# Set up cleanup trap +trap cleanup EXIT echo "making an id" -solana-keygen new -o /root/.config/solana/id.json --no-bip39-passphrase +if ! solana-keygen new -o /root/.config/solana/id.json --no-bip39-passphrase; then + echo "Failed to generate Solana keypair" + exit 1 +fi -solana config set --url localhost +if ! solana config set --url localhost; then + echo "Failed to set Solana config" + exit 1 +fi + echo "starting solana test validator..." -solana-test-validator & +solana-test-validator > "$LOG_FILE" 2>&1 & + +echo "Waiting for validator startup..." +sleep "$VALIDATOR_STARTUP_WAIT" + +# Verify validator is running +if ! solana cluster-version; then + echo "Failed to start Solana validator" + exit 1 +fi -sleep 5 # ... rest of the script ... - -# leave some time for debug if validator exits due to errors -sleep 1000 + +# Wait for program deployment to complete +waite2e/e2etests/test_spl_withdraw.go (2)
19-33: Extract magic numbers into named constants and improve documentation.The approval amount of 1 SOL is hardcoded and the comparison logic could be more explicit.
+// Maximum approved withdrawal amount in SOL +const MaxApprovedWithdrawalSOL = 1 + func TestSPLWithdraw(r *runner.E2ERunner, args []string) { // ... existing code ... - approvedAmount := new(big.Int).SetUint64(solana.LAMPORTS_PER_SOL) + approvedAmount := new(big.Int).SetUint64(MaxApprovedWithdrawalSOL * solana.LAMPORTS_PER_SOL)
58-69: Improve readability of balance verification logic.The nested big.Int operations and parentheses make the code hard to read. Consider extracting the verification logic into helper functions.
+func verifyReceiverBalance(r *runner.E2ERunner, before, after string, withdrawAmount *big.Int) { + beforeBig := parseBigInt(r, before) + afterBig := parseBigInt(r, after) + expected := new(big.Int).Add(beforeBig, withdrawAmount) + require.Zero(r, expected.Cmp(afterBig)) +} + func TestSPLWithdraw(r *runner.E2ERunner, args []string) { // ... existing code ... - require.Zero( - r, - new( - big.Int, - ).Add(withdrawAmount, parseBigInt(r, receiverBalanceBefore.Value.Amount)). - Cmp(parseBigInt(r, receiverBalanceAfter.Value.Amount)), - ) + verifyReceiverBalance(r, receiverBalanceBefore.Value.Amount, + receiverBalanceAfter.Value.Amount, withdrawAmount)e2e/e2etests/test_spl_withdraw_and_create_receiver_ata.go (4)
14-18: Enhance argument validation with descriptive error messageWhile the length check is good, consider adding a more descriptive error message to help developers understand the expected input.
- require.Len(r, args, 1) + require.Len(r, args, 1, "TestSPLWithdrawAndCreateReceiverAta requires exactly one argument: withdrawal amount")
26-35: Improve readability of balance comparisonsThe balance comparison could be more explicit, and the error message could be more descriptive.
- require.Equal(r, 1, zrc20BalanceBefore.Cmp(withdrawAmount), "Insufficient balance for withdrawal") + require.True(r, zrc20BalanceBefore.Cmp(withdrawAmount) > 0, "Insufficient balance: available %v, requested %v", zrc20BalanceBefore, withdrawAmount) - require.Equal( - r, - -1, - withdrawAmount.Cmp(approvedAmount), - "Withdrawal amount must be less than the approved amount (1e9)", - ) + require.True( + r, + withdrawAmount.Cmp(approvedAmount) < 0, + "Withdrawal amount %v exceeds approved limit of %v SOL", withdrawAmount, "1", + )
67-71: Enhance balance verification clarityThe balance verification could be more explicit and provide better error messages.
- require.Zero(r, withdrawAmount.Cmp(parseBigInt(r, receiverBalanceAfter.Value.Amount))) + require.Zero(r, withdrawAmount.Cmp(parseBigInt(r, receiverBalanceAfter.Value.Amount)), + "Expected receiver balance %v to equal withdrawal amount %v", + receiverBalanceAfter.Value.Amount, withdrawAmount) - require.Zero(r, new(big.Int).Sub(zrc20BalanceBefore, withdrawAmount).Cmp(zrc20BalanceAfter)) + expectedBalance := new(big.Int).Sub(zrc20BalanceBefore, withdrawAmount) + require.Zero(r, expectedBalance.Cmp(zrc20BalanceAfter), + "Expected ZRC20 balance %v after withdrawal, got %v", + expectedBalance, zrc20BalanceAfter)
1-72: Extract magic values into constantsConsider extracting magic values into named constants at the package level for better maintainability and reusability.
package e2etests +const ( + // DefaultApprovedAmount represents the default approved amount of 1 SOL in lamports + DefaultApprovedAmount = solana.LAMPORTS_PER_SOL +)pkg/contracts/solana/gateway.go (2)
63-64: Enhance function documentationThe function comment should provide more details about:
- The expected format of the input address
- The relationship between gateway address and rent payer PDA
- Return value significance
Consider updating the comment to:
-// ParseRentPayerPda parses the rent payer program derived address from the given string +// ParseRentPayerPda derives the rent payer's program derived address (PDA) from a gateway address. +// The input address should be a base58-encoded Solana public key representing the gateway. +// Returns the computed PDA that will be used for rent payment operations.
63-78: Consider refactoring to reduce code duplicationThe function shares similar logic with
ParseGatewayIDAndPda. Consider extracting the common PDA derivation logic into a helper function.+func deriveAddressFromSeed(gatewayID solana.PublicKey, seed string) (solana.PublicKey, error) { + seedBytes := []byte(seed) + pda, _, err := solana.FindProgramAddress([][]byte{seedBytes}, gatewayID) + return pda, err +} func ParseRentPayerPda(address string) (solana.PublicKey, error) { gatewayID, err := solana.PublicKeyFromBase58(address) if err != nil { return solana.PublicKey{}, errors.Wrap(err, "unable to decode address") } - seed := []byte(RentPayerPDASeed) - rentPayerPda, _, err = solana.FindProgramAddress([][]byte{seed}, gatewayID) - return rentPayerPda, err + return deriveAddressFromSeed(gatewayID, RentPayerPDASeed) }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 64-71: pkg/contracts/solana/gateway.go#L64-L71
Added lines #L64 - L71 were not covered by tests
[warning] 74-77: pkg/contracts/solana/gateway.go#L74-L77
Added lines #L74 - L77 were not covered by testszetaclient/chains/solana/signer/withdraw.go (3)
Line range hint
18-49: Consider enhancing error handling and type safety.While the implementation is solid, consider these improvements:
- The amount conversion from big.Int to uint64 could potentially overflow
- The error messages could be more structured for better error handling downstream
Consider applying these changes:
chainID := uint64(signer.Chain().ChainId) nonce := params.TssNonce -amount := params.Amount.Uint64() +amount, overflow := params.Amount.Uint64() +if overflow { + return nil, errors.Wrapf(types.ErrInvalidAmount, "amount %s exceeds uint64 max", params.Amount) +} // zero out the amount if cancelTx is set
Line range hint
1-138: Track unit test implementation.As mentioned in the PR objectives, unit tests are pending for the Solana functionality. While e2e tests are in place, unit tests are crucial for maintaining code quality and catching edge cases.
Would you like me to:
- Generate a template for unit tests covering the key scenarios?
- Open a GitHub issue to track the test implementation?
Compute budget optimization is still pending implementation
The issue #2599 regarding Solana compute budget and priority fees optimization remains open. The current implementation uses fixed fees (5K lamports) and default compute units (200K), which may need optimization for better performance and cost-efficiency.
zetaclient/chains/solana/signer/withdraw.go: Consider implementing dynamic compute budget based on transaction complexity- Current commented-out code shows intention to use
ComputeBudgetSetComputeUnitPrice, which aligns with the issue's scope🔗 Analysis chain
Line range hint
89-92: Verify the status of compute budget optimization.The TODO comment references issue #2599 regarding compute budget and priority fees optimization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the referenced issue is still open gh issue view 2599 --json state,title,bodyLength of output: 769
e2e/runner/setup_solana.go (1)
92-98: Consider using a new account slice variableWhile the implementation is correct, reusing the
accountSlicevariable from the previous initialization could make the code harder to maintain. Consider using a dedicated variable for rent payer accounts.-accountSlice = []*solana.AccountMeta{} +rentPayerAccounts := []*solana.AccountMeta{} -accountSlice = append(accountSlice, solana.Meta(rentPayerPdaComputed).WRITE()) -accountSlice = append(accountSlice, solana.Meta(privkey.PublicKey()).WRITE().SIGNER()) -accountSlice = append(accountSlice, solana.Meta(solana.SystemProgramID)) -instRentPayer.AccountValues = accountSlice +rentPayerAccounts = append(rentPayerAccounts, solana.Meta(rentPayerPdaComputed).WRITE()) +rentPayerAccounts = append(rentPayerAccounts, solana.Meta(privkey.PublicKey()).WRITE().SIGNER()) +rentPayerAccounts = append(rentPayerAccounts, solana.Meta(solana.SystemProgramID)) +instRentPayer.AccountValues = rentPayerAccountspkg/contracts/solana/instruction.go (1)
127-147: Add interface implementation declaration and improve documentation.
- Add interface implementation declaration for clarity:
+var _ OutboundInstruction = (*WithdrawSPLInstructionParams)(nil)
- Add documentation for the Decimals field to maintain consistency with other field documentation.
pkg/contracts/solana/gateway_message.go (1)
111-136: Enhance field documentation for clarityWhile the struct is well-structured, consider enhancing the documentation for better clarity:
tokenAccount: Specify if this is the source token accountrecipientAta: Clarify that this is the Associated Token Account for the recipient- Add units for the
amountfield (e.g., token base units)zetaclient/chains/solana/observer/outbound.go (1)
359-360: Add test coverage for ERC20 token type handling.While the PR objectives mention that unit tests will be added in a follow-up PR, it's important to ensure that this critical path for ERC20 token handling is tested. Consider adding tests that verify:
- Successful parsing of SPL withdrawal instructions
- Error handling for malformed instructions
- Integration with the broader outbound processing flow
Would you like me to help generate unit tests for the ERC20 token type handling in
ParseGatewayInstruction?🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 359-360: zetaclient/chains/solana/observer/outbound.go#L359-L360
Added lines #L359 - L360 were not covered by testszetaclient/chains/solana/observer/outbound_test.go (2)
237-240: Consider enhancing test coverage with table-driven tests.The test case effectively verifies error handling for unsupported coin types. Consider expanding it using a table-driven approach to test multiple unsupported coin types and standardize error messages.
Here's a suggested enhancement:
+var errUnsupportedCoinType = "unsupported outbound coin type" + func Test_ParseGatewayInstruction(t *testing.T) { // ... existing setup ... - t.Run("should return error on unsupported coin type", func(t *testing.T) { + tests := []struct { + name string + coinType coin.CoinType + }{ + {"Zeta coin type", coin.CoinType_Zeta}, + {"NFT coin type", coin.CoinType_NFT}, + // Add more unsupported types as needed + } + + for _, tt := range tests { + t.Run(fmt.Sprintf("should return error for %s", tt.name), func(t *testing.T) { txResult := testutils.LoadSolanaOutboundTxResult(t, TestDataDir, chain.ChainId, txHash) - inst, err := observer.ParseGatewayInstruction(txResult, gatewayID, coin.CoinType_Zeta) - require.ErrorContains(t, err, "unsupported outbound coin type") + inst, err := observer.ParseGatewayInstruction(txResult, gatewayID, tt.coinType) + require.ErrorContains(t, err, errUnsupportedCoinType) require.Nil(t, inst) - }) + }) + }
Line range hint
1-380: Consider refactoring test setup and constants.The test file demonstrates thorough coverage but could benefit from some structural improvements:
- Extract common test setup into test fixtures using
testing.TB.Helper().- Define constants for commonly used test values and error messages.
- Use more descriptive names for test cases, especially in error scenarios.
Example improvement for test setup:
type testFixture struct { chain chains.Chain gatewayID solana.PublicKey solClient *mocks.SolanaRPCClient tss interfaces.TSSSigner observer *observer.Observer } func setupTest(t testing.TB) *testFixture { t.Helper() chain := chains.SolanaDevnet gatewayID, err := solana.PublicKeyFromBase58(GatewayAddressTest) require.NoError(t, err) solClient := mocks.NewSolanaRPCClient(t) tss := mocks.NewMockTSS(chain, tssAddressTest, "") ob := createTestObserver(t, chain, solClient, tss) return &testFixture{ chain: chain, gatewayID: gatewayID, solClient: solClient, tss: tss, observer: ob, } }e2e/runner/solana.go (3)
33-43: Fix method documentation and enhance error handling.The method comment appears to be copy-pasted from
ComputePdaAddress. Additionally, consider enhancing error handling for better debugging.Apply this diff:
-// ComputePdaAddress computes the rent payer PDA address for the gateway program +// ComputeRentPayerPdaAddress computes the PDA address used for rent payments in the gateway program. +// It uses RentPayerPDASeed to derive a unique program-derived address that will be responsible +// for handling rent payments. func (r *E2ERunner) ComputeRentPayerPdaAddress() solana.PublicKey { seed := []byte(solanacontract.RentPayerPDASeed) GatewayProgramID := solana.MustPublicKeyFromBase58(solanacontract.SolanaGatewayProgramID) pdaComputed, bump, err := solana.FindProgramAddress([][]byte{seed}, GatewayProgramID) - require.NoError(r, err) + require.NoError(r, err, "failed to compute rent payer PDA: %v", err)
265-265: Document the reason for increasing mint amount.The mint amount has been increased by 1000x from 1_000_000 to 1_000_000_000. Please document the rationale behind this significant change as it could affect test scenarios.
Add a comment explaining the change:
+ // Mint 1 billion tokens (1_000_000_000) to ensure sufficient liquidity for all test scenarios mintToInstruction := token.NewMintToInstruction(uint64(1_000_000_000), tokenAccount.PublicKey(), ata, privateKey.PublicKey(), []solana.PublicKey{}).
395-422: Add input validation and improve documentation.The method follows good practices but could benefit from input validation and better documentation.
Apply this diff:
+// WithdrawSPLZRC20 withdraws an amount of ZRC20 SPL tokens to a specified Solana address. +// It handles the approval of gas tokens and monitors the cross-chain transaction until completion. +// +// Parameters: +// - to: destination Solana address +// - amount: amount of SPL tokens to withdraw +// - approveAmount: amount of gas tokens to approve for fee payment +// +// Returns: The completed cross-chain transaction func (r *E2ERunner) WithdrawSPLZRC20( to solana.PublicKey, amount *big.Int, approveAmount *big.Int, ) *crosschaintypes.CrossChainTx { + // Validate input parameters + require.NotNil(r, amount, "amount cannot be nil") + require.NotNil(r, approveAmount, "approveAmount cannot be nil") + require.True(r, amount.Sign() > 0, "amount must be positive") + require.True(r, approveAmount.Sign() > 0, "approveAmount must be positive") + // approve splzrc20 to spend gas tokens to pay gas feechangelog.md (3)
9-9: Consider adding more details about the SPL withdraw integration.The changelog entry could benefit from additional details about:
- The specific SPL token withdrawal functionality being integrated
- Any limitations or prerequisites
- Impact on existing functionality
Consider expanding the entry to:
-* [3134](https://github.com/zeta-chain/node/pull/3134) - integrate withdraw SPL +* [3134](https://github.com/zeta-chain/node/pull/3134) - integrate withdraw SPL: Add support for withdrawing SPL tokens to both existing token accounts and new Associated Token Accounts (ATA), with automatic ATA creation by the gateway's rent payer.
Line range hint
1-24: Consider marking this as a feature-complete milestone version.Since this PR represents the last feature addition as mentioned in the PR objectives, consider:
- Adding a clear version number for this release
- Adding a note indicating this is the feature-complete milestone
Consider adding:
# CHANGELOG +## Version v[X.Y.Z] - Feature Complete Release + +This version marks the completion of feature development for the SPL integration milestone. + ## Unreleased ### Features
11-11: Add missing test entries to the changelog.The PR objectives mention e2e tests for SPL token withdrawal scenarios, but these aren't reflected in the changelog.
Consider adding under the Tests section:
### Tests +* [3134](https://github.com/zeta-chain/node/pull/3134) - add e2e tests for SPL token withdrawals: + - Test withdrawing to existing token accounts + - Test withdrawing with automatic ATA creationzetaclient/chains/solana/signer/withdraw_spl.go (4)
64-64: Correct grammatical error in log messageThe log message contains a grammatical error. "Key-sign succeed" should be "Key-sign succeeded" to use the correct past tense.
Apply this diff to fix the grammatical error:
- signer.Logger().Std.Info().Msgf("Key-sign succeed for chain %d nonce %d", chainID, nonce) + signer.Logger().Std.Info().Msgf("Key-sign succeeded for chain %d nonce %d", chainID, nonce)
76-76: Remove redundant declaration of 'err' variableThe variable
erris already being used in the function scope. The explicit declarationvar err erroris unnecessary and can be removed for cleaner code.Apply this diff to remove the redundant declaration:
- var err error
115-118: Implement compute budget adjustments for optimized transaction feesThe TODO comment suggests exploring priority fees and compute budget settings to optimize transaction costs. Addressing this would enhance performance and align with best practices.
Would you like assistance in implementing the compute budget adjustments as per issue #2599?
162-162: Preallocate slice capacity for 'accountSlice'Since the number of accounts to append is known, preallocating the slice capacity improves performance by reducing memory allocations.
Apply this diff to preallocate the slice:
- var accountSlice []*solana.AccountMeta + accountSlice := make([]*solana.AccountMeta, 0, 10)zetaclient/chains/solana/signer/signer.go (6)
9-9: Remove Unnecessary Import of 'github.com/davecgh/go-spew/spew'The
spewpackage is typically used for debugging purposes. Since it's not used in the production code, please remove this import to keep the codebase clean.Apply this diff to remove the import:
- "github.com/davecgh/go-spew/spew"
50-51: Improve Clarity of Comment for 'rentPayerPda' FieldThe comment for
rentPayerPdacan be enhanced for better understanding. Consider rephrasing it to clearly explain the purpose of this field.Suggested change:
- // rent payer pda is the program derived address of the gateway program to pay rent for creating atas + // rentPayerPda is the program-derived address (PDA) used to pay rent for creating Associated Token Accounts (ATAs)
75-76: Correct the Error Message When Parsing 'rentPayerPda' FailsThe error message should accurately reflect the failure to parse the rent payer PDA, not the gateway address.
Apply this diff:
- return nil, errors.Wrapf(err, "cannot parse gateway address %s", chainParams.GatewayAddress) + return nil, errors.Wrapf(err, "cannot parse rent payer PDA from gateway address %s", chainParams.GatewayAddress)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 75-76: zetaclient/chains/solana/signer/signer.go#L75-L76
Added lines #L75 - L76 were not covered by tests
167-174: Ensure Consistent Error Messages in 'TryProcessOutbound'For consistency, align the error message format with other cases. This enhances readability and maintainability.
Suggested change:
- logger.Error().Err(err).Msgf("TryProcessOutbound: Fail to sign withdraw spl outbound") + logger.Error().Err(err).Msgf("TryProcessOutbound: Failed to sign withdraw SPL outbound")🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 167-172: zetaclient/chains/solana/signer/signer.go#L167-L172
Added lines #L167 - L172 were not covered by tests
[warning] 174-174: zetaclient/chains/solana/signer/signer.go#L174
Added line #L174 was not covered by tests
243-291: Refactor Common Logic to Reduce Code DuplicationBoth
prepareWithdrawTxandprepareWithdrawSPLTxfunctions share similar logic, including compliance checks and transaction preparation. Consider extracting the shared code into a helper function to enhance maintainability and reduce duplication.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 248-263: zetaclient/chains/solana/signer/signer.go#L248-L263
Added lines #L248 - L263 were not covered by tests
[warning] 266-269: zetaclient/chains/solana/signer/signer.go#L266-L269
Added lines #L266 - L269 were not covered by tests
[warning] 272-282: zetaclient/chains/solana/signer/signer.go#L272-L282
Added lines #L272 - L282 were not covered by tests
[warning] 285-288: zetaclient/chains/solana/signer/signer.go#L285-L288
Added lines #L285 - L288 were not covered by tests
266-269: Handle Errors Gracefully When Decoding Mint Account DetailsEnsure that the error returned when decoding mint account details includes context for easier debugging. Providing additional information can aid in identifying issues quickly.
Suggested change:
if err != nil { - return nil, err + return nil, errors.Wrapf(err, "failed to decode mint account details for asset %s", cctx.InboundParams.Asset) }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 266-269: zetaclient/chains/solana/signer/signer.go#L266-L269
Added lines #L266 - L269 were not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
changelog.md(1 hunks)cmd/zetae2e/local/local.go(1 hunks)contrib/localnet/solana/start-solana.sh(1 hunks)e2e/e2etests/e2etests.go(3 hunks)e2e/e2etests/test_spl_withdraw.go(1 hunks)e2e/e2etests/test_spl_withdraw_and_create_receiver_ata.go(1 hunks)e2e/runner/setup_solana.go(1 hunks)e2e/runner/solana.go(3 hunks)go.mod(1 hunks)pkg/contracts/solana/gateway.go(3 hunks)pkg/contracts/solana/gateway_message.go(2 hunks)pkg/contracts/solana/instruction.go(2 hunks)zetaclient/chains/solana/observer/outbound.go(1 hunks)zetaclient/chains/solana/observer/outbound_test.go(1 hunks)zetaclient/chains/solana/signer/signer.go(6 hunks)zetaclient/chains/solana/signer/withdraw.go(1 hunks)zetaclient/chains/solana/signer/withdraw_spl.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (15)
cmd/zetae2e/local/local.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
contrib/localnet/solana/start-solana.sh (1)
Pattern **/*.sh: Review the shell scripts, point out issues relative to security, performance, and maintainability.
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_spl_withdraw.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_spl_withdraw_and_create_receiver_ata.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/setup_solana.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/solana.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
pkg/contracts/solana/gateway.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
pkg/contracts/solana/gateway_message.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
pkg/contracts/solana/instruction.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/observer/outbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/observer/outbound_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/signer/signer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/signer/withdraw.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/signer/withdraw_spl.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🪛 GitHub Check: codecov/patch
pkg/contracts/solana/gateway.go
[warning] 64-71: pkg/contracts/solana/gateway.go#L64-L71
Added lines #L64 - L71 were not covered by tests
[warning] 74-77: pkg/contracts/solana/gateway.go#L74-L77
Added lines #L74 - L77 were not covered by tests
pkg/contracts/solana/gateway_message.go
[warning] 143-152: pkg/contracts/solana/gateway_message.go#L143-L152
Added lines #L143 - L152 were not covered by tests
[warning] 156-157: pkg/contracts/solana/gateway_message.go#L156-L157
Added lines #L156 - L157 were not covered by tests
[warning] 161-162: pkg/contracts/solana/gateway_message.go#L161-L162
Added lines #L161 - L162 were not covered by tests
[warning] 166-167: pkg/contracts/solana/gateway_message.go#L166-L167
Added lines #L166 - L167 were not covered by tests
[warning] 171-172: pkg/contracts/solana/gateway_message.go#L171-L172
Added lines #L171 - L172 were not covered by tests
[warning] 175-176: pkg/contracts/solana/gateway_message.go#L175-L176
Added lines #L175 - L176 were not covered by tests
[warning] 179-180: pkg/contracts/solana/gateway_message.go#L179-L180
Added lines #L179 - L180 were not covered by tests
[warning] 183-184: pkg/contracts/solana/gateway_message.go#L183-L184
Added lines #L183 - L184 were not covered by tests
[warning] 188-207: pkg/contracts/solana/gateway_message.go#L188-L207
Added lines #L188 - L207 were not covered by tests
[warning] 211-213: pkg/contracts/solana/gateway_message.go#L211-L213
Added lines #L211 - L213 were not covered by tests
[warning] 217-218: pkg/contracts/solana/gateway_message.go#L217-L218
Added lines #L217 - L218 were not covered by tests
[warning] 222-225: pkg/contracts/solana/gateway_message.go#L222-L225
Added lines #L222 - L225 were not covered by tests
[warning] 229-230: pkg/contracts/solana/gateway_message.go#L229-L230
Added lines #L229 - L230 were not covered by tests
[warning] 234-238: pkg/contracts/solana/gateway_message.go#L234-L238
Added lines #L234 - L238 were not covered by tests
pkg/contracts/solana/instruction.go
[warning] 150-155: pkg/contracts/solana/instruction.go#L150-L155
Added lines #L150 - L155 were not covered by tests
[warning] 159-160: pkg/contracts/solana/instruction.go#L159-L160
Added lines #L159 - L160 were not covered by tests
[warning] 164-165: pkg/contracts/solana/instruction.go#L164-L165
Added lines #L164 - L165 were not covered by tests
[warning] 170-176: pkg/contracts/solana/instruction.go#L170-L176
Added lines #L170 - L176 were not covered by tests
[warning] 179-181: pkg/contracts/solana/instruction.go#L179-L181
Added lines #L179 - L181 were not covered by tests
[warning] 183-183: pkg/contracts/solana/instruction.go#L183
Added line #L183 was not covered by tests
zetaclient/chains/solana/observer/outbound.go
[warning] 359-360: zetaclient/chains/solana/observer/outbound.go#L359-L360
Added lines #L359 - L360 were not covered by tests
zetaclient/chains/solana/signer/signer.go
[warning] 75-76: zetaclient/chains/solana/signer/signer.go#L75-L76
Added lines #L75 - L76 were not covered by tests
[warning] 167-172: zetaclient/chains/solana/signer/signer.go#L167-L172
Added lines #L167 - L172 were not covered by tests
[warning] 174-174: zetaclient/chains/solana/signer/signer.go#L174
Added line #L174 was not covered by tests
[warning] 248-263: zetaclient/chains/solana/signer/signer.go#L248-L263
Added lines #L248 - L263 were not covered by tests
[warning] 266-269: zetaclient/chains/solana/signer/signer.go#L266-L269
Added lines #L266 - L269 were not covered by tests
[warning] 272-282: zetaclient/chains/solana/signer/signer.go#L272-L282
Added lines #L272 - L282 were not covered by tests
[warning] 285-288: zetaclient/chains/solana/signer/signer.go#L285-L288
Added lines #L285 - L288 were not covered by tests
🔇 Additional comments (22)
e2e/e2etests/test_spl_withdraw.go (2)
1-12: LGTM: Clean package structure and imports.
The imports are well-organized and appropriately grouped between standard library and external dependencies.
34-44: Enhance security and error handling for key operations.
Consider adding documentation for error cases and implementing secure key handling practices.
e2e/e2etests/test_spl_withdraw_and_create_receiver_ata.go (1)
49-50: Verify withdrawal error handling
The withdrawal operation seems to lack explicit error handling. Please verify that WithdrawSPLZRC20 properly handles and propagates errors.
✅ Verification successful
The previous pattern didn't yield results, so I'm broadening the search to find any mentions of the withdrawal method and locate the test file for better context. Let's analyze the implementation details and error handling approach.
The previous output shows the beginning of the WithdrawSPLZRC20 implementation, but we need to see more of its implementation to verify the error handling approach. Let's get more context to analyze how errors are handled throughout the function.
Error handling is properly implemented
The WithdrawSPLZRC20 method has comprehensive error handling through the require.NoError assertions and utility functions:
- Errors from the approval transaction are checked via
require.NoError - Transaction receipt is verified using
MustWaitForTxReceipt - Transaction success is validated through
RequireTxSuccessful - The withdrawal transaction follows the same robust error checking pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for WithdrawSPLZRC20 implementation to verify error handling
ast-grep --pattern 'func $_ WithdrawSPLZRC20($_) {
$$$
}'
Length of output: 61
Script:
#!/bin/bash
# Search for WithdrawSPLZRC20 method with broader pattern and case-insensitive
rg -i "WithdrawSPLZRC20" -A 10
# Also search for the test file implementation
rg -l "test_spl_withdraw_and_create_receiver_ata.go"
Length of output: 2893
Script:
#!/bin/bash
# Get more context of the WithdrawSPLZRC20 implementation
rg -A 20 "func \(r \*E2ERunner\) WithdrawSPLZRC20" e2e/runner/solana.go
Length of output: 906
pkg/contracts/solana/gateway.go (2)
17-19: LGTM: Well-documented constant addition
The new constant follows the established patterns and is properly documented.
32-33: LGTM: Consistent discriminator implementation
The new discriminator follows the established pattern and integrates well with the existing codebase.
zetaclient/chains/solana/signer/withdraw.go (2)
16-16: LGTM: Clear and accurate function documentation.
The updated comment accurately describes the function's purpose and its use of TSS (Threshold Signature Scheme) for gateway withdraw instructions.
Line range hint 124-138: LGTM: Proper account metadata handling.
The function correctly sets up account metadata with appropriate write and signer flags, following Solana's best practices.
e2e/runner/setup_solana.go (2)
88-91: LGTM: Clear and consistent variable declarations
The variable declarations and PDA computation follow the established pattern, maintaining consistency with the existing gateway initialization implementation.
100-103: LGTM: Proper instruction serialization
The instruction serialization is implemented correctly, using the appropriate discriminator and error handling.
pkg/contracts/solana/instruction.go (2)
25-29: LGTM: InitializeRentPayerParams implementation is clean and well-documented.
149-166: Consider reducing code duplication in instruction parameter types.
The methods for WithdrawSPLInstructionParams are identical to WithdrawInstructionParams. Consider extracting common functionality into a shared base struct or utility functions to improve maintainability.
Example approach:
type BaseWithdrawParams struct {
Amount uint64
Signature [64]byte
RecoveryID uint8
MessageHash [32]byte
Nonce uint64
}
func (b *BaseWithdrawParams) Signer() (common.Address, error) {
// Common implementation
}
// WithdrawSPLInstructionParams and WithdrawInstructionParams would embed BaseWithdrawParamsNote: The current lack of test coverage is acknowledged and will be addressed in a follow-up PR.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 150-155: pkg/contracts/solana/instruction.go#L150-L155
Added lines #L150 - L155 were not covered by tests
[warning] 159-160: pkg/contracts/solana/instruction.go#L159-L160
Added lines #L159 - L160 were not covered by tests
[warning] 164-165: pkg/contracts/solana/instruction.go#L164-L165
Added lines #L164 - L165 were not covered by tests
pkg/contracts/solana/gateway_message.go (1)
11-22: LGTM: Clean separation of concerns
The documentation updates in MsgWithdraw appropriately reflect its focused responsibility on standard withdrawals, maintaining a clear separation from SPL token functionality.
zetaclient/chains/solana/observer/outbound.go (1)
359-360: Verify error handling in the outbound processing flow.
The new ERC20 case is integrated into a critical path of outbound transaction processing. Let's ensure proper error handling throughout the call chain.
Run the following script to analyze the error handling flow:
#!/bin/bash
# Description: Verify error handling in the outbound processing flow
# Check error handling in functions that call ParseGatewayInstruction
rg -A 5 "ParseGatewayInstruction\("
# Check error handling patterns in related functions
ast-grep --pattern 'if err := $$$; err != nil {
$$$
}'🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 359-360: zetaclient/chains/solana/observer/outbound.go#L359-L360
Added lines #L359 - L360 were not covered by tests
go.mod (3)
Line range hint 3-5: LGTM: Go toolchain version update.
The update from Go 1.22.7 to 1.22.8 is a minor version bump that likely includes security fixes and improvements.
Line range hint 391-399: LGTM: ZetaChain maintained forks.
The replace directives for ZetaChain maintained forks are properly configured, ensuring consistency across the codebase:
- tss-lib fork with threshold-dep-updates
- go-ethereum fork
- go-libp2p fork
- go-tss fork
319-323: Verify dependency compatibility and usage.
The new dependencies added:
github.com/bnb-chain/tss-lib: Ensure it's compatible with the existing threshold signature implementationgithub.com/montanaflynn/stats: Consider if this is necessary for the SPL withdrawal featuregithub.com/showa-93/go-mask: Verify its usage for sensitive data handling- Updated
protocol-contracts-solana/go-idl: Aligns with SPL integration
✅ Verification successful
Dependencies are appropriately integrated and utilized
The verification confirms proper integration of all new dependencies:
github.com/bnb-chain/tss-lib: Correctly used in TSS implementation for key generationgithub.com/montanaflynn/stats: Appropriately utilized in ETH withdrawal stress testsgithub.com/showa-93/go-mask: Integrated for sensitive data handling in client configurationprotocol-contracts-solana/go-idl: Properly implemented in Solana gateway contract integration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the new dependencies are actually used in the codebase
echo "Checking tss-lib usage..."
rg "github.com/bnb-chain/tss-lib" -l
echo "Checking stats package usage..."
rg "github.com/montanaflynn/stats" -l
echo "Checking go-mask usage..."
rg "github.com/showa-93/go-mask" -l
Length of output: 485
Script:
#!/bin/bash
# Check the actual implementation and usage patterns of these dependencies
echo "Checking tss-lib implementation..."
rg "github.com/bnb-chain/tss-lib" -A 5 zetaclient/tss/tss_signer.go
echo "Checking stats usage in stress test..."
rg "github.com/montanaflynn/stats" -A 5 e2e/e2etests/test_stress_eth_withdraw.go
echo "Checking go-mask implementation..."
rg "github.com/showa-93/go-mask" -A 5 zetaclient/config/types.go
echo "Checking protocol-contracts-solana/go-idl usage..."
rg "github.com/zeta-chain/protocol-contracts-solana/go-idl" -l
Length of output: 1279
cmd/zetae2e/local/local.go (1)
441-443: Consider reorganizing SPL token tests under admin tests.
The SPL token tests are currently mixed with regular Solana tests. For better organization and maintainability, consider moving these tests under the admin test suite since they involve administrative operations like whitelisting.
Apply this diff to reorganize the tests:
solanaTests := []string{
e2etests.TestSolanaDepositName,
e2etests.TestSolanaWithdrawName,
e2etests.TestSolanaDepositAndCallName,
e2etests.TestSolanaDepositAndCallRefundName,
e2etests.TestSolanaDepositRestrictedName,
e2etests.TestSolanaWithdrawRestrictedName,
// TODO move under admin tests
// https://github.com/zeta-chain/node/issues/3085
e2etests.TestSPLDepositName,
e2etests.TestSPLDepositAndCallName,
- e2etests.TestSPLWithdrawName,
- e2etests.TestSPLWithdrawAndCreateReceiverAtaName,
- e2etests.TestSolanaWhitelistSPLName,
}
eg.Go(solanaTestRoutine(conf, deployerRunner, verbose, solanaTests...))
}
if testAdmin {
eg.Go(adminTestRoutine(conf, deployerRunner, verbose,
+ // SPL token tests
+ e2etests.TestSPLWithdrawName,
+ e2etests.TestSPLWithdrawAndCreateReceiverAtaName,
+ e2etests.TestSolanaWhitelistSPLName,
e2etests.TestWhitelistERC20Name,Let's verify if there are any other SPL-related tests that should be moved:
e2e/e2etests/e2etests.go (2)
438-453: LGTM! Well-structured test definitions for SPL withdrawals.
The new test definitions for SPL withdrawals are well-organized and follow the established patterns:
- Clear test descriptions
- Consistent argument definitions
- Proper function references
490-490: Verify the impact of increased default SPL deposit amounts.
The default values for SPL deposit tests have been increased from "500000" to "12000000". While the values are consistent across both deposit tests, please ensure this increase aligns with the test requirements and won't cause issues with token supply limits.
Also applies to: 498-498
✅ Verification successful
The increased SPL deposit amount (12000000) is consistent with test requirements and poses no supply limit concerns.
The codebase analysis shows that:
- The increased value is consistently used across SPL deposit tests
- No explicit SPL token supply limits are defined in the codebase
- The value (12M tokens) is a reasonable test amount considering typical SPL token decimals and test requirements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new deposit amounts are within acceptable limits
# by searching for any supply cap or limit definitions in the codebase.
# Search for SPL token supply/limit definitions
rg -i "spl.*supply|spl.*limit|spl.*cap" --type go
Length of output: 51
Script:
#!/bin/bash
# Let's search for the previous value and any related SPL token test configurations
rg -B2 -A2 "500000" --type go
# Also search for any SPL token related test configurations
rg -B2 -A2 "spl.*token.*test" --type go -i
# Search specifically in the e2e test file for context
cat e2e/e2etests/e2etests.go
Length of output: 120815
zetaclient/chains/solana/signer/withdraw_spl.go (3)
16-68: Function 'createAndSignMsgWithdrawSPL' implementation is robust
The function correctly creates and signs the withdrawal message with proper error handling and validation. The logic for handling the cancelTx flag and zeroing out the amount is appropriate.
70-138: Function 'signWithdrawSPLTx' is well-structured
The function effectively wraps the withdrawal message into a Solana transaction, including serialization and signing processes. Error handling is thorough, and the use of context for transaction commitment is appropriate.
140-178: Function 'attachWithdrawSPLAccounts' correctly attaches required accounts
The function accurately assembles the necessary accounts for the withdrawal instruction. The use of FindAssociatedTokenAddress and the sequence of account metadata ensure proper transaction execution.
There was a problem hiding this comment.
Looks good to me. Would be great getting a re-review from @swift1337 before considering merging
| require.NotNil(r, receiverAtaAcc) | ||
|
|
||
| // verify balances are updated | ||
| receiverBalanceAfter, err := r.SolanaClient.GetTokenAccountBalance(r.Ctx, receiverAta, rpc.CommitmentConfirmed) |
There was a problem hiding this comment.
this was fixed in this PR
| @@ -26,6 +29,8 @@ const ( | |||
| var ( | |||
| // DiscriminatorInitialize returns the discriminator for Solana gateway 'initialize' instruction | |||
| DiscriminatorInitialize = idlgateway.IDLGateway.GetDiscriminator("initialize") | |||
| // DiscriminatorInitializeRentPayer returns the discriminator for Solana gateway 'initialize_rent_payer' instruction | |||
There was a problem hiding this comment.
nit: maybe add new lines between each constant?
There was a problem hiding this comment.
i will update this in follow up PR
| tokenAccount solana.PublicKey | ||
|
|
||
| // decimals of spl token | ||
| decimals uint8 |
There was a problem hiding this comment.
Is decimal a required field by gateway contract call for SPL withdraw?
There was a problem hiding this comment.
yes, decimals is used as argument in withdraw_spl in solana, needed by token transfer_checked function
| } | ||
|
|
||
| // parse rent payer PDA, used in case receiver ATA should be created in gateway | ||
| rentPayerPda, err := contracts.RentPayerPDA(gatewayID) |
There was a problem hiding this comment.
the error message might need update?
return nil, errors.Wrap(err, "cannot parse rent payer address")
There was a problem hiding this comment.
i will update this in follow up PR
| params *types.OutboundParams, | ||
| height uint64, | ||
| asset string, | ||
| decimals uint8, |
There was a problem hiding this comment.
the same question if this decimal is necessary or not
There was a problem hiding this comment.
yes, decimals is used as argument in withdraw_spl in solana, needed by token transfer_checked function
i think signer is always the first, and other accounts get mixed up in order, but i think correct for signer is: instead of simply: isn't order of accounts defined in gateway program? my understanding is that inst.Accounts is exactly that order defined in program |
| recipientAta, _, err := solana.FindAssociatedTokenAddress(msg.To(), msg.TokenAccount()) | ||
| if err != nil { | ||
| return nil, errors.Wrapf(err, "cannot find ATA for %s and token account %s", msg.To(), msg.TokenAccount()) | ||
| } |
There was a problem hiding this comment.
The recipient ATA is already retrieved in createAndSignMsgWithdrawSPL()
node/zetaclient/chains/solana/signer/withdraw_spl.go
Lines 49 to 52 in 08ff881
Here, it could be removed and msg.RecipientAta() used instead.
Description
missing:
How Has This Been Tested?
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Documentation