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

proto: Add Any Protobuf type and improve Duration and Timestamp Protobuf types #1452

Merged
merged 9 commits into from
Aug 6, 2024

Conversation

romac
Copy link
Member

@romac romac commented Jul 30, 2024

Closes: #1445

Notes

  • This PR only includes Any, Duration, and Timestamp for now, but we may add other well-known types if needed.
  • All three types come with a prost::Name impl, as requested.
  • Any::from_msg and Any::to_msg are provided.
  • I chose not to go with pbjson-build as it brings a dependency on chrono whereas we are using time at the moment. Also, there only a few protos and we can just inline the serde implementation generated by pbjson-build or write our own manually. I am happy to revisit this if deemed necessary.
  • JSON serialization for Any is 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 an Any. See the corresponding pbjson issue for more info. Instead, we currently use the same serialization format as pbjson.

Important

Before we merge this, we need to try out integration in ibc-proto-rs, cosmos-rust, ibc-rs, and Hermes.


  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Added entry in .changelog/

@romac romac requested a review from tony-iqlusion July 30, 2024 08:20
@romac
Copy link
Member Author

romac commented Jul 30, 2024

@tony-iqlusion Looking forward to hearing what you think :)

Copy link
Collaborator

@tony-iqlusion tony-iqlusion left a comment

Choose a reason for hiding this comment

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

This looks good to me

@ash-burnt
Copy link

Unlike prost_types::Timestamp, our Timestamp struct does not provide a way to construct it from a std::time::SystemTime, is this something we want to add?

@Peartes can you take a pass at this in a follow-up PR?

@romac romac changed the title proto: Expose well-known types Any, Duration and Timestamp proto: Add Any Protobuf type and improve Duration and Timestamp Protobuf types Jul 31, 2024
@romac romac marked this pull request as ready for review July 31, 2024 08:54
@romac
Copy link
Member Author

romac commented Jul 31, 2024

Unlike prost_types::Timestamp, our Timestamp struct does not provide a way to construct it from a std::time::SystemTime, is this something we want to add?

@Peartes can you take a pass at this in a follow-up PR?

I have taken care of it, as well as derivation of impls for schemars, borsh and parity-scale-codec.

@romac
Copy link
Member Author

romac commented Jul 31, 2024

Here is the draft integration PR for ibc-proto: cosmos/ibc-proto-rs#226

Copy link
Member

@Farhad-Shabani Farhad-Shabani left a comment

Choose a reason for hiding this comment

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

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?

@romac
Copy link
Member Author

romac commented Jul 31, 2024

Yes once merged it will resolve that as well.

@romac romac merged commit dd33030 into main Aug 6, 2024
25 checks passed
@romac romac deleted the romac/protos branch August 6, 2024 08:17
@tony-iqlusion
Copy link
Collaborator

@romac it looks like both tendermint and tendermint-proto still include prost-types in their Cargo.toml

@@ -15,8 +15,12 @@ description = """

[features]
default = []
std = []
Copy link
Collaborator

@tony-iqlusion tony-iqlusion Aug 7, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Opened an issue to track this: #1454

}

impl TryFrom<Duration> for core::time::Duration {
type Error = core::time::Duration;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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,
            )))
        }
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

tony-iqlusion added a commit that referenced this pull request Aug 8, 2024
Now that #1452 has landed `prost-types` isn't used.

This removes it as a dependency.
romac added a commit that referenced this pull request Aug 9, 2024
* 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>
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.

Generate code for all google.protobuf types
4 participants