feat: Implement alerts for personal sign #24469

vinistevam · 2024-05-09T20:23:42Z

Description

This PR aims to implement alerts for personal sign.

Context
The confirmation supporting the personal sign operation was one of the first confirmations implemented using the new confirmation architecture.

In order to test and demonstrate the use of the new alert system, we can incorporate suitable alerts into the personal sign confirmation.

Changes
Implementation

Changed personal-sign.tsx to use the AlertRow component.
Created a hook to support custom actions in alerts, PersonalSignAlertActions
Created usePersonalSignAlerts to aggregate the alerts from the providers and add those into the new alert system.
Created setConfirmationAlerts hook to update and clean alerts based on the current confirmation.
Created useConfirmationAlerts hook to integrate the types of confirmations into the new alert system.
Included the new SecurityAlertBanner in the title.tsx to render the general alerts coming from the new alert system.

New Banner Alert component

Created SecurityAlertBanner to support general alerts coming from a provider. This component was based on the SecurityProviderBannerAlert.
- Unit tests
- Storybook

Folder reorganization

confirmations/alerts to alerts-system (ui/components/app/ should not have several subfolders)

QA Design feedback

Only render in AlertModal only alerts tied to a field, general alerts should be displayed in the banner on top of the confirmation.
Hide the acknowledge checkbox for alerts with severity info and warning.
Move icon in the AlertModal be in the same level of the navigation and close buttons.
Include provider into the AlertModal.

Clean up, team feedback, QA tests and fixes

Created an AlertSeverity type to use in the Alert type, also added a provider optional property.
Fixed handleCloseModal in the footer.tsx.
Extracted to a utility file getBannerAlertSeverity, getProviderAlertSeverity and providerAlertNormalizer functions and added Unit tests.
Fixed alert-row when variant is undefined to use primarily the severity from the alert.
Fixed handleAcknowledgeClick

Related issues

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

Manual testing steps

Test regression for signature and personal sign re-designed confirmations.

PRs Related
#23719
#23625
#23664
#24049
#24214
#24400

Screenshots/Recordings

personal-sign-alert-system.webm

Security banner alert

Multiple alerts

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.

…lert-modal-component

…ne-alert-info-row-component

…n the row

…al.tsx Co-authored-by: Matthew Walsh <matthew.walsh@consensys.net>

…al.test.tsx Co-authored-by: Matthew Walsh <matthew.walsh@consensys.net>

…lert-modal-component

jpuri · 2024-05-24T08:19:15Z

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

+  const hasUnconfirmedAlerts = alerts.some(
+    (alert: Alert) =>
+      !isAlertConfirmed(alert.key) && alert.severity === Severity.Danger,
+  );


It might be useful to move methods like above to useAlerts so they can be re-used.

100%, I should have done that before. I didn't push it because I saw this improvement in your PR.

jpuri · 2024-05-24T08:21:08Z

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

+    }
+    return highestSeverity;
+  }, Severity.Info);
+}


Above may be just a forEach will be good.

jpuri · 2024-05-24T08:26:13Z

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

+      />
+    </Box>
+  );
+}


It will be useful to extract above component into separate file. We try to keep confirmation code lesser number of lines per file and smaller well defined components.

This makes sense, is it ok to extract in a different PR? I'm happy to do it.

ui/pages/confirmations/components/confirm/alert-system/alert-provider/alert-provider.tsx

ui/contexts/alertActionHandler.tsx

jpuri

I left some small feedbacks, changes largely look good.

metamaskbot · 2024-05-24T11:55:35Z

Builds ready [555e858]

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

