Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 8 additions & 2 deletions packages/rs-sdk-ffi/src/sdk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -358,8 +358,14 @@ pub unsafe extern "C" fn dash_sdk_create_trusted(config: *const DashSDKConfig) -
info!("dash_sdk_create_trusted: no DAPI addresses provided, using defaults for network");
// Use default addresses for the network
match network {
Network::Testnet => SdkBuilder::new_testnet(),
Network::Mainnet => SdkBuilder::new_mainnet(),
Network::Testnet => {
info!("dash_sdk_create_trusted: using default testnet addresses");
SdkBuilder::new_testnet()
}
Network::Mainnet => {
info!("dash_sdk_create_trusted: using default mainnet addresses");
SdkBuilder::new_mainnet()
}
Comment on lines 365 to 368
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟡 Suggestion: FFI mainnet fallback now uses only 5 default nodes instead of the previous 8

This PR changes dash_sdk_create_trusted() to delegate its no-address mainnet path to SdkBuilder::new_mainnet(). Before this change, the FFI code hardcoded 8 mainnet endpoints; after it, new_mainnet() supplies only 5 (149.28.241.190, 198.7.115.48, 134.255.182.186, 93.115.172.39, 5.189.164.253). That silently reduces the default fan-out for iOS/FFI consumers even though the PR is framed as a stale testnet address refresh. Either restore the three removed mainnet endpoints in SdkBuilder::new_mainnet() or call out the reduced default pool explicitly as an intended behavior change.

source: ['claude']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-sdk-ffi/src/sdk.rs`:
- [SUGGESTION] lines 387-390: FFI mainnet fallback now uses only 5 default nodes instead of the previous 8
  This PR changes `dash_sdk_create_trusted()` to delegate its no-address mainnet path to `SdkBuilder::new_mainnet()`. Before this change, the FFI code hardcoded 8 mainnet endpoints; after it, `new_mainnet()` supplies only 5 (`149.28.241.190`, `198.7.115.48`, `134.255.182.186`, `93.115.172.39`, `5.189.164.253`). That silently reduces the default fan-out for iOS/FFI consumers even though the PR is framed as a stale testnet address refresh. Either restore the three removed mainnet endpoints in `SdkBuilder::new_mainnet()` or call out the reduced default pool explicitly as an intended behavior change.

_ => {
error!(
?network,
Expand Down
26 changes: 19 additions & 7 deletions packages/rs-sdk/src/sdk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ use std::num::NonZeroUsize;
use std::path::Path;
#[cfg(feature = "mocks")]
use std::path::PathBuf;
use std::str::FromStr;
use std::sync::atomic::Ordering;
use std::sync::{atomic, Arc};
#[cfg(feature = "mocks")]
Expand Down Expand Up @@ -788,6 +789,8 @@ impl SdkBuilder {
/// This is a helper method that preconfigures [SdkBuilder] for testnet use.
/// Use this method if you want to connect to Dash Platform testnet during development and testing
/// of your solution.
///
/// Testnet addresses sourced from <https://quorums.testnet.networks.dash.org/masternodes>.
pub fn new_testnet() -> Self {
let address_list = default_address_list_for_network(Network::Testnet);

Expand All @@ -799,19 +802,28 @@ impl SdkBuilder {
/// This is a helper method that preconfigures [SdkBuilder] for production use.
/// Use this method if you want to connect to Dash Platform mainnet with production-ready product.
///
/// ## Panics
///
/// This method panics if the mainnet configuration cannot be loaded.
///
/// ## Unstable
///
/// This method is unstable and can be changed in the future.
/// Mainnet addresses sourced from mnowatch.org.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

💬 Nitpick: Docstrings point to mnowatch.org / quorums.testnet, but addresses now come from dash_network_seeds

Both new_testnet (line 793) and new_mainnet (line 805) docstrings claim addresses are sourced from mnowatch.org / quorums.testnet.networks.dash.org. After this PR, both methods call default_address_list_for_network (line 84), which iterates dash_network_seeds::evo_seeds(network) — a single-source-of-truth list refreshed weekly upstream in rust-dashcore. A maintainer trying to update IPs by following the docstring will look in the wrong place. Update both docstrings to reference dash_network_seeds (and optionally note that mnowatch / quorums.testnet are the upstream sources feeding that crate).

💡 Suggested change
Suggested change
/// Mainnet addresses sourced from mnowatch.org.
/// Mainnet addresses are sourced from [`dash_network_seeds::evo_seeds`] (upstream of mnowatch.org).

source: ['claude']

pub fn new_mainnet() -> Self {
let address_list = default_address_list_for_network(Network::Mainnet);

Self::new(address_list).with_network(Network::Mainnet)
}

/// Create a new SdkBuilder instance preconfigured for local network (dashmate gateway).
///
/// This is a helper method that preconfigures [SdkBuilder] for local development use
/// with a dashmate-managed node.
pub fn new_local() -> Self {
let addresses = AddressList::from_str("https://127.0.0.1:2443")
.expect("hardcoded local address must be valid");

Self {
addresses: Some(addresses),
network: Network::Regtest,
..Default::default()
Comment on lines 794 to +823
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟡 Suggestion: The new preset builder helpers were added without smoke tests

SdkBuilder::new_testnet(), new_mainnet(), and new_local() replace previous unimplemented!() stubs and now parse hardcoded address strings with .expect(...). There is an existing #[cfg(test)] module in this file, but this PR does not add any coverage for these new constructors. A typo in any hardcoded URI would now surface as a runtime panic the first time a caller uses the helper. Add a small unit test that constructs each preset and asserts the expected network and presence of addresses so these defaults are validated in CI.

source: ['claude']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-sdk/src/sdk.rs`:
- [SUGGESTION] lines 758-812: The new preset builder helpers were added without smoke tests
  `SdkBuilder::new_testnet()`, `new_mainnet()`, and `new_local()` replace previous `unimplemented!()` stubs and now parse hardcoded address strings with `.expect(...)`. There is an existing `#[cfg(test)]` module in this file, but this PR does not add any coverage for these new constructors. A typo in any hardcoded URI would now surface as a runtime panic the first time a caller uses the helper. Add a small unit test that constructs each preset and asserts the expected network and presence of addresses so these defaults are validated in CI.

}
}
Comment on lines +816 to +825
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟡 Suggestion: new_local diverges from new_testnet/new_mainnet construction style

