Implement EIP-6963 #263

rekmarks · 2023-05-23T21:42:22Z

Implements EIP-6963 according to the current state of the specification. Adds two new top-level imports, eip6963AnnounceProvider and eip6963RequestProvider, used by us and the dapp, respectively.

For reference, here are some EIP6963ProviderInfo values that would work for us:

export const MetaMaskEIP6963ProviderInfo: EIP6963ProviderInfo = {
  uuid: uuid(),
  name: 'MetaMask', // Update depending on the build, probably
  icon: 'https://raw.githubusercontent.com/MetaMask/brand-resources/cb6fd847f3a9cc5e231c749383c3898935e62eab/SVG/metamask-fox.svg', // TODO: Find a shorter URL
};

The purpose of the uuid field is to help dapps notice if multiple wallets are claiming the same name. It needs to be a valid v4 UUID. IMO we should create a UUID at the point where we call announceProvider (i.e. the content script for the extension). We could also send it from the background.

socket-security · 2023-05-23T21:43:43Z

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

rekmarks · 2023-05-27T18:14:04Z

src/EIP6963.ts

+function isValidProviderDetail(
+  providerDetail: unknown,
+): providerDetail is EIP6963ProviderDetail {
+  if (!isObject(providerDetail) || !isObject(providerDetail.info)) {
+    return false;
+  }
+  const { info } = providerDetail as EIP6963ProviderDetail;
+
+  return (
+    typeof info.icon === 'string' &&
+    isValidUrl(info.icon) &&
+    typeof info.name === 'string' &&
+    Boolean(info.name) &&
+    typeof info.uuid === 'string' &&
+    UUID_V4_REGEX.test(info.uuid) &&
+    typeof info.walletId === 'string' &&
+    Boolean(info.walletId)
+  );
+}


Normally I think we'd use superstruct or something for this, but it's important to keep the bundle size as small as possible.

superstruct is only 11.5 kB (or 3.4 kB gzipped), and @metmask/providers is 75.1 kB (or 21.1 kB gzipped). I don't think it makes a significant difference. 😅

Thoughts here? @rekmarks @Mrtenz

socket-security · 2023-06-08T16:02:54Z

No top level dependency changes detected. Learn more about Socket for GitHub ↗︎

src/EIP6963.ts

jest.setup.browser.js

src/EIP6963.test.ts

Gudahtt · 2023-07-10T21:48:17Z

jest.setup.browser.js

+  });
+});
+
+// Reset JSDOM. This attempts to remove side effects from tests, however it does


I ran into JSDOM state issues when working on the phishing warning page too. In that case, I found it easiest to just use a real browser. See MetaMask/phishing-warning#62

Likewise for browser-passworder, that one is a plain library that we wanted to test in a real browser to ensure we were using browser APIs correctly. That uses playwright too: https://github.com/MetaMask/browser-passworder/blob/f870292ec5db729c8f69bbe4b2d3cff60053f333/test/index.spec.ts

Something to consider, if this needs further maintenance or continues to be a burden.

…nceProviderInfo in initializeProvider

jiexi · 2023-09-27T21:32:19Z

This should be caught up to spec now

src/initializeInpageProvider.ts

src/EIP6963.ts

mcmire

I read over EIP-6963, and the implementation makes sense. Just had minor questions about tests and type assertions.

mcmire · 2023-10-04T22:36:05Z

.eslintrc.js

