Installer: let user choose derivation path by pythcoiner · Pull Request #1705 · wizardsardine/liana

pythcoiner · 2025-05-15T01:25:06Z

This PR closes #1700.

pythcoiner · 2025-05-15T02:52:56Z

actual state:

edouardparis · 2025-05-15T07:59:03Z

I would not use the warning icon but the tooltip one (icon::tooltip_icon) and move it on the right of
Import an extended public key by selecting a signing device.

Also I do not understand why incrementing account index makes the derivation path not standard.

pythcoiner · 2025-05-15T09:42:43Z

Also I do not understand why incrementing account index makes the derivation path not standard.

cc @bigspider

bigspider · 2025-05-15T09:54:33Z

Not sure what do you mean? Account numbers for the various supported BIPs are considered standard by the Ledger app if not larger than 100.

pythcoiner · 2025-05-15T10:03:23Z

Not sure what do you mean? Account numbers for the various supported BIPs are considered standard by the Ledger app if not larger than 100.

I failed to export an xpub w/ m/48'/1'/1'/2' (Nano S+ testnet app 2.2.0-beta)

and success if display == true

pythcoiner · 2025-05-15T11:02:36Z

I would not use the warning icon but the tooltip one (icon::tooltip_icon) and move it on the right of
Import an extended public key by selecting a signing device.

pythcoiner · 2025-05-16T12:06:48Z

fixed my mistake, the index stored was Unhardened

pythcoiner · 2025-05-16T12:11:03Z

 }

+pub fn derivation_path(network: Network, account: ChildNumber) -> DerivationPath {
+    assert!(account.is_hardened());


Note: added this assert to catch if an unhardened derivation path is used

nit: I feel it might be slightly better to return an Option or Result here and let the caller unwrap it.

jp1ac4

tACK 7d6fce8.

Tested with Ledger and Specter DIY. Added comments for some nits.

jp1ac4 · 2025-05-20T16:14:39Z

+                    version.as_ref(),
+                    *fingerprint,
+                    alias.as_ref(),
+                    accounts.get(fingerprint).cloned(),


nit: could also use .copied()

jp1ac4 · 2025-05-20T16:18:13Z

 }

