feat(keyring-snap-bridge): moving v1 SnapKeyring implementation to SnapKeyringV1#507
feat(keyring-snap-bridge): moving v1 SnapKeyring implementation to SnapKeyringV1#507
SnapKeyring implementation to SnapKeyringV1#507Conversation
ccharly
force-pushed
the
cc/snap-keyring-v2
branch
from
e8850d7 to
f9674dc
Compare
|
@metamaskbot publish-preview |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Preview builds have been published. See these instructions (from the Expand for full list of packages and versions. |
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 32951e7. Configure here.
| // error dialogs). | ||
| onceSaved.reject(error); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Empty keyring not cleaned up after async saveState failure
Low Severity
In #handleAccountCreated, when saveState() fails asynchronously, the catch handler removes the account from the registry and calls onUnregister, but the now-empty keyring is never removed from SnapKeyring.#snapKeyrings. The old code called #deleteAccount which included #removeSnapKeyringIfEmpty. The finally block in SnapKeyring.handleKeyringSnapMessage has already executed by the time this fire-and-forget catch runs, so the empty keyring lingers until the next deserialize.
Reviewed by Cursor Bugbot for commit 32951e7. Configure here.
There was a problem hiding this comment.
saveState should never really fail. And handling the keyring removal inside the keyring itself (in this async flow) would be a real challenge.
IMO, that's acceptable, since and empty keyring has no real bad side-effects!
2 participants
Following-up on:
The previous PR moved the v2 implementation into a proper class (per-Snaps), to slowly extract the monolith logic into more logical piece that we will re-use when splitting the existing
SnapKeyring.This new PR now also move the "v1 logic" out of it and move it in a dedicated keyring class (per-Snaps). The
SnapKeyringV2now extends this v1, making theSnapKeyringV2compatible with the v2 interface AND also supports the old one (allowing us to do a progressive update with the new incomingwithKeyringV2).The remaining part on the
SnapKeyringacts now as a coordinator/proxy around those new keyring objects. In the future, we would want to move this in a dedicated service (e.g theSnapAccountService) and each keyrings would live as usual, on theKeyringController, see:Note
Medium Risk
Refactors core Snap keyring message routing and request/account lifecycle handling into new per-snap classes, which could affect account events and pending request resolution across snaps. Adds teardown behavior (
destroy) that rejects in-flight async requests when a snap keyring is removed or state is reloaded.Overview
Refactors Snap keyring implementation into per-snap classes. Introduces
SnapKeyringV1(legacy event-driven flow + request lifecycle) and makesSnapKeyringV2extend it, so v2 batch APIs and v1 events share the same per-snap registry/client.SnapKeyringis simplified into a coordinator/proxy: it lazily creates per-snap keyrings (only onAccountCreated), routes signing/requests to the owning snap keyring, and now throws when receiving messages for unknown snap keyrings. It also adds explicit per-snap teardown viadestroy()(used on last-account removal and beforedeserialize) to reject pending async requests, and serializescreateAccountsacross snaps with a global mutex.Tests are updated to reflect the new unknown-snap error behavior and to assert pending requests are rejected when the last account/keyring is removed;
SnapKeyringV1is exported fromindex.tsand the changelog is updated.Reviewed by Cursor Bugbot for commit 32951e7. Bugbot is set up for automated code reviews on this repo. Configure here.