fix: skip solana unsupported transaction version #3206
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📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several updates across multiple files, focusing on enhancing transaction handling for the Solana blockchain. Key changes include the addition of a new section in the changelog for better organization of unreleased features, updates to dependency versions in 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
|
ws4charlie
requested review from
brewmaster012,
kingpinXD,
lumtis,
skosito,
swift1337 and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
zetaclient/chains/solana/rpc/rpc_live_test.go (1)
26-54: Enhance test documentation and coverageThe test function would benefit from:
- A function-level documentation explaining the test purpose and scenarios
- Constants for test values
- Additional assertions on transaction properties
Consider refactoring like this:
+// LiveTest_GetTransactionWithVersion tests the behavior of GetTransactionWithMaxVersion +// for both supported and unsupported transaction versions using a known version "0" +// transaction from devnet. func LiveTest_GetTransactionWithVersion(t *testing.T) { + const ( + // Known version "0" transaction from devnet + testTxSignature = "Wqgj7hAaUUSfLzieN912G7GxyGHijzBZgY135NtuFtPRjevK8DnYjWwQZy7LAKFQZu582wsjuab2QP27VMUJzAi" + ) // create a Solana devnet RPC client client := solanarpc.New(solanarpc.DevNet_RPC) - txSig := solana.MustSignatureFromBase58( - "Wqgj7hAaUUSfLzieN912G7GxyGHijzBZgY135NtuFtPRjevK8DnYjWwQZy7LAKFQZu582wsjuab2QP27VMUJzAi", - ) + txSig := solana.MustSignatureFromBase58(testTxSignature) t.Run("should get the transaction if the version is supported", func(t *testing.T) { ctx := context.Background() txResult, err := rpc.GetTransactionWithMaxVersion( ctx, client, txSig, &solanarpc.MaxSupportedTransactionVersion0, ) require.NoError(t, err) require.NotNil(t, txResult) + // Verify transaction properties + require.Equal(t, uint8(0), txResult.Version) + require.NotNil(t, txResult.Transaction) })zetaclient/chains/solana/rpc/rpc.go (1)
130-147: Enhance function documentation and error handling.While the implementation is solid, consider the following improvements:
Add comprehensive function documentation explaining:
- The purpose of maxTxVersion parameter
- The nil return behavior for unsupported versions
- The error handling strategy
Consider more precise error handling to avoid masking other potential JSON-RPC errors.
Here's a suggested improvement:
-// GetTransactionWithMaxVersion fetches a transaction with the given signature and max version. +// GetTransactionWithMaxVersion fetches a transaction with the given signature and max version. +// Parameters: +// - ctx: Context for the RPC call +// - client: Solana RPC client interface +// - signature: Transaction signature to fetch +// - maxTxVersion: Maximum supported transaction version. If nil, no version constraint is applied. +// +// Returns: +// - (*rpc.GetTransactionResult, nil) for successful retrieval +// - (nil, nil) for transactions with unsupported versions +// - (nil, error) for other errors func GetTransactionWithMaxVersion( ctx context.Context, client interfaces.SolanaRPCClient, signature solana.Signature, maxTxVersion *uint64, ) (*rpc.GetTransactionResult, error) { txResult, err := client.GetTransaction(ctx, signature, &rpc.GetTransactionOpts{ MaxSupportedTransactionVersion: maxTxVersion, }) - // skip unsupported transaction version error - if err != nil && strings.Contains(err.Error(), ErrorCodeUnsupportedTransactionVersion) { + // Handle specific case of unsupported transaction version + if err != nil && isUnsupportedVersionError(err) { return nil, nil } return txResult, err } + +// isUnsupportedVersionError checks if the error is specifically about unsupported transaction version +func isUnsupportedVersionError(err error) bool { + return err != nil && strings.Contains(err.Error(), ErrorCodeUnsupportedTransactionVersion) }zetaclient/chains/solana/observer/inbound.go (2)
103-104: Consider making transaction version configurableWhile using
rpc.MaxSupportedTransactionVersion0works for the current use case, consider making this configurable to facilitate future version support without code changes.-maxTxVersion := rpc.MaxSupportedTransactionVersion0 +maxTxVersion := ob.ChainParams().MaxSupportedTransactionVersion
110-119: Enhance error handling and loggingThe error handling structure could be simplified while improving logging for better observability.
-switch { -case txResult == nil: - ob.Logger().Inbound.Warn().Msgf("ObserveInbound: skip unsupported transaction sig %s", sigString) -default: - // filter inbound events and vote - err = ob.FilterInboundEventsAndVote(ctx, txResult) - if err != nil { - // we have to re-scan this signature on next ticker - return errors.Wrapf(err, "error FilterInboundEventAndVote for chain %d sig %s", chainID, sigString) - } +if txResult == nil { + ob.Logger().Inbound.Warn(). + Str("signature", sigString). + Msg("ObserveInbound: skipping unsupported transaction") + return nil +} + +// filter inbound events and vote +if err := ob.FilterInboundEventsAndVote(ctx, txResult); err != nil { + // we have to re-scan this signature on next ticker + return errors.Wrapf(err, "error FilterInboundEventAndVote for chain %d sig %s", chainID, sigString) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
changelog.md(1 hunks)go.mod(2 hunks)zetaclient/chains/solana/observer/inbound.go(1 hunks)zetaclient/chains/solana/rpc/rpc.go(3 hunks)zetaclient/chains/solana/rpc/rpc_live_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- changelog.md
🧰 Additional context used
📓 Path-based instructions (3)
zetaclient/chains/solana/observer/inbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/rpc/rpc.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/rpc/rpc_live_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (6)
zetaclient/chains/solana/rpc/rpc_live_test.go (2)
20-20: LGTM: Test integration is clean and consistent
The new test function is properly integrated into the live test suite, maintaining consistency with existing patterns.
30-34: Consider making the test more resilient to devnet resets
The hardcoded transaction signature from devnet could become invalid if the devnet is reset. Consider:
- Adding a comment explaining why this specific transaction was chosen
- Having a fallback mechanism or documenting the process to update the test data
✅ Verification successful
Transaction is still valid and accessible on devnet
The hardcoded transaction signature is currently valid and accessible on devnet. The transaction represents a good test case as it:
- Has version "0"
- Contains successful execution status
- Includes compute budget instructions
- Demonstrates a deposit operation with inner instructions
The test's choice of this specific transaction is well-justified for testing version handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if the transaction still exists on devnet
curl -X POST -H "Content-Type: application/json" -d '{
"jsonrpc": "2.0",
"id": 1,
"method": "getTransaction",
"params": [
"Wqgj7hAaUUSfLzieN912G7GxyGHijzBZgY135NtuFtPRjevK8DnYjWwQZy7LAKFQZu582wsjuab2QP27VMUJzAi",
{"encoding": "json", "maxSupportedTransactionVersion": 0}
]
}' https://api.devnet.solana.com
Length of output: 2943
zetaclient/chains/solana/rpc/rpc.go (2)
5-5: LGTM! Well-documented constant with proper source reference.
The error code constant is properly defined and documented with a link to the Solana source code, following Go naming conventions.
Also applies to: 22-25
130-147: Verify implementation meets PR objectives.
The implementation aligns well with the PR objectives:
- Handles version "0" transactions through the
maxTxVersionparameter - Gracefully skips unsupported transaction versions as required
Let's verify the error handling behavior:
✅ Verification successful
Implementation correctly handles unsupported transaction versions
The verification confirms the implementation aligns with the PR objectives and follows consistent error handling patterns:
- The error code
ErrorCodeUnsupportedTransactionVersion = "-32015"is properly defined and referenced - The changelog entry (PR fix: skip solana unsupported transaction version #3206) explicitly confirms the intention to skip unsupported transaction versions
- The error handling pattern is clean and follows best practices by checking both for nil error and specific error code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent error handling across the codebase
# Look for other places where we handle transaction version errors
rg -A 2 "ErrorCodeUnsupportedTransactionVersion|unsupported.*version|version.*unsupported"
# Check for similar error handling patterns
ast-grep --pattern 'if $err != nil && strings.Contains($err.Error(), $_)'
Length of output: 1190
go.mod (2)
285-285: Review security improvements in mongo-driver v1.12.2
The update to go.mongodb.org/mongo-driver v1.12.2 is a minor version bump that might include security fixes.
✅ Verification successful
No security-related changes found in mongo-driver v1.12.2
Based on the release notes, version 1.12.2 focuses on performance improvements in marshaling and compression, along with bug fixes in Client Side Encryption and transaction handling. No security-related changes or vulnerabilities were addressed in this release.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for security improvements in mongo-driver v1.12.2
# Get the changelog or release notes
gh api repos/mongodb/mongo-go-driver/releases/tags/v1.12.2 --jq .body
# Check for any security-related commits
gh api repos/mongodb/mongo-go-driver/commits --jq '.[] | select(.commit.message | test("security|CVE|vulnerability"; "i")) | .commit.message'
Length of output: 1957
29-29: Verify compatibility with solana-go v1.12.0
The update to github.com/gagliardetto/solana-go v1.12.0 aligns with the PR's objective to support version "0" Solana transactions.
✅ Verification successful
Update to solana-go v1.12.0 appears safe to proceed
Based on the changelog and codebase analysis:
- No breaking changes to transaction-related APIs were introduced in v1.12.0
- Changes are primarily bug fixes and feature additions
- Existing transaction handling code in the codebase remains compatible
Notable improvements that benefit this update:
- Memory leak fix in WebSocket client
- Pointer dereference fixes
- Added version support in TokenBalance type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes in solana-go v1.12.0
# Get the changelog or release notes
gh api repos/gagliardetto/solana-go/releases/tags/v1.12.0 --jq .body
# Search for any usage of potentially changed APIs
rg -l "solana-go" | xargs rg "GetTransaction|ParseTransaction"
Length of output: 12733
ws4charlie
added
SOLANA_TESTS
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3206 +/- ##
===========================================
- Coverage 62.74% 62.69% -0.06%
===========================================
Files 425 424 -1
Lines 30231 30244 +13
===========================================
- Hits 18969 18961 -8
- Misses 10425 10446 +21
Partials 837 837
|
Description
There are two types of Solana transactions in Solana network:
legacyand0. Current zetaclient implementation can only pulllegacyinbound transaction and returns error on version0. This will blocks inbound observation if user send a deposit tx to gateway using version0.The fix:
0in the RPC querygetTransaction.How Has This Been Tested?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests