refactor(sdk): extract fetch helpers to fix HRTB Send inference

[shumkov]

Issue being fixed or feature implemented

The default implementations of Fetch::fetch_with_metadata_and_proof, FetchMany::fetch_many_with_metadata_and_proof, and FetchUnproved::fetch_unproved_with_settings use #[async_trait] which desugars to
Pin<Box<dyn Future + Send + 'life>>. When downstream code calls these methods inside tokio::spawn, the compiler must prove that the resulting future is Send for all lifetimes (higher-ranked trait bound).
It fails because the default method body contains an inlined retry closure capturing &Sdk, creating a complex async state machine whose Send impl the compiler cannot prove universally.

This was previously masked because Document had a custom fetch_many override with a simpler body. When that override was removed in #3179 (it referenced the deleted parse_proof method),
Document::fetch_many fell through to the default trait implementation, exposing the HRTB failure for any caller using tokio::spawn.

More #3361

What was done?

Extracted the retry+closure logic from the three core default trait methods into standalone async fn helpers. The default methods become thin wrappers that call query.query() and delegate to the helper.

This breaks the HRTB Send inference chain because the standalone helper is a concrete async fn (not a trait default method), so the compiler proves Send at the helper's definition site where all types are
concrete.

For each trait:

• Fetch::fetch_with_metadata_and_proof → delegates to fetch_with_metadata_and_proof_impl()
• FetchMany::fetch_many_with_metadata_and_proof → delegates to fetch_many_with_metadata_and_proof_impl()
• FetchUnproved::fetch_unproved_with_settings → delegates to fetch_unproved_with_settings_impl()

Also removed stale #[async_trait::async_trait] from impl FetchMany<Identifier, Contenders> for ContenderWithSerializedDocument (bare impl with no async methods).

No call sites changed. No turbofish/UFCS needed. #[async_trait] stays on all traits.

How Has This Been Tested?

• cargo check -p dash-sdk — passes
• cargo check -p dash-sdk --tests — passes (all test files compile without changes)
• cargo clippy -p dash-sdk -- -D warnings — zero warnings
• cargo fmt --check --all — passes

Breaking Changes

None. The trait signatures are unchanged. The extracted helpers are private to the module.

Checklist:

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

For repository code-owners and collaborators only

• I have assigned this pull request to a milestone

Summary by CodeRabbit

• Refactor
  • Refactored internal fetch logic to extract retry mechanisms into standalone helper functions, improving code maintainability and organization.
  • No changes to public APIs or user-facing functionality.

[coderabbitai]

📝 Walkthrough

Walkthrough

Three files in the platform module extract private async helper functions to encapsulate retry logic and request execution. These refactoring changes improve code organization and address Send/lifetime constraints without modifying public API signatures.

Changes

Cohort / File(s)

Summary

Platform Fetch Helpers packages/rs-sdk/src/platform/fetch.rs, packages/rs-sdk/src/platform/fetch_many.rs, packages/rs-sdk/src/platform/fetch_unproved.rs

Extracted private async helper functions (fetch_with_metadata_and_proof_impl, fetch_many_with_metadata_and_proof_impl, fetch_unproved_with_settings_impl) to encapsulate retry logic and request execution. Delegation pattern applied to improve code organization and resolve Send/lifetime constraints. Removed async_trait attribute in one public impl block to align with new Future-based approach. Public APIs unchanged.

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 Helpers extracted with async care,
Private functions floating through the air,
Retry logic bundled up so tight,
Send constraints resolved just right!

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check	✅ Passed	The title 'refactor(sdk): extract fetch helpers to fix HRTB Send inference' directly describes the core change—extracting fetch helpers to resolve Send trait bound inference issues—and aligns with the main refactoring objective.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

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

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch refactor/sdk-rpitit-fetch-traits

📝 Coding Plan

• Generate coding plan for human review comments

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

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/src/platform/fetch_unproved.rs (1)

145-166: ⚠️ Potential issue | 🟡 Minor

