Feature: Bidirectional QR code accounts

[r001]

Bidirectional QR accounts feature allows Metamask to track accounts 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 bidirectional qr accounts (requestend in ISSUE #5640). This means that after applying this pull request in the "import account" menu there will be an "QR" tab that enables importing multiple ethereum addresses. 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-bidirectional-qr-keyring, the keyrings signTransaction() function is called.
3. Sign transaction adds an object to bidirectionalQrSignables array, which is monitored by the UI modal.js base component.
4. Since there is a new bidirectionalQrSignables entry modal.js starts the bidirectional-qr-sign-modal.js window, and displays QR code, and requests signature.
5. Once signature received from user, it is sent to background, and the tx is signed using the signature provided and handled in a normal way by Metamask.

There are four different signers this implementation supports:

1. Parity signer.
2. Ellipal signer.
3. Custom ERC67 signer.
4. Airsign signer.

Since signing has a defined interface, further signers can be added easily.

[r001]

@whymarrh @danjm @frankiebee Hi, This PR was originally PR
#5903 and then PR #6007. I have been maintaining this thread since dec 9 2018. I know It is a big change in the system, but I would appreciate some feedback from you about it, because I would not invest more time in it if you don't want to include it in develop. And I would be more than happy to work on it if you like it.

[whymarrh]

@tmashuang had reviewed a prior version of this PR, I added him here

[whymarrh]

Also, I think @tmashuang's comments in #6007 still stand, can you link to the PRs you've created for the dependent packages? (i.e. eth-keyring-controller)

[r001]

Also, I think @tmashuang's comments in #6007 still stand, can you link to the PRs you've created for the dependent packages? (i.e. eth-keyring-controller)

@tmashuang and @whymarrh
Sure, here they are:

ETH-keyring-controller pr:
MetaMask/KeyringController#29

The entirely new package, not a pr: eth-external-account-keyring:
https://github.com/r001/eth-external-account-keyring.git

BTW my apologies for making your lives harder by closing the PR all the time. 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 will not, this way you can't really roll back, only if you do another rebase yourself. But from now on I will just use force push, and this way you can keep track of the PR more easily.

[kumavis]

hello @r001, lots of good work here : )

I really like the airgapped bidirectional QR code style. As a side project, I'm working on adding support for the ellipal hardware wallet which works very similarly to yours. I worked out their current uri schema here https://github.com/kumavis/eth-ellipal-util

Could you clarify your external signing target? seems like the encoding and URI schema are specific to AirSign. This is fine, but we should be explicit and call this an AirSign account and not an "external account". If you are interested in your URI schema being generic and standardized, I would recommend opening an EIP/ERC.

[kumavis]

maybe we should land a refactor for the QR code widget outside of this PR that makes it more generically useful by other parts of the app

[kumavis]

i also look forward to writing something like

class EllipalSigner extends BidrectionalQrSigner {
  decodeReadAddressUri: () => { ... },
  encodeSignRequestUri: () => { ... },
  decodeSignedTxUri: () => { ... },
  ...
}

where the "signer" refers to a collection of views + routes + keyrings. one day!

[kumavis]

pulled out from a review thread:

I think using the keyring to trigger the qr code reader is not a good idea in this case. I think a better way to approach this would be:

• the tx confirmation flow checks if the keyring type for the account has a special ui signing flow
• if it does that flow is routed to
• BidirectionalQrCode signers use this flow to perform and relay the signature

[r001]

I have answeret this above.

[r001]

hello @r001, lots of good work here : )

I really like the airgapped bidirectional QR code style. As a side project, I'm working on adding support for the ellipal hardware wallet which works very similarly to yours. I worked out their current uri schema here https://github.com/kumavis/eth-ellipal-util

Could you clarify your external signing target? seems like the encoding and URI schema are specific to AirSign. This is fine, but we should be explicit and call this an AirSign account and not an "external account". If you are interested in your URI schema being generic and standardized, I would recommend opening an EIP/ERC.

I have refactored the code to be able to implement encoding/decoding in an easy way. I have implemented the Ellippal encoder/decoder into the project.

[r001]

i also look forward to writing something like

class EllipalSigner extends BidrectionalQrSigner {
  decodeReadAddressUri: () => { ... },
  encodeSignRequestUri: () => { ... },
  decodeSignedTxUri: () => { ... },
  ...
}

