feat: Mitigate risk for distracted users on queued transactions from different dApps #25852

pedronfigueiredo · 2024-07-16T10:30:58Z

Description

Adds new Banner alert and corresponding metrics event. Also adds metric to navigation when there's are queued confirmations.

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2758

Manual testing steps

Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). 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.

github-actions · 2024-07-16T10:31:12Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

matthewwalsh0 · 2024-07-17T08:42:38Z

app/_locales/en/messages.json

@@ -1920,6 +1920,9 @@
  "etherscanViewOn": {
    "message": "View on Etherscan"
  },
+  "existingRequestsBannerAlertDesc": {
+    "message": "To view and confirm your most recent request, you'll need to approve or reject existing requests first."


Is this a sample or final?

Is the fundamental risk here not the fact that the dApp or chain could have changed without the user realising?

Should that be referenced also?

Raising this with @bschorchit

Is the fundamental risk here not the fact that the dApp or chain could have changed without the user realising?

Wouldn't that be this issue here: https://github.com/MetaMask/MetaMask-planning/issues/2759 , @matthewwalsh0 ?

The current one that Pedro is working on is regarding queued requests initially not showing up until the user rejects or confirms the first. https://github.com/MetaMask/MetaMask-planning/issues/2758

I understand that, but does it not seem obvious to say you have to approve or reject existing requests first? Is the confusion we are trying to address that requests from another dApp aren't yet visible in the navigation?

...e-container/confirm-page-container-navigation/confirm-page-container-navigation.component.js

ui/pages/confirmations/components/confirm/nav/nav.tsx

test/e2e/tests/confirmations/queued-confirmations.spec.ts

jpuri · 2024-07-19T11:55:27Z

ui/pages/confirmations/hooks/useQueuedConfirmationEvents.ts

+      });
+    }
+  }, [JSON.stringify(properties), queuedRequestCount]);
+};


It looks bit unusual to have hook which does not return anything but itself performs task, an option could be we return a callback from this hook which user can invoke.

It's rare, but it feels like a good usage since we can automate the metrics based on dependencies and encapsulate that logic, rather than it being a component problem.

We do something similar in useSimulationMetrics already.

Concerning the documentation, it feels suitable since we are still synchronizing with an external system, in this case Segment.

jpuri · 2024-07-19T11:58:00Z

ui/selectors/approvals.ts

@@ -54,6 +54,10 @@ export function getPendingApprovals(state: ApprovalsMetaMaskState) {
  return Object.values(state.metamask.pendingApprovals ?? []);
 }

+export function pendingApprovalsSortedSelector(state: ApprovalsMetaMaskState) {
+  return getPendingApprovals(state).sort((a1, a2) => a1.time - a2.time);
+}


This may be duplicate:

metamask-extension/ui/pages/confirmations/selectors/confirm.ts

Line 26 in 3454333

export function pendingConfirmationsSortedSelector(

Does the one in confirm.ts not filter to specific types?

Exactly, I opted to create a new one that wouldn't filter any pending approvals.

jpuri · 2024-07-19T12:03:56Z

ui/pages/confirmations/hooks/useQueuedConfirmationEvents.ts

+} from '../../../selectors';
+
+export const useQueuedConfirmationsEvent = (queueType: QueueType) => {
+  const pendingApprovals = useSelector(pendingApprovalsSortedSelector);


This selector may be useful here:

metamask-extension/ui/pages/confirmations/selectors/confirm.ts

Line 36 in 3454333

const internalLatestPendingConfirmationSelector = createSelector(

Thanks Jyoti. However, this selector uses pendingConfirmationsSortedSelector which in turn filters the approval types. We want to show this banner for all confirmation types, including switch ethereum chain and others.

jpuri · 2024-07-19T12:11:29Z

...s/confirmation/components/queued-requests-banner-alert/queued-requests-banner-alert.test.tsx

+
+describe('<QueuedRequestsBannerAlert />', () => {
+  beforeEach(() => {
+    jest.resetAllMocks();


test case does not seems to have mocks

jpuri · 2024-07-19T12:11:41Z

...s/confirmation/components/queued-requests-banner-alert/queued-requests-banner-alert.test.tsx

+    );
+
+    expect(container).toMatchSnapshot();
+  });


Will be nice to test scenario when alerts are present

jpuri · 2024-07-19T12:13:08Z

test/e2e/tests/confirmations/alerts/queued-confirmations.spec.ts

+    await mockedTrackedQueueControllerEvent(server),
+    await mockedTrackedQueueControllerEvent(server),
+  ];
+}


jpuri

Changes look great, I left some small feedbacks.

ui/pages/confirmations/hooks/alerts/transactions/useQueuedConfirmationsAlerts.ts

matthewwalsh0 · 2024-07-19T12:49:26Z

ui/selectors/approvals.ts

@@ -54,6 +54,10 @@ export function getPendingApprovals(state: ApprovalsMetaMaskState) {
  return Object.values(state.metamask.pendingApprovals ?? []);
 }

+export function pendingApprovalsSortedSelector(state: ApprovalsMetaMaskState) {
+  return getPendingApprovals(state).sort((a1, a2) => a1.time - a2.time);
+}


Does the one in confirm.ts not filter to specific types?

greptile-apps

PR Summary

(updates since last review)

The pull request introduces a new banner alert and corresponding metrics event to mitigate risks for distracted users handling queued transactions from different dApps.

New Banner Alert: Added QueuedRequestsBannerAlert component in multiple files to notify users about queued transactions.
Metrics Event: Introduced ConfirmationQueued event and QueueType enum in shared/constants/metametrics.ts.
End-to-End Tests: Added test/e2e/tests/confirmations/alerts/queued-confirmations.spec.ts to cover new alert scenarios.
Localization: Added existingRequestsBannerAlertDesc message in app/_locales/en/messages.json.
Type Safety: Updated type definitions in various test files for better maintainability and clarity.

No major changes found since the last review.

_{39 file(s) reviewed, 2 comment(s)}
_{Edit PR Review Bot Settings}

greptile-apps · 2024-07-23T17:35:32Z

ui/pages/confirmations/hooks/alerts/transactions/useQueuedConfirmationsAlerts.ts

+  const queuedRequestCount = useSelector(getQueuedRequestCount);
+  const isQueuedConfirmation = queuedRequestCount > 0;


Style: Consider renaming isQueuedConfirmation to hasQueuedConfirmations for better readability.

greptile-apps · 2024-07-23T17:36:41Z

ui/pages/confirmations/hooks/useQueuedConfirmationEvents.ts

+      });
+    }
+  }, [
+    JSON.stringify(pendingApprovals),


Style: Using JSON.stringify in the dependency array can cause performance issues. Consider using a more efficient method to track changes in pendingApprovals.

greptile-apps

PR Summary

(updates since last review)

The pull request introduces a new banner alert and corresponding metrics event to mitigate risks for distracted users handling queued transactions from different dApps.

New Banner Alert: Added QueuedRequestsBannerAlert component in multiple files to notify users about queued transactions.
Metrics Event: Introduced ConfirmationQueued event and QueueType enum in shared/constants/metametrics.ts.
End-to-End Tests: Added test/e2e/tests/confirmations/alerts/queued-confirmations.spec.ts to cover new alert scenarios.
Localization: Added existingRequestsBannerAlertDesc message in app/_locales/en/messages.json.
Type Safety: Updated type definitions in various test files for better maintainability and clarity.

No major changes found since the last review.

_{82 file(s) reviewed, 2 comment(s)}
_{Edit PR Review Bot Settings}

greptile-apps · 2024-07-24T07:52:43Z

ui/pages/confirmations/hooks/alerts/transactions/useQueuedConfirmationsAlerts.ts

+  const queuedRequestCount = useSelector(getQueuedRequestCount);
+  const isQueuedConfirmation = queuedRequestCount > 0;


Style: Consider renaming isQueuedConfirmation to hasQueuedConfirmations for better readability.

greptile-apps · 2024-07-24T07:53:33Z

ui/pages/confirmations/hooks/useQueuedConfirmationEvents.ts

+      });
+    }
+  }, [
+    JSON.stringify(pendingApprovals),


Style: Using JSON.stringify in the dependency array can cause performance issues. Consider using a more efficient method to track changes in pendingApprovals.

greptile-apps

PR Summary

(updates since last review)

The pull request introduces a new banner alert and corresponding metrics event to mitigate risks for distracted users handling queued transactions from different dApps.

New Banner Alert: Added QueuedRequestsBannerAlert component in multiple files to notify users about queued transactions.
Metrics Event: Introduced ConfirmationQueued event and QueueType enum in shared/constants/metametrics.ts.
End-to-End Tests: Added test/e2e/tests/confirmations/alerts/queued-confirmations.spec.ts to cover new alert scenarios.
Localization: Added existingRequestsBannerAlertDesc message in app/_locales/en/messages.json.
Type Safety: Updated type definitions in various test files for better maintainability and clarity.

_{37 file(s) reviewed, 2 comment(s)}
_{Edit PR Review Bot Settings}

greptile-apps · 2024-07-24T08:26:07Z

ui/pages/confirmations/hooks/alerts/transactions/useQueuedConfirmationsAlerts.ts

+  const queuedRequestCount = useSelector(getQueuedRequestCount);
+  const isQueuedConfirmation = queuedRequestCount > 0;


Style: Consider renaming isQueuedConfirmation to hasQueuedConfirmations for better readability.

greptile-apps · 2024-07-24T08:27:00Z

ui/pages/confirmations/hooks/useQueuedConfirmationEvents.ts

+      });
+    }
+  }, [
+    JSON.stringify(pendingApprovals),


Style: Using JSON.stringify in the dependency array can cause performance issues. Consider using a more efficient method to track changes in pendingApprovals.

metamaskbot · 2024-07-25T09:08:24Z

Builds ready [95bbcd3]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (81 ± 10 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	72	140	108	18	8
		domContentLoaded	11	64	33	15	7
		load	56	116	81	20	10
		domInteractive	11	64	33	15	7

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 0 Bytes (0.00%)
ui: 7.6 KiB (0.10%)
common: 404 Bytes (0.01%)

greptile-apps

PR Summary

(updates since last review)

The pull request introduces a new banner alert and corresponding metrics event to mitigate risks for distracted users handling queued transactions from different dApps.

New Banner Alert: Added QueuedRequestsBannerAlert component in multiple files to notify users about queued transactions.
Metrics Event: Introduced ConfirmationQueued event and QueueType enum in shared/constants/metametrics.ts.
End-to-End Tests: Added test/e2e/tests/confirmations/alerts/queued-confirmations.spec.ts to cover new alert scenarios.
Localization: Added existingRequestsBannerAlertDesc message in app/_locales/en/messages.json.
Type Safety: Updated type definitions in various test files for better maintainability and clarity.

_{102 file(s) reviewed, 2 comment(s)}
_{Edit PR Review Bot Settings}

greptile-apps · 2024-07-25T09:33:23Z

ui/pages/confirmations/hooks/alerts/transactions/useQueuedConfirmationsAlerts.ts

+  const queuedRequestCount = useSelector(getQueuedRequestCount);
+  const isQueuedConfirmation = queuedRequestCount > 0;


Style: Consider renaming isQueuedConfirmation to hasQueuedConfirmations for better readability.

greptile-apps · 2024-07-25T09:34:19Z

ui/pages/confirmations/hooks/useQueuedConfirmationEvents.ts

+      });
+    }
+  }, [
+    JSON.stringify(pendingApprovals),


Style: Using JSON.stringify in the dependency array can cause performance issues. Consider using a more efficient method to track changes in pendingApprovals.

sonarcloud · 2024-07-25T10:07:19Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

greptile-apps

PR Summary

(updates since last review)

The pull request introduces a new banner alert and corresponding metrics event to mitigate risks for distracted users handling queued transactions from different dApps.

New Banner Alert: Added QueuedRequestsBannerAlert component in multiple files to notify users about queued transactions.
Metrics Event: Introduced ConfirmationQueued event and QueueType enum in shared/constants/metametrics.ts.
End-to-End Tests: Added test/e2e/tests/confirmations/alerts/queued-confirmations.spec.ts to cover new alert scenarios.
Localization: Added existingRequestsBannerAlertDesc message in app/_locales/en/messages.json.
Type Safety: Updated type definitions in various test files for better maintainability and clarity.

_{49 file(s) reviewed, 2 comment(s)}
_{Edit PR Review Bot Settings}

greptile-apps · 2024-07-25T10:19:21Z

ui/pages/confirmations/hooks/alerts/transactions/useQueuedConfirmationsAlerts.ts

+  const queuedRequestCount = useSelector(getQueuedRequestCount);
+  const isQueuedConfirmation = queuedRequestCount > 0;


Style: Consider renaming isQueuedConfirmation to hasQueuedConfirmations for better readability.

greptile-apps · 2024-07-25T10:20:11Z

ui/pages/confirmations/hooks/useQueuedConfirmationEvents.ts

+      });
+    }
+  }, [
+    JSON.stringify(pendingApprovals),


Style: Using JSON.stringify in the dependency array can cause performance issues. Consider using a more efficient method to track changes in pendingApprovals.

greptile-apps

PR Summary

(updates since last review)

Enhanced test coverage for the useSimulationMetrics hook to ensure robust behavior under various conditions.

Test Coverage: Expanded scenarios in ui/pages/confirmations/components/simulation-details/useSimulationMetrics.test.ts.
Mock Implementations: Utilized mocks for hooks and functions to simulate different simulation responses and asset types.
Unused Code Removal: Cleaned up unused mock return values to streamline the test suite.

No major changes found since the last review.

_{1 file(s) reviewed, no comment(s)}
_{Edit PR Review Bot Settings}

metamaskbot · 2024-07-25T10:41:13Z

Builds ready [907a000]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (63 ± 8 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	69	122	91	17	8
		domContentLoaded	9	57	23	14	7
		load	39	104	63	17	8
		domInteractive	9	57	23	14	7

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 0 Bytes (0.00%)
ui: 7.59 KiB (0.10%)
common: 404 Bytes (0.01%)

pedronfigueiredo added the team-confirmations Push issues to confirmations team label Jul 16, 2024

pedronfigueiredo self-assigned this Jul 16, 2024

pedronfigueiredo requested review from a team as code owners July 16, 2024 10:30

pedronfigueiredo force-pushed the pnf/2758 branch from 0b238e8 to c82d5fd Compare July 16, 2024 10:31

matthewwalsh0 requested changes Jul 17, 2024

View reviewed changes

pedronfigueiredo force-pushed the pnf/2758 branch 2 times, most recently from d8d480c to ea183e5 Compare July 19, 2024 09:20

pedronfigueiredo changed the title ~~Mitigate risk for distracted users on queued transactions from different dApps~~ feat: Mitigate risk for distracted users on queued transactions from different dApps Jul 19, 2024

pedronfigueiredo force-pushed the pnf/2758 branch 4 times, most recently from ce44b26 to 7d0c605 Compare July 19, 2024 10:43

jpuri reviewed Jul 19, 2024

View reviewed changes

pedronfigueiredo force-pushed the pnf/2758 branch from 7d0c605 to 1c2a951 Compare July 19, 2024 12:26

matthewwalsh0 reviewed Jul 19, 2024

View reviewed changes

matthewwalsh0 previously approved these changes Jul 19, 2024

View reviewed changes

pedronfigueiredo dismissed matthewwalsh0’s stale review via bf30e48 July 19, 2024 13:08

pedronfigueiredo force-pushed the pnf/2758 branch 5 times, most recently from 9158c9d to 69f936c Compare July 22, 2024 09:30

greptile-apps bot reviewed Jul 23, 2024

View reviewed changes

pedronfigueiredo force-pushed the pnf/2758 branch from 8e02757 to 0ca4905 Compare July 24, 2024 06:58

greptile-apps bot reviewed Jul 24, 2024

View reviewed changes

pedronfigueiredo force-pushed the pnf/2758 branch from 0ca4905 to bcfb8ce Compare July 24, 2024 08:04

greptile-apps bot reviewed Jul 24, 2024

View reviewed changes

pedronfigueiredo force-pushed the pnf/2758 branch from bcfb8ce to 95bbcd3 Compare July 25, 2024 08:29

greptile-apps bot reviewed Jul 25, 2024

View reviewed changes

pedronfigueiredo added 8 commits July 25, 2024 10:53

wip

a6eea5d

wip

ce291f5

wip

4d58aeb

wip

79521a3

wip

24f1a43

wip

4abda18

wip

f2308a5

wip

e05d762

pedronfigueiredo force-pushed the pnf/2758 branch from 95bbcd3 to e05d762 Compare July 25, 2024 09:53

wip

907a000

greptile-apps bot reviewed Jul 25, 2024

View reviewed changes

pedronfigueiredo requested review from jpuri and matthewwalsh0 July 25, 2024 10:25

jpuri approved these changes Jul 25, 2024

View reviewed changes

matthewwalsh0 approved these changes Jul 25, 2024

View reviewed changes

pedronfigueiredo merged commit 257acdd into develop Jul 25, 2024
76 checks passed

pedronfigueiredo deleted the pnf/2758 branch July 25, 2024 10:41

github-actions bot locked and limited conversation to collaborators Jul 25, 2024

metamaskbot added the release-12.3.0 Issue or pull request that will be included in release 12.3.0 label Jul 25, 2024

		const queuedRequestCount = useSelector(getQueuedRequestCount);
		const isQueuedConfirmation = queuedRequestCount > 0;

feat: Mitigate risk for distracted users on queued transactions from different dApps #25852

feat: Mitigate risk for distracted users on queued transactions from different dApps #25852

Conversation

pedronfigueiredo commented Jul 16, 2024 • edited by github-actions bot Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bschorchit Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpuri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Jul 23, 2024

Choose a reason for hiding this comment

greptile-apps bot Jul 23, 2024

Choose a reason for hiding this comment

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Jul 24, 2024

Choose a reason for hiding this comment

greptile-apps bot Jul 24, 2024

Choose a reason for hiding this comment

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Jul 24, 2024

Choose a reason for hiding this comment

greptile-apps bot Jul 24, 2024

Choose a reason for hiding this comment

metamaskbot commented Jul 25, 2024

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Jul 25, 2024

Choose a reason for hiding this comment

greptile-apps bot Jul 25, 2024

Choose a reason for hiding this comment

sonarcloud bot commented Jul 25, 2024

Quality Gate passed

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Jul 25, 2024

Choose a reason for hiding this comment

greptile-apps bot Jul 25, 2024

Choose a reason for hiding this comment

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

metamaskbot commented Jul 25, 2024

pedronfigueiredo commented Jul 16, 2024 •

edited by github-actions bot

Loading

bschorchit Jul 17, 2024 •

edited

Loading