From 0bec1df3baad422867eee93ed18f438e15d0e421 Mon Sep 17 00:00:00 2001 From: Brian Bergeron Date: Fri, 13 Oct 2023 11:07:07 -0500 Subject: [PATCH] Fix gas calculation checking wrong account balance (#21174) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Dapps can connect to / prompt transactions from a different account than the one currently selected within the wallet itself. When this occurs, `useGasFeeErrors` was doing a balance check against the wrong account. It was using the current account within the wallet, instead of the account issuing the transaction. This can cause balance errors even when the account has sufficient funds. This shows in 2 places in the UI, which are the only places checking `balanceError`: - Token approvals - Customizing gas popup Solution: Check balance of the account issuing the transaction. ## **Manual testing steps** 1. Within the wallet, select an empty account. 2. On the E2E Test Dapp, connect to a funded account. 3. Send ETH 4. Click "🌐 Site suggested" to customize gas 5. A balance error used to appear, but no longer should. ## **Screenshots/Recordings** ### **Before** https://github.com/MetaMask/metamask-extension/assets/3500406/f48eff21-f2ff-4d96-ba4a-ab15427092ec ## **Related issues** Fixes: https://github.com/MetaMask/metamask-extension/issues/20770 ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've clearly explained: - [x] What problem this PR is solving. - [x] How this problem was solved. - [x] How reviewers can test my changes. - [x] I’ve indicated what issue this PR is linked to: Fixes #??? - [x] I’ve included tests if applicable. - [ ] I’ve documented any added code. - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). - [x] I’ve properly set the pull request status: - [x] In case it's not yet "ready for review", I've set it to "draft". - [x] In case it's "ready for review", I've changed it from "draft" to "non-draft". ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- .../edit-gas-fee-popover.test.js | 21 +++++++++++++-- ui/hooks/gasFeeInput/test-utils.js | 3 +-- ui/hooks/gasFeeInput/useGasFeeErrors.js | 20 +++++++++----- ui/hooks/gasFeeInput/useGasFeeErrors.test.js | 25 ++++++++++++++++-- ui/hooks/gasFeeInput/useGasFeeInputs.test.js | 26 ++++++++++++++++--- 5 files changed, 80 insertions(+), 15 deletions(-) diff --git a/ui/components/app/edit-gas-fee-popover/edit-gas-fee-popover.test.js b/ui/components/app/edit-gas-fee-popover/edit-gas-fee-popover.test.js index eb8287a2da99..94f0df605d36 100644 --- a/ui/components/app/edit-gas-fee-popover/edit-gas-fee-popover.test.js +++ b/ui/components/app/edit-gas-fee-popover/edit-gas-fee-popover.test.js @@ -6,6 +6,11 @@ import { renderWithProvider } from '../../../../test/lib/render-helpers'; import { ETH } from '../../../helpers/constants/common'; import configureStore from '../../../store/store'; import { GasFeeContextProvider } from '../../../contexts/gasFee'; + +import { + TransactionStatus, + TransactionType, +} from '../../../../shared/constants/transaction'; import { NETWORK_TYPES, CHAIN_IDS, @@ -120,13 +125,25 @@ describe('EditGasFeePopover', () => { }); it('should not show insufficient balance message if transaction value is less than balance', () => { - render({ txProps: { userFeeLevel: 'high', txParams: { value: '0x64' } } }); + render({ + txProps: { + status: TransactionStatus.unapproved, + type: TransactionType.simpleSend, + userFeeLevel: 'high', + txParams: { value: '0x64', from: '0xAddress' }, + }, + }); expect(screen.queryByText('Insufficient funds.')).not.toBeInTheDocument(); }); it('should show insufficient balance message if transaction value is more than balance', () => { render({ - txProps: { userFeeLevel: 'high', txParams: { value: '0x5208' } }, + txProps: { + status: TransactionStatus.unapproved, + type: TransactionType.simpleSend, + userFeeLevel: 'high', + txParams: { value: '0x5208', from: '0xAddress' }, + }, }); expect(screen.queryByText('Insufficient funds.')).toBeInTheDocument(); }); diff --git a/ui/hooks/gasFeeInput/test-utils.js b/ui/hooks/gasFeeInput/test-utils.js index 86e00a822447..8d5225a12ccd 100644 --- a/ui/hooks/gasFeeInput/test-utils.js +++ b/ui/hooks/gasFeeInput/test-utils.js @@ -8,7 +8,6 @@ import { import { checkNetworkAndAccountSupports1559, getCurrentCurrency, - getSelectedAccount, getShouldShowFiat, getPreferences, txDataSelector, @@ -122,7 +121,7 @@ export const generateUseSelectorRouter = }, }; } - if (selector === getSelectedAccount) { + if (selector.toString().includes('getTargetAccount')) { return { balance: '0x440aa47cc2556', }; diff --git a/ui/hooks/gasFeeInput/useGasFeeErrors.js b/ui/hooks/gasFeeInput/useGasFeeErrors.js index b48f5cef0fcf..3fdedd035b2f 100644 --- a/ui/hooks/gasFeeInput/useGasFeeErrors.js +++ b/ui/hooks/gasFeeInput/useGasFeeErrors.js @@ -3,12 +3,14 @@ import { shallowEqual, useSelector } from 'react-redux'; import { GasEstimateTypes, GAS_LIMITS } from '../../../shared/constants/gas'; import { checkNetworkAndAccountSupports1559, - getSelectedAccount, + getTargetAccount, } from '../../selectors'; import { isLegacyTransaction } from '../../helpers/utils/transactions.util'; import { bnGreaterThan, bnLessThan } from '../../helpers/utils/util'; import { GAS_FORM_ERRORS } from '../../helpers/constants/gas'; import { Numeric } from '../../../shared/modules/Numeric'; +import { PENDING_STATUS_HASH } from '../../helpers/constants/transactions'; +import { TransactionType } from '../../../shared/constants/transaction'; const HIGH_FEE_WARNING_MULTIPLIER = 1.5; @@ -267,13 +269,19 @@ export function useGasFeeErrors({ [gasErrors, gasWarnings], ); - const { balance: ethBalance } = useSelector(getSelectedAccount, shallowEqual); - const balanceError = hasBalanceError( - minimumCostInHexWei, - transaction, - ethBalance, + const account = useSelector( + (state) => getTargetAccount(state, transaction?.txParams?.from), + shallowEqual, ); + // Balance check is only relevant for outgoing + pending transactions + const balanceError = + account !== undefined && + transaction?.type !== TransactionType.incoming && + transaction?.status in PENDING_STATUS_HASH + ? hasBalanceError(minimumCostInHexWei, transaction, account.balance) + : false; + return { gasErrors: errorsAndWarnings, hasGasErrors, diff --git a/ui/hooks/gasFeeInput/useGasFeeErrors.test.js b/ui/hooks/gasFeeInput/useGasFeeErrors.test.js index 48cedcaada06..feb407b818a1 100644 --- a/ui/hooks/gasFeeInput/useGasFeeErrors.test.js +++ b/ui/hooks/gasFeeInput/useGasFeeErrors.test.js @@ -2,6 +2,11 @@ import { renderHook } from '@testing-library/react-hooks'; import { GAS_FORM_ERRORS } from '../../helpers/constants/gas'; +import { + TransactionStatus, + TransactionType, +} from '../../../shared/constants/transaction'; + import { useGasFeeErrors } from './useGasFeeErrors'; import { @@ -24,10 +29,20 @@ jest.mock('react-redux', () => { }; }); +const mockTransaction = { + status: TransactionStatus.unapproved, + type: TransactionType.simpleSend, + txParams: { + from: '0x000000000000000000000000000000000000dead', + type: '0x2', + value: '100', + }, +}; + const renderUseGasFeeErrorsHook = (props) => { return renderHook(() => useGasFeeErrors({ - transaction: { txParams: { type: '0x2', value: '100' } }, + transaction: mockTransaction, gasLimit: '21000', gasPrice: '10', maxPriorityFeePerGas: '10', @@ -273,7 +288,13 @@ describe('useGasFeeErrors', () => { it('is true if balance is less than transaction value', () => { configureLegacy(); const { result } = renderUseGasFeeErrorsHook({ - transaction: { txParams: { type: '0x2', value: '0x440aa47cc2556' } }, + transaction: { + ...mockTransaction, + txParams: { + ...mockTransaction.txParams, + value: '0x440aa47cc2556', + }, + }, ...LEGACY_GAS_ESTIMATE_RETURN_VALUE, }); expect(result.current.balanceError).toBe(true); diff --git a/ui/hooks/gasFeeInput/useGasFeeInputs.test.js b/ui/hooks/gasFeeInput/useGasFeeInputs.test.js index 35205583ee8d..a6bbfd660d9a 100644 --- a/ui/hooks/gasFeeInput/useGasFeeInputs.test.js +++ b/ui/hooks/gasFeeInput/useGasFeeInputs.test.js @@ -1,6 +1,10 @@ import { act, renderHook } from '@testing-library/react-hooks'; import { useSelector } from 'react-redux'; -import { TransactionEnvelopeType } from '../../../shared/constants/transaction'; +import { + TransactionEnvelopeType, + TransactionStatus, + TransactionType, +} from '../../../shared/constants/transaction'; import { GasRecommendations, EditGasModes, @@ -39,6 +43,16 @@ jest.mock('react-redux', () => { }; }); +const mockTransaction = { + status: TransactionStatus.unapproved, + type: TransactionType.simpleSend, + txParams: { + from: '0x000000000000000000000000000000000000dead', + type: '0x2', + value: '100', + }, +}; + describe('useGasFeeInputs', () => { beforeEach(() => { jest.clearAllMocks(); @@ -141,7 +155,9 @@ describe('useGasFeeInputs', () => { }); it('should return false', () => { - const { result } = renderHook(() => useGasFeeInputs()); + const { result } = renderHook(() => + useGasFeeInputs(undefined, mockTransaction), + ); expect(result.current.balanceError).toBe(false); }); }); @@ -157,8 +173,12 @@ describe('useGasFeeInputs', () => { it('should return true', () => { const { result } = renderHook(() => useGasFeeInputs(null, { + ...mockTransaction, userFeeLevel: GasRecommendations.medium, - txParams: { gas: '0x5208' }, + txParams: { + ...mockTransaction.txParams, + gas: '0x5208', + }, }), ); expect(result.current.balanceError).toBe(true);