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!
| snap: { | ||
| name: keyring.snapId, | ||
| enabled: true, | ||
| id: keyring.snapId, | ||
| }, |
There was a problem hiding this comment.
We could query this from the SnapController, but that requires a new allowed action, thus a breaking change (but at least that would mimic what the current big SnapKeyring is doing)
![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 aa8bb9f. Configure here.
| return keyringType === (KeyringTypes.snap as string); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
isNormalKeyringType misclassifies Snap keyring v2 as normal
Medium Severity
isNormalKeyringType only excludes v1 snap keyrings via isSnapKeyringType (which checks against KeyringTypes.snap), but doesn't exclude the newly introduced v2 snap keyring type (KeyringType.Snap). Since these are different string values, calling isNormalKeyringType(KeyringType.Snap) incorrectly returns true. This publicly exported function would misclassify Snap keyring v2 accounts as "normal" keyrings, which could cause consumers to apply incorrect handling for those accounts.
Reviewed by Cursor Bugbot for commit aa8bb9f. Configure here.
1 participant
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 on keyring state changes and account reconstruction in
updateAccounts, which can impact account lists/selection if v2 keyring detection or lookup behavior differs from expectations.Overview
AccountsControllernow recognizes Snap keyring v2 (KeyringType.Snap) accounts during bothKeyringController:stateChangesyncing andupdateAccounts, retrieving them viaSnapKeyringV2.lookupByAddressand injecting minimal Snap metadata into the resulting internal account.Utilities were extended to map the v2 Snap keyring type to “Snap Account” and to detect v2 Snap keyring types (
isSnapKeyringV2Type), and the keyring state-change patching logic was simplified to a single unified patch. Tests and changelog were updated to cover v2 add/skip edge cases (missing keyring instance, deleted-before-add), and package scripts now run messenger action type generation/checking with theoxfmtformatter.Reviewed by Cursor Bugbot for commit aa8bb9f. Bugbot is set up for automated code reviews on this repo. Configure here.