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.

Sign up for GitHub

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

chore(iota)!: Set active env when adding new env if empty #5330

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

chore(iota)!: Set active env when adding new env if empty #5330

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
874f89b
chore(iota): Set active env when adding new env if empty
DaughterOfMars Feb 11, 2025
90167d3
skip prompting if calling new-env
DaughterOfMars Feb 11, 2025
135d703
review
DaughterOfMars Feb 11, 2025
201921a
more clippy
DaughterOfMars Feb 11, 2025
95c6bf5
Replace get_active_env with context version and fix mut access
DaughterOfMars Feb 11, 2025
978e3bb
comment
DaughterOfMars Feb 11, 2025
f9a099d
clippy
DaughterOfMars Feb 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 13 additions & 13 deletions crates/iota-sdk/src/iota_client_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,17 +79,18 @@ impl IotaClientConfig {
}

/// Get the first [`IotaEnv`] or one by its alias.
pub fn get_env(&self, alias: &Option<String>) -> Option<&IotaEnv> {
if let Some(alias) = alias {
self.envs.iter().find(|env| &env.alias == alias)
} else {
self.envs.first()
}
pub fn get_env(&self, alias: &str) -> Option<&IotaEnv> {
Copy link
Member

Choose a reason for hiding this comment

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

I don't really know how to feel about this change tbh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How so?

self.envs.iter().find(|env| &env.alias == alias)
DaughterOfMars marked this conversation as resolved.
Show resolved Hide resolved
}

/// Get the active [`IotaEnv`].
pub fn get_active_env(&self) -> Result<&IotaEnv, anyhow::Error> {
self.get_env(&self.active_env).ok_or_else(|| {
if let Some(active_env) = &self.active_env {
self.get_env(active_env)
} else {
self.envs.first()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

should we also set self.active_env to self.envs.first() then? Feels weird that we claim to have an active env but the internal state of the object is different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not a mutable fn and I don't think it should be

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but the API is kind of lying to the user, because the object's internal state doesn't reflect what the method returns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that's true. If there's no env the default is the first, which is represented in the state

Copy link
Contributor

Choose a reason for hiding this comment

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

currently get_active_env() and active_env() (autoimplemented getter) may return different values, and I don't think that's good.

.ok_or_else(|| {
anyhow!(
"Environment configuration not found for env [{}]",
self.active_env.as_deref().unwrap_or("None")
Expand All @@ -99,12 +100,11 @@ impl IotaClientConfig {

/// Add an [`IotaEnv`].
pub fn add_env(&mut self, env: IotaEnv) {
if !self
.envs
.iter()
.any(|other_env| other_env.alias == env.alias)
{
self.envs.push(env)
if self.get_env(&env.alias).is_none() {
if self.active_env.is_none() {
self.set_active_env(env.alias.clone());
}
self.envs.push(env);
Copy link
Contributor

@Alex6323 Alex6323 Feb 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think the environment should also be set to active in case the user tried to add the same environment again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand. You mean if the env exists and they try to add it again?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think I agree

Copy link
Contributor

@Alex6323 Alex6323 Feb 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

okay then ... I would still update the docs for this method to mention that it also sets the new env as active (but only if it was unknown before)

}
}
}
Expand Down
10 changes: 2 additions & 8 deletions crates/iota/src/client_commands.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1590,12 +1590,7 @@ impl IotaClientCommands {
basic_auth,
faucet,
} => {
if context
.config()
.envs()
.iter()
.any(|env| env.alias() == &alias)
{
if context.config().get_env(&alias).is_some() {
return Err(anyhow!(
"Environment config with name [{alias}] already exists."
));
Expand Down Expand Up @@ -1668,12 +1663,11 @@ impl IotaClientCommands {
}

pub fn switch_env(config: &mut IotaClientConfig, env: &str) -> Result<(), anyhow::Error> {
let env = Some(env.into());
ensure!(
config.get_env(&env).is_some(),
"Environment config not found for [{env:?}], add new environment config using the `iota client new-env` command."
);
config.set_active_env(env);
config.set_active_env(env.to_owned());
Ok(())
}
}
Expand Down
166 changes: 84 additions & 82 deletions crates/iota/src/iota_commands.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,7 @@ impl IotaCommand {
}
IotaCommand::Console { config } => {
let config = config.unwrap_or(iota_config_dir()?.join(IOTA_CLIENT_CONFIG));
prompt_if_no_config(&config, false, true).await?;
prompt_if_no_config(&config, false, true, true)?;
let context = WalletContext::new(&config, None, None)?;
start_console(context, &mut stdout(), &mut stderr()).await
}
Expand All @@ -467,9 +467,9 @@ impl IotaCommand {
prompt_if_no_config(
&config_path,
accept_defaults,
!matches!(cmd, Some(IotaClientCommands::NewEnv { .. })),
!matches!(cmd, Some(IotaClientCommands::NewAddress { .. })),
)
.await?;
)?;
if let Some(cmd) = cmd {
let mut context = WalletContext::new(&config_path, None, None)?;
cmd.execute(&mut context).await?.print(!json);
Expand All @@ -488,7 +488,7 @@ impl IotaCommand {
accept_defaults,
} => {
let config_path = config.unwrap_or(iota_config_dir()?.join(IOTA_CLIENT_CONFIG));
prompt_if_no_config(&config_path, accept_defaults, true).await?;
prompt_if_no_config(&config_path, accept_defaults, true, true)?;
let mut context = WalletContext::new(&config_path, None, None)?;
if let Some(cmd) = cmd {
cmd.execute(&mut context).await?.print(!json);
Expand Down Expand Up @@ -518,7 +518,7 @@ impl IotaCommand {
// address management.
let config = client_config
.unwrap_or(iota_config_dir()?.join(IOTA_CLIENT_CONFIG));
prompt_if_no_config(&config, false, true).await?;
prompt_if_no_config(&config, false, true, true)?;
let context = WalletContext::new(&config, None, None)?;
let client = context.get_client().await?;
build.chain_id = client.read_api().get_chain_identifier().await.ok();
Expand Down Expand Up @@ -1213,92 +1213,94 @@ async fn genesis(
Ok(())
}

async fn prompt_if_no_config(
fn prompt_for_environment(
wallet_conf_path: &Path,
accept_defaults: bool,
) -> anyhow::Result<IotaEnv> {
if let Some(v) = std::env::var_os("IOTA_CONFIG_WITH_RPC_URL") {
return Ok(IotaEnv::new("custom", v.into_string().unwrap()));
} else {
if accept_defaults {
print!(
"Creating config file [{:?}] with default (devnet) Full node server and ed25519 key scheme.",
wallet_conf_path
);
} else {
print!(
"Config file [{:?}] doesn't exist, do you want to connect to an IOTA Full node server [y/N]?",
wallet_conf_path
);
}
if accept_defaults || matches!(read_line(), Ok(line) if line.trim().to_lowercase() == "y") {
let url = if accept_defaults {
String::new()
} else {
print!("IOTA Full node server URL (Defaults to IOTA Testnet if not specified) : ");
read_line()?
};
if url.trim().is_empty() {
return Ok(IotaEnv::testnet());
} else {
print!("Environment alias for [{url}] : ");
let alias = read_line()?;
let alias = if alias.trim().is_empty() {
"custom".to_string()
} else {
alias
};
return Ok(IotaEnv::new(alias, url));
}
} else {
anyhow::bail!("no environment exists for the client")
}
}
}

fn prompt_if_no_config(
wallet_conf_path: &Path,
accept_defaults: bool,
prompt_for_env: bool,
generate_address: bool,
) -> anyhow::Result<()> {
// Prompt user for connect to devnet fullnode if config does not exist.
if !wallet_conf_path.exists() {
let env = match std::env::var_os("IOTA_CONFIG_WITH_RPC_URL") {
Some(v) => Some(IotaEnv::new("custom", v.into_string().unwrap())),
None => {
if accept_defaults {
print!(
"Creating config file [{:?}] with default (devnet) Full node server and ed25519 key scheme.",
wallet_conf_path
);
} else {
print!(
"Config file [{:?}] doesn't exist, do you want to connect to an IOTA Full node server [y/N]?",
wallet_conf_path
);
}
if accept_defaults
|| matches!(read_line(), Ok(line) if line.trim().to_lowercase() == "y")
{
let url = if accept_defaults {
String::new()
} else {
print!(
"IOTA Full node server URL (Defaults to IOTA Testnet if not specified) : "
);
read_line()?
};
Some(if url.trim().is_empty() {
IotaEnv::testnet()
} else {
print!("Environment alias for [{url}] : ");
let alias = read_line()?;
let alias = if alias.trim().is_empty() {
"custom".to_string()
} else {
alias
};
IotaEnv::new(alias, url)
})
} else {
None
}
}
};

if let Some(env) = env {
let keystore_path = wallet_conf_path
.parent()
.unwrap_or(&iota_config_dir()?)
.join(IOTA_KEYSTORE_FILENAME);
let keystore = Keystore::from(FileBasedKeystore::new(&keystore_path)?);
let mut config = IotaClientConfig::new(keystore).with_envs([env]);
// Get an existing address or generate a new one
if let Some(existing_address) = config.keystore().addresses().first() {
println!("Using existing address {existing_address} as active address.");
config = config.with_active_address(*existing_address);
} else if generate_address {
let key_scheme = if accept_defaults {
SignatureScheme::ED25519
} else {
println!(
"Select key scheme to generate keypair (0 for ed25519, 1 for secp256k1, 2: for secp256r1):"
);
match SignatureScheme::from_flag(read_line()?.trim()) {
Ok(s) => s,
Err(e) => return Err(anyhow!("{e}")),
}
};
let (new_address, phrase, scheme) = config
.keystore_mut()
.generate_and_add_new_key(key_scheme, None, None, None)?;
let alias = config.keystore().get_alias_by_address(&new_address)?;
let keystore_path = wallet_conf_path
.parent()
.unwrap_or(&iota_config_dir()?)
.join(IOTA_KEYSTORE_FILENAME);
let keystore = Keystore::from(FileBasedKeystore::new(&keystore_path)?);
let mut config = IotaClientConfig::new(keystore);
if prompt_for_env {
config.add_env(prompt_for_environment(wallet_conf_path, accept_defaults)?);
}
// Get an existing address or generate a new one
if let Some(existing_address) = config.keystore().addresses().first() {
println!("Using existing address {existing_address} as active address.");
config = config.with_active_address(*existing_address);
} else if generate_address {
let key_scheme = if accept_defaults {
SignatureScheme::ED25519
} else {
println!(
"Generated new keypair and alias for address with scheme {:?} [{alias}: {new_address}]",
scheme.to_string()
"Select key scheme to generate keypair (0 for ed25519, 1 for secp256k1, 2: for secp256r1):"
);
println!("Secret Recovery Phrase : [{phrase}]");
config = config.with_active_address(new_address);
}
config.persisted(wallet_conf_path).save()?;
match SignatureScheme::from_flag(read_line()?.trim()) {
Ok(s) => s,
Err(e) => return Err(anyhow!("{e}")),
}
};
let (new_address, phrase, scheme) = config
.keystore_mut()
.generate_and_add_new_key(key_scheme, None, None, None)?;
let alias = config.keystore().get_alias_by_address(&new_address)?;
println!(
"Generated new keypair and alias for address with scheme {:?} [{alias}: {new_address}]",
scheme.to_string()
);
println!("Secret Recovery Phrase : [{phrase}]");
config = config.with_active_address(new_address);
}
config.persisted(wallet_conf_path).save()?;
}
Ok(())
}
Expand Down
Loading