Consolidate channel upgrade message responses

[chatton]

Summary

Currently, we make use of a ResultType to indicate if a response was successful or failure as a way of making sure an error receipt is written to state and to inform the relayers that an upgrade should be cancelled.

However some other message responses don't have this field such as MsgChannelUpgradeInitResponse and MsgChannelUpgradeCancelResponse.

While they do not require the result to work, it may be a good idea to make the API consistent across all messages.

[DimitrisJim]

adding discussion label back, missed to talk about it in previous eng. call (see #4022 (comment))

[crodriguezvega]

Use ResponseResultType in Ack and Confirm responses.

[DimitrisJim]

Hm, this currently exists in both Ack and Confirm. It exists in all upgrade handlers responses except Init, Open and Cancel (and for Cancel, adding this result type leads to more confusion as #4022 (comment) stated).

I guess these were added during the refactor. Will probably close after a revisit to re-check I'm not missing something.

[colin-axner]

Yea, I think the response result can only be success in the other upgrade handlers as a failure indicates a successful tx but a failed handshake step. If we find it useful to return information about the upgrade in the resposne, I'd recommend changing the result type to string and using enums which can be updated in post release to make the other handshake steps more useful:

type MsgChannelUpgradeTryResponse struct {
	ChannelId       string             `protobuf:"bytes,1,opt,name=channel_id,json=channelId,proto3" json:"channel_id,omitempty"`
	Upgrade         Upgrade            `protobuf:"bytes,2,opt,name=upgrade,proto3" json:"upgrade"`
	UpgradeSequence uint64             `protobuf:"varint,3,opt,name=upgrade_sequence,json=upgradeSequence,proto3" json:"upgrade_sequence,omitempty"`
	Status          string `protobuf:"varint,4,opt,name=status,proto3,json:"result,omitempty"`
}

var (
    SUCCESS_UPGRADE_INIT = "SUCCESS_UPGRADE_INIT"
    SUCCESS_UPGRADE_FLUSHING = "SUCCESS_UPGRADE_FLUSHING"
    SUCCESS_UPGRADE_FLUSHCOMPLETE = "SUCCESS_UPGRADE_FLUSHCOMPLETE"
    SUCCESS_UPGRADE_COMPLETE = "SUCCESS_UPGRADE_COMPLETE"
    FAILURE_UPGRADE_CANCELLED = " FAILURE_UPGRADE_CANCELLED"
    FAILURE_COUNTERPARTY_OUT_OF_SYNC = "FAILURE_COUNTERPARTY_OUT_OF_SYNC"
)

I propose a string so that we aren't tied to a specific struct. That way we can just leave the success/failure as is but update to more descriptive responses if we so choose. Just a proposal though

This is similar to what I was ethereum do with one of their api's. They have a response status which is a string and then various string response values (valid, invalid, syncing)

[colin-axner]

The result type is only non-success in TRY, ACK, CONFIRM. We can remove from timeout if we want to be conservative. I would be in favour of that (and appears to be what we previously agreed by stating it for ack and confirm). @DimitrisJim want to remove it from the timeout msg?

[colin-axner]

Should we remove the channel id from try as well? https://github.com/cosmos/ibc-go/blob/04-channel-upgrades/proto/ibc/core/channel/v1/tx.proto#L324C1-L325C43

[DimitrisJim]

I'd be in favor of overhauling the resp type to allow for more context. It is currently used in previous handlers which would also require updating (recv, ack, timeout, timeoutonclose as far as I could tell).

Opening PR now for dropping resp from timeout and channel from try resp.

[colin-axner]

I'm calling this issue closed. I've opened a new issue for the idea I put out, but I don't think it's worth making any last minute changes for. If more descriptive result values are ever requested we can use the deprecate/new field approach