Common: Return UTC TZ time.Time from convert functions

[gbjk]

This fixes timestamps returning in Local TZ.

Type of change

• Bug fix (non-breaking change which fixes an issue)

How has this been tested

• go test ./... -race
• golangci-lint run

[codecov]

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.

❗ 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

@@            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	Coverage Δ
backtester/engine/backtest.go	56.35% <100.00%> (+4.45%)	⬆️
backtester/engine/live.go	84.47% <100.00%> (+1.38%)	⬆️
backtester/report/report.go	73.75% <100.00%> (+4.39%)	⬆️
common/convert/convert.go	94.89% <100.00%> (-0.24%)	⬇️
communications/base/base_interface.go	31.25% <100.00%> (+4.22%)	⬆️
core/version.go	76.47% <ø> (+6.47%)	⬆️
currency/storage.go	52.44% <100.00%> (+2.93%)	⬆️
engine/engine.go	41.66% <100.00%> (+1.95%)	⬆️
engine/ntp_manager.go	73.84% <100.00%> (+0.81%)	⬆️
engine/rpcserver.go	42.29% <100.00%> (+2.44%)	⬆️
... and 75 more

... and 313 files with indirect coverage changes

[shazbert]

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.

[gbjk]

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:

• the app's source of truth about local time probably doesn't match it's user's source of truth,
• the app doesn't want to be exposed to inconsistency with daylight savings changes or changes in its environment, especially when highly available.
• storage to DB is nearly always UTC
• communication between services is nearly always UTC

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]

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-]

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]

Rebased on master

[shazbert]

LGTM. There is a test failure in the CI builds for GetServerTime but it seems intermittent and not due to this PR. tACK.

[thrasher-]

For this to be merged, we also need a way to enforce this across the codebase so that new local time usage won't be introduced

[github-actions]

This PR is stale because it has been open 30 days with no activity. Please provide an update on the progress of this PR.