ICS-27 Revision

[seantking]

Continuing the conversation that has been ongoing here -> cosmos/interchain-accounts-demo#22

Issues that have been outlined thus far:

Channel Ordering:

At present, the specification does not outline how many accounts can be used/registered per channel.
The implementation here interprets the spec to mean that you can multiplex multiple accounts on a single channel namespaced via {port-id}/{channel-id}/{salt}.

A potential actor vector with this approach can be described as follows (thanks @ethanfrey):

In an unordered channel, if 5 packets come in: 1, 2, 3, 4, 5, a relayer could relay 5 and 2. An hour later relay 4. And never relay 1 or 3 (which would eventually time out).

In an ordered channel, I could not relay packet 2 until I relay 1 or I have sent proof that packet 1 timed out. That is the meaning of an ordered channel.

Now, in this case, assume there is a community pool that controls an account on another chain. I have a group account registered on the same channel. I see there is an upcoming vote from the community pool, so I send 100s of packets with long (2 weeks?) timeouts. I never pay for them. The community pool then writes packet 863 and wants to relay it. But to do so, they must wait until all my packets time out (and relay the timeout proof), or relay packets 1-862.

I see multiple distrusting accounts operating on one ordered channel as a huge attack vector. I think the definition of unordered channels and allowing multiple ordered channels on one connection are the principal advances in the ICS spec since I wrote one in late 2017 and we really should make use of this.

@okwme
You asked me earlier why can we not just say that the standard here (maybe defined in developer documentation somewhere) should be that relayers/client/helper module developers should only allow one account per channel. The problem is how can we enforce this? A user will always have to trust the client they are using, unless they are running their own relayer. In the short term, I guess this won't be a problem as we can communicate this to application developers who can enforce this in their own application logic. I guess longer term this would get messy.

Short term solution: 1 account per 1 ordered channel
Quoting @AdityaSripal
So I'm in favor of a single ORDERED channel dedicated to a single account for now, with partially ordered channels as an ideal upgrade that might happen in the 6+month time frame as ethan suggests.

This simplifies the packet data since we already know who the authorizer is whenever they send a packet through a channel.

We can just send the message bytes. The receiving chain already knows who the sender is because there can only be 1 SENDER address for that channel. So the receiving chain must simply check that the sender address of that channel is authorized to send the particular message(s) that are in the packet. NOTE: Signature checks still happen on sender chain, Application-dependent sender authentication still happens on receiving chain here.

My thinking is that the flow could look like this:

1. User submits something like a CreateICAMsg
2. The handler for this message will call OpenChanInit which will initiate the handshake process
3. A relayer will finish the handshake process. we can just abstract from how it does this. Though it is fairly trivial as it can just track events and complete handshakes, and may be incentivized to do so by end user
4. At the end of handshake process, the "register" step is complete and an address is created. This is not too complicated.
5. Basically an "address negotiation" step will be implemented in channel callbacks. We are using a similar approach for CCV, and this is definitely a supported usecase for channel callbacks.
6. User can immediately start sending messages once channel handshake is complete

The current ICS27 spec does not define specific enough implementation details for a cosmos SDK chain

This makes sense as the interchain IBC specs are for defining generic protocols. The question is where should a Cosmos ICS27 spec be kept? Should this just be an ADR/Architecture document kept in the repo with the specific implementation? Maybe we can just update the spec defined here to have more information.

Agreed upon updates to the spec:

1. Acknowledgment packet update.

Although the ICS spec leaves the actual acknowledgment as opaque bytes, it does
provide a recommendation for the format you can use, allowing contracts to
easily differentiate between success and error (and allow IBC explorers to label
such packets without knowing every protocol).

It is defined as part of the ICS4 - Channel Spec.

message Acknowledgement {
  // response contains either a result or an error and must be non-empty
  oneof response {
    bytes  result = 21;
    string error  = 22;
  }
}

