fix: look into Solana program log message to determine SPL token withdrawal failure #3231
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant updates across multiple components, focusing on enhancing functionalities related to Solana transactions, particularly for SPL token handling. Key changes include the refactoring of the TSS package, improvements in error handling for transaction validations, and the introduction of new methods and constants for better logging and withdrawal instruction management. Additionally, the changelog reflects modifications in tests, structural code improvements, and the removal of outdated features, all aimed at increasing maintainability and operational clarity. Changes
Possibly related PRs
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 |
ws4charlie
added
SOLANA_TESTS
ws4charlie
changed the title
ws4charlie
requested review from
brewmaster012,
kingpinXD,
lumtis,
skosito,
swift1337 and
fbac
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3231 +/- ##
========================================
Coverage 62.00% 62.00%
========================================
Files 428 428
Lines 30469 30476 +7
========================================
+ Hits 18891 18897 +6
- Misses 10738 10740 +2
+ Partials 840 839 -1
|
![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 (7)
pkg/contracts/solana/instruction_test.go (1)
83-123: LGTM: Well-structured test with room for enhancement.The test function follows good practices with table-driven tests and clear scenarios. Consider extracting repeated strings into constants for better maintainability:
const ( testSigner = "0xaD32427bA235a8350b7805C1b85147c8ea03F437" + testInstructionLog = "Program log: Instruction: WithdrawSPLToken" )zetaclient/chains/solana/observer/outbound.go (2)
Line range hint
338-342: Consider using a constant for instruction count validationFor better maintainability and clarity, consider extracting the expected instruction count to a named constant.
+const expectedInstructionCount = 1 + // there should be only one single instruction ('withdraw' or 'withdraw_spl_token' or 'whitelist_spl_mint') -if len(tx.Message.Instructions) != 1 { - return nil, fmt.Errorf("want 1 instruction, got %d", len(tx.Message.Instructions)) +if len(tx.Message.Instructions) != expectedInstructionCount { + return nil, fmt.Errorf("want %d instruction(s), got %d", expectedInstructionCount, len(tx.Message.Instructions)) }
Line range hint
355-365: Consider adding exhaustive coin type validationWhile the switch statement handles known coin types, it would be beneficial to validate that all possible coin types are explicitly handled during development.
Consider creating a test that verifies all
CoinTypeenum values are handled:func TestParseGatewayInstructionHandlesAllCoinTypes(t *testing.T) { // Get all CoinType values using reflection coinTypeType := reflect.TypeOf((*coin.CoinType)(nil)).Elem() for i := 0; i < coinTypeType.NumField(); i++ { field := coinTypeType.Field(i) if !field.IsExported() { continue } coinType := coin.CoinType(field.Tag.Get("protobuf_val")) // Verify each coin type is handled in the switch switch coinType { case coin.CoinType_Gas, coin.CoinType_Cmd, coin.CoinType_ERC20: // handled default: t.Errorf("CoinType %s is not handled in ParseGatewayInstruction", coinType) } } }zetaclient/chains/solana/observer/outbound_test.go (2)
510-510: LGTM! Consider adding more test cases.The error message update improves clarity by specifically mentioning "withdraw_spl_token instruction". While the current test coverage is good, consider adding these test cases:
- Test with invalid decimals
- Test with maximum token amount
- Test with zero token amount
Line range hint
1-511: Document test data management.The test file loads data from
TestDataDirbut its path and format aren't documented. Consider:
- Adding a package-level comment describing the test data structure
- Documenting the expected format of archived transaction results
- Including instructions for generating new test data
pkg/contracts/solana/instruction.go (2)
123-126: Implement unit tests for 'WithdrawInstructionParams.Failed' methodThe
Failedmethod forWithdrawInstructionParamscurrently always returnsfalse. To ensure future robustness, consider adding unit tests to validate this behavior.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 124-125: pkg/contracts/solana/instruction.go#L124-L125
Added lines #L124 - L125 were not covered by tests
Line range hint
200-216: Add unit tests for 'ParseInstructionWithdrawSPL' functionTo increase code reliability, implement unit tests that cover successful parsing and error handling of the
ParseInstructionWithdrawSPLfunction.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 203-203: pkg/contracts/solana/instruction.go#L203
Added line #L203 was not covered by tests
[warning] 212-212: pkg/contracts/solana/instruction.go#L212
Added line #L212 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
changelog.md(1 hunks)pkg/contracts/solana/instruction.go(7 hunks)pkg/contracts/solana/instruction_test.go(2 hunks)zetaclient/chains/solana/observer/outbound.go(4 hunks)zetaclient/chains/solana/observer/outbound_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
pkg/contracts/solana/instruction.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
pkg/contracts/solana/instruction_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/observer/outbound_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🪛 GitHub Check: codecov/patch
pkg/contracts/solana/instruction.go
[warning] 124-125: pkg/contracts/solana/instruction.go#L124-L125
Added lines #L124 - L125 were not covered by tests
[warning] 203-203: pkg/contracts/solana/instruction.go#L203
Added line #L203 was not covered by tests
[warning] 212-212: pkg/contracts/solana/instruction.go#L212
Added line #L212 was not covered by tests
[warning] 269-270: pkg/contracts/solana/instruction.go#L269-L270
Added lines #L269 - L270 were not covered by tests
zetaclient/chains/solana/observer/outbound.go
[warning] 161-161: zetaclient/chains/solana/observer/outbound.go#L161
Added line #L161 was not covered by tests
[warning] 163-165: zetaclient/chains/solana/observer/outbound.go#L163-L165
Added lines #L163 - L165 were not covered by tests
🔇 Additional comments (9)
pkg/contracts/solana/instruction_test.go (1)
4-4: LGTM: Import addition is appropriate.
The fmt package import is correctly added and utilized for string formatting in the test cases.
zetaclient/chains/solana/observer/outbound.go (2)
318-319: LGTM: Simplified nonce verification
The direct comparison improves code readability while maintaining the same functionality.
161-165:
Add test coverage for outbound status determination logic
The new logic for determining outbound status based on log messages lacks test coverage. This is a critical path that affects transaction finality.
Let's verify the error handling implementation:
Would you like me to help create comprehensive test cases for this new functionality?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 161-161: zetaclient/chains/solana/observer/outbound.go#L161
Added line #L161 was not covered by tests
[warning] 163-165: zetaclient/chains/solana/observer/outbound.go#L163-L165
Added lines #L163 - L165 were not covered by tests
changelog.md (1)
17-17: LGTM! The changelog entry is well-formatted and accurately describes the change.
The entry follows the established format and clearly describes the purpose of PR #3231, which aligns with the PR objectives.
pkg/contracts/solana/instruction.go (5)
5-6: Verify compatibility of the 'slices' package with the project's Go version
The slices package was introduced in Go 1.21 as part of the standard library. Please ensure that the project's Go version is 1.21 or higher to support this package.
15-22: Log message constants are well-defined and enhance clarity
Defining MsgWithdrawSPLTokenSuccess and MsgWithdrawSPLTokenNonExistentAta as constants improves maintainability and consistency in log message handling.
76-78: Addition of 'Failed' method to 'OutboundInstruction' interface ensures uniform error handling
Including the Failed(logMessages []string) bool method in the OutboundInstruction interface provides a standardized way to determine instruction failure based on log messages.
291-296: Usage of 'containsLogMessage' function enhances log message processing
The containsLogMessage function efficiently checks for substrings within log messages using slices.IndexFunc, improving readability and performance.
188-199:
Review the logic in 'WithdrawSPLInstructionParams.Failed' method for correctness and security
The Failed method uses log messages to determine the failure state of a withdraw_spl_token instruction. Ensure that:
- The logic correctly interprets the presence or absence of success and failure messages.
- The assumptions about log messages are validated to prevent malicious exploitation.
Consider adding comprehensive unit tests to cover different log scenarios, including cases where both messages are present.
There was a problem hiding this comment.
looks good to me apart from one small comment
also, for completness and clarity, can we please add details in PR description or issue, why is this needed for this type of failure, but not for type of failure for example if wrong nonce or signature is provided, in that case it is just ignored on zetaclient or?
Added more details according to our conversation in the Slack. |
Description
This PR is a part of the whole fix to issue: zeta-chain/protocol-contracts-solana#63
withdraw_spl_token. It is up to @skosito to use whatever message text reasonable for this purpose.zetaclientlooks into log message to determine if the SPL token withdrawal should be failed or not and then post corresponding outbound status to thezetacoreto finalize or abort (on failure) the CCTX.In the gateway program, there are only TWO cases when a
withdraw_spl_tokentx succeed:The gateway program should fail the
withdraw_spl_tokentx on any other errors: nonce mismatch, wrong signature, message hash mismatch, ata and mint address mismatch, etc.How Has This Been Tested?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Tests