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

Modify App Callbacks to improve version negotiation flow for Relayers #628

Closed
AdityaSripal opened this issue Dec 16, 2021 · 1 comment · Fixed by #629
Closed

Modify App Callbacks to improve version negotiation flow for Relayers #628

AdityaSripal opened this issue Dec 16, 2021 · 1 comment · Fixed by #629

Comments

@AdityaSripal
Copy link
Member

AdityaSripal commented Dec 16, 2021

Thanks to @ethanfrey for his feedback here: CosmWasm/wasmvm#269

To be clear about the original solution: This was a proposal intended to allow a more complex version negotiation for ICA with minimal disruption to core IBC. NegotiateAppVersion was a helper function added to the Application interface that relayers could optionally use to determine the correct versions to place into the handshake messages. It was not a necessary part of the application semantics, but made the handshake process smoother for automated relayers. Thanks to @damiannolan for his work on this.

But as @ethanfrey points out, if we are willing to make more substantial changes to core IBC; we can still simplify the process for relayers and remove this helper function NegotiateAppVersion from the required interface for application developers.

Taking the feedback into account we propose the following changes:

  1. Deprecate Version from the ChannelOpenTry message. If the relayer is going to specify a particular version, it should be the INIT relayer choice that determines the ultimate version; not a combination of INIT relayer choice and TRY relayer choice. Note: we will not remove the field as this is a breaking change, we will simply ignore the inputted value.
  2. Change OnChanOpenTry to return version string, err error. Core IBC will then set the channel struct with the version chosen by OnChanOpenTry

This makes NegotiateAppVersion unnecessary for the ICA workflow. We plan to make the above changes before the final release of ICA to ensure that we can remove NegotiateAppVersion from the interface before it gets real world usage. This will minimize the technical debt, as we can remove it immediately without going through deprecation.

Another set of changes can be added to further simplify the process but can be added at a later time:

  1. Change OnChanOpenInit to return version string, err error. If the relayer passes in a non-empty version string, then INIT callback should return error if that version is not supported. If the relayer passes in an empty string, then INIT callback should return the default version string signifying all of the versions that it can support. If there is no "default version" for a particular application, the application should return an error.

Currently relayers hardcode the versions that they pass in for apps like ICS20. As the number of applications on-chain increase, this is unsustainable. Relayers will want to open channels for a particular port, without even understanding what application that port corresponds to and what version string they should be passing in. Passing in an empty string is effectively an "unopinionated" relayer opening a channel. The channel version may simply be negotiated from the set of versions INIT chain supports and the set of versions the TRY chain supports.

This still leaves the option of "opinionated" relayers choosing a specific version(s) by providing a non-empty version string.

This change requires some more thought and is not immediately relevant; so we plan to consider and implement this change after ICA release.

@ethanfrey
Copy link
Contributor

This sounds like a very good proposal. Thank you.

My change doesn't modify core ibc in the sense of the protocol itself, but just makes full use of the fields. I do agree this is a minor breaking change to ibc-go applications.

I very much like the idea of "" version on channel init and letting the app decide. This allows a lot more flexible usage of this field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants