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.
There was a problem hiding this comment.
I don't think so since this should be constructed inside the constructor therefore we shouldn't need a migration nor breaking change
|
@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. |
mathieuartu
left a comment
mathieuartu
left a comment
There was a problem hiding this comment.
Looks good, added one possible readability improvement suggestion and a perf question
| return this.listMultichainAccounts().find( | ||
| (account) => account.address.toLowerCase() === address.toLowerCase(), | ||
| ); | ||
| const accountId = this.state.accountIdByAddress[address.toLowerCase()]; |
There was a problem hiding this comment.
Are the toLowerCase() calls necessary? What would be the outcome if we dropped this?
It could be beneficial (although minimal) in terms of perf not to rely on this, also because we often recompute the same values. My reasoning also comes from seeing the FIXME comments hehe
There was a problem hiding this comment.
Yeah, I discussed this one with @ccharly and he suggested we continue with this pattern and take a look at how we handle addresses across the app in one fell swoop (I don't feel great about lowercasing either).
There was a problem hiding this comment.
There might have been, I can't recall exactly but the conclusion was let's just do this for now and come back and fix later. The only reason I can think is if things weren't being done properly in the clients.
| const accounts = Object.values(accountsMap); | ||
| return accounts.reduce<Record<string, AccountId>>((acc, account) => { | ||
| acc[account.address.toLowerCase()] = account.id; | ||
| return acc; |
There was a problem hiding this comment.
Case-sensitive addresses can collide in new map
Medium Severity
accountIdByAddress is built with address.toLowerCase() for every account, and getAccountByAddress also lowercases lookups. For case-sensitive non-EVM addresses, different accounts can collapse to one key, so lookups can return the wrong account ID or overwrite another entry in accountIdByAddress.
Additional Locations (2)
| (account) => account.address.toLowerCase() === address.toLowerCase(), | ||
| ); | ||
| const accountId = this.state.accountIdByAddress[address.toLowerCase()]; | ||
| return accountId ? this.getAccount(accountId) : undefined; |
There was a problem hiding this comment.
Address lookup no longer returns first match
Medium Severity
getAccountByAddress now relies on accountIdByAddress, which stores only one AccountId per lowercased address. When multiple accounts share the same normalized address, later inserts overwrite earlier ones, so lookups can return a different account than before instead of the first matching account.
Additional Locations (2)
| accountIdByAddress: { | ||
| includeInStateLogs: false, | ||
| persist: false, | ||
| includeInDebugSnapshot: false, | ||
| usedInUi: true, | ||
| }, |
There was a problem hiding this comment.
Long term plan is to remove the accounts-controller. How will this be translated to the other controllers?
4 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 that requires client migrations and affects address-based account resolution paths. Risk is moderate due to widespread test/fixture updates and reliance on lowercasing addresses (noted as potentially incorrect for case-sensitive schemes).
Overview
BREAKING: Adds
accountIdByAddress(lowercased address -> account id) toAccountsControllerstate and derives/refreshes it during construction,updateAccounts, backup load, and keyring add/remove handling.getAccountByAddressnow uses this index instead of scanninglistMultichainAccounts, and tests/fixtures across dependent packages are updated to include the new state shape (withaccountIdByAddressmarked non-persisted/non-logged in metadata).Written by Cursor Bugbot for commit ceb8b26. This will update automatically on new commits. Configure here.