From 9b8b5bf452acac56569e668bff4924b787c6b25a Mon Sep 17 00:00:00 2001 From: Derek Brans Date: Wed, 9 Oct 2024 12:02:12 -0400 Subject: [PATCH 1/6] tests passing --- .../src/utils/simulation.test.ts | 123 ++++++++++++++---- .../src/utils/simulation.ts | 48 ++++--- 2 files changed, 130 insertions(+), 41 deletions(-) diff --git a/packages/transaction-controller/src/utils/simulation.test.ts b/packages/transaction-controller/src/utils/simulation.test.ts index b07c3a75ea..095115cd14 100644 --- a/packages/transaction-controller/src/utils/simulation.test.ts +++ b/packages/transaction-controller/src/utils/simulation.test.ts @@ -1,13 +1,16 @@ import type { LogDescription } from '@ethersproject/abi'; import { Interface } from '@ethersproject/abi'; +import { isHexString, type Hex } from '@metamask/utils'; import { + SimulationError, SimulationInvalidResponseError, SimulationRevertedError, } from '../errors'; import { SimulationErrorCode, SimulationTokenStandard } from '../types'; import { getSimulationData, + getValueFromBalanceTransaction, SupportedToken, type GetSimulationDataRequest, } from './simulation'; @@ -19,14 +22,34 @@ import { jest.mock('./simulation-api'); -const USER_ADDRESS_MOCK = '0x123'; -const OTHER_ADDRESS_MOCK = '0x456'; -const CONTRACT_ADDRESS_1_MOCK = '0x789'; -const CONTRACT_ADDRESS_2_MOCK = '0xDEF'; -const BALANCE_1_MOCK = '0x0'; -const BALANCE_2_MOCK = '0x1'; +// Utility function to encode uint256 values to 32-byte ABI format +const encodeUint256 = (value: string | number): Hex => { + // Pad to 32 bytes (64 characters) and add '0x' prefix + return `0x${BigInt(value).toString(16).padStart(64, '0')}` as Hex; +}; + +// Utility function to encode addresses to correct length +const encodeAddress = (address: Hex): Hex => { + // Ensure the address is a valid hex string and pad it to 20 bytes (40 characters) + if (!isHexString(address)) { + throw new Error('Invalid address format'); + } + return `0x${address.toLowerCase().substring(2).padStart(40, '0')}` as Hex; +}; + +const trimLeadingZeros = (hexString: Hex): Hex => { + const trimmed = hexString.replace(/^0x0+/u, '0x') as Hex; + return trimmed === '0x' ? '0x0' : trimmed; +}; + +const USER_ADDRESS_MOCK = encodeAddress('0x123'); +const OTHER_ADDRESS_MOCK = encodeAddress('0x456'); +const CONTRACT_ADDRESS_1_MOCK = encodeAddress('0x789'); +const CONTRACT_ADDRESS_2_MOCK = encodeAddress('0xDEF'); +const BALANCE_1_MOCK = encodeUint256('0x0'); +const BALANCE_2_MOCK = encodeUint256('0x1'); const DIFFERENCE_MOCK = '0x1'; -const VALUE_MOCK = '0x4'; +const VALUE_MOCK = encodeUint256('0x4'); const TOKEN_ID_MOCK = '0x5'; const OTHER_TOKEN_ID_MOCK = '0x6'; const ERROR_CODE_MOCK = 123; @@ -160,6 +183,7 @@ function createNativeBalanceResponse( return { transactions: [ { + return: encodeUint256(previousBalance), callTrace: { calls: [], logs: [], @@ -167,12 +191,12 @@ function createNativeBalanceResponse( stateDiff: { pre: { [USER_ADDRESS_MOCK]: { - balance: previousBalance, + balance: encodeUint256(previousBalance), }, }, post: { - [USER_ADDRESS_MOCK]: { - balance: newBalance, + [encodeAddress(USER_ADDRESS_MOCK)]: { + balance: encodeUint256(newBalance), }, }, }, @@ -194,7 +218,7 @@ function createBalanceOfResponse( return { transactions: [ ...previousBalances.map((previousBalance) => ({ - return: previousBalance, + return: encodeUint256(previousBalance), callTrace: { calls: [], logs: [], @@ -205,7 +229,7 @@ function createBalanceOfResponse( }, })), { - return: '0xabc', + return: encodeUint256('0xabc'), // Example correction with encoding callTrace: { calls: [], logs: [], @@ -216,7 +240,7 @@ function createBalanceOfResponse( }, }, ...newBalances.map((newBalance) => ({ - return: newBalance, + return: encodeUint256(newBalance), callTrace: { calls: [], logs: [], @@ -398,8 +422,8 @@ describe('Simulation Utils', () => { standard: tokenStandard, address: CONTRACT_ADDRESS_1_MOCK, id: tokenId, - previousBalance: BALANCE_1_MOCK, - newBalance: BALANCE_2_MOCK, + previousBalance: trimLeadingZeros(BALANCE_1_MOCK), + newBalance: trimLeadingZeros(BALANCE_2_MOCK), difference: DIFFERENCE_MOCK, isDecrease: false, }, @@ -501,8 +525,8 @@ describe('Simulation Utils', () => { standard: SimulationTokenStandard.erc20, address: CONTRACT_ADDRESS_1_MOCK, id: undefined, - previousBalance: BALANCE_2_MOCK, - newBalance: BALANCE_1_MOCK, + previousBalance: trimLeadingZeros(BALANCE_2_MOCK), + newBalance: trimLeadingZeros(BALANCE_1_MOCK), difference: DIFFERENCE_MOCK, isDecrease: true, }, @@ -545,8 +569,8 @@ describe('Simulation Utils', () => { standard: SimulationTokenStandard.erc721, address: CONTRACT_ADDRESS_1_MOCK, id: TOKEN_ID_MOCK, - previousBalance: BALANCE_1_MOCK, - newBalance: BALANCE_2_MOCK, + previousBalance: trimLeadingZeros(BALANCE_1_MOCK), + newBalance: trimLeadingZeros(BALANCE_2_MOCK), difference: DIFFERENCE_MOCK, isDecrease: false, }, @@ -554,8 +578,8 @@ describe('Simulation Utils', () => { standard: SimulationTokenStandard.erc721, address: CONTRACT_ADDRESS_1_MOCK, id: OTHER_TOKEN_ID_MOCK, - previousBalance: BALANCE_1_MOCK, - newBalance: BALANCE_2_MOCK, + previousBalance: trimLeadingZeros(BALANCE_1_MOCK), + newBalance: trimLeadingZeros(BALANCE_2_MOCK), difference: DIFFERENCE_MOCK, isDecrease: false, }, @@ -738,9 +762,9 @@ describe('Simulation Utils', () => { standard: SimulationTokenStandard.erc20, address: CONTRACT_ADDRESS_1_MOCK, id: undefined, - previousBalance: BALANCE_1_MOCK, - newBalance: BALANCE_2_MOCK, - difference: DIFFERENCE_MOCK, + previousBalance: trimLeadingZeros(BALANCE_1_MOCK), + newBalance: trimLeadingZeros(BALANCE_2_MOCK), + difference: '0x1', isDecrease: false, }, ], @@ -859,3 +883,54 @@ describe('Simulation Utils', () => { }); }); }); + +describe('getValueFromBalanceTransaction', () => { + const from = '0x1234567890123456789012345678901234567890'; + const contractAddress = '0xabcdef1234567890abcdef1234567890abcdef12' as Hex; + + it.each([ + [ + 'ERC20 balance', + SimulationTokenStandard.erc20, + '0x000000000000000000000000000000000000000000000000000000134c31d25200000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000', + '0x134c31d252', + ], + [ + 'ERC721 ownership', + SimulationTokenStandard.erc721, + '0x0000000000000000000000001234567890123456789012345678901234567890', + '0x1', + ], + [ + 'ERC721 non-ownership', + SimulationTokenStandard.erc721, + '0x000000000000000000000000abcdef1234567890abcdef1234567890abcdef12', + '0x0', + ], + [ + 'ERC1155 balance', + SimulationTokenStandard.erc1155, + '0x0000000000000000000000000000000000000000000000000000000000000064', + '0x64', + ], + ])('correctly decodes %s', (_, standard, returnValue, expected) => { + const token = { standard, address: contractAddress }; + const response = { return: returnValue as Hex }; + + const result = getValueFromBalanceTransaction(from, token, response); + + expect(result).toBe(expected); + }); + + it('throws SimulationInvalidResponseError on decoding failure', () => { + const token = { + standard: SimulationTokenStandard.erc20, + address: contractAddress, + }; + const response = { return: '0xInvalidData' as Hex }; + + expect(() => getValueFromBalanceTransaction(from, token, response)).toThrow( + SimulationError, + ); + }); +}); diff --git a/packages/transaction-controller/src/utils/simulation.ts b/packages/transaction-controller/src/utils/simulation.ts index 04ea5f66f4..cd61bac95c 100644 --- a/packages/transaction-controller/src/utils/simulation.ts +++ b/packages/transaction-controller/src/utils/simulation.ts @@ -477,24 +477,47 @@ function getEventTokenIds(event: ParsedEvent): (Hex | undefined)[] { } /** - * Extract the value from a balance transaction response. + * Extract the value from a balance transaction response using the correct ABI. * @param from - The address to check the balance of. * @param token - The token to check the balance of. * @param response - The balance transaction response. - * @returns The value of the balance transaction. + * @returns The value of the balance transaction as Hex. */ -function getValueFromBalanceTransaction( +export function getValueFromBalanceTransaction( from: Hex, token: SimulationToken, response: SimulationResponseTransaction, ): Hex { - const normalizedReturn = normalizeReturnValue(response.return); + const interfaces = getContractInterfaces(); + let decoded; - if (token.standard === SimulationTokenStandard.erc721) { - return normalizedReturn === from ? '0x1' : '0x0'; + try { + if (token.standard === SimulationTokenStandard.erc721) { + // ERC721: Check ownership and return 1 if the owner is the user, 0 otherwise. + decoded = ( + interfaces.get(SupportedToken.ERC721) as Interface + ).decodeFunctionResult('ownerOf', response.return); + const owner = decoded[0].toLowerCase(); + return owner === from.toLowerCase() ? '0x1' : '0x0'; + } + // Non-ERC721: ERC20 and ERC1155 use balanceOf + decoded = ( + interfaces.get( + token.standard === SimulationTokenStandard.erc20 + ? SupportedToken.ERC20 + : SupportedToken.ERC1155, + ) as Interface + ).decodeFunctionResult('balanceOf', response.return); + + return toHex(decoded[0] as Hex); + } catch (error) { + log('Failed to decode balance transaction', error, { token, response }); + throw new SimulationError( + `Failed to decode balance transaction for token ${ + token.address + }: ${String(error)}`, + ); } - - return normalizedReturn; } /** @@ -607,15 +630,6 @@ function getSimulationBalanceChange( }; } -/** - * Normalize a return value. - * @param value - The return value to normalize. - * @returns The normalized return value. - */ -function normalizeReturnValue(value: Hex): Hex { - return toHex(hexToBN(value)); -} - /** * Get the contract interfaces for all supported tokens. * @returns A map of supported tokens to their contract interfaces. From af65ed95db87ecbccd6cc2fdcadb79b81e9d266e Mon Sep 17 00:00:00 2001 From: Derek Brans Date: Wed, 9 Oct 2024 13:40:28 -0400 Subject: [PATCH 2/6] update TxC/jest.config.js --- packages/transaction-controller/jest.config.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/transaction-controller/jest.config.js b/packages/transaction-controller/jest.config.js index a7cd29f672..ee191ea742 100644 --- a/packages/transaction-controller/jest.config.js +++ b/packages/transaction-controller/jest.config.js @@ -18,7 +18,7 @@ module.exports = merge(baseConfig, { coverageThreshold: { global: { branches: 94.42, - functions: 97.46, + functions: 97.45, lines: 98.44, statements: 98.46, }, From 8458bf7b2cf7bc02e84145207ba215cb6f5d7edf Mon Sep 17 00:00:00 2001 From: Derek Brans Date: Fri, 11 Oct 2024 09:39:58 -0400 Subject: [PATCH 3/6] address comments --- .../transaction-controller/jest.config.js | 4 +- .../src/utils/simulation.test.ts | 221 +++++++----------- .../src/utils/simulation.ts | 63 ++--- 3 files changed, 121 insertions(+), 167 deletions(-) diff --git a/packages/transaction-controller/jest.config.js b/packages/transaction-controller/jest.config.js index ee191ea742..c618be1554 100644 --- a/packages/transaction-controller/jest.config.js +++ b/packages/transaction-controller/jest.config.js @@ -19,8 +19,8 @@ module.exports = merge(baseConfig, { global: { branches: 94.42, functions: 97.45, - lines: 98.44, - statements: 98.46, + lines: 98.37, + statements: 98.38, }, }, diff --git a/packages/transaction-controller/src/utils/simulation.test.ts b/packages/transaction-controller/src/utils/simulation.test.ts index 095115cd14..8b6975fa92 100644 --- a/packages/transaction-controller/src/utils/simulation.test.ts +++ b/packages/transaction-controller/src/utils/simulation.test.ts @@ -1,20 +1,21 @@ import type { LogDescription } from '@ethersproject/abi'; import { Interface } from '@ethersproject/abi'; -import { isHexString, type Hex } from '@metamask/utils'; +import { type Hex } from '@metamask/utils'; import { - SimulationError, SimulationInvalidResponseError, SimulationRevertedError, } from '../errors'; import { SimulationErrorCode, SimulationTokenStandard } from '../types'; import { getSimulationData, - getValueFromBalanceTransaction, SupportedToken, type GetSimulationDataRequest, } from './simulation'; -import type { SimulationResponseLog } from './simulation-api'; +import type { + SimulationResponseLog, + SimulationResponseTransaction, +} from './simulation-api'; import { simulateTransactions, type SimulationResponse, @@ -22,19 +23,10 @@ import { jest.mock('./simulation-api'); -// Utility function to encode uint256 values to 32-byte ABI format -const encodeUint256 = (value: string | number): Hex => { +// Utility function to encode addresses and values to 32-byte ABI format +const encodeTo32ByteHex = (value: string | number): Hex => { // Pad to 32 bytes (64 characters) and add '0x' prefix - return `0x${BigInt(value).toString(16).padStart(64, '0')}` as Hex; -}; - -// Utility function to encode addresses to correct length -const encodeAddress = (address: Hex): Hex => { - // Ensure the address is a valid hex string and pad it to 20 bytes (40 characters) - if (!isHexString(address)) { - throw new Error('Invalid address format'); - } - return `0x${address.toLowerCase().substring(2).padStart(40, '0')}` as Hex; + return `0x${BigInt(value).toString(16).padStart(64, '0')}`; }; const trimLeadingZeros = (hexString: Hex): Hex => { @@ -42,14 +34,14 @@ const trimLeadingZeros = (hexString: Hex): Hex => { return trimmed === '0x' ? '0x0' : trimmed; }; -const USER_ADDRESS_MOCK = encodeAddress('0x123'); -const OTHER_ADDRESS_MOCK = encodeAddress('0x456'); -const CONTRACT_ADDRESS_1_MOCK = encodeAddress('0x789'); -const CONTRACT_ADDRESS_2_MOCK = encodeAddress('0xDEF'); -const BALANCE_1_MOCK = encodeUint256('0x0'); -const BALANCE_2_MOCK = encodeUint256('0x1'); +const USER_ADDRESS_MOCK = '0x123'; +const OTHER_ADDRESS_MOCK = '0x456'; +const CONTRACT_ADDRESS_1_MOCK = '0x789'; +const CONTRACT_ADDRESS_2_MOCK = '0xDEF'; +const BALANCE_1_MOCK = '0x0'; +const BALANCE_2_MOCK = '0x1'; const DIFFERENCE_MOCK = '0x1'; -const VALUE_MOCK = encodeUint256('0x4'); +const VALUE_MOCK = '0x4'; const TOKEN_ID_MOCK = '0x5'; const OTHER_TOKEN_ID_MOCK = '0x6'; const ERROR_CODE_MOCK = 123; @@ -110,10 +102,16 @@ const PARSED_WRAPPED_ERC20_DEPOSIT_EVENT_MOCK = { args: [USER_ADDRESS_MOCK, { toHexString: () => VALUE_MOCK }], } as unknown as LogDescription; +const defaultResponseTx: SimulationResponseTransaction = { + return: encodeTo32ByteHex('0x0'), + callTrace: { calls: [], logs: [] }, + stateDiff: { pre: {}, post: {} }, +}; + const RESPONSE_NESTED_LOGS_MOCK: SimulationResponse = { transactions: [ { - return: '0x1', + ...defaultResponseTx, callTrace: { calls: [ { @@ -128,10 +126,6 @@ const RESPONSE_NESTED_LOGS_MOCK: SimulationResponse = { ], logs: [], }, - stateDiff: { - pre: {}, - post: {}, - }, }, ], }; @@ -152,22 +146,12 @@ function createLogMock(contractAddress: string) { * @param logs - The logs. * @returns Mock API response. */ -function createEventResponseMock(logs: SimulationResponseLog[]) { +function createEventResponseMock( + logs: SimulationResponseLog[], +): SimulationResponse { return { - transactions: [ - { - return: '0x', - callTrace: { - calls: [], - logs, - }, - stateDiff: { - pre: {}, - post: {}, - }, - }, - ], - } as unknown as SimulationResponse; + transactions: [{ ...defaultResponseTx, callTrace: { calls: [], logs } }], + }; } /** @@ -183,21 +167,14 @@ function createNativeBalanceResponse( return { transactions: [ { - return: encodeUint256(previousBalance), - callTrace: { - calls: [], - logs: [], - }, + ...defaultResponseTx, + return: encodeTo32ByteHex(previousBalance), stateDiff: { pre: { - [USER_ADDRESS_MOCK]: { - balance: encodeUint256(previousBalance), - }, + [USER_ADDRESS_MOCK]: { balance: previousBalance }, }, post: { - [encodeAddress(USER_ADDRESS_MOCK)]: { - balance: encodeUint256(newBalance), - }, + [USER_ADDRESS_MOCK]: { balance: newBalance }, }, }, }, @@ -218,37 +195,13 @@ function createBalanceOfResponse( return { transactions: [ ...previousBalances.map((previousBalance) => ({ - return: encodeUint256(previousBalance), - callTrace: { - calls: [], - logs: [], - }, - stateDiff: { - pre: {}, - post: {}, - }, + ...defaultResponseTx, + return: encodeTo32ByteHex(previousBalance), })), - { - return: encodeUint256('0xabc'), // Example correction with encoding - callTrace: { - calls: [], - logs: [], - }, - stateDiff: { - pre: {}, - post: {}, - }, - }, + defaultResponseTx, ...newBalances.map((newBalance) => ({ - return: encodeUint256(newBalance), - callTrace: { - calls: [], - logs: [], - }, - stateDiff: { - pre: {}, - post: {}, - }, + ...defaultResponseTx, + return: encodeTo32ByteHex(newBalance), })), ], } as unknown as SimulationResponse; @@ -770,6 +723,57 @@ describe('Simulation Utils', () => { ], }); }); + + // Ensures no regression of bug https://github.com/MetaMask/metamask-extension/issues/26521 + it('decodes raw balanceOf output correctly for ERC20 token with extra zeros', async () => { + const DECODED_BALANCE_BEFORE = '0x134c31d252'; + const DECODED_BALANCE_AFTER = '0x134c31d257'; + const EXPECTED_BALANCE_CHANGE = '0x5'; + + // Contract returns 64 extra zeros in raw output of balanceOf. + // Abi decoding should ignore them. + const encodeOutputWith64ExtraZeros = (value: string) => + (encodeTo32ByteHex(value) + ''.padStart(64, '0')) as Hex; + const RAW_BALANCE_BEFORE = encodeOutputWith64ExtraZeros( + DECODED_BALANCE_BEFORE, + ); + const RAW_BALANCE_AFTER = encodeOutputWith64ExtraZeros( + DECODED_BALANCE_AFTER, + ); + + mockParseLog({ + erc20: PARSED_ERC20_TRANSFER_EVENT_MOCK, + }); + + simulateTransactionsMock + .mockResolvedValueOnce( + createEventResponseMock([createLogMock(CONTRACT_ADDRESS_2_MOCK)]), + ) + .mockResolvedValueOnce({ + transactions: [ + { ...defaultResponseTx, return: RAW_BALANCE_BEFORE }, + defaultResponseTx, + { ...defaultResponseTx, return: RAW_BALANCE_AFTER }, + ], + }); + + const simulationData = await getSimulationData(REQUEST_MOCK); + + expect(simulationData).toStrictEqual({ + nativeBalanceChange: undefined, + tokenBalanceChanges: [ + { + standard: SimulationTokenStandard.erc20, + address: CONTRACT_ADDRESS_2_MOCK, + id: undefined, + previousBalance: DECODED_BALANCE_BEFORE, + newBalance: DECODED_BALANCE_AFTER, + difference: EXPECTED_BALANCE_CHANGE, + isDecrease: false, + }, + ], + }); + }); }); describe('returns error', () => { @@ -883,54 +887,3 @@ describe('Simulation Utils', () => { }); }); }); - -describe('getValueFromBalanceTransaction', () => { - const from = '0x1234567890123456789012345678901234567890'; - const contractAddress = '0xabcdef1234567890abcdef1234567890abcdef12' as Hex; - - it.each([ - [ - 'ERC20 balance', - SimulationTokenStandard.erc20, - '0x000000000000000000000000000000000000000000000000000000134c31d25200000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000', - '0x134c31d252', - ], - [ - 'ERC721 ownership', - SimulationTokenStandard.erc721, - '0x0000000000000000000000001234567890123456789012345678901234567890', - '0x1', - ], - [ - 'ERC721 non-ownership', - SimulationTokenStandard.erc721, - '0x000000000000000000000000abcdef1234567890abcdef1234567890abcdef12', - '0x0', - ], - [ - 'ERC1155 balance', - SimulationTokenStandard.erc1155, - '0x0000000000000000000000000000000000000000000000000000000000000064', - '0x64', - ], - ])('correctly decodes %s', (_, standard, returnValue, expected) => { - const token = { standard, address: contractAddress }; - const response = { return: returnValue as Hex }; - - const result = getValueFromBalanceTransaction(from, token, response); - - expect(result).toBe(expected); - }); - - it('throws SimulationInvalidResponseError on decoding failure', () => { - const token = { - standard: SimulationTokenStandard.erc20, - address: contractAddress, - }; - const response = { return: '0xInvalidData' as Hex }; - - expect(() => getValueFromBalanceTransaction(from, token, response)).toThrow( - SimulationError, - ); - }); -}); diff --git a/packages/transaction-controller/src/utils/simulation.ts b/packages/transaction-controller/src/utils/simulation.ts index cd61bac95c..5d8aef70a4 100644 --- a/packages/transaction-controller/src/utils/simulation.ts +++ b/packages/transaction-controller/src/utils/simulation.ts @@ -332,14 +332,14 @@ async function getTokenBalanceChanges( const previousBalanceCheckSkipped = !balanceTxs.before.get(token); const previousBalance = previousBalanceCheckSkipped ? '0x0' - : getValueFromBalanceTransaction( + : getAmountFromBalanceTransactionResult( request.from, token, // eslint-disable-next-line no-plusplus response.transactions[prevBalanceTxIndex++], ); - const newBalance = getValueFromBalanceTransaction( + const newBalance = getAmountFromBalanceTransactionResult( request.from, token, response.transactions[index + balanceTxs.before.size + 1], @@ -476,6 +476,24 @@ function getEventTokenIds(event: ParsedEvent): (Hex | undefined)[] { return [undefined]; } +/** + * Get the interface for a token standard. + * @param tokenStandard - The token standard. + * @returns The interface for the token standard. + */ +function getContractInterface( + tokenStandard: SimulationTokenStandard, +): Interface { + switch (tokenStandard) { + case SimulationTokenStandard.erc721: + return new Interface(abiERC721); + case SimulationTokenStandard.erc1155: + return new Interface(abiERC1155); + default: + return new Interface(abiERC20); + } +} + /** * Extract the value from a balance transaction response using the correct ABI. * @param from - The address to check the balance of. @@ -483,33 +501,22 @@ function getEventTokenIds(event: ParsedEvent): (Hex | undefined)[] { * @param response - The balance transaction response. * @returns The value of the balance transaction as Hex. */ -export function getValueFromBalanceTransaction( +function getAmountFromBalanceTransactionResult( from: Hex, token: SimulationToken, response: SimulationResponseTransaction, ): Hex { - const interfaces = getContractInterfaces(); - let decoded; + const contract = getContractInterface(token.standard); try { if (token.standard === SimulationTokenStandard.erc721) { - // ERC721: Check ownership and return 1 if the owner is the user, 0 otherwise. - decoded = ( - interfaces.get(SupportedToken.ERC721) as Interface - ).decodeFunctionResult('ownerOf', response.return); - const owner = decoded[0].toLowerCase(); - return owner === from.toLowerCase() ? '0x1' : '0x0'; + const result = contract.decodeFunctionResult('ownerOf', response.return); + const owner = result[0]; + return hexToBN(owner).eq(hexToBN(from)) ? '0x1' : '0x0'; } - // Non-ERC721: ERC20 and ERC1155 use balanceOf - decoded = ( - interfaces.get( - token.standard === SimulationTokenStandard.erc20 - ? SupportedToken.ERC20 - : SupportedToken.ERC1155, - ) as Interface - ).decodeFunctionResult('balanceOf', response.return); - - return toHex(decoded[0] as Hex); + + const result = contract.decodeFunctionResult('balanceOf', response.return); + return toHex(result[0]); } catch (error) { log('Failed to decode balance transaction', error, { token, response }); throw new SimulationError( @@ -532,22 +539,16 @@ function getBalanceTransactionData( from: Hex, tokenId?: Hex, ): Hex { + const contract = getContractInterface(tokenStandard); switch (tokenStandard) { case SimulationTokenStandard.erc721: - return new Interface(abiERC721).encodeFunctionData('ownerOf', [ - tokenId, - ]) as Hex; + return contract.encodeFunctionData('ownerOf', [tokenId]) as Hex; case SimulationTokenStandard.erc1155: - return new Interface(abiERC1155).encodeFunctionData('balanceOf', [ - from, - tokenId, - ]) as Hex; + return contract.encodeFunctionData('balanceOf', [from, tokenId]) as Hex; default: - return new Interface(abiERC20).encodeFunctionData('balanceOf', [ - from, - ]) as Hex; + return contract.encodeFunctionData('balanceOf', [from]) as Hex; } } From e4a5d4a7fccec5bd14e7a2e8455632decd8f307e Mon Sep 17 00:00:00 2001 From: Derek Brans Date: Fri, 11 Oct 2024 09:42:40 -0400 Subject: [PATCH 4/6] add a comment --- packages/transaction-controller/src/utils/simulation.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/transaction-controller/src/utils/simulation.test.ts b/packages/transaction-controller/src/utils/simulation.test.ts index 8b6975fa92..4afe1b3958 100644 --- a/packages/transaction-controller/src/utils/simulation.test.ts +++ b/packages/transaction-controller/src/utils/simulation.test.ts @@ -29,6 +29,7 @@ const encodeTo32ByteHex = (value: string | number): Hex => { return `0x${BigInt(value).toString(16).padStart(64, '0')}`; }; +// getSimulationData returns values in hex format with leading zeros trimmed. const trimLeadingZeros = (hexString: Hex): Hex => { const trimmed = hexString.replace(/^0x0+/u, '0x') as Hex; return trimmed === '0x' ? '0x0' : trimmed; From 00df4103d6e31a29d79ba623c87fd24be6523e2c Mon Sep 17 00:00:00 2001 From: Derek Brans Date: Fri, 11 Oct 2024 10:36:54 -0400 Subject: [PATCH 5/6] added regression test for user addresses that have a zero in their prefix. --- .../src/utils/simulation.test.ts | 46 ++++++++++++++----- 1 file changed, 35 insertions(+), 11 deletions(-) diff --git a/packages/transaction-controller/src/utils/simulation.test.ts b/packages/transaction-controller/src/utils/simulation.test.ts index 4afe1b3958..f5f24f162e 100644 --- a/packages/transaction-controller/src/utils/simulation.test.ts +++ b/packages/transaction-controller/src/utils/simulation.test.ts @@ -35,19 +35,22 @@ const trimLeadingZeros = (hexString: Hex): Hex => { return trimmed === '0x' ? '0x0' : trimmed; }; -const USER_ADDRESS_MOCK = '0x123'; -const OTHER_ADDRESS_MOCK = '0x456'; -const CONTRACT_ADDRESS_1_MOCK = '0x789'; -const CONTRACT_ADDRESS_2_MOCK = '0xDEF'; -const BALANCE_1_MOCK = '0x0'; -const BALANCE_2_MOCK = '0x1'; -const DIFFERENCE_MOCK = '0x1'; -const VALUE_MOCK = '0x4'; -const TOKEN_ID_MOCK = '0x5'; -const OTHER_TOKEN_ID_MOCK = '0x6'; +const USER_ADDRESS_MOCK = '0x123' as Hex; +const OTHER_ADDRESS_MOCK = '0x456' as Hex; +const CONTRACT_ADDRESS_1_MOCK = '0x789' as Hex; +const CONTRACT_ADDRESS_2_MOCK = '0xDEF' as Hex; +const BALANCE_1_MOCK = '0x0' as Hex; +const BALANCE_2_MOCK = '0x1' as Hex; +const DIFFERENCE_MOCK = '0x1' as Hex; +const VALUE_MOCK = '0x4' as Hex; +const TOKEN_ID_MOCK = '0x5' as Hex; +const OTHER_TOKEN_ID_MOCK = '0x6' as Hex; const ERROR_CODE_MOCK = 123; const ERROR_MESSAGE_MOCK = 'Test Error'; +// Regression test – leading zero in user address +const USER_ADDRESS_WITH_LEADING_ZEROS = '0x0123' as Hex; + const REQUEST_MOCK: GetSimulationDataRequest = { chainId: '0x1', from: USER_ADDRESS_MOCK, @@ -295,6 +298,7 @@ describe('Simulation Utils', () => { it.each([ { title: 'ERC-20 token', + from: USER_ADDRESS_MOCK, parsedEvent: PARSED_ERC20_TRANSFER_EVENT_MOCK, tokenType: SupportedToken.ERC20, tokenStandard: SimulationTokenStandard.erc20, @@ -304,6 +308,7 @@ describe('Simulation Utils', () => { }, { title: 'ERC-721 token', + from: USER_ADDRESS_MOCK, parsedEvent: PARSED_ERC721_TRANSFER_EVENT_MOCK, tokenType: SupportedToken.ERC721, tokenStandard: SimulationTokenStandard.erc721, @@ -311,8 +316,20 @@ describe('Simulation Utils', () => { previousBalances: [OTHER_ADDRESS_MOCK], newBalances: [USER_ADDRESS_MOCK], }, + { + // Regression test – leading zero in user address + title: 'ERC-721 token – where user address has leadding zero', + from: USER_ADDRESS_WITH_LEADING_ZEROS, + parsedEvent: PARSED_ERC721_TRANSFER_EVENT_MOCK, + tokenType: SupportedToken.ERC721, + tokenStandard: SimulationTokenStandard.erc721, + tokenId: TOKEN_ID_MOCK, + previousBalances: [OTHER_ADDRESS_MOCK], + newBalances: [USER_ADDRESS_WITH_LEADING_ZEROS], + }, { title: 'ERC-1155 token via single event', + from: USER_ADDRESS_MOCK, parsedEvent: PARSED_ERC1155_TRANSFER_SINGLE_EVENT_MOCK, tokenType: SupportedToken.ERC1155, tokenStandard: SimulationTokenStandard.erc1155, @@ -322,6 +339,7 @@ describe('Simulation Utils', () => { }, { title: 'ERC-1155 token via batch event', + from: USER_ADDRESS_MOCK, parsedEvent: PARSED_ERC1155_TRANSFER_BATCH_EVENT_MOCK, tokenType: SupportedToken.ERC1155, tokenStandard: SimulationTokenStandard.erc1155, @@ -331,6 +349,7 @@ describe('Simulation Utils', () => { }, { title: 'wrapped ERC-20 token', + from: USER_ADDRESS_MOCK, parsedEvent: PARSED_WRAPPED_ERC20_DEPOSIT_EVENT_MOCK, tokenType: SupportedToken.ERC20_WRAPPED, tokenStandard: SimulationTokenStandard.erc20, @@ -340,6 +359,7 @@ describe('Simulation Utils', () => { }, { title: 'legacy ERC-721 token', + from: USER_ADDRESS_MOCK, parsedEvent: PARSED_ERC721_TRANSFER_EVENT_MOCK, tokenType: SupportedToken.ERC721_LEGACY, tokenStandard: SimulationTokenStandard.erc721, @@ -350,6 +370,7 @@ describe('Simulation Utils', () => { ])( 'on $title', async ({ + from, parsedEvent, tokenStandard, tokenType, @@ -367,7 +388,10 @@ describe('Simulation Utils', () => { createBalanceOfResponse(previousBalances, newBalances), ); - const simulationData = await getSimulationData(REQUEST_MOCK); + const simulationData = await getSimulationData({ + chainId: '0x1', + from, + }); expect(simulationData).toStrictEqual({ nativeBalanceChange: undefined, From e34c9694c1db07c4393b571f4313a504bd979bc8 Mon Sep 17 00:00:00 2001 From: Derek Brans Date: Fri, 11 Oct 2024 11:29:46 -0400 Subject: [PATCH 6/6] fix address length in tests and go back to use toLowerCase to compare addresses --- .../src/utils/simulation.test.ts | 26 +++++++++++++------ .../src/utils/simulation.ts | 2 +- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/packages/transaction-controller/src/utils/simulation.test.ts b/packages/transaction-controller/src/utils/simulation.test.ts index f5f24f162e..1df0a186e8 100644 --- a/packages/transaction-controller/src/utils/simulation.test.ts +++ b/packages/transaction-controller/src/utils/simulation.test.ts @@ -35,10 +35,12 @@ const trimLeadingZeros = (hexString: Hex): Hex => { return trimmed === '0x' ? '0x0' : trimmed; }; -const USER_ADDRESS_MOCK = '0x123' as Hex; -const OTHER_ADDRESS_MOCK = '0x456' as Hex; -const CONTRACT_ADDRESS_1_MOCK = '0x789' as Hex; -const CONTRACT_ADDRESS_2_MOCK = '0xDEF' as Hex; +const USER_ADDRESS_MOCK = '0x1233333333333333333333333333333333333333' as Hex; +const OTHER_ADDRESS_MOCK = '0x4566666666666666666666666666666666666666' as Hex; +const CONTRACT_ADDRESS_1_MOCK = + '0x7899999999999999999999999999999999999999' as Hex; +const CONTRACT_ADDRESS_2_MOCK = + '0xDEFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF' as Hex; const BALANCE_1_MOCK = '0x0' as Hex; const BALANCE_2_MOCK = '0x1' as Hex; const DIFFERENCE_MOCK = '0x1' as Hex; @@ -49,7 +51,8 @@ const ERROR_CODE_MOCK = 123; const ERROR_MESSAGE_MOCK = 'Test Error'; // Regression test – leading zero in user address -const USER_ADDRESS_WITH_LEADING_ZEROS = '0x0123' as Hex; +const USER_ADDRESS_WITH_LEADING_ZERO = + '0x0012333333333333333333333333333333333333' as Hex; const REQUEST_MOCK: GetSimulationDataRequest = { chainId: '0x1', @@ -319,13 +322,20 @@ describe('Simulation Utils', () => { { // Regression test – leading zero in user address title: 'ERC-721 token – where user address has leadding zero', - from: USER_ADDRESS_WITH_LEADING_ZEROS, - parsedEvent: PARSED_ERC721_TRANSFER_EVENT_MOCK, + from: USER_ADDRESS_WITH_LEADING_ZERO, + parsedEvent: { + ...PARSED_ERC721_TRANSFER_EVENT_MOCK, + args: [ + OTHER_ADDRESS_MOCK, + USER_ADDRESS_WITH_LEADING_ZERO, + { toHexString: () => TOKEN_ID_MOCK }, + ], + }, tokenType: SupportedToken.ERC721, tokenStandard: SimulationTokenStandard.erc721, tokenId: TOKEN_ID_MOCK, previousBalances: [OTHER_ADDRESS_MOCK], - newBalances: [USER_ADDRESS_WITH_LEADING_ZEROS], + newBalances: [USER_ADDRESS_WITH_LEADING_ZERO], }, { title: 'ERC-1155 token via single event', diff --git a/packages/transaction-controller/src/utils/simulation.ts b/packages/transaction-controller/src/utils/simulation.ts index 5d8aef70a4..cb6ff73f06 100644 --- a/packages/transaction-controller/src/utils/simulation.ts +++ b/packages/transaction-controller/src/utils/simulation.ts @@ -512,7 +512,7 @@ function getAmountFromBalanceTransactionResult( if (token.standard === SimulationTokenStandard.erc721) { const result = contract.decodeFunctionResult('ownerOf', response.return); const owner = result[0]; - return hexToBN(owner).eq(hexToBN(from)) ? '0x1' : '0x0'; + return owner.toLowerCase() === from.toLowerCase() ? '0x1' : '0x0'; } const result = contract.decodeFunctionResult('balanceOf', response.return);