refactor(zetaclient): extract base observer and base signer from BTC #3394
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe pull request introduces a significant refactoring of the Bitcoin observer and signer initialization process across multiple files in the ZetaChain codebase. The primary objective is to decouple components by extracting base observer and base signer implementations, simplifying the constructor methods for Bitcoin-related components. This refactoring aims to improve modularity and testing flexibility by introducing a two-step initialization process using base observers and signers. Changes
Assessment against linked issues
Possibly related PRs
Suggested Labels
Suggested Reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (3)
zetaclient/chains/bitcoin/signer/signer.go (2)
Line range hint
149-150: Consider breaking down the SignWithdrawTx function.The TODO comment suggests simplification. The function is complex and handles multiple responsibilities including UTXO selection, transaction building, size checking, fee calculation, and signing.
Consider splitting this into smaller, focused functions:
+ func (signer *Signer) buildTransactionInputs(prevOuts []common.Unspent) (*wire.MsgTx, error) + func (signer *Signer) calculateAndValidateSize(tx *wire.MsgTx, to btcutil.Address) (uint64, error) + func (signer *Signer) signTransactionInputs(ctx context.Context, tx *wire.MsgTx, prevOuts []common.Unspent, height uint64, nonce uint64, chainId int64) error
Line range hint
319-320: Simplify TryProcessOutbound function.The TODO comment indicates this function needs simplification. It's handling too many responsibilities including parameter validation, compliance checks, transaction signing, and broadcasting.
Consider extracting the following responsibilities into separate methods:
+ func (signer *Signer) validateOutboundParams(params *types.OutboundParams) error + func (signer *Signer) handleComplianceAndDustChecks(cctx *types.CrossChainTx, params *types.OutboundParams) (bool, error) + func (signer *Signer) broadcastWithRetry(ctx context.Context, tx *wire.MsgTx, logger zerolog.Logger) errorzetaclient/chains/bitcoin/observer/observer.go (1)
Line range hint
124-149: Add Nil Check forbaseObserverParameterThe function assumes
baseObserveris not nil. Adding a nil check ensures robustness and prevents potential runtime panics.Apply this diff to validate the
baseObserver:func New(chain chains.Chain, baseObserver *base.Observer, rpc RPC) (*Observer, error) { + if baseObserver == nil { + return nil, errors.New("baseObserver cannot be nil") + } // get the bitcoin network params
🧹 Nitpick comments (6)
zetaclient/chains/bitcoin/signer/signer.go (1)
Line range hint
319-476: Improve error handling in TryProcessOutbound.The function uses early returns with logging but doesn't propagate errors, making it difficult to handle failures upstream.
Consider returning errors instead of just logging them:
- func (signer *Signer) TryProcessOutbound(ctx context.Context, ...) { + func (signer *Signer) TryProcessOutbound(ctx context.Context, ...) error { if err := validateParams(); err != nil { - logger.Error().Err(err).Msg("invalid params") - return + return fmt.Errorf("invalid params: %w", err) } // ... rest of the function + return nil }zetaclient/chains/bitcoin/observer/observer_test.go (2)
141-147: Replace Magic Number with Named ConstantThe hardcoded value
100used forblocksCacheSizereduces code readability. Consider defining a named constant or using an existing one to enhance clarity and maintainability.Apply this diff to introduce a named constant:
+const testBlocksCacheSize = 100 baseObserver, err := base.NewObserver( tt.chain, tt.chainParams, tt.coreClient, tt.tss, - 100, + testBlocksCacheSize, tt.ts, database, tt.logger, )
329-335: Use Consistent Constants for Block Cache SizeThe hardcoded value
100forblocksCacheSizeappears again. For consistency and ease of maintenance, consider defining and reusing a constant across tests.Apply this diff to define a shared constant:
+const testBlocksCacheSize = 100 baseObserver, err := base.NewObserver( chain, chainParams, zetacore, nil, - 100, + testBlocksCacheSize, nil, database, base.Logger{Std: log, Compliance: log}, )zetaclient/chains/bitcoin/observer/observer.go (1)
Line range hint
138-140: Avoid Unnecessary Pointer DereferencingThe assignment of
ObserverLoggerinvolves unnecessary dereferencing. Assigning the pointer directly enhances code efficiency.Apply this diff to simplify the logger initialization:
logger: Logger{ - ObserverLogger: *baseObserver.Logger(), + ObserverLogger: baseObserver.Logger(), UTXOs: baseObserver.Logger().Chain.With().Str("module", "utxos").Logger(), },zetaclient/orchestrator/v2_bootstrap.go (2)
17-18: DocumentbtcBlocksPerDayConstantAdding documentation for
btcBlocksPerDayimproves code clarity by explaining its significance.Apply this diff to include a comment:
+// btcBlocksPerDay represents the average number of Bitcoin blocks mined per day. const btcBlocksPerDay = 144
55-57: Check for Errors After Signer InitializationAlthough
btcsigner.Newdoes not return an error, ensure that the returned signer is not nil before proceeding.Apply this diff to include a nil check:
signer := btcsigner.New(baseSigner, rpcClient) +if signer == nil { + return nil, errors.New("failed to initialize Bitcoin signer") +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
zetaclient/chains/bitcoin/observer/observer.go(1 hunks)zetaclient/chains/bitcoin/observer/observer_test.go(2 hunks)zetaclient/chains/bitcoin/observer/outbound_test.go(1 hunks)zetaclient/chains/bitcoin/signer/signer.go(1 hunks)zetaclient/chains/bitcoin/signer/signer_test.go(2 hunks)zetaclient/orchestrator/v2_bootstrap.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
zetaclient/chains/bitcoin/observer/observer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/signer/signer_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/outbound_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/observer_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/signer/signer.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.
🔇 Additional comments (7)
zetaclient/chains/bitcoin/signer/signer.go (1)
61-62: LGTM! Clean constructor refactoring.The simplified constructor improves modularity and testability by accepting a pre-configured base signer.
zetaclient/chains/bitcoin/observer/observer_test.go (1)
341-343:⚠️ Potential issueAdd Error Handling After Observer Creation
After initializing the observer, it's crucial to handle any potential errors to ensure the observer is correctly instantiated.
Apply this diff to include error checking:
ob, err := observer.New(chain, baseObserver, client) +require.NoError(t, err) require.NoError(t, err)Likely invalid or redundant comment.
zetaclient/chains/bitcoin/observer/observer.go (1)
Line range hint
156-160: Handle Context Errors AppropriatelyEnsure that any errors due to context timeouts or cancellations in
LoadLastBlockScannedandLoadBroadcastedTxMapare properly handled to prevent unexpected behavior.Review the internal implementations to confirm that context-related errors are managed correctly.
zetaclient/orchestrator/v2_bootstrap.go (2)
72-75: Reuse Existing Constants for Block Cache SizeThe conditionally set
blocksCacheSizeduplicates the value ofbtcBlocksPerDay. Since the constant is already defined, this implementation is appropriate.
89-90: Ensure Consistent Error Handling innewBaseSignerThough
base.NewSignercurrently does not return an error, consider future-proofing by handling potential errors that may arise from changes in its implementation.Review
base.NewSignerfor any changes that might introduce errors, and update the method signature and error handling accordingly.zetaclient/chains/bitcoin/signer/signer_test.go (1)
53-55: LGTM! Clean separation of concerns in signer initialization.The two-step initialization pattern improves modularity by clearly separating the base signer creation from the Bitcoin-specific signer initialization.
zetaclient/chains/bitcoin/observer/outbound_test.go (1)
46-50: LGTM! Consistent initialization pattern with proper error handling.The two-step initialization pattern with proper error handling aligns well with the signer implementation, improving code consistency across the codebase.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3394 +/- ##
========================================
Coverage 63.40% 63.40%
========================================
Files 436 436
Lines 30690 30690
========================================
Hits 19460 19460
Misses 10394 10394
Partials 836 836
|
Closes #3331
Summary by CodeRabbit
Refactor
New Features
btcBlocksPerDayfor Bitcoin block cachingTests