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 signature tracing #11453

Merged
Copy link
Contributor

Choose a reason for hiding this comment

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

S: add a test case and spy on endTrace in app/components/Approvals/SignatureApproval/SignatureApproval.test.tsx

Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import React, { useCallback } from 'react';
import React, { useCallback, useEffect } from 'react';
import useApprovalRequest from '../../Views/confirmations/hooks/useApprovalRequest';
import { ApprovalTypes } from '../../../core/RPCMethods/RPCMethodMiddleware';
import SignatureRequestRoot from '../../Views/confirmations/components/SignatureRequest/Root';
import { endTrace, TraceName } from '../../../util/trace';

const SignatureApproval = () => {
const { approvalRequest, onReject, onConfirm } = useApprovalRequest();
const signatureRequestId = approvalRequest?.requestData?.requestId;

const onSignConfirm = useCallback(async () => {
await onConfirm({
Expand All @@ -14,6 +16,13 @@ const SignatureApproval = () => {
});
}, [onConfirm]);

useEffect(() => {
endTrace({
name: TraceName.NotificationDisplay,
id: signatureRequestId,
});
}, [signatureRequestId]);

const messageParams =
approvalRequest &&
[ApprovalTypes.PERSONAL_SIGN, ApprovalTypes.ETH_SIGN_TYPED_DATA].includes(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import Modal from 'react-native-modal';
import React, { useEffect, useState } from 'react';
import { StyleSheet } from 'react-native';
import { useNavigation } from '@react-navigation/native';
import { useSelector } from 'react-redux';
Copy link
Contributor

Choose a reason for hiding this comment

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

C: This should not be in the PR. If it was a file you worked on, reorganising imports is fine, but if it's only the import change, you may leave it as is.

import setSignatureRequestSecurityAlertResponse from '../../../../../../actions/signatureRequest';
import { store } from '../../../../../../store';
import { useTheme } from '../../../../../../util/theme';
Expand All @@ -10,7 +11,6 @@ import PersonalSign from '../../PersonalSign';
import TypedSign from '../../TypedSign';
import { MessageParams } from '../types';
import { ApprovalTypes } from '../../../../../../core/RPCMethods/RPCMethodMiddleware';
import { useSelector } from 'react-redux';

interface RootProps {
messageParams?: MessageParams;
Expand Down
4 changes: 4 additions & 0 deletions app/core/BackgroundBridge/BackgroundBridge.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import { NetworkStatus } from '@metamask/network-controller';
import { NETWORK_ID_LOADING } from '../redux/slices/inpageProvider';
import createUnsupportedMethodMiddleware from '../RPCMethods/createUnsupportedMethodMiddleware';
import createLegacyMethodMiddleware from '../RPCMethods/createLegacyMethodMiddleware';
import createTracingMiddleware from '../createTracingMiddleware';

const legacyNetworkId = () => {
const { networksMetadata, selectedNetworkClientId } =
Expand Down Expand Up @@ -442,6 +443,9 @@ export class BackgroundBridge extends EventEmitter {
}),
);

// Sentry tracing middleware
engine.push(createTracingMiddleware());

// Append PermissionController middleware
engine.push(
Engine.context.PermissionController.createPermissionMiddleware({
Expand Down
4 changes: 4 additions & 0 deletions app/core/Engine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ import {
SignatureController,
SignatureControllerActions,
SignatureControllerEvents,
SignatureControllerOptions,
} from '@metamask/signature-controller';
import { hasProperty, Hex, Json } from '@metamask/utils';
// TODO: Export this type from the package directly
Expand Down Expand Up @@ -250,6 +251,7 @@ import { keyringSnapPermissionsBuilder } from './SnapKeyring/keyringSnapsPermiss
import { HandleSnapRequestArgs } from './Snaps/types';
import { handleSnapRequest } from './Snaps/utils';
///: END:ONLY_INCLUDE_IF
import { trace } from '../util/trace';

const NON_EMPTY = 'NON_EMPTY';

Expand Down Expand Up @@ -1602,6 +1604,8 @@ class Engine {
networkController.getNetworkClientById(
networkController?.state.selectedNetworkClientId,
).configuration.chainId,
// This casting expected due to mismatch of browser and react-native version of Sentry traceContext
trace: trace as unknown as SignatureControllerOptions['trace'],
}),
loggingController,
///: BEGIN:ONLY_INCLUDE_IF(preinstalled-snaps,external-snaps)
Expand Down
2 changes: 2 additions & 0 deletions app/core/RPCMethods/RPCMethodMiddleware.test.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

C: This test file covers the new code changes, but doesn't test it.
If it's important to make sure trace is called, you could spy on it.

Original file line number Diff line number Diff line change
Expand Up @@ -1257,6 +1257,7 @@ describe('getRpcMethodMiddleware', () => {
from: addressMock,
meta: expect.any(Object),
origin: hostMock,
requestId: 1,
});
});

Expand Down Expand Up @@ -1306,6 +1307,7 @@ describe('getRpcMethodMiddleware', () => {
from: addressMock,
meta: expect.any(Object),
origin: hostMock,
requestId: 1,
},
expect.any(Object),
version,
Expand Down
34 changes: 30 additions & 4 deletions app/core/RPCMethods/RPCMethodMiddleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import Logger from '../../../app/util/Logger';
import DevLogger from '../SDKConnect/utils/DevLogger';
import { addTransaction } from '../../util/transaction-controller';
import Routes from '../../constants/navigation/Routes';
import { endTrace, trace, TraceName } from '../../util/trace';

// TODO: Replace "any" with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Expand Down Expand Up @@ -232,6 +233,7 @@ const generateRawSignature = async ({
// eslint-disable-next-line @typescript-eslint/no-explicit-any
checkTabActive: any;
}) => {
endTrace({ name: TraceName.Middleware, id: req.id });
const { SignatureController } = Engine.context;

const pageMeta = {
Expand Down Expand Up @@ -260,6 +262,7 @@ const generateRawSignature = async ({
{
data: req.params[1],
from: req.params[0],
requestId: req.id,
...pageMeta,
channelId,
origin: hostname,
Expand All @@ -271,6 +274,7 @@ const generateRawSignature = async ({
parseJsonData: false,
},
);
endTrace({ name: TraceName.Signature, id: req.id });

return rawSig;
};
Expand Down Expand Up @@ -538,6 +542,7 @@ export const getRpcMethodMiddleware = ({
const params = {
data: firstParam,
from: secondParam,
requestId: req.id,
};

if (resemblesAddress(firstParam) && !resemblesAddress(secondParam)) {
Expand Down Expand Up @@ -567,13 +572,18 @@ export const getRpcMethodMiddleware = ({
});

DevLogger.log(`personal_sign`, params, pageMeta, hostname);
PPOMUtil.validateRequest(req);

trace(
{ name: TraceName.PPOMValidation, parentContext: req.traceContext },
() => PPOMUtil.validateRequest(req),
);

const rawSig = await SignatureController.newUnsignedPersonalMessage({
...params,
...pageMeta,
origin: hostname,
});
endTrace({ name: TraceName.Signature, id: req.id });

res.result = rawSig;
},
Expand All @@ -594,6 +604,7 @@ export const getRpcMethodMiddleware = ({
},

eth_signTypedData: async () => {
endTrace({ name: TraceName.Middleware, id: req.id });
const { SignatureController } = Engine.context;
const pageMeta = {
meta: {
Expand All @@ -616,18 +627,23 @@ export const getRpcMethodMiddleware = ({
isWalletConnect,
});

PPOMUtil.validateRequest(req);
trace(
{ name: TraceName.PPOMValidation, parentContext: req.traceContext },
() => PPOMUtil.validateRequest(req),
);

const rawSig = await SignatureController.newUnsignedTypedMessage(
{
data: req.params[0],
from: req.params[1],
requestId: req.id,
...pageMeta,
origin: hostname,
},
req,
'V1',
);
endTrace({ name: TraceName.Signature, id: req.id });

res.result = rawSig;
},
Expand All @@ -638,7 +654,12 @@ export const getRpcMethodMiddleware = ({
? JSON.parse(req.params[1])
: req.params[1];
const chainId = data.domain.chainId;
PPOMUtil.validateRequest(req);

trace(
{ name: TraceName.PPOMValidation, parentContext: req.traceContext },
() => PPOMUtil.validateRequest(req),
);

res.result = await generateRawSignature({
version: 'V3',
req,
Expand All @@ -659,7 +680,12 @@ export const getRpcMethodMiddleware = ({
eth_signTypedData_v4: async () => {
const data = JSON.parse(req.params[1]);
const chainId = data.domain.chainId;
PPOMUtil.validateRequest(req);

trace(
OGPoyraz marked this conversation as resolved.
Show resolved Hide resolved
{ name: TraceName.PPOMValidation, parentContext: req.traceContext },
() => PPOMUtil.validateRequest(req),
);

res.result = await generateRawSignature({
version: 'V4',
req,
Expand Down
39 changes: 39 additions & 0 deletions app/core/createTracingMiddleware/index.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import type { JsonRpcRequest, PendingJsonRpcResponse } from 'json-rpc-engine';
import { default as createTracingMiddleware, MESSAGE_TYPE } from './index';

const REQUEST_MOCK = {
id: 'testId',
method: MESSAGE_TYPE.PERSONAL_SIGN,
} as JsonRpcRequest<unknown>;
const RESPONSE_MOCK = {} as PendingJsonRpcResponse<unknown>;
const NEXT_MOCK = jest.fn();

jest.mock('../../util/trace', () => ({
...jest.requireActual('../../util/trace'),
trace: jest.fn().mockResolvedValue({}),
}));

describe('createTracingMiddleware', () => {
let request: JsonRpcRequest<unknown> & { traceContext?: unknown };
beforeEach(() => {
jest.clearAllMocks();
request = { ...REQUEST_MOCK };
});

it('adds trace context to request if method is send transaction', async () => {
await createTracingMiddleware()(request, RESPONSE_MOCK, NEXT_MOCK);
expect(request.traceContext).toBeDefined();
});

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

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

expect(request.traceContext).toBeUndefined();
});
it('calls next', async () => {
await createTracingMiddleware()(request, RESPONSE_MOCK, NEXT_MOCK);
expect(NEXT_MOCK).toHaveBeenCalledTimes(1);
});
});
45 changes: 45 additions & 0 deletions app/core/createTracingMiddleware/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import type { JsonRpcRequest, PendingJsonRpcResponse } from 'json-rpc-engine';
import { trace, TraceName } from '../../util/trace';

export const MESSAGE_TYPE = {
ETH_SIGN_TYPED_DATA: 'eth_signTypedData',
ETH_SIGN_TYPED_DATA_V1: 'eth_signTypedData_v1',
ETH_SIGN_TYPED_DATA_V3: 'eth_signTypedData_v3',
ETH_SIGN_TYPED_DATA_V4: 'eth_signTypedData_v4',
PERSONAL_SIGN: 'personal_sign',
};

const METHOD_TYPE_TO_TRACE_NAME: Record<string, TraceName> = {
[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,
};

export default function createTracingMiddleware() {
return async function tracingMiddleware(
req: JsonRpcRequest<unknown> & { traceContext?: unknown },
_res: PendingJsonRpcResponse<unknown>,
next: () => void,
) {
const { id, method } = req;

andreahaku marked this conversation as resolved.
Show resolved Hide resolved
const traceName = METHOD_TYPE_TO_TRACE_NAME[method];

andreahaku marked this conversation as resolved.
Show resolved Hide resolved
if (traceName) {
andreahaku marked this conversation as resolved.
Show resolved Hide resolved
req.traceContext = await trace({
name: traceName,
id: id as string,
});

await trace({
name: TraceName.Middleware,
id: id as string,
parentContext: req.traceContext,
});
}

next();
};
}
3 changes: 3 additions & 0 deletions app/util/trace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ export enum TraceName {
Middleware = 'Middleware',
NestedTest1 = 'Nested Test 1',
NestedTest2 = 'Nested Test 2',
NotificationDisplay = 'Notification Display',
PPOMValidation = 'PPOM Validation',
Signature = 'Signature',
}

const ID_DEFAULT = 'default';
Expand Down
Loading