refactor: remove btc deposit fee v1 and improve unit tests #2899
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 WalkthroughWalkthroughThe pull request introduces several significant changes across multiple files, primarily focusing on the simplification and refactoring of Bitcoin transaction handling. Key modifications include the removal of the Changes
Sequence Diagram(s)sequenceDiagram
participant Observer
participant BTCInboundEvent
participant TransactionHandler
Observer->>TransactionHandler: ObserveInbound()
TransactionHandler->>BTCInboundEvent: Create new event
TransactionHandler->>Observer: Return BTCInboundEvent
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
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 #2899 +/- ##
===========================================
+ Coverage 67.20% 67.27% +0.06%
===========================================
Files 380 380
Lines 21170 21175 +5
===========================================
+ Hits 14227 14245 +18
+ Misses 6276 6264 -12
+ Partials 667 666 -1
|
|
!!!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 |
ws4charlie
requested review from
brewmaster012,
lumtis,
skosito,
swift1337 and
fbac
as code owners
…hain/node into remove-btc-deposit-fee-V1
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: 7
Outside diff range and nitpick comments (4)
zetaclient/chains/base/observer.go (1)
232-235: Add test coverage forIsBlockConfirmed.The static analysis tool reports that this new function is not covered by tests. To maintain a high-quality codebase and prevent potential bugs, it's crucial to add unit tests that verify the correctness of this function.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 232-235: zetaclient/chains/base/observer.go#L232-L235
Added lines #L232 - L235 were not covered by testszetaclient/chains/bitcoin/observer/witness_test.go (1)
Line range hint
97-97: Use field-by-field comparison for structs containing floating-point fieldsComparing structs with floating-point fields using
require.Equalmay lead to test failures due to precision discrepancies. It's advisable to compare each field individually, usingrequire.InDeltafor floating-point numbers to account for minor differences.Suggestion:
Replace:
require.Equal(t, eventExpected, event)With:
require.Equal(t, eventExpected.FromAddress, event.FromAddress) require.Equal(t, eventExpected.ToAddress, event.ToAddress) require.InDelta(t, eventExpected.Value, event.Value, 1e-8) require.Equal(t, eventExpected.DepositorFee, event.DepositorFee) require.Equal(t, eventExpected.MemoBytes, event.MemoBytes) require.Equal(t, eventExpected.BlockNumber, event.BlockNumber) require.Equal(t, eventExpected.TxHash, event.TxHash)This ensures that floating-point comparisons for
Valueare precise within an acceptable delta, improving test reliability.Also applies to: 136-136, 164-164
zetaclient/testutils/mocks/btc_rpc.go (1)
24-547: Avoid Using Underscores in Receiver Names for Idiomatic Go CodeThe receiver for the methods is named
_m, which includes an underscore prefix. In Go, it's idiomatic to use concise receiver names without underscores. Consider renaming the receiver tomorclientto improve code readability and adhere to Go conventions.Apply this diff to rename the receiver variable:
-func (_m *BTCRPCClient) CreateWallet(name string, opts ...rpcclient.CreateWalletOpt) (*btcjson.CreateWalletResult, error) { +func (m *BTCRPCClient) CreateWallet(name string, opts ...rpcclient.CreateWalletOpt) (*btcjson.CreateWalletResult, error) {Repeat this change for all methods to ensure consistency.
zetaclient/chains/bitcoin/observer/inbound.go (1)
Line range hint
168-182: Enhance error handling to process all events reliablyCurrently, if
PostVoteInboundreturns an error for an event, the function returns immediately, and subsequent events are not processed. This could lead to delays in handling other valid events. It would be more robust to log the error for the failed event and continue processing the remaining events.Here's a suggested modification:
for _, event := range events { msg := ob.GetInboundVoteMessageFromBtcEvent(event) if msg != nil { zetaHash, ballot, err := zetaCoreClient.PostVoteInbound( ctx, zetacore.PostVoteInboundGasLimit, zetacore.PostVoteInboundExecutionGasLimit, msg, ) if err != nil { ob.logger.Inbound.Error(). Err(err). - Msgf("observeInboundBTC: error posting to zetacore for tx %s", event.TxHash) - return err // we have to re-scan this block next time + Msgf("observeInboundBTC: error posting to zetacore for tx %s; continuing with next event", event.TxHash) + continue // Proceed to the next event } else if zetaHash != "" { ob.logger.Inbound.Info().Msgf("observeInboundBTC: PostVoteInbound zeta tx hash: %s inbound %s ballot %s fee %v", zetaHash, event.TxHash, ballot, event.DepositorFee) } } }This change ensures that all events are attempted, improving the resilience of the observer.
Tools
GitHub Check: codecov/patch
[warning] 180-180: zetaclient/chains/bitcoin/observer/inbound.go#L180
Added line #L180 was not covered by tests
[warning] 184-184: zetaclient/chains/bitcoin/observer/inbound.go#L184
Added line #L184 was not covered by tests
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (14)
- e2e/runner/bitcoin.go (0 hunks)
- zetaclient/chains/base/observer.go (1 hunks)
- zetaclient/chains/bitcoin/fee.go (1 hunks)
- zetaclient/chains/bitcoin/observer/inbound.go (8 hunks)
- zetaclient/chains/bitcoin/observer/inbound_test.go (22 hunks)
- zetaclient/chains/bitcoin/observer/observer.go (0 hunks)
- zetaclient/chains/bitcoin/observer/observer_test.go (8 hunks)
- zetaclient/chains/bitcoin/observer/outbound_test.go (1 hunks)
- zetaclient/chains/bitcoin/observer/witness.go (1 hunks)
- zetaclient/chains/bitcoin/observer/witness_test.go (8 hunks)
- zetaclient/chains/bitcoin/rpc/rpc_live_test.go (2 hunks)
- zetaclient/testdata/btc/chain_8332_msgtx_847139aa65aa4a5ee896375951cbf7417cfc8a4d6f277ec11f40cd87319f04aa.json (1 hunks)
- zetaclient/testutils/mocks/btc_rpc.go (1 hunks)
- zetaclient/testutils/testrpc/rpc_btc.go (2 hunks)
Files not reviewed due to no reviewable changes (2)
- e2e/runner/bitcoin.go
- zetaclient/chains/bitcoin/observer/observer.go
Files skipped from review due to trivial changes (1)
- zetaclient/chains/bitcoin/rpc/rpc_live_test.go
Additional context used
Path-based instructions (10)
zetaclient/chains/base/observer.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/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/observer_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/outbound_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.zetaclient/testutils/mocks/btc_rpc.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/testutils/testrpc/rpc_btc.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
GitHub Check: codecov/patch
zetaclient/chains/base/observer.go
[warning] 232-235: zetaclient/chains/base/observer.go#L232-L235
Added lines #L232 - L235 were not covered by testszetaclient/chains/bitcoin/observer/inbound.go
[warning] 131-132: zetaclient/chains/bitcoin/observer/inbound.go#L131-L132
Added lines #L131 - L132 were not covered by tests
[warning] 143-144: zetaclient/chains/bitcoin/observer/inbound.go#L143-L144
Added lines #L143 - L144 were not covered by tests
[warning] 152-152: zetaclient/chains/bitcoin/observer/inbound.go#L152
Added line #L152 was not covered by tests
[warning] 168-169: zetaclient/chains/bitcoin/observer/inbound.go#L168-L169
Added lines #L168 - L169 were not covered by tests
[warning] 180-180: zetaclient/chains/bitcoin/observer/inbound.go#L180
Added line #L180 was not covered by tests
[warning] 184-184: zetaclient/chains/bitcoin/observer/inbound.go#L184
Added line #L184 was not covered by tests
[warning] 294-296: zetaclient/chains/bitcoin/observer/inbound.go#L294-L296
Added lines #L294 - L296 were not covered by tests
[warning] 337-337: zetaclient/chains/bitcoin/observer/inbound.go#L337
Added line #L337 was not covered by tests
[warning] 355-355: zetaclient/chains/bitcoin/observer/inbound.go#L355
Added line #L355 was not covered by tests
[warning] 362-364: zetaclient/chains/bitcoin/observer/inbound.go#L362-L364
Added lines #L362 - L364 were not covered by tests
[warning] 367-367: zetaclient/chains/bitcoin/observer/inbound.go#L367
Added line #L367 was not covered by tests
[warning] 373-374: zetaclient/chains/bitcoin/observer/inbound.go#L373-L374
Added lines #L373 - L374 were not covered by tests
[warning] 378-378: zetaclient/chains/bitcoin/observer/inbound.go#L378
Added line #L378 was not covered by tests
Additional comments not posted (17)
zetaclient/testdata/btc/chain_8332_msgtx_847139aa65aa4a5ee896375951cbf7417cfc8a4d6f277ec11f40cd87319f04aa.json (1)
1-27: Approve the addition of the Bitcoin transaction test data file.The new JSON file provides a valid Bitcoin transaction structure for testing purposes. It includes essential components such as version, inputs, outputs, and lock time, making it a suitable test case for validating transaction handling in the codebase.
The file is appropriately placed in the
testdatadirectory, ensuring it does not interfere with the production code.zetaclient/testutils/testrpc/rpc_btc.go (1)
61-80: LGTM!The
CreateBTCRPCAndLoadTxfunction is a well-structured and useful addition to thetestrpcpackage. It enhances the testing capabilities by allowing for the simulation of Bitcoin RPC interactions with pre-defined transaction data.The function follows a clear logic flow:
- It creates a mock RPC client using the
mocks.NewBTCRPCClientfunction.- For each transaction hash provided, it constructs the file path for the corresponding
MsgTx, loads the transaction data using thetestutils.LoadObjectFromJSONFilefunction, and sets up the mock client to return the transaction when queried with its hash.- Finally, it returns the mock RPC client.
The function uses appropriate naming conventions and follows the Go best practices.
zetaclient/chains/bitcoin/observer/witness.go (1)
71-77: LGTM!The addition of the
DepositorFeefield to theBTCInboundEventstruct enhances the event's data representation by including the depositor's fee information. ThedepositorFeeparameter is used consistently throughout the function to validate the amount and calculate the actual deposit amount. The changes are logically sound and syntactically correct.zetaclient/chains/bitcoin/observer/observer_test.go (4)
75-77: LGTM!The changes made to the
MockBTCObserverfunction improve the testability and clarity of the mock interactions by using a more structured approach with thestretchr/testify/mockpackage. The initialization of the mock Bitcoin client and setting expectations for theGetBlockCountmethod call are consistent with the overall refactoring theme of the pull request.
105-108: LGTM!The changes made to the
Test_NewObserverfunction improve the consistency and clarity of the mock client usage across the test cases by using the new mock client structure. The creation of the mock Bitcoin client with a block height of 100 and setting expectations for theGetBlockCountmethod call are in line with the overall refactoring theme of the pull request.Also applies to: 126-126, 134-134, 143-143
208-208: LGTM!The changes made to the
Test_BlockCachefunction improve the testability and clarity of the mock interactions by using a more structured approach with thestretchr/testify/mockpackage. The creation of the mock Bitcoin client and setting expectations for various method calls likeGetBlockHash,GetBlockHeader, andGetBlockVerboseTxare consistent with the overall refactoring theme of the pull request.Also applies to: 215-217
254-256: LGTM!The changes made to the
Test_LoadLastBlockScannedfunction improve the consistency and clarity of the mock client usage across the test cases by using the new mock client structure. The creation of the mock Bitcoin client with a block height of 200 and setting expectations for theGetBlockCountmethod call are in line with the overall refactoring theme of the pull request.The additional test case for handling RPC errors, where a mock Bitcoin client that returns an RPC error is attached, enhances the test coverage and ensures proper error handling in the observer.
Also applies to: 288-291, 295-295
zetaclient/chains/base/observer.go (1)
232-235: LGTM!The
IsBlockConfirmedfunction correctly determines if a given block is confirmed by comparing it against the last observed block and the configured confirmation count. The logic and implementation are accurate.Tools
GitHub Check: codecov/patch
[warning] 232-235: zetaclient/chains/base/observer.go#L232-L235
Added lines #L232 - L235 were not covered by testszetaclient/chains/bitcoin/observer/outbound_test.go (1)
30-32: LGTM!The updated mock client instantiation using
mocks.NewBTCRPCClient(t)and the subsequent configuration of theGetBlockCountmethod response enhances the test's ability to simulate interactions with the Bitcoin RPC client more accurately. This change improves the test's fidelity and allows for more precise control over the responses during testing.zetaclient/chains/bitcoin/fee.go (1)
Line range hint
228-245: Implementation ofCalcDepositorFeeis clear and efficient.The refactored function accurately calculates the depositor fee based on the transaction fee rate. It properly handles different network parameters, ensures precise fee computations, and includes appropriate error handling.
zetaclient/chains/bitcoin/observer/witness_test.go (6)
8-8: Importing 'github.com/pkg/errors' is appropriateThe
"github.com/pkg/errors"package is correctly imported and utilized for error handling within the tests, enhancing the clarity and robustness of error messages.
12-12: Inclusion of 'github.com/stretchr/testify/mock' for mockingThe
"github.com/stretchr/testify/mock"package is appropriately imported and used for creating mock objects in your tests, which improves test isolation and reliability.
19-19: Utilization of 'testutils/testrpc' enhances test maintainabilityImporting
"github.com/zeta-chain/node/zetaclient/testutils/testrpc"streamlines the setup of mock RPC clients, improving test readability and maintainability by reducing boilerplate code.
62-62: Accurate calculation of 'depositorFee' for realistic testingThe
depositorFeeis correctly calculated using a fee rate of 28 sat/vB multiplied by the outbound gas price multiplier. This provides a more accurate simulation of transaction fees in your tests.
85-85: Refactored mock RPC client setup improves test clarityUsing
testrpc.CreateBTCRPCAndLoadTxto create the mock RPC client and load transaction data simplifies the test setup, enhancing code readability and reducing the potential for setup errors.
225-226: Proper mocking of RPC client errors enhances test robustnessBy configuring the mock RPC client to return an error on
GetRawTransaction, you effectively simulate error conditions, which is crucial for testing error handling logic.zetaclient/testutils/mocks/btc_rpc.go (1)
24-547: Adoption of testify/mock Improves Test Maintainability and StandardizationThe migration from a custom
MockBTCRPCClientto an autogenerated mock usingtestify/mockis a commendable improvement. Utilizingtestify/mockstandardizes the mocking framework across the codebase, reduces manual coding errors, and leverages a well-supported library. This change enhances the expressiveness and maintainability of your tests, ensuring they are concise and production-grade.
ws4charlie
changed the title
…eSenderFromScript() and added unit tests
Description
CalcDepositorFeeV1IsBlockConfirmed; BothWatchInboundandWatchInboundTrackertickers use it.Closes: #2766
How Has This Been Tested?
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Documentation