Stop blocking display of transaction approval window on resolution of ppom security alert response #22778

danjm · 2024-02-01T18:23:34Z

Description

Solves this problem:

For people on slow networks, the amount of time it takes from "Dapp button click" -> "Popup notification window appears" might be a problem. When blockaid is toggled off, and the network is throttled to slow via the chrome dev tools, I see a ~3 second worse case for this time, but when blockaid is toggled on, I see a 12 second (or even a bit longer) worst case scenario for this time.

The risk here is that users unexpectedly see a longer delay then usual, and so think they need to click the button again, and then get shown multiple confirmation requests and confirm all of them, leading to fund loss.

This can happen on either slow internet connections, or if an infura request is taking longer than usual (which could effect people regardless of internet connection). Yesterday when I first saw this issue, I wasn't slowing down my connection, but it still took ~20 seconds to open. Probably a somewhat rare case (because as I mentioned I didn't notice any problems again until I intentionally throttled the network requests), but even if it only affected 10% of users only once a month, that would be enough to get plenty of "why did metamask just take 20 seconds to open a confirmation window?!?" complaints.

Related issues

Fixes:

Manual testing steps

Go to this page...

Screenshots/Recordings

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.

github-actions · 2024-02-01T18:23:45Z

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.

jpuri · 2024-02-02T06:26:59Z

app/scripts/lib/ppom/ppom-middleware.ts

+        }
      }
    } catch (error: any) {
      sentry?.captureException(error);


Not important in this PR, but later we can refactor out validation from here as middleware is not needed for async validation - validation can now be done in controllers.

jpuri · 2024-02-02T06:27:29Z

app/scripts/lib/transaction/util.ts

  providerRequestsCount?: Record<string, number>;
+  securityAlertId?: string;
 };



This files does not seems to be present in 11.9 branch.

jpuri · 2024-02-02T06:29:29Z

app/scripts/metamask-controller.js

+      if (!foundConfirmation) {
+        await new Promise((resolve) => setTimeout(resolve, 100));
+      }
+    }


If we move validation from middleware I think we can avoid above logic for search confirmation.

… ppom security alert response

…requests

digiwand

🙌🏼

Works as expected and displays the LoadingIndicator while validation is in progress. Left a couple of non-blocking comments

digiwand · 2024-02-05T19:11:26Z

app/scripts/controllers/app-state.js

+
+  addSignatureSecurityAlertResponse(securityAlertResponse) {
+    const currentState = this.store.getState();
+    const { signatureSecurityAlertResponses } = currentState; //


Suggested change

const { signatureSecurityAlertResponses } = currentState; //

const { signatureSecurityAlertResponses } = currentState;

digiwand · 2024-02-05T19:17:51Z

app/scripts/lib/ppom/ppom-middleware.ts

+            reason: 'loading',
+            result_type: 'validation_in_progress',


[nit] we could define these as local values in shared/constants/security-provider.ts

digiwand · 2024-02-05T19:22:57Z

shared/constants/transaction.ts

+  'eth_signTypedData_v1',
+  'eth_signTypedData_v3',
+  'eth_signTypedData_v4',
+  'personal_sign',


we spoke with the Blockaid team about removing validation for 'personal_sign' types. We can do this here or another PR

Suggested change

'personal_sign',

let's do this in a separate PR

jpuri

LGTM

seaona · 2024-02-06T09:18:18Z

QA findings

1. Error when initiating Send from inside the Wallet

Note: this issue is not happening on develop.

Whenever initiating a send from inside the wallet, I see the error TypeError: Cannot read properties of undefined (reading 'securityAlertId') when I land into the lasy Confirmation screen and the validation stays loading indefinetly

reading-security-alerts.mp4

2. e2e failures: Spinner stays loading indefinitely in failed validation case

Whenever we are in the case where the validation will fail, we now see the spinner loading indefinitely instead of displaying the Validation Failed warning

This is what's happening with the e2e failures: we simulated a failed validation by using this mock

  await mockServer
    .forPost()
    .withJsonBodyIncluding({
      method: 'debug_traceCall',
      params: [{ accessList: [], data: '0x00000000' }],
    })
    .thenCallback(() => {
      return {
        statusCode: 500,
      };
    });
}

and we can see on the screenshot that thet Failed Validation warning never appeared. Instead, the spinner stayed there

validation-failed.mp4

…uest fails

metamaskbot · 2024-02-06T16:33:18Z

