Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
fix: fix accounts-controller new event handling
  • Loading branch information
ccharly committed Dec 12, 2025
commit fda91cd6b797bbe0b9b1380ee86abcf8fe23e665
4 changes: 2 additions & 2 deletions eslint-suppressions.json
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@
},
"packages/accounts-controller/src/AccountsController.ts": {
"@typescript-eslint/explicit-function-return-type": {
"count": 10
"count": 9
}
},
"packages/accounts-controller/src/tests/mocks.ts": {
Expand Down Expand Up @@ -3305,4 +3305,4 @@
"count": 2
}
}
}
}
80 changes: 42 additions & 38 deletions packages/accounts-controller/src/AccountsController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -755,43 +755,47 @@ export class AccountsController extends BaseController<
}

#handleOnKeyringAccountAdded(address: string, keyring: KeyringObject) {
let account: InternalAccount | undefined;

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

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[];

// 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;
let 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(this.state.internalAccounts.accounts);

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

internalAccounts.accounts[account.id] = {
...account,
metadata: {
...account.metadata,
name,
importTime: Date.now(),
lastSelected,
},
};
}
});
// If it's the first account, we need to select it.
const lastSelected =
accounts.length === 0 ? this.#getLastSelectedIndex() : 0;

// Update account metadata.
account = {
...account,
metadata: {
...account.metadata,
name,
importTime: Date.now(),
lastSelected,
},
};

if (account) {
this.messenger.publish('AccountsController:accountAdded', account);
// Not sure why, but the compiler still infers `account` as possibly undefined.
const internalAccount: InternalAccount = account;
this.#update(
(state) => {
state.internalAccounts.accounts[internalAccount.id] = internalAccount;
},
() => {
// Will be published before `:selectedAccountChange` event is published.
this.messenger.publish(
'AccountsController:accountAdded',
internalAccount,
);
},
);
}
}

Expand All @@ -802,10 +806,10 @@ export class AccountsController extends BaseController<

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

delete internalAccounts.accounts[account.id];
delete state.internalAccounts.accounts[account.id];
});

this.messenger.publish('AccountsController:accountRemoved', account.id);
}
}
Copy link

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

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
Loading