feat: bitcoin depositor fee V2 to achieve better dev experience #2765
Conversation
ws4charlie
requested review from
brewmaster012,
kingpinXD,
lumtis,
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 WalkthroughWalkthroughThe changes introduce a new feature for enhancing Bitcoin depositor fee calculations, allowing for more accurate fee assessments based on individual transaction parameters. This update includes new functions and constants to improve fee estimation and management within the Bitcoin chain client, alongside modifications to existing functions and enhancements to the testing framework. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ZetaHub
participant BitcoinChainClient
participant RPCClient
User->>ZetaHub: Send Bitcoin transaction
ZetaHub->>BitcoinChainClient: Calculate depositor fee
BitcoinChainClient->>RPCClient: Get recent fee rate
RPCClient-->>BitcoinChainClient: Return fee rate
BitcoinChainClient-->>ZetaHub: Return calculated fee
ZetaHub-->>User: Confirm transaction submission
Assessment against linked issues
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 as PR comments)
Additionally, you can add Documentation and Community
|
|
!!!WARNING!!! Be very careful about using Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203 Pay extra attention to the way |
![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, codebase verification and nitpick comments (4)
zetaclient/chains/bitcoin/rpc/rpc.go (1)
57-69: Ensure comprehensive error handling inGetRawTxByHash.The function
GetRawTxByHashcorrectly wraps errors with additional context, which is good for debugging. However, consider logging the errors before returning them to provide more visibility into potential issues during runtime.zetaclient/chains/bitcoin/fee.go (2)
40-44: Clarify the purpose of fee-related constants.The constants
DynamicDepositorFeeHeightV2,feeRateCountBackBlocks, anddefaultTestnetFeeRateare introduced to manage fee calculations. Ensure that these values are documented and justified, especially in production environments, to avoid confusion.
273-311: Optimize error handling inGetRecentFeeRate.The function retrieves the highest fee rate from recent blocks. Consider adding logging for each error case to improve traceability and debugging. Additionally, ensure that the default fee rate is appropriate for the testnet environment.
zetaclient/chains/bitcoin/rpc/rpc_live_test.go (1)
Line range hint
35-47: Add error handling for environment variables.The function should handle cases where
BTC_RPC_MAINNETorBTC_RPC_TESTNETenvironment variables are not set.Consider adding checks and returning an error if these variables are missing.
rpcMainnet := os.Getenv("BTC_RPC_MAINNET") rpcTestnet := os.Getenv("BTC_RPC_TESTNET") +if rpcMainnet == "" || rpcTestnet == "" { + return nil, errors.New("RPC environment variables are not set") +}
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- changelog.md (1 hunks)
- zetaclient/chains/bitcoin/fee.go (3 hunks)
- zetaclient/chains/bitcoin/observer/inbound.go (1 hunks)
- zetaclient/chains/bitcoin/observer/observer.go (2 hunks)
- zetaclient/chains/bitcoin/rpc/rpc.go (3 hunks)
- zetaclient/chains/bitcoin/rpc/rpc_live_test.go (10 hunks)
Files skipped from review due to trivial changes (1)
- changelog.md
Additional context used
Path-based instructions (5)
zetaclient/chains/bitcoin/rpc/rpc.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.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/rpc/rpc_live_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/observer.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Additional comments not posted (13)
zetaclient/chains/bitcoin/rpc/rpc.go (1)
126-169: Verify the fee calculation logic inGetTransactionFeeAndRate.The function calculates transaction fees and rates based on input and output values. Ensure that the
vin.Voutindex is always valid and that the transaction structure aligns with expectations. Consider adding checks or logging to handle unexpected transaction structures.Run the following script to verify the transaction structure:
zetaclient/chains/bitcoin/fee.go (1)
249-271: Verify the logic inCalcDepositorFeeV2.The function calculates the depositor fee using the transaction's fee rate. Verify that the fee rate calculation aligns with the intended logic and that the
BTCOutboundGasPriceMultiplieris correctly applied. Consider logging the calculated fee rate for debugging purposes.Run the following script to verify the fee rate calculation:
Verification successful
Logic in
CalcDepositorFeeV2is Correctly ImplementedThe function
CalcDepositorFeeV2correctly calculates the depositor fee by applying theBTCOutboundGasPriceMultiplierto the transaction's fee rate. The logic aligns with the intended design, and the multiplier is accurately set to 2.0. The existing tests confirm the correct application of this logic. No changes are necessary.
- The fee rate is calculated using the transaction's input and output values.
- The multiplier is correctly applied to the fee rate.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the fee rate calculation logic in `CalcDepositorFeeV2`. # Test: Search for fee rate calculations. Expect: Correct application of BTCOutboundGasPriceMultiplier. rg --type go -A 5 'CalcDepositorFeeV2'Length of output: 3050
Script:
#!/bin/bash # Verify the implementation of GetTransactionFeeAndRate and the value of BTCOutboundGasPriceMultiplier. # Search for the implementation of GetTransactionFeeAndRate ast-grep --lang go --pattern 'func GetTransactionFeeAndRate($_, $_) ($_, $_, error) { $$$ }' # Search for the declaration of BTCOutboundGasPriceMultiplier rg --type go 'BTCOutboundGasPriceMultiplier'Length of output: 4641
zetaclient/chains/bitcoin/observer/inbound.go (1)
424-434: Ensure correctness of conditional logic inGetBtcEvent.The conditional block switches to
CalcDepositorFeeV2based on network conditions. Verify that the conditions are correctly implemented and that the transition to version 2 fee calculation is seamless. Consider adding logging to track which fee calculation method is used.Run the following script to verify the conditional logic:
Verification successful
Conditional Logic in
GetBtcEventVerifiedThe conditional logic for switching to
CalcDepositorFeeV2is correctly implemented based on network conditions and block height. The transition is seamless, and the function is well-tested in various scenarios. No changes are necessary.
- The logic checks for TestNet3 or MainNet with the appropriate block height.
- Extensive test coverage in
inbound_test.goensures robustness.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the conditional logic for switching to `CalcDepositorFeeV2`. # Test: Search for conditional logic in `GetBtcEvent`. Expect: Correct implementation of network conditions. rg --type go -A 5 'GetBtcEvent'Length of output: 19792
zetaclient/chains/bitcoin/rpc/rpc_live_test.go (9)
Line range hint
62-79: LGTM!The function correctly retrieves and handles the fee rate.
81-102: LGTM!The function is well-structured and handles errors appropriately.
104-117: LGTM!The function serves as a placeholder for running individual live tests.
119-154: LGTM!The test function is comprehensive and checks multiple aspects of incoming transactions.
157-181: LGTM!The test correctly verifies the absence of incoming transactions.
Line range hint
185-200: LGTM!The test ensures proper creation and functionality of the Bitcoin RPC client.
Line range hint
204-222: LGTM!The test effectively covers valid and invalid scenarios for block height retrieval.
Line range hint
226-272: LGTM!The test effectively monitors and compares Bitcoin fee rates over time.
454-549: LGTM!The test accurately compares calculated transaction fees and rates with external data.
zetaclient/chains/bitcoin/observer/observer.go (1)
Line range hint
630-634: LGTM!The change to use
bitcoin.GetRecentFeeRatesimplifies the code and improves maintainability.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (5)
zetaclient/chains/bitcoin/rpc/rpc_live_test.go (4)
Line range hint
30-60: Add error handling for unset environment variables.The
createRPCClientfunction relies on environment variables for configuration. Consider adding checks to ensure these variables are set and provide meaningful error messages if they are not.rpcMainnet := os.Getenv("BTC_RPC_MAINNET") rpcTestnet := os.Getenv("BTC_RPC_TESTNET") + if rpcMainnet == "" || rpcTestnet == "" { + return nil, errors.New("environment variables BTC_RPC_MAINNET or BTC_RPC_TESTNET are not set") + }
Line range hint
185-202: Add assertions to verify RPC client properties.Consider adding assertions to verify the properties of the created RPC client, such as connection status.
require.NotNil(t, client) require.NoError(t, client.Ping())
Line range hint
226-274: Avoid infinite loops in tests.The
LiveTest_BitcoinFeeRatefunction contains an infinite loop, which is not suitable for tests. Consider using a finite loop or a timeout mechanism.- for { + for i := 0; i < 5; i++ {
Line range hint
276-349: Consider using structured logging.Using structured logging can improve the clarity and searchability of log messages.
log.Printf("block %d: gas rate %d == mempool.space gas rate\n", mb.Height, gasRate)zetaclient/chains/bitcoin/observer/observer.go (1)
Line range hint
553-599: Enhance error messages for better debugging.Consider adding more detailed error messages to aid in debugging.
return "", errors.Wrapf(err, "error getting raw transaction %s", vin.Txid)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- changelog.md (1 hunks)
- zetaclient/chains/bitcoin/fee.go (3 hunks)
- zetaclient/chains/bitcoin/observer/inbound.go (1 hunks)
- zetaclient/chains/bitcoin/observer/observer.go (2 hunks)
- zetaclient/chains/bitcoin/rpc/rpc.go (3 hunks)
- zetaclient/chains/bitcoin/rpc/rpc_live_test.go (10 hunks)
Files skipped from review due to trivial changes (1)
- changelog.md
Additional context used
Path-based instructions (5)
zetaclient/chains/bitcoin/rpc/rpc.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.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/rpc/rpc_live_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/observer.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Additional comments not posted (18)
zetaclient/chains/bitcoin/rpc/rpc.go (2)
57-69: FunctionGetRawTxByHashis well-implemented.The function correctly retrieves a raw transaction by hash and handles errors appropriately.
126-169: FunctionGetTransactionFeeAndRateis well-implemented.The function accurately calculates transaction fees and rates, with appropriate error handling and safety checks.
zetaclient/chains/bitcoin/fee.go (3)
41-41: ConstantDynamicDepositorFeeHeightV2is well-defined.The constant is clearly named and serves its intended purpose effectively.
249-271: FunctionCalcDepositorFeeV2is well-implemented.The function accurately calculates the depositor fee with appropriate error handling and network parameter checks.
273-311: FunctionGetRecentFeeRateis well-implemented.The function effectively retrieves the highest fee rate from recent blocks with comprehensive error handling.
zetaclient/chains/bitcoin/observer/inbound.go (1)
424-435: Conditional logic inGetBtcEventis well-implemented.The function correctly determines when to use
CalcDepositorFeeV2based on network conditions and block height, with appropriate error handling.zetaclient/chains/bitcoin/rpc/rpc_live_test.go (10)
Line range hint
62-79: LGTM! The function is well-implemented.The
getFeeRatefunction is correctly implemented with appropriate error handling.
104-117: LGTM! The function serves as a placeholder.The
Test_BitcoinLivefunction is appropriately set up as a placeholder for running individual live tests.
119-154: LGTM! The test is well-structured.The
LiveTest_FilterAndParseIncomingTxfunction is well-structured with clear assertions and error handling.
157-181: LGTM! The test correctly checks for no incoming transactions.The
LiveTest_FilterAndParseIncomingTx_Nopfunction is concise and correctly checks for the absence of incoming transactions.
Line range hint
204-222: LGTM! The test is well-implemented.The
LiveTest_GetBlockHeightByHashfunction is well-implemented with appropriate error handling and assertions.
Line range hint
351-362: LGTM! The test is straightforward and correct.The
LiveTest_AvgFeeRateMainnetMempoolSpacefunction is straightforward and correctly utilizes the helper function.
Line range hint
365-376: LGTM! The test is correctly implemented.The
LiveTest_AvgFeeRateTestnetMempoolSpacefunction is similar to the mainnet test and is correctly implemented.
379-387: LGTM! The test is concise and correct.The
LiveTest_GetRecentFeeRatefunction is concise and correctly asserts the fee rate.
454-549: LGTM! The test is comprehensive and detailed.The
Test_GetTransactionFeeAndRatefunction is comprehensive and includes detailed comparisons and assertions.
551-580: LGTM! The test is well-structured and meaningful.The
LiveTest_CalcDepositorFeeV2function is well-structured and includes meaningful test cases.zetaclient/chains/bitcoin/observer/observer.go (2)
Line range hint
72-116: LGTM! The function is well-implemented.The
NewObserverfunction is well-implemented, with appropriate error handling and initialization logic.
Line range hint
629-635: LGTM! The refactoring improves modularity.The refactoring to use
bitcoin.GetRecentFeeRateinspecialHandleFeeRateimproves modularity and maintainability.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2765 +/- ##
===========================================
- Coverage 66.92% 66.78% -0.15%
===========================================
Files 364 364
Lines 20471 20516 +45
===========================================
+ Hits 13701 13702 +1
- Misses 6143 6186 +43
- Partials 627 628 +1
|
Description
The current depositor fee calculation is based on the average fee rate of the block. After this modification, zetaclient will use inbound (the deposit tx) transaction's fee rate to calculate depositor fee.
This new impl will allow developer to accurately estimate depositor fee right after they build the deposit tx. The formula is
depositFee == (txFee / txVsize) * 68vB * 2txFeeis the deposit transaction's fee. This can be calculated by formula:totalInputValue - totalOutputValuetxVsizeis the deposit transaction's size in vByte. This can be retrieved from RPCgetrawtransaction(ZetaChain's implementation).Closes: #2155
How Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests