Revert "Moved subscribe and filter into network controller (#16693)" #18129

Gudahtt · 2023-03-13T19:47:07Z

This reverts commit 6f6984f. That commit was an RPC middleware refactor intended to move the subscribe and filter middleware into the network controller, to simplify the process of sharing this middleware between clients.

This refactor resulted in eth_subscribe notifications being sent on the wrong connections, causing the UI to break in some cases (the UI provider connection does not support notifications). This happened because the setupProviderEngine function runs per-connection, whereas the engine setup inside the network controller is global. The global network client cannot support notifications because it has no way to route them; they'll need to stay in the per-connection provider engine.

Closes #17467

Manual Testing Steps

Attempt to reproduce [Bug]: MetaMask stays loading indefinetly and get error StreamProvider - Unknown response id for response: [object Object] when doing *some* contract interactions #17467
Test that the filter and subscribe methods work correctly
- I'd recommend running this branch of the test-dapp locally to help with testing: Add interactions involving filters & subscriptions test-dapp#202

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.

metamaskbot · 2023-03-13T20:07:44Z

Builds ready [ebb6288]

builds: chrome, firefox
builds (beta): chrome, firefox
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, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1564 ± 46 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	97	289	123	41	20
		domContentLoaded	1429	1816	1547	95	46
		load	1458	1816	1564	96	46
		domInteractive	1429	1816	1547	95	46

Bundle size diffs [🚀 Bundle size reduced!]

background: -107 bytes
ui: 0 bytes
common: 0 bytes

codecov · 2023-03-13T20:07:52Z

Codecov Report

Merging #18129 (850b47e) into develop (7215fc2) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 850b47e differs from pull request most recent head fde1a64. Consider uploading reports for the commit fde1a64 to get more accurate results

@@             Coverage Diff             @@
##           develop   #18129      +/-   ##
===========================================
- Coverage    64.00%   64.00%   -0.00%     
===========================================
  Files          910      910              
  Lines        35505    35499       -6     
  Branches      9002     9001       -1     
===========================================
- Hits         22724    22719       -5     
+ Misses       12781    12780       -1

Impacted Files	Coverage Δ
.../scripts/controllers/network/network-controller.js	`86.27% <ø> (-1.16%)`	⬇️
.../scripts/controllers/permissions/specifications.js	`73.63% <ø> (ø)`
app/scripts/metamask-controller.js	`62.32% <100.00%> (+0.42%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Gudahtt · 2023-03-13T21:42:38Z

Sufficient automated test coverage has been added

We should be able to cover this with an e2e test. I'll have to give that some thought. I don't think it'd be feasible to capture this error in a unit test, since it's more of an integration problem.

Edit: This is now covered by an e2e test.

seaona · 2023-03-14T09:20:31Z

Looks like this fixed it. I cannot reproduce the error anymore @Gudahtt

This reverts commit 6f6984f. That commit was an RPC middleware refactor intended to move the subscribe and filter middleware into the network controller, to simplify the process of sharing this middleware between clients. This refactor resulted in `eth_subscribe` notifications being sent on the wrong connections, causing the UI to break in some cases (the UI `provider` connection does not support notifications). This happened because the `setupProviderEngine` function runs per-connection, whereas the engine setup inside the network controller is global. The global network client cannot support notifications because it has no way to route them; they'll need to stay in the per-connection provider engine. Closes #17467

An e2e test has been added that confirms subscriptions are only broadcast to the site that registered them. This test fails on `develop`.

Gudahtt · 2023-03-14T17:59:51Z

An e2e test has been added to confirm that this is working. It doesn't test the UI breakage, but it detects the problematic behavior by ensuring subscriptions from one dapp don't leak to another. Testing the UI directly didn't seem ideal because we may want to restore UI support for RPC notifications at some point.

Gudahtt · 2023-03-14T18:03:26Z

test/e2e/tests/eth-subscribe.spec.js

+          });
+        `);
+
+        // Verify that new block is seen on first dapp


We use a default block time of 2 seconds in our Ganache setup. Our findElement timeout is more than long enough to wait for this, but in the future we can improve this test by explicitly mining a block here using the evm_mine method enabled by this PR: #18149

Gudahtt · 2023-03-14T18:05:03Z

test/e2e/tests/eth-subscribe.spec.js

+        await driver.openNewPage('http://127.0.0.1:8080/');
+
+        const setupSubscriptionListener = `
+          const responseContainer = document.createElement('div');


We can replace this with the built-in test dapp subscribe functionality after this PR is merged: MetaMask/test-dapp#202

test/e2e/tests/eth-subscribe.spec.js

adonesky1 · 2023-03-14T20:51:05Z

Test that the filter and subscribe methods work correctly
I'd recommend running this branch of the test-dapp locally to help with testing: https://github.com/MetaMask/test-
dapp/pull/202

I tried this both on this branch and on develop and it worked on both...

adonesky1

LGTM! Tested and verified that eth_subscribe subscription messages are no longer broadcast to all pages where we inject our provider

test/e2e/tests/eth-subscribe.spec.js

metamaskbot · 2023-03-14T23:30:57Z

Builds ready [850b47e]

builds: chrome, firefox
builds (beta): chrome, firefox
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, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1568 ± 50 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	93	133	116	12	6
		domContentLoaded	1386	1794	1555	108	52
		load	1386	1815	1568	105	50
		domInteractive	1386	1794	1555	108	52

Bundle size diffs [🚀 Bundle size reduced!]

background: -107 bytes
ui: 0 bytes
common: 0 bytes

metamaskbot · 2023-03-14T23:31:02Z

Builds ready [850b47e]

builds: chrome, firefox
builds (beta): chrome, firefox
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, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1568 ± 50 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	93	133	116	12	6
		domContentLoaded	1386	1794	1555	108	52
		load	1386	1815	1568	105	50
		domInteractive	1386	1794	1555	108	52

Bundle size diffs [🚀 Bundle size reduced!]

background: -107 bytes
ui: 0 bytes
common: 0 bytes

chloeYue

LGTM, thanks!

metamaskbot · 2023-03-15T14:46:11Z

Builds ready [fde1a64]

builds: chrome, firefox
builds (beta): chrome, firefox
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, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1640 ± 62 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	96	151	117	13	6
		domContentLoaded	1455	1879	1619	109	52
		load	1455	2010	1640	129	62
		domInteractive	1455	1879	1619	109	52

Bundle size diffs [🚀 Bundle size reduced!]

background: -107 bytes
ui: 0 bytes
common: 0 bytes

Gudahtt marked this pull request as ready for review March 13, 2023 21:41

Gudahtt requested a review from a team as a code owner March 13, 2023 21:41

Gudahtt requested a review from chloeYue March 13, 2023 21:41

seaona mentioned this pull request Mar 14, 2023

[Bug]: MetaMask stays loading indefinetly and get error StreamProvider - Unknown response id for response: [object Object] when doing *some* contract interactions #17467

Closed

Gudahtt mentioned this pull request Mar 14, 2023

Allow Ganache RPC methods in e2e test #18149

Closed

8 tasks

Add e2e test

850b47e

An e2e test has been added that confirms subscriptions are only broadcast to the site that registered them. This test fails on `develop`.

Gudahtt force-pushed the revert-filter-subscribe-refactor branch from ebb6288 to 850b47e Compare March 14, 2023 17:54

Gudahtt commented Mar 14, 2023

View reviewed changes

adonesky1 reviewed Mar 14, 2023

View reviewed changes

test/e2e/tests/eth-subscribe.spec.js Outdated Show resolved Hide resolved

adonesky1 previously approved these changes Mar 14, 2023

View reviewed changes

Improve comments

2aa96da

Gudahtt dismissed adonesky1’s stale review via 2aa96da March 14, 2023 21:31

adonesky1 reviewed Mar 14, 2023

View reviewed changes

test/e2e/tests/eth-subscribe.spec.js Outdated Show resolved Hide resolved

Fix typo

fde1a64

adonesky1 approved these changes Mar 14, 2023

View reviewed changes

chloeYue approved these changes Mar 15, 2023

View reviewed changes

Gudahtt merged commit bd23a49 into develop Mar 15, 2023

Gudahtt deleted the revert-filter-subscribe-refactor branch March 15, 2023 14:46

github-actions bot locked and limited conversation to collaborators Mar 15, 2023

PeterYinusa added the rc-cherry-picked label Mar 30, 2023

Revert "Moved subscribe and filter into network controller (#16693)" #18129

Revert "Moved subscribe and filter into network controller (#16693)" #18129

Conversation

Gudahtt commented Mar 13, 2023

Manual Testing Steps

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

metamaskbot commented Mar 13, 2023

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Gudahtt commented Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seaona commented Mar 14, 2023

Uh oh!

Gudahtt commented Mar 14, 2023

Uh oh!

Gudahtt Mar 14, 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!

Gudahtt Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adonesky1 commented Mar 14, 2023

Uh oh!

adonesky1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

metamaskbot commented Mar 14, 2023

Uh oh!

metamaskbot commented Mar 14, 2023

Uh oh!

chloeYue left a comment

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Mar 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov bot commented Mar 13, 2023 •

edited

Loading

Gudahtt commented Mar 13, 2023 •

edited

Loading

Gudahtt Mar 14, 2023 •

edited

Loading