proto: Add Any Protobuf type and improve Duration and Timestamp Protobuf types#1452
proto: Add Any Protobuf type and improve Duration and Timestamp Protobuf types#1452
Any Protobuf type and improve Duration and Timestamp Protobuf types#1452Conversation
|
@tony-iqlusion Looking forward to hearing what you think :) |
tony-iqlusion
left a comment
There was a problem hiding this comment.
This looks good to me
@Peartes can you take a pass at this in a follow-up PR? |
Any, Duration and TimestampAny Protobuf type and improve Duration and Timestamp Protobuf types
…nSchema` on well-known types
…tobuf::Timestamp`, feature-guarded by an `std` feature
I have taken care of it, as well as derivation of impls for |
|
Here is the draft integration PR for ibc-proto: cosmos/ibc-proto-rs#226 |
Farhad-Shabani
left a comment
There was a problem hiding this comment.
Thank you @romac. Looks good from ibc-rs end as well.
Since ibc-proto-rs and tendermint-proto get hormonized, this also resolves an existing bump in ibc-rs. This PR probably closes #1053 as well?
|
Yes once merged it will resolve that as well. |
|
@romac it looks like both |
|
|
||
| [features] | ||
| default = [] | ||
| std = [] |
There was a problem hiding this comment.
I'm getting a bunch of build failures trying to use this feature:
$ cargo test --all-features
Compiling tendermint-proto v0.39.0
error[E0433]: failed to resolve: use of undeclared crate or module `std`
--> /Users/tarcieri/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tendermint-proto-0.39.0/src/google/protobuf/timestamp.rs:105:11
(repeat several dozen instances of that error re: std)
I think you forgot an extern crate std and it seems like CI probably isn't testing it.
| } | ||
|
|
||
| impl TryFrom<Duration> for core::time::Duration { | ||
| type Error = core::time::Duration; |
There was a problem hiding this comment.
Hmm, this previously returned DurationError:
https://docs.rs/prost-types/0.13.1/src/prost_types/duration.rs.html#86-104
impl TryFrom<Duration> for time::Duration {
type Error = DurationError;
/// Converts a `Duration` to a `std::time::Duration`, failing if the duration is negative.
fn try_from(mut duration: Duration) -> Result<time::Duration, DurationError> {
duration.normalize();
if duration.seconds >= 0 && duration.nanos >= 0 {
Ok(time::Duration::new(
duration.seconds as u64,
duration.nanos as u32,
))
} else {
Err(DurationError::NegativeDuration(time::Duration::new(
(-duration.seconds) as u64,
(-duration.nanos) as u32,
)))
}
}
}There was a problem hiding this comment.
Good catch, will fix it! As well as replace the From<core::time::Duration> instance with a TryFrom<core::time::Duration> following prost-types's implementation instead of pbjson-types which I lifted those from.
Now that #1452 has landed `prost-types` isn't used. This removes it as a dependency.
* proto: remove `prost-types` dependency Now that #1452 has landed `prost-types` isn't used. This removes it as a dependency. * Add changelog entry --------- Co-authored-by: Romain Ruetschi <romain@informal.systems>
Port-of: cometbft/tendermint-rs#1452 Closes #92
Port-of: cometbft/tendermint-rs#1452 Closes #92
Closes: #1445
Notes
Any,Duration, andTimestampfor now, but we may add other well-known types if needed.prost::Nameimpl, as requested.Any::from_msgandAny::to_msgare provided.pbjson-buildas it brings a dependency onchronowhereas we are usingtimeat the moment. Also, there only a few protos and we can just inline the serde implementation generated bypbjson-buildor write our own manually. I am happy to revisit this if deemed necessary.Anyis not compliant with the ProtoJSON spec because it is currently impossible to do so without knowledge of all registered types that may be serialized as anAny. See the correspondingpbjsonissue for more info. Instead, we currently use the same serialization format aspbjson.Important
Before we merge this, we need to try out integration in ibc-proto-rs, cosmos-rust, ibc-rs, and Hermes.
.changelog/