fix(suite-common): sanitise fetching graph history rates for empty account #14506
Conversation
| // if there were no balance movements at all, | ||
| // findOldestBalanceMovementTimestamp resulted with start date being the same as end date | ||
| // use 1 year into past instead, otherwise getTimestampsInTimeFrame will throw range error | ||
| if (startOfTimeFrameDate.getTime() === endOfTimeFrameDate.getTime()) { |
There was a problem hiding this comment.
I am not sure if this si best fix. I think it may be better to just return early here since if there is no movement we know that graph will just zero all the time. I think return [] or return [{ date: 1, value: 0 }, {date: 2, value: 0}] should work and it will be faster because otherwise we will be fetching timestamps for that whole year.
There was a problem hiding this comment.
So in such case it should probably check for every time if there are no movements of any coin to return early for each option 1d/1w/1m/...
I was thinking about this, but it felt suboptimal for such edge case. And this is the same behaviour there is for 1y option.
There was a problem hiding this comment.
Ok. I'll modify the behaviour for empty history.
There was a problem hiding this comment.
Another thing I'd be a little worried about is mutating startOfTimeFrameDate which is passed as parameter to this function. I'd rather define a new constant for it locally... But maybe it's ok, it's just harder to think about possible consequences. (But the mutation was there even before this PR, I know...)
There was a problem hiding this comment.
I've added check for empty account in 2091d87 and kept the timeframe update to better UX when touching the graph.
There was a problem hiding this comment.
Sorry but it seems unecessary complicated now, I need to take a proper look and think about it.
There was a problem hiding this comment.
I still think that best solution is return early if startOfTimeFrameDate === endOfTimeFrameDate because that's easy and will fix the issue. Don't care about the other timeframes it's not worth added complexity.
|
/rebase |
trezor-ci
force-pushed
the
fix/common/graph/empty-account-rates
branch
from
49b17fd
to
e54ea11
Compare
When no start timestamp was selected for wallet with only empty accounts, it threw range error. This was because
getTimestampsInTimeFramewould receive equal start date and end date. This can happen only if user has no account with txn history at all.So in such case I check and change the start date to 1y ago, to work the same as when such option(1y) is selected.
Related Issue
Resolve #14502
Screenshots: