feat(store/v2): add version exists #22235
Conversation
github-actions
bot
added
C:Store
C:server/v2
github-actions
bot
removed
C:server/v2
cool-develope
changed the title
cool-develope
requested review from
cool-develope,
kocubinski,
a team,
julienrbrt and
hieuvubk
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant changes 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
|
|
@cool-develope 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: 7
🧹 Outside diff range and nitpick comments (13)
store/v2/mock/types.go (1)
15-15: Consider adding documentation for theStateStorageinterfaceThe change from
store.VersionedDatabasetostore.VersionedWriteraligns with the PR objectives and reflects a shift in the expected functionality. This modification narrows the interface from a full database to just a writer, which could impact how the mock is used in tests.While the code adheres to the Uber Golang style guide, it would be beneficial to add a comment explaining the purpose of the
StateStorageinterface and the rationale behind usingVersionedWriterinstead ofVersionedDatabase. This would improve code clarity and maintainability.Consider adding a comment above the
StateStorageinterface to explain its purpose and the reason for usingVersionedWriter. For example:// StateStorage is a mock interface that represents a versioned storage writer. // It uses VersionedWriter to focus on write operations in the context of versioning.store/v2/storage/database.go (1)
19-19: New method addition looks good, but consider documentation.The new
VersionExistsmethod is a valuable addition to theDatabaseinterface, allowing for version existence checks. The method signature follows Go naming conventions and aligns well with the existing interface design.However, to improve clarity and maintainability:
- Consider adding a comment explaining the purpose and behavior of this method, following the style of other methods in the interface.
- Ensure that all implementations of the
Databaseinterface are updated to include this new method.You might want to add a comment like this above the method:
// VersionExists checks if a specific version exists in the database. // It returns true if the version exists, false otherwise, and any error encountered. VersionExists(version uint64) (bool, error)store/v2/storage/rocksdb/db_noflag.go (1)
39-41: LGTM with a minor suggestion for consistency.The new
VersionExistsmethod is correctly implemented and follows the same pattern as other methods in this file. It appropriately panics when RocksDB is not enabled.For consistency with other methods in the file, consider adding a blank line after the method definition.
Here's a suggested minor change:
func (db *Database) VersionExists(version uint64) (bool, error) { panic("rocksdb requires a build flag") } +store/v2/storage/storage_bench_test.go (1)
27-43: LGTM! Consider grouping similar backend initializations.The change from
store.VersionedDatabasetostore.VersionedWriteris consistent across all backend types and aligns with the PR objectives. This modification likely reflects a design change in the store package, moving to a more generalVersionedWriterinterface.To improve code organization and readability, consider extracting the common
storage.NewStorageStore(db, coretesting.NewNopLogger())call into a separate function. This would reduce duplication and make it easier to modify in the future if needed.Here's a suggested refactoring:
func newStorageStore(db store.VersionedDatabase) store.VersionedWriter { return storage.NewStorageStore(db, coretesting.NewNopLogger()) } backends = map[string]func(dataDir string) (store.VersionedWriter, error){ "rocksdb_versiondb_opts": func(dataDir string) (store.VersionedWriter, error) { db, err := rocksdb.New(dataDir) return newStorageStore(db), err }, // ... other backends }store/v2/storage/store.go (1)
87-90: New methodVersionExistslooks goodThe new
VersionExistsmethod is a valuable addition to theStorageStorestruct. It correctly delegates the version existence check to the underlying database.Consider adding a brief comment to explain the purpose of this method:
// VersionExists checks if the given version exists in the underlying database. // It returns true if the version exists, false otherwise, and any error encountered during the check. func (ss *StorageStore) VersionExists(version uint64) (bool, error) { return ss.db.VersionExists(version) }This comment would provide clarity on the method's functionality and return values.
store/v2/storage/rocksdb/db.go (1)
134-141: LGTM with a minor suggestion for improved clarity.The
VersionExistsmethod is well-implemented and follows the Uber Go Style Guide. It correctly checks if the given version is within the valid range.For improved clarity, consider adding a comment explaining the logic:
func (db *Database) VersionExists(version uint64) (bool, error) { latestVersion, err := db.GetLatestVersion() if err != nil { return false, err } // A version exists if it's between the earliest supported version (tsLow) // and the latest version, inclusive. return latestVersion >= version && version >= db.tsLow, nil }This comment helps future maintainers understand the version existence criteria at a glance.
store/v2/storage/sqlite/db.go (1)
130-137: LGTM: New VersionExists methodThe new
VersionExistsmethod correctly implements the version existence check, considering both the latest and earliest versions. It follows Go naming conventions and the Uber Go Style Guide.Suggestion: Consider adding a comment above the method to explain its purpose and behavior, enhancing code documentation.
Here's a suggested comment:
// VersionExists checks if the given version exists within the valid range of versions in the database. // It returns true if the version is between the earliest version and the latest version (inclusive), // and false otherwise. An error is returned if the latest version cannot be retrieved.store/v2/commitment/store.go (1)
Line range hint
278-290: LGTM! Consider adding a comment forconv.UnsafeBytesToStr.The
Getmethod implementation looks good. It correctly handles the case when the store is not found and provides informative error messages. The error handling follows best practices by wrapping errors with additional context.Consider adding a comment explaining the safety of using
conv.UnsafeBytesToStrfor convertingstoreKeyfrom[]bytetostring. This will help other developers understand why this conversion is safe in this context.store/v2/storage/pebbledb/db.go (1)
140-147: LGTM: New methodVersionExistslooks good.The implementation of
VersionExistsis correct and follows good practices:
- It properly handles potential errors from
GetLatestVersion.- The logic for checking version existence is correct, comparing against both the latest and earliest versions.
- The method signature adheres to Go conventions, returning a bool and an error.
One minor suggestion for improvement:
- Consider adding a comment explaining the method's purpose and behavior, especially regarding the earliest version check.
You could add a comment like this above the method:
// VersionExists checks if the given version exists in the database. // It returns true if the version is between the earliest and latest versions, inclusive.store/v2/root/store_test.go (2)
Line range hint
1-1010: Consider adding a specific test case for VersionedWriter.The test suite is comprehensive and covers various aspects of the RootStore functionality. However, given the change in the
newStoreWithBackendMountmethod signature fromVersionedDatabasetoVersionedWriter, it might be beneficial to add a specific test case that verifies the behavior of the RootStore with the newVersionedWriterinterface.This new test case could focus on the specific capabilities of the
VersionedWriterinterface and ensure that the RootStore correctly interacts with it. This would help validate the recent change and provide additional coverage for this specific interface.
Potential infinite recursion detected in
newStoreWithBackendMountmethod.
- The method calls itself multiple times without an apparent termination condition.
Please review the implementation to ensure that recursion is intentional and properly handled to prevent infinite loops.
🔗 Analysis chain
Line range hint
93-101: LGTM. Verify impact on test suite.The change from
store.VersionedDatabasetostore.VersionedWriterin the method signature is appropriate, as it likely represents a more specific interface requirement. This change is consistent with the provided summary.To ensure this change doesn't negatively impact the test suite, please run the following command to check all occurrences of
newStoreWithBackendMount:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check all occurrences of newStoreWithBackendMount to ensure compatibility with the new signature. rg --type go "newStoreWithBackendMount" store/v2/root/store_test.goLength of output: 350
Script:
#!/bin/bash # Description: Find all calls to newStoreWithBackendMount across the codebase to verify signature compatibility. rg --type go "newStoreWithBackendMount"Length of output: 462
store/v2/storage/storage_test_suite.go (1)
883-1021: LGTM! Well-structured and comprehensive test cases.The
TestVersionExistsfunction is well-implemented with a good variety of test cases covering different scenarios. The table-driven test approach is excellent for maintainability and readability.To further improve readability, consider grouping related test cases together and adding comments to separate these groups. For example:
// Fresh database scenarios { name: "Fresh database: version 0 exists", // ... }, { name: "Fresh database: version 1 exists", // ... }, // After setting latest version scenarios { name: "After setting latest version to 10, version 5 exists", // ... }, // After pruning scenarios { name: "After setting latest version to 10 and pruning to 5, version 4 does not exist", // ... }, // ...This grouping can make it easier to understand the different scenarios being tested at a glance.
store/v2/root/store.go (1)
126-126: Rename variable 'isExist' to 'exists' for readabilityConsider renaming the variable
isExisttoexiststo improve readability and adhere to Go naming conventions.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (17)
- runtime/v2/store.go (1 hunks)
- store/v2/commitment/store.go (1 hunks)
- store/v2/database.go (2 hunks)
- store/v2/mock/db_mock.go (1 hunks)
- store/v2/mock/types.go (1 hunks)
- store/v2/root/store.go (3 hunks)
- store/v2/root/store_mock_test.go (1 hunks)
- store/v2/root/store_test.go (1 hunks)
- store/v2/storage/database.go (1 hunks)
- store/v2/storage/pebbledb/db.go (1 hunks)
- store/v2/storage/rocksdb/db.go (1 hunks)
- store/v2/storage/rocksdb/db_noflag.go (1 hunks)
- store/v2/storage/sqlite/db.go (2 hunks)
- store/v2/storage/storage_bench_test.go (1 hunks)
- store/v2/storage/storage_test_suite.go (1 hunks)
- store/v2/storage/store.go (2 hunks)
- store/v2/store.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (17)
runtime/v2/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/commitment/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/database.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/mock/db_mock.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/root/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/root/store_mock_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"store/v2/root/store_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"store/v2/storage/database.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/storage/pebbledb/db.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/storage/rocksdb/db.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/storage/rocksdb/db_noflag.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/storage/sqlite/db.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/storage/storage_bench_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"store/v2/storage/storage_test_suite.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/storage/store.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 (13)
store/v2/storage/rocksdb/db_noflag.go (1)
Line range hint
1-71: Overall assessment: Changes are minimal and consistent.The addition of the
VersionExistsmethod to theDatabasestruct is consistent with the existing code structure and follows the established pattern for methods in this file. The new method maintains the same behavior as other methods when RocksDB is not enabled, which is to panic with an appropriate message.The changes adhere to the Uber Golang style guide and do not introduce any new issues or inconsistencies. The file's overall structure and purpose remain unchanged.
store/v2/storage/storage_bench_test.go (1)
Line range hint
1-193: Overall assessment: Changes are consistent and well-implemented.The modification from
store.VersionedDatabasetostore.VersionedWriterin thebackendsmap is consistent with the PR objectives. The existing benchmarks (BenchmarkGet, BenchmarkApplyChangeset, BenchmarkIterate) provide sufficient coverage for the core functionalities of the newVersionedWriterinterface.No additional tests seem necessary for this specific change, as the existing benchmarks cover the main operations (Get, ApplyChangeset, Iterate) that are likely part of both the old and new interfaces.
The code adheres to the Uber Golang style guide, and the changes don't introduce any apparent issues or inconsistencies.
store/v2/storage/store.go (1)
19-19: Interface implementation change:VersionedDatabasetoVersionedWriterThe type assertion has been updated to implement
store.VersionedWriterinstead ofstore.VersionedDatabase. This change suggests a shift in the expected functionality ofStorageStore, focusing more on writing capabilities.To ensure this change doesn't break existing functionality, please run the following script to check for any remaining usages of
StorageStoreas aVersionedDatabase:Also, verify that all necessary methods of
VersionedWriterare implemented:Compare the output of these two commands to ensure all required methods are implemented.
✅ Verification successful
Verified: Type assertion change to
VersionedWriterAll usages of
StorageStoreasVersionedDatabasehave been removed, andStorageStorecorrectly implements all required methods of theVersionedWriterinterface.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usages of StorageStore as VersionedDatabase rg --type go 'var\s+_\s+store\.VersionedDatabase\s+=\s+\(\*StorageStore\)\(nil\)'Length of output: 83
Script:
#!/bin/bash # List all methods of VersionedWriter interface ast-grep --lang go --pattern 'type VersionedWriter interface { $$$ }' # List all methods of StorageStore ast-grep --lang go --pattern 'type StorageStore struct { $$$ } $$$ func (ss *StorageStore) $_($_) $_ { $$$ }'Length of output: 1238
Script:
#!/bin/bash # List all methods of StorageStore using ast-grep ast-grep --lang go --pattern 'func (ss *StorageStore) $_($_) $_ { $$$ }'Length of output: 639
Script:
#!/bin/bash # List all methods of StorageStore using ripgrep rg --type go 'func \(ss \*StorageStore\) ' --context 1Length of output: 3154
store/v2/root/store_mock_test.go (3)
Line range hint
19-30: LGTM. Consider verifying interface implementations and updating documentation.The change from
store.VersionedDatabasetostore.VersionedWriterlooks good. It reflects a shift in the expected interface for state storage.Please verify that
store.VersionedWriterimplements all necessary interfaces, particularlystore.Pruner, as it's still being used in type assertions.If there's a function comment for
newTestRootStore(not visible in the provided code), please update it to reflect the new parameter type.
Line range hint
32-186: LGTM. Consider verifying test coverage with the new interface.The test functions remain unchanged and continue to use the updated
newTestRootStorefunction, which is good. This suggests that the interface change fromVersionedDatabasetoVersionedWriterdoesn't affect the tested functionality.Please verify that the existing tests still provide adequate coverage for all scenarios relevant to the
VersionedWriterinterface. Consider adding new test cases if there are any new behaviors or edge cases introduced by this interface change.
Line range hint
1-186: Overall, the changes and existing code look good.The modification to use
store.VersionedWriterinstead ofstore.VersionedDatabaseinnewTestRootStoreis the main change in this file. The existing tests remain unchanged and continue to use the updated function, which is positive.Key points:
- The code adheres to the Uber Golang style guide.
- The tests appear to provide good coverage of the
Storestruct's methods.To further improve the code:
- Ensure that the
VersionedWriterinterface is fully tested, including any new behaviors it might introduce.- Update any relevant documentation (function comments, package documentation) to reflect the interface change.
- Consider adding a brief comment explaining the rationale behind the interface change from
VersionedDatabasetoVersionedWriter, if not already documented elsewhere.store/v2/storage/sqlite/db.go (1)
106-110: LGTM: Improved SQL query readabilityThe SQL query has been reformatted from a single line to multiple lines, which significantly improves readability without changing the functionality. This change aligns with best practices for SQL formatting and the Uber Go Style Guide's emphasis on code readability.
store/v2/mock/db_mock.go (2)
408-415: LGTM: VersionExists method implementationThe
VersionExistsmethod is correctly implemented for theMockStateStoragestruct. It follows the established pattern for mock methods in this file, using the gomock controller to record the method call and return mocked values.
417-421: LGTM: VersionExists recorder method implementationThe
VersionExistsrecorder method is correctly implemented for theMockStateStorageMockRecorderstruct. It follows the established pattern for mock recorder methods in this file, using the gomock controller to record the method call with the correct method type.store/v2/root/store_test.go (1)
Line range hint
1-1010: Overall, the test suite looks robust and well-maintained.The change in the
newStoreWithBackendMountmethod signature fromVersionedDatabasetoVersionedWriteris the main modification in this file. The existing test cases cover a wide range of scenarios and appear to be comprehensive.To further improve the test suite:
- Verify that all usages of
newStoreWithBackendMountare compatible with the new signature.- Consider adding a specific test case for the
VersionedWriterinterface to ensure proper coverage of the recent change.These additions would enhance the robustness of the test suite and provide better validation for the
VersionedWriterfunctionality within the RootStore implementation.store/v2/root/store.go (3)
37-37: Updated 'stateStorage' field type to 'VersionedWriter'The change of the
stateStoragefield type tostore.VersionedWriterreflects the updated interface and aligns with the modifications in the codebase.
72-72: Updated parameter 'ss' in 'New' function to 'VersionedWriter'The
Newfunction now acceptsstore.VersionedWriteras thessparameter, which is consistent with the updatedstateStoragefield type.
137-137: Updated return type for 'GetStateStorage' methodThe
GetStateStoragemethod now returnsstore.VersionedWriter, matching the updated type of thestateStoragefield.
| @@ -70,7 +70,7 @@ type RootStore interface { | |||
| // Backend defines the interface for the RootStore backends. | |||
| type Backend interface { | |||
| // GetStateStorage returns the SS backend. | |||
| GetStateStorage() VersionedDatabase | |||
| GetStateStorage() VersionedWriter | |||
There was a problem hiding this comment.
💡 Codebase verification
Issues Found: Inconsistent Return Types for GetStateStorage()
The change from VersionedDatabase to VersionedWriter in the GetStateStorage() method affects the following implementations:
server/v2/cometbft/internal/mock/mock_store.go:86:MockStorestill returnsstorev2.VersionedDatabase.runtime/v2/store.go:46:GetStateStorage()returnsstorev2.VersionedWriter, which might conflict with existing mock implementations.
Ensure all implementations of the Backend interface are updated to use VersionedWriter to maintain consistency and prevent potential build or runtime errors.
🔗 Analysis chain
LGTM! Verify impact on dependent code.
The change from VersionedDatabase to VersionedWriter in the GetStateStorage() method signature looks good. This modification aligns with the PR objectives of enhancing the state machine functionality.
To ensure this change doesn't introduce breaking changes, please run the following script to check for any occurrences of GetStateStorage() that might be affected:
Please review the results to ensure that all occurrences are updated accordingly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for occurrences of GetStateStorage() that might be affected by the interface change.
# Search for GetStateStorage() method calls
echo "Searching for GetStateStorage() method calls:"
rg --type go -n 'GetStateStorage\(\)'
# Search for VersionedDatabase type usage
echo "Searching for VersionedDatabase type usage:"
rg --type go -n 'VersionedDatabase'
# Search for VersionedWriter type usage
echo "Searching for VersionedWriter type usage:"
rg --type go -n 'VersionedWriter'
Length of output: 4583
| @@ -43,7 +43,7 @@ type Store interface { | |||
| Query(storeKey []byte, version uint64, key []byte, prove bool) (storev2.QueryResult, error) | |||
|
|
|||
| // GetStateStorage returns the SS backend. | |||
| GetStateStorage() storev2.VersionedDatabase | |||
| GetStateStorage() storev2.VersionedWriter | |||
There was a problem hiding this comment.
💡 Codebase verification
Issues Found: Update Remaining VersionedDatabase References
The VersionedDatabase type is still referenced in multiple files:
store/v2/storage/store.gostore/v2/mock/types.goserver/v2/cometbft/internal/mock/mock_store.go
Please update these references to VersionedWriter to ensure consistency across the codebase.
🔗 Analysis chain
LGTM. Verify impact and update documentation if needed.
The change from storev2.VersionedDatabase to storev2.VersionedWriter for the GetStateStorage method return type looks good. This modification aligns with the PR objectives of enhancing the state machine functionality.
To ensure this change doesn't introduce any inconsistencies, please run the following script to check for any other occurrences of VersionedDatabase that might need updating:
Consider updating any relevant documentation or comments related to the GetStateStorage method to reflect this change in return type, if necessary.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other occurrences of VersionedDatabase that might need updating
# Search for VersionedDatabase usage
echo "Searching for VersionedDatabase usage:"
rg --type go "VersionedDatabase"
# Search for GetStateStorage method calls
echo "Searching for GetStateStorage method calls:"
ast-grep --lang go --pattern 'GetStateStorage()'
Length of output: 897
| func dbApplyChangeset( | ||
| t *testing.T, | ||
| db store.VersionedDatabase, | ||
| db store.VersionedWriter, |
There was a problem hiding this comment.
💡 Codebase verification
Additional occurrences of VersionedDatabase found.
To maintain consistency across the codebase, please update the following instances from VersionedDatabase to VersionedWriter:
store/v2/storage/store.gostore/v2/mock/types.goserver/v2/cometbft/internal/mock/mock_store.go
🔗 Analysis chain
LGTM! Interface update is consistent with the codebase changes.
The change from store.VersionedDatabase to store.VersionedWriter in the dbApplyChangeset function signature is appropriate and aligns with the transition mentioned in the summary.
To ensure consistency across the codebase, let's verify if there are any other occurrences of VersionedDatabase that might need updating:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining occurrences of VersionedDatabase
rg --type go "VersionedDatabase"
Length of output: 662
| SetLatestVersion(version uint64) error | ||
| ApplyChangeset(version uint64, cs *corestore.Changeset) error |
There was a problem hiding this comment.
Add missing documentation for exported methods SetLatestVersion and ApplyChangeset
The methods SetLatestVersion and ApplyChangeset are exported but lack doc comments. According to the Uber Go Style Guide, all exported functions and methods should have comments that begin with the method name and clearly describe their purpose. Please add doc comments to improve code readability and maintainability.
| GetLatestVersion() (uint64, error) | ||
| SetLatestVersion(version uint64) error | ||
| VersionExists(v uint64) (bool, error) |
There was a problem hiding this comment.
Use descriptive parameter name and add documentation for VersionExists method
The parameter v in VersionExists is not descriptive. For clarity and consistency, consider renaming it to version. Additionally, the method lacks a doc comment. As per the Uber Go Style Guide, please add a comment starting with the method name to explain its functionality.
| // SetInitialVersion sets the initial version of the committer. | ||
| SetInitialVersion(version uint64) error | ||
|
|
||
| // GetCommitInfo returns the CommitInfo for the given version. | ||
| GetCommitInfo(version uint64) (*proof.CommitInfo, error) | ||
|
|
||
| Get(storeKey []byte, version uint64, key []byte) ([]byte, error) |
There was a problem hiding this comment.
Add missing documentation for exported method Get
The method Get in the Committer interface is exported but doesn't have a doc comment. To comply with the Uber Go Style Guide, please provide a comment starting with the method name to describe its behavior and purpose.
| @@ -120,19 +120,21 @@ func (s *Store) StateLatest() (uint64, corestore.ReaderMap, error) { | |||
| return v, NewReaderMap(v, s), nil | |||
| } | |||
|
|
|||
| // StateAt checks if the requested version is present in ss. | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider renaming 'StateAt' method for clarity
The method name StateAt may not clearly convey its functionality. Consider renaming it to GetStateAtVersion or GetReaderMapAtVersion to better reflect that it retrieves a ReaderMap for a specific version.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
store/v2/root/store.go (2)
130-130: Refine the comment forStateAtto accurately describe its functionalityThe current comment does not fully convey the method's purpose. Consider updating it to reflect that
StateAtreturns aReaderMapfor the specified version if it exists.Suggested change:
-// StateAt checks if the requested version is present in ss. +// StateAt returns a ReaderMap for the specified version if it exists in the state storage.
133-133: RenameisExisttoexistsfor idiomatic Go namingBoolean variables are typically named using affirmative terms. Renaming
isExisttoexistsimproves readability and follows Go conventions.Apply this diff:
-isExist, err := s.stateStorage.VersionExists(v) +exists, err := s.stateStorage.VersionExists(v)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- store/v2/root/store.go (3 hunks)
- store/v2/root/store_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- store/v2/root/store_test.go
🧰 Additional context used
📓 Path-based instructions (1)
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 (3)
store/v2/root/store.go (3)
41-41: Change ofstateStoragetype toVersionedWriteris appropriateThe field
stateStorageis updated tostore.VersionedWriter, which aligns with its usage in the codebase.
77-77: Parameter type change inNewfunction matches updated field typeThe constructor now accepts
ss store.VersionedWriter, reflecting the updated type ofstateStorage.
144-145: Return type update inGetStateStoragealigns with field type changeThe method now returns
store.VersionedWriter, which is consistent with the updatedstateStoragefield.
Description
ref: #22243
this pr adds version exists in order to make stateat work for the state machine.
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
StateAtadded to check for the existence of a requested version in the state storage.Improvements
Bug Fixes