feat(proto)!: remove optional fields in protos #2940
Conversation
conorsch
force-pushed
the
no-optional-fields-in-protos
branch
from
15afac0
to
1d967c3
Compare
conorsch
force-pushed
the
no-optional-fields-in-protos
branch
from
1d967c3
to
2515c42
Compare
conorsch
force-pushed
the
no-optional-fields-in-protos
branch
from
2515c42
to
493c561
Compare
conorsch
force-pushed
the
no-optional-fields-in-protos
branch
from
493c561
to
dc71918
Compare
conorsch
force-pushed
the
no-optional-fields-in-protos
branch
from
dc71918
to
91070cd
Compare
conorsch
force-pushed
the
no-optional-fields-in-protos
branch
from
91070cd
to
b28c185
Compare
conorsch
force-pushed
the
no-optional-fields-in-protos
branch
from
b28c185
to
d1bc8ec
Compare
plaidfinch
had a problem deploying
to
smoke-test
GitHub Actions
Failure
conorsch
force-pushed
the
no-optional-fields-in-protos
branch
from
1c3dd33
to
78bd2c3
Compare
conorsch
force-pushed
the
no-optional-fields-in-protos
branch
from
7a9c6a7
to
843da8d
Compare
conorsch
changed the title
The cosmos fork of gogoproto, used to generate golang code from BSR, doesn't support "optional" fields in proto3 syntax. Rather than sort that out, we're removing use of optional to unblock integration work on interchaintest. In order to preserve the representation of optionality, we use submessages in the proto types, so we can include or omit the named field. A notable exception is for encrypted memos: since the encrypted memo payload must be a specific length, we can check for whether it's empty, and map an empty encrypted memo to None. H/t to @plaidfinch for describing the submessage pattern to me, and implementing it on the governance changes. Closes #2933.
conorsch
force-pushed
the
no-optional-fields-in-protos
branch
from
843da8d
to
04888f1
Compare
| Commit commit = 1; | ||
| } | ||
|
|
||
| // The sha1 hash of a git commit, used to indicate a specific version of the software. | ||
| message Commit { | ||
| string commit = 1; |
There was a problem hiding this comment.
Alternatively, this could just be string commit = 1 rather than a submessage, with the empty string meaning None.
|
|
||
| message UnbondingEpoch { | ||
| // The epoch in which stake becomes unbonded. | ||
| uint64 epoch = 1; | ||
| } | ||
|
|
||
| // Present if needed to specify unbonding epoch. | ||
| UnbondingEpoch unbonding_epoch = 2; |
There was a problem hiding this comment.
We could also just have uint64 unbonding_epoch = 2, since 0 is not a valid unbonding epoch.
| // Identifies the account group to query. Defaults to 0. | ||
| core.crypto.v1alpha1.AccountGroupId account_group_id = 14; |
There was a problem hiding this comment.
0 isn't a valid account group ID, right? So this comment should read
// If present, identifies the account group to query.
?
There was a problem hiding this comment.
Or we could just drop these entirely, since we don't use them at all and have no concrete plans to do multi-account view servers.
| // Submessage to represent optionality for specifying heights. | ||
| message BlockHeight { | ||
| uint64 height = 1; | ||
| } | ||
| // If present, return only transactions after this height. | ||
| optional uint64 start_height = 1; | ||
| BlockHeight start_height = 1; | ||
| // If present, return only transactions before this height. | ||
| optional uint64 end_height = 2; | ||
| BlockHeight end_height = 2; |
There was a problem hiding this comment.
Rather than defining a submessage, these should just be uint64 start_height and uint64 end_height, since 0 is not a valid block height.
| // Submessage to represent optionality for block height. | ||
| message BlockHeight { | ||
| uint64 height = 1; | ||
| } | ||
| // The height the transaction was included in a block, if known. | ||
| optional uint64 height = 1; | ||
| BlockHeight height = 1; |
There was a problem hiding this comment.
ditto above, this should just be a uint64.
| // Present if the note was spent, otherwise absent. | ||
| BlockHeight height_spent = 6; |
There was a problem hiding this comment.
ditto above, this should just be a uint64.
|
|
||
| // If present, height at which Swap was claimed. | ||
| BlockHeight height_claimed = 6; |
There was a problem hiding this comment.
ditto above, this should just be a uint64.
|
This PR changes a lot of scalar fields to have submessages, but in almost every case it would be simpler and easier (as well as less work, for ourselves making this change as well as all downstream users) to just use ordinary values. |
Great feedback, thanks for thorough review. I've opened a separate PR, #2951, with a fresh implementation as you describe. |
|
Superseded by #2951. |
The cosmos fork of gogoproto, used to generate golang code from BSR, doesn't support "optional" fields in proto3 syntax. Rather than sort that out, we're removing use of optional to unblock integration work on interchaintest.
In order to preserve the representation of optionality, we use submessages in the proto types, so we can include or omit the named field. A notable exception is for encrypted memos: since the encrypted memo payload must be a specific length, we can check for whether it's empty, and map an empty encrypted memo to None.
H/t to @plaidfinch for describing the submessage pattern to me, and implementing it on the governance changes.
Closes #2933.