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

Upgrade Bitcoin app version if above minimum but below Taproot minimum #1135

Open
darosior opened this issue Jun 24, 2024 · 17 comments · May be fixed by #1215
Open

Upgrade Bitcoin app version if above minimum but below Taproot minimum #1135

darosior opened this issue Jun 24, 2024 · 17 comments · May be fixed by #1215
Assignees
Labels
Feature New feature or functionality. GUI gui related Nice to have If it's not completed in time for the current version, it can be postponed Signing devices Anything related to signing devices ("hardware wallets")

Comments

@darosior
Copy link
Member

Enough people have been testing https://github.com/darosior/ledger_installer that i feel comfortable starting to integrate pieces into Liana. A good small first step would be to make it possible for Ledger users with a Bitcoin app whose version is below the version which introduced Taproot support to upgrade to the latest Bitcoin app. Note however those users must already have a Liana-compatible app (above our minimum supported version) otherwise the update of the app would also necessitates a firmware upgrade, which is not implemented at the moment (and not the best candidate for a first step).

@darosior darosior added GUI gui related Enhancement Improving an existing functionality Signing devices Anything related to signing devices ("hardware wallets") Feature New feature or functionality. and removed Enhancement Improving an existing functionality labels Jun 24, 2024
@pythcoiner
Copy link
Collaborator

how do you see this? call ledger_installer gui as an external process?

@darosior
Copy link
Member Author

No, i think the GUI should just have a button letting the user upgrade in case they choose to use Taproot but their device's Bitcoin app is too old. This would just call the library's function directly.

@pythcoiner
Copy link
Collaborator

open a modal to show hint (accept on device) and progress (bubling sardine?)?

@nondiremanuel nondiremanuel added this to the v7 - Liana milestone Jul 3, 2024
@nondiremanuel nondiremanuel added the Nice to have If it's not completed in time for the current version, it can be postponed label Jul 3, 2024
@darosior
Copy link
Member Author

I'm not sure yet how it should be integrated, but i think it should be minimal. For instance if we choose to have the upgrade button inside the signing devices list item, then opening a pop-up would be a pop-up on top of a pop-up which might not be ideal.

Maybe when you start tackling this, start by sketching out the idea and propose it here before moving on with the implementation?

@pythcoiner
Copy link
Collaborator

pythcoiner commented Jul 23, 2024

I've started look at this, i think we should at least have the feature in 3 location:

  • In installer "Select a signing device" modal:

Image

  • In the installer "Register descriptor step" (3/6)

Image

  • In wallet "Select signing device to sign with":

Image

The way i think to achieve this is to grey-out (disable) the signing device selection button and add inside it an " Upgrade my ledger" button that will trigger the upgrade process while keeping the modal opened:

Image

Points to consider

  • i think we should publish ledger_manager on crates.io
  • i think we should not let user leave the modal if an upgrade is in progress.
  • i think we should display a log of Hints & infos about the upgrade process.
  • Should we allow several signing devices to upgrade at same time?

@darosior
Copy link
Member Author

I was wondering why it would be necessary after the installer step, but it may be the case that a participant who shared his xpub may not have the appropriate version. In this case we should indeed have a version check before registering the descriptor and instruct the user to upgrade if the version is too old. But why having it at signing time? You may only ever sign with a device upon which was registered the descriptor, so having the check there seem sufficient?

@darosior
Copy link
Member Author

i think we should publish ledger_manager on crates.io

Sure, we can do it but it's not a blocker. Especially if it turns out we need to adjust a couple things.

i think we should not let user leave the modal if an upgrade is in progress.

Probably, yes. We can never ensure the user won't interrupt the upgrade though.

i think we should display a log of Hints & infos about the upgrade process.

Ideally we'd also show the progress on the UI but i can understand how it can be difficult.

Should we allow several signing devices to upgrade at same time?

Why not, but let's keep it simple for now. (Also this conflicts with your second statement of being stuck on the modal.)

@darosior
Copy link
Member Author

The way i think to achieve this is to grey-out (disable) the signing device selection button and add inside it an " Upgrade my ledger" button that will trigger the upgrade process while keeping the modal opened:

It's not clear for the user when they should click on the button nor is it technically correct because of the size restriction.

Instead what do you think of:

  1. Greying out the list item
  2. Displaying an attached yellow banner just below it (in our usual warning yellow) with mention "Your Ledger's Bitcoin app is too old to be used in Taproot descriptor." aligned on the left, and then an "upgrade" button aligned on the right.

I've tried to draft what i have in mind to be clearer:
signal-2024-07-23-094130

@pythcoiner
Copy link
Collaborator

I was wondering why it would be necessary after the installer step, but it may be the case that a participant who shared his xpub may not have the appropriate version. In this case we should indeed have a version check before registering the descriptor and instruct the user to upgrade if the version is too old. But why having it at signing time? You may only ever sign with a device upon which was registered the descriptor, so having the check there seem sufficient?

In the case an user have broken his ledger, replace by a new one, import his private key into, as Liana store the PoR, i think it will be detected as registered. So in this case user can end up to signing step w/o Liana told him the device is not registered.

@pythcoiner
Copy link
Collaborator

i think we should not let user leave the modal if an upgrade is in progress.

Probably, yes. We can never ensure the user won't interrupt the upgrade though.

user can only interrupt on hardware side and i think we can detect it and then allow user to leave the modal

i think we should display a log of Hints & infos about the upgrade process.

Ideally we'd also show the progress on the UI but i can understand how it can be difficult.

I think a progress bar is doable, I don't think difficult but not sure if it worth the time

@darosior
Copy link
Member Author

user can only interrupt on hardware side and i think we can detect it and then allow user to leave the modal

They can just close the software altogether. Preventing to close the modal is just to prevent a simple mistake imo (and might be worth it), but it's not possible to protect the user from themselves in this case.

@pythcoiner
Copy link
Collaborator

I've tried to draft what i have in mind to be clearer: signal-2024-07-23-094130

it's not exactly what you have drafted, but what do you think about this (modulo the color theme that should been adjusted for the button at least):

Image

(i do not see an easy way to make the yellow banner 'attached' to the device button, so I put it inside the button, as if below,the user can be confused if there is 2 following ledger in the list)

@pythcoiner
Copy link
Collaborator

or maybe something like this?

Image

@pythcoiner
Copy link
Collaborator

or like this:

Image

@darosior
Copy link
Member Author

darosior commented Jul 24, 2024 via email

@nondiremanuel
Copy link
Collaborator

One thing that could be worth considering is that this modal could be part of the UX refactoring.

@pythcoiner pythcoiner linked a pull request Jul 24, 2024 that will close this issue
@pythcoiner
Copy link
Collaborator

i think we should also hae the upgrade feature here, for the same reasons we need at signing step.

image

@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
Feature New feature or functionality. GUI gui related Nice to have If it's not completed in time for the current version, it can be postponed Signing devices Anything related to signing devices ("hardware wallets")
Projects
Status: In Review
Development

Successfully merging a pull request may close this issue.

3 participants