Skip to content

feat: add KeyringController:accountAdded event + use keyring events in AccountsController #7328

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ccharly wants to merge 16 commits into
base: main
Choose a base branch
from
+552 −735
Open

feat: add KeyringController:accountAdded event + use keyring events in AccountsController #7328

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
3fe4986
feat: add KeyringController:accountAdded event + use keyring events i…
ccharly Dec 5, 2025
b4700b2
fix: add missing return types
ccharly Dec 8, 2025
537e318
refactor: more DRY
ccharly Dec 8, 2025
e137a0b
fix: revert change to :accountRemoved
ccharly Dec 8, 2025
1a2a27c
refactor: keyringEventInfo -> keyringObject
ccharly Dec 8, 2025
e6eaaa1
test: add new tests
ccharly Dec 8, 2025
98c92c6
Merge branch 'main' into feat/keyring-controller-account-added
ccharly Dec 12, 2025
fe85881
test: add more test for persistAllKeyrings + lint
ccharly Dec 12, 2025
cab3f12
refactor: re-adapt accounts-controller tests
ccharly Dec 12, 2025
fda91cd
fix: fix accounts-controller new event handling
ccharly Dec 12, 2025
f7f52ce
chore: lint
ccharly Dec 12, 2025
5fee36e
chore: changelogs
ccharly Dec 12, 2025
54d6e16
refactor: override update to cover all possible use-cases
ccharly Dec 12, 2025
a5c5f5d
Merge branch 'main' into feat/keyring-controller-account-added
ccharly Dec 12, 2025
81198db
chore: add missing jsdocs
ccharly Dec 12, 2025
5009b0a
fix: remove extra :accountRemoved
ccharly Dec 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
feat: add KeyringController:accountAdded event + use keyring events i… 
…n AccountsController
  • Loading branch information
@ccharly
ccharly committed Dec 8, 2025
commit 3fe49867b7bc08de64841b6b37f8531f4f38e355
210 changes: 56 additions & 154 deletions packages/accounts-controller/src/AccountsController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,10 @@ import {
} from '@metamask/keyring-api';
import { KeyringTypes } from '@metamask/keyring-controller';
import type {
KeyringControllerState,
KeyringControllerGetKeyringsByTypeAction,
KeyringControllerStateChangeEvent,
KeyringControllerAccountAddedEvent,
KeyringControllerAccountRemovedEvent,
KeyringControllerGetStateAction,
KeyringObject,
} from '@metamask/keyring-controller';
Expand Down Expand Up @@ -202,6 +203,8 @@ export type AccountsControllerAccountAssetListUpdatedEvent = {
export type AllowedEvents =
| SnapStateChange
| KeyringControllerStateChangeEvent
| KeyringControllerAccountRemovedEvent
| KeyringControllerAccountAddedEvent
| SnapKeyringAccountAssetListUpdatedEvent
| SnapKeyringAccountBalancesUpdatedEvent
| SnapKeyringAccountTransactionsUpdatedEvent
Expand Down Expand Up @@ -506,7 +509,10 @@ export class AccountsController extends BaseController<
);
}

