-
Notifications
You must be signed in to change notification settings - Fork 15
Update ICS-27 Specification #22
Comments
These should probably be separate issues imo. But here are my suggestions. Channel OrderKeep it ORDERED, but remove user ability to control timeout height, and instead use a large default when sending packets. Address CreationSend just the salt (ideally the address of the owner) in Authorization Improvementtype 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. |
@AdityaSripal re: I'm not sure what the best practice here should be. Maybe you can share some thoughts? |
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.
This gives no info on what data format should be sent.
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. |
I would propose to define 1 format just for the SDK and and maybe rename it 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 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 |
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:
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. |
TL;DR my 3 big objections to this spec as it stands:
|
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 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? |
I like this approach. |
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. |
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. |
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. |
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) |
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 |
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 |
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 |
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? |
Correct.
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? |
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? |
If channels are lightweight is there any disadvantage to having N account for N channels? The benefits seem to be:
|
No, all packets have ( |
Aha. Yeah, I see now. Something like this -> https://github.com/cosmos/interchain-accounts/blob/master/x/ibc-account/keeper/handshake.go#L26 |
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. |
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 |
This should all ideally be documented. I just know it from integrating cosmwasm and working on ts-relayer. |
Nicely explained thank you. That makes a lot of sense to me. |
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) |
@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:
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? |
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) |
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 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)
That would be ideal, but I guess that is 6+ months out. |
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.
Hmm, sorry but I'm not following. Would you mind unpacking that further? How does using an un-ordered channel help us here?
👍 |
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. |
Also, thinking of the acknowledgement format. The spec is quite old and there has been some Although the ICS spec leave the actual acknowledgement as opaque bytes, it does 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 I would propose that ICS27 use that same format as a success/error envelope (the success bytes are protocol dependent). |
I'm wondering if there are any other security concerns to consider if we allow modules with port id's other than |
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 @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 |
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. |
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.
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.
Correct. I put the
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:
|
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. |
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 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 |
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. |
First off, thank you for your comments @AdityaSripal 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). |
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 |
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. |
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 🤝 |
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 |
Can we transfer this issue to |
👍 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. |
|
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
The text was updated successfully, but these errors were encountered: