Add custom network RPC method by rekmarks · Pull Request #9724 · MetaMask/metamask-extension

rekmarks · 2020-10-26T21:43:37Z

Implement wallet_addEthereumChain in background, per EIP-3085.

UI is WIP in a number of different PRs

metamaskbot · 2020-11-14T18:24:41Z

Builds ready [187dca2]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (400 ± 51 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	30	89	45	15	7
		domContentLoaded	273	631	398	107	51
		load	275	633	400	107	51
		domInteractive	273	630	397	107	51

Gudahtt · 2021-01-06T14:55:06Z

I believe this addresses #5101

metamaskbot · 2021-01-19T23:28:43Z

Builds ready [3acb000]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
code coverage: Report
dep viz: background
all artifacts

Page Load Metrics (672 ± 66 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	NaN	NaN	NaN	NaN	NaN
		domContentLoaded	362	864	666	132	63
		load	364	871	672	137	66
		domInteractive	362	863	665	132	63

brad-decker · 2021-02-03T19:44:27Z

@rekmarks 335b9ab implements the switch network approval. This is the only change since you last looked.

metamaskbot · 2021-02-03T19:57:19Z

Builds ready [335b9ab]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
code coverage: Report
all artifacts

Page Load Metrics (522 ± 40 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	43	80	54	8	4
		domContentLoaded	347	623	521	84	40
		load	348	624	522	84	40
		domInteractive	347	622	521	84	40

metamaskbot · 2021-02-04T21:54:14Z

Builds ready [b9c39cb]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (756 ± 60 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	54	108	82	12	6
		domContentLoaded	468	844	754	125	60
		load	469	845	756	125	60
		domInteractive	468	844	753	124	60

brad-decker · 2021-02-09T19:22:56Z

@danjm @Gudahtt -- I believe this is ready for review and if found to be good I'll comment out the code that makes the API active. Once we're ready to launch the feature I'll do a small PR to add that code back in.

danjm · 2021-02-10T14:17:55Z

ui/app/helpers/utils/util.js

- * or throws an error in case of failure.
- */
-export async function jsonRpcRequest(rpcUrl, rpcMethod, rpcParams = []) {
-  let fetchUrl = rpcUrl;


Review note: this method has been moved to rpc.utils.js but it is unchanged

danjm · 2021-02-10T14:19:02Z

shared/modules/rpc.utils.js

+ * @param {Array<unknown>} [rpcParams] - The RPC method params.
+ * @returns {Promise<unknown|undefined>} Returns the result of the RPC method call,
+ * or throws an error in case of failure.
+ */


Review note: this is the same as the jsonRpcRequest that was removed from ui/app/helpers/utils/util.js, the code is unchanged

danjm · 2021-02-10T14:27:31Z

app/scripts/lib/rpc-method-middleware/handlers/add-ethereum-chain.js

+      'chainId',
+      'chainName',
+      'blockExplorerUrls',
+      'iconUrls',


I notice that 'iconUrls' is not included in otherKeys, but neither is it used anywhere. Is there some reason we are allowing this in the request params even though it will go unused?

It is a part of EIP-3085, the goal here being to warn for extraneous keys that are not a part of the EIP. For example: 'blockExplorerUrls' is what we want, but someone might put 'blockExplorerUrl'. Beings as blockExplorerUrls is optional, without this check we'd accept the request and add the chain to the user's MetaMask without the blockExplorer, and the dapp would be none the wiser.

danjm · 2021-02-10T14:36:01Z

app/scripts/lib/rpc-method-middleware/handlers/add-ethereum-chain.js

+            chainId: existingNetwork.chainId,
+            nickname: existingNetwork.nickname,
+            ticker: existingNetwork.ticker,
+          },


Are we unable to support the updating of the block-explorer url?

when we get here the user will have already added the chain to their MetaMask, and the block explorer URL will have been set. This requestData is set because updateRpcTarget requires it.

danjm

I left a couple comments and questions

Co-authored-by: Erik Marks <25517051+rekmarks@users.noreply.github.com>

metamaskbot · 2021-02-10T21:56:58Z

Builds ready [bb63145]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (590 ± 22 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	44	89	62	13	6
		domContentLoaded	510	745	589	46	22
		load	512	746	590	46	22
		domInteractive	510	745	589	46	22

danjm

this looks good to me

metamaskbot · 2021-02-11T21:03:37Z

Builds ready [d04ab15]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (610 ± 36 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	46	83	60	11	5
		domContentLoaded	468	824	609	75	36
		load	470	826	610	75	36
		domInteractive	468	824	608	75	36

rekmarks added the DO-NOT-MERGE Pull requests that should not be merged label Oct 26, 2020

rekmarks mentioned this pull request Oct 26, 2020

Add Approval Controller MetaMask/core#289

Merged

danjm added this to the v8.1.next? milestone Nov 2, 2020

danjm requested a review from Gudahtt November 3, 2020 17:14

danjm assigned Gudahtt Nov 3, 2020

danjm self-requested a review November 3, 2020 17:27

danjm self-assigned this Nov 3, 2020

rekmarks force-pushed the custom-network-rpc-method branch 6 times, most recently from b583df3 to 187dca2 Compare November 14, 2020 18:06

pattarapon23 approved these changes Nov 14, 2020

View reviewed changes

pattarapon23 previously approved these changes Nov 14, 2020

View reviewed changes

MetaMask deleted a comment from pattarapon23 Nov 14, 2020

rekmarks dismissed pattarapon23’s stale review via 7d596aa December 15, 2020 17:56

rekmarks force-pushed the custom-network-rpc-method branch 2 times, most recently from 7d596aa to 11bf9e8 Compare December 15, 2020 18:44

rekmarks assigned brad-decker and unassigned Gudahtt and danjm Dec 15, 2020

brad-decker force-pushed the custom-network-rpc-method branch 3 times, most recently from b4bbd83 to 3acb000 Compare January 19, 2021 23:08

brad-decker force-pushed the custom-network-rpc-method branch 2 times, most recently from b606037 to 53028e1 Compare January 20, 2021 17:21

brad-decker force-pushed the custom-network-rpc-method branch from 335b9ab to 53c77ed Compare February 4, 2021 18:38

danjm reviewed Feb 10, 2021

View reviewed changes

danjm suggested changes Feb 10, 2021

View reviewed changes

brad-decker and others added 8 commits February 10, 2021 13:53

Add custom network RPC method

564686e

Co-authored-by: Erik Marks <25517051+rekmarks@users.noreply.github.com>

add additional logic checks

0e51f9b

change reqs

f5ad5f9

update to iconUrls

81011fc

Remove outdated comment

4b88b9e

add switch networks message type

e34ff42

lint

c6919d7

throw user rejection error on switch

bb63145

brad-decker force-pushed the custom-network-rpc-method branch from b9c39cb to bb63145 Compare February 10, 2021 21:42

brad-decker requested a review from danjm February 10, 2021 22:19

danjm previously approved these changes Feb 11, 2021

View reviewed changes

comment out handler

d04ab15

brad-decker dismissed danjm’s stale review via d04ab15 February 11, 2021 20:48

danjm approved these changes Feb 12, 2021

View reviewed changes

brad-decker merged commit e48053a into develop Feb 12, 2021

brad-decker deleted the custom-network-rpc-method branch February 12, 2021 15:26

github-actions bot locked and limited conversation to collaborators Feb 12, 2021

Gudahtt removed the DO-NOT-MERGE Pull requests that should not be merged label Feb 23, 2021

Conversation

rekmarks commented Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metamaskbot commented Nov 14, 2020

Uh oh!

Gudahtt commented Jan 6, 2021

Uh oh!

metamaskbot commented Jan 19, 2021

Uh oh!

brad-decker commented Feb 3, 2021

Uh oh!

metamaskbot commented Feb 3, 2021

Uh oh!

metamaskbot commented Feb 4, 2021

Uh oh!

brad-decker commented Feb 9, 2021

Uh oh!

danjm Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

danjm Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

danjm Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

brad-decker Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

danjm Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

brad-decker Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

danjm left a comment

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Feb 10, 2021

Uh oh!

danjm left a comment

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Feb 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

rekmarks commented Oct 26, 2020 •

edited

Loading