fix(sdk): honor explicit regtest addresses in dapi-client; tolerate empty discovery in wasm-sdk trusted context - #4538
Conversation
The regtest localhost workaround rewrote EVERY live address to 127.0.0.1:2443+i*100 (the stock local gateway ports), including addresses the caller configured explicitly. A local network that moves its ports (the dashmate e2e suites do, to run next to other networks) had every request silently redirected to whatever squats the stock ports on the machine - on a shared dev box, a completely different network. Only rewrite addresses that carry a non-loopback (docker-internal) host, which is the case the workaround exists for. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…xt prefetch Discovery only feeds the no-explicit-addresses path of withTrustedContext, but a failure made the whole prefetch unusable - and it fails routinely on local networks, where the quorum sidecar's per-masternode version checks reject the gateway's self-signed TLS and report no eligible masternodes. Degrade to a warning and an empty discovered list; SDKs constructed with explicit addresses are unaffected, and the quorum data proof verification needs is fetched before this point. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Cherry-picked from the state-sync e2e branch; the companion e2e assertion stays there (the spec file only exists on that branch). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
|
⛔ Final review complete — 1 blocking finding(s) (commit c3003da) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Sol-only technical fallback
The dapi-client change does not fully meet the PR's stated goal because caller-supplied non-default regtest addresses are still rewritten to the stock local gateway. The WASM fallback is correctly limited to regtest, but it suppresses more discovery failures than the advertised empty-result case and lacks deterministic regression coverage. Source: reviewer backends — Claude (sol-fallback-reviewer-general and sol-fallback-reviewer-rust-quality) and Codex; final verifier backend — Claude Agent SDK.
One or more required Phase-1 GLM Flash lanes remained technically unusable after the bounded exact-model retry. Their evidence was discarded as authoritative, and the complete selected role cohort was rerun fresh on exact gpt-5.6-sol before this fresh Sol verifier produced the final decision. No additional Phase-2 reviewer pass ran.
Review provenance
- Phase 1 GLM evidence: technically unusable after bounded retry; discarded from the decision
- GLM failure attempts:
codex-general-7a7b0ec185e24e1abc9547625b552aef(failed),codex-general-c8cfc5d33caf432d9ab79ff93406551f(failed),codex-rust-quality-0e35283a40384ebaa68eed2cf38840aa(failed),codex-rust-quality-1c3417d033ee4e929bf2af5eb13978b7(completed) - Sol-only fallback reasons:
launch_transport_or_nonzero_exit - Sol-only fallback reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Fresh verifier (Sol):
gpt-5.6-sol— final-verifier - Additional Phase 2 pass: not run; the Sol-only fallback is final
🔴 1 blocking | 🟡 2 suggestion(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-dapi-client/lib/dapiAddressProvider/ListDAPIAddressProvider.js`:
- [BLOCKING] packages/js-dapi-client/lib/dapiAddressProvider/ListDAPIAddressProvider.js:42-43: Explicit non-default regtest addresses are still rewritten
This condition treats only the exact hosts `127.0.0.1` and `localhost` as explicit addresses, but host value does not indicate address provenance. A caller-supplied address such as the README's `127.0.0.2`, a LAN address, or a container hostname still enters this branch and is mutated to `https://127.0.0.1:2443 + i*100`. This was reproduced through `createDAPIAddressProviderFromOptions({ network: 'local', dapiAddresses: ['127.0.0.2:45003:self-signed'] })`, which returned `127.0.0.1:2443`. Gate the compatibility rewrite on whether the provider was constructed from seeds or discovered masternodes, rather than checking two host strings, and add a factory-level regression test for a caller-supplied non-default address.
In `packages/wasm-sdk/src/context_provider.rs`:
- [SUGGESTION] packages/wasm-sdk/src/context_provider.rs:269-280: Only suppress the expected empty-discovery error on regtest
The PR describes tolerating the normal `No eligible masternode addresses discovered` result, but this match suppresses every discovery error on regtest. `fetch_masternode_addresses` can also fail because of transport errors, non-success HTTP responses, malformed JSON, or a sidecar-declared failure, and `fetch_addresses_from` can reject malformed URIs or addresses. Those failures currently become an empty discovered list, so a builder without explicit addresses silently retains its preset addresses and can contact an unintended endpoint. Preserve the provider error through this layer, represent the expected no-eligible-addresses result as a dedicated typed variant, and downgrade only that variant on regtest.
- [SUGGESTION] packages/wasm-sdk/src/context_provider.rs:269-280: Add deterministic tests for the network-scoped discovery fallback
No committed test executes the new branches in `prefetch_for`; existing unit tests construct contexts with `for_testing` and inject `discovered_addresses`, while the functional local check depends on a live endpoint and does not verify the public-network boundary. Add a deterministic HTTP fixture that serves valid `/quorums` and `/previous` responses followed by a `/masternodes` response with no eligible entries. Assert that regtest returns a context with no discovered addresses while mainnet or testnet propagates the same result, and verify that malformed or HTTP-failure responses remain errors on regtest once the catch is narrowed.
| const isLoopback = ['127.0.0.1', 'localhost'].includes(liveAddress.getHost()); | ||
| if (network && network.regtestEnabled && !isLoopback) { |
There was a problem hiding this comment.
🔴 Blocking: Explicit non-default regtest addresses are still rewritten
This condition treats only the exact hosts 127.0.0.1 and localhost as explicit addresses, but host value does not indicate address provenance. A caller-supplied address such as the README's 127.0.0.2, a LAN address, or a container hostname still enters this branch and is mutated to https://127.0.0.1:2443 + i*100. This was reproduced through createDAPIAddressProviderFromOptions({ network: 'local', dapiAddresses: ['127.0.0.2:45003:self-signed'] }), which returned 127.0.0.1:2443. Gate the compatibility rewrite on whether the provider was constructed from seeds or discovered masternodes, rather than checking two host strings, and add a factory-level regression test for a caller-supplied non-default address.
source: ['claude']
There was a problem hiding this comment.
Resolved in c3003da — Explicit non-default regtest addresses are still rewritten no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| let discovered_addresses = match Self::fetch_addresses_from(&inner).await { | ||
| Ok(addresses) => addresses, | ||
| Err(e) if network == dash_sdk::dpp::dashcore::Network::Regtest => { | ||
| tracing::warn!( | ||
| error = %e, | ||
| "trusted context: masternode discovery unavailable, continuing without \ | ||
| discovered addresses (explicitly configured addresses are unaffected)" | ||
| ); | ||
| Vec::new() | ||
| } | ||
| Err(e) => return Err(e), | ||
| }; |
There was a problem hiding this comment.
🟡 Suggestion: Only suppress the expected empty-discovery error on regtest
The PR describes tolerating the normal No eligible masternode addresses discovered result, but this match suppresses every discovery error on regtest. fetch_masternode_addresses can also fail because of transport errors, non-success HTTP responses, malformed JSON, or a sidecar-declared failure, and fetch_addresses_from can reject malformed URIs or addresses. Those failures currently become an empty discovered list, so a builder without explicit addresses silently retains its preset addresses and can contact an unintended endpoint. Preserve the provider error through this layer, represent the expected no-eligible-addresses result as a dedicated typed variant, and downgrade only that variant on regtest.
source: ['claude']
| let discovered_addresses = match Self::fetch_addresses_from(&inner).await { | ||
| Ok(addresses) => addresses, | ||
| Err(e) if network == dash_sdk::dpp::dashcore::Network::Regtest => { | ||
| tracing::warn!( | ||
| error = %e, | ||
| "trusted context: masternode discovery unavailable, continuing without \ | ||
| discovered addresses (explicitly configured addresses are unaffected)" | ||
| ); | ||
| Vec::new() | ||
| } | ||
| Err(e) => return Err(e), | ||
| }; |
There was a problem hiding this comment.
🟡 Suggestion: Add deterministic tests for the network-scoped discovery fallback
No committed test executes the new branches in prefetch_for; existing unit tests construct contexts with for_testing and inject discovered_addresses, while the functional local check depends on a live endpoint and does not verify the public-network boundary. Add a deterministic HTTP fixture that serves valid /quorums and /previous responses followed by a /masternodes response with no eligible entries. Assert that regtest returns a context with no discovered addresses while mainnet or testnet propagates the same result, and verify that malformed or HTTP-failure responses remain errors on regtest once the catch is narrowed.
source: ['claude']
The regtest docker-IP workaround exempted only the literal hosts 127.0.0.1 and localhost, so any other caller-supplied address (127.0.0.2, a LAN IP, a container hostname) was still clobbered to the stock local gateway ports. Gate the rewrite on address provenance instead: only addresses discovered from the masternode list (they carry a proRegTxHash) can hold an unreachable docker-internal host, so only those are rewritten. Adds a factory-level regression test for a caller-supplied non-default regtest address. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Blocking finding confirmed and fixed properly (pushed in
The two wasm-sdk suggestions (typed no-eligible-addresses error variant instead of the broad regtest-only catch, and deterministic HTTP-fixture tests for 🤖 Posted autonomously by Claude on behalf of pasta. |
2 participants
Issue being fixed or feature implemented
Two client-side bugs surfaced by state-sync e2e work, independent of the feature branches:
ListDAPIAddressProvider.getLiveAddress()rewrote EVERY address to127.0.0.1:2443 + i*100(the stock local-preset ports) whenever the network is regtest — so a client configured with explicit non-default addresses (e.g. an isolated test network, or any second local network on one machine) silently sent every wallet/DAPI request to whatever occupies the stock ports. On a shared dev machine that was a different chain entirely, which produced a long goose chase of "wallet funding is broken" reports: subscriptions landed on the wrong network's rs-dapi, and wallets watched a chain their payments were never on.What was done?
js-dapi-client: explicit loopback addresses are honored verbatim on regtest; the rewrite only applies where it originally made sense (+ unit test).wasm-sdk: trusted-context prefetch tolerates empty masternode discovery on regtest instead of failing the whole context build.How Has This Been Tested?
js-dapi-client unit suite: 319 passing including the new rewrite regression test. The wasm-sdk change was validated live: it is what allowed the state-sync e2e (#4530) to seed identities/contracts/documents through an isolated-port local network and re-read them proof-verified — the full suite there runs green with these fixes.
Both commits were code-reviewed (code-review-validator) as part of the e2e branch changeset before being cherry-picked here.
Breaking Changes
None intended. Behavior change is regtest-only: clients that (perhaps unknowingly) relied on the address rewrite to reach stock-port local networks while configured with different explicit addresses will now use their configured addresses — which is the documented contract.
Checklist:
🤖 Generated with Claude Code