Skip to content

Commit ac3f905

Browse files
vinistevamvinistevam
authored
fix: enhance metrics for Transaction/Signatures events (Shield) cp-13.10.0 (#37804)
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** This PR aims to add metrics for transaction shield. - `shield_result_response_latency_ms`: latency to receive response from Shield Upgrading `@metamask/shield-controller` from `^1.2.0` to ` ^2.1.0`, to support tracking latency of the requests. <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/37804?quickstart=1) ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: null ## **Related issues** Fixes: MetaMask/MetaMask-planning#6136 ## **Manual testing steps** 1. Start the app 2. Switch to Sepolia network 3. Mint some USDC on sepolia on [this smart contract](https://sepolia.etherscan.io/token/0xaa6e4b8e84a2c0957c8a8efb12a64d2f06700142#writeContract) (at least 80 for annual plan, 96 for monthly). 4. Import the token manually in the assets list 5. Navigate to Settings > Transaction Shield 6. Select pay with crypto, and Continue 7. The confirmation screen should appear and new shield properties should be added to the transaction/signature events 8. Check Segment ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Adds shield_result_response_latency_ms to transaction/signature event fragments, introduces getCoverageMetrics selector, updates tests, and bumps @metamask/shield-controller to ^2.1.0. > > - **Alerts/Telemetry**: > - `useShieldCoverageAlert` now records `shield_result_response_latency_ms` (from `metrics.latency` or `"N/A"`) in event fragments. > - Pulls metrics via new selector `getCoverageMetrics`; refines `getShieldResult` default handling and effect dependencies. > - **Selectors**: > - Add types `CoverageStatusResult`, `CoverageMetrics` and helper `getFirstCoverageResult`. > - Implement `getCoverageMetrics` and refactor `getCoverageStatus` to use shared helper. > - **Tests**: > - Extend `useShieldCoverageAlert.test` to assert latency propagation for tx/signature paths and various statuses. > - Add comprehensive tests for `getCoverageMetrics` and edge cases in `getCoverageStatus`. > - **Dependencies**: > - Upgrade `@metamask/shield-controller` to `^2.1.0`. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit bab9a95. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 9793ca4 commit ac3f905

File tree

6 files changed

+224
-60
lines changed

6 files changed

+224
-60
lines changed

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -352,7 +352,7 @@
352352
"@metamask/scure-bip39": "^2.0.3",
353353
"@metamask/seedless-onboarding-controller": "^5.0.0",
354354
"@metamask/selected-network-controller": "^25.0.0",
355-
"@metamask/shield-controller": "^1.2.0",
355+
"@metamask/shield-controller": "^2.1.0",
356356
"@metamask/signature-controller": "^35.0.0",
357357
"@metamask/smart-transactions-controller": "^20.0.0",
358358
"@metamask/snaps-controllers": "^16.1.1",

‎ui/pages/confirmations/hooks/alerts/useShieldCoverageAlert.test.ts‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ describe('useShieldCoverageAlert', () => {
5454
status?: string,
5555
reasonCode = 'E104',
5656
isTransaction: boolean = true,
57+
latency: number | string = 'N/A',
5758
): Record<string, unknown> => {
5859
const mockId = '123';
5960
const baseState = isTransaction
@@ -84,6 +85,9 @@ describe('useShieldCoverageAlert', () => {
8485
{
8586
status,
8687
reasonCode,
88+
metrics: {
89+
latency,
90+
},
8791
},
8892
],
8993
},
@@ -157,7 +161,7 @@ describe('useShieldCoverageAlert', () => {
157161
});
158162

159163
it('updates transaction event fragment with covered status', () => {
160-
const state = getStateWithCoverage('covered', 'E104');
164+
const state = getStateWithCoverage('covered', 'E104', true, 150);
161165
renderHookWithConfirmContextProvider(() => useShieldCoverageAlert(), state);
162166
expect(updateTransactionEventFragmentMock).toHaveBeenCalledWith(
163167
expect.objectContaining({
@@ -166,6 +170,8 @@ describe('useShieldCoverageAlert', () => {
166170
shield_result: 'covered',
167171
// eslint-disable-next-line @typescript-eslint/naming-convention
168172
shield_reason: 'shieldCoverageAlertCovered',
173+
// eslint-disable-next-line @typescript-eslint/naming-convention
174+
shield_result_response_latency_ms: 150,
169175
},
170176
}),
171177
expect.anything(),
@@ -182,6 +188,8 @@ describe('useShieldCoverageAlert', () => {
182188
shield_result: 'not_covered_malicious',
183189
// eslint-disable-next-line @typescript-eslint/naming-convention
184190
shield_reason: 'shieldCoverageAlertMessagePotentialRisks',
191+
// eslint-disable-next-line @typescript-eslint/naming-convention
192+
shield_result_response_latency_ms: 'N/A',
185193
},
186194
}),
187195
expect.anything(),
@@ -198,6 +206,8 @@ describe('useShieldCoverageAlert', () => {
198206
shield_result: 'not_covered',
199207
// eslint-disable-next-line @typescript-eslint/naming-convention
200208
shield_reason: 'shieldCoverageAlertMessagePotentialRisks',
209+
// eslint-disable-next-line @typescript-eslint/naming-convention
210+
shield_result_response_latency_ms: 'N/A',
201211
},
202212
}),
203213
expect.anything(),
@@ -214,14 +224,16 @@ describe('useShieldCoverageAlert', () => {
214224
shield_result: 'loading',
215225
// eslint-disable-next-line @typescript-eslint/naming-convention
216226
shield_reason: 'shieldCoverageAlertMessagePotentialRisks',
227+
// eslint-disable-next-line @typescript-eslint/naming-convention
228+
shield_result_response_latency_ms: 'N/A',
217229
},
218230
}),
219231
expect.anything(),
220232
);
221233
});
222234

223235
it('updates signature event fragment with correct metrics', () => {
224-
const state = getStateWithCoverage('covered', 'E104', false);
236+
const state = getStateWithCoverage('covered', 'E104', false, 200);
225237
renderHookWithConfirmContextProvider(() => useShieldCoverageAlert(), state);
226238

227239
expect(updateSignatureEventFragmentMock).toHaveBeenCalledWith(
@@ -231,6 +243,8 @@ describe('useShieldCoverageAlert', () => {
231243
shield_result: 'covered',
232244
// eslint-disable-next-line @typescript-eslint/naming-convention
233245
shield_reason: 'shieldCoverageAlertCovered',
246+
// eslint-disable-next-line @typescript-eslint/naming-convention
247+
shield_result_response_latency_ms: 200,
234248
},
235249
}),
236250
);

‎ui/pages/confirmations/hooks/alerts/useShieldCoverageAlert.ts‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
} from '../../../../helpers/constants/design-system';
1414
import { useI18nContext } from '../../../../hooks/useI18nContext';
1515
import {
16+
getCoverageMetrics,
1617
getCoverageStatus,
1718
ShieldState,
1819
} from '../../../../selectors/shield/coverage';
@@ -25,6 +26,8 @@ import { useSignatureEventFragment } from '../useSignatureEventFragment';
2526
import { useTransactionEventFragment } from '../useTransactionEventFragment';
2627
import { ShieldCoverageAlertMessage } from './transactions/ShieldCoverageAlertMessage';
2728

29+
const N_A = 'N/A';
30+
2831
const getModalBodyStr = (reasonCode: string | undefined) => {
2932
// grouping codes with a fallthrough pattern is not allowed by the linter
3033
let modalBodyStr: string;
@@ -169,10 +172,11 @@ const getShieldResult = (
169172
return 'not_covered_malicious';
170173
case 'unknown':
171174
return 'not_covered';
172-
case undefined:
173-
case 'not_shown':
174-
return 'loading';
175175
default:
176+
// Returns 'loading' for:
177+
// - undefined: coverage check not yet initiated or in progress
178+
// - 'not_shown': coverage didn't load before user confirmed
179+
// - any unexpected values: fail safe to loading state
176180
return 'loading';
177181
}
178182
};
@@ -190,6 +194,9 @@ export function useShieldCoverageAlert(): Alert[] {
190194
const { reasonCode, status } = useSelector((state) =>
191195
getCoverageStatus(state as ShieldState, id),
192196
);
197+
const metrics = useSelector((state) =>
198+
getCoverageMetrics(state as ShieldState, id),
199+
);
193200

194201
const { isEnabled, isPaused } = useEnableShieldCoverageChecks();
195202

@@ -221,6 +228,8 @@ export function useShieldCoverageAlert(): Alert[] {
221228
shield_result: getShieldResult(status),
222229
// eslint-disable-next-line @typescript-eslint/naming-convention
223230
shield_reason: modalBodyStr,
231+
// eslint-disable-next-line @typescript-eslint/naming-convention
232+
shield_result_response_latency_ms: metrics?.latency ?? N_A,
224233
};
225234

226235
if (isSignatureTransactionType(currentConfirmation)) {
@@ -237,12 +246,13 @@ export function useShieldCoverageAlert(): Alert[] {
237246
}
238247
}
239248
}, [
240-
status,
241-
modalBodyStr,
242-
isEnabled,
243-
isPaused,
244249
currentConfirmation,
245250
id,
251+
isEnabled,
252+
isPaused,
253+
metrics?.latency,
254+
modalBodyStr,
255+
status,
246256
updateSignatureEventFragment,
247257
updateTransactionEventFragment,
248258
]);

‎ui/selectors/shield/coverage.test.ts‎

Lines changed: 153 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1,54 +1,172 @@
11
import type { CoverageStatus } from '@metamask/shield-controller';
2-
import { getCoverageStatus, ShieldState } from './coverage';
2+
import {
3+
CoverageMetrics,
4+
getCoverageMetrics,
5+
getCoverageStatus,
6+
ShieldState,
7+
} from './coverage';
38

