Skip to content
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

some accepts/implements annotations don't resolve to existing scopes #14353

Closed
pyramation opened this issue Dec 18, 2022 · 2 comments · Fixed by #14476
Closed

some accepts/implements annotations don't resolve to existing scopes #14353

pyramation opened this issue Dec 18, 2022 · 2 comments · Fixed by #14476
Labels
C: Proto Proto definition and proto release

Comments

@pyramation
Copy link
Contributor

pyramation commented Dec 18, 2022

I've gone over every accepts/implements interface and found some discrepancies.

I've parsed the accepts and implements and analyzed every possible lookups that the Go interpreter will lookup based on the .proto and the name used in the annotation (link to the Go protobuf scope lookup algorithm)

If I misunderstood something, please let me know!

Here are the bad protos that I've found

cosmos.bank.v1beta1.Authorization
cosmos.bank.Authorization
cosmos.Authorization
Authorization
cosmos.gov.v1.Content
cosmos.gov.Content
cosmos.Content
Content
cosmos.group.v1.cosmos.group.v1.DecisionPolicy
cosmos.group.cosmos.group.v1.DecisionPolicy
cosmos.cosmos.group.v1.DecisionPolicy
cosmos.group.v1.DecisionPolicy
cosmos.staking.v1beta1.Authorization
cosmos.staking.Authorization
cosmos.Authorization
Authorization
@amaury1093 amaury1093 added the C: Proto Proto definition and proto release label Jan 3, 2023
@amaury1093
Copy link
Contributor

amaury1093 commented Jan 3, 2023

Thanks @pyramation!

If we agree to go with full-scoped names, I propose to update all SDK's proto files with these changes:

  • Authorization -> cosmos.authz.v1beta1.Authorization
  • Content -> cosmos.gov.v1beta1.Content

Edit: I found a couple more:

  • AccountI -> cosmos.auth.v1beta1.AccountI
  • DecisionPolicy -> cosmos.group.v1.DecisionPolicy
  • ModuleAccountI -> cosmos.auth.v1beta1.ModuleAccountI
  • FeeAllowanceI -> cosmos.feegrant.v1beta1.FeeAllowanceI
  • SupplyI -> cosmos.bank.v1beta1.SupplyI

@pyramation
Copy link
Contributor Author

I think fully-scoped names will solve the issue and remove ambiguity for new packages that use interfaces.

docs that may need updating after this change:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Proto Proto definition and proto release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants