Merge pull request #5066 from whymarrh/fix-key-export

Don't re-render the export modal when the selected identity changes

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((s) => this._onKeyringControllerUpdate(s))
-    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)
-    })
 
     // 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<void>}
+   * @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

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 () {

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),
   }
 }
 

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) {
+function mapStateToPropsFactory () {
-  return {
+  let selectedIdentity = null
-    warning: state.appState.warning,
+  return function mapStateToProps (state) {
-    privateKey: state.appState.accountDetail.privateKey,
+    // We should **not** change the identity displayed here even if it changes from underneath us.
-    network: state.metamask.network,
+    // If we do, we will be showing the user one private key and a **different** address and name.
-    selectedIdentity: getSelectedIdentity(state),
+    // Note that the selected identity **will** change from underneath us when we unlock the keyring
-    previousModalState: state.appState.modal.previousModalState.name,
+    // 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(),
   }, [