trezor-suite icon indicating copy to clipboard operation
trezor-suite copied to clipboard

Chore/refactor fiat rates

Open AdamSchinzel opened this issue 2 years ago • 5 comments

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.

  • bf009bedf973eb2d6e984b2fe8b209f60b40ce31 - Deleted rates from transactions since they are not anymore saved and retrieved from there.
  • ebaacbd5b1ce7f8821334376b38e0133b483e4c4 - 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).
  • c785e3cc93c65a7b5b1539c9d2fd3aef2dc9f886, b3de106163acda2b4ead168a82f5d64db10e445d - Show fiat value with new approach since we get rid of [fiatCurrencyCode]: Rate | undefined.
  • a5d8cdce4afde15f39d7ca61c0ef14ffaa1c4e34 - Just some docs update.
  • 022a93429cd2c953f5bf37826ec2b6e592155f8e - Store historic fiat rates to storage.
  • 0b5bb234163f245cf4d996e575c170fa3826ec89 - Remove + sign from day header for crypto and fiat amount if it's 0.

Related Issue

Resolve #10431 Resolve #12289

AdamSchinzel avatar Mar 14 '24 18:03 AdamSchinzel

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

matejkriz avatar Mar 15 '24 08:03 matejkriz

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.

AdamSchinzel avatar Mar 15 '24 17:03 AdamSchinzel

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

Nodonisko avatar Mar 15 '24 18:03 Nodonisko

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

matejkriz avatar Mar 19 '24 10:03 matejkriz

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

socket-security[bot] avatar Apr 02 '24 15:04 socket-security[bot]

Solana/Cardano token history price is missing in case there is just one manipulation with that specific token (on a page).

coingecko.ts

export const getFiatRatesForTimestamps = async (
    ticker: TickerId,
    timestamps: number[],
    fiatCurrencyCode: FiatCurrencyCode,
): Promise<HistoricalResponse | null> => {
    if (timestamps.length < 2) return null; // HERE it returns null

from and to cannot be same and I am not sure if this is correct coingecko endpoint to be used tbh

The endpoint is I think okey to use at least for tokens since there is not other option (for coins there is /history endpoint). Solved so it can accept also just one timestamp and improved the range selection.

AdamSchinzel avatar May 14 '24 19:05 AdamSchinzel

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.

matejkriz avatar May 17 '24 08:05 matejkriz

Mobile and common part seems OK

Nodonisko avatar May 18 '24 10:05 Nodonisko