-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
base: master
Are you sure you want to change the base?
Adapt app for metamask integration extension #5216
Conversation
@jacogr Ready for review. Updated PR description. Some changes might not be necessary once extension PR is merged |
package.json
Outdated
"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" |
There was a problem hiding this comment.
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 => ({ |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
waiting on polkadot-js/extension#566 to be merged and then remove dependencies as per #5216 (comment) |
package.json
Outdated
"stream-http": "^3.2.0", | ||
"web3": "^1.3.5" | ||
} | ||
"packageManager": "[email protected]" |
There was a problem hiding this comment.
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'
I added all the deps in another PR, including the required webpack overrides. (Had to do this when updating the I did have to make a small tweak to not create havoc out there for users of the 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(...); |
Had to add @types/node because of jestjs/jest#11640 |
@jacogr updated |
Is this still relevant? (ie. functionality missing?) |
This pull request introduces 11 alerts when merging 21588be into 34f28bf - view on LGTM.com new alerts:
|
@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 |
@jacogr I don't understand why Im getting |
yarn polkadot-dev-copy-to apps
), not sure if necessary once extension is merged