etherwall icon indicating copy to clipboard operation
etherwall copied to clipboard

Trezor T UI improvements

Open almindor opened this issue 7 years ago • 9 comments
trafficstars

Trezor T is now "supported" but needs more improvements before a release is done. Creating this as a separate issue.

Main thing is UI for handling of the workflow with passwords input on the device itself.

Continuation of #99

almindor avatar Jun 09 '18 20:06 almindor

Yes, exactly. I don't have any experience with Qt though, which makes me way less effective here, though :-(

Anyway, the main differences between a Trezor One and Trezor T when it comes to the workflow are those:

  • If you haven't "unlocked" (e.g. entered the PIN) your Trezor T yet, there is no way to even detect its presence, as it only enables the USB endpoint once it has been unlocked. Conversely, the T will never ask for a PIN via the USB api (you have to enter it on-device at all times).
  • For the passphrase, the Passphrase Request message sent by the Trezor T will set the on_device flag, depending on whether the user chose to enter the passphrase on the device or normally via the app. If the latter is chosen, the workflow to actually enter the passphrase is the same as on the One. If the passphrase is entered on-device, a PassphraseAck should be sent without any passphrase set as a payload.
  • Once the passphrase is entered, the Trezor will send the new PassphraseStateRequest, which the client needs to reply to with a PassphraseStateAck. It looks as if this will soon be the same behavior on the One, once trezor-core gets ported to it, only that the PassphraseStateAck is more or less meaningless there and just tells the client that the Trezor is now ready.

Most importantly for this issue, if the PassphraseRequest is received with on_device set, it'd be ideal if Etherwall displayed a message like "Please enter your passphrase on your Trezor".

Btw, I've also joined your gitter to make discussion easier! Thanks for the invite! :-)

gkaindl avatar Jun 11 '18 07:06 gkaindl

Ok so to sum it up I think:

  1. we don't need to change anything for the PinMatrixRequest since the model T simply doesn't send it in
  2. if we get a PassphraseRequest with on_device being set we should just display a badge (the blue timeouting message) to prompt user to enter password on device and wait for a PassphraseStateRequest and just Ack back on that. Once that is done we continue as if the password was accepted and continue with the signing.

almindor avatar Jun 11 '18 15:06 almindor

Added the badge logic, can you test to see if you get the message displayed? You should just get a Input your password on the TREZOR device "badge" to show up if the password request is supposed to be filled on device.

almindor avatar Jun 11 '18 15:06 almindor

I just tested it! It works – There's a badge shown while entering the passphrase on-device!

However, just before being prompted by the Trezor whether you want to enter the passphrase on-device or on the host, there's another badge shown briefly that says something like "Please complete the action on your Trezor: undefined". Maybe it's related to the ButtonRequest/ButtonAck handshake that happens before entering the passphrase? I'm not sure yet (but I didn't have much time today to investigate).

Also, 38d3dd396c1499e3721c184efc32efa95b64b6af seems to have broken the build on MacOS, since Q_OS_UNIX seems to be defined there as well. I fixed it by using #if defined(Q_OS_UNIX) && !defined(Q_OS_MACX) instead, but then DOWNLOAD_BASE_PATH is not defined in nodemanager.cpp – I "fixed" this by just adding an empty string instead to test now.

I can send you a PR for this if you want, but if this is all work in progress, it might not be useful right now.

gkaindl avatar Jun 11 '18 20:06 gkaindl

Hey thanks for testing. I'll fix the UNIX defined. I wanted EW to be compilable on BSDs etc. Will have to exclude mac from the list.

Will try and find out what the action cause could be and get back. Ping here if you find out more there.

almindor avatar Jun 11 '18 21:06 almindor

Is there any chance you can catch a screenshot of the badge with the unknown text? I can't find any source where that kind of description would be given.

almindor avatar Jun 13 '18 15:06 almindor

Sorry for the late reply! Here's the screenshot: It happens before the prompt to choose where to enter the passphrase is shown. It's probably related to the ButtonRequest/ButtonAck messages?

screen shot 2018-06-15 at 13 38 49

gkaindl avatar Jun 15 '18 11:06 gkaindl

I see, it's a missing ButtonRequest_PassphraseType = 14; enum value.

I suppose you need to choose if you want the password presented on trezor or on the device in this step?

I added the handler in master.

almindor avatar Jun 15 '18 14:06 almindor

Yes, exactly, that was it! The workflow is working correctly now.

gkaindl avatar Jun 17 '18 17:06 gkaindl