cosmos / ibc-go

Inter-Blockchain Communication Protocol (IBC) implementation in Golang.
https://ibc.cosmos.network/
MIT License
548 stars 586 forks source link

Investigate port router default support for multi-packetdata's #6955

Closed colin-axner closed 2 months ago

colin-axner commented 2 months ago

Summary

The port router refactor should supply a port router which is designed to handle multi-packet data's. That is given an application name, it performs a single callback. If a classic IBC packet is detected, a fallback mechanism should be supplied to perform callbacks over an ordered list.

One potential idea is to have a classic router which maintains the ordered callback list, while the updated port router maintains the simple mapping from application name to the implementation. Handshake callbacks would use the classic router and classic packets would use the classic router (can be detected by inspecting port/channelid)


For Admin Use

colin-axner commented 2 months ago

Okie dokie. After thinking about this a little, I see 2 points of design:

  1. app.go api
  2. internal usage

Neither should inform the other's decision.

app.go api

I see two approaches to the app.go api

separate routers

// classic ibc routing
legacyRouter.AddRoute(transfer, []IBCModule{transferModule, ics29Module, callbacksModule})

appRouter.AddRoute(transfer, transferModule)
appRouter.AddRoute(ics29, ics29Module)
appRouter.AddRoute(callbacks, callbacksModule)

composite router

router.AddRoute(transfer, []IBCModule{transferModule, ics29Module, callbacksModule})

// add additional standalone ibc apps
appRouter.AddRoute(ics29, ics29Module)
appRouter.AddRoute(callbacks, callbacksModule)

There may be a sneakier approach to the composite, but it requires adding contractual assertions about the api, such as adding a route using the first module in the list of module. I'm not a big fan of this since you do specific actions which cannot be conveyed in the api name alone.

internal usage

separate routers/handlers

Define 2 separate routers.

if packet.IsClassic() {
    cbs = k.ClassicRouter.GetRoute(packet.PortId)
} else {
    cbs = k.AppRouter.GetRoute(packet)
}

In this case, I expect both to return back a list of callbacks. The classic router having a map[string][]IBCModule while the newer app router constructing a list of []IBCModule based on the packet.Data (multi-packetdata)

composite router

A composite router could be achieved by adding routes for legacy types with a legacy ibc module

// in appRouter.AddRoute()
if modules > 1 {
    legacyIBCModule = legacy.NewIBCModule(modules)
}
router.AddRoute(legacyRoute(transfer), legacyIBCModule)
...

// in appRouter.GetRoute()
if packet.IsClassic() {
    return router.GetRoute(legacyRoute(packet.PortID))
} else {
    // construct cbs
    for _, pd := range packet.Data {
        cbs = append(cbs, router.GetRoute(pd.PortId))
    }
}
chatton commented 2 months ago

I was kind of back and forth on the composite vs split router, but I think I prefer the composite approach. We'll be able to keep everything in one place rather than splitting across classic/new and also get the benefit of a consistent API.

Just also double checking that in your composite router example you mean to be referencing the same router instance in all cases right? Not 2 separate ones, it should be:

router.AddRoute(transfer, []IBCModule{transferModule, ics29Module, callbacksModule})

// add additional standalone ibc apps
router.AddRoute(ics29, ics29Module)
router.AddRoute(callbacks, callbacksModule)

I think we can experiment implementation wise, but overall I am in favour of the composite approach 👍

colin-axner commented 2 months ago

Just also double checking that in your composite router example you mean to be referencing the same router instance in all cases right?

yup! copy/paste issue :smile:

colin-axner commented 2 months ago

closed by #7010