From 816afb578ded4c960754d02e34932cf367c73ad5 Mon Sep 17 00:00:00 2001 From: Matthew Walsh Date: Tue, 24 Sep 2024 21:25:12 +0100 Subject: [PATCH] Do not query 4byte if less than four bytes Skip data render if 0x. Ignore cached data if less than four bytes. --- app/scripts/lib/util.test.js | 24 +++++- app/scripts/lib/util.ts | 2 +- shared/lib/four-byte.test.ts | 19 +++++ shared/lib/four-byte.ts | 12 ++- shared/modules/transaction.utils.test.js | 17 ++++ shared/modules/transaction.utils.ts | 7 ++ .../confirm-hexdata/confirm-hexdata.js | 3 +- .../confirm-hexdata/confirm-hexdata.test.js | 33 ++++---- .../hooks/useDecodedTransactionData.test.ts | 28 ++++--- .../info/hooks/useDecodedTransactionData.ts | 3 +- .../confirm/info/hooks/useFourByte.test.ts | 4 +- .../useKnownMethodDataInTransaction.test.ts | 80 ------------------- .../hooks/useKnownMethodDataInTransaction.ts | 21 ----- .../transaction-data/transaction-data.tsx | 3 +- .../confirm-transaction-base.component.js | 3 +- .../token-allowance/token-allowance.js | 4 +- ui/selectors/selectors.js | 15 +++- 17 files changed, 136 insertions(+), 142 deletions(-) delete mode 100644 ui/pages/confirmations/components/confirm/info/hooks/useKnownMethodDataInTransaction.test.ts delete mode 100644 ui/pages/confirmations/components/confirm/info/hooks/useKnownMethodDataInTransaction.ts diff --git a/app/scripts/lib/util.test.js b/app/scripts/lib/util.test.js index d428c3cd7ae1..7ae7e7b36a88 100644 --- a/app/scripts/lib/util.test.js +++ b/app/scripts/lib/util.test.js @@ -369,21 +369,25 @@ describe('app utils', () => { name: 'Approve Tokens', }, }; + it('return null if use4ByteResolution is not true', async () => { expect( await getMethodDataName(knownMethodData, false, '0x60806040'), ).toStrictEqual(null); }); + it('return null if prefixedData is not defined', async () => { expect( await getMethodDataName(knownMethodData, true, undefined), ).toStrictEqual(null); }); + it('return details from knownMethodData if defined', async () => { expect( await getMethodDataName(knownMethodData, true, '0x60806040'), ).toStrictEqual(knownMethodData['0x60806040']); }); + it('invoke getMethodDataAsync if details not available in knownMethodData', async () => { const DUMMY_METHOD_NAME = { name: 'Dummy Method Name', @@ -392,9 +396,10 @@ describe('app utils', () => { .spyOn(FourBiteUtils, 'getMethodDataAsync') .mockResolvedValue(DUMMY_METHOD_NAME); expect( - await getMethodDataName(knownMethodData, true, '0x123'), + await getMethodDataName(knownMethodData, true, '0x123', jest.fn()), ).toStrictEqual(DUMMY_METHOD_NAME); }); + it('invoke addKnownMethodData if details not available in knownMethodData', async () => { const DUMMY_METHOD_NAME = { name: 'Dummy Method Name', @@ -413,5 +418,22 @@ describe('app utils', () => { ).toStrictEqual(DUMMY_METHOD_NAME); expect(addKnownMethodData).toHaveBeenCalledTimes(1); }); + + it('does not invoke addKnownMethodData if no method data available', async () => { + const addKnownMethodData = jest.fn(); + + jest.spyOn(FourBiteUtils, 'getMethodDataAsync').mockResolvedValue({}); + + expect( + await getMethodDataName( + knownMethodData, + true, + '0x123', + addKnownMethodData, + ), + ).toStrictEqual({}); + + expect(addKnownMethodData).toHaveBeenCalledTimes(0); + }); }); }); diff --git a/app/scripts/lib/util.ts b/app/scripts/lib/util.ts index 2caade79b83e..e41b2a00b670 100644 --- a/app/scripts/lib/util.ts +++ b/app/scripts/lib/util.ts @@ -411,7 +411,7 @@ export const getMethodDataName = async ( provider, ); - if (addKnownMethodData) { + if (methodData?.name) { addKnownMethodData(fourBytePrefix, methodData as MethodData); } diff --git a/shared/lib/four-byte.test.ts b/shared/lib/four-byte.test.ts index e25235e6ae54..2867aa2e51b7 100644 --- a/shared/lib/four-byte.test.ts +++ b/shared/lib/four-byte.test.ts @@ -25,6 +25,25 @@ describe('Four Byte', () => { expect(result).toStrictEqual('someOtherFunction(address,uint256)'); }); + + // @ts-expect-error This is missing from the Mocha type definitions + it.each([undefined, null, '', '0x', '0X'])( + 'returns undefined if four byte prefix is %s', + async (prefix: string) => { + expect(await getMethodFrom4Byte(prefix)).toBeUndefined(); + }, + ); + + // @ts-expect-error This is missing from the Mocha type definitions + it.each([ + ['with hex prefix', '0x1234567'], + ['without hex prefix', '1234567'], + ])( + 'returns undefined if length of four byte prefix %s is less than 8', + async (_: string, prefix: string) => { + expect(await getMethodFrom4Byte(prefix)).toBeUndefined(); + }, + ); }); describe('getMethodDataAsync', () => { diff --git a/shared/lib/four-byte.ts b/shared/lib/four-byte.ts index 167dfc563b73..05c91af9c768 100644 --- a/shared/lib/four-byte.ts +++ b/shared/lib/four-byte.ts @@ -1,4 +1,7 @@ import { MethodRegistry } from 'eth-method-registry'; +import { Hex } from '@metamask/utils'; +import { hasTransactionData } from '../modules/transaction.utils'; +import { stripHexPrefix } from '../modules/hexstring-utils'; import fetchWithCache from './fetch-with-cache'; type FourByteResult = { @@ -12,7 +15,14 @@ type FourByteResponse = { export async function getMethodFrom4Byte( fourBytePrefix: string, -): Promise { +): Promise { + if ( + !hasTransactionData(fourBytePrefix as Hex) || + stripHexPrefix(fourBytePrefix)?.length < 8 + ) { + return undefined; + } + const fourByteResponse = (await fetchWithCache({ url: `https://www.4byte.directory/api/v1/signatures/?hex_signature=${fourBytePrefix}`, fetchOptions: { diff --git a/shared/modules/transaction.utils.test.js b/shared/modules/transaction.utils.test.js index 133f2de9141e..28bfaaa34ed7 100644 --- a/shared/modules/transaction.utils.test.js +++ b/shared/modules/transaction.utils.test.js @@ -4,6 +4,7 @@ import { TransactionType } from '@metamask/transaction-controller'; import { createTestProviderTools } from '../../test/stub/provider'; import { determineTransactionType, + hasTransactionData, isEIP1559Transaction, isLegacyTransaction, parseStandardTokenTransactionData, @@ -417,4 +418,20 @@ describe('Transaction.utils', function () { }); }); }); + + describe('hasTransactionData', () => { + it.each([ + ['has prefix', '0x1234'], + ['has no prefix', '1234'], + ])('returns true if data %s', (_, data) => { + expect(hasTransactionData(data)).toBe(true); + }); + + it.each([undefined, null, '', '0x', '0X'])( + 'returns false if data is %s', + (data) => { + expect(hasTransactionData(data)).toBe(false); + }, + ); + }); }); diff --git a/shared/modules/transaction.utils.ts b/shared/modules/transaction.utils.ts index e09680c6c4bd..910fe9b6d4be 100644 --- a/shared/modules/transaction.utils.ts +++ b/shared/modules/transaction.utils.ts @@ -14,6 +14,7 @@ import { } from '@metamask/transaction-controller'; import type { TransactionParams } from '@metamask/transaction-controller'; +import { Hex } from '@metamask/utils'; import { AssetType, TokenStandard } from '../constants/transaction'; import { readAddressAsContract } from './contract-utils'; import { isEqualCaseInsensitive } from './string-utils'; @@ -319,3 +320,9 @@ export const parseTypedDataMessage = (dataToParse: string) => { return result; }; + +export function hasTransactionData(transactionData?: Hex): boolean { + return Boolean( + transactionData?.length && transactionData?.toLowerCase?.() !== '0x', + ); +} diff --git a/ui/pages/confirmations/components/confirm-hexdata/confirm-hexdata.js b/ui/pages/confirmations/components/confirm-hexdata/confirm-hexdata.js index 1309d7e7e618..8108cac67a0b 100644 --- a/ui/pages/confirmations/components/confirm-hexdata/confirm-hexdata.js +++ b/ui/pages/confirmations/components/confirm-hexdata/confirm-hexdata.js @@ -15,6 +15,7 @@ import { import Box from '../../../../components/ui/box'; import { Text } from '../../../../components/component-library'; import CopyRawData from '../transaction-decoding/components/ui/copy-raw-data'; +import { hasTransactionData } from '../../../../../shared/modules/transaction.utils'; const ConfirmHexData = ({ txData, dataHexComponent }) => { const t = useI18nContext(); @@ -28,7 +29,7 @@ const ConfirmHexData = ({ txData, dataHexComponent }) => { return dataHexComponent; } - if (!txParams.data || !txParams.to) { + if (!hasTransactionData(txParams.data) || !txParams.to) { return null; } diff --git a/ui/pages/confirmations/components/confirm-hexdata/confirm-hexdata.test.js b/ui/pages/confirmations/components/confirm-hexdata/confirm-hexdata.test.js index f9f41566c4c0..d31478775ba2 100644 --- a/ui/pages/confirmations/components/confirm-hexdata/confirm-hexdata.test.js +++ b/ui/pages/confirmations/components/confirm-hexdata/confirm-hexdata.test.js @@ -28,21 +28,24 @@ describe('ConfirmHexData', () => { expect(await findByText('Transfer')).toBeInTheDocument(); }); - it('should return null if transaction has no data', () => { - const { container } = renderWithProvider( - , - store, - ); - expect(container.firstChild).toStrictEqual(null); - }); + it.each([undefined, null, '', '0x', '0X'])( + 'should return null if transaction data is %s', + (data) => { + const { container } = renderWithProvider( + , + store, + ); + expect(container.firstChild).toStrictEqual(null); + }, + ); it('should return null if transaction has no to address', () => { const { container } = renderWithProvider( 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 5c185d98c282..c070e1ae0635 100644 --- a/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.test.ts +++ b/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.test.ts @@ -47,19 +47,23 @@ async function runHook(stateOptions?: Parameters[0]) { describe('useDecodedTransactionData', () => { const decodeTransactionDataMock = jest.mocked(decodeTransactionData); - it('returns undefined if no transaction data', async () => { - const result = await runHook({ - currentConfirmation: { - chainId: CHAIN_ID_MOCK, - txParams: { - data: '', - to: CONTRACT_ADDRESS_MOCK, - } as TransactionParams, - }, - }); + // @ts-expect-error This is missing from the Mocha type definitions + it.each([undefined, null, '', '0x', '0X'])( + 'returns undefined if transaction data is %s', + async () => { + const result = await runHook({ + currentConfirmation: { + chainId: CHAIN_ID_MOCK, + txParams: { + data: '', + to: CONTRACT_ADDRESS_MOCK, + } as TransactionParams, + }, + }); - expect(result).toStrictEqual({ pending: false, value: undefined }); - }); + 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 56b5ec0cb283..23c8c7af3f61 100644 --- a/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.ts +++ b/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.ts @@ -8,6 +8,7 @@ import { import { decodeTransactionData } from '../../../../../../store/actions'; import { DecodedTransactionDataResponse } from '../../../../../../../shared/types/transaction-decode'; import { currentConfirmationSelector } from '../../../../selectors'; +import { hasTransactionData } from '../../../../../../../shared/modules/transaction.utils'; export function useDecodedTransactionData(): AsyncResult< DecodedTransactionDataResponse | undefined @@ -21,7 +22,7 @@ export function useDecodedTransactionData(): AsyncResult< const transactionData = currentConfirmation?.txParams?.data as Hex; return useAsyncResult(async () => { - if (!transactionData?.length) { + if (!hasTransactionData(transactionData)) { return undefined; } diff --git a/ui/pages/confirmations/components/confirm/info/hooks/useFourByte.test.ts b/ui/pages/confirmations/components/confirm/info/hooks/useFourByte.test.ts index 1c3d66570d8b..146cae0771ec 100644 --- a/ui/pages/confirmations/components/confirm/info/hooks/useFourByte.test.ts +++ b/ui/pages/confirmations/components/confirm/info/hooks/useFourByte.test.ts @@ -54,7 +54,7 @@ describe('useFourByte', () => { }, ); - expect(result.current).toBeUndefined(); + expect(result.current).toBeNull(); }); it("returns undefined if it's not known even if resolution is enabled", () => { @@ -75,6 +75,6 @@ describe('useFourByte', () => { }, ); - expect(result.current).toBeUndefined(); + expect(result.current).toBeNull(); }); }); diff --git a/ui/pages/confirmations/components/confirm/info/hooks/useKnownMethodDataInTransaction.test.ts b/ui/pages/confirmations/components/confirm/info/hooks/useKnownMethodDataInTransaction.test.ts deleted file mode 100644 index 1c3d66570d8b..000000000000 --- a/ui/pages/confirmations/components/confirm/info/hooks/useKnownMethodDataInTransaction.test.ts +++ /dev/null @@ -1,80 +0,0 @@ -import { TransactionMeta } from '@metamask/transaction-controller'; -import { - CONTRACT_INTERACTION_SENDER_ADDRESS, - genUnapprovedContractInteractionConfirmation, -} from '../../../../../../../test/data/confirmations/contract-interaction'; -import mockState from '../../../../../../../test/data/mock-state.json'; -import { renderHookWithProvider } from '../../../../../../../test/lib/render-helpers'; -import { useFourByte } from './useFourByte'; - -describe('useFourByte', () => { - const depositHexData = '0xd0e30db0'; - - it('returns the method name and params', () => { - const currentConfirmation = genUnapprovedContractInteractionConfirmation({ - address: CONTRACT_INTERACTION_SENDER_ADDRESS, - txData: depositHexData, - }) as TransactionMeta; - - const { result } = renderHookWithProvider( - () => useFourByte(currentConfirmation), - { - ...mockState, - metamask: { - ...mockState.metamask, - use4ByteResolution: true, - knownMethodData: { - [depositHexData]: { name: 'Deposit', params: [] }, - }, - }, - }, - ); - - expect(result.current.name).toEqual('Deposit'); - expect(result.current.params).toEqual([]); - }); - - it('returns undefined if resolution is turned off', () => { - const currentConfirmation = genUnapprovedContractInteractionConfirmation({ - address: CONTRACT_INTERACTION_SENDER_ADDRESS, - txData: depositHexData, - }) as TransactionMeta; - - const { result } = renderHookWithProvider( - () => useFourByte(currentConfirmation), - { - ...mockState, - metamask: { - ...mockState.metamask, - use4ByteResolution: false, - knownMethodData: { - [depositHexData]: { name: 'Deposit', params: [] }, - }, - }, - }, - ); - - expect(result.current).toBeUndefined(); - }); - - it("returns undefined if it's not known even if resolution is enabled", () => { - const currentConfirmation = genUnapprovedContractInteractionConfirmation({ - address: CONTRACT_INTERACTION_SENDER_ADDRESS, - txData: depositHexData, - }) as TransactionMeta; - - const { result } = renderHookWithProvider( - () => useFourByte(currentConfirmation), - { - ...mockState, - metamask: { - ...mockState.metamask, - use4ByteResolution: true, - knownMethodData: {}, - }, - }, - ); - - expect(result.current).toBeUndefined(); - }); -}); diff --git a/ui/pages/confirmations/components/confirm/info/hooks/useKnownMethodDataInTransaction.ts b/ui/pages/confirmations/components/confirm/info/hooks/useKnownMethodDataInTransaction.ts deleted file mode 100644 index 821fbeaa9b5a..000000000000 --- a/ui/pages/confirmations/components/confirm/info/hooks/useKnownMethodDataInTransaction.ts +++ /dev/null @@ -1,21 +0,0 @@ -import { TransactionMeta } from '@metamask/transaction-controller'; -import { useDispatch, useSelector } from 'react-redux'; -import { - getKnownMethodData, - use4ByteResolutionSelector, -} from '../../../../../../selectors'; -import { getContractMethodData } from '../../../../../../store/actions'; - -export const useKnownMethodDataInTransaction = ( - currentConfirmation: TransactionMeta, -) => { - const dispatch = useDispatch(); - const use4ByteResolution = useSelector(use4ByteResolutionSelector); - const transactionData = currentConfirmation?.txParams?.data; - if (use4ByteResolution && transactionData) { - dispatch(getContractMethodData(currentConfirmation.txParams.data)); - } - const knownMethodData = - useSelector((state) => getKnownMethodData(state, transactionData)) || {}; - return { knownMethodData }; -}; diff --git a/ui/pages/confirmations/components/confirm/info/shared/transaction-data/transaction-data.tsx b/ui/pages/confirmations/components/confirm/info/shared/transaction-data/transaction-data.tsx index cc0eb454ba4b..6ad3899d7aaf 100644 --- a/ui/pages/confirmations/components/confirm/info/shared/transaction-data/transaction-data.tsx +++ b/ui/pages/confirmations/components/confirm/info/shared/transaction-data/transaction-data.tsx @@ -28,6 +28,7 @@ import { DecodedTransactionDataSource, } from '../../../../../../../../shared/types/transaction-decode'; import { UniswapPathPool } from '../../../../../../../../app/scripts/lib/transaction/decode/uniswap'; +import { hasTransactionData } from '../../../../../../../../shared/modules/transaction.utils'; export const TransactionData = () => { const currentConfirmation = useSelector(currentConfirmationSelector) as @@ -43,7 +44,7 @@ export const TransactionData = () => { return ; } - if (!transactionData?.length) { + if (!hasTransactionData(transactionData)) { return null; } diff --git a/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.component.js b/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.component.js index 8caf1043cf4a..5330a3685d47 100644 --- a/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.component.js +++ b/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.component.js @@ -74,6 +74,7 @@ import FeeDetailsComponent from '../components/fee-details-component/fee-details import { SimulationDetails } from '../components/simulation-details'; import { fetchSwapsFeatureFlags } from '../../swaps/swaps.util'; import { NetworkChangeToastLegacy } from '../components/confirm/network-change-toast'; +import { hasTransactionData } from '../../../../shared/modules/transaction.utils'; export default class ConfirmTransactionBase extends Component { static contextTypes = { @@ -630,7 +631,7 @@ export default class ConfirmTransactionBase extends Component { const { txParams: { data }, } = txData; - if (!data) { + if (!hasTransactionData(data)) { return null; } return ( diff --git a/ui/pages/confirmations/token-allowance/token-allowance.js b/ui/pages/confirmations/token-allowance/token-allowance.js index 123cd1354cc6..8ccd19ecb566 100644 --- a/ui/pages/confirmations/token-allowance/token-allowance.js +++ b/ui/pages/confirmations/token-allowance/token-allowance.js @@ -212,7 +212,9 @@ export default function TokenAllowance({ } const fee = useSelector((state) => transactionFeeSelector(state, fullTxData)); - const methodData = useSelector((state) => getKnownMethodData(state, data)); + const methodData = useSelector( + (state) => getKnownMethodData(state, data) ?? {}, + ); const { balanceError } = useGasFeeContext(); diff --git a/ui/selectors/selectors.js b/ui/selectors/selectors.js index 54757979bd44..ad40eeb4e3f2 100644 --- a/ui/selectors/selectors.js +++ b/ui/selectors/selectors.js @@ -107,6 +107,7 @@ import { PRIVACY_POLICY_DATE } from '../helpers/constants/privacy-policy'; import { ENVIRONMENT_TYPE_POPUP } from '../../shared/constants/app'; import { MultichainNativeAssets } from '../../shared/constants/multichain/assets'; import { BridgeFeatureFlagsKey } from '../../app/scripts/controllers/bridge/types'; +import { hasTransactionData } from '../../shared/modules/transaction.utils'; import { getAllUnapprovedTransactions, getCurrentNetworkTransactions, @@ -1239,14 +1240,20 @@ export function getRpcPrefsForCurrentProvider(state) { } export function getKnownMethodData(state, data) { - if (!data) { + const { knownMethodData, use4ByteResolution } = state.metamask; + + if (!use4ByteResolution || !hasTransactionData(data)) { return null; } + const prefixedData = addHexPrefix(data); const fourBytePrefix = prefixedData.slice(0, 10); - const { knownMethodData, use4ByteResolution } = state.metamask; - // If 4byte setting is off, we do not want to return the knownMethodData - return use4ByteResolution ? knownMethodData?.[fourBytePrefix] : undefined; + + if (fourBytePrefix.length < 10) { + return null; + } + + return knownMethodData?.[fourBytePrefix] ?? null; } export function getFeatureFlags(state) {