From 80e76b45ee67900b5a60da1ddcd8b310f1e92413 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Mon, 28 Nov 2016 12:43:44 -0800 Subject: [PATCH] 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 | 271 ++++++++---------- app/scripts/lib/nodeify.js | 59 ++++ app/scripts/metamask-controller.js | 26 +- package.json | 1 + .../lib/keyring-controller-test.js | 11 +- test/unit/keyring-controller-test.js | 28 +- test/unit/keyrings/hd-test.js | 2 - 7 files changed, 228 insertions(+), 170 deletions(-) create mode 100644 app/scripts/lib/nodeify.js diff --git a/app/scripts/keyring-controller.js b/app/scripts/keyring-controller.js index 4981c49df..5e6b0acbb 100644 --- a/app/scripts/keyring-controller.js +++ b/app/scripts/keyring-controller.js @@ -75,45 +75,41 @@ module.exports = class KeyringController extends EventEmitter { this.ethStore = ethStore } - createNewVaultAndKeychain (password, cb) { - this.createNewVault(password, (err) => { - if (err) return cb(err) - this.createFirstKeyTree(password, cb) - }) + createNewVaultAndKeychain (password) { + return this.createNewVault(password) + .then(this.createFirstKeyTree.bind(this)) + .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) => { - if (err) return cb(err) - this.addNewKeyring('HD Key Tree', { + return this.createNewVault(password) + .then(() => { + return this.addNewKeyring('HD Key Tree', { mnemonic: seed, numberOfAccounts: 1, - }).then(() => { - const firstKeyring = this.keyrings[0] - return firstKeyring.getAccounts() - }) - .then((accounts) => { - const firstAccount = accounts[0] - const hexAccount = normalize(firstAccount) - this.configManager.setSelectedAccount(hexAccount) - this.setupAccounts(accounts) - this.persistAllKeyrings() - .then(() => { - this.emit('update') - cb(err, this.getState()) - }) }) + }).then(() => { + const firstKeyring = this.keyrings[0] + return firstKeyring.getAccounts() }) + .then((accounts) => { + const firstAccount = accounts[0] + const hexAccount = normalize(firstAccount) + this.configManager.setSelectedAccount(hexAccount) + return this.setupAccounts(accounts) + }) + .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) => { - return keyring.getAccounts() - }) + .then(keyring => 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() - }) - .catch((err) => { - cb(err) + return password }) } - createFirstKeyTree (password, cb) { + createFirstKeyTree (password) { this.clearKeyrings() - this.addNewKeyring('HD Key Tree', {numberOfAccounts: 1}, (err) => { - 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) - return this.persistAllKeyrings() - }) - .then(() => { - cb() - }) - .catch((reason) => { - cb(reason) - }) + return this.addNewKeyring('HD Key Tree', {numberOfAccounts: 1}) + .then(() => { + return this.keyrings[0].getAccounts() }) + .then((accounts) => { + const firstAccount = accounts[0] + const hexAccount = normalize(firstAccount) + this.configManager.setSelectedAccount(hexAccount) + this.emit('newAccount', hexAccount) + return this.setupAccounts(accounts) + }).then(() => { + return this.placeSeedWords() + }) + .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) { - this.migrateOldVaultIfAny(password) + submitPassword (password) { + return this.migrateOldVaultIfAny(password) .then(() => { return this.unlockKeyrings(password) }) .then((keyrings) => { this.keyrings = keyrings - this.setupAccounts() - this.emit('update') - cb(null, this.getState()) - }) - .catch((err) => { - cb(err) + return this.setupAccounts() }) + .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) => { - if (cb) { - cb(reason) - } - return reason + .then(this.persistAllKeyrings.bind(this)) + .then(() => { + return keyring }) } - addNewAccount (keyRingNum = 0, cb) { + addNewAccount (keyRingNum = 0) { const ring = this.keyrings[keyRingNum] return ring.addAccounts(1) - .then((accounts) => { - return this.setupAccounts(accounts) - }) - .then(() => { - return this.persistAllKeyrings() - }) - .then(() => { - cb() - }) - .catch((reason) => { - cb(reason) - }) + .then(this.setupAccounts.bind(this)) + .then(this.persistAllKeyrings.bind(this)) } setupAccounts (accounts) { return this.getAccounts() .then((loadedAccounts) => { const arr = accounts || loadedAccounts - arr.forEach((account) => { - this.getBalanceAndNickname(account) - }) + return Promise.all(arr.map((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) { - cb(null, label) - } else { - return label - } + return Promise.resolve(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()) - .reduce((res, arr) => { - return res.concat(arr) - }, [])) + return Promise.all(keyrings.map(kr => kr.getAccounts())) + .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,24 +499,25 @@ 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) + gasPrice = gasPrice.mul(new BN(gasMultiplier * 100, 10)).div(new BN(100, 10)) + txParams.gasPrice = ethUtil.intToHex(gasPrice.toNumber()) - // Handle gas pricing - var gasMultiplier = this.configManager.getGasMultiplier() || 1 - var gasPrice = new BN(ethUtil.stripHexPrefix(txParams.gasPrice), 16) - gasPrice = gasPrice.mul(new BN(gasMultiplier * 100, 10)).div(new BN(100, 10)) - txParams.gasPrice = ethUtil.intToHex(gasPrice.toNumber()) + // normalize values + txParams.to = normalize(txParams.to) + txParams.from = normalize(txParams.from) + txParams.value = normalize(txParams.value) + txParams.data = normalize(txParams.data) + txParams.gasLimit = normalize(txParams.gasLimit || txParams.gas) + txParams.nonce = normalize(txParams.nonce) - // normalize values - txParams.to = normalize(txParams.to) - txParams.from = normalize(txParams.from) - txParams.value = normalize(txParams.value) - txParams.data = normalize(txParams.data) - txParams.gasLimit = normalize(txParams.gasLimit || txParams.gas) - txParams.nonce = normalize(txParams.nonce) - - const tx = new Transaction(txParams) - keyring.signTransaction(address, tx) + const tx = new Transaction(txParams) + 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 keyring.signMessage(address, msgParams.data) - .then((rawSig) => { + return this.getKeyringForAccount(address) + .then((keyring) => { + return keyring.signMessage(address, msgParams.data) + }).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 ring.getAccounts() - .map(normalize) - .includes(hexed) + return new Promise((resolve, reject) => { + + // Get all the keyrings, and associate them with their account list: + 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') - cb() + return this.fullUpdate() } - exportAccount (address, cb) { + exportAccount (address) { try { - const keyring = this.getKeyringForAccount(address) - return keyring.exportAccount(normalize(address)) - .then((privateKey) => { - cb(null, privateKey) - return privateKey + return this.getKeyringForAccount(address) + .then((keyring) => { + return keyring.exportAccount(normalize(address)) }) } 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 () { diff --git a/app/scripts/lib/nodeify.js b/app/scripts/lib/nodeify.js new file mode 100644 index 000000000..f48df34ef --- /dev/null +++ b/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 diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 701046e76..5bcb5b477 100644 --- a/app/scripts/metamask-controller.js +++ b/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), - createNewVaultAndKeychain: keyringController.createNewVaultAndKeychain.bind(keyringController), - createNewVaultAndRestore: keyringController.createNewVaultAndRestore.bind(keyringController), - clearSeedWordCache: keyringController.clearSeedWordCache.bind(keyringController), - addNewKeyring: keyringController.addNewKeyring.bind(keyringController), - addNewAccount: keyringController.addNewAccount.bind(keyringController), - submitPassword: keyringController.submitPassword.bind(keyringController), - setSelectedAccount: keyringController.setSelectedAccount.bind(keyringController), + placeSeedWords: nodeify(keyringController.placeSeedWords.bind(keyringController)), + createNewVaultAndKeychain: nodeify(keyringController.createNewVaultAndKeychain.bind(keyringController)), + createNewVaultAndRestore: nodeify(keyringController.createNewVaultAndRestore.bind(keyringController)), + clearSeedWordCache: nodeify(keyringController.clearSeedWordCache.bind(keyringController)), + addNewKeyring: nodeify(keyringController.addNewKeyring.bind(keyringController)), + addNewAccount: nodeify(keyringController.addNewAccount.bind(keyringController)), + submitPassword: nodeify(keyringController.submitPassword.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 diff --git a/package.json b/package.json index c6b389a6c..2d15e9c9e 100644 --- a/package.json +++ b/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", diff --git a/test/integration/lib/keyring-controller-test.js b/test/integration/lib/keyring-controller-test.js index 678744834..ae5ecc578 100644 --- a/test/integration/lib/keyring-controller-test.js +++ b/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) => { - assert.notOk(err) + this.keyringController.submitPassword(PASSWORD) + .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() + }) }) diff --git a/test/unit/keyring-controller-test.js b/test/unit/keyring-controller-test.js index 056e465ca..69a57ef52 100644 --- a/test/unit/keyring-controller-test.js +++ b/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) { - assert.ifError(err) + keyringController.createNewVaultAndKeychain(password) + .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) => { - assert.ifError(err) + keyringController.createNewVaultAndKeychain(password) + .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) => { - assert.ifError(err) + keyringController.saveAccountLabel(account, nick) + .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) => { - assert.ifError(err) + keyringController.createNewVaultAndRestore(password, seedWords) + .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() + }) }) }) diff --git a/test/unit/keyrings/hd-test.js b/test/unit/keyrings/hd-test.js index 2d9e0ffd9..dfc0ec908 100644 --- a/test/unit/keyrings/hd-test.js +++ b/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(() => {