Skip to content

fix(sdk): implementation of std::marker::Send is not general enough#3376

Closed
shumkov wants to merge 2 commits intov3.1-devfrom
refactor/sdk-rpitit-fetch-traits
Closed

fix(sdk): implementation of std::marker::Send is not general enough#3376
shumkov wants to merge 2 commits intov3.1-devfrom
refactor/sdk-rpitit-fetch-traits

Conversation

@shumkov
Copy link
Copy Markdown
Collaborator

@shumkov shumkov commented Mar 17, 2026

Issue being fixed or feature implemented

Fixes the HRTB Send inference failure (rust-lang/rust#96865) that prevents SDK consumers from using tokio::spawn with SDK operations when 28+ Fetch impl types exist.

This issue was introduced when shielded pool support added 8 new Fetch impls (20 -> 28), pushing the Rust compiler's type inference past its limit. The same bug affects other projects like tonic.

What was done?

Replaced #[async_trait] with explicit Pin<Box<dyn Future<Output = ...> + Send + 'a>> returns on:

  • Fetch trait (5 methods) — fetch.rs
  • FetchMany trait (5 methods) — fetch_many.rs
  • FetchUnproved trait (2 methods) — fetch_unproved.rs
  • BroadcastStateTransition trait (3 methods) — broadcast.rs
  • 21 transition traits (PutDocument, PutIdentity, TransferAddressFunds, ShieldFunds, WithdrawFromIdentity, etc.)

Why this works

With #[async_trait], the compiler still analyzes the trait method bodies to prove Send when monomorphizing across all impl types. With explicit BoxFuture, the compiler sees Box<dyn Future + Send> at the trait boundary and stops — Send is proven by construction via the boxed trait object.

No performance regression

#[async_trait] already performed the exact same boxing (Pin<Box<dyn Future + Send>>). We just make it explicit instead of macro-generated, removing the macro's involvement in the compiler's inference chain.

How Has This Been Tested?

  • cargo check -p dash-sdk — passes
  • cargo check -p dpp --all-features -p drive-abci -p platform-wallet -p platform-wallet-ffi — passes
  • cargo check -p wasm-sdk --target wasm32-unknown-unknown — passes (WASM unaffected)
  • dash-evo-tool compiles with tokio::spawn (no spawn_blocking workaround) against this branch — verified locally

Breaking Changes

None

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 17, 2026

Note

Reviews paused

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

Use the following commands to manage reviews:

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

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Default implementations in three platform fetch traits were refactored to compute the transport request up-front and delegate the retry/execution/parsing control flow to new module-level async helper functions. No public API signatures were changed.

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 non-public async helper functions (fetch_with_metadata_and_proof_impl, fetch_many_with_metadata_and_proof_impl, fetch_unproved_with_settings_impl). Each helper: accepts a concrete request, applies resolved RequestSettings, runs the retry loop, executes the transport request, traces/logs responses, parses into (object/option, ResponseMetadata, Proof) or (Option<T>, ResponseMetadata), maps parse errors into ExecutionError with address/retries, and returns the final ExecutionResponse/result. Adjusted generics and added explicit trait/bound requirements; removed #[async_trait::async_trait] from one public impl in fetch_many.rs. No public signatures changed.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

Poem

🐰 I hopped through code with nimble feet,

Pulled helpers out to keep things neat,
Requests computed, retries in line,
Proofs and metadata parsed just fine,
A tiny rabbit cheers: async is sweet!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The PR title mentions fixing Send implementation generality, but the actual primary change is refactoring fetch helpers to extract retry+closure logic from default async trait methods. The title is partially related but does not capture the main refactoring objective. Consider using a more descriptive title that reflects the primary refactoring: 'refactor(sdk): extract fetch helpers to fix HRTB Send inference' better captures the core change.
✅ Passed checks (2 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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 unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/sdk-rpitit-fetch-traits

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

❤️ Share

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

@github-actions github-actions bot added this to the v3.1.0 milestone Mar 17, 2026
@shumkov shumkov self-assigned this Mar 17, 2026
@shumkov shumkov moved this to In review / testing in Platform team Mar 17, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

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
Copy link
Copy Markdown
Collaborator Author

shumkov commented Mar 17, 2026

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
Copy link
Copy Markdown

codecov bot commented Mar 17, 2026

Codecov Report

❌ Patch coverage is 20.13652% with 234 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.93%. Comparing base (94cefb3) to head (f7ca355).
⚠️ Report is 1 commits behind head on v3.1-dev.

Files with missing lines Patch % Lines
...ckages/rs-sdk/src/platform/transition/broadcast.rs 0.00% 113 Missing ⚠️
packages/rs-sdk/src/platform/fetch_unproved.rs 29.16% 17 Missing ⚠️
...ges/rs-sdk/src/platform/transition/put_document.rs 0.00% 16 Missing ⚠️
...ges/rs-sdk/src/platform/transition/put_identity.rs 0.00% 8 Missing ⚠️
packages/rs-sdk/src/platform/fetch_many.rs 75.86% 7 Missing ⚠️
...tform/transition/top_up_identity_from_addresses.rs 0.00% 7 Missing ⚠️
...ackages/rs-sdk/src/platform/transition/waitable.rs 0.00% 7 Missing ⚠️
...ackages/rs-sdk/src/platform/transition/transfer.rs 0.00% 6 Missing ⚠️
packages/rs-sdk/src/platform/transition/vote.rs 0.00% 6 Missing ⚠️
...k/src/platform/transition/transfer_to_addresses.rs 0.00% 5 Missing ⚠️
... and 14 more

❌ Your patch status has failed because the patch coverage (20.13%) 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    #3376      +/-   ##
============================================
- Coverage     79.93%   79.93%   -0.01%     
============================================
  Files          2861     2861              
  Lines        280236   280236              
============================================
- Hits         223995   223993       -2     
- Misses        56241    56243       +2     
Components Coverage Δ
dpp 71.82% <ø> (ø)
drive 82.30% <ø> (ø)
drive-abci 86.67% <ø> (ø)
sdk 36.55% <20.13%> (ø)
dapi-client 79.06% <ø> (ø)
platform-version ∅ <ø> (∅)
platform-value 83.36% <ø> (ø)
platform-wallet 76.09% <ø> (ø)
drive-proof-verifier 55.26% <ø> (ø)
🚀 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
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 17, 2026

✅ DashSDKFFI.xcframework built for this PR.

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

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

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.

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

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

@shumkov shumkov force-pushed the refactor/sdk-rpitit-fetch-traits branch from 63c716a to 28d088e Compare March 17, 2026 18:38
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 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

@shumkov shumkov changed the title refactor(sdk): convert Fetch traits from async_trait to RPITIT refactor(sdk): extract fetch helpers to fix HRTB Send inference Mar 17, 2026
Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

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
Copy link
Copy Markdown
Contributor

lklimek commented Mar 19, 2026

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

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@lklimek lklimek left a comment

Choose a reason for hiding this comment

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

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

lklimek added a commit to dashpay/dash-evo-tool that referenced this pull request Mar 20, 2026
* feat: integrate platform mempool support

Bump dash-sdk to platform commit 6d9efa97b which includes
rust-dashcore mempool support (MempoolManager with bloom filter
monitoring) and SDK RPITIT refactor.

Key changes:
- Configure SPV client with BloomFilter mempool strategy
- Update process_mempool_transaction() call signature
- Adapt to Network::Dash -> Network::Mainnet rename
- Add DB migration 29 to update stored "dash" network strings
- Add Send-asserting wrappers for backend task futures to work
  around RPITIT HRTB Send inference limitations in the SDK

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* style: fix formatting in settings.rs

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* refactor: remove unsafe assert_send workaround

Rebase platform dependency onto aa86b74f (before PR #3376 which
paradoxically broke Send inference) so the compiler can natively
prove the backend task future is Send.

Changes:
- Update dash-sdk rev to 2414799b7 (mempool bump on clean base)
- Remove assert_send() transmute and all _send() wrapper methods
- Revert app.rs to call run_backend_task() directly

This commit is self-contained — revert it to restore the workaround
if a future platform update reintroduces the HRTB regression.

See: dashpay/platform#3376
See: rust-lang/rust#100013

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* test: add trace-level timing instrumentation to E2E harness

Add structured tracing to wait helpers and create_funded_test_wallet()
for diagnosing IS lock timing issues. All new logs are trace-level
(enable with RUST_LOG=backend_e2e=trace) except the final setup
summary which logs at info.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* chore: bump platform rev for bloom filter rebuild API

Update dash-sdk to platform commit d9de0fd2 which includes
rust-dashcore c451a1c6 adding notify_wallet_addresses_changed()
for triggering mempool bloom filter rebuilds.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* feat: trigger bloom filter rebuild on wallet/address changes

Call notify_wallet_addresses_changed() after loading a new wallet into
SPV and after registering DashPay contact addresses. This ensures the
mempool bloom filter includes newly added addresses so incoming
transactions are detected immediately rather than waiting for the next
block or SPV reconnect.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(test): let RUST_LOG override default trace level in E2E harness

The hardcoded .add_directive("backend_e2e=info") was overriding
RUST_LOG=backend_e2e=trace. Change to use RUST_LOG as primary source
with info as fallback when the env var is not set.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* revert: use original rust-dashcore 1af96dd without filter rebuild additions

Reverts bloom filter rebuild API additions (notify_wallet_addresses_changed)
that were added in commit eb948ae. The upstream MempoolManager has bugs
(masternode sync cascade blocking activation) that make these calls
ineffective. Failing tests proving these bugs are tracked in
dashpay/rust-dashcore#567.

Platform rev pinned to 2af8cac6 which uses rust-dashcore 1af96dd (PR #558).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* chore: update Cargo.lock for platform rev 2af8cac6

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* chore: bump platform rev to be2082b3c (rust-dashcore b03252af)

Updates platform to be2082b3c which pins rust-dashcore b03252af,
bringing bloom filter staleness detection via monitor_revision polling.

Adapts imports for key-wallet-manager → key-wallet crate merge
(upstream rust-dashcore #503).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(test): wait for SPV sync completion before broadcasting in E2E harness

The E2E harness was broadcasting funding transactions before the
MempoolManager's bloom filter was loaded to peers. Peers never relayed
IS locks for those txs because the filter didn't exist yet.

- Add wait_for_spv_running() that polls ConnectionStatus::spv_status()
  until SpvStatus::Running (fires after SyncComplete + bloom filter built)
- Call it in BackendTestContext::init() after spendable balance is confirmed
- Add 200ms sleep in create_funded_test_wallet() after wallet appears in
  SPV to let the mempool manager tick rebuild the bloom filter

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(test): use SpvManager status directly instead of ConnectionStatus

ConnectionStatus.spv_status is only updated from the UI frame loop,
which doesn't run in the E2E test harness. Read from SpvManager's
internal RwLock instead.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* chore: bump platform rev to 93556aa4b (rust-dashcore 450f72f)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* refactor: push SPV status to ConnectionStatus from event handlers

Eliminate polling of SpvManager.status() from the UI frame loop.
SpvManager event handlers now push status, peer count, and error
updates directly to ConnectionStatus via new setter methods, making
SPV status visible to headless/test code without a UI polling cycle.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs: document ConnectionStatus as single source of truth for connection health

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(db): replace wallet_addresses with identity_token_balances in migration 29

wallet_addresses table has no `network` column, causing migration 29
(rename "dash" → "mainnet") to fail with "no such column: network".
Replace with identity_token_balances which does have the column.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(db): include table name in migration 29 error messages

If a table update fails during the dash→mainnet rename, the error
now identifies which table caused the failure.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(spv): push Starting/Stopping status to ConnectionStatus immediately

write_status() updated the internal RwLock but did not push to
ConnectionStatus. The UI reads ConnectionStatus.spv_status() to
show the SPV Sync Status section, so it stayed hidden until
spawn_progress_watcher fired its first async tick.

Now start() and stop() push status to ConnectionStatus immediately,
matching the pattern used in all other status transitions.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* refactor(spv): centralize ConnectionStatus push in write_status()

Move the ConnectionStatus push into write_status() so every status
transition automatically propagates to the UI. Remove redundant
manual pushes from start(), stop(), run_spv_loop(), and run_client().

Async event handlers (spawn_progress_watcher, spawn_sync_event_handler)
still push directly because they write to the RwLock without going
through write_status() (they hold Arc<RwLock>, not &self).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: address remaining PR review comments

- fix(ui): testnet MNListDiff fallback now uses Network::Testnet (was
  incorrectly Network::Mainnet in file-not-found path)
- fix(spv): progress watcher no longer clears last_error when a second
  error arrives with error_msg=None (preserves first error)
- fix(spv): clear spv_no_peers_since when SPV transitions to non-active
  state, preventing stale "no peers" tooltip warnings
- docs: clarify ConnectionStatus scope — health is push-based, detailed
  sync progress may still be read from SpvManager

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* chore: add key_wallet and mempool_filter to default tracing filter

These crates were renamed/added upstream in rust-dashcore 450f72f
and were missing from the default EnvFilter, hiding debug logs.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(ui): show actionable SPV error details in banner and Settings

- Error banner now says "Go to Settings" and attaches the actual error
  message via with_details() for the collapsible details panel
- Settings SPV status label shows the error message instead of just "Error"
- Add public spv_last_error() getter on ConnectionStatus (DRY)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(db): deduplicate wallet transactions by txid before insert

With mempool support enabled, upstream wallet_transaction_history()
can return the same txid twice — as both a mempool entry (no height)
and a confirmed entry (with height). This caused a UNIQUE constraint
violation on (seed_hash, txid, network).

Deduplicate in replace_wallet_transactions(), preferring the confirmed
version over unconfirmed when both exist for the same txid.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* feat(wallet): add TransactionStatus enum for transaction lifecycle tracking

Add TransactionStatus (Unconfirmed/InstantSendLocked/Confirmed/ChainLocked)
to WalletTransaction model and database schema.

- New TransactionStatus enum with from_height() heuristic for inferring
  status from block height (confirmed vs unconfirmed)
- Migration 30: adds `status` column to wallet_transactions table
  (default 2=Confirmed for existing rows)
- INSERT OR REPLACE handles duplicate txids from mempool + block
- UI shows status labels: "Pending", "⚡ InstantSend", "Confirmed @n",
  "🔒 ChainLocked @n"

InstantSendLocked and ChainLocked require upstream support
(rust-dashcore#569) — currently only Unconfirmed/Confirmed are inferred.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* refactor(db): consolidate migrations 29+30 into single migration 29

Both migrations were introduced in this PR — no need for users to run
them separately. Migration 29 now does both: rename network "dash" to
"mainnet" AND add the transaction status column.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(ui): throttle system theme detection to prevent white flash during sync

When using the "System" color theme, dark_light::detect() was called
every frame. During SPV sync (high repaint frequency), transient wrong
values caused brief theme flips visible as white flashes.

Changes:
- Cache the resolved theme in AppState and only re-poll the OS every 2s
- Guard font initialization with AtomicBool so set_fonts runs once
- Immediately resolve and apply theme on preference change

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Revert "fix(ui): throttle system theme detection to prevent white flash during sync"

This reverts commit 7f822bb.

* fix(db): default transaction status to Unconfirmed for new inserts

The table-creation DEFAULT was Confirmed (2), which is wrong — transactions
should be unconfirmed until proven otherwise. The migration ALTER TABLE
correctly keeps DEFAULT 2 for pre-existing rows (all from confirmed RPC).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: address PR review comments (migration idempotency, status field, poison handling)

- Guard ALTER TABLE in migration 30 with pragma_table_info check to
  prevent duplicate column crash on DBs upgrading from version <14
- Make is_confirmed() authoritative via TransactionStatus instead of height
- Consistent recover-from-poison in ConnectionStatus setters
- Add TODO for write_last_error() ConnectionStatus push consolidation
- Add maintenance comments for progress watcher write_status bypass
- Add tracing::debug for unknown Network variant fallback

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
@lklimek lklimek added the priority: high High priority — address soon label Mar 23, 2026
@shumkov shumkov requested a review from lklimek March 26, 2026 07:15
@shumkov shumkov force-pushed the refactor/sdk-rpitit-fetch-traits branch from 3286fcf to 2e38979 Compare March 30, 2026 12:19
@thepastaclaw
Copy link
Copy Markdown
Collaborator

thepastaclaw commented Mar 30, 2026

Review Gate

Commit: f7ca3554

  • Debounce: 472m ago (need 30m)

  • CI checks: build failure: Rust workspace tests / Tests (macOS)

  • CodeRabbit review: comment found

  • Run review now (check to override)

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
packages/rs-sdk/src/platform/fetch_many.rs (1)

283-318: Don't keep &R alive inside the retry future.

The let request = &request; at Line 283 keeps &R across both awaits, so this future is Send only if R: Sync. That hidden strengthening is easy to miss and is the same “reference inside async state” shape the downstream Send discussion is already struggling with. Capturing an owned/cloned request in the closure instead keeps this helper aligned with the goal of reducing Send inference friction. If TransportRequest does not already imply Clone, please make that bound explicit here as well.

♻️ Suggested reshape
-    let request = &request;
-
-    let fut = |settings: RequestSettings| async move {
+    let fut = {
+        let request = request.clone();
+        move |settings: RequestSettings| {
+            let request = request.clone();
+            async move {
@@
-        sdk.parse_proof_with_metadata_and_proof::<R, O>(request.clone(), response)
+        sdk.parse_proof_with_metadata_and_proof::<R, O>(request, response)
             .await
@@
-    };
+            }
+        }
+    };
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/rs-sdk/src/platform/fetch_many.rs` around lines 283 - 318, The
closure assigned to fut captures let request = &request; which keeps a reference
to R across awaits making the future require R: Sync; change the closure to
capture an owned/cloned request instead (e.g., move a cloned request into the
async closure) so no long-lived &R is stored in the async state; ensure the
trait bounds include Clone for the request type (or TransportRequest: Clone) if
needed and update uses of request.clone() accordingly (symbols to edit: the
request binding, the fut closure, and any call sites like request.clone() passed
into execute and sdk.parse_proof_with_metadata_and_proof).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/rs-sdk/src/platform/fetch_many.rs`:
- Around line 283-318: The closure assigned to fut captures let request =
&request; which keeps a reference to R across awaits making the future require
R: Sync; change the closure to capture an owned/cloned request instead (e.g.,
move a cloned request into the async closure) so no long-lived &R is stored in
the async state; ensure the trait bounds include Clone for the request type (or
TransportRequest: Clone) if needed and update uses of request.clone()
accordingly (symbols to edit: the request binding, the fut closure, and any call
sites like request.clone() passed into execute and
sdk.parse_proof_with_metadata_and_proof).

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 48c48137-2067-4a78-acdd-7558107235d5

📥 Commits

Reviewing files that changed from the base of the PR and between 28d088e and 2e38979.

📒 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
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/rs-sdk/src/platform/fetch.rs
  • packages/rs-sdk/src/platform/fetch_unproved.rs

@shumkov shumkov force-pushed the refactor/sdk-rpitit-fetch-traits branch 2 times, most recently from ba4e032 to 0373efc Compare March 30, 2026 17:50
@shumkov shumkov changed the title refactor(sdk): extract fetch helpers to fix HRTB Send inference refactor(sdk): replace #[async_trait] with explicit BoxFuture to fix HRTB Send inference Mar 30, 2026
@shumkov shumkov changed the title refactor(sdk): replace #[async_trait] with explicit BoxFuture to fix HRTB Send inference fix(sdk): resolve 'Send is not general enough' error for async SDK consumers Mar 30, 2026
@shumkov shumkov force-pushed the refactor/sdk-rpitit-fetch-traits branch 2 times, most recently from 83e889a to dece16b Compare March 30, 2026 18:03
…traits

Replace #[async_trait] with explicit Pin<Box<dyn Future + Send + 'a>>
returns on Fetch, FetchMany, FetchUnproved, BroadcastStateTransition,
and all 21 transition traits in the SDK.

## Problem

The Rust compiler has a known HRTB Send inference bug
(rust-lang/rust#96865) that prevents it from proving async futures are
Send when the number of generic trait implementations exceeds a
threshold. With 28 Fetch impl types (after adding shielded pool
support), any consumer using tokio::spawn with SDK operations gets:

  error: implementation of Send is not general enough

This forced consumers (e.g. dash-evo-tool) to use spawn_blocking +
block_on as a workaround, wasting threads and blocking the async
executor.

## Solution

By returning BoxFuture explicitly instead of relying on #[async_trait],
the compiler sees Box<dyn Future + Send> and stops analyzing the inner
state machine. Send is proven by construction via the boxed trait
object. This is the same boxing that #[async_trait] performed — we just
make it explicit so the compiler's inference engine is not involved.

## Impact

- ALL SDK consumers can use tokio::spawn directly
- No spawn_blocking workaround needed
- No unsafe AssertSend wrapper needed
- No performance regression (same boxing as #[async_trait])
- WASM builds unaffected (verified)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@shumkov shumkov force-pushed the refactor/sdk-rpitit-fetch-traits branch from dece16b to 5b64a4a Compare March 30, 2026 18:05
@shumkov shumkov changed the title fix(sdk): resolve 'Send is not general enough' error for async SDK consumers fix(sdk): implementation of std::marker::Send is not general enough Mar 30, 2026
…nference

Introduce dash-sdk-macros crate with #[box_async] proc macro — a drop-in
replacement for #[async_trait] that avoids the HRTB Send inference bug
(rust-lang/rust#96865).

The macro performs the same async fn -> BoxFuture transformation but uses
explicit per-reference lifetimes instead of async_trait's unified
'async_trait lifetime, avoiding the problematic inference path.

Applied to: Fetch, FetchMany, FetchUnproved, FetchCurrent,
BroadcastStateTransition, and all 21 transition traits.

SDK compiles. Evo-tool Send issue persists due to deeper compiler
limitation — needs further investigation.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
"packages/rs-platform-wallet-ffi",
"packages/rs-platform-encryption",
"packages/wasm-sdk",
"packages/rs-sdk-macros",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why new crate, not add to rs-dash-platform-macros ?

@shumkov
Copy link
Copy Markdown
Collaborator Author

shumkov commented Mar 31, 2026

Closing this PR — the approach doesn't solve the consumer-side Send inference issue.

Investigation summary

The HRTB Send inference bug (rust-lang/rust#96865, rust-lang/rust#100013) triggers when 28+ Fetch impl types exist in the SDK. We tried multiple approaches:

  1. This PR (extract to standalone fns) — SDK compiles, but consumers like dash-evo-tool still fail
  2. Explicit BoxFuture returns — Same result
  3. Custom #[box_async] proc macro — Same result
  4. Upgrade async-trait to 0.1.89 — Doesn't help

The issue is in the Rust compiler's type inference, not in our code. All types ARE genuinely Send — the compiler just can't prove it through deep generic hierarchies with 28+ implementations.

The pragmatic workaround is spawn_blocking + block_on on the consumer side (dash-evo-tool), which is safe and correct. See dashpay/dash-evo-tool#806 for the evo-tool update to latest v3.1-dev.

Full investigation details: rust-lang/rust#96865

@shumkov shumkov closed this Mar 31, 2026
@github-project-automation github-project-automation bot moved this from In review / testing to Done in Platform team Mar 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: high High priority — address soon

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants