-
-
Notifications
You must be signed in to change notification settings - Fork 267
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
Chore/refactor fiat rates #11592
Chore/refactor fiat rates #11592
Conversation
65aad61
to
e5fe5b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should go deeper with refactor here. We should separate fiat rates and transactions as much as possible so I would prefer to not have any direct reference to specific txid
.
Solution that I suggest:
- Take transaction time round it to 24 hours for example (this will save lot of request)
- Store it in format
{ [FiatRateKey]: { [timestamp: number]: Rates }}
in fiat rates reducer - Create selector that will select this rate by timestamp + network symbol + contract
- Use this to save also for mainnets fiat rates not only tokens, no reason to split logic here because it's basically same except contract id
This approach is IMHO much more flexible and resource savy than when we tie fiat rates to specific transactions because:
- Fiat rates request saved (due to time rounding)
- When user don't have wallet remembered we don't need to delete rates after disconnect and we have imidiatelly avaiable them again when device is connected (again request saved, faster for user, offline support)
- If user have multiple transactions of same asset in one day they need just one entry
👍 Agree, I also had something similar in my mind but just thought I would do it in separate PR. |
I would suggest to do it in one to avoid multiple migrations of storage |
5c30630
to
6858da5
Compare
7e00594
to
11a2ed3
Compare
I really like the whole idea except this one point - I certainly wouldn't leave fiat rates stored, even rounded to days, unless we inform users about it somewhere nicely... |
ac31d9f
to
8fd520a
Compare
e032b6b
to
d0073b7
Compare
2a7c43f
to
f5b657d
Compare
Please wait with merging this after suite-native freeze (hopefully today/Monday) 🙏 I'll try to look at it, but I can't promise for Today. |
f5b657d
to
328c330
Compare
Mobile and common part seems OK |
…them in redux storage
c914148
to
5e7011e
Compare
Description
Bigger fiat rates refactor mainly to remove remaining legacy code, store historic rates from transactions to redux store and show fiat value for tokens in history.
[fiatCurrencyCode]: Rate | undefined
.Related Issue
Resolve #10431
Resolve #12289
Screenshots