From 308151cd49b914fa96a8fddf377bd85ddff735a7 Mon Sep 17 00:00:00 2001 From: Vladimir Saric <92527393+VSaric@users.noreply.github.com> Date: Thu, 2 Feb 2023 00:14:09 +0100 Subject: [PATCH] Display a warning and gas fee component for token allowance and NFT flow when transaction is expected to fail (#17437) Co-authored-by: Pedro Figueiredo Co-authored-by: Dan J Miller Co-authored-by: Brad Decker --- .../approve-content-card.js | 29 +++- .../transaction-alerts/transaction-alerts.js | 17 +-- .../ui/simulation-error-message/index.js | 1 + .../simulation-error-message.js | 33 +++++ .../simulation-error-message.test.js | 65 +++++++++ ui/hooks/useSimulationFailureWarning.js | 18 +++ .../confirm-approve-content.component.js | 63 +++++++-- .../confirm-approve-content.component.test.js | 130 ++++++++++++++++++ .../confirm-approve-content.stories.js | 1 + ui/pages/confirm-approve/confirm-approve.js | 9 ++ .../confirm-transaction-base.component.js | 18 +-- ui/pages/token-allowance/token-allowance.js | 27 ++++ 12 files changed, 369 insertions(+), 42 deletions(-) create mode 100644 ui/components/ui/simulation-error-message/index.js create mode 100644 ui/components/ui/simulation-error-message/simulation-error-message.js create mode 100644 ui/components/ui/simulation-error-message/simulation-error-message.test.js create mode 100644 ui/hooks/useSimulationFailureWarning.js diff --git a/ui/components/app/approve-content-card/approve-content-card.js b/ui/components/app/approve-content-card/approve-content-card.js index 539ee0574..5de4aa9fd 100644 --- a/ui/components/app/approve-content-card/approve-content-card.js +++ b/ui/components/app/approve-content-card/approve-content-card.js @@ -43,6 +43,8 @@ export default function ApproveContentCard({ isSetApproveForAll, isApprovalOrRejection, data, + userAcknowledgedGasMissing, + renderSimulationFailureWarning, }) { const t = useContext(I18nContext); @@ -91,9 +93,14 @@ export default function ApproveContentCard({ )} - {showEdit && showAdvanceGasFeeOptions && supportsEIP1559 && ( - - )} + {showEdit && + showAdvanceGasFeeOptions && + supportsEIP1559 && + !renderSimulationFailureWarning && ( + + )} )} {renderTransactionDetailsContent && - (!isMultiLayerFeeNetwork && supportsEIP1559 ? ( - + (!isMultiLayerFeeNetwork && + supportsEIP1559 && + !renderSimulationFailureWarning ? ( + ) : ( {supportsEIP1559 && hasSimulationError && ( - )} {supportsEIP1559 && pendingTransactions?.length > 0 && ( diff --git a/ui/components/ui/simulation-error-message/index.js b/ui/components/ui/simulation-error-message/index.js new file mode 100644 index 000000000..60ba44c74 --- /dev/null +++ b/ui/components/ui/simulation-error-message/index.js @@ -0,0 +1 @@ +export { default } from './simulation-error-message'; diff --git a/ui/components/ui/simulation-error-message/simulation-error-message.js b/ui/components/ui/simulation-error-message/simulation-error-message.js new file mode 100644 index 000000000..11118e4c6 --- /dev/null +++ b/ui/components/ui/simulation-error-message/simulation-error-message.js @@ -0,0 +1,33 @@ +import React, { useContext } from 'react'; +import PropTypes from 'prop-types'; +import ActionableMessage from '../actionable-message'; +import { I18nContext } from '../../../../.storybook/i18n'; + +export default function SimulationErrorMessage({ + userAcknowledgedGasMissing = false, + setUserAcknowledgedGasMissing, +}) { + const t = useContext(I18nContext); + + return ( + + ); +} + +SimulationErrorMessage.propTypes = { + userAcknowledgedGasMissing: PropTypes.bool, + setUserAcknowledgedGasMissing: PropTypes.func, +}; diff --git a/ui/components/ui/simulation-error-message/simulation-error-message.test.js b/ui/components/ui/simulation-error-message/simulation-error-message.test.js new file mode 100644 index 000000000..a3eab13bb --- /dev/null +++ b/ui/components/ui/simulation-error-message/simulation-error-message.test.js @@ -0,0 +1,65 @@ +import React from 'react'; +import configureMockStore from 'redux-mock-store'; +import { fireEvent } from '@testing-library/react'; +import { renderWithProvider } from '../../../../test/lib/render-helpers'; +import SimulationErrorMessage from './simulation-error-message'; + +describe('Simulation Error Message', () => { + const store = configureMockStore()({}); + let props = {}; + + beforeEach(() => { + props = { + userAcknowledgedGasMissing: false, + setUserAcknowledgedGasMissing: jest.fn(), + }; + }); + + it('should render SimulationErrorMessage component with I want to procced anyway link', () => { + const { queryByText } = renderWithProvider( + , + store, + ); + + expect( + queryByText( + 'We were not able to estimate gas. There might be an error in the contract and this transaction may fail.', + ), + ).toBeInTheDocument(); + expect(queryByText('I want to proceed anyway')).toBeInTheDocument(); + }); + + it('should render SimulationErrorMessage component without I want to procced anyway link', () => { + props.userAcknowledgedGasMissing = true; + const { queryByText } = renderWithProvider( + , + store, + ); + + expect( + queryByText( + 'We were not able to estimate gas. There might be an error in the contract and this transaction may fail.', + ), + ).toBeInTheDocument(); + expect(queryByText('I want to proceed anyway')).not.toBeInTheDocument(); + }); + + it('should render SimulationErrorMessage component with I want to proceed anyway and fire that event', () => { + props.userAcknowledgedGasMissing = false; + const { queryByText, getByText } = renderWithProvider( + , + store, + ); + + expect( + queryByText( + 'We were not able to estimate gas. There might be an error in the contract and this transaction may fail.', + ), + ).toBeInTheDocument(); + expect(queryByText('I want to proceed anyway')).toBeInTheDocument(); + + const proceedAnywayLink = getByText('I want to proceed anyway'); + fireEvent.click(proceedAnywayLink); + expect(props.setUserAcknowledgedGasMissing).toHaveBeenCalledTimes(1); + }); +}); diff --git a/ui/hooks/useSimulationFailureWarning.js b/ui/hooks/useSimulationFailureWarning.js new file mode 100644 index 000000000..fe04990df --- /dev/null +++ b/ui/hooks/useSimulationFailureWarning.js @@ -0,0 +1,18 @@ +import { useSelector } from 'react-redux'; +import { txDataSelector } from '../selectors'; + +/** + * Returns the simulation failure warning if a simulaiton error + * is present and user didn't acknowledge gas missing + * + * @param {boolean} userAcknowledgedGasMissing - Whether the user acknowledge gas missing + * @returns {boolean} simulation failure warning + */ + +export function useSimulationFailureWarning(userAcknowledgedGasMissing) { + const txData = useSelector(txDataSelector) || {}; + const hasSimulationError = Boolean(txData.simulationFails); + const renderSimulationFailureWarning = + hasSimulationError && !userAcknowledgedGasMissing; + return renderSimulationFailureWarning; +} diff --git a/ui/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.js b/ui/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.js index ccc9112ba..4ee99ec7f 100644 --- a/ui/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.js +++ b/ui/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.js @@ -9,6 +9,7 @@ import { formatCurrency } from '../../../helpers/utils/confirm-tx.util'; import Typography from '../../../components/ui/typography'; import Box from '../../../components/ui/box'; import Button from '../../../components/ui/button'; +import SimulationErrorMessage from '../../../components/ui/simulation-error-message'; import EditGasFeeButton from '../../../components/app/edit-gas-fee-button'; import MultiLayerFeeMessage from '../../../components/app/multilayer-fee-message'; import { @@ -65,12 +66,15 @@ export default class ConfirmApproveContent extends Component { isSetApproveForAll: PropTypes.bool, isApprovalOrRejection: PropTypes.bool, userAddress: PropTypes.string, + userAcknowledgedGasMissing: PropTypes.bool, + setUserAcknowledgedGasMissing: PropTypes.func, + renderSimulationFailureWarning: PropTypes.bool, }; state = { showFullTxDetails: false, copied: false, - setshowContractDetails: false, + setShowContractDetails: false, }; renderApproveContentCard({ @@ -84,7 +88,11 @@ export default class ConfirmApproveContent extends Component { footer, noBorder, }) { - const { supportsEIP1559 } = this.props; + const { + supportsEIP1559, + renderSimulationFailureWarning, + userAcknowledgedGasMissing, + } = this.props; const { t } = this.context; return (
)} - {showEdit && showAdvanceGasFeeOptions && supportsEIP1559 && ( - - )} + {showEdit && + showAdvanceGasFeeOptions && + supportsEIP1559 && + !renderSimulationFailureWarning && ( + + )}
)}
{content}
@@ -139,9 +152,19 @@ export default class ConfirmApproveContent extends Component { txData, isMultiLayerFeeNetwork, supportsEIP1559, + userAcknowledgedGasMissing, + renderSimulationFailureWarning, } = this.props; - if (!isMultiLayerFeeNetwork && supportsEIP1559) { - return ; + if ( + !isMultiLayerFeeNetwork && + supportsEIP1559 && + !renderSimulationFailureWarning + ) { + return ( + + ); } return (
@@ -491,8 +514,11 @@ export default class ConfirmApproveContent extends Component { tokenId, tokenAddress, assetName, + userAcknowledgedGasMissing, + setUserAcknowledgedGasMissing, + renderSimulationFailureWarning, } = this.props; - const { showFullTxDetails, setshowContractDetails } = this.state; + const { showFullTxDetails, setShowContractDetails } = this.state; return (
this.setState({ setshowContractDetails: true })} + onClick={() => this.setState({ setShowContractDetails: true })} > {t('verifyContractDetails')} - {setshowContractDetails && ( + {setShowContractDetails && ( this.setState({ setshowContractDetails: false })} + onClose={() => this.setState({ setShowContractDetails: false })} tokenName={tokenSymbol} tokenAddress={tokenAddress} toAddress={toAddress} @@ -558,6 +584,21 @@ export default class ConfirmApproveContent extends Component { )}
+ {renderSimulationFailureWarning && ( + + + setUserAcknowledgedGasMissing(true) + } + /> + + )} {this.renderApproveContentCard({ symbol: , title: t('transactionFee'), diff --git a/ui/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.test.js b/ui/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.test.js index 321151d57..647ec2b50 100644 --- a/ui/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.test.js +++ b/ui/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.test.js @@ -28,6 +28,10 @@ const props = { useNonceField: true, nextNonce: 1, customNonceValue: '2', + txData: { simulationFails: null }, + userAcknowledgedGasMissing: false, + setUserAcknowledgedGasMissing: jest.fn(), + renderSimulationFailureWarning: false, showCustomizeNonceModal: jest.fn(), chainId: '1337', rpcPrefs: {}, @@ -50,6 +54,12 @@ describe('ConfirmApproveContent Component', () => { ), ).toBeInTheDocument(); expect(queryByText('Verify contract details')).toBeInTheDocument(); + expect( + queryByText( + 'We were not able to estimate gas. There might be an error in the contract and this transaction may fail.', + ), + ).not.toBeInTheDocument(); + expect(queryByText('I want to proceed anyway')).not.toBeInTheDocument(); expect(queryByText('View full transaction details')).toBeInTheDocument(); const editButtons = getAllByText('Edit'); @@ -86,4 +96,124 @@ describe('ConfirmApproveContent Component', () => { ), ).toBeInTheDocument(); }); + + it('should render Confirm approve page correctly and simulation error message without I want to procced anyway link', () => { + props.userAcknowledgedGasMissing = true; + props.renderSimulationFailureWarning = true; + const { queryByText, getByText, getAllByText, getByTestId } = + renderComponent(props); + expect( + queryByText('https://metamask.github.io/test-dapp/'), + ).toBeInTheDocument(); + expect(getByTestId('confirm-approve-title').textContent).toStrictEqual( + ' Allow access to and transfer of your TestDappCollectibles (#1)? ', + ); + expect( + queryByText( + 'This allows a third party to access and transfer the following NFTs without further notice until you revoke its access.', + ), + ).toBeInTheDocument(); + expect(queryByText('Verify contract details')).toBeInTheDocument(); + expect( + queryByText( + 'We were not able to estimate gas. There might be an error in the contract and this transaction may fail.', + ), + ).toBeInTheDocument(); + expect(queryByText('I want to proceed anyway')).not.toBeInTheDocument(); + expect(queryByText('View full transaction details')).toBeInTheDocument(); + + const editButtons = getAllByText('Edit'); + + expect(queryByText('Transaction fee')).toBeInTheDocument(); + expect( + queryByText('A fee is associated with this request.'), + ).toBeInTheDocument(); + expect(queryByText(`${props.ethTransactionTotal} ETH`)).toBeInTheDocument(); + fireEvent.click(editButtons[0]); + expect(props.showCustomizeGasModal).toHaveBeenCalledTimes(2); + + expect(queryByText('Nonce')).toBeInTheDocument(); + expect(queryByText('2')).toBeInTheDocument(); + fireEvent.click(editButtons[1]); + expect(props.showCustomizeNonceModal).toHaveBeenCalledTimes(2); + + const showViewTxDetails = getByText('View full transaction details'); + expect(queryByText('Permission request')).not.toBeInTheDocument(); + expect(queryByText('Approved asset:')).not.toBeInTheDocument(); + expect(queryByText('Granted to:')).not.toBeInTheDocument(); + expect(queryByText('Data')).not.toBeInTheDocument(); + fireEvent.click(showViewTxDetails); + expect(getByText('Hide full transaction details')).toBeInTheDocument(); + expect(getByText('Permission request')).toBeInTheDocument(); + expect(getByText('Approved asset:')).toBeInTheDocument(); + expect(getByText('Granted to:')).toBeInTheDocument(); + expect(getByText('Contract (0x9bc5baF8...fEF4)')).toBeInTheDocument(); + expect(getByText('Data')).toBeInTheDocument(); + expect(getByText('Function: Approve')).toBeInTheDocument(); + expect( + getByText( + '0x095ea7b30000000000000000000000009bc5baf874d2da8d216ae9f137804184ee5afef40000000000000000000000000000000000000000000000000000000000011170', + ), + ).toBeInTheDocument(); + }); + + it('should render Confirm approve page correctly and simulation error message with I want to procced anyway link', () => { + props.userAcknowledgedGasMissing = false; + props.renderSimulationFailureWarning = true; + const { queryByText, getByText, getAllByText, getByTestId } = + renderComponent(props); + expect( + queryByText('https://metamask.github.io/test-dapp/'), + ).toBeInTheDocument(); + expect(getByTestId('confirm-approve-title').textContent).toStrictEqual( + ' Allow access to and transfer of your TestDappCollectibles (#1)? ', + ); + expect( + queryByText( + 'This allows a third party to access and transfer the following NFTs without further notice until you revoke its access.', + ), + ).toBeInTheDocument(); + expect(queryByText('Verify contract details')).toBeInTheDocument(); + expect( + queryByText( + 'We were not able to estimate gas. There might be an error in the contract and this transaction may fail.', + ), + ).toBeInTheDocument(); + expect(queryByText('I want to proceed anyway')).toBeInTheDocument(); + expect(queryByText('View full transaction details')).toBeInTheDocument(); + + const editButtons = getAllByText('Edit'); + + expect(queryByText('Transaction fee')).toBeInTheDocument(); + expect( + queryByText('A fee is associated with this request.'), + ).toBeInTheDocument(); + expect(queryByText(`${props.ethTransactionTotal} ETH`)).toBeInTheDocument(); + fireEvent.click(editButtons[0]); + expect(props.showCustomizeGasModal).toHaveBeenCalledTimes(3); + + expect(queryByText('Nonce')).toBeInTheDocument(); + expect(queryByText('2')).toBeInTheDocument(); + fireEvent.click(editButtons[1]); + expect(props.showCustomizeNonceModal).toHaveBeenCalledTimes(3); + + const showViewTxDetails = getByText('View full transaction details'); + expect(queryByText('Permission request')).not.toBeInTheDocument(); + expect(queryByText('Approved asset:')).not.toBeInTheDocument(); + expect(queryByText('Granted to:')).not.toBeInTheDocument(); + expect(queryByText('Data')).not.toBeInTheDocument(); + fireEvent.click(showViewTxDetails); + expect(getByText('Hide full transaction details')).toBeInTheDocument(); + expect(getByText('Permission request')).toBeInTheDocument(); + expect(getByText('Approved asset:')).toBeInTheDocument(); + expect(getByText('Granted to:')).toBeInTheDocument(); + expect(getByText('Contract (0x9bc5baF8...fEF4)')).toBeInTheDocument(); + expect(getByText('Data')).toBeInTheDocument(); + expect(getByText('Function: Approve')).toBeInTheDocument(); + expect( + getByText( + '0x095ea7b30000000000000000000000009bc5baf874d2da8d216ae9f137804184ee5afef40000000000000000000000000000000000000000000000000000000000011170', + ), + ).toBeInTheDocument(); + }); }); diff --git a/ui/pages/confirm-approve/confirm-approve-content/confirm-approve-content.stories.js b/ui/pages/confirm-approve/confirm-approve-content/confirm-approve-content.stories.js index 8e4b963e1..efc9b0878 100644 --- a/ui/pages/confirm-approve/confirm-approve-content/confirm-approve-content.stories.js +++ b/ui/pages/confirm-approve/confirm-approve-content/confirm-approve-content.stories.js @@ -129,6 +129,7 @@ export default { useNonceField: true, nextNonce: 1, customNonceValue: '2', + txData: { simulationFails: null }, chainId: '1337', rpcPrefs: {}, isContract: true, diff --git a/ui/pages/confirm-approve/confirm-approve.js b/ui/pages/confirm-approve/confirm-approve.js index 0a9e2bc36..493deef9f 100644 --- a/ui/pages/confirm-approve/confirm-approve.js +++ b/ui/pages/confirm-approve/confirm-approve.js @@ -29,6 +29,7 @@ import { checkNetworkAndAccountSupports1559, } from '../../selectors'; import { useApproveTransaction } from '../../hooks/useApproveTransaction'; +import { useSimulationFailureWarning } from '../../hooks/useSimulationFailureWarning'; import AdvancedGasFeePopover from '../../components/app/advanced-gas-fee-popover'; import EditGasFeePopover from '../../components/app/edit-gas-fee-popover'; import EditGasPopover from '../../components/app/edit-gas-popover/edit-gas-popover.component'; @@ -83,6 +84,8 @@ export default function ConfirmApprove({ const [customPermissionAmount, setCustomPermissionAmount] = useState(''); const [submitWarning, setSubmitWarning] = useState(''); const [isContract, setIsContract] = useState(false); + const [userAcknowledgedGasMissing, setUserAcknowledgedGasMissing] = + useState(false); const supportsEIP1559 = networkAndAccountSupports1559; @@ -92,6 +95,9 @@ export default function ConfirmApprove({ showCustomizeGasPopover, closeCustomizeGasPopover, } = useApproveTransaction(); + const renderSimulationFailureWarning = useSimulationFailureWarning( + userAcknowledgedGasMissing, + ); useEffect(() => { if (customPermissionAmount && previousTokenAmount.current !== tokenAmount) { @@ -246,6 +252,9 @@ export default function ConfirmApprove({ useNonceField={useNonceField} nextNonce={nextNonce} customNonceValue={customNonceValue} + userAcknowledgedGasMissing={userAcknowledgedGasMissing} + setUserAcknowledgedGasMissing={setUserAcknowledgedGasMissing} + renderSimulationFailureWarning={renderSimulationFailureWarning} updateCustomNonce={(value) => { dispatch(updateCustomNonce(value)); }} 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 18b716437..3c914a1da 100644 --- a/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js +++ b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js @@ -15,7 +15,7 @@ import CopyRawData from '../../components/app/transaction-decoding/components/ui import { PRIMARY, SECONDARY } from '../../helpers/constants/common'; import TextField from '../../components/ui/text-field'; -import ActionableMessage from '../../components/ui/actionable-message'; +import SimulationErrorMessage from '../../components/ui/simulation-error-message'; import Disclosure from '../../components/ui/disclosure'; import { EVENT } from '../../../shared/constants/metametrics'; import { @@ -563,18 +563,10 @@ export default class ConfirmTransactionBase extends Component { const simulationFailureWarning = () => (
- this.setUserAcknowledgedGasMissing(), - } + + this.setUserAcknowledgedGasMissing() } />
diff --git a/ui/pages/token-allowance/token-allowance.js b/ui/pages/token-allowance/token-allowance.js index 68b1eece1..05e5cd359 100644 --- a/ui/pages/token-allowance/token-allowance.js +++ b/ui/pages/token-allowance/token-allowance.js @@ -57,6 +57,8 @@ import { NUM_W_OPT_DECIMAL_COMMA_OR_DOT_REGEX, } from '../../../shared/constants/tokens'; import { ConfirmPageContainerNavigation } from '../../components/app/confirm-page-container'; +import { useSimulationFailureWarning } from '../../hooks/useSimulationFailureWarning'; +import SimulationErrorMessage from '../../components/ui/simulation-error-message'; export default function TokenAllowance({ origin, @@ -93,7 +95,12 @@ export default function TokenAllowance({ dappProposedTokenAmount !== '0', ); const [errorText, setErrorText] = useState(''); + const [userAcknowledgedGasMissing, setUserAcknowledgedGasMissing] = + useState(false); + const renderSimulationFailureWarning = useSimulationFailureWarning( + userAcknowledgedGasMissing, + ); const currentAccount = useSelector(getCurrentAccountWithSendEtherInfo); const networkIdentifier = useSelector(getNetworkIdentifier); const rpcPrefs = useSelector(getRpcPrefsForCurrentProvider); @@ -391,6 +398,21 @@ export default function TokenAllowance({ )} {!isFirstPage && ( + {renderSimulationFailureWarning && ( + + + setUserAcknowledgedGasMissing(true) + } + /> + + )} } title={t('transactionFee')} @@ -404,6 +426,8 @@ export default function TokenAllowance({ ethTransactionTotal={ethTransactionTotal} nativeCurrency={nativeCurrency} fullTxData={fullTxData} + userAcknowledgedGasMissing={userAcknowledgedGasMissing} + renderSimulationFailureWarning={renderSimulationFailureWarning} hexTransactionTotal={hexTransactionTotal} fiatTransactionTotal={fiatTransactionTotal} currentCurrency={currentCurrency} @@ -449,6 +473,9 @@ export default function TokenAllowance({ noBorder supportsEIP1559={supportsEIP1559} isSetApproveForAll={isSetApproveForAll} + fullTxData={fullTxData} + userAcknowledgedGasMissing={userAcknowledgedGasMissing} + renderSimulationFailureWarning={renderSimulationFailureWarning} isApprovalOrRejection={isApprovalOrRejection} data={customTxParamsData || data} />