fix: fix solana inbounds #3261
fix: fix solana inbounds #3261
Conversation
skosito
added
no-changelog
|
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📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files, primarily enhancing the verification of contract calls in Solana-related tests. Key changes include the introduction of a new method for checking contract calls with additional message data, updates to deposit instruction creation logic to incorporate a data parameter, and enhancements to parsing logic for deposit instructions. New constants and types related to deposit and call operations have also been added, alongside updates to test cases to reflect new transaction data. Additionally, new JSON files documenting transaction results have been introduced, while some outdated files have been removed. 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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3261 +/- ##
===========================================
+ Coverage 61.78% 61.84% +0.05%
===========================================
Files 431 431
Lines 30786 30848 +62
===========================================
+ Hits 19021 19077 +56
- Misses 10909 10913 +4
- Partials 856 858 +2
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
pkg/contracts/solana/inbound.go (1)
23-24: Update function comment to reflect new return valuesThe function
ParseInboundAsDepositnow returns both a*Depositand anerror. The comment should be updated to accurately describe the return values, ensuring it reflects the current function signature.e2e/utils/contracts.go (2)
35-56: Refactor to eliminate code duplicationThe new function
MustHaveCalledExampleContractWithMsgduplicates logic fromMustHaveCalledExampleContract. Consider refactoring to extract common assertions into a shared helper function or modifying the original function to accept an optional message parameter. This will improve maintainability and reduce redundancy in the test utilities.
55-55: Usebytes.Equalfor message comparisonWhen comparing byte slices, it's preferable to use
bytes.Equal(msg, lastMsg)instead of casting them to strings. This ensures accurate comparison of the message data, especially if it contains non-UTF8 encoded bytes.pkg/contracts/solana/gateway.go (1)
30-31: Fix incorrect comment for DiscriminatorDepositAndCallThe comment incorrectly states "DiscriminatorDeposit" instead of "DiscriminatorDepositAndCall".
Apply this fix:
-// DiscriminatorDeposit returns the discriminator for Solana gateway 'deposit_and_call' instruction +// DiscriminatorDepositAndCall returns the discriminator for Solana gateway 'deposit_and_call' instructione2e/runner/solana.go (1)
39-56: Consider extracting common serialization logic.The deposit instruction serialization logic is duplicated between
CreateDepositInstructionandCreateDepositSPLInstruction. Consider extracting the common pattern into a helper function to improve maintainability.Example refactor:
+func serializeDepositParams(discriminator uint8, amount uint64, receiver ethcommon.Address, data []byte) ([]byte, error) { + if data == nil { + return borsh.Serialize(solanacontract.DepositInstructionParams{ + Discriminator: discriminator, + Amount: amount, + Receiver: receiver, + }) + } + return borsh.Serialize(solanacontract.DepositAndCallInstructionParams{ + Discriminator: discriminator + 1, // Assuming AndCall discriminators are offset by 1 + Amount: amount, + Receiver: receiver, + Memo: data, + }) +}Then use it in both functions:
depositData, err := serializeDepositParams(solanacontract.DiscriminatorDeposit, amount, receiver, data)Also applies to: 102-119
pkg/contracts/solana/inbound_test.go (1)
Line range hint
42-63: Consider using test data constants for better maintainability.The test data (transaction hashes, addresses, expected values) is duplicated across test functions. Consider extracting these into constants or test fixtures to improve maintainability and make updates easier.
Example refactor:
const ( depositTxHash = "8UeJoxY6RbMg6bffsUtZ9f79vSnd4HCRdk5EQgNbAEDYQWXNraiKDtGDZBLp91oyF5eQyWdv6pEwW1vcitiB4px" depositAndCallTxHash = "5b7ShhHf8dvUjUBHgMvgH8FFqpfAd7vAGygZLaeNPhugXtY5fatPSACVkn13o7sw6Awob8EJnrwAuiKYqvi7ZkHa" e2eDeployerAccount = "37yGiHAnLvWZUNVwu9esp74YQFqxU1qHCbABkDvRddUQ" ) type testCase struct { name string txHash string expectedDeposit *Deposit }Also applies to: 145-181, 354-378
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
testutil/contracts/Example.binis excluded by!**/*.bin
📒 Files selected for processing (19)
e2e/e2etests/test_solana_deposit_call.go(1 hunks)e2e/e2etests/test_spl_deposit_and_call.go(1 hunks)e2e/runner/solana.go(2 hunks)e2e/utils/contracts.go(1 hunks)pkg/contracts/solana/gateway.go(1 hunks)pkg/contracts/solana/inbound.go(5 hunks)pkg/contracts/solana/inbound_test.go(8 hunks)pkg/contracts/solana/instruction.go(1 hunks)pkg/contracts/solana/testdata/22s5ERRRZmZXAuDMRdwUU33VnWZ7m8NHUZM6hyLH52JQPz5R7mXEkFcvHx88ujq3xDnt3z7sZdZ21JK2FC7vPw1o.json(1 hunks)pkg/contracts/solana/testdata/5b7ShhHf8dvUjUBHgMvgH8FFqpfAd7vAGygZLaeNPhugXtY5fatPSACVkn13o7sw6Awob8EJnrwAuiKYqvi7ZkHa.json(1 hunks)pkg/contracts/solana/testdata/5bXSQaq6BY1WhhF3Qm4pLHXxuyM9Mz1MrdMeoCFbimxw4uv11raQgAj4HGULPEQExPKB231rMhm6666dQMwf9fNN.json(4 hunks)pkg/contracts/solana/testdata/8UeJoxY6RbMg6bffsUtZ9f79vSnd4HCRdk5EQgNbAEDYQWXNraiKDtGDZBLp91oyF5eQyWdv6pEwW1vcitiB4px.json(3 hunks)testutil/contracts/Example.abi(1 hunks)testutil/contracts/Example.go(4 hunks)testutil/contracts/Example.json(2 hunks)testutil/contracts/Example.sol(2 hunks)zetaclient/chains/solana/observer/inbound_test.go(3 hunks)zetaclient/testdata/solana/chain_901_inbound_tx_result_24GzWsxYCFcwwJ2rzAsWwWC85aYKot6Rz3jWnBP1GvoAg5A9f1WinYyvyKseYM52q6i3EkotZdJuQomGGq5oxRYr.json(1 hunks)zetaclient/testdata/solana/chain_901_inbound_tx_result_MS3MPLN7hkbyCZFwKqXcg8fmEvQMD74fN6Ps2LSWXJoRxPW5ehaxBorK9q1JFVbqnAvu9jXm6ertj7kT7HpYw1j.json(0 hunks)
💤 Files with no reviewable changes (1)
- zetaclient/testdata/solana/chain_901_inbound_tx_result_MS3MPLN7hkbyCZFwKqXcg8fmEvQMD74fN6Ps2LSWXJoRxPW5ehaxBorK9q1JFVbqnAvu9jXm6ertj7kT7HpYw1j.json
✅ Files skipped from review due to trivial changes (2)
- zetaclient/testdata/solana/chain_901_inbound_tx_result_24GzWsxYCFcwwJ2rzAsWwWC85aYKot6Rz3jWnBP1GvoAg5A9f1WinYyvyKseYM52q6i3EkotZdJuQomGGq5oxRYr.json
- pkg/contracts/solana/testdata/5b7ShhHf8dvUjUBHgMvgH8FFqpfAd7vAGygZLaeNPhugXtY5fatPSACVkn13o7sw6Awob8EJnrwAuiKYqvi7ZkHa.json
🧰 Additional context used
📓 Path-based instructions (10)
pkg/contracts/solana/gateway.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/utils/contracts.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_call.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.
e2e/runner/solana.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
testutil/contracts/Example.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.
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.
e2e/e2etests/test_spl_deposit_and_call.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/inbound.go
[warning] 93-94: pkg/contracts/solana/inbound.go#L93-L94
Added lines #L93 - L94 were not covered by tests
[warning] 180-181: pkg/contracts/solana/inbound.go#L180-L181
Added lines #L180 - L181 were not covered by tests
🔇 Additional comments (20)
pkg/contracts/solana/inbound.go (1)
69-69: Verify consistency of Memo field assignment across parsing functions
There is an inconsistency in how the Memo field is constructed between the deposit functions and the deposit-and-call functions. In tryParseAsDeposit and tryParseAsDepositSPL, Memo is assigned inst.Receiver[:], whereas in tryParseAsDepositAndCall and tryParseAsDepositSPLAndCall, it is assigned append(instDepositAndCall.Receiver[:], instDepositAndCall.Memo...). Please verify whether this difference is intentional and ensure that the Memo field consistently represents the intended data across all parsing functions.
Also applies to: 104-104, 156-156, 191-191
testutil/contracts/Example.sol (2)
15-15: LGTM
The addition of the lastMessage public variable is appropriate and correctly declared. This enables the contract to store and expose the last received message as intended.
44-44: LGTM
Assigning the message parameter to lastMessage within onCrossChainCall properly records the incoming message, enhancing the contract's functionality.
e2e/e2etests/test_solana_deposit_call.go (1)
34-34: LGTM
Updating the test to use MustHaveCalledExampleContractWithMsg ensures that both the deposit amount and the message are validated. This enhances the test's coverage and accuracy.
testutil/contracts/Example.abi (1)
53-65: LGTM: New lastMessage function is well-structured
The function signature is correctly defined with appropriate mutability and return type for message retrieval operations.
Let's verify the function implementation:
✅ Verification successful
Verified: lastMessage function is correctly implemented and integrated
The lastMessage public state variable in the Solidity contract automatically generates the view function in the ABI. The variable is properly updated in the onCrossChainCall function, storing the cross-chain message data. The implementation aligns perfectly with the ABI definition and follows Solidity best practices for state variable accessibility and message handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify lastMessage function implementation
# Look for the function implementation in the contract source
ast-grep --pattern 'function lastMessage() public view returns (bytes) {
$$$
}'
Length of output: 83
Script:
#!/bin/bash
# First, let's find the contract implementation file
fd -e sol Example
# Then, let's search for lastMessage function implementation with context
rg -U "function lastMessage" -A 5
Length of output: 1404
Script:
#!/bin/bash
# Let's examine the actual Solidity contract implementation
cat testutil/contracts/Example.sol
Length of output: 868
pkg/contracts/solana/testdata/8UeJoxY6RbMg6bffsUtZ9f79vSnd4HCRdk5EQgNbAEDYQWXNraiKDtGDZBLp91oyF5eQyWdv6pEwW1vcitiB4px.json (2)
37-38: Balance changes correctly reflect deposit operation
The pre and post balances show a transfer of 14000000 lamports (12000000 deposit + 2000000 fee) from the sender to the receiver account.
58-61: Transaction logs properly document the deposit operation
The log messages clearly indicate:
- Deposit instruction execution
- System program invocation
- Detailed deposit information including amount and receiver
pkg/contracts/solana/gateway.go (1)
30-38: LGTM: New discriminators properly integrated
The new discriminators for deposit_and_call and deposit_spl_token_and_call operations are correctly initialized using the IDLGateway interface.
Let's verify the instruction handling for these discriminators:
✅ Verification successful
Let me verify the instruction handling for these discriminators by checking the implementation of the deposit and call functionality.
Let me verify the SPL deposit and call instruction handling since it wasn't found in the previous search.
Let me do one final check to verify the SPL deposit instruction handling implementation.
Discriminators properly integrated with complete instruction handling
The verification confirms that both deposit_and_call and deposit_spl_token_and_call discriminators are correctly integrated with proper instruction handling:
- The
DepositAndCallInstructionParamsstructure properly handles deposit_and_call with discriminator validation - The
DepositSPLAndCallInstructionParamsstructure properly handles deposit_spl_token_and_call with discriminator validation - Both discriminators are correctly initialized from the IDLGateway interface
- The instruction parsing logic in inbound.go correctly validates the discriminators and processes the instructions
- End-to-end tests exist for both instruction types, validating the complete flow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify instruction handling for new discriminators
# Look for instruction handling code
rg -A 10 "deposit_and_call|deposit_spl_token_and_call" --type go
Length of output: 25932
Script:
#!/bin/bash
# Search for deposit and call instruction handling implementation
ast-grep --pattern 'func $_(tx *solana.Transaction, $_, $_) (*Deposit, error) {
$$$
instDepositAndCall := DepositAndCallInstructionParams{}
$$$
}'
Length of output: 2288
Script:
#!/bin/bash
# Search for SPL deposit and call instruction handling
ast-grep --pattern 'func $_(tx *solana.Transaction, $_, $_) (*DepositSPL, error) {
$$$
instDepositSPLAndCall := DepositSPLAndCallInstructionParams{}
$$$
}'
Length of output: 163
Script:
#!/bin/bash
# Check for deposit instruction parameter structures
ast-grep --pattern 'type DepositSPLAndCallInstructionParams struct {
$$$
}'
Length of output: 965
Script:
#!/bin/bash
# Search for SPL deposit and call instruction handling with a broader pattern
ast-grep --pattern 'func $_(tx *solana.Transaction, $_, $_) (*DepositSPL, error) {
$$$
if inst$_.Discriminator == DiscriminatorDepositSPLAndCall {
$$$
}
$$$
}'
Length of output: 173
Script:
#!/bin/bash
# Alternative search for SPL deposit handling
rg -A 10 "DiscriminatorDepositSPLAndCall" --type go
Length of output: 2375
Script:
#!/bin/bash
# Search for SPL deposit parsing function
ast-grep --pattern 'func tryParseAsDepositSPL(tx *solana.Transaction, $_, $_) (*DepositSPL, error) {
$$$
}'
Length of output: 111
Script:
#!/bin/bash
# Search for SPL deposit instruction parameter structure
ast-grep --pattern 'type DepositSPLInstructionParams struct {
$$$
}'
Length of output: 743
e2e/e2etests/test_spl_deposit_and_call.go (1)
54-54: LGTM! Enhanced contract call verification
The update to use MustHaveCalledExampleContractWithMsg improves test coverage by verifying both the amount and message data.
pkg/contracts/solana/testdata/5bXSQaq6BY1WhhF3Qm4pLHXxuyM9Mz1MrdMeoCFbimxw4uv11raQgAj4HGULPEQExPKB231rMhm6666dQMwf9fNN.json (1)
Line range hint 95-95: Review the compute units consumption
The computeUnitsConsumed value of 274902869112 appears unusually high for a simple deposit transaction. Standard Solana transactions typically consume far fewer compute units.
Let's verify typical compute unit consumption for similar transactions:
pkg/contracts/solana/testdata/22s5ERRRZmZXAuDMRdwUU33VnWZ7m8NHUZM6hyLH52JQPz5R7mXEkFcvHx88ujq3xDnt3z7sZdZ21JK2FC7vPw1o.json (1)
78-85: LGTM! Well-structured transaction logs
The log messages clearly show the transaction flow:
- Program invocation
- Instruction type (DepositSplTokenAndCall)
- Transfer execution
- Successful completion
zetaclient/chains/solana/observer/inbound_test.go (2)
29-30: LGTM: Test data update
The transaction hash update maintains test functionality while using new test data.
80-88: Verify the significant amount change
The test amount has increased by 1000x (from 100000 to 100000000). While this appears to be intentional, please verify that this significant change in test values aligns with the expected behavior.
✅ Verification successful
The search results show that the amount 100000000 appears in various test files and contracts across the codebase, often representing different denominations in different contexts (BTC, ETH, etc.). Let's verify if this specific amount in the Solana test is consistent with other Solana-related tests.
Amount change in test is consistent with Solana test patterns
The amount change from 100000 to 100000000 in the test case is consistent with the existing Solana test patterns, as evidenced by the matching amount in other Solana test files. This appears to be a deliberate standardization of test values rather than an issue.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other test cases or constants using similar amounts
rg -g '*.go' "100000000" --context 3
Length of output: 65573
Script:
#!/bin/bash
# Search for amount patterns in Solana-related files
rg -g '*.go' -B 3 -A 3 "Amount.*[0-9]{8,}" zetaclient/chains/solana/
Length of output: 611
testutil/contracts/Example.json (1)
54-66: LGTM: Well-structured ABI addition
The new lastMessage function follows standard ABI patterns for view functions.
pkg/contracts/solana/instruction.go (4)
33-35: LGTM: Enhanced type safety
The change from variable-length []byte to fixed-size [20]byte for the receiver address improves type safety and clarity.
37-50: LGTM: Well-structured instruction params
The new struct clearly separates deposit and call parameters while maintaining good documentation.
52-62: LGTM: Consistent parameter structure
The changes maintain consistency with the non-SPL deposit instruction while properly documenting SPL-specific context.
64-77: LGTM: Consistent instruction parameter pattern
The new struct maintains consistency with other instruction parameter types while properly handling SPL-specific functionality.
testutil/contracts/Example.go (2)
243-272: LGTM! The generated code for LastMessage looks correct.
The auto-generated bindings for the new LastMessage method are properly implemented with appropriate error handling and type conversions.
167-171: LGTM! The updated bindExample implementation is correct.
The change from direct ABI usage to GetAbi() is a good practice as it ensures consistency with the metadata.
...2s5ERRRZmZXAuDMRdwUU33VnWZ7m8NHUZM6hyLH52JQPz5R7mXEkFcvHx88ujq3xDnt3z7sZdZ21JK2FC7vPw1o.json
Outdated
Show resolved
Hide resolved
Description
cherry-pick fix for solana inbounds #3255
How Has This Been Tested?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Chores