-
-
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
base: main
Are you sure you want to change the base?
feat: Add signature tracing #11453
Conversation
Bitrise✅✅✅ Commit hash: 7e7823e Note
|
@@ -14,6 +16,11 @@ const SignatureApproval = () => { | |||
}); | |||
}, [onConfirm]); | |||
|
|||
useAsyncResult(async () => await endTrace({ |
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.
Why do we need useAsyncResult
here as endTrace
isn't asynchronous?
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.
That's right, fixed
useAsyncResult(async () => await endTrace({ | ||
name: TraceName.NotificationDisplay, | ||
id: approvalRequest?.requestData?.requestId, | ||
}), [approvalRequest?.requestData?.requestId]); |
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.
Is it worth defining a local variable such as signatureRequestId
?
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.
Done
@@ -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( |
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.
Maybe not in this PR, but I wonder if we'd avoid a lot of duplication by using a generic function for all the signing methods?
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.
I tried that in the beginning but it was looking load of changes so didn't proceed. I definitely agree that it's worth doing some cleaning up on these rpc methods.
a5411b0
Bitrise❌❌❌ Commit hash: 3971aa6 Note
Tip
|
…equests-in-mobile
Bitrise❌❌❌ Commit hash: c6109d4 Note
Tip
|
…equests-in-mobile
Bitrise❌❌❌ Commit hash: 7a78eaf Note
Tip
|
…equests-in-mobile
Bitrise❌❌❌ Commit hash: db91a0d Note
Tip
|
Quality Gate passedIssues Measures |
Description
createTracingMiddleware
to start tracing for defined type of messages.trace
callback toSignatureController
.Notification Display
trace after signature confirmation landed on users screen.Related issues
Fixes: https://github.com/MetaMask/mobile-planning/issues/1883
Manual testing steps
MM_SENTRY_DSN_DEV
to developer Sentry dsn, see the value here https://github.com/MetaMask/metamask-extension/blob/develop/.metamaskrc.distSignature
trace will appear on this link: https://metamask.sentry.io/traces/?project=273496&query=signature%3ATransaction&source=traces&statsPeriod=1hScreenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist