Skip to content

Commit

Permalink
feat: Adding more metrics parameters for signature decoding (#29197)
Browse files Browse the repository at this point in the history
## **Description**

Adding more parameters to signature decoding metrics:

1. decoding_latency - this should capture the amount time it takes to
display the decoding results to users. This is similar to the existing
simulation_latency property for simulations.
2. decoding_description - this should capture any error message sent
along by the API. This is similar to how security_alerts_description
property works for blockaid.
3. decoding_in_progress - this value should be passed to
decoding_response property when the user approves or rejects the
signature request before we display the decoding output.

## **Related issues**

Fixes: MetaMask/MetaMask-planning#3778

## **Manual testing steps**

1. Enable metrics locally, go to test dapp
2. Submit permit sign
3. Check metrics for signature

## **Screenshots/Recordings**
NA

## **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.
  • Loading branch information
jpuri authored Jan 8, 2025
1 parent 40417b0 commit e5ff471
Show file tree
Hide file tree
Showing 5 changed files with 40 additions and 1 deletion.
2 changes: 2 additions & 0 deletions test/e2e/tests/confirmations/signatures/nft-permit.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ describe('Confirmation Signature - NFT Permit @no-mmi', function (this: Suite) {
uiCustomizations: ['redesigned_confirmation', 'permit'],
decodingChangeTypes: ['LISTING', 'RECEIVE'],
decodingResponse: 'CHANGE',
decodingDescription: null,
});

await assertVerifiedResults(driver, publicAddress);
Expand Down Expand Up @@ -117,6 +118,7 @@ describe('Confirmation Signature - NFT Permit @no-mmi', function (this: Suite) {
location: 'confirmation',
decodingChangeTypes: ['LISTING', 'RECEIVE'],
decodingResponse: 'CHANGE',
decodingDescription: null,
});
},
mockSignatureRejectedWithDecoding,
Expand Down
2 changes: 2 additions & 0 deletions test/e2e/tests/confirmations/signatures/permit.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ describe('Confirmation Signature - Permit @no-mmi', function (this: Suite) {
uiCustomizations: ['redesigned_confirmation', 'permit'],
decodingChangeTypes: ['LISTING', 'RECEIVE'],
decodingResponse: 'CHANGE',
decodingDescription: null,
});

await assertVerifiedResults(driver, publicAddress);
Expand Down Expand Up @@ -109,6 +110,7 @@ describe('Confirmation Signature - Permit @no-mmi', function (this: Suite) {
location: 'confirmation',
decodingChangeTypes: ['LISTING', 'RECEIVE'],
decodingResponse: 'CHANGE',
decodingDescription: null,
});
},
mockSignatureRejectedWithDecoding,
Expand Down
18 changes: 18 additions & 0 deletions test/e2e/tests/confirmations/signatures/signature-helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ type AssertSignatureMetricsOptions = {
securityAlertResponse?: string;
decodingChangeTypes?: string[];
decodingResponse?: string;
decodingDescription?: string | null;
};

type SignatureEventProperty = {
Expand All @@ -55,6 +56,7 @@ type SignatureEventProperty = {
eip712_primary_type?: string;
decoding_change_types?: string[];
decoding_response?: string;
decoding_description?: string | null;
ui_customizations?: string[];
location?: string;
};
Expand Down Expand Up @@ -83,6 +85,7 @@ export async function initializePages(driver: Driver) {
* @param securityAlertResponse
* @param decodingChangeTypes
* @param decodingResponse
* @param decodingDescription
*/
function getSignatureEventProperty(
signatureType: string,
Expand All @@ -92,6 +95,7 @@ function getSignatureEventProperty(
securityAlertResponse: string = BlockaidResultType.Loading,
decodingChangeTypes?: string[],
decodingResponse?: string,
decodingDescription?: string | null,
): SignatureEventProperty {
const signatureEventProperty: SignatureEventProperty = {
account_type: 'MetaMask',
Expand All @@ -112,6 +116,7 @@ function getSignatureEventProperty(
if (decodingResponse) {
signatureEventProperty.decoding_change_types = decodingChangeTypes;
signatureEventProperty.decoding_response = decodingResponse;
signatureEventProperty.decoding_description = decodingDescription;
}
return signatureEventProperty;
}
Expand Down Expand Up @@ -147,6 +152,7 @@ export async function assertSignatureConfirmedMetrics({
securityAlertResponse,
decodingChangeTypes,
decodingResponse,
decodingDescription,
}: AssertSignatureMetricsOptions) {
const events = await getEventPayloads(driver, mockedEndpoints);
const signatureEventProperty = getSignatureEventProperty(
Expand All @@ -157,6 +163,7 @@ export async function assertSignatureConfirmedMetrics({
securityAlertResponse,
decodingChangeTypes,
decodingResponse,
decodingDescription,
);

assertSignatureRequestedMetrics(
Expand Down Expand Up @@ -192,6 +199,7 @@ export async function assertSignatureRejectedMetrics({
securityAlertResponse,
decodingChangeTypes,
decodingResponse,
decodingDescription,
}: AssertSignatureMetricsOptions) {
const events = await getEventPayloads(driver, mockedEndpoints);
const signatureEventProperty = getSignatureEventProperty(
Expand All @@ -202,6 +210,7 @@ export async function assertSignatureRejectedMetrics({
securityAlertResponse,
decodingChangeTypes,
decodingResponse,
decodingDescription,
);

assertSignatureRequestedMetrics(
Expand Down Expand Up @@ -314,12 +323,21 @@ function compareDecodingAPIResponse(
expectedProperties.decoding_response,
`${eventName} event properties do not match: decoding_response is ${actualProperties.decoding_response}`,
);
assert.equal(
actualProperties.decoding_description,
expectedProperties.decoding_description,
`${eventName} event properties do not match: decoding_response is ${actualProperties.decoding_description}`,
);
}
// Remove the property from both objects to avoid comparison
delete expectedProperties.decoding_change_types;
delete expectedProperties.decoding_response;
delete expectedProperties.decoding_description;
delete expectedProperties.decoding_latency;
delete actualProperties.decoding_change_types;
delete actualProperties.decoding_response;
delete actualProperties.decoding_description;
delete actualProperties.decoding_latency;
}

export async function clickHeaderInfoBtn(driver: Driver) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ describe('useDecodedSignatureMetrics', () => {
properties: {
decoding_change_types: [],
decoding_response: 'NO_CHANGE',
decoding_description: null,
decoding_latency: 0,
},
});
});
Expand Down Expand Up @@ -115,6 +117,8 @@ describe('useDecodedSignatureMetrics', () => {
properties: {
decoding_change_types: ['APPROVE'],
decoding_response: 'CHANGE',
decoding_description: null,
decoding_latency: 0,
},
});
});
Expand Down Expand Up @@ -149,6 +153,8 @@ describe('useDecodedSignatureMetrics', () => {
properties: {
decoding_change_types: [],
decoding_response: 'SOME_ERROR',
decoding_description: 'some message',
decoding_latency: 0,
},
});
});
Expand Down
13 changes: 12 additions & 1 deletion ui/pages/confirmations/hooks/useDecodedSignatureMetrics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { useEffect } from 'react';

import { SignatureRequestType } from '../types/confirm';
import { useConfirmContext } from '../context/confirm';
import { useLoadingTime } from '../components/simulation-details/useLoadingTime';
import { useSignatureEventFragment } from './useSignatureEventFragment';

enum DecodingResponseType {
Expand All @@ -13,8 +14,13 @@ enum DecodingResponseType {
export function useDecodedSignatureMetrics(supportedByDecodingAPI: boolean) {
const { updateSignatureEventFragment } = useSignatureEventFragment();
const { currentConfirmation } = useConfirmContext<SignatureRequestType>();
const { loadingTime, setLoadingComplete } = useLoadingTime();
const { decodingLoading, decodingData } = currentConfirmation;

if (decodingLoading === false) {
setLoadingComplete();
}

const decodingChangeTypes = (decodingData?.stateChanges ?? []).map(
(change: DecodingDataStateChange) => change.changeType,
);
Expand All @@ -32,14 +38,19 @@ export function useDecodedSignatureMetrics(supportedByDecodingAPI: boolean) {

updateSignatureEventFragment({
properties: {
decoding_response: decodingResponse,
decoding_change_types: decodingChangeTypes,
decoding_description: decodingData?.error?.message ?? null,
decoding_latency: loadingTime ?? null,
decoding_response: decodingLoading
? 'decoding_in_progress'
: decodingResponse,
},
});
}, [
decodingResponse,
decodingLoading,
decodingChangeTypes,
loadingTime,
updateSignatureEventFragment,
]);
}

0 comments on commit e5ff471

Please sign in to comment.