feat(dpp)!: convert Signer trait to async#3492
Open
QuantumExplorer wants to merge 4 commits intov3.1-devfrom
Open
feat(dpp)!: convert Signer trait to async#3492QuantumExplorer wants to merge 4 commits intov3.1-devfrom
QuantumExplorer wants to merge 4 commits intov3.1-devfrom
Conversation
Convert the Signer<K> trait in rs-dpp to an async trait via #[async_trait], enabling true async signing for iOS HSM / Secure Enclave / keychain backends without blocking any thread during biometric prompts or hardware calls. - Signer::sign and Signer::sign_create_witness are now async; can_sign_with stays sync - All 9 concrete Signer implementations converted (SimpleSigner, SingleKeySigner, DummySigner, 2x TestAddressSigner, IdentitySignerWasm, PlatformAddressSignerWasm, VTableSigner, AddressSigner) - Propagate async through rs-dpp state-transition builders, rs-sdk transition builders, rs-drive-abci tests, wasm-sdk, and strategy-tests - Iterator closures calling signer.sign(...) rewritten as sequential for loops (order-preserving, borrow-friendly) - Teach #[stack_size] macro to wrap async fn bodies in a tokio current-thread runtime inside the spawned thread so integration tests keep working BREAKING CHANGE: rs-sdk-ffi Signer vtable redesigned with a completion- callback pattern. The old synchronous SignCallback + free_result_callback pair is replaced with SignAsyncCallback + SignCompletionCallback, so the iOS side can return from the sign callback immediately and invoke the Rust completion function later (from any thread) when the HSM/keychain response arrives. No thread is blocked during the wait. Migration notes for iOS consumers: - dash_sdk_signer_create no longer takes a free_result_callback - The sign callback signature changes: iOS stashes (completion_ctx, completion) and invokes completion(ctx, sig, len, err) when ready - SingleKeySigner now routes through a native (non-callback) path with no C-callback bounce, via signer_handle_from_single_key Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedToo many files! This PR contains 177 files, which is 27 over the limit of 150. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (177)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Collaborator
|
⏳ Review in progress (commit 9169382) |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3492 +/- ##
============================================
+ Coverage 84.83% 84.93% +0.10%
============================================
Files 2476 2476
Lines 267733 269916 +2183
============================================
+ Hits 227123 229246 +2123
- Misses 40610 40670 +60
🚀 New features to boost your workflow:
|
Contributor
|
✅ 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: "264aac33d5439625d3c8ddc6884d05a5452cb854e2ff40526754324f6fe1cf87"
)Xcode manual integration:
|
QuantumExplorer
commented
Apr 15, 2026
Member
Author
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
-
packages/rs-sdk-ffi/src/signer.rs:266-299: ifsign_asyncreturns without ever invokingcompletion, thecompletion_ctxbox is leaked andrx.awaitnever resolves. TheErr(_recv_err)branch here does not catch that case, so this hangs permanently instead of surfacing the intended protocol error. That makes cancellation / forgotten completion paths wedge the signer future forever. -
packages/rs-sdk/src/sdk.rs:316-327: auto-detect still parses the first proof withPlatformVersion::latest()and only learns the real protocol version after successful proof parsing. On an older network where proof interpretation differs fromlatest(), the first proof-backed request can fail before the SDK ever bootstraps itself, so the advertised auto-detect behavior is incomplete.
…llback If the FFI signer's sign_async returns without ever invoking completion, the oneshot Sender sits inside a Box handed to the C side and never drops, so the previous `rx.await` match arm `Err(_recv_err)` could never fire and the awaiting async fn would hang forever. Bound the wait with tokio::time::timeout (5 min — generous enough for biometric + HSM round-trips). On timeout we surface a recoverable ProtocolError and leak the one Box<Sender> rather than risk a use-after-free if the caller eventually does invoke completion. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
lklimek
reviewed
Apr 15, 2026
|
|
||
| __pycache__/ | ||
| .claude/worktrees/ | ||
| .claude/ |
Contributor
There was a problem hiding this comment.
did you ignore whole .claude by purpose ? Don't we want some shared claude config between all devs, to unify our performance?
| quote! { | ||
| builder | ||
| .spawn(move || { | ||
| ::tokio::runtime::Builder::new_current_thread() |
| use std::sync::Arc; | ||
|
|
||
| pub trait Signer<K>: Sync + Debug { | ||
| #[async_trait] |
Contributor
There was a problem hiding this comment.
Ensure we still want to use async_trtait vs ? Why in lib.rs we have #![allow(async_fn_in_trait)]?
| //#![deny(missing_docs)] | ||
| #![allow(dead_code)] | ||
| #![allow(clippy::result_large_err)] | ||
| #![allow(async_fn_in_trait)] |
Contributor
There was a problem hiding this comment.
We use legacy async_trtait everywhere, not sure if #![allow(async_fn_in_trait)] is intentional.
| let key_slice = std::slice::from_raw_parts(private_key, 32); | ||
| let mut key_array = Zeroizing::new([0u8; 32]); | ||
| key_array.copy_from_slice(key_slice); | ||
|
|
||
| // network won't matter here | ||
| // Network doesn't matter for signing purposes. |
Contributor
There was a problem hiding this comment.
Hardcoding the mainnet here is not a good approach, as it relies on SignleKeySigner internals. Either pass correct network, or remove the network arg from SingleKeySigner::new_from_slice()
| // point, so we bridge by spinning up a current-thread runtime just | ||
| // long enough to drive the signing future. This is safe because | ||
| // SingleKeySigner's async fn is non-blocking (pure CPU work). | ||
| let runtime = match tokio::runtime::Builder::new_current_thread() |
Contributor
There was a problem hiding this comment.
Double-check that this actually works. It risks "starting runtime within runtime" error. See also #3490 .
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Convert
Signer<K>inrs-dppto an async trait via#[async_trait], enabling true async signing for iOS HSM / Secure Enclave / keychain backends without blocking any thread during biometric prompts or hardware calls.Trait shape:
Scope
Signerimplementations converted (SimpleSigner,SingleKeySigner,DummySigner, 2×TestAddressSigner,IdentitySignerWasm,PlatformAddressSignerWasm,VTableSigner,AddressSigner)rs-dppstate-transition builders,rs-sdktransition builders,rs-drive-abcitests,wasm-sdk, andstrategy-tests— 177 files changedsigner.sign(...)rewritten as sequentialforloops (order-preserving, borrow-friendly — notry_join_all)#[stack_size]macro taught to wrapasync fnbodies in a tokio current-thread runtime inside the spawned thread so integration tests keep workingBreaking change: rs-sdk-ffi Signer vtable redesign
The old synchronous
SignCallback+free_result_callbackpair is replaced with a completion-callback pattern:The iOS side returns from
sign_asyncimmediately, stashes(completion_ctx, completion), and invokescompletion(ctx, sig, len, err)later — possibly from a different thread, possibly minutes later after a biometric prompt. The Rust side awaits atokio::sync::oneshot::Receiverin the meantime. No thread is blocked during the wait.iOS migration required (follow-up PR on swift-sdk)
The iOS Swift code in `packages/swift-sdk/` is left unmodified in this PR; updating it is explicit follow-up work.
Test plan
🤖 Generated with Claude Code