NetworkController: Use the same middleware stack as the extension #1116

BelfordZ · 2023-02-28T21:58:26Z

Description

The network controller in this repo exposes a JSON-RPC provider object built via the web3-provider-engine package. However, we have not supported this package for a long time, and the network controller in the extension uses a more modern provider built from json-rpc-engine and eth-json-rpc-middleware. In order to unify the two versions of the network controllers, we need to replace the middleware stack in this version with the one in the extension.

Changes

BREAKING: [@metamask/network-controller] Update NetworkController to use a simpler middleware stack derived from pieces of eth-json-rpc-middleware instead of web3-provider-engine. Consequences of this replacement are detailed in items below.
BREAKING: [@metamask/network-controller] A call to eth_chainId through a newly initialized provider where the type in the provider config state is "rpc" will now return the chainId in the provider config rather than the chain ID returned by the network.
BREAKING: [@metamask/network-controller] A call to eth_chainId through a newly initialized provider where the type in the provider config state is an Infura network will now return a hard-coded network ID matching the chain ID rather than the chain ID returned by the network.
BREAKING: [@metamask/network-controller] A call to eth_chainId through the provider created from switching the network via setActiveNetwork will now return the chainId in the network configuration object rather than the chain ID returned by the network.
BREAKING: [@metamask/network-controller] A call to net_version through the provider created from switching the network via setProviderType will now return a hard-coded network ID matching the chain ID rather than the network ID returned by the network.
BREAKING: [@metamask/network-controller] Previously, calls to RPC methods that took an object as its first parameter (e.g. eth_call) were intercepted such that unknown properties were removed from the object and any hex strings present in the remaining properties were normalized. This no longer happens and these methods will pass through to the network unchanged.
- Consumers who wish to use these methods are now responsible for passing an object that contains properties and values that the network is able to understand.
BREAKING: [@metamask/network-controller] A call to eth_getBalance, eth_getBlockByNumber, eth_getCode, eth_getTransactionCount, or eth_call will now be intercepted such that a block tag parameter of "latest" will be replaced with the latest known block number before being passed to the network.
- This substitution makes it more likely that we can return a cached response to the request.
BREAKING: [@metamask/network-controller] Previously, a call to eth_getTransactionCount was intercepted such that when the block tag was "pending", it would treat the count as the transaction nonce and cache it, using it for subsequent calls for the same address. This nonce would then be updated upon each call to eth_sendRawTransaction based on the nonce of the transaction being sent. The whole nonce cache was also cleared upon a call to evm_revert. This no longer happens, and these RPC methods will be passed to the network unchanged.
- Consumers may use the nonceTracker that @metamask/transaction-controller exposes in order to obtain a unique nonce.
BREAKING: [@metamask/network-controller] A call to web3_clientVersion is no longer intercepted to return a static result of "ProviderEngine/v<version>/javascript", but is passed through to the network.
BREAKING: [@metamask/network-controller] A call to net_listening is no longer intercepted to return a static result of true, but is passed through to the network.
BREAKING: [@metamask/network-controller] A call to eth_hashrate is no longer intercepted to return a static result of "0x00", but is passed through to the network.
BREAKING: [@metamask/network-controller] A call to eth_mining is no longer intercepted to return a static result of false, but is passed through to the network.
BREAKING: [@metamask/network-controller] Previously, eth_subscribe and eth_unsubscribe would never hit the network; instead, the behavior was polyfilled by polling the network for new blocks. Additionally, the newPendingTransactions parameter for eth_subscribe was unsupported. This polyfill is no longer present, and eth_subscribe and eth_unsubscribe are passed through to the network unchanged.
- Consumers wishing to recreate the prior behavior and use the block tracker to power subscriptions may employ the middleware provided by the eth-json-rpc-filters package.
BREAKING: [@metamask/network-controller] Previously, eth_newFilter, eth_newBlockFilter, eth_newPendingTransactionFilter, eth_uninstallFilter, eth_getFilterChanges, and eth_getFilterLogs would never hit the network; instead, the behavior was polyfilled by polling the network for new blocks and recording updates for registered filters. This polyfill is no longer present, and these RPC methods are passed through to the network unchanged.
- Consumers wishing to recreate the prior behavior and use the block tracker to power filters may employ the middleware provided by the eth-json-rpc-filters package.
BREAKING: [@metamask/network-controller] Interfacing with a network that exposes a websocket is no longer supported.
CHANGED: [@metamask/network-controller] Bump dependency eth-json-rpc-infura (now @metamask/eth-json-rpc-infura) from ^7.0.0 to ^8.0.0.
ADDED: [@metamask/network-controller] Add dependency eth-json-rpc-middleware ^11.0.0
ADDED: [@metamask/network-controller] Add dependency eth-json-rpc-provider ^1.0.0
ADDED: [@metamask/network-controller] Add dependency eth-block-tracker ^7.0.0
ADDED: [@metamask/network-controller] Add dependency json-rpc-engine ^6.1.0

References

Fixes #970.

Checklist

I've updated the test suite for new or updated code as appropriate
I've updated documentation for new or updated code as appropriate (note: this will usually be JSDoc)
I've highlighted breaking changes using the "BREAKING" category above as appropriate

mcmire

Did a first pass on this PR. Looks pretty straightforward in terms of implementation, I think all of the comments I had related to cleanup.

packages/network-controller/src/clients/createInfuraClient.ts

packages/network-controller/src/clients/createJsonRpcClient.ts

packages/network-controller/src/NetworkController.ts

packages/network-controller/tests/NetworkController.test.ts

packages/network-controller/tests/fake-provider-engine.ts

packages/network-controller/tests/provider-api-tests/block-hash-in-response.ts

packages/network-controller/tests/provider-api-tests/block-param.ts

socket-security · 2023-04-03T21:51:33Z

New dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No new dependency issues detected in pull request

Bot Commands

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

Ignoring: @metamask/[email protected], @metamask/[email protected], [email protected]

Pull request alert summary

Issue	Status
Critical CVE	✅ 0 issues
CVE	✅ 0 issues
Mild CVE	✅ 0 issues
Install scripts	✅ 0 issues
Native code	✅ 0 issues
Bin script confusion	✅ 0 issues
Bin script shell injection	✅ 0 issues
Unresolved require	✅ 0 issues
Invalid package.json	✅ 0 issues
HTTP dependency	✅ 0 issues
Git dependency	✅ 0 issues
GitHub dependency	✅ 0 issues
No bug tracker	✅ 0 issues
No contributors or author data	✅ 0 issues
No README	✅ 0 issues
Deprecated	✅ 0 issues
Unmaintained	✅ 0 issues
Potential typo squat	✅ 0 issues
Known Malware	✅ 0 issues
Telemetry	✅ 0 issues
Protestware/Troll package	✅ 0 issues
AI detected security risk	✅ 0 issues
AI warning	✅ 0 issues

📊 Modified Dependency Overview:

➕ Added Package	Capability Access	`+/-` Transitive Count	Publisher
@metamask/[email protected]	None	`+1`	gudahtt
@metamask/[email protected]	network	`+1`	metamaskbot

Gudahtt

Glad to see the diff here is reasonably small! I didn't review the tests in detail yet because I'd rather do that after we document the functional differences we've found, so that I can cross-reference that with the test changes.

Could you update the PR description as well when you get the chance?

packages/network-controller/src/NetworkController.ts

packages/network-controller/src/clients/createInfuraClient.ts

mcmire

This looks nice! I too am surprised by how small this PR is, but I guess this speaks to how many changes we've made so far 😅 ~~I did a first pass here~~ oh I guess this is a second pass lol and had some suggestions. I'll do a closer comparison of the JavaScript and TypeScript next.

Echoing Mark's request from earlier — would you mind updating the PR description to explain what this PR does so we have something to go off of when writing the changelog (and also for posterity in general)?

packages/network-controller/src/NetworkController.ts

packages/network-controller/src/create-network-client.ts

mcmire

Thanks for updating the PR description. Did another pass, this time in a little more detail. Tests look great! Mostly minor stuff.

packages/controller-utils/src/util.ts

packages/network-controller/tests/fake-provider-engine.ts

packages/network-controller/tests/NetworkController.test.ts

packages/network-controller/tests/provider-api-tests/no-block-param.ts

packages/network-controller/src/create-network-client.ts

mcmire

LGTM!

BelfordZ · 2023-04-11T18:03:12Z

@mcmire just rebased

packages/network-controller/tests/provider-api-tests/shared-tests.ts

packages/controller-utils/src/util.ts

packages/network-controller/tests/provider-api-tests/shared-tests.ts

Gudahtt · 2023-04-13T19:29:45Z

packages/network-controller/tests/provider-api-tests/shared-tests.ts

                await makeRpcCall(request);
-                await waitForNextBlockTracker(blockTracker, clock);


Do we know why this step is no longer needed?

@BelfordZ Do you remember? I feel like it might have something to do with https://github.com/MetaMask/web3-provider-engine/blob/cf612f898002833c36730d23972fe4c4dd483c76/index.js#L50, but I'm not sure.

sry for the delay.

We no longer need this because there was a subtle difference in the number of setTimeouts that occur between extension's network controller & the core one. Specifically the use of 'stoplight' which uses setTimeout if the lock is currently set. It only affected some tests based on whether or not the request would be made while the lock was held by another. Since we no longer are using this provider, this extra setTimeout goes away.

Gudahtt · 2023-05-03T18:19:29Z

packages/network-controller/tests/provider-api-tests/no-block-param.ts

@@ -921,7 +961,9 @@ export function testsForRpcMethodAssumingNoBlockParam(
          },
        );

-        expect(result).toBeUndefined();
+        await expect(promiseForResult).rejects.toThrow(


Should the test description here be updated to say "throws" instead of "returns"? Likewise for the other test updates in this module that now throw when they hadn't previously.

Hmm, interesting. I guess it depends on how people are likely to think about methods that return promises. When used with await, they act like synchronous methods, so we could say that they "return X" when they resolve and they "throw Y" when they reject. Our guideline is that await should be preferred, of course, so it makes sense to think about it like that. More accurately, though we could say that they "return a promise that resolves with X" or "return a promise that rejects with Y". Do you think it's worth it to be accurate or does it not matter?

Hmm, I tend to prefer the shorter form just for brevity but I don't have a strong preference either way. Either way would be a lot more clear than the current description

The `setFakeProvider` test helper used for the network controller unit tests had a return value that was unused, and an unnecessary waiting step. These have both been removed. This reduces the scope of #1116

Gudahtt

Looks good! I have finished reviewing this, and can't see any more clear opportunities to break this into smaller pieces.

packages/network-controller/tests/NetworkController.test.ts

Co-authored-by: Mark Stacey <[email protected]>

…-rebased * origin/main: Simplify the `setFakeProvider` test helper (#1317)

Gudahtt

LGTM! The PR description looks complete as well.

The one pending comment about the no-block-param test descriptions can wait for a later PR.

danfinlay · 2023-05-04T04:57:07Z

packages/network-controller/package.json

-    "uuid": "^8.3.2",
-    "web3-provider-engine": "^16.0.5"
+    "json-rpc-engine": "^6.1.0",
+    "uuid": "^8.3.2"


🎉🎉🎉🎉🎉🎉🎉

Our test helper modules have been covered by ESLint as though they were production code. This resulted in misleading coverage statistics and various erronous lint errors (which we've handled by disabling rules in affected files). The ESLint configuration has been updated to consider any modules under a `tests` directory to be test helpers. This helps reduce the scope of #1116

All unnecessary ESLint ignore comments have been removed from the network controller package. Most of these were added before we had decent types setup for the network client tests. The `mockRpcCall` function was converted from an arrow function to a function declaration to address lint errors, so the description was copied over from the extension. Relates to #1116

When constructing a non-Infura network client, the network `nickname` and `ticker` configuration properties were passed on down to `web3-provider-engine`. However these were entirely unused. There are no references to these in the `web3-provider-engine` package. Relates to #1116

An unnecessary mock has been removed from a helper function used by network client tests. The `getEIP1559Compatibility` function is only called as part of `lookupNetwork` (which is also mocked) as of #1236, so this has been redundant since then. Relates to #1116

The types used for the network client test helpers have been made more specific; the network type passed in is now required at a type-level to be an Infura network. This was a type-only change because we were already only passing in Infura neworks for this option. This was done to make a later refactor PR simpler. Relates to #1116

The network controller unit tests have been updated to avoid importing a constants file from another package using a relative path import. As a general rule we should be using package imports whenever referencing modules in other packages. This reduces the scope of #1116

JSDoc comments have been added to functions used for the network client tests. These were copied from the extension. This reduces the scope of #1116

Validation has been added to internal methods responsible for constructing a provider (upon initialization or after switching networks). They ensure that a custom provider has a chain ID and RPC URL set. This reduces the scope of #1116

The `setFakeProvider` test helper used for the network controller unit tests had a return value that was unused, and an unnecessary waiting step. These have both been removed. This reduces the scope of #1116

) The network controller in this repo exposes a JSON-RPC provider object built via the `web3-provider-engine` package. However, we have not supported this package for a long time, and the network controller in the extension uses a more modern provider built from `json-rpc-engine` and `eth-json-rpc-middleware`. In order to unify the two versions of the network controllers, we need to replace the middleware stack in this version with the one in the extension. Co-authored-by: Mark Stacey <[email protected]> Co-authored-by: Elliot Winkler <[email protected]> Co-authored-by: legobeat <[email protected]>