feat(store/v2): route to the commitment during migration #22290
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request enhance 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
|
|
@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: 4
🧹 Outside diff range and nitpick comments (7)
store/v2/root/store.go (2)
121-140: Excellent addition ofgetVersionedReadermethodThe new
getVersionedReadermethod effectively encapsulates the logic for retrieving a versioned reader from either state storage or state commitment. This improves code organization and reusability.Consider combining the error checks for both state storage and state commitment to provide a more informative error message. For example:
if err != nil { - return nil, err + return nil, fmt.Errorf("failed to check version existence: %w", err) }This change would provide more context in case of an error.
157-161: Effective refactoring ofStateAtmethodThe
StateAtmethod has been successfully refactored to use the newgetVersionedReadermethod, which simplifies the code and improves consistency withStateLatest.Consider improving error handling by wrapping the error returned from
getVersionedReader. This would provide more context about where the error occurred:func (s *Store) StateAt(v uint64) (corestore.ReaderMap, error) { vReader, err := s.getVersionedReader(v) - return NewReaderMap(v, vReader), err + if err != nil { + return nil, fmt.Errorf("failed to get versioned reader for version %d: %w", v, err) + } + return NewReaderMap(v, vReader), nil }This change would make debugging easier by providing more information about the error's origin.
store/v2/commitment/tree.go (2)
40-41: Clarify the comment for theReaderinterface.To adhere to Go documentation conventions, the comment can be more concise and start with the interface name. Consider rephrasing it as:
// Reader is an optional interface used to read data from the tree during the migration process.
43-44: Add comments to exported methods in theReaderinterface.According to the Go style guidelines, all exported methods should have comments. Please add comments to
GetandIteratormethods to improve code documentation.Example:
// Get retrieves the value for a given version and key. Get(version uint64, key []byte) ([]byte, error) // Iterator returns an iterator for a given version and key range. Iterator(version uint64, start, end []byte, ascending bool) (corestore.Iterator, error)store/v2/commitment/iavl/tree.go (2)
80-80: Improve function comments to adhere to GoDoc conventionsThe comments for the
GetandIteratormethods currently state that they implement theReaderinterface. To enhance clarity and conform to GoDoc conventions, consider updating the comments to describe the functionality of each method, starting with the method name.Apply the following changes:
-// Get implements the Reader interface. +// Get retrieves the value associated with the specified key at the given version. ... -// Iterator implements the Reader interface. +// Iterator returns an iterator over a range of keys for a specified version.Also applies to: 90-91
Line range hint
80-87: Potential issue with castingversionfromuint64toint64In both the
GetandIteratormethods, theversionparameter is cast fromuint64toint64when callingGetImmutable:immutableTree, err := t.tree.GetImmutable(int64(version))If
versionexceeds the maximum value ofint64, this casting could lead to incorrect behavior due to integer overflow.Consider adding a validation check to ensure that
versiondoes not exceedmath.MaxInt64before casting. For example:if version > math.MaxInt64 { return nil, fmt.Errorf("version %d is too large", version) }Alternatively, evaluate whether the underlying
iavlmethods can be updated to acceptuint64to avoid this issue.Also applies to: 90-98
store/v2/commitment/store.go (1)
302-309: Ensure proper formatting of byte slices in error messagesUsing
%sto format byte slices in error messages may not display the keys correctly if they contain non-printable characters. Consider formatting thekeyandstoreKeyusing%Xfor hexadecimal representation or%qfor a quoted string.Suggested change:
- return nil, fmt.Errorf("failed to get key %s from store %s: %w", key, storeKey, err) + return nil, fmt.Errorf("failed to get key %X from store %s: %w", key, storeKey, err)Or, if safe to convert to string:
- return nil, fmt.Errorf("failed to get key %s from store %s: %w", key, storeKey, err) + return nil, fmt.Errorf("failed to get key %s from store %s: %w", string(key), storeKey, err)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (6)
- store/v2/commitment/iavl/tree.go (3 hunks)
- store/v2/commitment/store.go (2 hunks)
- store/v2/commitment/tree.go (2 hunks)
- store/v2/database.go (0 hunks)
- store/v2/root/reader.go (2 hunks)
- store/v2/root/store.go (1 hunks)
💤 Files with no reviewable changes (1)
- store/v2/database.go
🧰 Additional context used
📓 Path-based instructions (5)
store/v2/commitment/iavl/tree.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/commitment/tree.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/root/reader.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.
🔇 Additional comments (14)
store/v2/root/store.go (1)
143-155: Well-executed refactoring ofStateLatestmethodThe refactoring of the
StateLatestmethod improves code clarity and maintainability. By utilizing the newgetVersionedReadermethod, it reduces code duplication and centralizes the logic for retrieving versioned readers.The error handling is concise and appropriate. Good job on returning early in case of errors, which improves readability.
store/v2/commitment/tree.go (1)
9-9: Import statements are correctly added.The new import statements for
corestoreandsnapshotstypesare necessary and properly formatted.store/v2/root/reader.go (3)
17-18: Field alignment and documentation look good.The struct fields in
ReaderMapandReaderare well-defined with clear comments, adhering to Go documentation standards and promoting code readability.Also applies to: 34-36
48-50: Efficient error handling inHasmethod.The
Hasmethod correctly handles errors returned fromvReader.Has, ensuring that any encountered error is promptly returned.
57-57: Methods correctly delegate tovReader.The
Get,Iterator, andReverseIteratormethods properly call the corresponding methods onvReader, maintaining the intended functionality.Also applies to: 61-61, 65-65
store/v2/commitment/iavl/tree.go (3)
17-17: Confirming implementation ofcommitment.Readerinterface
The addition ofvar _ commitment.Reader = (*IavlTree)(nil)correctly asserts thatIavlTreeimplements thecommitment.Readerinterface.
Line range hint
80-87:Getmethod logic and error handling are correctThe
Getmethod appropriately retrieves the immutable tree at the specified version and returns the value associated with the key. The error handling is properly implemented, and the method aligns with the expected functionality of thecommitment.Readerinterface.
90-98:Iteratormethod logic and error handling are appropriateThe
Iteratormethod correctly retrieves the immutable tree at the specified version and returns an iterator over the specified key range. Error handling is properly managed, ensuring that any issues in retrieving the immutable tree are surfaced.store/v2/commitment/store.go (6)
28-31: Verification ofstore.VersionedReaderimplementationThe
CommitStorecorrectly implements thestore.VersionedReaderinterface, and the accompanying comment accurately explains its usage during the migration process.
282-294:getReadermethod retrievesReaderinterface appropriatelyThe
getReadermethod efficiently fetches the tree associated with the givenstoreKeyand asserts that it implements theReaderinterface. Error handling is properly implemented.
296-301:VersionExistsmethod correctly checks version existenceThe
VersionExistsmethod accurately determines if a given version exists in the metadata by checking if the commit info is not nil.
317-321:Hasmethod correctly determines key presenceThe
Hasmethod appropriately checks for the existence of a key by utilizing theGetmethod and evaluating the length of the returned value.
323-331:Iteratormethod implemented correctlyThe
Iteratormethod correctly obtains an iterator over the key range for the specified version, ensuring forward iteration by passingtruefor the direction.
333-341:ReverseIteratormethod implemented correctlyThe
ReverseIteratormethod correctly obtains a reverse iterator over the key range for the specified version by passingfalsefor the direction.
| // Reader is the optional interface that is only used to read data from the tree | ||
| // during the migration process. | ||
| type Reader interface { | ||
| Get(version uint64, key []byte) ([]byte, error) | ||
| Iterator(version uint64, start, end []byte, ascending bool) (corestore.Iterator, error) | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider the impact of removing Get from Tree interface on existing implementations.
By removing the Get method from the Tree interface and introducing the Reader interface, existing code that relies on Tree.Get may break. Ensure that all implementations and usages of Tree are updated to reflect this change, and consider providing guidance or deprecation notices to assist with the migration.
| func NewReaderMap(v uint64, vr store.VersionedReader) *ReaderMap { | ||
| return &ReaderMap{ | ||
| rootStore: rs, | ||
| version: v, | ||
| vReader: vr, | ||
| version: v, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use descriptive parameter names for clarity.
In the constructors NewReaderMap and NewReader, the parameters v and vr are abbreviated and may reduce readability. According to the Uber Go Style Guide, parameter names should be self-explanatory. Consider renaming v to version and vr to versionedReader for better clarity.
Apply this diff to update the parameter names:
-func NewReaderMap(v uint64, vr store.VersionedReader) *ReaderMap {
+func NewReaderMap(version uint64, versionedReader store.VersionedReader) *ReaderMap {
return &ReaderMap{
- vReader: vr,
- version: v,
+ vReader: versionedReader,
+ version: version,
}
}
-func NewReader(v uint64, vr store.VersionedReader, actor []byte) *Reader {
+func NewReader(version uint64, versionedReader store.VersionedReader, actor []byte) *Reader {
return &Reader{
- version: v,
- vReader: vr,
+ version: version,
+ vReader: versionedReader,
actor: actor,
}
}Also applies to: 39-43
store/v2/root/reader.go
Outdated
|
|
||
| func (roa *ReaderMap) GetReader(actor []byte) (corestore.Reader, error) { | ||
| return NewReader(roa.version, roa.rootStore, actor), nil | ||
| return NewReader(roa.version, roa.vReader, actor), nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use conventional receiver names for consistency.
The receiver name roa is used for both ReaderMap and Reader methods, which can be confusing. As per Go conventions and the Uber Go Style Guide, receiver names should be a concise abbreviation of the type name. Consider using rm for ReaderMap and r for Reader to enhance code readability.
Apply this diff to update the receiver names:
-func (roa *ReaderMap) GetReader(actor []byte) (corestore.Reader, error) {
+func (rm *ReaderMap) GetReader(actor []byte) (corestore.Reader, error) {
- return NewReader(roa.version, roa.vReader, actor), nil
+ return NewReader(rm.version, rm.vReader, actor), nil
}
-func (roa *Reader) Has(key []byte) (bool, error) {
+func (r *Reader) Has(key []byte) (bool, error) {
- val, err := roa.vReader.Has(roa.actor, roa.version, key)
+ val, err := r.vReader.Has(r.actor, r.version, key)
if err != nil {
return false, err
}
return val, nil
}
-func (roa *Reader) Get(key []byte) ([]byte, error) {
+func (r *Reader) Get(key []byte) ([]byte, error) {
- return roa.vReader.Get(roa.actor, roa.version, key)
+ return r.vReader.Get(r.actor, r.version, key)
}
-func (roa *Reader) Iterator(start, end []byte) (corestore.Iterator, error) {
+func (r *Reader) Iterator(start, end []byte) (corestore.Iterator, error) {
- return roa.vReader.Iterator(roa.actor, roa.version, start, end)
+ return r.vReader.Iterator(r.actor, r.version, start, end)
}
-func (roa *Reader) ReverseIterator(start, end []byte) (corestore.Iterator, error) {
+func (r *Reader) ReverseIterator(start, end []byte) (corestore.Iterator, error) {
- return roa.vReader.ReverseIterator(roa.actor, roa.version, start, end)
+ return r.vReader.ReverseIterator(r.actor, r.version, start, end)
}Also applies to: 48-48, 57-57, 61-61, 65-65
alpe
left a comment
alpe
left a comment
There was a problem hiding this comment.
Very nice work. The new versioned reader makes it more readable.
I found one bug otherwise 👍
store/v2/commitment/store.go
Outdated
| // Has implements store.VersionedReader. | ||
| func (c *CommitStore) Has(storeKey []byte, version uint64, key []byte) (bool, error) { | ||
| val, err := c.Get(storeKey, version, key) | ||
| return len(val) > 0, err |
There was a problem hiding this comment.
This is not correct. Empty slice is an supported value but not nil
| return len(val) > 0, err | |
| return val != nil, err |
You can use this test to verify:
func (s *CommitStoreTestSuite) TestStore_Has() {
storeKeys := []string{storeKey1}
myKey := []byte("myKey")
const initialVersion = 1
specs := map[string]struct {
src *corestore.Changeset
queryVersion uint64
expExists bool
expErr bool
}{
"known key with some value": {
src: corestore.NewChangesetWithPairs(map[string]corestore.KVPairs{
storeKey1: {{Key: myKey, Value: []byte("my-value")}},
}),
queryVersion: initialVersion,
expExists: true,
},
"known key with empty value": {
src: corestore.NewChangesetWithPairs(map[string]corestore.KVPairs{
storeKey1: {{Key: myKey, Value: []byte("")}},
}),
queryVersion: initialVersion,
expExists: true,
},
"unknown key": {
src: corestore.NewChangesetWithPairs(map[string]corestore.KVPairs{}),
queryVersion: initialVersion,
expExists: false,
},
"unknown version": {
src: corestore.NewChangesetWithPairs(map[string]corestore.KVPairs{
storeKey1: {{Key: myKey, Value: []byte("")}},
}),
queryVersion: initialVersion + 1,
expErr: true,
},
}
for name, spec := range specs {
s.T().Run(name, func(t *testing.T) {
commitStore, err := s.NewStore(dbm.NewMemDB(), storeKeys, nil, coretesting.NewNopLogger())
require.NoError(t, err)
require.NoError(t, commitStore.WriteChangeset(spec.src))
_, err = commitStore.Commit(initialVersion)
require.NoError(t, err)
// when
gotResult, gotErr := commitStore.Has([]byte(storeKey1), spec.queryVersion, myKey)
// then
if spec.expErr {
require.Error(t, gotErr)
return
}
require.NoError(t, gotErr)
require.Equal(t, spec.expExists, gotResult)
})
}
}There was a problem hiding this comment.
good catch, let me try to add this test case!
There was a problem hiding this comment.
I had added the test to this file locally: https://github.com/cosmos/cosmos-sdk/blob/main/store/v2/commitment/store_test_suite.go
| } | ||
| } | ||
|
|
||
| return nil, fmt.Errorf("version %d does not exist", version) |
There was a problem hiding this comment.
note: this error is also returned when the type cast failed.
There was a problem hiding this comment.
yes, it is expected
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
store/v2/commitment/iavl/tree_test.go (1)
137-214: Good test coverage, with room for improvements.The
TestIavlTreeIteratorfunction provides good coverage of the basic iterator functionality, including forward and reverse iteration across different versions. However, there are several areas where the test could be enhanced:
- Consider adding checks for the values associated with the keys, not just the keys themselves.
- Add tests for iterating with start and end key boundaries to ensure proper handling of partial ranges.
- Include checks to verify that the iterator is exhausted after the last key (i.e.,
Valid()returnsfalseafter the lastNext()call).- Add tests for error cases, such as attempting to iterate over a non-existent version.
Here's an example of how you could add a check for iterator exhaustion:
// After the last iter.Next() call in each iteration test require.False(t, iter.Valid(), "Iterator should be exhausted")And an example of testing with start and end boundaries:
// Add this test case // iterate over keys with start and end boundaries iter, err = tree.Iterator(3, []byte("key3"), []byte("key7"), true) require.NoError(t, err) expectedKeys = []string{"key3", "key4", "key5", "key6"} for i := 0; iter.Valid(); i++ { require.Equal(t, expectedKeys[i], string(iter.Key())) iter.Next() } require.False(t, iter.Valid(), "Iterator should be exhausted") require.NoError(t, iter.Close())store/v2/root/store.go (2)
121-145: LGTM with a minor suggestion for improvementThe new
getVersionedReadermethod effectively centralizes the version retrieval logic, improving code organization. It correctly checks both the state storage and state commitment for the requested version.Consider combining the error checks for
VersionExistscalls to reduce code duplication:func (s *Store) getVersionedReader(version uint64) (store.VersionedReader, error) { - isExist, err := s.stateStorage.VersionExists(version) - if err != nil { - return nil, err - } - if isExist { + if isExist, err := s.stateStorage.VersionExists(version); err != nil { + return nil, fmt.Errorf("failed to check version existence in state storage: %w", err) + } else if isExist { return s.stateStorage, nil } if vReader, ok := s.stateCommitment.(store.VersionedReader); ok { - isExist, err := vReader.VersionExists(version) - if err != nil { - return nil, err - } - if isExist { + if isExist, err := vReader.VersionExists(version); err != nil { + return nil, fmt.Errorf("failed to check version existence in state commitment: %w", err) + } else if isExist { return vReader, nil } } return nil, fmt.Errorf("version %d does not exist", version) }This change would make the error handling more consistent and informative.
162-166: LGTM with a minor suggestion for error handlingThe refactored
StateAtmethod effectively uses the newgetVersionedReaderfunction, aligning well with the changes inStateLatest. The implementation is concise and clear.Consider wrapping the error returned by
getVersionedReaderto provide more context:func (s *Store) StateAt(v uint64) (corestore.ReaderMap, error) { vReader, err := s.getVersionedReader(v) - return NewReaderMap(v, vReader), err + if err != nil { + return nil, fmt.Errorf("failed to get versioned reader for version %d: %w", v, err) + } + return NewReaderMap(v, vReader), nil }This change would make the error more informative for debugging purposes.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
- store/v2/commitment/iavl/tree_test.go (1 hunks)
- store/v2/commitment/store.go (2 hunks)
- store/v2/root/store.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
store/v2/commitment/iavl/tree_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/commitment/store.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.
🔇 Additional comments (5)
store/v2/commitment/store.go (4)
28-31: LGTM: Correct implementation ofstore.VersionedReaderThe addition of the
store.VersionedReaderinterface implementation is correct and well-documented. The comment provides valuable context about its usage during the migration process.
296-300: LGTM: Correct implementation ofVersionExistsThe
VersionExistsmethod is well-implemented. It correctly uses themetadata.GetCommitInfomethod and properly handles the returned error.
302-316: LGTM: ImprovedGetmethod implementationThe updated
Getmethod is well-implemented. It correctly uses the newgetReadermethod, improving code reusability. The error handling is comprehensive and provides informative error messages.
323-341: LGTM: Well-implementedIteratorandReverseIteratormethodsThe
IteratorandReverseIteratormethods are well-implemented. They correctly use thegetReadermethod and provide consistent implementations. The error handling is appropriate, and the code is clean and readable.store/v2/root/store.go (1)
148-160: LGTM: Improved StateLatest implementationThe refactored
StateLatestmethod effectively utilizes the newgetVersionedReaderfunction, improving code clarity and reusability. The error handling is consistent, and the method correctly creates and returns aReaderMap.
alpe
left a comment
alpe
left a comment
There was a problem hiding this comment.
Fix looks good! Thanks for the update 🏅
| } | ||
|
|
||
| bz, err := tree.Get(version, key) | ||
| reader, ok := tree.(Reader) |
There was a problem hiding this comment.
this error could poise a problem, if a user does not implement reader on the commitment structure and this code path is called it would end in an error that can not be recovered.
can we leave some code comments about why this is here?
| } | ||
|
|
||
| // Iterator implements store.VersionedReader. | ||
| func (c *CommitStore) Iterator(storeKey []byte, version uint64, start, end []byte) (corestore.Iterator, error) { |
There was a problem hiding this comment.
| func (c *CommitStore) Iterator(storeKey []byte, version uint64, start, end []byte) (corestore.Iterator, error) { | |
| // WARNING: these are implemented for migration from store v1 to store v2, they should not be called | |
| func (c *CommitStore) Iterator(storeKey []byte, version uint64, start, end []byte) (corestore.Iterator, error) { |
tac0turtle
left a comment
tac0turtle
left a comment
There was a problem hiding this comment.
utACK, i think we need a few more comments just as a warning
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
store/v2/commitment/store.go (2)
328-336: Add warning comment toIteratormethodTo provide clarity on the intended use of the
Iteratormethod, consider adding a warning comment indicating that this method is only used during the migration process and should not be called outside of it.Consider updating the comment as:
// Iterator implements store.VersionedReader. +// WARNING: This function is only used during the migration process. Generally, the SC layer +// does not provide iterators for the CommitStore. func (c *CommitStore) Iterator(storeKey []byte, version uint64, start, end []byte) (corestore.Iterator, error) {
338-346: Add warning comment toReverseIteratormethodFor consistency and clarity, consider adding a warning comment to indicate that
ReverseIteratoris intended for use only during the migration process and should not be used otherwise.Update the comment as:
// ReverseIterator implements store.VersionedReader. +// WARNING: This function is only used during the migration process. Generally, the SC layer +// does not provide iterators for the CommitStore. func (c *CommitStore) ReverseIterator(storeKey []byte, version uint64, start, end []byte) (corestore.Iterator, error) {
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- store/v2/commitment/store.go (2 hunks)
- store/v2/root/reader.go (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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/root/reader.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (13)
store/v2/root/reader.go (9)
17-18: Fields inReaderMapare appropriately definedThe
vReaderandversionfields are correctly added to theReaderMapstruct to support versioned reading functionality.
21-24:NewReaderMapconstructor initializes fields correctlyThe constructor
NewReaderMapproperly initializes theReaderMapwithvReaderandversion, ensuring accurate setup for read operations.
28-29:GetReadermethod correctly creates a newReaderThe
GetReaderfunction correctly returns a newReaderinstance using the currentversionandvReader, along with the providedactor.
34-36: Fields inReaderare appropriately definedThe
Readerstruct fieldsversion,vReader, andactorare correctly defined to facilitate versioned read operations for a specific actor.
39-43:NewReaderconstructor initializes fields correctlyThe
NewReaderfunction accurately initializes a newReaderwith the providedversion,vReader, andactor, ensuring correct read behavior.
Line range hint
48-52:Hasmethod correctly delegates tovReader.HasThe
Hasmethod correctly callsvReader.Haswith the necessary parameters and handles errors appropriately to determine the existence of a key.
57-58:Getmethod correctly delegates tovReader.GetThe
Getmethod properly retrieves the value associated with a key by delegating tovReader.Get.
61-62:Iteratormethod correctly provides key iterationThe
Iteratormethod accurately creates an iterator over a range of keys by delegating tovReader.Iterator.
65-66:ReverseIteratormethod correctly provides reverse key iterationThe
ReverseIteratormethod correctly delegates tovReader.ReverseIteratorto iterate over keys in reverse order.store/v2/commitment/store.go (4)
28-32: LGTM!The implementation and accompanying comments are clear and conform to Go style guidelines.
283-299: Well-structuredgetReadermethodThe
getReadermethod is well-implemented and handles error cases appropriately.
301-305: Implementation ofVersionExistsis correctThe
VersionExistsmethod is correctly implemented and adheres to Go conventions.
322-326:Hasmethod correctly checks key existenceThe
Hasmethod is correctly implemented by checking if the value returned byGetis notnil.
| // Get implements store.VersionedReader. | ||
| func (c *CommitStore) Get(storeKey []byte, version uint64, key []byte) ([]byte, error) { | ||
| reader, err := c.getReader(conv.UnsafeBytesToStr(storeKey)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
There was a problem hiding this comment.
Ensure safe conversion of key and storeKey in error messages
In the error message, both key and storeKey are []byte but are being formatted using %s. If the byte slices contain non-printable characters or are not valid UTF-8, it could lead to unintended results.
Consider converting the byte slices to strings safely or formatting them as hexadecimal. Apply this diff to address the issue:
func (c *CommitStore) Get(storeKey []byte, version uint64, key []byte) ([]byte, error) {
reader, err := c.getReader(conv.UnsafeBytesToStr(storeKey))
if err != nil {
return nil, err
}
bz, err := reader.Get(version, key)
if err != nil {
- return nil, fmt.Errorf("failed to get key %s from store %s: %w", key, storeKey, err)
+ return nil, fmt.Errorf("failed to get key %x from store %s: %w", key, conv.UnsafeBytesToStr(storeKey), err)
}
return bz, nil
}📝 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.
| // Get implements store.VersionedReader. | |
| func (c *CommitStore) Get(storeKey []byte, version uint64, key []byte) ([]byte, error) { | |
| reader, err := c.getReader(conv.UnsafeBytesToStr(storeKey)) | |
| if err != nil { | |
| return nil, err | |
| } | |
| // Get implements store.VersionedReader. | |
| func (c *CommitStore) Get(storeKey []byte, version uint64, key []byte) ([]byte, error) { | |
| reader, err := c.getReader(conv.UnsafeBytesToStr(storeKey)) | |
| if err != nil { | |
| return nil, err | |
| } | |
| bz, err := reader.Get(version, key) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get key %x from store %s: %w", key, conv.UnsafeBytesToStr(storeKey), err) | |
| } | |
| return bz, nil | |
| } |
Description
Closes: #22243
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
Bug Fixes