cosmos / interchain-accounts-demo

ICA
83 stars 14 forks source link

Update ICS-27 Specification #22

Closed seantking closed 2 years ago

seantking commented 3 years ago

Let's discuss some potential updates to the ics-27 spec here.

Initial Topics:

Authentication Improvement

@AdityaSripal & @colin-axner would you care to share some of your ideas on potential authentication/authorization improvements?

Removal of Registration Message

See cosmwasm reflect contract spec -> https://github.com/CosmWasm/cosmwasm/tree/main/contracts/ibc-reflect

AdityaSripal commented 3 years ago

These should probably be separate issues imo. But here are my suggestions.

Channel Order

Keep it ORDERED, but remove user ability to control timeout height, and instead use a large default when sending packets.

Address Creation

Send just the salt (ideally the address of the owner) in RegisterAddressPacket, the receiving module will then return an address in its own format as a string in the ACK. This can then be stored in the keeper just as it currently is.

Authorization Improvement

type IBCRunTxPacket {
     MsgBytes []byte
     Authorizers []string // this could just be a single string (single address) for now
}

The RunTx packet data should deserialize to something like this, where the msg bytes are serialized msgs for the receiving module's state machine to process. And the authorizers are the list of interchain accounts that have authorized the msg bytes.

The sending module is responsible for ensuring that the interchain accounts have all authorized the submitted msg bytes, without trying to deserialize the msg bytes. This may be done on the sending module with signatures, governance proposals, group accounts, whatever.

The receiving module receives the msg bytes and the provided authorizers. It deserializes the msg bytes and uses its own state machine logic to retrieve the list of expected authorizers for the given msgs. It then checks if the expected authorizers match the given authorizers. If it does, then it executes the message.

This provides a clear separation of responsibility. The sending module is not responsible for knowing any of the state machine logic of the receiving chain. It only understands how to authorize the interchain accounts that it controls.

Similarly, the receiving chain does not need to understand how the interchain accounts controlled by the sending module were authorized. It only needs to use its state machine rules to determine if the provided authorization is sufficient to execute the message.

seantking commented 3 years ago

@AdityaSripal re: Channel Order I am updating the timeout as part of this MR -> https://github.com/cosmos/interchain-account/pull/27

I'm not sure what the best practice here should be. Maybe you can share some thoughts?

ethanfrey commented 3 years ago

First of all, the spec is very unclear at points and deviates quite a bit from the implementation. (Or maybe the implementation is just one particular interpretion of a very vague spec). I think it is essential that the spec contains enough info that someone can build a compatible module just using that.

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.

This gives no info on what data format should be sent.

When the value of type is REGISTER, IBCAccountPacketData.data can be used as information by which the account is created on the host chain. When the value of type is RUNTX, IBCAccountPacketData.data contains the tx bytes.

Is also vague. Is this a protobuf encoded sdk.Tx? Including properly SignInfo and signature? That would be the first interpretation of this, but that is also impossible to create (signing with a private key) from an-chain module. This needs to be specified.

ethanfrey commented 3 years ago

I would propose to define 1 format just for the SDK and and maybe rename it runSdkTx. Then all the ideas of supporting other tx formats can be left as a thought for the future, but we cleanly cover all the use cases in the near future.

Once we define this as an SDK type, we need to define is it proto or JSON encoded? (ICS20 encodes via JSON, not sure if proto is better for dealing with sdk.Msg and Any interfaces?) Secondly, what is encoded. One sdk.Msg? Many? A whole Tx? I think a good place to start is TxBody in ADR 020.

My proposal would be to just include the first two fields of this, like:

message RunTxPacket  {
    repeated google.protobuf.Any messages = 1;
    string memo = 2;
}

This can be serialized (JSON or proto - you can decide, just specify) into the Data field for the RUN_SDK_TX type.

ethanfrey commented 3 years ago

Re: channel ordering. Running multiple, independent accounts on one ordered channel is a bad idea.

