From adff1a27edd7420c0605df357b37f23f3b150c9e Mon Sep 17 00:00:00 2001 From: Shane Terence Odlum Date: Wed, 9 Apr 2025 11:58:48 +0100 Subject: [PATCH 01/11] chore: add metadata to account controller options for EVM HD accounts --- .../src/AccountsController.ts | 25 ++++++++++++++++--- packages/accounts-controller/src/utils.ts | 20 +++++++++++++++ 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index 78dc836634b..98b9fb23af5 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -38,7 +38,9 @@ import type { WritableDraft } from 'immer/dist/internal.js'; import type { MultichainNetworkControllerNetworkDidChangeEvent } from './types'; import { + getDerivationPathForIndex, getUUIDFromAddressOfNormalAccount, + isHdKeyringType, isNormalKeyringType, keyringTypeToName, } from './utils'; @@ -632,19 +634,34 @@ export class AccountsController extends BaseController< */ async #listNormalAccounts(): Promise { const internalAccounts: InternalAccount[] = []; - const { keyrings } = await this.messagingSystem.call( + const { keyrings, keyringsMetadata } = await this.messagingSystem.call( 'KeyringController:getState', ); - for (const keyring of keyrings) { + for (let keyringIndex = 0; keyringIndex < keyrings.length; keyringIndex++) { + const keyring = keyrings[keyringIndex]; const keyringType = keyring.type; if (!isNormalKeyringType(keyringType as KeyringTypes)) { // We only consider "normal accounts" here, so keep looping continue; } - for (const address of keyring.accounts) { + for ( + let accountIndex = 0; + accountIndex < keyring.accounts.length; + accountIndex++ + ) { + const address = keyring.accounts[accountIndex]; const id = getUUIDFromAddressOfNormalAccount(address); + let options = {}; + + if (isHdKeyringType(keyring.type as KeyringTypes)) { + options = { + entropySource: keyringsMetadata[keyringIndex].id, + derivationPath: getDerivationPathForIndex(accountIndex), + }; + } + const nameLastUpdatedAt = this.#populateExistingMetadata( id, 'nameLastUpdatedAt', @@ -653,7 +670,7 @@ export class AccountsController extends BaseController< internalAccounts.push({ id, address, - options: {}, + options, methods: [ EthMethod.PersonalSign, EthMethod.Sign, diff --git a/packages/accounts-controller/src/utils.ts b/packages/accounts-controller/src/utils.ts index 0c4e89a0662..48f7da6be9d 100644 --- a/packages/accounts-controller/src/utils.ts +++ b/packages/accounts-controller/src/utils.ts @@ -85,3 +85,23 @@ export function isNormalKeyringType(keyringType: KeyringTypes): boolean { // adapted later on if we have new kind of keyrings! return keyringType !== KeyringTypes.snap; } + +/** + * Check if a keyring is a HD keyring. + * + * @param keyringType - The account's keyring type. + * @returns True if the keyring is a HD keyring, false otherwise. + */ +export function isHdKeyringType(keyringType: KeyringTypes): boolean { + return keyringType === KeyringTypes.hd; +} + +/** + * Get the derivation path for the index of an account within a HD keyring. + * + * @param index - The account index. + * @returns The derivation path. + */ +export function getDerivationPathForIndex(index: number): string { + return `m/44'/60'/0'/0/${index}`; +} From 692a3d4e5943f19c658f477311e344d23bc24cc2 Mon Sep 17 00:00:00 2001 From: Shane Terence Odlum Date: Wed, 16 Apr 2025 17:20:16 +0100 Subject: [PATCH 02/11] chore: fix tests --- .../src/AccountsController.test.ts | 68 ++++++++++++++++++- .../src/AccountsController.ts | 3 + .../accounts-controller/src/tests/mocks.ts | 19 +++++- 3 files changed, 87 insertions(+), 3 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.test.ts b/packages/accounts-controller/src/AccountsController.test.ts index 42a95161d45..c9950b53eff 100644 --- a/packages/accounts-controller/src/AccountsController.test.ts +++ b/packages/accounts-controller/src/AccountsController.test.ts @@ -31,6 +31,7 @@ import { AccountsController, EMPTY_ACCOUNT } from './AccountsController'; import { createExpectedInternalAccount, createMockInternalAccount, + createMockInternalAccountOptions, ETH_EOA_METHODS, } from './tests/mocks'; import { @@ -901,6 +902,7 @@ describe('AccountsController', () => { name: 'Account 3', address: mockAccount3.address, keyringType: KeyringTypes.hd, + options: {}, }), ]); }); @@ -1744,6 +1746,12 @@ describe('AccountsController', () => { keyrings: [ { type: KeyringTypes.hd, accounts: [mockAddress1, mockAddress2] }, ], + keyringsMetadata: [ + { + id: 'mock-keyring-id-0', + name: 'mock-keyring-id-name', + }, + ], }), ); @@ -1772,12 +1780,14 @@ describe('AccountsController', () => { id: 'mock-id', address: mockAddress1, keyringType: KeyringTypes.hd, + options: createMockInternalAccountOptions(0, KeyringTypes.hd, 0), }), createExpectedInternalAccount({ name: 'Account 2', id: 'mock-id2', address: mockAddress2, keyringType: KeyringTypes.hd, + options: createMockInternalAccountOptions(0, KeyringTypes.hd, 1), }), ]; mockUUIDWithNormalAccounts(expectedAccounts); @@ -1800,6 +1810,12 @@ describe('AccountsController', () => { accounts: [mockSnapAccount, mockSnapAccount2], }, ], + keyringsMetadata: [ + { + id: 'mock-keyring-id-1', + name: 'mock-keyring-id-name', + }, + ], }), ); @@ -1893,6 +1909,12 @@ describe('AccountsController', () => { keyrings: [ { type: KeyringTypes.hd, accounts: [mockAddress1, mockAddress2] }, ], + keyringsMetadata: [ + { + id: 'mock-keyring-id-0', + name: 'mock-keyring-id-name', + }, + ], }), ); @@ -1918,12 +1940,16 @@ describe('AccountsController', () => { messenger, }); const expectedAccounts = [ - mockAccount, + { + ...mockAccount, + options: createMockInternalAccountOptions(0, KeyringTypes.hd, 0), + }, createExpectedInternalAccount({ name: 'Account 2', id: 'mock-id2', address: mockAddress2, keyringType: KeyringTypes.hd, + options: createMockInternalAccountOptions(0, KeyringTypes.hd, 1), }), ]; mockUUIDWithNormalAccounts(expectedAccounts); @@ -1957,6 +1983,16 @@ describe('AccountsController', () => { { type: KeyringTypes.hd, accounts: [mockAddress1] }, { type: KeyringTypes.snap, accounts: ['0x1234'] }, ], + keyringsMetadata: [ + { + id: 'mock-keyring-id-0', + name: 'mock-keyring-id-name', + }, + { + id: 'mock-keyring-id-1', + name: 'mock-keyring-id-name2', + }, + ], }), ); @@ -1975,6 +2011,7 @@ describe('AccountsController', () => { id: 'mock-id', address: mockAddress1, keyringType: KeyringTypes.hd, + options: createMockInternalAccountOptions(0, KeyringTypes.hd, 0), }), createExpectedInternalAccount({ name: 'Snap Account 1', // it is Snap Account 1 because it is the only snap account @@ -2014,6 +2051,16 @@ describe('AccountsController', () => { { type: KeyringTypes.snap, accounts: ['0x1234'] }, { type: KeyringTypes.hd, accounts: [mockAddress1] }, ], + keyringsMetadata: [ + { + id: 'mock-keyring-id-0', + name: 'mock-keyring-id-name', + }, + { + id: 'mock-keyring-id-1', + name: 'mock-keyring-id-name2', + }, + ], }), ); @@ -2032,6 +2079,7 @@ describe('AccountsController', () => { id: 'mock-id', address: mockAddress1, keyringType: KeyringTypes.hd, + options: createMockInternalAccountOptions(1, KeyringTypes.hd, 0), }), createExpectedInternalAccount({ name: 'Snap Account 1', // it is Snap Account 1 because it is the only snap account @@ -2057,7 +2105,6 @@ describe('AccountsController', () => { KeyringTypes.ledger, KeyringTypes.lattice, KeyringTypes.qr, - 'Custody - JSON - RPC', ])('should add accounts for %s type', async (keyringType) => { mockUUIDWithNormalAccounts([mockAccount]); @@ -2067,6 +2114,12 @@ describe('AccountsController', () => { 'KeyringController:getState', mockGetState.mockReturnValue({ keyrings: [{ type: keyringType, accounts: [mockAddress1] }], + keyringsMetadata: [ + { + id: 'mock-keyring-id-0', + name: 'mock-keyring-id-name', + }, + ], }), ); @@ -2096,6 +2149,7 @@ describe('AccountsController', () => { id: 'mock-id', address: mockAddress1, keyringType: keyringType as KeyringTypes, + options: createMockInternalAccountOptions(0, keyringType, 0), }), ]; @@ -2206,6 +2260,16 @@ describe('AccountsController', () => { { type: KeyringTypes.snap, accounts: ['0x1234'] }, { type: KeyringTypes.hd, accounts: [mockAddress1] }, ], + keyringsMetadata: [ + { + id: 'mock-keyring-id-1', + name: 'mock-keyring-id-name', + }, + { + id: 'mock-keyring-id-2', + name: 'mock-keyring-id-name2', + }, + ], }), ); diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index 98b9fb23af5..fce1e51c0c0 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -542,6 +542,8 @@ export class AccountsController extends BaseController< ); this.#update((state) => { + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-expect-error state.internalAccounts.accounts = accounts; }); } @@ -637,6 +639,7 @@ export class AccountsController extends BaseController< const { keyrings, keyringsMetadata } = await this.messagingSystem.call( 'KeyringController:getState', ); + for (let keyringIndex = 0; keyringIndex < keyrings.length; keyringIndex++) { const keyring = keyrings[keyringIndex]; const keyringType = keyring.type; diff --git a/packages/accounts-controller/src/tests/mocks.ts b/packages/accounts-controller/src/tests/mocks.ts index 18cc151224e..29be1c7eab2 100644 --- a/packages/accounts-controller/src/tests/mocks.ts +++ b/packages/accounts-controller/src/tests/mocks.ts @@ -37,6 +37,7 @@ export const createMockInternalAccount = ({ scopes, importTime = Date.now(), lastSelected = Date.now(), + options, }: { id?: string; address?: string; @@ -52,6 +53,7 @@ export const createMockInternalAccount = ({ }; importTime?: number; lastSelected?: number; + options?: Record; } = {}): InternalAccount => { const getInternalAccountDefaults = () => { switch (type) { @@ -80,7 +82,7 @@ export const createMockInternalAccount = ({ return { id, address, - options: {}, + options: options ?? {}, methods: methods ?? defaults.methods, scopes: scopes ?? defaults.scopes, type, @@ -104,3 +106,18 @@ export const createExpectedInternalAccount = ( lastSelected: expect.any(Number), }); }; + +export const createMockInternalAccountOptions = ( + keyringIndex: number, + keyringType: KeyringTypes, + groupIndex: number, +): Record => { + if (keyringType === KeyringTypes.hd) { + return { + entropySource: `mock-keyring-id-${keyringIndex}`, + derivationPath: `m/44'/60'/0'/0/${groupIndex}`, + }; + } + + return {}; +}; From dff973bb965e29bdf1e4f78641b84b59c5638572 Mon Sep 17 00:00:00 2001 From: Shane Terence Odlum Date: Thu, 17 Apr 2025 10:22:05 +0100 Subject: [PATCH 03/11] chore: update changelog --- packages/accounts-controller/CHANGELOG.md | 1 + packages/accounts-controller/src/AccountsController.ts | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/accounts-controller/CHANGELOG.md b/packages/accounts-controller/CHANGELOG.md index 1eb5d3f8cd0..8a99e005b47 100644 --- a/packages/accounts-controller/CHANGELOG.md +++ b/packages/accounts-controller/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Add `entropySource` and `derivationPath` to EVM HD account options via `listNormalAccounts` ([#5618](https://github.com/MetaMask/core/pull/5618)) - **BREAKING:** Bump peer dependency `@metamask/network-controller` to `^23.0.0` ([#5507](https://github.com/MetaMask/core/pull/5507)) ### Fixed diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index fce1e51c0c0..963234ab435 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -542,8 +542,6 @@ export class AccountsController extends BaseController< ); this.#update((state) => { - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-expect-error state.internalAccounts.accounts = accounts; }); } From f167c55a10f7f74c544d8f4e641ca89daf024eba Mon Sep 17 00:00:00 2001 From: Shane Terence Odlum Date: Thu, 17 Apr 2025 10:25:26 +0100 Subject: [PATCH 04/11] chore: update changelog --- packages/accounts-controller/CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/accounts-controller/CHANGELOG.md b/packages/accounts-controller/CHANGELOG.md index 8a99e005b47..0721904d59f 100644 --- a/packages/accounts-controller/CHANGELOG.md +++ b/packages/accounts-controller/CHANGELOG.md @@ -7,11 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +- Add `entropySource` and `derivationPath` to EVM HD account options via `listNormalAccounts` ([#5618](https://github.com/MetaMask/core/pull/5618)) + ## [27.0.0] ### Changed -- Add `entropySource` and `derivationPath` to EVM HD account options via `listNormalAccounts` ([#5618](https://github.com/MetaMask/core/pull/5618)) - **BREAKING:** Bump peer dependency `@metamask/network-controller` to `^23.0.0` ([#5507](https://github.com/MetaMask/core/pull/5507)) ### Fixed From 21d724151e85a835db83991f89c6a546bb25d14c Mon Sep 17 00:00:00 2001 From: Shane Terence Odlum Date: Thu, 17 Apr 2025 10:55:55 +0100 Subject: [PATCH 05/11] chore: remove custodykeyring --- packages/accounts-controller/src/utils.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/packages/accounts-controller/src/utils.ts b/packages/accounts-controller/src/utils.ts index 48f7da6be9d..6042e36f50c 100644 --- a/packages/accounts-controller/src/utils.ts +++ b/packages/accounts-controller/src/utils.ts @@ -11,12 +11,6 @@ import { v4 as uuid } from 'uuid'; * @returns The name of the keyring type. */ export function keyringTypeToName(keyringType: string): string { - // Custody keyrings are a special case, as they are not a single type - // they just start with the prefix `Custody` - if (isCustodyKeyring(keyringType)) { - return 'Custody'; - } - switch (keyringType) { case KeyringTypes.simple: { return 'Account'; From 2bb70439ab1528f0131aabf238647e0e8f96d9c4 Mon Sep 17 00:00:00 2001 From: Shane Terence Odlum Date: Thu, 17 Apr 2025 10:57:17 +0100 Subject: [PATCH 06/11] chore: remove custodykeyring --- packages/accounts-controller/src/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/accounts-controller/src/utils.ts b/packages/accounts-controller/src/utils.ts index 6042e36f50c..3bc80288919 100644 --- a/packages/accounts-controller/src/utils.ts +++ b/packages/accounts-controller/src/utils.ts @@ -1,4 +1,4 @@ -import { isCustodyKeyring, KeyringTypes } from '@metamask/keyring-controller'; +import { KeyringTypes } from '@metamask/keyring-controller'; import { hexToBytes } from '@metamask/utils'; import { sha256 } from 'ethereum-cryptography/sha256'; import type { V4Options } from 'uuid'; From 97163807cd828af1a3fc688817fd3c65a61fb200 Mon Sep 17 00:00:00 2001 From: Shane Terence Odlum Date: Thu, 17 Apr 2025 11:03:42 +0100 Subject: [PATCH 07/11] chore: update changelog --- packages/accounts-controller/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/accounts-controller/CHANGELOG.md b/packages/accounts-controller/CHANGELOG.md index a52513d0953..c8033681b23 100644 --- a/packages/accounts-controller/CHANGELOG.md +++ b/packages/accounts-controller/CHANGELOG.md @@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + - Add `entropySource` and `derivationPath` to EVM HD account options via `listNormalAccounts` ([#5618](https://github.com/MetaMask/core/pull/5618)) ### Changed From 4ec88eeb93351053881c9e8f61fa19db73f1fe04 Mon Sep 17 00:00:00 2001 From: Mircea Nistor Date: Wed, 30 Apr 2025 12:04:05 +0200 Subject: [PATCH 08/11] docs(accounts-controller): simplify changelog entry --- packages/accounts-controller/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/accounts-controller/CHANGELOG.md b/packages/accounts-controller/CHANGELOG.md index 4698cd75d42..7628cddad50 100644 --- a/packages/accounts-controller/CHANGELOG.md +++ b/packages/accounts-controller/CHANGELOG.md @@ -10,7 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Add new `setAccountNameAndSelectAccount` action ([#5714](https://github.com/MetaMask/core/pull/5714)) -- Add `entropySource` and `derivationPath` to EVM HD account options via `listNormalAccounts` ([#5618](https://github.com/MetaMask/core/pull/5618)) +- Add `entropySource` and `derivationPath` to EVM HD account options ([#5618](https://github.com/MetaMask/core/pull/5618)) ### Changed From 265987298ff8cc0627dcde5c41b80930a83b9d7a Mon Sep 17 00:00:00 2001 From: Mircea Nistor Date: Wed, 30 Apr 2025 12:15:38 +0200 Subject: [PATCH 09/11] style(accounts-controller): Apply style suggestions from code review Co-authored-by: Michele Esposito <34438276+mikesposito@users.noreply.github.com> --- .../accounts-controller/src/AccountsController.ts | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index 8f556acb824..e182e2ef975 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -684,24 +684,18 @@ export class AccountsController extends BaseController< */ async #listNormalAccounts(): Promise { const internalAccounts: InternalAccount[] = []; - const { keyrings, keyringsMetadata } = await this.messagingSystem.call( + const { keyrings, keyringsMetadata } = this.messagingSystem.call( 'KeyringController:getState', ); - for (let keyringIndex = 0; keyringIndex < keyrings.length; keyringIndex++) { - const keyring = keyrings[keyringIndex]; + for (const [keyringIndex, keyring] of keyrings.entries()) { const keyringType = keyring.type; if (!isNormalKeyringType(keyringType as KeyringTypes)) { // We only consider "normal accounts" here, so keep looping continue; } - for ( - let accountIndex = 0; - accountIndex < keyring.accounts.length; - accountIndex++ - ) { - const address = keyring.accounts[accountIndex]; + for (const [accountIndex, address] of keyring.accounts.entries()) { const id = getUUIDFromAddressOfNormalAccount(address); let options = {}; From 5b2c88bcb16d432514a57588a29c461cd38fa4de Mon Sep 17 00:00:00 2001 From: Mircea Nistor Date: Wed, 30 Apr 2025 12:19:19 +0200 Subject: [PATCH 10/11] docs(accounts-controller): Add reason for not reusing keyring instance properties Co-authored-by: Charly Chevalier --- packages/accounts-controller/src/AccountsController.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index e182e2ef975..56c344e8849 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -703,6 +703,9 @@ export class AccountsController extends BaseController< if (isHdKeyringType(keyring.type as KeyringTypes)) { options = { entropySource: keyringsMetadata[keyringIndex].id, + // NOTE: We are not using the `hdPath` from the associated keyring here and + // getting the keyring instance here feels a bit overkill. + // This will be naturally fixed once every keyring start using `KeyringAccount` and implement the keyring API. derivationPath: getDerivationPathForIndex(accountIndex), }; } From 5e14b0cfd121a3cee6597fb94c6554049eb9c790 Mon Sep 17 00:00:00 2001 From: Mircea Nistor Date: Wed, 30 Apr 2025 14:23:04 +0200 Subject: [PATCH 11/11] chore: empty commit to trigger GH workflows again :shrug: rerunning jobs did not work and merging the PR is blocked