where the "signer" refers to a collection of views + routes + keyrings. one day!

Done.

[r001]

i also look forward to writing something like

class EllipalSigner extends BidrectionalQrSigner {
  decodeReadAddressUri: () => { ... },
  encodeSignRequestUri: () => { ... },
  decodeSignedTxUri: () => { ... },
  ...
}

where the "signer" refers to a collection of views + routes + keyrings. one day!

Now it is possible, but you don't have to do that. I have implemented Ellipal support Please check it for me, just to make sure I got everything right in the code.

[kumavis]

And I also have pretty bad feelings about making so substantial changes since I've been doing this work on my free time, I did it because I am using bidirectional QR to interact with crypto and it seemed to be fun to make it work with Metamask. I have put quite lot of work into this, but to rewrite the whole code again.... just seems to be too much for me.

yeah, fair. let me see if we have some bounty funds available for this. I think its a really important feature.

[kumavis]

Signer, ParitySigner, Erc67Signer, Ellipal, AirSign

nice!

[kumavis]

I want to surface the remaining TODOs on this PR. @r001 do you have any you're still working on or is it just feedback stuff on our end?

[r001]

I want to surface the remaining TODOs on this PR. @r001 do you have any you're still working on or is it just feedback stuff on our end?

Well there are no remaining TODOs. Except if we are doing the UI flow model you have suggested before. Current solution keeps current code intact. None of the signing classes had to be changed, none of the UI side code had to be updated either (only of course QrScanner) and only one in TransactionController because MetaMask does not have a state to handle rejected signature. If you go the UI way, we have to mess up all the signing functionality on the background, besides having to write all the UI side code. I think it will be about the same amount of code, and same amount of mess at the end if we go into details. You can see the power of current solution when you hit 'cancel' to a pending tx (that was for an external account): the signing modal comes up, even if I had no idea about a feature like this when I wrote the code. Anything that will want to sign will pop the signing modal. But no more reasoning. Of course if the decision is still to go the UI way, lets do that.

[Gudahtt]

The CLA bot should hopefully be fixed now? I just updated it

[caffeinum]

Looks like this protocol offers very similar functionality and is supported in some ways by Metamask? Maybe we can at least reuse some code from there: https://walletconnect.org/

[aaronisme]

Hi all, I am from the @CoboVault team and we like the Bidirectional QR code accounts feature idea. Currently, a lot of hardware wallets can take the air-gapped way to transmit data(QR Code) and I think it can hugely reduce the attack surface and bring more security to the whole community. Also - QR code has much fewer compatibility issues compared to USB solutions.

But after I go through this PR, I think there are some limitations. I will list them and we can have more discussion:

1.Lack of multi-QR code support. The current implementation is lacking multi-QR Code support which will make this feature much less usable. With Defi booming, the transaction data became much larger. And it is almost impossible to put all the unsigned data into one QR code. So multi-QR code support is necessary.

2.Limitation of current QR-scanner library @zxing/library. The current @zxing library will stop scanning once it gets a frame of QR Codes, which is not suitable for multi-QR code.

3.Current account creation user flow seems strange. Users need to paste the address into the input box. Why not directly use the QR-scanner to do that? Or at least providing this function. Users can either paste the address or scan the QR code.

4.Extended public key or address? The current implementation is to provide the address to create an account. But how about just providing an extended public key and derivation path to MetaMask. With that feature, users can get a series of accounts/addresses on MetaMask. It will be much more user friendly. But this also introduces more changes to the current product logic and I think this is an open question.

5.Data encoding standard of the QR code(s). Currently, there seems no standard for data encoding on the QR Codes, different hardware wallets use their own way. I think an EIP of QR code data encoding is essential as QR code will be more and more adopted as a better solution compared to USB connection for hardware wallets.

Our team created a proof of concept project to resolve these limitations. And we are very happy to help improve this PR based on our experience.
Here is our code repo: https://github.com/CoboVault/metamask-extension
Here is the demo: https://twitter.com/CoboVault/status/1354027107098140672

[AndreasGassmann]

At AirGap, we would also be very interested in airgapped wallet support via QR codes. We built a proof-of-concept for this more than 2 years ago and the signing part worked very well, but at the time there was not much interest for that feature and we discontinued our extension. This is a small demo, the flow would basically be the same, but instead of our own "AirGap Extension", it would now be MetaMask handling the transaction prepare: https://www.youtube.com/watch?v=q6gS0MdpUDg

@aaronisme Thanks for working on this. I have a couple of notes regarding your points:

1. I completely agree with this. Multi-Page is a must.
2. We use a version of the zxing library in our apps and multi-page QR code scanning is possible. We use the angular version (@zxing/ngx-scanner).
3. I agree, both pasting and scanning an address should be possible.
4. I'm not sure I agree with this. I might be wrong, but I don't think extended public keys are used often in Ethereum. Most users have only one account, or if they choose to have multiple accounts with different derivation paths, they usually do it for a specific reason (eg. privacy). So I would personally not like if my extended public key would be shared. But maybe there are other use cases I didn't think of?
5. I also agree here. The "bc-ur" standard for multi-page QRs by BlockchainCommons is starting to be adopted by a number of wallets in the Bitcoin community. I don't know how the EIP process works, but it would be great if the bc-ur standard could be used as a foundation for that EIP (I think they are planning to submit a BIP for it as well). Cobo Vault and the "MetaMask Proof of Concept" are currently using bc-ur v1 and if I recall correctly, you mentioned that you are planning to update to bc-ur v2 soon (referring to this comment: Moving to UR2.0 BlockchainCommons/Gordian-Developer-Community#26 (comment)). Just FYI, someone is already working on a JS implementation of the latest specs: https://github.com/Apocentre/bc-ur

It would be great if this feature could find its way into the MetaMask extension.

[aaronisme]

At AirGap, we would also be very interested in airgapped wallet support via QR codes. We built a proof-of-concept for this more than 2 years ago and the signing part worked very well, but at the time there was not much interest for that feature and we discontinued our extension. This is a small demo, the flow would basically be the same, but instead of our own "AirGap Extension", it would now be MetaMask handling the transaction prepare: https://www.youtube.com/watch?v=q6gS0MdpUDg

@aaronisme Thanks for working on this. I have a couple of notes regarding your points:

1. I completely agree with this. Multi-Page is a must.
2. We use a version of the zxing library in our apps and multi-page QR code scanning is possible. We use the angular version (@zxing/ngx-scanner).
3. I agree, both pasting and scanning an address should be possible.
4. I'm not sure I agree with this. I might be wrong, but I don't think extended public keys are used often in Ethereum. Most users have only one account, or if they choose to have multiple accounts with different derivation paths, they usually do it for a specific reason (eg. privacy). So I would personally not like if my extended public key would be shared. But maybe there are other use cases I didn't think of?
5. I also agree here. The "bc-ur" standard for multi-page QRs by BlockchainCommons is starting to be adopted by a number of wallets in the Bitcoin community. I don't know how the EIP process works, but it would be great if the bc-ur standard could be used as a foundation for that EIP (I think they are planning to submit a BIP for it as well). Cobo Vault and the "MetaMask Proof of Concept" are currently using bc-ur v1 and if I recall correctly, you mentioned that you are planning to update to bc-ur v2 soon (referring to this comment: BlockchainCommons/Airgapped-Wallet-Community#26 (comment)). Just FYI, someone is already working on a JS implementation of the latest specs: https://github.com/Apocentre/bc-ur

It would be great if this feature could find its way into the MetaMask extension.

