From 75801e9502e309965014377500986fed5c5ad4c5 Mon Sep 17 00:00:00 2001 From: witmicko Date: Wed, 8 Mar 2023 16:33:27 +0000 Subject: [PATCH 1/2] delay chain validation (#17413) --- app/_locales/en/messages.json | 9 + .../handlers/add-ethereum-chain.js | 50 ++--- coverage-targets.js | 8 +- test/e2e/mock-e2e.js | 27 +++ test/e2e/tests/add-custom-network.spec.js | 208 +++++++++++++++++- .../confirmation-footer.js | 10 +- ui/pages/confirmation/confirmation.js | 44 +++- .../templates/add-ethereum-chain.js | 51 ++++- ui/pages/confirmation/templates/index.js | 1 + 9 files changed, 355 insertions(+), 53 deletions(-) diff --git a/app/_locales/en/messages.json b/app/_locales/en/messages.json index bd3750698b54..c5aacd5d5b69 100644 --- a/app/_locales/en/messages.json +++ b/app/_locales/en/messages.json @@ -263,6 +263,9 @@ "addToken": { "message": "Add token" }, + "addingCustomNetwork": { + "message": "Adding Network" + }, "address": { "message": "Address" }, @@ -1308,6 +1311,9 @@ "message": "Stack:", "description": "Title for error stack, which is displayed for debugging purposes" }, + "errorWhileConnectingToRPC": { + "message": "Error while connecting to the custom network." + }, "ethGasPriceFetchWarning": { "message": "Backup gas price is provided as the main gas estimation service is unavailable right now." }, @@ -2033,6 +2039,9 @@ "mismatchedNetworkSymbol": { "message": "The submitted currency symbol does not match what we expect for this chain ID." }, + "mismatchedRpcChainId": { + "message": "Chain ID returned by the custom network does not match the submitted chain ID." + }, "mismatchedRpcUrl": { "message": "According to our records the submitted RPC URL value does not match a known provider for this chain ID." }, diff --git a/app/scripts/lib/rpc-method-middleware/handlers/add-ethereum-chain.js b/app/scripts/lib/rpc-method-middleware/handlers/add-ethereum-chain.js index 7d08d0719854..150c7d351f61 100644 --- a/app/scripts/lib/rpc-method-middleware/handlers/add-ethereum-chain.js +++ b/app/scripts/lib/rpc-method-middleware/handlers/add-ethereum-chain.js @@ -10,7 +10,6 @@ import { isPrefixedFormattedHexString, isSafeChainId, } from '../../../../../shared/modules/network.utils'; -import { jsonRpcRequest } from '../../../../../shared/modules/rpc.utils'; const addEthereumChain = { methodNames: [MESSAGE_TYPE.ADD_ETHEREUM_CHAIN], @@ -155,6 +154,7 @@ async function addEthereumChainHandler( if (currentChainId === _chainId && currentRpcUrl === firstValidRPCUrl) { return end(); } + // If this network is already added with but is not the currently selected network // Ask the user to switch the network try { @@ -182,28 +182,6 @@ async function addEthereumChainHandler( return end(); } - let endpointChainId; - - try { - endpointChainId = await jsonRpcRequest(firstValidRPCUrl, 'eth_chainId'); - } catch (err) { - return end( - ethErrors.rpc.internal({ - message: `Request for method 'eth_chainId on ${firstValidRPCUrl} failed`, - data: { networkErr: err }, - }), - ); - } - - if (_chainId !== endpointChainId) { - return end( - ethErrors.rpc.invalidParams({ - message: `Chain ID returned by RPC URL ${firstValidRPCUrl} does not match ${_chainId}`, - data: { chainId: endpointChainId }, - }), - ); - } - if (typeof chainName !== 'string' || !chainName) { return end( ethErrors.rpc.invalidParams({ @@ -266,20 +244,18 @@ async function addEthereumChainHandler( } try { - await addCustomRpc( - await requestUserApproval({ - origin, - type: MESSAGE_TYPE.ADD_ETHEREUM_CHAIN, - requestData: { - chainId: _chainId, - blockExplorerUrl: firstValidBlockExplorerUrl, - chainName: _chainName, - rpcUrl: firstValidRPCUrl, - ticker, - }, - }), - ); - + const customRpc = await requestUserApproval({ + origin, + type: MESSAGE_TYPE.ADD_ETHEREUM_CHAIN, + requestData: { + chainId: _chainId, + blockExplorerUrl: firstValidBlockExplorerUrl, + chainName: _chainName, + rpcUrl: firstValidRPCUrl, + ticker, + }, + }); + await addCustomRpc(customRpc); sendMetrics({ event: 'Custom Network Added', category: EVENT.CATEGORIES.NETWORK, diff --git a/coverage-targets.js b/coverage-targets.js index 7c48ace8e8af..83ee2c5ccb40 100644 --- a/coverage-targets.js +++ b/coverage-targets.js @@ -6,10 +6,10 @@ // subset of files to check against these targets. module.exports = { global: { - lines: 64, - branches: 52.5, - statements: 63.1, - functions: 56.1, + lines: 64.5, + branches: 53, + statements: 63, + functions: 56.5, }, transforms: { branches: 100, diff --git a/test/e2e/mock-e2e.js b/test/e2e/mock-e2e.js index 9213a6e5b70f..cda544dedc78 100644 --- a/test/e2e/mock-e2e.js +++ b/test/e2e/mock-e2e.js @@ -32,6 +32,20 @@ async function setupMocking(server, testSpecificMock) { return {}; }, }); + await server + .forPost( + 'https://arbitrum-mainnet.infura.io/v3/00000000000000000000000000000000', + ) + .thenCallback(() => { + return { + statusCode: 200, + json: { + jsonrpc: '2.0', + id: '1675864782845', + result: '0xa4b1', + }, + }; + }); await server.forPost('https://api.segment.io/v1/batch').thenCallback(() => { return { @@ -372,6 +386,19 @@ async function setupMocking(server, testSpecificMock) { json: emptyHotlist, }; }); + + await server + .forPost('https://customnetwork.com/api/customRPC') + .thenCallback(() => { + return { + statusCode: 200, + json: { + jsonrpc: '2.0', + id: '1675864782845', + result: '0x122', + }, + }; + }); } module.exports = { setupMocking }; diff --git a/test/e2e/tests/add-custom-network.spec.js b/test/e2e/tests/add-custom-network.spec.js index b43a8a3aed8e..c46ca2694f05 100644 --- a/test/e2e/tests/add-custom-network.spec.js +++ b/test/e2e/tests/add-custom-network.spec.js @@ -17,6 +17,213 @@ describe('Custom network', function () { }, ], }; + + it('should show warning when adding chainId 0x1(ethereum) and be followed by an wrong chainId error', async function () { + await withFixtures( + { + dapp: true, + fixtures: new FixtureBuilder() + .withPermissionControllerConnectedToTestDapp() + .build(), + ganacheOptions, + 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/'); + await driver.executeScript(` + var params = [{ + chainId: "0x1", + chainName: "Fake Ethereum Network", + nativeCurrency: { + name: "", + symbol: "ETH", + decimals: 18 + }, + rpcUrls: ["https://customnetwork.com/api/customRPC"], + blockExplorerUrls: [ "http://localhost:8080/api/customRPC" ] + }] + window.ethereum.request({ + method: 'wallet_addEthereumChain', + params + }) + `); + const windowHandles = await driver.waitUntilXWindowHandles(3); + + await driver.switchToWindowWithTitle( + 'MetaMask Notification', + windowHandles, + ); + + await driver.clickElement({ + tag: 'button', + text: 'Approve', + }); + + const warningTxt = + 'You are adding a new RPC provider for Ethereum Mainnet'; + + await driver.findElement({ + tag: 'h4', + text: warningTxt, + }); + + await driver.clickElement({ + tag: 'button', + text: 'Approve', + }); + + const errMsg = + 'Chain ID returned by the custom network does not match the submitted chain ID.'; + await driver.findElement({ + tag: 'span', + text: errMsg, + }); + + const approveBtn = await driver.findElement({ + tag: 'button', + text: 'Approve', + }); + + assert.equal(await approveBtn.isEnabled(), false); + await driver.clickElement({ + tag: 'button', + text: 'Cancel', + }); + }, + ); + }); + it("don't add bad rpc custom network", async function () { + await withFixtures( + { + dapp: true, + fixtures: new FixtureBuilder() + .withPermissionControllerConnectedToTestDapp() + .build(), + ganacheOptions, + 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/'); + await driver.executeScript(` + var params = [{ + chainId: "0x123", + chainName: "Antani", + nativeCurrency: { + name: "", + symbol: "ANTANI", + decimals: 18 + }, + rpcUrls: ["https://customnetwork.com/api/customRPC"], + blockExplorerUrls: [ "http://localhost:8080/api/customRPC" ] + }] + window.ethereum.request({ + method: 'wallet_addEthereumChain', + params + }) + `); + const windowHandles = await driver.waitUntilXWindowHandles(3); + + await driver.switchToWindowWithTitle( + 'MetaMask Notification', + windowHandles, + ); + await driver.clickElement({ + tag: 'button', + text: 'Approve', + }); + + const errMsg = + 'Chain ID returned by the custom network does not match the submitted chain ID.'; + await driver.findElement({ + tag: 'span', + text: errMsg, + }); + + const approveBtn = await driver.findElement({ + tag: 'button', + text: 'Approve', + }); + + assert.equal(await approveBtn.isEnabled(), false); + await driver.clickElement({ + tag: 'button', + text: 'Cancel', + }); + }, + ); + }); + + it("don't add unreachable custom network", async function () { + await withFixtures( + { + dapp: true, + fixtures: new FixtureBuilder() + .withPermissionControllerConnectedToTestDapp() + .build(), + ganacheOptions, + 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/'); + await driver.executeScript(` + var params = [{ + chainId: "0x123", + chainName: "Antani", + nativeCurrency: { + name: "", + symbol: "ANTANI", + decimals: 18 + }, + rpcUrls: ["https://doesntexist.abc/customRPC"], + blockExplorerUrls: [ "http://localhost:8080/api/customRPC" ] + }] + window.ethereum.request({ + method: 'wallet_addEthereumChain', + params + }) + `); + const windowHandles = await driver.waitUntilXWindowHandles(3); + + await driver.switchToWindowWithTitle( + 'MetaMask Notification', + windowHandles, + ); + await driver.clickElement({ + tag: 'button', + text: 'Approve', + }); + + await driver.findElement({ + tag: 'span', + text: 'Error while connecting to the custom network.', + }); + + const approveBtn = await driver.findElement({ + tag: 'button', + text: 'Approve', + }); + + assert.equal(await approveBtn.isEnabled(), false); + await driver.clickElement({ + tag: 'button', + text: 'Cancel', + }); + }, + ); + }); + it('add custom network and switch the network', async function () { await withFixtures( { @@ -85,7 +292,6 @@ describe('Custom network', function () { await driver.clickElement({ tag: 'button', text: 'Close' }); await driver.clickElement({ tag: 'button', text: 'Approve' }); - await driver.clickElement({ tag: 'h6', text: 'Switch to Arbitrum One', diff --git a/ui/pages/confirmation/components/confirmation-footer/confirmation-footer.js b/ui/pages/confirmation/components/confirmation-footer/confirmation-footer.js index d73149fb8840..30f116d3f5e5 100644 --- a/ui/pages/confirmation/components/confirmation-footer/confirmation-footer.js +++ b/ui/pages/confirmation/components/confirmation-footer/confirmation-footer.js @@ -8,11 +8,15 @@ export default function ConfirmationFooter({ onCancel, submitText, cancelText, + loadingText, alerts, + loading, + submitAlerts, }) { return (
{alerts} + {submitAlerts}
{onCancel ? ( ) : null}
@@ -39,4 +44,7 @@ ConfirmationFooter.propTypes = { cancelText: PropTypes.string, onSubmit: PropTypes.func.isRequired, submitText: PropTypes.string.isRequired, + loadingText: PropTypes.string, + loading: PropTypes.bool, + submitAlerts: PropTypes.node, }; diff --git a/ui/pages/confirmation/confirmation.js b/ui/pages/confirmation/confirmation.js index 60b305f72460..d9c75e5970a7 100644 --- a/ui/pages/confirmation/confirmation.js +++ b/ui/pages/confirmation/confirmation.js @@ -177,6 +177,10 @@ export default function ConfirmationPage({ const setInputState = (key, value) => { setInputStates((currentState) => ({ ...currentState, [key]: value })); }; + const [loading, setLoading] = useState(false); + const [loadingText, setLoadingText] = useState(); + + const [submitAlerts, setSubmitAlerts] = useState([]); ///: BEGIN:ONLY_INCLUDE_IN(flask) const snap = useSelector((state) => @@ -258,14 +262,28 @@ export default function ConfirmationPage({ return INPUT_STATE_CONFIRMATIONS.includes(type); }; - const handleSubmit = () => - templateState[pendingConfirmation.id]?.useWarningModal - ? setShowWarningModal(true) - : templatedValues.onSubmit( - hasInputState(pendingConfirmation.type) - ? inputStates[MESSAGE_TYPE.SNAP_DIALOG_PROMPT] - : null, - ); + const handleSubmitResult = (submitResult) => { + if (submitResult?.length > 0) { + setLoadingText(templatedValues.submitText); + setSubmitAlerts(submitResult); + setLoading(true); + } else { + setLoading(false); + } + }; + const handleSubmit = async () => { + setLoading(true); + if (templateState[pendingConfirmation.id]?.useWarningModal) { + setShowWarningModal(true); + } else { + const inputState = hasInputState(pendingConfirmation.type) + ? inputStates[MESSAGE_TYPE.SNAP_DIALOG_PROMPT] + : null; + // submit result is an array of errors or empty on success + const submitResult = await templatedValues.onSubmit(inputState); + handleSubmitResult(submitResult); + } + }; return (
@@ -332,7 +350,8 @@ export default function ConfirmationPage({ {showWarningModal && ( { - await templatedValues.onSubmit(); + const res = await templatedValues.onSubmit(); + await handleSubmitResult(res); setShowWarningModal(false); }} onCancel={templatedValues.onCancel} @@ -362,6 +381,13 @@ export default function ConfirmationPage({ onCancel={templatedValues.onCancel} submitText={templatedValues.submitText} cancelText={templatedValues.cancelText} + loadingText={loadingText || templatedValues.loadingText} + loading={loading} + submitAlerts={submitAlerts.map((alert, idx) => ( + + + + ))} />
); diff --git a/ui/pages/confirmation/templates/add-ethereum-chain.js b/ui/pages/confirmation/templates/add-ethereum-chain.js index b3ade78dade8..d2cd9781483f 100644 --- a/ui/pages/confirmation/templates/add-ethereum-chain.js +++ b/ui/pages/confirmation/templates/add-ethereum-chain.js @@ -14,6 +14,7 @@ import { import { DEFAULT_ROUTE } from '../../../helpers/constants/routes'; import ZENDESK_URLS from '../../../helpers/constants/zendesk-url'; import fetchWithCache from '../../../../shared/lib/fetch-with-cache'; +import { jsonRpcRequest } from '../../../../shared/modules/rpc.utils'; const UNRECOGNIZED_CHAIN = { id: 'UNRECOGNIZED_CHAIN', @@ -101,6 +102,34 @@ const MISMATCHED_NETWORK_RPC = { }, }; +const MISMATCHED_NETWORK_RPC_CHAIN_ID = { + id: 'MISMATCHED_NETWORK_RPC_CHAIN_ID', + severity: SEVERITIES.DANGER, + content: { + element: 'span', + children: { + element: 'MetaMaskTranslation', + props: { + translationKey: 'mismatchedRpcChainId', + }, + }, + }, +}; + +const ERROR_CONNECTING_TO_RPC = { + id: 'ERROR_CONNECTING_TO_RPC', + severity: SEVERITIES.DANGER, + content: { + element: 'span', + children: { + element: 'MetaMaskTranslation', + props: { + translationKey: 'errorWhileConnectingToRPC', + }, + }, + }, +}; + async function getAlerts(pendingApproval) { const alerts = []; const safeChainsList = @@ -154,7 +183,7 @@ function getState(pendingApproval) { function getValues(pendingApproval, t, actions, history) { const originIsMetaMask = pendingApproval.origin === 'metamask'; - + const customRpcUrl = pendingApproval.requestData.rpcUrl; return { content: [ { @@ -180,6 +209,7 @@ function getValues(pendingApproval, t, actions, history) { }, ], }, + { element: 'Typography', key: 'title', @@ -331,7 +361,25 @@ function getValues(pendingApproval, t, actions, history) { ], cancelText: t('cancel'), submitText: t('approveButtonText'), + loadingText: t('addingCustomNetwork'), onSubmit: async () => { + let endpointChainId; + try { + endpointChainId = await jsonRpcRequest(customRpcUrl, 'eth_chainId'); + } catch (err) { + console.error( + `Request for method 'eth_chainId on ${customRpcUrl} failed`, + ); + return [ERROR_CONNECTING_TO_RPC]; + } + + if (pendingApproval.requestData.chainId !== endpointChainId) { + console.error( + `Chain ID returned by RPC URL ${customRpcUrl} does not match ${endpointChainId}`, + ); + return [MISMATCHED_NETWORK_RPC_CHAIN_ID]; + } + await actions.resolvePendingApproval( pendingApproval.id, pendingApproval.requestData, @@ -340,6 +388,7 @@ function getValues(pendingApproval, t, actions, history) { actions.addCustomNetwork(pendingApproval.requestData); history.push(DEFAULT_ROUTE); } + return []; }, onCancel: () => actions.rejectPendingApproval( diff --git a/ui/pages/confirmation/templates/index.js b/ui/pages/confirmation/templates/index.js index 1c4019b63ff4..12a042462571 100644 --- a/ui/pages/confirmation/templates/index.js +++ b/ui/pages/confirmation/templates/index.js @@ -33,6 +33,7 @@ const ALLOWED_TEMPLATE_KEYS = [ 'onSubmit', 'networkDisplay', 'submitText', + 'loadingText', ]; /** From 6222cf0b7de8c0deaf70fb3da9795d64b00f443d Mon Sep 17 00:00:00 2001 From: Pedro Figueiredo Date: Wed, 8 Mar 2023 16:55:59 +0000 Subject: [PATCH 2/2] increment keyring controller version (#18036) --- package.json | 2 +- yarn.lock | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/package.json b/package.json index baa8b5f8afa3..5f99891cdfd1 100644 --- a/package.json +++ b/package.json @@ -234,7 +234,7 @@ "@metamask/desktop": "^0.3.0", "@metamask/eth-json-rpc-infura": "^7.0.0", "@metamask/eth-json-rpc-middleware": "^10.0.0", - "@metamask/eth-keyring-controller": "^10.0.0", + "@metamask/eth-keyring-controller": "^10.0.1", "@metamask/eth-ledger-bridge-keyring": "^0.13.0", "@metamask/eth-token-tracker": "^4.0.0", "@metamask/etherscan-link": "^2.2.0", diff --git a/yarn.lock b/yarn.lock index cac6e78d4519..352b2b6d2a84 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3788,16 +3788,16 @@ __metadata: languageName: node linkType: hard -"@metamask/eth-keyring-controller@npm:^10.0.0": - version: 10.0.0 - resolution: "@metamask/eth-keyring-controller@npm:10.0.0" +"@metamask/eth-keyring-controller@npm:^10.0.1": + version: 10.0.1 + resolution: "@metamask/eth-keyring-controller@npm:10.0.1" dependencies: "@metamask/browser-passworder": ^4.0.2 "@metamask/eth-hd-keyring": ^6.0.0 "@metamask/eth-sig-util": 5.0.2 "@metamask/eth-simple-keyring": ^5.0.0 obs-store: ^4.0.3 - checksum: 6b06a74ae5da610289a4f5a7018b55d17d55b3d533fcd2d1675704902575d435373e4c2b3f4a019f6c14492f326e0dbfe7c04bd94ae8ebc38382fbe3b9c86b7c + checksum: 61ce1c7de37c414b5917c004e61c4d1ca71465bd95a00c749408fdde2105cd9c15bedc32c7ce2f6f2f4ff31ca8b6c7056f3460ad91e3d24d9f43974d84b5603b languageName: node linkType: hard @@ -24057,7 +24057,7 @@ __metadata: "@metamask/eslint-config-typescript": ^9.0.1 "@metamask/eth-json-rpc-infura": ^7.0.0 "@metamask/eth-json-rpc-middleware": ^10.0.0 - "@metamask/eth-keyring-controller": ^10.0.0 + "@metamask/eth-keyring-controller": ^10.0.1 "@metamask/eth-ledger-bridge-keyring": ^0.13.0 "@metamask/eth-token-tracker": ^4.0.0 "@metamask/etherscan-link": ^2.2.0