refactor(server/v2): eager config loading #22267
Conversation
github-actions
bot
added
C:CLI
C:x/upgrade
C:x/genutil
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThe changes introduced in this pull request encompass a comprehensive restructuring of the server and runtime modules to enhance configuration management and modularity. New types, functions, and methods are added to facilitate dynamic configuration of modules, allowing for improved initialization and error handling. The modifications also include the removal of certain methods and the introduction of new interfaces to streamline command handling and server setup, while ensuring that existing functionalities remain intact. 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: 0
🧹 Outside diff range and nitpick comments (2)
server/v2/commands.go (2)
29-31: Enhance error message for better debuggingThe error message when no components are provided could be more descriptive to help users understand what components are expected.
Apply this diff to improve the error message:
- return nil, errors.New("no components provided") + return nil, errors.New("no server components provided - at least one ServerComponent must be configured")
173-177: Consider making appBuildingCommands unexportedSince this variable is only used internally by IsAppRequired, it should be unexported to prevent external modification.
Apply this diff:
-var appBuildingCommands = [][]string{ +var defaultAppBuildingCommands = [][]string{
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- server/v2/commands.go (5 hunks)
- simapp/v2/simdv2/cmd/commands.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- simapp/v2/simdv2/cmd/commands.go
🧰 Additional context used
📓 Path-based instructions (1)
server/v2/commands.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (1)
server/v2/commands.go (1)
189-207: LGTM! Well-implemented command path checkingThe implementation is efficient with:
- Clear documentation including complexity analysis
- Pre-allocated slices for better performance
- Proper handling of command hierarchy traversal
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
simapp/v2/simdv2/cmd/commands.go (1)
35-42: LGTM! Consider enhancing documentation.The
CommandDependenciesstruct is well-designed and effectively encapsulates command dependencies. Consider adding godoc comments for each field to document their purpose and requirements.Add field documentation:
type CommandDependencies[T transaction.Tx] struct { + // globalAppConfig contains the application's global configuration globalAppConfig coreserver.ConfigMap + // txConfig provides transaction configuration and encoding txConfig client.TxConfig + // moduleManager manages the application's modules moduleManager *runtimev2.MM[T] + // simApp is the main application instance simApp *simapp.SimApp[T] + // consensusComponent handles consensus-related operations consensusComponent serverv2.ServerComponent[T] }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
- .github/CODEOWNERS (1 hunks)
- simapp/v2/simdv2/cmd/commands.go (2 hunks)
- simapp/v2/simdv2/cmd/root_di.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
simapp/v2/simdv2/cmd/commands.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/simdv2/cmd/root_di.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (3)
.github/CODEOWNERS (1)
28-28: LGTM! The ownership update aligns with recent architectural changes.Adding @kocubinski as a codeowner for the /server/v2/ directory is appropriate given their significant contributions to refactoring the configuration parsing system and addressing the dual dependency injection issues in this module.
simapp/v2/simdv2/cmd/commands.go (2)
137-150: Verify all callers are updated with new function signature.The function signature has changed to accept an additional
appparameter. Let's ensure all callers are updated accordingly.✅ Verification successful
Let me verify the function calls by checking their context.
All callers are properly updated with the new function signature
The verification shows that both callers of
genesisCommandare correctly passing all required parameters:
- In
simapp/v2/simdv2/cmd/commands.go:genesisCommand(deps.moduleManager, deps.simApp)- In
simapp/simd/cmd/commands.go:genesisCommand(moduleManager, appExport)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for function calls to genesisCommand ast-grep --pattern 'genesisCommand($$$)'Length of output: 204
Script:
#!/bin/bash # Get context around the genesisCommand calls rg "genesisCommand" -B 3 -A 1Length of output: 1566
66-82: Verify thread safety of component initialization.The CLI skeleton initialization path creates default components when they're nil. In a concurrent environment, this could lead to race conditions if multiple goroutines attempt to initialize components simultaneously.
| if err := autoCliOpts.EnhanceRootCommand(rootCommand); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for AutoCLI enhancement
The error from EnhanceRootCommand should include more context for better debugging.
if err := autoCliOpts.EnhanceRootCommand(rootCommand); err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to enhance root command with AutoCLI options: %w", err)
}📝 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 := autoCliOpts.EnhanceRootCommand(rootCommand); err != nil { | |
| return nil, err | |
| if err := autoCliOpts.EnhanceRootCommand(rootCommand); err != nil { | |
| return nil, fmt.Errorf("failed to enhance root command with AutoCLI options: %w", err) |
| subCommand, configMap, logger, err := factory.ParseCommand(rootCommand, args) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for command parsing
The error from ParseCommand could benefit from additional context to aid in debugging.
subCommand, configMap, logger, err := factory.ParseCommand(rootCommand, args)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to parse command %q: %w", rootCommand.Name(), err)
}📝 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.
| subCommand, configMap, logger, err := factory.ParseCommand(rootCommand, args) | |
| if err != nil { | |
| return nil, err | |
| subCommand, configMap, logger, err := factory.ParseCommand(rootCommand, args) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to parse command %q: %w", rootCommand.Name(), err) |
| if serverv2.IsAppRequired(subCommand) { | ||
| // server construction | ||
| simApp, err = simapp.NewSimApp[T](depinjectConfig, &autoCliOpts, &moduleManager, &clientCtx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } else { | ||
| // client construction | ||
| if err = depinject.Inject( | ||
| depinject.Configs( | ||
| simapp.AppConfig(), | ||
| depinjectConfig, | ||
| ), | ||
| &autoCliOpts, &moduleManager, &clientCtx, | ||
| ); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Add validation for injected dependencies
The code should validate that critical dependencies are properly initialized after injection for both server and client paths.
if serverv2.IsAppRequired(subCommand) {
simApp, err = simapp.NewSimApp[T](depinjectConfig, &autoCliOpts, &moduleManager, &clientCtx)
if err != nil {
return nil, fmt.Errorf("failed to create SimApp: %w", err)
}
+ if err := validateDependencies(&autoCliOpts, moduleManager, clientCtx); err != nil {
+ return nil, fmt.Errorf("invalid server dependencies: %w", err)
+ }
} else {
if err = depinject.Inject(
depinject.Configs(
simapp.AppConfig(),
depinjectConfig,
),
&autoCliOpts, &moduleManager, &clientCtx,
); err != nil {
return nil, err
}
+ if err := validateDependencies(&autoCliOpts, moduleManager, clientCtx); err != nil {
+ return nil, fmt.Errorf("invalid client dependencies: %w", err)
+ }
}
+func validateDependencies(opts *autocli.AppOptions, mm *runtime.MM[T], ctx client.Context) error {
+ if opts == nil {
+ return fmt.Errorf("autocli options not initialized")
+ }
+ if mm == nil {
+ return fmt.Errorf("module manager not initialized")
+ }
+ if ctx.TxConfig == nil {
+ return fmt.Errorf("transaction config not initialized")
+ }
+ return nil
+}Committable suggestion was skipped due to low confidence.
simapp/v2/simdv2/cmd/commands.go
Outdated
| func rootCommandPersistentPreRun(clientCtx client.Context) func(*cobra.Command, []string) error { | ||
| return func(cmd *cobra.Command, args []string) error { | ||
| // set the default command outputs | ||
| cmd.SetOut(cmd.OutOrStdout()) | ||
| cmd.SetErr(cmd.ErrOrStderr()) | ||
|
|
||
| // overwrite the FlagInvCheckPeriod | ||
| viper.Set(server.FlagInvCheckPeriod, 1) | ||
| viper.SetDefault(serverv2.FlagHome, simapp.DefaultNodeHome) | ||
| clientCtx = clientCtx.WithCmdContext(cmd.Context()) | ||
| clientCtx, err := client.ReadPersistentCommandFlags(clientCtx, cmd.Flags()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| var simApp *simapp.SimApp[T] | ||
| if height != -1 { | ||
| simApp = simapp.NewSimApp[T](logger, viper) | ||
| customClientTemplate, customClientConfig := initClientConfig() | ||
| clientCtx, err = config.CreateClientConfig( | ||
| clientCtx, customClientTemplate, customClientConfig) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := simApp.LoadHeight(uint64(height)); err != nil { | ||
| return genutilv2.ExportedApp{}, err | ||
| if err = client.SetCmdClientContextHandler(clientCtx, cmd); err != nil { | ||
| return err | ||
| } | ||
| } else { | ||
| simApp = simapp.NewSimApp[T](logger, viper) | ||
| } | ||
|
|
||
| return simApp.ExportAppStateAndValidators(jailAllowedAddrs) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding context timeout.
The command context setup could benefit from a timeout to prevent hanging in case of slow operations or network issues.
func rootCommandPersistentPreRun(clientCtx client.Context) func(*cobra.Command, []string) error {
return func(cmd *cobra.Command, args []string) error {
+ // Add timeout to prevent hanging
+ ctx, cancel := context.WithTimeout(cmd.Context(), 30*time.Second)
+ defer cancel()
- clientCtx = clientCtx.WithCmdContext(cmd.Context())
+ clientCtx = clientCtx.WithCmdContext(ctx)
// ... rest of the function
}
}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
simapp/v2/simdv2/cmd/root_di.go (3)
19-21: Add function documentationAdd a documentation comment explaining the purpose, parameters, and return values of the
NewRootCmdfunction. This will improve code maintainability and help other developers understand the function's usage.+// NewRootCmd creates and initializes a new root command for the simulation app. +// It accepts optional command-line arguments and returns the configured root command +// or an error if initialization fails. func NewRootCmd[T transaction.Tx]( args ...string, ) (*cobra.Command, error) {
35-38: Improve error handling for factory creationAdd context to the error returned from factory creation to aid in debugging.
if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create command factory: %w", err) }
85-89: Improve error handling for command initializationAdd context to errors returned from command initialization steps to aid in debugging.
factory.EnhanceRootCommand(rootCommand) _, err = InitRootCmd(rootCommand, logger, commandDeps) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to initialize root command: %w", err) }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
- .github/CODEOWNERS (1 hunks)
- simapp/v2/simdv2/cmd/commands.go (2 hunks)
- simapp/v2/simdv2/cmd/root_di.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- simapp/v2/simdv2/cmd/commands.go
🧰 Additional context used
📓 Path-based instructions (1)
simapp/v2/simdv2/cmd/root_di.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (1)
.github/CODEOWNERS (1)
28-28: LGTM! Addition of code owner aligns with architectural changes.The addition of @kocubinski as a code owner for
/server/v2/is appropriate given their significant contributions to the module's architecture, particularly the ongoing refactoring of configuration parsing and dependency injection improvements.
simapp/v2/simdv2/cmd/root_di.go
Outdated
| commandDeps := CommandDependencies[T]{ | ||
| globalAppConfig: configMap, | ||
| txConfig: clientCtx.TxConfig, | ||
| moduleManager: moduleManager, | ||
| simApp: simApp, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for command dependencies
The CommandDependencies struct is populated without validation. Consider adding validation to ensure required fields are properly set before proceeding.
+func (d CommandDependencies[T]) validate() error {
+ if d.globalAppConfig == nil {
+ return fmt.Errorf("globalAppConfig is required")
+ }
+ if d.txConfig == nil {
+ return fmt.Errorf("txConfig is required")
+ }
+ if d.moduleManager == nil {
+ return fmt.Errorf("moduleManager is required")
+ }
+ return nil
+}
commandDeps := CommandDependencies[T]{
globalAppConfig: configMap,
txConfig: clientCtx.TxConfig,
moduleManager: moduleManager,
simApp: simApp,
}
+if err := commandDeps.validate(); err != nil {
+ return nil, fmt.Errorf("invalid command dependencies: %w", err)
+}Committable suggestion was skipped due to low confidence.
| depinjectConfig = depinject.Configs( | ||
| depinject.Supply(logger, runtime.GlobalConfig(configMap)), | ||
| depinject.Provide(ProvideClientContext), | ||
| ) | ||
| ) | ||
| if serverv2.IsAppRequired(subCommand) { | ||
| // server construction | ||
| simApp, err = simapp.NewSimApp[T](depinjectConfig, &autoCliOpts, &moduleManager, &clientCtx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } else { | ||
| // client construction | ||
| if err = depinject.Inject( | ||
| depinject.Configs( | ||
| simapp.AppConfig(), | ||
| depinjectConfig, | ||
| ), | ||
| &autoCliOpts, &moduleManager, &clientCtx, | ||
| ); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract common dependency injection logic
The dependency injection configuration is duplicated between server and client paths. Consider extracting the common configuration into a separate function.
+func createBaseConfig(logger log.Logger, configMap runtime.ConfigMap) depinject.Config {
+ return depinject.Configs(
+ depinject.Supply(logger, runtime.GlobalConfig(configMap)),
+ depinject.Provide(ProvideClientContext),
+ )
+}
-depinjectConfig = depinject.Configs(
- depinject.Supply(logger, runtime.GlobalConfig(configMap)),
- depinject.Provide(ProvideClientContext),
-)
+depinjectConfig = createBaseConfig(logger, configMap)Committable suggestion was skipped due to low confidence.
julienrbrt
added
the
backport/v0.52.x
|
Added backport label for partial backport |
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (5)
simapp/v2/simdv2/cmd/depinject.go (1)
53-69: Enhance documentation and error handling.The comment about textual configuration could be more descriptive. Consider explaining:
- Why textual is enabled by default
- The implications of using gRPC instead of bank keeper
- When someone might want to change this default
Suggested comment improvement:
-// textual is enabled by default, we need to re-create the tx config grpc instead of bank keeper. +// Textual coin metadata querying is enabled by default for human-readable transaction display. +// We use gRPC queries instead of the bank keeper to avoid circular dependencies and allow +// metadata queries before the bank module is fully initialized.simapp/v2/simdv2/cmd/root_di.go (1)
19-21: Consider adding documentation about the command initialization patternThe new command initialization pattern is a significant architectural change. Consider adding documentation that explains:
- Why the command needs to be initialized twice
- The role of empty dependencies in the first initialization
- The relationship between server and client construction paths
simapp/v2/app_di.go (1)
87-118: Fix typos in configuration documentationThere are a few typos in the documentation comments:
- Line 111: "provinding" should be "providing"
- Line 113: Consider adding commas for better readability in "and appends "valoper" and "valcons""
simapp/v2/simdv2/cmd/commands.go (1)
35-42: Well-structured dependency container!The
CommandDependenciesstruct provides a clean way to manage command dependencies. The comment about fetching dependencies later from command context suggests good forward-thinking about potential improvements.Consider implementing a builder pattern for this struct in the future to make dependency injection more flexible and testable.
server/v2/api/grpcgateway/server.go (1)
78-78: Address the TODO: Register the gRPC-Gateway routesThe TODO comment indicates that the gRPC-Gateway routes need to be implemented. Registering the gRPC-Gateway routes is essential for the gateway to function correctly.
Would you like assistance in implementing the route registration or opening a GitHub issue to track this task?
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (8)
- runtime/v2/config.go (1 hunks)
- server/v2/api/grpcgateway/server.go (4 hunks)
- server/v2/command_factory.go (1 hunks)
- simapp/v2/app_di.go (4 hunks)
- simapp/v2/app_test.go (2 hunks)
- simapp/v2/simdv2/cmd/commands.go (2 hunks)
- simapp/v2/simdv2/cmd/depinject.go (1 hunks)
- simapp/v2/simdv2/cmd/root_di.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- runtime/v2/config.go
- simapp/v2/app_test.go
🧰 Additional context used
📓 Path-based instructions (6)
server/v2/api/grpcgateway/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/command_factory.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/app_di.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/simdv2/cmd/commands.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/simdv2/cmd/depinject.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/simdv2/cmd/root_di.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
📓 Learnings (1)
simapp/v2/simdv2/cmd/root_di.go (1)
Learnt from: kocubinski PR: cosmos/cosmos-sdk#22267 File: simapp/v2/simdv2/cmd/root_di.go:26-26 Timestamp: 2024-10-28T18:07:44.817Z Learning: In `simapp/v2/simdv2/cmd/root_di.go`, the initial call to `InitRootCmd` with empty `CommandDependencies` is required to initialize configuration.
🔇 Additional comments (7)
simapp/v2/simdv2/cmd/depinject.go (2)
1-18: LGTM! Well-organized imports.The imports are properly organized and grouped by standard library, external, and internal packages.
31-39: LGTM with previous review comments.The error handling concerns were already addressed in previous reviews.
simapp/v2/app_di.go (3)
49-63: LGTM! Well-structured configuration setup.The configuration changes improve modularity by:
- Separating module configuration
- Adding explicit account type providers
- Following dependency injection best practices
73-178: LGTM! Robust implementation with proper error handling.The refactored NewSimApp function:
- Uses dependency injection properly
- Implements thorough error handling
- Provides clear initialization flow
213-215: LGTM! Clean and focused implementation.The function follows the single responsibility principle and provides a clear interface for root store configuration.
simapp/v2/simdv2/cmd/commands.go (2)
66-82: Clean separation of CLI skeleton initialization!The conditional initialization based on
deps.SimAppprovides a clear separation between CLI skeleton and full app modes.
197-221:⚠️ Potential issueEnhance error handling with context
While the function correctly handles command setup, the error returns could be more informative.
Add context to error returns:
clientCtx, err := client.ReadPersistentCommandFlags(clientCtx, cmd.Flags()) if err != nil { - return err + return fmt.Errorf("failed to read persistent command flags: %w", err) } clientCtx, err = config.CreateClientConfig(clientCtx, customClientTemplate, customClientConfig) if err != nil { - return err + return fmt.Errorf("failed to create client config: %w", err) } if err = client.SetCmdClientContextHandler(clientCtx, cmd); err != nil { - return err + return fmt.Errorf("failed to set client context handler: %w", err) }Likely invalid or redundant comment.
| clientCtx := client.Context{}. | ||
| WithCodec(appCodec). | ||
| WithInterfaceRegistry(interfaceRegistry). | ||
| WithLegacyAmino(amino). | ||
| WithInput(os.Stdin). | ||
| WithAccountRetriever(types.AccountRetriever{}). | ||
| WithAddressCodec(addressCodec). | ||
| WithValidatorAddressCodec(validatorAddressCodec). | ||
| WithConsensusAddressCodec(consensusAddressCodec). | ||
| WithHomeDir(homeDir). | ||
| WithViper("") // uses by default the binary name as prefix | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making input source configurable.
The direct use of os.Stdin might make testing difficult and reduce flexibility. Consider accepting an io.Reader as a parameter or configuration option.
+type ClientContextConfig struct {
+ // ... other fields ...
+ Input io.Reader
+}
clientCtx := client.Context{}.
WithCodec(appCodec).
WithInterfaceRegistry(interfaceRegistry).
WithLegacyAmino(amino).
- WithInput(os.Stdin).
+ WithInput(codecConfig.Input).Committable suggestion was skipped due to low confidence.
| // ProvideClientContext is a depinject Provider function which assembles and returns a client.Context. | ||
| func ProvideClientContext( | ||
| configMap runtime.GlobalConfig, | ||
| appCodec codec.Codec, | ||
| interfaceRegistry codectypes.InterfaceRegistry, | ||
| txConfigOpts tx.ConfigOptions, | ||
| legacyAmino registry.AminoRegistrar, | ||
| addressCodec address.Codec, | ||
| validatorAddressCodec address.ValidatorAddressCodec, | ||
| consensusAddressCodec address.ConsensusAddressCodec, | ||
| ) client.Context { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider returning error and grouping parameters.
While the function is well-documented, consider these improvements:
- Return an error instead of using panics (as mentioned in previous reviews).
- Group related parameters (e.g., address codecs) into a configuration struct to improve maintainability.
Example refactor:
-func ProvideClientContext(
+type ClientContextConfig struct {
+ AddressCodec address.Codec
+ ValidatorAddressCodec address.ValidatorAddressCodec
+ ConsensusAddressCodec address.ConsensusAddressCodec
+}
+
+func ProvideClientContext(
configMap runtime.GlobalConfig,
appCodec codec.Codec,
interfaceRegistry codectypes.InterfaceRegistry,
txConfigOpts tx.ConfigOptions,
legacyAmino registry.AminoRegistrar,
- addressCodec address.Codec,
- validatorAddressCodec address.ValidatorAddressCodec,
- consensusAddressCodec address.ConsensusAddressCodec,
-) client.Context {
+ codecConfig ClientContextConfig,
+) (client.Context, error) {Committable suggestion was skipped due to low confidence.
| configWriter, err := InitRootCmd(rootCommand, log.NewNopLogger(), CommandDependencies[T]{}) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for InitRootCmd
The error from InitRootCmd should include more context for better debugging.
configWriter, err := InitRootCmd(rootCommand, log.NewNopLogger(), CommandDependencies[T]{})
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to initialize root command: %w", err)
}📝 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.
| configWriter, err := InitRootCmd(rootCommand, log.NewNopLogger(), CommandDependencies[T]{}) | |
| if err != nil { | |
| return nil, err | |
| } | |
| configWriter, err := InitRootCmd(rootCommand, log.NewNopLogger(), CommandDependencies[T]{}) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to initialize root command: %w", err) | |
| } |
| if err = depinject.Inject( | ||
| depinject.Configs( | ||
| simapp.AppConfig(), | ||
| depinjectConfig, | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify dependency injection configuration
The nested depinject.Configs calls can be flattened as they serve the same purpose.
if err = depinject.Inject(
- depinject.Configs(
- simapp.AppConfig(),
- depinjectConfig,
- ),
+ simapp.AppConfig(),
+ depinjectConfig,
&autoCliOpts, &moduleManager, &clientCtx,
); err != nil {📝 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 = depinject.Inject( | |
| depinject.Configs( | |
| simapp.AppConfig(), | |
| depinjectConfig, | |
| ), | |
| if err = depinject.Inject( | |
| simapp.AppConfig(), | |
| depinjectConfig, |
| nodeCmds := nodeservice.NewNodeCommands() | ||
| autoCliOpts.ModuleOptions = make(map[string]*autocliv1.ModuleOptions) | ||
| autoCliOpts.ModuleOptions[nodeCmds.Name()] = nodeCmds.AutoCLIOptions() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider error handling for node commands initialization
The node commands initialization and AutoCLI options setup should be wrapped in a separate function with proper error handling.
+func setupNodeCommands(opts *autocli.AppOptions) error {
+ nodeCmds := nodeservice.NewNodeCommands()
+ if nodeCmds == nil {
+ return fmt.Errorf("failed to create node commands")
+ }
+ if opts.ModuleOptions == nil {
+ opts.ModuleOptions = make(map[string]*autocliv1.ModuleOptions)
+ }
+ opts.ModuleOptions[nodeCmds.Name()] = nodeCmds.AutoCLIOptions()
+ return nil
+}
-nodeCmds := nodeservice.NewNodeCommands()
-autoCliOpts.ModuleOptions = make(map[string]*autocliv1.ModuleOptions)
-autoCliOpts.ModuleOptions[nodeCmds.Name()] = nodeCmds.AutoCLIOptions()
+if err := setupNodeCommands(&autoCliOpts); err != nil {
+ return nil, fmt.Errorf("failed to setup node commands: %w", err)
+}Committable suggestion was skipped due to low confidence.
| if errors.Is(err, pflag.ErrHelp) { | ||
| return cmd, nil, nil, nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use cobra.ErrHelp instead of pflag.ErrHelp for help requests
When checking for help requests, it's more appropriate to use cobra.ErrHelp to align with the Cobra library's error handling. This ensures consistency and better integration with Cobra's command parsing logic.
Apply this diff to use cobra.ErrHelp:
if err = cmd.ParseFlags(args); err != nil {
// help requested, return the command early
if errors.Is(err, pflag.ErrHelp) {
- return cmd, nil, nil, nil
+ return nil, nil, nil, cobra.ErrHelp
}
return nil, nil, nil, err
}Make sure to import the Cobra errors package if not already included:
import (
// ...
"github.com/spf13/cobra"
// ...
)| func NewCommandFactory(opts ...CommandFactoryOption) (*CommandFactory, error) { | ||
| f := &CommandFactory{} | ||
| for _, opt := range opts { | ||
| err := opt(f) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| return f, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider initializing vipr in NewCommandFactory
Currently, vipr is not initialized in the NewCommandFactory function, which could lead to nil pointer dereferences when it's used before being set. Initialize vipr during the creation of the CommandFactory to ensure it's ready for use.
Modify the NewCommandFactory function as follows:
func NewCommandFactory(opts ...CommandFactoryOption) (*CommandFactory, error) {
f := &CommandFactory{
+ vipr: viper.New(),
}
for _, opt := range opts {
err := opt(f)
if err != nil {
return nil, err
}
}
return f, nil
}This ensures that vipr is always available when methods like enhanceCommand are called.
📝 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.
| func NewCommandFactory(opts ...CommandFactoryOption) (*CommandFactory, error) { | |
| f := &CommandFactory{} | |
| for _, opt := range opts { | |
| err := opt(f) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } | |
| return f, nil | |
| } | |
| func NewCommandFactory(opts ...CommandFactoryOption) (*CommandFactory, error) { | |
| f := &CommandFactory{ | |
| vipr: viper.New(), | |
| } | |
| for _, opt := range opts { | |
| err := opt(f) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } | |
| return f, nil | |
| } |
| viper.SetEnvPrefix(f.envPrefix) | ||
| viper.SetEnvKeyReplacer(strings.NewReplacer(".", "_", "-", "_")) | ||
| viper.AutomaticEnv() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using the global Viper instance; use the vipr field instead
In the enhanceCommand method, the global Viper instance is being modified. This can lead to unexpected behavior, especially if multiple instances of CommandFactory are used concurrently in different parts of the application. It's better to utilize the vipr field within CommandFactory to maintain instance-specific configurations and avoid side effects.
Apply this diff to use the local Viper instance:
func (f *CommandFactory) enhanceCommand(cmd *cobra.Command) {
pflags := cmd.PersistentFlags()
pflags.String(FlagLogLevel, "info", "The logging level (trace|debug|info|warn|error|fatal|panic|disabled or '*:<level>,<key>:<level>')")
pflags.String(FlagLogFormat, "plain", "The logging format (json|plain)")
pflags.Bool(FlagLogNoColor, false, "Disable colored logs")
pflags.StringP(FlagHome, "", f.defaultHomeDir, "directory for config and data")
- viper.SetEnvPrefix(f.envPrefix)
- viper.SetEnvKeyReplacer(strings.NewReplacer(".", "_", "-", "_"))
- viper.AutomaticEnv()
+ f.vipr.SetEnvPrefix(f.envPrefix)
+ f.vipr.SetEnvKeyReplacer(strings.NewReplacer(".", "_", "-", "_"))
+ f.vipr.AutomaticEnv()
}📝 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.
| viper.SetEnvPrefix(f.envPrefix) | |
| viper.SetEnvKeyReplacer(strings.NewReplacer(".", "_", "-", "_")) | |
| viper.AutomaticEnv() | |
| } | |
| f.vipr.SetEnvPrefix(f.envPrefix) | |
| f.vipr.SetEnvKeyReplacer(strings.NewReplacer(".", "_", "-", "_")) | |
| f.vipr.AutomaticEnv() | |
| } |
| homeDir, envHome := "", "HOME" | ||
| if len(f.envPrefix) > 0 { | ||
| homeDir = os.Getenv(f.envPrefix + "_" + envHome) | ||
| } else { | ||
| homeDir = os.Getenv("NODE_" + envHome) | ||
| } | ||
| if homeDir != "" { | ||
| f.defaultHomeDir = filepath.Clean(homeDir) |
There was a problem hiding this comment.
Potential confusion with environment variable handling when envPrefix is empty
When envPrefix is empty, the code defaults to using the NODE_HOME environment variable to determine the home directory. This might not align with user expectations and could lead to unintended configurations. Consider defaulting to the standard HOME environment variable or ensuring that envPrefix is explicitly set to avoid ambiguity.
Apply this diff to default to the HOME environment variable when envPrefix is empty:
if len(f.envPrefix) > 0 {
homeDir = os.Getenv(f.envPrefix + "_" + envHome)
} else {
- homeDir = os.Getenv("NODE_" + envHome)
+ homeDir = os.Getenv(envHome)
}
if homeDir != "" {
f.defaultHomeDir = filepath.Clean(homeDir)
return nil
}This change ensures that in the absence of an envPrefix, the standard HOME environment variable is used.
📝 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.
| homeDir, envHome := "", "HOME" | |
| if len(f.envPrefix) > 0 { | |
| homeDir = os.Getenv(f.envPrefix + "_" + envHome) | |
| } else { | |
| homeDir = os.Getenv("NODE_" + envHome) | |
| } | |
| if homeDir != "" { | |
| f.defaultHomeDir = filepath.Clean(homeDir) | |
| homeDir, envHome := "", "HOME" | |
| if len(f.envPrefix) > 0 { | |
| homeDir = os.Getenv(f.envPrefix + "_" + envHome) | |
| } else { | |
| homeDir = os.Getenv(envHome) | |
| } | |
| if homeDir != "" { | |
| f.defaultHomeDir = filepath.Clean(homeDir) |
| f.logger, err = f.loggerFactory(f.vipr.AllSettings(), cmd.OutOrStdout()) | ||
| if err != nil { | ||
| return nil, nil, nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
Check for loggerFactory being nil before using f.logger
In the ParseCommand method, after creating the logger with loggerFactory, there is no check to handle the case where loggerFactory is nil. This could lead to a nil pointer dereference when f.logger is used later.
Add a check to ensure f.logger is not nil before proceeding, or provide a default logger implementation.
if f.loggerFactory != nil {
f.logger, err = f.loggerFactory(f.vipr.AllSettings(), cmd.OutOrStdout())
if err != nil {
return nil, nil, nil, err
}
+} else {
+ return nil, nil, nil, errors.New("loggerFactory is not set")
}Alternatively, ensure that a default loggerFactory is assigned during the CommandFactory initialization if it's a required dependency.
📝 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.
| f.logger, err = f.loggerFactory(f.vipr.AllSettings(), cmd.OutOrStdout()) | |
| if err != nil { | |
| return nil, nil, nil, err | |
| } | |
| } | |
| f.logger, err = f.loggerFactory(f.vipr.AllSettings(), cmd.OutOrStdout()) | |
| if err != nil { | |
| return nil, nil, nil, err | |
| } | |
| } else { | |
| return nil, nil, nil, errors.New("loggerFactory is not set") | |
| } |
(cherry picked from commit 31f97e9) # Conflicts: # .github/CODEOWNERS # runtime/v2/app.go # runtime/v2/builder.go # runtime/v2/module.go # server/v2/api/grpc/server.go # server/v2/api/grpcgateway/server.go # server/v2/api/rest/server.go # server/v2/api/rest/server_test.go # server/v2/api/telemetry/server.go # server/v2/commands.go # server/v2/logger.go # server/v2/server.go # server/v2/server_mock_test.go # server/v2/server_test.go # server/v2/store/server.go # server/v2/store/snapshot.go # server/v2/util.go # simapp/v2/simdv2/cmd/commands.go
* main: (24 commits) build(deps): upgrade to iavl@v1.3.1 (#22436) docs: Update tendermint validators query pagination documentation (#22412) refactor(client/v2): offchain uses client/v2/factory (#22344) feat: wire new handlers to grpc (#22333) fix(x/group): proper address rendering in error (#22425) refactor(serevr/v2/cometbft): update `RegisterQueryHandlers` and GRPC queries (#22403) docs: update ADR 59 (#22423) build(deps): Bump github.com/fsnotify/fsnotify from 1.7.0 to 1.8.0 in /tools/cosmovisor (#22407) docs: Module account address documentation (#22289) feat(baseapp): add per message telemetry (#22175) docs: Update Twitter Links to X in Documentation (#22408) docs: redirect the remote generation page (#22404) refactor(serverv2): remove unused interface methods, honuor context (#22394) fix(server/v2): return ErrHelp (#22399) feat(indexer): implement `schema.HasModuleCodec` interface in the `bank` module (#22349) refactor(math): refactor ApproxRoot for readality (#22263) docs: fix KWallet Handbook (#22395) feat(client/v2): broadcast logic (#22282) refactor(server/v2): eager config loading (#22267) test(system): check feePayers signature (#22389) ...
Description
Fixes: #22268
This PR refactors config parsing for server/v2. A long standing point of confusion since the adoption of depinject has been a pattern where dependency injection happens twice during application start up. After some analysis this seemed to only be necessary due to late binding of config, somewhat of an outgrowth of cobras command architecture.
Config parsing has been pulled up as early possible into
NewRootCmd. To facilitate thisinitRootCmdis called twice; once to initialize the CLI skeleton so that flags may be parsed (then combined with viper app.toml parsing), and then a second time immediately after DI occurs. The first invocation requires no DI outputs, the second does. A fully realized configuration blob is available immediately after the firstinitRootCmdinvocation, and is then supplied to DI and ServerComponent constructors.Another new feature of this design is introspection on the subcommand (e.g.
start,export genesis) in order to determine if a client command (i.e. no SimApp needed) or a server command has been invoked. These two cases use different composition roots for injection. As a consequence of this the function closures (which provided late bound config) fornewAppappExportare no longer needed; the server can be passed directly to the command handler.I believe this paves the way for a minimal root command helper so that even more boilerplate may be kept out of simapp/v2 and pushed down into server/v2, which would be a good follow up issue. The code seems much easier to follow now too since there is much less indirection and circular references which were previously resolved by late bound function closures.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
CommandFactoryto streamline command creation and configuration handling.Bug Fixes
Refactor
server.ConfigMap.Documentation
Chores
.github/CODEOWNERSfile.