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

CLI wallet: Ledger Nano support #1606

Merged

Conversation

Alex6323
Copy link
Contributor

@Alex6323 Alex6323 commented Nov 10, 2023

Description of change

Adds support for Ledger Nano to the CLI wallet and fixes some usability issues.

Links to any relevant issues

Closes #144

Type of change

Choose a type of change, and delete any options that are not relevant.

  • Enhancement (a non-breaking change which adds functionality)

@Thoralf-M Thoralf-M changed the title finish impl CLI wallet: Ledger Nano support Nov 10, 2023
Copy link
Member

@Thoralf-M Thoralf-M left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove let wallet = unlock_wallet(storage_path, None, None).await?; from node_info_command()?
We don't need this for a stronghold wallet and certainly not for a ledger nano one

Please also check if there are other places where one is requested to insert a pw even if not needed (not needed at all or just not needed with a ledger nano)

cli/src/helper.rs Outdated Show resolved Hide resolved
DaughterOfMars
DaughterOfMars previously approved these changes Nov 13, 2023
@Alex6323
Copy link
Contributor Author

Can you remove let wallet = unlock_wallet(storage_path, None, None).await?; from node_info_command()? We don't need this for a stronghold wallet and certainly not for a ledger nano one

Please also check if there are other places where one is requested to insert a pw even if not needed (not needed at all or just not needed with a ledger nano)

Are sure you need to enter a password to run the node info command? I don't. As you can see from the line you posted, password is None.

cli/src/helper.rs Outdated Show resolved Hide resolved
Co-authored-by: DaughterOfMars <[email protected]>
@Alex6323
Copy link
Contributor Author

Can you remove let wallet = unlock_wallet(storage_path, None, None).await?; from node_info_command()? We don't need this for a stronghold wallet and certainly not for a ledger nano one
Please also check if there are other places where one is requested to insert a pw even if not needed (not needed at all or just not needed with a ledger nano)

Are sure you need to enter a password to run the node info command? I don't. As you can see from the line you posted, password is None.

Actually during current implementation of handling several secret managers I came to realize that this should require a password atm, because this command uses the node as stored in the wallet, not just any node. Hence the node used should be considered private information. An alternative would be to require the URL, but I guess that's not what we want. We want the node info of the node that specifically is used to sync the wallet.

Copy link
Member

@Thoralf-M Thoralf-M left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running just cargo run --release has a different behavior (asks directly for stronghold pw) than running with init (shows secret manager selection)
Should both be like the init command?

Copy link
Member

@Thoralf-M Thoralf-M left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running a wallet command after it got initialized with ledger nano returns Stronghold snapshot not found at './stardust-cli-wallet.stronghold'. (get-info) or it asks for a stronghold password, just to then fail with wallet error: no mnemonic has been stored into the Stronghold vault (new-account)

@Alex6323
Copy link
Contributor Author

Running a wallet command after it got initialized with ledger nano returns Stronghold snapshot not found at './stardust-cli-wallet.stronghold'. (get-info) or it asks for a stronghold password, just to then fail with wallet error: no mnemonic has been stored into the Stronghold vault (new-account)

how did you run the wallet? Stronghold is still the default secret manager if you don't pass -s <SECRET_MANAGER> to the binary.

@Thoralf-M
Copy link
Member

Running a wallet command after it got initialized with ledger nano returns Stronghold snapshot not found at './stardust-cli-wallet.stronghold'. (get-info) or it asks for a stronghold password, just to then fail with wallet error: no mnemonic has been stored into the Stronghold vault (new-account)

how did you run the wallet? Stronghold is still the default secret manager if you don't pass -s <SECRET_MANAGER> to the binary.

Just cargo run --release get-info (before initialized with ledger nano)

@Alex6323
Copy link
Contributor Author

Running a wallet command after it got initialized with ledger nano returns Stronghold snapshot not found at './stardust-cli-wallet.stronghold'. (get-info) or it asks for a stronghold password, just to then fail with wallet error: no mnemonic has been stored into the Stronghold vault (new-account)

how did you run the wallet? Stronghold is still the default secret manager if you don't pass -s <SECRET_MANAGER> to the binary.

Just cargo run --release get-info (before initialized with ledger nano)

gonna fix the quirks ...

DaughterOfMars
DaughterOfMars previously approved these changes Jan 16, 2024
Thoralf-M
Thoralf-M previously approved these changes Jan 16, 2024
Thoralf-M
Thoralf-M previously approved these changes Jan 17, 2024
DaughterOfMars
DaughterOfMars previously approved these changes Jan 17, 2024
@thibault-martinez
Copy link
Member

(don't merge yet please, still having a look)

Copy link
Member

@thibault-martinez thibault-martinez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conflict

@Alex6323 Alex6323 marked this pull request as draft January 22, 2024 14:46
@Alex6323 Alex6323 marked this pull request as ready for review January 22, 2024 14:47
DaughterOfMars
DaughterOfMars previously approved these changes Jan 22, 2024
Thoralf-M
Thoralf-M previously approved these changes Jan 22, 2024
@thibault-martinez thibault-martinez merged commit a236939 into iotaledger:develop Jan 23, 2024
33 checks passed
@Alex6323 Alex6323 deleted the feat/cli/ledger-nano-support branch January 23, 2024 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cli Wallet: Ledger Nano support
4 participants