Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix disabled 'eth_sign' event & add 'Request Disabled' event type #18007

Merged
merged 10 commits into from
Mar 14, 2023
24 changes: 19 additions & 5 deletions app/scripts/lib/createRPCMethodTrackingMiddleware.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { errorCodes } from 'eth-rpc-errors';
import { MESSAGE_TYPE, ORIGIN_METAMASK } from '../../../shared/constants/app';
import { SECOND } from '../../../shared/constants/time';
import { detectSIWE } from '../../../shared/modules/siwe';
Expand Down Expand Up @@ -46,6 +47,7 @@ const RATE_LIMIT_MAP = {
const EVENT_NAME_MAP = {
[MESSAGE_TYPE.ETH_SIGN]: {
APPROVED: EVENT_NAMES.SIGNATURE_APPROVED,
DISABLED: EVENT_NAMES.REQUEST_DISABLED,
REJECTED: EVENT_NAMES.SIGNATURE_REJECTED,
REQUESTED: EVENT_NAMES.SIGNATURE_REQUESTED,
},
Expand Down Expand Up @@ -138,6 +140,12 @@ export default function createRPCMethodTrackingMiddleware({
// keys for the various events in the flow.
const eventType = EVENT_NAME_MAP[method];

const isDisabledEthSignAdvancedSetting =
method === MESSAGE_TYPE.ETH_SIGN &&
res.error?.code === errorCodes.rpc.methodNotFound;

const isDisabledRequest = isDisabledEthSignAdvancedSetting;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey, reason for duplicating the variable ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey, I did this for readability down the line. hope it helps, unless it's just me - In that case, happy to change it back


// Boolean variable that reduces code duplication and increases legibility
const shouldTrackEvent =
// Don't track if the request came from our own UI or background
Expand All @@ -149,7 +157,9 @@ export default function createRPCMethodTrackingMiddleware({
// Don't track if the user isn't participating in metametrics
userParticipatingInMetaMetrics === true;

if (shouldTrackEvent) {
// If shouldTrackEvent is true and isDisabledRequest is true, we will skip tracking the event
// here and track the event in the 'next' callback
if (shouldTrackEvent && !isDisabledRequest) {
// 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
Expand Down Expand Up @@ -195,10 +205,14 @@ export default function createRPCMethodTrackingMiddleware({
return callback();
}

// 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;
let event;
if (isDisabledRequest) {
event = eventType.DISABLED;
} else if (res.error?.code === 4001) {
event = eventType.REJECTED;
} else {
event = eventType.APPROVED;
}

const properties = {};

Expand Down
23 changes: 23 additions & 0 deletions app/scripts/lib/createRPCMethodTrackingMiddleware.test.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { errorCodes } from 'eth-rpc-errors';
import { MESSAGE_TYPE } from '../../../shared/constants/app';
import { EVENT_NAMES } from '../../../shared/constants/metametrics';
import { SECOND } from '../../../shared/constants/time';
Expand Down Expand Up @@ -172,6 +173,28 @@ describe('createRPCMethodTrackingMiddleware', () => {
});
});

it(`should track a ${EVENT_NAMES.REQUEST_DISABLED} for 'eth_sign' event if 'eth_sign' is disabled in advanced settings`, async () => {
const req = {
method: MESSAGE_TYPE.ETH_SIGN,
origin: 'some.dapp',
};
const res = {
error: { code: errorCodes.rpc.methodNotFound },
};
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.REQUEST_DISABLED,
properties: { signature_type: MESSAGE_TYPE.ETH_SIGN },
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,
Expand Down
1 change: 1 addition & 0 deletions shared/constants/metametrics.js
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,7 @@ export const EVENT_NAMES = {
PORTFOLIO_LINK_CLICKED: 'Portfolio Link Clicked',
PUBLIC_ADDRESS_COPIED: 'Public Address Copied',
PROVIDER_METHOD_CALLED: 'Provider Method Called',
REQUEST_DISABLED: 'Request Disabled',
SIGNATURE_APPROVED: 'Signature Approved',
SIGNATURE_REJECTED: 'Signature Rejected',
SIGNATURE_REQUESTED: 'Signature Requested',
Expand Down