From b24c4e95915973a715b761ede457ea61911b63b9 Mon Sep 17 00:00:00 2001 From: Matthew Walsh Date: Fri, 20 Dec 2024 22:34:31 +0000 Subject: [PATCH] fix (cherry-pick): navigation between watch asset approvals #29279 (#29401) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Cherry-pick of #29279 for release `12.10.0`. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29401?quickstart=1) ## **Related issues** ## **Manual testing steps** ## **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. --- .../usePendingTransactionAlerts.test.ts | 48 +------- .../hooks/useConfirmationNavigation.test.ts | 104 +++++++++++++++--- .../hooks/useConfirmationNavigation.ts | 4 +- ui/pages/home/home.container.js | 4 +- ui/selectors/approvals.ts | 46 +++++++- 5 files changed, 138 insertions(+), 68 deletions(-) diff --git a/ui/pages/confirmations/hooks/alerts/transactions/usePendingTransactionAlerts.test.ts b/ui/pages/confirmations/hooks/alerts/transactions/usePendingTransactionAlerts.test.ts index f7be0f93e2c1..e045a648f484 100644 --- 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 index edaddc0e520f..9804e14a0e00 100644 --- 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) { 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 index 9a5cdfc5cd9e..95bc1d93cd34 100644 --- 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 index 4fb46ccc1744..cae73b180f4d 100644 --- 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 index 55a5d052579c..f8f937bb2f34 100644 --- 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>) { + const tokenId = (approval.requestData?.asset as Record) + ?.tokenId; + + return approval.type === ApprovalType.WatchAsset && !tokenId; +} + +function isWatchNftApproval(approval: ApprovalRequest>) { + const tokenId = (approval.requestData?.asset as Record) + ?.tokenId; + + return approval.type === ApprovalType.WatchAsset && Boolean(tokenId); +}