fix(swift-sdk): attribute shielded registration errors to the right step and keep unconfirmed broadcasts safe

[QuantumExplorer]

Issue being fixed or feature implemented

The SwiftExampleApp shielded (Type-20 IdentityCreateFromShieldedPool) registration progress view misattributed broadcast-result errors to the "Generating Halo 2 proof" step. Observed live on devnet: a type-20 transition executed successfully on-chain, but the client's broadcast_and_wait failed while fetching/verifying the execution-result proof (server side since fixed in #3859). The app showed the registration as failed at the proof step — long after the proof had been generated and the transition broadcast and executed.

Worse than the cosmetic misattribution: on any error the wallet released the spent notes' reservations (cancel_pending) and the app treated the identity as unregistered, even though it could be live on-chain — the orphaned-identity hazard. Re-registering the same keys would fail the registered-key-hash stateful check and burn the funds to the fallback address minus a penalty.

What was done?

The whole flow previously ran as one opaque broadcast_and_wait, collapsing build, broadcast, and result-confirmation failures into a single ShieldedBroadcastFailed error. It is now staged so each failure class is handled on its own terms:

• rs-sdk: new identity_create_from_shielded_pool_transition trait method (build + structural validation only); the existing one-shot helper is reimplemented on top of it, so other callers see no behavior change.
• platform-wallet (operations.rs::identity_create_from_shielded_pool): broadcasts and waits as separate stages.
  • Build errors and broadcast/relay rejections behave as before (reservation released).
  • A wait-stage StateTransitionBroadcastError (Platform definitively reported the transition's execution error) also behaves as before.
  • Any other wait-stage error (result-proof fetch/verify failure, timeout, transport — the fix(drive): verify identity-create-from-shielded-pool results without unbounded terminal-key queries #3859 shape) is ambiguous: the wallet retries fetching the identity by its pre-derived id (4 attempts, ~3 s apart). If found, the flow completes as a normal success (local registration + persister row). If still unknown, it returns the new ShieldedBroadcastUnconfirmed { identity_id, reason } error and leaves the note reservations in place — pending_nullifiers is in-memory only, so the next nullifier sync promotes them to spent if the transition landed, and an app restart frees them if it didn't. Releasing them eagerly would invite double-spend attempts against possibly-consumed notes.
• rs-platform-wallet-ffi: new result codes ErrorShieldedBroadcastFailed = 16 (definitive rejection, notes released, safe to retry) and ErrorShieldedBroadcastUnconfirmed = 17 (broadcast accepted, result unconfirmed; uniquely, out_identity_id is also written on this code so the host can hold the slot).
• Swift SDK: mirrors the two codes; shieldedIdentityCreateFromPool throws a typed ShieldedIdentityCreateUnconfirmedError carrying the derived id on code 17.
• SwiftExampleApp:
  • IdentityRegistrationController gains an .unconfirmed(identityId:message:) phase that is active (holds the identity slot; submit() and the coordinator refuse re-entry) plus a FailureStage for step attribution; observeController marks the HD slot used on .unconfirmed.
  • The shielded progress step set grows from 4 to 5 with a distinct "Waiting for platform confirmation" step. Confirmation failures render an orange warning on that step (steps 1–3 done, register pending) with a "Broadcast succeeded — confirmation pending" banner instead of the red failure; broadcast rejections mark the broadcast step; only genuine pre-broadcast errors land on the Halo 2 step.
  • Pending Registrations renders .unconfirmed in orange ("Confirmation pending") and keeps it until the user dismisses it; the coordinator retains it like .failed rather than purging it like .completed.

All classification, fetch-retry, and reservation logic lives in Rust behind the FFI; the Swift layer only maps the typed error to presentation state, per the swift-sdk layering rules.

How Has This Been Tested?

• cargo check --all-targets --all-features --tests on dash-sdk, platform-wallet, platform-wallet-ffi; cargo clippy clean on all three; cargo fmt --all.
• cargo test -p platform-wallet --features shielded --lib: 220 passed. FFI error-mapping unit tests pass.
• New unit test asserts Phase.unconfirmed.isActive == true (slot held against re-submission).
• iOS xcframework rebuilt (regenerated header carries codes 16/17); SwiftExampleApp builds clean for the iPhone 16 simulator, including the test target.

Breaking Changes

None. The FFI function signature is unchanged; out_identity_id gains a documented additional write on the new unconfirmed result code only. The one-shot rs-sdk helper keeps its exact semantics.

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

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

@QuantumExplorer, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 10 minutes and 20 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more credits in the billing tab to continue.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 176f3889-8982-44ad-95bc-0f478d858ec0

📥 Commits

Reviewing files that changed from the base of the PR and between 787b0b3 and 6df799d.

📒 Files selected for processing (8)

• packages/rs-platform-wallet-ffi/src/error.rs
• packages/rs-platform-wallet/src/wallet/shielded/operations.rs
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/IdentityRegistrationController.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/RegistrationCoordinator.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingRegistrationsList.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/RegistrationProgressView.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/CreateIdentityResumableTests.swift

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/wonderful-hertz-71949d

---

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.}

[thepastaclaw]

✅ Review complete (commit 6df799d)

[thepastaclaw]

Code Review

PR cleanly splits broadcast and wait-stage handling for Type-20 shielded identity-create so post-broadcast confirmation failures hold the note reservation and surface an unconfirmed-with-id error, while genuine relay/CheckTx rejections continue to release. The classification, FFI wiring (codes 16/17, conditional out_identity_id), and Swift mirror are internally consistent. No in-scope blocking issues; a few defensive suggestions and one latent FFI-mapping gap worth fixing now.

🟡 3 suggestion(s)

1 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments with AI agents

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

In `packages/rs-platform-wallet-ffi/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/error.rs:194-214: Blanket From<PlatformWalletError> flattens ShieldedBroadcastFailed/Unconfirmed to ErrorUnknown
  The new `ShieldedBroadcastFailed` and `ShieldedBroadcastUnconfirmed` variants are mapped to the dedicated FFI codes 16/17 only inside `platform_wallet_manager_shielded_identity_create_from_pool`'s explicit match. The blanket `From<PlatformWalletError> for PlatformWalletFFIResult` here still falls through to `ErrorUnknown`, and `unwrap_result_or_return!` routes any `Err(...).into()` through this impl. Today nothing else propagates these variants via `?`, so the bug is latent — but the first time a helper does, the host will see `ErrorUnknown` instead of code 17, `out_identity_id` will be left zeroed (the blanket impl has no out-parameter), and Swift will fall back to `PlatformWalletError.unknown(...)` rather than `ShieldedIdentityCreateUnconfirmedError`. That re-introduces exactly the orphaned-identity/double-spend hazard this PR closes. Add explicit arms here for both variants (the unconfirmed case still cannot carry the id through a blanket conversion, but at least the typed code survives), and/or document on the variants that they must always be matched explicitly at the FFI entry point.

In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:786-842: New wait-stage classification has no Rust-level test pinning the safety invariant
  The whole orphaned-identity argument hinges on three branches added in this PR: `Err(StateTransitionBroadcastError)` → `ShieldedBroadcastFailed` (release reservation), other wait error + identity found by retry → success (notes finalized), other wait error + identity still missing → `ShieldedBroadcastUnconfirmed` with reservations intentionally retained. None of those have a unit/integration test in `rs-platform-wallet`. A future reorder of the `match` arms or refactor of `cancel_pending` could silently regress the wallet to the pre-PR behavior the description identifies as a double-spend/orphaned-identity hazard. A test (against a mocked `Sdk`/`Fetch`) that injects each of the three wait-error shapes and asserts which `PlatformWalletError` variant comes out — plus whether `cancel_pending` was called — would lock the invariant in.

In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/IdentityRegistrationController.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/IdentityRegistrationController.swift:222-230: FailureStage attribution treats every non-shieldedBroadcastFailed error as pre-broadcast
  The new mapping pins anything that isn't exactly `PlatformWalletError.shieldedBroadcastFailed` to `.beforeBroadcast`. That's correct for the two typed variants this PR introduces, but it also catches unrelated `PlatformWalletError` variants surfacing through the FFI bridge (e.g. wallet-handle / marshalling failures) and confidently attributes them to the Halo 2 build step purely by elimination — the same misattribution this PR is fixing, just for a different error class. Gate `.beforeBroadcast` on errors that actually come from the build/proof path (e.g. `.shieldedBuildError`) and surface anything else with no attribution (or a dedicated `.unknown` stage) so the progress view falls back to its elapsed-time heuristic rather than pointing at the wrong step.

[thepastaclaw]

Code Review

PR cleanly splits broadcast and result-wait for the Type-20 shielded identity-create flow, introduces the slot-holding ShieldedBroadcastUnconfirmed path, wires terminalAt and a fetch-by-derived-id fallback, and adds Drive/ABCI proof + direct-purchase regression tests. Latest delta (2270899→9efa3363) is a v3.1-dev merge plus a small terminalAt freeze in the Swift controller. All three prior suggestions are STILL VALID; one cumulative-head suggestion is added around UI dismissibility of the .unconfirmed row.

🟡 3 suggestion(s)

1 additional finding(s) omitted (not in diff).

1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

🤖 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-platform-wallet-ffi/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/error.rs:194-214: Blanket From<PlatformWalletError> still flattens shielded broadcast codes to ErrorUnknown
  Verified at head: lines 194-214 only special-case `NoSpendableInputs`/`OnlyOutputAddressesFunded`/`OnlyDustInputs` and `WalletAlreadyExists`; the two new variants `ShieldedBroadcastFailed` and `ShieldedBroadcastUnconfirmed` fall through to `ErrorUnknown`. They are mapped to codes 16/17 only inside the dedicated `platform_wallet_manager_shielded_identity_create_from_pool` match. Any future FFI entry point that propagates a `PlatformWalletError` through `?` / `.into()` will silently emit `ErrorUnknown`, defeating the slot-holding contract documented at the top of this file. Add explicit arms here (and keep the `out_identity_id` write at the dedicated entry point — only that path owns the typed id) so the codes survive future plumbing changes.

In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/IdentityRegistrationController.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/IdentityRegistrationController.swift:231-247: Non-broadcast errors are still attributed as `.beforeBroadcast` by elimination
  Verified at head: the catch-all classifies every error except `PlatformWalletError.shieldedBroadcastFailed` as `.beforeBroadcast`. The `ShieldedIdentityCreateUnconfirmedError` case is caught earlier, but any other typed wallet error surfacing through the FFI bridge (invalid handle, marshalling failure, generic wallet errors) is now confidently attributed to the build/Halo 2 step. That misleads the user about which stage actually failed and, for any failure that escapes after relay acceptance, marks a possibly-live transition as a normal pre-broadcast failure — exactly the misattribution this PR is trying to clean up. Gate `.beforeBroadcast` on errors that actually come from the proof/build path, and route unknown variants to an unattributed/`unknown` stage so the progress view falls back to elapsed-time heuristics.

In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingRegistrationsList.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingRegistrationsList.swift:74-98: `.unconfirmed` rows are user-dismissable, which can free the identity slot for a fund-burning retry
  Verified at head: `isDismissable` returns true for `.unconfirmed`, and `RegistrationCoordinator.dismiss` removes the controller. The comment frames this as harmless ("dismissing it only drops the Pending row"), but the start-gate at `RegistrationCoordinator` only inspects controllers it currently holds, and `CreateIdentityView.usedIdentityIndices` consults persisted `PersistentIdentity` rows — not the per-slot `isUsed` flag, by explicit design (`CreateIdentityView.swift:2007-2015`). If a user dismisses an `.unconfirmed` row before the identity-sync writes a `PersistentIdentity` for the derived id, the same index becomes selectable again. Re-submitting registers the same keys, hits the registered-key-hash stateful check, and burns the shielded funds to the fallback address minus penalty — the exact orphaned-identity hazard the `.unconfirmed` variant was added to prevent. Gate dismissibility on the persisted-identity row actually being present (or only allow dismiss when `controller.phase == .failed`).

[thepastaclaw]

Code Review

Verified the latest delta against the prior review at 9efa336: 3 of 4 prior findings are fixed (typed FFI codes preserved through the blanket From impl, failure-stage no longer attributed by elimination, .unconfirmed dismissal gated on a live PersistentIdentity row); the 4th (Rust-level test for the outer reservation-retention match) is partially addressed via unit tests for wait_error_is_definitive_rejection but the outer-match invariant itself is still untested. One new in-scope blocking issue: hasInFlightRegistrations in RegistrationCoordinator does not include .unconfirmed, so the OptionsView network-switch gate (the only thing keeping the FFI manager / coordinator alive while the slot is in-memory-only) re-enables as soon as the flow becomes .unconfirmed, allowing the same slot to lose its in-memory guard and be re-selected against an identity that is probably live on chain. Two pre-existing structural concerns (broadcast-stage transport ambiguity, persistent slot reservation across app restart) are recorded as out-of-scope follow-ups.

🔴 1 blocking | 🟡 1 suggestion(s)

1 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments with AI agents

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

In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/RegistrationCoordinator.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/RegistrationCoordinator.swift:44-53: Network-switch gate ignores .unconfirmed and can drop the only in-memory slot guard
  hasInFlightRegistrations returns true only for .preparingKeys and .inFlight, but this PR introduces .unconfirmed precisely because the on-chain identity may already exist and the live IdentityRegistrationController is the only thing keeping the slot from being re-selected before sync writes a PersistentIdentity row. The OptionsView network picker uses this flag (see OptionsView.swift:805 and the wrapper viewmodifier comment at :793-799) as its .disabled(_:) gate, and PlatformWalletManager+RegistrationCoordinator.swift:23-26 documents that switching networks tears down the active PlatformWalletManager (and with it the registrationCoordinator attached as an associated object). As soon as the flow reaches .unconfirmed, the gate releases, the user can switch networks (and switch back), the in-memory controller and pending_nullifiers reservation are gone, and usedIdentityIndices — which consults only PersistentIdentity — will once again surface the same HD slot. A second submission against the same registered key set will be rejected by the registered-key-hash stateful check and route the funded spend to the fallback address minus penalty — exactly the orphaned-identity / fund-burn hazard the .unconfirmed phase was introduced to prevent. The dismissal path in PendingRegistrationsList was hardened against this; this sibling gate must match.

In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:919-932: Outer-match reservation-retention invariant for ShieldedBroadcastUnconfirmed is still untested
  The latest delta adds unit tests for wait_error_is_definitive_rejection (operations.rs:1342-1368), which pin the wait-stage classifier. The complementary safety invariant — that the outer result match returns Err for ShieldedBroadcastUnconfirmed WITHOUT calling cancel_pending, while every other Err arm DOES cancel — is the actual protection against double-spending reserved notes after an ambiguous wait failure, and it has no Rust-level regression test. A small test seam that injects an unconfirmed result and asserts the selected_notes reservations remain held (and conversely, that a ShieldedBroadcastFailed result clears them) would prevent a future refactor from silently moving the unconfirmed arm into the generic cancellation branch. Partial carry-forward of prior finding #2.

[github-actions]

✅ DashSDKFFI.xcframework built for this PR.

• Workflow run: https://github.com/dashpay/platform/actions/runs/27395266524
• 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: "b44337feb32a232186b8847ae083a075d1659705aaee42dcb35bc940e39235b1"
)

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

Both prior findings (network-switch gate excluding .unconfirmed; missing test for the outer-match reservation-retention invariant) are resolved on head d9283d0: the Swift gate now delegates to phase.isActive which includes .unconfirmed, and the Rust outer match has been refactored to call error_releases_note_reservation, which is directly unit-tested for retain (Unconfirmed) vs release (definitive failures). No new in-scope blocking or suggestion-level defects introduced by the incremental delta.

[thepastaclaw]

Code Review

Prior findings at d9283d0 were revalidated — none existed and none carry forward. Cumulative review against the PR base and the latest delta against d9283d0 found no in-scope issues. The PR cleanly stages shielded IdentityCreateFromShieldedPool failures (build / broadcast / result-confirmation) so reservations are released only on definitive rejection, and the new typed FFI codes (16/17/18) plus the error_releases_note_reservation helper make the policy explicit and unit-tested. The Swift presentation layer correctly mirrors the new states. Codex's two FFI 'blocking' findings target deriveIdentityAuthKeyAtSlot (lines 926-936) and prePersistIdentityKeysForRegistration (lines 1056-1067), but those call sites are pre-existing in ManagedPlatformWallet.swift and are not touched by this PR — the PR's own new resolver call sites (in previewIdentityRegistrationKeys and discoverIdentities) already wrap their FFI calls in withExtendedLifetime(resolver), so they're out of scope for this PR.