When you have N accounts multiplexed on an ordered channel, it makes relaying one's own packet considerably more difficult (when this happens in absence of some altruistic relayer that will pay everyone's fees). We must design for the conditions that people are somehow responsible for paying their own packets (or relayers are selectively sponsoring them)

N accounts on N ordered channels makes sense for each account (which is why I went that way, and also more or less what is in the spec). I can pay my packet without having to pay 100 packets from other people.

N accounts on 1 unordered channel could work, as long as the accounts were careful to wait for confirmation before sending a second message that depended on the first. This would be (is?) less of an issue if RunTx takes []sdk.Msg not just one (so each packet does ordering or multiple actions)

Everything in the ICS spec suggests that Clients are heavy/expensive structs (and thus can be reused for multiple channels), but Channels are light weight (a TCP connection vs an Ethernet card). This was confirmed by @AdityaSripal in discord:

Channels are lightweight. I guess it's really a question of usability. N accounts on N ordered channels makes a lot of sense when the owner of ICA is responsible for relaying the packet. N accounts on 1 channel makes sense if there's going to be a relayer that just wants to relay ICA packets across 2 chains I was imagining the second scenario, although the first may end up being the case

Note that modern relayers (hermes and ts-relayer and maybe an updated go relayer) can be configured to relay all channels on on connection quite easily.

ethanfrey commented 3 years ago

TL;DR my 3 big objections to this spec as it stands:

  1. It does not define packet data format beyond bytes and some vague words of "data to be processed on the host chain". We need to make 1 very clear spec of data format used for the SDK. Adding other chains is for a future spec. But a spec must define enough to allow multiple compatible implementations.
  2. One ordered channel for multiple accounts is a bad idea. We cannot assume infinitely altruistic relayers forever. There are two alternative approaches which can be selected from.
  3. 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. which need 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.
okwme commented 3 years ago

Great input from @AdityaSripal and @ethanfrey thanks for taking the time and considering the issues!

In general I'm in favor of an approach that allows for a low risk MVP version to get on the hub ASAP. A lot of the points made above will be better informed by actual use of Interchain Accounts. Without those users I've been prioritizing the following user story:

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.

With regard to the multiple / single, ordered / unordered channels question this makes me think that realistically we should only expect to see one channel used for the distribution module account. The ICA module is broken into two pieces: the core module which processes incoming ICA transactions and the helper module which handles logic for creating outgoing ICA transactions and maintains a registry of accounts. My preference would be to keep the core module as minimal and flexible as possible and let the helper module be the focus of iterative designs and best practices.

If that were the case then the core should be able to accept many accounts on a single ordered channel. It should accept the array of authorizers and these should match those provided during tryRegisterIBCAccount. The work that @seantking is doing right now is to modify the helper account so that it is just used for the distribution module account user story. Since there is only one account in question it will in fact follow @ethanfrey 's suggestion of one account per channel port. A subsequent helper module meant for more general use can follow suit but we don't need to block progress on the creation of that module.


Ethan's other suggestions about vague wording in the spec seem great as well. My understanding was that @AdityaSripal was working on a PR for concrete changes to the spec, is that still the case? If so, what do you think about including those suggestions there?

seantking commented 3 years ago

I would propose to define 1 format just for the SDK and and maybe rename it runSdkTx. Then all the ideas of supporting other tx formats can be left as a thought for the future, but we cleanly cover all the use cases in the near future.

Once we define this as an SDK type, we need to define is it proto or JSON encoded? (ICS20 encodes via JSON, not sure if proto is better for dealing with sdk.Msg and Any interfaces?) Secondly, what is encoded. One sdk.Msg? Many? A whole Tx? I think a good place to start is TxBody in ADR 020.

My proposal would be to just include the first two fields of this, like:

message RunTxPacket  {
    repeated google.protobuf.Any messages = 1;
    string memo = 2;
}

This can be serialized (JSON or proto - you can decide, just specify) into the Data field for the RUN_SDK_TX type.

I like this approach.

ethanfrey commented 3 years ago

My preference would be to keep the core module as minimal and flexible as possible and let the helper module be the focus of iterative designs and best practices.

