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

Add timezone support to the gno standard library's time package #2688

Closed
deelawn opened this issue Aug 13, 2024 · 0 comments · Fixed by #2705
Closed

Add timezone support to the gno standard library's time package #2688

deelawn opened this issue Aug 13, 2024 · 0 comments · Fixed by #2705
Assignees

Comments

@deelawn
Copy link
Contributor

deelawn commented Aug 13, 2024

Description

Edit: Upon further research on how to implement a solution, I've realized what is presented below is not 100% correct. The examples given, run on my machine in my locality, were only possible because of my machine being in the locality of the PDT timezone; Go does not generally support recognizing location based on timezone abbreviations outside of the machine's locality. Therefore, this proposal's scope is restricted to implementing support for named locations, that is locations like America/New_York, when defining specific locations to be used when creating timestamps with the time.Date function, for example.

We currently don't have any real timezone support; only fixed offsets. The time package needs to fully support Locations so that timezones can be used as they are in Go. The current results attempted use of timezone abbreviations and offsets when parsing time strings will be unexpected to most users. See how we currently do not support loading locations. If the sentiment is that we should not fully support timezones, then we should, at the very least, update our documentation to indicate that the time package is not fully implemented.

Problem Example

Consider the following Go code:

package main

import (
	"fmt"
	"time"
)

const layout = "2006-01-02T15:04:05 MST"

func main() {
	aTime := "2024-08-12T00:00:00 PDT"
	paTime, err := time.Parse(layout, aTime)
	if err != nil {
		panic(err)
	}

	pbTime := time.Date(2024, time.August, 12, 0, 0, 0, 0, time.FixedZone("-7", 60*60*-7))

	fmt.Println("A time:", paTime)
	fmt.Println("B time:", pbTime)

	fmt.Println("A unix:", paTime.Unix())
	fmt.Println("B unix:", pbTime.Unix())

	paTime = paTime.Add(time.Hour * 24 * 30 * 4) // about 4 months
	pbTime = pbTime.Add(time.Hour * 24 * 30 * 4)

	fmt.Println("A time:", paTime)
	fmt.Println("B time:", pbTime)

	fmt.Println("A unix:", paTime.Unix())
	fmt.Println("B unix:", pbTime.Unix())
}

When run on an OS with an IANA timezone database, it should produce the following results:

A time: 2024-08-12 00:00:00 -0700 PDT
B time: 2024-08-12 00:00:00 -0700 -7
A unix: 1723446000
B unix: 1723446000
A time: 2024-12-09 23:00:00 -0800 PST
B time: 2024-12-10 00:00:00 -0700 -7
A unix: 1733814000
B unix: 1733814000

Note how Go recognizes the locations based on the PDT timezone abbreviation. The fixed offset time corresponds to the same timezone, so, as expected, the unix time is the same. After adding four months to each of these, the display time has changed but the unix time still remains the same. This is because Go recognizes that a change of timezone for the current location of time A has a timezone change from PDT -> PST during this period due to daylight savings. Go cannot recognize such a transition for the fixed timezone because no location is defined.

Now let's look at what happens when this is executed as Gno code:
https://play.gno.land/p/JfOtyeEg3fK

A time: 2024-08-12 00:00:00 +0000 PDT
B time: 2024-08-12 00:00:00 -0700 -7
A unix: 1723420800
B unix: 1723446000
A time: 2024-12-10 00:00:00 +0000 PDT
B time: 2024-12-10 00:00:00 -0700 -7
A unix: 1733788800
B unix: 1733814000

There a few things to highlight here:

  1. Time A is parsed and the PDT timezone is applied but no location is loaded so there is no offset
  2. Time B is correctly parsed as a fixed timezone
  3. Time A has an earlier unix time because no offset was specified and no location was loaded, so it is interpreted as UTC
  4. When adding four months to both times, time A experiences no timezone change because, without any location being loaded, it is impossible to discern

Why does this matter?

This has the potential to cause harm by way of unexpected results.

Here is an example:

A realm is written that is meant to be used as a prediction market. The author allows submissions up until ten minutes before the start of the event. The event is taking place in the CEST timezone, so they make a call to create an event and provide a string like 2024-09-01 21:00 CEST. This time will end up getting store as a value equivalent to 2024-09-01 20:00 UTC, so users are now able to make predictions one hour and fifty minutes past the start of the event. Or maybe the author of the realm makes a realm call today (2024-08-13) to schedule an event in December using a time string with a constant offset of +2, the current CEST timezone offset. Between now and then, the clocks will move forward one hour, switching back to CET, so the time of the event is now listed as starting one hour later than intended.

Can this be prevented?

Unexpected behavior like this could be prevented by encouraging users to only ever use integers for time -- don't try to parse strings to time due to the uncertainty around how they will be represented and displayed. While a defensive measure like this might work, it does not fix the underlying behavioral issues and someone is sure to make this mistake in the future.

Proposal for a fix

I propose we embed all timezone definitions and rules into the binary so they get loaded into a key-value store when starting the gno.land node. Any timezone location look ups will make a native function call to retrieve the value from the store.

Potential optimizations

Using a native function to strictly retrieve the timezone definitions and rules from the key-value store may not be good enough. Once the raw definition has been read via the native function call, it will call into LoadLocationFromTZData , which may incur a large number of VM execution cycles just to load the complete timezone history into the Location struct, especially if dealing with a timezone that has had many historical rule changes. We'd have to benchmark this to see how much it actually impacts execution, and consider also offloading this to the native function call as well, such that it returns the location struct rather than the raw timezone definition.

@deelawn deelawn self-assigned this Aug 13, 2024
deelawn added a commit that referenced this issue Sep 25, 2024
Closes #2688.

The PR adds support for timezone locations by embedding an [IANA
timezone database](https://github.com/eggert/tz) that gets loaded into
memory when gno.land is started. The gno stdlib's time package has been
updated by porting over code from the go stdlib to support locations.

This feature allows users to create time instances with locations to
ensure that temporal adjustments result in accurate results by following
both historical rules and current rules.

<details><summary>Contributors' checklist...</summary>

- [x] Added new tests, or not needed, or not feasible
- [x] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [x] Updated the official documentation or not needed
- [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [x] Added references to related issues and PRs
- [x] Provided any useful hints for running manual tests
- [x] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

1 participant