Refactor to use UUID instead of random number for txId

app/scripts/controllers/transactions/EtherscanRemoteTransactionSource.test.ts

@@ -3,7 +3,6 @@ import {
   TransactionStatus,
   TransactionType,
 } from '../../../../shared/constants/transaction';
-import createRandomId from '../../../../shared/modules/random-id';
 import type {
   EtherscanTokenTransactionMeta,
   EtherscanTransactionMeta,
@@ -21,9 +20,15 @@ jest.mock('./etherscan', () => ({
   fetchEtherscanTokenTransactions: jest.fn(),
 }));
 
-jest.mock('../../../../shared/modules/random-id');
+const ID_MOCK = '123';
+jest.mock('uuid', () => {
+  const actual = jest.requireActual('uuid');
 
-const ID_MOCK = 123;
+  return {
+    ...actual,
+    v1: () => ID_MOCK,
+  };
+});
 
 const ETHERSCAN_TRANSACTION_BASE_MOCK: EtherscanTransactionMetaBase = {
   blockNumber: '4535105',
@@ -136,8 +141,6 @@ describe('EtherscanRemoteTransactionSource', () => {
       typeof fetchEtherscanTokenTransactions
     >;
 
-  const createIdMock = createRandomId as jest.MockedFn<typeof createRandomId>;
-
   beforeEach(() => {
     jest.resetAllMocks();
 
@@ -148,8 +151,6 @@ describe('EtherscanRemoteTransactionSource', () => {
     fetchEtherscanTokenTransactionsMock.mockResolvedValue(
       ETHERSCAN_TOKEN_TRANSACTION_RESPONSE_EMPTY_MOCK,
     );
-
-    createIdMock.mockReturnValue(ID_MOCK);
   });
 
   describe('isSupportedNetwork', () => {

app/scripts/controllers/transactions/EtherscanRemoteTransactionSource.ts

@@ -1,7 +1,7 @@
 import { BNToHex } from '@metamask/controller-utils';
 import type { Hex } from '@metamask/utils';
 import { BN } from 'ethereumjs-util';
-import createId from '../../../../shared/modules/random-id';
+import { v1 as uuid } from 'uuid';
 
 import {
   TransactionMeta,
@@ -138,7 +138,7 @@ export class EtherscanRemoteTransactionSource
       blockNumber: txMeta.blockNumber,
       chainId: currentChainId,
       hash: txMeta.hash,
-      id: createId(),
+      id: uuid(),
       metamaskNetworkId: currentNetworkId,
       status: TransactionStatus.confirmed,
       time,

app/scripts/controllers/transactions/index.js

@@ -2841,7 +2841,7 @@ export default class TransactionController extends EventEmitter {
       const { origin } = txMeta;
 
       const approvalResult = await this._requestApproval(
-        String(txId),
+        txId,
         origin,
         { txId },
         {

app/scripts/controllers/transactions/tx-state-manager.js

@@ -2,7 +2,7 @@ import EventEmitter from '@metamask/safe-event-emitter';
 import { ObservableStore } from '@metamask/obs-store';
 import log from 'loglevel';
 import { values, keyBy, mapValues, omitBy, pickBy, sortBy } from 'lodash';
-import createId from '../../../../shared/modules/random-id';
+import { v1 as uuid } from 'uuid';
 import { TransactionStatus } from '../../../../shared/constants/transaction';
 import { METAMASK_CONTROLLER_EVENTS } from '../../metamask-controller';
 import { transactionMatchesNetwork } from '../../../../shared/modules/transaction.utils';
@@ -130,7 +130,7 @@ export default class TransactionStateManager extends EventEmitter {
     }
 
     return {
-      id: createId(),
+      id: uuid(),
       time: new Date().getTime(),
       status: TransactionStatus.unapproved,
       metamaskNetworkId: networkId,

app/scripts/migrations/099.test.ts

@@ -0,0 +1,91 @@
+import { migrate, version } from './099';
+
+const oldVersion = 98;
+describe('migration #99', () => {
+  it('updates the version metadata', async () => {
+    const oldStorage = {
+      meta: { version: oldVersion },
+      data: {},
+    };
+
+    const newStorage = await migrate(oldStorage);
+
+    expect(newStorage.meta).toStrictEqual({ version });
+  });
+
+  it('handles missing TransactionController', async () => {
+    const oldState = {
+      OtherController: {},
+    };
+
+    const transformedState = await migrate({
+      meta: { version: oldVersion },
+      data: oldState,
+    });
+
+    expect(transformedState.data).toEqual(oldState);
+  });
+
+  it('handles empty transactions', async () => {
+    const oldState = {
+      TransactionController: {
+        transactions: {},
+      },
+    };
+
+    const transformedState = await migrate({
+      meta: { version: oldVersion },
+      data: oldState,
+    });
+
+    expect(transformedState.data).toEqual(oldState);
+  });
+
+  it('handles missing state', async () => {
+    const transformedState = await migrate({
+      meta: { version: oldVersion },
+      data: {},
+    });
+
+    expect(transformedState.data).toEqual({});
+  });
+
+  it('updates transaction ids', async () => {
+    const oldState = {
+      TransactionController: {
+        transactions: {
+          1: {
+            id: 1,
+            otherProp: 1,
+          },
+          2: {
+            id: 2,
+            otherProp: 2,
+          },
+        },
+      },
+    };
+    const oldStorage = {
+      meta: { version: oldVersion },
+      data: oldState,
+    };
+
+    const newStorage = await migrate(oldStorage);
+
+    const migratedTransactions = (newStorage.data.TransactionController as any)
+      .transactions;
+
+    const [firstTxId, secondTxId] = Object.keys(migratedTransactions);
+
+    expect(migratedTransactions).toStrictEqual({
+      [firstTxId]: {
+        id: firstTxId,
+        otherProp: 1,
+      },
+      [secondTxId]: {
+        id: secondTxId,
+        otherProp: 2,
+      },
+    });
+  });
+});

app/scripts/migrations/099.ts

@@ -0,0 +1,56 @@
+import { cloneDeep, isEmpty } from 'lodash';
+import { v1 as uuid } from 'uuid';
+
+type VersionedData = {
+  meta: { version: number };
+  data: Record<string, unknown>;
+};
+
+export const version = 99;
+
+/**
+ * The core TransactionController uses strings for transaction IDs, specifically UUIDs generated by the uuid package.
+ * For the sake of standardisation and minimising code maintenance, the use of UUIDs is preferred.
+ * This migration updates the transaction IDs to UUIDs.
+ *
+ * @param originalVersionedData
+ */
+export async function migrate(
+  originalVersionedData: VersionedData,
+): Promise<VersionedData> {
+  const versionedData = cloneDeep(originalVersionedData);
+  versionedData.meta.version = version;
+  transformState(versionedData.data);
+  return versionedData;
+}
+
+function transformState(state: Record<string, any>) {
+  const transactionControllerState = state?.TransactionController || {};
+  const transactions = transactionControllerState?.transactions || {};
+
+  if (isEmpty(transactions)) {
+    return;
+  }
+
+  const newTxs = Object.keys(transactions).reduce(
+    (txs: { [key: string]: any }, oldTransactionId) => {
+      // Clone the transaction
+      const transaction = cloneDeep(transactions[oldTransactionId]);
+
+      // Assign a new id to the transaction
+      const newTransactionID = uuid();
+      transaction.id = newTransactionID;
+
+      return {
+        ...txs,
+        [newTransactionID]: transaction,
+      };
+    },
+    {},
+  );
+
+  state.TransactionController = {
+    ...transactionControllerState,
+    transactions: newTxs,
+  };
+}

app/scripts/migrations/index.js

@@ -105,6 +105,7 @@ import * as m095 from './095';
 import * as m096 from './096';
 import * as m097 from './097';
 import * as m098 from './098';
+import * as m099 from './099';
 
 const migrations = [
   m002,
@@ -207,5 +208,6 @@ const migrations = [
   m096,
   m097,
   m098,
+  m099,
 ];
 export default migrations;

shared/constants/transaction.ts

@@ -331,7 +331,7 @@ export interface TransactionMeta {
   blockNumber?: string;
   chainId: string;
   /** An internally unique tx identifier. */
-  id: number;
+  id: string;
   /** Time the transaction was first suggested, in unix epoch time (ms). */
   time: number;
   /** A string representing a name of transaction contract method. */