ICS27 Updates #638

AdityaSripal · 2022-01-17T18:15:00Z

remove metadata from portID and move it to version
create structured JSON map for version and recommend as best practice for negotiating metadata
remove deleting active channel on channel closing

AdityaSripal · 2022-01-17T18:20:50Z

spec/app/ics-027-interchain-accounts/README.md

+  "Encoding": ["supported encoding types", "..."],
+  "Tx_Types": ["supported transaction types", "..."],


I've included these two to be very explicit about any assumptions shared between the two chains. However, open to feedback on if we should standardize the encoding to all implementations.

Or if we should allow the tx_type to be implicit.

Note in the implementation there will only be one supported type for each

{ ... Encoding: protobuf, Tx_Types: sdk_multi_msg, }

colin-axner · 2022-01-19T11:47:28Z

This is a lot to review at once. Is it possible to split up?

seantking · 2022-01-19T14:37:51Z

This is a lot to review at once. Is it possible to split up?

I don't mean to be awkward but I agree with @colin-axner we should try to split up these IBC spec PRs a bit in the future.

AdityaSripal · 2022-01-19T15:45:03Z

I could move the active channel unsetting out since that is logically isolated, though that is only a small part of the PR.

The rest of the changes are tightly coupled. So I could make the port changes a separate PR, however that on its own would be incorrect without the changes to the version. So then separating these, in my opinion, makes correctness hard to track because the changes that would make interchain accounts secure are now in separate PRs and it is harder to verify that the changes together still create a correct specification. I'm open to a different opinion if reviewers still think it is worth separating these two. But I would want to make sure then that people are still considering both PRs in conjunction with each other and making sure that the changes make sense together.

crodriguezvega · 2022-01-19T19:59:16Z

spec/app/ics-027-interchain-accounts/README.md

+{
+  "Version": "ics27-1",
+  "Encoding": ["supported encoding types", "..."],
+  "Tx_Types": ["supported transaction types", "..."],


nit: You don't use the _ in the other fields to separate words (e.g. ControllerConnectionId), so should it be TxTypes instead?

These 2 fields are not part of the implementation in ibc-go, is that ok?

crodriguezvega · 2022-01-19T21:11:05Z

spec/app/ics-027-interchain-accounts/README.md

+}
+```
+
+Comments: On the ChanOpenAck step, the ICS27 application on the controller chain must verify the version string chosen by the host chain on ChanOpenTry. The controller chain must verify that it can support the negotiated encoding and tx type selected by the host chain. If either is unsupported, then it must return an error and abort the handshake.


Suggested change

Comments: On the ChanOpenAck step, the ICS27 application on the controller chain must verify the version string chosen by the host chain on ChanOpenTry. The controller chain must verify that it can support the negotiated encoding and tx type selected by the host chain. If either is unsupported, then it must return an error and abort the handshake.

Comments: On the `OnChanOpenAck` step, the ICS-27 application on the controller chain must verify the version string chosen by the host chain on `OnChanOpenTry`. The controller chain must verify that it can support the negotiated encoding and tx type selected by the host chain. If either is unsupported, then it must return an error and abort the handshake.

crodriguezvega · 2022-01-19T21:12:33Z

spec/app/ics-027-interchain-accounts/README.md

+  metadata = {
+    "Version": "ics27-1",
+    "Encoding": encoding,
+    "TxType": txType,


Should it be TxTypes or TxType?

crodriguezvega · 2022-01-19T21:22:11Z

spec/app/ics-027-interchain-accounts/README.md

+    "TxType": txType,
+    "ControllerConnectionId": cpMetadata.ControllerConnectionId,
+    "HostConnectionId": cpMetadata.HostConnectionId,
+    "Account": account,


This should be Address, right?

crodriguezvega · 2022-01-19T21:22:33Z

spec/app/ics-027-interchain-accounts/README.md

  // state change to keep track of successfully registered interchain account
-  SetInterchainAccountAddress(portID, accAddr)
+  SetInterchainAccountAddress(portID, metadata.Account)


.Address instead, right?

seantking · 2022-01-20T10:20:01Z

spec/app/ics-027-interchain-accounts/README.md

@@ -81,7 +81,7 @@ This is a helper function to open a new channel.
 This is a safety function in case of a channel closing and the controller chain needs to regain access to an interchain account on the host chain.

 ```typescript
