Address pr feedback.

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, which `hd_path` to derive the key at. Persisted on the identity (or its Secure Store entry) so later commands derive the same account without re-passing the flag. Not valid with `--public-key` or a raw secret key
+- `--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):**
 

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/soroban-cli/src/commands/keys/add.rs

@@ -42,9 +42,6 @@ pub enum Error {
 
     #[error("--hd-path is not valid with a secret key; secret keys cannot be derived")]
     HdPathNotSupportedForSecretKey,
-
-    #[error("--hd-path is not valid with --public-key; public keys cannot be derived")]
-    HdPathNotSupportedForPublicKey,
 }
 
 #[derive(Debug, clap::Parser, Clone)]
@@ -60,17 +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, which `hd_path` to derive the key at. Persisted on
-    /// the identity (or its Secure Store entry) so later commands derive the same account
-    /// without re-passing the flag. Not valid with `--public-key` or a raw secret key.
+    /// 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>,
 }
@@ -79,10 +82,6 @@ impl Cmd {
     pub fn run(&self, global_args: &global::Args) -> Result<(), Error> {
         let print = Print::new(global_args.quiet);
 
-        if self.public_key.is_some() && self.hd_path.is_some() {
-            return Err(Error::HdPathNotSupportedForPublicKey);
-        }
-
         if self.config_locator.read_identity(&self.name).is_ok() {
             if !self.overwrite {
                 return Err(Error::IdentityAlreadyExists(self.name.to_string()));
@@ -268,10 +267,22 @@ mod tests {
     }
 
     #[test]
-    fn test_run_rejects_hd_path_with_public_key() {
-        let (_tmp, _locator, cmd) = cmd_with_public_key(PUBLIC_KEY, Some(3));
-        let result = cmd.run(&global_args());
-        assert!(matches!(result, Err(Error::HdPathNotSupportedForPublicKey)));
+    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]

cmd/soroban-cli/src/config/locator.rs

@@ -322,14 +322,19 @@ impl Args {
         if let Key::Secret(Secret::SecureStore {
             entry_name,
             public_key: None,
-            ..
+            hd_path: persisted_hd_path,
         }) = &key
         {
-            let pk = secure_store::get_public_key(entry_name, hd_path)?;
+            // Honor the persisted hd_path when the caller passes None. Without
+            // this the cache gets populated at index 0 even when the identity
+            // was added with `--hd-path N`, which silently locks every later
+            // read to the wrong account.
+            let effective = hd_path.or(*persisted_hd_path);
+            let pk = secure_store::get_public_key(entry_name, effective)?;
             let migrated = Key::Secret(Secret::SecureStore {
                 entry_name: entry_name.clone(),
                 public_key: Some(pk.to_string()),
-                hd_path,
+                hd_path: effective,
             });
             // Best-effort write-back: if persistence fails we still return the
             // freshly-derived value so the current call succeeds.

cmd/soroban-cli/src/config/secret.rs

@@ -182,7 +182,7 @@ impl Secret {
                 let hd_path: u32 = hd_path
                     .unwrap_or_default()
                     .try_into()
-                    .expect("uszie bigger than u32");
+                    .expect("usize bigger than u32");
                 SignerKind::Ledger(ledger::new(hd_path).await?)
             }
             Secret::SecureStore {