From b684c094cba0591c3b2cd2c648c971c6ba9b8ff5 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Sat, 26 Aug 2023 00:28:24 -0230 Subject: [PATCH] Fix account selectors when balances are missing (#20385) * Fix account selectors when balances are missing Some of the account selectors we use would return an empty set of accounts if the `AccountTracker` state was missing. This resulted in UI crashes when trying to access the current selected account. The selectors have been updated to use the `identities` as the source- of-truth for the full set of accounts. This ensures that even if the balances are missing, each account will at least be represented by an empty object. * Fix unit test * Fix another unit test * Fix another unit test * Fix another unit test * Fix more unit tests --------- Co-authored-by: legobeat <109787230+legobeat@users.noreply.github.com> --- .../edit-gas-fee-popover.test.js | 3 + .../edit-gas-item/edit-gas-item.test.js | 3 + .../edit-gas-tooltip/edit-gas-tooltip.test.js | 3 + .../gas-details-item-title.test.js | 3 + .../custody-confirm-link-modal.js | 4 +- ui/ducks/send/send.test.js | 95 +++++++++++-------- ui/selectors/selectors.js | 64 +++++++++---- ui/store/actions.test.js | 6 ++ 8 files changed, 116 insertions(+), 65 deletions(-) diff --git a/ui/components/app/edit-gas-fee-popover/edit-gas-fee-popover.test.js b/ui/components/app/edit-gas-fee-popover/edit-gas-fee-popover.test.js index 3e371a05d..7d9c0adfa 100644 --- a/ui/components/app/edit-gas-fee-popover/edit-gas-fee-popover.test.js +++ b/ui/components/app/edit-gas-fee-popover/edit-gas-fee-popover.test.js @@ -61,6 +61,9 @@ const render = ({ txProps, contextProps } = {}) => { balance: '0x1F4', }, }, + identities: { + '0xAddress': {}, + }, selectedAddress: '0xAddress', featureFlags: { advancedInlineGas: true }, gasFeeEstimates: MOCK_FEE_ESTIMATE, diff --git a/ui/components/app/edit-gas-fee-popover/edit-gas-item/edit-gas-item.test.js b/ui/components/app/edit-gas-fee-popover/edit-gas-item/edit-gas-item.test.js index 44ac34b87..86f5ba097 100644 --- a/ui/components/app/edit-gas-fee-popover/edit-gas-item/edit-gas-item.test.js +++ b/ui/components/app/edit-gas-fee-popover/edit-gas-item/edit-gas-item.test.js @@ -67,6 +67,9 @@ const renderComponent = ({ balance: '0x176e5b6f173ebe66', }, }, + identities: { + '0xAddress': {}, + }, selectedAddress: '0xAddress', featureFlags: { advancedInlineGas: true }, gasEstimateType: 'fee-market', diff --git a/ui/components/app/edit-gas-fee-popover/edit-gas-tooltip/edit-gas-tooltip.test.js b/ui/components/app/edit-gas-fee-popover/edit-gas-tooltip/edit-gas-tooltip.test.js index dbeda43ac..97797c110 100644 --- a/ui/components/app/edit-gas-fee-popover/edit-gas-tooltip/edit-gas-tooltip.test.js +++ b/ui/components/app/edit-gas-fee-popover/edit-gas-tooltip/edit-gas-tooltip.test.js @@ -41,6 +41,9 @@ const renderComponent = (componentProps) => { balance: '0x176e5b6f173ebe66', }, }, + identities: { + '0xAddress': {}, + }, selectedAddress: '0xAddress', featureFlags: { advancedInlineGas: true }, }, diff --git a/ui/components/app/gas-details-item/gas-details-item-title/gas-details-item-title.test.js b/ui/components/app/gas-details-item/gas-details-item-title/gas-details-item-title.test.js index 9d8f36c4e..b57c7e32e 100644 --- a/ui/components/app/gas-details-item/gas-details-item-title/gas-details-item-title.test.js +++ b/ui/components/app/gas-details-item/gas-details-item-title/gas-details-item-title.test.js @@ -28,6 +28,9 @@ const render = () => { balance: '0x176e5b6f173ebe66', }, }, + identities: { + '0xAddress': {}, + }, selectedAddress: '0xAddress', }, }); diff --git a/ui/components/institutional/custody-confirm-link-modal/custody-confirm-link-modal.js b/ui/components/institutional/custody-confirm-link-modal/custody-confirm-link-modal.js index e7d0c4d5b..55747a204 100644 --- a/ui/components/institutional/custody-confirm-link-modal/custody-confirm-link-modal.js +++ b/ui/components/institutional/custody-confirm-link-modal/custody-confirm-link-modal.js @@ -11,7 +11,7 @@ import withModalProps from '../../../helpers/higher-order-components/with-modal- import { toChecksumHexAddress } from '../../../../shared/modules/hexstring-utils'; import { mmiActionsFactory } from '../../../store/institutional/institution-background'; import { setSelectedAddress } from '../../../store/actions'; -import { getMetaMaskAccountsRaw } from '../../../selectors'; +import { getMetaMaskIdentities } from '../../../selectors'; import { getMMIAddressFromModalOrAddress, getCustodyAccountDetails, @@ -42,7 +42,7 @@ const CustodyConfirmLink = ({ hideModal }) => { const dispatch = useDispatch(); const mmiActions = mmiActionsFactory(); const trackEvent = useContext(MetaMetricsContext); - const mmiAccounts = useSelector(getMetaMaskAccountsRaw); + const mmiAccounts = useSelector(getMetaMaskIdentities); const address = useSelector(getMMIAddressFromModalOrAddress); const custodyAccountDetails = useSelector(getCustodyAccountDetails); const { custodians } = useSelector(getMMIConfiguration); diff --git a/ui/ducks/send/send.test.js b/ui/ducks/send/send.test.js index cb10c7a87..e946edefb 100644 --- a/ui/ducks/send/send.test.js +++ b/ui/ducks/send/send.test.js @@ -77,6 +77,8 @@ import { draftTransactionInitialState, editExistingTransaction } from '.'; const mockStore = createMockStore([thunk]); +const mockAddress1 = '0xdafea492d9c6733ae3d56b7ed1adb60692c98123'; + jest.mock('./send', () => { const actual = jest.requireActual('./send'); return { @@ -1063,7 +1065,7 @@ describe('Send Slice', () => { describe('QR Code Detected', () => { const qrCodestate = getInitialSendStateWithExistingTxState({ recipient: { - address: '0xAddress', + address: mockAddress1, }, }); @@ -1102,7 +1104,7 @@ describe('Send Slice', () => { const draftTransaction = getTestUUIDTx(result); - expect(draftTransaction.recipient.address).toStrictEqual('0xAddress'); + expect(draftTransaction.recipient.address).toStrictEqual(mockAddress1); expect(draftTransaction.recipient.error).toStrictEqual( INVALID_RECIPIENT_ADDRESS_ERROR, ); @@ -1115,7 +1117,7 @@ describe('Send Slice', () => { ...INITIAL_SEND_STATE_FOR_EXISTING_DRAFT, selectedAccount: { balance: '0x0', - address: '0xAddress', + address: mockAddress1, }, }; @@ -1144,7 +1146,7 @@ describe('Send Slice', () => { ...INITIAL_SEND_STATE_FOR_EXISTING_DRAFT, selectedAccount: { balance: '0x0', - address: '0xAddress', + address: mockAddress1, }, }; @@ -1158,7 +1160,7 @@ describe('Send Slice', () => { const result = sendReducer(olderState, action); expect(result.selectedAccount.balance).toStrictEqual('0x0'); - expect(result.selectedAccount.address).toStrictEqual('0xAddress'); + expect(result.selectedAccount.address).toStrictEqual(mockAddress1); }); }); @@ -1167,13 +1169,13 @@ describe('Send Slice', () => { const accountsChangedState = { ...getInitialSendStateWithExistingTxState({ fromAccount: { - address: '0xAddress', + address: mockAddress1, balance: '0x0', }, }), stage: SEND_STAGES.EDIT, selectedAccount: { - address: '0xAddress', + address: mockAddress1, balance: '0x0', }, }; @@ -1182,7 +1184,7 @@ describe('Send Slice', () => { type: 'ACCOUNT_CHANGED', payload: { account: { - address: '0xAddress', + address: mockAddress1, balance: '0x1', }, }, @@ -1201,13 +1203,13 @@ describe('Send Slice', () => { const accountsChangedState = { ...getInitialSendStateWithExistingTxState({ fromAccount: { - address: '0xAddress', + address: mockAddress1, balance: '0x0', }, }), stage: SEND_STAGES.EDIT, selectedAccount: { - address: '0xAddress', + address: mockAddress1, balance: '0x0', }, }; @@ -1231,7 +1233,7 @@ describe('Send Slice', () => { ...INITIAL_SEND_STATE_FOR_EXISTING_DRAFT, stage: SEND_STAGES.EDIT, selectedAccount: { - address: '0xAddress', + address: mockAddress1, balance: '0x0', }, }; @@ -1274,23 +1276,23 @@ describe('Send Slice', () => { 1559: true, }, }, - selectedAddress: '0xAddress', - identities: { '0xAddress': { address: '0xAddress' } }, + selectedAddress: mockAddress1, + identities: { [mockAddress1]: { address: mockAddress1 } }, keyrings: [ { type: KeyringType.hdKeyTree, - accounts: ['0xAddress'], + accounts: [mockAddress1], }, ], accounts: { - '0xAddress': { - address: '0xAddress', + [mockAddress1]: { + address: mockAddress1, balance: '0x0', }, }, cachedBalances: { 0x5: { - '0xAddress': '0x0', + [mockAddress1]: '0x0', }, }, providerConfig: { @@ -1580,14 +1582,17 @@ describe('Send Slice', () => { }, cachedBalances: { [CHAIN_IDS.GOERLI]: { - '0xAddress': '0x0', + [mockAddress1]: '0x0', }, }, accounts: { - '0xAddress': { - address: '0xAddress', + [mockAddress1]: { + address: mockAddress1, }, }, + identities: { + [mockAddress1]: {}, + }, }, send: { ...getInitialSendStateWithExistingTxState({ @@ -1607,7 +1612,7 @@ describe('Send Slice', () => { userInputHexData: '', }), selectedAccount: { - address: '0xAddress', + address: mockAddress1, }, }, }; @@ -2379,16 +2384,18 @@ describe('Send Slice', () => { addressBook: { [CHAIN_IDS.GOERLI]: {}, }, - identities: {}, + identities: { + [mockAddress1]: {}, + }, accounts: { - '0xAddress': { - address: '0xAddress', + [mockAddress1]: { + address: mockAddress1, balance: '0x0', }, }, cachedBalances: { [CHAIN_IDS.GOERLI]: { - '0xAddress': '0x0', + [mockAddress1]: '0x0', }, }, tokenList: {}, @@ -2397,7 +2404,7 @@ describe('Send Slice', () => { id: 1, txParams: { data: '', - from: '0xAddress', + from: mockAddress1, to: '0xRecipientAddress', gas: GAS_LIMITS.SIMPLE, gasPrice: '0x3b9aca00', // 1000000000 @@ -2414,7 +2421,7 @@ describe('Send Slice', () => { ...getInitialSendStateWithExistingTxState({ id: 1, fromAccount: { - address: '0xAddress', + address: mockAddress1, }, }), }, @@ -2443,7 +2450,7 @@ describe('Send Slice', () => { type: AssetType.native, }, fromAccount: { - address: '0xAddress', + address: mockAddress1, balance: '0x0', }, gas: { @@ -2514,16 +2521,18 @@ describe('Send Slice', () => { addressBook: { [CHAIN_IDS.GOERLI]: {}, }, - identities: {}, + identities: { + [mockAddress1]: {}, + }, accounts: { - '0xAddress': { - address: '0xAddress', + [mockAddress1]: { + address: mockAddress1, balance: '0x0', }, }, cachedBalances: { [CHAIN_IDS.GOERLI]: { - '0xAddress': '0x0', + [mockAddress1]: '0x0', }, }, tokenList: {}, @@ -2533,10 +2542,10 @@ describe('Send Slice', () => { txParams: { data: generateERC721TransferData({ toAddress: BURN_ADDRESS, - fromAddress: '0xAddress', + fromAddress: mockAddress1, tokenId: BigNumber.from(15000).toString(), }), - from: '0xAddress', + from: mockAddress1, to: '0xNftAddress', gas: GAS_LIMITS.BASE_TOKEN_ESTIMATE, gasPrice: '0x3b9aca00', // 1000000000 @@ -2586,7 +2595,7 @@ describe('Send Slice', () => { type: AssetType.native, }, fromAccount: { - address: '0xAddress', + address: mockAddress1, balance: '0x0', }, gas: { @@ -2697,16 +2706,18 @@ describe('Send Slice', () => { addressBook: { [CHAIN_IDS.GOERLI]: {}, }, - identities: {}, + identities: { + [mockAddress1]: {}, + }, accounts: { - '0xAddress': { - address: '0xAddress', + [mockAddress1]: { + address: mockAddress1, balance: '0x0', }, }, cachedBalances: { [CHAIN_IDS.GOERLI]: { - '0xAddress': '0x0', + [mockAddress1]: '0x0', }, }, unapprovedTxs: { @@ -2722,7 +2733,7 @@ describe('Send Slice', () => { decimals: 18, }, }), - from: '0xAddress', + from: mockAddress1, to: '0xTokenAddress', gas: GAS_LIMITS.BASE_TOKEN_ESTIMATE, gasPrice: '0x3b9aca00', // 1000000000 @@ -2740,7 +2751,7 @@ describe('Send Slice', () => { }, }), selectedAccount: { - address: '0xAddress', + address: mockAddress1, balance: '0x0', }, stage: SEND_STAGES.EDIT, @@ -2777,7 +2788,7 @@ describe('Send Slice', () => { type: AssetType.native, }, fromAccount: { - address: '0xAddress', + address: mockAddress1, balance: '0x0', }, gas: { diff --git a/ui/selectors/selectors.js b/ui/selectors/selectors.js index 9cc5b74f3..4129cdcd2 100644 --- a/ui/selectors/selectors.js +++ b/ui/selectors/selectors.js @@ -281,28 +281,38 @@ export function deprecatedGetCurrentNetworkId(state) { return state.metamask.networkId ?? 'loading'; } +/** + * Get MetaMask accounts, including account name and balance. + */ export const getMetaMaskAccounts = createSelector( - getMetaMaskAccountsRaw, + getMetaMaskIdentities, + getMetaMaskAccountBalances, getMetaMaskCachedBalances, - (currentAccounts, cachedBalances) => - Object.entries(currentAccounts).reduce( - (selectedAccounts, [accountID, account]) => { - if (account.balance === null || account.balance === undefined) { - return { - ...selectedAccounts, - [accountID]: { - ...account, - balance: cachedBalances && cachedBalances[accountID], - }, - }; - } - return { - ...selectedAccounts, - [accountID]: account, + (identities, balances, cachedBalances) => + Object.keys(identities).reduce((accounts, address) => { + // TODO: mix in the identity state here as well, consolidating this + // selector with `accountsWithSendEtherInfoSelector` + let account = {}; + + if (balances[address]) { + account = { + ...account, + ...balances[address], }; - }, - {}, - ), + } + + if (account.balance === null || account.balance === undefined) { + account = { + ...account, + balance: cachedBalances && cachedBalances[address], + }; + } + + return { + ...accounts, + [address]: account, + }; + }, {}), ); export function getSelectedAddress(state) { @@ -325,11 +335,23 @@ export function getMetaMaskKeyrings(state) { return state.metamask.keyrings; } +/** + * Get identity state. + * + * @param {object} state - Redux state + * @returns {object} A map of account addresses to identities (which includes the account name) + */ export function getMetaMaskIdentities(state) { return state.metamask.identities; } -export function getMetaMaskAccountsRaw(state) { +/** + * Get account balances state. + * + * @param {object} state - Redux state + * @returns {object} A map of account addresses to account objects (which includes the account balance) + */ +export function getMetaMaskAccountBalances(state) { return state.metamask.accounts; } @@ -368,7 +390,7 @@ export const getMetaMaskAccountsConnected = createSelector( export function isBalanceCached(state) { const selectedAccountBalance = - state.metamask.accounts[getSelectedAddress(state)].balance; + getMetaMaskAccountBalances(state)[getSelectedAddress(state)]?.balance; const cachedBalance = getSelectedAccountCachedBalance(state); return Boolean(!selectedAccountBalance && cachedBalance); diff --git a/ui/store/actions.test.js b/ui/store/actions.test.js index ebf77ff77..21157703f 100644 --- a/ui/store/actions.test.js +++ b/ui/store/actions.test.js @@ -244,6 +244,9 @@ describe('Actions', () => { '0xAnotherAddress': '0x0', }, }, + identities: { + '0xAnotherAddress': {}, + }, }), ); @@ -1876,6 +1879,9 @@ describe('Actions', () => { balance: '0x0', }, }, + identities: { + '0xFirstAddress': {}, + }, cachedBalances: { '0x1': { '0xFirstAddress': '0x0',