Define what Success() in the Acknowledgement interface means

[webmaster128]

In ibc-go we have the interface

type Acknowledgement interface {
    Success() bool
    Acknowledgement() []byte
}

In wasmd we try to use that (not sure if that's the right way though).

Now it is not clear to me from the interface definition what Success() means. Does it mean a non-error acknowledgement? Does it mean the ack is committed to state and can be either success or error? We can only find this implementation details. But we don't want to derive the intended meaning from the implementation.

Ideally the Acknowledgement interface does not make a different between error and success and just stores an app specific binary acknowledgement. It is easy to envision non-binary acknowledgements in applications. In our case in wasmd we don't know the meaning if an acknowledgement that we receive from a contract.

[0xekez]

In addition to the above, the current Acknowledgement isn't spec compliant. ICS-004 says that ACKs SHOULD be encoded as protobufs:

message Acknowledgement {
  oneof response {
    bytes result = 21;
    string error = 22;
  }
}

However, the ACKs used in ibc-go's ICS-027 and ICS-20 implementations are JSON encoded.

Was this done intentionally?

Writing IBC protocols is currently very hard when the implementations are not spec compliant. As another example, ICS-027's spec says that messages must be signed by the sender; however, the implementation does not enforce this and seems only to require the signer be the sender if a signer is specified.

[webmaster128]

Was this done intentionally?

This is explicitly discussed in the ICS-20 spec. From there it was probably just continued, at least I did not find the section in ICS-27 yet. Anyways, this is a whole new (important!) topic that deserves its own ticket.

[crodriguezvega]

Thank you @webmaster128 and @0xekez for the discussion here.

Now it is not clear to me from the interface definition what Success() means. Does it mean a non-error acknowledgement? Does it mean the ack is committed to state and can be either success or error?

A successful acknowledgement is returned by the OnRecvPacket application callback is the callback executes completely without errors. Both success and error acknowledgements will be written to state (i.e. as long as the acknowledgement is not nil). Does this explanation help? We can find a place to add more information about this in our documentation then.

As another example, ICS-027's spec says that messages must be signed by the sender; however, the implementation does not enforce this and seems only to require the signer be the sender if a signer is specified.

I believe the spec and implementation are in sync: the message sent to the interchain account on the host (which is encoded in the data field of InterchainAccountPacketData) will be executed only if the message signer matches the interchain account address. And the message (MsgSendTx) to send the transaction to the interchain account includes an owner field that is the address of the "owner" of the interchain account, and that should be the same address that signs MsgSendTx. From that owner field a port ID is derived and there's channel capability associated with it, so that only the owner is capable to send messages to the interchain account. I hope this explanation helps!

However, the ACKs used in ibc-go's ICS-027 and ICS-20 implementations are JSON encoded. Was this done intentionally?

There is also an issue in the specs about this. We'll try to improve and clarify this.

[0xekez]

@crodriguezvega thank you for the additional info! I'm still not sure I understand this so I made an issue here writing up the question a little better: https://github.com/cosmos/ibc-go/issues/3494

[webmaster128]

Thank you @crodriguezvega. I wrote what I understood to be correct as a doc comment in #3515. If that is right, we can close here. If not or incomplete/unprecise, let's discuss over there. With this doc comment an interface implementor should have a much better idea how to implement the interface.