feat: integrate SPL deposits #3124
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📝 Walkthrough📝 WalkthroughWalkthroughThis pull request includes significant updates across multiple files, focusing on the integration of SPL token functionalities on the Solana blockchain. Key changes involve the addition of features for whitelisting SPL tokens, enhancements to contract handling, and improvements in testing frameworks. Additionally, various refactors and fixes enhance the overall structure and error handling of the existing codebase, particularly in relation to SPL and ZRC20 contracts. 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
|
|
!!!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 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3124 +/- ##
===========================================
- Coverage 63.29% 63.24% -0.05%
===========================================
Files 422 423 +1
Lines 29959 29981 +22
===========================================
Hits 18962 18962
- Misses 10157 10176 +19
- Partials 840 843 +3
|
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: 19
🧹 Outside diff range and nitpick comments (21)
e2e/e2etests/test_solana_whitelist_spl.go (2)
Line range hint
25-32: Enhance error handling and seed construction.The whitelist verification could benefit from more descriptive error messages and better seed management.
Consider these improvements:
- seed := [][]byte{[]byte("whitelist"), spl.PublicKey().Bytes()} + const whitelistSeedPrefix = "whitelist" + seed := [][]byte{[]byte(whitelistSeedPrefix), spl.PublicKey().Bytes()} whitelistEntryPDA, _, err := solana.FindProgramAddress(seed, r.GatewayProgram) require.NoError(r, err) whitelistEntryInfo, err := r.SolanaClient.GetAccountInfo(r.Ctx, whitelistEntryPDA) - require.Error(r, err) - require.Nil(r, whitelistEntryInfo) + require.Error(r, err, "expected whitelist entry to not exist") + require.Nil(r, whitelistEntryInfo, "whitelist entry should be nil before whitelisting")
Line range hint
34-71: Enhance test robustness and maintainability.The test could benefit from several improvements:
- Extract magic strings for token parameters
- Add comprehensive validation after whitelisting
- Configure timeout for mining wait
Consider applying these improvements:
+const ( + testSPLName = "TESTSPL" + testSPLSymbol = "TESTSPL" + testSPLDecimals = uint32(6) + testSPLGasLimit = uint64(100000) +) - "TESTSPL", - "TESTSPL", - 6, - 100000, + testSPLName, + testSPLSymbol, + testSPLDecimals, + testSPLGasLimit, // wait for the whitelist cctx to be mined - r.WaitForMinedCCTXFromIndex(whitelistCCTXIndex) + r.WaitForMinedCCTXFromIndex(whitelistCCTXIndex, runner.WithTimeout(30)) // check that whitelist entry exists for this spl whitelistEntryInfo, err = r.SolanaClient.GetAccountInfo(r.Ctx, whitelistEntryPDA) require.NoError(r, err) require.NotNil(r, whitelistEntryInfo) + + // Verify whitelist entry parameters + require.Equal(r, testSPLName, whitelistEntryInfo.Name, "incorrect token name") + require.Equal(r, testSPLSymbol, whitelistEntryInfo.Symbol, "incorrect token symbol") + require.Equal(r, testSPLDecimals, whitelistEntryInfo.Decimals, "incorrect decimals")pkg/contracts/solana/gateway.go (2)
20-20: Update comment to match renamed constant.The comment still references the old constant name
AccountsNumberOfDepositwhile the constant has been renamed toAccountsNumDeposit.- // AccountsNumberOfDeposit is the number of accounts required for Solana gateway deposit instruction + // AccountsNumDeposit is the number of accounts required for Solana gateway deposit instruction
Line range hint
1-1: Fix typo in package documentation.The package comment contains a typo: "privides" should be "provides".
-// Package solana privides structures and constants that are used when interacting with the gateway program on Solana chain. +// Package solana provides structures and constants that are used when interacting with the gateway program on Solana chain.e2e/e2etests/test_solana_deposit_spl.go (3)
24-38: Enhance error handling and account validation.While error checking exists, the error messages could be more descriptive. Additionally, consider validating that the token accounts were successfully created/found.
Consider enhancing the error handling:
pdaAta := r.FindOrCreateAssociatedTokenAccount(privKey, pda, r.SPLAddr) + require.NotNil(r, pdaAta, "failed to create/find PDA associated token account") pdaBalanceBefore, err := r.SolanaClient.GetTokenAccountBalance(r.Ctx, pdaAta, rpc.CommitmentConfirmed) - require.NoError(r, err) + require.NoError(r, err, "failed to get PDA token balance: %v", err)
39-46: Consider adding timeout and retry mechanisms.The test could be more robust with:
- Proper timeout handling for CCTX mining
- Retry mechanism for failed transactions
- Justification for the security comment disabling G115
Consider implementing a retry mechanism:
+ // retry configuration + const maxRetries = 3 + var lastErr error + + for i := 0; i < maxRetries; i++ { sig := r.SPLDepositAndCall(&privKey, uint64(amount), r.SPLAddr, r.EVMAddress(), nil) cctx := utils.WaitCctxMinedByInboundHash(r.Ctx, sig.String(), r.CctxClient, r.Logger, r.CctxTimeout) - r.Logger.CCTX(*cctx, "solana_deposit_spl") + if cctx != nil { + r.Logger.CCTX(*cctx, "solana_deposit_spl") + break + } + lastErr = fmt.Errorf("attempt %d: transaction failed to mine", i+1) + time.Sleep(time.Second * 2) + } + require.NoError(r, lastErr, "failed to execute deposit after %d attempts", maxRetries)
48-66: Improve balance verification clarity and precision.The balance verification logic, especially the final ZRC20 balance check, could be more explicit and easier to understand.
Consider refactoring the balance checks for better clarity:
- require.Zero(r, zrc20BalanceBefore.Add(zrc20BalanceBefore, big.NewInt(int64(amount))).Cmp(zrc20BalanceAfter)) + expectedBalance := new(big.Int).Add(zrc20BalanceBefore, big.NewInt(int64(amount))) + require.Equal(r, expectedBalance, zrc20BalanceAfter, "ZRC20 balance mismatch after deposit")cmd/zetae2e/config/config.go (1)
57-59: LGTM! Consider enhancing comment clarity.The SPL address export is implemented correctly, following the established pattern. The comment could be more descriptive to match the documentation style of other sections.
Consider updating the comment to be more specific:
-// copy contracts from deployer runner +// Export contract addresses from the deployer runner to the configurationx/fungible/keeper/foreign_coins.go (1)
114-114: Consider case-insensitive comparison for asset names.While the direct string comparison is cleaner, consider using case-insensitive comparison to prevent potential issues with differently cased but equivalent asset identifiers. This is especially important when dealing with cross-chain assets that might come from different sources.
- if asset == coin.Asset && coin.ForeignChainId == chainID { + if strings.EqualFold(asset, coin.Asset) && coin.ForeignChainId == chainID {pkg/contracts/solana/instruction.go (1)
37-47: Add discriminator constant.For consistency, add a constant for the SPL deposit instruction discriminator:
// Add at the package level with other discriminator constants var DiscriminatorDepositSPL = [8]byte{...} // Add appropriate discriminator valuee2e/runner/setup_zeta.go (1)
192-196: Consider adding address validation before initialization.While the implementation follows the established pattern, consider adding validation to ensure
splzrc20Addris not a zero address before initialization.// Set SPLZRC20Addr r.SPLZRC20Addr = ethcommon.HexToAddress(splzrc20Addr) +require.NotEqual(r, ethcommon.Address{}, r.SPLZRC20Addr, "spl zrc20 address is zero") r.SPLZRC20, err = zrc20.NewZRC20(r.SPLZRC20Addr, r.ZEVMClient) require.NoError(r, err)zetaclient/chains/solana/observer/inbound_test.go (1)
212-258: Test implementation looks good but could benefit from additional test cases.The test is well-structured with clear setup, execution, and validation phases. However, consider the following improvements:
Add error case scenarios:
- Invalid transaction data
- Missing or malformed memo
- Invalid SPL token address
- Insufficient balance
Consider extracting test data to constants or fixtures:
- Transaction hash
- Account addresses
- Expected values
Example structure for additional test cases:
func Test_ParseInboundAsDepositSPL(t *testing.T) { // Move test data to constants const ( testTxHash = "aY8yLDze6nHSRi7L5REozKAZY1aAyPJ6TfibiqQL5JGwgSBkYux5z5JfXs5ed8LZqpXUy4VijoU3x15mBd66ZGE" testSender = "37yGiHAnLvWZUNVwu9esp74YQFqxU1qHCbABkDvRddUQ" testSPLToken = "4GddKQ7baJpMyKna7bPPnhh7UQtpzfSGL1FgZ31hj4mp" ) // Common setup... tests := []struct { name string tx *solana.Transaction slot uint64 want *clienttypes.InboundEvent wantErr bool }{ {"successful deposit", /* current test case */ }, {"invalid transaction", /* error case */ }, {"missing memo", /* error case */ }, // Add more test cases... } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got, err := ob.ParseInboundAsDepositSPL(tt.tx, 0, tt.slot) if tt.wantErr { require.Error(t, err) return } require.NoError(t, err) require.EqualValues(t, tt.want, got) }) } }e2e/config/config.go (2)
Line range hint
122-145: Add validation for new SPL address fieldsThe new SPL address fields should be validated similar to other address fields to ensure they contain valid addresses.
Consider adding validation in the
Validate()method:func (c Config) Validate() error { if c.RPCs.Bitcoin.Params != Mainnet && c.RPCs.Bitcoin.Params != Testnet3 && c.RPCs.Bitcoin.Params != Regnet { return errors.New("invalid bitcoin params") } + // Validate SPL addresses if present + if c.Contracts.Solana.SPL != "" { + if _, err := c.Contracts.Solana.SPL.AsEVMAddress(); err != nil { + return fmt.Errorf("invalid Solana SPL address: %w", err) + } + } + if c.Contracts.ZEVM.SPLZRC20Addr != "" { + if _, err := c.Contracts.ZEVM.SPLZRC20Addr.AsEVMAddress(); err != nil { + return fmt.Errorf("invalid ZEVM SPL ZRC20 address: %w", err) + } + } err := c.DefaultAccount.Validate() if err != nil { return fmt.Errorf("validating deployer account: %w", err) } // ... rest of the validation }
Line range hint
169-189: Initialize new SPL fields in DefaultConfigThe
DefaultConfig()function should provide default values for the new SPL fields to maintain consistency with other fields.Consider updating the default configuration:
func DefaultConfig() Config { return Config{ RPCs: RPCs{ Zevm: "http://zetacore0:8545", EVM: "http://eth:8545", Bitcoin: BitcoinRPC{ Host: "bitcoin:18443", User: "smoketest", Pass: "123", HTTPPostMode: true, DisableTLS: true, Params: Regnet, }, ZetaCoreGRPC: "zetacore0:9090", ZetaCoreRPC: "http://zetacore0:26657", Solana: "http://solana:8899", }, ZetaChainID: "athens_101-1", Contracts: Contracts{ EVM: EVM{ ERC20: "0xff3135df4F2775f4091b81f4c7B6359CfA07862a", }, + Solana: Solana{ + SPL: "", // Initialize empty for optional field + }, + ZEVM: ZEVM{ + SPLZRC20Addr: "", // Initialize empty for optional field + }, }, } }cmd/zetae2e/local/local.go (1)
416-417: Consider grouping SPL-specific tests.While the test cases are correctly added to the Solana test suite, consider grouping SPL-specific tests together for better organization and maintainability.
Apply this diff to group SPL-specific 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.TestSolanaWhitelistSPLName, + // SPL Token Tests e2etests.TestSolanaDepositSPLName, e2etests.TestSolanaDepositSPLAndCallName, }e2e/txserver/zeta_tx_server.go (1)
522-527: Verify SPL token deployment success condition.Consider adding more context to the error message to help with debugging.
- return "", "", fmt.Errorf("unable to find spl zrc20") + return "", "", fmt.Errorf("unable to find spl zrc20 for chain %d and coin type %s", + chains.SolanaLocalnet.ChainId, coin.CoinType_ERC20)e2e/e2etests/e2etests.go (2)
467-482: Consider enhancing test coverage with additional test cases.The implementation looks good and follows the established patterns. However, consider adding the following test cases to ensure comprehensive coverage:
- SPL token withdrawal test case
- SPL token deposit to restricted address test case
- SPL token deposit with refund test case (similar to other asset types)
These additional test cases would align with the test coverage patterns seen for other asset types (ETH, ERC20, SOL) in the codebase.
471-471: Standardize amount parameter description.For consistency with other asset types, consider updating the amount parameter description to follow the established pattern:
- SOL uses "amount in lamport"
- ETH uses "amount in wei"
- Current: "amount of spl tokens"
- Suggested: "amount in [appropriate SPL token unit]"
Also applies to: 479-479
changelog.md (1)
8-8: LGTM! Consider enhancing the description.The entry correctly documents the SPL deposit integration feature. However, consider expanding the description to provide more context about what SPL deposit integration entails.
Consider updating the entry to be more descriptive:
-* [3124](https://github.com/zeta-chain/node/pull/3124) - deposit spl integration +* [3124](https://github.com/zeta-chain/node/pull/3124) - Add support for parsing and processing inbound SPL token deposits on Solanae2e/runner/solana.go (2)
195-235: Optimize the use ofFindOrCreateAssociatedTokenAccountinSPLDepositAndCallThe function calls
FindOrCreateAssociatedTokenAccountfor both the PDA and the deployer. While functional, this might introduce redundant computations.Consider refactoring to minimize repeated calls and enhance efficiency:
- Verify if both accounts require separate initialization or if they can be combined.
- Cache the results if the same accounts are used multiple times within the operation.
This optimization can improve performance, especially if the function is called frequently.
271-281: Verify the success of the minting transactionAfter broadcasting the minting transaction using
BroadcastTxSync, the code logs the transaction outputs but does not explicitly check for success.Consider adding verification to ensure the minting transaction was successful:
_, out = r.BroadcastTxSync(signedTx) + require.NotNil(r, out) + require.Equal(r, uint64(0), out.Meta.Err) r.Logger.Info("mint spl logs: %v", out.Meta.LogMessages)This ensures that any errors during transaction execution are caught promptly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (21)
changelog.md(1 hunks)cmd/zetae2e/config/config.go(2 hunks)cmd/zetae2e/config/contracts.go(2 hunks)cmd/zetae2e/local/local.go(2 hunks)e2e/config/config.go(2 hunks)e2e/e2etests/e2etests.go(2 hunks)e2e/e2etests/test_solana_deposit_and_call_spl.go(1 hunks)e2e/e2etests/test_solana_deposit_spl.go(1 hunks)e2e/e2etests/test_solana_whitelist_spl.go(1 hunks)e2e/runner/runner.go(3 hunks)e2e/runner/setup_solana.go(1 hunks)e2e/runner/setup_zeta.go(2 hunks)e2e/runner/solana.go(4 hunks)e2e/txserver/zeta_tx_server.go(5 hunks)pkg/contracts/solana/gateway.go(1 hunks)pkg/contracts/solana/instruction.go(1 hunks)x/fungible/keeper/foreign_coins.go(1 hunks)x/fungible/keeper/foreign_coins_test.go(0 hunks)zetaclient/chains/solana/observer/inbound.go(3 hunks)zetaclient/chains/solana/observer/inbound_test.go(2 hunks)zetaclient/testdata/solana/chain_901_inbound_tx_result_aY8yLDze6nHSRi7L5REozKAZY1aAyPJ6TfibiqQL5JGwgSBkYux5z5JfXs5ed8LZqpXUy4VijoU3x15mBd66ZGE.json(1 hunks)
💤 Files with no reviewable changes (1)
- x/fungible/keeper/foreign_coins_test.go
✅ Files skipped from review due to trivial changes (1)
- zetaclient/testdata/solana/chain_901_inbound_tx_result_aY8yLDze6nHSRi7L5REozKAZY1aAyPJ6TfibiqQL5JGwgSBkYux5z5JfXs5ed8LZqpXUy4VijoU3x15mBd66ZGE.json
🧰 Additional context used
📓 Path-based instructions (18)
cmd/zetae2e/config/config.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetae2e/config/contracts.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
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/config/config.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_solana_deposit_and_call_spl.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_spl.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_solana_whitelist_spl.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/runner.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/setup_zeta.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.
e2e/txserver/zeta_tx_server.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/instruction.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/fungible/keeper/foreign_coins.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/observer/inbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/observer/inbound_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🪛 GitHub Check: codecov/patch
zetaclient/chains/solana/observer/inbound.go
[warning] 329-330: zetaclient/chains/solana/observer/inbound.go#L329-L330
Added lines #L329 - L330 were not covered by tests
[warning] 340-344: zetaclient/chains/solana/observer/inbound.go#L340-L344
Added lines #L340 - L344 were not covered by tests
[warning] 407-412: zetaclient/chains/solana/observer/inbound.go#L407-L412
Added lines #L407 - L412 were not covered by tests
🔇 Additional comments (20)
e2e/e2etests/test_solana_deposit_spl.go (1)
16-19: 🛠️ Refactor suggestion
Add input validation for the deposit amount.
The amount parsing lacks validation for:
- Non-negative values
- Maximum allowable deposit limits
- Potential integer overflow scenarios
Consider adding these validations:
func TestSolanaDepositSPL(r *runner.E2ERunner, args []string) {
require.Len(r, args, 1)
amount := parseInt(r, args[0])
+ require.Greater(r, amount, 0, "deposit amount must be positive")
+ require.Less(r, amount, MaxSPLDeposit, "deposit amount exceeds maximum allowed")cmd/zetae2e/config/config.go (1)
72-72: LGTM! Verify SPL-related address handling.
The SPLZRC20 address export is implemented correctly and placed logically among other ZRC20 token addresses.
Let's verify that all SPL-related configurations are consistently handled:
✅ Verification successful
Based on the search results, I can now generate a final response since I have comprehensive information about SPL-related configurations across the codebase.
SPL-related address handling is consistently implemented
The codebase shows a thorough and consistent implementation of SPL-related configurations:
- SPL token addresses are properly handled in both Solana and ZEVM contexts
- Consistent naming conventions are maintained (
SPLAddr,SPLZRC20Addr) - Complete integration in deposit and call operations
- No TODOs or FIXMEs related to SPL configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent handling of SPL-related configurations across the codebase
# Test: Search for SPL-related configurations and contract addresses
rg -i 'spl.*addr|spl.*contract' --type go
# Test: Check for any potential SPL-related TODOs or missing implementations
rg -i 'todo.*spl|fixme.*spl' --type go
Length of output: 3597
x/fungible/keeper/foreign_coins.go (1)
114-116: LGTM! The simplified comparison logic is appropriate.
The removal of Ethereum-specific validation makes the function more generic and suitable for handling various asset types, including SPL tokens. This aligns well with the PR's objective of implementing SPL token support.
Let's verify there are no duplicate asset entries for the same chain:
✅ Verification successful
Simplified comparison logic is verified safe and appropriate
The codebase has robust validation against duplicate assets for the same chain:
-
The
ErrForeignCoinAlreadyExisterror is properly enforced in multiple places:- During ERC20 whitelisting (
x/crosschain/keeper/msg_server_whitelist_erc20.go) - During gas coin deployment (
x/fungible/keeper/gas_coin_and_pool.go) - During general foreign coin deployment (
x/fungible/keeper/evm.go)
- During ERC20 whitelisting (
-
The
GetForeignCoinFromAssetfunction's simplified comparison is safe because:- It operates on a filtered list specific to the chain ID
- The uniqueness constraint is enforced at insertion time
- Test cases verify the behavior for both existing and non-existing assets
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential duplicate assets on the same chain
# Look for test cases or configurations that might create duplicate assets
rg -A 5 "SetForeignCoins.*Asset.*ForeignChainId"
Length of output: 50
Script:
#!/bin/bash
# Let's try a more comprehensive search to understand the foreign coins setup and validation
# Check for foreign coins related functions and their implementations
ast-grep --pattern 'func ($_ *Keeper) SetForeignCoins($$$)'
# Look for test cases involving foreign coins
rg -l "ForeignCoin.*Asset.*ForeignChainId" --type go
# Check for any existing foreign coins validation
rg -A 5 "ValidateForeignCoin|GetForeignCoin" --type go
Length of output: 51738
Script:
#!/bin/bash
# Let's check for any duplicate asset entries for the same chain in tests or configurations
# Look for test cases setting up foreign coins
ast-grep --pattern 'k.SetForeignCoins(ctx, $$$)'
# Check for any test assertions around duplicate assets
rg -A 5 "ErrForeignCoinAlreadyExist" --type go
# Look for validation logic around duplicate assets
ast-grep --pattern 'func ($_ *Keeper) SetForeignCoins($$$)'
Length of output: 6660
e2e/runner/setup_solana.go (1)
90-93: Consider enhancing the SPL token deployment configuration and error handling.
The SPL token deployment could benefit from the following improvements:
- Extract the token amount as a configurable constant to improve maintainability.
- Add explicit error handling for the deployment operation.
- Add a more descriptive comment explaining the purpose of this test token.
Consider applying this improvement:
+// TokenAmount defines the initial supply for test SPL tokens
+const TestSPLTokenAmount = uint64(1_000_000)
// SetupSolana sets Solana contracts and params
func (r *E2ERunner) SetupSolana(deployerPrivateKey string) {
// ... existing code ...
- // deploy test spl
- tokenAccount := r.DeploySPL(&privkey, true, uint64(1_000_000))
- r.SPLAddr = tokenAccount.PublicKey()
+ // Deploy test SPL token with initial supply for e2e testing
+ tokenAccount := r.DeploySPL(&privkey, true, TestSPLTokenAmount)
+ require.NotNil(r, tokenAccount, "SPL token deployment failed")
+ r.SPLAddr = tokenAccount.PublicKey()
+ r.Logger.Info("Deployed test SPL token at: %s", r.SPLAddr.String())
}Additionally, please verify if the deployment should occur after the chain parameters setup, as this ordering might have implications for the e2e test flow.
pkg/contracts/solana/instruction.go (1)
37-47: LGTM! Structure and documentation are well-defined.
The new DepositSPLInstructionParams type is well-structured and properly documented, following the established patterns in the codebase.
cmd/zetae2e/config/contracts.go (1)
142-151: LGTM! The SPLZRC20 setup follows best practices.
The implementation:
- Follows consistent error handling pattern
- Properly validates address conversion
- Creates contract instance with appropriate error checks
e2e/runner/setup_zeta.go (1)
178-182: LGTM! Verify SPL address handling across the codebase.
The addition of SPL address parameter follows the established pattern. Let's verify the SPL address handling in related files.
✅ Verification successful
Let me analyze the SPL address handling in the codebase to verify if there are any inconsistencies or issues.
Let me analyze the SPL address handling further by looking at the deployment and initialization patterns.
Based on the analysis of the SPL address handling in the codebase, I can now provide a conclusive response about the review comment.
SPL address handling is consistent and properly implemented
The code shows proper handling of SPL addresses across the codebase with:
- Consistent address conversion using
String()method for SPL addresses - Proper deployment and initialization flow in
DeploySPLandDeployZRC20s - Appropriate error handling for SPL-related operations
- Comprehensive test coverage for SPL functionality
- Proper validation of addresses during deployment and transactions
The changes in setup_zeta.go follow the established patterns for handling SPL addresses, where:
- The SPL address is properly converted to string format using
String() - The address is passed to
DeployZRC20salong with other required parameters - Error handling is in place for the deployment process
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify SPL address handling in related files
# Look for potential SPL address usage patterns to ensure consistent handling
# Search for SPL address related patterns
rg -i "spl.*addr|addr.*spl" --type go
# Search for String() method calls on addresses to find similar patterns
rg "\.String\(\)" --type go -C 2
Length of output: 461096
Script:
#!/bin/bash
# Search for SPL address usage in function signatures and method definitions to understand the handling patterns
ast-grep --pattern 'func $FUNC($ARGS) $RET {
$$$
$ADDR.String()
$$$
}'
# Search for SPL address assignments and comparisons
rg -g '*.go' -B 2 -A 2 'SPLAddr.*='
rg -g '*.go' -B 2 -A 2 'SPL.*String\(\)'
# Look for SPL address validation patterns
rg -g '*.go' 'validate.*SPL|check.*SPL|verify.*SPL'
Length of output: 1946
Script:
#!/bin/bash
# Look for SPL deployment and initialization patterns
rg -g '*.go' -B 5 -A 5 'DeployZRC20s.*SPL'
rg -g '*.go' -B 5 -A 5 'DeploySPL'
# Check for SPL token validation and error handling
rg -g '*.go' -B 3 -A 3 'SPL.*error|err.*SPL'
# Look for SPL-related tests
rg -g '*_test.go' 'SPL|spl'
Length of output: 8601
zetaclient/chains/solana/observer/inbound_test.go (1)
5-5: LGTM: Import addition is appropriate.
The encoding/hex import is correctly placed and necessary for decoding the expected memo in the new SPL deposit test.
e2e/config/config.go (2)
122-123: LGTM: SPL field addition to Solana struct
The new SPL field is properly defined using DoubleQuotedString type and follows the established pattern for address fields.
145-145: LGTM: SPLZRC20Addr field addition to ZEVM struct
The new SPLZRC20Addr field follows consistent naming convention with other ZRC20 address fields and uses appropriate type.
e2e/runner/runner.go (1)
372-373: LGTM: Consistent logging format maintained.
The new logging statements for SPL-related addresses follow the established pattern and are placed in appropriate sections, maintaining code consistency.
Also applies to: 381-381
cmd/zetae2e/local/local.go (1)
221-224: LGTM! Clean integration of Solana setup.
The conditional block for Solana setup is well-placed and follows the existing pattern of the codebase.
e2e/txserver/zeta_tx_server.go (4)
384-388: Function signature change looks good.
The updated signature properly accommodates both ERC20 and SPL addresses as parameters, with a clear return type that matches the function's enhanced functionality.
392-404: Error handling is comprehensive and consistent.
The error handling has been properly updated to return empty strings for both return values in error cases, maintaining consistency with the new function signature. The error messages are descriptive and properly wrapped with additional context.
Also applies to: 414-414, 497-497, 502-502, 511-511, 519-520
452-461: SPL token deployment configuration looks good.
The SPL token deployment message is properly configured with:
- Correct chain ID (SolanaLocalnet)
- Appropriate decimal places (9) for Solana tokens
- Proper coin type (ERC20)
- Reasonable gas limit (100000)
530-530: Return statement is consistent with function signature.
The order of return values matches the function signature, returning both ERC20 and SPL ZRC20 contract addresses.
e2e/e2etests/e2etests.go (1)
64-65: LGTM: Constants follow naming conventions.
The new constants for SPL token tests maintain consistency with existing naming patterns and provide clear, descriptive identifiers.
e2e/runner/solana.go (2)
64-92: 'CreateWhitelistSPLMintInstruction' function is well-implemented
The function correctly constructs the whitelist_spl_mint instruction with appropriate parameters and account metadata.
94-130: 'CreateDepositSPLInstruction' function is properly structured
The function effectively assembles the deposit_spl instruction with correct account information and parameter usage.
zetaclient/chains/solana/observer/inbound.go (1)
415-416: Verify assumption about signer index in AccountKeys
The code assumes that the signer is always at tx.Message.AccountKeys[0]. This might not hold true in all cases, potentially leading to incorrect signer identification.
To confirm this assumption, you can analyze the transaction structures:
If the assumption does not hold, consider retrieving the signer dynamically, similar to GetSignerDeposit.
|
fixed the comments please recheck i see some unit tests miss for observing inbound, but also for current implementation, I suggest i revisit whole solana unit test state in zetaclient, and add whats missing after i finish withdraw spl @lumtis what do you say? |
Co-authored-by: Lucas Bertrand <[email protected]>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
@gartnera i used consensus breaking ack label because of failing consensus tests on merge, since the change in foreign_coins.go is making a difference in gas calculation please let me know if im using this correctly |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Yes |
| @@ -16,7 +16,11 @@ const ( | |||
|
|
|||
| // AccountsNumberOfDeposit is the number of accounts required for Solana gateway deposit instruction | |||
There was a problem hiding this comment.
Nit: Outdated AccountsNumberOfDeposit, should be accountsNumDeposit
Description
Part of #2938
How Has This Been Tested?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Documentation