From 1712e287c0df901a475d8c4c4cbf6c20d1c30e28 Mon Sep 17 00:00:00 2001 From: Matthew Walsh Date: Fri, 20 Dec 2024 17:25:52 +0000 Subject: [PATCH] fix (cherry-pick): remove reliance on transaction decode in confirmations #29341 (#29397) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Cherry-pick of #29341 for release 12.9.3. Difference to `main` is that redesigned transactions are disabled if the `Decode smart contracts` toggle is disabled. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29397?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. --- shared/lib/confirmation.utils.test.ts | 22 ++++++++++++++++ shared/lib/confirmation.utils.ts | 6 +++++ .../hooks/useDecodedTransactionData.test.ts | 26 +++++++++++++++++++ .../info/hooks/useDecodedTransactionData.ts | 12 ++++++++- .../hooks/useCurrentConfirmation.test.ts | 16 ++++++++++++ .../hooks/useCurrentConfirmation.ts | 3 +++ 6 files changed, 84 insertions(+), 1 deletion(-) diff --git a/shared/lib/confirmation.utils.test.ts b/shared/lib/confirmation.utils.test.ts index 552d78827a2e..add03a95813d 100644 --- a/shared/lib/confirmation.utils.test.ts +++ b/shared/lib/confirmation.utils.test.ts @@ -43,6 +43,17 @@ describe('confirmation.utils', () => { }), ).toBe(false); }); + + it('should return false if decoding is disabled', () => { + expect( + shouldUseRedesignForTransactions({ + transactionMetadataType: unsupportedTransactionType, + isRedesignedTransactionsUserSettingEnabled: true, // user setting enabled + isRedesignedConfirmationsDeveloperEnabled: false, // developer mode disabled + isDecodingEnabled: false, + }), + ).toBe(false); + }); }); describe('when developer mode is enabled', () => { @@ -93,6 +104,17 @@ describe('confirmation.utils', () => { }), ).toBe(false); }); + + it('should return false if decoding is disabled', () => { + expect( + shouldUseRedesignForTransactions({ + transactionMetadataType: unsupportedTransactionType, + isRedesignedTransactionsUserSettingEnabled: false, // user setting disabled + isRedesignedConfirmationsDeveloperEnabled: true, // developer mode enabled + isDecodingEnabled: false, + }), + ).toBe(false); + }); }); describe('when both user setting and developer mode are disabled', () => { diff --git a/shared/lib/confirmation.utils.ts b/shared/lib/confirmation.utils.ts index 24c5f258a5d0..1813211582be 100644 --- a/shared/lib/confirmation.utils.ts +++ b/shared/lib/confirmation.utils.ts @@ -37,11 +37,17 @@ export function shouldUseRedesignForTransactions({ transactionMetadataType, isRedesignedTransactionsUserSettingEnabled, isRedesignedConfirmationsDeveloperEnabled, + isDecodingEnabled, }: { transactionMetadataType?: TransactionType; isRedesignedTransactionsUserSettingEnabled: boolean; isRedesignedConfirmationsDeveloperEnabled: boolean; + isDecodingEnabled?: boolean; }): boolean { + if (isDecodingEnabled === false) { + return false; + } + return ( shouldUseRedesignForTransactionsUserMode( isRedesignedTransactionsUserSettingEnabled, diff --git a/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.test.ts b/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.test.ts index 32a711abf754..c12ce7025cb5 100644 --- a/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.test.ts +++ b/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.test.ts @@ -76,6 +76,32 @@ describe('useDecodedTransactionData', () => { expect(result).toStrictEqual({ pending: false, value: undefined }); }); + it('returns undefined if decode disabled', async () => { + decodeTransactionDataMock.mockResolvedValue(TRANSACTION_DECODE_SOURCIFY); + + const result = await runHook( + getMockConfirmStateForTransaction( + { + id: '123', + chainId: CHAIN_ID_MOCK, + type: TransactionType.contractInteraction, + status: TransactionStatus.unapproved, + txParams: { + data: TRANSACTION_DATA_UNISWAP, + to: CONTRACT_ADDRESS_MOCK, + } as TransactionParams, + }, + { + metamask: { + use4ByteResolution: false, + }, + }, + ), + ); + + expect(result).toStrictEqual({ pending: false, value: undefined }); + }); + it('returns the decoded data', async () => { decodeTransactionDataMock.mockResolvedValue(TRANSACTION_DECODE_SOURCIFY); diff --git a/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.ts b/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.ts index 5276e02eaad1..3486f16ed864 100644 --- a/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.ts +++ b/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.ts @@ -1,6 +1,7 @@ import { Hex } from '@metamask/utils'; import { TransactionMeta } from '@metamask/transaction-controller'; +import { useSelector } from 'react-redux'; import { AsyncResult, useAsyncResult, @@ -9,11 +10,13 @@ import { decodeTransactionData } from '../../../../../../store/actions'; import { DecodedTransactionDataResponse } from '../../../../../../../shared/types/transaction-decode'; import { useConfirmContext } from '../../../../context/confirm'; import { hasTransactionData } from '../../../../../../../shared/modules/transaction.utils'; +import { use4ByteResolutionSelector } from '../../../../../../selectors'; export function useDecodedTransactionData( transactionTypeFilter?: string, ): AsyncResult { const { currentConfirmation } = useConfirmContext(); + const isDecodeEnabled = useSelector(use4ByteResolutionSelector); const currentTransactionType = currentConfirmation?.type; const chainId = currentConfirmation?.chainId as Hex; @@ -23,6 +26,7 @@ export function useDecodedTransactionData( return useAsyncResult(async () => { if ( + !isDecodeEnabled || !hasTransactionData(transactionData) || !transactionTo || (transactionTypeFilter && @@ -36,5 +40,11 @@ export function useDecodedTransactionData( chainId, contractAddress, }); - }, [transactionData, transactionTo, chainId, contractAddress]); + }, [ + isDecodeEnabled, + transactionData, + transactionTo, + chainId, + contractAddress, + ]); } diff --git a/ui/pages/confirmations/hooks/useCurrentConfirmation.test.ts b/ui/pages/confirmations/hooks/useCurrentConfirmation.test.ts index 8c66873d448c..efd26d14da39 100644 --- a/ui/pages/confirmations/hooks/useCurrentConfirmation.test.ts +++ b/ui/pages/confirmations/hooks/useCurrentConfirmation.test.ts @@ -59,6 +59,7 @@ function buildState({ redesignedTransactionsEnabled, transaction, isRedesignedConfirmationsDeveloperEnabled, + isDecodingEnabled, }: { // eslint-disable-next-line @typescript-eslint/no-explicit-any message?: Partial; @@ -67,6 +68,7 @@ function buildState({ redesignedTransactionsEnabled?: boolean; transaction?: Partial; isRedesignedConfirmationsDeveloperEnabled?: boolean; + isDecodingEnabled?: boolean; }) { return { ...mockState, @@ -83,6 +85,7 @@ function buildState({ unapprovedPersonalMsgs: message ? { [message.id as string]: message } : {}, + use4ByteResolution: isDecodingEnabled ?? true, }, }; } @@ -290,6 +293,19 @@ describe('useCurrentConfirmation', () => { expect(currentConfirmation).toStrictEqual(TRANSACTION_MOCK); }); + it('returns undefined if transaction type correct and redesign enabled but decoding disabled', () => { + const currentConfirmation = runHook({ + pendingApprovals: [{ ...APPROVAL_MOCK, type: ApprovalType.Transaction }], + redesignedConfirmationsEnabled: true, + transaction: TRANSACTION_MOCK, + redesignedTransactionsEnabled: true, + isRedesignedConfirmationsDeveloperEnabled: false, + isDecodingEnabled: false, + }); + + expect(currentConfirmation).toBeUndefined(); + }); + describe('useCurrentConfirmation with env var', () => { beforeAll(() => { jest.resetModules(); diff --git a/ui/pages/confirmations/hooks/useCurrentConfirmation.ts b/ui/pages/confirmations/hooks/useCurrentConfirmation.ts index 1771f807de25..b2ccc5fd4594 100644 --- a/ui/pages/confirmations/hooks/useCurrentConfirmation.ts +++ b/ui/pages/confirmations/hooks/useCurrentConfirmation.ts @@ -11,6 +11,7 @@ import { getUnapprovedTransaction, oldestPendingConfirmationSelector, selectPendingApproval, + use4ByteResolutionSelector, } from '../../../selectors'; import { selectUnapprovedMessage } from '../../../selectors/signatures'; import { @@ -30,6 +31,7 @@ const useCurrentConfirmation = () => { const { id: paramsConfirmationId } = useParams<{ id: string }>(); const oldestPendingApproval = useSelector(oldestPendingConfirmationSelector); const confirmationId = paramsConfirmationId ?? oldestPendingApproval?.id; + const isDecodingEnabled = Boolean(useSelector(use4ByteResolutionSelector)); const isRedesignedSignaturesUserSettingEnabled = useSelector( getRedesignedConfirmationsEnabled, @@ -66,6 +68,7 @@ const useCurrentConfirmation = () => { transactionMetadataType: transactionMetadata?.type, isRedesignedTransactionsUserSettingEnabled, isRedesignedConfirmationsDeveloperEnabled, + isDecodingEnabled, }); const shouldUseRedesign =