Denodeify most of KeyringController

Mostly Fixes #893

A couple methods cache callbacks, and will require a larger refactor to fully denodeify.

Specifically, our methods involving web3 requests to sign a tx, sign a message, and approve or cancel either of those.

I think we should postpone those until the TxManager refactor, since it will likely handle this response caching itself.

app/scripts/keyring-controller.js

@@ -75,29 +75,29 @@ module.exports = class KeyringController extends EventEmitter {
     this.ethStore = ethStore
   }
 
-  createNewVaultAndKeychain (password, cb) {
+  createNewVaultAndKeychain (password) {
-    this.createNewVault(password, (err) => {
+    return this.createNewVault(password)
-      if (err) return cb(err)
+    .then(this.createFirstKeyTree.bind(this))
-      this.createFirstKeyTree(password, cb)
+    .then(this.fullUpdate.bind(this))
-    })
   }
 
-  createNewVaultAndRestore (password, seed, cb) {
+  createNewVaultAndRestore (password, seed) {
     if (typeof password !== 'string') {
-      return cb('Password must be text.')
+      return Promise.reject('Password must be text.')
     }
 
     if (!bip39.validateMnemonic(seed)) {
-      return cb('Seed phrase is invalid.')
+      return Promise.reject('Seed phrase is invalid.')
     }
 
     this.clearKeyrings()
 
-    this.createNewVault(password, (err) => {
+    return this.createNewVault(password)
-      if (err) return cb(err)
+    .then(() => {
-      this.addNewKeyring('HD Key Tree', {
+      return this.addNewKeyring('HD Key Tree', {
         mnemonic: seed,
         numberOfAccounts: 1,
+      })
     }).then(() => {
       const firstKeyring = this.keyrings[0]
       return firstKeyring.getAccounts()
@@ -106,14 +106,10 @@ module.exports = class KeyringController extends EventEmitter {
       const firstAccount = accounts[0]
       const hexAccount = normalize(firstAccount)
       this.configManager.setSelectedAccount(hexAccount)
-        this.setupAccounts(accounts)
+      return this.setupAccounts(accounts)
-        this.persistAllKeyrings()
-        .then(() => {
-          this.emit('update')
-          cb(err, this.getState())
-        })
-      })
     })
+    .then(this.persistAllKeyrings.bind(this))
+    .then(this.fullUpdate.bind(this))
   }
 
   migrateOldVaultIfAny (password) {
@@ -124,9 +120,7 @@ module.exports = class KeyringController extends EventEmitter {
 
       if (serialized && shouldMigrate) {
         return this.restoreKeyring(serialized)
-        .then((keyring) => {
+        .then(keyring => keyring.getAccounts())
-          return keyring.getAccounts()
-        })
         .then((accounts) => {
           this.configManager.setSelectedAccount(accounts[0])
           return this.persistAllKeyrings()
@@ -137,122 +131,91 @@ module.exports = class KeyringController extends EventEmitter {
     })
   }
 
-  createNewVault (password, cb) {
+  createNewVault (password) {
     return this.migrateOldVaultIfAny(password)
     .then(() => {
       this.password = password
       return this.persistAllKeyrings()
     })
     .then(() => {
-      cb()
+      return password
-    })
-    .catch((err) => {
-      cb(err)
     })
   }
 
-  createFirstKeyTree (password, cb) {
+  createFirstKeyTree (password) {
     this.clearKeyrings()
-    this.addNewKeyring('HD Key Tree', {numberOfAccounts: 1}, (err) => {
+    return this.addNewKeyring('HD Key Tree', {numberOfAccounts: 1})
-      if (err) return cb(err)
+    .then(() => {
-      this.keyrings[0].getAccounts()
+      return this.keyrings[0].getAccounts()
+    })
     .then((accounts) => {
       const firstAccount = accounts[0]
       const hexAccount = normalize(firstAccount)
-        this.configManager.setSelectedAccount(firstAccount)
+      this.configManager.setSelectedAccount(hexAccount)
-
-        this.placeSeedWords()
       this.emit('newAccount', hexAccount)
-        this.setupAccounts(accounts)
+      return this.setupAccounts(accounts)
-        return this.persistAllKeyrings()
+    }).then(() => {
-      })
+      return this.placeSeedWords()
-      .then(() => {
-        cb()
-      })
-      .catch((reason) => {
-        cb(reason)
-      })
     })
+    .then(this.persistAllKeyrings.bind(this))
   }
 
-  placeSeedWords (cb) {
+  placeSeedWords () {
     const firstKeyring = this.keyrings[0]
-    firstKeyring.serialize()
+    return firstKeyring.serialize()
     .then((serialized) => {
       const seedWords = serialized.mnemonic
       this.configManager.setSeedWords(seedWords)
-
-      if (cb) {
-        cb()
-      }
-
       this.emit('update')
+      return
     })
   }
 
-  submitPassword (password, cb) {
+  submitPassword (password) {
-    this.migrateOldVaultIfAny(password)
+    return this.migrateOldVaultIfAny(password)
     .then(() => {
       return this.unlockKeyrings(password)
     })
     .then((keyrings) => {
       this.keyrings = keyrings
-      this.setupAccounts()
+      return this.setupAccounts()
-      this.emit('update')
-      cb(null, this.getState())
-    })
-    .catch((err) => {
-      cb(err)
     })
+    .then(this.fullUpdate.bind(this))
   }
 
-  addNewKeyring (type, opts, cb) {
+  fullUpdate() {
+    this.emit('update')
+    return Promise.resolve(this.getState())
+  }
+
+  addNewKeyring (type, opts) {
     const Keyring = this.getKeyringClassForType(type)
     const keyring = new Keyring(opts)
     return keyring.getAccounts()
     .then((accounts) => {
       this.keyrings.push(keyring)
       return this.setupAccounts(accounts)
-    }).then(() => {
-      return this.persistAllKeyrings()
-    }).then(() => {
-      if (cb) {
-        cb(null, keyring)
-      }
-      return keyring
     })
-    .catch((reason) => {
+    .then(this.persistAllKeyrings.bind(this))
-      if (cb) {
+    .then(() => {
-        cb(reason)
+      return keyring
-      }
-      return reason
     })
   }
 
-  addNewAccount (keyRingNum = 0, cb) {
+  addNewAccount (keyRingNum = 0) {
     const ring = this.keyrings[keyRingNum]
     return ring.addAccounts(1)
-    .then((accounts) => {
+    .then(this.setupAccounts.bind(this))
-      return this.setupAccounts(accounts)
+    .then(this.persistAllKeyrings.bind(this))
-    })
-    .then(() => {
-      return this.persistAllKeyrings()
-    })
-    .then(() => {
-      cb()
-    })
-    .catch((reason) => {
-      cb(reason)
-    })
   }
 
   setupAccounts (accounts) {
     return this.getAccounts()
     .then((loadedAccounts) => {
       const arr = accounts || loadedAccounts
-      arr.forEach((account) => {
+      return Promise.all(arr.map((account) => {
-        this.getBalanceAndNickname(account)
+        return this.getBalanceAndNickname(account)
-      })
+      }))
     })
   }
 
@@ -261,7 +224,7 @@ module.exports = class KeyringController extends EventEmitter {
   getBalanceAndNickname (account) {
     const address = normalize(account)
     this.ethStore.addAccount(address)
-    this.createNickname(address)
+    return this.createNickname(address)
   }
 
   createNickname (address) {
@@ -276,16 +239,12 @@ module.exports = class KeyringController extends EventEmitter {
     return this.saveAccountLabel(hexAddress, name)
   }
 
-  saveAccountLabel (account, label, cb) {
+  saveAccountLabel (account, label) {
     const address = normalize(account)
     const configManager = this.configManager
     configManager.setNicknameForWallet(address, label)
     this.identities[address].name = label
-    if (cb) {
+    return Promise.resolve(label)
-      cb(null, label)
-    } else {
-      return label
-    }
   }
 
   persistAllKeyrings () {
@@ -327,7 +286,9 @@ module.exports = class KeyringController extends EventEmitter {
       return keyring.getAccounts()
     })
     .then((accounts) => {
-      this.setupAccounts(accounts)
+      return this.setupAccounts(accounts)
+    })
+    .then(() => {
       this.keyrings.push(keyring)
       return keyring
     })
@@ -346,16 +307,18 @@ module.exports = class KeyringController extends EventEmitter {
 
   getAccounts () {
     const keyrings = this.keyrings || []
-    return Promise.all(keyrings.map(kr => kr.getAccounts())
+    return Promise.all(keyrings.map(kr => kr.getAccounts()))
-    .reduce((res, arr) => {
+    .then((keyringArrays) => {
+      return keyringArrays.reduce((res, arr) => {
         return res.concat(arr)
-    }, []))
+      }, [])
+    })
   }
 
-  setSelectedAccount (address, cb) {
+  setSelectedAccount (address) {
     var addr = normalize(address)
     this.configManager.setSelectedAccount(addr)
-    cb(null, addr)
+    Promise.resolve(addr)
   }
 
   addUnconfirmedTransaction (txParams, onTxDoneCb, cb) {
@@ -536,8 +499,8 @@ module.exports = class KeyringController extends EventEmitter {
   signTransaction (txParams, cb) {
     try {
       const address = normalize(txParams.from)
-      const keyring = this.getKeyringForAccount(address)
+      return this.getKeyringForAccount(address)
-
+      .then((keyring) => {
         // Handle gas pricing
         var gasMultiplier = this.configManager.getGasMultiplier() || 1
         var gasPrice = new BN(ethUtil.stripHexPrefix(txParams.gasPrice), 16)
@@ -553,7 +516,8 @@ module.exports = class KeyringController extends EventEmitter {
         txParams.nonce = normalize(txParams.nonce)
 
         const tx = new Transaction(txParams)
-      keyring.signTransaction(address, tx)
+        return keyring.signTransaction(address, tx)
+      })
       .then((tx) => {
         // Add the tx hash to the persisted meta-tx object
         var txHash = ethUtil.bufferToHex(tx.hash())
@@ -572,10 +536,11 @@ module.exports = class KeyringController extends EventEmitter {
 
   signMessage (msgParams, cb) {
     try {
-      const keyring = this.getKeyringForAccount(msgParams.from)
       const address = normalize(msgParams.from)
+      return this.getKeyringForAccount(address)
+      .then((keyring) => {
         return keyring.signMessage(address, msgParams.data)
-      .then((rawSig) => {
+      }).then((rawSig) => {
         cb(null, rawSig)
         return rawSig
       })
@@ -586,10 +551,28 @@ module.exports = class KeyringController extends EventEmitter {
 
   getKeyringForAccount (address) {
     const hexed = normalize(address)
-    return this.keyrings.find((ring) => {
+    return new Promise((resolve, reject) => {
-      return ring.getAccounts()
+
-      .map(normalize)
+      // Get all the keyrings, and associate them with their account list:
-      .includes(hexed)
+      Promise.all(this.keyrings.map((keyring) => {
+        const accounts = keyring.getAccounts()
+        return Promise.all({
+          keyring,
+          accounts,
+        })
+      }))
+
+      // Find the keyring with the matching account and return it:
+      .then((result) => {
+        const match = result.find((candidate) => {
+          return candidate.accounts.map(normalize).includes(hexed)
+        })
+        if (match) {
+          resolve(match.keyring)
+        } else {
+          reject('No keyring found for the requested account.')
+        }
+      })
     })
   }
 
@@ -599,23 +582,19 @@ module.exports = class KeyringController extends EventEmitter {
     }
   }
 
-  setLocked (cb) {
+  setLocked () {
     this.password = null
     this.keyrings = []
-    this.emit('update')
+    return this.fullUpdate()
-    cb()
   }
 
-  exportAccount (address, cb) {
+  exportAccount (address) {
     try {
-      const keyring = this.getKeyringForAccount(address)
+      return this.getKeyringForAccount(address)
+      .then((keyring) => {
         return keyring.exportAccount(normalize(address))
-      .then((privateKey) => {
-        cb(null, privateKey)
-        return privateKey
       })
     } catch (e) {
-      cb(e)
       return Promise.reject(e)
     }
   }
@@ -627,9 +606,9 @@ module.exports = class KeyringController extends EventEmitter {
     return ethUtil.addHexPrefix(correct.toString(16))
   }
 
-  clearSeedWordCache (cb) {
+  clearSeedWordCache () {
     this.configManager.setSeedWords(null)
-    cb(null, this.configManager.getSelectedAccount())
+    return Promise.resolve(this.configManager.getSelectedAccount())
   }
 
   clearKeyrings () {

app/scripts/lib/nodeify.js

@@ -0,0 +1,59 @@
+/* NODEIFY
+ * Modified from original npm package "nodeify"
+ * https://github.com/then/nodeify
+ *
+ * Removed Promise dependency, to only support
+ * native Promises and reduce bundle size.
+ */
+
+var isPromise = require('is-promise')
+
+var nextTick
+if (typeof setImmediate === 'function') nextTick = setImmediate
+else if (typeof process === 'object' && process && process.nextTick) nextTick = process.nextTick
+else nextTick = function (cb) { setTimeout(cb, 0) }
+
+module.exports = nodeify
+function nodeify(promise, cb) {
+  if (typeof cb !== 'function') return promise
+  return promise
+    .then(function (res) {
+      nextTick(function () {
+        cb(null, res)
+      })
+    }, function (err) {
+      nextTick(function () {
+        cb(err)
+      })
+    })
+}
+function nodeifyThis(cb) {
+  return nodeify(this, cb)
+}
+
+nodeify.extend = extend
+nodeify.Promise = NodeifyPromise
+
+function extend(prom) {
+  if (prom && isPromise(prom)) {
+    prom.nodeify = nodeifyThis
+    var then = prom.then
+    prom.then = function () {
+      return extend(then.apply(this, arguments))
+    }
+    return prom
+  } else if (typeof prom === 'function') {
+    prom.prototype.nodeify = nodeifyThis
+  }
+}
+
+function NodeifyPromise(fn) {
+  if (!(this instanceof NodeifyPromise)) {
+    return new NodeifyPromise(fn)
+  }
+  Promise.call(this, fn)
+  extend(this)
+}
+
+NodeifyPromise.prototype = Object.create(Promise.prototype)
+NodeifyPromise.prototype.constructor = NodeifyPromise

app/scripts/metamask-controller.js

@@ -8,6 +8,7 @@ const Web3 = require('web3')
 const ConfigManager = require('./lib/config-manager')
 const extension = require('./lib/extension')
 const autoFaucet = require('./lib/auto-faucet')
+const nodeify = require('./lib/nodeify')
 
 
 module.exports = class MetamaskController {
@@ -62,21 +63,24 @@ module.exports = class MetamaskController {
       setGasMultiplier: this.setGasMultiplier.bind(this),
 
       // forward directly to keyringController
-      placeSeedWords: keyringController.placeSeedWords.bind(keyringController),
+      placeSeedWords: nodeify(keyringController.placeSeedWords.bind(keyringController)),
-      createNewVaultAndKeychain: keyringController.createNewVaultAndKeychain.bind(keyringController),
+      createNewVaultAndKeychain: nodeify(keyringController.createNewVaultAndKeychain.bind(keyringController)),
-      createNewVaultAndRestore: keyringController.createNewVaultAndRestore.bind(keyringController),
+      createNewVaultAndRestore: nodeify(keyringController.createNewVaultAndRestore.bind(keyringController)),
-      clearSeedWordCache: keyringController.clearSeedWordCache.bind(keyringController),
+      clearSeedWordCache: nodeify(keyringController.clearSeedWordCache.bind(keyringController)),
-      addNewKeyring: keyringController.addNewKeyring.bind(keyringController),
+      addNewKeyring: nodeify(keyringController.addNewKeyring.bind(keyringController)),
-      addNewAccount: keyringController.addNewAccount.bind(keyringController),
+      addNewAccount: nodeify(keyringController.addNewAccount.bind(keyringController)),
-      submitPassword: keyringController.submitPassword.bind(keyringController),
+      submitPassword: nodeify(keyringController.submitPassword.bind(keyringController)),
-      setSelectedAccount: keyringController.setSelectedAccount.bind(keyringController),
+      setSelectedAccount: nodeify(keyringController.setSelectedAccount.bind(keyringController)),
+      exportAccount: nodeify(keyringController.exportAccount.bind(keyringController)),
+      saveAccountLabel: nodeify(keyringController.saveAccountLabel.bind(keyringController)),
+      setLocked: nodeify(keyringController.setLocked.bind(keyringController)),
+
+      // signing methods
       approveTransaction: keyringController.approveTransaction.bind(keyringController),
       cancelTransaction: keyringController.cancelTransaction.bind(keyringController),
       signMessage: keyringController.signMessage.bind(keyringController),
       cancelMessage: keyringController.cancelMessage.bind(keyringController),
-      setLocked: keyringController.setLocked.bind(keyringController),
+
-      exportAccount: keyringController.exportAccount.bind(keyringController),
-      saveAccountLabel: keyringController.saveAccountLabel.bind(keyringController),
       // coinbase
       buyEth: this.buyEth.bind(this),
       // shapeshift

package.json

@@ -57,6 +57,7 @@
     "iframe": "^1.0.0",
     "iframe-stream": "^1.0.2",
     "inject-css": "^0.1.1",
+    "is-promise": "^2.1.0",
     "jazzicon": "^1.2.0",
     "menu-droppo": "^1.1.0",
     "metamask-logo": "^2.1.2",

test/integration/lib/keyring-controller-test.js

@@ -38,8 +38,8 @@ QUnit.test('keyringController:isInitialized', function (assert) {
 QUnit.test('keyringController:submitPassword', function (assert) {
   var done = assert.async()
 
-  this.keyringController.submitPassword(PASSWORD, (err, state) => {
+  this.keyringController.submitPassword(PASSWORD)
-    assert.notOk(err)
+  .then((state) => {
     assert.ok(state.identities[FIRST_ADDRESS])
     done()
   })
@@ -49,9 +49,14 @@ QUnit.test('keyringController:setLocked', function (assert) {
   var done = assert.async()
   var self = this
 
-  this.keyringController.setLocked(function(err) {
+  this.keyringController.setLocked()
+  .then(function() {
     assert.notOk(self.keyringController.password, 'password should be deallocated')
     assert.deepEqual(self.keyringController.keyrings, [], 'keyrings should be deallocated')
     done()
   })
+  .catch((reason) => {
+    assert.ifError(reason)
+    done()
+  })
 })

test/unit/keyring-controller-test.js

@@ -32,8 +32,8 @@ describe('KeyringController', function() {
     // Browser crypto is tested in the integration test suite.
     keyringController.encryptor = mockEncryptor
 
-    keyringController.createNewVaultAndKeychain(password, function (err, newState) {
+    keyringController.createNewVaultAndKeychain(password)
-      assert.ifError(err)
+    .then(function (newState) {
       state = newState
       done()
     })
@@ -50,12 +50,16 @@ describe('KeyringController', function() {
     it('should set a vault on the configManager', function(done) {
       keyringController.configManager.setVault(null)
       assert(!keyringController.configManager.getVault(), 'no previous vault')
-      keyringController.createNewVaultAndKeychain(password, (err, state) => {
+      keyringController.createNewVaultAndKeychain(password)
-        assert.ifError(err)
+      .then(() => {
         const vault = keyringController.configManager.getVault()
         assert(vault, 'vault created')
         done()
       })
+      .catch((reason) => {
+        assert.ifError(reason)
+        done()
+      })
     })
   })
 
@@ -124,13 +128,17 @@ describe('KeyringController', function() {
       const account = addresses[0]
       var nick = 'Test nickname'
       keyringController.identities[ethUtil.addHexPrefix(account)] = {}
-      keyringController.saveAccountLabel(account, nick, (err, label) => {
+      keyringController.saveAccountLabel(account, nick)
-        assert.ifError(err)
+      .then((label) => {
         assert.equal(label, nick)
         const persisted = keyringController.configManager.nicknameForWallet(account)
         assert.equal(persisted, nick)
         done()
       })
+      .catch((reason) => {
+        assert.ifError(reason)
+        done()
+      })
     })
 
     this.timeout(10000)
@@ -138,8 +146,8 @@ describe('KeyringController', function() {
       const account = addresses[0]
       var nick = 'Test nickname'
       keyringController.configManager.setNicknameForWallet(account, nick)
-      keyringController.createNewVaultAndRestore(password, seedWords, (err, state) => {
+      keyringController.createNewVaultAndRestore(password, seedWords)
-        assert.ifError(err)
+      .then((state) => {
 
         const identity = keyringController.identities['0x' + account]
         assert.equal(identity.name, nick)
@@ -147,6 +155,10 @@ describe('KeyringController', function() {
         assert(accounts)
         done()
       })
+      .catch((reason) => {
+        assert.ifError(reason)
+        done()
+      })
     })
   })
 

test/unit/keyrings/hd-test.js

@@ -57,13 +57,11 @@ describe('hd-keyring', function() {
 
   describe('#deserialize a private key', function() {
     it('serializes what it deserializes', function(done) {
-      console.log('deserializing ' + sampleMnemonic)
       keyring.deserialize({
         mnemonic: sampleMnemonic,
         numberOfAccounts: 1
       })
       .then(() => {
-        console.dir(keyring)
         assert.equal(keyring.wallets.length, 1, 'restores two accounts')
         return keyring.addAccounts(1)
       }).then(() => {