From 6918c526a1bf3925948f662b36bd1131e04fbcf3 Mon Sep 17 00:00:00 2001 From: seaona <54408225+seaona@users.noreply.github.com> Date: Thu, 27 Jun 2024 09:32:29 +0200 Subject: [PATCH 1/2] fix: flaky test ` onboarding @no-mmi doesn't make any network requests to infura before onboarding is completed/test-failure-screenshot-1.png` timeout (#25525) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This test appears flaky as sometimes not all the requests are made when we finish the onboarding. Notice how, we mock 5 requests, but in the logs, only 4 appear after the onboarding. Specifically the `net_version` request doesn't happen in this case. [EDIT] It seems that the explanation for this is what @HowardBraham mentions below: since we mock the balance as non-zero, the wallet keeps querying the balance for the subsequent accounts. This could be that causes the wallet to enter in a non-finishing loop, and the `net_version` request is also never triggered (see this should happen after the `eth_call`) ![image](https://github.com/MetaMask/metamask-extension/assets/54408225/291d94f5-e0aa-4ff9-b96b-46c8bcdd7cc8) [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25525?quickstart=1) ## **Related issues** Fixes: https://github.com/MetaMask/metamask-extension/issues/25543 ## **Manual testing steps** 1. Check ci ## **Screenshots/Recordings** ![Screenshot from 2024-06-26 15-05-52](https://github.com/MetaMask/metamask-extension/assets/54408225/93a9d29c-10ca-479d-9e67-e76c46c23fb8) ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.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. --- test/e2e/tests/onboarding/onboarding.spec.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/e2e/tests/onboarding/onboarding.spec.js b/test/e2e/tests/onboarding/onboarding.spec.js index de0939ac2282..c9cb594f403d 100644 --- a/test/e2e/tests/onboarding/onboarding.spec.js +++ b/test/e2e/tests/onboarding/onboarding.spec.js @@ -372,7 +372,7 @@ describe('MetaMask onboarding @no-mmi', function () { json: { jsonrpc: '2.0', id: '1111111111111111', - result: '0x1', + result: '0x0', }, }; }), @@ -508,7 +508,7 @@ describe('MetaMask onboarding @no-mmi', function () { json: { jsonrpc: '2.0', id: '1111111111111111', - result: '0x1', + result: '0x0', }, }; }), From f82a04ae4765f7799e1f26513884b7108ac339e3 Mon Sep 17 00:00:00 2001 From: Vinicius Stevam <45455812+vinistevam@users.noreply.github.com> Date: Thu, 27 Jun 2024 09:21:04 +0100 Subject: [PATCH 2/2] fix: enhance accuracy in determining recipient address for contract interactions (#25445) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This PR enhances the accuracy of recipient address determination during contract interactions. Previously, the logic incorrectly identified the `tokenToAddress` as the recipient for all contract interactions that weren't `simple send` and matched function signatures within the `ERC20`, `ERC721`, `ERC1155`, and `USDC` ABIs. So when the function matches one the ABI's but no `to` to `_to` is present in the parameters it falls back to return the first parameter, if present. This fix refined the logic to accurately use tokenToAddress as the recipient only for transactions explicitly involving token transfers from a specific list. This includes tokenMethodTransfer, tokenMethodTransferFrom, and tokenMethodSafeTransferFrom transaction types, ensuring recipient addresses are correctly determined for these specific interactions. Apart from that, we use the `txParamsToAddress`. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25445?quickstart=1) ## **Related issues** Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2679 https://github.com/MetaMask/metamask-extension/issues/24561 ## **Manual testing steps** 1. Instructions to test are in the above ticket ## **Screenshots/Recordings** [0-security-issue.webm](https://github.com/MetaMask/metamask-extension/assets/45455812/a68d6d84-abcc-4e79-b9ac-609b0ff242f5) ### **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/develop/.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/develop/.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. --- .../confirm-transaction-base.container.js | 36 +++++---- .../confirm-transaction-base.test.js | 73 +++++++++++++++---- .../confirm-transaction.transaction.test.js | 2 +- 3 files changed, 79 insertions(+), 32 deletions(-) diff --git a/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.container.js b/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.container.js index 5a22bfeb381c..7c8fb8f350f2 100644 --- a/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.container.js +++ b/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.container.js @@ -85,10 +85,7 @@ import { parseStandardTokenTransactionData, txParamsAreDappSuggested, } from '../../../../shared/modules/transaction.utils'; -import { - isEmptyHexString, - toChecksumHexAddress, -} from '../../../../shared/modules/hexstring-utils'; +import { toChecksumHexAddress } from '../../../../shared/modules/hexstring-utils'; import { getGasLoadingAnimationIsShowing } from '../../../ducks/app/app'; import { isLegacyTransaction } from '../../../helpers/utils/transactions.util'; @@ -108,7 +105,6 @@ import { } from '../../../selectors/institutional/selectors'; import { showCustodyConfirmLink } from '../../../store/institutional/institution-actions'; ///: END:ONLY_INCLUDE_IF -import { getTokenAddressParam } from '../../../helpers/utils/token-util'; import { calcGasTotal } from '../../../../shared/lib/transactions-controller-utils'; import { subtractHexes } from '../../../../shared/modules/conversion.utils'; import ConfirmTransactionBase from './confirm-transaction-base.component'; @@ -131,6 +127,24 @@ function addressIsNew(toAccounts, newAddress) { return !foundMatching; } +function getTokenToAddress(data, type) { + if ( + ![ + TransactionType.tokenMethodTransferFrom, + TransactionType.tokenMethodSafeTransferFrom, + TransactionType.tokenMethodTransfer, + ].includes(type) + ) { + return undefined; + } + + const transactionData = parseStandardTokenTransactionData(data); + + const value = transactionData?.args?._to || transactionData?.args?.to; + + return value?.toString().toLowerCase(); +} + const mapStateToProps = (state, ownProps) => { const { toAddress: propsToAddress, @@ -163,7 +177,6 @@ const mapStateToProps = (state, ownProps) => { to: txParamsToAddress, gasPrice, gas: gasLimit, - value: amount, data, } = (transaction && transaction.txParams) || txParams; const accounts = getMetaMaskAccounts(state); @@ -171,20 +184,13 @@ const mapStateToProps = (state, ownProps) => { const currentChainSupportsSmartTransactions = getCurrentChainSupportsSmartTransactions(state); - const transactionData = parseStandardTokenTransactionData(data); - const tokenToAddress = getTokenAddressParam(transactionData); - const { balance } = accounts[fromAddress]; const fromInternalAccount = getInternalAccountByAddress(state, fromAddress); const fromName = fromInternalAccount?.metadata.name; const keyring = findKeyringForAddress(state, fromAddress); - const isSendingAmount = - type === TransactionType.simpleSend || !isEmptyHexString(amount); - - const toAddress = isSendingAmount - ? txParamsToAddress - : propsToAddress || tokenToAddress || txParamsToAddress; + const tokenToAddress = getTokenToAddress(data, type); + const toAddress = propsToAddress || tokenToAddress || txParamsToAddress; const toAccounts = getSendToAccounts(state); diff --git a/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.test.js b/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.test.js index bc36b2667576..918c24ed6681 100644 --- a/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.test.js +++ b/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.test.js @@ -775,7 +775,7 @@ describe('Confirm Transaction Base', () => { TransactionType.contractInteraction; }); - describe('when there is an amount being sent (it should be treated as a general contract intereaction rather than custom one)', () => { + describe('when there is an amount being sent (it should be treated as a general contract interaction rather than custom one)', () => { it('should use txParams.to address (contract address)', async () => { const state = mockedStoreWithConfirmTxParams(baseStore, { ...mockTxParams, @@ -795,33 +795,69 @@ describe('Confirm Transaction Base', () => { }); }); - describe(`when there is no amount being sent`, () => { - it('should use propToAddress (toAddress passed as prop)', async () => { + describe('when determines the recipient from transaction data args', () => { + const testCases = [ + { + type: TransactionType.tokenMethodTransfer, + data: `0xa9059cbb000000000000000000000000${mockParsedTxDataToAddressWithout0x}0000000000000000000000000000000000000000000000000000000000000001`, + description: 'tokenMethodTransfer', + }, + { + type: TransactionType.tokenMethodSafeTransferFrom, + data: `0x42842e0e000000000000000000000000806627172af48bd5b0765d3449a7def80d6576ff000000000000000000000000${mockParsedTxDataToAddressWithout0x}000000000000000000000000000000000000000000000000000000000009a7cc`, + description: 'tokenMethodSafeTransferFrom', + }, + { + type: TransactionType.tokenMethodTransferFrom, + data: `0x23b872dd000000000000000000000000ac9539a7d5c43940af498008a7c8f3abb35c3725000000000000000000000000${mockParsedTxDataToAddressWithout0x}000000000000000000000000000000000000000000000000000000000009a7b8`, + description: 'tokenMethodTransferFrom', + }, + ]; + + it.each(testCases)( + 'identifies correctly the recipient for $description transactions', + async ({ type, data }) => { + const state = mockedStoreWithConfirmTxParams(baseStore, { + ...mockTxParams, + data, + }); + state.confirmTransaction.txData = { + ...state.confirmTransaction.txData, + type, + }; + + const { container } = await render({ state }); + + const recipientElem = container.querySelector( + sendToRecipientSelector, + ); + expect(recipientElem).toHaveTextContent(mockParsedTxDataToAddress); + }, + ); + }); + + describe('when a non-transfer function matching the ABIs', () => { + it('does not determine the recipient from transaction data args', async () => { const state = mockedStoreWithConfirmTxParams(baseStore, { ...mockTxParams, - value: '0x0', + data: `0xa22cb465000000000000000000000000${mockParsedTxDataToAddressWithout0x}0000000000000000000000000000000000000000000000000000000000000001`, }); state.confirmTransaction.txData = { ...state.confirmTransaction.txData, type: TransactionType.contractInteraction, }; - const props = { - // we want to test toAddress provided by ownProps in mapStateToProps, but this - // currently overrides toAddress this should pan out fine when we refactor the - // component into a functional component and remove the container.js file - toAddress: mockPropsToAddress, - }; - - const { container } = await render({ props, state }); + const { container } = await render({ state }); const recipientElem = container.querySelector( sendToRecipientSelector, ); - expect(recipientElem).toHaveTextContent(mockPropsToAddressConcat); + expect(recipientElem).toHaveTextContent(mockTxParamsToAddressConcat); }); + }); - it('should use address parsed from transaction data if propToAddress is not provided', async () => { + describe(`when there is no amount being sent`, () => { + it('should use propToAddress (toAddress passed as prop)', async () => { const state = mockedStoreWithConfirmTxParams(baseStore, { ...mockTxParams, value: '0x0', @@ -831,14 +867,19 @@ describe('Confirm Transaction Base', () => { type: TransactionType.contractInteraction, }; - const props = {}; + const props = { + // we want to test toAddress provided by ownProps in mapStateToProps, but this + // currently overrides toAddress this should pan out fine when we refactor the + // component into a functional component and remove the container.js file + toAddress: mockPropsToAddress, + }; const { container } = await render({ props, state }); const recipientElem = container.querySelector( sendToRecipientSelector, ); - expect(recipientElem).toHaveTextContent(mockParsedTxDataToAddress); + expect(recipientElem).toHaveTextContent(mockPropsToAddressConcat); }); it('should use txParams.to if neither propToAddress is not provided nor the transaction data to address were provided', async () => { diff --git a/ui/pages/confirmations/confirm-transaction/confirm-transaction.transaction.test.js b/ui/pages/confirmations/confirm-transaction/confirm-transaction.transaction.test.js index 5ea10a6183de..f9101c0e7d7c 100644 --- a/ui/pages/confirmations/confirm-transaction/confirm-transaction.transaction.test.js +++ b/ui/pages/confirmations/confirm-transaction/confirm-transaction.transaction.test.js @@ -54,7 +54,7 @@ describe('Confirm Transaction', () => { )), ); - expect(result.getByText('0xb19Ac...f0c5e')).toBeInTheDocument(); + expect(result.getByText('Ledger Hardware 2')).toBeInTheDocument(); expect(result.getByRole('button', { name: 'Details' })).toBeInTheDocument(); expect(result.getByRole('button', { name: 'Hex' })).toBeInTheDocument(); });