Skip to content

refactor(keyring-snap-bridge)!: remove SnapKeyringV1 inheritance#584

Merged
ccharly merged 12 commits into
mainfrom
cc/refactor/remove-v1-inheritance
Jul 6, 2026
Merged

refactor(keyring-snap-bridge)!: remove SnapKeyringV1 inheritance#584
ccharly merged 12 commits into
mainfrom
cc/refactor/remove-v1-inheritance

Conversation

@ccharly

@ccharly ccharly commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Removing inheritance so we the remaining v1 usage are explicit, since SnapKeyring (v2) was implicitly exposing v1 methods, it made it pretty difficult to sunset v1 completely.


Note

High Risk
Breaking public API and behavior split across account creation, signing, and snap messages in security-sensitive keyring code; incorrect v1/v2 routing could break snaps or block v2-only accounts.

Overview
Breaking: v2 SnapKeyring no longer extends SnapKeyringV1. V1 behavior is exposed only via an optional .v1 getter (set when the snap manifest has no endowment:keyring capabilities; undefined for v2-only snaps).

The per-snap v2 class now owns its registry and v2 KeyringInternalSnapClient, and on deserialize may compose a SnapKeyringV1 that shares the same registry. createAccounts / submitRequest route to v1 or the v2 client depending on snap version (including the v1 { options } wrapper vs flat v2 options).

The top-level SnapKeyring coordinator routes legacy v1 entry points (messages, signing, user ops, etc.) through getKeyringV1For and throws clear errors when the target snap is v2-only. setSelectedAccounts uses keyring.v1?. so v2 snaps are skipped.

SnapKeyringV1 gains optional shared registry, public bindSnapId, and createAccounts for v1 RPC. Changelog and tests document the API shift and v1/v2 routing.

Reviewed by Cursor Bugbot for commit 693e6f3. Bugbot is set up for automated code reviews on this repo. Configure here.

@ccharly ccharly marked this pull request as ready for review July 5, 2026 15:40
@ccharly ccharly requested a review from a team as a code owner July 5, 2026 15:40

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ 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 5e18a2b. Configure here.

Comment thread packages/keyring-snap-bridge/src/SnapKeyring.ts
@ccharly ccharly changed the title refactor!: remove SnapKeyringV1 inheritance refactor(keyring-snap-bridge)!: remove SnapKeyringV1 inheritance Jul 6, 2026
Comment thread packages/keyring-snap-bridge/src/SnapKeyring.ts

@hmalik88 hmalik88 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving with the caveat that we don't merge until we test integration against the various preview builds we have up (sol, core, this)

@ccharly ccharly added this pull request to the merge queue Jul 6, 2026
@ccharly

ccharly commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Was able to create account and the new .v1 is well defined when testing with the extension. Merging now so we can release and update core and the Solana Snap!

Merged via the queue into main with commit d1d8459 Jul 6, 2026
41 checks passed
@ccharly ccharly deleted the cc/refactor/remove-v1-inheritance branch July 6, 2026 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants