fix: prevent cctx being set as abortRefunded if the abort processing failed before the refund#3901
fix: prevent cctx being set as abortRefunded if the abort processing failed before the refund#3901
abortRefunded if the abort processing failed before the refund#3901Conversation
📝 WalkthroughWalkthroughThis update refines the abort refund logic in cross-chain transaction handling. It introduces nuanced error handling to distinguish between generic abort failures and specific onAbort handler failures, ensuring the refund status accurately reflects the underlying outcome. Associated tests and error definitions are updated to validate and support this improved behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CrossChainKeeper
participant FungibleKeeper
participant EVMContract
User->>CrossChainKeeper: Initiate abort (ProcessAbort)
CrossChainKeeper->>FungibleKeeper: ProcessAbort
FungibleKeeper->>EVMContract: Call onAbort
alt onAbort succeeds
FungibleKeeper-->>CrossChainKeeper: Success, no error
CrossChainKeeper-->>User: Mark abort as refunded (IsAbortRefunded = true)
else onAbort fails with ErrOnAbortFailed
FungibleKeeper-->>CrossChainKeeper: Error (ErrOnAbortFailed)
CrossChainKeeper-->>User: Mark abort as refunded (IsAbortRefunded = true)
else onAbort fails with other error
FungibleKeeper-->>CrossChainKeeper: Generic error
CrossChainKeeper-->>User: Do not mark abort as refunded (IsAbortRefunded = false)
end
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure ✨ Finishing Touches
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3901 +/- ##
===========================================
+ Coverage 64.69% 64.85% +0.15%
===========================================
Files 469 469
Lines 34352 34357 +5
===========================================
+ Hits 22224 22281 +57
+ Misses 11100 11044 -56
- Partials 1028 1032 +4
🚀 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)
x/fungible/keeper/deposits_test.go (1)
793-822: Consider improving test name for clarityWhile the test correctly verifies that ZETA token aborts are rejected with an error different from ErrOnAbortFailed, the test name could be more explicit about testing the error type distinction.
- t.Run("can't process abort for ZETA token", func(t *testing.T) { + t.Run("should reject ZETA token aborts with error distinct from ErrOnAbortFailed", func(t *testing.T) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
changelog.md(1 hunks)x/crosschain/keeper/abort.go(1 hunks)x/crosschain/keeper/abort_test.go(1 hunks)x/fungible/keeper/deposits.go(4 hunks)x/fungible/keeper/deposits_test.go(3 hunks)x/fungible/types/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.
x/fungible/types/evm.gox/crosschain/keeper/abort.gox/fungible/keeper/deposits.gox/fungible/keeper/deposits_test.gox/crosschain/keeper/abort_test.go
🧬 Code Graph Analysis (3)
x/crosschain/keeper/abort.go (1)
x/fungible/types/evm.go (1)
ErrOnAbortFailed(14-14)
x/fungible/keeper/deposits_test.go (5)
e2e/contracts/testabort/TestAbort.go (1)
TestAbortMetaData(43-46)x/crosschain/keeper/abort_test.go (1)
TestKeeper_ProcessAbort(24-189)pkg/chains/chains.go (1)
DefaultChainsList(511-551)testutil/sample/crypto.go (1)
EthAddress(89-91)x/fungible/types/evm.go (1)
ErrOnAbortFailed(14-14)
x/crosschain/keeper/abort_test.go (5)
testutil/keeper/crosschain.go (3)
CrosschainKeeperWithMocks(95-240)CrosschainMockOptions(38-47)GetCrosschainFungibleMock(291-295)testutil/sample/crosschain.go (1)
CrossChainTx(266-280)testutil/sample/crypto.go (1)
EthAddress(89-91)x/crosschain/types/status.go (1)
StatusMessages(8-13)x/fungible/types/evm.go (1)
ErrOnAbortFailed(14-14)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e-ok
🔇 Additional comments (13)
x/fungible/types/evm.go (1)
11-15: Appropriate error sentinel introduction for improved error handling.The new
ErrOnAbortFailedsentinel error effectively distinguishes between generic abort failures and the specific case where funds are refunded but theonAborthandler fails.x/crosschain/keeper/abort.go (1)
97-100: Improved conditional setting ofIsAbortRefundedflag.This change ensures the refunded status accurately reflects reality - only marking a transaction as refunded when either:
- The abort process completes successfully (err == nil), or
- The funds were refunded but the onAbort handler failed (ErrOnAbortFailed)
This prevents incorrect status reporting when actual refund processing fails.
x/fungible/keeper/deposits.go (3)
3-11: Proper error package imports organization.The addition of the standard "errors" package alongside renaming the existing errors import to "errorspkg" resolves potential naming conflicts while maintaining backward compatibility.
225-240: Enhanced error handling for abort processing.The modified error handling properly wraps errors from
CallExecuteAbortwith the sentinel errorErrOnAbortFailed. This creates a clear distinction between generic failures and the specific case where funds are refunded but the contract'sonAborthandler fails.This change works in conjunction with the updated logic in the crosschain module to ensure accurate tracking of refund status.
269-273: Consistent error handling with renamed package.The error wrapping was updated to use the renamed
errorspkgpackage alias, maintaining consistent error handling patterns.x/crosschain/keeper/abort_test.go (2)
118-152: Well-structured test for handling non-onAbort errorsThis test correctly verifies that when ProcessAbort fails with an error other than ErrOnAbortFailed, the CCTX status remains Aborted but IsAbortRefunded is not set to true. This distinction is crucial for the PR objective of preventing incorrect abortRefunded status.
154-188: Excellent test for onAbort failure handlingThis test properly validates that when ProcessAbort fails with ErrOnAbortFailed (joined with another error), the CCTX is still marked as abortRefunded despite the error. The use of errors.Join is appropriate for testing error wrapping behavior.
x/fungible/keeper/deposits_test.go (5)
4-4: Appropriate import for test abort contractThe import for the testabort contract is correctly added to support the new abort tests.
72-80: Well-implemented helper function for abort testsThis helper function follows the established pattern for contract deployment helpers in the codebase, making it easy to use in the new abort tests.
721-754: Good test for successful abort processingThis test correctly verifies that ProcessAbort works as expected when the onAbort call succeeds. It appropriately checks that the balance is updated after a successful abort operation.
756-791: Effective test for onAbort failure caseThis test properly validates that when onAbort fails (due to an invalid contract address), the error returned is ErrOnAbortFailed. It also correctly verifies that the balance is still updated despite the onAbort function failure, which is an important part of the behavior being fixed in this PR.
824-851: Complete test for invalid chain ID caseThis test correctly verifies that an abort for an invalid chain ID is rejected with an error that is not ErrOnAbortFailed, which helps validate the error handling patterns implemented in the PR.
changelog.md (1)
51-51: Changelog entry looks accurate and well-formatted.The new fix entry correctly documents PR #3901 under the "Fixes" section, matches the PR summary, and follows existing formatting conventions.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
3 participants
Description
Fixes https://github.com/zeta-chain/protocol-private/issues/244
Add also some tests that were missing for processAbort
Summary by CodeRabbit
Bug Fixes
Tests
Documentation