Provide standard Result<success, error> acknowlegement packet envelope

[ethanfrey]

Problem

When I send a packet, there are 2 possible results in the eyes of IBC:

• Timeout - OnTimeout
• Return Packet (success or error) - OnAcknowledgePacket

For my application, there are also 2 cases I need to handle (and these are what most clients care about):

• Packet Succeeded (with some result data)
• Packet failed (application error or timeout)

These are not the same cases and we need to do some protocol-specific packet inspection to map the IBC OnAcknowledgePacket call to success or failure.

If I build a IBC packet visualizer, that can handle dynamic protocols, which makes most sense to display to the user: "delivered / timed out" or "succeeded / failed"? Sure "Succeed / failed / timed out" may be the most desirable, but clearly the key information is whether the packet was accepted (and transfer made).

Proposal

I do not want to change the IBC protocol, but rather add some standard conventions on top of it that will enable better tooling for all users. Currently, acknowledgePacket receives acknowledgement: bytes for both success and failure cases. Where timeoutPacket receives no bytes. While timeoutPacket can easily be mapped to "error", we would need a standard envelope for the acknowledgement bytes to do so.

I propose the following standard as a protobuf format:

message Acknowledgement {
  option (gogoproto.equal) = true;
  option (gogoproto.goproto_stringer) = false;
  oneof response {
    bytes result = 21;
    string error = 22;
  }
}

I choose the fields 21 and 22 to not accidentally conflict with some other, unrelated format. If len(success) > 0 && len(error) == 0 then this is a clear success with msg.success representing any app-specific data to be parsed down the line. If len(success) == 0 && len(error) > 0 then this is a clear error with some error string, which may be displayed. If neither of those are true, then we cannot introspect this packet and leave it as "unknown" (for relayers / visualizers) or use app-specific parsing methods (for the on-chain logic).

An observer can then map timeoutPacket to Acknowledgement { error: "timeout" } and all packet responses can be marked as success/green with some opaque app-data, or failure/red with some human-readable error message. Furthermore, we could provide a helper function to do this mapping and then directly call success/failure callbacks, which would slightly simplify the logic in the modules, or at least ensure they do they same thing on error and timeout

Related Work

This is similar to the case that there is no enforced format to the version identifier in ICS3, but it is recommended to use the version/feature format defined in the cosmos-sdk (https://github.com/cosmos/cosmos-sdk/pull/6640).

I think this level of "recommended types" really belongs in the ICS repo first, with cosmos-sdk following it (or at least backported to the specs as "recommended formats"). Thus, I make the issue on the ics repo.

Next Steps

If this issue is generally accepted, I would write a PR adding this opt-in protocol to ics4 and define ics20 to make use of it.

After that is finalized, I would prepare a PR to update ics20 in the cosmos-sdk (unless someone else wishes to), so it uses the defined envelope.

I would also add this logic to the upcoming ibc introspection tools in cosmjs

History

The proto format has been updated, the original proto proposal we refer to below was:

message Acknowledgement {
  option (gogoproto.equal) = true;
  option (gogoproto.goproto_stringer) = false;
  bytes success = 21;
  string error = 22;
}

[cwgoes]

I concur with the problem description & with the proposal to standardise some success/failure envelope.

I have a few questions on the specifics:

1. Can we call the field result instead of success? I assume you intend it to be able to represent some data which resulted from successful packet execution.
2. Is it possible to represent a sum type (bytes success | string error) cleanly in protobuf (maybe with Any)? Is there some reason applications might want both a non-empty success field and a non-empty error field in the same acknowledgement? That seems confusing to me.
3. Does it make sense to have timeoutPacket use the same Acknowledgement type? That might be convenient for some applications, but I think it's a little strange because the Acknowledgement type can represent values that timeoutPacket should never be handling (e.g. a success).

[ethanfrey]

Very good points. I agree with both (1) and (2) and will update the proposal with that feedback.

For (3) timeout carries no other information that it failed in a timeout, and I proposed some app-level mapping of this to an AcknowledgePacket so we could have "onPacketError" / "onPacketSuccess" but on the protocol level I would not use this with timeout.

Unless you want to standardize this as the only return packet at the ics level. And then use bytes result | string error | bool timeout as a return value, so just have onPacketDone or something instead of onPacketTimeout + onPacketAcknowledged

[cwgoes]

For (3) timeout carries no other information that it failed in a timeout, and I proposed some app-level mapping of this to an AcknowledgePacket so we could have "onPacketError" / "onPacketSuccess" but on the protocol level I would not use this with timeout.

I think this is fine at the app-level, sure.

Unless you want to standardize this as the only return packet at the ics level. And then use bytes result | string error | bool timeout as a return value, so just have onPacketDone or something instead of onPacketTimeout + onPacketAcknowledged

That's not a bad idea; I would still want this to be optional (at the protocol level, acknowledgement data will remain []byte), but as a recommended encoding standard with included proto files and as an SDK interface this seems clean & a nice default.

[ethanfrey]

I updated the protobuf type above to use oneof. Shall I add the timeout field as well?

[cwgoes]

I updated the protobuf type above to use oneof. Shall I add the timeout field as well?

Thanks - no, that's fine, I think we can just add the onPacketDone in the SDK API, we don't need to change the serialisation.