+pub fn derivation_path(network: Network, account: ChildNumber) -> DerivationPath {
+    assert!(account.is_hardened());


nit: I feel it might be slightly better to return an Option or Result here and let the caller unwrap it.

jp1ac4 · 2025-05-20T16:20:01Z

+impl Account {
+    pub fn new(index: ChildNumber, fingerprint: Fingerprint) -> Self {
+        Self {
+            hardened_index: index,


nit: possibly there should be a check that the index argument is hardened given the field name, or otherwise I think you can rename the field back to index, which should be fine now that the type is ChildNumber.

hardened_index => index

jp1ac4 · 2025-05-20T16:21:39Z

+    let source = Row::new()
+        .push(p1_regular("Select the source of your key").bold())
+        .push(Space::with_width(10))
+        .push(info)


nit: I feel this tooltip would be more visible if it were next to the pick list within the HW card. Same applies for "Share Xpubs" below.

this have been request by @edouardparis previously and ACK'd by @nondiremanuel iirc

pythcoiner · 2025-05-20T16:31:17Z

nit: I feel it might be slightly better to return an Option or Result here and let the caller unwrap it.

I don't think it worth bother w/ Option/Result here, it's intended to catch an error at implementation time not at run time

jp1ac4

tACK 386c14c.

pythcoiner · 2025-05-21T09:53:10Z

added display of account after xpub fetched:

pythcoiner · 2025-05-21T12:58:29Z

If I choose the same key in a different path, the selected account is shown as 0 regardless of the selection made in the other path. So I think the account parameter is not being set in all places. Also, but less importantly, in the key list view, when viewing HWs, the account is not shown

fixed these 2 issues

…an xpub from signing device

pythcoiner · 2025-05-21T14:37:46Z

the "Register descriptor" step is showing the "Account #0"

fixed this issue

jp1ac4

tACK ce8e1f3.

edouardparis

ACK ce8e1f3

pythcoiner force-pushed the custom_deriv_path branch 2 times, most recently from 4c08ab5 to 611a3e8 Compare May 15, 2025 02:51

pythcoiner force-pushed the custom_deriv_path branch 3 times, most recently from 133abdb to c0c863a Compare May 16, 2025 12:04

pythcoiner commented May 16, 2025

View reviewed changes

pythcoiner marked this pull request as ready for review May 16, 2025 12:12

pythcoiner force-pushed the custom_deriv_path branch 3 times, most recently from 19afe39 to e2e411f Compare May 16, 2025 12:28

pythcoiner requested a review from edouardparis May 16, 2025 12:51

jp1ac4 reviewed May 19, 2025

View reviewed changes

Comment thread liana-ui/src/component/hw.rs Outdated

Comment thread liana-gui/src/installer/view/mod.rs Outdated

pythcoiner force-pushed the custom_deriv_path branch 3 times, most recently from 48f98e3 to b09fa49 Compare May 20, 2025 15:39

pythcoiner requested a review from jp1ac4 May 20, 2025 16:04

pythcoiner force-pushed the custom_deriv_path branch from b09fa49 to 7d6fce8 Compare May 20, 2025 16:09

jp1ac4 approved these changes May 20, 2025

View reviewed changes

pythcoiner force-pushed the custom_deriv_path branch from 7d6fce8 to 386c14c Compare May 20, 2025 16:34

jp1ac4 approved these changes May 20, 2025

View reviewed changes

nondiremanuel added this to Liana General May 21, 2025

nondiremanuel moved this to In Review in Liana General May 21, 2025

nondiremanuel assigned pythcoiner May 21, 2025

nondiremanuel added this to the v11 Liana - Multiwallet and Naming functionalities milestone May 21, 2025

pythcoiner force-pushed the custom_deriv_path branch from 386c14c to 5a05a84 Compare May 21, 2025 09:51

pythcoiner requested a review from jp1ac4 May 21, 2025 09:53

pythcoiner force-pushed the custom_deriv_path branch from 5a05a84 to 626eaa1 Compare May 21, 2025 12:55

pythcoiner added 2 commits May 21, 2025 16:32

ui: add a widget to let user select custom derivation path of an xpub

8efc55b

installer: let user choose account (derivation path) before fetching …

ce8e1f3

…an xpub from signing device

pythcoiner force-pushed the custom_deriv_path branch from 626eaa1 to ce8e1f3 Compare May 21, 2025 14:36

jp1ac4 approved these changes May 21, 2025

View reviewed changes

edouardparis approved these changes May 21, 2025

View reviewed changes

edouardparis merged commit cc00114 into wizardsardine:master May 21, 2025
15 of 19 checks passed

nondiremanuel moved this from In Review to Done in Liana General May 22, 2025

Conversation

pythcoiner commented May 15, 2025 • edited by edouardparis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pythcoiner commented May 15, 2025

Uh oh!

edouardparis commented May 15, 2025

Uh oh!

pythcoiner commented May 15, 2025

Uh oh!

bigspider commented May 15, 2025

Uh oh!

pythcoiner commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pythcoiner commented May 15, 2025

Uh oh!

pythcoiner commented May 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jp1ac4 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pythcoiner commented May 20, 2025

Uh oh!

jp1ac4 left a comment

Choose a reason for hiding this comment

Uh oh!

pythcoiner commented May 21, 2025

Uh oh!

pythcoiner commented May 21, 2025

Uh oh!

pythcoiner commented May 21, 2025

Uh oh!

jp1ac4 left a comment

Choose a reason for hiding this comment

Uh oh!

edouardparis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pythcoiner commented May 15, 2025 •

edited by edouardparis

Loading

pythcoiner commented May 15, 2025 •

edited

Loading