fix(swift-example-app): resume orphaned Broadcast identity-registration locks#4010
Conversation
…on locks Sibling of the address-funding fix (#4007). `ResumableRegistrationRow` gated its Resume button on `canFundIdentity` (statusRaw 2/3) and rendered a permanent "Waiting for InstantSendLock or ChainLock…" spinner for a Broadcast (statusRaw 1) lock. An identity registration interrupted at Broadcast — app killed mid-wait, or an aged lock whose InstantSend quorum rotated — left committed on-chain funds behind that spinner with no in-app recovery (the launch catch-up sweep caps each pass at 300s). Offer Resume for every row instead. Safe without any extra gate: the "Resumable Registrations" list already filters to statusRaw 1...3 and anti-joins on `(walletId, identityIndex)` unioned with active-controller slots, so any lock reaching a row is a genuinely-orphaned one with no driver. Resuming a Broadcast lock re-enters the finality wait via `resume_asset_lock`. Unlike the address-funding sibling — whose lock doesn't know its recipient, forcing the coarser `hasActiveFunding` gate — identity locks carry their `identityIndex`, so the anti-join excludes the in-flight lock precisely and no gate is needed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ 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 |
|
✅ Review complete (commit 786deb9) |
There was a problem hiding this comment.
Code Review
The PR fixes the intended Broadcast dead-end for identity-registration locks. However, the resumable identity list is sourced from every PersistentAssetLock on the network without a fundingTypeRaw filter, so making Broadcast rows unconditionally tappable now exposes non-identity (address / shielded) funding locks to CreateIdentityView(preselectedAssetLock:). Two stale doc comments also linger from the removed spinner branch.
🔴 1 blocking | 🟡 2 suggestion(s)
Source: reviewers: claude/opus (general, ok), codex/gpt-5.5 (general, ok); verifier: opus/opus.
🤖 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/Core/Views/IdentitiesContentView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/IdentitiesContentView.swift:494-497: Non-identity asset locks can now be resumed as identity registrations
`networkScopedAssetLocks` includes every `PersistentAssetLock` on the active network — including `fundingTypeRaw == 4` (AssetLockAddressTopUp) and `fundingTypeRaw == 5` (AssetLockShieldedAddressTopUp) rows. On the Rust side, `fund_from_asset_lock.rs:170` and `build.rs:335` show that address-funding calls pass `identity_index = 0` ("the resolver only consults it for `IdentityTopUp`, any value is fine"), which lands verbatim on the SwiftData row as `identityIndexRaw = 0` via `PlatformWalletPersistenceHandler.persistAssetLocks`. `crossWalletResumableLocks` only checks `statusRaw ∈ 1...3` and the `(walletId, identityIndex)` anti-join, so an interrupted address-funding lock at Broadcast (app killed mid-wait, or aged IS quorum) on a wallet with no identity at slot 0 and no active identity-registration controller for slot 0 will reach `ResumableRegistrationRow`. Pre-PR, that row rendered a spinner and was inert; post-PR it renders the unconditional Resume button, which sets `resumingAssetLock` and opens `CreateIdentityView(preselectedAssetLock: lock)` — driving `resumeIdentityWithAssetLock` against an address-funding outpoint. The commit message's safety argument ("no `hasActiveFunding` gate is needed here: identity locks carry their `identityIndex`") assumes the input is restricted to identity locks; it isn't. Filter to identity funding types before handing locks to the resumable-identity filter.
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/IdentitiesContentView.swift:299-322: Stale doc: `crossWalletResumableLocks` still describes the removed spinner→Resume staging
The doc comment justifying the `>= 1` status floor still says a Broadcast lock's trailing affordance "flips from a spinner to a Resume button automatically" once the persister advances the status. That branch was deleted from `trailingAffordance` in this PR — Broadcast rows now render the same Resume button as later statuses. The rationale for keeping Broadcast in the list (avoiding a vanish/reappear UX asymmetry) is still valid, but the wording needs updating. The `<= 3` upper-bound rationale further down also describes the Consumed regression as a "perpetual-spinner row"; with the spinner gone, the equivalent regression is a permanent Resume button that fails when tapped. Update both paragraphs to reflect the new affordance.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/CreateIdentityResumableTests.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/CreateIdentityResumableTests.swift:15-67: Stale test doc: references removed "spinner vs Resume button" staging
The class-level doc comment says the row's trailing affordance "— spinner vs. Resume button — is staged on `statusRaw >= 2` separately inside `ResumableRegistrationRow`", and `testAcceptsBroadcastForVisibility` similarly claims Broadcast "renders a spinner ('Waiting for InstantSendLock…') until SPV delivers the IS lock". Neither is true after this PR — every row renders a Resume button. The tests still pass because they only exercise the pure filter, but the comments now describe behavior that no longer exists and mis-document the intent for future readers.
|
Follow-up for the post-merge review blocker is open as #4015: it gates the Resumable Registrations list to identity-funding asset locks before routing into the identity resume flow. |
Sibling of #4007. The review of that PR flagged that the identity-registration resume list has the identical Broadcast dead-end, unfixed.
Issue being fixed or feature implemented
ResumableRegistrationRowgated its Resume button oncanFundIdentity(statusRaw 2/3) and rendered a permanent "Waiting for InstantSendLock or ChainLock…" spinner for a Broadcast (statusRaw 1) lock. An identity registration interrupted at Broadcast — app killed mid-wait, or an aged lock whose InstantSend quorum rotated — left committed on-chain funds behind that spinner with no in-app recovery (the launch catch-up sweep caps each pass at 300s, so an SPV that isn't caught up in time leaves the lock stuck until the next relaunch).What was done?
Offer Resume for every row in the "Resumable Registrations" list.
Safe without any extra gate: the list already filters to statusRaw 1...3 (
crossWalletResumableLocks) and anti-joins on(walletId, identityIndex)unioned with active-controller slots, so any lock reaching a row is a genuinely-orphaned one with no active driver. Resuming a Broadcast lock re-enters the finality wait viaresume_asset_lock.Unlike the address-funding sibling (#4007) — whose lock doesn't know its recipient, forcing a coarser
hasActiveFundinggate — identity locks carry theiridentityIndex, so the anti-join excludes the in-flight lock precisely and no gate is needed here. The row already showslock.statusLabel, so a Broadcast lock reads clearly next to the button.How Has This Been Tested?
Not yet built/run — validation is pending the xcframework rebuild that also picks up the related finality changes (#4006). Code-level self-review only; the safety argument rests on the existing precise anti-join (
crossWalletResumableLocks+identitySlots ∪ activeSlots).Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code