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(suite): add fiat rates to common #10385

Merged
merged 3 commits into from
Jan 2, 2024
Merged

Conversation

AdamSchinzel
Copy link
Member

@AdamSchinzel AdamSchinzel commented Dec 19, 2023

Recreation of rebased #9292 PR

Description

Desktop now uses almost the same fetching logic as mobile. The initial plan was to move it to @suite-comon/wallet-common and reuse it on both mobile and desktop but for now I kept the native code almost unchanged.

Live update of fiat rates from blockbook via websocket has been removed completely and there is only periodic fetch. That should be sufficient because the Blockbook only updates fiat rates once every 15 minutes anyway.

Fiat rates has been removed from permanent storage, so they need to be fetched again after reload of the app.

Related Issue

Resolve #8109
Resolve #8110
Resolve #9163

Copy link

socket-security bot commented Dec 19, 2023

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

@AdamSchinzel AdamSchinzel force-pushed the chore/add-fiat-rates-to-common branch 2 times, most recently from 75040fd to 1ff4f5a Compare December 19, 2023 12:03
@AdamSchinzel AdamSchinzel requested a review from a team as a code owner December 19, 2023 15:00
@matejkriz
Copy link
Member

In hindsight it's easier to evaluate, but next time an issue like this would definitely be better broken into smaller PRs. There are some refactorings and even smaller fixes, that could have been already in develop and previous releases that way.

@AdamSchinzel AdamSchinzel force-pushed the chore/add-fiat-rates-to-common branch 5 times, most recently from 294ca87 to 458045e Compare December 21, 2023 17:03
Copy link
Member

@tomasklim tomasklim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. When rates are not available, price updated 19k days ago (Added to new issue UI of fiat rates in Suite (last updated, loading,...) #10435)
    Screenshot 2023-12-26 at 21 59 25

Copy link
Member

@tomasklim tomasklim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@peter-sanderson @matejkriz could you please resolve your comments and check mine changes?

Fiat rates are displayed correctly for all networks.
Fiat rates are updated based on time, currency change
Fiat rates are available for ERC-20 and SOL (I havent checked ADA)

fiatRatesSelector is "duplicated" from native with some small changes such as removed memoization. @Nodonisko plans to do some more changes in mobile file, so it will remain duplicated at least for now.

Created issues
#10431 historical rates for tokens
#10435 incomplete handling of failed fetch of fiat rates, loading states,... more related to UI
#10464 do not use deprecated selector for fiat rates

@matejkriz
Copy link
Member

I've hit one bug, dashboard claims that the price has gone down in the last week, but in fact it has gone up (as trade box rightly says)...
image
image

@tomasklim tomasklim force-pushed the chore/add-fiat-rates-to-common branch 2 times, most recently from bef02ef to 3fc55d3 Compare January 2, 2024 14:07
@tomasklim
Copy link
Member

I've hit one bug, dashboard claims that the price has gone down in the last week, but in fact it has gone up (as trade box rightly says)... image image

7fc2b76
Screenshot 2024-01-02 at 8 36 51

Copy link
Member

@matejkriz matejkriz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@matejkriz
Copy link
Member

/rebase

Copy link

github-actions bot commented Jan 2, 2024

@trezor-ci trezor-ci force-pushed the chore/add-fiat-rates-to-common branch from 7fc2b76 to 3ec33e0 Compare January 2, 2024 15:28
@komret komret merged commit 009c41a into develop Jan 2, 2024
31 of 32 checks passed
@komret komret deleted the chore/add-fiat-rates-to-common branch January 2, 2024 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
5 participants