refactor(runtime/v2): use AppI.GetStore() to fetch an initialized RootStore#22205
Conversation
kocubinski
requested review from
facundomedica,
hieuvubk and
sontrinh16
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve a significant refactoring of store management and command initialization processes within the application. Key modifications include the removal of the singleton store builder pattern, updates to directly access the store from the application interface, streamlined command initialization functions, enhancements to the Changes
Possibly related PRs
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
|
julienrbrt
left a comment
julienrbrt
left a comment
There was a problem hiding this comment.
This one looks great 🙏
ghost
assigned
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
server/v2/types.go (1)
21-21: LGTM: New method added to AppI interface.The new
GetStore()method is a good addition to the AppI interface, providing access to the root store. This aligns well with the PR objectives.Consider adding a comment to document the purpose of this method, for example:
// GetStore returns the initialized RootStore for the application. GetStore() store.RootStoresimapp/v2/app_di.go (2)
Line range hint
86-156: LGTM: Improved documentation for customization optionsThe added comments provide clear guidance on how to customize various aspects of the application, such as account types and address codecs. This improvement in documentation will be beneficial for developers working with the codebase.
Consider adding a brief comment at the beginning of this section to indicate that these are advanced configuration options. For example:
// ADVANCED CONFIGURATION // // The following section provides options for customizing various aspects of the application. // Most users won't need to modify these settings.This would help set expectations for users reading through the configuration options.
Line range hint
170-173: LGTM: Improved store initialization with error handlingThe use of
storeBuilder.Buildfor store initialization and the addition of error handling are good improvements. This change enhances the robustness of the application initialization process.Consider using a more descriptive error message when panicking. For example:
if err != nil { panic(fmt.Errorf("failed to build store: %w", err)) }This would provide more context in case of a panic, making it easier to diagnose issues.
simapp/v2/simdv2/cmd/testnet.go (2)
Line range hint
90-101: LGTM! Consider updating the function documentation.The removal of the
sb root.Builderparameter simplifies the function signature and is consistent with the overall changes. The core functionality remains intact.Consider updating the function's documentation to reflect the removal of the
sbparameter:- // NewTestnetCmd creates a root testnet command with subcommands to run an in-process testnet or initialize - // validator configuration files for running a multi-validator testnet in a separate process + // NewTestnetCmd creates a root testnet command with subcommands to initialize + // validator configuration files for running a multi-validator testnet in a separate process
Line range hint
1-524: Overall refactoring looks good. Consistent removal ofroot.Builderdependency.The changes in this file are part of a larger refactoring effort to remove the dependency on
root.Builder. The modifications are consistent across all affected functions, and the core logic remains intact. This refactoring simplifies the code and potentially reduces coupling.Consider updating any related documentation or README files to reflect these changes, especially if they mention the usage of
root.Builderin the context of testnet initialization.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (10)
- runtime/v2/module.go (2 hunks)
- runtime/v2/store.go (0 hunks)
- server/v2/cometbft/server.go (1 hunks)
- server/v2/store/server.go (2 hunks)
- server/v2/types.go (2 hunks)
- simapp/v2/app_di.go (1 hunks)
- simapp/v2/app_test.go (0 hunks)
- simapp/v2/simdv2/cmd/commands.go (2 hunks)
- simapp/v2/simdv2/cmd/root_di.go (1 hunks)
- simapp/v2/simdv2/cmd/testnet.go (4 hunks)
💤 Files with no reviewable changes (2)
- runtime/v2/store.go
- simapp/v2/app_test.go
🧰 Additional context used
📓 Path-based instructions (8)
runtime/v2/module.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/store/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/types.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/root_di.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/simdv2/cmd/testnet.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (15)
server/v2/types.go (1)
11-11: LGTM: New import added correctly.The new import for the store package is correctly formatted and necessary for the interface changes.
simapp/v2/simdv2/cmd/root_di.go (1)
73-73: LGTM! VerifyinitRootCmdimplementation.The removal of the
storeBuilderparameter from theinitRootCmdfunction call aligns with the PR objectives of refactoring to useAppI.GetStore(). This change simplifies the function call and potentially improves code maintainability.To ensure consistency, please verify that the
initRootCmdfunction implementation has been updated to match this new signature. Run the following script to check the function definition:✅ Verification successful
initRootCmdsignature verified and matches the changes.The
initRootCmdfunction insimapp/v2/simdv2/cmd/commands.gohas been updated to include thetxConfigparameter, aligning with the changes inroot_di.go. This ensures consistency and maintains the integrity of the command initialization.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initRootCmd function signature # Test: Search for the initRootCmd function definition rg --type go -A 5 $'func initRootCmd'Length of output: 671
simapp/v2/simdv2/cmd/commands.go (3)
Line range hint
1-231: Overall changes and testing recommendationThe changes in this file are consistent with the PR objective of refactoring to use
AppI.GetStore(). ThestoreBuilderparameter has been removed from multiple function calls, and the import for therootpackage has been removed. These changes suggest a significant shift in how the store is initialized and accessed throughout the application.While the changes look good, I recommend:
- Thoroughly testing the affected functionality, especially the command initialization and execution processes.
- Updating any documentation that might reference the old
storeBuilderpattern.- Verifying that all parts of the codebase that previously relied on the
storeBuilderhave been appropriately updated.#!/bin/bash # Description: Check for any remaining uses of storeBuilder in the codebase # Expected: No occurrences of storeBuilder in function signatures or calls rg --type go 'storeBuilder'
84-85: Verify changes in serverstore.New functionThe
storeBuilderparameter has been removed from theserverstore.New[T]()function call. This change is consistent with the PR objective of removing the singleton store builder pattern. However, we should verify that:
- The
serverstore.Newfunction has been updated to handle this change.- The store initialization is now handled correctly within the function or elsewhere in the application.
#!/bin/bash # Description: Check the implementation of serverstore.New function # Expected: The function should no longer accept a storeBuilder parameter rg --type go -A 10 'func New\[T'
55-55: Verify changes in NewTestnetCmd functionThe
storeBuilderparameter has been removed from theNewTestnetCmdfunction call. This change aligns with the PR objective of refactoring to useAppI.GetStore(). However, we should verify that theNewTestnetCmdfunction has been updated accordingly to handle this change.✅ Verification successful
NewTestnetCmd function correctly updated
The
NewTestnetCmdfunction signatures in bothsimapp/v2/simdv2/cmd/testnet.goandsimapp/simd/cmd/testnet.gono longer include thestoreBuilderparameter, aligning with the removal in the function call.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the implementation of NewTestnetCmd function # Expected: The function should no longer use storeBuilder rg --type go -A 10 'func NewTestnetCmd'Length of output: 3464
Script:
#!/bin/bash # Description: Check the implementation of NewTestnetCmd function using ast-grep # Expected: The function should no longer use storeBuilder ast-grep --lang go --pattern 'func NewTestnetCmd' .Length of output: 398
Script:
#!/bin/bash # Description: Search for functions starting with NewTestnet in the Go codebase # Expected: Identify if the function has been renamed or moved rg --type go 'func NewTestnet\w*' .Length of output: 231
simapp/v2/app_di.go (1)
86-86: LGTM: Explicit declaration ofstoreBuilderThe explicit declaration of
storeBuilderimproves code clarity and aligns with the Uber Golang style guide. This change makes the variable's scope and purpose more evident.simapp/v2/simdv2/cmd/testnet.go (2)
Line range hint
105-146: LGTM! Changes are consistent and maintain functionality.The removal of the
sb root.Builderparameter from the function signature and theinitTestnetFilescall is consistent with the overall refactoring. The function's core logic and the command's description remain intact and accurate.
Line range hint
345-351: LGTM! Verify compatibility with updatedstore.New[T]()function.The removal of the
sb root.Builderparameter and the update tostore.New[T]()are consistent with the overall refactoring. The function's core logic remains intact.Ensure that the
store.New[T]()function is compatible with this change by checking its implementation:✅ Verification successful
Compatibility Verified! The
store.New[T]()function no longer requires any parameters, aligning with the recent changes. No issues found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the signature of store.New[T]() function # Expected: The function should not require any parameters rg --type go 'func New\[T' -A 3Length of output: 1401
server/v2/store/server.go (3)
30-31: SimplifiedNewfunction by removing thebuilderparameterThe
Newfunction has been refactored to remove the unnecessarybuilderparameter, which simplifies the server initialization and aligns with the refactoring goals.
34-35: UpdatedInitmethod to useapp.GetStore()The
Initmethod now initializess.backenddirectly usingapp.GetStore(). The parameters that are not used are appropriately named with underscores, following Go conventions.
83-83: Improved error message clarity inUnmarshalConfigThe error message now specifies "failed to unmarshal store config," providing clearer context for debugging purposes.
runtime/v2/module.go (3)
232-232: Update Comment to Include New ServiceThe addition of
// - store/v2/root.Builderin the comment correctly reflects the inclusion of thestoreBuilderin the default service bindings.
248-248: InitializestoreBuilderfor Dependency InjectionInitializing
storeBuilderwithroot.NewBuilder()properly sets up the store builder for use in dependency injection.
255-255: SupplystoreBuilderin Dependency InjectionIncluding
storeBuilderin thedepinject.Supply()call ensures that the store builder is supplied to the dependency injection container, making it available where needed.server/v2/cometbft/server.go (1)
104-104: Ensure all references tos.storeBuilderare removedSince
storeBuilderhas been removed fromCometBFTServer, please verify that there are no remaining references tos.storeBuilderin the codebase to prevent compilation errors.Run the following script to confirm that
s.storeBuilderis no longer used:✅ Verification successful
Verification Successful: No references to
s.storeBuilderremain in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for 's.storeBuilder' in Go files to ensure it's no longer referenced. # Test: Search for occurrences of 's.storeBuilder'. Expect: No matches. rg 's\.storeBuilder' --type goLength of output: 32
Script:
#!/bin/bash # Description: Comprehensive search for 's.storeBuilder' in Go files to ensure it's fully removed. # Search recursively with case-insensitive matching and include hidden directories rg -i 's\.storeBuilder' --type go --hiddenLength of output: 105
| } | ||
|
|
||
| func (s *Server[T]) Init(app serverv2.AppI[T], _ map[string]any, _ log.Logger) error { | ||
| s.backend = app.GetStore() |
There was a problem hiding this comment.
init should still set the config.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
server/v2/server_test.go (1)
52-54: Add a comment explaining theGetStoremethod's behavior.While returning
nilis a valid mock implementation, it would be helpful to add a comment explaining why this method returnsniland any potential implications for test scenarios.Consider adding a comment like this:
// GetStore returns nil as a mock implementation. // Note: This may need to be updated if tests require a non-nil RootStore. func (*mockApp[T]) GetStore() storev2.RootStore { return nil }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- server/v2/server_test.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/root_di.go
🧰 Additional context used
📓 Path-based instructions (1)
server/v2/server_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"
🔇 Additional comments (2)
server/v2/server_test.go (2)
72-74: LGTM: Simplified store server initialization.The removal of
mockStoreBuilderand direct use ofstore.New[transaction.Tx]()simplifies the test setup. This change aligns well with the refactoring in the main code and makes the test more straightforward.
Line range hint
1-124: Overall assessment: Changes improve code clarity and align with refactoring goals.The modifications in this file, particularly the addition of the
GetStoremethod tomockAppand the simplification of thestoreServerinitialization, align well with the PR objectives of refactoring store management. These changes improve code clarity and simplify the test setup without introducing any apparent issues.The test coverage appears sufficient for the changes associated with this PR. However, ensure that these modifications are consistent with changes in other files and that they adequately test the new
AppI.GetStore()functionality in the main code.
6 participants
Description
Alternative to: #22204
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
Refactor
General Cleanup