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.

Sign up for GitHub

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

Use network provider state, instead of CurrencyRateController state, to select 'nativeCurrency' #17450

Merged
merged 6 commits into from
Jan 27, 2023
+34 −13
Merged

Use network provider state, instead of CurrencyRateController state, to select 'nativeCurrency' #17450

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ed09bcb
Use network provider state, instead of CurrencyRateController state, …
danjm Jan 27, 2023
cddca59
Fix unit tests
danjm Jan 27, 2023
8d80a46
Lint fix
danjm Jan 27, 2023
49b65ad
Only use the network provider ticket for the native currency when use…
danjm Jan 27, 2023
f8c4120
Fix unit test
danjm Jan 27, 2023
c07ef73
Fix tests for real
danjm Jan 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion test/data/mock-state.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@
"network": "5",
"provider": {
"type": "rpc",
"chainId": "0x5"
"chainId": "0x5",
"ticker": "ETH"
},
"keyrings": [
{
Expand Down
4 changes: 4 additions & 0 deletions ui/components/app/transaction-activity-log/transaction-activity-log.container.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ describe('TransactionActivityLog container', () => {
conversionRate: 280.45,
nativeCurrency: 'ETH',
frequentRpcListDetail: [],
provider: {
ticker: 'ETH',
},
},
};

Expand All @@ -43,6 +46,7 @@ describe('TransactionActivityLog container', () => {
],
provider: {
rpcUrl: 'https://customnetwork.com/',
ticker: 'ETH',
},
},
};
Expand Down
3 changes: 3 additions & 0 deletions ui/ducks/confirm-transaction/confirm-transaction.duck.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,9 @@ describe('Confirm Transaction Duck', () => {
metamask: {
conversionRate: 468.58,
currentCurrency: 'usd',
provider: {
ticker: 'ETH',
},
},
confirmTransaction: {
ethTransactionAmount: '1',
Expand Down
6 changes: 5 additions & 1 deletion ui/ducks/metamask/metamask.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
accountsWithSendEtherInfoSelector,
checkNetworkAndAccountSupports1559,
getAddressBook,
getUseCurrencyRateCheck,
} from '../../selectors';
import { updateTransactionGasFees } from '../../store/actions';
import { setCustomGasLimit, setCustomGasPrice } from '../gas/gas.duck';
Expand Down Expand Up @@ -307,7 +308,10 @@ export function getConversionRate(state) {
}

export function getNativeCurrency(state) {
return state.metamask.nativeCurrency;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering if it is ok to use state.metamask.provider.ticker always

Copy link
Member

@Gudahtt Gudahtt Jan 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

const useCurrencyRateCheck = getUseCurrencyRateCheck(state);
return useCurrencyRateCheck
? state.metamask.nativeCurrency
: state.metamask.provider.ticker;
}

export function getSendHexDataFeatureFlagState(state) {
Expand Down
16 changes: 15 additions & 1 deletion ui/ducks/metamask/metamask.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,12 @@ describe('MetaMask Reducers', () => {
currentBlockGasLimit: '0x4c1878',
conversionRate: 1200.88200327,
nativeCurrency: 'ETH',
useCurrencyRateCheck: true,
network: '5',
provider: {
type: 'testnet',
chainId: '0x5',
ticker: 'TestETH',
},
accounts: {
'0xfdea65c8e26263f6d9a1b5de9555d2931a33b825': {
Expand Down Expand Up @@ -308,9 +310,21 @@ describe('MetaMask Reducers', () => {
});

describe('getNativeCurrency()', () => {
it('should return the ticker symbol of the selected network', () => {
it('should return nativeCurrency when useCurrencyRateCheck is true', () => {
expect(getNativeCurrency(mockState)).toStrictEqual('ETH');
});

it('should return the ticker symbol of the selected network when useCurrencyRateCheck is false', () => {
expect(
getNativeCurrency({
...mockState,
metamask: {
...mockState.metamask,
useCurrencyRateCheck: false,
},
}),
).toStrictEqual('TestETH');
});
});

describe('getSendHexDataFeatureFlagState()', () => {
Expand Down
5 changes: 2 additions & 3 deletions ui/pages/send/gas-display/gas-display.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,8 @@ export default function GasDisplay({ gasError }) {
const useCurrencyRateCheck = useSelector(getUseCurrencyRateCheck);
const { showFiatInTestnets, useNativeCurrencyAsPrimaryCurrency } =
useSelector(getPreferences);
const { nativeCurrency, provider, unapprovedTxs } = useSelector(
(state) => state.metamask,
);
const { provider, unapprovedTxs } = useSelector((state) => state.metamask);
const nativeCurrency = provider.ticker;
Copy link
Contributor

Choose a reason for hiding this comment

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

These selectors can be used here: getUnapprovedTransactions, getProvider.

const { chainId } = provider;
const networkName = NETWORK_TO_NAME_MAP[chainId];
const isInsufficientTokenError =
Expand Down
10 changes: 3 additions & 7 deletions ui/pages/send/send-content/__snapshots__/send-content.component.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,8 @@ exports[`SendContent Component render should match snapshot 1`] = `
<div
class="send-v2__asset-dropdown__asset-icon"
>
<img
alt=""
class="identicon"
src="./images/eth_logo.svg"
<div
class="identicon__image-border"
style="height: 36px; width: 36px; border-radius: 18px;"
/>
</div>
Expand All @@ -43,9 +41,7 @@ exports[`SendContent Component render should match snapshot 1`] = `
>
<div
class="send-v2__asset-dropdown__symbol"
>
ETH
</div>
/>
<div
class="send-v2__asset-dropdown__name"
>
Expand Down