-
Notifications
You must be signed in to change notification settings - Fork 334
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor Timestamps #903
Refactor Timestamps #903
Conversation
3b116f5
to
1c36c59
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good for timestamps anywhere.
Some suggestions on the constructor (and use of it) at the bottom. Please make a minor tweak and good to merge.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we add this, maybe add From<IbcTimeoutBlock>
as well to make that less verbose also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We offer .u128()
so that should be fine. But no need. Please make an issue to follow up on this before 1.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seconds: self.seconds + addition, | ||
nanos: self.nanos, | ||
} | ||
/// Creates a timestamp from nanoseconds since epoch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 const
s!).
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.time
toTimestamp
, such that we only need one field and simplify the conversion from block time to timeoutenum IbcTimeout
to an object with two fields in all 3 casesBoth of those should not block this PR.