Fix: advance defaults should never be used for swaps (#13617)

Tungez33 · Feb 14, 2022 · 2b5b787 · 2b5b787
1 parent b42e1f7
commit 2b5b787
Show file tree

Hide file tree

Showing 10 changed files with 111 additions and 27 deletions.
diff --git a/app/scripts/controllers/transactions/index.js b/app/scripts/controllers/transactions/index.js
@@ -55,6 +55,11 @@ const hstInterface = new ethers.utils.Interface(abi);
 
 const MAX_MEMSTORE_TX_LIST_SIZE = 100; // Number of transactions (by unique nonces) to keep in memory
 
+const SWAP_TRANSACTION_TYPES = [
+  TRANSACTION_TYPES.SWAP,
+  TRANSACTION_TYPES.SWAP_APPROVAL,
+];
+
 /**
  * @typedef {import('../../../../shared/constants/transaction').TransactionMeta} TransactionMeta
  * @typedef {import('../../../../shared/constants/transaction').TransactionMetaMetricsEventString} TransactionMetaMetricsEventString
@@ -337,9 +342,19 @@ export default class TransactionController extends EventEmitter {
    *
    * @param txParams
    * @param origin
+   * @param transactionType
    * @returns {txMeta}
    */
-  async addUnapprovedTransaction(txParams, origin) {
+  async addUnapprovedTransaction(txParams, origin, transactionType) {
+    if (
+      transactionType !== undefined &&
+      !SWAP_TRANSACTION_TYPES.includes(transactionType)
+    ) {
+      throw new Error(
+        `TransactionController - invalid transactionType value: ${transactionType}`,
+      );
+    }
+
     // validate
     const normalizedTxParams = txUtils.normalizeTxParams(txParams);
     const eip1559Compatibility = await this.getEIP1559Compatibility();
@@ -381,7 +396,7 @@ export default class TransactionController extends EventEmitter {
     const { type, getCodeResponse } = await this._determineTransactionType(
       txParams,
     );
-    txMeta.type = type;
+    txMeta.type = transactionType || type;
 
     // ensure value
     txMeta.txParams.value = txMeta.txParams.value
@@ -444,7 +459,11 @@ export default class TransactionController extends EventEmitter {
     if (eip1559Compatibility) {
       const { eip1559V2Enabled } = this.preferencesStore.getState();
       const advancedGasFeeDefaultValues = this.getAdvancedGasFee();
-      if (eip1559V2Enabled && Boolean(advancedGasFeeDefaultValues)) {
+      if (
+        eip1559V2Enabled &&
+        Boolean(advancedGasFeeDefaultValues) &&
+        !SWAP_TRANSACTION_TYPES.includes(txMeta.type)
+      ) {
         txMeta.userFeeLevel = CUSTOM_GAS_ESTIMATE;
         txMeta.txParams.maxFeePerGas = decGWEIToHexWEI(
           advancedGasFeeDefaultValues.maxBaseFee,
@@ -461,7 +480,7 @@ export default class TransactionController extends EventEmitter {
         //  then we set maxFeePerGas and maxPriorityFeePerGas to the suggested gasPrice.
         txMeta.txParams.maxFeePerGas = txMeta.txParams.gasPrice;
         txMeta.txParams.maxPriorityFeePerGas = txMeta.txParams.gasPrice;
-        if (eip1559V2Enabled) {
+        if (eip1559V2Enabled && txMeta.origin !== 'metamask') {
           txMeta.userFeeLevel = PRIORITY_LEVELS.DAPP_SUGGESTED;
         } else {
           txMeta.userFeeLevel = CUSTOM_GAS_ESTIMATE;

diff --git a/...nents/app/advanced-gas-fee-popover/advanced-gas-fee-defaults/advanced-gas-fee-defaults.js b/...nents/app/advanced-gas-fee-popover/advanced-gas-fee-defaults/advanced-gas-fee-defaults.js
@@ -23,7 +23,7 @@ const AdvancedGasFeeDefaults = () => {
   const t = useI18nContext();
   const dispatch = useDispatch();
   const {
-    hasErrors,
+    gasErrors,
     maxBaseFee,
     maxPriorityFeePerGas,
   } = useAdvancedGasFeePopoverContext();
@@ -86,7 +86,7 @@ const AdvancedGasFeeDefaults = () => {
           checked={isDefaultSettingsSelected}
           className="advanced-gas-fee-defaults__checkbox"
           onClick={handleUpdateDefaultSettings}
-          disabled={hasErrors}
+          disabled={gasErrors.maxFeePerGas || gasErrors.maxPriorityFeePerGas}
         />
         <Typography variant={TYPOGRAPHY.H7} color={COLORS.UI4} margin={0}>
           {isDefaultSettingsSelected

diff --git a/...nts/app/advanced-gas-fee-popover/advanced-gas-fee-inputs/base-fee-input/base-fee-input.js b/...nts/app/advanced-gas-fee-popover/advanced-gas-fee-inputs/base-fee-input/base-fee-input.js
@@ -2,7 +2,10 @@ import React, { useCallback, useEffect, useState } from 'react';
 import { useSelector } from 'react-redux';
 
 import { HIGH_FEE_WARNING_MULTIPLIER } from '../../../../../pages/send/send.constants';
-import { PRIORITY_LEVELS } from '../../../../../../shared/constants/gas';
+import {
+  EDIT_GAS_MODES,
+  PRIORITY_LEVELS,
+} from '../../../../../../shared/constants/gas';
 import { SECONDARY } from '../../../../../helpers/constants/common';
 import {
   bnGreaterThan,
@@ -47,7 +50,12 @@ const validateBaseFee = (value, gasFeeEstimates, maxPriorityFeePerGas) => {
 const BaseFeeInput = () => {
   const t = useI18nContext();
 
-  const { gasFeeEstimates, estimateUsed, maxFeePerGas } = useGasFeeContext();
+  const {
+    gasFeeEstimates,
+    estimateUsed,
+    maxFeePerGas,
+    editGasMode,
+  } = useGasFeeContext();
   const {
     maxPriorityFeePerGas,
     setErrorValue,
@@ -68,7 +76,8 @@ const BaseFeeInput = () => {
   const [baseFee, setBaseFee] = useState(() => {
     if (
       estimateUsed !== PRIORITY_LEVELS.CUSTOM &&
-      advancedGasFeeValues?.maxBaseFee
+      advancedGasFeeValues?.maxBaseFee &&
+      editGasMode !== EDIT_GAS_MODES.SWAPS
     ) {
       return advancedGasFeeValues.maxBaseFee;
     }

diff --git a/...puts/base-fee-input/basefee-input.test.js → ...uts/base-fee-input/base-fee-input.test.js b/...puts/base-fee-input/basefee-input.test.js → ...uts/base-fee-input/base-fee-input.test.js
@@ -1,7 +1,10 @@
 import React from 'react';
 import { fireEvent, screen } from '@testing-library/react';
 
-import { GAS_ESTIMATE_TYPES } from '../../../../../../shared/constants/gas';
+import {
+  EDIT_GAS_MODES,
+  GAS_ESTIMATE_TYPES,
+} from '../../../../../../shared/constants/gas';
 import { renderWithProvider } from '../../../../../../test/lib/render-helpers';
 import mockEstimates from '../../../../../../test/data/mock-estimates.json';
 import mockState from '../../../../../../test/data/mock-state.json';
@@ -20,7 +23,7 @@ jest.mock('../../../../../store/actions', () => ({
   removePollingTokenFromAppState: jest.fn(),
 }));
 
-const render = (txProps) => {
+const render = (txProps, contextProps) => {
   const store = configureStore({
     metamask: {
       ...mockState.metamask,
@@ -43,6 +46,7 @@ const render = (txProps) => {
         userFeeLevel: 'custom',
         ...txProps,
       }}
+      {...contextProps}
     >
       <AdvancedGasFeePopoverContextProvider>
         <BaseFeeInput />
@@ -56,17 +60,33 @@ describe('BaseFeeInput', () => {
   it('should renders advancedGasFee.baseFee value if current estimate used is not custom', () => {
     render({
       userFeeLevel: 'high',
+      txParams: {
+        maxFeePerGas: '0x2E90EDD000',
+      },
     });
     expect(document.getElementsByTagName('input')[0]).toHaveValue(100);
   });
 
+  it('should not advancedGasFee.baseFee value for swaps', () => {
+    render(
+      {
+        userFeeLevel: 'high',
+        txParams: {
+          maxFeePerGas: '0x2E90EDD000',
+        },
+      },
+      { editGasMode: EDIT_GAS_MODES.SWAPS },
+    );
+    expect(document.getElementsByTagName('input')[0]).toHaveValue(200);
+  });
+
   it('should renders baseFee values from transaction if current estimate used is custom', () => {
     render({
       txParams: {
-        maxFeePerGas: '0x174876E800',
+        maxFeePerGas: '0x2E90EDD000',
       },
     });
-    expect(document.getElementsByTagName('input')[0]).toHaveValue(100);
+    expect(document.getElementsByTagName('input')[0]).toHaveValue(200);
   });
   it('should show current value of estimatedBaseFee in subtext', () => {
     render({

diff --git a/...advanced-gas-fee-popover/advanced-gas-fee-inputs/priority-fee-input/priority-fee-input.js b/...advanced-gas-fee-popover/advanced-gas-fee-inputs/priority-fee-input/priority-fee-input.js
@@ -2,7 +2,10 @@ import React, { useEffect, useState } from 'react';
 import { useSelector } from 'react-redux';
 
 import { HIGH_FEE_WARNING_MULTIPLIER } from '../../../../../pages/send/send.constants';
-import { PRIORITY_LEVELS } from '../../../../../../shared/constants/gas';
+import {
+  EDIT_GAS_MODES,
+  PRIORITY_LEVELS,
+} from '../../../../../../shared/constants/gas';
 import { SECONDARY } from '../../../../../helpers/constants/common';
 import { decGWEIToHexWEI } from '../../../../../helpers/utils/conversions.util';
 import { getAdvancedGasFeeValues } from '../../../../../selectors';
@@ -49,6 +52,7 @@ const PriorityFeeInput = () => {
     setMaxPriorityFeePerGas,
   } = useAdvancedGasFeePopoverContext();
   const {
+    editGasMode,
     estimateUsed,
     gasFeeEstimates,
     maxPriorityFeePerGas,
@@ -63,7 +67,8 @@ const PriorityFeeInput = () => {
   const [priorityFee, setPriorityFee] = useState(() => {
     if (
       estimateUsed !== PRIORITY_LEVELS.CUSTOM &&
-      advancedGasFeeValues?.priorityFee
+      advancedGasFeeValues?.priorityFee &&
+      editGasMode !== EDIT_GAS_MODES.SWAPS
     ) {
       return advancedGasFeeValues.priorityFee;
     }

diff --git a/...ced-gas-fee-popover/advanced-gas-fee-inputs/priority-fee-input/priority-fee-input.test.js b/...ced-gas-fee-popover/advanced-gas-fee-inputs/priority-fee-input/priority-fee-input.test.js
@@ -1,7 +1,10 @@
 import React from 'react';
 import { fireEvent, screen } from '@testing-library/react';
 
-import { GAS_ESTIMATE_TYPES } from '../../../../../../shared/constants/gas';
+import {
+  EDIT_GAS_MODES,
+  GAS_ESTIMATE_TYPES,
+} from '../../../../../../shared/constants/gas';
 import { renderWithProvider } from '../../../../../../test/lib/render-helpers';
 import mockEstimates from '../../../../../../test/data/mock-estimates.json';
 import mockState from '../../../../../../test/data/mock-state.json';
@@ -20,7 +23,7 @@ jest.mock('../../../../../store/actions', () => ({
   removePollingTokenFromAppState: jest.fn(),
 }));
 
-const render = (txProps) => {
+const render = (txProps, contextProps) => {
   const store = configureStore({
     metamask: {
       ...mockState.metamask,
@@ -43,6 +46,7 @@ const render = (txProps) => {
         userFeeLevel: 'custom',
         ...txProps,
       }}
+      {...contextProps}
     >
       <AdvancedGasFeePopoverContextProvider>
         <PriorityfeeInput />
@@ -56,10 +60,26 @@ describe('PriorityfeeInput', () => {
   it('should renders advancedGasFee.priorityfee value if current estimate used is not custom', () => {
     render({
       userFeeLevel: 'high',
+      txParams: {
+        maxFeePerGas: '0x2E90EDD000',
+      },
     });
     expect(document.getElementsByTagName('input')[0]).toHaveValue(100);
   });
 
+  it('should not advancedGasFee.baseFee value for swaps', () => {
+    render(
+      {
+        userFeeLevel: 'high',
+        txParams: {
+          maxFeePerGas: '0x2E90EDD000',
+        },
+      },
+      { editGasMode: EDIT_GAS_MODES.SWAPS },
+    );
+    expect(document.getElementsByTagName('input')[0]).toHaveValue(200);
+  });
+
   it('should renders priorityfee value from transaction if current estimate used is custom', () => {
     render({
       txParams: {

diff --git a/ui/components/app/advanced-gas-fee-popover/context/advancedGasFeePopover.js b/ui/components/app/advanced-gas-fee-popover/context/advancedGasFeePopover.js
@@ -29,6 +29,7 @@ export const AdvancedGasFeePopoverContextProvider = ({ children }) => {
         gasLimit,
         hasErrors:
           errors.maxFeePerGas || errors.maxPriorityFeePerGas || errors.gasLimit,
+        gasErrors: errors,
         maxFeePerGas,
         maxPriorityFeePerGas,
         setErrorValue,

diff --git a/ui/components/app/edit-gas-fee-popover/edit-gas-item/useGasItemFeeDetails.js b/ui/components/app/edit-gas-fee-popover/edit-gas-item/useGasItemFeeDetails.js
@@ -58,7 +58,7 @@ export const useGasItemFeeDetails = (priorityLevel) => {
     if (estimateUsed === PRIORITY_LEVELS.CUSTOM) {
       maxFeePerGas = maxFeePerGasValue;
       maxPriorityFeePerGas = maxPriorityFeePerGasValue;
-    } else if (advancedGasFeeValues) {
+    } else if (advancedGasFeeValues && editGasMode !== EDIT_GAS_MODES.SWAPS) {
       maxFeePerGas = advancedGasFeeValues.maxBaseFee;
       maxPriorityFeePerGas = advancedGasFeeValues.priorityFee;
     }

diff --git a/ui/ducks/swaps/swaps.js b/ui/ducks/swaps/swaps.js
@@ -857,6 +857,7 @@ export const signAndSendTransactions = (history, metaMetricsEvent) => {
         addUnapprovedTransaction(
           { ...approveTxParams, amount: '0x0' },
           'metamask',
+          TRANSACTION_TYPES.SWAP_APPROVAL,
         ),
       );
       await dispatch(setApproveTxId(approveTxMeta.id));
@@ -881,7 +882,11 @@ export const signAndSendTransactions = (history, metaMetricsEvent) => {
     }
 
     const tradeTxMeta = await dispatch(
-      addUnapprovedTransaction(usedTradeTxParams, 'metamask'),
+      addUnapprovedTransaction(
+        usedTradeTxParams,
+        'metamask',
+        TRANSACTION_TYPES.SWAP,
+      ),
     );
     dispatch(setTradeTxId(tradeTxMeta.id));
 

diff --git a/ui/store/actions.js b/ui/store/actions.js
@@ -698,18 +698,23 @@ export function updateTransaction(txData, dontShowLoadingIndicator) {
   };
 }
 
-export function addUnapprovedTransaction(txParams, origin) {
+export function addUnapprovedTransaction(txParams, origin, type) {
   log.debug('background.addUnapprovedTransaction');
 
   return () => {
     return new Promise((resolve, reject) => {
-      background.addUnapprovedTransaction(txParams, origin, (err, txMeta) => {
-        if (err) {
-          reject(err);
-          return;
-        }
-        resolve(txMeta);
-      });
+      background.addUnapprovedTransaction(
+        txParams,
+        origin,
+        type,
+        (err, txMeta) => {
+          if (err) {
+            reject(err);
+            return;
+          }
+          resolve(txMeta);
+        },
+      );
     });
   };
 }