Skip to content

Commit

Permalink
fix(swaps): fix to round gas values to integer (#25488)
Browse files Browse the repository at this point in the history
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

This PR fixes rounding for max gas calculations, preventing the
following issue while trying to perform a swap


![image](https://github.com/MetaMask/metamask-extension/assets/8658960/077cd50d-7832-498d-b64b-7845171a4ea3)

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25488?quickstart=1)

## **Related issues**

Fixes:

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [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)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [x] 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.

---------

Co-authored-by: IF <139582705+infiniteflower@users.noreply.github.com>
  • Loading branch information
nikoferro and infiniteflower authored Jun 24, 2024
1 parent e1cfecf commit 096d73f
Show file tree
Hide file tree
Showing 5 changed files with 114 additions and 46 deletions.
36 changes: 36 additions & 0 deletions shared/lib/swaps-utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import { addHexPrefix } from '../../app/scripts/lib/util';
import { decimalToHex } from '../modules/conversion.utils';
import fetchWithCache from './fetch-with-cache';

const FALLBACK_GAS_MULTIPLIER = 1.5;

const TEST_CHAIN_IDS = [CHAIN_IDS.GOERLI, CHAIN_IDS.LOCALHOST];

const clientIdHeader = { 'X-Client-Id': SWAPS_CLIENT_ID };
Expand Down Expand Up @@ -325,3 +327,37 @@ export async function fetchTradesInfo(

return newQuotes;
}

/**
* Given a gas estimate, gas multiplier, max gas, and custom max gas, returns the max gas limit
* to use for a transaction.
*
* @param {string} gasEstimate - The gas estimate for the transaction.
* @param {number} gasMultiplier - The gas multiplier to use.
* @param {number} maxGas - The max gas limit to use.
* @param {string} customMaxGas - The custom max gas limit to use.
* @returns {string} The max gas limit to use for the transaction.
*/

export function calculateMaxGasLimit(
gasEstimate,
gasMultiplier = FALLBACK_GAS_MULTIPLIER,
maxGas,
customMaxGas,
) {
const gasLimitForMax = new BigNumber(gasEstimate || 0, 16)
.round(0)
.toString(16);

const usedGasLimitWithMultiplier = new BigNumber(gasLimitForMax, 16)
.times(gasMultiplier, 10)
.round(0)
.toString(16);

const nonCustomMaxGasLimit = gasEstimate
? usedGasLimitWithMultiplier
: `0x${decimalToHex(maxGas || 0)}`;
const maxGasLimit = customMaxGas || nonCustomMaxGasLimit;

return maxGasLimit;
}
48 changes: 47 additions & 1 deletion shared/lib/swaps-utils.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,11 @@ import {
TOKENS,
MOCK_TRADE_RESPONSE_2,
} from '../../ui/pages/swaps/swaps-util-test-constants';
import { fetchTradesInfo, shouldEnableDirectWrapping } from './swaps-utils';
import {
fetchTradesInfo,
shouldEnableDirectWrapping,
calculateMaxGasLimit,
} from './swaps-utils';

jest.mock('./storage-helpers', () => ({
getStorageItem: jest.fn(),
Expand Down Expand Up @@ -224,4 +228,46 @@ describe('Swaps Utils', () => {
expect(shouldEnableDirectWrapping(CHAIN_IDS.MAINNET)).toBe(false);
});
});

describe('calculateMaxGasLimit', () => {
const gasEstimate = '0x37b15';
const maxGas = 273740;
let expectedMaxGas = '42d4c';
let gasMultiplier = 1.2;
let customMaxGas = '';

it('should return the max gas limit', () => {
const result = calculateMaxGasLimit(
gasEstimate,
gasMultiplier,
maxGas,
customMaxGas,
);
expect(result).toStrictEqual(expectedMaxGas);
});

it('should return the custom max gas limit', () => {
customMaxGas = '46d4c';
const result = calculateMaxGasLimit(
gasEstimate,
gasMultiplier,
maxGas,
customMaxGas,
);
expect(result).toStrictEqual(customMaxGas);
});

it('should return the max gas limit with a gas multiplier of 4.5', () => {
gasMultiplier = 4.5;
expectedMaxGas = 'fa9df';
customMaxGas = '';
const result = calculateMaxGasLimit(
gasEstimate,
gasMultiplier,
maxGas,
customMaxGas,
);
expect(result).toStrictEqual(expectedMaxGas);
});
});
});
27 changes: 11 additions & 16 deletions ui/ducks/swaps/swaps.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ import {
} from '../../../shared/lib/transactions-controller-utils';
import { EtherDenomination } from '../../../shared/constants/common';
import { Numeric } from '../../../shared/modules/Numeric';
import { calculateMaxGasLimit } from '../../../shared/lib/swaps-utils';

