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

Bump prost to v0.13 and tonic to v0.12 #1444

Merged
merged 4 commits into from
Jul 17, 2024
Merged

Bump prost to v0.13 and tonic to v0.12 #1444

merged 4 commits into from
Jul 17, 2024

Conversation

tony-iqlusion
Copy link
Collaborator

Updates these dependencies to their latest versions.

Protos have not been regenerated using the latest prost-build/tonic-build however seem to be compatible.

See also: #1442

Updates these dependencies to their latest versions.

Protos have not been regenerated using the latest
`prost-build`/`tonic-build` however seem to be compatible.
@tony-iqlusion tony-iqlusion mentioned this pull request Jul 16, 2024
@tony-iqlusion
Copy link
Collaborator Author

Well great, I didn't try regenerating the protos as noted in the description/commit message, but it seems if you do regenerate them they no longer compile per the generated-protos-compile CI job

@tony-iqlusion
Copy link
Collaborator Author

The core problem seems to be that crate::google::protobuf::Duration (i.e. prost_types::Duration) is no longer a Copy type, which means no struct that contains it can be a Copy type any more.

I'm not sure why this change was made. It's a simple struct with two public fields which are themselves Copy types (i64 and i32)

@tony-iqlusion
Copy link
Collaborator Author

Sidebar: in cosmos-sdk-proto and ibc-proto we've discussed ditching prost-types and generating the google.protobuf protos directly from the protobuf schema using the same conventions as everything else, which among other things allows integrations with pbjson for building JSON serializers /cc @romac

See discussion here: cosmos/cosmos-rust#459 (review)

@romac
Copy link
Member

romac commented Jul 17, 2024

The core problem seems to be that crate::google::protobuf::Duration (i.e. prost_types::Duration) is no longer a Copy type, which means no struct that contains it can be a Copy type any more.

I'm not sure why this change was made. It's a simple struct with two public fields which are themselves Copy types (i64 and i32)

prost_types::Duration still implements Copy in prost-types 0.13.1: https://docs.rs/prost-types/latest/prost_types/struct.Duration.html#impl-Copy-for-Duration

Sidebar: in cosmos-sdk-proto and ibc-proto we've discussed ditching prost-types and generating the google.protobuf protos directly from the protobuf schema using the same conventions as everything else, which among other things allows integrations with pbjson for building JSON serializers /cc @romac

See discussion here: cosmos/cosmos-rust#459 (review)

That's already what we are already doing in tendermint-rs, see the generated code here: https://github.com/informalsystems/tendermint-rs/blob/main/proto/src/protobuf.rs

These types never implemented Copy in the first place, but we can add it manually. I can take care of that.

@romac
Copy link
Member

romac commented Jul 17, 2024

We should probably generate code for all google.protobuf types (ie. including Any) instead of copy-pasting a subset of it, and then use those directly from ibc-proto-rs instead of the latter having its own copy of them.

Otherwise one cannot pass a tendermint_proto Duration where an ibc-proto Duration is expected and vice-versa, same for the other well-known types.

What do you think?

@tony-iqlusion
Copy link
Collaborator Author

tony-iqlusion commented Jul 17, 2024

@romac sounds good to me! Seems like the first step towards unifying protos between tendermint-proto, cosmos-sdk-proto, and ibc-proto

I'd just ask to retain Any::{from_msg, to_msg}: https://docs.rs/prost-types/latest/prost_types/struct.Any.html#implementations

@romanc

This comment was marked as resolved.

@romac
Copy link
Member

romac commented Jul 17, 2024

I'd just ask to retain Any::{from_msg, to_msg}: https://docs.rs/prost-types/latest/prost_types/struct.Any.html#implementations

Yes good idea!

Let's do all this in a separate PR.

@romac romac changed the title Bump prost to v0.13; tonic to v0.12 Bump prost to v0.13 and tonic to v0.12 Jul 17, 2024
@romac romac changed the title Bump prost to v0.13 and tonic to v0.12 tendermint-proto: Bump prost to v0.13 and tonic to v0.12 Jul 17, 2024
@tony-iqlusion
Copy link
Collaborator Author

@romac I didn't put tendermint-proto on the front initially because the changes impact multiple crates

@romac romac changed the title tendermint-proto: Bump prost to v0.13 and tonic to v0.12 Bump prost to v0.13 and tonic to v0.12 Jul 17, 2024
@romac romac merged commit 321af79 into main Jul 17, 2024
23 checks passed
@romac romac deleted the prost-0.13+tonic-v0.12 branch July 17, 2024 13:50
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.

3 participants