perf!: Add accountIdByAddress to AccountsController state#7893
perf!: Add accountIdByAddress to AccountsController state#7893
accountIdByAddress to AccountsController state#7893Conversation
hmalik88
changed the title
accountIdByAddress to AccountsController stateaccountIdByAddress to AccountsController state
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| const accountId = | ||
| this.state.internalAccounts.accountIdByAddress[address.toLowerCase()]; | ||
| return accountId ? this.getAccount(accountId) : undefined; |
There was a problem hiding this comment.
We have this problem since quite some time now, but we should not lower-case addresses 😬 While EVM addresses are case-insensitive, non-EVM (at least Solana) are case-sensitives.
"Sometimes" (yes, we have this problem elsewhere 🥹) we're using the account.type to check if it's an EVM account type, and we either use checksummed addresses or lower-cased addresses.
Since we only have the address here, I wonder if we should check for the 0x prefix and lower-case only in this case?
Ideally we should just decide what is the format we want to use everywhere and we just stick to this, but we probably have some legacy code depending on non-sanitized addresses sometimes so...
Any opinion?
There was a problem hiding this comment.
Just decided to continue to lowercase and left a comment around it as we discussed!
|
|
||
| ### Added | ||
|
|
||
| - **BREAKING:** Add `accountIdByAddress` mapping to state and update `getAccountByAddress` function ([#7893](https://github.com/MetaMask/core/pull/7893)) |
There was a problem hiding this comment.
It classifies as breaking since we have to write a migration in the clients no? getAccountByAddress calls would break.
2 participants
Explanation
Currently when doing an account lookup by address we're doing an O(n) operation in the clients, this has been improved to O(1) by adding an address -> account id mapping in state. The
getAccountByAddressfunction now also takes advantage of this new piece of state to improve its lookup time.Note: This will require a migration in the clients.
References
N/A
Checklist
Note
Medium Risk
Breaking state-shape change for consumers and derived index must stay in sync with account mutations; mistakes could cause incorrect account resolution by address.
Overview
Adds a new
AccountsControllerstate fieldaccountIdByAddress(address ��accountId) to speed up address-based account lookups.getAccountByAddressnow resolves via this mapping rather than scanning accounts, and the mapping is (re)built whenever accounts are constructed/updated (constructor,updateAccounts, keyring add/remove handling, andloadBackup). The change is marked BREAKING in the accounts-controller changelog, and downstream tests were updated to include the new state shape (including in other packages that mockAccountsControllerstate).Written by Cursor Bugbot for commit 27e39dd. This will update automatically on new commits. Configure here.