refactor(server/v2): clean up storage use and config #22008
Conversation
kocubinski
requested review from
facundomedica,
hieuvubk,
julienrbrt and
sontrinh16
as code owners
|
Note Reviews pausedUse the following commands to manage reviews:
📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve significant modifications to various components within the application, focusing on the restructuring of the 📝 ChangesChanges
📝 Possibly related PRsPossibly related PRs
📝 Suggested labelsSuggested labels
📝 Suggested reviewersSuggested 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
|
github-actions
bot
added
C:Store
C:server/v2
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
server/v2/cometbft/types/store.go (1)
Line range hint
9-36: Consider adding interface documentationThe
Storeinterface seems to play a crucial role in the system. To improve code readability and maintainability, consider adding a comment block above the interface declaration explaining its purpose, responsibilities, and any important details about its usage.For example:
// Store represents the main storage interface for the Cosmos SDK. // It combines low-level storage operations with high-level state management, // providing a unified interface for interacting with the blockchain state. type Store interface { // ... existing code ... }This documentation will help developers understand the interface's role more quickly and use it correctly.
store/v2/store.go (1)
70-77: Well-structuredBackendinterface with clear method signaturesThe new
Backendinterface is a great addition that improves the overall structure of the codebase. It effectively encapsulates methods for retrieving state storage and state commitment, which were previously part of theRootStoreinterface. This change:
- Enhances modularity and maintainability
- Adheres to the interface segregation principle
- Uses clear and descriptive method names, following Go conventions
Consider adding a brief comment above the
Backendinterface to explain its purpose and relationship withRootStore, similar to the comments for other interfaces in this file. For example:// Backend defines the interface for accessing the underlying storage and commitment // backends used by the RootStore. type Backend interface { // ... (existing methods) }simapp/v2/simdv2/cmd/root_di.go (1)
88-89: LGTM: Updated initRootCmd call with store parameterThe modification to create a
storeusingstoreBuilder.Get()and pass it toinitRootCmdis consistent with the PR objectives. This change reflects the restructuring of thestore/v2.RootStoreinterface and aligns with the new storage management approach.Consider renaming the
storevariable to something more descriptive, such asrootStore, to better reflect its purpose and type:-store := storeBuilder.Get() -initRootCmd(rootCmd, clientCtx.TxConfig, store, moduleManager) +rootStore := storeBuilder.Get() +initRootCmd(rootCmd, clientCtx.TxConfig, rootStore, moduleManager)server/v2/store/snapshot.go (1)
79-79: Add a comment for the exported functionRestoreSnapshotCmd.According to the Uber Go Style Guide, all exported functions should have a comment explaining what the function does. Consider adding a comment for
RestoreSnapshotCmdto improve code documentation.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (10)
- server/v2/cometbft/server.go (3 hunks)
- server/v2/cometbft/types/store.go (1 hunks)
- server/v2/commands.go (1 hunks)
- server/v2/server_test.go (1 hunks)
- server/v2/store/server.go (4 hunks)
- server/v2/store/snapshot.go (3 hunks)
- server/v2/types.go (0 hunks)
- simapp/v2/simdv2/cmd/commands.go (3 hunks)
- simapp/v2/simdv2/cmd/root_di.go (3 hunks)
- store/v2/store.go (2 hunks)
💤 Files with no reviewable changes (1)
- server/v2/types.go
✅ Files skipped from review due to trivial changes (2)
- server/v2/commands.go
- server/v2/server_test.go
🧰 Additional context used
📓 Path-based instructions (7)
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/cometbft/types/store.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/store/snapshot.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.store/v2/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (19)
server/v2/cometbft/types/store.go (1)
10-11: Approved: Good use of interface embeddingThe addition of
storev2.Backendto theStoreinterface is a good refactoring decision. This change:
- Streamlines the interface by potentially reducing duplicate method declarations.
- Aligns with the PR objectives to simplify dependency management.
- Follows Go's composition over inheritance principle.
- Adheres to the Uber Golang style guide for interface composition.
server/v2/store/server.go (5)
12-12: LGTM: Import addition is appropriate.The addition of the
storev2import is consistent with the changes made in the file, particularly the use ofstorev2.Backend. The import alias is clear and follows good naming conventions.
26-26: LGTM: Server struct update aligns with PR objectives.The addition of the
storefield of typestorev2.Backendto theServerstruct is consistent with the PR's goal of streamlining storage-related dependency management. This change appropriately replaces the previousappCreatorfield.
29-30: LGTM: New function signature update is appropriate.The
Newfunction has been correctly updated to accept astorev2.Backendparameter instead of anappCreator. This change is consistent with the modifications made to theServerstruct and aligns with the PR objectives of streamlining storage-related dependencies.
66-66: LGTM: CLICommands method update is consistent.The update to the
RestoreSnapshotCmdcall in theCLICommandsmethod is appropriate. Usings.storeinstead ofs.appCreatoris consistent with the changes made to theServerstruct and aligns with the PR objectives of removing theappCreatordependency.
33-33: Signature update looks good, but consider logger usage.The
Initmethod signature has been appropriately updated to reflect the changes in theServerstruct and align with the PR objectives. The removal of theappIparameter usage is consistent with the goal of simplifying the interface.However, I noticed that the
loggerparameter is now unused.Could you please verify if the logger is intentionally unused now? If it's no longer needed, consider removing it from the method signature entirely. If it is still required elsewhere, please ensure it's properly utilized or document why it's kept as an unused parameter.
To help verify the logger usage, you can run the following command:
This will help us understand if there are any other implementations of the
Initmethod that still use the logger parameter.store/v2/store.go (2)
15-17: Excellent use of composition forRootStoreinterfaceThe addition of
PrunerandBackendto theRootStoreinterface is a good design choice. This change:
- Improves separation of concerns
- Aligns with Go's preference for composition over inheritance
- Simplifies the
RootStoreinterface, making it more modular and easier to maintainThis modification successfully achieves the PR objective of streamlining dependency management and simplifying the interface.
15-17: Verify removal of methods fromRootStoreThe AI summary mentions the removal of
GetStateStorage()andGetStateCommitment()methods from theRootStoreinterface. This change is consistent with the addition of theBackendinterface and aligns with the PR objectives of simplifying theRootStoreinterface.To confirm this change, please run the following script:
If the script output shows the
RootStoreinterface without these methods, it confirms their removal as mentioned in the AI summary.simapp/v2/simdv2/cmd/root_di.go (3)
35-35: LGTM: Addition of storeBuilder variableThe introduction of
storeBuilder runtime.StoreBuilderaligns with the PR objectives to streamline dependency management related to storage. This change is consistent with the removal of theGetStore()method from theAppIinterface as mentioned in the PR summary.
55-55: LGTM: storeBuilder added to dependency injectionThe addition of
storeBuilderto the dependency injection is consistent with its introduction as a variable. This ensures thatstoreBuilderis properly initialized through the dependency injection process, which is in line with the PR's objective of refactoring storage usage.
Line range hint
1-153: Overall assessment: Changes align well with PR objectivesThe modifications in this file successfully implement the intended refactoring of storage usage. The introduction of
storeBuilder, its integration into the dependency injection process, and the updatedinitRootCmdcall all contribute to streamlining the dependency management related to storage.These changes are consistent with the PR objectives, particularly:
- The removal of the
GetStore()method from theAppIinterface.- The restructuring of the
store/v2.RootStoreinterface.The code adheres to the Uber Golang style guide, maintaining good code quality and readability.
server/v2/cometbft/server.go (5)
46-46: LGTM: Addition ofstorefield toCometBFTServerstructThe addition of the
storefield of typetypes.Storeto theCometBFTServerstruct aligns with the PR objectives to streamline dependency management related to storage. This change is consistent with the removal of theGetStore()method from theAppIinterface as mentioned in the PR summary.
52-57: LGTM: UpdatedNewfunction signatureThe addition of the
storeparameter to theNewfunction signature is consistent with the changes made to theCometBFTServerstruct. This update ensures that thestoreis properly initialized when creating a newCometBFTServerinstance, which aligns with the PR objectives.
60-60: LGTM: Initialization ofstorefield inCometBFTServerThe initialization of the
storefield in theCometBFTServerstruct is consistent with the changes made to the struct definition and theNewfunction signature. This ensures that thestoreis properly set when creating a newCometBFTServerinstance.
115-115: LGTM: UpdatedConsensusinitialization withstoreparameterThe addition of the
storeparameter to theNewConsensusfunction call in theInitmethod is consistent with the changes made to theCometBFTServerstruct. This update ensures that theConsensusinstance has access to thestorefor state management, which aligns with the PR objectives of streamlining storage dependency management.
127-128: LGTM: Updated snapshot management to uses.storeThe changes to use
s.store.GetStateStorage()ands.store.GetStateCommitment()instead ofappI.GetStore()are consistent with the removal of theGetStore()method from theAppIinterface as mentioned in the PR summary. This update aligns with the PR objectives of streamlining storage dependency management.However, it's important to verify the type assertions
.(snapshots.StorageSnapshotter)and.(snapshots.CommitSnapshotter)to ensure type safety. Consider adding runtime checks or using type switches to handle potential type mismatches gracefully.To verify the type assertions, you can run the following script:
simapp/v2/simdv2/cmd/commands.go (3)
20-21: Imports are correctly organized and aliasedThe new imports
cometbfttypesandserverstoreare properly added with clear aliases that improve code readability and prevent naming conflicts.
87-87: Validate the addition ofserverstore.New[T](store)inserverv2.AddCommandsIncluding
serverstore.New[T](store)as an argument toserverv2.AddCommandsshould match the expected parameters of the function. Verify that this addition is appropriate and that it integrates correctly with the rest of the application's initialization process.Run the following script to examine the
serverv2.AddCommandsfunction signature:#!/bin/bash # Description: Check the parameters accepted by 'serverv2.AddCommands' to ensure compatibility. # Test: Locate the definition of 'serverv2.AddCommands' and display its signature. rg --type go -A 5 'func AddCommands\(' | grep -A 5 'func AddCommands('
82-85: Confirmstoreis correctly passed tocometbft.NewThe addition of
storeas a parameter tocometbft.Newshould align with its expected function signature. Ensure thatcometbft.Newacceptsstoreas an argument in this position and that the types are compatible.Run the following script to check the definition of
cometbft.New:#!/bin/bash # Description: Verify the function signature of 'cometbft.New' to confirm it accepts 'store' as a parameter. # Test: Find the definition of 'cometbft.New' in the codebase. rg --type go -A 5 'func New\(' | grep -A 5 'func New('
| sm := snapshots.NewManager( | ||
| snapshotStore, snapshots.NewSnapshotOptions(interval, uint32(keepRecent)), | ||
| store.GetStateCommitment().(snapshots.CommitSnapshotter), | ||
| store.GetStateStorage().(snapshots.StorageSnapshotter), | ||
| nil, logger) |
There was a problem hiding this comment.
Handle type assertions to prevent potential panics.
At lines 376-377, the code uses unchecked type assertions. The Uber Go Style Guide recommends using the two-value form to handle cases where the assertion might fail, preventing runtime panics.
Modify the code to handle type assertion failures:
commitSnapshotter, ok := store.GetStateCommitment().(snapshots.CommitSnapshotter)
if !ok {
return nil, fmt.Errorf("store does not implement snapshots.CommitSnapshotter")
}
storageSnapshotter, ok := store.GetStateStorage().(snapshots.StorageSnapshotter)
if !ok {
return nil, fmt.Errorf("store does not implement snapshots.StorageSnapshotter")
}
sm := snapshots.NewManager(
snapshotStore,
snapshots.NewSnapshotOptions(interval, uint32(keepRecent)),
commitSnapshotter,
storageSnapshotter,
nil,
logger,
)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
server/v2/store/server.go (1)
26-26: LGTM:Serverstruct update is appropriate.The addition of the
storefield of typestorev2.Backendaligns well with the PR objectives. It's a good step towards abstracting storage operations.Consider adding a brief comment above the
storefield to describe its purpose, e.g.:// store is the backend storage interface store storev2.Backendstore/v2/store.go (1)
70-77: Well-structured Backend interface with room for minor improvementThe new
Backendinterface is a great addition that aligns with the PR objectives. It effectively separates the concerns of state storage and state commitment from theRootStoreinterface, enhancing modularity and clarity.The interface is well-defined and follows good Go practices. However, to further improve documentation:
Consider adding a brief comment for each method to explain their purpose:
type Backend interface { // GetStateStorage returns the SS (State Storage) backend. GetStateStorage() VersionedDatabase // GetStateCommitment returns the SC (State Commitment) backend. GetStateCommitment() Committer }This addition would provide more context for developers working with this interface in the future.
simapp/v2/simdv2/cmd/root_di.go (1)
88-89: LGTM: Updated initRootCmd call with store parameterThe changes to the
initRootCmdfunction call align well with the PR objectives. The addition of thestoreparameter, obtained fromstoreBuilder.Get(), reflects the removal of theGetStore()method from theAppIinterface. This modification contributes to simplifying the interface and improving the overall code structure.For improved clarity, consider adding a brief comment explaining the purpose of the
storeparameter:// Get the store from the storeBuilder for use in initRootCmd store := storeBuilder.Get() initRootCmd(rootCmd, clientCtx.TxConfig, store, moduleManager)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (10)
- server/v2/cometbft/server.go (3 hunks)
- server/v2/cometbft/types/store.go (1 hunks)
- server/v2/commands.go (1 hunks)
- server/v2/server_test.go (1 hunks)
- server/v2/store/server.go (4 hunks)
- server/v2/store/snapshot.go (3 hunks)
- server/v2/types.go (0 hunks)
- simapp/v2/simdv2/cmd/commands.go (3 hunks)
- simapp/v2/simdv2/cmd/root_di.go (3 hunks)
- store/v2/store.go (2 hunks)
💤 Files with no reviewable changes (1)
- server/v2/types.go
✅ Files skipped from review due to trivial changes (2)
- server/v2/commands.go
- server/v2/server_test.go
🧰 Additional context used
📓 Path-based instructions (7)
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/cometbft/types/store.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/store/snapshot.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.store/v2/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (20)
server/v2/cometbft/types/store.go (2)
10-11: Approved: Interface simplification aligns with PR objectivesThe changes to the
Storeinterface, including embeddingstorev2.Backendand removingGetStateStorage()andGetStateCommitment()methods, effectively streamline the dependency management related to storage. This simplification aligns well with the PR objectives of reducing unnecessary complexity and improving the overall structure of the code.The embedding of
storev2.Backendis a good practice, as it allows theStoreinterface to inherit methods without explicitly declaring them, following the Uber Go Style Guide recommendation of preferring embedding to type aliases for interfaces.
Line range hint
1-38: Approved: Well-structured and documented interfaceThe overall structure and documentation of the
Storeinterface are commendable:
- The imports are correctly organized and follow the Uber Go Style Guide.
- Each method has a descriptive comment, which is a good practice and enhances code readability.
- The method signatures are consistent and follow Go naming conventions.
- The interface is well-organized and focused on a single responsibility, adhering to good design principles.
These aspects contribute to the maintainability and clarity of the code, which aligns with the PR's goal of improving the overall structure of the codebase.
server/v2/store/server.go (4)
12-12: LGTM: Import statement addition is appropriate.The addition of the
storev2import is consistent with the changes in the file and follows good naming conventions.
29-30: LGTM:Newfunction update is appropriate.The modification to accept
store storev2.Backendinstead ofappCreatoris consistent with theServerstruct changes and aligns with the PR objectives. This change simplifies the initialization process and removes the dependency onAppI.GetStore().
66-66: LGTM:CLICommandsmethod update is consistent.The change to use
s.storeinstead ofs.appCreatorin theRestoreSnapshotCmdcall is consistent with the other modifications in this PR.To ensure full compatibility, please verify that the
RestoreSnapshotCmdfunction has been updated to accept astorev2.Backendparameter instead of anAppCreator. You can run the following command to check the function signature:This will show the current signature of the
RestoreSnapshotCmdfunction, which should now accept astorev2.Backendparameter.✅ Verification successful
Verified:
RestoreSnapshotCmdfunction signature correctly updated.The
RestoreSnapshotCmdfunction now accepts astorev2.Backendparameter as intended, ensuring compatibility with the recent changes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
rg --type go -C 5 'func.*RestoreSnapshotCmd' server/v2/store/Length of output: 723
33-33: LGTM with a minor concern:Initmethod signature update.The change to ignore the
appIparameter is consistent with the PR objectives. However, ignoring theloggerparameter might lead to loss of logging capabilities in this method.Could you please confirm if logging is no longer needed in the
Initmethod? If logging is still required, consider keeping theloggerparameter and using it within the method body.To verify the usage of logging in the
Initmethod, you can run the following command:This will show if there were any logging statements in the
Initmethod that might have been removed.✅ Verification successful
loggerparameter inInitmethod is safely removed.No logging statements found in the
Initmethod, so removing theloggerparameter does not impact logging capabilities.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
rg --type go -C 5 'log(ger)?\.' server/v2/store/server.goLength of output: 445
store/v2/store.go (3)
15-17: Excellent refactoring of the RootStore interfaceThe changes to the
RootStoreinterface improve the code structure and align well with the PR objectives. By incorporating thePrunerandBackendinterfaces, you've effectively segregated responsibilities and simplified theRootStoreinterface. This refactoring enhances maintainability and clarity of the codebase.The removal of
GetStateStorage()andGetStateCommitment()methods fromRootStore(as indicated in the AI summary) further contributes to this simplification. These changes adhere to the interface segregation principle, which is consistent with good Go programming practices.
Line range hint
1-110: Summary of changes and their impactThe refactoring in this file significantly improves the structure and clarity of the
RootStoreinterface and related components. Key improvements include:
- Simplification of the
RootStoreinterface by embeddingPrunerandBackend.- Introduction of a new
Backendinterface to separate state storage and commitment concerns.- Moving the
Prunemethod to thePrunerinterface, adhering to the interface segregation principle.These changes align well with the PR objectives of streamlining dependency management and enhancing code maintainability. The refactoring follows good Go programming practices and the Uber Golang style guide.
Overall, this is a well-executed refactoring that should positively impact the codebase's organization and clarity.
15-15: Approve embedding of Pruner interface in RootStoreThe embedding of the
Prunerinterface inRootStoreis a positive change that aligns with the PR objectives. It streamlines dependency management and improves the overall structure of the code.To ensure this change doesn't introduce any unintended consequences, let's verify the usage of the
Prunemethod across the codebase:This verification will help ensure that all usages of the
Prunemethod are still valid after this refactoring.✅ Verification successful
Pruner interface embedding in RootStore verified
The absence of direct calls to
RootStore.Prune()and the presence ofPruneimplementations across the codebase confirm that embedding thePrunerinterface inRootStorehas been successfully implemented without introducing any issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any direct calls to RootStore.Prune() that might need updating # Test: Search for direct calls to RootStore.Prune(). Expect: No results, as Prune is now part of the Pruner interface. rg --type go 'RootStore.*\.Prune\(' # Test: Search for implementations of the Prune method. Expect: Implementations in concrete types that satisfy RootStore. rg --type go 'func \(.*\) Prune\('Length of output: 1576
simapp/v2/simdv2/cmd/root_di.go (1)
35-35: LGTM: Addition of storeBuilder variableThe introduction of the
storeBuildervariable of typeruntime.StoreBuilderaligns with the PR objectives to clean up storage use. This change appears to be part of the refactoring process to streamline dependency management related to storage.simapp/v2/simdv2/cmd/commands.go (4)
20-21: Approved: Importing necessary packagesThe addition of
cometbfttypesandserverstoreimports is appropriate and follows the Uber Golang style guide.
47-47: Approved: Addedstoreparameter toinitRootCmdIncluding
store cometbfttypes.Storein theinitRootCmdfunction signature enhances dependency management and aligns with the refactoring objectives.
82-82: Approved: Passingstoretocometbft.NewPassing the
storevariable tocometbft.Newensures proper initialization of the CometBFT component with the correct store configuration.
87-87: Approved: Initializing server store withstoreInitializing the server store using
serverstore.New[T](store)is appropriate and adheres to best practices for dependency injection.server/v2/cometbft/server.go (4)
46-46: Addition ofstorefield toCometBFTServerstructThe
storefield is appropriately added to theCometBFTServer[T]struct to encapsulate the state management functionality. This change enhances encapsulation and reduces dependency on external interfaces.
52-57: Update toNewfunction signature to includestoreparameterThe
Newfunction now accepts astore types.Storeparameter, aligning with the addition of thestorefield in the struct. The parameter ordering is logical, and the variadiccfgOptionsremains at the end, adhering to Go conventions.
60-60: Assignment ofstorefield in struct initializationThe
storefield is correctly initialized within theCometBFTServerconstructor, ensuring that thestoreis available throughout the instance's lifecycle.
115-115: Passings.storetoNewConsensusfunctionThe
s.storeis appropriately passed as an argument to theNewConsensusfunction, replacing the previous dependency on theappI.GetStore()method. This modification simplifies the dependency chain and aligns with the PR objective of cleaning up storage use.server/v2/store/snapshot.go (2)
79-79: Function signature updated appropriatelyThe
RestoreSnapshotCmdfunction now acceptsrootStore storev2.Backendas a parameter, aligning with the PR objectives to simplify dependency management and improve code structure.
351-353: Function signature updated for enhanced flexibilityChanging the
createSnapshotsManagerfunction to acceptstore storev2.Backendinstead ofstorev2.RootStoreenhances the flexibility of the snapshot management system and aligns with the refactoring goals.
server/v2/cometbft/server.go
Outdated
| ss := s.store.GetStateStorage().(snapshots.StorageSnapshotter) | ||
| sc := s.store.GetStateCommitment().(snapshots.CommitSnapshotter) |
There was a problem hiding this comment.
Handle unchecked type assertions to prevent potential panics
The type assertions on s.store.GetStateStorage() and s.store.GetStateCommitment() are unchecked and could lead to a panic if the assertions fail. It is recommended to perform type assertion checks to handle potential failures gracefully.
Apply the following changes to handle potential type assertion failures:
- ss := s.store.GetStateStorage().(snapshots.StorageSnapshotter)
- sc := s.store.GetStateCommitment().(snapshots.CommitSnapshotter)
+ ssInterface := s.store.GetStateStorage()
+ ss, ok := ssInterface.(snapshots.StorageSnapshotter)
+ if !ok {
+ return fmt.Errorf("failed to assert s.store.GetStateStorage() to snapshots.StorageSnapshotter")
+ }
+ scInterface := s.store.GetStateCommitment()
+ sc, ok := scInterface.(snapshots.CommitSnapshotter)
+ if !ok {
+ return fmt.Errorf("failed to assert s.store.GetStateCommitment() to snapshots.CommitSnapshotter")
+ }This change ensures that the type assertions are checked, and appropriate error handling is in place to prevent unexpected panics.
📝 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.
| ss := s.store.GetStateStorage().(snapshots.StorageSnapshotter) | |
| sc := s.store.GetStateCommitment().(snapshots.CommitSnapshotter) | |
| ssInterface := s.store.GetStateStorage() | |
| ss, ok := ssInterface.(snapshots.StorageSnapshotter) | |
| if !ok { | |
| return fmt.Errorf("failed to assert s.store.GetStateStorage() to snapshots.StorageSnapshotter") | |
| } | |
| scInterface := s.store.GetStateCommitment() | |
| sc, ok := scInterface.(snapshots.CommitSnapshotter) | |
| if !ok { | |
| return fmt.Errorf("failed to assert s.store.GetStateCommitment() to snapshots.CommitSnapshotter") | |
| } |
| sm := snapshots.NewManager( | ||
| snapshotStore, snapshots.NewSnapshotOptions(interval, uint32(keepRecent)), | ||
| store.GetStateCommitment().(snapshots.CommitSnapshotter), | ||
| store.GetStateStorage().(snapshots.StorageSnapshotter), | ||
| nil, logger) |
There was a problem hiding this comment.
Prevent potential runtime panic due to unchecked type assertions
In the call to snapshots.NewManager, the code performs unchecked type assertions:
store.GetStateCommitment().(snapshots.CommitSnapshotter),
store.GetStateStorage().(snapshots.StorageSnapshotter),If store.GetStateCommitment() or store.GetStateStorage() do not return types that implement the expected interfaces, this will cause a runtime panic.
Consider using the comma-ok idiom to safely perform the type assertions and handle any errors appropriately. Here's how you can modify the code:
+ commitSnapshotter, ok := store.GetStateCommitment().(snapshots.CommitSnapshotter)
+ if !ok {
+ return nil, fmt.Errorf("store's state commitment does not implement snapshots.CommitSnapshotter")
+ }
+ storageSnapshotter, ok := store.GetStateStorage().(snapshots.StorageSnapshotter)
+ if !ok {
+ return nil, fmt.Errorf("store's state storage does not implement snapshots.StorageSnapshotter")
+ }
- sm := snapshots.NewManager(
- snapshotStore, snapshots.NewSnapshotOptions(interval, uint32(keepRecent)),
- store.GetStateCommitment().(snapshots.CommitSnapshotter),
- store.GetStateStorage().(snapshots.StorageSnapshotter),
- nil, logger)
+ sm := snapshots.NewManager(
+ snapshotStore, snapshots.NewSnapshotOptions(interval, uint32(keepRecent)),
+ commitSnapshotter,
+ storageSnapshotter,
+ nil, logger)This change ensures that if the type assertions fail, a meaningful error is returned instead of causing a panic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
simapp/v2/simdv2/cmd/testnet.go (3)
91-91: Use consistent and descriptive parameter names forcometbfttypes.StoreThe parameter is named
rsinNewTestnetCmdandtestnetInitFilesCmd, but it's calledrootStoreininitTestnetFiles. For better readability and consistency, consider using the same descriptive name, such asrootStore, across all functions.
106-106: Maintain consistent parameter naming across functionsIn the function
testnetInitFilesCmd, the parameterrsis used forcometbfttypes.Store. To enhance code clarity, consider renaming it torootStoreto match the naming ininitTestnetFiles.
147-147: Ensure consistent naming when passing parametersWhen calling
initTestnetFiles, the variablersis passed asrootStore. Using consistent parameter names across functions improves readability. Consider renamingrstorootStorehere as well.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- simapp/v2/simdv2/cmd/commands.go (4 hunks)
- simapp/v2/simdv2/cmd/testnet.go (6 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/testnet.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
simapp/v2/simdv2/cmd/testnet.go (2)
344-344: Confirm the correct usage ofrootStoreincometbft.NewPassing
rootStoreintocometbft.Newaligns with the intended design to improve dependency management. Ensure thatrootStoreimplements the necessary interfaces expected bycometbft.New.
348-348: Validate the initialization ofstoreServerThe creation of
storeServerusingstore.New[T](rootStore)appears correct. Verify thatrootStoremeets the requirements of thestore.Newfunction to prevent potential runtime issues.
|
Added DNM, attending to it now. |
We do, system test is our coverage, as well as simapp-v2 tests: https://github.com/cosmos/cosmos-sdk/actions/runs/11131381060/job/30932833603?pr=22008 Personally, forgot to look at it, as I knew it was broken on other stuff. |
|
The simapp v2 tests are still failing |
store/v2/root/builder.go
Outdated
| // 2. Configuration and loading | ||
| // | ||
| // The Builder interface is used to facilitate this pattern. Namespaces (store keys) are registered | ||
| // by calling RegisterNamespace before Build is called. Build is then called with a Config |
(cherry picked from commit 05fb492) # Conflicts: # runtime/v2/app.go # runtime/v2/builder.go # runtime/v2/module.go # runtime/v2/store.go # server/v2/commands.go # server/v2/config_test.go # server/v2/server.go # server/v2/server_test.go # server/v2/store/server.go # server/v2/store/snapshot.go # server/v2/types.go # store/v2/root/factory.go # store/v2/store.go
#22176) Co-authored-by: Matt Kocubinski <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
Related to: #21678
This PR cleans up the dependency story for storage. After the merge of #21989 we no longer need to further bloat the
AppIinterface withGetStore() any, which is the core change in this PR.The
store/v2.RootStoreinterface was also broken up slightly so that the dependency lines are cleaner for clients in server/v2.Configuration specification was pushed down from
server/v2/storeintostore/v2directly. TheStoreBuilderwas also moved there to prevent a dependency from forming (in either direction)runtime/v2 <-> server/v2.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
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor