Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ICS-4: Acknowledgement Envelope is misleading #918

Open
plafer opened this issue Jan 20, 2023 · 1 comment
Open

ICS-4: Acknowledgement Envelope is misleading #918

plafer opened this issue Jan 20, 2023 · 1 comment
Labels
improvement Improvement or enhancement to make specs more comprehensible tao Transport, authentication, & ordering layer.

Comments

@plafer
Copy link
Contributor

plafer commented Jan 20, 2023

The section states:

The field numbers 21 and 22 were explicitly chosen to avoid accidental conflicts with other protobuf message formats used for acknowledgements. The first byte of any message with this format will be the non-ASCII values 0xaa (result) or 0xb2 (error).

This suggests that the protobuf encoding of this message is used. However, in ICS-20 and ICS-721,

Note that both the FungibleTokenPacketData as well as FungibleTokenPacketAcknowledgement must be JSON-encoded (not Protobuf encoded) when they serialized into packet data.

suggesting that apps don't even use that Acknowledgement Envelope as described in ICS-4.

In my opinion, it would be less confusing if we

  1. removed that section from ICS-4, and
  2. have apps specify their Acknowledgement format in JSON directly.

Note that as stated, the ICS-20/721 statement is incomplete. "JSON-encoded" doesn't imply "encode byte arrays with base64"; this is a by-product of the JSON-encoding proto3 spec (which took me some time to figure out). It is also implied that the JSON must have all spaces removed (implicitly done by MustSortJSON()). Specifying the format in JSON directly would remove any sort of confusion.

@crodriguezvega crodriguezvega added tao Transport, authentication, & ordering layer. improvement Improvement or enhancement to make specs more comprehensible labels Mar 31, 2023
@colin-axner
Copy link
Contributor

Great suggestion. Only caveat I would add is if the spec continues to allow any encoding format to be used for acknowledgements/packet data, it might still be worth keeping that section on proto encoding, but making it more explicit that standard encoding use has been JSON and not proto

I will open an issue for adding encoding format's of packet data as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement or enhancement to make specs more comprehensible tao Transport, authentication, & ordering layer.
Projects
None yet
Development

No branches or pull requests

3 participants