ICS27: Controller PortID Format should not contain version

[AdityaSripal]

title says it all

If we keep version in, then it becomes hard to upgrade the channel without changing the portID, which is undesired. There also isn't a compelling reason afaik to keep the version in the portID

cc: @colin-axner @seantking

[colin-axner]

It'd be great if the version could be a proto definition which is JSON marshalled, as @damiannolan suggests

[ethanfrey]

What are you trying to do with versions???

[colin-axner]

What are you trying to do with versions???

The ICS27 portID currently contains the connection sequence and the counterparty connection sequence. This was done for channel recovery in the case of channel closure (light client attack or packet timeout), as recently discussed, ordered channels could be implemented without closing the channel on packet timeout. This may remove the requirement of the connection sequences in the port ID, but changing this after the fact likely isn't possible

Since we need the connection sequences associated with the channel in ICS27 v1, we suggest moving the sequences to the ICS27 version, which will be upgradeable with channel upgradeability (see current proposed spec for connection)

If we are moving the connection sequences to the channel version, we think it'd make sense to use a typed defintion for the version instead of continuing to overload it with string parsing. The suggestion would be to have a proto definition which contains the: version, connection-id, counterparty connection-id, and interchain account address (existing version already contains version and interchain account address). Then using the JSON marshalled bytes of this version as the version bytes set in the channel structure.

Do you have concerns with this proposal?

[seantking]

What are you trying to do with versions???

The main reason we're putting metadata into the version string is so that we can register an interchain account + create the channel as one single step, rather than two. We create the interchain account on the channel creation handshake, and using the version string more like a "metadata" field allows us to pass the account address of the ICA back to the controller chain.

[ethanfrey]

Okay, I see why you use this from a UX perspective.

I have some strong reservations about you (mis-)using version as metadata, especially in the 2nd official spec coming from IG/ICF. This will encourage all other protocol designers to (mis-)use this field in the same way. Wouldn't it be better to simply add an optional metadata field in ibcv3 in some backwards compatible way, as ICA will require ibcv3 anyway?

Even without adding this metadata field, I think the same UX can be acheived with a much less invasive manner. Basically, after the handshake is confirmed (OnChannelAck) the "server" chain (the one creating the account) can send a message over the channel to the "client" chain (the counterparty that will call these accounts) registering the account address for the channel. This will only be called once and provide the address.

There may be a brief time between Creating the channel and registering the address when the channel will be un-callable by the client, but since the likely flow is (1) create channel (2) gov vote to send tokens to new address, which can be queried on the counterparty off-chain (3) gov vote to perform some action over the channel. And the probability that we execute the action over (3) before the final packet of (1) is finished processing is so amazingly small, I don't see a reason to bend the spec to achieve this.

As long as it happens with no further user intervention, saving one round trip is not a useful optimisation here IMO.

[AdityaSripal]

I have some strong reservations about you (mis-)using version as metadata, especially in the 2nd official spec coming from IG/ICF. This will encourage all other protocol designers to (mis-)use this field in the same way. Wouldn't it be better to simply add an optional metadata field in ibcv3 in some backwards compatible way, as ICA will require ibcv3 anyway?

What do you mean by ibcv3 in this context? If you mean ibc-go v3.0, then adding a metadata optional field is quite a big change since it also requires changing the handshake messages and callbacks. I'm also not 100% sure adding optional fields to proto messages is backwards compatible with the current way sdk does proto. cc: @colin-axner

Furthermore, I don't agree that this is a "misuse" of the version field. Unlike the connection version, the IBC core spec explicitly leaves the channel version completely unspecified because it is up to specific applications to decide how they wish to use it. In fact, when discussing this proposal with the team we briefly considered simply renaming the version field in channel handshake messages to metadata anyway since core IBC places no semantic meaning on it. However, this was too disruptive of a change, so we opted instead to keep it named version and document that it may be used as a generic metadata field if applications wish.

I think the same UX can be acheived with a much less invasive manner. Basically, after the handshake is confirmed (OnChannelAck) the "server" chain (the one creating the account) can send a message over the channel to the "client" chain (the counterparty that will call these accounts) registering the account address for the channel. This will only be called once and provide the address.
There may be a brief time between Creating the channel and registering the address when the channel will be un-callable by the client, but since the likely flow is (1) create channel (2) gov vote to send tokens to new address, which can be queried on the counterparty off-chain (3) gov vote to perform some action over the channel. And the probability that we execute the action over (3) before the final packet of (1) is finished processing is so amazingly small, I don't see a reason to bend the spec to achieve this.
As long as it happens with no further user intervention, saving one round trip is not a useful optimisation here IMO.

So this is what the original code was doing. We had a special "one-time" packet that would be sent from the host chain to the controller chain and would include the address of the interchain account.

The reasons for moving away from this approach was not for latency. It introduced a completely new packet type so that some final initialization logic can be completed after the channel is already established. This is a design pattern that in our opinion was worse to encourage, than one in which all initialization logic gets packaged in the handshake and we use the completely arbitrary version string to do any negotiation necessary between the two parties before the channel is created.

Once the channel is open, we can start sending messages knowing that all the initialization has been done. This preserves the semantics between channel handshake doing all of the setup necessary, and when we actually start sending packets we are executing the protocol with setup already being fully completed.

[ethanfrey]

Okay, thank you for expanding on your thinking. My big takeaway is that you wish to repurpose this field to generic handshake Metadata, not a protocol/version string.

I do think it would be non breaking on the protocol level to rename the field, with the same type and field number. And since this is used proto encoded not json encoded, this shouldn't change the wire encoding, just the calling code. (please double check).

In any case, this field should be well commented to now be generic handshake Metadata and some best practices should be documented with ics27 as the first example.

[ethanfrey]

As to best practices, let's get rid of any string parsing that tries to act like a version string (ics27-1-cosmos1jdieijnfe39dn383...) and rather use a proper machine readable codec.

Since this is a string field and meant to be human readable, I would suggest using JSON here, also for ics27. ics20-1 can be some "legacy" format or a simple format only used when no Metadata is needed.

As to ics27, I would propose something like this:

{
  "protocol": "ics27",
  "version": 1,
  "address": "cosmos1kdu783nfo83jns8a3"
}

Where address is not present on the first call (OnChanInit) but generated in OnChanTry and kept from there on.

This would address my largest concerns and show we are really using a Metadata field rather than try to misuse a version field.

I could see many other protocols using this to handle multiple versions, optional features, etc in an easy to parse format

[AdityaSripal]

I believe this is already the direction @colin-axner is planning to head in with this comment:

#626 (comment)

[ethanfrey]

I believe this is already the direction @colin-axner is planning to head in with this comment:

#626 (comment)

Thank you for this link. Below from @colin-axner

If we are moving the connection sequences to the channel version, we think it'd make sense to use a typed defintion for the version instead of continuing to overload it with string parsing. The suggestion would be to have a proto definition which contains the: version, connection-id, counterparty connection-id, and interchain account address (existing version already contains version and interchain account address). Then using the JSON marshalled bytes of this version as the version bytes set in the channel structure.

Yes. I agree here 💯
Using JSON bytes on the wire (so relayers can inspect a bit) and providing a protobuf message definition to define the types. (type safe JSON).

[mpoke]

@AdityaSripal Isn't this issue fixed by #642?

[AdityaSripal]

Yes it is, thank you for combing through the issues.

Closed b y #642