Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adapt app for metamask integration extension #5216

Open
wants to merge 40 commits into
base: master
Choose a base branch
from

Conversation

joelamouche
Copy link
Contributor

@joelamouche joelamouche commented May 5, 2021

  • Add changes to support new extension (tested with yarn polkadot-dev-copy-to apps), not sure if necessary once extension is merged
  • Add types to injected accounts, necessary for the MetaMask integration to work

@joelamouche joelamouche marked this pull request as draft May 5, 2021 16:47
@joelamouche joelamouche changed the title adapted webpack and added packages adapt app for metamask integration extension May 6, 2021
@joelamouche joelamouche marked this pull request as ready for review May 10, 2021 17:17
@joelamouche joelamouche changed the title adapt app for metamask integration extension Adapt app for metamask integration extension May 10, 2021
@joelamouche joelamouche marked this pull request as draft May 10, 2021 17:20
@joelamouche joelamouche marked this pull request as ready for review May 11, 2021 10:45
@joelamouche
Copy link
Contributor Author

@jacogr Ready for review. Updated PR description. Some changes might not be necessary once extension PR is merged

package.json Outdated
Comment on lines 103 to 109
"dependencies": {
"@metamask/detect-provider": "^1.2.0",
"assert": "^2.0.0",
"https-browserify": "^1.0.0",
"os-browserify": "^0.3.0",
"stream-http": "^3.2.0",
"web3": "^1.3.5"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These will just need to drop when the extension is included.

@@ -82,13 +84,14 @@ async function getInjectedAccounts (injectedPromise: Promise<InjectedExtension[]

const accounts = await web3Accounts();

return accounts.map(({ address, meta }, whenCreated): InjectedAccountExt => ({
return accounts.map(({ address, meta, type }, whenCreated): InjectedAccountExt => ({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On line 85 above it should explicitly ask for the ethereum types, when the flag is passed. (Comments in extension on this - I'm guessing this is based on the isEthereum flag)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDIT: Since we do filter on the type in the keyring anyway, it is most probably not needed, since we do have the same effect. (Probably just more descriptive always being explicit, i.e. no need to even pass these through unless needed)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what do you think I should do? Leave as is?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhhh.... ok this links through to this comment as well polkadot-js/extension#566 (comment)

In the keyring itself, with the injected load, we don't actually check the type at all, e.g. https://github.com/polkadot-js/ui/blob/master/packages/ui-keyring/src/Keyring.ts#L249-L260 - we just inject what is there. The best place to add a solution is either -

  • in the extension, i.e. add an extra flag (may not be the prettiest, as described in the linked comment it is kinda ugly)
  • on the keyring itself, i.e. filtering by type there (just need to follow there again, you did do some type filtering already there, maybe I'm missing something in the link)

Would not add a solution inside the apps UI since there are a lot of extension-dapp users, and don't want them to have to make all the same changes.

Anyway, we can test this once all ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the filter in the extension dapp (in web3accounts), so I will pass down the type flag here

@joelamouche
Copy link
Contributor Author

waiting on polkadot-js/extension#566 to be merged and then remove dependencies as per #5216 (comment)

@jacogr jacogr mentioned this pull request Aug 17, 2021
package.json Outdated
"stream-http": "^3.2.0",
"web3": "^1.3.5"
}
"packageManager": "[email protected]"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I removed this, I get the errors again, even though the dependency was updated @jacogr

➜  apps git:(jlm-adapt-app-to-metamask-extension) ✗ yarn start
$ polkadot-dev-clean-build 
[webpack-cli] Failed to load '/Users/antoineestienne/GitHubRepo/apps/packages/apps/webpack.serve.cjs' config
[webpack-cli] Error: Cannot find module 'stream-http'

@jacogr
Copy link
Member

jacogr commented Sep 6, 2021

I added all the deps in another PR, including the required webpack overrides. (Had to do this when updating the extension* packages to latest). This went here #6072

I did have to make a small tweak to not create havoc out there for users of the extension-dapp package - I split the metamask compat into a separate package (code as-is), basically to make it optional for others. See polkadot-js/extension#810

So obviously in apps we will always include it, but it just allows other users to correctly choose instead of having it forced on them (which also means updating their builds in the same way as done here)

The extension PR does mean a slight change to use, i.e. the function needs to be passed in as part of the web3Enable, e.g. https://github.com/polkadot-js/extension/pull/810/files#diff-60639b27a793d918f655ef618cd1778eb219812f1200d48079560408ac4ae3f0R66

Something like -

import initMetaMask from '@polkadot/extension-compat-metamask';
...
web3Enable('polkadot-js/apps', [initMetaMask]).then(...);

@joelamouche
Copy link
Contributor Author

Had to add @types/node because of jestjs/jest#11640

@joelamouche
Copy link
Contributor Author

@jacogr updated

@jacogr
Copy link
Member

jacogr commented Dec 22, 2021

Is this still relevant? (ie. functionality missing?)

@lgtm-com
Copy link

lgtm-com bot commented Dec 27, 2021

This pull request introduces 11 alerts when merging 21588be into 34f28bf - view on LGTM.com

new alerts:

  • 11 for Syntax error

@joelamouche
Copy link
Contributor Author

@jacogr Yes this is still relevant: the missing feature is the injection of MetaMask account sinto the app for ethereum parachains. I just tested it and it still works. HOWEVER, it uses the bytecode signing from MetaMask that triggers the relevant warning

@joelamouche
Copy link
Contributor Author

@jacogr I don't understand why Im getting export 'default' (imported as 'initMetaMask') was not found in '@polkadot/extension-compat-metamask' (possible exports: packageInfo)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants