fix(transaction-controller): use node-authoritative gas estimation

packages/transaction-controller/CHANGELOG.md

@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- Use the selected network client's gas estimate for transactions without caller-provided gas instead of assigning a fixed 21,000 gas limit to plain transfers ([#10245](https://github.com/MetaMask/core/pull/10245))
 - Bump `uuid` from `^9.0.1` to `^11.1.1` ([#10243](https://github.com/MetaMask/core/pull/10243))
 
 ## [70.0.1]

packages/transaction-controller/src/TransactionControllerIntegration.test.ts

@@ -498,8 +498,10 @@ describe('TransactionController Integration', () => {
           ),
           mocks: [
             buildEthBlockNumberRequestMock('0x1'),
+            buildEthGetBlockByNumberRequestMock('0x1'),
             buildEthGetCodeRequestMock(ACCOUNT_MOCK),
             buildEthGetCodeRequestMock(ACCOUNT_2_MOCK),
+            buildEthEstimateGasRequestMock(ACCOUNT_MOCK, ACCOUNT_2_MOCK),
             buildEthGasPriceRequestMock(),
             buildEthGasPriceRequestMock(),
           ],
@@ -513,6 +515,9 @@ describe('TransactionController Integration', () => {
           { networkClientId: 'sepolia' },
         );
         expect(transactionController.state.transactions).toHaveLength(1);
+        expect(transactionController.state.transactions[0].txParams.gas).toBe(
+          '0x5208',
+        );
         expect(transactionController.state.transactions[0].status).toBe(
           'unapproved',
         );
@@ -1066,6 +1071,7 @@ describe('TransactionController Integration', () => {
             buildEthGetCodeRequestMock(ACCOUNT_2_MOCK),
             buildEthGetCodeRequestMock(ACCOUNT_3_MOCK),
             buildEthEstimateGasRequestMock(ACCOUNT_MOCK, ACCOUNT_2_MOCK),
+            buildEthEstimateGasRequestMock(ACCOUNT_MOCK, ACCOUNT_3_MOCK),
             buildEthGasPriceRequestMock(),
             buildEthGasPriceRequestMock(),
             buildEthGetTransactionCountRequestMock(ACCOUNT_MOCK),

packages/transaction-controller/src/utils/gas.test.ts

@@ -10,7 +10,7 @@ import type {
 import { simulateTransactions } from '../api/simulation-api.js';
 import type { TransactionControllerMessenger } from '../TransactionController.js';
 import { TransactionEnvelopeType } from '../types.js';
-import type { TransactionMeta } from '../types.js';
+import type { TransactionMeta, TransactionParams } from '../types.js';
 import type {
   AuthorizationList,
   BatchTransactionParams,
@@ -31,7 +31,6 @@ import {
   estimateGasBatch,
   getProvidedBatchGasLimits,
   updateGas,
-  FIXED_GAS,
   DEFAULT_GAS_MULTIPLIER,
   MAX_GAS_BLOCK_PERCENT,
   INTRINSIC_GAS,
@@ -67,6 +66,8 @@ const GAS_2_MOCK = 12345;
 const SIMULATE_GAS_MOCK = 54321;
 const FROM_MOCK = '0xabc';
 const TO_MOCK = '0xdef';
+const NEW_ACCOUNT_MOCK = '0x0000000000000000000000000000000000000001';
+const DELEGATED_ACCOUNT_MOCK = '0x0000000000000000000000000000000000000002';
 const VALUE_MOCK = '0x1';
 const VALUE_MOCK_2 = '0x2';
 const DATA_MOCK = '0xabcdef';
@@ -461,52 +462,99 @@ describe('gas', () => {
         );
       });
 
-      describe('to fixed value', () => {
-        it('if not custom network and to parameter and no data and no code', async () => {
-          updateGasRequest.isCustomNetwork = false;
-          delete updateGasRequest.txMeta.txParams.data;
+      describe.each<[string, TransactionParams, Hex, Hex]>([
+        [
+          'self-transfer',
+          { from: FROM_MOCK, to: FROM_MOCK, value: '0x0' },
+          '0x5208',
+          '0x2ee0',
+        ],
+        [
+          'zero-value transfer to a distinct recipient',
+          { from: FROM_MOCK, to: TO_MOCK, value: '0x0' },
+          '0x5208',
+          '0x3a98',
+        ],
+        [
+          'value transfer to an existing recipient',
+          { from: FROM_MOCK, to: TO_MOCK, value: VALUE_MOCK },
+          '0x5208',
+          '0x5208',
+        ],
+        [
+          'value transfer to a new account',
+          { from: FROM_MOCK, to: NEW_ACCOUNT_MOCK, value: VALUE_MOCK },
+          '0x5208',
+          '0xb3b0',
+        ],
+        [
+          'contract creation',
+          { from: FROM_MOCK, data: DATA_MOCK, value: '0x0' },
+          '0xcf08',
+          '0x5dc0',
+        ],
+        [
+          'transfer to a delegated recipient',
+          { from: FROM_MOCK, to: DELEGATED_ACCOUNT_MOCK, value: '0x0' },
+          '0x6590',
+          '0x6b6c',
+        ],
+        [
+          'transfer from a delegated sender',
+          { from: DELEGATED_ACCOUNT_MOCK, to: TO_MOCK, value: '0x0' },
+          '0x6978',
+          '0x7334',
+        ],
+      ])('%s', (_description, txParams, legacyNodeGas, upgradedNodeGas) => {
+        it.each<[string, Hex]>([
+          ['legacy node', legacyNodeGas],
+          ['upgraded node', upgradedNodeGas],
+        ])('uses the %s response', async (_node, estimatedGas) => {
+          updateGasRequest.txMeta.txParams = cloneDeep(txParams);
+          updateGasRequest.txMeta.disableGasBuffer = true;
 
           mockQuery({
-            getCodeResponse: null,
+            getBlockByNumberResponse: {
+              gasLimit: toHex(BLOCK_GAS_LIMIT_MOCK),
+            },
+            estimateGasResponse: estimatedGas,
           });
 
           await updateGas(updateGasRequest);
 
-          expect(updateGasRequest.txMeta.txParams.gas).toBe(FIXED_GAS);
+          expect(updateGasRequest.txMeta.txParams.gas).toBe(estimatedGas);
+          expect(updateGasRequest.txMeta.gasLimitNoBuffer).toBe(estimatedGas);
           expect(updateGasRequest.txMeta.originalGasEstimate).toBe(
-            updateGasRequest.txMeta.txParams.gas,
+            estimatedGas,
           );
-          expectEstimateGasNotCalled();
-        });
-
-        it('if not custom network and to parameter and no data and empty code', async () => {
-          updateGasRequest.isCustomNetwork = false;
-          delete updateGasRequest.txMeta.txParams.data;
-
-          mockQuery({
-            getCodeResponse: '0x',
+          expect(rpcRequestMock).toHaveBeenCalledWith({
+            messenger: MESSENGER_MOCK,
+            networkClientId: NETWORK_CLIENT_ID_MOCK,
+            method: 'eth_estimateGas',
+            params: [
+              expect.objectContaining({
+                ...txParams,
+                value: txParams.value ?? '0x0',
+              }),
+            ],
           });
-
-          await updateGas(updateGasRequest);
-
-          expect(updateGasRequest.txMeta.txParams.gas).toBe(FIXED_GAS);
-          expect(updateGasRequest.txMeta.originalGasEstimate).toBe(
-            updateGasRequest.txMeta.txParams.gas,
+          expect(rpcRequestMock).not.toHaveBeenCalledWith(
+            expect.objectContaining({ method: 'eth_getCode' }),
           );
-          expectEstimateGasNotCalled();
         });
       });
     });
 
     describe('on estimate query error', () => {
-      it('sets gas to 35% of block gas limit', async () => {
+      it('sets gas to 35% of block gas limit for a plain transfer', async () => {
         const fallbackGas = Math.floor(
           BLOCK_GAS_LIMIT_MOCK * FALLBACK_MULTIPLIER_35_PERCENT,
         );
 
         getGasEstimateFallbackMock.mockReturnValue(
           GAS_ESTIMATE_FALLBACK_MULTIPLIER_MOCK,
         );
+        delete updateGasRequest.txMeta.txParams.data;
 
         mockQuery({
           getBlockByNumberResponse: {
@@ -521,6 +569,9 @@ describe('gas', () => {
         expect(updateGasRequest.txMeta.originalGasEstimate).toBe(
           updateGasRequest.txMeta.txParams.gas,
         );
+        expect(rpcRequestMock).toHaveBeenCalledWith(
+          expect.objectContaining({ method: 'eth_estimateGas' }),
+        );
       });
 
       it('sets simulationFails property', async () => {
@@ -914,7 +965,16 @@ describe('gas', () => {
       },
     );
 
-    it('normalizes authorization list in estimate request', async () => {
+    it('forwards the complete transaction shape in estimate request', async () => {
+      const accessList = [
+        {
+          address: '0x0000000000000000000000000000000000000001',
+          storageKeys: [
+            '0x0000000000000000000000000000000000000000000000000000000000000002',
+          ],
+        },
+      ];
+
       mockQuery({
         getBlockByNumberResponse: { gasLimit: toHex(BLOCK_GAS_LIMIT_MOCK) },
         estimateGasResponse: toHex(GAS_MOCK),
@@ -927,7 +987,9 @@ describe('gas', () => {
         messenger: MESSENGER_MOCK,
         txParams: {
           ...TRANSACTION_META_MOCK.txParams,
+          accessList,
           authorizationList: AUTHORIZATION_LIST_MOCK,
+          type: TransactionEnvelopeType.setCode,
           value: undefined,
         },
       });
@@ -939,6 +1001,7 @@ describe('gas', () => {
         params: [
           {
             ...TRANSACTION_META_MOCK.txParams,
+            accessList,
             authorizationList: [
               {
                 ...AUTHORIZATION_LIST_MOCK[0],
@@ -949,6 +1012,7 @@ describe('gas', () => {
                 yParity: '0x1',
               },
             ],
+            type: TransactionEnvelopeType.setCode,
             value: '0x0',
           },
         ],

packages/transaction-controller/src/utils/gas.ts

@@ -57,7 +57,6 @@ export type EstimateGasBatchResult = {
 
 export const log = createModuleLogger(projectLogger, 'gas');
 
-export const FIXED_GAS = '0x5208';
 export const DEFAULT_GAS_MULTIPLIER = 1.5;
 export const MAX_GAS_BLOCK_PERCENT = 90;
 export const INTRINSIC_GAS = 21000;
@@ -516,11 +515,6 @@ async function getGas(
     return [txMeta.txParams.gas, undefined, txMeta.txParams.gas];
   }
 
-  if (await requiresFixedGas(request)) {
-    log('Using fixed value', FIXED_GAS);
-    return [FIXED_GAS, undefined, FIXED_GAS];
-  }
-
   const {
     blockGasLimit,
     estimatedGas,
@@ -569,60 +563,6 @@ async function getGas(
   return [bufferedGas, simulationFails, estimatedGas, gasRevert];
 }
 
-/**
- * Determine if the gas for the provided request should be fixed.
- *
- * @param options - The options object.
- * @param options.messenger - The messenger instance for communication.
- * @param options.txMeta - The transaction meta object.
- * @param options.isCustomNetwork - Whether the network is a custom network.
- * @returns Whether the gas should be fixed.
- */
-async function requiresFixedGas({
-  messenger,
-  txMeta,
-  isCustomNetwork,
-}: UpdateGasRequest): Promise<boolean> {
-  const {
-    networkClientId,
-    txParams: { to, data, type },
-  } = txMeta;
-
-  if (
-    isCustomNetwork ||
-    !to ||
-    data ||
-    type === TransactionEnvelopeType.setCode
-  ) {
-    return false;
-  }
-
-  const code = await getCode(messenger, networkClientId, to);
-
-  return !code || code === '0x';
-}
-
-/**
- * Get the contract code for the provided address.
- *
- * @param messenger - The messenger instance for communication.
- * @param networkClientId - The network client ID.
- * @param address - The address to get the code for.
- * @returns The contract code.
- */
-async function getCode(
-  messenger: TransactionControllerMessenger,
-  networkClientId: NetworkClientId,
-  address: string,
-): Promise<string | undefined> {
-  return (await rpcRequest({
-    messenger,
-    networkClientId,
-    method: 'eth_getCode',
-    params: [address, 'latest'],
-  })) as string | undefined;
-}
-
 /**
  * Get the latest block from the network.
  *

packages/transaction-controller/tests/JsonRpcRequestMocks.ts

@@ -45,7 +45,7 @@ export function buildEthBlockNumberRequestMock(
 
 /**
  * Builds mock eth_getCode request.
- * Used by readAddressAsContract and requiresFixedGas.
+ * Used by readAddressAsContract.
  *
  * @param address - The hex address.
  * @param blockNumber - The hex block number.
@@ -106,7 +106,7 @@ export function buildEthGetBlockByNumberRequestMock(
 export function buildEthEstimateGasRequestMock(
   from: Hex,
   to: Hex,
-  result: Hex = '0x1',
+  result: Hex = '0x5208',
 ): JsonRpcRequestMock {
   return {
     request: {
@@ -116,7 +116,7 @@ export function buildEthEstimateGasRequestMock(
           from,
           to,
           value: '0x0',
-          gas: '0x0',
+          type: '0x2',
         },
       ],
     },