-
Notifications
You must be signed in to change notification settings - Fork 700
Middleware Wiring Improvements #8528
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
e83f32e
start stack builder
AdityaSripal cfe5e28
add testing
AdityaSripal 6b7ace2
Merge branch 'main' into aditya/mw-improvement
AdityaSripal 9e50f4b
add documentation
AdityaSripal ed1eede
Merge branch 'main' into aditya/mw-improvement
gjermundgaraba c33799e
lint and temporarily replace interchain-security
gjermundgaraba 7765fbb
Merge remote-tracking branch 'origin/main' into aditya/mw-improvement
gjermundgaraba 166bc4a
some cleanup
gjermundgaraba 3f5f265
improve functions and migration guide
AdityaSripal d7e3287
merge
AdityaSripal 6d7fda5
Update docs/docs/05-migrations/15-support-stackbuilder.md
AdityaSripal 79f0acd
address gjermund review
AdityaSripal 1530379
Merge branch 'main' into aditya/mw-improvement
gjermundgaraba 0fc30c7
fix build errors and lint
gjermundgaraba 8656668
lint docs
gjermundgaraba 6ba7a0b
Merge remote-tracking branch 'origin/main' into aditya/mw-improvement
gjermundgaraba b0c3bed
lint
gjermundgaraba File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,148 @@ | ||
--- | ||
title: Support the new StackBuilder primitive for Wiring Middlewares in the chain application | ||
sidebar_label: Support StackBuilder Wiring | ||
sidebar_position: 1 | ||
slug: /migrations/support-stackbuilder | ||
--- | ||
|
||
# Migration for Chains wishing to use StackBuilder | ||
|
||
The StackBuilder struct is a new primitive for wiring middleware in a simpler and less error-prone manner. It is not a breaking change thus the existing method of wiring middleware still works, though it is highly recommended to transition to the new wiring method. | ||
|
||
Refer to the [integration guide](../01-ibc/04-middleware/03-integration.md) to understand how to use this new middleware to improve middleware wiring in the chain application setup. | ||
|
||
# Migrations for Application Developers | ||
|
||
In order to be wired with the new StackBuilder primitive, applications and middlewares must implement new methods as part of their respective interfaces. | ||
|
||
IBC Applications must implement a new `SetICS4Wrapper` which will set the `ICS4Wrapper` through which the application will call `SendPacket` and `WriteAcknowledgement`. It is recommended that IBC applications are initialized first with the IBC ChannelKeeper directly, and then modified with a middleware ICS4Wrapper during the stack wiring. | ||
|
||
```go | ||
// SetICS4Wrapper sets the ICS4Wrapper. This function may be used after | ||
// the module's initialization to set the middleware which is above this | ||
// module in the IBC application stack. | ||
// 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. | ||
SetICS4Wrapper(wrapper ICS4Wrapper) | ||
``` | ||
|
||
Many applications have a stateful keeper that executes the logic for sending packets and writing acknowledgements. In this case, the keeper in the application must be a **pointer** reference so that it can be modified in place after initialization. | ||
|
||
The initialization should be modified to no longer take in an addition `ics4Wrapper` as this gets modified later by `SetICS4Wrapper`. The constructor function must also return a **pointer** reference so that it may be modified in-place by the stack builder. | ||
AdityaSripal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
Below is an example IBCModule that supports the stack builder wiring. | ||
|
||
E.g. | ||
|
||
```go | ||
type IBCModule struct { | ||
keeper *keeper.Keeper | ||
} | ||
|
||
// NewIBCModule creates a new IBCModule given the keeper | ||
func NewIBCModule(k *keeper.Keeper) *IBCModule { | ||
return &IBCModule{ | ||
keeper: k, | ||
} | ||
} | ||
|
||
// SetICS4Wrapper sets the ICS4Wrapper. This function may be used after | ||
// the module's initialization to set the middleware which is above this | ||
// module in the IBC application stack. | ||
func (im IBCModule) SetICS4Wrapper(wrapper porttypes.ICS4Wrapper) { | ||
if wrapper == nil { | ||
panic("ICS4Wrapper cannot be nil") | ||
} | ||
|
||
im.keeper.WithICS4Wrapper(wrapper) | ||
} | ||
|
||
/// Keeper file that has ICS4Wrapper internal to its own struct | ||
|
||
// Keeper defines the IBC fungible transfer keeper | ||
type Keeper struct { | ||
... | ||
ics4Wrapper porttypes.ICS4Wrapper | ||
|
||
// Keeper is initialized with ICS4Wrapper | ||
// being equal to the top-level channelKeeper | ||
// this can be changed by calling WithICS4Wrapper | ||
// with a different middleware ICS4Wrapper | ||
channelKeeper types.ChannelKeeper | ||
... | ||
} | ||
|
||
// WithICS4Wrapper sets the ICS4Wrapper. This function may be used after | ||
// the keepers creation to set the middleware which is above this module | ||
// in the IBC application stack. | ||
func (k *Keeper) WithICS4Wrapper(wrapper porttypes.ICS4Wrapper) { | ||
k.ics4Wrapper = wrapper | ||
} | ||
``` | ||
|
||
# Migration for Middleware Developers | ||
|
||
Since Middleware is itself implement the IBC application interface, it must also implement `SetICS4Wrapper` in the same way as IBC applications. | ||
|
||
Additionally, IBC Middleware has an underlying IBC application that it calls into as well. Previously this application would be set in the middleware upon construction. With the stack builder primitive, the application is only set during upon calling `stack.Build()`. Thus, middleware is additionally responsible for implementing the new method: `SetUnderlyingApplication`: | ||
|
||
```go | ||
// SetUnderlyingModule sets the underlying IBC module. This function may be used after | ||
// the middleware's initialization to set the ibc module which is below this middleware. | ||
SetUnderlyingApplication(IBCModule) | ||
``` | ||
|
||
The initialization should not include the ICS4Wrapper and application as this gets set later. The constructor function for Middlewares **must** be modified to return a **pointer** reference so that it can be modified in place by the stack builder. | ||
|
||
Below is an example middleware setup: | ||
|
||
```go | ||
// IBCMiddleware implements the ICS26 callbacks | ||
type IBCMiddleware struct { | ||
app porttypes.PacketUnmarshalerModule | ||
ics4Wrapper porttypes.ICS4Wrapper | ||
|
||
// this is a stateful middleware with its own internal keeper | ||
mwKeeper *keeper.MiddlewareKeeper | ||
|
||
// this is a middleware specific field | ||
mwField any | ||
} | ||
|
||
// NewIBCMiddleware creates a new IBCMiddleware given the keeper and underlying application. | ||
// NOTE: It **must** return a pointer reference so it can be | ||
// modified in place by the stack builder | ||
// NOTE: We do not pass in the underlying app and ICS4Wrapper here as this happens later | ||
func NewIBCMiddleware( | ||
mwKeeper *keeper.MiddlewareKeeper, mwField any, | ||
) *IBCMiddleware { | ||
return &IBCMiddleware{ | ||
mwKeeper: mwKeeper, | ||
mwField, mwField, | ||
} | ||
} | ||
|
||
// SetICS4Wrapper sets the ICS4Wrapper. This function may be used after the | ||
// middleware's creation to set the middleware which is above this module in | ||
// the IBC application stack. | ||
func (im *IBCMiddleware) SetICS4Wrapper(wrapper porttypes.ICS4Wrapper) { | ||
if wrapper == nil { | ||
panic("ICS4Wrapper cannot be nil") | ||
} | ||
im.mwKeeper.WithICS4Wrapper(wrapper) | ||
} | ||
|
||
// SetUnderlyingApplication sets the underlying IBC module. This function may be used after | ||
// the middleware's creation to set the ibc module which is below this middleware. | ||
func (im *IBCMiddleware) SetUnderlyingApplication(app porttypes.IBCModule) { | ||
if app == nil { | ||
panic(errors.New("underlying application cannot be nil")) | ||
} | ||
if im.app != nil { | ||
panic(errors.New("underlying application already set")) | ||
} | ||
im.app = app | ||
} | ||
``` |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Had to add this, because interchaintest pulls in this, and the changes here break it :/ |
||
|
||
replace ( | ||
github.com/cosmos/ibc-go/modules/light-clients/08-wasm/v10 => ../modules/light-clients/08-wasm | ||
// uncomment to use the local version of ibc-go, you will need to run `go mod tidy` in e2e directory. | ||
|
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll merge this now, but sounds like there are some doc improvements needed @womensrights