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

Fixed navigation through multiple unapproved transactions for ERC20 tokens #16822

Merged
merged 4 commits into from
Jan 11, 2023
Merged

Fixed navigation through multiple unapproved transactions for ERC20 tokens #16822

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
43685b5
Fixed navigation through multiple unapproved transactions for ERC20 t…
filipsekulic Dec 6, 2022
5665d5d
Fixed tx details activity-log currency
filipsekulic Dec 14, 2022
8220055
Fixed e2e test failure
filipsekulic Dec 20, 2022
453de3c
Fixed a problem with txId
filipsekulic Jan 9, 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
99 changes: 98 additions & 1 deletion ui/components/app/confirm-page-container/confirm-page-container-container.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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);
Expand Down
78 changes: 48 additions & 30 deletions ...ontainer/confirm-page-container-navigation/confirm-page-container-navigation.component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);

Copy link
Contributor

Choose a reason for hiding this comment

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

The "Navigate transactions - should confirm and remove an unapproved transaction" e2e test is failing because: TimeoutError: Waiting for element to be located By(xpath, .//*[contains(concat(' ', normalize-space(./@class), ' '), ' confirm-page-container-navigation ')][(contains(string(.), '1 of 3') or contains(string(.), '1of3'))])

This probably means that during the e2e test run showNavigation is false, which probably means that enumUnapprovedTxs is an empty array. Looking at this new code for getting currentNetworkUnapprovedTxs compared to the old code in confirm-transaction-base.container.js, I notice that the chainId and network are here derived from the transaction but in confirm-transaction-base.container.js they are taken directly from state.metamask.

If this feature is working when running it manually in the browser, for both custom approvals and erc-20 token transfers, then the problem is likely in e2e test fixtures (unless there is something wrong with getting the chainId and network from the transaction?... I don't think so, but worth a double check)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right @danjm. Good catch!
I fixed that and also made a small change that solved the problem, so now txData is being passed from a parent component.
You can check it here: 1d005ae

Copy link
Contributor

Choose a reason for hiding this comment

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

why did you have to change it so that txData is being passed in, instead of being selected? what problem did that solve?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When a user tries to confirm one of the transactions, it now shows the navigation, but doesn't get updated as it should - txData object being selected becomes empty, next tx cannot be seen on the next page and currentPosition is -1 as the currentTx cannot be found from enumUnapprovedTxs, because there is no txId from txData object which is empty at that moment.
Screencast:

Screen.Recording.2022-12-20.at.16.05.36.mov

Copy link
Contributor

Choose a reason for hiding this comment

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

txData is derived from state in confirm-transaction-base.container.js which means we can get it here without drilling the prop down the line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get it, but the problem with it is that somehow the txData gets empty when one of the transactions is confirmed.
You can see it in the above attached video where txDataFromSelector is actually the transaction data derived from state. I console.log-ged it in order to show this problem...

Copy link
Contributor

Choose a reason for hiding this comment

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

what was the code for getting txDataFromSelector? was it derived from state in the exact same way as it is in confirm-transaction-base.container.js?

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 (
<div
Expand Down Expand Up @@ -46,10 +77,10 @@ const ConfirmPageContainerNavigation = (props) => {
</div>
<div className="confirm-page-container-navigation__textcontainer">
<div className="confirm-page-container-navigation__navtext">
{positionOfCurrentTx} {ofText} {totalTx}
{positionOfCurrentTx} {t('ofTextNofM')} {totalTx}
</div>
<div className="confirm-page-container-navigation__longtext">
{requestsWaitingText}
{t('requestsAwaitingAcknowledgement')}
</div>
</div>
<div
Expand Down Expand Up @@ -77,17 +108,4 @@ const ConfirmPageContainerNavigation = (props) => {
);
};

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;
34 changes: 1 addition & 33 deletions ui/components/app/confirm-page-container/confirm-page-container.component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -212,18 +191,7 @@ export default class ConfirmPageContainer extends Component {
return (
<GasFeeContextProvider transaction={currentTransaction}>
<div className="page-container" data-testid="page-container">
<ConfirmPageContainerNavigation
totalTx={totalTx}
positionOfCurrentTx={positionOfCurrentTx}
nextTxId={nextTxId}
prevTxId={prevTxId}
showNavigation={showNavigation}
onNextTx={(txId) => onNextTx(txId)}
firstTx={firstTx}
lastTx={lastTx}
ofText={ofText}
requestsWaitingText={requestsWaitingText}
/>
<ConfirmPageContainerNavigation />
{assetStandard === ERC20 ||
assetStandard === ERC721 ||
assetStandard === ERC1155 ? (
Expand Down
55 changes: 2 additions & 53 deletions ui/pages/confirm-transaction-base/confirm-transaction-base.component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 ||
Expand All @@ -1255,6 +1203,7 @@ export default class ConfirmTransactionBase extends Component {
nativeCurrency={nativeCurrency}
isApprovalOrRejection={isApprovalOrRejection}
assetStandard={assetStandard}
txData={txData}
/>
</TransactionModalContextProvider>
);
Expand Down
1 change: 0 additions & 1 deletion ui/pages/confirm-transaction-base/confirm-transaction-base.container.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,6 @@ const mapStateToProps = (state, ownProps) => {
nonce,
unapprovedTxs,
unapprovedTxCount,
currentNetworkUnapprovedTxs,
customGas: {
gasLimit,
gasPrice,
Expand Down
4 changes: 4 additions & 0 deletions ui/pages/token-allowance/token-allowance.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -236,6 +237,9 @@ export default function TokenAllowance({

return (
<Box className="token-allowance-container page-container">
<Box>
<ConfirmPageContainerNavigation />
</Box>
<Box
paddingLeft={4}
paddingRight={4}
Expand Down