[22.0] stellar keys generate should no longer fund

[chadoh]

(Original reported issue by @chadoh below)

Currently, stellar keys generate not only generate keys, but also funds them. This is a bit confusing for the user (e.g. the original error Chad has encountered), and makes thing more complicated because same thing can be done in multiple ways.
The proposal is to remove fund functionality from keys generate, as well as all network related flags (no network connection is needed anymore, we just generating key pair), and --no-fund (this is a default behavior now).
User can still fund keys with stellar keys fund

Because this is a breaking change, we can't make it before 22.0

---

What version are you using?

$ stellar --version
stellar 21.0.0 (c558dd92613d91a34011cf5105869a5f57af230c)
soroban-env 21.1.0 (8d76e4037417b80dc25c979cc44b8e1281e8ad84)
soroban-env interface version 90194313216
stellar-xdr 21.1.0 (4dcbb918edcd793fdf064b3ee28ceacc6bd9d1bb)
xdr curr (70180d5e8d9caee9e8645ed8a38c36a8cf403cd9)

What did you do?

stellar keys generate alice

What did you expect to see?

If no network provided, should generate keys and quietly skip funding.

If I provide --no-fund, same behavior.

What did you see instead?

Errors unless network provided

[willemneal]

This is because we flatten in the network and told clap we expected either rpc url and network passphrase or network. One solution is to remove from clap and instead the error will move from parsing to just when a the network needs to be resolved. So it should be an easy fix since we already handle the latter error.

[leighmcculloch]

Another option is we remove funding from generating. I find it odd that every time I generate a key, I get an account on testnet, when sometimes I don't want an account created either now or not yet.

[willemneal]

Yeah perhaps we should flip the flag. This way users who want to have it work in one step can still use just one command.

[leighmcculloch]

If no network provided, should generate keys and quietly skip funding.

A downside of this is that when someone has a network set as an env var, it'd still be picked up there, so I could feel like I'm typing a simple generate command but then get hit with funding because I set the env var for commands that I expected to use the network.

[Ifropc]

I think the idea was to flip the logic to always not fund as a breaking change in 22.0
User can still provide --fund to fund the new accounts explicitly. WDYT @leighmcculloch ?

[leighmcculloch]

Adding a --fund flag isn't too different to folks using the fund command, so if we're removing the default fund behavior, I'd suggest we leave off the --fund flag so there's one way to do the thing rather than two.

But a flipped flag would be fine too, it's just two ways to do the same thing.

[Ifropc]

Hmm yeah that makes sense, you can just stellar keys generate alice && stellar keys fund alice, which is just a bit more verbose than stellar keys generate alice --fund. IMO it makes sense that generate only generates the key and doesn't fund it :)

[willemneal]

Yeah we should strive to make each command do one thing as much as possible. I am still a little annoyed that deploy hash --wasm and does the install as well. Being explicit as much as possible helps people understand the steps.

[fnando]

I feel the case where keys are funded is more common than the other way around. I believe we should fix --no-fund rather than changing the behavior. 🤔

If not, I vote for adding --fund instead, because that's better than having to issue two commands.

[Ifropc]

I think adding a --fund flag is fine.
I agree that most common use case is to create and then fund the key, but technically speaking key != account and personally it's been confusing to me why we are creating and funding an account (sometimes I just want to create a keypair and that's it)
Adding a flag also goes well with our approach with "command does only one thing, but can also do something extra with a flag -- it equals to piping result to a different command"

[Ifropc]

I feel the case where keys are funded is more common than the other way around. I believe we should fix --no-fund rather than changing the behavior. 🤔

BTW due to how clap flattening works we can't fix it right now, at least not easily 😔 (you can't make group to be optional, which network options is)

[willemneal]

One solution is to not have clap handle what is required. Currently there is a config.get_network which will throw an error if the args are not provided.
So we just need to remove required_unless_present and then the error will surface only when trying to resolve the network.

stellar-cli/cmd/soroban-cli/src/config/network.rs

Lines 41 to 67 in c8c3871

	/// RPC server endpoint
	#[arg(
	long = "rpc-url",
	requires = "network_passphrase",
	required_unless_present = "network",
	env = "STELLAR_RPC_URL",
	help_heading = HEADING_RPC,
	)]
	pub rpc_url: Option<String>,
	/// Network passphrase to sign the transaction sent to the rpc server
	#[arg(
	long = "network-passphrase",
	requires = "rpc_url",
	required_unless_present = "network",
	env = "STELLAR_NETWORK_PASSPHRASE",
	help_heading = HEADING_RPC,
	)]
	pub network_passphrase: Option<String>,
	/// Name of network to use from config
	#[arg(
	long,
	required_unless_present = "rpc_url",
	required_unless_present = "network_passphrase",
	env = "STELLAR_NETWORK",
	help_heading = HEADING_RPC,
	)]
	pub network: Option<String>,

[willemneal]

Or in the case where you just have the network args:

stellar-cli/cmd/soroban-cli/src/config/network.rs

Lines 71 to 88 in c8c3871

	pub fn get(&self, locator: &locator::Args) -> Result<Network, Error> {
	if let Some(name) = self.network.as_deref() {
	if let Ok(network) = locator.read_network(name) {
	return Ok(network);
	}
	}
	if let (Some(rpc_url), Some(network_passphrase)) =
	(self.rpc_url.clone(), self.network_passphrase.clone())
	{
	Ok(Network {
	rpc_url,
	network_passphrase,
	})
	} else {
	Err(Error::Network)
	}
	}
	}

[Ifropc]

We can indeed make all network args optional, but then we need to do validation ourselves, which will give a different input compared to clap (don't think it's a major downside, but still)

[fnando]

@Ifropc maybe this a good reason for #1616. It's easier to make one single argument optional when needed. 🤔

[Ifropc]

^ One of the reasons I want to see that change actually 😆

[willemneal]

We can indeed make all network args optional, but then we need to do validation ourselves, which will give a different input compared to clap (don't think it's a major downside, but still)

I'm saying we are already doing the validation. So removing the clap requires should not break anything, though we might need to improve the error message.

[github-actions]

This issue is stale because it has been assigned for 30 days with no activity. It will be closed in 30 days unless the stale label is removed, and the assignee is removed or updated.