Skip to content

Commit b24c4e9

Browse files
fix (cherry-pick): navigation between watch asset approvals #29279 (#29401)
## **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.
1 parent a2f4ee5 commit b24c4e9

File tree

5 files changed

+138
-68
lines changed

5 files changed

+138
-68
lines changed

Diff for: ui/pages/confirmations/hooks/alerts/transactions/usePendingTransactionAlerts.test.ts

+3-45
Original file line numberDiff line numberDiff line change
@@ -4,34 +4,18 @@ import {
44
TransactionStatus,
55
TransactionType,
66
} from '@metamask/transaction-controller';
7-
import { useSelector } from 'react-redux';
8-
import { useParams } from 'react-router-dom';
97
import { genUnapprovedContractInteractionConfirmation } from '../../../../../../test/data/confirmations/contract-interaction';
108
import { getMockConfirmState } from '../../../../../../test/data/confirmations/helper';
119
import { renderHookWithConfirmContextProvider } from '../../../../../../test/lib/confirmations/render-helpers';
1210
import { RowAlertKey } from '../../../../../components/app/confirm/info/row/constants';
1311
import { Severity } from '../../../../../helpers/constants/design-system';
14-
import {
15-
getRedesignedTransactionsEnabled,
16-
submittedPendingTransactionsSelector,
17-
} from '../../../../../selectors';
1812
import { PendingTransactionAlertMessage } from './PendingTransactionAlertMessage';
1913
import { usePendingTransactionAlerts } from './usePendingTransactionAlerts';
2014

21-
jest.mock('react-redux', () => ({
22-
...jest.requireActual('react-redux'),
23-
useSelector: jest.fn(),
24-
}));
25-
2615
jest.mock('./PendingTransactionAlertMessage', () => ({
2716
PendingTransactionAlertMessage: () => 'PendingTransactionAlertMessage',
2817
}));
2918

30-
jest.mock('react-router-dom', () => ({
31-
...jest.requireActual('react-router-dom'),
32-
useParams: jest.fn().mockReturnValue({ id: 'mock-transaction-id' }),
33-
}));
34-
3519
const ACCOUNT_ADDRESS = '0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc';
3620
const TRANSACTION_ID_MOCK = '123-456';
3721