+    {
+      files: ['jest.setup.browser.js'],
+      env: { browser: true },
+      // This file contains copypasta and we're not going to bother fixing these.


/Users/jiexi/Projects/providers/jest.setup.browser.js 25:1 error All hooks must be wrapped in a describe block jest/require-top-level-describe 30:5 error Missing JSDoc block description jsdoc/require-description 32:1 error Missing JSDoc @param "type" description jsdoc/require-param-description 32:1 error Missing JSDoc @param "type" type jsdoc/require-param-type 33:1 error Missing JSDoc @param "listener" description jsdoc/require-param-description 33:1 error Missing JSDoc @param "listener" type jsdoc/require-param-type 34:1 error Missing JSDoc @param "options" description jsdoc/require-param-description 34:1 error Missing JSDoc @param "options" type jsdoc/require-param-type 55:1 error All hooks must be wrapped in a describe block jest/require-top-level-describe

Definitely need to ignore jest/require-top-level-describe

do you feel it's worthwhile to add jsdoc to the rest? I can go either way

lol, this was me btw. It seems strictly not worth it, as I have a hard time imagining a lot of work being done in that file, but not my call of course!

I'm good with this, just thought it was funny :)

src/EIP6963.ts

src/EIP6963.test.ts

src/EIP6963.ts

mcmire · 2023-10-05T20:33:41Z

src/index.ts

@@ -20,4 +35,6 @@ export {
  setGlobalProvider,
  shimWeb3,
  StreamProvider,
+  eip6963AnnounceProvider,


Hmm, this name seems a bit mashed together. Too bad we can't put a type under a namespace of some kind.

hmm.. i'm not sure it would make sense to drop the EIP6963 prefixes either 😬

src/EIP6963.ts

…ider()

mcmire

Looks good to me!

## Explanation Helps to alleviate `window.ethereum` conflicts by supporting an alternative way for Dapps to discover and interact with providers. This PR implements [EIP-6963](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-6963.md) which adds an async event based provider discovery standard. ~~NOTE: I'm not sure if we strictly need the changes I made that add EIP-6963 support to the test dapp, but we probably want to add an e2e test for EIP-6963 and I'm not sure if we want our tests to rely on an external site like https://eip6963.org/ . Maybe we can test this by running some js in page like we do for testing parts of the json rpc in e2e environment.~~ I've added a spec for this that tests it directly in the browser console instead Ticket: [mmp-869](MetaMask/MetaMask-planning#869) Related: MetaMask/providers#263 ~~Related: MetaMask/test-dapp#243 # Screenshot <img width="470" alt="Screenshot 2023-09-27 at 4 41 13 PM" src="https://github.com/MetaMask/metamask-extension/assets/918701/88a0e334-fed9-45e8-80a2-329cb4c94ded"> ## Manual Testing Steps * Install and enable a few other browser extension wallets * Visit https://eip6963.org/ * MetaMask should show up in the list of providers and play nicely with all of them ## Pre-merge author checklist - [x] I've clearly explained: - [x] What problem this PR is solving - [x] How this problem was solved - [x] How reviewers can test my changes - [x] Sufficient automated test coverage has been added ## Pre-merge reviewer checklist - [ ] Manual testing (e.g. pull and build branch, run in browser, test code being changed) - [ ] PR is linked to the appropriate GitHub issue - [ ] **IF** this PR fixes a bug in the release milestone, add this PR to the release milestone If further QA is required (e.g. new feature, complex testing steps, large refactor), add the `Extension QA Board` label. In this case, a QA Engineer approval will be be required.

rekmarks added 2 commits May 23, 2023 14:07

Implement EIP-6963

753329c

Update lint config

0c7f43f

rekmarks added 3 commits May 24, 2023 10:21

Parameterize provider detail, remove hard-coded values

2715047

Add comments, validate uuid field

2aa85d5

Improve detail and info validation

117ae12

rekmarks force-pushed the eip-6963 branch from 69ecba9 to 117ae12 Compare May 25, 2023 04:58

rekmarks mentioned this pull request May 27, 2023

[Test] Patch in EIP-6963 support MetaMask/metamask-extension#19320

Closed

rekmarks commented May 27, 2023

View reviewed changes

rekmarks mentioned this pull request May 31, 2023

Make compatible with EIP-6963 MetaMask/detect-provider#81

Open

Remove 'walletId' field per spec updates

8eb8c8e