cosmos / ibc-go

Inter-Blockchain Communication Protocol (IBC) implementation in Golang.
https://ibc.cosmos.network/
MIT License
548 stars 586 forks source link

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

Closed DimitrisJim closed 4 months ago

DimitrisJim commented 4 months ago

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

DimitrisJim commented 4 months ago

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 commented 4 months ago

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.

colin-axner commented 4 months ago

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: