fix: Remove previousUserTraits from metametrics controller state (#30621)

danjm · danjm · commit ed5d3de6aa31 · 2025-03-10T17:07:53.000-02:30
Truly solving https://github.com/MetaMask/MetaMask-planning/issues/3932 required us to clear `previousUserTraits` from metametrics controller state. That property just functions as a cache, and we could actually maintain it in memory. The reason we need to clear it is that there are many users who had the `has_marketing_consent` trait populated to `true` when the application had a bug that prevented that trait from being submitted to segment. That bug is fixed, so new users don't hit that problem. However, to correctly track existing users that have `has_marketing_consent` set to true, we need the comparison of previous and current user traits in the `_buildUserTraitsObject` function to fail its equality check. To do that, we are clearing `previousUserTraits` so that, upon update of the extension and when the first metrics event is set to segment, the current user trait values will compare to undefined, and they will all be submitted to segment, including `has_marketing_consent`. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/30621?quickstart=1) Fixes: 1. Install a build from this branch 2. Open the background console and the network tab 3. Start onboarding. On the metametrics screen, click the checkbox and then "I agree" 4. The network tab should now include a request to segment with user traits, where has_marketing_consent is set to true -- 1. Follow the above steps on the v12.9.3 build (step 4 will fail) 2. Update the version of that install to the build from this branch 3. Log in. 4. The network tab should now include a request to segment with user traits, where has_marketing_consent is set to true    - [ ] 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). - [ ] 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](https://jsdoc.app/) format if applicable - [ ] 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. - [ ] 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.
diff --git a/app/scripts/constants/sentry-state.ts b/app/scripts/constants/sentry-state.ts
@@ -185,7 +185,6 @@ export const SENTRY_BACKGROUND_STATE = {
     fragments: false,
     metaMetricsId: true,
     participateInMetaMetrics: true,
-    previousUserTraits: false,
     segmentApiCalls: false,
     traits: false,
     dataCollectionForMarketing: false,
diff --git a/app/scripts/controllers/metametrics-controller.ts b/app/scripts/controllers/metametrics-controller.ts
@@ -209,10 +209,6 @@ const controllerMetadata = {
     persist: true,
     anonymous: false,
   },
-  previousUserTraits: {
-    persist: true,
-    anonymous: false,
-  },
   dataCollectionForMarketing: {
     persist: true,
     anonymous: false,
@@ -237,7 +233,6 @@ const controllerMetadata = {
  * @property fragments - Object keyed by UUID with stored fragments as values.
  * @property eventsBeforeMetricsOptIn - Array of queued events added before a user opts into metrics.
  * @property traits - Traits that are not derived from other state keys.
- * @property previousUserTraits - The user traits the last time they were computed.
  * @property dataCollectionForMarketing - Flag to determine if data collection for marketing is enabled.
  * @property marketingCampaignCookieId - The marketing campaign cookie id.
  * @property segmentApiCalls - Object keyed by messageId with segment event type and payload as values.
@@ -249,7 +244,6 @@ export type MetaMetricsControllerState = {
   fragments: Record<string, MetaMetricsEventFragment>;
   eventsBeforeMetricsOptIn: MetaMetricsEventPayload[];
   traits: MetaMetricsUserTraits;
-  previousUserTraits?: MetaMetricsUserTraits;
   dataCollectionForMarketing: boolean | null;
   marketingCampaignCookieId: string | null;
   segmentApiCalls: Record<
@@ -336,7 +330,6 @@ export const getDefaultMetaMetricsControllerState =
     latestNonAnonymousEventTimestamp: 0,
     eventsBeforeMetricsOptIn: [],
     traits: {},
-    previousUserTraits: {},
     fragments: {},
     segmentApiCalls: {},
   });
@@ -352,6 +345,8 @@ export default class MetaMetricsController extends BaseController<
 
   locale: string;
 
+  previousUserTraits?: MetaMetricsUserTraits;
+
   version: MetaMetricsControllerOptions['version'];
 
   #extension: MetaMetricsControllerOptions['extension'];
@@ -1178,7 +1173,7 @@ export default class MetaMetricsController extends BaseController<
         ? Object.keys(metamaskState.custodyAccountDetails)[0]
         : null;
     ///: END:ONLY_INCLUDE_IF
-    const { traits, previousUserTraits } = this.state;
+    const { traits } = this.state;
 
     const currentTraits = {
       [MetaMetricsUserTrait.AddressBookEntries]: sum(
@@ -1239,24 +1234,23 @@ export default class MetaMetricsController extends BaseController<
       ),
     };
 
-    if (!previousUserTraits && metamaskState.participateInMetaMetrics) {
-      this.update((state) => {
-        state.previousUserTraits = currentTraits;
-      });
+    if (!this.previousUserTraits && metamaskState.participateInMetaMetrics) {
+      this.previousUserTraits = currentTraits;
       return currentTraits;
     }
 
-    if (previousUserTraits && !isEqual(previousUserTraits, currentTraits)) {
+    if (
+      this.previousUserTraits &&
+      !isEqual(this.previousUserTraits, currentTraits)
+    ) {
       const updates = pickBy(currentTraits, (v, k) => {
-        // @ts-expect-error It's okay that `k` may not be a key of `previousUserTraits`, because we assume `isEqual` can handle it
-        const previous = previousUserTraits[k];
+        // @ts-expect-error It's okay that `k` may not be a key of `this.previousUserTraits`, because we assume `isEqual` can handle it
+        const previous = this.previousUserTraits[k];
         return !isEqual(previous, v);
       });
 
       if (metamaskState.participateInMetaMetrics) {
-        this.update((state) => {
-          state.previousUserTraits = currentTraits;
-        });
+        this.previousUserTraits = currentTraits;
       }
 
       return updates;
diff --git a/app/scripts/migrations/143.1.test.ts b/app/scripts/migrations/143.1.test.ts
@@ -0,0 +1,55 @@
+import { migrate, version } from './143.1';
+
+const oldVersion = 143;
+
+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 });
+  });
+
+  describe(`migration #${version}`, () => {
+    it('removes the previousUserTraits property from MetaMetricsController state and does not remove other properties', async () => {
+      const oldStorage = {
+        meta: { version: oldVersion },
+        data: {
+          MetaMetricsController: {
+            previousUserTraits: { test: 123 },
+            foo: 'bar',
+          },
+        },
+      };
+      const expectedData = {
+        MetaMetricsController: {
+          foo: 'bar',
+        },
+      };
+      const newStorage = await migrate(oldStorage);
+
+      expect(newStorage.data).toStrictEqual(expectedData);
+    });
+
+    it('has no effect if the previousUserTraits property does not exist', async () => {
+      const oldStorage = {
+        meta: { version: oldVersion },
+        data: {
+          MetaMetricsController: {
+            foo: 'bar',
+          },
+        },
+      };
+      const expectedData = {
+        MetaMetricsController: {
+          foo: 'bar',
+        },
+      };
+      const newStorage = await migrate(oldStorage);
+
+      expect(newStorage.data).toStrictEqual(expectedData);
+    });
+  });
+});
diff --git a/app/scripts/migrations/143.1.ts b/app/scripts/migrations/143.1.ts
@@ -0,0 +1,36 @@
+import { cloneDeep } from 'lodash';
+
+type VersionedData = {
+  meta: { version: number };
+  data: Record<string, unknown>;
+};
+
+export const version = 143.1;
+
+/**
+ * This migration deletes the `previousUserTraits` property from the MetaMetrics Controller state
+ *
+ * @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>) {
+  const metaMetricsControllerState = state?.MetaMetricsController as
+    | Record<string, unknown>
+    | undefined;
+
+  delete metaMetricsControllerState?.previousUserTraits;
+}
diff --git a/app/scripts/migrations/index.js b/app/scripts/migrations/index.js
@@ -168,6 +168,7 @@ const migrations = [
   require('./141'),
   require('./142'),
   require('./143'),
+  require('./143.1'),
 ];
 
 export default migrations;
diff --git a/test/e2e/tests/metrics/state-snapshots/errors-after-init-opt-in-background-state.json b/test/e2e/tests/metrics/state-snapshots/errors-after-init-opt-in-background-state.json
@@ -135,7 +135,6 @@
     "marketingCampaignCookieId": null,
     "eventsBeforeMetricsOptIn": "object",
     "traits": "object",
-    "previousUserTraits": "object",
     "fragments": "object",
     "segmentApiCalls": "object"
   },
diff --git a/test/e2e/tests/metrics/state-snapshots/errors-after-init-opt-in-ui-state.json b/test/e2e/tests/metrics/state-snapshots/errors-after-init-opt-in-ui-state.json
@@ -203,7 +203,6 @@
     "marketingCampaignCookieId": null,
     "eventsBeforeMetricsOptIn": "object",
     "traits": "object",
-    "previousUserTraits": "object",
     "fragments": "object",
     "segmentApiCalls": "object",
     "metaMetricsDataDeletionId": null,
diff --git a/test/integration/data/onboarding-completion-route.json b/test/integration/data/onboarding-completion-route.json
@@ -242,25 +242,6 @@
   "preventPollingOnNetworkRestart": false,
   "previousAppVersion": "",
   "previousMigrationVersion": 0,
-  "previousUserTraits": {
-    "address_book_entries": 1,
-    "install_date_ext": "",
-    "ledger_connection_type": "u2f",
-    "networks_added": [],
-    "networks_without_ticker": [],
-    "nft_autodetection_enabled": false,
-    "number_of_accounts": 1,
-    "number_of_nft_collections": 0,
-    "number_of_nfts": 0,
-    "number_of_tokens": 0,
-    "undefined": false,
-    "three_box_enabled": false,
-    "theme": "os",
-    "token_detection_enabled": true,
-    "desktop_enabled": false,
-    "security_providers": ["blockaid"],
-    "petname_addresses_count": 1
-  },
   "providerConfig": {
     "type": "sepolia",
     "chainId": "0xaa36a7",
