Remove Snap accounts even if the snap is disabled #21087

danroc · 2023-09-28T09:41:34Z

Description

Currently, when removing a snap, we send a listAccounts request to the snap to obtain the list of accounts it owns, then we proceed to remove them one by one. However, if the snap is disabled, we cannot retrieve this list and an error is thrown, blocking the snap removal.

This PR resolves this issue by retrieving the list of accounts from the SnapKeyring instead of calling the snap. By doing so, it also addresses another bug: the source of truth about the accounts managed by a snap should be the SnapKeyring rather than the snap itself. This is because the snap could potentially spoof addresses owned by other keyrings, leading to their inadvertent removal.

Fixes: https://github.com/MetaMask/accounts-planning/issues/55
Depends on: #20684

Manual testing steps

Install Flask, and onboard a SRP or create a new one.
Install the SSK.
Create 2 accounts using the SSK.
Go to "Settings" > "Snaps" > "MetaMask Simple Keyring Snap".
Disabled the snap.
Click on "Remove MetaMask SSK".
Confirm by clicking on "Remove Snap".
The snap should have been removed.
Check the console of the extension, you should see the following logs:

Screenshots/Recordings

Screen.Recording.2023-09-28.at.12.44.40.mov

Pre-merge author checklist

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 · 2023-09-28T09:41:47Z

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.

socket-security · 2023-09-29T14:08:06Z

Updated dependencies detected. Learn more about Socket for GitHub ↗︎

Packages	Version	New capabilities	Transitives	Size	Publisher
@metamask/eth-snap-keyring	0.3.0...0.3.1	None	`+0/-0`	94 kB	metamaskbot

metamaskbot · 2023-09-29T14:57:15Z

Builds ready [684da5c]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1071 ± 348 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	84	127	96	11	5
		domContentLoaded	69	119	88	12	6
		load	82	1803	1071	724	348
		domInteractive	69	119	88	12	6

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

background: 159 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

codecov · 2023-09-29T14:57:23Z

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (8fcd83b) 68.43% compared to head (1754e47) 68.41%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #21087      +/-   ##
===========================================
- Coverage    68.43%   68.41%   -0.02%     
===========================================
  Files         1012     1012              
  Lines        40507    40508       +1     
  Branches     10826    10823       -3     
===========================================
- Hits         27718    27710       -8     
- Misses       12789    12798       +9

