refactor(runtime/v2): rm singleton storebuilder scope #22204
Conversation
kocubinski
requested review from
julienrbrt,
a team and
hieuvubk
as code owners
📝 WalkthroughWalkthroughThe pull request introduces a significant refactoring of the store management logic in the application. Key changes include the removal of the singleton store builder pattern, updates to the Changes
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: 1
🧹 Outside diff range and nitpick comments (3)
simapp/v2/app_test.go (2)
43-43: Approve change, but suggest additional test coverageThe modification to use
root.NewBuilder()in theNewSimAppcall aligns with the PR's goal of removing the singleton storebuilder pattern. This change enhances flexibility in store management for tests.However, to ensure robustness:
Consider adding a specific test case that verifies the behavior of the new storebuilder implementation. This would increase confidence in the refactoring and improve overall test coverage.
Line range hint
1-165: Enhance test coverage for refactored storebuilder behaviorWhile the existing tests cover basic app initialization and state export, they don't explicitly verify the new non-singleton storebuilder behavior introduced by this refactoring.
To improve test coverage and ensure the refactoring achieves its goals:
- Add a test case that creates multiple
SimAppinstances and verifies they have independent stores.- Include a test that checks the behavior of the new
root.NewBuilder()implementation, ensuring it properly initializes and manages the store.These additions would provide more comprehensive coverage of the changes introduced in this PR.
simapp/v2/app_di.go (1)
94-94: LGTM:storeBuildercorrectly supplied to dependency injection.The addition of
storeBuilderto thedepinject.Supplycall within theappConfigdeclaration ensures that the new parameter is properly supplied to the dependency injection configuration. This change is consistent with the removal of the singleton storebuilder scope and allows for more flexible store management.Consider adding a blank line after the
storeBuilderentry for better readability:storeBuilder, + // ADVANCED CONFIGURATION
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (5)
- runtime/v2/store.go (0 hunks)
- simapp/v2/app_di.go (1 hunks)
- simapp/v2/app_test.go (2 hunks)
- simapp/v2/simdv2/cmd/commands.go (2 hunks)
- simapp/v2/simdv2/cmd/root_di.go (1 hunks)
💤 Files with no reviewable changes (1)
- runtime/v2/store.go
🧰 Additional context used
📓 Path-based instructions (4)
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/app_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"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 (6)
simapp/v2/simdv2/cmd/root_di.go (2)
35-35: Approved: Explicit initialization ofstoreBuilderThe change from a zero-value initialization to an explicit initialization with
root.NewBuilder()is a positive improvement. This aligns with the PR objective of removing the singleton pattern and enhances code clarity. It also adheres to the Uber Golang style guide by avoiding global state.
42-42: Approved: SupplystoreBuilderto dependency injectionThe modification to include
storeBuilderin thedepinject.Supplycall is consistent with its initialization. This change ensures that thestoreBuilderis properly supplied to the dependency injection system, which is crucial for the refactoring away from the singleton pattern. The code structure remains clean and adheres to the Golang style guidelines.simapp/v2/app_test.go (1)
24-24: LGTM: Import changes align with refactoring goalsThe addition of
"cosmossdk.io/store/v2/root"and removal of"cosmossdk.io/runtime/v2"align with the PR's objective of refactoring the storebuilder scope. The import statement follows the Uber Go Style Guide for grouping and ordering.simapp/v2/app_di.go (1)
84-86: LGTM: Variable declarations adjusted appropriately.The modifications to the variable declarations for
app,appBuilder, anderrare appropriate adjustments to accommodate the newstoreBuilderparameter. These changes maintain the overall structure and purpose of the function.simapp/v2/simdv2/cmd/commands.go (2)
69-72: Introduction ofappCreatorFunction Enhances ModularityThe new
appCreatorfunction encapsulates the app creation logic, improving code modularity and readability. Passing thestoreBuilderensures consistent store configuration throughout the application.
77-77: Consistent Application Initialization in Command SetupUsing
appCreatorinserverv2.AddCommandsensures consistent application initialization across commands, enhancing maintainability and reducing potential initialization discrepancies.
| @@ -79,19 +78,20 @@ func AppConfig() depinject.Config { | |||
| func NewSimApp[T transaction.Tx]( | |||
| logger log.Logger, | |||
| viper *viper.Viper, | |||
| storeBuilder root.Builder, | |||
There was a problem hiding this comment.
💡 Codebase verification
Action Required: NewSimApp calls missing storeBuilder parameter.
Several instances of NewSimApp function calls are missing the new storeBuilder parameter, which may lead to inconsistent store configurations across the application. Please update the following files to include the storeBuilder parameter:
tests/integration/store/rootmulti/rollback_test.gotests/e2e/genutil/export_test.gosimapp/sim_test.gosimapp/test_helpers.gosimapp/simd/cmd/commands.gosimapp/app_di.gosimapp/app.go
🔗 Analysis chain
LGTM: Addition of storeBuilder parameter enhances flexibility.
The addition of the storeBuilder parameter to the NewSimApp function signature aligns with the PR objective of removing the singleton storebuilder scope. This change allows for more flexibility in how the store is built and passed to the application.
To ensure this change is consistently applied, please run the following script to check for any occurrences of NewSimApp that might need updating:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all occurrences of NewSimApp function calls in the codebase
# Expect: All occurrences should have the new storeBuilder parameter
rg --type go -A 5 'NewSimApp\s*\('
Length of output: 7126
julienrbrt
added
the
backport/v0.52.x
|
Adding backport label as it touches simapp |
There was a problem hiding this comment.
Here storeBuilder is injected and supplied. We should remove the inject one.
|
I think we can close this one, as #22205 got 3 approvals in this one none. |
Description
A singleton was not a great idea.
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
Refactor