perf: remove call to getNextAvailableAccountName
#7137
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Accounts missing auto-generated names.
Newly added accounts in #handleOnKeyringStateChange no longer receive auto-generated names like "Account 1" or "Account 2". For non-Snap accounts, #getInternalAccountForNonSnapAccount returns an empty string for metadata.name when the account doesn't exist yet. Without the removed getNextAvailableAccountName call, these accounts are stored with empty names, causing them to appear nameless in the UI.
packages/accounts-controller/src/AccountsController.ts#L874-L882
core/packages/accounts-controller/src/AccountsController.ts
Lines 874 to 882 in 8c4d490
| ) as InternalAccount[]; | ||
|
|
||
| // Get next account name available for this given keyring. | ||
| const name = this.getNextAvailableAccountName( |
There was a problem hiding this comment.
should we also remove the action AccountsControllerGetNextAvailableAccountNameAction? And I see the method also being bind
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thats a good point, but the call would fail making this a breaking change, right?
There was a problem hiding this comment.
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!)
There was a problem hiding this comment.
@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.
hmalik88
changed the title
getNextAvailableAccountNamegetNextAvailableAccountName
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
Explanation
handleOnKeyringStateChangeis calling this function on every account which is taking ~ 200ms. It's been determined that we can remove this call.References
N/A
Checklist
Note
Removes
getNextAvailableAccountNameusage fromhandleOnKeyringStateChange, updates tests accordingly, and documents the change.getNextAvailableAccountNamein#handleOnKeyringStateChange, no longer settingmetadata.nameon add; rely on existing metadata/defaults.metadata.namefor newly added accounts; adjust rename tests; add coverage forgetNextAvailableAccountName(Ledger branch and HD/Simple grouping).CHANGELOG.mdunder Unreleased to reflect the removal.Written by Cursor Bugbot for commit 62605e1. This will update automatically on new commits. Configure here.