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

device: proper handling of user input #5055

Merged
merged 1 commit into from Jan 28, 2019

Conversation

Projects
None yet
4 participants
@selsta
Copy link
Contributor

commented Jan 9, 2019

(1) If the user denies something on the Ledger, a proper error message is now shown.
(2) Ledger doesn't time out anymore while waiting on user input.
(3) Lower the timeout to 2 seconds, this is enough for normal Ledger <-> System communication.

ping @xiphon, you were unhappy about my change to remove all the timeouts. Is this better?

@selsta selsta force-pushed the selsta:ledger-hid-improvements branch from cfa5fcc to 1a7be9c Jan 9, 2019

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

The idea is interesting, the location of the calls to user input is unfortunate but that's fixed by the hardware. I think it's a fair compromise. Did you try it with a very large tx to make sure 2 seconds is actually enough ?

@selsta

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2019

I tried sweep_all with my main wallet and it even worked fine with timeout set to 0.5 seconds, but you just reminded me that I forgot to handle the “export view key” user input.

@selsta selsta force-pushed the selsta:ledger-hid-improvements branch from 1a7be9c to f708da0 Jan 9, 2019

@selsta

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2019

Updated for the view key user input.

@cslashm

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

Seems OK for me. Good Idea

Show resolved Hide resolved src/device/device_ledger.cpp Outdated
Show resolved Hide resolved src/device/device_ledger.cpp Outdated
Show resolved Hide resolved src/device/device_ledger.cpp
Show resolved Hide resolved src/device/device_ledger.cpp Outdated
device: proper handling of user input
(1) If the user denies something on the Ledger,
    a proper error message is now shown.
(2) Ledger doesn't time out anymore while waiting
    on user input.
(3) Lower the timeout to 2 seconds, this is enough for
    normal Ledger <-> System communication.

@selsta selsta force-pushed the selsta:ledger-hid-improvements branch from f708da0 to 6c060e6 Jan 9, 2019

@selsta

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2019

@moneromooo-monero Added defines, is it better this way? The code is full of magic numbers, not sure if it’s worth to add defines now.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

It'd have been nice, someone who wants do spend the time on it can do it later :)

@cslashm

cslashm approved these changes Jan 9, 2019

@fluffypony
Copy link
Collaborator

left a comment

Reviewed

@fluffypony fluffypony merged commit 6c060e6 into monero-project:master Jan 28, 2019

6 of 10 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
buildbot/monero-static-osx-10.13 Build done.
Details
buildbot/monero-static-win32 Build done.
Details
buildbot/monero-static-win64 Build done.
Details
buildbot/monero-linux-armv7 Build done.
Details
buildbot/monero-linux-armv8 Build done.
Details
buildbot/monero-static-osx-10.11 Build done.
Details
buildbot/monero-static-osx-10.12 Build done.
Details
buildbot/monero-static-ubuntu-amd64 Build done.
Details
buildbot/monero-static-ubuntu-i686 Build done.
Details

fluffypony added a commit that referenced this pull request Jan 28, 2019

Merge pull request #5055
6c060e6 device: proper handling of user input (selsta)

@selsta selsta deleted the selsta:ledger-hid-improvements branch Feb 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.