ICS-4: chanOpenTry still stores a ChannelEnd with an incorrect version

[plafer]

In chanOpenTry, the following lines are incorrect:

    channel = ChannelEnd{TRYOPEN, order, counterpartyPortIdentifier,
                         counterpartyChannelIdentifier, connectionHops, version}
    provableStore.set(channelPath(portIdentifier, channelIdentifier), channel)

The version field is deprecated since the removal of crossing hellos, and so we shouldn't store it as part of our ChannelEnd.

Seems like this was forgotten to be updated in #629.

Related: #851

[crodriguezvega]

Thanks, @plafer. I have a question:

The version field is deprecated since the removal of crossing hellos, and so we shouldn't store it as part of our ChannelEnd.

As you pointed out in this issue, The version field in the datagram MsgChanOpenTry datagram is deprecated, but not the version passed to the chanOpenTry handler, right?

As it currently stands, given that in handleChanOpenTry in ICS26 the module onChanOpenTry callback is called first and then the chanOpenTry handler function is called with the version returned by the callback, why is the version used to construct the ChannelEnd in chanOpenTry incorrect? In ibc-go we do store the version as part of the channel end.

[plafer]

Ah yes, you're right; I probably got mixed up by reading the standard and ibc-go together, and forgot that the standard's ICS-26 calls onChanOpenTry first.

So IIUC, the error in the spec is now that onChanOpenTry is called with datagram.channelIdentifier, whereas it should be called with the channelIdentifier returned by chanOpenTry, right? Then, the correct way to do it would be to write the ChannelEnd to storage in ICS-26, as is done in ibc-go, right?

[crodriguezvega]

So IIUC, the error in the spec is now that onChanOpenTry is called with datagram.channelIdentifier, whereas it should be called with the channelIdentifier returned by chanOpenTry, right? Then, the correct way to do it would be to write the ChannelEnd to storage in ICS-26, as is done in ibc-go, right?

Where you say channelIdentifier, is that a typo and you actually meant version?

I think the problem in the spec right now is that one described in #851 (i.e. the difference betweek spend and ibc-go in the order of the calls to the callback and the handler). However, if we change the spec to match ibc-go and the handler is called before the callback, then we will have another problem as you mention: storing the channelEnd should probably be moved out of the handler and placed after the call to the callback so that it can be constructed with the version returned by the callback (what ibc-go does). I will check with @AdityaSripal what he thinks about this.

[plafer]

Where you say channelIdentifier, is that a typo and you actually meant version?

No I was really talking about the channelIdentifier. In ibc-go, the core chanOpenTry handler returns the newly generated channelID, which is then passed to the onChanOpenTry callback.

[crodriguezvega]

No I was really talking about the channelIdentifier. In ibc-go, the core chanOpenTry handler returns the newly generated channelID, which is then passed to the onChanOpenTry callback.

Sorry, I see now what you mean!
Yes, that looks like something that we should also fix. Would it make sense to close this issue (since there's no fix currently needed regarding the version, although there will be once we tackle #851) and open a new one for the channelIdentifier issue?

[plafer]

Absolutely