-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
chore: Clean up {accept,implement}_interface
#14476
Conversation
…osmos/cosmos-sdk into am/14353-clean-proto-annotations
UPGRADING.md
Outdated
+ "cosmos.feegrant.v1beta1.FeeAllowanceI" | ||
``` | ||
|
||
Please also check that in your own app's proto files that there are no single-word names for those two proto annotations. If so, then replace them with fully-qualified names, even though those names don't actually resolve to an actual protobuf entity. |
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.
With @pyramation, we decided to always use fully-scoped names. However, these names don't map to anything in protobuf. For example, cosmos.authz.v1beta1.Authorization
is not defined anywhere in proto files. Is that fine?
There's an optional proposal to map them to some human-readable description here: https://github.com/cosmos/cosmos-sdk/pull/14280/files#r1047005098
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.
IMO it's completely fine, but we can have some docs if necessary.
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.
docs that may need updating after this change:
- https://github.com/regen-network/cosmos-proto#implements_interface
- https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/core/05-encoding.md#interface-encoding-and-usage-of-any
Also, would be great if there is an ADR somewhere that may need updating? If we can describe that 1. these interfaces need to be fully-scoped, and also 2. won't be defined in any proto files, just so it's clear, that would be great.
I'd love to share the updated docs with a few teams and folks like @faddat and his team, so we can update protos everywhere.
[Cosmos SDK] Kudos, SonarCloud Quality Gate passed! |
@@ -0,0 +1,1703 @@ | |||
// Code generated by protoc-gen-gogo. DO NOT EDIT. |
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.
note to the person that'll do the backport to 0.47. this folder can be removed. (please re-run make proto-gen
)
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.
lgtm!
(cherry picked from commit b244ffb) # Conflicts: # CHANGELOG.md # api/cosmos/auth/v1beta1/auth.pulsar.go # api/cosmos/bank/v1beta1/bank.pulsar.go # api/cosmos/gov/v1beta1/gov.pulsar.go # api/cosmos/gov/v1beta1/tx.pulsar.go # proto/cosmos/auth/v1beta1/auth.proto # proto/cosmos/evidence/v1beta1/tx.proto # proto/cosmos/gov/v1beta1/gov.proto # proto/cosmos/gov/v1beta1/tx.proto # x/auth/types/auth.pb.go # x/bank/types/bank.pb.go # x/gov/types/v1beta1/gov.pb.go # x/gov/types/v1beta1/tx.pb.go
hey did we also update https://github.com/cosmos/ibc-go ? |
Description
Closes: #14353
cc @pyramation
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