Adding updates to composite router and scaffolding for LegacyIBCModule #7010
Conversation
chatton
requested review from
colin-axner,
AdityaSripal,
damiannolan and
srdtrk
as code owners
| legacyRoutes map[string]ClassicIBCModule | ||
|
|
||
| // classicRoutes facilitates the consecutive calls to AddRoute for existing modules. | ||
| classicRoutes map[string][]ClassicIBCModule |
There was a problem hiding this comment.
also aware having 2 things called legacy and classic of almost the same type is terrible naming, so we can change that :D
There was a problem hiding this comment.
maybe we can add a todo that one will be removed?
|
E2E tests failing due to issue that's being fixed in #7009 |
| if _, ok := cbs.(ClassicIBCModule); ok { | ||
| rtr.classicRoutes[module] = append(rtr.classicRoutes[module], cbs) | ||
|
|
||
| // in order to facilitate having a single LegacyIBCModule, but also allowing for | ||
| // consecutive calls to AddRoute to support existing functionality, we can re-create | ||
| // the legacy module with the routes as they get added. | ||
| if classicRoutes, ok := rtr.classicRoutes[module]; ok && len(classicRoutes) > 1 { | ||
| rtr.legacyRoutes[module] = NewLegacyIBCModule(classicRoutes...) | ||
| } | ||
| } else { |
There was a problem hiding this comment.
this part I don't understand. Is it to avoid changing the current wiring in app.go?
There was a problem hiding this comment.
essentially yes. In the app.go we do things like
appStack := ...
ibcAppRouter.AddRoute(name, appStack)
appStack = wrap(appStack)
ibcAppRouter.AddRoute(name, appStack)etc.
Previously, we just built up a slice of callbacks and appended, but the idea now is that we have one type (LegacyIBCModule) that contains this list.
So we can either
A: construct the full list of callbacks and pass to router.AddClassicRoute ( from app.go )
B: internally maintain this list and build up the LegacyIBCModule ( in router_v2.go )
This is solution B. We should be able to remove this when our app.gos use version A
There was a problem hiding this comment.
I think the app.go wiring on the router v2 added in the feature branch is actually incorrect. Apps shouldn't be doing the wrap() step anymore
I prefer option A
There was a problem hiding this comment.
Ah I understand the issue now. If we want to test ics29 for example, currently it wraps other apps and doesn't have an api to return as a standalone app, so to use it for certain handshake functions, we still call the wrapping. I guess once everything is done, we can remove the additional add route calls
There was a problem hiding this comment.
Maybe we can open an issue to track removal of this logic
There was a problem hiding this comment.
I think we cannot do option A until we remove the middleware altogether, looking at some code on the feature branch now
var transferStack porttypes.IBCModule
transferStack = transfer.NewIBCModule(app.TransferKeeper)
ibcAppRouter.AddRoute(ibctransfertypes.ModuleName, transferStack)
transferStack = ibcfee.NewIBCMiddleware(transferStack, app.IBCFeeKeeper)
ibcAppRouter.AddRoute(ibctransfertypes.ModuleName, transferStack)This does not seem correct, we are currently iterating over {transfer, feeWrappedTransfer } which will almost certainly not behave the same as a single fee Wrapped transfer with router v1
There was a problem hiding this comment.
We discussed offline ^ . We will stick with current wiring and I'll open an issue to change to the final form
| legacyRoutes map[string]ClassicIBCModule | ||
|
|
||
| // classicRoutes facilitates the consecutive calls to AddRoute for existing modules. | ||
| classicRoutes map[string][]ClassicIBCModule |
There was a problem hiding this comment.
maybe we can add a todo that one will be removed?
|
…/add-composite-router
There was a problem hiding this comment.
Thanks @chatton! I re-reviewed the code and added some issue links so as not to lead to any confusion. I am going to merge and we can continue work in fresh prs
…into cian/add-composite-router
Description
This PR applies some PoC router changes to the feature branch.
The main idea being introduced is the concept of a
ClassicIBCModulewhich will be the same as theIBCModulewe have now, and then in a future PR we will strip down theIBCModuleto no longer include the handshake fns.In the channel handshake fns, we will route to the
Classicmodules, and inSend/Recvetc will will route to the slimmerIBCModulemodules.There is a bit of a hack introduced in the AddRoute function which gradually builds up the LegacyIBCModule based on consecutive calls to AddRoute. I think we should be able to remove this entirely with a single call to AddClassicRoute.
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/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.