Middleware Wiring Improvements #8528
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8528 +/- ##
==========================================
- Coverage 58.00% 57.99% -0.01%
==========================================
Files 317 319 +2
Lines 22743 22814 +71
==========================================
+ Hits 13191 13231 +40
- Misses 8948 8978 +30
- Partials 604 605 +1
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:
|
AdityaSripal
requested review from
gjermundgaraba,
srdtrk and
DeshErBojhaa
as code owners
There was a problem hiding this comment.
Did a first pass, left a couple of questions, but this is clearly a big improvement, and it should be much harder to do this incorrectly. It still will require documentation and migration guides. At least the migration guide additions would be good to get sorted in this PR (for both chains and middleware devs).
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
This looks great! I left some more comments, but the main thing we need to get in here is docs on the wiring and migration docs for both chains and middleware devs.
FYI: I merged in main and linted, so you should probably take a look through all the code another time yourself too.
You should in particular check out the rate-limiting middleware as I integrated it myself, and made similar changes that you've made to the other middlewares.
| @@ -2,6 +2,9 @@ module github.com/cosmos/ibc-go/e2e | |||
|
|
|||
| go 1.24.3 | |||
|
|
|||
| // TODO: Remove when v11 release of interchaintest is available (that is where this one is coming from) | |||
| replace github.com/cosmos/interchain-security/v7 => github.com/cosmos/interchain-security/v7 v7.0.0-20250622154438-73c73cf686e5 | |||
There was a problem hiding this comment.
Had to add this, because interchaintest pulls in this, and the changes here break it :/
The PR that has the commit referenced in the replace: cosmos/interchain-security#2622
| // to ensure that the middleware can intercept and process these calls. | ||
| // Do not use the channel keeper directly to send packets or write acknowledgements | ||
| // as this will bypass the middleware. | ||
| SetICS4Wrapper(wrapper ICS4Wrapper) |
There was a problem hiding this comment.
A couple of things that might cause confusion:
- We have a mix of "SetICS4Wrapper" and "WithICS4Wrapper"
- We have these both in keepers and middleware (which I understand), but it makes for confusing interfaces
Can I suggest thinking about renaming so that perhaps the middleware SetICS4Wrapper is named distinctly from the keepers version of it? Ideally with something in the name that makes it clearer what it's for (and that it is middleware specific). There are also keepers that accidentally fulfill this interface, I don't think they should.
There was a problem hiding this comment.
That has been the general pattern.
middleware has SetICS4Wrapper
and keepers have:
WithICS4Wrapper. Is there a place where this pattern is broken or do you take issue with the pattern itself?
| @@ -0,0 +1,51 @@ | |||
| package types | |||
|
|
|||
| type IBCStackBuilder struct { | |||
There was a problem hiding this comment.
There is an opportunity to give this extra value by adding a query to ibc core (or somewhere where it makes sense) for users to see the stack. Skip go currently keeps a manual repository of which chains support PFM, for instance. If they could just query, it would help them automate this in the future.
Additionally, chain devs could debug their stack more easily.
It could either just take list here and add another function to the Middleware interface to return a name (types.ModuleName type of response), or maybe even have it come out of ICS4Wrapper so it actually flows through the application stack.
wdyt?
There was a problem hiding this comment.
This should be a seperate issue and PR if desired
There was a problem hiding this comment.
Fair! I'll create an issue for that 👍
There was a problem hiding this comment.
Pull Request Overview
This PR improves the middleware wiring across multiple modules by converting various keeper and module types to pointer types and standardizing the use of ICS4Wrapper across IBC modules. Key changes include updating the return types in NewKeeper functions, refactoring middleware stack building logic, and revising test cases to align with the new pointer-based designs.
Reviewed Changes
Copilot reviewed 50 out of 51 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| testing/simapp/app.go | Updated keeper pointer types for ICA, Transfer, and RateLimit modules. |
| testing/mock/middleware.go | Refactored ICS4Wrapper setter and related panic behavior. |
| testing/mock/ibc_module.go | Modified NewIBCModule return type to pointer and adjusted ICS4Wrapper conversion. |
| simapp/app.go | Reworked keeper instantiation and middleware wiring with pointer consistency. |
| modules/light-clients/08-wasm/testing/simapp/app.go | Applied updates for pointer types and middleware wiring in the testing simapp. |
| modules/core/api/module.go | Corrected naming of “PacketUnmarshalerModule” for clarity. |
| modules/core/ante/ante_test.go | Adjusted message creation functions to use updated parameter names. |
| modules/core/05-port/types/stack.go | Introduced IBCStackBuilder enhancements to support middleware stacking. |
| modules/core/05-port/types/module.go | Updated ICS4Wrapper setter documentation and interface methods. |
| modules/apps/transfer/** | Converted keeper and module functions to use pointer types and updated ICS4Wrapper wiring. |
| modules/apps/rate-limiting/** | Updated keeper creation, query server registration, and test cases to use pointer types. |
| modules/apps/packet-forward-middleware/** | Standardized keeper instantiation, updated ICS4Wrapper handling, and fixed type casts. |
| modules/apps/callbacks/** | Revised middleware and application wiring for callbacks, ensuring pointer correctness. |
| modules/apps/27-interchain-accounts/** | Updated keeper and IBCModule implementations for host and controller chains, including genesis and test files. |
| e2e/go.mod | Added a replace directive for interchain-security dependency version. |
| @@ -55,13 +58,26 @@ customIBCModule1 := custom.NewIBCModule(customKeeper1, "portCustom1") | |||
| customIBCModule2 := custom.NewIBCModule(customKeeper2, "portCustom2") | |||
|
|
|||
| // create IBC stacks by combining middleware with base application | |||
| // IBC Stack builders are initialized with the IBC ChannelKeeper which is the top-level ICS4Wrapper | |||
| // NOTE: since middleware2 is stateless it does not require a Keeper | |||
| // stack 1 contains mw1 -> mw3 -> transfer | |||
There was a problem hiding this comment.
from looking at this, although you specify the order I still don't think it is easy to understand conceptually and why one middleware should be before another in the ordering. Just having more examples doesn't really explain how you reason about the order of the stack. I think it is fine to have the mock examples with mw1 etc, but I think it would be useful to give more detail on how to reason about the underlying order. Concretely we have 3 middlewares in the repo and the assumption is users should expect to have them all for transfer so I think we should concretely relate to this.
There was a problem hiding this comment.
That is a good point. The code itself helps reason about what order you actually end up getting, but not necessarily which order they ought to be in.
It's hard to get too concrete on this, since it depends on which middlewares you use, but we can certainly use the ones in the repo as an example. We should def do that.
I think we could add a section on middleware ordering where we go through how the ordering works (i.e. how the ordering relates to the actual flow of a packet on send and receive) and use our own middlewares as an example to show how we decided on the order of those.
(@womensrights: Separately from this, the docs for each middleware should probably also make it clear if they have any place in the stack they should be - this is particularly relevant for rate limiting!)
There was a problem hiding this comment.
yes agreed, like there are likely some heuristics that could help you reason through ordering i.e. should this middleware act like a gate for any further processing (e.g. rate limiting) or should this middleware always be last to execute... just from the top of my head
and yes I think we should just be very concrete with what we have under our maintenance so reasoning should be really clear for pfm, rate limiting and callbacks in a stack
There was a problem hiding this comment.
also thinking about this more, it seems we are going to have multiple ways to wire middleware if you use the stack builder or not, I think this is going to be messy and confusing and there should only be one recommended way
There was a problem hiding this comment.
so perhaps this is just a docs thing as people could still choose to use the old way, but I think we only show the new way from v11 docs onwards
There was a problem hiding this comment.
I don't have good context anymore on exactly what order we should be prescribing. Especially in the context of the new middlewares in the repo.
We only are showing the stack builder approach in the docs from now on and only recommending this approach. Though i imagine some people will still use the old way.
I tried addressing this, but feel im missing the information i need at this point in terms of exactly what you want here. Since its not really in scope for the stack builder approach, Id prefer to merge this and have this comment be a general improvement of the middleware docs that we should tackle separately
There was a problem hiding this comment.
I'll merge this now, but sounds like there are some doc improvements needed @womensrights
Co-authored-by: Gjermund Garaba <[email protected]>
Description
closes: #5814
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.