Use network provider state, instead of CurrencyRateController state, to select 'nativeCurrency' #17450
Conversation
…select 'nativeCurrency'
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Builds ready [8d80a46]
Page Load Metrics (1516 ± 159 ms)
Bundle size diffs
|
| (state) => state.metamask, | ||
| ); | ||
| const { provider, unapprovedTxs } = useSelector((state) => state.metamask); | ||
| const nativeCurrency = provider.ticker; |
There was a problem hiding this comment.
These selectors can be used here: getUnapprovedTransactions, getProvider.
ui/ducks/metamask/metamask.js
Outdated
| @@ -307,7 +307,7 @@ export function getConversionRate(state) { | |||
| } | |||
|
|
|||
| export function getNativeCurrency(state) { | |||
| return state.metamask.nativeCurrency; | |||
| return state.metamask.provider.ticker; | |||
There was a problem hiding this comment.
We should not do this when the currencyratecontroller is being used (i.e. the toggle is on). Because it could make the conversion rate and the native currency be out of sync.
|
To do: ensure that conversion rate dependent values are never shown when the currency rate is loading
|
…CurrencyRateCheck is false
| @@ -307,7 +308,10 @@ export function getConversionRate(state) { | |||
| } | |||
|
|
|||
| export function getNativeCurrency(state) { | |||
| return state.metamask.nativeCurrency; | |||
There was a problem hiding this comment.
@jpuri @NiranjanaBinoy I updated this since your last review. The problem was pointed out to me by Mark. state.metamask.nativeCurrency is always set after the conversionRate updates. So if we stopped using it, the nativeCurrency and conversionRate could get out of sync, showing the user incorrect data.
I have changed the code so that we still use the nativeCurrency, but only when the useCurrencyRateCheck setting is on. When it is off, which is required for the bug we are trying to fix, we don't show converted values, so the concern about nativeCurrency and conversionRate being out of sync does not matter
There was a problem hiding this comment.
Just wondering if it is ok to use state.metamask.provider.ticker always
There was a problem hiding this comment.
To add more detail: currently last I checked, we were showing the old network currency and currency rate after switching, until the new rate has been loaded. This is pretty confusing, but we're using the CurrencyRateController nativeCurrency state in this situation so we can at least be confident the symbol we're showing next to the rate matches the rate.
Ideally we would show no fiat value while currency rates are loading. That would be less confusing. Once we do that, I think it would be safe to always use state.metamask.provider.ticker for the current currency.
But until we make that improvement, there is this tradeoff to consider.
Builds ready [c07ef73]
Page Load Metrics (1544 ± 174 ms)
Bundle size diffs
|
Alternative fix for #17429
I think this works because
state.metamask.nativeCurrencyis always set from the provider config: https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/metamask-controller.js#L1020-L1028Further explanation:
The bug from the user perspective is that if the "Show balance and token price checker" is off, then the network logo and "currency symbol" / "network ticker" will be incorrect after switching networks.
The reason for that is that we don't execute any of the code in the in
CurrencyRateControllerupdateExchangeRatesfunction if that toggle is off. That code updates thenativeCurrencyproperty in state, and that property is used throughout the UI to decide which logo and "currency symbol" / "network ticker" to show.The
nativeCurrencyproperty is, however, always set using data from the network provider state. This happens in the metamask controller on two occasions: when theCurrencyRateControlleris initialized, and when the network changes. So, I believe we can safely replace all references tostate.metamask.nativeCurrencywithstate.metamask.provider.ticker, and it will be functionally equivalent.