fix(sdk): run blocking Swift SDK Platform queries off the caller's actor - #5146
romchornyi wants to merge 4 commits into
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 21 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (30)
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 |
|
✅ Final review complete — no blockers (commit 610c083) · triage: normal |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The PR correctly moves blocking Platform FFI reads off the caller’s actor and preserves FFI ownership, but the newly concurrent availability checks introduce a stale-result race in the existing registration UI. An older lookup can overwrite the availability state for the username currently being edited, enabling an incorrect registration attempt or displaying the wrong status.
🟡 1 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Ignore stale username availability completions
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/RegisterNameView.swift:361-375
dpnsCheckAvailability now runs concurrently, so checkAvailabilityAutomatically() can have multiple requests in flight. The completion unconditionally assigns isAvailable and clears isChecking, while the text-field handler only invalidates the debounce timer and does not invalidate an already-running request. For example, a slow request for available name A can complete after the user changes to taken name B and B’s request returns false, overwriting B’s state with true and enabling the Register Name button. Capture the queried normalized name together with a request generation/token, and apply success and error results only when they still correspond to the current request and username. Add a regression test that completes two availability requests out of order.
source: gpt-6-astra (phase2-reviewer: general, architecture-layering, ffi-engineer, security-auditor)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: ffi-engineer); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — The changes introduce nontrivial Swift actor isolation, concurrent FFI execution, and handle-lifetime management across several query helpers, but do not modify any critical surface such as consensus, funds movement, cryptography, network deserialization, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— architecture-layering (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— ffi-engineer (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort high); agentphase2-reviewer
🤖 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/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/RegisterNameView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/RegisterNameView.swift:361-375: Ignore stale username availability completions
`dpnsCheckAvailability` now runs concurrently, so `checkAvailabilityAutomatically()` can have multiple requests in flight. The completion unconditionally assigns `isAvailable` and clears `isChecking`, while the text-field handler only invalidates the debounce timer and does not invalidate an already-running request. For example, a slow request for available name A can complete after the user changes to taken name B and B’s request returns `false`, overwriting B’s state with `true` and enabling the Register Name button. Capture the queried normalized name together with a request generation/token, and apply success and error results only when they still correspond to the current request and username. Add a regression test that completes two availability requests out of order.
|
Stale availability results ( |
|
/self-reviewed |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The off-main query implementation preserves SDK lifetime, FFI result ownership, and caller-actor responsiveness, and the original stale-completion race is fixed. One in-scope logic issue remains in the registration form: cancelling a lookup leaves the deduplication marker set, so editing away and back to the same name can permanently suppress the replacement lookup.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: ffi-engineer); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — The diff introduces nontrivial Swift actor-isolation, concurrent FFI execution, lifetime management, and stale availability-result handling, but does not itself change any qualifying critical surface. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— architecture-layering (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— ffi-engineer (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort high); agentphase2-reviewer,gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort high); agentphase2-reviewer
🤖 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/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/RegisterNameView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/RegisterNameView.swift:174-177: Clear the checked-name marker when cancelling its lookup
When the username changes, `availabilityGate.cancel()` invalidates the active request but `lastCheckedName` remains unchanged. If a lookup for `alice` is in flight, the user edits to another name, then edits back to `alice` before the debounce fires, the second edit sees `normalizedUsername == lastCheckedName` and schedules no new lookup at line 185. The original completion is rejected by the cancelled gate, leaving availability unset and registration disabled for the displayed name. Reset the deduplication marker when invalidating the lookup, or make the scheduling condition distinguish a cancelled request from an accepted result. Add a regression test covering edit-away/edit-back before the debounce fires.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Audit exclusive SDKWrapper borrows across concurrent FFI calls — Existing FFI entry points construct exclusive
&mut SDKWrapperreferences from shared SDK handles even for operations that appear read-only. This predates the current PR and no concrete memory-corruption issue was established here, but the borrowing model should be audited before further concurrent FFI expansion.- Follow-up: Track separately in a maintainer-requested FFI safety audit; replace unnecessary exclusive references with shared access or establish synchronization for genuinely exclusive operations.
|
/self-reviewed |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Both prior username-availability findings are fixed. The new executor nevertheless exposes overlapping shared and exclusive Rust SDK borrows between previously serialized query paths, leaving one deduplicated memory-safety blocker. All five changed files were inspected; the six current-source availability-gate tests passed in an isolated XCTest harness, and git diff --check passed; the full SDK/application suites were not rerun.
🔴 1 blocking
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: ffi-engineer); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — The diff introduces nontrivial Swift actor isolation, concurrent FFI execution, lifetime management, and stale username-result handling across SDK and UI code, but does not change any of the specified critical surfaces. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— architecture-layering (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— ffi-engineer (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort high); agentphase2-reviewer,gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort high); agentphase2-reviewer
🤖 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/swift-sdk/Sources/SwiftDashSDK/FFI/PlatformQueryExtensions.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/FFI/PlatformQueryExtensions.swift:2308-2315: Prevent shared query borrows from overlapping exclusive SDK borrows
Moving these calls off the main actor permits an unsafe overlap on the same SDK handle. For example, start `documentList` and let another main-actor task finish a successful `documentGet` while the list request remains pending. The list's Rust contract-fetch/search paths borrow SDK state through `&SDKWrapper` across `block_on`, while `documentGet` invokes `dash_sdk_document_destroy` in its defer at line 708. That entry point constructs `&mut SDKWrapper` in `packages/rs-sdk-ffi/src/document/util.rs:31`, overlapping the live shared borrow and violating Rust's aliasing requirements. In the base revision, these two query bodies were main-actor isolated and contained no suspension points, so this particular overlap could not occur. Existing background helpers expose related pre-existing cases, but do not make this newly enabled overlap safe. `Sdk: Send + Sync` and retaining `self` protect neither against exclusive-reference aliasing. Replace unnecessary exclusive SDKWrapper borrows in conflicting entry points with shared borrows, and synchronize any genuinely exclusive access across all callers; serializing only the new queue would not coordinate it with `documentGet` or independently dispatched operations.
|
/self-reviewed |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The complete diff at the exact reviewed head is sound: off-main query execution retains the SDK safely, live FFI entry points use shared wrapper borrows, and availability results are correctly gated against stale or cancelled requests. The suggested explicit MainActor annotation is unnecessary because Swift 6 infers MainActor isolation for members of a type conforming to SwiftUI's MainActor-isolated View protocol. No in-scope defects remain.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: ffi-engineer); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a cross-cutting Swift/Rust FFI concurrency change affecting numerous SDK entry points and actor isolation, but it does not modify consensus, funds movement, cryptography, key handling, peer-facing deserialization, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— architecture-layering (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— ffi-engineer (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Use the allocation destructor when releasing documentGet results — The existing documentGet cleanup calls dash_sdk_document_destroy and ignores its returned error. That entry point performs a not-implemented operation rather than freeing the document allocation, while dash_sdk_document_handle_destroy is the actual handle destructor; successful documentGet calls therefore leak the document and the allocated error. This behavior predates the reviewed changes and is not caused by this PR.
- Follow-up: Track separately: replace the documentGet cleanup with dash_sdk_document_handle_destroy and audit other Swift callers for the same destructor mismatch.
|
Bots are done — your move: post |
documentList, dpnsCheckAvailability and the DPNS contest vote-state reads were declared in `@MainActor extension SDK` and called FFI entry points that park the calling thread in `block_on` until DAPI answers. A caller could not move the round trip off the main thread: `Task.detached` still hops back to the main actor for a main-actor method. Add `performBlockingQuery(_:)`, which runs the FFI call on a concurrent dispatch queue and resumes the caller through a checked continuation. The SDK is strongly captured so its handle stays valid for the call; each blocking helper keeps its C-string arguments alive for the whole call and copies and frees the native result on the queue thread, so only Swift values leave it. - documentList and dpnsCheckAvailability are now `nonisolated` (same names and arguments; documentList returns `sending [String: Any]`). - dpnsContestVoteStateOffMain / dpnsContestIsOpenOffMain are new async variants; the synchronous dpnsContestVoteState / dpnsContestIsOpen stay for source compatibility and share the blocking helper. The queue is concurrent: every query takes the handle as a shared `&SDKWrapper` over a multi-threaded runtime, and a serial queue would make the newest availability check wait behind stale ones. Refs #5145 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…mple app `dpnsCheckAvailability` now runs off the main actor, so lookups started by RegisterNameView's debounce can overlap and finish out of order. A slow "available" answer for an earlier name could overwrite the "taken" answer for the name being edited and enable Register Name. Each lookup is tagged by `AvailabilityRequestGate`: only the newest one, for the name still in the field, may set the state, and editing the name drops any lookup in flight. `AvailabilityRequestGateTests` covers two lookups completing out of order, an edited name, and cancellation. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ancelled Editing the name cancels the lookup in flight, but the "already checked" marker kept the old name. Typing that name back before the debounce fired scheduled nothing, the cancelled answer was rejected, and availability stayed unset with Register disabled. The marker now lives in AvailabilityRequestGate and cancel() clears it; `needsLookup(for:)` replaces the view's own marker. Tests cover edit-away/edit-back and an answered lookup not being repeated. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…y point 35 rs-sdk-ffi entry points (document, identity, data contract and token state transitions, document fetch, and dash_sdk_document_destroy) turned the SDK handle into `&mut SDKWrapper` although none of them mutates it: they only read `wrapper.sdk` and call `wrapper.runtime.block_on`. Now that the Swift SDK runs Platform queries concurrently off the caller's actor, such a call (e.g. `dash_sdk_document_destroy` in `documentGet`'s defer) can overlap a query holding a shared `&SDKWrapper` across `block_on`, which aliases a live shared borrow and is undefined behaviour. All of them now borrow `&*(sdk_handle as *const SDKWrapper)`; behaviour and C signatures are unchanged. The only remaining exclusive access is `dash_sdk_destroy`, which the Swift SDK calls solely from `SDK.deinit`, unreachable while a query retains `self`. The invariant is documented on `SDKWrapper`, `dash_sdk_destroy` and the Swift query queue. On v5.0-dev dash_sdk_document_destroy no longer exists (#5120), so the entry points borrowed here are the remaining live-handle ones. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
b702b5c to
610c083
Compare
|
🌳 GroveDB structure This pull request changes the described GroveDB structure. Open it in the structure viewer: new nodes glow, removed ones stay as ghosts, and the tour walks through each change. Added (10 nodes)
Changed (1 node)
Compared |
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-10-03T16:07:43.264Z |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At exact head 610c083, the complete diff moves the selected blocking queries off the caller’s actor while preserving SDK lifetime, FFI argument/result ownership, and existing query behavior. All three prior findings are fixed, and no actionable in-scope findings remain. Validation was static only; the supplied CI snapshot from 2026-10-03T16:42:35Z still shows Rust workspace tests running and PR Hygiene pending.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The cross-language concurrency, handle-borrowing, and UI cancellation changes require careful review but do not themselves change consensus, funds-movement logic, cryptography, network deserialization, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
|
Bots are done — your move: post |
|
/self-reviewed |
|
Ready for review — |
llbartekll
left a comment
There was a problem hiding this comment.
Reviewed at 610c083. Approving; the three inline notes are non-blocking.
What I checked:
- Rust aliasing: after this PR the only place that still builds a
*mut SDKWrapperfrom a handle isdash_sdk_destroy(Box::from_raw). Every other live-handle entry point borrows&*(… as *const SDKWrapper),BigStackRuntime::block_ontakes&self, andruntime/trusted_providerareArcs, so overlapping shared borrows from the concurrent queue are sound. TheSDKWrapperdoc comment states the invariant for future entry points. - Swift lifetime / ownership:
performBlockingQueryretainsselfacross the FFI call,handleis written only ininit, anddash_sdk_destroyis reached only fromdeinit, so destruction cannot overlap a pending query. Each blocking half (documentListJSON,dpnsCheckAvailabilityBlocking,fetchContestVoteState) copies the FFI string and frees it before returning, anddocumentListJSONstill frees the contract handle on every path. - Stale-result gate:
AvailabilityRequestGatelogic holds for the out-of-order, edit-away/edit-back and cancel cases; the view callscancel()on every change whereneedsLookupis true, so thenamecomparison inacceptsis belt-and-braces. - CI on this head: the Swift job compiled and ran both new suites (
PlatformQueryOffMainTests3/3,AvailabilityRequestGateTests6/6). The one redRust workspace testsrun failed inrs-drivestructure::tests(GroveDB structure JSON), which this PR does not touch; the rerun on the same commit is green.
|
Bots are done — your move: address llbartekll left a review thread unresolved, then post |
|
Ready for review — |
Issue being fixed or feature implemented
Closes #5145. Swift SDK query helpers live in
@MainActor extension SDKblocks and call FFI entrypoints that block in
runtime.block_onuntil DAPI answers, so every call parks the main thread for afull network round trip — and callers cannot avoid it (a
Task.detachedstill hops back to the mainactor for the call). Dash Wallet iOS hits this on proof-link lookups (
documentList), usernameavailability while typing (
dpnsCheckAvailability) and contest state (dpnsContestVoteState/dpnsContestIsOpen).What was done?
SDK.performBlockingQuery(_:)(FFI/PlatformQueryExtensions.swift,nonisolated, internal):runs the FFI body on a concurrent
.userInitiatedqueue and resumes the caller through a checkedcontinuation — the pattern
dataContractGetOffMainalready used, generalised.selfis held withwithExtendedLifetimefor the call (the handle is freed only indeinit); every FFI result iscopied into a
SendableSwift value and freed on the queue thread.documentListanddpnsCheckAvailability(name:)are nownonisolatedand run through it;names and arguments unchanged (
documentListreturnssending [String: Any]), so existingtry awaitcallers go off-main without edits. Their blocking halves arenonisolated statichelpers.
Voting/SDK+DPNSContests.swift: newdpnsContestVoteStateOffMain(normalizedLabel:limit:)anddpnsContestIsOpenOffMain(normalizedLabel:). The synchronousdpnsContestVoteState/dpnsContestIsOpenstay (callers use them synchronously) and share onefetchContestVoteState.&SDKWrapper,dash_sdk::SdkisSend + Sync, andBigStackRuntime::block_onruns each call on its own threadover a multi-threaded runtime;
dpnsActiveContests/dataContractGetOffMainalready use the handleconcurrently. A serial queue would make the newest availability check wait behind stale ones.
@MainActorblock are left as they are to keep the diff small (and clearof feat(sdk): expose the document erase and history lifecycle on mobile and FFI #4660, which edits this file); moving one later is a two-line wrapper.
How Has This Been Tested?
SwiftTests/SwiftDashSDKTests/PlatformQueryOffMainTests.swift(FFI mock SDK, no network): thequery body runs off the main thread; a query parked on a semaphore is released by the main actor
(the caller's actor is not held);
documentList,dpnsCheckAvailabilityand both…OffMainvariants surface the mock's FFI error to a main-actor caller.
swift test --filter 'PlatformQueryOffMainTests|SDKMethodTests|DPNSContestDecoderTests': 30 tests,0 failures. Full
swift test: 727 tests, 0 failures on two consecutive runs (one earlier run had asingle failure whose output was not captured; not reproduced).
./build_ios.sh --target tests --profile dev: Rust sim + mac slices and SwiftExampleApp for the iOSSimulator with warnings as errors —
BUILD SUCCEEDED, covering the example app's main-actor callsites of
documentListanddpnsCheckAvailability.Breaking Changes
None. Same method names and arguments; two methods become
nonisolated, two…OffMainvariants areadded.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
610c083rust-sdk-ffi(packages/rs-sdk-ffi/src/data_contract/put.rs,packages/rs-sdk-ffi/src/document/create.rs,packages/rs-sdk-ffi/src/document/delete.rsand 22 more) — lklimek or shumkovswift-sdk— you own itWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.