@AndreasGassmann Thanks for your input.
For 4 from my point of view, I think currently a lot of Defi farmers will have a lot of addresses (accounts), so the extended public key will make things easier for them.
For 5 I think an EIP of it will be much better, whether to use the bc-ur is an open question to discuss. Since currently there are other encoding formats like UOS(https://github.com/maciejhirsz/uos).

[Tbaut]

Thanks for this PR. Really looking forward to it.

For 5 I think an EIP of it will be much better, whether to use the bc-ur is an open question to discuss. Since currently there are other encoding formats like UOS(https://github.com/maciejhirsz/uos).

Agreed, Parity Signer is using UOS, and we can now add Stylo to the list. It's a fork of Signer that I launched recently. These work well for Ethereum with MyCrypto Desktop app, and for Polkadot/Kusama with the Polkadot-js apps and extension.

[AndreasGassmann]

Is there a comparison between both bc-ur and UOS? Regarding performance, features and available tooling?

At AirGap we have been using our own custom multi-qr implementation for a couple of years now. It is similar to UOS in that it allows you to split the data up into multiple parts if necessary, and have the other party combine the parts to get the full payload.

However, there is a big problem with this. If one QR is missed, the user has to wait for a "full roundtrip" again until the missed QR is shown again. This is a problem for larger payloads. We have a use-case where we do a batch transaction with our apps that results in about 200 QR codes. Scanning those consistently can be very tricky, depending on the device. With iPhones the success rate is basically 100%, but especially older Android phones often drop QRs, making it a cumbersome process. The bc-ur standard (we were not involved in designing that standard) addresses this by randomising the parts that are contained in the QRs after the initial set.

In AirGap, we will most likely add support for bc-ur because of bitcoin and UOS because of polkadot. So we will support either standard and it doesn't really matter which route MetaMask / Ethereum is going. However, I do feel like bc-ur does have some clear advantages.

I could even see a combination of the two. We could use bc-ur as the encoding format for QR codes, but the underlying structure of the data could still be as defined in UOS. Only the multipart section would have to be removed and the payload could be passed into bc-ur, which handles the fragmentation of the data into QR codes.

[soralit]

Is there a comparison between both bc-ur and UOS? Regarding performance, features and available tooling?

At AirGap we have been using our own custom multi-qr implementation for a couple of years now. It is similar to UOS in that it allows you to split the data up into multiple parts if necessary, and have the other party combine the parts to get the full payload.

However, there is a big problem with this. If one QR is missed, the user has to wait for a "full roundtrip" again until the missed QR is shown again. This is a problem for larger payloads. We have a use-case where we do a batch transaction with our apps that results in about 200 QR codes. Scanning those consistently can be very tricky, depending on the device. With iPhones the success rate is basically 100%, but especially older Android phones often drop QRs, making it a cumbersome process. The bc-ur standard (we were not involved in designing that standard) addresses this by randomising the parts that are contained in the QRs after the initial set.

In AirGap, we will most likely add support for bc-ur because of bitcoin and UOS because of polkadot. So we will support either standard and it doesn't really matter which route MetaMask / Ethereum is going. However, I do feel like bc-ur does have some clear advantages.

I could even see a combination of the two. We could use bc-ur as the encoding format for QR codes, but the underlying structure of the data could still be as defined in UOS. Only the multipart section would have to be removed and the payload could be passed into bc-ur, which handles the fragmentation of the data into QR codes.

Hi @AndreasGassmann and all people interested in this topic, I'm an Engineer from Cobo Vault team. Would you like to step to this discussion to have a further discussion? For the talk gradually running out of scope of this PR.

[aaronisme]

Is there a comparison between both bc-ur and UOS? Regarding performance, features and available tooling?
At AirGap we have been using our own custom multi-qr implementation for a couple of years now. It is similar to UOS in that it allows you to split the data up into multiple parts if necessary, and have the other party combine the parts to get the full payload.
However, there is a big problem with this. If one QR is missed, the user has to wait for a "full roundtrip" again until the missed QR is shown again. This is a problem for larger payloads. We have a use-case where we do a batch transaction with our apps that results in about 200 QR codes. Scanning those consistently can be very tricky, depending on the device. With iPhones the success rate is basically 100%, but especially older Android phones often drop QRs, making it a cumbersome process. The bc-ur standard (we were not involved in designing that standard) addresses this by randomising the parts that are contained in the QRs after the initial set.
In AirGap, we will most likely add support for bc-ur because of bitcoin and UOS because of polkadot. So we will support either standard and it doesn't really matter which route MetaMask / Ethereum is going. However, I do feel like bc-ur does have some clear advantages.
I could even see a combination of the two. We could use bc-ur as the encoding format for QR codes, but the underlying structure of the data could still be as defined in UOS. Only the multipart section would have to be removed and the payload could be passed into bc-ur, which handles the fragmentation of the data into QR codes.

Hi @AndreasGassmann and all people interested in this topic, I'm an Engineer from Cobo Vault team. Would you like to step to this discussion to have a further discussion? For the talk gradually running out of scope of this PR.

Hi @AndreasGassmann, @Tbaut
@soralit is our Frontend Lead, and I think we can have a more detailed discussion on the channel?

[darkwing]

Thank you for your work on this @r001 ! We've been testing this out and found that transactions may fail if the data is too large. We'll be moving toward an animating QR solution. Thank you again!