From fc83a1b6316093f4a9966f2682b5a28e06ce987c Mon Sep 17 00:00:00 2001 From: Filip Sekulic Date: Wed, 11 Jan 2023 16:01:50 +0100 Subject: [PATCH] Fixed navigation through multiple unapproved transactions for ERC20 tokens (#16822) --- .../confirm-page-container-container.test.js | 99 ++++++++++++++++++- ...irm-page-container-navigation.component.js | 78 +++++++++------ .../confirm-page-container.component.js | 34 +------ .../confirm-transaction-base.component.js | 55 +---------- .../confirm-transaction-base.container.js | 1 - ui/pages/token-allowance/token-allowance.js | 4 + 6 files changed, 153 insertions(+), 118 deletions(-) diff --git a/ui/components/app/confirm-page-container/confirm-page-container-container.test.js b/ui/components/app/confirm-page-container/confirm-page-container-container.test.js index a77386adc..eda6801b9 100644 --- a/ui/components/app/confirm-page-container/confirm-page-container-container.test.js +++ b/ui/components/app/confirm-page-container/confirm-page-container-container.test.js @@ -29,7 +29,6 @@ describe('Confirm Page Container Container Test', () => { fromAddress: '0xd8f6a2ffb0fc5952d16c9768b71cfd35b6399aa5', toAddress: '0x7a1A4Ad9cc746a70ee58568466f7996dD0aCE4E8', origin: 'testOrigin', // required - onNextTx: sinon.spy(), // Footer onCancelAll: sinon.spy(), onCancel: sinon.spy(), @@ -69,6 +68,104 @@ describe('Confirm Page Container Container Test', () => { showEdit: true, hideSenderToRecipient: false, toName: '0x7a1...E4E8', + txData: { + id: 1230035278491151, + time: 1671022500513, + status: 'unapproved', + metamaskNetworkId: '80001', + originalGasEstimate: '0xea60', + userEditedGasLimit: false, + chainId: '0x13881', + loadingDefaults: false, + dappSuggestedGasFees: { + gasPrice: '0x4a817c800', + gas: '0xea60', + }, + sendFlowHistory: [], + txParams: { + from: '0xdd34b35ca1de17dfcdc07f79ff1f8f94868c40a1', + to: '0x7a67ff4a59594a56d46e9308a5c6e197fa83a3cf', + value: '0x0', + data: '0x095ea7b30000000000000000000000009bc5baf874d2da8d216ae9f137804184ee5afef40000000000000000000000000000000000000000000000000000000000011170', + gas: '0xea60', + maxFeePerGas: '0x0', + maxPriorityFeePerGas: '0x0', + }, + origin: 'https://metamask.github.io', + type: 'simpleSend', + history: [ + { + id: 1230035278491151, + time: 1671022500513, + status: 'unapproved', + metamaskNetworkId: '80001', + originalGasEstimate: '0xea60', + userEditedGasLimit: false, + chainId: '0x13881', + loadingDefaults: true, + dappSuggestedGasFees: { + gasPrice: '0x4a817c800', + gas: '0xea60', + }, + sendFlowHistory: [], + txParams: { + from: '0xdd34b35ca1de17dfcdc07f79ff1f8f94868c40a1', + to: '0x7a67ff4a59594a56d46e9308a5c6e197fa83a3cf', + value: '0x0', + data: '0x095ea7b30000000000000000000000009bc5baf874d2da8d216ae9f137804184ee5afef40000000000000000000000000000000000000000000000000000000000011170', + gas: '0xea60', + gasPrice: '0x4a817c800', + }, + origin: 'https://metamask.github.io', + type: 'simpleSend', + }, + [ + { + op: 'remove', + path: '/txParams/gasPrice', + note: 'Added new unapproved transaction.', + timestamp: 1671022501288, + }, + { + op: 'add', + path: '/txParams/maxFeePerGas', + value: '0x0', + }, + { + op: 'add', + path: '/txParams/maxPriorityFeePerGas', + value: '0x0', + }, + { + op: 'replace', + path: '/loadingDefaults', + value: false, + }, + { + op: 'add', + path: '/userFeeLevel', + value: 'custom', + }, + { + op: 'add', + path: '/defaultGasEstimates', + value: { + estimateType: 'custom', + gas: '0xea60', + maxFeePerGas: '0', + maxPriorityFeePerGas: '0', + }, + }, + ], + ], + userFeeLevel: 'custom', + defaultGasEstimates: { + estimateType: 'custom', + gas: '0xea60', + maxFeePerGas: '0', + maxPriorityFeePerGas: '0', + }, + }, }; describe('Render and simulate button clicks', () => { const store = configureMockStore()(mockState); diff --git a/ui/components/app/confirm-page-container/confirm-page-container-navigation/confirm-page-container-navigation.component.js b/ui/components/app/confirm-page-container/confirm-page-container-navigation/confirm-page-container-navigation.component.js index 06b944a6e..94753f90a 100755 --- a/ui/components/app/confirm-page-container/confirm-page-container-navigation/confirm-page-container-navigation.component.js +++ b/ui/components/app/confirm-page-container/confirm-page-container-navigation/confirm-page-container-navigation.component.js @@ -1,19 +1,50 @@ -import React from 'react'; -import PropTypes from 'prop-types'; +import React, { useContext } from 'react'; +import { useDispatch, useSelector } from 'react-redux'; +import { useHistory, useParams } from 'react-router-dom'; +import { + getCurrentChainId, + getUnapprovedTransactions, +} from '../../../../selectors'; +import { transactionMatchesNetwork } from '../../../../../shared/modules/transaction.utils'; +import { I18nContext } from '../../../../contexts/i18n'; +import { CONFIRM_TRANSACTION_ROUTE } from '../../../../helpers/constants/routes'; +import { clearConfirmTransaction } from '../../../../ducks/confirm-transaction/confirm-transaction.duck'; +import { hexToDecimal } from '../../../../../shared/lib/metamask-controller-utils'; -const ConfirmPageContainerNavigation = (props) => { - const { - onNextTx, - totalTx, - positionOfCurrentTx, - nextTxId, - prevTxId, - showNavigation, - firstTx, - lastTx, - ofText, - requestsWaitingText, - } = props; +const ConfirmPageContainerNavigation = () => { + const t = useContext(I18nContext); + const dispatch = useDispatch(); + const history = useHistory(); + const { id } = useParams(); + + const unapprovedTxs = useSelector(getUnapprovedTransactions); + const currentChainId = useSelector(getCurrentChainId); + const network = hexToDecimal(currentChainId); + + const currentNetworkUnapprovedTxs = Object.keys(unapprovedTxs) + .filter((key) => + transactionMatchesNetwork(unapprovedTxs[key], currentChainId, network), + ) + .reduce((acc, key) => ({ ...acc, [key]: unapprovedTxs[key] }), {}); + + const enumUnapprovedTxs = Object.keys(currentNetworkUnapprovedTxs); + + const currentPosition = enumUnapprovedTxs.indexOf(id); + + const totalTx = enumUnapprovedTxs.length; + const positionOfCurrentTx = currentPosition + 1; + const nextTxId = enumUnapprovedTxs[currentPosition + 1]; + const prevTxId = enumUnapprovedTxs[currentPosition - 1]; + const showNavigation = enumUnapprovedTxs.length > 1; + const firstTx = enumUnapprovedTxs[0]; + const lastTx = enumUnapprovedTxs[enumUnapprovedTxs.length - 1]; + + const onNextTx = (txId) => { + if (txId) { + dispatch(clearConfirmTransaction()); + history.push(`${CONFIRM_TRANSACTION_ROUTE}/${txId}`); + } + }; return (
{
- {positionOfCurrentTx} {ofText} {totalTx} + {positionOfCurrentTx} {t('ofTextNofM')} {totalTx}
- {requestsWaitingText} + {t('requestsAwaitingAcknowledgement')}
{ ); }; -ConfirmPageContainerNavigation.propTypes = { - totalTx: PropTypes.number, - positionOfCurrentTx: PropTypes.number, - onNextTx: PropTypes.func, - nextTxId: PropTypes.string, - prevTxId: PropTypes.string, - showNavigation: PropTypes.bool, - firstTx: PropTypes.string, - lastTx: PropTypes.string, - ofText: PropTypes.string, - requestsWaitingText: PropTypes.string, -}; - export default ConfirmPageContainerNavigation; diff --git a/ui/components/app/confirm-page-container/confirm-page-container.component.js b/ui/components/app/confirm-page-container/confirm-page-container.component.js index 3827b68f8..369028fa0 100644 --- a/ui/components/app/confirm-page-container/confirm-page-container.component.js +++ b/ui/components/app/confirm-page-container/confirm-page-container.component.js @@ -84,17 +84,6 @@ export default class ConfirmPageContainer extends Component { origin: PropTypes.string.isRequired, ethGasPriceWarning: PropTypes.string, networkIdentifier: PropTypes.string, - // Navigation - totalTx: PropTypes.number, - positionOfCurrentTx: PropTypes.number, - nextTxId: PropTypes.string, - prevTxId: PropTypes.string, - showNavigation: PropTypes.bool, - onNextTx: PropTypes.func, - firstTx: PropTypes.string, - lastTx: PropTypes.string, - ofText: PropTypes.string, - requestsWaitingText: PropTypes.string, // Footer onCancelAll: PropTypes.func, onCancel: PropTypes.func, @@ -161,16 +150,6 @@ export default class ConfirmPageContainer extends Component { nonce, unapprovedTxCount, warning, - totalTx, - positionOfCurrentTx, - nextTxId, - prevTxId, - showNavigation, - onNextTx, - firstTx, - lastTx, - ofText, - requestsWaitingText, hideSenderToRecipient, showAccountInHeader, origin, @@ -212,18 +191,7 @@ export default class ConfirmPageContainer extends Component { return (
- onNextTx(txId)} - firstTx={firstTx} - lastTx={lastTx} - ofText={ofText} - requestsWaitingText={requestsWaitingText} - /> + {assetStandard === ERC20 || assetStandard === ERC721 || assetStandard === ERC1155 ? ( diff --git a/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js index ab9bef765..2d1e66c60 100644 --- a/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js +++ b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js @@ -7,10 +7,7 @@ import ConfirmPageContainer from '../../components/app/confirm-page-container'; import TransactionDecoding from '../../components/app/transaction-decoding'; import { isBalanceSufficient } from '../send/send.utils'; import { addHexes } from '../../helpers/utils/conversions.util'; -import { - CONFIRM_TRANSACTION_ROUTE, - DEFAULT_ROUTE, -} from '../../helpers/constants/routes'; +import { DEFAULT_ROUTE } from '../../helpers/constants/routes'; import { INSUFFICIENT_FUNDS_ERROR_KEY, GAS_LIMIT_TOO_LOW_ERROR_KEY, @@ -116,7 +113,6 @@ export default class ConfirmTransactionBase extends Component { transactionStatus: PropTypes.string, txData: PropTypes.object, unapprovedTxCount: PropTypes.number, - currentNetworkUnapprovedTxs: PropTypes.object, customGas: PropTypes.object, // Component props actionKey: PropTypes.string, @@ -1015,33 +1011,6 @@ export default class ConfirmTransactionBase extends Component { ); } - handleNextTx(txId) { - const { history, clearConfirmTransaction } = this.props; - - if (txId) { - clearConfirmTransaction(); - history.push(`${CONFIRM_TRANSACTION_ROUTE}/${txId}`); - } - } - - getNavigateTxData() { - const { currentNetworkUnapprovedTxs, txData: { id } = {} } = this.props; - const enumUnapprovedTxs = Object.keys(currentNetworkUnapprovedTxs); - const currentPosition = enumUnapprovedTxs.indexOf(id ? id.toString() : ''); - - return { - totalTx: enumUnapprovedTxs.length, - positionOfCurrentTx: currentPosition + 1, - nextTxId: enumUnapprovedTxs[currentPosition + 1], - prevTxId: enumUnapprovedTxs[currentPosition - 1], - showNavigation: enumUnapprovedTxs.length > 1, - firstTx: enumUnapprovedTxs[0], - lastTx: enumUnapprovedTxs[enumUnapprovedTxs.length - 1], - ofText: this.context.t('ofTextNofM'), - requestsWaitingText: this.context.t('requestsAwaitingAcknowledgement'), - }; - } - _beforeUnloadForGasPolling = () => { this._isMounted = false; if (this.state.pollingToken) { @@ -1150,17 +1119,6 @@ export default class ConfirmTransactionBase extends Component { const hasSimulationError = Boolean(txData.simulationFails); const renderSimulationFailureWarning = hasSimulationError && !userAcknowledgedGasMissing; - const { - totalTx, - positionOfCurrentTx, - nextTxId, - prevTxId, - showNavigation, - firstTx, - lastTx, - ofText, - requestsWaitingText, - } = this.getNavigateTxData(); // This `isTokenApproval` case is added to handle possible rendering of this component from // confirm-approve.js when `assetStandard` is `undefined`. That will happen if the request to @@ -1222,16 +1180,6 @@ export default class ConfirmTransactionBase extends Component { errorKey={errorKey} hasSimulationError={hasSimulationError} warning={submitWarning} - totalTx={totalTx} - positionOfCurrentTx={positionOfCurrentTx} - nextTxId={nextTxId} - prevTxId={prevTxId} - showNavigation={showNavigation} - onNextTx={(txId) => this.handleNextTx(txId)} - firstTx={firstTx} - lastTx={lastTx} - ofText={ofText} - requestsWaitingText={requestsWaitingText} disabled={ renderSimulationFailureWarning || !valid || @@ -1255,6 +1203,7 @@ export default class ConfirmTransactionBase extends Component { nativeCurrency={nativeCurrency} isApprovalOrRejection={isApprovalOrRejection} assetStandard={assetStandard} + txData={txData} /> ); diff --git a/ui/pages/confirm-transaction-base/confirm-transaction-base.container.js b/ui/pages/confirm-transaction-base/confirm-transaction-base.container.js index be74a6c78..e68f1eeaf 100644 --- a/ui/pages/confirm-transaction-base/confirm-transaction-base.container.js +++ b/ui/pages/confirm-transaction-base/confirm-transaction-base.container.js @@ -219,7 +219,6 @@ const mapStateToProps = (state, ownProps) => { nonce, unapprovedTxs, unapprovedTxCount, - currentNetworkUnapprovedTxs, customGas: { gasLimit, gasPrice, diff --git a/ui/pages/token-allowance/token-allowance.js b/ui/pages/token-allowance/token-allowance.js index 24d35e67d..55aaa6270 100644 --- a/ui/pages/token-allowance/token-allowance.js +++ b/ui/pages/token-allowance/token-allowance.js @@ -53,6 +53,7 @@ import { setCustomTokenAmount } from '../../ducks/app/app'; import { valuesFor } from '../../helpers/utils/util'; import { calcTokenAmount } from '../../../shared/lib/transactions-controller-utils'; import { MAX_TOKEN_ALLOWANCE_AMOUNT } from '../../../shared/constants/tokens'; +import { ConfirmPageContainerNavigation } from '../../components/app/confirm-page-container'; export default function TokenAllowance({ origin, @@ -236,6 +237,9 @@ export default function TokenAllowance({ return ( + + +