refactor: (BitcoinRBF-Step2): some minimum code refactoring for Bitcoin RBF adoption #3381
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 comprehensive refactoring and enhancements to the Bitcoin client functionality within the ZetaChain node, focusing on improving transaction handling, fee estimation, and observability. The changes span multiple components of the Bitcoin client, including observer, signer, and utility modules, with a particular emphasis on Replace-By-Fee (RBF) adoption and more robust transaction management. Changes
Suggested labels
Possibly related PRs
Suggested Reviewers
The pull request represents a significant step in improving the Bitcoin client's robustness, with particular attention to transaction fee management, address handling, and testing infrastructure. The changes demonstrate a methodical approach to refactoring, ensuring type safety, improving error handling, and enhancing the overall reliability of Bitcoin transaction processing within the ZetaChain ecosystem. 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
|
|
!!!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
added
zetaclient
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: 14
🔭 Outside diff range comments (4)
zetaclient/chains/bitcoin/observer/outbound_test.go (1)
Line range hint
265-349: Consider using satoshis instead of BTC for precise calculations.The test cases use floating-point values for Bitcoin amounts, which can lead to precision issues. Consider using satoshis (integer values) for calculations to ensure exact precision.
Example conversion:
-selected, err := ob.SelectUTXOs(ctx, 0.01, 5, 0, math.MaxUint16, true) +selected, err := ob.SelectUTXOs(ctx, 1000000, 5, 0, math.MaxUint16, true) // 0.01 BTC = 1,000,000 satoshiszetaclient/chains/evm/signer/signer.go (1)
Line range hint
279-392: Consider refactoring complex conditional logic using a strategy pattern.The
SignOutboundFromCCTXfunction contains complex nested if-else statements that make the code hard to maintain. Consider using a strategy pattern to handle different transaction types.Example approach:
type OutboundSigner interface { Sign(ctx context.Context, cctx *crosschaintypes.CrossChainTx, data *OutboundData) (*ethtypes.Transaction, error) } // Implement different strategies type ComplianceSigner struct{} type AdminCommandSigner struct{} type V2ProtocolSigner struct{} // etc. // Factory to get appropriate signer func GetOutboundSigner(cctx *crosschaintypes.CrossChainTx) OutboundSigner { // Return appropriate signer based on conditions }zetaclient/chains/bitcoin/signer/sign.go (1)
Line range hint
251-255: Concurrency Safety forIsTSSTransactionMethodThe
IsTSSTransactionmethod accesses thetssOutboundHashesmap but does not use synchronization mechanisms. Since maps are not thread-safe in Go, consider adding a read lock to prevent concurrent access issues.Apply this diff to add a read lock:
func (ob *Observer) IsTSSTransaction(txid string) bool { + ob.Mu().RLock() + defer ob.Mu().RUnlock() _, found := ob.tssOutboundHashes[txid] return found }zetaclient/chains/bitcoin/common/fee.go (1)
Add test coverage for GetRecentFeeRate function
The review comment is accurate. While CalcBlockAvgFeeRate has comprehensive test coverage, GetRecentFeeRate remains untested. Essential test cases should include:
- Network parameter validation (mainnet rejection)
- Block traversal and fee rate calculation across multiple blocks
- Error handling for RPC failures
- Default fee rate fallback behavior
- Edge cases with zero fee rates
Consider following the testing patterns established in
inbound_test.go, which demonstrates thorough error case coverage and proper test data management.🔗 Analysis chain
Line range hint
250-290: Add test coverage for fee rate calculation.The
GetRecentFeeRatefunction lacks test coverage according to static analysis.Consider adding test cases for:
- Different network parameters
- Various block fee rates
- Error scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find existing test files for fee rate calculation fd -e go -E '*_test.go' fee | xargs rg -l 'TestGetRecentFeeRate|Test_GetRecentFeeRate'Length of output: 88
Script:
#!/bin/bash # Find all fee-related test files fd -e go test fee # Search for CalcBlockAvgFeeRate tests and implementations rg -l "CalcBlockAvgFeeRate" --type go # Look for mock RPC implementations that could be used in testing rg -l "type.*RPC.*interface" --type go rg -l "Mock.*RPC" --type goLength of output: 917
Script:
#!/bin/bash # Check test content for CalcBlockAvgFeeRate rg -A 10 "CalcBlockAvgFeeRate" zetaclient/chains/bitcoin/observer/inbound_test.go zetaclient/chains/bitcoin/client/client_test.go # Look for Bitcoin-specific mock implementations rg -l "Mock.*Bitcoin.*RPC|Bitcoin.*Mock.*RPC" --type goLength of output: 8590
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 250-250: zetaclient/chains/bitcoin/common/fee.go#L250
Added line #L250 was not covered by tests
🧹 Nitpick comments (36)
zetaclient/testutils/testdata.go (1)
120-126: LGTM! Consider adding a brief comment about the return value.The implementation is correct and follows the established patterns. Consider adding a brief comment about the returned transaction object to maintain consistency with other functions in the file.
// LoadBTCTransaction loads archived Bitcoin transaction from file +// Returns the transaction details as GetTransactionResult func LoadBTCTransaction(t *testing.T, dir string, chainID int64, txHash string) *btcjson.GetTransactionResult {zetaclient/testutils/mocks/zetacore_client_opts.go (1)
37-46: Consider adding parameter documentation and type-specific matchers.While the implementation is correct, consider these improvements:
- Add documentation to describe the expected parameters and behavior
- Use type-specific matchers instead of
mock.Anythingwhere possible to catch type mismatches early in testsExample improvement:
+// WithPostOutboundTracker sets up expectations for PostOutboundTracker method +// Parameters: +// - param1: <type> - description +// - param2: <type> - description +// - param3: <type> - description +// - param4: <type> - description func (_m *ZetacoreClient) WithPostOutboundTracker(zetaTxHash string) *ZetacoreClient { - on := _m.On("PostOutboundTracker", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Maybe() + on := _m.On("PostOutboundTracker", + mock.MatchedBy(func(p interface{}) bool { return true }), // Replace with type-specific matcher + mock.MatchedBy(func(p interface{}) bool { return true }), // Replace with type-specific matcher + mock.MatchedBy(func(p interface{}) bool { return true }), // Replace with type-specific matcher + mock.MatchedBy(func(p interface{}) bool { return true }), // Replace with type-specific matcher + ).Maybe()zetaclient/orchestrator/orchestrator.go (3)
442-442: Error messages include contextual information.The error messages in
ScheduleCCTXEVMnow consistently include outbound ID and chain information, which aids in debugging. However, consider standardizing the error message format.Apply this pattern for error messages:
-Msgf("ScheduleCCTXEVM: GetAllOutboundTrackerByChain failed for chain %d", chainID) +Msgf("ScheduleCCTXEVM: failed to get outbound tracker: chain=%d", chainID)Also applies to: 464-464, 468-468, 478-478, 483-483, 511-511
541-541: Error messages in Solana handler follow similar pattern.The error messages in
ScheduleCCTXSolanamaintain consistency with the EVM handler. However, the same message format standardization should be applied here.Apply this pattern for error messages:
-Msgf("ScheduleCCTXSolana: chain observer is not a solana observer") +Msgf("ScheduleCCTXSolana: invalid observer type: expected=solana")Also applies to: 559-559, 563-563, 573-573, 578-578, 585-585
Line range hint
431-599: Consider extracting common scheduling logic.Both
ScheduleCCTXEVMandScheduleCCTXSolanashare similar patterns for:
- Parameter validation
- Outbound tracking
- Vote confirmation
- Keysign scheduling
This could be refactored into a common base implementation to reduce code duplication.
Consider creating a base scheduler that implements the common logic, with chain-specific implementations handling only the unique aspects of each chain.
zetaclient/chains/bitcoin/observer/inbound_test.go (1)
Line range hint
161-209: Consider adding more test cases for edge scenarios.The current test suite could benefit from additional test cases to cover:
- Extremely large transaction values
- Malformed Bitcoin addresses
- Maximum memo size scenarios
Example test case structure:
tests := []struct { name string event *observer.BTCInboundEvent nilVote bool }{ + { + name: "should return nil for extremely large value", + event: &observer.BTCInboundEvent{ + Value: math.MaxInt64, + MemoBytes: testutil.HexToBytes(t, "2d07a9cbd57dcca3e2cf966c88bc874445b6e3b668656c6c6f207361746f736869"), + }, + nilVote: true, + }, + { + name: "should return nil for malformed address", + event: &observer.BTCInboundEvent{ + FromAddress: "invalid_address", + MemoBytes: testutil.HexToBytes(t, "2d07a9cbd57dcca3e2cf966c88bc874445b6e3b668656c6c6f207361746f736869"), + }, + nilVote: true, + }, // existing test cases... }zetaclient/chains/bitcoin/observer/observer_test.go (2)
205-215: Consider adding edge cases to strengthen the test coverage.While the basic functionality is tested, consider adding these test cases:
- Maximum uint64 value
- Setting nonce multiple times
- Concurrent access scenarios
Here's a suggested enhancement:
func Test_SetPendingNonce(t *testing.T) { + t.Run("basic functionality", func(t *testing.T) { // create observer ob := newTestSuite(t, chains.BitcoinMainnet, "") // ensure pending nonce is 0 require.Zero(t, ob.GetPendingNonce()) // set and get pending nonce nonce := uint64(100) ob.SetPendingNonce(nonce) require.Equal(t, nonce, ob.GetPendingNonce()) + }) + + t.Run("edge cases", func(t *testing.T) { + ob := newTestSuite(t, chains.BitcoinMainnet, "") + + // Test max uint64 + maxNonce := uint64(^uint64(0)) + ob.SetPendingNonce(maxNonce) + require.Equal(t, maxNonce, ob.GetPendingNonce()) + + // Test multiple updates + ob.SetPendingNonce(1) + ob.SetPendingNonce(2) + require.Equal(t, uint64(2), ob.GetPendingNonce()) + }) }
288-295: Add validation for database path.The current implementation doesn't validate the provided dbPath. Consider adding checks for:
- Path existence when non-empty
- Path permissions
- Invalid characters in path
Here's a suggested enhancement:
if dbPath == "" { database, err = db.NewFromSqliteInMemory(true) } else { + // Validate database path + if _, err := os.Stat(dbPath); err != nil { + if os.IsNotExist(err) { + if err := os.MkdirAll(dbPath, 0755); err != nil { + return nil, fmt.Errorf("failed to create database directory: %w", err) + } + } else { + return nil, fmt.Errorf("invalid database path: %w", err) + } + } database, err = db.NewFromSqlite(dbPath, "test.db", true) }zetaclient/chains/bitcoin/observer/outbound_test.go (3)
Line range hint
13-13: Consider using a more robust test data path resolution.The hardcoded relative path to the testdata directory could break if the project structure changes. Consider using
go-testfixturesor a similar package to handle test data paths more robustly.-var TestDataDir = "../../../" +var TestDataDir = filepath.Join("testdata", "bitcoin", "outbound")
Line range hint
141-241: Refactor to table-driven tests for better maintainability.The TSS vout validation test cases contain repetitive setup code. Consider refactoring to table-driven tests to reduce duplication and make it easier to add new test cases.
func TestCheckTSSVout(t *testing.T) { chain := chains.BitcoinMainnet chainID := chain.ChainId nonce := uint64(148) ob := MockBTCObserverMainnet(t, nil) tests := []struct { name string modifyVout func(*btcjson.GetRawTransactionResult) wantErr bool errContains string }{ { name: "valid TSS vout should pass", modifyVout: func(r *btcjson.GetRawTransactionResult) {}, wantErr: false, }, { name: "should fail if vout 0 is not to the TSS address", modifyVout: func(r *btcjson.GetRawTransactionResult) { r.Vout[0].ScriptPubKey.Hex = "0014ba8be635673034d4d0ddc9447409b594385ec4aa" }, wantErr: true, errContains: "not match TSS address", }, // Add other test cases here } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { rawResult, cctx := testutils.LoadBTCTxRawResultNCctx(t, TestDataDir, chainID, nonce) tt.modifyVout(rawResult) params := cctx.GetCurrentOutboundParam() err := ob.checkTSSVout(params, rawResult.Vout) if tt.wantErr { require.ErrorContains(t, err, tt.errContains) } else { require.NoError(t, err) } }) } }
Line range hint
351-471: Extract common setup code to reduce duplication.The UTXO consolidation test cases contain repetitive setup code. Consider extracting the common setup into a helper function to improve maintainability.
func setupUTXOTest(t *testing.T, nonce uint64) *Observer { ob := createObserverWithUTXOs(t) dummyTxID := "6e6f71d281146c1fc5c755b35908ee449f26786c84e2ae18f98b268de40b7ec4" mineTxNSetNonceMark(t, ob, nonce, dummyTxID, -1) return ob }Then use it in the test cases:
t.Run("should not consolidate", func(t *testing.T) { ob := setupUTXOTest(t, 0) // Test logic here })zetaclient/chains/evm/signer/signer.go (2)
Line range hint
279-279: Address TODO comment regarding function simplification.The TODO comment indicates technical debt that should be addressed. Would you like me to create an issue to track this improvement?
Line range hint
141-241: Consider breaking downTryProcessOutboundinto smaller, focused functions.The function is handling multiple responsibilities including context management, transaction setup, signing, and broadcasting. This makes it harder to test and maintain.
Consider extracting the following responsibilities into separate functions:
- Transaction setup and validation
- Error handling and recovery
- Broadcasting logic
zetaclient/chains/bitcoin/signer/signer.go (2)
168-168: Typographical error in commentThere's a minor typo in the comment. "broacasting" should be corrected to "broadcasting" for clarity.
Apply this diff to fix the typo:
- // try broacasting tx with increasing backoff (1s, 2s, 4s, 8s, 16s) in case of RPC error + // try broadcasting tx with increasing backoff (1s, 2s, 4s, 8s, 16s) in case of RPC error
168-178: Consider using a dedicated backoff library for improved retry logicWhile the current exponential backoff implementation using
time.Sleepfunctions correctly, utilizing a dedicated backoff library like go-retryablehttp can offer more features such as jitter, maximum elapsed time, and better control over retry logic.zetaclient/chains/bitcoin/observer/utxos.go (1)
32-36: Avoid usingrecoverto handle panics; use proper error handling insteadRelying on
recoverto catch panics can obscure the root causes of errors and make debugging more difficult. It's better to prevent panics by ensuring errors are properly checked and handled throughout the code.zetaclient/chains/bitcoin/signer/sign.go (2)
227-229: Error Wrapping Enhancements for ClarityThe error message in
fmt.Errorf("SignBatch failed: %v", err)could be enhanced usingerrors.Wrapto preserve the stack trace and provide more context.Apply this diff to use
errors.Wrap:- return fmt.Errorf("SignBatch failed: %v", err) + return errors.Wrap(err, "SignBatch failed")Ensure that the
errorspackage is consistently used for error wrapping throughout the codebase.
136-138: Simplify Return StatementThe return statement in
AddTxInputscan be simplified by directly returning theamountsslice after the loop.Apply this diff to simplify the code:
- } - - return amounts, nil + } + return amounts, nil }zetaclient/chains/evm/rpc/rpc_live_test.go (1)
65-73: Consider enhancing gas price validation.While the basic validation ensures non-zero gas price, consider adding upper bound checks to catch unreasonable values that might indicate network issues.
func LiveTest_SuggestGasPrice(t *testing.T) { client, err := ethclient.Dial(URLBaseMainnet) require.NoError(t, err) ctx := context.Background() gasPrice, err := client.SuggestGasPrice(ctx) require.NoError(t, err) require.True(t, gasPrice.Cmp(big.NewInt(0)) > 0) + // Add reasonable upper bound check (e.g., 1000 gwei) + maxGasPrice := new(big.Int).Mul(big.NewInt(1000), big.NewInt(1e9)) + require.True(t, gasPrice.Cmp(maxGasPrice) < 0, "Gas price exceeds reasonable maximum") }zetaclient/chains/bitcoin/observer/gas_price.go (1)
56-70: Consider using constants for network types.The switch statement could be more maintainable using constants for network types. Also, consider adding a default case to handle future network types.
func (ob *Observer) specialHandleFeeRate(ctx context.Context) (int64, error) { + const ( + errUnsupportedNetwork = "unsupported bitcoin network type %d" + ) switch ob.Chain().NetworkType { case chains.NetworkType_privnet: return client.FeeRateRegnet, nil case chains.NetworkType_testnet: feeRateEstimated, err := common.GetRecentFeeRate(ctx, ob.rpc, ob.netParams) if err != nil { return 0, errors.Wrapf(err, "error GetRecentFeeRate") } return feeRateEstimated, nil default: - return 0, fmt.Errorf(" unsupported bitcoin network type %d", ob.Chain().NetworkType) + return 0, fmt.Errorf(errUnsupportedNetwork, ob.Chain().NetworkType) } }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 57-68: zetaclient/chains/bitcoin/observer/gas_price.go#L57-L68
Added lines #L57 - L68 were not covered by testszetaclient/chains/bitcoin/observer/db.go (1)
57-69: Consider using transactions for map updates.The function updates two maps independently. Consider using a transaction-like pattern to ensure atomic updates or reverting both if one fails.
func (ob *Observer) LoadBroadcastedTxMap() error { var broadcastedTransactions []clienttypes.OutboundHashSQLType if err := ob.DB().Client().Find(&broadcastedTransactions).Error; err != nil { ob.logger.Chain.Error().Err(err).Msgf("error iterating over db for chain %d", ob.Chain().ChainId) return err } + tempOutboundHashes := make(map[string]bool) + tempBroadcastedTx := make(map[string]string) for _, entry := range broadcastedTransactions { - ob.tssOutboundHashes[entry.Hash] = true - ob.broadcastedTx[entry.Key] = entry.Hash + tempOutboundHashes[entry.Hash] = true + tempBroadcastedTx[entry.Key] = entry.Hash } + ob.Mu().Lock() + ob.tssOutboundHashes = tempOutboundHashes + ob.broadcastedTx = tempBroadcastedTx + ob.Mu().Unlock() return nil }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 61-63: zetaclient/chains/bitcoin/observer/db.go#L61-L63
Added lines #L61 - L63 were not covered by testszetaclient/chains/bitcoin/client/mockgen.go (1)
48-48: Add method documentation.Consider adding a comment describing the purpose, parameters, and return values of the
GetEstimatedFeeRatemethod to maintain consistency with other interface methods.+// GetEstimatedFeeRate returns the estimated fee rate in satoshis/vbyte for a transaction +// to be confirmed within confTarget blocks. If regnet is true, returns a hardcoded fee rate. GetEstimatedFeeRate(ctx context.Context, confTarget int64, regnet bool) (int64, error)zetaclient/zetacore/broadcast_test.go (1)
34-34: Consider adding boundary test cases.While the error handling test cases are comprehensive, consider adding boundary test cases for the numeric parameters:
- Maximum uint64 value for nonce
- Negative values for chain ID
- Zero values for both parameters
zetaclient/chains/bitcoin/observer/db_test.go (2)
47-48: Replace magic number with a named constant.The block number 199 appears to be arbitrary. Consider defining it as a named constant to improve test readability and maintainability.
+const testLastBlockScanned = 199 -ob.WriteLastBlockScannedToDB(199) +ob.WriteLastBlockScannedToDB(testLastBlockScanned)
76-77: Add test case for partial RPC failure.The RPC error test only covers complete failure. Consider adding a test case where the RPC call succeeds but returns an unexpected block number.
zetaclient/chains/bitcoin/signer/sign_test.go (2)
93-257: Consider adding test cases for edge cases in fee calculation.While the test coverage is good, consider adding test cases for:
- Maximum possible fee scenarios
- Minimum fee threshold scenarios
- Fee calculation with dust amounts
259-326: Add test cases for different signature types.The current test only verifies the presence of witness data. Consider adding test cases for:
- Different signature types (P2PKH, P2SH, P2WPKH, P2WSH)
- Invalid signature scenarios
- Signature verification
zetaclient/chains/bitcoin/observer/event_test.go (1)
309-309: Consider adding test cases for empty observer chain ID.The empty string parameter in
newTestSuitecalls suggests a potential edge case. Consider adding test cases for invalid chain ID scenarios.Also applies to: 357-357, 397-397
zetaclient/chains/bitcoin/signer/signer_test.go (2)
97-170: Enhance RBF test coverage.While the test cases cover basic RBF outbound scenarios, consider adding explicit assertions for:
- RBF-specific attributes (sequence numbers)
- Fee rate comparisons between original and replacement transactions
- Multiple replacement iterations
Example test case structure:
{ name: "should successfully replace transaction with higher fee rate", chain: chains.BitcoinMainnet, nonce: uint64(148), feeRate: 2.0, // multiplier for replacement attempts: 3, // number of replacement iterations }
337-354: Parameterize context creation helper.The function uses hardcoded values which limits its reusability. Consider accepting parameters for:
- Chain selection
- Chain parameters
-func makeCtx(t *testing.T) context.Context { +func makeCtx(t *testing.T, chain chains.Chain, params int64) context.Context { app := zctx.New(config.New(false), nil, zerolog.Nop()) - chain := chains.BitcoinMainnet - btcParams := mocks.MockChainParams(chain.ChainId, 2) + btcParams := mocks.MockChainParams(chain.ChainId, params)changelog.md (1)
10-10: Consider expanding the changelog description.The current description "some code refactoring in zetaclient for Bitcoin RBF adoption" could be more specific about the key refactoring changes, such as the relocation of Bitcoin observer methods and the creation of reusable methods.
Consider this more detailed description:
-* [3381](https://github.com/zeta-chain/node/pull/3381) - some code refactoring in zetaclient for Bitcoin RBF adoption +* [3381](https://github.com/zeta-chain/node/pull/3381) - refactor Bitcoin observer methods into separate files (db.go, gas_price.go, utxos.go), add reusable methods for Bitcoin signing (AddTxInputs, SignTx), and introduce OutboundData struct for Bitcoin RBF adoptionzetaclient/chains/bitcoin/observer/observer.go (3)
107-109: Consolidate redundant comments fortssOutboundHashesThe variable
tssOutboundHasheshas two comments that convey similar information. Merging them into a single, concise comment will enhance code readability and maintainability.Apply this diff to consolidate the comments:
- // tssOutboundHashes indexes included tx with tx hash - // tssOutboundHashes keeps track of outbound hashes sent from TSS address + // tssOutboundHashes keeps track of outbound transaction hashes sent from the TSS address
198-203: Evaluate the use of atomic operations forpendingNonceThe
SetPendingNoncemethod uses mutex locks to synchronize access topendingNonce. AspendingNonceis of typeuint64, utilizing atomic operations (e.g.,atomic.LoadUint64,atomic.StoreUint64) could improve performance by reducing locking overhead while maintaining thread safety. Consider adopting atomic operations if they align with the synchronization requirements of your application.
268-269: Add unit tests forisNodeEnabledmethodThe
isNodeEnabledmethod lacks test coverage, as indicated by static analysis tools. Implementing unit tests for this method will enhance code reliability and ensure that node enablement checks function as expected.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 268-269: zetaclient/chains/bitcoin/observer/observer.go#L268-L269
Added lines #L268 - L269 were not covered by testszetaclient/chains/bitcoin/observer/outbound.go (2)
53-53: Use formal language in log messagesThe log message contains informal language ("oops"), which is not suitable for production environments. Rewriting the message with formal language enhances professionalism and clarity in the logs.
Apply this diff to update the log message:
- ob.logger.Outbound.Warn().Msgf("oops, got multiple (%d) outbound hashes", len(tracker.HashList)) + ob.logger.Outbound.Warn().Msgf("Detected multiple (%d) outbound hashes", len(tracker.HashList))🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 53-53: zetaclient/chains/bitcoin/observer/outbound.go#L53
Added line #L53 was not covered by tests
88-97: Add unit tests forTryIncludeOutboundmethodThe
TryIncludeOutboundmethod plays a critical role in the outbound transaction flow but is not covered by unit tests. Implementing tests will verify its behavior under various conditions, such as successful inclusion, failures, and edge cases, thereby enhancing code reliability.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 88-95: zetaclient/chains/bitcoin/observer/outbound.go#L88-L95
Added lines #L88 - L95 were not covered by tests
[warning] 97-97: zetaclient/chains/bitcoin/observer/outbound.go#L97
Added line #L97 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
changelog.md(1 hunks)testutil/sample/crypto.go(2 hunks)x/crosschain/types/cctx_test.go(1 hunks)x/crosschain/types/revert_options_test.go(1 hunks)zetaclient/chains/bitcoin/client/client_test.go(2 hunks)zetaclient/chains/bitcoin/client/helpers.go(3 hunks)zetaclient/chains/bitcoin/client/mockgen.go(1 hunks)zetaclient/chains/bitcoin/common/fee.go(7 hunks)zetaclient/chains/bitcoin/common/fee_test.go(11 hunks)zetaclient/chains/bitcoin/observer/db.go(1 hunks)zetaclient/chains/bitcoin/observer/db_test.go(1 hunks)zetaclient/chains/bitcoin/observer/event_test.go(5 hunks)zetaclient/chains/bitcoin/observer/gas_price.go(1 hunks)zetaclient/chains/bitcoin/observer/inbound_test.go(2 hunks)zetaclient/chains/bitcoin/observer/observer.go(5 hunks)zetaclient/chains/bitcoin/observer/observer_test.go(5 hunks)zetaclient/chains/bitcoin/observer/outbound.go(6 hunks)zetaclient/chains/bitcoin/observer/outbound_test.go(7 hunks)zetaclient/chains/bitcoin/observer/utxos.go(1 hunks)zetaclient/chains/bitcoin/signer/outbound_data.go(1 hunks)zetaclient/chains/bitcoin/signer/outbound_data_test.go(1 hunks)zetaclient/chains/bitcoin/signer/sign.go(1 hunks)zetaclient/chains/bitcoin/signer/sign_test.go(1 hunks)zetaclient/chains/bitcoin/signer/signer.go(4 hunks)zetaclient/chains/bitcoin/signer/signer_test.go(10 hunks)zetaclient/chains/evm/observer/observer_test.go(0 hunks)zetaclient/chains/evm/rpc/rpc_live_test.go(4 hunks)zetaclient/chains/evm/signer/signer.go(1 hunks)zetaclient/logs/fields.go(1 hunks)zetaclient/orchestrator/orchestrator.go(10 hunks)zetaclient/orchestrator/v2_bootstrap.go(0 hunks)zetaclient/testdata/btc/chain_8332_msgtx_030cd813443f7b70cc6d8a544d320c6d8465e4528fc0f3410b599dc0b26753a0.json(1 hunks)zetaclient/testdata/btc/chain_8332_tx_030cd813443f7b70cc6d8a544d320c6d8465e4528fc0f3410b599dc0b26753a0.json(1 hunks)zetaclient/testutils/mocks/bitcoin_client.go(1 hunks)zetaclient/testutils/mocks/zetacore_client_opts.go(1 hunks)zetaclient/testutils/testdata.go(1 hunks)zetaclient/testutils/testdata_naming.go(1 hunks)zetaclient/zetacore/broadcast.go(2 hunks)zetaclient/zetacore/broadcast_test.go(1 hunks)
💤 Files with no reviewable changes (2)
- zetaclient/orchestrator/v2_bootstrap.go
- zetaclient/chains/evm/observer/observer_test.go
✅ Files skipped from review due to trivial changes (2)
- zetaclient/testdata/btc/chain_8332_tx_030cd813443f7b70cc6d8a544d320c6d8465e4528fc0f3410b599dc0b26753a0.json
- zetaclient/testdata/btc/chain_8332_msgtx_030cd813443f7b70cc6d8a544d320c6d8465e4528fc0f3410b599dc0b26753a0.json
🧰 Additional context used
📓 Path-based instructions (34)
zetaclient/logs/fields.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/testutils/testdata.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/client/client_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/testutils/mocks/bitcoin_client.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/revert_options_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/cctx_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/signer/signer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/client/mockgen.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/testutils/testdata_naming.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/zetacore/broadcast_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
testutil/sample/crypto.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/testutils/mocks/zetacore_client_opts.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/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/db_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/signer/outbound_data_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/gas_price.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/client/helpers.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_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/signer/outbound_data.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/signer/signer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/zetacore/broadcast.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/utxos.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/orchestrator/orchestrator.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/signer/sign.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.
zetaclient/chains/bitcoin/common/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/outbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/common/fee_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/signer/sign_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/db.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_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
📓 Learnings (2)
zetaclient/chains/bitcoin/observer/event_test.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#2987
File: pkg/memo/fields_v0_test.go:270-320
Timestamp: 2024-11-12T13:20:12.658Z
Learning: In the `FieldsV0` struct of the `memo` package, `RevertAddress` may be left empty when `CallOnRevert` is set to true.
zetaclient/chains/bitcoin/observer/db.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#3306
File: zetaclient/chains/bitcoin/observer/db.go:11-25
Timestamp: 2025-01-14T00:09:01.416Z
Learning: In Bitcoin observer, non-critical DB operations (like saving broadcasted transactions) should log errors but not return them to callers, as the errors are already handled appropriately through logging and don't affect the critical in-memory state.
🪛 GitHub Check: codecov/patch
zetaclient/chains/bitcoin/observer/gas_price.go
[warning] 15-34: zetaclient/chains/bitcoin/observer/gas_price.go#L15-L34
Added lines #L15 - L34 were not covered by tests
[warning] 38-41: zetaclient/chains/bitcoin/observer/gas_price.go#L38-L41
Added lines #L38 - L41 were not covered by tests
[warning] 44-51: zetaclient/chains/bitcoin/observer/gas_price.go#L44-L51
Added lines #L44 - L51 were not covered by tests
[warning] 53-53: zetaclient/chains/bitcoin/observer/gas_price.go#L53
Added line #L53 was not covered by tests
[warning] 57-68: zetaclient/chains/bitcoin/observer/gas_price.go#L57-L68
Added lines #L57 - L68 were not covered by tests
zetaclient/chains/bitcoin/client/helpers.go
[warning] 120-123: zetaclient/chains/bitcoin/client/helpers.go#L120-L123
Added lines #L120 - L123 were not covered by tests
[warning] 127-131: zetaclient/chains/bitcoin/client/helpers.go#L127-L131
Added lines #L127 - L131 were not covered by tests
[warning] 133-145: zetaclient/chains/bitcoin/client/helpers.go#L133-L145
Added lines #L133 - L145 were not covered by tests
[warning] 147-147: zetaclient/chains/bitcoin/client/helpers.go#L147
Added line #L147 was not covered by tests
zetaclient/chains/bitcoin/observer/observer.go
[warning] 268-269: zetaclient/chains/bitcoin/observer/observer.go#L268-L269
Added lines #L268 - L269 were not covered by tests
zetaclient/chains/bitcoin/common/fee.go
[warning] 250-250: zetaclient/chains/bitcoin/common/fee.go#L250
Added line #L250 was not covered by tests
[warning] 290-290: zetaclient/chains/bitcoin/common/fee.go#L290
Added line #L290 was not covered by tests
zetaclient/chains/bitcoin/observer/outbound.go
[warning] 38-41: zetaclient/chains/bitcoin/observer/outbound.go#L38-L41
Added lines #L38 - L41 were not covered by tests
[warning] 43-46: zetaclient/chains/bitcoin/observer/outbound.go#L43-L46
Added lines #L43 - L46 were not covered by tests
[warning] 49-50: zetaclient/chains/bitcoin/observer/outbound.go#L49-L50
Added lines #L49 - L50 were not covered by tests
[warning] 53-53: zetaclient/chains/bitcoin/observer/outbound.go#L53
Added line #L53 was not covered by tests
[warning] 64-66: zetaclient/chains/bitcoin/observer/outbound.go#L64-L66
Added lines #L64 - L66 were not covered by tests
[warning] 88-95: zetaclient/chains/bitcoin/observer/outbound.go#L88-L95
Added lines #L88 - L95 were not covered by tests
[warning] 97-97: zetaclient/chains/bitcoin/observer/outbound.go#L97
Added line #L97 was not covered by tests
[warning] 101-101: zetaclient/chains/bitcoin/observer/outbound.go#L101
Added line #L101 was not covered by tests
[warning] 124-126: zetaclient/chains/bitcoin/observer/outbound.go#L124-L126
Added lines #L124 - L126 were not covered by tests
[warning] 143-144: zetaclient/chains/bitcoin/observer/outbound.go#L143-L144
Added lines #L143 - L144 were not covered by tests
[warning] 232-232: zetaclient/chains/bitcoin/observer/outbound.go#L232
Added line #L232 was not covered by tests
[warning] 234-234: zetaclient/chains/bitcoin/observer/outbound.go#L234
Added line #L234 was not covered by tests
[warning] 240-240: zetaclient/chains/bitcoin/observer/outbound.go#L240
Added line #L240 was not covered by tests
[warning] 242-242: zetaclient/chains/bitcoin/observer/outbound.go#L242
Added line #L242 was not covered by tests
[warning] 258-258: zetaclient/chains/bitcoin/observer/outbound.go#L258
Added line #L258 was not covered by tests
[warning] 262-262: zetaclient/chains/bitcoin/observer/outbound.go#L262
Added line #L262 was not covered by tests
zetaclient/chains/bitcoin/observer/db.go
[warning] 22-25: zetaclient/chains/bitcoin/observer/db.go#L22-L25
Added lines #L22 - L25 were not covered by tests
[warning] 61-63: zetaclient/chains/bitcoin/observer/db.go#L61-L63
Added lines #L61 - L63 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: start-e2e-test / e2e
🔇 Additional comments (33)
zetaclient/testutils/testdata_naming.go (1)
67-70: LGTM! The function maintains consistency with existing patterns.The implementation follows the established naming conventions and maintains consistency with other Bitcoin-related filename functions in the codebase.
zetaclient/testutils/mocks/zetacore_client_opts.go (1)
37-46: Well-integrated with existing codebase patterns!The new method follows the established builder pattern and error handling conventions consistently used throughout the file.
zetaclient/orchestrator/orchestrator.go (2)
411-411: Method naming convention updated for consistency.The renaming of methods to use "CCTX" instead of "Cctx" improves consistency with the codebase's naming conventions.
Also applies to: 416-416
431-432: Function signatures follow consistent naming pattern.The renamed functions
ScheduleCCTXEVMandScheduleCCTXSolanamaintain consistent parameter ordering and types, which is good for maintainability.Also applies to: 530-531
zetaclient/chains/bitcoin/observer/inbound_test.go (2)
158-158: LGTM! Test suite initialization updated correctly.The addition of the empty string parameter for dbPath aligns with the updated test suite initialization pattern.
170-170: LGTM! Explicit string conversion improves type safety.The addition of
.String()ensures consistent string representation of Bitcoin addresses in test data.zetaclient/chains/bitcoin/observer/observer_test.go (3)
16-16: LGTM!The addition of the testutils import is appropriate for accessing test constants.
168-168: LGTM!Using an in-memory database for these tests is the correct approach as it ensures test isolation and performance.
Also applies to: 192-192
281-286: LGTM!The TSS signer initialization based on chain type is well-structured and uses appropriate test constants.
zetaclient/chains/evm/signer/signer.go (2)
522-523: LGTM! Improved type safety in error handling.The direct usage of numeric types instead of string conversions enhances type safety and reduces unnecessary conversions.
Line range hint
92-117: Review and implement EIP-1559 support.The commented code in
newTxsuggests incomplete implementation of EIP-1559 (dynamic fee transactions). This should be either implemented or the commented code should be removed.zetaclient/chains/bitcoin/signer/outbound_data.go (1)
103-104: Ensure addition ofminRelayFeedoes not cause an overflowWhen adding
minRelayFeetofeeRate, there's a potential risk of integer overflow if the values are large. Consider implementing a check to ensure that the addition does not exceed the maximum value ofint64.zetaclient/chains/bitcoin/signer/signer.go (1)
75-82: NewTSSToPkScriptmethod correctly generates the TSS public key scriptThe implementation of
TSSToPkScriptappropriately encapsulates the generation of the pay-to-address script for the TSS public key. This enhances code modularity and reusability.zetaclient/chains/bitcoin/signer/sign.go (1)
215-220:⚠️ Potential issueValidate Previous Output Fetcher Parameters
In
SignTx, thetxscript.NewTxSigHashesfunction is called with aCannedPrevOutputFetcherinitialized with an empty script and a zero amount. Ensure that the previous output fetcher is correctly initialized with the actual previous outputs' scripts and amounts to generate accurate signature hashes.Please verify that the previous outputs are correctly fetched and used in the signature hash calculation. If necessary, update the
CannedPrevOutputFetcherinitialization.zetaclient/logs/fields.go (1)
12-12: LGTM! Consistent constant definition.The new
FieldOutboundIDconstant follows the established naming pattern and maintains consistency with other logging field keys.zetaclient/chains/evm/rpc/rpc_live_test.go (1)
21-21: LGTM! Consistent URL constant definition.The
URLBaseMainnetconstant follows the established naming pattern and URL structure.zetaclient/chains/bitcoin/observer/db.go (1)
12-27: LGTM! Follows established error handling pattern.The implementation correctly follows the learning that non-critical DB operations should log errors but not return them.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 22-25: zetaclient/chains/bitcoin/observer/db.go#L22-L25
Added lines #L22 - L25 were not covered by testsx/crosschain/types/revert_options_test.go (1)
52-52: LGTM! Type-safe address handling.The explicit conversion of Bitcoin address to string format ensures type safety and consistent address handling across the codebase.
zetaclient/zetacore/broadcast.go (1)
162-172: LGTM! Type safety improvements.The changes enhance type safety by using more appropriate types (
uint64fornonceandint64fortoChain) and update the logging statements accordingly.zetaclient/chains/bitcoin/client/helpers.go (2)
15-24: LGTM! Well-defined constants.The constants are appropriately named, documented, and have correct values for Bitcoin fee rate handling.
126-148: Add unit tests and improve error handling.The function logic is correct, but consider these improvements:
- Add unit tests to verify fee rate estimation for various scenarios.
- Consider consolidating error messages into constants or using a custom error type for better error handling.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 127-131: zetaclient/chains/bitcoin/client/helpers.go#L127-L131
Added lines #L127 - L131 were not covered by tests
[warning] 133-145: zetaclient/chains/bitcoin/client/helpers.go#L133-L145
Added lines #L133 - L145 were not covered by tests
[warning] 147-147: zetaclient/chains/bitcoin/client/helpers.go#L147
Added line #L147 was not covered by teststestutil/sample/crypto.go (2)
95-104: LGTM! Improved return type.The function now returns a more appropriate type (
*btcutil.AddressWitnessPubKeyHash) instead of a string representation.
106-111: LGTM! Well-structured helper function.The function provides a convenient way to generate payment scripts for test addresses.
zetaclient/chains/bitcoin/signer/outbound_data_test.go (1)
18-241: LGTM! Comprehensive test coverage.The test suite is well-structured with:
- Clear test case descriptions
- Good coverage of edge cases and error scenarios
- Proper validation of expected outputs
- Effective use of test helpers and sample data
zetaclient/chains/bitcoin/signer/sign_test.go (1)
24-91: LGTM! Comprehensive test cases for transaction input handling.The test cases effectively cover:
- Success scenario with multiple inputs
- Failure on invalid transaction ID
- Failure on invalid amount
x/crosschain/types/cctx_test.go (1)
143-143: LGTM! Consistent string representation of Bitcoin addresses.The change ensures type safety by explicitly converting the Bitcoin address to its string representation.
zetaclient/chains/bitcoin/common/fee.go (1)
22-32: LGTM! Well-documented constants with clear purpose.The constants are well-organized and properly documented with their respective byte sizes and purposes.
zetaclient/chains/bitcoin/observer/event_test.go (1)
35-35: LGTM! Consistent address type handling.The changes ensure consistent string representation of Bitcoin addresses across test cases.
Also applies to: 252-252, 403-403
zetaclient/chains/bitcoin/client/client_test.go (1)
140-140: LGTM!The changes correctly update the test assertions to:
- Include depositor fee in transaction value validation
- Use a more focused block range for fee rate testing
Also applies to: 335-335
zetaclient/chains/bitcoin/common/fee_test.go (1)
186-189: LGTM!The changes successfully:
- Add test coverage for zero UTXO scenarios
- Update type signatures from uint64 to int64 consistently
- Maintain comprehensive test coverage for all address types
Also applies to: 201-203, 225-227, 249-253, 270-325
zetaclient/testutils/mocks/bitcoin_client.go (1)
360-386: LGTM!The auto-generated mock implementation for
GetEstimatedFeeRatecorrectly follows the established patterns for:
- Parameter handling
- Return value management
- Error scenarios
zetaclient/chains/bitcoin/observer/observer.go (1)
251-255: FunctionIsTSSTransactionimplementation is optimalThe
IsTSSTransactionmethod efficiently checks whether a transaction was sent by the TSS address by looking up the transaction ID in thetssOutboundHashesmap. The implementation is clear and adheres to best practices.zetaclient/chains/bitcoin/observer/outbound.go (1)
24-28:⚠️ Potential issueSet
minTxConfirmationsto a secure value for productionThe constant
minTxConfirmationsis currently set to0, allowing transactions with zero confirmations to be considered valid. This poses a significant security risk in a production environment, as unconfirmed transactions are vulnerable to double-spending attacks. It is advisable to setminTxConfirmationsto a safer value, such as1or higher, to ensure transactions are confirmed on the blockchain before being accepted.Apply this diff to set a secure confirmation threshold:
- minTxConfirmations = 0 + minTxConfirmations = 1⛔ Skipped due to learnings
Learnt from: ws4charlie PR: zeta-chain/node#3306 File: zetaclient/chains/bitcoin/observer/outbound.go:28-32 Timestamp: 2025-01-13T23:51:09.334Z Learning: In Bitcoin observer, `minTxConfirmations` is set to 0 to allow observing mempool transactions without waiting for block confirmations, which is necessary due to Bitcoin's 10-minute block time. This is a deliberate design choice despite potential security implications, as other safety checks are in place through TSS validation.
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
zetaclient/chains/bitcoin/common/fee.go (1)
Add test coverage for fee rate calculation.
The function
GetRecentFeeRatelacks test coverage for critical paths:
- Mainnet rejection logic
- Block count error handling
- Fee rate calculations from recent blocks
- Default testnet fee rate fallback
Please add unit tests to verify these scenarios as originally suggested.
🔗 Analysis chain
Line range hint
250-290: Add test coverage for fee rate calculation.This function lacks test coverage which is critical as it handles fee rate calculations. Please add unit tests to verify:
- Mainnet rejection
- Block count error handling
- Fee rate calculation for various scenarios
- Default testnet fee rate fallback
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test coverage for GetRecentFeeRate function ast-grep --pattern 'func TestGetRecentFeeRate'Length of output: 48
Script:
#!/bin/bash # Find test files in the bitcoin/common directory fd ".*_test.go$" zetaclient/chains/bitcoin/common # Search for any fee-related test functions rg "func Test.*Fee.*" zetaclient/chains/bitcoin/common -A 5 # Search for any usage of GetRecentFeeRate in test files rg "GetRecentFeeRate" zetaclient/chains/bitcoin/common --type goLength of output: 630
Script:
#!/bin/bash # Examine the content of fee_test.go cat zetaclient/chains/bitcoin/common/fee_test.goLength of output: 18019
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 250-250: zetaclient/chains/bitcoin/common/fee.go#L250
Added line #L250 was not covered by tests
🧹 Nitpick comments (31)
zetaclient/chains/bitcoin/signer/signer_test.go (4)
1-34: Consider grouping imports for better organization.Consider organizing imports into logical groups:
- Standard library
- External dependencies
- Internal packages
import ( + // Standard library "context" "encoding/hex" "testing" + // External dependencies "github.com/btcsuite/btcd/btcec/v2" "github.com/btcsuite/btcd/btcec/v2/ecdsa" "github.com/btcsuite/btcd/btcjson" "github.com/btcsuite/btcd/btcutil" "github.com/btcsuite/btcd/chaincfg" "github.com/btcsuite/btcd/chaincfg/chainhash" "github.com/btcsuite/btcd/txscript" "github.com/btcsuite/btcd/wire" "github.com/ethereum/go-ethereum/crypto" "github.com/rs/zerolog" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + // Internal packages "github.com/zeta-chain/node/pkg/chains" crosschaintypes "github.com/zeta-chain/node/x/crosschain/types" observertypes "github.com/zeta-chain/node/x/observer/types" "github.com/zeta-chain/node/zetaclient/chains/base" "github.com/zeta-chain/node/zetaclient/chains/bitcoin/observer" "github.com/zeta-chain/node/zetaclient/chains/bitcoin/signer" "github.com/zeta-chain/node/zetaclient/config" zctx "github.com/zeta-chain/node/zetaclient/context" "github.com/zeta-chain/node/zetaclient/db" "github.com/zeta-chain/node/zetaclient/keys" "github.com/zeta-chain/node/zetaclient/metrics" "github.com/zeta-chain/node/zetaclient/outboundprocessor" "github.com/zeta-chain/node/zetaclient/testutils" "github.com/zeta-chain/node/zetaclient/testutils/mocks" )
37-38: Consider using a more robust way to locate test data.Using a relative path for test data directory could be fragile if the test file is moved. Consider using
go:embedorfilepath.Join()with a base directory determined at runtime.
Line range hint
173-238: Consider extracting test constants for better maintainability.The hardcoded private key and test values in P2PH and P2WPH tests should be moved to test constants or a separate test data file.
+const ( + testPrivKeyHex = "22a47fa09a223f2aa079edf85a7c2d4f8720ee63e502ee2869afab7de234b80c" + testAmount = 100000000 // 1 BTC in satoshis +)Also applies to: 241-334
337-398: Enhance helper function documentation.While the helper functions are well-implemented, consider adding more detailed documentation explaining:
- Expected inputs and outputs
- Any side effects
- Usage examples
zetaclient/chains/bitcoin/observer/observer_test.go (2)
205-215: Enhance test coverage with additional edge cases.Consider adding test cases for:
- Setting maximum uint64 value
- Setting to zero after non-zero value
- Concurrent access if the nonce is accessed by multiple goroutines
298-300: Enhance test logger configuration.The current logger setup could be improved to:
- Set a consistent log level for tests
- Configure sampling for high-volume test logs
-testLogger := zerolog.New(zerolog.NewTestWriter(t)) +testLogger := zerolog.New(zerolog.NewTestWriter(t)). + Level(zerolog.InfoLevel). + Sample(&zerolog.BasicSampler{N: 10}) logger := base.Logger{Std: testLogger, Compliance: testLogger}zetaclient/zetacore/broadcast_test.go (1)
34-34: Consider using meaningful test values and adding edge cases.While the test correctly uses the new parameter types, consider:
- Using meaningful values that reflect real scenarios (e.g., actual nonce values)
- Adding edge cases like MAX_UINT64 for nonce and negative chain IDs
- retry, report := HandleBroadcastError(input, 100, 1, "") + testCases := []struct { + name string + err error + nonce uint64 + toChain int64 + outboundHash string + wantRetry bool + wantReport bool + }{ + { + name: "nonce too low with typical value", + err: errors.New("nonce too low"), + nonce: 1000, + toChain: 1, + wantRetry: false, + wantReport: false, + }, + { + name: "edge case - max nonce", + err: errors.New("nonce too low"), + nonce: ^uint64(0), + toChain: -1, + wantRetry: false, + wantReport: false, + }, + }zetaclient/zetacore/broadcast.go (1)
162-162: Enhance function documentation with return value descriptions.The function comment should explain the meaning of the boolean return values and when they are set.
-// HandleBroadcastError returns whether to retry in a few seconds, and whether to report via AddOutboundTracker -// returns (bool retry, bool report) +// HandleBroadcastError handles broadcast errors and determines the next action. +// Parameters: +// - err: The error encountered during broadcast +// - nonce: The transaction nonce (sequence number) +// - toChain: The destination chain ID +// - outboundHash: The hash of the outbound transaction +// Returns: +// - retry: true if the broadcast should be retried +// - report: true if the error should be reported via AddOutboundTrackerzetaclient/chains/bitcoin/signer/outbound_data.go (3)
21-49: Consider grouping related fields and adding validation tags.The struct fields could be organized into logical groups using comments, and validation tags could be added for better data integrity:
type OutboundData struct { + // Transaction identifiers chainID int64 + nonce uint64 + height uint64 + // Transaction details to btcutil.Address amount float64 `validate:"gte=0"` feeRate int64 `validate:"gt=0"` txSize int64 `validate:"gt=0"` + // Internal state amountSats int64 cancelTx bool - nonce uint64 - height uint64 }
51-137: Extract validation logic into separate functions.The constructor handles multiple responsibilities. Consider extracting validation logic into separate functions for better maintainability:
+func validateInputs(cctx *types.CrossChainTx) error { + if cctx == nil { + return errors.New("cctx is nil") + } + if cctx.InboundParams.CoinType != coin.CoinType_Gas { + return errors.New("can only send gas token to a Bitcoin network") + } + return nil +} +func validateFeeRate(params *types.OutboundTxParams) (int64, error) { + feeRate, err := strconv.ParseInt(params.GasPrice, 10, 64) + if err != nil || feeRate <= 0 { + return 0, fmt.Errorf("invalid fee rate %s", params.GasPrice) + } + return feeRate, nil +} func NewOutboundData(...) (*OutboundData, error) { - if cctx == nil { - return nil, errors.New("cctx is nil") - } + if err := validateInputs(cctx); err != nil { + return nil, err + } params := cctx.GetCurrentOutboundParam() - if cctx.InboundParams.CoinType != coin.CoinType_Gas { - return nil, errors.New("can only send gas token to a Bitcoin network") - } - - feeRate, err := strconv.ParseInt(params.GasPrice, 10, 64) - if err != nil || feeRate <= 0 { - return nil, fmt.Errorf("invalid fee rate %s", params.GasPrice) - } + feeRate, err := validateFeeRate(params) + if err != nil { + return nil, err + }
92-93: Consider using btcutil.Amount for Bitcoin amount conversions.The manual conversion to BTC and satoshis could be replaced with btcutil.Amount for safer conversions:
-amount := float64(params.Amount.Uint64()) / 1e8 -amountSats := params.Amount.BigInt().Int64() +amountSats := params.Amount.BigInt().Int64() +amount := btcutil.Amount(amountSats).ToBTC()zetaclient/chains/bitcoin/signer/outbound_data_test.go (2)
18-241: Consider adding sub-benchmarks and edge cases.The test coverage is comprehensive, but consider these improvements:
- Add benchmarks for performance-critical paths:
func BenchmarkNewOutboundData(b *testing.B) { // Setup test data chain := chains.BitcoinMainnet receiver, _ := chains.DecodeBtcAddress("bc1qaxf82vyzy8y80v000e7t64gpten7gawewzu42y", chain.ChainId) b.Run("successful_creation", func(b *testing.B) { cctx := sample.CrossChainTx(b, "0x123") // Setup cctx... b.ResetTimer() for i := 0; i < b.N; i++ { NewOutboundData(cctx, chain.ChainId, 101, 0.00001, log.Logger, log.Logger) } }) }
- Add edge cases:
- Maximum possible amount
- Maximum possible gas limit (just below MaxInt64)
- Empty receiver address
- Zero height
231-231: Consider mocking the logger for cleaner tests.The test is using the global logger. Consider creating a mock or test logger:
-outboundData, err := NewOutboundData(tt.cctx, tt.chainID, tt.height, tt.minRelayFee, log.Logger, log.Logger) +testLogger := zerolog.New(ioutil.Discard) +outboundData, err := NewOutboundData(tt.cctx, tt.chainID, tt.height, tt.minRelayFee, testLogger, testLogger)zetaclient/chains/bitcoin/common/fee.go (1)
77-84: Improve error handling for invalid input.Consider returning an error for negative input values to make the validation more explicit.
func EstimateOutboundSize(numInputs int64, payees []btcutil.Address) (int64, error) { + if numInputs < 0 { + return 0, fmt.Errorf("negative number of inputs: %d", numInputs) + } if numInputs <= 0 { return 0, nil }zetaclient/chains/bitcoin/common/fee_test.go (1)
249-252: Consider using integer arithmetic for error tolerance.The current floating-point calculation could lead to precision issues. Consider using integer arithmetic:
-vError := int64( - 0.25 + float64(x)/4, -) // 1st witness incurs 0.25 more vByte error than others (which incurs 1/4 vByte per witness) +vError := (x + 1) / 4 // Integer division: each witness contributes 1/4 vByte errorzetaclient/chains/bitcoin/observer/utxos.go (1)
44-44: Enhance error message by including the underlying error.Including the original error message in the returned error can improve debugging and provide more context about the failure.
Apply this diff to include the underlying error:
- return fmt.Errorf("error getting bitcoin tss address") + return fmt.Errorf("error getting bitcoin TSS address: %v", err)zetaclient/chains/bitcoin/observer/observer.go (1)
268-269: Add unit tests forisNodeEnabledmethod to improve test coverage.The
isNodeEnabledmethod is currently not covered by any tests. Adding unit tests will help ensure the method behaves as expected and prevent potential issues in the future.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 268-269: zetaclient/chains/bitcoin/observer/observer.go#L268-L269
Added lines #L268 - L269 were not covered by testszetaclient/chains/bitcoin/observer/outbound.go (1)
74-98: Add unit tests forTryIncludeOutboundmethod to ensure reliability.The
TryIncludeOutboundmethod plays a crucial role in including outbound transactions but lacks test coverage. Implementing unit tests will help validate its functionality and maintain code quality.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 88-95: zetaclient/chains/bitcoin/observer/outbound.go#L88-L95
Added lines #L88 - L95 were not covered by tests
[warning] 97-97: zetaclient/chains/bitcoin/observer/outbound.go#L97
Added line #L97 was not covered by testszetaclient/chains/evm/rpc/rpc_live_test.go (1)
65-73: Enhance test robustness and resource management.While the basic test functionality is present, consider these improvements:
- Add client connection cleanup using
defer client.Close()- Add timeout to context
- Add more comprehensive gas price validations
func LiveTest_SuggestGasPrice(t *testing.T) { client, err := ethclient.Dial(URLBaseMainnet) require.NoError(t, err) + defer client.Close() - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() gasPrice, err := client.SuggestGasPrice(ctx) require.NoError(t, err) - require.True(t, gasPrice.Cmp(big.NewInt(0)) > 0) + require.True(t, gasPrice.Cmp(big.NewInt(0)) > 0, "gas price should be positive") + require.True(t, gasPrice.BitLen() <= 256, "gas price should not exceed 256 bits") }zetaclient/chains/bitcoin/observer/gas_price.go (1)
56-70: Add error handling for unsupported network types.The error handling for unsupported network types could be more informative.
func (ob *Observer) specialHandleFeeRate(ctx context.Context) (int64, error) { switch ob.Chain().NetworkType { // ... existing cases ... default: - return 0, fmt.Errorf(" unsupported bitcoin network type %d", ob.Chain().NetworkType) + return 0, fmt.Errorf("unsupported bitcoin network type %d for fee rate estimation", ob.Chain().NetworkType) } }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 57-68: zetaclient/chains/bitcoin/observer/gas_price.go#L57-L68
Added lines #L57 - L68 were not covered by testszetaclient/testutils/mocks/zetacore_client_opts.go (1)
37-46: Consider using specific mock matchers.While the implementation is functional, using specific matchers would make the mock expectations more precise.
func (_m *ZetacoreClient) WithPostOutboundTracker(zetaTxHash string) *ZetacoreClient { - on := _m.On("PostOutboundTracker", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Maybe() + on := _m.On( + "PostOutboundTracker", + mock.AnythingOfType("*context.Context"), + mock.AnythingOfType("string"), // outboundID + mock.AnythingOfType("string"), // txHash + mock.AnythingOfType("uint64"), // blockHeight + ).Maybe() if zetaTxHash != "" { on.Return(zetaTxHash, nil) } else { on.Return("", errSomethingIsWrong) } return _m }zetaclient/chains/bitcoin/observer/db.go (1)
29-55: Consider adding context timeout for RPC calls.While the implementation is solid, RPC calls to Bitcoin nodes should have appropriate timeouts to prevent hanging in case of network issues.
func (ob *Observer) LoadLastBlockScanned(ctx context.Context) error { + // Add timeout for RPC calls + ctx, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() err := ob.Observer.LoadLastBlockScanned(ob.Logger().Chain)zetaclient/chains/bitcoin/observer/db_test.go (2)
17-38: Add test case for concurrent access.Consider adding a test case that verifies thread safety by simulating concurrent access to SaveBroadcastedTx.
t.Run("should handle concurrent saves", func(t *testing.T) { ob := newTestSuite(t, chains.BitcoinMainnet, "") var wg sync.WaitGroup for i := 0; i < 10; i++ { wg.Add(1) go func(nonce uint64) { defer wg.Done() txHash := sample.BtcHash().String() ob.SaveBroadcastedTx(txHash, nonce) }(uint64(i)) } wg.Wait() })
60-62: Use t.Setenv instead of os.Setenv in tests.Using t.Setenv is preferred in tests as it automatically handles cleanup.
- os.Setenv(envvar, "invalid") - defer os.Unsetenv(envvar) + t.Setenv(envvar, "invalid")zetaclient/chains/bitcoin/signer/sign_test.go (3)
24-91: Consider adding more edge cases to the test suite.The test suite follows good practices with table-driven tests. Consider adding these edge cases:
- Maximum number of inputs
- Zero amount inputs
- Duplicate transaction IDs
Would you like me to help generate these additional test cases?
93-257: Avoid hardcoding Bitcoin addresses in tests.Replace the hardcoded Bitcoin address with a dynamically generated one using the existing test utilities:
- receiver := "bc1qaxf82vyzy8y80v000e7t64gpten7gawewzu42y" + receiver := sample.BtcAddressP2WPKH(t, chains.BitcoinMainnet.Params).String()
259-326: Add error cases to the test suite.The test suite only covers the happy path. Consider adding these test cases:
- Invalid input amounts
- Missing witness data
- Invalid signature
- Context cancellation
Would you like me to help generate these error test cases?
zetaclient/testutils/testdata.go (1)
120-126: LGTM! Consider adding documentation.The function implementation is clean and follows the established patterns. Consider adding a doc comment to describe the expected file format and location.
Add this documentation above the function:
// LoadBTCTransaction loads a Bitcoin transaction from a JSON file in the testdata directory. // The file should contain a serialized btcjson.GetTransactionResult object.zetaclient/chains/bitcoin/signer/signer.go (3)
168-177: Optimize Retry Logic inBroadcastOutboundIn the
BroadcastOutboundfunction, the retry loop sleeps before the first broadcast attempt, causing unnecessary delay. Modify the loop to attempt broadcasting immediately and sleep only after a failed attempt.Proposed change:
- backOff := broadcastBackoff - for i := 0; i < broadcastRetries; i++ { - time.Sleep(backOff) + for i := 0; i < broadcastRetries; i++ { // broadcast tx err := signer.Broadcast(ctx, tx) if err != nil { logger.Warn().Err(err).Fields(lf).Msgf("broadcasting Bitcoin outbound, retry %d", i) + time.Sleep(backOff) backOff *= 2 continue } logger.Info().Fields(lf).Msg("broadcasted Bitcoin outbound successfully") // successful broadcast; no need to retry break }
168-200: Add Logging for Failed Broadcast AttemptsIf the broadcast fails after all retries in
BroadcastOutbound, there is no log indicating the final failure. Add a log message after the retries are exhausted to inform that broadcasting was unsuccessful.Add after the loop:
if i == broadcastRetries { logger.Error().Fields(lf).Msg("Failed to broadcast Bitcoin outbound after maximum retries") }
119-119: Correct Logging Message GrammarThe error message should be corrected for proper grammar.
Change:
- logger.Error().Err(err).Msgf("failed get bitcoin network info") + logger.Error().Err(err).Msg("failed to get Bitcoin network info")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
changelog.md(1 hunks)testutil/sample/crypto.go(2 hunks)x/crosschain/types/cctx_test.go(1 hunks)x/crosschain/types/revert_options_test.go(1 hunks)zetaclient/chains/bitcoin/client/client_test.go(2 hunks)zetaclient/chains/bitcoin/client/helpers.go(3 hunks)zetaclient/chains/bitcoin/client/mockgen.go(1 hunks)zetaclient/chains/bitcoin/common/fee.go(7 hunks)zetaclient/chains/bitcoin/common/fee_test.go(11 hunks)zetaclient/chains/bitcoin/observer/db.go(1 hunks)zetaclient/chains/bitcoin/observer/db_test.go(1 hunks)zetaclient/chains/bitcoin/observer/event_test.go(5 hunks)zetaclient/chains/bitcoin/observer/gas_price.go(1 hunks)zetaclient/chains/bitcoin/observer/inbound_test.go(2 hunks)zetaclient/chains/bitcoin/observer/observer.go(5 hunks)zetaclient/chains/bitcoin/observer/observer_test.go(5 hunks)zetaclient/chains/bitcoin/observer/outbound.go(6 hunks)zetaclient/chains/bitcoin/observer/outbound_test.go(7 hunks)zetaclient/chains/bitcoin/observer/utxos.go(1 hunks)zetaclient/chains/bitcoin/signer/outbound_data.go(1 hunks)zetaclient/chains/bitcoin/signer/outbound_data_test.go(1 hunks)zetaclient/chains/bitcoin/signer/sign.go(1 hunks)zetaclient/chains/bitcoin/signer/sign_test.go(1 hunks)zetaclient/chains/bitcoin/signer/signer.go(4 hunks)zetaclient/chains/bitcoin/signer/signer_test.go(10 hunks)zetaclient/chains/evm/observer/observer_test.go(0 hunks)zetaclient/chains/evm/rpc/rpc_live_test.go(4 hunks)zetaclient/chains/evm/signer/signer.go(1 hunks)zetaclient/logs/fields.go(1 hunks)zetaclient/orchestrator/orchestrator.go(10 hunks)zetaclient/orchestrator/v2_bootstrap.go(0 hunks)zetaclient/testdata/btc/chain_8332_msgtx_030cd813443f7b70cc6d8a544d320c6d8465e4528fc0f3410b599dc0b26753a0.json(1 hunks)zetaclient/testdata/btc/chain_8332_tx_030cd813443f7b70cc6d8a544d320c6d8465e4528fc0f3410b599dc0b26753a0.json(1 hunks)zetaclient/testutils/mocks/bitcoin_client.go(1 hunks)zetaclient/testutils/mocks/zetacore_client_opts.go(1 hunks)zetaclient/testutils/testdata.go(1 hunks)zetaclient/testutils/testdata_naming.go(1 hunks)zetaclient/zetacore/broadcast.go(2 hunks)zetaclient/zetacore/broadcast_test.go(1 hunks)
💤 Files with no reviewable changes (2)
- zetaclient/orchestrator/v2_bootstrap.go
- zetaclient/chains/evm/observer/observer_test.go
✅ Files skipped from review due to trivial changes (3)
- zetaclient/testdata/btc/chain_8332_tx_030cd813443f7b70cc6d8a544d320c6d8465e4528fc0f3410b599dc0b26753a0.json
- zetaclient/testdata/btc/chain_8332_msgtx_030cd813443f7b70cc6d8a544d320c6d8465e4528fc0f3410b599dc0b26753a0.json
- zetaclient/orchestrator/orchestrator.go
🧰 Additional context used
📓 Path-based instructions (33)
zetaclient/logs/fields.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/testutils/testdata_naming.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/zetacore/broadcast_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/testutils/testdata.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/signer/signer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/revert_options_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/client/client_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/gas_price.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/zetacore/broadcast.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/client/mockgen.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/signer/outbound_data.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
testutil/sample/crypto.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/testutils/mocks/zetacore_client_opts.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/client/helpers.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/cctx_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/signer/outbound_data_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/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/signer/sign_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/db.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/testutils/mocks/bitcoin_client.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/db_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/utxos.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.
zetaclient/chains/bitcoin/signer/sign.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/common/fee_test.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.
zetaclient/chains/bitcoin/observer/event_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/signer/signer_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.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/common/fee.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/client/client_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/signer/outbound_data.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#3306
File: zetaclient/chains/bitcoin/signer/sign.go:155-211
Timestamp: 2025-01-13T23:23:35.853Z
Learning: Amount validation for Bitcoin outbound transactions, including checks for gas price, recipient address, and dust threshold, is performed in NewOutboundData before the transaction signing process begins.
zetaclient/chains/bitcoin/observer/db.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#3306
File: zetaclient/chains/bitcoin/observer/db.go:11-25
Timestamp: 2025-01-14T00:09:01.416Z
Learning: In Bitcoin observer, non-critical DB operations (like saving broadcasted transactions) should log errors but not return them to callers, as the errors are already handled appropriately through logging and don't affect the critical in-memory state.
zetaclient/chains/bitcoin/observer/event_test.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#2987
File: pkg/memo/fields_v0_test.go:270-320
Timestamp: 2024-11-12T13:20:12.658Z
Learning: In the `FieldsV0` struct of the `memo` package, `RevertAddress` may be left empty when `CallOnRevert` is set to true.
🪛 GitHub Check: codecov/patch
zetaclient/chains/bitcoin/observer/gas_price.go
[warning] 15-34: zetaclient/chains/bitcoin/observer/gas_price.go#L15-L34
Added lines #L15 - L34 were not covered by tests
[warning] 38-41: zetaclient/chains/bitcoin/observer/gas_price.go#L38-L41
Added lines #L38 - L41 were not covered by tests
[warning] 44-51: zetaclient/chains/bitcoin/observer/gas_price.go#L44-L51
Added lines #L44 - L51 were not covered by tests
[warning] 53-53: zetaclient/chains/bitcoin/observer/gas_price.go#L53
Added line #L53 was not covered by tests
[warning] 57-68: zetaclient/chains/bitcoin/observer/gas_price.go#L57-L68
Added lines #L57 - L68 were not covered by tests
zetaclient/chains/bitcoin/client/helpers.go
[warning] 120-123: zetaclient/chains/bitcoin/client/helpers.go#L120-L123
Added lines #L120 - L123 were not covered by tests
[warning] 127-131: zetaclient/chains/bitcoin/client/helpers.go#L127-L131
Added lines #L127 - L131 were not covered by tests
[warning] 133-145: zetaclient/chains/bitcoin/client/helpers.go#L133-L145
Added lines #L133 - L145 were not covered by tests
[warning] 147-147: zetaclient/chains/bitcoin/client/helpers.go#L147
Added line #L147 was not covered by tests
zetaclient/chains/bitcoin/observer/db.go
[warning] 22-25: zetaclient/chains/bitcoin/observer/db.go#L22-L25
Added lines #L22 - L25 were not covered by tests
[warning] 61-63: zetaclient/chains/bitcoin/observer/db.go#L61-L63
Added lines #L61 - L63 were not covered by tests
zetaclient/chains/bitcoin/observer/observer.go
[warning] 268-269: zetaclient/chains/bitcoin/observer/observer.go#L268-L269
Added lines #L268 - L269 were not covered by tests
zetaclient/chains/bitcoin/observer/outbound.go
[warning] 38-41: zetaclient/chains/bitcoin/observer/outbound.go#L38-L41
Added lines #L38 - L41 were not covered by tests
[warning] 43-46: zetaclient/chains/bitcoin/observer/outbound.go#L43-L46
Added lines #L43 - L46 were not covered by tests
[warning] 49-50: zetaclient/chains/bitcoin/observer/outbound.go#L49-L50
Added lines #L49 - L50 were not covered by tests
[warning] 53-53: zetaclient/chains/bitcoin/observer/outbound.go#L53
Added line #L53 was not covered by tests
[warning] 64-66: zetaclient/chains/bitcoin/observer/outbound.go#L64-L66
Added lines #L64 - L66 were not covered by tests
[warning] 88-95: zetaclient/chains/bitcoin/observer/outbound.go#L88-L95
Added lines #L88 - L95 were not covered by tests
[warning] 97-97: zetaclient/chains/bitcoin/observer/outbound.go#L97
Added line #L97 was not covered by tests
[warning] 101-101: zetaclient/chains/bitcoin/observer/outbound.go#L101
Added line #L101 was not covered by tests
[warning] 124-126: zetaclient/chains/bitcoin/observer/outbound.go#L124-L126
Added lines #L124 - L126 were not covered by tests
[warning] 143-144: zetaclient/chains/bitcoin/observer/outbound.go#L143-L144
Added lines #L143 - L144 were not covered by tests
[warning] 232-232: zetaclient/chains/bitcoin/observer/outbound.go#L232
Added line #L232 was not covered by tests
[warning] 234-234: zetaclient/chains/bitcoin/observer/outbound.go#L234
Added line #L234 was not covered by tests
[warning] 240-240: zetaclient/chains/bitcoin/observer/outbound.go#L240
Added line #L240 was not covered by tests
[warning] 242-242: zetaclient/chains/bitcoin/observer/outbound.go#L242
Added line #L242 was not covered by tests
[warning] 258-258: zetaclient/chains/bitcoin/observer/outbound.go#L258
Added line #L258 was not covered by tests
[warning] 262-262: zetaclient/chains/bitcoin/observer/outbound.go#L262
Added line #L262 was not covered by tests
zetaclient/chains/bitcoin/common/fee.go
[warning] 250-250: zetaclient/chains/bitcoin/common/fee.go#L250
Added line #L250 was not covered by tests
[warning] 290-290: zetaclient/chains/bitcoin/common/fee.go#L290
Added line #L290 was not covered by tests
🔇 Additional comments (26)
zetaclient/chains/bitcoin/signer/signer_test.go (2)
40-83: LGTM! Well-structured test suite setup.The test suite setup is well-organized with proper mocking of dependencies and clear initialization.
97-170: LGTM! Comprehensive test cases for BroadcastOutbound.The test cases cover important scenarios including RBF and failure cases.
zetaclient/zetacore/broadcast.go (1)
169-172: LGTM! Well-structured logging implementation.The logging improvements are excellent:
- Uses proper field types for type safety
- Adds method field for better tracing
- Maintains consistency across error cases
zetaclient/chains/bitcoin/common/fee.go (1)
22-32: LGTM! Well-documented constants with clear purpose.The constants are properly documented with their respective byte sizes and the type conversion to
int64is consistent with the function signature changes.zetaclient/chains/bitcoin/common/fee_test.go (1)
270-270: LGTM! Consistent use of require.Zero for nil address tests.The test assertions are clear and consistent across all address types, improving test readability.
Also applies to: 281-281, 292-292, 303-303, 314-314, 325-325
zetaclient/chains/bitcoin/observer/outbound.go (1)
25-28:⚠️ Potential issueSet
minTxConfirmationsto a safer default value for production.Having
minTxConfirmationsset to0may allow transactions with zero confirmations to be considered valid, which can pose security risks. It's advisable to set this value to at least1to ensure that only transactions with confirmed blocks are processed.Apply this diff to update the confirmation threshold:
const ( // minTxConfirmations is the minimum confirmations for a Bitcoin tx to be considered valid by the observer // Note: please change this value to 1 to be able to run the Bitcoin E2E RBF test - minTxConfirmations = 0 + minTxConfirmations = 1 )⛔ Skipped due to learnings
Learnt from: ws4charlie PR: zeta-chain/node#3306 File: zetaclient/chains/bitcoin/observer/outbound.go:28-32 Timestamp: 2025-01-13T23:51:09.334Z Learning: In Bitcoin observer, `minTxConfirmations` is set to 0 to allow observing mempool transactions without waiting for block confirmations, which is necessary due to Bitcoin's 10-minute block time. This is a deliberate design choice despite potential security implications, as other safety checks are in place through TSS validation.zetaclient/logs/fields.go (1)
12-12: LGTM: New field constant follows conventions.The addition of
FieldOutboundIDaligns with existing field naming patterns and supports the Bitcoin RBF integration objectives.zetaclient/chains/evm/rpc/rpc_live_test.go (1)
21-21: LGTM: Base mainnet URL constant added.The constant follows the existing URL naming pattern.
zetaclient/chains/bitcoin/observer/db.go (1)
12-27: LGTM! Thread-safe implementation with proper error handling.The implementation correctly uses mutex for thread safety and follows the established pattern of logging DB errors without returning them.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 22-25: zetaclient/chains/bitcoin/observer/db.go#L22-L25
Added lines #L22 - L25 were not covered by testsx/crosschain/types/revert_options_test.go (1)
52-52: LGTM! Improved type safety.The explicit conversion to string using
.String()method improves type safety and clarity.zetaclient/chains/bitcoin/client/mockgen.go (1)
48-48: LGTM! Clean interface extension.The new method follows the existing pattern and has a clear, well-defined signature.
zetaclient/chains/bitcoin/observer/db_test.go (1)
94-120: LGTM! Comprehensive persistence testing.The test effectively verifies that transaction data persists across different observer instances.
zetaclient/chains/bitcoin/client/helpers.go (2)
119-124: Add unit tests for fee rate conversion.The function logic looks correct, but it lacks test coverage. Please add unit tests to verify the conversion from BTC/KB to sat/vB.
Would you like me to help generate comprehensive test cases for this function?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 120-123: zetaclient/chains/bitcoin/client/helpers.go#L120-L123
Added lines #L120 - L123 were not covered by tests
126-148: Add unit tests and improve error handling.The function has good error handling but lacks test coverage. Consider the following improvements:
- Add unit tests to verify fee rate estimation for both regnet and non-regnet modes.
- Consider using a custom error type for better error handling.
Here's a suggested improvement for error handling:
+type FeeRateError struct { + Reason string + FeeRate *float64 +} + +func (e *FeeRateError) Error() string { + return fmt.Sprintf("fee rate error: %s", e.Reason) +} func (c *Client) GetEstimatedFeeRate(ctx context.Context, confTarget int64, regnet bool) (int64, error) { if regnet { return FeeRateRegnet, nil } feeResult, err := c.EstimateSmartFee(ctx, confTarget, &types.EstimateModeEconomical) if err != nil { - return 0, errors.Wrap(err, "unable to estimate smart fee") + return 0, &FeeRateError{Reason: "unable to estimate smart fee", FeeRate: nil} } if feeResult.Errors != nil { - return 0, fmt.Errorf("fee result contains errors: %s", feeResult.Errors) + return 0, &FeeRateError{Reason: fmt.Sprintf("fee result contains errors: %s", feeResult.Errors), FeeRate: nil} } if feeResult.FeeRate == nil { - return 0, fmt.Errorf("nil fee rate") + return 0, &FeeRateError{Reason: "nil fee rate", FeeRate: nil} } if *feeResult.FeeRate <= 0 || *feeResult.FeeRate >= maxBTCSupply { - return 0, fmt.Errorf("invalid fee rate: %f", *feeResult.FeeRate) + return 0, &FeeRateError{Reason: fmt.Sprintf("invalid fee rate: %f", *feeResult.FeeRate), FeeRate: feeResult.FeeRate} } return FeeRateToSatPerByte(*feeResult.FeeRate).Int64(), nil }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 127-131: zetaclient/chains/bitcoin/client/helpers.go#L127-L131
Added lines #L127 - L131 were not covered by tests
[warning] 133-145: zetaclient/chains/bitcoin/client/helpers.go#L133-L145
Added lines #L133 - L145 were not covered by tests
[warning] 147-147: zetaclient/chains/bitcoin/client/helpers.go#L147
Added line #L147 was not covered by teststestutil/sample/crypto.go (2)
95-104: LGTM!The function follows best practices for generating test Bitcoin addresses and properly handles error cases.
106-111: LGTM!The function efficiently reuses
BtcAddressP2WPKHand properly generates payment scripts for testing purposes.zetaclient/chains/bitcoin/observer/event_test.go (1)
35-35: LGTM! Consistent address string conversion.The changes correctly handle Bitcoin address conversion to string representation, maintaining consistency across test cases.
Also applies to: 252-252, 403-403
zetaclient/chains/bitcoin/client/client_test.go (1)
140-140: LGTM! Improved assertion accuracy.The test now correctly validates the total amount by including the depositor fee in the assertion.
zetaclient/chains/bitcoin/observer/outbound_test.go (1)
266-269: LGTM! Improved result handling with structured types.The changes enhance code clarity by using a structured result type instead of multiple return values. This makes the code more maintainable and type-safe.
Also applies to: 274-278, 284-287, 293-296, 302-306, 312-316, 322-327, 332-336, 341-344, 363-368, 377-382, 391-401, 410-420, 435-444, 459-470
zetaclient/testutils/mocks/bitcoin_client.go (1)
360-386: LGTM! Well-structured mock implementation.The mock implementation follows best practices with:
- Proper panic handling for unspecified return values
- Type assertion handling for different return scenarios
- Consistent error propagation
x/crosschain/types/cctx_test.go (1)
143-143: LGTM! Consistent address string handling in tests.The explicit
.String()call aligns with the codebase's approach to Bitcoin address handling, ensuring consistent string representation in tests.changelog.md (1)
10-10: LGTM! Clear and well-formatted changelog entry.The entry follows the changelog format and clearly describes the Bitcoin RBF adoption refactoring changes.
zetaclient/chains/evm/signer/signer.go (1)
522-523: Ensure Consistent Types forHandleBroadcastErrorParametersThe
HandleBroadcastErrorfunction now receivescctx.GetCurrentOutboundParam().TssNonceandtoChain.ID()directly without converting them to strings. Verify thatHandleBroadcastErroraccepts these parameters as integer types (uint64orint64) to prevent type mismatches.zetaclient/testutils/testdata_naming.go (1)
67-70: LGTM! Clean and consistent implementation.The function follows the established naming pattern and maintains consistency with other file naming functions in the module.
zetaclient/chains/bitcoin/observer/inbound_test.go (2)
170-170: LGTM! Consistent address string representation.Using
String()method on the Bitcoin address ensures consistent string representation across the codebase.
158-158: Verify the purpose of the empty string parameter.The test suite initialization has been modified to include an empty string parameter. Please document the purpose of this parameter in the test setup.
…ForRegnetAndTestnet; code simplification
…e fee rate according to Bitcoin network type
…t-bitcoin-RBF-zetaclient-refactor-minimum
Description
db.gogas_price.go,utxos.go) and add a few unit tests.sign.go; create reusable methodsAddTxInputsSignTxand unit tests.GetEstimatedFeeRateBitcoin RPC helper function.TryIncludeOutboundas the only entry point for outbound inclusion. Refactor on Bitcoinoutbound.go.OutboundDatastruct to check and pass Bitcoin outbound tx data, similar to EVM chain.ObserveOutboundfor outbound tracker processing.BroadcastOutboundto handle tx broadcasting; add unit test.int64to be consistent with Bitcoin APIs, e.g.mempool.GetTxVirtualSize. The Bitcoin usesint64to represent output value inwire.NewTxOut, so usingint64makes calculationvalue = txSize * feeRateeasier.How Has This Been Tested?
Summary by CodeRabbit
Release Notes for ZetaChain Bitcoin Client Update
New Features
Improvements
Technical Enhancements