Make --network take precedence over rpc-url/passphrase env vars#2579
Make --network take precedence over rpc-url/passphrase env vars#2579fnando wants to merge 2 commits into
--network take precedence over rpc-url/passphrase env vars#2579Conversation
|
Claude finished @fnando's task in 4m 14s —— View job Claude Code Review
SummaryOverall this is a solid bug fix with thoughtful UX (the warning + help-text update is exactly the right mitigation for a precedence change). The implementation is small, well-commented, and the integration tests cover the warning matrix nicely. A few concerns worth discussing before merge — none are blockers, but several deserve a decision on intent. Behavior changes worth flagging in the changelog
Implementation notes
Tests
Things that look good
See inline comments on |
There was a problem hiding this comment.
Pull request overview
This PR updates network argument resolution so a named network takes precedence over RPC URL/passphrase settings, adds a one-time warning for ignored RPC settings, and documents the precedence rules in CLI help.
Changes:
- Prioritizes named network resolution in
Args::get(). - Adds quiet-aware warning plumbing and integration/unit coverage for precedence behavior.
- Updates full help documentation with configuration precedence details.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
FULL_HELP_DOCS.md |
Adds generated/help documentation for configuration precedence. |
cmd/soroban-cli/src/print.rs |
Adds global quiet-state helpers for deep warning emission. |
cmd/soroban-cli/src/config/network.rs |
Changes network resolution precedence, adds warning logic, removes dead error variant, and adds a unit test. |
cmd/soroban-cli/src/commands/mod.rs |
Adds CONFIGURATION section to CLI long help. |
cmd/soroban-cli/src/cli.rs |
Records parsed --quiet state after CLI parsing. |
cmd/crates/soroban-test/tests/it/integration/network.rs |
Adds integration tests for warning behavior and quiet suppression. |
|
|
||
| #[tokio::test] |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| (Some(network), rpc_url, network_passphrase) => { | ||
| warn_if_overridden(network, rpc_url.as_deref(), network_passphrase.as_deref()); | ||
| Ok(locator.read_network(network)?) | ||
| } |
There was a problem hiding this comment.
Precedence violation when --rpc-url is an explicit CLI flag and --network comes from a use default.
set_env_from_config in cli.rs populates STELLAR_NETWORK from stellar network use NAME. Clap then assigns it to self.network via env = "STELLAR_NETWORK". By the time we reach this arm, self.network = Some("…") is indistinguishable from a CLI-typed --network flag.
So a user who runs:
stellar network use testnet # sets the on-disk default
stellar tx ... --rpc-url=http://custom.example.com --network-passphrase=foowill have their explicit --rpc-url CLI flag silently overridden by the use default — a (use, CLI, CLI) source-tuple. That contradicts the precedence ladder you just added to the help text (CLI > env > use). The new "extra rule" documents the carve-out, but the more typical user expectation is that a flag they typed wins over a config default they may have forgotten about.
If you want to honor the documented precedence rigorously, clap exposes ArgMatches::value_source() which lets you check whether each value came from CommandLine, EnvVariable, or DefaultValue/programmatic. The rule could then be: ignore rpc-url/passphrase only when they have a strictly lower source than the network name. That's more code but it matches the documented precedence and avoids surprising the network use user.
At minimum, this trade-off (network always wins, regardless of source) deserves to be called out explicitly in CHANGELOG / release notes since it's a behavior change.
| // Emits the network-precedence warning at most once per process so commands | ||
| // that call `Args::get` multiple times (e.g. sign + sign_fee_bump) don't spam | ||
| // the user. Honors the global `--quiet` flag recorded in `cli::main`. | ||
| fn warn_if_overridden(network: &str, rpc_url: Option<&str>, network_passphrase: Option<&str>) { | ||
| static WARN_ONCE: Once = Once::new(); | ||
| let ignored = match (rpc_url.is_some(), network_passphrase.is_some()) { | ||
| (true, true) => { | ||
| "--rpc-url / STELLAR_RPC_URL and --network-passphrase / STELLAR_NETWORK_PASSPHRASE" | ||
| } | ||
| (true, false) => "--rpc-url / STELLAR_RPC_URL", | ||
| (false, true) => "--network-passphrase / STELLAR_NETWORK_PASSPHRASE", | ||
| (false, false) => return, | ||
| }; | ||
| WARN_ONCE.call_once(|| { | ||
| Print::new(print::is_quiet()).warnln(format!( | ||
| "--network={network} takes precedence; ignoring {ignored}" | ||
| )); | ||
| }); | ||
| } |
There was a problem hiding this comment.
A few thoughts on this helper:
1. The --network={network} wording is misleading when the source is env / use.
The third integration test (network_flag_warning_fires_for_env_only_network) intentionally exercises a setup where the user typed no --network flag — only STELLAR_NETWORK=local. The user then sees --network=local takes precedence and may legitimately wonder which --network flag the warning refers to. Consider phrasing the warning in a source-agnostic way, e.g. network "local" takes precedence (or surfacing the actual source: network "local" (from STELLAR_NETWORK) takes precedence). This is also where STELLAR_NETWORK_SOURCE, already set by set_env_value_from_config, could be consulted to disambiguate env vs use.
2. --rpc-url / STELLAR_RPC_URL reads as one combined thing.
Slash-joined names can scan as "the rpc-url/stellar-rpc-url option". Minor, but or is unambiguous: ignoring --rpc-url or STELLAR_RPC_URL.
3. The Once is process-global.
That's fine for the typical CLI invocation (one process, one warning). It's worth knowing that in a long-lived test/library context, a second Args::get with overrides would be silenced even if the first call had none — Once doesn't reset. Not a bug given current usage, just a caveat for future use.
4. The Print::new(...) allocation only matters when not quiet.
You can short-circuit before formatting the message: if print::is_quiet() { return; } at the top of the call_once. Saves a format! allocation when --quiet is set. Trivial.
| static QUIET: OnceLock<bool> = OnceLock::new(); | ||
|
|
||
| /// Record whether `--quiet` was passed on the command line. Called once from | ||
| /// `cli::main` after parsing so resolvers running deep in the call stack can | ||
| /// honor the global flag without it being threaded through every signature. | ||
| pub fn set_quiet(quiet: bool) { | ||
| let _ = QUIET.set(quiet); | ||
| } | ||
|
|
||
| /// Read the recorded `--quiet` flag, defaulting to `false` if [`set_quiet`] | ||
| /// hasn't run yet (e.g. in unit tests that don't go through `cli::main`). | ||
| pub fn is_quiet() -> bool { | ||
| *QUIET.get().unwrap_or(&false) | ||
| } |
There was a problem hiding this comment.
A process-global OnceLock<bool> is a pragmatic fix for not threading quiet through 25 call sites, but it's worth flagging a couple of properties:
-
Cross-test pollution risk. If any future test calls
set_quiet(true)directly (or via a code path that does), every subsequent test in the samecargo testprocess seesis_quiet() == true. The current PR is safe because all integration tests use subprocesses (new_assert_cmd), but a future in-process unit test that touches this could silently mute warnings in unrelated tests. A short doc-comment warning against callingset_quietfrom tests would be helpful. -
let _ = QUIET.set(quiet);silently swallows the second-set error. That's fine becausecli::mainonly calls it once, but it would be more robust to eitherdebug_assert!(QUIET.set(quiet).is_ok())or rename it toinit_quietto signal "call exactly once". -
Mild alternative if you wanted to avoid global state entirely: thread an
Arc<Print>(or just thebool) throughArgs::getvia thelocator: &locator::Argsparameter — thelocatoralready carries config-dir state through the call graph. Not necessarily worth the refactor, but it would localize the side effect.
|
|
||
| #[tokio::test] | ||
| async fn network_name_wins_over_rpc_tuple() { | ||
| use crate::test_utils::with_env_set; | ||
|
|
||
| let tmp = tempfile::tempdir().unwrap(); | ||
| with_env_set("STELLAR_CONFIG_HOME", tmp.path(), || { | ||
| let args = Args { | ||
| network: Some("testnet".to_string()), | ||
| rpc_url: Some("http://other.example.com:59999".to_string()), | ||
| network_passphrase: Some("Some Other Passphrase".to_string()), | ||
| rpc_headers: Vec::new(), | ||
| }; | ||
|
|
||
| let result = args.get(&locator::Args::default()).unwrap(); | ||
|
|
||
| assert_eq!(result.rpc_url, "https://soroban-testnet.stellar.org"); | ||
| assert_eq!(result.network_passphrase, passphrase::TESTNET); | ||
| }); |
There was a problem hiding this comment.
This test is a good smoke test of the precedence flip, but note that "testnet" is hard-coded in DEFAULTS (see locator.rs:390), so read_network returns the built-in default without ever touching the tmpdir you set up via STELLAR_CONFIG_HOME. In effect, this only proves that a built-in default beats the rpc tuple — not that a user-saved (stellar network add) on-disk network does.
To exercise the actual on-disk path (the case from the PR description: ".env file sets STELLAR_RPC_URL + STELLAR_NETWORK_PASSPHRASE, user has saved a custom network"), you'd need to either:
- write a network JSON under
tmp/network/foo.jsonand usenetwork: Some("foo"), or - assert on a named network that overrides the built-in (e.g.
testnetwith a custom on-disk rpc).
Also worth a unit-level assertion that warn_if_overridden did/didn't fire, even if just by capturing stderr — currently that's only covered by the integration tests, which means the warning is silently untested in the fast unit-test loop.
This is interesting and conflicts with my own workflows and so I suspect others workflows too. I have selected a network by name to get the passphrase and then overridden the rpc url. That seems like reasonable behaviour too.
I think named network flag precedes all network env vars is intuitive, but the part that isn't is doing |
What
Close stellar/stellar-cli-internal#85.
Two changes to network resolution in
Args::get():--network NAME,STELLAR_NETWORK, orstellar network use NAME) now always resolves via the on-disk config and ignores any rpc-url / network-passphrase set in any source. Previously, when all three were set, the wildcard match arm silently picked the rpc tuple and discarded the named network.--quiet.stellar --helpdocumenting the precedence (flag > env > use) and the named-network rule.Internal: removes the dead
Error::CannotUseBothRpcAndNetworkvariant (never returned anywhere). Adds a tinyprint::set_quiet/is_quietOnceLockpair soArgs::getcan honor the global--quietflag without threading it through 25 call sites.Why
If a
.envfile setSTELLAR_RPC_URL+STELLAR_NETWORK_PASSPHRASE, then runningstellar … --network mainnetwould silently send the request to whatever rpc the env happened to specify, ignoring the explicit--networkflag. A named network is the user's clearer intent; resolving it via the on-disk config is what they meant.The warning and help-text additions surface the precedence rule so users don't get surprised again — e.g. a developer with
STELLAR_RPC_URLset in their shell who then runsstellar network info --network mainnetwill see exactly what's happening.Known limitations
N/A