refactor(collections): remove core dependency #24081
Conversation
📝 WalkthroughWalkthroughThis pull request refactors the dependency structure across the collections module. Several import paths have been updated to replace references from the old Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test
participant Ctx as coretesting.Context
participant KS as KVStoreService
participant DC as dummyCtx
T->>Ctx: Call Context()
Ctx-->>T: Return context with dummyCtx (store map)
T->>KS: Call KVStoreService(ctx, moduleName)
KS->>DC: Retrieve KVStore for moduleName from store map
DC-->>KS: Return KVStore
KS-->>T: Return KVStore instance
Possibly related PRs
Suggested labels
Suggested reviewers
🪧 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
|
![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
🧹 Nitpick comments (5)
collections/indexes/indexes_test.go (1)
6-8: Successfully updated test imports to use internal coretesting.The test code has been properly updated to use the new internal coretesting package and corecompat store interface, aligning with the PR's goal of removing core dependencies.
According to the static analysis hint, there might be a formatting issue with the imports. Consider running
go fmtto ensure proper formatting.- - "cosmossdk.io/collections/internal/coretesting" - - store "cosmossdk.io/collections/corecompat" + "cosmossdk.io/collections/internal/coretesting" + store "cosmossdk.io/collections/corecompat"🧰 Tools
🪛 golangci-lint (1.64.8)
6-6: File is not properly formatted
(gci)
collections/internal/coretesting/store.go (1)
14-19: Check for Overwriting Existing Stores
This function unconditionally overwrites any store that might already exist under the same module name. Consider verifying if one already exists to avoid unintended overwrites or collisions.func KVStoreService(ctx context.Context, moduleName string) store.KVStoreService { + if _, ok := unwrap(ctx).stores[moduleName]; ok { + // Optionally return the existing store or return an error/warning + } unwrap(ctx).stores[moduleName] = NewMemKV() return kvStoreService{ moduleName: moduleName, } }collections/corecompat/genesis.go (1)
13-20: Minor Typo in Documentation
There is a small grammatical typo: “closers” instead of “closes.”-// iteration. It is important the caller closers the writer AND checks the error +// iteration. It is important the caller closes the writer AND checks the errorcollections/internal/coretesting/memdb.go (1)
3-10: Format File for Consistency
The static analysis tool indicates a formatting discrepancy. Consider running tools likegciorgoimportsto ensure the imports and overall format conform to style guidelines.collections/collections_test.go (1)
10-12: Import structure updated to remove core dependencyThe imports have been updated to replace core dependencies with internal/compatible alternatives:
- Using the internal testing package
cosmossdk.io/collections/internal/coretesting- Aliasing the new store compatibility layer as
storeThis change aligns with the PR objective to remove the core dependency.
Note: There's a minor formatting issue flagged by
gcistatic analysis. Consider fixing the spacing between imports.- "cosmossdk.io/collections/internal/coretesting" - - store "cosmossdk.io/collections/corecompat" + "cosmossdk.io/collections/internal/coretesting" + store "cosmossdk.io/collections/corecompat"🧰 Tools
🪛 golangci-lint (1.64.8)
10-10: File is not properly formatted
(gci)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
collections/go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
collections/collections_test.go(1 hunks)collections/corecompat/codec.go(1 hunks)collections/corecompat/doc.go(1 hunks)collections/corecompat/genesis.go(1 hunks)collections/corecompat/store.go(1 hunks)collections/genesis_test.go(2 hunks)collections/go.mod(0 hunks)collections/indexed_map_test.go(1 hunks)collections/indexes/indexes_test.go(1 hunks)collections/internal/coretesting/context.go(1 hunks)collections/internal/coretesting/memdb.go(1 hunks)collections/internal/coretesting/store.go(1 hunks)collections/iter.go(1 hunks)collections/lookup_map_test.go(1 hunks)collections/map.go(1 hunks)collections/protocodec/collections.go(2 hunks)collections/quad_test.go(1 hunks)collections/schema.go(9 hunks)collections/triple_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- collections/go.mod
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.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.
collections/map.gocollections/corecompat/doc.gocollections/genesis_test.gocollections/triple_test.gocollections/indexed_map_test.gocollections/quad_test.gocollections/corecompat/codec.gocollections/iter.gocollections/indexes/indexes_test.gocollections/lookup_map_test.gocollections/internal/coretesting/context.gocollections/corecompat/store.gocollections/corecompat/genesis.gocollections/collections_test.gocollections/internal/coretesting/store.gocollections/protocodec/collections.gocollections/internal/coretesting/memdb.gocollections/schema.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"
collections/genesis_test.gocollections/triple_test.gocollections/indexed_map_test.gocollections/quad_test.gocollections/indexes/indexes_test.gocollections/lookup_map_test.gocollections/collections_test.go
🧬 Code Definitions (5)
collections/genesis_test.go (1)
collections/corecompat/genesis.go (1) (1)
GenesisSource(11-11)
collections/corecompat/store.go (2)
collections/protocodec/collections.go (2) (2)
value(116-116)value(126-126)collections/iter.go (1) (1)
Iterator(211-218)
collections/internal/coretesting/store.go (2)
collections/internal/coretesting/context.go (2) (2)
Context(11-18)unwrap(25-32)collections/internal/coretesting/memdb.go (1) (1)
NewMemKV(34-41)
collections/protocodec/collections.go (3)
codec/collections.go (2) (2)
collValue(75-78)protoMessage(56-59)collections/corecompat/codec.go (1) (1)
Codec(4-7)core/codec/codec.go (1) (1)
Codec(6-9)
collections/schema.go (1)
collections/corecompat/genesis.go (2) (2)
GenesisTarget(20-20)GenesisSource(11-11)
🪛 golangci-lint (1.64.8)
collections/indexes/indexes_test.go
6-6: File is not properly formatted
(gci)
collections/collections_test.go
10-10: File is not properly formatted
(gci)
collections/internal/coretesting/store.go
7-7: File is not properly formatted
(gci)
collections/schema.go
10-10: File is not properly formatted
(gci)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tests (00)
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (39)
collections/iter.go (1)
10-10:✅ Verification successful
LGTM: Successfully migrated import for store package.
The import has been correctly updated to use the new
corecompatpackage instead of the core dependency, which aligns with the PR objective to remove core dependencies.
🏁 Script executed:
#!/bin/bash # Verify the compatibility of store interfaces between core and corecompat # Check if all required store interfaces are properly defined in corecompat echo "Checking store interfaces in corecompat..." rg -A 5 "type (KVStore|Iterator)" collections/corecompat/ echo "Checking for any imports still using cosmossdk.io/core/store in collections package..." rg "cosmossdk.io/core/store" collections/ --glob "!**/*.md" --glob "!**/internal/**"Length of output: 1680
Import Migration Verified:
- The store package import in
collections/iter.gohas been successfully updated to usecosmossdk.io/collections/corecompat.- Verification shows that the store interfaces (e.g.,
KVStore,Iterator) incollections/corecompat/store.goare defined as expected.- No remaining references to the deprecated
cosmossdk.io/core/storewere found in the collections package.LGTM.
collections/map.go (1)
9-9: LGTM: Successfully migrated store import to corecompat.The import has been correctly updated to use the new
corecompatpackage instead of the direct core dependency, which aligns with the PR objective of removing core dependencies.collections/corecompat/codec.go (1)
1-27: Well-structured interface definitions for codec functionality.The new interfaces provide a clean abstraction layer for encoding and decoding operations, which allows collections to work with codecs without direct dependencies on the core package. The interfaces are properly documented and follow good Go practices with clear, descriptive names and focused responsibilities.
collections/indexes/indexes_test.go (1)
12-14: LGTM: Test helper function now uses coretesting package.The function correctly uses the new coretesting package instead of the core testing package, maintaining the same functionality while removing the core dependency.
collections/internal/coretesting/context.go (4)
9-10: Well-Scoped Context Key
Declaring an empty private struct for context keys is a common best practice, preventing key collisions.
11-18: Context Initialization Looks Good
The function neatly provides a preconfigured background context for testing. No immediate concerns noted.
20-22: Internal Structure Successfully Encapsulated
This unexporteddummyCtxstruct keeps test stores internal. It’s clear and sufficiently minimal for test usage.
24-31: Consider Avoiding Panics in Broader Context
While panics may be acceptable for testing, in production-like code paths returning an error might be safer. For test code, this is fine.collections/internal/coretesting/store.go (2)
21-23: Minimal Data Holder
The struct is straightforward, storing only the module name, aligning well with the single-purpose interface approach.
25-31: Stored Module Lookup
Panicking for a missing module is consistent with test usage, though you might consider an error return if you ever need graceful error handling.collections/corecompat/genesis.go (1)
5-11: Well-Documented Genesis Source
The function type and comments clearly describe stream-based reading. The nil response for missing fields is intuitive.collections/internal/coretesting/memdb.go (4)
28-107: In-Memory Key-Value Logic Appears Sound
TheMemKVstruct and its supporting methods (set,get,delete,iterator, etc.) are coherent. The B-Tree usage is appropriate, and error checks for empty keys are helpful. Overall, this is well-engineered for an in-memory store.
109-259: Iterator Implementation is Well-Structured
YourmemIteratorcorrectly handles ascending/descending logic and ensures valid boundaries. Panics remain acceptable for tests.
261-340: Thread-Safe Wrapper on MemKV
MemDBuses mutex locks for concurrency, wrappingMemKVeffectively. Methods likeGet,Set, andIteratorappear consistent. Excellent for testing scenarios.
342-455: Batch Operations are Correct and Feature-Rich
memDBBatchimplements essential batch sets/deletes alongside size tracking, finalizing with a write or close. The partial example of concurrency and error checks is balanced.collections/corecompat/doc.go (1)
1-4: Well-structured package documentationThe package documentation clearly explains the purpose of the
corecompatpackage and provides a solid rationale for redefinition of the interfaces fromcosmossdk.io/core. This aligns well with the PR objective of removing core dependencies.collections/indexed_map_test.go (3)
11-11: Import path update is consistent with PR objectivesThe change from using the core testing package to an internal coretesting package aligns with the goal of removing dependencies on the core module. The test code continues to function correctly with this change.
47-50: Test continues to function with new importThe test code properly utilizes the new
coretestingpackage, showing the import change was implemented correctly. The test initializes context and store service using the new package path without requiring any functional changes.
128-129: Successful migration to internal testing packageThe test initialization in this section also successfully uses the new
coretestingpackage, demonstrating consistent implementation of the dependency change throughout the file.collections/lookup_map_test.go (2)
9-9: Import path update aligns with PR objectivesThe import change from core testing to internal coretesting package successfully implements the goal of removing dependencies on the core module.
13-15: Test maintains functionality with updated importsThe test initialization correctly uses the new
coretestingpackage for creating context and key-value store service. This confirms that the internal package provides the same functionality as the previous core testing package.collections/triple_test.go (2)
10-10: Import path update follows pattern in related filesThe change to use the internal coretesting package is consistent with the pattern seen in other test files, maintaining a consistent approach to removing core dependencies.
22-24: Test implementation successfully uses new packageThe test initialization continues to work correctly with the new
coretestingpackage, confirming that the internal implementation provides the necessary functionality for the tests.collections/quad_test.go (1)
10-10: Dependency moved to internal packageThis change updates the import path from
cosmossdk.io/core/testingtocosmossdk.io/collections/internal/coretesting, which aligns with the PR objective of removing the core dependency. The test functionality remains unchanged, but now relies on an internal testing package rather than an external one.collections/genesis_test.go (2)
11-11: Import updated to use corecompat instead of core/appmoduleThe import has been updated to use the new compatibility layer instead of the direct core dependency, which aligns with the PR objective.
113-113: Function signature updated to use corecompat.GenesisSourceThe return type has been updated from
appmodule.GenesisSourcetocorecompat.GenesisSourceto match the new import. This maintains compatibility while removing the core dependency.The implementation of the function remains unchanged, which indicates that the interface defined in
corecompatmatches the original interface fromappmodule.collections/corecompat/store.go (1)
1-84: New compatibility layer implementing core store interfacesThis new file creates a compatibility layer that defines several interfaces previously imported from the core module:
KVStoreService- For retrieving KVStore from contextMemoryStoreService- For accessing memory-backed KVStoreKVStore- For key-value operationsIterator- For iterating over keysThe interfaces are well-documented with clear comments explaining their purpose, behavior, and contracts. Each method includes information about error conditions and usage requirements.
This approach effectively removes the dependency on the core module while maintaining the same interfaces, which should minimize the impact on existing code.
collections/protocodec/collections.go (3)
13-13: Import updated to use new corecompat package.The code now imports
corecompatto replace the previous core package dependency, aligning with the PR's goal of removing direct core dependencies.
60-60: Updated type casting to use the new corecompat package.The implementation now casts the codec to
corecompat.Codecinstead of using the previous core codec type while maintaining the same function signature and behavior.
64-64: Updated field type to use the new corecompat package.The
cdcfield in thecollValuestruct has been updated to usecorecompat.Codecinstead of the previous core codec type, completing the dependency migration.collections/schema.go (9)
172-172: Updated DefaultGenesis method signature.The method signature has been updated to use
corecompat.GenesisTargetinstead of its core counterpart, maintaining the same functionality while reducing direct dependencies on the core package.
183-183: Updated defaultGenesis method signature.The private implementation method signature has been updated to use
corecompat.GenesisTargetto match the public method's changes.
199-199: Updated ValidateGenesis method signature.The method signature has been updated to use
corecompat.GenesisSourceinstead of its core counterpart, maintaining the same functionality while reducing direct dependencies on the core package.
209-209: Updated validateGenesis method signature.The private implementation method signature has been updated to use
corecompat.GenesisSourceto match the public method's changes.
230-230: Updated InitGenesis method signature.The method signature has been updated to use
corecompat.GenesisSourceinstead of its core counterpart, maintaining the same functionality while reducing direct dependencies on the core package.
241-241: Updated initGenesis method signature.The private implementation method signature has been updated to use
corecompat.GenesisSourceto match the public method's changes.
262-262: Updated ExportGenesis method signature.The method signature has been updated to use
corecompat.GenesisTargetinstead of its core counterpart, maintaining the same functionality while reducing direct dependencies on the core package.
273-273: Updated exportGenesis method signature.The private implementation method signature has been updated to use
corecompat.GenesisTargetto match the public method's changes.
10-12:❓ Verification inconclusive
Import statements updated to use new corecompat package.
The code now imports the store interface and related types from
corecompatpackage instead of directly depending on core packages. However, there's a formatting issue identified by static analysis.
🏁 Script executed:
#!/bin/bash # Format the import statements properly gofmt -w collections/schema.goLength of output: 32
Attention: Verify Formatting and Import Updates in collections/schema.go
The import statements now correctly reference the new
corecompatpackage using the aliasstorealongside the direct import. The file was auto-formatted withgofmt -w, and no errors were reported. However, since thegofmtrun produced no output, please manually verify that all formatting issues flagged by static analysis have been resolved and confirm that having both the aliased and unaliased imports is intentional.🧰 Tools
🪛 golangci-lint (1.64.8)
10-10: File is not properly formatted
(gci)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
collections/internal/coretesting/memdb.go (2)
253-259: Use error returns instead of panics for invalid iterator states.Relying on a panic in
assertValid()could disrupt the flow of code using the iterator, especially in high-level application logic. It may be safer to rely on error handling rather than panicking.
261-312: Revisit lock usage for iterators.Calls to
IteratorandReverseIteratorsimply delegate tobt.kvwithout acquiring a lock, which could allow writes to occur while iterating. This can lead to inconsistent iterator views of the data. Consider adding read locks around iteration for thread safety, or document that concurrent writes are unsupported during iteration.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
collections/go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
collections/go.mod(0 hunks)collections/internal/coretesting/memdb.go(1 hunks)collections/internal/coretesting/store.go(1 hunks)
💤 Files with no reviewable changes (1)
- collections/go.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- collections/internal/coretesting/store.go
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.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.
collections/internal/coretesting/memdb.go
🧬 Code Definitions (1)
collections/internal/coretesting/memdb.go (1)
collections/internal/coretesting/store.go (1) (1)
_(10-10)
🪛 golangci-lint (1.64.8)
collections/internal/coretesting/memdb.go
22-22: var errValueNil is unused
(unused)
23-23: var errBatchClosed is unused
(unused)
341-341: type opType is unused
(unused)
344-344: const opTypeSet is unused
(unused)
345-345: const opTypeDelete is unused
(unused)
348-348: type operation is unused
(unused)
355-355: type memDBBatch is unused
(unused)
362-362: func newMemDBBatch is unused
(unused)
371-371: func (*memDBBatch).Set is unused
(unused)
387-387: func (*memDBBatch).Delete is unused
(unused)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: test-simapp-v2
- GitHub Check: test-system-v2
- GitHub Check: test-sim-nondeterminism
- GitHub Check: test-integration
- GitHub Check: build (arm64)
- GitHub Check: build (amd64)
- GitHub Check: Analyze
- GitHub Check: golangci-lint
- GitHub Check: Summary
🔇 Additional comments (1)
collections/internal/coretesting/memdb.go (1)
20-24: Static analysis warnings appear to be false positives.The
errValueNilanderrBatchClosedvariables are correctly referenced later in the batch operations, so you can safely disregard the "unused" lint messages related to them.🧰 Tools
🪛 golangci-lint (1.64.8)
22-22: var
errValueNilis unused(unused)
23-23: var
errBatchClosedis unused(unused)
| func (bt MemKV) set(key, value []byte) { | ||
| bt.tree.Set(newItem(key, value)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider validating keys and values for consistency with batch logic.
Here, MemKV#set accepts any key/value without validation, while batch-based Set checks for empty keys and nil values. Standardizing validation across both paths would help achieve consistent behavior when storing data.
| // Package corecompat defines the store, codec and genesis interface that collections expects | ||
| // or implements. These interfaces are all redefined from cosmossdk.io/core in order to | ||
| // avoid a direct dependency on cosmossdk.io/core. | ||
| package corecompat |
There was a problem hiding this comment.
Does this package feel like the right place to put this stuff?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
collections/internal/coretesting/memdb.go (4)
18-18: Run Go format/lint.The file doesn't appear to be formatted according to gofumpt style rules. Please run gofumpt or a similar tool to ensure consistent formatting.
🧰 Tools
🪛 golangci-lint (1.64.8)
18-18: File is not properly formatted
(gofumpt)
25-37: Use pointer receivers for consistency with Go best practices.While using value receivers can be acceptable in some situations, typical Go convention suggests pointer receivers for methods that modify internal state or hold references (e.g., the
*btree.BTreeGpointer). Consider changing these methods to use pointer receivers for better clarity and maintainability.-type MemKV struct { +type MemKV struct { tree *btree.BTreeG[item] } -func (bt MemKV) set(key, value []byte) { +func (bt *MemKV) set(key, value []byte) { bt.tree.Set(newItem(key, value)) }
56-73: Unify the empty-key handling.
iteratorandreverseIteratorreturn an error for empty start/end keys, butSetandDeleteallow them without complaint. Consider providing a single consistent rule (e.g., always reject empty keys) to avoid unexpected usage errors.
249-254: Reconsider panicking in test utility code.Panic on invalid iterator state can be acceptable in test-only code, but consider returning an error instead for non-catastrophic conditions. This approach can help avoid unexpected crashes and encourage more robust error handling, especially if
MemKVis used outside strict test contexts.🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 253-253: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
collections/genesis_test.go(1 hunks)collections/internal/coretesting/memdb.go(1 hunks)collections/protocodec/collections.go(1 hunks)collections/schema.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- collections/genesis_test.go
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.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.
collections/protocodec/collections.gocollections/internal/coretesting/memdb.gocollections/schema.go
🪛 golangci-lint (1.64.8)
collections/internal/coretesting/memdb.go
18-18: File is not properly formatted
(gofumpt)
collections/schema.go
10-10: ST1019: package "cosmossdk.io/collections/corecompat" is being imported more than once
(stylecheck)
11-11: ST1019(related information): other import of "cosmossdk.io/collections/corecompat"
(stylecheck)
🪛 GitHub Check: CodeQL
collections/internal/coretesting/memdb.go
[warning] 253-253: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (3)
collections/protocodec/collections.go (1)
13-13: Import path change looks goodThe import path has been correctly updated to use the new
corecompatpackage instead of the previous core dependency, which aligns with the PR objective of removing dependencies oncosmossdk.io/core.collections/internal/coretesting/memdb.go (2)
39-54: Validate keys consistently.Currently,
set,get, anddeletedo not reject empty keys, while other methods (iteratorandreverseIterator) raise errors for empty slices. Standardizing this validation across all entry points ensures consistent behavior and reduces bug risks.
75-103: Note on concurrency.The B-tree is configured with
NoLocks: true, meaning this store is not safe for concurrent usage. If concurrency is needed, add a synchronization mechanism or lock-based B-tree. Otherwise, clearly document that this store is single-thread-only.Would you like to verify if concurrency is required anywhere else in this module? If so, we can propose a locking strategy.
collections/schema.go
Outdated
| appmodule "cosmossdk.io/collections/corecompat" | ||
| store "cosmossdk.io/collections/corecompat" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix duplicate import issue
The package cosmossdk.io/collections/corecompat is being imported twice with different aliases. This violates Go style guidelines and creates unnecessary confusion.
Consider refactoring to use a single import with appropriate naming:
-appmodule "cosmossdk.io/collections/corecompat"
-store "cosmossdk.io/collections/corecompat"
+corecompat "cosmossdk.io/collections/corecompat"Then update references throughout the file to use the appropriate qualified names (e.g., corecompat.KVStore, corecompat.GenesisTarget).
📝 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.
| appmodule "cosmossdk.io/collections/corecompat" | |
| store "cosmossdk.io/collections/corecompat" | |
| corecompat "cosmossdk.io/collections/corecompat" |
🧰 Tools
🪛 golangci-lint (1.64.8)
10-10: ST1019: package "cosmossdk.io/collections/corecompat" is being imported more than once
(stylecheck)
11-11: ST1019(related information): other import of "cosmossdk.io/collections/corecompat"
(stylecheck)
|
|
||
| type dummyKey struct{} | ||
|
|
||
| func Context() context.Context { |
There was a problem hiding this comment.
I think instead of defining this stuff we should just go back to how we tested and created a test db before:
This is a ref from collections/v0.4.0
|
I don't think we need a backport label because it's a separate go mod and we can tag off of main. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
collections/go.mod (1)
17-48: Indirect Dependencies Validation.
This block adds several indirect dependencies (e.g., dependencies from CockroachDB, DataDog, Prometheus, etc.) to support the updated dependency graph after removing core. It is recommended to rungo mod tidyto ensure that all entries are necessary and there are no redundant or outdated dependencies. Additionally, please verify that these versions are compatible and do not introduce transitive conflicts.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
collections/go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
collections/go.mod(2 hunks)collections/internal/coretesting/store.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- collections/internal/coretesting/store.go
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: test-sim-nondeterminism
- GitHub Check: test-simapp-v2
- GitHub Check: test-integration
- GitHub Check: test-system-v2
- GitHub Check: build (arm64)
- GitHub Check: Analyze
- GitHub Check: build (amd64)
- GitHub Check: golangci-lint
- GitHub Check: Summary
🔇 Additional comments (1)
collections/go.mod (1)
7-7: Dependency Update: Introduce cosmos-db Dependency.
The new dependency ong.yxqyang.asia/cosmos/cosmos-db v1.1.1replaces the removed core dependency and supports the new refactoring direction. Please verify that this dependency fully meets the required interface expectations previously provided bycosmossdk.io/core.
collections/corecompat/genesis.go
Outdated
There was a problem hiding this comment.
are there any downsides with the type Foo = ... style approach here?
There was a problem hiding this comment.
The main downside is that you cannot add new interface members without breaking compatibility. We may have new store interfaces eventually but these interfaces have not changed for a long time so I consider them pretty stable.
There was a problem hiding this comment.
i guess we can just call this testutil
why dont we just target v53 here? eventually we will overwrite main with v53 contents |
Double checking this, it looks like we've already backported collections v1 to the v53 branch is that right? |
yes |
|
Okay, I'll try changing the base branch of this PR |
aaronc
force-pushed
the
aaronc/collections-no-core
branch
from
1efb5b2 to
f91a55e
Compare
|
Okay, it's rebased now |
Description
This PR removes the dependency of
cosmossdk.io/collectionsoncosmossdk.io/coreby:coretypes in thecollections/corecompatpackagecoretestingcode into thecollections/internal/testutilpackageThis should allow
collectionswork transparently with any version ofcorewhich has store types defined as type aliases (ex. #24088), or with any other version ofcoreif the proper adapter types are used.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