switch to collections in the ibchooks module.#2768
Conversation
📝 WalkthroughWalkthroughThe ibchooks module now uses collections-backed storage for params, packet callbacks, and packet ack actors. App wiring passes a KVStoreService into the keeper, and the module registers a migration while bumping consensus version to 2. Changesibchooks collections storage
Sequence Diagram(s)Migration registration sequenceDiagram
participant AppModule
participant keeper.NewMigrator
participant cfg.RegisterMigration
participant Migrator
participant Keeper
AppModule->>keeper.NewMigrator: create Migrator from Keeper
AppModule->>cfg.RegisterMigration: register version 1 migration
cfg.RegisterMigration->>Migrator: call Migrate1to2
Migrator->>Keeper: rekey legacy state into collections storage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
x/ibchooks/keeper/migrations.go (2)
54-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: log unrecognized/unparseable legacy entries instead of silently skipping.
Entries that fail
ParseUint(Lines 58-61, 66-69) or hitdefault(Lines 73-74) are left in place with no record. For a one-shot upgrade migration, emitting their count/keys at warn level would make post-upgrade auditing of any orphaned legacy state much easier.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@x/ibchooks/keeper/migrations.go` around lines 54 - 78, The migration in migrations.go silently skips legacy entries that fail ParseUint or do not match the expected key patterns, which makes orphaned state hard to audit. Update the legacy-entry loop in the migration logic that handles packetCallbacks and packetAckActors to emit a warning (ideally including the key or at least a count) whenever an entry falls through the ParseUint failure paths or the default branch, while still continuing the migration. Keep the existing Set/Delete behavior intact and use the existing migration/keeper context to locate the logging point.
24-83: 🗄️ Data Integrity & Integration | 🔵 TrivialVerify the legacy key/value contract the whole migration depends on.
The migration is safe based on the codebase history, but the comment oversimplifies the Params preservation:
- Legacy keys: Confirmed. Legacy packet keys are raw strings
channel::seqandchannel::seq::ackwith no prefix byte. The skip logic (checking for0x01,0x02,0x03) and parsing logic hold.- Legacy values: Confirmed. Callback values are raw bech32 strings and ack values are raw bytes; the migration preserves these byte-for-byte using the new collections codecs.
- Params encoding: The claim "Params are already byte-identical" is risky without qualification. Legacy v1 used
cdc.MustMarshal(¶ms)(proto/AMINO). The new v2 usescodec.CollValue[types.Params](cdc), which delegates to the codec. While this produces identical bytes only if the codec instance andParamsproto definition are strictly unchanged, failure to ensure this invariant could cause decode panics on post-upgrade reads.Action: Add a comment or run a schema check to ensure the
Paramsproto definition and binary encoding remain unchanged in this upgrade to guarantee byte compatibility. If the proto definition or codec changed, an explicit Params migration is required.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@x/ibchooks/keeper/migrations.go` around lines 24 - 83, Verify and document the legacy key/value contract that Migrate1to2 in Migrator depends on, especially the Params path. Keep the existing packet key detection and value preservation logic, but add an explicit safeguard/comment around types.ParamsKeyBz and codec.CollValue[types.Params](cdc) to confirm the Params bytes remain compatible with the legacy cdc.MustMarshal encoding. If the Params proto/codec has changed, update the migration to rewrite Params explicitly instead of relying on byte-identical preservation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@x/ibchooks/keeper/keeper.go`:
- Around line 135-141: GetPacketCallback and GetPacketAckActor are treating
every collections.Get failure as a missing key, which hides real
read/decoding/storage errors. Update both methods to only return an empty result
when the error is collections.ErrNotFound, and otherwise propagate or surface
the unexpected error. Use the existing Keeper methods GetPacketCallback and
GetPacketAckActor plus the packetCallbacks/packetAckActors collections lookups
to apply the standard collections error handling pattern.
---
Nitpick comments:
In `@x/ibchooks/keeper/migrations.go`:
- Around line 54-78: The migration in migrations.go silently skips legacy
entries that fail ParseUint or do not match the expected key patterns, which
makes orphaned state hard to audit. Update the legacy-entry loop in the
migration logic that handles packetCallbacks and packetAckActors to emit a
warning (ideally including the key or at least a count) whenever an entry falls
through the ParseUint failure paths or the default branch, while still
continuing the migration. Keep the existing Set/Delete behavior intact and use
the existing migration/keeper context to locate the logging point.
- Around line 24-83: Verify and document the legacy key/value contract that
Migrate1to2 in Migrator depends on, especially the Params path. Keep the
existing packet key detection and value preservation logic, but add an explicit
safeguard/comment around types.ParamsKeyBz and
codec.CollValue[types.Params](cdc) to confirm the Params bytes remain compatible
with the legacy cdc.MustMarshal encoding. If the Params proto/codec has changed,
update the migration to rewrite Params explicitly instead of relying on
byte-identical preservation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23b58747-065d-438e-a603-7121691d214f
📒 Files selected for processing (7)
.changelog/unreleased/features/2492-collections-ibchooks.mdapp/app.gox/ibchooks/keeper/keeper.gox/ibchooks/keeper/migrations.gox/ibchooks/keeper/params.gox/ibchooks/module.gox/ibchooks/types/keys.go
| func (k Keeper) GetPacketCallback(ctx sdk.Context, channel string, packetSequence uint64) string { | ||
| store := ctx.KVStore(k.storeKey) | ||
| return string(store.Get(GetPacketCallbackKey(channel, packetSequence))) | ||
| v, err := k.packetCallbacks.Get(ctx, collections.Join(channel, packetSequence)) | ||
| if err != nil { | ||
| return "" | ||
| } | ||
| return v | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🌐 Web query:
In cosmossdk.io/collections v1.4.0, does Map.Get return collections.ErrNotFound for a missing key, and what other errors can it return (e.g. value decoding errors)?
💡 Result:
In cosmossdk.io/collections v1.4.0, the Map.Get method does return collections.ErrNotFound when a requested key is missing from the underlying storage [1][2]. Beyond ErrNotFound, Map.Get can return other errors, most notably those related to encoding and decoding [1]. Because the collections framework relies on value codecs to deserialize data retrieved from the store, any failure during this deserialization process will result in an error, typically categorized under collections.ErrEncoding (which is aliased to the SDK's codec encoding error) [1]. Additionally, because Map.Get interacts with the underlying KVStore, it can propagate errors returned by the storage layer itself (e.g., database access issues). Developers frequently use errors.Is(err, collections.ErrNotFound) to check for missing keys specifically, allowing them to handle such cases—such as by returning a default value—differently than actual system or data integrity errors [2].
Citations:
- 1: https://github.com/cosmos/cosmos-sdk/blob/release/v0.54.x/collections/collections.go
- 2: https://docs.cosmos.network/sdk/latest/tutorials/example/03-build-a-module
🏁 Script executed:
cat -n x/ibchooks/keeper/keeper.go | sed -n '162,184p'Repository: provenance-io/provenance
Length of output: 970
🏁 Script executed:
rg --type go "func.*GetParams.*Returns" -A 20Repository: provenance-io/provenance
Length of output: 162
🏁 Script executed:
rg --type go "func.*GetParams" -A 15Repository: provenance-io/provenance
Length of output: 162
🏁 Script executed:
rg --type go "GetParams" -n -B 2 -A 15Repository: provenance-io/provenance
Length of output: 162
🏁 Script executed:
find . -name "params.go" -type f | head -5Repository: provenance-io/provenance
Length of output: 311
🏁 Script executed:
cat -n ./x/exchange/keeper/params.go | grep -A 15 "func.*GetParams"Repository: provenance-io/provenance
Length of output: 1363
🏁 Script executed:
rg --type go "collections.ErrNotFound" -n -B 3 -A 5 | head -40Repository: provenance-io/provenance
Length of output: 162
🏁 Script executed:
rg "ErrNotFound" --type go -n | head -20Repository: provenance-io/provenance
Length of output: 162
Avoid swallowing non-ErrNotFound errors from collections Get.
GetPacketCallback and GetPacketAckActor return empty results on any error from Get. In cosmossdk.io/collections, Map.Get returns collections.ErrNotFound only for missing keys, while other errors (e.g., decoding failures, storage corruption) propagate distinct values. Handling all errors identically masks data integrity issues, potentially dropping in-flight callbacks or actors instead of surfacing the failure.
Match the standard pattern: return empty only when the key is missing, and propagate or panic on actual errors.
🛠 Proposed alignment for GetPacketCallback
func (k Keeper) GetPacketCallback(ctx sdk.Context, channel string, packetSequence uint64) string {
v, err := k.packetCallbacks.Get(ctx, collections.Join(channel, packetSequence))
if err != nil {
- return ""
+ if errors.Is(err, collections.ErrNotFound) {
+ return ""
+ }
+ panic(err)
}
return v
}🛠 Proposed alignment for GetPacketAckActor
func (k Keeper) GetPacketAckActor(ctx sdk.Context, channel string, packetSequence uint64) (string, string) {
rawData, err := k.packetAckActors.Get(ctx, collections.Join(channel, packetSequence))
if err != nil {
- return "", ""
+ if errors.Is(err, collections.ErrNotFound) {
+ return "", ""
+ }
+ panic(err)
}
// ... rest of function📝 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 (k Keeper) GetPacketCallback(ctx sdk.Context, channel string, packetSequence uint64) string { | |
| store := ctx.KVStore(k.storeKey) | |
| return string(store.Get(GetPacketCallbackKey(channel, packetSequence))) | |
| v, err := k.packetCallbacks.Get(ctx, collections.Join(channel, packetSequence)) | |
| if err != nil { | |
| return "" | |
| } | |
| return v | |
| } | |
| func (k Keeper) GetPacketCallback(ctx sdk.Context, channel string, packetSequence uint64) string { | |
| v, err := k.packetCallbacks.Get(ctx, collections.Join(channel, packetSequence)) | |
| if err != nil { | |
| if errors.Is(err, collections.ErrNotFound) { | |
| return "" | |
| } | |
| panic(err) | |
| } | |
| return v | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@x/ibchooks/keeper/keeper.go` around lines 135 - 141, GetPacketCallback and
GetPacketAckActor are treating every collections.Get failure as a missing key,
which hides real read/decoding/storage errors. Update both methods to only
return an empty result when the error is collections.ErrNotFound, and otherwise
propagate or surface the unexpected error. Use the existing Keeper methods
GetPacketCallback and GetPacketAckActor plus the packetCallbacks/packetAckActors
collections lookups to apply the standard collections error handling pattern.
1 participant
Description
closes: Switch to collections in the ibchooks module. #2492
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments..changelog/unreleased(see Adding Changes).Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.Summary by CodeRabbit
New Features
Bug Fixes