fix: Remove old properties from state #29792

mcmire · 2025-01-17T20:51:17Z

Description

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.

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

Before

After

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). 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.

github-actions · 2025-01-17T20:51:31Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

mcmire · 2025-01-17T20:52:45Z

app/scripts/migrations/139.ts

+    delete state.NetworkController.networkConfigurations;
+    // Removed in 800a9d3a177446ff2d05e3e95ec06b3658474207 with a migration, but
+    // still persists for some people for some reason
+    delete state.NetworkController.providerConfig;


I found this error in Sentry: https://metamask.sentry.io/issues/6011869130/events/039861ddb07f4b39b947edba3bbd710e/. I am not sure why it is occurring. We already have a migration that removes this property. So do we need this here? Kind of strange that some people still have this property.

mcmire · 2025-01-17T20:53:48Z

app/scripts/migrations/139.ts

+    delete state.PreferencesController.infuraBlocked;
+    // Removed in 4f66dc948fee54b8491227414342ab0d373475f1 with a migration, but
+    // still persists for some people for some reason
+    delete state.PreferencesController.useCollectibleDetection;


I found this error in Sentry here: https://metamask.sentry.io/issues/6042074159/events/5711f95785d741739e5d0fa5ad19e7c0/. Again we already have a migration that should be removing this property, so I'm not sure if this is needed?

metamaskbot · 2025-01-17T21:26:06Z

Builds ready [f964268]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- content-script: 0
- offscreen: 0

Page Load Metrics (1736 ± 65 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	295	2060	1647	331	159
		domContentLoaded	1491	2048	1709	134	64
		load	1511	2063	1736	136	65
		domInteractive	26	68	36	12	6
		backgroundConnect	6	61	23	19	9
		firstReactRender	16	78	43	23	11
		getState	4	51	16	17	8
		initialActions	0	1	0	0	0
		loadScripts	1072	1519	1258	115	55
		setupStore	6	56	15	17	8
		uiStartup	1708	2281	1962	146	70

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 1.55 KiB (0.03%)
ui: 38 Bytes (0.00%)
common: 0 Bytes (0.00%)

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. However, we have made a targeted effort over the past few years to migrate all controllers to BaseController v2, 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.

metamaskbot · 2025-01-17T22:09:49Z

Builds ready [e69d847]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- content-script: 0
- offscreen: 0

Page Load Metrics (1768 ± 76 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	347	2088	1641	455	218
		domContentLoaded	1499	2063	1740	153	73
		load	1517	2095	1768	159	76
		domInteractive	26	88	41	17	8
		backgroundConnect	4	101	26	29	14
		firstReactRender	16	96	47	29	14
		getState	4	62	18	19	9
		initialActions	0	0	0	0	0
		loadScripts	1059	1516	1277	123	59
		setupStore	6	87	20	26	13
		uiStartup	1756	2798	2058	258	124

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 1.55 KiB (0.03%)
ui: 38 Bytes (0.00%)
common: 0 Bytes (0.00%)

metamaskbot · 2025-01-17T22:55:27Z

Builds ready [f28fa49]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- content-script: 0
- offscreen: 0

Page Load Metrics (1930 ± 229 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	345	3625	1795	649	312
		domContentLoaded	1515	3604	1904	482	231
		load	1576	3619	1930	477	229
		domInteractive	25	113	47	23	11
		backgroundConnect	7	73	26	20	9
		firstReactRender	18	197	46	43	21
		getState	5	74	23	23	11
		initialActions	0	1	0	0	0
		loadScripts	1091	2778	1434	391	188
		setupStore	6	71	15	18	9
		uiStartup	1779	4493	2297	621	298

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 1.65 KiB (0.03%)
ui: 38 Bytes (0.00%)
common: 0 Bytes (0.00%)

mcmire added the team-wallet-framework label Jan 17, 2025

mcmire commented Jan 17, 2025

View reviewed changes

mcmire changed the title ~~Remove old properties from state~~ fix: Remove old properties from state Jan 17, 2025

mcmire force-pushed the remove-old-state-properties branch from f964268 to e69d847 Compare January 17, 2025 21:35

mcmire marked this pull request as ready for review January 17, 2025 22:12

Remove a couple more properties

f28fa49

mcmire requested a review from a team January 17, 2025 23:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Remove old properties from state #29792

fix: Remove old properties from state #29792

mcmire commented Jan 17, 2025 •

edited by github-actions bot

Loading

github-actions bot commented Jan 17, 2025

mcmire Jan 17, 2025

mcmire Jan 17, 2025

metamaskbot commented Jan 17, 2025

metamaskbot commented Jan 17, 2025

metamaskbot commented Jan 17, 2025

fix: Remove old properties from state #29792

Are you sure you want to change the base?

fix: Remove old properties from state #29792

Conversation

mcmire commented Jan 17, 2025 • edited by github-actions bot Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Jan 17, 2025

mcmire Jan 17, 2025

Choose a reason for hiding this comment

mcmire Jan 17, 2025

Choose a reason for hiding this comment

metamaskbot commented Jan 17, 2025

metamaskbot commented Jan 17, 2025

metamaskbot commented Jan 17, 2025

mcmire commented Jan 17, 2025 •

edited by github-actions bot

Loading