feat(server/v2) Add prune cmd to serverv2 #20736
Conversation
WalkthroughWalkthroughThe primary change introduces new functionality to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant StoreComponent
participant PruningHandler
participant Database
User->>CLI: Execute PrunesCmd()
CLI->>StoreComponent: Initialize PrunesCmd
StoreComponent->>PruningHandler: Parse options and prepare for pruning
PruningHandler->>Database: Retrieve latest height
PruningHandler->>Database: Calculate pruning heights
PruningHandler->>Database: Perform pruning operations
Database-->>PruningHandler: Return pruning results
PruningHandler-->>StoreComponent: Return success/failure
StoreComponent-->>CLI: Command execution result
CLI-->>User: Display results
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
@hieuvubk your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
server/v2/store/config.go (1)
17-21: Consider adding comments to theConfigstruct fields to enhance code readability and maintainability.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
server/v2/store/go.sumis excluded by!**/*.sum
Files selected for processing (12)
- server/v2/cometbft/server.go (1 hunks)
- server/v2/commands.go (1 hunks)
- server/v2/server.go (2 hunks)
- server/v2/store/commands.go (1 hunks)
- server/v2/store/config.go (1 hunks)
- server/v2/store/flags.go (1 hunks)
- server/v2/store/go.mod (1 hunks)
- server/v2/store/server.go (1 hunks)
- simapp/v2/go.mod (2 hunks)
- simapp/v2/simdv2/cmd/commands.go (3 hunks)
- store/v2/root/store.go (1 hunks)
- store/v2/store.go (1 hunks)
Files skipped from review due to trivial changes (3)
- server/v2/store/flags.go
- server/v2/store/go.mod
- simapp/v2/go.mod
Additional context used
Path-based instructions (9)
server/v2/store/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/store/server.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.store/v2/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/store/commands.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/server.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.server/v2/cometbft/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/root/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (12)
server/v2/store/config.go (1)
7-13: Ensure default configuration values align with system requirements.Verification successful
The default values for
AppDBBackend,PruningKeepRecent, andPruningIntervalin theDefaultConfigfunction are consistent with their usage across the codebase. The default values are set to empty or zero, which aligns with the system requirements.
AppDBBackendis set to an empty string, which is consistent.PruningKeepRecentis set to 0, which is consistent.PruningIntervalis set to 0, which is consistent.No discrepancies were found in the default configuration values.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the default configuration values align with system requirements. # Test: Check if the default values are consistent across similar configurations. rg --type go "PruningOptionDefault"Length of output: 1243
Script:
#!/bin/bash # Description: Verify the default configuration values align with system requirements. # Test: Check if the default values for `AppDBBackend`, `PruningKeepRecent`, and `PruningInterval` are consistent across similar configurations. rg --type go "AppDBBackend" rg --type go "PruningKeepRecent" rg --type go "PruningInterval"Length of output: 10845
server/v2/store/server.go (4)
14-16: TheStoreComponentstruct is well-defined. Ensure theconfigfield is properly managed throughout the component's lifecycle.
18-20: TheNewmethod correctly initializes aStoreComponent. Consider adding error handling if future configurations require it.
46-52: TheConfigmethod effectively handles null configurations by returning a default configuration. This is a good fallback mechanism.
22-24: TheInitmethod currently always succeeds. Verify that this is the intended behavior, especially in error scenarios.server/v2/commands.go (1)
73-73: The modification to usenewAppinCLICommandsaligns with the changes in the server interfaces. Ensure all dependent components are updated accordingly.Verification successful
The verification confirms that all instances of
CLICommandshave been updated to align with the new signature usingnewApp.
server/v2/server.goat line 121server/v2/store/server.goat line 38server/v2/cometbft/server.goat line 218All dependent components are correctly updated.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all dependent components are updated to use the new `CLICommands` signature. # Test: Search for outdated uses of `CLICommands`. rg --type go "CLICommands" --context 5Length of output: 3250
Script:
#!/bin/bash # Description: Verify that all instances of `CLICommands` are updated to use the new `CLICommands` signature with `newApp`. # Verify `CLICommands` implementation in `server/v2/server.go` ast-grep --lang go --pattern $'func (s *Server) CLICommands(appCreator AppCreator[transaction.Tx]) CLIConfig { $$$ }' server/v2/server.go # Verify `CLICommands` implementation in `server/v2/store/server.go` ast-grep --lang go --pattern $'func (s StoreComponent) CLICommands(appCreator serverv2.AppCreator[transaction.Tx]) serverv2.CLIConfig { $$$ }' server/v2/store/server.go # Verify `CLICommands` implementation in `server/v2/cometbft/server.go` ast-grep --lang go --pattern $'func (s *CometBFTServer[T]) CLICommands(_ serverv2.AppCreator[T]) serverv2.CLIConfig { $$$ }' server/v2/cometbft/server.goLength of output: 2552
store/v2/store.go (1)
65-65: The addition of thePrunemethod to theRootStoreinterface is crucial for implementing pruning functionality. Ensure it is correctly implemented across all concrete store implementations.server/v2/store/commands.go (2)
21-101: The implementation of thePrunesCmdfunction looks solid. It correctly sets up a new Cobra command, handles command-line arguments, and performs the pruning operation. However, consider adding more detailed error messages to improve troubleshooting and user experience.
103-125: ThegetPruningOptionsFromFlagsfunction is well-implemented with a clear handling of different pruning strategies. However, ensure that all possible error paths are adequately tested to prevent runtime issues.simapp/v2/simdv2/cmd/commands.go (1)
88-88: The addition of thestore.New()to the root command initialization is correctly implemented. Ensure that all dependencies and configurations required by the store component are properly set up before this point.server/v2/cometbft/server.go (1)
218-218: TheCLICommandsmethod is correctly implemented to include various necessary commands. Consider adding documentation or examples on how to use these commands for better user guidance.store/v2/root/store.go (1)
434-436: The implementation of thePrunemethod is straightforward and correctly delegates to thepruningManager. However, ensure thatpruningManageris always properly initialized before this method is called to prevent nil pointer dereferences.
| // v.Set("store.options.sc-pruning-option.keep-recent", keepRecent) // entry that read from app.toml | ||
| // v.Set("store.options.ss-pruning-option.keep-recent", keepRecent) | ||
|
|
||
| err = overrideKeepRecent(filepath.Join(rootDir, "config"), keepRecent) |
There was a problem hiding this comment.
Have to do a hacky way cause spf13/viper#1106.
If cmd has keep-recent flag, overwrite app.toml then read it again
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
|
|
||
| func overrideKeepRecent(configPath string, keepRecent uint64) error { | ||
| bz, err := os.ReadFile(filepath.Join(configPath, "app.toml")) |
Check failure
Code scanning / gosec
Potential file inclusion via variable Error
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
store/v2/options.go (1)
Line range hint
1-1: Use constants for error messages.The error messages are clear and provide actionable feedback, which is excellent for debugging and user experience. However, consider using constants for the minimum values in the error messages to maintain consistency and ease future modifications.
+ const minPruningInterval = 10 + const minKeepRecent = 2 - ErrPruningIntervalTooSmall = fmt.Errorf("'pruning-interval' must not be less than %d. For the most aggressive pruning, select pruning = \"everything\"", pruneEverythingInterval) + ErrPruningIntervalTooSmall = fmt.Errorf("'pruning-interval' must not be less than %d. For the most aggressive pruning, select pruning = \"everything\"", minPruningInterval) - ErrPruningKeepRecentTooSmall = fmt.Errorf("'pruning-keep-recent' must not be less than %d. For the most aggressive pruning, select pruning = \"everything\"", pruneEverythingKeepRecent) + ErrPruningKeepRecentTooSmall = fmt.Errorf("'pruning-keep-recent' must not be less than %d. For the most aggressive pruning, select pruning = \"everything\"", minKeepRecent)
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (3)
runtime/v2/go.sumis excluded by!**/*.sumserver/v2/go.sumis excluded by!**/*.sumstore/v2/go.sumis excluded by!**/*.sum
Files selected for processing (15)
- runtime/v2/builder.go (4 hunks)
- runtime/v2/go.mod (5 hunks)
- runtime/v2/module.go (4 hunks)
- runtime/v2/types.go (1 hunks)
- server/v2/go.mod (4 hunks)
- server/v2/store/commands.go (1 hunks)
- server/v2/store/config.go (1 hunks)
- server/v2/store/flags.go (1 hunks)
- server/v2/store/server.go (1 hunks)
- simapp/v2/app_di.go (5 hunks)
- simapp/v2/go.mod (1 hunks)
- simapp/v2/simdv2/cmd/commands.go (2 hunks)
- store/v2/go.mod (3 hunks)
- store/v2/options.go (1 hunks)
- store/v2/root/factory.go (5 hunks)
Files skipped from review due to trivial changes (1)
- runtime/v2/types.go
Files skipped from review as they are similar to previous changes (8)
- runtime/v2/go.mod
- server/v2/go.mod
- server/v2/store/config.go
- server/v2/store/flags.go
- server/v2/store/server.go
- simapp/v2/simdv2/cmd/commands.go
- store/v2/go.mod
- store/v2/root/factory.go
Additional context used
Path-based instructions (5)
store/v2/options.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/store/commands.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/builder.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.runtime/v2/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
GitHub Check: gosec
server/v2/store/commands.go
[failure] 130-130: Potential file inclusion via variable
Potential file inclusion via variable
Additional comments not posted (20)
store/v2/options.go (3)
Line range hint
1-1: LGTM!The addition of constants for various pruning strategies and associated parameters is well-defined and improves code readability and maintainability.
Line range hint
1-1: LGTM!The new functions
NewPruningOptionFromStringandValidateare well-designed and improve the flexibility and validation of pruning options.
Line range hint
1-1: LGTM!The modifications to
NewPruningOptionand the addition ofNewPruningOptionWithCustomenhance the functionality of pruning options.server/v2/store/commands.go (4)
19-83: LGTM!The
PrunesCmdfunction is well-structured and provides comprehensive CLI documentation for the pruning command. It correctly binds command flags and handles errors appropriately. The use ofcobra.RangeArgsensures that the command does not accept more than one argument, which is a good practice for CLI design.
86-127: LGTM! But monitor for a future fix in Viper.The
createRootStorefunction is well-designed and handles the creation of the root store and management of pruning options. However, it includes a hack to override thekeep-recentvalue in theapp.tomlfile due to a limitation in Viper. Consider monitoring for a future fix in Viper to remove this workaround.
3-17: LGTM!The import statements are necessary for the functionality of the file.
1-2: LGTM!The package declaration is correct and aligns with the file's purpose.
runtime/v2/builder.go (5)
29-29: LGTM!The addition of the
viperfield to theAppBuilderstruct enhances configuration management.
124-138: LGTM!The updates to the
Buildmethod improve the functionality and structure of the method by including logic for initializing a new database instance and creating store options.
Line range hint
3-19: LGTM!The import statements are necessary for the functionality of the file.
Line range hint
1-2: LGTM!The package declaration is correct and aligns with the file's purpose.
Line range hint
1-1: LGTM!The
AppBuilderOptionfunctions are well-designed and enhance the flexibility of theAppBuilder.simapp/v2/app_di.go (3)
Line range hint
9-9:
LGTM! Import added correctly.The import of
viperis necessary for the new configuration management approach.
Line range hint
154-154:
LGTM! Struct modified correctly.The addition of the
Viperfield in theAppInputsstruct is consistent with the new configuration management approach.
Line range hint
166-167:
LGTM! Function updated correctly.The conditional check and assignment of the
Viperinstance in theSetupAppBuilderfunction are necessary and correctly implemented.runtime/v2/module.go (3)
9-9: LGTM! Import added correctly.The import of
viperis necessary for the new configuration management approach.
154-154: LGTM! Struct modified correctly.The addition of the
Viperfield in theAppInputsstruct is consistent with the new configuration management approach.
166-167: LGTM! Function updated correctly.The conditional check and assignment of the
Viperinstance in theSetupAppBuilderfunction are necessary and correctly implemented.simapp/v2/go.mod (2)
13-13: LGTM! Version updated correctly.The update to a specific timestamped version for
cosmossdk.io/server/v2is necessary for stability and clarity.
15-15: LGTM! Dependency marked as indirect correctly.Marking
cosmossdk.io/store/v2as indirect is appropriate if it is no longer directly used but still required by other packages.
store/v2/options.go
Outdated
| KeepRecent uint64 `mapstructure:"keep-recent" toml:"keep-recent"` | ||
|
|
||
| // Interval sets the number of how often to prune. | ||
| // If set to 0, no pruning will be done. | ||
| Interval uint64 | ||
| Interval uint64 `mapstructure:"interval" toml:"interval"` |
There was a problem hiding this comment.
Refactor suggestion: Consistent struct field tagging.
The struct tags for KeepRecent and Interval fields have been added for mapstructure and toml. This is good for serialization and deserialization consistency. However, consider adding JSON tags as well to maintain a uniform approach across different serialization formats.
- KeepRecent uint64 `mapstructure:"keep-recent" toml:"keep-recent"`
- Interval uint64 `mapstructure:"interval" toml:"interval"`
+ KeepRecent uint64 `json:"keepRecent" mapstructure:"keep-recent" toml:"keep-recent"`
+ Interval uint64 `json:"interval" mapstructure:"interval" toml:"interval"`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.
| KeepRecent uint64 `mapstructure:"keep-recent" toml:"keep-recent"` | |
| // Interval sets the number of how often to prune. | |
| // If set to 0, no pruning will be done. | |
| Interval uint64 | |
| Interval uint64 `mapstructure:"interval" toml:"interval"` | |
| KeepRecent uint64 `json:"keepRecent" mapstructure:"keep-recent" toml:"keep-recent"` | |
| // Interval sets the number of how often to prune. | |
| // If set to 0, no pruning will be done. | |
| Interval uint64 `json:"interval" mapstructure:"interval" toml:"interval"` |
| func overrideKeepRecent(configPath string, keepRecent uint64) error { | ||
| bz, err := os.ReadFile(filepath.Join(configPath, "app.toml")) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| lines := strings.Split(string(bz), "\n") | ||
|
|
||
| for i, line := range lines { | ||
| if strings.Contains(line, "keep-recent") { | ||
| lines[i] = fmt.Sprintf("keep-recent = %d", keepRecent) | ||
| } | ||
| } | ||
| output := strings.Join(lines, "\n") | ||
|
|
||
| return os.WriteFile(filepath.Join(configPath, "app.toml"), []byte(output), 0o600) | ||
| } |
There was a problem hiding this comment.
Potential file inclusion vulnerability.
The overrideKeepRecent function modifies the app.toml file to set the keep-recent value. However, it includes a potential file inclusion vulnerability. Ensure that the file path is properly sanitized and validated before reading or writing to the file.
- bz, err := os.ReadFile(filepath.Join(configPath, "app.toml"))
+ configFilePath := filepath.Join(configPath, "app.toml")
+ if !filepath.IsAbs(configFilePath) || !strings.HasPrefix(configFilePath, configPath) {
+ return fmt.Errorf("invalid file path: %s", configFilePath)
+ }
+ bz, err := os.ReadFile(configFilePath)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 overrideKeepRecent(configPath string, keepRecent uint64) error { | |
| bz, err := os.ReadFile(filepath.Join(configPath, "app.toml")) | |
| if err != nil { | |
| return err | |
| } | |
| lines := strings.Split(string(bz), "\n") | |
| for i, line := range lines { | |
| if strings.Contains(line, "keep-recent") { | |
| lines[i] = fmt.Sprintf("keep-recent = %d", keepRecent) | |
| } | |
| } | |
| output := strings.Join(lines, "\n") | |
| return os.WriteFile(filepath.Join(configPath, "app.toml"), []byte(output), 0o600) | |
| } | |
| func overrideKeepRecent(configPath string, keepRecent uint64) error { | |
| configFilePath := filepath.Join(configPath, "app.toml") | |
| if !filepath.IsAbs(configFilePath) || !strings.HasPrefix(configFilePath, configPath) { | |
| return fmt.Errorf("invalid file path: %s", configFilePath) | |
| } | |
| bz, err := os.ReadFile(configFilePath) | |
| if err != nil { | |
| return err | |
| } | |
| lines := strings.Split(string(bz), "\n") | |
| for i, line := range lines { | |
| if strings.Contains(line, "keep-recent") { | |
| lines[i] = fmt.Sprintf("keep-recent = %d", keepRecent) | |
| } | |
| } | |
| output := strings.Join(lines, "\n") | |
| return os.WriteFile(filepath.Join(configPath, "app.toml"), []byte(output), 0o600) | |
| } |
Tools
GitHub Check: gosec
[failure] 130-130: Potential file inclusion via variable
Potential file inclusion via variable
runtime/v2/builder.go
Outdated
| @@ -116,6 +121,22 @@ func (a *AppBuilder[T]) Build(opts ...AppBuilderOption[T]) (*App[T], error) { | |||
|
|
|||
| a.app.stf = stf | |||
|
|
|||
| v := a.viper | |||
| home := v.GetString(FlagHome) | |||
| scRawDb, err := db.NewGoLevelDB("application", filepath.Join(home, "data"), nil) | |||
There was a problem hiding this comment.
so store/v2 is goleveldb only @kocubinski?
There was a problem hiding this comment.
we can handle multiple dbs by db-backend in app.toml
There was a problem hiding this comment.
That's for v1, but maybe v2 doesn't support multiple backend? Wanted to confirm, otherwise this being hardcoded in runtime isn't great (but easily solvable with an appbuilder options). Let's wait for an answer
There was a problem hiding this comment.
store v2 has support for pebbledb as well
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
store/v2/go.sumis excluded by!**/*.sum
Files selected for processing (7)
- runtime/v2/builder.go (4 hunks)
- server/v2/store/commands.go (1 hunks)
- server/v2/store/config.go (1 hunks)
- store/v2/commitment/iavl/config.go (1 hunks)
- store/v2/go.mod (2 hunks)
- store/v2/options.go (1 hunks)
- store/v2/root/factory.go (4 hunks)
Files skipped from review as they are similar to previous changes (6)
- server/v2/store/commands.go
- server/v2/store/config.go
- store/v2/commitment/iavl/config.go
- store/v2/go.mod
- store/v2/options.go
- store/v2/root/factory.go
Additional context used
Path-based instructions (1)
runtime/v2/builder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (8)
runtime/v2/builder.go (8)
10-10: Approved: Addition ofviperimport.The addition of the
viperpackage is appropriate for enhancing configuration management.
29-29: Approved: Addition ofviperfield toAppBuilderstruct.The addition of the
viperfield enhances the configuration management capabilities of theAppBuilder.
124-126: Approved: Integration ofviperin theBuildmethod.The integration of
viperfor retrieving the home directory and other configurations is appropriate.
127-132: Approved: Reading store options usingviper.The logic for reading store options from the configuration using
viperis correct.
134-137: Approved: Initialization of new database instance.The initialization of a new database instance using
db.NewDBis correct.
139-146: Approved: Creation ofstoreOptionsvariable.The creation of the
storeOptionsvariable encapsulates the necessary parameters for root store creation effectively.
147-149: Approved: Call torootstore.CreateRootStore.The call to
rootstore.CreateRootStorewith thestoreOptionsis correct.
Line range hint
151-192:
Approved: Initialization ofappManagerBuilder.The initialization of the
appManagerBuilderwith the necessary parameters is correct.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
runtime/v2/go.sumis excluded by!**/*.sum
Files selected for processing (5)
- runtime/v2/builder.go (4 hunks)
- runtime/v2/go.mod (5 hunks)
- server/v2/go.mod (4 hunks)
- simapp/v2/go.mod (1 hunks)
- simapp/v2/simdv2/cmd/commands.go (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- runtime/v2/go.mod
- server/v2/go.mod
- simapp/v2/go.mod
- simapp/v2/simdv2/cmd/commands.go
Additional context used
Path-based instructions (1)
runtime/v2/builder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (9)
runtime/v2/builder.go (9)
10-10: Import statement for Viper addedThe import statement for
github.com/spf13/viperis necessary for the configuration management added in this file.
29-29: New fieldviperadded toAppBuilderstructThe new field
viperof type*viper.Viperis added to theAppBuilderstruct for configuration management.
127-129: Retrieve home directory from Viper configurationThe home directory is retrieved from the Viper configuration using
v.GetString(FlagHome). Ensure thatFlagHomeis correctly set in the configuration.
130-135: Store options initializationThe store options are initialized from the Viper configuration. Ensure that the
store.optionssection exists in the configuration file and is correctly formatted.
137-141: Database initializationThe database is initialized using
db.NewDB. Ensure that thestore.app-db-backendconfiguration is correctly set and supports the specified database type.
142-149: Store options encapsulationThe store options are encapsulated within the
rootstore.FactoryOptionsstruct. This enhances the clarity and structure of the code.
150-153: Create root storeThe
CreateRootStoremethod is called with the encapsulated store options. Ensure that the method handles errors appropriately.
Line range hint
155-180:
App manager builder initializationThe
appManagerBuilderis initialized with the necessary parameters for building the app manager. Ensure that the initialization is correct and handles errors appropriately.
Line range hint
182-186:
Build app managerThe
Buildmethod is called forappManagerand the result is assigned toa.app.AppManager. Ensure that the method handles errors appropriately.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (6)
- runtime/v2/builder.go (4 hunks)
- server/v2/store/commands.go (1 hunks)
- server/v2/store/config.go (1 hunks)
- store/v2/commitment/iavl/config.go (1 hunks)
- store/v2/options.go (1 hunks)
- store/v2/root/factory.go (4 hunks)
Files skipped from review as they are similar to previous changes (4)
- server/v2/store/commands.go
- server/v2/store/config.go
- store/v2/commitment/iavl/config.go
- store/v2/options.go
Additional context used
Path-based instructions (2)
store/v2/root/factory.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/builder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (1)
store/v2/root/factory.go (1)
35-42: LGTM!The
Optionsstruct is well-defined with appropriate tags and comments.
| func DefaultStoreOptions() Options { | ||
| return Options{ | ||
| SSType: 0, | ||
| SCType: 0, | ||
| SCPruningOption: &store.PruningOption{ | ||
| KeepRecent: 2, | ||
| Interval: 1, | ||
| }, | ||
| SSPruningOption: &store.PruningOption{ | ||
| KeepRecent: 2, | ||
| Interval: 1, | ||
| }, | ||
| IavlConfig: &iavl.Config{ | ||
| CacheSize: 100_000, | ||
| SkipFastStorageUpgrade: true, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Consider moving the ensureDir function outside.
To enhance readability, consider moving the ensureDir function outside the CreateRootStore function.
func ensureDir(dir string) error {
if err := os.MkdirAll(dir, 0o0755); err != nil {
return fmt.Errorf("failed to create directory %s: %w", dir, err)
}
return nil
}
func CreateRootStore(opts *FactoryOptions) (store.RootStore, error) {
var (
ssDb storage.Database
ss *storage.StorageStore
sc *commitment.CommitStore
err error
)
storeOpts := opts.Options
// ... rest of the code
}| v := a.viper | ||
| home := v.GetString(FlagHome) | ||
|
|
||
| storeOpts := rootstore.DefaultStoreOptions() | ||
| if err := v.Sub("store.options").Unmarshal(&storeOpts); err != nil { | ||
| return nil, fmt.Errorf("failed to store options: %w", err) | ||
| } | ||
|
|
||
| scRawDb, err := db.NewDB(db.DBType(v.GetString("store.app-db-backend")), "application", filepath.Join(home, "data"), nil) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| storeOptions := &rootstore.FactoryOptions{ | ||
| Logger: a.app.logger, | ||
| RootDir: home, | ||
| Options: storeOpts, | ||
| StoreKeys: append(a.app.storeKeys, "stf"), | ||
| SCRawDB: scRawDb, | ||
| } | ||
| a.storeOptions = storeOptions | ||
|
|
There was a problem hiding this comment.
Improve error handling for v.Sub("store.options").Unmarshal(&storeOpts).
To provide more context, improve the error handling for v.Sub("store.options").Unmarshal(&storeOpts).
- if err := v.Sub("store.options").Unmarshal(&storeOpts); err != nil {
- return nil, fmt.Errorf("failed to store options: %w", err)
+ sub := v.Sub("store.options")
+ if sub == nil {
+ return nil, fmt.Errorf("failed to get sub configuration for store.options")
+ }
+ if err := sub.Unmarshal(&storeOpts); err != nil {
+ return nil, fmt.Errorf("failed to unmarshal store 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.
| v := a.viper | |
| home := v.GetString(FlagHome) | |
| storeOpts := rootstore.DefaultStoreOptions() | |
| if err := v.Sub("store.options").Unmarshal(&storeOpts); err != nil { | |
| return nil, fmt.Errorf("failed to store options: %w", err) | |
| } | |
| scRawDb, err := db.NewDB(db.DBType(v.GetString("store.app-db-backend")), "application", filepath.Join(home, "data"), nil) | |
| if err != nil { | |
| panic(err) | |
| } | |
| storeOptions := &rootstore.FactoryOptions{ | |
| Logger: a.app.logger, | |
| RootDir: home, | |
| Options: storeOpts, | |
| StoreKeys: append(a.app.storeKeys, "stf"), | |
| SCRawDB: scRawDb, | |
| } | |
| a.storeOptions = storeOptions | |
| v := a.viper | |
| home := v.GetString(FlagHome) | |
| storeOpts := rootstore.DefaultStoreOptions() | |
| sub := v.Sub("store.options") | |
| if sub == nil { | |
| return nil, fmt.Errorf("failed to get sub configuration for store.options") | |
| } | |
| if err := sub.Unmarshal(&storeOpts); err != nil { | |
| return nil, fmt.Errorf("failed to unmarshal store options: %w", err) | |
| } | |
| scRawDb, err := db.NewDB(db.DBType(v.GetString("store.app-db-backend")), "application", filepath.Join(home, "data"), nil) | |
| if err != nil { | |
| panic(err) | |
| } | |
| storeOptions := &rootstore.FactoryOptions{ | |
| Logger: a.app.logger, | |
| RootDir: home, | |
| Options: storeOpts, | |
| StoreKeys: append(a.app.storeKeys, "stf"), | |
| SCRawDB: scRawDb, | |
| } | |
| a.storeOptions = storeOptions |
Description
Closes: #20511, #19674
AppI,AppCreator,CLIConfigfromserver/v2tocore/serverstore/v2Prunemethod instore/v2.StoreinterfaceAuthor 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
Summary by CodeRabbit
-
- Introduced a command to prune app history states, allowing users to manage storage more effectively.
- Added a default configuration setup for easier application management.
-
- Enhanced pruning options with new strategies and validation rules for better configuration management.
- Streamlined configuration management by consolidating related settings into a single structure.
-
- Updated multiple dependencies to enhance configuration management and overall functionality of the application.
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -New Features
Improvements
Dependencies