2. Use cases
   A complaint has been that the ics27 spec does not outline use cases and how this protocol can be used in the real world. Should we add a section for this?

Governance: @okwme

I'm one of many delegators on a Cosmos SDK blockchain with ATOMs in its community pool. 
Me and my fellow delegators want our community pool ATOMs to be delegated to a validator on the Cosmos Hub so we can collectively earn Cosmos Hub block rewards. 
We also want to use some portion of our native token within our community pool to provide liquidity to our token on the Cosmos Hub DEX.

Smart Contracts: @ethanfrey

Implementing ICS27 (sending part) in a CosmWasm contract, and then using that contract to control an account on the Cosmos Hub. 
In particular to experiment with DAO governance and staking derivatives. (The CosmWasm governance modules are significantly more advanced and flexible than the x/groups modules planned for 0.44)

3. Remove the section mentioning that ports must be "ibcaccount" @ethanfrey

Multiple Channels and Ports are poorly defined and include some words like Both machines A and B accept new channels from any module on another machine, if and only if: The other module is bound to the "ibcaccount" port. 

This needs to be removed, as you can have multiple modules on the sending chain all sending IbcAccount messages, each with a different channel and different access rules.

4. I vote to change any mention of IBCAccount/ibcaccount to InterchainAccount/interchainaccount

[seantking]

I guess the easiest thing to do is to open a PR with the agreed-upon changes and take it from there.

[cwgoes]

I see multiple distrusting accounts operating on one ordered channel as a huge attack vector.

I agree, ordered channels should not be used by multiple mutually distrusting actors, in general.

In the future, when IBC supports something like #550, different accounts could have logically separate ordering on a single channel, which would address most of the concerns here I think.

[seantking]

Also, the initial ics-27 spec assumes that the port-id on both sides (sending chain and receiving) must be the same. I'm assuming when this was written that was the case. This is no longer true and a module on one chain can send IBC packets to a module with a different port id on the other end.

The primary goal here is to get a minimalist interchain-account module onto the hub, that allows for chains to register and control accounts on the hub. Seeing as this is the case isn't the following (described in the spec) unnecessary for the core interchain account module?

Each chain must implement the interfaces as defined in this section in order to support interchain accounts.

tryRegisterIBCAccount method in IBCAccountModule interface defines the way to request the creation of an IBC account on the host chain (or counterparty chain that the IBC account lives on).
The host chain creates an IBC account using its account creation logic, which may use data within the packet (such as destination port, destination channel, etc) and other additional data for the process.
The origin chain can receive the address of the account that was created from the acknowledge packet.

tryRunTx method in IBCAccountModule interface defines the way to create an outgoing packet for a specific type.
Type indicates how the IBC account transaction should be constructed and serialised for the host chain.
Generally, type indicates what blockchain framework the host chain was built on.

You can see how this is imeplemted here and here

Seeing as this can now be handled outside of the core interchain account module we should simply define what the receiving functionality should look like and describe the expected packet structure etc. A chain that wants to register an account on another chain should handle the logic to create outgoing packets and also handle the logic to create a new port per user & channel (ordered) creation.

[colin-axner]

Re: Channel Ordering

I think there is consensus here. Update the spec to require ordered channels which use one account per channel. It should be noted that a future version of the specification will use #550. We can use the ICA version to indicate this.

Re: cosmos sdk specific details

I'm curious to hear @cwgoes thoughts, but details such as how to decode a message being sent to an SDK chain should be added as ICS 27 SDK spec. I think in general this file should only require information needed for successful ICS 27 communication. Particularly information the counterparty or a relayer might need to successfully construct an IBC packet that can be understood once relayed. I'm not aware of anything outside of how to encode/decode the message which would be required. We should not define the actual message types. Just that it should be an encoded []sdk.Msg as opposed to specifying support for MsgSend or something like that

1. Using the standard ack makes sense to me
2. Use cases shouldn't be added to the spec, but you can add a pointer to a use case document (imo)
3. Yes this is necessary for one acc per channel support
4. Strongly in favor of this. I find ibc-account confusing

