Skip to content

Commit

Permalink
Patch: Compress transaction history (#26236)
Browse files Browse the repository at this point in the history
## **Description**

Compress transaction history down to 100 entries and truncate any
existing transaction historys to just 100 entries. This should fix
reports we've seen from production of extremely poor performance and
crashes caused by long transaction histories, and will also prevent this
problem from happening again.

Transaction history has been truncated because in the extreme cases, it
would be prohibitively expensive to compress. The downside is that some
state of how the transaction has changed may be lost. But this is
unlikely to impact users because we only show a limited number of events
from the transaction history in our UI, and these events are more likely
to be at the beginning of long transaction histories. Even if a
displayed event is lost, the impact on the UI is minimal (it's only
shown on the transaction details page under "Activity log", and only for
informational purposes).

For details on how the transaction compression works, and how it
prevents history size from growing unbouned, see
MetaMask/core#4555

The transaction controller change has been applied using a patch. The
patch was generated from the core repository branch
`patch/transaction-controller-v32-compress-history`. It will be made on
`develop` at a later date because it is blocked by other controller
updates at the moment.

The truncation is performed by a migration that was added in #26291 and
cherry-picked into this branch

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26236?quickstart=1)

## **Related issues**

Relates to #9372

## **Manual testing steps**

To test the migration:
* Checkout v11.15.6 and create a dev build (unfortunately the repro
steps don't work on MV3 due to snow being enabled even in dev builds, so
we're using a pre-MV3 build)
* Install the dev build from the `dist/chrome` directory and proceed
through onboarding
* Make a single transaction
* Any chain, any transaction, as long as it's approved. We don't even
need to wait for it to settle.
* Run this command in the background console to add a huge history to
the transaction controller and restart the extension:
  ```
  chrome.storage.local.get(
    null,
    (state) => {
      state.data.TransactionController.transactions[0].history.push(
        ...[...Array(100000).keys()].map(() => [
          {
value: '[ethjs-rpc] rpc error with payload
{"id":[number],"jsonrpc":"2.0","params":["0x"],"method":"eth_getTransactionReceipt"}
{ "code": -32603, "message": "Internal JSON-RPC error.", "data": {
"code": -32602, "message": "invalid argument 0: hex string has length 0,
want 64 for common.Hash", "cause": null }, "stack": ...',
            path: '/warning/error',
            op: 'replace'
          },
          {
            note: 'transactions/pending-tx-tracker#event: tx:warning',
            value: 'There was a problem loading this transaction.',
            path: '/warning/message',
            op: 'replace'
          },
        ]),
      );
      chrome.storage.local.set(state, () => chrome.runtime.reload());
    }
  );
  ```
  * The extension should become extremely slow
* Disable the extension
* Switch to this branch and create a dev build
* Enable and reload the extension
  * The extension should no longer be slow

To test that the compression is working, we would want to get a
transaction in a "stuck pending" state where an error is captured each
iteration. I am not yet sure how to do that unfortunately.

## **Screenshots/Recordings**

N/A

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
  • Loading branch information
Gudahtt authored Aug 5, 2024
1 parent ce1ed8d commit 014b56c
Show file tree
Hide file tree
Showing 6 changed files with 407 additions and 4 deletions.
Binary file not shown.
253 changes: 253 additions & 0 deletions app/scripts/migrations/120.3.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,253 @@
import { cloneDeep } from 'lodash';
import { migrate, version } from './120.3';

const sentryCaptureExceptionMock = jest.fn();

global.sentry = {
captureException: sentryCaptureExceptionMock,
};

const oldVersion = 120.2;

describe('migration #120.3', () => {
afterEach(() => {
jest.resetAllMocks();
});

it('updates the version metadata', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {},
};

const newStorage = await migrate(oldStorage);

expect(newStorage.meta).toStrictEqual({ version });
});

it('returns state unchanged if TransactionController state is missing', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
PreferencesController: {},
},
};
const oldStorageDataClone = cloneDeep(oldStorage.data);

const newStorage = await migrate(oldStorage);

expect(newStorage.data).toStrictEqual(oldStorageDataClone);
});

it('reports error and returns state unchanged if TransactionController state is invalid', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
PreferencesController: {},
TransactionController: 'invalid',
},
};
const oldStorageDataClone = cloneDeep(oldStorage.data);

const newStorage = await migrate(oldStorage);

expect(sentryCaptureExceptionMock).toHaveBeenCalledWith(
`Migration ${version}: Invalid TransactionController state of type 'string'`,
);
expect(newStorage.data).toStrictEqual(oldStorageDataClone);
});

it('returns state unchanged if transactions are missing', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
PreferencesController: {},
TransactionController: {},
},
};
const oldStorageDataClone = cloneDeep(oldStorage.data);

const newStorage = await migrate(oldStorage);

expect(newStorage.data).toStrictEqual(oldStorageDataClone);
});

it('reports error and returns state unchanged if transactions property is invalid', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
PreferencesController: {},
TransactionController: {
transactions: 'invalid',
},
},
};
const oldStorageDataClone = cloneDeep(oldStorage.data);

const newStorage = await migrate(oldStorage);

expect(sentryCaptureExceptionMock).toHaveBeenCalledWith(
`Migration ${version}: Invalid TransactionController transactions state of type 'string'`,
);
expect(newStorage.data).toStrictEqual(oldStorageDataClone);
});

it('reports error and returns state unchanged if there is an invalid transaction', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
PreferencesController: {},
TransactionController: {
transactions: [
{}, // empty object is valid for the purposes of this migration
'invalid',
{},
],
},
},
};
const oldStorageDataClone = cloneDeep(oldStorage.data);

const newStorage = await migrate(oldStorage);

expect(sentryCaptureExceptionMock).toHaveBeenCalledWith(
`Migration ${version}: Invalid transaction of type 'string'`,
);
expect(newStorage.data).toStrictEqual(oldStorageDataClone);
});

it('reports error and returns state unchanged if there is a transaction with an invalid history property', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
PreferencesController: {},
TransactionController: {
transactions: [
{}, // empty object is valid for the purposes of this migration
{
history: 'invalid',
},
{},
],
},
},
};
const oldStorageDataClone = cloneDeep(oldStorage.data);

const newStorage = await migrate(oldStorage);

expect(sentryCaptureExceptionMock).toHaveBeenCalledWith(
`Migration ${version}: Invalid transaction history of type 'string'`,
);
expect(newStorage.data).toStrictEqual(oldStorageDataClone);
});

it('returns state unchanged if there are no transactions', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
PreferencesController: {},
TransactionController: {
transactions: [],
},
},
};
const oldStorageDataClone = cloneDeep(oldStorage.data);

const newStorage = await migrate(oldStorage);

expect(newStorage.data).toStrictEqual(oldStorageDataClone);
});

it('returns state unchanged if there are no transactions with history', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
PreferencesController: {},
TransactionController: {
transactions: [{}, {}, {}],
},
},
};
const oldStorageDataClone = cloneDeep(oldStorage.data);

const newStorage = await migrate(oldStorage);

expect(newStorage.data).toStrictEqual(oldStorageDataClone);
});

it('returns state unchanged if there are no transactions with history exceeding max size', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
PreferencesController: {},
TransactionController: {
transactions: [
{
history: [...Array(99).keys()],
},
{
history: [...Array(100).keys()],
},
{
history: [],
},
],
},
},
};
const oldStorageDataClone = cloneDeep(oldStorage.data);

const newStorage = await migrate(oldStorage);

expect(newStorage.data).toStrictEqual(oldStorageDataClone);
});

it('trims histories exceeding max size', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
PreferencesController: {},
TransactionController: {
transactions: [
{
history: [...Array(99).keys()],
},
{
history: [...Array(100).keys()],
},
{
history: [...Array(101).keys()],
},
{
history: [...Array(1000).keys()],
},
],
},
},
};
const oldStorageDataClone = cloneDeep(oldStorage.data);

const newStorage = await migrate(oldStorage);

expect(newStorage.data).toStrictEqual({
...oldStorageDataClone,
TransactionController: {
transactions: [
{
history: [...Array(99).keys()],
},
{
history: [...Array(100).keys()],
},
{
history: [...Array(100).keys()],
},
{
history: [...Array(100).keys()],
},
],
},
});
});
});
114 changes: 114 additions & 0 deletions app/scripts/migrations/120.3.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
import { RuntimeObject, hasProperty, isObject } from '@metamask/utils';
import { cloneDeep } from 'lodash';
import log from 'loglevel';

type VersionedData = {
meta: { version: number };
data: Record<string, unknown>;
};

export const version = 120.3;

const MAX_TRANSACTION_HISTORY_LENGTH = 100;

