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

Chore/refactor fiat rates #11592

Merged
merged 7 commits into from
May 21, 2024
Merged

Conversation

AdamSchinzel
Copy link
Contributor

@AdamSchinzel AdamSchinzel commented Mar 14, 2024

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.

  • 58ea5b5 - Deleted rates from transactions since they are not anymore saved and retrieved from there.
  • a9273b9 - Group transactions per symbol and contract and then fetch rates for timestamps, save fiat rates for timestamps to redux storage, round timestamps to hour so we make less requests (24 hours is in my and @tomasklim opinion way too much).
  • 28c28c2, 5e9b807 - Show fiat value with new approach since we get rid of [fiatCurrencyCode]: Rate | undefined.
  • d8e51ac - Just some docs update.
  • 5092ece - Store historic fiat rates to storage.
  • f5b657d - Remove + sign from day header for crypto and fiat amount if it's 0.

Related Issue

Resolve #10431
Resolve #12289

Screenshots

Screenshot 2024-05-15 at 12 39 46 PM

Screenshot 2024-05-15 at 10 47 53 AM Screenshot 2024-05-15 at 10 41 23 AM Screenshot 2024-05-15 at 10 37 09 AM

@AdamSchinzel AdamSchinzel requested review from a team, tomasklim and matejkriz as code owners March 14, 2024 18:52
@AdamSchinzel AdamSchinzel force-pushed the chore/tokens-transactions-fiat-rates branch 2 times, most recently from 65aad61 to e5fe5b8 Compare March 14, 2024 20:33
@matejkriz matejkriz self-assigned this Mar 15, 2024
@matejkriz
Copy link
Member

matejkriz commented Mar 15, 2024

It does not work on my machine 🤔


wallet NOT remembered -

develop:
image
image


chore/tokens-transactions-fiat-rates:
see the zeros for ETH

image image

first time I tried with wallet remembered on develop, then switch to chore/tokens-transactions-fiat-rates to see the migration in action and I got in this state (no historical fiat rates even for ETH):
image

Copy link
Contributor

@Nodonisko Nodonisko left a 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:

  1. Take transaction time round it to 24 hours for example (this will save lot of request)
  2. Store it in format { [FiatRateKey]: { [timestamp: number]: Rates }} in fiat rates reducer
  3. Create selector that will select this rate by timestamp + network symbol + contract
  4. 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:

  1. Fiat rates request saved (due to time rounding)
  2. 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)
  3. If user have multiple transactions of same asset in one day they need just one entry

@AdamSchinzel
Copy link
Contributor Author

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:

  1. Take transaction time round it to 24 hours for example (this will save lot of request)

  2. Store it in format { [FiatRateKey]: { [timestamp: number]: Rates }} in fiat rates reducer

  3. Create selector that will select this rate by timestamp + network symbol + contract

  4. 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:

  1. Fiat rates request saved (due to time rounding)

  2. 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)

  3. 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.

@Nodonisko
Copy link
Contributor

I would suggest to do it in one to avoid multiple migrations of storage

@AdamSchinzel AdamSchinzel force-pushed the chore/tokens-transactions-fiat-rates branch 3 times, most recently from 5c30630 to 6858da5 Compare March 18, 2024 18:28
@AdamSchinzel AdamSchinzel force-pushed the chore/tokens-transactions-fiat-rates branch 3 times, most recently from 7e00594 to 11a2ed3 Compare March 18, 2024 18:48
@AdamSchinzel AdamSchinzel changed the title Chore/tokens transactions fiat rates Chore/refactor fiat rates Mar 18, 2024
@matejkriz
Copy link
Member

2. we don't need to delete rates after disconnect and we have imidiatelly avaiable them again when device is connected

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

@AdamSchinzel AdamSchinzel requested a review from matejkriz March 20, 2024 12:42
@AdamSchinzel AdamSchinzel force-pushed the chore/tokens-transactions-fiat-rates branch 7 times, most recently from ac31d9f to 8fd520a Compare May 15, 2024 08:47
@MiroslavProchazka MiroslavProchazka added the build-desktop This will trigger the build of desktop apps for your PR label May 15, 2024
@AdamSchinzel AdamSchinzel force-pushed the chore/tokens-transactions-fiat-rates branch 3 times, most recently from e032b6b to d0073b7 Compare May 15, 2024 10:06
@AdamSchinzel AdamSchinzel requested a review from tomasklim May 15, 2024 10:37
@AdamSchinzel AdamSchinzel force-pushed the chore/tokens-transactions-fiat-rates branch from 2a7c43f to f5b657d Compare May 15, 2024 10:39
@tomasklim tomasklim dismissed their stale review May 17, 2024 08:21

Feel free to check it somebody else now

@matejkriz
Copy link
Member

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.

@AdamSchinzel AdamSchinzel force-pushed the chore/tokens-transactions-fiat-rates branch from f5b657d to 328c330 Compare May 17, 2024 09:14
@Nodonisko
Copy link
Contributor

Mobile and common part seems OK

@AdamSchinzel AdamSchinzel force-pushed the chore/tokens-transactions-fiat-rates branch from c914148 to 5e7011e Compare May 21, 2024 07:40
@AdamSchinzel AdamSchinzel enabled auto-merge (squash) May 21, 2024 07:41
@AdamSchinzel AdamSchinzel merged commit f40b8bf into develop May 21, 2024
72 of 74 checks passed
@AdamSchinzel AdamSchinzel deleted the chore/tokens-transactions-fiat-rates branch May 21, 2024 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build-desktop This will trigger the build of desktop apps for your PR
Projects
No open projects
Status: 🔎 To be reviewed
Development

Successfully merging this pull request may close these issues.

Remove +/- from 0 values Historical fiat rates for ERC20 are missing
8 participants