refactor: delete all trackers when finalizing CCTX #2615
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 recent changes introduce enhancements primarily focused on the management and validation of outbound transaction trackers within the system. A new function, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LocalE2ETest
participant CctxClient
participant TrackerManager
User->>LocalE2ETest: Initiate Local E2E Test
LocalE2ETest->>CctxClient: Perform Network Report Validation
LocalE2ETest->>TrackerManager: checkTrackers()
TrackerManager-->>LocalE2ETest: Validate No Outbound Trackers
LocalE2ETest-->>User: Test Complete
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
kingpinXD
requested review from
brewmaster012,
lumtis,
ws4charlie,
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: 4
Outside diff range, codebase verification and nitpick comments (3)
x/crosschain/keeper/msg_server_vote_outbound_tx_test.go (3)
Line range hint
156-174:
Ensure comprehensive assertions inTestKeeper_VoteOutbound.The test function should include more assertions to verify the state of the
CrossChainTxobject after the vote.msg := types.MsgVoteOutbound{ CctxHash: cctx.Index, OutboundTssNonce: cctx.GetCurrentOutboundParam().TssNonce, OutboundChain: cctx.GetCurrentOutboundParam().ReceiverChainId, Status: chains.ReceiveStatus_success, Creator: observer, ObservedOutboundHash: sample.Hash().String(), ValueReceived: cctx.GetCurrentOutboundParam().Amount, ObservedOutboundBlockHeight: 10, ObservedOutboundEffectiveGasPrice: math.NewInt(21), ObservedOutboundGasUsed: 21, CoinType: cctx.InboundParams.CoinType, } _, err := msgServer.VoteOutbound(ctx, &msg) require.NoError(t, err) c, found := k.GetCrossChainTx(ctx, cctx.Index) require.True(t, found) require.Equal(t, types.CctxStatus_OutboundMined, c.CctxStatus.Status) require.Equal(t, msg.Digest(), c.GetCurrentOutboundParam().BallotIndex) // Additional assertions require.NotNil(t, c.GetCurrentOutboundParam()) require.Equal(t, observer, c.GetCurrentOutboundParam().Creator) require.Equal(t, 10, c.GetCurrentOutboundParam().ObservedOutboundBlockHeight)
Line range hint
214-235:
Ensure comprehensive assertions inTestKeeper_VoteOutboundfor failed votes.The test function should include more assertions to verify the state of the
CrossChainTxobject after the vote fails.msg := types.MsgVoteOutbound{ CctxHash: cctx.Index, OutboundTssNonce: cctx.GetCurrentOutboundParam().TssNonce, OutboundChain: cctx.GetCurrentOutboundParam().ReceiverChainId, Status: chains.ReceiveStatus_failed, Creator: observer, ObservedOutboundHash: sample.Hash().String(), ValueReceived: cctx.GetCurrentOutboundParam().Amount, ObservedOutboundBlockHeight: 10, ObservedOutboundEffectiveGasPrice: math.NewInt(21), ObservedOutboundGasUsed: 21, CoinType: cctx.InboundParams.CoinType, } _, err := msgServer.VoteOutbound(ctx, &msg) require.NoError(t, err) c, found := k.GetCrossChainTx(ctx, cctx.Index) require.True(t, found) require.Equal(t, types.CctxStatus_PendingRevert, c.CctxStatus.Status) require.Equal(t, oldParamsLen+1, len(c.OutboundParams)) require.Equal(t, types.TxFinalizationStatus_Executed, c.OutboundParams[oldParamsLen-1].TxFinalizationStatus) require.Equal(t, types.TxFinalizationStatus_NotFinalized, cctx.GetCurrentOutboundParam().TxFinalizationStatus) // Additional assertions require.NotNil(t, c.GetCurrentOutboundParam()) require.Equal(t, observer, c.GetCurrentOutboundParam().Creator) require.Equal(t, 10, c.GetCurrentOutboundParam().ObservedOutboundBlockHeight)
Line range hint
581-660:
Update function call to match the new signature for saving outbound.The
SaveOutboundfunction call should be updated to match the new simplified signature.- k.SaveOutbound(ctx, cctx) + k.SaveOutbound(ctx, cctx)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (8)
- changelog.md (1 hunks)
- cmd/zetae2e/local/local.go (1 hunks)
- docs/cli/zetacored/zetacored_tx_fungible_update-gateway-contract.md (1 hunks)
- x/crosschain/keeper/msg_server_vote_outbound_tx.go (4 hunks)
- x/crosschain/keeper/msg_server_vote_outbound_tx_test.go (11 hunks)
- x/crosschain/types/cctx.go (1 hunks)
- x/crosschain/types/cctx_test.go (1 hunks)
- x/observer/keeper/pending_nonces_test.go (2 hunks)
Files skipped from review due to trivial changes (2)
- changelog.md
- docs/cli/zetacored/zetacored_tx_fungible_update-gateway-contract.md
Additional context used
Path-based instructions (6)
x/observer/keeper/pending_nonces_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/types/cctx.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/keeper/msg_server_vote_outbound_tx.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/types/cctx_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.cmd/zetae2e/local/local.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/keeper/msg_server_vote_outbound_tx_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Additional comments not posted (9)
x/observer/keeper/pending_nonces_test.go (1)
141-189: LGTM!The new test case
test removal within range only using fixed valueis well-structured and covers various edge cases for nonce removal. It ensures that the integrity of theNonceLowandNonceHighvalues is maintained and that pending nonces for other chains are unaffected.x/crosschain/types/cctx.go (1)
167-169: LGTM!The new method
SetOutboundBallotIndexis a valuable addition to theCrossChainTxstruct. It enhances the control flow related to outbound parameters by providing a direct way to modify the ballot index.x/crosschain/keeper/msg_server_vote_outbound_tx.go (4)
93-94: LGTM!Setting the finalized ballot index in the
cctxenhances the tracking of the current outbound parameters. This change ensures that the outbound parameters are accurately tracked and updated.
176-179: LGTM!Removing the
ballotIndexparameter from theSaveFailedOutboundfunction simplifies the method signature and streamlines the process of saving outbound transactions.
185-186: LGTM!Removing the
ballotIndexparameter from theSaveSuccessfulOutboundfunction simplifies the method signature and streamlines the process of saving outbound transactions.
200-206: LGTM!Removing the
ballotIndexparameter and processing all outbound parameters in a loop improves the robustness of the outbound transaction handling. This change ensures that all relevant outbound parameters are processed consistently.cmd/zetae2e/local/local.go (1)
365-365: EnsurecheckTrackersis called in the appropriate context.The
checkTrackersfunction should be called in a context where it makes sense to validate the absence of outbound trackers. Ensure this is the correct place.x/crosschain/keeper/msg_server_vote_outbound_tx_test.go (2)
520-520: Update function call to match the new signature for successful outbound.The
SaveSuccessfulOutboundfunction call should be updated to match the new simplified signature.- k.SaveSuccessfulOutbound(ctx, cctx) + k.SaveSuccessfulOutbound(ctx, cctx)Likely invalid or redundant comment.
530-553: Update function call to match the new signature for multiple successful trackers.The
SaveSuccessfulOutboundfunction call should be updated to match the new simplified signature.- k.SaveSuccessfulOutbound(ctx, cctx) + k.SaveSuccessfulOutbound(ctx, cctx)Likely invalid or redundant comment.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2615 +/- ##
===========================================
- Coverage 70.35% 70.35% -0.01%
===========================================
Files 338 338
Lines 18594 18593 -1
===========================================
- Hits 13082 13081 -1
Misses 4920 4920
Partials 592 592
|
Earlier this ballot was set in the SaveOutbound. |
lumtis
changed the title
@lumtis could you give me an example of what you mean, Imo the title reflects the change |
I changed the name from |
okay that makes sense , thanks for the edit . |
Description
Closes #2610
How Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests