refactor(transaction-controller): address PR review feedback

Remove instant option from addTransaction in favour of dedicated startTransaction method. Extract validation into reusable functions, move pipeline types to lifecycle/types.ts, deduplicate shared setup into createTransaction, alphabetise TransactionContext, and remove premature ready check in approveTransaction.

Author: matthewwalsh0

packages/transaction-controller/CHANGELOG.md

@@ -9,9 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
-- Add `instant` option to `addTransaction` that adds the transaction to state immediately with default values, deferring gas estimation, fee calculation, and type resolution to the background ([#XXXX](https://github.com/MetaMask/core/pull/XXXX))
-- Add `ready` property to `TransactionMeta` that indicates whether essential async data has been resolved for instant transactions ([#XXXX](https://github.com/MetaMask/core/pull/XXXX))
-- Add safety guards that prevent approving, speeding up, or cancelling transactions when `ready` is `false` ([#XXXX](https://github.com/MetaMask/core/pull/XXXX))
+- Add `startTransaction` method for synchronous transaction creation with deferred async data resolution ([#8248](https://github.com/MetaMask/core/pull/8248))
+  - Transaction is immediately available in state with `ready: false`
+  - Gas estimation, fee calculation, and type resolution run in the background
+  - `ready` flips to `true` once essential data resolves
 
 ## [62.22.0]
 

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

@@ -3499,7 +3499,7 @@ describe('TransactionController', () => {
       });
 
       describe('ready gate', () => {
-        it('resolves data before approval when using instant via addTransaction', async () => {
+        it('resolves data before approval when using startTransaction', async () => {
           const { controller } = setupController({
             messengerOptions: {
               addTransactionApprovalRequest: {
@@ -3508,7 +3508,7 @@ describe('TransactionController', () => {
             },
           });
 
-          const { result } = await controller.addTransaction(
+          const { result } = controller.startTransaction(
             {
               from: ACCOUNT_MOCK,
               gas: '0x0',
@@ -3518,7 +3518,6 @@ describe('TransactionController', () => {
             },
             {
               networkClientId: NETWORK_CLIENT_ID_MOCK,
-              instant: true,
             },
           );
 
@@ -3920,36 +3919,34 @@ describe('TransactionController', () => {
       });
     });
 
-    describe('when instant option is true', () => {
-      it('returns immediately with ready false', async () => {
+    describe('startTransaction', () => {
+      it('returns immediately with ready false', () => {
         const { controller } = setupController();
 
-        const { transactionMeta } = await controller.addTransaction(
+        const { transactionMeta } = controller.startTransaction(
           {
             from: ACCOUNT_MOCK,
             to: ACCOUNT_MOCK,
           },
           {
             networkClientId: NETWORK_CLIENT_ID_MOCK,
-            instant: true,
           },
         );
 
         expect(transactionMeta.ready).toBe(false);
         expect(transactionMeta.status).toBe(TransactionStatus.unapproved);
       });
 
-      it('adds transaction to state immediately', async () => {
+      it('adds transaction to state immediately', () => {
         const { controller } = setupController();
 
-        const { transactionMeta } = await controller.addTransaction(
+        const { transactionMeta } = controller.startTransaction(
           {
             from: ACCOUNT_MOCK,
             to: ACCOUNT_MOCK,
           },
           {
             networkClientId: NETWORK_CLIENT_ID_MOCK,
-            instant: true,
           },
         );
 
@@ -3964,14 +3961,13 @@ describe('TransactionController', () => {
       it('sets ready to true after background resolution', async () => {
         const { controller } = setupController();
 
-        const { transactionMeta } = await controller.addTransaction(
+        const { transactionMeta } = controller.startTransaction(
           {
             from: ACCOUNT_MOCK,
             to: ACCOUNT_MOCK,
           },
           {
             networkClientId: NETWORK_CLIENT_ID_MOCK,
-            instant: true,
           },
         );
 
@@ -3984,47 +3980,45 @@ describe('TransactionController', () => {
         expect(txInState?.ready).toBe(true);
       });
 
-      it('throws if instant is true with an external origin', async () => {
+      it('throws with an external origin', () => {
         const { controller } = setupController();
 
-        await expect(
-          controller.addTransaction(
+        expect(() =>
+          controller.startTransaction(
             {
               from: ACCOUNT_MOCK,
               to: ACCOUNT_MOCK,
             },
             {
               networkClientId: NETWORK_CLIENT_ID_MOCK,
-              instant: true,
               origin: 'https://metamask.github.io/test-dapp/',
             },
           ),
-        ).rejects.toThrow(
-          'The instant option is not supported for external transactions.',
+        ).toThrow(
+          'startTransaction is not supported for external transactions.',
         );
       });
 
-      it('does not throw if instant is true with ORIGIN_METAMASK', async () => {
+      it('does not throw with ORIGIN_METAMASK', () => {
         const { controller } = setupController();
 
-        const addResult = await controller.addTransaction(
+        const startResult = controller.startTransaction(
           {
             from: ACCOUNT_MOCK,
             to: ACCOUNT_MOCK,
           },
           {
             networkClientId: NETWORK_CLIENT_ID_MOCK,
-            instant: true,
             origin: ORIGIN_METAMASK,
           },
         );
 
-        expect(addResult).toMatchObject({
+        expect(startResult).toMatchObject({
           transactionMeta: expect.objectContaining({ ready: false }),
         });
       });
 
-      it('publishes unapprovedTransactionAdded event immediately', async () => {
+      it('publishes unapprovedTransactionAdded event immediately', () => {
         const { controller, messenger } = setupController();
         const listener = jest.fn();
 
@@ -4033,22 +4027,21 @@ describe('TransactionController', () => {
           listener,
         );
 
-        await controller.addTransaction(
+        controller.startTransaction(
           {
             from: ACCOUNT_MOCK,
             to: ACCOUNT_MOCK,
           },
           {
             networkClientId: NETWORK_CLIENT_ID_MOCK,
-            instant: true,
           },
         );
 
         expect(listener).toHaveBeenCalledTimes(1);
         expect(listener.mock.calls[0][0]).toMatchObject({ ready: false });
       });
 
-      it('does not set ready when instant is not true', async () => {
+      it('does not set ready when using addTransaction', async () => {
         const { controller } = setupController();
 
         const { transactionMeta } = await controller.addTransaction(
@@ -4064,42 +4057,38 @@ describe('TransactionController', () => {
         expect(transactionMeta.ready).toBeUndefined();
       });
 
-      it('uses caller-provided type without waiting for background', async () => {
+      it('uses caller-provided type without waiting for background', () => {
         const { controller } = setupController();
 
-        const { transactionMeta } = await controller.addTransaction(
+        const { transactionMeta } = controller.startTransaction(
           {
             from: ACCOUNT_MOCK,
             to: ACCOUNT_MOCK,
           },
           {
             networkClientId: NETWORK_CLIENT_ID_MOCK,
-            instant: true,
             type: TransactionType.simpleSend,
           },
         );
 
         expect(transactionMeta.type).toBe(TransactionType.simpleSend);
       });
 
-      it('works with requireApproval false', async () => {
+      it('works with requireApproval false', () => {
         const { controller } = setupController();
 
-        const { transactionMeta, result } = await controller.addTransaction(
+        const { transactionMeta, result } = controller.startTransaction(
           {
             from: ACCOUNT_MOCK,
             to: ACCOUNT_MOCK,
           },
           {
             networkClientId: NETWORK_CLIENT_ID_MOCK,
-            instant: true,
             requireApproval: false,
           },
         );
 
-        result.catch(() => {
-          // Expected: ready gate rejects because ready is false
-        });
+        result.catch(() => undefined);
 
         expect(transactionMeta.ready).toBe(false);
         expect(
@@ -4109,28 +4098,28 @@ describe('TransactionController', () => {
         ).toBe(true);
       });
 
-      it('adds multiple instant transactions in quick succession', async () => {
+      it('adds multiple transactions in quick succession', () => {
         uuidModuleMock.v1
           .mockImplementationOnce(() => 'aaa-instant-1')
           .mockImplementationOnce(() => 'bbb-instant-2')
           .mockImplementationOnce(() => 'ccc-instant-3');
 
         const { controller } = setupController();
 
-        const results = await Promise.all([
-          controller.addTransaction(
+        const results = [
+          controller.startTransaction(
             { from: ACCOUNT_MOCK, to: ACCOUNT_MOCK },
-            { networkClientId: NETWORK_CLIENT_ID_MOCK, instant: true },
+            { networkClientId: NETWORK_CLIENT_ID_MOCK },
           ),
-          controller.addTransaction(
+          controller.startTransaction(
             { from: ACCOUNT_MOCK, to: ACCOUNT_MOCK },
-            { networkClientId: NETWORK_CLIENT_ID_MOCK, instant: true },
+            { networkClientId: NETWORK_CLIENT_ID_MOCK },
           ),
-          controller.addTransaction(
+          controller.startTransaction(
             { from: ACCOUNT_MOCK, to: ACCOUNT_MOCK },
-            { networkClientId: NETWORK_CLIENT_ID_MOCK, instant: true },
+            { networkClientId: NETWORK_CLIENT_ID_MOCK },
           ),
-        ]);
+        ];
 
         const ids = results.map((res) => res.transactionMeta.id);
 
@@ -4147,14 +4136,13 @@ describe('TransactionController', () => {
 
         const { controller } = setupController();
 
-        const { transactionMeta, result } = await controller.addTransaction(
+        const { transactionMeta, result } = controller.startTransaction(
           {
             from: ACCOUNT_MOCK,
             to: ACCOUNT_MOCK,
           },
           {
             networkClientId: NETWORK_CLIENT_ID_MOCK,
-            instant: true,
           },
         );
 
@@ -4654,14 +4642,13 @@ describe('TransactionController', () => {
       it('throws when transaction has ready false', async () => {
         const { controller } = setupController();
 
-        const { transactionMeta } = await controller.addTransaction(
+        const { transactionMeta } = controller.startTransaction(
           {
             from: ACCOUNT_MOCK,
             to: ACCOUNT_MOCK,
           },
           {
             networkClientId: NETWORK_CLIENT_ID_MOCK,
-            instant: true,
           },
         );
 
@@ -5132,14 +5119,13 @@ describe('TransactionController', () => {
       it('throws when transaction has ready false', async () => {
         const { controller } = setupController();
 
-        const { transactionMeta } = await controller.addTransaction(
+        const { transactionMeta } = controller.startTransaction(
           {
             from: ACCOUNT_MOCK,
             to: ACCOUNT_MOCK,
           },
           {
             networkClientId: NETWORK_CLIENT_ID_MOCK,
-            instant: true,
           },
         );
 

packages/transaction-controller/src/TransactionController.ts

@@ -89,6 +89,10 @@ import {
   startTransaction as startTransactionPipeline,
   addTransaction as addTransactionPipeline,
 } from './lifecycle/pipeline';
+import type {
+  StartTransactionResult,
+  TransactionContext,
+} from './lifecycle/types';
 import { projectLogger as log } from './logger';
 import type {
   DappSuggestedGasFees,
@@ -127,8 +131,6 @@ import type {
   PublishHookResult,
   GetGasFeeTokensRequest,
   InternalAccount,
-  StartTransactionResult,
-  TransactionContext,
 } from './types';
 import {
   GasFeeEstimateLevel,
@@ -3053,16 +3055,6 @@ export class TransactionController extends BaseController<
 
     log('Approving transaction', transactionMeta);
 
-    if (transactionMeta.ready === false) {
-      this.#failTransaction(
-        transactionMeta,
-        new Error(
-          'Transaction is not ready. Essential async data has not resolved yet.',
-        ),
-      );
-      return ApprovalState.NotApproved;
-    }
-
     try {
       if (!this.#sign) {
         this.#failTransaction(

packages/transaction-controller/src/index.ts

@@ -95,10 +95,6 @@ export type {
   TransactionMeta,
   TransactionParams,
   TransactionReceipt,
-  PipelineCallbacks,
-  StartTransactionResult,
-  TransactionContext,
-  TransactionStage,
   ValidateSecurityRequest,
 } from './types';
 export {
@@ -124,3 +120,9 @@ export { SUPPORTED_CHAIN_IDS as INCOMING_TRANSACTIONS_SUPPORTED_CHAIN_IDS } from
 export { HARDFORK } from './utils/prepare';
 export { addTransaction, startTransaction } from './lifecycle/pipeline';
 export { data } from './lifecycle/stages/data';
+export type {
+  PipelineCallbacks,
+  StartTransactionResult,
+  TransactionContext,
+  TransactionStage,
+} from './lifecycle/types';