If that were the case then the core should be able to accept many accounts on a single ordered channel.

The current design allows (but does not enforce) the one channel per account approach. It would be a minimal code chain to enforce it (and actually make the core module simpler in the process). Multiplexing them on one channel just seems to cause issues in the future and I see no reason to continue that approach if it is less than 1 day to modify that.

seantking commented 3 years ago

First of all, the spec is very unclear at points and deviates quite a bit from the implementation. (Or maybe the implementation is just one particular interpretion of a very vague spec). I think it is essential that the spec contains enough info that someone can build a compatible module just using that.

I think it is a worthwhile endeavor to make the spec more clear at a minimum so that it can inform other implementations better. I think in this instance the implementation is one particular interpretation of a vague spec as you put it.

ethanfrey commented 3 years ago

If that were the case then the core should be able to accept many accounts on a single ordered channel.

The problem is this gets very confusing if you look at the details. I assume that this means 3 different "users" on the remote chain (community pool, a group "multisig" and a pubkey account) could all control independent accounts on the host chain. But, for some reason, they all multiplex on one channel. There is no reason for that and actually complicates matters a channels handle multiplexing and security isolation for you.

If you want one sender on the remote side (eg. community pool) to control multiple related accounts on the host side over a single channel, then I might understand the "one channel, many accounts" approach for this use-case. As far as I understand, this is not what is being discussed. But I also didn't see the planned use cases clearly specified anywhere. Doing so, may remove much discussion as we have a common vision of what we are building.

ethanfrey commented 3 years ago

My personal interest is 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)

seantking commented 3 years ago
  1. 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. which need 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.

