Skip to content

Commit

Permalink
fix: Remove old properties from state (#29792)
Browse files Browse the repository at this point in the history
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

There are several kinds of errors in Sentry which indicate that there
are properties in controller state we are attempting to persist that do
not have corresponding metadata. This indicates that these properties
were removed at some point from the controller's state but no migration
was added that removed them from the persisted wallet state. In many
cases, at the time of removal, such a migration was not needed because
the controller in question inherited from BaseController v1. We have
made a targeted effort over the past few years to migrate all
controllers to BaseController v2, however, and so it matters now that
every property have corresponding metadata or else are removed from
state. We don't want these errors to show up in Sentry because they
create noise, so this commit removes these properties from state.

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

## **Related issues**

Fixes #28289.
Fixes #28290.
Fixes #28300.
Fixes #28302.
Fixes #28344.
Fixes #28608.
Fixes #29746.

## **Manual testing steps**

These changes should not affect users in any way since the errors we are
trying to avoid occur out of band and should not crash anything.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

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

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
  • Loading branch information
mcmire authored and matteoscurati committed Jan 27, 2025
1 parent 87d8b5a commit b5d2bf9
Show file tree
Hide file tree
Showing 3 changed files with 395 additions and 0 deletions.
305 changes: 305 additions & 0 deletions app/scripts/migrations/140.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,305 @@
import { omit } from 'lodash';
import { migrate, version } from './140';

const oldVersion = 139;

const dataWithAllControllerProperties = {
AppStateController: {},
NetworkController: {},
PreferencesController: {},
};

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

const newStorage = await migrate(oldStorage);

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

it('does nothing if state has no AppStateController property, even if it has other relevant properties', async () => {
const data = omit(dataWithAllControllerProperties, 'AppStateController');
const oldStorage = {
meta: { version: oldVersion },
data,
};

const newStorage = await migrate(oldStorage);

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

it('deletes AppStateController.collectiblesDropdownState from state', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
AppStateController: {
collectiblesDropdownState: 'test',
},
},
};

const newStorage = await migrate(oldStorage);

expect(newStorage.data).toStrictEqual({
AppStateController: {},
});
});

it('deletes AppStateController.serviceWorkerLastActiveTime from state', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
AppStateController: {
serviceWorkerLastActiveTime: 5,
},
},
};

const newStorage = await migrate(oldStorage);

expect(newStorage.data).toStrictEqual({
AppStateController: {},
});
});

it('deletes AppStateController.showPortfolioTooltip from state', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
AppStateController: {
showPortfolioTooltip: true,
},
},
};

const newStorage = await migrate(oldStorage);

expect(newStorage.data).toStrictEqual({
AppStateController: {},
});
});

it('does nothing if state has no NetworkController property, even if it has other relevant properties', async () => {
const data = omit(dataWithAllControllerProperties, 'NetworkController');
const oldStorage = {
meta: { version: oldVersion },
data,
};

const newStorage = await migrate(oldStorage);

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

it('deletes NetworkController.networkConfigurations from state', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
NetworkController: {
networkConfigurations: {
'AAAA-AAAA-AAAA-AAAA': {
doesnt: 'matter',
},
},
},
},
};

const newStorage = await migrate(oldStorage);

expect(newStorage.data).toStrictEqual({
NetworkController: {},
});
});

it('deletes NetworkController.providerConfig from state', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
NetworkController: {
providerConfig: {
doesnt: 'matter',
},
},
},
};

const newStorage = await migrate(oldStorage);

expect(newStorage.data).toStrictEqual({
NetworkController: {},
});
});

it('does nothing if state has no PreferencesController property, even if it has other relevant properties', async () => {
const data = omit(dataWithAllControllerProperties, 'PreferencesController');
const oldStorage = {
meta: { version: oldVersion },
data,
};

const newStorage = await migrate(oldStorage);

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

it('deletes PreferencesController.customNetworkListEnabled from state', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
PreferencesController: {
customNetworkListEnabled: true,
},
},
};

const newStorage = await migrate(oldStorage);

expect(newStorage.data).toStrictEqual({
PreferencesController: {},
});
});

it('deletes PreferencesController.disabledRpcMethodPreferences from state', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
PreferencesController: {
disabledRpcMethodPreferences: {
doesnt: 'matter',
},
},
},
};

const newStorage = await migrate(oldStorage);

expect(newStorage.data).toStrictEqual({
PreferencesController: {},
});
});

it('deletes PreferencesController.eip1559V2Enabled from state', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
PreferencesController: {
eip1559V2Enabled: true,
},
},
};

const newStorage = await migrate(oldStorage);

expect(newStorage.data).toStrictEqual({
PreferencesController: {},
});
});

it('deletes PreferencesController.hasDismissedOpenSeaToBlockaidBanner from state', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
PreferencesController: {
hasDismissedOpenSeaToBlockaidBanner: true,
},
},
};

const newStorage = await migrate(oldStorage);

expect(newStorage.data).toStrictEqual({
PreferencesController: {},
});
});

it('deletes PreferencesController.hasMigratedFromOpenSeaToBlockaid from state', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
PreferencesController: {
hasMigratedFromOpenSeaToBlockaid: true,
},
},
};

