diff --git a/.changelog/unreleased/breaking-changes/1030-remove-chrono.md b/.changelog/unreleased/breaking-changes/1030-remove-chrono.md new file mode 100644 index 000000000..7d8385ee5 --- /dev/null +++ b/.changelog/unreleased/breaking-changes/1030-remove-chrono.md @@ -0,0 +1,11 @@ +- `[tendermint]` Reform `tendermint::Time` + ([#1030](https://github.com/informalsystems/tendermint-rs/issues/1030)): + * The struct content is made private. + * The range of acceptable values is restricted to years 1-9999 + (as reckoned in UTC). + * Removed conversions from/to `chrono::DateTime`. + * Changes in error variants: removed `TimestampOverflow`, replaced with + `TimestampNanosOutOfRange`; removed `ChronoParse`, replaced with `TimeParse`. +- `[tendermint-rpc]` Use `OffsetDateTime` and `Date` types provided by the `time` crate + in query operands instead of their `chrono` counterparts. + ([#1030](https://github.com/informalsystems/tendermint-rs/issues/1030)) diff --git a/.changelog/unreleased/improvements/1030-new-time-api.md b/.changelog/unreleased/improvements/1030-new-time-api.md new file mode 100644 index 000000000..a9c269f0d --- /dev/null +++ b/.changelog/unreleased/improvements/1030-new-time-api.md @@ -0,0 +1,11 @@ +- Remove dependencies on the `chrono` crate. + ([#1030](https://github.com/informalsystems/tendermint-rs/issues/1030)) +- `[tendermint]` Improve `tendermint::Time` + ([#1030](https://github.com/informalsystems/tendermint-rs/issues/1030)): + * Restrict the validity range of `Time` to dates with years in the range + 1-9999, to match the specification of protobuf message `Timestamp`. + Add an `ErrorDetail` variant `DateOutOfRange` to report when this + restriction is not met. + * Added a conversion to, and a fallible conversion from, + `OffsetDateTime` of the `time` crate. + * Added `Time` methods `checked_add` and `checked_sub`. diff --git a/abci/src/codec.rs b/abci/src/codec.rs index 8e80736dd..d9fd649c6 100644 --- a/abci/src/codec.rs +++ b/abci/src/codec.rs @@ -146,7 +146,7 @@ where Ok(len) => len, // We've potentially only received a partial length delimiter Err(_) if src_len <= MAX_VARINT_LENGTH => return Ok(None), - //Err(e) => return Err(Error::decode(e)), + // Err(e) => return Err(Error::decode(e)), Err(e) => return Err(e), }; let remaining = tmp.remaining() as u64; diff --git a/proto/Cargo.toml b/proto/Cargo.toml index afd8e8023..aab957435 100644 --- a/proto/Cargo.toml +++ b/proto/Cargo.toml @@ -28,8 +28,6 @@ num-derive = { version = "0.3", default-features = false } # TODO(thane): Remove restrictions once js-sys issue is resolved (causes the Substrate no_std check to fail) time = { version = ">=0.3, <0.3.12", default-features = false, features = ["macros", "parsing"] } flex-error = { version = "0.4.4", default-features = false } -# TODO(hdevalence): re-added while rebasing the ABCI domain types PR again -chrono = { version = "0.4", default-features = false, features = ["serde", "alloc"] } [dev-dependencies] serde_json = { version = "1.0", default-features = false, features = ["alloc"] } diff --git a/proto/src/chrono.rs b/proto/src/chrono.rs deleted file mode 100644 index 3b969f0f0..000000000 --- a/proto/src/chrono.rs +++ /dev/null @@ -1,53 +0,0 @@ -use core::convert::TryInto; - -use chrono::{DateTime, Duration, TimeZone, Utc}; - -use crate::google::protobuf as pb; - -impl From> for pb::Timestamp { - fn from(dt: DateTime) -> pb::Timestamp { - pb::Timestamp { - seconds: dt.timestamp(), - // This can exceed 1_000_000_000 in the case of a leap second, but - // even with a leap second it should be under 2_147_483_647. - nanos: dt - .timestamp_subsec_nanos() - .try_into() - .expect("timestamp_subsec_nanos bigger than i32::MAX"), - } - } -} - -impl From for DateTime { - fn from(ts: pb::Timestamp) -> DateTime { - Utc.timestamp(ts.seconds, ts.nanos as u32) - } -} - -// Note: we convert a protobuf::Duration into a chrono::Duration, not a -// std::time::Duration, because std::time::Durations are unsigned, but the -// protobuf duration is signed. - -impl From for pb::Duration { - fn from(d: Duration) -> pb::Duration { - // chrono's Duration stores the fractional part as `nanos: i32` - // internally but doesn't provide a way to access it, only a way to get - // the *total* number of nanoseconds. so we have to do this cool and fun - // hoop-jumping maneuver - let seconds = d.num_seconds(); - let nanos = (d - Duration::seconds(seconds)) - .num_nanoseconds() - .expect("we computed the fractional part, so there's no overflow") - .try_into() - .expect("the fractional part fits in i32"); - - pb::Duration { seconds, nanos } - } -} - -impl From for Duration { - fn from(d: pb::Duration) -> Duration { - // there's no constructor that supplies both at once - Duration::seconds(d.seconds) + Duration::nanoseconds(d.nanos as i64) - } -} diff --git a/proto/src/lib.rs b/proto/src/lib.rs index 312403de6..49e41fde6 100644 --- a/proto/src/lib.rs +++ b/proto/src/lib.rs @@ -19,7 +19,6 @@ pub mod google { } } -mod chrono; mod error; #[allow(warnings)] mod tendermint; diff --git a/tendermint/Cargo.toml b/tendermint/Cargo.toml index cc50f5cbe..cb4fe8046 100644 --- a/tendermint/Cargo.toml +++ b/tendermint/Cargo.toml @@ -31,7 +31,7 @@ rustdoc-args = ["--cfg", "docsrs"] [dependencies] async-trait = { version = "0.1", default-features = false } -bytes = { version = "1.0", default-features = false } +bytes = { version = "1.0", default-features = false, features = ["serde"] } ed25519 = { version = "1.3", default-features = false } ed25519-dalek = { version = "1", default-features = false, features = ["u64_backend"] } futures = { version = "0.3", default-features = false } @@ -54,8 +54,6 @@ zeroize = { version = "1.1", default-features = false, features = ["zeroize_deri flex-error = { version = "0.4.4", default-features = false } k256 = { version = "0.11", optional = true, default-features = false, features = ["ecdsa", "sha256"] } ripemd160 = { version = "0.9", default-features = false, optional = true } -# TODO(hdevalence): re-added while rebasing the ABCI domain types PR again -chrono = { version = "0.4", default-features = false, features = ["serde", "alloc"] } [features] default = ["std"] diff --git a/tendermint/src/abci/request/init_chain.rs b/tendermint/src/abci/request/init_chain.rs index 8072c2f6e..bb50b553f 100644 --- a/tendermint/src/abci/request/init_chain.rs +++ b/tendermint/src/abci/request/init_chain.rs @@ -1,8 +1,7 @@ use bytes::Bytes; -use chrono::{DateTime, Utc}; use super::super::types::ValidatorUpdate; -use crate::{block, consensus, prelude::*}; +use crate::{block, consensus, prelude::*, Time}; /// Called on genesis to initialize chain state. /// @@ -10,7 +9,7 @@ use crate::{block, consensus, prelude::*}; #[derive(Clone, PartialEq, Eq, Debug)] pub struct InitChain { /// The genesis time. - pub time: DateTime, + pub time: Time, /// The ID of the blockchain. pub chain_id: String, /// Initial consensus-critical parameters. diff --git a/tendermint/src/abci/types.rs b/tendermint/src/abci/types.rs index 4df1a95e7..54344d692 100644 --- a/tendermint/src/abci/types.rs +++ b/tendermint/src/abci/types.rs @@ -8,9 +8,8 @@ use core::convert::{TryFrom, TryInto}; use bytes::Bytes; -use chrono::{DateTime, Utc}; -use crate::{block, prelude::*, vote, Error, PublicKey}; +use crate::{block, prelude::*, vote, Error, PublicKey, Time}; /// A validator address with voting power. /// @@ -80,7 +79,7 @@ pub struct Evidence { /// The height when the offense occurred. pub height: block::Height, /// The corresponding time when the offense occurred. - pub time: DateTime, + pub time: Time, /// Total voting power of the validator set at `height`. /// /// This is included in case the ABCI application does not store historical @@ -246,7 +245,10 @@ impl TryFrom for Evidence { .ok_or_else(Error::missing_validator)? .try_into()?, height: evidence.height.try_into()?, - time: evidence.time.ok_or_else(Error::missing_timestamp)?.into(), + time: evidence + .time + .ok_or_else(Error::missing_timestamp)? + .try_into()?, total_voting_power: evidence.total_voting_power.try_into()?, }) } diff --git a/tendermint/src/time.rs b/tendermint/src/time.rs index 00114efb0..0cb6e330f 100644 --- a/tendermint/src/time.rs +++ b/tendermint/src/time.rs @@ -34,7 +34,7 @@ use crate::{error::Error, prelude::*}; // For memory efficiency, the inner member is `PrimitiveDateTime`, with assumed // UTC offset. The `assume_utc` method is used to get the operational // `OffsetDateTime` value. -#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Hash, Serialize, Deserialize)] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Serialize, Deserialize)] #[serde(try_from = "Timestamp", into = "Timestamp")] pub struct Time(PrimitiveDateTime); @@ -117,6 +117,20 @@ impl Time { pub fn to_rfc3339(&self) -> String { timestamp::to_rfc3339_nanos(self.0.assume_utc()) } + + /// Computes `self + duration`, returning `None` if an overflow occurred. + pub fn checked_add(self, duration: Duration) -> Option { + let duration = duration.try_into().ok()?; + let t = self.0.checked_add(duration)?; + Self::from_utc(t.assume_utc()).ok() + } + + /// Computes `self - duration`, returning `None` if an overflow occurred. + pub fn checked_sub(self, duration: Duration) -> Option { + let duration = duration.try_into().ok()?; + let t = self.0.checked_sub(duration)?; + Self::from_utc(t.assume_utc()).ok() + } } impl fmt::Display for Time { @@ -151,19 +165,12 @@ impl Add for Time { type Output = Result; fn add(self, rhs: Duration) -> Self::Output { - // Work around not being able to depend on time 0.3.5 - // https://github.com/informalsystems/tendermint-rs/issues/1047 - let lhs_nanos = self.0.assume_utc().unix_timestamp_nanos(); - let rhs_nanos: i128 = rhs - .as_nanos() - .try_into() - .map_err(|_| Error::duration_out_of_range())?; - let res_nanos = lhs_nanos - .checked_add(rhs_nanos) + let duration = rhs.try_into().map_err(|_| Error::duration_out_of_range())?; + let t = self + .0 + .checked_add(duration) .ok_or_else(Error::duration_out_of_range)?; - let t = OffsetDateTime::from_unix_timestamp_nanos(res_nanos) - .map_err(|_| Error::duration_out_of_range())?; - Self::from_utc(t) + Self::from_utc(t.assume_utc()) } } @@ -171,19 +178,12 @@ impl Sub for Time { type Output = Result; fn sub(self, rhs: Duration) -> Self::Output { - // Work around not being able to depend on time 0.3.5 - // https://github.com/informalsystems/tendermint-rs/issues/1047 - let lhs_nanos = self.0.assume_utc().unix_timestamp_nanos(); - let rhs_nanos: i128 = rhs - .as_nanos() - .try_into() - .map_err(|_| Error::duration_out_of_range())?; - let res_nanos = lhs_nanos - .checked_sub(rhs_nanos) + let duration = rhs.try_into().map_err(|_| Error::duration_out_of_range())?; + let t = self + .0 + .checked_sub(duration) .ok_or_else(Error::duration_out_of_range)?; - let t = OffsetDateTime::from_unix_timestamp_nanos(res_nanos) - .map_err(|_| Error::duration_out_of_range())?; - Self::from_utc(t) + Self::from_utc(t.assume_utc()) } } @@ -376,31 +376,31 @@ mod tests { proptest! { #[test] - fn add_regular((dt, d) in args_for_regular_add()) { + fn checked_add_regular((dt, d) in args_for_regular_add()) { let t: Time = dt.try_into().unwrap(); - let t = (t + d).unwrap(); + let t = t.checked_add(d).unwrap(); let res: OffsetDateTime = t.into(); assert_eq!(res, dt + d); } #[test] - fn sub_regular((dt, d) in args_for_regular_sub()) { + fn checked_sub_regular((dt, d) in args_for_regular_sub()) { let t: Time = dt.try_into().unwrap(); - let t = (t - d).unwrap(); + let t = t.checked_sub(d).unwrap(); let res: OffsetDateTime = t.into(); assert_eq!(res, dt - d); } #[test] - fn add_overflow((dt, d) in args_for_overflowed_add()) { + fn checked_add_overflow((dt, d) in args_for_overflowed_add()) { let t: Time = dt.try_into().unwrap(); - assert!((t + d).is_err()); + assert_eq!(t.checked_add(d), None); } #[test] - fn sub_overflow((dt, d) in args_for_overflowed_sub()) { + fn checked_sub_overflow((dt, d) in args_for_overflowed_sub()) { let t: Time = dt.try_into().unwrap(); - assert!((t - d).is_err()); + assert_eq!(t.checked_sub(d), None); } } }