fix(swift-example-app): resume orphaned Broadcast asset-lock top-ups#4007
Conversation
A platform-address top-up that broadcast its asset lock but was
interrupted before finality (app killed mid-wait, or a prior attempt
failed) leaves a PersistentAssetLock at statusRaw 1 (Broadcast). The
"Pending Platform Top Ups" row gated its Resume affordance on
canFundIdentity (statusRaw ∈ {2,3}), so a Broadcast row rendered only a
non-interactive spinner — no way to resume, and thus a permanent
"Waiting for InstantSend / ChainLock…" dead end with committed on-chain
funds stuck behind it.
resume_asset_lock already supports resuming a Broadcast lock (it re-enters
the finality wait, now unbounded), so the fix is UI-only: offer Resume on
a Broadcast orphan row.
The one hazard: a fresh-build fund sits at Broadcast for the entire
(now unbounded) finality wait, so its lock co-renders as an orphan row.
The controller doesn't expose the outpoint it's driving, so we can't tell
that in-flight lock from a genuinely-orphaned one — and resuming the
in-flight one would spawn a second consuming flow on the same outpoint.
Gate the Broadcast Resume on "no funding in flight on this wallet": a
safe superset, since active controllers are transient, so a truly
orphaned Broadcast lock always becomes resumable once they clear.
Proof-ready (statusRaw 2/3) rows keep their existing unconditional Resume
(brief co-render window).
Also make the resume view's footer status-aware: it no longer claims a
Broadcast lock "reached a usable proof state" — it says resuming will
wait for finality before crediting.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
llbartekll and
shumkov
as code owners
|
Warning Review limit reached
Next review available in: 22 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 (3)
✨ 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 a4ace46) |
There was a problem hiding this comment.
Code Review
Presentation-layer only PR that exposes Resume on truly-orphaned Broadcast asset-lock rows, gated on wallet-scoped 'no funding in flight'. No Rust/FFI/consensus surface touched and the actual resume still routes through platform-wallet's existing FFI. One real SwiftUI observation-lifecycle gap in the new hasActiveFunding gate is worth calling out; the coarse wallet-scope granularity is a documented trade-off.
Source: reviewers=claude-general(opus), codex-general(gpt-5.5), claude-security-auditor(opus), codex-security-auditor(gpt-5.5), claude-ffi-engineer(opus), codex-ffi-engineer(gpt-5.5); verifier=claude(opus).
1 suggestion(s) | 1 nitpick(s)
Review findings
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingPlatformFundFromAssetLocksList.swift:60: `hasActiveFunding` goes stale on controller phase transitions
The parent view observes `AddressFundFromAssetLockCoordinator` via `@ObservedObject`, which only fires `objectWillChange` on mutations of the `@Published controllers` dictionary (add/remove/reassign keys). An in-place `phase` transition on a controller already in the map (`.inFlight` → `.failed` or `.completed`) does not mutate the dict — the controller is a reference type stored as a value, so the dict itself is unchanged. The in-flight child row observes the controller directly and updates, but the parent's `body` is not re-invoked, so `hasActiveFunding` (computed from `inFlight.contains { $0.phase.isActive }`) sticks at the value captured when the controller was inserted at `.inFlight`. Consequences: (1) a truly-orphaned Broadcast row co-existing with a `.failed` controller keeps rendering the non-interactive `Waiting…` spinner instead of Resume until the user taps Dismiss on the failed row (which does mutate the map and re-renders the parent); (2) a truly-orphaned Broadcast row co-existing with a `.completed` controller stays non-resumable for the ~30s until the retention sweep removes the completed slot. The primary app-killed-during-broadcast recovery path (no controllers in memory on relaunch) is unaffected. A simple fix is to also observe phase changes at the parent level — e.g., have the parent watch each controller's `objectWillChange` and re-publish, or expose an `@Published anyPhaseActive: Bool` on the coordinator that is updated by the controllers on phase transitions.
- [NITPICK] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingPlatformFundFromAssetLocksList.swift:60-72: Wallet-scoped `hasActiveFunding` blocks unrelated orphan Broadcast rows
`hasActiveFunding` is wallet-scoped, not outpoint- or account-scoped, so a genuinely orphaned Broadcast lock on wallet W stays non-resumable while any unrelated address-fund controller on W is `.inFlight`. This is called out as a deliberate 'safe superset' in both the code comment and the PR description because `AddressFundFromAssetLockController` does not currently expose the outpoint it is driving, and the stubbed `excludingControllerOutpoints` de-dupe therefore returns nil today. With the ChainLock finality wait now unbounded (#4006), the 'unresumable' window can be arbitrarily long in the worst case. The plumbing for the precise fix is already stubbed at lines 61–72; once the controller exposes its outpoint after broadcast, this coarse gate should be tightened rather than left in place.
| // orphaned Broadcast lock always becomes resumable once they | ||
| // clear. `.completed`/`.failed` controllers aren't consuming, so | ||
| // only `.isActive` (`.inFlight`) counts. | ||
| let hasActiveFunding = inFlight.contains { $0.phase.isActive } |
There was a problem hiding this comment.
🟡 Suggestion: hasActiveFunding goes stale on controller phase transitions
The parent view observes AddressFundFromAssetLockCoordinator via @ObservedObject, which only fires objectWillChange on mutations of the @Published controllers dictionary (add/remove/reassign keys). An in-place phase transition on a controller already in the map (.inFlight → .failed or .completed) does not mutate the dict — the controller is a reference type stored as a value, so the dict itself is unchanged. The in-flight child row observes the controller directly and updates, but the parent's body is not re-invoked, so hasActiveFunding (computed from inFlight.contains { $0.phase.isActive }) sticks at the value captured when the controller was inserted at .inFlight. Consequences: (1) a truly-orphaned Broadcast row co-existing with a .failed controller keeps rendering the non-interactive Waiting… spinner instead of Resume until the user taps Dismiss on the failed row (which does mutate the map and re-renders the parent); (2) a truly-orphaned Broadcast row co-existing with a .completed controller stays non-resumable for the ~30s until the retention sweep removes the completed slot. The primary app-killed-during-broadcast recovery path (no controllers in memory on relaunch) is unaffected. A simple fix is to also observe phase changes at the parent level — e.g., have the parent watch each controller's objectWillChange and re-publish, or expose an @Published anyPhaseActive: Bool on the coordinator that is updated by the controllers on phase transitions.
source: ['codex-general', 'claude-general']
There was a problem hiding this comment.
Resolved in a4ace46 — hasActiveFunding goes stale on controller phase transitions no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…ver proof-ready rows Follow-up review of the Broadcast-resume change surfaced three issues in the `hasActiveFunding` gate; this addresses them. 1. Stale gate (correctness). The gate is computed in a body that observes only the coordinator, whose @published `controllers` map re-publishes on insert/remove only — NOT when a controller already in the map flips phase. So `.inFlight → .failed` (an ST submit failing, leaving a genuinely-orphaned Broadcast lock) did not re-render the list, and the orphan kept the "Waiting…" spinner instead of Resume — the exact dead end this PR fixes — until the user dismissed the failed row. The reverse `.failed → .inFlight` retry (reusing the controller via `startFunding`) also mutated no map entry, leaving Resume visible on a lock a retry was driving. Fix: the coordinator now forwards each controller's `objectWillChange` to its own, so phase transitions re-render observers (`phaseObservers`, torn down with the controller). 2. Proof-ready rows were ungated (correctness). The gate only covered the Broadcast branch; the `canFundIdentity` (statusRaw 2/3) branch always showed Resume. A fresh fund's own lock passes through 2/3 while its ST is mid-consume, co-rendering as an orphan row with a live Resume button — the same duplicate-consume race the gate exists to prevent, one status later. Unify: suppress Resume on EVERY row while a funding is in flight; otherwise (every visible row is statusRaw 1/2/3) offer Resume. 3. Misnomer (clarity). The `inFlight` local actually held controllers of all phases (completed/failed linger until dismissed), making `inFlight.contains { $0.phase.isActive }` read as redundant and inviting a `!inFlight.isEmpty` "simplification" that would regress the gate. Renamed to `walletControllers` / `controllersForWallet`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior reconciliation: prior-1 FIXED — AddressFundFromAssetLockCoordinator now maintains phaseObservers: [SlotKey: AnyCancellable] that forward each controller's objectWillChange to the coordinator's own, and the list's hasActiveFunding (an @ObservedObject coordinator observer) now re-renders on .idle→.inFlight→.completed/.failed and on .failed→.inFlight retry; subscriptions are registered before submit() and are removed on dismiss() and by the 30s retention sweep, so no leak. prior-2 INTENTIONALLY DEFERRED — the wallet-scoped gate is deliberately broadened to suppress Resume on every status (1/2/3) while a funding is in flight, documented as a safe superset until controllers expose their outpoint via the already-stubbed excludingControllerOutpoints: de-dupe seam. Carried-forward prior findings: 0. New findings in latest delta: 0. Change is presentation-only, respects Swift SDK architectural rules (no orchestration, actual resume routes through platform-wallet), no consensus/versioning surface touched.
Source: reviewers=reviewer-claude-general-4007-1783252933(opus), reviewer-codex-general-4007-1783252933(gpt-5.5); verifier=verifier-4007-1783252933(opus).
2 participants
Follow-up to #4006 (merged). That PR made the asset-lock ChainLock wait unbounded so a top-up can't falsely "fail"; this one gives an interrupted top-up a way out of the resulting Broadcast state.
Issue being fixed or feature implemented
Defect 2 from the ADDR-09 top-up QA. A platform-address top-up that broadcast its asset lock but was interrupted before finality — app killed mid-wait, or a prior attempt failed — leaves a
PersistentAssetLockatstatusRaw 1(Broadcast). The "Pending Platform Top Ups" row gated its Resume affordance oncanFundIdentity(statusRaw ∈ {2,3}), so a Broadcast row rendered only a non-interactive spinner — no way to resume, cancel, or retry. Result: a permanent "Waiting for InstantSend / ChainLock…" dead end with committed on-chain funds stuck behind it.What was done?
resume_asset_lockalready supports resuming a Broadcast lock (it re-enters the finality wait — now unbounded, per #4006), so the fix is UI-only.statusRaw 1) orphan row inPendingPlatformFundFromAssetLocksList. Tapping it routes through the existingresumeFundFromAssetLock→platform-walletpath.statusRaw 2/3) rows keep their existing unconditional Resume (their co-render window is brief).FundFromAssetLockPlatformAddressView: it no longer claims a Broadcast lock "reached a usable proof state" — it says resuming will wait for finality, then credit the address.No Rust and no FFI changes; this is presentation logic only (the resume itself routes through
platform-wallet), consistent with the Swift-SDK architectural rules.How Has This Been Tested?
Not yet built/run — validation is pending the xcframework rebuild (
build_ios.sh) that also picks up #4006's runtime behavior, after which the resume can be exercised on the simulator against testnet (kill the app mid-top-up → confirm the orphaned Broadcast row now offers Resume and completes on finality). Code-level self-review only so far: single call site of the changed row initializer, andPersistentAssetLock: AssetLockResumeRowconformance is in place.Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code