fix(crosschain): do not check zetachain sender for failed revert outbound #3166
Conversation
lumtis
added
no-changelog
lumtis
requested review from
brewmaster012,
kingpinXD,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
|
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📝 WalkthroughWalkthroughThe changes in this pull request primarily involve the Changes
Assessment against linked issues
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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3166 +/- ##
========================================
Coverage 62.84% 62.85%
========================================
Files 424 424
Lines 30140 30138 -2
========================================
Hits 18942 18942
+ Misses 10360 10358 -2
Partials 838 838
|
![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
🧹 Outside diff range and nitpick comments (1)
x/crosschain/keeper/cctx_orchestrator_validate_outbound.go (1)
332-341: Improve error handling consistencyThe error handling in this block uses different styles (
errors.Wrapvsfmt.Errorf). Consider standardizing the error handling approach.Consider this improvement:
- if err != nil { - return errors.Wrap(err, "failed to get ZetaChain chainID") - } - if cctx.InboundParams.SenderChainId != zetaChain.ChainId { - return fmt.Errorf( - "sender chain for withdraw cctx is not ZetaChain expected %d got %d", - zetaChain.ChainId, - cctx.InboundParams.SenderChainId, - ) - } + if err != nil { + return errors.Wrap(err, "failed to get ZetaChain chainID") + } + if cctx.InboundParams.SenderChainId != zetaChain.ChainId { + return errors.Errorf( + "sender chain for withdraw cctx is not ZetaChain (expected: %d, got: %d)", + zetaChain.ChainId, + cctx.InboundParams.SenderChainId, + ) + }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 330-341: x/crosschain/keeper/cctx_orchestrator_validate_outbound.go#L330-L341
Added lines #L330 - L341 were not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
x/crosschain/keeper/cctx_orchestrator_validate_outbound.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
x/crosschain/keeper/cctx_orchestrator_validate_outbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🪛 GitHub Check: codecov/patch
x/crosschain/keeper/cctx_orchestrator_validate_outbound.go
[warning] 330-341: x/crosschain/keeper/cctx_orchestrator_validate_outbound.go#L330-L341
Added lines #L330 - L341 were not covered by tests
🔇 Additional comments (1)
x/crosschain/keeper/cctx_orchestrator_validate_outbound.go (1)
330-341:
Verify alignment with PR objectives regarding ZetaChain sender check
The PR objectives state that we should not check the ZetaChain sender for failed revert outbound transactions. However, this code block implements a ZetaChain sender check. If this check is specifically for withdraw transactions (as suggested by the error message), this should be clearly documented in the code comments.
Let's verify the intended behavior:
Consider:
- Adding a comment explaining why this check is necessary for withdraw transactions
- Adding test coverage for this validation logic
- Clarifying if this check should be skipped for revert transactions as per PR objectives
Would you like assistance in generating the test cases?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 330-341: x/crosschain/keeper/cctx_orchestrator_validate_outbound.go#L330-L341
Added lines #L330 - L341 were not covered by tests
Description
If status is
CctxStatus_PendingRevertwhen processing a failed outbound then the sender chain will not be ZetaChain but the chain that initiated the inboundCloses: #3165
Summary by CodeRabbit
Bug Fixes
Refactor