feat(zetaclient): propagate context across codebase & refactor zetacore client #2428
Conversation
|
Warning Rate limit exceeded@swift1337 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 13 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent changes introduce context handling across various functions for better control over execution flow and context-specific information tracking. This includes modifications in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Observer
participant Signer
participant Retry
participant BackgroundTask
Client->>Observer: ObserveInbound(ctx, tx, receipt)
Observer->>Signer: Sign(ctx, data, txData)
Signer->>Retry: DoWithBackoff(Sign, backoff)
Retry-->>Signer: Signed Transaction
Signer->>Observer: Return Signed Transaction
Observer->>Client: Return Observed Data
Client->>BackgroundTask: Work(ctx, taskFunc)
BackgroundTask->>Logger: Log Task Start
BackgroundTask->>Client: Execute Task
BackgroundTask->>Logger: Log Task End
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
# Conflicts: # zetaclient/chains/base/signer.go # zetaclient/chains/evm/signer/outbound_data.go # zetaclient/chains/evm/signer/outbound_data_test.go # zetaclient/chains/evm/signer/signer.go # zetaclient/chains/evm/signer/signer_test.go # zetaclient/orchestrator/orchestrator.go # zetaclient/zetacore/client.go # zetaclient/zetacore/client_query_test.go # zetaclient/zetacore/query.go # zetaclient/zetacore/tx.go # zetaclient/zetacore/tx_test.go
swift1337
requested review from
brewmaster012,
kingpinXD,
ws4charlie,
skosito and
fbac
as code owners
swift1337
added
zetaclient
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range, codebase verification and nitpick comments (12)
zetaclient/zetacore/constant.go (3)
6-7: Add context to constant comment.The comment for
DefaultBaseGasPricecould be more descriptive to explain its usage context.- // DefaultBaseGasPrice is the default base gas price + // DefaultBaseGasPrice is the default base gas price used for transactions on the ZetaChain network
15-15: Fix grammatical error in comment.The comment for
PostVoteInboundGasLimitcontains a grammatical error.- // PostVoteInboundGasLimit is the gas limit for voting on observed inbound tx (for zetachain itself) + // PostVoteInboundGasLimit is the gas limit for voting on observed inbound transactions (for ZetaChain itself)
39-39: Fix grammatical error in comment.The comment for
PostVoteOutboundGasLimitcontains a grammatical error.- // PostVoteOutboundGasLimit is the gas limit for voting on observed outbound tx (for zetachain itself) + // PostVoteOutboundGasLimit is the gas limit for voting on observed outbound transactions (for ZetaChain itself)zetaclient/zetacore/broadcast.go (1)
168-196: Improve error logging.Consider adding more context to the error logs to make it easier to diagnose issues.
- evt.Msg(m) + evt.Str("error_type", "nonce too low").Msg(m)- evt.Msg("Broadcast replacement") + evt.Str("error_type", "replacement transaction underpriced").Msg("Broadcast replacement")- evt.Msg("Broadcast duplicates") + evt.Str("error_type", "already known").Msg("Broadcast duplicates")- evt.Msg("Broadcast error. Retrying...") + evt.Str("error_type", "general broadcast error").Msg("Broadcast error. Retrying...")zetaclient/chains/bitcoin/observer/observer.go (5)
6-6: Remove unnecessary blank line.The blank line after the import statement is unnecessary and can be removed for better readability.
- "context" + "context"
Line range hint
221-277: Handle context cancellation and deadlines.Consider handling context cancellation and deadlines in the
WatchRPCStatusfunction.// Example of handling context cancellation select { case <-ctx.Done(): return ctx.Err() case <-ticker.C: if !ob.GetChainParams().IsSupported { continue } // existing code... }Tools
GitHub Check: codecov/patch
[warning] 196-196: zetaclient/chains/bitcoin/observer/observer.go#L196
Added line #L196 was not covered by tests
[warning] 200-200: zetaclient/chains/bitcoin/observer/observer.go#L200
Added line #L200 was not covered by tests
[warning] 203-203: zetaclient/chains/bitcoin/observer/observer.go#L203
Added line #L203 was not covered by tests
343-347: Improve variable declaration readability.The variable declarations can be improved for better readability.
- var ( - err error - feeRateEstimated uint64 - ) + var err error + var feeRateEstimated uint64
Line range hint
433-454: Handle context cancellation and deadlines.Consider handling context cancellation and deadlines in the
WatchUTXOsfunction.// Example of handling context cancellation select { case <-ctx.Done(): return ctx.Err() case <-ticker.C(): if !ob.GetChainParams().IsSupported { continue } // existing code... }
Line range hint
673-696: Handle context cancellation and deadlines.Consider handling context cancellation and deadlines in the
postBlockHeaderfunction.select { case <-ctx.Done(): return ctx.Err() default: // existing code... }zetaclient/chains/bitcoin/observer/outbound.go (3)
35-35: Ensure proper context cancellation and timeout handling.While the context is being passed correctly, consider adding context cancellation and timeout handling to avoid potential issues with long-running operations.
ctx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel()
61-64: Log context errors with appropriate severity.Logging at
Infolevel for an error may not be appropriate. Consider usingErrororWarnlevel to log context-related errors.- ob.logger.Outbound.Info(). + ob.logger.Outbound.Error().
157-157: Consider adding context cancellation or timeout handling.To avoid potential issues with long-running operations, consider adding context cancellation or timeout handling.
ctx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel()
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (8)
- changelog.md (1 hunks)
- e2e/README.md (1 hunks)
- go.mod (1 hunks)
- zetaclient/chains/bitcoin/observer/observer.go (14 hunks)
- zetaclient/chains/bitcoin/observer/outbound.go (20 hunks)
- zetaclient/chains/bitcoin/rpc/rpc_live_test.go (1 hunks)
- zetaclient/zetacore/broadcast.go (5 hunks)
- zetaclient/zetacore/constant.go (2 hunks)
Files skipped from review due to trivial changes (2)
- e2e/README.md
- zetaclient/chains/bitcoin/rpc/rpc_live_test.go
Files skipped from review as they are similar to previous changes (1)
- go.mod
Additional context used
Path-based instructions (4)
zetaclient/zetacore/constant.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/zetacore/broadcast.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/bitcoin/observer/observer.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.
GitHub Check: codecov/patch
zetaclient/chains/bitcoin/observer/observer.go
[warning] 196-196: zetaclient/chains/bitcoin/observer/observer.go#L196
Added line #L196 was not covered by tests
[warning] 200-200: zetaclient/chains/bitcoin/observer/observer.go#L200
Added line #L200 was not covered by tests
[warning] 203-203: zetaclient/chains/bitcoin/observer/observer.go#L203
Added line #L203 was not covered by tests
Additional comments not posted (10)
zetaclient/zetacore/broadcast.go (3)
36-38: Handle context cancellation and deadlines.Consider handling context cancellation and deadlines for the
GetBlockHeightfunction call.
41-43: Handle context cancellation and deadlines.Consider handling context cancellation and deadlines for the
GetBaseGasPricefunction call.
56-57: Potential data race onc.blockHeight.Ensure that
c.blockHeightis protected by the mutex during read operations to avoid data races.zetaclient/chains/bitcoin/observer/outbound.go (6)
126-130: Context propagation is well-implemented.The function correctly propagates the context and handles the necessary checks and logging.
260-260: Context propagation is well-implemented.The function correctly propagates the context and handles the necessary checks and logging.
346-346: Context propagation is well-implemented.The function correctly propagates the context and handles the necessary checks and logging.
378-378: Context propagation is well-implemented.The function correctly propagates the context and handles the necessary checks and logging.
433-433: Context propagation is well-implemented.The function correctly propagates the context and handles the necessary checks and logging.
516-516: Context propagation is well-implemented.The function correctly propagates the context and handles the necessary checks and logging.
changelog.md (1)
61-61: LGTM!The changelog entry clearly logs the feature update for propagating context across the codebase and refactoring the zetacore client.
swift1337
force-pushed
the
feat/zetaclient-ctx
branch
from
605036f to
d36876d
Compare
lumtis
added
UPGRADE_LIGHT_TESTS
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- zetaclient/chains/bitcoin/observer/outbound.go (20 hunks)
Files skipped from review as they are similar to previous changes (1)
- zetaclient/chains/bitcoin/observer/outbound.go
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (9)
zetaclient/chains/bitcoin/observer/outbound.go (9)
55-60: Optimize the nested if statements.The nested if statements can be simplified for better readability and maintainability.
- if !app.IsOutboundObservationEnabled(ob.GetChainParams()) { - sampledLogger.Info(). - Msgf("WatchOutbound: outbound observation is disabled for chain %d", chainID) - continue - } + if !app.IsOutboundObservationEnabled(ob.GetChainParams()) { + sampledLogger.Info().Msgf("WatchOutbound: outbound observation is disabled for chain %d", chainID) + continue + }
Line range hint
166-170: Improve error logging forcheckIncludedTx.Consider adding more context to the error log to make it easier to debug issues related to transaction inclusion.
- ob.logger.Outbound.Error().Err(err).Msgf("IsOutboundProcessed: error verify bitcoin outbound %s outboundID %s", txHash, outboundID) + ob.logger.Outbound.Error().Err(err).Msgf("IsOutboundProcessed: error verifying bitcoin outbound transaction %s with outboundID %s", txHash, outboundID)
273-273: Improve error handling forgetOutboundIDByNonce.Consider adding more context to the error log to make it easier to debug issues related to nonce-mark UTXO selection.
- return nil, 0, 0, 0, err + return nil, 0, 0, 0, fmt.Errorf("failed to get outbound ID by nonce: %w", err)
361-361: Improve error handling forgetOutboundIDByNonce.Consider adding more context to the error log to make it easier to debug issues related to nonce-mark UTXO selection.
- ob.logger.Chain.Error().Err(err).Msg("refreshPendingNonce: error getting last outbound txid") + ob.logger.Chain.Error().Err(err).Msgf("refreshPendingNonce: error getting last outbound txid for nonce %d", nonceLow-1)
385-385: Improve error handling forGetCctxByNonce.Consider adding more context to the error log to make it easier to debug issues related to CCTX retrieval.
- return "", errors.Wrapf(err, "getOutboundIDByNonce: error getting cctx for nonce %d", nonce) + return "", fmt.Errorf("failed to get CCTX by nonce: %w", err)
450-450: Improve error handling forcheckTssOutboundResult.Consider adding more context to the error log to make it easier to debug issues related to TSS outbound result verification.
- ob.logger.Outbound.Error().Err(err).Msgf("checkIncludedTx: error verify bitcoin outbound %s outboundID %s", txHash, outboundID) + ob.logger.Outbound.Error().Err(err).Msgf("checkIncludedTx: error verifying bitcoin outbound transaction %s with outboundID %s", txHash, outboundID)
526-526: Improve error handling forcheckTSSVin.Consider adding more context to the error log to make it easier to debug issues related to TSS VIN checks.
- return errors.Wrapf(err, "checkTssOutboundResult: invalid TSS Vin in outbound %s nonce %d", hash, nonce) + return fmt.Errorf("invalid TSS Vin in outbound %s nonce %d: %w", hash, nonce, err)
570-570: Improve error handling forgetOutboundIDByNonce.Consider adding more context to the error log to make it easier to debug issues related to nonce-mark UTXO selection.
- return fmt.Errorf("checkTSSVin: error findTxIDByNonce %d", nonce-1) + return fmt.Errorf("checkTSSVin: error finding TxID by nonce %d: %w", nonce-1, err)
Line range hint
599-599: Improve error handling forDecodeTSSVout.Consider adding more context to the error log to make it easier to debug issues related to VOUT decoding.
- return err + return fmt.Errorf("failed to decode TSS VOUT: %w", err)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- zetaclient/chains/bitcoin/observer/outbound.go (20 hunks)
- zetaclient/chains/evm/observer/outbound.go (10 hunks)
- zetaclient/zetacore/client_monitor.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- zetaclient/chains/evm/observer/outbound.go
- zetaclient/zetacore/client_monitor.go
Additional context used
Path-based instructions (1)
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.
Additional comments not posted (10)
zetaclient/chains/bitcoin/observer/outbound.go (10)
35-35: Ensure proper context cancellation and timeout handling.While the context is being passed correctly, consider adding context cancellation and timeout handling to avoid potential issues with long-running operations.
ctx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel()
36-39: Handle potential error fromFromContext.Returning
nilmay not be appropriate. Consider returning the error or handling it differently to ensure proper error propagation.- return nil + return fmt.Errorf("failed to retrieve context: %w", err)
125-129: Ensure proper context propagation and cancellation.While the context is being passed correctly, consider adding context cancellation and timeout handling to avoid potential issues with long-running operations.
ctx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel()
226-231: Use a more explicit variable name for log fields.
lfmight not be a standard abbreviation for log fields. Use a more explicit variable name for better readability.- lf := map[string]any{ + logFields := map[string]any{
259-259: Ensure proper context propagation and cancellation.While the context is being passed correctly, consider adding context cancellation and timeout handling to avoid potential issues with long-running operations.
ctx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel()
345-345: Ensure proper context propagation and cancellation.While the context is being passed correctly, consider adding context cancellation and timeout handling to avoid potential issues with long-running operations.
ctx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel()
377-377: Ensure proper context propagation and cancellation.While the context is being passed correctly, consider adding context cancellation and timeout handling to avoid potential issues with long-running operations.
ctx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel()
432-432: Ensure proper context propagation and cancellation.While the context is being passed correctly, consider adding context cancellation and timeout handling to avoid potential issues with long-running operations.
ctx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel()
515-515: Ensure proper context propagation and cancellation.While the context is being passed correctly, consider adding context cancellation and timeout handling to avoid potential issues with long-running operations.
ctx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel()
554-554: Ensure proper context propagation and cancellation.While the context is being passed correctly, consider adding context cancellation and timeout handling to avoid potential issues with long-running operations.
ctx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel()
swift1337
force-pushed
the
feat/zetaclient-ctx
branch
from
d7ccb9c to
5ff1728
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- pkg/bg/bg.go (1 hunks)
- pkg/bg/bg_test.go (1 hunks)
Additional context used
Path-based instructions (2)
pkg/bg/bg.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.pkg/bg/bg_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
GitHub Check: codecov/patch
pkg/bg/bg.go
[warning] 53-53: pkg/bg/bg.go#L53
Added line #L53 was not covered by tests
Additional comments not posted (5)
pkg/bg/bg.go (3)
18-20: LGTM!The
WithNamefunction is correctly implemented.
22-24: LGTM!The
WithLoggerfunction is correctly implemented.
51-62: LGTM!The
logErrorfunction is correctly implemented and effectively logs errors.Tools
GitHub Check: codecov/patch
[warning] 53-53: pkg/bg/bg.go#L53
Added line #L53 was not covered by testspkg/bg/bg_test.go (2)
14-75: LGTM!The
TestWorkfunction is well-implemented and covers various scenarios effectively.
78-81: LGTM!The
assertChanClosedfunction is correctly implemented.
Description
This PR revamps the
zetaclientcodebase mainly by wiringctx context.Contextacross the codebase.retrierpackage with exponential backoff out-of-the-boxbgpackage for running go routines safelyzetacoreclient package, separate each QueryClient into a different filectxacross the codebasemockeryforZetaClientinterfaceHow Has This Been Tested?
Summary by CodeRabbit
New Features
Refactor
Tests