Skip to content
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

Port router refactor #6941

Open
colin-axner opened this issue Jul 24, 2024 · 1 comment · May be fixed by #6314
Open

Port router refactor #6941

colin-axner opened this issue Jul 24, 2024 · 1 comment · May be fixed by #6314
Assignees
Labels
04-channel 05-port Issues concerns the 05-port submodule epic type: refactor Architecture, code or CI improvements that may or may not tackle technical debt.
Milestone

Comments

@colin-axner
Copy link
Contributor

colin-axner commented Jul 24, 2024

This issue was written with @damiannolan

Summary

The port router refactor should simplify core IBC's usage of the port router and application callbacks by:

  • removing capabilities
  • converting IBC stacks into ordered list of callbacks
  • removing the bi-directional flow of IBC core <> IBC app messaging (allows ICS4Wrapper removal)

Problem

Capability

The capability module is currently used for routing to application modules and was intended to provide object capability authorization, but in practice it only provides pseudo-security.
To accomplish this, it uses an in-memory mapping from port/channel keys to the application and requests that when applications wish to send a packet, it must retrieve it's in memory object and provide to core IBC, which can authenticate that the correct object was provided to send on that port.
The problem is that this is an overly complex approach. There is already a contractual assumption that modules built on the SDK are not malicious and in addition, using an in-memory map is very problematic as that map must be reloaded anytime a node starts and stops which can lead to very difficult bugs to debug.
The routing can be greatly simplified, by using a simple map which is defined on the app.go and relies solely on the port.

See related issue: #71, #3817

ICS4Wrapper

The ICS4Wrapper is currently used to allow actions to be initiated by the bottom most IBC application in an IBC stack. This functionality includes: SendPacket, WriteAcknowledgement and GetAppVersion.
The problem with the ICS4Wrapper is that it is wired in reverse of the IBC stack and creates an additional conceptual model of how IBC middlewares and apps are wired, which can be difficult for devs to reason about and wire correctly. There have been several instances of incorrectly wired ICS4Wrappers.
Because the flow of activity can happen in either direction from top of the stack or from bottom of the stack, it creates a cyclical dependency which is difficult to handle when wiring up the IBC stack.

See related issues: #3371, #4427

Middleware Wiring

Initially, the middleware abstractions were very useful as it allowed for other IBC applications to extend and build upon existing IBC apps without abandoning the accumulated state and network effect(s) of an already existing channel.
However, as the complexity of IBC has increased, we have found that the middleware abstraction has its limits and can be very problematic as we increase its complexity in the number of applications on a middleware stack.
This can be exemplified by the difficulty in creating new versions, packet data, or acknowledgement structures for middlewares (as discussed below), but also in the confusion around properly wiring an IBC application in an app.go file.

See related issues: #4444, #5814

Wrapping versions, acks and packet data

IBC applications may wish to act upon another IBC application, which is why they might be placed in a middleware stack and in addition, they may want their own information to be encoded into a channel version, packet data or acknowledgement.
The problem is that given the current middleware abstractions, applications must wrap their information around the version, packet data, or acknowledgement that the application below then in the stack returns.
This is problematic because it requires having access to the entire IBC stack in order to unwrap any version, packet data, or acknowledgement.

Early on after the middleware design was introduced, there was a request to allow other applications, such as ibc hooks, to extend the existing transfer packet data with their own information.
This was accomplished by adding the memo into the transfer packet data, with the idea that we would later extend the packet to support multiple packet datas.

Proposal

Remove capabilities

Both port and channel capabilities should be removed. This can be done by maintaining a simple map from the port name (IBC application name) or port prefix (dynamic port creation) to the actual application implementation.

Ordered list of callbacks

We should convert IBC application wrapping, which generates an implicit IBC stack, into an explicit ordered list of callbacks for a given port ID. The purpose of this is to decouple applications from one another, while maintaining backwards compatibility with existing IBC usage.

Future features of packet processing may choose to invoke a single application in this list of callbacks (which is why we must decouple application from one another).

Removing the bi-directional flow of IBC core <> IBC app messenging

Replacing ICS4Wrapper with core API's

The ICS4Wrapper should be replaced with API's which allow for IBC messaging to occur from core IBC initially, rather than being initiated by IBC applications

SendPacket

One example of this is the SendPacket api on the ICS4Wrapper. It should be moved to the IBC application interface as OnSendPacket. A new msg type should be added to the core IBC msg server, which allows for users or applications to send packets. It will invoke the ordered list of callbacks in the same fashion it would for receiving, acknowledging or timing out a packet.

Wrapping/unwrapping version and acknowledgement

Given that applications will no longer depend on one another when being wired, but will continue to potentially have wrapped versions and acknowledgements from historical usage, we must provide interfaces for core IBC to wrap and unwrap versions or acknowledgement when necessary. This should condense down the existing complexity around wrapping/unwrapping versions and acknoweldgement into interfaces which hopefully will be removed with the adoption of future work, such as multi-packetdata packets.

Future work, multi-packetdata

Once the port router refactor has been completed, it should be possible to introduce a new port and packet data type.
The port will indicate that a packet is sent using a new multi-packetdata structure.
This structure should contain a list of {application port, encoding type, packet data type, packet data value} which can then be delivered to an individual IBC application.
This would allow users to send packets without needing to rely on the ordered list of callbacks or the wrapped channel versions or acknowledgements.
They can choose to interact with any number of IBC applications atomically within a single packet.

A initial investigation occurred in #6314

@colin-axner colin-axner added type: refactor Architecture, code or CI improvements that may or may not tackle technical debt. 04-channel 05-port Issues concerns the 05-port submodule labels Jul 24, 2024
@colin-axner colin-axner added this to the v10.0.0 milestone Jul 24, 2024
@colin-axner colin-axner linked a pull request Jul 24, 2024 that will close this issue
10 tasks
@crodriguezvega crodriguezvega pinned this issue Jul 24, 2024
@colin-axner
Copy link
Contributor Author

Given the complexity involved with handling packet receives for classic packets using the new router format, the team is starting investigation into #7008 and #7012 and how we could enable a new v2 design on the packet/application layer which would fade out usage of the old design and enable applications to embrace a new design which would:

  • no ics4wrapper
  • no middleware wiring
  • interfaces designed for stateless middleware, one-sided applications, standard applications
  • no channel upgrades necessary (version/encoding included on per packet data level)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
04-channel 05-port Issues concerns the 05-port submodule epic type: refactor Architecture, code or CI improvements that may or may not tackle technical debt.
Projects
Status: In progress 👷
Development

Successfully merging a pull request may close this issue.

6 participants