[cwgoes]

In general, I think SDK-specific details should not be included in the ICS specifications. There are cases where this is a bit awkward - this case of interchain accounts, because the content of the messages is chain-specific, and also client recovery via governance, which is quite complex but not specified here because it is specific to the SDK governance system. I think this clear separation is worth maintaining - but we should be careful to clearly specify the SDK-specific parts in the SDK repository internal documentation.

[colin-axner]

I guess the packet data decoding spec for SDK can be linked in the example implementation section. I think client recovery definitely doesn't need to be specified here as it is self contained in the ibc-go implementation (relayers or counterpartys need not implement any functionality to support it)

[ethanfrey]

Use cases shouldn't be added to the spec, but you can add a pointer to a use case document (imo)

details such as how to decode a message being sent to an SDK chain should be added as ICS 27 SDK spec.

These are 2 things I miss a lot in the current repo. They have great abstract specs, but actual details to implement it byte-for-byte compatible with another sdk implementation, as well as the higher level user stories are very valuable. Simply saying "put them somewhere else" is a bit too dismissive in my eyes.

Let us define a proper place for them and start requiring them for all new specs (and backporting them for old ones). Many good devs say the spec is almost unapproachable as the actual goal and constraints are never clearly communicated. I think this re-write of ICS27 to include such information is great and should be supported.

[ethanfrey]

As to the questions directly

1. Use standard ack 👍
2. Let's add a use case doc and find a place to store these in general
3. Yes, remove constraints
4. I love the InterchainAccountname. Let's do it.

[seantking]

Use cases shouldn't be added to the spec, but you can add a pointer to a use case document (imo)

details such as how to decode a message being sent to an SDK chain should be added as ICS 27 SDK spec.

These are 2 things I miss a lot in the current repo. They have great abstract specs, but actual details to implement it byte-for-byte compatible with another sdk implementation, as well as the higher level user stories are very valuable. Simply saying "put them somewhere else" is a bit too dismissive in my eyes.

Let us define a proper place for them and start requiring them for all new specs (and backporting them for old ones). Many good devs say the spec is almost unapproachable as the actual goal and constraints are never clearly communicated. I think this re-write of ICS27 to include such information is great and should be supported.

I was thinking about writing an ADR outlining the cosmos-sdk implementation and maybe keeping this in the modules repo (potentially in ibc-go in the future?). But, I'm totally open to whatever is decided here, it would be nice to make a decision on this soon.

I think we should agree on a protocol for this and then get moving:

1. What exactly (no more, no less) should be included in the ics-27 SDK spec (and other future specifications/architecture documents for other implementations)? Can this be an ADR kept in the implementation's own repo like I'm doing now, or something else?
2. Where are we keeping higher-level user stories & use cases?

I see @ethanfrey point about making these specifications more approachable by other developers, and I also understand the desire to keep the ics standards as abstract protocols. I think some decisions need to be made here. What does the future of interchain standards look like?

In the PR I've made I have assumed we're keeping this on the abstract side of things with each chain-specific implementation having its own specification in another repo, but I'm happy to rewrite depending on what we decide here.

[ethanfrey]

I am happy to have them separate documents and any organization.
But I think these are 3 essential items that should all be documents: what/why, abstract how, detailed sdk-how

[cwgoes]

My two cents: the documents in this repository should be sufficient for byte-to-byte compatibility & data exchange between IBC implementations - it should not be necessary to consult anywhere else. For the core standards, I think this is mostly the case - but if not, please file issues. The core standards, however, are not in any way specific to one SDK or implementation.

Perhaps we should have a different policy for application-level standards (of which ICS 27 is one). One option is to just make ICS 27 Cosmos-SDK-specific, and if someone wants to implement interchain accounts for another SDK they will be required to write a separate ICS which specifies all the specific messages. I think this is actually fine, maybe it would be clearer.