From b495b3f42233fd129d3e3e9eec5fe681d3335fb1 Mon Sep 17 00:00:00 2001 From: mootz12 Date: Mon, 16 Mar 2026 13:23:14 -0400 Subject: [PATCH 1/3] feat: add user confirmation for keys rm command --- FULL_HELP_DOCS.md | 4 + .../soroban-test/tests/it/integration/keys.rs | 112 ++++++++++++++++++ cmd/soroban-cli/src/commands/keys/rm.rs | 31 +++++ cookbook/stellar-keys.mdx | 2 +- 4 files changed, 148 insertions(+), 1 deletion(-) diff --git a/FULL_HELP_DOCS.md b/FULL_HELP_DOCS.md index e197e0238a..6f21219ee6 100644 --- a/FULL_HELP_DOCS.md +++ b/FULL_HELP_DOCS.md @@ -1283,6 +1283,10 @@ Remove an identity - `` — Identity to remove +###### **Options:** + +- `--force` — Skip confirmation prompt + ###### **Options (Global):** - `--global` — ⚠️ Deprecated: global config is always on diff --git a/cmd/crates/soroban-test/tests/it/integration/keys.rs b/cmd/crates/soroban-test/tests/it/integration/keys.rs index 311a60bc2d..6047827318 100644 --- a/cmd/crates/soroban-test/tests/it/integration/keys.rs +++ b/cmd/crates/soroban-test/tests/it/integration/keys.rs @@ -210,3 +210,115 @@ async fn unset_default_identity() { .stdout(predicate::str::contains("STELLAR_ACCOUNT=").not()) .success(); } + +#[tokio::test] +async fn rm_requires_confirmation() { + let sandbox = &TestEnv::new(); + sandbox + .new_assert_cmd("keys") + .arg("generate") + .arg("rmtest1") + .assert() + .success(); + + // Piping "n" should cancel removal + sandbox + .new_assert_cmd("keys") + .arg("rm") + .arg("rmtest1") + .write_stdin("n\n") + .assert() + .stderr(predicate::str::contains("removal cancelled by user")) + .failure(); + + sandbox + .new_assert_cmd("keys") + .arg("address") + .arg("rmtest1") + .assert() + .success(); + + // Piping empty input (just Enter) should default to cancel + sandbox + .new_assert_cmd("keys") + .arg("rm") + .arg("rmtest1") + .write_stdin("\n") + .assert() + .stderr(predicate::str::contains("removal cancelled by user")) + .failure(); + + sandbox + .new_assert_cmd("keys") + .arg("address") + .arg("rmtest1") + .assert() + .success(); + + // Piping "y" should confirm removal + sandbox + .new_assert_cmd("keys") + .arg("rm") + .arg("rmtest1") + .write_stdin("y\n") + .assert() + .stderr(predicate::str::contains( + "Removing the key's cli config file", + )) + .success(); + + sandbox + .new_assert_cmd("keys") + .arg("address") + .arg("rmtest1") + .assert() + .failure(); +} + +#[tokio::test] +async fn rm_with_force_skips_confirmation() { + let sandbox = &TestEnv::new(); + sandbox + .new_assert_cmd("keys") + .arg("generate") + .arg("rmtest2") + .assert() + .success(); + + sandbox + .new_assert_cmd("keys") + .arg("rm") + .arg("rmtest2") + .arg("--force") + .assert() + .success(); + + sandbox + .new_assert_cmd("keys") + .arg("address") + .arg("rmtest2") + .assert() + .failure(); +} + +#[tokio::test] +async fn rm_nonexistent_key() { + let sandbox = &TestEnv::new(); + + // Without --force: should fail before prompting + sandbox + .new_assert_cmd("keys") + .arg("rm") + .arg("doesnotexist") + .assert() + .failure(); + + // With --force: should still fail + sandbox + .new_assert_cmd("keys") + .arg("rm") + .arg("doesnotexist") + .arg("--force") + .assert() + .failure(); +} diff --git a/cmd/soroban-cli/src/commands/keys/rm.rs b/cmd/soroban-cli/src/commands/keys/rm.rs index c0ba979092..9cd5108821 100644 --- a/cmd/soroban-cli/src/commands/keys/rm.rs +++ b/cmd/soroban-cli/src/commands/keys/rm.rs @@ -1,5 +1,8 @@ +use std::io::{self, BufRead, IsTerminal}; + use crate::commands::global; use crate::config::address::KeyName; +use crate::print::Print; use super::super::config::locator; @@ -7,6 +10,10 @@ use super::super::config::locator; pub enum Error { #[error(transparent)] Locator(#[from] locator::Error), + #[error("removal cancelled by user")] + Cancelled, + #[error(transparent)] + Io(#[from] io::Error), } #[derive(Debug, clap::Parser, Clone)] @@ -15,12 +22,36 @@ pub struct Cmd { /// Identity to remove pub name: KeyName, + /// Skip confirmation prompt + #[arg(long)] + pub force: bool, + #[command(flatten)] pub config: locator::Args, } impl Cmd { pub fn run(&self, global_args: &global::Args) -> Result<(), Error> { + if !self.force { + let print = Print::new(global_args.quiet); + let stdin = io::stdin(); + + // Check that the key exists before asking for confirmation + self.config.read_identity(&self.name)?; + + // Show the prompt only when the user can see it + if stdin.is_terminal() { + print.warnln(format!( + "Are you sure you want to remove the key '{}'? This action cannot be undone. (y/N)", + self.name + )); + } + let mut response = String::new(); + stdin.lock().read_line(&mut response)?; + if !response.trim().eq_ignore_ascii_case("y") { + return Err(Error::Cancelled); + } + } Ok(self.config.remove_identity(&self.name, global_args)?) } } diff --git a/cookbook/stellar-keys.mdx b/cookbook/stellar-keys.mdx index bccf61bef3..1356f12c4c 100644 --- a/cookbook/stellar-keys.mdx +++ b/cookbook/stellar-keys.mdx @@ -112,7 +112,7 @@ You can also fund the account while creating the key by using `stellar keys gene When you no longer need this identity, remove it using: ```bash -stellar keys rm carol +stellar keys rm carol --force ``` Output: From a45341ef99acf02d57c43f31d3c2e088507f845c Mon Sep 17 00:00:00 2001 From: mootz12 Date: Tue, 7 Apr 2026 20:06:19 -0400 Subject: [PATCH 2/3] chore: correctly error for local keys and add key file path to user prompt --- cmd/soroban-cli/src/commands/keys/rm.rs | 21 ++++++++++++++------ cmd/soroban-cli/src/config/locator.rs | 26 +++++++++++++++++++++---- 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/cmd/soroban-cli/src/commands/keys/rm.rs b/cmd/soroban-cli/src/commands/keys/rm.rs index 9cd5108821..173640fc30 100644 --- a/cmd/soroban-cli/src/commands/keys/rm.rs +++ b/cmd/soroban-cli/src/commands/keys/rm.rs @@ -2,10 +2,9 @@ use std::io::{self, BufRead, IsTerminal}; use crate::commands::global; use crate::config::address::KeyName; +use crate::config::locator::{self, KeyType, Location}; use crate::print::Print; -use super::super::config::locator; - #[derive(thiserror::Error, Debug)] pub enum Error { #[error(transparent)] @@ -14,6 +13,10 @@ pub enum Error { Cancelled, #[error(transparent)] Io(#[from] io::Error), + #[error( + "please migrate from local storage using `stellar config migrate` before removing keys" + )] + LocalStorage(), } #[derive(Debug, clap::Parser, Clone)] @@ -33,17 +36,23 @@ pub struct Cmd { impl Cmd { pub fn run(&self, global_args: &global::Args) -> Result<(), Error> { if !self.force { - let print = Print::new(global_args.quiet); + let print = Print::new(false); let stdin = io::stdin(); // Check that the key exists before asking for confirmation - self.config.read_identity(&self.name)?; + let (_, location) = self.config.read_identity_with_location(&self.name)?; + // TODO: Remove check for local storage once it's no longer supported + if let Location::Local(_) = location { + return Err(Error::LocalStorage()); + } // Show the prompt only when the user can see it if stdin.is_terminal() { + let config_path = KeyType::Identity.path(location.as_ref(), &self.name); print.warnln(format!( - "Are you sure you want to remove the key '{}'? This action cannot be undone. (y/N)", - self.name + "Are you sure you want to remove the key '{}' at '{}'? This action cannot be undone. (y/N)", + self.name, + config_path.display() )); } let mut response = String::new(); diff --git a/cmd/soroban-cli/src/config/locator.rs b/cmd/soroban-cli/src/config/locator.rs index 9237f582da..b07ae3a9da 100644 --- a/cmd/soroban-cli/src/config/locator.rs +++ b/cmd/soroban-cli/src/config/locator.rs @@ -120,6 +120,7 @@ pub struct Args { pub config_dir: Option, } +#[derive(Clone)] pub enum Location { Local(PathBuf), Global(PathBuf), @@ -288,6 +289,12 @@ impl Args { KeyType::Identity.read_with_global(name, self) } + // TODO: Remove once local storage is no longer supported + pub fn read_identity_with_location(&self, name: &str) -> Result<(Key, Location), Error> { + utils::validate_name(name)?; + KeyType::Identity.read_with_global_with_location(name, self) + } + pub fn read_key(&self, key_or_name: &str) -> Result { key_or_name .parse() @@ -643,15 +650,23 @@ impl KeyType { key: &str, locator: &Args, ) -> Result { + Ok(self.read_with_global_with_location(key, locator)?.0) + } + + pub fn read_with_global_with_location( + &self, + key: &str, + locator: &Args, + ) -> Result<(T, Location), Error> { for location in locator.local_and_global()? { let path = self.path(location.as_ref(), key); if let Ok(t) = Self::read_from_path(&path) { - if let Location::Local(config_dir) = location { + if let Location::Local(config_dir) = location.clone() { print_deprecation_warning(&config_dir); } - return Ok(t); + return Ok((t, location)); } } Err(Error::ConfigMissing(self.to_string(), key.to_string())) @@ -702,9 +717,12 @@ impl KeyType { pwd.join(self.to_string()) } - fn path(&self, pwd: &Path, key: &str) -> PathBuf { + pub fn path(&self, pwd: &Path, key: &str) -> PathBuf { let mut path = self.root(pwd).join(key); - path.set_extension("toml"); + match self { + KeyType::Identity | KeyType::Network => path.set_extension("toml"), + KeyType::ContractIds => path.set_extension("json"), + }; path } From ed1e40bc352450be7b5f1ce4fa7548ba2aaf57d6 Mon Sep 17 00:00:00 2001 From: mootz12 Date: Wed, 8 Apr 2026 11:48:32 -0400 Subject: [PATCH 3/3] chore: update rust version --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 3bdbcf39e2..44cafa2366 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,7 +22,7 @@ exclude = [ [workspace.package] version = "25.2.0" -rust-version = "1.89.0" +rust-version = "1.92.0" # Dependencies located in this repo: [workspace.dependencies.soroban-cli]