feat: add SIWE mismatch account warning alert and hide duplicate warnings in MultipleAlertModal by digiwand · Pull Request #25559 · MetaMask/metamask-extension

digiwand · 2024-06-27T12:50:17Z

Description

Adds account mismatch warning alerts to SIWE redesign pages
Adds support to hide duplicate warnings in MultipleAlertModal

Related issues

Fixes: #25317

Manual testing steps

comment out isSIWE condition in ui/pages/confirmations/hooks/useCurrentConfirmation.ts file
turn on redesign in Experimental Settings
yarn start
Go to test-dapp and test SIWE Bad Account button

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-06-27T12:50:30Z

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.

codecov · 2024-06-27T17:21:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.66%. Comparing base (1d33d6f) to head (23cd578).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #25559      +/-   ##
===========================================
+ Coverage    69.65%   69.66%   +0.01%     
===========================================
  Files         1355     1356       +1     
  Lines        48007    48029      +22     
  Branches     13235    13239       +4     
===========================================
+ Hits         33437    33459      +22     
  Misses       14570    14570

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

metamaskbot · 2024-06-27T17:31:58Z

Builds ready [69cc879]

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, 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 (322 ± 280 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	73	447	120	78	37
		domContentLoaded	11	160	33	31	15
		load	48	2038	322	584	280
		domInteractive	11	160	33	31	15

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

background: 0 Bytes (0.00%)
ui: 2.42 KiB (0.03%)
common: 153 Bytes (0.00%)

metamaskbot · 2024-06-27T20:22:16Z

Builds ready [23cd578]

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, 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 (91 ± 20 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	68	293	124	60	29
		domContentLoaded	10	88	32	22	11
		load	48	206	91	41	20
		domInteractive	10	88	32	22	11

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

background: 0 Bytes (0.00%)
ui: 2.54 KiB (0.04%)
common: 153 Bytes (0.00%)

metamaskbot · 2024-06-27T20:22:39Z

Builds ready [23cd578]

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, 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 (91 ± 20 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	68	293	124	60	29
		domContentLoaded	10	88	32	22	11
		load	48	206	91	41	20
		domInteractive	10	88	32	22	11

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

background: 0 Bytes (0.00%)
ui: 2.54 KiB (0.04%)
common: 153 Bytes (0.00%)

jpuri · 2024-06-28T05:51:28Z

ui/pages/confirmations/hooks/alerts/signatures/useAccountMismatchAlerts.ts

+  }, [isMismatchAccount, t]);
+
+  return alerts;
+}


Above hook will re-render only if confirmation changes, thus do we really need useMemo ?

We currently do this as standard on all alert hooks since they are returning arrays so it ensures any usages and future usages have stable data, at the cost of only around 4 extra lines.

vinistevam · 2024-06-28T12:27:00Z

ui/components/app/alert-system/multiple-alert-modal/multiple-alert-modal.tsx

+  const alerts = useMemo(() => {
+    return allAlerts.reduce((result: Alert[], alert: Alert) => {
+      const isDuplicateWarning = result.some((a: Alert) => {
+        return a.severity === Severity.Warning && a.message === alert.message;


Currently, the only use case is for warning alerts (mismatch account). While this addresses the present scenario, should we extend it to handle duplicates for all alert severities?

I agree. I think since the danger severity would require confirmation, it would require more updates. Because of this I think it’d be better to extend the functionality when we need it

Assuming we definitely want duplicate alerts on different fields, would it be cleaner to encapsulate this within useAlerts in a property such as uniqueAlerts?

matthewwalsh0 · 2024-06-28T14:05:02Z

ui/pages/confirmations/hooks/alerts/signatures/useAccountMismatchAlerts.ts

+        severity: Severity.Warning,
+      },
+      {
+        field: 'signingInWith',


Why are we adding a single alert to two fields?

I understood our alert system assumes alerts are general or targeted to a single field?

Can we not pick the most relevant single field such as signing in with?

oh I think you may be right. I was referring to the design in the screenshot but in Figma, it does indeed look like the alert only exists on "Signing in with". Let me double-check w/ design

thanks for comment! at least for this release, we will want to show only 1 alert on "Signing in with". I'll create a new PR for this in case we'd like to show the same alert on two different rows in the future

matthewwalsh0 · 2024-06-28T14:06:41Z

ui/components/app/alert-system/multiple-alert-modal/multiple-alert-modal.tsx

+  const alerts = useMemo(() => {
+    return allAlerts.reduce((result: Alert[], alert: Alert) => {
+      const isDuplicateWarning = result.some((a: Alert) => {
+        return a.severity === Severity.Warning && a.message === alert.message;


Assuming we definitely want duplicate alerts on different fields, would it be cleaner to encapsulate this within useAlerts in a property such as uniqueAlerts?

matthewwalsh0 · 2024-06-28T14:07:18Z

ui/pages/confirmations/components/confirm/info/personal-sign/personal-sign.tsx

        {isSIWE && (
-          <ConfirmInfoRow label={t('signingInWith')}>
+          <ConfirmInfoAlertRow
+            alertKey="signingInWith"


To avoid duplicate strings, there is now a RowAlertKey enum inside ui/components/app/confirm/info/row/constants.ts.

nicee. I think it's a bit confusing RowAlertKey represents the field [key], but I think I got it #25613

possibly we could rename these to something like

RowAlertKey → RowAlertFieldKey or RowAlertField

alertKey property to fieldKey

matthewwalsh0 · 2024-06-28T14:08:38Z

ui/pages/confirmations/hooks/alerts/signatures/useAccountMismatchAlerts.ts

+        reason: t('alertReasonWrongAccount'),
+        severity: Severity.Warning,
+      },
+    ] as Alert[];


Why do we need to cast? Does this suggest we have bad data or a bad type?

no need. Removed it here #25613

matthewwalsh0 · 2024-06-28T14:08:59Z

ui/pages/confirmations/hooks/alerts/signatures/useAccountMismatchAlerts.ts

+    ] as Alert[];
+  }, [isMismatchAccount, t]);
+
+  return alerts;


Minor, but can we return the hook directly to make it explicit we're caching the result?

updated #25613

matthewwalsh0 · 2024-06-28T14:09:57Z

ui/pages/confirmations/hooks/alerts/signatures/useAccountMismatchAlerts.ts

+  }, [isMismatchAccount, t]);
+
+  return alerts;
+}


We currently do this as standard on all alert hooks since they are returning arrays so it ensures any usages and future usages have stable data, at the cost of only around 4 extra lines.

digiwand · 2024-07-01T19:34:01Z

Rather than showing the same alert on two different rows, we will display it only on "Signing in with". Closing this PR in favor of #25613

metamaskbot added the team-confirmations Push issues to confirmations team label Jun 27, 2024

Base automatically changed from feat-sign-refactor-alerts to develop June 27, 2024 16:21

digiwand added 3 commits June 27, 2024 19:02

feat: add SIWE mismatch account warning alert

df2adeb

yarn lint:fix

2a29248

test:fix: include account alert updates to siwe-sign snapshot

69cc879

digiwand force-pushed the feat-sign-account-mistmatch-alerts branch from 32de38d to 69cc879 Compare June 27, 2024 17:02

feat: hide warning duplicates from MultipleAlertModal

ecd4254

digiwand marked this pull request as ready for review June 27, 2024 18:48

digiwand requested review from a team as code owners June 27, 2024 18:48

style: update comment

23cd578

digiwand changed the title ~~feat: add SIWE mismatch account warning alert~~ feat: add SIWE mismatch account warning alert and hide duplicate warnings in MultipleAlertModal Jun 27, 2024

jpuri reviewed Jun 28, 2024

View reviewed changes

vinistevam reviewed Jun 28, 2024

View reviewed changes

matthewwalsh0 reviewed Jun 28, 2024

View reviewed changes

digiwand added a commit that referenced this pull request Jul 1, 2024

style: address comments from #25559

1cb4c1a

digiwand closed this Jul 1, 2024

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

Conversation

digiwand commented Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions bot commented Jun 27, 2024

Uh oh!

codecov bot commented Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

metamaskbot commented Jun 27, 2024

Uh oh!

metamaskbot commented Jun 27, 2024

Uh oh!

metamaskbot commented Jun 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

digiwand commented Jul 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

digiwand commented Jun 27, 2024 •

edited

Loading

codecov bot commented Jun 27, 2024 •

edited

Loading