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

Add version wrap/unwrap interfaces to handle backwards compatibility #6954

Closed
3 tasks
colin-axner opened this issue Jul 25, 2024 · 2 comments
Closed
3 tasks
Assignees
Labels
05-port Issues concerns the 05-port submodule

Comments

@colin-axner
Copy link
Contributor

colin-axner commented Jul 25, 2024

Summary

initial investigation. Should be swept into a classic.BackwardsCompatibleModule interface

Must handle:

  • unwrapping channel version given an ordered list of callbacks
  • wrapping channel version given an ordered list of callbacks
  • supplying the correct application version in channel handshakes and packet processing steps

Note: needs access to ctx, portID, channelID


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@colin-axner colin-axner added the 05-port Issues concerns the 05-port submodule label Jul 25, 2024
@colin-axner colin-axner added this to the 05-port refactor alpha milestone Jul 25, 2024
@colin-axner
Copy link
Contributor Author

colin-axner commented Jul 30, 2024

Alrighty, time to detangle this mess

So, the original version of ibc only had a single application per channel. Eventually to extend ibc functionality, middlewares were added in a backwards compatible fashion. The focus on backwards compatibility required using "hacks" or short-term solutions in order to extend functionality.

The middleware design allowed the introduction of other ibc applications to act on the packet that was destined for transfer. With the port router refactor and likely multi-packet data, applications which are currently middlewares (ics29 for example), will be able to be moved to their own standalone application and still process act on packets which have a transfer action.

These middlewares, which in reality are standalone applications, required making use of some of the privileges afforded only to base applications. The channel version in this example.

This version is initially provided by the relayers in ChanOpenInit/ChanOpenTry. A middleware will try to decode this version into its own type. If it succeeds, the middleware is enabled. If it fails, the middleware should not be active for this channel. The version will be renegotiated in this fashion for ChanUpgradeInit/ChanUpgradeTry.

In all other instances, the middleware should have a contextual mapping to which channels have the version enabled (and thus have added their own wrapping). The channel version is now being provided in the packet lifecycle callbacks.

Thus we have 2 approaches to the unwrapping function:

  • brute force decode (not very safe)
  • contextual unwrapping (doesn't work on init/try handshake functions)

I would prefer to use the contextual unwrapping. One approach to handling the init/try cases is by modifying the api which relayers submit the desired version. Instead of providing the wrapped version, which applications then need to guess at, we can provide a map[string]string (or something similar) where we have a map of app name to its version. The downside of this is that relayers need to make modifications to their handshake functions, but I don't think it is the end of the world, since it is simpler than trying to provide a wrapped channel version

Alternatively, we could create 2 interfaces for unwrapping. Maybe there's another solution lurking around the corner?


We may consider storing a quicker lookup of these unwrapped versions as the version will be set on the channel in the handshake and only modified during a channel upgrade. Our setChannel function could be responsible for modifying our additional lookup

@chatton
Copy link
Contributor

chatton commented Jul 30, 2024

I think I really like the idea of the relayers providing an additional mapping, I think it makes a lot of less and would lower the overall complexity.

I've never been a fan of the wrapped channel version, since it's quite error prone. A simple map[string]string should do the trick, be simple to implement and not add greatly reduce the complexity compared to the current implementation.

I think it will be required to store these unwrapped versions in state for situations such as determining if a v1 transfer channel (on an ibc-go version that supports v2) can process a v2 packet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
05-port Issues concerns the 05-port submodule
Projects
Status: Done 🥳
Development

No branches or pull requests

2 participants