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

refactor: store currency rate in settings ref #1645

Merged
merged 5 commits into from
Oct 25, 2022
Merged

Conversation

lisabaut
Copy link
Contributor

@lisabaut lisabaut commented Oct 20, 2022

Describe the changes you have made in this PR

This is a refactoring which resulted from out the discussion in PR #1614

  • store latest currency rate in SettingContext ref instead of state to prevent a re-render
  • when getFiatValue is called => always check if the currency in the state matches with the currency in the ref, and update otherwise
  • => this ensures that the fiat value is calculated with the correct rate
  • => this ensures that the latest currency rate is called from the background script only once when the currency changed
  • Also wrap getFiatValue in a try/catch block and display an error when a sat to fiat conversion failed

Unfortunately this reverted a few commits from this PR again (sorry @escapedcat 🌷 )

FIXES

This results in a similar problem when changing the currency on the Settings Page:
=> the fiatValue within the AccountMenu needs several steps to show the correct new value because the currencyRate gets updated after getFiatValue is called
=> "$38.76" (initial value) => change to CNY => "CN¥38.76" (correct currency, wrong value) => "CN¥79.65" (correct currency, correct value)

Screenshots

Try/Catch Toast Error

toast

Link this PR to an issue [optional]

Fixes #1608

Type of change

  • fix: Bug fix (non-breaking change which fixes an issue)

How has this been tested?

  • checking how the AccountMenu renders the fiatValue

Checklist

  • My code follows the style guidelines of this project and performed a self-review of my own code
  • New and existing tests pass locally with my changes
  • I checked if I need to make corresponding changes to the documentation (and made those changes if needed)

@github-actions
Copy link

github-actions bot commented Oct 20, 2022

🚀 Thanks for the pull request!

Here are the current build files for testing:

Download and unzip the file for your browser. Refer to the readme for detailed install instructions.


This build is brought to you by: Adam Fiscor (who recently dropped 1337 sats):

The future is bright. We just have a lot of work to do.

Want to sponsor the next build? send some sats to ⚡️builds@getalby.com (don't forget to provide your name)

Don't forget: keep earning sats!

@bumi
Copy link
Collaborator

bumi commented Oct 20, 2022

ups merged this other PR. sorry for the conflict

@lisabaut lisabaut force-pushed the fix-currency-rate-state branch from 3f02c63 to 7f167d8 Compare October 20, 2022 18:08
Copy link
Contributor

@escapedcat escapedcat left a comment

Choose a reason for hiding this comment

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

Awesome, thanks Lisa! Kinda missed the promises :P

@lisabaut
Copy link
Contributor Author

@escapedcat Please review again. I added a try/catch block too and opened a ticket for a found bug
#1649

Copy link
Contributor

@escapedcat escapedcat left a comment

Choose a reason for hiding this comment

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

Right, some do not support certain currencies? Thanks!

Copy link
Collaborator

@bumi bumi left a comment

Choose a reason for hiding this comment

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

why do we need the await her now again? because now we load the rate always when we do getFiatValue and in getCurrencyRate we check if it is already loaded, if nowt we load it throuh the api.getCurrencyRate() call?

correct?

src/app/context/SettingsContext.tsx Outdated Show resolved Hide resolved
src/app/context/SettingsContext.tsx Outdated Show resolved Hide resolved
@lisabaut
Copy link
Contributor Author

As discussed. This can be merged

@lisabaut lisabaut merged commit 249a655 into master Oct 25, 2022
@lisabaut lisabaut deleted the fix-currency-rate-state branch October 25, 2022 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sometimes fiat value appear as 0 but should show none info or other kind of rendering
3 participants