Persist --hd-path on plain seed-phrase keys (#2540)

Author: fnando

FULL_HELP_DOCS.md

@@ -1138,7 +1138,7 @@ Add a new identity (keypair, ledger, OS specific secure store)
 
 - `--public-key <PUBLIC_KEY>` — Add a public key, ed25519, or muxed account, e.g. G1.., M2..
 - `--overwrite` — Overwrite existing identity if it already exists. When combined with --secure-store, also replaces the existing Secure Store entry
-- `--hd-path <HD_PATH>` — When importing a seed phrase into the Secure Store, which `hd_path` to derive the key at
+- `--hd-path <HD_PATH>` — When importing a seed phrase, which `hd_path` to derive the key at. Persisted on the identity so later commands derive the same account without re-passing the flag. Not valid with `--public-key` or a raw secret key
 
 ###### **Options (Global):**
 
@@ -1209,7 +1209,7 @@ Generate a new identity using a 24-word seed phrase The seed phrase can be store
 
   On Mac this uses Keychain, on Windows it is Secure Store Service, and on \*nix platforms it uses a combination of the kernel keyutils and DBus-based Secret Service.
 
-- `--hd-path <HD_PATH>` — With `--as-secret` or `--secure-store`, which `hd_path` to derive the key at from the seed phrase
+- `--hd-path <HD_PATH>` — Which `hd_path` to derive the key at from the seed phrase. Honored across all storage modes: with `--as-secret` it picks which derived key is stored, with `--secure-store` or plain seed-phrase storage it is persisted on the identity so later commands derive the same account without re-passing the flag
 - `--fund` — Fund generated key pair
 
   Default value: `false`

cmd/crates/soroban-test/tests/it/integration/hello_world.rs

@@ -71,7 +71,7 @@ async fn invoke_contract() {
     let dir = sandbox.config_dir();
     let seed_phrase = std::fs::read_to_string(dir.join("identity/test.toml")).unwrap();
     let s = toml::from_str::<secret::Secret>(&seed_phrase).unwrap();
-    let secret::Secret::SeedPhrase { seed_phrase } = s else {
+    let secret::Secret::SeedPhrase { seed_phrase, .. } = s else {
         panic!("Expected seed phrase")
     };
 

cmd/crates/soroban-test/tests/it/integration/keys.rs

@@ -351,6 +351,79 @@ async fn rm_with_force_skips_confirmation() {
         .failure();
 }
 
+// `keys generate --hd-path N` (plain seed-phrase storage) must persist N so
+// that later `keys address` calls without `--hd-path` derive at index N rather
+// than the default. Guards the user-visible contract from #2538 across CLI
+// parsing, identity-file I/O, and key derivation.
+#[tokio::test]
+async fn hd_path_persists_for_keys_generate() {
+    let sandbox = &TestEnv::new();
+    sandbox
+        .new_assert_cmd("keys")
+        .args(["generate", "hd-gen", "--hd-path", "5"])
+        .assert()
+        .success();
+
+    let address_default = pubkey_for_identity(sandbox, "hd-gen");
+    let address_explicit = sandbox
+        .new_assert_cmd("keys")
+        .args(["address", "hd-gen", "--hd-path", "5"])
+        .assert()
+        .success()
+        .stdout_as_str();
+    let address_zero = sandbox
+        .new_assert_cmd("keys")
+        .args(["address", "hd-gen", "--hd-path", "0"])
+        .assert()
+        .success()
+        .stdout_as_str();
+
+    assert_eq!(
+        address_default, address_explicit,
+        "expected `keys address hd-gen` (no flag) to derive at the persisted hd_path 5"
+    );
+    assert_ne!(
+        address_default, address_zero,
+        "expected hd_path 5 derivation to differ from hd_path 0"
+    );
+}
+
+#[tokio::test]
+async fn hd_path_persists_for_keys_add_seed_phrase() {
+    let sandbox = &TestEnv::new();
+    let seed_phrase = "aisle reflect depart add safe fury dress artist bronze abuse warrior clap inquiry ask mandate deputy view trade debate flip priority boy depart recipe";
+
+    sandbox
+        .new_assert_cmd("keys")
+        .write_stdin(format!("{seed_phrase}\n"))
+        .args(["add", "hd-add", "--hd-path", "5"])
+        .assert()
+        .success();
+
+    let address_default = pubkey_for_identity(sandbox, "hd-add");
+    let address_explicit = sandbox
+        .new_assert_cmd("keys")
+        .args(["address", "hd-add", "--hd-path", "5"])
+        .assert()
+        .success()
+        .stdout_as_str();
+    let address_zero = sandbox
+        .new_assert_cmd("keys")
+        .args(["address", "hd-add", "--hd-path", "0"])
+        .assert()
+        .success()
+        .stdout_as_str();
+
+    assert_eq!(
+        address_default, address_explicit,
+        "expected `keys address hd-add` (no flag) to derive at the persisted hd_path 5"
+    );
+    assert_ne!(
+        address_default, address_zero,
+        "expected hd_path 5 derivation to differ from hd_path 0"
+    );
+}
+
 #[tokio::test]
 async fn rm_nonexistent_key() {
     let sandbox = &TestEnv::new();

cmd/crates/soroban-test/tests/it/integration/secure_store.rs

@@ -169,4 +169,35 @@ async fn secure_store_key_management() {
         "expected cached public_key to match the address derived at hd_path 5, \
          but identity file was:\n{identity_toml}"
     );
+
+    // Strip the cached public_key but keep hd_path = 5: simulates a legacy
+    // identity that persisted hd_path but never cached the derived pubkey.
+    // `keys address` without --hd-path must rederive at the persisted index 5
+    // and write the index-5 pubkey back to the cache — not silently overwrite
+    // it with the index-0 pubkey.
+    let stripped: String = identity_toml
+        .lines()
+        .filter(|line| !line.trim_start().starts_with("public_key"))
+        .collect::<Vec<_>>()
+        .join("\n");
+    std::fs::write(&identity_path, stripped).unwrap();
+
+    let address_after_rederive = sandbox
+        .new_assert_cmd("keys")
+        .args(["address", add_hd_path])
+        .assert()
+        .success()
+        .stdout_as_str();
+    assert_eq!(
+        address_after_rederive.trim(),
+        address_hd_path.trim(),
+        "expected `keys address {add_hd_path}` (no --hd-path) to derive at the persisted hd_path 5"
+    );
+
+    let identity_toml = std::fs::read_to_string(&identity_path).unwrap();
+    assert!(
+        identity_toml.contains(&format!("public_key = \"{}\"", address_hd_path.trim())),
+        "expected cache write-back to use the persisted hd_path, \
+         but identity file was:\n{identity_toml}"
+    );
 }

cmd/crates/soroban-test/tests/it/util.rs

@@ -18,6 +18,7 @@ pub fn add_key(dir: &Path, name: &str, kind: SecretKind, data: &str) {
     let secret = match kind {
         SecretKind::Seed => Secret::SeedPhrase {
             seed_phrase: data.to_string(),
+            hd_path: None,
         },
         SecretKind::Key => Secret::SecretKey {
             secret_key: data.to_string(),

cmd/soroban-cli/src/commands/keys/add.rs

@@ -39,6 +39,9 @@ pub enum Error {
          unset STELLAR_SECRET_KEY or provide a seed phrase instead"
     )]
     SecureStoreRequiresSeedPhrase,
+
+    #[error("--hd-path is not valid with a secret key; secret keys cannot be derived")]
+    HdPathNotSupportedForSecretKey,
 }
 
 #[derive(Debug, clap::Parser, Clone)]
@@ -54,15 +57,23 @@ pub struct Cmd {
     pub config_locator: locator::Args,
 
     /// Add a public key, ed25519, or muxed account, e.g. G1.., M2..
-    #[arg(long, conflicts_with = "seed_phrase", conflicts_with = "secret_key")]
+    #[arg(
+        long,
+        conflicts_with = "seed_phrase",
+        conflicts_with = "secret_key",
+        conflicts_with = "hd_path"
+    )]
     pub public_key: Option<String>,
 
     /// Overwrite existing identity if it already exists. When combined with
     /// --secure-store, also replaces the existing Secure Store entry.
     #[arg(long)]
     pub overwrite: bool,
 
-    /// When importing a seed phrase into the Secure Store, which `hd_path` to derive the key at.
+    /// When importing a seed phrase, which `hd_path` to derive the key at.
+    /// Persisted on the identity so later commands derive the same account
+    /// without re-passing the flag. Not valid with `--public-key` or a raw
+    /// secret key.
     #[arg(long)]
     pub hd_path: Option<usize>,
 }
@@ -98,7 +109,7 @@ impl Cmd {
                 return Err(Error::SecureStoreRequiresSeedPhrase);
             }
         } else if let Ok(secret_key) = std::env::var("STELLAR_SECRET_KEY") {
-            return Ok(secret_key.parse()?);
+            return build_secret(&secret_key, self.hd_path);
         }
 
         if self.secrets.secure_store {
@@ -123,8 +134,8 @@ impl Cmd {
         } else {
             let prompt = "Type a secret key or 12/24 word seed phrase:";
             let secret_key = read_password(print, prompt)?;
-            let secret = secret_key.parse()?;
-            if let Secret::SeedPhrase { seed_phrase } = &secret {
+            let secret = build_secret(&secret_key, self.hd_path)?;
+            if let Secret::SeedPhrase { seed_phrase, .. } = &secret {
                 if seed_phrase.split_whitespace().count() < 24 {
                     print.warnln("The provided seed phrase lacks sufficient entropy and should be avoided. Using a 24-word seed phrase is a safer option.".to_string());
                     print.warnln(
@@ -138,6 +149,18 @@ impl Cmd {
     }
 }
 
+fn build_secret(input: &str, hd_path: Option<usize>) -> Result<Secret, Error> {
+    let secret: Secret = input.parse()?;
+    match (secret, hd_path) {
+        (Secret::SecretKey { .. }, Some(_)) => Err(Error::HdPathNotSupportedForSecretKey),
+        (Secret::SeedPhrase { seed_phrase, .. }, hd_path) => Ok(Secret::SeedPhrase {
+            seed_phrase,
+            hd_path,
+        }),
+        (secret, _) => Ok(secret),
+    }
+}
+
 fn read_password(print: &Print, prompt: &str) -> Result<String, Error> {
     if std::io::stdin().is_terminal() {
         // Interactive: prompt and read from TTY
@@ -190,13 +213,84 @@ mod tests {
         (temp_dir, locator, cmd)
     }
 
+    fn cmd_with_public_key(
+        public_key: &str,
+        hd_path: Option<usize>,
+    ) -> (tempfile::TempDir, locator::Args, Cmd) {
+        let (temp_dir, locator, mut cmd) = set_up_test();
+        cmd.public_key = Some(public_key.to_string());
+        cmd.hd_path = hd_path;
+        (temp_dir, locator, cmd)
+    }
+
     fn global_args() -> global::Args {
         global::Args {
             quiet: true,
             ..Default::default()
         }
     }
 
+    #[test]
+    fn test_build_secret_persists_hd_path_on_seed_phrase() {
+        let secret = build_secret(SEED_PHRASE, Some(5)).unwrap();
+        match secret {
+            Secret::SeedPhrase {
+                seed_phrase,
+                hd_path,
+            } => {
+                assert_eq!(seed_phrase, SEED_PHRASE);
+                assert_eq!(hd_path, Some(5));
+            }
+            other => panic!("expected SeedPhrase variant, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn test_build_secret_seed_phrase_without_hd_path() {
+        let secret = build_secret(SEED_PHRASE, None).unwrap();
+        match secret {
+            Secret::SeedPhrase { hd_path, .. } => assert_eq!(hd_path, None),
+            other => panic!("expected SeedPhrase variant, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn test_build_secret_rejects_hd_path_with_secret_key() {
+        let result = build_secret(SECRET_KEY, Some(5));
+        assert!(matches!(result, Err(Error::HdPathNotSupportedForSecretKey)));
+    }
+
+    #[test]
+    fn test_build_secret_secret_key_without_hd_path() {
+        let secret = build_secret(SECRET_KEY, None).unwrap();
+        assert!(matches!(secret, Secret::SecretKey { .. }));
+    }
+
+    #[test]
+    fn test_clap_rejects_hd_path_with_public_key() {
+        // clap-level conflict: --public-key cannot be combined with --hd-path.
+        // Driving through `try_parse_from` rather than constructing `Cmd`
+        // directly is what exercises the conflict.
+        use clap::Parser;
+
+        let result = Cmd::try_parse_from([
+            "add",
+            "test_name",
+            "--public-key",
+            PUBLIC_KEY,
+            "--hd-path",
+            "3",
+        ]);
+        let err = result.expect_err("clap must reject --public-key + --hd-path");
+        assert_eq!(err.kind(), clap::error::ErrorKind::ArgumentConflict);
+    }
+
+    #[test]
+    fn test_run_accepts_public_key_without_hd_path() {
+        let (_tmp, _locator, cmd) = cmd_with_public_key(PUBLIC_KEY, None);
+        assert!(cmd.run(&global_args()).is_ok());
+    }
+
     #[test]
     fn public_key_flag_accepts_public_key() {
         let (_tmp, locator, mut cmd) = set_up_test();