Skip to content

feat(wasm-sdk): first-class devnet support with trusted-context prefetch#3748

Merged
QuantumExplorer merged 10 commits into
v3.1-devfrom
feat/wasm-sdk-devnet-trusted-context
May 27, 2026
Merged

feat(wasm-sdk): first-class devnet support with trusted-context prefetch#3748
QuantumExplorer merged 10 commits into
v3.1-devfrom
feat/wasm-sdk-devnet-trusted-context

Conversation

@PastaPastaPasta
Copy link
Copy Markdown
Member

@PastaPastaPasta PastaPastaPasta commented May 27, 2026

Issue being fixed or feature implemented

The WASM SDK has no first-class concept of devnets. To reach a devnet today, consumers like native-platform-explorer must masquerade as testnet with { network: 'testnet', trusted: false, addresses: [...] }, which disables proof verification entirely. As soon as a proof-bearing response comes back, WasmContext::get_quorum_public_key hard-errors with:

"Non-trusted mode is not supported in WASM. Please use the trusted SDK builders (new_mainnet_trusted or new_testnet_trusted) instead."

(The error message itself is also stale — it references builder names that no longer exist.)

After v3.1.0-dev.6's gRPC trailer-transport fix lands, this error becomes the dominant failure mode on every devnet state query.

What was done?

Adds devnets as a first-class network across three packages:

  • rs-sdk-trusted-context-provider: Devnet quorum URLs now follow the testnet/mainnet shape — https://quorums.<name>.networks.dash.org (was https://quorums.devnet.<name>.networks.dash.org). All validation rules for devnet names are unchanged.
  • wasm-sdk:
    • WasmTrustedContext::prefetchDevnet(devnet_name) — async factory parallel to prefetchTestnet. URL derived from the devnet name.
    • WasmTrustedContext::prefetchDevnetWithUrl(base_url) — escape hatch for when the public DNS isn't deployed yet.
    • WasmSdkBuilder::newDevnet() factory + a "devnet" arm on withAddresses.
    • Updated the stale WasmContext::get_quorum_public_key error message to reference current method names (prefetchMainnet/Testnet/Devnet/Local).
  • js-evo-sdk:
    • Extends EvoSDKOptions.network to 'mainnet' | 'testnet' | 'local' | 'devnet'.
    • New devnetName and quorumUrl options.
    • EvoSDK.devnet(name, options) and EvoSDK.devnetTrusted(name, options) static factories.
    • Constructor-time validation rejects misconfigurations: missing devnetName and addresses, non-trusted devnet without addresses, quorumUrl without trusted, quorumUrl on non-devnet networks.

Once published, the migration for the explorer is new EvoSDK({ addresses, network: 'testnet', trusted: false })EvoSDK.devnetTrusted('paloma') (with proof verification restored).

How Has This Been Tested?

  • cargo test -p rs-sdk-trusted-context-provider: URL-builder tests pass with the new pattern (test_get_quorum_base_url, test_devnet_without_name_returns_error, test_invalid_devnet_names). One pre-existing DNS-dependent test (test_known_contracts) fails the same way without these changes; not a regression.
  • yarn workspace @dashevo/evo-sdk test:unit: 200 passing (was 198), including 9 new tests covering both happy paths and every new validation rule for EvoSDK.devnet / EvoSDK.devnetTrusted (including "devnet with only addresses, no devnetName" and "reject quorumUrl when trusted is false").
  • cargo check -p wasm-sdk / wasm32 build: could not run on the dev machine — local Apple clang lacks the wasm32 backend. Rust changes are small (~50 lines) and copy-of-existing-patterns from prefetch_testnet / prefetch_local_with_url. An independent code-review pass flagged a stale doc comment and a silently-discarded `quorumUrl` on non-trusted devnet; both addressed. CI will exercise the full wasm build.

Breaking Changes

  • The default devnet quorum URL pattern changes from `https://quorums.devnet..networks.dash.org` → `https://quorums..networks.dash.org`. This is a behavior change for any existing caller of `TrustedHttpContextProvider::new(Network::Devnet, Some(name), ...)` that was relying on the old hostname. No public devnet quorum DNS has been deployed yet, so in practice no live caller depends on the old shape — but flagging here for transparency. The escape hatch (`new_with_url` / `prefetchDevnetWithUrl`) lets callers pin a different URL if needed.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

Summary by CodeRabbit

  • New Features

    • Devnet support: named devnets and quorum-URL override; devnet usable in address-based or trusted modes. New convenience helpers for devnet and trusted-devnet SDK creation; WASM builder and trusted-context prefetch helpers for devnet and explicit URLs.
  • Documentation

    • Updated devnet quorum endpoint URL format and examples.
  • Tests

    • Added unit tests for devnet behaviors, validations, and trusted-context handling.
  • Bug Fixes / Validation

    • Stronger devnet name and quorum-URL validations (including reserved names and HTTPS rules).

Review Change Stack

Adds prefetchDevnet/prefetchDevnetWithUrl to WasmTrustedContext, a newDevnet factory and "devnet" arm to WasmSdkBuilder, and EvoSDK.devnet/devnetTrusted factories to js-evo-sdk so devnet consumers no longer need to masquerade as testnet to use the SDK.

Devnet quorum URLs now follow the testnet/mainnet shape: https://quorums.<name>.networks.dash.org (was https://quorums.devnet.<name>...). The stale WasmContext non-trusted error message that referenced removed builder names is also corrected.
@github-actions github-actions Bot added this to the v3.1.0 milestone May 27, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 27, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds devnet support across EvoSDK, WASM SDK, and the trusted-context provider: new devnet options and validation, devnet factory helpers, trusted-context prefetch for devnet (by name or URL), WASM devnet builder, unit tests, and standardized devnet quorum endpoint format.

Changes

Devnet Network Support

Layer / File(s) Summary
Trusted provider: quorum URL format & validation
packages/rs-sdk-trusted-context-provider/*
Devnet quorum base URL changed to https://quorums.<devnet_name>.networks.dash.org in docs, implementation, and tests; reserved devnet names are rejected; production custom URLs require https://.
WASM: builder parsing and tests
packages/wasm-sdk/src/sdk.rs, packages/wasm-sdk/tests/unit/*
WasmSdkBuilder accepts devnet, adds newDevnet() (JS newDevnet), and tests updated to accept devnet and include it in invalid-network errors.
WASM: Trusted-context prefetch factories
packages/wasm-sdk/src/context_provider.rs
Adds prefetchDevnet/prefetchDevnetWithUrl and prefetch*WithUrl variants, refactors prefetch logic into prefetch_for, disables refetch-if-not-found, and updates docs/error text.
JS: EvoSDK options, constructor, and connect
packages/js-evo-sdk/src/sdk.ts
EvoSDKOptions adds devnet, devnetName, quorumUrl; constructor enforces devnet/trusted validation; connect() prefetches trusted devnet context (by quorumUrl or devnetName) and selects WasmSdkBuilder.newDevnet() for network==='devnet'.
JS: Devnet public factories & withAddresses signature
packages/js-evo-sdk/src/sdk.ts
Adds EvoSDK.devnet() and EvoSDK.devnetTrusted() helpers; withAddresses() now accepts devnet and optional devnetName.
JS: Devnet unit tests
packages/js-evo-sdk/tests/unit/sdk.spec.ts
Adds tests for devnet() and devnetTrusted() validating instance options, error cases, and quorumUrl acceptance/rejection rules.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant EvoSDK
  participant WasmTrustedContext
  participant WasmSdkBuilder
  participant TrustedHttpProvider
  Client->>EvoSDK: connect()
  EvoSDK->>WasmTrustedContext: prefetchDevnetWithUrl(quorumUrl) or prefetchDevnet(devnetName)
  WasmTrustedContext->>TrustedHttpProvider: construct provider, update quorum caches, fetch masternode addresses
  EvoSDK->>WasmSdkBuilder: WasmSdkBuilder.newDevnet()
  WasmSdkBuilder->>WasmTrustedContext: withTrustedContext(trustedContext)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~40 minutes

Possibly related issues

Suggested reviewers

  • QuantumExplorer
  • shumkov
  • lklimek

Poem

🐰 I hopped through code to add devnet cheer,
Builders, prefetches, and tests appear,
Quorum hosts shortened, names checked with care,
From Rust to WASM to TypeScript there,
A tiny rabbit smiles — devnets are here!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main feature being introduced: first-class devnet support with trusted-context prefetch functionality across the SDK packages.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/wasm-sdk-devnet-trusted-context

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a 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

🤖 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 `@packages/js-evo-sdk/src/sdk.ts`:
- Around line 41-43: The comment for devnetName is inaccurate: update the doc on
the devnetName field (devnetName) to reflect the constructor's actual rules —
state that devnetName is required when network === 'devnet' unless explicit
addresses are provided, and explicitly note that if trusted === false for a
devnet, explicit addresses are mandatory and devnetName alone is not sufficient;
make the same edit to the duplicate comment block around lines 223-225 so both
docstrings match the enforced behavior in the constructor/validation logic.
🪄 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: fc16aa81-e64e-449e-920a-fa16782cb5e6

📥 Commits

Reviewing files that changed from the base of the PR and between e230c9e and 3abe919.

📒 Files selected for processing (7)
  • packages/js-evo-sdk/src/sdk.ts
  • packages/js-evo-sdk/tests/unit/sdk.spec.ts
  • packages/rs-sdk-trusted-context-provider/README.md
  • packages/rs-sdk-trusted-context-provider/src/lib.rs
  • packages/rs-sdk-trusted-context-provider/src/provider.rs
  • packages/wasm-sdk/src/context_provider.rs
  • packages/wasm-sdk/src/sdk.rs

Comment thread packages/js-evo-sdk/src/sdk.ts
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 27, 2026

✅ DashSDKFFI.xcframework built for this PR.

SwiftPM (host the zip at a stable URL, then use):

.binaryTarget(
  name: "DashSDKFFI",
  url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
  checksum: "7f87eda26653fd5b3c3eb39c73616364125c5252e58dfea92ff0e7a55c659100"
)

Xcode manual integration:

  • Download 'DashSDKFFI.xcframework' artifact from the run link above.
  • Drag it into your app target (Frameworks, Libraries & Embedded Content) and set Embed & Sign.
  • If using the Swift wrapper package, point its binaryTarget to the xcframework location or add the package and place the xcframework at the expected path.

…t support

Two wasm-sdk unit tests assumed devnet was rejected by WasmSdkBuilder.withAddresses and pinned the old error message — replace with one that verifies devnet is now accepted, and update the remaining assertion to match the new "mainnet, testnet, devnet, or local" list.

Tighten the devnetName field comment and EvoSDK.devnet JSDoc to match the constructor's actual rules: devnetName is required only for trusted devnet (used to derive the quorum URL); non-trusted devnet requires explicit addresses.
@codecov
Copy link
Copy Markdown

codecov Bot commented May 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.17%. Comparing base (e230c9e) to head (c32f246).
⚠️ Report is 2 commits behind head on v3.1-dev.

Additional details and impacted files
@@             Coverage Diff              @@
##           v3.1-dev    #3748      +/-   ##
============================================
+ Coverage     87.16%   87.17%   +0.01%     
============================================
  Files          2607     2607              
  Lines        319420   319489      +69     
============================================
+ Hits         278413   278506      +93     
+ Misses        41007    40983      -24     
Components Coverage Δ
dpp 87.71% <ø> (+0.04%) ⬆️
drive 85.95% <ø> (ø)
drive-abci 89.60% <ø> (ø)
sdk ∅ <ø> (∅)
dapi-client ∅ <ø> (∅)
platform-version ∅ <ø> (∅)
platform-value 92.17% <ø> (ø)
platform-wallet ∅ <ø> (∅)
drive-proof-verifier 49.16% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…ontexts

Custom quorum endpoints are a legitimate testing case for every network, not just devnet. Add prefetchMainnetWithUrl / prefetchTestnetWithUrl mirroring the local/devnet pattern, refactor the four prefetch variants through a shared prefetch_for helper, and let js-evo-sdk's quorumUrl option flow through to whichever network is selected.

Constructor guards relaxed: quorumUrl is now valid for any trusted network, and a trusted devnet may be constructed from quorumUrl alone (devnetName is only needed when the URL has to be derived from the name).
Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

Verified the PR against the worktree at 3abe919. Three confirmed in-scope issues: a blocking test/code drift in the WASM builder spec (the suite still asserts devnet must be rejected), a security/footgun in the new flat devnet URL shape (reserved names like 'mainnet' silently resolve to production quorum endpoints), and a defense-in-depth gap where the newly-exposed prefetchDevnetWithUrl entry point accepts plaintext HTTP for trust-root material. Two consistency/doc nits also kept. Several out-of-scope adjacent items are tracked separately.

🔴 1 blocking | 🟡 2 suggestion(s) | 💬 2 nitpick(s)

3 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments 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/wasm-sdk/tests/unit/builder-with-addresses.spec.ts`:
- [BLOCKING] packages/wasm-sdk/tests/unit/builder-with-addresses.spec.ts:73-95: WASM unit spec still asserts `withAddresses(..., 'devnet')` is rejected after the implementation began accepting it
  `packages/wasm-sdk/src/sdk.rs:205-216` now maps `"devnet" => Network::Devnet` and the error string was updated to `"Expected: mainnet, testnet, devnet, or local"`. But `builder-with-addresses.spec.ts:73-83` still has `it('should reject devnet', ...)` asserting both that the call throws and that the message contains `'mainnet, testnet or local'`. The `'should reject invalid network name'` case at lines 85-95 also asserts the old error string and will fail against the new message. Once the WASM artifact is rebuilt for tests, this suite breaks and no longer describes the new API. Update the spec to assert a positive devnet case (build succeeds) and update the invalid-network assertion to the new error wording.

In `packages/rs-sdk-trusted-context-provider/src/lib.rs`:
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/src/lib.rs:28-53: Devnet-name validation does not reject reserved names that now alias the mainnet/testnet quorum hostnames
  The PR flattens the devnet URL shape from `https://quorums.devnet.<name>.networks.dash.org` to `https://quorums.<name>.networks.dash.org`. The old shape was structurally namespaced; the new shape shares the namespace with mainnet/testnet/local. The validator at lines 30-46 only checks character class and edge hyphens, so `name = "mainnet" | "testnet" | "local" | "regtest"` all pass and produce the production-network quorum URL while the SDK is configured with `Network::Devnet`.

  Via the new `EvoSDK.devnetTrusted('mainnet')` / `WasmTrustedContext.prefetchDevnet('mainnet')` surface, a typo or attacker-influenced devnet name silently pulls real mainnet quorum public keys (and via `withTrustedContext`, real mainnet masternode addresses) into a session labeled `network: 'devnet'`. Proof verification then runs devnet payloads against mainnet quorum keys — a cross-network mismatch instead of an early, loud configuration error. The previous URL pattern made this collision impossible; the new pattern removes the structural guardrail and the validator is the only line of defense.

  Reject `mainnet | testnet | devnet | local | regtest` (case-insensitive) in `get_quorum_base_url`. The escape hatch (`new_with_url` / `prefetchDevnetWithUrl`) still lets callers point at any URL explicitly.

In `packages/rs-sdk-trusted-context-provider/src/provider.rs`:
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/src/provider.rs:137-145: `new_with_url` (now reachable from JS via `prefetchDevnetWithUrl`) does not require HTTPS for trust-root fetches
  Before this PR, `new_with_url` was only reachable from native Rust callers and from `prefetch_local_with_url` (loopback). This PR introduces `WasmTrustedContext::prefetch_devnet_with_url` (context_provider.rs:209-234), exposing `new_with_url` to JS callers with an arbitrary `base_url` for `Network::Devnet`. The constructor does not validate the scheme.

  The documented use case is exactly the dangerous one: "Use this when the default `https://quorums.<devnet_name>.networks.dash.org` URL is not yet deployed." A developer pointing at an early-stage devnet sidecar over `http://` lets any on-path attacker tamper with `/quorums` (the SDK's root of trust for proof verification) and `/masternodes` (the addresses `withTrustedContext` injects into the builder). The UI then shows "proof verified" on attacker-controlled responses signed by attacker-controlled quorum keys.

  Reject non-HTTPS URLs for `Network::Devnet` (and other non-loopback networks) at construction. `prefetch_local_with_url`/`prefetch_local` are scoped to 127.0.0.1 and remain acceptable plaintext.
Out-of-scope follow-up suggestions (4)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

  • WasmSdkBuilder.newDevnet() doesn't enforce the 'must pair with addresses or trusted context' contract at build time — The new new_devnet() constructs the inner SdkBuilder with AddressList::default() (empty). Callers are expected to add addresses or attach a trusted context; nothing fails fast if they don't. EvoSDK enforces this at the JS layer (the primary intended consumer), so direct WASM consumers are the only ones who could hit a confusing downstream connectivity failure. Pre-existing builder-pattern convention; not introduced by this PR but a new factory that inherits it.
    • Follow-up: Consider a follow-up to validate in WasmSdkBuilder.build() that a Devnet builder has either a trusted context or a non-empty address list.
  • Devnet name validation accepts Unicode alphanumericsget_quorum_base_url uses c.is_alphanumeric(), which is Unicode-aware (Cyrillic etc. pass). The resulting hostname would need IDN handling before reqwest. Pre-existing and unchanged by this PR; the URL flattening just expands the surface where this validation is the only check. Worth tightening to ASCII alphanumerics across the devnet-name validators.
    • Follow-up: Open a follow-up to restrict devnet-name validation to ASCII alphanumerics + hyphens across get_quorum_base_url and any JS-side mirrors.
  • Network::Devnet falls back to port 443 in fetch_masternode_addressesprovider.rs:309-313 selects the DAPI default port via match self.network { Mainnet => 443, Testnet => 1443, _ => 443 }. Devnet now flows through this path as a first-class network; entries without platformHTTPPort will fall back to 443, but devnets typically use 1443 like testnet. Pre-existing match arm not introduced by this PR; in practice the discovery payload usually carries platformHTTPPort.
    • Follow-up: Track a separate issue to revisit Devnet's default DAPI port fallback (1443 vs 443).
  • Boilerplate duplication across the five prefetch_* methods — After this PR, prefetch_mainnet, prefetch_testnet, prefetch_devnet, prefetch_devnet_with_url, and prefetch_local_with_url are five copies of the same ~14-line body. The duplication is pre-existing; this PR follows the established pattern faithfully, but adding two more copies makes consolidation more worthwhile.
    • Follow-up: Open a separate cleanup PR to extract the shared post-construction body into a private helper.

Comment thread packages/js-evo-sdk/src/sdk.ts
Comment thread packages/wasm-sdk/src/context_provider.rs
…t roots

Two guardrails closing security gaps surfaced by review on the flattened devnet URL shape:

1. get_quorum_base_url now rejects reserved devnet names (mainnet/testnet/devnet/local/regtest, case-insensitive). The old https://quorums.devnet.<name>... pattern was structurally namespaced; the new https://quorums.<name>... pattern shares the namespace with non-devnet networks, so without this guard EvoSDK.devnetTrusted('mainnet') would silently fetch real mainnet quorum keys into a Network::Devnet session.

2. new_with_url now requires HTTPS unless the host is loopback (127.0.0.1/localhost/[::1]). The base URL is the SDK's root of trust for proof verification; plaintext over a non-loopback link would let an on-path attacker replace quorum keys and discovered masternode addresses while the UI displayed 'proof verified'.

Adds four unit tests covering both rules.
new_with_url is an explicit escape hatch — the caller types the full URL and picks the scheme. The documented use case ("public DNS isn't deployed yet for this devnet") often coincides with "this early-stage devnet doesn't have a cert yet either," so HTTPS-only here actively blocks the legitimate workflow without protecting any naïve-user surface.

Keeps the reserved-name guard from the prior commit (mainnet/testnet/devnet/local/regtest), which prevents typo-level cross-network footguns without restricting any legitimate use case.
Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

Latest delta (3abe9199..df019bb7) fixes the stale WASM unit test and the EvoSDK.devnet JSDoc precondition. I found no new defects in that delta.

Prior finding reconciliation for df019bb7:

  • FIXED: builder-with-addresses.spec.ts now accepts devnet and expects the updated invalid-network message.
  • FIXED: EvoSDK.devnet JSDoc now says non-trusted devnet requires explicit addresses.
  • STILL VALID: devnet names can still use reserved aliases like mainnet/testnet.
  • STILL VALID: prefetchDevnetWithUrl / new_with_url still allows plaintext HTTP for devnet trust-root fetches.
  • STILL VALID: prefetchDevnetWithUrl still takes String while sibling prefetchLocalWithUrl takes &str.

🟡 2 suggestion(s) | 💬 1 nitpick

3 findings are carried forward from the prior review because they are still valid at this head.

🤖 Prompt for all review comments 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-trusted-context-provider/src/lib.rs`:
- [SUGGESTION] lines 28-53: Devnet-name validator accepts reserved names that alias mainnet/testnet/local quorum hostnames
  `get_quorum_base_url` formats devnet URLs as `https://quorums.<name>.networks.dash.org`, which shares the namespace used by mainnet/testnet/local. The validator only checks character set and hyphen placement, so `mainnet`, `testnet`, `local`, and `regtest` are accepted as devnet names and resolve to the production quorum endpoints while the configured `Network` enum is `Devnet`. Reject reserved names up front.

In `packages/rs-sdk-trusted-context-provider/src/provider.rs`:
- [SUGGESTION] lines 137-177: `new_with_url` reachable from JS via `prefetchDevnetWithUrl` does not require HTTPS for non-local trust-root fetches
  `WasmTrustedContext.prefetchDevnetWithUrl(base_url)` forwards a JS-supplied string into `TrustedHttpContextProvider::new_with_url` with `Network::Devnet`. The constructor performs DNS resolution only on native targets and never enforces the URL scheme, so callers can pass `http://...` and fetch quorum keys / masternode discovery data over plaintext HTTP. Require HTTPS for mainnet/testnet/devnet and preserve HTTP only for local/regtest loopback use cases.

In `packages/wasm-sdk/src/context_provider.rs`:
- [NITPICK] lines 209-234: `prefetchDevnetWithUrl` takes `String` while sibling `prefetchLocalWithUrl` takes `&str`
  The wasm-bindgen surface is inconsistent: `prefetch_devnet_with_url(base_url: String)` vs `prefetch_local_with_url(base_url: &str)`. Both forms copy the JS string into Rust, so this is API-surface noise rather than a correctness issue.

Reviewed commit: df019bb7

Comment thread packages/wasm-sdk/src/context_provider.rs
Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

Prior finding reconciliation for 8daa3d5: carried forward STILL VALID prior findings: #2:trusted-context-custom-url-https, #4:wasm-prefetch-devnet-string-asymmetry; fixed prior findings: #1:trusted-context-devnet-reserved-name-alias-primary, #3:trusted-context-devnet-reserved-name-alias-duplicate. Prior reserved-name items #1 and #3 are duplicates and are consolidated into one public finding when still valid. Latest-delta findings are listed separately when present.

At head 8daa3d5, the prior reserved-devnet-name finding is FIXED (validator + tests at lib.rs:47-60 and provider.rs:836-859). Two prior findings carry forward: (1) new_with_url still accepts arbitrary URL schemes including plaintext http:// for trusted contexts, and this PR widens the surface by adding prefetchMainnetWithUrl/prefetchTestnetWithUrl plus quorumUrl overrides in EvoSDK; (2) the prefetch*WithUrl parameter shapes are inconsistent (String for mainnet/testnet/devnet, &str for local). No new defects introduced by the latest delta; the prefetch_for refactor is clean.

🟡 1 suggestion(s) | 💬 1 nitpick(s)

1 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments 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-trusted-context-provider/src/provider.rs`:
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/src/provider.rs:137-177: Plaintext-scheme trust-root URLs are still accepted; this PR widens the surface to mainnet/testnet
  `TrustedHttpContextProvider::new_with_url` performs no URL-scheme check, and on wasm/iOS it skips the DNS pre-resolution as well (provider.rs:142-144). The HTTPS-required guard added in 586df73c33 was explicitly reverted in 8daa3d5a12 to preserve an escape hatch for early devnets lacking certs. That rationale is reasonable for `Network::Devnet`/`Network::Regtest`, but this same PR newly exposes the same plaintext sink for production networks: commit d2a7c1b810 added `WasmTrustedContext::prefetchMainnetWithUrl` / `prefetchTestnetWithUrl` (context_provider.rs:124-158), and `js-evo-sdk` now forwards `quorumUrl` for `network: 'mainnet'` / `'testnet'` (sdk.ts). The base URL is the SDK's root of trust for proof verification: an attacker who can MITM a plaintext fetch can substitute quorum public keys (forging proofs the SDK will accept) and replace the discovered masternode address list (funneling later DAPI traffic through attacker-controlled nodes), while the SDK still reports proofs as verified. For `Mainnet`/`Testnet` there is no `no-cert-yet` workflow that justifies plaintext. Recommend enforcing `https` when `network` is `Mainnet` or `Testnet`, keeping the deferred behavior for `Devnet`/`Regtest`. This is carried forward from prior finding `trusted-context-custom-url-https`; the devnet/regtest scope is intentionally deferred per the revert commit, but the new mainnet/testnet surface is in scope for this PR and not covered by the cited rationale.
Out-of-scope follow-up suggestions (3)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

  • Reserved-name validator can be sidestepped by non-ASCII alphanumeric look-alikesget_quorum_base_url at lib.rs:36 uses char::is_alphanumeric(), which accepts non-ASCII alphabetics; the reserved-name guard at lib.rs:52-60 lowercases via to_ascii_lowercase and compares against ASCII strings. A devnet name like mаinnet (Cyrillic а) passes the alphanumeric check, is unchanged by to_ascii_lowercase, and slips past the reserved-name match. The IDN-encoded hostname differs from the real quorums.mainnet.networks.dash.org, so this is not a direct alias bypass, but it is a UI homograph risk for tools displaying the chosen devnet name. The alphanumeric rule predates this PR, so out of scope; a follow-up tightening to ASCII alphanumeric + hyphen would close it cleanly.
    • Follow-up: Open a separate issue to tighten devnet-name validation to is_ascii_alphanumeric() + hyphen.
  • EvoSDK.mainnetTrusted / testnetTrusted JSDoc and option types don't surface the new quorumUrl override — Commit d2a7c1b made quorumUrl meaningful on every trusted network, but only EvoSDK.devnetTrusted documents the option. mainnetTrusted/testnetTrusted accept only ConnectionOptions per their typings, so callers cannot discover the override from types alone. Documentation/typing gap only.
    • Follow-up: Add quorumUrl?: string to the option type of mainnetTrusted/testnetTrusted and document the staging-endpoint use case.
  • WasmContext silently returns Ok(None) for get_token_configuration/get_data_contract in non-trusted mode — Pre-existing in WasmContext (context_provider.rs:43-68): the non-trusted path returns Ok(None) and only logs a tracing warning when a token configuration is requested. JS callers get a downstream proof-verification failure instead of a clear 'trusted context required' error. Unchanged by this PR.
    • Follow-up: Track separately to surface a structured error earlier when token/data-contract lookups are requested without a trusted context.

Review details

🤖 Prompt for all review comments 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-trusted-context-provider/src/provider.rs`:
- [SUGGESTION] lines 137-177: Plaintext-scheme trust-root URLs are still accepted; this PR widens the surface to mainnet/testnet
  `TrustedHttpContextProvider::new_with_url` performs no URL-scheme check, and on wasm/iOS it skips the DNS pre-resolution as well (provider.rs:142-144). The HTTPS-required guard added in 586df73c33 was explicitly reverted in 8daa3d5a12 to preserve an escape hatch for early devnets lacking certs. That rationale is reasonable for `Network::Devnet`/`Network::Regtest`, but this same PR newly exposes the same plaintext sink for production networks: commit d2a7c1b810 added `WasmTrustedContext::prefetchMainnetWithUrl` / `prefetchTestnetWithUrl` (context_provider.rs:124-158), and `js-evo-sdk` now forwards `quorumUrl` for `network: 'mainnet'` / `'testnet'` (sdk.ts). The base URL is the SDK's root of trust for proof verification: an attacker who can MITM a plaintext fetch can substitute quorum public keys (forging proofs the SDK will accept) and replace the discovered masternode address list (funneling later DAPI traffic through attacker-controlled nodes), while the SDK still reports proofs as verified. For `Mainnet`/`Testnet` there is no `no-cert-yet` workflow that justifies plaintext. Recommend enforcing `https` when `network` is `Mainnet` or `Testnet`, keeping the deferred behavior for `Devnet`/`Regtest`. This is carried forward from prior finding `trusted-context-custom-url-https`; the devnet/regtest scope is intentionally deferred per the revert commit, but the new mainnet/testnet surface is in scope for this PR and not covered by the cited rationale.

In `packages/wasm-sdk/src/context_provider.rs`:
- [NITPICK] lines 124-219: `prefetch*WithUrl` signatures are inconsistent across the four URL-taking factories
  After the `prefetch_for` refactor there are four `prefetch*_with_url` async factories exposed across the wasm-bindgen boundary: `prefetch_mainnet_with_url(base_url: String)` (line 125), `prefetch_testnet_with_url(base_url: String)` (line 149), `prefetch_devnet_with_url(base_url: String)` (line 185), and `prefetch_local_with_url(base_url: &str)` (line 210). wasm-bindgen copies the JS string into Rust either way, so this has no correctness or memory impact, but the surface is noticeably asymmetric — three siblings use `String`, only `prefetch_local_with_url` still uses `&str` (which then internally calls `.to_string()` at line 216). Align all four on one shape (`String` is the simpler fix, matches the other three, and removes the internal `.to_string()`). Carried forward from prior finding `wasm-prefetch-devnet-string-asymmetry` — now inverted, with local being the odd one out.

Reviewed commit: 8daa3d5a

Comment thread packages/wasm-sdk/src/context_provider.rs
… URLs

This PR added prefetchMainnetWithUrl/prefetchTestnetWithUrl to support testing against staging or self-hosted quorums. Production networks have HTTPS deployed and there's no legitimate plaintext workflow on that surface — a typo into http:// silently weakens the SDK's root of trust (quorum public keys + discovered masternode addresses) and any MitM can forge proofs the SDK will then accept as verified.

Devnet/Regtest keep the plaintext escape hatch (early devnets without certs, loopback sidecars) per the previous revert discussion.
Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

Prior finding reconciliation for 1051e4e: carried forward STILL VALID prior findings: #1:trusted-context-plaintext-url-surface-mainnet-testnet, #2:wasm-prefetch-with-url-signature-inconsistency; fixed prior findings: none. Latest-delta findings are listed separately when present.

Latest delta (1051e4e) is whitespace-only (cargo fmt) and introduces no new behavior. Both prior findings are STILL VALID at current head and carried forward: plaintext-scheme trust-root URLs are accepted by new_with_url for mainnet/testnet/devnet (the HTTPS guard was explicitly reverted in 8daa3d5 as an intentional escape hatch for early devnets), and the four prefetch*WithUrl wasm-bindgen factories use inconsistent String vs &str parameter shapes. No new defects in the latest delta.

🟡 1 suggestion(s) | 💬 1 nitpick(s)

1 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments 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-trusted-context-provider/src/provider.rs`:
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/src/provider.rs:137-177: Plaintext trust-root URLs accepted across mainnet/testnet/devnet via new URL-taking factories
  `TrustedHttpContextProvider::new_with_url` performs no URL-scheme validation, and on `wasm32`/`ios` builds it also skips `verify_domain_resolves` (provider.rs:142-144). The HTTPS-required guard added in 586df73c was deliberately reverted in 8daa3d5a to preserve an escape hatch for early devnets without deployed DNS. This PR widens the reach of that escape hatch by exposing URL-taking factories not just for devnet but for mainnet (`prefetchMainnetWithUrl`) and testnet (`prefetchTestnetWithUrl`) as well (context_provider.rs:124-194), and routes the JS-side `EvoSDKOptions.quorumUrl` through the same path. The trust-root response bootstraps quorum public keys used directly in proof verification, so a network-positioned attacker who can MITM a plaintext fetch can substitute attacker-controlled quorum keys and forge proofs that the SDK will accept. The browser/iOS branches are the most exposed because they don't even pre-resolve the host.

  Given the revert was intentional, this may be an accepted-risk choice — but at minimum the surface should be narrowed (require `https` for non-loopback hosts on mainnet/testnet, allow `http://127.0.0.1`/`localhost`/`::1` for the local sidecar use case) or the policy explicitly documented on `new_with_url` so future callers and JS integrators understand HTTPS is their responsibility. Both Claude and Codex independently flagged this and agree on the mitigation shape.

Review details

🤖 Prompt for all review comments 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-trusted-context-provider/src/provider.rs`:
- [SUGGESTION] lines 137-177: Plaintext trust-root URLs accepted across mainnet/testnet/devnet via new URL-taking factories
  `TrustedHttpContextProvider::new_with_url` performs no URL-scheme validation, and on `wasm32`/`ios` builds it also skips `verify_domain_resolves` (provider.rs:142-144). The HTTPS-required guard added in 586df73c was deliberately reverted in 8daa3d5a to preserve an escape hatch for early devnets without deployed DNS. This PR widens the reach of that escape hatch by exposing URL-taking factories not just for devnet but for mainnet (`prefetchMainnetWithUrl`) and testnet (`prefetchTestnetWithUrl`) as well (context_provider.rs:124-194), and routes the JS-side `EvoSDKOptions.quorumUrl` through the same path. The trust-root response bootstraps quorum public keys used directly in proof verification, so a network-positioned attacker who can MITM a plaintext fetch can substitute attacker-controlled quorum keys and forge proofs that the SDK will accept. The browser/iOS branches are the most exposed because they don't even pre-resolve the host.

Given the revert was intentional, this may be an accepted-risk choice — but at minimum the surface should be narrowed (require `https` for non-loopback hosts on mainnet/testnet, allow `http://127.0.0.1`/`localhost`/`::1` for the local sidecar use case) or the policy explicitly documented on `new_with_url` so future callers and JS integrators understand HTTPS is their responsibility. Both Claude and Codex independently flagged this and agree on the mitigation shape.

In `packages/wasm-sdk/src/context_provider.rs`:
- [NITPICK] lines 124-219: `prefetch*WithUrl` factories mix `String` and `&str` parameter shapes
  `prefetch_mainnet_with_url` (l.125), `prefetch_testnet_with_url` (l.149), and `prefetch_devnet_with_url` (l.185) take `base_url: String`; `prefetch_local_with_url` (l.210-211) takes `base_url: &str` and calls `.to_string()` internally. wasm-bindgen accepts both for JS-string-backed arguments, so the JS-visible signatures are identical — this is API-surface noise rather than a correctness issue. Since the PR is already touching this family of exports, converging on one shape (`String`, to match the other three) would keep the Rust wrapper layer consistent and make future maintenance uniform.

Reviewed commit: 1051e4e3

Comment thread packages/wasm-sdk/src/context_provider.rs
Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

Cumulative incremental review of PR #3748 at head 3752ed7. Prior findings status: (1) stale prefetchDevnet doc comment — FIXED; (2) silently-discarded quorumUrl on non-trusted devnet — FIXED; (3) mainnet/testnet plaintext quorum URLs — FIXED in this delta via the new HTTPS guard in TrustedHttpContextProvider::new_with_url plus a regression test; (4) devnet plaintext escape hatch — INTENTIONALLY DEFERRED per the in-source comment justifying early-stage devnets and loopback sidecars (recorded out-of-scope, not blocking). New latest-delta findings: none blocking. Remaining items are low-confidence nits and one symmetry suggestion in js-evo-sdk.

🟡 1 suggestion(s) | 💬 3 nitpick(s)

🤖 Prompt for all review comments 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/js-evo-sdk/src/sdk.ts`:
- [SUGGESTION] packages/js-evo-sdk/src/sdk.ts:76-88: `devnetName` is silently accepted on non-devnet networks
  The constructor guards `quorumUrl` against `trusted === false` but does not guard `devnetName` against non-devnet networks. `new EvoSDK({ network: 'testnet', devnetName: 'paloma' })` is accepted and the field is stored on `this.options`, but `connect()` only consults `devnetName` when `network === 'devnet'`. The misconfig is benign at runtime but masks typos like `network: 'testent'`. Add a symmetric check — `if (devnetName && network !== 'devnet') throw …` — to match the validation surface already established for `quorumUrl`.

Comment thread packages/js-evo-sdk/src/sdk.ts
Comment thread packages/rs-sdk-trusted-context-provider/src/provider.rs Outdated
Comment thread packages/rs-sdk-trusted-context-provider/src/provider.rs
Comment thread packages/wasm-sdk/src/context_provider.rs
Symmetric with the existing quorumUrl-without-trusted guard: devnetName has no effect outside network === 'devnet', so silently accepting it masks typos like { network: 'testent', devnetName: 'paloma' } where the user meant 'devnet'.
… consistency

Three nitpicks from review:

1. HTTPS gate now parses with url::Url and compares scheme case-insensitively, accepting 'HTTPS://' and rejecting malformed URLs cleanly instead of a byte-prefix match.

2. Add positive-path tests asserting the gate does NOT reject https on mainnet/testnet or plaintext on devnet/regtest — guards against a future refactor broadening the gate silently.

3. Normalize prefetchLocalWithUrl to take String like the other three sibling factories.
Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

Cumulative incremental review of PR #3748 at head 7c362c4. The latest delta (3752ed7..7c362c4) is a 9-line constructor guard in js-evo-sdk that rejects devnetName on non-devnet networks; it is well-scoped, has a regression test, and introduces no new defects. Prior-finding reconciliation: FIXED — devnetName typo guard (verified at packages/js-evo-sdk/src/sdk.ts:85-89 with test at tests/unit/sdk.spec.ts:293-296); quorumUrl discarded on non-trusted (verified at sdk.ts:90-92); plaintext mainnet/testnet trust roots (verified at provider.rs:149-156); stale prefetchDevnet rustdoc; reserved devnet names. STILL VALID (carried forward) — case-sensitive HTTPS prefix match; HTTPS-gate test covers only the negative path; prefetch*WithUrl String vs &str inconsistency; book and js-evo-sdk README still document only three networks and omit the new devnet factories this PR exposes. INTENTIONALLY DEFERRED — devnet/regtest plaintext trust-root escape hatch (documented design, scope-gated to non-production networks). DROPPED — codex's newDevnet() empty-AddressList blocking claim: the raw wasm WasmSdkBuilder::new_devnet() rustdoc at packages/wasm-sdk/src/sdk.rs:250-255 documents the required pairing, and the JS-facing EvoSDK constructor (sdk.ts:76-89) enforces it for the first-class entry points this PR adds, so the documented contract is not violated through this PR's stated surface. No new findings in the latest delta.

_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3748 7c362c4
STDOUT:

STDERR:
Traceback (most recent call last):
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 221, in
result = post_review(repo, pr_number, h), so I posted the same verified findings as a top-level review body._

Reviewed commit: 7c362c4

🟡 2 suggestion(s) | 💬 3 nitpick(s)

Verified findings

suggestion: [carried-forward] README documents `network` as only 'testnet' | 'mainnet' | 'local' and omits the new devnet factories

packages/js-evo-sdk/README.md (line 48)

This PR adds first-class devnet support via EvoSDK.devnet(name, opts) and EvoSDK.devnetTrusted(name, opts) (packages/js-evo-sdk/src/sdk.ts:242, 259) and a corresponding network: 'devnet' branch in the constructor (sdk.ts:76-89). The npm-facing README still types network as 'testnet' | 'mainnet' | 'local' and the preset-factories sentence at line 55 lists only the six pre-existing factories, so consumers reading the package's top-level usage guide are told network: 'devnet' is invalid and will not discover the new factories. Since this is the public-facing surface this PR adds, the README should reflect it.

suggestion: [carried-forward] Book page still claims 'three built-in networks' and does not document the new devnet factories

book/src/evo-sdk/networks-and-environments.md (line 3)

The Evo SDK book page asserts "three built-in network configurations" and the table only enumerates Testnet/Mainnet/Local. This PR introduces EvoSDK.devnet() and EvoSDK.devnetTrusted() as first-class entry points, plus the prefetchDevnet / prefetchDevnetWithUrl WASM factories and the devnetName / quorumUrl config options. The user-facing docs for the very feature this PR adds remain blank, which materially undermines discoverability of the new API.

nitpick: [carried-forward] `prefetchLocalWithUrl` takes `&str` while the three sibling factories take `String`

packages/wasm-sdk/src/context_provider.rs (line 209)

Verified at head 7c362c4: prefetch_mainnet_with_url (line 125), prefetch_testnet_with_url (line 149), and prefetch_devnet_with_url (line 185) all take base_url: String, but prefetch_local_with_url at line 210 takes &str and immediately calls .to_string() at line 216. wasm-bindgen marshals both shapes identically from JS, so this is not a correctness or memory-safety defect at the FFI boundary — purely a Rust-side inconsistency across four sibling factories. Normalizing to String removes the redundant allocation hop and makes the four factories trivially uniform.

nitpick: [carried-forward] HTTPS gate uses byte-prefix, case-sensitive match

packages/rs-sdk-trusted-context-provider/src/provider.rs (line 149)

Verified at head 7c362c4: the guard is base_url.starts_with("https://"). RFC-3986 schemes are case-insensitive, so "HTTPS://..." is rejected with a misleading 'must use https://' error, and a literal "https://" with an empty authority slips this check and fails downstream with a less actionable error. Fail-closed (over-rejection, never under-acceptance), so not a security defect. Parsing with url::Url::parse(...) and comparing .scheme().eq_ignore_ascii_case("https") would make the gate schema-driven.

nitpick: [carried-forward] HTTPS-gate test pins only the rejection path

packages/rs-sdk-trusted-context-provider/src/provider.rs (line 852)

test_new_with_url_rejects_plaintext_for_production_networks covers the negative arm of the new guard but does not assert the (intentionally permitted) complements: http://... for Network::Devnet / Network::Regtest, and https://... for mainnet/testnet, must not be rejected by the new gate. A future refactor that broadened the gate to all networks would pass the existing test silently. Because verify_domain_resolves is cfg-gated to non-wasm/non-iOS, the positive assertions need to either target a wasm/iOS cfg path or assert that any returned error is not the new NetworkError("... must use https://...") variant.

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

- [SUGGESTION] In `packages/js-evo-sdk/README.md`:48-55: [carried-forward] README documents `network` as only 'testnet' | 'mainnet' | 'local' and omits the new devnet factories
  This PR adds first-class devnet support via `EvoSDK.devnet(name, opts)` and `EvoSDK.devnetTrusted(name, opts)` (packages/js-evo-sdk/src/sdk.ts:242, 259) and a corresponding `network: 'devnet'` branch in the constructor (sdk.ts:76-89). The npm-facing README still types `network` as `'testnet' | 'mainnet' | 'local'` and the preset-factories sentence at line 55 lists only the six pre-existing factories, so consumers reading the package's top-level usage guide are told `network: 'devnet'` is invalid and will not discover the new factories. Since this is the public-facing surface this PR adds, the README should reflect it.
- [SUGGESTION] In `book/src/evo-sdk/networks-and-environments.md`:3-12: [carried-forward] Book page still claims 'three built-in networks' and does not document the new devnet factories
  The Evo SDK book page asserts "three built-in network configurations" and the table only enumerates Testnet/Mainnet/Local. This PR introduces `EvoSDK.devnet()` and `EvoSDK.devnetTrusted()` as first-class entry points, plus the `prefetchDevnet` / `prefetchDevnetWithUrl` WASM factories and the `devnetName` / `quorumUrl` config options. The user-facing docs for the very feature this PR adds remain blank, which materially undermines discoverability of the new API.
- [NITPICK] In `packages/wasm-sdk/src/context_provider.rs`:209-219: [carried-forward] `prefetchLocalWithUrl` takes `&str` while the three sibling factories take `String`
  Verified at head 7c362c4070: `prefetch_mainnet_with_url` (line 125), `prefetch_testnet_with_url` (line 149), and `prefetch_devnet_with_url` (line 185) all take `base_url: String`, but `prefetch_local_with_url` at line 210 takes `&str` and immediately calls `.to_string()` at line 216. wasm-bindgen marshals both shapes identically from JS, so this is not a correctness or memory-safety defect at the FFI boundary — purely a Rust-side inconsistency across four sibling factories. Normalizing to `String` removes the redundant allocation hop and makes the four factories trivially uniform.
- [NITPICK] In `packages/rs-sdk-trusted-context-provider/src/provider.rs`:149-156: [carried-forward] HTTPS gate uses byte-prefix, case-sensitive match
  Verified at head 7c362c4070: the guard is `base_url.starts_with("https://")`. RFC-3986 schemes are case-insensitive, so `"HTTPS://..."` is rejected with a misleading 'must use https://' error, and a literal `"https://"` with an empty authority slips this check and fails downstream with a less actionable error. Fail-closed (over-rejection, never under-acceptance), so not a security defect. Parsing with `url::Url::parse(...)` and comparing `.scheme().eq_ignore_ascii_case("https")` would make the gate schema-driven.
- [NITPICK] In `packages/rs-sdk-trusted-context-provider/src/provider.rs`:852-868: [carried-forward] HTTPS-gate test pins only the rejection path
  `test_new_with_url_rejects_plaintext_for_production_networks` covers the negative arm of the new guard but does not assert the (intentionally permitted) complements: `http://...` for `Network::Devnet` / `Network::Regtest`, and `https://...` for mainnet/testnet, must not be rejected by the new gate. A future refactor that broadened the gate to all networks would pass the existing test silently. Because `verify_domain_resolves` is cfg-gated to non-wasm/non-iOS, the positive assertions need to either target a wasm/iOS cfg path or assert that any returned error is not the new `NetworkError("... must use https://...")` variant.

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

Cumulative incremental review of PR #3748 at head dd8cad3. Latest delta (7c362c4..dd8cad3) is a focused hardening pass on the trusted-context provider: the HTTPS gate now uses Url::parse + eq_ignore_ascii_case("https"), three positive-path tests were added, and prefetch_local_with_url was normalized to base_url: String to match its three sibling factories. No new defects in the latest delta. Prior-finding reconciliation: FIXED — case-sensitive HTTPS prefix check; HTTPS-gate test pinning only the negative path; prefetchLocalWithUrl &str-vs-String inconsistency; devnetName silently accepted on non-devnet; quorumUrl discarded when non-trusted; reserved devnet-name aliasing; stale wasm doc/error strings. STILL VALID (carried forward) — js-evo-sdk README still types network as 'testnet'|'mainnet'|'local' and omits the new devnet factories; book Networks page still claims 'three built-in networks'. INTENTIONALLY DEFERRED — devnet/regtest plaintext quorum URL escape hatch (prior reviewer accepted tradeoff; out-of-scope follow-up). Codex flagged two items as 'blocking' that are actually pre-existing documented behavior (trusted-context replacing builder addresses; discovered-DAPI hosts rewritten to https://); both are out of this PR's scope and not new defects. Net: only two carried-forward documentation suggestions remain.

🟡 2 suggestion(s)

2 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments 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/js-evo-sdk/README.md`:
- [SUGGESTION] packages/js-evo-sdk/README.md:48-55: [carried-forward] README omits the new devnet factories and types `network` without `'devnet'`
  Verified STILL VALID at head dd8cad3090. This PR adds first-class devnet support — `EvoSDK.devnet()`, `EvoSDK.devnetTrusted()`, and a `network: 'devnet'` constructor branch with `devnetName` / `quorumUrl` options — but the npm-facing README at L48 still types `network` as `'testnet' | 'mainnet' | 'local'`, and the preset-factories paragraph at L55 lists only the six pre-existing factories. Consumers reading the package's top-level usage guide will not discover the new API surface this PR introduces.

In `book/src/evo-sdk/networks-and-environments.md`:
- [SUGGESTION] book/src/evo-sdk/networks-and-environments.md:3-12: [carried-forward] Book page still claims 'three built-in networks' and does not document devnet factories
  Verified STILL VALID at head dd8cad3090. L3-4 still asserts "three built-in network configurations" and the table at L8-12 only enumerates Testnet/Mainnet/Local. This PR promotes devnet to a first-class entry point (`EvoSDK.devnetTrusted`, `prefetchDevnet`/`prefetchDevnetWithUrl`, `devnetName`/`quorumUrl` config options). The user-facing book page for the very feature this PR adds remains blank.

README config table now lists addresses/devnetName/quorumUrl and includes 'devnet' in the network union; factories paragraph adds EvoSDK.devnet/devnetTrusted with usage snippets. Book networks-and-environments page now covers four built-in networks with a dedicated Devnets section showing trusted, custom-URL, and non-trusted variants.
@github-actions
Copy link
Copy Markdown
Contributor

📖 Book Preview built successfully.

Download the preview from the workflow artifacts.
To view locally: download the artifact, unzip, and open index.html.

Updated at 2026-05-27T17:55:10.153Z

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

Both mandatory prior documentation findings are fixed at c32f2466: js-evo-sdk/README.md now documents the devnet network value, devnetName/quorumUrl/addresses, and the EvoSDK.devnet / EvoSDK.devnetTrusted factories; book/src/evo-sdk/networks-and-environments.md now says “four built-in network configurations”, adds Devnet, and includes a dedicated Devnets section.

Two non-blocking issues remain: one suggestion around addresses being silently overwritten in trusted devnet mode, and one README wording nit.

🟡 1 suggestion(s) | 💬 1 nitpick(s)

🤖 Prompt for all review comments 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/js-evo-sdk/src/sdk.ts`:
- [SUGGESTION] packages/js-evo-sdk/src/sdk.ts:165-182: Trusted devnet drops caller-supplied `addresses` — docs/validation should match the actual override semantics
  In `connect()`, the builder is first configured from explicit `addresses` via `WasmSdkBuilder.withAddresses(addresses, network)`, then `builder.withTrustedContext(context)` is applied whenever `trusted: true`. The Rust side (`packages/wasm-sdk/src/sdk.rs:296-313`) unconditionally replaces the address list with `context.discovered_addresses()` whenever the trusted context exposes any addresses — so the caller's explicit addresses are silently discarded before `build()`.
  
  The new devnet API makes this user-visible: `EvoSDK.devnetTrusted(name, { quorumUrl, addresses })` reads naturally as 'trust this quorum, but pin DAPI to these specific endpoints', and the README's `addresses` row says it 'replaces built-in defaults'. In practice it can only override for non-trusted mode, or for trusted setups where the context happens to discover no addresses.
  
  Either (a) document explicitly that `addresses` is ignored under `trusted: true` when the context provides any discovered addresses (and ideally warn/throw at construction when both are supplied), or (b) thread explicit `addresses` through `withTrustedContext` so they take precedence over discovered ones. Pre-existing for trusted mainnet/testnet, but this PR is what exposes it as a public option on the devnet factory.

In `packages/js-evo-sdk/README.md`:
- [NITPICK] packages/js-evo-sdk/README.md:51-51: README row for `devnetName` is internally inconsistent
  Two small inaccuracies vs. the constructor in `packages/js-evo-sdk/src/sdk.ts`:
  
  1. 'Required when `network: "devnet"` and `trusted: true`' — the constructor (sdk.ts L78-81) accepts trusted devnet with `devnetName` *or* `quorumUrl`; either is sufficient. The very next table row documents `quorumUrl` as an override, and tests assert trusted devnet works with only `quorumUrl`.
  2. 'ignored otherwise' — for non-devnet networks the constructor (sdk.ts L85-89) throws `EvoSDK: devnetName is only valid when network === 'devnet'`, not silently ignored. The trailing clause 'only valid when `network === "devnet"`' already captures this; 'ignored' contradicts it.

  Suggested wording:
  ```suggestion
  | `devnetName` | `string` | — | Short name of the devnet (e.g. `'paloma'`). Used to derive the trusted-context quorum URL; required for `trusted: true` devnet unless `quorumUrl` is supplied. Only valid when `network === 'devnet'` (rejected on other networks). |
</details>

*Reviewed commit: `c32f2466`*

@QuantumExplorer QuantumExplorer merged commit a29f1c0 into v3.1-dev May 27, 2026
23 checks passed
@QuantumExplorer QuantumExplorer deleted the feat/wasm-sdk-devnet-trusted-context branch May 27, 2026 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants