refactor(zetaclient): move app context update from zetacore client #3131
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 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces multiple updates across various components of the ZetaChain node, including enhancements, refactoring, and fixes. Key features include the ability to whitelist SPL tokens on Solana, improvements in build reproducibility, and the integration of SPL deposits. The application context update mechanism has been centralized within the orchestrator, and several methods have been added or modified to streamline functionality. Additionally, the changelog has been updated to reflect these changes, alongside the transition to version 21.0.0. 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
|
swift1337
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
skosito and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (18)
pkg/rpc/clients_cosmos.go (1)
16-28: Clean refactoring of GetUpgradePlan method.The changes improve the method in several ways:
- Returning a value type instead of a pointer eliminates nil pointer concerns
- Using switch for control flow makes the logic clearer
- Error wrapping provides better context for debugging
The implementation aligns well with the PR's objective of simplifying upgrade plan detection.
Consider documenting the error cases in the method comment to help API consumers handle different scenarios appropriately. For example:
// GetUpgradePlan returns the current upgrade plan. // Returns empty plan if no upgrade is planned. // Returns empty plan and error if failed to fetch the plan.zetaclient/orchestrator/contextupdater_test.go (3)
16-24: Consider adding documentation for test data setup.While the setup is clean, adding comments explaining the significance of the mock parameters (e.g., why 100 was chosen) would improve maintainability.
func Test_UpdateAppContext(t *testing.T) { + // Mock chain parameters with block confirmation of 100 blocks var ( eth = chains.Ethereum ethParams = mocks.MockChainParams(eth.ChainId, 100)
25-60: Enhance test coverage with additional assertions.While the test successfully verifies the addition of new chains, consider:
- Verifying that mock methods were called with expected parameters
- Asserting the state of other context properties (e.g., crosschain flags, TSS)
Add these assertions after the existing checks:
require.NoError(t, err) + // Verify mock calls + zetacore.AssertCalled(t, "GetBlockHeight", mock.Anything) + zetacore.AssertCalled(t, "GetChainParams", mock.Anything) + + // Verify context state + require.True(t, app.IsInboundEnabled()) + require.True(t, app.IsOutboundEnabled()) + require.Equal(t, "0x123", app.GetTSSKey())
61-82: Consider adding edge cases for upgrade plan detection.The test effectively verifies basic upgrade detection, but consider adding test cases for:
- Upgrade plan at current height
- Upgrade plan far in the future
- Empty upgrade plan name
Example additional test case:
t.Run("Ignores distant upgrade plan", func(t *testing.T) { ctx := context.Background() app := createAppContext(t, eth, ethParams) zetacore := mocks.NewZetacoreClient(t) logger := zerolog.New(zerolog.NewTestWriter(t)) zetacore.On("GetBlockHeight", mock.Anything).Return(int64(123), nil) zetacore.On("GetUpgradePlan", mock.Anything).Return(upgradetypes.Plan{ Name: "future", Height: 1000, // Far in the future }, nil) err := UpdateAppContext(ctx, app, zetacore, logger) require.NoError(t, err) // Should not trigger upgrade for distant plans })pkg/rpc/clients.go (1)
Line range hint
101-108: Function name change accurately reflects its behavior.The rename from
NewGRPCClienttoNewGRPCClientsbetter represents that the function creates multiple client interfaces. This aligns with the single responsibility principle and improves code clarity.However, there are two areas for improvement:
- The gRPC connection should be managed to prevent resource leaks
- Error wrapping could be more descriptive for better debugging
Consider applying these improvements:
// NewGRPCClients creates a Clients which uses gRPC as the transport func NewGRPCClients(url string, opts ...grpc.DialOption) (Clients, error) { grpcConn, err := grpc.Dial(url, opts...) if err != nil { - return Clients{}, err + return Clients{}, fmt.Errorf("failed to establish gRPC connection to %s: %w", url, err) } + // Store the connection in the Clients struct + clients, err := newClients(client.Context{}.WithGRPCClient(grpcConn)) + if err != nil { + grpcConn.Close() + return Clients{}, fmt.Errorf("failed to initialize clients: %w", err) + } - clientCtx := client.Context{}.WithGRPCClient(grpcConn) - return newClients(clientCtx) + return clients, nil }Additionally, consider adding a
Close()method to theClientsstruct to properly clean up resources:// Close closes all client connections and cleans up resources func (c *Clients) Close() error { if conn := c.Auth.ClientConn(); conn != nil { return conn.Close() } return nil }cmd/zetaclientd/inbound.go (1)
Line range hint
46-196: Recommend restructuring for improved maintainability and error handling.The
InboundGetBallotfunction would benefit from several structural improvements:
- The argument validation is incorrect:
-cobra.ExactArgs(2) +if err := cobra.ExactArgs(2)(cmd, args); err != nil { + return err +}
- The function is quite long and handles multiple responsibilities. Consider breaking it down into smaller, focused functions:
func InboundGetBallot(cmd *cobra.Command, args []string) error { if err := cobra.ExactArgs(2)(cmd, args); err != nil { return err } ctx, client, appContext, err := setupContext(args) if err != nil { return err } ballotIdentifier, err := getBallotIdentifier(ctx, client, appContext, args) if err != nil { return err } return displayBallotInfo(ctx, client, ballotIdentifier) }
- Chain-specific logic should be extracted to dedicated handlers:
type ChainHandler interface { GetBallotIdentifier(ctx context.Context, hash string) (string, error) } type EVMChainHandler struct { client zetacore.Client observer evmobserver.Observer // ... other dependencies } type BitcoinChainHandler struct { client zetacore.Client observer btcobserver.Observer // ... other dependencies }Would you like me to provide the complete refactoring implementation?
zetaclient/zetacore/client.go (2)
43-46: Consider improving struct field organization and documentation.While the removal of stateful components aligns with the PR objectives, consider:
- Grouping related fields together (e.g., all configuration-related fields)
- Adding documentation for the fields to explain their purpose
type Client struct { zetacore_rpc.Clients + // Configuration config config.ClientConfiguration + encodingCfg etherminttypes.EncodingConfig + chainID string + chain chains.Chain + + // Authentication + keys keyinterfaces.ObserverKeys cosmosClientContext cosmosclient.Context + // State blockHeight int64 accountNumber map[authz.KeyType]uint64 seqNumber map[authz.KeyType]uint64 + // Utilities logger zerolog.Logger mu sync.RWMutex - encodingCfg etherminttypes.EncodingConfig - keys keyinterfaces.ObserverKeys - chainID string - chain chains.Chain }
Line range hint
234-235: Address TODO comment in SetAccountNumber method.The TODO suggests this method should be part of the client constructor. This aligns with the PR's goal of making the client more stateless and predictable.
Would you like help refactoring this method into the constructor? This would involve:
- Moving the account number initialization logic to NewClient
- Removing the SetAccountNumber method
- Updating any existing callers
zetaclient/chains/interfaces/interfaces.go (1)
106-108: Add documentation to clarify chain types.While the new methods follow good interface design patterns, the distinction between "Supported" and "Additional" chains needs clarification. Consider adding method documentation to explain:
- What constitutes a supported chain vs. an additional chain
- The relationship between these methods and GetChainParams
- Usage guidelines for consumers of this interface
Example documentation:
// GetSupportedChains returns the list of primary chains supported by the system // <add description of what makes a chain "supported"> GetSupportedChains(ctx context.Context) ([]chains.Chain, error) // GetAdditionalChains returns secondary or auxiliary chains beyond the primary supported set // <add description of what makes a chain "additional"> GetAdditionalChains(ctx context.Context) ([]chains.Chain, error) // GetChainParams returns the configuration parameters for all chains // <add description of relationship with supported/additional chains> GetChainParams(ctx context.Context) ([]*observertypes.ChainParams, error)zetaclient/zetacore/tx_test.go (1)
Line range hint
108-116: Consider enhancing error test coverage.The commented-out test case for failed broadcast in
TestZetacore_PostGasPricecould provide valuable negative test coverage. Consider:
- Re-implementing it with a shorter timeout
- Converting to table-driven tests to cover more error scenarios efficiently
Example refactor:
func TestZetacore_PostGasPrice(t *testing.T) { tests := []struct { name string setup func(*testing.T) *ZetaCoreClient wantErr bool }{ { name: "success case", setup: func(t *testing.T) *ZetaCoreClient { return setupZetacoreClient(t, withDefaultObserverKeys(), withAccountRetriever(t, 100, 100), withTendermint(mocks.NewSDKClientWithErr(t, nil, 0).SetBroadcastTxHash(sampleHash)), ) }, wantErr: false, }, { name: "broadcast failure", setup: func(t *testing.T) *ZetaCoreClient { return setupZetacoreClient(t, withDefaultObserverKeys(), withAccountRetriever(t, 100, 100), withTendermint(mocks.NewSDKClientWithErr(t, errors.New("broadcast error"), 0)), ) }, wantErr: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { client := tt.setup(t) hash, err := client.PostVoteGasPrice(context.Background(), chains.BscMainnet, 1000000, 0, 1234) if tt.wantErr { require.Error(t, err) require.Empty(t, hash) } else { require.NoError(t, err) require.Equal(t, sampleHash, hash) } }) } }cmd/zetaclientd/start.go (1)
Line range hint
354-366: Enhance comment clarity for background configuration updatesThe comment could be more specific about the nature and frequency of background configuration updates from ZetaCore.
Consider updating the comment to:
-// It also handles background configuration updates from zetacore +// It also handles periodic background updates of chain configurations, parameters, and TSS information from ZetaCorezetaclient/orchestrator/orchestrator.go (1)
142-144: Add documentation and consider thread safetyThe
Stop()method should include documentation explaining its purpose and behavior. Additionally, consider adding mutex protection to prevent potential race conditions during shutdown.+// Stop gracefully shuts down the orchestrator by closing the stop channel. +// This method is thread-safe and idempotent. func (oc *Orchestrator) Stop() { + oc.mu.Lock() + defer oc.mu.Unlock() + select { + case <-oc.stop: + // already closed + return + default: close(oc.stop) + } }changelog.md (2)
17-17: Enhance the description of PR #3131.The current description "move app context update from zetacore client" could be more detailed to better reflect the architectural changes and benefits. Consider adding more context about:
- The move to the orchestrator package
- The transformation of ZetaCore client into a stateless service
- The improved separation of concerns
-* [3131](https://github.com/zeta-chain/node/pull/3131) - move app context update from zetacore client +* [3131](https://github.com/zeta-chain/node/pull/3131) - refactor: move app context update from zetacore client to orchestrator, transforming ZetaCore client into a stateless service for improved separation of concerns
Line range hint
1-1000: Standardize category ordering across versions.Consider maintaining a consistent order of categories across all versions. Suggested order:
- Breaking Changes
- Features
- Fixes
- Refactor
- Tests
- CI
- Docs
- Chores
zetaclient/orchestrator/contextupdater.go (4)
3-14: Replacegithub.com/pkg/errorswith the standard libraryerrorspackageTo reduce external dependencies and adhere to modern Go practices, consider replacing
github.com/pkg/errorswith Go's standarderrorspackage, which provides error wrapping capabilities since Go 1.13.Apply this diff to update the imports:
import ( "context" upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types" - "github.com/pkg/errors" "github.com/rs/zerolog" "github.com/zeta-chain/node/pkg/chains" "github.com/zeta-chain/node/pkg/ticker" observertypes "github.com/zeta-chain/node/x/observer/types" zctx "github.com/zeta-chain/node/zetaclient/context" + "errors" )And modify error wrapping throughout the file using
fmt.Errorfwith%w. For example:- return errors.Wrap(err, "unable to get zeta block height") + return fmt.Errorf("unable to get zeta block height: %w", err)Ensure all instances of
errors.Wrapanderrors.Wrapfare updated accordingly.
29-60: Separate the task function for clarity and maintainabilityConsider extracting the anonymous task function within
runAppContextUpdaterinto a named method. This enhances readability and allows for easier unit testing and potential reuse.Apply this refactoring:
func (oc *Orchestrator) runAppContextUpdater(ctx context.Context) error { app, err := zctx.FromContext(ctx) if err != nil { return err } interval := ticker.DurationFromUint64Seconds(app.Config().ConfigUpdateTicker) oc.logger.Info().Msg("UpdateAppContext worker started") - task := func(ctx context.Context, t *ticker.Ticker) error { - err := UpdateAppContext(ctx, app, oc.zetacoreClient, oc.logger.Sampled) - switch { - case errors.Is(err, ErrUpgradeRequired): - oc.onUpgradeDetected(err) - t.Stop() - return nil - case err != nil: - oc.logger.Err(err).Msg("UpdateAppContext failed") - } - - return nil - } + // Extracted task function + return oc.updateAppContextTask(ctx, app, interval) + +} + +func (oc *Orchestrator) updateAppContextTask(ctx context.Context, app *zctx.AppContext, interval time.Duration) error { return ticker.Run( ctx, interval, - task, + func(ctx context.Context, t *ticker.Ticker) error { + err := UpdateAppContext(ctx, app, oc.zetacoreClient, oc.logger.Sampled) + switch { + case errors.Is(err, ErrUpgradeRequired): + oc.onUpgradeDetected(err) + t.Stop() + return nil + case err != nil: + oc.logger.Err(err).Msg("UpdateAppContext failed") + } + + return nil + }, ticker.WithLogger(oc.logger.Logger, "UpdateAppContext"), ticker.WithStopChan(oc.stop), ) }
67-102: Usefmt.Errorffor error wrappingUpdate error handling to use
fmt.Errorfwith the%wverb for error wrapping. This aligns with standard Go practices and improves error traceability.For example, modify the error wrapping as follows:
- return errors.Wrap(err, "unable to get zeta block height") + return fmt.Errorf("unable to get zeta block height: %w", err)Apply similar changes to all error wrapping instances in the
UpdateAppContextfunction.
152-153: MakeupgradeRangea configurable parameterCurrently,
upgradeRangeis hard-coded to2. Consider makingupgradeRangea configurable parameter to provide flexibility and adjustability based on deployment needs.Modify the code to read
upgradeRangefrom the configuration:const ( - upgradeRange = 2 ) +upgradeRange := app.Config().UpgradeRangeEnsure that
UpgradeRangeis added to the application configuration with an appropriate default value.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (15)
changelog.md(1 hunks)cmd/zetaclientd/inbound.go(2 hunks)cmd/zetaclientd/start.go(5 hunks)cmd/zetaclientd/utils.go(3 hunks)pkg/rpc/clients.go(1 hunks)pkg/rpc/clients_cosmos.go(1 hunks)zetaclient/chains/interfaces/interfaces.go(3 hunks)zetaclient/orchestrator/contextupdater.go(1 hunks)zetaclient/orchestrator/contextupdater_test.go(1 hunks)zetaclient/orchestrator/orchestrator.go(2 hunks)zetaclient/testutils/mocks/zetacore_client.go(5 hunks)zetaclient/zetacore/client.go(3 hunks)zetaclient/zetacore/client_worker.go(0 hunks)zetaclient/zetacore/constant.go(0 hunks)zetaclient/zetacore/tx_test.go(1 hunks)
💤 Files with no reviewable changes (2)
- zetaclient/zetacore/client_worker.go
- zetaclient/zetacore/constant.go
🧰 Additional context used
📓 Path-based instructions (12)
cmd/zetaclientd/inbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetaclientd/start.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetaclientd/utils.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
pkg/rpc/clients.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
pkg/rpc/clients_cosmos.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/interfaces/interfaces.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/orchestrator/contextupdater.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/orchestrator/contextupdater_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/orchestrator/orchestrator.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/testutils/mocks/zetacore_client.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/zetacore/client.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/zetacore/tx_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (21)
pkg/rpc/clients_cosmos.go (1)
15-16: Documentation improvement is clear and helpful.
The added comment clearly explains the empty plan return case, which is important for API consumers.
zetaclient/orchestrator/contextupdater_test.go (1)
1-14: LGTM: Well-organized imports and correct package declaration.
The imports are properly organized and the package name correctly matches the directory structure.
cmd/zetaclientd/utils.go (1)
4-4: LGTM: Import additions are appropriate.
The new imports support the context-aware operations and interface requirements for the new function.
Also applies to: 17-17
cmd/zetaclientd/inbound.go (2)
25-25: LGTM: Import addition aligns with refactoring goals.
The addition of the orchestrator package import supports the PR's objective of moving app context updates from zetacore client to orchestrator.
73-74: LGTM: Context update properly migrated to orchestrator.
The change successfully moves the app context update responsibility to the orchestrator package while maintaining proper error handling, aligning with the single-responsibility principle outlined in the PR objectives.
zetaclient/zetacore/client.go (1)
25-25: LGTM: Import changes align with stateless client design.
The addition of the logs package and removal of unused imports (context, time) aligns well with the PR's objective of transforming the client into a stateless component.
zetaclient/chains/interfaces/interfaces.go (2)
14-14: LGTM: Import aligns with new functionality.
The addition of the upgrade types import is necessary for the new GetUpgradePlan method's return type.
138-140: LGTM: Method changes align with refactoring goals.
The addition of GetUpgradePlan and reorganization of PostOutboundTracker support the transition to a stateless client design. Let's verify the implementation across the codebase.
✅ Verification successful
Let me verify the implementation of GetUpgradePlan since that's a key addition to the interface.
Method changes are properly implemented and integrated
The verification confirms:
GetUpgradePlanis correctly implemented inpkg/rpc/clients_cosmos.gousing the upgrade module'sCurrentPlanendpoint- The method is properly integrated into the context updater workflow in
zetaclient/orchestrator/contextupdater.go PostOutboundTrackerrelocation maintains interface consistency- Test coverage exists in
pkg/rpc/clients_test.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of new methods and removal of Stop()
# Test 1: Check for complete implementation of new methods
echo "Checking implementation of new interface methods..."
rg -A 5 "func.*GetSupportedChains|func.*GetAdditionalChains|func.*GetChainParams|func.*GetUpgradePlan"
# Test 2: Verify Stop() method removal
echo "Verifying Stop() method removal..."
rg "func.*Stop\(\s*\)"
# Test 3: Check for any remaining direct app context updates
echo "Checking for direct app context updates..."
rg "zctx\.AppContext"
Length of output: 37734
Script:
#!/bin/bash
# Check implementation of GetUpgradePlan
echo "Checking GetUpgradePlan implementation..."
rg -A 5 "func.*GetUpgradePlan|GetCurrentPlan"
# Check usage of GetUpgradePlan
echo "Checking GetUpgradePlan usage..."
rg "GetUpgradePlan\("
Length of output: 2086
zetaclient/zetacore/tx_test.go (2)
Line range hint 1-1: Verify test coverage for relocated app context update functionality.
The removal of the UpdateAppContext test from this file aligns with the PR objective of moving the app context update functionality to the orchestrator. However, we should ensure equivalent test coverage exists in the orchestrator package.
#!/bin/bash
# Search for new test coverage in orchestrator package
fd -e go -x rg -l "TestUpdateAppContext|TestAppContextUpdate" {} \; orchestrator/18-18: Verify thorchain TSS dependency management.
The addition of the thorchain TSS blame package import suggests a dependency on an external TSS implementation. Ensure this dependency is properly vendored and version-locked in the module management system.
✅ Verification successful
TSS dependency is properly managed via go.mod replacement directive
The thorchain TSS dependency is correctly declared and managed:
- Declared version:
gitlab.com/thorchain/tss/go-tss v1.6.5 - Replaced with fork:
github.com/zeta-chain/go-tss v0.0.0-20241031223543-18765295f992
The import in zetaclient/zetacore/tx_test.go is consistent with the project's TSS implementation, which is extensively used across the codebase for TSS-related functionality and testing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the TSS dependency is properly declared in go.mod
rg -A 1 "gitlab.com/thorchain/tss/go-tss" go.mod
Length of output: 86
Script:
#!/bin/bash
# Let's verify the TSS dependency usage and declaration comprehensively
# Check for TSS imports across the codebase
echo "=== TSS Import Usage ==="
rg -l "gitlab.com/thorchain/tss/go-tss"
# Check go.mod content for any thorchain dependencies
echo -e "\n=== Thorchain Dependencies in go.mod ==="
rg "thorchain" go.mod
# Check if there are any vendored TSS packages
echo -e "\n=== Vendored TSS Packages ==="
fd -t d "tss" vendor/
# Look for TSS-related test functions
echo -e "\n=== TSS Test Functions ==="
rg "func.*Test.*TSS|func.*Test.*Blame" -g "*.go"
Length of output: 4450
cmd/zetaclientd/start.go (3)
103-105: LGTM: Successfully decoupled context update logic
The changes effectively move the context update responsibility from the ZetaCore client to dedicated functions, improving separation of concerns and maintainability.
Also applies to: 144-146
349-349: LGTM: Enhanced error handling
The addition of error wrapping with contextual messages improves error traceability and debugging capabilities.
Also applies to: 366-366, 371-371
388-393: LGTM: Clean shutdown handling
The shutdown logic is now more straightforward and properly orchestrated through the maestro.Stop() call.
zetaclient/testutils/mocks/zetacore_client.go (4)
23-24: LGTM: Import addition is appropriate.
The addition of the upgradetypes import from cosmos-sdk is necessary to support the new GetUpgradePlan functionality.
51-79: LGTM: New mock methods follow established patterns.
The newly added mock methods (GetAdditionalChains, GetChainParams, GetSupportedChains, GetUpgradePlan) are well-implemented with:
- Consistent error handling
- Proper return value checking
- Clear panic messages
- Appropriate type assertions
Also applies to: 197-225, 499-527, 587-613
Line range hint 1-1000: LGTM: Mock implementation maintains consistency and quality.
The mock implementation:
- Follows consistent patterns across all methods
- Maintains proper error handling
- Aligns with the PR objectives of transforming the zetacore client
- Uses appropriate type assertions and panic messages
Line range hint 1-1: Verify complete removal of Stop-related functionality.
The removal of OnBeforeStop and Stop methods aligns with the PR objective to transform the zetacore client into a stateless client. Let's verify no lingering references exist.
✅ Verification successful
Let me verify if these Stop methods are related to the zetacore client context by checking their usage.
Stop method removal verification completed successfully
The Stop methods found in the codebase are not related to the removed ZetaCore client functionality. They belong to different components:
- DynamicTicker, Metrics, and TelemetryServer: Infrastructure components
- Observer and Orchestrator: Higher-level components that use the ZetaCore client
- Various mocks and utilities
The original OnBeforeStop and Stop methods from the ZetaCore client have been completely removed, with no lingering references, which aligns with the transformation to a stateless client architecture.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the removed methods
# Test: Search for OnBeforeStop and Stop method references. Expect: No results.
echo "Checking for OnBeforeStop references..."
rg "OnBeforeStop" --type go
echo "Checking for Stop method references in zetacore client..."
rg "func.*Stop\(\s*\)" --type go
Length of output: 968
Script:
#!/bin/bash
# Check for zetacore client related Stop references
echo "Checking ZetaCore client interface definition..."
rg "type ZetaCoreClient interface" -A 20 --type go
echo "Checking for imports of zetacore client in files with Stop methods..."
for file in $(rg -l 'func.*Stop\(\s*\)' --type go); do
echo "=== $file ==="
rg "zetacore|ZetaCore" "$file"
done
Length of output: 3833
zetaclient/orchestrator/orchestrator.go (2)
137-137: LGTM: Background worker initialization is consistent
The addition of runAppContextUpdater follows the established pattern for background worker initialization and maintains consistent error handling.
780-786: LGTM: Graceful shutdown enhancement
The addition of ticker.WithStopChan(oc.stop) improves the graceful shutdown capabilities of the observer-signer synchronization process.
Let's verify that all ticker usages in the codebase consistently handle graceful shutdown:
✅ Verification successful
All ticker.Run calls consistently implement graceful shutdown
The codebase demonstrates consistent usage of ticker.WithStopChan across all ticker.Run implementations, including:
- EVM chain observers
- TON chain observers
- Orchestrator components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of WithStopChan across ticker implementations
# Expect: All ticker.Run calls should include WithStopChan for graceful shutdown
ast-grep --pattern 'ticker.Run($$$, $$$, $$$, $$$)'
Length of output: 3404
changelog.md (1)
Line range hint 1-1000: Version numbering follows semantic versioning.
The changelog correctly follows semantic versioning (MAJOR.MINOR.PATCH) with 'v' prefix, making version tracking clear and consistent.
zetaclient/orchestrator/contextupdater.go (1)
108-126: Confirm exclusion of ZetaChain from chainParams
The code intentionally excludes ZetaChain from chainParams. Verify that this exclusion is appropriate and that ZetaChain parameters are handled elsewhere to prevent potential issues.
Please confirm that ZetaChain's parameters are managed correctly outside this loop and that excluding it here does not lead to incomplete application context updates.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3131 +/- ##
===========================================
+ Coverage 63.08% 63.14% +0.05%
===========================================
Files 423 423
Lines 29875 29874 -1
===========================================
+ Hits 18848 18864 +16
+ Misses 10186 10170 -16
+ Partials 841 840 -1
|
…worker # Conflicts: # changelog.md
swift1337
force-pushed
the
refactor/zetacore-worker
branch
from
49fce59
to
9c9d683
Compare
Previously,
zetacoreClientwas a client that also handled background context updates and orchestrator shutdown, which was confusing. This PR refactors this behavior, making it only responsible for querying the node.Stop()from ZetaCore client, effectively making it a client 🥁 rather than a stateful serviceCloses #2589
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Tests
UpdateAppContextfunction, covering successful updates and upgrade plan detection.