Cleanup detect-tokens controller and tests (#8329)

The tests for the detect-tokens controller were nearly all broken. They
have been fixed, and a few improvements were made to controller itself
to help with this.

* The core `detectNewTokens` method has been updated to be async, so
that the caller can know when the operation had completed.
* The part of the function that used `Web3` to check the token balances
has been split into a separate function, so that that part could be
stubbed out in tests. Eventually we should test this using `ganache`
instead, but this was an easier first step.
* The internal `tokenAddresses` array is now initialized on
construction, rather than upon the first Preferences controller update.
The `detectNewTokens` function would have previously failed if it ran
prior to this initialization, so it was failing if called before any
preferences state changes.

Additionally, the `detectTokenBalance` function was removed, as it was
no longer used.

The tests have been updated to ensure they're actually testing the
behavior they purport to be testing. I've simulated a test failure with
each one to check that it'd fail when it should. The preferences
controller instance was updated to set addresses correctly as well.

app/scripts/controllers/detect-tokens.js

@@ -4,7 +4,6 @@ import { warn } from 'loglevel'
 import { MAINNET } from './network/enums'
 // By default, poll every 3 minutes
 const DEFAULT_INTERVAL = 180 * 1000
-const ERC20_ABI = [{ 'constant': true, 'inputs': [{ 'name': '_owner', 'type': 'address' }], 'name': 'balanceOf', 'outputs': [{ 'name': 'balance', 'type': 'uint256' }], 'payable': false, 'type': 'function' }]
 import SINGLE_CALL_BALANCES_ABI from 'single-call-balance-checker-abi'
 
 const SINGLE_CALL_BALANCES_ADDRESS = '0xb1f8e55c7f64d203c1400b9d8555d050f94adf39'
@@ -36,6 +35,7 @@ class DetectTokensController {
     if (this._network.store.getState().provider.type !== MAINNET) {
       return
     }
+
     const tokensToDetect = []
     this.web3.setProvider(this._network._provider)
     for (const contractAddress in contracts) {
@@ -44,38 +44,31 @@ class DetectTokensController {
       }
     }
 
-    const ethContract = this.web3.eth.contract(SINGLE_CALL_BALANCES_ABI).at(SINGLE_CALL_BALANCES_ADDRESS)
+    let result
-    ethContract.balances([this.selectedAddress], tokensToDetect, (error, result) => {
+    try {
-      if (error) {
+      result = await this._getTokenBalances(tokensToDetect)
-        warn(`MetaMask - DetectTokensController single call balance fetch failed`, error)
+    } catch (error) {
-        return
+      warn(`MetaMask - DetectTokensController single call balance fetch failed`, error)
+      return
+    }
+
+    tokensToDetect.forEach((tokenAddress, index) => {
+      const balance = result[index]
+      if (balance && !balance.isZero()) {
+        this._preferences.addToken(tokenAddress, contracts[tokenAddress].symbol, contracts[tokenAddress].decimals)
       }
-      tokensToDetect.forEach((tokenAddress, index) => {
-        const balance = result[index]
-        if (balance && !balance.isZero()) {
-          this._preferences.addToken(tokenAddress, contracts[tokenAddress].symbol, contracts[tokenAddress].decimals)
-        }
-      })
     })
   }
 
-  /**
+  async _getTokenBalances (tokens) {
-   * Find if selectedAddress has tokens with contract in contractAddress.
+    const ethContract = this.web3.eth.contract(SINGLE_CALL_BALANCES_ABI).at(SINGLE_CALL_BALANCES_ADDRESS)
-   *
+    return new Promise((resolve, reject) => {
-   * @param {string} contractAddress - Hex address of the token contract to explore.
+      ethContract.balances([this.selectedAddress], tokens, (error, result) => {
-   * @returns {boolean} - If balance is detected, token is added.
+        if (error) {
-   *
+          return reject(error)
-   */
-  async detectTokenBalance (contractAddress) {
-    const ethContract = this.web3.eth.contract(ERC20_ABI).at(contractAddress)
-    ethContract.balanceOf(this.selectedAddress, (error, result) => {
-      if (!error) {
-        if (!result.isZero()) {
-          this._preferences.addToken(contractAddress, contracts[contractAddress].symbol, contracts[contractAddress].decimals)
         }
-      } else {
+        return resolve(result)
-        warn(`MetaMask - DetectTokensController balance fetch failed for ${contractAddress}.`, error)
+      })
-      }
     })
   }
 
@@ -114,9 +107,13 @@ class DetectTokensController {
       return
     }
     this._preferences = preferences
+    const currentTokens = preferences.store.getState().tokens
+    this.tokenAddresses = currentTokens
+      ? currentTokens.map((token) => token.address)
+      : []
     preferences.store.subscribe(({ tokens = [] }) => {
-      this.tokenAddresses = tokens.map((obj) => {
+      this.tokenAddresses = tokens.map((token) => {
-        return obj.address
+        return token.address
       })
     })
     preferences.store.subscribe(({ selectedAddress }) => {

test/unit/app/controllers/detect-tokens-test.js

@@ -2,13 +2,17 @@ import assert from 'assert'
 import nock from 'nock'
 import sinon from 'sinon'
 import ObservableStore from 'obs-store'
+import contracts from 'eth-contract-metadata'
+import BigNumber from 'bignumber.js'
+
 import DetectTokensController from '../../../../app/scripts/controllers/detect-tokens'
 import NetworkController from '../../../../app/scripts/controllers/network/network'
 import PreferencesController from '../../../../app/scripts/controllers/preferences'
+import { MAINNET, ROPSTEN } from '../../../../app/scripts/controllers/network/enums'
 
 describe('DetectTokensController', function () {
   const sandbox = sinon.createSandbox()
-  let clock, keyringMemStore, network, preferences, controller
+  let keyringMemStore, network, preferences
 
   const noop = () => {}
 
@@ -26,8 +30,10 @@ describe('DetectTokensController', function () {
     keyringMemStore = new ObservableStore({ isUnlocked: false })
     network = new NetworkController()
     preferences = new PreferencesController({ network })
-    controller = new DetectTokensController({ preferences: preferences, network: network, keyringMemStore: keyringMemStore })
+    preferences.setAddresses([
-
+      '0x7e57e2',
+      '0xbc86727e770de68b1060c91f6bb6945c73e10388',
+    ])
     network.initializeProvider(networkControllerProviderConfig)
 
   })
@@ -45,12 +51,9 @@ describe('DetectTokensController', function () {
   })
 
   it('should be called on every polling period', async function () {
-    clock = sandbox.useFakeTimers()
+    const clock = sandbox.useFakeTimers()
-    const network = new NetworkController()
+    network.setProviderType(MAINNET)
-    network.initializeProvider(networkControllerProviderConfig)
+    const controller = new DetectTokensController({ preferences, network, keyringMemStore })
-    network.setProviderType('mainnet')
-    const preferences = new PreferencesController({ network })
-    const controller = new DetectTokensController({ preferences: preferences, network: network, keyringMemStore: keyringMemStore })
     controller.isOpen = true
     controller.isUnlocked = true
 
@@ -66,52 +69,61 @@ describe('DetectTokensController', function () {
     sandbox.assert.calledThrice(stub)
   })
 
-  it('should not check tokens while in test network', async function () {
+  it('should not check tokens while on test network', async function () {
+    sandbox.useFakeTimers()
+    network.setProviderType(ROPSTEN)
+    const controller = new DetectTokensController({ preferences, network, keyringMemStore })
     controller.isOpen = true
     controller.isUnlocked = true
 
-    const stub = sandbox.stub(controller, 'detectTokenBalance')
+    const stub = sandbox.stub(controller, '_getTokenBalances')
-      .withArgs('0x0D262e5dC4A06a0F1c90cE79C7a60C09DfC884E4').returns(true)
-      .withArgs('0xBC86727E770de68B1060C91f6BB6945c73e10388').returns(true)
 
     await controller.detectNewTokens()
     sandbox.assert.notCalled(stub)
   })
 
-  it('should only check and add tokens while in main network', async function () {
+  it('should check and add tokens while on main network', async function () {
-    const controller = new DetectTokensController({ preferences: preferences, network: network, keyringMemStore: keyringMemStore })
+    sandbox.useFakeTimers()
+    network.setProviderType(MAINNET)
+    const controller = new DetectTokensController({ preferences, network, keyringMemStore })
     controller.isOpen = true
     controller.isUnlocked = true
 
-    sandbox.stub(controller, 'detectTokenBalance')
+    const contractAddressses = Object.keys(contracts)
-      .withArgs('0x0D262e5dC4A06a0F1c90cE79C7a60C09DfC884E4')
+    const erc20ContractAddresses = contractAddressses
-      .returns(preferences.addToken('0x0d262e5dc4a06a0f1c90ce79c7a60c09dfc884e4', 'J8T', 8))
+      .filter((contractAddress) => contracts[contractAddress].erc20 === true)
-      .withArgs('0xBC86727E770de68B1060C91f6BB6945c73e10388')
+
-      .returns(preferences.addToken('0xbc86727e770de68b1060c91f6bb6945c73e10388', 'XNK', 18))
+    const existingTokenAddress = erc20ContractAddresses[0]
+    const existingToken = contracts[existingTokenAddress]
+    await preferences.addToken(existingTokenAddress, existingToken.symbol, existingToken.decimals)
+
+    const tokenAddressToAdd = erc20ContractAddresses[1]
+    const tokenToAdd = contracts[tokenAddressToAdd]
+
+    const contractAddresssesToDetect = contractAddressses
+      .filter((address) => address !== existingTokenAddress)
+    const indexOfTokenToAdd = contractAddresssesToDetect.indexOf(tokenAddressToAdd)
+
+    const balances = new Array(contractAddresssesToDetect.length)
+    balances[indexOfTokenToAdd] = new BigNumber(10)
+
+    sandbox.stub(controller, '_getTokenBalances')
+      .returns(Promise.resolve(balances))
 
     await controller.detectNewTokens()
-    assert.deepEqual(preferences.store.getState().tokens, [{ address: '0x0d262e5dc4a06a0f1c90ce79c7a60c09dfc884e4', decimals: 8, symbol: 'J8T' },
-      { address: '0xbc86727e770de68b1060c91f6bb6945c73e10388', decimals: 18, symbol: 'XNK' }])
-  })
 
-  it('should not detect same token while in main network', async function () {
+    assert.deepEqual(
-    preferences.addToken('0x0d262e5dc4a06a0f1c90ce79c7a60c09dfc884e4', 'J8T', 8)
+      preferences.store.getState().tokens,
-    const controller = new DetectTokensController({ preferences: preferences, network: network, keyringMemStore: keyringMemStore })
+      [
-    controller.isOpen = true
+        { address: existingTokenAddress.toLowerCase(), decimals: existingToken.decimals, symbol: existingToken.symbol },
-    controller.isUnlocked = true
+        { address: tokenAddressToAdd.toLowerCase(), decimals: tokenToAdd.decimals, symbol: tokenToAdd.symbol },
-
+      ]
-    sandbox.stub(controller, 'detectTokenBalance')
+    )
-      .withArgs('0x0D262e5dC4A06a0F1c90cE79C7a60C09DfC884E4')
-      .returns(preferences.addToken('0x0d262e5dc4a06a0f1c90ce79c7a60c09dfc884e4', 'J8T', 8))
-      .withArgs('0xBC86727E770de68B1060C91f6BB6945c73e10388')
-      .returns(preferences.addToken('0xbc86727e770de68b1060c91f6bb6945c73e10388', 'XNK', 18))
-
-    await controller.detectNewTokens()
-    assert.deepEqual(preferences.store.getState().tokens, [{ address: '0x0d262e5dc4a06a0f1c90ce79c7a60c09dfc884e4', decimals: 8, symbol: 'J8T' },
-      { address: '0xbc86727e770de68b1060c91f6bb6945c73e10388', decimals: 18, symbol: 'XNK' }])
   })
 
   it('should trigger detect new tokens when change address', async function () {
+    sandbox.useFakeTimers()
+    const controller = new DetectTokensController({ preferences, network, keyringMemStore })
     controller.isOpen = true
     controller.isUnlocked = true
     const stub = sandbox.stub(controller, 'detectNewTokens')
@@ -120,6 +132,8 @@ describe('DetectTokensController', function () {
   })
 
   it('should trigger detect new tokens when submit password', async function () {
+    sandbox.useFakeTimers()
+    const controller = new DetectTokensController({ preferences, network, keyringMemStore })
     controller.isOpen = true
     controller.selectedAddress = '0x0'
     const stub = sandbox.stub(controller, 'detectNewTokens')
@@ -127,14 +141,26 @@ describe('DetectTokensController', function () {
     sandbox.assert.called(stub)
   })
 
-  it('should not trigger detect new tokens when not open or not unlocked', async function () {
+  it('should not trigger detect new tokens when not unlocked', async function () {
+    const clock = sandbox.useFakeTimers()
+    network.setProviderType(MAINNET)
+    const controller = new DetectTokensController({ preferences, network, keyringMemStore })
     controller.isOpen = true
     controller.isUnlocked = false
-    const stub = sandbox.stub(controller, 'detectTokenBalance')
+    const stub = sandbox.stub(controller, '_getTokenBalances')
     clock.tick(180000)
     sandbox.assert.notCalled(stub)
+  })
+
+  it('should not trigger detect new tokens when not open', async function () {
+    const clock = sandbox.useFakeTimers()
+    network.setProviderType(MAINNET)
+    const controller = new DetectTokensController({ preferences, network, keyringMemStore })
+    // trigger state update from preferences controller
+    await preferences.setSelectedAddress('0xbc86727e770de68b1060c91f6bb6945c73e10388')
     controller.isOpen = false
     controller.isUnlocked = true
+    const stub = sandbox.stub(controller, '_getTokenBalances')
     clock.tick(180000)
     sandbox.assert.notCalled(stub)
   })