fix: allow BTC revert with dust amount #3142
Conversation
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📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces enhancements to the Bitcoin deposit functionality by adding a new test case that addresses scenarios involving dust outputs. It includes the implementation of a new constant for the test and a corresponding test function to validate the behavior of Bitcoin deposits that may revert due to insufficient amounts. Additionally, modifications are made to the Changes
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 #3142 +/- ##
===========================================
- Coverage 63.19% 63.17% -0.02%
===========================================
Files 423 423
Lines 29887 29893 +6
===========================================
Hits 18886 18886
- Misses 10163 10169 +6
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: 3
🧹 Outside diff range and nitpick comments (9)
e2e/e2etests/test_bitcoin_deposit_and_call_revert_with_dust.go (3)
14-21: Enhance test documentation with prerequisites and expected behavior.Consider adding a detailed doc comment that includes:
- Prerequisites for running the test
- Expected behavior and success criteria
- Explanation of the dust amount threshold
Example improvement:
-// TestBitcoinDepositAndCallRevertWithDust sends a Bitcoin deposit that reverts with a dust amount in the revert outbound. +// TestBitcoinDepositAndCallRevertWithDust tests the handling of Bitcoin deposits that result in dust amounts during revert. +// +// Prerequisites: +// - Live Bitcoin network or local Bitcoin network with mining capability +// - Sufficient BTC balance in deployer account +// +// Expected behavior: +// - The deposit transaction should be processed +// - The revert should be triggered due to invalid address +// - The outbound amount should be less than dust threshold (1000 satoshis) +// - The CCTX status should be marked as Reverted
29-33: Enhance UTXO error handling with specific messages.The error handling could be more descriptive to aid in debugging test failures.
Consider this improvement:
utxos, err := r.ListDeployerUTXOs() -require.NoError(r, err) -require.NotEmpty(r, utxos) +require.NoError(r, err, "failed to list deployer UTXOs") +require.NotEmpty(r, utxos, "no UTXOs found for deployer address")
34-42: Improve test data generation and transaction verification.The test execution could benefit from more explicit data generation and thorough verification.
Consider these improvements:
-nonExistReceiver := sample.EthAddress() -badMemo := append(nonExistReceiver.Bytes(), []byte("gibberish-memo")...) +// Generate an invalid receiver address and memo that will trigger a revert +nonExistReceiver := sample.EthAddress() +invalidMemo := append(nonExistReceiver.Bytes(), []byte("invalid-memo-to-trigger-revert")...) txHash, err := r.SendToTSSFromDeployerWithMemo(amount, utxos, badMemo) require.NoError(r, err) -require.NotEmpty(r, txHash) +require.NotEmpty(r, txHash, "expected non-empty transaction hash") +require.NotEqual(r, "0x0000000000000000000000000000000000000000000000000000000000000000", + txHash.String(), "expected valid transaction hash")zetaclient/chains/bitcoin/signer/signer.go (3)
423-427: Extract dust amount check into a helper functionConsider extracting the dust amount check into a helper function for better reusability and testability.
+func isDustAmount(amount *big.Int) bool { + return amount.Uint64() < constant.BTCWithdrawalDustAmount +} // check dust amount -dustAmount := params.Amount.Uint64() < constant.BTCWithdrawalDustAmount +dustAmount := isDustAmount(params.Amount)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 424-427: zetaclient/chains/bitcoin/signer/signer.go#L424-L427
Added lines #L424 - L427 were not covered by tests
426-426: Use constant in logging statementReplace the magic number with the constant value in the logging statement for consistency.
-logger.Warn().Msgf("dust amount %d sats, canceling tx", params.Amount.Uint64()) +logger.Warn().Msgf("dust amount %d sats (below %d), canceling tx", params.Amount.Uint64(), constant.BTCWithdrawalDustAmount)
429-435: Consider enhancing error handlingThe current implementation silently sets the amount to 0 when canceling the transaction. Consider:
- Adding more detailed logging about why the transaction was canceled
- Potentially returning early to avoid unnecessary processing
// set the amount to 0 when the tx should be cancelled cancelTx := restrictedCCTX || dustAmount if cancelTx { + reason := []string{} + if restrictedCCTX { + reason = append(reason, "restricted address") + } + if dustAmount { + reason = append(reason, "dust amount") + } + logger.Info().Msgf("Canceling transaction due to: %s", strings.Join(reason, ", ")) amount = 0.0 } else { logger.Info().Msgf("SignGasWithdraw: to %s, value %d sats", to.EncodeAddress(), params.Amount.Uint64()) }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 430-435: zetaclient/chains/bitcoin/signer/signer.go#L430-L435
Added lines #L430 - L435 were not covered by testscmd/zetae2e/local/local.go (1)
Line range hint
102-102: Consider implementing the TODO for better maintainability.The TODO comment correctly identifies the need to simplify this file. Consider breaking down the test categories into separate files:
bitcoin_tests.goerc20_tests.goethereum_tests.go
etc.This would improve maintainability and make it easier to add new test cases in the future.
zetaclient/chains/bitcoin/observer/outbound.go (1)
535-536: Implementation looks good, but consider improving error messages.The logic correctly handles dust amounts similar to restricted transactions. However, consider making the error message more specific when the transaction is cancelled due to dust amount versus being restricted.
if compliance.IsCctxRestricted(cctx) || params.Amount.Uint64() < constant.BTCWithdrawalDustAmount { err = ob.checkTSSVoutCancelled(params, rawResult.Vout) if err != nil { + reason := "restricted" + if params.Amount.Uint64() < constant.BTCWithdrawalDustAmount { + reason = "dust amount" + } return errors.Wrapf( err, - "checkTssOutboundResult: invalid TSS Vout in cancelled outbound %s nonce %d", + "checkTssOutboundResult: invalid TSS Vout in cancelled (%s) outbound %s nonce %d", + reason, hash, nonce, ) }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 536-536: zetaclient/chains/bitcoin/observer/outbound.go#L536
Added line #L536 was not covered by testse2e/e2etests/e2etests.go (1)
559-563: Consider adding amount parameter and enhancing description.While the test registration follows the established pattern, consider these improvements:
- Add an
ArgDefinitionfor the dust amount to make the test configurable, similar to other Bitcoin tests.- Enhance the description to specify the dust amount threshold.
Apply this diff:
runner.NewE2ETest( TestBitcoinDepositAndCallRevertWithDustName, - "deposit Bitcoin into ZEVM; revert with dust amount that aborts the CCTX", []runner.ArgDefinition{}, + "deposit Bitcoin into ZEVM; revert when amount is below dust threshold (546 satoshis)", + []runner.ArgDefinition{ + {Description: "amount in btc", DefaultValue: "0.00000546"}, + }, TestBitcoinDepositAndCallRevertWithDust, ),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
cmd/zetae2e/local/local.go(1 hunks)e2e/e2etests/e2etests.go(2 hunks)e2e/e2etests/test_bitcoin_deposit_and_call_revert_with_dust.go(1 hunks)zetaclient/chains/bitcoin/observer/outbound.go(2 hunks)zetaclient/chains/bitcoin/signer/signer.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
cmd/zetae2e/local/local.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.
e2e/e2etests/test_bitcoin_deposit_and_call_revert_with_dust.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/outbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/signer/signer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
📓 Learnings (1)
zetaclient/chains/bitcoin/observer/outbound.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#2899
File: zetaclient/chains/bitcoin/observer/inbound.go:131-132
Timestamp: 2024-11-10T18:26:41.816Z
Learning: ObserveInbound coverage will be improved in future refactor.
🪛 GitHub Check: codecov/patch
zetaclient/chains/bitcoin/observer/outbound.go
[warning] 536-536: zetaclient/chains/bitcoin/observer/outbound.go#L536
Added line #L536 was not covered by tests
zetaclient/chains/bitcoin/signer/signer.go
[warning] 417-418: zetaclient/chains/bitcoin/signer/signer.go#L417-L418
Added lines #L417 - L418 were not covered by tests
[warning] 424-427: zetaclient/chains/bitcoin/signer/signer.go#L424-L427
Added lines #L424 - L427 were not covered by tests
[warning] 430-435: zetaclient/chains/bitcoin/signer/signer.go#L430-L435
Added lines #L430 - L435 were not covered by tests
🔇 Additional comments (5)
zetaclient/chains/bitcoin/signer/signer.go (1)
24-24: LGTM!
The import statement is correctly placed and properly utilized for accessing the dust amount constant.
cmd/zetae2e/local/local.go (1)
322-322: LGTM! Verify test implementation.
The addition of TestBitcoinDepositAndCallRevertWithDustName to the Bitcoin deposit tests is consistent with the PR objectives and follows the established naming convention.
Let's verify the test implementation exists:
✅ Verification successful
Test implementation verified and correctly handles dust amount scenario
The test implementation in e2e/e2etests/test_bitcoin_deposit_and_call_revert_with_dust.go is properly implemented and aligns with the PR objectives:
- Tests BTC deposit with dust amount (0.00200100 BTC + deposit fee)
- Verifies revert behavior when amount is less than dust threshold
- Includes appropriate assertions for CCTX status and outbound amount validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the test implementation exists in the codebase
# Expected: Find the implementation of TestBitcoinDepositAndCallRevertWithDust
# Search for the test implementation
rg -l "TestBitcoinDepositAndCallRevertWithDust"
# Get the implementation details with context
ast-grep --pattern 'func TestBitcoinDepositAndCallRevertWithDust($$$) {
$$$
}'
Length of output: 4074
zetaclient/chains/bitcoin/observer/outbound.go (2)
16-16: LGTM: Import addition is correct.
The added import for the constant package is necessary for accessing BTCWithdrawalDustAmount.
535-536: Add test coverage for dust amount handling.
The new condition for handling dust amounts needs test coverage to ensure the functionality works as expected.
Would you like me to help create test cases for:
- Transaction with amount below dust threshold
- Transaction with amount equal to dust threshold
- Transaction with amount above dust threshold
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 536-536: zetaclient/chains/bitcoin/observer/outbound.go#L536
Added line #L536 was not covered by tests
e2e/e2etests/e2etests.go (1)
83-83: LGTM: Constant follows established naming pattern.
The constant name and value are consistent with other Bitcoin test constants and clearly describe the test's purpose.
Description
Merge #3140 on upstream
We should address the fix at ZetaCore level in the end to make CCTX aborted instead of reverted (will be cleaner) -> #3135
But still add this one to have an additional guardian on ZetaClient, the issue above will also require an adidtional E2E test for dust amount on Solana
Summary by CodeRabbit
New Features
Bug Fixes
Documentation