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

Added Ledger support to the wallet. #2068

Closed
wants to merge 8 commits into from
Closed

Conversation

murisi
Copy link
Collaborator

@murisi murisi commented Oct 27, 2023

Describe your changes

Added Ledger support to the namadaw command, expanded the Store to also hold derivation paths and public keys, and attempted to simplify the wallet logic. More specifically:

  • Derivation paths are now stored in the wallet to ease future signing operations
  • Public keys are now stored in the wallet so that the namadaw key list command remains as informative even in the absence of the secret key (which may be being stored externally on a hardware wallet)
  • Modified the key generation command to always generate and print out a seed phrase
  • Encryption is now done in store.rs to ensure that the key derivations are always maximized and are done correctly:
    • I.e. a public key and public key hash of the same alias should always be derived from inserting secret key to wallet
  • The SDK now takes derivation paths using the type DerivationPath instead of String
    • This enabled the removal of string parsing logic (like checking for "default" keyword) from the SDK
  • Broke up the SDK key derivation function because it was doing many things and its parameters were like flags
  • Implemented key derivation given a derivation path using the hardware wallet
  • Enabled transactions to be signed using the hardware wallet when it is connected
  • Parameterized SDK functions depending on randomness with a randomness parameter
  • Removed code that allowed multiple wallet entries sharing the same alias to be inserted separately
  • Implemented Serde (de)serialization of public keys and derivation paths to enable Store (de)serialization
  • Augmented the namadaw key list command to also print out public keys (labelled as external) even when no corresponding secret key exists

closes #70

Indicate on which release or other PRs this topic is based on

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

@murisi murisi force-pushed the murisi/hw-wallet-integration branch 5 times, most recently from c121113 to d78759c Compare October 30, 2023 12:31
@murisi murisi force-pushed the murisi/hw-wallet-integration branch from d78759c to 959933b Compare October 31, 2023 11:05
@murisi murisi force-pushed the murisi/hw-wallet-integration branch 2 times, most recently from 28b65f2 to 5a56b2e Compare November 6, 2023 10:27
@murisi murisi force-pushed the murisi/hw-wallet-integration branch from 5a56b2e to 302db22 Compare November 6, 2023 10:33
@murisi murisi marked this pull request as ready for review November 6, 2023 10:34
Copy link
Collaborator

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

Most of the changes look fine, but I have some minor concerns about the flow.

impl FromStr for DerivationPath {
type Err = DerivationPathError;

fn from_str(s: &str) -> Result<Self, Self::Err> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was it introduced? Can DerivationPath::from_path_str be used instead, which also takes the scheme into account?

Copy link
Contributor

@karbyshev karbyshev Nov 6, 2023

Choose a reason for hiding this comment

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

For SchemeType::Ed25519 scheme, all path segments must be hardened.

Copy link
Collaborator Author

@murisi murisi Nov 7, 2023

Choose a reason for hiding this comment

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

I see your point. FromStr was introduced to support the implementation of Serde::Deserialize for DerivationPath. And this is required in order to be able to store DerivationPaths in the wallet's Store, which itself is deserialized from file storage using Serde. I guess requiring scheme information to deserialize derivation paths would mean having to tag derivation paths with their scheme types. Couldn't we instead just deserialize all well-formatted (untagged) derivation paths (e.g. m/.../.../...) and delay the hardening/validations to when the actual SLIP10/BIP32 key computations are actually computed?

On the second point: I believe that all path segments are still being hardened for SchemeType::Ed25519. The slip10_ed25519::derive_ed25519_private_key function hardens all indices using the statement let hardened_index = 0x80000000 | *i;. As a separate thought: Instead of implicitly hardening Ed25519 paths, have we considered just rejecting non-hardened Ed25519 paths as is done in one of the implementations linked to from the SLIP10 specification? See https://github.com/elucidsoft/dotnetstandard-bip32/blob/master/dotnetstandard-bip32/BIP32.cs#L65 where they use the regex ^m(\/[0-9]+')+$ for derivation paths.

Copy link
Contributor

@karbyshev karbyshev Nov 7, 2023

Choose a reason for hiding this comment

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

I guess requiring scheme information to deserialize derivation paths would mean having to tag derivation paths with their scheme types.

I would prefer this solution. I think that explicit storage of the scheme tag might be also useful from the user perspective.

@murisi murisi force-pushed the murisi/hw-wallet-integration branch from 3678a6d to e6f2716 Compare November 7, 2023 08:24
@murisi murisi closed this Nov 7, 2023
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.

ledger (HW wallet) keys integration
3 participants