Skip to content

Commit

Permalink
Do not query 4byte if less than four bytes
Browse files Browse the repository at this point in the history
Skip data render if 0x.
Ignore cached data if less than four bytes.
  • Loading branch information
matthewwalsh0 committed Sep 24, 2024
1 parent 24f9828 commit 816afb5
Show file tree
Hide file tree
Showing 17 changed files with 136 additions and 142 deletions.
24 changes: 23 additions & 1 deletion app/scripts/lib/util.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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',
Expand All @@ -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);
});
});
});
2 changes: 1 addition & 1 deletion app/scripts/lib/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,7 @@ export const getMethodDataName = async (
provider,
);

if (addKnownMethodData) {
if (methodData?.name) {
addKnownMethodData(fourBytePrefix, methodData as MethodData);
}

Expand Down
19 changes: 19 additions & 0 deletions shared/lib/four-byte.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
12 changes: 11 additions & 1 deletion shared/lib/four-byte.ts
Original file line number Diff line number Diff line change
@@ -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 = {
Expand All @@ -12,7 +15,14 @@ type FourByteResponse = {

export async function getMethodFrom4Byte(
fourBytePrefix: string,
): Promise<string> {
): Promise<string | undefined> {
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: {
Expand Down
17 changes: 17 additions & 0 deletions shared/modules/transaction.utils.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { TransactionType } from '@metamask/transaction-controller';
import { createTestProviderTools } from '../../test/stub/provider';
import {
determineTransactionType,
hasTransactionData,
isEIP1559Transaction,
isLegacyTransaction,
parseStandardTokenTransactionData,
Expand Down Expand Up @@ -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);
},
);
});
});
7 changes: 7 additions & 0 deletions shared/modules/transaction.utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -319,3 +320,9 @@ export const parseTypedDataMessage = (dataToParse: string) => {

return result;
};

export function hasTransactionData(transactionData?: Hex): boolean {
return Boolean(
transactionData?.length && transactionData?.toLowerCase?.() !== '0x',
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -28,7 +29,7 @@ const ConfirmHexData = ({ txData, dataHexComponent }) => {
return dataHexComponent;
}

if (!txParams.data || !txParams.to) {
if (!hasTransactionData(txParams.data) || !txParams.to) {
return null;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,21 +28,24 @@ describe('ConfirmHexData', () => {
expect(await findByText('Transfer')).toBeInTheDocument();
});

it('should return null if transaction has no data', () => {
const { container } = renderWithProvider(
<ConfirmHexData
txData={{
txParams: {
data: '0x608060405234801',
},
origin: 'https://metamask.github.io',
type: 'transfer',
}}
/>,
store,
);
expect(container.firstChild).toStrictEqual(null);
});
it.each([undefined, null, '', '0x', '0X'])(
'should return null if transaction data is %s',
(data) => {
const { container } = renderWithProvider(
<ConfirmHexData
txData={{
txParams: {
data,
},
origin: 'https://metamask.github.io',
type: 'transfer',
}}
/>,
store,
);
expect(container.firstChild).toStrictEqual(null);
},
);

it('should return null if transaction has no to address', () => {
const { container } = renderWithProvider(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,19 +47,23 @@ async function runHook(stateOptions?: Parameters<typeof buildState>[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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand All @@ -75,6 +75,6 @@ describe('useFourByte', () => {
},
);

expect(result.current).toBeUndefined();
expect(result.current).toBeNull();
});
});

This file was deleted.

This file was deleted.

Loading

0 comments on commit 816afb5

Please sign in to comment.