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

Update cosmos-sdk-proto to use cosmos-sdk v0.44.1 and ibc-go v1.2.0 #138

Merged

Conversation

ChristianBorst
Copy link

@ChristianBorst ChristianBorst commented Oct 7, 2021

Update to use cosmos-sdk v0.44.1. An important consideration in this update is that the ibc module has been split off into it's own repo at https://github.com/cosmos/ibc-go.

In the compilation of the ibc-go protos, @jkilpatr discovered that the service compilation (in the compile_*_services() functions) recompiles and overwrites the regular definitions (in the respective compile_*_protos() functions), resulting in missing definitions. This is a result of combining all .proto definitions into a single file, e.g. ibc-go/proto/ibc/applications/transfer/v1 has genesis.proto, query.proto, transfer.proto, tx.proto. After running 2 stage compilation for the protos and then the services, the GenesisState struct from ibc-go/proto/ibc/applications/transfer/v1/genesis.proto was missing in ibc.applications.transfer.v1.rs. Cancelling the service compilation (aka the 2nd stage) resulted in the GenesisState struct appearing but all grpc client methods from the other files were not being generated. Applying the service compilation method (using tonic_build) to all ibc-go proto files results in everything we want compiled.

This 1 stage compilation approach was applied to the updated cosmos-sdk protos and several missing definitions were discovered (see the latest commit for which ones precisely).

NOTE: wasmd and cosmrs have not been touched and require updates

NOTE: the last commit in this PR is a quick fix to resolve a namespace conflict introduced by prost's generation of an enum and a struct both named Validators in cosmos-sdk-proto/src/prost/cosmos.staking.v1beta1.rs's stake_authorization mod. This is not a sustainable solution, but it does allow the module to compile. I renamed the enum to IsStakeAuthorizationValidators and the struct to StakeAuthorizationValidators to most closely match the generated go names here and here

@adizere
Copy link

adizere commented Oct 7, 2021

Lingering issue: proto_build generates a conflicting struct and enum both named Validators

We're encountering the same problem in our own proto generation approach. The fix we have is manual.
informalsystems/hermes@fc0eab5

It would be worth opening an issue in the SDK repository to fix it.

@ChristianBorst ChristianBorst force-pushed the christianborst/update-sdk-v0.44.1 branch from d38a6ba to 65b7b99 Compare October 7, 2021 18:53
@ChristianBorst ChristianBorst changed the title WIP: Update cosmos-sdk-proto to use cosmos-sdk v0.44.1 and ibc-go v1.2.0 Update cosmos-sdk-proto to use cosmos-sdk v0.44.1 and ibc-go v1.2.0 Oct 7, 2021
@ChristianBorst
Copy link
Author

We're encountering the same problem in our own proto generation approach. The fix we have is manual. informalsystems/ibc-rs@fc0eab5

It would be worth opening an issue in the SDK repository to fix it.

Thanks for the suggestion, I like the short and sweet approach you took in ibc-rs. I renamed the members as closely to the generated go members used in the cosmos-sdk source repo, but I will be opening an issue suggesting a name change.

@@ -1,6 +1,6 @@
[package]
name = "cosmos-sdk-proto"
version = "0.7.0" # Also update html_root_url in lib.rs when bumping this
version = "0.7.1" # Also update html_root_url in lib.rs when bumping this
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the services have been modified with this update (e.g. cosmos.bank.v1beta1.rs), so a version change to v0.8.0 may be preferable

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do that, perhaps it's worth considering trying to (temporarily) switch to Informal's fork of Prost? If nothing else, it would allow the docs to build /cc @xla
https://github.com/informalsystems/tendermint-rs/pull/1005/files

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would belong in another pr, we can bump to 0.8.0 there. Does that sound good?

@ChristianBorst
Copy link
Author

We're encountering the same problem in our own proto generation approach. The fix we have is manual. informalsystems/ibc-rs@fc0eab5
It would be worth opening an issue in the SDK repository to fix it.

Thanks for the suggestion, I like the short and sweet approach you took in ibc-rs. I renamed the members as closely to the generated go members used in the cosmos-sdk source repo, but I will be opening an issue suggesting a name change.

Issue submitted: cosmos/cosmos-sdk#10323

@ChristianBorst ChristianBorst force-pushed the christianborst/update-sdk-v0.44.1 branch from 65b7b99 to 6c01a92 Compare October 7, 2021 20:25
Recently the cosmos team split the x/ibc module into its own
repo located at github.com/cosmos/ibc-go, these changes add
the ibc-go repo as a submodule, update cosmos-sdk-go to v0.44.1
and enable integration of ibc-go protos with the cosmos-sdk protos
In my effort to add support for the new cosmos/ibc-go repo,
jkilpatr discovered that the compile_*_proto_services() functions
recompile and overwrite files previously generated in the respective
compile_*_protos() functions. This change fixes cosmos-sdk proto
compilation by consolidating proto definitions and services into
one function: compile_sdk_protos_and_services().

Fixed compiled protos to follow.
See previous commit regarding the code fix leading to these updated
compiled proto files.
In v0.43.* the staking module introduced authz.proto, which has
an enum (oneof validators) and a struct (message Validators) both
defined under message StakeAuthorization. When this struct runs through
prost it creates a namespace conflict. This commit simply renames
those conflicts. Since this StakeAuthorization is new to cosmos-rust
no downstream issues should ocurr.
@ChristianBorst ChristianBorst force-pushed the christianborst/update-sdk-v0.44.1 branch from 6c01a92 to af39fcf Compare October 7, 2021 21:42
@jkilpatr jkilpatr merged commit 1e8e5ed into cosmos:main Oct 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants