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

packages/rs-platform-wallet-ffi/src/error.rs

@@ -218,6 +218,25 @@ impl From<PlatformWalletError> for PlatformWalletFFIResult {
             PlatformWalletError::WalletAlreadyExists(..) => {
                 PlatformWalletFFIResultCode::ErrorWalletAlreadyExists
             }
+            // The two shielded broadcast/wait variants. Today nothing routes
+            // them through this blanket impl — the dedicated match in
+            // `platform_wallet_manager_shielded_identity_create_from_pool`
+            // (`shielded_send.rs`) owns them so it can also write
+            // `out_identity_id` on the unconfirmed code. But any *future* FFI
+            // entry point that propagates these via `?` / `.into()` would
+            // otherwise silently flatten them to `ErrorUnknown` and defeat the
+            // slot-holding contract. A blanket conversion can't write
+            // `out_identity_id` (it has no out-param), so the most it can do is
+            // keep the typed code alive — which is what these arms guarantee.
+            PlatformWalletError::ShieldedBroadcastFailed(..) => {
+                PlatformWalletFFIResultCode::ErrorShieldedBroadcastFailed
+            }
+            PlatformWalletError::ShieldedBroadcastUnconfirmed { .. } => {
+                PlatformWalletFFIResultCode::ErrorShieldedBroadcastUnconfirmed
+            }
+            PlatformWalletError::ShieldedSpendUnconfirmed { .. } => {
+                PlatformWalletFFIResultCode::ErrorShieldedSpendUnconfirmed
+            }
             _ => PlatformWalletFFIResultCode::ErrorUnknown,
         };
         PlatformWalletFFIResult::err(code, error.to_string())
@@ -521,6 +540,61 @@ mod tests {
         );
     }
 
+    /// The two shielded broadcast/wait variants map to their dedicated FFI
+    /// codes through the blanket `From` impl rather than flattening to
+    /// `ErrorUnknown`. The dedicated `shielded_send.rs` match owns the live
+    /// path (it also writes `out_identity_id` on the unconfirmed code), but
+    /// any future entry point propagating these via `?` / `.into()` must keep
+    /// the typed code — these arms guarantee that. The typed Display rendering
+    /// still survives as the message.
+    #[test]
+    fn shielded_broadcast_variants_map_to_dedicated_codes() {
+        let failed = PlatformWalletError::ShieldedBroadcastFailed("relay rejected".to_string());
+        let rendered = failed.to_string();
+        let result: PlatformWalletFFIResult = failed.into();
+        assert_eq!(
+            result.code,
+            PlatformWalletFFIResultCode::ErrorShieldedBroadcastFailed,
+            "ShieldedBroadcastFailed should map to ErrorShieldedBroadcastFailed (rendered: {rendered})"
+        );
+        let msg = unsafe { std::ffi::CStr::from_ptr(result.message) }
+            .to_string_lossy()
+            .into_owned();
+        assert_eq!(msg, rendered, "Display payload must survive verbatim");
+
+        let unconfirmed = PlatformWalletError::ShieldedBroadcastUnconfirmed {
+            identity_id: dpp::prelude::Identifier::from([7u8; 32]),
+            reason: "result proof fetch failed".to_string(),
+        };
+        let rendered = unconfirmed.to_string();
+        let result: PlatformWalletFFIResult = unconfirmed.into();
+        assert_eq!(
+            result.code,
+            PlatformWalletFFIResultCode::ErrorShieldedBroadcastUnconfirmed,
+            "ShieldedBroadcastUnconfirmed should map to ErrorShieldedBroadcastUnconfirmed (rendered: {rendered})"
+        );
+        let msg = unsafe { std::ffi::CStr::from_ptr(result.message) }
+            .to_string_lossy()
+            .into_owned();
+        assert_eq!(msg, rendered, "Display payload must survive verbatim");
+
+        let spend_unconfirmed = PlatformWalletError::ShieldedSpendUnconfirmed {
+            operation: "unshield",
+            reason: "wait timed out".to_string(),
+        };
+        let rendered = spend_unconfirmed.to_string();
+        let result: PlatformWalletFFIResult = spend_unconfirmed.into();
+        assert_eq!(
+            result.code,
+            PlatformWalletFFIResultCode::ErrorShieldedSpendUnconfirmed,
+            "ShieldedSpendUnconfirmed should map to ErrorShieldedSpendUnconfirmed (rendered: {rendered})"
+        );
+        let msg = unsafe { std::ffi::CStr::from_ptr(result.message) }
+            .to_string_lossy()
+            .into_owned();
+        assert_eq!(msg, rendered, "Display payload must survive verbatim");
+    }
+
     /// Other wallet-error variants without a dedicated FFI arm still
     /// fall through to `ErrorUnknown` while carrying the typed
     /// Display rendering as the message. Pin this so the catch-all

packages/rs-platform-wallet/src/wallet/shielded/operations.rs

@@ -874,6 +874,7 @@ where
             }
         };
 
+
         // Pull the verified `Identity` out of the proof result. The expected variant is
         // `VerifiedIdentityWithShieldedNullifiers`; if drive-abci ever returns a different one the
         // broadcast still SUCCEEDED, so we don't turn it into an error — we synthesize the identity
@@ -957,12 +958,31 @@ where
         // very hazard this variant exists to prevent.
         Err(e @ PlatformWalletError::ShieldedBroadcastUnconfirmed { .. }) => Err(e),
         Err(e) => {
-            cancel_pending(store, id, &selected_notes).await;
+            if error_releases_note_reservation(&e) {
+                cancel_pending(store, id, &selected_notes).await;
+            }
             Err(e)
         }
     }
 }
 
+/// Whether a failed identity-create should release the notes reserved for it.
+///
+/// `false` ONLY for [`PlatformWalletError::ShieldedBroadcastUnconfirmed`]: the broadcast was
+/// accepted and the transition may have executed, so the reservation must be retained. Releasing it
+/// now would invite double-spend attempts against notes that may already be consumed on chain — the
+/// very hazard that variant exists to prevent. `pending_nullifiers` is in-memory only (see
+/// `SubwalletState`, "never persisted; the next sync after a crash reconciles") and `mark_spent`
+/// during nullifier sync clears matching reservations, so if the transition actually executed the
+/// next sync promotes these notes to spent; if it truly never landed, an app restart drops the
+/// in-memory reservation and frees them.
+///
+/// Everything else is a definitive pre-execution / build / rejection failure: the spend never
+/// happened, so the reservation must be released.
+fn error_releases_note_reservation(e: &PlatformWalletError) -> bool {
+    !matches!(e, PlatformWalletError::ShieldedBroadcastUnconfirmed { .. })
+}
+
 /// Number of times [`identity_create_from_shielded_pool`] re-fetches the new identity by its
 /// derived id after a post-broadcast result-confirmation failure, before declaring the broadcast
 /// unconfirmed.
@@ -1686,3 +1706,41 @@ mod reserve_shield_fee_tests {
         assert!(matches!(err, PlatformWalletError::ShieldedBuildError(_)));
     }
 }
+
+#[cfg(test)]
+mod note_reservation_release_tests {
+    use super::*;
+
+    /// `ShieldedBroadcastUnconfirmed` is the one failure that must NOT release the reservation: the
+    /// broadcast was accepted and the transition may have executed, so freeing the notes invites a
+    /// double-spend against notes that may already be consumed on chain. The next nullifier sync
+    /// reconciles them.
+    #[test]
+    fn unconfirmed_broadcast_retains_reservation() {
+        let e = PlatformWalletError::ShieldedBroadcastUnconfirmed {
+            identity_id: Identifier::from([7u8; 32]),
+            reason: "result proof unavailable".to_string(),
+        };
+        assert!(
+            !error_releases_note_reservation(&e),
+            "ShieldedBroadcastUnconfirmed must retain the note reservation"
+        );
+    }
+
+    /// Every other failure is a definitive pre-execution / build / rejection failure — the spend
+    /// never happened, so the reservation must be released.
+    #[test]
+    fn definitive_failures_release_reservation() {
+        let releasing: Vec<PlatformWalletError> = vec![
+            PlatformWalletError::ShieldedBroadcastFailed("rejected on merits".to_string()),
+            PlatformWalletError::ShieldedBuildError("note selection failed".to_string()),
+            PlatformWalletError::ShieldedStoreError("store write failed".to_string()),
+        ];
+        for e in &releasing {
+            assert!(
+                error_releases_note_reservation(e),
+                "{e:?} must release the note reservation"
+            );
+        }
+    }
+}

packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/IdentityRegistrationController.swift

@@ -100,15 +100,19 @@ final class IdentityRegistrationController: ObservableObject {
 
     /// Which stage a shielded `.failed` terminal state failed at, so
     /// `RegistrationProgressView` can attribute the red marker to the
-    /// right step instead of always blaming the Halo 2 proof step.
-    /// Only meaningful for `fundingKind == .shieldedPool` and only set
-    /// alongside a `.failed` phase.
+    /// right step. Only meaningful for `fundingKind == .shieldedPool`.
+    ///
+    /// Deliberately only ONE case: `.broadcastRejected` is the single
+    /// failure shape we can attribute with confidence (a typed
+    /// `PlatformWalletError.shieldedBroadcastFailed`). We cannot honestly
+    /// claim a "before broadcast" stage by elimination — Rust build/proof
+    /// errors arrive as a generic `.walletOperation`, indistinguishable at
+    /// this layer from an invalid handle, a marshalling failure, or any other
+    /// non-broadcast error. So `failureStage` is left `nil` for everything
+    /// else and the progress view falls back to its elapsed-time heuristic.
+    /// Extensible: add a case here only when a new typed error actually lets
+    /// us attribute the stage with certainty.
     enum FailureStage {
-        /// Failed before or during the broadcast itself — build / proof
-        /// error, or a relay/CheckTx broadcast rejection. The shielded
-        /// progress view keeps the existing elapsed-time heuristic
-        /// (note-selection vs Halo 2 proof) for the pre-broadcast slice.
-        case beforeBroadcast
         /// Platform definitively rejected the broadcast transition (a
         /// `PlatformWalletError.shieldedBroadcastFailed`). Attributed to
         /// the "Broadcasting transition" step.
@@ -120,9 +124,13 @@ final class IdentityRegistrationController: ObservableObject {
     /// `.completed(id) | .failed(message) | .unconfirmed(id, message)`.
     @Published private(set) var phase: Phase = .idle
 
-    /// Stage attribution for a shielded `.failed` phase. `nil` whenever
-    /// the phase is not a shielded failure. Reset at the start of every
-    /// `submit` so a retry doesn't inherit the previous attempt's stage.
+    /// Stage attribution for a shielded `.failed` phase. `nil` means
+    /// "unattributed" — either the phase isn't a shielded failure, or the
+    /// failure wasn't a confidently-attributable broadcast rejection. On a
+    /// `nil` shielded failure the progress view falls back to its
+    /// elapsed-time heuristic (note-selection vs Halo 2 proof). Reset at the
+    /// start of every `submit` so a retry doesn't inherit the previous
+    /// attempt's stage.
     @Published private(set) var failureStage: FailureStage?
 
     /// Slot this controller is bound to. Stored so the coordinator
@@ -143,7 +151,8 @@ final class IdentityRegistrationController: ObservableObject {
     private(set) var lastSubmittedAt: Date?
 
     /// Timestamp of the most recent terminal transition
-    /// (`.completed` / `.failed`). Freezes the elapsed-time anchor
+    /// (`.completed` / `.failed` / `.unconfirmed`). Freezes the
+    /// elapsed-time anchor
     /// for `RegistrationProgressView`'s shielded step heuristic: a
     /// `.failed` row is retained until the user dismisses it, and
     /// deriving its step from live `Date()` would let the failed
@@ -194,8 +203,11 @@ final class IdentityRegistrationController: ObservableObject {
     /// `body` performs the actual FFI call. It runs detached on a
     /// background priority and reports the identity id on success
     /// or rethrows on failure. The controller flips `phase` to
-    /// `.completed` / `.unconfirmed` / `.failed` accordingly, and on a
-    /// shielded `.failed` records `failureStage` for step attribution.
+    /// `.completed` / `.unconfirmed` / `.failed` accordingly. On a shielded
+    /// `.failed` it records `failureStage = .broadcastRejected` ONLY for a
+    /// `PlatformWalletError.shieldedBroadcastFailed`; every other error leaves
+    /// `failureStage` nil (unattributed — the progress view uses its
+    /// elapsed-time heuristic) rather than guessing a stage by elimination.
     func submit(body: @escaping () async throws -> Data) {
         switch phase {
         case .idle, .preparingKeys, .failed:
@@ -228,16 +240,17 @@ final class IdentityRegistrationController: ObservableObject {
                     )
                 }
             } catch {
-                // Attribute the failure stage so the shielded progress view
-                // can point the red marker at the right step. A
-                // `shieldedBroadcastFailed` is a definitive platform/relay
-                // rejection of the broadcast; everything else (build / Halo 2
-                // proof errors) failed before the broadcast.
-                let stage: FailureStage
+                // Only a `shieldedBroadcastFailed` is a confidently-attributable
+                // broadcast rejection → mark the broadcast step. Everything else
+                // (Rust build / Halo 2 proof errors, which surface as a generic
+                // `.walletOperation`, plus invalid-handle / marshalling failures)
+                // is indistinguishable at this layer, so leave `failureStage` nil
+                // and let the progress view's elapsed-time heuristic place it.
+                let stage: FailureStage?
                 if case PlatformWalletError.shieldedBroadcastFailed = error {
                     stage = .broadcastRejected
                 } else {
-                    stage = .beforeBroadcast
+                    stage = nil
                 }
                 await MainActor.run {
                     self?.failureStage = stage

packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/RegistrationCoordinator.swift

@@ -35,20 +35,36 @@ final class RegistrationCoordinator: ObservableObject {
     /// observe map mutations via `objectWillChange`.
     @Published private(set) var controllers: [SlotKey: IdentityRegistrationController] = [:]
 
-    /// True when at least one slot is currently in flight (phase
-    /// `.preparingKeys` or `.inFlight`). Used by the network
-    /// toggle's `.disabled(_:)` modifier — switching testnet ↔
-    /// mainnet mid-flight tears down the FFI manager and would
-    /// abort the in-flight call mid-stream. The UI guards against
-    /// that race by reading this flag.
+    /// True when at least one slot is still holding itself against a
+    /// fresh registration — exactly `controller.phase.isActive`
+    /// (`.preparingKeys`, `.inFlight`, or `.unconfirmed`). Used by the
+    /// network toggle's `.disabled(_:)` modifier.
+    ///
+    /// Two reasons a slot must hold this gate:
+    /// - `.preparingKeys` / `.inFlight`: switching testnet ↔ mainnet
+    ///   mid-flight tears down the FFI manager and would abort the
+    ///   in-flight call mid-stream.
+    /// - `.unconfirmed`: the identity is probably live on chain. The
+    ///   picker's `usedIdentityIndices` unions the persisted `isUsed`
+    ///   reservation with the `PersistentIdentity` rows, but that
+    ///   reservation write is best-effort (silent no-op when the slot row
+    ///   is beyond the derived lookahead) — so the live controller remains
+    ///   a load-bearing guard until the identity row lands via sync.
+    ///   Switching networks tears down the `PlatformWalletManager` and
+    ///   with it this coordinator, dropping the controller (and the
+    ///   Rust-side note reservation); the same HD slot could become
+    ///   selectable and a re-submission would be rejected by the
+    ///   registered-key-hash stateful check and burn the funded spend.
+    ///
+    /// Reading `isActive` directly (rather than re-listing the cases)
+    /// keeps this gate from drifting from the phase model, mirroring
+    /// `PendingRegistrationsList.isDismissable`. UX trade-off, by design
+    /// (same as the dismissal gate): an `.unconfirmed` row blocks network
+    /// switching until it becomes dismissable (the identity row arrives
+    /// via sync) or the app restarts.
     var hasInFlightRegistrations: Bool {
         controllers.contains { _, controller in
-            switch controller.phase {
-            case .preparingKeys, .inFlight:
-                return true
-            default:
-                return false
-            }
+            controller.phase.isActive
         }
     }
 

packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift

@@ -1463,15 +1463,20 @@ struct CreateIdentityView: View {
                     // the inline failure state.
                     return
                 case .unconfirmed:
-                    // Broadcast landed but its result couldn't be
-                    // confirmed; the identity is probably already live on
-                    // chain. Mark the slot used (same as `.completed`) so
-                    // the next registration can't reuse these keys and
-                    // burn funds against the registered-key-hash stateful
-                    // check. We do NOT persist a `PersistentIdentity` row
-                    // here — the proof-verified identity wasn't returned;
-                    // the next sync writes the row once the identity is
-                    // confirmed on chain.
+                    // Broadcast landed but its result couldn't be confirmed;
+                    // the identity is probably already live on chain. Mark the
+                    // slot used (same as `.completed`) — `usedIdentityIndices`
+                    // unions the persisted `isUsed` reservation with the
+                    // `PersistentIdentity` rows, so this is the reservation
+                    // that holds the slot across app restarts until the next
+                    // sync writes the identity row. It is best-effort (silent
+                    // no-op when the slot row is beyond the derived
+                    // lookahead), so the live `.unconfirmed` controller and
+                    // the dismissibility gate in `PendingRegistrationsList`
+                    // remain as defense in depth. We do NOT persist a
+                    // `PersistentIdentity` row here — the proof-verified
+                    // identity wasn't returned; the next sync writes the row
+                    // once the identity is confirmed on chain.
                     markIdentitySlotUsed(
                         walletId: walletId,
                         identityIndex: identityIndex