From 80e76b45ee67900b5a60da1ddcd8b310f1e92413 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Mon, 28 Nov 2016 12:43:44 -0800 Subject: [PATCH 1/9] 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 | 275 ++++++++---------- 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, 230 insertions(+), 172 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)) + } + + fullUpdate() { + this.emit('update') + return Promise.resolve(this.getState()) } - addNewKeyring (type, opts, cb) { + 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) - - // 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) - - const tx = new Transaction(txParams) - keyring.signTransaction(address, tx) + 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()) + + // 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) + 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(() => { From 9e764b193517c935fa04d4722357cb48abcfb2a2 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Mon, 28 Nov 2016 17:27:20 -0800 Subject: [PATCH 2/9] Fix nodeify --- app/scripts/lib/nodeify.js | 68 ++++++++------------------------------ test/unit/nodeify-test.js | 22 ++++++++++++ 2 files changed, 35 insertions(+), 55 deletions(-) create mode 100644 test/unit/nodeify-test.js diff --git a/app/scripts/lib/nodeify.js b/app/scripts/lib/nodeify.js index f48df34ef..56b793852 100644 --- a/app/scripts/lib/nodeify.js +++ b/app/scripts/lib/nodeify.js @@ -1,59 +1,17 @@ -/* 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)) +module.exports = function (promiseFn) { + return function () { + var args = [] + for (var i = 0; i < arguments.length - 1; i++) { + args.push(arguments[i]) } - return prom - } else if (typeof prom === 'function') { - prom.prototype.nodeify = nodeifyThis - } -} + var cb = arguments[arguments.length - 1] -function NodeifyPromise(fn) { - if (!(this instanceof NodeifyPromise)) { - return new NodeifyPromise(fn) + return promiseFn.apply(this, args) + .then(function (result) { + cb(null, result) + }) + .catch(function (reason) { + cb(reason) + }) } - Promise.call(this, fn) - extend(this) } - -NodeifyPromise.prototype = Object.create(Promise.prototype) -NodeifyPromise.prototype.constructor = NodeifyPromise diff --git a/test/unit/nodeify-test.js b/test/unit/nodeify-test.js new file mode 100644 index 000000000..d4ac2ea0b --- /dev/null +++ b/test/unit/nodeify-test.js @@ -0,0 +1,22 @@ +const assert = require('assert') +const nodeify = require('../../app/scripts/lib/nodeify') + +describe.only('nodeify', function() { + + var obj = { + foo: 'bar', + promiseFunc: function (a) { + var solution = this.foo + a + return Promise.resolve(solution) + } + } + + it('should retain original context', function(done) { + var nodified = nodeify(obj.promiseFunc).bind(obj) + nodified('baz', function (err, res) { + assert.equal(res, 'barbaz') + done() + }) + }) + +}) From 9d401f91370049c1bd50016808881a14ea892b29 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Mon, 28 Nov 2016 17:27:28 -0800 Subject: [PATCH 3/9] Fix nodeify usage --- app/scripts/metamask-controller.js | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 5bcb5b477..1a872a3ab 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -63,17 +63,17 @@ module.exports = class MetamaskController { setGasMultiplier: this.setGasMultiplier.bind(this), // forward directly to 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)), + 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), From 12906df601fcd549c2671c1cf999f37c30a4490d Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Mon, 28 Nov 2016 17:27:44 -0800 Subject: [PATCH 4/9] Remove unused dep --- package.json | 1 - 1 file changed, 1 deletion(-) diff --git a/package.json b/package.json index 2d15e9c9e..c6b389a6c 100644 --- a/package.json +++ b/package.json @@ -57,7 +57,6 @@ "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", From b81f00849d53ee780bf26d4d6f79aef4ebdba34c Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Tue, 29 Nov 2016 11:40:49 -0800 Subject: [PATCH 5/9] Annotated KeyringController --- app/scripts/keyring-controller.js | 715 +++++++++++++++++++++--------- 1 file changed, 496 insertions(+), 219 deletions(-) diff --git a/app/scripts/keyring-controller.js b/app/scripts/keyring-controller.js index 5e6b0acbb..7d29fb5d8 100644 --- a/app/scripts/keyring-controller.js +++ b/app/scripts/keyring-controller.js @@ -25,6 +25,13 @@ const createId = require('./lib/random-id') module.exports = class KeyringController extends EventEmitter { + // PUBLIC METHODS + // + // THE FIRST SECTION OF METHODS ARE PUBLIC-FACING, + // MEANING THEY ARE USED BY CONSUMERS OF THIS CLASS. + // + // THEIR SURFACE AREA SHOULD BE CHANGED WITH GREAT CARE. + constructor (opts) { super() this.configManager = opts.configManager @@ -46,6 +53,46 @@ module.exports = class KeyringController extends EventEmitter { }) } + // Set Store + // + // Allows setting the ethStore after the constructor. + // This is currently required because of the initialization order + // of the ethStore and this class. + // + // Eventually would be nice to be able to add this in the constructor. + setStore (ethStore) { + this.ethStore = ethStore + } + + // Full Update + // returns Promise( @object state ) + // + // Emits the `update` event and + // returns a Promise that resolves to the current state. + // + // Frequently used to end asynchronous chains in this class, + // indicating consumers can often either listen for updates, + // or accept a state-resolving promise to consume their results. + // + // Not all methods end with this, that might be a nice refactor. + fullUpdate() { + this.emit('update') + return Promise.resolve(this.getState()) + } + + // Get State + // returns @object state + // + // This method returns a hash representing the current state + // that the keyringController manages. + // + // It is extended in the MetamaskController along with the EthStore + // state, and its own state, to create the metamask state branch + // that is passed to the UI. + // + // This is currently a rare example of a synchronously resolving method + // in this class, but will need to be Promisified when we move our + // persistence to an async model. getState () { const configManager = this.configManager const address = configManager.getSelectedAccount() @@ -71,16 +118,30 @@ module.exports = class KeyringController extends EventEmitter { } } - setStore (ethStore) { - this.ethStore = ethStore - } - + // Create New Vault And Keychain + // @string password - The password to encrypt the vault with + // + // returns Promise( @object state ) + // + // Destroys any old encrypted storage, + // creates a new encrypted store with the given password, + // randomly creates a new HD wallet with 1 account, + // faucets that account on the testnet. createNewVaultAndKeychain (password) { - return this.createNewVault(password) + return this.persistAllKeyrings(password) .then(this.createFirstKeyTree.bind(this)) .then(this.fullUpdate.bind(this)) } + // CreateNewVaultAndRestore + // @string password - The password to encrypt the vault with + // @string seed - The BIP44-compliant seed phrase. + // + // returns Promise( @object state ) + // + // Destroys any old encrypted storage, + // creates a new encrypted store with the given password, + // creates a new HD wallet from the given seed with 1 account. createNewVaultAndRestore (password, seed) { if (typeof password !== 'string') { return Promise.reject('Password must be text.') @@ -92,7 +153,7 @@ module.exports = class KeyringController extends EventEmitter { this.clearKeyrings() - return this.createNewVault(password) + return this.persistAllKeyrings(password) .then(() => { return this.addNewKeyring('HD Key Tree', { mnemonic: seed, @@ -112,65 +173,54 @@ module.exports = class KeyringController extends EventEmitter { .then(this.fullUpdate.bind(this)) } - migrateOldVaultIfAny (password) { - const shouldMigrate = !!this.configManager.getWallet() && !this.configManager.getVault() - return this.idStoreMigrator.migratedVaultForPassword(password) - .then((serialized) => { - this.password = password - - if (serialized && shouldMigrate) { - return this.restoreKeyring(serialized) - .then(keyring => keyring.getAccounts()) - .then((accounts) => { - this.configManager.setSelectedAccount(accounts[0]) - return this.persistAllKeyrings() - }) - } else { - return Promise.resolve() - } - }) - } - - createNewVault (password) { - return this.migrateOldVaultIfAny(password) - .then(() => { - this.password = password - return this.persistAllKeyrings() - }) - .then(() => { - return password - }) - } - - createFirstKeyTree (password) { - this.clearKeyrings() - 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 + // returns Promise( @object state ) + // + // Adds the current vault's seed words to the UI's state tree. + // + // Used when creating a first vault, to allow confirmation. + // Also used when revealing the seed words in the confirmation view. placeSeedWords () { const firstKeyring = this.keyrings[0] return firstKeyring.serialize() .then((serialized) => { const seedWords = serialized.mnemonic this.configManager.setSeedWords(seedWords) - this.emit('update') - return + return this.fullUpdate() }) } + // ClearSeedWordCache + // + // returns Promise( @string currentSelectedAccount ) + // + // Removes the current vault's seed words from the UI's state tree, + // ensuring they are only ever available in the background process. + clearSeedWordCache () { + this.configManager.setSeedWords(null) + return Promise.resolve(this.configManager.getSelectedAccount()) + } + + // Set Locked + // returns Promise( @object state ) + // + // This method deallocates all secrets, and effectively locks metamask. + setLocked () { + this.password = null + this.keyrings = [] + return this.fullUpdate() + } + + // Submit Password + // @string password + // + // returns Promise( @object state ) + // + // Attempts to decrypt the current vault and load its keyrings + // into memory. + // + // Temporarily also migrates any old-style vaults first, as well. + // (Pre MetaMask 3.0.0) submitPassword (password) { return this.migrateOldVaultIfAny(password) .then(() => { @@ -183,11 +233,17 @@ module.exports = class KeyringController extends EventEmitter { .then(this.fullUpdate.bind(this)) } - fullUpdate() { - this.emit('update') - return Promise.resolve(this.getState()) - } - + // Add New Keyring + // @string type + // @object opts + // + // returns Promise( @Keyring keyring ) + // + // Adds a new Keyring of the given `type` to the vault + // and the current decrypted Keyrings array. + // + // All Keyring classes implement a unique `type` string, + // and this is used to retrieve them from the keyringTypes array. addNewKeyring (type, opts) { const Keyring = this.getKeyringClassForType(type) const keyring = new Keyring(opts) @@ -202,43 +258,42 @@ module.exports = class KeyringController extends EventEmitter { }) } + // Add New Account + // @number keyRingNum + // + // returns Promise( @object state ) + // + // Calls the `addAccounts` method on the Keyring + // in the kryings array at index `keyringNum`, + // and then saves those changes. addNewAccount (keyRingNum = 0) { const ring = this.keyrings[keyRingNum] return ring.addAccounts(1) .then(this.setupAccounts.bind(this)) .then(this.persistAllKeyrings.bind(this)) + .then(this.fullUpdate.bind(this)) } - setupAccounts (accounts) { - return this.getAccounts() - .then((loadedAccounts) => { - const arr = accounts || loadedAccounts - return Promise.all(arr.map((account) => { - return this.getBalanceAndNickname(account) - })) - }) - } - - // Takes an account address and an iterator representing - // the current number of named accounts. - getBalanceAndNickname (account) { - const address = normalize(account) - this.ethStore.addAccount(address) - return this.createNickname(address) - } - - createNickname (address) { - const hexAddress = normalize(address) - var i = Object.keys(this.identities).length - const oldNickname = this.configManager.nicknameForWallet(address) - const name = oldNickname || `Account ${++i}` - this.identities[hexAddress] = { - address: hexAddress, - name, - } - return this.saveAccountLabel(hexAddress, name) + // Set Selected Account + // @string address + // + // returns Promise( @string address ) + // + // Sets the state's `selectedAccount` value + // to the specified address. + setSelectedAccount (address) { + var addr = normalize(address) + this.configManager.setSelectedAccount(addr) + Promise.resolve(addr) } + // Save Account Label + // @string account + // @string label + // + // returns Promise( @string label ) + // + // Persists a nickname equal to `label` for the specified account. saveAccountLabel (account, label) { const address = normalize(account) const configManager = this.configManager @@ -247,80 +302,45 @@ module.exports = class KeyringController extends EventEmitter { return Promise.resolve(label) } - persistAllKeyrings () { - 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], - } + // Export Account + // @string address + // + // returns Promise( @string privateKey ) + // + // Requests the private key from the keyring controlling + // the specified address. + // + // Returns a Promise that may resolve with the private key string. + exportAccount (address) { + try { + return this.getKeyringForAccount(address) + .then((keyring) => { + return keyring.exportAccount(normalize(address)) }) - })) - .then((serializedKeyrings) => { - return this.encryptor.encrypt(this.password, serializedKeyrings) - }) - .then((encryptedString) => { - this.configManager.setVault(encryptedString) - return true - }) - } - - unlockKeyrings (password) { - const encryptedVault = this.configManager.getVault() - return this.encryptor.decrypt(password, encryptedVault) - .then((vault) => { - this.password = password - vault.forEach(this.restoreKeyring.bind(this)) - return this.keyrings - }) - } - - restoreKeyring (serialized) { - const { type, data } = serialized - const Keyring = this.getKeyringClassForType(type) - const keyring = new Keyring() - return keyring.deserialize(data) - .then(() => { - return keyring.getAccounts() - }) - .then((accounts) => { - return this.setupAccounts(accounts) - }) - .then(() => { - this.keyrings.push(keyring) - return keyring - }) + } catch (e) { + return Promise.reject(e) + } } - getKeyringClassForType (type) { - const Keyring = this.keyringTypes.reduce((res, kr) => { - if (kr.type === type) { - return kr - } else { - return res - } - }) - return Keyring - } - getAccounts () { - const keyrings = this.keyrings || [] - return Promise.all(keyrings.map(kr => kr.getAccounts())) - .then((keyringArrays) => { - return keyringArrays.reduce((res, arr) => { - return res.concat(arr) - }, []) - }) - } + // SIGNING RELATED METHODS + // + // SIGN, SUBMIT TX, CANCEL, AND APPROVE. + // THIS SECTION INVOLVES THE REQUEST, STORING, AND SIGNING OF DATA + // WITH THE KEYS STORED IN THIS CONTROLLER. - setSelectedAccount (address) { - var addr = normalize(address) - this.configManager.setSelectedAccount(addr) - Promise.resolve(addr) - } + // Add Unconfirmed Transaction + // @object txParams + // @function onTxDoneCb + // @function cb + // + // Calls back `cb` with @object txData = { txParams } + // Calls back `onTxDoneCb` with `true` or an `error` depending on result. + // + // Prepares the given `txParams` for final confirmation and approval. + // Estimates gas and other preparatory steps. + // Caches the requesting Dapp's callback, `onTxDoneCb`, for resolution later. addUnconfirmedTransaction (txParams, onTxDoneCb, cb) { var self = this const configManager = this.configManager @@ -446,28 +466,38 @@ module.exports = class KeyringController extends EventEmitter { } } - addUnconfirmedMessage (msgParams, cb) { - // create txData obj with parameters and meta data - var time = (new Date()).getTime() - var msgId = createId() - var msgData = { - id: msgId, - msgParams: msgParams, - time: time, - status: 'unconfirmed', - } - messageManager.addMsg(msgData) - console.log('addUnconfirmedMessage:', msgData) + // Cancel Transaction + // @string txId + // @function cb + // + // Calls back `cb` with no error if provided. + // + // Forgets any tx matching `txId`. + cancelTransaction (txId, cb) { + const configManager = this.configManager + var approvalCb = this._unconfTxCbs[txId] || noop - // keep the cb around for after approval (requires user interaction) - // This cb fires completion to the Dapp's write operation. - this._unconfMsgCbs[msgId] = cb + // reject tx + approvalCb(null, false) + // clean up + configManager.rejectTx(txId) + delete this._unconfTxCbs[txId] - // signal update - this.emit('update') - return msgId + if (cb && typeof cb === 'function') { + cb() + } } + // Approve Transaction + // @string txId + // @function cb + // + // Calls back `cb` with no error always. + // + // Attempts to sign a Transaction with `txId` + // and submit it to the blockchain. + // + // Calls back the cached Dapp's confirmation callback, also. approveTransaction (txId, cb) { const configManager = this.configManager var approvalCb = this._unconfTxCbs[txId] || noop @@ -480,22 +510,6 @@ module.exports = class KeyringController extends EventEmitter { delete this._unconfTxCbs[txId] this.emit('update') } - - cancelTransaction (txId, cb) { - const configManager = this.configManager - var approvalCb = this._unconfTxCbs[txId] || noop - - // reject tx - approvalCb(null, false) - // clean up - configManager.rejectTx(txId) - delete this._unconfTxCbs[txId] - - if (cb && typeof cb === 'function') { - cb() - } - } - signTransaction (txParams, cb) { try { const address = normalize(txParams.from) @@ -534,14 +548,77 @@ module.exports = class KeyringController extends EventEmitter { } } + // Add Unconfirmed Message + // @object msgParams + // @function cb + // + // Does not call back, only emits an `update` event. + // + // Adds the given `msgParams` and `cb` to a local cache, + // for displaying to a user for approval before signing or canceling. + addUnconfirmedMessage (msgParams, cb) { + // create txData obj with parameters and meta data + var time = (new Date()).getTime() + var msgId = createId() + var msgData = { + id: msgId, + msgParams: msgParams, + time: time, + status: 'unconfirmed', + } + messageManager.addMsg(msgData) + console.log('addUnconfirmedMessage:', msgData) + + // keep the cb around for after approval (requires user interaction) + // This cb fires completion to the Dapp's write operation. + this._unconfMsgCbs[msgId] = cb + + // signal update + this.emit('update') + return msgId + } + + // Cancel Message + // @string msgId + // @function cb (optional) + // + // Calls back to cached `unconfMsgCb`. + // Calls back to `cb` if provided. + // + // Forgets any messages matching `msgId`. + cancelMessage (msgId, cb) { + var approvalCb = this._unconfMsgCbs[msgId] || noop + + // reject tx + approvalCb(null, false) + // clean up + messageManager.rejectMsg(msgId) + delete this._unconfTxCbs[msgId] + + if (cb && typeof cb === 'function') { + cb() + } + } + + // Sign Message + // @object msgParams + // @function cb + // + // returns Promise(@buffer rawSig) + // calls back @function cb with @buffer rawSig + // calls back cached Dapp's @function unconfMsgCb. + // + // Attempts to sign the provided @object msgParams. signMessage (msgParams, cb) { try { + var approvalCb = this._unconfMsgCbs[msgId] || noop const address = normalize(msgParams.from) return this.getKeyringForAccount(address) .then((keyring) => { return keyring.signMessage(address, msgParams.data) }).then((rawSig) => { cb(null, rawSig) + approvalCb(null, true) return rawSig }) } catch (e) { @@ -549,6 +626,224 @@ module.exports = class KeyringController extends EventEmitter { } } + // PRIVATE METHODS + // + // THESE METHODS ARE ONLY USED INTERNALLY TO THE KEYRING-CONTROLLER + // AND SO MAY BE CHANGED MORE LIBERALLY THAN THE ABOVE METHODS. + + // Migrate Old Vault If Any + // @string password + // + // returns Promise() + // + // Temporary step used when logging in. + // Checks if old style (pre-3.0.0) Metamask Vault exists. + // If so, persists that vault in the new vault format + // with the provided password, so the other unlock steps + // may be completed without interruption. + migrateOldVaultIfAny (password) { + const shouldMigrate = !!this.configManager.getWallet() && !this.configManager.getVault() + return this.idStoreMigrator.migratedVaultForPassword(password) + .then((serialized) => { + this.password = password + + if (serialized && shouldMigrate) { + return this.restoreKeyring(serialized) + .then(keyring => keyring.getAccounts()) + .then((accounts) => { + this.configManager.setSelectedAccount(accounts[0]) + return this.persistAllKeyrings() + }) + } else { + return Promise.resolve() + } + }) + } + + // Create First Key Tree + // returns @Promise + // + // Clears the vault, + // creates a new one, + // creates a random new HD Keyring with 1 account, + // makes that account the selected account, + // faucets that account on testnet, + // puts the current seed words into the state tree. + createFirstKeyTree () { + this.clearKeyrings() + 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)) + } + + // Setup Accounts + // @array accounts + // + // returns @Promise(@object account) + // + // Initializes the provided account array + // Gives them numerically incremented nicknames, + // and adds them to the ethStore for regular balance checking. + setupAccounts (accounts) { + return this.getAccounts() + .then((loadedAccounts) => { + const arr = accounts || loadedAccounts + return Promise.all(arr.map((account) => { + return this.getBalanceAndNickname(account) + })) + }) + } + + // Get Balance And Nickname + // @string account + // + // returns Promise( @string label ) + // + // Takes an account address and an iterator representing + // the current number of named accounts. + getBalanceAndNickname (account) { + const address = normalize(account) + this.ethStore.addAccount(address) + return this.createNickname(address) + } + + // Create Nickname + // @string address + // + // returns Promise( @string label ) + // + // Takes an address, and assigns it an incremented nickname, persisting it. + createNickname (address) { + const hexAddress = normalize(address) + var i = Object.keys(this.identities).length + const oldNickname = this.configManager.nicknameForWallet(address) + const name = oldNickname || `Account ${++i}` + this.identities[hexAddress] = { + address: hexAddress, + name, + } + return this.saveAccountLabel(hexAddress, name) + } + + // Persist All Keyrings + // @password string + // + // returns Promise + // + // Iterates the current `keyrings` array, + // serializes each one into a serialized array, + // encrypts that array with the provided `password`, + // and persists that encrypted string to storage. + persistAllKeyrings (password = this.password) { + this.password = password + 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) + }) + .then((encryptedString) => { + this.configManager.setVault(encryptedString) + return true + }) + } + + // Unlock Keyrings + // @string password + // + // returns Promise( @array keyrings ) + // + // Attempts to unlock the persisted encrypted storage, + // initializing the persisted keyrings to RAM. + unlockKeyrings (password) { + const encryptedVault = this.configManager.getVault() + return this.encryptor.decrypt(password, encryptedVault) + .then((vault) => { + this.password = password + vault.forEach(this.restoreKeyring.bind(this)) + return this.keyrings + }) + } + + // Restore Keyring + // @object serialized + // + // returns Promise( @Keyring deserialized ) + // + // Attempts to initialize a new keyring from the provided + // serialized payload. + // + // On success, returns the resulting @Keyring instance. + restoreKeyring (serialized) { + const { type, data } = serialized + const Keyring = this.getKeyringClassForType(type) + const keyring = new Keyring() + return keyring.deserialize(data) + .then(() => { + return keyring.getAccounts() + }) + .then((accounts) => { + return this.setupAccounts(accounts) + }) + .then(() => { + this.keyrings.push(keyring) + return keyring + }) + } + + // Get Keyring Class For Type + // @string type + // + // Returns @class Keyring + // + // Searches the current `keyringTypes` array + // for a Keyring class whose unique `type` property + // matches the provided `type`, + // returning it if it exists. + getKeyringClassForType (type) { + return this.keyringTypes.find(kr => kr.type === type) + } + + // Get Accounts + // returns Promise( @Array[ @string accounts ] ) + // + // Returns the public addresses of all current accounts + // managed by all currently unlocked keyrings. + getAccounts () { + const keyrings = this.keyrings || [] + return Promise.all(keyrings.map(kr => kr.getAccounts())) + .then((keyringArrays) => { + return keyringArrays.reduce((res, arr) => { + return res.concat(arr) + }, []) + }) + } + + // Get Keyring For Account + // @string address + // + // returns Promise(@Keyring keyring) + // + // Returns the currently initialized keyring that manages + // the specified `address` if one exists. getKeyringForAccount (address) { const hexed = normalize(address) return new Promise((resolve, reject) => { @@ -576,29 +871,12 @@ module.exports = class KeyringController extends EventEmitter { }) } - cancelMessage (msgId, cb) { - if (cb && typeof cb === 'function') { - cb() - } - } - - setLocked () { - this.password = null - this.keyrings = [] - return this.fullUpdate() - } - - exportAccount (address) { - try { - return this.getKeyringForAccount(address) - .then((keyring) => { - return keyring.exportAccount(normalize(address)) - }) - } catch (e) { - return Promise.reject(e) - } - } - + // Add Gas Buffer + // @string gas (as hexadecimal value) + // + // returns @string bufferedGas (as hexadecimal value) + // + // Adds a healthy buffer of gas to an initial gas estimate. addGasBuffer (gas) { const gasBuffer = new BN('100000', 10) const bnGas = new BN(ethUtil.stripHexPrefix(gas), 16) @@ -606,11 +884,10 @@ module.exports = class KeyringController extends EventEmitter { return ethUtil.addHexPrefix(correct.toString(16)) } - clearSeedWordCache () { - this.configManager.setSeedWords(null) - return Promise.resolve(this.configManager.getSelectedAccount()) - } - + // Clear Keyrings + // + // Deallocates all currently managed keyrings and accounts. + // Used before initializing a new vault. clearKeyrings () { let accounts try { From 4b6b1db4f0fddfe3a640656311a58429ed48753c Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Tue, 29 Nov 2016 11:41:13 -0800 Subject: [PATCH 6/9] Ordered keyringController methods the same in metamask-controller --- app/scripts/keyring-controller.js | 6 +++++- app/scripts/metamask-controller.js | 8 ++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/app/scripts/keyring-controller.js b/app/scripts/keyring-controller.js index 7d29fb5d8..e6a69d9ed 100644 --- a/app/scripts/keyring-controller.js +++ b/app/scripts/keyring-controller.js @@ -611,7 +611,11 @@ module.exports = class KeyringController extends EventEmitter { // Attempts to sign the provided @object msgParams. signMessage (msgParams, cb) { try { - var approvalCb = this._unconfMsgCbs[msgId] || noop + + const msgId = msgParams.metamaskId + delete msgParams.metamaskId + const approvalCb = this._unconfMsgCbs[msgId] || noop + const address = normalize(msgParams.from) return this.getKeyringForAccount(address) .then((keyring) => { diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 1a872a3ab..ae761c753 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -63,17 +63,17 @@ module.exports = class MetamaskController { setGasMultiplier: this.setGasMultiplier.bind(this), // forward directly to keyringController - placeSeedWords: nodeify(keyringController.placeSeedWords).bind(keyringController), createNewVaultAndKeychain: nodeify(keyringController.createNewVaultAndKeychain).bind(keyringController), createNewVaultAndRestore: nodeify(keyringController.createNewVaultAndRestore).bind(keyringController), + placeSeedWords: nodeify(keyringController.placeSeedWords).bind(keyringController), clearSeedWordCache: nodeify(keyringController.clearSeedWordCache).bind(keyringController), + setLocked: nodeify(keyringController.setLocked).bind(keyringController), + submitPassword: nodeify(keyringController.submitPassword).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), + exportAccount: nodeify(keyringController.exportAccount).bind(keyringController), // signing methods approveTransaction: keyringController.approveTransaction.bind(keyringController), From 85d5b12f8dd4029bf93a7fb0b5f65b306cbaaa3c Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Tue, 29 Nov 2016 12:44:42 -0800 Subject: [PATCH 7/9] Fix tx adding code Broken in this commit: https://github.com/MetaMask/metamask-plugin/commit/bc39cd7b894ddf0f3724d4af3cfc30c2638e0939 Synchronous methods were added to an `async.waterfall` array. This commit also removes the delegate call checking, since we concluded it was misinformed. --- app/scripts/keyring-controller.js | 28 +++---------------------- ui/app/components/pending-tx-details.js | 25 ---------------------- 2 files changed, 3 insertions(+), 50 deletions(-) diff --git a/app/scripts/keyring-controller.js b/app/scripts/keyring-controller.js index e6a69d9ed..0045890be 100644 --- a/app/scripts/keyring-controller.js +++ b/app/scripts/keyring-controller.js @@ -1,7 +1,6 @@ const async = require('async') const bind = require('ap').partial const ethUtil = require('ethereumjs-util') -const ethBinToOps = require('eth-bin-to-ops') const EthQuery = require('eth-query') const bip39 = require('bip39') const Transaction = require('ethereumjs-tx') @@ -369,30 +368,9 @@ module.exports = class KeyringController extends EventEmitter { // calculate metadata for tx async.parallel([ - analyzeForDelegateCall, analyzeGasUsage, ], didComplete) - // perform static analyis on the target contract code - function analyzeForDelegateCall (cb) { - if (txParams.to) { - query.getCode(txParams.to, function (err, result) { - if (err) return cb(err) - var code = ethUtil.toBuffer(result) - if (code !== '0x') { - var ops = ethBinToOps(code) - var containsDelegateCall = ops.some((op) => op.name === 'DELEGATECALL') - txData.containsDelegateCall = containsDelegateCall - cb() - } else { - cb() - } - }) - } else { - cb() - } - } - function analyzeGasUsage (cb) { query.getBlockByNumber('latest', true, function (err, block) { if (err) return cb(err) @@ -416,7 +394,7 @@ module.exports = class KeyringController extends EventEmitter { query.estimateGas(txParams, cb) } - function checkForGasError (txData, estimatedGasHex) { + function checkForGasError (txData, estimatedGasHex, cb) { txData.estimatedGas = estimatedGasHex // all gas used - must be an error if (estimatedGasHex === txData.txParams.gas) { @@ -425,7 +403,7 @@ module.exports = class KeyringController extends EventEmitter { cb() } - function setTxGas (txData, blockGasLimitHex) { + function setTxGas (txData, blockGasLimitHex, cb) { const txParams = txData.txParams // if OOG, nothing more to do if (txData.simulationFails) { @@ -443,7 +421,7 @@ module.exports = class KeyringController extends EventEmitter { // try adding an additional gas buffer to our estimation for safety const estimatedGasBn = new BN(ethUtil.stripHexPrefix(txData.estimatedGas), 16) const blockGasLimitBn = new BN(ethUtil.stripHexPrefix(blockGasLimitHex), 16) - const estimationWithBuffer = self.addGasBuffer(estimatedGasBn) + const estimationWithBuffer = new BN(self.addGasBuffer(estimatedGasBn), 16) // added gas buffer is too high if (estimationWithBuffer.gt(blockGasLimitBn)) { txParams.gas = txData.estimatedGas diff --git a/ui/app/components/pending-tx-details.js b/ui/app/components/pending-tx-details.js index 42fb4c870..89472b221 100644 --- a/ui/app/components/pending-tx-details.js +++ b/ui/app/components/pending-tx-details.js @@ -154,8 +154,6 @@ PTXP.render = function () { ]), ]), // End of Table - this.warnIfNeeded(), - ]) ) } @@ -201,29 +199,6 @@ PTXP.miniAccountPanelForRecipient = function () { } } -// Should analyze if there is a DELEGATECALL opcode -// in the recipient contract, and show a warning if so. -PTXP.warnIfNeeded = function () { - const containsDelegateCall = !!this.props.txData.containsDelegateCall - - if (!containsDelegateCall) { - return null - } - - return h('span.error', { - style: { - fontFamily: 'Montserrat Light', - fontSize: '13px', - display: 'flex', - justifyContent: 'center', - }, - }, [ - h('i.fa.fa-lg.fa-info-circle', { style: { margin: '5px' } }), - h('span', ' Your identity may be used in other contracts!'), - ]) -} - - function forwardCarrat () { return ( From ff3f6cc36adcc9a3c780d6cc0f2a05c3c45c3162 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Tue, 29 Nov 2016 14:13:12 -0800 Subject: [PATCH 8/9] Bind ethQuery to estimateGas to allow it to be moved around. --- app/scripts/keyring-controller.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/scripts/keyring-controller.js b/app/scripts/keyring-controller.js index 0045890be..7976a9e3a 100644 --- a/app/scripts/keyring-controller.js +++ b/app/scripts/keyring-controller.js @@ -375,14 +375,14 @@ module.exports = class KeyringController extends EventEmitter { query.getBlockByNumber('latest', true, function (err, block) { if (err) return cb(err) async.waterfall([ - bind(estimateGas, txData, block.gasLimit), + bind(estimateGas, query, txData, block.gasLimit), bind(checkForGasError, txData), bind(setTxGas, txData, block.gasLimit), ], cb) }) } - function estimateGas (txData, blockGasLimitHex, cb) { + function estimateGas (query, txData, blockGasLimitHex, cb) { const txParams = txData.txParams // check if gasLimit is already specified txData.gasLimitSpecified = Boolean(txParams.gas) From 5bf1018d7540e0d89aa866e8d7f709e577bb99e3 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Tue, 29 Nov 2016 14:56:02 -0800 Subject: [PATCH 9/9] Flattened addTx async methods --- app/scripts/keyring-controller.js | 171 ++++++++++++++---------------- package.json | 2 +- 2 files changed, 83 insertions(+), 90 deletions(-) diff --git a/app/scripts/keyring-controller.js b/app/scripts/keyring-controller.js index 7976a9e3a..900b3ca25 100644 --- a/app/scripts/keyring-controller.js +++ b/app/scripts/keyring-controller.js @@ -1,11 +1,10 @@ const async = require('async') -const bind = require('ap').partial const ethUtil = require('ethereumjs-util') const EthQuery = require('eth-query') const bip39 = require('bip39') const Transaction = require('ethereumjs-tx') const EventEmitter = require('events').EventEmitter - +const filter = require('promise-filter') const normalize = require('./lib/sig-util').normalize const encryptor = require('./lib/encryptor') const messageManager = require('./lib/message-manager') @@ -341,7 +340,6 @@ module.exports = class KeyringController extends EventEmitter { // Estimates gas and other preparatory steps. // Caches the requesting Dapp's callback, `onTxDoneCb`, for resolution later. addUnconfirmedTransaction (txParams, onTxDoneCb, cb) { - var self = this const configManager = this.configManager // create txData obj with parameters and meta data @@ -358,7 +356,6 @@ module.exports = class KeyringController extends EventEmitter { metamaskNetworkId: this.getNetwork(), } - // keep the onTxDoneCb around for after approval/denial (requires user interaction) // This onTxDoneCb fires completion to the Dapp's write operation. this._unconfTxCbs[txId] = onTxDoneCb @@ -367,81 +364,80 @@ module.exports = class KeyringController extends EventEmitter { var query = new EthQuery(provider) // calculate metadata for tx - async.parallel([ - analyzeGasUsage, - ], didComplete) - - function analyzeGasUsage (cb) { - query.getBlockByNumber('latest', true, function (err, block) { - if (err) return cb(err) - async.waterfall([ - bind(estimateGas, query, txData, block.gasLimit), - bind(checkForGasError, txData), - bind(setTxGas, txData, block.gasLimit), - ], cb) - }) + this.analyzeTxGasUsage(query, txData, this.txDidComplete.bind(this, txData, cb)) + } + + estimateTxGas (query, txData, blockGasLimitHex, cb) { + const txParams = txData.txParams + // check if gasLimit is already specified + txData.gasLimitSpecified = Boolean(txParams.gas) + // if not, fallback to block gasLimit + if (!txData.gasLimitSpecified) { + txParams.gas = blockGasLimitHex } + // run tx, see if it will OOG + query.estimateGas(txParams, cb) + } - function estimateGas (query, txData, blockGasLimitHex, cb) { - const txParams = txData.txParams - // check if gasLimit is already specified - txData.gasLimitSpecified = Boolean(txParams.gas) - // if not, fallback to block gasLimit - if (!txData.gasLimitSpecified) { - txParams.gas = blockGasLimitHex - } - // run tx, see if it will OOG - query.estimateGas(txParams, cb) + checkForTxGasError (txData, estimatedGasHex, cb) { + txData.estimatedGas = estimatedGasHex + // all gas used - must be an error + if (estimatedGasHex === txData.txParams.gas) { + txData.simulationFails = true } + cb() + } - function checkForGasError (txData, estimatedGasHex, cb) { - txData.estimatedGas = estimatedGasHex - // all gas used - must be an error - if (estimatedGasHex === txData.txParams.gas) { - txData.simulationFails = true - } + setTxGas (txData, blockGasLimitHex, cb) { + const txParams = txData.txParams + // if OOG, nothing more to do + if (txData.simulationFails) { cb() + return } - - function setTxGas (txData, blockGasLimitHex, cb) { - const txParams = txData.txParams - // if OOG, nothing more to do - if (txData.simulationFails) { - cb() - return - } - // if gasLimit was specified and doesnt OOG, - // use original specified amount - if (txData.gasLimitSpecified) { - txData.estimatedGas = txParams.gas - cb() - return - } - // if gasLimit not originally specified, - // try adding an additional gas buffer to our estimation for safety - const estimatedGasBn = new BN(ethUtil.stripHexPrefix(txData.estimatedGas), 16) - const blockGasLimitBn = new BN(ethUtil.stripHexPrefix(blockGasLimitHex), 16) - const estimationWithBuffer = new BN(self.addGasBuffer(estimatedGasBn), 16) - // added gas buffer is too high - if (estimationWithBuffer.gt(blockGasLimitBn)) { - txParams.gas = txData.estimatedGas - // added gas buffer is safe - } else { - const gasWithBufferHex = ethUtil.intToHex(estimationWithBuffer) - txParams.gas = gasWithBufferHex - } + // if gasLimit was specified and doesnt OOG, + // use original specified amount + if (txData.gasLimitSpecified) { + txData.estimatedGas = txParams.gas cb() return } + // if gasLimit not originally specified, + // try adding an additional gas buffer to our estimation for safety + const estimatedGasBn = new BN(ethUtil.stripHexPrefix(txData.estimatedGas), 16) + const blockGasLimitBn = new BN(ethUtil.stripHexPrefix(blockGasLimitHex), 16) + const estimationWithBuffer = new BN(this.addGasBuffer(estimatedGasBn), 16) + // added gas buffer is too high + if (estimationWithBuffer.gt(blockGasLimitBn)) { + txParams.gas = txData.estimatedGas + // added gas buffer is safe + } else { + const gasWithBufferHex = ethUtil.intToHex(estimationWithBuffer) + txParams.gas = gasWithBufferHex + } + cb() + return + } - function didComplete (err) { + txDidComplete (txData, cb, err) { + if (err) return cb(err) + const configManager = this.configManager + configManager.addTx(txData) + // signal update + this.emit('update') + // signal completion of add tx + cb(null, txData) + } + + analyzeTxGasUsage (query, txData, cb) { + query.getBlockByNumber('latest', true, (err, block) => { if (err) return cb(err) - configManager.addTx(txData) - // signal update - self.emit('update') - // signal completion of add tx - cb(null, txData) - } + async.waterfall([ + this.estimateTxGas.bind(this, query, txData, block.gasLimit), + this.checkForTxGasError.bind(this, txData), + this.setTxGas.bind(this, txData, block.gasLimit), + ], cb) + }) } // Cancel Transaction @@ -488,6 +484,7 @@ module.exports = class KeyringController extends EventEmitter { delete this._unconfTxCbs[txId] this.emit('update') } + signTransaction (txParams, cb) { try { const address = normalize(txParams.from) @@ -828,28 +825,23 @@ module.exports = class KeyringController extends EventEmitter { // the specified `address` if one exists. getKeyringForAccount (address) { const hexed = normalize(address) - 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.') - } - }) + return Promise.all(this.keyrings.map((keyring) => { + return Promise.all([ + keyring, + keyring.getAccounts(), + ]) + })) + .then(filter((candidate) => { + const accounts = candidate[1].map(normalize) + return accounts.includes(hexed) + })) + .then((winners) => { + if (winners && winners.length > 0) { + return winners[0][0] + } else { + throw new Error('No keyring found for the requested account.') + } }) } @@ -888,4 +880,5 @@ module.exports = class KeyringController extends EventEmitter { } + function noop () {} diff --git a/package.json b/package.json index c6b389a6c..683938aad 100644 --- a/package.json +++ b/package.json @@ -33,7 +33,6 @@ ] }, "dependencies": { - "ap": "^0.2.0", "async": "^1.5.2", "bip39": "^2.2.0", "browserify-derequire": "^0.9.4", @@ -67,6 +66,7 @@ "pojo-migrator": "^2.1.0", "polyfill-crypto.getrandomvalues": "^1.0.0", "post-message-stream": "^1.0.0", + "promise-filter": "^1.1.0", "pumpify": "^1.3.4", "qrcode-npm": "0.0.3", "react": "^15.0.2",