Skip to content

perf: remove call to getNextAvailableAccountName #7137

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
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

perf: remove call to getNextAvailableAccountName #7137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bc77e27
feat: remove call to getNextAvailableAccountName
hmalik88 Nov 12, 2025
8c4d490
Merge remote-tracking branch 'origin/main' into hm/mul-1242
hmalik88 Nov 12, 2025
a679ac7
chore: update changelog
hmalik88 Nov 12, 2025
ec0e904
fix: update tests
hmalik88 Nov 12, 2025
d27ff28
test: add tests to cover uncovered lines
hmalik88 Nov 12, 2025
ec0f132
Merge remote-tracking branch 'origin/main' into hm/mul-1242
hmalik88 Nov 12, 2025
62605e1
Merge remote-tracking branch 'origin/main' into hm/mul-1242
hmalik88 Nov 14, 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
5 changes: 5 additions & 0 deletions packages/accounts-controller/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Changed

- Remove usage of `getNextAvailableAccountName` in `handleOnKeyringStateChange` ([#7137](https://github.com/MetaMask/core/pull/7137))
- The function call was decided to be removed since it was taking a lot of time, we compute account names in the `AccountTreeController` regardless.

## [34.0.0]

### Changed
Expand Down
91 changes: 57 additions & 34 deletions packages/accounts-controller/src/AccountsController.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
import { SnapStatus } from '@metamask/snaps-utils';
import type { CaipChainId } from '@metamask/utils';
import type { V4Options } from 'uuid';
import * as uuid from 'uuid';

Check warning on line 30 in packages/accounts-controller/src/AccountsController.test.ts

View workflow job for this annotation

GitHub Actions / Lint, build, and test / Lint (22.x) 

No exported names found in module 'uuid'

import type {
AccountsControllerMessenger,
Expand Down Expand Up @@ -78,7 +78,7 @@
type: EthAccountType.Eoa,
scopes: [EthScope.Eoa],
metadata: {
name: 'Account 1',
name: '',
keyring: { type: KeyringTypes.hd },
importTime: 1691565967600,
lastSelected: 1691565967656,
Expand All @@ -94,7 +94,7 @@
type: EthAccountType.Eoa,
scopes: [EthScope.Eoa],
metadata: {
name: 'Account 2',
name: '',
keyring: { type: KeyringTypes.hd },
importTime: 1691565967600,
lastSelected: 1955565967656,
Expand Down Expand Up @@ -786,7 +786,7 @@
setExpectedLastSelectedAsAny(
createExpectedInternalAccount({
id: 'mock-id3',
name: 'Snap Account 2',
name: '',
address: mockAccount3.address,
keyringType: mockAccount3.metadata.keyring.type as KeyringTypes,
snap: mockAccount3.metadata.snap,
Expand Down Expand Up @@ -970,7 +970,7 @@
MockExpectedInternalAccountBuilder.from(
createExpectedInternalAccount({
id: 'mock-id3',
name: 'Account 3',
name: '',
address: mockAccount3.address,
keyringType: KeyringTypes.hd,
}),
Expand Down Expand Up @@ -1039,7 +1039,7 @@
MockExpectedInternalAccountBuilder.from(
createExpectedInternalAccount({
id: 'mock-id3',
name: 'Account 3',
name: '',
address: mockAccount3.address,
keyringType: KeyringTypes.hd,
}),
Expand Down Expand Up @@ -1510,13 +1510,13 @@
const messenger = buildMessenger();
const mockInitialAccount = createMockInternalAccount({
id: 'mock-id',
name: 'Account 1',
name: '',
address: '0x123',
keyringType: KeyringTypes.hd,
});
const mockReinitialisedAccount = createMockInternalAccount({
id: 'mock-id2',
name: 'Account 1',
name: '',
address: '0x456',
keyringType: KeyringTypes.hd,
// Entropy options are added automatically by the controller.
Expand Down Expand Up @@ -3309,27 +3309,6 @@
accountsController.getAccountExpect(mockAccount.id),
);
});

it('throw an error if the account name already exists', () => {
const { accountsController } = setupAccountsController({
initialState: {
internalAccounts: {
accounts: {
[mockAccount.id]: mockAccount,
[mockAccount2.id]: mockAccount2,
},
selectedAccount: mockAccount.id,
},
},
});

expect(() =>
accountsController.setAccountNameAndSelectAccount(
mockAccount.id,
mockAccount2.metadata.name,
),
).toThrow('Account name already exists');
});
});

describe('setAccountName', () => {
Expand Down Expand Up @@ -3392,21 +3371,29 @@
);
});

it('throw an error if the account name already exists', () => {
it('throws when renaming to an existing account name', () => {
const existingName = 'Existing Name';
const mockAccountWithName = createMockInternalAccount({
id: 'mock-id-2',
name: existingName,
address: '0xabc',
keyringType: KeyringTypes.hd,
});

const { accountsController } = setupAccountsController({
initialState: {
internalAccounts: {
accounts: {
[mockAccount.id]: mockAccount,
[mockAccount2.id]: mockAccount2,
[mockAccountWithName.id]: mockAccountWithName,
},
selectedAccount: mockAccount.id,
},
},
});

expect(() =>
accountsController.setAccountName(mockAccount.id, 'Account 2'),
accountsController.setAccountName(mockAccount.id, existingName),
).toThrow('Account name already exists');
});
});
Expand Down Expand Up @@ -3437,19 +3424,19 @@
// those keyring types are "grouped" together)
const mockSimpleKeyring1 = createMockInternalAccount({
id: 'mock-id2',
name: 'Account 2',
name: '',
address: '0x555',
keyringType: KeyringTypes.simple,
});
const mockSimpleKeyring2 = createMockInternalAccount({
id: 'mock-id3',
name: 'Account 3',
name: '',
address: '0x666',
keyringType: KeyringTypes.simple,
});
const mockSimpleKeyring3 = createMockInternalAccount({
id: 'mock-id4',
name: 'Account 4',
name: '',
address: '0x777',
keyringType: KeyringTypes.simple,
});
Expand Down Expand Up @@ -3940,6 +3927,42 @@
);
expect(accountName).toBe('Account 4');
});

it('computes next name for non-HD keyring types (Ledger branch)', () => {
const { accountsController } = setupAccountsController({});

const ledger1 = createMockInternalAccount({
id: 'ledger-1',
name: 'Ledger 1',
address: '0xdeadbeef1',
keyringType: KeyringTypes.ledger,
});

const next = accountsController.getNextAvailableAccountName(
KeyringTypes.ledger,
[ledger1],
);
expect(next).toBe('Ledger 2');
});

it('treats Simple and HD as one group (invokes simple-type check)', () => {
const { accountsController } = setupAccountsController({});

const simple1 = createMockInternalAccount({
id: 'simple-1',
name: 'Account 1',
address: '0xfacefeed1',
keyringType: KeyringTypes.simple,
});

// Asking for next HD name with only a Simple account forces the
// filter to evaluate the Simple branch, covering isSimpleKeyringType.
const next = accountsController.getNextAvailableAccountName(
KeyringTypes.hd,
[simple1],
);
expect(next).toBe('Account 2');
});
});
});
});
Expand Down
7 changes: 0 additions & 7 deletions packages/accounts-controller/src/AccountsController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -868,12 +868,6 @@ export class AccountsController extends BaseController<
internalAccounts.accounts,
) as InternalAccount[];

// Get next account name available for this given keyring.
const name = this.getNextAvailableAccountName(
Copy link
Member

@gantunesr gantunesr Nov 12, 2025

Choose a reason for hiding this comment

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

should we also remove the action AccountsControllerGetNextAvailableAccountNameAction? And I see the method also being bind

Copy link
Contributor Author

@hmalik88 hmalik88 Nov 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I wasn't sure if I should remove that and if it was being used in the clients. Since @danroc mentioned that we should just remove the calls.

Copy link
Member

@gantunesr gantunesr Nov 13, 2025

Choose a reason for hiding this comment

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

Thats a good point, but the call would fail making this a breaking change, right?

Copy link
Contributor

@ccharly ccharly Nov 13, 2025

Choose a reason for hiding this comment

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

So, I think we can categorize this PR as perf: improvement for now.

I think we can keep the action and make this just a minor/patch change (not sure which yet, but we can decide later 😄)

However, I'm not 100% sure of the side-effects of this change @hmalik88, so could you make a preview build + make test/integration PRs on both clients and see how that goes?

We just got some errors around naming with another fix today, and some e2e were failing because of that (because we did not migrate everything yet to the full BIP-44 integration unfortunately!)

Copy link
Contributor Author

@hmalik88 hmalik88 Nov 14, 2025

Choose a reason for hiding this comment

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

@gantunesr I don't think the call would fail since we didn't remove the function itself, just the call to it in the state change handler.

@ccharly yup, I can make a preview build and test.

account.metadata.keyring.type,
accounts,
);

// If it's the first account, we need to select it.
const lastSelected =
accounts.length === 0 ? this.#getLastSelectedIndex() : 0;
Expand All @@ -882,7 +876,6 @@ export class AccountsController extends BaseController<
...account,
metadata: {
...account.metadata,
name,
importTime: Date.now(),
lastSelected,
},
Expand Down
Loading