Refactor Timestamps #903
Refactor Timestamps #903
Conversation
webmaster128
force-pushed
the
refactor-timestamps
branch
from
3b116f5
to
1c36c59
Compare
| block: IbcTimeoutBlock, | ||
| }, | ||
| } | ||
|
|
||
| impl From<Timestamp> for IbcTimeout { | ||
| fn from(time: Timestamp) -> IbcTimeout { | ||
| IbcTimeout::TimestampNanos(time.seconds * 1_000_000_000 + time.nanos) | ||
| fn from(timestamp: Timestamp) -> IbcTimeout { |
There was a problem hiding this comment.
If we add this, maybe add From<IbcTimeoutBlock> as well to make that less verbose also?
| #[derive(Copy, Clone, Default, Debug, PartialEq, Eq, PartialOrd, Ord, JsonSchema)] | ||
| pub struct Uint128(#[schemars(with = "String")] pub u128); | ||
| pub struct Uint128( | ||
| #[schemars(with = "String")] pub u128, // Simon thinks this should be private, but does not want to worry about breaking code right now |
There was a problem hiding this comment.
We offer .u128() so that should be fine. But no need. Please make an issue to follow up on this before 1.0
| seconds: self.seconds + addition, | ||
| nanos: self.nanos, | ||
| } | ||
| /// Creates a timestamp from nanoseconds since epoch |
There was a problem hiding this comment.
Hmmm... no 2 value constructor?
We must do eg. Timestamp::from_seconds(env.block.time).plus_nanos(env.block.nanos)?
It is a bit verbose, but it also means you won't mix up the two (identically typed) arguments. I guess it is good like that
There was a problem hiding this comment.
The question is where you get a two component time stamp from and of that case really matters. The example you provided is covered by BlockInfo.timestamp() where the calculation is a bit raw but more direct. Otherwise seconds plus nanos works (even for consts!).
| seconds: self.time, | ||
| nanos: self.time_nanos, | ||
| } | ||
| let nanos_since_epoch = self.time * 1_000_000_000 + self.time_nanos; |
There was a problem hiding this comment.
I wouldn't put this math here (with Timestamp internals), but rather:
Timestamp::from_seconds(self.time).plus_nanos(self.nanos) (or another constructor you invent)
There was a problem hiding this comment.
Yeah, right. When I wrote the code plus_nanos did not yet exist.
Anyways, I'll leave it for now and change the type of BlockInfo.time, such that it disappears.
|
As to the follow up questions:
That sounds like a very good idea.
Do you mean the difference between: {
"timeout": {
"timestamp": "123456789000000000",
"block": null
}
}and {
"timeout": {
"timestamp": "123456789000000000",
}
}I think the second is cleaner (and Go compatible) Also note that when we parse JSON, if eg |
This primarily fixes the problem that a u64 nanosecond timestamp exceeds the 53 bit safe integer range after 15 weeks (starting at unix epoch).
It also supersedes #902 (closes #902) by changing the Timestamp type to uint64, serialized to string.
Open questions / follow ups:
BlockInfo.timetoTimestamp, such that we only need one field and simplify the conversion from block time to timeoutenum IbcTimeoutto an object with two fields in all 3 casesBoth of those should not block this PR.