From 63633635abaf092e071b3f29ff04f0b0643d0768 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Thu, 16 Apr 2020 15:20:01 -0300 Subject: [PATCH] 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 | 78 +++------- .../permissions/restrictedMethods.js | 20 ++- app/scripts/controllers/preferences.js | 11 +- app/scripts/metamask-controller.js | 1 - .../controllers/metamask-controller-test.js | 40 ++++- .../unit/app/controllers/permissions/mocks.js | 24 +-- .../permissions-controller-test.js | 139 +++-------------- .../permissions/restricted-methods-test.js | 140 ++++++++++++++++-- 8 files changed, 250 insertions(+), 203 deletions(-) diff --git a/app/scripts/controllers/permissions/index.js b/app/scripts/controllers/permissions/index.js index e1e23131d..b33349063 100644 --- a/app/scripts/controllers/permissions/index.js +++ b/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, + }) } /** diff --git a/app/scripts/controllers/permissions/restrictedMethods.js b/app/scripts/controllers/permissions/restrictedMethods.js index 73ac7d94e..717361d96 100644 --- a/app/scripts/controllers/permissions/restrictedMethods.js +++ b/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( diff --git a/app/scripts/controllers/preferences.js b/app/scripts/controllers/preferences.js index a8fc7df6d..facb65ebf 100644 --- a/app/scripts/controllers/preferences.js +++ b/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) } diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 3e8777942..5f01893d2 100644 --- a/app/scripts/metamask-controller.js +++ b/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()), diff --git a/test/unit/app/controllers/metamask-controller-test.js b/test/unit/app/controllers/metamask-controller-test.js index 6c82fccf3..f3b92872c 100644 --- a/test/unit/app/controllers/metamask-controller-test.js +++ b/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 }, }) diff --git a/test/unit/app/controllers/permissions/mocks.js b/test/unit/app/controllers/permissions/mocks.js index 8bdce55c6..7e44dc62b 100644 --- a/test/unit/app/controllers/permissions/mocks.js +++ b/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', diff --git a/test/unit/app/controllers/permissions/permissions-controller-test.js b/test/unit/app/controllers/permissions/permissions-controller-test.js index db5fb5fae..ac3b54865 100644 --- a/test/unit/app/controllers/permissions/permissions-controller-test.js +++ b/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] diff --git a/test/unit/app/controllers/permissions/restricted-methods-test.js b/test/unit/app/controllers/permissions/restricted-methods-test.js index 26f477bed..6b95cdb69 100644 --- a/test/unit/app/controllers/permissions/restricted-methods-test.js +++ b/test/unit/app/controllers/permissions/restricted-methods-test.js @@ -1,35 +1,147 @@ 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') + await assert.rejects( + ethAccountsMethod(null, res, null), + fooError, + 'Should reject with expected error' + ) - const fooError = new Error('foo') + assert.deepEqual( + res, { error: fooError }, + 'response should have expected error and no result' + ) + }) - assert.deepEqual( - err, fooError, - 'should end with expected error' - ) + 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) - assert.deepEqual( - res, { error: fooError }, - 'response should have expected error and no result' - ) + 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') }) }) })