#assertAccountCanBeRenamed(account: InternalAccount, accountName: string) {
#assertAccountCanBeRenamed(
account: InternalAccount,
accountName: string,
): void {
if (
this.listMultichainAccounts().find(
(internalAccount) =>
Expand Down Expand Up @@ -748,168 +754,59 @@ export class AccountsController extends BaseController<
this.messenger.publish(event, ...payload);
}

/**
* Handles changes in the keyring state, specifically when new accounts are added or removed.
*
* @param keyringState - The new state of the keyring controller.
* @param keyringState.isUnlocked - True if the keyrings are unlocked, false otherwise.
* @param keyringState.keyrings - List of all keyrings.
*/
#handleOnKeyringStateChange({
isUnlocked,
keyrings,
}: KeyringControllerState): void {
// TODO: Change when accountAdded event is added to the keyring controller.

// We check for keyrings length to be greater than 0 because the extension client may try execute
// submit password twice and clear the keyring state.
// https://github.com/MetaMask/KeyringController/blob/2d73a4deed8d013913f6ef0c9f5c0bb7c614f7d3/src/KeyringController.ts#L910
if (!isUnlocked || keyrings.length === 0) {
return;
}

// State patches.
const generatePatch = () => {
return {
previous: {} as Record<string, InternalAccount>,
added: [] as {
address: string;
keyring: KeyringObject;
}[],
updated: [] as InternalAccount[],
removed: [] as InternalAccount[],
};
};
const patches = {
snap: generatePatch(),
normal: generatePatch(),
};

// Gets the patch object based on the keyring type (since Snap accounts and other accounts
// are handled differently).
const patchOf = (type: string) => {
if (isSnapKeyringType(type)) {
return patches.snap;
}
return patches.normal;
};

// Create a map (with lower-cased addresses) of all existing accounts.
for (const account of this.listMultichainAccounts()) {
const address = account.address.toLowerCase();
const patch = patchOf(account.metadata.keyring.type);
#handleOnKeyringAccountAdded(address: string, keyring: KeyringObject) {
let account: InternalAccount | undefined;

patch.previous[address] = account;
}
this.#update((state) => {
const { internalAccounts } = state;

// Go over all keyring changes and create patches out of it.
const addresses = new Set<string>();
for (const keyring of keyrings) {
const patch = patchOf(keyring.type);
account = this.#getInternalAccountFromAddressAndType(address, keyring);
if (account) {
// Re-compute the list of accounts everytime, so we can make sure new names
// are also considered.
const accounts = Object.values(
internalAccounts.accounts,
) as InternalAccount[];

for (const accountAddress of keyring.accounts) {
// Lower-case address to use it in the `previous` map.
const address = accountAddress.toLowerCase();
const account = patch.previous[address];
// Get next account name available for this given keyring.
const name = this.getNextAvailableAccountName(
account.metadata.keyring.type,
accounts,
);

if (account) {
// If the account exists before, this might be an update.
patch.updated.push(account);
} else {
// Otherwise, that's a new account.
patch.added.push({
address,
keyring,
});
}
// If it's the first account, we need to select it.
const lastSelected =
accounts.length === 0 ? this.#getLastSelectedIndex() : 0;

// Keep track of those address to check for removed accounts later.
addresses.add(address);
internalAccounts.accounts[account.id] = {
...account,
metadata: {
...account.metadata,
name,
importTime: Date.now(),
lastSelected,
},
};
}
}
});

// We might have accounts associated with removed keyrings, so we iterate
// over all previous known accounts and check against the keyring addresses.
for (const patch of [patches.snap, patches.normal]) {
for (const [address, account] of Object.entries(patch.previous)) {
// If a previous address is not part of the new addesses, then it got removed.
if (!addresses.has(address)) {
patch.removed.push(account);
}
}
if (account) {
this.messenger.publish('AccountsController:accountAdded', account);
}
}

// Diff that we will use to publish events afterward.
const diff = {
removed: [] as string[],
added: [] as InternalAccount[],
};
#handleOnKeyringAccountRemoved(address: string) {
const account = this.listMultichainAccounts().find(
({ address: accountAddress }) => accountAddress === address,
);
Copy link

@cursor cursor bot Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Case-sensitive address comparison may fail to find accounts

The #handleOnKeyringAccountRemoved method uses strict equality (===) to compare addresses, while the existing getAccountByAddress method uses case-insensitive comparison with .toLowerCase() on both sides. Ethereum addresses can exist in different case formats (checksummed vs lowercase), so if there's any inconsistency in how addresses are stored versus how they're received from the KeyringController:accountRemoved event, the account lookup will fail silently and the account won't be removed from the state.

Fix in Cursor Fix in Web

Copy link

@cursor cursor bot Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Address comparison uses strict equality instead of case-insensitive

The #handleOnKeyringAccountRemoved method uses strict equality (===) to compare addresses, but the codebase convention (seen in getAccountByAddress and utils.ts) uses case-insensitive comparison with .toLowerCase(). Ethereum addresses can be checksummed (mixed case) or lowercase, so if the KeyringController emits an address with different casing than what's stored in AccountsController, the account won't be found and won't be removed, leaving orphaned accounts in state.

Fix in Cursor Fix in Web


this.#update(
(state) => {
if (account) {
this.#update((state) => {
const { internalAccounts } = state;

for (const patch of [patches.snap, patches.normal]) {
for (const account of patch.removed) {
delete internalAccounts.accounts[account.id];

diff.removed.push(account.id);
}

for (const added of patch.added) {
const account = this.#getInternalAccountFromAddressAndType(
added.address,
added.keyring,
);

if (account) {
// Re-compute the list of accounts everytime, so we can make sure new names
// are also considered.
const accounts = Object.values(
internalAccounts.accounts,
) as InternalAccount[];

// Get next account name available for this given keyring.
const name = this.getNextAvailableAccountName(
account.metadata.keyring.type,
accounts,
);

// If it's the first account, we need to select it.
const lastSelected =
accounts.length === 0 ? this.#getLastSelectedIndex() : 0;

internalAccounts.accounts[account.id] = {
...account,
metadata: {
...account.metadata,
name,
importTime: Date.now(),
lastSelected,
},
};

diff.added.push(internalAccounts.accounts[account.id]);
}
}
}
},
// Will get executed after the update, but before re-selecting an account in case
// the current one is not valid anymore.
() => {
// Now publish events
for (const id of diff.removed) {
this.messenger.publish('AccountsController:accountRemoved', id);
}

for (const account of diff.added) {
this.messenger.publish('AccountsController:accountAdded', account);
}
},
);

// NOTE: Since we also track "updated" accounts with our patches, we could fire a new event
// like `accountUpdated` (we would still need to check if anything really changed on the account).
delete internalAccounts.accounts[account.id];
});
}
}
Copy link

@cursor cursor bot Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Event ordering inconsistency when removing selected account

The #handleOnKeyringAccountRemoved method publishes accountRemoved event after calling #update, while the analogous #handleOnKeyringAccountAdded method publishes accountAdded inside the beforeAutoSelectAccount callback of #update. This asymmetry causes an event ordering bug: when the removed account is the currently selected one, #update triggers auto-selection and publishes selectedAccountChange before accountRemoved gets published. This reverses the expected event order. The PR includes a test ("fires :accountAdded before :selectedAccountChange") that validates the correct ordering for added accounts, but the same ordering guarantee isn't maintained for removed accounts. Moving messenger.publish('AccountsController:accountRemoved', ...) into a beforeAutoSelectAccount callback would fix this inconsistency.

Fix in Cursor Fix in Web

Copy link
Contributor Author

@ccharly ccharly Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true for the asymmetry, but IMO this makes more sense to call :selectedAccountChange before :accountRemoved:

  • We change the account cause it will get removed
  • We remove the account

This way, we don't have any "dangling" account being wrongly selected (from a consumer point of view)


/**
Expand Down Expand Up @@ -1215,8 +1112,13 @@ export class AccountsController extends BaseController<
this.#handleOnSnapStateChange(snapStateState),
);

this.messenger.subscribe('KeyringController:stateChange', (keyringState) =>
this.#handleOnKeyringStateChange(keyringState),
this.messenger.subscribe('KeyringController:accountRemoved', (address) =>
this.#handleOnKeyringAccountRemoved(address),
);

this.messenger.subscribe(
'KeyringController:accountAdded',
(address, keyring) => this.#handleOnKeyringAccountAdded(address, keyring),
);
Copy link

@cursor cursor bot Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Wallet lock removes accounts and loses custom names

The old #handleOnKeyringStateChange method had a guard if (!isUnlocked || keyrings.length === 0) { return; } that prevented account state changes when the wallet was locked. The new event-based handlers lack this guard. When setLocked() is called, it sets state.keyrings = [], which triggers accountRemoved events for all accounts via the update override. The AccountsController processes these events and removes all accounts. On unlock, accounts are re-added with auto-generated names via getNextAvailableAccountName, causing loss of any custom account names the user had set.

Additional Locations (1)

Fix in Cursor Fix in Web


this.messenger.subscribe(
Expand Down
Loading
Loading