/**
* This migration trims the size of any large transaction histories. This will
* result in some loss of information, but the impact is minor. The lost data
* is only used in the "Activity log" on the transaction details page.
*
* @param originalVersionedData - Versioned MetaMask extension state, exactly
* what we persist to dist.
* @param originalVersionedData.meta - State metadata.
* @param originalVersionedData.meta.version - The current state version.
* @param originalVersionedData.data - The persisted MetaMask state, keyed by
* controller.
* @returns Updated versioned MetaMask extension state.
*/
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, unknown>): void {
if (!hasProperty(state, 'TransactionController')) {
log.warn(`Migration ${version}: Missing TransactionController state`);
return;
} else if (!isObject(state.TransactionController)) {
global.sentry?.captureException(
`Migration ${version}: Invalid TransactionController state of type '${typeof state.TransactionController}'`,
);
return;
}

const transactionControllerState = state.TransactionController;

if (!hasProperty(transactionControllerState, 'transactions')) {
log.warn(
`Migration ${version}: Missing TransactionController transactions`,
);
return;
} else if (!Array.isArray(transactionControllerState.transactions)) {
global.sentry?.captureException(
`Migration ${version}: Invalid TransactionController transactions state of type '${typeof transactionControllerState.transactions}'`,
);
return;
}

const { transactions } = transactionControllerState;
const validTransactions = transactions.filter(isObject);
if (transactions.length !== validTransactions.length) {
const invalidTransaction = transactions.find(
(transaction) => !isObject(transaction),
);
global.sentry?.captureException(
`Migration ${version}: Invalid transaction of type '${typeof invalidTransaction}'`,
);
return;
}

const validHistoryTransactions = validTransactions.filter(
hasValidTransactionHistory,
);
if (validHistoryTransactions.length !== validTransactions.length) {
const invalidTransaction = validTransactions.find(
(transaction) => !hasValidTransactionHistory(transaction),
);
global.sentry?.captureException(
`Migration ${version}: Invalid transaction history of type '${typeof invalidTransaction?.history}'`,
);
return;
}

for (const transaction of validHistoryTransactions) {
if (
transaction.history &&
transaction.history.length > MAX_TRANSACTION_HISTORY_LENGTH
) {
transaction.history = transaction.history.slice(
0,
MAX_TRANSACTION_HISTORY_LENGTH,
);
}
}
}

/**
* Check whether the given object has a valid `history` property, or no `history`
* property. We just check that it's an array, we don't validate the contents.
*
* @param transaction - The object to validate.
* @returns True if the given object was valid, false otherwise.
*/
function hasValidTransactionHistory(
transaction: RuntimeObject,
): transaction is RuntimeObject & {
history: undefined | unknown[];
} {
return (
!hasProperty(transaction, 'history') || Array.isArray(transaction.history)
);
}
1 change: 1 addition & 0 deletions app/scripts/migrations/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ const migrations = [
require('./120'),
require('./120.1'),
require('./120.2'),
require('./120.3'),
];

export default migrations;
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@
"@trezor/schema-utils@npm:1.0.2": "patch:@trezor/schema-utils@npm%3A1.0.2#~/.yarn/patches/@trezor-schema-utils-npm-1.0.2-7dd48689b2.patch",
"lavamoat-core@npm:^15.1.1": "patch:lavamoat-core@npm%3A15.1.1#~/.yarn/patches/lavamoat-core-npm-15.1.1-51fbe39988.patch",
"@metamask/snaps-sdk": "^5.0.0",
"@metamask/transaction-controller": "^32.0.0",
"@metamask/transaction-controller": "patch:@metamask/transaction-controller@npm%3A32.0.0#~/.yarn/patches/@metamask-transaction-controller-npm-32.0.0-e23c2c3443.patch",
"@babel/runtime@npm:^7.7.6": "patch:@babel/runtime@npm%3A7.24.0#~/.yarn/patches/@babel-runtime-npm-7.24.0-7eb1dd11a2.patch",
"@babel/runtime@npm:^7.9.2": "patch:@babel/runtime@npm%3A7.24.0#~/.yarn/patches/@babel-runtime-npm-7.24.0-7eb1dd11a2.patch",
"@babel/runtime@npm:^7.12.5": "patch:@babel/runtime@npm%3A7.24.0#~/.yarn/patches/@babel-runtime-npm-7.24.0-7eb1dd11a2.patch",
Expand Down Expand Up @@ -350,7 +350,7 @@
"@metamask/snaps-rpc-methods": "^9.1.3",
"@metamask/snaps-sdk": "^5.0.0",
"@metamask/snaps-utils": "patch:@metamask/snaps-utils@npm%3A7.6.0#~/.yarn/patches/@metamask-snaps-utils-npm-7.6.0-5569b09766.patch",
"@metamask/transaction-controller": "^32.0.0",
"@metamask/transaction-controller": "patch:@metamask/transaction-controller@npm%3A32.0.0#~/.yarn/patches/@metamask-transaction-controller-npm-32.0.0-e23c2c3443.patch",
"@metamask/user-operation-controller": "^10.0.0",
"@metamask/utils": "^8.2.1",
"@ngraveio/bc-ur": "^1.1.12",
Expand Down
Loading

0 comments on commit 014b56c

Please sign in to comment.