feat: use the sdk/core/store.KVStoreWithBatch interface instead of iavl/db.DB interface
#980
Conversation
WalkthroughThe recent changes reflect a substantial refactoring of the codebase, primarily focusing on transitioning from the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant DB as KVStoreWithBatch
User->>App: Initialize Application
App->>DB: OpenDB()
DB-->>App: Return KVStoreWithBatch
App->>DB: Perform Batch Operations
DB-->>App: Return Results
User->>App: View Stats
App->>DB: PrintDBStats()
DB-->>App: Provide Stats
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
cmd/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
Files selected for processing (23)
- CHANGELOG.md (1 hunks)
- batch.go (1 hunks)
- benchmarks/bench_test.go (3 hunks)
- cmd/go.mod (1 hunks)
- cmd/iaviewer/main.go (4 hunks)
- db/README.md (1 hunks)
- db/memdb.go (1 hunks)
- db/prefixdb.go (1 hunks)
- db/types.go (2 hunks)
- doc.go (1 hunks)
- docs/proof/proof.md (1 hunks)
- go.mod (1 hunks)
- immutable_tree.go (2 hunks)
- mock/db_mock.go (5 hunks)
- mock/store_mock.go (1 hunks)
- mockgen.sh (1 hunks)
- mutable_tree.go (2 hunks)
- mutable_tree_test.go (1 hunks)
- nodedb.go (2 hunks)
- nodedb_test.go (1 hunks)
- testutils_test.go (1 hunks)
- tree_random_test.go (2 hunks)
- tree_test.go (2 hunks)
Files skipped from review due to trivial changes (2)
- mock/store_mock.go
- mutable_tree_test.go
Additional comments not posted (37)
mockgen.sh (1)
4-5: Explicit source specification and expanded mock generation.The changes improve clarity by explicitly specifying the source file for
DBand expand mock generation to includeIteratorandBatchfrom thecosmossdk.io/core/storepackage. This enhances maintainability and aligns with best practices.db/README.md (1)
12-21: Simplified database instantiation and improved clarity.The changes simplify the database instantiation process by using
NewGoLevelDBand remove unnecessary complexity by eliminatingNewWrapper. This reflects a more direct and efficient approach to database management.cmd/go.mod (1)
6-12: Routine dependency updates and removals.The updates to
cosmossdk.io/coreandgit.luolix.top/cosmos/gogoprotoare standard practice for maintaining compatibility. However, verify that the removal of indirect dependencies does not impact functionality.Verification successful
Dependencies safely removed.
The removed dependencies are not referenced in the codebase or
go.sum, indicating they are no longer needed. This confirms their removal does not impact the project.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of removed indirect dependencies. # Test: Search for any usage of removed dependencies in the codebase. rg --fixed-strings --type go 'github.com/getsentry/sentry-go' 'github.com/klauspost/compress' 'github.com/kr/pretty' 'github.com/kr/text' 'github.com/linxGnu/grocksdb' 'github.com/prometheus/client_golang' 'github.com/prometheus/client_model' 'github.com/prometheus/common' 'github.com/prometheus/procfs' 'github.com/rogpeppe/go-internal'Length of output: 939
Script:
#!/bin/bash # Description: Check for any remaining references to removed dependencies in go.sum. # Test: Search for any occurrences of removed dependencies in the go.sum file. rg --fixed-strings 'github.com/getsentry/sentry-go' 'github.com/klauspost/compress' 'github.com/kr/pretty' 'github.com/kr/text' 'github.com/linxGnu/grocksdb' 'github.com/prometheus/client_golang' 'github.com/prometheus/client_model' 'github.com/prometheus/common' 'github.com/prometheus/procfs' 'github.com/rogpeppe/go-internal' go.sumLength of output: 936
go.mod (4)
13-13: Switch togo.uber.org/mock.The dependency has changed from
github.com/golang/mocktogo.uber.org/mock. This might affect the mocking strategy in tests. Verify the impact on existing tests.Verification successful
Mocking library successfully updated to
go.uber.org/mock.The tests and mock files have been updated to use the new mocking library
go.uber.org/mock, as evidenced by the imports innodedb_test.go,mutable_tree_test.go,store_mock.go, anddb_mock.go. This indicates that the transition fromgit.luolix.top/golang/mockhas been implemented.
- Files updated:
nodedb_test.gomutable_tree_test.gomock/store_mock.gomock/db_mock.goThe impact on existing tests seems to have been addressed.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of switching to `go.uber.org/mock`. # Test: Search for usages of the new mocking library and ensure tests are updated. rg --type go 'go.uber.org/mock' -A 3Length of output: 691
17-17: Dependency Update:github.com/cosmos/gogoproto.The
github.com/cosmos/gogoprotodependency has been updated to versionv1.6.0. This update may include improvements or bug fixes. Ensure compatibility with the updated version.
6-6: Dependency Update:cosmossdk.io/core.The
cosmossdk.io/coredependency has been updated to a newer version. This change may include important improvements or bug fixes. Ensure compatibility with the updated version.
22-22: New Indirect Dependency:github.com/kr/pretty.The addition of
github.com/kr/prettyas an indirect dependency may affect how data is pretty-printed in tests or logs. Verify its usage and impact.doc.go (1)
12-12: Import Path Change: Database Module.The import path for the database module has been changed to
"github.com/cosmos/iavl/db". This reorganization may affect how theMutableTreeinteracts with the database layer. Ensure the change aligns with the new architecture.Verification successful
Import Path Change Verified: Database Module
The import path change to
"github.com/cosmos/iavl/db"is correctly integrated across the codebase, particularly in test files involvingMutableTree. These tests ensure that the interactions with the database layer function as expected, aligning with the new architecture.
- Verified usage in test files such as
tree_test.go,mutable_tree_test.go, and others.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the import path change on `MutableTree` interactions. # Test: Search for usages of the new import path to ensure correct integration. rg --type go 'github.com/cosmos/iavl/db' -A 3Length of output: 3081
Script:
#!/bin/bash # Description: Verify MutableTree interactions with the new db import in test files. # Test: Search for MutableTree usage in test files that import the db package. rg --type go 'MutableTree' -A 5Length of output: 77619
db/types.go (3)
31-32: New Method:Set.The
Setmethod has been added to theDBinterface, allowing key-value assignments with error handling for nil keys or values. This enhances the interface's functionality.
34-35: New Method:Delete.The
Deletemethod has been added to theDBinterface, facilitating key removal with error handling for nil keys. This addition improves the interface's usability.
20-21: Clarification: Purpose of theDBInterface.The comment has been updated to clarify that the
DBinterface is primarily intended for mock creation. Ensure this aligns with the overall architecture and testing strategy.batch.go (2)
14-14: LGTM!The change from
dbm.DBtocorestore.KVStoreWithBatchin theBatchWithFlusherstruct aligns with the PR objectives and enhances batch operation capabilities.
Line range hint
23-27: Constructor update approved.The
NewBatchWithFlusherfunction now correctly usescorestore.KVStoreWithBatch, aligning with the updated struct definition.cmd/iaviewer/main.go (3)
Line range hint
91-109: Parameter update approved.The
PrintDBStatsfunction now correctly usescorestore.KVStoreWithBatch, ensuring consistency with the new database type.
126-129: Simplification approved.The
ReadTreefunction now directly uses thedbinstance, simplifying the interaction withNewMutableTree.
Line range hint
63-89: LGTM! VerifyNewGoLevelDBusage.The
OpenDBfunction now returnscorestore.KVStoreWithBatch, aligning with the PR objectives. Ensure that the removal of the third parameter inNewGoLevelDBis consistent with its API.Verification successful
API Consistency Verified for
NewGoLevelDBThe
NewGoLevelDBfunction is consistently used with two parameters across the codebase, matching its definition. There is no indication of an API change requiring a third parameter. The review comment's concern about the removal of a third parameter is incorrect.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `NewGoLevelDB` to ensure API consistency. # Test: Search for `NewGoLevelDB` function usage. Expect: Only occurrences with two parameters. rg --type go -A 2 $'NewGoLevelDB'Length of output: 2716
mock/db_mock.go (6)
56-68: Addition ofDeletemethod approved.The
Deletemethod enhances the mock's capability to simulate key deletions, aligning with the updated interface.
101-110: Return type update approved.The
Iteratormethod now correctly usesstore.Iterator, aligning with the updated interface definitions.
116-122: Return type update approved.The
NewBatchmethod now correctly usesstore.Batch, aligning with the updated interface definitions.
130-138: Return type update approved.The
NewBatchWithSizemethod now correctly usesstore.Batch, aligning with the updated interface definitions.
144-153: Return type update approved.The
ReverseIteratormethod now correctly usesstore.Iterator, aligning with the updated interface definitions.
159-169: Signature update approved.The
Setmethod now usesanyfor the recorder, enhancing type safety and clarity in the mock's expected calls.CHANGELOG.md (1)
12-12: Changelog entry looks good.The entry for PR #980 accurately reflects the change from
iavl/db.DBtosdk/core/store.KVStoreWithBatch.testutils_test.go (1)
319-319: Function signature update looks good.The change from
dbm.DBtocorestore.KVStoreWithBatchinbenchmarkImmutableAvlTreeWithDBaligns with the PR objectives.Ensure that all tests and benchmarks using this function are updated to reflect the new interface.
Verification successful
All usages of
benchmarkImmutableAvlTreeWithDBare updated correctly.The function
benchmarkImmutableAvlTreeWithDBis used only once in the codebase, and it matches the new interface signature. No further updates are necessary.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `benchmarkImmutableAvlTreeWithDB` are updated to the new interface. # Test: Search for the function usage. Expect: Only occurrences with the new signature. rg --type go -A 5 $'benchmarkImmutableAvlTreeWithDB'Length of output: 431
db/prefixdb.go (1)
157-157: Function signature update looks good.The change from
DBtocorestore.KVStoreWithBatchinIteratePrefixis consistent with the transition to the new interface.Ensure that all calls to
IteratePrefixare updated to reflect the new interface.Verification successful
No calls to
IteratePrefixfound in the codebase. The functionIteratePrefixis defined but not used anywhere else in the codebase, so there are no calls that need updating to reflect the new interface.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `IteratePrefix` are updated to the new interface. # Test: Search for the function usage. Expect: Only occurrences with the new signature. rg --type go -A 5 $'IteratePrefix'Length of output: 430
Script:
#!/bin/bash # Description: Perform a comprehensive search for all usages of `IteratePrefix` across the codebase. # Search for the function usage in all Go files. rg --type go 'IteratePrefix'Length of output: 228
immutable_tree.go (1)
26-26: LGTM! But verify the function usage in the codebase.The change from
dbm.DBtocorestore.KVStoreWithBatchis approved.However, ensure that all function calls to
NewImmutableTreematch the new signature.Verification successful
Function usage verified: All calls to
NewImmutableTreematch the updated function signature withcorestore.KVStoreWithBatch. TheMemDBtype is confirmed to implement the required interface.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `NewImmutableTree` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'NewImmutableTree'Length of output: 1131
Script:
#!/bin/bash # Search for the definition of `dbm.NewMemDB` to verify its return type. rg --type go 'func NewMemDB' -A 2 # Search for the definition of `db` in `diff_test.go` to verify its type. rg --type go 'var db' diff_test.go -A 2Length of output: 192
Script:
#!/bin/bash # Search for the implementation of `MemDB` to verify if it implements `corestore.KVStoreWithBatch`. rg --type go 'type MemDB' -A 10 # Search for the definition of `db` in the broader context of `diff_test.go`. rg --type go 'db := ' diff_test.go -A 2Length of output: 549
db/memdb.go (1)
52-52: LGTM!The type assertion change from
DBtocorestore.KVStoreWithBatchis consistent with the new interface requirements.benchmarks/bench_test.go (3)
367-367: LGTM! But verify the function usage in the codebase.The change from
dbm.DBtocorestore.KVStoreWithBatchinrunSuiteis approved.However, ensure that all function calls to
runSuitematch the new signature.Verification successful
Function usage verified.
The
runSuitefunction is called with parameters matching its new signature usingcorestore.KVStoreWithBatch. No discrepancies found in the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `runSuite` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'runSuite'Length of output: 659
28-28: LGTM! But verify the function usage in the codebase.The change from
dbm.DBtocorestore.KVStoreWithBatchinprepareTreeis approved.However, ensure that all function calls to
prepareTreematch the new signature.Verification successful
Function usage is correct in the codebase.
The
prepareTreefunction inbenchmarks/bench_test.gois used with the correct new signature. No mismatches were found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `prepareTree` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'prepareTree'Length of output: 2956
342-342: LGTM! But verify the variable usage in the codebase.The change from
dbm.DBtocorestore.KVStoreWithBatchinrunBenchmarksis approved.However, ensure that all variable usages of
dmatch the new type.Verification successful
Variable usage is consistent with the new type
The variable
din therunBenchmarksfunction is correctly used ascorestore.KVStoreWithBatch. The changes align with the new type throughout its usage in the function and when passed torunSuite. No further issues were found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all variable usages of `d` match the new type. # Test: Search for the variable usage. Expect: Only occurrences of the new type. rg --type go -A 5 $'var d corestore.KVStoreWithBatch'Length of output: 54
Script:
#!/bin/bash # Description: Search for the declaration and usage of the variable `d` in the `runBenchmarks` function. # Find the definition of the `runBenchmarks` function and search for `d` within it. ast-grep --lang go --pattern $'func runBenchmarks($_) { $$$ var d $_ $$$ }'Length of output: 82
Script:
#!/bin/bash # Description: Extract the content of the `runBenchmarks` function to verify the usage of the variable `d`. # Search for the `runBenchmarks` function and display its content. rg --type go -A 50 'func runBenchmarks'Length of output: 2564
docs/proof/proof.md (1)
114-114: Verify documentation consistency with new dependency.The import statement for the
dbpackage has been changed fromgit.luolix.top/cosmos/cosmos-dbtogit.luolix.top/cosmos/iavl/db. Ensure that the documentation accurately reflects any changes in functionality or usage due to this dependency shift.nodedb_test.go (1)
11-11: Verify compatibility with the new mocking library.The import statement for
gomockhas been changed fromgit.luolix.top/golang/mock/gomocktogo.uber.org/mock/gomock. Ensure that the test implementations are compatible with the API and behavior of the new library.Verification successful
Compatibility Verified with New Mocking Library
The usage of
gomockin your test files appears to be standard and should remain compatible with the new import pathgo.uber.org/mock/gomock. The methods and patterns used, such asgomock.NewControllerandgomock.Any, are typical and supported. No deprecated or incompatible usages were found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify compatibility with the new `gomock` library from `go.uber.org`. # Test: Check for any deprecated or incompatible usage of `gomock`. Expect: No deprecated usage. rg 'gomock\.' -A 3Length of output: 12898
tree_random_test.go (1)
13-13: Verify the impact of the interface change on test logic.The
loadTreefunction now usescorestore.KVStoreWithBatchinstead ofdbm.DB. Ensure that this change is reflected in the test logic and that all interactions with the database are compatible with the new interface.Verification successful
The interface change to
corestore.KVStoreWithBatchis correctly reflected in the test logic. TheloadTreefunction and related test functions intree_random_test.goappropriately use the new interface, ensuring compatibility with the updated database interactions. No issues were found in the test logic regarding this change.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the interface change on test logic. # Test: Check for any incompatible usage of `corestore.KVStoreWithBatch`. Expect: No incompatible usage. rg 'corestore\.KVStoreWithBatch' -A 5Length of output: 5985
Script:
#!/bin/bash # Description: Check the usage of corestore.KVStoreWithBatch in tree_random_test.go and related test logic. # Find all functions and their usage in tree_random_test.go to verify compatibility with corestore.KVStoreWithBatch. ast-grep --lang go --pattern 'func $_($$$) { $$$ }' tree_random_test.goLength of output: 20945
mutable_tree.go (1)
48-48: Update function signature to useKVStoreWithBatch.The
NewMutableTreefunction now accepts acorestore.KVStoreWithBatchinstead ofdbm.DB. Ensure that all calls to this function are updated to pass the correct type.nodedb.go (2)
81-81: UpdatenodeDBstruct to useKVStoreWithBatch.The
dbfield in thenodeDBstruct now usescorestore.KVStoreWithBatch. This change should enhance batch processing capabilities.
96-96: UpdatenewNodeDBfunction signature to useKVStoreWithBatch.The
newNodeDBfunction now accepts acorestore.KVStoreWithBatchparameter. Ensure that all calls to this function are updated accordingly.tree_test.go (1)
Line range hint
37-47:
LGTM! But verify the function usage in the test suite.The change to return
corestore.KVStoreWithBatchis consistent with the PR's objective. Ensure that all tests usinggetTestDBare compatible with the new return type.Verification successful
Verification Complete: Function Usage Consistent with Changes
The
getTestDBfunction's usage intree_test.gois consistent with its updated return typecorestore.KVStoreWithBatch. All instances create aNewMutableTreewith the returned object, indicating compatibility with the new interface. No further issues found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of the `getTestDB` function in the test suite. # Test: Search for the function usage. Expect: Only occurrences compatible with the new return type. rg --type go -A 5 $'getTestDB'Length of output: 1820
|
will this be 1.4 or on an existing line? should we retract the version with iavldb as well? |
yes, v1.3.0 doesn't make sense at this time, keep the existing line like v1.3.1 and retract v1.3.0, how about that? |
|
This is using an unreleased version of core, so how is it going to be backported? |
Context
ref: cosmos/cosmos-sdk#21225
iavl/db.DBinterfacecosmos-dbandiavl/dbSummary by CodeRabbit
New Features
corestore.KVStoreWithBatch, improving efficiency and batch operation support across various components.DBinterface with new methodsSetandDeletefor better key-value management.Bug Fixes
Documentation
Tests
Chores