-
Notifications
You must be signed in to change notification settings - Fork 29
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
Add prompt_pin and send_pin HWI commands #106
base: master
Are you sure you want to change the base?
Conversation
Concept ACK. Please add a comment if you were able to manually test this with a physical device. |
ba5a0c8
to
5bdb37f
Compare
Thanks @notmandatory, I've added an ignore note. I have also added the prompt PIN command. Also something important to note, in the test, I had to use get_client and specify device path due to this bug in HWI itself: bitcoin-core/HWI#636 The problem is that currently, due to how we process errors in enumerate, it will not be possible to get the path of the Trezor One using Rust HWI, which makes this a lot less useful. This is because when the enumerate encounters an error, it won't return the rest of the device data like its path, but instead will only return an HWI Error. This prevents us from learning the path, and being able to use it in get client before the device is unlocked. So what can be done is to instead of returning an error, return the fields we can, and add an error field to HWIDevice itself. If that solution makes sense let me know and I could implement it. It will require some chnages to the data structures though, which is why I want to know first if this would be a good way to do it. |
5bdb37f
to
1bdbcbc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 1bdbcbc
@ben-kaufman do you want to get #105 in before we do a new release for this one? |
@notmandatory sure, I think I can resolve the conflicts this week. It's mainly just getting the new signer component to use the new structure. |
@notmandatory I believe after merging this, we should also handle the issue that prevents an effective use of it by changing how device errors are handled on enumerate. Is there a good place to discuss the best way to handle these errors, or should I just submit a suggested implementation PR? |
Add support for HWI prompt_pin and send_pin commands (for Trezor One).
Not sure there's a good way to automate testing for that with an emulator though.
Tested on a physical Trezor One.