gno icon indicating copy to clipboard operation
gno copied to clipboard

rfc: change the `time` stdlib package

Open thehowl opened this issue 2 years ago • 3 comments

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.

thehowl avatar May 25 '23 15:05 thehowl

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

moul avatar Oct 21 '24 16:10 moul

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.

MikaelVallenet avatar Oct 21 '24 18:10 MikaelVallenet

@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().

thehowl avatar Oct 22 '24 16:10 thehowl

Added a related comment here: https://github.com/gnolang/gno/pull/3016#pullrequestreview-2397436168

moul avatar Oct 27 '24 08:10 moul