Fix Sign-in With Ethereum (SIWE) metametric, add tests, and clean RPC method middleware event logic (#18008)

* rpc middleware: update comment

* rpc middleware: use errorCodes const

* rpc middleware: only create event props once

* rpc middleware: rn properties -> eventProperties

* rpc middleware: use TransactionStatus const

* rpc middleware: use const for ui_customizations

* rpc middleware: no need to push null eventProp
- also removes === null check which makes this logic a bit more robust

* rpc middleware: rn METRIC..OPTIONS -> METRIC..OPT

* clean: add consistency

* rpc middleware: refactor let msgParams

* lint: rm unused METAMETRIC_KEY

* fix test: do not pass ui_customizations: null

* rpc middleware test: consolidate tests

* rpc middleware: fix siwe event
.push returns length of mutated array

* rpc middleware test: add siwe test

* rpc middleware test: rm redudant set

app/scripts/lib/createRPCMethodTrackingMiddleware.js

@@ -1,12 +1,12 @@
 import { errorCodes } from 'eth-rpc-errors';
 import { MESSAGE_TYPE, ORIGIN_METAMASK } from '../../../shared/constants/app';
+import { TransactionStatus } from '../../../shared/constants/transaction';
 import { SECOND } from '../../../shared/constants/time';
 import { detectSIWE } from '../../../shared/modules/siwe';
 import {
   EVENT,
   EVENT_NAMES,
-  METAMETRIC_KEY_OPTIONS,
+  METAMETRIC_KEY_OPT,
-  METAMETRIC_KEY,
 } from '../../../shared/constants/metametrics';
 
 /**
@@ -41,7 +41,7 @@ const RATE_LIMIT_MAP = {
 
 /**
  * For events with user interaction (approve / reject | cancel) this map will
- * return an object with APPROVED, REJECTED and REQUESTED keys that map to the
+ * return an object with APPROVED, REJECTED, REQUESTED, and FAILED keys that map to the
  * appropriate event names.
  */
 const EVENT_NAME_MAP = {
@@ -142,6 +142,8 @@ export default function createRPCMethodTrackingMiddleware({
     // keys for the various events in the flow.
     const eventType = EVENT_NAME_MAP[method];
 
+    const eventProperties = {};
+
     // Boolean variable that reduces code duplication and increases legibility
     const shouldTrackEvent =
       // Don't track if the request came from our own UI or background
@@ -162,27 +164,21 @@ export default function createRPCMethodTrackingMiddleware({
         ? eventType.REQUESTED
         : EVENT_NAMES.PROVIDER_METHOD_CALLED;
 
-      const properties = {};
-
-      let msgParams;
-
       if (event === EVENT_NAMES.SIGNATURE_REQUESTED) {
-        properties.signature_type = method;
+        eventProperties.signature_type = method;
 
         const data = req?.params?.[0];
         const from = req?.params?.[1];
         const paramsExamplePassword = req?.params?.[2];
 
-        msgParams = {
-          ...paramsExamplePassword,
-          from,
-          data,
-          origin,
-        };
-
         const msgData = {
-          msgParams,
+          msgParams: {
-          status: 'unapproved',
+            ...paramsExamplePassword,
+            from,
+            data,
+            origin,
+          },
+          status: TransactionStatus.unapproved,
           type: req.method,
         };
 
@@ -193,25 +189,21 @@ export default function createRPCMethodTrackingMiddleware({
           );
 
           if (securityProviderResponse?.flagAsDangerous === 1) {
-            properties.ui_customizations = ['flagged_as_malicious'];
+            eventProperties.ui_customizations = [
+              METAMETRIC_KEY_OPT.ui_customizations.flaggedAsMalicious,
+            ];
           } else if (securityProviderResponse?.flagAsDangerous === 2) {
-            properties.ui_customizations = ['flagged_as_safety_unknown'];
+            eventProperties.ui_customizations = [
-          } else {
+              METAMETRIC_KEY_OPT.ui_customizations.flaggedAsSafetyUnknown,
-            properties.ui_customizations = null;
+            ];
           }
 
           if (method === MESSAGE_TYPE.PERSONAL_SIGN) {
             const { isSIWEMessage } = detectSIWE({ data });
             if (isSIWEMessage) {
-              properties.ui_customizations === null
+              eventProperties.ui_customizations = (
-                ? (properties.ui_customizations = [
+                eventProperties.ui_customizations || []
-                    METAMETRIC_KEY_OPTIONS[METAMETRIC_KEY.UI_CUSTOMIZATIONS]
+              ).concat(METAMETRIC_KEY_OPT.ui_customizations.SIWE);
-                      .SIWE,
-                  ])
-                : properties.ui_customizations.push(
-                    METAMETRIC_KEY_OPTIONS[METAMETRIC_KEY.UI_CUSTOMIZATIONS]
-                      .SIWE,
-                  );
             }
           }
         } catch (e) {
@@ -220,7 +212,7 @@ export default function createRPCMethodTrackingMiddleware({
           );
         }
       } else {
-        properties.method = method;
+        eventProperties.method = method;
       }
 
       trackEvent({
@@ -229,7 +221,7 @@ export default function createRPCMethodTrackingMiddleware({
         referrer: {
           url: origin,
         },
-        properties,
+        properties: eventProperties,
       });
 
       rateLimitTimeouts[method] = setTimeout(() => {
@@ -242,8 +234,6 @@ export default function createRPCMethodTrackingMiddleware({
         return callback();
       }
 
-      const properties = {};
-
       // The rpc error methodNotFound implies that 'eth_sign' is disabled in Advanced Settings
       const isDisabledEthSignAdvancedSetting =
         method === MESSAGE_TYPE.ETH_SIGN &&
@@ -254,79 +244,20 @@ export default function createRPCMethodTrackingMiddleware({
       let event;
       if (isDisabledRPCMethod) {
         event = eventType.FAILED;
-        properties.error = res.error;
+        eventProperties.error = res.error;
-      } else if (res.error?.code === 4001) {
+      } else if (res.error?.code === errorCodes.provider.userRejectedRequest) {
         event = eventType.REJECTED;
       } else {
         event = eventType.APPROVED;
       }
 
-      let msgParams;
-
-      if (eventType.REQUESTED === EVENT_NAMES.SIGNATURE_REQUESTED) {
-        properties.signature_type = method;
-
-        const data = req?.params?.[0];
-        const from = req?.params?.[1];
-        const paramsExamplePassword = req?.params?.[2];
-
-        msgParams = {
-          ...paramsExamplePassword,
-          from,
-          data,
-          origin,
-        };
-
-        const msgData = {
-          msgParams,
-          status: 'unapproved',
-          type: req.method,
-        };
-
-        try {
-          const securityProviderResponse = await securityProviderRequest(
-            msgData,
-            req.method,
-          );
-
-          if (securityProviderResponse?.flagAsDangerous === 1) {
-            properties.ui_customizations = ['flagged_as_malicious'];
-          } else if (securityProviderResponse?.flagAsDangerous === 2) {
-            properties.ui_customizations = ['flagged_as_safety_unknown'];
-          } else {
-            properties.ui_customizations = null;
-          }
-
-          if (method === MESSAGE_TYPE.PERSONAL_SIGN) {
-            const { isSIWEMessage } = detectSIWE({ data });
-            if (isSIWEMessage) {
-              properties.ui_customizations === null
-                ? (properties.ui_customizations = [
-                    METAMETRIC_KEY_OPTIONS[METAMETRIC_KEY.UI_CUSTOMIZATIONS]
-                      .SIWE,
-                  ])
-                : properties.ui_customizations.push(
-                    METAMETRIC_KEY_OPTIONS[METAMETRIC_KEY.UI_CUSTOMIZATIONS]
-                      .SIWE,
-                  );
-            }
-          }
-        } catch (e) {
-          console.warn(
-            `createRPCMethodTrackingMiddleware: Error calling securityProviderRequest - ${e}`,
-          );
-        }
-      } else {
-        properties.method = method;
-      }
-
       trackEvent({
         event,
         category: EVENT.CATEGORIES.INPAGE_PROVIDER,
         referrer: {
           url: origin,
         },
-        properties,
+        properties: eventProperties,
       });
 
       return callback();

app/scripts/lib/createRPCMethodTrackingMiddleware.test.js

@@ -1,7 +1,11 @@
 import { errorCodes } from 'eth-rpc-errors';
 import { MESSAGE_TYPE } from '../../../shared/constants/app';
-import { EVENT_NAMES } from '../../../shared/constants/metametrics';
+import {
+  EVENT_NAMES,
+  METAMETRIC_KEY_OPT,
+} from '../../../shared/constants/metametrics';
 import { SECOND } from '../../../shared/constants/time';
+import { detectSIWE } from '../../../shared/modules/siwe';
 import createRPCMethodTrackingMiddleware from './createRPCMethodTrackingMiddleware';
 
 const trackEvent = jest.fn();
@@ -52,6 +56,12 @@ function getNext(timeout = 500) {
 const waitForSeconds = async (seconds) =>
   await new Promise((resolve) => setTimeout(resolve, SECOND * seconds));
 
+jest.mock('../../../shared/modules/siwe', () => ({
+  detectSIWE: jest.fn().mockImplementation(() => {
+    return { isSIWEMessage: false };
+  }),
+}));
+
 describe('createRPCMethodTrackingMiddleware', () => {
   afterEach(() => {
     jest.resetAllMocks();
@@ -153,7 +163,7 @@ describe('createRPCMethodTrackingMiddleware', () => {
       };
 
       const res = {
-        error: { code: 4001 },
+        error: { code: errorCodes.provider.userRejectedRequest },
       };
       const { next, executeMiddlewareStack } = getNext();
       await handler(req, res, next);
@@ -230,6 +240,36 @@ describe('createRPCMethodTrackingMiddleware', () => {
       expect(trackEvent.mock.calls[1][0].properties.method).toBe('eth_chainId');
     });
 
+    it('should track Sign-in With Ethereum (SIWE) message if detected', async () => {
+      const req = {
+        method: MESSAGE_TYPE.PERSONAL_SIGN,
+        origin: 'some.dapp',
+      };
+      const res = {
+        error: null,
+      };
+      const { next, executeMiddlewareStack } = getNext();
+
+      detectSIWE.mockImplementation(() => {
+        return { isSIWEMessage: true };
+      });
+
+      await handler(req, res, next);
+      await executeMiddlewareStack();
+
+      expect(trackEvent).toHaveBeenCalledTimes(2);
+
+      expect(trackEvent.mock.calls[1][0]).toMatchObject({
+        category: 'inpage_provider',
+        event: EVENT_NAMES.SIGNATURE_APPROVED,
+        properties: {
+          signature_type: MESSAGE_TYPE.PERSONAL_SIGN,
+          ui_customizations: [METAMETRIC_KEY_OPT.ui_customizations.SIWE],
+        },
+        referrer: { url: 'some.dapp' },
+      });
+    });
+
     describe(`when '${MESSAGE_TYPE.ETH_SIGN}' is disabled in advanced settings`, () => {
       it(`should track ${EVENT_NAMES.SIGNATURE_FAILED} and include error property`, async () => {
         const mockError = { code: errorCodes.rpc.methodNotFound };
@@ -258,93 +298,89 @@ describe('createRPCMethodTrackingMiddleware', () => {
         });
       });
     });
-  });
 
-  describe('participateInMetaMetrics is set to true with a request flagged as safe', () => {
+    describe('when request is flagged as safe by security provider', () => {
-    beforeEach(() => {
+      it(`should immediately track a ${EVENT_NAMES.SIGNATURE_REQUESTED} event`, async () => {
-      metricsState.participateInMetaMetrics = true;
+        const req = {
-    });
+          method: MESSAGE_TYPE.ETH_SIGN,
+          origin: 'some.dapp',
+        };
+        const res = {
+          error: null,
+        };
+        const { next } = getNext();
 
-    it(`should immediately track a ${EVENT_NAMES.SIGNATURE_REQUESTED} event which is flagged as safe`, async () => {
+        await handler(req, res, next);
-      const req = {
-        method: MESSAGE_TYPE.ETH_SIGN,
-        origin: 'some.dapp',
-      };
 
-      const res = {
+        expect(trackEvent).toHaveBeenCalledTimes(1);
-        error: null,
+        expect(trackEvent.mock.calls[0][0]).toMatchObject({
-      };
+          category: 'inpage_provider',
-      const { next } = getNext();
+          event: EVENT_NAMES.SIGNATURE_REQUESTED,
-      await handler(req, res, next);
+          properties: {
-      expect(trackEvent).toHaveBeenCalledTimes(1);
+            signature_type: MESSAGE_TYPE.ETH_SIGN,
-      expect(trackEvent.mock.calls[0][0]).toMatchObject({
+          },
-        category: 'inpage_provider',
+          referrer: { url: 'some.dapp' },
-        event: EVENT_NAMES.SIGNATURE_REQUESTED,
+        });
-        properties: {
-          signature_type: MESSAGE_TYPE.ETH_SIGN,
-          ui_customizations: null,
-        },
-        referrer: { url: 'some.dapp' },
       });
     });
-  });
 
-  describe('participateInMetaMetrics is set to true with a request flagged as malicious', () => {
+    describe('when request is flagged as malicious by security provider', () => {
-    beforeEach(() => {
+      beforeEach(() => {
-      metricsState.participateInMetaMetrics = true;
+        flagAsDangerous = 1;
-      flagAsDangerous = 1;
+      });
-    });
 
-    it(`should immediately track a ${EVENT_NAMES.SIGNATURE_REQUESTED} event which is flagged as malicious`, async () => {
+      it(`should immediately track a ${EVENT_NAMES.SIGNATURE_REQUESTED} event which is flagged as malicious`, async () => {
-      const req = {
+        const req = {
-        method: MESSAGE_TYPE.ETH_SIGN,
+          method: MESSAGE_TYPE.ETH_SIGN,
-        origin: 'some.dapp',
+          origin: 'some.dapp',
-      };
+        };
+        const res = {
+          error: null,
+        };
+        const { next } = getNext();
 
-      const res = {
+        await handler(req, res, next);
-        error: null,
+
-      };
+        expect(trackEvent).toHaveBeenCalledTimes(1);
-      const { next } = getNext();
+        expect(trackEvent.mock.calls[0][0]).toMatchObject({
-      await handler(req, res, next);
+          category: 'inpage_provider',
-      expect(trackEvent).toHaveBeenCalledTimes(1);
+          event: EVENT_NAMES.SIGNATURE_REQUESTED,
-      expect(trackEvent.mock.calls[0][0]).toMatchObject({
+          properties: {
-        category: 'inpage_provider',
+            signature_type: MESSAGE_TYPE.ETH_SIGN,
-        event: EVENT_NAMES.SIGNATURE_REQUESTED,
+            ui_customizations: ['flagged_as_malicious'],
-        properties: {
+          },
-          signature_type: MESSAGE_TYPE.ETH_SIGN,
+          referrer: { url: 'some.dapp' },
-          ui_customizations: ['flagged_as_malicious'],
+        });
-        },
-        referrer: { url: 'some.dapp' },
       });
     });
-  });
 
-  describe('participateInMetaMetrics is set to true with a request flagged as safety unknown', () => {
+    describe('when request flagged as safety unknown by security provider', () => {
-    beforeEach(() => {
+      beforeEach(() => {
-      metricsState.participateInMetaMetrics = true;
+        flagAsDangerous = 2;
-      flagAsDangerous = 2;
+      });
-    });
 
-    it(`should immediately track a ${EVENT_NAMES.SIGNATURE_REQUESTED} event which is flagged as safety unknown`, async () => {
+      it(`should immediately track a ${EVENT_NAMES.SIGNATURE_REQUESTED} event which is flagged as safety unknown`, async () => {
-      const req = {
+        const req = {
-        method: MESSAGE_TYPE.ETH_SIGN,
+          method: MESSAGE_TYPE.ETH_SIGN,
-        origin: 'some.dapp',
+          origin: 'some.dapp',
-      };
+        };
+        const res = {
+          error: null,
+        };
+        const { next } = getNext();
 
-      const res = {
+        await handler(req, res, next);
-        error: null,
+
-      };
+        expect(trackEvent).toHaveBeenCalledTimes(1);
-      const { next } = getNext();
+        expect(trackEvent.mock.calls[0][0]).toMatchObject({
-      await handler(req, res, next);
+          category: 'inpage_provider',
-      expect(trackEvent).toHaveBeenCalledTimes(1);
+          event: EVENT_NAMES.SIGNATURE_REQUESTED,
-      expect(trackEvent.mock.calls[0][0]).toMatchObject({
+          properties: {
-        category: 'inpage_provider',
+            signature_type: MESSAGE_TYPE.ETH_SIGN,
-        event: EVENT_NAMES.SIGNATURE_REQUESTED,
+            ui_customizations: ['flagged_as_safety_unknown'],
-        properties: {
+          },
-          signature_type: MESSAGE_TYPE.ETH_SIGN,
+          referrer: { url: 'some.dapp' },
-          ui_customizations: ['flagged_as_safety_unknown'],
+        });
-        },
-        referrer: { url: 'some.dapp' },
       });
     });
   });

shared/constants/metametrics.js

@@ -462,17 +462,19 @@ export const CONTEXT_PROPS = {
 };
 
 /**
- * These types correspond to the keys in the METAMETRIC_KEY_OPTIONS object
+ * These types correspond to the keys in the METAMETRIC_KEY_OPT object
  */
 export const METAMETRIC_KEY = {
   UI_CUSTOMIZATIONS: `ui_customizations`,
 };
 
 /**
- * This object maps a method name to a METAMETRIC_KEY
+ * This object maps a METAMETRIC_KEY to an object of possible options
  */
-export const METAMETRIC_KEY_OPTIONS = {
+export const METAMETRIC_KEY_OPT = {
   [METAMETRIC_KEY.UI_CUSTOMIZATIONS]: {
+    flaggedAsMalicious: 'flagged_as_malicious',
+    flaggedAsSafetyUnknown: 'flagged_as_safety_unknown',
     SIWE: 'sign_in_with_ethereum',
   },
 };