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

rfc3339 direct ser/deser fix for protobuf Timestamp #666

Merged
merged 4 commits into from
Nov 10, 2020

Conversation

greg-szabo
Copy link
Member

@greg-szabo greg-szabo commented Nov 9, 2020

A fix for an issue that bubbled up during #665 .

The Time domain type was incorrectly serialized by itself. (It was correctly serialized as part of another struct.)

This change implements correct serialization for tendermint::Time via tendermint_proto::Timestamp.
Timestamp needed a helper struct so it could be serialized/deserialized without custom serializers.
You can clearly see the dependencies on serializers in this change:

  • The serializer to correctly ser/deser an RFC3339 string into a Timestamp is moved into tendermint_proto::serializers::timestamp.

  • The Timestamp struct needs a helper struct so it can use those serializers. Otherwise Timestamp would have required a custom serializer.

  • With the Timestamp struct directly serializable, there's no need for special serialization for Option<Timestamp>. All structs having that field can now be serialized by just defining serialization over Option<> with the tendermint_proto::serializers::optional module.

  • Unfortunately, the tendermint::Time struct is directly serialized/deserialized in testgen structs (and possibly in IBC) in several places. But now that it is directly serializable through Timestamp (and from an RFC3339 string), there is no need for extra annotation when serializing/deserializing it. It's going to be interesting if/when someone comes to us that they need it serialized differently (in the {seconds:x,nanos:x} format, isntead of RFC3339).

  • Referenced an issue explaining the need for the change

  • Updated all relevant documentation in docs

  • Updated all code comments where relevant

  • Wrote tests

  • Updated CHANGELOG.md

romac
romac previously approved these changes Nov 10, 2020
Copy link
Member

@romac romac left a comment

Choose a reason for hiding this comment

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

I like the new Rfc3339 struct :)

Just suggested a whitespace change in the README, but otherwise LGTM :)

CHANGELOG.md Outdated Show resolved Hide resolved
@romac romac self-requested a review November 10, 2020 09:55
@andrey-kuprianov
Copy link
Contributor

@greg-szabo What I especially like about this PR is that it removes more code than it adds -- it simplifies things instead of complicating them 💯

@andrey-kuprianov andrey-kuprianov merged commit 9b6239d into master Nov 10, 2020
@andrey-kuprianov andrey-kuprianov deleted the greg/time-serde branch November 10, 2020 10:08
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