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

Ledger upgrade feature #1215

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pythcoiner
Copy link
Collaborator

@pythcoiner pythcoiner commented Jul 24, 2024

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:

image

After clicking on Upgrade liana close the ledger bitcoin app and start upgrade:

image

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:

image

This feature is implemented in several locations:

  • Installer: Descriptor creation step
  • Installer: Descriptor registration step
  • App: PSBT signing step
  • App: Settings for descriptore registration step
  • App: Receive for address verification step

Note: This PR rely on changes made in ledger_manager.

closes #1135

@nondiremanuel nondiremanuel added this to the v7 - Liana milestone Jul 24, 2024
@pythcoiner pythcoiner force-pushed the issue_1135 branch 22 times, most recently from da653de to 4eacd15 Compare August 2, 2024 04:47
@pythcoiner
Copy link
Collaborator Author

@darosior i actually display the upgrade progress logs in a 'scrollable' console, what do you think?

image

(we can add a progress bar after 'Upgrading:' also)

@nondiremanuel
Copy link
Collaborator

nondiremanuel commented Aug 2, 2024

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.

@darosior
Copy link
Member

darosior commented Aug 2, 2024

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".

@pythcoiner
Copy link
Collaborator Author

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:

  • quit the bitcoin app in order to start upgrading
  • accept app listing
  • accept to upgrade the app

those are 3 actions the user had to perform.

@pythcoiner pythcoiner force-pushed the issue_1135 branch 2 times, most recently from 5e3c820 to 1a5104e Compare August 6, 2024 02:23
@pythcoiner pythcoiner force-pushed the issue_1135 branch 5 times, most recently from da88d7c to ac05ac7 Compare August 6, 2024 11:55
@pythcoiner
Copy link
Collaborator Author

note to myself: seems we can get the install progress see

@pythcoiner pythcoiner force-pushed the issue_1135 branch 8 times, most recently from ab195ea to 4e12e1b Compare August 19, 2024 02:53
@pythcoiner pythcoiner marked this pull request as ready for review August 19, 2024 03:14
@pythcoiner pythcoiner changed the title [WIP] Ledger upgrade feature Ledger upgrade feature Aug 19, 2024
gui/src/hw.rs Show resolved Hide resolved
hws: Vec<HardwareWallet>,
still: Vec<String>,
hws_upgrade: Vec<HardwareWallet>,
still_upgrade: Vec<String>,
Copy link
Member

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

Copy link
Collaborator Author

@pythcoiner pythcoiner Aug 19, 2024

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()

@pythcoiner pythcoiner force-pushed the issue_1135 branch 2 times, most recently from e2dd5fa to e18c971 Compare August 20, 2024 12:57
@nondiremanuel nondiremanuel added the Nice to have If it's not completed in time for the current version, it can be postponed label Aug 28, 2024
@nondiremanuel
Copy link
Collaborator

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.

@nondiremanuel nondiremanuel modified the milestones: v7 - Liana, v8 - Liana Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Nice to have If it's not completed in time for the current version, it can be postponed
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

Upgrade Bitcoin app version if above minimum but below Taproot minimum
4 participants