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

Mark fields not needing to be nullable as with gogoproto.nullable = false option #6550

Closed
3 tasks
DimitrisJim opened this issue Jun 10, 2024 · 3 comments
Closed
3 tasks
Assignees
Labels
20-transfer type: feature New features, sub-features or integrations

Comments

@DimitrisJim
Copy link
Contributor

DimitrisJim commented Jun 10, 2024

This applies to both fields on the MsgTransfer and fields of the new ForwardingInfo message.

Surfaced during internal walkthrough of ics20 v2 forwarding (commit efcfa5d)


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@crodriguezvega crodriguezvega added the type: feature New features, sub-features or integrations label Jun 10, 2024
@DimitrisJim DimitrisJim self-assigned this Jun 11, 2024
@DimitrisJim
Copy link
Contributor Author

DimitrisJim commented Jun 11, 2024

thinking about this the only place I'd add gogoproto.nullable=false would be in the embedded Hops of ForwardInfo since its the only place that should be set to some value as part of a correctly initialized ForwardInfo. For ForwardInfo on the message and packet, it is meant to be optional and potentially missing, signifying a normal transfer.

In addition, thinking of how these protos would be generated in better typed languages like Rust, it makes sense to have the fields ForwardInfo on MsgTransfer and on the packet data as an Option (we get pointers but, what can you do. I do not actually know how these protos would be generated in Rust, though).

@DimitrisJim
Copy link
Contributor Author

DimitrisJim commented Jun 12, 2024

marked hops as stated, going to slap needs discussion on it so we can talk about it for a quick minute.

Pertains to Forwarding fields on the message and packet; I'd be perfectly fine to make it non-nullable there if people strongly prefer it.

@DimitrisJim DimitrisJim added the needs discussion Issues that need discussion before they can be worked on label Jun 12, 2024
@colin-axner
Copy link
Contributor

I was originally in favour of using a non-nullable field as I couldn't see a benefit from a go perspective. But I did some investigation. Both protobuf and json act in synchrony regarding the following behaviour.

Pointer, empty = no field emitted/encoded
Pointer, set = field emitted/encoded
No pointer, empty = field emitted/encoded as empty ({} in json, *\x00 something like this in protobuf)
No pointer, set = field emitted/encoded

Given this. By using nullable:

  • the decoding actor can differentiate between non-set and empty values
  • smaller encoding size (slightly more performant)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
20-transfer type: feature New features, sub-features or integrations
Projects
Archived in project
Development

No branches or pull requests

3 participants