-
-
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
Conversation
Bitrise✅✅✅ Commit hash: 7e7823e Note
|
app/components/Approvals/SignatureApproval/SignatureApproval.tsx
Outdated
Show resolved
Hide resolved
app/components/Approvals/SignatureApproval/SignatureApproval.tsx
Outdated
Show resolved
Hide resolved
a5411b0
…equests-in-mobile
…equests-in-mobile
…equests-in-mobile
Quality Gate passedIssues Measures |
Bitrise✅✅✅ Commit hash: db91a0d Note
|
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.
some optimisations suggested
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.
LGTM
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.
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.
@@ -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 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.
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
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