-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
feat: Validate Msg proto annotations #13793
Conversation
…05-valid-annotations
…05-valid-annotations
This is R4R again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK, nice to see it cleaned up some items
Co-authored-by: Marko <[email protected]>
…05-valid-annotations
fs, ok := c.protoFiles.(*protoregistry.Files) | ||
if !ok { | ||
return fmt.Errorf("expected *protoregistry.Files in GetSignersContext, got %T", c.protoFiles) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to add this change after #15413.
@aaronc @kocubinski In practice, would we ever want protodesc.Resolver
not to be *protoregistry.Files
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe for testing failure modes in unit tests?
Edit: If we find a need, we could create an interface in signing
like
RangeFiles(f func(protoreflect.FileDescriptor) bool)
and cast to that instead of protoregistry.Files
. But since we don't have that need yet, and that would be a non-breaking change, I vote this is fine for now.
Co-authored-by: Marko <[email protected]>
Description
This PR aims to do 2 things:
Msg
services have thecosmos.msg.service=true
annotation.Msg*
messages have thecosmos.msg.signer=...
annotation.If it's not the case, then it prints a warning to StdErr at startup.
It also fixes a couple of wrong proto annotations it caught 💪
Closes: #13405
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change