49
describe('shield coverage selectors', () => {
510
const confirmationId = 'abc123';
611

7-
it('returns undefined when there are no coverage results', () => {
8-
const state = {
12+
const createStateWithResult = (result: {
13+
status?: CoverageStatus;
14+
reasonCode?: string;
15+
metrics?: { latency?: number };
16+
}): ShieldState =>
17+
({
918
metamask: {
10-
coverageResults: {},
19+
coverageResults: {
20+
[confirmationId]: {
21+
results: [result],
22+
},
23+
},
1124
},
12-
} as unknown as ShieldState;
25+
}) as unknown as ShieldState;
26+
27+
describe('getCoverageStatus', () => {
28+
it('returns undefined when there are no coverage results', () => {
29+
const state = {
30+
metamask: {
31+
coverageResults: {},
32+
},
33+
} as unknown as ShieldState;
34+
35+
const result = getCoverageStatus(state, confirmationId);
36+
expect(result).toEqual({ status: undefined, reasonCode: undefined });
37+
});
38+
39+
it('returns undefined when results array is empty', () => {
40+
const state = {
41+
metamask: {
42+
coverageResults: {
43+
[confirmationId]: { results: [] },
44+
},
45+
},
46+
} as unknown as ShieldState;
47+
48+
const result = getCoverageStatus(state, confirmationId);
49+
expect(result).toEqual({ status: undefined, reasonCode: undefined });
50+
});
51+
52+
it('returns status and reasonCode from the first result', () => {
53+
const status: CoverageStatus = 'covered';
54+
const reasonCode = 'ok';
55+
const state = {
56+
metamask: {
57+
coverageResults: {
58+
[confirmationId]: {
59+
results: [
60+
{
61+
status,
62+
reasonCode,
63+
},
64+
{ status: 'other', reasonCode: 'ignored' },
65+
],
66+
},
67+
},
68+
},
69+
} as unknown as ShieldState;
1370

14-
const result = getCoverageStatus(state, confirmationId);
15-
expect(result).toEqual({ status: undefined, reasonCode: undefined });
71+
const result = getCoverageStatus(state, confirmationId);
72+
expect(result.status).toBe(status);
73+
expect(result.reasonCode).toBe(reasonCode);
74+
});
1675
});
1776

18-
it('returns undefined when results array is empty', () => {
19-
const state = {
20-
metamask: {
21-
coverageResults: {
22-
[confirmationId]: { results: [] },
77+
describe('getCoverageMetrics', () => {
78+
it('returns undefined when there are no coverage results', () => {
79+
const state = {
80+
metamask: {
81+
coverageResults: {},
2382
},
83+
} as unknown as ShieldState;
84+
85+
const result = getCoverageMetrics(state, confirmationId);
86+
expect(result).toBeUndefined();
87+
});
88+
89+
const metricsTestCases = [
90+
{
91+
description: 'metrics with latency',
92+
metrics: { latency: 123 },
93+
expectedLatency: 123,
94+
},
95+
{
96+
description: 'metrics with latency value of 0',
97+
metrics: { latency: 0 },
98+
expectedLatency: 0,
99+
},
100+
{
101+
description: 'empty metrics object',
102+
metrics: {},
103+
expectedLatency: undefined,
24104
},
25-
} as unknown as ShieldState;
105+
{
106+
description: 'large latency values',
107+
metrics: { latency: 999999 },
108+
expectedLatency: 999999,
109+
},
110+
];
111+
// @ts-expect-error This function is missing from the Mocha type definitions
112+
it.each(metricsTestCases)(
113+
'returns $description',
114+
({
115+
metrics,
116+
expectedLatency,
117+
}: {
118+
metrics: CoverageMetrics;
119+
expectedLatency?: number;
120+
}) => {
121+
const state = createStateWithResult({
122+
status: 'covered',
123+
metrics,
124+
});
26125

27-
const result = getCoverageStatus(state, confirmationId);
28-
expect(result).toEqual({ status: undefined, reasonCode: undefined });
29-
});
126+
const result = getCoverageMetrics(state, confirmationId);
30127

31-
it('returns status and reasonCode from the first result', () => {
32-
const status: CoverageStatus = 'covered';
33-
const reasonCode = 'ok';
34-
const state = {
35-
metamask: {
36-
coverageResults: {
37-
[confirmationId]: {
38-
results: [
39-
{
40-
status,
41-
reasonCode,
42-
},
43-
{ status: 'other', reasonCode: 'ignored' },
44-
],
128+
expect(result).toEqual(metrics);
129+
expect(result?.latency).toBe(expectedLatency);
130+
},
131+
);
132+
133+
it('returns undefined when metrics property is missing', () => {
134+
const state = createStateWithResult({
135+
status: 'covered',
136+
reasonCode: 'ok',
137+
});
138+
139+
const result = getCoverageMetrics(state, confirmationId);
140+
141+
expect(result).toBeUndefined();
142+
});
143+
144+
it('returns metrics from the first result only', () => {
145+
const metrics = { latency: 123 };
146+
const state = {
147+
metamask: {
148+
coverageResults: {
149+
[confirmationId]: {
150+
results: [
151+
{
152+
status: 'covered',
153+
reasonCode: 'ok',
154+
metrics,
155+
},
156+
{
157+
status: 'unknown',
158+
metrics: { latency: 456 },
159+
},
160+
],
161+
},
45162
},
46163
},
47-
},
48-
} as unknown as ShieldState;
164+
} as unknown as ShieldState;
165+
166+
const result = getCoverageMetrics(state, confirmationId);
49167

50-
const result = getCoverageStatus(state, confirmationId);
51-
expect(result.status).toBe(status);
52-
expect(result.reasonCode).toBe(reasonCode);
168+
expect(result).toEqual(metrics);
169+
expect(result?.latency).toBe(123);
170+
});
53171
});
54172
});

0 commit comments

Comments
 (0)