I think there might be some confusion here (pls tell me if I'm wrong). But, as it stands only the core ibc-account module sends ibc packets to other ibc-account modules. The helper modules consume the ibc-account keeper and call the keeper functions directly e.g -> https://github.com/cosmos/interchain-accounts/blob/master/x/inter-tx/keeper/account.go#L17

For example, registering an account here: https://github.com/cosmos/interchain-accounts/blob/master/x/ibc-account/keeper/relay.go#L38

and sending a RunTx type here: https://github.com/cosmos/interchain-accounts/blob/master/x/ibc-account/keeper/relay.go#L124

However, I guess in theory you could create a new module that registers the ibcaccount port and then interacts with an ibc-account module on another chain. Is that what you mean?

seantking commented 3 years ago

N accounts on 1 unordered channel could work, as long as the accounts were careful to wait for confirmation before sending a second message that depended on the first. This would be (is?) less of an issue if RunTx takes []sdk.Msg not just one (so each packet does ordering or multiple actions)

With the current implementation, this is achieved by storing the registered account in a temporary queue that waits for a successful acknowledgment from the destination chain before popping the address off of the queue and storing it in a registry.

You can see the queue defined here -> https://github.com/cosmos/interchain-accounts/blob/master/x/inter-tx/keeper/register_queue.go

Using the queue here -> https://github.com/cosmos/interchain-accounts/blob/master/x/inter-tx/keeper/account.go#L22

The final callback here -> https://github.com/cosmos/interchain-accounts/blob/master/x/inter-tx/keeper/callback.go

and wiring the callback up here -> https://github.com/cosmos/interchain-accounts/blob/master/app/app.go#L624

Note this is all done on the helper module side of things, which in my opinion does complicate things in terms of implementation as there are no guidelines for achieving this in the ics-27 spec. We are relying on the following hooks defined in the core module to achieve this -> https://github.com/cosmos/interchain-accounts/blob/master/x/ibc-account/types/hook.go

ethanfrey commented 3 years ago

However, I guess in theory you could create a new module that registers the ibcaccount port and then interacts with an ibc-account module on another chain. Is that what you mean?

I want another module that binds to another port and interacts with the ibc-account module.

At one point (probably when this spec was original written) ics20 enforced the remote port to transfer as well. This was relaxed, as only the version was needed to enforce proper communication. CosmWasm contracts will register some port like wasm.cosmos12f82hfr39fh2fo32r1vn32vt3ef4t5h4542t and it seems arbitrary to block them from communcating (we demo'd CosmWasm contracts talking directly to transfer port, allowing us to send cw20 tokens to another chain)

seantking commented 3 years ago

However, I guess in theory you could create a new module that registers the ibcaccount port and then interacts with an ibc-account module on another chain. Is that what you mean?

I want another module that binds to another port and interacts with the ibc-account module.

At one point (probably when this spec was original written) ics20 enforced the remote port to transfer as well. This was relaxed, as only the version was needed to enforce proper communication. CosmWasm contracts will register some port like wasm.cosmos12f82hfr39fh2fo32r1vn32vt3ef4t5h4542t and it seems arbitrary to block them from communcating (we demo'd CosmWasm contracts talking directly to transfer port, allowing us to send cw20 tokens to another chain)

Interesting. I was under the impression that this was enforced at the IBC level and that the sending module would need to bind to the same port as the receiving module. Do you know where I can read the specifics of this in the documentation somewhere?

@colin-axner @AdityaSripal maybe you can shine some light here?

seantking commented 3 years ago

The problem is this gets very confusing if you look at the details. I assume that this means 3 different "users" on the remote chain (community pool, a group "multisig" and a pubkey account) could all control independent accounts on the host chain.

Correct.

But I also didn't see the planned use cases clearly specified anywhere. Doing so may remove much discussion as we have a common vision of what we are building.

Good point. The primary user story we have been using as a guiding light is registering an interchain account on behalf of a community pool (distribution module) and controlling said account via governance proposals.

@hxrts I know you had some ideas for use with the regen groups module?

seantking commented 3 years ago

When you have N accounts multiplexed on an ordered channel, it makes relaying one's own packet considerably more difficult (when this happens in absence of some altruistic relayer that will pay everyone's fees). We must design for the conditions that people are somehow responsible for paying their own packets (or relayers are selectively sponsoring them)

Maybe a dumb question but are you saying here that if we have N accounts multiplexed on a single ordered channel it's going to be more difficult for future relayers (that selectively filter packets or charge for services) to relay said packets efficiently? Can you explain to me why that is the case?

seantking commented 3 years ago

If channels are lightweight is there any disadvantage to having N account for N channels? The benefits seem to be:

  1. Less code (we can potentially remove some of the temporary registration queue logic posted above?)
  2. Potentially improve UX (We could remove the Register Message entirely similar to the reflect contract)
  3. Future-proofing the spec to work with less naive implementations of relayers (where users pay for relay services or relayers selectively filter packets). I'm still not sure why this is the case if someone can enlighten me @AdityaSripal @ethanfrey
ethanfrey commented 3 years ago

Interesting. I was under the impression that this was enforced at the IBC level and that the sending module would need to bind to the same port as the receiving module.

No, all packets have (src_port, src_channel) and (dest_port, dest_channel) pairs. If you want to enforce src_port == dest_port you must do that explicitly in the channel handshake logic.

seantking commented 3 years ago

Interesting. I was under the impression that this was enforced at the IBC level and that the sending module would need to bind to the same port as the receiving module.

No, all packets have (src_port, src_channel) and (dest_port, dest_channel) pairs. If you want to enforce src_port == dest_port you must do that explicitly in the channel handshake logic.

Aha. Yeah, I see now. Something like this -> https://github.com/cosmos/interchain-accounts/blob/master/x/ibc-account/keeper/handshake.go#L26

ethanfrey commented 3 years ago

Maybe a dumb question but are you saying here that if we have N accounts multiplexed on a single ordered channel it's going to be more difficult for future relayers (that selectively filter packets or charge for services) to relay said packets efficiently? Can you explain to me why that is the case?

In an unordered channels, 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 a 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 week?) time outs. 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 principle advances in the ICS spec since I wrote one in late 2017 and we really should make use of this.

ethanfrey commented 3 years ago

Interesting. I was under the impression that this was enforced at the IBC level and that the sending module would need to bind to the same port as the receiving module.

No, all packets have (src_port, src_channel) and (dest_port, dest_channel) pairs. If you want to enforce src_port == dest_port you must do that explicitly in the channel handshake logic.

Aha. Yeah, I see now. Something like this -> https://github.com/cosmos/interchain-accounts/blob/master/x/ibc-account/keeper/handshake.go#L26

Actually that ensures that the local port matches the port we have bound, which is a paranoid check, as that is already enforced by the ibc module before OnChanOpenInit is called. (Maybe it wasn't enforced last summer when the MVP code was written). It would be enforced by something like if counterparty.PortID != portID (in OnChanOpenTry and OnChanOpenAck - which are the two that have properly filled in remote port IDs)

ethanfrey commented 3 years ago

This should all ideally be documented. I just know it from integrating cosmwasm and working on ts-relayer.

seantking commented 3 years ago

Maybe a dumb question but are you saying here that if we have N accounts multiplexed on a single ordered channel it's going to be more difficult for future relayers (that selectively filter packets or charge for services) to relay said packets efficiently? Can you explain to me why that is the case?

In an unordered channels, 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 a 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 week?) time outs. 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 principle advances in the ICS spec since I wrote one in late 2017 and we really should make use of this.

Nicely explained thank you. That makes a lot of sense to me.

seantking commented 3 years ago

I'm going to start investigating what changes need to be made to the codebase in order to update the data packet structure and switch to a single ordered channel per account.

ethanfrey commented 3 years ago

I'm going to start investigating what changes need to be made to the codebase in order to update the data packet structure and switch to a single ordered channel per account.

Nice. For a simple first step, you can just remove/ignore the data in Register packets and the salt, and just ensure there is no acount for this (remote port, remote channel) pair before creating it.

Auto-creating register during the OnChanInit callbacks is much more complicated and maybe undesired. (You need another query to get the account address anyway)

seantking commented 3 years ago

@ethanfrey how would you recommend creating a new channel for each interchain account? I spoke to @colin-axner he mentioned this would have to be some kind of async process with some kind of mechanism like:

  1. user wants to register an account, add to queue, emit some kind of event that a client can pick up on
  2. Client creates a new channel
  3. when a new ibc account channel is opened, pop user from queue and assign that channel to that user

This seems like it could be a major negative in using N account per N channel as it's adding overhead for channel creation and complexity within the channel handshake callbacks. Perhaps I am missing something?

colin-axner commented 3 years ago

Generating a new port ID for each user seems like the best approach for the one user per channel approach.

Long term, ICA should be using partially ordered channel (ordered based on sequence for each account)

ethanfrey commented 3 years ago

Generating a new port ID for each user seems like the best approach for the one user per channel approach.

This is how we did it for the cosmwasm contract. You need to bind a new port locally, then a relayer creates a channel. We know who controls the channel as the port (contract in our case) is tied to a user.

This seems like it could be a major negative in using N account per N channel as it's adding overhead for channel creation and complexity within the channel handshake callbacks. Perhaps I am missing something?

This may be more overhead than desired - create a port, then create a channel to create a new account (which requires an off-chain process). What about simply using an un-ordered channel then? (And you can keep the salt design)

Long term, ICA should be using partially ordered channel (ordered based on sequence for each account)

That would be ideal, but I guess that is 6+ months out. This is actually a great real-world use case for partially ordered channels. I feel it positive to have concrete uses when discussing theory so they actually solve the original problem well.

seantking commented 3 years ago

This seems like it could be a major negative in using N account per N channel as it's adding overhead for channel creation and complexity within the channel handshake callbacks. Perhaps I am missing something?

This may be more overhead than desired - create a port, then create a channel to create a new account (which requires an off-chain process).

Am I correct in assuming the off-chain process here would be listening for some kind of emitted event and then call the relayer client to create a new channel for a specific port-id? I'm guessing this is when you decided to register the account on the on-channel-created callback, to save the user having to make another call to register an account.

I feel like this flow makes sense for a smart contract approach but I'm not so sure if we're working with the ica module.

What about simply using an un-ordered channel then? (And you can keep the salt design)

Hmm, sorry but I'm not following. Would you mind unpacking that further? How does using an un-ordered channel help us here?

Long term, ICA should be using partially ordered channel (ordered based on sequence for each account)

That would be ideal, but I guess that is 6+ months out. This is actually a great real-world use case for partially ordered channels. I feel it positive to have concrete uses when discussing theory so they actually solve the original problem well.

:+1:

ethanfrey commented 3 years ago

An unordered channel lets packets be relayed in any order. I can send 100 packets and never relay them. You submit one packet and can immediately relay just that one without having to relay all of mine. If there are dependencies between messages, you can pack multiple messages into one packet (in the format proposed above). I agree partial ordering is better, but I think unordered would actually solve this problem without hurting the UX too much.

ethanfrey commented 3 years ago

Also, thinking of the acknowledgement format. The spec is quite old and there has been some work since then.

Although the ICS spec leave the actual acknowledgement 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;
  }
}

