From df646a03eb0c6efaf8597ccd4c52ac2f2d5476a0 Mon Sep 17 00:00:00 2001 From: Brad Decker Date: Wed, 20 Jul 2022 08:25:04 -0500 Subject: [PATCH] Fix Provider Tracking Metrics (#15082) --- .../lib/createRPCMethodTrackingMiddleware.js | 228 +++++++++++++----- .../createRPCMethodTrackingMiddleware.test.js | 217 +++++++++++++++++ jest.config.js | 8 + shared/constants/app.js | 3 + shared/constants/metametrics.js | 9 + 5 files changed, 411 insertions(+), 54 deletions(-) create mode 100644 app/scripts/lib/createRPCMethodTrackingMiddleware.test.js diff --git a/app/scripts/lib/createRPCMethodTrackingMiddleware.js b/app/scripts/lib/createRPCMethodTrackingMiddleware.js index 89141357f..3362e29ad 100644 --- a/app/scripts/lib/createRPCMethodTrackingMiddleware.js +++ b/app/scripts/lib/createRPCMethodTrackingMiddleware.js @@ -1,19 +1,91 @@ +import { MESSAGE_TYPE, ORIGIN_METAMASK } from '../../../shared/constants/app'; import { EVENT, EVENT_NAMES } from '../../../shared/constants/metametrics'; import { SECOND } from '../../../shared/constants/time'; -const USER_PROMPTED_EVENT_NAME_MAP = { - eth_signTypedData_v4: EVENT_NAMES.SIGNATURE_REQUESTED, - eth_signTypedData_v3: EVENT_NAMES.SIGNATURE_REQUESTED, - eth_signTypedData: EVENT_NAMES.SIGNATURE_REQUESTED, - eth_personal_sign: EVENT_NAMES.SIGNATURE_REQUESTED, - eth_sign: EVENT_NAMES.SIGNATURE_REQUESTED, - eth_getEncryptionPublicKey: EVENT_NAMES.ENCRYPTION_PUBLIC_KEY_REQUESTED, - eth_decrypt: EVENT_NAMES.DECRYPTION_REQUESTED, - wallet_requestPermissions: EVENT_NAMES.PERMISSIONS_REQUESTED, - eth_requestAccounts: EVENT_NAMES.PERMISSIONS_REQUESTED, +/** + * These types determine how the method tracking middleware handles incoming + * requests based on the method name. There are three options right now but + * the types could be expanded to cover other options in the future. + */ +const RATE_LIMIT_TYPES = { + RATE_LIMITED: 'rate_limited', + BLOCKED: 'blocked', + NON_RATE_LIMITED: 'non_rate_limited', }; -const samplingTimeouts = {}; +/** + * This object maps a method name to a RATE_LIMIT_TYPE. If not in this map the + * default is 'RATE_LIMITED' + */ +const RATE_LIMIT_MAP = { + [MESSAGE_TYPE.ETH_SIGN]: RATE_LIMIT_TYPES.NON_RATE_LIMITED, + [MESSAGE_TYPE.ETH_SIGN_TYPED_DATA]: RATE_LIMIT_TYPES.NON_RATE_LIMITED, + [MESSAGE_TYPE.ETH_SIGN_TYPED_DATA_V3]: RATE_LIMIT_TYPES.NON_RATE_LIMITED, + [MESSAGE_TYPE.ETH_SIGN_TYPED_DATA_V4]: RATE_LIMIT_TYPES.NON_RATE_LIMITED, + [MESSAGE_TYPE.PERSONAL_SIGN]: RATE_LIMIT_TYPES.NON_RATE_LIMITED, + [MESSAGE_TYPE.ETH_DECRYPT]: RATE_LIMIT_TYPES.NON_RATE_LIMITED, + [MESSAGE_TYPE.ETH_GET_ENCRYPTION_PUBLIC_KEY]: + RATE_LIMIT_TYPES.NON_RATE_LIMITED, + [MESSAGE_TYPE.ETH_REQUEST_ACCOUNTS]: RATE_LIMIT_TYPES.RATE_LIMITED, + [MESSAGE_TYPE.WALLET_REQUEST_PERMISSIONS]: RATE_LIMIT_TYPES.RATE_LIMITED, + [MESSAGE_TYPE.SEND_METADATA]: RATE_LIMIT_TYPES.BLOCKED, + [MESSAGE_TYPE.GET_PROVIDER_STATE]: RATE_LIMIT_TYPES.BLOCKED, +}; + +/** + * For events with user interaction (approve / reject | cancel) this map will + * return an object with APPROVED, REJECTED and REQUESTED keys that map to the + * appropriate event names. + */ +const EVENT_NAME_MAP = { + [MESSAGE_TYPE.ETH_SIGN]: { + APPROVED: EVENT_NAMES.SIGNATURE_APPROVED, + REJECTED: EVENT_NAMES.SIGNATURE_REJECTED, + REQUESTED: EVENT_NAMES.SIGNATURE_REQUESTED, + }, + [MESSAGE_TYPE.ETH_SIGN_TYPED_DATA]: { + APPROVED: EVENT_NAMES.SIGNATURE_APPROVED, + REJECTED: EVENT_NAMES.SIGNATURE_REJECTED, + REQUESTED: EVENT_NAMES.SIGNATURE_REQUESTED, + }, + [MESSAGE_TYPE.ETH_SIGN_TYPED_DATA_V3]: { + APPROVED: EVENT_NAMES.SIGNATURE_APPROVED, + REJECTED: EVENT_NAMES.SIGNATURE_REJECTED, + REQUESTED: EVENT_NAMES.SIGNATURE_REQUESTED, + }, + [MESSAGE_TYPE.ETH_SIGN_TYPED_DATA_V4]: { + APPROVED: EVENT_NAMES.SIGNATURE_APPROVED, + REJECTED: EVENT_NAMES.SIGNATURE_REJECTED, + REQUESTED: EVENT_NAMES.SIGNATURE_REQUESTED, + }, + [MESSAGE_TYPE.PERSONAL_SIGN]: { + APPROVED: EVENT_NAMES.SIGNATURE_APPROVED, + REJECTED: EVENT_NAMES.SIGNATURE_REJECTED, + REQUESTED: EVENT_NAMES.SIGNATURE_REQUESTED, + }, + [MESSAGE_TYPE.ETH_DECRYPT]: { + APPROVED: EVENT_NAMES.DECRYPTION_APPROVED, + REJECTED: EVENT_NAMES.DECRYPTION_REJECTED, + REQUESTED: EVENT_NAMES.DECRYPTION_REQUESTED, + }, + [MESSAGE_TYPE.ETH_GET_ENCRYPTION_PUBLIC_KEY]: { + APPROVED: EVENT_NAMES.ENCRYPTION_PUBLIC_KEY_APPROVED, + REJECTED: EVENT_NAMES.ENCRYPTION_PUBLIC_KEY_REJECTED, + REQUESTED: EVENT_NAMES.ENCRYPTION_PUBLIC_KEY_REQUESTED, + }, + [MESSAGE_TYPE.ETH_REQUEST_ACCOUNTS]: { + APPROVED: EVENT_NAMES.PERMISSIONS_APPROVED, + REJECTED: EVENT_NAMES.PERMISSIONS_REJECTED, + REQUESTED: EVENT_NAMES.PERMISSIONS_REQUESTED, + }, + [MESSAGE_TYPE.WALLET_REQUEST_PERMISSIONS]: { + APPROVED: EVENT_NAMES.PERMISSIONS_APPROVED, + REJECTED: EVENT_NAMES.PERMISSIONS_REJECTED, + REQUESTED: EVENT_NAMES.PERMISSIONS_REQUESTED, + }, +}; + +const rateLimitTimeouts = {}; /** * Returns a middleware that tracks inpage_provider usage using sampling for @@ -21,65 +93,113 @@ const samplingTimeouts = {}; * signature requests * * @param {object} opts - options for the rpc method tracking middleware - * @param {Function} opts.trackEvent - trackEvent method from MetaMetricsController - * @param {Function} opts.getMetricsState - get the state of MetaMetricsController + * @param {Function} opts.trackEvent - trackEvent method from + * MetaMetricsController + * @param {Function} opts.getMetricsState - get the state of + * MetaMetricsController + * @param {number} [opts.rateLimitSeconds] - number of seconds to wait before + * allowing another set of events to be tracked. * @returns {Function} */ export default function createRPCMethodTrackingMiddleware({ trackEvent, getMetricsState, + rateLimitSeconds = 60, }) { return function rpcMethodTrackingMiddleware( /** @type {any} */ req, /** @type {any} */ res, /** @type {Function} */ next, ) { - const startTime = Date.now(); - const { origin } = req; + const { origin, method } = req; + + // Determine what type of rate limit to apply based on method + const rateLimitType = + RATE_LIMIT_MAP[method] ?? RATE_LIMIT_TYPES.RATE_LIMITED; + + // If the rateLimitType is RATE_LIMITED check the rateLimitTimeouts + const rateLimited = + rateLimitType === RATE_LIMIT_TYPES.RATE_LIMITED && + typeof rateLimitTimeouts[method] !== 'undefined'; + + // Get the participateInMetaMetrics state to determine if we should track + // anything. This is extra redundancy because this value is checked in + // the metametrics controller's trackEvent method as well. + const userParticipatingInMetaMetrics = + getMetricsState().participateInMetaMetrics === true; + + // Get the event type, each of which has APPROVED, REJECTED and REQUESTED + // keys for the various events in the flow. + const eventType = EVENT_NAME_MAP[method]; + + // Boolean variable that reduces code duplication and increases legibility + const shouldTrackEvent = + // Don't track if the request came from our own UI or background + origin !== ORIGIN_METAMASK && + // Don't track if this is a blocked method + rateLimitType !== RATE_LIMIT_TYPES.BLOCKED && + // Don't track if the rate limit has been hit + rateLimited === false && + // Don't track if the user isn't participating in metametrics + userParticipatingInMetaMetrics === true; + + if (shouldTrackEvent) { + // We track an initial "requested" event as soon as the dapp calls the + // provider method. For the events not special cased this is the only + // event that will be fired and the event name will be + // 'Provider Method Called'. + const event = eventType + ? eventType.REQUESTED + : EVENT_NAMES.PROVIDER_METHOD_CALLED; + + const properties = {}; + + if (event === EVENT_NAMES.SIGNATURE_REQUESTED) { + properties.signature_type = method; + } else { + properties.method = method; + } + + trackEvent({ + event, + category: EVENT.CATEGORIES.INPAGE_PROVIDER, + referrer: { + url: origin, + }, + properties, + }); + + rateLimitTimeouts[method] = setTimeout(() => { + delete rateLimitTimeouts[method]; + }, SECOND * rateLimitSeconds); + } next((callback) => { - const endTime = Date.now(); - if (!getMetricsState().participateInMetaMetrics) { + if (shouldTrackEvent === false || typeof eventType === 'undefined') { return callback(); } - if (USER_PROMPTED_EVENT_NAME_MAP[req.method]) { - const userRejected = res.error?.code === 4001; - trackEvent({ - event: USER_PROMPTED_EVENT_NAME_MAP[req.method], - category: EVENT.CATEGORIES.INPAGE_PROVIDER, - referrer: { - url: origin, - }, - properties: { - method: req.method, - status: userRejected ? 'rejected' : 'approved', - error_code: res.error?.code, - error_message: res.error?.message, - has_result: typeof res.result !== 'undefined', - duration: endTime - startTime, - }, - }); - } else if (typeof samplingTimeouts[req.method] === 'undefined') { - trackEvent({ - event: 'Provider Method Called', - category: EVENT.CATEGORIES.INPAGE_PROVIDER, - referrer: { - url: origin, - }, - properties: { - method: req.method, - error_code: res.error?.code, - error_message: res.error?.message, - has_result: typeof res.result !== 'undefined', - duration: endTime - startTime, - }, - }); - // Only record one call to this method every ten seconds to avoid - // overloading network requests. - samplingTimeouts[req.method] = setTimeout(() => { - delete samplingTimeouts[req.method]; - }, SECOND * 10); + + // An error code of 4001 means the user rejected the request, which we + // can use here to determine which event to track. + const event = + res.error?.code === 4001 ? eventType.REJECTED : eventType.APPROVED; + + const properties = {}; + + if (eventType.REQUESTED === EVENT_NAMES.SIGNATURE_REQUESTED) { + properties.signature_type = method; + } else { + properties.method = method; } + + trackEvent({ + event, + category: EVENT.CATEGORIES.INPAGE_PROVIDER, + referrer: { + url: origin, + }, + properties, + }); return callback(); }); }; diff --git a/app/scripts/lib/createRPCMethodTrackingMiddleware.test.js b/app/scripts/lib/createRPCMethodTrackingMiddleware.test.js new file mode 100644 index 000000000..ffa953fa0 --- /dev/null +++ b/app/scripts/lib/createRPCMethodTrackingMiddleware.test.js @@ -0,0 +1,217 @@ +import { MESSAGE_TYPE } from '../../../shared/constants/app'; +import { EVENT_NAMES } from '../../../shared/constants/metametrics'; +import { SECOND } from '../../../shared/constants/time'; +import createRPCMethodTrackingMiddleware from './createRPCMethodTrackingMiddleware'; + +const trackEvent = jest.fn(); +const metricsState = { participateInMetaMetrics: null }; +const getMetricsState = () => metricsState; + +const handler = createRPCMethodTrackingMiddleware({ + trackEvent, + getMetricsState, + rateLimitSeconds: 1, +}); + +function getNext(timeout = 500) { + let deferred; + const promise = new Promise((resolve) => { + deferred = { + resolve, + }; + }); + const cb = () => deferred.resolve(); + let triggerNext; + setTimeout(() => { + deferred.resolve(); + }, timeout); + return { + executeMiddlewareStack: async () => { + if (triggerNext) { + triggerNext(() => cb()); + } + return await deferred.resolve(); + }, + promise, + next: (postReqHandler) => { + triggerNext = postReqHandler; + }, + }; +} + +const waitForSeconds = async (seconds) => + await new Promise((resolve) => setTimeout(resolve, SECOND * seconds)); + +describe('createRPCMethodTrackingMiddleware', () => { + afterEach(() => { + jest.resetAllMocks(); + metricsState.participateInMetaMetrics = null; + }); + + describe('before participateInMetaMetrics is set', () => { + it('should not track an event for a signature request', async () => { + const req = { + method: MESSAGE_TYPE.ETH_SIGN, + origin: 'some.dapp', + }; + + const res = { + error: null, + }; + const { executeMiddlewareStack, next } = getNext(); + handler(req, res, next); + await executeMiddlewareStack(); + expect(trackEvent).not.toHaveBeenCalled(); + }); + }); + + describe('participateInMetaMetrics is set to false', () => { + beforeEach(() => { + metricsState.participateInMetaMetrics = false; + }); + + it('should not track an event for a signature request', async () => { + const req = { + method: MESSAGE_TYPE.ETH_SIGN, + origin: 'some.dapp', + }; + + const res = { + error: null, + }; + const { executeMiddlewareStack, next } = getNext(); + handler(req, res, next); + await executeMiddlewareStack(); + expect(trackEvent).not.toHaveBeenCalled(); + }); + }); + + describe('participateInMetaMetrics is set to true', () => { + beforeEach(() => { + metricsState.participateInMetaMetrics = true; + }); + + it(`should immediately track a ${EVENT_NAMES.SIGNATURE_REQUESTED} event`, () => { + const req = { + method: MESSAGE_TYPE.ETH_SIGN, + origin: 'some.dapp', + }; + + const res = { + error: null, + }; + const { next } = getNext(); + handler(req, res, next); + expect(trackEvent).toHaveBeenCalledTimes(1); + expect(trackEvent.mock.calls[0][0]).toMatchObject({ + category: 'inpage_provider', + event: EVENT_NAMES.SIGNATURE_REQUESTED, + properties: { signature_type: MESSAGE_TYPE.ETH_SIGN }, + referrer: { url: 'some.dapp' }, + }); + }); + + it(`should track a ${EVENT_NAMES.SIGNATURE_APPROVED} event if the user approves`, async () => { + const req = { + method: MESSAGE_TYPE.ETH_SIGN_TYPED_DATA_V4, + origin: 'some.dapp', + }; + + const res = { + error: null, + }; + const { next, executeMiddlewareStack } = getNext(); + 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.ETH_SIGN_TYPED_DATA_V4 }, + referrer: { url: 'some.dapp' }, + }); + }); + + it(`should track a ${EVENT_NAMES.SIGNATURE_REJECTED} event if the user approves`, async () => { + const req = { + method: MESSAGE_TYPE.PERSONAL_SIGN, + origin: 'some.dapp', + }; + + const res = { + error: { code: 4001 }, + }; + const { next, executeMiddlewareStack } = getNext(); + handler(req, res, next); + await executeMiddlewareStack(); + expect(trackEvent).toHaveBeenCalledTimes(2); + expect(trackEvent.mock.calls[1][0]).toMatchObject({ + category: 'inpage_provider', + event: EVENT_NAMES.SIGNATURE_REJECTED, + properties: { signature_type: MESSAGE_TYPE.PERSONAL_SIGN }, + referrer: { url: 'some.dapp' }, + }); + }); + + it(`should track a ${EVENT_NAMES.PERMISSIONS_APPROVED} event if the user approves`, async () => { + const req = { + method: MESSAGE_TYPE.ETH_REQUEST_ACCOUNTS, + origin: 'some.dapp', + }; + + const res = {}; + const { next, executeMiddlewareStack } = getNext(); + handler(req, res, next); + await executeMiddlewareStack(); + expect(trackEvent).toHaveBeenCalledTimes(2); + expect(trackEvent.mock.calls[1][0]).toMatchObject({ + category: 'inpage_provider', + event: EVENT_NAMES.PERMISSIONS_APPROVED, + properties: { method: MESSAGE_TYPE.ETH_REQUEST_ACCOUNTS }, + referrer: { url: 'some.dapp' }, + }); + }); + + it(`should never track blocked methods such as ${MESSAGE_TYPE.GET_PROVIDER_STATE}`, () => { + const req = { + method: MESSAGE_TYPE.GET_PROVIDER_STATE, + origin: 'www.notadapp.com', + }; + + const res = { + error: null, + }; + const { next, executeMiddlewareStack } = getNext(); + handler(req, res, next); + expect(trackEvent).not.toHaveBeenCalled(); + executeMiddlewareStack(); + }); + + it(`should only track events when not rate limited`, async () => { + const req = { + method: 'eth_chainId', + origin: 'some.dapp', + }; + + const res = { + error: null, + }; + + let callCount = 0; + + while (callCount < 3) { + callCount += 1; + const { next, executeMiddlewareStack } = getNext(); + handler(req, res, next); + await executeMiddlewareStack(); + if (callCount !== 3) { + await waitForSeconds(0.6); + } + } + + expect(trackEvent).toHaveBeenCalledTimes(2); + expect(trackEvent.mock.calls[0][0].properties.method).toBe('eth_chainId'); + expect(trackEvent.mock.calls[1][0].properties.method).toBe('eth_chainId'); + }); + }); +}); diff --git a/jest.config.js b/jest.config.js index 38a48471f..ad12bb85b 100644 --- a/jest.config.js +++ b/jest.config.js @@ -1,6 +1,7 @@ module.exports = { collectCoverageFrom: [ '/app/scripts/controllers/permissions/**/*.js', + '/app/scripts/lib/createRPCMethodTrackingMiddleware.js', '/shared/**/*.js', '/ui/**/*.js', ], @@ -20,6 +21,12 @@ module.exports = { lines: 100, statements: 100, }, + './app/scripts/lib/createRPCMethodTrackingMiddleware.js': { + branches: 95.65, + functions: 100, + lines: 100, + statements: 100, + }, }, // TODO: enable resetMocks // resetMocks: true, @@ -34,6 +41,7 @@ module.exports = { '/app/scripts/platforms/*.test.js', 'app/scripts/controllers/network/**/*.test.js', '/app/scripts/controllers/permissions/**/*.test.js', + '/app/scripts/lib/createRPCMethodTrackingMiddleware.test.js', ], testTimeout: 2500, transform: { diff --git a/shared/constants/app.js b/shared/constants/app.js index 4dad386dc..bc27e0df1 100644 --- a/shared/constants/app.js +++ b/shared/constants/app.js @@ -39,11 +39,14 @@ export const MESSAGE_TYPE = { ETH_REQUEST_ACCOUNTS: 'eth_requestAccounts', ETH_SIGN: 'eth_sign', ETH_SIGN_TYPED_DATA: 'eth_signTypedData', + ETH_SIGN_TYPED_DATA_V3: 'eth_signTypedData_v3', + ETH_SIGN_TYPED_DATA_V4: 'eth_signTypedData_v4', GET_PROVIDER_STATE: 'metamask_getProviderState', LOG_WEB3_SHIM_USAGE: 'metamask_logWeb3ShimUsage', PERSONAL_SIGN: 'personal_sign', SEND_METADATA: 'metamask_sendDomainMetadata', SWITCH_ETHEREUM_CHAIN: 'wallet_switchEthereumChain', + WALLET_REQUEST_PERMISSIONS: 'wallet_requestPermissions', WATCH_ASSET: 'wallet_watchAsset', WATCH_ASSET_LEGACY: 'metamask_watchAsset', ///: BEGIN:ONLY_INCLUDE_IN(flask) diff --git a/shared/constants/metametrics.js b/shared/constants/metametrics.js index 8b85fb45f..8d18c9dc0 100644 --- a/shared/constants/metametrics.js +++ b/shared/constants/metametrics.js @@ -276,9 +276,18 @@ export const REJECT_NOTFICIATION_CLOSE_SIG = */ export const EVENT_NAMES = { + ENCRYPTION_PUBLIC_KEY_APPROVED: 'Encryption Public Key Approved', + ENCRYPTION_PUBLIC_KEY_REJECTED: 'Encryption Public Key Rejected', ENCRYPTION_PUBLIC_KEY_REQUESTED: 'Encryption Public Key Requested', + DECRYPTION_APPROVED: 'Decryption Approved', + DECRYPTION_REJECTED: 'Decryption Rejected', DECRYPTION_REQUESTED: 'Decryption Requested', + PERMISSIONS_APPROVED: 'Permissions Approved', + PERMISSIONS_REJECTED: 'Permissions Rejected', PERMISSIONS_REQUESTED: 'Permissions Requested', + PROVIDER_METHOD_CALLED: 'Provider Method Called', + SIGNATURE_APPROVED: 'Signature Approved', + SIGNATURE_REJECTED: 'Signature Rejected', SIGNATURE_REQUESTED: 'Signature Requested', TOKEN_ADDED: 'Token Added', TOKEN_DETECTED: 'Token Detected',