-
Notifications
You must be signed in to change notification settings - Fork 114
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
Remove IBC protos in favour of the ibc-proto
crate
#459
base: main
Are you sure you want to change the base?
Conversation
prost = "0.12" | ||
prost-types = "0.12" | ||
prost-wkt-types = "0.5" |
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.
It seems like this crate requires protoc
?
error: failed to run custom build command for `prost-wkt-types v0.5.0`
Caused by:
process didn't exit successfully: `/home/runner/work/cosmos-rust/cosmos-rust/target/release/build/prost-wkt-types-d2782fb67e78a862/build-script-build` (exit status: 101)
--- stderr
thread 'main' panicked at 'Could not find `protoc` installation and this build crate cannot proceed without
this knowledge. If `protoc` is installed and this crate had trouble finding
it, you can set the `PROTOC` environment variable with the specific path to your
installed `protoc` binary.If you're on debian, try `apt-get install protobuf-compiler` or download it from https://github.com/protocolbuffers/protobuf/releases
As things currently stand, both tendermint-proto
and cosmos-sdk-proto
pregenerate all the protos so downstream users don't need protoc
installed in order to be able to use the crates.
So changing this would make protoc
a hard dependency where it wasn't before.
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.
Ah my bad, I should have realized that and asked if it was okay. I actually don't see a good reason for moving to prost-wkt-types
right now so I will revert that change so that we can keep pre-generating all the protos.
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.
Oh now I remember: Because some Protobuf messages contain Any
, Duration
or Timestamp
, we need those so-called "well-known" types to derive Serialize
in order to derive Serialize
for the enclosing message via pbjson
. Unfortunately, prost-types
does not provide such an impl.
Potential solutions I see:
- a) Use
prost-wkt-types
instead ofprost-types
and incur the cost of downstream users having to installprotoc
- b) Use
pbjson-types
instead ofprost-types
- c) Switch between
prost-types
andpbjson-types
at compile time based on whether theserde
feature is enabled or not
Would one of those be acceptable or do you see another solution?
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.
- d) Instruct
prost-build
to generate Rust definitions for thegoogle.protobuf
messages (ie. those "well-known types") instead of forwarding them toprost-types
or another crate and letpbjson-build
derive theserde
impls for those.
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.
Do any of those have an integration with the prost::Name
trait like prost_types::Any::{from_msg, to_msg}
do? We use that a lot in cosmrs
: https://docs.rs/prost-types/latest/prost_types/struct.Any.html#impl-Any
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.
I suppose if we generated our own Rust definitions for the google.protobuf
messages, we could add from_msg
/to_msg
just like prost-types
does.
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.
@romac perhaps to break this PR up into more manageable chunks, you could PR just the change to generate our own google.protobuf.*
protos which can be augmented with pbjson-build
serializers? (and potentially other changes to proto-build
as needed)
Closes: #458
Closes: #442
Closes: #83
As promised, here is my follow-up to #458 and its corresponding issue in the ibc-proto-rs repository.
Here is the corresponding PR in
ibc-proto
which removes the generated Cosmos SDK protos and uses them viacosmos-sdk-proto
instead: cosmos/ibc-proto-rs#187Changes
ics23
crate instead of bundling them incosmos-sdk-proto
pbjson
.TODO
ics23
v0.11.1 once released (needed forinformalsystems-pbjson
v0.7.0)informalsystems-pbjson
to v0.7.0 ics23#274tendermint-proto
v0.34.1 once released (needed for missing serde annotations on some protos)Questions
serde
to match the corresponding feature inibc-proto
, but perhapspbjson
orprotojson
is a better name?This PR is probably best reviewed commit by commit.
@tony-iqlusion This is by no mean the definitive way to go about deduplicating the work, so I am eager to hear what you think about this and to discuss further with you how best to proceeded.