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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jan 4, 2023
Merged

chore: Clean up {accept,implement}_interface #14476

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b760d1c
chore: Clean up `{accept,implement}_interface`
amaury1093 Jan 4, 2023
78231eb
Changelog
amaury1093 Jan 4, 2023
6790b24
Merge branch 'main' into am/14353-clean-proto-annotations
amaury1093 Jan 4, 2023
4fc7f2f
typo
amaury1093 Jan 4, 2023
dc0705b
Merge branch 'am/14353-clean-proto-annotations' of ssh://github.com/c…
amaury1093 Jan 4, 2023
a0da913
fix upgrading
amaury1093 Jan 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Changelog
  • Loading branch information
@amaury1093
amaury1093 committed Jan 4, 2023
commit 78231eb4b41d0d6be593c707a184163a064bed40
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Improvements

* (module)[#14415](https://github.com/cosmos/cosmos-sdk/pull/14415) Loosen assertions in SetOrderBeginBlockers() and SetOrderEndBlockers()
* (protobuf) [#14476](https://github.com/cosmos/cosmos-sdk/pull/14476) Clean up protobuf annotations `{accepts,implements}_interface`.
* (module) [#14415](https://github.com/cosmos/cosmos-sdk/pull/14415) Loosen assertions in SetOrderBeginBlockers() and SetOrderEndBlockers()
* (context)[#14384](https://github.com/cosmos/cosmos-sdk/pull/14384) refactor(context): Pass EventManager to the context as an interface.
* (types) [#14354](https://github.com/cosmos/cosmos-sdk/pull/14354) - improve performance on Context.KVStore and Context.TransientStore by 40%
* (crypto/keyring) [#14151](https://github.com/cosmos/cosmos-sdk/pull/14151) Move keys presentation from `crypto/keyring` to `client/keys`
Expand Down
25 changes: 25 additions & 0 deletions UPGRADING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,31 @@ message MsgSetWithdrawAddress {
}
```

#### `{accepts,implements}_interface` proto annotations
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved

The SDK is normalizing the strings inside the Protobuf `accepts_interface` and `implements_interface` annotations. We require them to be fully-scoped names. They will soon be used by code generators like Pulsar and Telescope to match which messages can or cannot be packed inside `Any`s.

Here are the following replacements that you need to perform on your proto files:

```diff
- "Content"
+ "cosmos.gov.v1beta1.Content"
- "Authorization"
+ "cosmos.authz.v1beta1.Authorization"
- "Content"
+ "cosmos.base.v1beta1.Msg"
- "AccountI"
+ "cosmos.auth.v1beta1.AccountI"
- "ModuleAccountI"
+ "cosmos.auth.v1beta1.ModuleAccountI"
- "FeeAllowanceI"
+ "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.
Copy link
Contributor Author

@amaury1093 amaury1093 Jan 4, 2023 •
edited
Loading

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

Copy link
Contributor

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.

Copy link
Contributor

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:

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.


For more information, see the [encoding guide](./docs/docs/core/05-encoding.md).

<!-- todo: cosmos.scalar types -->

When clients interract with a node they are required to set a codec in in the grpc.Dial. More information can be found in this [doc](https://docs.cosmos.network/v0.46/run-node/interact-node.html#programmatically-via-go).