Although it suggests this is a Protobuf object, the ICS spec doesn't define whether to encode it as JSON or Protobuf. In the ICS20 implementation, this is JSON encoded when returned from a contract.

I would propose that ICS27 use that same format as a success/error envelope (the success bytes are protocol dependent). You can choose if JSON or Protobuf makes more sense for this use-case, but I think keeping things standardized is a great boon.

seantking commented 3 years ago
  1. 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. which need 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.

I'm wondering if there are any other security concerns to consider if we allow modules with port id's other than ibcaccount to send ibc packets to the interchain account module. I've been working under the assumption that the best practice was to have the ica module speak directly to another ica module.

seantking commented 3 years ago

Authorization Improvement

type IBCRunTxPacket {
     MsgBytes []byte
     Authorizers []string // this could just be a single string (single address) for now
}

The RunTx packet data should deserialize to something like this, where the msg bytes are serialized msgs for the receiving module's state machine to process. And the authorizers are the list of interchain accounts that have authorized the msg bytes.

The sending module is responsible for ensuring that the interchain accounts have all authorized the submitted msg bytes, without trying to deserialize the msg bytes. This may be done on the sending module with signatures, governance proposals, group accounts, whatever.

The receiving module receives the msg bytes and the provided authorizers. It deserializes the msg bytes and uses its own state machine logic to retrieve the list of expected authorizers for the given msgs. It then checks if the expected authorizers match the given authorizers. If it does, then it executes the message.

