feat! implement token filter IBC middleware #1219
Conversation
There was a problem hiding this comment.
Nice work! The general logic looks good for OnRecvPacket.
I like the approach for avoiding boilerplate code but I think there is some concerns for maintainability as its really subtle how things are wired here. My main concern is that transfer sends completely bypass the middleware
x/tokenfilter/ibc_middleware.go
Outdated
| type tokenFilterMiddleware struct { | ||
| porttypes.IBCModule | ||
| porttypes.ICS4Wrapper | ||
| } |
There was a problem hiding this comment.
Kinda nice trick with the embedding to avoid code boilerplate. Was this the intention here?
I do like the idea of not having to have a keeper package, given its a stateless middleware.
But I would've expected to see the transfer keeper composed with something like app.TokenFilterKeeper.
app.TransferKeeper = ibctransferkeeper.NewKeeper(
appCodec, keys[ibctransfertypes.StoreKey], app.GetSubspace(ibctransfertypes.ModuleName),
- app.IBCKeeper.ChannelKeeper, app.IBCKeeper.ChannelKeeper, &app.IBCKeeper.PortKeeper,
+ app.TokenFilterKeeper, app.IBCKeeper.ChannelKeeper, &app.IBCKeeper.PortKeeper,
app.AccountKeeper, app.BankKeeper, scopedTransferKeeper,
)At the moment its completely bypassing this tokenfilter middleware for SendPacket. With the above, the TokenFilterKeeper would just be a simple struct composed with the ibc channel keeper.
E.g. for packets being received the flow is: ibc core -> token filter -> transfer where token filter essentially inherits transfers callbacks but overrides OnRecvPacket. When transfer sends a packet, the current flow is: transfer -> ibc core.
While its technically fine for this right now, and token filter doesn't care about outbound packets I think the configuration might be a little bit error prone, for example, the ICS4Wrapper in this struct is never actually used. It would definitely become apparent especially if you try to add more middlewares to the transfer application stack in future.
Does this make sense?
There was a problem hiding this comment.
Kinda nice trick with the embedding to avoid code boilerplate. Was this the intention here?
Yeah that was the intention.
While its technically fine for this right now, and token filter doesn't care about outbound packets I think the configuration might be a little bit error prone, for example, the ICS4Wrapper in this struct is never actually used. It would definitely become apparent especially if you try to add more middlewares to the transfer application stack in future.
I think I understand this. So because I just reference the ChannelKeeper directly, any middleware below this would get bypassed? Technically I could remove ICS4Wrapper and the middleware would instead just be an IBCModule. Which is fine because all it's used for is in AddRoute which doesn't care that it's Middleware
Also, I'm not sure if I really care about SendPacket in the case of the TokenFilter and I would like to avoid having a keeper if possible.
There was a problem hiding this comment.
So because I just reference the ChannelKeeper directly, any middleware below this would get bypassed?
Yes, because ChannelKeeper is passed directly as the ICS4Wrapper arg to NewTransferKeeper then SendPacket would bypass all middlewares in the application stack.
Also, I'm not sure if I really care about SendPacket in the case of the TokenFilter and I would like to avoid having a keeper if possible.
Yeah I understand wanting to avoid a keeper, and like you say, SendPacket is essentially going to be a passthrough anyways and not implement any logic.
I guess its just a general concern that you should be aware of with the current way its wired up, if you decide to make any changes or add additional middlewares in future which will want to implement logic on SendPacket.
I would probably advise for taking the approach where the token filter is invoked bi-directionally (for sends and recvs), even if its a no-op/passthrough.
i.e.
recv: ibc core -> token filter -> transfer
send: transfer -> token filter -> ibc core
This way you avoid any future errors made my devs who are unaware of the call flow.
All that being said, I understand your reasoning for avoiding boilerplate and the need for a keeper package. As long as you're aware of the risks then feel free to go ahead with this.
There was a problem hiding this comment.
fwiw you could define an ICS4Wrapper struct within this package, and use that as the arg in NewTransferKeeper. You'd avoiding having to add a keeper package that way and still stick to convention and adhere to the correct flows
There was a problem hiding this comment.
Ok I've given what you suggested a go just for conventions sake.
It's does strike me however a bit strange that Middleware is both IBCModule and ICS4Wrapper. This is kind of impossible to implement within the same struct else you'd end with circular dependencies. In this case transfer keeper needs to both import token filter and be imported by token filter.
MSevey
requested review from
a team and
MSevey
and removed request for
a team
Codecov Report
@@ Coverage Diff @@
## main #1219 +/- ##
==========================================
+ Coverage 48.11% 48.41% +0.29%
==========================================
Files 72 74 +2
Lines 4086 4131 +45
==========================================
+ Hits 1966 2000 +34
- Misses 1948 1959 +11
Partials 172 172
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
MSevey
requested review from
a team
and removed request for
MSevey and
a team
|
will finish reviewing this by the end of the day. I was attempting to test using ibctest to test this Friday, but we can definitely push that into a different PR later. This change makes sense to me afaiu it! |
Yeah some integration tests were definitely on my mind as well but I don't think they're that trivial when it comes to IBC so I was going to address that in a different PR |
There was a problem hiding this comment.
nice! I don't think it could be implemented any simpler 👍 👍
created #1249 for us to create some test between now an mainnet after this gets included in a docker container on ghcr. We'll also be testing this thoroughly an all testnets between now and then.
|
also, huge thanks to @damiannolan for lending your expertise! 🙂 |
We have a pretty nice e2e module which uses the ibctest docker framework, its a thin wrapper around it to suit the needs of ibc-go. We maintain it as a separate go module which you could import. We've found it extremely valuable and its caught many small issues, even some within the sdk etc. Essentially you build a test struct which embeds Might be worth checking out some of tests in here if you're interested https://github.com/cosmos/ibc-go/blob/main/e2e |
There was a problem hiding this comment.
Very clean PR @cmwaters!
Thanks for the feedback around the ibc middleware wiring. We'd like to improve the dev ux wrt wiring application stacks and I think this PR brought some things to light especially for stateless middlewares that don't necessarily require keepers.
Will look into it thanks |
Closes: #235
This PR creates and wires up a new IBC middleware that acts as a firewall, rejecting all
FungibleTokenPacketData(i.e. transfer packets) that have a denom which did not originally came from the celestia chain.This simple implemenation will mean that the chain state only consists of the native celestia token.