Skip to content

Commit

Permalink
feat: Add performance metrics for signature requests (#26967)
Browse files Browse the repository at this point in the history
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

- Adds `Signature` traces
- Wraps `newUnsignedTypedMessage` in a utility function, adds end of
`Middleware` trace and `Signature` trace after hash is returned from
background.

Note that this PR still using a preview will be updated once core is
released.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26967?quickstart=1)

## **Related issues**

Fixes: MetaMask/MetaMask-planning#2902

## **Manual testing steps**

No QA needed.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

An example Sentry trace for `Signature`
https://metamask.sentry.io/performance/trace/53446691f4d44d40a5f9de82f1cebd05/?fov=0%2C4381.00004196167&node=txn-d4053774d6c2495b8564fdce1fac8bec&pageEnd&pageStart&project=273496&source=traces&statsPeriod=15m&timestamp=1725629491.052


### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [x] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [x] 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.

---------

Co-authored-by: Vinicius Stevam <45455812+vinistevam@users.noreply.github.com>
  • Loading branch information
OGPoyraz and vinistevam authored Sep 26, 2024
1 parent 77176e0 commit f36a831
Show file tree
Hide file tree
Showing 11 changed files with 209 additions and 52 deletions.
2 changes: 1 addition & 1 deletion app/scripts/lib/createTracingMiddleware.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ describe('createTracingMiddleware', () => {
});

