feat: implementing alerts for typed sign data signatures #24702

jpuri · 2024-05-22T08:05:06Z

Description

Extend alert system to typed signatures.

Related issues

Fixes: #24611

Manual testing steps

Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

I’ve followed MetaMask 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-05-22T08:05:16Z

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-05-22T12:39:16Z

Codecov Report

Attention: Patch coverage is 81.81818% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 65.73%. Comparing base (d27a233) to head (e57494d).
Report is 191 commits behind head on develop.

Files	Patch %	Lines
...nents/app/alert-system/alert-modal/alert-modal.tsx	33.33%	4 Missing ⚠️
...system/confirm-alert-modal/confirm-alert-modal.tsx	71.43%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #24702      +/-   ##
===========================================
- Coverage    67.37%   65.73%   -1.64%     
===========================================
  Files         1278     1363      +85     
  Lines        49881    54269    +4388     
  Branches     12944    14107    +1163     
===========================================
+ Hits         33605    35673    +2068     
- Misses       16276    18596    +2320

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

metamaskbot · 2024-05-22T15:17:20Z

Builds ready [aa6f1db]

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, 6
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (545 ± 400 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	67	130	89	19	9
		domContentLoaded	7	27	13	6	3
		load	44	2116	545	833	400
		domInteractive	7	27	13	6	3

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

background: 0 Bytes (0.00%)
ui: 13.92 KiB (0.21%)
common: 348 Bytes (0.01%)

metamaskbot · 2024-05-23T10:33:00Z

Builds ready [2bfa479]

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, 6
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1463 ± 604 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	77	152	105	22	11
		domContentLoaded	10	67	19	16	8
		load	63	2899	1463	1258	604
		domInteractive	10	67	19	16	8

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

background: 0 Bytes (0.00%)
ui: 11.34 KiB (0.17%)
common: 354 Bytes (0.01%)

ui/components/app/alerts-system/utils.ts

ui/hooks/useAlerts.ts

ui/pages/confirmations/hooks/setConfirmationAlerts.ts

ui/pages/confirmations/utils/confirm.ts

metamaskbot · 2024-05-27T14:26:38Z

Builds ready [7dac653]

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, 6
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (970 ± 565 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	60	431	119	76	37
		domContentLoaded	8	188	31	39	19
		load	46	2784	970	1178	565
		domInteractive	8	188	31	39	19

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

background: 0 Bytes (0.00%)
ui: 15.15 KiB (0.23%)
common: 608 Bytes (0.01%)

vinistevam · 2024-05-28T08:45:41Z

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


-  const unconfirmedDangerAlerts = fieldAlerts.filter(


vinistevam · 2024-05-28T08:45:55Z

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

-        onClick={hasDangerFieldAlerts ? handleOpenConfirmModal : onSubmit}
-        danger={hasDangerAlerts ? true : isDangerButton}
+        onClick={hasDangerAlerts ? handleOpenConfirmModal : onSubmit}
+        danger={hasDangerAlerts}
        size={ButtonSize.Lg}
        disabled={hasUnconfirmedDangerAlerts ? false : disabled}
      >
        {hasUnconfirmedDangerAlerts ? t('reviewAlerts') : t('confirm')}


After our discussion regarding the designs for the MVP , I wanted to confirm a few details.
Once we receive a general alert from Blockaid, the button will be labeled Review Alerts. Clicking this button will open a friction modal where the user can acknowledge the alert and proceed to confirm the action.

Is that correct?

Yes, that is how it will work.

vinistevam · 2024-05-28T08:46:17Z

ui/components/app/alert-system/confirm-alert-modal/confirm-alert-modal.test.tsx

-      <ConfirmAlertModal {...defaultProps} />,
-      mockStore,
-    );
+  // todo: following 2 tests have been temporarily commented out


Do we need to comment out these two unit tests? I understand that the MVP does not support multiple providers, but can we keep the use case in the unit tests unless they are broken due to the MVP changes?

Yep that is issue these test are breaking.

The base branch was changed.

metamaskbot · 2024-05-29T07:49:04Z

Builds ready [bc973c5]

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, 6
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (431 ± 404 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	65	143	97	20	9
		domContentLoaded	9	52	17	11	5
		load	60	2614	431	841	404
		domInteractive	9	52	17	11	5

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

background: 0 Bytes (0.00%)
ui: -4.8 KiB (-0.07%)
common: 412 Bytes (0.01%)

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

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

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

bschorchit · 2024-05-29T11:52:34Z

Testing the latest build on this PR, I see malicious warning showing up as blue, missing the see details section and with a blue Confirm button first before both the banner and button change to red.

bschorchit · 2024-05-29T11:56:44Z

Also, the see details section within the warning might be rendering with the wrong font/size cc: @SayaGT

Screenshot from this build:

Screenshot from current prod:

jpuri · 2024-05-29T13:09:58Z

Hey @bschorchit : I tested it using malicious permits in test dapp and it seems to work well:

SayaGT · 2024-05-29T13:48:28Z

@bschorchit @jpuri yes the font size for the alert details seems wrong. It should be Body-MD (baseline). Also the CTA shouldn't be "Review alerts", it should be Confirm.

bschorchit · 2024-05-29T16:23:37Z

Hey @bschorchit : I tested it using malicious permits in test dapp and it seems to work well:

I do see it as red as well, but sometimes only after ~1s. It doesn't happen every time, but I can consistently reproduce by trying a few times and alternating between Malicious Permit, Malicious Trade Order, Malicious Seaport

Here's a recording:

alert.mov

bschorchit · 2024-05-29T16:47:37Z

Summing up to facilitate reading (sorry for the multiple comments):

Font size for alert details should be Body-MD (baseline) - source
CTA should be Confirm rather than Review alerts as this is the single alert scenario - source
Potential issue with rendering blue banner first - source
Small copy updates to the friction modal - (sorry this is new, and totally on me for not getting content frozen sooner. I'll get the final copy soon) - we can tackle this on a follow up PR to prevent blocking this one from being blocked.

vinistevam

Amazing PR! 🥇 🚀

metamaskbot · 2024-05-30T11:18:43Z

Builds ready [e57494d]

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
- content-script: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (829 ± 557 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	69	169	104	30	14
		domContentLoaded	9	71	20	16	8
		load	65	3121	829	1159	557
		domInteractive	9	71	20	16	8

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

background: 0 Bytes (0.00%)
ui: -4.77 KiB (-0.07%)
common: 410 Bytes (0.01%)

jpuri requested review from a team as code owners May 22, 2024 08:05

vinistevam force-pushed the feat/implement-personal-sign-alerts branch 3 times, most recently from 86ac594 to 5891026 Compare May 23, 2024 07:24

matthewwalsh0 reviewed May 23, 2024

View reviewed changes

ui/components/app/alerts-system/utils.ts Outdated Show resolved Hide resolved

ui/hooks/useAlerts.ts Show resolved Hide resolved

ui/pages/confirmations/hooks/setConfirmationAlerts.ts Outdated Show resolved Hide resolved

ui/pages/confirmations/utils/confirm.ts Show resolved Hide resolved

pedronfigueiredo previously approved these changes May 23, 2024

View reviewed changes

jpuri force-pushed the type_sign_data_alerts branch 15 times, most recently from 53049d5 to a473161 Compare May 27, 2024 13:43

vinistevam reviewed May 28, 2024

View reviewed changes

Base automatically changed from feat/implement-personal-sign-alerts to develop May 28, 2024 11:08

jpuri force-pushed the type_sign_data_alerts branch from 7dac653 to 09eea0a Compare May 29, 2024 07:11

Implementing alerts for typed sign data signatures

bc973c5

jpuri force-pushed the type_sign_data_alerts branch from 09eea0a to bc973c5 Compare May 29, 2024 07:15

jpuri changed the title ~~Implementing alerts for typed sign data signatures~~ feat: implementing alerts for typed sign data signatures May 29, 2024

jpuri added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) confirmation-redesign team-confirmations Push issues to confirmations team labels May 29, 2024

matthewwalsh0 reviewed May 29, 2024

View reviewed changes

update

408668b

Merge branch 'develop' into type_sign_data_alerts

74898a1

matthewwalsh0 approved these changes May 30, 2024

View reviewed changes

Merge branch 'develop' into type_sign_data_alerts

e57494d

vinistevam approved these changes May 30, 2024

View reviewed changes

jpuri merged commit e4bf8b3 into develop May 30, 2024
71 of 72 checks passed

jpuri deleted the type_sign_data_alerts branch May 30, 2024 11:13

github-actions bot locked and limited conversation to collaborators May 30, 2024

github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label May 30, 2024

metamaskbot added the release-11.18.0 label May 30, 2024

gauthierpetetin added the release-12.0.0 Issue or pull request that will be included in release 12.0.0 label Jun 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implementing alerts for typed sign data signatures #24702

feat: implementing alerts for typed sign data signatures #24702

jpuri commented May 22, 2024

github-actions bot commented May 22, 2024

codecov bot commented May 22, 2024 •

edited

Loading

metamaskbot commented May 22, 2024

metamaskbot commented May 23, 2024

metamaskbot commented May 27, 2024

vinistevam May 28, 2024

vinistevam May 28, 2024

jpuri May 28, 2024

vinistevam May 28, 2024

jpuri May 28, 2024

metamaskbot commented May 29, 2024

bschorchit commented May 29, 2024 •

edited

Loading

bschorchit commented May 29, 2024

jpuri commented May 29, 2024

SayaGT commented May 29, 2024 •

edited

Loading

bschorchit commented May 29, 2024

bschorchit commented May 29, 2024

vinistevam left a comment

metamaskbot commented May 30, 2024

feat: implementing alerts for typed sign data signatures #24702

feat: implementing alerts for typed sign data signatures #24702

Conversation

jpuri commented May 22, 2024

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented May 22, 2024

codecov bot commented May 22, 2024 • edited Loading

Codecov Report

metamaskbot commented May 22, 2024

metamaskbot commented May 23, 2024

metamaskbot commented May 27, 2024

vinistevam May 28, 2024

Choose a reason for hiding this comment

vinistevam May 28, 2024

Choose a reason for hiding this comment

jpuri May 28, 2024

Choose a reason for hiding this comment

vinistevam May 28, 2024

Choose a reason for hiding this comment

jpuri May 28, 2024

Choose a reason for hiding this comment

metamaskbot commented May 29, 2024

bschorchit commented May 29, 2024 • edited Loading

bschorchit commented May 29, 2024

jpuri commented May 29, 2024

SayaGT commented May 29, 2024 • edited Loading

bschorchit commented May 29, 2024

bschorchit commented May 29, 2024

vinistevam left a comment

Choose a reason for hiding this comment

metamaskbot commented May 30, 2024

codecov bot commented May 22, 2024 •

edited

Loading

bschorchit commented May 29, 2024 •

edited

Loading

SayaGT commented May 29, 2024 •

edited

Loading