Remove unnecessary #[async_trait] annotation.

The fn maybe_from_unproved_with_metadata method is synchronous and returns Result<...> directly, not a Future. The #[async_trait] attribute is used to convert async methods into boxed futures and is unnecessary here. This is inconsistent with the CurrentQuorumsInfo implementation of the same trait, which correctly omits the annotation. Remove the annotation.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/rs-sdk/src/platform/fetch_unproved.rs` around lines 145 - 166, The
impl block for FromUnproved<EvoNode> incorrectly includes the
#[async_trait::async_trait] attribute even though
maybe_from_unproved_with_metadata is synchronous; remove the
#[async_trait::async_trait] annotation from the impl so the impl block for
FromUnproved<EvoNode> (and its method maybe_from_unproved_with_metadata) matches
the synchronous pattern used by CurrentQuorumsInfo and does not attempt to treat
the method as async. Ensure the impl compiles without the attribute.

🧹 Nitpick comments (1)

packages/rs-sdk/src/platform/fetch.rs (1)

257-260: Minor: Redundant match arm conversion.

The match block converts Some(item) to Some(item.into()), but since item is already of type S (the same type expected in the output), the .into() call appears to be a no-op identity conversion. This is harmless but slightly redundant.

♻️ Optional simplification

         match object {
-            Some(item) => Ok((item.into(), response_metadata, proof)),
+            Some(item) => Ok((Some(item), response_metadata, proof)),
             None => Ok((None, response_metadata, proof)),
         }

Or more concisely:

-        match object {
-            Some(item) => Ok((item.into(), response_metadata, proof)),
-            None => Ok((None, response_metadata, proof)),
-        }
+        Ok((object, response_metadata, proof))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/rs-sdk/src/platform/fetch.rs` around lines 257 - 260, The match on
`object` is doing an unnecessary identity conversion for `Some(item)` by calling
`item.into()`; replace the match with a direct mapped value (e.g., transform
`object` to the desired output once using `.map(Into::into)` or equivalent) and
return Ok((mapped_object, response_metadata, proof)) instead—update the code
around the `object`, `response_metadata`, and `proof` return to use the
simplified expression.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@packages/rs-sdk/src/platform/fetch_unproved.rs`:
- Around line 145-166: The impl block for FromUnproved<EvoNode> incorrectly
includes the #[async_trait::async_trait] attribute even though
maybe_from_unproved_with_metadata is synchronous; remove the
#[async_trait::async_trait] annotation from the impl so the impl block for
FromUnproved<EvoNode> (and its method maybe_from_unproved_with_metadata) matches
the synchronous pattern used by CurrentQuorumsInfo and does not attempt to treat
the method as async. Ensure the impl compiles without the attribute.

---

Nitpick comments:
In `@packages/rs-sdk/src/platform/fetch.rs`:
- Around line 257-260: The match on `object` is doing an unnecessary identity
conversion for `Some(item)` by calling `item.into()`; replace the match with a
direct mapped value (e.g., transform `object` to the desired output once using
`.map(Into::into)` or equivalent) and return Ok((mapped_object,
response_metadata, proof)) instead—update the code around the `object`,
`response_metadata`, and `proof` return to use the simplified expression.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ceb6761b-a8b8-4357-989f-06bf1aab57c3

📥 Commits

Reviewing files that changed from the base of the PR and between 5fe1484 and 62382e1.

📒 Files selected for processing (3)

• packages/rs-sdk/src/platform/fetch.rs
• packages/rs-sdk/src/platform/fetch_many.rs
• packages/rs-sdk/src/platform/fetch_unproved.rs

[thepastaclaw]

Code Review

Clean, well-executed RPITIT migration that correctly replaces #[async_trait] with native return-position impl Trait on Fetch, FetchMany, and FetchUnproved. The transformation is semantically equivalent — the explicit Self: Send bound was already implicitly enforced by #[async_trait] (without ?Send). The UFCS changes in wasm-sdk are mechanically necessary due to a known Rust inference limitation with RPITIT. No blocking issues found.

Reviewed commit: 6555aea

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

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-sdk/src/platform/fetch.rs`:
- [SUGGESTION] lines 95-167: Add a compile-time regression test proving fetch futures are Send
  This PR's stated goal is to fix Send inference (issue #3361) so fetch futures can be spawned. While the `+ Send` bound on the return type is a compile-time assertion, a dedicated regression test would guard against future regressions more explicitly. A simple `fn assert_send<T: Send>(_: &T) {}` or `tokio::spawn(Identity::fetch(&sdk, id))` in a test module would catch any re-introduction of non-Send captures.

In `packages/wasm-sdk/src/queries/identity.rs`:
- [SUGGESTION] lines 673-678: UFCS required even for single-impl types due to Query<T> ambiguity, not trait ambiguity
  Types likeIdentityBalance have only one FetchMany impl, yet UFCS is required. The root cause is that `Vec<Identifier>` implements `Query<T>` for multiple `T` values, and RPITIT's opaque return type blocks the inference path that async_trait's boxed future allowed. This is a known Rust limitation (rust-lang/rust#113495). Not actionable now, but worth noting in a code comment for future maintainers who may wonder why UFCS is needed for seemingly unambiguous cases.

[shumkov]

Addressed review comments

Compile-time regression test for Send — Good suggestion. The + Send bound on the RPITIT return types is already a compile-time assertion (the code won't compile if the future isn't Send). But a dedicated test would be more explicit. Will add in a follow-up.

UFCS line length (token.rs lines 689/736) — These are auto-formatted by cargo fmt and pass --check. Breaking them further would fight the formatter. Leaving as-is.

Redundant match/into in fetch_impl — Good catch, this is pre-existing. The match { Some(item) => Ok((item.into(), ...)), None => Ok((None, ...)) } is equivalent to Ok((object, ...)). Simplified in the latest push.

UFCS required for single-impl types — Correct analysis. This is a known Rust limitation (rust-lang/rust#113495) where RPITIT's opaque return type blocks Query inference. Added a comment in the UFCS call sites explaining why.

Remove stale #[async_trait] from fetch_unproved.rs — Done in latest push. The FromUnproved trait is a regular trait, not async_trait.

rs-sdk-ffi type inference — Fixed in latest push. Added UFCS turbofish to all 8 affected files in rs-sdk-ffi.

Second address.clone() — Pre-existing minor optimization. Will address in a follow-up cleanup.

[codecov]

Codecov Report

❌ Patch coverage is 79.69925% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.92%. Comparing base (2bb561b) to head (3286fcf).

Files with missing lines	Patch %	Lines
packages/rs-sdk/src/platform/fetch_unproved.rs	58.13%	18 Missing ⚠️
packages/rs-sdk/src/platform/fetch_many.rs	88.37%	5 Missing ⚠️
packages/rs-sdk/src/platform/fetch.rs	91.48%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           v3.1-dev    #3376   +/-   ##
=========================================
  Coverage     79.91%   79.92%           
=========================================
  Files          2861     2861           
  Lines        279648   279700   +52     
=========================================
+ Hits         223493   223541   +48     
- Misses        56155    56159    +4     

Components	Coverage Δ
dpp	71.82% <ø> (ø)
drive	82.26% <ø> (ø)
drive-abci	86.68% <ø> (ø)
sdk	37.13% <79.69%> (+0.57%)	⬆️
dapi-client	79.06% <ø> (ø)
platform-version	∅ <ø> (∅)
platform-value	83.36% <ø> (ø)
platform-wallet	76.55% <ø> (ø)
drive-proof-verifier	55.43% <ø> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

✅ DashSDKFFI.xcframework built for this PR.

• Workflow run: https://github.com/dashpay/platform/actions/runs/23294198416
• Artifacts: DashSDKFFI.xcframework (folder), DashSDKFFI.xcframework.zip, xc_checksum.txt

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

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

Xcode manual integration:

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

[thepastaclaw]

Code Review

Clean incremental push completing the RPITIT migration. The three follow-up commits add necessary UFCS disambiguation across rs-sdk-ffi (7 files), wasm-sdk (3 files), and SDK tests (3 files), remove a stale #[async_trait] attribute, and fix an unused variable warning. All six review agents (general, rust-quality, ffi-engineer × Claude/Codex) confirmed correctness with no blocking issues. The UFCS type parameters correctly match the intended FetchMany<K, O> implementations.

Reviewed commit: 63c716a

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/rs-sdk/src/platform/fetch_many.rs`:
- Around line 210-211: The FetchMany trait still uses
#[async_trait::async_trait] and async fn methods; remove the attribute and
rewrite each async method signature (e.g., fetch_many_with_metadata_and_proof,
along with other async methods declared on FetchMany) to return a native future
type like -> impl std::future::Future<Output = Result<...>> + Send, update all
implementors to return the appropriate async block or future (instead of relying
on async_trait), and adjust helper functions such as
fetch_many_with_metadata_and_proof_impl and the call site that delegates via let
request = query.query(sdk.prove())?;
fetch_many_with_metadata_and_proof_impl(sdk, request, settings).await so they
match the new return types (i.e., return the future rather than an async fn),
ensuring all trait method signatures and implementations use impl Future + Send
consistently.

