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.

It must be due to some migration failure that we have not yet understood. I think it is good to attempt to delete again here, but we should investigate further if it continues to persist after this

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 · 2025-01-20T20:36:42Z

@cryptodev-2s @danjm Oops, had to fix merge conflicts. Can I get another review?

metamaskbot · 2025-01-20T20:59:48Z

Builds ready [ec7ae60]

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 (1579 ± 55 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1459	1957	1579	115	55
		domContentLoaded	1428	1942	1556	117	56
		load	1459	1964	1579	115	55
		domInteractive	23	72	37	15	7
		backgroundConnect	6	64	21	19	9
		firstReactRender	15	97	41	31	15
		getState	4	52	11	13	6
		initialActions	0	1	0	0	0
		loadScripts	1005	1434	1137	100	48
		setupStore	5	51	13	15	7
		uiStartup	1610	2245	1896	194	93

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

background: 1.65 KiB (0.03%)
ui: 0 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

danjm previously approved these changes Jan 20, 2025

View reviewed changes

cryptodev-2s previously approved these changes Jan 20, 2025

View reviewed changes

Merge branch 'main' into remove-old-state-properties

ec7ae60

mcmire dismissed stale reviews from cryptodev-2s and danjm via ec7ae60 January 20, 2025 20:26

cryptodev-2s approved these changes Jan 20, 2025

View reviewed changes

danjm approved these changes Jan 21, 2025

View reviewed changes

mcmire added this pull request to the merge queue Jan 21, 2025

Merged via the queue into main with commit fcb30d7 Jan 21, 2025
72 checks passed

mcmire deleted the remove-old-state-properties branch January 21, 2025 16:25

github-actions bot locked and limited conversation to collaborators Jan 21, 2025

metamaskbot added the release-12.12.0 Issue or pull request that will be included in release 12.12.0 label Jan 21, 2025

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

danjm Jan 20, 2025

mcmire Jan 17, 2025

metamaskbot commented Jan 17, 2025

metamaskbot commented Jan 17, 2025

metamaskbot commented Jan 17, 2025

mcmire commented Jan 20, 2025

metamaskbot commented Jan 20, 2025

fix: Remove old properties from state #29792

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

danjm Jan 20, 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 20, 2025

metamaskbot commented Jan 20, 2025

mcmire commented Jan 17, 2025 •

edited by github-actions bot

Loading