Networking and Bootstrapping configuration changes#6901
Open
jmwample wants to merge 6 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Contributor
📝 WalkthroughWalkthroughNymNetworkDetails' optional API URL fields are consolidated into a required NetworkingSpecifics struct (nym_api_urls, nym_vpn_api_urls, dns_fallbacks). A new sandbox network module is added, env parsing/export logic is updated, and consumers across validator-client, http-api-client, gateway-probe, SDK, wallet-types, and localnet-orchestrator are updated to the new accessor API. Two unrelated delegator files refactor match arms to use guards. ChangesNetworkingSpecifics refactor and consumers
Estimated code review effort: 3 (Moderate) | ~30 minutes Delegation Event Match-Guard Refactor
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 task
benedettadavico
modified the milestones:
Ziller - (mainnet release 7th july),
Amsterdam - (mainnet release 21st july)
jmwample
changed the title
jmwample
marked this pull request as ready for review
July 7, 2026 16:10
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
common/network-defaults/src/network.rs (1)
157-220: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winImprove the
NYM_APISpanic messagecommon/network-defaults/src/network.rs:173-175—try_parse_api_urls()collapses missing and malformed JSON into an empty vec, sofirst().expect("nym api not set")panics with the same message in both cases. A message that namesNYM_APIS, or propagates the parse error, would make startup failures much easier to diagnose.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@common/network-defaults/src/network.rs` around lines 157 - 220, The panic in NymNetworkDetails::new_from_env for NYM_APIS is ambiguous because try_parse_api_urls() can return an empty list for both missing and malformed input, so first().expect("nym api not set") hides the real failure. Update the NYM_APIS handling in new_from_env() to distinguish absent from invalid values by propagating or surfacing the parse error, and make the panic/error message explicitly reference NYM_APIS so startup issues are diagnosable.
🧹 Nitpick comments (2)
common/http-api-client/src/lib.rs (1)
652-657: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent access pattern: bypasses the
nym_api_urls()accessor.Other consumers (tests.rs, gateway-probe, ip_mix_stream.rs) use
network.nym_api_urls(), but this reaches intonetwork.networking.nym_api_urls.clone()directly, coupling to the internal struct layout. Since this function is already#[deprecated], the practical payoff of fixing this is limited, but for consistency:- let urls = network.networking.nym_api_urls.clone(); + let urls = network.nym_api_urls();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@common/http-api-client/src/lib.rs` around lines 652 - 657, `from_network` is reaching into `NymNetworkDetails` internals by cloning `network.networking.nym_api_urls` directly instead of using the `nym_api_urls()` accessor. Update this method to follow the same access pattern as the other call sites by retrieving the URLs through `network.nym_api_urls()` and passing them into `Self::new_with_fronted_urls`, keeping the implementation consistent with `NymNetworkDetails` usage elsewhere.common/network-defaults/src/sandbox.rs (1)
73-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate/inconsistent sandbox nym-api URL representation.
validators()hardcodes the sandbox nym-api endpoint as"https://sandbox-nym-api1.nymtech.net/api"(no trailing slash), whileNYM_APIShardcodes the same host as"https://sandbox-nym-api1.nymtech.net/api/"(trailing slash). Two independent literals for the same endpoint risk drifting apart on future updates, and the trailing-slash mismatch could matter if either consumer resolves relative paths viaUrl::join.Consider deriving one from the other (e.g., building
validators()'s api_url fromNYM_APIS[0].url) to keep a single source of truth.Also applies to: 88-94
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@common/network-defaults/src/sandbox.rs` around lines 73 - 82, The sandbox nym-api endpoint is defined twice with inconsistent trailing slashes, which can drift and affect URL joining. Update the sandbox URL handling so `validators()` and `NYM_APIS` share a single source of truth, ideally by reusing `NYM_APIS[0].url` when building the API URL. Keep the `validators()` and `NYM_APIS` symbols aligned so both resolve to the exact same sandbox endpoint representation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@common/client-libs/validator-client/src/client.rs`:
- Around line 115-128: The Validator client configuration currently only reads
details.endpoints[0].api_url, so it ignores the richer network metadata and
never uses the fallback/fronting URLs. Update try_from_nym_network_details in
client::Config to carry the network API URL list and front hosts through from
NymNetworkDetails, and thread those values into
nym_http_api_client::ClientBuilder::new_with_fronted_urls instead of collapsing
to a single api_url. Make sure the Config type and any constructor paths are
adjusted to preserve the full URL set for the client setup.
In `@common/network-defaults/src/network.rs`:
- Around line 103-126: `NymNetworkDetails::new_empty()` is inheriting mainnet
networking defaults through `NetworkingSpecifics::default()`, which makes the
supposedly empty constructor carry non-empty API/DNS state. Update `new_empty()`
to build `networking` explicitly with empty `nym_api_urls`, `nym_vpn_api_urls`,
and `dns_fallbacks` instead of relying on `NetworkingSpecifics::default()`,
while leaving the rest of the `NymNetworkDetails` initialization unchanged.
---
Outside diff comments:
In `@common/network-defaults/src/network.rs`:
- Around line 157-220: The panic in NymNetworkDetails::new_from_env for NYM_APIS
is ambiguous because try_parse_api_urls() can return an empty list for both
missing and malformed input, so first().expect("nym api not set") hides the real
failure. Update the NYM_APIS handling in new_from_env() to distinguish absent
from invalid values by propagating or surfacing the parse error, and make the
panic/error message explicitly reference NYM_APIS so startup issues are
diagnosable.
---
Nitpick comments:
In `@common/http-api-client/src/lib.rs`:
- Around line 652-657: `from_network` is reaching into `NymNetworkDetails`
internals by cloning `network.networking.nym_api_urls` directly instead of using
the `nym_api_urls()` accessor. Update this method to follow the same access
pattern as the other call sites by retrieving the URLs through
`network.nym_api_urls()` and passing them into `Self::new_with_fronted_urls`,
keeping the implementation consistent with `NymNetworkDetails` usage elsewhere.
In `@common/network-defaults/src/sandbox.rs`:
- Around line 73-82: The sandbox nym-api endpoint is defined twice with
inconsistent trailing slashes, which can drift and affect URL joining. Update
the sandbox URL handling so `validators()` and `NYM_APIS` share a single source
of truth, ideally by reusing `NYM_APIS[0].url` when building the API URL. Keep
the `validators()` and `NYM_APIS` symbols aligned so both resolve to the exact
same sandbox endpoint representation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95aa6595-d14e-4b4d-8e68-fc6a7c9b0f04
📒 Files selected for processing (15)
common/client-core/src/client/base_client/mod.rscommon/client-libs/validator-client/src/client.rscommon/commands/src/validator/mixnet/delegators/delegate_to_multiple_mixnodes.rscommon/commands/src/validator/mixnet/delegators/query_for_delegations.rscommon/http-api-client/src/lib.rscommon/http-api-client/src/tests.rscommon/network-defaults/src/lib.rscommon/network-defaults/src/network.rscommon/network-defaults/src/sandbox.rsnym-gateway-probe/src/common/helpers.rsnym-wallet/nym-wallet-types/src/network.rsnym-wallet/nym-wallet-types/src/network/sandbox.rssdk/rust/nym-sdk/src/ipr_wrapper/ip_mix_stream.rssdk/rust/nym-sdk/src/mixnet/client.rstools/internal/localnet-orchestrator/src/orchestrator/network.rs
💤 Files with no reviewable changes (1)
- nym-wallet/nym-wallet-types/src/network/sandbox.rs
Comment on lines
+115
to
+128
| /// Create a Validator Client configuration from a provided Network Details. | ||
| /// This uses the included nym api url of the defined validator | ||
| pub fn try_from_nym_network_details( | ||
| details: &NymNetworkDetails, | ||
| ) -> Result<Self, ValidatorClientError> { | ||
| let mut api_url = details | ||
| .endpoints | ||
| .iter() | ||
| .filter_map(|d| d.api_url.as_ref()) | ||
| .map(|url| Url::parse(url)) | ||
| .collect::<Result<Vec<_>, _>>()?; | ||
|
|
||
| if let Some(nym_api_urls) = details.nym_api_urls.as_ref() { | ||
| api_url.extend( | ||
| nym_api_urls | ||
| .iter() | ||
| .map(|url| url.url.parse()) | ||
| .collect::<Result<Vec<_>, _>>()?, | ||
| ); | ||
| } | ||
|
|
||
| if api_url.is_empty() { | ||
| return Err(ValidatorClientError::NoAPIUrlAvailable); | ||
| } | ||
| let api_url = details.endpoints[0] | ||
| .api_url | ||
| .clone() | ||
| .ok_or(ValidatorClientError::NoAPIUrlAvailable)? | ||
| .parse() | ||
| .map_err(ValidatorClientError::MalformedUrlProvided)?; | ||
|
|
||
| Ok(Config { | ||
| api_url: api_url.pop().unwrap(), | ||
| api_url, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Files\n'
git ls-files 'common/client-libs/validator-client/src/client.rs' 'common/network-defaults/src/network.rs' 'nym-gateway-probe/src/common/helpers.rs' 'common/commands/src/context/mod.rs' 'sdk/rust/nym-sdk/src/mixnet/client.rs'
printf '\n## Outline: validator-client/src/client.rs\n'
ast-grep outline common/client-libs/validator-client/src/client.rs --view expanded || true
printf '\n## Outline: common/network-defaults/src/network.rs\n'
ast-grep outline common/network-defaults/src/network.rs --view expanded || true
printf '\n## Search nym_api_urls usage\n'
rg -n "nym_api_urls\\(" common nym-gateway-probe sdk/rust -g '!**/target/**' || true
printf '\n## Relevant slices\n'
sed -n '1,220p' common/client-libs/validator-client/src/client.rs
printf '\n--- network.rs ---\n'
sed -n '1,260p' common/network-defaults/src/network.rs
printf '\n--- gateway probe helper ---\n'
sed -n '1,220p' nym-gateway-probe/src/common/helpers.rs
printf '\n--- context mod ---\n'
sed -n '1,220p' common/commands/src/context/mod.rs
printf '\n--- sdk client ---\n'
sed -n '1,220p' sdk/rust/nym-sdk/src/mixnet/client.rsRepository: nymtech/nym
Length of output: 46673
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## validator-client client methods around construction\n'
sed -n '220,380p' common/client-libs/validator-client/src/client.rs
printf '\n## http-api-client fronted URL support\n'
rg -n "new_with_fronted_urls|front_hosts|ApiUrl" common/http-api-client common/client-core common/client-libs -g '!**/target/**' || true
printf '\n## relevant nym_api client constructor signatures\n'
rg -n "pub fn new_url|pub fn new_with_timeout|pub fn new_with_user_agent|pub fn new_with_fronted_urls" common/http-api-client -g '!**/target/**' || trueRepository: nymtech/nym
Length of output: 10162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Diff for validator-client config function\n'
git diff --unified=40 -- common/client-libs/validator-client/src/client.rs | sed -n '1,220p'
printf '\n## Diff summary for network defaults and validator client\n'
git diff --stat -- common/client-libs/validator-client/src/client.rs common/network-defaults/src/network.rsRepository: nymtech/nym
Length of output: 254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Search validator-client for multi-URL support\n'
rg -n "with_nym_api_urls|new_with_fronted_urls|front_hosts|nym_api_urls\\(" common/client-libs/validator-client common -g '!**/target/**' | sed -n '1,220p'
printf '\n## Validator-client docs/comments around Config\n'
sed -n '90,140p' common/client-libs/validator-client/src/client.rsRepository: nymtech/nym
Length of output: 6599
Thread the network API URL list through validator-client
try_from_nym_network_details collapses NymNetworkDetails to endpoints[0].api_url, so validator-client never consumes networking.nym_api_urls or front_hosts. That drops the fallback/domain-fronting path available elsewhere in the repo; if this client is meant to use the new network metadata, Config needs to carry the URL list into nym_http_api_client::ClientBuilder::new_with_fronted_urls instead of a single URL.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@common/client-libs/validator-client/src/client.rs` around lines 115 - 128,
The Validator client configuration currently only reads
details.endpoints[0].api_url, so it ignores the richer network metadata and
never uses the fallback/fronting URLs. Update try_from_nym_network_details in
client::Config to carry the network API URL list and front hosts through from
NymNetworkDetails, and thread those values into
nym_http_api_client::ClientBuilder::new_with_fronted_urls instead of collapsing
to a single api_url. Make sure the Config type and any constructor paths are
adjusted to preserve the full URL set for the client setup.
Comment on lines
+103
to
+126
| #[derive(Clone, Debug, Hash, Serialize, Deserialize, PartialEq, Eq, JsonSchema)] | ||
| #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] | ||
| pub struct DnsFallback { | ||
| pub url: String, | ||
| pub addresses: Vec<String>, | ||
| } | ||
|
|
||
| #[derive(Clone, Debug, Hash, Serialize, Deserialize, PartialEq, Eq, JsonSchema)] | ||
| #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] | ||
| pub struct NetworkingSpecifics { | ||
| pub nym_api_urls: Vec<ApiUrl>, | ||
| pub nym_vpn_api_urls: Vec<ApiUrl>, | ||
| pub dns_fallbacks: Vec<DnsFallback>, | ||
| // pub internal_nameservers: std::any::Any, | ||
| // pub covert channels: std::any::Any, | ||
| } | ||
|
|
||
| // by default we assume the same defaults as mainnet, i.e. same prefixes and denoms | ||
| impl Default for NetworkingSpecifics { | ||
| fn default() -> Self { | ||
| NymNetworkDetails::mainnet_specifics() | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- network.rs outline ---'
ast-grep outline common/network-defaults/src/network.rs --view expanded || true
echo
echo '--- relevant line ranges ---'
sed -n '1,260p' common/network-defaults/src/network.rs | nl -ba | sed -n '1,260p'
echo
echo '--- search for new_empty and mainnet_specifics ---'
rg -n "new_empty\(|mainnet_specifics\(|struct NymNetworkDetails|impl Default for NetworkingSpecifics|set_nym_api_urls|nym_network_details\(" -S .
echo
echo '--- files containing NymNetworkDetails::new_empty ---'
rg -n "NymNetworkDetails::new_empty\(" -S .Repository: nymtech/nym
Length of output: 5496
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("common/network-defaults/src/network.rs")
text = p.read_text()
for needle in ["pub struct NetworkingSpecifics", "impl Default for NetworkingSpecifics", "pub fn new_empty()", "mainnet_specifics()", "set_nym_api_urls"]:
idx = text.find(needle)
print(f"\n### {needle} @ {idx}")
if idx != -1:
start = max(0, idx - 600)
end = min(len(text), idx + 1200)
print(text[start:end])
PYRepository: nymtech/nym
Length of output: 9339
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- caller file ---'
sed -n '1,260p' tools/internal/localnet-orchestrator/src/orchestrator/network.rs | nl -ba | sed -n '1,260p'Repository: nymtech/nym
Length of output: 209
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("tools/internal/localnet-orchestrator/src/orchestrator/network.rs")
text = path.read_text().splitlines()
for i, line in enumerate(text, 1):
if "nym_network_details" in line or "new_empty()" in line or "set_nym_api_urls" in line or "nym_api_endpoint" in line:
start = max(1, i - 12)
end = min(len(text), i + 30)
print(f"\n--- lines {start}-{end} ---")
for j in range(start, end + 1):
print(f"{j:4d}: {text[j-1]}")
PYRepository: nymtech/nym
Length of output: 20367
new_empty() should not inherit networking defaults. NymNetworkDetails::new_empty() currently uses NetworkingSpecifics::default(), and that default is wired to mainnet_specifics(). That makes the “empty” constructor carry non-empty networking state, so callers like the localnet orchestrator can end up with unexpected API URLs when nym_api_endpoint is unset. Initialize networking with empty vectors in new_empty() instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@common/network-defaults/src/network.rs` around lines 103 - 126,
`NymNetworkDetails::new_empty()` is inheriting mainnet networking defaults
through `NetworkingSpecifics::default()`, which makes the supposedly empty
constructor carry non-empty API/DNS state. Update `new_empty()` to build
`networking` explicitly with empty `nym_api_urls`, `nym_vpn_api_urls`, and
`dns_fallbacks` instead of relying on `NetworkingSpecifics::default()`, while
leaving the rest of the `NymNetworkDetails` initialization unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NYM-927
This PR moves toward distilling and synchronizing the specific networking information that is needed to bootstrap connections such that the structure can be shares and re-used in both nym core applications and in VPN / discovery use cases.
This change:
NetworkDetailsstructurenym_vpn_api_urlfield (this was deprecated by the addition ofnym_vpn_api_urls)nym_vpn_api_urlsandnym_api_urlsinto a newNetworkingSpecificsstructurenym-wallet-typesto thenym-network-detailscrate to match the mainnet network details.This change is
Summary by CodeRabbit
New Features
Bug Fixes
Refactor