In `@packages/rs-sdk/src/platform/fetch.rs`:
- Around line 161-162: Remove the #[async_trait] usage on the Fetch trait and
convert it to RPITIT by adding an associated generic Future type and changing
the async method into a generic-returning fn: add type Future<'a>: Future<Output
= Result<...>> + Send + 'a; change async fn fetch(...) -> Result<...> to fn
fetch<'a>(&'a self, sdk: &'a Sdk, query: Query, settings: Settings) ->
Self::Future<'a>; then update all impls of Fetch (the concrete impl struct(s)
that define fetch) to define the associated type and return their async work as
a boxed/pinned future (e.g., Box::pin(async move { ... })) or another concrete
Future type; also update callers like fetch_with_metadata_and_proof_impl and
places that call fetch (the code building request via query.query(sdk.prove())
and awaiting fetch) to accept the new Future-returning signature (no
#[async_trait] wrapping).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: af2a6e18-cd79-4e2a-801b-5c3b8c43fc82

📥 Commits

Reviewing files that changed from the base of the PR and between 63c716a and 28d088e.

📒 Files selected for processing (3)

• packages/rs-sdk/src/platform/fetch.rs
• packages/rs-sdk/src/platform/fetch_many.rs
• packages/rs-sdk/src/platform/fetch_unproved.rs

[thepastaclaw]

Code Review

Clean mechanical refactoring that extracts retry+closure logic from three #[async_trait] trait default methods into standalone async fn helpers, fixing HRTB Send inference failures with tokio::spawn. The code is unchanged — just moved from inline to free functions. All trait bounds correctly mirror their originals. The stale #[async_trait] removal on ContenderWithSerializedDocument is correct. No new findings on this push.

Reviewed commit: 28d088e

🟡 1 suggestion(s)

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-sdk/src/platform/fetch.rs`:
- [SUGGESTION] lines 218-272: No compile-time regression test for Send on fetched futures
  The entire purpose of this refactoring is to ensure futures from `Fetch::fetch`, `FetchMany::fetch_many`, and `FetchUnproved::fetch_unproved` are `Send` for `tokio::spawn`. However, there's no compile-time assertion to catch a regression if someone later moves the closure back inline or adds a non-Send capture.

A zero-cost compile-time test would make the invariant explicit:
```rust
#[allow(dead_code)]
fn assert_send<T: Send>(_: T) {}

#[allow(dead_code)]
fn fetch_future_is_send(sdk: &Sdk) {
    assert_send(Identity::fetch(sdk, Identifier::new([0u8; 32])));
}

</details>

[lklimek]

Downstream consumer report: Send bounds still broken for tokio::spawn callers

Hey folks. Spent some quality time integrating dash-evo-tool against this branch and I have good news and less good news.

Good news: The refactor works exactly as described. The extracted helper functions break the HRTB chain inside the SDK itself. Clean approach, no API breakage, nicely done.

Less good news: The Send bound problem resurfaces the moment a downstream consumer composes these trait method calls inside tokio::spawn. The RPITIT return types are opaque, and when they get captured in a larger async block, the compiler still cannot prove the aggregate future is Send.

What breaks

In dash-evo-tool, run_backend_task() spawns tokio tasks that call Fetch::fetch(), FetchMany::fetch_many(), etc. After updating to this branch, I get:

error: implementation of `Send` is not general enough
  --> src/context.rs:XXX:YY
   |
   = note: `Send` is implemented for `&'0 Sdk`, but not for `&'1 Sdk`
   = note: required because it appears within the type `impl Future<Output = ...>`
   = note: required for `{async block@...}` to implement `Send`
   = note: required by a bound in `tokio::spawn`

Same story for &DataContract references captured across await points. The SDK's internal HRTB is resolved, but the opaque future types returned to consumers still defeat the compiler's Send inference when composed.

Workaround applied

I ended up writing this in dash-evo-tool to unblock integration:

/// SAFETY: The future produced by `run_backend_task_impl` is Send in practice —
/// all captured state (`Arc<AppContext>`, `BackendTask`, owned channel sender) is Send.
/// The compiler cannot prove this due to RPITIT opaque types from dash-sdk traits
/// defeating HRTB Send inference when composed in async blocks.
unsafe fn assert_send<T>(
    future: impl Future<Output = T>,
) -> impl Future<Output = T> + Send {
    // Transmute to add Send bound that the compiler can't infer
    let boxed: Pin<Box<dyn Future<Output = T>>> = Box::pin(future);
    std::mem::transmute::<
        Pin<Box<dyn Future<Output = T>>>,
        Pin<Box<dyn Future<Output = T> + Send>>,
    >(boxed)
}

It works. I do not love it. Every downstream consumer of the SDK will hit the same wall and need a similar escape hatch.

Suggestion

Would it be worth providing Send-safe entry points from the SDK side? A few options:

1. Box the futures in the trait methods themselves — #[async_trait] was already doing this implicitly via Pin<Box<dyn Future + Send>>. The RPITIT migration removed that boxing, which is what exposed the problem. Adding explicit boxing back (even selectively) would restore Send provability for consumers.

2. Provide _send() companion methods that return Pin<Box<dyn Future<Output = T> + Send + '_>> — consumers who need tokio::spawn compatibility use these, others use the zero-cost RPITIT versions.

3. Add a SendFetch / SendFetchMany wrapper at the SDK level that does the boxing once, so consumers don't each independently reach for unsafe transmute.

Option 1 is the least disruptive since it's what #[async_trait] was giving you before. The performance cost of one Box per fetch call is negligible compared to the network round-trip.

See dashpay/dash-evo-tool#776 for the full integration context.

---

To be clear, this PR is a solid step forward — it correctly identifies and fixes the HRTB problem at the trait-default-method level. The issue is just that RPITIT's opacity propagates the same class of problem to the consumer side. Figured it's better to flag this now while the refactor is in flight rather than after merge.

_{🤖 Co-authored by Claudius the Magnificent AI Agent}

[thepastaclaw]

Incremental Review (push 3286fcfc — merge of v3.1-dev, no PR-file changes)

Pure merge of v3.1-dev into the PR branch. The three modified SDK files (fetch.rs, fetch_many.rs, fetch_unproved.rs) are unchanged from the prior approved review at 28d088e0.

Prior APPROVE stands — the helper extraction approach correctly resolves the HRTB Send inference issue.

[lklimek]

Dash Evo tool doesn't build with this pr, see dashpay/dash-evo-tool#777