Create a separate type for absolute time + some related fixes and refactoring #1235
Conversation
TheQuantumPhysicist
requested review from
muursh,
erubboli,
iljakuklic and
azarovh
as code owners
| pub const fn saturating_duration_add(&self, duration: Duration) -> Self { | ||
| Self { | ||
| time: self.time.saturating_add(duration), | ||
| } | ||
| } | ||
|
|
||
| pub const fn saturating_duration_sub(&self, t: Duration) -> Self { | ||
| Self { | ||
| time: self.time.saturating_sub(t), | ||
| } | ||
| } |
There was a problem hiding this comment.
I'd suggest to name these methods just saturating_add and saturating_sub, so that Duration is implied as the type of the operand, because it's implied in operator + and for consistency with standard types like SystemTime.
Instead, I'd rename the current saturating_sub to saturating_diff.
There was a problem hiding this comment.
This also occurred to me, and I'm not insistent about it. My only issue with it is that usually saturating_sub usually takes Self. Using something other than Self seems unnatural.
There was a problem hiding this comment.
One of the problems you hit when coming up with a name for this is that both Time - Duration (returning Time) and Time - Time (returning Duration or something similar) are sensible operations. Plain saturating_sub should probably be reserved for the latter, even if it's never implemented.
There was a problem hiding this comment.
That's pretty much my thought process... saturating_sub should be reserved for Time - Time. It's kind of the norm in rust.
| impl std::ops::Add<Duration> for Time { | ||
| type Output = Option<Self>; | ||
|
|
||
| fn add(self, other: Duration) -> Option<Self> { | ||
| self.time.checked_add(other).map(|time| Self { time }) | ||
| } | ||
| } |
There was a problem hiding this comment.
I'd suggest to make Add implementation panic, just like it does for Duration and SystemTime.
For returning an Option, I'd add a separate method checked_add.
Same for Sub (also, taking into account the suggestion above, we could have both checked_sub for subtracting durations and checked_diff for subtracting times).
There was a problem hiding this comment.
Please don't take this the wrong way... but... the irony... comparing this to issue #1221... 😄
Having said that: I do prefer enforcing checked math wherever possible... especially when it's global like this one.
I do understand there are many expects, but I'm OK with that as long as it's fully controlled. In fact, for uint, we're moving to further checked_add by default everywhere.
There was a problem hiding this comment.
As you say, but I'd argue that this comment and the mentioned issue have nothing in common.
Here we talking about whether to panic implicitly or explicitly. IMO for types like Duration implicit panics are OK and since with explicit ones we lose convenience (quite a lot of it IMO), I'd go with the former. But I can see your point too.
In the issue though, the question is whether we should panic in certain situations or not.
| pub const fn from_time(time: Time) -> Self { | ||
| Self::from_duration_since_epoch(time.as_duration_since_epoch()) | ||
| } | ||
|
|
||
| pub const fn into_time(self) -> Time { | ||
| Time::from_duration_since_epoch(self.as_duration_since_epoch()) | ||
| } | ||
|
|
There was a problem hiding this comment.
I wonder whether we should have impl From<BlockTimestamp> for Time and impl From<Time> for BlockTimestamp instead of these functions (or in addition to them).
Some of the conversions would become less verbose.
There was a problem hiding this comment.
I thought about that, but the reason I didn't go for it was that Time and BlockTimestamp don't necessarily have 1:1 equivalence. In fact, I was thinking of calling the function in BlockTimestamp "approximate_from_time()".
| let max_block_timestamp = BlockTimestamp::from_time(max_accepted_time); | ||
| if block_timestamp > max_block_timestamp { | ||
| let block_timestamp = block_timestamp.as_duration_since_epoch(); | ||
| let max_block_timestamp = max_block_timestamp.as_duration_since_epoch(); | ||
|
|
||
| let clock_diff = | ||
| block_timestamp.checked_sub(max_block_timestamp).unwrap_or_else(|| { |
There was a problem hiding this comment.
I'd suggest to implement checked subtraction for Time (which I'd call checked_diff) and use it here directly instead of converting times to durations.
Same for the V2 peer.
| next_time_resend_own_address = (next_time_resend_own_address + delay) | ||
| .expect("Time derived from local clock; cannot fail"); |
There was a problem hiding this comment.
Again, AddAssign would be nice to have.
There was a problem hiding this comment.
Doesn't work with Option<_>
| @@ -77,11 +77,11 @@ fn reachable_reconnects(#[case] seed: Seed) { | |||
| address.transition_to(AddressStateTransitionTo::ConnectionFailed, now, &mut rng); | |||
| connection_attempts += 1; | |||
| } | |||
| now += Duration::from_secs(60); | |||
| now = (now + Duration::from_secs(60)).unwrap(); | |||
There was a problem hiding this comment.
Same comment about AddAssign
| /// Table for banned addresses | ||
| /// Table for banned addresses vs when they can be unbanned (Duration is timestamp since UNIX Epoch) | ||
| pub DBBannedAddresses: Map<String, Duration>, |
There was a problem hiding this comment.
Seems like Duration is not quite appropriate here (but not sure Time is).
There was a problem hiding this comment.
Time cannot be be encoded, so Duration is just something that can be encoded. It's an implementation detail. It's converted to Time when loaded.
Please check the code relevant to your work and see if it makes sense.
There's a test broken in block production because it used the wrong time source and modified the global mocked time. That's gonna be fixed in this PR before merging it.