-
Notifications
You must be signed in to change notification settings - Fork 277
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: Use SystemTime
for timestamps
#5015
base: main
Are you sure you want to change the base?
refactor: Use SystemTime
for timestamps
#5015
Conversation
From SDK standpoint, it can be useful to clearly see the separation between durations and timestamps in the schema. Currently they are just |
One option I see here is to create |
Signed-off-by: Dmitry Murzin <[email protected]>
07ee499
to
6b9dbad
Compare
#[cfg(feature = "std")] | ||
pub fn creation_time(&self) -> SystemTime { |
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.
but how am I going to access it now in wasm
?
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.
it's needed
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.
What do you think about adding separate methods specificaly for wasm?
#[cfg(not(feature = "std"))]
pub fn creation_time_ms(&self) -> u64 {
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.
As a proposal, we could use jiff::Timestamp
/chrono::DateTime
as a main type instead of std::time::SystemTime
. Both can work in no-std context.
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 think core::time::Duration
was a painstaking effort to make it compatible with no_std
. Is it worth having separate methods to achieve semantic consistency? A time span from the origin can be considered a timestamp
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 need to decide something. Either introducing other no_std
compatible types or staying in core::time::Duration
is acceptable to me
That would be helpful. |
Context
Fixes #4729
Solution
Use
std::time::SystemTime
for time stamps instead ofstd::time::Duration
.SystemTime
requiresstd
, so it will not be available inside WASM.chrono::DateTime
might be used, but it will be more complex (in particular we will have to handle leap seconds), so I am not sure that it is worth itMigration Guide (optional)
Review notes (optional)
Checklist
CONTRIBUTING.md
.