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

feat: Add performance metrics for signature requests #26967

Merged
merged 25 commits into from
Sep 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
9dbf5c3
Temp
OGPoyraz Sep 6, 2024
21c9bb0
Add Notification Display trace end
OGPoyraz Sep 6, 2024
617203e
Add unit test for signature util
OGPoyraz Sep 6, 2024
2eae7c7
Enable tracing for all type of signatures
OGPoyraz Sep 9, 2024
39b93e1
Merge branch 'develop' into feat/signature-performance-traces-impleme…
OGPoyraz Sep 9, 2024
b09d425
Fix suggestions
OGPoyraz Sep 10, 2024
7c0a741
remove unused import
OGPoyraz Sep 12, 2024
b5528fe
Fix suggestions
OGPoyraz Sep 16, 2024
5a68eff
Merge branch 'develop' into feat/signature-performance-traces-impleme…
OGPoyraz Sep 17, 2024
b42e389
Update dependencies
OGPoyraz Sep 17, 2024
20c3d7e
dedupe
OGPoyraz Sep 17, 2024
bfda177
Update test
OGPoyraz Sep 17, 2024
f91ed1d
Fix lint
OGPoyraz Sep 17, 2024
0d7eea2
Fix unit test
OGPoyraz Sep 17, 2024
c59d0e6
Fix ppom test
OGPoyraz Sep 17, 2024
9097c62
fix mmi-build
OGPoyraz Sep 17, 2024
905ab08
Merge branch 'develop' into feat/signature-performance-traces-impleme…
OGPoyraz Sep 17, 2024
5cc0021
Merge branch 'develop' into feat/signature-performance-traces-impleme…
OGPoyraz Sep 18, 2024
0bf8712
Merge branch 'develop' into feat/signature-performance-traces-impleme…
OGPoyraz Sep 19, 2024
baa6f61
Merge branch 'develop' into feat/signature-performance-traces-impleme…
OGPoyraz Sep 19, 2024
dde207e
Merge branch 'develop' into feat/signature-performance-traces-impleme…
OGPoyraz Sep 23, 2024
2d743ac
Fix tests
OGPoyraz Sep 23, 2024
f08fdb2
Update package.json
OGPoyraz Sep 24, 2024
8edcb4d
Add callback
OGPoyraz Sep 26, 2024
5ccb223
Merge branch 'develop' into feat/signature-performance-traces-impleme…
OGPoyraz Sep 26, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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);
matthewwalsh0 marked this conversation as resolved.
Show resolved Hide resolved

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) =>
matthewwalsh0 marked this conversation as resolved.
Show resolved Hide resolved
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