From f0f5554846ee84d8fe1e063f8b403659c40e19e5 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Wed, 11 Mar 2020 15:40:35 -0300 Subject: [PATCH] Fix selectors that use `metamask.send.from` state (#8182) The `metamask.send.from` field was assumed by various selectors to be an object, but instead it was recently set to a string. The selectors have been updated to assume it's a string, and to fetch the full account object explicitly. The selector `getSendFromObject` was repurposed for this, as that's basically what it already did. The optional address parameter was removed though, as that was only used to fetch the `from` address in cases where the `send` state was set without there being a `from` address set. That case is no longer possible, as the `from` address is always set upon the initialization of the `send` state. The `getSendFromObject` selector no longer fetches the 'name' of that address from the address book state either. This property was not used in either of the cases this selector was used. --- test/unit/ui/app/selectors.spec.js | 29 +++++++---------- ui/app/helpers/utils/util.js | 4 --- .../send/send-footer/send-footer.container.js | 13 +------- ui/app/pages/send/send.container.js | 13 +------- ui/app/pages/send/send.selectors.js | 26 ++++++--------- .../send/tests/send-selectors-test-data.js | 5 +-- .../pages/send/tests/send-selectors.test.js | 32 ++++--------------- ui/app/selectors/selectors.js | 9 +++--- ui/app/selectors/transactions.js | 11 ------- 9 files changed, 35 insertions(+), 107 deletions(-) diff --git a/test/unit/ui/app/selectors.spec.js b/test/unit/ui/app/selectors.spec.js index a9a10e2a9..17214a90a 100644 --- a/test/unit/ui/app/selectors.spec.js +++ b/test/unit/ui/app/selectors.spec.js @@ -114,26 +114,19 @@ describe('Selectors', function () { assert.equal(gasIsLoading, false) }) - describe('Send From', function () { - it('#getSendFrom', function () { - const sendFrom = selectors.getSendFrom(mockState) - assert.equal(sendFrom, '0xc42edfcc21ed14dda456aa0756c153f7985d8813') - }) + it('#getForceGasMin', function () { + const forceGasMin = selectors.getForceGasMin(mockState) + assert.equal(forceGasMin, null) + }) - it('#getForceGasMin', function () { - const forceGasMin = selectors.getForceGasMin(mockState) - assert.equal(forceGasMin, null) - }) + it('#getSendAmount', function () { + const sendAmount = selectors.getSendAmount(mockState) + assert.equal(sendAmount, '1bc16d674ec80000') + }) - it('#getSendAmount', function () { - const sendAmount = selectors.getSendAmount(mockState) - assert.equal(sendAmount, '1bc16d674ec80000') - }) - - it('#getSendMaxModeState', function () { - const sendMaxModeState = selectors.getSendMaxModeState(mockState) - assert.equal(sendMaxModeState, false) - }) + it('#getSendMaxModeState', function () { + const sendMaxModeState = selectors.getSendMaxModeState(mockState) + assert.equal(sendMaxModeState, false) }) it('#getCurrentCurrency', function () { diff --git a/ui/app/helpers/utils/util.js b/ui/app/helpers/utils/util.js index bf968cff9..90e054866 100644 --- a/ui/app/helpers/utils/util.js +++ b/ui/app/helpers/utils/util.js @@ -288,10 +288,6 @@ export function getOriginFromUrl (url) { return origin } -export function getTxById (transactions = [], targetId) { - return transactions.find(({ id }) => String(id) === targetId) -} - export function getAccountByAddress (accounts = [], targetAddress) { return accounts.find(({ address }) => address === targetAddress) } diff --git a/ui/app/pages/send/send-footer/send-footer.container.js b/ui/app/pages/send/send-footer/send-footer.container.js index 5b9c60760..81859cb1a 100644 --- a/ui/app/pages/send/send-footer/send-footer.container.js +++ b/ui/app/pages/send/send-footer/send-footer.container.js @@ -24,10 +24,6 @@ import { getSendErrors, } from '../send.selectors' import { getGasIsLoading } from '../../../selectors/selectors' -import { networkTransactionsSelector } from '../../../selectors/transactions' -import { - getTxById, -} from '../../../helpers/utils/util' import { isSendFormInError, } from './send-footer.selectors' @@ -51,20 +47,13 @@ function mapStateToProps (state) { const gasEstimateType = activeButtonIndex >= 0 ? gasButtonInfo[activeButtonIndex].gasEstimateType : 'custom' - - let fromAddress const editingTransactionId = getSendEditingTransactionId(state) - if (editingTransactionId) { - const transactions = networkTransactionsSelector(state) - const tx = getTxById(transactions, editingTransactionId) - fromAddress = tx && tx.txParams && tx.txParams.from - } return { amount: getSendAmount(state), data: getSendHexData(state), editingTransactionId, - from: getSendFromObject(state, fromAddress), + from: getSendFromObject(state), gasLimit: getGasLimit(state), gasPrice: getGasPrice(state), gasTotal: getGasTotal(state), diff --git a/ui/app/pages/send/send.container.js b/ui/app/pages/send/send.container.js index 49f5b060a..71b7338bf 100644 --- a/ui/app/pages/send/send.container.js +++ b/ui/app/pages/send/send.container.js @@ -28,9 +28,6 @@ import { getSelectedAddress, getAddressBook, } from '../../selectors/selectors' -import { - networkTransactionsSelector, -} from '../../selectors/transactions' import { getTokens } from './send-content/add-recipient/add-recipient.selectors' import { updateSendTo, @@ -54,18 +51,10 @@ import { } from './send.utils.js' import { isValidDomainName, - getTxById, } from '../../helpers/utils/util' function mapStateToProps (state) { - - let fromAddress const editingTransactionId = getSendEditingTransactionId(state) - if (editingTransactionId) { - const transactions = networkTransactionsSelector(state) - const tx = getTxById(transactions, editingTransactionId) - fromAddress = tx && tx.txParams && tx.txParams.from - } return { addressBook: getAddressBook(state), @@ -74,7 +63,7 @@ function mapStateToProps (state) { blockGasLimit: getBlockGasLimit(state), conversionRate: getConversionRate(state), editingTransactionId, - from: getSendFromObject(state, fromAddress), + from: getSendFromObject(state), gasLimit: getGasLimit(state), gasPrice: getGasPrice(state), gasTotal: getGasTotal(state), diff --git a/ui/app/pages/send/send.selectors.js b/ui/app/pages/send/send.selectors.js index ca013af23..36c1ab710 100644 --- a/ui/app/pages/send/send.selectors.js +++ b/ui/app/pages/send/send.selectors.js @@ -3,9 +3,8 @@ import { multiplyCurrencies } from '../../helpers/utils/conversion-util' import { accountsWithSendEtherInfoSelector, getAddressBook, - getCurrentAccountWithSendEtherInfo, - getTargetAccountWithSendEtherInfo, - getMetaMaskAccounts, + getSelectedAccount, + getTargetAccount, getSelectedAddress, } from '../../selectors/selectors' import { estimateGasPriceFromRecentBlocks, calcGasTotal } from './send.utils' @@ -68,13 +67,6 @@ export function getRecentBlocks (state) { return state.metamask.recentBlocks } -export function getSelectedAccount (state) { - const accounts = getMetaMaskAccounts(state) - const selectedAddress = getSelectedAddress(state) - - return accounts[selectedAddress] -} - export function getSelectedIdentity (state) { const selectedAddress = getSelectedAddress(state) const identities = state.metamask.identities @@ -147,15 +139,15 @@ export function getSendFrom (state) { } export function getSendFromBalance (state) { - const from = getSendFrom(state) || getSelectedAccount(state) - return from.balance + const fromAccount = getSendFromObject(state) + return fromAccount.balance } -export function getSendFromObject (state, address) { - if (address) { - return getTargetAccountWithSendEtherInfo(state, address) - } - return getSendFrom(state) || getCurrentAccountWithSendEtherInfo(state) +export function getSendFromObject (state) { + const fromAddress = getSendFrom(state) + return fromAddress + ? getTargetAccount(state, fromAddress) + : getSelectedAccount(state) } export function getSendMaxModeState (state) { diff --git a/ui/app/pages/send/tests/send-selectors-test-data.js b/ui/app/pages/send/tests/send-selectors-test-data.js index 1d748b9e1..37b06e1a6 100644 --- a/ui/app/pages/send/tests/send-selectors-test-data.js +++ b/ui/app/pages/send/tests/send-selectors-test-data.js @@ -172,10 +172,7 @@ export default { 'gasPrice': '0xaa', 'gasTotal': '0xb451dc41b578', 'tokenBalance': 3434, - 'from': { - 'address': '0xabcdefg', - 'balance': '0x5f4e3d2c1', - }, + 'from': '0xc5b8dbac4c1d3f152cdeb400e2313f309c410acb', 'to': '0x987fedabc', 'amount': '0x080', 'memo': '', diff --git a/ui/app/pages/send/tests/send-selectors.test.js b/ui/app/pages/send/tests/send-selectors.test.js index 72d442410..98f63c94c 100644 --- a/ui/app/pages/send/tests/send-selectors.test.js +++ b/ui/app/pages/send/tests/send-selectors.test.js @@ -17,7 +17,6 @@ import { getGasTotal, getPrimaryCurrency, getRecentBlocks, - getSelectedAccount, getSelectedIdentity, getSelectedToken, getSelectedTokenContract, @@ -233,21 +232,6 @@ describe('send selectors', function () { }) }) - describe('getSelectedAccount()', function () { - it('should return the currently selected account', function () { - assert.deepEqual( - getSelectedAccount(mockState), - { - code: '0x', - balance: '0x0', - nonce: '0x0', - address: '0xd85a4b6a394794842887b8284293d69163007bbb', - } - ) - }) - }) - - describe('getSelectedIdentity()', function () { it('should return the identity object of the currently selected address', function () { assert.deepEqual( @@ -368,10 +352,7 @@ describe('send selectors', function () { it('should return the send.from', function () { assert.deepEqual( getSendFrom(mockState), - { - address: '0xabcdefg', - balance: '0x5f4e3d2c1', - } + '0xc5b8dbac4c1d3f152cdeb400e2313f309c410acb', ) }) }) @@ -380,7 +361,7 @@ describe('send selectors', function () { it('should get the send.from balance if it exists', function () { assert.equal( getSendFromBalance(mockState), - '0x5f4e3d2c1' + '0x37452b1315889f80' ) }) @@ -404,13 +385,15 @@ describe('send selectors', function () { assert.deepEqual( getSendFromObject(mockState), { - address: '0xabcdefg', - balance: '0x5f4e3d2c1', + address: '0xc5b8dbac4c1d3f152cdeb400e2313f309c410acb', + balance: '0x37452b1315889f80', + code: '0x', + nonce: '0xa', } ) }) - it('should return the current account with send ether info if send.from does not exist', function () { + it('should return the current account if send.from does not exist', function () { const editedMockState = { metamask: Object.assign({}, mockState.metamask, { send: { @@ -425,7 +408,6 @@ describe('send selectors', function () { balance: '0x0', nonce: '0x0', address: '0xd85a4b6a394794842887b8284293d69163007bbb', - name: 'Send Account 4', } ) }) diff --git a/ui/app/selectors/selectors.js b/ui/app/selectors/selectors.js index 9fc2a434b..aca01076a 100644 --- a/ui/app/selectors/selectors.js +++ b/ui/app/selectors/selectors.js @@ -159,6 +159,11 @@ export function getSelectedAccount (state) { return accounts[selectedAddress] } +export function getTargetAccount (state, targetAddress) { + const accounts = getMetaMaskAccounts(state) + return accounts[targetAddress] +} + export function getSelectedToken (state) { const tokens = state.metamask.tokens || [] const selectedTokenAddress = state.metamask.selectedTokenAddress @@ -271,10 +276,6 @@ export function getForceGasMin (state) { return state.metamask.send.forceGasMin } -export function getSendFrom (state) { - return state.metamask.send.from -} - export function getSendAmount (state) { return state.metamask.send.amount } diff --git a/ui/app/selectors/transactions.js b/ui/app/selectors/transactions.js index 6741fcb08..11536f5ad 100644 --- a/ui/app/selectors/transactions.js +++ b/ui/app/selectors/transactions.js @@ -109,17 +109,6 @@ const transactionSelectorReturnHelper = (selectedTokenAddress, transactions) => .sort((a, b) => b.time - a.time) } -export const networkTransactionsSelector = createSelector( - selectedTokenAddressSelector, - transactionSubSelector, - currentNetworkTxListSelector, - (selectedTokenAddress, subSelectorTxList = [], networkTxList = []) => { - const txsToRender = networkTxList.concat(subSelectorTxList) - - return transactionSelectorReturnHelper(selectedTokenAddress, txsToRender) - } -) - export const transactionsSelector = createSelector( selectedTokenAddressSelector, transactionSubSelector,