ICS-20: Acknowledgement::AsRef<u8> is incorrect

[plafer]

Bug Summary

The Acknowledgement struct's serialization in ICS-20 is incorrect.

Details

The Acknowledgement struct in ICS-20 has 2 different ways to serialize to bytes: the AsRef<[u8]> and serde.

The correct one seems to be the serde way (see ibc-go); it's also the one that has tests for it. However, the AsRef implementation is the one that's being used in write_acknowledgement().

Version

0.27.0

[plafer]

Since the JSON serialization/deserialization is quite simple, it can be done by hand in the AsRef implementation, and hence remove the dependency of the transfer module on serde.

The serialized form of the success message is:

{"success":"AQ=="}

and the error message:

{"error":"<error message>"}

[plafer]

I was confused because we had tested token transfers between basecoin-rs and gaiad, and everything seemed to work fine. However, basecoin-rs still uses ibc-rs v0.16.0, which did not write acknowledgements after processing packets. I confirmed this behavior between ibc-go's simd and basecoin-rs. Hence, the acknowledgements in ibc-rs were never integration-tested against gaiad.

I also confirmed that the success message output by ibc-go is {"success":"AQ=="}.

We will fix the output of acknowledgements in #364 and properly integration-test them in #370.

[plafer]

Since the JSON serialization/deserialization is quite simple, it can be done by hand in the AsRef implementation, and hence remove the dependency of the transfer module on serde.

The serialized form of the success message is:

{"success":"AQ=="}

and the error message:

{"error":"<error message>"}

Actually I was wrong there. An implementation could insert spaces in its JSON and still be compatible with ibc-go. The precise requirements for the acknowledgement are:

• Success: a JSON object containing only one key: "result" (and any associated value)
  • i.e. JSON of the form { "result": "<whatever>" }
• Error: a JSON object containing only one key: "error" (and any associated value)
  • i.e. JSON of the form { "error": "<whatever>" }

Explanation

Assume chain A sends a token to chain B.

• chain A: calls send_transfer()
• relayer relays packet to chain B
• chain B: recvPacket() is called
  • the packet makes its way to the token transfer's onRecvPacket callback, which returns an acknowledgement (assume { "result": "hello world" } for this example)
  • a WriteAcknowledgement event is generated, which contains the full json output
  • The acknowledgement is hashed and stored at the appropriate location in the store
• relayer relays the ack contained in the event back to chain A
• chain A: calls acknowledgePacket()
  • the core handler hashes the acknowledgement and verifies the Merkle proof of the hash. The check succeeds.
  • the router passes the ack to the token transfer's OnAcknowledgementPacket(). The JSON is parsed, and the parsing succeeds. Specifically, it is parsed into Acknowledgement_Result, with the Result field containing "hello world" (as an ASCII-encoded byte array).

The key realizations are:

• The Merkle proof for the acknowledgement will succeed on chain A as long as the bytes are actually what was hashed and stored on chain B; even if they weren't JSON-decodable
• A JSON decoder is used to parse the bytes in the token transfer's OnAcknowledgementPacket(), and the "internal" value of the Success or Error structs is never used
  • So only the JSON keys are important ("result" or "error"), and whitespaces won't fail the parse