Clarify usage of VerificationLogEntry.canonicalized_body. #74
Conversation
This updates the doc comment of canonicalize_body to make it clear that it is an optional field, and provides more details on when the field should be set / when it can be safely omitted. Signed-off-by: Billy Lynch <billy@chainguard.dev>
| // `canonicalized_body` matches the signature provided in the | ||
| // `Bundle.content`. | ||
| // If not set, clients are responsible for constructing an equivalent | ||
| // payload from other sources to verify the signature. | ||
| bytes canonicalized_body = 7; |
There was a problem hiding this comment.
Should this have the optional keyword?
| bytes canonicalized_body = 7; | |
| optional bytes canonicalized_body = 7; |
There was a problem hiding this comment.
¯_(ツ)_/¯ I'm in different.
proto3 generally takes a stance of everything is "optional", i.e. it's not considered an error if you don't set a field, it just won't include it in serialization if it's the zero value. IIUC the optional keyword for proto3 is really for field existence if you want to distinguish between the field wasn't set vs the empty string was set, which I don't think gives us much value in this context.
There was a problem hiding this comment.
Gotcha, yeah it's unclear how declaring optional should affect anything. In server code, anything can be nil anyway and must be checked, so there's functionally no difference.
Summary
This updates the doc comment of canonicalized_body to make it clear that it is an optional field, and provides more details on when the field should be set / when it can be safely omitted.
This is not intended to be a change in behavior - this is just clarifying existing intent of the field.
Fixes #72
Release Note
Documentation
If there's anything else I need to do besides
make all, let me know!