Support QR hardware

[soralit]

Add QR hardware support

Description

• CHANGED:

  • Change signTypedMessage method to support QR hardware.

• ADDED:

  • Add QR hardware Keyring
  • Add QR hardware's interactive methods, including read keyring, submit signature and related methods.

Checklist

• Tests are included if applicable
• Any added code is fully documented

[soralit]

Hi @gantunesr , I will be on holidays next week, so I just raised this PR (which is not fully tested) to talk about the API definition and code style. Please have a look when you get time.

Besides, I have a question about signTypedMessage: Why not just use the signTypedMessage method in ETHKeyringController but exporting the account and sign messages in a raw way? I think all the keyring types have implemented signTypedData method.

[gantunesr]

Hey @soralit, to answer your question regarding signTypedMessage and the other devs in the team can correct me if needed. Looks like the reason was mainly about the standardization around the method and the controllers. I wasn't here at that moment, so I'm not really sure about the details.

[mcmire]

I took a look at this PR. I am a bit wary of these changes because it feels like it expands the responsibilities of KeyringController in a way that doesn't feel quite right. After this change, there will be some methods that are designed to work with our Keyring objects, and there will be some methods that are designed to work with yours.

That said, we know that our existing keyring code could use work. In this case, it doesn't help that our keyring modules are untyped. We plan on addressing these and other concerns in future updates, so I'm okay with this now with the knowledge that these changes are fairly self-contained and that we have plans for improvement later.

With that in mind I made some suggestions for how we could improve this code.

[soralit]

Hi @mcmire , thanks for your review and comments. I will look into your suggestions and update the code ASAP.

[mcmire]

Just a few more code improvement suggestions.

Crucially I noticed that there don't seem to be any unit tests for the new methods you've added to the controller. Would you be willing to add that to this file?

[soralit]

Hi @mcmire , thanks for your suggestions. I will append a commit to add unit tests later.

[mcmire]

A few more things. Also — would you mind adding return types to each method? I know TypeScript can fill these in automatically but I think it's better if we are explicit as 1) it forces the author to think about what the return type should be and 2) it removes one step for the reader to understand what the expected return type of the methods are.

[soralit]

Hi @mcmire , sorry for the late reply. I'm working on the bug fixing on mobile side these days. I just resolved the code issues and add types for new methods. I will append the unit tests tomorrow.

[soralit]

Hi @mcmire , just update the unit tests and rebased the code, please have a review when you have time.

[mcmire]

Just a few more minor things. Everything generally looks good to me otherwise (although I'll ping some other people to get some second opinions).

[gantunesr]

I'm leaving some more comments to get the opinion of the other devs related to this development.

[gantunesr]

@soralit looks like your PR is not meeting the code coverage requirement

Jest: "global" coverage threshold for functions (96%) not met: 95.94%

[mcmire]

@gantunesr The existing coverage is very close to the threshold, so any small changes could send it over the edge. I agree we should double-check the test coverage but I have a PR here that should make this better: #736.

[mcmire]

Thanks for putting up with all of our comments! I'm quite happy with this. Nice work!

[gantunesr]

🚀 🚀 🚀 🚀