@@ -59,6 +43,7 @@ function runHook({
5943
transactions?: TransactionMeta[];
6044
} = {}) {
6145
let pendingApprovals = {};
46+
6247
if (currentConfirmation) {
6348
pendingApprovals = {
6449
[currentConfirmation.id as string]: {
@@ -68,12 +53,14 @@ function runHook({
6853
};
6954
transactions.push(currentConfirmation);
7055
}
56+
7157
const state = getMockConfirmState({
7258
metamask: {
7359
pendingApprovals,
7460
transactions,
7561
},
7662
});
63+
7764
const response = renderHookWithConfirmContextProvider(
7865
usePendingTransactionAlerts,
7966
state,
@@ -83,19 +70,8 @@ function runHook({
8370
}
8471

8572
describe('usePendingTransactionAlerts', () => {
86-
const useSelectorMock = useSelector as jest.Mock;
87-
8873
beforeEach(() => {
8974
jest.resetAllMocks();
90-
91-
(useParams as jest.Mock).mockReturnValue({ id: 'mock-transaction-id' });
92-
93-
useSelectorMock.mockImplementation((selector) => {
94-
if (selector.toString().includes('pendingApprovalsSortedSelector')) {
95-
return [];
96-
}
97-
return undefined;
98-
});
9975
});
10076

10177
it('returns no alerts if no confirmation', () => {
@@ -152,24 +128,6 @@ describe('usePendingTransactionAlerts', () => {
152128
});
153129

154130
it('returns alert if submitted transaction', () => {
155-
useSelectorMock.mockImplementation((selector) => {
156-
if (selector === submittedPendingTransactionsSelector) {
157-
return [
158-
{ name: 'first transaction', id: '1' },
159-
{ name: 'second transaction', id: '2' },
160-
];
161-
} else if (selector === getRedesignedTransactionsEnabled) {
162-
return true;
163-
} else if (selector.toString().includes('getUnapprovedTransaction')) {
164-
return { type: TransactionType.contractInteraction };
165-
} else if (
166-
selector.toString().includes('pendingApprovalsSortedSelector')
167-
) {
168-
return [];
169-
}
170-
return undefined;
171-
});
172-
173131
const alerts = runHook({
174132
currentConfirmation: CONFIRMATION_MOCK,
175133
transactions: [TRANSACTION_META_MOCK],

Diff for: ui/pages/confirmations/hooks/useConfirmationNavigation.test.ts

+86-18
Original file line numberDiff line numberDiff line change
@@ -28,34 +28,40 @@ jest.mock('../confirmation/templates', () => ({
2828
const APPROVAL_ID_MOCK = '123-456';
2929
const APPROVAL_ID_2_MOCK = '456-789';
3030

31-
function renderHook(
32-
approvalType: ApprovalType,
33-
requestData?: Json,
34-
approvalFlows?: ApprovalFlowState[],
35-
) {
31+
function renderHookWithState(state: Record<string, unknown>) {
3632
const { result } = renderHookWithProvider(() => useConfirmationNavigation(), {
3733
...mockState,
3834
metamask: {
3935
...mockState.metamask,
40-
pendingApprovals: {
41-
[APPROVAL_ID_MOCK]: {
42-
id: APPROVAL_ID_MOCK,
43-
type: approvalType,
44-
requestData,
45-
},
46-
[APPROVAL_ID_2_MOCK]: {
47-
id: APPROVAL_ID_2_MOCK,
48-
type: approvalType,
49-
requestData,
50-
},
51-
},
52-
approvalFlows,
36+
...state,
5337
},
5438
});
5539

5640
return result.current;
5741
}
5842

43+
function renderHook(
44+
approvalType: ApprovalType,
45+
requestData?: Json,
46+
approvalFlows?: ApprovalFlowState[],
47+
) {
48+
return renderHookWithState({
49+
pendingApprovals: {
50+
[APPROVAL_ID_MOCK]: {
51+
id: APPROVAL_ID_MOCK,
52+
type: approvalType,
53+
requestData,
54+
},
55+
[APPROVAL_ID_2_MOCK]: {
56+
id: APPROVAL_ID_2_MOCK,
57+
type: approvalType,
58+
requestData,
59+
},
60+
},
61+
approvalFlows,
62+
});
63+
}
64+
5965
describe('useConfirmationNavigation', () => {
6066
const useHistoryMock = jest.mocked(useHistory);
6167
const history = { replace: jest.fn() };
@@ -202,6 +208,36 @@ describe('useConfirmationNavigation', () => {
202208
const result = renderHook(ApprovalType.Transaction);
203209
expect(result.count).toBe(2);
204210
});
211+
212+
// @ts-expect-error This function is missing from the Mocha type definitions
213+
it.each([
214+
['token', undefined],
215+
['NFT', '123'],
216+
])(
217+
'ignores additional watch %s approvals',
218+
(_title: string, tokenId?: string) => {
219+
const result = renderHookWithState({
220+
pendingApprovals: {
221+
[APPROVAL_ID_MOCK]: {
222+
id: APPROVAL_ID_MOCK,
223+
type: ApprovalType.WatchAsset,
224+
requestData: { asset: { tokenId } },
225+
},
226+
[APPROVAL_ID_2_MOCK]: {
227+
id: APPROVAL_ID_2_MOCK,
228+
type: ApprovalType.Transaction,
229+
},
230+
duplicate: {
231+
id: 'duplicate',
232+
type: ApprovalType.WatchAsset,
233+
requestData: { asset: { tokenId } },
234+
},
235+
},
236+
});
237+
238+
expect(result.count).toBe(2);
239+
},
240+
);
205241
});
206242

207243
describe('getIndex', () => {
@@ -224,5 +260,37 @@ describe('useConfirmationNavigation', () => {
224260
APPROVAL_ID_2_MOCK,
225261
]);
226262
});
263+
264+
// @ts-expect-error This function is missing from the Mocha type definitions
265+
it.each([
266+
['token', undefined],
267+
['NFT', '123'],
268+
])(
269+
'ignores additional watch %s approvals',
270+
(_title: string, tokenId?: string) => {
271+
const result = renderHookWithState({
272+
pendingApprovals: {
273+
[APPROVAL_ID_MOCK]: {
274+
id: APPROVAL_ID_MOCK,
275+
type: ApprovalType.WatchAsset,
276+
requestData: { asset: { tokenId } },
277+
},
278+
[APPROVAL_ID_2_MOCK]: {
279+
id: APPROVAL_ID_2_MOCK,
280+
type: ApprovalType.Transaction,
281+
},
282+
duplicate: {
283+
id: 'duplicate',
284+
type: ApprovalType.WatchAsset,
285+
requestData: { asset: { tokenId } },
286+
},
287+
},
288+
});
289+
290+
expect(
291+
result.confirmations.map(({ id }: { id: string }) => id),
292+
).toEqual([APPROVAL_ID_MOCK, APPROVAL_ID_2_MOCK]);
293+
},
294+
);
227295
});
228296
});

Diff for: ui/pages/confirmations/hooks/useConfirmationNavigation.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ import {
1919
import { isSignatureTransactionType } from '../utils';
2020
import {
2121
getApprovalFlows,
22-
pendingApprovalsSortedSelector,
22+
selectPendingApprovalsForNavigation,
2323
} from '../../../selectors';
2424

2525
const CONNECT_APPROVAL_TYPES = [
@@ -30,7 +30,7 @@ const CONNECT_APPROVAL_TYPES = [
3030
];
3131

3232
export function useConfirmationNavigation() {
33-
const confirmations = useSelector(pendingApprovalsSortedSelector, isEqual);
33+
const confirmations = useSelector(selectPendingApprovalsForNavigation);
3434
const approvalFlows = useSelector(getApprovalFlows, isEqual);
3535
const history = useHistory();
3636

Diff for: ui/pages/home/home.container.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ import {
4040
getSelectedInternalAccount,
4141
getQueuedRequestCount,
4242
getEditedNetwork,
43-
pendingApprovalsSortedSelector,
43+
selectPendingApprovalsForNavigation,
4444
///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
4545
getAccountType,
4646
///: END:ONLY_INCLUDE_IF
@@ -108,7 +108,7 @@ const mapStateToProps = (state) => {
108108
const totalUnapprovedAndQueuedRequestCount =
109109
totalUnapprovedCount + queuedRequestCount;
110110
const swapsEnabled = getSwapsFeatureIsLive(state);
111-
const pendingApprovals = pendingApprovalsSortedSelector(state);
111+
const pendingApprovals = selectPendingApprovalsForNavigation(state);
112112

113113
///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
114114
const institutionalConnectRequests = getInstitutionalConnectRequests(state);

Diff for: ui/selectors/approvals.ts

+45-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
1-
import { ApprovalControllerState } from '@metamask/approval-controller';
1+
import {
2+
ApprovalControllerState,
3+
ApprovalRequest,
4+
} from '@metamask/approval-controller';
25
import { ApprovalType } from '@metamask/controller-utils';
36
import { createSelector } from 'reselect';
7+
import { Json } from '@metamask/utils';
48
import { createDeepEqualSelector } from '../../shared/modules/selectors/util';
59

610
export type ApprovalsMetaMaskState = {
@@ -58,6 +62,32 @@ export function pendingApprovalsSortedSelector(state: ApprovalsMetaMaskState) {
5862
return getPendingApprovals(state).sort((a1, a2) => a1.time - a2.time);
5963
}
6064

65+
/**
66+
* Returns pending approvals sorted by time for use in confirmation navigation.
67+
* Excludes duplicate watch asset approvals as they are combined into a single confirmation.
68+
*/
69+
export const selectPendingApprovalsForNavigation = createDeepEqualSelector(
70+
pendingApprovalsSortedSelector,
71+
(sortedPendingApprovals) =>
72+
sortedPendingApprovals.filter((approval, index) => {
73+
if (
74+
isWatchNftApproval(approval) &&
75+
sortedPendingApprovals.findIndex(isWatchNftApproval) !== index
76+
) {
77+
return false;
78+
}
79+
80+
if (
81+
isWatchTokenApproval(approval) &&
82+
sortedPendingApprovals.findIndex(isWatchTokenApproval) !== index
83+
) {
84+
return false;
85+
}
86+
87+
return true;
88+
}),
89+
);
90+
6191
const internalSelectPendingApproval = createSelector(
6292
getPendingApprovals,
6393
(_state: ApprovalsMetaMaskState, id: string) => id,
@@ -68,3 +98,17 @@ export const selectPendingApproval = createDeepEqualSelector(
6898
internalSelectPendingApproval,
6999
(approval) => approval,
70100
);
101+
102+
function isWatchTokenApproval(approval: ApprovalRequest<Record<string, Json>>) {
103+
const tokenId = (approval.requestData?.asset as Record<string, string>)
104+
?.tokenId;
105+
106+
return approval.type === ApprovalType.WatchAsset && !tokenId;
107+
}
108+
109+
function isWatchNftApproval(approval: ApprovalRequest<Record<string, Json>>) {
110+
const tokenId = (approval.requestData?.asset as Record<string, string>)
111+
?.tokenId;
112+
113+
return approval.type === ApprovalType.WatchAsset && Boolean(tokenId);
114+
}

0 commit comments

Comments
 (0)