-
Notifications
You must be signed in to change notification settings - Fork 410
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
Metadata Negotiation in ICS27 #642
Conversation
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.
Looks great! General ACK, but see comments
Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
@@ -162,7 +162,7 @@ This port will be used to create channels between the controller & host chain fo | |||
2. The controller chain emits an event signaling to open a new channel on this port given a connection. | |||
3. A relayer listening for `ChannelOpenInit` events will continue the channel creation handshake. | |||
4. During the `OnChanOpenTry` callback on the host chain an interchain account will be registered and a mapping of the interchain account address to the owner account address will be stored in state (this is used for authenticating transactions on the host chain at execution time). | |||
5. During the `OnChanOpenAck` callback on the controller chain a record of the interchain account address registered on the host chain during `OnChanOpenTry` is set in state with a mapping from portID -> interchain account address. See [version negotiation](#Version-negotiation) section below for how to implement this. | |||
5. During the `OnChanOpenAck` callback on the controller chain a record of the interchain account address registered on the host chain during `OnChanOpenTry` is set in state with a mapping from portID -> interchain account address. See [metadata negotiation](#Metadata-negotiation) section below for how to implement this. |
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.
@colin-axner Did we say that we needed to update this mapping to include connection seq?
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.
The portID is unique from the perspective of the controller chain, so no the connection seq is not needed
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.
Great improvements 🤝
Would it be worth adding this check in this PR as well? I can also do a small follow-up.
Co-authored-by: Damian Nolan <damiannolan@gmail.com>
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.
Where do we plan to specify what the TxType
values mean. For example, in ibc-go if we specify the tx type as sdk.Msg
, where can we put this in the spec?
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.
Looks great. I left some comments bellow.
@@ -317,21 +381,37 @@ function onChanOpenTry( | |||
channelIdentifier: Identifier, | |||
counterpartyPortIdentifier: Identifier, | |||
counterpartyChannelIdentifier: Identifier, | |||
version: string, | |||
counterpartyVersion: string) { | |||
counterpartyVersion: string) (version: string) { |
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 is not consistent with the definition of onChanOpenTry
in ICS-26. However, it is consistent with the ibc-go implementation. Could you also modify ICS-26 in this PR or should we open an issue?
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.
Yes we should create a new issue to update ICS26
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.
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.
Yeah, it was modified by #629. I was looking at an older branch. We really need to add some release tags to the IBC specs and separate IBC apps from IBC core. It will become very difficult to keep track of all these changes to IBC core (some of them quite relevant) while writing spec for apps.
Co-authored-by: Marius Poke <marius.poke@posteo.de>
Pull Metadata negotiation logic out of #638