feat: implement object stores #22893
Conversation
|
Warning Rate limit exceeded@mmsqe has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request introduces significant changes to the Cosmos SDK's store package, focusing on implementing generics and enhancing type safety across various store implementations. The changes include supporting object stores, introducing generic key-value stores, and updating iterator mechanisms. The modifications span multiple files and modules, with a primary emphasis on making the store package more flexible and type-safe through the use of Go generics. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant BaseApp as BaseApp
participant StoreKey as ObjectStoreKey
participant ObjectStore as Object Store
App->>BaseApp: Mount Object Stores
BaseApp->>StoreKey: Create Object Store Key
StoreKey-->>BaseApp: Return Store Key
BaseApp->>ObjectStore: Initialize Store
ObjectStore-->>BaseApp: Store Ready
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
|
generic interface generic btree generic cachekv generic transient store support ObjStore changelog Update CHANGELOG.md Signed-off-by: yihuang <huang@crypto.com> 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/exp@v0.0.0-20241108190413-2d47ceb2692f?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.
github-actions
bot
added
C:x/staking
C:CLI
C:x/gov
C:x/slashing
C:x/distribution
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
CHANGELOG.md (4)
Line range hint
1-1: Add title to the CHANGELOG fileThe CHANGELOG file should start with a # CHANGELOG title at the top for better documentation.
+ # CHANGELOG
Line range hint
1-15: Documentation formatting can be improvedThe guiding principles section should be formatted as a proper markdown list for better readability.
- Guiding Principles: + # Guiding Principles + + * Changelogs are for humans, not machines. + * There should be an entry for every single version. + * The same types of changes should be grouped. + * Versions and sections should be linkable. + * The latest version comes first. + * The release date of each version is displayed. + * Mention whether you follow Semantic Versioning.
Line range hint
17-27: Usage section needs better formattingThe usage section should use proper markdown formatting with code blocks for examples.
- Change log entries are to be added to the Unreleased section under the - appropriate stanza (see below). Each entry is required to include a tag and - the Github issue reference in the following format: + # Usage + + Change log entries should be added to the "Unreleased" section under the + appropriate stanza (see below). Each entry must include a tag and + the Github issue reference in the following format: + + ``` + * (<tag>) \#<issue-number> message + ```
Line range hint
29-38: Types of changes section needs better organizationThe stanza types should be organized in a table or list with descriptions.
- Types of changes (Stanzas): + ## Types of Changes + + | Stanza | Description | + |--------|-------------| + | Features | New features | + | Improvements | Changes in existing functionality | + | Deprecated | Soon-to-be removed features | + | Bug Fixes | Bug fixes | + | Client Breaking | Breaking Protobuf/gRPC/REST routes | + | CLI Breaking | Breaking CLI commands | + | API Breaking | Breaking exported APIs | + | State Machine Breaking | Changes affecting the application state |
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (24)
client/v2/go.sumis excluded by!**/*.sumserver/v2/cometbft/go.sumis excluded by!**/*.sumsimapp/go.sumis excluded by!**/*.sumtools/benchmark/go.sumis excluded by!**/*.sumx/accounts/defaults/base/go.sumis excluded by!**/*.sumx/accounts/defaults/lockup/go.sumis excluded by!**/*.sumx/accounts/defaults/multisig/go.sumis excluded by!**/*.sumx/accounts/go.sumis excluded by!**/*.sumx/authz/go.sumis excluded by!**/*.sumx/bank/go.sumis excluded by!**/*.sumx/circuit/go.sumis excluded by!**/*.sumx/consensus/go.sumis excluded by!**/*.sumx/distribution/go.sumis excluded by!**/*.sumx/epochs/go.sumis excluded by!**/*.sumx/evidence/go.sumis excluded by!**/*.sumx/feegrant/go.sumis excluded by!**/*.sumx/gov/go.sumis excluded by!**/*.sumx/group/go.sumis excluded by!**/*.sumx/mint/go.sumis excluded by!**/*.sumx/nft/go.sumis excluded by!**/*.sumx/protocolpool/go.sumis excluded by!**/*.sumx/slashing/go.sumis excluded by!**/*.sumx/staking/go.sumis excluded by!**/*.sumx/upgrade/go.sumis excluded by!**/*.sum
📒 Files selected for processing (28)
CHANGELOG.md(1 hunks)baseapp/baseapp.go(2 hunks)client/v2/go.mod(2 hunks)go.mod(3 hunks)runtime/module.go(2 hunks)server/v2/cometbft/go.mod(2 hunks)simapp/go.mod(1 hunks)tools/benchmark/go.mod(2 hunks)x/accounts/defaults/base/go.mod(2 hunks)x/accounts/defaults/lockup/go.mod(2 hunks)x/accounts/defaults/multisig/go.mod(2 hunks)x/accounts/go.mod(2 hunks)x/authz/go.mod(2 hunks)x/bank/go.mod(2 hunks)x/circuit/go.mod(2 hunks)x/consensus/go.mod(2 hunks)x/distribution/go.mod(2 hunks)x/epochs/go.mod(2 hunks)x/evidence/go.mod(2 hunks)x/feegrant/go.mod(2 hunks)x/gov/go.mod(2 hunks)x/group/go.mod(2 hunks)x/mint/go.mod(2 hunks)x/nft/go.mod(2 hunks)x/protocolpool/go.mod(2 hunks)x/slashing/go.mod(2 hunks)x/staking/go.mod(2 hunks)x/upgrade/go.mod(2 hunks)
✅ Files skipped from review due to trivial changes (18)
- x/accounts/defaults/base/go.mod
- x/group/go.mod
- server/v2/cometbft/go.mod
- x/consensus/go.mod
- x/upgrade/go.mod
- x/feegrant/go.mod
- x/protocolpool/go.mod
- x/circuit/go.mod
- x/nft/go.mod
- x/bank/go.mod
- x/accounts/defaults/multisig/go.mod
- x/distribution/go.mod
- tools/benchmark/go.mod
- client/v2/go.mod
- x/gov/go.mod
- x/slashing/go.mod
- x/accounts/go.mod
- x/mint/go.mod
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- baseapp/baseapp.go
🧰 Additional context used
📓 Path-based instructions (2)
runtime/module.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"
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (9)
simapp/go.mod (1)
47-49: Verify security fix replace directiveThe replace directive for
github.com/gin-gonic/ginaddresses vulnerabilities GHSA-h395-qcrw-5vmq and GHSA-3vp4-m3rf-835h. Let's verify if v1.9.1 is still the latest secure version.✅ Verification successful
Security fix verified - v1.9.1 addresses all known vulnerabilities
The current replace directive using v1.9.1 is secure as it patches all known vulnerabilities, including the mentioned GHSA-h395-qcrw-5vmq and GHSA-3vp4-m3rf-835h. While v1.10.0 is now available, upgrading is optional as there are no known security issues in v1.9.1.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for latest version and security advisories for gin-gonic/gin # Check latest version from GitHub API echo "Latest release version:" gh api repos/gin-gonic/gin/releases/latest --jq .tag_name # Check for security advisories echo -e "\nSecurity advisories:" gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/gin-gonic/gin") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 1790
runtime/module.go (2)
246-250: LGTM! Clean implementation of object store key providerThe implementation follows the established patterns and properly integrates with the dependency injection system.
102-102: LGTM! Proper registration of the new providerThe ProvideObjectStoreKey is correctly registered in the module's init function.
CHANGELOG.md (1)
Line range hint
40-2000: Version entries are well organizedThe version entries follow a consistent format with clear sections for features, improvements, bug fixes and breaking changes. The organization is good and helps users understand changes between versions.
x/accounts/defaults/lockup/go.mod (1)
3-3: LGTM: Go version and IAVL dependency updates.The updates to Go 1.23.4 and IAVL v1.3.5 are consistent with the coordinated dependency update across all Cosmos SDK modules.
Let's verify the IAVL changelog for any breaking changes:
Also applies to: 59-59
x/evidence/go.mod (1)
3-3: LGTM: Consistent dependency updates.The updates to Go 1.23.4 and IAVL v1.3.5 match the changes in other modules.
Also applies to: 60-60
x/epochs/go.mod (1)
3-3: LGTM: Aligned with module-wide updates.The Go version and IAVL dependency updates are consistent with other modules.
Also applies to: 52-52
x/staking/go.mod (1)
3-3: LGTM: Synchronized dependency updates.The updates to Go 1.23.4 and IAVL v1.3.5 maintain consistency across the codebase.
Also applies to: 53-53
x/authz/go.mod (1)
3-3: LGTM: Uniform dependency updates.The Go version and IAVL dependency updates align with the changes across all modules.
Also applies to: 53-53
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
Based on the comprehensive summary, here are the release notes:
New Features
ObjectStoreAPI in the contextImprovements
Bug Fixes
Breaking Changes
testutil/networkpackageDependency Updates
github.com/cosmos/iavlto v1.3.5