fix: skip depositor fee calculation on irrelevant transactions #3162
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several updates to the ZetaChain project, including new features like the ability to whitelist SPL tokens on Solana and enhancements to SPL deposits and withdrawals. Significant refactoring has occurred, including the removal of the HSM signer and simplification of the zetaclientd configuration. The tests have been expanded to include new scenarios for concurrent operations and error handling. Fixes address various issues, such as emissions registration and transaction handling, thereby improving overall system reliability and user experience. 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
|
ws4charlie
requested review from
brewmaster012,
kingpinXD,
lumtis,
skosito,
swift1337 and
fbac
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3162 +/- ##
===========================================
+ Coverage 62.64% 62.67% +0.03%
===========================================
Files 424 424
Lines 30124 30128 +4
===========================================
+ Hits 18872 18884 +12
+ Misses 10410 10402 -8
Partials 842 842
|
![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 (8)
zetaclient/chains/bitcoin/fee.go (3)
59-60: Enhance documentation for DepositorFeeCalculator typeWhile the type definition is well-structured, consider adding more detailed documentation to explain:
- The purpose and use cases of this calculator
- The expected behavior when calculating fees
- Any constraints or assumptions about the input parameters
Add this documentation above the type definition:
+// DepositorFeeCalculator defines a function type that computes Bitcoin depositor fees. +// It takes a Bitcoin RPC client, transaction details, and network parameters as input, +// and returns the calculated fee in BTC along with any potential errors encountered +// during the calculation process. type DepositorFeeCalculator func(interfaces.BTCRPCClient, *btcjson.TxRawResult, *chaincfg.Params) (float64, error)
Line range hint
89-108: Optimize EstimateOutboundSize function for better performance and error handlingConsider optimizing the function to reduce allocations and improve error handling:
- Pre-calculate total bytes for payees to avoid multiple additions
- Use more specific error types for better error handling
Consider this optimization:
func EstimateOutboundSize(numInputs uint64, payees []btcutil.Address) (uint64, error) { if numInputs == 0 { return 0, nil } numOutputs := 2 + uint64(len(payees)) bytesWiredTx := WiredTxSize(numInputs, numOutputs) bytesInput := numInputs * bytesPerInput bytesOutput := uint64(2) * bytesPerOutputP2WPKH - bytesToPayees := uint64(0) + var bytesToPayees uint64 + var err error for _, to := range payees { sizeOutput, err := GetOutputSizeByAddress(to) if err != nil { - return 0, err + return 0, fmt.Errorf("failed to get output size for address: %w", err) } bytesToPayees += sizeOutput } bytesWitness := bytes1stWitness + (numInputs-1)*bytesPerWitness return bytesWiredTx + bytesInput + bytesOutput + bytesToPayees + bytesWitness/blockchain.WitnessScaleFactor, nil }
Line range hint
214-234: Enhance CalcDepositorFee with input validation and documentationThe function should validate the gas price multiplier and document its impact on fee calculations.
Consider these improvements:
+// CalcDepositorFee calculates the depositor fee for a given transaction. +// The fee is calculated based on the transaction's fee rate and adjusted by BTCOutboundGasPriceMultiplier. +// For regression networks, it returns the default fee. +// +// Parameters: +// - rpcClient: Bitcoin RPC client interface +// - rawResult: Raw transaction result containing fee information +// - netParams: Network parameters to determine the network type +// +// Returns: +// - float64: Calculated fee in BTC +// - error: Any error encountered during calculation func CalcDepositorFee( rpcClient interfaces.BTCRPCClient, rawResult *btcjson.TxRawResult, netParams *chaincfg.Params, ) (float64, error) { + if clientcommon.BTCOutboundGasPriceMultiplier <= 0 { + return 0, fmt.Errorf("invalid gas price multiplier: %f", clientcommon.BTCOutboundGasPriceMultiplier) + } + if netParams.Name == chaincfg.RegressionNetParams.Name { return DefaultDepositorFee, nil }zetaclient/chains/bitcoin/observer/witness_test.go (2)
64-64: Consider extracting fee rate constant and documenting mock setupThe fee rate of 28 sat/vB should be extracted as a named constant. Additionally, consider documenting the
mockDepositFeeCalculatorhelper function for better test maintainability.+const ( + // TestFeeRate represents the fee rate of the test transaction + // https://mempool.space/tx/847139aa65aa4a5ee896375951cbf7417cfc8a4d6f277ec11f40cd87319f04aa + TestFeeRate = 28 // sat/vB +) + +// mockDepositFeeCalculator creates a mock fee calculator for testing +// depositorFee: the fee to return +// err: the error to return, if any +func mockDepositFeeCalculator(depositorFee float64, err error) bitcoin.DepositorFeeCalculator { + // Implementation here +} - feeCalculator := mockDepositFeeCalculator(depositorFee, nil) + feeCalculator := mockDepositFeeCalculator(bitcoin.DepositorFee(TestFeeRate * clientcommon.BTCOutboundGasPriceMultiplier), nil)
202-219: Enhance error assertion specificityWhile the test case correctly verifies error propagation, consider making the error assertion more specific to ensure the exact error path is being tested.
- require.ErrorContains(t, err, "rpc error") + require.ErrorIs(t, err, errors.New("failed to calculate depositor fee: rpc error"))zetaclient/chains/bitcoin/observer/inbound.go (1)
425-430: Consider enhancing error handling for fee calculationWhile the current implementation handles fee calculation errors, consider adding specific error types to distinguish between different failure scenarios (e.g., RPC failures vs validation errors). This would enable more precise error handling in upstream code.
Example enhancement:
// calculate depositor fee depositorFee, err = feeCalculator(rpcClient, &tx, netParams) if err != nil { - return nil, errors.Wrapf(err, "error calculating depositor fee for inbound %s", tx.Txid) + switch { + case errors.Is(err, bitcoin.ErrRPCFailure): + return nil, errors.Wrapf(err, "RPC failure while calculating depositor fee for inbound %s", tx.Txid) + case errors.Is(err, bitcoin.ErrInvalidAmount): + logger.Info().Msgf("Invalid amount for depositor fee calculation in txid %s", tx.Txid) + return nil, nil + default: + return nil, errors.Wrapf(err, "error calculating depositor fee for inbound %s", tx.Txid) + } }zetaclient/chains/bitcoin/observer/inbound_test.go (1)
Line range hint
1-763: Consider adding edge cases for fee calculation.While the test coverage is comprehensive for address types and basic error scenarios, consider adding test cases for edge cases in fee calculation:
- Maximum possible fee values
- Zero fee scenarios
- Fee calculation with decimal precision edge cases
changelog.md (1)
30-30: LGTM! Consider adding more details about the fix.The changelog entry correctly documents the fix from PR #3162. To improve clarity, consider expanding the description to explain why this change was needed and its impact:
-* [3162](https://github.com/zeta-chain/node/pull/3162) - skip depositor fee calculation if transaction does not involve TSS address +* [3162](https://github.com/zeta-chain/node/pull/3162) - skip depositor fee calculation for transactions not involving TSS address to optimize gas usage and prevent unnecessary fee calculations
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
changelog.md(1 hunks)zetaclient/chains/bitcoin/fee.go(1 hunks)zetaclient/chains/bitcoin/observer/inbound.go(5 hunks)zetaclient/chains/bitcoin/observer/inbound_test.go(23 hunks)zetaclient/chains/bitcoin/observer/witness.go(2 hunks)zetaclient/chains/bitcoin/observer/witness_test.go(8 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
zetaclient/chains/bitcoin/fee.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.
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.
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/bitcoin/observer/witness_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/fee.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:366-371
Timestamp: 2024-11-12T13:20:12.658Z
Learning: The `bitcoin.CalcDepositorFee` function is covered by live tests.
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.
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.
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.
🪛 GitHub Check: codecov/patch
zetaclient/chains/bitcoin/observer/inbound.go
[warning] 266-266: zetaclient/chains/bitcoin/observer/inbound.go#L266
Added line #L266 was not covered by tests
[warning] 320-320: zetaclient/chains/bitcoin/observer/inbound.go#L320
Added line #L320 was not covered by tests
[warning] 387-387: zetaclient/chains/bitcoin/observer/inbound.go#L387
Added line #L387 was not covered by tests
🔇 Additional comments (9)
zetaclient/chains/bitcoin/observer/witness_test.go (1)
97-97: LGTM: Consistent fee calculator integration
The fee calculator is consistently integrated across all test cases, maintaining a clean and uniform testing pattern.
Also applies to: 135-135, 176-176, 196-196, 235-235, 257-257, 291-291
zetaclient/chains/bitcoin/observer/inbound.go (3)
266-266: LGTM: Fee calculation modularization
The introduction of bitcoin.CalcDepositorFee as a parameter enhances modularity and testability. While static analysis flags this line as untested, our records indicate it's covered by live tests.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 266-266: zetaclient/chains/bitcoin/observer/inbound.go#L266
Added line #L266 was not covered by tests
320-320: LGTM: Consistent fee calculation approach
The change maintains consistency with the fee calculation refactoring while preserving the robust error handling for retry scenarios.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 320-320: zetaclient/chains/bitcoin/observer/inbound.go#L320
Added line #L320 was not covered by tests
382-387: LGTM: Clean network-specific handling
The changes maintain a clear distinction between mainnet and testnet handling while consistently applying the new fee calculation approach.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 387-387: zetaclient/chains/bitcoin/observer/inbound.go#L387
Added line #L387 was not covered by tests
zetaclient/chains/bitcoin/observer/inbound_test.go (3)
34-39: LGTM! Well-structured mock function.
The mockDepositFeeCalculator function is well-designed, providing a clean way to simulate fee calculation with configurable return values.
289-289: LGTM! Consistent integration of fee calculator mock.
The fee calculator mock is consistently integrated across all test cases, maintaining the test's integrity while adapting to the new fee calculation approach.
Also applies to: 321-321, 345-345, 369-369, 393-393, 417-417, 437-437, 457-457, 471-471, 491-491, 530-530, 550-550, 570-570, 601-601, 635-635, 655-655, 677-677, 707-707, 727-727, 758-758
497-514: LGTM! Comprehensive error handling test.
The new test case effectively verifies the system's behavior when RPC fails to calculate the depositor fee, ensuring robust error handling in production scenarios.
zetaclient/chains/bitcoin/observer/witness.go (2)
27-27: Enhancement: Injected Fee Calculator for Modularity
Introducing feeCalculator as a parameter enhances the modularity and testability of the function by allowing dynamic fee calculation strategies.
43-47: Proper Error Wrapping in Fee Calculation
The error from feeCalculator is appropriately wrapped with context using errors.Wrapf, aiding in precise error tracking and debugging.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Description
The official fix that replaces #3161
How Has This Been Tested?
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
Tests