Moved subscribe and filter into network controller #16693

BelfordZ · 2022-11-25T18:32:02Z

Our middleware for handling subscription and filter-related methods (eth-json-rpc-filters) currently lives in our RPC pipeline ahead of the network stack. This commit moves this middleware to the network client middleware instead. There are two reasons for this change. First, this middleware wraps RPC methods that are supported by the network. Second, it is necessary for this middleware to live with the network client so that it will aid us in unifying the NetworkController in this repo and the NetworkController in the controllers repo.

Fixes: #16573

github-actions · 2022-11-25T18:32: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.

metamaskbot · 2022-11-25T19:05:24Z

Builds ready [6ad94ca]

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, 5
- 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, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1988 ± 107 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	93	142	110	13	6
		domContentLoaded	1564	2289	1964	212	102
		load	1564	2358	1988	222	107
		domInteractive	1564	2288	1964	212	102

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

background: 554 bytes
ui: 0 bytes
common: 0 bytes

fixes #16573

metamaskbot · 2022-11-25T21:33:43Z

Builds ready [fc3011a]

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, 5
- 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, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2242 ± 138 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	112	188	140	21	10
		domContentLoaded	1592	2659	2235	288	138
		load	1592	2659	2242	288	138
		domInteractive	1592	2659	2235	288	138

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

background: 554 bytes
ui: 0 bytes
common: 0 bytes

Gudahtt

LGTM!

Though we should manually test that this works (I haven't done that yet). I don't think these methods are covered by any e2e tests or our test dapp, unfortunately.

app/scripts/controllers/network/createJsonRpcClient.js

Gudahtt · 2022-12-08T20:32:20Z

We should come up with test instructions so that QA has something to go by.

mcmire

Anything that we add to the network client files we need to make sure is covered in the network client integration tests.

I can work on this.

Gudahtt · 2022-12-09T22:18:14Z

I agree that we need tests. It looks like mobile supports these methods via web3-provider-engine, they're part of the provider mobile uses from that package. So perhaps we can consider adding tests as a blocker for replacing web3-provider-engine.

I don't think it needs to block this PR though. The network client tests are already not exhaustive.

mcmire · 2022-12-09T22:20:36Z

@Gudahtt Sure, as long as we don't forget about it.

Dismissing the change requests as we can write tests later.

Gudahtt · 2022-12-09T22:30:25Z

@mcmire I added a ticket here: #16899 and added it to the merge network controllers epic.

brad-decker · 2022-12-13T16:57:56Z

@BelfordZ is this ready to be merged?

mcmire · 2022-12-15T00:09:16Z

I was asked to help with manually testing this PR, and to do this, I've been adding some more buttons and things to the test dapp so we can test the filter and subscription middleware. However, I don't think all of the filter middleware works as intended. I've added a couple of issues around this: MetaMask/eth-json-rpc-filters#80, MetaMask/eth-json-rpc-filters#81. So I'm not able to test all of the functionality but only some of it. I haven't tested the subscription middleware yet, but I will do that tomorrow.

mcmire · 2022-12-16T00:33:48Z

Here are the changes for the test dapp which allow us to test filters and subscriptions: MetaMask/test-dapp#202

I used this on this branch and it seems that there are some issues:

When creating a log filter, I get this JSON-RPC response: {"jsonrpc":"2.0","error":{"code":-32600,"message":"invalid json request"}}
Creating a block filter works just fine
When creating a subscription (either way), I get this response: The method "eth_subscribe" does not exist / is not available.

mcmire

Marking this as changes requested. I can look into this tomorrow.

app/scripts/metamask-controller.js

app/scripts/controllers/network/createInfuraClient.js

Gudahtt

It seems that the subscription middleware is failing because it used to be before the permission middleware, but now it's after, and the subscription methods are missing from the list of unrestricted methods (in app/scripts/controllers/permissions/specifications.js, the unrestrictedMethods variable).

app/scripts/controllers/network/createInfuraClient.js

app/scripts/controllers/network/createJsonRpcClient.js

Gudahtt · 2022-12-16T15:29:16Z

In testing out this PR, even with the suggestions I made above applied locally, I'm still having trouble getting subscriptions to work. It looks like there are two distinct problems: notifications are not propagating correctly from the network client to the dapp, and we're seeing occasional empty block responses from Infura that are resulting in the request failing.

For the empty block responses problem, this is happening due to a race condition between getLatestBlock and getBlockByNumber - there's a period of time during which the latest block number cannot be retrieved by getBlockByNumber,it lags behind slightly. This is what the retryOnEmpty middleware was doing for us - it would automatically retry.

We'll need to move these middleware up a level, into the network controller, so that we can pass them a provider that includes the retryOnEmpty middleware.

app/scripts/metamask-controller.js

app/scripts/controllers/network/network-controller.js

metamaskbot · 2022-12-17T00:30:36Z

Builds ready [9c2b960]

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, 5
- 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, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2055 ± 105 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	98	198	122	24	11
		domContentLoaded	1569	2391	2026	221	106
		load	1572	2391	2055	219	105
		domInteractive	1569	2391	2026	221	106

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

background: 106 bytes
ui: 0 bytes
common: 0 bytes

Gudahtt

LGTM!

Made the requested changes.

metamaskbot · 2022-12-19T21:21:35Z

Builds ready [911667c]

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, 5
- 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, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2329 ± 226 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	111	319	145	42	20
		domContentLoaded	1703	3647	2310	457	219
		load	1703	3647	2329	471	226
		domInteractive	1703	3647	2310	457	219

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

background: 106 bytes
ui: 0 bytes
common: 0 bytes

BelfordZ force-pushed the fix/sub-filter-into-network branch from 6ad94ca to 4f2b0d0 Compare November 25, 2022 21:00

Moved subscribe and filter into network controller

fc3011a

fixes #16573

BelfordZ force-pushed the fix/sub-filter-into-network branch from 4f2b0d0 to fc3011a Compare November 25, 2022 21:01

BelfordZ requested a review from Gudahtt November 29, 2022 17:39

BelfordZ marked this pull request as ready for review November 29, 2022 18:34

BelfordZ requested a review from a team as a code owner November 29, 2022 18:34

Gudahtt previously approved these changes Dec 6, 2022

View reviewed changes

app/scripts/controllers/network/createJsonRpcClient.js Outdated Show resolved Hide resolved

Gudahtt added the needs-qa Label will automate into QA workspace label Dec 6, 2022

mcmire previously requested changes Dec 9, 2022

View reviewed changes

mcmire approved these changes Dec 9, 2022

View reviewed changes

Gudahtt mentioned this pull request Dec 9, 2022

Add tests for JSON-RPC filter and subscription middleware #16899

Open

Merge branch 'develop' into fix/sub-filter-into-network

6daf7df

mcmire previously requested changes Dec 16, 2022

View reviewed changes

Gudahtt reviewed Dec 16, 2022

View reviewed changes

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

Gudahtt reviewed Dec 16, 2022

View reviewed changes

app/scripts/controllers/network/createInfuraClient.js Outdated Show resolved Hide resolved

Gudahtt reviewed Dec 16, 2022

View reviewed changes

app/scripts/controllers/network/createInfuraClient.js Outdated Show resolved Hide resolved

app/scripts/controllers/network/createJsonRpcClient.js Outdated Show resolved Hide resolved

Gudahtt reviewed Dec 16, 2022

View reviewed changes

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

Remove extraneous comments

e51455a

Fix issues

469bbed

mcmire dismissed Gudahtt’s stale review via 469bbed December 16, 2022 23:58

Merge branch 'develop' into fix/sub-filter-into-network

9c2b960

mcmire reviewed Dec 17, 2022

View reviewed changes

app/scripts/controllers/network/network-controller.js Show resolved Hide resolved

Gudahtt approved these changes Dec 19, 2022

View reviewed changes

Merge branch 'develop' into fix/sub-filter-into-network

911667c

brad-decker approved these changes Dec 19, 2022

View reviewed changes

jpuri approved these changes Dec 20, 2022

View reviewed changes

mcmire merged commit 6f6984f into develop Dec 20, 2022

mcmire deleted the fix/sub-filter-into-network branch December 20, 2022 17:28

github-actions bot locked and limited conversation to collaborators Dec 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moved subscribe and filter into network controller #16693

Moved subscribe and filter into network controller #16693

BelfordZ commented Nov 25, 2022 •

edited by mcmire

Loading

github-actions bot commented Nov 25, 2022

metamaskbot commented Nov 25, 2022

metamaskbot commented Nov 25, 2022

Gudahtt left a comment

Gudahtt commented Dec 8, 2022

mcmire left a comment

Gudahtt commented Dec 9, 2022

mcmire commented Dec 9, 2022

Gudahtt commented Dec 9, 2022 •

edited

Loading

brad-decker commented Dec 13, 2022

mcmire commented Dec 15, 2022

mcmire commented Dec 16, 2022

mcmire left a comment

Gudahtt left a comment

Gudahtt commented Dec 16, 2022

metamaskbot commented Dec 17, 2022

Gudahtt left a comment

metamaskbot commented Dec 19, 2022

Moved subscribe and filter into network controller #16693

Moved subscribe and filter into network controller #16693

Conversation

BelfordZ commented Nov 25, 2022 • edited by mcmire Loading

github-actions bot commented Nov 25, 2022

metamaskbot commented Nov 25, 2022

metamaskbot commented Nov 25, 2022

Gudahtt left a comment

Choose a reason for hiding this comment

Gudahtt commented Dec 8, 2022

mcmire left a comment

Choose a reason for hiding this comment

Gudahtt commented Dec 9, 2022

mcmire commented Dec 9, 2022

Gudahtt commented Dec 9, 2022 • edited Loading

brad-decker commented Dec 13, 2022

mcmire commented Dec 15, 2022

mcmire commented Dec 16, 2022

mcmire left a comment

Choose a reason for hiding this comment

Gudahtt left a comment

Choose a reason for hiding this comment

Gudahtt commented Dec 16, 2022

metamaskbot commented Dec 17, 2022

Gudahtt left a comment

Choose a reason for hiding this comment

metamaskbot commented Dec 19, 2022

BelfordZ commented Nov 25, 2022 •

edited by mcmire

Loading

Gudahtt commented Dec 9, 2022 •

edited

Loading