From d5ee4fb88222953d953677448e544198670678b5 Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Thu, 25 Jul 2024 10:13:14 -0230 Subject: [PATCH 01/15] Ensure that the outstream end/finished handler in setupControllerConnection in metamask-controller.js is called when it should be --- app/scripts/metamask-controller.js | 54 ++++- app/scripts/metamask-controller.test.js | 291 ++++++++++++++++++++++++ 2 files changed, 335 insertions(+), 10 deletions(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 44dc9a67fbeb..cdb9f77df6df 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -1,5 +1,5 @@ import EventEmitter from 'events'; -import { pipeline } from 'readable-stream'; +import readableStream, { pipeline } from 'readable-stream'; import { AssetsContractController, CurrencyRateController, @@ -19,6 +19,7 @@ import { JsonRpcEngine } from 'json-rpc-engine'; import { createEngineStream } from 'json-rpc-middleware-stream'; import { providerAsMiddleware } from '@metamask/eth-json-rpc-middleware'; import { debounce, throttle, memoize, wrap } from 'lodash'; +import { v4 as uuid } from 'uuid'; import { KeyringController, keyringBuilderFactory, @@ -374,6 +375,7 @@ export default class MetamaskController extends EventEmitter { // this keeps track of how many "controllerStream" connections are open // the only thing that uses controller connections are open metamask UI instances this.activeControllerConnections = 0; + this.finishedControllerStreamIds = new Set(); this.offscreenPromise = opts.offscreenPromise ?? Promise.resolve(); @@ -1537,6 +1539,7 @@ export default class MetamaskController extends EventEmitter { this.triggerNetworkrequests(); } else { this.stopNetworkRequests(); + this.finishedControllerStreamIds.clear(); } }); @@ -4976,7 +4979,9 @@ export default class MetamaskController extends EventEmitter { // setup multiplexing const mux = setupMultiplex(connectionStream); // connect features - this.setupControllerConnection(mux.createStream('controller')); + const newControllerStream = mux.createStream('controller'); + newControllerStream.metamaskStreamId = uuid(); + this.setupControllerConnection(newControllerStream); this.setupProviderConnectionEip1193( mux.createStream('provider'), sender, @@ -5073,14 +5078,43 @@ export default class MetamaskController extends EventEmitter { this.once('startUISync', startUISync); } - outStream.on('end', () => { - this.activeControllerConnections -= 1; - this.emit( - 'controllerConnectionChanged', - this.activeControllerConnections, - ); - this.removeListener('update', handleUpdate); - }); + const outstreamEndHandler = () => { + if ( + !this.finishedControllerStreamIds.has(outStream.metamaskStreamId) && + this.activeControllerConnections > 0 + ) { + this.finishedControllerStreamIds.add(outStream.metamaskStreamId); + this.activeControllerConnections -= 1; + + this.emit( + 'controllerConnectionChanged', + this.activeControllerConnections, + ); + this.removeListener('update', handleUpdate); + } + }; + + // The presence of both of the below handlers may be redundant. + // After upgrading metamask/object-multiples to v2.0.0, which included + // an upgrade of readable-streams from v2 to v3, we saw that the + // `outStream.on('end'` handler was almost never being called. This seems to + // related to how v3 handles errors vs how v2 handles errors; there + // are "premature close" errors in both cases, although in the case + // of v2 they don't prevent `outStream.on('end'` from being called. + // At the time that this comment was committed, it was known that we + // need to investigate and resolve the underlying error, however, + // for expediency, we are not addressing them at this time. Instead, we + // can observe that `readableStream.finished` preserves the same + // functionality as we had when we relied on readable-stream v2. Meanwhile, + // the `outStream.on('end'` handler was observed to have been called at least once. + // As we do not yet fully understand the conditions under which both of these + // are being called, we include both handlers at the risk of redundancy. Logic has + // been added to the handler to ensure that calling it more than once does + // not have any affect. + + readableStream.finished(outStream, outstreamEndHandler); + + outStream.on('end', outstreamEndHandler); } /** diff --git a/app/scripts/metamask-controller.test.js b/app/scripts/metamask-controller.test.js index 8f5e37123a9e..d42beae3cae1 100644 --- a/app/scripts/metamask-controller.test.js +++ b/app/scripts/metamask-controller.test.js @@ -27,6 +27,7 @@ import { RatesController, TokenListController, } from '@metamask/assets-controllers'; +import ObjectMultiplex from '@metamask/object-multiplex'; import { TrezorKeyring } from '@metamask/eth-trezor-keyring'; import { LedgerKeyring } from '@metamask/eth-ledger-bridge-keyring'; import { NETWORK_TYPES } from '../../shared/constants/network'; @@ -1406,9 +1407,299 @@ describe('MetaMaskController', () => { }); metamaskController.setupTrustedCommunication(streamTest, messageSender); + await promise; streamTest.end(); }); + + it('uses a new multiplex to set up a connection with a "controller" stream that has a unique metamaskStreamId', () => { + jest.spyOn(metamaskController, 'setupControllerConnection'); + + const streamTest = createThroughStream((chunk, _, cb) => { + cb(chunk); + }); + + metamaskController.setupTrustedCommunication(streamTest, {}); + + expect(metamaskController.setupControllerConnection).toHaveBeenCalled(); + expect( + metamaskController.setupControllerConnection, + ).toHaveBeenCalledWith( + expect.objectContaining({ + metamaskStreamId: expect.stringMatching( + /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/u, + ), + _name: 'controller', + _parent: expect.any(ObjectMultiplex), + }), + ); + }); + + const createTestStream = () => { + const { + promise: onFinishedCallbackPromise, + resolve: onFinishedCallbackResolve, + } = deferredPromise(); + const { promise: onStreamEndPromise, resolve: onStreamEndResolve } = + deferredPromise(); + const testStream = createThroughStream((chunk, _, cb) => { + expect(chunk.name).toStrictEqual('controller'); + onStreamEndResolve(); + cb(); + }); + + return { + onFinishedCallbackPromise, + onStreamEndPromise, + onFinishedCallbackResolve, + testStream, + }; + }; + + it('sets up a controller connection which emits a controllerConnectionChanged event when the controller connection is created and ended, and activeControllerConnections are updated accordingly', async () => { + const mockControllerConnectionChangedHandler = jest.fn(); + + const { + onStreamEndPromise, + onFinishedCallbackPromise, + onFinishedCallbackResolve, + testStream, + } = createTestStream(); + + metamaskController.on( + 'controllerConnectionChanged', + (activeControllerConnections) => { + mockControllerConnectionChangedHandler(activeControllerConnections); + if ( + mockControllerConnectionChangedHandler.mock.calls.length === 2 + ) { + onFinishedCallbackResolve(); + } + }, + ); + + expect(metamaskController.activeControllerConnections).toBe(0); + + metamaskController.setupTrustedCommunication(testStream, {}); + + expect(mockControllerConnectionChangedHandler).toHaveBeenCalledTimes(1); + expect(mockControllerConnectionChangedHandler).toHaveBeenLastCalledWith( + 1, + ); + + expect(metamaskController.activeControllerConnections).toBe(1); + + await onStreamEndPromise; + testStream.end(); + + await onFinishedCallbackPromise; + + expect(metamaskController.activeControllerConnections).toBe(0); + expect(mockControllerConnectionChangedHandler).toHaveBeenCalledTimes(2); + expect(mockControllerConnectionChangedHandler).toHaveBeenLastCalledWith( + 0, + ); + }); + + it('can be called multiple times to set up multiple controller connections, which can be ended independently', async () => { + const mockControllerConnectionChangedHandler = jest.fn(); + + const testStreams = [ + createTestStream(), + createTestStream(), + createTestStream(), + createTestStream(), + createTestStream(), + ]; + metamaskController.on( + 'controllerConnectionChanged', + (activeControllerConnections) => { + const initialChangeHandlerCallCount = + mockControllerConnectionChangedHandler.mock.calls.length; + mockControllerConnectionChangedHandler(activeControllerConnections); + + if ( + initialChangeHandlerCallCount === 5 && + activeControllerConnections === 4 + ) { + testStreams[1].onFinishedCallbackResolve(); + } + if ( + initialChangeHandlerCallCount === 7 && + activeControllerConnections === 2 + ) { + testStreams[3].onFinishedCallbackResolve(); + testStreams[4].onFinishedCallbackResolve(); + } + if ( + initialChangeHandlerCallCount === 9 && + activeControllerConnections === 0 + ) { + testStreams[2].onFinishedCallbackResolve(); + testStreams[0].onFinishedCallbackResolve(); + } + }, + ); + + metamaskController.setupTrustedCommunication( + testStreams[0].testStream, + {}, + ); + metamaskController.setupTrustedCommunication( + testStreams[1].testStream, + {}, + ); + metamaskController.setupTrustedCommunication( + testStreams[2].testStream, + {}, + ); + metamaskController.setupTrustedCommunication( + testStreams[3].testStream, + {}, + ); + metamaskController.setupTrustedCommunication( + testStreams[4].testStream, + {}, + ); + + expect(metamaskController.activeControllerConnections).toBe(5); + + await testStreams[1].promise; + testStreams[1].testStream.end(); + + await testStreams[1].onFinishedCallbackPromise; + + expect(metamaskController.activeControllerConnections).toBe(4); + + await testStreams[3].promise; + testStreams[3].testStream.end(); + + await testStreams[4].promise; + testStreams[4].testStream.end(); + + await testStreams[3].onFinishedCallbackPromise; + await testStreams[4].onFinishedCallbackPromise; + + expect(metamaskController.activeControllerConnections).toBe(2); + + await testStreams[2].promise; + testStreams[2].testStream.end(); + + await testStreams[0].promise; + testStreams[0].testStream.end(); + + await testStreams[2].onFinishedCallbackPromise; + await testStreams[0].onFinishedCallbackPromise; + + expect(metamaskController.activeControllerConnections).toBe(0); + }); + + it('adds controller connections, which when ended, add their stream ids to this.finishedControllerStreamIds', async () => { + jest + .spyOn(metamaskController, 'triggerNetworkrequests') + .mockImplementation(); + jest + .spyOn(metamaskController.onboardingController.store, 'getState') + .mockReturnValue({ completedOnboarding: true }); + const mockControllerConnectionChangedHandler = jest.fn(); + + const testStreams = [ + createTestStream(), + createTestStream(2), + createTestStream(3), + createTestStream(4), + createTestStream(5), + ]; + metamaskController.on( + 'controllerConnectionChanged', + (activeControllerConnections) => { + const initialChangeHandlerCallCount = + mockControllerConnectionChangedHandler.mock.calls.length; + mockControllerConnectionChangedHandler(activeControllerConnections); + if ( + initialChangeHandlerCallCount === 8 && + activeControllerConnections === 1 + ) { + testStreams[1].onFinishedCallbackResolve(); + testStreams[3].onFinishedCallbackResolve(); + testStreams[4].onFinishedCallbackResolve(); + testStreams[2].onFinishedCallbackResolve(); + } + if ( + initialChangeHandlerCallCount === 9 && + activeControllerConnections === 0 + ) { + testStreams[0].onFinishedCallbackResolve(); + } + }, + ); + + metamaskController.setupTrustedCommunication( + testStreams[0].testStream, + {}, + ); + metamaskController.setupTrustedCommunication( + testStreams[1].testStream, + {}, + ); + metamaskController.setupTrustedCommunication( + testStreams[2].testStream, + {}, + ); + metamaskController.setupTrustedCommunication( + testStreams[3].testStream, + {}, + ); + metamaskController.setupTrustedCommunication( + testStreams[4].testStream, + {}, + ); + + expect( + Array.from(metamaskController.finishedControllerStreamIds), + ).toStrictEqual([]); + + await testStreams[1].promise; + testStreams[1].testStream.end(); + await testStreams[3].promise; + testStreams[3].testStream.end(); + + await testStreams[4].promise; + testStreams[4].testStream.end(); + await testStreams[2].promise; + testStreams[2].testStream.end(); + await testStreams[1].onFinishedCallbackPromise; + await testStreams[3].onFinishedCallbackPromise; + await testStreams[4].onFinishedCallbackPromise; + await testStreams[2].onFinishedCallbackPromise; + expect( + Array.from(metamaskController.finishedControllerStreamIds), + ).toStrictEqual( + expect.arrayContaining([ + expect.stringMatching( + /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/u, + ), + expect.stringMatching( + /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/u, + ), + expect.stringMatching( + /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/u, + ), + expect.stringMatching( + /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/u, + ), + ]), + ); + + await testStreams[0].promise; + testStreams[0].testStream.end(); + + await testStreams[0].onFinishedCallbackPromise; + + expect( + Array.from(metamaskController.finishedControllerStreamIds), + ).toStrictEqual([]); + }); }); describe('#markPasswordForgotten', () => { From 32ea2ffad27bb40bf59d4e7ff555e2f96e2e494d Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Thu, 25 Jul 2024 15:01:29 -0230 Subject: [PATCH 02/15] Add e2e test to verify that there are no eth_call and eth_getBalance requests to infura after the UI has closed --- privacy-snapshot.json | 20 +-- .../account-tracker-api-usage.spec.ts | 170 ++++++++++++++++++ test/e2e/webdriver/chrome.js | 1 + 3 files changed, 181 insertions(+), 10 deletions(-) create mode 100644 test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts diff --git a/privacy-snapshot.json b/privacy-snapshot.json index a855a5a3f126..1143db9792f4 100644 --- a/privacy-snapshot.json +++ b/privacy-snapshot.json @@ -6,6 +6,7 @@ "api.web3modal.com", "app.ens.domains", "arbitrum-mainnet.infura.io", + "authentication.api.cx.metamask.io", "bafkreifvhjdf6ve4jfv6qytqtux5nd4nwnelioeiqx5x2ez5yrgrzk7ypi.ipfs.dweb.link", "bafybeidxfmwycgzcp4v2togflpqh2gnibuexjy4m4qqwxp7nh3jx5zlh4y.ipfs.dweb.link", "cdn.segment.com", @@ -23,18 +24,22 @@ "gas.api.cx.metamask.io", "github.com", "goerli.infura.io", + "lattice.gridplus.io", + "localhost:12345", "localhost:8000", "localhost:8545", + "localhost:9999", "mainnet.infura.io", "metamask.eth", "metamask.github.io", "min-api.cryptocompare.com", "nft.api.cx.metamask.io", + "oidc.api.cx.metamask.io", + "on-ramp-content.api.cx.metamask.io", + "on-ramp-content.uat-api.cx.metamask.io", "phishing-detection.api.cx.metamask.io", "portfolio.metamask.io", "price.api.cx.metamask.io", - "on-ramp-content.api.cx.metamask.io", - "on-ramp-content.uat-api.cx.metamask.io", "proxy.api.cx.metamask.io", "raw.githubusercontent.com", "registry.npmjs.org", @@ -49,12 +54,7 @@ "token.api.cx.metamask.io", "tokens.api.cx.metamask.io", "tx-sentinel-ethereum-mainnet.api.cx.metamask.io", - "unresponsive-rpc.url", - "www.4byte.directory", - "lattice.gridplus.io", "unresponsive-rpc.test", - "authentication.api.cx.metamask.io", - "oidc.api.cx.metamask.io", - "price.api.cx.metamask.io", - "token.api.cx.metamask.io" -] + "unresponsive-rpc.url", + "www.4byte.directory" +] \ No newline at end of file diff --git a/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts b/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts new file mode 100644 index 000000000000..306577b0d23c --- /dev/null +++ b/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts @@ -0,0 +1,170 @@ +import { strict as assert } from 'assert'; +import { CompletedRequest } from 'mockttp'; +import { + withFixtures, + defaultGanacheOptions, + unlockWallet, +} from '../../helpers'; +import FixtureBuilder from '../../fixture-builder'; +import { Driver } from '../../webdriver/driver'; +import { Mockttp } from '../../mock-e2e'; + +async function mockInfura(mockServer: Mockttp): Promise { + const blockNumber = { value: 0 }; + + return [ + await mockServer + .forPost(/infura/u) + .withJsonBodyIncluding({ method: 'eth_blockNumber' }) + .times(50) + .thenCallback(() => { + blockNumber.value += 1; + + return { + statusCode: 200, + json: { + jsonrpc: '2.0', + id: '1111111111111111', + result: blockNumber.value.toString(16), + }, + }; + }), + await mockServer.forPost(/infura/u).thenCallback(() => { + return { + statusCode: 200, + json: { + jsonrpc: '2.0', + id: '1111111111111111', + result: '0x1', + }, + }; + }), + await mockServer.forGet(/infura/u).thenCallback(() => { + return { + statusCode: 200, + json: { + jsonrpc: '2.0', + id: '1111111111111111', + result: '0x0', + }, + }; + }), + ]; +} + +async function getAllInfuraJsonRpcRequests( + mockedEndpoint: Mockttp[], +): Promise { + const allInfuraJsonRpcRequests: CompletedRequest[] = []; + let seenRequests; + let seenProviderRequests; + + for (const m of mockedEndpoint) { + seenRequests = await m.getSeenRequests(); + seenProviderRequests = seenRequests.filter((request) => + request.url.match('infura'), + ); + + for (const r of seenProviderRequests) { + const json = await r.body.getJson(); + allInfuraJsonRpcRequests.push(json); + } + } + + return allInfuraJsonRpcRequests; +} + +describe('Account Tracker API Usage', function () { + it('should not make eth_call or eth_getBalance requests before the UI is opened and should make those requests after the UI is opened', async function () { + await withFixtures( + { + fixtures: new FixtureBuilder().withNetworkControllerOnMainnet().build(), + ganacheOptions: defaultGanacheOptions, + title: this.test.fullTitle(), + testSpecificMock: mockInfura, + }, + async ({ + driver, + mockedEndpoint, + }: { + driver: Driver; + mockedEndpoint: Mockttp; + }) => { + await driver.delay(3000); + let allInfuraJsonRpcRequests = await getAllInfuraJsonRpcRequests( + mockedEndpoint, + ); + let ethCallAndGetBalanceRequests = allInfuraJsonRpcRequests.filter( + ({ method }) => method === 'eth_getBalance' || method === 'eth_call', + ); + + assert.ok( + ethCallAndGetBalanceRequests.length === 0, + 'An eth_call or eth_getBalance request has been made to infura before opening the UI', + ); + + await unlockWallet(driver); + await driver.delay(3000); + + allInfuraJsonRpcRequests = await getAllInfuraJsonRpcRequests( + mockedEndpoint, + ); + ethCallAndGetBalanceRequests = allInfuraJsonRpcRequests.filter( + ({ method }) => method === 'eth_getBalance' || method === 'eth_call', + ); + + assert.ok( + ethCallAndGetBalanceRequests.length > 0, + 'No eth_call or eth_getBalance request has been made to infura since opening the UI', + ); + }, + ); + }); + + it('should not make eth_call or eth_getBalance requests after the UI is closed', async function () { + await withFixtures( + { + fixtures: new FixtureBuilder().withNetworkControllerOnMainnet().build(), + ganacheOptions: defaultGanacheOptions, + title: this.test.fullTitle(), + testSpecificMock: mockInfura, + }, + async ({ + driver, + mockedEndpoint, + }: { + driver: Driver; + mockedEndpoint: Mockttp; + }) => { + await unlockWallet(driver); + await driver.delay(3000); + const initialInfuraJsonRpcRequests = await getAllInfuraJsonRpcRequests( + mockedEndpoint, + ); + + await driver.openNewURL('chrome://extensions'); + await driver.delay(20000); + + const currentInfuraJsonRpcRequests = await getAllInfuraJsonRpcRequests( + mockedEndpoint, + ); + const initialEthCallAndGetBalanceRequests = + initialInfuraJsonRpcRequests.filter( + ({ method }) => + method === 'eth_getBalance' || method === 'eth_call', + ); + const currentEthCallAndGetBalanceRequests = + currentInfuraJsonRpcRequests.filter( + ({ method }) => + method === 'eth_getBalance' || method === 'eth_call', + ); + + assert.ok( + initialEthCallAndGetBalanceRequests.length === + currentEthCallAndGetBalanceRequests.length, + 'An eth_call or eth_getBalance request has been made to infura after closing the UI', + ); + }, + ); + }); +}); diff --git a/test/e2e/webdriver/chrome.js b/test/e2e/webdriver/chrome.js index 2e90eec7dd67..ac3e916f3053 100644 --- a/test/e2e/webdriver/chrome.js +++ b/test/e2e/webdriver/chrome.js @@ -30,6 +30,7 @@ class ChromeDriver { }) { const args = [ `--proxy-server=${getProxyServer(proxyPort)}`, // Set proxy in the way that doesn't interfere with Selenium Manager + `--proxy-bypass-list=<-loopback>`, '--disable-features=OptimizationGuideModelDownloading,OptimizationHintsFetching,OptimizationTargetPrediction,OptimizationHints,NetworkTimeServiceQuerying', // Stop chrome from calling home so much (auto-downloads of AI models; time sync) '--disable-component-update', // Stop chrome from calling home so much (auto-update) '--disable-dev-shm-usage', From 236fcbe5dd87f13f9211d016c51817b9366f8669 Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Thu, 25 Jul 2024 16:28:54 -0230 Subject: [PATCH 03/15] lint fix --- privacy-snapshot.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/privacy-snapshot.json b/privacy-snapshot.json index 1143db9792f4..b0c97eeb2357 100644 --- a/privacy-snapshot.json +++ b/privacy-snapshot.json @@ -57,4 +57,4 @@ "unresponsive-rpc.test", "unresponsive-rpc.url", "www.4byte.directory" -] \ No newline at end of file +] From 646c0222a86da4eb56ea7f80ff078c9149516524 Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Thu, 25 Jul 2024 18:22:06 -0230 Subject: [PATCH 04/15] Fix typescript in test file --- .../account-tracker-api-usage.spec.ts | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts b/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts index 306577b0d23c..a21307a6b7fa 100644 --- a/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts +++ b/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts @@ -1,5 +1,6 @@ import { strict as assert } from 'assert'; -import { CompletedRequest } from 'mockttp'; +import { MockedEndpoint } from 'mockttp'; +import { JsonRpcRequest } from '@metamask/utils'; import { withFixtures, defaultGanacheOptions, @@ -9,7 +10,7 @@ import FixtureBuilder from '../../fixture-builder'; import { Driver } from '../../webdriver/driver'; import { Mockttp } from '../../mock-e2e'; -async function mockInfura(mockServer: Mockttp): Promise { +async function mockInfura(mockServer: Mockttp): Promise { const blockNumber = { value: 0 }; return [ @@ -53,9 +54,9 @@ async function mockInfura(mockServer: Mockttp): Promise { } async function getAllInfuraJsonRpcRequests( - mockedEndpoint: Mockttp[], -): Promise { - const allInfuraJsonRpcRequests: CompletedRequest[] = []; + mockedEndpoint: MockedEndpoint[], +): Promise { + const allInfuraJsonRpcRequests: JsonRpcRequest[] = []; let seenRequests; let seenProviderRequests; @@ -67,7 +68,9 @@ async function getAllInfuraJsonRpcRequests( for (const r of seenProviderRequests) { const json = await r.body.getJson(); - allInfuraJsonRpcRequests.push(json); + if (json !== undefined) { + allInfuraJsonRpcRequests.push(json); + } } } @@ -80,7 +83,7 @@ describe('Account Tracker API Usage', function () { { fixtures: new FixtureBuilder().withNetworkControllerOnMainnet().build(), ganacheOptions: defaultGanacheOptions, - title: this.test.fullTitle(), + title: this.test?.fullTitle(), testSpecificMock: mockInfura, }, async ({ @@ -88,7 +91,7 @@ describe('Account Tracker API Usage', function () { mockedEndpoint, }: { driver: Driver; - mockedEndpoint: Mockttp; + mockedEndpoint: MockedEndpoint[]; }) => { await driver.delay(3000); let allInfuraJsonRpcRequests = await getAllInfuraJsonRpcRequests( @@ -126,7 +129,7 @@ describe('Account Tracker API Usage', function () { { fixtures: new FixtureBuilder().withNetworkControllerOnMainnet().build(), ganacheOptions: defaultGanacheOptions, - title: this.test.fullTitle(), + title: this.test?.fullTitle(), testSpecificMock: mockInfura, }, async ({ @@ -134,7 +137,7 @@ describe('Account Tracker API Usage', function () { mockedEndpoint, }: { driver: Driver; - mockedEndpoint: Mockttp; + mockedEndpoint: MockedEndpoint[]; }) => { await unlockWallet(driver); await driver.delay(3000); From 582f4c032ee3c011da83c74d50213d124ce3dfad Mon Sep 17 00:00:00 2001 From: legobt <6wbvkn0j@anonaddy.me> Date: Thu, 25 Jul 2024 19:31:14 +0000 Subject: [PATCH 05/15] chore: readable-stream import cleanup --- app/scripts/metamask-controller.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 3b4440f7f95b..f10303457825 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -1,5 +1,5 @@ import EventEmitter from 'events'; -import readableStream, { pipeline } from 'readable-stream'; +import { finished, pipeline } from 'readable-stream'; import { AssetsContractController, CurrencyRateController, @@ -5111,8 +5111,8 @@ export default class MetamaskController extends EventEmitter { // been added to the handler to ensure that calling it more than once does // not have any affect. - readableStream.finished(outStream, outstreamEndHandler); + finished(outStream, outstreamEndHandler); outStream.on('end', outstreamEndHandler); } From dc7032af0a5296be77936bd629b3198945a6e7c1 Mon Sep 17 00:00:00 2001 From: legobt <6wbvkn0j@anonaddy.me> Date: Thu, 25 Jul 2024 20:07:05 +0000 Subject: [PATCH 06/15] chore: track outStream state directly on stream object --- app/scripts/metamask-controller.js | 31 +++++++++++------------------- 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index f10303457825..beefc9e470c5 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -1,5 +1,5 @@ import EventEmitter from 'events'; -import { finished, pipeline } from 'readable-stream'; +import { finished, pipeline } from 'readable-stream'; import { AssetsContractController, CurrencyRateController, @@ -19,7 +19,6 @@ import { JsonRpcEngine } from 'json-rpc-engine'; import { createEngineStream } from 'json-rpc-middleware-stream'; import { providerAsMiddleware } from '@metamask/eth-json-rpc-middleware'; import { debounce, throttle, memoize, wrap } from 'lodash'; -import { v4 as uuid } from 'uuid'; import { KeyringController, keyringBuilderFactory, @@ -375,7 +374,6 @@ export default class MetamaskController extends EventEmitter { // this keeps track of how many "controllerStream" connections are open // the only thing that uses controller connections are open metamask UI instances this.activeControllerConnections = 0; - this.finishedControllerStreamIds = new Set(); this.offscreenPromise = opts.offscreenPromise ?? Promise.resolve(); @@ -1539,7 +1537,6 @@ export default class MetamaskController extends EventEmitter { this.triggerNetworkrequests(); } else { this.stopNetworkRequests(); - this.finishedControllerStreamIds.clear(); } }); @@ -4978,9 +4975,7 @@ export default class MetamaskController extends EventEmitter { // setup multiplexing const mux = setupMultiplex(connectionStream); // connect features - const newControllerStream = mux.createStream('controller'); - newControllerStream.metamaskStreamId = uuid(); - this.setupControllerConnection(newControllerStream); + this.setupControllerConnection(mux.createStream('controller')); this.setupProviderConnectionEip1193( mux.createStream('provider'), sender, @@ -5078,17 +5073,13 @@ export default class MetamaskController extends EventEmitter { } const outstreamEndHandler = () => { - if ( - !this.finishedControllerStreamIds.has(outStream.metamaskStreamId) && - this.activeControllerConnections > 0 - ) { - this.finishedControllerStreamIds.add(outStream.metamaskStreamId); + if (!outStream.mmFinished) { this.activeControllerConnections -= 1; - this.emit( 'controllerConnectionChanged', this.activeControllerConnections, ); + outStream.mmFinished = true; this.removeListener('update', handleUpdate); } }; @@ -5105,15 +5096,15 @@ export default class MetamaskController extends EventEmitter { // for expediency, we are not addressing them at this time. Instead, we // can observe that `readableStream.finished` preserves the same // functionality as we had when we relied on readable-stream v2. Meanwhile, - // the `outStream.on('end'` handler was observed to have been called at least once. - // As we do not yet fully understand the conditions under which both of these - // are being called, we include both handlers at the risk of redundancy. Logic has - // been added to the handler to ensure that calling it more than once does - // not have any affect. - + // the `outStream.on('end')` handler was observed to have been called at least once. + // In an abundance of caution to prevent against unexpected future behavioral changes in + // streams implementations, we redundantly use multiple paths to attach the same event handler. + // The outstreamEndHandler therefore needs to be idempotent, which introduces the `mmFinished` property. + outStream.mmFinished = false; finished(outStream, outstreamEndHandler); - outStream.on('end', outstreamEndHandler); + outStream.once('close', outstreamEndHandler); + outStream.once('end', outstreamEndHandler); } /** From fe44d809cfccb2ab6507f779848de079b1ae84a2 Mon Sep 17 00:00:00 2001 From: legobt <6wbvkn0j@anonaddy.me> Date: Thu, 25 Jul 2024 20:48:34 +0000 Subject: [PATCH 07/15] chore: test update --- app/scripts/metamask-controller.test.js | 47 ++++++++++--------------- 1 file changed, 18 insertions(+), 29 deletions(-) diff --git a/app/scripts/metamask-controller.test.js b/app/scripts/metamask-controller.test.js index d42beae3cae1..3b9c731c97ca 100644 --- a/app/scripts/metamask-controller.test.js +++ b/app/scripts/metamask-controller.test.js @@ -1412,7 +1412,7 @@ describe('MetaMaskController', () => { streamTest.end(); }); - it('uses a new multiplex to set up a connection with a "controller" stream that has a unique metamaskStreamId', () => { + it('uses a new multiplex to set up a connection', () => { jest.spyOn(metamaskController, 'setupControllerConnection'); const streamTest = createThroughStream((chunk, _, cb) => { @@ -1426,9 +1426,6 @@ describe('MetaMaskController', () => { metamaskController.setupControllerConnection, ).toHaveBeenCalledWith( expect.objectContaining({ - metamaskStreamId: expect.stringMatching( - /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/u, - ), _name: 'controller', _parent: expect.any(ObjectMultiplex), }), @@ -1594,7 +1591,9 @@ describe('MetaMaskController', () => { expect(metamaskController.activeControllerConnections).toBe(0); }); - it('adds controller connections, which when ended, add their stream ids to this.finishedControllerStreamIds', async () => { + // this test could be improved by testing for actual behavior of handlers, + // without touching rawListeners from test + it('attaches listeners for trusted communication streams and removes them as streams close', async () => { jest .spyOn(metamaskController, 'triggerNetworkrequests') .mockImplementation(); @@ -1610,6 +1609,9 @@ describe('MetaMaskController', () => { createTestStream(4), createTestStream(5), ]; + const baseUpdateListenerCount = + metamaskController.rawListeners('update').length; + metamaskController.on( 'controllerConnectionChanged', (activeControllerConnections) => { @@ -1655,14 +1657,16 @@ describe('MetaMaskController', () => { {}, ); - expect( - Array.from(metamaskController.finishedControllerStreamIds), - ).toStrictEqual([]); - await testStreams[1].promise; + + expect(metamaskController.rawListeners('update')).toHaveLength( + baseUpdateListenerCount + 5, + ); + testStreams[1].testStream.end(); await testStreams[3].promise; testStreams[3].testStream.end(); + testStreams[3].testStream.end(); await testStreams[4].promise; testStreams[4].testStream.end(); @@ -1672,23 +1676,8 @@ describe('MetaMaskController', () => { await testStreams[3].onFinishedCallbackPromise; await testStreams[4].onFinishedCallbackPromise; await testStreams[2].onFinishedCallbackPromise; - expect( - Array.from(metamaskController.finishedControllerStreamIds), - ).toStrictEqual( - expect.arrayContaining([ - expect.stringMatching( - /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/u, - ), - expect.stringMatching( - /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/u, - ), - expect.stringMatching( - /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/u, - ), - expect.stringMatching( - /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/u, - ), - ]), + expect(metamaskController.rawListeners('update')).toHaveLength( + baseUpdateListenerCount + 1, ); await testStreams[0].promise; @@ -1696,9 +1685,9 @@ describe('MetaMaskController', () => { await testStreams[0].onFinishedCallbackPromise; - expect( - Array.from(metamaskController.finishedControllerStreamIds), - ).toStrictEqual([]); + expect(metamaskController.rawListeners('update')).toHaveLength( + baseUpdateListenerCount, + ); }); }); From aac4e571ab76ff7c8ce8d279091c8e86b20fd9c1 Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Thu, 25 Jul 2024 20:57:53 -0230 Subject: [PATCH 08/15] Remove no longer needed proxy-bypass-list chrome option, and no longer needed privacy snapshot addition --- privacy-snapshot.json | 18 +++++++++--------- test/e2e/webdriver/chrome.js | 1 - 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/privacy-snapshot.json b/privacy-snapshot.json index b0c97eeb2357..a855a5a3f126 100644 --- a/privacy-snapshot.json +++ b/privacy-snapshot.json @@ -6,7 +6,6 @@ "api.web3modal.com", "app.ens.domains", "arbitrum-mainnet.infura.io", - "authentication.api.cx.metamask.io", "bafkreifvhjdf6ve4jfv6qytqtux5nd4nwnelioeiqx5x2ez5yrgrzk7ypi.ipfs.dweb.link", "bafybeidxfmwycgzcp4v2togflpqh2gnibuexjy4m4qqwxp7nh3jx5zlh4y.ipfs.dweb.link", "cdn.segment.com", @@ -24,22 +23,18 @@ "gas.api.cx.metamask.io", "github.com", "goerli.infura.io", - "lattice.gridplus.io", - "localhost:12345", "localhost:8000", "localhost:8545", - "localhost:9999", "mainnet.infura.io", "metamask.eth", "metamask.github.io", "min-api.cryptocompare.com", "nft.api.cx.metamask.io", - "oidc.api.cx.metamask.io", - "on-ramp-content.api.cx.metamask.io", - "on-ramp-content.uat-api.cx.metamask.io", "phishing-detection.api.cx.metamask.io", "portfolio.metamask.io", "price.api.cx.metamask.io", + "on-ramp-content.api.cx.metamask.io", + "on-ramp-content.uat-api.cx.metamask.io", "proxy.api.cx.metamask.io", "raw.githubusercontent.com", "registry.npmjs.org", @@ -54,7 +49,12 @@ "token.api.cx.metamask.io", "tokens.api.cx.metamask.io", "tx-sentinel-ethereum-mainnet.api.cx.metamask.io", - "unresponsive-rpc.test", "unresponsive-rpc.url", - "www.4byte.directory" + "www.4byte.directory", + "lattice.gridplus.io", + "unresponsive-rpc.test", + "authentication.api.cx.metamask.io", + "oidc.api.cx.metamask.io", + "price.api.cx.metamask.io", + "token.api.cx.metamask.io" ] diff --git a/test/e2e/webdriver/chrome.js b/test/e2e/webdriver/chrome.js index f87160ac7f63..4a8d41afe9ec 100644 --- a/test/e2e/webdriver/chrome.js +++ b/test/e2e/webdriver/chrome.js @@ -30,7 +30,6 @@ class ChromeDriver { }) { const args = [ `--proxy-server=${getProxyServer(proxyPort)}`, // Set proxy in the way that doesn't interfere with Selenium Manager - `--proxy-bypass-list=<-loopback>`, '--disable-features=OptimizationGuideModelDownloading,OptimizationHintsFetching,OptimizationTargetPrediction,OptimizationHints,NetworkTimeServiceQuerying', // Stop chrome from calling home so much (auto-downloads of AI models; time sync) '--disable-component-update', // Stop chrome from calling home so much (auto-update) '--disable-dev-shm-usage', From a31544443deed3f068d5ec04857d22a6ca82fc0d Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Fri, 26 Jul 2024 07:55:11 -0230 Subject: [PATCH 09/15] Fix e2e on firefox by using a empty page instead of a browser specific page --- test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts b/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts index a21307a6b7fa..b08e524a50fb 100644 --- a/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts +++ b/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts @@ -145,7 +145,7 @@ describe('Account Tracker API Usage', function () { mockedEndpoint, ); - await driver.openNewURL('chrome://extensions'); + await driver.openNewURL('about:blank'); await driver.delay(20000); const currentInfuraJsonRpcRequests = await getAllInfuraJsonRpcRequests( From 74226e039628fd99eb5471faa037399c97d0d5d9 Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Fri, 26 Jul 2024 10:12:17 -0230 Subject: [PATCH 10/15] Add code comment explaining increasing block number in api mock --- test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts b/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts index b08e524a50fb..58a7141893b6 100644 --- a/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts +++ b/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts @@ -19,6 +19,10 @@ async function mockInfura(mockServer: Mockttp): Promise { .withJsonBodyIncluding({ method: 'eth_blockNumber' }) .times(50) .thenCallback(() => { + // We need to ensure the mocked block number keeps increasing, + // as this results in block tracker listeners firing, which is + // needed for the potential account tracker network requests being + // tested against here. blockNumber.value += 1; return { From ac50f666eebae7403308e438d4d4598d0a762778 Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Fri, 26 Jul 2024 10:13:29 -0230 Subject: [PATCH 11/15] Add eth_getBlockByNumber to requests to check for after the ui has closed --- .../account-tracker-api-usage.spec.ts | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts b/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts index 58a7141893b6..600513309150 100644 --- a/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts +++ b/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts @@ -83,6 +83,8 @@ async function getAllInfuraJsonRpcRequests( describe('Account Tracker API Usage', function () { it('should not make eth_call or eth_getBalance requests before the UI is opened and should make those requests after the UI is opened', async function () { + const RPC_METHODS_TO_TEST = ['eth_call', 'eth_getBalance']; + await withFixtures( { fixtures: new FixtureBuilder().withNetworkControllerOnMainnet().build(), @@ -102,7 +104,7 @@ describe('Account Tracker API Usage', function () { mockedEndpoint, ); let ethCallAndGetBalanceRequests = allInfuraJsonRpcRequests.filter( - ({ method }) => method === 'eth_getBalance' || method === 'eth_call', + ({ method }) => RPC_METHODS_TO_TEST.includes(method), ); assert.ok( @@ -117,7 +119,7 @@ describe('Account Tracker API Usage', function () { mockedEndpoint, ); ethCallAndGetBalanceRequests = allInfuraJsonRpcRequests.filter( - ({ method }) => method === 'eth_getBalance' || method === 'eth_call', + ({ method }) => RPC_METHODS_TO_TEST.includes(method), ); assert.ok( @@ -129,6 +131,12 @@ describe('Account Tracker API Usage', function () { }); it('should not make eth_call or eth_getBalance requests after the UI is closed', async function () { + const RPC_METHODS_TO_TEST = [ + 'eth_getBlockByNumber', + 'eth_call', + 'eth_getBalance', + ]; + await withFixtures( { fixtures: new FixtureBuilder().withNetworkControllerOnMainnet().build(), @@ -156,14 +164,12 @@ describe('Account Tracker API Usage', function () { mockedEndpoint, ); const initialEthCallAndGetBalanceRequests = - initialInfuraJsonRpcRequests.filter( - ({ method }) => - method === 'eth_getBalance' || method === 'eth_call', + initialInfuraJsonRpcRequests.filter(({ method }) => + RPC_METHODS_TO_TEST.includes(method), ); const currentEthCallAndGetBalanceRequests = - currentInfuraJsonRpcRequests.filter( - ({ method }) => - method === 'eth_getBalance' || method === 'eth_call', + currentInfuraJsonRpcRequests.filter(({ method }) => + RPC_METHODS_TO_TEST.includes(method), ); assert.ok( From 98a10a1ddf3ff8208f04e5f3881783ed39bbb408 Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Fri, 26 Jul 2024 10:15:03 -0230 Subject: [PATCH 12/15] Code cleanup --- .../account-tracker-api-usage.spec.ts | 49 +++++++++++++------ 1 file changed, 34 insertions(+), 15 deletions(-) diff --git a/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts b/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts index 600513309150..604ad3c7829b 100644 --- a/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts +++ b/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts @@ -81,6 +81,15 @@ async function getAllInfuraJsonRpcRequests( return allInfuraJsonRpcRequests; } +function getSpecifiedJsonRpcRequests( + jsonRpcRequestArray: JsonRpcRequest[], + methodsToGet: string[], +) { + return jsonRpcRequestArray.filter(({ method }) => + methodsToGet.includes(method), + ); +} + describe('Account Tracker API Usage', function () { it('should not make eth_call or eth_getBalance requests before the UI is opened and should make those requests after the UI is opened', async function () { const RPC_METHODS_TO_TEST = ['eth_call', 'eth_getBalance']; @@ -103,13 +112,16 @@ describe('Account Tracker API Usage', function () { let allInfuraJsonRpcRequests = await getAllInfuraJsonRpcRequests( mockedEndpoint, ); - let ethCallAndGetBalanceRequests = allInfuraJsonRpcRequests.filter( - ({ method }) => RPC_METHODS_TO_TEST.includes(method), + let ethCallAndGetBalanceRequests = getSpecifiedJsonRpcRequests( + allInfuraJsonRpcRequests, + RPC_METHODS_TO_TEST, ); assert.ok( ethCallAndGetBalanceRequests.length === 0, - 'An eth_call or eth_getBalance request has been made to infura before opening the UI', + `An ${RPC_METHODS_TO_TEST.join( + ' or ', + )} request has been made to infura before opening the UI`, ); await unlockWallet(driver); @@ -118,13 +130,16 @@ describe('Account Tracker API Usage', function () { allInfuraJsonRpcRequests = await getAllInfuraJsonRpcRequests( mockedEndpoint, ); - ethCallAndGetBalanceRequests = allInfuraJsonRpcRequests.filter( - ({ method }) => RPC_METHODS_TO_TEST.includes(method), + ethCallAndGetBalanceRequests = getSpecifiedJsonRpcRequests( + allInfuraJsonRpcRequests, + RPC_METHODS_TO_TEST, ); assert.ok( ethCallAndGetBalanceRequests.length > 0, - 'No eth_call or eth_getBalance request has been made to infura since opening the UI', + `No ${RPC_METHODS_TO_TEST.join( + ' or ', + )} request has been made to infura since opening the UI`, ); }, ); @@ -163,19 +178,23 @@ describe('Account Tracker API Usage', function () { const currentInfuraJsonRpcRequests = await getAllInfuraJsonRpcRequests( mockedEndpoint, ); - const initialEthCallAndGetBalanceRequests = - initialInfuraJsonRpcRequests.filter(({ method }) => - RPC_METHODS_TO_TEST.includes(method), - ); - const currentEthCallAndGetBalanceRequests = - currentInfuraJsonRpcRequests.filter(({ method }) => - RPC_METHODS_TO_TEST.includes(method), - ); + + const initialEthCallAndGetBalanceRequests = getSpecifiedJsonRpcRequests( + initialInfuraJsonRpcRequests, + RPC_METHODS_TO_TEST, + ); + + const currentEthCallAndGetBalanceRequests = getSpecifiedJsonRpcRequests( + currentInfuraJsonRpcRequests, + RPC_METHODS_TO_TEST, + ); assert.ok( initialEthCallAndGetBalanceRequests.length === currentEthCallAndGetBalanceRequests.length, - 'An eth_call or eth_getBalance request has been made to infura after closing the UI', + `An ${RPC_METHODS_TO_TEST.join( + ' or ', + )} request has been made to infura after closing the UI`, ); }, ); From ca67fc4f5dc46260f2e1d857f2c39bf64db6be56 Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Mon, 29 Jul 2024 12:17:51 -0230 Subject: [PATCH 13/15] Clarify testing of eth_getBlockByNumber --- .../account-tracker-api-usage.spec.ts | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts b/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts index 604ad3c7829b..1644007d895d 100644 --- a/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts +++ b/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts @@ -92,6 +92,10 @@ function getSpecifiedJsonRpcRequests( describe('Account Tracker API Usage', function () { it('should not make eth_call or eth_getBalance requests before the UI is opened and should make those requests after the UI is opened', async function () { + // Note: we are not testing that eth_getBlockByNumber is not called before the UI + // is opened because there is a known bug that results in it being called if the + // user is already onboarded: https://github.com/MetaMask/MetaMask-planning/issues/2151 + // Once that issue is resolved, we can add eth_getBlockByNumber to the below array. const RPC_METHODS_TO_TEST = ['eth_call', 'eth_getBalance']; await withFixtures( @@ -112,13 +116,13 @@ describe('Account Tracker API Usage', function () { let allInfuraJsonRpcRequests = await getAllInfuraJsonRpcRequests( mockedEndpoint, ); - let ethCallAndGetBalanceRequests = getSpecifiedJsonRpcRequests( + let rpcMethodsToTestRequests = getSpecifiedJsonRpcRequests( allInfuraJsonRpcRequests, RPC_METHODS_TO_TEST, ); assert.ok( - ethCallAndGetBalanceRequests.length === 0, + rpcMethodsToTestRequests.length === 0, `An ${RPC_METHODS_TO_TEST.join( ' or ', )} request has been made to infura before opening the UI`, @@ -130,13 +134,13 @@ describe('Account Tracker API Usage', function () { allInfuraJsonRpcRequests = await getAllInfuraJsonRpcRequests( mockedEndpoint, ); - ethCallAndGetBalanceRequests = getSpecifiedJsonRpcRequests( + rpcMethodsToTestRequests = getSpecifiedJsonRpcRequests( allInfuraJsonRpcRequests, RPC_METHODS_TO_TEST, ); assert.ok( - ethCallAndGetBalanceRequests.length > 0, + rpcMethodsToTestRequests.length > 0, `No ${RPC_METHODS_TO_TEST.join( ' or ', )} request has been made to infura since opening the UI`, @@ -145,7 +149,7 @@ describe('Account Tracker API Usage', function () { ); }); - it('should not make eth_call or eth_getBalance requests after the UI is closed', async function () { + it('should not make eth_call or eth_getBalance or eth_getBlockByNumber requests after the UI is closed', async function () { const RPC_METHODS_TO_TEST = [ 'eth_getBlockByNumber', 'eth_call', @@ -179,19 +183,19 @@ describe('Account Tracker API Usage', function () { mockedEndpoint, ); - const initialEthCallAndGetBalanceRequests = getSpecifiedJsonRpcRequests( + const initialRpcMethodsToTestRequests = getSpecifiedJsonRpcRequests( initialInfuraJsonRpcRequests, RPC_METHODS_TO_TEST, ); - const currentEthCallAndGetBalanceRequests = getSpecifiedJsonRpcRequests( + const currentRpcMethodsToTestRequests = getSpecifiedJsonRpcRequests( currentInfuraJsonRpcRequests, RPC_METHODS_TO_TEST, ); assert.ok( - initialEthCallAndGetBalanceRequests.length === - currentEthCallAndGetBalanceRequests.length, + initialRpcMethodsToTestRequests.length === + currentRpcMethodsToTestRequests.length, `An ${RPC_METHODS_TO_TEST.join( ' or ', )} request has been made to infura after closing the UI`, From fda57b303be6a964743c561ba1ceafd69bded5b8 Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Mon, 29 Jul 2024 12:19:23 -0230 Subject: [PATCH 14/15] Update test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts Co-authored-by: Danica Shen --- test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts b/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts index 1644007d895d..26445555920a 100644 --- a/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts +++ b/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts @@ -5,6 +5,7 @@ import { withFixtures, defaultGanacheOptions, unlockWallet, + veryLargeDelayMs, } from '../../helpers'; import FixtureBuilder from '../../fixture-builder'; import { Driver } from '../../webdriver/driver'; @@ -112,7 +113,7 @@ describe('Account Tracker API Usage', function () { driver: Driver; mockedEndpoint: MockedEndpoint[]; }) => { - await driver.delay(3000); + await driver.delay(veryLargeDelayMs); let allInfuraJsonRpcRequests = await getAllInfuraJsonRpcRequests( mockedEndpoint, ); @@ -129,7 +130,7 @@ describe('Account Tracker API Usage', function () { ); await unlockWallet(driver); - await driver.delay(3000); + await driver.delay(veryLargeDelayMs); allInfuraJsonRpcRequests = await getAllInfuraJsonRpcRequests( mockedEndpoint, @@ -171,7 +172,7 @@ describe('Account Tracker API Usage', function () { mockedEndpoint: MockedEndpoint[]; }) => { await unlockWallet(driver); - await driver.delay(3000); + await driver.delay(veryLargeDelayMs); const initialInfuraJsonRpcRequests = await getAllInfuraJsonRpcRequests( mockedEndpoint, ); From 27df50c86d6d6b55bf48be66286b4ab203ed73be Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Mon, 29 Jul 2024 12:25:53 -0230 Subject: [PATCH 15/15] Add code comment explaining the 20000 ms delay --- test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts b/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts index 26445555920a..5ab35138ce7b 100644 --- a/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts +++ b/test/e2e/tests/api-usage/account-tracker-api-usage.spec.ts @@ -178,6 +178,8 @@ describe('Account Tracker API Usage', function () { ); await driver.openNewURL('about:blank'); + // The delay is intentionally 20000, to ensure we cover at least 1 polling + // loop of time for the block tracker. await driver.delay(20000); const currentInfuraJsonRpcRequests = await getAllInfuraJsonRpcRequests(