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

Integrate MetaMask/web3 into polkadot-js interface #4024

Open
joelamouche opened this issue Nov 13, 2020 · 17 comments
Open

Integrate MetaMask/web3 into polkadot-js interface #4024

joelamouche opened this issue Nov 13, 2020 · 17 comments

Comments

@joelamouche
Copy link
Contributor

As of now, for an ethereum compatible blockchain, the user has to manually import their MetaMask accounts to the app by exporting the private key and entering it.

This is a little bit of friction that could be avoided if we used the injected web3 object to list accounts and sign transactions.

It would require a lot of code change because it would mean using web3 instead of ui-keyring everywhere (if isEthereum is true or maybe a manual switch).

Maybe there is a way to wrap web3 to have the same interface as keyring.

What do you think @jacogr ?

@jacogr
Copy link
Member

jacogr commented Nov 13, 2020

If it can expose this extension interface, it can work as a normal extension - https://github.com/polkadot-js/extension/blob/master/packages/extension-inject/src/types.ts#L95

A compat layer has actually been done before, see https://github.com/polkadot-js/extension/blob/master/packages/extension-dapp/src/compat/singleSource.ts - in that case it took that object and translated it into the known (supports multiple extensions) interface.

Happy with a compat layer like done above, however direct support for the Metamask layer is going to create too many edge-cases.

@joelamouche
Copy link
Contributor Author

Sounds good, I will take at the compatibility layer. Thanks!

@jacogr
Copy link
Member

jacogr commented Nov 13, 2020

Shout if you need help. While simple it is not exceptionally well documented. But I believe it is do-able - the only issues may be on the signing layer, but there are examples scattered around.

@joelamouche
Copy link
Contributor Author

@jacogr I'm back on this issue.

So if I understand correctly, I need to copy https://github.com/polkadot-js/extension/blob/master/packages/extension-dapp/src/compat/singleSource.ts to adapt injected metamask web3 to conform to the right interface. Is there any way I can test the single source extension injection and extension-dapp when I dev on it?

@jacogr
Copy link
Member

jacogr commented Nov 24, 2020

I will get back to you on that - need to have a refresher first.

@joelamouche
Copy link
Contributor Author

Hi @jacogr , did you get the occasion to look at the code? Thanks in advance

@jacogr
Copy link
Member

jacogr commented Dec 7, 2020

So the best way to test, against apps, is via the following setup -

  • <ROOT>/apps
  • <ROOT>/extension

<ROOT> being any place, just as long as they two repos are on the same level

Then

  • make the changes in the extension, run yarn polkadot-dev-copy-to apps to build and copy the updated packages
  • re-start apps with yarn start to pickup the new changes

@joelamouche
Copy link
Contributor Author

Hi @jacogr

So I pushed some changes on polkadot-js/extension#566

First off, I want to say that the singleSource has probably not been tested because the initCompat function was never called.

Secondly, when I build the apps after running yarn polkadot-dev-copy-to apps I get this error:
Screenshot 2020-12-08 at 14 40 09

Please let me know how to add web3 as dependency for this package.

Thanks in advance,

Antoine

@jacogr
Copy link
Member

jacogr commented Dec 8, 2020

It was actually tested and working :) It was however not maintained since the SingleSource app effectively became unused. (I didn't remove the code completely, only since it may have had some use in the future - like now)

It is just a normal monorepo, nothing special. Remove the peerDep, just keep it as a normal dependency.

@joelamouche
Copy link
Contributor Author

Thanks for the help.

So I was able to display the address on polkadot-js/extension#566 but only with a small modififcation on the ui-keyring (polkadot-js/ui#413)

I was not able to get the signer to work. This is not suprising since the signer is never loaded into the app. I did notice that there was a signer option in the signAndSend functions of the signer component.

How should we load the signer? As part of the address object (with updated type) or as a different parallel loading process?

@jacogr
Copy link
Member

jacogr commented Dec 8, 2020

Yes. It gets injected alongside on the same object.

Weird that the original SingleSource doesn’t have the signer anymore - it must be ancient then (aka pre the current signing interfaces)

@joelamouche
Copy link
Contributor Author

Summary of the current state of MetaMask integration into apps:

Now

  • accounts are injected successfully and display with correct balance
  • when implementing signRaw with the eth_sign call, we do trigger the signature on the extension, but nothing gets return
  • MetaMask is planing on discontinuing support for eth_sign in favor of signTypedData and personal_sign because it is a security issue
  • MetaMask appends the signed data with "Ethereum Signature :" message to prevent the signature from being used elsewhere (which is exactly what we are trying to do)

Short term

Since my goal is to support the basic functionality of being able to transfer tokens from the app, we can use the eth_signTransaction call to MM and then post the tx's byte code as an extrinsic (unsigned) to the apps using the .transac call. This would however require to change the signer logic as it is used in the app.

Longer term

We don't know yet if we want to do this, but in the future we will have some governance/staking related features that will need to be called a signed extrinsic and we will need MetaMask to support signing bytecode without anything appended. We don't know if that's something that they want to do but it could be worth asking them.

What do you think @jacogr

@jacogr
Copy link
Member

jacogr commented Dec 11, 2020

Does personal_sign have the same wrapper around it? And signTypedData? If they sign raw bytes, it is possibly worth-while exploring either already.

@joelamouche
Copy link
Contributor Author

joelamouche commented Dec 14, 2020

https://docs.metamask.io/guide/signing-data.html#a-brief-history
ethereum/go-ethereum#2940

personal_sign does prepend data with "\x19Ethereum Signed Message:\n" + len(message). and signTypedData is even worse because it requires the data to be in a json-like format

@joelamouche
Copy link
Contributor Author

I was able to make a test proving that MetaMask still supports eth_sign without the prefix : https://github.com/joelamouche/test-dapp/tree/testEthSign

So I'm resuming the effort to integrate MetaMask

polkadot-js/common#969 (ready)
polkadot-js/extension#566 (wip)
polkadot-js/ui#464 (ready)
#5216 (wip)

Are all part of a first milestone: displaying injected accounts from metamask into the app

@joelamouche
Copy link
Contributor Author

@jacogr Great news! Metamask integration was tested with a simple transfer!
All PRs are ready but maybe apps should be merged last (see description)

@joelamouche
Copy link
Contributor Author

@albertov19 ready to be tested again

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

No branches or pull requests

2 participants