feat: implement object stores #24108
Conversation
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 resolve Apply suggestions from code review cleanup
📝 WalkthroughWalkthroughThis pull request introduces support for mounting object stores in the BaseApp and adds a new ObjectStore API to the Context. In addition, it adjusts proposal processing, event logging in the group module, and bank module functionality (including a new SpendableBalances gRPC query). Public API changes include updates to the Calculator and runtime modules. A large set of store implementations (cachekv, gaskv, cachemulti, listenkv, prefix, rootmulti, tracekv, transient, etc.) have been refactored to leverage Go generics for improved type safety and flexibility. Finally, multiple go.mod files have been updated with new dependency versions and replace directives for the store module. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Context
participant MultiStore
participant ObjStoreAdapter
Client->>Context: Call ObjectStore(key)
Context->>MultiStore: GetObjKVStore(key)
MultiStore-->>Context: Return ObjKVStore instance
Context->>ObjStoreAdapter: Invoke gaskv.NewObjStore(ObjStore, gasMeter, transientKVGasConfig)
ObjStoreAdapter-->>Context: Return wrapped object store
Context-->>Client: Return ObjKVStore
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.64.8)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package sqlite3: could not load export data: no export data for "github.com/bvinc/go-sqlite-lite/sqlite3"" ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
|
github-actions
bot
added
C:x/staking
C:CLI
C:x/gov
C:x/slashing
C:x/distribution
github-actions
bot
added
C:x/accounts/multisig
C:server/v2 cometbft
C:x/accounts/base
labels
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| 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
| // AssertValidValueGeneric checks if the value is valid(value is not nil and within length limit) | ||
| func AssertValidValueGeneric[V any](value V, isZero func(V) bool, valueLen func(V) int) { | ||
| if isZero(value) { | ||
| panic("value is nil") |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (12)
x/staking/go.mod (1)
167-170: Update Replace Directives for Local Module Paths
The new replace block now mapscosmossdk.io/storeto../../storeandcosmossdk.io/x/bankto../bank. This aligns with the coordinated dependency management across modules. Please confirm that the relative paths are correct relative to the repository structure. Also, note that the accompanying TODO on line 166 indicates these directives are temporary until the modules are fully spun out.store/listenkv/store.go (1)
133-137: Update method documentation to match implementationThe documentation above the
CacheWrapmethod still indicates that the method panics, but the implementation has been changed to return a cache-wrapped store instead of panicking.Update the documentation to accurately reflect the new behavior:
- // CacheWrap implements the KVStore interface. It panics as a Store - // cannot be cache wrapped. + // CacheWrap implements the KVStore interface. It returns a cache-wrapped + // version of the store. func (s *Store) CacheWrap() types.CacheWrap { return cachekv.NewStore(s) }store/tracekv/store.go (1)
164-166: Updated CacheWrap implementation now delegates to parent storeThe method now properly implements the CacheWrapper interface by delegating to the parent store's CacheWrap method instead of panicking. This is a good improvement and aligns with the Store interface requiring CacheWrapper implementation.
However, there's an inconsistency with CacheWrapWithTrace (line 171) which still panics with "cannot CacheWrapWithTrace a TraceKVStore". Consider updating CacheWrapWithTrace to also delegate to the parent for consistency.
func (tkv *Store) CacheWrapWithTrace(_ io.Writer, _ types.TraceContext) types.CacheWrap { - panic("cannot CacheWrapWithTrace a TraceKVStore") + return tkv.parent.CacheWrapWithTrace(_, _) }store/tracekv/store_test.go (1)
284-287: Test updated to remove panic expectationThe test has been updated to align with the implementation change in
CacheWrap(). However, the test now only verifies thatCacheWrap()doesn't panic, but doesn't validate that it correctly delegates to the parent store.Consider enhancing this test to verify that the method properly delegates to the parent's CacheWrap method, perhaps by using a mock parent store or checking properties of the returned CacheWrap.
func TestTraceKVStoreCacheWrap(t *testing.T) { store := newEmptyTraceKVStore(nil) - store.CacheWrap() + cacheWrap := store.CacheWrap() + // Verify cacheWrap is not nil and has expected properties + require.NotNil(t, cacheWrap) + // Additional verification that it delegates correctly }store/internal/btreeadaptor.go (2)
25-28: Incorrect comment format for Has methodThe comment for the Has method doesn't follow the standard Go comment format. Function comments should be complete sentences that begin with the function name.
-// Hash Implements GKVStore. +// Has implements GKVStore. It checks if a key exists in the store. func (ts *BTreeStore[V]) Has(key []byte) bool { return !ts.isZero(ts.Get(key)) }
30-44: Iterator methods panic on errorsBoth Iterator methods panic when encountering errors from the underlying BTree. While this follows the pattern of other packages in this codebase, consider if this is the desired behavior or if returning an error would be more appropriate to allow for graceful error handling.
Additionally, consider adding validation for the input parameters.
types/context.go (1)
356-359: Fix the comment style for consistency.The implementation of
ObjectStorelooks good and properly follows the same pattern as the existing store methods. However, the comment ends with a comma instead of a period, which is inconsistent with other similar method comments.-// ObjectStore fetches an object store from the MultiStore, +// ObjectStore fetches an object store from the MultiStore.runtime/module.go (1)
246-250: Add config.SkipStoreKeys check for consistency.The
ProvideObjectStoreKeyfunction is missing theconfig.SkipStoreKeyscheck that exists in the other similar functions (ProvideKVStoreKey,ProvideTransientStoreKey, andProvideMemoryStoreKey). This creates an inconsistency with the established pattern.-func ProvideObjectStoreKey(key depinject.ModuleKey, app *AppBuilder) *storetypes.ObjectStoreKey { +func ProvideObjectStoreKey( + config *runtimev1alpha1.Module, + key depinject.ModuleKey, + app *AppBuilder, +) *storetypes.ObjectStoreKey { + if slices.Contains(config.SkipStoreKeys, key.Name()) { + return nil + } + storeKey := storetypes.NewObjectStoreKey(fmt.Sprintf("object:%s", key.Name())) registerStoreKey(app, storeKey) return storeKey }Also, the function is missing a comment describing its purpose, unlike other similar provider functions.
store/types/store.go (1)
303-311: Introduction of type aliases for typed vs. object-based stores
The new type aliases (Iterator,BasicKVStore,ObjIterator,ObjKVStore, etc.) neatly wrap the generic interfaces with specialized names. Consider verifying overhead for reflection-based usage inObjKVStore.store/prefix/store.go (1)
184-193: Next method and TODO
The TODO comment suggests potentially settingpito nil after invalidation. Consider whether future refactoring is needed to reflect that behavior or if the current approach suffices.store/gaskv/store.go (1)
9-10: ObjectValueLength constant
DefiningObjectValueLengthas the gas cost basis for object storage is a reasonable approximation. Consider revisiting to ensure the cost remains balanced if object usage expands.store/cachekv/store.go (1)
326-327: Formatting alert from static analysis.
Consider reformatting around line 326. Running Go format tools will address any spacing or ordering issues.🧰 Tools
🪛 golangci-lint (1.64.8)
326-326: File is not properly formatted
(gci)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (26)
client/v2/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumserver/v2/cometbft/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sumtests/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 (52)
CHANGELOG.md(1 hunks)baseapp/baseapp.go(2 hunks)client/v2/go.mod(3 hunks)go.mod(3 hunks)runtime/module.go(2 hunks)runtime/store.go(1 hunks)server/v2/cometbft/go.mod(3 hunks)simapp/v2/go.mod(3 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(4 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/prefix/store.go(9 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(7 hunks)store/types/validity.go(2 hunks)tests/go.mod(3 hunks)tools/benchmark/go.mod(2 hunks)types/context.go(1 hunks)x/accounts/defaults/base/go.mod(3 hunks)x/accounts/defaults/lockup/go.mod(2 hunks)x/accounts/defaults/multisig/go.mod(3 hunks)x/accounts/go.mod(3 hunks)x/authz/go.mod(3 hunks)x/bank/go.mod(3 hunks)x/circuit/go.mod(3 hunks)x/consensus/go.mod(3 hunks)x/distribution/go.mod(3 hunks)x/epochs/go.mod(3 hunks)x/evidence/go.mod(3 hunks)x/feegrant/go.mod(3 hunks)x/gov/go.mod(3 hunks)x/group/go.mod(3 hunks)x/mint/go.mod(3 hunks)x/nft/go.mod(3 hunks)x/protocolpool/go.mod(3 hunks)x/slashing/go.mod(3 hunks)x/staking/go.mod(3 hunks)x/upgrade/go.mod(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`**/*.md`: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
store/CHANGELOG.mdCHANGELOG.md
`**/*.go`: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/internal/btree/memiterator_test.gostore/tracekv/store_test.goruntime/module.gostore/types/validity.gostore/listenkv/store.gostore/tracekv/store.gotypes/context.gostore/listenkv/store_test.gostore/internal/btreeadaptor.gostore/internal/btree/btree_test.goruntime/store.gostore/cachekv/search_benchmark_test.gostore/cachemulti/store.gostore/internal/btree/btree.gostore/prefix/store.gostore/internal/btree/memiterator.gostore/transient/store.gostore/rootmulti/store_test.gostore/cachekv/internal/mergeiterator.gostore/rootmulti/store.gostore/gaskv/store.gostore/types/store.gostore/cachekv/store.gobaseapp/baseapp.go
`**/*_test.go`: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
store/internal/btree/memiterator_test.gostore/tracekv/store_test.gostore/listenkv/store_test.gostore/internal/btree/btree_test.gostore/cachekv/search_benchmark_test.gostore/rootmulti/store_test.go
`tests/**/*`: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
tests/go.mod
🧬 Code Definitions (14)
store/tracekv/store_test.go (2)
store/cachekv/store.go (15)
store(71-73)store(76-91)store(94-101)store(104-107)store(110-117)store(119-135)store(138-181)store(184-186)store(189-194)store(200-202)store(205-207)store(209-233)store(318-390)store(392-412)store(419-428)store/types/store.go (1)
CacheWrap(336-341)
store/tracekv/store.go (6)
store/cachemulti/store.go (1)
Store(24-31)store/cachekv/store.go (2)
Store(30-30)parent(218-218)store/gaskv/store.go (2)
Store(14-14)parent(128-128)store/types/store.go (2)
Store(16-19)CacheWrap(336-341)store/listenkv/store.go (2)
Store(15-19)parent(70-70)store/transient/store.go (2)
Store(29-31)Store(40-42)
store/internal/btreeadaptor.go (3)
store/types/store.go (11)
KVStore(306-306)key(449-451)key(453-455)key(471-473)key(476-478)key(494-496)key(499-501)key(513-515)key(518-520)Iterator(304-304)GIterator(275-301)store/internal/btree/btree.go (1)
BTree(25-27)store/cachekv/store.go (2)
err(217-217)NewGStore(59-68)
store/cachekv/search_benchmark_test.go (4)
store/cachekv/store.go (2)
cValue(20-23)GStore(43-56)store/gaskv/store.go (1)
GStore(34-41)store/transient/store.go (1)
GStore(19-21)store/internal/btree/btree.go (1)
NewBTree(30-37)
store/cachemulti/store.go (3)
store/types/store.go (5)
CacheWrap(336-341)CacheWrapper(343-349)TraceContext(526-526)Store(16-19)KVStore(306-306)store/cachekv/store.go (17)
store(71-73)store(76-91)store(94-101)store(104-107)store(110-117)store(119-135)store(138-181)store(184-186)store(189-194)store(200-202)store(205-207)store(209-233)store(318-390)store(392-412)store(419-428)Store(30-30)NewStore(34-40)store/tracekv/store.go (2)
Store(27-31)NewStore(47-49)
store/internal/btree/memiterator.go (1)
store/internal/btree/btree.go (5)
item(84-87)BTree(25-27)newItem(95-97)empty(44-44)empty(53-53)
store/transient/store.go (4)
store/types/store.go (4)
Committer(22-32)Store(16-19)KVStore(306-306)StoreType(363-363)store/gaskv/store.go (3)
Store(14-14)ObjStore(23-23)GStore(34-41)store/internal/btreeadaptor.go (8)
BTreeStore(14-18)NewBTreeStore(21-23)ts(26-28)ts(30-36)ts(38-44)ts(47-49)ts(52-54)ts(57-59)store/internal/btree/btree.go (1)
NewBTree(30-37)
store/rootmulti/store_test.go (2)
store/types/store.go (4)
CommitKVStore(324-327)NewKVStoreKey(427-434)StoreKey(410-413)CommitStore(45-48)store/rootmulti/store.go (2)
db(1054-1054)NewStore(91-105)
store/cachekv/internal/mergeiterator.go (3)
store/cachekv/store.go (2)
parent(218-218)err(217-217)store/gaskv/store.go (1)
parent(128-128)store/types/store.go (2)
GIterator(275-301)Iterator(304-304)
store/rootmulti/store.go (6)
store/types/store.go (17)
StoreKey(410-413)CommitStore(45-48)Store(16-19)key(449-451)key(453-455)key(471-473)key(476-478)key(494-496)key(499-501)key(513-515)key(518-520)CommitKVStore(324-327)KVStore(306-306)s(96-101)s(104-109)s(113-123)StoreTypeObject(373-373)store/iavl/store.go (7)
Store(36-40)_(27-27)_(28-28)_(29-29)_(30-30)_(31-31)_(32-32)store/cachekv/store.go (17)
Store(30-30)store(71-73)store(76-91)store(94-101)store(104-107)store(110-117)store(119-135)store(138-181)store(184-186)store(189-194)store(200-202)store(205-207)store(209-233)store(318-390)store(392-412)store(419-428)_(32-32)store/gaskv/store.go (3)
Store(14-14)ObjStore(23-23)_(12-12)store/listenkv/store.go (9)
Store(15-19)s(29-32)s(36-40)s(44-47)s(51-53)s(57-59)s(63-65)s(69-79)_(10-10)store/transient/store.go (6)
Store(29-31)Store(40-42)ObjStore(45-47)ObjStore(56-58)_(11-11)_(12-12)
store/gaskv/store.go (9)
store/types/store.go (14)
KVStore(306-306)Store(16-19)GKVStore(254-272)key(449-451)key(453-455)key(471-473)key(476-478)key(494-496)key(499-501)key(513-515)key(518-520)Iterator(304-304)GIterator(275-301)TraceContext(526-526)store/cachekv/store.go (5)
Store(30-30)GStore(43-56)parent(218-218)NewGStore(59-68)_(32-32)store/prefix/store.go (6)
Store(14-14)GStore(42-48)NewGStore(50-61)_(19-19)_(20-20)_(153-153)store/cachemulti/store.go (2)
Store(24-31)_(33-33)store/listenkv/store.go (3)
Store(15-19)parent(70-70)_(10-10)store/tracekv/store.go (2)
Store(27-31)parent(96-96)store/types/validity.go (1)
AssertValidValueGeneric(34-39)store/internal/btree/memiterator.go (1)
_(12-12)store/internal/btreeadaptor.go (1)
_(11-11)
store/types/store.go (5)
store/prefix/store.go (4)
Store(14-14)_(19-19)_(20-20)_(153-153)store/rootmulti/store.go (3)
Store(59-80)_(83-83)_(84-84)store/iavl/store.go (7)
Store(36-40)_(27-27)_(28-28)_(29-29)_(30-30)_(31-31)_(32-32)store/cachemulti/store.go (2)
Store(24-31)_(33-33)store/transient/store.go (2)
Store(29-31)Store(40-42)
store/cachekv/store.go (4)
store/types/store.go (14)
CacheKVStore(316-321)Store(16-19)KVStore(306-306)GKVStore(254-272)key(449-451)key(453-455)key(471-473)key(476-478)key(494-496)key(499-501)key(513-515)key(518-520)Iterator(304-304)GIterator(275-301)store/internal/btree/btree.go (2)
BTree(25-27)NewBTree(30-37)store/types/validity.go (1)
AssertValidValueGeneric(34-39)store/cachekv/internal/mergeiterator.go (1)
NewCacheMergeIterator(29-39)
baseapp/baseapp.go (1)
store/types/store.go (10)
ObjectStoreKey(481-483)key(449-451)key(453-455)key(471-473)key(476-478)key(494-496)key(499-501)key(513-515)key(518-520)StoreTypeObject(373-373)
🪛 GitHub Check: CodeQL
store/types/validity.go
[warning] 36-36: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
store/cachemulti/store.go
[warning] 177-177: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
store/rootmulti/store.go
[warning] 697-697: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
🪛 golangci-lint (1.64.8)
store/cachekv/store.go
326-326: File is not properly formatted
(gci)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (201)
x/circuit/go.mod (3)
57-57: Updategithub.com/cosmos/iavlDependency Version
The dependency version has been upgraded to v1.3.5 consistently with other modules. Please verify that this version is fully compatible with the rest of the codebase.
86-86: Upgradegithub.com/hashicorp/go-pluginDependency
The version update to v1.6.3 aligns with the coordinated dependency changes across the project. Confirm that this update introduces no breaking changes for plugin usage.
164-169: New Replace Directive forcosmossdk.io/store
A new replace directive mappingcosmossdk.io/storeto../../storehas been added to facilitate local development. Please verify that the relative path is correct within the project’s module structure.x/accounts/defaults/lockup/go.mod (2)
58-58: Updategithub.com/cosmos/iavlDependency
The dependency update to v1.3.5 is in line with similar changes elsewhere. No further issues were detected.
156-162: New Replace Directives for Module Paths
The replace block now mapscosmossdk.io/storeto../../../../storealong with updates for other modules. Ensure that these relative paths correctly reflect the directory layout for proper resolution during builds.x/nft/go.mod (3)
57-57: Updategithub.com/cosmos/iavlDependency Version
The upgrade to v1.3.5 is consistent with the broader dependency updates in the repository.
86-86: Upgradegithub.com/hashicorp/go-pluginDependency
The dependency has been updated to v1.6.3. Please ensure that this change does not introduce incompatibilities with existing plugin interfaces.
162-168: Introduce New Replace Directive forcosmossdk.io/store
Mappingcosmossdk.io/storeto../../storesupports local development and testing. Confirm that the provided relative paths (also forx/bankandx/staking) are correct.x/accounts/defaults/multisig/go.mod (3)
54-54: Updategithub.com/cosmos/iavlDependency Version
Upgraded to v1.3.5 to maintain consistency. No issues observed.
80-80: Upgradegithub.com/hashicorp/go-pluginDependency
The version is now v1.6.3; please double-check compatibility with any code utilizing this package.
163-168: New Replace Directives for Module Paths
The new replace block—including the mapping forcosmossdk.io/storeto../../../../store—ensures proper resolution given the module's deeper directory structure. Verify that these relative paths are correct within the project.x/authz/go.mod (2)
51-51: Updategithub.com/cosmos/iavlDependency Version
The dependency now uses v1.3.5, which is consistent with updates across the repository.
170-176: New Replace Directive forcosmossdk.io/storeand Related Modules
A new replace block mapscosmossdk.io/storeto../../store(along with adjustments forx/bankandx/staking). Please ensure these relative paths match the intended project layout and work correctly in the build environment.x/consensus/go.mod (3)
56-56: Dependency Version Update for cosmos/iavl
The dependencygithub.com/cosmos/iavlis now updated to versionv1.3.5. Please verify that this minor version bump is compatible with all dependent components in the consensus module.
85-85: Dependency Version Update for go-plugin
github.com/hashicorp/go-pluginhas been updated tov1.6.3. Confirm that this change does not introduce any incompatibilities in the consensus module’s integration with external plugins.
163-167: Local Replace Directive for Store Module
A new replace directive mapscosmossdk.io/storeto the local path../../store, while the bank and staking modules are similarly remapped. Please double-check that these relative paths accurately reflect the repository’s layout and that all modules build correctly with these changes.x/evidence/go.mod (2)
59-59: Update for cosmos/iavl Dependency
Thegithub.com/cosmos/iavldependency is now set to versionv1.3.5. Ensure that this updated version is fully compatible with the evidence module’s functionality.
165-167: Confirm Replace Directive for Store Module
The replace directive mappingcosmossdk.io/storeto../../store(along with the mappings forcosmossdk.io/x/bankandcosmossdk.io/x/staking) is consistent with our local development flow. Please verify that these relative paths are correct within the evidence module’s context.x/accounts/go.mod (2)
64-64: Updated Dependency for cosmos/iavl
The accounts module now usesgithub.com/cosmos/iavl v1.3.5. This version update should harmonize with changes across the repository; please ensure there are no breaking changes for account-related operations.
171-174: Revised Replace Directives for Local Modules
The added replace directives forcosmossdk.io/store,cosmossdk.io/x/bank, andcosmossdk.io/x/stakingnow point to local paths (../../store,../bank, and../staking, respectively). Double-check these paths to ensure they correctly resolve in the accounts module.x/protocolpool/go.mod (2)
59-59: Dependency Update for cosmos/iavl in Protocol Pool
The protocol pool now depends ongithub.com/cosmos/iavl v1.3.5. This update aligns with other modules; please confirm that no protocol pool functionality is adversely impacted by this version change.
165-168: Local Path Mapping for Modules in Protocol Pool
The replace directive remapscosmossdk.io/storeto../../storealong with the corresponding bank and staking modules. Verify that these local paths are correct relative to the protocol pool’s location and that they support smooth local development.client/v2/go.mod (2)
67-67: Updated Dependency for cosmos/iavl
Theclient/v2module now updatesgithub.com/cosmos/iavltov1.3.5, matching the rest of the codebase. Please check that this update does not affect the client functionality.
170-174: Replace Directives for Client Module
In the client module, the replace directive mapscosmossdk.io/storeto../../storeand uses relative paths (./../../x/bankand./../../x/staking) for the bank and staking modules. These paths differ slightly from other modules due to the client’s directory structure. Please confirm that these paths are correct and that they integrate seamlessly during builds and tests.go.mod (3)
83-83: Dependency Update: cosmos/iavl version bumped to v1.3.5
The update from v1.3.4 to v1.3.5 ensures consistency with other modules. Please verify that no breaking changes are introduced downstream.
108-108: Dependency Update: hashicorp/go-plugin version bumped to v1.6.3
This change aligns with the updates in other modules. Ensure that any integration relying on this dependency has been tested against the new version.
178-178: New Replace Directive: mapping cosmossdk.io/store to ./store
The local replacement helps with development and testing; please confirm that the relative path is correct for all development environments.x/bank/go.mod (3)
53-53: Dependency Update: cosmos/iavl version updated to v1.3.5
The update is consistent with repository-wide changes. Please verify that the updated version does not cause integration issues.
77-77: Dependency Update: hashicorp/go-plugin version updated to v1.6.3
This bump ensures the module references remain consistent. Double-check that any consumers of this package have been updated accordingly.
170-173: Replace Directive Adjustments in Module Scope
The new replace directives mapping:
–cosmossdk.io/storeto../../store
–cosmossdk.io/x/stakingto../staking
appear correct relative to the module’s directory. Please verify these local paths in your development setup.x/accounts/defaults/base/go.mod (2)
55-55: Dependency Update: cosmos/iavl version bumped to v1.3.5
This version update is in line with changes across the codebase. Please ensure that any API changes from this dependency are fully compatible with this module.
163-168: Replace Directive: Updating cosmossdk.io/store path
The replacement mappingcosmossdk.io/store => ../../../../storeappears to be set for local development. Verify that the relative path reflects the correct directory structure for consumers in this module.x/mint/go.mod (1)
50-50: Dependency Update: cosmos/iavl version bumped to v1.3.5
The dependency update maintains consistency with other modules. Please confirm that this new version works seamlessly with module functionalities.server/v2/cometbft/go.mod (1)
186-189: Temporary Replace Directive for cosmossdk.io/store
The new replace directive
replace cosmossdk.io/store => ../../../store
is marked as temporary (with a TODO note). Verify that this redirection correctly supports integration with the object store functionality and that there is a plan to remove it once the modules are fully spun out.x/feegrant/go.mod (2)
68-68: Dependency Update: cosmos/iavl to v1.3.5
The cosmos/iavl dependency has been upgraded to v1.3.5. This change is consistent with the coordinated dependency updates across the repository. Please verify that the new version does not introduce any breaking API changes or incompatibilities.
98-98: Dependency Update: hashicorp/go-plugin to v1.6.3
The hashicorp/go-plugin dependency has been updated to v1.6.3. Ensure that this version update is compatible with existing implementations using this package within the module.tests/go.mod (2)
105-105: Dependency Update in Tests: cosmos/iavl v1.3.5
The cosmos/iavl version in the tests module has been updated to v1.3.5. This aligns with the changes in the main modules. Please double-check that any tests relying on this dependency continue to function as expected.
138-138: Dependency Update in Tests: hashicorp/go-plugin v1.6.3
The hashicorp/go-plugin dependency is now updated to v1.6.3 in the tests module, mirroring the update in the main modules. Confirm that the updated version integrates well with the testing environment.x/epochs/go.mod (3)
50-50: Dependency Update: cosmos/iavl to v1.3.5 in Epochs
The cosmos/iavl dependency for the epochs module is updated to v1.3.5. Please ensure that any changes in the library’s behaviour are reflected in the module’s functionality.
78-78: Dependency Update: hashicorp/go-plugin to v1.6.3 in Epochs
The hashicorp/go-plugin dependency is bumped to v1.6.3 here. Verify that the plugin interfaces used in this module continue to work as intended after the upgrade.
160-160: Replace Directive Update: cosmossdk.io/store
The replace directive for cosmossdk.io/store has been updated to point to../../store. Please confirm that this correctly redirects to the new object store implementation and that the local development workflow remains seamless.x/distribution/go.mod (2)
60-60: Dependency Update: cosmos/iavl to v1.3.5 in Distribution
The cosmos/iavl dependency is updated to v1.3.5 in the distribution module. Ensure that any API changes in this new version are handled appropriately within the module’s logic.
89-89: Dependency Update: hashicorp/go-plugin to v1.6.3 in Distribution
The hashicorp/go-plugin dependency is now set to v1.6.3. Please verify that this update is compatible with the distribution module’s functionality and does not break any integration points.simapp/v2/go.mod (1)
306-306: Replace Directive Update: cosmossdk.io/store in simapp/v2
The replace directive for cosmossdk.io/store has been changed to point to../../store. This update should support the new object store functionality introduced in this PR. Please double-check that the path resolution works correctly during local development and integration tests.tools/benchmark/go.mod (2)
54-54: Dependency Update for cosmos/iavl
The dependency forgithub.com/cosmos/iavlhas been updated tov1.3.5. This minor version bump appears appropriate; please just ensure it is fully compatible with the rest of the codebase.
152-157: New Replace Directive for Local Store Module
A replace directive was added to mapcosmossdk.io/storeto a local path (../../store). This supports local development of the store module. Verify that the relative path is correct in the context of your project structure and that the change is consistently reflected in all modules.x/gov/go.mod (2)
57-59: Updated Dependency Versions
The module now usesgithub.com/cosmos/iavl v1.3.5andgithub.com/hashicorp/go-plugin v1.6.3(as seen in the dependency block). These updates are in line with the coordinated dependency refresh. Please double-check that all breaking changes (if any) are addressed downstream.
33-35: New Replace Directive Verification
According to the PR summary, a new replace directive mappingcosmossdk.io/storeis expected in this module as well. Ensure that this directive (e.g.cosmossdk.io/store => ../../store) is present and correctly positioned, aligning with changes in other modules.x/group/go.mod (2)
64-64: Dependency Update for cosmos/iavl
The version ofgithub.com/cosmos/iavlhas been updated tov1.3.5. This change aligns with similar updates elsewhere; please confirm that this upgrade does not affect any dependent code.
170-176: Local Replace Directive for Store Module
A replace block now mapscosmossdk.io/storeto../../store(along with several other local mappings). This should streamline local development. Verify that all relative paths (including those for bank, gov, and staking) are correct and that no unintended consequences occur due to these changes.x/slashing/go.mod (2)
61-61: Updated Dependency for cosmos/iavl
The dependencygithub.com/cosmos/iavlhas been upgraded tov1.3.5. Please ensure that this update does not break compatibility with any slashing-specific logic.
163-171: Verify Replace Block for Module Dependencies
The replace block now includes a mapping forcosmossdk.io/storeto../../store, along with local replacements for other modules. Confirm that these changes harmonize with the overall dependency management strategy across the repository.x/upgrade/go.mod (2)
75-75: Dependency Update for cosmos/iavl in Upgrade Module
The dependencygithub.com/cosmos/iavlis now atv1.3.5in this module as well. This update is consistent with the other modules; please verify that any functionality relying on iavl continues to operate as expected.
201-207: New Replace Directive for Store Module in Upgrade Module
A new replace directive mappingcosmossdk.io/storeto../../storehas been added in this module’s replace block. Ensure that the relative paths for all locally replaced modules (client/v2, bank, gov, staking) are correct and that the migration to the new object store approach is smooth.x/staking/go.mod (2)
62-62: Dependency Update: Cosmos IAVL Version Bump
The version forgithub.com/cosmos/iavlhas been updated tov1.3.5as intended. Please ensure compatibility with upstream changes and that the integration tests cover any potential breaking changes from this bump.
91-91: Dependency Update: HashiCorp Go-Plugin Version Upgrade
The update ofgithub.com/hashicorp/go-pluginto versionv1.6.3appears to be in line with the coordinated dependency updates. Please verify that the new version maintains the expected behavior and does not introduce regressions in components that rely on this plugin.store/listenkv/store.go (2)
6-6: LGTM: Import added to support the new implementation.The
cachekvpackage import has been correctly added to support the updated implementation of theCacheWrapmethod.
139-143:✅ Verification successful
Consider updating CacheWrapWithTrace for consistency
While the
CacheWrapmethod has been updated to return a cache-wrapped store instead of panicking, theCacheWrapWithTracemethod still panics. This creates an inconsistency in behavior between these two closely related methods.Consider updating the
CacheWrapWithTracemethod to return a cache-wrapped store with trace support, similar to howCacheWraphas been updated:func (s *Store) CacheWrapWithTrace(w io.Writer, tc types.TraceContext) types.CacheWrap { - panic("cannot CacheWrapWithTrace a ListenKVStore") + return cachekv.NewStore(s).CacheWrapWithTrace(w, tc) }
🏁 Script executed:
#!/bin/bash # Check if cachekv.Store implements CacheWrapWithTrace correctly # Look for the CacheWrapWithTrace method implementation in cachekv store echo "Checking if cachekv.Store implements CacheWrapWithTrace..." grep -A 10 "func .* CacheWrapWithTrace" store/cachekv/store.goLength of output: 517
Update CacheWrapWithTrace for Consistency
The current implementation of
CacheWrapWithTraceinstore/listenkv/store.gopanics, which is inconsistent with the updated cache wrapping behavior observed incachekv.Store. The verification confirms thatcachekv.Storeimplements the method as follows:func (store *GStore[V]) CacheWrapWithTrace(w io.Writer, tc types.TraceContext) types.CacheWrap { if store, ok := any(store).(*GStore[[]byte]); ok { return NewStore(tracekv.NewStore(store, w, tc)) } return store.CacheWrap() }To align the behavior between these related methods, update the
CacheWrapWithTraceinListenKVStoreto return a cache-wrapped store with trace support:func (s *Store) CacheWrapWithTrace(w io.Writer, tc types.TraceContext) types.CacheWrap { - panic("cannot CacheWrapWithTrace a ListenKVStore") + return cachekv.NewStore(s).CacheWrapWithTrace(w, tc) }CHANGELOG.md (1)
1-3161: Well-maintained and comprehensive changelogThe CHANGELOG.md is comprehensive and well-organized, following a consistent structure throughout. Each version is properly documented with categorized changes (Features, Improvements, Bug Fixes, etc.) and appropriate PR links.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
91-91: Unordered list indentation
Expected: 4; Actual: 2(MD007, ul-indent)
92-92: Unordered list indentation
Expected: 4; Actual: 2(MD007, ul-indent)
93-93: Unordered list indentation
Expected: 4; Actual: 2(MD007, ul-indent)
364-364: Multiple headings with the same content
null(MD024, no-duplicate-heading)
374-374: Multiple headings with the same content
null(MD024, no-duplicate-heading)
416-416: Multiple headings with the same content
null(MD024, no-duplicate-heading)
883-883: Multiple headings with the same content
null(MD024, no-duplicate-heading)
1333-1333: Multiple headings with the same content
null(MD024, no-duplicate-heading)
2123-2123: Multiple headings with the same content
null(MD024, no-duplicate-heading)
store/internal/btree/btree_test.go (2)
1-1: Package name updated to match directory structureThe package name change from
internaltobtreecorrectly aligns with the directory structure, making imports more intuitive.
12-12: Generic type parameter specified for NewBTreeThe BTree implementation has been updated to use Go generics, and the test code now explicitly provides the value type as
[]byte. This is a good improvement for type safety and clarity.Also applies to: 43-43, 174-174
store/internal/btreeadaptor.go (1)
11-23: New BTreeStore implementation with genericsThe BTreeStore implementation is well-structured and properly uses Go generics to provide type safety. The implementation pattern follows Go conventions and the constructor function is clean and idiomatic.
store/rootmulti/store_test.go (7)
659-659: Casting topauseableCommitKVStoreStubseems appropriate.
This explicit cast is acceptable in test code, but please ensure the originalstore.stores[testStoreKey1]is always aCommitKVStoreto avoid possible panic.
905-915: Utility function for hashing store commits looks good.
The approach of aggregating store commit IDs and hashing them is sound for generating a combined hash. No concerns.
960-961:commitStoreStubdeclaration is clear.
This stub extendsCommitStorefor testing, which is a coherent approach for tracking commits. No issues found.
965-966: OverridingCommit()to track commit calls is valid.
This wrapper increments the counter and delegates to the embedded store, which is appropriate for testing.
971-977:prepareStoreMapfunction is straightforward.
Mounting additional store keys (including object store keys) is cleanly done and should not introduce issues.
981-992: Constructing the stub map for multiple keys is correct.
Mapping each test key to its correspondingcommitStoreStubis clear. Double-check type assertions at runtime to prevent panics in other contexts beyond testing.
1023-1023: Accessing thecommitStoreStubfrom the map.
This cast is fine for test code. No further issues.store/internal/btree/memiterator.go (13)
1-1: Renaming package tobtreeis consistent with the new module layout.
No issues noted.
12-12: Interface assertion formemIterator[[]byte]implementstypes.Iterator.
Ensures correctness at compile time. Good practice.
17-18: GenericmemIterator[V any]definition looks clean.
Storing the internalIterG[item[V]]is a correct approach for typed iteration.
26-30: Generic constructornewMemIterator
- Properly handles ascending/descending logic.
- Uses a placeholder
empty VforSeek.- Checks boundary conditions for start/end keys.
This is well-organized.Also applies to: 34-34, 40-40
67-69:Domain()correctly returns iterator boundaries.
Aligns with the requiredIteratorinterface.
71-74:Close()releases the internal iterator.
Straightforward resource cleanup.
76-81:Error()method signals invalid iterator state.
Complies with the interface definition.
83-85:Valid()method check is correct.
No issues; preserves the standard iteration contract.
87-99:Next()advances the iterator.
Handles ascending/descending. ThekeyInRangecheck ensures correct iteration edge handling.
101-109:keyInRangerelevancy check is well-implemented.
Properly checks boundaries for both ascending and descending iterations.
111-113:Key()returns underlying item key.
Safely accesses the current item.
115-117:Value()now generic.
Returns the typed value, leveraging theitem[V].valuefield.
119-123:assertValid()panics on iterator misuse.
Appropriate for internal usage in this implementation.store/internal/btree/btree.go (11)
1-1: Renaming package tobtreefor consistency.
No concerns.
25-27: GenericBTree[V any]struct
Decouples value type from implementation, aligning with the new generics approach.
30-37:NewBTree[V any]()constructor
Creates the internalbtree.BTreeGwith sensible options. No issues noted.
39-49:SetandGetmethods
Setproperly inserts or updates keyed items.Getreturns the type-safeVand a default if not found.
Implementation is straightforward.
52-55:Deletemethod
Removes the matching key using a placeholder default value for lookups. Correct approach.
57-69:IteratorandReverseIterator
Both handle empty key checks and return a generic iterator. No issues noted.
73-77:Copy()method
Returns a shadow copy from the underlying B-tree. Good for copy-on-write semantics.
79-81:Clear()method
Invokestree.Clear(). Straightforward design.
84-87: Genericitem[V any]struct
Holds both key and typed value. Maintains clarity in usage.
90-92:byKeyscomparator
Uses byte comparison for ordering. Matches the B-tree’s expected comparator signature.
95-97:newItemconstructor
Consistent with the typed item approach.store/CHANGELOG.md (1)
34-36: LGTM! Clear and properly formatted changelog entry.The changelog entry follows the guidelines, is properly formatted, and accurately summarizes the new object store feature.
runtime/module.go (1)
102-102: LGTM! Well integrated into the init function.The addition of
ProvideObjectStoreKeyin the init function is properly placed with other similar providers.store/internal/btree/memiterator_test.go (1)
8-8: LGTM! Proper use of generics.The changes to use
NewBTree[[]byte]()instead ofNewBTree()properly adapt the tests to the new generic implementation. Using generics improves type safety while maintaining the same test functionality.Also applies to: 35-35, 63-63
store/types/validity.go (3)
33-39: Good use of generics for enhanced type safetyThe new
AssertValidValueGenericfunction elegantly extends value validation to arbitrary types using Go generics. This approach is more type-safe than using raw byte slices and allows for custom validation logic through the callback functions.🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 36-36: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
41-45: Improved error handling witherrors.NewGood refactoring to centralize the length validation in a separate function and use
errors.Newfor the panic message. This improves code organization and provides better error context.
36-36:❓ Verification inconclusive
Handle potential panic in consensus-critical code paths
This line could potentially cause a chain halt if called during BeginBlock or EndBlock consensus methods.
Consider adding a recovery mechanism if this validation is used in consensus-critical paths, or ensure it's only called in safe contexts. You can verify where this function is called with:
🏁 Script executed:
#!/bin/bash # Check if AssertValidValueGeneric is called in BeginBlock or EndBlock methods rg "BeginBlock|EndBlock" -A 10 -B 10 | rg "AssertValidValueGeneric" # Also check for indirect calls through interfaces that might lead to this function rg -A 3 "func.*BeginBlock|func.*EndBlock" | rg -A 10 "AssertValid|panic"Length of output: 3652
Attention: Consensus-Critical Panic Handling
While the search results did not reveal any direct or indirect calls to
AssertValidValueGenericin consensus-critical methods (such as BeginBlock or EndBlock), please remain cautious. Ensure that this panic is never triggered in a context that could halt the chain—if there's any possibility it might be invoked in consensus-critical paths, consider either:
- Adding a recovery mechanism or a safe wrapper around this validation, or
- Explicitly documenting that this function is only used in non-critical code paths.
This precaution will help prevent any inadvertent chain halts in production.
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 36-36: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain haltbaseapp/baseapp.go (2)
305-307: Good addition of ObjectStoreKey supportThe addition of ObjectStoreKey handling in MountStores follows the existing pattern for other store types, maintaining consistency in the codebase.
346-354: Well-implemented MountObjectStores methodThe implementation follows the established pattern of other Mount* methods, ensuring consistent behavior. Using slices.Sorted with maps.Keys for deterministic iteration is a good practice.
runtime/store.go (2)
166-172: Updated Iterator return type for consistencyThe change from
store.Iteratortostoretypes.Iteratoraligns with the broader object store implementation, ensuring proper interface compatibility across the codebase.
174-180: Updated ReverseIterator return type for consistencySimilarly to the Iterator method, updating ReverseIterator to return
storetypes.Iteratorensures consistency with the new object store interfaces.store/cachekv/search_benchmark_test.go (1)
25-43: Good adaptation of benchmarks to use generic typesThe changes properly update the test to work with the new generic
GStore[[]byte]andcValue[[]byte]types. This ensures test coverage of the refactored code with Go generics.One minor suggestion - consider parameterizing the type in a variable at the top of the function for easier maintenance:
func generateStore() *Store { + type storeValueType = []byte - cache := map[string]*cValue[[]byte]{} + cache := map[string]*cValue[storeValueType]{} // ... rest of function - return &GStore[[]byte]{ + return &GStore[storeValueType]{store/cachemulti/store.go (6)
25-25: Adopting a more general cache interface is beneficial.Replacing
db's type totypes.CacheWrapnicely broadens compatibility for various cache store wrappers.
39-43: Flexible function signature update.Changing from a more specific
types.KVStoreto atypes.CacheWrapperparameter better accommodates different store implementations (e.g., object stores).
50-61: Conditional trace wrapping logic looks good.The code safely checks for an underlying
KVStorebefore wrapping withtracekv. This aligns with the comment “only support tracing on KVStore.”
164-170: Helper method enhances readability.Encapsulating store retrieval in
getCacheWrapclarifies the logic. The panic handling is consistent with the SDK's pattern.
172-179: Coherent addition of typed getter.
GetKVStoreuses a type assertion to retrieve aKVStore. Panicking on mismatch is consistent with the codebase’s approach.🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 177-177: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
181-187: New method for ObjKVStore retrieval.Introducing
GetObjKVStorematches the pattern of typed store access. Same panic pattern on mismatch, so it remains consistent.store/rootmulti/store.go (5)
69-79: Shifting to a more generalized CommitStore map.These fields (
storesasmap[types.StoreKey]types.CommitStore, etc.) establish a generalized approach for commit-capable stores, facilitating object store integration.
177-186: Type assertion ensures CommitKVStore usage.This newly introduced
GetCommitKVStoremethod panics if the store is not aCommitKVStore. It follows the current SDK style but could lead to chain halts if invoked incorrectly in consensus paths.
695-698: Potential chain halt due to panic on mismatch.If the store is not a
KVStore, the code panics, matching the typical approach but possibly causing a halt in consensus if triggered during BeginBlock/EndBlock.🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 697-697: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
710-722: ObjKVStore retrieval method.Similar to other typed store getters, this method panics if the store isn't an
ObjKVStore. The approach is consistent with the rest of the SDK’s store design.
890-893: Skipping snapshot for non-persistent store variants.Adding
*transient.ObjStoreto thecasestatement ensures object stores are not snapshotted, which aligns with transient/memory store handling.store/transient/store.go (14)
4-5: Imports for internal B-tree usage.Bringing in
internal/btreeis essential for the new generic store approach.
13-15: Ensuring interfaces are satisfied.Declaring these interface checks for
ObjStoredemonstrates the new type’s alignment withCommitterandObjKVStore.
19-21: Generic store definition.Wrapping
BTreeStore[V]inGStore[V]introduces a flexible design for transient data of any type.
23-26: Parameterizing GStore creation.Allowing custom
isZeroandvalueLenfunctions promotes reusability and robust store logic for different data types.
28-30: Conceptual specialization for byte slices.
Storereuses the same generic logic specialized for raw bytes, maintaining consistency with existing patterns.
34-38: Builder function for byte-based GStore.Initializes an ephemeral store that treats
nilas zero. This is consistent with typical usage patterns.
40-42: Identifying the store type for clarity.Returning
StoreTypeTransientis correct for an ephemeral store.
44-55: Generic object store approach introduced.
ObjStoreextends the same design to handle any typed values, broadening usage scenarios.
56-58: Returning object store type.Declaring
StoreTypeObjectaligns with the new store classification, enhancing clarity in the multi-store.
62-68: Transient commit logic.
Commit()clears the store, reflecting the transient nature. This approach is valid for ephemeral data.
72-72: No-op pruning retrieval.Transient stores do not persist or prune data, so returning undefined pruning options is reasonable.
76-76: Empty commit ID is appropriate.Since transient data is never persisted, returning an empty
CommitIDis consistent.
80-80: Version always zero.Ephemeral stores do not retain historical versions, so the returned
0is accurate.
85-85: No hashing needed.Returning an empty byte slice reflects the fact that nothing is persisted to hash in a transient store.
store/types/store.go (8)
141-141: Add usage verification for new method
The newGetObjKVStore(StoreKey) ObjKVStoremethod aligns well with existing specialized getters (e.g.,GetKVStore). Please ensure all call-sites are updated accordingly to avoid runtime panics or missed integrations.
238-253: Introduction of generic basic KV store
TheGBasicKVStore[V any]interface elegantly extends key-value operations to arbitrary generic types. The design is consistent with the new generics-based approach and looks solid.
254-272: Generic KV store interface
TheGKVStore[V any]interface effectively layers iteration capabilities on top ofGBasicKVStore[V]. No issues found; it appears to follow established store interface patterns.
274-301: Generic iterator interface
TheGIterator[V any]interface for typed iteration looks consistent with the existing iteration pattern. This change should make the code more flexible while retaining familiarity for developers.
373-374: Added StoreTypeObject
AddingStoreTypeObjectis a logical extension to the existing set of store types. Make sure all code paths handle this new type to prevent unexpected panics or mismatches.
399-400: String representation for StoreTypeObject
The string resolution forStoreTypeObjectneatly matches the existing pattern for store types.
480-501: ObjectStoreKey implementation
TheObjectStoreKeystruct and its constructor follow the same pattern used by other store key types such asKVStoreKeyandTransientStoreKey. No issues found.
601-613: New factory function for multiple ObjectStoreKeys
NewObjectStoreKeysparallels other functions likeNewKVStoreKeysorNewTransientStoreKeysand should ease the creation of object store keys in bulk.store/prefix/store.go (23)
13-16: Introduced aliased store types
DefiningStore = GStore[[]byte]andObjStore = GStore[any]provides a convenient shorthand for byte-based and object-based prefix stores, respectively.
18-21: Interface compliance assertions
AssertingStoreimplementstypes.KVStoreandObjStoreimplementstypes.ObjKVStoreis a good practice to guarantee interface conformance at compile time.
23-29: NewStore constructor
Creating a prefix store for byte slices viaNewStoreand passing in customisZero/valueLenclosures ensures robust handling of nil checks and length computations.
31-37: NewObjStore constructor
NewObjStorecleanly abstracts object-based storage in a prefix, reusing the same pattern asNewStore. This should streamline usage of object-based KV operations.
39-48: Generic prefix store struct
GStore[V any]extends a parenttypes.GKVStore[V]with an additional prefix mechanism and injection points for zero checks and value-length calculations. This is a flexible and modular approach.
70-70: Key prefixing logic
Thekey(key []byte)helper properly appends the prefix to user-supplied keys. This is essential for scoping data within the parent store.
83-86: CacheWrap integration
Wrapping withcachekv.NewGStoreensures that prefix-based operations inherit proper caching behavior. Implementation and usage appear straightforward.
88-93: Conditional CacheWrapWithTrace
The type assertion to handle tracing forGStore[[]byte]is a nice optimization that avoids unnecessary reflection for typed scenarios.
96-101: Get method for generic store
TheGetmethod straightforwardly obtains the value from the parent store using the prefixed key, maintaining minimal overhead.
102-105: Has method
Usings.key(key)ensures prefix isolation. No concerns regarding correctness or performance in this short method.
107-112: Set method with validation
types.AssertValidKey(key)andtypes.AssertValidValueGeneric(value, ...)enforce data integrity. This is a welcome improvement over blindly storing arbitrary data.
115-117: Delete method
TheDeleteimplementation is straightforward and consistent with the rest of the prefix store logic.
121-134: Iterator method
Appending the prefix to the start and computing an appropriate end boundary is well aligned with prefix store semantics. ReturningnewPrefixIteratoris consistent with the design.
138-151: ReverseIterator method
Mirrors the forward iterator logic for reversed iteration. Properly increments the prefix ifendis nil, ensuring correct iteration domain.
153-153: Interface assertion
Affirming that(*prefixIterator[[]byte])(nil)implementstypes.Iteratorhelps catch potential interface mismatch at compile time.
155-161: prefixIterator struct
Encapsulates iterator state, including the parent iterator. Using avalidflag to track validity is a clean approach.
163-171: newPrefixIterator factory
Nicely sets up theprefixIteratorby verifying the parent's key has the correct prefix on initialization.
173-176: Domain method
Returns the original start and end range unmodified, ensuring partial prefix iteration semantics remain transparent to the caller.
179-181: Valid method
Combines the localvalidflag with the parent iterator'sValid()method to maintain consistent state.
196-205: Key method
Strips the prefix from the parent iterator's key, which aligns with the logic behindprefixIterator. No concerns.
208-214: Value method
Simply returns the parent's value. This is consistent with the prefix store pattern where only the key is prefixed.
217-219: Close method
Delegates resource cleanup to the parent iterator. Straightforward and standard.
223-229: Error method
Returns an error if the iterator is invalid. The direct parent delegation is a simple, clean approach.store/gaskv/store.go (21)
14-14: Aliased Store type
Store = GStore[[]byte]matches the approach in other store packages, simplifying gas-tracked key/value operations for raw byte slices.
16-21: NewStore constructor
NewStoreconstructs a gas-tracked store for byte-based values. The closures supplied forisZeroandvalueLenproperly handle nil checks and length calculation.
23-23: Aliased ObjStore type
ObjStore = GStore[any]extends gas tracking to arbitrary typed objects. This is a consistent extension of the generic store concept.
25-30: NewObjStore constructor
Similar toNewStorebut specialized for any-typed values with a fixedObjectValueLengthcost. This is a straightforward solution for object-based gas accounting.
32-41: Generic GStore struct
AddsgasMeter,gasConfig, and function pointersisZero/valueLento the parent store. This neatly centralizes gas logic in one layer.
43-50: NewGStore function
Constructs aGStore[V]with the provided gas meter and config. This design allows flexible injection of gas accounting logic.
62-64: GetStoreType
Defers to the underlying parent store. The implementation is minimal but necessary to satisfy theStoreinterface.
67-73: Get method with gas consumption
Reading keys and values incurs both flat and per-byte gas costs. This ensures usage-based billing and discourages large unbounded keys/values.
79-86: Set method with validation and gas consumption
Appropriately charges a flat write cost plus a cost proportional to key and value size. The call totypes.AssertValidValueGenericenforces data correctness.
102-107: Iterator method
Wraps the parent iterator with gas accounting that charges for iteration steps. Implementation is consistent with prior singledirectional iteration design.
109-115: ReverseIterator method
As withIterator, but reversed. The structure fosters consistency in how gas is charged for both ascending and descending access.
117-125: CacheWrap and CacheWrapWithTrace
Gas-tracked stores do not support caching or tracing wraps directly here, preventing nested layering complexities. This matches the typical design inGasKVStore.
127-139: Shared iterator creation
gs.iterator(start, end, ascending)centralizes logic for both forward and reverse iterators, returning a gas-aware iterator instance.
141-146: gasIterator struct
Wraps a parent generic iterator to inject gas consumption calls. This is a clean approach to layering in incremental usage-based fees.
148-155: newGasIterator constructor
Solid constructor that sets references and then returns agasIterator[V]. Straightforward.
163-165: Valid method
Delegates validity checks to the parent iterator. Correct approach to ensure consistent behavior.
170-173: Next method
CallsconsumeSeekGas()before advancing the parent iterator. This design enforces usage-based fees for each iteration step.
184-186: Value method
Returns the parent's typed value. The direct pass-through ensures no overhead beyond normal iteration costs.
189-191: Close method
Safe delegation to the underlying iterator'sClose. No code smell here.
194-196: Error method
Again, simply delegates to the parent. This is a consistent pattern.
200-207: consumeSeekGas method
Appropriately charges for key length and value length, then applies a flat cost per iteration step. This ensures fair gas accounting.store/cachekv/store.go (10)
12-12: No issues with the new import.
20-23: Good use of generics forcValue[V].
This design properly stores any typeVand flags when data is dirtied.
25-27:kvPair[V]structure looks fine.
Clear key-value pairing using generics.
30-30: Type aliasStore = GStore[[]byte]appears consistent.
This eases migration from the old byte-based store while still adopting generics.
35-40: Bridging toNewGStoreis well-structured.
Good job passing lambdas for zero-check and value length for[]byte.
42-56:GStore[V]declaration looks solid.
Introducing a sync.Mutex, typed caches, and anisZerofunc is an effective approach. Just ensure locking remains consistent for all read/write paths.
58-68:NewGStoreconstructor is well-implemented.
Nice initialization of fields, consistent with the store's design.
75-76: Generically typedGetmethod is correct.
It properly locks before retrieving and caches as needed.
94-96: Validation usingAssertValidValueGeneric.
Ensures type-based checks and disallows zero values if required.
110-116: Deletion mechanism.
Using the “zero value” as a tombstone is consistent with your approach.store/cachekv/internal/mergeiterator.go (10)
17-19: IntroducingcacheMergeIterator[V]withisZero.
This generic approach is consistent with the rest of the refactor; usingisZeroto detect deletions is clean.Also applies to: 24-24
27-27: Enforcing interface compliance with(*cacheMergeIterator[[]byte])(nil).
Good practice to ensure at compile time that the iterator satisfiestypes.Iterator.
29-30:NewCacheMergeIterator: constructor approach is clear.
Pass-through ofisZeroensures consistent logic when merging cache and parent iterators.
139-139:Close()with error handling.
Properly closes both iterators and returns any errors found.
151-151:Error()method.
Straightforward approach to reflect invalid iterator state.
160-162:assertValid()defers toError().
This pattern is consistent and ensures a single source of truth for validity checks.
168-169:compare(a, b []byte)respects ascending vs. descending.
Negating the result for descending is a common solution.
182-183:skipCacheDeletesleveragesisZero.
Good clarity: it repeatedly skips deleted items from the cache portion.
193-193:skipUntilExistsOrInvalid
Effectively fast-forwards until a non-deleted key is found or both iterators are exhausted.
217-218: Deletion check withisZero(valueC).
This is the right condition to discard cache entries representing tombstone values.Also applies to: 229-229
| func TestListenKVStoreCacheWrap(t *testing.T) { | ||
| store := newEmptyListenKVStore(nil) | ||
| require.Panics(t, func() { store.CacheWrap() }) | ||
| store.CacheWrap() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test needs assertions to verify the new behavior
The test has been updated to align with the changed implementation in store.go, where CacheWrap() no longer panics. However, this test is now not very useful as it doesn't verify the actual behavior of the method - it merely checks that it doesn't panic.
Consider enhancing this test to verify that the returned object is of the expected type and behavior:
func TestListenKVStoreCacheWrap(t *testing.T) {
store := newEmptyListenKVStore(nil)
- store.CacheWrap()
+ cacheStore := store.CacheWrap()
+ require.NotNil(t, cacheStore, "CacheWrap should return a non-nil store")
+ require.IsType(t, &cachekv.Store{}, cacheStore, "CacheWrap should return a cachekv.Store")
+
+ // Basic functionality test to ensure it works as expected
+ key, value := []byte("test-key"), []byte("test-value")
+ cacheStore.Set(key, value)
+ require.Equal(t, value, cacheStore.Get(key))
+
+ // Original store should not be affected until Write is called
+ require.Nil(t, store.Get(key))
}📝 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 TestListenKVStoreCacheWrap(t *testing.T) { | |
| store := newEmptyListenKVStore(nil) | |
| require.Panics(t, func() { store.CacheWrap() }) | |
| store.CacheWrap() | |
| } | |
| func TestListenKVStoreCacheWrap(t *testing.T) { | |
| store := newEmptyListenKVStore(nil) | |
| cacheStore := store.CacheWrap() | |
| require.NotNil(t, cacheStore, "CacheWrap should return a non-nil store") | |
| require.IsType(t, &cachekv.Store{}, cacheStore, "CacheWrap should return a cachekv.Store") | |
| // Basic functionality test to ensure it works as expected | |
| key, value := []byte("test-key"), []byte("test-value") | |
| cacheStore.Set(key, value) | |
| require.Equal(t, value, cacheStore.Get(key)) | |
| // Original store should not be affected until Write is called | |
| require.Nil(t, store.Get(key)) | |
| } |
| // 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.
🛠️ Refactor suggestion
CacheWrapWithTrace ignores trace parameters
The CacheWrapWithTrace method doesn't use the provided writer and trace context parameters, making it functionally identical to CacheWrap. This seems inconsistent with the method's purpose.
// 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)
+ // Use the writer and trace context to create a trace-enabled cache wrapper
+ return cachekv.NewGStore(tracekv.NewStore(ts, w, tc), ts.isZero, ts.valueLen)
}Note: The implementation requires verification since it depends on how tracekv.NewStore works with generic stores. The actual implementation may need adjustment.
📝 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.
| // 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) | |
| } | |
| // CacheWrapWithTrace branches the underlying store. | |
| func (ts *BTreeStore[V]) CacheWrapWithTrace(w io.Writer, tc types.TraceContext) types.CacheWrap { | |
| // Use the writer and trace context to create a trace-enabled cache wrapper | |
| return cachekv.NewGStore(tracekv.NewStore(ts, w, tc), ts.isZero, ts.valueLen) | |
| } |
|
could you target this against release/v0.53.x? |
2 participants
Description
Reopen #22893
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
Improvements
Chores