refactor(snap-keyring): refactor Snap keyring implementation #30244

ccharly · 2025-02-11T11:30:39Z

Description

Refactor the current implementation with better typing and by re-organizing things a bit.

We have some new modifications coming up soon on the Snap keyring flows/logic, so I'd like to have this small refactor to come first to ease the incoming PR reviews.

Requires this one to be merged first:

chore: update accounts deps + some related peer deps #30205

Related issues

N/A

Manual testing steps

Run yarn start:flask
Install the SSK: https://metamask.github.io/snap-simple-keyring/latest/
Creates an SSK account

You should see the usual dialogs for non-preinstalled Snaps (account renaming, confirmation dialogs to create the account)

Creates a Bitcoin/Solana account

Those are preinstalled, so some dialogs will be skipped

Remove those accounts

You should see a dialog (to confirm the account deletion) upon the removal of an SSK account
You should not see the same dialog for Bitcoin/Solana

Screenshots/Recordings

Before

After

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

github-actions · 2025-02-11T11:30:52Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

metamaskbot · 2025-02-11T12:17:52Z

Builds ready [da3122d]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12
- content-script: 0
- offscreen: 0

Page Load Metrics (1828 ± 65 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	428	2075	1763	335	161
		domContentLoaded	1543	2024	1797	120	58
		load	1556	2080	1828	134	65
		domInteractive	26	102	44	25	12
		backgroundConnect	9	91	30	25	12
		firstReactRender	15	80	35	22	11
		getState	5	56	15	14	7
		initialActions	0	1	0	0	0
		loadScripts	1109	1572	1321	114	55
		setupStore	6	62	20	19	9
		uiStartup	1795	2365	2086	157	75

Bundle size diffs [🚀 Bundle size reduced!]

background: 12.42 KiB (0.21%)
ui: 0 Bytes (0.00%)
common: -33.51 KiB (-0.36%)

metamaskbot · 2025-02-11T15:45:49Z

Builds ready [c319f07]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12
- content-script: 0
- offscreen: 0

Page Load Metrics (1966 ± 187 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	637	2344	1821	325	156
		domContentLoaded	1556	3461	1928	393	189
		load	1569	3469	1966	389	187
		domInteractive	23	251	52	51	25
		backgroundConnect	13	96	40	26	12
		firstReactRender	18	91	58	23	11
		getState	6	59	19	18	9
		initialActions	0	0	0	0	0
		loadScripts	1136	2682	1417	324	155
		setupStore	8	91	20	24	11
		uiStartup	1800	3947	2301	461	221

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 1.76 KiB (0.03%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

metamaskbot · 2025-02-11T18:25:51Z

Builds ready [d01d898]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12
- content-script: 0
- offscreen: 0

Page Load Metrics (1636 ± 50 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	477	1867	1575	273	131
		domContentLoaded	1433	1853	1612	104	50
		load	1445	1872	1636	104	50
		domInteractive	24	94	43	25	12
		backgroundConnect	10	72	24	18	9
		firstReactRender	15	96	50	29	14
		getState	5	61	12	15	7
		initialActions	0	1	0	0	0
		loadScripts	1018	1399	1173	93	45
		setupStore	8	70	19	20	10
		uiStartup	1682	2168	1903	138	66

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 1.76 KiB (0.03%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

metamaskbot · 2025-02-17T18:45:26Z

Builds ready [85ff164]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12
- content-script: 0
- offscreen: 0

Page Load Metrics (1632 ± 60 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1471	2004	1633	124	60
		domContentLoaded	1457	1974	1614	120	58
		load	1473	2010	1632	125	60
		domInteractive	23	92	37	20	10
		backgroundConnect	9	60	20	15	7
		firstReactRender	14	73	47	26	12
		getState	4	53	11	14	7
		initialActions	0	0	0	0	0
		loadScripts	1009	1509	1183	108	52
		setupStore	6	61	11	12	6
		uiStartup	1689	2272	1879	137	66

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 1.75 KiB (0.03%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

app/scripts/lib/snap-keyring/snaps.ts

Co-authored-by: Daniel Rocha <[email protected]>

metamaskbot · 2025-02-18T10:21:58Z

Builds ready [f0bd0b6]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12
- content-script: 0
- offscreen: 0

Page Load Metrics (1553 ± 73 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1383	1877	1559	149	71
		domContentLoaded	1357	1857	1534	146	70
		load	1365	1875	1553	153	73
		domInteractive	23	81	38	19	9
		backgroundConnect	8	59	21	16	7
		firstReactRender	13	71	35	23	11
		getState	4	57	13	15	7
		initialActions	0	1	0	0	0
		loadScripts	957	1365	1112	114	55
		setupStore	6	56	15	16	8
		uiStartup	1563	2153	1763	177	85

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 1.74 KiB (0.03%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

app/scripts/lib/snap-keyring/snaps.ts

metamaskbot · 2025-02-18T12:23:16Z

Builds ready [0783263]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12
- content-script: 0
- offscreen: 0

Page Load Metrics (1721 ± 89 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	364	1923	1623	311	149
		domContentLoaded	1499	2328	1692	182	88
		load	1508	2333	1721	185	89
		domInteractive	15	76	36	15	7
		backgroundConnect	10	81	34	25	12
		firstReactRender	14	70	31	22	10
		getState	5	39	12	8	4
		initialActions	0	1	0	0	0
		loadScripts	1052	1760	1212	157	75
		setupStore	6	61	22	20	10
		uiStartup	1723	2648	1963	202	97

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 1.87 KiB (0.03%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

ccharly added the team-accounts label Feb 11, 2025

ccharly self-assigned this Feb 11, 2025

ccharly force-pushed the refactor/snap-keyring-impl branch from 704bee8 to da3122d Compare February 11, 2025 11:43

ccharly force-pushed the refactor/snap-keyring-impl branch 2 times, most recently from 5ad24ac to c319f07 Compare February 11, 2025 15:12

ccharly marked this pull request as ready for review February 11, 2025 15:14

ccharly requested a review from a team as a code owner February 11, 2025 15:14

ccharly force-pushed the refactor/snap-keyring-impl branch from c319f07 to d01d898 Compare February 11, 2025 17:51

ccharly force-pushed the refactor/snap-keyring-impl branch from d01d898 to 5e64564 Compare February 17, 2025 09:37

refactor: refactor Snap keyring implementation

2027dea

ccharly force-pushed the refactor/snap-keyring-impl branch from 5e64564 to 2027dea Compare February 17, 2025 10:18

ccharly added 2 commits February 17, 2025 17:52

Merge branch 'main' into refactor/snap-keyring-impl

da66d99

refactor: use asserts for assertSnapIdIsValid

85ff164

Merge branch 'main' into refactor/snap-keyring-impl

6cec980

danroc reviewed Feb 18, 2025

View reviewed changes

app/scripts/lib/snap-keyring/snaps.ts Outdated Show resolved Hide resolved

danroc reviewed Feb 18, 2025

View reviewed changes

app/scripts/lib/snap-keyring/snaps.ts Outdated Show resolved Hide resolved

chore: typo + remove log

f0bd0b6

Co-authored-by: Daniel Rocha <[email protected]>

danroc previously approved these changes Feb 18, 2025

View reviewed changes

montelaidev reviewed Feb 18, 2025

View reviewed changes

app/scripts/lib/snap-keyring/snaps.ts Outdated Show resolved Hide resolved

refactor: use isSnapId

093555e

ccharly dismissed danroc’s stale review via 093555e February 18, 2025 11:30

montelaidev previously approved these changes Feb 18, 2025

View reviewed changes

ccharly added 2 commits February 18, 2025 12:48

refactor: use assertIsValidSnapId from snaps-utils

95c4fe2

test: fix mockSnapId

0783263

ccharly dismissed montelaidev’s stale review via 0783263 February 18, 2025 11:48

montelaidev approved these changes Feb 18, 2025

View reviewed changes

danroc approved these changes Feb 18, 2025

View reviewed changes

gantunesr added this pull request to the merge queue Feb 18, 2025

Merged via the queue into main with commit 6d08a65 Feb 18, 2025
73 checks passed

gantunesr deleted the refactor/snap-keyring-impl branch February 18, 2025 12:56

github-actions bot locked and limited conversation to collaborators Feb 18, 2025

metamaskbot added the release-12.14.0 Issue or pull request that will be included in release 12.14.0 label Feb 18, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(snap-keyring): refactor Snap keyring implementation #30244

refactor(snap-keyring): refactor Snap keyring implementation #30244

ccharly commented Feb 11, 2025 •

edited

Loading

github-actions bot commented Feb 11, 2025

metamaskbot commented Feb 11, 2025

metamaskbot commented Feb 11, 2025

metamaskbot commented Feb 11, 2025

metamaskbot commented Feb 17, 2025

metamaskbot commented Feb 18, 2025

metamaskbot commented Feb 18, 2025

refactor(snap-keyring): refactor Snap keyring implementation #30244

refactor(snap-keyring): refactor Snap keyring implementation #30244

Conversation

ccharly commented Feb 11, 2025 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Feb 11, 2025

metamaskbot commented Feb 11, 2025

metamaskbot commented Feb 11, 2025

metamaskbot commented Feb 11, 2025

metamaskbot commented Feb 17, 2025

metamaskbot commented Feb 18, 2025

metamaskbot commented Feb 18, 2025

ccharly commented Feb 11, 2025 •

edited

Loading