Mark optional protobuf fields as gogoproto.nullable in request messages

[Pitasi]

For example

message QueryKeyRequestsRequest {
  cosmos.base.query.v1beta1.PageRequest pagination = 1;
  uint64 keychain_id = 2;
  KeyRequestStatus status = 3; // Optional
  uint64 space_id = 4;
}

should be

message QueryKeyRequestsRequest {
  cosmos.base.query.v1beta1.PageRequest pagination = 1;
  uint64 keychain_id = 2 [ (gogoproto.nullable) = true ];
  KeyRequestStatus status = 3 [ (gogoproto.nullable) = true ];
  uint64 space_id = 4 [ (gogoproto.nullable) = true ];
}

this helps code generators (i.e. Telescope) to mark fields as optional/nullable.

[optifat]

@Pitasi Is the // Optional comment sufficient for the field to be such (no, judging by the example)? If not what are the criteria for that?

[Pitasi]

Are you asking how to mark a field as optional? In this case we have to add [ (gogoproto.nullable) = true ].

Or, are you asking how to determine if a field should be optional or not? In this case I don't have a precise answer, we need to go through all the queries and check the code to understand if a field is required to be there or not.

[optifat]

are you asking how to determine if a field should be optional or not?

Yes, I was asking about that

[backsapc]

@Pitasi I have a couple of questions.

1. In your example, the fields space_id and keychain_id became nullable. Is that correct? If so, does it mean we're making most of the query parameters nullable? Should we remove values like KeyRequestStatus_KEY_REQUEST_STATUS_UNSPECIFIED then?
2. Do we care about commands, such as UpdateSpace? Specifically, there are fields AdminRuleId and SignRuleId. If the user doesn't provide values for these fields, they will be filled with default values (0). This means the user could accidentally change the rule. This might not be an issue if we use PUT semantics, but it could be problematic otherwise.

[Pitasi]

@Pitasi I have a couple of questions.

1. In your example, the fields space_id and keychain_id became nullable. Is that correct? If so, does it mean we're making most of the query parameters nullable?

yes I think that makes sense for list operations in general, to have a way of getting everything but then you're able to filter it down by passing more params

Should we remove values like KeyRequestStatus_KEY_REQUEST_STATUS_UNSPECIFIED then?

since protobuf enums are numbers in Go, it's a best practice to leave those. Otherwise, in Go you wouldn't be able to differentiate "the param was not specified" by "the param is 0"

2. Do we care about commands, such as UpdateSpace? Specifically, there are fields AdminRuleId and SignRuleId. If the user doesn't provide values for these fields, they will be filled with default values (0). This means the user could accidentally change the rule. This might not be an issue if we use PUT semantics, but it could be problematic otherwise.

let's use PUT semantic as you said, it's more explicit and less error-prone

hope that helps!