it('does not add trace context to request if method not supported', async () => {
request.method = MESSAGE_TYPE.ETH_SIGN_TYPED_DATA_V4;
request.method = 'unsupportedMethod';

await createTracingMiddleware()(request, RESPONSE_MOCK, NEXT_MOCK);

Expand Down
21 changes: 18 additions & 3 deletions app/scripts/lib/createTracingMiddleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,19 @@
import { MESSAGE_TYPE } from '../../../shared/constants/app';
import { trace, TraceName } from '../../../shared/lib/trace';

const METHOD_TYPE_TO_TRACE_NAME: Record<string, TraceName> = {
[MESSAGE_TYPE.ETH_SEND_TRANSACTION]: TraceName.Transaction,
[MESSAGE_TYPE.ETH_SIGN_TYPED_DATA]: TraceName.Signature,
[MESSAGE_TYPE.ETH_SIGN_TYPED_DATA_V1]: TraceName.Signature,
[MESSAGE_TYPE.ETH_SIGN_TYPED_DATA_V3]: TraceName.Signature,
[MESSAGE_TYPE.ETH_SIGN_TYPED_DATA_V4]: TraceName.Signature,
[MESSAGE_TYPE.PERSONAL_SIGN]: TraceName.Signature,
};

const METHOD_TYPE_TO_TAGS: Record<string, Record<string, string>> = {
[MESSAGE_TYPE.ETH_SEND_TRANSACTION]: { source: 'dapp' },
};

export default function createTracingMiddleware() {
return async function tracingMiddleware(
req: any,
Expand All @@ -12,11 +25,13 @@ export default function createTracingMiddleware() {
) {
const { id, method } = req;

if (method === MESSAGE_TYPE.ETH_SEND_TRANSACTION) {
const traceName = METHOD_TYPE_TO_TRACE_NAME[method];

if (traceName) {
req.traceContext = await trace({
name: TraceName.Transaction,
name: traceName,
id,
tags: { source: 'dapp' },
tags: METHOD_TYPE_TO_TAGS[method],
});

await trace({
Expand Down
29 changes: 8 additions & 21 deletions app/scripts/lib/ppom/ppom-middleware.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { type Hex, JsonRpcResponseStruct } from '@metamask/utils';
import * as ControllerUtils from '@metamask/controller-utils';
import { detectSIWE, SIWEMessage } from '@metamask/controller-utils';

import { CHAIN_IDS } from '../../../../shared/constants/network';

Expand All @@ -18,7 +18,10 @@ import {
import { SecurityAlertResponse } from './types';

jest.mock('./ppom-util');
jest.mock('@metamask/controller-utils');
jest.mock('@metamask/controller-utils', () => ({
...jest.requireActual('@metamask/controller-utils'),
detectSIWE: jest.fn(),
}));

const SECURITY_ALERT_ID_MOCK = '123';
const INTERNAL_ACCOUNT_ADDRESS = '0xec1adf982415d2ef5ec55899b9bfb8bc0f29251b';
Expand Down Expand Up @@ -112,6 +115,7 @@ describe('PPOMMiddleware', () => {
const generateSecurityAlertIdMock = jest.mocked(generateSecurityAlertId);
const handlePPOMErrorMock = jest.mocked(handlePPOMError);
const isChainSupportedMock = jest.mocked(isChainSupported);
const detectSIWEMock = jest.mocked(detectSIWE);

beforeEach(() => {
jest.resetAllMocks();
Expand All @@ -120,6 +124,7 @@ describe('PPOMMiddleware', () => {
generateSecurityAlertIdMock.mockReturnValue(SECURITY_ALERT_ID_MOCK);
handlePPOMErrorMock.mockReturnValue(SECURITY_ALERT_RESPONSE_MOCK);
isChainSupportedMock.mockResolvedValue(true);
detectSIWEMock.mockReturnValue({ isSIWEMessage: false } as SIWEMessage);

globalThis.sentry = {
withIsolationScope: jest
Expand Down Expand Up @@ -275,25 +280,7 @@ describe('PPOMMiddleware', () => {
tabId: 1048745900,
securityAlertResponse: undefined,
};
jest.spyOn(ControllerUtils, 'detectSIWE').mockReturnValue({
isSIWEMessage: true,
parsedMessage: {
address: '0x935e73edb9ff52e23bac7f7e049a1ecd06d05477',
chainId: 1,
domain: 'metamask.github.io',
expirationTime: null,
issuedAt: '2021-09-30T16:25:24.000Z',
nonce: '32891757',
notBefore: '2022-03-17T12:45:13.610Z',
requestId: 'some_id',
scheme: null,
statement:
'I accept the MetaMask Terms of Service: https://community.metamask.io/tos',
uri: 'https://metamask.github.io',
version: '1',
resources: null,
},
});
detectSIWEMock.mockReturnValue({ isSIWEMessage: true } as SIWEMessage);

// @ts-expect-error Passing invalid input for testing purposes
await middleware(req, undefined, () => undefined);
Expand Down
64 changes: 64 additions & 0 deletions app/scripts/lib/signature/util.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import type { SignatureController } from '@metamask/signature-controller';
import type {
OriginalRequest,
TypedMessageParams,
} from '@metamask/message-manager';
import { endTrace, TraceName } from '../../../../shared/lib/trace';
import { MESSAGE_TYPE } from '../../../../shared/constants/app';

export type SignatureParams = [TypedMessageParams, OriginalRequest];

export type MessageType = keyof typeof MESSAGE_TYPE;

export type AddSignatureMessageRequest = {
signatureParams: SignatureParams;
signatureController: SignatureController;
};

async function handleSignature(
signatureParams: SignatureParams,
signatureController: SignatureController,
functionName: keyof SignatureController,
) {
const [, signatureRequest] = signatureParams;
const { id } = signatureRequest;
const actionId = id?.toString();

endTrace({ name: TraceName.Middleware, id: actionId });

// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore: Expected 4-5 arguments, but got 2.
const hash = await signatureController[functionName](...signatureParams);

endTrace({ name: TraceName.Signature, id: actionId });

return hash;
}

export async function addTypedMessage({
signatureParams,
signatureController,
}: {
signatureParams: SignatureParams;
signatureController: SignatureController;
}) {
return handleSignature(
signatureParams,
signatureController,
'newUnsignedTypedMessage',
);
}

export async function addPersonalMessage({
signatureParams,
signatureController,
}: {
signatureParams: SignatureParams;
signatureController: SignatureController;
}) {
return handleSignature(
signatureParams,
signatureController,
'newUnsignedPersonalMessage',
);
}
67 changes: 67 additions & 0 deletions app/scripts/lib/signature/utils.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import type { SignatureController } from '@metamask/signature-controller';
import type {
OriginalRequest,
TypedMessageParams,
} from '@metamask/message-manager';
import { endTrace, TraceName } from '../../../../shared/lib/trace';
import { addTypedMessage } from './util';
import type { AddSignatureMessageRequest, SignatureParams } from './util';

jest.mock('../../../../shared/lib/trace', () => ({
...jest.requireActual('../../../../shared/lib/trace'),
endTrace: jest.fn(),
}));

describe('addSignatureMessage', () => {
const idMock = 1234;
const hashMock = 'hash-mock';
const messageParamsMock = {
from: '0x12345',
} as TypedMessageParams;

const originalRequestMock = {
id: idMock,
} as OriginalRequest;

const signatureParamsMock: SignatureParams = [
messageParamsMock,
originalRequestMock,
];
const signatureControllerMock: SignatureController = {
newUnsignedTypedMessage: jest.fn(() => hashMock),
newUnsignedPersonalMessage: jest.fn(() => hashMock),
} as unknown as SignatureController;

beforeEach(() => {
jest.clearAllMocks();
});

it('should return a hash when called with valid parameters', async () => {
const request: AddSignatureMessageRequest = {
signatureParams: signatureParamsMock,
signatureController: signatureControllerMock,
};

const result = await addTypedMessage(request);
expect(result).toBe(hashMock);
});

it('should call endTrace with correct parameters', async () => {
const request: AddSignatureMessageRequest = {
signatureParams: signatureParamsMock,
signatureController: signatureControllerMock,
};

await addTypedMessage(request);

expect(endTrace).toHaveBeenCalledTimes(2);
expect(endTrace).toHaveBeenCalledWith({
name: TraceName.Middleware,
id: idMock.toString(),
});
expect(endTrace).toHaveBeenCalledWith({
name: TraceName.Signature,
id: idMock.toString(),
});
});
});
41 changes: 25 additions & 16 deletions app/scripts/metamask-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,9 @@ import { snapKeyringBuilder, getAccountsBySnapId } from './lib/snap-keyring';
///: END:ONLY_INCLUDE_IF
import { encryptorFactory } from './lib/encryptor-factory';
import { addDappTransaction, addTransaction } from './lib/transaction/util';
///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
import { addTypedMessage, addPersonalMessage } from './lib/signature/util';
///: END:ONLY_INCLUDE_IF
import { LatticeKeyringOffscreen } from './lib/offscreen-bridge/lattice-offscreen-keyring';
import PREINSTALLED_SNAPS from './snaps/preinstalled-snaps';
import { WeakRefObjectMap } from './lib/WeakRefObjectMap';
Expand Down Expand Up @@ -1956,6 +1959,7 @@ export default class MetamaskController extends EventEmitter {
getAllState: this.getState.bind(this),
getCurrentChainId: () =>
getCurrentChainId({ metamask: this.networkController.state }),
trace,
});

this.signatureController.hub.on(
Expand Down Expand Up @@ -2248,22 +2252,27 @@ export default class MetamaskController extends EventEmitter {
),
// msg signing
///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
processTypedMessage:
this.signatureController.newUnsignedTypedMessage.bind(
this.signatureController,
),
processTypedMessageV3:
this.signatureController.newUnsignedTypedMessage.bind(
this.signatureController,
),
processTypedMessageV4:
this.signatureController.newUnsignedTypedMessage.bind(
this.signatureController,
),
processPersonalMessage:
this.signatureController.newUnsignedPersonalMessage.bind(
this.signatureController,
),

processTypedMessage: (...args) =>
addTypedMessage({
signatureController: this.signatureController,
signatureParams: args,
}),
processTypedMessageV3: (...args) =>
addTypedMessage({
signatureController: this.signatureController,
signatureParams: args,
}),
processTypedMessageV4: (...args) =>
addTypedMessage({
signatureController: this.signatureController,
signatureParams: args,
}),
processPersonalMessage: (...args) =>
addPersonalMessage({
signatureController: this.signatureController,
signatureParams: args,
}),
///: END:ONLY_INCLUDE_IF

///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -351,8 +351,9 @@
"@metamask/rpc-errors": "^6.2.1",
"@metamask/safe-event-emitter": "^3.1.1",
"@metamask/scure-bip39": "^2.0.3",
"@metamask/selected-network-controller": "^15.0.2",
"@metamask/selected-network-controller": "^18.0.1",
"@metamask/signature-controller": "^19.0.0",
"@metamask/signature-controller": "^19.1.0",
"@metamask/smart-transactions-controller": "^13.0.0",
"@metamask/snaps-controllers": "^9.7.0",
"@metamask/snaps-execution-environments": "^6.7.2",
Expand Down
8 changes: 8 additions & 0 deletions shared/constants/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -124,3 +124,11 @@ export const FIREFOX_BUILD_IDS = [
] as const;

export const UNKNOWN_TICKER_SYMBOL = 'UNKNOWN';

export const TRACE_ENABLED_SIGN_METHODS = [
MESSAGE_TYPE.ETH_SIGN_TYPED_DATA,
MESSAGE_TYPE.ETH_SIGN_TYPED_DATA_V1,
MESSAGE_TYPE.ETH_SIGN_TYPED_DATA_V3,
MESSAGE_TYPE.ETH_SIGN_TYPED_DATA_V4,
MESSAGE_TYPE.PERSONAL_SIGN,
];
1 change: 1 addition & 0 deletions shared/lib/trace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export enum TraceName {
NotificationDisplay = 'Notification Display',
PPOMValidation = 'PPOM Validation',
SetupStore = 'Setup Store',
Signature = 'Signature',
Transaction = 'Transaction',
UIStartup = 'UI Startup',
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { Route, Switch, useHistory, useParams } from 'react-router-dom';
import {
ENVIRONMENT_TYPE_NOTIFICATION,
ORIGIN_METAMASK,
TRACE_ENABLED_SIGN_METHODS,
} from '../../../../shared/constants/app';
import Loading from '../../../components/ui/loading-screen';
import {
Expand Down Expand Up @@ -105,11 +106,15 @@ const ConfirmTransaction = () => {
return undefined;
}

const traceId = TRACE_ENABLED_SIGN_METHODS.includes(type)
? transaction.msgParams?.requestId?.toString()
: id;

return await endBackgroundTrace({
name: TraceName.NotificationDisplay,
id,
id: traceId,
});
}, [id, isNotification]);
}, [id, isNotification, type, transaction.msgParams]);

const transactionId = id;
const isValidTokenMethod = isTokenMethodAction(type);
Expand Down
16 changes: 8 additions & 8 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -6213,20 +6213,20 @@ __metadata:
languageName: node
linkType: hard

"@metamask/signature-controller@npm:^19.0.0":
version: 19.0.0
resolution: "@metamask/signature-controller@npm:19.0.0"
"@metamask/signature-controller@npm:^19.1.0":
version: 19.1.0
resolution: "@metamask/signature-controller@npm:19.1.0"
dependencies:
"@metamask/base-controller": "npm:^7.0.0"
"@metamask/controller-utils": "npm:^11.1.0"
"@metamask/message-manager": "npm:^10.0.3"
"@metamask/base-controller": "npm:^7.0.1"
"@metamask/controller-utils": "npm:^11.3.0"
"@metamask/message-manager": "npm:^10.1.1"
"@metamask/utils": "npm:^9.1.0"
lodash: "npm:^4.17.21"
peerDependencies:
"@metamask/approval-controller": ^7.0.0
"@metamask/keyring-controller": ^17.0.0
"@metamask/logging-controller": ^6.0.0
checksum: 10/9eec874bddee00a969a0231367c55c2b1768ad029c8125929603544ddc94b1e7c833457e39aa0aa5fed19608cb68633f0a90ca40a5639a8d6e2c84dbf9756feb
checksum: 10/ac01b4ba6708e2e74b92ef1c5d4fb9aeff06ae2bd3b445fe8a10bc8e84641ad3bed6fb245f0303ef9d13b7458d022ef07d5ce211a05b14e1ad5ce44ad49cd4ec
languageName: node
linkType: hard

Expand Down Expand Up @@ -26146,7 +26146,7 @@ __metadata:
"@metamask/safe-event-emitter": "npm:^3.1.1"
"@metamask/scure-bip39": "npm:^2.0.3"
"@metamask/selected-network-controller": "npm:^18.0.1"
"@metamask/signature-controller": "npm:^19.0.0"
"@metamask/signature-controller": "npm:^19.1.0"
"@metamask/smart-transactions-controller": "npm:^13.0.0"
"@metamask/snaps-controllers": "npm:^9.7.0"
"@metamask/snaps-execution-environments": "npm:^6.7.2"
Expand Down

0 comments on commit f36a831

Please sign in to comment.