feat(accounts-controller): add support for Snap keyring v2#8513
feat(accounts-controller): add support for Snap keyring v2#8513
Conversation
ccharly
force-pushed
the
cc/feat/accounts-controller-snap-keyring-v2
branch
from
2851f32 to
235c68e
Compare
| const generatePatch = (): StatePatch => { | ||
| return { | ||
| previous: {}, | ||
| added: [], | ||
| updated: [], | ||
| removed: [], | ||
| }; | ||
| }; | ||
| const patches = { | ||
| snap: generatePatch(), | ||
| normal: generatePatch(), | ||
| }; | ||
|
|
||
| // Gets the patch object based on the keyring type (since Snap accounts and other accounts | ||
| // are handled differently). | ||
| const patchOf = (type: string): StatePatch => { | ||
| if (isSnapKeyringType(type)) { | ||
| return patches.snap; | ||
| } | ||
| return patches.normal; |
There was a problem hiding this comment.
Just removed all this since we don't need that separation at all!
| metadata: { | ||
| name: '', | ||
| importTime: Date.now(), | ||
| lastSelected: 0, | ||
| keyring: { | ||
| type: KeyringType.Snap, | ||
| }, | ||
| snap: { | ||
| name: keyring.snapId, | ||
| enabled: true, | ||
| id: keyring.snapId, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The new Snap keyring v2 only use KeyringAccount, thus we don't have any metadata for those!
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c9d2191. Configure here.
| // Snap keyring v2 are "per-Snaps", so we need to iterate over all of them to find the account. | ||
| for (const keyring of keyrings) { | ||
| if (keyring instanceof SnapKeyringV2) { |
There was a problem hiding this comment.
This seems inefficient, can we keep a map in memory for faster lookups?
There was a problem hiding this comment.
maybe thats an overkill given that there won't be hundreds of snap keyrings
There was a problem hiding this comment.
No actually I added that a safe-guard (with a small runtime cost yep) but :getKeyringsByType should """guarantee""" us that we get SnapKeyringV2 instances.
Also, I couldn't use :withKeyring* here because this code has to be synchronous 😬 Ultimately, account storage will get delegated to the KeyringController that will keep all KeyringAccounts in memory (for synchronous access).
We won't need that trick anymore once everything will be migrated to v2!
2 participants
Explanation
Adding support for Snap keyring v2 for this controller. For now, those keyrings are not in use, but we will need this to be able to the accounts associated with those new keyrings.
References
N/A
Checklist
Note
Medium Risk
Touches account synchronization logic and adds a new path for constructing internal accounts from Snap v2 keyrings; incorrect matching or metadata injection could cause missing/incorrect accounts during keyring state updates.
Overview
Adds support for
SnapKeyringv2 accounts so theAccountsControllercan materialize Snap v2 keyring addresses into internal accounts duringKeyringController:stateChangeandupdateAccounts.This introduces Snap v2 type detection (
KeyringType.Snap), looks up accounts viaSnapKeyringV2.lookupByAddressacross per-snap keyring instances, and injects minimalmetadata(includingmetadata.snap.id) when converting those results into internal accounts. Utilities and tests are updated to recognize/label the v2 keyring type and cover edge cases where the v2 keyring instance/account cannot be found, and the changelog documents the new support.Reviewed by Cursor Bugbot for commit c49b5a7. Bugbot is set up for automated code reviews on this repo. Configure here.