From a90c152485b84d6b382218543a6b38918a3ce6cf Mon Sep 17 00:00:00 2001 From: Whymarrh Whitby Date: Thu, 16 Aug 2018 14:58:58 -0230 Subject: [PATCH 1/4] Update AccountModalContainer to accept a selectedIdentity prop --- ui/app/components/modals/account-modal-container.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ui/app/components/modals/account-modal-container.js b/ui/app/components/modals/account-modal-container.js index a9856b20f..aa0593df8 100644 --- a/ui/app/components/modals/account-modal-container.js +++ b/ui/app/components/modals/account-modal-container.js @@ -7,9 +7,9 @@ const actions = require('../../actions') const { getSelectedIdentity } = require('../../selectors') const Identicon = require('../identicon') -function mapStateToProps (state) { +function mapStateToProps (state, ownProps) { return { - selectedIdentity: getSelectedIdentity(state), + selectedIdentity: ownProps.selectedIdentity || getSelectedIdentity(state), } } From 1e8e8bdfc87903249320b97d569c64c55a524899 Mon Sep 17 00:00:00 2001 From: Whymarrh Whitby Date: Tue, 14 Aug 2018 14:42:30 -0230 Subject: [PATCH 2/4] Don't re-render the export modal when the selected identity changes --- .../modals/export-private-key-modal.js | 25 +++++++++++++------ 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/ui/app/components/modals/export-private-key-modal.js b/ui/app/components/modals/export-private-key-modal.js index 80ece425f..99b61bf9d 100644 --- a/ui/app/components/modals/export-private-key-modal.js +++ b/ui/app/components/modals/export-private-key-modal.js @@ -11,13 +11,21 @@ const ReadOnlyInput = require('../readonly-input') const copyToClipboard = require('copy-to-clipboard') const { checksumAddress } = require('../../util') -function mapStateToProps (state) { - return { - warning: state.appState.warning, - privateKey: state.appState.accountDetail.privateKey, - network: state.metamask.network, - selectedIdentity: getSelectedIdentity(state), - previousModalState: state.appState.modal.previousModalState.name, +function mapStateToPropsFactory () { + let selectedIdentity = null + return function mapStateToProps (state) { + // We should **not** change the identity displayed here even if it changes from underneath us. + // If we do, we will be showing the user one private key and a **different** address and name. + // Note that the selected identity **will** change from underneath us when we unlock the keyring + // which is the expected behavior that we are side-stepping. + selectedIdentity = selectedIdentity || getSelectedIdentity(state) + return { + warning: state.appState.warning, + privateKey: state.appState.accountDetail.privateKey, + network: state.metamask.network, + selectedIdentity, + previousModalState: state.appState.modal.previousModalState.name, + } } } @@ -43,7 +51,7 @@ ExportPrivateKeyModal.contextTypes = { t: PropTypes.func, } -module.exports = connect(mapStateToProps, mapDispatchToProps)(ExportPrivateKeyModal) +module.exports = connect(mapStateToPropsFactory, mapDispatchToProps)(ExportPrivateKeyModal) ExportPrivateKeyModal.prototype.exportAccountAndGetPrivateKey = function (password, address) { @@ -113,6 +121,7 @@ ExportPrivateKeyModal.prototype.render = function () { const { privateKey } = this.state return h(AccountModalContainer, { + selectedIdentity, showBackButton: previousModalState === 'ACCOUNT_DETAILS', backButtonAction: () => showAccountDetailModal(), }, [ From 003d445a98164dac0c0529dfd69f5e3987d7d05c Mon Sep 17 00:00:00 2001 From: Whymarrh Whitby Date: Thu, 16 Aug 2018 12:59:39 -0230 Subject: [PATCH 3/4] Update unlock logic to not overwrite existing selected address --- app/scripts/metamask-controller.js | 42 +++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 29838ad2d..4ee88186a 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -137,19 +137,7 @@ module.exports = class MetamaskController extends EventEmitter { encryptor: opts.encryptor || undefined, }) - // If only one account exists, make sure it is selected. - this.keyringController.memStore.subscribe((state) => { - const addresses = state.keyrings.reduce((res, keyring) => { - return res.concat(keyring.accounts) - }, []) - if (addresses.length === 1) { - const address = addresses[0] - this.preferencesController.setSelectedAddress(address) - } - // ensure preferences + identities controller know about all addresses - this.preferencesController.addAddresses(addresses) - this.accountTracker.syncWithAddresses(addresses) - }) + this.keyringController.memStore.subscribe((s) => this._onKeyringControllerUpdate(s)) // detect tokens controller this.detectTokensController = new DetectTokensController({ @@ -1278,6 +1266,34 @@ module.exports = class MetamaskController extends EventEmitter { ) } + /** + * Handle a KeyringController update + * @param {object} state the KC state + * @return {Promise} + * @private + */ + async _onKeyringControllerUpdate (state) { + const {isUnlocked, keyrings} = state + const addresses = keyrings.reduce((acc, {accounts}) => acc.concat(accounts), []) + + if (!addresses.length) { + return + } + + // Ensure preferences + identities controller know about all addresses + this.preferencesController.addAddresses(addresses) + this.accountTracker.syncWithAddresses(addresses) + + const wasLocked = !isUnlocked + if (wasLocked) { + const oldSelectedAddress = this.preferencesController.getSelectedAddress() + if (!addresses.includes(oldSelectedAddress)) { + const address = addresses[0] + await this.preferencesController.setSelectedAddress(address) + } + } + } + /** * A method for emitting the full MetaMask state to all registered listeners. * @private From e803b8e047b5acad9143fe28d99d9e7d65211f46 Mon Sep 17 00:00:00 2001 From: Whymarrh Whitby Date: Mon, 20 Aug 2018 12:07:44 -0230 Subject: [PATCH 4/4] Add test cases for MetaMaskController#_onKeyringControllerUpdate --- .../controllers/metamask-controller-test.js | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/test/unit/app/controllers/metamask-controller-test.js b/test/unit/app/controllers/metamask-controller-test.js index a798d41e2..85c78fe1e 100644 --- a/test/unit/app/controllers/metamask-controller-test.js +++ b/test/unit/app/controllers/metamask-controller-test.js @@ -814,6 +814,77 @@ describe('MetaMaskController', function () { }) }) + describe('#_onKeyringControllerUpdate', function () { + it('should do nothing if there are no keyrings in state', async function () { + const addAddresses = sinon.fake() + const syncWithAddresses = sinon.fake() + sandbox.replace(metamaskController, 'preferencesController', { + addAddresses, + }) + sandbox.replace(metamaskController, 'accountTracker', { + syncWithAddresses, + }) + + const oldState = metamaskController.getState() + await metamaskController._onKeyringControllerUpdate({keyrings: []}) + + assert.ok(addAddresses.notCalled) + assert.ok(syncWithAddresses.notCalled) + assert.deepEqual(metamaskController.getState(), oldState) + }) + + it('should update selected address if keyrings was locked', async function () { + const addAddresses = sinon.fake() + const getSelectedAddress = sinon.fake.returns('0x42') + const setSelectedAddress = sinon.fake() + const syncWithAddresses = sinon.fake() + sandbox.replace(metamaskController, 'preferencesController', { + addAddresses, + getSelectedAddress, + setSelectedAddress, + }) + sandbox.replace(metamaskController, 'accountTracker', { + syncWithAddresses, + }) + + const oldState = metamaskController.getState() + await metamaskController._onKeyringControllerUpdate({ + isUnlocked: false, + keyrings: [{ + accounts: ['0x1', '0x2'], + }], + }) + + assert.deepEqual(addAddresses.args, [[['0x1', '0x2']]]) + assert.deepEqual(syncWithAddresses.args, [[['0x1', '0x2']]]) + assert.deepEqual(setSelectedAddress.args, [['0x1']]) + assert.deepEqual(metamaskController.getState(), oldState) + }) + + it('should NOT update selected address if already unlocked', async function () { + const addAddresses = sinon.fake() + const syncWithAddresses = sinon.fake() + sandbox.replace(metamaskController, 'preferencesController', { + addAddresses, + }) + sandbox.replace(metamaskController, 'accountTracker', { + syncWithAddresses, + }) + + const oldState = metamaskController.getState() + await metamaskController._onKeyringControllerUpdate({ + isUnlocked: true, + keyrings: [{ + accounts: ['0x1', '0x2'], + }], + }) + + assert.deepEqual(addAddresses.args, [[['0x1', '0x2']]]) + assert.deepEqual(syncWithAddresses.args, [[['0x1', '0x2']]]) + assert.deepEqual(metamaskController.getState(), oldState) + }) + }) + }) function deferredPromise () {