Skip to content
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

rfc: change the time stdlib package #851

Open
thehowl opened this issue May 25, 2023 · 4 comments · May be fixed by #3016
Open

rfc: change the time stdlib package #851

thehowl opened this issue May 25, 2023 · 4 comments · May be fixed by #3016
Assignees
Labels
🗺️good first issue🗺️ Ideal for newcomer contributors 📦 🤖 gnovm Issues or PRs gnovm related 🌟 improvement performance improvements, refactors ...

Comments

@thehowl
Copy link
Member

thehowl commented May 25, 2023

Following Jae's comments on #657:

I think this might be the right thing to do, but it's a bit weird.

One, we need to make sure that the declaration of type Location is safe, for when Location is ever set (now or in the future). So for example, Location shouldn't be modifiable, otherwise any user can modify a *Location for all users, a bad security breach. Not only should we scan the stdlibs/time code, but also I guess everything about the current implementation, including functions that haven't been ported yet, to make sure it will always be safe to completely port it and maintain it.

Also, it seems a bit unnecessary to support this time implementation because (a) it is a struct, which is more expensive than a primary value, (b) at least in the blockchain context we don't need to support that weird "wall timer" thing, (c) it honestly seems way too complex even in Go because of the "wall timer" thing. So I imagine we may want to implement another "time" package for gno in the future that is simpler.

But anyways, the stdlibs/time.Format is already used for boards, so seems fine to go ahead for now.

My thoughts: I do think that a time package should be as close as possible to Go's, because time's methods and types are probably the ones I've committed to memory the most. But I agree it can probably be simplified, especially re: Location and monotonic time, both of which have relative importance in the blockchain chase.

We could make it so the time package always works in UTC by default, with no concept of a "local" time, but retaining the Location struct; so we can then add methods like FormatInLocation which can display time to the user in their local times. We could then add a tzdata package (not in stdlib) that stores the tzdata for all locations (so that a package/realm can do tzdata.Get("Europe/Rome"), returning a *Location which does not affect other users.

One more thought re: usability. From the end user side, reading times only in UTC on a real-life realm, say the new #791 microblog realm, I think can be very annoying if they're not of the crowd of remote workers who are used to converting back and forth their local time to UTC anyway. So continuing on #439, I think we could think about a way to custom-render time in markdown; for instance:

Posted on [11 Jan 2020 11:10 UTC](#time-RFC822)

Posted on [11/01/2020](#time-02%2F01%2F2006)

This is understood by "smart renderers" which can adjust this to be in the viewer's timezone (client-side), by parsing the time with the given format assuming a UTC location if not given, using Go's reference time (01/02 03:04:05PM '06 -0700) and understanding common time formats which are constants in the time package.

@moul
Copy link
Member

moul commented Oct 21, 2024

@thehowl, I believe we discussed this recently. Can you clarify the most up-to-date plan regarding this?

@MikaelVallenet
Copy link
Member

MikaelVallenet commented Oct 21, 2024

Hello, I'd be interested in working on it, I've started a PR on my fork with a deletion of about 90% of the pkg where the time structure is only an int64 timestamp since 1970 without timezone logic (UTC default)
I'd appreciate more details on what you're planning for this issue.

@thehowl
Copy link
Member Author

thehowl commented Oct 22, 2024

@MikaelVallenet Sounds good, please go ahead.

Some other things to consider, re: #2348 as well, also after discussing a bit with @moul:

  • For what concerns gnoweb, we want to use a system where the chain only works on UTC times, and then we can have renders modify times where appropriate.
    • for instance, using [2024-10-11 12:33:41](#relative) can be converted to "10 minutes ago", and [2024-10-11 12:33:41](#local) can be converted to the local time of the user. cc/ @alexiscolin @gfanton.
    • But by default, we encourage primarily using ISO timestamps (YYYY-MM-DD hh:mm:ss) in UTC; they are largely understood by computers and "fall back" gracefully on the human eye.
  • So, on-chain, we only work with UTC timestamps - possibly without a need of TimestampNano because we don't have that precision anyway.

But yes, adding timezones was probably a fluke.

For what concerns testing, I think it's reasonable at the start of gno test to set a TimestampSeconds at the UNIX timestamp of starting the test. Or maybe we want it fixed in time... but, seeing as this is the testing environment, if we end up wanting to change it we can just change it later.

You may want to move internal/os.Sleep to the testing stdlib's time. Or maybe, let's have a SetNow which can take a time.Time to be set as the current value of Now().

@moul
Copy link
Member

moul commented Oct 27, 2024

Added a related comment here: #3016 (review)

@Kouteki Kouteki moved this from Backlog to Todo in 🧙‍♂️gno.land core team Oct 28, 2024
@Kouteki Kouteki moved this from Todo to In Progress in 🧙‍♂️gno.land core team Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗺️good first issue🗺️ Ideal for newcomer contributors 📦 🤖 gnovm Issues or PRs gnovm related 🌟 improvement performance improvements, refactors ...
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

6 participants