Switch to collections in the ibcratelimit module.#2647
Open
nagarajdivine wants to merge 2 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR migrates the x/ibcratelimit module from direct KVStore key usage to collections-based storage, replacing storetypes.StoreKey with store.KVStoreService, introducing a collections.Schema and ParamsItem, changing NewKeeper and SetParams signatures, and propagating error handling where SetParams is called. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App (module wiring)
participant Keeper as ibcratelimit.Keeper
participant Schema as collections.Schema
participant Store as KVStoreService
App->>Keeper: NewKeeper(storeService, ...)
note right of Keeper: builds Schema (collections.NewSchemaBuilder)\ninitializes ParamsItem
Keeper->>Schema: ParamsItem.Set(ctx, params)
Schema->>Store: persist params under prefix
note left of App: During InitGenesis / UpdateParams
App->>Keeper: InitGenesis calls SetParams(ctx, params)
Keeper->>ParamsItem: Set(ctx, params)
ParamsItem->>Store: write params
Store-->>Keeper: ack / error
Keeper-->>App: return error (propagated) or success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
x/ibcratelimit/keeper/keeper.go (1)
26-30: Keep the collection handles unexported unless another package needs them.Exposing
SchemaandParamsItemmakes the keeper's storage layout part of its public API for no obvious gain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/ibcratelimit/keeper/keeper.go` around lines 26 - 30, The keeper exposes storage handles Schema and ParamsItem which should be unexported; rename those fields to schema and paramsItem on the keeper struct (and update any constructor/initializer and internal references to use schema and paramsItem) so the storage layout is not part of the public API, and ensure any external access uses exported getter methods if needed rather than direct field access.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@x/ibcratelimit/keeper/genesis.go`:
- Around line 28-30: The panic message in InitGenesis (inside the k.SetParams
error handling) contains a typo "ibctatelimit" making failures hard to search;
update the fmt.Errorf string used in the panic to the correct module name
"ibcratelimit" (i.e., change the message passed to panic(fmt.Errorf(...)) in the
k.SetParams error block), and scan the InitGenesis function (and nearby error
messages) for any other occurrences of the misspelled module name to correct
them similarly.
In `@x/ibcratelimit/keeper/keeper.go`:
- Around line 69-76: The current Get path masks collections.ErrNotFound by
returning ibcratelimit.DefaultParams(), hiding migration/deserialization errors;
change the behavior in the ParamsItem.Get handling so that when err != nil you
do not convert ErrNotFound into a default—return the original err (i.e., remove
the special-case that returns DefaultParams on errors from k.ParamsItem.Get) and
update callers such as ExportGenesis and the params query to handle and
propagate that error, or alternatively add a unit test that verifies older
on-chain param bytes deserialize correctly with the new format (testing
k.ParamsItem.Get round-trip) if you intentionally want backward compatibility.
In `@x/ibcratelimit/keeper/msg_server.go`:
- Around line 40-42: The error returned when calling k.SetParams(ctx,
msg.Params) in the UpdateParams handler currently contains a misspelling and the
wrong operation name; update the fmt.Errorf message inside UpdateParams (the
code that wraps the error from k.SetParams) to correctly spell "ibcratelimit"
and to say "failed to set ibcratelimit params during UpdateParams" (reference:
method UpdateParams, call k.SetParams).
---
Nitpick comments:
In `@x/ibcratelimit/keeper/keeper.go`:
- Around line 26-30: The keeper exposes storage handles Schema and ParamsItem
which should be unexported; rename those fields to schema and paramsItem on the
keeper struct (and update any constructor/initializer and internal references to
use schema and paramsItem) so the storage layout is not part of the public API,
and ensure any external access uses exported getter methods if needed rather
than direct field access.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 023d5c91-0705-4441-97cf-5ba33b6e4d2b
📒 Files selected for processing (7)
.changelog/unreleased/features/2493-collections-ibcratelimit.mdapp/app.gotestutil/ibc/testchain.gox/ibcratelimit/keeper/genesis.gox/ibcratelimit/keeper/keeper.gox/ibcratelimit/keeper/msg_server.gox/ibcratelimit/keys.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
x/ibcratelimit/keeper/keeper.go (1)
4-4: Collapse the redundantErrNotFoundbranch inGetParams.The current conditional returns the same value for all errors, so the special-case adds noise without behavior change.
♻️ Suggested simplification
import ( - "errors" "fmt" @@ func (k Keeper) GetParams(ctx sdk.Context) (params ibcratelimit.Params, err error) { params, err = k.ParamsItem.Get(ctx) if err != nil { - if errors.Is(err, collections.ErrNotFound) { - return ibcratelimit.Params{}, err - } return ibcratelimit.Params{}, err } return params, nil }Also applies to: 70-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/ibcratelimit/keeper/keeper.go` at line 4, The GetParams function currently checks for sdkerrors.ErrNotFound and returns the same (nil, false, nil) result as the generic error branch; collapse the redundant branch by removing the ErrNotFound special-case and handling all errors uniformly: on any error from k.paramstore.Get (including ErrNotFound) return (nil, false, nil) or the intended canonical response, leaving a single error-check path; update both occurrences referenced (GetParams and the similar block around lines 70-74) so only one conditional handles non-nil errors from paramstore.Get.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@x/ibcratelimit/keeper/keeper.go`:
- Line 4: The GetParams function currently checks for sdkerrors.ErrNotFound and
returns the same (nil, false, nil) result as the generic error branch; collapse
the redundant branch by removing the ErrNotFound special-case and handling all
errors uniformly: on any error from k.paramstore.Get (including ErrNotFound)
return (nil, false, nil) or the intended canonical response, leaving a single
error-check path; update both occurrences referenced (GetParams and the similar
block around lines 70-74) so only one conditional handles non-nil errors from
paramstore.Get.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc7ed41c-29fa-4d60-9638-6a08ca1801b2
📒 Files selected for processing (3)
x/ibcratelimit/keeper/genesis.gox/ibcratelimit/keeper/keeper.gox/ibcratelimit/keeper/msg_server.go
🚧 Files skipped from review as they are similar to previous changes (1)
- x/ibcratelimit/keeper/msg_server.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
closes: Switch to collections in the ibcratelimit module. #2493
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