search

thrasher-corp / gocryptotrader

A cryptocurrency trading bot and framework supporting multiple exchanges written in Golang.
MIT License
2.95k stars 793 forks source link

Common: Return UTC TZ time.Time from convert functions #1551

Open gbjk opened 1 month ago

gbjk commented 1 month ago

This fixes timestamps returning in Local TZ.

Type of change

How has this been tested

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 67.68293% with 106 lines in your changes missing coverage. Please review.

Project coverage is 37.99%. Comparing base (98f025e) to head (c6decee). Report is 1 commits behind head on master.

:exclamation: Current head c6decee differs from pull request most recent head 9aa913e

Please upload reports for the commit 9aa913e to get more accurate results.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/thrasher-corp/gocryptotrader/pull/1551/graphs/tree.svg?width=650&height=150&src=pr&token=41784B23TS&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=thrasher-corp)](https://app.codecov.io/gh/thrasher-corp/gocryptotrader/pull/1551?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=thrasher-corp) ```diff @@ Coverage Diff @@ ## master #1551 +/- ## ========================================== + Coverage 36.26% 37.99% +1.72% ========================================== Files 419 415 -4 Lines 183133 153253 -29880 ========================================== - Hits 66421 58231 -8190 + Misses 108605 86933 -21672 + Partials 8107 8089 -18 ``` | [Files](https://app.codecov.io/gh/thrasher-corp/gocryptotrader/pull/1551?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=thrasher-corp) | Coverage Δ | | |---|---|---| | [backtester/engine/backtest.go](https://app.codecov.io/gh/thrasher-corp/gocryptotrader/pull/1551?src=pr&el=tree&filepath=backtester%2Fengine%2Fbacktest.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=thrasher-corp#diff-YmFja3Rlc3Rlci9lbmdpbmUvYmFja3Rlc3QuZ28=) | `56.35% <100.00%> (+4.45%)` | :arrow_up: | | [backtester/engine/live.go](https://app.codecov.io/gh/thrasher-corp/gocryptotrader/pull/1551?src=pr&el=tree&filepath=backtester%2Fengine%2Flive.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=thrasher-corp#diff-YmFja3Rlc3Rlci9lbmdpbmUvbGl2ZS5nbw==) | `84.47% <100.00%> (+1.38%)` | :arrow_up: | | [backtester/report/report.go](https://app.codecov.io/gh/thrasher-corp/gocryptotrader/pull/1551?src=pr&el=tree&filepath=backtester%2Freport%2Freport.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=thrasher-corp#diff-YmFja3Rlc3Rlci9yZXBvcnQvcmVwb3J0Lmdv) | `73.75% <100.00%> (+4.39%)` | :arrow_up: | | [common/convert/convert.go](https://app.codecov.io/gh/thrasher-corp/gocryptotrader/pull/1551?src=pr&el=tree&filepath=common%2Fconvert%2Fconvert.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=thrasher-corp#diff-Y29tbW9uL2NvbnZlcnQvY29udmVydC5nbw==) | `94.89% <100.00%> (-0.24%)` | :arrow_down: | | [communications/base/base\_interface.go](https://app.codecov.io/gh/thrasher-corp/gocryptotrader/pull/1551?src=pr&el=tree&filepath=communications%2Fbase%2Fbase_interface.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=thrasher-corp#diff-Y29tbXVuaWNhdGlvbnMvYmFzZS9iYXNlX2ludGVyZmFjZS5nbw==) | `31.25% <100.00%> (+4.22%)` | :arrow_up: | | [core/version.go](https://app.codecov.io/gh/thrasher-corp/gocryptotrader/pull/1551?src=pr&el=tree&filepath=core%2Fversion.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=thrasher-corp#diff-Y29yZS92ZXJzaW9uLmdv) | `76.47% <ø> (+6.47%)` | :arrow_up: | | [currency/storage.go](https://app.codecov.io/gh/thrasher-corp/gocryptotrader/pull/1551?src=pr&el=tree&filepath=currency%2Fstorage.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=thrasher-corp#diff-Y3VycmVuY3kvc3RvcmFnZS5nbw==) | `52.44% <100.00%> (+2.93%)` | :arrow_up: | | [engine/engine.go](https://app.codecov.io/gh/thrasher-corp/gocryptotrader/pull/1551?src=pr&el=tree&filepath=engine%2Fengine.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=thrasher-corp#diff-ZW5naW5lL2VuZ2luZS5nbw==) | `41.66% <100.00%> (+1.95%)` | :arrow_up: | | [engine/ntp\_manager.go](https://app.codecov.io/gh/thrasher-corp/gocryptotrader/pull/1551?src=pr&el=tree&filepath=engine%2Fntp_manager.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=thrasher-corp#diff-ZW5naW5lL250cF9tYW5hZ2VyLmdv) | `73.84% <100.00%> (+0.81%)` | :arrow_up: | | [engine/rpcserver.go](https://app.codecov.io/gh/thrasher-corp/gocryptotrader/pull/1551?src=pr&el=tree&filepath=engine%2Frpcserver.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=thrasher-corp#diff-ZW5naW5lL3JwY3NlcnZlci5nbw==) | `42.29% <100.00%> (+2.44%)` | :arrow_up: | | ... and [75 more](https://app.codecov.io/gh/thrasher-corp/gocryptotrader/pull/1551?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=thrasher-corp) | | ... and [313 files with indirect coverage changes](https://app.codecov.io/gh/thrasher-corp/gocryptotrader/pull/1551/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=thrasher-corp)
gbjk commented 1 month ago

The only issue I have with this change is consistency and can lead to a bit of confusion take gateio package gateioTime struct for example this will have local time variables as well and any time.Unix handling also. So the entire library will need to be updated to this standard. I have tended to keep everything local and push UTC to the boundary or when it's needed e.g. gRPC and rest calls or kline package. Unless there is some serious implications that I am unaware of which might well be, that is my only concern.

You're right, we need to consistency everywhere. I raised this 2 months ago and agreed UTC internal and Local external (in this thread](https://gocryptotrader.slack.com/archives/C05QQD37JBG/p1711776611515419?thread_ts=1711764832.429979&cid=C05QQD37JBG)

Happy to validate the thinking though. Internal times for server apps are normally UTC because:

So we should have one of two PRs:

  1. This PR, extended to ensure all calls to Now() and Unix() are UTC()ed
  2. Or change Parse to switch to local

I couldn't find anyone arguing for Local over UTC in a quick google. I don't believe I introduced any confirmation bias.

Unless you object I'll follow path (1) and ensure we're universally UTC until user facing exits, which should be localised. That includes sending UTC to any grpc clients other than GCT itself.

shazbert commented 1 month ago

Thanks for the context, I do see your point, I don't mind as long as it's consistent, but you got to take into account the overhaul of all time implementations to adhere to that standard. At the end of the day with all my time variables any sort of outbound data I will tack on a .UTC(), I usually don't assume time.Time is any standard at any time and won't assume even after this potential update.

@thrasher- @gloriousCode Thoughts?

thrasher- commented 1 month ago

Agree with UTC internal and local external (like gctcli), everything would have to adhere to this rule though. Comments should also explicility be modified to mention that it's UTC. Local times should also be inputted on client facing apps like gctcli and then converted to UTC behind the scenes to make it human friendly and vice versa.

gbjk commented 2 weeks ago

Rebased on master