fix: allow evm chain stuck outbound replacement by using latest nonce#3956
fix: allow evm chain stuck outbound replacement by using latest nonce#3956ws4charlie merged 4 commits intodevelopfrom
Conversation
…o perform pre-broadcasting check
|
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 update the logic for retrieving the transaction nonce in EVM signing operations from using the pending nonce to the latest finalized nonce. This adjustment affects the broadcast method, associated tests, and test utilities. The changelog documents this fix, clarifying its intent to enable transaction replacement. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Signer
participant EVMNode
Client->>Signer: Request to broadcast transaction
Signer->>EVMNode: Query latest finalized nonce (NonceAt)
EVMNode-->>Signer: Return latest nonce
Signer->>Signer: Compare transaction nonce with latest nonce
alt If transaction nonce < latest nonce
Signer->>Client: Skip broadcast (already processed)
else If transaction nonce >= latest nonce
Signer->>EVMNode: Broadcast transaction
EVMNode-->>Signer: Broadcast result
Signer->>Client: Return result
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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3956 +/- ##
========================================
Coverage 64.11% 64.11%
========================================
Files 474 474
Lines 34842 34842
========================================
Hits 22340 22340
Misses 11477 11477
Partials 1025 1025
🚀 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 (1)
zetaclient/chains/evm/signer/signer.go (1)
495-498: Address test coverage gap for new logging.The logic and variable renaming are correct, but the new log statement on line 497 lacks test coverage as indicated by static analysis.
Consider adding a test case that covers the scenario where
latestNonce > nonceto ensure the new log message is tested:func TestBroadcastOutbound_SkipLowNonce(t *testing.T) { // Setup test with latestNonce > cctx.nonce // Verify log message is generated correctly }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 497-497: zetaclient/chains/evm/signer/signer.go#L497
Added line #L497 was 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/evm/signer/signer.go(1 hunks)zetaclient/chains/evm/signer/signer_test.go(2 hunks)zetaclient/testutils/testrpc/rpc_evm.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.
zetaclient/testutils/testrpc/rpc_evm.gozetaclient/chains/evm/signer/signer_test.gozetaclient/chains/evm/signer/signer.go
🪛 GitHub Check: codecov/patch
zetaclient/chains/evm/signer/signer.go
[warning] 497-497: zetaclient/chains/evm/signer/signer.go#L497
Added line #L497 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: start-e2e-test / e2e
🔇 Additional comments (5)
zetaclient/testutils/testrpc/rpc_evm.go (1)
39-39: LGTM: Test utility properly updated to reflect nonce method change.The method rename from
MockPendingNonceAttoMockNonceAtcorrectly aligns with the main signer implementation changes while preserving the mock functionality.zetaclient/chains/evm/signer/signer_test.go (2)
152-154: LGTM: Test comments and mock calls correctly updated.The changes properly reflect the transition from pending nonce to finalized nonce retrieval, maintaining test consistency with the main implementation.
178-179: LGTM: Mock setup properly updated for nonce retrieval change.The test correctly uses
MockNonceAtto align with the updated nonce retrieval logic in the main signer implementation.changelog.md (1)
12-12: LGTM: Changelog properly documents the EVM transaction replacement fix.The entry clearly explains the purpose of using the latest nonce for pre-broadcast checks to enable transaction replacement capability.
zetaclient/chains/evm/signer/signer.go (1)
487-491: LGTM: Core nonce retrieval logic correctly updated.The change from
PendingNonceAttoNonceAtwith nil block parameter properly implements the finalized nonce strategy, enabling replacement of stuck pending transactions.
lumtis
changed the title
…ow-evm-outbound-tx-replacement
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
3 participants
Description
This PR is to backport the
v31fix: #3955How Has This Been Tested?
Summary by CodeRabbit
Bug Fixes
Documentation