refactor(zetaclient): wait for exact PDA nonce before signing Solana outbound#3940
refactor(zetaclient): wait for exact PDA nonce before signing Solana outbound#3940ws4charlie merged 11 commits intodevelopfrom
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 changes introduce a mechanism to wait for the exact PDA nonce before processing Solana outbound transactions, update commitment levels for transaction handling, and refine retry logic. The SolanaRPCClient interface and its mock gain a new method to fetch account info with options. The changelog is updated for accuracy and completeness. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Signer
participant SolanaRPC
participant Zetacore
Client->>Signer: TryProcessOutbound(ctx, cctx, zetacoreClient, height)
Note right of Signer: Create context with pdaNonceWaitTimeout
Signer->>SolanaRPC: waitExactGatewayNonce(ctx, nonce)
loop Until PDA nonce == target nonce or timeout
SolanaRPC->>SolanaRPC: GetAccountInfoWithOpts(ctx, account, opts)
SolanaRPC-->>Signer: AccountInfoResult (with current nonce)
alt PDA nonce < target
Note right of Signer: Sleep and retry
else PDA nonce == target
Note right of Signer: Proceed
else PDA nonce > target or error/timeout
Note right of Signer: Abort processing
end
end
alt PDA nonce == target
Signer->>Signer: signTx(ctx, inst, limit)
Signer->>Signer: broadcastOutbound(ctx, outbound, chainID, nonce, logger, zetacoreClient)
Signer-->>Client: Outbound processed
else
Signer-->>Client: Abort processing
end
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
|
|
!!!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
changed the title
ws4charlie
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3940 +/- ##
===========================================
- Coverage 64.18% 64.08% -0.10%
===========================================
Files 474 474
Lines 34803 34854 +51
===========================================
Hits 22337 22337
- Misses 11438 11489 +51
Partials 1028 1028
🚀 New features to boost your workflow:
|
ws4charlie
added
chain:solana
SOLANA_TESTS
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
zetaclient/chains/solana/signer/signer.go (2)
321-321: Consider adding a comment for clarity.While the range-based loop is cleaner, it might be less immediately clear that this loops exactly
broadcastRetriestimes.Consider adding a comment:
// try broacasting tx with increasing backoff (1s, 2s, 4s, 8s, 16s, 32s, 64s) // to tolerate tx nonce mismatch with PDA nonce or unknown RPC error backOff := broadcastBackoff +// retry up to broadcastRetries times for range broadcastRetries {🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 321-321: zetaclient/chains/solana/signer/signer.go#L321
Added line #L321 was not covered by tests
500-505: Simplify timeout handling.The manual deadline check is redundant since
ctx.Err()already returnscontext.DeadlineExceededwhen the deadline is exceeded.Simplify the timeout handling:
for { if ctx.Err() != nil { + logger.Error().Err(ctx.Err()).Msgf("context cancelled while waiting for gateway nonce") return false } - // check timeout to avoid infinite waiting - if deadline, ok := ctx.Deadline(); ok { - if time.Now().After(deadline) { - logger.Error().Msgf("timeout reached on waiting for gateway nonce") - return false - } - }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 500-504: zetaclient/chains/solana/signer/signer.go#L500-L504
Added lines #L500 - L504 were not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
changelog.md(1 hunks)zetaclient/chains/interfaces/interfaces.go(1 hunks)zetaclient/chains/solana/signer/signer.go(6 hunks)zetaclient/testutils/mocks/solana_rpc.go(2 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/interfaces/interfaces.gozetaclient/testutils/mocks/solana_rpc.gozetaclient/chains/solana/signer/signer.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.
🧬 Code Graph Analysis (1)
zetaclient/testutils/mocks/solana_rpc.go (1)
zetaclient/chains/interfaces/interfaces.go (1)
SolanaRPCClient(141-182)
🪛 GitHub Check: codecov/patch
zetaclient/chains/solana/signer/signer.go
[warning] 232-238: zetaclient/chains/solana/signer/signer.go#L232-L238
Added lines #L232 - L238 were not covered by tests
[warning] 259-259: zetaclient/chains/solana/signer/signer.go#L259
Added line #L259 was not covered by tests
[warning] 321-321: zetaclient/chains/solana/signer/signer.go#L321
Added line #L321 was not covered by tests
[warning] 342-342: zetaclient/chains/solana/signer/signer.go#L342
Added line #L342 was not covered by tests
[warning] 487-497: zetaclient/chains/solana/signer/signer.go#L487-L497
Added lines #L487 - L497 were not covered by tests
[warning] 500-504: zetaclient/chains/solana/signer/signer.go#L500-L504
Added lines #L500 - L504 were not covered by tests
[warning] 508-515: zetaclient/chains/solana/signer/signer.go#L508-L515
Added lines #L508 - L515 were not covered by tests
[warning] 519-523: zetaclient/chains/solana/signer/signer.go#L519-L523
Added lines #L519 - L523 were not covered by tests
[warning] 525-539: zetaclient/chains/solana/signer/signer.go#L525-L539
Added lines #L525 - L539 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: start-solana-test / e2e
🔇 Additional comments (5)
zetaclient/chains/interfaces/interfaces.go (1)
147-151: LGTM!The new
GetAccountInfoWithOptsmethod is a clean addition to the interface that follows the established pattern and provides necessary flexibility for querying account information with specific options.changelog.md (1)
39-40: LGTM!The changelog updates are accurate - the typo fix and the new entry for PR #3940 properly document the changes.
zetaclient/testutils/mocks/solana_rpc.go (1)
1-1: LGTM!The auto-generated mock implementation for
GetAccountInfoWithOptscorrectly follows the mockery pattern and supports the new interface method.Also applies to: 50-78
zetaclient/chains/solana/signer/signer.go (2)
36-46: Well-defined constants with clear documentation.The timeout and commitment level choices are well-reasoned and properly documented. The 1-minute timeout provides adequate coverage for parallel CCTX processing, and the "confirmed" commitment level strikes a good balance between safety and transaction expiration risk.
259-259: Good use of the defined constant.Using
broadcastOutboundCommitmentinstead of a hardcoded value improves consistency and maintainability.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 259-259: zetaclient/chains/solana/signer/signer.go#L259
Added line #L259 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
zetaclient/chains/solana/signer/signer.go (1)
525-542: Consider adding a maximum sleep duration cap.While the proportional sleep based on nonce difference is clever, consider adding an explicit cap to prevent excessive sleep times:
// #nosec G115 always in range sleepDuration := time.Second * time.Duration(nonce-pda.Nonce) + // Cap the sleep duration to prevent excessive waiting + const maxSleepDuration = 10 * time.Second + if sleepDuration > maxSleepDuration { + sleepDuration = maxSleepDuration + } time.Sleep(sleepDuration)Also, this method needs comprehensive test coverage to verify all branches and edge cases.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 525-539: zetaclient/chains/solana/signer/signer.go#L525-L539
Added lines #L525 - L539 were not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
changelog.md(1 hunks)zetaclient/chains/interfaces/interfaces.go(1 hunks)zetaclient/chains/solana/signer/signer.go(6 hunks)zetaclient/testutils/mocks/solana_rpc.go(2 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/interfaces/interfaces.gozetaclient/testutils/mocks/solana_rpc.gozetaclient/chains/solana/signer/signer.go
🧬 Code Graph Analysis (1)
zetaclient/testutils/mocks/solana_rpc.go (1)
zetaclient/chains/interfaces/interfaces.go (1)
SolanaRPCClient(141-182)
🪛 GitHub Check: codecov/patch
zetaclient/chains/solana/signer/signer.go
[warning] 232-238: zetaclient/chains/solana/signer/signer.go#L232-L238
Added lines #L232 - L238 were not covered by tests
[warning] 259-259: zetaclient/chains/solana/signer/signer.go#L259
Added line #L259 was not covered by tests
[warning] 321-321: zetaclient/chains/solana/signer/signer.go#L321
Added line #L321 was not covered by tests
[warning] 342-342: zetaclient/chains/solana/signer/signer.go#L342
Added line #L342 was not covered by tests
[warning] 487-497: zetaclient/chains/solana/signer/signer.go#L487-L497
Added lines #L487 - L497 were not covered by tests
[warning] 500-504: zetaclient/chains/solana/signer/signer.go#L500-L504
Added lines #L500 - L504 were not covered by tests
[warning] 508-515: zetaclient/chains/solana/signer/signer.go#L508-L515
Added lines #L508 - L515 were not covered by tests
[warning] 519-523: zetaclient/chains/solana/signer/signer.go#L519-L523
Added lines #L519 - L523 were not covered by tests
[warning] 525-539: zetaclient/chains/solana/signer/signer.go#L525-L539
Added lines #L525 - L539 were not covered by tests
🔇 Additional comments (7)
zetaclient/chains/interfaces/interfaces.go (1)
147-151: LGTM!The addition of
GetAccountInfoWithOptsto the interface is clean and follows the established pattern. This method appropriately extends the functionality to support commitment level options.changelog.md (1)
39-40: LGTM!The changelog updates are accurate - fixed the typo in PR #3848 and added an appropriate entry for the current PR #3940.
zetaclient/testutils/mocks/solana_rpc.go (1)
1-1: LGTM!The mock implementation correctly reflects the interface changes. The method follows the standard mockery pattern and the version update is routine maintenance.
Also applies to: 50-78
zetaclient/chains/solana/signer/signer.go (4)
36-46: Well-documented constants with appropriate values.The 1-minute timeout is reasonable for the expected finality time, and the commitment level choice is well-justified with clear documentation explaining the tradeoffs.
259-259: Good refactoring to use constant.Replacing the hardcoded commitment level with the constant improves maintainability and consistency.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 259-259: zetaclient/chains/solana/signer/signer.go#L259
Added line #L259 was not covered by tests
321-321: Nice simplification using idiomatic Go.The range loop is cleaner when the index isn't needed.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 321-321: zetaclient/chains/solana/signer/signer.go#L321
Added line #L321 was not covered by tests
342-342: Consistent commitment level usage.Good to maintain consistency by using the same commitment level constant throughout the broadcast process.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 342-342: zetaclient/chains/solana/signer/signer.go#L342
Added line #L342 was not covered by tests
3 participants
Description
Replaces the original PRs:
#3633
#3708 simplified with same performance and no extra operational overhead.
The key points:
zetaclientwill NOT pre-sign Solana outbound and avoid starting the two minuterecentBlockhashtimer too early (ref).zetaclientwill use consistent commitment levelCommitmentConfirmedto handle outbound broadcasting to avoid state mismatches.pda_nonceto eliminate nonce mismatches.Performance Test Comparison
We see 13% reduction in total runtime and 25% reductions in average latency.
Before:
⬇
After:
⬇
With more performance tuning:
How Has This Been Tested?
Summary by CodeRabbit