imp!: define helper trait for Timestamp conversions + update ConsensusState::timestamp() to return Result
#1353
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1353 +/- ##
==========================================
+ Coverage 66.76% 66.85% +0.08%
==========================================
Files 225 225
Lines 22614 22628 +14
==========================================
+ Hits 15098 15127 +29
+ Misses 7516 7501 -15 ☔ View full report in Codecov by Sentry. |
| /// Utility trait for converting a `Timestamp` into a host-specific time format. | ||
| pub trait IntoHostTime<T: Sized> { | ||
| /// Converts a `Timestamp` into another time representation of type `T`. | ||
| /// | ||
| /// This method adapts the `Timestamp` to a domain-specific format, which | ||
| /// could represent a custom timestamp used by a light client, or any | ||
| /// hosting environment that requires its own time format. | ||
| fn into_host_time(self) -> Result<T, TimestampError>; | ||
| } | ||
|
|
||
| /// Utility trait for converting an arbitrary host-specific time format into a | ||
| /// `Timestamp`. | ||
| pub trait IntoTimestamp { | ||
| /// Converts a time representation of type `T` back into a `Timestamp`. | ||
| /// | ||
| /// This can be used to convert from custom light client or host time | ||
| /// formats back into the standard `Timestamp` format. | ||
| fn into_timestamp(self) -> Result<Timestamp, TimestampError>; | ||
| } | ||
|
|
||
| impl<T: TryFrom<OffsetDateTime>> IntoHostTime<T> for Timestamp { | ||
| fn into_host_time(self) -> Result<T, TimestampError> { | ||
| T::try_from(self.into()).map_err(|_| TimestampError::InvalidDate) | ||
| } | ||
| } | ||
|
|
||
| fn try_from(tm_time: Time) -> Result<Self, Self::Error> { | ||
| let odt: OffsetDateTime = tm_time.into(); | ||
| odt.try_into() | ||
| impl<T: Into<OffsetDateTime>> IntoTimestamp for T { | ||
| fn into_timestamp(self) -> Result<Timestamp, TimestampError> { | ||
| Timestamp::try_from(self.into()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Am I correct to understand, these helper traits with auto-impls only reduce extra function calls and imports ?
There was a problem hiding this comment.
That’s one benefit. But more importantly it removes tendermint dependency in ibc_primitives. This fits with the plan to completely decouple the ibc-rs core stack from the tendermint.
It also makes things easier for light client developers or integrators. By implementing these traits, they can smoother convert their own time type to Timestamp wherever ibc-rs asks for, or reconstruct their types from ibc-rs’s Timestamp if needed.
There was a problem hiding this comment.
I meant whether these traits are just syntactic sugars. Because, the developers could also just use T::try_from(OffsetDateTime::from(ibc_timestamp)) instead of ibc_timestamp.into_host_time() and Timestamp::try_from(OffsetDateTime::from(host_timestamp)) instead of host_timestamp.into_timestamp().
There was a problem hiding this comment.
Ah, I get what you mean now. It’s not quite that way.
We can't really assume the structure of a time type across different hosts or what conversions they use. They might not even be using OffsetDateTime. So, I added default implementations for any time type that follows this conversion path, just for convenience. As it's the case with tendermint::Time. But this won’t necessarily apply to other timestamp types in different ConsensusState structs.
There was a problem hiding this comment.
Hmm. I am trying to understand, why would user need these two helper traits?
It seems to me, for a struct HostTime, they can just implement impl TryFrom<Timestamp> for HostTime and impl TryFrom<HostTime> for Timestamp.
We can just use the trait bounds as TryFrom<Timestamp> and TryFrom<HostTime> appropriately in ibc-rs.
There was a problem hiding this comment.
Basically, IntoHostTime<T> is equivalent to TryInto<T> and IntoTimestamp is equivalent to TryInto<Timestamp>.
There was a problem hiding this comment.
Basically,
IntoHostTime<T>is equivalent toTryInto<T>andIntoTimestampis equivalent toTryInto<Timestamp>.
Totally, this works too. But if a host has multiple TryFrom<HostTime> implementations, which usually can be the case, the consumer would need to specify the exact type and use the full path. Can’t simply call try_from or try_into on the time object. Plus, as you mentioned, this would definitely benefit from being syntactic sugar, making it more readable and easier to work with.
There was a problem hiding this comment.
Yea. I am ok with this. Thanks, Farhad, for explaining.
Closes: #1352
Closes: #1323
PR author checklist:
unclog.docs/).Reviewer checklist:
Files changedin the GitHub PR explorer.