From 8c5fdf779c14cb4215ba93ef418ae7a74576f7a7 Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Thu, 9 Dec 2021 04:26:10 +0530 Subject: [PATCH] Fixing EIP-1559 V2 advance gas fee inputs validations (#13013) --- app/_locales/en/messages.json | 2 +- .../advanced-gas-fee-gas-limit.js | 6 +---- .../base-fee-input/base-fee-input.js | 12 ++++------ .../priority-fee-input/priority-fee-input.js | 13 ++++++----- .../priority-fee-input.test.js | 18 +++++++++++++++ .../advanced-gas-fee-popover.test.js | 21 +++++++++++------ .../advanced-gas-fee-save.js | 5 ++-- .../context/advancedGasFeePopover.js | 23 +++++++++++++------ 8 files changed, 64 insertions(+), 36 deletions(-) diff --git a/app/_locales/en/messages.json b/app/_locales/en/messages.json index 01280d57f..991b1bd4e 100644 --- a/app/_locales/en/messages.json +++ b/app/_locales/en/messages.json @@ -845,7 +845,7 @@ "message": "Max priority fee must be greater than 0 GWEI" }, "editGasMaxPriorityFeeBelowMinimumV2": { - "message": "Priority fee must be at least 1 GWEI" + "message": "Priority fee must be greater than 0." }, "editGasMaxPriorityFeeHigh": { "message": "Max priority fee is higher than necessary. You may pay more than needed." diff --git a/ui/components/app/advanced-gas-fee-popover/advanced-gas-fee-gas-limit/advanced-gas-fee-gas-limit.js b/ui/components/app/advanced-gas-fee-popover/advanced-gas-fee-gas-limit/advanced-gas-fee-gas-limit.js index 8cb52318a..e1efdfc00 100644 --- a/ui/components/app/advanced-gas-fee-popover/advanced-gas-fee-gas-limit/advanced-gas-fee-gas-limit.js +++ b/ui/components/app/advanced-gas-fee-popover/advanced-gas-fee-gas-limit/advanced-gas-fee-gas-limit.js @@ -22,9 +22,7 @@ const validateGasLimit = (gasLimit, minimumGasLimitDec) => { const AdvancedGasFeeGasLimit = () => { const t = useI18nContext(); const { - setDirty, setGasLimit: setGasLimitInContext, - setHasError, } = useAdvancedGasFeePopoverContext(); const { gasLimit: gasLimitInTransaction, @@ -36,15 +34,13 @@ const AdvancedGasFeeGasLimit = () => { const updateGasLimit = (value) => { setGasLimit(value); - setDirty(true); }; useEffect(() => { setGasLimitInContext(gasLimit); const error = validateGasLimit(gasLimit, minimumGasLimitDec); setGasLimitError(error); - setHasError(Boolean(error)); - }, [gasLimit, minimumGasLimitDec, setGasLimitInContext, setHasError]); + }, [gasLimit, minimumGasLimitDec, setGasLimitInContext]); if (isEditing) { return ( diff --git a/ui/components/app/advanced-gas-fee-popover/advanced-gas-fee-inputs/base-fee-input/base-fee-input.js b/ui/components/app/advanced-gas-fee-popover/advanced-gas-fee-inputs/base-fee-input/base-fee-input.js index a2c62e4eb..85e17883e 100644 --- a/ui/components/app/advanced-gas-fee-popover/advanced-gas-fee-inputs/base-fee-input/base-fee-input.js +++ b/ui/components/app/advanced-gas-fee-popover/advanced-gas-fee-inputs/base-fee-input/base-fee-input.js @@ -67,10 +67,9 @@ const BaseFeeInput = () => { const t = useI18nContext(); const { gasFeeEstimates, estimateUsed, maxFeePerGas } = useGasFeeContext(); const { - setDirty, - setHasError, - setMaxFeePerGas, maxPriorityFeePerGas, + setErrorValue, + setMaxFeePerGas, } = useAdvancedGasFeePopoverContext(); const { estimatedBaseFee } = gasFeeEstimates; @@ -133,7 +132,6 @@ const BaseFeeInput = () => { } setMaxBaseFeeGWEI(baseFeeInGWEI); setMaxBaseFeeMultiplier(baseFeeMultiplierValue); - setDirty(true); }, [ editingInGwei, @@ -141,7 +139,6 @@ const BaseFeeInput = () => { numberOfDecimalsPrimary, setMaxBaseFeeGWEI, setMaxBaseFeeMultiplier, - setDirty, ], ); @@ -152,14 +149,15 @@ const BaseFeeInput = () => { gasFeeEstimates, maxPriorityFeePerGas, ); + setBaseFeeError(error); - setHasError(Boolean(error)); + setErrorValue('maxFeePerGas', error === 'editGasMaxBaseFeeImbalance'); }, [ gasFeeEstimates, maxBaseFeeGWEI, maxPriorityFeePerGas, - setHasError, setBaseFeeError, + setErrorValue, setMaxFeePerGas, ]); diff --git a/ui/components/app/advanced-gas-fee-popover/advanced-gas-fee-inputs/priority-fee-input/priority-fee-input.js b/ui/components/app/advanced-gas-fee-popover/advanced-gas-fee-inputs/priority-fee-input/priority-fee-input.js index e023a8777..fbf1da2b5 100644 --- a/ui/components/app/advanced-gas-fee-popover/advanced-gas-fee-inputs/priority-fee-input/priority-fee-input.js +++ b/ui/components/app/advanced-gas-fee-popover/advanced-gas-fee-inputs/priority-fee-input/priority-fee-input.js @@ -17,7 +17,7 @@ import { useAdvancedGasFeePopoverContext } from '../../context'; import AdvancedGasFeeInputSubtext from '../../advanced-gas-fee-input-subtext'; const validatePriorityFee = (value, gasFeeEstimates) => { - if (value < 1) { + if (value <= 0) { return 'editGasMaxPriorityFeeBelowMinimumV2'; } if ( @@ -43,8 +43,7 @@ const PriorityFeeInput = () => { const t = useI18nContext(); const advancedGasFeeValues = useSelector(getAdvancedGasFeeValues); const { - setDirty, - setHasError, + setErrorValue, setMaxPriorityFeePerGas, } = useAdvancedGasFeePopoverContext(); const { @@ -72,18 +71,20 @@ const PriorityFeeInput = () => { const updatePriorityFee = (value) => { setPriorityFee(value); - setDirty(true); }; useEffect(() => { setMaxPriorityFeePerGas(priorityFee); const error = validatePriorityFee(priorityFee, gasFeeEstimates); + setErrorValue( + 'maxPriorityFeePerGas', + error === 'editGasMaxPriorityFeeBelowMinimumV2', + ); setPriorityFeeError(error); - setHasError(Boolean(error)); }, [ gasFeeEstimates, priorityFee, - setHasError, + setErrorValue, setMaxPriorityFeePerGas, setPriorityFeeError, ]); diff --git a/ui/components/app/advanced-gas-fee-popover/advanced-gas-fee-inputs/priority-fee-input/priority-fee-input.test.js b/ui/components/app/advanced-gas-fee-popover/advanced-gas-fee-inputs/priority-fee-input/priority-fee-input.test.js index 9c7fbf5e4..7657b7ebc 100644 --- a/ui/components/app/advanced-gas-fee-popover/advanced-gas-fee-inputs/priority-fee-input/priority-fee-input.test.js +++ b/ui/components/app/advanced-gas-fee-popover/advanced-gas-fee-inputs/priority-fee-input/priority-fee-input.test.js @@ -1,4 +1,5 @@ import React from 'react'; +import { fireEvent, screen } from '@testing-library/react'; import { GAS_ESTIMATE_TYPES } from '../../../../../../shared/constants/gas'; import { renderWithProvider } from '../../../../../../test/lib/render-helpers'; @@ -67,4 +68,21 @@ describe('PriorityfeeInput', () => { }); expect(document.getElementsByTagName('input')[0]).toHaveValue(2); }); + + it('should show error if value entered is 0', () => { + render({ + txParams: { + maxPriorityFeePerGas: '0x174876E800', + }, + }); + expect( + screen.queryByText('Priority fee must be greater than 0.'), + ).not.toBeInTheDocument(); + fireEvent.change(document.getElementsByTagName('input')[0], { + target: { value: 0 }, + }); + expect( + screen.queryByText('Priority fee must be greater than 0.'), + ).toBeInTheDocument(); + }); }); diff --git a/ui/components/app/advanced-gas-fee-popover/advanced-gas-fee-popover.test.js b/ui/components/app/advanced-gas-fee-popover/advanced-gas-fee-popover.test.js index b0b78e9f1..1767143b9 100644 --- a/ui/components/app/advanced-gas-fee-popover/advanced-gas-fee-popover.test.js +++ b/ui/components/app/advanced-gas-fee-popover/advanced-gas-fee-popover.test.js @@ -36,7 +36,6 @@ const render = () => { balance: '0x1F4', }, }, - advancedGasFee: { priorityFee: 100 }, featureFlags: { advancedInlineGas: true }, gasFeeEstimates: mockEstimates[GAS_ESTIMATE_TYPES.FEE_MARKET].gasFeeEstimates, @@ -46,7 +45,7 @@ const render = () => { return renderWithProvider( @@ -56,16 +55,24 @@ const render = () => { }; describe('AdvancedGasFeePopover', () => { - it('should renders save button disabled by default', () => { + it('should renders save button enabled by default', () => { render(); + expect(screen.queryByRole('button', { name: 'Save' })).not.toBeDisabled(); + }); + + it('should disable save button if priority fee 0 is entered', () => { + render(); + fireEvent.change(document.getElementsByTagName('input')[1], { + target: { value: 0 }, + }); expect(screen.queryByRole('button', { name: 'Save' })).toBeDisabled(); }); - it('should enable save button as input value is changed', () => { + it('should disable save button if priority fee entered is greater than base fee', () => { render(); - fireEvent.change(document.getElementsByTagName('input')[0], { - target: { value: 3 }, + fireEvent.change(document.getElementsByTagName('input')[1], { + target: { value: 100000 }, }); - expect(screen.queryByRole('button', { name: 'Save' })).not.toBeDisabled(); + expect(screen.queryByRole('button', { name: 'Save' })).toBeDisabled(); }); }); diff --git a/ui/components/app/advanced-gas-fee-popover/advanced-gas-fee-save/advanced-gas-fee-save.js b/ui/components/app/advanced-gas-fee-popover/advanced-gas-fee-save/advanced-gas-fee-save.js index c40f4cad1..23dd4e5c6 100644 --- a/ui/components/app/advanced-gas-fee-popover/advanced-gas-fee-save/advanced-gas-fee-save.js +++ b/ui/components/app/advanced-gas-fee-popover/advanced-gas-fee-save/advanced-gas-fee-save.js @@ -13,9 +13,8 @@ const AdvancedGasFeeSaveButton = () => { const { closeModal } = useTransactionModalContext(); const { updateTransaction } = useGasFeeContext(); const { - isDirty, gasLimit, - hasError, + hasErrors, maxFeePerGas, maxPriorityFeePerGas, } = useAdvancedGasFeePopoverContext(); @@ -31,7 +30,7 @@ const AdvancedGasFeeSaveButton = () => { }; return ( - ); diff --git a/ui/components/app/advanced-gas-fee-popover/context/advancedGasFeePopover.js b/ui/components/app/advanced-gas-fee-popover/context/advancedGasFeePopover.js index 853cb8bdc..5297f3902 100644 --- a/ui/components/app/advanced-gas-fee-popover/context/advancedGasFeePopover.js +++ b/ui/components/app/advanced-gas-fee-popover/context/advancedGasFeePopover.js @@ -1,4 +1,4 @@ -import React, { createContext, useContext, useState } from 'react'; +import React, { createContext, useCallback, useContext, useState } from 'react'; import PropTypes from 'prop-types'; export const AdvancedGasFeePopoverContext = createContext({}); @@ -7,20 +7,29 @@ export const AdvancedGasFeePopoverContextProvider = ({ children }) => { const [gasLimit, setGasLimit] = useState(); const [maxFeePerGas, setMaxFeePerGas] = useState(); const [maxPriorityFeePerGas, setMaxPriorityFeePerGas] = useState(); - const [isDirty, setDirty] = useState(); - const [hasError, setHasError] = useState(false); + const [errors, setErrors] = useState({ + maxFeePerGas: false, + maxPriorityFeePerGas: false, + }); + + const setErrorValue = useCallback( + (field, value) => { + if (errors[field] !== value) { + setErrors({ ...errors, [field]: value }); + } + }, + [errors, setErrors], + ); return (