Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix gas calculation checking wrong account balance #21174

Merged
merged 3 commits into from
Oct 13, 2023
Merged

Fix gas calculation checking wrong account balance #21174

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0a50779
Check gas against correct account balance
bergeron Oct 3, 2023
25a9129
fix edit gas popover test
bergeron Oct 3, 2023
506f20a
Merge branch 'develop' of github.com:MetaMask/metamask-extension into…
bergeron Oct 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions ui/components/app/edit-gas-fee-popover/edit-gas-fee-popover.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ import { ETH } from '../../../helpers/constants/common';
import configureStore from '../../../store/store';
import { GasFeeContextProvider } from '../../../contexts/gasFee';

import {
TransactionStatus,
TransactionType,
} from '../../../../shared/constants/transaction';
import EditGasFeePopover from './edit-gas-fee-popover';

jest.mock('../../../store/actions', () => ({
Expand Down Expand Up @@ -112,13 +116,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
View file
Open in desktop
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')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hack because the selector is actually a lambda, to allow getTargetAccount to accept a second parameter. Therefore it can't use function equality like the others.

A better approach to this kind of mocking, would keep the selectors from the product code and mock the state instead. Better to mock the lowest level thing, so that the product selectors are tested too if they change.

return {
balance: '0x440aa47cc2556',
};
Expand Down
20 changes: 14 additions & 6 deletions ui/hooks/gasFeeInput/useGasFeeErrors.js
View file
Open in desktop
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
Comment on lines +277 to +281
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that this balance check was being called in cases it should not. Like for incoming transactions, and transactions that have already confirmed. Because we won't have the from account in our state for incoming transactions, this guard is needed.

A better fix may be to further split these gas operations, so that callers can request only the necessary gas information for their contexts.

? hasBalanceError(minimumCostInHexWei, transaction, account.balance)
: false;

return {
gasErrors: errorsAndWarnings,
hasGasErrors,
Expand Down
25 changes: 23 additions & 2 deletions ui/hooks/gasFeeInput/useGasFeeErrors.test.js
View file
Open in desktop
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
View file
Open in desktop
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
Loading