feat: implement object stores #22893
Conversation
📝 Walkthrough<details>
<summary>📝 Walkthrough</summary>
## Walkthrough
This pull request introduces significant changes across multiple modules in the Cosmos SDK, focusing on enhancing store functionality, introducing generics, and improving type safety. Key modifications include supporting nested message simulation, adding a Linux-specific crypto/keyring backend, enabling hex key imports, and introducing an object store mounting capability in the base application. The changes also involve upgrading RocksDB libraries, simplifying integration tests, and making several API-breaking changes.
## Changes
| File/Module | Change Summary |
|------------|----------------|
| `CHANGELOG.md` | Updated with new entries for unreleased changes, including object store support and various module enhancements |
| `baseapp/baseapp.go` | Added `MountObjectStores` method to mount object stores |
| `runtime/store.go` | Updated iterator return types from `store.Iterator` to `storetypes.Iterator` |
| `server/mock/store.go` | Removed `CacheWrapWithListeners` method; added `GetObjKVStore` method |
| `store/cachekv` | Implemented generic `BTree` and iterator structures |
| `store/transient` | Added generic store implementation with `GStore` and `ObjStore` |
| `store/types` | Introduced generic interfaces, added `ObjectStoreKey`, extended `StoreType` |
| `types/context.go` | Added `ObjectStore` method to retrieve object stores |
## Possibly related PRs
- **#22280**: Updates to `CHANGELOG.md` with new features
- **#22495**: Changelog updates for core components
- **#22739**: SimApp changelog updates
- **#22794**: Store v2 architectural decision record
## Suggested Labels
`C:server/v2`, `C:collections`, `C:collections/protocodec`, `Type: ADR`
## Suggested Reviewers
- kocubinski
- julienrbrt
- tac0turtle
- sontrinh16
- akhilkumarpilli
</details>Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
|
generic interface generic btree generic cachekv generic transient store support ObjStore changelog Update CHANGELOG.md Signed-off-by: yihuang <[email protected]> object store key Apply review suggestions fix merge conflict fix snapshot revert dependers Problem: store key type assertion is incorrect (cosmos#244) fix and add test
baseapp/baseapp.go
Outdated
| "strconv" | ||
| "sync" | ||
|
|
||
| abci "github.com/cometbft/cometbft/api/cometbft/abci/v1" | ||
| cmtproto "github.com/cometbft/cometbft/api/cometbft/types/v1" | ||
| "github.com/cometbft/cometbft/crypto/tmhash" | ||
| "github.com/cosmos/gogoproto/proto" | ||
| "golang.org/x/exp/maps" |
There was a problem hiding this comment.
Let's kill this dep please and use stdlib
store/cachekv/store.go
Outdated
There was a problem hiding this comment.
Can we get a changelog in store/CHANGELOG.md? This can be under 1.10.0 (we've only tagged 1.10.0 rc.1)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| func (cms Store) GetKVStore(key types.StoreKey) types.KVStore { | ||
| store, ok := cms.getCacheWrap(key).(types.KVStore) | ||
| if !ok { | ||
| panic(fmt.Sprintf("store with key %v is not KVStore", key)) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| store := types.KVStore(s) | ||
| store, ok := s.(types.KVStore) | ||
| if !ok { | ||
| panic(fmt.Sprintf("store with key %v is not KVStore", key)) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| // AssertValidValueLength checks if the value length is within length limit | ||
| func AssertValidValueLength(l int) { | ||
| if l > MaxValueLength { | ||
| panic(errors.New("value is too large")) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
mmsqe
force-pushed
the
obj_store
branch
2 times, most recently
from
46b5240 to
b09d61e
Compare
mmsqe
requested review from
a team,
cool-develope,
kocubinski,
hieuvubk,
facundomedica,
aaronc and
testinginprod
as code owners
![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
♻️ Duplicate comments (1)
store/rootmulti/store.go (1)
697-697:⚠️ Potential issueAvoid panic in
GetKVStoreto prevent potential chain halt.Using
panicinGetKVStorecould cause a chain halt if the store is not aKVStore. It's safer to return an error instead of panicking, which allows for graceful error handling.Apply this diff to return an error:
-func (rs *Store) GetKVStore(key types.StoreKey) types.KVStore { +func (rs *Store) GetKVStore(key types.StoreKey) (types.KVStore, error) { s := rs.stores[key] if s == nil { panic(fmt.Sprintf("store does not exist for key: %s", key.Name())) } - store, ok := s.(types.KVStore) + store, ok := s.(types.KVStore) if !ok { - panic(fmt.Sprintf("store with key %v is not KVStore", key)) + return nil, fmt.Errorf("store with key %v is not KVStore", key) } if rs.TracingEnabled() { store = tracekv.NewStore(store, rs.traceWriter, rs.getTracingContext()) } if rs.ListeningEnabled(key) { store = listenkv.NewStore(store, key, rs.listeners[key]) } - return store + return store, nil }
🧹 Nitpick comments (12)
store/internal/btree/memiterator.go (1)
17-19: Update comments to reflect generic implementationThe
memIteratorstruct is now generic, but the comments have not been updated to reflect this change.Consider updating the comments to include the type parameter
Vand explain any relevant changes due to generics.store/transient/store.go (1)
52-52: Consider refiningvalueLenfunction forObjStoreIn
NewObjStore, thevalueLenfunction returns1for all values, which might not provide meaningful length validation for different object sizes.Consider implementing
valueLento reflect the actual size of the stored objects if precise value length is necessary.store/cachekv/store.go (1)
337-337: Remove unusednolint:staticcheckdirectivesThe
//nolint:staticcheckdirectives on lines 337, 349, 392, 396, and 403 are unnecessary as they do not suppress anystaticcheckwarnings. The linter indicates that these directives are unused. Please remove or update them accordingly.Apply this diff to remove the unnecessary directives:
- unsorted := make([]*kvPair[V], 0) //nolint:staticcheck // We are in store v1. + unsorted := make([]*kvPair[V], 0) ... - unsorted = append(unsorted, &kvPair[V]{Key: []byte(key), Value: cacheValue.value}) //nolint:staticcheck // We are in store v1. + unsorted = append(unsorted, &kvPair[V]{Key: []byte(key), Value: cacheValue.value}) ... - kvL := make([]*kvPair[V], 0, 1+endIndex-startIndex) //nolint:staticcheck // We are in store v1. + kvL := make([]*kvPair[V], 0, 1+endIndex-startIndex) ... - kvL = append(kvL, &kvPair[V]{Key: []byte(key), Value: cacheValue.value}) //nolint:staticcheck // We are in store v1. + kvL = append(kvL, &kvPair[V]{Key: []byte(key), Value: cacheValue.value}) ... - func (store *GStore[V]) clearUnsortedCacheSubset(unsorted []*kvPair[V], sortState sortState) { //nolint:staticcheck // We are in store v1. + func (store *GStore[V]) clearUnsortedCacheSubset(unsorted []*kvPair[V], sortState sortState) {Also applies to: 349-349, 392-392, 396-396, 403-403
🧰 Tools
🪛 golangci-lint (1.62.2)
337-337: directive
//nolint:staticcheck // We are in store v1.is unused for linter "staticcheck"(nolintlint)
store/types/store.go (1)
150-150: Add documentation comment for the exported methodGetObjKVStore.The method
GetObjKVStoreis exported but lacks a documentation comment. According to the Go conventions and the Uber Golang style guide, all exported functions and methods should have a comment that begins with the function name.store/rootmulti/store.go (1)
1101-1101: Correct grammatical error in error message.The error message should use "an" instead of "a" before "ObjectStoreKey".
Apply this diff to fix the error message:
return nil, fmt.Errorf("unexpected key type for a ObjectStoreKey; got: %s, %T", key.String(), key) +return nil, fmt.Errorf("unexpected key type for an ObjectStoreKey; got: %s, %T", key.String(), key)store/types/validity.go (1)
36-36: Simplify the panic statement inAssertValidValueLength.Using
errors.Newinsidepanicis unnecessary sincepanicaccepts a string. Simplify the panic to directly include the error message.Apply this diff to simplify the panic:
func AssertValidValueLength(l int) { if l > MaxValueLength { - panic(errors.New("value is too large")) + panic("value is too large") } }store/cachemulti/store.go (1)
173-188: Consider adding error return instead of panicWhile panics are acceptable for development-time errors, consider returning an error for production code to allow for more graceful error handling.
-func (cms Store) GetKVStore(key types.StoreKey) types.KVStore { +func (cms Store) GetKVStore(key types.StoreKey) (types.KVStore, error) { store, ok := cms.getCacheWrap(key).(types.KVStore) if !ok { - panic(fmt.Sprintf("store with key %v is not KVStore", key)) + return nil, fmt.Errorf("store with key %v is not KVStore", key) } - return store + return store, nil } -func (cms Store) GetObjKVStore(key types.StoreKey) types.ObjKVStore { +func (cms Store) GetObjKVStore(key types.StoreKey) (types.ObjKVStore, error) { store, ok := cms.getCacheWrap(key).(types.ObjKVStore) if !ok { - panic(fmt.Sprintf("store with key %v is not ObjKVStore", key)) + return nil, fmt.Errorf("store with key %v is not ObjKVStore", key) } - return store + return store, nil }types/context.go (1)
353-353: Improve method documentationThe comment is incomplete and doesn't explain the purpose or behavior of the ObjectStore method.
Apply this diff to improve the documentation:
-// ObjectStore fetches an object store from the MultiStore, +// ObjectStore fetches an object store from the MultiStore and wraps it with gas metering. +// The returned store automatically tracks gas usage based on the configured transient gas parameters.CHANGELOG.md (4)
Line range hint
1-1: Add title to changelog fileAdd a descriptive title at the top of the file to clearly identify it as the Cosmos SDK changelog.
+# Cosmos SDK Changelog + # Changelog
Line range hint
1-2000: Consider splitting changelog into separate files by major versionThe changelog file is quite large and contains multiple major versions. Consider splitting it into separate files per major version (e.g. CHANGELOG_v0.47.md, CHANGELOG_v0.46.md etc) to improve readability and maintenance.
Line range hint
1-2000: Standardize version entry formatSome version entries use different heading levels and date formats. Standardize on:
- H2 (##) for version numbers
- ISO date format (YYYY-MM-DD)
Line range hint
1-2000: Add table of contentsAdd a table of contents at the top of the file listing all major versions for easier navigation.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
CHANGELOG.md(1 hunks)baseapp/baseapp.go(1 hunks)go.mod(1 hunks)runtime/store.go(1 hunks)server/mock/store.go(1 hunks)store/CHANGELOG.md(1 hunks)store/cachekv/internal/mergeiterator.go(11 hunks)store/cachekv/search_benchmark_test.go(2 hunks)store/cachekv/store.go(11 hunks)store/cachemulti/store.go(5 hunks)store/gaskv/store.go(2 hunks)store/internal/btree/btree.go(3 hunks)store/internal/btree/btree_test.go(4 hunks)store/internal/btree/memiterator.go(6 hunks)store/internal/btree/memiterator_test.go(3 hunks)store/internal/btreeadaptor.go(1 hunks)store/listenkv/store.go(2 hunks)store/listenkv/store_test.go(1 hunks)store/rootmulti/store.go(17 hunks)store/rootmulti/store_test.go(5 hunks)store/tracekv/store.go(1 hunks)store/tracekv/store_test.go(1 hunks)store/transient/store.go(1 hunks)store/types/store.go(6 hunks)store/types/validity.go(2 hunks)tests/go.mod(1 hunks)types/context.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (26)
store/types/validity.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/listenkv/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/go.mod (1)
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
store/tracekv/store.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"
types/context.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/internal/btree/btree_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/tracekv/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/listenkv/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"
baseapp/baseapp.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/internal/btree/memiterator_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"
server/mock/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
runtime/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/cachekv/search_benchmark_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/rootmulti/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/rootmulti/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/internal/btreeadaptor.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/cachemulti/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
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.
store/cachekv/internal/mergeiterator.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/internal/btree/btree.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/gaskv/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/cachekv/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/internal/btree/memiterator.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
store/cachemulti/store.go
8-8: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
store/cachekv/store.go
337-337: directive //nolint:staticcheck // We are in store v1. is unused for linter "staticcheck"
(nolintlint)
349-349: directive //nolint:staticcheck // We are in store v1. is unused for linter "staticcheck"
(nolintlint)
392-392: directive //nolint:staticcheck // We are in store v1. is unused for linter "staticcheck"
(nolintlint)
396-396: directive //nolint:staticcheck // We are in store v1. is unused for linter "staticcheck"
(nolintlint)
403-403: directive //nolint:staticcheck // We are in store v1. is unused for linter "staticcheck"
(nolintlint)
🔇 Additional comments (35)
store/cachekv/search_benchmark_test.go (5)
7-7: Import of the btree package is appropriate
The addition of the btree import is necessary for utilizing the new generic btree structure.
25-25: Initialization of cache map with generic type
The cache map is correctly initialized as map[string]*cValue[[]byte]{} to accommodate the use of generics.
30-30: Assignment to cache with generic cValue
Assigning &cValue[[]byte]{} to the cache entries aligns with the updated generic cValue type.
36-36: Assignment to cache for 'Z' keys is correct
The initialization of cache entries for 'Z' keys with *cValue[[]byte]{} is appropriate.
39-42: Return statement uses generic GStore correctly
The function returns a *GStore[[]byte] with the updated fields, including the new generic btree for sortedCache.
store/internal/btree/memiterator.go (1)
83-85: Method implementations with generics are appropriate
The methods of the memIterator[V] are correctly updated to accommodate the generic type V.
store/transient/store.go (3)
14-15: Type assertions for ObjStore interfaces are correct
The type assertions confirm that *ObjStore implements types.Committer and types.ObjKVStore, which is appropriate.
19-21: Implementation of generic GStore is appropriate
The GStore[V] struct correctly embeds internal.BTreeStore[V], leveraging generics for flexibility.
85-87: Method WorkingHash returns an empty byte slice
The WorkingHash method appropriately returns an empty byte slice as transient stores do not maintain state between transactions.
store/internal/btree/btree.go (1)
Line range hint 25-96: LGTM
The generics implementation in the btree package enhances flexibility while maintaining functionality. The code changes are well-structured and conform to Go conventions.
store/cachekv/internal/mergeiterator.go (1)
Line range hint 17-229: LGTM
The adaptation of cacheMergeIterator to use generics is correctly implemented. The methods are consistent with Go's best practices, and the logic remains sound.
store/gaskv/store.go (1)
Line range hint 9-213: LGTM
The introduction of generics into the gaskv package is thoughtfully executed. The use of valueLen functions and isZero checks appropriately handles different data types.
store/types/store.go (2)
Line range hint 382-409: Consistent addition of StoreTypeObject to StoreType enum.
The introduction of StoreTypeObject is properly integrated in the StoreType enum and the String() method. This maintains consistency with other store types.
346-347: Verify the impact of embedding CacheWrapper into CacheWrap.
Embedding CacheWrapper into CacheWrap changes the interface hierarchy. Ensure that this modification does not introduce unintended behavior and that all implementations of CacheWrap are updated accordingly.
Run the following script to check for implementations of CacheWrap that may be affected:
✅ Verification successful
Let me gather more specific information about the CacheWrap and CacheWrapper interfaces and their implementations.
Let me try a different approach to find the implementations.
The embedding of CacheWrapper in CacheWrap interface is a valid design choice
Based on the verification results, this is a proper interface composition pattern. The CacheWrapper interface defines the caching behavior (CacheWrap() and CacheWrapWithTrace()), while CacheWrap extends it by adding the Write() method. All implementations across the codebase correctly implement both interfaces' methods:
- Multiple store types implement both
Write()and the caching methods (CacheWrap,CacheWrapWithTrace) - The implementation is consistent across different store types (tracekv, gaskv, iavl, rootmulti, etc.)
- No conflicts or inconsistencies were found in the interface implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of CacheWrap that do not embed CacheWrapper.
# Find all types that implement CacheWrap
rg --type go 'type .* struct' -A 5 | grep -E 'type .* struct' | while read -r line; do
struct_name=$(echo "$line" | awk '{print $2}')
if ! rg --type go "$struct_name" -A 5 | grep -q 'CacheWrapper'; then
echo "Struct $struct_name does not embed CacheWrapper"
fi
done
Length of output: 332
Script:
#!/bin/bash
# Find the definition and implementations of CacheWrap and CacheWrapper
echo "=== CacheWrap interface definition ==="
ast-grep --pattern 'type CacheWrap interface {
$$$
}'
echo -e "\n=== CacheWrapper interface definition ==="
ast-grep --pattern 'type CacheWrapper interface {
$$$
}'
echo -e "\n=== Structs implementing CacheWrap ==="
rg --type go -B 2 -A 5 'type \w+\s+struct\s*{.*CacheWrap'
echo -e "\n=== Structs implementing CacheWrapper ==="
rg --type go -B 2 -A 5 'type \w+\s+struct\s*{.*CacheWrapper'
Length of output: 1525
Script:
#!/bin/bash
# Find implementations of CacheWrap and CacheWrapper
echo "=== Files containing CacheWrap implementations ==="
rg --type go "struct.*{" -A 5 | grep -B 5 -A 5 "CacheWrap"
echo -e "\n=== Usage of CacheWrapper in struct definitions ==="
rg --type go "struct.*{" -A 5 | grep -B 5 -A 5 "CacheWrapper"
echo -e "\n=== Methods implementing CacheWrap interface ==="
rg --type go "func.*Write\(\)"
echo -e "\n=== Methods implementing CacheWrapper interface ==="
rg --type go "func.*CacheWrap\(\)"
rg --type go "func.*CacheWrapWithTrace\("
Length of output: 4872
store/internal/btree/memiterator_test.go (2)
8-8: Update instantiation of NewBTree with type parameter.
The usage of NewBTree[[]byte]() introduces generics correctly. Ensure that all usages of NewBTree in the codebase are updated to include the appropriate type parameters.
1-1: Verify the package name change to ensure correct visibility and import paths.
Changing the package name from internal to btree may affect the visibility and import statements. Ensure that the package name aligns with the directory structure and intended usage.
Run the following script to find all import statements that might be affected:
✅ Verification successful
Let me check the actual package declarations and imports to verify if there's any mismatch.
Package name change is consistent with internal visibility
The package name change from internal to btree is correct and doesn't affect visibility or imports. The package is still located in the store/internal/btree directory, maintaining its internal visibility, while all imports correctly reference this path as cosmossdk.io/store/internal/btree. The package name btree better reflects its functionality without affecting its accessibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all files importing the btree package.
rg --type go 'store/internal/btree' --files-with-matches
Length of output: 174
Script:
#!/bin/bash
# Check the actual package declarations and imports
echo "=== Package declaration in btree package ==="
rg --type go '^package' store/internal/btree/
echo -e "\n=== Import statements referencing btree ==="
rg --type go '".*store/internal/btree"' -A 1 store/transient/store.go store/internal/btreeadaptor.go store/cachekv/search_benchmark_test.go store/cachekv/store.go
Length of output: 1120
store/listenkv/store.go (2)
6-6: LGTM: Import addition aligns with implementation change
The addition of the cachekv import supports the new CacheWrap implementation.
136-136: Verify CacheWrap behavior in block STM context
While the implementation change from panic to returning a cache store is correct, we should ensure this behavior aligns with block STM requirements.
Consider documenting the caching strategy for block STM integration in the package documentation.
✅ Verification successful
CacheWrap implementation in ListenKV store is safe to use
Based on the codebase analysis, the change from panic to returning a cache store is consistent with other store implementations (IAVL, Prefix, DB Adapter) and there's no evidence of special block STM requirements that would be violated. The CacheWrap behavior is well-tested across the codebase and follows a standard pattern of wrapping with cachekv.NewStore.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CacheWrap usage patterns in block STM related code
rg -l "CacheWrap.*block.*stm"
Length of output: 31
Script:
#!/bin/bash
# Let's search for block STM related code first
rg -l "block.*stm" --type go
# Then search for CacheWrap usage patterns more broadly
rg "CacheWrap" --type go -A 3
# Also check for any tests involving CacheWrap
rg "CacheWrap.*Test" --type go
Length of output: 19085
Script:
#!/bin/bash
# Let's look for block STM related code with different patterns
rg "block.*stm|stm.*block" --type go -A 3
# Also check for any documentation about block STM
rg -l "block.*stm" --type md
# Let's also check for any tests involving CacheWrap with block or STM
rg "Test.*CacheWrap.*(?:block|stm)" --type go -A 5
Length of output: 127
store/CHANGELOG.md (1)
34-37: LGTM: Clear and well-formatted changelog entry
The changelog entry follows the guidelines and clearly documents the object store feature addition.
store/tracekv/store.go (1)
164-166: LGTM: Consistent CacheWrap implementation
The implementation correctly delegates to the parent store's CacheWrap, maintaining consistency with the listenkv store changes. The updated comment accurately reflects the interface being implemented.
✅ Verification successful
Let me analyze the CacheWrap implementations across different store types to verify the consistency of the tracekv store's implementation.
From the results, I can see several patterns:
- Most stores (mem, listenkv, prefix, iavl, cache, dbadapter) use
cachekv.NewStore(s) - Some specialized stores (rootmulti, cachemulti) use CacheMultiStore casting
- BTreeStore and GStore use their specialized NewGStore constructors
- Only gaskv store panics
- tracekv delegates to parent's CacheWrap
Let me verify if the tracekv store's parent type is consistent with this pattern.
Let me try a different approach to check the tracekv store's structure.
Let me try one more search specifically for the tracekv store structure.
Verified: CacheWrap implementation is correct and consistent
The tracekv store's CacheWrap implementation correctly delegates to its parent store, which is of type types.KVStore. This is consistent with the store's design pattern where it acts as a wrapper that traces operations while delegating actual storage operations to its parent. The implementation aligns with other stores in the codebase where:
- Most basic stores (mem, listenkv, prefix, iavl) use
cachekv.NewStore - Wrapper stores (like tracekv) delegate to their parent
- Specialized stores use their specific implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent CacheWrap implementation across store types
rg -A 2 "CacheWrap\(\) types\.CacheWrap"
Length of output: 1980
Script:
#!/bin/bash
# Check the Store struct definition in tracekv
rg -A 5 "type Store struct" store/tracekv/store.go
Length of output: 52
Script:
#!/bin/bash
# Use ast-grep to find the Store struct definition in tracekv
ast-grep --pattern 'type Store struct {
$$$
}'
Length of output: 6129
Script:
#!/bin/bash
# Search specifically in the tracekv store file
cat store/tracekv/store.go
Length of output: 5653
server/mock/store.go (1)
113-115: LGTM!
The new GetObjKVStore method follows the established pattern for unimplemented methods in this mock store.
store/cachemulti/store.go (2)
Line range hint 40-62: LGTM: Improved type safety and tracing logic
The changes enhance type safety by:
- Using
CacheWrapperinterface instead of concrete type - Adding proper type assertion for tracing support
165-171: LGTM: Well-structured helper method
The getCacheWrap helper method effectively reduces code duplication and centralizes the store retrieval logic.
store/internal/btree/btree_test.go (1)
12-12: LGTM: Clean generic type parameter usage
The addition of the []byte type parameter to NewBTree calls is consistent and maintains the original test functionality.
Also applies to: 43-43, 174-174
store/listenkv/store_test.go (1)
275-275: Verify CacheWrap behavior change
The test now expects CacheWrap to execute without panicking, which is a significant behavioral change. This needs to be verified against the store's implementation.
runtime/store.go (2)
166-171: LGTM: Iterator implementation with proper error handling
The implementation correctly handles errors from the underlying store and maintains consistency with the adapter pattern.
Line range hint 174-179: LGTM: ReverseIterator implementation with proper error handling
The implementation correctly handles errors from the underlying store and maintains consistency with the adapter pattern.
store/tracekv/store_test.go (1)
286-286: Verify consistent CacheWrap behavior across stores
The test now expects CacheWrap to execute without panicking, similar to the change in listenkv store. This behavioral change should be consistent across all store implementations.
✅ Verification successful
This will help us locate all store implementations and their CacheWrap methods to verify consistency.
CacheWrap behavior is consistent across store implementations
The verification shows that CacheWrap behavior is consistent across store implementations:
- Most stores (tracekv, prefix, listenkv, mem, iavl, dbadapter) implement CacheWrap by returning a new cache wrapper
- Only gaskv store explicitly panics on CacheWrap, which is an intentional design choice
- Mock and runtime stores are test/mock implementations that panic by design
The removal of panic expectation in tracekv store test is correct and aligns with the actual implementation that delegates to parent's CacheWrap.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent CacheWrap behavior across store implementations
# Expected: Find all store implementations and verify consistent behavior
# Find all store implementations
fd -e go -e go store.go | xargs ast-grep --pattern 'func ($store *Store) CacheWrap() $ret {
$$$
}'
Length of output: 102
Script:
#!/bin/bash
# Let's try a different approach to find CacheWrap implementations
# First, find all store.go files
fd store.go
# Then search for CacheWrap method implementations
rg -A 5 "func.*CacheWrap\(\)" --type go
Length of output: 7025
go.mod (1)
57-57: Verify stability of experimental package
The golang.org/x/exp package is used for experimental features. Please ensure that the required functionality is stable enough for production use.
✅ Verification successful
Let me check which specific packages from golang.org/x/exp are being used to better assess their stability.
Experimental package usage is stable and limited to well-established features
The golang.org/x/exp package is being used for three specific, stable features:
constraintspackage for generic type constraintsmapspackage for map utilitiesslicespackage for slice utilities
These specific packages have been stable and widely adopted in the Go ecosystem. Recent commit history shows no breaking changes, and the usage is limited to utility functions that have been promoted to the standard library in Go 1.21+.
Locations using these packages:
math/max_min.go: Usingconstraintsfor generic math operationsorm/model/ormdb/genesis.go: Usingmapsutilitiesorm/internal/codegen/query.go: Usingmapsandslicesutilitiesdepinject/*: Using all three packages for dependency injection implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any breaking changes or stability issues in the golang.org/x/exp package
# Get the package info and check for any stability warnings
curl -s "https://pkg.go.dev/golang.org/x/[email protected]?tab=doc" | grep -i "stability"
# Check git history for breaking changes
gh api graphql -f query='
{
repository(owner: "golang", name: "exp") {
object(expression: "master") {
... on Commit {
history(first: 10, since: "2024-01-01T00:00:00Z") {
nodes {
messageHeadline
committedDate
}
}
}
}
}
}'
Length of output: 1577
Script:
#!/bin/bash
# Find Go files that import golang.org/x/exp packages
rg --type go "\"golang.org/x/exp/" -B 1 -A 1
# List all direct dependencies on golang.org/x/exp
go list -f '{{.Deps}}' ./... | tr '[' ' ' | tr ']' ' ' | tr ' ' '\n' | grep "golang.org/x/exp" | sort -u
Length of output: 1225
tests/go.mod (1)
288-288: LGTM!
The replace directive correctly points to the local store module, which is the standard practice for testing in the Cosmos SDK.
store/rootmulti/store_test.go (4)
659-659: LGTM: Correct type assertion for testing pause pruning functionality.
The store assignment correctly casts to the expected interface type for testing the pause pruning behavior.
832-832: LGTM: Consistent test store key addition.
The new test store key follows the existing naming pattern and is properly added to support object store testing.
971-982: LGTM: Proper test setup for object stores.
The store map preparation correctly includes the new object store type while maintaining the existing test setup pattern.
983-994: LGTM: Consistent store map initialization.
The store map initialization properly includes the object store stub while maintaining consistency with existing test patterns.
baseapp/baseapp.go (1)
346-354: LGTM: Well-implemented object stores mounting method.
The implementation follows best practices:
- Consistent with existing mount methods pattern
- Uses deterministic key ordering
- Properly integrates with the store type system
| func (ts *BTreeStore[V]) Iterator(start, end []byte) types.GIterator[V] { | ||
| it, err := ts.BTree.Iterator(start, end) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| return it | ||
| } |
There was a problem hiding this comment.
Avoid panicking in Iterator; return errors instead
In the Iterator method, panicking on error is discouraged in library code. According to the Uber Go style guide, library functions should return errors rather than panicking.
Consider modifying the method to return an error or handle it gracefully without panicking.
| func (ts *BTreeStore[V]) ReverseIterator(start, end []byte) types.GIterator[V] { | ||
| it, err := ts.BTree.ReverseIterator(start, end) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| return it | ||
| } |
There was a problem hiding this comment.
Avoid panicking in ReverseIterator; return errors instead
Similar to the Iterator method, ReverseIterator panics when an error occurs. It's advisable to return errors to the caller instead of panicking, per the Uber Go style guide.
Modify the method to handle errors without panicking to improve robustness.
| // CacheWrapWithTrace branches the underlying store. | ||
| func (ts *BTreeStore[V]) CacheWrapWithTrace(w io.Writer, tc types.TraceContext) types.CacheWrap { | ||
| return cachekv.NewGStore(ts, ts.isZero, ts.valueLen) | ||
| } |
There was a problem hiding this comment.
Parameters w and tc are unused in CacheWrapWithTrace
The CacheWrapWithTrace method does not utilize its parameters w io.Writer and tc types.TraceContext. Unused parameters can lead to confusion and potential issues.
Ensure that these parameters are used to enable tracing functionality, or remove them if they are unnecessary.
| // ObjectStoreKey is used for indexing transient stores in a MultiStore | ||
| type ObjectStoreKey struct { | ||
| name string | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Correct the comment for ObjectStoreKey struct.
The comment incorrectly mentions that ObjectStoreKey is used for indexing transient stores. It should state that ObjectStoreKey is used for indexing object stores in a MultiStore.
Apply this diff to correct the comment:
-// ObjectStoreKey is used for indexing transient stores in a MultiStore
+// ObjectStoreKey is used for indexing object stores in a MultiStore
type ObjectStoreKey struct {
name string
}📝 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.
| // ObjectStoreKey is used for indexing transient stores in a MultiStore | |
| type ObjectStoreKey struct { | |
| name string | |
| } | |
| // ObjectStoreKey is used for indexing object stores in a MultiStore | |
| type ObjectStoreKey struct { | |
| name string | |
| } |
| @@ -860,7 +887,7 @@ func (rs *Store) Snapshot(height uint64, protoWriter protoio.Writer) error { | |||
| stores := []namedStore{} | |||
| keys := keysFromStoreKeyMap(rs.stores) | |||
| for _, key := range keys { | |||
| switch store := rs.GetCommitKVStore(key).(type) { | |||
| switch store := rs.GetCommitStore(key).(type) { | |||
There was a problem hiding this comment.
💡 Codebase verification
Updates needed for object store snapshotting
Based on the code analysis, the object store is implemented as a transient store type but the current snapshotting functionality only handles IAVL stores explicitly. The Snapshot method in rootmulti/store.go needs to be updated to properly handle StoreTypeObject to ensure data consistency during snapshot and restore operations.
Key findings:
- The
Snapshotmethod currently only exports IAVL stores - Object stores are implemented using
transient.NewObjStore() - No snapshot handling logic exists for object stores in the codebase
🔗 Analysis chain
Confirm compatibility with snapshotting of object stores.
The snapshotting functionality currently handles iavl.Store types. Verify that the introduction of ObjectStoreKey and associated stores doesn't require updates to the snapshotting logic.
Run the following script to search for usages of StoreTypeObject in snapshot-related code:
Let me gather more information about the snapshot implementation to verify the handling of different store types.
Let me check one more aspect about the snapshot implementation, specifically focusing on the object store handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of StoreTypeObject in snapshotting code.
rg --type go 'StoreTypeObject' --files-with-matches | xargs rg 'Snapshot|Restore'
Length of output: 4238
Script:
#!/bin/bash
# Check snapshot implementation details and store type handling
ast-grep --pattern 'type $_ struct {
$$$
storeType types.StoreType
$$$
}'
# Also check for store type handling in snapshot-related code
rg -A 5 'case types\.StoreType' store/
Length of output: 2829
Script:
#!/bin/bash
# Check snapshot implementation for object store
rg -B 2 -A 5 'Snapshot|Restore' store/transient/store.go
Length of output: 58
| // AssertValidValueLength checks if the value length is within length limit | ||
| func AssertValidValueLength(l int) { | ||
| if l > MaxValueLength { | ||
| panic(errors.New("value is too large")) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider returning an error instead of panicking in validation functions.
Instead of panicking on invalid input, it might be better to return an error. This provides better control over error handling and improves robustness.
Apply this diff to modify the function to return an error:
-func AssertValidValueLength(l int) {
+func AssertValidValueLength(l int) error {
if l > MaxValueLength {
- panic(errors.New("value is too large"))
+ return errors.New("value is too large")
}
+ return nil
}And update the calling function accordingly.
📝 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.
| // AssertValidValueLength checks if the value length is within length limit | |
| func AssertValidValueLength(l int) { | |
| if l > MaxValueLength { | |
| panic(errors.New("value is too large")) | |
| // AssertValidValueLength checks if the value length is within length limit | |
| func AssertValidValueLength(l int) error { | |
| if l > MaxValueLength { | |
| return errors.New("value is too large") | |
| } | |
| return nil | |
| } |
types/context.go
Outdated
| @@ -350,6 +350,11 @@ func (c Context) TransientStore(key storetypes.StoreKey) storetypes.KVStore { | |||
| return gaskv.NewStore(c.ms.GetKVStore(key), c.gasMeter, c.transientKVGasConfig) | |||
| } | |||
|
|
|||
| // ObjectStore fetches an object store from the MultiStore, | |||
| func (c Context) OjectStore(key storetypes.StoreKey) storetypes.ObjKVStore { | |||
There was a problem hiding this comment.
Fix typo in method name
The method name contains a typo: "OjectStore" should be "ObjectStore".
Apply this diff to fix the typo:
-func (c Context) OjectStore(key storetypes.StoreKey) storetypes.ObjKVStore {
+func (c Context) ObjectStore(key storetypes.StoreKey) storetypes.ObjKVStore {📝 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.
| func (c Context) OjectStore(key storetypes.StoreKey) storetypes.ObjKVStore { | |
| func (c Context) ObjectStore(key storetypes.StoreKey) storetypes.ObjKVStore { |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
store/rootmulti/store_test.go (2)
971-994: Consider enhancing error handlingWhile the implementation is correct, consider adding specific error handling for object store mounting failures to improve test robustness.
func prepareStoreMap() (map[types.StoreKey]types.CommitStore, error) { db := coretesting.NewMemDB() store := NewStore(db, log.NewNopLogger(), metrics.NewNoOpMetrics()) - store.MountStoreWithDB(types.NewKVStoreKey("iavl1"), types.StoreTypeIAVL, nil) - store.MountStoreWithDB(types.NewKVStoreKey("iavl2"), types.StoreTypeIAVL, nil) - store.MountStoreWithDB(types.NewTransientStoreKey("trans1"), types.StoreTypeTransient, nil) - store.MountStoreWithDB(types.NewObjectStoreKey("obj1"), types.StoreTypeObject, nil) + if err := store.MountStoreWithDB(types.NewKVStoreKey("iavl1"), types.StoreTypeIAVL, nil); err != nil { + return nil, fmt.Errorf("failed to mount iavl1: %w", err) + } + if err := store.MountStoreWithDB(types.NewKVStoreKey("iavl2"), types.StoreTypeIAVL, nil); err != nil { + return nil, fmt.Errorf("failed to mount iavl2: %w", err) + } + if err := store.MountStoreWithDB(types.NewTransientStoreKey("trans1"), types.StoreTypeTransient, nil); err != nil { + return nil, fmt.Errorf("failed to mount trans1: %w", err) + } + if err := store.MountStoreWithDB(types.NewObjectStoreKey("obj1"), types.StoreTypeObject, nil); err != nil { + return nil, fmt.Errorf("failed to mount obj1: %w", err) + }
Line range hint
1023-1034: Consider using named constants for test valuesThe test implementation could be improved by defining constants for magic numbers to enhance readability and maintainability.
+const ( + initialVersion = int64(1) + expectedCommitted = 0 +) + func TestCommitStores(t *testing.T) { // ... - var version int64 = 1 + var version int64 = initialVersion removalMap := map[types.StoreKey]bool{} res := commitStores(version, storeMap, removalMap) // ... - require.Equal(t, tc.exptectCommit, store.Committed) + require.Equal(t, expectedCommitted, store.Committed)store/transient/store.go (2)
19-26: Add documentation for valueLen parameterWhile the implementation is solid, the purpose and requirements of the
valueLenfunction parameter should be documented. This is especially important as it affects store behavior and validation.Add documentation like this:
// NewGStore constructs new generic transient store +// Parameters: +// - isZero: function to determine if a value should be considered empty/nil +// - valueLen: function to calculate the logical size of a value for validation purposes func NewGStore[V any](isZero func(V) bool, valueLen func(V) int) *GStore[V] {
62-87: Enhance documentation for store operationsWhile the implementation is correct, the behavior of these methods, especially
Commit(), should be better documented to explain their transient nature and side effects.Add documentation like this:
// Commit cleans up Store. -// Implements CommitStore +// Implements CommitStore. As this is a transient store: +// - All data is cleared on commit +// - CommitID is always empty +// - Version is always 0 func (ts *GStore[V]) Commit() (id types.CommitID) {store/gaskv/store.go (3)
9-10: Document the rationale for ObjectValueLength valueThe value
16seems arbitrary. Please document why this specific byte size was chosen for transient objects gas accounting.
25-30: Consider variable gas costs for different object sizesUsing a fixed
ObjectValueLengthfor all objects might not accurately reflect their actual memory footprint. Consider implementing a more granular approach that accounts for the actual size of different object types.
78-83: Consider returning errors instead of panickingWhile validation is good, panicking on nil values might be too aggressive. Consider returning an error instead, allowing callers to handle the invalid value case gracefully.
-func (gs *GStore[V]) assertValidValue(value V) { +func (gs *GStore[V]) assertValidValue(value V) error { if gs.isZero(value) { - panic("value is nil") + return fmt.Errorf("value cannot be nil") } types.AssertValidValueLength(gs.valueLen(value)) + return nil }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
store/CHANGELOG.md(1 hunks)store/gaskv/store.go(2 hunks)store/rootmulti/store_test.go(5 hunks)store/transient/store.go(1 hunks)types/context.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- store/CHANGELOG.md
- types/context.go
🧰 Additional context used
📓 Path-based instructions (3)
store/transient/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/rootmulti/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/gaskv/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (9)
store/rootmulti/store_test.go (4)
659-661: LGTM: Type change aligns with interface renaming
The change from CommitKVStore to CommitStore in the test stub is consistent with the interface renaming mentioned in the summary.
832-832: LGTM: New store key follows conventions
The addition of testStoreKey4 follows the existing naming pattern and is properly integrated with the test infrastructure.
Line range hint 905-919: LGTM: Function signature updated correctly
The hashStores function has been properly updated to use the new CommitStore type while maintaining its core functionality.
960-969: LGTM: Well-structured test stub implementation
The commitStoreStub provides a clean way to track commit calls while implementing the CommitStore interface.
store/transient/store.go (3)
4-15: LGTM: Import statements and interface checks are well structured
The import organization and interface compliance checks are comprehensive, including both Store and ObjStore implementations.
28-42: LGTM: Store implementation is clean and type-safe
The Store specialization for []byte is well implemented with appropriate zero checks and length calculations.
44-57: Review the hardcoded value length in ObjStore
The ObjStore constructor uses a hardcoded value length of 1 for all objects, which might not accurately represent the actual size of stored objects. This could affect store behavior and resource management.
Let's check if this value length is used in validation or size calculations:
Consider:
- Implementing a more accurate size calculation based on object serialization
- Documenting why a constant size of 1 was chosen
- Adding validation to ensure this doesn't cause issues with store limits
✅ Verification successful
Let me search for more specific usage of the value length validation in the codebase.
Let me check the MaxValueLength constant and its usage.
Hardcoded value length of 1 is appropriate for ObjStore
The constant value length of 1 for ObjStore is actually correct and here's why:
- The value length is used for validation against MaxValueLength (2^31 - 1) to prevent storing excessively large values
- Since ObjStore stores references to objects rather than the objects themselves, a constant size of 1 is a reasonable representation
- The value length is primarily used for gas calculation and validation boundaries, not for actual memory allocation
The implementation follows the same pattern used throughout the codebase where AssertValidValueLength is used as a safety check against the MaxValueLength constant.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of valueLen function in store validation
rg -A 5 "valueLen|ValidateLength|SizeOf" --type go
Length of output: 56624
Script:
#!/bin/bash
# Search for value length validation usage
rg -A 5 "AssertValidValueLength|ValidateLength" --type go
Length of output: 1091
Script:
#!/bin/bash
# Search for MaxValueLength constant definition and usage
rg -A 3 "MaxValueLength\s*=" --type go
Length of output: 478
store/gaskv/store.go (2)
32-40: LGTM! Well-designed generic implementation
The generic struct design with customizable zero-check and value-length functions provides good flexibility while maintaining type safety.
134-145: LGTM! Well-structured iterator implementation
The iterator implementation properly handles both ascending and descending iteration while maintaining gas accounting.
| func (gi *gasIterator[V]) consumeSeekGas() { | ||
| if gi.Valid() { | ||
| key := gi.Key() | ||
| value := gi.Value() | ||
|
|
||
| gi.gasMeter.ConsumeGas(gi.gasConfig.ReadCostPerByte*types.Gas(len(key)), types.GasValuePerByteDesc) | ||
| gi.gasMeter.ConsumeGas(gi.gasConfig.ReadCostPerByte*types.Gas(len(value)), types.GasValuePerByteDesc) | ||
| gi.gasMeter.ConsumeGas(gi.gasConfig.ReadCostPerByte*types.Gas(gi.valueLen(value)), types.GasValuePerByteDesc) | ||
| } | ||
| gi.gasMeter.ConsumeGas(gi.gasConfig.IterNextCostFlat, types.GasIterNextCostFlatDesc) |
There was a problem hiding this comment.
Address potential gas calculation overflow in iterator
Similar to the Set method, the consumeSeekGas function needs overflow protection for gas calculations.
Consider using safe math operations here as well:
- gi.gasMeter.ConsumeGas(gi.gasConfig.ReadCostPerByte*types.Gas(len(key)), types.GasValuePerByteDesc)
- gi.gasMeter.ConsumeGas(gi.gasConfig.ReadCostPerByte*types.Gas(gi.valueLen(value)), types.GasValuePerByteDesc)
+ if keyGas, overflow := SafeMul(gi.gasConfig.ReadCostPerByte, types.Gas(len(key))); !overflow {
+ gi.gasMeter.ConsumeGas(keyGas, types.GasValuePerByteDesc)
+ } else {
+ return fmt.Errorf("gas calculation overflow for key length %d", len(key))
+ }
+ if valueGas, overflow := SafeMul(gi.gasConfig.ReadCostPerByte, types.Gas(gi.valueLen(value))); !overflow {
+ gi.gasMeter.ConsumeGas(valueGas, types.GasValuePerByteDesc)
+ } else {
+ return fmt.Errorf("gas calculation overflow for value length %d", gi.valueLen(value))
+ }Committable suggestion skipped: line range outside the PR's diff.
store/gaskv/store.go
Outdated
| func (gs *GStore[V]) Set(key []byte, value V) { | ||
| types.AssertValidKey(key) | ||
| types.AssertValidValue(value) | ||
| gs.assertValidValue(value) | ||
| gs.gasMeter.ConsumeGas(gs.gasConfig.WriteCostFlat, types.GasWriteCostFlatDesc) | ||
| // TODO overflow-safe math? | ||
| gs.gasMeter.ConsumeGas(gs.gasConfig.WriteCostPerByte*types.Gas(len(key)), types.GasWritePerByteDesc) | ||
| gs.gasMeter.ConsumeGas(gs.gasConfig.WriteCostPerByte*types.Gas(len(value)), types.GasWritePerByteDesc) | ||
| gs.gasMeter.ConsumeGas(gs.gasConfig.WriteCostPerByte*types.Gas(gs.valueLen(value)), types.GasWritePerByteDesc) | ||
| gs.parent.Set(key, value) | ||
| } |
There was a problem hiding this comment.
Address potential gas calculation overflow
The TODO comment about overflow-safe math needs attention. Large keys or values could cause gas calculation overflow.
Consider using safe math operations:
- gs.gasMeter.ConsumeGas(gs.gasConfig.WriteCostPerByte*types.Gas(len(key)), types.GasWritePerByteDesc)
- gs.gasMeter.ConsumeGas(gs.gasConfig.WriteCostPerByte*types.Gas(gs.valueLen(value)), types.GasWritePerByteDesc)
+ if keyGas, overflow := SafeMul(gs.gasConfig.WriteCostPerByte, types.Gas(len(key))); !overflow {
+ gs.gasMeter.ConsumeGas(keyGas, types.GasWritePerByteDesc)
+ } else {
+ return fmt.Errorf("gas calculation overflow for key length %d", len(key))
+ }
+ if valueGas, overflow := SafeMul(gs.gasConfig.WriteCostPerByte, types.Gas(gs.valueLen(value))); !overflow {
+ gs.gasMeter.ConsumeGas(valueGas, types.GasWritePerByteDesc)
+ } else {
+ return fmt.Errorf("gas calculation overflow for value length %d", gs.valueLen(value))
+ }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
CHANGELOG.md (1)
Several PR links in the changelog are invalid or inaccessible
The verification process has identified multiple broken PR links in the changelog. These broken links could make it difficult for users to track the history of changes and access the detailed implementation discussions.
Key issues found:
- Invalid PR references: Measure average time consumed per each
Message#13981, crypto and x/auth: Add extra checks on signatures/pubkeys + check the signature first in antehandler #14372, Construct an example app to emulate etcd #238, [Epic]: Rosetta migration to standalone repo #16276, Add Query Module Configurations for AutoCLI #15901, [Epic]: ValidateBasic #15648, Type casting errors when using a different CommitMultiStore implementation #14019, Unhandled error when vesting locked amount > balance #13691, Bank Module Send Disabled Token Configuration #11859- Some of these PRs may have been merged and later deleted, or the PR numbers may be incorrect
Recommendation: These PR references should be verified and updated with correct links or additional context should be provided in the changelog entries themselves.
🔗 Analysis chain
Line range hint
1-1000: Verify all referenced PR linksThe changelog contains many PR reference links. We should verify these are all valid and accessible.
Based on the script output, I found that most PR links in the changelog are valid, but there are a few that are not found:
- Measure average time consumed per each
Message#13981, crypto and x/auth: Add extra checks on signatures/pubkeys + check the signature first in antehandler #14372, Construct an example app to emulate etcd #238, [Epic]: Rosetta migration to standalone repo #16276, Add Query Module Configurations for AutoCLI #15901, [Epic]: ValidateBasic #15648, Type casting errors when using a different CommitMultiStore implementation #14019, Unhandled error when vesting locked amount > balance #13691, Bank Module Send Disabled Token Configuration #11859 and a few othersLet me verify these PRs with a different approach to ensure they weren't just temporary API failures:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Extract PR numbers and verify they exist grep -o "#[0-9]\+" CHANGELOG.md | tr -d '#' | while read pr; do gh pr view "$pr" --repo cosmos/cosmos-sdk >/dev/null 2>&1 if [ $? -ne 0 ]; then echo "PR #$pr not found" fi doneLength of output: 67331
Script:
#!/bin/bash # Check specific PRs that failed in previous run for pr in 13981 14372 238 16276 15901 15648 14019 13691 11859; do echo "Checking PR #$pr..." gh pr view "$pr" --repo cosmos/cosmos-sdk --json number,state,title 2>/dev/null || echo "Not found" doneLength of output: 1937
♻️ Duplicate comments (1)
store/types/store.go (1)
489-492:⚠️ Potential issueFix incorrect comment for ObjectStoreKey
The comment incorrectly states that
ObjectStoreKeyis for transient stores.-// ObjectStoreKey is used for indexing transient stores in a MultiStore +// ObjectStoreKey is used for indexing object stores in a MultiStore type ObjectStoreKey struct {
🧹 Nitpick comments (6)
runtime/module.go (1)
246-251: Add Documentation Comment forProvideObjectStoreKeyPer Go conventions and the Uber Go Style Guide, exported functions should have a comment that begins with the function name to explain its purpose.
Consider adding:
// ProvideObjectStoreKey creates and registers a new ObjectStoreKey for the given module key. func ProvideObjectStoreKey(key depinject.ModuleKey, app *AppBuilder) *storetypes.ObjectStoreKey { storeKey := storetypes.NewObjectStoreKey(fmt.Sprintf("object:%s", key.Name())) registerStoreKey(app, storeKey) return storeKey }This enhances code readability and maintains consistency in documentation.
store/cachekv/store.go (2)
42-56: Add documentation for validation functionsConsider adding documentation for
isZeroandvalueLenfunction parameters to clarify their purpose and expected behavior.Add comments like:
type GStore[V any] struct { - // isZero is a function that returns true if the value is considered "zero", for []byte and pointers the zero value - // is `nil`, zero value is not allowed to set to a key, and it's returned if the key is not found. + // isZero is a function that returns true if the value is considered "zero". + // For []byte and pointers, the zero value is `nil`. + // Zero values are not allowed to be set to a key and are returned when a key is not found. isZero func(V) bool zeroValue V - // valueLen validates the value before it's set + // valueLen returns the length of the value and is used for validation before setting. + // For []byte, this is typically len(v). For other types, implement appropriate length calculation. valueLen func(V) int
Line range hint
119-138: Consider optimizing cache reset thresholdThe cache reset threshold of 100,000 items is hardcoded. Consider making this configurable and documenting the rationale for this specific value.
+// defaultCacheResetSize is the default threshold at which the cache is reset +// rather than cleared item by item. This value was chosen based on performance +// testing with Osmosis' InitGenesis. +const defaultCacheResetSize = 100_000 func (store *GStore[V]) resetCaches() { - if len(store.cache) > 100_000 { + if len(store.cache) > defaultCacheResetSize {baseapp/baseapp.go (1)
349-357: Add godoc for MountObjectStoresConsider adding detailed documentation for the
MountObjectStoresfunction to explain its purpose and usage.+// MountObjectStores mounts all object stores with the BaseApp's internal +// commit multi-store. Object stores are mounted in alphabetical order based on +// their keys to ensure deterministic ordering. +// +// Parameters: +// - keys: A map of store names to their corresponding ObjectStoreKey pointers func (app *BaseApp) MountObjectStores(keys map[string]*storetypes.ObjectStoreKey) {CHANGELOG.md (2)
Line range hint
1-1: Add title to the changelog fileThe changelog file should start with a clear title like "# Changelog" to follow standard markdown practices.
+ # Changelog
Line range hint
1-1000: Improve changelog formatting consistencyThe changelog has some inconsistencies in formatting:
- Some version entries are missing dates
- Version links are formatted differently in places
- Some bullet points use
*while others use-Consider:
- Adding dates for all versions
- Using consistent version link format
- Standardizing on
-for all bullet points
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.md(1 hunks)baseapp/baseapp.go(2 hunks)runtime/module.go(2 hunks)store/cachekv/store.go(11 hunks)store/gaskv/store.go(2 hunks)store/prefix/store.go(9 hunks)store/types/store.go(7 hunks)store/types/validity.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
runtime/module.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
baseapp/baseapp.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.
store/prefix/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
store/gaskv/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/cachekv/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/types/validity.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
store/cachekv/store.go
330-330: directive //nolint:staticcheck // We are in store v1. is unused for linter "staticcheck"
(nolintlint)
342-342: directive //nolint:staticcheck // We are in store v1. is unused for linter "staticcheck"
(nolintlint)
385-385: directive //nolint:staticcheck // We are in store v1. is unused for linter "staticcheck"
(nolintlint)
389-389: directive //nolint:staticcheck // We are in store v1. is unused for linter "staticcheck"
(nolintlint)
396-396: directive //nolint:staticcheck // We are in store v1. is unused for linter "staticcheck"
(nolintlint)
🔇 Additional comments (14)
store/prefix/store.go (5)
13-16: Type Definitions are Clear and Appropriate
The type aliases for Store and ObjStore enhance code readability and maintainability by providing clear abstractions.
23-29: Proper Implementation of NewStore Function
The NewStore function correctly initializes a new Store instance using generics, which improves type safety and flexibility.
31-37: Consistent Use of Generics in NewObjStore
The NewObjStore function mirrors NewStore with appropriate adjustments for object stores, maintaining consistency across the codebase.
42-47: Well-Defined Generic GStore Struct
The definition of GStore[V any] is clear, and the addition of isZero and valueLen functions enhances its utility for different value types.
50-59: Efficient Initialization in NewGStore
The NewGStore function effectively constructs a new GStore instance with the provided parameters, promoting reusability.
store/types/validity.go (2)
41-44: Consider Returning Errors Instead of Panicking in AssertValidValueLength
As previously noted, returning errors provides better control flow than panicking when validation fails.
34-39: 🛠️ Refactor suggestion
Consider Returning Errors Instead of Panicking in AssertValidValueGeneric
Using panic for input validation can be disruptive. It's better to return an error to allow callers to handle it gracefully.
Modify the function to return an error:
-func AssertValidValueGeneric[V any](value V, isZero func(V) bool, valueLen func(V) int) {
+func AssertValidValueGeneric[V any](value V, isZero func(V) bool, valueLen func(V) int) error {
if isZero(value) {
- panic("value is nil")
+ return errors.New("value is nil")
}
AssertValidValueLength(valueLen(value))
+ return nil
}Update calling functions to handle the error accordingly.
Likely invalid or redundant comment.
store/gaskv/store.go (3)
73-73: Address Potential Gas Calculation Overflow
The gas calculation may overflow when multiplying large values. Consider using safe arithmetic operations to prevent this.
85-85: Address Potential Gas Calculation Overflow in Set Method
Similar to the previous point, ensure that gas calculations in the Set method are overflow-safe.
206-206: Address Potential Gas Calculation Overflow in Iterator
As noted before, the gas calculation in the iterator could overflow. Implement safe math operations to handle large values.
store/cachekv/store.go (1)
20-28: LGTM: Clean generic type definitions
The generic type definitions are well-structured and follow Go best practices for generics.
store/types/store.go (2)
247-320: LGTM: Well-designed generic store interfaces
The generic store interfaces are well-structured with clear separation of concerns between basic and extended functionality.
610-622: Add changelog entry for new object store functionality
Please add an entry in the store/CHANGELOG.md for the new object store functionality.
CHANGELOG.md (1)
Line range hint 2-2: LGTM: Clear version organization
The changelog is well organized with clear version sections and release dates.
| func (s GStore[V]) CacheWrapWithTrace(w io.Writer, tc types.TraceContext) types.CacheWrap { | ||
| if store, ok := any(s).(*GStore[[]byte]); ok { | ||
| return cachekv.NewGStore(tracekv.NewStore(store, w, tc), store.isZero, store.valueLen) | ||
| } | ||
| return s.CacheWrap() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor Type Assertion in CacheWrapWithTrace Method
Consider modifying the type assertion for better clarity and correctness. The current implementation may lead to unintended behavior due to value receiver semantics.
Change the receiver to a pointer to avoid unnecessary copying and to ensure accurate type assertion:
-func (s GStore[V]) CacheWrapWithTrace(w io.Writer, tc types.TraceContext) types.CacheWrap {
+func (s *GStore[V]) CacheWrapWithTrace(w io.Writer, tc types.TraceContext) types.CacheWrap {
if store, ok := any(s).(*GStore[[]byte]); ok {
return cachekv.NewGStore(tracekv.NewStore(store, w, tc), store.isZero, store.valueLen)
}
return s.CacheWrap()
}This adjustment ensures the type assertion operates on the pointer receiver, aligning with Go best practices.
📝 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.
| func (s GStore[V]) CacheWrapWithTrace(w io.Writer, tc types.TraceContext) types.CacheWrap { | |
| if store, ok := any(s).(*GStore[[]byte]); ok { | |
| return cachekv.NewGStore(tracekv.NewStore(store, w, tc), store.isZero, store.valueLen) | |
| } | |
| return s.CacheWrap() | |
| func (s *GStore[V]) CacheWrapWithTrace(w io.Writer, tc types.TraceContext) types.CacheWrap { | |
| if store, ok := any(s).(*GStore[[]byte]); ok { | |
| return cachekv.NewGStore(tracekv.NewStore(store, w, tc), store.isZero, store.valueLen) | |
| } | |
| return s.CacheWrap() |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
store/types/validity.go (2)
33-39: 🛠️ Refactor suggestionConsider returning errors instead of panicking in validation functions.
Using
panicfor input validation can make error handling less flexible. Returning an error allows callers to handle invalid input more gracefully.Apply this diff to modify the function:
-func AssertValidValueGeneric[V any](value V, isZero func(V) bool, valueLen func(V) int) { - if isZero(value) { - panic("value is nil") - } - AssertValidValueLength(valueLen(value)) -} +func AssertValidValueGeneric[V any](value V, isZero func(V) bool, valueLen func(V) int) error { + if isZero(value) { + return errors.New("value is nil") + } + return AssertValidValueLength(valueLen(value)) +}And update the calling functions to handle the error accordingly.
41-44: 🛠️ Refactor suggestionReturn errors instead of panicking in
AssertValidValueLength.Consistent with the previous suggestion, returning an error improves error handling by avoiding panics.
Apply this diff to modify the function:
-func AssertValidValueLength(l int) { +func AssertValidValueLength(l int) error { if l > MaxValueLength { - panic(errors.New("value is too large")) + return errors.New("value is too large") } + return nil }Ensure to handle the returned error in functions that call
AssertValidValueLength.store/gaskv/store.go (3)
73-73:⚠️ Potential issuePrevent potential integer overflow in gas calculation for
Getmethod.The multiplication in gas consumption
gs.gasConfig.ReadCostPerByte * types.Gas(gs.valueLen(value))could overflow for large values. Use safe multiplication to prevent this.Apply this diff:
- gs.gasMeter.ConsumeGas(gs.gasConfig.ReadCostPerByte*types.Gas(gs.valueLen(value)), types.GasReadPerByteDesc) + if valueGas, overflow := safeMul(gs.gasConfig.ReadCostPerByte, types.Gas(gs.valueLen(value))); overflow { + panic("gas calculation overflow for value length") + } + gs.gasMeter.ConsumeGas(valueGas, types.GasReadPerByteDesc)Implement
safeMulto handle overflow checks.
85-85:⚠️ Potential issueAddress possible overflow in gas calculation for
Setmethod.Similar to the
Getmethod, the gas calculation may overflow with large keys or values. Apply overflow checks.Apply this diff:
- gs.gasMeter.ConsumeGas(gs.gasConfig.WriteCostPerByte*types.Gas(gs.valueLen(value)), types.GasWritePerByteDesc) + if valueGas, overflow := safeMul(gs.gasConfig.WriteCostPerByte, types.Gas(gs.valueLen(value))); overflow { + panic("gas calculation overflow for value length") + } + gs.gasMeter.ConsumeGas(valueGas, types.GasWritePerByteDesc)Ensure to define
safeMulfor overflow-safe multiplication.
200-206:⚠️ Potential issueEnsure gas calculation in
consumeSeekGasis overflow-safe.The gas consumption calculations for keys and values might overflow for large lengths. Implement overflow checks to prevent this.
Apply this diff:
- gi.gasMeter.ConsumeGas(gi.gasConfig.ReadCostPerByte*types.Gas(len(key)), types.GasValuePerByteDesc) + if keyGas, overflow := safeMul(gi.gasConfig.ReadCostPerByte, types.Gas(len(key))); overflow { + panic("gas calculation overflow for key length") + } + gi.gasMeter.ConsumeGas(keyGas, types.GasValuePerByteDesc) - gi.gasMeter.ConsumeGas(gi.gasConfig.ReadCostPerByte*types.Gas(gi.valueLen(value)), types.GasValuePerByteDesc) + if valueGas, overflow := safeMul(gi.gasConfig.ReadCostPerByte, types.Gas(gi.valueLen(value))); overflow { + panic("gas calculation overflow for value length") + } + gi.gasMeter.ConsumeGas(valueGas, types.GasValuePerByteDesc)Implement
safeMulto handle overflow securely.
🧹 Nitpick comments (16)
store/cachekv/store.go (3)
42-56: Consider documenting generic type parameter constraints.While the generic implementation is correct, it would be helpful to document the constraints or expectations for type parameter
V.Add documentation for the generic type parameter:
// GStore wraps an in-memory cache around an underlying types.KVStore. +// Type parameter V represents the value type stored in the cache. +// V must be comparable and should implement appropriate serialization methods. type GStore[V any] struct {
Line range hint
119-138: Consider parameterizing the cache size threshold.The hardcoded value of 100,000 for cache size threshold could be made configurable to allow different applications to tune based on their needs.
Consider making this configurable:
+// DefaultCacheSize is the default maximum number of entries in the cache +// before it is reset to prevent memory issues. +const DefaultCacheSize = 100_000 + func (store *GStore[V]) resetCaches() { - if len(store.cache) > 100_000 { + if len(store.cache) > DefaultCacheSize {
Line range hint
330-342: Remove unused nolint directives.The staticcheck nolint directives appear to be unused according to the static analysis hints.
Remove the unnecessary nolint directives:
- unsorted := make([]*kvPair[V], 0) //nolint:staticcheck // We are in store v1. + unsorted := make([]*kvPair[V], 0)🧰 Tools
🪛 golangci-lint (1.62.2)
330-330: directive
//nolint:staticcheck // We are in store v1.is unused for linter "staticcheck"(nolintlint)
CHANGELOG.md (4)
Line range hint
1-1: Add missing version release datesSome version entries in the changelog are missing their release dates (e.g. v0.50.0). Consider adding release dates for all versions to maintain consistency and help users track the timeline of changes.
Line range hint
1-2000: Standardize PR reference formatThe PR references are inconsistent throughout the changelog. Some use full URLs like
[#13576](https://github.com/cosmos/cosmos-sdk/pull/13576)while others use just the PR number like[#9246]. Consider standardizing to use full URLs consistently for better navigation.
Line range hint
1-2000: Improve change description claritySome change descriptions are too terse or use technical jargon without explanation. Consider adding brief explanations for technical changes to help users understand the impact. For example, the entry "Add
MarshalInterfaceandUnmarshalInterface" could explain what these interfaces are used for.
Line range hint
1-2000: Add section navigation linksThe changelog is quite long. Consider adding a table of contents at the top with links to each major version section to improve navigation.
store/prefix/store.go (5)
32-37: Add documentation comment for exported functionNewGStore.The
NewGStorefunction is exported but lacks a documentation comment. According to the Uber Go Style Guide, all exported functions should have comments explaining their purpose and usage.Apply this diff to add a documentation comment:
+// NewGStore creates a new generic store with the given parent store, prefix, and value handlers. func NewGStore[V any]( parent types.GKVStore[V], prefix []byte, isZero func(V) bool, valueLen func(V) int, ) GStore[V] {
Line range hint
70-74: Simplify function by removing unnecessary named return variable.In the
keymethod, the named return variableresis not necessary and can be omitted for clarity.Apply this diff to simplify the function:
-func (s GStore[V]) key(key []byte) (res []byte) { +func (s GStore[V]) key(key []byte) []byte { if key == nil { panic("nil key on Store") } - res = cloneAppend(s.prefix, key) - return + return cloneAppend(s.prefix, key) }
85-93: Simplify type assertion inCacheWrapWithTracemethod.The type assertion using
any(s)can be simplified sincesis already of typeGStore[V]. This improves readability and reduces unnecessary complexity.Apply this diff to simplify the method:
func (s GStore[V]) CacheWrapWithTrace(w io.Writer, tc types.TraceContext) types.CacheWrap { - if store, ok := any(s).(*GStore[[]byte]); ok { - return cachekv.NewGStore(tracekv.NewStore(store, w, tc), store.isZero, store.valueLen) - } - return s.CacheWrap() + return cachekv.NewGStore(tracekv.NewStore(s, w, tc), s.isZero, s.valueLen) }
Line range hint
185-188: Correct misleading comment inNextmethod ofprefixIterator.The comment suggests setting
pitonil, but the code sets thevalidflag tofalse. Update the comment to reflect the actual behavior.Apply this diff to fix the comment:
if pi.iter.Next(); !pi.iter.Valid() || !bytes.HasPrefix(pi.iter.Key(), pi.prefix) { - // TODO: shouldn't pi be set to nil instead? + // Mark the iterator as invalid when the end is reached. pi.valid = false }
Line range hint
223-230: Adjust error handling inErrormethod ofprefixIterator.Returning an error when the iterator is invalid may not be appropriate, as reaching the end of an iteration is not necessarily an error condition.
Apply this diff to modify the error handling:
func (pi *prefixIterator[V]) Error() error { - if !pi.Valid() { - return errors.New("invalid prefixIterator") - } - return nil + return pi.iter.Error() }store/types/validity.go (1)
3-4: Separate standard library imports from third-party imports.The import of the
"errors"package should be in a separate import block for standard library packages, as per the Uber Go Style Guide.Apply this diff to adjust the import grouping:
package types -import "errors" +import ( + "errors" +) var (store/gaskv/store.go (1)
118-123: Improve error messages for unimplementedCacheWrapmethods.The panic messages for
CacheWrapandCacheWrapWithTracecan be more descriptive to aid debugging.Apply this diff to enhance the messages:
func (gs *GStore[V]) CacheWrap() types.CacheWrap { - panic("cannot CacheWrap a GasKVStore") + panic("CacheWrap is not supported for GasKVStore") } func (gs *GStore[V]) CacheWrapWithTrace(_ io.Writer, _ types.TraceContext) types.CacheWrap { - panic("cannot CacheWrapWithTrace a GasKVStore") + panic("CacheWrapWithTrace is not supported for GasKVStore") }runtime/module.go (2)
246-250: Add documentation comment for exported functionProvideObjectStoreKey.The
ProvideObjectStoreKeyfunction is exported but lacks a documentation comment. Adding a comment improves code readability and provides clarity on its purpose.Apply this diff to add a documentation comment:
+// ProvideObjectStoreKey creates a new ObjectStoreKey for the given module key and registers it with the AppBuilder. func ProvideObjectStoreKey(key depinject.ModuleKey, app *AppBuilder) *storetypes.ObjectStoreKey { storeKey := storetypes.NewObjectStoreKey(fmt.Sprintf("object:%s", key.Name())) registerStoreKey(app, storeKey) return storeKey }
247-247: Use consistent formatting infmt.Sprintfcalls.For clarity, consider using
%qto quote the module name in the store key.Apply this diff:
- storeKey := storetypes.NewObjectStoreKey(fmt.Sprintf("object:%s", key.Name())) + storeKey := storetypes.NewObjectStoreKey(fmt.Sprintf("object:%q", key.Name()))This change makes it clear that the module name is a string value within the key.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
go.sumis excluded by!**/*.sumsimapp/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
CHANGELOG.md(1 hunks)baseapp/baseapp.go(2 hunks)go.mod(1 hunks)runtime/module.go(2 hunks)simapp/go.mod(1 hunks)simapp/v2/go.mod(1 hunks)store/cachekv/store.go(11 hunks)store/gaskv/store.go(2 hunks)store/prefix/store.go(9 hunks)store/types/store.go(7 hunks)store/types/validity.go(2 hunks)tests/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/go.mod
🧰 Additional context used
📓 Path-based instructions (8)
store/types/validity.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
baseapp/baseapp.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
runtime/module.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/gaskv/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
store/types/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/cachekv/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/prefix/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
store/cachekv/store.go
330-330: directive //nolint:staticcheck // We are in store v1. is unused for linter "staticcheck"
(nolintlint)
342-342: directive //nolint:staticcheck // We are in store v1. is unused for linter "staticcheck"
(nolintlint)
385-385: directive //nolint:staticcheck // We are in store v1. is unused for linter "staticcheck"
(nolintlint)
389-389: directive //nolint:staticcheck // We are in store v1. is unused for linter "staticcheck"
(nolintlint)
396-396: directive //nolint:staticcheck // We are in store v1. is unused for linter "staticcheck"
(nolintlint)
🔇 Additional comments (10)
go.mod (1)
188-190: LGTM: Module replacements are correctly configured.
The replace directives appropriately point to local module paths, which is necessary for local development and testing.
store/cachekv/store.go (2)
20-28: LGTM: Generic type definitions are well structured.
The introduction of generics through cValue[V] and kvPair[V] improves type safety while maintaining flexibility.
94-97: Verify value validation logic.
The validation of generic values relies on external validation functions. We should ensure these functions are properly implemented across all value types.
✅ Verification successful
Let me gather more information about the validation implementation.
Value validation logic is properly implemented and consistent
The validation logic is well-structured and consistently used across different store implementations:
- The generic validation in
AssertValidValueGenericcorrectly handles both nil checks and size limits - Two types of concrete implementations are consistently used throughout the codebase:
- For
[]byte: Usesnilcheck andlen()for size - For generic objects: Uses
nilcheck and constant size (1 or ObjectValueLength)
- For
- All store implementations properly pass these validation functions during initialization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for implementations of isZero and valueLen functions
# to verify consistent validation across different value types
# Search for isZero implementations
rg -A 5 'func.*isZero.*\(.*\).*bool'
# Search for valueLen implementations
rg -A 5 'func.*valueLen.*\(.*\).*int'
Length of output: 4292
Script:
#!/bin/bash
# Let's check the actual validation logic in types/validity.go
cat store/types/validity.go
# Also search for any concrete implementations or tests that show how these functions are used
rg -A 5 'NewGStore.*\('
Length of output: 6095
simapp/go.mod (1)
255-255: LGTM: Store module replacement is correctly configured.
The replace directive for the store module is properly aligned with the local development setup.
simapp/v2/go.mod (1)
307-307: LGTM!
The replace directive for cosmossdk.io/store pointing to a local path is appropriate for local development and testing in a monorepo setup.
store/types/store.go (3)
247-316: Well-structured generic store interfaces!
The introduction of generics improves type safety while maintaining backward compatibility through type aliases. The interfaces are well-documented and follow Go's best practices.
489-492: Fix incorrect documentation for ObjectStoreKey
The comment incorrectly states that ObjectStoreKey is used for indexing transient stores.
Apply this diff to fix the documentation:
-// ObjectStoreKey is used for indexing transient stores in a MultiStore
+// ObjectStoreKey is used for indexing object stores in a MultiStore
type ObjectStoreKey struct {
name string
}610-622: LGTM: Well-implemented constructor with safety checks
The NewObjectStoreKeys constructor correctly:
- Checks for name conflicts using
assertNoCommonPrefix - Returns a map of pointers following the capability-based security principle
- Maintains consistency with other store key constructors
baseapp/baseapp.go (2)
308-310: LGTM: Proper integration of ObjectStoreKey
The ObjectStoreKey case is correctly integrated into the MountStores method, maintaining consistency with other store types.
349-357: LGTM: Well-implemented MountObjectStores method
The implementation:
- Follows the established pattern of other Mount* methods
- Uses sorted keys for deterministic mounting order
- Properly delegates to the underlying MountStore method
Description
Upstream crypto-org-chain#206 as it's needed for integrating with block stm
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
ObjectStoreAPI for enhanced data management.Bug Fixes
Improvements
Documentation