Fix order of accounts in `eth_accounts` response (#8342)

* Fix order of accounts in `eth_accounts` response

The accounts returned by `eth_accounts` were in a fixed order - the
order in which the keyring returned them - rather than ordered with the
selected account first. The accounts returned by the `accountsChanged`
event were ordered with the selected account first, but the same order
wasn't used for `eth_accounts`.

We needed to store additional state in order to determine the correct
account order correctly on all dapps. We had only been storing the
current selected account, but since we also need to determine the
primary account per dapp (i.e. the last "selected" account among the
accounts exposed to that dapp), that wasn't enough.

A `lastSelected` property has been added to each identity in the
preferences controller to keep track of the last selected time. This
property is set to the current time (in milliseconds) whenever a new
selection is made. The accounts returned with `accountsChanged` and by
`eth_accounts` are both ordered by this property.

The `updatePermittedAccounts` function was merged with the internal
methods for responding to account selection, to keep things simpler. It
wasn't called externally anyway, so it wasn't needed in the public API.

* Remove caveat update upon change in selected account

The order of accounts in the caveat isn't meaningful, so the caveat
doesn't need to be updated when the accounts get re-ordered.

* Emit event regardless of account order

Now that we're no longer relying upon the caveat for the account order,
we also have no way of knowing if a particular account selection
resulted in a change in order or not. The notification is now emitted
whenever an exposed account is selected - even if the order stayed the
same.

The inpage provider currently caches the account order, so it can be
relied upon to ignore these redundant events. We were already emiting
redundant `accountsChanged` events in some cases anyway.

app/scripts/controllers/permissions/index.js

@@ -47,7 +47,10 @@ export class PermissionsController {
     this.notifyAllDomains = notifyAllDomains
     this._showPermissionRequest = showPermissionRequest
 
-    this._restrictedMethods = getRestrictedMethods(this)
+    this._restrictedMethods = getRestrictedMethods({
+      getKeyringAccounts: this.getKeyringAccounts.bind(this),
+      getIdentities: this._getIdentities.bind(this),
+    })
     this.permissionsLog = new PermissionsLogController({
       restrictedMethods: Object.keys(this._restrictedMethods),
       store: this.store,
@@ -56,6 +59,7 @@ export class PermissionsController {
     this.pendingApprovalOrigins = new Set()
     this._initializePermissions(restoredPermissions)
     this._lastSelectedAddress = preferences.getState().selectedAddress
+    this.preferences = preferences
 
     preferences.subscribe(async ({ selectedAddress }) => {
       if (selectedAddress && selectedAddress !== this._lastSelectedAddress) {
@@ -139,6 +143,15 @@ export class PermissionsController {
     return Boolean(this.permissions.getPermission(origin, permission))
   }
 
+  /**
+   * Gets the identities from the preferences controller store
+   *
+   * @returns {Object} identities
+   */
+  _getIdentities () {
+    return this.preferences.getState().identities
+  }
+
   /**
    * Submits a permissions request to rpc-cap. Internal, background use only.
    *
@@ -293,31 +306,6 @@ export class PermissionsController {
     }
   }
 
-  /**
-   * Update the accounts exposed to the given origin. Changes the eth_accounts
-   * permissions and emits accountsChanged.
-   * At least one account must be exposed. If no accounts are to be exposed, the
-   * eth_accounts permissions should be removed completely.
-   *
-   * Throws error if the update fails.
-   *
-   * @param {string} origin - The origin to change the exposed accounts for.
-   * @param {string[]} accounts - The new account(s) to expose.
-   */
-  async updatePermittedAccounts (origin, accounts) {
-
-    await this.validatePermittedAccounts(accounts)
-
-    this.permissions.updateCaveatFor(
-      origin, 'eth_accounts', CAVEAT_NAMES.exposedAccounts, accounts
-    )
-
-    this.notifyDomain(origin, {
-      method: NOTIFICATION_NAMES.accountsChanged,
-      result: accounts,
-    })
-  }
-
   /**
    * Finalizes a permissions request. Throws if request validation fails.
    * Clones the passed-in parameters to prevent inadvertent modification.
@@ -434,7 +422,7 @@ export class PermissionsController {
 
   /**
    * When a new account is selected in the UI, emit accountsChanged to each origin
-   * where the account order has changed.
+   * where the selected account is exposed.
    *
    * Note: This will emit "false positive" accountsChanged events, but they are
    * handled by the inpage provider.
@@ -461,46 +449,26 @@ export class PermissionsController {
     await Promise.all(
       connectedDomains
         .map(
-          (origin) => this._handleConnectedAccountSelected(origin, account)
+          (origin) => this._handleConnectedAccountSelected(origin)
         )
     )
   }
 
   /**
-   * When a new account is selected in the UI for 'origin', emit accountsChanged
-   * to 'origin' if the selected account is permitted.
+   * When a new account is selected in the UI, emit accountsChanged to 'origin'
    *
    * Note: This will emit "false positive" accountsChanged events, but they are
    * handled by the inpage provider.
    *
-   * @param {string} origin - The origin.
-   * @param {string} account - The newly selected account's address.
+   * @param {string} origin - The origin
    */
-  async _handleConnectedAccountSelected (origin, account) {
+  async _handleConnectedAccountSelected (origin) {
     const permittedAccounts = await this.getAccounts(origin)
 
-    if (
-      typeof origin !== 'string' || !origin.length
-    ) {
-      throw new Error('Origin should be a non-empty string.')
-    }
-
-    // do nothing if the account is not permitted for the origin, or
-    // if it's already first in the array of permitted accounts
-    if (
-      !permittedAccounts.includes(account) ||
-      permittedAccounts[0] === account
-    ) {
-      return
-    }
-
-    const newPermittedAccounts = [account].concat(
-      permittedAccounts.filter((_account) => _account !== account)
-    )
-
-    // update permitted accounts to ensure that accounts are returned
-    // in the same order every time
-    await this.updatePermittedAccounts(origin, newPermittedAccounts)
+    this.notifyDomain(origin, {
+      method: NOTIFICATION_NAMES.accountsChanged,
+      result: permittedAccounts,
+    })
   }
 
   /**

app/scripts/controllers/permissions/restrictedMethods.js

@@ -1,12 +1,28 @@
-export default function getRestrictedMethods (permissionsController) {
+export default function getRestrictedMethods ({ getIdentities, getKeyringAccounts }) {
   return {
 
     'eth_accounts': {
       description: `View the addresses of the user's chosen accounts.`,
       method: (_, res, __, end) => {
-        permissionsController.getKeyringAccounts()
+        getKeyringAccounts()
           .then((accounts) => {
+            const identities = getIdentities()
             res.result = accounts
+              .sort((firstAddress, secondAddress) => {
+                if (!identities[firstAddress]) {
+                  throw new Error(`Missing identity for address ${firstAddress}`)
+                } else if (!identities[secondAddress]) {
+                  throw new Error(`Missing identity for address ${secondAddress}`)
+                } else if (identities[firstAddress].lastSelected === identities[secondAddress].lastSelected) {
+                  return 0
+                } else if (identities[firstAddress].lastSelected === undefined) {
+                  return 1
+                } else if (identities[secondAddress].lastSelected === undefined) {
+                  return -1
+                }
+
+                return identities[secondAddress].lastSelected - identities[firstAddress].lastSelected
+              })
             end()
           })
           .catch(

app/scripts/controllers/preferences.js

@@ -370,8 +370,15 @@ class PreferencesController {
   setSelectedAddress (_address) {
     const address = normalizeAddress(_address)
     this._updateTokens(address)
-    this.store.updateState({ selectedAddress: address })
-    const tokens = this.store.getState().tokens
+
+    const { identities, tokens } = this.store.getState()
+    const selectedIdentity = identities[address]
+    if (!selectedIdentity) {
+      throw new Error(`Identity for '${address} not found`)
+    }
+
+    selectedIdentity.lastSelected = Date.now()
+    this.store.updateState({ identities, selectedAddress: address })
     return Promise.resolve(tokens)
   }
 

app/scripts/metamask-controller.js

@@ -576,7 +576,6 @@ export default class MetamaskController extends EventEmitter {
       getApprovedAccounts: nodeify(permissionsController.getAccounts.bind(permissionsController)),
       rejectPermissionsRequest: nodeify(permissionsController.rejectPermissionsRequest, permissionsController),
       removePermissionsFor: permissionsController.removePermissionsFor.bind(permissionsController),
-      updatePermittedAccounts: nodeify(permissionsController.updatePermittedAccounts, permissionsController),
       legacyExposeAccounts: nodeify(permissionsController.legacyExposeAccounts, permissionsController),
 
       getRequestAccountTabIds: (cb) => cb(null, this.getRequestAccountTabIds()),

test/unit/app/controllers/metamask-controller-test.js

@@ -243,18 +243,45 @@ describe('MetaMaskController', function () {
         return Promise.resolve('0x0')
       })
 
+      let startTime = Date.now()
       await metamaskController.createNewVaultAndRestore('foobar1337', TEST_SEED)
-      assert.deepEqual(metamaskController.getState().identities, {
+      let endTime = Date.now()
+
+      const firstVaultIdentities = cloneDeep(metamaskController.getState().identities)
+      assert.ok(
+        (
+          firstVaultIdentities[TEST_ADDRESS].lastSelected >= startTime &&
+          firstVaultIdentities[TEST_ADDRESS].lastSelected <= endTime
+        ),
+        `'${firstVaultIdentities[TEST_ADDRESS].lastSelected}' expected to be between '${startTime}' and '${endTime}'`
+      )
+      delete firstVaultIdentities[TEST_ADDRESS].lastSelected
+      assert.deepEqual(firstVaultIdentities, {
         [TEST_ADDRESS]: { address: TEST_ADDRESS, name: DEFAULT_LABEL },
       })
 
       await metamaskController.preferencesController.setAccountLabel(TEST_ADDRESS, 'Account Foo')
-      assert.deepEqual(metamaskController.getState().identities, {
+
+      const labelledFirstVaultIdentities = cloneDeep(metamaskController.getState().identities)
+      delete labelledFirstVaultIdentities[TEST_ADDRESS].lastSelected
+      assert.deepEqual(labelledFirstVaultIdentities, {
         [TEST_ADDRESS]: { address: TEST_ADDRESS, name: 'Account Foo' },
       })
 
+      startTime = Date.now()
       await metamaskController.createNewVaultAndRestore('foobar1337', TEST_SEED_ALT)
-      assert.deepEqual(metamaskController.getState().identities, {
+      endTime = Date.now()
+
+      const secondVaultIdentities = cloneDeep(metamaskController.getState().identities)
+      assert.ok(
+        (
+          secondVaultIdentities[TEST_ADDRESS_ALT].lastSelected >= startTime &&
+          secondVaultIdentities[TEST_ADDRESS_ALT].lastSelected <= endTime
+        ),
+        `'${secondVaultIdentities[TEST_ADDRESS_ALT].lastSelected}' expected to be between '${startTime}' and '${endTime}'`
+      )
+      delete secondVaultIdentities[TEST_ADDRESS_ALT].lastSelected
+      assert.deepEqual(secondVaultIdentities, {
         [TEST_ADDRESS_ALT]: { address: TEST_ADDRESS_ALT, name: DEFAULT_LABEL },
       })
     })
@@ -271,8 +298,13 @@ describe('MetaMaskController', function () {
         return Promise.resolve('0x14ced5122ce0a000')
       })
 
+      const startTime = Date.now()
       await metamaskController.createNewVaultAndRestore('foobar1337', TEST_SEED)
-      assert.deepEqual(metamaskController.getState().identities, {
+
+      const identities = cloneDeep(metamaskController.getState().identities)
+      assert.ok(identities[TEST_ADDRESS].lastSelected >= startTime && identities[TEST_ADDRESS].lastSelected <= Date.now())
+      delete identities[TEST_ADDRESS].lastSelected
+      assert.deepEqual(identities, {
         [TEST_ADDRESS]: { address: TEST_ADDRESS, name: DEFAULT_LABEL },
         [TEST_ADDRESS_2]: { address: TEST_ADDRESS_2, name: DEFAULT_LABEL_2 },
       })

test/unit/app/controllers/permissions/mocks.js

@@ -34,6 +34,16 @@ const keyringAccounts = deepFreeze([
 
 const getKeyringAccounts = async () => [ ...keyringAccounts ]
 
+const getIdentities = () => {
+  return keyringAccounts.reduce(
+    (identities, address, index) => {
+      identities[address] = { address, name: `Account ${index}` }
+      return identities
+    },
+    {}
+  )
+}
+
 // perm controller initialization helper
 const getRestrictedMethods = (permController) => {
   return {
@@ -73,7 +83,10 @@ export function getPermControllerOpts () {
     notifyAllDomains: noop,
     preferences: {
       getState: () => {
-        return { selectedAddress: keyringAccounts[0] }
+        return {
+          identities: getIdentities(),
+          selectedAddress: keyringAccounts[0],
+        }
       },
       subscribe: noop,
     },
@@ -317,14 +330,6 @@ export const getters = deepFreeze({
       },
     },
 
-    updatePermittedAccounts: {
-      invalidOrigin: () => {
-        return {
-          message: 'No such permission exists for the given domain.',
-        }
-      },
-    },
-
     legacyExposeAccounts: {
       badOrigin: () => {
         return {
@@ -584,6 +589,7 @@ export const getters = deepFreeze({
  * Objects with immutable mock values.
  */
 export const constants = deepFreeze({
+  ALL_ACCOUNTS: keyringAccounts,
 
   DUMMY_ACCOUNT: '0xabc',
 

test/unit/app/controllers/permissions/permissions-controller-test.js

@@ -33,6 +33,7 @@ const {
 } = getters
 
 const {
+  ALL_ACCOUNTS,
   ACCOUNT_ARRAYS,
   DUMMY_ACCOUNT,
   ORIGINS,
@@ -460,115 +461,6 @@ describe('permissions controller', function () {
     })
   })
 
-  describe('updatePermittedAccounts', function () {
-
-    let permController, notifications
-
-    beforeEach(function () {
-      notifications = initNotifications()
-      permController = initPermController(notifications)
-      grantPermissions(
-        permController, ORIGINS.a,
-        PERMS.finalizedRequests.eth_accounts(ACCOUNT_ARRAYS.a)
-      )
-      grantPermissions(
-        permController, ORIGINS.b,
-        PERMS.finalizedRequests.eth_accounts(ACCOUNT_ARRAYS.b)
-      )
-    })
-
-    it('throws on invalid accounts', async function () {
-
-      await assert.rejects(
-        permController.updatePermittedAccounts(ORIGINS.a, {}),
-        ERRORS.validatePermittedAccounts.invalidParam(),
-        'should throw on non-array accounts param'
-      )
-
-      await assert.rejects(
-        permController.updatePermittedAccounts(ORIGINS.a, []),
-        ERRORS.validatePermittedAccounts.invalidParam(),
-        'should throw on empty array accounts param'
-      )
-
-      await assert.rejects(
-        permController.updatePermittedAccounts(ORIGINS.a, [DUMMY_ACCOUNT]),
-        ERRORS.validatePermittedAccounts.nonKeyringAccount(DUMMY_ACCOUNT),
-        'should throw on non-keyring account'
-      )
-    })
-
-    it('throws if origin invalid or lacks eth_accounts permission', async function () {
-
-      await assert.rejects(
-        permController.updatePermittedAccounts(false, ACCOUNT_ARRAYS.a),
-        ERRORS.updatePermittedAccounts.invalidOrigin(),
-        'should throw on invalid origin'
-      )
-
-      await assert.rejects(
-        permController.updatePermittedAccounts(ORIGINS.c, ACCOUNT_ARRAYS.a),
-        ERRORS.updatePermittedAccounts.invalidOrigin(),
-        'should throw on origin without eth_accounts permission'
-      )
-    })
-
-    it('successfully updates permitted accounts', async function () {
-
-      await permController.updatePermittedAccounts(ORIGINS.a, ACCOUNT_ARRAYS.b)
-      await permController.updatePermittedAccounts(ORIGINS.b, ACCOUNT_ARRAYS.c)
-
-      let aAccounts = await permController.getAccounts(ORIGINS.a)
-      let bAccounts = await permController.getAccounts(ORIGINS.b)
-
-      assert.deepEqual(
-        aAccounts, ACCOUNT_ARRAYS.b,
-        'first origin should have correct accounts'
-      )
-      assert.deepEqual(
-        bAccounts, ACCOUNT_ARRAYS.c,
-        'first origin should have correct accounts'
-      )
-
-      assert.deepEqual(
-        notifications[ORIGINS.a][0],
-        NOTIFICATIONS.newAccounts(ACCOUNT_ARRAYS.b),
-        'first origin should have correct notification'
-      )
-      assert.deepEqual(
-        notifications[ORIGINS.b][0],
-        NOTIFICATIONS.newAccounts(ACCOUNT_ARRAYS.c),
-        'second origin should have correct notification'
-      )
-
-      await permController.updatePermittedAccounts(ORIGINS.a, ACCOUNT_ARRAYS.c)
-      await permController.updatePermittedAccounts(ORIGINS.b, ACCOUNT_ARRAYS.a)
-
-      aAccounts = await permController.getAccounts(ORIGINS.a)
-      bAccounts = await permController.getAccounts(ORIGINS.b)
-
-      assert.deepEqual(
-        aAccounts, ACCOUNT_ARRAYS.c,
-        'first origin should have correct accounts'
-      )
-      assert.deepEqual(
-        bAccounts, ACCOUNT_ARRAYS.a,
-        'first origin should have correct accounts'
-      )
-
-      assert.deepEqual(
-        notifications[ORIGINS.a][1],
-        NOTIFICATIONS.newAccounts(ACCOUNT_ARRAYS.c),
-        'first origin should have correct notification'
-      )
-      assert.deepEqual(
-        notifications[ORIGINS.b][1],
-        NOTIFICATIONS.newAccounts(ACCOUNT_ARRAYS.a),
-        'second origin should have correct notification'
-      )
-    })
-  })
-
   describe('finalizePermissionsRequest', function () {
 
     let permController
@@ -740,15 +632,24 @@ describe('permissions controller', function () {
 
   describe('preferences state update', function () {
 
-    let permController, notifications, preferences
+    let permController, notifications, preferences, identities
 
     beforeEach(function () {
-      preferences = {
-        getState: () => {
-          return { selectedAddress: DUMMY_ACCOUNT }
+      identities = ALL_ACCOUNTS.reduce(
+        (identities, account) => {
+          identities[account] = {}
+          return identities
         },
+        {}
+      )
+      preferences = {
+        getState: sinon.stub(),
         subscribe: sinon.stub(),
       }
+      preferences.getState.returns({
+        identities,
+        selectedAddress: DUMMY_ACCOUNT,
+      })
       notifications = initNotifications()
       permController = new PermissionsController({
         ...getPermControllerOpts(),
@@ -767,6 +668,7 @@ describe('permissions controller', function () {
     })
 
     it('should throw if given invalid account', async function () {
+
       assert(preferences.subscribe.calledOnce)
       assert(preferences.subscribe.firstCall.args.length === 1)
       const onPreferencesUpdate = preferences.subscribe.firstCall.args[0]
@@ -795,7 +697,8 @@ describe('permissions controller', function () {
       )
     })
 
-    it('should do nothing if account already first in array for each connected site', async function () {
+    it('should emit notification if account already first in array for each connected site', async function () {
+      identities[ACCOUNT_ARRAYS.a[0]] = { lastSelected: 1000 }
       assert(preferences.subscribe.calledOnce)
       assert(preferences.subscribe.firstCall.args.length === 1)
       const onPreferencesUpdate = preferences.subscribe.firstCall.args[0]
@@ -803,16 +706,19 @@ describe('permissions controller', function () {
       await onPreferencesUpdate({ selectedAddress: ACCOUNT_ARRAYS.a[0] })
 
       assert.deepEqual(
-        notifications[ORIGINS.b], [],
+        notifications[ORIGINS.b],
+        [NOTIFICATIONS.newAccounts([...ACCOUNT_ARRAYS.a, EXTRA_ACCOUNT])],
         'should not have emitted notification'
       )
       assert.deepEqual(
-        notifications[ORIGINS.c], [],
+        notifications[ORIGINS.c],
+        [NOTIFICATIONS.newAccounts(ACCOUNT_ARRAYS.a)],
         'should not have emitted notification'
       )
     })
 
     it('should emit notification just for connected domains', async function () {
+      identities[EXTRA_ACCOUNT] = { lastSelected: 1000 }
       assert(preferences.subscribe.calledOnce)
       assert(preferences.subscribe.firstCall.args.length === 1)
       const onPreferencesUpdate = preferences.subscribe.firstCall.args[0]
@@ -831,6 +737,7 @@ describe('permissions controller', function () {
     })
 
     it('should emit notification for multiple connected domains', async function () {
+      identities[ACCOUNT_ARRAYS.a[1]] = { lastSelected: 1000 }
       assert(preferences.subscribe.calledOnce)
       assert(preferences.subscribe.firstCall.args.length === 1)
       const onPreferencesUpdate = preferences.subscribe.firstCall.args[0]

test/unit/app/controllers/permissions/restricted-methods-test.js

@@ -1,28 +1,25 @@
 import { strict as assert } from 'assert'
+import pify from 'pify'
 
 import getRestrictedMethods
   from '../../../../../app/scripts/controllers/permissions/restrictedMethods'
 
 describe('restricted methods', function () {
-
-  // this method is tested extensively in other permissions tests
   describe('eth_accounts', function () {
-
-    it('handles failure', async function () {
+    it('should handle getKeyringAccounts error', async function () {
       const restrictedMethods = getRestrictedMethods({
         getKeyringAccounts: async () => {
           throw new Error('foo')
         },
       })
+      const ethAccountsMethod = pify(restrictedMethods.eth_accounts.method)
 
       const res = {}
-      restrictedMethods.eth_accounts.method(null, res, null, (err) => {
-
       const fooError = new Error('foo')
-
-        assert.deepEqual(
-          err, fooError,
-          'should end with expected error'
+      await assert.rejects(
+        ethAccountsMethod(null, res, null),
+        fooError,
+        'Should reject with expected error'
       )
 
       assert.deepEqual(
@@ -30,6 +27,121 @@ describe('restricted methods', function () {
         'response should have expected error and no result'
       )
     })
+
+    it('should handle missing identity for first account when sorting', async function () {
+      const restrictedMethods = getRestrictedMethods({
+        getIdentities: () => {
+          return { '0x7e57e2': {} }
+        },
+        getKeyringAccounts: async () => ['0x7e57e2', '0x7e57e3'],
+      })
+      const ethAccountsMethod = pify(restrictedMethods.eth_accounts.method)
+
+      const res = {}
+      await assert.rejects(ethAccountsMethod(null, res, null))
+      assert.ok(res.error instanceof Error, 'result should have error')
+      assert.deepEqual(Object.keys(res), ['error'], 'result should only contain error')
+    })
+
+    it('should handle missing identity for second account when sorting', async function () {
+      const restrictedMethods = getRestrictedMethods({
+        getIdentities: () => {
+          return { '0x7e57e3': {} }
+        },
+        getKeyringAccounts: async () => ['0x7e57e2', '0x7e57e3'],
+      })
+      const ethAccountsMethod = pify(restrictedMethods.eth_accounts.method)
+
+      const res = {}
+      await assert.rejects(ethAccountsMethod(null, res, null))
+      assert.ok(res.error instanceof Error, 'result should have error')
+      assert.deepEqual(Object.keys(res), ['error'], 'result should only contain error')
+    })
+
+    it('should return accounts in keyring order when none are selected', async function () {
+      const keyringAccounts = ['0x7e57e2', '0x7e57e3', '0x7e57e4', '0x7e57e5']
+      const restrictedMethods = getRestrictedMethods({
+        getIdentities: () => {
+          return keyringAccounts.reduce(
+            (identities, address) => {
+              identities[address] = {}
+              return identities
+            },
+            {}
+          )
+        },
+        getKeyringAccounts: async () => [...keyringAccounts],
+      })
+      const ethAccountsMethod = pify(restrictedMethods.eth_accounts.method)
+
+      const res = {}
+      await ethAccountsMethod(null, res, null)
+      assert.deepEqual(res, { result: keyringAccounts }, 'should return accounts in correct order')
+    })
+
+    it('should return accounts in keyring order when all have same last selected time', async function () {
+      const keyringAccounts = ['0x7e57e2', '0x7e57e3', '0x7e57e4', '0x7e57e5']
+      const restrictedMethods = getRestrictedMethods({
+        getIdentities: () => {
+          return keyringAccounts.reduce(
+            (identities, address) => {
+              identities[address] = { lastSelected: 1000 }
+              return identities
+            },
+            {}
+          )
+        },
+        getKeyringAccounts: async () => [...keyringAccounts],
+      })
+      const ethAccountsMethod = pify(restrictedMethods.eth_accounts.method)
+
+      const res = {}
+      await ethAccountsMethod(null, res, null)
+      assert.deepEqual(res, { result: keyringAccounts }, 'should return accounts in correct order')
+    })
+
+    it('should return accounts sorted by last selected (descending)', async function () {
+      const keyringAccounts = ['0x7e57e2', '0x7e57e3', '0x7e57e4', '0x7e57e5']
+      const expectedResult = keyringAccounts.slice().reverse()
+      const restrictedMethods = getRestrictedMethods({
+        getIdentities: () => {
+          return keyringAccounts.reduce(
+            (identities, address, index) => {
+              identities[address] = { lastSelected: index * 1000 }
+              return identities
+            },
+            {}
+          )
+        },
+        getKeyringAccounts: async () => [...keyringAccounts],
+      })
+      const ethAccountsMethod = pify(restrictedMethods.eth_accounts.method)
+
+      const res = {}
+      await ethAccountsMethod(null, res, null)
+      assert.deepEqual(res, { result: expectedResult }, 'should return accounts in correct order')
+    })
+
+    it('should return accounts sorted by last selected (descending) with unselected accounts last, in keyring order', async function () {
+      const keyringAccounts = ['0x7e57e2', '0x7e57e3', '0x7e57e4', '0x7e57e5', '0x7e57e6']
+      const expectedResult = ['0x7e57e4', '0x7e57e2', '0x7e57e3', '0x7e57e5', '0x7e57e6']
+      const restrictedMethods = getRestrictedMethods({
+        getIdentities: () => {
+          return {
+            '0x7e57e2': { lastSelected: 1000 },
+            '0x7e57e3': {},
+            '0x7e57e4': { lastSelected: 2000 },
+            '0x7e57e5': {},
+            '0x7e57e6': {},
+          }
+        },
+        getKeyringAccounts: async () => [...keyringAccounts],
+      })
+      const ethAccountsMethod = pify(restrictedMethods.eth_accounts.method)
+
+      const res = {}
+      await ethAccountsMethod(null, res, null)
+      assert.deepEqual(res, { result: expectedResult }, 'should return accounts in correct order')
     })
   })
 })