test: extend solana withdraw and call unit tests #3561
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 📝 WalkthroughWalkthroughThis pull request updates changelog entries for added clarity on SOL/SPL withdraw and call integrations, and it introduces new test cases to cover revert scenarios. Method signatures in the runner and test suites are modified to accept dynamic revert options. New contract types, messages, and instruction parameter structures for handling increment nonce operations have been added. The outbound transaction flow now includes fallback mechanisms, and dependency and test data updates for Solana transactions have been incorporated. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant S as Signer
participant N as Solana Network
C->>S: Trigger outbound transaction
S->>S: Prepare primary transaction (tx)
S->>S: Create fallback transaction via prepareIncrementNonceTx()
S->>N: Broadcast primary transaction
alt Broadcast fails due to error (non-nonce mismatch)
S->>N: Broadcast fallback transaction
end
N-->>S: Transaction confirmation or error
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3561 +/- ##
========================================
Coverage 64.72% 64.72%
========================================
Files 461 461
Lines 31894 31894
========================================
Hits 20644 20644
Misses 10345 10345
Partials 905 905 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
e2e/runner/solana.go (2)
512-513: 💡 Verification agent🧩 Analysis chain
Address the TODO comment about gas limit.
The TODO comment suggests uncertainty about the gas limit value. Consider documenting the rationale for the current gas limit value or adjusting it based on empirical data.
🏁 Script executed:
#!/bin/bash # Search for gas limit usage patterns in the codebase rg -A 2 'GasLimit.*250000'Length of output: 2025
Clarify Gas Limit Rationale in WithdrawAndCallSOLZRC20
We've verified that the gas limit of 250000 is consistently applied throughout the codebase—including in
e2e/runner/solana.go, the test contracts, and test data. Nevertheless, the TODO comment still indicates uncertainty about whether this value is optimal. Please update the code by either:
- Documenting the rationale behind choosing 250000 as the gas limit (e.g., based on empirical data or standard practice), or
- Adjusting the value if further investigation suggests a better alternative.
This will help ensure clarity and maintain consistency across the codebase.
- File:
e2e/runner/solana.go(lines relating togatewayzevm.CallOptions{GasLimit: big.NewInt(250000)})
604-605: 💡 Verification agent🧩 Analysis chain
Address the TODO comment about gas limit.
Similar to the SOL withdraw function, the gas limit value needs documentation or adjustment based on empirical data.
🏁 Script executed:
#!/bin/bash # Search for gas limit patterns in tests rg -A 2 'TestSPLWithdrawAndCall.*GasLimit'Length of output: 44
🏁 Script executed:
#!/bin/bash echo "Searching for 'gas limit' occurrences in e2e/runner/solana.go..." rg -n "gas limit" e2e/runner/solana.go echo "Searching for references to 'SPLWithdrawAndCall' across the repository..." rg -n "SPLWithdrawAndCall" .Length of output: 1574
Action Required: Address the Gas Limit TODO in WithdrawAndCallSPLZRC20
The code in
e2e/runner/solana.go(lines 604-605) still includes the// TODO: gas limit?comment. Similar to the SOL withdraw function, the gas limit should either be documented—explaining how its value is determined from empirical data—or appropriately adjusted to ensure consistency and optimal performance.
- Location:
e2e/runner/solana.go, lines 604-605.- Recommendation: Align the gas limit logic with the SOL withdraw implementation by either documenting the chosen default value based on testing/empirical data or revisiting the value to match current performance requirements.
🧹 Nitpick comments (15)
e2e/e2etests/test_spl_withdraw_and_call.go (1)
69-77: Consider setting a non-zero OnRevertGasLimit.Setting
OnRevertGasLimitto 0 might prevent proper handling of revert scenarios. Consider setting a reasonable gas limit based on the expected operations in the revert path.- OnRevertGasLimit: big.NewInt(0), + OnRevertGasLimit: big.NewInt(100000), // Adjust based on revert path requirementse2e/e2etests/test_solana_withdraw_and_call_revert_with_call.go (2)
59-59: Consider parameterizing the OnRevertGasLimit value.The hardcoded gas limit of 0 might be too restrictive. Consider making this configurable or using a reasonable default value to handle different revert scenarios.
- OnRevertGasLimit: big.NewInt(0), + OnRevertGasLimit: runner.DefaultRevertGasLimit, // Define this constant with a reasonable value
46-47: Consider adding test description for payload generation.The
randomPayloadfunction usage lacks context about what kind of payload is being generated and its purpose in the test.Add a comment explaining the purpose and structure of the random payload:
+ // Generate random payload to simulate contract interaction data payload := randomPayload(r) r.AssertTestDAppEVMCalled(false, payload, withdrawAmount)e2e/e2etests/test_spl_withdraw_and_call_revert.go (2)
39-40: Fix incomplete error message.The error message for withdrawal amount validation is truncated.
- "Withdrawal amount must be less than the %v", + "Withdrawal amount must be less than the approved amount: %v",
92-94: Remove redundant balance check.The balance is already being checked and logged at lines 88-90.
Remove the redundant balance check:
- balanceAfter, err := r.SPLZRC20.BalanceOf(&bind.CallOpts{}, r.EVMAddress()) - require.NoError(r, err) - r.Logger.Info("runner balance of SOL after withdraw: %d", balanceAfter)zetaclient/chains/solana/observer/outbound_test.go (1)
47-54: Group related test constants together.The test transaction constants are scattered. Consider grouping them by type (withdraw, execute, increment) for better organization.
+ // Withdraw transaction test data withdrawTxTest = "5iBYjBYCphzjHKfmPwddMWpV2RNssmzk9Z8NNmV9Rei71pZKBTEVdkmUeyXfn7eWbV8932uSsPfBxgA7UgERNTvq" withdrawFailedTxTest = "5nFUQgNSdqTd4aPS4a1xNcbehj19hDzuQLfBqFRj8g7BJdESVY6hFuTFPWFuV6aWAfzEMfVfCdNu9DfzVp5FsHg5" withdrawSPLTxTest = "3NgoR4K9FJq7UunorPRGW9wpqMV8oNvZERejutd7bKmqh3CKEV5DMZndhZn7hQ1i4RhTyHXRWxtR5ZNVHmmjAUSF" + // Execute transaction test data executeTxTest = "4ZuPTkYtBGDyDZNHKyHxEKL98VeaefAMUzmZVL2BrgwCvog7CqpjrRoegXDt6bD7w8dffGKGcDZqFYFi5vkAK8eo" executeSPLTxTest = "d3WvqtwFws9yftpxSrmwXqb48ZbBVjvxz34zY5Mo9TxaAPxsudPa68nDXZeShvK8UqtM84TgGfpdrgeX65q5WCW" + // Increment nonce transaction test data incrementNonceTxTest = "5dpFTsscUKCGVQzL9bAUSuEE6yLXaf7d1wMjZa7RLqvtSUtAdfcdxQHNsbfcS2Sfzu4zBVxMJC2KWzuaUUbg1ZGk"zetaclient/chains/solana/signer/increment_nonce.go (2)
84-88: Track the TODO comment about fee optimization.The TODO comment about outbound fee optimization should be tracked properly.
Would you like me to create an issue to track the fee optimization task? The issue would cover:
- Investigation of priority fee implementation
- Analysis of compute budget options
- Performance impact assessment
66-73: Consider adding a comment explaining the account permissions.The account metadata setup could benefit from documentation explaining the permission requirements.
inst := solana.GenericInstruction{ ProgID: signer.gatewayID, DataBytes: dataBytes, + // Account permissions: + // - Relayer key: Writable and signer for transaction authorization + // - PDA: Writable for nonce state updates AccountValues: []*solana.AccountMeta{ solana.Meta(signer.relayerKey.PublicKey()).WRITE().SIGNER(), solana.Meta(signer.pda).WRITE(), }, }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 66-79: zetaclient/chains/solana/signer/increment_nonce.go#L66-L79
Added lines #L66 - L79 were not covered by testspkg/contracts/solana/gateway_message_test.go (3)
80-99: Test coverage for nonce increments is commendable.
The test verifies the hash forMsgIncrementNoncethoroughly. However, consider adding a negative or edge-case test for invalid amounts or nonces to ensure stricter coverage.
101-122: Consider expanding test scenarios.
Test_MsgExecuteHashcorrectly verifies hashing mechanics. You may also add test cases with atypical or large input data to ensure resilience and detect potential overflow issues.
124-148: Solid testing of SPL execution messages.
Test_MsgExecuteSPLHashlooks comprehensive. As a minor suggestion, you could include negative validations (e.g., invalid addresses or zero amounts) to improve reliability.zetaclient/chains/solana/signer/signer.go (1)
287-316: Ensure comprehensive tests for increment-nonce flows.
prepareIncrementNonceTxis well-structured, but lines 287-316 remain uncovered by tests per the static analysis. Adding a dedicated test would verify that both valid and compliance-restricted transactions handle increment nonce as intended.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 287-302: zetaclient/chains/solana/signer/signer.go#L287-L302
Added lines #L287 - L302 were not covered by tests
[warning] 305-308: zetaclient/chains/solana/signer/signer.go#L305-L308
Added lines #L305 - L308 were not covered by tests
[warning] 311-314: zetaclient/chains/solana/signer/signer.go#L311-L314
Added lines #L311 - L314 were not covered by tests
[warning] 316-316: zetaclient/chains/solana/signer/signer.go#L316
Added line #L316 was not covered by testspkg/contracts/solana/gateway_message.go (1)
123-211: New message type aligns well with existing patterns.
MsgIncrementNoncefollows the established structure, with well-defined fields forchainID,nonce, andamount. Consider validatingamountor clarifying if negative or zero amounts are acceptable. Companion documentation could also help explain usage constraints.pkg/contracts/solana/instruction.go (2)
144-161: Consider enhancing error messages for better debugging.The error messages in
ParseInstructionIncrementNoncecould be more descriptive by including the expected discriminator value.- return nil, fmt.Errorf("not an increment_nonce instruction: %v", inst.Discriminator) + return nil, fmt.Errorf("not an increment_nonce instruction: got %v, want %v", inst.Discriminator, DiscriminatorIncrementNonce)
548-572: Consider optimizing JSON marshaling in DecodeExecuteMsg.The current implementation performs unnecessary JSON marshaling and unmarshaling steps. Consider directly mapping the unpacked values to the
ExecuteMsgstruct.func DecodeExecuteMsg(msgbz []byte) (ExecuteMsg, error) { args, err := GetExecuteMsgAbi() if err != nil { return ExecuteMsg{}, err } unpacked, err := args.Unpack(msgbz) if err != nil { return ExecuteMsg{}, err } - jsonMsg, err := json.Marshal(unpacked[0]) - if err != nil { - return ExecuteMsg{}, err - } - - var msg ExecuteMsg - err = json.Unmarshal(jsonMsg, &msg) - if err != nil { - return ExecuteMsg{}, err - } + msg, ok := unpacked[0].(struct { + Accounts []AccountMeta + Data []byte + }) + if !ok { + return ExecuteMsg{}, fmt.Errorf("failed to convert unpacked data to ExecuteMsg") + } + + return ExecuteMsg(msg), nil - return msg, nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
contrib/localnet/solana/connected.sois excluded by!**/*.socontrib/localnet/solana/connected_spl.sois excluded by!**/*.sogo.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
changelog.md(1 hunks)cmd/zetae2e/local/local.go(2 hunks)e2e/e2etests/e2etests.go(3 hunks)e2e/e2etests/test_solana_withdraw_and_call.go(2 hunks)e2e/e2etests/test_solana_withdraw_and_call_revert_with_call.go(1 hunks)e2e/e2etests/test_spl_withdraw_and_call.go(2 hunks)e2e/e2etests/test_spl_withdraw_and_call_revert.go(1 hunks)e2e/runner/solana.go(4 hunks)go.mod(1 hunks)pkg/contracts/solana/gateway.go(1 hunks)pkg/contracts/solana/gateway_message.go(2 hunks)pkg/contracts/solana/gateway_message_test.go(2 hunks)pkg/contracts/solana/instruction.go(6 hunks)zetaclient/chains/solana/observer/outbound.go(3 hunks)zetaclient/chains/solana/observer/outbound_test.go(2 hunks)zetaclient/chains/solana/signer/increment_nonce.go(1 hunks)zetaclient/chains/solana/signer/signer.go(6 hunks)zetaclient/testdata/solana/chain_901_outbound_tx_result_4ZuPTkYtBGDyDZNHKyHxEKL98VeaefAMUzmZVL2BrgwCvog7CqpjrRoegXDt6bD7w8dffGKGcDZqFYFi5vkAK8eo.json(1 hunks)zetaclient/testdata/solana/chain_901_outbound_tx_result_5dpFTsscUKCGVQzL9bAUSuEE6yLXaf7d1wMjZa7RLqvtSUtAdfcdxQHNsbfcS2Sfzu4zBVxMJC2KWzuaUUbg1ZGk.json(1 hunks)zetaclient/testdata/solana/chain_901_outbound_tx_result_d3WvqtwFws9yftpxSrmwXqb48ZbBVjvxz34zY5Mo9TxaAPxsudPa68nDXZeShvK8UqtM84TgGfpdrgeX65q5WCW.json(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- go.mod
- zetaclient/testdata/solana/chain_901_outbound_tx_result_5dpFTsscUKCGVQzL9bAUSuEE6yLXaf7d1wMjZa7RLqvtSUtAdfcdxQHNsbfcS2Sfzu4zBVxMJC2KWzuaUUbg1ZGk.json
- zetaclient/testdata/solana/chain_901_outbound_tx_result_d3WvqtwFws9yftpxSrmwXqb48ZbBVjvxz34zY5Mo9TxaAPxsudPa68nDXZeShvK8UqtM84TgGfpdrgeX65q5WCW.json
- zetaclient/testdata/solana/chain_901_outbound_tx_result_4ZuPTkYtBGDyDZNHKyHxEKL98VeaefAMUzmZVL2BrgwCvog7CqpjrRoegXDt6bD7w8dffGKGcDZqFYFi5vkAK8eo.json
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to ...
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/observer/outbound.goe2e/e2etests/test_spl_withdraw_and_call.gocmd/zetae2e/local/local.goe2e/e2etests/test_solana_withdraw_and_call.gozetaclient/chains/solana/signer/increment_nonce.goe2e/e2etests/test_solana_withdraw_and_call_revert_with_call.gozetaclient/chains/solana/observer/outbound_test.gopkg/contracts/solana/gateway_message_test.gopkg/contracts/solana/gateway.goe2e/e2etests/e2etests.goe2e/e2etests/test_spl_withdraw_and_call_revert.gozetaclient/chains/solana/signer/signer.goe2e/runner/solana.gopkg/contracts/solana/gateway_message.gopkg/contracts/solana/instruction.go
🪛 GitHub Check: codecov/patch
zetaclient/chains/solana/observer/outbound.go
[warning] 112-114: zetaclient/chains/solana/observer/outbound.go#L112-L114
Added lines #L112 - L114 were not covered by tests
[warning] 151-151: zetaclient/chains/solana/observer/outbound.go#L151
Added line #L151 was not covered by tests
[warning] 154-157: zetaclient/chains/solana/observer/outbound.go#L154-L157
Added lines #L154 - L157 were not covered by tests
[warning] 314-315: zetaclient/chains/solana/observer/outbound.go#L314-L315
Added lines #L314 - L315 were not covered by tests
zetaclient/chains/solana/signer/increment_nonce.go
[warning] 21-31: zetaclient/chains/solana/signer/increment_nonce.go#L21-L31
Added lines #L21 - L31 were not covered by tests
[warning] 34-42: zetaclient/chains/solana/signer/increment_nonce.go#L34-L42
Added lines #L34 - L42 were not covered by tests
[warning] 45-45: zetaclient/chains/solana/signer/increment_nonce.go#L45
Added line #L45 was not covered by tests
[warning] 52-64: zetaclient/chains/solana/signer/increment_nonce.go#L52-L64
Added lines #L52 - L64 were not covered by tests
[warning] 66-79: zetaclient/chains/solana/signer/increment_nonce.go#L66-L79
Added lines #L66 - L79 were not covered by tests
[warning] 82-94: zetaclient/chains/solana/signer/increment_nonce.go#L82-L94
Added lines #L82 - L94 were not covered by tests
[warning] 97-101: zetaclient/chains/solana/signer/increment_nonce.go#L97-L101
Added lines #L97 - L101 were not covered by tests
[warning] 103-105: zetaclient/chains/solana/signer/increment_nonce.go#L103-L105
Added lines #L103 - L105 were not covered by tests
[warning] 107-107: zetaclient/chains/solana/signer/increment_nonce.go#L107
Added line #L107 was not covered by tests
zetaclient/chains/solana/signer/signer.go
[warning] 135-135: zetaclient/chains/solana/signer/signer.go#L135
Added line #L135 was not covered by tests
[warning] 154-158: zetaclient/chains/solana/signer/signer.go#L154-L158
Added lines #L154 - L158 were not covered by tests
[warning] 161-161: zetaclient/chains/solana/signer/signer.go#L161
Added line #L161 was not covered by tests
[warning] 180-184: zetaclient/chains/solana/signer/signer.go#L180-L184
Added lines #L180 - L184 were not covered by tests
[warning] 187-187: zetaclient/chains/solana/signer/signer.go#L187
Added line #L187 was not covered by tests
[warning] 213-213: zetaclient/chains/solana/signer/signer.go#L213
Added line #L213 was not covered by tests
[warning] 265-269: zetaclient/chains/solana/signer/signer.go#L265-L269
Added lines #L265 - L269 were not covered by tests
[warning] 287-302: zetaclient/chains/solana/signer/signer.go#L287-L302
Added lines #L287 - L302 were not covered by tests
[warning] 305-308: zetaclient/chains/solana/signer/signer.go#L305-L308
Added lines #L305 - L308 were not covered by tests
[warning] 311-314: zetaclient/chains/solana/signer/signer.go#L311-L314
Added lines #L311 - L314 were not covered by tests
[warning] 316-316: zetaclient/chains/solana/signer/signer.go#L316
Added line #L316 was not covered by tests
🪛 markdownlint-cli2 (0.17.2)
changelog.md
15-15: Bare URL used
null
(MD034, no-bare-urls)
🔇 Additional comments (12)
pkg/contracts/solana/gateway.go (1)
45-46: LGTM!The new discriminator variable for the 'increment_nonce' instruction follows the established pattern and naming conventions.
cmd/zetae2e/local/local.go (1)
408-408: LGTM!The new test cases for Solana and SPL withdraw and call revert scenarios enhance the test coverage appropriately.
Also applies to: 420-420
changelog.md (1)
13-14: LGTM!The changelog entries accurately reflect the code changes and follow consistent formatting.
Also applies to: 18-19
e2e/e2etests/test_spl_withdraw_and_call.go (1)
12-12: LGTM!The import addition is necessary for the RevertOptions functionality.
e2e/e2etests/test_solana_withdraw_and_call.go (1)
59-67: LGTM! The revert options are properly configured.The addition of revert options with
OnRevertGasLimit: 0aligns with the expected behavior for Solana withdraw and call operations.e2e/e2etests/e2etests.go (1)
540-547: LGTM! The test definitions are well-structured.The new test cases for Solana and SPL withdraw and call revert scenarios follow the established naming conventions and include proper descriptions and argument definitions.
Also applies to: 556-563
e2e/runner/solana.go (2)
479-485: LGTM! The revert options are properly integrated.The function signature update and parameter passing to
WithdrawAndCall0are implemented correctly.Also applies to: 519-521
559-565: LGTM! The revert options are properly integrated.The function signature update and parameter passing to
WithdrawAndCall0are implemented correctly.Also applies to: 611-613
pkg/contracts/solana/gateway_message_test.go (1)
6-6: No issues with the newly added import.
This import forgithub.com/ethereum/go-ethereum/commonis clear and appropriate for Ethereum address handling.zetaclient/chains/solana/signer/signer.go (1)
265-269: Fallback logic may mask potential errors.
By defaulting tofallbackTxwhenever the error message does not contain"NonceMismatch", the system might inadvertently override legitimate transaction failures. Consider confirming that the fallback approach does not inadvertently process unintended errors.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 265-269: zetaclient/chains/solana/signer/signer.go#L265-L269
Added lines #L265 - L269 were not covered by testspkg/contracts/solana/gateway_message.go (1)
17-17: Instruction code is clearly assigned.
DefiningInstructionIncrementNonceas byte value7maintains consistency with the other instruction constants.pkg/contracts/solana/instruction.go (1)
81-94: Interface enhancement looks good!The addition of
InstructionDiscriminator()method to theOutboundInstructioninterface provides a consistent way to access instruction discriminators across all implementations.
Description
Just adding some unit tests that were missed in withdraw and call PRs. Tests are in same format as other outbounds unit tests.
How Has This Been Tested?
Summary by CodeRabbit
Documentation
New Features
Tests
Chores