-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: Add signature tracing #11453
Changes from all commits
83f6750
15c179a
c6b4d38
4446959
7296ae0
d057315
7e7823e
ea0edc6
a5411b0
3971aa6
c6109d4
7a78eaf
db91a0d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'; | ||
|
@@ -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; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
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); | ||
}); | ||
}); |
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(); | ||
}; | ||
} |
There was a problem hiding this comment.
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