-
Notifications
You must be signed in to change notification settings - Fork 44
feat(wasm-sdk): add custom masternode address configuration #2805
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: v2.1-dev
Are you sure you want to change the base?
Conversation
Add ability to configure SDK with custom masternode addresses via new `custom()` factory method and `addresses` option. Supports both testnet and mainnet with automatic trusted quorum prefetch. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Improves API clarity by renaming custom() method to withAddresses() to better describe its purpose of configuring specific masternode addresses. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughAdds optional masternode addresses: JS EvoSDK accepts Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant EvoJS as EvoSDK (JS)
participant WasmBuilder as WasmSdkBuilder (WASM)
participant Trust as TrustedContext
rect rgb(245,250,255)
App->>EvoJS: EvoSDK.withAddresses(addresses, network, options)
EvoJS-->>App: EvoSDK instance (not connected)
end
rect rgb(240,255,245)
App->>EvoJS: connect()
alt addresses provided
EvoJS->>WasmBuilder: withAddresses(addresses, network)
WasmBuilder->>Trust: parse & validate addresses, validate network
Trust-->>WasmBuilder: reuse/create trusted context (prefetch quorums)
else no addresses
EvoJS->>WasmBuilder: withNetworkPreset(network)
WasmBuilder->>Trust: prefetch trusted quorums
Trust-->>WasmBuilder: Ready
end
WasmBuilder->>WasmBuilder: build()
WasmBuilder-->>EvoJS: SDK handle
EvoJS-->>App: connected client
end
opt Error path
WasmBuilder-->>EvoJS: WasmSdkError (invalid address/network)
EvoJS-->>App: Error propagated
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/js-evo-sdk/src/sdk.ts (1)
115-117
: Bug: pass undefined to wasm, not null (Option interop)
withSettings
expectsOption
params. wasm‑bindgen mapsOption<T>
to JSundefined
(notnull
). Passingnull
risks conversion errors.Apply:
- builder = builder.withSettings(connectTimeoutMs ?? null, timeoutMs ?? null, retries ?? null, banFailedAddress ?? null); + builder = builder.withSettings(connectTimeoutMs, timeoutMs, retries, banFailedAddress);
🧹 Nitpick comments (2)
packages/wasm-sdk/src/sdk.rs (2)
148-165
: Validate empty address list earlyCurrently an empty array reaches build-time and panics. Return a clear invalid-argument error up front.
Suggested change:
// Parse and validate addresses - let parsed_addresses: Result<Vec<Address>, _> = addresses + if addresses.is_empty() { + return Err(WasmSdkError::invalid_argument( + "addresses must be a non-empty array".to_string(), + )); + } + let parsed_addresses: Result<Vec<Address>, _> = addresses .into_iter() .map(|addr| { addr.parse::<Uri>() .map_err(|e| format!("Invalid URI '{}': {}", addr, e)) .and_then(|uri| { Address::try_from(uri) .map_err(|e| format!("Invalid address: {}", e)) }) }) .collect();
197-203
: Unreachable network branchAfter validating only mainnet/testnet, this
_
arm can’t be reached. Remove it or replace withunreachable!()
to avoid dead code.- _ => { - return Err(WasmSdkError::invalid_argument(format!( - "Unsupported network '{}'. Only mainnet and testnet are supported.", - network - ))) - } + // Unreachable due to prior validation + _ => unreachable!("network already validated to mainnet or testnet"),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/js-evo-sdk/src/sdk.ts
(4 hunks)packages/js-evo-sdk/tests/unit/sdk.spec.mjs
(2 hunks)packages/wasm-sdk/src/sdk.rs
(1 hunks)packages/wasm-sdk/tests/unit/builder-with-addresses.spec.mjs
(1 hunks)
🔇 Additional comments (1)
packages/js-evo-sdk/src/sdk.ts (1)
90-99
: Confirm intent: addresses path always uses trusted context
withAddresses
uses a trusted context under the hood and you always prefetch trusted quorums. This ignores thetrusted
option (as docs state). Is this intentional?If non‑trusted connections with custom addresses are desired, we should thread
trusted
through to select the appropriate context provider.
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/wasm-sdk/tests/unit/builder-with-addresses.spec.mjs (1)
180-196
: Fix: use variable TEST_ADDRESS_1, not the string literal.Line 182 passes the string literal
'TEST_ADDRESS_1'
instead of the variableTEST_ADDRESS_1
. This will cause URI parsing to fail because it will attempt to parse the literal string'TEST_ADDRESS_1'
as a URI.Apply this diff:
let builder = sdk.WasmSdkBuilder.withAddresses( - ['TEST_ADDRESS_1'], + [TEST_ADDRESS_1], 'testnet' );
🧹 Nitpick comments (1)
packages/wasm-sdk/tests/unit/builder-with-addresses.spec.mjs (1)
103-107
: Use test constant instead of hardcoded IP address.Line 104 uses a hardcoded IP address instead of the test constants defined at the top of the file. For consistency with the rest of the tests and to avoid making network assumptions, use
TEST_ADDRESS_1
or define a new constant.Apply this diff:
const builder2 = sdk.WasmSdkBuilder.withAddresses( - ['https://149.28.241.190:443'], + [TEST_ADDRESS_1], 'Mainnet' );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/wasm-sdk/src/sdk.rs
(1 hunks)packages/wasm-sdk/tests/unit/builder-with-addresses.spec.mjs
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/wasm-sdk/src/sdk.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/wasm-sdk/tests/unit/builder-with-addresses.spec.mjs
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
packages/wasm-sdk/**
📄 CodeRabbit inference engine (CLAUDE.md)
Keep WASM SDK docs in sync (run generate_docs.py) when updating the WASM SDK
Files:
packages/wasm-sdk/tests/unit/builder-with-addresses.spec.mjs
packages/**/tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Place unit and integration tests alongside each package in packages//tests
Files:
packages/wasm-sdk/tests/unit/builder-with-addresses.spec.mjs
🧬 Code graph analysis (1)
packages/wasm-sdk/tests/unit/builder-with-addresses.spec.mjs (1)
packages/js-evo-sdk/tests/unit/sdk.spec.mjs (19)
TEST_ADDRESS_1
(4-4)TEST_ADDRESS_2
(5-5)TEST_ADDRESS_3
(6-6)sdk
(21-21)sdk
(28-28)sdk
(35-35)sdk
(42-42)sdk
(49-49)sdk
(56-70)sdk
(88-91)sdk
(99-101)sdk
(108-111)sdk
(118-130)sdk
(147-151)sdk
(174-174)sdk
(183-183)sdk
(191-191)sdk
(199-199)sdk
(207-211)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build JS packages / Build JS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/wasm-sdk/tests/unit/builder-with-addresses.spec.mjs (1)
103-108
: Replace hardcoded IP with test constant.Line 104 uses a hardcoded IP address
'https://149.28.241.190:443'
instead of the established test constants. This is inconsistent with the rest of the suite, which uses RFC 6761 reserved.test
domain addresses to ensure no network calls occur.Apply this diff to use a test constant:
const builder2 = sdk.WasmSdkBuilder.withAddresses( - ['https://149.28.241.190:443'], + [TEST_ADDRESS_2], 'Mainnet' );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/wasm-sdk/tests/unit/builder-with-addresses.spec.mjs
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
packages/wasm-sdk/**
📄 CodeRabbit inference engine (CLAUDE.md)
Keep WASM SDK docs in sync (run generate_docs.py) when updating the WASM SDK
Files:
packages/wasm-sdk/tests/unit/builder-with-addresses.spec.mjs
packages/**/tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Place unit and integration tests alongside each package in packages//tests
Files:
packages/wasm-sdk/tests/unit/builder-with-addresses.spec.mjs
🧬 Code graph analysis (1)
packages/wasm-sdk/tests/unit/builder-with-addresses.spec.mjs (1)
packages/js-evo-sdk/tests/unit/sdk.spec.mjs (19)
TEST_ADDRESS_1
(4-4)TEST_ADDRESS_2
(5-5)TEST_ADDRESS_3
(6-6)sdk
(21-21)sdk
(28-28)sdk
(35-35)sdk
(42-42)sdk
(49-49)sdk
(56-70)sdk
(88-91)sdk
(99-101)sdk
(108-111)sdk
(118-130)sdk
(147-151)sdk
(174-174)sdk
(183-183)sdk
(191-191)sdk
(199-199)sdk
(207-211)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Rust packages (wasm-sdk) / Unused dependencies
- GitHub Check: Rust packages (wasm-sdk) / Linting
- GitHub Check: Rust packages (wasm-sdk) / Formatting
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
🔇 Additional comments (3)
packages/wasm-sdk/tests/unit/builder-with-addresses.spec.mjs (3)
17-57
: LGTM! Valid configurations are well tested.The test cases appropriately cover single/multiple addresses on testnet and mainnet, with proper resource cleanup via
free()
.
111-136
: LGTM! Address validation is comprehensive.The tests appropriately verify rejection of empty arrays and invalid URIs, with specific error message assertions.
138-184
: LGTM! Builder method chaining is well tested.The tests comprehensively cover method chaining scenarios, including single and multiple chained calls, with proper verification of the built SDK and resource cleanup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/wasm-sdk/tests/unit/builder-with-addresses.spec.mjs (1)
96-108
: Replace hardcoded IP with test constant.The case-insensitive test uses a hardcoded IP address
'https://149.28.241.190:443'
on line 104, which is inconsistent with the RFC 6761 reserved domain principle stated in the comment on line 3 and the rest of the test file.Apply this diff to maintain consistency:
const builder2 = sdk.WasmSdkBuilder.withAddresses( - ['https://149.28.241.190:443'], + [TEST_ADDRESS_2], 'Mainnet', );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/js-evo-sdk/tests/unit/sdk.spec.mjs
(2 hunks)packages/wasm-sdk/src/sdk.rs
(1 hunks)packages/wasm-sdk/tests/unit/builder-with-addresses.spec.mjs
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/js-evo-sdk/tests/unit/sdk.spec.mjs
- packages/wasm-sdk/src/sdk.rs
🧰 Additional context used
📓 Path-based instructions (2)
packages/wasm-sdk/**
📄 CodeRabbit inference engine (CLAUDE.md)
Keep WASM SDK docs in sync (run generate_docs.py) when updating the WASM SDK
Files:
packages/wasm-sdk/tests/unit/builder-with-addresses.spec.mjs
packages/**/tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Place unit and integration tests alongside each package in packages//tests
Files:
packages/wasm-sdk/tests/unit/builder-with-addresses.spec.mjs
🧬 Code graph analysis (1)
packages/wasm-sdk/tests/unit/builder-with-addresses.spec.mjs (1)
packages/js-evo-sdk/tests/unit/sdk.spec.mjs (19)
TEST_ADDRESS_1
(4-4)TEST_ADDRESS_2
(5-5)TEST_ADDRESS_3
(6-6)sdk
(21-21)sdk
(28-28)sdk
(35-35)sdk
(42-42)sdk
(49-49)sdk
(56-70)sdk
(88-91)sdk
(99-101)sdk
(108-111)sdk
(118-130)sdk
(147-151)sdk
(174-174)sdk
(183-183)sdk
(191-191)sdk
(199-199)sdk
(207-211)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Rust packages (wasm-sdk) / Unused dependencies
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Rust packages (wasm-sdk) / Linting
- GitHub Check: Rust packages (wasm-sdk) / Formatting
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
🔇 Additional comments (5)
packages/wasm-sdk/tests/unit/builder-with-addresses.spec.mjs (5)
1-11
: LGTM!The test setup is well-structured:
- Uses RFC 6761 reserved
.test
domain to avoid real network calls- Properly initializes the WASM module before running tests
- Clear test address constants for reuse
13-15
: LGTM!Basic sanity check that the new API method exists.
17-57
: LGTM!The valid configuration tests are comprehensive and well-structured:
- Tests single and multiple addresses
- Tests both mainnet and testnet networks
- Properly verifies builder and built SDK instances
- Consistently performs resource cleanup with
free()
111-136
: LGTM!Address validation tests properly verify:
- Empty address arrays are rejected with clear error message
- Invalid URIs (missing host) are caught with informative error messages
138-184
: LGTM!Builder method chaining tests are comprehensive:
- Tests chaining with
withSettings()
andwithVersion()
- Tests chaining multiple methods together (
withVersion
,withProofs
,withSettings
,withLogs
)- Consistently verifies builder instance, build success, version check, and proper cleanup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/wasm-sdk/src/sdk.rs (2)
130-212
: Well-implemented custom address configuration.The implementation correctly validates addresses and network, reuses cached trusted contexts when available, and constructs the SDK builder appropriately. The error messages are clear and helpful.
Consider extracting the context retrieval pattern into a helper function to reduce duplication between mainnet and testnet branches:
fn get_or_create_context( network: Network, cached_context: &Mutex<Option<WasmTrustedContext>>, create_fn: impl FnOnce() -> Result<WasmTrustedContext, ContextProviderError>, ) -> Result<WasmTrustedContext, WasmSdkError> { let guard = cached_context.lock().unwrap(); guard.clone() .map(Ok) .unwrap_or_else(|| { create_fn() .map_err(|e| WasmSdkError::from(dash_sdk::Error::from(e))) }) }Then simplify lines 183-204:
let trusted_context = match network { Network::Dash => get_or_create_context( network, &MAINNET_TRUSTED_CONTEXT, WasmTrustedContext::new_mainnet, )?, Network::Testnet => get_or_create_context( network, &TESTNET_TRUSTED_CONTEXT, WasmTrustedContext::new_testnet, )?, _ => unreachable!("Network already validated to mainnet or testnet"), };
183-204
: Optional: Cache newly created contexts.When a trusted context is created (because it wasn't cached), it's not stored back in the static cache. This means subsequent calls will create new contexts rather than reusing them. While not incorrect (the prefetch functions populate the cache), storing newly created contexts could improve performance for users who don't call prefetch first.
Consider storing the newly created context:
let trusted_context = match network { Network::Dash => { let mut guard = MAINNET_TRUSTED_CONTEXT.lock().unwrap(); if let Some(ctx) = guard.as_ref() { ctx.clone() } else { let ctx = WasmTrustedContext::new_mainnet() .map_err(|e| WasmSdkError::from(dash_sdk::Error::from(e)))?; *guard = Some(ctx.clone()); ctx } } Network::Testnet => { let mut guard = TESTNET_TRUSTED_CONTEXT.lock().unwrap(); if let Some(ctx) = guard.as_ref() { ctx.clone() } else { let ctx = WasmTrustedContext::new_testnet() .map_err(|e| WasmSdkError::from(dash_sdk::Error::from(e)))?; *guard = Some(ctx.clone()); ctx } } _ => unreachable!("Network already validated to mainnet or testnet"), };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/wasm-sdk/src/sdk.rs
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs
: Format Rust code with cargo fmt
Run Clippy linter for Rust code
Files:
packages/wasm-sdk/src/sdk.rs
packages/wasm-sdk/**
📄 CodeRabbit inference engine (CLAUDE.md)
Keep WASM SDK docs in sync (run generate_docs.py) when updating the WASM SDK
Files:
packages/wasm-sdk/src/sdk.rs
🧬 Code graph analysis (1)
packages/wasm-sdk/src/sdk.rs (4)
packages/rs-sdk/src/sdk.rs (5)
context_provider
(342-347)new_mainnet
(873-877)new_testnet
(855-859)address_list
(590-596)new
(838-843)packages/wasm-sdk/src/error.rs (2)
invalid_argument
(73-75)new
(55-67)packages/rs-dapi-client/src/address_list.rs (4)
uri
(65-67)try_from
(52-60)from_iter
(274-281)new
(132-134)packages/wasm-sdk/src/context_provider.rs (2)
new_mainnet
(102-114)new_testnet
(116-128)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Rust packages (wasm-sdk) / Formatting
- GitHub Check: Rust packages (wasm-sdk) / Unused dependencies
- GitHub Check: Rust packages (wasm-sdk) / Linting
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
@QuantumExplorer how's this look to you? We can't merge without your approval. |
Issue being fixed or feature implemented
WASM-SDK / Evo SDK did not support connecting to a developer-specified set of nodes. This prevented some testing from being easy to do.
What was done?
Added the ability to specify custom masternode addresses when connecting to Dash Platform, allowing users to connect to specific nodes.
withAddresses()
factory method to bothWasmSdkBuilder
andEvoSDK
for specifying custom masternode addressesHow Has This Been Tested?
Locally
Breaking Changes
N/A
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Tests