Skip to content

Commit

Permalink
Fix gas calculation checking wrong account balance (#21174)
Browse files Browse the repository at this point in the history
## **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: #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.
  • Loading branch information
bergeron authored Oct 13, 2023
1 parent 9d1bd43 commit 0bec1df
Show file tree
Hide file tree
Showing 5 changed files with 80 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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();
});
Expand Down
3 changes: 1 addition & 2 deletions ui/hooks/gasFeeInput/test-utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import {
import {
checkNetworkAndAccountSupports1559,
getCurrentCurrency,
getSelectedAccount,
getShouldShowFiat,
getPreferences,
txDataSelector,
Expand Down Expand Up @@ -122,7 +121,7 @@ export const generateUseSelectorRouter =
},
};
}
if (selector === getSelectedAccount) {
if (selector.toString().includes('getTargetAccount')) {
return {
balance: '0x440aa47cc2556',
};
Expand Down
20 changes: 14 additions & 6 deletions ui/hooks/gasFeeInput/useGasFeeErrors.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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,
Expand Down
25 changes: 23 additions & 2 deletions ui/hooks/gasFeeInput/useGasFeeErrors.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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',
Expand Down Expand Up @@ -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);
Expand Down
26 changes: 23 additions & 3 deletions ui/hooks/gasFeeInput/useGasFeeInputs.test.js
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
});
});
Expand All @@ -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);
Expand Down

0 comments on commit 0bec1df

Please sign in to comment.