From d1161369e4cfdb31d83000560734cfc741b56f8b Mon Sep 17 00:00:00 2001 From: Matthew Walsh Date: Thu, 7 Nov 2024 09:08:18 +0000 Subject: [PATCH] fix: gas limit estimation (#4897) Remove the calculated fallback `gas` property from the `eth_estimateGas` request. Remove all gas fee properties from the `eth_estimateGas` request. Calculate fallback gas, max gas limit, and buffer using fractions rather than decimals. --- .../src/utils/gas.test.ts | 182 +++++++++++++----- .../transaction-controller/src/utils/gas.ts | 44 +++-- 2 files changed, 167 insertions(+), 59 deletions(-) diff --git a/packages/transaction-controller/src/utils/gas.test.ts b/packages/transaction-controller/src/utils/gas.test.ts index 97c34c0e3c..3060f841a0 100644 --- a/packages/transaction-controller/src/utils/gas.test.ts +++ b/packages/transaction-controller/src/utils/gas.test.ts @@ -1,5 +1,3 @@ -/* eslint-disable jsdoc/require-jsdoc */ - import { query } from '@metamask/controller-utils'; import type EthQuery from '@metamask/eth-query'; @@ -12,7 +10,8 @@ import { updateGas, FIXED_GAS, DEFAULT_GAS_MULTIPLIER, - GAS_ESTIMATE_FALLBACK_MULTIPLIER, + GAS_ESTIMATE_FALLBACK_BLOCK_PERCENT, + MAX_GAS_BLOCK_PERCENT, } from './gas'; jest.mock('@metamask/controller-utils', () => ({ @@ -21,20 +20,18 @@ jest.mock('@metamask/controller-utils', () => ({ })); const GAS_MOCK = 100; -const BLOCK_GAS_LIMIT_MOCK = 1234567; +const BLOCK_GAS_LIMIT_MOCK = 123456789; const BLOCK_NUMBER_MOCK = '0x5678'; -// TODO: Replace `any` with type -// eslint-disable-next-line @typescript-eslint/no-explicit-any -const ETH_QUERY_MOCK = {} as any as EthQuery; +const ETH_QUERY_MOCK = {} as unknown as EthQuery; +const FALLBACK_MULTIPLIER = GAS_ESTIMATE_FALLBACK_BLOCK_PERCENT / 100; +const MAX_GAS_MULTIPLIER = MAX_GAS_BLOCK_PERCENT / 100; const TRANSACTION_META_MOCK = { txParams: { data: '0x1', to: '0x2', }, - // TODO: Replace `any` with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any -} as any as TransactionMeta; +} as unknown as TransactionMeta; const UPDATE_GAS_REQUEST_MOCK = { txMeta: TRANSACTION_META_MOCK, @@ -43,6 +40,11 @@ const UPDATE_GAS_REQUEST_MOCK = { ethQuery: ETH_QUERY_MOCK, } as UpdateGasRequest; +/** + * Converts number to hex string. + * @param value - The number to convert. + * @returns The hex string. + */ function toHex(value: number) { return `0x${value.toString(16)}`; } @@ -51,22 +53,26 @@ describe('gas', () => { const queryMock = jest.mocked(query); let updateGasRequest: UpdateGasRequest; + /** + * Mocks query responses. + * @param options - The options. + * @param options.getCodeResponse - The response for getCode. + * @param options.getBlockByNumberResponse - The response for getBlockByNumber. + * @param options.estimateGasResponse - The response for estimateGas. + * @param options.estimateGasError - The error for estimateGas. + */ function mockQuery({ getCodeResponse, getBlockByNumberResponse, estimateGasResponse, estimateGasError, }: { - // TODO: Replace `any` with type // eslint-disable-next-line @typescript-eslint/no-explicit-any getCodeResponse?: any; - // TODO: Replace `any` with type // eslint-disable-next-line @typescript-eslint/no-explicit-any getBlockByNumberResponse?: any; - // TODO: Replace `any` with type // eslint-disable-next-line @typescript-eslint/no-explicit-any estimateGasResponse?: any; - // TODO: Replace `any` with type // eslint-disable-next-line @typescript-eslint/no-explicit-any estimateGasError?: any; }) { @@ -85,6 +91,9 @@ describe('gas', () => { } } + /** + * Assert that estimateGas was not called. + */ function expectEstimateGasNotCalled() { expect(queryMock).not.toHaveBeenCalledWith( expect.anything(), @@ -95,6 +104,7 @@ describe('gas', () => { beforeEach(() => { updateGasRequest = JSON.parse(JSON.stringify(UPDATE_GAS_REQUEST_MOCK)); + jest.resetAllMocks(); }); describe('updateGas', () => { @@ -149,8 +159,10 @@ describe('gas', () => { ); }); - it('to estimate if estimate greater than 90% of block gas limit', async () => { - const estimatedGas = Math.ceil(BLOCK_GAS_LIMIT_MOCK * 0.9 + 10); + it('to estimate if estimate greater than percentage of block gas limit', async () => { + const estimatedGas = Math.ceil( + BLOCK_GAS_LIMIT_MOCK * MAX_GAS_MULTIPLIER + 10, + ); mockQuery({ getBlockByNumberResponse: { gasLimit: toHex(BLOCK_GAS_LIMIT_MOCK) }, @@ -165,10 +177,12 @@ describe('gas', () => { ); }); - it('to padded estimate if padded estimate less than 90% of block gas limit', async () => { - const blockGasLimit90Percent = BLOCK_GAS_LIMIT_MOCK * 0.9; - const estimatedGasPadded = Math.ceil(blockGasLimit90Percent - 10); - const estimatedGas = Math.round(estimatedGasPadded / 1.5); + it('to padded estimate if padded estimate less than percentage of block gas limit', async () => { + const maxGasLimit = BLOCK_GAS_LIMIT_MOCK * MAX_GAS_MULTIPLIER; + const estimatedGasPadded = Math.floor(maxGasLimit) - 10; + const estimatedGas = Math.ceil( + estimatedGasPadded / DEFAULT_GAS_MULTIPLIER, + ); mockQuery({ getBlockByNumberResponse: { gasLimit: toHex(BLOCK_GAS_LIMIT_MOCK) }, @@ -185,9 +199,9 @@ describe('gas', () => { ); }); - it('to padded estimate using chain multiplier if padded estimate less than 90% of block gas limit', async () => { - const blockGasLimit90Percent = BLOCK_GAS_LIMIT_MOCK * 0.9; - const estimatedGasPadded = Math.ceil(blockGasLimit90Percent - 10); + it('to padded estimate using chain multiplier if padded estimate less than percentage of block gas limit', async () => { + const maxGasLimit = BLOCK_GAS_LIMIT_MOCK * MAX_GAS_MULTIPLIER; + const estimatedGasPadded = Math.ceil(maxGasLimit - 10); const estimatedGas = estimatedGasPadded; // Optimism multiplier is 1 updateGasRequest.chainId = CHAIN_IDS.OPTIMISM; @@ -207,10 +221,14 @@ describe('gas', () => { ); }); - it('to 90% of block gas limit if padded estimate only is greater than 90% of block gas limit', async () => { - const blockGasLimit90Percent = Math.round(BLOCK_GAS_LIMIT_MOCK * 0.9); - const estimatedGasPadded = blockGasLimit90Percent + 10; - const estimatedGas = Math.ceil(estimatedGasPadded / 1.5); + it('to percentage of block gas limit if padded estimate only is greater than percentage of block gas limit', async () => { + const maxGasLimit = Math.round( + BLOCK_GAS_LIMIT_MOCK * MAX_GAS_MULTIPLIER, + ); + const estimatedGasPadded = maxGasLimit + 10; + const estimatedGas = Math.ceil( + estimatedGasPadded / DEFAULT_GAS_MULTIPLIER, + ); mockQuery({ getBlockByNumberResponse: { gasLimit: toHex(BLOCK_GAS_LIMIT_MOCK) }, @@ -219,9 +237,7 @@ describe('gas', () => { await updateGas(updateGasRequest); - expect(updateGasRequest.txMeta.txParams.gas).toBe( - toHex(blockGasLimit90Percent), - ); + expect(updateGasRequest.txMeta.txParams.gas).toBe(toHex(maxGasLimit)); expect(updateGasRequest.txMeta.originalGasEstimate).toBe( updateGasRequest.txMeta.txParams.gas, ); @@ -267,7 +283,7 @@ describe('gas', () => { describe('on estimate query error', () => { it('sets gas to 35% of block gas limit', async () => { const fallbackGas = Math.floor( - BLOCK_GAS_LIMIT_MOCK * GAS_ESTIMATE_FALLBACK_MULTIPLIER, + BLOCK_GAS_LIMIT_MOCK * FALLBACK_MULTIPLIER, ); mockQuery({ @@ -357,7 +373,7 @@ describe('gas', () => { it('returns estimated gas as 35% of block gas limit on error', async () => { const fallbackGas = Math.floor( - BLOCK_GAS_LIMIT_MOCK * GAS_ESTIMATE_FALLBACK_MULTIPLIER, + BLOCK_GAS_LIMIT_MOCK * FALLBACK_MULTIPLIER, ); mockQuery({ @@ -378,47 +394,121 @@ describe('gas', () => { simulationFails: expect.any(Object), }); }); + + it('removes gas fee properties from estimate request', async () => { + mockQuery({ + getBlockByNumberResponse: { gasLimit: toHex(BLOCK_GAS_LIMIT_MOCK) }, + estimateGasResponse: toHex(GAS_MOCK), + }); + + await estimateGas( + { + ...TRANSACTION_META_MOCK.txParams, + gasPrice: '0x1', + maxFeePerGas: '0x2', + maxPriorityFeePerGas: '0x3', + }, + ETH_QUERY_MOCK, + ); + + expect(queryMock).toHaveBeenCalledWith(ETH_QUERY_MOCK, 'estimateGas', [ + { + ...TRANSACTION_META_MOCK.txParams, + value: expect.anything(), + }, + ]); + }); + + it('normalizes data in estimate request', async () => { + mockQuery({ + getBlockByNumberResponse: { gasLimit: toHex(BLOCK_GAS_LIMIT_MOCK) }, + estimateGasResponse: toHex(GAS_MOCK), + }); + + await estimateGas( + { + ...TRANSACTION_META_MOCK.txParams, + data: '123', + }, + ETH_QUERY_MOCK, + ); + + expect(queryMock).toHaveBeenCalledWith(ETH_QUERY_MOCK, 'estimateGas', [ + expect.objectContaining({ + ...TRANSACTION_META_MOCK.txParams, + data: '0x123', + }), + ]); + }); + + it('normalizes value in estimate request', async () => { + mockQuery({ + getBlockByNumberResponse: { gasLimit: toHex(BLOCK_GAS_LIMIT_MOCK) }, + estimateGasResponse: toHex(GAS_MOCK), + }); + + await estimateGas( + { + ...TRANSACTION_META_MOCK.txParams, + value: undefined, + }, + ETH_QUERY_MOCK, + ); + + expect(queryMock).toHaveBeenCalledWith(ETH_QUERY_MOCK, 'estimateGas', [ + { + ...TRANSACTION_META_MOCK.txParams, + value: '0x0', + }, + ]); + }); }); describe('addGasBuffer', () => { - it('returns estimated gas if greater than 90% of block gas limit', () => { - const estimatedGas = Math.ceil(BLOCK_GAS_LIMIT_MOCK * 0.9 + 10); + it('returns estimated gas if greater than percentage of block gas limit', () => { + const estimatedGas = Math.ceil( + BLOCK_GAS_LIMIT_MOCK * MAX_GAS_MULTIPLIER + 10, + ); const result = addGasBuffer( toHex(estimatedGas), toHex(BLOCK_GAS_LIMIT_MOCK), - 1.5, + DEFAULT_GAS_MULTIPLIER, ); expect(result).toBe(toHex(estimatedGas)); }); - it('returns padded estimate if less than 90% of block gas limit', () => { - const blockGasLimit90Percent = BLOCK_GAS_LIMIT_MOCK * 0.9; - const estimatedGasPadded = Math.ceil(blockGasLimit90Percent - 10); - const estimatedGas = Math.round(estimatedGasPadded / 1.5); + it('returns padded estimate if less than percentage of block gas limit', () => { + const maxGasLimit = BLOCK_GAS_LIMIT_MOCK * MAX_GAS_MULTIPLIER; + const estimatedGasPadded = Math.floor(maxGasLimit - 10); + const estimatedGas = Math.ceil( + estimatedGasPadded / DEFAULT_GAS_MULTIPLIER, + ); const result = addGasBuffer( toHex(estimatedGas), toHex(BLOCK_GAS_LIMIT_MOCK), - 1.5, + DEFAULT_GAS_MULTIPLIER, ); expect(result).toBe(toHex(estimatedGasPadded)); }); - it('returns 90% of block gas limit if padded estimate only is greater than 90% of block gas limit', () => { - const blockGasLimit90Percent = Math.round(BLOCK_GAS_LIMIT_MOCK * 0.9); - const estimatedGasPadded = blockGasLimit90Percent + 10; - const estimatedGas = Math.ceil(estimatedGasPadded / 1.5); + it('returns percentage of block gas limit if padded estimate only is greater than percentage of block gas limit', () => { + const maxGasLimit = Math.round(BLOCK_GAS_LIMIT_MOCK * MAX_GAS_MULTIPLIER); + const estimatedGasPadded = maxGasLimit + 10; + const estimatedGas = Math.ceil( + estimatedGasPadded / DEFAULT_GAS_MULTIPLIER, + ); const result = addGasBuffer( toHex(estimatedGas), toHex(BLOCK_GAS_LIMIT_MOCK), - 1.5, + DEFAULT_GAS_MULTIPLIER, ); - expect(result).toBe(toHex(blockGasLimit90Percent)); + expect(result).toBe(toHex(maxGasLimit)); }); }); }); diff --git a/packages/transaction-controller/src/utils/gas.ts b/packages/transaction-controller/src/utils/gas.ts index 91f83778d5..2ffc79df25 100644 --- a/packages/transaction-controller/src/utils/gas.ts +++ b/packages/transaction-controller/src/utils/gas.ts @@ -1,6 +1,11 @@ /* eslint-disable jsdoc/require-jsdoc */ -import { BNToHex, hexToBN, query } from '@metamask/controller-utils'; +import { + BNToHex, + fractionBN, + hexToBN, + query, +} from '@metamask/controller-utils'; import type EthQuery from '@metamask/eth-query'; import type { Hex } from '@metamask/utils'; import { add0x, createModuleLogger } from '@metamask/utils'; @@ -20,7 +25,8 @@ export const log = createModuleLogger(projectLogger, 'gas'); export const FIXED_GAS = '0x5208'; export const DEFAULT_GAS_MULTIPLIER = 1.5; -export const GAS_ESTIMATE_FALLBACK_MULTIPLIER = 0.35; +export const GAS_ESTIMATE_FALLBACK_BLOCK_PERCENT = 35; +export const MAX_GAS_BLOCK_PERCENT = 90; export async function updateGas(request: UpdateGasRequest) { const { txMeta } = request; @@ -49,22 +55,28 @@ export async function estimateGas( const request = { ...txParams }; const { data, value } = request; - const { gasLimit: gasLimitHex, number: blockNumber } = await getLatestBlock( + const { gasLimit: blockGasLimit, number: blockNumber } = await getLatestBlock( ethQuery, ); - const gasLimitBN = hexToBN(gasLimitHex); + const blockGasLimitBN = hexToBN(blockGasLimit); + + const fallback = BNToHex( + fractionBN(blockGasLimitBN, GAS_ESTIMATE_FALLBACK_BLOCK_PERCENT, 100), + ); request.data = data ? add0x(data) : data; - request.gas = BNToHex(gasLimitBN.muln(GAS_ESTIMATE_FALLBACK_MULTIPLIER)); request.value = value || '0x0'; - let estimatedGas = request.gas; - let simulationFails; + delete request.gasPrice; + delete request.maxFeePerGas; + delete request.maxPriorityFeePerGas; + + let estimatedGas = fallback; + let simulationFails: TransactionMeta['simulationFails']; try { estimatedGas = await query(ethQuery, 'estimateGas', [request]); - // TODO: Replace `any` with type // eslint-disable-next-line @typescript-eslint/no-explicit-any } catch (error: any) { simulationFails = { @@ -72,15 +84,15 @@ export async function estimateGas( errorKey: error.errorKey, debug: { blockNumber, - blockGasLimit: gasLimitHex, + blockGasLimit, }, }; - log('Estimation failed', { ...simulationFails, fallback: estimateGas }); + log('Estimation failed', { ...simulationFails, fallback }); } return { - blockGasLimit: gasLimitHex, + blockGasLimit, estimatedGas, simulationFails, }; @@ -92,8 +104,14 @@ export function addGasBuffer( multiplier: number, ) { const estimatedGasBN = hexToBN(estimatedGas); - const maxGasBN = hexToBN(blockGasLimit).muln(0.9); - const paddedGasBN = estimatedGasBN.muln(multiplier); + + const maxGasBN = fractionBN( + hexToBN(blockGasLimit), + MAX_GAS_BLOCK_PERCENT, + 100, + ); + + const paddedGasBN = fractionBN(estimatedGasBN, multiplier * 100, 100); if (estimatedGasBN.gt(maxGasBN)) { const estimatedGasHex = add0x(estimatedGas);