This provides a clear separation of responsibility. The sending module is not responsible for knowing any of the state machine logic of the receiving chain. It only understands how to authorize the interchain accounts that it controls.

Similarly, the receiving chain does not need to understand how the interchain accounts controlled by the sending module were authorized. It only needs to use its state machine rules to determine if the provided authorization is sufficient to execute the message.

Thanks for the suggestion. I think this is a good approach also (if I understand correctly). Am I correct in saying the primary benefit here is that we can be more explicit about what the shape of our packet data looks like (rather than ambiguous bytes) and also provide future-proofing for different message formats?

In the short term, I'm leaning towards the approach that @ethanfrey has suggested with having version 1 of the interchain accounts spec be cosmos sdk specific with runSdkTx.

@ethanfrey did you see the current implementation for handling different message formats? It's documented here -> https://github.com/cosmos/interchain-accounts/blob/master/x/ibc-account/spec/04_keeper.md

and used like so -> https://github.com/cosmos/interchain-accounts/blob/master/x/ibc-account/keeper/keeper.go#L18 & https://github.com/cosmos/interchain-accounts/blob/master/app/app.go#L329

seantking commented 3 years ago
```protobuf
message Acknowledgement {
  // response contains either a result or an error and must be non-empty
  oneof response {
    bytes  result = 21;
    string error  = 22;
  }
}

This is a good idea. Are you thinking that on successful registration of an account we send the account address of the creator and the new interchain account address as result? We can then mark the account as registered on the origin chain.

AdityaSripal commented 3 years ago

I'm wondering if there are any other security concerns to consider if we allow modules with port id's other than ibcaccount to send ibc packets to the interchain account module. I've been working under the assumption that the best practice was to have the ica module speak directly to another ica module.

This can be enforced by checking the channel version. Rather than assuming that all ica modules use the same unique port, assume all ica modules use the same channel version string.

An unordered channel lets packets be relayed in any order. I can send 100 packets and never relay them. You submit one packet and can immediately relay just that one without having to relay all of mine. If there are dependencies between messages, you can pack multiple messages into one packet (in the format proposed above). I agree partial ordering is better, but I think unordered would actually solve this problem without hurting the UX too much.

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.

Thanks for the suggestion. I think this is a good approach also (if I understand correctly). Am I correct in saying the primary benefit here is that we can be more explicit about what the shape of our packet data looks like (rather than ambiguous bytes) and also provide future-proofing for different message formats?

Correct. I put the bytes there just to make the general point that sdk.Msg is too specific to keep in an interchain protocol. I believe []Any is sufficiently future-proof and maybe more developer-friendly. Please confirm @colin-axner

RE: UNORDERED channel

I don't understand the discussion around possibly using UNORDERED channels. Regardless of whether each user creates a new portID, creating an UNORDERED channel will require that the sequence ordering for each account gets enforced by the application. This is unnecessarily complex, when we can just use the IBC ORDERED channel to enforce sequence ordering for us. I would still say we use 1 ORDERED channel per account.

My thinking is that the flow could look like this:

seantking commented 3 years ago

My thinking is that the flow could look like this:

  • User submits something like a CreateICAMsg
  • The handler for this message will call OpenChanInit which will initiate the handshake process
  • 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
  • At the end of handshake process, the "register" step is complete and an address is created. This is not too complicated. 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.
  • User can immediately start sending messages once channel handshake is complete

This flow makes a lot of sense after realizing that client ux will likely have some kind of integrated relayer. In this case I think 1 Account per ordered channel is the right approach.

AdityaSripal commented 3 years ago

If there are dependencies between messages, you can pack multiple messages into one packet (in the format proposed above)

This is true, and can allow for us to use UNORDERED channels. But this means that independent messages sent by the same user can be processed in any order. I'm a bit hesitant to do this mainly because it breaks the semantics of sending messages from a BaseAccount to the chain as one normally would.

I would prefer to keep the process by which one sends messages to a chain over tendermint mempool to be semantically identical to the way one would send it over IBC. Any difference in how the messages are processed will have to be very well-documented, or people will get it wrong because they assumed tx processing works the same way as with a regular account

seantking commented 3 years ago

Correct. I put the bytes there just to make the general point that sdk.Msg is too specific to keep in an interchain protocol. I believe []Any is sufficiently future-proof and maybe more developer-friendly. Please confirm @colin-axner

If that is all we need to do in order to keep it future proof then great. I do see Ethans point for wanting to make the protocol more specific in some instances. For example, specifying if the data field is JSON or proto encoded. I think we should say the current standard is to keep it proto ecoded.

ethanfrey commented 3 years ago

First off, thank you for your comments @AdityaSripal Making an explicit assumption (requirement) of some sort of integrated relayer for nice UX is very important. And it would be good to ensure that this design works with the existing relayers (is enough info in the OnChannelInit message? we assume both version == counterparty_version, which is quite safe I assume)

I also prefer N channels * 1 account/channel but the comment about registration UX was a point I could not solve, so suggested UNORDERED as a fallback (so no one could fill it with spam).

ethanfrey commented 3 years ago

Correct. I put the bytes there just to make the general point that sdk.Msg is too specific to keep in an interchain protocol. I believe []Any is sufficiently future-proof and maybe more developer-friendly. Please confirm @colin-axner

If that is all we need to do in order to keep it future proof then great. I do see Ethans point for wanting to make the protocol more specific in some instances. For example, specifying if the data field is JSON or proto encoded. I think we should say the current standard is to keep it proto encoded.

There is this ideal of keeping "ICS specs' so generic they work on any chain. Which means that you cannot actually implement the specs without some other chain-specific details. These chain-specific details never make it into specs, so I (and many others) are left to pour through source code to build a compatible version.

This is not what specs are supposed to be. You should be able to hand me some thorough specs of an API and I can build and test a program against that spec, and then later I plug it into a real system and they should integrate well (minus rough edges with business logic and "what is a valid identifier" type things).

If the place for these implementation details beyond Any or bytes is not in ICS, where is it? Would it make sense to have a generic ICS27 spec (with the flow and theory) and a CosmosSDK-ICS27 spec to specify a particular packet format that could be sent for RunTx for Cosmos chains? I am happy to have them split, or even in another repo. But that level of details needs to be in a markdown file somewhere and not just in Go.

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

This is a good idea. Are you thinking that on successful registration of an account we send the account address of the creator and the new interchain account address as result? We can then mark the account as registered on the origin chain.

Yeah, exactly. We just use this envelope for success/error. And the result bytes are pretty much whatever is spec'd now (the new addresses in some TBD format as return value from RegisterTx, some TDB data as return from RunTx). It would be good to specify those bytes as well, just saying they can be serialized and wrapped in this.

seantking commented 3 years ago

There is this ideal of keeping "ICS specs' so generic they work on any chain. Which means that you cannot actually implement the specs without some other chain-specific details. These chain-specific details never make it into specs, so I (and many others) are left to pour through source code to build a compatible version.

This is not what specs are supposed to be. You should be able to hand me some thorough specs of an API and I can build and test a program against that spec, and then later I plug it into a real system and they should integrate well (minus rough edges with business logic and "what is a valid identifier" type things).

If the place for these implementation details beyond Any or bytes is not in ICS, where is it? Would it make sense to have a generic ICS27 spec (with the flow and theory) and a CosmosSDK-ICS27 spec to specify a particular packet format that could be sent for RunTx for Cosmos chains? I am happy to have them split, or even in another repo. But that level of details needs to be in a markdown file somewhere and not just in Go.

I can see the value in differentiating between a generic protocol and a specific implementation. There is a crossover but also some meaningful differences. I think for now this can be noted in an update to the spec. It's quite a minimal update. I'm working on a branch to update the document at the moment. I'll make a draft PR tomorrow probably and we can start commenting there.

cheers :handshake:

colin-axner commented 3 years ago

Correct. I put the bytes there just to make the general point that sdk.Msg is too specific to keep in an interchain protocol. I believe []Any is sufficiently future-proof and maybe more developer-friendly. Please confirm @colin-axner

[]Any sounds fine to me

Would it make sense to have a generic ICS27 spec (with the flow and theory) and a CosmosSDK-ICS27 spec to specify a particular packet format that could be sent for RunTx for Cosmos chains? I am happy to have them split, or even in another repo. But that level of details needs to be in a markdown file somewhere and not just in Go.

Having a generic ICS27 and a SDK-ICS27 spec makes the most sense to me. It is definitely problematic to have chain specific requirements which are unspecified but are required for cross chain compatibility. Events are another great example of this problem

If things need to be standardized or specified, it should go in ICS imo

colin-axner commented 3 years ago

Can we transfer this issue to cosmos/ibc?

ethanfrey commented 3 years ago

Having a generic ICS27 and a SDK-ICS27 spec makes the most sense to me. It is definitely problematic to have chain specific requirements which are unspecified but are required for cross chain compatibility. Events are another great example of this problem

If things need to be standardized or specified, it should go in ICS imo

:+1: Sounds like we are on the same page. Happy for any organization/split of specs, but all this info needs to be there somewhere. ICS repo has felt a bit too much "building in the clouds" and alergic to implementation details (or use cases to explain the why of the spec). Happy if there is motion to add such details somehow.

seantking commented 3 years ago

Can we transfer this issue to cosmos/ibc?

@ethanfrey @colin-axner @okwme

https://github.com/cosmos/ibc/issues/564