fix(server/v2/cometbft): fix mock store #22293
Conversation
julienrbrt
requested review from
cool-develope,
kocubinski,
a team,
facundomedica and
sontrinh16
as code owners
📝 WalkthroughWalkthroughThe changes in this pull request involve modifications 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
|
github-actions
bot
added
C:Store
C:server/v2
|
@julienrbrt 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: 1
🧹 Outside diff range and nitpick comments (4)
store/v2/mock/types.go (1)
Remove Unused
UpgradableDatabasefromStateStorageInterfaceThe
StateStorageinterface currently embedsstore.UpgradableDatabase, but there are no implementations or usages ofUpgradableDatabasefound in the codebase. It is recommended to removestore.UpgradableDatabasefrom theStateStorageinterface to eliminate unnecessary dependencies.
- File: store/v2/mock/types.go
- Lines: 10-16
🔗 Analysis chain
Line range hint
14-18: Verify consistency of embedded interfacesThe
StateStorageinterface correctly embedsstore.VersionedWriter, which aligns with the updated comment. However, it also embedsstore.UpgradableDatabase. Please verify if this is still appropriate given the transition fromVersionedDatabasetoVersionedWriter.To check the definitions and usages of these interfaces:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check definitions and usages of VersionedWriter and UpgradableDatabase echo "Definition of VersionedWriter:" rg --type go "type\s+VersionedWriter\s+interface" echo "Definition of UpgradableDatabase:" rg --type go "type\s+UpgradableDatabase\s+interface" echo "Usages of UpgradableDatabase in relation to VersionedWriter:" rg --type go -C 5 "UpgradableDatabase.*VersionedWriter|VersionedWriter.*UpgradableDatabase"Length of output: 590
Script:
#!/bin/bash # Description: Find all implementations of the StateStorage interface echo "Searching for implementations of StateStorage:" rg --type go "type\s+\w+\s+struct\s+.*StateStorage" echo "Searching for methods implementing UpgradableDatabase:" rg --type go "func\s+\(.*\)\s+.*UpgradableDatabase" echo "Checking for deprecation notices related to UpgradableDatabase:" rg --type go "Deprecated.*UpgradableDatabase" echo "Listing all references to UpgradableDatabase in comments:" rg --type go "//.*UpgradableDatabase"Length of output: 997
server/v2/cometbft/internal/mock/mock_store.go (2)
19-19: LGTM! Consider updating documentation.The change from
storev2.VersionedDatabasetostorev2.VersionedWriterfor theStoragefield is appropriate. It suggests a more focused interface for writing versioned data, which aligns well with the mock store's responsibilities.Consider adding a comment to explain the rationale behind using
VersionedWriterinstead ofVersionedDatabase. This would help future maintainers understand the design decision.
86-86: LGTM! Update method documentation.The change in the return type from
storev2.VersionedDatabasetostorev2.VersionedWriteris consistent with the previous modifications.Consider updating the method documentation to reflect the new return type and explain any implications of this change for users of the
MockStore.store/v2/storage/store.go (1)
25-25: LGTM with a minor suggestion for documentation improvement.The update to use
store.VersionedWriterinstead ofstore.VersionedDatabasein theStorageStoredocumentation and interface assertions is correct and aligns with the changes in the underlying functionality.Consider expanding the documentation slightly to provide more context:
- // StorageStore is a wrapper around the store.VersionedWriter interface. + // StorageStore is a wrapper around the store.VersionedWriter interface, + // providing versioned storage capabilities for the Cosmos SDK.This addition gives readers a clearer understanding of the struct's purpose within the larger system.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (4)
- server/v2/cometbft/internal/mock/mock_store.go (3 hunks)
- store/v2/mock/types.go (1 hunks)
- store/v2/storage/README.md (4 hunks)
- store/v2/storage/store.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
server/v2/cometbft/internal/mock/mock_store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/mock/types.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/storage/README.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"store/v2/storage/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 LanguageTool
store/v2/storage/README.md
[misspelling] ~95-~95: Did you mean “meant”?
Context: ...rface in thestoragepackage which is mean to be represent aVersionedWriterwit...(MEAN_MEANT)
[grammar] ~95-~95: Consider using either the past participle “represented” or the present participle “representing” here.
Context: ...estoragepackage which is mean to be represent aVersionedWriterwith only the neces...(BEEN_PART_AGREEMENT)
🔇 Additional comments (7)
store/v2/mock/types.go (1)
13-13: Comment update reflects interface change.The comment has been updated to indicate that
StateStorageis now a mock ofstore.VersionedWriterinstead ofstore.VersionedDatabase. This change aligns with the PR objectives and the AI-generated summary, which mention a transition fromstorev2.VersionedDatabasetostorev2.VersionedWriter.To ensure consistency across the codebase, let's verify the usage of
VersionedWriter:✅ Verification successful
Comment update verified successfully.
The
StateStoragecomment now correctly referencesstore.VersionedWriter, and there are no remaining occurrences ofVersionedDatabasein the codebase. This change aligns with the project's objectives and maintains consistency.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for occurrences of VersionedWriter and VersionedDatabase echo "Occurrences of VersionedWriter:" rg --type go "VersionedWriter" echo "Occurrences of VersionedDatabase:" rg --type go "VersionedDatabase"Length of output: 2211
server/v2/cometbft/internal/mock/mock_store.go (3)
39-39: LGTM! Consistent change.The modification of the
ssparameter type fromstorev2.VersionedDatabasetostorev2.VersionedWriteris consistent with the previous changes. This ensures type consistency throughout the mock store implementation.
Line range hint
1-138: Overall assessment: Changes are consistent and well-implemented.The modifications in this file consistently replace
storev2.VersionedDatabasewithstorev2.VersionedWriteracross theMockStoreimplementation. This change likely reflects a design decision to use a more specific interface for writing versioned data.Key points:
- All changes are consistent and properly implemented.
- The transition to
VersionedWritermight provide better encapsulation of write operations.- Minor suggestions for documentation updates have been made to improve code clarity.
To further improve the PR:
- Update relevant documentation to explain the rationale behind using
VersionedWriter.- Verify that all callers of these modified functions are updated accordingly.
- Ensure that comprehensive tests are in place to validate the behavior with the new interface.
23-23: LGTM! Verifystorage.NewStorageStorereturn type.The change in the return type from
storev2.VersionedDatabasetostorev2.VersionedWriteris consistent with theMockStorestruct modification.Please verify that
storage.NewStorageStorenow returns astorev2.VersionedWriter. Run the following command to check:Ensure that
$ret_typematchesstorev2.VersionedWriteror a type that implements this interface.store/v2/storage/README.md (3)
5-5: LGTM: Interface update correctly reflectedThe change from
VersionedDatabasetoVersionedWriteris consistent with the reported interface update.
29-29: LGTM: Interface update correctly reflected in RocksDB sectionThe change from
VersionedDatabasetoVersionedWriteris consistent in the RocksDB implementation description.
45-45: LGTM: Interface update correctly reflected in SQLite sectionThe change from
VersionedDatabasetoVersionedWriteris consistent in the SQLite implementation description.
julienrbrt
added
the
backport/v0.52.x
(cherry picked from commit f01baf3) # Conflicts: # store/v2/mock/types.go # store/v2/storage/README.md # store/v2/storage/store.go
Co-authored-by: Julien Robert <[email protected]>
Description
Follow-up of #22285
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
VersionedWriter, enhancing performance and compatibility.Documentation
VersionedDatabasetoVersionedWriteracross thestoragepackage.Bug Fixes
StorageStorefunctionality.