-
Notifications
You must be signed in to change notification settings - Fork 586
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
Adding updates to composite router and scaffolding for LegacyIBCModule #7010
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this part I don't understand. Is it to avoid changing the current wiring in app.go?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can open an issue to track removal of this logic
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can add a todo that one will be removed?
Quality Gate passed for 'ibc-go'Issues Measures |
…/add-composite-router
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.
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
ClassicIBCModule
which will be the same as theIBCModule
we have now, and then in a future PR we will strip down theIBCModule
to no longer include the handshake fns.In the channel handshake fns, we will route to the
Classic
modules, and inSend
/Recv
etc will will route to the slimmerIBCModule
modules.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/
).godoc
comments.Files changed
in the GitHub PR explorer.SonarCloud Report
in the comment section below once CI passes.