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

Trezor T UI improvements #102

Open
almindor opened this issue Jun 9, 2018 · 9 comments
Open

Trezor T UI improvements #102

almindor opened this issue Jun 9, 2018 · 9 comments
Assignees

Comments

@almindor
Copy link
Owner

almindor commented Jun 9, 2018

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

@gkaindl
Copy link
Contributor

gkaindl commented Jun 11, 2018

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! :-)

@almindor
Copy link
Owner Author

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
Copy link
Owner Author

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.

@gkaindl
Copy link
Contributor

gkaindl commented Jun 11, 2018

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, 38d3dd3 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.

@almindor
Copy link
Owner Author

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
Copy link
Owner Author

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.

@gkaindl
Copy link
Contributor

gkaindl commented Jun 15, 2018

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

@almindor
Copy link
Owner Author

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.

@gkaindl
Copy link
Contributor

gkaindl commented Jun 17, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants