-
Notifications
You must be signed in to change notification settings - Fork 57
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
Ledger upgrade feature #1215
base: master
Are you sure you want to change the base?
Ledger upgrade feature #1215
Conversation
da653de
to
4eacd15
Compare
@darosior i actually display the upgrade progress logs in a 'scrollable' console, what do you think? (we can add a progress bar after 'Upgrading:' also) |
I'm not sure the logs are adding much value to the user. I personally think it clutters the UI for little or no value to the user, which generally only cares about the progress and the success / failure statuses. |
Yeah. Do you think some are necessary to show the users? I can think of "please confirm on your device" but don't have others on the top of my mind. If it's only this one we could even just have this message: "Upgrading, please check your device". |
Several are necessary,as it can takes some time to establish the connexion w/ the ledger HSM (w/ bad connection), and imho it can feel very confusing as sometimes nothing changing on device screen. we should at least display:
those are 3 actions the user had to perform. |
5e3c820
to
1a5104e
Compare
da88d7c
to
ac05ac7
Compare
note to myself: seems we can get the install progress see |
ab195ea
to
4e12e1b
Compare
hws: Vec<HardwareWallet>, | ||
still: Vec<String>, | ||
hws_upgrade: Vec<HardwareWallet>, | ||
still_upgrade: Vec<String>, |
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.
Previously we did not need the still field and now we have
still and still_upgrade. Can we just pass one unique still: &mut Vec<String>
that we fill with ids from "ready" device and upgrading
to the poll_ledger etc
and use it to pass device new connection has they are either ready or in upgrade state ?
I may miss a subtility
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.
still_upgrade
& hws_upgrade
need NOT to be reset at the end of refresh()
: hws
& still
are need to be reset, they are not stricly needed in the state, but i add them for convenience & readability as we need to pass only state
to all poll_xxx()
and handle_xxx_device()
e2dd5fa
to
e18c971
Compare
Moving this and the relative issue to v8 since as per team discussion we won't be able to properly review and test it given the close deadline. |
This PR introduce a feature to upgrade the ledger app if an app already supporting miniscript but not taproot installed on device.
If upgrade possible this is displayed to user:
After clicking on
Upgrade
liana close the ledger bitcoin app and start upgrade:After upgrading, the user should validate app opening on device.
Note: There is some ledger firmware that allows to install app that support miniscript but do not allow install app that support taproot (i hit this whith Nano S+ w/ firmware 1.1.0 the latest upgradable app is 2.1.3), to handle this case we perform a version check after upgrade, if the latest app does not support taproot (< 2.2.0) we ask user to upgrade his firmware with Ledger Live:
This feature is implemented in several locations:
Note: This PR rely on changes made in
ledger_manager
.closes #1135