test: extend solana unit tests #3158
Conversation
skosito
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
swift1337 and
fbac
as code owners
|
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📝 WalkthroughWalkthroughThe pull request encompasses a series of changes primarily focused on renaming variables and parameters related to SPL token management within the Solana ecosystem. Key modifications include changing instances of 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 #3158 +/- ##
===========================================
+ Coverage 62.67% 62.84% +0.16%
===========================================
Files 424 424
Lines 30128 30140 +12
===========================================
+ Hits 18884 18942 +58
+ Misses 10402 10360 -42
+ Partials 842 838 -4
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (14)
pkg/contracts/solana/gateway_message_test.go (2)
51-60: Consider enhancing test coverage with table-driven tests.While the test setup is well-structured, consider implementing table-driven tests to cover multiple scenarios and edge cases. This would make it easier to add new test cases and improve maintainability.
Example refactor:
func Test_MsgWithdrawSPLHash(t *testing.T) { - t.Run("should pass for archived inbound, receipt and cctx", func(t *testing.T) { + tests := []struct { + name string + chainID uint64 + nonce uint64 + amount uint64 + mintAccount solana.PublicKey + to solana.PublicKey + wantHash string + }{ + { + name: "should pass for archived inbound", + chainID: uint64(chains.SolanaLocalnet.ChainId), + nonce: 0, + amount: 1336000, + mintAccount: solana.MustPublicKeyFromBase58("AS48jKNQsDGkEdDvfwu1QpqjtqbCadrAq9nGXjFmdX3Z"), + to: solana.MustPublicKeyFromBase58("37yGiHAnLvWZUNVwu9esp74YQFqxU1qHCbABkDvRddUQ"), + wantHash: "87fa5c0ed757c6e1ea9d8976537eaf7868bc1f1bbf55ab198a01645d664fe0ae", + }, + // Add more test cases here + }
67-67: Document or parameterize the hardcoded decimals value.The decimals value of 8 is hardcoded in the
NewMsgWithdrawSPLcall. Consider documenting why this specific value is used or making it configurable through test parameters.- hash := contracts.NewMsgWithdrawSPL(chainID, nonce, amount, 8, mintAccount, to, toAta).Hash() + const tokenDecimals = 8 // Standard decimals for SPL token X + hash := contracts.NewMsgWithdrawSPL(chainID, nonce, amount, tokenDecimals, mintAccount, to, toAta).Hash()pkg/contracts/solana/inbound.go (2)
105-106: Consider extracting magic number into a named constantThe account count validation uses a magic number through
accountsNumDeposit. Consider defining this as a documented constant at package level for better maintainability and self-documentation.const ( MaxSignaturesPerTicker = 100 + // AccountsNumDeposit is the number of accounts required for a deposit instruction + // [signer, pda, system_program] + AccountsNumDeposit = 3 )
129-133: Consider extracting magic number into a named constantSimilar to the previous suggestion, consider defining
accountsNumberDepositSPLas a documented constant at package level.const ( MaxSignaturesPerTicker = 100 + // AccountsNumberDepositSPL is the number of accounts required for a deposit SPL instruction + // [signer, pda, whitelist_entry, mint_account, token_program, from, to] + AccountsNumberDepositSPL = 7 )pkg/contracts/solana/inbound_test.go (1)
71-124: LGTM: Comprehensive error case coverage with room for improvementThe new test cases effectively cover important validation scenarios:
- Discriminator validation
- Account count validation
- Signer requirement validation
Consider enhancing the error validation by checking specific error messages:
deposit, err := ParseInboundAsDeposit(tx, 0, txResult.Slot) require.Error(t, err) +require.Contains(t, err.Error(), "expected 3 accounts") require.Nil(t, deposit)This would ensure that the correct validation is failing for the expected reason.
pkg/contracts/solana/instruction.go (3)
131-131: Add compile-time interface compliance checkAdd a compile-time check to ensure
WithdrawSPLInstructionParamsimplements theOutboundInstructioninterface.Add this line before the struct definition:
+var _ OutboundInstruction = (*WithdrawSPLInstructionParams)(nil)
Line range hint
192-194: Improve error message specificityThe error message should specifically mention that this is for SPL withdrawal instructions.
- return nil, fmt.Errorf("not a withdraw instruction: %v", inst.Discriminator) + return nil, fmt.Errorf("not a withdraw SPL instruction: %v", inst.Discriminator)
131-131: Enhance documentation for the Decimals fieldThe current comment could be more descriptive about the purpose and usage of the Decimals field.
- // Decimals is decimals for spl token + // Decimals represents the number of decimal places used by the SPL token. + // For example, if Decimals is 6, then an amount of 1000000 represents 1 token.e2e/runner/solana.go (4)
Line range hint
166-177: Enhance error handling in ATA resolutionWhile the function correctly handles ATA creation, consider improving error handling for production readiness.
func (r *E2ERunner) ResolveSolanaATA( payer solana.PrivateKey, owner solana.PublicKey, mintAccount solana.PublicKey, ) solana.PublicKey { pdaAta, _, err := solana.FindAssociatedTokenAddress(owner, mintAccount) require.NoError(r, err) - info, _ := r.SolanaClient.GetAccountInfo(r.Ctx, pdaAta) + info, err := r.SolanaClient.GetAccountInfo(r.Ctx, pdaAta) + require.NoError(r, err) + if info != nil { return pdaAta }
205-207: Extract whitelist seed construction to a constantConsider extracting the whitelist seed construction to improve maintainability and reusability.
+const WhitelistSeedPrefix = "whitelist" func (r *E2ERunner) SPLDepositAndCall(...) solana.Signature { - seed := [][]byte{[]byte("whitelist"), mintAccount.Bytes()} + seed := [][]byte{[]byte(WhitelistSeedPrefix), mintAccount.Bytes()} whitelistEntryPDA, _, err := solana.FindProgramAddress(seed, r.GatewayProgram)
265-266: Extract magic number for initial mint amountConsider extracting the hardcoded mint amount to a named constant for better maintainability.
+const InitialMintAmount = 1_000_000_000 - mintToInstruction := token.NewMintToInstruction(uint64(1_000_000_000), mintAccount.PublicKey(), ata, privateKey.PublicKey(), []solana.PublicKey{}). + mintToInstruction := token.NewMintToInstruction(uint64(InitialMintAmount), mintAccount.PublicKey(), ata, privateKey.PublicKey(), []solana.PublicKey{}).
Line range hint
279-288: Extract whitelist verification logicConsider extracting the whitelist verification logic into a separate function for better code organization.
+func (r *E2ERunner) isTokenWhitelisted(mintAccount solana.PublicKey) (bool, solana.PublicKey, error) { + seed := [][]byte{[]byte("whitelist"), mintAccount.Bytes()} + whitelistEntryPDA, _, err := solana.FindProgramAddress(seed, r.GatewayProgram) + if err != nil { + return false, solana.PublicKey{}, err + } + + whitelistEntryInfo, err := r.SolanaClient.GetAccountInfo(r.Ctx, whitelistEntryPDA) + return whitelistEntryInfo != nil, whitelistEntryPDA, err +} func (r *E2ERunner) DeploySPL(privateKey *solana.PrivateKey, whitelist bool) *solana.Wallet { // ... existing code ... if whitelist { - seed := [][]byte{[]byte("whitelist"), mintAccount.PublicKey().Bytes()} - whitelistEntryPDA, _, err := solana.FindProgramAddress(seed, r.GatewayProgram) - require.NoError(r, err) - - whitelistEntryInfo, err := r.SolanaClient.GetAccountInfo(r.Ctx, whitelistEntryPDA) + isWhitelisted, whitelistEntryPDA, err := r.isTokenWhitelisted(mintAccount.PublicKey()) require.Error(r, err) - if whitelistEntryInfo != nil { + if isWhitelisted { return mintAccount }zetaclient/chains/solana/observer/outbound_test.go (2)
364-390: Consider adding edge case tests.While the happy path test is comprehensive, consider adding test cases for:
- Zero token amount
- Maximum possible decimals (9 for SPL tokens)
- Maximum possible nonce value
364-424: Consider refactoring to table-driven tests.The test could be more maintainable using table-driven tests, consistent with Go's testing best practices. This would make it easier to add new test cases and reduce code duplication.
Example refactor:
func Test_ParseInstructionWithdrawSPL(t *testing.T) { - // the test chain and transaction hash - chain := chains.SolanaDevnet - txHash := withdrawSPLTxTest - txAmount := uint64(1000000) + tests := []struct { + name string + txHash string + modifyInst func(*solana.CompiledInstruction) + expectedErr string + expectedInst *contracts.InstructionWithdrawSPL + }{ + { + name: "successful parsing", + txHash: withdrawSPLTxTest, + expectedInst: &contracts.InstructionWithdrawSPL{ + TokenAmount: 1000000, + Decimals: 6, + Nonce: 3, + }, + }, + { + name: "invalid instruction data", + txHash: withdrawSPLTxTest, + modifyInst: func(inst *solana.CompiledInstruction) { + inst.Data = []byte("invalid instruction data") + }, + expectedErr: "error deserializing instruction", + }, + // Add more test cases here + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + txResult := testutils.LoadSolanaOutboundTxResult(t, TestDataDir, chains.SolanaDevnet.ChainId, tt.txHash) + tx, err := txResult.Transaction.GetTransaction() + require.NoError(t, err) + + instruction := tx.Message.Instructions[0] + if tt.modifyInst != nil { + tt.modifyInst(&instruction) + } + + inst, err := contracts.ParseInstructionWithdrawSPL(instruction) + if tt.expectedErr != "" { + require.ErrorContains(t, err, tt.expectedErr) + require.Nil(t, inst) + return + } + + require.NoError(t, err) + require.Equal(t, tt.expectedInst.TokenAmount, inst.TokenAmount) + require.Equal(t, tt.expectedInst.Decimals, inst.Decimals) + require.Equal(t, tt.expectedInst.Nonce, inst.GatewayNonce()) + }) + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (11)
e2e/runner/setup_solana.go(1 hunks)e2e/runner/solana.go(7 hunks)pkg/contracts/solana/gateway.go(1 hunks)pkg/contracts/solana/gateway_message.go(4 hunks)pkg/contracts/solana/gateway_message_test.go(1 hunks)pkg/contracts/solana/inbound.go(1 hunks)pkg/contracts/solana/inbound_test.go(3 hunks)pkg/contracts/solana/instruction.go(1 hunks)zetaclient/chains/solana/observer/outbound_test.go(2 hunks)zetaclient/chains/solana/signer/withdraw_spl.go(3 hunks)zetaclient/testdata/solana/chain_901_outbound_tx_result_3NgoR4K9FJq7UunorPRGW9wpqMV8oNvZERejutd7bKmqh3CKEV5DMZndhZn7hQ1i4RhTyHXRWxtR5ZNVHmmjAUSF.json(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- e2e/runner/setup_solana.go
- pkg/contracts/solana/gateway.go
- zetaclient/testdata/solana/chain_901_outbound_tx_result_3NgoR4K9FJq7UunorPRGW9wpqMV8oNvZERejutd7bKmqh3CKEV5DMZndhZn7hQ1i4RhTyHXRWxtR5ZNVHmmjAUSF.json
🧰 Additional context used
📓 Path-based instructions (8)
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_message.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_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
pkg/contracts/solana/inbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
pkg/contracts/solana/inbound_test.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_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/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_message.go
[warning] 179-180: pkg/contracts/solana/gateway_message.go#L179-L180
Added lines #L179 - L180 were not covered by tests
pkg/contracts/solana/inbound.go
[warning] 101-102: pkg/contracts/solana/inbound.go#L101-L102
Added lines #L101 - L102 were not covered by tests
[warning] 125-126: pkg/contracts/solana/inbound.go#L125-L126
Added lines #L125 - L126 were not covered by tests
zetaclient/chains/solana/signer/withdraw_spl.go
[warning] 43-43: zetaclient/chains/solana/signer/withdraw_spl.go#L43
Added line #L43 was not covered by tests
[warning] 49-49: zetaclient/chains/solana/signer/withdraw_spl.go#L49
Added line #L49 was not covered by tests
[warning] 51-51: zetaclient/chains/solana/signer/withdraw_spl.go#L51
Added line #L51 was not covered by tests
[warning] 55-55: zetaclient/chains/solana/signer/withdraw_spl.go#L55
Added line #L55 was not covered by tests
[warning] 88-88: zetaclient/chains/solana/signer/withdraw_spl.go#L88
Added line #L88 was not covered by tests
[warning] 90-90: zetaclient/chains/solana/signer/withdraw_spl.go#L90
Added line #L90 was not covered by tests
[warning] 93-93: zetaclient/chains/solana/signer/withdraw_spl.go#L93
Added line #L93 was not covered by tests
[warning] 95-95: zetaclient/chains/solana/signer/withdraw_spl.go#L95
Added line #L95 was not covered by tests
[warning] 105-105: zetaclient/chains/solana/signer/withdraw_spl.go#L105
Added line #L105 was not covered by tests
🔇 Additional comments (11)
pkg/contracts/solana/inbound.go (1)
97-145: Overall improvements to account resolution look good
The changes to both functions improve the robustness of account resolution and validation:
- Using
ResolveInstructionAccountsinstead of direct access - Clear validation of account counts
- Explicit signer checks
- Well-documented account ordering
The code is now more maintainable and secure.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 101-102: pkg/contracts/solana/inbound.go#L101-L102
Added lines #L101 - L102 were not covered by tests
[warning] 125-126: pkg/contracts/solana/inbound.go#L125-L126
Added lines #L125 - L126 were not covered by tests
zetaclient/chains/solana/signer/withdraw_spl.go (3)
42-55: LGTM: Improved terminology accuracy
The renaming from tokenAccount to mintAccount better aligns with Solana's terminology, where mint accounts represent the token definition. This change enhances code clarity and domain accuracy.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 43-43: zetaclient/chains/solana/signer/withdraw_spl.go#L43
Added line #L43 was not covered by tests
[warning] 49-49: zetaclient/chains/solana/signer/withdraw_spl.go#L49
Added line #L49 was not covered by tests
[warning] 51-51: zetaclient/chains/solana/signer/withdraw_spl.go#L51
Added line #L51 was not covered by tests
[warning] 55-55: zetaclient/chains/solana/signer/withdraw_spl.go#L55
Added line #L55 was not covered by tests
105-105: LGTM: Correct account metadata setup
The mint account is correctly included in the instruction's account metadata list with appropriate permissions.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 105-105: zetaclient/chains/solana/signer/withdraw_spl.go#L105
Added line #L105 was not covered by tests
Line range hint 128-132: Consider implementing dynamic compute budget
The TODO comment indicates a potential optimization for handling transaction fees. While the current fixed fee approach works, implementing dynamic compute budgets could improve transaction reliability during network congestion.
Let's check if this issue is being tracked:
✅ Verification successful
Dynamic compute budget optimization is actively tracked
The concern is valid and already being tracked in issue #2599 "Explorer Solana priority fee and compute budget for outbound." The TODO comment accurately reflects the current fixed fee of 5K lamports and the need for optimization through priority fees and compute budgets.
- Current implementation in
withdraw_spl.gouses a fixed fee approach - Issue Explorer Solana priority fee and compute budget for outbound. #2599 is in an OPEN state and properly labeled with "zetaclient" and "chain:solana"
- The optimization path through compute budget parameters is already outlined in the code comments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if the compute budget issue is being tracked
gh issue view 2599 --json title,state,labels
Length of output: 354
Script:
#!/bin/bash
# Check the content of the file for the TODO comment and its context
rg -A 5 "TODO" zetaclient/chains/solana/signer/withdraw_spl.go
Length of output: 395
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 43-43: zetaclient/chains/solana/signer/withdraw_spl.go#L43
Added line #L43 was not covered by tests
[warning] 49-49: zetaclient/chains/solana/signer/withdraw_spl.go#L49
Added line #L49 was not covered by tests
[warning] 51-51: zetaclient/chains/solana/signer/withdraw_spl.go#L51
Added line #L51 was not covered by tests
[warning] 55-55: zetaclient/chains/solana/signer/withdraw_spl.go#L55
Added line #L55 was not covered by tests
pkg/contracts/solana/inbound_test.go (2)
11-13: LGTM: Imports are well-organized and necessary
The added imports for Solana and Borsh libraries are required for the new test cases and follow good organization practices.
Line range hint 19-33: Verify test data availability
The tests load transaction data from JSON files, but these files are not visible in the review.
✅ Verification successful
The previous attempt showed that the specific JSON files weren't found. Let's search more broadly to understand what test data files exist and how they are referenced in the tests.
Test data files are present and correctly referenced
The test data files mentioned in the code are present in the repository:
pkg/contracts/solana/testdata/MS3MPLN7hkbyCZFwKqXcg8fmEvQMD74fN6Ps2LSWXJoRxPW5ehaxBorK9q1JFVbqnAvu9jXm6ertj7kT7HpYw1j.jsonpkg/contracts/solana/testdata/aY8yLDze6nHSRi7L5REozKAZY1aAyPJ6TfibiqQL5JGwgSBkYux5z5JfXs5ed8LZqpXUy4VijoU3x15mBd66ZGE.json
The files are also correctly referenced in the test code at pkg/contracts/solana/inbound_test.go using the LoadObjectFromJSONFile function.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if test data files exist
ls testdata/MS3MPLN7hkbyCZFwKqXcg8fmEvQMD74fN6Ps2LSWXJoRxPW5ehaxBorK9q1JFVbqnAvu9jXm6ertj7kT7HpYw1j.json \
testdata/aY8yLDze6nHSRi7L5REozKAZY1aAyPJ6TfibiqQL5JGwgSBkYux5z5JfXs5ed8LZqpXUy4VijoU3x15mBd66ZGE.json
# Check JSON validity
for f in testdata/*.json; do
if [ -f "$f" ]; then
jq '.' "$f" >/dev/null
fi
done
Length of output: 564
Script:
#!/bin/bash
# Let's check if there are any JSON files in the testdata directory or its subdirectories
fd -e json -t f
# Also check if there are any references to JSON test files in the test code
rg -g "*.go" "testdata.*\.json" -A 2 -B 2
Length of output: 17846
pkg/contracts/solana/gateway_message.go (4)
122-123: LGTM: Improved terminology accuracy
The rename from tokenAccount to mintAccount better reflects the field's purpose in representing the SPL token's mint address.
142-142: LGTM: Constructor changes align with field renaming
The parameter and field assignment changes maintain consistency with the struct field renaming.
Also applies to: 150-150
179-180: Add test coverage for the MintAccount getter
While the getter implementation is correct, it lacks test coverage according to the code coverage report. Since this PR focuses on extending Solana unit tests, we should ensure this new method is tested.
#!/bin/bash
# Check if there are any existing tests for MintAccount getter
rg -l "func.*Test.*MintAccount" .Would you like me to help generate a test case for this getter method?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 179-180: pkg/contracts/solana/gateway_message.go#L179-L180
Added lines #L179 - L180 were not covered by tests
203-203: Verify hash compatibility with existing data
The hash calculation now uses mintAccount instead of tokenAccount. While this change is consistent with the renaming, please verify that this doesn't affect compatibility with any existing stored hashes in the system.
✅ Verification successful
Change is safe - test vectors validate hash calculation
The hash calculation change from tokenAccount to mintAccount is validated by existing test vectors in Test_MsgWithdrawSPLHash. The test explicitly verifies the hash using the mintAccount parameter against a known hash value, confirming that this is the intended behavior rather than a breaking change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for stored hashes or hash comparisons in tests
rg -A 2 "withdraw_spl_token.*[Hh]ash"
Length of output: 35
Script:
#!/bin/bash
# Search for tests involving hash verification or comparison
rg -A 5 "Hash\(\)" pkg/contracts/solana/
Length of output: 4760
Script:
#!/bin/bash
# Search for any stored or hardcoded hashes in tests
rg -A 2 "hash.*:.*[\"'].*[\"']" pkg/contracts/solana/
Length of output: 1033
Script:
#!/bin/bash
# Look for test files containing both hash and mint/token account references
rg -l "(Hash|hash).*mint.*account|mint.*account.*(Hash|hash)" pkg/contracts/solana/
Length of output: 85
Script:
#!/bin/bash
# Check if there are any test vectors or fixtures containing hashes
fd -e json -e yaml -x grep -l "hash" {}
Length of output: 6857
Script:
#!/bin/bash
# Search for any hash-related test assertions
rg -A 2 "assert.*[Hh]ash" pkg/contracts/solana/
Length of output: 49
Script:
#!/bin/bash
# Search for test vectors specifically for MsgWithdrawSPL
rg -A 10 "Test_MsgWithdrawSPLHash" pkg/contracts/solana/gateway_message_test.go
Length of output: 619
Script:
#!/bin/bash
# Look for any stored hash test vectors
rg -A 5 "wantHashBytes.*:=" pkg/contracts/solana/gateway_message_test.go
Length of output: 839
zetaclient/chains/solana/observer/outbound_test.go (1)
41-43: LGTM: Well-documented test constant.
The constant follows the established pattern and includes a descriptive comment.
Description
tokenAccountwas used instead ofmintAccountHow Has This Been Tested?
Summary by CodeRabbit
Release Notes
New Features
Improvements
tokenAccounttomintAccountacross multiple components for clarity.Documentation