Use `importAccountWithStrategy` from core `KeyringController` #19815

mikesposito · 2023-06-29T08:48:20Z

Explanation

With this PR we start using the importAccountWithStrategy method from core KeyringController instead of calling EthKeyringController directly.

As strategies keys are different on KeyringController, they have been changed on the extension code and tests, to match the new ones.

Fixes Keyring consolidation: Use AccountImportStrategy enum from core for importAccountWithStrategy #18763
Fixes Keyring consolidation: Support new importedAccountAddress return object #18764
Fixes Keyrings consolidation: Select account after calling importAccountWithStrategy #18765

Screenshots/Screencaps

Before

After

Manual Testing Steps

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-06-29T08:48:34Z

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-06-29T09:29:56Z

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

🚮 Removed packages: ethereumjs-wallet@0.6.5

metamaskbot · 2023-06-29T10:47:26Z

Builds ready [66e876e]

builds: chrome, firefox
builds (beta): chrome
builds (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
- 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 (1752 ± 68 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	101	157	124	16	8
		domContentLoaded	1486	1997	1714	141	68
		load	1520	1998	1752	142	68
		domInteractive	1486	1997	1714	141	68

Bundle size diffs [🚀 Bundle size reduced!]

background: 108549 bytes
ui: 115007 bytes
common: -114929 bytes

codecov · 2023-06-29T10:47:33Z

Codecov Report

Merging #19815 (b5730db) into develop (9e302ea) will decrease coverage by 0.03%.
The diff coverage is 14.29%.

@@             Coverage Diff             @@
##           develop   #19815      +/-   ##
===========================================
- Coverage    68.66%   68.63%   -0.03%     
===========================================
  Files          990      989       -1     
  Lines        38274    38227      -47     
  Branches     10260    10256       -4     
===========================================
- Hits         26279    26235      -44     
+ Misses       11995    11992       -3

Files Changed	Coverage Δ
app/scripts/metamask-controller.js	`64.39% <0.00%> (-0.16%)`	⬇️
...onents/multichain/import-account/import-account.js	`21.74% <0.00%> (ø)`
...omponents/multichain/import-account/private-key.js	`46.15% <0.00%> (ø)`
ui/components/multichain/import-account/json.js	`75.00% <100.00%> (ø)`

mikesposito · 2023-07-19T11:24:17Z

Depends on #19659

mikesposito · 2023-07-25T08:50:30Z

@metamaskbot update-policies

metamaskbot · 2023-07-25T09:01:05Z

Policies updated

metamaskbot · 2023-07-25T09:39:56Z

Builds ready [b593868]

builds: chrome, firefox
builds (beta): chrome
builds (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
- 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 (1560 ± 53 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	102	191	140	25	12
		domContentLoaded	1356	1838	1560	110	53
		load	1356	1838	1560	110	53
		domInteractive	1356	1838	1560	110	53

Bundle size diffs [🚀 Bundle size reduced!]

background: -44.74 KiB (-1.04%)
ui: 118.52 KiB (1.56%)
common: -118.43 KiB (-2.99%)

mcmire

Looks good!

mikesposito · 2023-08-01T11:05:56Z

@metamaskbot update-policies

metamaskbot · 2023-08-01T11:17:47Z

Policies updated

mikesposito · 2023-08-02T12:01:27Z

Rebased and resolved yarn.lock conflicts

metamaskbot · 2023-08-02T12:38:48Z

Builds ready [af5ae69]

builds: chrome, firefox
builds (beta): chrome
builds (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
- 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 (2687 ± 163 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	203	405	313	62	30
		domContentLoaded	2008	3287	2684	339	163
		load	2008	3288	2687	338	163
		domInteractive	2008	3287	2684	339	163

Bundle size diffs [🚀 Bundle size reduced!]

background: -44.62 KiB (-1.15%)
ui: 118.52 KiB (1.54%)
common: -118.44 KiB (-3.18%)

metamaskbot · 2023-08-14T08:44:11Z

Builds ready [ec16bf0]

builds: chrome, firefox
builds (beta): chrome
builds (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
- 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 (1601 ± 42 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	117	209	140	21	10
		domContentLoaded	1470	1820	1601	87	42
		load	1470	1820	1601	87	42
		domInteractive	1470	1820	1601	87	42

Bundle size diffs [🚀 Bundle size reduced!]

background: -44.62 KiB (-1.14%)
ui: 118.52 KiB (1.54%)
common: -118.44 KiB (-3.18%)

cryptodev-2s

LGTM!

mcmire · 2023-08-14T16:19:42Z

@mikesposito Looks like there are conflicts again :(

mikesposito · 2023-08-16T09:34:30Z

@metamaskbot update-policies

metamaskbot · 2023-08-16T09:46:33Z

Policies updated

metamaskbot · 2023-08-16T10:23:49Z

Builds ready [72e8f62]

builds: chrome, firefox
builds (beta): chrome
builds (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
- 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 (1572 ± 42 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	109	156	133	14	7
		domContentLoaded	1415	1813	1572	88	42
		load	1415	1813	1572	88	42
		domInteractive	1415	1813	1572	88	42

Bundle size diffs [🚀 Bundle size reduced!]

background: -44.63 KiB (-1.14%)
ui: 118.52 KiB (1.54%)
common: -118.44 KiB (-3.18%)

metamaskbot · 2023-08-16T12:53:25Z

Builds ready [b5730db]

builds: chrome, firefox
builds (beta): chrome
builds (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
- 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 (1485 ± 42 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	107	199	126	21	10
		domContentLoaded	1350	1660	1485	88	42
		load	1350	1660	1485	87	42
		domInteractive	1350	1660	1485	88	42

Bundle size diffs [🚀 Bundle size reduced!]

background: -44.63 KiB (-1.14%)
ui: 118.52 KiB (1.54%)
common: -118.44 KiB (-3.18%)

cryptodev-2s

LGTM!

mcmire

One nit, but looks good regardless.

mcmire · 2023-08-16T19:42:49Z

app/scripts/metamask-controller.actions.test.js

      await metamaskController.createNewVaultAndKeychain('test@123');
      await Promise.all([
-        metamaskController.importAccountWithStrategy('Private Key', [
+        metamaskController.importAccountWithStrategy('privateKey', [


Nit: It might be nice if we can import an enum and use that here (and other places) so we can see the significance of this value. Not a big deal if that's not doable though.

mikesposito force-pushed the refactor/use-core-keyring-controller branch 5 times, most recently from c65c6b9 to 2d584d7 Compare July 19, 2023 09:37

mikesposito mentioned this pull request Jul 19, 2023

Use @metamask/keyring-controller #19659

Merged

8 tasks

mikesposito force-pushed the refactor/use-core-keyring-controller branch 2 times, most recently from 95313e3 to 1567ae0 Compare July 24, 2023 10:32

Base automatically changed from refactor/use-core-keyring-controller to develop July 24, 2023 18:44

mikesposito force-pushed the refactor/use-importaccount-from-kc branch from 66e876e to 63f2266 Compare July 25, 2023 08:31

mikesposito force-pushed the refactor/use-importaccount-from-kc branch from 63f2266 to 7c6a29c Compare July 25, 2023 08:51

mikesposito marked this pull request as ready for review July 25, 2023 08:53

mikesposito requested review from a team as code owners July 25, 2023 08:53

mcmire previously approved these changes Jul 31, 2023

View reviewed changes

mikesposito dismissed mcmire’s stale review via ba0ecc6 August 1, 2023 11:05

mikesposito force-pushed the refactor/use-importaccount-from-kc branch from b593868 to ba0ecc6 Compare August 1, 2023 11:05

mikesposito requested a review from mcmire August 1, 2023 11:06

mcmire previously approved these changes Aug 1, 2023

View reviewed changes

mikesposito force-pushed the refactor/use-importaccount-from-kc branch from 79d7255 to af5ae69 Compare August 2, 2023 12:01

mikesposito requested a review from mcmire August 2, 2023 12:01

mikesposito requested a review from cryptodev-2s August 3, 2023 08:19

mikesposito added the team-shared-libraries label Aug 14, 2023

mikesposito force-pushed the refactor/use-importaccount-from-kc branch from af5ae69 to ec16bf0 Compare August 14, 2023 08:08

cryptodev-2s previously approved these changes Aug 14, 2023

View reviewed changes

mikesposito dismissed cryptodev-2s’s stale review via c4d7e34 August 16, 2023 09:34

mikesposito force-pushed the refactor/use-importaccount-from-kc branch from ec16bf0 to c4d7e34 Compare August 16, 2023 09:34

mikesposito requested a review from cryptodev-2s August 16, 2023 09:34

mikesposito and others added 4 commits August 16, 2023 14:16

refactor: use new importaccount from keyring controller

2786ef4

Update LavaMoat policies

5d423a7

Update LavaMoat policies

6328e8c

Update LavaMoat policies

b5730db

mikesposito force-pushed the refactor/use-importaccount-from-kc branch from 72e8f62 to b5730db Compare August 16, 2023 12:16

cryptodev-2s approved these changes Aug 16, 2023

View reviewed changes

mcmire approved these changes Aug 16, 2023

View reviewed changes

mikesposito merged commit 3aa5b7d into develop Aug 17, 2023

mikesposito deleted the refactor/use-importaccount-from-kc branch August 17, 2023 07:11

github-actions bot locked and limited conversation to collaborators Aug 17, 2023

metamaskbot added the release-10.36.0 Issue or pull request that will be included in release 10.36.0 label Aug 17, 2023

Gudahtt added release-11.1.0 Issue or pull request that will be included in release 11.1.0 and removed release-10.36.0 Issue or pull request that will be included in release 10.36.0 labels Sep 19, 2023

Use importAccountWithStrategy from core KeyringController #19815

Use importAccountWithStrategy from core KeyringController #19815

Uh oh!

Conversation

mikesposito commented Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation

Screenshots/Screencaps

Before

After

Manual Testing Steps

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions bot commented Jun 29, 2023

Uh oh!

socket-security bot commented Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metamaskbot commented Jun 29, 2023

Uh oh!

codecov bot commented Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mikesposito commented Jul 19, 2023

Uh oh!

mikesposito commented Jul 25, 2023

Uh oh!

metamaskbot commented Jul 25, 2023

Uh oh!

metamaskbot commented Jul 25, 2023

Uh oh!

mcmire left a comment

Choose a reason for hiding this comment

Uh oh!

mikesposito commented Aug 1, 2023

Uh oh!

metamaskbot commented Aug 1, 2023

Uh oh!

mikesposito commented Aug 2, 2023

Uh oh!

metamaskbot commented Aug 2, 2023

Uh oh!

metamaskbot commented Aug 14, 2023

Uh oh!

cryptodev-2s left a comment

Choose a reason for hiding this comment

Uh oh!

mcmire commented Aug 14, 2023

Uh oh!

mikesposito commented Aug 16, 2023

Uh oh!

metamaskbot commented Aug 16, 2023

Uh oh!

metamaskbot commented Aug 16, 2023

Uh oh!

metamaskbot commented Aug 16, 2023

Uh oh!

cryptodev-2s left a comment

Choose a reason for hiding this comment

Uh oh!

mcmire left a comment

Choose a reason for hiding this comment

Uh oh!

mcmire Aug 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Use `importAccountWithStrategy` from core `KeyringController` #19815

Use `importAccountWithStrategy` from core `KeyringController` #19815

mikesposito commented Jun 29, 2023 •

edited

Loading

socket-security bot commented Jun 29, 2023 •

edited

Loading

codecov bot commented Jun 29, 2023 •

edited

Loading

mcmire Aug 16, 2023 •

edited

Loading