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

feat(stdlibs): simplify time pkg & remove tz + monoclock logic related #3016

Draft
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

MikaelVallenet
Copy link
Member

@MikaelVallenet MikaelVallenet commented Oct 24, 2024

fix #851

The purpose of this Pull Request is to simplify the Time pkg, which comes from Go, in order to remove unnecessary functions for Gno.

This Pull Request removes the concept of monotonic clock, which corresponds to the time since the start of the program, often used to calculate the time interval between two events more reliably.

I've also removed the notion of timezone, and now an instance of Time is composed as follows:

type Time struct {
	sec  int64
	nsec int32
}

When I parse a date with a different timezone, it's automatically transformed into UTC.
However, the goal would be to allow a Time instance to be displayed in a specific timezone with a markdown pattern, especially for Gnoweb.

[2024-10-11 12:33:41](#local)
[2024-10-11 12:33:41](#Europe/Rome) => 2024-10-11 14:33:41 (Summer time)

Most of the PR is code deletion, not addition, especially the bitwise logic. see article

I've also adapted some unnecessary methods to avoid breaking realms, such as the Zone method, which returns the tz name and offset before the realm name, and is used in the events realm.

// This method is here to avoid breaking changes but always return UTC and offset 0
func (t Time) Zone() (name string, offset int) {
	name, offset = "UTC", 0
	return
}

I also kept some unused param like Location in Date to validate the CI & keep same API as Go:

func Date(year int, month Month, day, hour, min, sec, nsec int, loc *Location) Time {...}

In the code snippet above, i kept the loc parameter even if i don't use it anymore.

I'd like to take this opportunity to say that I haven't answered the testing question, because the PR seems tedious enough to review as it is. If this PR ends up being merged, I'm thinking of doing a second PR for the testing part, to keep things as atomic as possible.

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().
Originally posted by @thehowl in #851 (comment)

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests

@MikaelVallenet MikaelVallenet changed the title feat(stdlibs): simply time pkg & remove tz & monoclock logic related feat(stdlibs): simplify time pkg & remove tz & monoclock logic related Oct 24, 2024
@github-actions github-actions bot added 🧾 package/realm Tag used for new Realms or Packages. 📦 🤖 gnovm Issues or PRs gnovm related labels Oct 24, 2024
@MikaelVallenet MikaelVallenet changed the title feat(stdlibs): simplify time pkg & remove tz & monoclock logic related feat(stdlibs): simplify time pkg & remove tz + monoclock logic related Oct 24, 2024
Copy link

codecov bot commented Oct 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.75%. Comparing base (0246761) to head (03cab99).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3016      +/-   ##
==========================================
- Coverage   63.80%   63.75%   -0.05%     
==========================================
  Files         549      549              
  Lines       78833    78833              
==========================================
- Hits        50300    50262      -38     
- Misses      25144    25176      +32     
- Partials     3389     3395       +6     
Flag Coverage Δ
contribs/gnodev 61.16% <ø> (ø)
contribs/gnofaucet 15.77% <ø> (ø)
gno.land 73.70% <ø> (ø)
gnovm 67.89% <ø> (-0.06%) ⬇️
misc/genstd 79.72% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MikaelVallenet MikaelVallenet marked this pull request as ready for review October 24, 2024 14:28
@Kouteki Kouteki added this to the 🚀 Mainnet launch milestone Oct 24, 2024
@Kouteki Kouteki added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Oct 26, 2024
Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good globally. However, after discussing with Jae which parameters should be specified in the new x/params module (#3003), we believe it would be beneficial to include a "global chain default timezone" to accommodate more regional chains. We can apply the same logic of markdown-based conversion. Instead of having no timezone support or encouraging per-contract timezone support, we ideally want to implement per-chain timezone support.

@MikaelVallenet
Copy link
Member Author

I understand and agree with you, i will look to adapt the PR to set the default tz as a parameter of the chain handle by x/params

@Kouteki Kouteki added the in focus Core team is prioritizing this work label Nov 13, 2024
@thehowl thehowl removed the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Nov 14, 2024
@Kouteki Kouteki removed the request for review from a team November 15, 2024 09:17
@MikaelVallenet MikaelVallenet marked this pull request as draft November 18, 2024 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in focus Core team is prioritizing this work 📦 🤖 gnovm Issues or PRs gnovm related 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: No status
Status: In Review
Development

Successfully merging this pull request may close these issues.

rfc: change the time stdlib package
4 participants