Builds ready [a9f4524]

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
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (751 ± 29 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	83	165	109	20	9
		domContentLoaded	9	32	17	6	3
		load	686	940	751	61	29
		domInteractive	9	32	17	6	3

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

background: 2.46 KiB (0.07%)
ui: 418 Bytes (0.01%)
common: 262 Bytes (0.01%)

metamaskbot · 2024-02-06T17:08:50Z

Missing release label release-11.9.0 on PR. Adding release label release-11.9.0 on PR and removing other release labels(release-11.11.0), as PR was cherry-picked in branch 11.9.0.

danjm force-pushed the ppom-async-poc branch 3 times, most recently from 382c36c to 30df005 Compare February 2, 2024 05:53

danjm marked this pull request as ready for review February 2, 2024 06:07

danjm requested a review from a team as a code owner February 2, 2024 06:07

jpuri reviewed Feb 2, 2024

View reviewed changes

danjm force-pushed the ppom-async-poc branch from fa2b676 to 141b107 Compare February 5, 2024 16:05

danjm added 5 commits February 5, 2024 12:50

Stop blocking display of transaction approval window on resolution of…

ccb7db0

… ppom security alert response

Ensure async security alert response handling also handles signature …

21be5fe

…requests

Fix unit test

25cca58

fix e2e tests

59c9698

Fix ppom-middleware.test.ts

1ba896e

danjm force-pushed the ppom-async-poc branch from e8d30b9 to 1ba896e Compare February 5, 2024 16:27

digiwand previously approved these changes Feb 5, 2024

View reviewed changes

jpuri previously approved these changes Feb 6, 2024

View reviewed changes

seaona added the DO-NOT-MERGE Pull requests that should not be merged label Feb 6, 2024

danjm added 2 commits February 6, 2024 10:26

Fix blockaid validation when creating tx from Send, and when ppom req…

04ccd1c

…uest fails

Add signatureSecurityAlertResponses to SENTRY_BACKGROUND_STATE

52ff091

danjm dismissed stale reviews from jpuri and digiwand via 52ff091 February 6, 2024 13:57

seaona removed the DO-NOT-MERGE Pull requests that should not be merged label Feb 6, 2024

jpuri previously approved these changes Feb 6, 2024

View reviewed changes

Fix unit tests

f0fa366

danjm dismissed jpuri’s stale review via f0fa366 February 6, 2024 15:06

digiwand previously approved these changes Feb 6, 2024

View reviewed changes

Clean up

56b57be

danjm dismissed digiwand’s stale review via 56b57be February 6, 2024 15:13

jpuri previously approved these changes Feb 6, 2024

View reviewed changes

Fix multichain test

81a44f9

danjm dismissed jpuri’s stale review via 81a44f9 February 6, 2024 15:44

Disable test on multichain

a9f4524

danjm merged commit 0364739 into develop Feb 6, 2024

danjm deleted the ppom-async-poc branch February 6, 2024 16:26

github-actions bot locked and limited conversation to collaborators Feb 6, 2024

metamaskbot added the release-11.11.0 Issue or pull request that will be included in release 11.11.0 label Feb 6, 2024

metamaskbot added release-11.9.0 Issue or pull request that will be included in release 11.9.0 and removed release-11.11.0 Issue or pull request that will be included in release 11.11.0 labels Feb 6, 2024

	const { signatureSecurityAlertResponses } = currentState; //
	const { signatureSecurityAlertResponses } = currentState;

Stop blocking display of transaction approval window on resolution of ppom security alert response #22778

Stop blocking display of transaction approval window on resolution of ppom security alert response #22778

Uh oh!

Conversation

danjm commented Feb 1, 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 Feb 1, 2024

Uh oh!

jpuri Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpuri Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

jpuri Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

digiwand left a comment

Choose a reason for hiding this comment

Uh oh!

digiwand Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

digiwand Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

digiwand Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

danjm Feb 6, 2024

Choose a reason for hiding this comment

Uh oh!

jpuri left a comment

Choose a reason for hiding this comment

Uh oh!

seaona commented Feb 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

QA findings

1. Error when initiating Send from inside the Wallet

2. e2e failures: Spinner stays loading indefinitely in failed validation case

Uh oh!

metamaskbot commented Feb 6, 2024

Uh oh!

metamaskbot commented Feb 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

danjm commented Feb 1, 2024 •

edited

Loading

jpuri Feb 2, 2024 •

edited

Loading

seaona commented Feb 6, 2024 •

edited

Loading