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: 8
🧹 Outside diff range and nitpick comments (8)
scripts/init-simapp-v2.sh (4)
Line range hint
4-7: Enhance error handling for binary validation.The current binary validation could be more robust by checking if the found binary is executable.
Consider this enhanced validation:
-SIMD_BIN=${SIMD_BIN:=$(which simdv2 2>/dev/null)} - -if [ -z "$SIMD_BIN" ]; then echo "SIMD_BIN is not set. Make sure to run 'COSMOS_BUILD_OPTIONS=v2 make install' before"; exit 1; fi -echo "using $SIMD_BIN" +SIMD_BIN=${SIMD_BIN:=$(which simdv2 2>/dev/null)} + +if [ -z "$SIMD_BIN" ]; then + echo "Error: SIMD_BIN is not set. Make sure to run 'COSMOS_BUILD_OPTIONS=v2 make install' before" + exit 1 +elif [ ! -x "$SIMD_BIN" ]; then + echo "Error: $SIMD_BIN is not executable" + exit 1 +fi +echo "Using binary: $SIMD_BIN"
Line range hint
8-13: Add error handling for configuration commands.The configuration commands should verify their success to ensure proper initialization.
Consider wrapping the commands in a function with error checking:
+set -e # Exit on error + +configure_client() { + local cmd=$1 + local output + + if ! output=$($cmd); then + echo "Error executing: $cmd" + echo "Output: $output" + exit 1 + fi +} + SIMD_HOME=$($SIMD_BIN config home) if [ -d "$SIMD_HOME" ]; then rm -rv $SIMD_HOME; fi -$SIMD_BIN config set client chain-id simapp-v2-chain -$SIMD_BIN config set client keyring-backend test -$SIMD_BIN config set client keyring-default-keyname alice -$SIMD_BIN config set app rest.enable true +configure_client "$SIMD_BIN config set client chain-id simapp-v2-chain" +configure_client "$SIMD_BIN config set client keyring-backend test" +configure_client "$SIMD_BIN config set client keyring-default-keyname alice" +configure_client "$SIMD_BIN config set app rest.enable true"
Line range hint
17-20: Improve genesis file modification safety.The current
jqmodifications could be safer by validating the JSON structure and backing up the genesis file.Consider this safer approach:
+# Backup genesis file +cp "$SIMD_HOME/config/genesis.json" "$SIMD_HOME/config/genesis.json.backup" + +modify_genesis() { + local path=$1 + local value=$2 + local temp_file + + # Create temporary file in the same directory + temp_file=$(mktemp "$SIMD_HOME/config/.genesis.tmp.XXXXXX") + + if ! jq --arg value "$value" "$path = \$value" "$SIMD_HOME/config/genesis.json" > "$temp_file"; then + echo "Error: Failed to modify genesis file at path $path" + rm -f "$temp_file" + exit 1 + fi + + mv "$temp_file" "$SIMD_HOME/config/genesis.json" +} + -jq '.app_state.gov.params.voting_period = "600s"' $SIMD_HOME/config/genesis.json > temp.json && mv temp.json $SIMD_HOME/config/genesis.json -jq '.app_state.gov.params.expedited_voting_period = "300s"' $SIMD_HOME/config/genesis.json > temp.json && mv temp.json $SIMD_HOME/config/genesis.json -jq '.app_state.mint.minter.inflation = "0.300000000000000000"' $SIMD_HOME/config/genesis.json > temp.json && mv temp.json $SIMD_HOME/config/genesis.json +modify_genesis '.app_state.gov.params.voting_period' "600s" +modify_genesis '.app_state.gov.params.expedited_voting_period' "300s" +modify_genesis '.app_state.mint.minter.inflation' "0.300000000000000000"
Add error handling for genesis account setup and transaction collection
The review suggestion is valid as both init scripts lack proper error handling for the genesis account setup and transaction collection commands. The scripts already have error handling for the
SIMD_BINcheck but miss it for critical genesis operations. The suggested refactoring would improve reliability by:
- Wrapping account creation in a function with error checking
- Adding error handling for genesis transaction generation
- Adding error handling for transaction collection
- Maintaining consistency with existing error handling patterns in the scripts
The proposed changes in the review comment are appropriate and would enhance the script's robustness.
🔗 Analysis chain
Line range hint
21-24: Add validation for genesis account setup.The genesis account setup and transaction collection should include error checking.
Let's verify the account setup commands:
Consider adding error handling:
+setup_genesis_account() { + local account=$1 + local amount=$2 + + if ! $SIMD_BIN genesis add-genesis-account "$account" "$amount" --keyring-backend test; then + echo "Error: Failed to add genesis account for $account" + exit 1 + fi +} + -$SIMD_BIN genesis add-genesis-account alice 5000000000stake --keyring-backend test -$SIMD_BIN genesis add-genesis-account bob 5000000000stake --keyring-backend test -$SIMD_BIN genesis gentx alice 1000000stake --chain-id simapp-v2-chain -$SIMD_BIN genesis collect-gentxs +setup_genesis_account "alice" "5000000000stake" +setup_genesis_account "bob" "5000000000stake" + +if ! $SIMD_BIN genesis gentx alice 1000000stake --chain-id simapp-v2-chain; then + echo "Error: Failed to generate genesis transaction" + exit 1 +fi + +if ! $SIMD_BIN genesis collect-gentxs; then + echo "Error: Failed to collect genesis transactions" + exit 1 +fi🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the genesis accounts are properly set up if [ -f "$SIMD_HOME/config/genesis.json" ]; then # Check if the accounts exist in genesis jq -e '.app_state.auth.accounts[] | select(.address != null)' "$SIMD_HOME/config/genesis.json" fiLength of output: 33
Script:
#!/bin/bash # Let's check the actual script content and any similar scripts for error handling patterns rg -l "genesis add-genesis-account" --type sh rg -l "genesis collect-gentxs" --type sh rg -A 5 "if.*genesis" --type shLength of output: 772
Script:
#!/bin/bash # Let's examine the content of both init scripts to see their implementation cat scripts/init-simapp.sh scripts/init-simapp-v2.shLength of output: 2790
simapp/v2/simdv2/cmd/root_di.go (2)
35-42: Enhance error handling with contextThe error handling could be more descriptive to help with debugging. Consider wrapping errors with additional context.
if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create command factory: %w", err) } subCommand, configMap, logger, err := factory.ParseCommand(rootCommand, args) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to parse command: %w", err) }
50-55: Remove unnecessary nesting of depinject.ConfigsThe nested
depinject.Configscalls are redundant and can be simplified.-depinjectConfig = depinject.Configs( - depinject.Configs( - depinject.Supply(logger, runtime.GlobalConfig(configMap)), - depinject.Provide(ProvideClientContext), - ), -) +depinjectConfig = depinject.Configs( + depinject.Supply(logger, runtime.GlobalConfig(configMap)), + depinject.Provide(ProvideClientContext), +)server/v2/command_builder.go (1)
27-32: Create the referenced issue for removing the viper field.The TODO comment mentions that an issue needs to be created after merging #22267. Please create the issue and update the comment with the actual issue number to replace
<CREATE ME>.Would you like me to help create this issue to track the removal of the viper field?
simapp/v2/simdv2/cmd/commands.go (1)
35-42: Enhance type documentation with field descriptions.While the type documentation provides a good overview, consider adding detailed documentation for each field to improve maintainability.
// commandDependencies is a struct that contains all the dependencies needed to initialize the root command. // an alternative design could fetch these even later from the command context +// +// Fields: +// - globalAppConfig: Configuration map for the entire application +// - txConfig: Transaction configuration for encoding/decoding +// - moduleManager: Manages the application's modules +// - simApp: Instance of the simulation application +// - consensusComponent: Handles consensus-related operations type commandDependencies[T transaction.Tx] struct {
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (10)
- scripts/init-simapp-v2.sh (1 hunks)
- server/v2/api/grpc/server.go (3 hunks)
- server/v2/api/rest/server.go (2 hunks)
- server/v2/api/rest/server_test.go (1 hunks)
- server/v2/cometbft/server.go (3 hunks)
- server/v2/command_builder.go (1 hunks)
- simapp/v2/simdv2/cmd/commands.go (2 hunks)
- simapp/v2/simdv2/cmd/root_di.go (1 hunks)
- simapp/v2/simdv2/cmd/testnet.go (1 hunks)
- x/genutil/v2/types.go (0 hunks)
💤 Files with no reviewable changes (1)
- x/genutil/v2/types.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/v2/api/rest/server_test.go
- simapp/v2/simdv2/cmd/testnet.go
🧰 Additional context used
📓 Path-based instructions (6)
server/v2/api/grpc/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/api/rest/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/command_builder.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/root_di.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (19)
server/v2/api/rest/server.go (5)
9-9: LGTM: Import additions align with new functionalityThe new imports support the eager config loading and dependency injection requirements.
Also applies to: 13-13
43-43: Type assertion can be avoidedThe type assertion
srv.Config().(*Config)is still present. As mentioned in previous reviews, this could be made type-safe by changingConfig()to return*Configdirectly.
29-52: LGTM: Improved initialization with proper DI and error handlingThe changes to
Newfunction properly implement:
- Early configuration loading
- Dependency injection through appManager
- Explicit error handling for config unmarshaling
54-61: LGTM: Well-documented config initialization functionThe new
NewWithConfigOptionsfunction provides a clean way to handle configuration separately from server initialization, supporting the eager config loading objective.
29-52: Verify impact on snapshot restore functionalitySince this refactor changes server initialization, please verify that it resolves the snapshot restore panic from issue #22268.
simapp/v2/simdv2/cmd/root_di.go (2)
88-97: Review the order of command enhancement operationsThe current sequence of operations (factory enhancement → initialization → AutoCLI enhancement) might lead to conflicts or overrides in command configuration. Consider consolidating all enhancements before the final initialization.
#!/bin/bash # Check for potential command conflicts or overrides rg -A 3 "Command.*already|flag.*already"
19-29: Consider improving initial command setupThe initial setup uses a nop logger and empty command dependencies, which might mask potential initialization issues. Consider passing a proper logger and validating the command dependencies even during the initial setup phase.
server/v2/api/grpc/server.go (5)
Line range hint
24-28: LGTM: Import changes are appropriate.The new imports are correctly organized and necessary for the enhanced server functionality.
46-55: LGTM: Constructor signature improvements enhance dependency injection.The new constructor signature explicitly requires all dependencies, making the initialization process more robust and maintainable.
83-89: LGTM: Well-documented configuration constructor.The new constructor clearly documents its limited purpose and usage constraints.
200-200: LGTM: Start method implementation.The implementation correctly handles server startup with appropriate error handling and logging.
67-70: Verify GRPC server configuration and reflection setup.The GRPC server configuration looks correct, but let's verify the usage of these handlers across the codebase.
Also applies to: 74-74
✅ Verification successful
Let me gather more information about the GRPC handler implementation to ensure completeness of verification.
GRPC server configuration is correctly implemented and consistent
The verification shows:
- The GRPC server configuration is consistent across different server implementations
- The custom codec and unknown service handler are properly implemented
- Query handlers are correctly registered and used throughout the codebase
- GRPC reflection is properly set up for external client discovery
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify GRPC handler usage patterns # Look for other GRPC server configurations to ensure consistency # Search for other GRPC server configurations rg -A 3 "grpc.NewServer\(" --type go # Search for query handler registrations rg "map\[string\]appmodulev2.Handler" --type goLength of output: 3312
Script:
#!/bin/bash # Check the implementation of newProtoCodec and UnknownServiceHandler ast-grep --pattern 'func newProtoCodec($_) $_' # Look for the actual implementation of UnknownServiceHandler ast-grep --pattern 'func makeUnknownServiceHandler($_) $_' # Check if there are any other similar handler patterns rg -A 5 "UnknownServiceHandler" --type goLength of output: 1374
simapp/v2/simdv2/cmd/commands.go (2)
66-82: LGTM: CLI skeleton initialization looks good.The initial config parsing logic with nil checks is well-structured.
141-144:⚠️ Potential issueAdd safe type assertion for genutil module.
The current type assertion could panic if the module is not of the expected type.
- genTxValidator = moduleManager.Modules()[genutiltypes.ModuleName].(genutil.AppModule).GenTxValidator() + if module, ok := moduleManager.Modules()[genutiltypes.ModuleName].(genutil.AppModule); ok { + genTxValidator = module.GenTxValidator() + } else { + return nil, fmt.Errorf("module %s is not of type genutil.AppModule", genutiltypes.ModuleName) + }Likely invalid or redundant comment.
server/v2/cometbft/server.go (5)
21-31: LGTM: Import changes are well-organizedThe new imports are properly organized and necessary for the enhanced functionality.
67-67: LGTM: Improved error handling in New functionThe addition of error return type enhances error handling and follows Go best practices.
80-86: LGTM: Robust error handling in configuration unmarshalingThe error handling for configuration unmarshaling is comprehensive and provides clear error messages.
149-157: LGTM: Well-implemented indexer initializationThe indexer setup includes proper error handling, logging, and configuration validation.
164-170: LGTM: Well-documented configuration functionThe
NewWithConfigOptionsfunction is properly documented with its limitations and follows Go best practices.
| indexEvents := make(map[string]struct{}, len(srv.config.AppTomlConfig.IndexEvents)) | ||
| for _, e := range srv.config.AppTomlConfig.IndexEvents { | ||
| indexEvents[e] = struct{}{} | ||
| } |
There was a problem hiding this comment.
Add nil check for AppTomlConfig.IndexEvents
The initialization of indexEvents assumes AppTomlConfig.IndexEvents is not nil. Add a nil check to prevent potential panic.
-indexEvents := make(map[string]struct{}, len(srv.config.AppTomlConfig.IndexEvents))
-for _, e := range srv.config.AppTomlConfig.IndexEvents {
+events := srv.config.AppTomlConfig.IndexEvents
+if events == nil {
+ events = []string{}
+}
+indexEvents := make(map[string]struct{}, len(events))
+for _, e := range events {
indexEvents[e] = struct{}{}
}📝 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.
| indexEvents := make(map[string]struct{}, len(srv.config.AppTomlConfig.IndexEvents)) | |
| for _, e := range srv.config.AppTomlConfig.IndexEvents { | |
| indexEvents[e] = struct{}{} | |
| } | |
| events := srv.config.AppTomlConfig.IndexEvents | |
| if events == nil { | |
| events = []string{} | |
| } | |
| indexEvents := make(map[string]struct{}, len(events)) | |
| for _, e := range events { | |
| indexEvents[e] = struct{}{} | |
| } |
julienrbrt
left a comment
julienrbrt
left a comment
There was a problem hiding this comment.
Those new APIs are great. My comments are only nits now. Love the new flow.
Could you add yourself as code owner of simapp/v2 and server/v2 btw? You've did quite a few improvements there so it be great that you get always pinged for future PRs.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (7)
simapp/v2/simdv2/cmd/root_di.go (1)
19-29: Consider using a real logger for initial command setupUsing a nop logger during initial command setup might hide important initialization issues. Consider using a real logger with appropriate log level to ensure proper debugging capabilities.
-configWriter, err := initRootCmd(rootCommand, log.NewNopLogger(), commandDependencies[T]{}) +logger := log.NewLogger(os.Stderr) // or appropriate logger setup +configWriter, err := initRootCmd(rootCommand, logger, commandDependencies[T]{})x/genutil/v2/cli/export.go (1)
Line range hint
36-52: Enhance warning message clarityWhile the nil handling is well-implemented with efficient file streaming, the warning message could be more descriptive to help users understand the implications.
Consider this improvement:
- if _, err := fmt.Fprintln(cmd.ErrOrStderr(), "WARNING: App exporter not defined. Returning genesis file."); err != nil { + if _, err := fmt.Fprintln(cmd.ErrOrStderr(), "WARNING: App exporter not defined. Returning unmodified genesis file without state export."); err != nil {server/v2/commands.go (3)
20-28: Update function documentation to reflect new parametersThe function documentation should be updated to describe the new parameters (
loggerandglobalAppConfig) and return values (ConfigWriter).// AddCommands add the server commands to the root command -// It configures the config handling and the logger handling +// It configures the config handling and the logger handling. +// +// Parameters: +// - rootCmd: The root command to add server commands to +// - logger: Logger instance for command execution +// - globalAppConfig: Application-wide configuration map +// - globalServerConfig: Server-specific configuration +// - components: Server components to be initialized +// +// Returns a ConfigWriter interface and an error if no components are provided
64-68: Update function documentation for createStartCommandThe function documentation should be updated to reflect the new parameters.
-// createStartCommand creates the start command for the application. +// createStartCommand creates the start command for the application. +// +// Parameters: +// - server: The server instance to be started +// - config: Server configuration map +// - logger: Logger instance for command execution
173-177: Document the structure of appBuildingCommandsAdd documentation to explain the expected format of the command paths array.
-// appBuildingCommands are the commands which need a full application to be built +// appBuildingCommands defines the command paths that require a full application to be built. +// Each inner slice represents a command path from root to leaf, in that order. +// For example, ["genesis", "export"] represents the "genesis export" command.server/v2/command_builder.go (2)
27-31: Create tracking issue for TODO comment.The TODO comment references merging #22267 and mentions that a new issue needs to be created. Please create the referenced issue to track the removal of the
viprfield.Would you like me to help create a GitHub issue to track this task?
177-182: Add nil check for cmd.OutOrStdout().When creating the logger,
cmd.OutOrStdout()is called without a nil check. While it's unlikely to be nil in practice, it's safer to add a defensive check.Consider adding a nil check:
if f.loggerFactory != nil { + out := cmd.OutOrStdout() + if out == nil { + out = os.Stdout + } - f.logger, err = f.loggerFactory(f.vipr.AllSettings(), cmd.OutOrStdout()) + f.logger, err = f.loggerFactory(f.vipr.AllSettings(), out) if err != nil { return nil, nil, nil, err } }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (5)
- server/v2/command_builder.go (1 hunks)
- server/v2/commands.go (5 hunks)
- simapp/v2/app_di.go (4 hunks)
- simapp/v2/simdv2/cmd/root_di.go (1 hunks)
- x/genutil/v2/cli/export.go (3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
server/v2/command_builder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/commands.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/root_di.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/genutil/v2/cli/export.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (9)
x/genutil/v2/cli/export.go (2)
24-24: LGTM: Interface-based design improves modularityThe change from concrete type
v2.AppExporterto interface typeExportableAppenhances modularity and follows good design principles by depending on abstractions rather than concrete implementations.
61-65: Verify height validationWhile the height loading logic is well-structured, we should verify that negative heights (other than -1) are properly validated.
Let's check if there are any height validation patterns in the codebase:
server/v2/commands.go (1)
110-117:⚠️ Potential issueAdd type assertion check for CPU profile path
The current implementation could panic if
cpuProfileFileis not a string.func wrapCPUProfile(logger log.Logger, cfg server.ConfigMap, callbackFn func() error) error { - cpuProfileFile, ok := cfg[FlagCPUProfiling] - if !ok { + cpuProfileFile, exists := cfg[FlagCPUProfiling] + if !exists { // if cpu profiling is not enabled, just run the callback return callbackFn() } + + profilePath, ok := cpuProfileFile.(string) + if !ok { + return errors.New("cpu profile path must be a string") + } - f, err := os.Create(cpuProfileFile.(string)) + f, err := os.Create(profilePath)Likely invalid or redundant comment.
simapp/v2/app_di.go (6)
63-77: Well-structured configuration setup with proper modularity!The configuration setup properly separates concerns by introducing specific account type providers and base account options. This modular approach enhances maintainability and flexibility.
87-89: Improved function signature enhances flexibility!The new signature with
depinject.Configand variadic outputs aligns well with dependency injection patterns and supports eager config loading as intended in the PR objectives.
101-153: Excellent documentation of configuration options!The comprehensive comments provide clear guidance for customizing various aspects of the application, including authentication, staking, and minting modules. The included examples make it easier for developers to understand and implement custom configurations.
169-191: Robust error handling implementation!The error handling has been properly implemented with clear error propagation and meaningful error messages. This aligns well with Go best practices for error handling.
241-243: Clean and focused configuration provider!The
ProvideRootStoreConfigfunction follows the single responsibility principle and properly handles store configuration unmarshaling.
195-203:⚠️ Potential issueConsider removing or refactoring this test helper
Based on the previous discussion, since this function is only used in tests within the same package, consider either:
- Removing it and using
NewSimAppdirectly in tests, or- If keeping it, refactor to return an error instead of panicking
If keeping the function, consider this refactor:
-func NewSimAppWithInputs[T transaction.Tx]( - logger log.Logger, - viper *viper.Viper, -) *SimApp[T] { +func NewSimAppWithInputs[T transaction.Tx]( + logger log.Logger, + viper *viper.Viper, +) (*SimApp[T], error) { app, err := NewSimApp[T](depinject.Configs( depinject.Supply(logger, runtime.GlobalConfig(viper.AllSettings()))), ) if err != nil { - panic(err) + return nil, err } - return app + return app, nil }
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (2)
server/v2/command_builder.go (2)
1-16: Consider using underscore in package name.The package name
serverv2should beserver_v2to follow Go naming conventions for packages with version numbers. This improves readability and consistency.
27-32: Convert TODO comment into a tracked issue.The TODO comment indicates technical debt regarding the viper instance field. This should be tracked in the issue system for better visibility and follow-up.
Would you like me to create a GitHub issue to track this technical debt item? The issue would reference #22267 and #22388 for context.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- server/v2/command_builder.go (1 hunks)
- simapp/v2/simdv2/cmd/root_di.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
server/v2/command_builder.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)
simapp/v2/simdv2/cmd/root_di.go (2)
73-78:⚠️ Potential issueAdd validation for command dependencies
The
commandDependenciesstruct is populated without validation, which could lead to runtime panics if any required fields are nil.+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) +}Likely invalid or redundant comment.
49-52: 🛠️ Refactor suggestionSimplify dependency injection configuration
The nested
depinject.Configscalls are redundant and can be simplified.-depinjectConfig = depinject.Configs( - depinject.Supply(logger, runtime.GlobalConfig(configMap)), - depinject.Provide(ProvideClientContext), -) +depinjectConfig := depinject.Supply( + logger, + runtime.GlobalConfig(configMap), + depinject.Provide(ProvideClientContext), +)Likely invalid or redundant comment.
server/v2/command_builder.go (1)
57-83: Add path validation and security checks.The home directory path construction could benefit from additional security measures:
- Validate that the path doesn't contain directory traversal attempts
- Check for absolute path when joining with defaultHomeBasename
- Consider adding path length limits to prevent potential DoS
Let's check for similar security measures in other parts of the codebase:
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.
| 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.
| 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
left a comment
julienrbrt
left a comment
There was a problem hiding this comment.
lgtm! I just pinged you at two comments that were left unanswered
|
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") | |
| } |
12 participants
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.