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

External accounts implemented for issue #5640 #6143

Closed
wants to merge 23 commits into from

Conversation

r001
Copy link

@r001 r001 commented Feb 12, 2019

This is the reopen of PR #6007.
External acccounts allow Metamask to track account whose private keys are stored on a safe device, and tx data and signatures are exchanged by using QR codes. This allows very safe account password storage.

This pull request enables users to create so called external accounts. This means that after applying this pull request in the "import account" menu there will be an "external" tab that enables importing an ethereum address. There is no local private key stored, but upon confirming a transaction a QR code is presented and the transaction can be signed by an external signer. And the signature can be provided in the signature input field. By providing the signature, the transaction will be signed, and published.

The way it works is the following:

  1. User creates transaction (or msg signing) and Confirms it.
  2. Request goes to the background, and since the from-accunt belongs to eth-external-account-keyring, the keyrings signTransaction() function is called.
  3. Sign transaction adds an object to eth-keyring-controller's extToSign array, which is monitored by the UI modal.js base component.
  4. Since there is a new extToSign entry ui/app/components/modals/modal.js starts the external-sign-modal.js window, and displays QR code, and requests signature.
  5. Once signature received from user, the tx is segned normall, and handled in a standard way by Metamask.

To allow for external signatures, eth-keyring-controller package had to be updated, and eth-external-account-keyring package was created. So in the package.json my updated ones are the sources.
Eth-external-account-keyring is available at:
https://github.com/r001/eth-external-account-keyring

Install instructions for reference implementation of encoding/decoding QR code can be found at
https://github.com/r001/qr-encoding/blob/master/INSTALL.md

Modified signer that can create appropriate signature can be found at :
https://github.com/r001/dapptools/tree/ethsign-signature
To create signature use ethsign tx --signature-only <tx options>...

@r001 r001 requested review from danjm and whymarrh as code owners February 12, 2019 18:18
@r001
Copy link
Author

r001 commented Feb 12, 2019

@tmashuang This is the followup of PR #6007 . Sorry I could not reopen the issue.
I have implemented Parity Signer and ERC67 style of signing too. I will be doing some bugfixes that you have suggested and then we are ready to merge it I hope.

@r001
Copy link
Author

r001 commented Feb 12, 2019

@caffeinum I have implemented both Parity Signer, and ERC67 types of signing for external accounts in Metamask.

@r001 r001 closed this Feb 14, 2019
@r001 r001 deleted the external-accounts branch February 14, 2019 00:23
@r001 r001 reopened this Feb 14, 2019
@r001 r001 force-pushed the external-accounts branch from ac2d7e5 to 7c37b8b Compare February 14, 2019 00:27
@r001 r001 requested a review from frankiebee as a code owner February 14, 2019 00:27
@r001
Copy link
Author

r001 commented Feb 14, 2019

I would need some help with css styling: "Sign with" dropdown should be placed over "signature" input. The problem is that the "sign with" puts his dropdown under "signaturee" input because of some weird zIndex stuff. Also the dropdowns should be of equal size. And different screensizes should be applied to QR code size. The QR code should be sized as large as possible.

@r001
Copy link
Author

r001 commented Feb 14, 2019

Some additional tests would be also nice to have.

@r001 r001 closed this Mar 8, 2019
@caffeinum
Copy link

@r001 why'd you close?

@r001
Copy link
Author

r001 commented Mar 10, 2019

@caffeinum you can find the new one here: PR #6267
BTW I keep screwing with the rebasing with git: if I don't rebase, then tests fail, if I do rebase and recreate the whole branch it gets closed here, if I force push then only the very last commit will contain the rebase the previous ones do not, this way you can't really roll back, only if you do another rebase yourself. But now I will just use force push, and the PR #6267 wont't be closed until it is either rejected or accepted by the team.

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.

3 participants