feat: register aborted CCTX for Bitcoin inbound that carries insufficient depositor fee #3358
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 📝 WalkthroughWalkthroughThis pull request introduces a comprehensive enhancement to the Bitcoin deposit process, focusing on improving error handling and user awareness when deposits fail due to insufficient depositor fees. The changes span multiple components of the system, including client-side observers, core transaction types, and end-to-end testing infrastructure, with the primary goal of providing more transparent feedback about deposit failures. Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested reviewers
Detailed AnalysisThe pull request implements a robust solution for handling Bitcoin deposits with insufficient fees by:
The implementation provides a clean, extensible approach to improving transaction transparency and user experience when dealing with Bitcoin deposits. 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
|
ws4charlie
added
zetacore
…t-create-aborted-CCTX-on-insufficient-deposit-fee
ws4charlie
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (1)
e2e/e2etests/test_stress_btc_deposit.go (1)
Line range hint
105-105: Restore error handling for ListDeployerUTXOs.Removing error handling for
ListDeployerUTXOscould mask potential issues during testing. Consider restoring the error checks to maintain test robustness.-utxos := r.ListDeployerUTXOs() +utxos, err := r.ListDeployerUTXOs() +require.NoError(r, err)Also applies to: 145-145
🧹 Nitpick comments (11)
e2e/e2etests/test_bitcoin_deposit_abort_with_low_deposit_fee.go (1)
14-35: LGTM! Consider enhancing test coverage with edge cases.The test effectively validates the core functionality of aborting transactions with insufficient fees. Consider adding test cases for:
- Amount exactly equal to the depositor fee
- Multiple consecutive low-fee deposits
e2e/e2etests/test_bitcoin_deposit_and_call_revert.go (1)
30-32: Document UTXO selection strategy.The test uses only the first UTXO without explaining why. Consider:
- Adding a comment explaining the UTXO selection strategy
- Testing with multiple UTXOs to ensure broader coverage
e2e/e2etests/test_bitcoin_std_deposit.go (1)
43-43: Consider introducing a test helper for deposit operations.The boolean parameter is consistently used across multiple test files. Consider introducing a test helper function that encapsulates this common pattern and documents the purpose of the validation flag.
+// helper.go +const ( + // ValidateDeposit controls whether the deposit operation should perform + // additional validation checks (add detailed explanation) + ValidateDeposit = true +) + +// DepositBTCWithStandardMemo wraps DepositBTCWithAmount with standard validation +func (r *E2ERunner) DepositBTCWithStandardMemo(amount *big.Float, memo *memo.InboundMemo) string { + return r.DepositBTCWithAmount(amount, memo, ValidateDeposit) +} // In test file -txHash := r.DepositBTCWithAmount(amount, memo, true) +txHash := r.DepositBTCWithStandardMemo(amount, memo)x/crosschain/client/cli/tx_vote_inbound.go (1)
92-92: Add validation and documentation for the error message parameter.The empty string parameter appears to be a placeholder for error messages. Consider:
- Adding validation for the error message length
- Documenting the purpose and usage of this parameter in the command help text
- "", + // TODO: Add error message parameter to command args and validate + "", // Error message placeholder for future usex/crosschain/types/inbound_parsing.go (1)
146-146: Consider using a constant for the default error message.The empty string
""is repeated across multiple functions. Consider introducing a constant for better maintainability:+const DefaultErrorMessage = "" - "", + DefaultErrorMessage,Also applies to: 195-195, 245-245
zetaclient/chains/bitcoin/observer/inbound.go (1)
393-393: Consider adding documentation for the ErrMessage field.While the implementation is correct, adding a comment explaining the purpose and usage of the ErrMessage field would improve code maintainability.
BTCInboundEvent{ + // ErrMessage stores any error that occurred during inbound processing + // Currently used to track insufficient depositor fee errors ErrMessage: errMessage, }e2e/e2etests/test_stress_btc_deposit.go (1)
31-31: Document the purpose of the boolean parameter.The third parameter
trueinDepositBTCWithAmountlacks context. Consider adding a named parameter or documenting its purpose.zetaclient/chains/bitcoin/observer/witness.go (1)
180-185: Consider additional validation in DeductDepositorFee.While the function correctly handles the basic case, consider adding validation for negative values.
func DeductDepositorFee(deposited, depositorFee float64) (float64, error) { + if deposited < 0 || depositorFee < 0 { + return 0, fmt.Errorf("negative values not allowed: deposited=%v, fee=%v", deposited, depositorFee) + } if deposited < depositorFee { return 0, fmt.Errorf("deposited amount %v is less than depositor fee %v", deposited, depositorFee) } return deposited - depositorFee, nil }docs/spec/crosschain/messages.md (1)
195-195: Fix indentation: replace tab with spaces.The line uses hard tabs for indentation. Replace with spaces to maintain consistency with the rest of the file.
- string error_message = 20; + string error_message = 20;🧰 Tools
🪛 Markdownlint (0.37.0)
195-195: Column: 1
Hard tabs(MD010, no-hard-tabs)
e2e/runner/bitcoin.go (1)
147-147: Define magic numbers as constants for clarityThe hardcoded amount
0.11used in the donation transaction should be defined as a constant. This improves code readability and makes future adjustments easier.Apply this diff:
+const DonationBTCAmount = 0.11 func (r *E2ERunner) DonateBTC() { // ... // it also serves as gas fee for the TSS to send BTC to other addresses - _, err := r.SendToTSSFromDeployerWithMemo(0.11, utxos[:2], []byte(constant.DonationMessage)) + _, err := r.SendToTSSFromDeployerWithMemo(DonationBTCAmount, utxos[:2], []byte(constant.DonationMessage)) require.NoError(r, err) }e2e/e2etests/test_bitcoin_deposit.go (1)
16-16: Replace literal boolean with a named constant or variablePassing a boolean literal like
truedirectly can make the code less readable. Consider defining a variable or using a constant to indicate the purpose of the boolean value.Apply this diff:
+addDepositFee := true txHash := r.DepositBTCWithAmount(depositAmount, nil, addDepositFee)This makes the code self-explanatory and enhances maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
typescript/zetachain/zetacore/crosschain/tx_pb.d.tsis excluded by!**/*_pb.d.tsx/crosschain/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (35)
changelog.md(1 hunks)cmd/zetae2e/local/bitcoin.go(1 hunks)cmd/zetae2e/local/local.go(1 hunks)docs/spec/crosschain/messages.md(1 hunks)e2e/e2etests/e2etests.go(2 hunks)e2e/e2etests/test_bitcoin_deposit.go(1 hunks)e2e/e2etests/test_bitcoin_deposit_abort_with_low_deposit_fee.go(1 hunks)e2e/e2etests/test_bitcoin_deposit_and_call_revert.go(1 hunks)e2e/e2etests/test_bitcoin_deposit_and_call_revert_with_dust.go(1 hunks)e2e/e2etests/test_bitcoin_deposit_and_withdraw_with_dust.go(1 hunks)e2e/e2etests/test_bitcoin_deposit_call.go(2 hunks)e2e/e2etests/test_bitcoin_donation.go(1 hunks)e2e/e2etests/test_bitcoin_std_deposit.go(1 hunks)e2e/e2etests/test_bitcoin_std_deposit_and_call.go(1 hunks)e2e/e2etests/test_bitcoin_std_deposit_and_call_revert.go(1 hunks)e2e/e2etests/test_bitcoin_std_deposit_and_call_revert_other_address.go(1 hunks)e2e/e2etests/test_crosschain_swap.go(2 hunks)e2e/e2etests/test_stress_btc_deposit.go(1 hunks)e2e/runner/bitcoin.go(5 hunks)proto/zetachain/zetacore/crosschain/tx.proto(1 hunks)x/crosschain/client/cli/tx_vote_inbound.go(1 hunks)x/crosschain/keeper/cctx_gateway_zevm.go(1 hunks)x/crosschain/keeper/evm_hooks.go(2 hunks)x/crosschain/types/cctx.go(1 hunks)x/crosschain/types/inbound_parsing.go(3 hunks)x/crosschain/types/message_vote_inbound.go(2 hunks)x/crosschain/types/message_vote_inbound_test.go(14 hunks)zetaclient/chains/bitcoin/observer/event.go(3 hunks)zetaclient/chains/bitcoin/observer/inbound.go(3 hunks)zetaclient/chains/bitcoin/observer/inbound_test.go(2 hunks)zetaclient/chains/bitcoin/observer/witness.go(3 hunks)zetaclient/chains/bitcoin/observer/witness_test.go(3 hunks)zetaclient/chains/evm/observer/v2_inbound.go(3 hunks)zetaclient/chains/solana/observer/inbound.go(1 hunks)zetaclient/zetacore/tx.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/crosschain/keeper/evm_hooks.go
🧰 Additional context used
📓 Path-based instructions (32)
e2e/e2etests/test_stress_btc_deposit.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/event.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.
e2e/e2etests/test_bitcoin_std_deposit.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.
cmd/zetae2e/local/bitcoin.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/keeper/cctx_gateway_zevm.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/client/cli/tx_vote_inbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_bitcoin_donation.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_bitcoin_std_deposit_and_call.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.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_call.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_bitcoin_std_deposit_and_call_revert.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_bitcoin_std_deposit_and_call_revert_other_address.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/types/cctx.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/message_vote_inbound_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/witness_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/zetacore/tx.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
proto/zetachain/zetacore/crosschain/tx.proto (1)
Pattern **/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.
e2e/e2etests/test_crosschain_swap.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.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/message_vote_inbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/witness.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/observer/inbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/inbound.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_abort_with_low_deposit_fee.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/bitcoin.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/inbound_parsing.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/observer/v2_inbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/inbound_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
📓 Learnings (4)
zetaclient/chains/bitcoin/observer/witness_test.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#2899
File: zetaclient/chains/bitcoin/observer/inbound.go:366-371
Timestamp: 2024-11-12T13:20:12.658Z
Learning: The `bitcoin.CalcDepositorFee` function is covered by live tests.
zetaclient/chains/bitcoin/observer/witness.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#2899
File: zetaclient/chains/bitcoin/observer/inbound.go:366-371
Timestamp: 2024-11-12T13:20:12.658Z
Learning: The `bitcoin.CalcDepositorFee` function is covered by live tests.
zetaclient/chains/bitcoin/observer/inbound.go (2)
Learnt from: ws4charlie
PR: zeta-chain/node#2899
File: zetaclient/chains/bitcoin/observer/inbound.go:37-38
Timestamp: 2024-11-12T13:20:12.658Z
Learning: In `BTCInboundEvent`, it's acceptable to use `float64` for monetary values (`Value` and `DepositorFee`) because precision is ensured through conversion to integer when building the vote message.
Learnt from: ws4charlie
PR: zeta-chain/node#2899
File: zetaclient/chains/bitcoin/observer/inbound.go:366-371
Timestamp: 2024-11-12T13:20:12.658Z
Learning: The `bitcoin.CalcDepositorFee` function is covered by live tests.
zetaclient/chains/bitcoin/observer/inbound_test.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#2899
File: zetaclient/chains/bitcoin/observer/inbound.go:366-371
Timestamp: 2024-11-12T13:20:12.658Z
Learning: The `bitcoin.CalcDepositorFee` function is covered by live tests.
🪛 Markdownlint (0.37.0)
docs/spec/crosschain/messages.md
195-195: Column: 1
Hard tabs
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: start-e2e-test / e2e
🔇 Additional comments (23)
x/crosschain/keeper/cctx_gateway_zevm.go (1)
Line range hint
32-49: LGTM! Clean error handling implementation.The error handling logic for EVM deposits is well-structured, with clear distinction between contract reverts and other errors.
e2e/e2etests/test_bitcoin_deposit_call.go (1)
37-37: Verify implications of using single UTXO.The test now uses only the first UTXO (
utxos[:1]). While this might be sufficient for basic testing, consider adding test cases that validate behavior with multiple UTXOs to ensure comprehensive coverage.✅ Verification successful
Single UTXO usage is consistent with codebase patterns
The use of a single UTXO in this test aligns with the established patterns across the codebase. Multiple UTXO support exists in the implementation but is only utilized for specific scenarios (e.g., donations), while standard deposit, withdrawal, and cross-chain operations consistently use single UTXOs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are other test files testing multiple UTXO scenarios rg -l "SendToTSSFromDeployerWithMemo.*utxos\[.*\]" --type goLength of output: 377
Script:
#!/bin/bash # Check UTXO usage patterns in the identified files rg "SendToTSSFromDeployerWithMemo.*utxos" -A 2 --type goLength of output: 2602
e2e/e2etests/test_bitcoin_deposit_and_call_revert_with_dust.go (1)
42-42: Consider using multiple UTXOs for comprehensive testing.Using only the first UTXO (
utxos[:1]) might not cover scenarios where multiple UTXOs are needed or when specific UTXO selection is important.cmd/zetae2e/local/bitcoin.go (1)
61-62: LGTM: Test setup enhanced with BTC donation.The addition of
DonateBTC()before deposits properly sets up the test environment for validating insufficient depositor fee scenarios.x/crosschain/types/message_vote_inbound.go (1)
74-74: LGTM: Error message propagation added.The addition of the error message field enhances error handling capabilities, particularly useful for tracking insufficient depositor fee cases.
Also applies to: 98-98
x/crosschain/types/cctx.go (1)
275-275: LGTM: Error propagation implemented correctly.The modification ensures that error messages from inbound votes are properly preserved in the CCTX status, which is essential for tracking insufficient depositor fee cases.
zetaclient/chains/solana/observer/inbound.go (1)
315-315: Consider implementing error message handling for Solana inbound transactions.While the empty string parameter has been added to align with the updated
NewMsgVoteInboundinterface, Solana might benefit from similar error handling as implemented for Bitcoin inbound transactions.Run this script to check if Solana has similar error conditions that could benefit from error message handling:
zetaclient/chains/bitcoin/observer/inbound.go (2)
324-324: LGTM! Good practice adding explicit error message handling.The addition of the errMessage variable enhances error tracking capabilities.
349-355: Well-structured error handling for insufficient depositor fees.The implementation properly captures and forwards error messages to zetacore, allowing for better tracking of failed deposits.
x/crosschain/types/message_vote_inbound_test.go (1)
41-41: LGTM! Comprehensive test coverage maintained.All test cases have been properly updated to include the new error message parameter, maintaining test coverage for the modified interface.
Also applies to: 68-68, 108-108, 143-143, 182-182, 216-216, 252-252, 273-273, 295-295, 329-329, 351-351, 374-374, 397-397, 420-420
zetaclient/chains/bitcoin/observer/witness_test.go (2)
108-151: LGTM! Thorough test coverage for error handling.The test case properly validates the error message emission when the deposit amount is insufficient.
334-375: Well-structured test cases for depositor fee deduction.The test cases cover all important scenarios:
- Successful deduction
- Zero remaining amount
- Insufficient deposit amount
zetaclient/chains/evm/observer/v2_inbound.go (1)
201-201: LGTM: Error message parameter addition is consistent.The addition of the empty string parameter for error messages in both
newDepositInboundVoteandnewCallInboundVotefunctions maintains consistency in the message structure.Also applies to: 338-338
zetaclient/chains/bitcoin/observer/inbound_test.go (1)
332-367: LGTM: Comprehensive test coverage for insufficient deposit fee scenario.The test case thoroughly validates the behavior when deposit amount is less than the required fee:
- Correctly sets up the test scenario with 1 satoshi less than the required fee
- Verifies the event value is set to zero
- Validates the error message format
cmd/zetae2e/local/local.go (1)
301-301: LGTM: Test case properly integrated.The new test case
TestBitcoinDepositAndAbortWithLowDepositFeeNameis appropriately placed in the basic Bitcoin deposit tests group, indicating its fundamental importance in validating deposit fee requirements.e2e/e2etests/e2etests.go (2)
85-85: LGTM: Test constant follows naming convention.The constant
TestBitcoinDepositAndAbortWithLowDepositFeeNamefollows the established naming pattern and clearly indicates its purpose.
651-656: LGTM: Test definition is well-structured.The E2ETest definition is properly configured:
- Clear description of the test purpose
- Empty argument list is appropriate as the test likely uses predefined values
- Follows the established pattern of test definitions
zetaclient/chains/bitcoin/observer/witness.go (1)
50-57: LGTM! Error message handling implementation.The addition of
ErrMessagefield enhances error tracking for failed deposits, aligning well with the PR objectives.e2e/e2etests/test_crosschain_swap.go (1)
105-105: Error handling issue for ListDeployerUTXOs.Same issue as in test_stress_btc_deposit.go regarding removed error handling.
Also applies to: 145-145
proto/zetachain/zetacore/crosschain/tx.proto (1)
187-189: LGTM! Well-documented protobuf field addition.The error_message field is properly documented and follows protobuf best practices.
zetaclient/chains/bitcoin/observer/event.go (2)
50-52: LGTM! Well-structured error field addition.The ErrMessage field addition to BTCInboundEvent is well-documented and aligns with the PR objectives.
201-201: LGTM! Consistent error message propagation.The error message propagation in both NewInboundVoteFrom methods maintains consistency across the codebase.
Also applies to: 239-239
changelog.md (1)
5-8: LGTM!The changelog entry is well-formatted and provides clear information about the feature addition.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3358 +/- ##
===========================================
- Coverage 63.22% 63.21% -0.01%
===========================================
Files 436 436
Lines 30636 30658 +22
===========================================
+ Hits 19369 19381 +12
- Misses 10432 10441 +9
- Partials 835 836 +1
|
Description
Solution:
To allow users to be able to track failed inbounds caused by low depositor fee, we register an aborted CCTX with
zeroamount and and proper error message.Some considerations:
Changes:
zetaclientpass error message of"deposited amount %v is less than depositor fee %vto zetacore inMsgVoteInboundstruct.zetacore, theCCTXGatewayZEVMregisters an aborted CCTX when it sees above error message fed byzetaclient.Closes: #3193
How Has This Been Tested?
Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
Release Notes
New Features
Improvements
Testing
Bug Fixes