feat(store): add new api LatestVersion #22305
Conversation
cool-develope
requested review from
kocubinski,
a team,
facundomedica,
aaronc and
testinginprod
as code owners
📝 WalkthroughWalkthroughThe pull request introduces modifications primarily to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
store/rootmulti/store.go (1)
Line range hint
443-461: LGTM: Enhanced LastCommitID method with minor optimization suggestionThe changes to the
LastCommitIDmethod improve its robustness by properly handling cases wherelastCommitInfois nil or its CommitID hash is empty. This ensures that a validCommitIDstructure is always returned.A minor optimization could be made by pre-computing the empty hash:
var emptyHash = sha256.Sum256([]byte{}) func (rs *Store) LastCommitID() types.CommitID { // ... existing code ... appHash := emptyHash[:] // ... rest of the code ... }Also, consider adding a comment explaining why we're using an empty SHA256 hash in these cases for better code clarity.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (8)
- baseapp/baseapp.go (1 hunks)
- store/CHANGELOG.md (1 hunks)
- store/iavl/store.go (1 hunks)
- store/mem/store.go (1 hunks)
- store/rootmulti/dbadapter.go (1 hunks)
- store/rootmulti/store.go (1 hunks)
- store/transient/store.go (1 hunks)
- store/types/store.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
baseapp/baseapp.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"store/iavl/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/mem/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/rootmulti/dbadapter.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/rootmulti/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/transient/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/types/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (10)
store/rootmulti/dbadapter.go (1)
Line range hint
1-55: Overall structure and consistency look goodThe new
LatestVersionmethod fits well with the existing structure of thecommitDBStoreAdapter. The file maintains consistency in terms of naming conventions and code organization, adhering to the Uber Go Style Guide.store/transient/store.go (2)
Line range hint
1-58: Overall, the changes look good and align with the PR objectives.The addition of the
LatestVersionmethod to theStorestruct completes the implementation of theCommitterinterface. This change is in line with the PR objectives to improve version management and potentially address performance issues.The existing code remains unchanged, maintaining the overall structure and functionality of the
Storeclass as a wrapper for a MemDB with Committer implementation.
45-48: LGTM. Consider adding a comment explaining the rationale.The implementation of
LatestVersionis correct and follows the Golang style guide. However, it might be helpful to add a comment explaining why it always returns 0 for a transient store.Consider adding a comment like:
// LatestVersion implements Committer. It always returns 0 for a transient store // as it doesn't persist data across commits.Let's verify if there are any callers that might expect a non-zero version:
✅ Verification successful
Add a comment explaining why
LatestVersionalways returns 0The
LatestVersionmethod correctly always returns 0 for transient stores, as expected. To improve code clarity, consider adding a comment explaining this design choice.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to LatestVersion() on Store or Committer types rg --type go 'LatestVersion\(\)' -C 3Length of output: 68860
store/CHANGELOG.md (1)
28-30: LGTM: Changelog entry is well-formatted and accurate.The new changelog entry under the "Improvements" section accurately reflects the changes introduced in this PR. It follows the guidelines by including the module name, PR link, and a clear description of the improvement.
store/iavl/store.go (1)
149-152: LGTM: New method aligns with PR objectivesThe new
LatestVersionmethod is a well-implemented addition to theStorestruct. It provides a direct way to retrieve the latest version of the IAVL tree, which aligns with the PR objectives of improving performance. The implementation is simple and efficient, leveraging the existingVersion()method of the tree.store/types/store.go (3)
Line range hint
191-193: LGTM: Consistent addition to MultiStore interfaceThe addition of
LatestVersion() int64to theMultiStoreinterface is consistent with the changes made to theCommitterinterface and aligns with the PR objectives.Consider enhancing the comment to provide more context:
- // LatestVersion returns the latest version in the store + // LatestVersion returns the latest version (height) of the store. + // This method is added to improve performance by avoiding unnecessary computations.Ensure that all implementations of the
MultiStoreinterface are updated to include this new method. Run the following command to find potential places that need updates:#!/bin/bash # Find all files that might implement the MultiStore interface rg -t go "type \w+ struct" | rg -v "_test.go" | xargs -I {} rg -l "func \(\w+ \*?\w+\) (CacheMultiStore|GetStore|GetKVStore|TracingEnabled|SetTracer|SetTracingContext)" {} | xargs -I {} echo "Check file: {}"
24-24: Overall impact: Focused changes align with performance improvement goalsThe additions of the
LatestVersion() int64method to both theCommitterandMultiStoreinterfaces are well-contained and consistent. These changes align with the PR objectives of improving performance related to version retrieval without introducing breaking changes to the existing package structure.Consider the following to ensure smooth integration of these changes:
- Update all implementations of
CommitterandMultiStoreinterfaces to include the newLatestVersion()method.- Ensure that the new method is used in place of any existing inefficient version retrieval operations throughout the codebase.
- Add unit tests to verify the correct implementation and usage of the new
LatestVersion()method.- Update relevant documentation to reflect these changes and their performance implications.
To ensure comprehensive coverage of these changes, run the following command to find all files that might need updates or additional tests:
#!/bin/bash # Find all files that might be affected by the new LatestVersion method rg -t go "(Committer|MultiStore)" | rg -v "_test.go" | cut -d: -f1 | sort -u | xargs -I {} echo "Check file for updates and tests: {}"Also applies to: 191-193
24-24: LGTM: New method aligns with performance improvement goalsThe addition of
LatestVersion() int64to theCommitterinterface is a good step towards addressing the performance issues mentioned in the PR objectives. This method will allow for efficient retrieval of the latest version without unnecessary computations.Ensure that all implementations of the
Committerinterface are updated to include this new method. Run the following command to find potential places that need updates:store/rootmulti/store.go (1)
437-441: LGTM: Improved robustness in LatestVersion methodThe changes to the
LatestVersionmethod enhance its robustness by properly handling cases wherelastCommitInfois nil. This is a good improvement that ensures the method always returns a valid version, even when commit info hasn't been initialized yet.baseapp/baseapp.go (1)
400-400: Approve the performance improvement in LastBlockHeight method.The change from
app.cms.LastCommitID().Versiontoapp.cms.LatestVersion()addresses the performance issue mentioned in the PR objectives. This direct retrieval of the latest version from the commit multi-store should be more efficient.To ensure this change doesn't have unintended consequences, please run the following verification:
| func (cdsa commitDBStoreAdapter) LatestVersion() int64 { | ||
| return -1 | ||
| } |
There was a problem hiding this comment.
Consider revising the LatestVersion implementation
The current implementation of LatestVersion always returns -1, which may not accurately represent the latest version in all scenarios. This could lead to unexpected behavior or errors in code that relies on this method.
Consider the following suggestions:
- If -1 is intended to represent an uninitialized state, it might be better to return an error along with the version to indicate this special case.
- If possible, implement logic to return the actual latest version of the store.
- Add a comment explaining the rationale behind returning -1 if this is the intended behavior.
Here's a potential improvement:
func (cdsa commitDBStoreAdapter) LatestVersion() (int64, error) {
// TODO: Implement logic to get the actual latest version
return -1, errors.New("latest version not available")
}This change would require updating the interface and other implementations as well.
| @@ -59,4 +59,6 @@ func (s *Store) GetPruning() pruningtypes.PruningOptions { | |||
|
|
|||
| func (s Store) LastCommitID() (id types.CommitID) { return } | |||
|
|
|||
| func (s Store) LatestVersion() (version int64) { return } | |||
There was a problem hiding this comment.
💡 Codebase verification
Standardize LatestVersion() Method Across Stores
The LatestVersion() method in store/mem/store.go currently returns an int64, whereas other store implementations return uint64. This inconsistency can lead to type mismatches and potential bugs.
To ensure consistency and prevent type-related issues:
- Standardize the Return Type: Decide on a single return type (
int64oruint64) for theLatestVersion()method across all store implementations. - Implement the Method Correctly: Update the
LatestVersion()method instore/mem/store.goto return the actual latest version instead of the zero value. - Review Method Naming: Ensure that all version-related methods (
GetLatestVersion,LoadLatestVersion, etc.) are clearly defined and consistently named to reflect their functionality.
// LatestVersion returns the latest version of the store.
// This method is introduced to improve performance by avoiding
// repeated hash computations in GetLastBlockHeight.
func (s Store) LatestVersion() (version uint64) {
// TODO: Implement the logic to return the latest version
return 0
}🔗 Analysis chain
Implement the LatestVersion() method
The LatestVersion() method has been added as per the PR objectives, but it currently has an empty implementation. This doesn't fulfill the intended purpose of returning the latest version of the store.
Please implement the method to return the actual latest version. Consider the following:
- How is the version information stored or tracked in the
Storestruct? - Should this method read from a field, or calculate the version on-the-fly?
Also, add a comment explaining the purpose of this method. For example:
// LatestVersion returns the latest version of the store.
// This method is introduced to improve performance by avoiding
// repeated hash computations in GetLastBlockHeight.
func (s Store) LatestVersion() (version int64) {
// TODO: Implement the logic to return the latest version
return 0
}To ensure this method is used correctly throughout the codebase, let's check its usage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of LatestVersion method on Store
rg --type go 'Store.*LatestVersion\(\)'
Length of output: 3891
|
@julienrbrt need to backport to v0.50 and v0.52 |
Store v1 is tagged from main so we are fine 👍 |
|
TYSM!!! |
Description
Closes: #22259
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
LatestVersionmethod across various store structures, enhancing version management capabilities.LastBlockHeightmethod to reflect the most current application state.Improvements
LatestVersionmethod and previous version details.Bug Fixes