Skip to content

fix(swift-example-app): resume orphaned Broadcast identity-registration locks#4010

Merged
QuantumExplorer merged 1 commit into
v4.1-devfrom
claude/resume-orphaned-broadcast-registrations
Jul 5, 2026
Merged

fix(swift-example-app): resume orphaned Broadcast identity-registration locks#4010
QuantumExplorer merged 1 commit into
v4.1-devfrom
claude/resume-orphaned-broadcast-registrations

Conversation

@QuantumExplorer

Copy link
Copy Markdown
Member

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

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, 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 via resume_asset_lock.

Unlike the address-funding sibling (#4007) — whose lock doesn't know its recipient, forcing a coarser hasActiveFunding gate — identity locks carry their identityIndex, so the anti-join excludes the in-flight lock precisely and no gate is needed here. The row already shows lock.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:

  • 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

…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>
@coderabbitai

coderabbitai Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

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

Next review available in: 15 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

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.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 740c4e40-10f4-464a-9fd5-ef5f0b2dbdea

📥 Commits

Reviewing files that changed from the base of the PR and between e8b9da2 and 786deb9.

📒 Files selected for processing (1)
  • packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/IdentitiesContentView.swift
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/resume-orphaned-broadcast-registrations

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

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added this to the v4.1.0 milestone Jul 5, 2026
@thepastaclaw

thepastaclaw commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

✅ Review complete (commit 786deb9)

@QuantumExplorer QuantumExplorer merged commit 56f37ac into v4.1-dev Jul 5, 2026
17 checks passed
@QuantumExplorer QuantumExplorer deleted the claude/resume-orphaned-broadcast-registrations branch July 5, 2026 12:28

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@thepastaclaw

Copy link
Copy Markdown
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants