fix(sdk): sdk emits incompatible getDocuments wire against pre-v3.1 networks (QueryContext approach)#3711
Conversation
… to V0 + correct decode-error string PR #3633 ("feat(platform): getDocuments v1") bumped DriveAbciQueryDocumentVersions::document_query.{max_version, default_current_version} from 0 -> 1 inside the single DRIVE_ABCI_QUERY_VERSIONS_V1 bundle, retroactively claiming every historical protocol version reports V1 doc-query. But v3.0.0 (PROTOCOL_VERSION_11, the live testnet release at this work's time) ships and only understands the V0 wire shape on the server. This commit forks a new DRIVE_ABCI_QUERY_VERSIONS_V0 bundle (verbatim copy of _V1 except document_query is pinned to {min:0, max:0, default_current:0}) and re-binds PROTOCOL_VERSION_V1..V11 to it. PV_12 (v3.1-dev, the genuine V1 consumer) is untouched. Also fixes the misleading 'could not decode data contracts query' copy-paste typo in rs-drive-abci/src/query/document_query/mod.rs - the failing handler is the documents query. This commit is shape-equivalent to the platform-version + drive-abci pieces of #3699; the SDK-side changes (encoder dispatch, Fetch::Query split, QueryContext) land in the follow-up commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…re document encoder Lands the SDK-side half of the v3.0-testnet getDocuments-decode-error fix (paired with the platform-version V0 re-pin in the previous commit). Same observable behaviour as PR #3699 but with a cleaner shape: instead of threading `&Sdk` through `Query::query` (which drags every Sdk-transitive dep into the encoder layer), the trait takes a small borrow-style `QueryContext` bundle. # What changed ## New: `QueryContext<'a>` (packages/rs-sdk/src/platform/query_context.rs) ``` pub struct QueryContext<'a> { pub request_settings: &'a RequestSettings, pub protocol_version: &'a PlatformVersion, pub prove: bool, } ``` Constructed via `Sdk::query_context()` for normal callers, or directly in unit tests that exercise the encoder without spinning up an `Sdk::new_mock()`. Includes `without_proofs()` for the FetchUnproved code path. ## Trait `Query<T>::query` signature Was `fn query(self, prove: bool) -> Result<T, Error>`. Now `fn query(&self, ctx: &QueryContext<'_>) -> Result<T, Error>`. `&self` instead of `self` so callers don't have to clone before the encoder fires; the encoder itself clones internally where the wire type requires owned data. ## `SdkBuilder::with_initial_version(&PlatformVersion)` Seeds the SDK protocol-version atomic at boot without disabling auto-detect. Lets a wallet boot against v3.0 testnet (PV_11 -> V0 wire) and continue working as testnet upgrades; the existing `maybe_update_protocol_version` ratchet still fires on the first network response with a higher PV. ## `TryFromPlatformVersioned<DocumentQuery> for GetDocumentsRequest` PV-aware wire encoder. Dispatches on `platform_version.drive_abci.query.document_query.default_current_version`: 0 -> V0 (legacy CBOR where/order_by), 1 -> V1 (structured WhereClause/OrderClause + optional limit + selects/group_by/having/ offset). V1-only features (group_by, having, count_star projections) return `Error::Config` against a V0 dispatch with a clear 'requires Platform v3.1+' message rather than emitting malformed V0 the server would round-trip and reject opaquely. ## `Fetch::Query` / `Fetch::Request` split `Fetch::Query` is the user-facing rich query that `FromProof` binds to (and what the mock cache keys on). `Fetch::Request` is the wire-encoded proto. For documents: `type Query = DocumentQuery; type Request = GetDocumentsRequest`. For every other operation (~63 sites) it's `type Query = Self::Request` - one extra line per impl. Wire encoding moves from `DocumentQuery::execute_transport` (where `&Sdk` was unreachable) into `impl Query<GetDocumentsRequest> for DocumentQuery::query(&self, ctx)` (where `ctx.protocol_version` is in hand). The 'smuggle PV through a struct field plus Any-downcast in the blanket impl' workaround from earlier iterations is gone - wire-version awareness is compiler-enforced per request type, and the same pattern extends to any future versioned request type as a 5-line `impl Query<NewWireType> for NewRichType` away. ## Call-site sweep - `fetch.rs` / `fetch_many.rs` -> `query.query(&sdk.query_context())` - `fetch_unproved.rs` -> `query.query(&sdk.query_context().without_proofs())` - `MockDashPlatformSdk::expect_fetch{,_many}` -> same shape - 70+ `Query<T>` impl sites across rs-sdk + rs-sdk-ffi updated to the new signature # Difference vs #3699 #3699 routes `&Sdk` directly into the encoder. This PR routes a small `&QueryContext` borrow instead. Trade-off: | Aspect | #3699 (&Sdk) | this PR (QueryContext) | |--------|--------------|------------------------| | Test ergonomics | requires `Sdk::new_mock()` | construct directly | | Coupling | encoder layer depends on full Sdk | only on the fields it reads | | Lifetime surface | none extra | `QueryContext<'a>` borrow | | New type | none | one struct, three fields | | LOC delta | comparable | comparable | # Breaking changes Target branch is v3.1-dev, so out-of-tree consumers will need: - `Query<T>::query()`: was `fn query(self, prove: bool)`, now `fn query(&self, ctx: &QueryContext<'_>)`. External impls must switch to `&self` and pull `prove` from `ctx.prove`. - `Fetch`: new `type Query` associated type. External impls need `type Query = Self::Request;` (default-style) unless they want the rich/wire split. - `DocumentQuery` no longer implements `TransportRequest`. Code sending `DocumentQuery` via `DapiRequest` directly will not compile - use `Document::fetch(sdk, query)` (handles PV-aware encoding internally) or `GetDocumentsRequest::try_from_platform_versioned(query, sdk.version())?` for the explicit transport request. - `SdkBuilder::with_initial_version` is purely additive alongside the existing `with_version`. # Test plan - cargo check --workspace: clean - cargo fmt --all: no diff - cargo clippy --workspace -- -D warnings: clean - cargo test -p dash-sdk --features mocks,offline-testing --lib: 133/0/6 - cargo test -p dash-sdk --features mocks,offline-testing --tests: 127/0/8 - cargo test -p drive-abci --lib query: 585/0/1 - cargo test -p platform-version: 0/0/0 (no harness) - cargo test -p platform-wallet --no-run: builds clean The four document offline tests carry the same `#[ignore]` pin as PR #3699's `fedfce8396` (mock cache key changed by the Fetch::Query/Request split) - vectors will be regenerated against testnet in a follow-up. # New test demonstrating the win `encoder_dispatches_v0_via_query_context_without_sdk` constructs a `QueryContext` directly with a synthetic V0-pinned PlatformVersion, calls `DocumentQuery::query(&ctx)`, and asserts the wire shape is `GetDocumentsRequestV0`. The same code with the latest PlatformVersion asserts V1. Encoder is now testable without `Sdk::new_mock()`. # Related - Replaces approach in #3699 (`&Sdk` threading). If reviewers prefer this shape, #3699 and the sibling #3549 wallet PR will be reverted and this approach cherry-picked. - Causes: #3633 (getDocuments v1 - introduced the version asymmetry) - Compounds: #3695 (rs-dapi-client per-error-class ban policy) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR refactors the SDK query pipeline from a boolean ChangesQuery Context Refactoring and Version-Aware DocumentQuery Encoding
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-sdk/src/platform/documents/document_query.rs (1)
567-570: 💤 Low valueRedundant match expression in V0 start field encoding.
The match maps each
Startvariant to itself, which is effectively a no-op. Unlike the V1 path (which convertsStart→V1Start), V0 uses the sameStarttype on both sides.♻️ Simplify by passing `start` directly
- start: start.map(|s| match s { - Start::StartAfter(b) => Start::StartAfter(b), - Start::StartAt(b) => Start::StartAt(b), - }), + start,🤖 Prompt for 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. In `@packages/rs-sdk/src/platform/documents/document_query.rs` around lines 567 - 570, The code is performing a redundant match when populating the V0 `start` field: `start: start.map(|s| match s { Start::StartAfter(b) => Start::StartAfter(b), Start::StartAt(b) => Start::StartAt(b), })`; replace this no-op mapping by passing `start` directly (e.g., `start: start`) so the V0 encoder uses the existing `Start` value without unnecessary pattern matching. Ensure you update the construction that sets the V0 `start` field (the closure mapping the `Start` enum) to use the direct value.
🤖 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.
Nitpick comments:
In `@packages/rs-sdk/src/platform/documents/document_query.rs`:
- Around line 567-570: The code is performing a redundant match when populating
the V0 `start` field: `start: start.map(|s| match s { Start::StartAfter(b) =>
Start::StartAfter(b), Start::StartAt(b) => Start::StartAt(b), })`; replace this
no-op mapping by passing `start` directly (e.g., `start: start`) so the V0
encoder uses the existing `Start` value without unnecessary pattern matching.
Ensure you update the construction that sets the V0 `start` field (the closure
mapping the `Start` enum) to use the direct value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c6d19fa-45ff-4010-8526-2559fa3aa09f
📒 Files selected for processing (49)
packages/rs-drive-abci/src/query/document_query/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v0.rspackages/rs-platform-version/src/version/v1.rspackages/rs-platform-version/src/version/v10.rspackages/rs-platform-version/src/version/v11.rspackages/rs-platform-version/src/version/v2.rspackages/rs-platform-version/src/version/v3.rspackages/rs-platform-version/src/version/v4.rspackages/rs-platform-version/src/version/v5.rspackages/rs-platform-version/src/version/v6.rspackages/rs-platform-version/src/version/v7.rspackages/rs-platform-version/src/version/v8.rspackages/rs-platform-version/src/version/v9.rspackages/rs-sdk-ffi/src/data_contract/queries/history.rspackages/rs-sdk-ffi/src/evonode/queries/proposed_epoch_blocks_by_ids.rspackages/rs-sdk-ffi/src/evonode/queries/proposed_epoch_blocks_by_range.rspackages/rs-sdk/Cargo.tomlpackages/rs-sdk/src/mock/sdk.rspackages/rs-sdk/src/platform.rspackages/rs-sdk/src/platform/documents/document_average.rspackages/rs-sdk/src/platform/documents/document_count.rspackages/rs-sdk/src/platform/documents/document_query.rspackages/rs-sdk/src/platform/documents/document_split_averages.rspackages/rs-sdk/src/platform/documents/document_split_counts.rspackages/rs-sdk/src/platform/documents/document_split_sums.rspackages/rs-sdk/src/platform/documents/document_sum.rspackages/rs-sdk/src/platform/fetch.rspackages/rs-sdk/src/platform/fetch_many.rspackages/rs-sdk/src/platform/fetch_unproved.rspackages/rs-sdk/src/platform/group_actions.rspackages/rs-sdk/src/platform/identities_contract_keys_query.rspackages/rs-sdk/src/platform/query.rspackages/rs-sdk/src/platform/query_context.rspackages/rs-sdk/src/platform/tokens/identity_token_balances.rspackages/rs-sdk/src/platform/tokens/token_contract_info.rspackages/rs-sdk/src/platform/tokens/token_info.rspackages/rs-sdk/src/platform/tokens/token_pre_programmed_distributions.rspackages/rs-sdk/src/platform/tokens/token_status.rspackages/rs-sdk/src/platform/tokens/token_total_supply.rspackages/rs-sdk/src/platform/types/epoch.rspackages/rs-sdk/src/platform/types/finalized_epoch.rspackages/rs-sdk/src/platform/types/identity.rspackages/rs-sdk/src/sdk.rspackages/rs-sdk/tests/fetch/common.rspackages/rs-sdk/tests/fetch/document.rspackages/rs-sdk/tests/fetch/document_query_v0_v1.rspackages/rs-sdk/tests/fetch/mod.rspackages/rs-sdk/tests/fetch/tokens/token_contract_info.rs
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3711 +/- ##
============================================
- Coverage 87.17% 87.17% -0.01%
============================================
Files 2601 2601
Lines 318220 318220
============================================
- Hits 277421 277409 -12
- Misses 40799 40811 +12
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Wire-compatibility fix is well-engineered: the V0 query-version fork plus QueryContext/Fetch::Query/Fetch::Request split correctly restore pre-v3.1 testnet compatibility. No true blockers — the most serious findings are (a) the new with_initial_version seed is not propagated into the mock SDK's proof-verifier path, so offline mock tests still verify against the wrong version, and (b) default auto-detect SDKs still emit V1 on the first getDocuments because the protocol-version atomic seeds to 0 and version() falls back to latest(); the mitigation is opt-in via with_initial_version, which is also not bridged into rs-sdk-ffi. Several smaller suggestions cover undocumented precedence between with_version and with_initial_version, inconsistent prove=false handling across sibling Query impls, a redundant double-clone on the non-versioned fetch path, and disabled document fetch integration tests.
🟡 9 suggestion(s) | 💬 1 nitpick(s)
10 additional finding(s)
suggestion: with_initial_version is not propagated into the mock proof-verifier path
packages/rs-sdk/src/sdk.rs (line 1114)
In the mock branch of SdkBuilder::build(), MockDashPlatformSdk::new(self.version, ...) is constructed from self.version (the explicit/latest PlatformVersion), ignoring self.initial_version. Later, when a mock request falls back to real proof parsing instead of a pre-recorded from_proof expectation, MockDashPlatformSdk::parse_proof_with_metadata() reads self.version() (mock/sdk.rs:82-84, used at :510), which returns self.platform_version — i.e. latest(), not the requested initial version. The new compatibility story for pre-v3.1 networks therefore still breaks in mocks / offline-testing, which is precisely where these document fixtures are exercised. Seed the inner mock with the same effective initial version as the outer SDK.
let mock_version = if self.version_explicit {
self.version
} else {
self.initial_version.unwrap_or(self.version)
};
let mock_sdk = MockDashPlatformSdk::new(mock_version, Arc::clone(&dapi));
suggestion: Default auto-detect SDKs still emit V1 documents on the first getDocuments against pre-v3.1 networks
packages/rs-sdk/src/sdk.rs (line 489)
Sdk::query_context() reads self.version(), and version() falls back to PlatformVersion::latest() whenever the per-instance atomic is still 0. DocumentQuery's Query<GetDocumentsRequest> impl then dispatches entirely on ctx.protocol_version. Because auto-detect only updates the atomic after a successful response metadata parse, a brand-new SDK that talks to a pre-v3.1 network and makes getDocuments as its first meaningful request will still encode V1, get the same gRPC Unknown decode error described in the PR body, and never ratchet the protocol version downward. SdkBuilder::with_initial_version() is the documented mitigation but it is opt-in; default builders still seed 0. Either (a) make the auto-detect fallback bias to the lowest still-supported PV instead of latest(), (b) issue a cheap version-discovery probe before any document query, or (c) call this limitation out prominently in SdkBuilder rustdoc so callers running against pre-v3.1 networks know they must opt in.
suggestion: with_version silently overrides with_initial_version without a diagnostic
packages/rs-sdk/src/sdk.rs (line 1048)
In build(), when self.version_explicit is true the explicit with_version() seed wins and self.initial_version is silently discarded; the new rustdoc on with_initial_version (sdk.rs:898-919) does not document this precedence. A caller doing .with_version(latest).with_initial_version(pv11) likely meant the v3.0-network behaviour but will instead get auto-detect off and PV pinned to latest — precisely the configuration this PR is fixing. Either reject the combination at build() time with Error::Config, or emit a tracing::warn! describing the precedence so the misconfiguration is loud rather than silent.
suggestion: Inconsistent prove=false handling across sibling Query impls
packages/rs-sdk/src/platform/query.rs (line 357)
Query<DocumentQuery> for DriveDocumentQuery<'_> (query.rs:357-374) hard-rejects !ctx.prove with Error::Config("dash-sdk does not support non-proven queries; proof verification is mandatory on the SDK fetch path"). The very next impl, Query<DocumentQuery> for DocumentQuery (query.rs:381-389), accepts the same condition with only a tracing::warn! and returns Ok(self.clone()). The wire encoders hardcode prove: true regardless, so the warn path is technically correct — but two sibling Query<DocumentQuery> impls living next to each other disagreeing about whether !ctx.prove is a hard error is a footgun: callers reaching the fetch path via DriveDocumentQuery see a typed error, callers via a pre-built DocumentQuery silently proceed. Pick one policy (both warn or both reject) and document it.
suggestion: Two-step encode adds a redundant clone for every non-versioned fetch
packages/rs-sdk/src/platform/fetch.rs (line 173)
Fetch::fetch_with_metadata_and_proof() now does let owned_rich = query.query(&ctx)?; let owned_wire = owned_rich.query(&ctx)?;. For the vast majority of Fetch impls (Identity, DataContract, all token/balance types, …) type Query = Self::Request, so the second call resolves to the blanket impl<T> Query<T> for T and produces Ok(self.clone()). The same pattern exists in FetchMany. Net effect on every normal fetch: one extra full clone of the proto request — gratuitous when no version-aware encoding is needed. Specialize via a sealed helper trait that short-circuits when Self::Query == Self::Request, or otherwise add a fast path so the non-versioned majority does not take the extra copy.
suggestion: Every document fetch integration test on the changed path is disabled
packages/rs-sdk/tests/fetch/document.rs (line 15)
This PR changes the core Document::fetch / fetch_many path from Request = DocumentQuery to a rich-query/wire-request split — the highest-risk part of the refactor. However the four integration tests that actually cover those paths (document_read, document_read_no_document, document_list_drive_query, document_list_document_query) are all now #[ignore] pending vector regeneration. The new unit tests in document_query_v0_v1.rs validate encoder shape only; they do not exercise the full offline/live fetch path that now depends on Fetch::Query vs Fetch::Request, mock-expectation keying, and proof parsing. Regenerate at least one document fetch fixture so the end-to-end round-trip is covered before this lands.
suggestion: FFI does not expose with_initial_version — Swift consumers cannot invoke the documented mitigation
packages/rs-sdk-ffi/src/sdk.rs (line 1)
SdkBuilder::with_initial_version(&PlatformVersion) (rs-sdk/src/sdk.rs:916) is the documented mitigation for booting against a pre-v3.1 network (e.g. v3.0 testnet, PV_11). The encoder dispatches on ctx.protocol_version, which Sdk::query_context() reads from the SDK's per-instance atomic; without with_initial_version, that atomic falls back to PlatformVersion::latest() (PV_12).
rs-sdk-ffi's SDK construction (rs-sdk-ffi/src/sdk.rs:121, 148, 204, 231, 361-411) never calls with_initial_version. A Swift app pointed at v3.0 testnet will therefore: default to PV_12 → V1 wire encoding, issue Document::fetch_many → V1 GetDocumentsRequest, get the gRPC Unknown decode error, and have its node banned by rs-dapi-client before auto-detect can ratchet downward. The Rust-side fix is complete; the FFI surface is not. Add an initial_protocol_version: u32 field to DashSDKConfig (or a new FFI entry point) so iOS/Swift callers can pin the boot version.
suggestion: Query for DocumentQuery silently drops ctx.prove
packages/rs-sdk/src/platform/documents/document_query.rs (line 952)
impl Query<GetDocumentsRequest> for DocumentQuery forwards to GetDocumentsRequest::try_from_platform_versioned(self.clone(), ctx.protocol_version) and discards ctx.prove. Inside encode_v0/encode_v1, prove: true is hardcoded on the wire (document_query.rs:476, :571). Today documents are always proof-verified, so this is intentional — but a future caller using FetchUnproved or passing ctx.without_proofs() will still emit prove: true on the wire silently. Either honor ctx.prove so there is a single source of truth, or short-circuit with Error::Config when !ctx.prove so the mismatch is loud rather than silent.
suggestion: QueryContext.request_settings reflects SDK defaults, not per-call effective settings
packages/rs-sdk/src/platform/query_context.rs (line 28)
Sdk::query_context() borrows &self.dapi_client_settings (sdk.rs:511-517), but Fetch::fetch_with_metadata_and_proof later composes those defaults with the caller's Option<RequestSettings> after the ctx has been handed to Query::query (fetch.rs:217-219). No current encoder reads ctx.request_settings, so today this is harmless — but the rustdoc on query_context.rs:29-31 justifies the field as forward-compatibility for an encoder that consults transport hints. The day such an encoder lands, it will observe the SDK-default settings rather than the effective per-request settings, which is a subtle correctness bug waiting to happen. Either compose the per-call settings before building the QueryContext, or drop the field until the first real consumer materializes (YAGNI is better than a structurally misleading public field).
nitpick: Identity-mapping the V0 Start enum is redundant
packages/rs-sdk/src/platform/documents/document_query.rs (line 567)
In encode_v0 (document_query.rs:494-573), start is already Option<Start> where Start = get_documents_request_v0::Start. The expression start.map(|s| match s { Start::StartAfter(b) => Start::StartAfter(b), Start::StartAt(b) => Start::StartAt(b) }) is a verbatim identity map on the same enum (in contrast to encode_v1 at :456-459, where Start is translated to the distinct V1Start). It can simply be start.
start,
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
| /// Construct via [`crate::Sdk::query_context`] for normal use, or directly in | ||
| /// unit tests that want to exercise the encoder without an `Sdk`. | ||
| #[derive(Debug, Clone, Copy)] | ||
| pub struct QueryContext<'a> { |
There was a problem hiding this comment.
Looks more like settings rather than context
There was a problem hiding this comment.
Let's name it QuerySettings
There was a problem hiding this comment.
Renamed in 407e57cbc0: QueryContext → QuerySettings, file moved to query_settings.rs, Sdk::query_context() → Sdk::query_settings(). ~30 callsite touchups across rs-sdk and rs-sdk-ffi. Thanks for the nudge — the name reads more honestly now.
Three Query::query impls in rs-sdk-ffi were missed during the QueryContext refactor and tripped E0050 after the merge: - data_contract/queries/history.rs - evonode/queries/proposed_epoch_blocks_by_ids.rs - evonode/queries/proposed_epoch_blocks_by_range.rs Swap the old (prove, &Sdk) parameters for &QueryContext<'_> and forward ctx.prove into the wire request. None of these impls needed &Sdk for transport, so the previous _sdk binding goes away cleanly.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Re-validated all carried-forward findings against d7f4869. The cleanest divergence between agents: claude-rust-quality identified a blocking regression that the other two general/codex agents missed — un-ignoring the four document fetch integration tests without regenerating their vectors will hard-fail CI, because the mock loader's load_expectations_sync has no match arm for the legacy msg_DocumentQuery_*.json files now present in tests/vectors/document_*/ and falls through to Error::Config("unknown request type DocumentQuery"), which setup_api().expect("initialize api") will panic on under default features (which include offline-testing). The other nine convergent findings all reproduce against the current head and are carried forward.
🔴 1 blocking | 🟡 8 suggestion(s) | 💬 1 nitpick(s)
10 additional finding(s)
blocking: Un-ignored document fetch tests will fail in CI because vectors were not regenerated
packages/rs-sdk/tests/fetch/document.rs (line 15)
Commit 1ca35c831f removes #[ignore] from document_read, document_read_no_document, document_list_drive_query, and document_list_document_query, but the test vectors in packages/rs-sdk/tests/vectors/document_*/ are still the pre-refactor msg_DocumentQuery_*.json files (verified: each of the four directories contains a msg_DocumentQuery_<hash>.json). The PR's Fetch::Query/Fetch::Request split makes the wire request GetDocumentsRequest; MockDashPlatformSdk::load_expectations_sync (rs-sdk/src/mock/sdk.rs:131-264) dispatches on basename.split('_').nth(1) and has no "DocumentQuery" match arm, so it falls into the catch-all _ arm at :257-263 and returns Error::Config("unknown request type DocumentQuery in …, missing match arm in load_expectations?"). Config::setup_api() (tests/fetch/config.rs:222-227) builds the SDK with .with_dump_dir(&dump_dir).build().expect("initialize api"), so the error becomes a panic before any test body runs. dash-sdk's default features include offline-testing (Cargo.toml:74-81), so the standard CI invocation will hit this path. Either regenerate the four document fetch vectors so the new wire shape (msg_GetDocumentsRequest_*.json) lands with the same commit, or restore #[ignore] with a follow-up tracking ticket — shipping known-failing tests under the rationale of "surface in CI" isn't appropriate for merge.
suggestion: with_initial_version is not propagated into the mock proof-verifier path
packages/rs-sdk/src/sdk.rs (line 1114)
In the mock branch of SdkBuilder::build() (sdk.rs:1114), the inner MockDashPlatformSdk::new(self.version, …) is seeded from self.version and ignores self.initial_version, while the outer Sdk.protocol_version atomic (sdk.rs:1127-1135) does honour initial_version. MockDashPlatformSdk::version() (mock/sdk.rs:82-84) returns the inner platform_version, which is what parse_proof_with_metadata uses when a mock request misses a prerecorded expectation and falls back to real FromProof decoding. In mocks/offline-testing, this means a builder configured for a pre-v3.1 fixture via with_initial_version() still verifies proofs against PlatformVersion::latest(). With the four document fetch tests now un-ignored, this is exactly the offline path that gets exercised. Seed the inner mock with the same effective initial version as the outer SDK.
let mock_version = if self.version_explicit {
self.version
} else {
self.initial_version.unwrap_or(self.version)
};
let mock_sdk = MockDashPlatformSdk::new(mock_version, Arc::clone(&dapi));
suggestion: Default auto-detect SDKs still emit V1 documents on the first getDocuments against pre-v3.1 networks
packages/rs-sdk/src/sdk.rs (line 489)
Sdk::query_context() (sdk.rs:511-517) reads self.version(), and version() falls back to PlatformVersion::latest() whenever the per-instance atomic is still 0 (sdk.rs:489-492). DocumentQuery's Query<GetDocumentsRequest> impl (document_query.rs:952-958) dispatches entirely on ctx.protocol_version. Auto-detect only updates the atomic after a successful response metadata parse, so a brand-new default-builder SDK that talks to a pre-v3.1 network and makes getDocuments as its first meaningful request will still encode V1, receive the gRPC Unknown decode error described in the PR body, and have its node banned by rs-dapi-client before it can ratchet downward. with_initial_version() is opt-in. Either bias the auto-detect fallback toward the lowest still-supported PV, issue a cheap version-discovery probe before any document query, or call this limitation out prominently in SdkBuilder rustdoc.
suggestion: with_version silently overrides with_initial_version without a diagnostic
packages/rs-sdk/src/sdk.rs (line 1048)
In build() (sdk.rs:1051-1058 dapi branch and 1127-1135 mock branch), when self.version_explicit is true the explicit with_version() seed wins and self.initial_version is silently discarded; the rustdoc on with_initial_version does not document this precedence. A caller writing .with_version(latest).with_initial_version(pv11) likely intends the v3.0-network behaviour but instead gets auto-detect off and PV pinned to latest — precisely the configuration this PR is fixing. Either reject the combination at build() time with Error::Config, or emit a tracing::warn! describing the precedence.
suggestion: Inconsistent prove=false handling across sibling Query impls
packages/rs-sdk/src/platform/query.rs (line 357)
Query<DocumentQuery> for DriveDocumentQuery<'_> (query.rs:357-374) hard-rejects !ctx.prove with Error::Config. The very next impl, Query<DocumentQuery> for DocumentQuery (query.rs:381-389), accepts the same condition with only a tracing::warn! and returns Ok(self.clone()). The wire encoders hardcode prove: true regardless, so the warn path is technically correct — but two sibling impls disagreeing about whether !ctx.prove is a hard error is a footgun: callers reaching the fetch path via DriveDocumentQuery see a typed error, callers via a pre-built DocumentQuery silently proceed. Pick one policy (both warn or both reject) and document it.
suggestion: Two-step encode adds a redundant clone for every non-versioned fetch
packages/rs-sdk/src/platform/fetch.rs (line 173)
Fetch::fetch_with_metadata_and_proof() (fetch.rs:173-175) does let owned_rich = query.query(&ctx)?; let owned_wire = owned_rich.query(&ctx)?;. For the majority of Fetch impls (Identity, DataContract, all token/balance types, …) type Query = Self::Request, so the second call resolves to the blanket impl<T> Query<T> for T and produces Ok(self.clone()). The same pattern exists in FetchMany. Net effect on every normal fetch: one extra full clone of the proto request — gratuitous when no version-aware encoding is needed. Specialize via a sealed helper trait that short-circuits when Self::Query == Self::Request, or otherwise add a fast path so the non-versioned majority does not take the extra copy.
suggestion: FFI does not expose with_initial_version — Swift consumers cannot invoke the documented mitigation
packages/rs-sdk-ffi/src/sdk.rs (line 1)
Verified at d7f4869: grep -r with_initial_version|initial_protocol_version packages/rs-sdk-ffi/ returns zero hits. SdkBuilder::with_initial_version(&PlatformVersion) (rs-sdk/src/sdk.rs:916) is the PR's documented mitigation for booting against a pre-v3.1 network (e.g. v3.0 testnet, PV_11). Without it, the FFI's per-instance atomic seeds to 0 → Sdk::version() falls back to PlatformVersion::latest() (PV_12 today) → first Document::fetch_many encodes V1 GetDocumentsRequest on the wire → Drive on PV_11 returns gRPC Unknown { message: "could not decode data contracts query" } → rs-dapi-client bans the node before auto-detect can ratchet downward. The Rust-side fix is complete; the FFI surface — the primary downstream consumer (iOS SDK / SwiftExampleApp) — is not. Add an initial_protocol_version: u32 field to DashSDKConfig/DashSDKConfigTrusted, then in the FFI build path do if cfg.initial_protocol_version != 0 { builder = builder.with_initial_version(PlatformVersion::get(cfg.initial_protocol_version)?); }, or add a new entry point like dash_sdk_set_initial_protocol_version invoked before any fetch.
suggestion: Query for DocumentQuery silently drops ctx.prove
packages/rs-sdk/src/platform/documents/document_query.rs (line 952)
The impl forwards to GetDocumentsRequest::try_from_platform_versioned(self.clone(), ctx.protocol_version) and discards ctx.prove. Inside encode_v0/encode_v1, prove: true is hardcoded on the wire (document_query.rs:571 in encode_v0, and analogously in encode_v1). Today documents are always proof-verified, so this is intentional — but a future caller using FetchUnproved or passing ctx.without_proofs() will still emit prove: true on the wire silently. The three FFI impls touched in the latest delta now read ctx.prove directly; this impl is the remaining outlier. Either honour ctx.prove (single source of truth), or short-circuit with Error::Config when !ctx.prove so the mismatch is loud rather than silent.
suggestion: QueryContext.request_settings reflects SDK defaults, not per-call effective settings
packages/rs-sdk/src/platform/query_context.rs (line 28)
Sdk::query_context() (sdk.rs:511-517) borrows &self.dapi_client_settings, but Fetch::fetch_with_metadata_and_proof (fetch.rs:217-219) composes those defaults with the caller's Option<RequestSettings> after the ctx has been handed to Query::query. No current encoder reads ctx.request_settings, so today this is harmless — but the rustdoc on query_context.rs:29-31 justifies the field as forward-compatibility for an encoder that consults transport hints. The day such an encoder lands, it will observe the SDK-default settings rather than the effective per-request settings, which is a subtle correctness bug waiting to happen. Either compose the per-call settings before building the QueryContext, or drop the field until the first real consumer materializes.
nitpick: Identity-mapping the V0 Start enum is redundant
packages/rs-sdk/src/platform/documents/document_query.rs (line 567)
In encode_v0, start is already Option<Start> where Start = get_documents_request_v0::Start. The expression start.map(|s| match s { Start::StartAfter(b) => Start::StartAfter(b), Start::StartAt(b) => Start::StartAt(b) }) is a verbatim identity map on the same enum (in contrast to encode_v1, where Start is translated to the distinct V1Start). It can simply be start,.
start,
🤖 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.
- [BLOCKING] In `packages/rs-sdk/tests/fetch/document.rs`:15-15: Un-ignored document fetch tests will fail in CI because vectors were not regenerated
Commit `1ca35c831f` removes `#[ignore]` from `document_read`, `document_read_no_document`, `document_list_drive_query`, and `document_list_document_query`, but the test vectors in `packages/rs-sdk/tests/vectors/document_*/` are still the pre-refactor `msg_DocumentQuery_*.json` files (verified: each of the four directories contains a `msg_DocumentQuery_<hash>.json`). The PR's `Fetch::Query`/`Fetch::Request` split makes the wire request `GetDocumentsRequest`; `MockDashPlatformSdk::load_expectations_sync` (rs-sdk/src/mock/sdk.rs:131-264) dispatches on `basename.split('_').nth(1)` and has no `"DocumentQuery"` match arm, so it falls into the catch-all `_` arm at :257-263 and returns `Error::Config("unknown request type DocumentQuery in …, missing match arm in load_expectations?")`. `Config::setup_api()` (tests/fetch/config.rs:222-227) builds the SDK with `.with_dump_dir(&dump_dir).build().expect("initialize api")`, so the error becomes a panic before any test body runs. `dash-sdk`'s default features include `offline-testing` (Cargo.toml:74-81), so the standard CI invocation will hit this path. Either regenerate the four document fetch vectors so the new wire shape (`msg_GetDocumentsRequest_*.json`) lands with the same commit, or restore `#[ignore]` with a follow-up tracking ticket — shipping known-failing tests under the rationale of "surface in CI" isn't appropriate for merge.
- [SUGGESTION] In `packages/rs-sdk/src/sdk.rs`:1114-1114: with_initial_version is not propagated into the mock proof-verifier path
In the mock branch of `SdkBuilder::build()` (sdk.rs:1114), the inner `MockDashPlatformSdk::new(self.version, …)` is seeded from `self.version` and ignores `self.initial_version`, while the outer `Sdk.protocol_version` atomic (sdk.rs:1127-1135) does honour `initial_version`. `MockDashPlatformSdk::version()` (mock/sdk.rs:82-84) returns the inner `platform_version`, which is what `parse_proof_with_metadata` uses when a mock request misses a prerecorded expectation and falls back to real `FromProof` decoding. In `mocks`/`offline-testing`, this means a builder configured for a pre-v3.1 fixture via `with_initial_version()` still verifies proofs against `PlatformVersion::latest()`. With the four document fetch tests now un-ignored, this is exactly the offline path that gets exercised. Seed the inner mock with the same effective initial version as the outer SDK.
- [SUGGESTION] In `packages/rs-sdk/src/sdk.rs`:489-517: Default auto-detect SDKs still emit V1 documents on the first getDocuments against pre-v3.1 networks
`Sdk::query_context()` (sdk.rs:511-517) reads `self.version()`, and `version()` falls back to `PlatformVersion::latest()` whenever the per-instance atomic is still `0` (sdk.rs:489-492). `DocumentQuery`'s `Query<GetDocumentsRequest>` impl (document_query.rs:952-958) dispatches entirely on `ctx.protocol_version`. Auto-detect only updates the atomic after a successful response metadata parse, so a brand-new default-builder SDK that talks to a pre-v3.1 network and makes `getDocuments` as its first meaningful request will still encode V1, receive the `gRPC Unknown` decode error described in the PR body, and have its node banned by rs-dapi-client before it can ratchet downward. `with_initial_version()` is opt-in. Either bias the auto-detect fallback toward the lowest still-supported PV, issue a cheap version-discovery probe before any document query, or call this limitation out prominently in `SdkBuilder` rustdoc.
- [SUGGESTION] In `packages/rs-sdk/src/sdk.rs`:1048-1060: with_version silently overrides with_initial_version without a diagnostic
In `build()` (sdk.rs:1051-1058 dapi branch and 1127-1135 mock branch), when `self.version_explicit` is true the explicit `with_version()` seed wins and `self.initial_version` is silently discarded; the rustdoc on `with_initial_version` does not document this precedence. A caller writing `.with_version(latest).with_initial_version(pv11)` likely intends the v3.0-network behaviour but instead gets auto-detect off and PV pinned to latest — precisely the configuration this PR is fixing. Either reject the combination at `build()` time with `Error::Config`, or emit a `tracing::warn!` describing the precedence.
- [SUGGESTION] In `packages/rs-sdk/src/platform/query.rs`:357-389: Inconsistent prove=false handling across sibling Query<DocumentQuery> impls
`Query<DocumentQuery> for DriveDocumentQuery<'_>` (query.rs:357-374) hard-rejects `!ctx.prove` with `Error::Config`. The very next impl, `Query<DocumentQuery> for DocumentQuery` (query.rs:381-389), accepts the same condition with only a `tracing::warn!` and returns `Ok(self.clone())`. The wire encoders hardcode `prove: true` regardless, so the warn path is technically correct — but two sibling impls disagreeing about whether `!ctx.prove` is a hard error is a footgun: callers reaching the fetch path via `DriveDocumentQuery` see a typed error, callers via a pre-built `DocumentQuery` silently proceed. Pick one policy (both warn or both reject) and document it.
- [SUGGESTION] In `packages/rs-sdk/src/platform/fetch.rs`:173-177: Two-step encode adds a redundant clone for every non-versioned fetch
`Fetch::fetch_with_metadata_and_proof()` (fetch.rs:173-175) does `let owned_rich = query.query(&ctx)?; let owned_wire = owned_rich.query(&ctx)?;`. For the majority of `Fetch` impls (`Identity`, `DataContract`, all token/balance types, …) `type Query = Self::Request`, so the second call resolves to the blanket `impl<T> Query<T> for T` and produces `Ok(self.clone())`. The same pattern exists in `FetchMany`. Net effect on every normal fetch: one extra full clone of the proto request — gratuitous when no version-aware encoding is needed. Specialize via a sealed helper trait that short-circuits when `Self::Query == Self::Request`, or otherwise add a fast path so the non-versioned majority does not take the extra copy.
- [SUGGESTION] In `packages/rs-sdk-ffi/src/sdk.rs`:1-1: FFI does not expose with_initial_version — Swift consumers cannot invoke the documented mitigation
Verified at d7f486922e: `grep -r with_initial_version|initial_protocol_version packages/rs-sdk-ffi/` returns zero hits. `SdkBuilder::with_initial_version(&PlatformVersion)` (rs-sdk/src/sdk.rs:916) is the PR's documented mitigation for booting against a pre-v3.1 network (e.g. v3.0 testnet, PV_11). Without it, the FFI's per-instance atomic seeds to `0` → `Sdk::version()` falls back to `PlatformVersion::latest()` (PV_12 today) → first `Document::fetch_many` encodes V1 `GetDocumentsRequest` on the wire → Drive on PV_11 returns `gRPC Unknown { message: "could not decode data contracts query" }` → rs-dapi-client bans the node before auto-detect can ratchet downward. The Rust-side fix is complete; the FFI surface — the primary downstream consumer (iOS SDK / SwiftExampleApp) — is not. Add an `initial_protocol_version: u32` field to `DashSDKConfig`/`DashSDKConfigTrusted`, then in the FFI build path do `if cfg.initial_protocol_version != 0 { builder = builder.with_initial_version(PlatformVersion::get(cfg.initial_protocol_version)?); }`, or add a new entry point like `dash_sdk_set_initial_protocol_version` invoked before any fetch.
- [SUGGESTION] In `packages/rs-sdk/src/platform/documents/document_query.rs`:952-958: Query<GetDocumentsRequest> for DocumentQuery silently drops ctx.prove
The impl forwards to `GetDocumentsRequest::try_from_platform_versioned(self.clone(), ctx.protocol_version)` and discards `ctx.prove`. Inside `encode_v0`/`encode_v1`, `prove: true` is hardcoded on the wire (document_query.rs:571 in encode_v0, and analogously in encode_v1). Today documents are always proof-verified, so this is intentional — but a future caller using `FetchUnproved` or passing `ctx.without_proofs()` will still emit `prove: true` on the wire silently. The three FFI impls touched in the latest delta now read `ctx.prove` directly; this impl is the remaining outlier. Either honour `ctx.prove` (single source of truth), or short-circuit with `Error::Config` when `!ctx.prove` so the mismatch is loud rather than silent.
- [SUGGESTION] In `packages/rs-sdk/src/platform/query_context.rs`:28-38: QueryContext.request_settings reflects SDK defaults, not per-call effective settings
`Sdk::query_context()` (sdk.rs:511-517) borrows `&self.dapi_client_settings`, but `Fetch::fetch_with_metadata_and_proof` (fetch.rs:217-219) composes those defaults with the caller's `Option<RequestSettings>` *after* the `ctx` has been handed to `Query::query`. No current encoder reads `ctx.request_settings`, so today this is harmless — but the rustdoc on query_context.rs:29-31 justifies the field as forward-compatibility for an encoder that consults transport hints. The day such an encoder lands, it will observe the SDK-default settings rather than the effective per-request settings, which is a subtle correctness bug waiting to happen. Either compose the per-call settings *before* building the `QueryContext`, or drop the field until the first real consumer materializes.
- [NITPICK] In `packages/rs-sdk/src/platform/documents/document_query.rs`:567-570: Identity-mapping the V0 Start enum is redundant
In `encode_v0`, `start` is already `Option<Start>` where `Start = get_documents_request_v0::Start`. The expression `start.map(|s| match s { Start::StartAfter(b) => Start::StartAfter(b), Start::StartAt(b) => Start::StartAt(b) })` is a verbatim identity map on the same enum (in contrast to `encode_v1`, where `Start` is translated to the distinct `V1Start`). It can simply be `start,`.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
…03/004, PROJ-002/003) - SEC-001: document fail-fast intent in mock expect helpers (INTENTIONAL comments) - RUST-001: drop no-op Start closure in V0 documents wire encoder - RUST-003: standardize unproved-query rejection to unimplemented!() across all Query impls - RUST-004: drop duplicate ciborium dev-dep from rs-sdk Cargo.toml - PROJ-002: add per-PV pinning caveat to Sdk::with_initial_version docstring - PROJ-003: document that associated-type defaults are nightly-only (RFC 2532)
…y heterogeneity The blanket `warn + Ok(self.clone())` on `impl<T> Query<T> for T` is the documented contract for `fetch_unproved` targets (contested resources, EvoNodeStatus, CurrentQuorumsInfo, etc.); collapsing it to `unimplemented!()` made `fetch_unproved` panic at runtime instead of flowing through to the wire request. `DriveDocumentQuery`'s typed `Err(Error::Config(..))` rejection is the intentional "proven path is mandatory" boundary on the SDK fetch path and is meaningfully different from the blanket impl — callers handle the error rather than crashing. Per-site policy here is intentional, not inconsistency to clean up. This commit restores: - blanket `impl<T> Query<T> for T`: `tracing::warn!(..)` + `Ok(self.clone())` - `Query<DocumentQuery> for DriveDocumentQuery`: typed `Err(Error::Config(..))` with the rationale comment - `Query<DocumentQuery> for DocumentQuery`: `tracing::warn!(..)` + `Ok(self.clone())` Other PR #3711 triage fixes (SEC-001 comments, RUST-001 no-op Start closure drop, RUST-004 ciborium dedupe, PROJ-002 docstring caveat, PROJ-003 nightly-feature note) are preserved.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative verification at c723cfe. The two latest commits are doc/comments-only on the review-relevant surface (one prior nitpick fixed: V0 Start identity-map removed; one prior finding intentionally deferred per the explicit revert commit title). 8 carry-forward findings remain valid — including one blocking: the four un-ignored document fetch tests will deterministically panic in offline CI because their fixture directories still contain pre-refactor msg_DocumentQuery_*.json files while the mock loader only knows GetDocumentsRequest. Two independent reviewers converged on every finding.
🔴 1 blocking | 🟡 7 suggestion(s)
8 additional finding(s)
blocking: Re-enabled document fetch tests will panic in offline CI — fixtures were not regenerated
packages/rs-sdk/tests/fetch/document.rs (line 15)
Verified end-to-end at c723cfe:
- tests/fetch/document.rs:15,82,108,153 — the four tests (document_read, document_read_no_document, document_list_drive_query, document_list_document_query) have no #[ignore] attribute.
- tests/vectors/document_read/, document_read_no_document/, document_list_document_query/, document_list_drive_query/ — each still contains pre-refactor msg_DocumentQuery_.json files (confirmed via directory listing).
- src/mock/sdk.rs:131-264 — load_expectations_sync dispatches on basename.split('_').nth(1). Match arms at :136-256 cover GetDocumentsRequest but there is no "DocumentQuery" arm; the catch-all _ at :257-263 returns Error::Config("unknown request type {} in {}, missing match arm in load_expectations?").
- src/sdk.rs:1150-1152 — the mock branch of SdkBuilder::build() calls guard.load_expectations_sync(dump_dir)? before returning, so the error surfaces during setup_api(...).expect("initialize api").
- dash-sdk's default features include offline-testing, so the standard cargo test -p dash-sdk path enters this branch.
Net effect: every one of the four un-ignored tests will panic at SDK initialization before any assertion runs, with unknown request type DocumentQuery in .../msg_DocumentQuery_<hash>.json. The PR rationale ("failures show up in CI rather than silently passing") is not an acceptable merge state. Either regenerate the four vectors against testnet so they land as msg_GetDocumentsRequest_*.json in the same commit, or restore #[ignore] with a tracking issue.
suggestion: with_initial_version() is not propagated into the mock proof-verifier
packages/rs-sdk/src/sdk.rs (line 1119)
In the mock branch of SdkBuilder::build() at sdk.rs:1119, MockDashPlatformSdk::new(self.version, Arc::clone(&dapi)) is still seeded from self.version, while the outer Sdk.protocol_version atomic at sdk.rs:1132-1140 honours self.initial_version when version_explicit is false. MockDashPlatformSdk::version() returns the inner pinned PlatformVersion, used by parse_proof_with_metadata when a mock request misses a prerecorded expectation and falls back to real FromProof decoding. A builder configured for a pre-v3.1 fixture via with_initial_version() therefore verifies proofs against PlatformVersion::latest(), not the requested initial version — split-brain state between encoder PV and verifier PV inside the same SDK. With the four document fetch tests now un-ignored, this is exactly the offline path that gets exercised.
let mock_version = if self.version_explicit {
self.version
} else {
self.initial_version.unwrap_or(self.version)
};
let mock_sdk = MockDashPlatformSdk::new(mock_version, Arc::clone(&dapi));
suggestion: Default auto-detect SDKs still emit latest-version documents on first getDocuments against pre-v3.1 networks
packages/rs-sdk/src/sdk.rs (line 489)
Sdk::version() at sdk.rs:489-492 falls back to PlatformVersion::latest() whenever the per-instance atomic is 0, and Sdk::query_context() at sdk.rs:511-517 hands that PV straight to the DocumentQuery → GetDocumentsRequest encoder (document_query.rs:949-956), which dispatches entirely on ctx.protocol_version. Auto-detect only updates the atomic after a successful response-metadata parse, so a default-built SDK that talks to a pre-v3.1 network and makes getDocuments as its first meaningful request still encodes V1, hits the gRPC Unknown decode error described in the PR body, and gets its node banned by rs-dapi-client before auto-detect can ratchet downward. The new Caveat block on with_initial_version's docstring (sdk.rs:917-920) documents the encoder-pinning requirement but does not change the default behaviour. Either bias the auto-detect fallback toward the lowest still-supported PV, issue a cheap version-discovery probe before the first document query, or call this limitation out prominently at the SdkBuilder doc level — not only inside the opt-in helper.
suggestion: with_version() silently overrides with_initial_version() with no diagnostic
packages/rs-sdk/src/sdk.rs (line 1130)
In both the dapi branch and the mock branch of build() (sdk.rs:1132-1140), when version_explicit is true the with_version() seed wins and self.initial_version is silently discarded. The new Caveat paragraph on with_initial_version's rustdoc talks about encoder pinning but does not document this precedence. A caller writing .with_version(latest).with_initial_version(pv11) likely intends pre-v3.1-network behaviour but instead gets auto-detect disabled and PV pinned to latest — precisely the configuration this PR is fixing. The compiler cannot express this conflict in the type system, so either reject the combination at build() time with Error::Config, or emit a tracing::warn! describing the precedence.
suggestion: FFI does not expose with_initial_version — Swift/iOS consumers cannot invoke the PR's documented mitigation
packages/rs-sdk-ffi/src/types.rs (line 60)
Re-verified: grep -r 'with_initial_version|initial_protocol_version|initial_version' over packages/rs-sdk-ffi/ returns zero matches at c723cfe. DashSDKConfig/DashSDKConfigTrusted in types.rs:60-74 has no initial_protocol_version field, and dash_sdk_create / dash_sdk_create_extended / dash_sdk_create_trusted all build SdkBuilder without any with_initial_version() hook (sdk.rs:97-153, :177-253, :288-420). SdkBuilder::with_initial_version(&PlatformVersion) is the PR's documented mitigation for booting against a pre-v3.1 network (v3.0 testnet, PV_11), yet it is unreachable from the FFI surface — which is the primary downstream consumer (iOS SDK / SwiftExampleApp) that motivates this monorepo's FFI crate. Cross-boundary consequence: a Swift app on a PV_11 network seeds the atomic at 0 → Sdk::version() returns latest() (PV_12) → first Document::fetch_many encodes V1 → Drive returns gRPC Unknown → rs-dapi-client bans the node before auto-detect can ratchet. Minimal fix: add initial_protocol_version: u32 to DashSDKConfig/DashSDKConfigTrusted (0 as a clean sentinel — no valid PV is 0; ABI stays repr(C)-clean), then in the FFI build path apply with_initial_version(PlatformVersion::get(cfg.initial_protocol_version)?) when non-zero. Alternative: a dash_sdk_set_initial_protocol_version entry point invoked before any fetch.
suggestion: Two-step encode adds a redundant clone on every non-versioned fetch
packages/rs-sdk/src/platform/fetch.rs (line 177)
fetch.rs:178-179 does let owned_rich: Self::Query = query.query(&ctx)?; let owned_wire: Self::Request = owned_rich.query(&ctx)?;. For the majority of Fetch impls (Identity, DataContract, all token/balance types, …) type Query = Self::Request, so the second call resolves to the blanket impl Query for T and returns Ok(self.clone()). The same pattern exists in FetchMany. The latest delta added a comment justifying the trait shape (associated-type defaults are nightly-only per RFC 2532), but did not address the runtime cost: every ordinary fetch now pays an extra full clone of the proto request even though only document queries need a second version-aware encoding pass. Specialize via a sealed helper trait that short-circuits when Self::Query == Self::Request, or add a stable fast path so the non-versioned majority avoids the extra copy.
suggestion: Query for DocumentQuery silently drops ctx.prove
packages/rs-sdk/src/platform/documents/document_query.rs (line 949)
document_query.rs:949-956 forwards only ctx.protocol_version to GetDocumentsRequest::try_from_platform_versioned(self.clone(), ctx.protocol_version) and discards ctx.prove. Inside encode_v0/encode_v1, prove: true is hardcoded on the wire. Today documents are always proof-verified, so this is intentional — but a future caller using FetchUnproved or passing ctx.without_proofs() will silently emit prove: true on the wire. The c723cfe revert explicitly documents per-site policy as intentional for upstream Query impls, but this downstream wire encoder is the remaining outlier that ignores one field of a context object nearby impls do honour — a maintainability trap. Either honour ctx.prove for a single source of truth, or short-circuit with Error::Config when !ctx.prove so the mismatch is loud.
suggestion: QueryContext.request_settings reflects SDK defaults, not per-call effective settings
packages/rs-sdk/src/platform/query_context.rs (line 28)
Sdk::query_context() at sdk.rs:511-517 borrows &self.dapi_client_settings, but Fetch::fetch_with_metadata_and_proof composes those defaults with the caller's Option after the ctx has been handed to Query::query. No current encoder reads ctx.request_settings, so this is harmless today — but the rustdoc on query_context.rs:29-31 justifies the field as forward-compatibility for an encoder that consults transport hints. The day such an encoder lands, it will observe SDK-default settings rather than effective per-request settings — a subtle correctness bug waiting to happen, and a behavioral change rather than internal refactor once downstream code starts depending on the field. Either compose per-call settings before building the QueryContext, or drop the field until the first real consumer materializes (YAGNI > structurally misleading public field).
🤖 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.
- [BLOCKING] In `packages/rs-sdk/tests/fetch/document.rs`:15-15: Re-enabled document fetch tests will panic in offline CI — fixtures were not regenerated
Verified end-to-end at c723cfe092:
1. tests/fetch/document.rs:15,82,108,153 — the four tests (document_read, document_read_no_document, document_list_drive_query, document_list_document_query) have no #[ignore] attribute.
2. tests/vectors/document_read/, document_read_no_document/, document_list_document_query/, document_list_drive_query/ — each still contains pre-refactor msg_DocumentQuery_<hash>.json files (confirmed via directory listing).
3. src/mock/sdk.rs:131-264 — load_expectations_sync dispatches on basename.split('_').nth(1). Match arms at :136-256 cover GetDocumentsRequest but there is no "DocumentQuery" arm; the catch-all _ at :257-263 returns Error::Config("unknown request type {} in {}, missing match arm in load_expectations?").
4. src/sdk.rs:1150-1152 — the mock branch of SdkBuilder::build() calls guard.load_expectations_sync(dump_dir)? before returning, so the error surfaces during setup_api(...).expect("initialize api").
5. dash-sdk's default features include offline-testing, so the standard cargo test -p dash-sdk path enters this branch.
Net effect: every one of the four un-ignored tests will panic at SDK initialization before any assertion runs, with `unknown request type DocumentQuery in .../msg_DocumentQuery_<hash>.json`. The PR rationale ("failures show up in CI rather than silently passing") is not an acceptable merge state. Either regenerate the four vectors against testnet so they land as msg_GetDocumentsRequest_*.json in the same commit, or restore #[ignore] with a tracking issue.
- [SUGGESTION] In `packages/rs-sdk/src/sdk.rs`:1119-1119: with_initial_version() is not propagated into the mock proof-verifier
In the mock branch of SdkBuilder::build() at sdk.rs:1119, MockDashPlatformSdk::new(self.version, Arc::clone(&dapi)) is still seeded from self.version, while the outer Sdk.protocol_version atomic at sdk.rs:1132-1140 honours self.initial_version when version_explicit is false. MockDashPlatformSdk::version() returns the inner pinned PlatformVersion, used by parse_proof_with_metadata when a mock request misses a prerecorded expectation and falls back to real FromProof decoding. A builder configured for a pre-v3.1 fixture via with_initial_version() therefore verifies proofs against PlatformVersion::latest(), not the requested initial version — split-brain state between encoder PV and verifier PV inside the same SDK. With the four document fetch tests now un-ignored, this is exactly the offline path that gets exercised.
- [SUGGESTION] In `packages/rs-sdk/src/sdk.rs`:489-517: Default auto-detect SDKs still emit latest-version documents on first getDocuments against pre-v3.1 networks
Sdk::version() at sdk.rs:489-492 falls back to PlatformVersion::latest() whenever the per-instance atomic is 0, and Sdk::query_context() at sdk.rs:511-517 hands that PV straight to the DocumentQuery → GetDocumentsRequest encoder (document_query.rs:949-956), which dispatches entirely on ctx.protocol_version. Auto-detect only updates the atomic after a successful response-metadata parse, so a default-built SDK that talks to a pre-v3.1 network and makes getDocuments as its first meaningful request still encodes V1, hits the gRPC Unknown decode error described in the PR body, and gets its node banned by rs-dapi-client before auto-detect can ratchet downward. The new Caveat block on with_initial_version's docstring (sdk.rs:917-920) documents the encoder-pinning requirement but does not change the default behaviour. Either bias the auto-detect fallback toward the lowest still-supported PV, issue a cheap version-discovery probe before the first document query, or call this limitation out prominently at the SdkBuilder doc level — not only inside the opt-in helper.
- [SUGGESTION] In `packages/rs-sdk/src/sdk.rs`:1130-1140: with_version() silently overrides with_initial_version() with no diagnostic
In both the dapi branch and the mock branch of build() (sdk.rs:1132-1140), when version_explicit is true the with_version() seed wins and self.initial_version is silently discarded. The new Caveat paragraph on with_initial_version's rustdoc talks about encoder pinning but does not document this precedence. A caller writing .with_version(latest).with_initial_version(pv11) likely intends pre-v3.1-network behaviour but instead gets auto-detect disabled and PV pinned to latest — precisely the configuration this PR is fixing. The compiler cannot express this conflict in the type system, so either reject the combination at build() time with Error::Config, or emit a tracing::warn! describing the precedence.
- [SUGGESTION] In `packages/rs-sdk-ffi/src/types.rs`:60-74: FFI does not expose with_initial_version — Swift/iOS consumers cannot invoke the PR's documented mitigation
Re-verified: grep -r 'with_initial_version|initial_protocol_version|initial_version' over packages/rs-sdk-ffi/ returns zero matches at c723cfe092. DashSDKConfig/DashSDKConfigTrusted in types.rs:60-74 has no initial_protocol_version field, and dash_sdk_create / dash_sdk_create_extended / dash_sdk_create_trusted all build SdkBuilder without any with_initial_version() hook (sdk.rs:97-153, :177-253, :288-420). SdkBuilder::with_initial_version(&PlatformVersion) is the PR's documented mitigation for booting against a pre-v3.1 network (v3.0 testnet, PV_11), yet it is unreachable from the FFI surface — which is the primary downstream consumer (iOS SDK / SwiftExampleApp) that motivates this monorepo's FFI crate. Cross-boundary consequence: a Swift app on a PV_11 network seeds the atomic at 0 → Sdk::version() returns latest() (PV_12) → first Document::fetch_many encodes V1 → Drive returns gRPC Unknown → rs-dapi-client bans the node before auto-detect can ratchet. Minimal fix: add initial_protocol_version: u32 to DashSDKConfig/DashSDKConfigTrusted (0 as a clean sentinel — no valid PV is 0; ABI stays repr(C)-clean), then in the FFI build path apply with_initial_version(PlatformVersion::get(cfg.initial_protocol_version)?) when non-zero. Alternative: a dash_sdk_set_initial_protocol_version entry point invoked before any fetch.
- [SUGGESTION] In `packages/rs-sdk/src/platform/fetch.rs`:177-179: Two-step encode adds a redundant clone on every non-versioned fetch
fetch.rs:178-179 does `let owned_rich: Self::Query = query.query(&ctx)?; let owned_wire: Self::Request = owned_rich.query(&ctx)?;`. For the majority of Fetch impls (Identity, DataContract, all token/balance types, …) type Query = Self::Request, so the second call resolves to the blanket impl<T> Query<T> for T and returns Ok(self.clone()). The same pattern exists in FetchMany. The latest delta added a comment justifying the trait shape (associated-type defaults are nightly-only per RFC 2532), but did not address the runtime cost: every ordinary fetch now pays an extra full clone of the proto request even though only document queries need a second version-aware encoding pass. Specialize via a sealed helper trait that short-circuits when Self::Query == Self::Request, or add a stable fast path so the non-versioned majority avoids the extra copy.
- [SUGGESTION] In `packages/rs-sdk/src/platform/documents/document_query.rs`:949-956: Query<GetDocumentsRequest> for DocumentQuery silently drops ctx.prove
document_query.rs:949-956 forwards only ctx.protocol_version to GetDocumentsRequest::try_from_platform_versioned(self.clone(), ctx.protocol_version) and discards ctx.prove. Inside encode_v0/encode_v1, prove: true is hardcoded on the wire. Today documents are always proof-verified, so this is intentional — but a future caller using FetchUnproved or passing ctx.without_proofs() will silently emit prove: true on the wire. The c723cfe092 revert explicitly documents per-site policy as intentional for upstream Query<DocumentQuery> impls, but this downstream wire encoder is the remaining outlier that ignores one field of a context object nearby impls do honour — a maintainability trap. Either honour ctx.prove for a single source of truth, or short-circuit with Error::Config when !ctx.prove so the mismatch is loud.
- [SUGGESTION] In `packages/rs-sdk/src/platform/query_context.rs`:28-38: QueryContext.request_settings reflects SDK defaults, not per-call effective settings
Sdk::query_context() at sdk.rs:511-517 borrows &self.dapi_client_settings, but Fetch::fetch_with_metadata_and_proof composes those defaults with the caller's Option<RequestSettings> after the ctx has been handed to Query::query. No current encoder reads ctx.request_settings, so this is harmless today — but the rustdoc on query_context.rs:29-31 justifies the field as forward-compatibility for an encoder that consults transport hints. The day such an encoder lands, it will observe SDK-default settings rather than effective per-request settings — a subtle correctness bug waiting to happen, and a behavioral change rather than internal refactor once downstream code starts depending on the field. Either compose per-call settings before building the QueryContext, or drop the field until the first real consumer materializes (YAGNI > structurally misleading public field).
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
…ORM_* vars Five DPNS network tests (one in queries.rs, four in contested_queries.rs) hardcoded https://52.12.176.90:1443 (a testnet HPMN now half-up — TCP listens, TLS hangs), which silently bypassed packages/rs-sdk/tests/.env and made vector regeneration against a local devnet or SSH tunnel impossible. Introduce a shared #[cfg(test)] helper test_address::test_address_list() that loads tests/.env (best-effort) and builds an AddressList from DASH_SDK_PLATFORM_HOST / DASH_SDK_PLATFORM_PORT / DASH_SDK_PLATFORM_SSL — matching the existing Config schema used by tests/fetch/config.rs. The five hardcoded blocks now call this helper. Network/with_network(Testnet) is left untouched.
…and-aid endpoint helper
Five #[cfg(test)] blocks in src/platform/dpns_usernames/{queries,contested_queries}.rs
were really network-integration tests — they only used pub methods on Sdk (search,
availability, resolve, contested-vote-state, etc.) and shipped their own band-aid
test_address.rs helper to read DASH_SDK_PLATFORM_* env vars. They belong in tests/,
where the existing fetch::Config harness already loads tests/.env via dotenvy/envy
and exposes address_list() against the very same env vars.
This commit:
- Relocates all five #[ignore] network tests into a new tests/dpns_usernames.rs
integration binary that re-mounts fetch::config + fetch::generated_data
(#[path] sub-modules) so it can call Config::new().address_list() instead of
parsing a hardcoded URL.
- Folds the previously hardcoded tests/dpns_queries_test.rs (which also pinned
https://52.12.176.90:1443) into the same file via the shared
build_testnet_sdk() helper, deleting the old file.
- Deletes src/platform/dpns_usernames/test_address.rs (the band-aid) and the
five #[cfg(test)] mod tests blocks. No public-API change — the production
helpers all stay where they were.
Net: -test_address.rs, -2 inline mod tests, -dpns_queries_test.rs,
+1 unified tests/dpns_usernames.rs, +consistent .env-driven endpoint config.
Run with:
cargo test -p dash-sdk --features generate-test-vectors -- --include-ignored
… local Core RPC quorum lookup build_testnet_sdk used TrustedHttpContextProvider::new(Network::Testnet, ...), which fetches quorum info from a hardcoded testnet HTTP endpoint. Three of the seven relocated DPNS tests (test_dpns_queries, test_dpns_queries_from_docs, test_dpns_search_variations) panicked with `Failed to find quorum: Quorum not found for type 106 and hash ...` when run against a local devnet or SSH tunnel — the testnet quorums simply don't match the local chain. Every other network test in `tests/fetch/*` solves this by going through `Config::setup_api(namespace)`, which wires the SDK with `SdkBuilder::with_core(host, port, user, password)`. Quorums are then resolved via the local Dash Core RPC the user is already running. This commit: - Replaces build_testnet_sdk with a one-line async helper that delegates to `Config::new().setup_api(namespace).await`. - Drops TrustedHttpContextProvider, build_testnet_context_provider, and the manual DPNS system-contract pre-load (Core RPC + standard fetch path is enough — no test depended on the pre-loaded contract for assertions). - Gives each test a unique namespace so per-test vector dump dirs don't collide when running with `--features generate-test-vectors`. The Network::Testnet hint is dropped along with the trusted context provider — `Config::setup_api` doesn't expose the network field, and the canonical `tests/fetch/*` suite operates without it just fine for proof verification via Core RPC.
…(chain-agnostic)
The relocated test asserted testnet-specific data: that "alice" was registered,
that it resolved to a hardcoded identity, that a prefix search returned at
least one row. Against a local devnet or any reset chain those assertions
fail — the chain simply doesn't have that data.
Downgrade to a call-and-log smoke test (renamed test_dpns_queries_smoke):
each SDK call still uses .expect("…should succeed") so transport or
proof-verification failures still crash loudly, but the existence of
specific on-chain rows is logged, not asserted. The test now exercises the
DPNS query code paths against any network.
There was a problem hiding this comment.
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/rs-sdk/tests/dpns_usernames.rs (1)
37-653:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftNetwork-dependent integration tests should not hit live Platform/Core directly.
These tests invoke real network endpoints from
tests/, which makes them non-deterministic and environment-coupled. Please move this coverage to an e2e suite boundary (or gate it outside normal integration tests) and keeptests/mocked via the existing SDK mock harness.As per coding guidelines:
Unit and integration tests should not perform network calls; mock dependencies.🤖 Prompt for 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. In `@packages/rs-sdk/tests/dpns_usernames.rs` around lines 37 - 653, These integration tests call real network endpoints via build_testnet_sdk and must be moved out of unit/integration tests or switched to the SDK mock harness; either (A) move the test functions (test_dpns_queries, test_contested_queries, test_get_current_dpns_contests, test_get_contested_non_resolved_usernames, test_get_non_resolved_dpns_contests_for_identity, test_dpns_queries_smoke, test_dpns_search_variations) into an e2e test module/crate (or gate them behind an "e2e" cfg/feature) so they do not run in standard CI, or (B) refactor each to use the existing mock builder instead of build_testnet_sdk (replace calls to build_testnet_sdk(...) with the mock factory, e.g. build_mock_sdk or the SDK mock harness) and assert using mocked responses; update any helper calls that depend on real network data (search_dpns_names, check_dpns_name_availability, resolve_dpns_name_to_identity, get_dpns_usernames_by_identity, get_contested_dpns_normalized_usernames, get_contested_dpns_vote_state, get_contested_dpns_usernames_by_identity, get_contested_dpns_identity_votes, get_current_dpns_contests, get_contested_non_resolved_usernames, get_non_resolved_dpns_contests_for_identity) accordingly so tests no longer perform live network I/O.
🤖 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/rs-sdk/tests/dpns_usernames.rs`:
- Around line 74-91: The test currently swallows errors from the SDK call
get_contested_dpns_normalized_usernames by matching Err(e) and only printing;
change the test to assert the call succeeds (e.g., expect/unwrap on the Result
from get_contested_dpns_normalized_usernames stored in all_contested) so
transport/proof failures fail the test, and then handle the successful Vec case
to allow an empty list as a non-failure; apply the same change pattern to other
similar places noted (the Err(e) branches at the other ranges) so all
network/proof calls assert success rather than quietly logging errors.
---
Outside diff comments:
In `@packages/rs-sdk/tests/dpns_usernames.rs`:
- Around line 37-653: These integration tests call real network endpoints via
build_testnet_sdk and must be moved out of unit/integration tests or switched to
the SDK mock harness; either (A) move the test functions (test_dpns_queries,
test_contested_queries, test_get_current_dpns_contests,
test_get_contested_non_resolved_usernames,
test_get_non_resolved_dpns_contests_for_identity, test_dpns_queries_smoke,
test_dpns_search_variations) into an e2e test module/crate (or gate them behind
an "e2e" cfg/feature) so they do not run in standard CI, or (B) refactor each to
use the existing mock builder instead of build_testnet_sdk (replace calls to
build_testnet_sdk(...) with the mock factory, e.g. build_mock_sdk or the SDK
mock harness) and assert using mocked responses; update any helper calls that
depend on real network data (search_dpns_names, check_dpns_name_availability,
resolve_dpns_name_to_identity, get_dpns_usernames_by_identity,
get_contested_dpns_normalized_usernames, get_contested_dpns_vote_state,
get_contested_dpns_usernames_by_identity, get_contested_dpns_identity_votes,
get_current_dpns_contests, get_contested_non_resolved_usernames,
get_non_resolved_dpns_contests_for_identity) accordingly so tests no longer
perform live network I/O.
🪄 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: 414d0972-ccf1-48e9-8c21-1ff29c8d621b
📒 Files selected for processing (4)
packages/rs-sdk/src/platform/dpns_usernames/contested_queries.rspackages/rs-sdk/src/platform/dpns_usernames/queries.rspackages/rs-sdk/tests/dpns_queries_test.rspackages/rs-sdk/tests/dpns_usernames.rs
💤 Files with no reviewable changes (3)
- packages/rs-sdk/tests/dpns_queries_test.rs
- packages/rs-sdk/src/platform/dpns_usernames/contested_queries.rs
- packages/rs-sdk/src/platform/dpns_usernames/queries.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative verification at 23b3a4a. The latest 4-commit delta is exclusively DPNS test relocation and does not touch any reviewed production path. All 8 prior findings remain valid at the same files/lines (re-verified end-to-end). One new finding from this delta: the new tests/dpns_usernames.rs header documents a cargo test invocation that cannot actually exercise the live-network path because default features (including offline-testing) take precedence over network-testing. The blocking issue — re-enabled document fetch tests pointing at obsolete msg_DocumentQuery_*.json fixtures — is unchanged: load_expectations_sync only recognises GetDocumentsRequest, so each un-ignored test panics at SDK init in offline mode (the default).
🔴 1 blocking | 🟡 8 suggestion(s)
9 additional finding(s)
blocking: Re-enabled offline document fetch tests will deterministically panic — fixtures were not regenerated
packages/rs-sdk/tests/fetch/document.rs (line 15)
Re-verified at 23b3a4a. The four document fetch tests (document_read at :15, document_read_no_document at :82, document_list_drive_query at :108, document_list_document_query at :153) are all un-ignored, but their vector directories still contain only pre-refactor msg_DocumentQuery_*.json files (e.g. document_read/msg_DocumentQuery_2a42b0af…json and …_4816e45a…json, document_list_drive_query/msg_DocumentQuery_2fd2f7ae…json, etc.) and no msg_GetDocumentsRequest_*.json. MockDashPlatformSdk::load_expectations_sync at src/mock/sdk.rs:136-137 only recognises "GetDocumentsRequest"; the catch-all returns Error::Config("unknown request type … missing match arm in load_expectations?"). The mock branch of SdkBuilder::build() at src/sdk.rs:1150-1152 calls guard.load_expectations_sync(dump_dir)? before returning, so the error fires at SDK init via Config::setup_api(...) (which uses .expect("cannot initialize api")). dash-sdk defaults include offline-testing, so plain cargo test -p dash-sdk enters this branch and each test panics before any assertion runs. Either regenerate the four fixtures against testnet so they land as msg_GetDocumentsRequest_*.json in the same commit, or restore #[ignore] with a tracking issue. Shipping known-panicking offline tests on the rationale that ‘failures show up in CI’ is not an acceptable merge state.
suggestion: with_initial_version() is not propagated into the mock proof-verifier
packages/rs-sdk/src/sdk.rs (line 1119)
Verified at sdk.rs:1119. The mock branch of SdkBuilder::build() still constructs MockDashPlatformSdk::new(self.version, Arc::clone(&dapi)), while the outer Sdk.protocol_version atomic at sdk.rs:1132-1140 honours self.initial_version when !self.version_explicit. MockDashPlatformSdk::version() returns the inner pinned PlatformVersion, which is used by parse_proof_with_metadata whenever a mock request misses a prerecorded expectation and falls back to real FromProof decoding. A builder configured via with_initial_version() therefore encodes requests at the requested initial PV but verifies proofs against PlatformVersion::latest() — split-brain state within a single SDK instance. With the four document fetch tests now un-ignored, this is exactly the offline path that will be exercised once the fixture-regeneration issue above is addressed.
let mock_version = if self.version_explicit {
self.version
} else {
self.initial_version.unwrap_or(self.version)
};
let mock_sdk = MockDashPlatformSdk::new(mock_version, Arc::clone(&dapi));
suggestion: Default auto-detect SDKs still emit latest-version documents on first getDocuments against pre-v3.1 networks
packages/rs-sdk/src/sdk.rs (line 489)
Sdk::version() at sdk.rs:489-492 falls back to PlatformVersion::latest() whenever the per-instance atomic is 0, and Sdk::query_context() at sdk.rs:511-517 hands that PV straight to the DocumentQuery → GetDocumentsRequest encoder (document_query.rs:949-956), which dispatches entirely on ctx.protocol_version. Auto-detect only updates the atomic after a successful response-metadata parse, so a default-built SDK that talks to a pre-v3.1 network and makes getDocuments as its first meaningful request still encodes V1, hits the gRPC Unknown decode error described in the PR body, and gets its node banned by rs-dapi-client before auto-detect can ratchet downward. The new rustdoc caveat on with_initial_version documents this requirement but does not change the default behaviour. Either bias the auto-detect fallback toward the lowest still-supported PV, issue a cheap version-discovery probe before the first document query, or call this limitation out prominently at the SdkBuilder doc level (not only inside the opt-in helper).
suggestion: with_version() silently overrides with_initial_version() with no diagnostic
packages/rs-sdk/src/sdk.rs (line 1130)
Verified at sdk.rs:1132-1140 (mock branch) and the symmetric dapi branch. When self.version_explicit is true the explicit with_version() seed wins and self.initial_version is silently discarded. The with_initial_version rustdoc talks about encoder pinning but does not document this precedence at the SdkBuilder level. A caller writing .with_version(latest).with_initial_version(pv11) likely intends pre-v3.1-network behaviour but instead gets auto-detect disabled and PV pinned to latest — precisely the misconfiguration this PR is meant to fix. The compiler cannot express this conflict in the type system, so either reject the combination at build() time with Error::Config, or emit a tracing::warn! describing the precedence.
suggestion: FFI does not expose with_initial_version — Swift/iOS consumers cannot invoke the PR's documented mitigation
packages/rs-sdk-ffi/src/types.rs (line 60)
Re-verified: Grep for with_initial_version|initial_protocol_version over packages/rs-sdk-ffi/ returns zero matches. DashSDKConfig/DashSDKConfigTrusted at types.rs:60-74 have no initial_protocol_version field, and dash_sdk_create / dash_sdk_create_extended / dash_sdk_create_trusted all build SdkBuilder without any with_initial_version() hook. SdkBuilder::with_initial_version(&PlatformVersion) is the PR's documented mitigation for booting against a pre-v3.1 network (v3.0 testnet, PV_11), yet it is unreachable from the FFI surface — which is the primary downstream consumer (iOS SDK / SwiftExampleApp) that motivates this monorepo's FFI crate. Cross-boundary consequence: a Swift app on a PV_11 network seeds the atomic at 0 → Sdk::version() returns latest() → first Document::fetch_many encodes V1 → Drive returns gRPC Unknown → rs-dapi-client ban-lists the masternode before auto-detect ratchets. Minimal fix: add pub initial_protocol_version: u32 to DashSDKConfig/DashSDKConfigTrusted (0 as a clean sentinel — no valid PV is 0; struct stays #[repr(C)]-clean), then in the FFI build path apply with_initial_version(PlatformVersion::get(cfg.initial_protocol_version)?) when non-zero. Alternative: a dash_sdk_set_initial_protocol_version entry point invoked before any fetch.
suggestion: Two-step encode adds a redundant clone on every non-versioned fetch
packages/rs-sdk/src/platform/fetch.rs (line 177)
Verified at fetch.rs:177-179: let ctx = sdk.query_context(); let owned_rich: Self::Query = query.query(&ctx)?; let owned_wire: Self::Request = owned_rich.query(&ctx)?;. For the majority of Fetch impls (Identity, DataContract, all token/balance types, …) type Query = Self::Request, so the second call resolves to the blanket impl<T> Query<T> for T and just returns Ok(self.clone()). The same pattern exists in FetchMany. Every ordinary fetch now pays an extra full clone of the proto request even though only document queries need a second version-aware encoding pass. Specialize via a sealed helper trait that short-circuits when Self::Query == Self::Request, or add a stable fast path so the non-versioned majority avoids the extra copy.
suggestion: Query for DocumentQuery silently drops ctx.prove
packages/rs-sdk/src/platform/documents/document_query.rs (line 949)
document_query.rs:949-956 forwards only ctx.protocol_version to GetDocumentsRequest::try_from_platform_versioned(self.clone(), ctx.protocol_version) and discards ctx.prove. Inside encode_v0/encode_v1, prove: true is hardcoded on the wire. Today documents are always proof-verified, so this is intentional — but a future caller using FetchUnproved or passing ctx.without_proofs() will silently emit prove: true on the wire. The c723cfe revert explicitly documents per-site policy as intentional for upstream Query<DocumentQuery> impls, but this downstream wire encoder is the remaining outlier ignoring one field of a context object nearby impls do honour. Either honour ctx.prove for a single source of truth, or short-circuit with Error::Config when !ctx.prove so the mismatch is loud.
suggestion: QueryContext.request_settings reflects SDK defaults, not per-call effective settings
packages/rs-sdk/src/platform/query_context.rs (line 28)
Sdk::query_context() at sdk.rs:511-517 borrows &self.dapi_client_settings, but Fetch::fetch_with_metadata_and_proof composes those defaults with the caller's Option<RequestSettings> after the ctx has been handed to Query::query. No current encoder reads ctx.request_settings, so this is harmless today — but the rustdoc on query_context.rs:29-31 justifies the field as forward-compatibility for an encoder that consults transport hints. The day such an encoder lands, it will observe SDK-default settings rather than effective per-request settings — a subtle correctness bug waiting to happen, and a behavioural change rather than internal refactor once downstream code starts depending on the field. Either compose per-call settings before building the QueryContext, or drop the field until the first real consumer materializes.
suggestion: Documented `cargo test` invocation cannot reach the live-network path under default features
packages/rs-sdk/tests/dpns_usernames.rs (line 10)
The header tells contributors to run cargo test -p dash-sdk --features generate-test-vectors -- --include-ignored, but that command leaves the crate's default features enabled. packages/rs-sdk/Cargo.toml:75-80 makes offline-testing part of default, and Cargo.toml:121 plus tests/fetch/config.rs:182-222 confirm that when both offline-testing and network-testing are enabled, the #[cfg(feature = "offline-testing")] branch wins ("offline-testing takes precedence over network-testing"). Since generate-test-vectors only adds network-testing, the documented command still builds the mock SDK and loads on-disk vectors instead of hitting the configured Platform endpoint — exactly the opposite of what the test file advertises. Either add --no-default-features --features generate-test-vectors,mocks,dapi-grpc/client,token_reward_explanations (or equivalent), or document the actual incantation that disables offline-testing.
🤖 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.
- [BLOCKING] In `packages/rs-sdk/tests/fetch/document.rs`:15-153: Re-enabled offline document fetch tests will deterministically panic — fixtures were not regenerated
Re-verified at 23b3a4aaad. The four document fetch tests (`document_read` at :15, `document_read_no_document` at :82, `document_list_drive_query` at :108, `document_list_document_query` at :153) are all un-ignored, but their vector directories still contain only pre-refactor `msg_DocumentQuery_*.json` files (e.g. `document_read/msg_DocumentQuery_2a42b0af…json` and `…_4816e45a…json`, `document_list_drive_query/msg_DocumentQuery_2fd2f7ae…json`, etc.) and no `msg_GetDocumentsRequest_*.json`. `MockDashPlatformSdk::load_expectations_sync` at `src/mock/sdk.rs:136-137` only recognises `"GetDocumentsRequest"`; the catch-all returns `Error::Config("unknown request type … missing match arm in load_expectations?")`. The mock branch of `SdkBuilder::build()` at `src/sdk.rs:1150-1152` calls `guard.load_expectations_sync(dump_dir)?` before returning, so the error fires at SDK init via `Config::setup_api(...)` (which uses `.expect("cannot initialize api")`). `dash-sdk` defaults include `offline-testing`, so plain `cargo test -p dash-sdk` enters this branch and each test panics before any assertion runs. Either regenerate the four fixtures against testnet so they land as `msg_GetDocumentsRequest_*.json` in the same commit, or restore `#[ignore]` with a tracking issue. Shipping known-panicking offline tests on the rationale that ‘failures show up in CI’ is not an acceptable merge state.
- [SUGGESTION] In `packages/rs-sdk/src/sdk.rs`:1119-1119: with_initial_version() is not propagated into the mock proof-verifier
Verified at sdk.rs:1119. The mock branch of `SdkBuilder::build()` still constructs `MockDashPlatformSdk::new(self.version, Arc::clone(&dapi))`, while the outer `Sdk.protocol_version` atomic at sdk.rs:1132-1140 honours `self.initial_version` when `!self.version_explicit`. `MockDashPlatformSdk::version()` returns the inner pinned `PlatformVersion`, which is used by `parse_proof_with_metadata` whenever a mock request misses a prerecorded expectation and falls back to real `FromProof` decoding. A builder configured via `with_initial_version()` therefore encodes requests at the requested initial PV but verifies proofs against `PlatformVersion::latest()` — split-brain state within a single SDK instance. With the four document fetch tests now un-ignored, this is exactly the offline path that will be exercised once the fixture-regeneration issue above is addressed.
- [SUGGESTION] In `packages/rs-sdk/src/sdk.rs`:489-517: Default auto-detect SDKs still emit latest-version documents on first getDocuments against pre-v3.1 networks
`Sdk::version()` at sdk.rs:489-492 falls back to `PlatformVersion::latest()` whenever the per-instance atomic is 0, and `Sdk::query_context()` at sdk.rs:511-517 hands that PV straight to the `DocumentQuery → GetDocumentsRequest` encoder (document_query.rs:949-956), which dispatches entirely on `ctx.protocol_version`. Auto-detect only updates the atomic after a successful response-metadata parse, so a default-built SDK that talks to a pre-v3.1 network and makes `getDocuments` as its first meaningful request still encodes V1, hits the gRPC Unknown decode error described in the PR body, and gets its node banned by rs-dapi-client before auto-detect can ratchet downward. The new rustdoc caveat on `with_initial_version` documents this requirement but does not change the default behaviour. Either bias the auto-detect fallback toward the lowest still-supported PV, issue a cheap version-discovery probe before the first document query, or call this limitation out prominently at the `SdkBuilder` doc level (not only inside the opt-in helper).
- [SUGGESTION] In `packages/rs-sdk/src/sdk.rs`:1130-1140: with_version() silently overrides with_initial_version() with no diagnostic
Verified at sdk.rs:1132-1140 (mock branch) and the symmetric dapi branch. When `self.version_explicit` is true the explicit `with_version()` seed wins and `self.initial_version` is silently discarded. The `with_initial_version` rustdoc talks about encoder pinning but does not document this precedence at the `SdkBuilder` level. A caller writing `.with_version(latest).with_initial_version(pv11)` likely intends pre-v3.1-network behaviour but instead gets auto-detect disabled and PV pinned to latest — precisely the misconfiguration this PR is meant to fix. The compiler cannot express this conflict in the type system, so either reject the combination at `build()` time with `Error::Config`, or emit a `tracing::warn!` describing the precedence.
- [SUGGESTION] In `packages/rs-sdk-ffi/src/types.rs`:60-74: FFI does not expose with_initial_version — Swift/iOS consumers cannot invoke the PR's documented mitigation
Re-verified: Grep for `with_initial_version|initial_protocol_version` over `packages/rs-sdk-ffi/` returns zero matches. `DashSDKConfig`/`DashSDKConfigTrusted` at types.rs:60-74 have no `initial_protocol_version` field, and `dash_sdk_create` / `dash_sdk_create_extended` / `dash_sdk_create_trusted` all build `SdkBuilder` without any `with_initial_version()` hook. `SdkBuilder::with_initial_version(&PlatformVersion)` is the PR's documented mitigation for booting against a pre-v3.1 network (v3.0 testnet, PV_11), yet it is unreachable from the FFI surface — which is the primary downstream consumer (iOS SDK / SwiftExampleApp) that motivates this monorepo's FFI crate. Cross-boundary consequence: a Swift app on a PV_11 network seeds the atomic at 0 → `Sdk::version()` returns `latest()` → first `Document::fetch_many` encodes V1 → Drive returns gRPC Unknown → `rs-dapi-client` ban-lists the masternode before auto-detect ratchets. Minimal fix: add `pub initial_protocol_version: u32` to `DashSDKConfig`/`DashSDKConfigTrusted` (0 as a clean sentinel — no valid PV is 0; struct stays `#[repr(C)]`-clean), then in the FFI build path apply `with_initial_version(PlatformVersion::get(cfg.initial_protocol_version)?)` when non-zero. Alternative: a `dash_sdk_set_initial_protocol_version` entry point invoked before any fetch.
- [SUGGESTION] In `packages/rs-sdk/src/platform/fetch.rs`:177-179: Two-step encode adds a redundant clone on every non-versioned fetch
Verified at fetch.rs:177-179: `let ctx = sdk.query_context(); let owned_rich: Self::Query = query.query(&ctx)?; let owned_wire: Self::Request = owned_rich.query(&ctx)?;`. For the majority of `Fetch` impls (Identity, DataContract, all token/balance types, …) `type Query = Self::Request`, so the second call resolves to the blanket `impl<T> Query<T> for T` and just returns `Ok(self.clone())`. The same pattern exists in `FetchMany`. Every ordinary fetch now pays an extra full clone of the proto request even though only document queries need a second version-aware encoding pass. Specialize via a sealed helper trait that short-circuits when `Self::Query == Self::Request`, or add a stable fast path so the non-versioned majority avoids the extra copy.
- [SUGGESTION] In `packages/rs-sdk/src/platform/documents/document_query.rs`:949-956: Query<GetDocumentsRequest> for DocumentQuery silently drops ctx.prove
document_query.rs:949-956 forwards only `ctx.protocol_version` to `GetDocumentsRequest::try_from_platform_versioned(self.clone(), ctx.protocol_version)` and discards `ctx.prove`. Inside `encode_v0`/`encode_v1`, `prove: true` is hardcoded on the wire. Today documents are always proof-verified, so this is intentional — but a future caller using `FetchUnproved` or passing `ctx.without_proofs()` will silently emit `prove: true` on the wire. The c723cfe092 revert explicitly documents per-site policy as intentional for upstream `Query<DocumentQuery>` impls, but this downstream wire encoder is the remaining outlier ignoring one field of a context object nearby impls do honour. Either honour `ctx.prove` for a single source of truth, or short-circuit with `Error::Config` when `!ctx.prove` so the mismatch is loud.
- [SUGGESTION] In `packages/rs-sdk/src/platform/query_context.rs`:28-38: QueryContext.request_settings reflects SDK defaults, not per-call effective settings
`Sdk::query_context()` at sdk.rs:511-517 borrows `&self.dapi_client_settings`, but `Fetch::fetch_with_metadata_and_proof` composes those defaults with the caller's `Option<RequestSettings>` *after* the `ctx` has been handed to `Query::query`. No current encoder reads `ctx.request_settings`, so this is harmless today — but the rustdoc on query_context.rs:29-31 justifies the field as forward-compatibility for an encoder that consults transport hints. The day such an encoder lands, it will observe SDK-default settings rather than effective per-request settings — a subtle correctness bug waiting to happen, and a behavioural change rather than internal refactor once downstream code starts depending on the field. Either compose per-call settings before building the `QueryContext`, or drop the field until the first real consumer materializes.
- [SUGGESTION] In `packages/rs-sdk/tests/dpns_usernames.rs`:10-13: Documented `cargo test` invocation cannot reach the live-network path under default features
The header tells contributors to run `cargo test -p dash-sdk --features generate-test-vectors -- --include-ignored`, but that command leaves the crate's default features enabled. `packages/rs-sdk/Cargo.toml:75-80` makes `offline-testing` part of `default`, and `Cargo.toml:121` plus `tests/fetch/config.rs:182-222` confirm that when both `offline-testing` and `network-testing` are enabled, the `#[cfg(feature = "offline-testing")]` branch wins ("offline-testing takes precedence over network-testing"). Since `generate-test-vectors` only adds `network-testing`, the documented command still builds the mock SDK and loads on-disk vectors instead of hitting the configured Platform endpoint — exactly the opposite of what the test file advertises. Either add `--no-default-features --features generate-test-vectors,mocks,dapi-grpc/client,token_reward_explanations` (or equivalent), or document the actual incantation that disables `offline-testing`.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
lklimek
changed the title
Reverts the following commits, moved to a dedicated PR off v3.1-dev so #3711 can stay focused on the V0/V1 wire compatibility fix: 09df598 test(rs-sdk): make DPNS network tests honor tests/.env DASH_SDK_PLATFORM_* vars 9745211 refactor(rs-sdk): move DPNS network tests from src/ to tests/, drop band-aid endpoint helper 23ee2ae test(rs-sdk): wire relocated DPNS tests through Config::setup_api for local Core RPC quorum lookup 23b3a4a test(rs-sdk): loosen test_dpns_queries_from_docs to smoke-test shape (chain-agnostic) The contested_resource vector regeneration (f384ca7) is independent and stays on this PR.
|
✅ 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: "e6f382463e1f2578d448fd1238437301df98815eca2592f548a5812c430a62e1"
)Xcode manual integration:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Most of the carried review comments are real and still apply at f384ca7c3d8c6be7a5a9a59cfe0a2fe184fb11f7. The remaining valid issues are all suggestions rather than blockers, concentrated in protocol-version bootstrapping, the new builder/API surface, and the mismatch between the Rust-side compatibility knob and the FFI boundary.
🟡 6 suggestion(s)
6 additional finding(s)
suggestion: `with_initial_version()` does not seed the inner mock proof parser
packages/rs-sdk/src/sdk.rs (line 1119)
The mock branch of SdkBuilder::build() still constructs MockDashPlatformSdk with self.version, while the outer Sdk.protocol_version atomic is seeded from self.initial_version when auto-detect is left on. That creates two independent protocol-version sources inside one mock SDK: request encoding reads sdk.query_context().protocol_version, but fallback proof parsing in MockDashPlatformSdk::parse_proof_with_metadata() reads self.platform_version. In offline/mock runs that use with_initial_version(), the request can be encoded for one historical platform version and then parsed against another, which defeats the compatibility guarantee this API is supposed to provide.
let mock_version = if self.version_explicit {
self.version
} else {
self.initial_version.unwrap_or(self.version)
};
let mock_sdk = MockDashPlatformSdk::new(mock_version, Arc::clone(&dapi));
suggestion: A default auto-detect SDK still sends the newest document-query wire shape on its first request
packages/rs-sdk/src/sdk.rs (line 489)
Sdk::version() still falls back to PlatformVersion::latest() whenever the per-instance atomic is 0, and Sdk::query_context() passes that value straight into the version-dispatched document encoder. The actual network version is only learned after a response is parsed, so a freshly built SDK still emits the latest GetDocumentsRequest shape on its first document query against an older network unless the caller already knew to seed with_initial_version(). That means the runtime default still reproduces the first-contact incompatibility this PR is trying to address.
suggestion: `with_version()` silently overrides `with_initial_version()`
packages/rs-sdk/src/sdk.rs (line 888)
The builder exposes both with_version() and with_initial_version(), but build() gives unconditional precedence to version_explicit and discards the initial seed without any diagnostic. Those setters look composable from the public API, so a caller can believe they are bootstrapping auto-detect for an older network while actually pinning the SDK permanently. This is a real footgun in the new version-seeding surface because the conflict is invisible until requests start using the wrong protocol version policy.
suggestion: The FFI boundary still cannot express the initial protocol-version seed needed for older-network interop
packages/rs-sdk-ffi/src/types.rs (line 52)
The Rust SDK now documents SdkBuilder::with_initial_version(...) as the way to make the first request compatible with pre-v3.1 networks, but DashSDKConfig still has no field for that seed and the exported constructors in packages/rs-sdk-ffi/src/sdk.rs never call with_initial_version(). As a result, C and Swift callers are forced onto the old latest() bootstrap path even though native Rust callers now have a correctness knob for that case. The compatibility fix is therefore incomplete at the public boundary most non-Rust consumers use.
suggestion: The new rich-query to wire-request split adds an unconditional extra clone on the common path
packages/rs-sdk/src/platform/fetch.rs (line 177)
fetch_with_metadata_and_proof() now normalizes the caller input into Self::Query and immediately re-encodes that into Self::Request. For the common case where Self::Query = Self::Request, the blanket Query<T> for T impl makes that second step a pure clone of the same owned request. This is not a correctness issue, but it adds avoidable copying on every ordinary fetch even though only the document-query path actually needs a distinct rich-to-wire conversion.
suggestion: The documented vector-regeneration command still runs the offline harness by default
packages/rs-sdk/tests/dpns_usernames.rs (line 10)
The header tells contributors to run cargo test -p dash-sdk --features generate-test-vectors -- --include-ignored, but dash-sdk enables offline-testing by default in Cargo.toml, and generate-test-vectors only adds network-testing. tests/fetch/config.rs explicitly gives offline-testing precedence when both features are enabled, so that command replays existing vectors instead of hitting a live network. As written, the checked-in regeneration instructions are not reproducible.
🤖 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/rs-sdk/src/sdk.rs`:1119-1149: `with_initial_version()` does not seed the inner mock proof parser
The mock branch of `SdkBuilder::build()` still constructs `MockDashPlatformSdk` with `self.version`, while the outer `Sdk.protocol_version` atomic is seeded from `self.initial_version` when auto-detect is left on. That creates two independent protocol-version sources inside one mock SDK: request encoding reads `sdk.query_context().protocol_version`, but fallback proof parsing in `MockDashPlatformSdk::parse_proof_with_metadata()` reads `self.platform_version`. In offline/mock runs that use `with_initial_version()`, the request can be encoded for one historical platform version and then parsed against another, which defeats the compatibility guarantee this API is supposed to provide.
- [SUGGESTION] In `packages/rs-sdk/src/sdk.rs`:489-517: A default auto-detect SDK still sends the newest document-query wire shape on its first request
`Sdk::version()` still falls back to `PlatformVersion::latest()` whenever the per-instance atomic is `0`, and `Sdk::query_context()` passes that value straight into the version-dispatched document encoder. The actual network version is only learned after a response is parsed, so a freshly built SDK still emits the latest `GetDocumentsRequest` shape on its first document query against an older network unless the caller already knew to seed `with_initial_version()`. That means the runtime default still reproduces the first-contact incompatibility this PR is trying to address.
- [SUGGESTION] In `packages/rs-sdk/src/sdk.rs`:888-923: `with_version()` silently overrides `with_initial_version()`
The builder exposes both `with_version()` and `with_initial_version()`, but `build()` gives unconditional precedence to `version_explicit` and discards the initial seed without any diagnostic. Those setters look composable from the public API, so a caller can believe they are bootstrapping auto-detect for an older network while actually pinning the SDK permanently. This is a real footgun in the new version-seeding surface because the conflict is invisible until requests start using the wrong protocol version policy.
- [SUGGESTION] In `packages/rs-sdk-ffi/src/types.rs`:52-74: The FFI boundary still cannot express the initial protocol-version seed needed for older-network interop
The Rust SDK now documents `SdkBuilder::with_initial_version(...)` as the way to make the first request compatible with pre-v3.1 networks, but `DashSDKConfig` still has no field for that seed and the exported constructors in `packages/rs-sdk-ffi/src/sdk.rs` never call `with_initial_version()`. As a result, C and Swift callers are forced onto the old `latest()` bootstrap path even though native Rust callers now have a correctness knob for that case. The compatibility fix is therefore incomplete at the public boundary most non-Rust consumers use.
- [SUGGESTION] In `packages/rs-sdk/src/platform/fetch.rs`:177-179: The new rich-query to wire-request split adds an unconditional extra clone on the common path
`fetch_with_metadata_and_proof()` now normalizes the caller input into `Self::Query` and immediately re-encodes that into `Self::Request`. For the common case where `Self::Query = Self::Request`, the blanket `Query<T> for T` impl makes that second step a pure clone of the same owned request. This is not a correctness issue, but it adds avoidable copying on every ordinary fetch even though only the document-query path actually needs a distinct rich-to-wire conversion.
- [SUGGESTION] In `packages/rs-sdk/tests/dpns_usernames.rs`:10-12: The documented vector-regeneration command still runs the offline harness by default
The header tells contributors to run `cargo test -p dash-sdk --features generate-test-vectors -- --include-ignored`, but `dash-sdk` enables `offline-testing` by default in `Cargo.toml`, and `generate-test-vectors` only adds `network-testing`. `tests/fetch/config.rs` explicitly gives `offline-testing` precedence when both features are enabled, so that command replays existing vectors instead of hitting a live network. As written, the checked-in regeneration instructions are not reproducible.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
There was a problem hiding this comment.
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/rs-sdk/tests/vectors/document_list_document_query/quorum_pubkey-106-303c6a7b1e3aa25fa847ce5f6d6f39c3252081bcf291d632bbf38d43878d4777.json`:
- Line 1: The file contains an invalid top-level JSON value (a raw unquoted
string: the long hex blob in the vector file) which breaks parsing; update the
vector file
(packages/rs-sdk/tests/vectors/document_list_document_query/quorum_pubkey-106-303c6a7b1e3aa25fa847ce5f6d6f39c3252081bcf291d632bbf38d43878d4777.json)
so the top-level value is valid JSON—e.g., wrap the hex string in quotes to
become a JSON string or convert it into a JSON object/array as required by the
tests (ensure the test harness expects a string vs object), then run the vector
tests to confirm parsing succeeds.
🪄 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: 0c4a81cc-50ef-4d85-a985-c9efdbbcae9d
📒 Files selected for processing (25)
packages/rs-sdk/tests/vectors/document_list_document_query/msg_DocumentQuery_2a42b0afe6c6f58b02d8152142acec7d11a37410433366315d1103f81859344b.jsonpackages/rs-sdk/tests/vectors/document_list_document_query/msg_GetDataContractRequest_e87a2e6acef76975c30eb7272da71733fb6ad13495beb7ca1b6a6c4ceb30e0f7.jsonpackages/rs-sdk/tests/vectors/document_list_document_query/msg_GetDocumentsRequest_02213251619b5f2925ddad3e4e0a2380e57616a701bfbde50d048d23e0e09094.jsonpackages/rs-sdk/tests/vectors/document_list_document_query/quorum_pubkey-106-303c6a7b1e3aa25fa847ce5f6d6f39c3252081bcf291d632bbf38d43878d4777.jsonpackages/rs-sdk/tests/vectors/document_list_document_query/quorum_pubkey-106-3603d12872604475619be7ad682f8339ad60debd43b3f430bd12d51eac268936.jsonpackages/rs-sdk/tests/vectors/document_list_drive_query/msg_DocumentQuery_2fd2f7aebe5686d4e4179323b49e8920dea81c3f44b9549c238dcb82cccc9923.jsonpackages/rs-sdk/tests/vectors/document_list_drive_query/msg_GetDataContractRequest_e87a2e6acef76975c30eb7272da71733fb6ad13495beb7ca1b6a6c4ceb30e0f7.jsonpackages/rs-sdk/tests/vectors/document_list_drive_query/msg_GetDocumentsRequest_331458c78645abf6543c22204af22570c7fa61490e1c5c381c73a4a3e1bacc84.jsonpackages/rs-sdk/tests/vectors/document_list_drive_query/quorum_pubkey-106-303c6a7b1e3aa25fa847ce5f6d6f39c3252081bcf291d632bbf38d43878d4777.jsonpackages/rs-sdk/tests/vectors/document_list_drive_query/quorum_pubkey-106-3603d12872604475619be7ad682f8339ad60debd43b3f430bd12d51eac268936.jsonpackages/rs-sdk/tests/vectors/document_read/msg_DocumentQuery_2a42b0afe6c6f58b02d8152142acec7d11a37410433366315d1103f81859344b.jsonpackages/rs-sdk/tests/vectors/document_read/msg_DocumentQuery_4816e45a0ba02d69c291f23322410a8556ecf1145e99c87071cc8998a97d7ccb.jsonpackages/rs-sdk/tests/vectors/document_read/msg_GetDataContractRequest_e87a2e6acef76975c30eb7272da71733fb6ad13495beb7ca1b6a6c4ceb30e0f7.jsonpackages/rs-sdk/tests/vectors/document_read/msg_GetDocumentsRequest_02213251619b5f2925ddad3e4e0a2380e57616a701bfbde50d048d23e0e09094.jsonpackages/rs-sdk/tests/vectors/document_read/msg_GetDocumentsRequest_2496027c61fad2723bcec60b574423e3d41ce93dde03d5299b46b8e4858a0e3b.jsonpackages/rs-sdk/tests/vectors/document_read/quorum_pubkey-106-1c97c16f6b472d0beadfb1bb83aadc440dc2be19879e09aeb342056f8f58e0fe.jsonpackages/rs-sdk/tests/vectors/document_read/quorum_pubkey-106-3603d12872604475619be7ad682f8339ad60debd43b3f430bd12d51eac268936.jsonpackages/rs-sdk/tests/vectors/document_read_no_contract/msg_GetDataContractRequest_e4cf74168e03a40bd159451456b501c1ba166a2dd8f6efb31b0289dc011da983.jsonpackages/rs-sdk/tests/vectors/document_read_no_contract/quorum_pubkey-106-1c97c16f6b472d0beadfb1bb83aadc440dc2be19879e09aeb342056f8f58e0fe.jsonpackages/rs-sdk/tests/vectors/document_read_no_contract/quorum_pubkey-106-3603d12872604475619be7ad682f8339ad60debd43b3f430bd12d51eac268936.jsonpackages/rs-sdk/tests/vectors/document_read_no_document/msg_DocumentQuery_f531f69255dae40cef880290c177ff33d2448d30f2527374349b9f507b44c5c1.jsonpackages/rs-sdk/tests/vectors/document_read_no_document/msg_GetDataContractRequest_e87a2e6acef76975c30eb7272da71733fb6ad13495beb7ca1b6a6c4ceb30e0f7.jsonpackages/rs-sdk/tests/vectors/document_read_no_document/msg_GetDocumentsRequest_9256bdd6fc78e23f941d17d93b0230cf0f134e114c5e23ad7f08ed94c92f962f.jsonpackages/rs-sdk/tests/vectors/document_read_no_document/quorum_pubkey-106-1c97c16f6b472d0beadfb1bb83aadc440dc2be19879e09aeb342056f8f58e0fe.jsonpackages/rs-sdk/tests/vectors/document_read_no_document/quorum_pubkey-106-3603d12872604475619be7ad682f8339ad60debd43b3f430bd12d51eac268936.json
💤 Files with no reviewable changes (5)
- packages/rs-sdk/tests/vectors/document_read_no_contract/quorum_pubkey-106-3603d12872604475619be7ad682f8339ad60debd43b3f430bd12d51eac268936.json
- packages/rs-sdk/tests/vectors/document_read_no_document/quorum_pubkey-106-3603d12872604475619be7ad682f8339ad60debd43b3f430bd12d51eac268936.json
- packages/rs-sdk/tests/vectors/document_list_drive_query/quorum_pubkey-106-3603d12872604475619be7ad682f8339ad60debd43b3f430bd12d51eac268936.json
- packages/rs-sdk/tests/vectors/document_read/quorum_pubkey-106-3603d12872604475619be7ad682f8339ad60debd43b3f430bd12d51eac268936.json
- packages/rs-sdk/tests/vectors/document_list_document_query/quorum_pubkey-106-3603d12872604475619be7ad682f8339ad60debd43b3f430bd12d51eac268936.json
…rename QueryContext to QuerySettings Two follow-ups from #3711 review triage. CMT-001: `SdkBuilder::with_initial_version` now mutates `self.version` directly instead of populating a parallel `initial_version: Option<&'static PlatformVersion>` field. The existing `(version, version_explicit)` pair already encodes "user-pinned vs auto-detect-seeded" — `version_explicit=true` means pinned, `false` means auto-detect with the atomic seeded from `self.version`. One fewer field, identical public API (`with_version` and `with_initial_version` keep their signatures and observable contracts). CMT-002: `QueryContext` is renamed to `QuerySettings` per shumkov's review note that the type reads more like settings than context. File moved (`query_context.rs` -> `query_settings.rs`), struct renamed, all ~30 internal callsites updated (query.rs, fetch*.rs, documents/document_query.rs, sdk.rs, tokens/*, types/*, rs-sdk-ffi). The `Sdk::query_context()` method also becomes `Sdk::query_settings()`. Breaking change for any consumer importing `dash_sdk::platform::QueryContext` — only landed in #3711, no external consumers yet. Gates: cargo fmt --all; cargo check --workspace; cargo clippy -p dash-sdk -p rs-sdk-ffi --tests -- -D warnings; cargo test -p dash-sdk --lib (133 passed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Five prior suggestions are still valid at ec137fdd9d9d44333fb3b5aaae54df27dc315bf8: the SDK still has inconsistent mock version seeding, still falls back to latest() for the first auto-detect request, still lets with_version() silently override with_initial_version(), still does not expose the initial-version seed through FFI, and still clones the request twice on the common fetch path. I did not confirm any additional latest-delta defects beyond those carried forward. One prior finding is resolved: the stale vector-regeneration command disappeared when packages/rs-sdk/tests/dpns_usernames.rs was removed in favor of the new test layout.
🟡 5 suggestion(s)
5 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/src/sdk.rs`:
- [SUGGESTION] lines 1119-1141: Mock SDK initialization still seeds two different protocol-version sources
`SdkBuilder::build()` still constructs `MockDashPlatformSdk` with `self.version` at line 1119, while the outer `Sdk.protocol_version` atomic is seeded from `self.initial_version` when auto-detect remains enabled at lines 1132-1140. That leaves a single mock SDK instance with one version used by `sdk.version()` / `sdk.query_context()` and a different version used by the mock proof parser. Tests that rely on `with_initial_version()` can therefore encode requests and parse proofs under different protocol versions.
In `packages/rs-sdk/src/sdk.rs`:
- [SUGGESTION] lines 489-515: Auto-detect mode still sends the newest wire shape on the first request
`Sdk::version()` still resolves an uninitialized protocol-version atomic to `PlatformVersion::latest()` at lines 489-491, and `Sdk::query_context()` forwards that value directly into version-dispatched encoders at lines 511-515. The actual network version is only learned after a response has already been parsed, so a freshly built auto-detect SDK still encodes its first document query using the binary's newest wire shape. The new docs and tests acknowledge this behavior, but the compatibility gap remains real for callers that do not manually seed `with_initial_version()`.
In `packages/rs-sdk/src/sdk.rs`:
- [SUGGESTION] lines 892-923: `with_version()` still silently overrides `with_initial_version()`
The builder still exposes `with_version()` and `with_initial_version()` as independent public setters, but `with_version()` flips `version_explicit` at lines 892-895 and the build path only consults `initial_version` when that flag is false. A caller can chain both methods and believe the SDK will bootstrap with an initial seed while remaining in auto-detect mode, but the explicit pin wins silently and disables auto-detect instead. That configuration conflict should be rejected or made unambiguous.
In `packages/rs-sdk-ffi/src/types.rs`:
- [SUGGESTION] lines 60-74: FFI callers still cannot set the initial protocol-version seed
`DashSDKConfig` still has no field for an initial protocol version, and the FFI constructors build `SdkBuilder` instances only from the network, addresses, and optional context provider before calling `build()`. That means the Rust-side mitigation for older-network first-request compatibility, `SdkBuilder::with_initial_version(...)`, is still unavailable to FFI consumers. Native Rust callers can avoid the bootstrap incompatibility, but Swift/C consumers cannot express the same configuration.
In `packages/rs-sdk/src/platform/fetch.rs`:
- [SUGGESTION] lines 177-179: The fetch trampoline still performs an extra clone on the non-versioned path
`fetch_with_metadata_and_proof()` still materializes a rich query with `query.query(&ctx)?` and then immediately converts it again with `owned_rich.query(&ctx)?` at lines 177-179. For the common case where `Self::Query = Self::Request`, that second step goes through the blanket `impl Query<T> for T`, which just clones the same request value. The same pattern is mirrored in `fetch_many_with_metadata_and_proof()`, so every non-versioned fetch still pays an avoidable allocation/copy introduced by the rich-query/request split.
Reviewed commit: ec137fd
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
…on Fetch clone path Two annotations responding to pastaclaw review on #3711 (sits logically on top of 407e57c). CMT-007 (deferred): `DashSDKConfig` in rs-sdk-ffi cannot express an initial protocol-version seed for talking to older networks. Tagged with a TODO pointing at CMT-005 — once the core SDK auto-detects PV before encoding the first request, FFI inherits the fix without an API surface change. CMT-008 (accept_risk): the `Self::Query = Self::Request` common-case path in `Fetch::fetch_with_metadata_and_proof` goes through the blanket `Query<T> for T` impl, which turns the encode step into a clone of the same owned request (~63 impls). Documented with an INTENTIONAL comment; a `fn encode_request_owned()` default would specialize the path away as future-perf work — uniform trait shape wins for now. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Thanks for the carry-forward review. Triage of the 6 findings from your
Resolved findings will fall off your next review pass; the three deferred/accepted items are documented in-source so future bisects don't lose the rationale. Triage performed via Claudius the Magnificent AI Agent. |
|
@coderabbitai review full |
|
✅ Actions performedFull review triggered. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Two carried-forward prior findings remain valid in packages/rs-sdk/src/sdk.rs: the default auto-detect path still encodes the first request using PlatformVersion::latest(), and the builder still lets with_version() silently keep auto-detect disabled even after with_initial_version(). One prior mock-version split was fixed by seeding both mock and outer SDK from self.version, but the latest delta still leaves a new mock-only split: proof fallback continues to read an immutable stored version after the outer SDK ratchets forward from response metadata.
🟡 3 suggestion(s)
1 additional finding(s)
suggestion: Mock proof fallback keeps using a frozen protocol version after the outer SDK auto-detects a newer one
packages/rs-sdk/src/mock/sdk.rs (lines 82-84)
MockDashPlatformSdk::version() always returns the immutable platform_version captured at construction time. That stale value is used by parse_proof_with_metadata() when the request is not satisfied from from_proof_expectations, while request encoding in the same mock instance already consults sdk.query_settings() and therefore uses the outer SDK's mutable protocol-version atomic. After a mock response metadata upgrade ratchets the outer SDK forward, later requests can be encoded with the new protocol version but decoded from proof with the old one, recreating the split-version behavior for dump-backed or fallback proof verification flows.
pub(crate) fn version(&self) -> &'static PlatformVersion {
if let Some(sdk) = self.sdk.load().as_ref() {
PlatformVersion::get(sdk.protocol_version_number()).unwrap_or(self.platform_version)
} else {
self.platform_version
}
}
🤖 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/src/sdk.rs`:
- [SUGGESTION] lines 489-515: Default auto-detect still emits the newest wire shape before the SDK has learned the network version
`Sdk::version()` still falls back to `PlatformVersion::latest()` whenever the stored protocol version does not map to a known historical version, and `query_settings()` feeds that value straight into version-dispatched encoders. In this PR, `SdkBuilder::default()` still seeds `self.version` to `latest()`, and `build()` now copies that value directly into the per-instance atomic, so a default auto-detect SDK still sends its first versioned request using the newest wire shape. The real network version is only learned after proof parsing succeeds in `verify_response_metadata()`, which means the first request to an older network can still fail before auto-detect has a chance to correct the SDK state.
In `packages/rs-sdk/src/sdk.rs`:
- [SUGGESTION] lines 891-916: `with_initial_version()` does not actually restore auto-detect after `with_version()`
`with_version()` sets `version_explicit = true`, but `with_initial_version()` only overwrites `self.version`. A caller that chains `.with_version(v2).with_initial_version(v1)` still builds a pinned SDK with auto-detect disabled, even though the later call site reads like an initial seed for auto-detect mode. That makes the public builder order-dependent in a non-obvious way and contradicts the method documentation that says `version_explicit` stays `false` for this path.
In `packages/rs-sdk/src/mock/sdk.rs`:
- [SUGGESTION] lines 82-84: Mock proof fallback keeps using a frozen protocol version after the outer SDK auto-detects a newer one
`MockDashPlatformSdk::version()` always returns the immutable `platform_version` captured at construction time. That stale value is used by `parse_proof_with_metadata()` when the request is not satisfied from `from_proof_expectations`, while request encoding in the same mock instance already consults `sdk.query_settings()` and therefore uses the outer SDK's mutable protocol-version atomic. After a mock response metadata upgrade ratchets the outer SDK forward, later requests can be encoded with the new protocol version but decoded from proof with the old one, recreating the split-version behavior for dump-backed or fallback proof verification flows.
Reviewed commit: 5d5952f
Inline review comment posting was rejected by GitHub, so I posted the findings body-only.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
packages/rs-sdk/tests/fetch/document_query_v0_v1.rs (1)
44-50: 💤 Low valueMemoize the leaked
PlatformVersionto avoid one allocation per test.
Box::leakhere intentionally produces a'staticreference, butv0_dispatch_version()is called from multiple tests, so each invocation leaks a freshPlatformVersionclone. Wrapping it in aOnceLockkeeps the'staticsemantics with a single allocation across the binary.♻️ Suggested refactor
fn v0_dispatch_version() -> &'static PlatformVersion { - let mut pv = PlatformVersion::latest().clone(); - pv.drive_abci.query.document_query.default_current_version = 0; - pv.drive_abci.query.document_query.min_version = 0; - pv.drive_abci.query.document_query.max_version = 0; - Box::leak(Box::new(pv)) + static V0_PV: std::sync::OnceLock<PlatformVersion> = std::sync::OnceLock::new(); + V0_PV.get_or_init(|| { + let mut pv = PlatformVersion::latest().clone(); + pv.drive_abci.query.document_query.default_current_version = 0; + pv.drive_abci.query.document_query.min_version = 0; + pv.drive_abci.query.document_query.max_version = 0; + pv + }) }🤖 Prompt for 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. In `@packages/rs-sdk/tests/fetch/document_query_v0_v1.rs` around lines 44 - 50, The current v0_dispatch_version() clones and Box::leak's a new PlatformVersion on every call causing repeated leaks; change it to memoize the leaked PlatformVersion using a static OnceLock (or Lazy) so the PlatformVersion is allocated and leaked only once: create a static OnceLock<Box<PlatformVersion>> (or OnceLock<&'static PlatformVersion>), initialize it on first call with the modified pv, and return the stored &'static PlatformVersion thereafter instead of leaking a new Box each invocation.packages/rs-sdk/src/platform/fetch_many.rs (1)
283-285: 💤 Low valueStale doc:
fetch_by_identifiersrequirement now binds toSelf::Query, notSelf::Request.The where-clause on line 291 was updated to
Vec<Identifier>: Query<<Self as FetchMany<K, O>>::Query>, but the## Requirementsblock still referencesSelf::Request.📝 Suggested doc update
/// ## Requirements /// - /// `Vec<Identifier>` must implement [Query] for [Self::Request]. + /// `Vec<Identifier>` must implement [Query] for [Self::Query].🤖 Prompt for 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. In `@packages/rs-sdk/src/platform/fetch_many.rs` around lines 283 - 285, The documentation for fetch_by_identifiers is stale: update the "## Requirements" block to reflect the new where-clause binding to Self::Query instead of Self::Request; specifically change the text that says "`Vec<Identifier>` must implement [Query] for [Self::Request]" to state that "`Vec<Identifier>` must implement [Query] for [Self::Query]" (matching the actual bound Vec<Identifier>: Query<<Self as FetchMany<K, O>>::Query> used in the implementation).packages/rs-sdk/src/platform/fetch.rs (1)
39-41: 💤 Low valueDoc comment is stale after the rich/wire split.
The trait-level docstring still says implementors must define only
Fetch::Request; with this change they must define bothFetch::QueryandFetch::Request.📝 Suggested doc update
-/// Implementors of this trait must define the associated [`Fetch::Request`] type. +/// Implementors of this trait must define the associated [`Fetch::Query`] (rich, +/// user-facing) and [`Fetch::Request`] (wire) types. When the two coincide, +/// declare them as the same type — associated-type defaults are nightly-only. /// All methods have default implementations; override [fetch_with_metadata_and_proof()](Fetch::fetch_with_metadata_and_proof) /// if custom fetch logic is needed, as other methods are convenience methods that call it.🤖 Prompt for 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. In `@packages/rs-sdk/src/platform/fetch.rs` around lines 39 - 41, Update the trait-level docstring to reflect the rich/wire split: change the statement that implementors must define only `Fetch::Request` to say they must define both associated types `Fetch::Query` and `Fetch::Request`; keep the note that methods have default implementations and that `fetch_with_metadata_and_proof()` is the primary method to override for custom fetch logic, and reference `Fetch::Query`, `Fetch::Request`, and `fetch_with_metadata_and_proof()` so readers know which symbols are required/related.packages/rs-sdk/src/mock/sdk.rs (1)
320-343: ⚡ Quick winFactor out the duplicated sdk-load + rich/wire derivation.
The three public mock setters (
expect_fetch,remove_fetch_expectation,expect_fetch_many) each re-loadself.sdk, encode the rich form, then re-encode the wire form, with an identical SEC-001 comment block repeated verbatim. The only thing that varies is the associated-type pair (Fetch::Query/Fetch::RequestvsFetchMany::Query/FetchMany::Request), which both reduce to the sameQuery<Rich>→Query<Wire>chain. Extracting a small private helper would remove ~30 lines of duplication, give the SEC-001 invariant one canonical home, and make any future change (e.g., switching to a non-panicking error path) a one-liner.♻️ Sketch of a private helper
/// Derive the (rich, wire) pair from a user query against the live Sdk. /// /// INTENTIONAL(SEC-001): test-harness fail-fast — encoder errors for V1-only /// DocumentQuery features against a V0 PlatformVersion crash the test setup /// loudly rather than silently propagate. fn derive_rich_wire<Q, Rich, Wire>(&self, query: Q) -> (Rich, Wire) where Q: Query<Rich>, Rich: Query<Wire>, { let sdk_guard = self.sdk.load(); let sdk = sdk_guard .as_ref() .expect("sdk must be set when creating mock"); let ctx = sdk.query_settings(); let rich: Rich = query.query(&ctx).expect("query must be correct"); let wire: Wire = rich.query(&ctx).expect("wire encoding must succeed"); (rich, wire) }Call sites then collapse to:
- let sdk_guard = self.sdk.load(); - let sdk = sdk_guard - .as_ref() - .expect("sdk must be set when creating mock"); - // INTENTIONAL(SEC-001): test-harness fail-fast — encoder errors for V1-only DocumentQuery features - // against a V0 PlatformVersion should crash the test setup loudly rather than silently propagate. - let rich: <O as Fetch>::Query = query - .query(&sdk.query_settings()) - .expect("query must be correct"); - let wire: <O as Fetch>::Request = rich - .query(&sdk.query_settings()) - .expect("wire encoding must succeed"); - self.expect(&rich, wire, object).await?; + let (rich, wire) = self.derive_rich_wire::<_, <O as Fetch>::Query, <O as Fetch>::Request>(query); + self.expect(&rich, wire, object).await?;Also applies to: 348-366, 398-435
🤖 Prompt for 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. In `@packages/rs-sdk/src/mock/sdk.rs` around lines 320 - 343, The three public mock setters expect_fetch, remove_fetch_expectation, and expect_fetch_many duplicate the sdk load + rich→wire query derivation (including the SEC-001 comment); factor that logic into a private helper (e.g., derive_rich_wire) that takes a Query and returns the (rich, wire) pair, preserving the SEC-001 comment in one canonical location, then replace each call site to call derive_rich_wire and use its returned rich and wire values; ensure generics match the associated types (Q: Query<Rich>, Rich: Query<Wire>) and keep the same expect(...) panic messages semantics.packages/rs-sdk/src/platform/documents/document_query.rs (2)
949-956: 💤 Low value
Query::queryignoresctx.prove; consider explicit handling.
prove: trueis hardcoded in bothencode_v0andencode_v1, butQuery::queryaccepts aQuerySettingswhoseprovefield is silently discarded here. The blanketQuery<T> for Timpl inquery.rsemits atracing::warn!when proofs are disabled (per the comment at lines 468–476), so this works at runtime. Still, an inline note here pointing back to that warning point would make the silent drop easier to follow for future maintainers.Separately,
self.clone()clones all clause vectors on every query (theArc<DataContract>clone is cheap, butwhere_clauses/order_by_clauses/havingare deep-cloned). The PR's triage notes already accept the extra clone as intentional, so this is not blocking.🤖 Prompt for 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. In `@packages/rs-sdk/src/platform/documents/document_query.rs` around lines 949 - 956, DocumentQuery::query currently drops QuerySettings.prove (ctx.prove) when constructing the GetDocumentsRequest via GetDocumentsRequest::try_from_platform_versioned(self.clone(), ctx.protocol_version); fix this by propagating the prove flag instead of ignoring it — either extend try_from_platform_versioned to accept the prove boolean and pass ctx.prove through, or call an existing encoder that accepts prove (so encode_v0/encode_v1 honor ctx.prove rather than hardcoding true); if you intentionally keep proofs always enabled, add a concise inline comment in DocumentQuery::query pointing to the blanket Query<T> impl’s tracing::warn (query.rs around the comment at lines ~468–476) to explain why ctx.prove is ignored.
556-570: 💤 Low value
encode_v0silently hardcodesprove: truewithout the V1 documentation.
encode_v1includes a multi-line comment (lines 468-475) explaining thatprove: trueis intentional andSdkBuilder::with_proofs(false)is a no-op for document fetch.encode_v0repeats the same hardcoding at line 568 without any explanation, so a future reader patching V0-specific behavior may not realize the rationale carries over. Add a brief reference to the V1 comment (or move the rationale to a shared helper/constant) so both encoders are documented consistently.🤖 Prompt for 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. In `@packages/rs-sdk/src/platform/documents/document_query.rs` around lines 556 - 570, The encode_v0 implementation currently hardcodes prove: true in the GetDocumentsRequest without explaining why; update encode_v0 to include a brief comment referencing the same rationale used in encode_v1 (the multi-line comment explaining that SdkBuilder::with_proofs(false) is a no-op for document fetch) or refactor the shared rationale into a single helper/constant used by both encode_v0 and encode_v1 (e.g., a shared comment or constant near GetDocumentsRequest creation) so future maintainers see the reason for prove: true; ensure references to encode_v0, encode_v1, and GetDocumentsRequest are used so the change is applied to both encoders.
🤖 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/rs-sdk/tests/vectors/document_read_no_contract/quorum_pubkey-106-1c97c16f6b472d0beadfb1bb83aadc440dc2be19879e09aeb342056f8f58e0fe.json`:
- Line 1: The fixture contains an invalid bare token on Line 1
("8791ef8bb5b32600a62a220d0afc2071ad64c9a41e97531fcd86de8ed7ef655425ce88687e65f5067d88f687c5a3abec");
wrap that value in valid JSON (e.g., replace the bare token with a JSON string:
"8791ef8bb5b32600a62a220d0afc2071ad64c9a41e97531fcd86de8ed7ef655425ce88687e65f5067d88f687c5a3abec"
or the expected JSON structure) so the fixture parses correctly.
In
`@packages/rs-sdk/tests/vectors/document_read_no_document/quorum_pubkey-106-1c97c16f6b472d0beadfb1bb83aadc440dc2be19879e09aeb342056f8f58e0fe.json`:
- Line 1: The fixture contains a bare hex string on line 1 which is not valid
JSON; update the test vector so the value is valid JSON (for example, wrap the
hex in a JSON string or place it in an object with a descriptive key) so the
loader can parse it deterministically—e.g., change the file content to a quoted
string or to an object like {"quorum_pubkey": "<hex>"} and ensure the fixture
name
(quorum_pubkey-106-1c97c16f6b472d0beadfb1bb83aadc440dc2be19879e09aeb342056f8f58e0fe)
matches the new JSON shape expected by the tests.
---
Nitpick comments:
In `@packages/rs-sdk/src/mock/sdk.rs`:
- Around line 320-343: The three public mock setters expect_fetch,
remove_fetch_expectation, and expect_fetch_many duplicate the sdk load +
rich→wire query derivation (including the SEC-001 comment); factor that logic
into a private helper (e.g., derive_rich_wire) that takes a Query and returns
the (rich, wire) pair, preserving the SEC-001 comment in one canonical location,
then replace each call site to call derive_rich_wire and use its returned rich
and wire values; ensure generics match the associated types (Q: Query<Rich>,
Rich: Query<Wire>) and keep the same expect(...) panic messages semantics.
In `@packages/rs-sdk/src/platform/documents/document_query.rs`:
- Around line 949-956: DocumentQuery::query currently drops QuerySettings.prove
(ctx.prove) when constructing the GetDocumentsRequest via
GetDocumentsRequest::try_from_platform_versioned(self.clone(),
ctx.protocol_version); fix this by propagating the prove flag instead of
ignoring it — either extend try_from_platform_versioned to accept the prove
boolean and pass ctx.prove through, or call an existing encoder that accepts
prove (so encode_v0/encode_v1 honor ctx.prove rather than hardcoding true); if
you intentionally keep proofs always enabled, add a concise inline comment in
DocumentQuery::query pointing to the blanket Query<T> impl’s tracing::warn
(query.rs around the comment at lines ~468–476) to explain why ctx.prove is
ignored.
- Around line 556-570: The encode_v0 implementation currently hardcodes prove:
true in the GetDocumentsRequest without explaining why; update encode_v0 to
include a brief comment referencing the same rationale used in encode_v1 (the
multi-line comment explaining that SdkBuilder::with_proofs(false) is a no-op for
document fetch) or refactor the shared rationale into a single helper/constant
used by both encode_v0 and encode_v1 (e.g., a shared comment or constant near
GetDocumentsRequest creation) so future maintainers see the reason for prove:
true; ensure references to encode_v0, encode_v1, and GetDocumentsRequest are
used so the change is applied to both encoders.
In `@packages/rs-sdk/src/platform/fetch_many.rs`:
- Around line 283-285: The documentation for fetch_by_identifiers is stale:
update the "## Requirements" block to reflect the new where-clause binding to
Self::Query instead of Self::Request; specifically change the text that says
"`Vec<Identifier>` must implement [Query] for [Self::Request]" to state that
"`Vec<Identifier>` must implement [Query] for [Self::Query]" (matching the
actual bound Vec<Identifier>: Query<<Self as FetchMany<K, O>>::Query> used in
the implementation).
In `@packages/rs-sdk/src/platform/fetch.rs`:
- Around line 39-41: Update the trait-level docstring to reflect the rich/wire
split: change the statement that implementors must define only `Fetch::Request`
to say they must define both associated types `Fetch::Query` and
`Fetch::Request`; keep the note that methods have default implementations and
that `fetch_with_metadata_and_proof()` is the primary method to override for
custom fetch logic, and reference `Fetch::Query`, `Fetch::Request`, and
`fetch_with_metadata_and_proof()` so readers know which symbols are
required/related.
In `@packages/rs-sdk/tests/fetch/document_query_v0_v1.rs`:
- Around line 44-50: The current v0_dispatch_version() clones and Box::leak's a
new PlatformVersion on every call causing repeated leaks; change it to memoize
the leaked PlatformVersion using a static OnceLock (or Lazy) so the
PlatformVersion is allocated and leaked only once: create a static
OnceLock<Box<PlatformVersion>> (or OnceLock<&'static PlatformVersion>),
initialize it on first call with the modified pv, and return the stored &'static
PlatformVersion thereafter instead of leaking a new Box each invocation.
🪄 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: 8a34366b-4e01-4bff-9a9a-089cfcc0368d
📒 Files selected for processing (73)
packages/rs-drive-abci/src/query/document_query/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v0.rspackages/rs-platform-version/src/version/v1.rspackages/rs-platform-version/src/version/v10.rspackages/rs-platform-version/src/version/v11.rspackages/rs-platform-version/src/version/v2.rspackages/rs-platform-version/src/version/v3.rspackages/rs-platform-version/src/version/v4.rspackages/rs-platform-version/src/version/v5.rspackages/rs-platform-version/src/version/v6.rspackages/rs-platform-version/src/version/v7.rspackages/rs-platform-version/src/version/v8.rspackages/rs-platform-version/src/version/v9.rspackages/rs-sdk-ffi/src/data_contract/queries/history.rspackages/rs-sdk-ffi/src/evonode/queries/proposed_epoch_blocks_by_ids.rspackages/rs-sdk-ffi/src/evonode/queries/proposed_epoch_blocks_by_range.rspackages/rs-sdk-ffi/src/types.rspackages/rs-sdk/src/mock/sdk.rspackages/rs-sdk/src/platform.rspackages/rs-sdk/src/platform/documents/document_average.rspackages/rs-sdk/src/platform/documents/document_count.rspackages/rs-sdk/src/platform/documents/document_query.rspackages/rs-sdk/src/platform/documents/document_split_averages.rspackages/rs-sdk/src/platform/documents/document_split_counts.rspackages/rs-sdk/src/platform/documents/document_split_sums.rspackages/rs-sdk/src/platform/documents/document_sum.rspackages/rs-sdk/src/platform/fetch.rspackages/rs-sdk/src/platform/fetch_many.rspackages/rs-sdk/src/platform/fetch_unproved.rspackages/rs-sdk/src/platform/group_actions.rspackages/rs-sdk/src/platform/identities_contract_keys_query.rspackages/rs-sdk/src/platform/query.rspackages/rs-sdk/src/platform/query_settings.rspackages/rs-sdk/src/platform/tokens/identity_token_balances.rspackages/rs-sdk/src/platform/tokens/token_contract_info.rspackages/rs-sdk/src/platform/tokens/token_info.rspackages/rs-sdk/src/platform/tokens/token_pre_programmed_distributions.rspackages/rs-sdk/src/platform/tokens/token_status.rspackages/rs-sdk/src/platform/tokens/token_total_supply.rspackages/rs-sdk/src/platform/types/epoch.rspackages/rs-sdk/src/platform/types/finalized_epoch.rspackages/rs-sdk/src/platform/types/identity.rspackages/rs-sdk/src/sdk.rspackages/rs-sdk/tests/fetch/common.rspackages/rs-sdk/tests/fetch/document_query_v0_v1.rspackages/rs-sdk/tests/fetch/mod.rspackages/rs-sdk/tests/fetch/tokens/token_contract_info.rspackages/rs-sdk/tests/vectors/document_list_document_query/msg_DocumentQuery_2a42b0afe6c6f58b02d8152142acec7d11a37410433366315d1103f81859344b.jsonpackages/rs-sdk/tests/vectors/document_list_document_query/msg_GetDataContractRequest_e87a2e6acef76975c30eb7272da71733fb6ad13495beb7ca1b6a6c4ceb30e0f7.jsonpackages/rs-sdk/tests/vectors/document_list_document_query/msg_GetDocumentsRequest_02213251619b5f2925ddad3e4e0a2380e57616a701bfbde50d048d23e0e09094.jsonpackages/rs-sdk/tests/vectors/document_list_document_query/quorum_pubkey-106-303c6a7b1e3aa25fa847ce5f6d6f39c3252081bcf291d632bbf38d43878d4777.jsonpackages/rs-sdk/tests/vectors/document_list_document_query/quorum_pubkey-106-3603d12872604475619be7ad682f8339ad60debd43b3f430bd12d51eac268936.jsonpackages/rs-sdk/tests/vectors/document_list_drive_query/msg_DocumentQuery_2fd2f7aebe5686d4e4179323b49e8920dea81c3f44b9549c238dcb82cccc9923.jsonpackages/rs-sdk/tests/vectors/document_list_drive_query/msg_GetDataContractRequest_e87a2e6acef76975c30eb7272da71733fb6ad13495beb7ca1b6a6c4ceb30e0f7.jsonpackages/rs-sdk/tests/vectors/document_list_drive_query/msg_GetDocumentsRequest_331458c78645abf6543c22204af22570c7fa61490e1c5c381c73a4a3e1bacc84.jsonpackages/rs-sdk/tests/vectors/document_list_drive_query/quorum_pubkey-106-303c6a7b1e3aa25fa847ce5f6d6f39c3252081bcf291d632bbf38d43878d4777.jsonpackages/rs-sdk/tests/vectors/document_list_drive_query/quorum_pubkey-106-3603d12872604475619be7ad682f8339ad60debd43b3f430bd12d51eac268936.jsonpackages/rs-sdk/tests/vectors/document_read/msg_DocumentQuery_2a42b0afe6c6f58b02d8152142acec7d11a37410433366315d1103f81859344b.jsonpackages/rs-sdk/tests/vectors/document_read/msg_DocumentQuery_4816e45a0ba02d69c291f23322410a8556ecf1145e99c87071cc8998a97d7ccb.jsonpackages/rs-sdk/tests/vectors/document_read/msg_GetDataContractRequest_e87a2e6acef76975c30eb7272da71733fb6ad13495beb7ca1b6a6c4ceb30e0f7.jsonpackages/rs-sdk/tests/vectors/document_read/msg_GetDocumentsRequest_02213251619b5f2925ddad3e4e0a2380e57616a701bfbde50d048d23e0e09094.jsonpackages/rs-sdk/tests/vectors/document_read/msg_GetDocumentsRequest_2496027c61fad2723bcec60b574423e3d41ce93dde03d5299b46b8e4858a0e3b.jsonpackages/rs-sdk/tests/vectors/document_read/quorum_pubkey-106-1c97c16f6b472d0beadfb1bb83aadc440dc2be19879e09aeb342056f8f58e0fe.jsonpackages/rs-sdk/tests/vectors/document_read/quorum_pubkey-106-3603d12872604475619be7ad682f8339ad60debd43b3f430bd12d51eac268936.jsonpackages/rs-sdk/tests/vectors/document_read_no_contract/msg_GetDataContractRequest_e4cf74168e03a40bd159451456b501c1ba166a2dd8f6efb31b0289dc011da983.jsonpackages/rs-sdk/tests/vectors/document_read_no_contract/quorum_pubkey-106-1c97c16f6b472d0beadfb1bb83aadc440dc2be19879e09aeb342056f8f58e0fe.jsonpackages/rs-sdk/tests/vectors/document_read_no_contract/quorum_pubkey-106-3603d12872604475619be7ad682f8339ad60debd43b3f430bd12d51eac268936.jsonpackages/rs-sdk/tests/vectors/document_read_no_document/msg_DocumentQuery_f531f69255dae40cef880290c177ff33d2448d30f2527374349b9f507b44c5c1.jsonpackages/rs-sdk/tests/vectors/document_read_no_document/msg_GetDataContractRequest_e87a2e6acef76975c30eb7272da71733fb6ad13495beb7ca1b6a6c4ceb30e0f7.jsonpackages/rs-sdk/tests/vectors/document_read_no_document/msg_GetDocumentsRequest_9256bdd6fc78e23f941d17d93b0230cf0f134e114c5e23ad7f08ed94c92f962f.jsonpackages/rs-sdk/tests/vectors/document_read_no_document/quorum_pubkey-106-1c97c16f6b472d0beadfb1bb83aadc440dc2be19879e09aeb342056f8f58e0fe.jsonpackages/rs-sdk/tests/vectors/document_read_no_document/quorum_pubkey-106-3603d12872604475619be7ad682f8339ad60debd43b3f430bd12d51eac268936.json
💤 Files with no reviewable changes (5)
- packages/rs-sdk/tests/vectors/document_read_no_contract/quorum_pubkey-106-3603d12872604475619be7ad682f8339ad60debd43b3f430bd12d51eac268936.json
- packages/rs-sdk/tests/vectors/document_list_document_query/quorum_pubkey-106-3603d12872604475619be7ad682f8339ad60debd43b3f430bd12d51eac268936.json
- packages/rs-sdk/tests/vectors/document_list_drive_query/quorum_pubkey-106-3603d12872604475619be7ad682f8339ad60debd43b3f430bd12d51eac268936.json
- packages/rs-sdk/tests/vectors/document_read_no_document/quorum_pubkey-106-3603d12872604475619be7ad682f8339ad60debd43b3f430bd12d51eac268936.json
- packages/rs-sdk/tests/vectors/document_read/quorum_pubkey-106-3603d12872604475619be7ad682f8339ad60debd43b3f430bd12d51eac268936.json
…bility + dedup mock setters (#3711) Triage round 2 follow-ups for PR #3711. CMT-001 (MEDIUM): MockDashPlatformSdk::version() no longer captures the PlatformVersion at construction time. It now delegates to the outer Sdk's auto-detect-aware atomic so request-encode (sdk.query_settings()) and proof-decode (parse_proof_with_metadata) read through a single source — mock ratchets from response metadata are visible to both paths. The constructor's frozen `version` argument is dropped. CMT-002 (MEDIUM): SdkBuilder::with_initial_version() now resets version_explicit=false so it re-enables auto-detect when called after with_version(). Builder calls are last-write-wins; the previous behaviour silently kept auto-detect disabled. CMT-005 (INFO): Extracted encode_rich_to_wire() helper in MockDashPlatformSdk. expect_fetch, remove_fetch_expectation and expect_fetch_many now share a single rich-then-wire encoding path with the SEC-001 fail-fast comment colocated on the helper. CMT-006 (INFO): Documented `prove: true` rationale in document_query::encode_v0() to match encode_v1(). Test coverage: - test_with_initial_version_after_with_version_restores_auto_detect asserts last-write-wins composability and auto-detect re-enablement. - test_mock_version_follows_outer_sdk_atomic asserts mock version() tracks outer Sdk ratchets driven by ResponseMetadata. Gates: cargo check --workspace, cargo clippy -p dash-sdk -- -D warnings, cargo fmt --all, cargo test -p dash-sdk --lib --features mocks (135 pass). Co-Authored-By: Claude Opus 4.7 (1M context) <842586+lklimek@users.noreply.github.com>
|
Round-2 triage results — now at
Remaining nitpicks on Triage performed via Claudius the Magnificent AI Agent. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
I re-reviewed current HEAD 30e3178c0. The V0/V1 encoder split and with_initial_version path look covered by the new tests, but I still see the main compatibility gap on the default/FFI SDK paths.
Findings:
-
Blocking — the default SDK path still sends the latest
getDocumentswire shape on the first request to an older network.SdkBuilderstill defaults toPlatformVersion::latest()(packages/rs-sdk/src/sdk.rs:762) andbuild()seedsprotocol_versiondirectly from that version (packages/rs-sdk/src/sdk.rs:1051-1054,packages/rs-sdk/src/sdk.rs:1121-1124).Sdk::query_settings()then passesself.version()into the query encoder (packages/rs-sdk/src/sdk.rs:511-515), andDocumentQueryselects V0 vs V1 purely from that platform version (packages/rs-sdk/src/platform/documents/document_query.rs:379-392,packages/rs-sdk/src/platform/documents/document_query.rs:953-958).The new
with_initial_version(...)escape hatch works, but it is opt-in (packages/rs-sdk/src/sdk.rs:891-920). The code now also documents the same bootstrap limitation: a fresh auto-detect SDK useslatest()until a response arrives, so the first request can fail before the SDK can correct itself (packages/rs-sdk/src/sdk.rs:351-365). That meansSdkBuilder::new(...).build()can still emit the V1getDocumentsrequest against a pre-v3.1 network, which is the incompatibility this PR is meant to fix. -
Blocking — FFI/Swift callers have no way to use the new initial-version workaround.
DashSDKConfighas no protocol-version seed field and explicitly notes that “FFI cannot express initial protocol-version seed for older-network interop” (packages/rs-sdk-ffi/src/types.rs:59-65). The FFI constructors build via.with_network(network)and then.build()without callingwith_initial_versionorwith_version(packages/rs-sdk-ffi/src/sdk.rs:119-153,packages/rs-sdk-ffi/src/sdk.rs:201-261,packages/rs-sdk-ffi/src/sdk.rs:357-424). Swift initializes the sameDashSDKConfigand callsdash_sdk_create_trusted(packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift:97-128).So Rust callers can opt in manually, but Swift/FFI consumers still cannot avoid the incompatible first request when targeting a pre-v3.1 network.
-
Non-blocking/compat note — the public Rust trait refactor is source-breaking for downstream implementors.
Query::querynow takes&selfplusQuerySettings(packages/rs-sdk/src/platform/query.rs:95-110), and bothFetchandFetchManynow require an additional associatedtype Query(packages/rs-sdk/src/platform/fetch.rs:60-88,packages/rs-sdk/src/platform/fetch_many.rs:87-115). That may be acceptable for this release window, but it should be intentional/documented because downstream implementations of these public traits will need source changes.
Validation run locally on this head:
cargo test -p dash-sdk document_query_v0_v1 --features offline-testing -- --nocapture— passed (10 passedin the targeted suite)cargo test -p dash-sdk test_with_initial_version_seeds_to_older_network_version --features offline-testing -- --nocapture— passed
Given the two default/FFI compatibility gaps above, I’m requesting changes.
Aligns parameter and binding names with the type rename in 407e57c (QueryContext → QuerySettings). Purely cosmetic; no behavior change. Inner `let settings = sdk.query_settings()` bindings shadowed an outer `settings: Option<RequestSettings>` fn parameter in fetch.rs, fetch_many.rs, and fetch_unproved.rs. Renamed the outer params to `request_settings` to disambiguate from the QuerySettings binding.
…ument_query_v0_v1 tests Follow-up to f91ea4a. macOS CI caught a `--tests`-only collision: `let settings = RequestSettings::default()` immediately shadowed by `let settings = QuerySettings { request_settings: &settings, ... }` works on the first construction (the inner `&settings` resolves to the prior `RequestSettings` binding) but breaks on the second `QuerySettings` literal in the same scope (E0308 — `&settings` now resolves to the freshly-shadowed `QuerySettings`). Renamed the outer `RequestSettings` binding to `request_settings` in all affected tests — same disambiguation pattern applied to fetch.rs/fetch_many.rs/fetch_unproved.rs in the previous commit. No behavior change. Files touched: - packages/rs-sdk/tests/fetch/common.rs - packages/rs-sdk/tests/fetch/document_query_v0_v1.rs (both tests) - packages/rs-sdk/tests/fetch/tokens/token_contract_info.rs (both tests)
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-forward prior findings: the core SDK still seeds auto-detect instances with PlatformVersion::latest(), so the first version-dispatched request can be encoded for the wrong wire version before response metadata is available. New findings in the latest delta: the FFI still exposes retry/timeout/verification config knobs that the Rust constructors never apply, so non-Rust callers get a silent configuration mismatch. The previously reported FFI lack of an initial protocol-version seed remains functionally true, but this PR now marks it as an explicit deferment and should not be re-raised on this review.
🟡 2 suggestion(s)
1 additional finding(s) omitted (not in diff).
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/src/sdk.rs`:
- [SUGGESTION] lines 489-515: Auto-detect SDKs still encode the first versioned request with `PlatformVersion::latest()`
Carried-forward prior finding: `Sdk::version()` still falls back to `PlatformVersion::latest()` when the per-instance atomic has not learned a network version yet, and `Sdk::query_settings()` passes that value directly into version-dispatched encoders. The builder still defaults `self.version` to `latest()` and seeds the runtime atomic from that value in `build()`, so a fresh auto-detect SDK has no distinct "unknown protocol version" bootstrap state. The documents encoder dispatches on `settings.protocol_version` in `packages/rs-sdk/src/platform/documents/document_query.rs:953-958`, which means the first `getDocuments` request to an older network can still be serialized in the newer wire shape and fail before `verify_response_metadata()` has any chance to ratchet the SDK to the real network version. `with_initial_version()` is only an opt-in mitigation; the default constructor path still preserves the incompatibility.
In `packages/rs-sdk-ffi/src/sdk.rs`:
- [SUGGESTION] lines 97-153: FFI SDK constructors ignore the transport settings they expose in `DashSDKConfig`
New finding in the latest delta: `DashSDKConfig` exposes `request_retry_count`, `request_timeout_ms`, and `skip_asset_lock_proof_verification` as ABI fields in `packages/rs-sdk-ffi/src/types.rs:64-79`, and both the FFI README and Swift wrapper actively populate those fields (`packages/rs-sdk-ffi/README.md:80-85`, `packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift:98-103`). But `dash_sdk_create()` builds with only `SdkBuilder::new...().with_network(...)`, `dash_sdk_create_extended()` repeats the same pattern, and `dash_sdk_create_trusted()` does not translate those fields either. None of those constructors call `SdkBuilder::with_settings()`, and `skip_asset_lock_proof_verification` is only copied through wrapper structs without affecting runtime behavior. As a result, C and Swift callers believe they are configuring retries, timeouts, or proof-verification behavior, but the Rust side silently drops those settings during SDK construction.
Reviewed commit: eac45e61
GitHub rejected inline review comments for this update, so the findings are posted body-only.
4 participants
Bug
Same as #3699:
SDK clients running against any network older than Dash Platform v3.1 (e.g. v3.0 testnet, currently the shared testnet) failed every
getDocumentsrequest with a server-side decode error and a cascade of side-effects:gRPC Unknown { message: "decoding error: could not decode data contracts query" }. The text says "data contracts" because v3.0'srs-drive-abci/src/query/document_query/mod.rs:31has a copy-paste typo in its error string — the failing handler is in fact the documents query.rs-dapi-clientsaw an unfamiliarUnknownand ban-listed each masternode that returned one. The pool drained toNoAvailableAddresseswithin seconds.Root cause
PR #3633 (
feat(platform): getDocuments v1) added a V1 wire shape forGetDocumentsRequestalongside the legacy V0 shape and bumpedDriveAbciQueryDocumentVersions::document_query.{max_version, default_current_version}from0to1inside the existingDRIVE_ABCI_QUERY_VERSIONS_V1bundle. No_V0bundle was forked. EveryPROTOCOL_VERSION_Nfrom PV_1 through PV_11 continued to binddrive_abci.query = DRIVE_ABCI_QUERY_VERSIONS_V1, retroactively claiming those historical PVs reported V1 doc-query — even thoughv3.0.0(PV_11) ships and only understands V0 wire.The SDK side compounded this:
TryFrom<DocumentQuery> for GetDocumentsRequesthardcodedPlatformVersion::latest()regardless of which network the SDK was talking to.Fix
Same behaviour as #3699; same five pieces, but the SDK-side architecture uses a
QueryContextborrow instead of&Sdk.Commit 1 —
DRIVE_ABCI_QUERY_VERSIONS_V0+ PV_V1..V11 re-pin + drive-abci typoDRIVE_ABCI_QUERY_VERSIONS_V0bundle (verbatim fork of_V1withdocument_querypinned to{min:0, max:0, default_current:0})."could not decode data contracts query"→"could not decode documents query"typo inrs-drive-abci/src/query/document_query/mod.rs.Commit 2 —
QueryContext+ Fetch::Query/Request split + PV-aware encoderNew
QueryContext<'a>type (packages/rs-sdk/src/platform/query_context.rs):Constructed via
Sdk::query_context()for normal callers, or directly in unit tests that exercise the encoder without anSdk. Includeswithout_proofs()for the FetchUnproved code path.Trait
Query<T>::querysignature changes fromfn query(self, prove: bool)tofn query(&self, ctx: &QueryContext<'_>).&selfinstead ofselfso callers don't have to clone before the encoder fires; the encoder itself clones internally where the wire type requires owned data.SdkBuilder::with_initial_version(&PlatformVersion)seeds the SDK's protocol-version atomic at boot without disabling auto-detect. Lets a wallet boot against v3.0 testnet (PV_11 → V0 wire) and continue working as testnet upgrades.TryFromPlatformVersioned<DocumentQuery> for GetDocumentsRequestis the PV-aware wire encoder. Dispatches onplatform_version.drive_abci.query.document_query.default_current_version. V1-only features (group_by,having, count/sum/avg projections) returnError::Configagainst a V0 dispatch.Fetch::Query/Fetch::Requestsplit:Fetch::Queryis the user-facing rich query thatFromProofbinds to;Fetch::Requestis the wire-encoded proto. For documents:type Query = DocumentQuery; type Request = GetDocumentsRequest;. Every other operation getstype Query = Self::Request— one extra line.§ Approach comparison vs #3699
#3699 routes
&Sdkdirectly into the encoder. This PR routes a small&QueryContextborrow instead.&Sdk)QueryContext)Sdk::new_mock()QueryContextdirectlySdk(transport, mock cache, nonce cache, context provider, …)QueryContext<'a>borrowCopy/Clonestruct, three pub fieldsquery_context.rs, all.query(...)call sites grow&sdk.query_context())&Sdkget used for?" — answer: protocol version onlySdkQueryContextThe concrete payoff is in
encoder_dispatches_v0_via_query_context_without_sdk— a new test added in commit 2 that exercises the V0/V1 dispatch by constructing aQueryContextdirectly, noSdk::new_mock()required. The same shape lets any encoder be unit-tested in isolation.Honest cost
QueryContext<'a>), one extra accessor onSdk(query_context()), one lifetime parameter to thread through.let ctx = sdk.query_context(); query.query(&ctx)?instead ofquery.query(true, sdk)?. Three extra lexical tokens.Queryimpls that today dofn query(self, prove: bool)need both a&selfswitch and actx.proveread. fix(rs-sdk,drive-abci): SDK emits incompatible getDocuments wire against pre-v3.1 networks #3699's breaking surface requires the&self/sdkswitch — comparable churn, slightly different shape.If the reviewer reads "
&Sdkis fine, we don't need a new type" — that's a defensible position. The arg forQueryContextis the encoder-in-isolation testability and the named-type self-documentation; the arg for&Sdkis "we already pass&Sdkeverywhere else, this is just one more callsite." Both are valid; this PR exists so reviewers can pick.Test plan
New test exercising the no-
Sdkencoder path:Four document offline tests carry the same
#[ignore]pin as #3699'sfedfce8396(mock cache key changed by the Fetch::Query/Request split); vectors will be regenerated against testnet in a follow-up.Breaking changes
Target branch is
v3.1-dev. Out-of-tree consumers will need:Query<T>::query(): wasfn query(self, prove: bool), nowfn query(&self, ctx: &QueryContext<'_>). External impls switch to&selfand pullprovefromctx.prove.Fetch: newtype Queryassociated type. External impls addtype Query = Self::Request;unless they want a rich/wire split.DocumentQueryno longer implementsTransportRequest. UseDocument::fetch(sdk, query)orGetDocumentsRequest::try_from_platform_versioned(query, sdk.version())?for the explicit transport request.SdkBuilder::with_initial_versionis purely additive.Related
&Sdkinstead ofQueryContext).feat/rs-platform-wallet-e2e).Checklist
For repository code-owners and reviewers only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Update — chosen approach
PR #3699 (the
&Sdkvariant) is now closed in favor of this PR.Two follow-ups landed on top of the original two commits:
1ca35c831ftest(rs-sdk): re-enable document fetch tests after QueryContext refactor— the four document fetch tests intests/fetch/document.rshad#[ignore]while waiting on vector regeneration. Removed; vectors will be regenerated manually and any failures show up in CI rather than silently passing.d7f486922efix(rs-sdk-ffi): migrate remaining Query::query impls to QueryContext— three impls inrs-sdk-ffi(data_contract/queries/history.rs,evonode/queries/proposed_epoch_blocks_by_ids.rs,evonode/queries/proposed_epoch_blocks_by_range.rs) were missed in the original refactor; signatures now take&QueryContext<'_>.