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

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

This reverts commit 6f6984fa58. 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

* Add e2e test

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

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

@@ -2,18 +2,13 @@ import { strict as assert } from 'assert';
 import EventEmitter from 'events';
 import { ComposedStore, ObservableStore } from '@metamask/obs-store';
 import { JsonRpcEngine } from 'json-rpc-engine';
-import {
-  providerFromEngine,
-  providerFromMiddleware,
-} from '@metamask/eth-json-rpc-middleware';
+import { providerFromEngine } from '@metamask/eth-json-rpc-middleware';
 import log from 'loglevel';
 import {
   createSwappableProxy,
   createEventEmitterProxy,
 } from 'swappable-obj-proxy';
 import EthQuery from 'eth-query';
-import createFilterMiddleware from 'eth-json-rpc-filters';
-import createSubscriptionManager from 'eth-json-rpc-filters/subscriptionManager';
 import { v4 as random } from 'uuid';
 import {
   INFURA_PROVIDER_TYPES,
@@ -460,26 +455,9 @@ export default class NetworkController extends EventEmitter {
   }
 
   _setNetworkClient({ networkMiddleware, blockTracker }) {
-    const networkProvider = providerFromMiddleware(networkMiddleware);
-    const filterMiddleware = createFilterMiddleware({
-      provider: networkProvider,
-      blockTracker,
-    });
-    const subscriptionManager = createSubscriptionManager({
-      provider: networkProvider,
-      blockTracker,
-    });
-
     const engine = new JsonRpcEngine();
-    subscriptionManager.events.on('notification', (message) =>
-      engine.emit('notification', message),
-    );
-    engine.push(filterMiddleware);
-    engine.push(subscriptionManager.middleware);
     engine.push(networkMiddleware);
-
     const provider = providerFromEngine(engine);
-
     this._setProviderAndBlockTracker({ provider, blockTracker });
   }
 

app/scripts/controllers/permissions/specifications.js

@@ -260,12 +260,10 @@ export const unrestrictedMethods = Object.freeze([
   'eth_signTypedData_v1',
   'eth_signTypedData_v3',
   'eth_signTypedData_v4',
-  'eth_subscribe',
   'eth_submitHashrate',
   'eth_submitWork',
   'eth_syncing',
   'eth_uninstallFilter',
-  'eth_unsubscribe',
   'metamask_getProviderState',
   'metamask_watchAsset',
   'net_listening',

app/scripts/metamask-controller.js

@@ -10,6 +10,8 @@ import {
   KeyringController,
   keyringBuilderFactory,
 } from '@metamask/eth-keyring-controller';
+import createFilterMiddleware from 'eth-json-rpc-filters';
+import createSubscriptionManager from 'eth-json-rpc-filters/subscriptionManager';
 import {
   errorCodes as rpcErrorCodes,
   EthereumRpcError,
@@ -3898,19 +3900,21 @@ export default class MetamaskController extends EventEmitter {
    * @param {tabId} [options.tabId] - The tab ID of the sender - if the sender is within a tab
    */
   setupProviderEngine({ origin, subjectType, sender, tabId }) {
-    const { provider } = this;
-
     // setup json rpc engine stack
     const engine = new JsonRpcEngine();
+    const { blockTracker, provider } = this;
 
-    // forward notifications from network provider
-    provider.on('data', (error, message) => {
-      if (error) {
-        // This should never happen, this error parameter is never set
-        throw error;
-      }
-      engine.emit('notification', message);
+    // create filter polyfill middleware
+    const filterMiddleware = createFilterMiddleware({ provider, blockTracker });
+
+    // create subscription polyfill middleware
+    const subscriptionManager = createSubscriptionManager({
+      provider,
+      blockTracker,
     });
+    subscriptionManager.events.on('notification', (message) =>
+      engine.emit('notification', message),
+    );
 
     if (isManifestV3) {
       engine.push(createDupeReqFilterMiddleware());
@@ -4063,6 +4067,9 @@ export default class MetamaskController extends EventEmitter {
     );
     ///: END:ONLY_INCLUDE_IN
 
+    // filter and subscription polyfills
+    engine.push(filterMiddleware);
+    engine.push(subscriptionManager.middleware);
     if (subjectType !== SubjectType.Internal) {
       // permissions
       engine.push(

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

@@ -0,0 +1,78 @@
+const { convertToHexValue, withFixtures } = require('../helpers');
+const FixtureBuilder = require('../fixture-builder');
+
+describe('eth_subscribe', function () {
+  const ganacheOptions = {
+    accounts: [
+      {
+        secretKey:
+          '0x7C9529A67102755B7E6102D6D950AC5D5863C98713805CEC576B945B15B71EAC',
+        balance: convertToHexValue(25000000000000000000),
+      },
+    ],
+  };
+
+  it('only broadcasts subscription notifications on the page that registered the subscription', async function () {
+    await withFixtures(
+      {
+        dapp: true,
+        fixtures: new FixtureBuilder()
+          .withPermissionControllerConnectedToTestDapp()
+          .build(),
+        ganacheOptions,
+        dappOptions: { numberOfDapps: 2 },
+        title: this.test.title,
+      },
+      async ({ driver }) => {
+        await driver.navigate();
+        await driver.fill('#password', 'correct horse battery staple');
+        await driver.press('#password', driver.Key.ENTER);
+
+        await driver.openNewPage('http://127.0.0.1:8080/');
+
+        const setupSubscriptionListener = `
+          const responseContainer = document.createElement('div');
+          responseContainer.setAttribute('id', 'eth-subscribe-responses');
+
+          const body = window.document.getElementsByTagName('body')[0];
+          body.appendChild(responseContainer);
+
+          window.ethereum.addListener('message', (message) => {
+            if (message.type === 'eth_subscription') {
+              const response = document.createElement('p');
+              response.setAttribute('data-testid', 'eth-subscribe-response');
+              response.append(JSON.stringify(message.data.result));
+
+              const responses = window.document.getElementById('eth-subscribe-responses');
+              responses.appendChild(response)
+            }
+          });
+        `;
+
+        await driver.executeScript(setupSubscriptionListener);
+        // A `newHeads` subscription will emit a notification for each new block
+        // See here for more information: https://docs.infura.io/infura/networks/ethereum/json-rpc-methods/subscription-methods/eth_subscribe
+        await driver.executeScript(`
+          window.ethereum.request({
+            method: 'eth_subscribe',
+            params: ['newHeads']
+          });
+        `);
+
+        // Verify that the new block is seen on the first dapp
+        await driver.findElement('[data-testid="eth-subscribe-response"]');
+
+        // Switch to the second dapp
+        await driver.openNewPage('http://127.0.0.1:8081/');
+
+        // Setup the same subscription listener as on the first dapp, but without registering a new subscription
+        await driver.executeScript(setupSubscriptionListener);
+
+        // Verify that the new block is not seen on the second dapp
+        await driver.assertElementNotPresent(
+          '[data-testid="eth-subscribe-response"]',
+        );
+      },
+    );
+  });
+});