refactor: use `withKeyring` method #25435

mikesposito · 2024-06-20T10:00:26Z

Description

With @metamask/keyring-controller v16 a new method is available which simplify safe direct keyring interactions which are not available through KeyringController.

Through withKeyring it's possible to lock KeyringController's main mutex and select a specific keyring to interact with while KeyringController will be unusable concurrently. withKeyring also takes care of persisting keyrings and updating the controller state at the end of the operation, which makes it possible to use it in substitution of getKeyringsByType and persistAllKeyrings.

Currently, most of the direct keyring interactions in the extension are made for hardware devices and snaps.

This PR is supposed to be merged after #25033

Related issues

Fixes: #24276

Manual testing steps

Affected workflows may include:

Lock
Unlock
Add account
Remove account
Connect hardware wallet
All hardware wallet interactions

Screenshots/Recordings

N/A

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.

mikesposito · 2024-06-20T10:04:17Z

app/scripts/metamask-controller.js

+        if (this.isKeyringAvailable(LedgerKeyring.type)) {
+          await this.withKeyringForDevice(
+            { name: HardwareDeviceNames.ledger },
+            async (keyring) => this.setLedgerTransportPreference(keyring),
+          );
+        }


I think this could also be completely removed, since there shouldn't be any Ledger keyring right after creating (or restoring) the vault.

The this.isKeyringAvailable(LedgerKeyring.type) condition helps not creating the keyring in case it was not present in the first place

Unfortunately we creates a new Ledger keyring for each user irrespective of whether they are Ledger users. This does seem bad, but I am unsure what the consequences would be for stopping this practice. The comment does suggest that setting this preference as soon as possible was critical, but I don't recall why.

Could we consider continuing the questionable practice of creating a Ledger keyring for all users (as we currently do), and stop doing this in a separate PR? So that we can reduce the changes of complications from this refactor.

We might also want to drop the await here, given the comment above

Good point, we can address this in a separate PR. I removed isKeyringAvailable from the two occurrences of this practice for Ledger (and the isKeyringAvailable function itself, since it is now unused), and left the promise dangling.

Perhaps a good place to set the transport preference would be in withKeyringForDevice since it is used for all hardware keyring interactions

app/scripts/metamask-controller.js

mikesposito · 2024-06-24T14:28:18Z

app/scripts/metamask-controller.js

-    const keyring = await this.keyringController.getKeyringForAccount(address);
    // Remove account from the keyring
    await this.keyringController.removeAccount(address);
-    const updatedKeyringAccounts = keyring ? await keyring.getAccounts() : {};
-    if (updatedKeyringAccounts?.length === 0) {
-      keyring.destroy?.();
-    }


KeyringController calls keyring.destroy() automatically when the keyring is left with no accounts

mikesposito · 2024-06-24T15:46:10Z

app/scripts/metamask-controller.js

-    const keyring = await this.getKeyringForDevice(deviceName);
+    return this.withKeyringForDevice({ name: deviceName }, async (keyring) => {
+      for (const address of keyring.accounts) {
+        await this._onAccountRemoved(address);


By extracting side effects from removeAccount we can execute side effects here without ending up in a deadlock.

The account is removed from KeyringController anyway when calling keyring.forgetDevice()

Hmm. These side effects are a little strange (permissions should be automatically removed when an account is deleted, for instance). But I see that this is similar to how it already works today. Seems like a reasonable solution to me

I agree. Also, ideally, side effects should be executed after the account removal instead of before since an error could occur (that now would also trigger a rollback)

Gudahtt · 2024-06-25T20:24:34Z

I have temporarily moved this back into draft to resolve conflicts

metamaskbot · 2024-07-12T10:05:55Z

Builds ready [2baee85]

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, 5
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (251 ± 253 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	76	156	108	23	11
		domContentLoaded	10	67	27	17	8
		load	45	1957	251	527	253
		domInteractive	10	67	27	17	8

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

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

codecov · 2024-07-12T10:07:23Z

Codecov Report

Attention: Patch coverage is 64.00000% with 27 lines in your changes missing coverage. Please review.

Project coverage is 69.75%. Comparing base (d243eed) to head (b93dd74).
Report is 9 commits behind head on develop.

Files	Patch %	Lines
app/scripts/metamask-controller.js	64.00%	27 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #25435      +/-   ##
===========================================
- Coverage    69.75%   69.75%   -0.00%     
===========================================
  Files         1400     1400              
  Lines        49419    49421       +2     
  Branches     13668    13664       -4     
===========================================
  Hits         34470    34470              
- Misses       14949    14951       +2

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

app/scripts/metamask-controller.js

Gudahtt

Looks great! Just one pending issue about the unlockedAddress casing, and one nit.

Gudahtt

LGTM!

sonarcloud · 2024-07-18T12:11:09Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

cryptodev-2s

LGTM!

metamaskbot · 2024-07-18T19:13:09Z

Builds ready [b93dd74]

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, 5
- common: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (106 ± 31 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	80	482	148	88	42
		domContentLoaded	10	257	48	51	25
		load	44	356	106	65	31
		domInteractive	10	257	48	51	25

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

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

mikesposito changed the base branch from develop to bump-keyring-controller June 20, 2024 10:00

metamaskbot added the team-wallet-framework label Jun 20, 2024

mikesposito commented Jun 20, 2024

View reviewed changes

mikesposito commented Jun 24, 2024

View reviewed changes

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

mikesposito commented Jun 24, 2024

View reviewed changes

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

mikesposito commented Jun 24, 2024

View reviewed changes

mikesposito marked this pull request as ready for review June 24, 2024 14:31

mikesposito requested a review from a team as a code owner June 24, 2024 14:31

mikesposito commented Jun 24, 2024

View reviewed changes

Base automatically changed from bump-keyring-controller to develop June 25, 2024 08:49

ccharly requested review from a team as code owners June 25, 2024 08:49

Gudahtt marked this pull request as draft June 25, 2024 20:24

Gudahtt changed the base branch from develop to bump-keyring-controller June 25, 2024 20:26

mikesposito added 14 commits June 25, 2024 18:00

refactor: use withKeyring in MetamaskController

309d61e

fix: unlockHardwareWalletAccount account label assignment

4ff2c60

fix: no-shadow var

b8139cf

test: remove unapplicable test case

6ce4941

test: change getKeyringForDevice to withKeyringForDevice

7953eff

fix: shadow var

5f83a21

test: remove outdated cases

77723b5

fix: unlockHardwareWalletAccount should return normalized address

ac50918

refactor: leave setLedgerTransportPreference promise dangling

c896f29

fix: deadlock on account removal

a25b945

test: remove outdated cases

10fc798

fix: forgetDevice should remove keyring through this.removeAccount

710cdd9

refactor: extract side effects from removeAccount

b3bc905

fix: removeAccount should return the removed address

4e9bd9f

Gudahtt force-pushed the use-with-keyring branch from 4276021 to 4e9bd9f Compare June 25, 2024 20:34

Gudahtt marked this pull request as ready for review June 25, 2024 20:34

Merge branch 'develop' into use-with-keyring

45be5c4

mikesposito requested a review from a team July 9, 2024 09:27

mikesposito added 2 commits July 10, 2024 13:21

Merge branch 'develop' into use-with-keyring

7958f65

Merge branch 'develop' into use-with-keyring

2baee85

mikesposito and others added 2 commits July 15, 2024 10:46

Merge branch 'develop' into use-with-keyring

bbf5af9

refactor: remove isKeyringAvailable

a7397d9

Gudahtt reviewed Jul 16, 2024

View reviewed changes

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

Gudahtt reviewed Jul 16, 2024

View reviewed changes

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

Gudahtt reviewed Jul 16, 2024

View reviewed changes

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

Gudahtt requested changes Jul 16, 2024

View reviewed changes

refactor: normalize unlockedAccount address

01ca220

Gudahtt previously approved these changes Jul 16, 2024

View reviewed changes

refactor: rename deviceSelector argument

ee14fba

mikesposito dismissed Gudahtt’s stale review via ee14fba July 16, 2024 14:20

Merge branch 'develop' into use-with-keyring

c09d281

Gudahtt approved these changes Jul 16, 2024

View reviewed changes

mcmire requested a review from a team July 16, 2024 15:19

Gudahtt requested a review from a team July 17, 2024 12:51

mikesposito added 2 commits July 18, 2024 10:04

Merge branch 'develop' into use-with-keyring

10e3236

Merge branch 'develop' into use-with-keyring

b93dd74

cryptodev-2s approved these changes Jul 18, 2024

View reviewed changes

mikesposito merged commit def6b15 into develop Jul 18, 2024
79 checks passed

mikesposito deleted the use-with-keyring branch July 18, 2024 20:01

github-actions bot locked and limited conversation to collaborators Jul 18, 2024

metamaskbot added the release-12.3.0 Issue or pull request that will be included in release 12.3.0 label Jul 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use `withKeyring` method #25435

refactor: use `withKeyring` method #25435

mikesposito commented Jun 20, 2024 •

edited

Loading

mikesposito Jun 20, 2024

Gudahtt Jul 15, 2024

Gudahtt Jul 15, 2024

mikesposito Jul 16, 2024 •

edited

Loading

mikesposito Jun 24, 2024

mikesposito Jun 24, 2024

Gudahtt Jul 16, 2024

mikesposito Jul 16, 2024

Gudahtt commented Jun 25, 2024

metamaskbot commented Jul 12, 2024

codecov bot commented Jul 12, 2024 •

edited

Loading

Gudahtt left a comment

Gudahtt left a comment

sonarcloud bot commented Jul 18, 2024

cryptodev-2s left a comment

metamaskbot commented Jul 18, 2024

refactor: use withKeyring method #25435

refactor: use withKeyring method #25435

Conversation

mikesposito commented Jun 20, 2024 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Pre-merge author checklist

Pre-merge reviewer checklist

mikesposito Jun 20, 2024

Choose a reason for hiding this comment

Gudahtt Jul 15, 2024

Choose a reason for hiding this comment

Gudahtt Jul 15, 2024

Choose a reason for hiding this comment

mikesposito Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

mikesposito Jun 24, 2024

Choose a reason for hiding this comment

mikesposito Jun 24, 2024

Choose a reason for hiding this comment

Gudahtt Jul 16, 2024

Choose a reason for hiding this comment

mikesposito Jul 16, 2024

Choose a reason for hiding this comment

Gudahtt commented Jun 25, 2024

metamaskbot commented Jul 12, 2024

codecov bot commented Jul 12, 2024 • edited Loading

Codecov Report

Gudahtt left a comment

Choose a reason for hiding this comment

Gudahtt left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jul 18, 2024

Quality Gate passed

cryptodev-2s left a comment

Choose a reason for hiding this comment

metamaskbot commented Jul 18, 2024

refactor: use `withKeyring` method #25435

refactor: use `withKeyring` method #25435

mikesposito commented Jun 20, 2024 •

edited

Loading

mikesposito Jul 16, 2024 •

edited

Loading

codecov bot commented Jul 12, 2024 •

edited

Loading