diff --git a/packages/multichain-account-service/CHANGELOG.md b/packages/multichain-account-service/CHANGELOG.md index fe40d5794da..7451f25bb8f 100644 --- a/packages/multichain-account-service/CHANGELOG.md +++ b/packages/multichain-account-service/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Fix `BaseBip44AccountProvider` leaking removed accounts and stale account IDs ([#10069](https://github.com/MetaMask/core/pull/10069)) + - `getAccounts()` now filters out entries the `AccountsController` no longer knows about instead of returning them as if they were real accounts. + - `init()` now replaces the tracked account set instead of adding to it, so a re-init after an account removal (e.g. on unlock) no longer leaves the removed account's ID tracked forever. + ### Changed - Bump `@metamask/accounts-controller` from `^39.1.0` to `^39.1.1` ([#9969](https://github.com/MetaMask/core/pull/9969)) diff --git a/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.test.ts b/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.test.ts new file mode 100644 index 00000000000..f478d730225 --- /dev/null +++ b/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.test.ts @@ -0,0 +1,168 @@ +import type { + CreateAccountOptions, + KeyringAccount, +} from '@metamask/keyring-api'; +import type { KeyringCapabilities } from '@metamask/keyring-api/v2'; +import type { Bip44Account } from '@metamask/account-api'; +import type { InternalAccount } from '@metamask/keyring-internal-api'; + +import { + getMultichainAccountServiceMessenger, + getRootMessenger, + MOCK_HD_ACCOUNT_1, + MOCK_HD_ACCOUNT_2, +} from '../tests/index.js'; +import type { RootMessenger } from '../tests/index.js'; +import { BaseBip44AccountProvider } from './BaseBip44AccountProvider.js'; + +/** + * Minimal concrete subclass so we can exercise the abstract base class + * directly, matching the pattern used to test other abstract collaborators + * in this package. + */ +class TestAccountProvider extends BaseBip44AccountProvider< + Bip44Account +> { + get capabilities(): KeyringCapabilities { + return { supportsEnabling: true } as unknown as KeyringCapabilities; + } + + getName(): string { + return 'TestAccountProvider'; + } + + async resyncAccounts(): Promise { + // No-op for this test double. + } + + isAccountCompatible(): boolean { + return true; + } + + async createAccounts( + _options: CreateAccountOptions, + ): Promise[]> { + return []; + } + + async deleteAccount(): Promise { + // No-op for this test double. + } + + async discoverAccounts(): Promise[]> { + return []; + } +} + +/** + * Sets up a provider wired to a messenger whose + * `AccountsController:getAccounts` handler mirrors the real + * `AccountsController.getAccounts` contract: one slot per requested ID, + * `undefined` in place for any ID the controller no longer knows about + * (rather than omitting it, which would silently shrink the array). + * + * @param knownAccounts - The accounts the mocked AccountsController knows + * about. + * @returns The provider under test and its messenger. + */ +function setup(knownAccounts: InternalAccount[] = []): { + provider: TestAccountProvider; + messenger: RootMessenger; +} { + const rootMessenger = getRootMessenger(); + const messenger = getMultichainAccountServiceMessenger(rootMessenger); + + rootMessenger.registerActionHandler( + 'AccountsController:getAccounts', + (accountIds: string[]) => + accountIds.map((id) => + knownAccounts.find((account) => account.id === id), + ), + ); + + return { + provider: new TestAccountProvider(messenger), + messenger: rootMessenger, + }; +} + +describe('BaseBip44AccountProvider', () => { + describe('getAccounts', () => { + it('filters out accounts the AccountsController no longer knows about', () => { + // Only account 1 is "known" -- account 2 has been removed from the + // AccountsController's perspective, but is still tracked by this + // provider (e.g. before a reconciling init() call happens). + const { provider } = setup([MOCK_HD_ACCOUNT_1]); + provider.init([MOCK_HD_ACCOUNT_1.id, MOCK_HD_ACCOUNT_2.id]); + + const accounts = provider.getAccounts(); + + expect(accounts).toStrictEqual([MOCK_HD_ACCOUNT_1]); + expect(accounts).not.toContain(undefined); + }); + + it('returns an empty array when none of the tracked accounts exist anymore', () => { + const { provider } = setup([]); + provider.init([MOCK_HD_ACCOUNT_1.id, MOCK_HD_ACCOUNT_2.id]); + + expect(provider.getAccounts()).toStrictEqual([]); + }); + + it('returns all accounts unchanged when every tracked account still exists', () => { + const { provider } = setup([MOCK_HD_ACCOUNT_1, MOCK_HD_ACCOUNT_2]); + provider.init([MOCK_HD_ACCOUNT_1.id, MOCK_HD_ACCOUNT_2.id]); + + expect(provider.getAccounts()).toStrictEqual([ + MOCK_HD_ACCOUNT_1, + MOCK_HD_ACCOUNT_2, + ]); + }); + }); + + describe('init', () => { + it('does not leave stale account IDs behind after a re-init with a smaller set', () => { + const { provider } = setup([MOCK_HD_ACCOUNT_1, MOCK_HD_ACCOUNT_2]); + + // First init: both accounts are tracked. + provider.init([MOCK_HD_ACCOUNT_1.id, MOCK_HD_ACCOUNT_2.id]); + expect( + provider.isAligned({ entropySource: 'x', groupIndex: 0 }, [ + MOCK_HD_ACCOUNT_2.id, + ]), + ).toBe(true); + + // Account 2 gets removed upstream; a real caller (e.g. the mobile + // app's Authentication flow, which calls + // MultichainAccountService.init() -> provider.init() on every + // unlock) re-initializes the provider with only the surviving + // account. + provider.init([MOCK_HD_ACCOUNT_1.id]); + + // The stale ID for the removed account must not still be tracked. + expect( + provider.isAligned({ entropySource: 'x', groupIndex: 0 }, [ + MOCK_HD_ACCOUNT_2.id, + ]), + ).toBe(false); + expect( + provider.isAligned({ entropySource: 'x', groupIndex: 0 }, [ + MOCK_HD_ACCOUNT_1.id, + ]), + ).toBe(true); + }); + + it('is idempotent when called repeatedly with the same accounts', () => { + const { provider } = setup([MOCK_HD_ACCOUNT_1]); + + provider.init([MOCK_HD_ACCOUNT_1.id]); + provider.init([MOCK_HD_ACCOUNT_1.id]); + provider.init([MOCK_HD_ACCOUNT_1.id]); + + expect( + provider.isAligned({ entropySource: 'x', groupIndex: 0 }, [ + MOCK_HD_ACCOUNT_1.id, + ]), + ).toBe(true); + }); + }); +}); diff --git a/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts b/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts index 6554a1229fe..392ae3486ab 100644 --- a/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts +++ b/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts @@ -138,16 +138,18 @@ export abstract class BaseBip44AccountProvider< } /** - * Add accounts to the provider. + * Initialize the provider with the given accounts. * * Note: There's an implicit assumption that the accounts are BIP-44 compatible. * - * @param accounts - The accounts to add. + * This replaces the provider's internal account list rather than adding to + * it, so a re-init (e.g. after an account has been removed) does not leave + * stale IDs behind from a previous call. + * + * @param accounts - The accounts to initialize the provider with. */ init(accounts: Account['id'][]): void { - for (const account of accounts) { - this.accounts.add(account); - } + this.accounts = new Set(accounts); } /** @@ -170,8 +172,14 @@ export abstract class BaseBip44AccountProvider< 'AccountsController:getAccounts', accountsIds, ); - // we cast here because we know that the accounts are BIP-44 compatible - return internalAccounts as unknown as Account[]; + // `AccountsController:getAccounts` returns `undefined` for any ID it no + // longer knows about (e.g. the account was removed but this provider's + // internal ID list hasn't been reconciled yet). Filter those out rather + // than casting them through as if they were real accounts. + // we cast here because we know that the remaining accounts are BIP-44 compatible + return internalAccounts.filter( + (account): account is NonNullable => Boolean(account), + ) as unknown as Account[]; } /** diff --git a/packages/multichain-account-service/src/tests/providers.ts b/packages/multichain-account-service/src/tests/providers.ts index 4f09c453b29..cb91f84632b 100644 --- a/packages/multichain-account-service/src/tests/providers.ts +++ b/packages/multichain-account-service/src/tests/providers.ts @@ -111,7 +111,9 @@ export function setupBip44AccountProvider({ mocks.createAccounts.mockResolvedValue([]); mocks.init.mockImplementation( (accountIds: Bip44Account['id'][]) => { - accountIds.forEach((id) => mocks.accounts.add(id)); + // Mirrors the real BaseBip44AccountProvider#init(), which replaces + // (rather than adds to) the tracked account set on every call. + mocks.accounts = new Set(accountIds); }, );