From 738d2722c5ffb639a4d9f20e564fda5d28bdf09a Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Fri, 6 Aug 2021 17:01:30 -0230 Subject: [PATCH] Fixes updates on the confirm screen. (#11788) * Fixes updates on the confirm screen. * Better handling of internal send transactions * maxFee -> maxFeePerGas property name fix * Remove redundant setEstimateToUse call in onManualChange * Fix unit tests * rebase error fix * Fixes to speedup loading and transaction breakdown priority fee * Fix lint and unit tests * Ensure gas price based transaction that have been customized (e.g. speed up and retry) are properly initialized in useGasFeeInputs * Clean up * Link fix --- app/scripts/controllers/swaps.js | 8 ++ app/scripts/controllers/swaps.test.js | 1 + app/scripts/controllers/transactions/index.js | 13 +++ app/scripts/metamask-controller.js | 4 + .../advanced-gas-controls.component.js | 3 +- .../edit-gas-display.component.js | 10 ++- .../edit-gas-popover.component.js | 22 +++-- .../transaction-list-item.component.js | 2 + ui/ducks/swaps/swaps.js | 3 + ui/helpers/utils/conversions.util.js | 2 +- ui/hooks/useGasFeeInputs.js | 90 +++++++------------ ...onfirm-token-transaction-base.container.js | 25 +++--- .../confirm-transaction-base.component.js | 39 +++++--- .../confirm-transaction-base.container.js | 6 +- ui/pages/swaps/view-quote/view-quote.js | 7 +- ui/selectors/confirm-transaction.js | 25 ++++-- ui/store/actions.js | 7 ++ 17 files changed, 156 insertions(+), 111 deletions(-) diff --git a/app/scripts/controllers/swaps.js b/app/scripts/controllers/swaps.js index 1615b2594..238dba04c 100644 --- a/app/scripts/controllers/swaps.js +++ b/app/scripts/controllers/swaps.js @@ -73,6 +73,7 @@ const initialState = { customGasPrice: null, customMaxFeePerGas: null, customMaxPriorityFeePerGas: null, + swapsUserFeeLevel: '', selectedAggId: null, customApproveTxData: '', errorKey: '', @@ -456,6 +457,13 @@ export default class SwapsController { }); } + setSwapsUserFeeLevel(swapsUserFeeLevel) { + const { swapsState } = this.store.getState(); + this.store.updateState({ + swapsState: { ...swapsState, swapsUserFeeLevel }, + }); + } + setSwapsTxMaxFeePriorityPerGas(maxPriorityFeePerGas) { const { swapsState } = this.store.getState(); this.store.updateState({ diff --git a/app/scripts/controllers/swaps.test.js b/app/scripts/controllers/swaps.test.js index 25d7738aa..4ed83ef25 100644 --- a/app/scripts/controllers/swaps.test.js +++ b/app/scripts/controllers/swaps.test.js @@ -133,6 +133,7 @@ const EMPTY_INIT_STATE = { swapsFeatureIsLive: true, useNewSwapsApi: false, swapsQuoteRefreshTime: 60000, + swapsUserFeeLevel: '', }, }; diff --git a/app/scripts/controllers/transactions/index.js b/app/scripts/controllers/transactions/index.js index af85a0310..9afa217e1 100644 --- a/app/scripts/controllers/transactions/index.js +++ b/app/scripts/controllers/transactions/index.js @@ -433,7 +433,20 @@ export default class TransactionController extends EventEmitter { ) { txMeta.txParams.maxFeePerGas = txMeta.txParams.gasPrice; txMeta.txParams.maxPriorityFeePerGas = txMeta.txParams.gasPrice; + txMeta.userFeeLevel = 'custom'; } else { + if ( + (defaultMaxFeePerGas && + defaultMaxPriorityFeePerGas && + !txMeta.txParams.maxFeePerGas && + !txMeta.txParams.maxPriorityFeePerGas) || + txMeta.origin === 'metamask' + ) { + txMeta.userFeeLevel = 'medium'; + } else { + txMeta.userFeeLevel = 'custom'; + } + if (defaultMaxFeePerGas && !txMeta.txParams.maxFeePerGas) { // If the dapp has not set the gasPrice or the maxFeePerGas, then we set maxFeePerGas // with the one returned by the gasFeeController, if that is available. diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index f1c2cba85..adef54dbb 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -1089,6 +1089,10 @@ export default class MetamaskController extends EventEmitter { swapsController.setSwapsLiveness, swapsController, ), + setSwapsUserFeeLevel: nodeify( + swapsController.setSwapsUserFeeLevel, + swapsController, + ), // MetaMetrics trackMetaMetricsEvent: nodeify( diff --git a/ui/components/app/advanced-gas-controls/advanced-gas-controls.component.js b/ui/components/app/advanced-gas-controls/advanced-gas-controls.component.js index 75281a2e0..ee9f3429d 100644 --- a/ui/components/app/advanced-gas-controls/advanced-gas-controls.component.js +++ b/ui/components/app/advanced-gas-controls/advanced-gas-controls.component.js @@ -40,7 +40,8 @@ export default function AdvancedGasControls({ const showFeeMarketFields = networkAndAccountSupport1559 && (gasEstimateType === GAS_ESTIMATE_TYPES.FEE_MARKET || - gasEstimateType === GAS_ESTIMATE_TYPES.ETH_GASPRICE); + gasEstimateType === GAS_ESTIMATE_TYPES.ETH_GASPRICE || + isGasEstimatesLoading); return (
diff --git a/ui/components/app/edit-gas-display/edit-gas-display.component.js b/ui/components/app/edit-gas-display/edit-gas-display.component.js index 28dc11458..26c15ad9c 100644 --- a/ui/components/app/edit-gas-display/edit-gas-display.component.js +++ b/ui/components/app/edit-gas-display/edit-gas-display.component.js @@ -77,7 +77,9 @@ export default function EditGasDisplay({ ); const [showAdvancedForm, setShowAdvancedForm] = useState( - !estimateToUse || !networkAndAccountSupport1559, + !estimateToUse || + estimateToUse === 'custom' || + !networkAndAccountSupport1559, ); const [hideRadioButtons, setHideRadioButtons] = useState( showAdvancedInlineGasIfPossible, @@ -110,7 +112,7 @@ export default function EditGasDisplay({ return (
- {warning && ( + {warning && !isGasEstimatesLoading && (
)} - {showTopError && ( + {showTopError && !isGasEstimatesLoading && (
)} - {requireDappAcknowledgement && ( + {requireDappAcknowledgement && !isGasEstimatesLoading && (
export const getCustomMaxPriorityFeePerGas = (state) => state.metamask.swapsState.customMaxPriorityFeePerGas; +export const getSwapsUserFeeLevel = (state) => + state.metamask.swapsState.swapsUserFeeLevel; + export const getFetchParams = (state) => state.metamask.swapsState.fetchParams; export const getQuotes = (state) => state.metamask.swapsState.quotes; diff --git a/ui/helpers/utils/conversions.util.js b/ui/helpers/utils/conversions.util.js index 78a28293b..cdfee12b9 100644 --- a/ui/helpers/utils/conversions.util.js +++ b/ui/helpers/utils/conversions.util.js @@ -172,7 +172,7 @@ export function addHexes(aHexWEI, bHexWEI) { } export function subtractHexes(aHexWEI, bHexWEI) { - return addCurrencies(aHexWEI, bHexWEI, { + return subtractCurrencies(aHexWEI, bHexWEI, { aBase: 16, bBase: 16, toNumericBase: 'hex', diff --git a/ui/hooks/useGasFeeInputs.js b/ui/hooks/useGasFeeInputs.js index 8dda30e21..37da6a796 100644 --- a/ui/hooks/useGasFeeInputs.js +++ b/ui/hooks/useGasFeeInputs.js @@ -1,7 +1,7 @@ import { addHexPrefix } from 'ethereumjs-util'; import { useCallback, useState } from 'react'; import { useSelector } from 'react-redux'; -import { findKey, isEqual } from 'lodash'; +import { isEqual } from 'lodash'; import { GAS_ESTIMATE_TYPES, EDIT_GAS_MODES, @@ -103,32 +103,6 @@ function getGasFeeEstimate( return String(fallback); } -/** - * This method tries to determine if any estimate level matches the - * current maxFeePerGas and maxPriorityFeePerGas values. If we find - * a match, we can pre-select a radio button in the RadioGroup - */ -function getMatchingEstimateFromGasFees( - gasFeeEstimates, - maxFeePerGas, - maxPriorityFeePerGas, - gasPrice, - networkAndAccountSupports1559, -) { - return ( - findKey(gasFeeEstimates, (estimate) => { - if (networkAndAccountSupports1559) { - return ( - Number(estimate?.suggestedMaxPriorityFeePerGas) === - Number(maxPriorityFeePerGas) && - Number(estimate?.suggestedMaxFeePerGas) === Number(maxFeePerGas) - ); - } - return estimate?.gasPrice === gasPrice; - }) || null - ); -} - /** * @typedef {Object} GasFeeInputReturnType * @property {DecGweiString} [maxFeePerGas] - the maxFeePerGas input value. @@ -229,33 +203,30 @@ export function useGasFeeInputs( ); const [initialMatchingEstimateLevel] = useState( - getMatchingEstimateFromGasFees( - gasFeeEstimates, - initialMaxFeePerGas, - initialMaxPriorityFeePerGas, - initialGasPrice, - networkAndAccountSupports1559, - ), + transaction?.userFeeLevel || null, ); + const initialFeeParamsAreCustom = + initialMatchingEstimateLevel === 'custom' || + initialMatchingEstimateLevel === null; // This hook keeps track of a few pieces of transitional state. It is // transitional because it is only used to modify a transaction in the // metamask (background) state tree. const [maxFeePerGas, setMaxFeePerGas] = useState( - initialMaxFeePerGas && !initialMatchingEstimateLevel + initialMaxFeePerGas && initialFeeParamsAreCustom ? initialMaxFeePerGas : null, ); const [maxPriorityFeePerGas, setMaxPriorityFeePerGas] = useState( - initialMaxPriorityFeePerGas && !initialMatchingEstimateLevel + initialMaxPriorityFeePerGas && initialFeeParamsAreCustom ? initialMaxPriorityFeePerGas : null, ); const [gasPriceHasBeenManuallySet, setGasPriceHasBeenManuallySet] = useState( - false, + initialMatchingEstimateLevel === 'custom', ); const [gasPrice, setGasPrice] = useState( - initialGasPrice && !initialMatchingEstimateLevel ? initialGasPrice : null, + initialGasPrice && initialFeeParamsAreCustom ? initialGasPrice : null, ); const [gasLimit, setGasLimit] = useState( Number(hexToDecimal(transaction?.txParams?.gas ?? minimumGasLimit)), @@ -265,7 +236,7 @@ export function useGasFeeInputs( const dontDefaultToAnEstimateLevel = userPrefersAdvancedGas && transaction?.txParams?.maxPriorityFeePerGas && - transaction?.txParams?.gasPrice; + transaction?.txParams?.maxFeePerGas; const initialEstimateToUse = transaction ? initialMatchingEstimateLevel @@ -514,28 +485,28 @@ export function useGasFeeInputs( { value: ethBalance, fromNumericBase: 'hex' }, ); + const handleGasLimitOutOfBoundError = useCallback(() => { + if (gasErrors.gasLimit === GAS_FORM_ERRORS.GAS_LIMIT_OUT_OF_BOUNDS) { + const transactionGasLimitDec = hexToDecimal(transaction?.txParams?.gas); + const minimumGasLimitDec = hexToDecimal(minimumGasLimit); + setGasLimit( + transactionGasLimitDec > minimumGasLimitDec + ? transactionGasLimitDec + : minimumGasLimitDec, + ); + } + }, [minimumGasLimit, gasErrors.gasLimit, transaction]); // When a user selects an estimate level, it will wipe out what they have // previously put in the inputs. This returns the inputs to the estimated // values at the level specified. - const setEstimateToUse = useCallback( - (estimateLevel) => { - setInternalEstimateToUse(estimateLevel); - if (gasErrors.gasLimit === GAS_FORM_ERRORS.GAS_LIMIT_OUT_OF_BOUNDS) { - const transactionGasLimit = hexToDecimal(transaction?.txParams?.gas); - const minimumGasLimitDec = hexToDecimal(minimumGasLimit); - setGasLimit( - transactionGasLimit > minimumGasLimitDec - ? transactionGasLimit - : minimumGasLimitDec, - ); - } - setMaxFeePerGas(null); - setMaxPriorityFeePerGas(null); - setGasPrice(null); - setGasPriceHasBeenManuallySet(false); - }, - [minimumGasLimit, gasErrors.gasLimit, transaction], - ); + const setEstimateToUse = (estimateLevel) => { + setInternalEstimateToUse(estimateLevel); + handleGasLimitOutOfBoundError(); + setMaxFeePerGas(null); + setMaxPriorityFeePerGas(null); + setGasPrice(null); + setGasPriceHasBeenManuallySet(false); + }; return { maxFeePerGas: maxFeePerGasToUse, @@ -561,7 +532,8 @@ export function useGasFeeInputs( gasErrors: errorsAndWarnings, hasGasErrors: hasBlockingGasErrors, onManualChange: () => { - setEstimateToUse(null); + setInternalEstimateToUse('custom'); + handleGasLimitOutOfBoundError(); // Restore existing values setGasPrice(gasPriceToUse); setGasLimit(gasLimit); diff --git a/ui/pages/confirm-token-transaction-base/confirm-token-transaction-base.container.js b/ui/pages/confirm-token-transaction-base/confirm-token-transaction-base.container.js index 58c093bea..aec55e531 100644 --- a/ui/pages/confirm-token-transaction-base/confirm-token-transaction-base.container.js +++ b/ui/pages/confirm-token-transaction-base/confirm-token-transaction-base.container.js @@ -13,7 +13,6 @@ import { getTokenValueParam, } from '../../helpers/utils/token-util'; import { hexWEIToDecETH } from '../../helpers/utils/conversions.util'; -import { getHexGasTotal } from '../../helpers/utils/confirm-tx.util'; import ConfirmTokenTransactionBase from './confirm-token-transaction-base.component'; const mapStateToProps = (state, ownProps) => { @@ -34,32 +33,28 @@ const mapStateToProps = (state, ownProps) => { const { txData: { id: transactionId, - txParams: { to: tokenAddress, data, maxFeePerGas, gasPrice, gas } = {}, + txParams: { to: tokenAddress, data } = {}, } = {}, } = confirmTransaction; - const ethTransactionTotalMaxAmount = Number( - hexWEIToDecETH( - getHexGasTotal({ - gasPrice: maxFeePerGas ?? gasPrice, - gasLimit: gas, - }), - ), - ).toFixed(6); - const transaction = currentNetworkTxList.find( ({ id }) => id === (Number(paramsTransactionId) || transactionId), ) || {}; - const { ethTransactionTotal, fiatTransactionTotal } = transactionFeeSelector( - state, - transaction, - ); + const { + ethTransactionTotal, + fiatTransactionTotal, + hexMaximumTransactionFee, + } = transactionFeeSelector(state, transaction); const tokens = getTokens(state); const currentToken = tokens?.find(({ address }) => tokenAddress === address); const { decimals, symbol: tokenSymbol } = currentToken || {}; + const ethTransactionTotalMaxAmount = Number( + hexWEIToDecETH(hexMaximumTransactionFee), + ).toFixed(6); + const tokenData = getTokenData(data); const tokenValue = getTokenValueParam(tokenData); const toAddress = getTokenAddressParam(tokenData); 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 c26671323..e15b80126 100644 --- a/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js +++ b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js @@ -4,7 +4,6 @@ import { ENVIRONMENT_TYPE_NOTIFICATION } from '../../../shared/constants/app'; import { getEnvironmentType } from '../../../app/scripts/lib/util'; import ConfirmPageContainer from '../../components/app/confirm-page-container'; import { isBalanceSufficient } from '../send/send.utils'; -import { getHexGasTotal } from '../../helpers/utils/confirm-tx.util'; import { addHexes, hexToDecimal, @@ -110,6 +109,8 @@ export default class ConfirmTransactionBase extends Component { gasIsLoading: PropTypes.bool, primaryTotalTextOverrideMaxAmount: PropTypes.string, useNativeCurrencyAsPrimaryCurrency: PropTypes.bool, + maxFeePerGas: PropTypes.string, + maxPriorityFeePerGas: PropTypes.string, }; state = { @@ -275,6 +276,7 @@ export default class ConfirmTransactionBase extends Component { primaryTotalTextOverride, secondaryTotalTextOverride, hexMinimumTransactionFee, + hexMaximumTransactionFee, hexTransactionTotal, useNonceField, customNonceValue, @@ -283,8 +285,9 @@ export default class ConfirmTransactionBase extends Component { getNextNonce, txData, useNativeCurrencyAsPrimaryCurrency, - hexMaximumTransactionFee, primaryTotalTextOverrideMaxAmount, + maxFeePerGas, + maxPriorityFeePerGas, } = this.props; const { t } = this.context; @@ -305,14 +308,7 @@ export default class ConfirmTransactionBase extends Component { return ( ); @@ -467,9 +463,12 @@ export default class ConfirmTransactionBase extends Component { subTitle={ } />, @@ -611,6 +610,8 @@ export default class ConfirmTransactionBase extends Component { history, mostRecentOverviewPage, updateCustomNonce, + maxFeePerGas, + maxPriorityFeePerGas, } = this.props; const { submitting } = this.state; @@ -618,6 +619,20 @@ export default class ConfirmTransactionBase extends Component { return; } + if (maxFeePerGas) { + txData.txParams = { + ...txData.txParams, + maxFeePerGas, + }; + } + + if (maxPriorityFeePerGas) { + txData.txParams = { + ...txData.txParams, + maxPriorityFeePerGas, + }; + } + this.setState( { submitting: true, 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 4c8120553..6ccaf561d 100644 --- a/ui/pages/confirm-transaction-base/confirm-transaction-base.container.js +++ b/ui/pages/confirm-transaction-base/confirm-transaction-base.container.js @@ -118,6 +118,7 @@ const mapStateToProps = (state, ownProps) => { hexMinimumTransactionFee, hexMaximumTransactionFee, hexTransactionTotal, + gasEstimationObject, } = transactionFeeSelector(state, transaction); if (transaction && transaction.simulationFails) { @@ -152,8 +153,9 @@ const mapStateToProps = (state, ownProps) => { } customNonceValue = getCustomNonceValue(state); const isEthGasPrice = getIsEthGasPriceFetched(state); - const noGasPrice = getNoGasPriceFetched(state); + const noGasPrice = !supportsEIP1599 && getNoGasPriceFetched(state); const { useNativeCurrencyAsPrimaryCurrency } = getPreferences(state); + return { balance, fromAddress, @@ -196,6 +198,8 @@ const mapStateToProps = (state, ownProps) => { supportsEIP1599, gasIsLoading: isGasEstimatesLoading || gasLoadingAnimationIsShowing, useNativeCurrencyAsPrimaryCurrency, + maxFeePerGas: gasEstimationObject.maxFeePerGas, + maxPriorityFeePerGas: gasEstimationObject.maxPriorityFeePerGas, }; }; diff --git a/ui/pages/swaps/view-quote/view-quote.js b/ui/pages/swaps/view-quote/view-quote.js index e71875cc6..cdfbf0195 100644 --- a/ui/pages/swaps/view-quote/view-quote.js +++ b/ui/pages/swaps/view-quote/view-quote.js @@ -26,6 +26,7 @@ import { getCustomSwapsGas, // Gas limit. getCustomMaxFeePerGas, getCustomMaxPriorityFeePerGas, + getSwapsUserFeeLevel, getDestinationTokenInfo, getUsedSwapsGasPrice, getTopQuote, @@ -128,6 +129,7 @@ export default function ViewQuote() { const customMaxGas = useSelector(getCustomSwapsGas); const customMaxFeePerGas = useSelector(getCustomMaxFeePerGas); const customMaxPriorityFeePerGas = useSelector(getCustomMaxPriorityFeePerGas); + const swapsUserFeeLevel = useSelector(getSwapsUserFeeLevel); const tokenConversionRates = useSelector(getTokenExchangeRates); const memoizedTokenConversionRates = useEqualityCheck(tokenConversionRates); const { balance: ethBalance } = useSelector(getSelectedAccount); @@ -153,7 +155,9 @@ export default function ViewQuote() { if (networkAndAccountSupports1559) { // For Swaps we want to get 'high' estimations by default. // eslint-disable-next-line react-hooks/rules-of-hooks - gasFeeInputs = useGasFeeInputs('high'); + gasFeeInputs = useGasFeeInputs('high', { + userFeeLevel: swapsUserFeeLevel || 'high', + }); } const { isBestQuote } = usedQuote; @@ -670,6 +674,7 @@ export default function ViewQuote() { {showEditGasPopover && networkAndAccountSupports1559 && ( { + await promisifiedBackground.setSwapsUserFeeLevel(swapsCustomUserFeeLevel); + await forceUpdateMetamaskState(dispatch); + }; +} + export function customSwapsGasParamsUpdated(gasLimit, gasPrice) { return async (dispatch) => { await promisifiedBackground.setSwapsTxGasPrice(gasPrice);