From 917290e7641882f605c0d4d7fe9fdc464aa54d05 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Wed, 15 Mar 2023 12:16:31 -0230 Subject: [PATCH] Revert "Moved subscribe and filter into network controller (#16693)" (#18129) * Revert "Moved subscribe and filter into network controller (#16693)" This reverts commit 6f6984fa58c4797e809387b3b3d0b28e72b252bf. 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`. --- .../controllers/network/network-controller.js | 24 +----- .../controllers/permissions/specifications.js | 2 - app/scripts/metamask-controller.js | 25 +++--- test/e2e/tests/eth-subscribe.spec.js | 78 +++++++++++++++++++ 4 files changed, 95 insertions(+), 34 deletions(-) create mode 100644 test/e2e/tests/eth-subscribe.spec.js diff --git a/app/scripts/controllers/network/network-controller.js b/app/scripts/controllers/network/network-controller.js index 6cf66f5ddb2e..6f0e08cbe43c 100644 --- a/app/scripts/controllers/network/network-controller.js +++ b/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 }); } diff --git a/app/scripts/controllers/permissions/specifications.js b/app/scripts/controllers/permissions/specifications.js index 3779a904a232..4072398d12b9 100644 --- a/app/scripts/controllers/permissions/specifications.js +++ b/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', diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 9c496fd8ba12..61f52a635363 100644 --- a/app/scripts/metamask-controller.js +++ b/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( diff --git a/test/e2e/tests/eth-subscribe.spec.js b/test/e2e/tests/eth-subscribe.spec.js new file mode 100644 index 000000000000..d7713516459c --- /dev/null +++ b/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"]', + ); + }, + ); + }); +});