From 5b7a9071aeba69766d6771cdfa4053511a1a4b5c Mon Sep 17 00:00:00 2001 From: Goktug Poyraz Date: Fri, 15 Sep 2023 14:52:06 +0200 Subject: [PATCH] Refactor transaction ids to be UUIDs instead of random number --- .../EtherscanRemoteTransactionSource.ts | 4 +- app/scripts/controllers/transactions/index.js | 2 +- .../transactions/tx-state-manager.js | 4 +- app/scripts/migrations/098.test.ts | 83 +++++++++++++++++++ app/scripts/migrations/098.ts | 56 +++++++++++++ shared/constants/transaction.ts | 2 +- ui/ducks/app/app.ts | 2 +- .../confirm-transaction.component.js | 2 +- ui/store/actions.ts | 24 +++--- 9 files changed, 159 insertions(+), 20 deletions(-) create mode 100644 app/scripts/migrations/098.test.ts create mode 100644 app/scripts/migrations/098.ts diff --git a/app/scripts/controllers/transactions/EtherscanRemoteTransactionSource.ts b/app/scripts/controllers/transactions/EtherscanRemoteTransactionSource.ts index ee37d0d4b2ec..52660b6c918a 100644 --- a/app/scripts/controllers/transactions/EtherscanRemoteTransactionSource.ts +++ b/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 { v4 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, diff --git a/app/scripts/controllers/transactions/index.js b/app/scripts/controllers/transactions/index.js index 76c7d529e43e..d58aed054d84 100644 --- a/app/scripts/controllers/transactions/index.js +++ b/app/scripts/controllers/transactions/index.js @@ -2825,7 +2825,7 @@ export default class TransactionController extends EventEmitter { const { origin } = txMeta; const approvalResult = await this._requestApproval( - String(txId), + txId, origin, { txId }, { diff --git a/app/scripts/controllers/transactions/tx-state-manager.js b/app/scripts/controllers/transactions/tx-state-manager.js index 20e142c6c052..2d737908b317 100644 --- a/app/scripts/controllers/transactions/tx-state-manager.js +++ b/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 { v4 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, diff --git a/app/scripts/migrations/098.test.ts b/app/scripts/migrations/098.test.ts new file mode 100644 index 000000000000..86006872f737 --- /dev/null +++ b/app/scripts/migrations/098.test.ts @@ -0,0 +1,83 @@ +import { migrate, version } from './098'; + +const oldVersion = 97; +describe('migration #98', () => { + 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.transactions; + + Object.keys(migratedTransactions).forEach((newTxId) => { + expect(typeof newTxId).toBe('string'); + expect(newTxId).toBe(migratedTransactions[newTxId].id); + }); + }); +}); diff --git a/app/scripts/migrations/098.ts b/app/scripts/migrations/098.ts new file mode 100644 index 000000000000..194adf86da8c --- /dev/null +++ b/app/scripts/migrations/098.ts @@ -0,0 +1,56 @@ +import { cloneDeep, isEmpty } from 'lodash'; +import { v4 as uuid } from 'uuid'; + +type VersionedData = { + meta: { version: number }; + data: Record; +}; + +export const version = 98; + +/** + * 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 { + const versionedData = cloneDeep(originalVersionedData); + versionedData.meta.version = version; + transformState(versionedData.data); + return versionedData; +} + +function transformState(state: Record) { + 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, + }; +} diff --git a/shared/constants/transaction.ts b/shared/constants/transaction.ts index 8a5ac8f98eb6..c83040ec4442 100644 --- a/shared/constants/transaction.ts +++ b/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. */ diff --git a/ui/ducks/app/app.ts b/ui/ducks/app/app.ts index 4a440031c0fe..f4db4a5a4ec6 100644 --- a/ui/ducks/app/app.ts +++ b/ui/ducks/app/app.ts @@ -69,7 +69,7 @@ interface AppState { newTokensImported: string; onboardedInThisUISession: boolean; customTokenAmount: string; - txId: number | null; + txId: string | null; accountDetailsAddress: string; ///: BEGIN:ONLY_INCLUDE_IN(snaps) snapsInstallPrivacyWarningShown: boolean; diff --git a/ui/pages/confirm-transaction/confirm-transaction.component.js b/ui/pages/confirm-transaction/confirm-transaction.component.js index b570ea059164..2bfea276ce03 100644 --- a/ui/pages/confirm-transaction/confirm-transaction.component.js +++ b/ui/pages/confirm-transaction/confirm-transaction.component.js @@ -90,7 +90,7 @@ const ConfirmTransaction = () => { ]); const { id, type } = transaction; - const transactionId = id && String(id); + const transactionId = id; const isValidTokenMethod = isTokenMethodAction(type); const isValidTransactionId = transactionId && diff --git a/ui/store/actions.ts b/ui/store/actions.ts index 0ffabd57319a..7e8e08342180 100644 --- a/ui/store/actions.ts +++ b/ui/store/actions.ts @@ -699,7 +699,7 @@ export function decryptMsg( } dispatch(updateMetamaskState(newState)); - dispatch(completedTx(decryptedMsgData.metamaskId)); + dispatch(completedTx(decryptedMsgData.metamaskId as unknown as string)); dispatch(closeCurrentNotificationWindow()); return decryptedMsgData; }; @@ -732,7 +732,7 @@ export function encryptionPublicKeyMsg( } dispatch(updateMetamaskState(newState)); - dispatch(completedTx(msgData.metamaskId)); + dispatch(completedTx(msgData.metamaskId as unknown as string)); dispatch(closeCurrentNotificationWindow()); return msgData; }; @@ -774,7 +774,7 @@ const updateMetamaskStateFromBackground = (): Promise< * @param previousGasParams - Object of gas params to set as previous */ export function updatePreviousGasParams( - txId: number, + txId: string, previousGasParams: Record, ): ThunkAction< Promise, @@ -799,7 +799,7 @@ export function updatePreviousGasParams( } export function updateEditableParams( - txId: number, + txId: string, editableParams: Partial, ): ThunkAction< Promise, @@ -834,7 +834,7 @@ export function updateEditableParams( * @returns */ export function updateTransactionSendFlowHistory( - txId: number, + txId: string, currentSendFlowHistoryLength: number, sendFlowHistory: DraftTransaction['history'], ): ThunkAction< @@ -890,7 +890,7 @@ export async function restoreUserData(jsonString: Json): Promise { // TODO: codeword: NOT_A_THUNK @brad-decker export function updateTransactionGasFees( - txId: number, + txId: string, txGasFees: Partial, ): ThunkAction< Promise, @@ -1098,7 +1098,7 @@ export async function getTransactions( } export function completedTx( - txId: number, + txId: string, ): ThunkAction { return (dispatch: MetaMaskReduxDispatch) => { dispatch({ @@ -1110,7 +1110,7 @@ export function completedTx( }; } -export function updateTransactionParams(txId: number, txParams: TxParams) { +export function updateTransactionParams(txId: string, txParams: TxParams) { return { type: actionConstants.UPDATE_TRANSACTION_PARAMS, id: txId, @@ -1322,7 +1322,7 @@ export function cancelDecryptMsg( } dispatch(updateMetamaskState(newState)); - dispatch(completedTx(msgData.id)); + dispatch(completedTx(msgData.id as unknown as string)); dispatch(closeCurrentNotificationWindow()); return msgData; }; @@ -1349,7 +1349,7 @@ export function cancelEncryptionPublicKeyMsg( } dispatch(updateMetamaskState(newState)); - dispatch(completedTx(msgData.id)); + dispatch(completedTx(msgData.id as unknown as string)); dispatch(closeCurrentNotificationWindow()); return msgData; }; @@ -2047,12 +2047,12 @@ export function clearPendingTokens(): Action { } export function createCancelTransaction( - txId: number, + txId: string, customGasSettings: CustomGasSettings, options: { estimatedBaseFee?: string } = {}, ): ThunkAction { log.debug('background.cancelTransaction'); - let newTxId: number; + let newTxId: string; return (dispatch: MetaMaskReduxDispatch) => { const actionId = generateActionId();