Remove `eth-keyring-controller` use from `MetamaskController` #20504

mikesposito · 2023-08-17T09:28:02Z

Explanation

This PR removes the leftover EthKeyringController interactions from MetamaskController.

Updated @metamask/signature-controller to ^6.0.0 along with its patch
Removed interactions with @metamask/eth-keyring-controller from MetamaskController
- Now state passed to UI is taken from @metamask/keyring-controller
Fixed behaviour for some tests, needed after this refactoring
- createNewVaultAndKeychain test assertion slightly changed
- preferencesController.removeAddress mocked to always resolve
MetamaskController.getState() returns a flattened state object which now includes coreKeyringController's state, but the vault property is removed

References

Manual Testing Steps

Interactions with keyrings should work as before

Pre-merge author checklist

I've clearly explained:
- What problem this PR is solving
- How this problem was solved
- How reviewers can test my changes
Sufficient automated test coverage has been added

Pre-merge reviewer checklist

Manual testing (e.g. pull and build branch, run in browser, test code being changed)
PR is linked to the appropriate GitHub issue
IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

github-actions · 2023-08-17T09:28:14Z

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-25T11:29:54Z

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

Packages	Version	New capabilities	Transitives	Size	Publisher
@metamask/signature-controller	5.3.0...6.1.1	None	`+0/-0`	93.1 kB

🚮 Removed packages: @metamask/[email protected]

mikesposito · 2023-09-27T11:47:03Z

app/scripts/metamask-controller.test.js

+        await metamaskController.coreKeyringController.createNewVaultAndRestore(
+          'password',
+          TEST_SEED,
+        );


We can't really test anything properly without a vault and an unlocked KeyringController.

.yarn/patches/@metamask-keyring-controller-npm-8.0.0-806630ae4e.patch

mikesposito · 2023-09-27T12:30:32Z

package.json

@@ -245,7 +245,7 @@
    "@metamask/design-tokens": "^1.12.0",
    "@metamask/desktop": "^0.3.0",
    "@metamask/eth-json-rpc-middleware": "^11.0.0",
-    "@metamask/eth-keyring-controller": "^10.0.1",
+    "@metamask/eth-keyring-controller": "^13.0.1",


This is kept here for keyringBuilderFactory only

mikesposito · 2023-09-27T12:33:29Z

@metamaskbot update-policies

metamaskbot · 2023-09-27T12:47:31Z

Policies updated

metamaskbot · 2023-09-29T10:05:26Z

Builds ready [cff1339]

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 (703 ± 360 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	84	143	101	16	8
		domContentLoaded	66	137	89	17	8
		load	77	1729	703	750	360
		domInteractive	66	137	89	17	8

Bundle size diffs [🚀 Bundle size reduced!]

background: -50.34 KiB (-1.35%)
ui: 1.22 KiB (0.02%)
common: -1.24 KiB (-0.03%)

socket-security · 2023-09-29T12:29:48Z

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

Ignoring: @metamask/[email protected]

Next steps

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore [email protected] bar@* or ignore all packages with @SocketSecurity ignore-all

Gudahtt

LGTM!

mikesposito · 2023-09-29T13:06:28Z

@SocketSecurity ignore @metamask/[email protected]

This is our package, 6.1.1 published ~1 hour ago

mikesposito · 2023-09-29T13:28:55Z

@metamaskbot update-policies

metamaskbot · 2023-09-29T13:43:29Z

Policies updated

metamaskbot · 2023-09-29T14:23:12Z

Builds ready [9eaabf7]

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 (606 ± 336 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	86	128	100	12	6
		domContentLoaded	68	124	91	17	8
		load	80	1663	606	699	336
		domInteractive	68	124	91	17	8

Bundle size diffs [🚀 Bundle size reduced!]

background: -50.34 KiB (-1.35%)
ui: 1.22 KiB (0.02%)
common: -1.24 KiB (-0.03%)

mikesposito mentioned this pull request Aug 17, 2023

Drop @metamask/eth-keyring-controller #20508

Closed

8 tasks

cryptodev-2s force-pushed the refactor/remove-eth-keyring-controller-usage branch from 8b95f54 to d344faf Compare August 28, 2023 13:11

cryptodev-2s added the team-wallet-framework label Aug 30, 2023

mikesposito changed the title ~~refactor: remove remaining eth-keyring-controller use~~ Remove eth-keyring-controller use from MetamaskController Sep 4, 2023

mikesposito force-pushed the refactor/remove-eth-keyring-controller-usage branch 2 times, most recently from 5a68ad1 to 0a51549 Compare September 12, 2023 13:20

mikesposito force-pushed the refactor/remove-eth-keyring-controller-usage branch 3 times, most recently from c0170e6 to c921094 Compare September 25, 2023 11:20

mikesposito force-pushed the refactor/remove-eth-keyring-controller-usage branch 2 times, most recently from a903e51 to ad7f9ac Compare September 26, 2023 18:02

This comment was marked as resolved.

Sign in to view

This comment was marked as outdated.

Sign in to view

mikesposito force-pushed the refactor/remove-eth-keyring-controller-usage branch from 3d7823d to d1f00a0 Compare September 27, 2023 11:44

mikesposito commented Sep 27, 2023

View reviewed changes

.yarn/patches/@metamask-keyring-controller-npm-8.0.0-806630ae4e.patch Outdated Show resolved Hide resolved

mikesposito force-pushed the refactor/remove-eth-keyring-controller-usage branch from 9a19494 to 579f639 Compare September 27, 2023 12:29

mikesposito commented Sep 27, 2023

View reviewed changes

mikesposito force-pushed the refactor/remove-eth-keyring-controller-usage branch 4 times, most recently from 7f27e19 to 351a6cb Compare September 27, 2023 14:03

mikesposito marked this pull request as ready for review September 27, 2023 14:08

mikesposito requested review from a team as code owners September 27, 2023 14:08

mikesposito added 8 commits September 29, 2023 11:26

refactor: remove eth-keyring-controller from metamask-controller

8ad5fbb

fix: remove keyringTypes from tests

fa65e6a

refactor: update keyring-controller patch

fa0a65d

test: fix unlockHardwareWalletAccount tests

7dbf294

fix: prevent vault from being exposed to UI

ad11698

test: mock preferencesController.removeAddress

86b69b7

chore: update policies

1fce6a0

test: update state snapshot

cff1339

mikesposito force-pushed the refactor/remove-eth-keyring-controller-usage branch from 01d1761 to cff1339 Compare September 29, 2023 09:26

refactor: keyring-controller 8.0.1 and remove patch

de44deb

Gudahtt previously approved these changes Sep 29, 2023

View reviewed changes

chore: yarn dedupe

bffdd0a

mikesposito dismissed Gudahtt’s stale review via bffdd0a September 29, 2023 13:03

mikesposito requested review from Gudahtt, MajorLift, kanthesha and cryptodev-2s September 29, 2023 13:07

Update LavaMoat policies

9eaabf7

Gudahtt approved these changes Sep 29, 2023

View reviewed changes

legobeat approved these changes Sep 30, 2023

View reviewed changes

mikesposito merged commit 0a215ca into develop Sep 30, 2023
9 checks passed

mikesposito deleted the refactor/remove-eth-keyring-controller-usage branch September 30, 2023 08:02

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `eth-keyring-controller` use from `MetamaskController` #20504

Remove `eth-keyring-controller` use from `MetamaskController` #20504

mikesposito commented Aug 17, 2023 •

edited

Loading

github-actions bot commented Aug 17, 2023

socket-security bot commented Sep 25, 2023 •

edited

Loading

This comment was marked as resolved.

This comment was marked as outdated.

mikesposito Sep 27, 2023

mikesposito Sep 27, 2023

mikesposito commented Sep 27, 2023

metamaskbot commented Sep 27, 2023

metamaskbot commented Sep 29, 2023

socket-security bot commented Sep 29, 2023 •

edited

Loading

Gudahtt left a comment

mikesposito commented Sep 29, 2023

mikesposito commented Sep 29, 2023

metamaskbot commented Sep 29, 2023

metamaskbot commented Sep 29, 2023

Remove eth-keyring-controller use from MetamaskController #20504

Remove eth-keyring-controller use from MetamaskController #20504

Conversation

mikesposito commented Aug 17, 2023 • edited Loading

Explanation

References

Manual Testing Steps

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Aug 17, 2023

socket-security bot commented Sep 25, 2023 • edited Loading

This comment was marked as resolved.

This comment was marked as outdated.

mikesposito Sep 27, 2023

Choose a reason for hiding this comment

mikesposito Sep 27, 2023

Choose a reason for hiding this comment

mikesposito commented Sep 27, 2023

metamaskbot commented Sep 27, 2023

metamaskbot commented Sep 29, 2023

socket-security bot commented Sep 29, 2023 • edited Loading

Next steps

Gudahtt left a comment

Choose a reason for hiding this comment

mikesposito commented Sep 29, 2023

mikesposito commented Sep 29, 2023

metamaskbot commented Sep 29, 2023

metamaskbot commented Sep 29, 2023

Remove `eth-keyring-controller` use from `MetamaskController` #20504

Remove `eth-keyring-controller` use from `MetamaskController` #20504

mikesposito commented Aug 17, 2023 •

edited

Loading

socket-security bot commented Sep 25, 2023 •

edited

Loading

socket-security bot commented Sep 29, 2023 •

edited

Loading