Merge pull request #888 from MetaMask/i844-AsyncKeyrings

Modify Keyring protocol to always return promises for public methods

app/scripts/keyring-controller.js

@@ -98,17 +98,20 @@ module.exports = class KeyringController extends EventEmitter {
       this.addNewKeyring('HD Key Tree', {
         mnemonic: seed,
         numberOfAccounts: 1,
-      }, (err) => {
-        if (err) return cb(err)
+      }).then(() => {
         const firstKeyring = this.keyrings[0]
-        const accounts = firstKeyring.getAccounts()
+        return firstKeyring.getAccounts()
+      })
+      .then((accounts) => {
         const firstAccount = accounts[0]
         const hexAccount = normalize(firstAccount)
         this.configManager.setSelectedAccount(hexAccount)
         this.setupAccounts(accounts)
-
-        this.emit('update')
-        cb()
+        this.persistAllKeyrings()
+        .then(() => {
+          this.emit('update')
+          cb(err, this.getState())
+        })
       })
     })
   }
@@ -117,14 +120,20 @@ module.exports = class KeyringController extends EventEmitter {
     const shouldMigrate = !!this.configManager.getWallet() && !this.configManager.getVault()
     return this.idStoreMigrator.migratedVaultForPassword(password)
     .then((serialized) => {
+      this.password = password
+
       if (serialized && shouldMigrate) {
-        this.password = password
-        const keyring = this.restoreKeyring(serialized)
-        this.keyrings.push(keyring)
-        this.configManager.setSelectedAccount(keyring.getAccounts()[0])
-        return this.persistAllKeyrings()
+        return this.restoreKeyring(serialized)
+        .then((keyring) => {
+          return keyring.getAccounts()
+        })
+        .then((accounts) => {
+          this.configManager.setSelectedAccount(accounts[0])
+          return this.persistAllKeyrings()
+        })
+      } else {
+        return Promise.resolve()
       }
-      return
     })
   }
 
@@ -145,17 +154,20 @@ module.exports = class KeyringController extends EventEmitter {
   createFirstKeyTree (password, cb) {
     this.clearKeyrings()
     this.addNewKeyring('HD Key Tree', {numberOfAccounts: 1}, (err) => {
-      const accounts = this.keyrings[0].getAccounts()
-      const firstAccount = accounts[0]
-      const hexAccount = normalize(firstAccount)
-      this.configManager.setSelectedAccount(firstAccount)
+      if (err) return cb(err)
+      this.keyrings[0].getAccounts()
+      .then((accounts) => {
+        const firstAccount = accounts[0]
+        const hexAccount = normalize(firstAccount)
+        this.configManager.setSelectedAccount(firstAccount)
 
-      this.placeSeedWords()
-      this.emit('newAccount', hexAccount)
-      this.setupAccounts(accounts)
-      this.persistAllKeyrings()
+        this.placeSeedWords()
+        this.emit('newAccount', hexAccount)
+        this.setupAccounts(accounts)
+        return this.persistAllKeyrings()
+      })
       .then(() => {
-        cb(err)
+        cb()
       })
       .catch((reason) => {
         cb(reason)
@@ -165,13 +177,17 @@ module.exports = class KeyringController extends EventEmitter {
 
   placeSeedWords (cb) {
     const firstKeyring = this.keyrings[0]
-    const seedWords = firstKeyring.serialize().mnemonic
-    this.configManager.setSeedWords(seedWords)
+    firstKeyring.serialize()
+    .then((serialized) => {
+      const seedWords = serialized.mnemonic
+      this.configManager.setSeedWords(seedWords)
 
-    if (cb) {
-      cb()
-    }
-    this.emit('update')
+      if (cb) {
+        cb()
+      }
+
+      this.emit('update')
+    })
   }
 
   submitPassword (password, cb) {
@@ -193,24 +209,35 @@ module.exports = class KeyringController extends EventEmitter {
   addNewKeyring (type, opts, cb) {
     const Keyring = this.getKeyringClassForType(type)
     const keyring = new Keyring(opts)
-    const accounts = keyring.getAccounts()
-
-    this.keyrings.push(keyring)
-    this.setupAccounts(accounts)
-    this.persistAllKeyrings()
-    .then(() => {
-      cb()
+    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) => {
-      cb(reason)
+      if (cb) {
+        cb(reason)
+      }
+      return reason
     })
   }
 
   addNewAccount (keyRingNum = 0, cb) {
     const ring = this.keyrings[keyRingNum]
-    const accounts = ring.addAccounts(1)
-    this.setupAccounts(accounts)
-    this.persistAllKeyrings()
+    return ring.addAccounts(1)
+    .then((accounts) => {
+      return this.setupAccounts(accounts)
+    })
+    .then(() => {
+      return this.persistAllKeyrings()
+    })
     .then(() => {
       cb()
     })
@@ -220,9 +247,12 @@ module.exports = class KeyringController extends EventEmitter {
   }
 
   setupAccounts (accounts) {
-    var arr = accounts || this.getAccounts()
-    arr.forEach((account) => {
-      this.getBalanceAndNickname(account)
+    return this.getAccounts()
+    .then((loadedAccounts) => {
+      const arr = accounts || loadedAccounts
+      arr.forEach((account) => {
+        this.getBalanceAndNickname(account)
+      })
     })
   }
 
@@ -259,13 +289,19 @@ module.exports = class KeyringController extends EventEmitter {
   }
 
   persistAllKeyrings () {
-    const serialized = this.keyrings.map((keyring) => {
-      return {
-        type: keyring.type,
-        data: keyring.serialize(),
-      }
+    return Promise.all(this.keyrings.map((keyring) => {
+      return Promise.all([keyring.type, keyring.serialize()])
+      .then((serializedKeyringArray) => {
+        // Label the output values on each serialized Keyring:
+        return {
+          type: serializedKeyringArray[0],
+          data: serializedKeyringArray[1],
+        }
+      })
+    }))
+    .then((serializedKeyrings) => {
+      return this.encryptor.encrypt(this.password, serializedKeyrings)
     })
-    return this.encryptor.encrypt(this.password, serialized)
     .then((encryptedString) => {
       this.configManager.setVault(encryptedString)
       return true
@@ -286,13 +322,15 @@ module.exports = class KeyringController extends EventEmitter {
     const { type, data } = serialized
     const Keyring = this.getKeyringClassForType(type)
     const keyring = new Keyring()
-    keyring.deserialize(data)
-
-    const accounts = keyring.getAccounts()
-    this.setupAccounts(accounts)
-
-    this.keyrings.push(keyring)
-    return keyring
+    return keyring.deserialize(data)
+    .then(() => {
+      return keyring.getAccounts()
+    })
+    .then((accounts) => {
+      this.setupAccounts(accounts)
+      this.keyrings.push(keyring)
+      return keyring
+    })
   }
 
   getKeyringClassForType (type) {
@@ -308,10 +346,10 @@ module.exports = class KeyringController extends EventEmitter {
 
   getAccounts () {
     const keyrings = this.keyrings || []
-    return keyrings.map(kr => kr.getAccounts())
+    return Promise.all(keyrings.map(kr => kr.getAccounts())
     .reduce((res, arr) => {
       return res.concat(arr)
-    }, [])
+    }, []))
   }
 
   setSelectedAccount (address, cb) {
@@ -514,18 +552,19 @@ module.exports = class KeyringController extends EventEmitter {
       txParams.gasLimit = normalize(txParams.gasLimit || txParams.gas)
       txParams.nonce = normalize(txParams.nonce)
 
-      let tx = new Transaction(txParams)
-      tx = keyring.signTransaction(address, tx)
-
-      // Add the tx hash to the persisted meta-tx object
-      var txHash = ethUtil.bufferToHex(tx.hash())
-      var metaTx = this.configManager.getTx(txParams.metamaskId)
-      metaTx.hash = txHash
-      this.configManager.updateTx(metaTx)
-
-      // return raw serialized tx
-      var rawTx = ethUtil.bufferToHex(tx.serialize())
-      cb(null, rawTx)
+      const tx = new Transaction(txParams)
+      keyring.signTransaction(address, tx)
+      .then((tx) => {
+        // Add the tx hash to the persisted meta-tx object
+        var txHash = ethUtil.bufferToHex(tx.hash())
+        var metaTx = this.configManager.getTx(txParams.metamaskId)
+        metaTx.hash = txHash
+        this.configManager.updateTx(metaTx)
+
+        // return raw serialized tx
+        var rawTx = ethUtil.bufferToHex(tx.serialize())
+        cb(null, rawTx)
+      })
     } catch (e) {
       cb(e)
     }
@@ -535,8 +574,11 @@ module.exports = class KeyringController extends EventEmitter {
     try {
       const keyring = this.getKeyringForAccount(msgParams.from)
       const address = normalize(msgParams.from)
-      const rawSig = keyring.signMessage(address, msgParams.data)
-      cb(null, rawSig)
+      return keyring.signMessage(address, msgParams.data)
+      .then((rawSig) => {
+        cb(null, rawSig)
+        return rawSig
+      })
     } catch (e) {
       cb(e)
     }
@@ -567,10 +609,14 @@ module.exports = class KeyringController extends EventEmitter {
   exportAccount (address, cb) {
     try {
       const keyring = this.getKeyringForAccount(address)
-      const privateKey = keyring.exportAccount(normalize(address))
-      cb(null, privateKey)
+      return keyring.exportAccount(normalize(address))
+      .then((privateKey) => {
+        cb(null, privateKey)
+        return privateKey
+      })
     } catch (e) {
       cb(e)
+      return Promise.reject(e)
     }
   }
 

app/scripts/keyrings/hd.js

@@ -21,10 +21,10 @@ class HdKeyring extends EventEmitter {
   }
 
   serialize () {
-    return {
+    return Promise.resolve({
       mnemonic: this.mnemonic,
       numberOfAccounts: this.wallets.length,
-    }
+    })
   }
 
   deserialize (opts = {}) {
@@ -40,6 +40,8 @@ class HdKeyring extends EventEmitter {
     if ('numberOfAccounts' in opts) {
       this.addAccounts(opts.numberOfAccounts)
     }
+
+    return Promise.resolve()
   }
 
   addAccounts (numberOfAccounts = 1) {
@@ -55,11 +57,12 @@ class HdKeyring extends EventEmitter {
       newWallets.push(wallet)
       this.wallets.push(wallet)
     }
-    return newWallets.map(w => w.getAddress().toString('hex'))
+    const hexWallets = newWallets.map(w => w.getAddress().toString('hex'))
+    return Promise.resolve(hexWallets)
   }
 
   getAccounts () {
-    return this.wallets.map(w => w.getAddress().toString('hex'))
+    return Promise.resolve(this.wallets.map(w => w.getAddress().toString('hex')))
   }
 
   // tx is an instance of the ethereumjs-transaction class.
@@ -67,7 +70,7 @@ class HdKeyring extends EventEmitter {
     const wallet = this._getWalletForAccount(address)
     var privKey = wallet.getPrivateKey()
     tx.sign(privKey)
-    return tx
+    return Promise.resolve(tx)
   }
 
   // For eth_sign, we need to sign transactions:
@@ -77,12 +80,12 @@ class HdKeyring extends EventEmitter {
     var privKey = wallet.getPrivateKey()
     var msgSig = ethUtil.ecsign(new Buffer(message, 'hex'), privKey)
     var rawMsgSig = ethUtil.bufferToHex(sigUtil.concatSig(msgSig.v, msgSig.r, msgSig.s))
-    return rawMsgSig
+    return Promise.resolve(rawMsgSig)
   }
 
   exportAccount (address) {
     const wallet = this._getWalletForAccount(address)
-    return wallet.getPrivateKey().toString('hex')
+    return Promise.resolve(wallet.getPrivateKey().toString('hex'))
   }
 
 

app/scripts/keyrings/simple.js

@@ -8,10 +8,6 @@ class SimpleKeyring extends EventEmitter {
 
   /* PUBLIC METHODS */
 
-  static type () {
-    return type
-  }
-
   constructor (opts) {
     super()
     this.type = type
@@ -20,7 +16,7 @@ class SimpleKeyring extends EventEmitter {
   }
 
   serialize () {
-    return this.wallets.map(w => w.getPrivateKey().toString('hex'))
+    return Promise.resolve(this.wallets.map(w => w.getPrivateKey().toString('hex')))
   }
 
   deserialize (wallets = []) {
@@ -29,6 +25,7 @@ class SimpleKeyring extends EventEmitter {
       const wallet = Wallet.fromPrivateKey(b)
       return wallet
     })
+    return Promise.resolve()
   }
 
   addAccounts (n = 1) {
@@ -37,11 +34,12 @@ class SimpleKeyring extends EventEmitter {
       newWallets.push(Wallet.generate())
     }
     this.wallets = this.wallets.concat(newWallets)
-    return newWallets.map(w => w.getAddress().toString('hex'))
+    const hexWallets = newWallets.map(w => w.getAddress().toString('hex'))
+    return Promise.resolve(hexWallets)
   }
 
   getAccounts () {
-    return this.wallets.map(w => w.getAddress().toString('hex'))
+    return Promise.resolve(this.wallets.map(w => w.getAddress().toString('hex')))
   }
 
   // tx is an instance of the ethereumjs-transaction class.
@@ -49,7 +47,7 @@ class SimpleKeyring extends EventEmitter {
     const wallet = this._getWalletForAccount(address)
     var privKey = wallet.getPrivateKey()
     tx.sign(privKey)
-    return tx
+    return Promise.resolve(tx)
   }
 
   // For eth_sign, we need to sign transactions:
@@ -59,12 +57,12 @@ class SimpleKeyring extends EventEmitter {
     var privKey = wallet.getPrivateKey()
     var msgSig = ethUtil.ecsign(new Buffer(message, 'hex'), privKey)
     var rawMsgSig = ethUtil.bufferToHex(sigUtil.concatSig(msgSig.v, msgSig.r, msgSig.s))
-    return rawMsgSig
+    return Promise.resolve(rawMsgSig)
   }
 
   exportAccount (address) {
     const wallet = this._getWalletForAccount(address)
-    return wallet.getPrivateKey().toString('hex')
+    return Promise.resolve(wallet.getPrivateKey().toString('hex'))
   }
 
 

test/unit/keyring-controller-test.js

@@ -61,7 +61,7 @@ describe('KeyringController', function() {
 
   describe('#restoreKeyring', function() {
 
-    it(`should pass a keyring's serialized data back to the correct type.`, function() {
+    it(`should pass a keyring's serialized data back to the correct type.`, function(done) {
       const mockSerialized = {
         type: 'HD Key Tree',
         data: {
@@ -74,12 +74,21 @@ describe('KeyringController', function() {
       mock.expects('getBalanceAndNickname')
       .exactly(1)
 
-      var keyring = keyringController.restoreKeyring(mockSerialized)
-      assert.equal(keyring.wallets.length, 1, 'one wallet restored')
-      assert.equal(keyring.getAccounts()[0], addresses[0])
-      mock.verify()
+      keyringController.restoreKeyring(mockSerialized)
+      .then((keyring) => {
+        assert.equal(keyring.wallets.length, 1, 'one wallet restored')
+        return keyring.getAccounts()
+      })
+      .then((accounts) => {
+        assert.equal(accounts[0], addresses[0])
+        mock.verify()
+        done()
+      })
+      .catch((reason) => {
+        assert.ifError(reason)
+        done()
+      })
     })
-
   })
 
   describe('#migrateOldVaultIfAny', function() {
@@ -92,6 +101,7 @@ describe('KeyringController', function() {
       })
       .catch((reason) => {
         assert.ifError(reason)
+        done()
       })
     })
   })
@@ -110,15 +120,17 @@ describe('KeyringController', function() {
   })
 
   describe('#saveAccountLabel', function() {
-    it ('sets the nickname', function() {
+    it ('sets the nickname', function(done) {
       const account = addresses[0]
       var nick = 'Test nickname'
       keyringController.identities[ethUtil.addHexPrefix(account)] = {}
-      const label = keyringController.saveAccountLabel(account, nick)
-      assert.equal(label, nick)
-
-      const persisted = keyringController.configManager.nicknameForWallet(account)
-      assert.equal(persisted, nick)
+      keyringController.saveAccountLabel(account, nick, (err, label) => {
+        assert.ifError(err)
+        assert.equal(label, nick)
+        const persisted = keyringController.configManager.nicknameForWallet(account)
+        assert.equal(persisted, nick)
+        done()
+      })
     })
 
     this.timeout(10000)
@@ -126,9 +138,7 @@ describe('KeyringController', function() {
       const account = addresses[0]
       var nick = 'Test nickname'
       keyringController.configManager.setNicknameForWallet(account, nick)
-      console.log('calling to restore')
       keyringController.createNewVaultAndRestore(password, seedWords, (err, state) => {
-        console.dir({err})
         assert.ifError(err)
 
         const identity = keyringController.identities['0x' + account]
@@ -143,12 +153,15 @@ describe('KeyringController', function() {
   describe('#getAccounts', function() {
     it('returns the result of getAccounts for each keyring', function() {
       keyringController.keyrings = [
-        { getAccounts() { return [1,2,3] } },
-        { getAccounts() { return [4,5,6] } },
+        { getAccounts() { return Promise.resolve([1,2,3]) } },
+        { getAccounts() { return Promise.resolve([4,5,6]) } },
       ]
 
-      const result = keyringController.getAccounts()
-      assert.deepEqual(result, [1,2,3,4,5,6])
+      keyringController.getAccounts()
+      .then((result) => {
+        assert.deepEqual(result, [1,2,3,4,5,6])
+        done()
+      })
     })
   })
 

test/unit/keyrings/hd-test.js

@@ -16,15 +16,18 @@ describe('hd-keyring', function() {
     keyring = new HdKeyring()
   })
 
-  describe('constructor', function() {
+  describe('constructor', function(done) {
     keyring = new HdKeyring({
       mnemonic: sampleMnemonic,
       numberOfAccounts: 2,
     })
 
     const accounts = keyring.getAccounts()
-    assert.equal(accounts[0], firstAcct)
-    assert.equal(accounts[1], secondAcct)
+    .then((accounts) => {
+      assert.equal(accounts[0], firstAcct)
+      assert.equal(accounts[1], secondAcct)
+      done()
+    })
   })
 
   describe('Keyring.type', function() {
@@ -44,49 +47,64 @@ describe('hd-keyring', function() {
 
   describe('#serialize empty wallets.', function() {
     it('serializes a new mnemonic', function() {
-      const output = keyring.serialize()
-      assert.equal(output.numberOfAccounts, 0)
-      assert.equal(output.mnemonic, null)
+      keyring.serialize()
+      .then((output) => {
+        assert.equal(output.numberOfAccounts, 0)
+        assert.equal(output.mnemonic, null)
+      })
     })
   })
 
   describe('#deserialize a private key', function() {
-    it('serializes what it deserializes', function() {
+    it('serializes what it deserializes', function(done) {
+      console.log('deserializing ' + sampleMnemonic)
       keyring.deserialize({
         mnemonic: sampleMnemonic,
         numberOfAccounts: 1
       })
-      assert.equal(keyring.wallets.length, 1, 'restores two accounts')
-      keyring.addAccounts(1)
-
-      const accounts = keyring.getAccounts()
-      assert.equal(accounts[0], firstAcct)
-      assert.equal(accounts[1], secondAcct)
-      assert.equal(accounts.length, 2)
-
-      const serialized = keyring.serialize()
-      assert.equal(serialized.mnemonic, sampleMnemonic)
+      .then(() => {
+        console.dir(keyring)
+        assert.equal(keyring.wallets.length, 1, 'restores two accounts')
+        return keyring.addAccounts(1)
+      }).then(() => {
+        return keyring.getAccounts()
+      }).then((accounts) => {
+        assert.equal(accounts[0], firstAcct)
+        assert.equal(accounts[1], secondAcct)
+        assert.equal(accounts.length, 2)
+
+        return keyring.serialize()
+      }).then((serialized) => {
+        assert.equal(serialized.mnemonic, sampleMnemonic)
+        done()
+      })
     })
   })
 
   describe('#addAccounts', function() {
     describe('with no arguments', function() {
-      it('creates a single wallet', function() {
+      it('creates a single wallet', function(done) {
         keyring.addAccounts()
-        assert.equal(keyring.wallets.length, 1)
+        .then(() => {
+          assert.equal(keyring.wallets.length, 1)
+          done()
+        })
       })
     })
 
     describe('with a numeric argument', function() {
-      it('creates that number of wallets', function() {
+      it('creates that number of wallets', function(done) {
         keyring.addAccounts(3)
-        assert.equal(keyring.wallets.length, 3)
+        .then(() => {
+          assert.equal(keyring.wallets.length, 3)
+          done()
+        })
       })
     })
   })
 
   describe('#getAccounts', function() {
-    it('calls getAddress on each wallet', function() {
+    it('calls getAddress on each wallet', function(done) {
 
       // Push a mock wallet
       const desiredOutput = 'foo'
@@ -101,8 +119,11 @@ describe('hd-keyring', function() {
       })
 
       const output = keyring.getAccounts()
-      assert.equal(output[0], desiredOutput)
-      assert.equal(output.length, 1)
+      .then((output) => {
+        assert.equal(output[0], desiredOutput)
+        assert.equal(output.length, 1)
+        done()
+      })
     })
   })
 })

test/unit/keyrings/simple-test.js

@@ -28,19 +28,23 @@ describe('simple-keyring', function() {
   })
 
   describe('#serialize empty wallets.', function() {
-    it('serializes an empty array', function() {
-      const output = keyring.serialize()
-      assert.deepEqual(output, [])
+    it('serializes an empty array', function(done) {
+      keyring.serialize()
+      .then((output) => {
+        assert.deepEqual(output, [])
+        done()
+      })
     })
   })
 
   describe('#deserialize a private key', function() {
     it('serializes what it deserializes', function() {
       keyring.deserialize([privKeyHex])
-      assert.equal(keyring.wallets.length, 1, 'has one wallet')
-
-      const serialized = keyring.serialize()
-      assert.equal(serialized[0], privKeyHex)
+      .then(() => {
+        assert.equal(keyring.wallets.length, 1, 'has one wallet')
+        const serialized = keyring.serialize()
+        assert.equal(serialized[0], privKeyHex)
+      })
     })
   })
 
@@ -48,20 +52,24 @@ describe('simple-keyring', function() {
     describe('with no arguments', function() {
       it('creates a single wallet', function() {
         keyring.addAccounts()
-        assert.equal(keyring.wallets.length, 1)
+        .then(() => {
+          assert.equal(keyring.wallets.length, 1)
+        })
       })
     })
 
     describe('with a numeric argument', function() {
       it('creates that number of wallets', function() {
         keyring.addAccounts(3)
-        assert.equal(keyring.wallets.length, 3)
+        .then(() => {
+          assert.equal(keyring.wallets.length, 3)
+        })
       })
     })
   })
 
   describe('#getAccounts', function() {
-    it('calls getAddress on each wallet', function() {
+    it('calls getAddress on each wallet', function(done) {
 
       // Push a mock wallet
       const desiredOutput = 'foo'
@@ -75,9 +83,12 @@ describe('simple-keyring', function() {
         }
       })
 
-      const output = keyring.getAccounts()
-      assert.equal(output[0], desiredOutput)
-      assert.equal(output.length, 1)
+      keyring.getAccounts()
+      .then((output) => {
+        assert.equal(output[0], desiredOutput)
+        assert.equal(output.length, 1)
+        done()
+      })
     })
   })
 })