feat: attach failure reason to solana increment nonce#3918
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 📝 WalkthroughWalkthroughThis update introduces the ability to attach failure reasons to Solana increment nonce operations. The changes extend instruction parameters, message structures, and fallback logic to capture and propagate failure reasons. Test cases and test data are updated to reflect the new behavior, and the changelog documents the feature addition. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Signer
participant Solana
participant Contracts
Client->>Signer: Initiate outbound transaction
Signer->>Solana: Broadcast transaction
Solana-->>Signer: Return error (if any)
Signer->>Signer: parseRPCErrorForFallback(error, program)
alt Fallback required
Signer->>Contracts: Add failure reason to MsgIncrementNonce
Signer->>Solana: Create & sign fallback increment nonce transaction
Solana-->>Signer: Return fallback transaction result
else No fallback
Signer->>Client: Return transaction result or error
end
sequenceDiagram
participant Signer
participant Contracts
Signer->>Contracts: MsgIncrementNonce.AddFailureReason(reason)
Signer->>Contracts: Serialize IncrementNonceInstructionParams (includes FailureReason)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 #3918 +/- ##
===========================================
- Coverage 64.84% 64.84% -0.01%
===========================================
Files 471 471
Lines 34417 34434 +17
===========================================
+ Hits 22319 22330 +11
- Misses 11069 11074 +5
- Partials 1029 1030 +1
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
zetaclient/chains/solana/signer/fallback_tx.go (1)
9-42: Consider adding test coverage for edge case error handling.The implementation correctly handles various error scenarios with appropriate early returns. However, static analysis indicates that error handling paths at lines 18, 23, and 28 lack test coverage. These edge cases should be tested to ensure robust error handling.
Consider adding test cases for:
- Non-RPC errors that don't contain "Error processing Instruction"
- RPC errors with invalid data structure
- RPC errors with missing logs field
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 18-18: zetaclient/chains/solana/signer/fallback_tx.go#L18
Added line #L18 was not covered by tests
[warning] 23-23: zetaclient/chains/solana/signer/fallback_tx.go#L23
Added line #L23 was not covered by tests
[warning] 28-28: zetaclient/chains/solana/signer/fallback_tx.go#L28
Added line #L28 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
contrib/localnet/solana/gateway.sois excluded by!**/*.so
📒 Files selected for processing (10)
changelog.md(1 hunks)pkg/contracts/solana/gateway.json(2 hunks)pkg/contracts/solana/gateway_message.go(2 hunks)pkg/contracts/solana/instruction.go(1 hunks)zetaclient/chains/solana/observer/outbound_test.go(2 hunks)zetaclient/chains/solana/signer/fallback_tx.go(1 hunks)zetaclient/chains/solana/signer/fallback_tx_test.go(1 hunks)zetaclient/chains/solana/signer/increment_nonce.go(1 hunks)zetaclient/chains/solana/signer/signer.go(3 hunks)zetaclient/testdata/solana/chain_901_outbound_tx_result_5dpFTsscUKCGVQzL9bAUSuEE6yLXaf7d1wMjZa7RLqvtSUtAdfcdxQHNsbfcS2Sfzu4zBVxMJC2KWzuaUUbg1ZGk.json(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/signer/increment_nonce.gozetaclient/chains/solana/observer/outbound_test.gopkg/contracts/solana/gateway_message.gopkg/contracts/solana/instruction.gozetaclient/chains/solana/signer/fallback_tx_test.gozetaclient/chains/solana/signer/signer.gozetaclient/chains/solana/signer/fallback_tx.go
🧠 Learnings (1)
zetaclient/chains/solana/signer/signer.go (1)
Learnt from: gartnera
PR: zeta-chain/node#3632
File: zetaclient/chains/solana/signer/signer.go:304-304
Timestamp: 2025-03-04T22:39:58.395Z
Learning: The Solana signer implementation in zetaclient/chains/solana/signer/signer.go has limited test coverage, particularly for the transaction broadcasting logic with fallback scenarios. Adding this coverage has been acknowledged as a potential future improvement outside the scope of immediate fixes.
🪛 GitHub Check: codecov/patch
zetaclient/chains/solana/signer/increment_nonce.go
[warning] 105-105: zetaclient/chains/solana/signer/increment_nonce.go#L105
Added line #L105 was not covered by tests
zetaclient/chains/solana/signer/signer.go
[warning] 331-338: zetaclient/chains/solana/signer/signer.go#L331-L338
Added lines #L331 - L338 were not covered by tests
[warning] 341-344: zetaclient/chains/solana/signer/signer.go#L341-L344
Added lines #L341 - L344 were not covered by tests
[warning] 346-346: zetaclient/chains/solana/signer/signer.go#L346
Added line #L346 was not covered by tests
[warning] 375-376: zetaclient/chains/solana/signer/signer.go#L375-L376
Added lines #L375 - L376 were not covered by tests
zetaclient/chains/solana/signer/fallback_tx.go
[warning] 18-18: zetaclient/chains/solana/signer/fallback_tx.go#L18
Added line #L18 was not covered by tests
[warning] 23-23: zetaclient/chains/solana/signer/fallback_tx.go#L23
Added line #L23 was not covered by tests
[warning] 28-28: zetaclient/chains/solana/signer/fallback_tx.go#L28
Added line #L28 was not covered by tests
[warning] 54-55: zetaclient/chains/solana/signer/fallback_tx.go#L54-L55
Added lines #L54 - L55 were not covered by tests
🔇 Additional comments (11)
changelog.md (1)
32-32: LGTM: Feature properly documented in changelog.The changelog entry accurately describes the new failure reason attachment capability for Solana increment nonce operations and follows the established format.
pkg/contracts/solana/instruction.go (1)
171-172: LGTM: Clean addition of failure reason field.The
FailureReasonfield is properly typed, documented, and follows Go naming conventions. The field placement within the struct is logical and maintains consistency with other fields.pkg/contracts/solana/gateway.json (1)
1311-1312: LGTM: Clean schema extension for failure reason tracking.The addition of the
failure_reasonstring parameter to theincrement_nonceinstruction follows the established schema pattern and includes appropriate documentation. This aligns well with the broader feature to capture detailed failure context for outbound transaction failures.Also applies to: 1385-1388
pkg/contracts/solana/gateway_message.go (3)
150-152: Well-structured field addition with proper encapsulation.The private
failureReasonfield follows Go conventions and maintains good encapsulation of the failure context data.
164-172: Clean accessor pattern implementation.The getter and setter methods follow standard Go conventions and provide appropriate access control to the failure reason field. The method naming is clear and consistent with the existing codebase.
189-206:Details
❓ Verification inconclusive
Verify exclusion of failure reason from hash calculation.
The
Hash()method intentionally excludes thefailureReasonfield from the cryptographic hash computation. This appears to be a deliberate design choice, treating the failure reason as metadata that shouldn't affect message integrity verification.Please confirm this design decision is intentional. If the failure reason should be part of the message integrity verification, it should be included in the hash calculation:
🏁 Script executed:
#!/bin/bash # Search for how other message fields are handled in hash calculations rg -A 10 -B 5 "func.*Hash.*\[32\]byte" --type goLength of output: 10429
🏁 Script executed:
#!/bin/bash # Display the definition and fields of MsgIncrementNonce rg -n "type MsgIncrementNonce" -A 10 pkg/contracts/solana/gateway_message.go # Check for any occurrence of failureReason in this file rg -n "failureReason" -n pkg/contracts/solana/gateway_message.goLength of output: 697
Confirm Intentional Omission of
failureReasonin Hash ComputationThe
MsgIncrementNonce.Hash()method serializes onlychainID,nonce, andamount, omitting thefailureReasonfield (metadata) from the digest:• File:
pkg/contracts/solana/gateway_message.go
• Method:func (msg *MsgIncrementNonce) Hash() [32]byte(lines ~189–206)Please verify this is a deliberate design choice. If
failureReasonmust be covered by the integrity check, append it before hashing, for example:message = append(message, []byte(msg.failureReason)...) return crypto.Keccak256Hash(message)Otherwise, no further action is needed.
zetaclient/testdata/solana/chain_901_outbound_tx_result_5dpFTsscUKCGVQzL9bAUSuEE6yLXaf7d1wMjZa7RLqvtSUtAdfcdxQHNsbfcS2Sfzu4zBVxMJC2KWzuaUUbg1ZGk.json (1)
1-68: Test data properly demonstrates the failure reason feature.The updated test data correctly includes the failure reason in the log messages, aligning with the PR objective to attach failure reasons to Solana increment nonce operations.
zetaclient/chains/solana/signer/fallback_tx.go (1)
44-59: Well-implemented failure reason extraction logic.The function correctly extracts failure reasons from logs by identifying lines that start with "Program " and contain " failed". The approach of returning the first failure is appropriate as noted in the comment.
Note: Lines 54-55 (empty failures array case) lack test coverage but the implementation correctly handles this edge case.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 54-55: zetaclient/chains/solana/signer/fallback_tx.go#L54-L55
Added lines #L54 - L55 were not covered by testszetaclient/chains/solana/signer/fallback_tx_test.go (1)
27-114: Comprehensive test coverage for the refactored fallback logic.The tests effectively validate both the fallback decision and failure reason extraction. Test scenarios appropriately cover various error conditions including NonceMismatch, program invocations, and invalid error types.
zetaclient/chains/solana/signer/signer.go (2)
40-43: Appropriate refactoring to support dynamic fallback transaction creation.The change from storing a pre-signed
FallbackTxto storing aFallbackMsgenables dynamic attachment of failure reasons at broadcast time, aligning well with the PR objectives.
361-378: Clean simplification of the outbound creation logic.The method has been appropriately simplified to only sign the main transaction and store the fallback message, deferring fallback transaction creation to broadcast time when the actual failure reason is available.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 375-376: zetaclient/chains/solana/signer/signer.go#L375-L376
Added lines #L375 - L376 were not covered by tests
lumtis
left a comment
lumtis
left a comment
There was a problem hiding this comment.
Any way to add an assertion in the Solana withdraw and call fail E2E test?
3 participants
Description
solana PR: zeta-chain/protocol-contracts-solana#115
increment logs example with failure reason attached:
How Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation