From 7a34024a109a00ca97b3403df33aa4cdcda1eaf2 Mon Sep 17 00:00:00 2001 From: Daedalus <0xDaedalus@users.noreply.github.com> Date: Tue, 14 Mar 2023 14:03:29 +0100 Subject: [PATCH 1/3] Switch to a given network if adding a network that is already added. Since a given dapp does not necessarily have a sense of what networks are supported within the wallet (and since some dapps rely exclusively on wallet_addEthereumChain) we should support the functionality of switching to a network when receiving a wallet_addEthereumChain message with a chainID that we already support. --- background/services/provider-bridge/index.ts | 24 ++++++++++++++++---- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/background/services/provider-bridge/index.ts b/background/services/provider-bridge/index.ts index 5ca4a6b93d..173a522dfe 100644 --- a/background/services/provider-bridge/index.ts +++ b/background/services/provider-bridge/index.ts @@ -525,6 +525,25 @@ export default class ProviderBridgeService extends BaseService { ) } + const [rawChainData, address, siteTitle, favicon] = params + const validatedData = validateAddEthereumChainParameter( + rawChainData as AddEthereumChainParameter + ) + + const supportedNetwork = + await this.internalEthereumProviderService.getTrackedNetworkByChainId( + validatedData.chainId + ) + + if (supportedNetwork) { + // If the network is already added - just switch to it. + return await this.internalEthereumProviderService.routeSafeRPCRequest( + method, + params, + origin + ) + } + const id = this.addNetworkRequestId.toString() this.addNetworkRequestId += 1 @@ -540,11 +559,6 @@ export default class ProviderBridgeService extends BaseService { } }) - const [rawChainData, address, siteTitle, favicon] = params - const validatedData = validateAddEthereumChainParameter( - rawChainData as AddEthereumChainParameter - ) - const userConfirmation = new Promise((resolve, reject) => { this.#pendingAddNetworkRequests[id] = { resolve, From 5e8bc26a4bb6fd5f84955dbf621f8e73e9f64424 Mon Sep 17 00:00:00 2001 From: Antonio Salazar Cardozo Date: Mon, 3 Jul 2023 21:47:18 -0400 Subject: [PATCH 2/3] Fix concurrency error in provider bridge test The new check for whether a new custom network already exists changes timing that was expected to be static. The relevant assertions are now wrapped in retries that will wait a certain amount of time for the given assertion to pass. --- background/services/provider-bridge/index.ts | 2 +- .../provider-bridge/tests/index.unit.test.ts | 18 +++++++++++------- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/background/services/provider-bridge/index.ts b/background/services/provider-bridge/index.ts index d2b38881d4..f67114ea7e 100644 --- a/background/services/provider-bridge/index.ts +++ b/background/services/provider-bridge/index.ts @@ -542,7 +542,7 @@ export default class ProviderBridgeService extends BaseService { this.handleAddNetworkRequest(id, false) } }) - + const [rawChainData, address, siteTitle, favicon] = params const validatedData = validateAddEthereumChainParameter( rawChainData as AddEthereumChainParameter diff --git a/background/services/provider-bridge/tests/index.unit.test.ts b/background/services/provider-bridge/tests/index.unit.test.ts index af95ab95d1..adbcfd45ec 100644 --- a/background/services/provider-bridge/tests/index.unit.test.ts +++ b/background/services/provider-bridge/tests/index.unit.test.ts @@ -6,8 +6,8 @@ import sinon from "sinon" import browser from "webextension-polyfill" // eslint-disable-next-line import/no-extraneous-dependencies import { waitFor } from "@testing-library/dom" +import * as popupUtils from "../show-popup" import * as featureFlags from "../../../features" -import { wait } from "../../../lib/utils" import { createProviderBridgeService } from "../../../tests/factories" import { AddEthereumChainParameter } from "../../internal-ethereum-provider" import ProviderBridgeService from "../index" @@ -133,6 +133,7 @@ describe("ProviderBridgeService", () => { const { enablingPermission } = BASE_DATA jest.spyOn(featureFlags, "isEnabled").mockImplementation(() => true) + const showPopupSpy = jest.spyOn(popupUtils, "default") const request = providerBridgeService.routeContentScriptRPCRequest( { @@ -147,17 +148,20 @@ describe("ProviderBridgeService", () => { const IEP = providerBridgeService["internalEthereumProviderService"] const spy = jest.spyOn(IEP, "routeSafeRPCRequest") - await wait(0) // wait next tick to setup popup + // wait until popup is set up + await waitFor(() => expect(showPopupSpy).toHaveBeenCalled()) const validatedPayload = validateAddEthereumChainParameter( params[0] as AddEthereumChainParameter ) - expect(providerBridgeService.getNewCustomRPCDetails("0")).toEqual({ - ...validatedPayload, - favicon: "favicon.png", - siteTitle: "some site", - }) + await waitFor(() => + expect(providerBridgeService.getNewCustomRPCDetails("0")).toEqual({ + ...validatedPayload, + favicon: "favicon.png", + siteTitle: "some site", + }) + ) expect(spy).not.toHaveBeenCalled() providerBridgeService.handleAddNetworkRequest("0", true) From af041d676028a404e2b96c1a7a9c69af5b69b081 Mon Sep 17 00:00:00 2001 From: Antonio Salazar Cardozo Date: Mon, 3 Jul 2023 22:21:27 -0400 Subject: [PATCH 3/3] Fix switching networks when adding an existing network This fix is the result of adding a test to confirm a popup is not shown. Notably, the behavior of switching the network silently requires extra vetting; however, this commit implements it as that was the branch's intent. --- background/services/provider-bridge/index.ts | 22 +++---- .../provider-bridge/tests/index.unit.test.ts | 60 ++++++++++++++++++- 2 files changed, 69 insertions(+), 13 deletions(-) diff --git a/background/services/provider-bridge/index.ts b/background/services/provider-bridge/index.ts index f67114ea7e..04a7282456 100644 --- a/background/services/provider-bridge/index.ts +++ b/background/services/provider-bridge/index.ts @@ -532,17 +532,6 @@ export default class ProviderBridgeService extends BaseService { this.addNetworkRequestId += 1 - const window = await showExtensionPopup( - AllowedQueryParamPage.addNewChain, - { requestId: id.toString() } - ) - - browser.windows.onRemoved.addListener((removed) => { - if (removed === window.id) { - this.handleAddNetworkRequest(id, false) - } - }) - const [rawChainData, address, siteTitle, favicon] = params const validatedData = validateAddEthereumChainParameter( rawChainData as AddEthereumChainParameter @@ -562,6 +551,17 @@ export default class ProviderBridgeService extends BaseService { ) } + const window = await showExtensionPopup( + AllowedQueryParamPage.addNewChain, + { requestId: id.toString() } + ) + + browser.windows.onRemoved.addListener((removed) => { + if (removed === window.id) { + this.handleAddNetworkRequest(id, false) + } + }) + const userConfirmation = new Promise((resolve, reject) => { this.#pendingAddNetworkRequests[id] = { resolve, diff --git a/background/services/provider-bridge/tests/index.unit.test.ts b/background/services/provider-bridge/tests/index.unit.test.ts index adbcfd45ec..bfca958c35 100644 --- a/background/services/provider-bridge/tests/index.unit.test.ts +++ b/background/services/provider-bridge/tests/index.unit.test.ts @@ -4,6 +4,8 @@ import { } from "@tallyho/provider-bridge-shared" import sinon from "sinon" import browser from "webextension-polyfill" +// FIXME Pull the appropriate dependency to this package.json so we're not +// FIXME relying on weird cross-package dependencies. // eslint-disable-next-line import/no-extraneous-dependencies import { waitFor } from "@testing-library/dom" import * as popupUtils from "../show-popup" @@ -12,6 +14,7 @@ import { createProviderBridgeService } from "../../../tests/factories" import { AddEthereumChainParameter } from "../../internal-ethereum-provider" import ProviderBridgeService from "../index" import { validateAddEthereumChainParameter } from "../utils" +import { ETHEREUM } from "../../../constants" const WINDOW = { focused: true, @@ -144,8 +147,8 @@ describe("ProviderBridgeService", () => { enablingPermission.origin ) - // eslint-disable-next-line @typescript-eslint/dot-notation - const IEP = providerBridgeService["internalEthereumProviderService"] + // @ts-expect-error private access to reference the service + const IEP = providerBridgeService.internalEthereumProviderService const spy = jest.spyOn(IEP, "routeSafeRPCRequest") // wait until popup is set up @@ -176,5 +179,58 @@ describe("ProviderBridgeService", () => { await expect(request).resolves.toEqual(null) // resolves without errors }) + + it("should skip user confirmation if the network already exists", async () => { + const params = [ + { + chainId: "1", + chainName: "Ethereum Mainnet", + nativeCurrency: { name: "Ether", symbol: "ETH", decimals: 18 }, + iconUrl: undefined, + rpcUrls: ["booyan"], + blockExplorerUrls: ["https://etherscan.io"], + }, + "0xd8da6bf26964af9d7eed9e03e53415d37aa96045", + "some site", + "favicon.png", + ] + + const { enablingPermission } = BASE_DATA + + jest.spyOn(featureFlags, "isEnabled").mockImplementation(() => true) + const showPopupSpy = jest.spyOn(popupUtils, "default") + + const internalEthereumProvider = + // @ts-expect-error private access to reference the service + providerBridgeService.internalEthereumProviderService + jest + .spyOn(internalEthereumProvider, "getTrackedNetworkByChainId") + .mockImplementation(() => Promise.resolve(ETHEREUM)) + const internalEthereumProviderSpy = jest.spyOn( + internalEthereumProvider, + "routeSafeRPCRequest" + ) + + const request = providerBridgeService.routeContentScriptRPCRequest( + { + ...enablingPermission, + }, + "wallet_addEthereumChain", + params, + enablingPermission.origin + ) + + await waitFor(() => + expect(internalEthereumProviderSpy).toHaveBeenCalledWith( + "wallet_addEthereumChain", + params, + BASE_DATA.origin + ) + ) + + // expect no popup + expect(showPopupSpy).not.toHaveBeenCalled() + await expect(request).resolves.toEqual(null) // resolves without errors + }) }) })