feat(sdk): source mainnet/testnet bootstrap from dash-network-seeds#3533
feat(sdk): source mainnet/testnet bootstrap from dash-network-seeds#3533lklimek wants to merge 3 commits intofeat/bump-rust-dashcore-v0.42-devfrom
Conversation
Agent-Logs-Url: https://github.com/dashpay/platform/sessions/792a6a35-3c58-465b-91a3-31122aaf67e5 Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dashpay/platform/sessions/792a6a35-3c58-465b-91a3-31122aaf67e5 Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>
Replace the hardcoded `DEFAULT_MAINNET_ADDRESSES` and `DEFAULT_TESTNET_ADDRESSES` constants in `SdkBuilder::new_mainnet()` / `new_testnet()` with a filtered query against `dash_network_seeds::evo_seeds(network)`. The seed lists are refreshed weekly upstream in `rust-dashcore` (same rev as the other dash-* workspace deps), giving us a single source of truth for bootstrap DAPI endpoints instead of a manually curated subset. Only Evo (HPMN) masternodes run Dash Platform, so we filter to those and build `https://<ip>:<platform_http_port>` URIs — discarding the Core P2P port on `seed.address`, which is not what DAPI clients need. Malformed entries are silently skipped rather than panicking; the DAPI client rotates across the remaining addresses. No reachability/SSL filter is applied because the upstream list already ships only HPMNs from the Core DMN list and many Platform nodes legitimately use self-signed certs. Tests now assert the bootstrap list is non-empty and every entry uses the correct platform HTTP port (443 mainnet, 1443 testnet), which is stable across weekly seed refreshes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
|
✅ Review complete (commit 1f37eaf) |
There was a problem hiding this comment.
Pull request overview
Centralizes mainnet/testnet SDK bootstrapping and replaces hardcoded DAPI gateway lists with seed-derived addresses sourced from the dash-network-seeds crate.
Changes:
- Implement
SdkBuilder::new_mainnet()/new_testnet()to construct builders with seed-sourced default bootstrap lists. - Update WASM SDK and FFI layers to delegate mainnet/testnet construction to the shared
SdkBuilderconstructors. - Add unit tests asserting non-empty bootstrap lists and correct per-network Platform HTTP ports.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/rs-sdk/src/sdk.rs | Adds seed-sourced default address list builder, implements mainnet/testnet constructors, and adds tests for non-empty/port correctness. |
| packages/wasm-sdk/src/sdk.rs | Removes duplicated hardcoded gateway lists; uses shared SdkBuilder::new_mainnet() / new_testnet(). |
| packages/rs-sdk-ffi/src/sdk.rs | Collapses inline per-network address arrays into calls to shared SdkBuilder constructors. |
| packages/rs-sdk/Cargo.toml | Adds dash-network-seeds dependency to the SDK crate. |
| Cargo.toml | Adds dash-network-seeds to workspace dependencies pinned to the rust-dashcore rev. |
| Cargo.lock | Records the new dependency in the lockfile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let url = format!("https://{}:{}", seed.address.ip(), port); | ||
| if let Ok(uri) = url.parse::<Uri>() { | ||
| if let Ok(address) = Address::try_from(uri) { | ||
| list.add(address); | ||
| } |
There was a problem hiding this comment.
default_address_list_for_network() can return an empty AddressList if all seeds are skipped (e.g., missing platform_http_port or parse failures). That will later panic in rs_dapi_client::DapiClient::new() because it creates a ConnectionPool with capacity 3 * address_list.len() (0 => non-zero expect). Consider explicitly handling the empty case here (e.g., panic with a clear message or return a Result/config error) so failures are deterministic and actionable.
| pub fn new_mainnet() -> Self { | ||
| unimplemented!( | ||
| "Mainnet address list not implemented yet. Use new() and provide address list." | ||
| ) | ||
| let address_list = default_address_list_for_network(Network::Mainnet); | ||
|
|
||
| Self::new(address_list).with_network(Network::Mainnet) |
There was a problem hiding this comment.
new_mainnet() now derives the bootstrap list from dash_network_seeds and will only panic indirectly if the derived address list is empty (via DapiClient::new() expecting a non-zero pool capacity). The rustdoc above still describes a generic "configuration cannot be loaded" panic; consider updating it to the actual failure mode and/or making the panic explicit with a clearer message when the seed-derived list is empty.
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified all three Claude FFI-engineer findings against the actual code. Two are purely speculative (private function with intentional documented panic; deterministic compile-time seed parsing covered by build-time tests). One is a real but latent defensive concern about IPv6 formatting. Codex security review found no issues.
Reviewed commit: 1f37eaf
💬 1 nitpick(s)
| let Some(port) = seed.platform_http_port else { | ||
| continue; | ||
| }; | ||
| let url = format!("https://{}:{}", seed.address.ip(), port); |
There was a problem hiding this comment.
💬 Nitpick: IPv6 seed addresses would be silently dropped due to missing brackets
format!("https://{}:{}", seed.address.ip(), port) does not bracket IPv6 literals. seed.address is a SocketAddr (dash-network-seeds lib.rs:310) and the upstream parser accepts IPv6 via parse::<IpAddr>(). Today the bundled seeds are IPv4-only, so this is latent — but if a future weekly refresh introduces an IPv6 evo node, the produced string (e.g. https://2001:db8::1:443) is not a valid Uri authority, parse::<Uri>() fails at line 93, and the entry is silently skipped. Cross-boundary impact: FFI/WASM consumers receive a smaller bootstrap list than upstream maintainers intended, with no log signal. The fix is a one-liner: branch on IpAddr::V6 and wrap with [...].
source: ['claude']
Issue being fixed or feature implemented
Supersedes #3530 (Copilot draft that got stuck on a sandboxed CI run). Implements #3529 — remove hardcoded DAPI bootstrap address lists from WASM SDK and FFI, centralize in
dash-sdk, and source from the weekly-refresheddash-network-seedscrate that rust-dashcore v0.42-dev ships (dashpay/rust-dashcore#678).What was done?
Stacked on #3532 (rust-dashcore v0.42-dev bump). Once #3532 merges, this PR auto-retargets
v3.1-devwith a clean 3-commit diff.Centralization (Copilot's two commits, cherry-picked)
SdkBuilder::new_mainnet()/new_testnet()inpackages/rs-sdk/src/sdk.rsmove fromunimplemented!()to real constructors that return a fully wired builder with the default bootstrap address list and correctNetworkvariant.packages/wasm-sdk/src/sdk.rs— dropped its owndefault_mainnet_addresses()/default_testnet_addresses()helpers, now callsSdkBuilder::new_mainnet()/new_testnet()directly.packages/rs-sdk-ffi/src/sdk.rs—dash_sdk_create_trusted()'s 54 lines of inline per-network address arrays collapse into two lines that delegate.Seed-sourced addresses (the actual #3529 deliverable)
dash-network-seedsworkspace dep pinned to the samerust-dashcorerev as the otherdash-*crates.default_address_list_for_network()indash-sdknow iteratesdash_network_seeds::evo_seeds(network)and buildshttps://<ip>:<platform_http_port>URIs — discarding the Core P2P port onseed.addresssince DAPI clients want the Platform HTTP port, which differs by network (mainnet 443, testnet 1443) and is encoded per-node in the seed file.evo_seedsalready ships only HPMNs known to the Core DMN list, and many Dash Platform nodes legitimately use self-signed certs. Let the DAPI client rotate.rust-dashcorebump (~360 mainnet Evo nodes, ~31 testnet).Tests
How Has This Been Tested?
Locally:
cargo check -p dash-sdk -p wasm-sdk -p rs-sdk-ffi --all-targets— cleancargo test -p dash-sdk --lib— 117 pass, 0 fail (3 new:new_mainnet_sources_bootstrap_from_seeds,new_testnet_sources_bootstrap_from_seeds,bootstrap_counts_reasonable)cargo clippy -p dash-sdk --all-targets -- -D warnings— no new findingscargo fmt --all --check— cleanParse timing of
evo_seeds(Mainnet)(~360 entries): ~382µs. No caching added — builder construction happens ≤2 times per SDK lifetime.Breaking Changes
None.
SdkBuilder::new_mainnet()/new_testnet()wereunimplemented!()placeholders before this PR — no production caller could have been reaching them. Callers of WASM SDK / FFI get a refreshed address list on every bump ofrust-dashcore; same contract, better data.Follow-up (separate issue, not this PR):
packages/js-dapi-client/lib/networkConfigs.jsstill hardcodes a single testnet seed. JS SDKs can't consume Rust constants without WASM glue or codegen; left as is and deserves its own ticket.Checklist:
For repository code-owners and collaborators only
Credit: @copilot drafted the centralization and delegation layer in #3530. Final commit replaces the baked-in IPs with the dynamic seed query.
🤖 Co-authored by Claudius the Magnificent AI Agent