feat(btc): implement new btc rpc package #3349
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive refactoring of the Bitcoin RPC client implementation in the Zeta Chain node, focusing on enhancing context management, improving observability, and streamlining the RPC interaction model. The changes span multiple packages, introducing a new Bitcoin client implementation with improved context handling, metrics tracking, and more modular design. Changes
Sequence DiagramsequenceDiagram
participant Client as Bitcoin RPC Client
participant Observer as ZetaChain Observer
participant Blockchain as Bitcoin Blockchain
Client->>Observer: Initialize with context
Observer->>Blockchain: Ping/Healthcheck
Blockchain-->>Observer: Return network status
Observer->>Client: Record metrics
Client->>Blockchain: Perform RPC calls with context
Blockchain-->>Client: Return transaction/block data
Client->>Observer: Provide processed data
Possibly related PRs
Suggested Labels
Suggested Reviewers
Technical AnalysisThe refactoring introduces several key improvements:
The changes represent a significant architectural improvement in how Bitcoin RPC interactions are managed within the Zeta Chain node, promoting more robust, observable, and maintainable code. Finishing Touches
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 |
swift1337
added
chain:bitcoin
swift1337
force-pushed
the
feat/btc-rpc
branch
from
7966efb to
757ad54
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
e2e/runner/setup_bitcoin.go (1)
Line range hint
89-107: Consider using structured types for wallet creation parameters.The current approach of manually marshaling wallet creation parameters to JSON is error-prone and hard to maintain.
Consider creating a dedicated type:
type WalletCreationParams struct { Name string `json:"wallet_name"` DisablePrivKeys bool `json:"disable_private_keys"` Blank bool `json:"blank"` Passphrase string `json:"passphrase"` AvoidReuse bool `json:"avoid_reuse"` Descriptors bool `json:"descriptors"` LoadOnStartup bool `json:"load_on_startup"` }zetaclient/context/app.go (1)
Line range hint
238-248: Replace customslicesEqualwithslices.Equal.The standard library's
slices.Equalfunction provides the same functionality and is likely more optimized.-func slicesEqual[T constraints.Ordered](a, b []T) bool { - if len(a) != len(b) { - return false - } - - for i := range a { - if a[i] != b[i] { - return false - } - } - - return true -}Use
slices.Equal(freshChainIDs, existingChainIDs)directly in the code.zetaclient/chains/bitcoin/observer/inbound.go (1)
Line range hint
24-108: Decompose ObserveInbound into smaller functions.The function is handling multiple responsibilities. Consider breaking it down into smaller, focused functions:
- Block height validation
- Transaction filtering
- Vote processing
func (ob *Observer) ObserveInbound(ctx context.Context) error { if err := ob.validateAndUpdateBlockHeight(ctx); err != nil { return err } if !ob.IsBlockConfirmed(ob.LastBlockScanned() + 1) { return nil } return ob.processInboundBlock(ctx, ob.LastBlockScanned() + 1) } func (ob *Observer) validateAndUpdateBlockHeight(ctx context.Context) error { // Block height validation logic } func (ob *Observer) processInboundBlock(ctx context.Context, blockNumber uint64) error { // Transaction processing logic }
🧹 Nitpick comments (40)
zetaclient/chains/bitcoin/observer/outbound.go (3)
Line range hint
359-367: Consider adding error type checks for better error handling.The error returned from
GetTransactionByStrcould be more specifically handled to differentiate between network errors, invalid transaction errors, or other Bitcoin RPC specific errors._, getTxResult, err := ob.rpc.GetTransactionByStr(ctx, txid) if err != nil { + if errors.Is(err, bitcoin.ErrTxNotFound) { + return "", fmt.Errorf("getOutboundIDByNonce: transaction %s not found for nonce %d", txid, nonce) + } return "", errors.Wrapf( err, "getOutboundIDByNonce: error getting outbound result for nonce %d hash %s", nonce, txid, ) }
Line range hint
402-419: Enhance error logging with transaction details.The error logging in
checkIncludedTxcould be more informative by including additional context about the transaction state.hash, getTxResult, err := ob.rpc.GetTransactionByStr(ctx, txHash) if err != nil { - ob.logger.Outbound.Error().Err(err).Msgf("checkIncludedTx: error GetTxResultByHash: %s", txHash) + ob.logger.Outbound.Error().Err(err). + Str("tx_hash", txHash). + Uint64("nonce", cctx.GetCurrentOutboundParam().TssNonce). + Msgf("checkIncludedTx: failed to get transaction result") return nil, false }
Line range hint
1-644: Consider implementing metrics for UTXO management.The UTXO selection and management logic is critical for Bitcoin operations. Adding Prometheus metrics would help monitor:
- UTXO pool size
- Average UTXO value
- UTXO consolidation frequency
- Failed transaction attempts
zetaclient/chains/bitcoin/observer/observer_test.go (4)
73-74: LGTM! Consider enhancing mock setup with context validation.The transition to the new Bitcoin client mock is well-structured. The mock setup correctly handles the context parameter.
Consider adding context validation to ensure proper context propagation:
-btcClient.On("GetBlockCount", mock.Anything).Return(int64(100), nil) +btcClient.On("GetBlockCount", mock.MatchedBy(func(ctx context.Context) bool { + return ctx != nil +})).Return(int64(100), nil)Also applies to: 80-80
105-105: Consider retaining the detailed error message.The simplified error message might make debugging more challenging. The original message "unable to get BTC net params for chain" provides more context about the failure.
-errorMessage: "unable to get BTC net params", +errorMessage: "unable to get BTC net params for chain",
208-208: LGTM! Well-structured test cases with proper context handling.The LoadLastBlockScanned tests comprehensively cover success and error scenarios with proper context propagation.
Consider using test table pattern for better organization of the LoadLastBlockScanned test cases:
tests := []struct { name string chain chains.Chain envVar string envVal string mockFn func(*mocks.BitcoinClient) wantErr string want int64 }{ // ... test cases }Also applies to: 216-216, 230-230, 245-245, 253-253
304-305: LGTM! Well-structured test suite with proper context integration.The test suite modifications properly handle context and mock setup.
Consider adding a context with timeout in the test suite to prevent indefinite test execution:
-ctx := context.Background() +ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) +defer cancel()Also applies to: 311-312, 317-318, 340-340
zetaclient/chains/bitcoin/observer/outbound_test.go (2)
Line range hint
19-20: Consider improving test data path management.The test data directory is defined using a relative path. This could be fragile if the working directory changes during test execution.
Consider using:
- A more robust path resolution mechanism
- Environment variables for test data location
- Embedding test data in the binary using
go:embed-var TestDataDir = "../../../" +//go:embed testdata +var testData embed.FS + +var TestDataDir = "testdata"
Line range hint
362-500: Enhance test readability with named constants and detailed descriptions.The UTXO consolidation tests use magic numbers that could be more self-documenting.
Consider defining named constants for frequently used values:
+const ( + // UTXO consolidation thresholds + defaultUTXOCap = 10 + consolidationRank = 5 + + // Test amounts + smallAmount = 0.01 + largeAmount = 8.72 + + // Test nonce values + startingNonce = 24105431 +)Also, consider expanding test descriptions to include:
- Input parameters
- Expected consolidation behavior
- Reasoning behind the test case
zetaclient/chains/bitcoin/signer/signer.go (2)
60-65: RefactorNewfunction to clarify parameter orderConsider reordering the parameters in the
Newfunction for better readability. Placing theloggeras the last parameter is a common Go convention and enhances code clarity.Apply this diff to adjust the parameter order:
-func New(chain chains.Chain, tss interfaces.TSSSigner, rpc RPC, logger base.Logger) *Signer { +func New(chain chains.Chain, tss interfaces.TSSSigner, rpc RPC, logger base.Logger) *Signer {This suggestion aims to improve consistency with Go conventions.
Line range hint
405-421: Enhance retry logic with exponential backoff and jitterImplementing jitter in the retry mechanism prevents synchronized retries across multiple instances, reducing the risk of a thundering herd problem. This improvement promotes better resilience and network stability.
Apply this diff to introduce jitter:
for i := 0; i < broadcastRetries; i++ { - time.Sleep(backOff) + time.Sleep(backOff + time.Duration(rand.Int63n(int64(backOff)))) err := signer.Broadcast(ctx, tx) if err != nil { logger.Warn().Err(err).Fields(lf).Msgf("Broadcasting Bitcoin tx, retry %d", i) backOff *= 2 continue } logger.Info().Fields(lf).Msgf("Broadcast Bitcoin tx successfully") // Additional logic... }Ensure that the
math/randpackage is imported to utilize therand.Int63nfunction.zetaclient/chains/bitcoin/observer/observer.go (2)
Line range hint
289-309: Optimize UTXO fetching by specifying minimum confirmationsIn the
FetchUTXOsmethod, unconfirmed UTXOs are filtered out after retrieval. To improve efficiency, consider settingminConfto1inListUnspentMinMaxAddressesto exclude unconfirmed UTXOs upfront.Apply this diff to adjust the
minConfparameter:-utxos, err := ob.rpc.ListUnspentMinMaxAddresses(ctx, 0, maxConfirmations, []btcutil.Address{tssAddr}) +utxos, err := ob.rpc.ListUnspentMinMaxAddresses(ctx, 1, maxConfirmations, []btcutil.Address{tssAddr})This change reduces unnecessary data processing and network load.
433-441: Enhance error message for unsupported network typesIncluding the actual network type in the error message provides better context and aids in debugging.
Apply this diff to improve the error message:
-default: - return 0, fmt.Errorf(" unsupported bitcoin network type %d", ob.Chain().NetworkType) +default: + return 0, fmt.Errorf("unsupported bitcoin network type: %v", ob.Chain().NetworkType)This modification ensures that the error message accurately reflects the unsupported network type.
zetaclient/chains/bitcoin/observer/rpc_status.go (1)
Line range hint
16-24: Consider enhancing error context.While the error handling is correct, consider adding more context about the block time in the error message when RPC status check fails.
- return errors.Wrap(err, "unable to check RPC status") + return errors.Wrapf(err, "unable to check RPC status (block time: %v)", blockTime)zetaclient/orchestrator/v2_bootstrap.go (1)
Line range hint
51-54: Consider tracking TODO items.The TODO comments about extracting base observer and signer components should be tracked in the issue system.
Would you like me to create GitHub issues to track these architectural improvements?
zetaclient/testutils/testrpc/rpc_btc.go (1)
Line range hint
61-76: LGTM! Well-structured test helper.The mock implementation correctly handles context and provides a clean interface for loading test transactions.
Consider enhancing error handling for file operations:
var msgTx wire.MsgTx - testutils.LoadObjectFromJSONFile(t, &msgTx, nameMsgTx) + err := testutils.LoadObjectFromJSONFile(t, &msgTx, nameMsgTx) + require.NoError(t, err, "failed to load test transaction data")zetaclient/chains/bitcoin/client/mockgen.go (2)
15-19: Consider adding comprehensive interface documentation.The interface lacks detailed documentation explaining its purpose, usage patterns, and implementation requirements. This documentation is crucial for maintainers and implementers.
Add a detailed comment block explaining:
- Interface purpose and scope
- Implementation requirements
- Usage examples
- Any specific threading or context cancellation considerations
79-84: Clarify the versioning in method nameSignRawTransactionWithWallet2.The
2suffix in the method name suggests versioning but lacks explanation. Consider using a more descriptive name or adding documentation explaining the difference between versions.Consider renaming to better reflect its purpose, for example:
-SignRawTransactionWithWallet2( +SignRawTransactionWithWalletAndWitness(cmd/zetaclientd/inbound.go (1)
165-168: Consider extracting client initialization to a separate function.The client initialization logic could be moved to a dedicated function to improve code organization and reusability.
Consider refactoring to:
+func initializeBitcoinClient(bitcoinConfig *config.BitcoinConfig, chainID int64) (btcclient.Client, error) { + return btcclient.New(bitcoinConfig, chainID, zerolog.Nop()) +} -rpcClient, err := btcclient.New(bitcoinConfig, chain.ID(), zerolog.Nop()) +rpcClient, err := initializeBitcoinClient(bitcoinConfig, chain.ID())zetaclient/chains/bitcoin/observer/witness.go (2)
45-48: Consider adding context timeout for fee calculation.The fee calculation RPC call could potentially be slow. Consider adding a timeout to prevent long-running operations.
- depositorFee, err := feeCalculator(ctx, rpc, &tx, netParams) + ctxWithTimeout, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() + depositorFee, err := feeCalculator(ctxWithTimeout, rpc, &tx, netParams)
73-76: Consider adding retry mechanism for sender address lookup.The GetSenderAddressByVin operation might fail due to temporary network issues.
- fromAddress, err := GetSenderAddressByVin(ctx, rpc, tx.Vin[0], netParams) + fromAddress, err := common.WithRetry(func() (string, error) { + return GetSenderAddressByVin(ctx, rpc, tx.Vin[0], netParams) + }, 3, time.Second)e2e/runner/accounting.go (1)
102-102: Consider parallel processing for UTXO listing.When dealing with multiple TSS addresses, UTXO listing could be parallelized for better performance.
+ type utxoResult struct { + balance float64 + err error + } + ch := make(chan utxoResult) + + for _, tssAddress := range allTssAddress.TssList { + go func(addr *observertypes.TSS) { + btcTssAddress, err := zetacrypto.GetTssAddrBTC(addr.TssPubkey, r.BitcoinParams) + if err != nil { + ch <- utxoResult{err: err} + return + } + utxos, err := r.BtcRPCClient.ListUnspent(r.Ctx) + if err != nil { + ch <- utxoResult{err: err} + return + } + var balance float64 + for _, utxo := range utxos { + if utxo.Address == btcTssAddress { + balance += utxo.Amount + } + } + ch <- utxoResult{balance: balance} + }(tssAddress) + }zetaclient/chains/bitcoin/client/helpers.go (2)
107-156: Consider implementing fee calculation caching.The GetTransactionFeeAndRate function makes multiple RPC calls. Consider implementing a cache for frequently accessed transactions.
+type feeCache struct { + fee int64 + feeRate int64 + expires time.Time +} + +var ( + feeCacheMu sync.RWMutex + feeCache map[string]feeCache +) + +func (c *Client) GetTransactionFeeAndRate(ctx context.Context, rawResult *types.TxRawResult) (int64, int64, error) { + feeCacheMu.RLock() + if cache, ok := feeCache[rawResult.Txid]; ok && time.Now().Before(cache.expires) { + feeCacheMu.RUnlock() + return cache.fee, cache.feeRate, nil + } + feeCacheMu.RUnlock()
158-186: Enhance health check with more comprehensive metrics.The Healthcheck function could provide more detailed health metrics.
+type HealthStatus struct { + BlockHeight int64 + BlockTime time.Time + UTXOCount int + TotalBalance float64 + NetworkLatency time.Duration +} + +func (c *Client) Healthcheck(ctx context.Context, tssAddress btcutil.Address) (*HealthStatus, error) {e2e/e2etests/test_crosschain_swap.go (1)
162-162: Enhance error assertions in the test.The test should verify specific error conditions when the transaction fails.
- txraw, err := r.BtcRPCClient.GetRawTransactionVerbose(r.Ctx, outboundHash) - require.NoError(r, err) + txraw, err := r.BtcRPCClient.GetRawTransactionVerbose(r.Ctx, outboundHash) + require.NoError(r, err, "failed to get raw transaction: %v", err) + require.NotNil(r, txraw, "transaction should not be nil") + require.NotEmpty(r, txraw.Txid, "transaction ID should not be empty")zetaclient/context/app.go (1)
169-170: Consider usingslices.Sortin a single line.The sorting operations can be combined for better readability.
-slices.Sort(freshChainIDs) -slices.Sort(existingChainIDs) +slices.Sort(freshChainIDs); slices.Sort(existingChainIDs)zetaclient/metrics/metrics.go (1)
211-219: Consider adjusting histogram buckets for RPC latency.The current buckets might not capture very fast RPC calls effectively.
- Buckets: []float64{0.05, 0.1, 0.2, 0.3, 0.5, 1, 1.5, 2, 3, 5, 7.5, 10, 15}, // 50ms to 15s + Buckets: []float64{0.001, 0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10}, // 1ms to 10szetaclient/chains/bitcoin/signer/signer_test.go (2)
33-35: Document the purpose ofcWrapper.Add a comment explaining why this wrapper is needed and its role in testing.
+// cWrapper wraps the test suite's *C type to provide a no-op Cleanup method +// required by the test infrastructure. type cWrapper struct{ *C } func (cWrapper) Cleanup(func()) { /* noop */ }
53-58: Consider extracting common test setup logic.Both signer instantiations share similar patterns. Consider creating a helper function.
+func newTestSigner(t testing.TB, chain chains.Chain, tss TSS) *Signer { + return New( + chain, + tss, + mocks.NewBitcoinClient(t), + base.DefaultLogger(), + ) +}Also applies to: 232-236
zetaclient/chains/bitcoin/observer/witness_test.go (3)
54-55: Consider adding context timeout for tests.While using
context.Background()works, consider usingcontext.WithTimeoutto ensure tests don't hang indefinitely in case of issues.- ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel()
194-194: Reduce duplication in mock client creation.Consider extracting mock client creation into a test helper function to improve maintainability and reduce code duplication.
func createMockBitcoinClient(t *testing.T) *mocks.BitcoinClient { return mocks.NewBitcoinClient(t) }Also applies to: 214-214, 235-235, 255-255, 284-284
Line range hint
54-305: Add test case for context cancellation.The test suite should include a case that verifies the behavior when the context is cancelled, ensuring proper resource cleanup and error handling.
func TestGetBtcEventWithWitness_ContextCancellation(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() // Cancel immediately rpcClient := mocks.NewBitcoinClient(t) _, err := observer.GetBtcEventWithWitness( ctx, rpcClient, btcjson.TxRawResult{}, "testAddress", 0, log.Logger, &chaincfg.MainNetParams, mockDepositFeeCalculator(0, nil), ) require.ErrorIs(t, err, context.Canceled) }zetaclient/chains/bitcoin/observer/inbound.go (1)
Line range hint
230-241: Improve error handling and logging in FilterAndParseIncomingTx.Consider adding more detailed error handling and logging to help with debugging and monitoring:
- Log skipped transactions
- Return explicit errors for validation failures
event, err := GetBtcEvent(ctx, rpc, tx, tssAddress, blockNumber, logger, netParams, common.CalcDepositorFee) if err != nil { + logger.Debug(). + Str("txid", tx.Txid). + Err(err). + Msg("Skipping transaction due to error") // unable to parse the tx, the caller should retry return nil, errors.Wrapf(err, "error getting btc event for tx %s in block %d", tx.Txid, blockNumber) } if event != nil { events = append(events, event) logger.Info().Msgf("FilterAndParseIncomingTx: found btc event for tx %s in block %d", tx.Txid, blockNumber) +} else { + logger.Debug(). + Str("txid", tx.Txid). + Msg("Skipping transaction: no valid event found") }e2e/runner/bitcoin.go (1)
Line range hint
32-45: Extract magic numbers into named constants and improve UTXO filtering.The UTXO filtering logic uses a magic number and could be more explicit.
+const ( + // MinTestUTXOAmount minimum UTXO amount required for testing + MinTestUTXOAmount = 1.0 +) // filter big-enough UTXOs for test if running on Regtest if r.IsLocalBitcoin() { - utxosFiltered := []btcjson.ListUnspentResult{} - for _, utxo := range utxos { - if utxo.Amount >= 1.0 { - utxosFiltered = append(utxosFiltered, utxo) - } - } + utxosFiltered := filterUTXOsByAmount(utxos, MinTestUTXOAmount) return utxosFiltered, nil } +func filterUTXOsByAmount(utxos []btcjson.ListUnspentResult, minAmount float64) []btcjson.ListUnspentResult { + filtered := make([]btcjson.ListUnspentResult, 0, len(utxos)) + for _, utxo := range utxos { + if utxo.Amount >= minAmount { + filtered = append(filtered, utxo) + } + } + return filtered +}e2e/runner/runner.go (1)
Line range hint
71-89: Improve E2ERunner struct organization using field grouping.Consider organizing the struct fields into logical groups using embedded structs for better maintainability and readability.
type ClientConnections struct { ZEVMClient *ethclient.Client EVMClient *ethclient.Client BtcRPCClient *btcclient.Client SolanaClient *rpc.Client } type E2ERunner struct { // Group related fields Connections ClientConnections // Other field groups... // Keep mutex at the end as per Go conventions mutex sync.Mutex }zetaclient/chains/bitcoin/client/client_test.go (3)
32-34: Consider extracting hardcoded URLs to constants.The hardcoded URLs in comments should be extracted to package-level constants to improve maintainability.
+const ( + btcMainnetAPIURL = "https://btc.nownodes.io/<token>" + btcTestnetAPIURL = "https://btc-testnet.nownodes.io/<token>" +)
74-75: Replace magic number with a named constant.The block number
879_088should be extracted to a constant with a descriptive name for better readability and maintainability.+const testBlockHeight = 879_088 -require.True(t, bn > 879_088) +require.True(t, bn > testBlockHeight)
235-264: Consider using table-driven tests for fee rate comparisons.The fee rate comparison tests could be refactored to use table-driven tests for better test coverage and maintainability.
+func TestBitcoinFeeRate(t *testing.T) { + tests := []struct { + name string + blockTarget int64 + mode *types.EstimateSmartFeeMode + wantHigher bool + }{ + { + name: "conservative rate higher than economical for 1 block", + blockTarget: 1, + mode: &types.EstimateModeConservative, + wantHigher: true, + }, + // Add more test cases + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Test implementation + }) + } +}changelog.md (2)
8-8: Add more context to the Bitcoin RPC implementation entry.The changelog entry could be more descriptive about the specific improvements in performance and observability.
Consider expanding the entry to:
-* [3349](https://github.com/zeta-chain/node/pull/3349) - implement new bitcoin rpc in zetaclient with improved performance and observability. +* [3349](https://github.com/zeta-chain/node/pull/3349) - Implement new Bitcoin RPC in zetaclient: + - Enhanced performance through optimized request handling + - Improved observability with detailed metrics and logging + - Added context-aware RPC calls for better timeout management
Line range hint
1-24: Improve changelog organization for better readability.The unreleased section could be better organized to help users quickly understand the impact of changes.
Consider:
- Adding a brief summary at the top highlighting major changes
- Grouping related changes together
- Adding subsections for breaking changes if any
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
changelog.md(1 hunks)cmd/zetaclientd/inbound.go(2 hunks)cmd/zetae2e/config/clients.go(2 hunks)e2e/config/config.go(2 hunks)e2e/e2etests/helpers.go(1 hunks)e2e/e2etests/test_crosschain_swap.go(1 hunks)e2e/runner/accounting.go(1 hunks)e2e/runner/balances.go(1 hunks)e2e/runner/bitcoin.go(7 hunks)e2e/runner/clients.go(1 hunks)e2e/runner/runner.go(2 hunks)e2e/runner/setup_bitcoin.go(3 hunks)zetaclient/chains/bitcoin/client/client.go(1 hunks)zetaclient/chains/bitcoin/client/client_test.go(1 hunks)zetaclient/chains/bitcoin/client/commands.go(1 hunks)zetaclient/chains/bitcoin/client/helpers.go(1 hunks)zetaclient/chains/bitcoin/client/mockgen.go(1 hunks)zetaclient/chains/bitcoin/common/fee.go(7 hunks)zetaclient/chains/bitcoin/observer/inbound.go(13 hunks)zetaclient/chains/bitcoin/observer/inbound_test.go(25 hunks)zetaclient/chains/bitcoin/observer/observer.go(15 hunks)zetaclient/chains/bitcoin/observer/observer_test.go(10 hunks)zetaclient/chains/bitcoin/observer/outbound.go(4 hunks)zetaclient/chains/bitcoin/observer/outbound_test.go(2 hunks)zetaclient/chains/bitcoin/observer/rpc_status.go(1 hunks)zetaclient/chains/bitcoin/observer/witness.go(4 hunks)zetaclient/chains/bitcoin/observer/witness_test.go(10 hunks)zetaclient/chains/bitcoin/rpc/rpc.go(0 hunks)zetaclient/chains/bitcoin/rpc/rpc_live_test.go(0 hunks)zetaclient/chains/bitcoin/signer/signer.go(6 hunks)zetaclient/chains/bitcoin/signer/signer_test.go(3 hunks)zetaclient/chains/interfaces/interfaces.go(0 hunks)zetaclient/context/app.go(2 hunks)zetaclient/metrics/metrics.go(1 hunks)zetaclient/orchestrator/bootstrap_test.go(0 hunks)zetaclient/orchestrator/v2_bootstrap.go(3 hunks)zetaclient/orchestrator/v2_orchestrator_test.go(0 hunks)zetaclient/testutils/mocks/bitcoin_client.go(1 hunks)zetaclient/testutils/mocks/btc_rpc.go(0 hunks)zetaclient/testutils/testrpc/rpc_btc.go(4 hunks)
💤 Files with no reviewable changes (6)
- zetaclient/orchestrator/v2_orchestrator_test.go
- zetaclient/orchestrator/bootstrap_test.go
- zetaclient/chains/interfaces/interfaces.go
- zetaclient/chains/bitcoin/rpc/rpc_live_test.go
- zetaclient/chains/bitcoin/rpc/rpc.go
- zetaclient/testutils/mocks/btc_rpc.go
🧰 Additional context used
📓 Path-based instructions (33)
e2e/e2etests/test_crosschain_swap.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/clients.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/metrics/metrics.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/config/config.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/accounting.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/helpers.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/runner.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/rpc_status.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/context/app.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/signer/signer_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/bitcoin.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
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.
e2e/runner/setup_bitcoin.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/balances.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/orchestrator/v2_bootstrap.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetae2e/config/clients.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.
cmd/zetaclientd/inbound.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/testutils/testrpc/rpc_btc.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/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/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/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/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/inbound.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.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/client/commands.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/observer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
📓 Learnings (1)
zetaclient/chains/bitcoin/observer/inbound.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#2899
File: zetaclient/chains/bitcoin/observer/inbound.go:131-132
Timestamp: 2024-11-12T13:20:12.658Z
Learning: ObserveInbound coverage will be improved in future refactor.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: start-e2e-test / e2e
🔇 Additional comments (24)
zetaclient/chains/bitcoin/observer/outbound.go (2)
150-150: LGTM: Block height retrieval implementation looks correct.The change from
GetBlockHeightByHashtoGetBlockHeightByStrmaintains the same functionality while following the new RPC interface.
486-486: LGTM: Raw transaction retrieval implementation is correct.The change from
GetRawTxResulttoGetRawTransactionResultmaintains the same functionality while following the new RPC interface.zetaclient/chains/bitcoin/observer/observer_test.go (1)
174-176: LGTM! Comprehensive test coverage for block cache operations.The mock setup and test cases properly handle context propagation and cover both cache hit and miss scenarios.
Also applies to: 179-179, 185-185
zetaclient/chains/bitcoin/observer/outbound_test.go (1)
37-38: Verify mock client setup aligns with production usage.The mock client initialization has been updated to use
NewBitcoinClientinstead of the previous implementation. Ensure that the mock's behavior matches the production client's interface.✅ Verification successful
Mock client setup verified and aligned with codebase patterns.
The mock initialization and expectation setup follows the established patterns used consistently across the Bitcoin-related test files in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for BitcoinClient interface definition and usage ast-grep --pattern 'type BitcoinClient interface { $$$ }' # Search for production usage patterns rg -A 5 'NewBitcoinClient'Length of output: 12955
cmd/zetae2e/config/clients.go (1)
78-98: Correct Implementation of Bitcoin Client InitializationThe
getBtcClientfunction has been updated properly to initialize the new Bitcoin client using the provided configuration. The mapping betweene2eConfig.Paramsand the correspondingchains.Chainvalues is accurate, and error handling for invalid parameters is appropriately implemented.zetaclient/chains/bitcoin/client/client.go (1)
147-179: Robust HTTP Request Handling insendRequestMethodThe
sendRequestmethod correctly handles HTTP requests and responses, including appropriate error checking and status code verification. The inclusion of detailed logging and metrics recording enhances observability and aids in troubleshooting.zetaclient/chains/bitcoin/common/fee.go (3)
59-65: Introduction of Context-Aware RPC InterfaceThe addition of the
RPCinterface with context-aware methods improves the modularity and control over RPC calls, enabling better cancellation and timeout management. This change enhances the robustness of the fee calculation mechanisms.
Line range hint
233-245: Proper Context Integration in Fee CalculationThe
CalcDepositorFeefunction has been refactored to accept acontext.Contextand the newRPCinterface, aligning with best practices for context propagation in Go. This adjustment ensures that long-running RPC calls can be managed more effectively.
Line range hint
258-281: Enhanced Fee Rate Retrieval with ContextThe
GetRecentFeeRatefunction now incorporates acontext.Contextparameter and utilizes the updatedRPCinterface. This change improves the function's ability to handle cancellations and timeouts during RPC interactions, which is vital for reliable fee estimation.zetaclient/chains/bitcoin/signer/signer.go (2)
49-52: Introduction of theRPCinterface enhances abstraction and testabilityThe addition of the
RPCinterface abstracts the RPC client interactions, improving modularity and facilitating unit testing by allowing for easy mocking of RPC calls.
Line range hint
259-273: Inclusion of context inBroadcastmethod improves control flowAdding the
ctxparameter to theBroadcastmethod enhances resource management by allowing call cancellation and timeout control.zetaclient/chains/bitcoin/observer/rpc_status.go (1)
10-10: LGTM! Good context propagation.The addition of context parameter enables proper request lifecycle management and cancellation support.
zetaclient/orchestrator/v2_bootstrap.go (2)
32-35: LGTM! Clean client initialization.The new client initialization with proper error handling and logger injection follows best practices.
67-67: Consider error handling for signer initialization.The signer initialization doesn't include error handling. Consider whether error conditions should be handled here.
e2e/runner/clients.go (1)
23-23: LGTM! Clean type transition.The transition to the new
btcclient.Clienttype aligns with the new context-aware implementation.zetaclient/testutils/testrpc/rpc_btc.go (1)
35-35: Verify mainnet configuration.Setting "mainnet" as default RPCParams might not be suitable for all test scenarios.
Consider parameterizing this value based on test requirements.
e2e/e2etests/helpers.go (1)
Line range hint
62-78: Consider sanitizing sensitive transaction information in logs.The current logging implementation might expose sensitive transaction details. Consider implementing log sanitization for production environments.
zetaclient/chains/bitcoin/observer/witness.go (1)
21-22: LGTM! Good improvement in function signature.The addition of context and RPC interface parameters enhances cancellation handling and testability.
e2e/runner/balances.go (1)
165-165: LGTM! Context handling improvement.The addition of context parameter aligns with the PR's objective of improving context handling for RPC calls.
zetaclient/metrics/metrics.go (1)
202-209: LGTM! Well-structured RPC call counter.The metric is well-defined with appropriate labels for status, client, and method tracking.
e2e/config/config.go (1)
109-112: LGTM! Configuration structure simplified.The removal of
HTTPPostModeandDisableTLSfields streamlines the configuration by moving these implementation details into the client.Run the following script to verify no remaining references to the removed fields:
✅ Verification successful
Confirmed: Configuration changes are isolated and safe
The removed fields (
HTTPPostModeandDisableTLS) fromBitcoinRPCstruct ine2e/config/config.godon't have any impact on the codebase. The only occurrences found are in test code using a different struct (rpcclient.ConnConfig).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to removed fields rg -i "HTTPPostMode|DisableTLS" --type goLength of output: 141
Script:
#!/bin/bash # Check imports and context of the test file echo "=== File imports ===" rg -A 5 "^import" pkg/proofs/headers_test.go echo -e "\n=== Field usage context ===" rg -B 5 -A 5 "HTTPPostMode|DisableTLS" pkg/proofs/headers_test.goLength of output: 630
zetaclient/chains/bitcoin/observer/inbound_test.go (1)
223-224: LGTM! Context parameter consistently added.The addition of context parameters aligns with the new context-aware design, improving request lifecycle management.
Also applies to: 277-278, 707-708
zetaclient/testutils/mocks/bitcoin_client.go (1)
1-2: LGTM! Auto-generated mock file.This is an auto-generated mock file. No manual modifications should be made. Update the source interface if changes are needed.
changelog.md (1)
Line range hint
26-1000: LGTM! Historical changelog entries are well-structured.The historical entries follow a consistent format and provide good traceability through PR links.
| body := fmt.Sprintf( | ||
| `{"jsonrpc":"%s","id":%d,"method":"%s","params":%s}`, | ||
| rpcVersion, | ||
| commandID, | ||
| method, | ||
| paramsBytes, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use JSON marshalling instead of string formatting for request body construction
Constructing the JSON-RPC request body using fmt.Sprintf can lead to injection vulnerabilities and parsing errors if method or params contain special characters. Employ JSON marshalling to ensure correctness and security.
Apply this diff to modify the request construction:
-body := fmt.Sprintf(
- `{"jsonrpc":"%s","id":%d,"method":"%s","params":%s}`,
- rpcVersion,
- commandID,
- method,
- paramsBytes,
-)
+requestBody := struct {
+ JSONRPC string ` + "`json:\"jsonrpc\"`" + `
+ ID int ` + "`json:\"id\"`" + `
+ Method string ` + "`json:\"method\"`" + `
+ Params []json.RawMessage ` + "`json:\"params\"`" + `
+}{
+ JSONRPC: rpcVersion,
+ ID: commandID,
+ Method: method,
+ Params: params,
+}
+
+bodyBytes, err := json.Marshal(requestBody)
+if err != nil {
+ return nil, errors.Wrap(err, "unable to marshal request body")
+}This change ensures that all fields are properly escaped and encoded, eliminating the risk of malformed JSON.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (2)
e2e/runner/accounting.go (1)
Line range hint
102-111: Consider using integer arithmetic for Bitcoin amounts.Using floating-point arithmetic for Bitcoin amounts can lead to precision issues. Consider using
btcutil.Amount(int64) for satoshi calculations instead of float64.- tssTotalBalance := float64(0) + tssTotalBalance := btcutil.Amount(0) for _, tssAddress := range allTssAddress.TssList { btcTssAddress, err := zetacrypto.GetTssAddrBTC(tssAddress.TssPubkey, r.BitcoinParams) if err != nil { continue } utxos, err := r.BtcRPCClient.ListUnspent(r.Ctx) if err != nil { continue } for _, utxo := range utxos { if utxo.Address == btcTssAddress { - tssTotalBalance += utxo.Amount + amountSats, err := btcutil.NewAmount(utxo.Amount) + if err != nil { + continue + } + tssTotalBalance += amountSats } } }e2e/runner/bitcoin.go (1)
Line range hint
261-310: Consider breaking down the transaction handling into smaller functions.The transaction creation, signing, and submission logic could be split into separate functions for better maintainability and error handling.
+func (r *E2ERunner) createAndSignTransaction(ctx context.Context, inputs []btcjson.TransactionInput, amountMap map[btcutil.Address]btcutil.Amount, memo []byte) (*wire.MsgTx, error) { + tx, err := r.BtcRPCClient.CreateRawTransaction(ctx, inputs, amountMap, nil) + if err != nil { + return nil, fmt.Errorf("create transaction: %w", err) + } + + // Add memo output... + + stx, signed, err := r.BtcRPCClient.SignRawTransactionWithWallet2(ctx, tx, inputsForSign) + if err != nil { + return nil, fmt.Errorf("sign transaction: %w", err) + } + if !signed { + return nil, fmt.Errorf("transaction not signed") + } + + return stx, nil +} +func (r *E2ERunner) submitAndConfirmTransaction(ctx context.Context, tx *wire.MsgTx) (*chainhash.Hash, error) { + txid, err := r.BtcRPCClient.SendRawTransaction(ctx, tx, true) + if err != nil { + return nil, fmt.Errorf("send transaction: %w", err) + } + + // Generate blocks and get confirmation... + + return txid, nil +}
🧹 Nitpick comments (34)
zetaclient/chains/bitcoin/observer/observer_test.go (1)
208-208: Consider moving context initialization to test suite setupWhile the context handling is correct, consider moving the context initialization to the test suite setup for better reusability and consistency across tests.
func Test_LoadLastBlockScanned(t *testing.T) { // use Bitcoin mainnet chain for testing chain := chains.BitcoinMainnet - ctx := context.Background() t.Run("should load last block scanned", func(t *testing.T) { // create observer and write 199 as last block scanned ob := newTestSuite(t, chain) ob.WriteLastBlockScannedToDB(199) // load last block scanned - err := ob.LoadLastBlockScanned(ctx) + err := ob.LoadLastBlockScanned(ob.ctx)Also applies to: 216-216, 230-230, 242-242, 245-245, 253-253
zetaclient/testutils/testrpc/rpc_btc.go (1)
61-61: Consider returning an interface instead of concrete mock typeWhile the implementation changes are correct, returning a concrete mock type (
*mocks.BitcoinClient) instead of an interface could make tests more brittle and harder to maintain. Consider defining and returning an interface that specifies only the methods needed for testing.Example interface definition:
type BTCTestClient interface { GetRawTransaction(ctx context.Context, hash *chainhash.Hash) (*btcutil.Tx, error) // Add other required methods }Then update the return type:
-func CreateBTCRPCAndLoadTx(t *testing.T, dir string, chainID int64, txHashes ...string) *mocks.BitcoinClient { +func CreateBTCRPCAndLoadTx(t *testing.T, dir string, chainID int64, txHashes ...string) BTCTestClient {Also applies to: 63-63, 76-76
zetaclient/chains/bitcoin/common/fee.go (1)
Line range hint
265-278: Add safety check to prevent negative block heights when calculating fee rates.In the loop starting at line 274, consider adding a condition to ensure
blockNumber - iremains non-negative. This will prevent potential errors if the current block height is less thanfeeRateCountBackBlocks.Apply this diff to implement the safety check:
for i := int64(0); i < feeRateCountBackBlocks; i++ { + if blockNumber - i < 0 { + break + } // get the block hash, err := rpc.GetBlockHash(ctx, blockNumber - i) if err != nil { return 0, err }zetaclient/chains/bitcoin/observer/inbound.go (4)
Line range hint
62-70: Ensure Correct Context Usage in Cached Block RetrievalIn
GetBlockByNumberCached, verify that the contextctxis appropriately utilized within the caching mechanism and any underlying RPC calls to handle cancellations and deadlines effectively.
148-151: Enhance Error Handling for Transaction RetrievalWhen retrieving the raw transaction using
GetRawTransactionVerbose, consider handling specific error cases, such as transaction not found (ErrRPCNoTxInfo), to provide more informative error messages and improve debugging.Apply this diff to enhance error handling:
tx, err := ob.rpc.GetRawTransactionVerbose(ctx, hash) if err != nil { + if rpcErr, ok := err.(*btcjson.RPCError); ok && rpcErr.Code == btcjson.ErrRPCNoTxInfo { + return "", fmt.Errorf("transaction %s not found: %w", txHash, err) + } return "", err }
Line range hint
290-302: Optimize Network Parameter Handling in Event RetrievalThe conditional checks for network parameters in
GetBtcEventcan be refactored for better maintainability. Consider implementing a strategy pattern or using a map of handlers based on network names to streamline the logic.
Line range hint
397-414: Reduce Redundant RPC Calls in Sender Address RetrievalIn
GetSenderAddressByVin, fetching the entire previous transaction can be resource-intensive. Optimize by retrieving only the necessary output information if the RPC API supports it, or implement caching for frequently accessed transactions.zetaclient/chains/bitcoin/signer/signer.go (2)
Line range hint
344-350: Handle Network Information Retrieval Failures GracefullyWhen fetching network information with
GetNetworkInfo, consider implementing retry logic or more detailed error handling to gracefully manage transient network issues or RPC failures.
Line range hint
405-414: Enhanced Error Reporting on Transaction Broadcast FailuresIn
TryProcessOutbound, upon broadcasting failures, enrich the error logging with context such as the outbound transaction details and the number of retries attempted to facilitate easier troubleshooting.zetaclient/chains/bitcoin/client/commands.go (1)
137-169: Clarify Fee Handling inSendRawTransactionThe logic for setting
maxFeeRatebased onallowHighFeesis appropriate. For clarity, document the implications of this parameter to ensure that fee-related errors are understood and handled correctly by the caller.zetaclient/chains/bitcoin/observer/observer.go (3)
31-66: Consider Organizing Methods Within theRPCInterfaceThe
RPCinterface contains a broad set of methods related to Bitcoin RPC functionalities. For better readability and maintainability, consider grouping related methods together and adding method documentation to clarify their purposes and usage.
182-184: Make Context Timeout inNewObserverConfigurableThe timeout value of 5 seconds in the
NewObserverfunction is hardcoded. To enhance flexibility across different operational environments, consider making the timeout duration configurable.Apply this diff to introduce a configurable timeout:
-func NewObserver( - chain chains.Chain, - rpc RPC, - chainParams observertypes.ChainParams, - zetacoreClient interfaces.ZetacoreClient, - tss interfaces.TSSSigner, - database *db.DB, - logger base.Logger, - ts *metrics.TelemetryServer, -) (*Observer, error) { - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) +func NewObserver( + chain chains.Chain, + rpc RPC, + chainParams observertypes.ChainParams, + zetacoreClient interfaces.ZetacoreClient, + tss interfaces.TSSSigner, + database *db.DB, + logger base.Logger, + ts *metrics.TelemetryServer, + timeout time.Duration, +) (*Observer, error) { + ctx, cancel := context.WithTimeout(context.Background(), timeout)
439-439: Correct Formatting in Error MessageThere is an extra space at the beginning of the error message in the
specialHandleFeeRatefunction. Remove the space to maintain consistent error message formatting.Apply this diff to correct the error message:
- return 0, fmt.Errorf(" unsupported bitcoin network type %d", ob.Chain().NetworkType) + return 0, fmt.Errorf("unsupported bitcoin network type %d", ob.Chain().NetworkType)zetaclient/orchestrator/v2_bootstrap.go (1)
32-32: Pass Contextual Logger to RPC ClientWhen creating the
rpcClient, consider passing a context-aware logger to enhance logging capabilities and provide more informative logs that include request context.e2e/e2etests/helpers.go (1)
62-63: Consider wrapping the error for better context.While the context addition is good, consider wrapping the error to provide more context about the operation:
- rawTx, err := r.BtcRPCClient.GetRawTransactionVerbose(r.Ctx, hash) - require.NoError(r, err) + rawTx, err := r.BtcRPCClient.GetRawTransactionVerbose(r.Ctx, hash) + if err != nil { + return fmt.Errorf("failed to get raw transaction %s: %w", hash, err) + }e2e/runner/setup_bitcoin.go (1)
101-104: Extract database error message as a constant.Consider extracting the error message to a package-level constant for better maintainability:
+const errDatabaseExists = "Database already exists" _, err := r.BtcRPCClient.RawRequest(r.Ctx, "createwallet", argsRawMsg) if err != nil { - require.ErrorContains(r, err, "Database already exists") + require.ErrorContains(r, err, errDatabaseExists) }zetaclient/chains/bitcoin/client/mockgen.go (2)
19-86: Enhance interface documentation with method groupings.Consider adding documentation comments to group related methods for better readability:
type client interface { + // Health and Network Operations Ping(ctx context.Context) error Healthcheck(ctx context.Context, tssAddress btcutil.Address) (time.Time, error) GetNetworkInfo(ctx context.Context) (*types.GetNetworkInfoResult, error) + // Block Operations GetBlockCount(ctx context.Context) (int64, error) GetBlockHash(ctx context.Context, blockHeight int64) (*hash.Hash, error) // ... rest of the methods ...Also, consider adding godoc comments for each method describing its purpose and any specific behavior notes.
15-18: Document the rationale for unexported interface.While the comment explains that the interface is unexported for mock generation, it would be helpful to document why this approach was chosen over alternatives.
-// client represents interface version of Client. -// It's unexported on purpose ONLY for mock generation. +// client represents the interface version of Client. +// It's unexported to prevent external packages from implementing this interface +// directly, while still allowing mock generation. This ensures that all Bitcoin +// client implementations go through our concrete Client type, maintaining +// consistent behavior across the codebase.cmd/zetae2e/config/clients.go (1)
78-98: Consider enhancing error handling and modularity.The implementation could benefit from the following improvements:
- Make the error message in the default case more descriptive by including valid parameter options.
- Extract chain type determination to a separate function for better maintainability.
func getBtcClient(e2eConfig config.BitcoinRPC) (*btcclient.Client, error) { cfg := zetaclientconfig.BTCConfig{ RPCUsername: e2eConfig.User, RPCPassword: e2eConfig.Pass, RPCHost: e2eConfig.Host, RPCParams: string(e2eConfig.Params), } + + chain, err := getBitcoinChain(e2eConfig.Params) + if err != nil { + return nil, err + } + + return btcclient.New(cfg, chain.ChainId, zerolog.Nop()) +} + +func getBitcoinChain(params config.BitcoinParams) (chains.Chain, error) { switch params { case config.Regnet: - chain = chains.BitcoinRegtest + return chains.BitcoinRegtest, nil case config.Testnet3: - chain = chains.BitcoinTestnet + return chains.BitcoinTestnet, nil case config.Mainnet: - chain = chains.BitcoinMainnet + return chains.BitcoinMainnet, nil default: - return nil, fmt.Errorf("invalid bitcoin params %s", e2eConfig.Params) + return chains.Chain{}, fmt.Errorf("invalid bitcoin params %s, expected one of: %s, %s, %s", + params, config.Regnet, config.Testnet3, config.Mainnet) } - - return btcclient.New(cfg, chain.ChainId, zerolog.Nop()) }zetaclient/chains/bitcoin/client/helpers.go (4)
14-62: Reduce error handling duplication in string conversion helpers.Consider consolidating the error handling pattern by making the helper functions more concise.
func (c *Client) GetBlockVerboseByStr(ctx context.Context, blockHash string) (*types.GetBlockVerboseTxResult, error) { h, err := strToHash(blockHash) if err != nil { - return nil, err + return nil, fmt.Errorf("get block verbose: %w", err) } return c.GetBlockVerbose(ctx, h) } func (c *Client) GetBlockHeightByStr(ctx context.Context, blockHash string) (int64, error) { h, err := strToHash(blockHash) if err != nil { - return 0, err + return 0, fmt.Errorf("get block height: %w", err) } res, err := c.GetBlockVerbose(ctx, h) if err != nil { return 0, errors.Wrap(err, "unable to get block verbose") } return res.Height, nil }
64-105: Enhance code organization in GetRawTransactionResult.Consider using constants and early returns to improve readability and maintainability.
+const ( + // TxNotConfirmed indicates a transaction with zero confirmations + TxNotConfirmed = 0 + // TxNotIncluded indicates a transaction not included in any block + TxNotIncluded = -1 +) + func (c *Client) GetRawTransactionResult(ctx context.Context, hash *chainhash.Hash, res *types.GetTransactionResult, ) (types.TxRawResult, error) { - switch { - case res.Confirmations == 0: + if res.Confirmations == TxNotConfirmed { // for pending tx, we query the raw tx rawResult, err := c.GetRawTransactionVerbose(ctx, hash) - if err != nil { - return types.TxRawResult{}, errors.Wrapf(err, "unable to get raw tx verbose %s", res.TxID) - } - return *rawResult, nil - case res.Confirmations > 0: + return *rawResult, errors.Wrapf(err, "unable to get raw tx verbose %s", res.TxID) + } + + if res.Confirmations > 0 {
107-156: Add input validation in GetTransactionFeeAndRate.Consider adding validation for nil input and empty transaction details.
func (c *Client) GetTransactionFeeAndRate(ctx context.Context, rawResult *types.TxRawResult) (int64, int64, error) { + if rawResult == nil { + return 0, 0, errors.New("nil transaction result") + } + if len(rawResult.Vin) == 0 { + return 0, 0, fmt.Errorf("transaction %s has no inputs", rawResult.Txid) + }
158-186: Consider parameterizing UTXO range in Healthcheck.The hardcoded UTXO range (0-1000000) should be configurable to accommodate different use cases.
+// DefaultMinConfirmations is the default minimum number of confirmations for UTXOs +const DefaultMinConfirmations = 0 +// DefaultMaxConfirmations is the default maximum number of confirmations for UTXOs +const DefaultMaxConfirmations = 1000000 + -func (c *Client) Healthcheck(ctx context.Context, tssAddress btcutil.Address) (time.Time, error) { +func (c *Client) Healthcheck(ctx context.Context, tssAddress btcutil.Address, minConf, maxConf int) (time.Time, error) { + if minConf < 0 { + minConf = DefaultMinConfirmations + } + if maxConf <= 0 { + maxConf = DefaultMaxConfirmations + }e2e/e2etests/test_crosschain_swap.go (1)
162-162: LGTM! Context parameter addition is aligned with the new RPC client design.The addition of context parameter enhances request lifecycle management and cancellation capabilities.
Consider extracting the context timeout into a constant or configuration value for better maintainability:
const btcRPCTimeout = 30 * time.Second ctx, cancel := context.WithTimeout(r.Ctx, btcRPCTimeout) defer cancel() txraw, err := r.BtcRPCClient.GetRawTransactionVerbose(ctx, outboundHash)zetaclient/context/app.go (1)
Line range hint
238-248: Consider usingslices.Equalfromgolang.org/x/exp/slices.The custom
slicesEqualimplementation could be replaced with the standard library's implementation for better maintainability and potential performance improvements.import "golang.org/x/exp/slices" // Replace custom implementation with: if !slices.Equal(existingChainIDs, freshChainIDs) { // ... rest of the code }zetaclient/metrics/metrics.go (1)
211-219: LGTM! Well-configured RPC duration histogram.The duration histogram with appropriate bucket ranges (50ms to 15s) will provide useful latency insights. The labels align well with the counter metric for consistent monitoring.
Consider extracting the bucket values into a constant for reuse:
var rpcDurationBuckets = []float64{0.05, 0.1, 0.2, 0.3, 0.5, 1, 1.5, 2, 3, 5, 7.5, 10, 15} RPCClientDuration = promauto.NewHistogramVec( prometheus.HistogramOpts{ Namespace: ZetaClientNamespace, Name: "rpc_client_duration_seconds", Help: "Histogram of rpc client calls durations in seconds", Buckets: rpcDurationBuckets, }, []string{"status", "client", "method"}, )zetaclient/chains/bitcoin/signer/signer_test.go (1)
33-35: Add documentation for the test wrapper type.Consider adding documentation to explain the purpose of
cWrapperand why itsCleanupmethod is a no-op implementation.+// cWrapper wraps a testing.C pointer and implements a no-op Cleanup method +// to satisfy the testing interface requirements type cWrapper struct{ *C } func (cWrapper) Cleanup(func()) { /* noop */ }zetaclient/chains/bitcoin/observer/witness_test.go (2)
54-55: Consider using context with timeout for tests.While using context is good practice, consider adding a timeout to prevent indefinite test execution in case of issues.
- ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel()
Line range hint
284-302: Consider extracting mock transaction setup into a helper function.The transaction setup logic could be moved to a dedicated helper function to improve readability and reusability.
+func setupMockTransaction(t *testing.T, rpcClient *mocks.BitcoinClient, preHash string, preVout uint32) *btcutil.Tx { + // load archived MsgTx and modify previous input script to invalid + msgTx := testutils.LoadBTCMsgTx(t, TestDataDir, chain.ChainId, preHash) + msgTx.TxOut[preVout].PkScript = []byte{0x00, 0x01} + tx := btcutil.NewTx(msgTx) + + // mock rpc response + rpcClient.On("GetRawTransaction", mock.Anything, mock.Anything).Return(tx, nil) + + return tx +}zetaclient/chains/bitcoin/client/client_test.go (2)
201-209: Improve error handling in fee rate tests.Consider using
require.NoErrorinstead of direct error checks to maintain consistency with other tests and provide better error messages.-if errCon1 != nil { - t.Error(errCon1) -} +require.NoError(t, errCon1) -if errEco1 != nil { - t.Error(errEco1) -} +require.NoError(t, errEco1)
236-238: Consider parameterizing the test duration.The commented sleep duration could be made configurable to support different test scenarios.
+const defaultTestDuration = 5 * time.Minute -// please uncomment this interval for long running test -//time.Sleep(time.Duration(5) * time.Minute) +if duration := os.Getenv("TEST_DURATION"); duration != "" { + if d, err := time.ParseDuration(duration); err == nil { + time.Sleep(d) + } +} else { + time.Sleep(defaultTestDuration) +}zetaclient/chains/bitcoin/observer/inbound_test.go (3)
Line range hint
56-122: Consider using table-driven tests for better organization.While the error test cases are comprehensive, converting them to a table-driven test format would:
- Reduce code duplication
- Make it easier to add new test cases
- Improve readability and maintenance
Example refactor:
-t.Run("block has no transactions", func(t *testing.T) { - emptyVb := btcjson.GetBlockVerboseTxResult{Tx: []btcjson.TxRawResult{}} - _, err := common.CalcBlockAvgFeeRate(&emptyVb, &chaincfg.MainNetParams) - require.Error(t, err) - require.ErrorContains(t, err, "block has no transactions") -}) +tests := []struct { + name string + block func(*btcjson.GetBlockVerboseTxResult) *btcjson.GetBlockVerboseTxResult + wantErr string +}{ + { + name: "block has no transactions", + block: func(b *btcjson.GetBlockVerboseTxResult) *btcjson.GetBlockVerboseTxResult { + return &btcjson.GetBlockVerboseTxResult{Tx: []btcjson.TxRawResult{}} + }, + wantErr: "block has no transactions", + }, + // Add other test cases here +} + +for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + block := tt.block(&blockVb) + _, err := common.CalcBlockAvgFeeRate(block, &chaincfg.MainNetParams) + require.Error(t, err) + require.ErrorContains(t, err, tt.wantErr) + }) +}
Line range hint
222-273: Consider adding context cancellation test cases.While the tests cover the main functionality, they could be enhanced by verifying context handling:
- Test context cancellation
- Test context timeout
- Verify cleanup of resources
Example addition:
+t.Run("should handle context cancellation", func(t *testing.T) { + rpcClient := mocks.NewBitcoinClient(t) + ctx, cancel := context.WithCancel(context.Background()) + cancel() // Cancel immediately + + txVin := btcjson.Vin{Txid: txHash, Vout: 2} + sender, err := observer.GetSenderAddressByVin(ctx, rpcClient, txVin, net) + require.ErrorIs(t, err, context.Canceled) + require.Empty(t, sender) +})
Line range hint
275-622: Reduce code duplication in BTC event tests.The test cases for different address types share similar patterns. Consider extracting common setup and validation logic:
Example refactor:
+type btcEventTestCase struct { + name string + preHash string + preVout uint32 + wantAddress string + setupMock func(*mocks.BitcoinClient) +} + +func runBtcEventTest(t *testing.T, tc btcEventTestCase, tx *btcjson.TxRawResult) { + rpcClient := mocks.NewBitcoinClient(t) + if tc.setupMock != nil { + tc.setupMock(rpcClient) + } + + tx.Vin[0].Txid = tc.preHash + tx.Vin[0].Vout = tc.preVout + + event, err := observer.GetBtcEventWithoutWitness( + ctx, rpcClient, *tx, tssAddress, blockNumber, + log.Logger, net, feeCalculator, + ) + require.NoError(t, err) + require.Equal(t, tc.wantAddress, event.FromAddress) +} + +tests := []btcEventTestCase{ + { + name: "should get BTC inbound event from P2WPKH sender", + preHash: "c5d224963832fc0b9a597251c2342a17b25e481a88cc9119008e8f8296652697", + preVout: 2, + wantAddress: "bc1q68kxnq52ahz5vd6c8czevsawu0ux9nfrzzrh6e", + }, + // Add other test cases +} + +for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + runBtcEventTest(t, tt, tx) + }) +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
changelog.md(1 hunks)cmd/zetaclientd/inbound.go(2 hunks)cmd/zetae2e/config/clients.go(2 hunks)e2e/config/config.go(2 hunks)e2e/e2etests/helpers.go(1 hunks)e2e/e2etests/test_crosschain_swap.go(1 hunks)e2e/runner/accounting.go(1 hunks)e2e/runner/balances.go(1 hunks)e2e/runner/bitcoin.go(7 hunks)e2e/runner/clients.go(1 hunks)e2e/runner/runner.go(2 hunks)e2e/runner/setup_bitcoin.go(3 hunks)zetaclient/chains/bitcoin/client/client.go(1 hunks)zetaclient/chains/bitcoin/client/client_test.go(1 hunks)zetaclient/chains/bitcoin/client/commands.go(1 hunks)zetaclient/chains/bitcoin/client/helpers.go(1 hunks)zetaclient/chains/bitcoin/client/mockgen.go(1 hunks)zetaclient/chains/bitcoin/common/fee.go(7 hunks)zetaclient/chains/bitcoin/observer/inbound.go(13 hunks)zetaclient/chains/bitcoin/observer/inbound_test.go(25 hunks)zetaclient/chains/bitcoin/observer/observer.go(15 hunks)zetaclient/chains/bitcoin/observer/observer_test.go(10 hunks)zetaclient/chains/bitcoin/observer/outbound.go(4 hunks)zetaclient/chains/bitcoin/observer/outbound_test.go(2 hunks)zetaclient/chains/bitcoin/observer/rpc_status.go(1 hunks)zetaclient/chains/bitcoin/observer/witness.go(4 hunks)zetaclient/chains/bitcoin/observer/witness_test.go(10 hunks)zetaclient/chains/bitcoin/rpc/rpc.go(0 hunks)zetaclient/chains/bitcoin/rpc/rpc_live_test.go(0 hunks)zetaclient/chains/bitcoin/signer/signer.go(6 hunks)zetaclient/chains/bitcoin/signer/signer_test.go(3 hunks)zetaclient/chains/interfaces/interfaces.go(0 hunks)zetaclient/context/app.go(2 hunks)zetaclient/metrics/metrics.go(1 hunks)zetaclient/orchestrator/bootstrap_test.go(0 hunks)zetaclient/orchestrator/v2_bootstrap.go(3 hunks)zetaclient/orchestrator/v2_orchestrator_test.go(0 hunks)zetaclient/testutils/mocks/bitcoin_client.go(1 hunks)zetaclient/testutils/mocks/btc_rpc.go(0 hunks)zetaclient/testutils/testrpc/rpc_btc.go(4 hunks)
💤 Files with no reviewable changes (6)
- zetaclient/orchestrator/bootstrap_test.go
- zetaclient/chains/interfaces/interfaces.go
- zetaclient/orchestrator/v2_orchestrator_test.go
- zetaclient/chains/bitcoin/rpc/rpc.go
- zetaclient/chains/bitcoin/rpc/rpc_live_test.go
- zetaclient/testutils/mocks/btc_rpc.go
✅ Files skipped from review due to trivial changes (1)
- changelog.md
🧰 Additional context used
📓 Path-based instructions (33)
zetaclient/metrics/metrics.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/helpers.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/accounting.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.
e2e/runner/runner.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetaclientd/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/rpc_status.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/context/app.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.
e2e/e2etests/test_crosschain_swap.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/config/config.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.
cmd/zetae2e/config/clients.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/balances.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/bitcoin.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
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/orchestrator/v2_bootstrap.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.
e2e/runner/setup_bitcoin.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.
e2e/runner/clients.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/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/outbound.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/observer.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/client/client.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/client/commands.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.
🪛 GitHub Check: codecov/patch
zetaclient/chains/bitcoin/client/client.go
[warning] 73-74: zetaclient/chains/bitcoin/client/client.go#L73-L74
Added lines #L73 - L74 were not covered by tests
[warning] 78-82: zetaclient/chains/bitcoin/client/client.go#L78-L82
Added lines #L78 - L82 were not covered by tests
[warning] 84-100: zetaclient/chains/bitcoin/client/client.go#L84-L100
Added lines #L84 - L100 were not covered by tests
[warning] 102-102: zetaclient/chains/bitcoin/client/client.go#L102
Added line #L102 was not covered by tests
[warning] 106-110: zetaclient/chains/bitcoin/client/client.go#L106-L110
Added lines #L106 - L110 were not covered by tests
[warning] 114-117: zetaclient/chains/bitcoin/client/client.go#L114-L117
Added lines #L114 - L117 were not covered by tests
[warning] 119-124: zetaclient/chains/bitcoin/client/client.go#L119-L124
Added lines #L119 - L124 were not covered by tests
[warning] 127-127: zetaclient/chains/bitcoin/client/client.go#L127
Added line #L127 was not covered by tests
[warning] 130-136: zetaclient/chains/bitcoin/client/client.go#L130-L136
Added lines #L130 - L136 were not covered by tests
[warning] 138-142: zetaclient/chains/bitcoin/client/client.go#L138-L142
Added lines #L138 - L142 were not covered by tests
[warning] 144-144: zetaclient/chains/bitcoin/client/client.go#L144
Added line #L144 was not covered by tests
[warning] 147-156: zetaclient/chains/bitcoin/client/client.go#L147-L156
Added lines #L147 - L156 were not covered by tests
[warning] 158-161: zetaclient/chains/bitcoin/client/client.go#L158-L161
Added lines #L158 - L161 were not covered by tests
[warning] 163-168: zetaclient/chains/bitcoin/client/client.go#L163-L168
Added lines #L163 - L168 were not covered by tests
[warning] 170-172: zetaclient/chains/bitcoin/client/client.go#L170-L172
Added lines #L170 - L172 were not covered by tests
[warning] 174-176: zetaclient/chains/bitcoin/client/client.go#L174-L176
Added lines #L174 - L176 were not covered by tests
[warning] 178-178: zetaclient/chains/bitcoin/client/client.go#L178
Added line #L178 was not covered by tests
[warning] 181-187: zetaclient/chains/bitcoin/client/client.go#L181-L187
Added lines #L181 - L187 were not covered by tests
[warning] 189-190: zetaclient/chains/bitcoin/client/client.go#L189-L190
Added lines #L189 - L190 were not covered by tests
[warning] 193-197: zetaclient/chains/bitcoin/client/client.go#L193-L197
Added lines #L193 - L197 were not covered by tests
[warning] 199-202: zetaclient/chains/bitcoin/client/client.go#L199-L202
Added lines #L199 - L202 were not covered by tests
[warning] 204-204: zetaclient/chains/bitcoin/client/client.go#L204
Added line #L204 was not covered by tests
[warning] 207-212: zetaclient/chains/bitcoin/client/client.go#L207-L212
Added lines #L207 - L212 were not covered by tests
[warning] 214-214: zetaclient/chains/bitcoin/client/client.go#L214
Added line #L214 was not covered by tests
[warning] 217-221: zetaclient/chains/bitcoin/client/client.go#L217-L221
Added lines #L217 - L221 were not covered by tests
[warning] 223-223: zetaclient/chains/bitcoin/client/client.go#L223
Added line #L223 was not covered by tests
[warning] 226-230: zetaclient/chains/bitcoin/client/client.go#L226-L230
Added lines #L226 - L230 were not covered by tests
[warning] 232-232: zetaclient/chains/bitcoin/client/client.go#L232
Added line #L232 was not covered by tests
[warning] 235-248: zetaclient/chains/bitcoin/client/client.go#L235-L248
Added lines #L235 - L248 were not covered by tests
[warning] 252-255: zetaclient/chains/bitcoin/client/client.go#L252-L255
Added lines #L252 - L255 were not covered by tests
🔇 Additional comments (40)
zetaclient/chains/bitcoin/observer/observer_test.go (6)
4-4: LGTM: Context-aware mock setupThe changes correctly implement context-aware mock client initialization, aligning with the PR's objective of enhancing context management.
Also applies to: 73-74
80-80: LGTM: Improved test structure and error messagingThe test structure has been updated to use the new BitcoinClient type, and the error message has been simplified for better clarity.
Also applies to: 105-105
174-176: LGTM: Comprehensive context-aware cache testingThe block cache tests have been properly updated to include context handling, covering both cache miss and hit scenarios effectively.
Also applies to: 179-179, 185-185
304-305: LGTM: Well-structured test suiteThe test suite struct has been properly updated to support context-aware testing with appropriate field ordering.
311-312: LGTM: Comprehensive test suite initializationThe test suite initialization properly sets up all components including context and mock client with appropriate method expectations.
Also applies to: 317-318
340-340: LGTM: Complete context integrationThe test suite properly includes the context in its return value, completing the context-aware testing setup.
zetaclient/testutils/testrpc/rpc_btc.go (2)
11-11: LGTM: Clean import additionThe addition of the testify mock package aligns with the new mocking approach.
35-35: Verify test compatibility with mainnet parameterThe change from empty string to "mainnet" for RPCParams makes the network choice explicit, which is good. However, verify that existing tests aren't affected by this change.
✅ Verification successful
RPCParams change is safe for existing tests
The modification from empty string to "mainnet" is verified to be compatible with existing tests. All test files explicitly set their own network parameters and don't rely on default values.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests that might be affected by the RPCParams change rg -l "RPCParams" --type go --glob "*_test.go"Length of output: 126
Script:
#!/bin/bash # Check RPCParams usage in test files echo "=== config/types_test.go ===" rg -A 5 -B 5 "RPCParams" zetaclient/config/types_test.go echo -e "\n=== bitcoin/client/client_test.go ===" rg -A 5 -B 5 "RPCParams" zetaclient/chains/bitcoin/client/client_test.goLength of output: 1404
zetaclient/chains/bitcoin/common/fee.go (1)
59-65: IntroduceRPCinterface to enhance modularity and testability.The addition of the
RPCinterface is a sound design choice that promotes abstraction and simplifies mocking for tests, thereby improving the modularity of the codebase.zetaclient/chains/bitcoin/observer/inbound.go (4)
26-28: Proper Context Propagation in RPC CallsThe addition of the
ctxparameter toGetBlockCountenhances context management, allowing for better control over request cancellations and timeouts.
Line range hint
77-82: Consistent Context Passing in Function CallsUpdating
FilterAndParseIncomingTxto includectxandrpcparameters improves context handling. Ensure that all internal calls within this function propagate the context consistently to maintain uniformity across RPC interactions.
Line range hint
180-187: Efficient Event Retrieval with Context AwarenessPassing
ctxtoGetBtcEventensures that the event retrieval process respects context cancellations and timeouts, enhancing the robustness of the observer.
214-231: Thread Safety in Event ParsingIn
FilterAndParseIncomingTx, when appending events to theeventsslice, ensure thread safety if the function is used concurrently. Although currently safe, consider synchronization mechanisms if parallel processing is introduced.zetaclient/chains/bitcoin/signer/signer.go (3)
49-52: Abstraction via RPC Interface Enhances TestabilityIntroducing the
RPCinterface decouples theSignerfrom specific client implementations, improving modularity and facilitating easier mocking in unit tests.
60-65: Constructor Update Aligns with Dependency Injection PrinciplesModifying the
Newfunction to accept anRPCinterface allows for greater flexibility and aligns with dependency injection best practices.
Line range hint
259-272: Context-Aware Transaction Broadcasting Enhances ControlAdding
ctxto theBroadcastmethod and propagating it toSendRawTransactionallows for better control over transaction broadcasts, enabling timeouts and cancellation when necessary.zetaclient/chains/bitcoin/client/commands.go (1)
24-26: Efficient Connectivity Check inPingMethodUsing
GetBlockCountwithin thePingmethod is an efficient way to verify the client's connectivity and responsiveness without introducing significant overhead.zetaclient/chains/bitcoin/observer/observer.go (1)
359-359: Verify All Call Sites Updated for Method Signature ChangesThe methods
GetBlockByNumberCachedandLoadLastBlockScannednow accept acontext.Contextparameter. Please ensure that all call sites have been updated accordingly to prevent potential runtime errors.Run the following script to find any usages missing the new context parameter:
Also applies to: 392-392
zetaclient/chains/bitcoin/observer/rpc_status.go (1)
Line range hint
10-24: Method Update with Context ParameterThe
CheckRPCStatusmethod now accepts acontext.Contextparameter and correctly utilizes it in theob.rpc.Healthcheckcall. The error handling logic appropriately manages different node states and potential errors.e2e/runner/clients.go (1)
15-15: LGTM: Clean transition to the new BTC client.The change from
rpcclient.Clienttobtcclient.Clientaligns with the PR's objective of implementing a more efficient, context-aware Bitcoin client.Also applies to: 23-23
cmd/zetaclientd/inbound.go (1)
165-165: LGTM! Clean transition to the new Bitcoin client.The change properly maintains error handling while transitioning to the new client implementation.
zetaclient/context/app.go (1)
169-171: LGTM! Efficient pre-sorting of slices.The sorting of slices before comparison ensures consistent ordering for the equality check.
e2e/runner/balances.go (1)
165-165: LGTM! Context propagation enhances request control.The addition of context parameter aligns with the new Bitcoin RPC client design and enables better request lifecycle management.
zetaclient/metrics/metrics.go (1)
202-209: LGTM! Well-structured RPC call counter metric.The counter metric with labels for status, client, and method provides comprehensive RPC call tracking.
zetaclient/chains/bitcoin/signer/signer_test.go (2)
53-57: LGTM: Clean test setup with mocked dependencies.The signer instantiation is well-structured, using dependency injection for better testability.
232-237: LGTM: Consistent test setup pattern.The test case follows the established pattern for signer instantiation and mock setup.
zetaclient/chains/bitcoin/observer/witness_test.go (1)
Line range hint
194-202: LGTM: Well-structured mock usage with proper context handling.The test properly initializes the mock Bitcoin client and consistently passes the context through the call chain.
🧰 Tools
🪛 Gitleaks (8.21.2)
191-191: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
e2e/runner/bitcoin.go (2)
35-38: LGTM: Proper context propagation in RPC call.The context is correctly passed through to the RPC client method.
Line range hint
373-401: LGTM: Clean implementation of block generation and transaction verification.The functions are well-focused and handle errors appropriately.
e2e/runner/runner.go (1)
Line range hint
50-89: LGTM: Clean transition to new Bitcoin client type.The change to use the new
btcclient.Clienttype is well-organized and maintains clean code structure.e2e/config/config.go (2)
109-112: LGTM! Clean and focused struct definition.The simplified
BitcoinRPCstruct aligns well with the new Bitcoin client implementation, removing redundant configuration fields.
167-170: LGTM! Appropriate default values for local testing.The default configuration values are well-suited for a local testing environment.
zetaclient/chains/bitcoin/observer/outbound_test.go (1)
37-38: LGTM! Updated mock client instantiation.The test now uses the new Bitcoin client mock, aligning with the updated implementation.
zetaclient/chains/bitcoin/observer/outbound.go (4)
150-150: LGTM! Updated RPC call with context.The method now correctly uses the new client implementation with proper context handling.
359-359: LGTM! Updated transaction retrieval method.The code now uses the new client's
GetTransactionByStrmethod with proper context handling.
402-402: LGTM! Consistent use of new client method.The code maintains consistency with the new client implementation.
486-486: LGTM! Updated raw transaction retrieval.The code now uses the new client's
GetRawTransactionResultmethod with proper context handling.zetaclient/chains/bitcoin/observer/inbound_test.go (2)
35-38: Well-structured mock helper function!The implementation provides a clean and reusable way to create mock fee calculators for testing different scenarios.
Line range hint
40-54: Excellent test implementation using real-world data!The test effectively validates fee rate calculations against actual blockchain data from mempool.space, providing high confidence in the implementation's accuracy.
zetaclient/testutils/mocks/bitcoin_client.go (1)
1-893: Well-structured auto-generated mock implementation.The mock provides comprehensive coverage of the Bitcoin client interface with proper cleanup handling. The implementation includes:
- Complete interface coverage
- Proper error handling
- Clean cleanup mechanism
- Type-safe method implementations
Note: Since this is an auto-generated file, avoid manual modifications. Instead, update the interface definition and regenerate the mock if changes are needed.
| ctx context.Context, | ||
| rpc RPC, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update function documentation to reflect new parameters.
The function GetBtcEventWithWitness now includes ctx context.Context and rpc RPC as parameters. Please update the function's documentation to accurately describe the new signature and parameters to maintain clarity and consistency.
| func New(cfg config.BTCConfig, chainID int64, logger zerolog.Logger, opts ...Opt) (*Client, error) { | ||
| params, err := resolveParams(cfg.RPCParams) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "unable to resolve chain params") | ||
| } | ||
|
|
||
| clientName := fmt.Sprintf("btc:%d", chainID) | ||
|
|
||
| c := &Client{ | ||
| hostURL: normalizeHostURL(cfg.RPCHost, true), | ||
| client: defaultHTTPClient(), | ||
| config: cfg, | ||
| params: params, | ||
| clientName: clientName, | ||
| logger: logger.With(). | ||
| Str(logs.FieldModule, "btc_client"). | ||
| Int64(logs.FieldChain, chainID). | ||
| Logger(), | ||
| } | ||
|
|
||
| for _, opt := range opts { | ||
| opt(c) | ||
| } | ||
|
|
||
| return c, nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Increase unit test coverage for client initialization and command execution.
The New constructor and sendCommand method are essential for client functionality but currently lack unit tests. Implementing tests for these functions will improve reliability and facilitate future maintenance.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 78-82: zetaclient/chains/bitcoin/client/client.go#L78-L82
Added lines #L78 - L82 were not covered by tests
[warning] 84-100: zetaclient/chains/bitcoin/client/client.go#L84-L100
Added lines #L84 - L100 were not covered by tests
[warning] 102-102: zetaclient/chains/bitcoin/client/client.go#L102
Added line #L102 was not covered by tests
| if res.StatusCode != http.StatusOK { | ||
| return rawResponse{}, errors.Errorf("unexpected status code %d (%s)", res.StatusCode, resBody) | ||
| } |
There was a problem hiding this comment.
Avoid including full response body in error messages to prevent sensitive data exposure.
Including resBody in the error message may risk logging sensitive information. It is advisable to exclude the response body or sanitize it to prevent potential data leakage.
Apply this diff to address the issue:
- return rawResponse{}, errors.Errorf("unexpected status code %d (%s)", res.StatusCode, resBody)
+ return rawResponse{}, errors.Errorf("unexpected status code %d", res.StatusCode)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if res.StatusCode != http.StatusOK { | |
| return rawResponse{}, errors.Errorf("unexpected status code %d (%s)", res.StatusCode, resBody) | |
| } | |
| if res.StatusCode != http.StatusOK { | |
| return rawResponse{}, errors.Errorf("unexpected status code %d", res.StatusCode) | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 170-172: zetaclient/chains/bitcoin/client/client.go#L170-L172
Added lines #L170 - L172 were not covered by tests
| if err = json.Unmarshal(resBody, &out); err != nil { | ||
| return rawResponse{}, errors.Wrapf(err, "unable to unmarshal rpc response (%s)", resBody) | ||
| } |
There was a problem hiding this comment.
Prevent logging full response body in error messages to enhance security.
At these lines, including the entire resBody in the error message could expose sensitive data. Consider removing the response body from the error to safeguard against information leakage.
Apply this diff to mitigate the risk:
- return rawResponse{}, errors.Wrapf(err, "unable to unmarshal rpc response (%s)", resBody)
+ return rawResponse{}, errors.Wrap(err, "unable to unmarshal rpc response")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err = json.Unmarshal(resBody, &out); err != nil { | |
| return rawResponse{}, errors.Wrapf(err, "unable to unmarshal rpc response (%s)", resBody) | |
| } | |
| if err = json.Unmarshal(resBody, &out); err != nil { | |
| return rawResponse{}, errors.Wrap(err, "unable to unmarshal rpc response") | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 174-176: zetaclient/chains/bitcoin/client/client.go#L174-L176
Added lines #L174 - L176 were not covered by tests
| ctx context.Context, | ||
| rpc RPC, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update function comments to reflect new parameters ctx and rpc.
The CalcDepositorFee function signature has been updated to include context.Context and RPC. Ensure that the function's documentation is revised accordingly to maintain accurate and helpful comments.
| wif := "" | ||
| if privKeyWIF != nil { | ||
| wif = privKeyWIF.String() | ||
| } | ||
|
|
||
| cmd := types.NewImportPrivKeyCmd(wif, &label, &rescan) | ||
|
|
||
| _, err := c.sendCommand(ctx, cmd) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Secure Handling of Private Keys in ImportPrivKeyRescan
Ensure that private keys provided to ImportPrivKeyRescan are handled securely:
- Avoid logging the
privKeyWIFor including it in error messages. - Confirm that any errors returned do not expose sensitive information.
Review the code to ensure compliance with security best practices and apply necessary safeguards.
| func (c *Client) RawRequest(ctx context.Context, method string, params []json.RawMessage) (json.RawMessage, error) { | ||
| switch { | ||
| case method == "": | ||
| return nil, errors.New("no method") | ||
| case params == nil: | ||
| params = []json.RawMessage{} | ||
| } | ||
|
|
||
| paramsBytes, err := json.Marshal(params) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "unable to marshal params") | ||
| } | ||
|
|
||
| body := fmt.Sprintf( | ||
| `{"jsonrpc":"%s","id":%d,"method":"%s","params":%s}`, | ||
| rpcVersion, | ||
| commandID, | ||
| method, | ||
| paramsBytes, | ||
| ) | ||
|
|
||
| req, err := c.newRequest(ctx, []byte(body)) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "unable to create request") | ||
| } | ||
|
|
||
| res, err := c.sendRequest(req, method) | ||
| switch { | ||
| case err != nil: | ||
| return nil, errors.Wrapf(err, "%q failed", method) | ||
| case res.Error != nil: | ||
| return nil, errors.Wrapf(res.Error, "got rpc error for %q", method) | ||
| } | ||
|
|
||
| return res.Result, nil | ||
| } |
There was a problem hiding this comment.
Input Validation in RawRequest to Prevent Injection Attacks
In the RawRequest method, consider validating or sanitizing the method and params inputs to prevent potential JSON-RPC injection attacks. Ensure that only intended methods are callable and inputs are properly checked.
Implement input validation mechanisms to enhance security.
| if err != nil { | ||
| return nil, errors.Wrap(err, "unable to create signer") | ||
| } | ||
| signer := btcsigner.New(*rawChain, oc.deps.TSS, rpcClient, oc.logger.base) |
There was a problem hiding this comment.
Handle Possible Errors When Initializing the Signer
The btcsigner.New function previously included error handling, which has been removed. If btcsigner.New can return an error, it is crucial to handle it to prevent potential runtime panics or unhandled exceptions.
Apply this diff to reintroduce error handling:
- signer := btcsigner.New(*rawChain, oc.deps.TSS, rpcClient, oc.logger.base)
+ signer, err := btcsigner.New(*rawChain, oc.deps.TSS, rpcClient, oc.logger.base)
+ if err != nil {
+ return nil, errors.Wrap(err, "unable to create signer")
+ }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
I wonder if there is a way to actually run this in CI? Maybe just throw it on the e2e side? Not sure if it's actually terribly important since the methods will have coverage on e2e anyway.
There was a problem hiding this comment.
We already have the rpc_count and rpc_duration_seconds why can't we use those metrics?
There was a problem hiding this comment.
RPCCount = promauto.NewCounterVec(
prometheus.CounterOpts{
Namespace: ZetaClientNamespace,
Name: "rpc_count",
Help: "A counter for number of total RPC requests",
},
[]string{"host", "code"},
)
// RPCLatency is a histogram of the RPC latency
RPCLatency = promauto.NewHistogramVec(
prometheus.HistogramOpts{
Namespace: ZetaClientNamespace,
Name: "rpc_duration_seconds",
Help: "A histogram of the RPC duration in seconds",
Buckets: prometheus.DefBuckets,
},
[]string{"host"},
)vs
RPCClientCounter = promauto.NewCounterVec(
prometheus.CounterOpts{
Namespace: ZetaClientNamespace,
Name: "rpc_client_calls_total",
Help: "Total number of rpc calls",
},
[]string{"status", "client", "method"},
)
RPCClientDuration = promauto.NewHistogramVec(
prometheus.HistogramOpts{
Namespace: ZetaClientNamespace,
Name: "rpc_client_duration_seconds",
Help: "Histogram of rpc client calls durations in seconds",
Buckets: []float64{0.05, 0.1, 0.2, 0.3, 0.5, 1, 1.5, 2, 3, 5, 7.5, 10, 15}, // 50ms to 15s
},
[]string{"status", "client", "method"},
)the latter support status (ok), client (btc:18443), and method (getbestblockhash). I plan to use them for all RPC clients to have 2 metrics that cover all basic needs.
There was a problem hiding this comment.
Especially on the histogram metric that's really huge cardinality if you track every method. That would probably increase our metrics storage size like 20-50%.
I understand this is more valuable than the high level metrics. Let's talk about it tomorrow morning.
|
|
||
| c := &Client{ | ||
| hostURL: normalizeHostURL(cfg.RPCHost, true), | ||
| client: defaultHTTPClient(), |
There was a problem hiding this comment.
use metrics.GetInstrumentedHTTPClient to get a http client with automatic metrics? related to my comment on metrics.go
There was a problem hiding this comment.
It's not usefull for RPC as it has only 1 endpoint instead of /eth_this, /eth_that, ...
| return nil, errors.Wrap(err, "unable to marshal cmd") | ||
| } | ||
|
|
||
| // ps: we can add retry logic if needed |
There was a problem hiding this comment.
imo the best place to do retries in our case is from the top down (orchestrator/scheduler level)
| } | ||
|
|
||
| func (c *Client) sendRequest(req *http.Request, method string) (out rawResponse, err error) { | ||
| c.logger.Debug().Str("rpc.method", method).Msg("Sending request") |
There was a problem hiding this comment.
somewhat nitpicky but it would be nice if we injected the logger into the context and extracted it when logging so that the log is attributed to the correct subsystem.
There was a problem hiding this comment.
Let's revisit in future if needed
| @@ -26,6 +28,43 @@ import ( | |||
| clienttypes "github.com/zeta-chain/node/zetaclient/types" | |||
| ) | |||
|
|
|||
| type RPC interface { | |||
There was a problem hiding this comment.
I think I preferred this in the separate interfaces package. Or at least next to the client.
It's useful to have an assertion that a type implements the actual interface (example: var _ GasTx = (*legacytx.StdTx)(nil)). With this structure I don't think it's possible because it's a circular import.
There was a problem hiding this comment.
I don't see this as an issue at all. IMO this is the best idiomatic way of doing this. Observer doesn't need to care who is a client, only what it can do. The rest is a concern of someone who calls NewObserver.
Also, we have linters, test cases, and basically the code won't compile if something is not implemented.
| body := fmt.Sprintf( | ||
| `{"jsonrpc":"%s","id":%d,"method":"%s","params":%s}`, | ||
| rpcVersion, | ||
| commandID, | ||
| method, | ||
| paramsBytes, | ||
| ) |
There was a problem hiding this comment.
coderabbit already noted this is an issue but I'm opening another thread because this is a must fix issue.
This PR implements Bitcoin client in Go-"native" way
btcsuitetypes, but removes all redundant features such as websockets or backward compatibility with old BTC versions that are not used anymorectxfor each callchan-based design is replaced with a simple "stateless" request-response approachctxmight block execution indefinitely & block other routines (see this and this)zetaclient/chains/bitcoin/rpczetaclient/chains/bitcoin/rpc&interfaces.BTCRPCClientwith the new clientOne of examples
Before
After
Related:
Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
Release Notes
New Features
Improvements
Changes
Performance
Testing