Skip to content

fix: alert system and refine SIWE and contract interaction alerts#27205

Merged
vinistevam merged 27 commits intodevelopfrom
fix/alert-system
Sep 26, 2024
Merged

fix: alert system and refine SIWE and contract interaction alerts#27205
vinistevam merged 27 commits intodevelopfrom
fix/alert-system

Conversation

@vinistevam
Copy link
Copy Markdown
Contributor

@vinistevam vinistevam commented Sep 17, 2024

Description

This PR aims to apply fixes and improvements to the Alert System in general and specific alerts such as SIWE and contract interaction.

Fixes:

1- Display a more generic and toned-down message on the friction modal.

2- Show alerts before the Confirm Alert Modal (final friction modal) when there is a danger alert.

3- Fixed CTA button when the alert has not been acknowledged.

Fixes from UX QA feedback
Figma

  • Change the Danger icon to security-search icon when button is Review Alerts or Review Alert - ✔️
  • Remove blockaid dedicated alert modal - ✔️
  • Remove the “tooltip icon” whenever an alert is present - ✔️
  • Always display a critical alert first when a critical alert is present:
    • Inline alerts - ✔️
    • Multiple Alert Modal - ✔️
  • Disable the CTA "Review Alerts" whenever a blocking alert is present - ✔️
  • Add "Got it" for blocking alerts. (this button will be responsible for closing the modal) - ✔️
  • Remove the close X button - ✔️

Open in GitHub Codespaces

Related issues

Fixes: #27132 #27147 #27118

Manual testing steps

Case 1

  1. Go to test dapp
  2. Don't have funds
  3. Trigger malicious mint erc20 on Sepolia
  4. Click on Review alerts

Case 2

  1. Go to test dapp
  2. Trigger malicious contract interaction on Ethereum
  3. Click on Review alerts

Case 3

  1. Go to test dapp
  2. Click on Sign In With Ethereum - Bad Domain
  3. Click Confirm

Case 4

  1. Deploy an ERC721 contract from the test dapp
  2. Trigger a transaction with a nonce +1, so it's not confirmed
  3. Now trigger a Mint
  4. Click on the alerts (there are 2)
  5. Navigate to the 2nd alert
  6. Click got it --> MM broke

Screenshots/Recordings

image

alert-fix.webm
mint-without-funds.webm
Contract.Interaction.and.Alerts.webm

Before

After

Pre-merge author checklist

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.

@vinistevam vinistevam added the team-confirmations Push issues to confirmations team label Sep 17, 2024
@github-actions
Copy link
Copy Markdown
Contributor

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.

@vinistevam vinistevam marked this pull request as ready for review September 17, 2024 11:58
@vinistevam vinistevam requested review from a team as code owners September 17, 2024 11:58
@vinistevam vinistevam changed the title fix: Alert system fix: alert system and refine SIWE and contract interaction alerts Sep 17, 2024
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [8fa2cd6]
Page Load Metrics (1543 ± 72 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint20020531358502241
domContentLoaded12942044152314369
load13022053154314972
domInteractive137138168
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: 91 Bytes (0.00%)
  • common: -1 Bytes (-0.00%)

@codecov
Copy link
Copy Markdown

codecov bot commented Sep 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.01%. Comparing base (e7df8e2) to head (8fa2cd6).
Report is 2 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #27205      +/-   ##
===========================================
+ Coverage    70.01%   70.01%   +0.01%     
===========================================
  Files         1445     1445              
  Lines        50194    50199       +5     
  Branches     14041    14043       +2     
===========================================
+ Hits         35139    35146       +7     
+ Misses       15055    15053       -2     

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

@@ -1019,10 +1019,10 @@
"message": "I have acknowledged the alert and still want to proceed"
},
"confirmAlertModalDetails": {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To change the copy correctly and trigger new translations, you can add the new copy under a new translation key, and delete the old key in all languages.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, thanks for pointing that out. I've applied the recommended changes. 474affa

Copy link
Copy Markdown
Contributor

@pedronfigueiredo pedronfigueiredo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just left one comment regarding the translation files

@sonarqubecloud
Copy link
Copy Markdown

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [474affa]
Page Load Metrics (1732 ± 107 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint19822261529588282
domContentLoaded147522151704214103
load148422271732224107
domInteractive19113422512
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: 92 Bytes (0.00%)
  • common: -1 Bytes (-0.00%)

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [14d6c72]
Page Load Metrics (1768 ± 85 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14992113177717885
domContentLoaded14552070173616780
load15032127176817885
domInteractive187338167
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 23 Bytes (0.00%)
  • common: 9 Bytes (0.00%)

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [7b18c80]
Page Load Metrics (1866 ± 71 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint15992136187615976
domContentLoaded15732069182913464
load15892145186614871
domInteractive13118412311
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 23 Bytes (0.00%)
  • common: 9 Bytes (0.00%)

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [1349889]
Page Load Metrics (1855 ± 89 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint16382502186918589
domContentLoaded16292438183017584
load16372518185518689
domInteractive236835147
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 365 Bytes (0.00%)
  • common: 9 Bytes (0.00%)

digiwand
digiwand previously approved these changes Sep 24, 2024
Copy link
Copy Markdown
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looks good! added 1 non-blocking suggestion

merge conflicts are blocking merge

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [613b40d]
Page Load Metrics (2092 ± 169 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint158530592094351168
domContentLoaded157129502057332159
load158330772092352169
domInteractive17218624220
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 155 Bytes (0.00%)
  • common: 9 Bytes (0.00%)

Copy link
Copy Markdown
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm!

@sonarqubecloud
Copy link
Copy Markdown

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [c9dc17d]
Page Load Metrics (1931 ± 97 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31925231710589283
domContentLoaded16062437190719594
load16142526193120297
domInteractive208748188
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 155 Bytes (0.00%)
  • common: 9 Bytes (0.00%)

@vinistevam vinistevam merged commit 62f7b61 into develop Sep 26, 2024
@vinistevam vinistevam deleted the fix/alert-system branch September 26, 2024 09:31
@github-actions github-actions bot locked and limited conversation to collaborators Sep 26, 2024
@metamaskbot metamaskbot added the release-12.6.0 Issue or pull request that will be included in release 12.6.0 label Sep 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

release-12.6.0 Issue or pull request that will be included in release 12.6.0 team-confirmations Push issues to confirmations team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Issues with alert system for SIWE

5 participants