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

## **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.


<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

## **Related issues**

Fixes:

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've clearly explained what problem this PR is solving and how it
is solved.
- [ ] I've linked related issues
- [ ] I've included manual testing steps
- [ ] I've included screenshots/recordings if applicable
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [ ] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [ ] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **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.

Author: danjm

app/scripts/controllers/app-state.js

@@ -65,6 +65,7 @@ export default class AppStateController extends EventEmitter {
         '0x539': true,
       },
       surveyLinkLastClickedOrClosed: null,
+      signatureSecurityAlertResponses: {},
     });
     this.timer = null;
 
@@ -441,8 +442,19 @@ export default class AppStateController extends EventEmitter {
       },
     });
   }
-
   ///: END:ONLY_INCLUDE_IF
+
+  addSignatureSecurityAlertResponse(securityAlertResponse) {
+    const currentState = this.store.getState();
+    const { signatureSecurityAlertResponses } = currentState;
+    this.store.updateState({
+      signatureSecurityAlertResponses: {
+        ...signatureSecurityAlertResponses,
+        [securityAlertResponse.securityAlertId]: securityAlertResponse,
+      },
+    });
+  }
+
   /**
    * A setter for the currentPopupId which indicates the id of popup window that's currently active
    *

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

@@ -91,7 +91,7 @@ describe('PPOMMiddleware', () => {
     const usePPOM = async () => {
       throw new Error('some error');
     };
-    const middlewareFunction = createMiddleWare(usePPOM);
+    const middlewareFunction = createMiddleWare({ usePPOM });
     const req = {
       method: 'eth_sendTransaction',
       securityAlertResponse: undefined,

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

@@ -1,25 +1,23 @@
 import { PPOM } from '@blockaid/ppom_release';
 import { PPOMController } from '@metamask/ppom-validator';
 import { NetworkController } from '@metamask/network-controller';
+import { v4 as uuid } from 'uuid';
 
 import {
   BlockaidReason,
   BlockaidResultType,
 } from '../../../../shared/constants/security-provider';
 import { CHAIN_IDS } from '../../../../shared/constants/network';
+import { SIGNING_METHODS } from '../../../../shared/constants/transaction';
 import { PreferencesController } from '../../controllers/preferences';
+import { SecurityAlertResponse } from '../transaction/util';
 
 const { sentry } = global as any;
 
-const ConfirmationMethods = Object.freeze([
+const CONFIRMATION_METHODS = Object.freeze([
   'eth_sendRawTransaction',
   'eth_sendTransaction',
-  'eth_sign',
-  'eth_signTypedData',
-  'eth_signTypedData_v1',
-  'eth_signTypedData_v3',
-  'eth_signTypedData_v4',
-  'personal_sign',
+  ...SIGNING_METHODS,
 ]);
 
 export const SUPPORTED_CHAIN_IDS: string[] = [
@@ -44,12 +42,19 @@ export const SUPPORTED_CHAIN_IDS: string[] = [
  * @param ppomController - Instance of PPOMController.
  * @param preferencesController - Instance of PreferenceController.
  * @param networkController - Instance of NetworkController.
+ * @param appStateController
+ * @param updateSecurityAlertResponseByTxId
  * @returns PPOMMiddleware function.
  */
 export function createPPOMMiddleware(
   ppomController: PPOMController,
   preferencesController: PreferencesController,
   networkController: NetworkController,
+  appStateController: any,
+  updateSecurityAlertResponseByTxId: (
+    req: any,
+    securityAlertResponse: SecurityAlertResponse,
+  ) => void,
 ) {
   return async (req: any, _res: any, next: () => void) => {
     try {
@@ -58,15 +63,46 @@ export function createPPOMMiddleware(
       const { chainId } = networkController.state.providerConfig;
       if (
         securityAlertsEnabled &&
-        ConfirmationMethods.includes(req.method) &&
+        CONFIRMATION_METHODS.includes(req.method) &&
         SUPPORTED_CHAIN_IDS.includes(chainId)
       ) {
         // eslint-disable-next-line require-atomic-updates
-        req.securityAlertResponse = await ppomController.usePPOM(
-          async (ppom: PPOM) => {
-            return ppom.validateJsonRpc(req);
-          },
-        );
+        const securityAlertId = uuid();
+
+        ppomController.usePPOM(async (ppom: PPOM) => {
+          try {
+            const securityAlertResponse = await ppom.validateJsonRpc(req);
+            securityAlertResponse.securityAlertId = securityAlertId;
+            updateSecurityAlertResponseByTxId(req, securityAlertResponse);
+          } catch (error: any) {
+            sentry?.captureException(error);
+            console.error('Error validating JSON RPC using PPOM: ', error);
+            const securityAlertResponse = {
+              result_type: BlockaidResultType.Failed,
+              reason: BlockaidReason.failed,
+              description:
+                'Validating the confirmation failed by throwing error.',
+            };
+            updateSecurityAlertResponseByTxId(req, securityAlertResponse);
+          }
+        });
+
+        if (SIGNING_METHODS.includes(req.method)) {
+          req.securityAlertResponse = {
+            securityAlertId,
+          };
+          appStateController.addSignatureSecurityAlertResponse({
+            reason: BlockaidResultType.Loading,
+            result_type: BlockaidReason.inProgress,
+            securityAlertId,
+          });
+        } else {
+          req.securityAlertResponse = {
+            reason: BlockaidResultType.Loading,
+            result_type: BlockaidReason.inProgress,
+            securityAlertId,
+          };
+        }
       }
     } catch (error: any) {
       sentry?.captureException(error);

app/scripts/lib/setupSentry.js

@@ -211,6 +211,7 @@ export const SENTRY_BACKGROUND_STATE = {
     selectedAddress: false,
     snapRegistryList: false,
     theme: true,
+    signatureSecurityAlertResponses: false,
     transactionSecurityCheckEnabled: true,
     use4ByteResolution: true,
     useAddressBarEnsResolution: true,

app/scripts/lib/transaction/util.test.ts

@@ -16,6 +16,15 @@ import {
   addTransaction,
 } from './util';
 
+jest.mock('uuid', () => {
+  const actual = jest.requireActual('uuid');
+
+  return {
+    ...actual,
+    v4: jest.fn(),
+  };
+});
+
 const TRANSACTION_PARAMS_MOCK: TransactionParams = {
   from: '0x1',
 };
@@ -380,8 +389,8 @@ describe('Transaction Utils', () => {
         ).toHaveBeenCalledWith(TRANSACTION_PARAMS_MOCK, {
           ...TRANSACTION_OPTIONS_MOCK,
           securityAlertResponse: {
-            reason: 'testReason',
-            result_type: 'testResultType',
+            reason: 'loading',
+            result_type: 'validation_in_progress',
           },
         });
 

app/scripts/lib/transaction/util.ts

@@ -3,6 +3,10 @@ import {
   TransactionController,
   TransactionMeta,
   TransactionParams,
+  WalletDevice,
+  TransactionType,
+  SendFlowHistoryEntry,
+  Result,
 } from '@metamask/transaction-controller';
 import {
   AddUserOperationOptions,
@@ -12,11 +16,45 @@ import {
 import { PPOMController } from '@metamask/ppom-validator';
 import { captureException } from '@sentry/browser';
 import { addHexPrefix } from 'ethereumjs-util';
+import { v4 as uuid } from 'uuid';
 import { SUPPORTED_CHAIN_IDS } from '../ppom/ppom-middleware';
+import {
+  BlockaidReason,
+  BlockaidResultType,
+} from '../../../../shared/constants/security-provider';
 ///: END:ONLY_INCLUDE_IF
 
+/**
+ * Type for security alert response from transaction validator.
+ */
+export type SecurityAlertResponse = {
+  reason: string;
+  features?: string[];
+  result_type: string;
+  providerRequestsCount?: Record<string, number>;
+  securityAlertId?: string;
+};
+
 export type AddTransactionOptions = NonNullable<
-  Parameters<TransactionController['addTransaction']>[1]
+  Parameters<
+    (
+      txParams: TransactionParams,
+      options?: {
+        actionId?: string;
+        deviceConfirmedOn?: WalletDevice;
+        method?: string;
+        origin?: string;
+        requireApproval?: boolean | undefined;
+        securityAlertResponse?: SecurityAlertResponse;
+        sendFlowHistory?: SendFlowHistoryEntry[];
+        swaps?: {
+          hasApproveTx?: boolean;
+          meta?: Partial<TransactionMeta>;
+        };
+        type?: TransactionType;
+      },
+    ) => Promise<Result>
+  >[1]
 >;
 
 type BaseAddTransactionRequest = {
@@ -30,16 +68,6 @@ type BaseAddTransactionRequest = {
   userOperationController: UserOperationController;
 };
 
-/**
- * Type for security alert response from transaction validator.
- */
-export type SecurityAlertResponse = {
-  reason: string;
-  features?: string[];
-  result_type: string;
-  providerRequestsCount?: Record<string, number>;
-};
-
 type FinalAddTransactionRequest = BaseAddTransactionRequest & {
   transactionOptions: AddTransactionOptions;
 };
@@ -83,6 +111,12 @@ export async function addDappTransaction(
 
 export async function addTransaction(
   request: AddTransactionRequest,
+  ///: BEGIN:ONLY_INCLUDE_IF(blockaid)
+  updateSecurityAlertResponseByTxId: (
+    req: AddTransactionOptions | undefined,
+    securityAlertResponse: SecurityAlertResponse,
+  ) => void,
+  ///: END:ONLY_INCLUDE_IF
 ): Promise<TransactionMeta> {
   ///: BEGIN:ONLY_INCLUDE_IF(blockaid)
   const {
@@ -109,13 +143,36 @@ export async function addTransaction(
         ],
       };
 
-      const securityAlertResponse = await ppomController.usePPOM(
-        async (ppom) => {
-          return ppom.validateJsonRpc(ppomRequest);
-        },
-      );
-
-      request.transactionOptions.securityAlertResponse = securityAlertResponse;
+      const securityAlertId = uuid();
+
+      ppomController.usePPOM(async (ppom) => {
+        try {
+          const securityAlertResponse = await ppom.validateJsonRpc(ppomRequest);
+          updateSecurityAlertResponseByTxId(
+            request.transactionOptions,
+            securityAlertResponse,
+          );
+        } catch (e) {
+          captureException(e);
+          console.error('Error validating JSON RPC using PPOM: ', e);
+          const securityAlertResponse = {
+            result_type: BlockaidResultType.Failed,
+            reason: BlockaidReason.failed,
+            description:
+              'Validating the confirmation failed by throwing error.',
+          };
+          updateSecurityAlertResponseByTxId(
+            request.transactionOptions,
+            securityAlertResponse,
+          );
+        }
+      });
+
+      request.transactionOptions.securityAlertResponse = {
+        reason: BlockaidResultType.Loading,
+        result_type: BlockaidReason.inProgress,
+        securityAlertId,
+      };
     } catch (e) {
       captureException(e);
     }

app/scripts/metamask-controller.js

@@ -143,7 +143,11 @@ import { BrowserRuntimePostMessageStream } from '@metamask/post-message-stream';
 import { toChecksumHexAddress } from '../../shared/modules/hexstring-utils';
 ///: END:ONLY_INCLUDE_IF
 
-import { AssetType, TokenStandard } from '../../shared/constants/transaction';
+import {
+  AssetType,
+  TokenStandard,
+  SIGNING_METHODS,
+} from '../../shared/constants/transaction';
 import {
   GAS_API_BASE_URL,
   GAS_DEV_API_BASE_URL,
@@ -2986,6 +2990,7 @@ export default class MetamaskController extends EventEmitter {
             transactionOptions,
             waitForSubmit: false,
           }),
+          this.updateSecurityAlertResponseByTxId.bind(this),
         ),
       addTransactionAndWaitForPublish: (
         transactionParams,
@@ -2997,6 +3002,7 @@ export default class MetamaskController extends EventEmitter {
             transactionOptions,
             waitForSubmit: true,
           }),
+          this.updateSecurityAlertResponseByTxId.bind(this),
         ),
       createTransactionEventFragment:
         createTransactionEventFragmentWithTxId.bind(
@@ -4251,6 +4257,42 @@ export default class MetamaskController extends EventEmitter {
     }
   };
 
+  async updateSecurityAlertResponseByTxId(req, securityAlertResponse) {
+    let foundConfirmation = false;
+
+    while (!foundConfirmation) {
+      if (SIGNING_METHODS.includes(req.method)) {
+        foundConfirmation = Object.values(
+          this.signatureController.messages,
+        ).find(
+          (message) =>
+            message.securityAlertResponse?.securityAlertId ===
+            req.securityAlertResponse.securityAlertId,
+        );
+      } else {
+        foundConfirmation = this.txController.state.transactions.find(
+          (meta) =>
+            meta.securityAlertResponse?.securityAlertId ===
+            req.securityAlertResponse.securityAlertId,
+        );
+      }
+      if (!foundConfirmation) {
+        await new Promise((resolve) => setTimeout(resolve, 100));
+      }
+    }
+
+    if (SIGNING_METHODS.includes(req.method)) {
+      this.appStateController.addSignatureSecurityAlertResponse(
+        securityAlertResponse,
+      );
+    } else {
+      this.txController.updateSecurityAlertResponse(
+        foundConfirmation.id,
+        securityAlertResponse,
+      );
+    }
+  }
+
   //=============================================================================
   // PASSWORD MANAGEMENT
   //=============================================================================
@@ -4643,6 +4685,8 @@ export default class MetamaskController extends EventEmitter {
         this.ppomController,
         this.preferencesController,
         this.networkController,
+        this.appStateController,
+        this.updateSecurityAlertResponseByTxId.bind(this),
       ),
     );
     ///: END:ONLY_INCLUDE_IF

app/scripts/metamask-controller.test.js

@@ -1084,6 +1084,9 @@ describe('MetaMaskController', () => {
 
       beforeEach(() => {
         initializeMockMiddlewareLog();
+        metamaskController.preferencesController.setSecurityAlertsEnabled(
+          false,
+        );
       });
 
       afterAll(() => {