fix: improve fallback tx error handling#3770
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 📝 WalkthroughWalkthroughThe pull request integrates several changes across different modules. It updates the changelog with a note on fallback transaction error handling, amends a test payload for a specific revert condition, introduces a new function to analyze error log sequences in Solana contracts, and adds corresponding tests. Additionally, the error handling in the Solana signer’s Changes
Sequence Diagram(s)sequenceDiagram
participant S as Signer
participant N as Solana Network
participant C as Contracts (Error Checker)
S->>N: Broadcast transaction
N-->>S: Return error response
S->>C: Invoke ProgramInvokedAfterTargetInErrStr(errMsg, targetProgram)
C-->>S: Return true/false based on error log scan
alt Fallback condition met
S->>N: Broadcast fallback transaction
else
S->>S: Handle error without fallback
end
Possibly related PRs
Suggested labels
Suggested reviewers
🪧 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 #3770 +/- ##
===========================================
+ Coverage 64.37% 64.40% +0.03%
===========================================
Files 462 463 +1
Lines 32915 32961 +46
===========================================
+ Hits 21188 21229 +41
- Misses 10755 10757 +2
- Partials 972 975 +3
🚀 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: 0
🧹 Nitpick comments (2)
pkg/contracts/solana/instruction_test.go (1)
82-197: Good test coverage, but could benefit from some refinements.The test function
Test_ProgramInvokedAfterTargetInErrStrprovides comprehensive coverage for theProgramInvokedAfterTargetInErrStrfunction, covering key scenarios including:
- No program invoked after the target gateway program
- A different program invoked after the target gateway program
- The gateway program invoked again after the initial invocation
I recommend the following improvements for better maintainability and robustness:
+// targetGatewayProgramID is the Solana program ID used for testing +const targetGatewayProgramID = "94U5AHQMKkV5txNJ17QPXWoh474PheGou6cNP2FEuL1d" func Test_ProgramInvokedAfterTargetInErrStr(t *testing.T) { - t.Run("no program invoked after gateway", func(t *testing.T) { + // Define test cases to avoid repetition of test structure + testCases := []struct { + name string + errorStr string + targetProgram string + expected bool + }{ + { + name: "no program invoked after gateway", + targetProgram: targetGatewayProgramID, + expected: false, + errorStr: `(*jsonrpc.RPCError)(0x400233b920)({ // ...error string content... + })`, + }, + { + name: "program invoked after gateway", + targetProgram: targetGatewayProgramID, + expected: true, + errorStr: `(*jsonrpc.RPCError)(0x40019dc210)({ // ...error string content... + })`, + }, + { + name: "gateway invoked after gateway", + targetProgram: targetGatewayProgramID, + expected: true, + errorStr: `(*jsonrpc.RPCError)(0x40019dc210)({ // ...error string content... + })`, + }, + { + name: "empty error string", + targetProgram: targetGatewayProgramID, + expected: false, + errorStr: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + invoked := contracts.ProgramInvokedAfterTargetInErrStr(tc.errorStr, tc.targetProgram) + if tc.expected { + require.True(t, invoked) + } else { + require.False(t, invoked) + } + }) + }Consider adding a comment to explain the purpose of these tests and the structure of the error strings, e.g.:
+// Test_ProgramInvokedAfterTargetInErrStr verifies the ProgramInvokedAfterTargetInErrStr function +// which analyzes Solana JSON-RPC error logs to determine if a program was invoked after +// a target program. This is used for transaction fallback decisions. func Test_ProgramInvokedAfterTargetInErrStr(t *testing.T) {changelog.md (1)
21-21: Changelog Entry for PR 3770 – Fallback TX Error Handling:
The new entry is clear and properly formatted, aligning with the other changelog items in the Fixes section. It succinctly indicates the improvement in fallback transaction error handling. Consider whether additional context (such as mentioning that this change alters how errors from a fallback scenario are differentiated based on program invocation) might further aid future readers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
contrib/localnet/solana/connected.sois excluded by!**/*.socontrib/localnet/solana/connected_spl.sois excluded by!**/*.so
📒 Files selected for processing (5)
changelog.md(1 hunks)e2e/e2etests/test_solana_withdraw_and_call_revert_with_call.go(1 hunks)pkg/contracts/solana/instruction.go(2 hunks)pkg/contracts/solana/instruction_test.go(1 hunks)zetaclient/chains/solana/signer/signer.go(1 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.
e2e/e2etests/test_solana_withdraw_and_call_revert_with_call.gopkg/contracts/solana/instruction.gozetaclient/chains/solana/signer/signer.gopkg/contracts/solana/instruction_test.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-27T14:00:41.939Z
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.
🧬 Code Definitions (2)
zetaclient/chains/solana/signer/signer.go (1)
pkg/contracts/solana/instruction.go (1)
ProgramInvokedAfterTargetInErrStr(589-612)
pkg/contracts/solana/instruction_test.go (1)
pkg/contracts/solana/instruction.go (1)
ProgramInvokedAfterTargetInErrStr(589-612)
🪛 GitHub Check: codecov/patch
zetaclient/chains/solana/signer/signer.go
[warning] 347-353: zetaclient/chains/solana/signer/signer.go#L347-L353
Added lines #L347 - L353 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: start-solana-test / e2e
🔇 Additional comments (3)
e2e/e2etests/test_solana_withdraw_and_call_revert_with_call.go (1)
54-54: More specific test case with NonceMismatch error handling.The test payload has been expanded to include "NonceMismatch", which aligns with the enhanced error handling in the
broadcastOutboundfunction that now differentiates between regular errors and NonceMismatch errors.pkg/contracts/solana/instruction.go (1)
587-612: Good implementation of program invocation analysis.This function provides a necessary utility to determine if other programs were invoked after a specific target program, which enables more precise error handling in Solana transactions. The implementation uses a clean regex approach to identify program invocations in error logs.
zetaclient/chains/solana/signer/signer.go (1)
347-353:Details
❓ Verification inconclusive
Improved fallback transaction handling logic for program invocations.
The conditional logic now handles two distinct scenarios for using fallback transactions:
- When a program is invoked after the gateway, regardless of error type
- When an error occurs that is not a NonceMismatch error
This enhancement allows for more robust handling of Solana transaction failures.
This implementation lacks test coverage. Consider adding unit tests specifically for this error handling logic to ensure it behaves as expected in various scenarios.
🏁 Script executed:
#!/bin/bash # Check if there are tests covering the broadcastOutbound function in the Solana signer rg -A 10 -B 10 "Test.*broadcastOutbound" --type goLength of output: 52
Enhanced Fallback Transaction Handling – Test Coverage Needed
The updated conditional logic in
zetaclient/chains/solana/signer/signer.goproperly refines the fallback transaction mechanism for Solana. It differentiates between cases where a program is invoked after the gateway—ensuring that the fallback is used regardless of the error—and when NonceMismatch errors should be bypassed to accommodate multiple relay attempts.However, our investigation indicates that there is currently no unit test covering this logic (as verified by the absence of matching tests for
broadcastOutbound). To bolster confidence in these changes, please add dedicated tests that simulate:
- An error message containing
"Error processing Instruction"with a valid fallback transaction, including scenarios where a program is invoked after the gateway.- A case where the error is a
"NonceMismatch"and the fallback transaction should not be applied when no post-target program invocation is detected.Once these tests are in place, we can ensure the robustness of error handling in production.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 347-353: zetaclient/chains/solana/signer/signer.go#L347-L353
Added lines #L347 - L353 were not covered by tests
lumtis
left a comment
lumtis
left a comment
There was a problem hiding this comment.
Wondering, do we have a case where we produce a NonceMismatch from signers in E2E tests?
Is it something that could be reproduced?
issue reported is reproduced in e2e test in this repo, connected program is reverting with NonceMismatch error |
The test check that the false positive is handled, but doesn't check for the actual NonceMismatch from ZetaClient that should be retried and not reverted? |
those are happening constantly as we have 2 relayers locally that are submitting txs, so that is implicitly tested out with solana outbounds working |
4 participants
Description
extended solana examples a bit to enable e2e test zeta-chain/protocol-contracts-solana#97
idea is to check error logs for
Program <PROGRAM_ID> invokedlogs, and to check if some program is invoked after gatewaywe probably should look into other solana golang libraries, this one just gives error string so we must do some parsing to figure out on our own
How Has This Been Tested?
Summary by CodeRabbit
Documentation
Bug Fixes
Tests