-function InitChannel(portId: string, connectionId: string) returns (nil){
+function InitChannel(portId: string, connectionId: string) returns (nil) {


Can we remove this? We decided not to implement it for the time being in ibc-go and will instead leave it up to ica-auth application developers.

seantking · 2022-01-20T10:22:06Z

spec/app/ics-027-interchain-accounts/README.md

   // checks to make sure the account has not already been registered
-   // creates a new address on chain 
+   // creates a new address on chain deterministically given counterpartyPortId


I guess this should probably also mention the connection-id based on colins issue yesterday? I think the grammar here needs an update either way.

seantking · 2022-01-20T10:25:51Z

spec/app/ics-027-interchain-accounts/README.md


-ICS-04 allows for each channel version negotiation to be application-specific. In the case of interchain accounts, the channel version set during the `OnChanOpenInit` step (controller chain) must be `ics27-<version>` & the version set during the host chain `OnChanOpenTry` step will include the interchain account address that will be created ([see summary table below](#Version-negotiation-summary)). Note that the generation of this address is stateless, and can be generated in advance of the account creation. 
+ICS-27 takes advantage of [ICS-04 channel version negotiation](../../core/ics-004-channel-and-packet-semantics/README.md#versioning) to negotiate metadata and channel parameters during the channel handshake. The metadata will contain the encoding format along with the transaction type so that the counterparties can agree on the structure and encoding of the interchain transactions. The metadata sent from the host chain on the TRY step will also contain the interchain account address, so that it can be relayed to the controller chain. At the end of the channel handshake, both the controller and host chains will store a mapping of the controller chain portID to the newly registered interchain account address ([account registration flow](#Register-account-flow)). 


This is a big improvement. Nice

AdityaSripal · 2022-01-31T14:55:44Z

Superceded by #641 and #642

* pull out just portID changes from #638 * Make metadata changes to ics27 and add json metadata as best practice in ICS4 * Update spec/app/ics-027-interchain-accounts/README.md Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * simplify encoding/tx_type negotiation and fix pseudocode formatting * fix call of RegisterInterchainAccount * Apply suggestions from code review Co-authored-by: Damian Nolan <damiannolan@gmail.com> * remove plural * fix try logic to no longer select encoding and tx type * nits * Apply suggestions from code review Co-authored-by: Marius Poke <marius.poke@posteo.de> * address mpoke suggestions Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com> Co-authored-by: Marius Poke <marius.poke@posteo.de>

AdityaSripal added 2 commits January 17, 2022 19:01

move metadata from portID to version and create structured metadata

dd3e1f2

remove deleting active channels

cc86de9

AdityaSripal requested review from cezarad and cwgoes as code owners January 17, 2022 18:15

AdityaSripal requested review from ethanfrey, seantking and colin-axner January 17, 2022 18:15

AdityaSripal commented Jan 17, 2022

View reviewed changes

AdityaSripal mentioned this pull request Jan 18, 2022

Change Fee Version format to JSON cosmos/ibc-go#756

Closed

crodriguezvega reviewed Jan 19, 2022

View reviewed changes

seantking reviewed Jan 20, 2022

View reviewed changes

AdityaSripal added a commit that referenced this pull request Jan 20, 2022

pull out just portID changes from #638

d8ecbcc

AdityaSripal mentioned this pull request Jan 20, 2022

ICS27 Port Changes #641

Merged

seantking reviewed Jan 20, 2022

View reviewed changes

AdityaSripal mentioned this pull request Jan 20, 2022

Metadata Negotiation in ICS27 #642

Merged

AdityaSripal added a commit that referenced this pull request Jan 20, 2022

pull out just portID changes from #638 (#641)

203299d

AdityaSripal closed this Jan 31, 2022

cwgoes deleted the aditya/ics27-updates branch January 31, 2022 15:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ICS27 Updates #638

ICS27 Updates #638

AdityaSripal commented Jan 17, 2022

AdityaSripal Jan 17, 2022

colin-axner commented Jan 19, 2022

seantking commented Jan 19, 2022

AdityaSripal commented Jan 19, 2022 •

edited

Loading

crodriguezvega Jan 19, 2022

crodriguezvega Jan 19, 2022

crodriguezvega Jan 19, 2022

crodriguezvega Jan 19, 2022 •

edited

Loading

crodriguezvega Jan 19, 2022

crodriguezvega Jan 19, 2022

seantking Jan 20, 2022

seantking Jan 20, 2022

seantking Jan 20, 2022

AdityaSripal commented Jan 31, 2022

		"Encoding": ["supported encoding types", "..."],
		"Tx_Types": ["supported transaction types", "..."],

	Comments: On the ChanOpenAck step, the ICS27 application on the controller chain must verify the version string chosen by the host chain on ChanOpenTry. The controller chain must verify that it can support the negotiated encoding and tx type selected by the host chain. If either is unsupported, then it must return an error and abort the handshake.
	Comments: On the `OnChanOpenAck` step, the ICS-27 application on the controller chain must verify the version string chosen by the host chain on `OnChanOpenTry`. The controller chain must verify that it can support the negotiated encoding and tx type selected by the host chain. If either is unsupported, then it must return an error and abort the handshake.


		ICS-04 allows for each channel version negotiation to be application-specific. In the case of interchain accounts, the channel version set during the `OnChanOpenInit` step (controller chain) must be `ics27-<version>` & the version set during the host chain `OnChanOpenTry` step will include the interchain account address that will be created ([see summary table below](#Version-negotiation-summary)). Note that the generation of this address is stateless, and can be generated in advance of the account creation.
		ICS-27 takes advantage of [ICS-04 channel version negotiation](../../core/ics-004-channel-and-packet-semantics/README.md#versioning) to negotiate metadata and channel parameters during the channel handshake. The metadata will contain the encoding format along with the transaction type so that the counterparties can agree on the structure and encoding of the interchain transactions. The metadata sent from the host chain on the TRY step will also contain the interchain account address, so that it can be relayed to the controller chain. At the end of the channel handshake, both the controller and host chains will store a mapping of the controller chain portID to the newly registered interchain account address ([account registration flow](#Register-account-flow)).

ICS27 Updates #638

ICS27 Updates #638

Conversation

AdityaSripal commented Jan 17, 2022

Choose a reason for hiding this comment

colin-axner commented Jan 19, 2022

seantking commented Jan 19, 2022

AdityaSripal commented Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crodriguezvega Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal commented Jan 31, 2022

AdityaSripal commented Jan 19, 2022 •

edited

Loading

crodriguezvega Jan 19, 2022 •

edited

Loading