Skip to content
This repository has been archived by the owner on Jun 16, 2022. It is now read-only.

two last point on graph use same countervalue #917

Closed
gre opened this issue Jul 8, 2018 · 8 comments
Closed

two last point on graph use same countervalue #917

gre opened this issue Jul 8, 2018 · 8 comments
Assignees
Labels
bug Oops, this is a bug regression subtype of bug. this used to work, this no longer

Comments

@gre
Copy link
Contributor

gre commented Jul 8, 2018

Ledger Live Version and Operating System

  • Ledger Live rc.2
  • Platform: mac

Expected behavior

the two last point countervalues should be different like other points

Actual behavior

two last points are identically

Steps to reproduce the behavior

a

@gre gre added bug Oops, this is a bug regression subtype of bug. this used to work, this no longer labels Jul 8, 2018
@meriadec meriadec self-assigned this Jul 8, 2018
meriadec added a commit to meriadec/ledger-live-desktop that referenced this issue Jul 8, 2018
@meriadec meriadec mentioned this issue Jul 8, 2018
@gre
Copy link
Contributor Author

gre commented Jul 9, 2018

can no longer repro the bug this morning

@gre gre closed this as completed Jul 9, 2018
@gre gre reopened this Jul 9, 2018
@gre
Copy link
Contributor Author

gre commented Jul 9, 2018

ok the issue is that the 2 last point are pointing to the same date.

for countervalues we use "close" date value so basically Jul 09 at 00:00 should be a lookup in Jul 08 (Jul 08 whole day countervalue is stored on 2018-07-08 but in reality is at time Jul 09 00.00). if we do - 1 ms as a hack somewhere it would fix the problem but need to think this a bit more. cc @meriadec

capture d ecran 2018-07-09 a 07 35 36

@gre
Copy link
Contributor Author

gre commented Jul 9, 2018

capture d ecran 2018-07-09 a 07 41 13

like this hack literally work but meh.

@gre
Copy link
Contributor Author

gre commented Jul 9, 2018

I think something more fundamental wrong is going on btw

look at this graph, it's saying 2018-07-08 but on the axis it's Jul 9

capture d ecran 2018-07-09 a 07 42 29

@gre
Copy link
Contributor Author

gre commented Jul 9, 2018

this can wait after the release. focus on other things ;)

@meriadec
Copy link
Member

meriadec commented Jul 9, 2018

2 things:

  • "can't repro bug this morning" : not really, the "bug" (which is not really a bug) is present this morning, but we don't see it because...it's the morning and the 2 points for today are too close
  • the date problem comes from the fact the date is just an ISOString cutted at 0-10 indexes. With the format fix commit, the dates are correct (and in the right format)

If you launch the tests in live-common, you will see there is two items for the same date at the end of the array. It's not a regression, always been like that.

@gre
Copy link
Contributor Author

gre commented Jul 9, 2018

@meriadec no actually i can repro xD but yeah. mmh no the problem is that we build date with the start of the day and not at the "end of the previous day" (it needs to be like this to reflect what we actually do on countervalue api) and it's my fault, it's a regression in live-common. i just locally fixed it from live-common and i think that's all we need to do

helpers/account.js, getBalanceHistory

  t = new Date(startOfDay(t) - 1) // midnight yesterday

don't worry about the bug for now, we need to focus on the release

@meriadec
Copy link
Member

meriadec commented Jul 9, 2018

will do PR for the date format though

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Oops, this is a bug regression subtype of bug. this used to work, this no longer
Projects
None yet
Development

No branches or pull requests

2 participants