From f785f7795964dfdb67d8a828b97d1aeb3285996f Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Tue, 12 Jul 2022 14:50:20 -0230 Subject: [PATCH] Improve confirm screen tests (#15163) * Improve confirm screen tests * Fix transaction controller unit test * Fix unit test fixtures * Fix e2e test --- .../controllers/transactions/index.test.js | 1 + shared/modules/transaction.utils.js | 38 ++--- shared/modules/transaction.utils.test.js | 89 +++++++++-- test/e2e/fixtures/special-settings/state.json | 146 ++++++++++++++++++ test/e2e/tests/send-eth.spec.js | 48 ++++++ .../confirm-transaction-base.container.js | 6 +- 6 files changed, 297 insertions(+), 31 deletions(-) create mode 100644 test/e2e/fixtures/special-settings/state.json diff --git a/app/scripts/controllers/transactions/index.test.js b/app/scripts/controllers/transactions/index.test.js index 422aaadd7..c8adf4191 100644 --- a/app/scripts/controllers/transactions/index.test.js +++ b/app/scripts/controllers/transactions/index.test.js @@ -2274,6 +2274,7 @@ describe('Transaction Controller', function () { }); it('updates editible params when type changes from simple send to token transfer', async function () { + providerResultStub.eth_getCode = '0xab'; // test update gasFees await txController.updateEditableParams('1', { data: diff --git a/shared/modules/transaction.utils.js b/shared/modules/transaction.utils.js index 1910ef2f3..688548b8e 100644 --- a/shared/modules/transaction.utils.js +++ b/shared/modules/transaction.utils.js @@ -148,33 +148,35 @@ export async function determineTransactionType(txParams, query) { log.debug('Failed to parse transaction data.', error, data); } - const tokenMethodName = [ - TRANSACTION_TYPES.TOKEN_METHOD_APPROVE, - TRANSACTION_TYPES.TOKEN_METHOD_SET_APPROVAL_FOR_ALL, - TRANSACTION_TYPES.TOKEN_METHOD_TRANSFER, - TRANSACTION_TYPES.TOKEN_METHOD_TRANSFER_FROM, - TRANSACTION_TYPES.TOKEN_METHOD_SAFE_TRANSFER_FROM, - ].find((methodName) => isEqualCaseInsensitive(methodName, name)); - let result; - if (data && tokenMethodName) { - result = tokenMethodName; - } else if (data && !to) { - result = TRANSACTION_TYPES.DEPLOY_CONTRACT; - } - let contractCode; - if (!result) { + if (data && !to) { + result = TRANSACTION_TYPES.DEPLOY_CONTRACT; + } else { const { contractCode: resultCode, isContractAddress, } = await readAddressAsContract(query, to); contractCode = resultCode; - result = isContractAddress - ? TRANSACTION_TYPES.CONTRACT_INTERACTION - : TRANSACTION_TYPES.SIMPLE_SEND; + + if (isContractAddress) { + const tokenMethodName = [ + TRANSACTION_TYPES.TOKEN_METHOD_APPROVE, + TRANSACTION_TYPES.TOKEN_METHOD_SET_APPROVAL_FOR_ALL, + TRANSACTION_TYPES.TOKEN_METHOD_TRANSFER, + TRANSACTION_TYPES.TOKEN_METHOD_TRANSFER_FROM, + TRANSACTION_TYPES.TOKEN_METHOD_SAFE_TRANSFER_FROM, + ].find((methodName) => isEqualCaseInsensitive(methodName, name)); + + result = + data && tokenMethodName + ? tokenMethodName + : TRANSACTION_TYPES.CONTRACT_INTERACTION; + } else { + result = TRANSACTION_TYPES.SIMPLE_SEND; + } } return { type: result, getCodeResponse: contractCode }; diff --git a/shared/modules/transaction.utils.test.js b/shared/modules/transaction.utils.test.js index fba8c7827..6998022e4 100644 --- a/shared/modules/transaction.utils.test.js +++ b/shared/modules/transaction.utils.test.js @@ -111,13 +111,23 @@ describe('Transaction.utils', function () { const genericProvider = createTestProviderTools().provider; const query = new EthQuery(genericProvider); - it('should return a simple send type when to is truthy but data is falsy', async function () { + it('should return a simple send type when to is truthy and is not a contract address', async function () { + const _providerResultStub = { + // 1 gwei + eth_gasPrice: '0x0de0b6b3a7640000', + // by default, all accounts are external accounts (not contracts) + eth_getCode: '0x', + }; + const _provider = createTestProviderTools({ + scaffold: _providerResultStub, + }).provider; + const result = await determineTransactionType( { to: '0xabc', data: '', }, - query, + new EthQuery(_provider), ); expect(result).toMatchObject({ type: TRANSACTION_TYPES.SIMPLE_SEND, @@ -125,33 +135,78 @@ describe('Transaction.utils', function () { }); }); - it('should return a token transfer type when data is for the respective method call', async function () { + it('should return a token transfer type when the recipient is a contract and data is for the respective method call', async function () { + const _providerResultStub = { + // 1 gwei + eth_gasPrice: '0x0de0b6b3a7640000', + // by default, all accounts are external accounts (not contracts) + eth_getCode: '0xab', + }; + const _provider = createTestProviderTools({ + scaffold: _providerResultStub, + }).provider; + const result = await determineTransactionType( { - to: '0xabc', + to: '0x9e673399f795D01116e9A8B2dD2F156705131ee9', data: '0xa9059cbb0000000000000000000000002f318C334780961FB129D2a6c30D0763d9a5C970000000000000000000000000000000000000000000000000000000000000000a', }, - query, + new EthQuery(_provider), ); expect(result).toMatchObject({ type: TRANSACTION_TYPES.TOKEN_METHOD_TRANSFER, - getCodeResponse: undefined, + getCodeResponse: '0xab', }); }); - it('should return a token approve type when data is for the respective method call', async function () { + it('should NOT return a token transfer type when the recipient is not a contract but the data matches the respective method call', async function () { + const _providerResultStub = { + // 1 gwei + eth_gasPrice: '0x0de0b6b3a7640000', + // by default, all accounts are external accounts (not contracts) + eth_getCode: '0x', + }; + const _provider = createTestProviderTools({ + scaffold: _providerResultStub, + }).provider; + const result = await determineTransactionType( { - to: '0xabc', + to: '0x9e673399f795D01116e9A8B2dD2F156705131ee9', + data: + '0xa9059cbb0000000000000000000000002f318C334780961FB129D2a6c30D0763d9a5C970000000000000000000000000000000000000000000000000000000000000000a', + }, + new EthQuery(_provider), + ); + expect(result).toMatchObject({ + type: TRANSACTION_TYPES.SIMPLE_SEND, + getCodeResponse: '0x', + }); + }); + + it('should return a token approve type when when the recipient is a contract and data is for the respective method call', async function () { + const _providerResultStub = { + // 1 gwei + eth_gasPrice: '0x0de0b6b3a7640000', + // by default, all accounts are external accounts (not contracts) + eth_getCode: '0xab', + }; + const _provider = createTestProviderTools({ + scaffold: _providerResultStub, + }).provider; + + const result = await determineTransactionType( + { + to: '0x9e673399f795D01116e9A8B2dD2F156705131ee9', data: '0x095ea7b30000000000000000000000002f318C334780961FB129D2a6c30D0763d9a5C9700000000000000000000000000000000000000000000000000000000000000005', }, - query, + new EthQuery(_provider), ); expect(result).toMatchObject({ type: TRANSACTION_TYPES.TOKEN_METHOD_APPROVE, - getCodeResponse: undefined, + getCodeResponse: '0xab', }); }); @@ -184,12 +239,22 @@ describe('Transaction.utils', function () { }); it('should return a simple send type with a null getCodeResponse when to is truthy and there is data and but getCode returns an error', async function () { + const _providerResultStub = { + // 1 gwei + eth_gasPrice: '0x0de0b6b3a7640000', + // by default, all accounts are external accounts (not contracts) + eth_getCode: null, + }; + const _provider = createTestProviderTools({ + scaffold: _providerResultStub, + }).provider; + const result = await determineTransactionType( { - to: '0xabc', + to: '0x9e673399f795D01116e9A8B2dD2F156705131ee9', data: '0xabd', }, - query, + new EthQuery(_provider), ); expect(result).toMatchObject({ type: TRANSACTION_TYPES.SIMPLE_SEND, diff --git a/test/e2e/fixtures/special-settings/state.json b/test/e2e/fixtures/special-settings/state.json new file mode 100644 index 000000000..6163d7621 --- /dev/null +++ b/test/e2e/fixtures/special-settings/state.json @@ -0,0 +1,146 @@ +{ + "data": { + "AppStateController": { + "mkrMigrationReminderTimestamp": null + }, + "CachedBalancesController": { + "cachedBalances": { + "4": {} + } + }, + "CurrencyController": { + "conversionDate": 1575697244.188, + "conversionRate": 149.61, + "currentCurrency": "usd", + "nativeCurrency": "ETH" + }, + "IncomingTransactionsController": { + "incomingTransactions": {}, + "incomingTxLastFetchedBlocksByNetwork": { + "goerli": null, + "kovan": null, + "mainnet": null, + "rinkeby": 5570536 + } + }, + "KeyringController": { + "vault": "{\"data\":\"s6TpYjlUNsn7ifhEFTkuDGBUM1GyOlPrim7JSjtfIxgTt8/6MiXgiR/CtFfR4dWW2xhq85/NGIBYEeWrZThGdKGarBzeIqBfLFhw9n509jprzJ0zc2Rf+9HVFGLw+xxC4xPxgCS0IIWeAJQ+XtGcHmn0UZXriXm8Ja4kdlow6SWinB7sr/WM3R0+frYs4WgllkwggDf2/Tv6VHygvLnhtzp6hIJFyTjh+l/KnyJTyZW1TkZhDaNDzX3SCOHT\",\"iv\":\"FbeHDAW5afeWNORfNJBR0Q==\",\"salt\":\"TxZ+WbCW6891C9LK/hbMAoUsSEW1E8pyGLVBU6x5KR8=\"}" + }, + "NetworkController": { + "network": "1337", + "provider": { + "nickname": "Localhost 8545", + "rpcUrl": "http://localhost:8545", + "chainId": "0x539", + "ticker": "ETH", + "type": "rpc" + } + }, + "NotificationController": { + "notifications": { + "1": { + "isShown": true + }, + "3": { + "isShown": true + }, + "5": { + "isShown": true + }, + "6": { + "isShown": true + }, + "8": { + "isShown": true + }, + "12": { + "isShown": true + } + } + }, + "OnboardingController": { + "onboardingTabs": {}, + "seedPhraseBackedUp": false + }, + "PermissionsMetadata": { + "domainMetadata": { + "metamask.github.io": { + "icon": null, + "name": "M E T A M A S K M E S H T E S T" + } + }, + "permissionsHistory": {}, + "permissionsLog": [ + { + "id": 746677923, + "method": "eth_accounts", + "methodType": "restricted", + "origin": "metamask.github.io", + "request": { + "id": 746677923, + "jsonrpc": "2.0", + "method": "eth_accounts", + "origin": "metamask.github.io", + "params": [] + }, + "requestTime": 1575697241368, + "response": { + "id": 746677923, + "jsonrpc": "2.0", + "result": [] + }, + "responseTime": 1575697241370, + "success": true + } + ] + }, + "PreferencesController": { + "accountTokens": { + "0x5cfe73b6021e818b776b421b1c4db2474086a7e1": { + "rinkeby": [], + "ropsten": [] + } + }, + "assetImages": {}, + "completedOnboarding": true, + "dismissSeedBackUpReminder": true, + "currentLocale": "en", + "featureFlags": { + "showIncomingTransactions": true, + "transactionTime": false, + "sendHexData": true + }, + "firstTimeFlowType": "create", + "forgottenPassword": false, + "frequentRpcListDetail": [], + "identities": { + "0x5cfe73b6021e818b776b421b1c4db2474086a7e1": { + "address": "0x5cfe73b6021e818b776b421b1c4db2474086a7e1", + "name": "Account 1" + } + }, + "knownMethodData": {}, + "lostIdentities": {}, + "metaMetricsId": null, + "participateInMetaMetrics": false, + "preferences": { + "useNativeCurrencyAsPrimaryCurrency": true + }, + "selectedAddress": "0x5cfe73b6021e818b776b421b1c4db2474086a7e1", + "suggestedTokens": {}, + "tokens": [], + "useBlockie": false, + "useNonceField": false, + "usePhishDetect": true, + "useTokenDetection": true + }, + "config": {}, + "firstTimeInfo": { + "date": 1575697234195, + "version": "7.7.0" + } + }, + "meta": { + "version": 40 + } +} diff --git a/test/e2e/tests/send-eth.spec.js b/test/e2e/tests/send-eth.spec.js index e34ab5731..f14bb1bc5 100644 --- a/test/e2e/tests/send-eth.spec.js +++ b/test/e2e/tests/send-eth.spec.js @@ -92,6 +92,54 @@ describe('Send ETH from inside MetaMask using default gas', function () { }); }); +describe('Send ETH non-contract address with data that matches ERC20 transfer data signature', function () { + const ganacheOptions = { + accounts: [ + { + secretKey: + '0x7C9529A67102755B7E6102D6D950AC5D5863C98713805CEC576B945B15B71EAC', + balance: convertToHexValue(25000000000000000000), + }, + ], + }; + it('renders the correct recipient on the confirmation screen', async function () { + await withFixtures( + { + fixtures: 'special-settings', + ganacheOptions, + title: this.test.title, + }, + async ({ driver }) => { + await driver.navigate(); + await driver.fill('#password', 'correct horse battery staple'); + await driver.press('#password', driver.Key.ENTER); + + await driver.clickElement('[data-testid="eth-overview-send"]'); + + await driver.fill( + 'input[placeholder="Search, public address (0x), or ENS"]', + '0xc427D562164062a23a5cFf596A4a3208e72Acd28', + ); + + await driver.fill( + 'textarea[placeholder="Optional', + '0xa9059cbb0000000000000000000000002f318C334780961FB129D2a6c30D0763d9a5C970000000000000000000000000000000000000000000000000000000000000000a', + ); + + await driver.clickElement({ text: 'Next', tag: 'button' }); + + await driver.clickElement({ text: '0xc42...cd28' }); + + const recipientAddress = await driver.findElements({ + text: '0xc427D562164062a23a5cFf596A4a3208e72Acd28', + }); + + assert.equal(recipientAddress.length, 1); + }, + ); + }); +}); + /* eslint-disable-next-line mocha/max-top-level-suites */ describe('Send ETH from inside MetaMask using advanced gas modal', function () { const ganacheOptions = { diff --git a/ui/pages/confirm-transaction-base/confirm-transaction-base.container.js b/ui/pages/confirm-transaction-base/confirm-transaction-base.container.js index e36049c80..7321d2793 100644 --- a/ui/pages/confirm-transaction-base/confirm-transaction-base.container.js +++ b/ui/pages/confirm-transaction-base/confirm-transaction-base.container.js @@ -54,6 +54,7 @@ import { toChecksumHexAddress } from '../../../shared/modules/hexstring-utils'; import { getGasLoadingAnimationIsShowing } from '../../ducks/app/app'; import { isLegacyTransaction } from '../../helpers/utils/transactions.util'; import { CUSTOM_GAS_ESTIMATE } from '../../../shared/constants/gas'; +import { TRANSACTION_TYPES } from '../../../shared/constants/transaction'; import { isEqualCaseInsensitive } from '../../../shared/modules/string-utils'; import { getTokenAddressParam } from '../../helpers/utils/token-util'; import ConfirmTransactionBase from './confirm-transaction-base.component'; @@ -112,7 +113,10 @@ const mapStateToProps = (state, ownProps) => { const { balance } = accounts[fromAddress]; const { name: fromName } = identities[fromAddress]; - const toAddress = propsToAddress || tokenToAddress || txParamsToAddress; + let toAddress = txParamsToAddress; + if (type !== TRANSACTION_TYPES.SIMPLE_SEND) { + toAddress = propsToAddress || tokenToAddress || txParamsToAddress; + } const tokenList = getTokenList(state); const useTokenDetection = getUseTokenDetection(state);