new_testnet and new_mainnet build via Self::new(address_list).with_network(...). new_local instead uses a struct literal with ..Default::default(). Functionally equivalent today, but if Self::new later adds logging, validation, or initializes a new field non-default, new_local will silently skip that. Use the same construction path as the sister methods for consistency and future-proofing.

💡 Suggested change
Suggested change
pub fn new_local() -> Self {
let addresses = AddressList::from_str("https://127.0.0.1:2443")
.expect("hardcoded local address must be valid");
Self {
addresses: Some(addresses),
network: Network::Regtest,
..Default::default()
}
}
pub fn new_local() -> Self {
let addresses = AddressList::from_str("https://127.0.0.1:2443")
.expect("hardcoded local address must be valid");
Self::new(addresses).with_network(Network::Regtest)
}

source: ['claude']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-sdk/src/sdk.rs`:
- [SUGGESTION] lines 816-825: new_local diverges from new_testnet/new_mainnet construction style
  `new_testnet` and `new_mainnet` build via `Self::new(address_list).with_network(...)`. `new_local` instead uses a struct literal with `..Default::default()`. Functionally equivalent today, but if `Self::new` later adds logging, validation, or initializes a new field non-default, `new_local` will silently skip that. Use the same construction path as the sister methods for consistency and future-proofing.


/// Configure network type.
///
/// Defaults to Network::Mainnet which is mainnet.
Expand Down
21 changes: 1 addition & 20 deletions packages/wasm-sdk/src/sdk.rs
Original file line number Diff line number Diff line change
@@ -1,28 +1,12 @@
use crate::context_provider::{WasmContext, WasmTrustedContext};
use crate::error::WasmSdkError;
use dash_sdk::dpp::version::PlatformVersion;
use dash_sdk::sdk::Uri;
use dash_sdk::{Sdk, SdkBuilder};
use rs_dapi_client::{Address, RequestSettings};
use std::ops::{Deref, DerefMut};
use std::time::Duration;
use wasm_bindgen::prelude::wasm_bindgen;

fn parse_addresses(addresses: &'static [&str]) -> Vec<Address> {
addresses
.iter()
.filter_map(|addr| {
Uri::from_maybe_shared(addr)
.ok()
.and_then(|uri| Address::try_from(uri).ok())
})
.collect()
}

fn default_local_addresses() -> Vec<Address> {
parse_addresses(&["https://127.0.0.1:2443"])
}

#[wasm_bindgen]
pub struct WasmSdk {
sdk: Sdk,
Expand Down Expand Up @@ -248,10 +232,7 @@ impl WasmSdkBuilder {
/// Create a new SdkBuilder preconfigured for a local network using default dashmate gateway.
#[wasm_bindgen(js_name = "local")]
pub fn new_local() -> Self {
let address_list = dash_sdk::sdk::AddressList::from_iter(default_local_addresses());
let sdk_builder = SdkBuilder::new(address_list)
.with_network(dash_sdk::dpp::dashcore::Network::Regtest)
.with_context_provider(WasmContext {});
let sdk_builder = SdkBuilder::new_local().with_context_provider(WasmContext {});

Self {
inner: sdk_builder,
Expand Down
Loading