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

update: major revisions to ics-27 specification #567

Merged
merged 23 commits into from
Aug 6, 2021

Conversation

seantking
Copy link
Contributor

@seantking seantking commented Apr 27, 2021

Updating the ics-027 specification based on ongoing discussions:

#564
cosmos/interchain-accounts-demo#22

message Acknowledgement {
// response contains either a result or an error and must be non-empty
oneof response {
bytes result = 21;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do we want to send back to the sending chain on a successful RunTx? I specified above that we should send the chain-id and the address of the interchain account but I'm wondering what is actually necessary here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we defined the subcall standard in CosmWasm (to allow getting results from a call to another contract/module), we pass back the data field and the events returned.

A similar format is used in the tx responsed (data and events for each message).

Maybe events are too variable for returning over IBC, but I would at least include the data field (which is part of the ResultHash)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure what an event would be in this context. Would you mind giving me an example?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every message in the sdk returns optional binary data and a list of events.

Events each have a type string, and a list of key-value attributes (also strings).

Eg.bank send emits "transfer" type with recipient, sender, and amount as attributes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is best to allow chains to specify their own interpretation of the result as opposed to defining some standard within interchain-account. The interchain-account module should define that it expects the standard Acknowledgement envelope and then pass the results to the helper module. In the same way the helper module is expected to be capable of constructing a message the receiving chain can interpret, it should be capable of understanding the responses from the receiving chain

It is probably hard to generalize all usage of the result into a single definition

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should at least be agreed upon in the version. Perhaps SDK chains should be sending result and events back

`InterchainAccountPacketData` contains an array of messages that an interchain account can run and a memo string that is sent to the recieving chain. The example below is defined as a proto encoded message but each chain can encode this differently. This should be clearly defined in the documentation for each chain specific implementation.

```typescript
message InterchainAccountPacketData {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically what our RunTx message type was previously. Seeing as we have removed the need for the register message entirely this makes sense to me. The only message type we will be sending will be for running transactions on the receiving chain.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. But you may want to make multiple types for future extensibility? ics27-2

Keeping a type field (Any) here with just one defined, supported concrete form is a good step there.
(Not necessary, but an idea)

```typescript
message InterchainAccountPacketData {
repeated google.protobuf.Any messages = 1;
string memo = 2;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ethanfrey What was your idea for using memo here? I'm wondering what data is useful to send here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can drop it. It just came from the standard sdk.Tx protobuf type, and figured it might be interesting for documentation. The same way the memo field on transactions doesn't actually do anything but can be observed by clients.

Fine to remove if you don't want it. Figured an optional field wouldn't hurt

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of keeping memo. It seems to be a key way for exchanges and wallets to track information. We're running into this issue on ics-20


`runTx` executes a transaction after the transaction has been successfully authenticated.
The recieving chain must implement authentication with regards to ensuring that the incoming messages are sent by the owner of the targetted interchain account. With regards to authorization, it is up to each chain specific implementation to decide if the hosted interchain accounts have authority to invoke all of the chains message types or only a subset. This configuration may be set up at module initialization.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation does not have any concept of Authorization on the receiving chain i.e. what messages interchain accounts can execute. I think having this as an optional feature makes sense. Some chains may not want remote chains to be able to execute every message type. Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea is good in the spec. Allowing the receiver to reject packets.

The wire protocol would remain the same, correct? Just another potential error case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. In Cosmos SDK land this could just be a list of message types we can pass into the module at init time as an optional field. If the incoming message type is disallowed it will throw an unauthorized error.

@@ -201,14 +208,15 @@ function onChanOpenAck(
// port has already been validated
// assert that version is "ics27-1"
abortTransactionUnless(version === "ics27-1")
// state change to keep track of successfully registered interchain account
confirmInterchainAccountRegistration()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where we would invoke a state change to keep a local registry of what accounts are registered on what chains. I'm wondering, can we pass back the address of the interchain account here?

@AdityaSripal Is this what you had in mind?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of passing back account here

Copy link
Contributor Author

@seantking seantking May 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@colin-axner I have three questions:

  1. Is it possible to pass the address back here? I searched through the spec & looked at the IBC module and I see no way to pass data back and forth during the handshake process. I'm guessing no.

  2. Is it even necessary to pass the account back here? I guess we should be able to infer what the address of the registered interchain account is based on the channel-id + port-id (interchain_account).

  3. A big concern I have here is what happens if the OnChanOpenConfirm callback is never fulfilled? We are confirming our interchain account having been registered here, but what if the channel is never actually completely created (if the handshake doesn't get completed). How does IBC handle this? What can cause a handshake to never be fulfilled (OnChanOpenConfirm not being called/throwing an error)? I guess if it's an error on the final callback then the channel won't get created. Otherwise, I imagine it's up to the relayer-specific logic to keep retrying. This is probably an edge case, however, in the case of creating an interchain account on behalf of say the distribution module, whereby only one interchain account will probably be created (still up to the sending side), we don't want a situation whereby we get a false confirmation.

I guess even in the case of a false confirmation, we could bake in some logic to check if the channel exists before using the account and if it doesn't we can try and create a new channel/register a new account.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. No, channels are really not designed to be passing information. The only mechanism I can think of is using the channel version
  2. Correct, although I'm a little confused. Isn't this the controlling side so won't the account be the portID? The counterparty can get this information if needed via the counterparty PortID
  3. Packets cannot be received if the channel isn't OPEN. I don't see any reasons for concern. The handshake can always be completed unless there is a bug or the counterparty chain refuses to communicate. OnChanAck is like the account is created and OnChanConfirm is like the account is ready to be used

@seantking seantking marked this pull request as ready for review April 28, 2021 12:25

`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.
### Architecture Diagram
![](https://i.imgur.com/HX1h2u2.png)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hosted this on imgur just for now. Where should we keep this asset?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://giphy.com/ :trollface:

But seriously, can't you commit the png to this repo and link it in the same directory?

Copy link
Contributor Author

@seantking seantking Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's the obvious solution. I just wanted to agree a protocol for this. All the images in the other specs are currently broken links.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use git lfs to commit .png to the repository - the other links we should fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any thoughts on the actual diagram in general?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand the arrows, why are arrows going to the relayer? The ordering (numbers) makes sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback, perhaps we can remove/improve the arrows in some way.

In this context, we're calling onChanOpenInit and emitting an event, which the relayer picks up and then begins the handshake process on both chains for creating a channel. The arrows are trying to symbolize that the relayer is both listening for events + coordinating the handshake process. I guess this isn't being communicated clearly tho.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see - yes, that's true, maybe labelling the arrows or something would help.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this in the end.

@seantking seantking mentioned this pull request Apr 30, 2021
- Fault containment: Interchain account must follow rules of its host chain, even in times of Byzantine behaviour by the counterparty chain (the chain that manages the account)
- The chain that controls the account must process the results asynchronously and according to the chain's logic. The result's code should be 0 if the transaction was successful and an error code other than 0 if the transaction failed.
- Sending and receiving transactions will be processed in an ordered channel where packets are delivered exactly in the order which they were sent.
- Fault containment: An interchain account must follow rules of its host chain, even in times of Byzantine behavior by the counterparty chain (the chain that manages the account)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand what this means - what kind of protection is possible, if the chain managing the account is Byzantine? Seems to me like you'd need extra signatures or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question, I left it in but I wasn't entirely sure either if I'm being honest. I think the point here is that the interchain account is just a regular account, even tho it's being controlled by another chain it has no more power than any other account on the chain.

I'm okay with removing this if others are.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see - I think that's a good point, but it's not what I assumed the sentence meant, maybe we can just rephrase?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that we need to clarify what are the semantics we want to provide in case of forks on controlling chain.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also like interchain accounts that have limited off-chain control. e.g., you can control staking but not selling. In that case, byzantine behavior by the controller could only compromise the allowed subset of functionality.


`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.
### Architecture Diagram
![](https://i.imgur.com/HX1h2u2.png)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use git lfs to commit .png to the repository - the other links we should fix.

spec/app/ics-027-interchain-accounts/README.md Outdated Show resolved Hide resolved
Co-authored-by: Christopher Goes <cwgoes@pluranimity.org>
Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

part way through, will continue reviewing later - currently on sending interface

- The chain that controls the account must process the results asynchronously and according to the chain's logic. The result's code should be 0 if the transaction was successful and an error code other than 0 if the transaction failed.
- Sending and receiving transactions will be processed in an ordered channel where packets are delivered exactly in the order which they were sent.
- Fault containment: An interchain account must follow rules of its host chain, even in times of Byzantine behavior by the counterparty chain (the chain that manages the account)
- The chain that controls the account must process the results asynchronously and according to the chain's logic. The acknowledgment message should contain a result of an error as described in [ics-4](https://github.com/cosmos/ibc/tree/master/spec/core/ics-004-channel-and-packet-semantics#acknowledgement-envelope).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand according to the chain's logic. This seems to be referring to receiving acknowledgements but what logic is done when processing a failed/successful acknowledgement?

Or maybe the wording is incorrect? Do you think it is referring to the OnRecv or OnAcknowledge callbacks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I'll update this.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a problem with this beyond that it's maybe irrelevant/distracting information. The origin chain may have custom logic like: once first tx is submitted and successfully processed create a new one and begin the process of submitting it. Basically the origin chain is must process the results async and is able to use that ack within it's own logic.

Copy link

@okwme okwme May 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- The chain that controls the account must process the results asynchronously and according to the chain's logic. The acknowledgment message should contain a result of an error as described in [ics-4](https://github.com/cosmos/ibc/tree/master/spec/core/ics-004-channel-and-packet-semantics#acknowledgement-envelope).
- The chain that controls the account must process the results asynchronously. The acknowledgment message should contain a result of an error as described in [ics-4](https://github.com/cosmos/ibc/tree/master/spec/core/ics-004-channel-and-packet-semantics#acknowledgement-envelope) which can be used as part of the chain's own application logic.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

result of an error

"result or error..."?

spec/app/ics-027-interchain-accounts/README.md Outdated Show resolved Hide resolved
spec/app/ics-027-interchain-accounts/README.md Outdated Show resolved Hide resolved
spec/app/ics-027-interchain-accounts/README.md Outdated Show resolved Hide resolved
spec/app/ics-027-interchain-accounts/README.md Outdated Show resolved Hide resolved
spec/app/ics-027-interchain-accounts/README.md Outdated Show resolved Hide resolved
spec/app/ics-027-interchain-accounts/README.md Outdated Show resolved Hide resolved
spec/app/ics-027-interchain-accounts/README.md Outdated Show resolved Hide resolved
spec/app/ics-027-interchain-accounts/README.md Outdated Show resolved Hide resolved
spec/app/ics-027-interchain-accounts/README.md Outdated Show resolved Hide resolved
seantking and others added 4 commits May 4, 2021 17:13
Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic work!! LGTM. Left mostly typo fixes and a couple suggestions

spec/app/ics-027-interchain-accounts/README.md Outdated Show resolved Hide resolved
spec/app/ics-027-interchain-accounts/README.md Outdated Show resolved Hide resolved
message Acknowledgement {
// response contains either a result or an error and must be non-empty
oneof response {
bytes result = 21;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is best to allow chains to specify their own interpretation of the result as opposed to defining some standard within interchain-account. The interchain-account module should define that it expects the standard Acknowledgement envelope and then pass the results to the helper module. In the same way the helper module is expected to be capable of constructing a message the receiving chain can interpret, it should be capable of understanding the responses from the receiving chain

It is probably hard to generalize all usage of the result into a single definition

spec/app/ics-027-interchain-accounts/README.md Outdated Show resolved Hide resolved
spec/app/ics-027-interchain-accounts/README.md Outdated Show resolved Hide resolved
seantking and others added 2 commits May 7, 2021 19:33
Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
RUNTX
}
#### Sending Interface
The `tryRegisterInterchainAccount` method in the `InterchainAccountModule` interface defines the way to request the creation of an interchain account on a remote chain. The remote chain creates an interchain account using its own account creation logic. Due to the limitation of ordered channels, the recommended way to achieve this when calling `tryRegisterInterchainAccount` is to dynamically bind a new port with the port id set as the address of the owner of the account (if the port is not already bound), invoke `OpenChanInit` via an IBC module which will initiate the handshake process and emit an event signaling to a relayer to generate a new channel between both chains. The remote chain can then create the interchain account in the `ChanOpenTry` callback as part of the channel creation handshake process defined in [ics-4](https://github.com/cosmos/ibc/tree/master/spec/core/ics-004-channel-and-packet-semantics). Once the `chanOpenAck` callback fires the handshake-originating (sending) chain can assume the account registration is successful.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the reason why the protocol is tied to channel handshake is a requirement that interchain account needs to be mapped to a single channel? What I don't get is how the mapping between interchain account on dst chain, controller on source chain and channel is achieved.

Copy link
Contributor Author

@seantking seantking May 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm planning to push some updates to this tomorrow. I don't think the emitting of the event is necessary here or needs to be part of the protocol. Also, the relayer functionality for finishing a handshake based on an event is not implemented yet.

Is the reason why the protocol is tied to channel handshake is a requirement that interchain account needs to be mapped to a single channel?

This was the idea. But perhaps this does not need to be part of the protocol at all. I'm thinking that we begin registering the account on the onChanOpenInit callback rather than wait for the event as mentioned here.

### Port & channel setup
Receiving chains (chains that will host interchain accounts) must always bind to a port with the id `interchain_account`. Sending chains will bind to ports dynamically, with each port id being the address of the interchain account owner.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can a single account owner (identified with its address) be owning several interchain accounts on a given receiving chain?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also curious if a single account can own several interchain accounts on different receiving chains. I'd like to use a single account on chainA to create an account on chainB, chainC, and chainD and I can use my one authentication setup to control all 3 accounts

- The channel being created is ordered.
- The version string is "ics27-1".
- The channel being created is ordered
- The version string is "ics27-1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a need to check if port id is equal to account address and that message is signed with the address owner, or we don't care if someone else create interchain account in name of some owner's address as only owner can control it after?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a good point, as technically anyone can create a channel. I think it's okay to allow anyone to create a channel as the account can only be controlled by the owner anyway.

I'm not sure how we would enforce this otherwise. The relayer will be signing these messages right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since ChanOpenInit is being initiated by module from the RegisterAddress function, we can check address against the signer of the RegisterAddress message

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this should be checked on controlling chain, NOT host chain as this will only get relayer-signed messages

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the code on controller chain is enforcing this property we don't need a check at all

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. We do need a check on executeTx (host chain) to ensure that the signer of a message (returned from getSigners()) is in fact the interchain account address associated with the SourcePortId. Impl;cemented like so: https://github.com/cosmos/ibc-go/blob/interchain-accounts/modules/apps/ibc-account/keeper/relay.go#L144

}
```

```typescript
function onChanOpenConfirm(
portIdentifier: Identifier,
channelIdentifier: Identifier) {
// accept channel confirmations, port has already been validated
}
```

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case channel is closed, access to the interchain account is lost if I am not wrong. If that's true, access to assets is lost, so I wonder if there is something we can do here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking there are probably several ways to achieve reclaiming an account.

A question that comes to mind: Do we want several channels to be active at one time, all of which can send instructions to the account?

Also, if owner A needs to reclaim an account, what channel are they using to do that? In the case of a DAO the reclaiming process would need to be communicated over IBC.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just maintain a single active channel at a time. Multiple channels at a time means there's no global ordering of messages

- Fault containment: Interchain account must follow rules of its host chain, even in times of Byzantine behaviour by the counterparty chain (the chain that manages the account)
- The chain that controls the account must process the results asynchronously and according to the chain's logic. The result's code should be 0 if the transaction was successful and an error code other than 0 if the transaction failed.
- Sending and receiving transactions will be processed in an ordered channel where packets are delivered exactly in the order which they were sent.
- Fault containment: An interchain account must follow rules of its host chain, even in times of Byzantine behavior by the counterparty chain (the chain that manages the account)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Fault containment: An interchain account must follow rules of its host chain, even in times of Byzantine behavior by the counterparty chain (the chain that manages the account)
- Fault containment: The origin chain of an Interchain Account has complete control of authorization and authentication for the account. The host chain accepts at face value that the origin chain is acting correctly when sending messages on behalf of the Interchain Account.

This was my interpretation of the meaning and purpose of the section.

The interchain account specification defines the general way to register an interchain account and transfer tx bytes. The counterparty chain is responsible for deserialising and executing the tx bytes, and the sending chain should know how counterparty chain will handle the tx bytes in advance.
The implementation of interchain accounts is non-symmetric. This means that each sending chain can have a different way to generate an interchain account and each receiving chain can have a different set of transactions that may be deserialised in different ways. For example, chains that use the Cosmos SDK will deserialise tx bytes using Protobuf, but if the counterparty chain is a smart contract on Ethereum, it may deserialise tx bytes by an ABI that is a minimal serialisation algorithm for the smart contract.

A chain can implement one or both parts to the interchain accounts protocol (sending and receiving). A sending chain registering and controlling an account does not necessarily have to allow other chains to register accounts on its own chain, and vice versa.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
A chain can implement one or both parts to the interchain accounts protocol (sending and receiving). A sending chain registering and controlling an account does not necessarily have to allow other chains to register accounts on its own chain, and vice versa.
A chain can implement one or both parts of the interchain accounts protocol (sending and receiving). A sending chain registering and controlling an account does not necessarily have to allow other chains to register accounts on its own chain, and vice versa.


Also, each chain must know how the counterparty chains serialise/deserialise transaction bytes in order to send transactions via IBC. And the counterparty chain must implement the process of safely executing IBC transactions by verifying the authority of the transaction's signers.
- New interchain accounts must not conflict with existing ones
- Each chain must keep track of which counterparty chain created each new interchain account
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Each chain must keep track of which counterparty chain created each new interchain account
- Each origin chain and host chain must track which channels are used for which accounts.

- Fault containment: An interchain account must follow rules of its host chain, even in times of Byzantine behavior by the counterparty chain (the chain that manages the account)
- The chain that controls the account must process the results asynchronously and according to the chain's logic. The acknowledgment message should contain a result of an error as described in [ics-4](https://github.com/cosmos/ibc/tree/master/spec/core/ics-004-channel-and-packet-semantics#acknowledgement-envelope).
- Sending and receiving transactions will be processed in an ordered channel where packets are delivered exactly in the order in which they were sent.
- Each ordered channel must only allow one interchain account to use it, otherwise, malicious users may be able to block transactions from being received. Practically, N accounts per N channels can be achieved by creating a new port for each user (owner of the interchain account) and creating a unique channel for each new registration request. Future versions of ics-27 will use partially ordered channels to allow multiple interchain accounts on the same channel to preserve account sequence ordering without being reliant on one another
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are plenty of scenarios in which a module on a remote chain might be controlling multiple accounts on the destination chain, so sharing a channel adds no additional risk.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Dean, thanks for the feedback!

Regarding multiple untrusted users (account owners) using the same channel please take a look at this comment for reference: cosmos/interchain-accounts-demo#22 (comment).

The issue stems from multiple parties using the same ordered channel.


Each chain must satisfy following features to create a interchain account:
The interchain account specification defines the general way to register an interchain account and transfer tx bytes. The counterparty chain is responsible for deserialising and executing the tx bytes, and the sending chain should know how the counterparty chain will handle the tx bytes in advance. Each chain-specific implementation should clearly document how serialization/deserialization of transactions happens and ensure the required packet data format is clearly outlined.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's asymmetric, let's use terms like source and destination or some such. "counterparty" is too context sensitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback. I think you're right.


`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.
### Architecture Diagram
![](https://i.imgur.com/HX1h2u2.png)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the diagram just TryRegisterChainAccount followed by normal channel setup handshake?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diagram, in this case, is signaling that the registration of the account is occurring during the channel creation process, specifically in the handshake callbacks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the diagram


`runTx` executes a transaction after the transaction has been successfully authenticated.
The receiving chain must implement authentication with regard to ensuring that the incoming messages are sent by the owner of the targeted interchain account. With regards to authorization, it is up to each chain-specific implementation to decide if the hosted interchain accounts have the authority to invoke all of the chain's message types or only a subset. This configuration may be set up at module initialization.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's controlling access, then it's not authentication, it's authorization. Why does something have to be from the owner? Does that mean control of an account is not transferrable?

Copy link
Contributor Author

@seantking seantking Jun 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this design, the account can only be controlled by a single owner, correct.

Regarding authorization/authentication: I guess I was thinking of this as a validation step on the receiving side for ensuring the incoming IBC packet/instructions for the account are actually from the account owner. However, as Colin pointed out here this may not be necessary.

Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have some questions regarding account recovery

spec/app/ics-027-interchain-accounts/README.md Outdated Show resolved Hide resolved

### Definitions
### Definitions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good definitions 👍

### Definitions
### Definitions

- Interchain Account: An account on a host chain. An interchain account has all the capabilites of a normal account. However, rather than signing transactions with a private key, a controller chain will send IBC packets to the host chain which signal what transactions the interchain account should execute
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Interchain Account: An account on a host chain. An interchain account has all the capabilites of a normal account. However, rather than signing transactions with a private key, a controller chain will send IBC packets to the host chain which signal what transactions the interchain account should execute
- Interchain Account: An account on a host chain. An interchain account has all the capabilites of a normal account. However, rather than signing transactions with a private key, the account owner on the controller chain will send IBC packets to the host chain which signal what transactions the interchain account should execute

- The chain that controls the account must process the results asynchronously and according to the chain's logic. The result's code should be 0 if the transaction was successful and an error code other than 0 if the transaction failed.
- Sending and receiving transactions will be processed in an ordered channel where packets are delivered exactly in the order which they were sent.
- Permissionless
- Fault containment: An interchain account must follow rules of its host chain, even in times of Byzantine behavior by the controller chain (the chain that manages the account)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify what this means? What is the security model here between two chains. What is each chain trusted with?

From my understanding controller chain is trusted with authentication, host chain is trusted with correct execution of messages

- Sending and receiving transactions will be processed in an ordered channel where packets are delivered exactly in the order which they were sent.
- Permissionless
- Fault containment: An interchain account must follow rules of its host chain, even in times of Byzantine behavior by the controller chain (the chain that manages the account)
- Sending and receiving transactions will be processed in an ordered channel where packets are delivered exactly in the order in which they were sent
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a particular account or for all accounts between two chains?

@@ -201,14 +197,16 @@ function onChanOpenAck(
// port has already been validated
// assert that version is "ics27-1"
abortTransactionUnless(version === "ics27-1")
// state change to keep track of successfully registered interchain account
setActiveChannel(SourcePortId)
}
```

```typescript
function onChanOpenConfirm(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also need Close callbacks since that needs to "unset" active channel. Is the plan that we create a new channel against the same portID to "recover" the account?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes exactly

- The channel being created is ordered.
- The version string is "ics27-1".
- The channel being created is ordered
- The version string is "ics27-1"

```typescript
function onChanOpenInit(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be erroring here and on all handshake messages if activeChannel already set

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated this

}
```

```typescript
function onChanOpenConfirm(
portIdentifier: Identifier,
channelIdentifier: Identifier) {
// accept channel confirmations, port has already been validated
}
```

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just maintain a single active channel at a time. Multiple channels at a time means there's no global ordering of messages

Comment on lines 312 to 318
function onTimeoutPacket(packet: Packet) {
// Receiving chain should handle this event as if the tx in packet has failed
switch (ack.type) {
case Type.RUNTX:
onTxFailed(packet.sourcePort, packet.sourceChannel, packet.data.data)
return
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this no longer true?

Comment on lines -169 to -170
// only allow channels to "ibcaccount" port on counterparty chain
abortTransactionUnless(counterpartyPortIdentifier === "ibcaccount")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't these handshakes still need to check counterparty port based on:

The interchain account module on a host chain must always bind to a port with the id interchain_account. Controller chains will bind to ports dynamically, with each port id set as ics27-1-{owner-address}.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes updated.

@@ -16,114 +16,119 @@ This standard document specifies packet data structure, state machine handling l

### Motivation

On Ethereum, there are two types of accounts: externally owned accounts, controlled by private keys, and contract accounts, controlled by their contract code ([ref](https://github.com/ethereum/wiki/wiki/White-Paper)). Similar to Ethereum's CA (contract accounts), interchain accounts are managed by another chain while retaining all the capabilities of a normal account (i.e. stake, send, vote, etc). While an Ethereum CA's contract logic is performed within Ethereum's EVM, interchain accounts are managed by another chain via IBC in a way such that the owner of the account retains full control over how it behaves.
On Ethereum, there are two types of accounts: externally owned accounts, controlled by private keys, and contract accounts, controlled by their contract code ([ref](https://github.com/ethereum/wiki/wiki/White-Paper)). Similar to Ethereum's contract accounts, interchain accounts are controlled by another chain (not a private key) while retaining all the capabilities of a normal account (i.e. stake, send, vote, etc). While an Ethereum CA's contract logic is performed within Ethereum's EVM, interchain accounts are managed by a seperate chain via IBC in a way such that the owner of the account retains full control over how it behaves. ICS27-1 primarily targets the use cases of DAO investing and staking derivatives over IBC.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'separate'


`IBCAccountPacketData` specifies the type of packet. The `IBCAccountPacketData.data` is arbitrary and can be used for different purposes depending on the value of `IBCAccountPacketData.type`.
`runTx` executes a transaction based on the IBC packet recieved from the controller chain.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

received

Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks great. My only blocking concern is whether we're sending address back in the acknowledgment

spec/app/ics-027-interchain-accounts/README.md Outdated Show resolved Hide resolved
```typescript
message InterchainAccountPacketData {
repeated google.protobuf.Any messages = 1;
string memo = 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of keeping memo. It seems to be a key way for exchanges and wallets to track information. We're running into this issue on ics-20

Comment on lines 265 to 266
// Sets the interchainAccount address on the controller side for a given owner
setInterchainAccountAddress(sourcePort, result)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't make sense. The result is not the interchain address anymore from my understanding. Should this be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As colin suggests here: https://github.com/cosmos/ibc/pull/567/files#r669637554

We want to return the address of the interchain account on packet sequence 1. So in this case result would include the address.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spec/app/ics-027-interchain-accounts/README.md Outdated Show resolved Hide resolved
}
```

```typescript
function onChanCloseConfirm(
portIdentifier: Identifier,
channelIdentifier: Identifier) {
// no action necessary
unsetActiveChannel(SourcePort)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

spec/app/ics-027-interchain-accounts/README.md Outdated Show resolved Hide resolved
spec/app/ics-027-interchain-accounts/README.md Outdated Show resolved Hide resolved
spec/app/ics-027-interchain-accounts/README.md Outdated Show resolved Hide resolved
spec/app/ics-027-interchain-accounts/README.md Outdated Show resolved Hide resolved
spec/app/ics-027-interchain-accounts/README.md Outdated Show resolved Hide resolved
spec/app/ics-027-interchain-accounts/README.md Outdated Show resolved Hide resolved
Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!! Splitting out my final comment into an issue to be tackled in separate PR

Comment on lines 249 to 251
const interchainAccount = getInterchainAccount(ctx, SourcePortId)
return Acknowledgement{
result: result
result: interchainAccount
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acking since this is a quick change. But this should not just contain the interchain account. Otherwise we don't properly get the result of our first message execution.

We should get the interchain account in addition to first message result on ack sequence 1. The way to marshal both in ack, and then unmarshal should be specified in this document.

Will create a separate issue for this to be tackled in subsequent PR

@colin-axner
Copy link
Contributor

@milosevic and @cwgoes whenever time arises to review this pr again, I think it'd be useful to reserve any minor fixes for a separate issue/followup pr and significant issues for requested changes. This pr is a major improvement to the existing spec, but is also overflowed with comments. I think it'd be a lot easier to review any remaining changes in separate prs

@seantking
Copy link
Contributor Author

@milosevic and @cwgoes whenever time arises to review this pr again, I think it'd be useful to reserve any minor fixes for a separate issue/followup pr and significant issues for requested changes. This pr is a major improvement to the existing spec, but is also overflowed with comments. I think it'd be a lot easier to review any remaining changes in separate prs

I pushed a bunch more changes to this, mostly just making everything clearer and using a similar layout to the fee spec which had a nice section for the interface.

@milosevic If you have time please take a look, it would be nice to get this merged in as 100+ comments make this PR extremely challenging to review. I think future PRs can tackle the minor updates that are necessary. It is evolving as the implementation evolves.

@milosevic milosevic merged commit 2756584 into cosmos:master Aug 6, 2021
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 this pull request may close these issues.

9 participants