fix: shield coverage result polling

packages/shield-controller/CHANGELOG.md

@@ -7,6 +7,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+
+- Updated internal coverage result polling and log logic. ([#6847](https://github.com/MetaMask/core/pull/6847))
+  - Added cancellation logic to the polling.
+  - Updated implementation of timeout.
+  - Cancel any pending requests before starting new polling or logging.
+- Updated TransactionMeta comparison in `TransactionController:stateChange` subscriber to avoid triggering multiple check coverage result unnecessarily. ([#6847](https://github.com/MetaMask/core/pull/6847))
+- Removed `Personal Sign` check from the check signature coverage result. ([#6847](https://github.com/MetaMask/core/pull/6847))
+
 ## [0.3.2]
 
 ### Changed

packages/shield-controller/src/ShieldController.test.ts

@@ -11,6 +11,7 @@ import {
 } from '@metamask/transaction-controller';
 
 import { ShieldController } from './ShieldController';
+import { TX_META_SIMULATION_DATA_MOCKS } from '../tests/data';
 import { createMockBackend, MOCK_COVERAGE_ID } from '../tests/mocks/backend';
 import { createMockMessenger } from '../tests/mocks/messenger';
 import {
@@ -169,6 +170,47 @@ describe('ShieldController', () => {
       });
     });
 
+    TX_META_SIMULATION_DATA_MOCKS.forEach(
+      ({ description, previousSimulationData, newSimulationData }) => {
+        it(`should check coverage when ${description}`, async () => {
+          const { baseMessenger, backend } = setup();
+          const previousTxMeta = {
+            ...generateMockTxMeta(),
+            simulationData: previousSimulationData,
+          };
+          const coverageResultReceived =
+            setupCoverageResultReceived(baseMessenger);
+
+          // Add transaction.
+          baseMessenger.publish(
+            'TransactionController:stateChange',
+            { transactions: [previousTxMeta] } as TransactionControllerState,
+            undefined as never,
+          );
+          expect(await coverageResultReceived).toBeUndefined();
+          expect(backend.checkCoverage).toHaveBeenCalledWith({
+            txMeta: previousTxMeta,
+          });
+
+          // Simulate transaction.
+          const txMeta2 = { ...previousTxMeta };
+          txMeta2.simulationData = newSimulationData;
+          const coverageResultReceived2 =
+            setupCoverageResultReceived(baseMessenger);
+          baseMessenger.publish(
+            'TransactionController:stateChange',
+            { transactions: [txMeta2] } as TransactionControllerState,
+            undefined as never,
+          );
+          expect(await coverageResultReceived2).toBeUndefined();
+          expect(backend.checkCoverage).toHaveBeenCalledWith({
+            coverageId: MOCK_COVERAGE_ID,
+            txMeta: txMeta2,
+          });
+        });
+      },
+    );
+
     it('throws an error when the coverage ID has changed', async () => {
       const { controller, backend } = setup();
       backend.checkCoverage.mockResolvedValueOnce({

packages/shield-controller/src/ShieldController.ts

@@ -5,7 +5,6 @@ import type {
 } from '@metamask/base-controller';
 import {
   SignatureRequestStatus,
-  SignatureRequestType,
   type SignatureRequest,
   type SignatureStateChange,
 } from '@metamask/signature-controller';
@@ -236,10 +235,7 @@ export class ShieldController extends BaseController<
 
       // Check coverage if the signature request is new and has type
       // `personal_sign`.
-      if (
-        !previousSignatureRequest &&
-        signatureRequest.type === SignatureRequestType.PersonalSign
-      ) {
+      if (!previousSignatureRequest) {
         this.checkSignatureCoverage(signatureRequest).catch(
           // istanbul ignore next
           (error) => log('Error checking coverage:', error),
@@ -268,15 +264,15 @@ export class ShieldController extends BaseController<
     );
     for (const transaction of transactions) {
       const previousTransaction = previousTransactionsById.get(transaction.id);
+      // Check if the simulation data has changed.
+      const simulationDataChanged = this.#compareTransactionSimulationData(
+        transaction.simulationData,
+        previousTransaction?.simulationData,
+      );
 
       // Check coverage if the transaction is new or if the simulation data has
       // changed.
-      if (
-        !previousTransaction ||
-        // Checking reference equality is sufficient because this object is
-        // replaced if the simulation data has changed.
-        previousTransaction.simulationData !== transaction.simulationData
-      ) {
+      if (!previousTransaction || simulationDataChanged) {
         this.checkCoverage(transaction).catch(
           // istanbul ignore next
           (error) => log('Error checking coverage:', error),
@@ -443,4 +439,61 @@ export class ShieldController extends BaseController<
   #getLatestCoverageId(itemId: string): string | undefined {
     return this.state.coverageResults[itemId]?.results[0]?.coverageId;
   }
+
+  /**
+   * Compares the simulation data of a transaction.
+   *
+   * @param simulationData - The simulation data of the transaction.
+   * @param previousSimulationData - The previous simulation data of the transaction.
+   * @returns Whether the simulation data has changed.
+   */
+  #compareTransactionSimulationData(
+    simulationData?: TransactionMeta['simulationData'],
+    previousSimulationData?: TransactionMeta['simulationData'],
+  ) {
+    if (!simulationData && !previousSimulationData) {
+      return false;
+    }
+
+    // check the simulation error
+    if (
+      simulationData?.error?.code !== previousSimulationData?.error?.code ||
+      simulationData?.error?.message !== previousSimulationData?.error?.message
+    ) {
+      return true;
+    }
+
+    // check the native balance change
+    if (
+      simulationData?.nativeBalanceChange?.difference !==
+        previousSimulationData?.nativeBalanceChange?.difference ||
+      simulationData?.nativeBalanceChange?.newBalance !==
+        previousSimulationData?.nativeBalanceChange?.newBalance ||
+      simulationData?.nativeBalanceChange?.previousBalance !==
+        previousSimulationData?.nativeBalanceChange?.previousBalance ||
+      simulationData?.nativeBalanceChange?.isDecrease !==
+        previousSimulationData?.nativeBalanceChange?.isDecrease
+    ) {
+      return true;
+    }
+
+    // check the token balance changes
+    if (
+      simulationData?.tokenBalanceChanges?.length !==
+        previousSimulationData?.tokenBalanceChanges?.length ||
+      simulationData?.tokenBalanceChanges?.some(
+        (tokenBalanceChange, index) =>
+          tokenBalanceChange.difference !==
+          previousSimulationData?.tokenBalanceChanges?.[index]?.difference,
+      )
+    ) {
+      return true;
+    }
+
+    // check the isUpdatedAfterSecurityCheck
+    return (
+      simulationData?.isUpdatedAfterSecurityCheck !==
+      previousSimulationData?.isUpdatedAfterSecurityCheck
+    );
+  }
 }