From 72d20c92d69fc9ad13306d1cc232096dba0f8f5f Mon Sep 17 00:00:00 2001 From: Dan Miller Date: Wed, 16 Aug 2023 14:29:17 -0230 Subject: [PATCH 01/23] Version v10.34.5 --- CHANGELOG.md | 5 ++++- package.json | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 583701a69..b7f1393ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [10.34.5] + ## [10.34.4] ### Changed - Updated snaps execution environment ([#20420](https://github.com/MetaMask/metamask-extension/pull/20420)) @@ -3885,7 +3887,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Uncategorized - Added the ability to restore accounts from seed words. -[Unreleased]: https://github.com/MetaMask/metamask-extension/compare/v10.34.4...HEAD +[Unreleased]: https://github.com/MetaMask/metamask-extension/compare/v10.34.5...HEAD +[10.34.5]: https://github.com/MetaMask/metamask-extension/compare/v10.34.4...v10.34.5 [10.34.4]: https://github.com/MetaMask/metamask-extension/compare/v10.34.3...v10.34.4 [10.34.3]: https://github.com/MetaMask/metamask-extension/compare/v10.34.2...v10.34.3 [10.34.2]: https://github.com/MetaMask/metamask-extension/compare/v10.34.1...v10.34.2 diff --git a/package.json b/package.json index 297c63612..b1790a1e9 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "metamask-crx", - "version": "10.34.4", + "version": "10.34.5", "private": true, "repository": { "type": "git", From 442181de949a474b20292a93b09a5ea56690438d Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Tue, 15 Aug 2023 19:19:48 -0230 Subject: [PATCH 02/23] Log before and after each migration run (#20424) * Log before and after each migration run * Use loglevel --- app/scripts/lib/migrator/index.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/scripts/lib/migrator/index.js b/app/scripts/lib/migrator/index.js index ca2abb8ea..5925430f0 100644 --- a/app/scripts/lib/migrator/index.js +++ b/app/scripts/lib/migrator/index.js @@ -1,4 +1,5 @@ import EventEmitter from 'events'; +import log from 'loglevel'; /** * @typedef {object} Migration @@ -36,6 +37,8 @@ export default class Migrator extends EventEmitter { // perform each migration for (const migration of pendingMigrations) { try { + log.info(`Running migration ${migration.version}...`); + // attempt migration and validate const migratedData = await migration.migrate(versionedData); if (!migratedData.data) { @@ -52,6 +55,8 @@ export default class Migrator extends EventEmitter { // accept the migration as good // eslint-disable-next-line no-param-reassign versionedData = migratedData; + + log.info(`Migration ${migration.version} complete`); } catch (err) { // rewrite error message to add context without clobbering stack const originalErrorMessage = err.message; From 594dde58b15315c146f53f1a90b3c3b616868d1e Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Wed, 16 Aug 2023 11:52:25 -0230 Subject: [PATCH 03/23] Add additional Sentry E2E tests (#20425) * Reorganize Sentry error e2e tests The tests have been reorganized into different describe blocks. Each describe block is for either before or after initialization, and either with or without opting into metrics. This was done to simplify later test additions. The conditions for each test are now in the describe block, letting us test additional things in each of these conditions. The conditions were flattened to a single level to avoid excessive indentation. * Add error e2e test for background and UI errors The Sentry e2e tests before initialization only tested background errors, and the after initialization tests only tested UI errors. Now both types of errors are tested in both scenarios. * Add error e2e tests for Sentry error state E2E tests have been added to test the state object sent along with each Sentry error. At the moment this object is empty in some circumstances, but this will change in later PRs. * Rename throw test error function * Only setup debug/test state hooks in dev/test builds The state hooks used for debugging and testing are now only included in dev or test builds. The function name was updated and given a JSDoc description to explain this more clearly as well. * Add state snapshot assertions State snapshot assertions have been added to the e2e error tests. These snapshots will be very useful in reviewing a few PRs that will follow this one. We might decide to remove these snapshots after this set of Sentry refactors, as they might be more work to maintain than they're worth. But they will be useful at least in the short-term. The login step has been removed from a few tests because it introduced indeterminacy (the login process continued asynchronously after the login, and sometimes was not finished when the error was triggered). * Ensure login page has rendered during setup This fixes an intermittent failure on Firefox * Format snapshots with prettier before writing them * Use defined set of date fields rather than infering from name * Remove waits for error screen The error screen only appears after a long timeout, and it doesn't affect the next test steps at all. --- app/scripts/metamask-controller.js | 18 + test/e2e/tests/errors.spec.js | 466 ++++++++++++++++-- ...rs-after-init-opt-in-background-state.json | 50 ++ .../errors-after-init-opt-in-ui-state.json | 57 +++ ...s-before-init-opt-in-background-state.json | 1 + .../errors-before-init-opt-in-ui-state.json | 1 + ui/index.js | 45 +- ui/store/actions.ts | 11 + 8 files changed, 610 insertions(+), 39 deletions(-) create mode 100644 test/e2e/tests/state-snapshots/errors-after-init-opt-in-background-state.json create mode 100644 test/e2e/tests/state-snapshots/errors-after-init-opt-in-ui-state.json create mode 100644 test/e2e/tests/state-snapshots/errors-before-init-opt-in-background-state.json create mode 100644 test/e2e/tests/state-snapshots/errors-before-init-opt-in-ui-state.json diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 7c40d960a..364f8ee8b 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -2633,6 +2633,9 @@ export default class MetamaskController extends EventEmitter { assetsContractController.getBalancesInSingleCall.bind( assetsContractController, ), + + // E2E testing + throwTestError: this.throwTestError.bind(this), }; } @@ -4340,6 +4343,21 @@ export default class MetamaskController extends EventEmitter { return nonceLock.nextNonce; } + /** + * Throw an artificial error in a timeout handler for testing purposes. + * + * @param message - The error message. + * @deprecated This is only mean to facilitiate E2E testing. We should not + * use this for handling errors. + */ + throwTestError(message) { + setTimeout(() => { + const error = new Error(message); + error.name = 'TestError'; + throw error; + }); + } + //============================================================================= // CONFIG //============================================================================= diff --git a/test/e2e/tests/errors.spec.js b/test/e2e/tests/errors.spec.js index 960135215..4ab80a7de 100644 --- a/test/e2e/tests/errors.spec.js +++ b/test/e2e/tests/errors.spec.js @@ -1,8 +1,70 @@ +const { resolve } = require('path'); +const { promises: fs } = require('fs'); const { strict: assert } = require('assert'); +const { get, has, set } = require('lodash'); const { Browser } = require('selenium-webdriver'); +const { format } = require('prettier'); const { convertToHexValue, withFixtures } = require('../helpers'); const FixtureBuilder = require('../fixture-builder'); +const dateFields = ['metamask.conversionDate']; + +/** + * Transform date properties to value types, to ensure that state is + * consistent between test runs. + * + * @param {unknown} data - The data to transform + */ +function transformDates(data) { + for (const field of dateFields) { + if (has(data, field)) { + set(data, field, typeof get(data, field)); + } + } + return data; +} + +/** + * Check that the data provided matches the snapshot. + * + * @param {object }args - Function arguments. + * @param {any} args.data - The data to compare with the snapshot. + * @param {string} args.snapshot - The name of the snapshot. + * @param {boolean} [args.update] - Whether to update the snapshot if it doesn't match. + */ +async function matchesSnapshot({ + data: unprocessedData, + snapshot, + update = process.env.UPDATE_SNAPSHOTS === 'true', +}) { + const data = transformDates(unprocessedData); + + const snapshotPath = resolve(__dirname, `./state-snapshots/${snapshot}.json`); + const rawSnapshotData = await fs.readFile(snapshotPath, { + encoding: 'utf-8', + }); + const snapshotData = JSON.parse(rawSnapshotData); + + try { + assert.deepStrictEqual(data, snapshotData); + } catch (error) { + if (update && error instanceof assert.AssertionError) { + const stringifiedData = JSON.stringify(data); + // filepath specified so that Prettier can infer which parser to use + // from the file extension + const formattedData = format(stringifiedData, { + filepath: 'something.json', + }); + await fs.writeFile(snapshotPath, formattedData, { + encoding: 'utf-8', + }); + console.log(`Snapshot '${snapshot}' updated`); + return; + } + throw error; + } +} + describe('Sentry errors', function () { const migrationError = process.env.SELENIUM_BROWSER === Browser.CHROME @@ -41,8 +103,8 @@ describe('Sentry errors', function () { ], }; - describe('before initialization', function () { - it('should NOT send error events when participateInMetaMetrics is false', async function () { + describe('before initialization, after opting out of metrics', function () { + it('should NOT send error events in the background', async function () { await withFixtures( { fixtures: { @@ -73,7 +135,43 @@ describe('Sentry errors', function () { }, ); }); - it('should send error events', async function () { + + it('should NOT send error events in the UI', async function () { + await withFixtures( + { + fixtures: new FixtureBuilder() + .withMetaMetricsController({ + metaMetricsId: null, + participateInMetaMetrics: false, + }) + .build(), + ganacheOptions, + title: this.test.title, + failOnConsoleError: false, + testSpecificMock: mockSentryTestError, + }, + async ({ driver, mockedEndpoint }) => { + await driver.navigate(); + await driver.findElement('#password'); + // Erase `getSentryState` hook, simulating a "before initialization" state + await driver.executeScript( + 'window.stateHooks.getSentryState = undefined', + ); + + // Wait for Sentry request + await driver.delay(3000); + const isPending = await mockedEndpoint.isPending(); + assert.ok( + isPending, + 'A request to sentry was sent when it should not have been', + ); + }, + ); + }); + }); + + describe('before initialization, after opting into metrics', function () { + it('should send error events in background', async function () { await withFixtures( { fixtures: { @@ -112,40 +210,47 @@ describe('Sentry errors', function () { }, ); }); - }); - describe('after initialization', function () { - it('should NOT send error events when participateInMetaMetrics is false', async function () { + it('should capture background application state', async function () { await withFixtures( { - fixtures: new FixtureBuilder() - .withMetaMetricsController({ - metaMetricsId: null, - participateInMetaMetrics: false, - }) - .build(), + fixtures: { + ...new FixtureBuilder() + .withMetaMetricsController({ + metaMetricsId: 'fake-metrics-id', + participateInMetaMetrics: true, + }) + .build(), + // Intentionally corrupt state to trigger migration error during initialization + meta: undefined, + }, ganacheOptions, title: this.test.title, failOnConsoleError: false, - testSpecificMock: mockSentryTestError, + testSpecificMock: mockSentryMigratorError, }, async ({ driver, mockedEndpoint }) => { await driver.navigate(); - await driver.fill('#password', 'correct horse battery staple'); - await driver.press('#password', driver.Key.ENTER); - // Trigger error - driver.executeScript('window.stateHooks.throwTestError()'); - driver.delay(3000); + // Wait for Sentry request - const isPending = await mockedEndpoint.isPending(); - assert.ok( - isPending, - 'A request to sentry was sent when it should not have been', - ); + await driver.wait(async () => { + const isPending = await mockedEndpoint.isPending(); + return isPending === false; + }, 3000); + + const [mockedRequest] = await mockedEndpoint.getSeenRequests(); + const mockTextBody = mockedRequest.body.text.split('\n'); + const mockJsonBody = JSON.parse(mockTextBody[2]); + const appState = mockJsonBody?.extra?.appState; + await matchesSnapshot({ + data: appState, + snapshot: 'errors-before-init-opt-in-background-state', + }); }, ); }); - it('should send error events', async function () { + + it('should send error events in UI', async function () { await withFixtures( { fixtures: new FixtureBuilder() @@ -161,15 +266,171 @@ describe('Sentry errors', function () { }, async ({ driver, mockedEndpoint }) => { await driver.navigate(); - await driver.fill('#password', 'correct horse battery staple'); - await driver.press('#password', driver.Key.ENTER); + await driver.findElement('#password'); + // Erase `getSentryState` hook, simulating a "before initialization" state + await driver.executeScript( + 'window.stateHooks.getSentryState = undefined', + ); + // Trigger error - driver.executeScript('window.stateHooks.throwTestError()'); + await driver.executeScript('window.stateHooks.throwTestError()'); + // Wait for Sentry request await driver.wait(async () => { const isPending = await mockedEndpoint.isPending(); return isPending === false; - }, 10000); + }, 3000); + const [mockedRequest] = await mockedEndpoint.getSeenRequests(); + const mockTextBody = mockedRequest.body.text.split('\n'); + const mockJsonBody = JSON.parse(mockTextBody[2]); + const { level } = mockJsonBody; + const [{ type, value }] = mockJsonBody.exception.values; + // Verify request + assert.equal(type, 'TestError'); + assert.equal(value, 'Test Error'); + assert.equal(level, 'error'); + }, + ); + }); + + it('should capture UI application state', async function () { + await withFixtures( + { + fixtures: new FixtureBuilder() + .withMetaMetricsController({ + metaMetricsId: 'fake-metrics-id', + participateInMetaMetrics: true, + }) + .build(), + ganacheOptions, + title: this.test.title, + failOnConsoleError: false, + testSpecificMock: mockSentryTestError, + }, + async ({ driver, mockedEndpoint }) => { + await driver.navigate(); + await driver.findElement('#password'); + // Erase `getSentryState` hook, simulating a "before initialization" state + await driver.executeScript( + 'window.stateHooks.getSentryState = undefined', + ); + + // Trigger error + await driver.executeScript('window.stateHooks.throwTestError()'); + + // Wait for Sentry request + await driver.wait(async () => { + const isPending = await mockedEndpoint.isPending(); + return isPending === false; + }, 3000); + const [mockedRequest] = await mockedEndpoint.getSeenRequests(); + const mockTextBody = mockedRequest.body.text.split('\n'); + const mockJsonBody = JSON.parse(mockTextBody[2]); + const appState = mockJsonBody?.extra?.appState; + await matchesSnapshot({ + data: appState, + snapshot: 'errors-before-init-opt-in-ui-state', + }); + }, + ); + }); + }); + + describe('after initialization, after opting out of metrics', function () { + it('should NOT send error events in the background', async function () { + await withFixtures( + { + fixtures: new FixtureBuilder() + .withMetaMetricsController({ + metaMetricsId: null, + participateInMetaMetrics: false, + }) + .build(), + ganacheOptions, + title: this.test.title, + failOnConsoleError: false, + testSpecificMock: mockSentryTestError, + }, + async ({ driver, mockedEndpoint }) => { + await driver.navigate(); + await driver.findElement('#password'); + + // Trigger error + await driver.executeScript( + 'window.stateHooks.throwTestBackgroundError()', + ); + + // Wait for Sentry request + const isPending = await mockedEndpoint.isPending(); + assert.ok( + isPending, + 'A request to sentry was sent when it should not have been', + ); + }, + ); + }); + + it('should NOT send error events in the UI', async function () { + await withFixtures( + { + fixtures: new FixtureBuilder() + .withMetaMetricsController({ + metaMetricsId: null, + participateInMetaMetrics: false, + }) + .build(), + ganacheOptions, + title: this.test.title, + failOnConsoleError: false, + testSpecificMock: mockSentryTestError, + }, + async ({ driver, mockedEndpoint }) => { + await driver.navigate(); + await driver.findElement('#password'); + + // Trigger error + await driver.executeScript('window.stateHooks.throwTestError()'); + + // Wait for Sentry request + const isPending = await mockedEndpoint.isPending(); + assert.ok( + isPending, + 'A request to sentry was sent when it should not have been', + ); + }, + ); + }); + }); + + describe('after initialization, after opting into metrics', function () { + it('should send error events in background', async function () { + await withFixtures( + { + fixtures: new FixtureBuilder() + .withMetaMetricsController({ + metaMetricsId: 'fake-metrics-id', + participateInMetaMetrics: true, + }) + .build(), + ganacheOptions, + title: this.test.title, + failOnConsoleError: false, + testSpecificMock: mockSentryTestError, + }, + async ({ driver, mockedEndpoint }) => { + await driver.navigate(); + await driver.findElement('#password'); + + // Trigger error + await driver.executeScript( + 'window.stateHooks.throwTestBackgroundError()', + ); + + // Wait for Sentry request + await driver.wait(async () => { + const isPending = await mockedEndpoint.isPending(); + return isPending === false; + }, 3000); const [mockedRequest] = await mockedEndpoint.getSeenRequests(); const mockTextBody = mockedRequest.body.text.split('\n'); const mockJsonBody = JSON.parse(mockTextBody[2]); @@ -184,5 +445,154 @@ describe('Sentry errors', function () { }, ); }); + + it('should capture background application state', async function () { + await withFixtures( + { + fixtures: new FixtureBuilder() + .withMetaMetricsController({ + metaMetricsId: 'fake-metrics-id', + participateInMetaMetrics: true, + }) + .build(), + ganacheOptions, + title: this.test.title, + failOnConsoleError: false, + testSpecificMock: mockSentryTestError, + }, + async ({ driver, mockedEndpoint }) => { + await driver.navigate(); + await driver.findElement('#password'); + + // Trigger error + await driver.executeScript( + 'window.stateHooks.throwTestBackgroundError()', + ); + + // Wait for Sentry request + await driver.wait(async () => { + const isPending = await mockedEndpoint.isPending(); + return isPending === false; + }, 3000); + const [mockedRequest] = await mockedEndpoint.getSeenRequests(); + const mockTextBody = mockedRequest.body.text.split('\n'); + const mockJsonBody = JSON.parse(mockTextBody[2]); + const appState = mockJsonBody?.extra?.appState; + assert.deepStrictEqual(Object.keys(appState), [ + 'browser', + 'store', + 'version', + ]); + assert.ok( + typeof appState?.browser === 'string' && + appState?.browser.length > 0, + 'Invalid browser state', + ); + assert.ok( + typeof appState?.version === 'string' && + appState?.version.length > 0, + 'Invalid version state', + ); + await matchesSnapshot({ + data: appState.store, + snapshot: 'errors-after-init-opt-in-background-state', + }); + }, + ); + }); + + it('should send error events in UI', async function () { + await withFixtures( + { + fixtures: new FixtureBuilder() + .withMetaMetricsController({ + metaMetricsId: 'fake-metrics-id', + participateInMetaMetrics: true, + }) + .build(), + ganacheOptions, + title: this.test.title, + failOnConsoleError: false, + testSpecificMock: mockSentryTestError, + }, + async ({ driver, mockedEndpoint }) => { + await driver.navigate(); + await driver.findElement('#password'); + + // Trigger error + await driver.executeScript('window.stateHooks.throwTestError()'); + + // Wait for Sentry request + await driver.wait(async () => { + const isPending = await mockedEndpoint.isPending(); + return isPending === false; + }, 3000); + const [mockedRequest] = await mockedEndpoint.getSeenRequests(); + const mockTextBody = mockedRequest.body.text.split('\n'); + const mockJsonBody = JSON.parse(mockTextBody[2]); + const { level, extra } = mockJsonBody; + const [{ type, value }] = mockJsonBody.exception.values; + const { participateInMetaMetrics } = extra.appState.store.metamask; + // Verify request + assert.equal(type, 'TestError'); + assert.equal(value, 'Test Error'); + assert.equal(level, 'error'); + assert.equal(participateInMetaMetrics, true); + }, + ); + }); + + it('should capture UI application state', async function () { + await withFixtures( + { + fixtures: new FixtureBuilder() + .withMetaMetricsController({ + metaMetricsId: 'fake-metrics-id', + participateInMetaMetrics: true, + }) + .build(), + ganacheOptions, + title: this.test.title, + failOnConsoleError: false, + testSpecificMock: mockSentryTestError, + }, + async ({ driver, mockedEndpoint }) => { + await driver.navigate(); + await driver.findElement('#password'); + + // Trigger error + await driver.executeScript('window.stateHooks.throwTestError()'); + + // Wait for Sentry request + await driver.wait(async () => { + const isPending = await mockedEndpoint.isPending(); + return isPending === false; + }, 3000); + const [mockedRequest] = await mockedEndpoint.getSeenRequests(); + const mockTextBody = mockedRequest.body.text.split('\n'); + const mockJsonBody = JSON.parse(mockTextBody[2]); + const appState = mockJsonBody?.extra?.appState; + assert.deepStrictEqual(Object.keys(appState), [ + 'browser', + 'store', + 'version', + ]); + assert.ok( + typeof appState?.browser === 'string' && + appState?.browser.length > 0, + 'Invalid browser state', + ); + assert.ok( + typeof appState?.version === 'string' && + appState?.version.length > 0, + 'Invalid version state', + ); + await matchesSnapshot({ + data: appState.store, + snapshot: 'errors-after-init-opt-in-ui-state', + }); + }, + ); + }); }); }); diff --git a/test/e2e/tests/state-snapshots/errors-after-init-opt-in-background-state.json b/test/e2e/tests/state-snapshots/errors-after-init-opt-in-background-state.json new file mode 100644 index 000000000..efdf28622 --- /dev/null +++ b/test/e2e/tests/state-snapshots/errors-after-init-opt-in-background-state.json @@ -0,0 +1,50 @@ +{ + "metamask": { + "isInitialized": true, + "connectedStatusPopoverHasBeenShown": true, + "defaultHomeActiveTabName": null, + "networkId": "1337", + "providerConfig": { + "nickname": "Localhost 8545", + "ticker": "ETH", + "type": "rpc" + }, + "isUnlocked": false, + "useBlockie": false, + "useNonceField": false, + "usePhishDetect": true, + "featureFlags": { "showIncomingTransactions": true }, + "currentLocale": "en", + "forgottenPassword": false, + "preferences": { + "hideZeroBalanceTokens": false, + "showFiatInTestnets": false, + "showTestNetworks": false, + "useNativeCurrencyAsPrimaryCurrency": true + }, + "ipfsGateway": "dweb.link", + "participateInMetaMetrics": true, + "metaMetricsId": "fake-metrics-id", + "conversionDate": "number", + "conversionRate": 1700, + "nativeCurrency": "ETH", + "currentCurrency": "usd", + "alertEnabledness": { "unconnectedAccount": true, "web3ShimUsage": true }, + "seedPhraseBackedUp": true, + "firstTimeFlowType": "import", + "completedOnboarding": true, + "incomingTxLastFetchedBlockByChainId": { + "0x1": null, + "0xe708": null, + "0x5": null, + "0xaa36a7": null, + "0xe704": null + }, + "currentBlockGasLimit": "0x1c9c380", + "unapprovedDecryptMsgCount": 0, + "unapprovedEncryptionPublicKeyMsgCount": 0, + "unapprovedMsgCount": 0, + "unapprovedPersonalMsgCount": 0, + "unapprovedTypedMessagesCount": 0 + } +} diff --git a/test/e2e/tests/state-snapshots/errors-after-init-opt-in-ui-state.json b/test/e2e/tests/state-snapshots/errors-after-init-opt-in-ui-state.json new file mode 100644 index 000000000..7300c3a05 --- /dev/null +++ b/test/e2e/tests/state-snapshots/errors-after-init-opt-in-ui-state.json @@ -0,0 +1,57 @@ +{ + "gas": { "customData": { "price": null, "limit": null } }, + "history": { "mostRecentOverviewPage": "/" }, + "metamask": { + "isInitialized": true, + "isUnlocked": false, + "isAccountMenuOpen": false, + "customNonceValue": "", + "useBlockie": false, + "featureFlags": { "showIncomingTransactions": true }, + "welcomeScreenSeen": false, + "currentLocale": "en", + "currentBlockGasLimit": "", + "preferences": { + "hideZeroBalanceTokens": false, + "showFiatInTestnets": false, + "showTestNetworks": false, + "useNativeCurrencyAsPrimaryCurrency": true + }, + "firstTimeFlowType": "import", + "completedOnboarding": true, + "participateInMetaMetrics": true, + "nextNonce": null, + "conversionRate": 1300, + "nativeCurrency": "ETH", + "connectedStatusPopoverHasBeenShown": true, + "defaultHomeActiveTabName": null, + "networkId": "1337", + "providerConfig": { + "nickname": "Localhost 8545", + "ticker": "ETH", + "type": "rpc" + }, + "useNonceField": false, + "usePhishDetect": true, + "forgottenPassword": false, + "ipfsGateway": "dweb.link", + "metaMetricsId": "fake-metrics-id", + "conversionDate": "number", + "currentCurrency": "usd", + "alertEnabledness": { "unconnectedAccount": true, "web3ShimUsage": true }, + "seedPhraseBackedUp": true, + "incomingTxLastFetchedBlockByChainId": { + "0x1": null, + "0xe708": null, + "0x5": null, + "0xaa36a7": null, + "0xe704": null + }, + "unapprovedDecryptMsgCount": 0, + "unapprovedEncryptionPublicKeyMsgCount": 0, + "unapprovedMsgCount": 0, + "unapprovedPersonalMsgCount": 0, + "unapprovedTypedMessagesCount": 0 + }, + "unconnectedAccount": { "state": "CLOSED" } +} diff --git a/test/e2e/tests/state-snapshots/errors-before-init-opt-in-background-state.json b/test/e2e/tests/state-snapshots/errors-before-init-opt-in-background-state.json new file mode 100644 index 000000000..0967ef424 --- /dev/null +++ b/test/e2e/tests/state-snapshots/errors-before-init-opt-in-background-state.json @@ -0,0 +1 @@ +{} diff --git a/test/e2e/tests/state-snapshots/errors-before-init-opt-in-ui-state.json b/test/e2e/tests/state-snapshots/errors-before-init-opt-in-ui-state.json new file mode 100644 index 000000000..0967ef424 --- /dev/null +++ b/test/e2e/tests/state-snapshots/errors-before-init-opt-in-ui-state.json @@ -0,0 +1 @@ +{} diff --git a/ui/index.js b/ui/index.js index 1f1aeb281..e9ac462bd 100644 --- a/ui/index.js +++ b/ui/index.js @@ -81,7 +81,7 @@ export default function launchMetamaskUi(opts, cb) { return; } startApp(metamaskState, backgroundConnection, opts).then((store) => { - setupDebuggingHelpers(store); + setupStateHooks(store); cb( null, store, @@ -191,16 +191,39 @@ async function startApp(metamaskState, backgroundConnection, opts) { return store; } -function setupDebuggingHelpers(store) { - /** - * The following stateHook is a method intended to throw an error, used in - * our E2E test to ensure that errors are attempted to be sent to sentry. - */ - window.stateHooks.throwTestError = async function () { - const error = new Error('Test Error'); - error.name = 'TestError'; - throw error; - }; +/** + * Setup functions on `window.stateHooks`. Some of these support + * application features, and some are just for debugging or testing. + * + * @param {object} store - The Redux store. + */ +function setupStateHooks(store) { + if (process.env.METAMASK_DEBUG || process.env.IN_TEST) { + /** + * The following stateHook is a method intended to throw an error, used in + * our E2E test to ensure that errors are attempted to be sent to sentry. + * + * @param {string} [msg] - The error message to throw, defaults to 'Test Error' + */ + window.stateHooks.throwTestError = async function (msg = 'Test Error') { + const error = new Error(msg); + error.name = 'TestError'; + throw error; + }; + /** + * The following stateHook is a method intended to throw an error in the + * background, used in our E2E test to ensure that errors are attempted to be + * sent to sentry. + * + * @param {string} [msg] - The error message to throw, defaults to 'Test Error' + */ + window.stateHooks.throwTestBackgroundError = async function ( + msg = 'Test Error', + ) { + store.dispatch(actions.throwTestBackgroundError(msg)); + }; + } + window.stateHooks.getCleanAppState = async function () { const state = clone(store.getState()); state.version = global.platform.getVersion(); diff --git a/ui/store/actions.ts b/ui/store/actions.ts index 509127bf6..16cb68ef0 100644 --- a/ui/store/actions.ts +++ b/ui/store/actions.ts @@ -4378,6 +4378,17 @@ export async function getCurrentNetworkEIP1559Compatibility(): Promise< return networkEIP1559Compatibility; } +/** + * Throw an error in the background for testing purposes. + * + * @param message - The error message. + * @deprecated This is only mean to facilitiate E2E testing. We should not use + * this for handling errors. + */ +export async function throwTestBackgroundError(message: string): Promise { + await submitRequestToBackground('throwTestError', [message]); +} + ///: BEGIN:ONLY_INCLUDE_IN(snaps) /** * Set status of popover warning for the first snap installation. From 789122d5878bec8b6db7abf2e5a7b87057950efc Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Wed, 16 Aug 2023 12:22:38 -0230 Subject: [PATCH 04/23] Capture app and migration version (#20458) * Add AppMetadataController so current and previous application and migration version can be captured in sentry * Add currentAppVersion, previousAppVersion, previousMigrationVersion, currentMigrationVersion to SENTRY_OBJECT * Update app/scripts/controllers/app-metadata.ts Co-authored-by: Mark Stacey * Update app/scripts/controllers/app-metadata.ts Co-authored-by: Mark Stacey * Update app/scripts/controllers/app-metadata.ts Co-authored-by: Mark Stacey * Fix types * Add tests for app-metadata.test.ts * Lint fixes * Modify loadStateFromPersistence to return the whole versionData object, so that the migration version can be passed to the metamask-controller on instantiation * Remove reference to implementation details in test descriptions in app/scripts/controllers/app-metadata.test.ts * Reset all mocks afterEach in AppMetadataController * Refactor AppMetadataController to be passed version instead of calling platform.version directly (for ease of unit testing the MetaMask Controller) * Make maybeUpdateAppVersion and maybeUpdateMigrationVersion private, and remove unit tests of those specific functions --------- Co-authored-by: Mark Stacey --- app/scripts/background.js | 9 +- app/scripts/controllers/app-metadata.test.ts | 104 +++++++++++++++++++ app/scripts/controllers/app-metadata.ts | 99 ++++++++++++++++++ app/scripts/lib/setupSentry.js | 4 + app/scripts/metamask-controller.js | 11 ++ 5 files changed, 225 insertions(+), 2 deletions(-) create mode 100644 app/scripts/controllers/app-metadata.test.ts create mode 100644 app/scripts/controllers/app-metadata.ts diff --git a/app/scripts/background.js b/app/scripts/background.js index 44d3870ad..4ce5419c5 100644 --- a/app/scripts/background.js +++ b/app/scripts/background.js @@ -264,7 +264,8 @@ browser.runtime.onConnectExternal.addListener(async (...args) => { */ async function initialize() { try { - const initState = await loadStateFromPersistence(); + const initData = await loadStateFromPersistence(); + const initState = initData.data; const initLangCode = await getFirstPreferredLangCode(); ///: BEGIN:ONLY_INCLUDE_IN(desktop) @@ -287,6 +288,7 @@ async function initialize() { initLangCode, {}, isFirstMetaMaskControllerSetup, + initData.meta, ); if (!isManifestV3) { await loadPhishingWarningPage(); @@ -417,7 +419,7 @@ export async function loadStateFromPersistence() { localStore.set(versionedData.data); // return just the data - return versionedData.data; + return versionedData; } /** @@ -430,12 +432,14 @@ export async function loadStateFromPersistence() { * @param {string} initLangCode - The region code for the language preferred by the current user. * @param {object} overrides - object with callbacks that are allowed to override the setup controller logic (usefull for desktop app) * @param isFirstMetaMaskControllerSetup + * @param {object} stateMetadata - Metadata about the initial state and migrations, including the most recent migration version */ export function setupController( initState, initLangCode, overrides, isFirstMetaMaskControllerSetup, + stateMetadata, ) { // // MetaMask Controller @@ -462,6 +466,7 @@ export function setupController( localStore, overrides, isFirstMetaMaskControllerSetup, + currentMigrationVersion: stateMetadata.version, }); setupEnsIpfsResolver({ diff --git a/app/scripts/controllers/app-metadata.test.ts b/app/scripts/controllers/app-metadata.test.ts new file mode 100644 index 000000000..890811ee2 --- /dev/null +++ b/app/scripts/controllers/app-metadata.test.ts @@ -0,0 +1,104 @@ +import assert from 'assert'; +import AppMetadataController from './app-metadata'; + +const EXPECTED_DEFAULT_STATE = { + currentAppVersion: '', + previousAppVersion: '', + previousMigrationVersion: 0, + currentMigrationVersion: 0, +}; + +describe('AppMetadataController', () => { + describe('constructor', () => { + it('accepts initial state and does not modify it if currentMigrationVersion and platform.getVersion() match respective values in state', async () => { + const initState = { + currentAppVersion: '1', + previousAppVersion: '1', + previousMigrationVersion: 1, + currentMigrationVersion: 1, + }; + const appMetadataController = new AppMetadataController({ + state: initState, + currentMigrationVersion: 1, + currentAppVersion: '1', + }); + assert.deepStrictEqual(appMetadataController.store.getState(), initState); + }); + + it('sets default state and does not modify it', async () => { + const appMetadataController = new AppMetadataController({ + state: {}, + }); + assert.deepStrictEqual( + appMetadataController.store.getState(), + EXPECTED_DEFAULT_STATE, + ); + }); + + it('sets default state and does not modify it if options version parameters match respective default values', async () => { + const appMetadataController = new AppMetadataController({ + state: {}, + currentMigrationVersion: 0, + currentAppVersion: '', + }); + assert.deepStrictEqual( + appMetadataController.store.getState(), + EXPECTED_DEFAULT_STATE, + ); + }); + + it('updates the currentAppVersion state property if options.currentAppVersion does not match the default value', async () => { + const appMetadataController = new AppMetadataController({ + state: {}, + currentMigrationVersion: 0, + currentAppVersion: '1', + }); + assert.deepStrictEqual(appMetadataController.store.getState(), { + ...EXPECTED_DEFAULT_STATE, + currentAppVersion: '1', + }); + }); + + it('updates the currentAppVersion and previousAppVersion state properties if options.currentAppVersion, currentAppVersion and previousAppVersion are all different', async () => { + const appMetadataController = new AppMetadataController({ + state: { + currentAppVersion: '2', + previousAppVersion: '1', + }, + currentAppVersion: '3', + currentMigrationVersion: 0, + }); + assert.deepStrictEqual(appMetadataController.store.getState(), { + ...EXPECTED_DEFAULT_STATE, + currentAppVersion: '3', + previousAppVersion: '2', + }); + }); + + it('updates the currentMigrationVersion state property if the currentMigrationVersion param does not match the default value', async () => { + const appMetadataController = new AppMetadataController({ + state: {}, + currentMigrationVersion: 1, + }); + assert.deepStrictEqual(appMetadataController.store.getState(), { + ...EXPECTED_DEFAULT_STATE, + currentMigrationVersion: 1, + }); + }); + + it('updates the currentMigrationVersion and previousMigrationVersion state properties if the currentMigrationVersion param, the currentMigrationVersion state property and the previousMigrationVersion state property are all different', async () => { + const appMetadataController = new AppMetadataController({ + state: { + currentMigrationVersion: 2, + previousMigrationVersion: 1, + }, + currentMigrationVersion: 3, + }); + assert.deepStrictEqual(appMetadataController.store.getState(), { + ...EXPECTED_DEFAULT_STATE, + currentMigrationVersion: 3, + previousMigrationVersion: 2, + }); + }); + }); +}); diff --git a/app/scripts/controllers/app-metadata.ts b/app/scripts/controllers/app-metadata.ts new file mode 100644 index 000000000..0d745730d --- /dev/null +++ b/app/scripts/controllers/app-metadata.ts @@ -0,0 +1,99 @@ +import EventEmitter from 'events'; +import { ObservableStore } from '@metamask/obs-store'; + +/** + * The state of the AppMetadataController + */ +export type AppMetadataControllerState = { + currentAppVersion: string; + previousAppVersion: string; + previousMigrationVersion: number; + currentMigrationVersion: number; +}; + +/** + * The options that NetworkController takes. + */ +export type AppMetadataControllerOptions = { + currentMigrationVersion?: number; + currentAppVersion?: string; + state?: Partial; +}; + +const defaultState: AppMetadataControllerState = { + currentAppVersion: '', + previousAppVersion: '', + previousMigrationVersion: 0, + currentMigrationVersion: 0, +}; + +/** + * The AppMetadata controller stores metadata about the current extension instance, + * including the currently and previously installed versions, and the most recently + * run migration. + * + */ +export default class AppMetadataController extends EventEmitter { + /** + * Observable store containing controller data. + */ + store: ObservableStore; + + /** + * Constructs a AppMetadata controller. + * + * @param options - the controller options + * @param options.state - Initial controller state. + * @param options.currentMigrationVersion + * @param options.currentAppVersion + */ + constructor({ + currentAppVersion = '', + currentMigrationVersion = 0, + state = {}, + }: AppMetadataControllerOptions) { + super(); + + this.store = new ObservableStore({ + ...defaultState, + ...state, + }); + + this.#maybeUpdateAppVersion(currentAppVersion); + + this.#maybeUpdateMigrationVersion(currentMigrationVersion); + } + + /** + * Updates the currentAppVersion in state, and sets the previousAppVersion to the old currentAppVersion. + * + * @param maybeNewAppVersion + */ + #maybeUpdateAppVersion(maybeNewAppVersion: string): void { + const oldCurrentAppVersion = this.store.getState().currentAppVersion; + + if (maybeNewAppVersion !== oldCurrentAppVersion) { + this.store.updateState({ + currentAppVersion: maybeNewAppVersion, + previousAppVersion: oldCurrentAppVersion, + }); + } + } + + /** + * Updates the migrationVersion in state. + * + * @param maybeNewMigrationVersion + */ + #maybeUpdateMigrationVersion(maybeNewMigrationVersion: number): void { + const oldCurrentMigrationVersion = + this.store.getState().currentMigrationVersion; + + if (maybeNewMigrationVersion !== oldCurrentMigrationVersion) { + this.store.updateState({ + previousMigrationVersion: oldCurrentMigrationVersion, + currentMigrationVersion: maybeNewMigrationVersion, + }); + } + } +} diff --git a/app/scripts/lib/setupSentry.js b/app/scripts/lib/setupSentry.js index dd0eb7bd6..967361315 100644 --- a/app/scripts/lib/setupSentry.js +++ b/app/scripts/lib/setupSentry.js @@ -35,9 +35,11 @@ export const SENTRY_STATE = { connectedStatusPopoverHasBeenShown: true, conversionDate: true, conversionRate: true, + currentAppVersion: true, currentBlockGasLimit: true, currentCurrency: true, currentLocale: true, + currentMigrationVersion: true, customNonceValue: true, defaultHomeActiveTabName: true, desktopEnabled: true, @@ -56,6 +58,8 @@ export const SENTRY_STATE = { nextNonce: true, participateInMetaMetrics: true, preferences: true, + previousAppVersion: true, + previousMigrationVersion: true, providerConfig: { nickname: true, ticker: true, diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 364f8ee8b..64bc9a26f 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -192,6 +192,7 @@ import createMetaRPCHandler from './lib/createMetaRPCHandler'; import { previousValueComparator } from './lib/util'; import createMetamaskMiddleware from './lib/createMetamaskMiddleware'; import EncryptionPublicKeyController from './controllers/encryption-public-key'; +import AppMetadataController from './controllers/app-metadata'; import { CaveatMutatorFactories, @@ -258,6 +259,8 @@ export default class MetamaskController extends EventEmitter { // instance of a class that wraps the extension's storage local API. this.localStoreApiWrapper = opts.localStore; + this.currentMigrationVersion = opts.currentMigrationVersion; + // observable state store this.store = new ComposableObservableStore({ state: initState, @@ -278,6 +281,12 @@ export default class MetamaskController extends EventEmitter { } }); + this.appMetadataController = new AppMetadataController({ + state: initState.AppMetadataController, + currentMigrationVersion: this.currentMigrationVersion, + currentAppVersion: version, + }); + // next, we will initialize the controllers // controller initialization order matters @@ -1506,6 +1515,7 @@ export default class MetamaskController extends EventEmitter { this.store.updateStructure({ AppStateController: this.appStateController.store, + AppMetadataController: this.appMetadataController.store, TransactionController: this.txController.store, KeyringController: this.keyringController.store, PreferencesController: this.preferencesController.store, @@ -1550,6 +1560,7 @@ export default class MetamaskController extends EventEmitter { this.memStore = new ComposableObservableStore({ config: { AppStateController: this.appStateController.store, + AppMetadataController: this.appMetadataController.store, NetworkController: this.networkController, CachedBalancesController: this.cachedBalancesController.store, KeyringController: this.keyringController.memStore, From 1ab0c9e160b40e0ae005cd00f39fe46cc052ca31 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Wed, 16 Aug 2023 13:34:12 -0230 Subject: [PATCH 05/23] Fix Sentry error e2e tests (#20479) The state fixtures in the Sentry e2e tests became invalid in #20458 due to a conflict with that change (the new state properties were missing). The state fixtures have been updated. --- .../errors-after-init-opt-in-background-state.json | 4 ++++ .../state-snapshots/errors-after-init-opt-in-ui-state.json | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/test/e2e/tests/state-snapshots/errors-after-init-opt-in-background-state.json b/test/e2e/tests/state-snapshots/errors-after-init-opt-in-background-state.json index efdf28622..03fc85edf 100644 --- a/test/e2e/tests/state-snapshots/errors-after-init-opt-in-background-state.json +++ b/test/e2e/tests/state-snapshots/errors-after-init-opt-in-background-state.json @@ -3,6 +3,10 @@ "isInitialized": true, "connectedStatusPopoverHasBeenShown": true, "defaultHomeActiveTabName": null, + "currentAppVersion": "10.34.4", + "previousAppVersion": "", + "previousMigrationVersion": 0, + "currentMigrationVersion": 94, "networkId": "1337", "providerConfig": { "nickname": "Localhost 8545", diff --git a/test/e2e/tests/state-snapshots/errors-after-init-opt-in-ui-state.json b/test/e2e/tests/state-snapshots/errors-after-init-opt-in-ui-state.json index 7300c3a05..4ecaa0417 100644 --- a/test/e2e/tests/state-snapshots/errors-after-init-opt-in-ui-state.json +++ b/test/e2e/tests/state-snapshots/errors-after-init-opt-in-ui-state.json @@ -25,6 +25,10 @@ "nativeCurrency": "ETH", "connectedStatusPopoverHasBeenShown": true, "defaultHomeActiveTabName": null, + "currentAppVersion": "10.34.4", + "previousAppVersion": "", + "previousMigrationVersion": 0, + "currentMigrationVersion": 94, "networkId": "1337", "providerConfig": { "nickname": "Localhost 8545", From e7b113caa25d49ac6116065dc442f06807065700 Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Mon, 14 Aug 2023 21:00:34 +0200 Subject: [PATCH 06/23] Bump SES to fix audit failure (#20434) * Bump SES to fix audit failure * Freeze Symbol --- app/scripts/lockdown-more.js | 2 +- package.json | 2 +- yarn.lock | 20 ++++++++++++++++++-- 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/app/scripts/lockdown-more.js b/app/scripts/lockdown-more.js index e6637602a..317052312 100644 --- a/app/scripts/lockdown-more.js +++ b/app/scripts/lockdown-more.js @@ -28,7 +28,7 @@ try { const namedIntrinsics = Reflect.ownKeys(new Compartment().globalThis); // These named intrinsics are not automatically hardened by `lockdown` - const shouldHardenManually = new Set(['eval', 'Function']); + const shouldHardenManually = new Set(['eval', 'Function', 'Symbol']); const globalProperties = new Set([ // universalPropertyNames is a constant added by lockdown to global scope diff --git a/package.json b/package.json index b1790a1e9..7e47069af 100644 --- a/package.json +++ b/package.json @@ -351,7 +351,7 @@ "redux-thunk": "^2.3.0", "remove-trailing-slash": "^0.1.1", "reselect": "^3.0.1", - "ses": "^0.18.4", + "ses": "^0.18.7", "single-call-balance-checker-abi": "^1.0.0", "unicode-confusables": "^0.1.1", "uuid": "^8.3.2", diff --git a/yarn.lock b/yarn.lock index 476f1f709..3a3e78fa4 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1884,6 +1884,13 @@ __metadata: languageName: node linkType: hard +"@endo/env-options@npm:^0.1.3": + version: 0.1.3 + resolution: "@endo/env-options@npm:0.1.3" + checksum: da8c66865d4d30b0053a00960657dc36f022975a888f0dd6a2f6bb37b9fe731f45a02a2cf263d93b1a40fcb37b25f8ba7076cb8af9e93fd95f496365d9382930 + languageName: node + linkType: hard + "@ensdomains/address-encoder@npm:^0.1.7": version: 0.1.9 resolution: "@ensdomains/address-encoder@npm:0.1.9" @@ -24758,7 +24765,7 @@ __metadata: selenium-webdriver: ^4.9.0 semver: ^7.3.5 serve-handler: ^6.1.2 - ses: ^0.18.4 + ses: ^0.18.7 single-call-balance-checker-abi: ^1.0.0 sinon: ^9.0.0 source-map: ^0.7.2 @@ -31342,13 +31349,22 @@ __metadata: languageName: node linkType: hard -"ses@npm:^0.18.1, ses@npm:^0.18.4": +"ses@npm:^0.18.1": version: 0.18.4 resolution: "ses@npm:0.18.4" checksum: 9afd6edcf390a693926ef728ebb5a435994bbb0f915009ad524c6588cf62e2f66f6d4b4b2694f093b2af2e92c003947ad55404750d756ba75ce70c8636a7ba02 languageName: node linkType: hard +"ses@npm:^0.18.7": + version: 0.18.7 + resolution: "ses@npm:0.18.7" + dependencies: + "@endo/env-options": ^0.1.3 + checksum: 75ac014771d9bc1f747193c6d0f9e7d2d7700a10311ba8d805d9bc78d4c20d4ef40537f0535b1ea6abf06babf67e70f8bd37b2ad68ad54992a0c5ce842181c87 + languageName: node + linkType: hard + "set-blocking@npm:^2.0.0, set-blocking@npm:~2.0.0": version: 2.0.0 resolution: "set-blocking@npm:2.0.0" From 83c8a6be994cf74d81c081648a9e0a4518818dbd Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Wed, 16 Aug 2023 08:33:18 -0230 Subject: [PATCH 07/23] Update `protobufjs` (#20469) Update `protobufjs` to the latest version. This resolves a security advisory for this package. The advisory is concerning prototype pollution, so it likely never affected us due to LavaMoat protections. --- yarn.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/yarn.lock b/yarn.lock index 3a3e78fa4..944abfeba 100644 --- a/yarn.lock +++ b/yarn.lock @@ -28543,8 +28543,8 @@ __metadata: linkType: hard "protobufjs@npm:^6.11.3": - version: 6.11.3 - resolution: "protobufjs@npm:6.11.3" + version: 6.11.4 + resolution: "protobufjs@npm:6.11.4" dependencies: "@protobufjs/aspromise": ^1.1.2 "@protobufjs/base64": ^1.1.2 @@ -28562,7 +28562,7 @@ __metadata: bin: pbjs: bin/pbjs pbts: bin/pbts - checksum: 4a6ce1964167e4c45c53fd8a312d7646415c777dd31b4ba346719947b88e61654912326101f927da387d6b6473ab52a7ea4f54d6f15d63b31130ce28e2e15070 + checksum: 6b7fd7540d74350d65c38f69f398c9995ae019da070e79d9cd464a458c6d19b40b07c9a026be4e10704c824a344b603307745863310c50026ebd661ce4da0663 languageName: node linkType: hard From d610aad2fd3300dfb1bdb07c061cfef4cf444eb0 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Wed, 16 Aug 2023 14:40:44 -0230 Subject: [PATCH 08/23] Split Sentry mask into UI and background masks (#20426) The state mask used to anonymize the Sentry state snapshots has been split into UI and background masks. This was done to simplify later refactors. There should be no functional changes. --- app/scripts/background.js | 11 ++-- app/scripts/lib/setupSentry.js | 101 +++++++++++++++++---------------- ui/index.js | 8 +-- 3 files changed, 64 insertions(+), 56 deletions(-) diff --git a/app/scripts/background.js b/app/scripts/background.js index 4ce5419c5..0c07adf4a 100644 --- a/app/scripts/background.js +++ b/app/scripts/background.js @@ -41,7 +41,7 @@ import Migrator from './lib/migrator'; import ExtensionPlatform from './platforms/extension'; import LocalStore from './lib/local-store'; import ReadOnlyNetworkStore from './lib/network-store'; -import { SENTRY_STATE } from './lib/setupSentry'; +import { SENTRY_BACKGROUND_STATE } from './lib/setupSentry'; import createStreamSink from './lib/createStreamSink'; import NotificationManager, { @@ -876,11 +876,14 @@ browser.runtime.onInstalled.addListener(({ reason }) => { function setupSentryGetStateGlobal(store) { global.stateHooks.getSentryState = function () { - const fullState = store.getState(); - const debugState = maskObject({ metamask: fullState }, SENTRY_STATE); + const backgroundState = store.getState(); + const maskedBackgroundState = maskObject( + backgroundState, + SENTRY_BACKGROUND_STATE, + ); return { browser: window.navigator.userAgent, - store: debugState, + store: { metamask: maskedBackgroundState }, version: platform.getVersion(), }; }; diff --git a/app/scripts/lib/setupSentry.js b/app/scripts/lib/setupSentry.js index 967361315..881f3c49e 100644 --- a/app/scripts/lib/setupSentry.js +++ b/app/scripts/lib/setupSentry.js @@ -23,59 +23,64 @@ export const ERROR_URL_ALLOWLIST = { SEGMENT: 'segment.io', }; +// This describes the subset of background controller state attached to errors +// sent to Sentry These properties have some potential to be useful for +// debugging, and they do not contain any identifiable information. +export const SENTRY_BACKGROUND_STATE = { + alertEnabledness: true, + completedOnboarding: true, + connectedStatusPopoverHasBeenShown: true, + conversionDate: true, + conversionRate: true, + currentAppVersion: true, + currentBlockGasLimit: true, + currentCurrency: true, + currentLocale: true, + currentMigrationVersion: true, + customNonceValue: true, + defaultHomeActiveTabName: true, + desktopEnabled: true, + featureFlags: true, + firstTimeFlowType: true, + forgottenPassword: true, + incomingTxLastFetchedBlockByChainId: true, + ipfsGateway: true, + isAccountMenuOpen: true, + isInitialized: true, + isUnlocked: true, + metaMetricsId: true, + nativeCurrency: true, + networkId: true, + networkStatus: true, + nextNonce: true, + participateInMetaMetrics: true, + preferences: true, + previousAppVersion: true, + previousMigrationVersion: true, + providerConfig: { + nickname: true, + ticker: true, + type: true, + }, + seedPhraseBackedUp: true, + unapprovedDecryptMsgCount: true, + unapprovedEncryptionPublicKeyMsgCount: true, + unapprovedMsgCount: true, + unapprovedPersonalMsgCount: true, + unapprovedTypedMessagesCount: true, + useBlockie: true, + useNonceField: true, + usePhishDetect: true, + welcomeScreenSeen: true, +}; + // This describes the subset of Redux state attached to errors sent to Sentry // These properties have some potential to be useful for debugging, and they do // not contain any identifiable information. -export const SENTRY_STATE = { +export const SENTRY_UI_STATE = { gas: true, history: true, - metamask: { - alertEnabledness: true, - completedOnboarding: true, - connectedStatusPopoverHasBeenShown: true, - conversionDate: true, - conversionRate: true, - currentAppVersion: true, - currentBlockGasLimit: true, - currentCurrency: true, - currentLocale: true, - currentMigrationVersion: true, - customNonceValue: true, - defaultHomeActiveTabName: true, - desktopEnabled: true, - featureFlags: true, - firstTimeFlowType: true, - forgottenPassword: true, - incomingTxLastFetchedBlockByChainId: true, - ipfsGateway: true, - isAccountMenuOpen: true, - isInitialized: true, - isUnlocked: true, - metaMetricsId: true, - nativeCurrency: true, - networkId: true, - networkStatus: true, - nextNonce: true, - participateInMetaMetrics: true, - preferences: true, - previousAppVersion: true, - previousMigrationVersion: true, - providerConfig: { - nickname: true, - ticker: true, - type: true, - }, - seedPhraseBackedUp: true, - unapprovedDecryptMsgCount: true, - unapprovedEncryptionPublicKeyMsgCount: true, - unapprovedMsgCount: true, - unapprovedPersonalMsgCount: true, - unapprovedTypedMessagesCount: true, - useBlockie: true, - useNonceField: true, - usePhishDetect: true, - welcomeScreenSeen: true, - }, + metamask: SENTRY_BACKGROUND_STATE, unconnectedAccount: true, }; diff --git a/ui/index.js b/ui/index.js index e9ac462bd..6f9a148ed 100644 --- a/ui/index.js +++ b/ui/index.js @@ -8,7 +8,7 @@ import browser from 'webextension-polyfill'; import { getEnvironmentType } from '../app/scripts/lib/util'; import { AlertTypes } from '../shared/constants/alerts'; import { maskObject } from '../shared/modules/object.utils'; -import { SENTRY_STATE } from '../app/scripts/lib/setupSentry'; +import { SENTRY_UI_STATE } from '../app/scripts/lib/setupSentry'; import { ENVIRONMENT_TYPE_POPUP } from '../shared/constants/app'; import switchDirection from '../shared/lib/switch-direction'; import { setupLocale } from '../shared/lib/error-utils'; @@ -234,11 +234,11 @@ function setupStateHooks(store) { return state; }; window.stateHooks.getSentryState = function () { - const fullState = store.getState(); - const debugState = maskObject(fullState, SENTRY_STATE); + const reduxState = store.getState(); + const maskedReduxState = maskObject(reduxState, SENTRY_UI_STATE); return { browser: window.navigator.userAgent, - store: debugState, + store: maskedReduxState, version: global.platform.getVersion(), }; }; From 1ad47c660bff8d856dc2806c5f01b04808afe47e Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Wed, 16 Aug 2023 15:21:18 -0230 Subject: [PATCH 09/23] Use unflattened state for Sentry (#20428) The unflattened background state is now attached to any Sentry errors from the background process. This provides a clearer picture of the state of the wallet, and unblocks further improvements to Sentry state which will come in later PRs. --- app/scripts/background.js | 4 +- app/scripts/lib/setupSentry.js | 133 ++++++++++++------ test/e2e/tests/errors.spec.js | 37 +++-- ...rs-after-init-opt-in-background-state.json | 64 +++------ 4 files changed, 138 insertions(+), 100 deletions(-) diff --git a/app/scripts/background.js b/app/scripts/background.js index 0c07adf4a..c368728d6 100644 --- a/app/scripts/background.js +++ b/app/scripts/background.js @@ -876,14 +876,14 @@ browser.runtime.onInstalled.addListener(({ reason }) => { function setupSentryGetStateGlobal(store) { global.stateHooks.getSentryState = function () { - const backgroundState = store.getState(); + const backgroundState = store.memStore.getState(); const maskedBackgroundState = maskObject( backgroundState, SENTRY_BACKGROUND_STATE, ); return { browser: window.navigator.userAgent, - store: { metamask: maskedBackgroundState }, + store: maskedBackgroundState, version: platform.getVersion(), }; }; diff --git a/app/scripts/lib/setupSentry.js b/app/scripts/lib/setupSentry.js index 881f3c49e..1838598d0 100644 --- a/app/scripts/lib/setupSentry.js +++ b/app/scripts/lib/setupSentry.js @@ -27,60 +27,103 @@ export const ERROR_URL_ALLOWLIST = { // sent to Sentry These properties have some potential to be useful for // debugging, and they do not contain any identifiable information. export const SENTRY_BACKGROUND_STATE = { - alertEnabledness: true, - completedOnboarding: true, - connectedStatusPopoverHasBeenShown: true, - conversionDate: true, - conversionRate: true, - currentAppVersion: true, - currentBlockGasLimit: true, - currentCurrency: true, - currentLocale: true, - currentMigrationVersion: true, - customNonceValue: true, - defaultHomeActiveTabName: true, - desktopEnabled: true, - featureFlags: true, - firstTimeFlowType: true, - forgottenPassword: true, - incomingTxLastFetchedBlockByChainId: true, - ipfsGateway: true, - isAccountMenuOpen: true, - isInitialized: true, - isUnlocked: true, - metaMetricsId: true, - nativeCurrency: true, - networkId: true, - networkStatus: true, - nextNonce: true, - participateInMetaMetrics: true, - preferences: true, - previousAppVersion: true, - previousMigrationVersion: true, - providerConfig: { - nickname: true, - ticker: true, - type: true, + AccountTracker: { + currentBlockGasLimit: true, + }, + AlertController: { + alertEnabledness: true, + }, + AppMetadataController: { + currentAppVersion: true, + previousAppVersion: true, + previousMigrationVersion: true, + currentMigrationVersion: true, + }, + AppStateController: { + connectedStatusPopoverHasBeenShown: true, + defaultHomeActiveTabName: true, + }, + CurrencyController: { + conversionDate: true, + conversionRate: true, + currentCurrency: true, + nativeCurrency: true, + }, + DecryptMessageController: { + unapprovedDecryptMsgCount: true, + }, + DesktopController: { + desktopEnabled: true, + }, + EncryptionPublicKeyController: { + unapprovedEncryptionPublicKeyMsgCount: true, + }, + IncomingTransactionsController: { + incomingTxLastFetchedBlockByChainId: true, + }, + KeyringController: { + isUnlocked: true, + }, + MetaMetricsController: { + metaMetricsId: true, + participateInMetaMetrics: true, + }, + NetworkController: { + networkId: true, + networkStatus: true, + providerConfig: { + nickname: true, + ticker: true, + type: true, + }, + }, + OnboardingController: { + completedOnboarding: true, + firstTimeFlowType: true, + seedPhraseBackedUp: true, + }, + PreferencesController: { + currentLocale: true, + featureFlags: true, + forgottenPassword: true, + ipfsGateway: true, + preferences: true, + useBlockie: true, + useNonceField: true, + usePhishDetect: true, + }, + SignatureController: { + unapprovedMsgCount: true, + unapprovedPersonalMsgCount: true, + unapprovedTypedMessagesCount: true, }, - seedPhraseBackedUp: true, - unapprovedDecryptMsgCount: true, - unapprovedEncryptionPublicKeyMsgCount: true, - unapprovedMsgCount: true, - unapprovedPersonalMsgCount: true, - unapprovedTypedMessagesCount: true, - useBlockie: true, - useNonceField: true, - usePhishDetect: true, - welcomeScreenSeen: true, }; +const flattenedBackgroundStateMask = Object.values( + SENTRY_BACKGROUND_STATE, +).reduce((partialBackgroundState, controllerState) => { + return { + ...partialBackgroundState, + ...controllerState, + }; +}, {}); + // This describes the subset of Redux state attached to errors sent to Sentry // These properties have some potential to be useful for debugging, and they do // not contain any identifiable information. export const SENTRY_UI_STATE = { gas: true, history: true, - metamask: SENTRY_BACKGROUND_STATE, + metamask: { + ...flattenedBackgroundStateMask, + // This property comes from the background but isn't in controller state + isInitialized: true, + // These properties are in the `metamask` slice but not in the background state + customNonceValue: true, + isAccountMenuOpen: true, + nextNonce: true, + welcomeScreenSeen: true, + }, unconnectedAccount: true, }; diff --git a/test/e2e/tests/errors.spec.js b/test/e2e/tests/errors.spec.js index 4ab80a7de..8784ef168 100644 --- a/test/e2e/tests/errors.spec.js +++ b/test/e2e/tests/errors.spec.js @@ -7,7 +7,8 @@ const { format } = require('prettier'); const { convertToHexValue, withFixtures } = require('../helpers'); const FixtureBuilder = require('../fixture-builder'); -const dateFields = ['metamask.conversionDate']; +const backgroundDateFields = ['CurrencyController.conversionDate']; +const uiDateFields = ['metamask.conversionDate']; /** * Transform date properties to value types, to ensure that state is @@ -15,8 +16,23 @@ const dateFields = ['metamask.conversionDate']; * * @param {unknown} data - The data to transform */ -function transformDates(data) { - for (const field of dateFields) { +function transformBackgroundDates(data) { + for (const field of backgroundDateFields) { + if (has(data, field)) { + set(data, field, typeof get(data, field)); + } + } + return data; +} + +/** + * Transform date properties to value types, to ensure that state is + * consistent between test runs. + * + * @param {unknown} data - The data to transform + */ +function transformUiDates(data) { + for (const field of uiDateFields) { if (has(data, field)) { set(data, field, typeof get(data, field)); } @@ -33,12 +49,10 @@ function transformDates(data) { * @param {boolean} [args.update] - Whether to update the snapshot if it doesn't match. */ async function matchesSnapshot({ - data: unprocessedData, + data, snapshot, update = process.env.UPDATE_SNAPSHOTS === 'true', }) { - const data = transformDates(unprocessedData); - const snapshotPath = resolve(__dirname, `./state-snapshots/${snapshot}.json`); const rawSnapshotData = await fs.readFile(snapshotPath, { encoding: 'utf-8', @@ -243,7 +257,7 @@ describe('Sentry errors', function () { const mockJsonBody = JSON.parse(mockTextBody[2]); const appState = mockJsonBody?.extra?.appState; await matchesSnapshot({ - data: appState, + data: transformBackgroundDates(appState), snapshot: 'errors-before-init-opt-in-background-state', }); }, @@ -328,7 +342,7 @@ describe('Sentry errors', function () { const mockJsonBody = JSON.parse(mockTextBody[2]); const appState = mockJsonBody?.extra?.appState; await matchesSnapshot({ - data: appState, + data: transformUiDates(appState), snapshot: 'errors-before-init-opt-in-ui-state', }); }, @@ -436,7 +450,8 @@ describe('Sentry errors', function () { const mockJsonBody = JSON.parse(mockTextBody[2]); const { level, extra } = mockJsonBody; const [{ type, value }] = mockJsonBody.exception.values; - const { participateInMetaMetrics } = extra.appState.store.metamask; + const { participateInMetaMetrics } = + extra.appState.store.MetaMetricsController; // Verify request assert.equal(type, 'TestError'); assert.equal(value, 'Test Error'); @@ -494,7 +509,7 @@ describe('Sentry errors', function () { 'Invalid version state', ); await matchesSnapshot({ - data: appState.store, + data: transformBackgroundDates(appState.store), snapshot: 'errors-after-init-opt-in-background-state', }); }, @@ -588,7 +603,7 @@ describe('Sentry errors', function () { 'Invalid version state', ); await matchesSnapshot({ - data: appState.store, + data: transformUiDates(appState.store), snapshot: 'errors-after-init-opt-in-ui-state', }); }, diff --git a/test/e2e/tests/state-snapshots/errors-after-init-opt-in-background-state.json b/test/e2e/tests/state-snapshots/errors-after-init-opt-in-background-state.json index 03fc85edf..f41fee885 100644 --- a/test/e2e/tests/state-snapshots/errors-after-init-opt-in-background-state.json +++ b/test/e2e/tests/state-snapshots/errors-after-init-opt-in-background-state.json @@ -1,52 +1,32 @@ { - "metamask": { - "isInitialized": true, + "AccountTracker": { "currentBlockGasLimit": "0x1c9c380" }, + "AppStateController": { "connectedStatusPopoverHasBeenShown": true, - "defaultHomeActiveTabName": null, - "currentAppVersion": "10.34.4", - "previousAppVersion": "", - "previousMigrationVersion": 0, - "currentMigrationVersion": 94, + "defaultHomeActiveTabName": null + }, + "CurrencyController": { + "conversionDate": "number", + "conversionRate": 1700, + "nativeCurrency": "ETH", + "currentCurrency": "usd" + }, + "DecryptMessageController": { "unapprovedDecryptMsgCount": 0 }, + "EncryptionPublicKeyController": { + "unapprovedEncryptionPublicKeyMsgCount": 0 + }, + "MetaMetricsController": { + "participateInMetaMetrics": true, + "metaMetricsId": "fake-metrics-id" + }, + "NetworkController": { "networkId": "1337", "providerConfig": { "nickname": "Localhost 8545", "ticker": "ETH", "type": "rpc" - }, - "isUnlocked": false, - "useBlockie": false, - "useNonceField": false, - "usePhishDetect": true, - "featureFlags": { "showIncomingTransactions": true }, - "currentLocale": "en", - "forgottenPassword": false, - "preferences": { - "hideZeroBalanceTokens": false, - "showFiatInTestnets": false, - "showTestNetworks": false, - "useNativeCurrencyAsPrimaryCurrency": true - }, - "ipfsGateway": "dweb.link", - "participateInMetaMetrics": true, - "metaMetricsId": "fake-metrics-id", - "conversionDate": "number", - "conversionRate": 1700, - "nativeCurrency": "ETH", - "currentCurrency": "usd", - "alertEnabledness": { "unconnectedAccount": true, "web3ShimUsage": true }, - "seedPhraseBackedUp": true, - "firstTimeFlowType": "import", - "completedOnboarding": true, - "incomingTxLastFetchedBlockByChainId": { - "0x1": null, - "0xe708": null, - "0x5": null, - "0xaa36a7": null, - "0xe704": null - }, - "currentBlockGasLimit": "0x1c9c380", - "unapprovedDecryptMsgCount": 0, - "unapprovedEncryptionPublicKeyMsgCount": 0, + } + }, + "SignatureController": { "unapprovedMsgCount": 0, "unapprovedPersonalMsgCount": 0, "unapprovedTypedMessagesCount": 0 From 915bf2ae88d8ee904a92669a1ceb162c07fc1cac Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Wed, 16 Aug 2023 16:56:20 -0230 Subject: [PATCH 10/23] Capture exception with sentry when invariant conditions are met in migrations (#20427) * capture exception for sentry when invariant conditions are met in migration 82 * Code cleanup * Capture exceptions in invariant conditions for migrations 83,84,85,86,89,91,93,94 * Update app/scripts/migrations/082.test.js Co-authored-by: Mark Stacey * Code cleanup * Fix SentryObject type declaration * Stop throwing error if preferences controller is undefined * Refactor 084 and 086 to remove double negative * Capture exceptions for invariant states in in migrations 87,88,90 and 92 * lint fix * log warning in migration 82 when preferences controller is undefined --------- Co-authored-by: Mark Stacey --- app/scripts/migrations/082.test.js | 178 +++++++++++++++++++- app/scripts/migrations/082.ts | 55 +++++- app/scripts/migrations/083.test.js | 65 +++++++ app/scripts/migrations/083.ts | 10 ++ app/scripts/migrations/084.test.js | 72 ++++++++ app/scripts/migrations/084.ts | 21 ++- app/scripts/migrations/085.test.js | 29 ++++ app/scripts/migrations/085.ts | 5 + app/scripts/migrations/086.test.js | 75 +++++++++ app/scripts/migrations/086.ts | 19 +++ app/scripts/migrations/087.test.js | 69 ++++++++ app/scripts/migrations/087.ts | 5 + app/scripts/migrations/088.test.ts | 262 +++++++++++++++++++++++++++++ app/scripts/migrations/088.ts | 80 +++++++++ app/scripts/migrations/089.test.ts | 58 +++++++ app/scripts/migrations/089.ts | 13 ++ app/scripts/migrations/090.test.js | 19 ++- app/scripts/migrations/090.ts | 22 ++- app/scripts/migrations/091.test.ts | 58 +++++++ app/scripts/migrations/091.ts | 13 ++ app/scripts/migrations/092.test.ts | 29 ++++ app/scripts/migrations/092.ts | 9 + types/global.d.ts | 4 + 23 files changed, 1153 insertions(+), 17 deletions(-) diff --git a/app/scripts/migrations/082.test.js b/app/scripts/migrations/082.test.js index 6f221c5c6..0060c853c 100644 --- a/app/scripts/migrations/082.test.js +++ b/app/scripts/migrations/082.test.js @@ -1,6 +1,12 @@ import { v4 } from 'uuid'; import { migrate, version } from './082'; +const sentryCaptureExceptionMock = jest.fn(); + +global.sentry = { + captureException: sentryCaptureExceptionMock, +}; + jest.mock('uuid', () => { const actual = jest.requireActual('uuid'); @@ -472,10 +478,72 @@ describe('migration #82', () => { }, }; const newStorage = await migrate(oldStorage); + expect(sentryCaptureExceptionMock).toHaveBeenCalled(); expect(newStorage.data).toStrictEqual(oldStorage.data); }); - it('should not change anything if there is no frequentRpcListDetail property on PreferencesController', async () => { + it('should capture an exception if any PreferencesController.frequentRpcListDetail entries are not objects', async () => { + const oldStorage = { + meta: { + version: 81, + }, + data: { + PreferencesController: { + transactionSecurityCheckEnabled: false, + useBlockie: false, + useCurrencyRateCheck: true, + useMultiAccountBalanceChecker: true, + useNftDetection: false, + useNonceField: false, + frequentRpcListDetail: [ + { + chainId: '0x539', + nickname: 'Localhost 8545', + rpcPrefs: {}, + rpcUrl: 'http://localhost:8545', + ticker: 'ETH', + }, + 'invalid entry type', + 1, + ], + }, + NetworkController: { + network: '1', + networkDetails: { + EIPS: { + 1559: true, + }, + }, + previousProviderStore: { + chainId: '0x89', + nickname: 'Polygon Mainnet', + rpcPrefs: {}, + rpcUrl: + 'https://polygon-mainnet.infura.io/v3/373266a93aab4acda48f89d4fe77c748', + ticker: 'MATIC', + type: 'rpc', + }, + provider: { + chainId: '0x1', + nickname: '', + rpcPrefs: {}, + rpcUrl: '', + ticker: 'ETH', + type: 'mainnet', + }, + }, + }, + }; + await migrate(oldStorage); + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error( + `state.PreferencesController.frequentRpcListDetail contains an element of type string`, + ), + ); + }); + + it('should not change anything, and not capture an exception, if there is no frequentRpcListDetail property on PreferencesController but there is a networkConfigurations object', async () => { const oldStorage = { meta: { version: 81, @@ -556,9 +624,60 @@ describe('migration #82', () => { }, }; const newStorage = await migrate(oldStorage); + expect(sentryCaptureExceptionMock).not.toHaveBeenCalled(); expect(newStorage.data).toStrictEqual(oldStorage.data); }); + it('should capture an exception if there is no frequentRpcListDetail property on PreferencesController and no networkConfiguration object', async () => { + const oldStorage = { + meta: { + version: 81, + }, + data: { + PreferencesController: { + transactionSecurityCheckEnabled: false, + useBlockie: false, + useCurrencyRateCheck: true, + useMultiAccountBalanceChecker: true, + useNftDetection: false, + useNonceField: false, + }, + NetworkController: { + network: '1', + networkDetails: { + EIPS: { + 1559: true, + }, + }, + previousProviderStore: { + chainId: '0x89', + nickname: 'Polygon Mainnet', + rpcPrefs: {}, + rpcUrl: + 'https://polygon-mainnet.infura.io/v3/373266a93aab4acda48f89d4fe77c748', + ticker: 'MATIC', + type: 'rpc', + }, + provider: { + chainId: '0x1', + nickname: '', + rpcPrefs: {}, + rpcUrl: '', + ticker: 'ETH', + type: 'mainnet', + }, + }, + }, + }; + await migrate(oldStorage); + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error( + `typeof state.PreferencesController.frequentRpcListDetail is undefined`, + ), + ); + }); + it('should change nothing if PreferencesController is undefined', async () => { const oldStorage = { meta: { @@ -595,4 +714,61 @@ describe('migration #82', () => { const newStorage = await migrate(oldStorage); expect(newStorage.data).toStrictEqual(oldStorage.data); }); + + it('should capture an exception if PreferencesController is not an object', async () => { + const oldStorage = { + meta: { + version: 81, + }, + data: { + NetworkController: { + network: '1', + networkDetails: { + EIPS: { + 1559: true, + }, + }, + previousProviderStore: { + chainId: '0x89', + nickname: 'Polygon Mainnet', + rpcPrefs: {}, + rpcUrl: + 'https://polygon-mainnet.infura.io/v3/373266a93aab4acda48f89d4fe77c748', + ticker: 'MATIC', + type: 'rpc', + }, + provider: { + chainId: '0x1', + nickname: '', + rpcPrefs: {}, + rpcUrl: '', + ticker: 'ETH', + type: 'mainnet', + }, + }, + PreferencesController: false, + }, + }; + await migrate(oldStorage); + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.PreferencesController is boolean`), + ); + }); + + it('should capture an exception if NetworkController is undefined', async () => { + const oldStorage = { + meta: { + version: 81, + }, + data: { + PreferencesController: {}, + }, + }; + await migrate(oldStorage); + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.NetworkController is undefined`), + ); + }); }); diff --git a/app/scripts/migrations/082.ts b/app/scripts/migrations/082.ts index fdda1fb8d..29ff59eea 100644 --- a/app/scripts/migrations/082.ts +++ b/app/scripts/migrations/082.ts @@ -1,6 +1,7 @@ import { cloneDeep } from 'lodash'; import { hasProperty, isObject } from '@metamask/utils'; import { v4 } from 'uuid'; +import log from 'loglevel'; export const version = 82; @@ -25,14 +26,56 @@ export async function migrate(originalVersionedData: { } function transformState(state: Record) { + if (!hasProperty(state, 'PreferencesController')) { + log.warn(`state.PreferencesController is undefined`); + return state; + } + if (!isObject(state.PreferencesController)) { + global.sentry?.captureException?.( + new Error( + `typeof state.PreferencesController is ${typeof state.PreferencesController}`, + ), + ); + return state; + } if ( - !hasProperty(state, 'PreferencesController') || - !isObject(state.PreferencesController) || - !isObject(state.NetworkController) || - !hasProperty(state.PreferencesController, 'frequentRpcListDetail') || - !Array.isArray(state.PreferencesController.frequentRpcListDetail) || - !state.PreferencesController.frequentRpcListDetail.every(isObject) + !hasProperty(state, 'NetworkController') || + !isObject(state.NetworkController) ) { + global.sentry?.captureException?.( + new Error( + `typeof state.NetworkController is ${typeof state.NetworkController}`, + ), + ); + return state; + } + if ( + !hasProperty(state.PreferencesController, 'frequentRpcListDetail') || + !Array.isArray(state.PreferencesController.frequentRpcListDetail) + ) { + const inPost077SupplementFor082State = + state.NetworkController.networkConfigurations && + state.PreferencesController.frequentRpcListDetail === undefined; + if (!inPost077SupplementFor082State) { + global.sentry?.captureException?.( + new Error( + `typeof state.PreferencesController.frequentRpcListDetail is ${typeof state + .PreferencesController.frequentRpcListDetail}`, + ), + ); + } + return state; + } + if (!state.PreferencesController.frequentRpcListDetail.every(isObject)) { + const erroneousElement = + state.PreferencesController.frequentRpcListDetail.find( + (element) => !isObject(element), + ); + global.sentry?.captureException?.( + new Error( + `state.PreferencesController.frequentRpcListDetail contains an element of type ${typeof erroneousElement}`, + ), + ); return state; } const { PreferencesController, NetworkController } = state; diff --git a/app/scripts/migrations/083.test.js b/app/scripts/migrations/083.test.js index c59951aef..b8f91dfcd 100644 --- a/app/scripts/migrations/083.test.js +++ b/app/scripts/migrations/083.test.js @@ -1,6 +1,12 @@ import { v4 } from 'uuid'; import { migrate, version } from './083'; +const sentryCaptureExceptionMock = jest.fn(); + +global.sentry = { + captureException: sentryCaptureExceptionMock, +}; + jest.mock('uuid', () => { const actual = jest.requireActual('uuid'); @@ -165,6 +171,24 @@ describe('migration #83', () => { expect(newStorage).toStrictEqual(expectedNewStorage); }); + it('should capture an exception if state.NetworkController is undefined', async () => { + const oldStorage = { + meta: { + version, + }, + data: { + testProperty: 'testValue', + }, + }; + + await migrate(oldStorage); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.NetworkController is undefined`), + ); + }); + it('should not modify state if state.NetworkController is not an object', async () => { const oldStorage = { meta: { @@ -190,6 +214,25 @@ describe('migration #83', () => { expect(newStorage).toStrictEqual(expectedNewStorage); }); + it('should capture an exception if state.NetworkController is not an object', async () => { + const oldStorage = { + meta: { + version, + }, + data: { + NetworkController: false, + testProperty: 'testValue', + }, + }; + + await migrate(oldStorage); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.NetworkController is boolean`), + ); + }); + it('should not modify state if state.NetworkController.networkConfigurations is undefined', async () => { const oldStorage = { meta: { @@ -221,6 +264,28 @@ describe('migration #83', () => { expect(newStorage).toStrictEqual(expectedNewStorage); }); + it('should capture an exception if state.NetworkController.networkConfigurations is undefined', async () => { + const oldStorage = { + meta: { + version, + }, + data: { + NetworkController: { + testNetworkControllerProperty: 'testNetworkControllerValue', + networkConfigurations: undefined, + }, + testProperty: 'testValue', + }, + }; + + await migrate(oldStorage); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof NetworkController.networkConfigurations is undefined`), + ); + }); + it('should not modify state if state.NetworkController.networkConfigurations is an empty object', async () => { const oldStorage = { meta: { diff --git a/app/scripts/migrations/083.ts b/app/scripts/migrations/083.ts index cc3e3b16b..bd7ba62e4 100644 --- a/app/scripts/migrations/083.ts +++ b/app/scripts/migrations/083.ts @@ -25,11 +25,21 @@ export async function migrate(originalVersionedData: { function transformState(state: Record) { if (!isObject(state.NetworkController)) { + global.sentry?.captureException?.( + new Error( + `typeof state.NetworkController is ${typeof state.NetworkController}`, + ), + ); return state; } const { NetworkController } = state; if (!isObject(NetworkController.networkConfigurations)) { + global.sentry?.captureException?.( + new Error( + `typeof NetworkController.networkConfigurations is ${typeof NetworkController.networkConfigurations}`, + ), + ); return state; } diff --git a/app/scripts/migrations/084.test.js b/app/scripts/migrations/084.test.js index 138bfacb6..bd0e1b35c 100644 --- a/app/scripts/migrations/084.test.js +++ b/app/scripts/migrations/084.test.js @@ -1,6 +1,16 @@ import { migrate } from './084'; +const sentryCaptureExceptionMock = jest.fn(); + +global.sentry = { + captureException: sentryCaptureExceptionMock, +}; + describe('migration #84', () => { + afterEach(() => { + jest.resetAllMocks(); + }); + it('updates the version metadata', async () => { const originalVersionedData = buildOriginalVersionedData({ meta: { @@ -27,6 +37,21 @@ describe('migration #84', () => { expect(newVersionedData.data).toStrictEqual(originalVersionedData.data); }); + it('captures an exception if the network controller state does not exist', async () => { + const originalVersionedData = buildOriginalVersionedData({ + data: { + test: '123', + }, + }); + + await migrate(originalVersionedData); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.NetworkController is undefined`), + ); + }); + const nonObjects = [undefined, null, 'test', 1, ['test']]; for (const invalidState of nonObjects) { it(`does not change the state if the network controller state is ${invalidState}`, async () => { @@ -40,6 +65,21 @@ describe('migration #84', () => { expect(newVersionedData.data).toStrictEqual(originalVersionedData.data); }); + + it(`captures an exception if the network controller state is ${invalidState}`, async () => { + const originalVersionedData = buildOriginalVersionedData({ + data: { + NetworkController: invalidState, + }, + }); + + await migrate(originalVersionedData); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.NetworkController is ${typeof invalidState}`), + ); + }); } it('does not change the state if the network controller state does not include "network"', async () => { @@ -56,6 +96,38 @@ describe('migration #84', () => { expect(newVersionedData.data).toStrictEqual(originalVersionedData.data); }); + it('captures an exception if the network controller state does not include "network" and does not include "networkId"', async () => { + const originalVersionedData = buildOriginalVersionedData({ + data: { + NetworkController: { + test: '123', + }, + }, + }); + + await migrate(originalVersionedData); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.NetworkController.network is undefined`), + ); + }); + + it('does not capture an exception if the network controller state does not include "network" but does include "networkId"', async () => { + const originalVersionedData = buildOriginalVersionedData({ + data: { + NetworkController: { + test: '123', + networkId: 'foobar', + }, + }, + }); + + await migrate(originalVersionedData); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(0); + }); + it('replaces "network" in the network controller state with "networkId": null, "networkStatus": "unknown" if it is "loading"', async () => { const originalVersionedData = buildOriginalVersionedData({ data: { diff --git a/app/scripts/migrations/084.ts b/app/scripts/migrations/084.ts index 66a2f45ae..7551ed2c6 100644 --- a/app/scripts/migrations/084.ts +++ b/app/scripts/migrations/084.ts @@ -25,9 +25,26 @@ export async function migrate(originalVersionedData: { function transformState(state: Record) { if ( !hasProperty(state, 'NetworkController') || - !isObject(state.NetworkController) || - !hasProperty(state.NetworkController, 'network') + !isObject(state.NetworkController) ) { + global.sentry?.captureException?.( + new Error( + `typeof state.NetworkController is ${typeof state.NetworkController}`, + ), + ); + return state; + } + if (!hasProperty(state.NetworkController, 'network')) { + const thePost077SupplementFor084HasNotModifiedState = + state.NetworkController.networkId === undefined; + if (thePost077SupplementFor084HasNotModifiedState) { + global.sentry?.captureException?.( + new Error( + `typeof state.NetworkController.network is ${typeof state + .NetworkController.network}`, + ), + ); + } return state; } diff --git a/app/scripts/migrations/085.test.js b/app/scripts/migrations/085.test.js index 6b7b4967d..cfe7ff1b0 100644 --- a/app/scripts/migrations/085.test.js +++ b/app/scripts/migrations/085.test.js @@ -1,5 +1,11 @@ import { migrate, version } from './085'; +const sentryCaptureExceptionMock = jest.fn(); + +global.sentry = { + captureException: sentryCaptureExceptionMock, +}; + jest.mock('uuid', () => { const actual = jest.requireActual('uuid'); @@ -10,6 +16,10 @@ jest.mock('uuid', () => { }); describe('migration #85', () => { + afterEach(() => { + jest.resetAllMocks(); + }); + it('should update the version metadata', async () => { const oldStorage = { meta: { @@ -39,6 +49,25 @@ describe('migration #85', () => { expect(newStorage.data).toStrictEqual(oldData); }); + it('should capture an exception there is no network controller state', async () => { + const oldData = { + other: 'data', + }; + const oldStorage = { + meta: { + version: 84, + }, + data: oldData, + }; + + await migrate(oldStorage); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.NetworkController is undefined`), + ); + }); + it('should return state unaltered if there is no network controller previous provider state', async () => { const oldData = { other: 'data', diff --git a/app/scripts/migrations/085.ts b/app/scripts/migrations/085.ts index 03499d2b2..2ba22a346 100644 --- a/app/scripts/migrations/085.ts +++ b/app/scripts/migrations/085.ts @@ -24,6 +24,11 @@ export async function migrate(originalVersionedData: { function transformState(state: Record) { if (!isObject(state.NetworkController)) { + global.sentry?.captureException?.( + new Error( + `typeof state.NetworkController is ${typeof state.NetworkController}`, + ), + ); return state; } diff --git a/app/scripts/migrations/086.test.js b/app/scripts/migrations/086.test.js index f38f0444d..cd6a61377 100644 --- a/app/scripts/migrations/086.test.js +++ b/app/scripts/migrations/086.test.js @@ -1,5 +1,11 @@ import { migrate, version } from './086'; +const sentryCaptureExceptionMock = jest.fn(); + +global.sentry = { + captureException: sentryCaptureExceptionMock, +}; + jest.mock('uuid', () => { const actual = jest.requireActual('uuid'); @@ -10,6 +16,10 @@ jest.mock('uuid', () => { }); describe('migration #86', () => { + afterEach(() => { + jest.resetAllMocks(); + }); + it('should update the version metadata', async () => { const oldStorage = { meta: { @@ -39,6 +49,25 @@ describe('migration #86', () => { expect(newStorage.data).toStrictEqual(oldData); }); + it('should capture an exception if there is no network controller state', async () => { + const oldData = { + other: 'data', + }; + const oldStorage = { + meta: { + version: 85, + }, + data: oldData, + }; + + await migrate(oldStorage); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.NetworkController is undefined`), + ); + }); + it('should return state unaltered if there is no network controller provider state', async () => { const oldData = { other: 'data', @@ -59,6 +88,52 @@ describe('migration #86', () => { expect(newStorage.data).toStrictEqual(oldData); }); + it('should capture an exception if there is no network controller provider state and no providerConfig state', async () => { + const oldData = { + other: 'data', + NetworkController: { + networkConfigurations: { + foo: 'bar', + }, + }, + }; + const oldStorage = { + meta: { + version: 85, + }, + data: oldData, + }; + + await migrate(oldStorage); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.NetworkController.provider is undefined`), + ); + }); + + it('should not capture an exception if there is no network controller provider state but there is a providerConfig state', async () => { + const oldData = { + other: 'data', + NetworkController: { + networkConfigurations: { + foo: 'bar', + }, + providerConfig: {}, + }, + }; + const oldStorage = { + meta: { + version: 85, + }, + data: oldData, + }; + + await migrate(oldStorage); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(0); + }); + it('should rename the provider config state', async () => { const oldData = { other: 'data', diff --git a/app/scripts/migrations/086.ts b/app/scripts/migrations/086.ts index c4b60e270..709934f50 100644 --- a/app/scripts/migrations/086.ts +++ b/app/scripts/migrations/086.ts @@ -37,5 +37,24 @@ function transformState(state: Record) { NetworkController: networkControllerState, }; } + if (!isObject(state.NetworkController)) { + global.sentry?.captureException?.( + new Error( + `typeof state.NetworkController is ${typeof state.NetworkController}`, + ), + ); + } else if (!hasProperty(state.NetworkController, 'provider')) { + const thePost077SupplementFor086HasNotModifiedState = + state.NetworkController.providerConfig === undefined; + if (thePost077SupplementFor086HasNotModifiedState) { + global.sentry?.captureException?.( + new Error( + `typeof state.NetworkController.provider is ${typeof state + .NetworkController.provider}`, + ), + ); + } + } + return state; } diff --git a/app/scripts/migrations/087.test.js b/app/scripts/migrations/087.test.js index 1d631e926..ebd9495ad 100644 --- a/app/scripts/migrations/087.test.js +++ b/app/scripts/migrations/087.test.js @@ -1,6 +1,16 @@ import { migrate, version } from './087'; +const sentryCaptureExceptionMock = jest.fn(); + +global.sentry = { + captureException: sentryCaptureExceptionMock, +}; + describe('migration #87', () => { + afterEach(() => { + jest.resetAllMocks(); + }); + it('should update the version metadata', async () => { const oldStorage = { meta: { @@ -53,6 +63,65 @@ describe('migration #87', () => { expect(newStorage.data).toStrictEqual(oldData); }); + it('should return state unaltered if TokensController state is not an object', async () => { + const oldData = { + other: 'data', + TokensController: false, + }; + const oldStorage = { + meta: { + version: 86, + }, + data: oldData, + }; + + const newStorage = await migrate(oldStorage); + expect(newStorage.data).toStrictEqual(oldData); + }); + + it('should capture an exception if TokensController state is not an object', async () => { + const oldData = { + other: 'data', + TokensController: false, + }; + const oldStorage = { + meta: { + version: 86, + }, + data: oldData, + }; + + await migrate(oldStorage); + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.TokensController is boolean`), + ); + }); + + it('should not capture an exception if TokensController state is an object', async () => { + const oldData = { + other: 'data', + TokensController: { + allDetectedTokens: {}, + allIgnoredTokens: {}, + allTokens: {}, + detectedTokens: [], + ignoredTokens: [], + suggestedAssets: [], + tokens: [], + }, + }; + const oldStorage = { + meta: { + version: 86, + }, + data: oldData, + }; + + await migrate(oldStorage); + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(0); + }); + it('should remove the suggested assets state', async () => { const oldData = { other: 'data', diff --git a/app/scripts/migrations/087.ts b/app/scripts/migrations/087.ts index 92093f967..f900faab6 100644 --- a/app/scripts/migrations/087.ts +++ b/app/scripts/migrations/087.ts @@ -24,6 +24,11 @@ export async function migrate(originalVersionedData: { function transformState(state: Record) { if (!isObject(state.TokensController)) { + global.sentry?.captureException?.( + new Error( + `typeof state.TokensController is ${typeof state.TokensController}`, + ), + ); return state; } diff --git a/app/scripts/migrations/088.test.ts b/app/scripts/migrations/088.test.ts index a33c30eff..ca672f982 100644 --- a/app/scripts/migrations/088.test.ts +++ b/app/scripts/migrations/088.test.ts @@ -1,6 +1,16 @@ import { migrate } from './088'; +const sentryCaptureExceptionMock = jest.fn(); + +global.sentry = { + captureException: sentryCaptureExceptionMock, +}; + describe('migration #88', () => { + afterEach(() => { + jest.resetAllMocks(); + }); + it('updates the version metadata', async () => { const oldStorage = { meta: { version: 87 }, @@ -26,6 +36,24 @@ describe('migration #88', () => { expect(newStorage.data).toStrictEqual(oldData); }); + it('captures an exception if the NftController property is not an object', async () => { + const oldData = { + TokenListController: {}, + TokensController: {}, + NftController: false, + }; + const oldStorage = { + meta: { version: 87 }, + data: oldData, + }; + + await migrate(oldStorage); + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.NftController is boolean`), + ); + }); + it('returns the state unaltered if the NftController object has no allNftContracts property', async () => { const oldData = { NftController: { @@ -58,6 +86,26 @@ describe('migration #88', () => { expect(newStorage.data).toStrictEqual(oldData); }); + it('captures an exception if it NftController.allNftContracts is not an object', async () => { + const oldData = { + TokenListController: {}, + TokensController: {}, + NftController: { + allNftContracts: 'foo', + }, + }; + const oldStorage = { + meta: { version: 87 }, + data: oldData, + }; + + await migrate(oldStorage); + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.NftController.allNftContracts is string`), + ); + }); + it('returns the state unaltered if any value of the NftController.allNftContracts object is not an object itself', async () => { const oldData = { NftController: { @@ -324,6 +372,26 @@ describe('migration #88', () => { expect(newStorage.data).toStrictEqual(oldData); }); + it('captures an exception if it NftController.allNfts is not an object', async () => { + const oldData = { + TokenListController: {}, + TokensController: {}, + NftController: { + allNfts: 'foo', + }, + }; + const oldStorage = { + meta: { version: 87 }, + data: oldData, + }; + + await migrate(oldStorage); + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.NftController.allNfts is string`), + ); + }); + it('returns the state unaltered if any value of the NftController.allNfts object is not an object itself', async () => { const oldData = { NftController: { @@ -656,6 +724,91 @@ describe('migration #88', () => { expect(newStorage.data).toStrictEqual(oldData); }); + it('captures an exception if it has no TokenListController property', async () => { + const oldData = { + TokensController: {}, + NftController: { + allNfts: { + '0x111': { + '0x10': [ + { + name: 'NFT 1', + description: 'Description for NFT 1', + image: 'nft1.jpg', + standard: 'ERC721', + tokenId: '1', + address: '0xaaa', + }, + ], + }, + }, + allNftContracts: { + '0x111': { + '0x10': [ + { + name: 'Contract 1', + address: '0xaaa', + }, + ], + }, + }, + }, + }; + const oldStorage = { + meta: { version: 87 }, + data: oldData, + }; + + await migrate(oldStorage); + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.TokenListController is undefined`), + ); + }); + + it('captures an exception if the TokenListController property is not an object', async () => { + const oldData = { + TokensController: {}, + NftController: { + allNfts: { + '0x111': { + '0x10': [ + { + name: 'NFT 1', + description: 'Description for NFT 1', + image: 'nft1.jpg', + standard: 'ERC721', + tokenId: '1', + address: '0xaaa', + }, + ], + }, + }, + allNftContracts: { + '0x111': { + '0x10': [ + { + name: 'Contract 1', + address: '0xaaa', + }, + ], + }, + }, + }, + TokenListController: false, + }; + const oldStorage = { + meta: { version: 87 }, + data: oldData, + }; + + await migrate(oldStorage); + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.TokenListController is boolean`), + ); + }); + it('returns the state unaltered if the TokenListController object has no tokensChainsCache property', async () => { const oldData = { TokenListController: { @@ -688,6 +841,25 @@ describe('migration #88', () => { expect(newStorage.data).toStrictEqual(oldData); }); + it('captures an exception if the TokenListController.tokensChainsCache property is not an object', async () => { + const oldData = { + TokenListController: { + tokensChainsCache: 'foo', + }, + TokensController: {}, + }; + const oldStorage = { + meta: { version: 87 }, + data: oldData, + }; + + await migrate(oldStorage); + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.TokenListController.tokensChainsCache is string`), + ); + }); + it('rewrites TokenListController.tokensChainsCache so that decimal chain IDs are converted to hex strings', async () => { const oldStorage = { meta: { version: 87 }, @@ -919,6 +1091,39 @@ describe('migration #88', () => { expect(newStorage.data).toStrictEqual(oldData); }); + it('captures an exception if it has no TokensController property', async () => { + const oldData = { + TokenListController: {}, + }; + const oldStorage = { + meta: { version: 87 }, + data: oldData, + }; + + await migrate(oldStorage); + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.TokensController is undefined`), + ); + }); + + it('captures an exception if the TokensController property is not an object', async () => { + const oldData = { + TokenListController: {}, + TokensController: false, + }; + const oldStorage = { + meta: { version: 87 }, + data: oldData, + }; + + await migrate(oldStorage); + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.TokensController is boolean`), + ); + }); + it('returns the state unaltered if the TokensController object has no allTokens property', async () => { const oldData = { TokensController: { @@ -951,6 +1156,25 @@ describe('migration #88', () => { expect(newStorage.data).toStrictEqual(oldData); }); + it('captures an exception if the TokensController.allTokens property is not an object', async () => { + const oldData = { + TokenListController: {}, + TokensController: { + allTokens: 'foo', + }, + }; + const oldStorage = { + meta: { version: 87 }, + data: oldData, + }; + + await migrate(oldStorage); + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.TokensController.allTokens is string`), + ); + }); + it('rewrites TokensController.allTokens so that decimal chain IDs are converted to hex strings', async () => { const oldStorage = { meta: { version: 87 }, @@ -1163,6 +1387,25 @@ describe('migration #88', () => { expect(newStorage.data).toStrictEqual(oldData); }); + it('captures an exception if the TokensController.allIgnoredTokens property is not an object', async () => { + const oldData = { + TokenListController: {}, + TokensController: { + allIgnoredTokens: 'foo', + }, + }; + const oldStorage = { + meta: { version: 87 }, + data: oldData, + }; + + await migrate(oldStorage); + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.TokensController.allIgnoredTokens is string`), + ); + }); + it('rewrites TokensController.allIgnoredTokens so that decimal chain IDs are converted to hex strings', async () => { const oldStorage = { meta: { version: 87 }, @@ -1323,6 +1566,25 @@ describe('migration #88', () => { expect(newStorage.data).toStrictEqual(oldData); }); + it('captures an exception if the TokensController.allDetectedTokens property is not an object', async () => { + const oldData = { + TokenListController: {}, + TokensController: { + allDetectedTokens: 'foo', + }, + }; + const oldStorage = { + meta: { version: 87 }, + data: oldData, + }; + + await migrate(oldStorage); + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.TokensController.allDetectedTokens is string`), + ); + }); + it('rewrites TokensController.allDetectedTokens so that decimal chain IDs are converted to hex strings', async () => { const oldStorage = { meta: { version: 87 }, diff --git a/app/scripts/migrations/088.ts b/app/scripts/migrations/088.ts index a4b874b2f..5ede1b0fa 100644 --- a/app/scripts/migrations/088.ts +++ b/app/scripts/migrations/088.ts @@ -1,6 +1,7 @@ import { hasProperty, Hex, isObject, isStrictHexString } from '@metamask/utils'; import { BN } from 'ethereumjs-util'; import { cloneDeep, mapKeys } from 'lodash'; +import log from 'loglevel'; type VersionedData = { meta: { version: number }; @@ -70,6 +71,16 @@ function migrateData(state: Record): void { } }); } + } else if (hasProperty(nftControllerState, 'allNftContracts')) { + global.sentry?.captureException?.( + new Error( + `typeof state.NftController.allNftContracts is ${typeof nftControllerState.allNftContracts}`, + ), + ); + } else { + log.warn( + `typeof state.NftController.allNftContracts is ${typeof nftControllerState.allNftContracts}`, + ); } // Migrate NftController.allNfts @@ -96,9 +107,25 @@ function migrateData(state: Record): void { } }); } + } else if (hasProperty(nftControllerState, 'allNfts')) { + global.sentry?.captureException?.( + new Error( + `typeof state.NftController.allNfts is ${typeof nftControllerState.allNfts}`, + ), + ); + } else { + log.warn( + `typeof state.NftController.allNfts is ${typeof nftControllerState.allNfts}`, + ); } state.NftController = nftControllerState; + } else if (hasProperty(state, 'NftController')) { + global.sentry?.captureException?.( + new Error(`typeof state.NftController is ${typeof state.NftController}`), + ); + } else { + log.warn(`typeof state.NftController is undefined`); } if ( @@ -124,7 +151,24 @@ function migrateData(state: Record): void { tokenListControllerState.tokensChainsCache, (_, chainId: string) => toHex(chainId), ); + } else if (hasProperty(tokenListControllerState, 'tokensChainsCache')) { + global.sentry?.captureException?.( + new Error( + `typeof state.TokenListController.tokensChainsCache is ${typeof state + .TokenListController.tokensChainsCache}`, + ), + ); + } else { + log.warn( + `typeof state.TokenListController.tokensChainsCache is undefined`, + ); } + } else { + global.sentry?.captureException?.( + new Error( + `typeof state.TokenListController is ${typeof state.TokenListController}`, + ), + ); } if ( @@ -150,6 +194,16 @@ function migrateData(state: Record): void { allTokens, (_, chainId: string) => toHex(chainId), ); + } else if (hasProperty(tokensControllerState, 'allTokens')) { + global.sentry?.captureException?.( + new Error( + `typeof state.TokensController.allTokens is ${typeof tokensControllerState.allTokens}`, + ), + ); + } else { + log.warn( + `typeof state.TokensController.allTokens is ${typeof tokensControllerState.allTokens}`, + ); } // Migrate TokensController.allIgnoredTokens @@ -169,6 +223,16 @@ function migrateData(state: Record): void { allIgnoredTokens, (_, chainId: string) => toHex(chainId), ); + } else if (hasProperty(tokensControllerState, 'allIgnoredTokens')) { + global.sentry?.captureException?.( + new Error( + `typeof state.TokensController.allIgnoredTokens is ${typeof tokensControllerState.allIgnoredTokens}`, + ), + ); + } else { + log.warn( + `typeof state.TokensController.allIgnoredTokens is ${typeof tokensControllerState.allIgnoredTokens}`, + ); } // Migrate TokensController.allDetectedTokens @@ -188,9 +252,25 @@ function migrateData(state: Record): void { allDetectedTokens, (_, chainId: string) => toHex(chainId), ); + } else if (hasProperty(tokensControllerState, 'allDetectedTokens')) { + global.sentry?.captureException?.( + new Error( + `typeof state.TokensController.allDetectedTokens is ${typeof tokensControllerState.allDetectedTokens}`, + ), + ); + } else { + log.warn( + `typeof state.TokensController.allDetectedTokens is ${typeof tokensControllerState.allDetectedTokens}`, + ); } state.TokensController = tokensControllerState; + } else { + global.sentry?.captureException?.( + new Error( + `typeof state.TokensController is ${typeof state.TokensController}`, + ), + ); } } diff --git a/app/scripts/migrations/089.test.ts b/app/scripts/migrations/089.test.ts index 00868ff74..91511d315 100644 --- a/app/scripts/migrations/089.test.ts +++ b/app/scripts/migrations/089.test.ts @@ -1,5 +1,14 @@ import { migrate, version } from './089'; +const sentryCaptureExceptionMock = jest.fn(); + +global.sentry = { + startSession: jest.fn(), + endSession: jest.fn(), + toggleSession: jest.fn(), + captureException: sentryCaptureExceptionMock, +}; + jest.mock('uuid', () => { const actual = jest.requireActual('uuid'); @@ -10,6 +19,10 @@ jest.mock('uuid', () => { }); describe('migration #89', () => { + afterEach(() => { + jest.resetAllMocks(); + }); + it('should update the version metadata', async () => { const oldStorage = { meta: { @@ -39,6 +52,25 @@ describe('migration #89', () => { expect(newStorage.data).toStrictEqual(oldData); }); + it('should capture an exception if there is no network controller state', async () => { + const oldData = { + other: 'data', + }; + const oldStorage = { + meta: { + version: 88, + }, + data: oldData, + }; + + await migrate(oldStorage); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.NetworkController is undefined`), + ); + }); + it('should return state unaltered if there is no network controller providerConfig state', async () => { const oldData = { other: 'data', @@ -61,6 +93,32 @@ describe('migration #89', () => { expect(newStorage.data).toStrictEqual(oldData); }); + it('should capture an exception if there is no network controller providerConfig state', async () => { + const oldData = { + other: 'data', + NetworkController: { + networkConfigurations: { + id1: { + foo: 'bar', + }, + }, + }, + }; + const oldStorage = { + meta: { + version: 88, + }, + data: oldData, + }; + + await migrate(oldStorage); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.NetworkController.providerConfig is undefined`), + ); + }); + it('should return state unaltered if the providerConfig already has an id', async () => { const oldData = { other: 'data', diff --git a/app/scripts/migrations/089.ts b/app/scripts/migrations/089.ts index cc1bfa4dc..d4faebc22 100644 --- a/app/scripts/migrations/089.ts +++ b/app/scripts/migrations/089.ts @@ -66,6 +66,19 @@ function transformState(state: Record) { ...state, NetworkController: state.NetworkController, }; + } else if (!isObject(state.NetworkController)) { + global.sentry?.captureException?.( + new Error( + `typeof state.NetworkController is ${typeof state.NetworkController}`, + ), + ); + } else if (!isObject(state.NetworkController.providerConfig)) { + global.sentry?.captureException?.( + new Error( + `typeof state.NetworkController.providerConfig is ${typeof state + .NetworkController.providerConfig}`, + ), + ); } return state; } diff --git a/app/scripts/migrations/090.test.js b/app/scripts/migrations/090.test.js index 6a28c60f2..13e39e648 100644 --- a/app/scripts/migrations/090.test.js +++ b/app/scripts/migrations/090.test.js @@ -2,7 +2,17 @@ import { migrate, version } from './090'; const PREVIOUS_VERSION = version - 1; +const sentryCaptureExceptionMock = jest.fn(); + +global.sentry = { + captureException: sentryCaptureExceptionMock, +}; + describe('migration #90', () => { + afterEach(() => { + jest.resetAllMocks(); + }); + it('updates the version metadata', async () => { const oldStorage = { meta: { @@ -31,7 +41,7 @@ describe('migration #90', () => { expect(newStorage.data).toStrictEqual(oldStorage.data); }); - it('does not change the state if the phishing controller state is invalid', async () => { + it('captures an exception if the phishing controller state is invalid', async () => { const oldStorage = { meta: { version: PREVIOUS_VERSION, @@ -39,9 +49,12 @@ describe('migration #90', () => { data: { PhishingController: 'this is not valid' }, }; - const newStorage = await migrate(oldStorage); + await migrate(oldStorage); - expect(newStorage.data).toStrictEqual(oldStorage.data); + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.PhishingController is string`), + ); }); it('does not change the state if the listState property does not exist', async () => { diff --git a/app/scripts/migrations/090.ts b/app/scripts/migrations/090.ts index e45ec05e4..a2d50cab6 100644 --- a/app/scripts/migrations/090.ts +++ b/app/scripts/migrations/090.ts @@ -1,5 +1,6 @@ import { cloneDeep } from 'lodash'; import { hasProperty, isObject } from '@metamask/utils'; +import log from 'loglevel'; export const version = 90; @@ -23,11 +24,22 @@ export async function migrate(originalVersionedData: { } function transformState(state: Record) { - if ( - !hasProperty(state, 'PhishingController') || - !isObject(state.PhishingController) || - !hasProperty(state.PhishingController, 'listState') - ) { + if (!hasProperty(state, 'PhishingController')) { + log.warn(`typeof state.PhishingController is undefined`); + return state; + } + if (!isObject(state.PhishingController)) { + global.sentry?.captureException?.( + new Error( + `typeof state.PhishingController is ${typeof state.PhishingController}`, + ), + ); + return state; + } + if (!hasProperty(state.PhishingController, 'listState')) { + log.warn( + `typeof state.PhishingController.listState is ${typeof state.PhishingController}`, + ); return state; } diff --git a/app/scripts/migrations/091.test.ts b/app/scripts/migrations/091.test.ts index d4836f003..6a1f14ed7 100644 --- a/app/scripts/migrations/091.test.ts +++ b/app/scripts/migrations/091.test.ts @@ -1,6 +1,15 @@ import { cloneDeep } from 'lodash'; import { migrate, version } from './091'; +const sentryCaptureExceptionMock = jest.fn(); + +global.sentry = { + startSession: jest.fn(), + endSession: jest.fn(), + toggleSession: jest.fn(), + captureException: sentryCaptureExceptionMock, +}; + jest.mock('uuid', () => { const actual = jest.requireActual('uuid'); @@ -11,6 +20,10 @@ jest.mock('uuid', () => { }); describe('migration #91', () => { + afterEach(() => { + jest.resetAllMocks(); + }); + it('should update the version metadata', async () => { const oldStorage = { meta: { @@ -40,6 +53,25 @@ describe('migration #91', () => { expect(newStorage.data).toStrictEqual(oldData); }); + it('should capture an exception if there is no network controller state', async () => { + const oldData = { + other: 'data', + }; + const oldStorage = { + meta: { + version: 90, + }, + data: oldData, + }; + + await migrate(oldStorage); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.NetworkController is undefined`), + ); + }); + it('should return state unaltered if there is no network controller networkConfigurations state', async () => { const oldData = { other: 'data', @@ -60,6 +92,32 @@ describe('migration #91', () => { expect(newStorage.data).toStrictEqual(oldData); }); + it('should capture an exception if there is no network controller networkConfigurations state', async () => { + const oldData = { + other: 'data', + NetworkController: { + providerConfig: { + foo: 'bar', + }, + }, + }; + const oldStorage = { + meta: { + version: 90, + }, + data: oldData, + }; + + await migrate(oldStorage); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error( + `typeof state.NetworkController.networkConfigurations is undefined`, + ), + ); + }); + it('should return state unaltered if the networkConfigurations all have a chainId', async () => { const oldData = { other: 'data', diff --git a/app/scripts/migrations/091.ts b/app/scripts/migrations/091.ts index c0661746a..874f6ed9f 100644 --- a/app/scripts/migrations/091.ts +++ b/app/scripts/migrations/091.ts @@ -50,6 +50,19 @@ function transformState(state: Record) { ...state, NetworkController: state.NetworkController, }; + } else if (!isObject(state.NetworkController)) { + global.sentry?.captureException?.( + new Error( + `typeof state.NetworkController is ${typeof state.NetworkController}`, + ), + ); + } else if (!isObject(state.NetworkController.networkConfigurations)) { + global.sentry?.captureException?.( + new Error( + `typeof state.NetworkController.networkConfigurations is ${typeof state + .NetworkController.networkConfigurations}`, + ), + ); } return state; } diff --git a/app/scripts/migrations/092.test.ts b/app/scripts/migrations/092.test.ts index b44c04602..b7337c871 100644 --- a/app/scripts/migrations/092.test.ts +++ b/app/scripts/migrations/092.test.ts @@ -3,7 +3,20 @@ import { migrate, version } from './092'; const PREVIOUS_VERSION = version - 1; +const sentryCaptureExceptionMock = jest.fn(); + +global.sentry = { + startSession: jest.fn(), + endSession: jest.fn(), + toggleSession: jest.fn(), + captureException: sentryCaptureExceptionMock, +}; + describe('migration #92', () => { + afterEach(() => { + jest.resetAllMocks(); + }); + it('should update the version metadata', async () => { const oldStorage = { meta: { @@ -33,6 +46,22 @@ describe('migration #92', () => { expect(newStorage.data).toStrictEqual(oldData); }); + it('captures an exception if the phishing controller state is invalid', async () => { + const oldStorage = { + meta: { + version: PREVIOUS_VERSION, + }, + data: { PhishingController: 'this is not valid' }, + }; + + await migrate(oldStorage); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.PhishingController is string`), + ); + }); + it('should return state unaltered if there is no phishing controller last fetched state', async () => { const oldData = { other: 'data', diff --git a/app/scripts/migrations/092.ts b/app/scripts/migrations/092.ts index bf5469614..44ef2a6c7 100644 --- a/app/scripts/migrations/092.ts +++ b/app/scripts/migrations/092.ts @@ -1,5 +1,6 @@ import { cloneDeep } from 'lodash'; import { hasProperty, isObject } from '@metamask/utils'; +import log from 'loglevel'; export const version = 92; @@ -30,6 +31,14 @@ function transformState(state: Record) { ) { delete state.PhishingController.stalelistLastFetched; delete state.PhishingController.hotlistLastFetched; + } else if (hasProperty(state, 'PhishingController')) { + global.sentry?.captureException?.( + new Error( + `typeof state.PhishingController is ${typeof state.PhishingController}`, + ), + ); + } else { + log.warn(`typeof state.PhishingController is undefined`); } return state; } diff --git a/types/global.d.ts b/types/global.d.ts index f1e8c1b18..a288e0f77 100644 --- a/types/global.d.ts +++ b/types/global.d.ts @@ -1,6 +1,7 @@ // In order for variables to be considered on the global scope they must be // declared using var and not const or let, which is why this rule is disabled /* eslint-disable no-var */ +import * as Sentry from '@sentry/browser'; declare class Platform { openTab: (opts: { url: string }) => void; @@ -11,6 +12,9 @@ declare class Platform { export declare global { var platform: Platform; + // Sentry is undefined in dev, so use optional chaining + var sentry: Sentry | undefined; + namespace jest { interface Matchers { toBeFulfilled(): Promise; From dc7ebe979eaea281c23f47f85b9b49e1edcaac61 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Wed, 16 Aug 2023 19:45:31 -0230 Subject: [PATCH 11/23] Add additional validation for persisted state metadata (#20462) Additional validation has been added for persisted state metadata. Beforehand we just checked that the state itself wasn't falsy. Now we ensure that the metadata is an object with a valid version as well. --- app/scripts/background.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/app/scripts/background.js b/app/scripts/background.js index c368728d6..f4e46c9f0 100644 --- a/app/scripts/background.js +++ b/app/scripts/background.js @@ -13,6 +13,7 @@ import debounce from 'debounce-stream'; import log from 'loglevel'; import browser from 'webextension-polyfill'; import { storeAsStream } from '@metamask/obs-store'; +import { isObject } from '@metamask/utils'; ///: BEGIN:ONLY_INCLUDE_IN(snaps) import { ApprovalType } from '@metamask/controller-utils'; ///: END:ONLY_INCLUDE_IN @@ -411,6 +412,19 @@ export async function loadStateFromPersistence() { versionedData = await migrator.migrateData(versionedData); if (!versionedData) { throw new Error('MetaMask - migrator returned undefined'); + } else if (!isObject(versionedData.meta)) { + throw new Error( + `MetaMask - migrator metadata has invalid type '${typeof versionedData.meta}'`, + ); + } else if (typeof versionedData.meta.version !== 'number') { + throw new Error( + `MetaMask - migrator metadata version has invalid type '${typeof versionedData + .meta.version}'`, + ); + } else if (!isObject(versionedData.data)) { + throw new Error( + `MetaMask - migrator data has invalid type '${typeof versionedData.data}'`, + ); } // this initializes the meta/version data as a class variable to be used for future writes localStore.setMetadata(versionedData.meta); From 805ce29e11a096e7adb755b05498a221dc2a9f2e Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Wed, 16 Aug 2023 21:11:17 -0230 Subject: [PATCH 12/23] Add types of hidden properties to Sentry data (#20457) * Add types of hidden properties to Sentry data The masked wallet state object sent to Sentry has been updated to include the type of each property omitted from the mask. This lets us at least see the full state shape, making it easier to see when errors are caused by invalid state. Relates to #20449 * Remove inconsistent state snapshot properties The state snapshot tests have been updated to exclude properties that were shown to differ between runs. --- shared/modules/object.utils.js | 4 + test/e2e/tests/errors.spec.js | 60 +++++++--- ...rs-after-init-opt-in-background-state.json | 63 ++++++++-- .../errors-after-init-opt-in-ui-state.json | 110 +++++++++++++++++- 4 files changed, 211 insertions(+), 26 deletions(-) diff --git a/shared/modules/object.utils.js b/shared/modules/object.utils.js index 24119be56..ce7eb1da1 100644 --- a/shared/modules/object.utils.js +++ b/shared/modules/object.utils.js @@ -7,6 +7,8 @@ * should be included, and a sub-mask implies the property should be further * masked according to that sub-mask. * + * If a property is not found in the last, its type is included instead. + * * @param {object} object - The object to mask * @param {Object} mask - The mask to apply to the object */ @@ -16,6 +18,8 @@ export function maskObject(object, mask) { state[key] = object[key]; } else if (mask[key]) { state[key] = maskObject(object[key], mask[key]); + } else { + state[key] = typeof object[key]; } return state; }, {}); diff --git a/test/e2e/tests/errors.spec.js b/test/e2e/tests/errors.spec.js index 8784ef168..3098a5083 100644 --- a/test/e2e/tests/errors.spec.js +++ b/test/e2e/tests/errors.spec.js @@ -1,42 +1,72 @@ const { resolve } = require('path'); const { promises: fs } = require('fs'); const { strict: assert } = require('assert'); -const { get, has, set } = require('lodash'); +const { get, has, set, unset } = require('lodash'); const { Browser } = require('selenium-webdriver'); const { format } = require('prettier'); const { convertToHexValue, withFixtures } = require('../helpers'); const FixtureBuilder = require('../fixture-builder'); -const backgroundDateFields = ['CurrencyController.conversionDate']; -const uiDateFields = ['metamask.conversionDate']; +const maskedBackgroundFields = [ + 'CurrencyController.conversionDate', // This is a timestamp that changes each run +]; +const maskedUiFields = [ + 'metamask.conversionDate', // This is a timestamp that changes each run +]; + +const removedBackgroundFields = [ + // This property is timing-dependent + 'AccountTracker.currentBlockGasLimit', + // These properties are set to undefined, causing inconsistencies between Chrome and Firefox + 'AppStateController.currentPopupId', + 'AppStateController.timeoutMinutes', + 'TokenListController.tokensChainsCache', +]; + +const removedUiFields = [ + // This property is timing-dependent + 'metamask.currentBlockGasLimit', + // These properties are set to undefined, causing inconsistencies between Chrome and Firefox + 'metamask.currentPopupId', + 'metamask.timeoutMinutes', + 'metamask.tokensChainsCache', +]; /** - * Transform date properties to value types, to ensure that state is - * consistent between test runs. + * Transform background state to make it consistent between test runs. * * @param {unknown} data - The data to transform */ -function transformBackgroundDates(data) { - for (const field of backgroundDateFields) { +function transformBackgroundState(data) { + for (const field of maskedBackgroundFields) { if (has(data, field)) { set(data, field, typeof get(data, field)); } } + for (const field of removedBackgroundFields) { + if (has(data, field)) { + unset(data, field); + } + } return data; } /** - * Transform date properties to value types, to ensure that state is - * consistent between test runs. + * Transform UI state to make it consistent between test runs. * * @param {unknown} data - The data to transform */ -function transformUiDates(data) { - for (const field of uiDateFields) { +function transformUiState(data) { + for (const field of maskedUiFields) { if (has(data, field)) { set(data, field, typeof get(data, field)); } } + for (const field of removedUiFields) { + if (has(data, field)) { + unset(data, field); + } + } return data; } @@ -257,7 +287,7 @@ describe('Sentry errors', function () { const mockJsonBody = JSON.parse(mockTextBody[2]); const appState = mockJsonBody?.extra?.appState; await matchesSnapshot({ - data: transformBackgroundDates(appState), + data: transformBackgroundState(appState), snapshot: 'errors-before-init-opt-in-background-state', }); }, @@ -342,7 +372,7 @@ describe('Sentry errors', function () { const mockJsonBody = JSON.parse(mockTextBody[2]); const appState = mockJsonBody?.extra?.appState; await matchesSnapshot({ - data: transformUiDates(appState), + data: transformUiState(appState), snapshot: 'errors-before-init-opt-in-ui-state', }); }, @@ -509,7 +539,7 @@ describe('Sentry errors', function () { 'Invalid version state', ); await matchesSnapshot({ - data: transformBackgroundDates(appState.store), + data: transformBackgroundState(appState.store), snapshot: 'errors-after-init-opt-in-background-state', }); }, @@ -603,7 +633,7 @@ describe('Sentry errors', function () { 'Invalid version state', ); await matchesSnapshot({ - data: transformUiDates(appState.store), + data: transformUiState(appState.store), snapshot: 'errors-after-init-opt-in-ui-state', }); }, diff --git a/test/e2e/tests/state-snapshots/errors-after-init-opt-in-background-state.json b/test/e2e/tests/state-snapshots/errors-after-init-opt-in-background-state.json index f41fee885..2f29cc98c 100644 --- a/test/e2e/tests/state-snapshots/errors-after-init-opt-in-background-state.json +++ b/test/e2e/tests/state-snapshots/errors-after-init-opt-in-background-state.json @@ -1,34 +1,81 @@ { - "AccountTracker": { "currentBlockGasLimit": "0x1c9c380" }, + "AccountTracker": { "accounts": "object" }, "AppStateController": { "connectedStatusPopoverHasBeenShown": true, - "defaultHomeActiveTabName": null + "defaultHomeActiveTabName": null, + "browserEnvironment": "object", + "popupGasPollTokens": "object", + "notificationGasPollTokens": "object", + "fullScreenGasPollTokens": "object", + "recoveryPhraseReminderHasBeenShown": "boolean", + "recoveryPhraseReminderLastShown": "number", + "outdatedBrowserWarningLastShown": "number", + "nftsDetectionNoticeDismissed": "boolean", + "showTestnetMessageInDropdown": "boolean", + "showBetaHeader": "boolean", + "showProductTour": "boolean", + "trezorModel": "object", + "nftsDropdownState": "object", + "termsOfUseLastAgreed": "number", + "qrHardware": "object", + "usedNetworks": "object", + "snapsInstallPrivacyWarningShown": "boolean", + "serviceWorkerLastActiveTime": "number" }, + "ApprovalController": "object", + "CachedBalancesController": "object", "CurrencyController": { "conversionDate": "number", "conversionRate": 1700, "nativeCurrency": "ETH", - "currentCurrency": "usd" + "currentCurrency": "usd", + "pendingCurrentCurrency": "object", + "pendingNativeCurrency": "object", + "usdConversionRate": "number" + }, + "DecryptMessageController": { + "unapprovedDecryptMsgs": "object", + "unapprovedDecryptMsgCount": 0 }, - "DecryptMessageController": { "unapprovedDecryptMsgCount": 0 }, "EncryptionPublicKeyController": { + "unapprovedEncryptionPublicKeyMsgs": "object", "unapprovedEncryptionPublicKeyMsgCount": 0 }, + "EnsController": "object", "MetaMetricsController": { "participateInMetaMetrics": true, - "metaMetricsId": "fake-metrics-id" + "metaMetricsId": "fake-metrics-id", + "eventsBeforeMetricsOptIn": "object", + "traits": "object", + "fragments": "object", + "segmentApiCalls": "object", + "previousUserTraits": "object" }, "NetworkController": { + "selectedNetworkClientId": "string", "networkId": "1337", "providerConfig": { + "chainId": "string", "nickname": "Localhost 8545", + "rpcPrefs": "object", + "rpcUrl": "string", "ticker": "ETH", - "type": "rpc" - } + "type": "rpc", + "id": "string" + }, + "networksMetadata": "object", + "networkConfigurations": "object" }, "SignatureController": { + "unapprovedMsgs": "object", + "unapprovedPersonalMsgs": "object", + "unapprovedTypedMessages": "object", "unapprovedMsgCount": 0, "unapprovedPersonalMsgCount": 0, "unapprovedTypedMessagesCount": 0 - } + }, + "SwapsController": "object", + "TokenRatesController": "object", + "TokensController": "object", + "TxController": "object" } diff --git a/test/e2e/tests/state-snapshots/errors-after-init-opt-in-ui-state.json b/test/e2e/tests/state-snapshots/errors-after-init-opt-in-ui-state.json index 4ecaa0417..129ae885a 100644 --- a/test/e2e/tests/state-snapshots/errors-after-init-opt-in-ui-state.json +++ b/test/e2e/tests/state-snapshots/errors-after-init-opt-in-ui-state.json @@ -1,16 +1,28 @@ { + "DNS": "object", + "activeTab": "object", + "appState": "object", + "confirmTransaction": "object", "gas": { "customData": { "price": null, "limit": null } }, "history": { "mostRecentOverviewPage": "/" }, + "invalidCustomNetwork": "object", + "localeMessages": "object", "metamask": { "isInitialized": true, "isUnlocked": false, "isAccountMenuOpen": false, + "isNetworkMenuOpen": "boolean", + "identities": "object", + "unapprovedTxs": "object", + "networkConfigurations": "object", + "addressBook": "object", + "contractExchangeRates": "object", + "pendingTokens": "object", "customNonceValue": "", "useBlockie": false, "featureFlags": { "showIncomingTransactions": true }, "welcomeScreenSeen": false, "currentLocale": "en", - "currentBlockGasLimit": "", "preferences": { "hideZeroBalanceTokens": false, "showFiatInTestnets": false, @@ -19,31 +31,89 @@ }, "firstTimeFlowType": "import", "completedOnboarding": true, + "knownMethodData": "object", + "use4ByteResolution": "boolean", "participateInMetaMetrics": true, "nextNonce": null, "conversionRate": 1300, "nativeCurrency": "ETH", "connectedStatusPopoverHasBeenShown": true, "defaultHomeActiveTabName": null, + "browserEnvironment": "object", + "popupGasPollTokens": "object", + "notificationGasPollTokens": "object", + "fullScreenGasPollTokens": "object", + "recoveryPhraseReminderHasBeenShown": "boolean", + "recoveryPhraseReminderLastShown": "number", + "outdatedBrowserWarningLastShown": "number", + "nftsDetectionNoticeDismissed": "boolean", + "showTestnetMessageInDropdown": "boolean", + "showBetaHeader": "boolean", + "showProductTour": "boolean", + "trezorModel": "object", + "nftsDropdownState": "object", + "termsOfUseLastAgreed": "number", + "qrHardware": "object", + "usedNetworks": "object", + "snapsInstallPrivacyWarningShown": "boolean", + "serviceWorkerLastActiveTime": "number", "currentAppVersion": "10.34.4", "previousAppVersion": "", "previousMigrationVersion": 0, "currentMigrationVersion": 94, + "selectedNetworkClientId": "string", "networkId": "1337", "providerConfig": { + "chainId": "string", "nickname": "Localhost 8545", + "rpcPrefs": "object", + "rpcUrl": "string", "ticker": "ETH", - "type": "rpc" + "type": "rpc", + "id": "string" }, + "networksMetadata": "object", + "cachedBalances": "object", + "keyringTypes": "object", + "keyrings": "object", "useNonceField": false, "usePhishDetect": true, + "dismissSeedBackUpReminder": "boolean", + "disabledRpcMethodPreferences": "object", + "useMultiAccountBalanceChecker": "boolean", + "useTokenDetection": "boolean", + "useNftDetection": "boolean", + "useCurrencyRateCheck": "boolean", + "openSeaEnabled": "boolean", + "advancedGasFee": "object", + "lostIdentities": "object", "forgottenPassword": false, "ipfsGateway": "dweb.link", + "useAddressBarEnsResolution": "boolean", + "infuraBlocked": "boolean", + "ledgerTransportType": "string", + "snapRegistryList": "object", + "transactionSecurityCheckEnabled": "boolean", + "theme": "string", + "isLineaMainnetReleased": "boolean", + "selectedAddress": "string", "metaMetricsId": "fake-metrics-id", + "eventsBeforeMetricsOptIn": "object", + "traits": "object", + "fragments": "object", + "segmentApiCalls": "object", + "previousUserTraits": "object", "conversionDate": "number", "currentCurrency": "usd", + "pendingCurrentCurrency": "object", + "pendingNativeCurrency": "object", + "usdConversionRate": "number", "alertEnabledness": { "unconnectedAccount": true, "web3ShimUsage": true }, + "unconnectedAccountAlertShownOrigins": "object", + "web3ShimUsageOrigins": "object", "seedPhraseBackedUp": true, + "onboardingTabs": "object", + "incomingTransactions": "object", "incomingTxLastFetchedBlockByChainId": { "0x1": null, "0xe708": null, @@ -51,11 +121,45 @@ "0xaa36a7": null, "0xe704": null }, + "subjects": "object", + "permissionHistory": "object", + "permissionActivityLog": "object", + "subjectMetadata": "object", + "announcements": "object", + "gasFeeEstimates": "object", + "estimatedGasFeeTimeBounds": "object", + "gasEstimateType": "string", + "tokenList": "object", + "preventPollingOnNetworkRestart": "boolean", + "tokens": "object", + "ignoredTokens": "object", + "detectedTokens": "object", + "allTokens": "object", + "allIgnoredTokens": "object", + "allDetectedTokens": "object", + "smartTransactionsState": "object", + "allNftContracts": "object", + "allNfts": "object", + "ignoredNfts": "object", + "accounts": "object", + "currentNetworkTxList": "object", + "unapprovedDecryptMsgs": "object", "unapprovedDecryptMsgCount": 0, + "unapprovedEncryptionPublicKeyMsgs": "object", "unapprovedEncryptionPublicKeyMsgCount": 0, + "unapprovedMsgs": "object", + "unapprovedPersonalMsgs": "object", + "unapprovedTypedMessages": "object", "unapprovedMsgCount": 0, "unapprovedPersonalMsgCount": 0, - "unapprovedTypedMessagesCount": 0 + "unapprovedTypedMessagesCount": 0, + "swapsState": "object", + "ensResolutionsByAddress": "object", + "pendingApprovals": "object", + "pendingApprovalCount": "number", + "approvalFlows": "object" }, + "send": "object", + "swaps": "object", "unconnectedAccount": { "state": "CLOSED" } } From 80746e67b50219f12d736f276c94063b79043d25 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Thu, 17 Aug 2023 09:29:05 -0230 Subject: [PATCH 13/23] Improve Sentry state pre-initialization (#20491) * Improve Sentry state pre-initialization Previously the masked state snapshot sent to Sentry would be blank for errors that occured during initialization. Instead we'll now include some basic information in all cases, and a masked copy of the persisted state if it happens after the first time the persisted state is read. * Add test * Fix crash when persisted state not yet fetched * Add descriptions for initial state hooks * Update comments to reflect recent changes * Re-order imports to follow conventions * Move initial state hooks back to module-level The initial state hooks are now setup at the top-level of their module. This ensures that they're setup prior to later imports. Calling a function to setup these hooks in the entrypoint module wouldn't accomplish this even if it was run "before" the imports because ES6 imports always get hoisted to the top of the file. The `localStore` instance wasn't available statically, so a new state hook was introduced for retrieving the most recent retrieved persisted state. * Fix error e2e tests --- app/scripts/background.js | 27 ++--- app/scripts/lib/local-store.js | 3 + app/scripts/lib/local-store.test.js | 57 +++++++-- app/scripts/lib/network-store.js | 2 + app/scripts/lib/setup-initial-state-hooks.js | 61 ++++++++++ app/scripts/lib/setup-persisted-state-hook.js | 10 -- app/scripts/ui.js | 18 ++- test/e2e/tests/errors.spec.js | 58 ++++++--- ...s-before-init-opt-in-background-state.json | 112 ++++++++++++++++- .../errors-before-init-opt-in-ui-state.json | 113 +++++++++++++++++- ui/index.js | 9 +- 11 files changed, 408 insertions(+), 62 deletions(-) create mode 100644 app/scripts/lib/setup-initial-state-hooks.js delete mode 100644 app/scripts/lib/setup-persisted-state-hook.js diff --git a/app/scripts/background.js b/app/scripts/background.js index f4e46c9f0..7e89ce1c3 100644 --- a/app/scripts/background.js +++ b/app/scripts/background.js @@ -2,9 +2,11 @@ * @file The entry point for the web extension singleton process. */ -// This import sets up a global function required for Sentry to function. +// Disabled to allow setting up initial state hooks first + +// This import sets up global functions required for Sentry to function. // It must be run first in case an error is thrown later during initialization. -import './lib/setup-persisted-state-hook'; +import './lib/setup-initial-state-hooks'; import EventEmitter from 'events'; import endOfStream from 'end-of-stream'; @@ -69,6 +71,12 @@ import DesktopManager from '@metamask/desktop/dist/desktop-manager'; ///: END:ONLY_INCLUDE_IN /* eslint-enable import/order */ +// Setup global hook for improved Sentry state snapshots during initialization +const inTest = process.env.IN_TEST; +const localStore = inTest ? new ReadOnlyNetworkStore() : new LocalStore(); +global.stateHooks.getMostRecentPersistedState = () => + localStore.mostRecentRetrievedState; + const { sentry } = global; const firstTimeState = { ...rawFirstTimeState }; @@ -92,9 +100,6 @@ const openMetamaskTabsIDs = {}; const requestAccountTabIds = {}; let controller; -// state persistence -const inTest = process.env.IN_TEST; -const localStore = inTest ? new ReadOnlyNetworkStore() : new LocalStore(); let versionedData; if (inTest || process.env.METAMASK_DEBUG) { @@ -889,17 +894,9 @@ browser.runtime.onInstalled.addListener(({ reason }) => { }); function setupSentryGetStateGlobal(store) { - global.stateHooks.getSentryState = function () { + global.stateHooks.getSentryAppState = function () { const backgroundState = store.memStore.getState(); - const maskedBackgroundState = maskObject( - backgroundState, - SENTRY_BACKGROUND_STATE, - ); - return { - browser: window.navigator.userAgent, - store: maskedBackgroundState, - version: platform.getVersion(), - }; + return maskObject(backgroundState, SENTRY_BACKGROUND_STATE); }; } diff --git a/app/scripts/lib/local-store.js b/app/scripts/lib/local-store.js index b0a223561..40908af5e 100644 --- a/app/scripts/lib/local-store.js +++ b/app/scripts/lib/local-store.js @@ -16,6 +16,7 @@ export default class ExtensionStore { // once data persistence fails once and it flips true we don't send further // data persistence errors to sentry this.dataPersistenceFailing = false; + this.mostRecentRetrievedState = null; } setMetadata(initMetaData) { @@ -66,8 +67,10 @@ export default class ExtensionStore { // extension.storage.local always returns an obj // if the object is empty, treat it as undefined if (isEmpty(result)) { + this.mostRecentRetrievedState = null; return undefined; } + this.mostRecentRetrievedState = result; return result; } diff --git a/app/scripts/lib/local-store.test.js b/app/scripts/lib/local-store.test.js index 2c3cea405..8b786ca81 100644 --- a/app/scripts/lib/local-store.test.js +++ b/app/scripts/lib/local-store.test.js @@ -2,11 +2,12 @@ import browser from 'webextension-polyfill'; import LocalStore from './local-store'; jest.mock('webextension-polyfill', () => ({ + runtime: { lastError: null }, storage: { local: true }, })); -const setup = ({ isSupported }) => { - browser.storage.local = isSupported; +const setup = ({ localMock = jest.fn() } = {}) => { + browser.storage.local = localMock; return new LocalStore(); }; describe('LocalStore', () => { @@ -15,21 +16,27 @@ describe('LocalStore', () => { }); describe('contructor', () => { it('should set isSupported property to false when browser does not support local storage', () => { - const localStore = setup({ isSupported: false }); + const localStore = setup({ localMock: false }); expect(localStore.isSupported).toBe(false); }); it('should set isSupported property to true when browser supports local storage', () => { - const localStore = setup({ isSupported: true }); + const localStore = setup(); expect(localStore.isSupported).toBe(true); }); + + it('should initialize mostRecentRetrievedState to null', () => { + const localStore = setup({ localMock: false }); + + expect(localStore.mostRecentRetrievedState).toBeNull(); + }); }); describe('setMetadata', () => { it('should set the metadata property on LocalStore', () => { const metadata = { version: 74 }; - const localStore = setup({ isSupported: true }); + const localStore = setup(); localStore.setMetadata(metadata); expect(localStore.metadata).toStrictEqual(metadata); @@ -38,21 +45,21 @@ describe('LocalStore', () => { describe('set', () => { it('should throw an error if called in a browser that does not support local storage', async () => { - const localStore = setup({ isSupported: false }); + const localStore = setup({ localMock: false }); await expect(() => localStore.set()).rejects.toThrow( 'Metamask- cannot persist state to local store as this browser does not support this action', ); }); it('should throw an error if not passed a truthy value as an argument', async () => { - const localStore = setup({ isSupported: true }); + const localStore = setup(); await expect(() => localStore.set()).rejects.toThrow( 'MetaMask - updated state is missing', ); }); it('should throw an error if passed a valid argument but metadata has not yet been set', async () => { - const localStore = setup({ isSupported: true }); + const localStore = setup(); await expect(() => localStore.set({ appState: { test: true } }), ).rejects.toThrow( @@ -61,7 +68,7 @@ describe('LocalStore', () => { }); it('should not throw if passed a valid argument and metadata has been set', async () => { - const localStore = setup({ isSupported: true }); + const localStore = setup(); localStore.setMetadata({ version: 74 }); await expect(async function () { localStore.set({ appState: { test: true } }); @@ -71,9 +78,39 @@ describe('LocalStore', () => { describe('get', () => { it('should return undefined if called in a browser that does not support local storage', async () => { - const localStore = setup({ isSupported: false }); + const localStore = setup({ localMock: false }); const result = await localStore.get(); expect(result).toStrictEqual(undefined); }); + + it('should update mostRecentRetrievedState', async () => { + const localStore = setup({ + localMock: { + get: jest + .fn() + .mockImplementation(() => + Promise.resolve({ appState: { test: true } }), + ), + }, + }); + + await localStore.get(); + + expect(localStore.mostRecentRetrievedState).toStrictEqual({ + appState: { test: true }, + }); + }); + + it('should reset mostRecentRetrievedState to null if storage.local is empty', async () => { + const localStore = setup({ + localMock: { + get: jest.fn().mockImplementation(() => Promise.resolve({})), + }, + }); + + await localStore.get(); + + expect(localStore.mostRecentRetrievedState).toStrictEqual(null); + }); }); }); diff --git a/app/scripts/lib/network-store.js b/app/scripts/lib/network-store.js index ea6ba5876..2f4c0a1b0 100644 --- a/app/scripts/lib/network-store.js +++ b/app/scripts/lib/network-store.js @@ -15,6 +15,7 @@ export default class ReadOnlyNetworkStore { this._initialized = false; this._initializing = this._init(); this._state = undefined; + this.mostRecentRetrievedState = null; } /** @@ -30,6 +31,7 @@ export default class ReadOnlyNetworkStore { const response = await fetchWithTimeout(FIXTURE_SERVER_URL); if (response.ok) { this._state = await response.json(); + this.mostRecentRetrievedState = this._state; } } catch (error) { log.debug(`Error loading network state: '${error.message}'`); diff --git a/app/scripts/lib/setup-initial-state-hooks.js b/app/scripts/lib/setup-initial-state-hooks.js new file mode 100644 index 000000000..fa02987a7 --- /dev/null +++ b/app/scripts/lib/setup-initial-state-hooks.js @@ -0,0 +1,61 @@ +import { maskObject } from '../../../shared/modules/object.utils'; +import ExtensionPlatform from '../platforms/extension'; +import LocalStore from './local-store'; +import ReadOnlyNetworkStore from './network-store'; +import { SENTRY_BACKGROUND_STATE } from './setupSentry'; + +const platform = new ExtensionPlatform(); +const localStore = process.env.IN_TEST + ? new ReadOnlyNetworkStore() + : new LocalStore(); + +/** + * Get the persisted wallet state. + * + * @returns The persisted wallet state. + */ +globalThis.stateHooks.getPersistedState = async function () { + return await localStore.get(); +}; + +const persistedStateMask = { + data: SENTRY_BACKGROUND_STATE, + meta: { + version: true, + }, +}; + +/** + * Get a state snapshot to include with Sentry error reports. This uses the + * persisted state pre-initialization, and the in-memory state post- + * initialization. In both cases the state is anonymized. + * + * @returns A Sentry state snapshot. + */ +globalThis.stateHooks.getSentryState = function () { + const sentryState = { + browser: window.navigator.userAgent, + version: platform.getVersion(), + }; + if (globalThis.stateHooks.getSentryAppState) { + return { + ...sentryState, + state: globalThis.stateHooks.getSentryAppState(), + }; + } else if (globalThis.stateHooks.getMostRecentPersistedState) { + const persistedState = globalThis.stateHooks.getMostRecentPersistedState(); + if (persistedState) { + return { + ...sentryState, + persistedState: maskObject( + // `getMostRecentPersistedState` is used here instead of + // `getPersistedState` to avoid making this an asynchronous function. + globalThis.stateHooks.getMostRecentPersistedState(), + persistedStateMask, + ), + }; + } + return sentryState; + } + return sentryState; +}; diff --git a/app/scripts/lib/setup-persisted-state-hook.js b/app/scripts/lib/setup-persisted-state-hook.js deleted file mode 100644 index 9b29fad26..000000000 --- a/app/scripts/lib/setup-persisted-state-hook.js +++ /dev/null @@ -1,10 +0,0 @@ -import LocalStore from './local-store'; -import ReadOnlyNetworkStore from './network-store'; - -const localStore = process.env.IN_TEST - ? new ReadOnlyNetworkStore() - : new LocalStore(); - -globalThis.stateHooks.getPersistedState = async function () { - return await localStore.get(); -}; diff --git a/app/scripts/ui.js b/app/scripts/ui.js index 565e8187a..aeb93cacb 100644 --- a/app/scripts/ui.js +++ b/app/scripts/ui.js @@ -1,13 +1,15 @@ +// Disabled to allow setting up initial state hooks first + +// This import sets up global functions required for Sentry to function. +// It must be run first in case an error is thrown later during initialization. +import './lib/setup-initial-state-hooks'; + // polyfills import '@formatjs/intl-relativetimeformat/polyfill'; // dev only, "react-devtools" import is skipped in prod builds import 'react-devtools'; -// This import sets up a global function required for Sentry to function. -// It must be run first in case an error is thrown later during initialization. -import './lib/setup-persisted-state-hook'; - import PortStream from 'extension-port-stream'; import browser from 'webextension-polyfill'; @@ -34,6 +36,14 @@ import ExtensionPlatform from './platforms/extension'; import { setupMultiplex } from './lib/stream-utils'; import { getEnvironmentType, getPlatform } from './lib/util'; import metaRPCClientFactory from './lib/metaRPCClientFactory'; +import LocalStore from './lib/local-store'; +import ReadOnlyNetworkStore from './lib/network-store'; + +// Setup global hook for improved Sentry state snapshots during initialization +const inTest = process.env.IN_TEST; +const localStore = inTest ? new ReadOnlyNetworkStore() : new LocalStore(); +global.stateHooks.getMostRecentPersistedState = () => + localStore.mostRecentRetrievedState; const container = document.getElementById('app-content'); diff --git a/test/e2e/tests/errors.spec.js b/test/e2e/tests/errors.spec.js index 3098a5083..5f2128d32 100644 --- a/test/e2e/tests/errors.spec.js +++ b/test/e2e/tests/errors.spec.js @@ -197,9 +197,9 @@ describe('Sentry errors', function () { async ({ driver, mockedEndpoint }) => { await driver.navigate(); await driver.findElement('#password'); - // Erase `getSentryState` hook, simulating a "before initialization" state + // Erase `getSentryAppState` hook, simulating a "before initialization" state await driver.executeScript( - 'window.stateHooks.getSentryState = undefined', + 'window.stateHooks.getSentryAppState = undefined', ); // Wait for Sentry request @@ -286,8 +286,23 @@ describe('Sentry errors', function () { const mockTextBody = mockedRequest.body.text.split('\n'); const mockJsonBody = JSON.parse(mockTextBody[2]); const appState = mockJsonBody?.extra?.appState; + assert.deepStrictEqual(Object.keys(appState), [ + 'browser', + 'version', + 'persistedState', + ]); + assert.ok( + typeof appState?.browser === 'string' && + appState?.browser.length > 0, + 'Invalid browser state', + ); + assert.ok( + typeof appState?.version === 'string' && + appState?.version.length > 0, + 'Invalid version state', + ); await matchesSnapshot({ - data: transformBackgroundState(appState), + data: transformBackgroundState(appState.persistedState), snapshot: 'errors-before-init-opt-in-background-state', }); }, @@ -311,9 +326,9 @@ describe('Sentry errors', function () { async ({ driver, mockedEndpoint }) => { await driver.navigate(); await driver.findElement('#password'); - // Erase `getSentryState` hook, simulating a "before initialization" state + // Erase `getSentryAppState` hook, simulating a "before initialization" state await driver.executeScript( - 'window.stateHooks.getSentryState = undefined', + 'window.stateHooks.getSentryAppState = undefined', ); // Trigger error @@ -354,9 +369,9 @@ describe('Sentry errors', function () { async ({ driver, mockedEndpoint }) => { await driver.navigate(); await driver.findElement('#password'); - // Erase `getSentryState` hook, simulating a "before initialization" state + // Erase `getSentryAppState` hook, simulating a "before initialization" state await driver.executeScript( - 'window.stateHooks.getSentryState = undefined', + 'window.stateHooks.getSentryAppState = undefined', ); // Trigger error @@ -371,8 +386,23 @@ describe('Sentry errors', function () { const mockTextBody = mockedRequest.body.text.split('\n'); const mockJsonBody = JSON.parse(mockTextBody[2]); const appState = mockJsonBody?.extra?.appState; + assert.deepStrictEqual(Object.keys(appState), [ + 'browser', + 'version', + 'persistedState', + ]); + assert.ok( + typeof appState?.browser === 'string' && + appState?.browser.length > 0, + 'Invalid browser state', + ); + assert.ok( + typeof appState?.version === 'string' && + appState?.version.length > 0, + 'Invalid version state', + ); await matchesSnapshot({ - data: transformUiState(appState), + data: transformBackgroundState(appState.persistedState), snapshot: 'errors-before-init-opt-in-ui-state', }); }, @@ -481,7 +511,7 @@ describe('Sentry errors', function () { const { level, extra } = mockJsonBody; const [{ type, value }] = mockJsonBody.exception.values; const { participateInMetaMetrics } = - extra.appState.store.MetaMetricsController; + extra.appState.state.MetaMetricsController; // Verify request assert.equal(type, 'TestError'); assert.equal(value, 'Test Error'); @@ -525,8 +555,8 @@ describe('Sentry errors', function () { const appState = mockJsonBody?.extra?.appState; assert.deepStrictEqual(Object.keys(appState), [ 'browser', - 'store', 'version', + 'state', ]); assert.ok( typeof appState?.browser === 'string' && @@ -539,7 +569,7 @@ describe('Sentry errors', function () { 'Invalid version state', ); await matchesSnapshot({ - data: transformBackgroundState(appState.store), + data: transformBackgroundState(appState.state), snapshot: 'errors-after-init-opt-in-background-state', }); }, @@ -577,7 +607,7 @@ describe('Sentry errors', function () { const mockJsonBody = JSON.parse(mockTextBody[2]); const { level, extra } = mockJsonBody; const [{ type, value }] = mockJsonBody.exception.values; - const { participateInMetaMetrics } = extra.appState.store.metamask; + const { participateInMetaMetrics } = extra.appState.state.metamask; // Verify request assert.equal(type, 'TestError'); assert.equal(value, 'Test Error'); @@ -619,8 +649,8 @@ describe('Sentry errors', function () { const appState = mockJsonBody?.extra?.appState; assert.deepStrictEqual(Object.keys(appState), [ 'browser', - 'store', 'version', + 'state', ]); assert.ok( typeof appState?.browser === 'string' && @@ -633,7 +663,7 @@ describe('Sentry errors', function () { 'Invalid version state', ); await matchesSnapshot({ - data: transformUiState(appState.store), + data: transformUiState(appState.state), snapshot: 'errors-after-init-opt-in-ui-state', }); }, diff --git a/test/e2e/tests/state-snapshots/errors-before-init-opt-in-background-state.json b/test/e2e/tests/state-snapshots/errors-before-init-opt-in-background-state.json index 0967ef424..422bcb0e2 100644 --- a/test/e2e/tests/state-snapshots/errors-before-init-opt-in-background-state.json +++ b/test/e2e/tests/state-snapshots/errors-before-init-opt-in-background-state.json @@ -1 +1,111 @@ -{} +{ + "data": { + "AlertController": { + "alertEnabledness": { "unconnectedAccount": true, "web3ShimUsage": true }, + "unconnectedAccountAlertShownOrigins": "object", + "web3ShimUsageOrigins": "object" + }, + "AnnouncementController": "object", + "AppStateController": { + "browserEnvironment": "object", + "nftsDropdownState": "object", + "connectedStatusPopoverHasBeenShown": true, + "termsOfUseLastAgreed": "number", + "defaultHomeActiveTabName": null, + "fullScreenGasPollTokens": "object", + "notificationGasPollTokens": "object", + "popupGasPollTokens": "object", + "qrHardware": "object", + "recoveryPhraseReminderHasBeenShown": "boolean", + "recoveryPhraseReminderLastShown": "number", + "showTestnetMessageInDropdown": "boolean", + "trezorModel": "object", + "usedNetworks": "object", + "snapsInstallPrivacyWarningShown": "boolean" + }, + "CachedBalancesController": "object", + "CurrencyController": { + "conversionDate": 1665507600, + "conversionRate": 1300, + "currentCurrency": "usd", + "nativeCurrency": "ETH", + "usdConversionRate": "number" + }, + "GasFeeController": "object", + "IncomingTransactionsController": { + "incomingTransactions": "object", + "incomingTxLastFetchedBlockByChainId": { + "0x1": null, + "0xe708": null, + "0x5": null, + "0xaa36a7": null, + "0xe704": null + } + }, + "KeyringController": { "vault": "string" }, + "MetaMetricsController": { + "eventsBeforeMetricsOptIn": "object", + "fragments": "object", + "metaMetricsId": "fake-metrics-id", + "participateInMetaMetrics": true, + "traits": "object" + }, + "NetworkController": { + "networkId": "1337", + "selectedNetworkClientId": "string", + "networksMetadata": "object", + "providerConfig": { + "chainId": "string", + "nickname": "Localhost 8545", + "rpcPrefs": "object", + "rpcUrl": "string", + "ticker": "ETH", + "type": "rpc", + "id": "string" + }, + "networkConfigurations": "object" + }, + "OnboardingController": { + "completedOnboarding": true, + "firstTimeFlowType": "import", + "onboardingTabs": "object", + "seedPhraseBackedUp": true + }, + "PermissionController": "object", + "PreferencesController": { + "advancedGasFee": "object", + "currentLocale": "en", + "dismissSeedBackUpReminder": "boolean", + "featureFlags": { "showIncomingTransactions": true }, + "forgottenPassword": false, + "identities": "object", + "infuraBlocked": "boolean", + "ipfsGateway": "dweb.link", + "knownMethodData": "object", + "ledgerTransportType": "string", + "lostIdentities": "object", + "openSeaEnabled": "boolean", + "preferences": { + "hideZeroBalanceTokens": false, + "showFiatInTestnets": false, + "showTestNetworks": false, + "useNativeCurrencyAsPrimaryCurrency": true + }, + "selectedAddress": "string", + "theme": "string", + "useBlockie": false, + "useNftDetection": "boolean", + "useNonceField": false, + "usePhishDetect": true, + "useTokenDetection": "boolean", + "useCurrencyRateCheck": "boolean", + "useMultiAccountBalanceChecker": "boolean" + }, + "SmartTransactionsController": "object", + "SubjectMetadataController": "object", + "TokensController": "object", + "TransactionController": "object", + "config": "object", + "firstTimeInfo": "object" + } +} diff --git a/test/e2e/tests/state-snapshots/errors-before-init-opt-in-ui-state.json b/test/e2e/tests/state-snapshots/errors-before-init-opt-in-ui-state.json index 0967ef424..0a9fac1d0 100644 --- a/test/e2e/tests/state-snapshots/errors-before-init-opt-in-ui-state.json +++ b/test/e2e/tests/state-snapshots/errors-before-init-opt-in-ui-state.json @@ -1 +1,112 @@ -{} +{ + "data": { + "AlertController": { + "alertEnabledness": { "unconnectedAccount": true, "web3ShimUsage": true }, + "unconnectedAccountAlertShownOrigins": "object", + "web3ShimUsageOrigins": "object" + }, + "AnnouncementController": "object", + "AppStateController": { + "browserEnvironment": "object", + "nftsDropdownState": "object", + "connectedStatusPopoverHasBeenShown": true, + "termsOfUseLastAgreed": "number", + "defaultHomeActiveTabName": null, + "fullScreenGasPollTokens": "object", + "notificationGasPollTokens": "object", + "popupGasPollTokens": "object", + "qrHardware": "object", + "recoveryPhraseReminderHasBeenShown": "boolean", + "recoveryPhraseReminderLastShown": "number", + "showTestnetMessageInDropdown": "boolean", + "trezorModel": "object", + "usedNetworks": "object", + "snapsInstallPrivacyWarningShown": "boolean" + }, + "CachedBalancesController": "object", + "CurrencyController": { + "conversionDate": 1665507600, + "conversionRate": 1300, + "currentCurrency": "usd", + "nativeCurrency": "ETH", + "usdConversionRate": "number" + }, + "GasFeeController": "object", + "IncomingTransactionsController": { + "incomingTransactions": "object", + "incomingTxLastFetchedBlockByChainId": { + "0x1": null, + "0xe708": null, + "0x5": null, + "0xaa36a7": null, + "0xe704": null + } + }, + "KeyringController": { "vault": "string" }, + "MetaMetricsController": { + "eventsBeforeMetricsOptIn": "object", + "fragments": "object", + "metaMetricsId": "fake-metrics-id", + "participateInMetaMetrics": true, + "traits": "object" + }, + "NetworkController": { + "networkId": "1337", + "selectedNetworkClientId": "string", + "networksMetadata": "object", + "providerConfig": { + "chainId": "string", + "nickname": "Localhost 8545", + "rpcPrefs": "object", + "rpcUrl": "string", + "ticker": "ETH", + "type": "rpc", + "id": "string" + }, + "networkConfigurations": "object" + }, + "OnboardingController": { + "completedOnboarding": true, + "firstTimeFlowType": "import", + "onboardingTabs": "object", + "seedPhraseBackedUp": true + }, + "PermissionController": "object", + "PreferencesController": { + "advancedGasFee": "object", + "currentLocale": "en", + "dismissSeedBackUpReminder": "boolean", + "featureFlags": { "showIncomingTransactions": true }, + "forgottenPassword": false, + "identities": "object", + "infuraBlocked": "boolean", + "ipfsGateway": "dweb.link", + "knownMethodData": "object", + "ledgerTransportType": "string", + "lostIdentities": "object", + "openSeaEnabled": "boolean", + "preferences": { + "hideZeroBalanceTokens": false, + "showFiatInTestnets": false, + "showTestNetworks": false, + "useNativeCurrencyAsPrimaryCurrency": true + }, + "selectedAddress": "string", + "theme": "string", + "useBlockie": false, + "useNftDetection": "boolean", + "useNonceField": false, + "usePhishDetect": true, + "useTokenDetection": "boolean", + "useCurrencyRateCheck": "boolean", + "useMultiAccountBalanceChecker": "boolean" + }, + "SmartTransactionsController": "object", + "SubjectMetadataController": "object", + "TokensController": "object", + "TransactionController": "object", + "config": "object", + "firstTimeInfo": "object" + }, + "meta": { "version": 74 } +} diff --git a/ui/index.js b/ui/index.js index 6f9a148ed..de1432a80 100644 --- a/ui/index.js +++ b/ui/index.js @@ -233,14 +233,9 @@ function setupStateHooks(store) { }); return state; }; - window.stateHooks.getSentryState = function () { + window.stateHooks.getSentryAppState = function () { const reduxState = store.getState(); - const maskedReduxState = maskObject(reduxState, SENTRY_UI_STATE); - return { - browser: window.navigator.userAgent, - store: maskedReduxState, - version: global.platform.getVersion(), - }; + return maskObject(reduxState, SENTRY_UI_STATE); }; } From 2cd60d94e8a3c947cb2ff97d0acb9789c6ffa654 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Thu, 17 Aug 2023 09:04:30 -0230 Subject: [PATCH 14/23] Remove invalid `tokensChainsCache` state (#20495) Migration #77 would set the `TokenListController.tokensChainsCache` state to `undefined` if it wasn't already set to anything when that migration was run. This is probably harmless except that it results in Sentry errors during migrations, and it results in that property having a value (at least temporarily) that doesn't match its type. Migration #77 has been updated to prevent this property from being set to `undefined` going forward. A new migration has been added to delete this value for any users already affected by this problem. The new migration was named "92.1" so that it could run after 92 but before 93, to make backporting this to v10.34.x easier (v10.34.x is currently on migration 92). "92.1" is still a valid number so this should work just as well as a whole number. --- app/scripts/migrations/077.js | 21 ++- app/scripts/migrations/077.test.js | 70 +++++++++ app/scripts/migrations/092.1.test.ts | 139 ++++++++++++++++++ app/scripts/migrations/092.1.ts | 53 +++++++ app/scripts/migrations/index.js | 2 + test/e2e/tests/errors.spec.js | 2 - .../errors-after-init-opt-in-ui-state.json | 1 + 7 files changed, 284 insertions(+), 4 deletions(-) create mode 100644 app/scripts/migrations/092.1.test.ts create mode 100644 app/scripts/migrations/092.1.ts diff --git a/app/scripts/migrations/077.js b/app/scripts/migrations/077.js index 5a5d4f1ac..9adb293ba 100644 --- a/app/scripts/migrations/077.js +++ b/app/scripts/migrations/077.js @@ -1,4 +1,6 @@ import { cloneDeep } from 'lodash'; +import log from 'loglevel'; +import { hasProperty, isObject } from '@metamask/utils'; import transformState077For082 from './077-supplements/077-supplement-for-082'; import transformState077For084 from './077-supplements/077-supplement-for-084'; import transformState077For086 from './077-supplements/077-supplement-for-086'; @@ -29,8 +31,23 @@ export default { }; function transformState(state) { - const TokenListController = state?.TokenListController || {}; - + if (!hasProperty(state, 'TokenListController')) { + log.warn('Skipping migration, TokenListController state is missing'); + return state; + } else if (!isObject(state.TokenListController)) { + global.sentry?.captureException?.( + new Error( + `typeof state.TokenListController is ${typeof state.TokenListController}`, + ), + ); + return state; + } else if (!hasProperty(state.TokenListController, 'tokensChainsCache')) { + log.warn( + 'Skipping migration, TokenListController.tokensChainsCache state is missing', + ); + return state; + } + const { TokenListController } = state; const { tokensChainsCache } = TokenListController; let dataCache; diff --git a/app/scripts/migrations/077.test.js b/app/scripts/migrations/077.test.js index 53efb5cd5..67de1a8fa 100644 --- a/app/scripts/migrations/077.test.js +++ b/app/scripts/migrations/077.test.js @@ -1,11 +1,22 @@ +import { cloneDeep } from 'lodash'; import migration77 from './077'; +const sentryCaptureExceptionMock = jest.fn(); + +global.sentry = { + startSession: jest.fn(), + endSession: jest.fn(), + toggleSession: jest.fn(), + captureException: sentryCaptureExceptionMock, +}; + describe('migration #77', () => { it('should update the version metadata', async () => { const oldStorage = { meta: { version: 76, }, + data: {}, }; const newStorage = await migration77.migrate(oldStorage); @@ -13,6 +24,65 @@ describe('migration #77', () => { version: 77, }); }); + + it('should return state unchanged if token list controller is missing', async () => { + const oldStorage = { + meta: { + version: 76, + }, + data: { + Foo: { + bar: 'baz', + }, + }, + }; + + const newStorage = await migration77.migrate(cloneDeep(oldStorage)); + + expect(newStorage.data).toStrictEqual(oldStorage.data); + }); + + it('should capture an exception if the TokenListController state is invalid', async () => { + const oldStorage = { + meta: { + version: 76, + }, + data: { + TokenListController: 'test', + }, + }; + + await migration77.migrate(oldStorage); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.TokenListController is string`), + ); + }); + + it('should return state unchanged if tokenChainsCache is missing', async () => { + const oldStorage = { + meta: { + version: 76, + }, + data: { + TokenListController: { + tokenList: { + '0x514910771af9ca656af840dff83e8264ecf986ca': { + address: '0x514910771af9ca656af840dff83e8264ecf986ca', + symbol: 'LINK', + decimals: 18, + }, + }, + }, + }, + }; + + const newStorage = await migration77.migrate(cloneDeep(oldStorage)); + + expect(newStorage.data).toStrictEqual(oldStorage.data); + }); + it('should change the data from array to object for a single network', async () => { const oldStorage = { meta: { diff --git a/app/scripts/migrations/092.1.test.ts b/app/scripts/migrations/092.1.test.ts new file mode 100644 index 000000000..7ab1045ca --- /dev/null +++ b/app/scripts/migrations/092.1.test.ts @@ -0,0 +1,139 @@ +import { cloneDeep } from 'lodash'; +import { migrate, version } from './092.1'; + +const PREVIOUS_VERSION = 92; + +const sentryCaptureExceptionMock = jest.fn(); + +global.sentry = { + startSession: jest.fn(), + endSession: jest.fn(), + toggleSession: jest.fn(), + captureException: sentryCaptureExceptionMock, +}; + +describe('migration #92.1', () => { + afterEach(() => { + jest.resetAllMocks(); + }); + + it('should update the version metadata', async () => { + const oldStorage = { + meta: { + version: PREVIOUS_VERSION, + }, + data: {}, + }; + + const newStorage = await migrate(oldStorage); + expect(newStorage.meta).toStrictEqual({ + version, + }); + }); + + it('should return state unaltered if there is no TokenListController state', async () => { + const oldData = { + other: 'data', + }; + const oldStorage = { + meta: { + version: PREVIOUS_VERSION, + }, + data: oldData, + }; + + const newStorage = await migrate(cloneDeep(oldStorage)); + expect(newStorage.data).toStrictEqual(oldData); + }); + + it('captures an exception if the TokenListController state is invalid', async () => { + const oldStorage = { + meta: { + version: PREVIOUS_VERSION, + }, + data: { TokenListController: 'this is not valid' }, + }; + + await migrate(oldStorage); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(`typeof state.TokenListController is string`), + ); + }); + + it('should return state unaltered if there is no TokenListController tokensChainsCache state', async () => { + const oldData = { + other: 'data', + TokenListController: { + tokenList: {}, + }, + }; + const oldStorage = { + meta: { + version: PREVIOUS_VERSION, + }, + data: oldData, + }; + + const newStorage = await migrate(cloneDeep(oldStorage)); + expect(newStorage.data).toStrictEqual(oldData); + }); + + it('should return state unaltered if the tokensChainsCache state is an unexpected type', async () => { + const oldData = { + other: 'data', + TokenListController: { + tokensChainsCache: 'unexpected string', + }, + }; + const oldStorage = { + meta: { + version: PREVIOUS_VERSION, + }, + data: oldData, + }; + + const newStorage = await migrate(cloneDeep(oldStorage)); + expect(newStorage.data).toStrictEqual(oldData); + }); + + it('should return state unaltered if the tokensChainsCache state is valid', async () => { + const oldData = { + other: 'data', + TokenListController: { + tokensChainsCache: {}, + }, + }; + const oldStorage = { + meta: { + version: PREVIOUS_VERSION, + }, + data: oldData, + }; + + const newStorage = await migrate(cloneDeep(oldStorage)); + expect(newStorage.data).toStrictEqual(oldData); + }); + + it('should remove undefined tokensChainsCache state', async () => { + const oldData = { + other: 'data', + TokenListController: { + tokensChainsCache: undefined, + }, + }; + const oldStorage = { + meta: { + version: PREVIOUS_VERSION, + }, + data: oldData, + }; + + const newStorage = await migrate(cloneDeep(oldStorage)); + expect(newStorage.data).toStrictEqual({ + other: 'data', + TokenListController: {}, + }); + }); +}); diff --git a/app/scripts/migrations/092.1.ts b/app/scripts/migrations/092.1.ts new file mode 100644 index 000000000..62b47fb04 --- /dev/null +++ b/app/scripts/migrations/092.1.ts @@ -0,0 +1,53 @@ +import { cloneDeep } from 'lodash'; +import { hasProperty, isObject } from '@metamask/utils'; +import log from 'loglevel'; + +export const version = 92.1; + +/** + * Check whether the `TokenListController.tokensChainsCache` state is + * `undefined`, and delete it if so. + * + * This property was accidentally set to `undefined` by an earlier revision of + * migration #77 in some cases. + * + * @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: { + meta: { version: number }; + data: Record; +}) { + const versionedData = cloneDeep(originalVersionedData); + versionedData.meta.version = version; + versionedData.data = transformState(versionedData.data); + return versionedData; +} + +function transformState(state: Record) { + if (!hasProperty(state, 'TokenListController')) { + log.warn('Skipping migration, TokenListController state is missing'); + return state; + } else if (!isObject(state.TokenListController)) { + global.sentry?.captureException?.( + new Error( + `typeof state.TokenListController is ${typeof state.TokenListController}`, + ), + ); + return state; + } else if (!hasProperty(state.TokenListController, 'tokensChainsCache')) { + log.warn( + 'Skipping migration, TokenListController.tokensChainsCache state is missing', + ); + return state; + } + + if (state.TokenListController.tokensChainsCache === undefined) { + delete state.TokenListController.tokensChainsCache; + } + + return state; +} diff --git a/app/scripts/migrations/index.js b/app/scripts/migrations/index.js index dc4fcd4e0..f5c169a1f 100644 --- a/app/scripts/migrations/index.js +++ b/app/scripts/migrations/index.js @@ -96,6 +96,7 @@ import * as m089 from './089'; import * as m090 from './090'; import * as m091 from './091'; import * as m092 from './092'; +import * as m092point1 from './092.1'; const migrations = [ m002, @@ -189,6 +190,7 @@ const migrations = [ m090, m091, m092, + m092point1, ]; export default migrations; diff --git a/test/e2e/tests/errors.spec.js b/test/e2e/tests/errors.spec.js index 5f2128d32..36f1921b3 100644 --- a/test/e2e/tests/errors.spec.js +++ b/test/e2e/tests/errors.spec.js @@ -20,7 +20,6 @@ const removedBackgroundFields = [ // These properties are set to undefined, causing inconsistencies between Chrome and Firefox 'AppStateController.currentPopupId', 'AppStateController.timeoutMinutes', - 'TokenListController.tokensChainsCache', ]; const removedUiFields = [ @@ -29,7 +28,6 @@ const removedUiFields = [ // These properties are set to undefined, causing inconsistencies between Chrome and Firefox 'metamask.currentPopupId', 'metamask.timeoutMinutes', - 'metamask.tokensChainsCache', ]; /** diff --git a/test/e2e/tests/state-snapshots/errors-after-init-opt-in-ui-state.json b/test/e2e/tests/state-snapshots/errors-after-init-opt-in-ui-state.json index 129ae885a..89bb21722 100644 --- a/test/e2e/tests/state-snapshots/errors-after-init-opt-in-ui-state.json +++ b/test/e2e/tests/state-snapshots/errors-after-init-opt-in-ui-state.json @@ -130,6 +130,7 @@ "estimatedGasFeeTimeBounds": "object", "gasEstimateType": "string", "tokenList": "object", + "tokensChainsCache": "object", "preventPollingOnNetworkRestart": "boolean", "tokens": "object", "ignoredTokens": "object", From 01b009c9ada46cc40d4429e8106173f8ce270e13 Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Thu, 17 Aug 2023 14:55:15 -0230 Subject: [PATCH 15/23] Run yarn dedupe to deal with unused ses dependency in yarn.lock, for v10.34.5 --- yarn.lock | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/yarn.lock b/yarn.lock index 944abfeba..17d9856dd 100644 --- a/yarn.lock +++ b/yarn.lock @@ -31349,14 +31349,7 @@ __metadata: languageName: node linkType: hard -"ses@npm:^0.18.1": - version: 0.18.4 - resolution: "ses@npm:0.18.4" - checksum: 9afd6edcf390a693926ef728ebb5a435994bbb0f915009ad524c6588cf62e2f66f6d4b4b2694f093b2af2e92c003947ad55404750d756ba75ce70c8636a7ba02 - languageName: node - linkType: hard - -"ses@npm:^0.18.7": +"ses@npm:^0.18.1, ses@npm:^0.18.7": version: 0.18.7 resolution: "ses@npm:0.18.7" dependencies: From 1d130081214b23e1838a028270c5aa686f399fe8 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Fri, 18 Aug 2023 05:47:23 -0230 Subject: [PATCH 16/23] Fix Sentry breadcrumbs collection during initialization (#20521) * Fix Sentry MetaMetrics detection The refactor of the Sentry state in #20491 accidentally broke our opt- in detection. The opt-in detection has been updated to look for both types of application state (during and after initialization). * Continue suppressing breadcrumbs during onboarding * Fix how onboarding status is retrieved The check for whether the user had completed onboarding assumed that the application state was post-initialization UI state. It has been updated to handle background state and pre-initialization state as well. * Remove unnecessary optional chain operators * Add missing optional chain operator * Fix JSDoc description parameter type --- app/scripts/lib/setupSentry.js | 102 ++++++++++++++++++++++++++------- 1 file changed, 82 insertions(+), 20 deletions(-) diff --git a/app/scripts/lib/setupSentry.js b/app/scripts/lib/setupSentry.js index 1838598d0..c6d29995e 100644 --- a/app/scripts/lib/setupSentry.js +++ b/app/scripts/lib/setupSentry.js @@ -127,6 +127,71 @@ export const SENTRY_UI_STATE = { unconnectedAccount: true, }; +/** + * Returns whether MetaMetrics is enabled, given the application state. + * + * @param {{ state: unknown} | { persistedState: unknown }} appState - Application state + * @returns `true` if MetaMask's state has been initialized, and MetaMetrics + * is enabled, `false` otherwise. + */ +function getMetaMetricsEnabledFromAppState(appState) { + // during initialization after loading persisted state + if (appState.persistedState) { + return getMetaMetricsEnabledFromPersistedState(appState.persistedState); + // After initialization + } else if (appState.state) { + // UI + if (appState.state.metamask) { + return Boolean(appState.state.metamask.participateInMetaMetrics); + } + // background + return Boolean( + appState.state.MetaMetricsController?.participateInMetaMetrics, + ); + } + // during initialization, before first persisted state is read + return false; +} + +/** + * Returns whether MetaMetrics is enabled, given the persisted state. + * + * @param {unknown} persistedState - Application state + * @returns `true` if MetaMask's state has been initialized, and MetaMetrics + * is enabled, `false` otherwise. + */ +function getMetaMetricsEnabledFromPersistedState(persistedState) { + return Boolean( + persistedState?.data?.MetaMetricsController?.participateInMetaMetrics, + ); +} + +/** + * Returns whether onboarding has completed, given the application state. + * + * @param {Record} appState - Application state + * @returns `true` if MetaMask's state has been initialized, and MetaMetrics + * is enabled, `false` otherwise. + */ +function getOnboardingCompleteFromAppState(appState) { + // during initialization after loading persisted state + if (appState.persistedState) { + return Boolean( + appState.persistedState.data?.OnboardingController?.completedOnboarding, + ); + // After initialization + } else if (appState.state) { + // UI + if (appState.state.metamask) { + return Boolean(appState.state.metamask.completedOnboarding); + } + // background + return Boolean(appState.state.OnboardingController?.completedOnboarding); + } + // during initialization, before first persisted state is read + return false; +} + export default function setupSentry({ release, getState }) { if (!release) { throw new Error('Missing release'); @@ -164,22 +229,21 @@ export default function setupSentry({ release, getState }) { } /** - * A function that returns whether MetaMetrics is enabled. This should also - * return `false` if state has not yet been initialzed. + * Returns whether MetaMetrics is enabled. If the application hasn't yet + * been initialized, the persisted state will be used (if any). * - * @returns `true` if MetaMask's state has been initialized, and MetaMetrics - * is enabled, `false` otherwise. + * @returns `true` if MetaMetrics is enabled, `false` otherwise. */ async function getMetaMetricsEnabled() { const appState = getState(); - if (Object.keys(appState) > 0) { - return Boolean(appState?.store?.metamask?.participateInMetaMetrics); + if (appState.state || appState.persistedState) { + return getMetaMetricsEnabledFromAppState(appState); } + // If we reach here, it means the error was thrown before initialization + // completed, and before we loaded the persisted state for the first time. try { const persistedState = await globalThis.stateHooks.getPersistedState(); - return Boolean( - persistedState?.data?.MetaMetricsController?.participateInMetaMetrics, - ); + return getMetaMetricsEnabledFromPersistedState(persistedState); } catch (error) { console.error(error); return false; @@ -227,17 +291,15 @@ function hideUrlIfNotInternal(url) { */ export function beforeBreadcrumb(getState) { return (breadcrumb) => { - if (getState) { - const appState = getState(); - if ( - Object.values(appState).length && - (!appState?.store?.metamask?.participateInMetaMetrics || - !appState?.store?.metamask?.completedOnboarding || - breadcrumb?.category === 'ui.input') - ) { - return null; - } - } else { + if (!getState) { + return null; + } + const appState = getState(); + if ( + !getMetaMetricsEnabledFromAppState(appState) || + !getOnboardingCompleteFromAppState(appState) || + breadcrumb?.category === 'ui.input' + ) { return null; } const newBreadcrumb = removeUrlsFromBreadCrumb(breadcrumb); From 3e26da493f68ff5ea9ca7f6da63a8648d2f7bd0b Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Thu, 17 Aug 2023 12:22:37 -0230 Subject: [PATCH 17/23] Initialize composable observable store after update (#20468) * Initialize composable observable store after update The composable observable store now updates state immediately when the structure is updated. Previously each store would only be updated after the first state change. This ensures that the composable observable store state is always complete. * SUpport falsy controller state We now use the nullish coalescing operator when checkint store.state, so that we don't accidentally ignore falsy state. Co-authored-by: Frederik Bolding * Add test for falsy controller state * Update state snapshots A change on `develop` required another state update. --------- Co-authored-by: Frederik Bolding --- app/scripts/lib/ComposableObservableStore.js | 4 + .../lib/ComposableObservableStore.test.js | 40 ++++++++++ ...rs-after-init-opt-in-background-state.json | 80 +++++++++++++++++++ 3 files changed, 124 insertions(+) diff --git a/app/scripts/lib/ComposableObservableStore.js b/app/scripts/lib/ComposableObservableStore.js index ff722a82d..80d9c483d 100644 --- a/app/scripts/lib/ComposableObservableStore.js +++ b/app/scripts/lib/ComposableObservableStore.js @@ -51,6 +51,7 @@ export default class ComposableObservableStore extends ObservableStore { updateStructure(config) { this.config = config; this.removeAllListeners(); + const initialState = {}; for (const key of Object.keys(config)) { if (!config[key]) { throw new Error(`Undefined '${key}'`); @@ -72,7 +73,10 @@ export default class ComposableObservableStore extends ObservableStore { }, ); } + + initialState[key] = store.state ?? store.getState?.(); } + this.updateState(initialState); } /** diff --git a/app/scripts/lib/ComposableObservableStore.test.js b/app/scripts/lib/ComposableObservableStore.test.js index b6a58f1e1..bb3dd48fd 100644 --- a/app/scripts/lib/ComposableObservableStore.test.js +++ b/app/scripts/lib/ComposableObservableStore.test.js @@ -120,6 +120,46 @@ describe('ComposableObservableStore', () => { }); }); + it('should initialize state with all three types of stores', () => { + const controllerMessenger = new ControllerMessenger(); + const exampleStore = new ObservableStore(); + const exampleController = new ExampleController({ + messenger: controllerMessenger, + }); + const oldExampleController = new OldExampleController(); + exampleStore.putState('state'); + exampleController.updateBar('state'); + oldExampleController.updateBaz('state'); + const store = new ComposableObservableStore({ controllerMessenger }); + + store.updateStructure({ + Example: exampleController, + OldExample: oldExampleController, + Store: exampleStore, + }); + + expect(store.getState()).toStrictEqual({ + Example: { bar: 'state' }, + OldExample: { baz: 'state' }, + Store: 'state', + }); + }); + + it('should initialize falsy state', () => { + const controllerMessenger = new ControllerMessenger(); + const exampleStore = new ObservableStore(); + exampleStore.putState(false); + const store = new ComposableObservableStore({ controllerMessenger }); + + store.updateStructure({ + Example: exampleStore, + }); + + expect(store.getState()).toStrictEqual({ + Example: false, + }); + }); + it('should return flattened state', () => { const controllerMessenger = new ControllerMessenger(); const fooStore = new ObservableStore({ foo: 'foo' }); diff --git a/test/e2e/tests/state-snapshots/errors-after-init-opt-in-background-state.json b/test/e2e/tests/state-snapshots/errors-after-init-opt-in-background-state.json index 2f29cc98c..583be2020 100644 --- a/test/e2e/tests/state-snapshots/errors-after-init-opt-in-background-state.json +++ b/test/e2e/tests/state-snapshots/errors-after-init-opt-in-background-state.json @@ -1,5 +1,18 @@ { "AccountTracker": { "accounts": "object" }, + "AddressBookController": "object", + "AlertController": { + "alertEnabledness": { "unconnectedAccount": true, "web3ShimUsage": true }, + "unconnectedAccountAlertShownOrigins": "object", + "web3ShimUsageOrigins": "object" + }, + "AnnouncementController": "object", + "AppMetadataController": { + "currentAppVersion": "10.34.4", + "previousAppVersion": "", + "previousMigrationVersion": 0, + "currentMigrationVersion": 94 + }, "AppStateController": { "connectedStatusPopoverHasBeenShown": true, "defaultHomeActiveTabName": null, @@ -24,6 +37,7 @@ }, "ApprovalController": "object", "CachedBalancesController": "object", + "CronjobController": "object", "CurrencyController": { "conversionDate": "number", "conversionRate": 1700, @@ -42,6 +56,22 @@ "unapprovedEncryptionPublicKeyMsgCount": 0 }, "EnsController": "object", + "GasFeeController": "object", + "IncomingTransactionsController": { + "incomingTransactions": "object", + "incomingTxLastFetchedBlockByChainId": { + "0x1": null, + "0xe708": null, + "0x5": null, + "0xaa36a7": null, + "0xe704": null + } + }, + "KeyringController": { + "isUnlocked": false, + "keyringTypes": "object", + "keyrings": "object" + }, "MetaMetricsController": { "participateInMetaMetrics": true, "metaMetricsId": "fake-metrics-id", @@ -66,6 +96,51 @@ "networksMetadata": "object", "networkConfigurations": "object" }, + "NftController": "object", + "NotificationController": "object", + "OnboardingController": { + "seedPhraseBackedUp": true, + "firstTimeFlowType": "import", + "completedOnboarding": true, + "onboardingTabs": "object" + }, + "PermissionController": "object", + "PermissionLogController": "object", + "PreferencesController": { + "useBlockie": false, + "useNonceField": false, + "usePhishDetect": true, + "dismissSeedBackUpReminder": "boolean", + "disabledRpcMethodPreferences": "object", + "useMultiAccountBalanceChecker": "boolean", + "useTokenDetection": "boolean", + "useNftDetection": "boolean", + "use4ByteResolution": "boolean", + "useCurrencyRateCheck": "boolean", + "openSeaEnabled": "boolean", + "advancedGasFee": "object", + "featureFlags": { "showIncomingTransactions": true }, + "knownMethodData": "object", + "currentLocale": "en", + "identities": "object", + "lostIdentities": "object", + "forgottenPassword": false, + "preferences": { + "hideZeroBalanceTokens": false, + "showFiatInTestnets": false, + "showTestNetworks": false, + "useNativeCurrencyAsPrimaryCurrency": true + }, + "ipfsGateway": "dweb.link", + "useAddressBarEnsResolution": "boolean", + "infuraBlocked": "boolean", + "ledgerTransportType": "string", + "snapRegistryList": "object", + "transactionSecurityCheckEnabled": "boolean", + "theme": "string", + "isLineaMainnetReleased": "boolean", + "selectedAddress": "string" + }, "SignatureController": { "unapprovedMsgs": "object", "unapprovedPersonalMsgs": "object", @@ -74,7 +149,12 @@ "unapprovedPersonalMsgCount": 0, "unapprovedTypedMessagesCount": 0 }, + "SmartTransactionsController": "object", + "SnapController": "object", + "SnapsRegistry": "object", + "SubjectMetadataController": "object", "SwapsController": "object", + "TokenListController": "object", "TokenRatesController": "object", "TokensController": "object", "TxController": "object" From c2163434dbbc8fc791b99e69a05f95af7cbf5c48 Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Fri, 18 Aug 2023 11:15:45 -0230 Subject: [PATCH 18/23] Fix and test log.info calls run for each migration (#20517) * Fix and test log.info calls run for each migration In migrator/index.js, log.info is called before an after each migration. These calls are intended to produce breadcrumbs to be captured by sentry in cases where errors happen during or shortly after migrations are run. These calls were not causing any output to the console because the log.setLevel calls in ui/index.js were setting a 'warn value in local storage that was being used by logLevel in the background. This commit fixes the problem by setting the `persist` param of setLevel to false, so that the background no longer reads the ui's log level. Tests are added to verify that these logs are captured in sentry breadcrumbs when there is a migration error due to an invariant state. * Improve breadcrumb message matching The test modified in this commit asserts eqaulity of messages from breadcrumbs and hard coded expected results. This could cause failures, as sometimes the messages contain whitespace characters. This commit ensures the assertions only check that the expected string is within the message string, ignoring extra characters. --- app/scripts/background.js | 2 +- test/e2e/fixture-builder.js | 7 ++++ test/e2e/tests/errors.spec.js | 60 +++++++++++++++++++++++++++++++++++ ui/index.js | 2 +- 4 files changed, 69 insertions(+), 2 deletions(-) diff --git a/app/scripts/background.js b/app/scripts/background.js index 7e89ce1c3..aa655e5a4 100644 --- a/app/scripts/background.js +++ b/app/scripts/background.js @@ -88,7 +88,7 @@ const metamaskInternalProcessHash = { const metamaskBlockedPorts = ['trezor-connect']; -log.setDefaultLevel(process.env.METAMASK_DEBUG ? 'debug' : 'info'); +log.setLevel(process.env.METAMASK_DEBUG ? 'debug' : 'info', false); const platform = new ExtensionPlatform(); const notificationManager = new NotificationManager(); diff --git a/test/e2e/fixture-builder.js b/test/e2e/fixture-builder.js index ed3e3aaac..3528ec5aa 100644 --- a/test/e2e/fixture-builder.js +++ b/test/e2e/fixture-builder.js @@ -724,6 +724,13 @@ class FixtureBuilder { return this; } + withBadPreferencesControllerState() { + merge(this.fixture.data, { + PreferencesController: 5, + }); + return this; + } + withTokensControllerERC20() { merge(this.fixture.data.TokensController, { tokens: [ diff --git a/test/e2e/tests/errors.spec.js b/test/e2e/tests/errors.spec.js index 36f1921b3..5c557b256 100644 --- a/test/e2e/tests/errors.spec.js +++ b/test/e2e/tests/errors.spec.js @@ -124,6 +124,18 @@ describe('Sentry errors', function () { }); } + async function mockSentryInvariantMigrationError(mockServer) { + return await mockServer + .forPost('https://sentry.io/api/0000000/envelope/') + .withBodyIncluding('typeof state.PreferencesController is number') + .thenCallback(() => { + return { + statusCode: 200, + json: {}, + }; + }); + } + async function mockSentryTestError(mockServer) { return await mockServer .forPost('https://sentry.io/api/0000000/envelope/') @@ -307,6 +319,54 @@ describe('Sentry errors', function () { ); }); + it('should capture migration log breadcrumbs when there is an invariant state error in a migration', async function () { + await withFixtures( + { + fixtures: { + ...new FixtureBuilder() + .withMetaMetricsController({ + metaMetricsId: 'fake-metrics-id', + participateInMetaMetrics: true, + }) + .withBadPreferencesControllerState() + .build(), + }, + ganacheOptions, + title: this.test.title, + failOnConsoleError: false, + testSpecificMock: mockSentryInvariantMigrationError, + }, + async ({ driver, mockedEndpoint }) => { + await driver.navigate(); + + // Wait for Sentry request + await driver.wait(async () => { + const isPending = await mockedEndpoint.isPending(); + return isPending === false; + }, 3000); + + const [mockedRequest] = await mockedEndpoint.getSeenRequests(); + const mockTextBody = mockedRequest.body.text.split('\n'); + const mockJsonBody = JSON.parse(mockTextBody[2]); + const breadcrumbs = mockJsonBody?.breadcrumbs ?? []; + const migrationLogBreadcrumbs = breadcrumbs.filter((breadcrumb) => { + return breadcrumb.message?.match(/Running migration \d+/u); + }); + const migrationLogMessages = migrationLogBreadcrumbs.map( + (breadcrumb) => + breadcrumb.message.match(/(Running migration \d+)/u)[1], + ); + const firstMigrationLog = migrationLogMessages[0]; + const lastMigrationLog = + migrationLogMessages[migrationLogMessages.length - 1]; + + assert.equal(migrationLogMessages.length, 8); + assert.equal(firstMigrationLog, 'Running migration 75'); + assert.equal(lastMigrationLog, 'Running migration 82'); + }, + ); + }); + it('should send error events in UI', async function () { await withFixtures( { diff --git a/ui/index.js b/ui/index.js index de1432a80..208006de1 100644 --- a/ui/index.js +++ b/ui/index.js @@ -27,7 +27,7 @@ import Root from './pages'; import txHelper from './helpers/utils/tx-helper'; import { _setBackgroundConnection } from './store/action-queue'; -log.setLevel(global.METAMASK_DEBUG ? 'debug' : 'warn'); +log.setLevel(global.METAMASK_DEBUG ? 'debug' : 'warn', false); let reduxStore; From fa778d5af9783b62ef083b355bd45885b32cb8b7 Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Fri, 18 Aug 2023 12:55:33 -0230 Subject: [PATCH 19/23] Update snapshot tests for errors.spec.js --- ...ors-after-init-opt-in-background-state.json | 18 +++++++----------- .../errors-after-init-opt-in-ui-state.json | 11 +++++------ ...rs-before-init-opt-in-background-state.json | 3 +-- .../errors-before-init-opt-in-ui-state.json | 3 +-- 4 files changed, 14 insertions(+), 21 deletions(-) diff --git a/test/e2e/tests/state-snapshots/errors-after-init-opt-in-background-state.json b/test/e2e/tests/state-snapshots/errors-after-init-opt-in-background-state.json index 583be2020..b682921e7 100644 --- a/test/e2e/tests/state-snapshots/errors-after-init-opt-in-background-state.json +++ b/test/e2e/tests/state-snapshots/errors-after-init-opt-in-background-state.json @@ -8,10 +8,10 @@ }, "AnnouncementController": "object", "AppMetadataController": { - "currentAppVersion": "10.34.4", + "currentAppVersion": "10.34.5", "previousAppVersion": "", "previousMigrationVersion": 0, - "currentMigrationVersion": 94 + "currentMigrationVersion": 92.1 }, "AppStateController": { "connectedStatusPopoverHasBeenShown": true, @@ -36,8 +36,8 @@ "serviceWorkerLastActiveTime": "number" }, "ApprovalController": "object", + "BackupController": "undefined", "CachedBalancesController": "object", - "CronjobController": "object", "CurrencyController": { "conversionDate": "number", "conversionRate": 1700, @@ -70,7 +70,8 @@ "KeyringController": { "isUnlocked": false, "keyringTypes": "object", - "keyrings": "object" + "keyrings": "object", + "encryptionKey": "object" }, "MetaMetricsController": { "participateInMetaMetrics": true, @@ -82,8 +83,8 @@ "previousUserTraits": "object" }, "NetworkController": { - "selectedNetworkClientId": "string", "networkId": "1337", + "networkStatus": "available", "providerConfig": { "chainId": "string", "nickname": "Localhost 8545", @@ -93,11 +94,10 @@ "type": "rpc", "id": "string" }, - "networksMetadata": "object", + "networkDetails": "object", "networkConfigurations": "object" }, "NftController": "object", - "NotificationController": "object", "OnboardingController": { "seedPhraseBackedUp": true, "firstTimeFlowType": "import", @@ -115,7 +115,6 @@ "useMultiAccountBalanceChecker": "boolean", "useTokenDetection": "boolean", "useNftDetection": "boolean", - "use4ByteResolution": "boolean", "useCurrencyRateCheck": "boolean", "openSeaEnabled": "boolean", "advancedGasFee": "object", @@ -132,7 +131,6 @@ "useNativeCurrencyAsPrimaryCurrency": true }, "ipfsGateway": "dweb.link", - "useAddressBarEnsResolution": "boolean", "infuraBlocked": "boolean", "ledgerTransportType": "string", "snapRegistryList": "object", @@ -150,8 +148,6 @@ "unapprovedTypedMessagesCount": 0 }, "SmartTransactionsController": "object", - "SnapController": "object", - "SnapsRegistry": "object", "SubjectMetadataController": "object", "SwapsController": "object", "TokenListController": "object", diff --git a/test/e2e/tests/state-snapshots/errors-after-init-opt-in-ui-state.json b/test/e2e/tests/state-snapshots/errors-after-init-opt-in-ui-state.json index 89bb21722..af2f71406 100644 --- a/test/e2e/tests/state-snapshots/errors-after-init-opt-in-ui-state.json +++ b/test/e2e/tests/state-snapshots/errors-after-init-opt-in-ui-state.json @@ -32,7 +32,6 @@ "firstTimeFlowType": "import", "completedOnboarding": true, "knownMethodData": "object", - "use4ByteResolution": "boolean", "participateInMetaMetrics": true, "nextNonce": null, "conversionRate": 1300, @@ -57,12 +56,12 @@ "usedNetworks": "object", "snapsInstallPrivacyWarningShown": "boolean", "serviceWorkerLastActiveTime": "number", - "currentAppVersion": "10.34.4", + "currentAppVersion": "10.34.5", "previousAppVersion": "", "previousMigrationVersion": 0, - "currentMigrationVersion": 94, - "selectedNetworkClientId": "string", + "currentMigrationVersion": 92.1, "networkId": "1337", + "networkStatus": "available", "providerConfig": { "chainId": "string", "nickname": "Localhost 8545", @@ -72,10 +71,11 @@ "type": "rpc", "id": "string" }, - "networksMetadata": "object", + "networkDetails": "object", "cachedBalances": "object", "keyringTypes": "object", "keyrings": "object", + "encryptionKey": "object", "useNonceField": false, "usePhishDetect": true, "dismissSeedBackUpReminder": "boolean", @@ -89,7 +89,6 @@ "lostIdentities": "object", "forgottenPassword": false, "ipfsGateway": "dweb.link", - "useAddressBarEnsResolution": "boolean", "infuraBlocked": "boolean", "ledgerTransportType": "string", "snapRegistryList": "object", diff --git a/test/e2e/tests/state-snapshots/errors-before-init-opt-in-background-state.json b/test/e2e/tests/state-snapshots/errors-before-init-opt-in-background-state.json index 422bcb0e2..f3adc0cd9 100644 --- a/test/e2e/tests/state-snapshots/errors-before-init-opt-in-background-state.json +++ b/test/e2e/tests/state-snapshots/errors-before-init-opt-in-background-state.json @@ -52,8 +52,7 @@ }, "NetworkController": { "networkId": "1337", - "selectedNetworkClientId": "string", - "networksMetadata": "object", + "networkStatus": "available", "providerConfig": { "chainId": "string", "nickname": "Localhost 8545", diff --git a/test/e2e/tests/state-snapshots/errors-before-init-opt-in-ui-state.json b/test/e2e/tests/state-snapshots/errors-before-init-opt-in-ui-state.json index 0a9fac1d0..704026b0b 100644 --- a/test/e2e/tests/state-snapshots/errors-before-init-opt-in-ui-state.json +++ b/test/e2e/tests/state-snapshots/errors-before-init-opt-in-ui-state.json @@ -52,8 +52,7 @@ }, "NetworkController": { "networkId": "1337", - "selectedNetworkClientId": "string", - "networksMetadata": "object", + "networkStatus": "available", "providerConfig": { "chainId": "string", "nickname": "Localhost 8545", From 016a1ef4e4f4412824c5b723b9703a2279dcf935 Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Fri, 18 Aug 2023 12:58:08 -0230 Subject: [PATCH 20/23] Remove GHSA-h755-8qp9-cq8 from advisory exclusions because yarn audit output no longer flags that advisory --- .iyarc | 5 ----- 1 file changed, 5 deletions(-) diff --git a/.iyarc b/.iyarc index 9e16de044..cea1e59eb 100644 --- a/.iyarc +++ b/.iyarc @@ -4,8 +4,3 @@ GHSA-257v-vj4p-3w2h # request library is subject to SSRF. # addressed by temporary patch in .yarn/patches/request-npm-2.88.2-f4a57c72c4.patch GHSA-p8p7-x288-28g6 - -# Prototype pollution -# Not easily patched -# Minimal risk to us because we're using lockdown which also prevents this case of prototype pollution -GHSA-h755-8qp9-cq85 From 0a3241e30db934083709994af95b6d2f1e236b75 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Fri, 18 Aug 2023 16:32:28 -0230 Subject: [PATCH 21/23] Fix pre-initialization UI error state capture (#20529) In the case where an error is thrown in the UI before initialization has finished, we aren't capturing the application state correctly for Sentry errors. We had a test case for this, but the test case was broken due to a mistake in how the `network-store` was setup (it was not matching the behavior of the real `local-tstore` module). The pre-initialization state capture logic was updated to rely solely upon the `localStore` instance used by Sentry to determine whether the user had opted-in to metrics or not. This simplifies the logic a great deal, removing the need for the `getMostRecentPersistedState` state hook. It also ensures that state is captured corretly pre- initialization in both the background and UI. --- app/scripts/background.js | 9 ++--- app/scripts/lib/network-store.js | 6 ++- app/scripts/lib/setup-initial-state-hooks.js | 39 ++++++++++++-------- app/scripts/ui.js | 8 ---- 4 files changed, 31 insertions(+), 31 deletions(-) diff --git a/app/scripts/background.js b/app/scripts/background.js index aa655e5a4..830452033 100644 --- a/app/scripts/background.js +++ b/app/scripts/background.js @@ -71,12 +71,6 @@ import DesktopManager from '@metamask/desktop/dist/desktop-manager'; ///: END:ONLY_INCLUDE_IN /* eslint-enable import/order */ -// Setup global hook for improved Sentry state snapshots during initialization -const inTest = process.env.IN_TEST; -const localStore = inTest ? new ReadOnlyNetworkStore() : new LocalStore(); -global.stateHooks.getMostRecentPersistedState = () => - localStore.mostRecentRetrievedState; - const { sentry } = global; const firstTimeState = { ...rawFirstTimeState }; @@ -100,6 +94,9 @@ const openMetamaskTabsIDs = {}; const requestAccountTabIds = {}; let controller; +// state persistence +const inTest = process.env.IN_TEST; +const localStore = inTest ? new ReadOnlyNetworkStore() : new LocalStore(); let versionedData; if (inTest || process.env.METAMASK_DEBUG) { diff --git a/app/scripts/lib/network-store.js b/app/scripts/lib/network-store.js index 2f4c0a1b0..3a0326a2b 100644 --- a/app/scripts/lib/network-store.js +++ b/app/scripts/lib/network-store.js @@ -31,7 +31,6 @@ export default class ReadOnlyNetworkStore { const response = await fetchWithTimeout(FIXTURE_SERVER_URL); if (response.ok) { this._state = await response.json(); - this.mostRecentRetrievedState = this._state; } } catch (error) { log.debug(`Error loading network state: '${error.message}'`); @@ -49,6 +48,11 @@ export default class ReadOnlyNetworkStore { if (!this._initialized) { await this._initializing; } + // Delay setting this until after the first read, to match the + // behavior of the local store. + if (!this.mostRecentRetrievedState) { + this.mostRecentRetrievedState = this._state; + } return this._state; } diff --git a/app/scripts/lib/setup-initial-state-hooks.js b/app/scripts/lib/setup-initial-state-hooks.js index fa02987a7..f65bb5257 100644 --- a/app/scripts/lib/setup-initial-state-hooks.js +++ b/app/scripts/lib/setup-initial-state-hooks.js @@ -5,7 +5,9 @@ import ReadOnlyNetworkStore from './network-store'; import { SENTRY_BACKGROUND_STATE } from './setupSentry'; const platform = new ExtensionPlatform(); -const localStore = process.env.IN_TEST + +// This instance of `localStore` is used by Sentry to get the persisted state +const sentryLocalStore = process.env.IN_TEST ? new ReadOnlyNetworkStore() : new LocalStore(); @@ -15,7 +17,7 @@ const localStore = process.env.IN_TEST * @returns The persisted wallet state. */ globalThis.stateHooks.getPersistedState = async function () { - return await localStore.get(); + return await sentryLocalStore.get(); }; const persistedStateMask = { @@ -37,25 +39,30 @@ globalThis.stateHooks.getSentryState = function () { browser: window.navigator.userAgent, version: platform.getVersion(), }; + // If `getSentryAppState` is set, it implies that initialization has completed if (globalThis.stateHooks.getSentryAppState) { return { ...sentryState, state: globalThis.stateHooks.getSentryAppState(), }; - } else if (globalThis.stateHooks.getMostRecentPersistedState) { - const persistedState = globalThis.stateHooks.getMostRecentPersistedState(); - if (persistedState) { - return { - ...sentryState, - persistedState: maskObject( - // `getMostRecentPersistedState` is used here instead of - // `getPersistedState` to avoid making this an asynchronous function. - globalThis.stateHooks.getMostRecentPersistedState(), - persistedStateMask, - ), - }; - } - return sentryState; + } else if ( + // This is truthy if Sentry has retrieved state at least once already. This + // should always be true because Sentry calls `getPersistedState` during + // error processing (before this function is called) if `getSentryAppState` + // hasn't been set yet. + sentryLocalStore.mostRecentRetrievedState + ) { + return { + ...sentryState, + persistedState: maskObject( + sentryLocalStore.mostRecentRetrievedState, + persistedStateMask, + ), + }; } + // This branch means that local storage has not yet been read, so we have + // no choice but to omit the application state. + // This should be unreachable, unless an error was encountered during error + // processing. return sentryState; }; diff --git a/app/scripts/ui.js b/app/scripts/ui.js index aeb93cacb..6f923ce39 100644 --- a/app/scripts/ui.js +++ b/app/scripts/ui.js @@ -36,14 +36,6 @@ import ExtensionPlatform from './platforms/extension'; import { setupMultiplex } from './lib/stream-utils'; import { getEnvironmentType, getPlatform } from './lib/util'; import metaRPCClientFactory from './lib/metaRPCClientFactory'; -import LocalStore from './lib/local-store'; -import ReadOnlyNetworkStore from './lib/network-store'; - -// Setup global hook for improved Sentry state snapshots during initialization -const inTest = process.env.IN_TEST; -const localStore = inTest ? new ReadOnlyNetworkStore() : new LocalStore(); -global.stateHooks.getMostRecentPersistedState = () => - localStore.mostRecentRetrievedState; const container = document.getElementById('app-content'); From ddeaeb5ba51715b15e10e2631f2c92d3b8ee4a83 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Fri, 18 Aug 2023 18:26:27 -0230 Subject: [PATCH 22/23] Fix Sentry breadcrumb collection during initialization (again) (#20532) Sentry breadcrumb collection during initialization was broken in #20529 because we failed to consider that the `getSentryState` check was also used for an opt-in check in the `beforeBreadcrumb` hook. I had assumed that `getSentryState` was only used to get state to add additional context to an error report. But the function has a second purpose: to get state for the purposes of checking whether the user has opted into MetaMetrics. In this second case, `mostRecentRetrievedState` is sometimes unset (which violates an assumption made in #20529) The `getMostRecentPersistedState` hook removed in #20529 has been restored, ensuring that the `getSentryState` function returns Sentry state after loading state for the first time, but before the first error has occurred. This mistake didn't cause e2e tests to fail because multiple errors are currently thrown in the background upon initialization on `develop` (relating to Snow scuttling). These errors were early enough that they happened before the console logs that our breadcrumb test was testing for. When #20529 was ported onto the v10.34.5 RC, these errors were not present so the test failed correctly. --- app/scripts/background.js | 10 +++-- app/scripts/lib/setup-initial-state-hooks.js | 40 ++++++++++++-------- 2 files changed, 30 insertions(+), 20 deletions(-) diff --git a/app/scripts/background.js b/app/scripts/background.js index 830452033..44f9d177d 100644 --- a/app/scripts/background.js +++ b/app/scripts/background.js @@ -71,6 +71,12 @@ import DesktopManager from '@metamask/desktop/dist/desktop-manager'; ///: END:ONLY_INCLUDE_IN /* eslint-enable import/order */ +// Setup global hook for improved Sentry state snapshots during initialization +const inTest = process.env.IN_TEST; +const localStore = inTest ? new ReadOnlyNetworkStore() : new LocalStore(); +global.stateHooks.getMostRecentPersistedState = () => + localStore.mostRecentRetrievedState; + const { sentry } = global; const firstTimeState = { ...rawFirstTimeState }; @@ -93,10 +99,6 @@ let uiIsTriggering = false; const openMetamaskTabsIDs = {}; const requestAccountTabIds = {}; let controller; - -// state persistence -const inTest = process.env.IN_TEST; -const localStore = inTest ? new ReadOnlyNetworkStore() : new LocalStore(); let versionedData; if (inTest || process.env.METAMASK_DEBUG) { diff --git a/app/scripts/lib/setup-initial-state-hooks.js b/app/scripts/lib/setup-initial-state-hooks.js index f65bb5257..9b3f18e09 100644 --- a/app/scripts/lib/setup-initial-state-hooks.js +++ b/app/scripts/lib/setup-initial-state-hooks.js @@ -28,9 +28,12 @@ const persistedStateMask = { }; /** - * Get a state snapshot to include with Sentry error reports. This uses the - * persisted state pre-initialization, and the in-memory state post- - * initialization. In both cases the state is anonymized. + * Get a state snapshot for Sentry. This is used to add additional context to + * error reports, and it's used when processing errors and breadcrumbs to + * determine whether the user has opted into Metametrics. + * + * This uses the persisted state pre-initialization, and the in-memory state + * post-initialization. In both cases the state is anonymized. * * @returns A Sentry state snapshot. */ @@ -47,22 +50,27 @@ globalThis.stateHooks.getSentryState = function () { }; } else if ( // This is truthy if Sentry has retrieved state at least once already. This - // should always be true because Sentry calls `getPersistedState` during - // error processing (before this function is called) if `getSentryAppState` - // hasn't been set yet. - sentryLocalStore.mostRecentRetrievedState + // should always be true when getting context for an error report, but can + // be unset when Sentry is performing the opt-in check. + sentryLocalStore.mostRecentRetrievedState || + // This is only set in the background process. + globalThis.stateHooks.getMostRecentPersistedState ) { - return { - ...sentryState, - persistedState: maskObject( - sentryLocalStore.mostRecentRetrievedState, - persistedStateMask, - ), - }; + const persistedState = + sentryLocalStore.mostRecentRetrievedState || + globalThis.stateHooks.getMostRecentPersistedState(); + // This can be unset when this method is called in the background for an + // opt-in check, but the state hasn't been loaded yet. + if (persistedState) { + return { + ...sentryState, + persistedState: maskObject(persistedState, persistedStateMask), + }; + } } // This branch means that local storage has not yet been read, so we have // no choice but to omit the application state. - // This should be unreachable, unless an error was encountered during error - // processing. + // This should be unreachable when getting context for an error report, but + // can be false when Sentry is performing the opt-in check. return sentryState; }; From f83c7ff3ef21503797723fa30b086e6938a8a5e5 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Fri, 18 Aug 2023 18:30:40 -0230 Subject: [PATCH 23/23] Update changelog for v10.34.5 (#20533) --- CHANGELOG.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b7f1393ca..dc05b3ced 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ## [10.34.5] +### Changed +- Improve error diagnostic information + - Add additional logging for state migrations ([#20424](https://github.com/MetaMask/metamask-extension/pull/20424), [#20517](https://github.com/MetaMask/metamask-extension/pull/20517), [#20521](https://github.com/MetaMask/metamask-extension/pull/20521)) + - Improve diagnostic state snapshot ([#20457](https://github.com/MetaMask/metamask-extension/pull/20457), [#20491](https://github.com/MetaMask/metamask-extension/pull/20491), [#20428](https://github.com/MetaMask/metamask-extension/pull/20428), [#20458](https://github.com/MetaMask/metamask-extension/pull/20458)) + - Capture additional errors when state migrations fail ([#20427](https://github.com/MetaMask/metamask-extension/pull/20427)) + +### Fixed +- Fix bug where state was temporarily incomplete upon initial load ([#20468](https://github.com/MetaMask/metamask-extension/pull/20468)) + - In rare circumstances, this bug may have resulted in data loss (of preferences, permissions, or tracked NFTs/tokens) or UI crashes. ## [10.34.4] ### Changed