refactor: Refactor extension storage classes #24016

brad-decker · 2024-04-12T20:55:16Z

Description

adds a new class that models the shared surface of the two storage solutions we utilize, and then creates two new classes designed to extend from that one. The goal of this Pull request is to consolidate some of the operations of state management that were previously being implemented in background.js (the process of creating a new state tree). We relied upon a bullish value being returned from the .get() method to know to generate a new first time state object fro the user. This logic has been moved into the storage class so that determining whether to use a new state tree is handled within the storage system itself.

This is a step towards providing a fail safe for state corruption. The new class will be utilized for this additional functionality.

Related issues

Fixes:

Manual testing steps

No functional changes should have occurred. Manual regression cases to test include:

make some changes such as a adding a network or changing settings. Close the browser. Re-open the browser and open metamask. The data should be unchanged.
Install an old version... such as 11.10.0. Make some changes. Upgrade to the build generated from this branch. Verify that the state of the wallet, and its data, is as expected.

Screenshots/Recordings

Before

After

Pre-merge author checklist

I’ve followed MetaMask 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.

metamaskbot · 2024-04-15T15:54:45Z

Builds ready [9837883]

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

Page Load Metrics (1288 ± 593 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	81	347	141	66	32
		domContentLoaded	15	99	31	19	9
		load	68	3188	1288	1235	593
		domInteractive	15	99	31	19	9

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

background: -1.85 KiB (-0.06%)
ui: 587 Bytes (0.01%)
common: 6.78 KiB (0.13%)

codecov · 2024-04-25T17:36:32Z

Codecov Report

Attention: Patch coverage is 44.44444% with 65 lines in your changes are missing coverage. Please review.

Project coverage is 67.46%. Comparing base (cfcbd1e) to head (91b92a6).

Files	Patch %	Lines
shared/modules/Storage/ReadOnlyNetworkStore.ts	0.00%	43 Missing ⚠️
shared/modules/Storage/ExtensionStore.ts	70.77%	19 Missing ⚠️
app/scripts/lib/setup-initial-state-hooks.js	0.00%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #24016      +/-   ##
===========================================
- Coverage    67.48%   67.46%   -0.02%     
===========================================
  Files         1288     1289       +1     
  Lines        50153    50181      +28     
  Branches     13023    13027       +4     
===========================================
+ Hits         33842    33852      +10     
- Misses       16311    16329      +18

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

metamaskbot · 2024-04-25T17:49:27Z

Builds ready [be80104]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8
- content-script: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (882 ± 603 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	57	152	90	27	13
		domContentLoaded	9	37	14	7	3
		load	45	3049	882	1256	603
		domInteractive	9	37	14	7	3

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

background: -1.85 KiB (-0.05%)
ui: 0 Bytes (0.00%)
common: 6.78 KiB (0.11%)

app/scripts/background.js

shared/modules/Storage/ExtensionStore.ts

DDDDDanica · 2024-04-26T11:56:33Z

shared/modules/Storage/ExtensionStore.ts

+    // we use this flag to avoid flooding sentry with a ton of errors:
+    // once data persistence fails once and it flips true we don't send further
+    // data persistence errors to sentry
+    this.dataPersistenceFailing = false;


shared/modules/Storage/ExtensionStore.ts

metamaskbot · 2024-05-06T15:48:41Z

Builds ready [8e43959]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8
- content-script: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (648 ± 569 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	54	151	82	27	13
		domContentLoaded	7	55	15	11	5
		load	43	3327	648	1185	569
		domInteractive	7	55	15	11	5

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

background: -1.85 KiB (-0.05%)
ui: 0 Bytes (0.00%)
common: 6.57 KiB (0.11%)

danjm · 2024-05-16T08:21:54Z

app/scripts/background.js

@@ -81,12 +80,14 @@ import DesktopManager from '@metamask/desktop/dist/desktop-manager';

 // Setup global hook for improved Sentry state snapshots during initialization
 const inTest = process.env.IN_TEST;
-const localStore = inTest ? new ReadOnlyNetworkStore() : new LocalStore();
+const migrator = new Migrator({ migrations });


Previously the new Migrator({ migrations }) call was inside loadStateFromPersistence which was inside the try block of initialize, the catch of which uses rejectInitialization coming from the deferredPromise() call. It seems unlikely that the Migrator constructor could throw an error, but if it did somehow, it would no longer be handled by rejectInitialization. But again, maybe it is not really possible for the Migrator constructor to error?

Also, the timing of creating the migrator has changed slightly: it used to await a localStore.get call, but now will happen immediately on the browser loading the background file. This probably has no consequence, but I thought I'd point it out

I think this is fine and that these changes don't have any negative impact.

danjm · 2024-05-16T08:33:28Z

app/scripts/background.js

-  // this should never happen but new error reporting suggests that it has
-  // for a small number of users
-  // https://github.com/metamask/metamask-extension/issues/3919
-  if (versionedData && !versionedData.data) {


Is this check for !versionedData.data, or an alternative to it, present in the new ExtensionStore.ts? I can't find it.

Addressed by 70e6191

danjm · 2024-05-16T08:41:19Z

shared/modules/Storage/ExtensionStore.ts

+      // if the object is empty, treat it as undefined
+      if (isEmpty(result)) {
+        this.mostRecentRetrievedState = null;
+        this.stateCorruptionDetected = true;


I might be missing it, but I don't see this.stateCorruptionDetected being used anywhere. I see it being set to true and false, but then not used otherwise

Now used in unit tests as of 70e6191

Will be used in application code in a following PR

danjm · 2024-05-16T08:53:26Z

shared/modules/Storage/ReadOnlyNetworkStore.ts

+        this.#state = await response.json();
+      }
+    } catch (error) {
+      if (isErrorWithMessage(error)) {


Could we ever have an error without a message here? If so, why would we ignore it?

Addressed in ada46a4

danjm · 2024-05-16T09:06:01Z

shared/modules/Storage/ReadOnlyNetworkStore.ts

+      this.#initialized = true;
+    }
+  }
+


The line we used to have in background.js that was replaced by a call to isFirstTimeInstall was
const storeAlreadyExisted = Boolean(await localStore.get());

In turn, that get call had:

if (!this._initialized) { await this._initializing; }

So previously, in background.js, when the localStore was a ReadOnlyNetworkStore, the onInstall() function was waiting for init() to resolve, but now it is not awaiting that. Is that intentional?

should const result = this.#state; be const result = await this.get();?

Addressed in e391c77

danjm

I have completed my first review on this. Left some questions and comments. Nice work overall, a very good improvement to the codebase

salimtb · 2024-05-28T09:34:20Z

shared/modules/Storage/ReadOnlyNetworkStore.ts

+ * A read-only network-based storage wrapper
+ */
+export default class ReadOnlyNetworkStore extends BaseStorage {
+  #initialized: boolean;


QQ: why are we using # instead of private keyword here ?

Given that background.js is still just javascript, the private javascript field ensures that these properties will not be misused or unintentionally modified.

salimtb · 2024-05-28T09:39:56Z

shared/modules/Storage/ReadOnlyNetworkStore.ts

+    this.stateCorruptionDetected = false;
+    this.dataPersistenceFailing = false;
+    this.migrator = migrator;
+    this.firstTimeInstall = false;


given that our constructor doesn't rely on a parameter-based variable for initialization, why do we initiate it there? Could we consider an alternative approach like the following?

export default class ReadOnlyNetworkStore extends BaseStorage { #initialized: boolean = false; #initialized = false; #promiseToInitialize?: Promise<void>; #state = null; mostRecentRetrievedState = null; stateCorruptionDetected = false; dataPersistenceFailing = false; migrator:migrator; firstTimeInstall = false; constructor({ migrator }: { migrator: Migrator }) { super(); this.migrator = migrator; this.promiseToInitialize = this.init(); } .....

Could you say more about this? What are the benefits you see from this approach?

danjm · 2024-11-15T16:32:08Z

I need to fix the failing tests...

app/scripts/lib/Stores/ExtensionStore.ts

DDDDDanica · 2024-11-29T16:36:00Z

app/scripts/lib/Stores/ExtensionStore.ts

+   */
+  #set(obj: {
+    data: IntermediaryStateType;
+    meta: { version: number };


Nit: Changing to metadata as it is referred as this.metadata across the store

That would require a state migration, which I'd like to avoid for the sake of a renaming. We could consider renaming this.metadata to this.meta

app/scripts/lib/Stores/ExtensionStore.test.ts

app/scripts/lib/Stores/ExtensionStore.ts

app/scripts/lib/Stores/ExtensionStore.test.ts

app/scripts/lib/Stores/ExtensionStore.ts

app/scripts/background.js

danjm

Unit tests for ReadOnlyNetworkStore
delete the existing store files

…ore possible empty/corrupt vault types

…as an error without a message

Co-authored-by: Danica Shen <[email protected]>

metamaskbot · 2024-12-11T02:18:19Z

Builds ready [f0c377f]

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

Page Load Metrics (2104 ± 187 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	392	3404	2018	543	261
		domContentLoaded	1580	3388	2084	390	187
		load	1627	3406	2104	390	187
		domInteractive	24	76	41	15	7
		backgroundConnect	13	112	26	23	11
		firstReactRender	16	37	21	5	2
		getState	79	243	133	32	15
		initialActions	0	1	0	0	0
		loadScripts	1222	2262	1605	259	124
		setupStore	7	20	10	4	2
		uiStartup	1829	4018	2440	485	233

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

background: -298.99 KiB (-5.69%)
ui: 0 Bytes (0.00%)
common: 303.41 KiB (3.77%)

DDDDDanica · 2024-12-12T16:55:37Z

LGTM !

danjm · 2024-12-13T14:36:53Z

app/scripts/lib/Stores/ExtensionStore.ts

+
+  async isFirstTimeInstall(): Promise<boolean> {
+    const result = await this.#get();
+    return Boolean(isEmpty(result));


nit: this Boolean is redundant

danjm · 2024-12-13T15:46:22Z

app/scripts/lib/Stores/ReadOnlyNetworkStore.ts

+const FIXTURE_SERVER_PORT = 12345;
+const FIXTURE_SERVER_URL = `http://${FIXTURE_SERVER_HOST}:${FIXTURE_SERVER_PORT}/state.json`;
+
+/**


This comment could be more descriptive

Gudahtt · 2024-12-13T15:57:52Z

app/scripts/lib/Stores/BaseStore.ts

+   * metadata key of this class is set on the 'meta' key of the underlying
+   * storage implementation (e.g. chrome.storage.local).
+   */
+  set metadata(metadata: { version: number }) {


We should consider how to ensure:

This is always set prior to persistence

This is correctly loaded from persisted state (or backup persisted state) prior to migrations running

This cannot be set to the incorrect value (e.g. guard where/how it's set)

Gudahtt · 2024-12-13T19:52:33Z

app/scripts/lib/Stores/ExtensionStore.ts

+      // has changed using migrations to adapt to backwards incompatible changes
+      await this.#set({ data: state, meta: this.metadata });
+      if (this.dataPersistenceFailing) {
+        this.dataPersistenceFailing = false;


Looking at this makes me think we're mixing concerns here. This logic is helpful for our "primary store" no matter what storage backend we're using. But it's implemented in a storage-backend-specific class.

Perhaps instead what we want is a single "PersistedStore" class, and then separate "BaseStorage", "ExtensionStorage", and "NetworkStorage" classes that are used by the "PersistedStore". The "PersistedStore" would handle metadata/version management, fallback logic, tracking "failure" state, etc, while the storage classes would just get/set, and could be easily switched around without any other logic changes.

brad-decker changed the base branch from develop to feat/new-onboarding-type April 12, 2024 20:55

brad-decker changed the title ~~Feat/refactor state classes~~ Refactor(storage): Refactor extension storage classes Apr 12, 2024

brad-decker added the team-extension-platform label Apr 15, 2024

brad-decker marked this pull request as ready for review April 15, 2024 15:30

brad-decker requested a review from a team as a code owner April 15, 2024 15:30

brad-decker force-pushed the feat/new-onboarding-type branch from 9e06f1d to b451ae0 Compare April 15, 2024 15:35

gauthierpetetin assigned brad-decker, DDDDDanica and a-breslin and unassigned a-breslin Apr 22, 2024

Base automatically changed from feat/new-onboarding-type to develop April 23, 2024 17:22

brad-decker force-pushed the feat/refactor-state-classes branch from 9837883 to fa740a8 Compare April 24, 2024 17:45

DDDDDanica reviewed Apr 26, 2024

View reviewed changes

app/scripts/background.js Outdated Show resolved Hide resolved

DDDDDanica reviewed Apr 26, 2024

View reviewed changes

shared/modules/Storage/ExtensionStore.ts Outdated Show resolved Hide resolved

DDDDDanica reviewed Apr 26, 2024

View reviewed changes

shared/modules/Storage/ExtensionStore.ts Outdated Show resolved Hide resolved

DDDDDanica previously approved these changes Apr 29, 2024

View reviewed changes

brad-decker dismissed DDDDDanica’s stale review via 8e43959 May 6, 2024 15:15

danjm reviewed May 16, 2024

View reviewed changes

danjm requested changes May 16, 2024

View reviewed changes

salimtb reviewed May 28, 2024

View reviewed changes

danjm changed the title ~~Refactor(storage): Refactor extension storage classes~~ refactor (storage): Refactor extension storage classes Nov 15, 2024

DDDDDanica reviewed Nov 29, 2024

View reviewed changes

app/scripts/lib/Stores/ExtensionStore.test.ts Outdated Show resolved Hide resolved

app/scripts/lib/Stores/ExtensionStore.ts Show resolved Hide resolved

app/scripts/lib/Stores/ExtensionStore.ts Outdated Show resolved Hide resolved

app/scripts/background.js Outdated Show resolved Hide resolved

danjm changed the title ~~refactor (storage): Refactor extension storage classes~~ refactor: Refactor extension storage classes Dec 9, 2024

danjm requested changes Dec 9, 2024

View reviewed changes

brad-decker and others added 11 commits December 9, 2024 15:12

refactor storage system

6fa5a22

Capture sentry error if there is an empty/corrupt vault, and handle m…

44bdfbb

…ore possible empty/corrupt vault types

Ensure log.debug call in case fetch in init in ReadOnlyNetworkStore h…

1c805ca

…as an error without a message

Ensure that in ReadOnlyNetworkStore waits for initializing to complete

b8f75d6

Lint fix

ce2079c

Ensure that first time state is returned if state has no data

7a3373d

Update app/scripts/lib/Stores/ExtensionStore.ts

a5d0eec

Co-authored-by: Danica Shen <[email protected]>

Update test description

8048bd6

Use const for DEFAULT_INITIAL_STATE in ExtensionStore.test.ts

eb46312

Improve naming and usage of versionedData variable in background.js

e1c3f7d

lint fix

82d2650

danjm force-pushed the feat/refactor-state-classes branch from 381a058 to 82d2650 Compare December 9, 2024 18:42

danjm added 2 commits December 9, 2024 15:33

Capture exception with sentry in both error cases ExtensionStore.get

0bbbb9e

Lint fix

f0c377f

danjm added 3 commits December 12, 2024 13:07

Add app/scripts/lib/Stores/ReadOnlyNetworkStore.test.ts

1ca2982

Delete files that are no longer used

16df6c5

Use async/await for get/set in ExtensionStore.ts

e2d023f

DDDDDanica approved these changes Dec 12, 2024

View reviewed changes

danjm reviewed Dec 13, 2024

View reviewed changes

Gudahtt reviewed Dec 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Refactor extension storage classes #24016

refactor: Refactor extension storage classes #24016

brad-decker commented Apr 12, 2024 •

edited by danjm

Loading

metamaskbot commented Apr 15, 2024

codecov bot commented Apr 25, 2024 •

edited

Loading

metamaskbot commented Apr 25, 2024

DDDDDanica Apr 26, 2024

metamaskbot commented May 6, 2024

danjm May 16, 2024

danjm Nov 15, 2024

danjm May 16, 2024

danjm Nov 15, 2024

danjm May 16, 2024

danjm Nov 15, 2024

danjm May 16, 2024

danjm Nov 15, 2024

danjm May 16, 2024

danjm May 16, 2024

danjm Nov 15, 2024

danjm left a comment

salimtb May 28, 2024

danjm Nov 15, 2024

salimtb May 28, 2024 •

edited

Loading

danjm Nov 15, 2024

danjm commented Nov 15, 2024

DDDDDanica Nov 29, 2024

danjm Dec 9, 2024

danjm left a comment •

edited

Loading

metamaskbot commented Dec 11, 2024

DDDDDanica commented Dec 12, 2024

danjm Dec 13, 2024

danjm Dec 13, 2024

Gudahtt Dec 13, 2024

Gudahtt Dec 13, 2024

refactor: Refactor extension storage classes #24016

Are you sure you want to change the base?

refactor: Refactor extension storage classes #24016

Conversation

brad-decker commented Apr 12, 2024 • edited by danjm Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

metamaskbot commented Apr 15, 2024

codecov bot commented Apr 25, 2024 • edited Loading

Codecov Report

metamaskbot commented Apr 25, 2024

Choose a reason for hiding this comment

metamaskbot commented May 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danjm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salimtb May 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danjm commented Nov 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danjm left a comment • edited Loading

Choose a reason for hiding this comment

metamaskbot commented Dec 11, 2024

DDDDDanica commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brad-decker commented Apr 12, 2024 •

edited by danjm

Loading

codecov bot commented Apr 25, 2024 •

edited

Loading

salimtb May 28, 2024 •

edited

Loading

danjm left a comment •

edited

Loading