Middleware Wiring Improvements #8528
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8528 +/- ##
==========================================
+ Coverage 57.71% 57.74% +0.02%
==========================================
Files 291 292 +1
Lines 21258 21307 +49
==========================================
+ Hits 12269 12303 +34
- Misses 8421 8435 +14
- Partials 568 569 +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:
|
| @@ -26,27 +26,37 @@ var ( | |||
| // ICA controller keeper and the underlying application. | |||
| type IBCMiddleware struct { | |||
| app porttypes.IBCModule | |||
| keeper keeper.Keeper | |||
| keeper *keeper.Keeper | |||
There was a problem hiding this comment.
I made the keepers inside middlewares and modules to now be pointers so that I can call WithICS4Wrapper on them and have it persist in other references
There was a problem hiding this comment.
Is this a requirement for middlewares wiring up with this?
There was a problem hiding this comment.
If you are using a keeper function to do SendPacket and WriteAck then yes. We have middlewares that don't use keepers (e.g. callbacks) so then its not needed
| storeService: storeService, | ||
| cdc: cdc, | ||
| ics4Wrapper: ics4Wrapper, | ||
| ics4Wrapper: channelKeeper, |
There was a problem hiding this comment.
NewKeeper defaults to using channelKeeper as ICS4Wrapper. If you need this changed, you should use the stack builder primitive to set a new ICS4Wrapper
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).
| @@ -26,27 +26,37 @@ var ( | |||
| // ICA controller keeper and the underlying application. | |||
| type IBCMiddleware struct { | |||
| app porttypes.IBCModule | |||
| keeper keeper.Keeper | |||
| keeper *keeper.Keeper | |||
There was a problem hiding this comment.
Is this a requirement for middlewares wiring up with this?
|
|
||
| // Add transfer stack to IBC Router | ||
| ibcRouter.AddRoute(ibctransfertypes.ModuleName, transferStack) | ||
| ibcRouter.AddRoute(ibctransfertypes.ModuleName, transferStack.Build(app.IBCKeeper.ChannelKeeper)) |
There was a problem hiding this comment.
Would it make more sense for the channel keeper to just be set on construction instead of being passed into the build method?
| // The ICS4Wrapper **must** be used for sending packets and writing acknowledgements | ||
| // 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. |
There was a problem hiding this comment.
Just so I understand: this has always been a requirement of middlewares (to not use the channel keeper directly)?
There was a problem hiding this comment.
That is correct, though many people don't do it anyway. Which ends up being functionally ok because most of the middlewares are modifying recvpacket and not send and writeack
There was a problem hiding this comment.
Pull Request Overview
This PR refactors middleware wiring across various modules by renaming types (e.g. changing "PacketUnmarshalarModule" to "PacketUnmarshalerModule") and updating function signatures to use pointer semantics. It also simplifies constructor parameters and adjusts test cases accordingly.
- Renames interfaces for packet unmarshaling to use the corrected "PacketUnmarshalerModule" spelling.
- Updates NewIBCMiddleware constructors and keeper initializations to return pointers and remove redundant parameters.
- Refactors test cases and middleware stack building to align with the new design.
Reviewed Changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| modules/apps/packet-forward-middleware/ibc_middleware.go | Renames interface type and simplifies constructor signature. |
| modules/apps/callbacks/* | Updates naming, function signatures, and test expectations for middleware (including renaming setters and updating panic conditions). |
| modules/apps/27-interchain-accounts/* | Refactors keeper and IBC module creation (including pointer conversions and parameter adjustments) and updates associated tests. |
| modules/apps/27-interchain-accounts/controller/* | Simplifies middleware wiring and updates keeper constructors to use pointer types while removing redundant arguments. |
Comments suppressed due to low confidence (2)
modules/apps/callbacks/ibc_middleware.go:59
- The function 'SetICS4Wrapper' lacks a deprecation notice even though tests reference it as deprecated (suggesting the use of 'WithICS4Wrapper'). Please update its documentation to clarify its status and recommended usage.
func (im *IBCMiddleware) SetICS4Wrapper(wrapper porttypes.ICS4Wrapper) {
modules/apps/27-interchain-accounts/host/keeper/keeper.go:70
- Assigning 'channelKeeper' to both the 'ics4Wrapper' and 'channelKeeper' fields may be non-intuitive without additional context; ensure this design choice is intentional and consider clarifying the abstraction in code comments or documentation.
ics4Wrapper: channelKeeper,
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.