fix: make crosschain-call with invalid withdraw revert #3321
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 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces comprehensive updates across the ZetaChain ecosystem, focusing on cross-chain functionality, testing frameworks, and contract interactions. The changes span multiple components, including new telemetry endpoints, enhanced Bitcoin and Solana transaction handling, and the introduction of a new Changes
Sequence DiagramsequenceDiagram
participant User
participant Withdrawer
participant ZetaChain
participant SourceChain
User->>Withdrawer: Initiate Withdrawal
Withdrawer->>ZetaChain: onCrossChainCall
ZetaChain->>SourceChain: Validate Withdrawal
alt Withdrawal Valid
SourceChain-->>ZetaChain: Approve Transaction
ZetaChain-->>Withdrawer: Process Withdrawal
else Withdrawal Invalid
ZetaChain-->>Withdrawer: Abort Transaction
Withdrawer-->>User: Revert Notification
end
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
|
lumtis
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3321 +/- ##
===========================================
+ Coverage 61.71% 61.97% +0.25%
===========================================
Files 440 444 +4
Lines 31141 31400 +259
===========================================
+ Hits 19220 19459 +239
- Misses 11058 11073 +15
- Partials 863 868 +5
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
x/crosschain/keeper/evm_deposit.go (7)
103-105: Ensure consistent handling of temporary context.Creating a temporary context using CacheContext is appropriate for rollback purposes, but ensure that any logs or events written during partial failures are properly accounted for. Consider adding structured error logs for better observability in production.
107-107: Add logging for deposit call details.When calling ZRC20DepositAndCallContract, log relevant parameters (e.g., 'to', 'message', etc.) within this block. Detailed logs facilitate debugging in cross-chain flows.
123-123: Consider error classification.When returning err here, ensure it is classified differently from revert-inducing errors. This helps with better error propagation and improved instrumentation for cross-chain debugging.
132-132: Clarify the meaning of InCCTXIndexKey.Make sure this key is well-documented. If it is widely used across modules, consider extracting it as a constant in a shared package and providing descriptive commentary.
138-139: Validate logs prior to processing.Before calling ProcessLogs, consider verifying the structure and type of each log to ensure robust error handling if logs have unexpected formats.
141-144: Refine revert handling.While skipping commit() ensures no partial state is persisted, you might wish to emit an event or log the reason for the revert for better traceability and to facilitate postmortem analyses.
146-146: Broaden event metadata.The event includes minimal attributes. Consider adding inbound/outbound chain details, block height, or user references for improved on-chain analytics.
e2e/contracts/withdrawer/Withdrawer.sol (1)
34-35: Avoid repeating approval logic.Because both onCrossChainCall and onCall repeat the same approval logic, consider refactoring into a private helper method if you foresee more logic being added in the future.
e2e/e2etests/test_bitcoin_deposit_and_withdraw_with_dust.go (2)
17-17: Ensure robust coverage with additional test scenarios.Testing a dust amount is beneficial, but consider adding scenarios with random dust-like values or boundary conditions (e.g., exactly 1000 satoshis) to ensure consistent handling.
50-51: Revisit error message matching.Matching the exact string from crosschaintypes.ErrCannotProcessWithdrawal could become brittle if the error message changes. Consider matching on a stable error code or type instead.
changelog.md (1)
32-32: Consider enhancing the changelog entry description.The current entry could be more descriptive to better reflect the changes implemented in PR #3321. Consider expanding it to:
-* [3321](https://github.com/zeta-chain/node/pull/3321) - make crosschain-call with invalid withdraw revert +* [3321](https://github.com/zeta-chain/node/pull/3321) - fix crosschain-call behavior to properly revert when processing invalid withdrawals (e.g., Bitcoin dust amounts)This provides more context about the specific scenario being addressed and the intended behavior.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
e2e/contracts/withdrawer/Withdrawer.binis excluded by!**/*.bin
📒 Files selected for processing (13)
changelog.md(1 hunks)cmd/zetae2e/local/local.go(1 hunks)e2e/contracts/withdrawer/Withdrawer.abi(1 hunks)e2e/contracts/withdrawer/Withdrawer.go(1 hunks)e2e/contracts/withdrawer/Withdrawer.json(1 hunks)e2e/contracts/withdrawer/Withdrawer.sol(1 hunks)e2e/contracts/withdrawer/bindings.go(1 hunks)e2e/e2etests/e2etests.go(2 hunks)e2e/e2etests/test_bitcoin_deposit_and_withdraw_with_dust.go(1 hunks)x/crosschain/keeper/evm_deposit.go(4 hunks)x/crosschain/keeper/evm_deposit_test.go(11 hunks)x/crosschain/keeper/evm_hooks.go(2 hunks)x/crosschain/keeper/evm_hooks_test.go(6 hunks)
✅ Files skipped from review due to trivial changes (2)
- e2e/contracts/withdrawer/bindings.go
- e2e/contracts/withdrawer/Withdrawer.go
👮 Files not reviewed due to content moderation or server errors (2)
- x/crosschain/keeper/evm_deposit_test.go
- x/crosschain/keeper/evm_hooks_test.go
🧰 Additional context used
📓 Path-based instructions (7)
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/evm_deposit_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_bitcoin_deposit_and_withdraw_with_dust.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/keeper/evm_deposit.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/keeper/evm_hooks_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/e2etests.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/keeper/evm_hooks.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (11)
x/crosschain/keeper/evm_deposit.go (3)
119-120: Validate necessity of committing on revert.
Currently, commit() is invoked during contract reverts to save logs. Confirm that partial and potentially invalid states aren't mistakenly being recorded in the keeper store. Consider storing only relevant event logs without finalizing other potentially problematic state changes.
158-159: Review commit logic ordering.
If multiple calls to the keeper occur before commit(), ensure that all temporary changes remain consistent. Test with concurrency or repeated calls to detect possible race conditions.
161-161: Validate final error handling path.
After the function returns, confirm that any leftover or uncommitted states do not cause partial side effects or leak into subsequent calls or transactions.
e2e/e2etests/test_bitcoin_deposit_and_withdraw_with_dust.go (2)
26-27: Confirm dust threshold across different chain configurations.
Different chain setups may apply different dust thresholds. Validate that 100 satoshis is relevant for all environments. Provide clarifying comments or constants for improved maintainability.
53-57: Add checks for partial withdrawals.
Currently, you verify zero BTC balance. Consider also ensuring no partial balances remain if a withdrawal partially succeeded. This helps confirm the revert logic is fully effective.
e2e/contracts/withdrawer/Withdrawer.abi (1)
1-116: Confirm ABI alignment with contract code.
The ABI definitions match the constructor and two functions in Withdrawer.sol. Ensure any future changes to the contract are mirrored here, or consider an automated ABI generation approach to avoid manual drift.
e2e/contracts/withdrawer/Withdrawer.json (1)
1-119: LGTM! Well-structured ABI definition.
The ABI definition follows best practices with:
- Clear function signatures and parameter types
- Appropriate state mutability modifiers
- Well-structured Context type definition
- Consistent naming conventions
x/crosschain/keeper/evm_hooks.go (2)
136-136: LGTM! Consistent naming convention.
The function call has been updated to use the consistent ZRC20 naming convention.
306-308: LGTM! Function name follows naming convention.
The function name has been updated to maintain consistency with the ZRC20 naming pattern across the codebase.
cmd/zetae2e/local/local.go (1)
306-306: LGTM! Enhanced test coverage for Bitcoin dust transactions.
The addition of TestBitcoinDepositAndWithdrawWithDustName to advanced Bitcoin deposit tests appropriately covers the edge case of handling dust amounts in Bitcoin transactions.
e2e/e2etests/e2etests.go (1)
590-591: LGTM: Well-structured test additions.
The new test definitions and description updates maintain consistency with the existing test structure and naming conventions.
Also applies to: 598-599, 602-607
Co-authored-by: skosito <[email protected]>
Co-authored-by: skosito <[email protected]>
| @@ -100,8 +100,11 @@ func (k Keeper) HandleEVMDeposit(ctx sdk.Context, cctx *types.CrossChainTx) (boo | |||
| return false, fmt.Errorf("HandleEVMDeposit: unable to decode address: %w", err) | |||
| } | |||
|
|
|||
| // use a temporary context to not commit any state change in case of error | |||
| tmpCtx, commit := ctx.CacheContext() | |||
|
|
|||
There was a problem hiding this comment.
| ZRC20DepositAndCallContract is solely responsible for calling the contract and depositing tokens if needed and does not include any other side effects or any logic that modifies the state directly |
Consider adding this comment or something similar for code readability, as the commit / discard of state is a bit complicated to follow when reading the code .
Description
Solves #3308
If a crosschain call initiate an invalid withdraw (example dust amount for Bitcoin withdraw) makes the CCXT reverts
Solution
In
HandleEVMCallwhere crosschain calls are handled, whenProcessLogslogs fails, we consider that the crosschain call reverts because the withdraw initiated in this call can't be processed, we therefore returnscontractReverted == trueso it enters the revert workflow.Since in this case the deposit contract call was successful and there is an effective state transition in the EVM, we now use
tmpCtx, we only commit it if the contract has a regular revert or the call is sucessful andProcessLogsdoesn't fail.The E2E test checks that the BTC balance remains 0 after the calls, this means the EVM state transition has not been processed.
Summary by CodeRabbit
Release Notes
New Features
/systemtimein the application.Bug Fixes
Documentation
Tests