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

Inconsistent reporting for periods of time #449

Closed
uhliksk opened this issue Jul 29, 2022 · 7 comments · Fixed by #454 · May be fixed by naiba4/binance-trading-bot#2 or naiba4/binance-trading-bot#3
Closed

Inconsistent reporting for periods of time #449

uhliksk opened this issue Jul 29, 2022 · 7 comments · Fixed by #454 · May be fixed by naiba4/binance-trading-bot#2 or naiba4/binance-trading-bot#3
Labels
bug Something isn't working

Comments

@uhliksk
Copy link
Contributor

uhliksk commented Jul 29, 2022

Version

v0.0.88 (9c2e4f5)

Description

When Day, Week or Month is selected as period to summarize closed trades there are inconsistent results between main screen and closed trades form because of main screen is reporting closed trades based on UTC time and closed trades form is reporting based on local time.

To Reproduce

  1. Make closed trade in between UTC midnight and local time midnight.
  2. Click on "D" on main screen.
  3. Click on Closed Trades icon on main screen and select "Day".
  4. Compare values on main screen and closed trades form to see the inconsistency.

Expected Behaviours

All reports should be done in local time.

Screenshots

image

Additional context

Optionally it can be reported on UTC time if it is easier to implement, but in any case reporting shoud be using the same time range across the application.

@uhliksk uhliksk added the bug Something isn't working label Jul 29, 2022
@uhliksk
Copy link
Contributor Author

uhliksk commented Aug 2, 2022

Solved using TZ with time zone in `.env':

TZ=Europe/Bratislava

## Live
BINANCE_LIVE_API_KEY=
BINANCE_LIVE_SECRET_KEY=

...

It will be better if there is a way to set the time zone from host to docker container automatically.

@uhliksk
Copy link
Contributor Author

uhliksk commented Aug 3, 2022

Today I found after applying time zone in .env it's even worse. It looks like records in database are also in local time and then everything shifted by UTC offset in opposite way. The time measuring should be completely checked and rewriten to accept host local time without negative impact on database records.

@habibalkhabbaz
Copy link
Contributor

habibalkhabbaz commented Aug 4, 2022

Hello @uhliksk

I can see that in the code sometimes we use moment().format() and sometimes we use moment().utc().format()
It would be easier to debug if you let us knew on which records you have this problem.

@uhliksk
Copy link
Contributor Author

uhliksk commented Aug 4, 2022

Hello @habibalkhabbaz

I think it can't be solved without sending current time zone settings from client to server or even from host to docker. For the server side it's better to keep it running on UTC and for the client side to see proper reports it will have to send not just "day", "week", "month" parameters, but exact date and time of the start and the end of period. This way the server will be always consistent in recording time of events and the only thing impacted by the client time zone will be the client side.

@uhliksk
Copy link
Contributor Author

uhliksk commented Aug 4, 2022

@habibalkhabbaz I think I found the problem. The archivedAt is stored as moment().format(), but selected period is formatted as moment().toISOString(). This leads to comparing strings like 2022-08-03T21:59:59.880+02:00 with 2022-08-03T00:00:00.000Z which is not aware of time zone and it's just comparing simple string. That's why changing timezone will mess the results and time with UTC offset is compared with wrong date. It can be fixed by storing archivedAt in '.toISOString()` format to get time stored in database always in UTC format which will result in proper simple string comparing. In combination with client sending it's timezone to server it'll be working as expected. I'll create PR to this solution and we can discuss it there then.

@habibalkhabbaz
Copy link
Contributor

You are right! @uhliksk

I think it's even better to save all dates in UTC format in the database so all dates will be consistent and we can just format it to our local time for displaying data.

@uhliksk
Copy link
Contributor Author

uhliksk commented Aug 4, 2022

@habibalkhabbaz I found week is also not aware of user's locale and it's always using the Sunday as first day of week. I'll add another commit into #454 to fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants