feat: add Whitelist message ability to whitelist SPL tokens on Solana #2984
Conversation
|
@lumtis @ws4charlie could you please do initial review for this draft? i left some questions and open points, thanks |
|
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 introduces significant updates to the ZetaChain node in version 21.0.0, including new features such as stateful precompiled contracts, an RPC package for zetacored, and enhancements for Solana and Bitcoin support. The changes also expand testing capabilities, particularly for Solana's SPL token whitelisting. Refactoring efforts focus on improving code quality and test coverage. Additionally, various modules have been updated to enhance functionality and stability, alongside modifications to dependencies and JSON structures related to transaction processing. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🧹 Outside diff range and nitpick comments (27)
pkg/contracts/solana/gateway_message_test.go (2)
Line range hint
16-17: Test case name could be more descriptive.The current test case name "should pass for archived inbound, receipt and cctx" doesn't clearly reflect what's being tested. Consider renaming to better describe the specific scenario being validated.
- t.Run("should pass for archived inbound, receipt and cctx", func(t *testing.T) { + t.Run("should generate correct hash for withdraw message with valid parameters", func(t *testing.T) {
Line range hint
18-29: Consider expanding test coverage with table-driven tests.The current test only validates a single scenario. Consider implementing table-driven tests to cover multiple cases including edge cases (e.g., zero amount, maximum values).
Here's a suggested implementation:
func Test_MsgWithdrawHash(t *testing.T) { tests := []struct { name string chainID uint64 nonce uint64 amount uint64 to solana.PublicKey want string }{ { name: "valid parameters", chainID: uint64(chains.SolanaLocalnet.ChainId), nonce: 0, amount: 1336000, to: solana.MustPublicKeyFromBase58("37yGiHAnLvWZUNVwu9esp74YQFqxU1qHCbABkDvRddUQ"), want: "aa609ef9480303e8d743f6e36fe1bea0cc56b8d27dcbd8220846125c1181b681", }, // Add more test cases here } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { wantHashBytes, err := hex.DecodeString(tt.want) require.NoError(t, err) hash := contracts.NewMsgWithdraw(tt.chainID, tt.nonce, tt.amount, tt.to).Hash() require.True(t, bytes.Equal(hash[:], wantHashBytes)) }) } }cmd/zetae2e/local/solana.go (1)
Line range hint
1-63: Consider enhancing test result reporting.While the function's structure and error handling are solid, the test reporting could be more detailed for debugging purposes.
Consider enhancing the test reporting by adding more detailed metrics:
if err := solanaRunner.RunE2ETests(testsToRun); err != nil { return fmt.Errorf("solana tests failed: %v", err) } +// Add detailed test metrics +solanaRunner.Logger.Print("📊 Test Summary:") +solanaRunner.Logger.Print(" • Total tests run: %d", len(testsToRun)) +solanaRunner.Logger.Print(" • Duration: %s", time.Since(startTime).String())x/crosschain/types/message_whitelist_erc20.go (2)
Line range hint
1-67: Consider documenting the multi-asset support.The message type and its methods now support multiple asset types beyond ERC20, but this isn't reflected in the documentation or type names.
Add documentation to clarify the broader scope:
+// MsgWhitelistERC20 defines a message to whitelist various asset types including but not limited to +// ERC20 tokens and SPL tokens. The name remains ERC20-specific for backward compatibility but will +// be updated in future versions. type MsgWhitelistERC20 struct {
Line range hint
1-67: Future refactoring consideration.As mentioned in the PR objectives, the ERC20-specific naming should be updated to reflect the multi-asset support. Consider creating a tracking issue for this technical debt.
Would you like me to create a GitHub issue to track the following tasks?
- Rename
MsgWhitelistERC20toMsgWhitelistAsset- Update all related type names and method signatures
- Migrate existing code to use the new naming
e2e/e2etests/test_solana_whitelist_spl.go (2)
3-12: Consider organizing imports following standard Go conventions.Consider grouping imports into standard library, external dependencies, and internal packages, separated by blank lines:
import ( + "context" + "github.com/gagliardetto/solana-go" "github.com/stretchr/testify/require" + "github.com/zeta-chain/node/e2e/runner" "github.com/zeta-chain/node/e2e/txserver" "github.com/zeta-chain/node/e2e/utils" "github.com/zeta-chain/node/pkg/chains" crosschaintypes "github.com/zeta-chain/node/x/crosschain/types" )
14-22: Add comprehensive test documentation and improve error handling.The test function would benefit from a detailed documentation block explaining:
- Purpose of the test
- Prerequisites
- Expected outcomes
- Any specific setup requirements
Also, consider adding more descriptive error messages:
func TestSolanaWhitelistSPL(r *runner.E2ERunner, _ []string) { + // TestSolanaWhitelistSPL verifies the SPL token whitelisting process by: + // 1. Deploying a new SPL token + // 2. Verifying no existing whitelist entry + // 3. Whitelisting the token + // 4. Confirming successful whitelisting + // Prerequisites: Solana local network must be running privkey, err := solana.PrivateKeyFromBase58(r.Account.SolanaPrivateKey.String()) - require.NoError(r, err) + require.NoError(r, err, "failed to load Solana private key")pkg/contracts/solana/gateway.go (1)
Line range hint
1-1: Fix typo in package documentationThere's a typo in the package documentation: "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.x/crosschain/types/message_whitelist_erc20_test.go (2)
35-38: Enhance test case description for better clarity.The test case name "invalid asset" could be more descriptive about what makes the asset invalid. Consider renaming to "empty asset address" to better reflect the specific validation being tested.
- name: "invalid asset", + name: "empty asset address",
Line range hint
35-44: Consider adding SPL-specific test cases.Given that this PR introduces SPL token support, consider adding test cases that validate SPL-specific addresses and scenarios. This would ensure the validation logic works correctly for both ERC20 and SPL tokens.
Example addition:
{ name: "valid SPL token address", msg: types.NewMsgWhitelistERC20( sample.AccAddress(), "TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA", // Example SPL token program ID 1, "name", "symbol", 6, 10, ), error: false, },zetaclient/testutils/constant.go (2)
45-47: Consider implementing a more robust configuration management system.While the comment explains the current setup with different deployer keys for development and production, maintaining such sensitive information in code presents potential security risks and maintenance challenges.
Consider implementing:
- Environment-based configuration management
- Secure secret management system integration
- Clear documentation of the migration plan to unify the key pairs
This would provide:
- Better security through proper secret management
- Easier maintenance and deployment across environments
- Clear separation of configuration from code
47-47: Document the gateway address format and validation requirements.The gateway address appears to be a Solana public key. To prevent potential issues during testing:
- Add a comment describing the expected format
- Consider adding validation helpers in the test utilities
- Include information about how this address was generated
Example comment format:
var GatewayAddresses = map[int64]string{ + // Solana devnet gateway address (Base58 encoded public key) + // Generated using [tool/method] for development testing chains.SolanaDevnet.ChainId: "94U5AHQMKkV5txNJ17QPXWoh474PheGou6cNP2FEuL1d", }e2e/runner/setup_solana.go (1)
64-64: LGTM! Consider adding documentation for the new parameter.The addition of the empty private keys slice parameter aligns with the gateway program updates for SPL token whitelisting. While the change is correct, adding a comment explaining the purpose of this parameter would improve maintainability.
Add a comment above the line explaining the role of additional signers:
+ // Create and sign transaction with no additional signers for gateway initialization signedTx := r.CreateSignedTransaction([]solana.Instruction{&inst}, privkey, []solana.PrivateKey{})pkg/contracts/solana/gateway_message.go (1)
141-144: Method name inconsistency with Go conventions.The method name
WhitelistCandidate()doesn't follow Go's getter naming convention. In Go, getters typically don't include the "get" prefix or the field name directly.- func (msg *MsgWhitelist) WhitelistCandidate() solana.PublicKey { + func (msg *MsgWhitelist) Candidate() solana.PublicKey { return msg.whitelistCandidate }pkg/contracts/solana/instruction.go (1)
122-175: Consider reducing code duplication with WithdrawInstructionParams.There's significant overlap between
WhitelistInstructionParamsandWithdrawInstructionParams. Consider extracting common fields and methods into a shared base struct.Consider refactoring to:
type BaseInstructionParams struct { Discriminator [8]byte Signature [64]byte RecoveryID uint8 MessageHash [32]byte Nonce uint64 } type WhitelistInstructionParams struct { BaseInstructionParams } type WithdrawInstructionParams struct { BaseInstructionParams Amount uint64 }This would reduce duplication and make the code more maintainable as more SPL integrations are added.
go.mod (1)
Line range hint
1-406: Consider updating Go version requirement.The module requires Go 1.22.7 but uses toolchain 1.22.8. Consider updating the
godirective to match the toolchain version for consistency.-go 1.22.7 +go 1.22.8cmd/zetae2e/local/local.go (2)
408-408: LGTM! Consider adding a descriptive comment.The addition of
TestSolanaWhitelistSPLNameto the Solana test suite is well-placed. Consider adding a comment to document the test's purpose and any prerequisites.solanaTests := []string{ e2etests.TestSolanaDepositName, e2etests.TestSolanaWithdrawName, e2etests.TestSolanaDepositAndCallName, e2etests.TestSolanaDepositAndCallRefundName, e2etests.TestSolanaDepositRestrictedName, e2etests.TestSolanaWithdrawRestrictedName, + // Test SPL token whitelisting functionality e2etests.TestSolanaWhitelistSPLName, }
Line range hint
142-143: Consider prioritizing the TODO for better maintainability.The TODO comment about simplifying this file is becoming more relevant with the addition of new tests. Consider breaking down the test suites into separate files based on blockchain type (e.g.,
solana_tests.go,bitcoin_tests.go). This would:
- Improve maintainability
- Make test dependencies clearer
- Facilitate easier addition of new test categories
- Enhance code organization
pkg/contracts/solana/gateway.json (2)
Line range hint
369-721: Consider adding instruction documentation.While the new instructions are well-structured, they would benefit from documentation explaining:
- The purpose and intended use of each instruction
- The required permissions and authorization flow
- The expected behavior and side effects
1002-1011: Enhance error messages for better debugging.Consider providing more descriptive error messages to aid in debugging:
{ "code": 6008, "name": "DepositPaused", - "msg": "DepositPaused" + "msg": "Deposits are currently paused by the authority" }, { "code": 6009, "name": "SPLAtaAndMintAddressMismatch", - "msg": "SPLAtaAndMintAddressMismatch" + "msg": "The provided ATA does not match the specified mint address" }changelog.md (1)
30-30: Consider enhancing the changelog entry with more details.The current entry "whitelist spl tokens" could be more descriptive. Consider expanding it to include:
- The purpose of the whitelist feature
- Any notable implementation details
- Impact on existing functionality
Example:
-* [2984](https://github.com/zeta-chain/node/pull/2984) - whitelist spl tokens +* [2984](https://github.com/zeta-chain/node/pull/2984) - Add SPL token whitelisting functionality to enable controlled token integration with corresponding Solana program changeszetaclient/chains/solana/signer/whitelist.go (1)
80-83: Address the TODO: Implement Compute Budget AdjustmentsThe TODO comment indicates that the current implementation uses a fixed fee of 5,000 lamports. Exploring priority fees and adjusting the compute budget can optimize transaction performance and cost efficiency, especially for complex transactions.
Would you like assistance in implementing the compute budget adjustments or creating a GitHub issue to track this enhancement?
x/crosschain/keeper/msg_server_whitelist_erc20.go (1)
Line range hint
50-60: Improve efficiency by indexing whitelisted assetsIterating over all foreign coins to check if an asset is already whitelisted can become inefficient as the list grows. Implementing an index or map can significantly optimize this lookup.
Modify the keeper to maintain an index:
// When adding a foreign coin key := generateAssetChainKey(msg.Erc20Address, msg.ChainId) k.foreignCoinsIndex[key] = true // When checking for existence key := generateAssetChainKey(msg.Erc20Address, msg.ChainId) if k.foreignCoinsIndex[key] { return nil, errorsmod.Wrapf( fungibletypes.ErrForeignCoinAlreadyExist, "asset (%s) already whitelisted on chain (%d)", msg.Erc20Address, msg.ChainId, ) }Implement
generateAssetChainKeyas a helper function to create a unique key based on the asset address and chain ID.e2e/runner/solana.go (3)
87-91: Improve Variable Naming for ClarityThe variable
apkin the loop overadditionalPrivateKeysmay not be immediately clear to readers. Consider renaming it to a more descriptive name, such asadditionalPrivKeyorkey, to enhance code readability.
104-113: Enhance Code Documentation for MaintainabilityThe comment on line 104 provides a general overview. To improve maintainability and aid future developers, consider adding detailed comments explaining each step within the
DeploySPLfunction. Descriptions for obtaining the minimum balance, creating account instructions, and initializing the mint will enhance clarity.
179-179: Confirm Necessity of Empty Slice foradditionalPrivateKeysIn the
SOLDepositAndCallfunction, you are passing an empty slice[]solana.PrivateKey{}foradditionalPrivateKeys. If no additional private keys are required, consider passingnilinstead for clarity. Ensure thatCreateSignedTransactionhandlesnilslices appropriately.zetaclient/chains/solana/signer/signer.go (1)
137-141: Ensure consistent use of logger for error messagesCurrently, there is inconsistency in how the logger is used for error messages. In some places,
signer.Logger().Std.Error()is used, while in others,logger.Error()is utilized. For consistency and to adhere to standard logging practices, consider using the same logger instance throughout the method.For example, replace
signer.Logger().Std.Error()withlogger.Error()where appropriate.- signer.Logger(). - Std.Error(). + logger.Error(). Err(err). Msgf("TryProcessOutbound: error decoding spl from relayed msg")Also applies to: 157-160, 174-177, 199-202, 216-219
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
contrib/localnet/solana/gateway.sois excluded by!**/*.sogo.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
changelog.md(1 hunks)cmd/zetae2e/local/local.go(1 hunks)cmd/zetae2e/local/solana.go(1 hunks)e2e/e2etests/e2etests.go(2 hunks)e2e/e2etests/test_solana_whitelist_spl.go(1 hunks)e2e/runner/setup_solana.go(1 hunks)e2e/runner/solana.go(5 hunks)go.mod(1 hunks)pkg/contracts/solana/gateway.go(2 hunks)pkg/contracts/solana/gateway.json(13 hunks)pkg/contracts/solana/gateway_message.go(2 hunks)pkg/contracts/solana/gateway_message_test.go(1 hunks)pkg/contracts/solana/instruction.go(1 hunks)x/crosschain/keeper/msg_server_whitelist_erc20.go(1 hunks)x/crosschain/types/message_whitelist_erc20.go(1 hunks)x/crosschain/types/message_whitelist_erc20_test.go(1 hunks)zetaclient/chains/solana/observer/outbound.go(3 hunks)zetaclient/chains/solana/signer/signer.go(2 hunks)zetaclient/chains/solana/signer/whitelist.go(1 hunks)zetaclient/testdata/solana/chain_901_inbound_tx_result_MS3MPLN7hkbyCZFwKqXcg8fmEvQMD74fN6Ps2LSWXJoRxPW5ehaxBorK9q1JFVbqnAvu9jXm6ertj7kT7HpYw1j.json(2 hunks)zetaclient/testutils/constant.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (17)
cmd/zetae2e/local/local.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetae2e/local/solana.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_whitelist_spl.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/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/instruction.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/keeper/msg_server_whitelist_erc20.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/message_whitelist_erc20.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/message_whitelist_erc20_test.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/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/whitelist.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/testutils/constant.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (21)
cmd/zetae2e/local/solana.go (2)
29-29: LGTM: ZetaTxServer integration looks correct.
The addition of WithZetaTxServer option is well-placed and correctly utilizes the deployer's transaction server instance. This change aligns with the PR's objective of integrating SPL whitelist functionality.
Line range hint 1-63: Verify test coverage for SPL whitelist functionality.
Since this PR introduces SPL whitelist features, we should verify that appropriate test cases are included.
✅ Verification successful
SPL whitelist test coverage is comprehensive and well-implemented
The test coverage for SPL whitelist functionality is thorough and includes critical verification steps:
- Deploys a new SPL token
- Verifies initial absence of whitelist entry
- Tests whitelist creation through admin policy
- Validates ZRC20 creation and liquidity cap initialization
- Confirms CCTX creation and mining
- Verifies final existence of whitelist entry in Solana
The test is properly registered in the e2etests package and integrated into the Solana test routine through the WithZetaTxServer configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for SPL whitelist related test cases
# Expected: Find test cases related to SPL whitelist functionality
# Search for SPL whitelist related test cases
echo "Searching for SPL whitelist test cases..."
rg -l "TestSolanaWhitelistSPL|WhitelistSPL" --type go
# Search for SPL whitelist related test setup
echo "Searching for SPL whitelist test setup..."
rg "WithZetaTxServer" --type go
Length of output: 1123
Script:
#!/bin/bash
# Let's examine the actual implementation of the SPL whitelist test
cat e2e/e2etests/test_solana_whitelist_spl.go
# Also check the test registration in e2etests.go
rg "TestSolanaWhitelistSPL" e2e/e2etests/e2etests.go -A 5 -B 5
Length of output: 3135
zetaclient/testdata/solana/chain_901_inbound_tx_result_MS3MPLN7hkbyCZFwKqXcg8fmEvQMD74fN6Ps2LSWXJoRxPW5ehaxBorK9q1JFVbqnAvu9jXm6ertj7kT7HpYw1j.json (3)
50-56: LGTM: Log messages correctly updated.
The log messages have been properly updated to reflect the new program ID while maintaining the same operational flow and compute unit consumption.
11-11: Verify consistency of updated account keys across test suite.
The account keys have been updated to reflect the new program addresses for SPL token whitelist functionality. Let's ensure these changes are consistent across all test data files.
#!/bin/bash
# Description: Check for consistency of account keys across test data files
# Search for old and new account keys in test data
echo "Checking for any remaining old program IDs..."
rg -l "ZETAjseVjuFsxdRxo6MmTCvqFwb3ZHUx56Co3vCmGis" "zetaclient/testdata"
echo "Verifying consistent usage of new program IDs..."
rg -l "94U5AHQMKkV5txNJ17QPXWoh474PheGou6cNP2FEuL1d" "zetaclient/testdata"Also applies to: 13-13
Line range hint 24-28: Verify instruction data format for whitelist operation.
The instruction data appears to be base58-encoded. Let's verify that the format aligns with the expected whitelist instruction layout.
✅ Verification successful
Base58-encoded instruction data format is consistent with whitelist operation
Based on the codebase analysis, the instruction data format in the test file is valid and follows the expected structure for Solana gateway operations. The code shows:
- The instruction data is properly base58-encoded and follows the Borsh serialization format used throughout the codebase
- The whitelist operation is handled by
WhitelistInstructionParamsstruct which expects:- An 8-byte discriminator (validated against
DiscriminatorWhitelistSplMint()) - A 64-byte signature
- Additional metadata including nonce and message hash
- An 8-byte discriminator (validated against
The test data matches this structure and is consistent with other gateway operations like deposits and withdrawals in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check instruction data format consistency
# Search for similar instruction patterns in test data
echo "Checking instruction data patterns..."
rg -l "4ALHYcAj3zFsNjmfeq7nDK1E8BsxRQRzhLjrqzmjYzL" "zetaclient/testdata"
# Look for instruction definition in Solana program
echo "Checking instruction definition..."
rg -A 5 "whitelist_spl_mint" --type rust
Length of output: 500
Script:
#!/bin/bash
# Let's search for instruction data format definitions and patterns
# Look for instruction data handling in Solana-related code
echo "Searching for instruction data handling..."
rg -A 5 "instruction_data" --type rust
# Search for base58 encoding/decoding patterns
echo "Checking base58 encoding patterns..."
rg -A 3 "base58" --type rust
# Look for whitelist-related instruction definitions
echo "Checking whitelist instruction definitions..."
rg -A 5 "(struct|enum).*Whitelist" --type rust
Length of output: 842
Script:
#!/bin/bash
# Let's try a different approach to find instruction-related code
# Find all Rust files first
echo "Finding Rust files..."
fd -e rs
# Look for instruction-related JSON test files
echo "Checking test data files..."
fd -e json -x cat {} | jq -r 'select(.instruction != null) | .'
# Search for instruction patterns in all files
echo "Searching instruction patterns..."
rg -g '!target' -g '!.git' "instruction" -A 5
Length of output: 51835
pkg/contracts/solana/gateway.go (3)
7-7: LGTM: Clean import addition
The new import for the generated IDL package is properly placed and follows Go conventions.
47-49: LGTM: Consistent implementation of new discriminator function
The new DiscriminatorWhitelistSplMint function follows the established pattern and is well-documented.
24-44: Verify instruction names against Solana IDL
The refactoring to use GetDiscriminator improves maintainability by removing hardcoded values. However, we should verify that the instruction names match exactly with the Solana program's IDL to prevent runtime errors.
✅ Verification successful
All instruction names verified in Solana IDL
The verification confirms that all discriminator function names (initialize, deposit, deposit_spl_token, withdraw, withdraw_spl_token) exactly match the instruction names defined in the Solana program's IDL at pkg/contracts/solana/gateway.json. The refactoring is safe to proceed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the instruction names exist in the IDL
# Test: Search for instruction definitions in the IDL
rg -A 5 '"name":\s*"(initialize|deposit|deposit_spl_token|withdraw|withdraw_spl_token)"' --type json
Length of output: 4379
pkg/contracts/solana/instruction.go (2)
120-121: LGTM: Interface implementation is correctly declared.
The nil type assertion ensures WhitelistInstructionParams implements all methods of the OutboundInstruction interface at compile time.
149-152: LGTM: GatewayNonce and TokenAmount implementations are correct.
Both methods are simple and follow the interface contract correctly. The TokenAmount returning 0 is appropriate for a whitelist operation that doesn't involve token transfers.
Also applies to: 154-157
zetaclient/chains/solana/observer/outbound.go (2)
160-160: LGTM: Consistent handling of outbound amount
The implementation correctly handles the outbound amount for both regular and restricted CCTX cases, maintaining consistency with the existing codebase patterns.
357-358: Consider architectural improvements for instruction handling
While the current implementation is functional, consider the following architectural improvements:
- Abstract common instruction parsing logic to reduce coupling with withdraw-specific implementation
- Implement consistent parsing approach across all instruction types
- Create a unified interface for different instruction types (withdraw, whitelist, etc.)
This would align with the PR objectives of reducing code duplication as more SPL integrations are developed.
Let's verify the instruction handling patterns:
go.mod (2)
Line range hint 391-396: LGTM: Well-structured replace directives for core dependencies.
The replace directives for core dependencies are well-organized and include clear comments explaining the replacements, particularly for the broken goleveldb issue.
Line range hint 399-406: Verify compatibility of forked dependencies.
The ZetaChain maintained forks are properly documented with comments and specific commit hashes. However, ensure that these forks are regularly synchronized with their upstream repositories to incorporate security fixes and improvements.
✅ Verification successful
Forked dependencies are actively maintained and recently updated
Based on the verification results, all forked dependencies show recent activity:
- zeta-chain/go-tss: Last updated October 29, 2024
- zeta-chain/go-libp2p: Last updated July 30, 2024
- zeta-chain/go-ethereum: Last updated July 18, 2024
- zeta-chain/tss-lib: Last updated January 26, 2024
All forks demonstrate active maintenance within the last year, with the most critical dependency (go-tss) updated just recently. This indicates proper synchronization with upstream repositories and active maintenance of security fixes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the last commit dates of the forked repositories to ensure they're maintained
for repo in "zeta-chain/tss-lib" "zeta-chain/go-ethereum" "zeta-chain/go-libp2p" "zeta-chain/go-tss"; do
echo "Checking $repo..."
gh repo view "$repo" --json updatedAt --jq '.updatedAt'
done
Length of output: 1062
pkg/contracts/solana/gateway.json (1)
31-43: LGTM: Well-structured PDA seeds implementation.
The PDA seeds implementation follows Solana's best practices with clear separation of concerns and deterministic derivation using constant seeds.
Also applies to: 87-99, 432-444, 475-496, 669-681, 743-755, 850-868
e2e/e2etests/e2etests.go (2)
64-64: LGTM: Test constant follows naming conventions.
The new constant TestSolanaWhitelistSPLName is well-placed within the Solana tests section and follows the established naming pattern.
456-461: Verify the referenced test function exists.
The test definition is well-structured and follows the established pattern. Let's verify the existence of the referenced test function.
✅ Verification successful
Test function TestSolanaWhitelistSPL exists and is properly implemented
The test function is found in e2e/e2etests/test_solana_whitelist_spl.go and contains a complete implementation for testing SPL whitelisting functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that TestSolanaWhitelistSPL function is defined
# Test: Search for the function definition
ast-grep --pattern 'func TestSolanaWhitelistSPL($$$) {
$$$
}'
Length of output: 4539
changelog.md (1)
Line range hint 1-1000: LGTM! Well-structured changelog.
The changelog follows best practices with:
- Clear version numbering and categorization
- Proper linking to PRs
- Consistent formatting across entries
zetaclient/chains/solana/signer/whitelist.go (2)
91-101: Confirm All Required Signers Are Included
When signing the transaction, it's crucial to verify that all necessary signers are present to avoid transaction rejection due to missing signatures.
Please ensure that any additional required signers, beyond the relayer's private key, have been added to the transaction. If the transaction involves program-derived addresses (PDAs) or other accounts requiring signatures, they must be explicitly included.
47-55: Validate Serialized Instruction Parameters
Ensure that the serialization of WhitelistInstructionParams correctly aligns with the Solana program's expected data format. Any mismatch in serialization can lead to transaction failures or unintended behavior.
Run the following script to verify that the serialized instruction parameters match the expected schema:
This script helps ensure that all necessary fields are included and correctly defined.
✅ Verification successful
Let me gather more information about the WhitelistInstructionParams structure since the previous script had issues with the ast-grep pattern.
Let me verify one final aspect about the MessageHash and Nonce fields since they weren't fully visible in the previous output.
Serialization Format Correctly Matches Solana Program Definition
The serialization of WhitelistInstructionParams is correctly implemented. The struct definition in pkg/contracts/solana/instruction.go perfectly aligns with the usage in the code under review:
- Discriminator: [8]byte
- Signature: [64]byte
- RecoveryID: uint8
- MessageHash: [32]byte
- Nonce: uint64
All fields are present with proper types and are being populated with appropriate values from the message object using the correct accessor methods.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the WhitelistInstructionParams serialization matches the Solana program specification.
# Expected fields in WhitelistInstructionParams
EXPECTED_FIELDS=("Discriminator" "Signature" "RecoveryID" "MessageHash" "Nonce")
# Use ast-grep to find the struct definition and list its fields
ast-grep --pattern $'struct WhitelistInstructionParams {
$$$
}' --lang go zetaclient/chains/solana/**/*.go | grep 'WhitelistInstructionParams' -A 10 | grep -E '^\s+\w+' | awk '{print $1}' > actual_fields.txt
# Compare the expected fields with the actual fields
for field in "${EXPECTED_FIELDS[@]}"; do
if ! grep -q "$field" actual_fields.txt; then
echo "Field $field is missing in WhitelistInstructionParams"
fi
done
rm actual_fields.txt
Length of output: 2378
Script:
#!/bin/bash
# Search for WhitelistInstructionParams definition using ripgrep for better context
rg -A 10 "type WhitelistInstructionParams struct" --type go
# Also check the contracts package where the struct is being used
rg -A 5 "DiscriminatorWhitelistSplMint" --type go
Length of output: 2239
Script:
#!/bin/bash
# Get the complete struct definition
rg -A 15 "type WhitelistInstructionParams struct" --type go
# Also verify the borsh serialization implementation
rg -B 2 -A 5 "func \(.*WhitelistInstructionParams.*\) (Serialize|Deserialize)" --type go
Length of output: 1193
e2e/runner/solana.go (1)
67-67: Enhance Functionality with additionalPrivateKeys Parameter
The addition of the additionalPrivateKeys []solana.PrivateKey parameter to the CreateSignedTransaction function appropriately extends its capability to handle transactions requiring multiple signatures. This implementation aligns with clean code principles and enhances expressiveness.
skosito
changed the title
opened issue for renaming with TODO in code in proto file: #3083 |
| foreignCoins := k.fungibleKeeper.GetAllForeignCoins(ctx) | ||
| for _, fCoin := range foreignCoins { | ||
| assetAddr := ethcommon.HexToAddress(fCoin.Asset) | ||
| if assetAddr == erc20Addr && fCoin.ForeignChainId == msg.ChainId { | ||
| if fCoin.Asset == msg.Erc20Address && fCoin.ForeignChainId == msg.ChainId { | ||
| return nil, errorsmod.Wrapf( |
There was a problem hiding this comment.
You could use k.fungibleKeeper.GetForeignCoinFromAsset() instead to re-use existing logic.
Description
Related solana program PR: zeta-chain/protocol-contracts-solana#60
This also includes latest version of gateway, and minor fixes so existing functionality works with latest gateway.
There is some duplicate code between whitelist and withdraw, as we add more spl integration soon it will be more clear which parts can be abstracted away.
NOTE: I didnt rename erc20 to something more generic, like maybe
asset? in MsgWhitelistERC20 and related code, if thats needed will do in next PR, to focus on reviewing other pieces here, as that renaming should be simple but will result in bunch of files changed.Also, this is adding protocol-contracts-solana package with autogenerated IDL for discriminators for now. There are some todos to generate types as well there, but it is out of scope of this PR.
How Has This Been Tested?
Summary by CodeRabbit
Release Notes for Version 21.0.0
New Features
Bug Fixes
Tests
Chores