From c0d3db6a8ce2fe0522fa4e3cba1e01d10469280f Mon Sep 17 00:00:00 2001 From: kumavis Date: Tue, 31 Jan 2017 20:02:38 -0800 Subject: [PATCH 1/6] keyring - synchronous getState --- app/scripts/keyring-controller.js | 55 ++++++++++++++++------------- app/scripts/metamask-controller.js | 38 +++++++++----------- test/unit/idStore-migration-test.js | 13 +++---- 3 files changed, 51 insertions(+), 55 deletions(-) diff --git a/app/scripts/keyring-controller.js b/app/scripts/keyring-controller.js index f7a4e4010..3737d1b55 100644 --- a/app/scripts/keyring-controller.js +++ b/app/scripts/keyring-controller.js @@ -2,6 +2,7 @@ const ethUtil = require('ethereumjs-util') const BN = ethUtil.BN const bip39 = require('bip39') const EventEmitter = require('events').EventEmitter +const extend = require('xtend') const ObservableStore = require('obs-store') const filter = require('promise-filter') const encryptor = require('browser-passworder') @@ -32,6 +33,7 @@ class KeyringController extends EventEmitter { super() const initState = opts.initState || {} this.store = new ObservableStore(initState) + this.memStore = new ObservableStore({}) this.configManager = opts.configManager this.ethStore = opts.ethStore this.encryptor = encryptor @@ -74,30 +76,27 @@ class KeyringController extends EventEmitter { // in this class, but will need to be Promisified when we move our // persistence to an async model. getState () { - return Promise.all(this.keyrings.map(this.displayForKeyring)) - .then((displayKeyrings) => { - const state = this.store.getState() - // old wallet - const wallet = this.configManager.getWallet() - return { - // computed - isInitialized: (!!wallet || !!state.vault), - isUnlocked: (!!this.password), - keyrings: displayKeyrings, - // hard coded - keyringTypes: this.keyringTypes.map(krt => krt.type), - // memStore - identities: this.identities, - // configManager - seedWords: this.configManager.getSeedWords(), - isDisclaimerConfirmed: this.configManager.getConfirmedDisclaimer(), - currentFiat: this.configManager.getCurrentFiat(), - conversionRate: this.configManager.getConversionRate(), - conversionDate: this.configManager.getConversionDate(), - // messageManager - unconfMsgs: messageManager.unconfirmedMsgs(), - messages: messageManager.getMsgList(), - } + const state = this.store.getState() + // old wallet + const wallet = this.configManager.getWallet() + const memState = this.memStore.getState() + return extend(memState, { + // computed + isInitialized: (!!wallet || !!state.vault), + isUnlocked: (!!this.password), + // hard coded + keyringTypes: this.keyringTypes.map(krt => krt.type), + // memStore + identities: this.identities, + // configManager + seedWords: this.configManager.getSeedWords(), + isDisclaimerConfirmed: this.configManager.getConfirmedDisclaimer(), + currentFiat: this.configManager.getCurrentFiat(), + conversionRate: this.configManager.getConversionRate(), + conversionDate: this.configManager.getConversionDate(), + // messageManager + unconfMsgs: messageManager.unconfirmedMsgs(), + messages: messageManager.getMsgList(), }) } @@ -164,6 +163,7 @@ class KeyringController extends EventEmitter { setLocked () { this.password = null this.keyrings = [] + this._updateMemStoreKeyrings() return this.fullUpdate() } @@ -639,6 +639,13 @@ class KeyringController extends EventEmitter { this.identities = {} } + _updateMemStoreKeyrings() { + Promise.all(this.keyrings.map(this.displayForKeyring)) + .then((keyrings) => { + this.memStore.updateState({ keyrings }) + }) + } + } module.exports = KeyringController diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 2c379f8d9..17b5683a1 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -169,22 +169,19 @@ module.exports = class MetamaskController extends EventEmitter { // getState () { - return this.keyringController.getState() - .then((keyringControllerState) => { - return extend( - this.state, - this.ethStore.getState(), - this.configManager.getConfig(), - this.txManager.getState(), - keyringControllerState, - this.preferencesController.store.getState(), - this.noticeController.getState(), - { - shapeShiftTxList: this.configManager.getShapeShiftTxList(), - lostAccounts: this.configManager.getLostAccounts(), - } - ) - }) + return extend( + this.state, + this.ethStore.getState(), + this.configManager.getConfig(), + this.txManager.getState(), + this.keyringController.getState(), + this.preferencesController.store.getState(), + this.noticeController.getState(), + { + shapeShiftTxList: this.configManager.getShapeShiftTxList(), + lostAccounts: this.configManager.getLostAccounts(), + } + ) } // @@ -199,7 +196,7 @@ module.exports = class MetamaskController extends EventEmitter { return { // etc - getState: nodeify(this.getState.bind(this)), + getState: (cb) => cb(null, this.getState()), setRpcTarget: this.setRpcTarget.bind(this), setProviderType: this.setProviderType.bind(this), useEtherscanProvider: this.useEtherscanProvider.bind(this), @@ -295,11 +292,8 @@ module.exports = class MetamaskController extends EventEmitter { ) } - sendUpdate () { - this.getState() - .then((state) => { - this.emit('update', state) - }) + sendUpdate () { + this.emit('update', this.getState()) } // diff --git a/test/unit/idStore-migration-test.js b/test/unit/idStore-migration-test.js index 3aaf4bb94..1537e88ec 100644 --- a/test/unit/idStore-migration-test.js +++ b/test/unit/idStore-migration-test.js @@ -83,15 +83,10 @@ describe('IdentityStore to KeyringController migration', function() { describe('entering a password', function() { it('should identify an old wallet as an initialized keyring', function(done) { keyringController.configManager.setWallet('something') - keyringController.getState() - .then((state) => { - assert(state.isInitialized, 'old vault counted as initialized.') - assert(!state.lostAccounts, 'no lost accounts') - done() - }) - .catch((err) => { - done(err) - }) + const state = keyringController.getState() + assert(state.isInitialized, 'old vault counted as initialized.') + assert(!state.lostAccounts, 'no lost accounts') + done() }) }) }) From ad060e267810dfd1217d5ddf40516976c8b2d68e Mon Sep 17 00:00:00 2001 From: kumavis Date: Tue, 31 Jan 2017 22:35:11 -0800 Subject: [PATCH 2/6] metamask - inherit some configManager state from keyring controller --- app/scripts/keyring-controller.js | 20 ++++++++++---------- app/scripts/metamask-controller.js | 3 +++ 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/app/scripts/keyring-controller.js b/app/scripts/keyring-controller.js index 3737d1b55..a7e5fed71 100644 --- a/app/scripts/keyring-controller.js +++ b/app/scripts/keyring-controller.js @@ -2,7 +2,6 @@ const ethUtil = require('ethereumjs-util') const BN = ethUtil.BN const bip39 = require('bip39') const EventEmitter = require('events').EventEmitter -const extend = require('xtend') const ObservableStore = require('obs-store') const filter = require('promise-filter') const encryptor = require('browser-passworder') @@ -33,7 +32,9 @@ class KeyringController extends EventEmitter { super() const initState = opts.initState || {} this.store = new ObservableStore(initState) - this.memStore = new ObservableStore({}) + this.memStore = new ObservableStore({ + keyrings: [], + }) this.configManager = opts.configManager this.ethStore = opts.ethStore this.encryptor = encryptor @@ -80,7 +81,7 @@ class KeyringController extends EventEmitter { // old wallet const wallet = this.configManager.getWallet() const memState = this.memStore.getState() - return extend(memState, { + const result = { // computed isInitialized: (!!wallet || !!state.vault), isUnlocked: (!!this.password), @@ -88,16 +89,15 @@ class KeyringController extends EventEmitter { keyringTypes: this.keyringTypes.map(krt => krt.type), // memStore identities: this.identities, - // configManager - seedWords: this.configManager.getSeedWords(), - isDisclaimerConfirmed: this.configManager.getConfirmedDisclaimer(), - currentFiat: this.configManager.getCurrentFiat(), - conversionRate: this.configManager.getConversionRate(), - conversionDate: this.configManager.getConversionDate(), + keyrings: memState.keyrings, // messageManager unconfMsgs: messageManager.unconfirmedMsgs(), messages: messageManager.getMsgList(), - }) + // configManager + seedWords: this.configManager.getSeedWords(), + isDisclaimerConfirmed: this.configManager.getConfirmedDisclaimer(), + } + return result } // Create New Vault And Keychain diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 17b5683a1..23ced75f1 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -180,6 +180,9 @@ module.exports = class MetamaskController extends EventEmitter { { shapeShiftTxList: this.configManager.getShapeShiftTxList(), lostAccounts: this.configManager.getLostAccounts(), + currentFiat: this.configManager.getCurrentFiat(), + conversionRate: this.configManager.getConversionRate(), + conversionDate: this.configManager.getConversionDate(), } ) } From d326e7d11efff754670164560964a146cdf7f53b Mon Sep 17 00:00:00 2001 From: kumavis Date: Tue, 31 Jan 2017 23:39:39 -0800 Subject: [PATCH 3/6] tests - mock-dev - remove persistence --- mock-dev.js | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/mock-dev.js b/mock-dev.js index bd3a1ad77..b20c3be3a 100644 --- a/mock-dev.js +++ b/mock-dev.js @@ -16,7 +16,6 @@ const extend = require('xtend') const render = require('react-dom').render const h = require('react-hyperscript') const pipe = require('mississippi').pipe -const LocalStorageStore = require('obs-store/lib/localStorage') const Root = require('./ui/app/root') const configureStore = require('./ui/app/store') const actions = require('./ui/app/actions') @@ -27,7 +26,6 @@ const firstTimeState = require('./app/scripts/first-time-state') const extension = require('./development/mockExtension') const noop = function () {} -const STORAGE_KEY = 'metamask-config' // // Query String @@ -56,27 +54,15 @@ const injectCss = require('inject-css') // MetaMask Controller // -let dataStore = new LocalStorageStore({ storageKey: STORAGE_KEY }) -// initial state for first time users -if (!dataStore.getState()) { - dataStore.putState(firstTimeState) -} - const controller = new MetamaskController({ // User confirmation callbacks: showUnconfirmedMessage: noop, unlockAccountMessage: noop, showUnapprovedTx: noop, // initial state - initState: dataStore.getState(), + initState: firstTimeState, }) -// setup state persistence -pipe( - controller.store, - dataStore -) - // // User Interface // From 41c93ceb7eccd4c77f95e21b5965d306e4122e8e Mon Sep 17 00:00:00 2001 From: kumavis Date: Wed, 1 Feb 2017 00:02:10 -0800 Subject: [PATCH 4/6] keyring - add keyringtypes to memStore --- app/scripts/keyring-controller.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/scripts/keyring-controller.js b/app/scripts/keyring-controller.js index a7e5fed71..c5ecac089 100644 --- a/app/scripts/keyring-controller.js +++ b/app/scripts/keyring-controller.js @@ -31,14 +31,15 @@ class KeyringController extends EventEmitter { constructor (opts) { super() const initState = opts.initState || {} + this.keyringTypes = keyringTypes this.store = new ObservableStore(initState) this.memStore = new ObservableStore({ keyrings: [], + keyringTypes: this.keyringTypes.map(krt => krt.type), }) this.configManager = opts.configManager this.ethStore = opts.ethStore this.encryptor = encryptor - this.keyringTypes = keyringTypes this.keyrings = [] this.identities = {} // Essentially a name hash @@ -85,9 +86,8 @@ class KeyringController extends EventEmitter { // computed isInitialized: (!!wallet || !!state.vault), isUnlocked: (!!this.password), - // hard coded - keyringTypes: this.keyringTypes.map(krt => krt.type), // memStore + keyringTypes: memState.keyringTypes, identities: this.identities, keyrings: memState.keyrings, // messageManager From cd5d95260026333735f25a179c7018077e0da44e Mon Sep 17 00:00:00 2001 From: kumavis Date: Wed, 1 Feb 2017 00:17:48 -0800 Subject: [PATCH 5/6] keyring - move identities into memStore --- app/scripts/keyring-controller.js | 22 +++++++++++++++------- test/unit/keyring-controller-test.js | 6 ++++-- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/app/scripts/keyring-controller.js b/app/scripts/keyring-controller.js index c5ecac089..22ceec889 100644 --- a/app/scripts/keyring-controller.js +++ b/app/scripts/keyring-controller.js @@ -34,14 +34,14 @@ class KeyringController extends EventEmitter { this.keyringTypes = keyringTypes this.store = new ObservableStore(initState) this.memStore = new ObservableStore({ - keyrings: [], keyringTypes: this.keyringTypes.map(krt => krt.type), + keyrings: [], + identities: {}, }) this.configManager = opts.configManager this.ethStore = opts.ethStore this.encryptor = encryptor this.keyrings = [] - this.identities = {} // Essentially a name hash this._unconfMsgCbs = {} @@ -88,7 +88,7 @@ class KeyringController extends EventEmitter { isUnlocked: (!!this.password), // memStore keyringTypes: memState.keyringTypes, - identities: this.identities, + identities: memState.identities, keyrings: memState.keyrings, // messageManager unconfMsgs: messageManager.unconfirmedMsgs(), @@ -245,7 +245,9 @@ class KeyringController extends EventEmitter { walletNicknames[hexAddress] = label this.store.updateState({ walletNicknames }) // update state on memStore - this.identities[hexAddress].name = label + const identities = this.memStore.getState().identities + identities[hexAddress].name = label + this.memStore.updateState({ identities }) return Promise.resolve(label) } catch (err) { return Promise.reject(err) @@ -439,14 +441,16 @@ class KeyringController extends EventEmitter { // Takes an address, and assigns it an incremented nickname, persisting it. createNickname (address) { const hexAddress = normalizeAddress(address) - const currentIdentityCount = Object.keys(this.identities).length + 1 + const identities = this.memStore.getState().identities + const currentIdentityCount = Object.keys(identities).length + 1 const nicknames = this.store.getState().walletNicknames || {} const existingNickname = nicknames[hexAddress] const name = existingNickname || `Account ${currentIdentityCount}` - this.identities[hexAddress] = { + identities[hexAddress] = { address: hexAddress, name, } + this.memStore.updateState({ identities }) return this.saveAccountLabel(hexAddress, name) } @@ -635,8 +639,12 @@ class KeyringController extends EventEmitter { this.ethStore.removeAccount(address) }) + // clear keyrings from memory this.keyrings = [] - this.identities = {} + this.memStore.updateState({ + keyrings: [], + identities: {}, + }) } _updateMemStoreKeyrings() { diff --git a/test/unit/keyring-controller-test.js b/test/unit/keyring-controller-test.js index 347aa2bdf..aae4cdfd6 100644 --- a/test/unit/keyring-controller-test.js +++ b/test/unit/keyring-controller-test.js @@ -104,7 +104,7 @@ describe('KeyringController', function() { it('should add the address to the identities hash', function() { const fakeAddress = '0x12345678' keyringController.createNickname(fakeAddress) - const identities = keyringController.identities + const identities = keyringController.memStore.getState().identities const identity = identities[fakeAddress] assert.equal(identity.address, fakeAddress) }) @@ -114,7 +114,9 @@ describe('KeyringController', function() { it ('sets the nickname', function(done) { const account = addresses[0] var nick = 'Test nickname' - keyringController.identities[ethUtil.addHexPrefix(account)] = {} + const identities = keyringController.memStore.getState().identities + identities[ethUtil.addHexPrefix(account)] = {} + keyringController.memStore.updateState({ identities }) keyringController.saveAccountLabel(account, nick) .then((label) => { try { From 1cb730144d6b1c05007323bbc31e4375373628fb Mon Sep 17 00:00:00 2001 From: kumavis Date: Wed, 1 Feb 2017 00:31:26 -0800 Subject: [PATCH 6/6] metamask - adopt isInitialized from keyring controller --- app/scripts/keyring-controller.js | 3 --- app/scripts/metamask-controller.js | 9 ++++++++- test/unit/idStore-migration-test.js | 9 --------- 3 files changed, 8 insertions(+), 13 deletions(-) diff --git a/app/scripts/keyring-controller.js b/app/scripts/keyring-controller.js index 22ceec889..be54ab00b 100644 --- a/app/scripts/keyring-controller.js +++ b/app/scripts/keyring-controller.js @@ -78,13 +78,10 @@ class KeyringController extends EventEmitter { // in this class, but will need to be Promisified when we move our // persistence to an async model. getState () { - const state = this.store.getState() // old wallet - const wallet = this.configManager.getWallet() const memState = this.memStore.getState() const result = { // computed - isInitialized: (!!wallet || !!state.vault), isUnlocked: (!!this.password), // memStore keyringTypes: memState.keyringTypes, diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 23ced75f1..222a1d618 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -169,14 +169,21 @@ module.exports = class MetamaskController extends EventEmitter { // getState () { + const wallet = this.configManager.getWallet() + const vault = this.keyringController.store.getState().vault + const isInitialized = (!!wallet || !!vault) return extend( + { + isInitialized, + }, this.state, this.ethStore.getState(), - this.configManager.getConfig(), this.txManager.getState(), this.keyringController.getState(), this.preferencesController.store.getState(), this.noticeController.getState(), + // config manager + this.configManager.getConfig(), { shapeShiftTxList: this.configManager.getShapeShiftTxList(), lostAccounts: this.configManager.getLostAccounts(), diff --git a/test/unit/idStore-migration-test.js b/test/unit/idStore-migration-test.js index 1537e88ec..81a99ef63 100644 --- a/test/unit/idStore-migration-test.js +++ b/test/unit/idStore-migration-test.js @@ -80,13 +80,4 @@ describe('IdentityStore to KeyringController migration', function() { }) }) - describe('entering a password', function() { - it('should identify an old wallet as an initialized keyring', function(done) { - keyringController.configManager.setWallet('something') - const state = keyringController.getState() - assert(state.isInitialized, 'old vault counted as initialized.') - assert(!state.lostAccounts, 'no lost accounts') - done() - }) - }) })