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 c8d67af7b1f4..e28f4d4c0c5c 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 19ccc1ac1976..35f2f42c4792 100644 --- a/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.test.ts +++ b/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.test.ts @@ -38,22 +38,26 @@ async function runHook(state: Record) { describe('useDecodedTransactionData', () => { const decodeTransactionDataMock = jest.mocked(decodeTransactionData); - it('returns undefined if no transaction data', async () => { - const result = await runHook( - getMockConfirmStateForTransaction({ - id: '123', - chainId: CHAIN_ID_MOCK, - type: TransactionType.contractInteraction, - status: TransactionStatus.unapproved, - 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 (data: string) => { + const result = await runHook( + getMockConfirmStateForTransaction({ + id: '123', + chainId: CHAIN_ID_MOCK, + type: TransactionType.contractInteraction, + status: TransactionStatus.unapproved, + 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 3515ad2503f5..b2d69df413d4 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 { useConfirmContext } from '../../../../context/confirm'; +import { hasTransactionData } from '../../../../../../../shared/modules/transaction.utils'; export function useDecodedTransactionData(): AsyncResult< DecodedTransactionDataResponse | undefined @@ -19,7 +20,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 bdff31ac33e6..5d4a023c82bb 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).toEqual({}); + 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).toEqual({}); + 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 bdff31ac33e6..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 empty object 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).toEqual({}); - }); - - 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).toEqual({}); - }); -}); 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 b8c3a44078f1..4af44b5a310d 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 @@ -29,6 +29,7 @@ import { // eslint-disable-next-line import/no-restricted-paths import { UniswapPathPool } from '../../../../../../../../app/scripts/lib/transaction/decode/uniswap'; import { useConfirmContext } from '../../../../../context/confirm'; +import { hasTransactionData } from '../../../../../../../../shared/modules/transaction.utils'; export const TransactionData = () => { const { currentConfirmation } = useConfirmContext(); @@ -42,7 +43,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 2cdb9a419aa1..4bc313f6503b 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 @@ -73,6 +73,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 = { @@ -640,7 +641,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 64346a655dfd..bdc1547b7246 100644 --- a/ui/selectors/selectors.js +++ b/ui/selectors/selectors.js @@ -107,6 +107,7 @@ import { MultichainNativeAssets } from '../../shared/constants/multichain/assets // TODO: Remove restricted import // eslint-disable-next-line import/no-restricted-paths import { BridgeFeatureFlagsKey } from '../../app/scripts/controllers/bridge/types'; +import { hasTransactionData } from '../../shared/modules/transaction.utils'; import { getAllUnapprovedTransactions, getCurrentNetworkTransactions, @@ -1164,14 +1165,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] ?? {} : {}; + + if (fourBytePrefix.length < 10) { + return null; + } + + return knownMethodData?.[fourBytePrefix] ?? null; } export function getFeatureFlags(state) {