Page Load Metrics (1426 ± 599 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	59	186	102	29	14
		domContentLoaded	9	35	14	6	3
		load	48	3237	1426	1248	599
		domInteractive	9	35	14	6	3

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

background: 0 Bytes (0.00%)
ui: 19.95 KiB (0.29%)
common: 196 Bytes (0.00%)

metamaskbot · 2024-05-27T08:47:39Z

Builds ready [3e092e3]

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 (788 ± 528 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	63	194	101	33	16
		domContentLoaded	9	64	17	11	6
		load	51	2747	788	1100	528
		domInteractive	9	64	17	11	6

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

background: 0 Bytes (0.00%)
ui: 19.95 KiB (0.30%)
common: 196 Bytes (0.00%)

metamaskbot · 2024-05-27T10:24:02Z

Builds ready [fc35032]

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 (554 ± 470 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	65	148	89	23	11
		domContentLoaded	8	64	19	17	8
		load	51	2862	554	979	470
		domInteractive	8	64	19	17	8

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

background: 0 Bytes (0.00%)
ui: 19.95 KiB (0.30%)
common: 196 Bytes (0.00%)

metamaskbot · 2024-05-28T08:02:14Z

Builds ready [05041ce]

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 (414 ± 392 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	63	138	90	19	9
		domContentLoaded	8	49	14	11	5
		load	51	2469	414	816	392
		domInteractive	8	49	14	11	5

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

background: 0 Bytes (0.00%)
ui: 19.95 KiB (0.30%)
common: 196 Bytes (0.00%)

vinistevam and others added 30 commits March 18, 2024 10:35

add base for single alert modal

7f7bdbd

creating hooks and tests

aaa6fb5

add duck and reducer

2193e4e

add ui for alert modal, unit tests and storybook

b039238

Merge branch 'develop' into feat/single-alert-modal-component

ac44233

Merge branch 'develop' into feat/single-alert-modal-component

38fb78a

Merge branch 'develop' into feat/single-alert-modal-component

4b7a5ad

fix e2e

f04477c

add multiple alert modal component with unit test and storybook

6aee513

add more unit tests

59334f1

update test and documentation

9d99df2

Merge branch 'develop' into feat/single-alert-modal-component

c7144da

Merge branch 'feat/single-alert-modal-component' into feat/multiple-a…

19ea3e4

…lert-modal-component

add inline alert to info row component

66a437e

add custom button to alert modal

a405398

Merge branch 'feat/single-alert-modal-component' into feat/multiple-a…

9ea3dcb

…lert-modal-component

Merge branch 'feat/multiple-alert-modal-component' into feat/add-inli…

ac31cf3

…ne-alert-info-row-component

fix lint

1948524

lint fix

58ea3f0

Merge branch 'feat/multiple-alert-modal-component' into feat/add-inli…

2d64381

…ne-alert-info-row-component

update snapshots and add mock for confirm alerts

9f2b4f6

add logic to show alert modal when clicking inline alert introduced i…

d71dac5

…n the row

fix lint

ec5a812

Merge branch 'develop' into feat/single-alert-modal-component

603e861

Update ui/pages/confirmations/components/alerts/alert-modal/alert-mod…

3958889

…al.tsx Co-authored-by: Matthew Walsh <matthew.walsh@consensys.net>

Update ui/pages/confirmations/components/alerts/alert-modal/alert-mod…

f582164

…al.tsx Co-authored-by: Matthew Walsh <matthew.walsh@consensys.net>

Update ui/pages/confirmations/components/alerts/alert-modal/alert-mod…

b95d5fa

…al.test.tsx Co-authored-by: Matthew Walsh <matthew.walsh@consensys.net>

apply suggestions and feedback

69d4a05

Merge branch 'feat/single-alert-modal-component' into feat/multiple-a…

1daff6e

…lert-modal-component

fix css

9c78903

jpuri reviewed May 24, 2024

View reviewed changes

matthewwalsh0 reviewed May 24, 2024

View reviewed changes

ui/pages/confirmations/components/confirm/alert-system/alert-provider/alert-provider.tsx Outdated Show resolved Hide resolved

ui/contexts/alertActionHandler.tsx Outdated Show resolved Hide resolved

jpuri previously approved these changes May 24, 2024

View reviewed changes

vinistevam mentioned this pull request May 24, 2024

fix: add processAction on AlertActionHandlerProvider, reorganize files, and update CODEOWNERS #24766

Draft

7 tasks

vinistevam and others added 2 commits May 24, 2024 10:54

Merge branch 'develop' into feat/implement-personal-sign-alerts

1a89f0e

apply team feedback

555e858

vinistevam dismissed jpuri’s stale review via 555e858 May 24, 2024 11:14

vinistevam requested review from kumavis and a team as code owners May 24, 2024 11:14

matthewwalsh0 previously approved these changes May 24, 2024

View reviewed changes

Merge branch 'develop' into feat/implement-personal-sign-alerts

3e092e3

jpuri previously approved these changes May 27, 2024

View reviewed changes

vinistevam dismissed stale reviews from jpuri and matthewwalsh0 via 6568ea8 May 27, 2024 09:07

remove codeowners change to do it in a separate PR

fc35032

vinistevam force-pushed the feat/implement-personal-sign-alerts branch from 6568ea8 to fc35032 Compare May 27, 2024 09:40

Merge branch 'develop' into feat/implement-personal-sign-alerts

05041ce

pedronfigueiredo approved these changes May 28, 2024

View reviewed changes

matthewwalsh0 approved these changes May 28, 2024

View reviewed changes

vinistevam merged commit 17de691 into develop May 28, 2024
71 of 72 checks passed

vinistevam deleted the feat/implement-personal-sign-alerts branch May 28, 2024 11:08

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

metamaskbot added the release-11.18.0 label May 28, 2024

gauthierpetetin added the release-12.0.0 label Jun 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement alerts for personal sign #24469

feat: Implement alerts for personal sign #24469

vinistevam commented May 9, 2024 •

edited

jpuri May 24, 2024

vinistevam May 24, 2024

jpuri May 24, 2024

vinistevam May 24, 2024

jpuri May 24, 2024

vinistevam May 24, 2024

jpuri left a comment

metamaskbot commented May 24, 2024

metamaskbot commented May 27, 2024

metamaskbot commented May 27, 2024

metamaskbot commented May 28, 2024

feat: Implement alerts for personal sign #24469

feat: Implement alerts for personal sign #24469

Conversation

vinistevam commented May 9, 2024 • edited

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

jpuri May 24, 2024

Choose a reason for hiding this comment

vinistevam May 24, 2024

Choose a reason for hiding this comment

jpuri May 24, 2024

Choose a reason for hiding this comment

vinistevam May 24, 2024

Choose a reason for hiding this comment

jpuri May 24, 2024

Choose a reason for hiding this comment

vinistevam May 24, 2024

Choose a reason for hiding this comment

jpuri left a comment

Choose a reason for hiding this comment

metamaskbot commented May 24, 2024

metamaskbot commented May 27, 2024

metamaskbot commented May 27, 2024

metamaskbot commented May 28, 2024

vinistevam commented May 9, 2024 •

edited