fix: navigation between watch asset approvals (#29279)

## **Description** Ignore any additional watch token and watch NFT approvals in the confirmation navigation, since both types of watch asset confirmation combine data from multiple approval requests. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29279?quickstart=1) ## **Related issues** Fixes: #29189 ## **Manual testing steps** See issue. ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). 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.
MetaMask · Dec 20, 2024 · fde9fa1 · fde9fa1
1 parent 1c6391c
commit fde9fa1
Show file tree

Hide file tree

Showing 5 changed files with 138 additions and 68 deletions.
diff --git a/ui/pages/confirmations/hooks/alerts/transactions/usePendingTransactionAlerts.test.ts b/ui/pages/confirmations/hooks/alerts/transactions/usePendingTransactionAlerts.test.ts
@@ -4,34 +4,18 @@ import {
   TransactionStatus,
   TransactionType,
 } from '@metamask/transaction-controller';
-import { useSelector } from 'react-redux';
-import { useParams } from 'react-router-dom';
 import { genUnapprovedContractInteractionConfirmation } from '../../../../../../test/data/confirmations/contract-interaction';
 import { getMockConfirmState } from '../../../../../../test/data/confirmations/helper';
 import { renderHookWithConfirmContextProvider } from '../../../../../../test/lib/confirmations/render-helpers';
 import { RowAlertKey } from '../../../../../components/app/confirm/info/row/constants';
 import { Severity } from '../../../../../helpers/constants/design-system';
-import {
-  getRedesignedTransactionsEnabled,
-  submittedPendingTransactionsSelector,
-} from '../../../../../selectors';
 import { PendingTransactionAlertMessage } from './PendingTransactionAlertMessage';
 import { usePendingTransactionAlerts } from './usePendingTransactionAlerts';
 
-jest.mock('react-redux', () => ({
-  ...jest.requireActual('react-redux'),
-  useSelector: jest.fn(),
-}));
-
 jest.mock('./PendingTransactionAlertMessage', () => ({
   PendingTransactionAlertMessage: () => 'PendingTransactionAlertMessage',
 }));
 
-jest.mock('react-router-dom', () => ({
-  ...jest.requireActual('react-router-dom'),
-  useParams: jest.fn().mockReturnValue({ id: 'mock-transaction-id' }),
-}));
-
 const ACCOUNT_ADDRESS = '0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc';
 const TRANSACTION_ID_MOCK = '123-456';
 
@@ -59,6 +43,7 @@ function runHook({
   transactions?: TransactionMeta[];
 } = {}) {
   let pendingApprovals = {};
+
   if (currentConfirmation) {
     pendingApprovals = {
       [currentConfirmation.id as string]: {
@@ -68,12 +53,14 @@ function runHook({
     };
     transactions.push(currentConfirmation);
   }
+
   const state = getMockConfirmState({
     metamask: {
       pendingApprovals,
       transactions,
     },
   });
+
   const response = renderHookWithConfirmContextProvider(
     usePendingTransactionAlerts,
     state,
@@ -83,19 +70,8 @@ function runHook({
 }
 
 describe('usePendingTransactionAlerts', () => {
-  const useSelectorMock = useSelector as jest.Mock;
-
   beforeEach(() => {
     jest.resetAllMocks();
-
-    (useParams as jest.Mock).mockReturnValue({ id: 'mock-transaction-id' });
-
-    useSelectorMock.mockImplementation((selector) => {
-      if (selector.toString().includes('pendingApprovalsSortedSelector')) {
-        return [];
-      }
-      return undefined;
-    });
   });
 
   it('returns no alerts if no confirmation', () => {
@@ -152,24 +128,6 @@ describe('usePendingTransactionAlerts', () => {
   });
 
   it('returns alert if submitted transaction', () => {
-    useSelectorMock.mockImplementation((selector) => {
-      if (selector === submittedPendingTransactionsSelector) {
-        return [
-          { name: 'first transaction', id: '1' },
-          { name: 'second transaction', id: '2' },
-        ];
-      } else if (selector === getRedesignedTransactionsEnabled) {
-        return true;
-      } else if (selector.toString().includes('getUnapprovedTransaction')) {
-        return { type: TransactionType.contractInteraction };
-      } else if (
-        selector.toString().includes('pendingApprovalsSortedSelector')
-      ) {
-        return [];
-      }
-      return undefined;
-    });
-
     const alerts = runHook({
       currentConfirmation: CONFIRMATION_MOCK,
       transactions: [TRANSACTION_META_MOCK],

diff --git a/ui/pages/confirmations/hooks/useConfirmationNavigation.test.ts b/ui/pages/confirmations/hooks/useConfirmationNavigation.test.ts
@@ -28,34 +28,40 @@ jest.mock('../confirmation/templates', () => ({
 const APPROVAL_ID_MOCK = '123-456';
 const APPROVAL_ID_2_MOCK = '456-789';
 
-function renderHook(
-  approvalType: ApprovalType,
-  requestData?: Json,
-  approvalFlows?: ApprovalFlowState[],
-) {
+function renderHookWithState(state: Record<string, unknown>) {
   const { result } = renderHookWithProvider(() => useConfirmationNavigation(), {
     ...mockState,
     metamask: {
       ...mockState.metamask,
-      pendingApprovals: {
-        [APPROVAL_ID_MOCK]: {
-          id: APPROVAL_ID_MOCK,
-          type: approvalType,
-          requestData,
-        },
-        [APPROVAL_ID_2_MOCK]: {
-          id: APPROVAL_ID_2_MOCK,
-          type: approvalType,
-          requestData,
-        },
-      },
-      approvalFlows,
+      ...state,
     },
   });
 
   return result.current;
 }
 
+function renderHook(
+  approvalType: ApprovalType,
+  requestData?: Json,
+  approvalFlows?: ApprovalFlowState[],
+) {
+  return renderHookWithState({
+    pendingApprovals: {
+      [APPROVAL_ID_MOCK]: {
+        id: APPROVAL_ID_MOCK,
+        type: approvalType,
+        requestData,
+      },
+      [APPROVAL_ID_2_MOCK]: {
+        id: APPROVAL_ID_2_MOCK,
+        type: approvalType,
+        requestData,
+      },
+    },
+    approvalFlows,
+  });
+}
+
 describe('useConfirmationNavigation', () => {
   const useHistoryMock = jest.mocked(useHistory);
   const history = { replace: jest.fn() };
@@ -202,6 +208,36 @@ describe('useConfirmationNavigation', () => {
       const result = renderHook(ApprovalType.Transaction);
       expect(result.count).toBe(2);
     });
+
+    // @ts-expect-error This function is missing from the Mocha type definitions
+    it.each([
+      ['token', undefined],
+      ['NFT', '123'],
+    ])(
+      'ignores additional watch %s approvals',
+      (_title: string, tokenId?: string) => {
+        const result = renderHookWithState({
+          pendingApprovals: {
+            [APPROVAL_ID_MOCK]: {
+              id: APPROVAL_ID_MOCK,
+              type: ApprovalType.WatchAsset,
+              requestData: { asset: { tokenId } },
+            },
+            [APPROVAL_ID_2_MOCK]: {
+              id: APPROVAL_ID_2_MOCK,
+              type: ApprovalType.Transaction,
+            },
+            duplicate: {
+              id: 'duplicate',
+              type: ApprovalType.WatchAsset,
+              requestData: { asset: { tokenId } },
+            },
+          },
+        });
+
+        expect(result.count).toBe(2);
+      },
+    );
   });
 
   describe('getIndex', () => {
@@ -224,5 +260,37 @@ describe('useConfirmationNavigation', () => {
         APPROVAL_ID_2_MOCK,
       ]);
     });
+
+    // @ts-expect-error This function is missing from the Mocha type definitions
+    it.each([
+      ['token', undefined],
+      ['NFT', '123'],
+    ])(
+      'ignores additional watch %s approvals',
+      (_title: string, tokenId?: string) => {
+        const result = renderHookWithState({
+          pendingApprovals: {
+            [APPROVAL_ID_MOCK]: {
+              id: APPROVAL_ID_MOCK,
+              type: ApprovalType.WatchAsset,
+              requestData: { asset: { tokenId } },
+            },
+            [APPROVAL_ID_2_MOCK]: {
+              id: APPROVAL_ID_2_MOCK,
+              type: ApprovalType.Transaction,
+            },
+            duplicate: {
+              id: 'duplicate',
+              type: ApprovalType.WatchAsset,
+              requestData: { asset: { tokenId } },
+            },
+          },
+        });
+
+        expect(
+          result.confirmations.map(({ id }: { id: string }) => id),
+        ).toEqual([APPROVAL_ID_MOCK, APPROVAL_ID_2_MOCK]);
+      },
+    );
   });
 });
diff --git a/ui/pages/confirmations/hooks/useConfirmationNavigation.ts b/ui/pages/confirmations/hooks/useConfirmationNavigation.ts
@@ -19,7 +19,7 @@ import {
 import { isSignatureTransactionType } from '../utils';
 import {
   getApprovalFlows,
-  pendingApprovalsSortedSelector,
+  selectPendingApprovalsForNavigation,
 } from '../../../selectors';
 
 const CONNECT_APPROVAL_TYPES = [
@@ -30,7 +30,7 @@ const CONNECT_APPROVAL_TYPES = [
 ];
 
 export function useConfirmationNavigation() {
-  const confirmations = useSelector(pendingApprovalsSortedSelector, isEqual);
+  const confirmations = useSelector(selectPendingApprovalsForNavigation);
   const approvalFlows = useSelector(getApprovalFlows, isEqual);
   const history = useHistory();
 

diff --git a/ui/pages/home/home.container.js b/ui/pages/home/home.container.js
@@ -40,7 +40,7 @@ import {
   getSelectedInternalAccount,
   getQueuedRequestCount,
   getEditedNetwork,
-  pendingApprovalsSortedSelector,
+  selectPendingApprovalsForNavigation,
   ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
   getAccountType,
   ///: END:ONLY_INCLUDE_IF
@@ -108,7 +108,7 @@ const mapStateToProps = (state) => {
   const totalUnapprovedAndQueuedRequestCount =
     totalUnapprovedCount + queuedRequestCount;
   const swapsEnabled = getSwapsFeatureIsLive(state);
-  const pendingApprovals = pendingApprovalsSortedSelector(state);
+  const pendingApprovals = selectPendingApprovalsForNavigation(state);
 
   ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
   const institutionalConnectRequests = getInstitutionalConnectRequests(state);

diff --git a/ui/selectors/approvals.ts b/ui/selectors/approvals.ts
@@ -1,6 +1,10 @@
-import { ApprovalControllerState } from '@metamask/approval-controller';
+import {
+  ApprovalControllerState,
+  ApprovalRequest,
+} from '@metamask/approval-controller';
 import { ApprovalType } from '@metamask/controller-utils';
 import { createSelector } from 'reselect';
+import { Json } from '@metamask/utils';
 import { createDeepEqualSelector } from '../../shared/modules/selectors/util';
 
 export type ApprovalsMetaMaskState = {
@@ -58,6 +62,32 @@ export function pendingApprovalsSortedSelector(state: ApprovalsMetaMaskState) {
   return getPendingApprovals(state).sort((a1, a2) => a1.time - a2.time);
 }
 
+/**
+ * Returns pending approvals sorted by time for use in confirmation navigation.
+ * Excludes duplicate watch asset approvals as they are combined into a single confirmation.
+ */
+export const selectPendingApprovalsForNavigation = createDeepEqualSelector(
+  pendingApprovalsSortedSelector,
+  (sortedPendingApprovals) =>
+    sortedPendingApprovals.filter((approval, index) => {
+      if (
+        isWatchNftApproval(approval) &&
+        sortedPendingApprovals.findIndex(isWatchNftApproval) !== index
+      ) {
+        return false;
+      }
+
+      if (
+        isWatchTokenApproval(approval) &&
+        sortedPendingApprovals.findIndex(isWatchTokenApproval) !== index
+      ) {
+        return false;
+      }
+
+      return true;
+    }),
+);
+
 const internalSelectPendingApproval = createSelector(
   getPendingApprovals,
   (_state: ApprovalsMetaMaskState, id: string) => id,
@@ -68,3 +98,17 @@ export const selectPendingApproval = createDeepEqualSelector(
   internalSelectPendingApproval,
   (approval) => approval,
 );
+
+function isWatchTokenApproval(approval: ApprovalRequest<Record<string, Json>>) {
+  const tokenId = (approval.requestData?.asset as Record<string, string>)
+    ?.tokenId;
+
+  return approval.type === ApprovalType.WatchAsset && !tokenId;
+}
+
+function isWatchNftApproval(approval: ApprovalRequest<Record<string, Json>>) {
+  const tokenId = (approval.requestData?.asset as Record<string, string>)
+    ?.tokenId;
+
+  return approval.type === ApprovalType.WatchAsset && Boolean(tokenId);
+}