-
Notifications
You must be signed in to change notification settings - Fork 419
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
Add metamask extension web3 #566
Add metamask extension web3 #566
Conversation
joelamouche
commented
Dec 8, 2020
•
edited
Loading
edited
- Create injectMetaMaskWeb3 function to inject MetaMask accounts and allow signing Tx
- First sends a request to MM to ask the user to authorize the app
- Metamask accounts are now displayed in the accounts section
- User can use theses accounts to sign tx
- Signing will trigger a MM window with a warning (because signing unprefixed messages is dangerous)
MetaMask integration is back 🥳 @jacogr After a long inquiry, I discovered that eth_sign (unprefixed) is available. |
@jacogr There is a problem though: when I run
web3 being a dependency added by my extension |
And when I install
And similar errors for http, https, os using yarn add to add them doesnt work |
Adapting the apps helped: polkadot-js/apps#5216 The console shows the right logs and MetaMask does ask me for user permissions. but then the address doesn't display in the accounts. Can you refresh my memory: what do I need to do to display it and test it? |
I think the problem is that the injcted accounts are injected without a type and default to substrate type addresses. Furthermore, it looks like |
polkadot-js/common#969 (ready) Are all part of a first milestone: displaying injected accounts from metamask into the app |
@ubaistp ? |
@jacogr ready for review (tested) |
@jacogr updated and ready for review |
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.
Think we are close. Some small comments and updates.
(Bit difficult wading through the reformats as well to ensure that the logic is stil in-tact)
@@ -73,15 +73,13 @@ function injectSingleSource (win: SingleWindow): void { | |||
// https://github.com/cennznet/singlesource-extension/blob/f7cb35b54e820bf46339f6b88ffede1b8e140de0/react-example/src/App.js#L19 | |||
export default function initSingleSource (): Promise<boolean> { | |||
return new Promise((resolve): void => { | |||
window.addEventListener('load', (): void => { | |||
const win = window as Window & SingleWindow; | |||
const win = window as Window & SingleWindow; |
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.
Believe I commented elsewhere - the original is correct. (All ok, since this will be dropped anyway)
console.error(`Error initializing ${name}: ${error.message}`); | ||
}) | ||
]) | ||
Object.entries(win.injectedWeb3).map( |
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.
Still applicable, but will do a manual revert of this and the one above once in.
injected.map( | ||
async ({ accounts, name: source, signer }): Promise<InjectedAccountWithMeta[]> => { | ||
try { | ||
const list = await accounts.get(); |
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 on web3Accounts()
would just add an extra flag for the types, e.g. ['sr25519', 'ed25519', 'ethereum']
- there may be some dance required by users to correctly pass the flag, but it is all ok.
So I think the easiest is to indeed initialise all source, but then just filter the accounts if need be. By default would not use (if nothing passed) the filter as `['sr25519', 'ed25519'] - obviously if no type is available, just pass it through.
The tricky bit here is that if we don't have sr25519
we never pass through the accounts with no type.
Should work in the case of the apps UI as well, e.g. in the case of Moonbeam, we just pass through the ['ethereum']
type. (As mentioned above that interface may also require some additional juggling, but all ok)
Added a deps PR (web3 & detect-provider) in #794 just to eliminate conflicts. |
Co-authored-by: Jaco <[email protected]>
Co-authored-by: Jaco <[email protected]>
@jacogr updated and ready for review |
@jacogr Updated! |
Yes, sr/ed is default. However the Substrate secp accounts can also be used and it valid. In broad terms there is just a split between the Ethereum-style and Substrate-style. |
@jacogr updated and ready for review |
@jacogr please review when you have some time |
On it, want to get it in ASAP. (And importantly, get a release out with this) Could you just look at the yarn.lock conflicts? |
@jacogr done |
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.
Thank you.
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |