-
Notifications
You must be signed in to change notification settings - Fork 103
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
Fix instances of time.Now()
which should be set to UTC
#551
Conversation
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.
LGTM 🎉
Yes, that sounds good. Perhaps we can simply create a |
We have some other writes that should probably be UTC? taproot-assets/tapfreighter/parcel.go Line 353 in 5373ccc
Coin lease duration seems like local time rn instead of UTC also: taproot-assets/tapfreighter/wallet.go Line 236 in 5373ccc
taproot-assets/tapfreighter/chain_porter.go Line 974 in 5373ccc
IIUC we should query the DB in UTC as well: Line 3467 in 5373ccc
taproot-assets/cmd/tapcli/addrs.go Line 127 in 5373ccc
taproot-assets/tapdb/universe_federation.go Line 118 in 5373ccc
taproot-assets/tapgarden/custodian.go Line 531 in 5373ccc
We also still have a bunch of non-UTC usage in tests and the test mock utilities. In general I imagine we should store all times in UTC and only convert to local timezone for display, @guggero ? |
@jharveyb I don't think we need to do anything outside of the So we should only make sure we use |
Ah I see, the timezone we use elsewhere won't matter since the DB normalizes to UTC before write I guess? I looked through the DB logic for other tables where we have timestamps and they look ok. |
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.
Good catch 👍🏽
Closes #541
I have confirmed that the failing unit test now passes.
I think we should enforce UTC location in
clock.NewTestClock
(LND). Thoughts on this @guggero ?