Update to tendermint =0.23.2 and remove chrono #1665
Conversation
Update to the related changes in tendermint API.
mzabaluev
force-pushed
the
mikhail/tendermint-0.23.2-and-remove-chrono
branch
from
56a2cda to
fcea486
Compare
Got some clarification from Soares about this.
| pub fn as_datetime(&self) -> Option<DateTime<Utc>> { | ||
| self.time | ||
| /// Convert a `Timestamp` to an optional [`OffsetDateTime`] | ||
| pub fn into_datetime(self) -> Option<OffsetDateTime> { |
There was a problem hiding this comment.
I wonder if we'd be more interested in getting tendermint::Time here?
That would also get rid of a public dependency on time.
There was a problem hiding this comment.
Outside of ics07, we are not supposed to depend on Tendermint-specific code but I guess we can make an exception for the Time struct since it's general purpose?
There was a problem hiding this comment.
In my mad scientist's secret world assimilation plan, we'll migrate to a common domain types crate that will provide idiomatic Rust support for Google's well-known protobuf types, and tendermint::Time will be morphed into one of the types from that yet unpublished crate. Until then, it could be whatever seems most practical at the moment.
| @@ -107,7 +107,7 @@ impl ClientDef for TendermintClient { | |||
| untrusted_state, | |||
| trusted_state, | |||
| &options, | |||
| Time(chrono::Utc::now()), | |||
| OffsetDateTime::now_utc().try_into().unwrap(), | |||
There was a problem hiding this comment.
Should we not perhaps implement a now_utc() method for the Time struct to avoid directly importing dependencies from the time crate? I'd generally prefer to use interfaces that hide their underlying implementations as far as possible, as it reduces entanglement.
There was a problem hiding this comment.
The trouble with now_utc is, it requires std. So I assume it can't be part of Tendermint's core API without an std feature gate, too.
There was a problem hiding this comment.
Yeah I can look into providing that mechanism. It's probably preferable to avoid mixing newtypes and the types they're supposed to wrap.
There was a problem hiding this comment.
We'd still want to provide the TryFrom/Into conversions from/to OffsetDateTime for convenience, and time is not an optional dependency at the moment (though it could be after redoing the internal representation of Time per ADR-010).
* Bump tendermint-rs versions to 0.23.2 * modules: Replaced uses of chrono with time-rs Update to the related changes in tendermint API. * relayer: Removed uses of chrono * no-std-check: Remove chrono
Closes #1639
Description
Update to changes in tendermint-rs 0.23.2 and remove all direct chrono dependencies.
Also renamed or removed
Timestampmethods:as_nanosecondsrenamed tonanosecondswith deprecation of the old convention-breaking name;as_datetimeremoved,into_datetimecomes in its stead.from_datetimeremoved,From<tendermint::Time>remains as a substitute.PR author checklist:
unclog.docs/).Reviewer checklist:
Files changedin the GitHub PR explorer.