feat(simapp): Wire rate limitting middleware to v2 in simapp#8919
feat(simapp): Wire rate limitting middleware to v2 in simapp#8919Eric-Warehime wants to merge 2 commits intomainfrom
Conversation
Greptile SummaryThis PR wires the rate-limiting v2 middleware in front of the ICS-20 v2 transfer module within simapp, mirroring the existing v1 middleware stack. A thin
Confidence Score: 3/5Not safe to merge as-is; native IBC v2 sends on non-aliased paths are unconditionally blocked by the current keeper implementation. A P1 defect where any outgoing native IBC v2 transfer (non-aliased client ID) triggers ErrSequenceSendNotFound inside SendRateLimitedPacket and is rejected, plus the pre-existing TODO in v2ToV1Packet flagging the conversion as potentially incorrect. modules/apps/rate-limiting/v2/ibc_middleware.go — the v2ToV1Packet TODO and the OnSendPacket path that calls SendRateLimitedPacket need review before simapp can safely enable this middleware. Important Files Changed
|
| // IBC core surfaces the original v1 channel id (e.g. "channel-X") as the | ||
| // source/destination client, so v2 transfers on those paths share the same | ||
| // (denom, channelOrClientID) rate-limit keys as classic ICS-20 transfers. | ||
| transferStackV2 := ratelimitingv2.NewIBCMiddleware(*app.RateLimitKeeper, transferv2.NewIBCModule(app.TransferKeeper)) |
There was a problem hiding this comment.
Native IBC v2 sends blocked for non-aliased client paths
SendRateLimitedPacket begins by calling k.channelKeeper.GetNextSequenceSend(ctx, sourcePort, sourceChannel) where sourceChannel is the sourceClient string from the v2 packet. For native IBC v2 paths that use actual client IDs (e.g. "07-tendermint-0") rather than aliased channel IDs, no channel-sequence entry is stored, so GetNextSequenceSend returns !found and the function immediately returns ErrSequenceSendNotFound — blocking the outgoing transfer entirely, regardless of whether any rate-limit quota is configured for that path.
The existing comment acknowledges this works for aliased v1 channels, but any native v2 client path will be unconditionally rejected by OnSendPacket. The v2 middleware should check whether the identifier corresponds to an aliased channel and skip the GetNextSequenceSend path (or skip the pending-packet tracking entirely) for native v2 client IDs.
| // IBC core surfaces the original v1 channel id (e.g. "channel-X") as the | ||
| // source/destination client, so v2 transfers on those paths share the same | ||
| // (denom, channelOrClientID) rate-limit keys as classic ICS-20 transfers. | ||
| transferStackV2 := ratelimitingv2.NewIBCMiddleware(*app.RateLimitKeeper, transferv2.NewIBCModule(app.TransferKeeper)) |
There was a problem hiding this comment.
Value copy of keeper may drift from the singleton
*app.RateLimitKeeper dereferences the pointer and passes a shallow value copy of the keeper to NewIBCMiddleware. The v2 IBCMiddleware stores this copy by value (keeper keeper.Keeper). Any future call to app.RateLimitKeeper.SetICS4Wrapper(...) — e.g. added during stack-wiring refactors — would update only the original pointer and not the copy inside the v2 middleware. The v1 stack avoids this by passing the pointer directly. Consider accepting *keeper.Keeper in NewIBCMiddleware (mirroring the v1 signature) to keep both stacks consistent and resilient to future wiring changes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8919 +/- ##
=======================================
Coverage 66.52% 66.52%
=======================================
Files 327 327
Lines 17143 17143
=======================================
Hits 11404 11404
Misses 5048 5048
Partials 691 691
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Can you also wire this in testing/simapp/app.go
| // IBC core surfaces the original v1 channel id (e.g. "channel-X") as the | ||
| // source/destination client, so v2 transfers on those paths share the same | ||
| // (denom, channelOrClientID) rate-limit keys as classic ICS-20 transfers. | ||
| transferStackV2 := ratelimitingv2.NewIBCMiddleware(*app.RateLimitKeeper, transferv2.NewIBCModule(app.TransferKeeper)) |
There was a problem hiding this comment.
I think there is a problem in the rate limit v2, this keeper should have taken a pointer rather than a copy. So that we can have:
| transferStackV2 := ratelimitingv2.NewIBCMiddleware(*app.RateLimitKeeper, transferv2.NewIBCModule(app.TransferKeeper)) | |
| transferStackV2 := ratelimitingv2.NewIBCMiddleware(app.RateLimitKeeper, transferv2.NewIBCModule(app.TransferKeeper)) |
This should be a simple change in ratelimitingv2.
Description
Wire rate limitting into v2 transfer stack within simapp.
closes: #XXXX
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/) if anything is changed.godoccomments if relevant.Files changedin the GitHub PR explorer.