const newStorage = await migrate(oldStorage);

expect(newStorage.data).toStrictEqual({
PreferencesController: {},
});
});

it('deletes PreferencesController.improvedTokenAllowanceEnabled from state', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
PreferencesController: {
improvedTokenAllowanceEnabled: true,
},
},
};

const newStorage = await migrate(oldStorage);

expect(newStorage.data).toStrictEqual({
PreferencesController: {},
});
});

it('deletes PreferencesController.infuraBlocked from state', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
PreferencesController: {
infuraBlocked: true,
},
},
};

const newStorage = await migrate(oldStorage);

expect(newStorage.data).toStrictEqual({
PreferencesController: {},
});
});

it('deletes PreferencesController.useCollectibleDetection from state', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
PreferencesController: {
useCollectibleDetection: true,
},
},
};

const newStorage = await migrate(oldStorage);

expect(newStorage.data).toStrictEqual({
PreferencesController: {},
});
});

it('deletes PreferencesController.useStaticTokenList from state', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
PreferencesController: {
useStaticTokenList: true,
},
},
};

const newStorage = await migrate(oldStorage);

expect(newStorage.data).toStrictEqual({
PreferencesController: {},
});
});
});
89 changes: 89 additions & 0 deletions app/scripts/migrations/140.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import { hasProperty, isObject } from '@metamask/utils';
import { cloneDeep } from 'lodash';

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

export const version = 140;

/**
* This migration deletes properties from state which have been removed in
* previous commits.
*
* @param originalVersionedData - Versioned MetaMask extension state, exactly
* what we persist to dist.
* @param originalVersionedData.meta - State metadata.
* @param originalVersionedData.meta.version - The current state version.
* @param originalVersionedData.data - The persisted MetaMask state, keyed by
* controller.
* @returns Updated versioned MetaMask extension state.
*/
export async function migrate(
originalVersionedData: VersionedData,
): Promise<VersionedData> {
const versionedData = cloneDeep(originalVersionedData);
versionedData.meta.version = version;
transformState(versionedData.data);
return versionedData;
}

function transformState(state: Record<string, unknown>) {
if (
hasProperty(state, 'AppStateController') &&
isObject(state.AppStateController)
) {
// Removed in 33cc8d587aad05c0b41871ba3676676a3ce5680e with a migration, but
// still persists for some people for some reason
// See https://metamask.sentry.io/issues/6223008336/events/723c5195130e4c5584b53a6656a85595/
delete state.AppStateController.collectiblesDropdownState;
// Removed in 4ea52511eb7934bf0ce6b9b7d570a525120229ce
delete state.AppStateController.serviceWorkerLastActiveTime;
// Removed in 24e0a9030b1a715a008e0c5dfaf9c552bcdb304e with a migration, but
// still persists for some people for some reason
// See https://metamask.sentry.io/issues/6223008336/events/a2cc42d6ed79485a8b2e9072d8033720/
delete state.AppStateController.showPortfolioTooltip;
}

if (
hasProperty(state, 'NetworkController') &&
isObject(state.NetworkController)
) {
// Removed in 555d42b9ead0f4919356ff16e11c663c5e38639e
delete state.NetworkController.networkConfigurations;
// Removed in 800a9d3a177446ff2d05e3e95ec06b3658474207 with a migration, but
// still persists for some people for some reason
// See: https://metamask.sentry.io/issues/6011869130/events/039861ddb07f4b39b947edba3bbd710e/
delete state.NetworkController.providerConfig;
}

if (
hasProperty(state, 'PreferencesController') &&
isObject(state.PreferencesController)
) {
// Removed in 6c84e9604c7160dd91c685f301f3c8bd128ad3e3
delete state.PreferencesController.customNetworkListEnabled;
// Removed in e6ecd956b054a29481071e4eded2f8cd17d137d2
delete state.PreferencesController.disabledRpcMethodPreferences;
// Removed in 8125473dc53476b6685c5e85918f89bce87e3006
delete state.PreferencesController.eip1559V2Enabled;
// Removed in 699ddccc76302df6130835dc6655077806bf6335
delete state.PreferencesController.hasDismissedOpenSeaToBlockaidBanner;
// I could find references to this in the commit history, but don't know
// where it was removed
delete state.PreferencesController.hasMigratedFromOpenSeaToBlockaid;
// Removed in f988dc1c5ef98ec72212d1f58e736556273b68f7
delete state.PreferencesController.improvedTokenAllowanceEnabled;
// Removed in 315c043785cd5d7a4b0f7e974097ccac18a6b241
delete state.PreferencesController.infuraBlocked;
// Removed in 4f66dc948fee54b8491227414342ab0d373475f1 with a migration, but
// still persists for some people for some reason
// See: https://metamask.sentry.io/issues/6042074159/events/5711f95785d741739e5d0fa5ad19e7c0/
delete state.PreferencesController.useCollectibleDetection;
// Removed in eb987a47b51ce410de0047ec883bb4549ce80c85
delete state.PreferencesController.useStaticTokenList;
}

return state;
}
Loading

0 comments on commit b5d2bf9

Please sign in to comment.