Remove eth-keyring-controller from DecryptMessageController #20808

mikesposito · 2023-09-11T09:47:54Z

Explanation

This PR removes the keyringController constructor options property, as the KeyringController's method can now be invoked through the messaging system.

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-09-11T09:48:07Z

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.

codecov · 2023-09-12T13:21:54Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02% 🎉

Comparison is base (ed323ee) 68.25% compared to head (630744c) 68.26%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #20808      +/-   ##
===========================================
+ Coverage    68.25%   68.26%   +0.02%     
===========================================
  Files         1002     1002              
  Lines        40130    40137       +7     
  Branches     10730    10743      +13     
===========================================
+ Hits         27387    27398      +11     
+ Misses       12743    12739       -4

Files Changed	Coverage Δ
app/scripts/metamask-controller.js	`53.98% <ø> (+0.04%)`	⬆️
app/scripts/controllers/decrypt-message.ts	`82.91% <100.00%> (+2.32%)`	⬆️

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

metamaskbot · 2023-09-12T13:31:21Z

Builds ready [b8f95a5]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): 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
- 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 (1755 ± 70 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	115	183	141	19	9
		domContentLoaded	1534	2078	1755	146	70
		load	1534	2078	1755	146	70
		domInteractive	1534	2078	1755	146	70

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

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

metamaskbot · 2023-09-14T08:55:25Z

Builds ready [740d398]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): 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
- 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 (1568 ± 41 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	117	165	133	14	7
		domContentLoaded	1435	1758	1568	86	41
		load	1435	1759	1568	86	41
		domInteractive	1435	1758	1568	86	41

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

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

cryptodev-2s

LGTM!

app/scripts/controllers/decrypt-message.ts

metamaskbot · 2023-09-18T13:52:54Z

Builds ready [630744c]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): 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
- 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 (1462 ± 30 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	103	137	118	9	4
		domContentLoaded	1367	1603	1461	62	30
		load	1368	1604	1462	61	30
		domInteractive	1367	1603	1461	62	30

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

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

cryptodev-2s

LGTM!

Gudahtt

LGTM!

mikesposito added the team-wallet-framework label Sep 11, 2023

mikesposito requested a review from a team as a code owner September 11, 2023 09:47

mikesposito force-pushed the refactor/remove-keyring-from-decrypt-controller branch 3 times, most recently from 8ef55f4 to b8f95a5 Compare September 12, 2023 12:52

mikesposito force-pushed the refactor/remove-keyring-from-decrypt-controller branch from b8f95a5 to 740d398 Compare September 14, 2023 08:16

mikesposito force-pushed the refactor/remove-keyring-from-decrypt-controller branch from 740d398 to 8b201dd Compare September 14, 2023 14:01

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

View reviewed changes

Gudahtt reviewed Sep 14, 2023

View reviewed changes

app/scripts/controllers/decrypt-message.ts Show resolved Hide resolved

Gudahtt reviewed Sep 14, 2023

View reviewed changes

app/scripts/controllers/decrypt-message.ts Outdated Show resolved Hide resolved

Gudahtt reviewed Sep 14, 2023

View reviewed changes

app/scripts/controllers/decrypt-message.ts Show resolved Hide resolved

mikesposito dismissed cryptodev-2s’s stale review via 6cb3831 September 18, 2023 08:20

mikesposito added 3 commits September 18, 2023 15:17

refactor: remove eth-keyring-controller from DecryptMessageController

c3d0172

refactor: apply @Gudahtt suggestions

3fa6678

test: add cases for invalid encryption data

630744c

mikesposito force-pushed the refactor/remove-keyring-from-decrypt-controller branch from a31c0d8 to 630744c Compare September 18, 2023 13:17

mikesposito requested review from Gudahtt and cryptodev-2s September 18, 2023 13:17

cryptodev-2s approved these changes Sep 18, 2023

View reviewed changes

Gudahtt approved these changes Sep 18, 2023

View reviewed changes

mikesposito merged commit ca7c028 into develop Sep 18, 2023
9 checks passed

mikesposito deleted the refactor/remove-keyring-from-decrypt-controller branch September 18, 2023 14:44

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove eth-keyring-controller from DecryptMessageController #20808

Remove eth-keyring-controller from DecryptMessageController #20808

mikesposito commented Sep 11, 2023 •

edited

Loading

github-actions bot commented Sep 11, 2023

codecov bot commented Sep 12, 2023 •

edited

Loading

metamaskbot commented Sep 12, 2023

metamaskbot commented Sep 14, 2023

cryptodev-2s left a comment

metamaskbot commented Sep 18, 2023

cryptodev-2s left a comment

Gudahtt left a comment

Remove eth-keyring-controller from DecryptMessageController #20808

Remove eth-keyring-controller from DecryptMessageController #20808

Conversation

mikesposito commented Sep 11, 2023 • edited Loading

Explanation

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Sep 11, 2023

codecov bot commented Sep 12, 2023 • edited Loading

Codecov Report

metamaskbot commented Sep 12, 2023

metamaskbot commented Sep 14, 2023

cryptodev-2s left a comment

Choose a reason for hiding this comment

metamaskbot commented Sep 18, 2023

cryptodev-2s left a comment

Choose a reason for hiding this comment

Gudahtt left a comment

Choose a reason for hiding this comment

mikesposito commented Sep 11, 2023 •

edited

Loading

codecov bot commented Sep 12, 2023 •

edited

Loading