Refactor SignerSource to expose DerivationPath to other kinds of signers (#16933)

* One use statement

* Add stdin uri scheme

* Convert parse_signer_source to return Result

* A-Z deps

* Convert Usb data to Locator

* Pull DerivationPath out of Locator

* Wrap SignerSource to share derivation_path

* Review comments

* Check Filepath existence, readability in parse_signer_source

Cargo.lock

@@ -4107,6 +4107,7 @@ dependencies = [
  "rpassword",
  "solana-remote-wallet",
  "solana-sdk",
+ "tempfile",
  "thiserror",
  "tiny-bip39 0.8.0",
  "uriparse",
@@ -5102,6 +5103,7 @@ dependencies = [
  "num-derive",
  "num-traits",
  "pbkdf2 0.6.0",
+ "qstring",
  "rand 0.7.3",
  "rand_chacha 0.2.2",
  "rand_core 0.6.2",
@@ -5121,6 +5123,7 @@ dependencies = [
  "solana-sdk-macro 1.7.0",
  "thiserror",
  "tiny-bip39 0.7.3",
+ "uriparse",
 ]
 
 [[package]]

clap-utils/Cargo.toml

@@ -20,6 +20,9 @@ uriparse = "0.6.3"
 url = "2.1.0"
 chrono = "0.4"
 
+[dev-dependencies]
+tempfile = "3.1.0"
+
 [lib]
 name = "solana_clap_utils"
 

clap-utils/src/fee_payer.rs

@@ -1,5 +1,7 @@
-use crate::{input_validators, ArgConstant};
-use clap::Arg;
+use {
+    crate::{input_validators, ArgConstant},
+    clap::Arg,
+};
 
 pub const FEE_PAYER_ARG: ArgConstant<'static> = ArgConstant {
     name: "fee_payer",

clap-utils/src/input_parsers.rs

@@ -1,19 +1,21 @@
-use crate::keypair::{
-    keypair_from_seed_phrase, pubkey_from_path, resolve_signer_from_path, signer_from_path,
-    ASK_KEYWORD, SKIP_SEED_PHRASE_VALIDATION_ARG,
+use {
+    crate::keypair::{
+        keypair_from_seed_phrase, pubkey_from_path, resolve_signer_from_path, signer_from_path,
+        ASK_KEYWORD, SKIP_SEED_PHRASE_VALIDATION_ARG,
+    },
+    chrono::DateTime,
+    clap::ArgMatches,
+    solana_remote_wallet::remote_wallet::RemoteWalletManager,
+    solana_sdk::{
+        clock::UnixTimestamp,
+        commitment_config::CommitmentConfig,
+        genesis_config::ClusterType,
+        native_token::sol_to_lamports,
+        pubkey::Pubkey,
+        signature::{read_keypair_file, Keypair, Signature, Signer},
+    },
+    std::{str::FromStr, sync::Arc},
 };
-use chrono::DateTime;
-use clap::ArgMatches;
-use solana_remote_wallet::remote_wallet::RemoteWalletManager;
-use solana_sdk::{
-    clock::UnixTimestamp,
-    commitment_config::CommitmentConfig,
-    genesis_config::ClusterType,
-    native_token::sol_to_lamports,
-    pubkey::Pubkey,
-    signature::{read_keypair_file, Keypair, Signature, Signer},
-};
-use std::{str::FromStr, sync::Arc};
 
 // Return parsed values from matches at `name`
 pub fn values_of<T>(matches: &ArgMatches<'_>, name: &str) -> Option<Vec<T>>

clap-utils/src/input_validators.rs

@@ -1,13 +1,15 @@
-use crate::keypair::{parse_signer_source, SignerSource, ASK_KEYWORD};
-use chrono::DateTime;
-use solana_sdk::{
-    clock::{Epoch, Slot},
-    hash::Hash,
-    pubkey::{Pubkey, MAX_SEED_LEN},
-    signature::{read_keypair_file, Signature},
+use {
+    crate::keypair::{parse_signer_source, SignerSourceKind, ASK_KEYWORD},
+    chrono::DateTime,
+    solana_sdk::{
+        clock::{Epoch, Slot},
+        hash::Hash,
+        pubkey::{Pubkey, MAX_SEED_LEN},
+        signature::{read_keypair_file, Signature},
+    },
+    std::fmt::Display,
+    std::str::FromStr,
 };
-use std::fmt::Display;
-use std::str::FromStr;
 
 fn is_parsable_generic<U, T>(string: T) -> Result<(), String>
 where
@@ -108,8 +110,11 @@ pub fn is_valid_pubkey<T>(string: T) -> Result<(), String>
 where
     T: AsRef<str> + Display,
 {
-    match parse_signer_source(string.as_ref()) {
-        SignerSource::Filepath(path) => is_keypair(path),
+    match parse_signer_source(string.as_ref())
+        .map_err(|err| format!("{}", err))?
+        .kind
+    {
+        SignerSourceKind::Filepath(path) => is_keypair(path),
         _ => Ok(()),
     }
 }

clap-utils/src/keypair.rs

@@ -1,31 +1,36 @@
-use crate::{
-    input_parsers::pubkeys_sigs_of,
-    offline::{SIGNER_ARG, SIGN_ONLY_ARG},
-    ArgConstant,
-};
-use bip39::{Language, Mnemonic, Seed};
-use clap::ArgMatches;
-use rpassword::prompt_password_stderr;
-use solana_remote_wallet::{
-    remote_keypair::generate_remote_keypair,
-    remote_wallet::{maybe_wallet_manager, RemoteWalletError, RemoteWalletManager},
-};
-use solana_sdk::{
-    hash::Hash,
-    message::Message,
-    pubkey::Pubkey,
-    signature::{
-        keypair_from_seed, keypair_from_seed_phrase_and_passphrase, read_keypair,
-        read_keypair_file, Keypair, NullSigner, Presigner, Signature, Signer,
+use {
+    crate::{
+        input_parsers::pubkeys_sigs_of,
+        offline::{SIGNER_ARG, SIGN_ONLY_ARG},
+        ArgConstant,
     },
-};
-use std::{
-    convert::TryFrom,
-    error,
-    io::{stdin, stdout, Write},
-    process::exit,
-    str::FromStr,
-    sync::Arc,
+    bip39::{Language, Mnemonic, Seed},
+    clap::ArgMatches,
+    rpassword::prompt_password_stderr,
+    solana_remote_wallet::{
+        locator::{Locator as RemoteWalletLocator, LocatorError as RemoteWalletLocatorError},
+        remote_keypair::generate_remote_keypair,
+        remote_wallet::{maybe_wallet_manager, RemoteWalletError, RemoteWalletManager},
+    },
+    solana_sdk::{
+        derivation_path::{DerivationPath, DerivationPathError},
+        hash::Hash,
+        message::Message,
+        pubkey::Pubkey,
+        signature::{
+            keypair_from_seed, keypair_from_seed_phrase_and_passphrase, read_keypair,
+            read_keypair_file, Keypair, NullSigner, Presigner, Signature, Signer,
+        },
+    },
+    std::{
+        convert::TryFrom,
+        error,
+        io::{stdin, stdout, Write},
+        process::exit,
+        str::FromStr,
+        sync::Arc,
+    },
+    thiserror::Error,
 };
 
 pub struct SignOnly {
@@ -132,34 +137,72 @@ impl DefaultSigner {
     }
 }
 
-pub(crate) enum SignerSource {
+pub(crate) struct SignerSource {
+    pub kind: SignerSourceKind,
+    pub derivation_path: Option<DerivationPath>,
+}
+
+impl SignerSource {
+    fn new(kind: SignerSourceKind) -> Self {
+        Self {
+            kind,
+            derivation_path: None,
+        }
+    }
+}
+
+pub(crate) enum SignerSourceKind {
     Ask,
     Filepath(String),
-    Usb(String),
+    Usb(RemoteWalletLocator),
     Stdin,
     Pubkey(Pubkey),
 }
 
-pub(crate) fn parse_signer_source<S: AsRef<str>>(source: S) -> SignerSource {
+#[derive(Debug, Error)]
+pub(crate) enum SignerSourceError {
+    #[error("unrecognized signer source")]
+    UnrecognizedSource,
+    #[error(transparent)]
+    RemoteWalletLocatorError(#[from] RemoteWalletLocatorError),
+    #[error(transparent)]
+    DerivationPathError(#[from] DerivationPathError),
+    #[error(transparent)]
+    IoError(#[from] std::io::Error),
+}
+
+pub(crate) fn parse_signer_source<S: AsRef<str>>(
+    source: S,
+) -> Result<SignerSource, SignerSourceError> {
     let source = source.as_ref();
     match uriparse::URIReference::try_from(source) {
-        Err(_) => SignerSource::Filepath(source.to_string()),
+        Err(_) => Err(SignerSourceError::UnrecognizedSource),
         Ok(uri) => {
             if let Some(scheme) = uri.scheme() {
                 let scheme = scheme.as_str().to_ascii_lowercase();
                 match scheme.as_str() {
-                    "ask" => SignerSource::Ask,
-                    "file" => SignerSource::Filepath(uri.path().to_string()),
-                    "usb" => SignerSource::Usb(source.to_string()),
-                    _ => SignerSource::Filepath(source.to_string()),
+                    "ask" => Ok(SignerSource::new(SignerSourceKind::Ask)),
+                    "file" => Ok(SignerSource::new(SignerSourceKind::Filepath(
+                        uri.path().to_string(),
+                    ))),
+                    "stdin" => Ok(SignerSource::new(SignerSourceKind::Stdin)),
+                    "usb" => Ok(SignerSource {
+                        kind: SignerSourceKind::Usb(RemoteWalletLocator::new_from_uri(&uri)?),
+                        derivation_path: DerivationPath::from_uri(&uri)?,
+                    }),
+                    _ => Err(SignerSourceError::UnrecognizedSource),
                 }
             } else {
                 match source {
-                    "-" => SignerSource::Stdin,
-                    ASK_KEYWORD => SignerSource::Ask,
+                    "-" => Ok(SignerSource::new(SignerSourceKind::Stdin)),
+                    ASK_KEYWORD => Ok(SignerSource::new(SignerSourceKind::Ask)),
                     _ => match Pubkey::from_str(source) {
-                        Ok(pubkey) => SignerSource::Pubkey(pubkey),
-                        Err(_) => SignerSource::Filepath(source.to_string()),
+                        Ok(pubkey) => Ok(SignerSource::new(SignerSourceKind::Pubkey(pubkey))),
+                        Err(_) => std::fs::metadata(source)
+                            .map(|_| {
+                                SignerSource::new(SignerSourceKind::Filepath(source.to_string()))
+                            })
+                            .map_err(|err| err.into()),
                     },
                 }
             }
@@ -210,8 +253,12 @@ pub fn signer_from_path_with_config(
     wallet_manager: &mut Option<Arc<RemoteWalletManager>>,
     config: &SignerFromPathConfig,
 ) -> Result<Box<dyn Signer>, Box<dyn error::Error>> {
-    match parse_signer_source(path) {
-        SignerSource::Ask => {
+    let SignerSource {
+        kind,
+        derivation_path,
+    } = parse_signer_source(path)?;
+    match kind {
+        SignerSourceKind::Ask => {
             let skip_validation = matches.is_present(SKIP_SEED_PHRASE_VALIDATION_ARG.name);
             Ok(Box::new(keypair_from_seed_phrase(
                 keypair_name,
@@ -219,7 +266,7 @@ pub fn signer_from_path_with_config(
                 false,
             )?))
         }
-        SignerSource::Filepath(path) => match read_keypair_file(&path) {
+        SignerSourceKind::Filepath(path) => match read_keypair_file(&path) {
             Err(e) => Err(std::io::Error::new(
                 std::io::ErrorKind::Other,
                 format!("could not read keypair file \"{}\". Run \"solana-keygen new\" to create a keypair file: {}", path, e),
@@ -227,17 +274,18 @@ pub fn signer_from_path_with_config(
             .into()),
             Ok(file) => Ok(Box::new(file)),
         },
-        SignerSource::Stdin => {
+        SignerSourceKind::Stdin => {
             let mut stdin = std::io::stdin();
             Ok(Box::new(read_keypair(&mut stdin)?))
         }
-        SignerSource::Usb(path) => {
+        SignerSourceKind::Usb(locator) => {
             if wallet_manager.is_none() {
                 *wallet_manager = maybe_wallet_manager()?;
             }
             if let Some(wallet_manager) = wallet_manager {
                 Ok(Box::new(generate_remote_keypair(
-                    path,
+                    locator,
+                    derivation_path.unwrap_or_default(),
                     wallet_manager,
                     matches.is_present("confirm_key"),
                     keypair_name,
@@ -246,7 +294,7 @@ pub fn signer_from_path_with_config(
                 Err(RemoteWalletError::NoDeviceFound.into())
             }
         }
-        SignerSource::Pubkey(pubkey) => {
+        SignerSourceKind::Pubkey(pubkey) => {
             let presigner = pubkeys_sigs_of(matches, SIGNER_ARG.name)
                 .as_ref()
                 .and_then(|presigners| presigner_from_pubkey_sigs(&pubkey, presigners));
@@ -271,8 +319,9 @@ pub fn pubkey_from_path(
     keypair_name: &str,
     wallet_manager: &mut Option<Arc<RemoteWalletManager>>,
 ) -> Result<Pubkey, Box<dyn error::Error>> {
-    match parse_signer_source(path) {
-        SignerSource::Pubkey(pubkey) => Ok(pubkey),
+    let SignerSource { kind, .. } = parse_signer_source(path)?;
+    match kind {
+        SignerSourceKind::Pubkey(pubkey) => Ok(pubkey),
         _ => Ok(signer_from_path(matches, path, keypair_name, wallet_manager)?.pubkey()),
     }
 }
@@ -283,14 +332,18 @@ pub fn resolve_signer_from_path(
     keypair_name: &str,
     wallet_manager: &mut Option<Arc<RemoteWalletManager>>,
 ) -> Result<Option<String>, Box<dyn error::Error>> {
-    match parse_signer_source(path) {
-        SignerSource::Ask => {
+    let SignerSource {
+        kind,
+        derivation_path,
+    } = parse_signer_source(path)?;
+    match kind {
+        SignerSourceKind::Ask => {
             let skip_validation = matches.is_present(SKIP_SEED_PHRASE_VALIDATION_ARG.name);
             // This method validates the seed phrase, but returns `None` because there is no path
             // on disk or to a device
             keypair_from_seed_phrase(keypair_name, skip_validation, false).map(|_| None)
         }
-        SignerSource::Filepath(path) => match read_keypair_file(&path) {
+        SignerSourceKind::Filepath(path) => match read_keypair_file(&path) {
             Err(e) => Err(std::io::Error::new(
                 std::io::ErrorKind::Other,
                 format!("could not read keypair file \"{}\". Run \"solana-keygen new\" to create a keypair file: {}", path, e),
@@ -298,19 +351,20 @@ pub fn resolve_signer_from_path(
             .into()),
             Ok(_) => Ok(Some(path.to_string())),
         },
-        SignerSource::Stdin => {
+        SignerSourceKind::Stdin => {
             let mut stdin = std::io::stdin();
             // This method validates the keypair from stdin, but returns `None` because there is no
             // path on disk or to a device
             read_keypair(&mut stdin).map(|_| None)
         }
-        SignerSource::Usb(path) => {
+        SignerSourceKind::Usb(locator) => {
             if wallet_manager.is_none() {
                 *wallet_manager = maybe_wallet_manager()?;
             }
             if let Some(wallet_manager) = wallet_manager {
                 let path = generate_remote_keypair(
-                    path,
+                    locator,
+                    derivation_path.unwrap_or_default(),
                     wallet_manager,
                     matches.is_present("confirm_key"),
                     keypair_name,
@@ -414,7 +468,9 @@ fn sanitize_seed_phrase(seed_phrase: &str) -> String {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use solana_remote_wallet::locator::Manufacturer;
     use solana_sdk::system_instruction;
+    use tempfile::NamedTempFile;
 
     #[test]
     fn test_sanitize_seed_phrase() {
@@ -458,35 +514,108 @@ mod tests {
 
     #[test]
     fn test_parse_signer_source() {
-        assert!(matches!(parse_signer_source("-"), SignerSource::Stdin));
         assert!(matches!(
-            parse_signer_source(ASK_KEYWORD),
-            SignerSource::Ask
+            parse_signer_source("-").unwrap(),
+            SignerSource {
+                kind: SignerSourceKind::Stdin,
+                derivation_path: None,
+            }
+        ));
+        let ask = "stdin:".to_string();
+        assert!(matches!(
+            parse_signer_source(&ask).unwrap(),
+            SignerSource {
+                kind: SignerSourceKind::Stdin,
+                derivation_path: None,
+            }
+        ));
+        assert!(matches!(
+            parse_signer_source(ASK_KEYWORD).unwrap(),
+            SignerSource {
+                kind: SignerSourceKind::Ask,
+                derivation_path: None,
+            }
         ));
         let pubkey = Pubkey::new_unique();
         assert!(
-            matches!(parse_signer_source(&pubkey.to_string()), SignerSource::Pubkey(p) if p == pubkey)
+            matches!(parse_signer_source(&pubkey.to_string()).unwrap(), SignerSource {
+                kind: SignerSourceKind::Pubkey(p),
+                derivation_path: None,
+            }
+            if p == pubkey)
+        );
+
+        // Set up absolute and relative path strs
+        let file0 = NamedTempFile::new().unwrap();
+        let path = file0.path();
+        assert!(path.is_absolute());
+        let absolute_path_str = path.to_str().unwrap();
+
+        let file1 = NamedTempFile::new_in(std::env::current_dir().unwrap()).unwrap();
+        let path = file1.path().file_name().unwrap().to_str().unwrap();
+        let path = std::path::Path::new(path);
+        assert!(path.is_relative());
+        let relative_path_str = path.to_str().unwrap();
+
+        assert!(
+            matches!(parse_signer_source(absolute_path_str).unwrap(), SignerSource {
+                kind: SignerSourceKind::Filepath(p),
+                derivation_path: None,
+            } if p == absolute_path_str)
         );
-        let path = "/absolute/path".to_string();
-        assert!(matches!(parse_signer_source(&path), SignerSource::Filepath(p) if p == path));
-        let path = "relative/path".to_string();
-        assert!(matches!(parse_signer_source(&path), SignerSource::Filepath(p) if p == path));
+        assert!(
+            matches!(parse_signer_source(&relative_path_str).unwrap(), SignerSource {
+                kind: SignerSourceKind::Filepath(p),
+                derivation_path: None,
+            } if p == relative_path_str)
+        );
+
         let usb = "usb://ledger".to_string();
-        assert!(matches!(parse_signer_source(&usb), SignerSource::Usb(u) if u == usb));
-        // Catchall into SignerSource::Filepath
-        let junk = "sometextthatisnotapubkey".to_string();
+        let expected_locator = RemoteWalletLocator {
+            manufacturer: Manufacturer::Ledger,
+            pubkey: None,
+        };
+        assert!(matches!(parse_signer_source(&usb).unwrap(), SignerSource {
+                kind: SignerSourceKind::Usb(u),
+                derivation_path: None,
+            } if u == expected_locator));
+        let usb = "usb://ledger?key=0/0".to_string();
+        let expected_locator = RemoteWalletLocator {
+            manufacturer: Manufacturer::Ledger,
+            pubkey: None,
+        };
+        let expected_derivation_path = Some(DerivationPath::new_bip44(Some(0), Some(0)));
+        assert!(matches!(parse_signer_source(&usb).unwrap(), SignerSource {
+                kind: SignerSourceKind::Usb(u),
+                derivation_path: d,
+            } if u == expected_locator && d == expected_derivation_path));
+        // Catchall into SignerSource::Filepath fails
+        let junk = "sometextthatisnotapubkeyorfile".to_string();
         assert!(Pubkey::from_str(&junk).is_err());
-        assert!(matches!(parse_signer_source(&junk), SignerSource::Filepath(j) if j == junk));
+        assert!(matches!(
+            parse_signer_source(&junk),
+            Err(SignerSourceError::IoError(_))
+        ));
 
         let ask = "ask:".to_string();
-        assert!(matches!(parse_signer_source(&ask), SignerSource::Ask));
-        let path = "/absolute/path".to_string();
+        assert!(matches!(
+            parse_signer_source(&ask).unwrap(),
+            SignerSource {
+                kind: SignerSourceKind::Ask,
+                derivation_path: None,
+            }
+        ));
         assert!(
-            matches!(parse_signer_source(&format!("file:{}", path)), SignerSource::Filepath(p) if p == path)
+            matches!(parse_signer_source(&format!("file:{}", absolute_path_str)).unwrap(), SignerSource {
+                kind: SignerSourceKind::Filepath(p),
+                derivation_path: None,
+            } if p == absolute_path_str)
         );
-        let path = "relative/path".to_string();
         assert!(
-            matches!(parse_signer_source(&format!("file:{}", path)), SignerSource::Filepath(p) if p == path)
+            matches!(parse_signer_source(&format!("file:{}", relative_path_str)).unwrap(), SignerSource {
+                kind: SignerSourceKind::Filepath(p),
+                derivation_path: None,
+            } if p == relative_path_str)
         );
     }
 }

clap-utils/src/memo.rs

@@ -1,5 +1,4 @@
-use crate::ArgConstant;
-use clap::Arg;
+use {crate::ArgConstant, clap::Arg};
 
 pub const MEMO_ARG: ArgConstant<'static> = ArgConstant {
     name: "memo",

clap-utils/src/nonce.rs

@@ -1,5 +1,7 @@
-use crate::{input_validators::*, offline::BLOCKHASH_ARG, ArgConstant};
-use clap::{App, Arg};
+use {
+    crate::{input_validators::*, offline::BLOCKHASH_ARG, ArgConstant},
+    clap::{App, Arg},
+};
 
 pub const NONCE_ARG: ArgConstant<'static> = ArgConstant {
     name: "nonce",

clap-utils/src/offline.rs

@@ -1,5 +1,7 @@
-use crate::{input_validators::*, ArgConstant};
-use clap::{App, Arg};
+use {
+    crate::{input_validators::*, ArgConstant},
+    clap::{App, Arg},
+};
 
 pub const BLOCKHASH_ARG: ArgConstant<'static> = ArgConstant {
     name: "blockhash",

programs/bpf/Cargo.lock

@@ -3461,6 +3461,7 @@ dependencies = [
  "num-derive 0.3.0",
  "num-traits",
  "pbkdf2 0.6.0",
+ "qstring",
  "rand 0.7.3",
  "rand_chacha 0.2.2",
  "rand_core 0.6.2",
@@ -3479,6 +3480,7 @@ dependencies = [
  "solana-program 1.7.0",
  "solana-sdk-macro 1.7.0",
  "thiserror",
+ "uriparse",
 ]
 
 [[package]]

remote-wallet/src/locator.rs

@@ -1,8 +1,5 @@
 use {
-    solana_sdk::{
-        derivation_path::{DerivationPath, DerivationPathError},
-        pubkey::{ParsePubkeyError, Pubkey},
-    },
+    solana_sdk::pubkey::{ParsePubkeyError, Pubkey},
     std::{
         convert::{Infallible, TryFrom, TryInto},
         str::FromStr,
@@ -77,8 +74,6 @@ pub enum LocatorError {
     #[error(transparent)]
     PubkeyError(#[from] ParsePubkeyError),
     #[error(transparent)]
-    DerivationPathError(#[from] DerivationPathError),
-    #[error(transparent)]
     UriReferenceError(#[from] URIReferenceError),
     #[error("unimplemented scheme")]
     UnimplementedScheme,
@@ -96,15 +91,12 @@ impl From<Infallible> for LocatorError {
 pub struct Locator {
     pub manufacturer: Manufacturer,
     pub pubkey: Option<Pubkey>,
-    pub derivation_path: Option<DerivationPath>,
 }
 
 impl std::fmt::Display for Locator {
     fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
         let maybe_path = self.pubkey.map(|p| p.to_string());
         let path = maybe_path.as_deref().unwrap_or("/");
-        let maybe_query = self.derivation_path.as_ref().map(|d| d.get_query());
-        let maybe_query2 = maybe_query.as_ref().map(|q| &q[1..]);
 
         let mut builder = URIReferenceBuilder::new();
         builder
@@ -113,8 +105,6 @@ impl std::fmt::Display for Locator {
             .try_authority(Some(self.manufacturer.as_ref()))
             .unwrap()
             .try_path(path)
-            .unwrap()
-            .try_query(maybe_query2)
             .unwrap();
 
         let uri = builder.build().unwrap();
@@ -141,28 +131,7 @@ impl Locator {
                         None
                     }
                 });
-                let key = if let Some(query) = uri.query() {
-                    let query_str = query.as_str();
-                    let query = qstring::QString::from(query_str);
-                    if query.len() > 1 {
-                        return Err(DerivationPathError::InvalidDerivationPath(
-                            "invalid query string, extra fields not supported".to_string(),
-                        )
-                        .into());
-                    }
-                    let key = query.get("key");
-                    if key.is_none() {
-                        return Err(DerivationPathError::InvalidDerivationPath(format!(
-                            "invalid query string `{}`, only `key` supported",
-                            query_str,
-                        ))
-                        .into());
-                    }
-                    key.map(|v| v.to_string())
-                } else {
-                    None
-                };
-                Self::new_from_parts(host.as_str(), path, key.as_deref())
+                Self::new_from_parts(host.as_str(), path)
             }
             (Some(_scheme), Some(_host)) => Err(LocatorError::UnimplementedScheme),
             (None, Some(_host)) => Err(LocatorError::UnimplementedScheme),
@@ -170,18 +139,15 @@ impl Locator {
         }
     }
 
-    pub fn new_from_parts<V, VE, P, PE, D, DE>(
+    pub fn new_from_parts<V, VE, P, PE>(
         manufacturer: V,
         pubkey: Option<P>,
-        derivation_path: Option<D>,
     ) -> Result<Self, LocatorError>
     where
         VE: Into<LocatorError>,
         V: TryInto<Manufacturer, Error = VE>,
         PE: Into<LocatorError>,
         P: TryInto<Pubkey, Error = PE>,
-        DE: Into<LocatorError>,
-        D: TryInto<DerivationPath, Error = DE>,
     {
         let manufacturer = manufacturer.try_into().map_err(|e| e.into())?;
         let pubkey = if let Some(pubkey) = pubkey {
@@ -189,15 +155,9 @@ impl Locator {
         } else {
             None
         };
-        let derivation_path = if let Some(derivation_path) = derivation_path {
-            Some(derivation_path.try_into().map_err(|e| e.into())?)
-        } else {
-            None
-        };
         Ok(Self {
             manufacturer,
             pubkey,
-            derivation_path,
         })
     }
 }
@@ -225,76 +185,50 @@ mod tests {
         let manufacturer_str = "ledger";
         let pubkey = Pubkey::new_unique();
         let pubkey_str = pubkey.to_string();
-        let derivation_path = DerivationPath::new_bip44(Some(0), Some(0));
-        let derivation_path_str = "0/0";
 
         let expect = Locator {
             manufacturer,
             pubkey: None,
-            derivation_path: None,
         };
         assert!(matches!(
-            Locator::new_from_parts(manufacturer, None::<Pubkey>, None::<DerivationPath>),
+            Locator::new_from_parts(manufacturer, None::<Pubkey>),
             Ok(e) if e == expect,
         ));
         assert!(matches!(
-            Locator::new_from_parts(manufacturer_str, None::<Pubkey>, None::<DerivationPath>),
+            Locator::new_from_parts(manufacturer_str, None::<Pubkey>),
             Ok(e) if e == expect,
         ));
 
         let expect = Locator {
             manufacturer,
             pubkey: Some(pubkey),
-            derivation_path: None,
-        };
-        assert!(matches!(
-            Locator::new_from_parts(manufacturer, Some(pubkey), None::<DerivationPath>),
-            Ok(e) if e == expect,
-        ));
-        assert!(matches!(
-            Locator::new_from_parts(manufacturer_str, Some(pubkey_str.as_str()), None::<DerivationPath>),
-            Ok(e) if e == expect,
-        ));
-
-        let expect = Locator {
-            manufacturer,
-            pubkey: None,
-            derivation_path: Some(derivation_path.clone()),
         };
         assert!(matches!(
-            Locator::new_from_parts(manufacturer, None::<Pubkey>, Some(derivation_path)),
+            Locator::new_from_parts(manufacturer, Some(pubkey)),
             Ok(e) if e == expect,
         ));
         assert!(matches!(
-            Locator::new_from_parts(manufacturer, None::<Pubkey>, Some(derivation_path_str)),
+            Locator::new_from_parts(manufacturer_str, Some(pubkey_str.as_str())),
             Ok(e) if e == expect,
         ));
 
         assert!(matches!(
-            Locator::new_from_parts("bad-manufacturer", None::<Pubkey>, None::<DerivationPath>),
+            Locator::new_from_parts("bad-manufacturer", None::<Pubkey>),
             Err(LocatorError::ManufacturerError(e)) if e == ManufacturerError,
         ));
         assert!(matches!(
-            Locator::new_from_parts(manufacturer, Some("bad-pubkey"), None::<DerivationPath>),
+            Locator::new_from_parts(manufacturer, Some("bad-pubkey")),
             Err(LocatorError::PubkeyError(e)) if e == ParsePubkeyError::Invalid,
         ));
-        let bad_path = "bad-derivation-path".to_string();
-        assert!(matches!(
-            Locator::new_from_parts(manufacturer, None::<Pubkey>, Some(bad_path.as_str())),
-            Err(LocatorError::DerivationPathError(
-                DerivationPathError::InvalidDerivationPath(_)
-            )),
-        ));
     }
 
     #[test]
     fn test_locator_new_from_uri() {
-        let derivation_path = DerivationPath::new_bip44(Some(0), Some(0));
         let manufacturer = Manufacturer::Ledger;
         let pubkey = Pubkey::new_unique();
         let pubkey_str = pubkey.to_string();
 
-        // usb://ledger/{PUBKEY}?key=0'/0'
+        // usb://ledger/{PUBKEY}?key=0/0
         let mut builder = URIReferenceBuilder::new();
         builder
             .try_scheme(Some("usb"))
@@ -309,7 +243,6 @@ mod tests {
         let expect = Locator {
             manufacturer,
             pubkey: Some(pubkey),
-            derivation_path: Some(derivation_path.clone()),
         };
         assert_eq!(Locator::new_from_uri(&uri), Ok(expect));
 
@@ -326,7 +259,6 @@ mod tests {
         let expect = Locator {
             manufacturer,
             pubkey: Some(pubkey),
-            derivation_path: None,
         };
         assert_eq!(Locator::new_from_uri(&uri), Ok(expect));
 
@@ -343,7 +275,6 @@ mod tests {
         let expect = Locator {
             manufacturer,
             pubkey: None,
-            derivation_path: None,
         };
         assert_eq!(Locator::new_from_uri(&uri), Ok(expect));
 
@@ -360,64 +291,6 @@ mod tests {
         let expect = Locator {
             manufacturer,
             pubkey: None,
-            derivation_path: None,
-        };
-        assert_eq!(Locator::new_from_uri(&uri), Ok(expect));
-
-        // usb://ledger?key=0/0
-        let mut builder = URIReferenceBuilder::new();
-        builder
-            .try_scheme(Some("usb"))
-            .unwrap()
-            .try_authority(Some(Manufacturer::Ledger.as_ref()))
-            .unwrap()
-            .try_path("")
-            .unwrap()
-            .try_query(Some("key=0/0"))
-            .unwrap();
-        let uri = builder.build().unwrap();
-        let expect = Locator {
-            manufacturer,
-            pubkey: None,
-            derivation_path: Some(derivation_path.clone()),
-        };
-        assert_eq!(Locator::new_from_uri(&uri), Ok(expect));
-
-        // usb://ledger?key=0'/0'
-        let mut builder = URIReferenceBuilder::new();
-        builder
-            .try_scheme(Some("usb"))
-            .unwrap()
-            .try_authority(Some(Manufacturer::Ledger.as_ref()))
-            .unwrap()
-            .try_path("")
-            .unwrap()
-            .try_query(Some("key=0'/0'"))
-            .unwrap();
-        let uri = builder.build().unwrap();
-        let expect = Locator {
-            manufacturer,
-            pubkey: None,
-            derivation_path: Some(derivation_path.clone()),
-        };
-        assert_eq!(Locator::new_from_uri(&uri), Ok(expect));
-
-        // usb://ledger/?key=0/0
-        let mut builder = URIReferenceBuilder::new();
-        builder
-            .try_scheme(Some("usb"))
-            .unwrap()
-            .try_authority(Some(Manufacturer::Ledger.as_ref()))
-            .unwrap()
-            .try_path("/")
-            .unwrap()
-            .try_query(Some("key=0/0"))
-            .unwrap();
-        let uri = builder.build().unwrap();
-        let expect = Locator {
-            manufacturer,
-            pubkey: None,
-            derivation_path: Some(derivation_path),
         };
         assert_eq!(Locator::new_from_uri(&uri), Ok(expect));
 
@@ -465,79 +338,10 @@ mod tests {
             Locator::new_from_uri(&uri),
             Err(LocatorError::PubkeyError(ParsePubkeyError::Invalid))
         );
-
-        // usb://ledger?bad-key=0/0
-        let mut builder = URIReferenceBuilder::new();
-        builder
-            .try_scheme(Some("usb"))
-            .unwrap()
-            .try_authority(Some(Manufacturer::Ledger.as_ref()))
-            .unwrap()
-            .try_path("")
-            .unwrap()
-            .try_query(Some("bad-key=0/0"))
-            .unwrap();
-        let uri = builder.build().unwrap();
-        assert!(matches!(
-            Locator::new_from_uri(&uri),
-            Err(LocatorError::DerivationPathError(_))
-        ));
-
-        // usb://ledger?key=bad-value
-        let mut builder = URIReferenceBuilder::new();
-        builder
-            .try_scheme(Some("usb"))
-            .unwrap()
-            .try_authority(Some(Manufacturer::Ledger.as_ref()))
-            .unwrap()
-            .try_path("")
-            .unwrap()
-            .try_query(Some("key=bad-value"))
-            .unwrap();
-        let uri = builder.build().unwrap();
-        assert!(matches!(
-            Locator::new_from_uri(&uri),
-            Err(LocatorError::DerivationPathError(_))
-        ));
-
-        // usb://ledger?key=
-        let mut builder = URIReferenceBuilder::new();
-        builder
-            .try_scheme(Some("usb"))
-            .unwrap()
-            .try_authority(Some(Manufacturer::Ledger.as_ref()))
-            .unwrap()
-            .try_path("")
-            .unwrap()
-            .try_query(Some("key="))
-            .unwrap();
-        let uri = builder.build().unwrap();
-        assert!(matches!(
-            Locator::new_from_uri(&uri),
-            Err(LocatorError::DerivationPathError(_))
-        ));
-
-        // usb://ledger?key
-        let mut builder = URIReferenceBuilder::new();
-        builder
-            .try_scheme(Some("usb"))
-            .unwrap()
-            .try_authority(Some(Manufacturer::Ledger.as_ref()))
-            .unwrap()
-            .try_path("")
-            .unwrap()
-            .try_query(Some("key"))
-            .unwrap();
-        let uri = builder.build().unwrap();
-        assert!(matches!(
-            Locator::new_from_uri(&uri),
-            Err(LocatorError::DerivationPathError(_))
-        ));
     }
 
     #[test]
     fn test_locator_new_from_path() {
-        let derivation_path = DerivationPath::new_bip44(Some(0), Some(0));
         let manufacturer = Manufacturer::Ledger;
         let pubkey = Pubkey::new_unique();
         let path = format!("usb://ledger/{}?key=0/0", pubkey);
@@ -548,7 +352,6 @@ mod tests {
         let expect = Locator {
             manufacturer,
             pubkey: Some(pubkey),
-            derivation_path: Some(derivation_path.clone()),
         };
         assert_eq!(Locator::new_from_path(path), Ok(expect));
 
@@ -557,7 +360,6 @@ mod tests {
         let expect = Locator {
             manufacturer,
             pubkey: Some(pubkey),
-            derivation_path: None,
         };
         assert_eq!(Locator::new_from_path(path), Ok(expect));
 
@@ -566,7 +368,6 @@ mod tests {
         let expect = Locator {
             manufacturer,
             pubkey: None,
-            derivation_path: None,
         };
         assert_eq!(Locator::new_from_path(path), Ok(expect));
 
@@ -575,25 +376,6 @@ mod tests {
         let expect = Locator {
             manufacturer,
             pubkey: None,
-            derivation_path: None,
-        };
-        assert_eq!(Locator::new_from_path(path), Ok(expect));
-
-        // usb://ledger?key=0'/0'
-        let path = "usb://ledger?key=0'/0'";
-        let expect = Locator {
-            manufacturer,
-            pubkey: None,
-            derivation_path: Some(derivation_path.clone()),
-        };
-        assert_eq!(Locator::new_from_path(path), Ok(expect));
-
-        // usb://ledger/?key=0'/0'
-        let path = "usb://ledger?key=0'/0'";
-        let expect = Locator {
-            manufacturer,
-            pubkey: None,
-            derivation_path: Some(derivation_path),
         };
         assert_eq!(Locator::new_from_path(path), Ok(expect));
 
@@ -617,40 +399,5 @@ mod tests {
             Locator::new_from_path(path),
             Err(LocatorError::PubkeyError(ParsePubkeyError::Invalid))
         );
-
-        // usb://ledger?bad-key=0/0
-        let path = "usb://ledger?bad-key=0/0";
-        assert!(matches!(
-            Locator::new_from_path(path),
-            Err(LocatorError::DerivationPathError(_))
-        ));
-
-        // usb://ledger?bad-key=0'/0'
-        let path = "usb://ledger?bad-key=0'/0'";
-        assert!(matches!(
-            Locator::new_from_path(path),
-            Err(LocatorError::DerivationPathError(_))
-        ));
-
-        // usb://ledger?key=bad-value
-        let path = format!("usb://ledger/{}?key=bad-value", pubkey);
-        assert!(matches!(
-            Locator::new_from_path(path),
-            Err(LocatorError::DerivationPathError(_))
-        ));
-
-        // usb://ledger?key=
-        let path = format!("usb://ledger/{}?key=", pubkey);
-        assert!(matches!(
-            Locator::new_from_path(path),
-            Err(LocatorError::DerivationPathError(_))
-        ));
-
-        // usb://ledger?key
-        let path = format!("usb://ledger/{}?key", pubkey);
-        assert!(matches!(
-            Locator::new_from_path(path),
-            Err(LocatorError::DerivationPathError(_))
-        ));
     }
 }

remote-wallet/src/remote_keypair.rs

@@ -1,7 +1,7 @@
 use {
     crate::{
         ledger::get_ledger_from_info,
-        locator::Manufacturer,
+        locator::{Locator, Manufacturer},
         remote_wallet::{
             RemoteWallet, RemoteWalletError, RemoteWalletInfo, RemoteWalletManager,
             RemoteWalletType,
@@ -56,12 +56,13 @@ impl Signer for RemoteKeypair {
 }
 
 pub fn generate_remote_keypair(
-    path: String,
+    locator: Locator,
+    derivation_path: DerivationPath,
     wallet_manager: &RemoteWalletManager,
     confirm_key: bool,
     keypair_name: &str,
 ) -> Result<RemoteKeypair, RemoteWalletError> {
-    let (remote_wallet_info, derivation_path) = RemoteWalletInfo::parse_path(path)?;
+    let remote_wallet_info = RemoteWalletInfo::parse_locator(locator);
     if remote_wallet_info.manufacturer == Manufacturer::Ledger {
         let ledger = get_ledger_from_info(remote_wallet_info, keypair_name, wallet_manager)?;
         let path = format!("{}{}", ledger.pretty_path, derivation_path.get_query());

remote-wallet/src/remote_wallet.rs

@@ -253,20 +253,12 @@ pub struct RemoteWalletInfo {
 }
 
 impl RemoteWalletInfo {
-    pub fn parse_path(path: String) -> Result<(Self, DerivationPath), RemoteWalletError> {
-        let Locator {
-            manufacturer,
-            pubkey,
-            derivation_path,
-        } = Locator::new_from_path(path)?;
-        Ok((
-            RemoteWalletInfo {
-                manufacturer,
-                pubkey: pubkey.unwrap_or_default(),
-                ..RemoteWalletInfo::default()
-            },
-            derivation_path.unwrap_or_default(),
-        ))
+    pub fn parse_locator(locator: Locator) -> Self {
+        RemoteWalletInfo {
+            manufacturer: locator.manufacturer,
+            pubkey: locator.pubkey.unwrap_or_default(),
+            ..RemoteWalletInfo::default()
+        }
     }
 
     pub fn get_pretty_path(&self) -> String {
@@ -308,32 +300,13 @@ mod tests {
     use super::*;
 
     #[test]
-    fn test_parse_path() {
+    fn test_parse_locator() {
         let pubkey = solana_sdk::pubkey::new_rand();
-        let (wallet_info, derivation_path) =
-            RemoteWalletInfo::parse_path(format!("usb://ledger/{:?}?key=1/2", pubkey)).unwrap();
-        assert!(wallet_info.matches(&RemoteWalletInfo {
-            model: "nano-s".to_string(),
-            manufacturer: Manufacturer::Ledger,
-            serial: "".to_string(),
-            host_device_path: "/host/device/path".to_string(),
-            pubkey,
-            error: None,
-        }));
-        assert_eq!(derivation_path, DerivationPath::new_bip44(Some(1), Some(2)));
-        let (wallet_info, derivation_path) =
-            RemoteWalletInfo::parse_path(format!("usb://ledger/{:?}?key=1'/2'", pubkey)).unwrap();
-        assert!(wallet_info.matches(&RemoteWalletInfo {
-            model: "nano-s".to_string(),
+        let locator = Locator {
             manufacturer: Manufacturer::Ledger,
-            serial: "".to_string(),
-            host_device_path: "/host/device/path".to_string(),
-            pubkey,
-            error: None,
-        }));
-        assert_eq!(derivation_path, DerivationPath::new_bip44(Some(1), Some(2)));
-        let (wallet_info, derivation_path) =
-            RemoteWalletInfo::parse_path(format!("usb://ledger/{:?}?key=1\'/2\'", pubkey)).unwrap();
+            pubkey: Some(pubkey),
+        };
+        let wallet_info = RemoteWalletInfo::parse_locator(locator);
         assert!(wallet_info.matches(&RemoteWalletInfo {
             model: "nano-s".to_string(),
             manufacturer: Manufacturer::Ledger,
@@ -342,46 +315,21 @@ mod tests {
             pubkey,
             error: None,
         }));
-        assert_eq!(derivation_path, DerivationPath::new_bip44(Some(1), Some(2)));
 
-        // Test that wallet id need not be complete for key derivation to work
-        let (wallet_info, derivation_path) =
-            RemoteWalletInfo::parse_path("usb://ledger?key=1".to_string()).unwrap();
-        assert!(wallet_info.matches(&RemoteWalletInfo {
-            model: "nano-s".to_string(),
+        // Test that pubkey need not be populated
+        let locator = Locator {
             manufacturer: Manufacturer::Ledger,
-            serial: "".to_string(),
-            host_device_path: "/host/device/path".to_string(),
-            pubkey: Pubkey::default(),
-            error: None,
-        }));
-        assert_eq!(derivation_path, DerivationPath::new_bip44(Some(1), None));
-        let (wallet_info, derivation_path) =
-            RemoteWalletInfo::parse_path("usb://ledger/?key=1/2".to_string()).unwrap();
+            pubkey: None,
+        };
+        let wallet_info = RemoteWalletInfo::parse_locator(locator);
         assert!(wallet_info.matches(&RemoteWalletInfo {
-            model: "".to_string(),
+            model: "nano-s".to_string(),
             manufacturer: Manufacturer::Ledger,
             serial: "".to_string(),
             host_device_path: "/host/device/path".to_string(),
             pubkey: Pubkey::default(),
             error: None,
         }));
-        assert_eq!(derivation_path, DerivationPath::new_bip44(Some(1), Some(2)));
-
-        // Failure cases
-        assert!(
-            RemoteWalletInfo::parse_path("usb://ledger/bad-pubkey?key=1/2".to_string()).is_err()
-        );
-        assert!(RemoteWalletInfo::parse_path("usb://?key=1/2".to_string()).is_err());
-        assert!(RemoteWalletInfo::parse_path("usb:/ledger?key=1/2".to_string()).is_err());
-        assert!(RemoteWalletInfo::parse_path("ledger?key=1/2".to_string()).is_err());
-        assert!(RemoteWalletInfo::parse_path("usb://ledger?key=1/2/3".to_string()).is_err());
-        // Other query strings cause an error
-        assert!(
-            RemoteWalletInfo::parse_path("usb://ledger/?key=1/2&test=other".to_string()).is_err()
-        );
-        assert!(RemoteWalletInfo::parse_path("usb://ledger/?Key=1/2".to_string()).is_err());
-        assert!(RemoteWalletInfo::parse_path("usb://ledger/?test=other".to_string()).is_err());
     }
 
     #[test]

sdk/Cargo.toml

@@ -44,36 +44,38 @@ byteorder = { version = "1.3.4", optional = true }
 chrono = { version = "0.4", optional = true }
 curve25519-dalek = { version = "2.1.0", optional = true }
 derivation-path = { version = "0.1.3", default-features = false }
+digest = { version = "0.9.0", optional = true }
+ed25519-dalek = { version = "=1.0.1", optional = true }
 generic-array = { version = "0.14.3", default-features = false, features = ["serde", "more_lengths"], optional = true }
 hex = "0.4.2"
 hmac = "0.10.1"
 itertools =  "0.9.0"
 lazy_static = "1.4.0"
+libsecp256k1 = { version = "0.3.5", optional = true }
 log = "0.4.11"
 memmap2 = { version = "0.1.0", optional = true }
 num-derive = "0.3"
 num-traits = "0.2"
 pbkdf2 = { version = "0.6.0", default-features = false }
+qstring = "0.7.2"
 rand = { version = "0.7.0", optional = true }
 rand_chacha = { version = "0.2.2", optional = true }
 rand_core = "0.6.2"
+rustversion = "1.0.4"
 serde = "1.0.122"
 serde_bytes = "0.11"
 serde_derive = "1.0.103"
 serde_json = { version = "1.0.56", optional = true }
 sha2 = "0.9.2"
-thiserror = "1.0"
-ed25519-dalek = { version = "=1.0.1", optional = true }
+sha3 = { version = "0.9.1", optional = true }
 solana-crate-features = { path = "../crate-features", version = "=1.7.0", optional = true }
 solana-logger = { path = "../logger", version = "=1.7.0", optional = true }
 solana-frozen-abi = { path = "../frozen-abi", version = "=1.7.0" }
 solana-frozen-abi-macro = { path = "../frozen-abi/macro", version = "=1.7.0" }
 solana-program = { path = "program", version = "=1.7.0" }
 solana-sdk-macro = { path = "macro", version = "=1.7.0" }
-rustversion = "1.0.4"
-libsecp256k1 = { version = "0.3.5", optional = true }
-sha3 = { version = "0.9.1", optional = true }
-digest = { version = "0.9.0", optional = true }
+thiserror = "1.0"
+uriparse = "0.6.3"
 
 [dev-dependencies]
 curve25519-dalek = "2.1.0"

sdk/src/derivation_path.rs

@@ -7,6 +7,7 @@ use {
         str::FromStr,
     },
     thiserror::Error,
+    uriparse::URIReference,
 };
 
 const ACCOUNT_INDEX: usize = 2;
@@ -109,6 +110,7 @@ impl DerivationPath {
         self.0.path()
     }
 
+    // Assumes `key` query-string key
     pub fn get_query(&self) -> String {
         if let Some(account) = &self.account() {
             if let Some(change) = &self.change() {
@@ -120,6 +122,34 @@ impl DerivationPath {
             "".to_string()
         }
     }
+
+    // Only accepts single query string pair of type `key`
+    pub fn from_uri(uri: &URIReference<'_>) -> Result<Option<Self>, DerivationPathError> {
+        if let Some(query) = uri.query() {
+            let query_str = query.as_str();
+            if query_str.is_empty() {
+                return Ok(None);
+            }
+            let query = qstring::QString::from(query_str);
+            if query.len() > 1 {
+                return Err(DerivationPathError::InvalidDerivationPath(
+                    "invalid query string, extra fields not supported".to_string(),
+                ));
+            }
+            let key = query.get("key");
+            if key.is_none() {
+                return Err(DerivationPathError::InvalidDerivationPath(format!(
+                    "invalid query string `{}`, only `key` supported",
+                    query_str,
+                )));
+            }
+            // Use from_key_str instead of TryInto here to make it a little more explicit that this
+            // generates a Solana bip44 DerivationPath
+            key.map(Self::from_key_str).transpose()
+        } else {
+            Ok(None)
+        }
+    }
 }
 
 impl fmt::Debug for DerivationPath {
@@ -161,6 +191,7 @@ impl Bip44 for Solana {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use uriparse::URIReferenceBuilder;
 
     struct TestCoin;
     impl Bip44 for TestCoin {
@@ -263,6 +294,190 @@ mod tests {
         );
     }
 
+    #[test]
+    fn test_new_from_uri() {
+        let derivation_path = DerivationPath::new_bip44(Some(0), Some(0));
+
+        // test://path?key=0/0
+        let mut builder = URIReferenceBuilder::new();
+        builder
+            .try_scheme(Some("test"))
+            .unwrap()
+            .try_authority(Some("path"))
+            .unwrap()
+            .try_path("")
+            .unwrap()
+            .try_query(Some("key=0/0"))
+            .unwrap();
+        let uri = builder.build().unwrap();
+        assert_eq!(
+            DerivationPath::from_uri(&uri).unwrap(),
+            Some(derivation_path.clone())
+        );
+
+        // test://path?key=0'/0'
+        let mut builder = URIReferenceBuilder::new();
+        builder
+            .try_scheme(Some("test"))
+            .unwrap()
+            .try_authority(Some("path"))
+            .unwrap()
+            .try_path("")
+            .unwrap()
+            .try_query(Some("key=0'/0'"))
+            .unwrap();
+        let uri = builder.build().unwrap();
+        assert_eq!(
+            DerivationPath::from_uri(&uri).unwrap(),
+            Some(derivation_path.clone())
+        );
+
+        // test://path?key=0\'/0\'
+        let mut builder = URIReferenceBuilder::new();
+        builder
+            .try_scheme(Some("test"))
+            .unwrap()
+            .try_authority(Some("path"))
+            .unwrap()
+            .try_path("")
+            .unwrap()
+            .try_query(Some("key=0\'/0\'"))
+            .unwrap();
+        let uri = builder.build().unwrap();
+        assert_eq!(
+            DerivationPath::from_uri(&uri).unwrap(),
+            Some(derivation_path)
+        );
+
+        // test://path
+        let mut builder = URIReferenceBuilder::new();
+        builder
+            .try_scheme(Some("test"))
+            .unwrap()
+            .try_authority(Some("path"))
+            .unwrap()
+            .try_path("")
+            .unwrap();
+        let uri = builder.build().unwrap();
+        assert_eq!(DerivationPath::from_uri(&uri).unwrap(), None);
+
+        // test://path?
+        let mut builder = URIReferenceBuilder::new();
+        builder
+            .try_scheme(Some("test"))
+            .unwrap()
+            .try_authority(Some("path"))
+            .unwrap()
+            .try_path("")
+            .unwrap()
+            .try_query(Some(""))
+            .unwrap();
+        let uri = builder.build().unwrap();
+        assert_eq!(DerivationPath::from_uri(&uri).unwrap(), None);
+
+        // test://path?key=0/0/0
+        let mut builder = URIReferenceBuilder::new();
+        builder
+            .try_scheme(Some("test"))
+            .unwrap()
+            .try_authority(Some("path"))
+            .unwrap()
+            .try_path("")
+            .unwrap()
+            .try_query(Some("key=0/0/0"))
+            .unwrap();
+        let uri = builder.build().unwrap();
+        assert!(matches!(
+            DerivationPath::from_uri(&uri),
+            Err(DerivationPathError::InvalidDerivationPath(_))
+        ));
+
+        // test://path?key=0/0&bad-key=0/0
+        let mut builder = URIReferenceBuilder::new();
+        builder
+            .try_scheme(Some("test"))
+            .unwrap()
+            .try_authority(Some("path"))
+            .unwrap()
+            .try_path("")
+            .unwrap()
+            .try_query(Some("key=0/0&bad-key=0/0"))
+            .unwrap();
+        let uri = builder.build().unwrap();
+        assert!(matches!(
+            DerivationPath::from_uri(&uri),
+            Err(DerivationPathError::InvalidDerivationPath(_))
+        ));
+
+        // test://path?bad-key=0/0
+        let mut builder = URIReferenceBuilder::new();
+        builder
+            .try_scheme(Some("test"))
+            .unwrap()
+            .try_authority(Some("path"))
+            .unwrap()
+            .try_path("")
+            .unwrap()
+            .try_query(Some("bad-key=0/0"))
+            .unwrap();
+        let uri = builder.build().unwrap();
+        assert!(matches!(
+            DerivationPath::from_uri(&uri),
+            Err(DerivationPathError::InvalidDerivationPath(_))
+        ));
+
+        // test://path?key=bad-value
+        let mut builder = URIReferenceBuilder::new();
+        builder
+            .try_scheme(Some("test"))
+            .unwrap()
+            .try_authority(Some("path"))
+            .unwrap()
+            .try_path("")
+            .unwrap()
+            .try_query(Some("key=bad-value"))
+            .unwrap();
+        let uri = builder.build().unwrap();
+        assert!(matches!(
+            DerivationPath::from_uri(&uri),
+            Err(DerivationPathError::InvalidDerivationPath(_))
+        ));
+
+        // test://path?key=
+        let mut builder = URIReferenceBuilder::new();
+        builder
+            .try_scheme(Some("test"))
+            .unwrap()
+            .try_authority(Some("path"))
+            .unwrap()
+            .try_path("")
+            .unwrap()
+            .try_query(Some("key="))
+            .unwrap();
+        let uri = builder.build().unwrap();
+        assert!(matches!(
+            DerivationPath::from_uri(&uri),
+            Err(DerivationPathError::InvalidDerivationPath(_))
+        ));
+
+        // test://path?key
+        let mut builder = URIReferenceBuilder::new();
+        builder
+            .try_scheme(Some("test"))
+            .unwrap()
+            .try_authority(Some("path"))
+            .unwrap()
+            .try_path("")
+            .unwrap()
+            .try_query(Some("key"))
+            .unwrap();
+        let uri = builder.build().unwrap();
+        assert!(matches!(
+            DerivationPath::from_uri(&uri),
+            Err(DerivationPathError::InvalidDerivationPath(_))
+        ));
+    }
+
     #[test]
     fn test_get_query() {
         let derivation_path = DerivationPath::new_bip44_with_coin(TestCoin, None, None);