Files	Coverage Δ
app/scripts/lib/snap-keyring/index.ts	`100.00% <100.00%> (ø)`
app/scripts/lib/snap-keyring/snap-keyring.ts	`17.65% <40.00%> (-1.10%)`	⬇️
app/scripts/metamask-controller.js	`53.55% <25.00%> (-0.34%)`	⬇️
ui/store/actions.ts	`43.35% <0.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

gantunesr

I didn't run the client but the code LGTM

app/scripts/metamask-controller.js

owencraston

Code looks ✅
Ran through the steps and they all were 🤌

Currently, when removing a snap, we send a listAccounts request to the snap to obtain the list of accounts it owns, then we proceed to remove them one by one. However, if the snap is disabled, we cannot retrieve this list and an error is thrown, blocking the snap removal. This PR resolves this issue by retrieving the list of accounts from the SnapKeyring instead of calling the snap. By doing so, it also addresses another bug: the source of truth about the accounts managed by a snap should be the SnapKeyring rather than the snap itself. This is because the snap could potentially spoof addresses owned by other keyrings, leading to their inadvertent removal.

Mrtenz · 2023-09-29T18:20:01Z

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

+ * @param snapId - Snap ID to get accounts for.
+ * @returns The addresses of the accounts.
+ */
+export const getAccountsBySnapId = async (controller: any, snapId: string) => {


Can this be typed?

I just pushed a commit to address this with better types.

owencraston · 2023-09-29T19:23:22Z

I added better types to the getAccountsBySnapId method and confirmed it is still working as expected.
https://github.com/MetaMask/metamask-extension/assets/22918444/6dabb015-7873-4390-9504-7d09c2f33eff

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

metamaskbot · 2023-09-29T20:17:47Z

Builds ready [1754e47]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (872 ± 372 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	84	123	100	12	6
		domContentLoaded	70	120	94	15	7
		load	82	1884	872	775	372
		domInteractive	70	120	94	15	7

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

danroc added team-accounts DO-NOT-MERGE Pull requests that should not be merged type-bug labels Sep 28, 2023

danroc marked this pull request as ready for review September 28, 2023 10:47

danroc requested review from a team and kumavis as code owners September 28, 2023 10:47

danroc force-pushed the feature/remove-account-snap branch 2 times, most recently from 5c325aa to 1417058 Compare September 29, 2023 17:05

gantunesr previously approved these changes Sep 29, 2023

View reviewed changes

app/scripts/metamask-controller.js Outdated Show resolved Hide resolved

gantunesr added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Sep 29, 2023

danroc force-pushed the feature/remove-account-snap branch from 1417058 to 79b6e5c Compare September 29, 2023 17:28

owencraston previously approved these changes Sep 29, 2023

View reviewed changes

danroc added 2 commits September 29, 2023 20:10

refactor: move getAccountsBySnapId() to snap-keyring.ts

6b307df

danroc dismissed stale reviews from owencraston and gantunesr via 6b307df September 29, 2023 18:11

danroc force-pushed the feature/remove-account-snap branch from 79b6e5c to 6b307df Compare September 29, 2023 18:11

owencraston previously approved these changes Sep 29, 2023

View reviewed changes

Mrtenz reviewed Sep 29, 2023

View reviewed changes

better types for getAccountsBySnapId

de740fb

owencraston dismissed their stale review via de740fb September 29, 2023 19:22

owencraston reviewed Sep 29, 2023

View reviewed changes

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

annotate return type to avoid casting

1754e47

Mrtenz approved these changes Sep 29, 2023

View reviewed changes

gantunesr approved these changes Sep 29, 2023

View reviewed changes

gantunesr removed DO-NOT-MERGE Pull requests that should not be merged needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Sep 29, 2023

owencraston merged commit 0988ef5 into develop Sep 29, 2023
10 of 11 checks passed

owencraston deleted the feature/remove-account-snap branch September 29, 2023 20:21

github-actions bot locked and limited conversation to collaborators Sep 29, 2023

metamaskbot added the release-11.3.0 Issue or pull request that will be included in release 11.3.0 label Sep 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Snap accounts even if the snap is disabled #21087

Remove Snap accounts even if the snap is disabled #21087

danroc commented Sep 28, 2023 •

edited by owencraston

Loading

github-actions bot commented Sep 28, 2023

socket-security bot commented Sep 29, 2023

metamaskbot commented Sep 29, 2023

codecov bot commented Sep 29, 2023 •

edited

Loading

gantunesr left a comment

owencraston left a comment

Mrtenz Sep 29, 2023

owencraston Sep 29, 2023

owencraston commented Sep 29, 2023

metamaskbot commented Sep 29, 2023

Remove Snap accounts even if the snap is disabled #21087

Remove Snap accounts even if the snap is disabled #21087

Conversation

danroc commented Sep 28, 2023 • edited by owencraston Loading

Description

Manual testing steps

Screenshots/Recordings

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Sep 28, 2023

socket-security bot commented Sep 29, 2023

metamaskbot commented Sep 29, 2023

codecov bot commented Sep 29, 2023 • edited Loading

Codecov Report

gantunesr left a comment

Choose a reason for hiding this comment

owencraston left a comment

Choose a reason for hiding this comment

Mrtenz Sep 29, 2023

Choose a reason for hiding this comment

owencraston Sep 29, 2023

Choose a reason for hiding this comment

owencraston commented Sep 29, 2023

metamaskbot commented Sep 29, 2023

danroc commented Sep 28, 2023 •

edited by owencraston

Loading

codecov bot commented Sep 29, 2023 •

edited

Loading