Set a default value for code in _determineTransactionCategory (#6604)

* Set a default value for code in _determineTransactionCategory

* Adds e2e tests that fail when token txs without gas param are not properly handled.

* Adds unit tests for _determineTransactionCategory

* Base error throwing and simple gas setting in estimateTxGas on transactionCategory

app/scripts/controllers/transactions/index.js

@@ -191,7 +191,7 @@ class TransactionController extends EventEmitter {
     }
     txUtils.validateTxParams(normalizedTxParams)
     // construct txMeta
-    const { transactionCategory, code } = await this._determineTransactionCategory(txParams)
+    const { transactionCategory, getCodeResponse } = await this._determineTransactionCategory(txParams)
     let txMeta = this.txStateManager.generateTxMeta({
       txParams: normalizedTxParams,
       type: TRANSACTION_TYPE_STANDARD,
@@ -204,7 +204,7 @@ class TransactionController extends EventEmitter {
       // check whether recipient account is blacklisted
       recipientBlacklistChecker.checkAccount(txMeta.metamaskNetworkId, normalizedTxParams.to)
       // add default tx params
-      txMeta = await this.addTxGasDefaults(txMeta, code)
+      txMeta = await this.addTxGasDefaults(txMeta, getCodeResponse)
     } catch (error) {
       log.warn(error)
       txMeta.loadingDefaults = false
@@ -224,7 +224,7 @@ class TransactionController extends EventEmitter {
   @param txMeta {Object} - the txMeta object
   @returns {Promise<object>} resolves with txMeta
 */
-  async addTxGasDefaults (txMeta, code) {
+  async addTxGasDefaults (txMeta, getCodeResponse) {
     const txParams = txMeta.txParams
     // ensure value
     txParams.value = txParams.value ? ethUtil.addHexPrefix(txParams.value) : '0x0'
@@ -235,7 +235,7 @@ class TransactionController extends EventEmitter {
     }
     txParams.gasPrice = ethUtil.addHexPrefix(gasPrice.toString(16))
     // set gasLimit
-    return await this.txGasUtil.analyzeGasUsage(txMeta, code)
+    return await this.txGasUtil.analyzeGasUsage(txMeta, getCodeResponse)
   }
 
   /**
@@ -593,6 +593,7 @@ class TransactionController extends EventEmitter {
       try {
         code = await this.query.getCode(to)
       } catch (e) {
+        code = null
         log.warn(e)
       }
       // For an address with no code, geth will return '0x', and ganache-core v2.2.1 will return '0x0'
@@ -601,7 +602,7 @@ class TransactionController extends EventEmitter {
       result = codeIsEmpty ? SEND_ETHER_ACTION_KEY : CONTRACT_INTERACTION_KEY
     }
 
-    return { transactionCategory: result, code }
+    return { transactionCategory: result, getCodeResponse: code }
   }
 
   /**

app/scripts/controllers/transactions/tx-gas-utils.js

@@ -6,6 +6,7 @@ const {
 } = require('../../lib/util')
 const log = require('loglevel')
 const { addHexPrefix } = require('ethereumjs-util')
+const { SEND_ETHER_ACTION_KEY } = require('../../../../ui/app/helpers/constants/transactions.js')
 const SIMPLE_GAS_COST = '0x5208' // Hex for 21000, cost of a simple send.
 
 import { TRANSACTION_NO_CONTRACT_ERROR_KEY } from '../../../../ui/app/helpers/constants/error-keys'
@@ -25,14 +26,13 @@ class TxGasUtil {
 
   /**
     @param txMeta {Object} - the txMeta object
-    @param code {string} - the code at the txs address, as returned by this.query.getCode(to)
     @returns {object} the txMeta object with the gas written to the txParams
   */
-  async analyzeGasUsage (txMeta, code) {
+  async analyzeGasUsage (txMeta, getCodeResponse) {
     const block = await this.query.getBlockByNumber('latest', false)
     let estimatedGasHex
     try {
-      estimatedGasHex = await this.estimateTxGas(txMeta, block.gasLimit, code)
+      estimatedGasHex = await this.estimateTxGas(txMeta, block.gasLimit, getCodeResponse)
     } catch (err) {
       log.warn(err)
       txMeta.simulationFails = {
@@ -55,10 +55,9 @@ class TxGasUtil {
     Estimates the tx's gas usage
     @param txMeta {Object} - the txMeta object
     @param blockGasLimitHex {string} - hex string of the block's gas limit
-    @param code {string} - the code at the txs address, as returned by this.query.getCode(to)
     @returns {string} the estimated gas limit as a hex string
   */
-  async estimateTxGas (txMeta, blockGasLimitHex, code) {
+  async estimateTxGas (txMeta, blockGasLimitHex, getCodeResponse) {
     const txParams = txMeta.txParams
 
     // check if gasLimit is already specified
@@ -75,9 +74,9 @@ class TxGasUtil {
     // see if we can set the gas based on the recipient
     if (hasRecipient) {
       // For an address with no code, geth will return '0x', and ganache-core v2.2.1 will return '0x0'
-      const codeIsEmpty = !code || code === '0x' || code === '0x0'
+      const categorizedAsSimple = txMeta.transactionCategory === SEND_ETHER_ACTION_KEY
 
-      if (codeIsEmpty) {
+      if (categorizedAsSimple) {
         // if there's data in the params, but there's no contract code, it's not a valid transaction
         if (txParams.data) {
           const err = new Error('TxGasUtil - Trying to call a function on a non-contract address')
@@ -85,7 +84,7 @@ class TxGasUtil {
           err.errorKey = TRANSACTION_NO_CONTRACT_ERROR_KEY
 
           // set the response on the error so that we can see in logs what the actual response was
-          err.getCodeResponse = code
+          err.getCodeResponse = getCodeResponse
           throw err
         }
 

test/e2e/beta/contract-test/contract.js

@@ -37,6 +37,8 @@ web3.currentProvider.enable().then(() => {
   const createToken = document.getElementById('createToken')
   const transferTokens = document.getElementById('transferTokens')
   const approveTokens = document.getElementById('approveTokens')
+  const transferTokensWithoutGas = document.getElementById('transferTokensWithoutGas')
+  const approveTokensWithoutGas = document.getElementById('approveTokensWithoutGas')
 
   deployButton.addEventListener('click', async function () {
     document.getElementById('contractStatus').innerHTML = 'Deploying'
@@ -135,6 +137,29 @@ web3.currentProvider.enable().then(() => {
               console.log(result)
             })
           })
+
+          transferTokensWithoutGas.addEventListener('click', function (event) {
+            console.log(`event`, event)
+            contract.transfer('0x2f318C334780961FB129D2a6c30D0763d9a5C970', '7', {
+              from: web3.eth.accounts[0],
+              to: contract.address,
+              data: '0xa9059cbb0000000000000000000000002f318C334780961FB129D2a6c30D0763d9a5C970000000000000000000000000000000000000000000000000000000000000000a',
+              gasPrice: '20000000000',
+            }, function (result) {
+              console.log('result', result)
+            })
+          })
+
+          approveTokensWithoutGas.addEventListener('click', function () {
+            contract.approve('0x2f318C334780961FB129D2a6c30D0763d9a5C970', '7', {
+              from: web3.eth.accounts[0],
+              to: contract.address,
+              data: '0x095ea7b30000000000000000000000002f318C334780961FB129D2a6c30D0763d9a5C9700000000000000000000000000000000000000000000000000000000000000005',
+              gasPrice: '20000000000',
+            }, function (result) {
+              console.log(result)
+            })
+          })
         }
     })
 

test/e2e/beta/contract-test/index.html

@@ -27,6 +27,8 @@
       <button id="createToken">Create Token</button>
       <button id="transferTokens">Transfer Tokens</button>
       <button id="approveTokens">Approve Tokens</button>
+      <button id="transferTokensWithoutGas">Transfer Tokens Without Gas</button>
+      <button id="approveTokensWithoutGas">Approve Tokens Without Gas</button>
     </div>
   </div>
 

test/e2e/beta/metamask-beta-ui.spec.js

@@ -812,10 +812,31 @@ describe('MetaMask', function () {
       await delay(regularDelayMs)
 
       const [gasPriceInput, gasLimitInput] = await findElements(driver, By.css('.advanced-tab__gas-edit-row__input'))
-      await gasPriceInput.clear()
+      await gasPriceInput.sendKeys(Key.chord(Key.CONTROL, 'a'))
+      await delay(50)
+
+      await gasPriceInput.sendKeys(Key.BACK_SPACE)
+      await delay(50)
+      await gasPriceInput.sendKeys(Key.BACK_SPACE)
+      await delay(50)
       await gasPriceInput.sendKeys('10')
-      await gasLimitInput.clear()
+      await delay(50)
+      await gasLimitInput.sendKeys(Key.chord(Key.CONTROL, 'a'))
+      await delay(50)
+      await gasLimitInput.sendKeys(Key.BACK_SPACE)
+      await delay(50)
+      await gasLimitInput.sendKeys(Key.BACK_SPACE)
+      await delay(50)
+      await gasLimitInput.sendKeys(Key.BACK_SPACE)
+      await delay(50)
+      await gasLimitInput.sendKeys(Key.BACK_SPACE)
+      await delay(50)
+      await gasLimitInput.sendKeys(Key.BACK_SPACE)
+      await delay(50)
       await gasLimitInput.sendKeys('60001')
+      await delay(50)
+      await gasLimitInput.sendKeys(Key.chord(Key.CONTROL, 'e'))
+      await delay(50)
 
       const save = await findElement(driver, By.xpath(`//button[contains(text(), 'Save')]`))
       await save.click()
@@ -1175,7 +1196,7 @@ describe('MetaMask', function () {
       const transferTokens = await findElement(driver, By.xpath(`//button[contains(text(), 'Approve Tokens')]`))
       await transferTokens.click()
 
-      await closeAllWindowHandlesExcept(driver, extension)
+      await closeAllWindowHandlesExcept(driver, [extension, dapp])
       await driver.switchTo().window(extension)
       await delay(regularDelayMs)
 
@@ -1228,21 +1249,31 @@ describe('MetaMask', function () {
       await delay(regularDelayMs)
 
       const [gasPriceInput, gasLimitInput] = await findElements(driver, By.css('.advanced-tab__gas-edit-row__input'))
-      await gasPriceInput.clear()
+      await gasPriceInput.sendKeys(Key.chord(Key.CONTROL, 'a'))
-      await delay(tinyDelayMs)
+      await delay(50)
+
+      await gasPriceInput.sendKeys(Key.BACK_SPACE)
+      await delay(50)
+      await gasPriceInput.sendKeys(Key.BACK_SPACE)
+      await delay(50)
       await gasPriceInput.sendKeys('10')
-      await delay(tinyDelayMs)
+      await delay(50)
-      await gasLimitInput.clear()
-      await delay(tinyDelayMs)
       await gasLimitInput.sendKeys(Key.chord(Key.CONTROL, 'a'))
-      await gasLimitInput.sendKeys('60000')
+      await delay(50)
-      await gasLimitInput.sendKeys(Key.chord(Key.CONTROL, 'e'))
-
-      // Needed for different behaviour of input in different versions of firefox
-      const gasLimitInputValue = await gasLimitInput.getAttribute('value')
-      if (gasLimitInputValue === '600001') {
       await gasLimitInput.sendKeys(Key.BACK_SPACE)
-      }
+      await delay(50)
+      await gasLimitInput.sendKeys(Key.BACK_SPACE)
+      await delay(50)
+      await gasLimitInput.sendKeys(Key.BACK_SPACE)
+      await delay(50)
+      await gasLimitInput.sendKeys(Key.BACK_SPACE)
+      await delay(50)
+      await gasLimitInput.sendKeys(Key.BACK_SPACE)
+      await delay(50)
+      await gasLimitInput.sendKeys('60001')
+      await delay(50)
+      await gasLimitInput.sendKeys(Key.chord(Key.CONTROL, 'e'))
+      await delay(50)
 
       const save = await findElement(driver, By.css('.page-container__footer-button'))
       await save.click()
@@ -1271,6 +1302,105 @@ describe('MetaMask', function () {
     })
   })
 
+  describe('Tranfers a custom token from dapp when no gas value is specified', () => {
+    it('transfers an already created token, without specifying gas', async () => {
+      const windowHandles = await driver.getAllWindowHandles()
+      const extension = windowHandles[0]
+      const dapp = await switchToWindowWithTitle(driver, 'E2E Test Dapp', windowHandles)
+      await closeAllWindowHandlesExcept(driver, [extension, dapp])
+      await delay(regularDelayMs)
+
+      await driver.switchTo().window(dapp)
+
+      const transferTokens = await findElement(driver, By.xpath(`//button[contains(text(), 'Transfer Tokens Without Gas')]`))
+      await transferTokens.click()
+
+      await closeAllWindowHandlesExcept(driver, [extension, dapp])
+      await driver.switchTo().window(extension)
+      await delay(regularDelayMs)
+
+      await driver.wait(async () => {
+        const pendingTxes = await findElements(driver, By.css('.transaction-list__pending-transactions .transaction-list-item'))
+        return pendingTxes.length === 1
+      }, 10000)
+
+      const [txListItem] = await findElements(driver, By.css('.transaction-list-item'))
+      const [txListValue] = await findElements(driver, By.css('.transaction-list-item__amount--primary'))
+      await driver.wait(until.elementTextMatches(txListValue, /-7\s*TST/))
+      await txListItem.click()
+      await delay(regularDelayMs)
+    })
+
+    it('submits the transaction', async function () {
+      await delay(regularDelayMs)
+      const confirmButton = await findElement(driver, By.xpath(`//button[contains(text(), 'Confirm')]`))
+      await confirmButton.click()
+      await delay(regularDelayMs)
+    })
+
+    it('finds the transaction in the transactions list', async function () {
+      await driver.wait(async () => {
+        const confirmedTxes = await findElements(driver, By.css('.transaction-list__completed-transactions .transaction-list-item'))
+        return confirmedTxes.length === 4
+      }, 10000)
+
+      const txValues = await findElements(driver, By.css('.transaction-list-item__amount--primary'))
+      await driver.wait(until.elementTextMatches(txValues[0], /-7\s*TST/))
+      const txStatuses = await findElements(driver, By.css('.transaction-list-item__action'))
+      await driver.wait(until.elementTextMatches(txStatuses[0], /Sent Tokens/))
+    })
+  })
+
+  describe('Approves a custom token from dapp when no gas value is specified', () => {
+    it('approves an already created token', async () => {
+      const windowHandles = await driver.getAllWindowHandles()
+      const extension = windowHandles[0]
+      const dapp = await switchToWindowWithTitle(driver, 'E2E Test Dapp', windowHandles)
+      await closeAllWindowHandlesExcept(driver, [extension, dapp])
+      await delay(regularDelayMs)
+
+      await driver.switchTo().window(dapp)
+      await delay(tinyDelayMs)
+
+      const transferTokens = await findElement(driver, By.xpath(`//button[contains(text(), 'Approve Tokens Without Gas')]`))
+      await transferTokens.click()
+
+      await closeAllWindowHandlesExcept(driver, extension)
+      await driver.switchTo().window(extension)
+      await delay(regularDelayMs)
+
+      await driver.wait(async () => {
+        const pendingTxes = await findElements(driver, By.css('.transaction-list__pending-transactions .transaction-list-item'))
+        return pendingTxes.length === 1
+      }, 10000)
+
+      const [txListItem] = await findElements(driver, By.css('.transaction-list-item'))
+      const [txListValue] = await findElements(driver, By.css('.transaction-list-item__amount--primary'))
+      await driver.wait(until.elementTextMatches(txListValue, /-7\s*TST/))
+      await txListItem.click()
+      await delay(regularDelayMs)
+    })
+
+    it('submits the transaction', async function () {
+      await delay(regularDelayMs)
+      const confirmButton = await findElement(driver, By.xpath(`//button[contains(text(), 'Confirm')]`))
+      await confirmButton.click()
+      await delay(regularDelayMs)
+    })
+
+    it('finds the transaction in the transactions list', async function () {
+      await driver.wait(async () => {
+        const confirmedTxes = await findElements(driver, By.css('.transaction-list__completed-transactions .transaction-list-item'))
+        return confirmedTxes.length === 5
+      }, 10000)
+
+      const txValues = await findElements(driver, By.css('.transaction-list-item__amount--primary'))
+      await driver.wait(until.elementTextMatches(txValues[0], /-7\s*TST/))
+      const txStatuses = await findElements(driver, By.css('.transaction-list-item__action'))
+      await driver.wait(until.elementTextMatches(txStatuses[0], /Approve/))
+    })
+  })
+
   describe('Hide token', () => {
     it('hides the token when clicked', async () => {
       const [hideTokenEllipsis] = await findElements(driver, By.css('.token-list-item__ellipsis'))

test/unit/app/controllers/transactions/tx-controller-test.js

@@ -8,6 +8,13 @@ const TransactionController = require('../../../../../app/scripts/controllers/tr
 const {
   TRANSACTION_TYPE_RETRY,
 } = require('../../../../../app/scripts/controllers/transactions/enums')
+const {
+  TOKEN_METHOD_APPROVE,
+  TOKEN_METHOD_TRANSFER,
+  SEND_ETHER_ACTION_KEY,
+  DEPLOY_CONTRACT_ACTION_KEY,
+  CONTRACT_INTERACTION_KEY,
+} = require('../../../../../ui/app/helpers/constants/transactions.js')
 const { createTestProviderTools, getTestAccounts } = require('../../../../stub/provider')
 
 const noop = () => true
@@ -537,6 +544,86 @@ describe('Transaction Controller', function () {
     })
   })
 
+  describe('#_determineTransactionCategory', function () {
+    it('should return a simple send transactionCategory when to is truthy but data is falsey', async function () {
+      const result = await txController._determineTransactionCategory({
+        to: '0xabc',
+        data: '',
+      })
+      assert.deepEqual(result, { transactionCategory: SEND_ETHER_ACTION_KEY, getCodeResponse: undefined })
+    })
+
+    it('should return a token transfer transactionCategory when data is for the respective method call', async function () {
+      const result = await txController._determineTransactionCategory({
+        to: '0xabc',
+        data: '0xa9059cbb0000000000000000000000002f318C334780961FB129D2a6c30D0763d9a5C970000000000000000000000000000000000000000000000000000000000000000a',
+      })
+      assert.deepEqual(result, { transactionCategory: TOKEN_METHOD_TRANSFER, getCodeResponse: undefined })
+    })
+
+    it('should return a token approve transactionCategory when data is for the respective method call', async function () {
+      const result = await txController._determineTransactionCategory({
+        to: '0xabc',
+        data: '0x095ea7b30000000000000000000000002f318C334780961FB129D2a6c30D0763d9a5C9700000000000000000000000000000000000000000000000000000000000000005',
+      })
+      assert.deepEqual(result, { transactionCategory: TOKEN_METHOD_APPROVE, getCodeResponse: undefined })
+    })
+
+    it('should return a contract deployment transactionCategory when to is falsey and there is data', async function () {
+      const result = await txController._determineTransactionCategory({
+        to: '',
+        data: '0xabd',
+      })
+      assert.deepEqual(result, { transactionCategory: DEPLOY_CONTRACT_ACTION_KEY, getCodeResponse: undefined })
+    })
+
+    it('should return a simple send transactionCategory with a 0x getCodeResponse when there is data and but the to address is not a contract address', async function () {
+      const result = await txController._determineTransactionCategory({
+        to: '0x9e673399f795D01116e9A8B2dD2F156705131ee9',
+        data: '0xabd',
+      })
+      assert.deepEqual(result, { transactionCategory: SEND_ETHER_ACTION_KEY, getCodeResponse: '0x' })
+    })
+
+    it('should return a simple send transactionCategory with a null getCodeResponse when to is truthy and there is data and but getCode returns an error', async function () {
+      const result = await txController._determineTransactionCategory({
+        to: '0xabc',
+        data: '0xabd',
+      })
+      assert.deepEqual(result, { transactionCategory: SEND_ETHER_ACTION_KEY, getCodeResponse: null })
+    })
+
+    it('should return a contract interaction transactionCategory with the correct getCodeResponse when to is truthy and there is data and it is not a token transaction', async function () {
+      const _providerResultStub = {
+        // 1 gwei
+        eth_gasPrice: '0x0de0b6b3a7640000',
+        // by default, all accounts are external accounts (not contracts)
+        eth_getCode: '0xa',
+      }
+      const _provider = createTestProviderTools({ scaffold: _providerResultStub }).provider
+      const _fromAccount = getTestAccounts()[0]
+      const _blockTrackerStub = new EventEmitter()
+      _blockTrackerStub.getCurrentBlock = noop
+      _blockTrackerStub.getLatestBlock = noop
+      const _txController = new TransactionController({
+        provider: _provider,
+        getGasPrice: function () { return '0xee6b2800' },
+        networkStore: new ObservableStore(currentNetworkId),
+        txHistoryLimit: 10,
+        blockTracker: _blockTrackerStub,
+        signTransaction: (ethTx) => new Promise((resolve) => {
+          ethTx.sign(_fromAccount.key)
+          resolve()
+        }),
+      })
+      const result = await _txController._determineTransactionCategory({
+        to: '0x9e673399f795D01116e9A8B2dD2F156705131ee9',
+        data: 'abd',
+      })
+      assert.deepEqual(result, { transactionCategory: CONTRACT_INTERACTION_KEY, getCodeResponse: '0x0a' })
+    })
+  })
+
   describe('#getPendingTransactions', function () {
     beforeEach(function () {
       txController.txStateManager._saveTxList([