export const GAS_PRICES_LOADING_STATES = {
INITIAL: 'INITIAL',
Expand All @@ -102,8 +103,6 @@ export const GAS_PRICES_LOADING_STATES = {
COMPLETED: 'COMPLETED',
};

export const FALLBACK_GAS_MULTIPLIER = 1.5;

const initialState = {
aggregatorMetadata: null,
approveTxId: null,
Expand Down Expand Up @@ -1106,20 +1105,16 @@ export const signAndSendTransactions = (
const usedQuote = getUsedQuote(state);
const usedTradeTxParams = usedQuote.trade;

const estimatedGasLimit = new BigNumber(
usedQuote?.gasEstimate || 0,
16,
).toString(16);

const maxGasLimit =
customSwapsGas ||
(usedQuote?.gasEstimate
? `0x${estimatedGasLimit}`
: `0x${decimalToHex(
new BigNumber(usedQuote?.maxGas)
.mul(usedQuote?.gasMultiplier || FALLBACK_GAS_MULTIPLIER)
.toString() || 0,
)}`);
const estimatedGasLimit = new BigNumber(usedQuote?.gasEstimate || 0, 16)
.round(0)
.toString(16);

const maxGasLimit = calculateMaxGasLimit(
usedQuote?.gasEstimate,
usedQuote?.gasMultiplier,
usedQuote?.maxGas,
customSwapsGas,
);

const usedGasPrice = getUsedSwapsGasPrice(state);
usedTradeTxParams.gas = maxGasLimit;
Expand Down
26 changes: 10 additions & 16 deletions ui/pages/swaps/prepare-swap-page/review-quote.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import { usePrevious } from '../../../hooks/usePrevious';
import { useGasFeeInputs } from '../../confirmations/hooks/useGasFeeInputs';
import { MetaMetricsContext } from '../../../contexts/metametrics';
import {
FALLBACK_GAS_MULTIPLIER,
getQuotes,
getSelectedQuote,
getApproveTxParams,
Expand Down Expand Up @@ -135,7 +134,10 @@ import {
toPrecisionWithoutTrailingZeros,
} from '../../../../shared/lib/transactions-controller-utils';
import { addHexPrefix } from '../../../../app/scripts/lib/util';
import { calcTokenValue } from '../../../../shared/lib/swaps-utils';
import {
calcTokenValue,
calculateMaxGasLimit,
} from '../../../../shared/lib/swaps-utils';
import { GAS_FEES_LEARN_MORE_URL } from '../../../../shared/lib/ui-utils';
import ExchangeRateDisplay from '../exchange-rate-display';
import InfoTooltip from '../../../components/ui/info-tooltip';
Expand Down Expand Up @@ -295,20 +297,12 @@ export default function ReviewQuote({ setReceiveToAmount }) {
usedQuote?.gasEstimateWithRefund ||
`0x${decimalToHex(usedQuote?.averageGas || 0)}`;

const estimatedGasLimit = new BigNumber(
usedQuote?.gasEstimate || 0,
16,
).toString(16);

const nonCustomMaxGasLimit = usedQuote?.gasEstimate
? `0x${estimatedGasLimit}`
: `0x${decimalToHex(
new BigNumber(usedQuote?.maxGas)
.mul(usedQuote?.gasMultiplier || FALLBACK_GAS_MULTIPLIER)
.toString() || 0,
)}`;

const maxGasLimit = customMaxGas || nonCustomMaxGasLimit;
const maxGasLimit = calculateMaxGasLimit(
usedQuote?.gasEstimate,
usedQuote?.gasMultiplier,
usedQuote?.maxGas,
customMaxGas,
);

let maxFeePerGas;
let maxPriorityFeePerGas;
Expand Down
23 changes: 10 additions & 13 deletions ui/pages/swaps/view-quote/view-quote.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import { useGasFeeInputs } from '../../confirmations/hooks/useGasFeeInputs';
import { MetaMetricsContext } from '../../../contexts/metametrics';
import FeeCard from '../fee-card';
import {
FALLBACK_GAS_MULTIPLIER,
getQuotes,
getSelectedQuote,
getApproveTxParams,
Expand Down Expand Up @@ -109,7 +108,10 @@ import {
toPrecisionWithoutTrailingZeros,
} from '../../../../shared/lib/transactions-controller-utils';
import { addHexPrefix } from '../../../../app/scripts/lib/util';
import { calcTokenValue } from '../../../../shared/lib/swaps-utils';
import {
calcTokenValue,
calculateMaxGasLimit,
} from '../../../../shared/lib/swaps-utils';
import {
addHexes,
decGWEIToHexWEI,
Expand Down Expand Up @@ -230,17 +232,12 @@ export default function ViewQuote() {
usedQuote?.gasEstimateWithRefund ||
`0x${decimalToHex(usedQuote?.averageGas || 0)}`;

const gasLimitForMax = usedQuote?.gasEstimate || `0x0`;

const usedGasLimitWithMultiplier = new BigNumber(gasLimitForMax, 16)
.times(usedQuote?.gasMultiplier || FALLBACK_GAS_MULTIPLIER, 10)
.round(0)
.toString(16);

const nonCustomMaxGasLimit = usedQuote?.gasEstimate
? usedGasLimitWithMultiplier
: `0x${decimalToHex(usedQuote?.maxGas || 0)}`;
const maxGasLimit = customMaxGas || nonCustomMaxGasLimit;
const maxGasLimit = calculateMaxGasLimit(
usedQuote?.gasEstimate,
usedQuote?.gasMultiplier,
usedQuote?.maxGas,
customMaxGas,
);

let maxFeePerGas;
let maxPriorityFeePerGas;
Expand Down

0 comments on commit 096d73f

Please sign in to comment.