feat(wallet): retry stuck asset-lock transfers from the tx detail sheet - #949
Merged
Conversation
A funding asset lock can park mid-transfer: built or broadcast but never IS/CL-locked (the "Broadcasting" status), or locked on Core but with the Platform transition never landing (app killed, network drop). Until now the detail sheet just showed the stuck status with no way forward. The sheet now offers a recovery action for those states: "Rebroadcast" while the transaction is unconfirmed (status built/ broadcast), "Complete Transfer" once the lock is on-chain and only the Platform side remains. Both drive the SDK's crash-recovery resume entry points on the EXISTING tracked outpoint - rebroadcast if needed, IS/CL wait, Platform submit, consume - so a retry can never build (and strand) a second lock: - identity top-up locks (types 1/2): resumeTopUpWithAssetLock against the wallet's identity, PIN-gated, refreshing the identity snapshot on success. consumeInvitationVoucher stays false - a generic retry surface must never silently consume an invitation voucher. - Core -> Platform address funding (type 4): ShieldedTransferCoordinator.resumeFundPlatform. - Core -> Shielded funding (type 5): ShieldedTransferCoordinator.resumeAssetLock. - identity registration/invitation locks (types 0/3) are deliberately excluded - they recover through the Join DashPay flow, which owns key preparation. The await spans the whole recovery under a progress HUD (a worst case includes the IS/CL wait), the rows re-derive afterward either way - even a failed retry can have advanced the lock, and the status row should say so - and backing out of the PIN prompt is treated as a non-error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds asset-lock recovery for supported funding types. Transaction details expose retryable locks, invoke the recovery service, show progress and results, refresh transaction data, and display localized recovery states. ChangesAsset-lock recovery
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TxDetailModel
participant TxDetailViewController
participant AssetLockRecoveryService
participant SwiftDashSDK
TxDetailModel-->>TxDetailViewController: provide retry metadata
TxDetailViewController->>AssetLockRecoveryService: recover asset lock
AssetLockRecoveryService->>SwiftDashSDK: resume supported funding route
SwiftDashSDK-->>AssetLockRecoveryService: return result
AssetLockRecoveryService-->>TxDetailViewController: return success or error
TxDetailViewController->>TxDetailModel: refresh transaction state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
DashWallet/Sources/Infrastructure/SwiftDashSDK/AssetLockRecoveryService.swift (2)
91-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDistinguish the two
notReadycauses.Both a missing wallet and a missing
identityIdmap toRecoveryError.notReady, which renders as "Wallet is not ready". For an identity top-up retry, a missing identity is a different condition. Separate the guards so the log and the message identify the actual cause.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DashWallet/Sources/Infrastructure/SwiftDashSDK/AssetLockRecoveryService.swift` around lines 91 - 102, Update retryIdentityTopUp to use separate guards for the wallet and identityId prerequisites instead of combining them. Keep the wallet failure mapped to the existing wallet-not-ready error, and map the missing identityId to a distinct recovery error/message so logs and user-facing output identify the actual cause.
66-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the duplicate coordinator resume branches.
ShieldedTransferCoordinatoris@MainActor; both resume methods are non-throwingasyncmethods with the specified labels, andPhasedefines.failed(String). Combine cases 4 and 5, then callcheckTerminalPhaseonce.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DashWallet/Sources/Infrastructure/SwiftDashSDK/AssetLockRecoveryService.swift` around lines 66 - 84, Update retry in AssetLockRecoveryService to combine funding types 4 and 5 into one branch, instantiate a single ShieldedTransferCoordinator, invoke the appropriate resume method based on the funding type, and call checkTerminalPhase once afterward. Preserve the existing method labels, await behavior, and unsupportedRoute handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@DashWallet/Sources/Infrastructure/SwiftDashSDK/AssetLockRecoveryService.swift`:
- Around line 107-113: Update checkTerminalPhase to preserve and inspect the
typed CoordinatorError.authCancelled state, or the coordinator’s explicit
cancelled phase, instead of comparing localized errorDescription text. Map that
typed cancellation to DWIdentityAuthorizer.AuthError.cancelled while continuing
to convert other failed messages to RecoveryError.failed.
---
Nitpick comments:
In
`@DashWallet/Sources/Infrastructure/SwiftDashSDK/AssetLockRecoveryService.swift`:
- Around line 91-102: Update retryIdentityTopUp to use separate guards for the
wallet and identityId prerequisites instead of combining them. Keep the wallet
failure mapped to the existing wallet-not-ready error, and map the missing
identityId to a distinct recovery error/message so logs and user-facing output
identify the actual cause.
- Around line 66-84: Update retry in AssetLockRecoveryService to combine funding
types 4 and 5 into one branch, instantiate a single ShieldedTransferCoordinator,
invoke the appropriate resume method based on the funding type, and call
checkTerminalPhase once afterward. Preserve the existing method labels, await
behavior, and unsupportedRoute handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e5c293c2-8ce8-48b5-970a-ea3e873492b9
📒 Files selected for processing (5)
DashWallet.xcodeproj/project.pbxprojDashWallet/Sources/Infrastructure/SwiftDashSDK/AssetLockRecoveryService.swiftDashWallet/Sources/UI/Tx/Details/Model/TxDetailModel.swiftDashWallet/Sources/UI/Tx/Details/TxDetailViewController.swiftDashWallet/en.lproj/Localizable.strings
5 tasks
…entity error Addresses the review on #949. - `ShieldedTransferCoordinator` now records the typed error behind a `.failed` phase in `lastFailure`, so `AssetLockRecoveryService` can tell a PIN cancel from a real failure without comparing localized text (the `Phase` payload is display copy and would silently stop matching on any wording or locale change). Cleared on `beginTransfer()`/`reset()`; the single `phase = .failed` assignment site keeps the two in step. - Split the identity top-up preconditions: a missing wallet stays `.notReady`, a missing identity gets its own `.noIdentity` error and log line, so both the log and the alert name the actual cause. - Collapse the two coordinator resume branches (funding types 4 and 5) so the resume call and its terminal-phase check stay a single pair. Clean `dashpay` simulator build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
5 tasks
This was referenced Aug 10, 2026
This was referenced Aug 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
A funding asset lock can park mid-transfer: built or broadcast but never IS/CL-locked (the tx detail sheet's "Broadcasting" status — see the 2 DASH identity top-up that motivated this), or locked on Core but with the Platform transition never landing (app killed, network drop between broadcast and submit). The detail sheet showed the stuck status with no way forward.
It now offers a recovery action for those states:
How
Both drive the SDK's crash-recovery resume entry points on the existing tracked outpoint — rebroadcast if needed, IS/CL wait, Platform submit, consume — so a retry can never build (and strand) a second lock:
resumeTopUpWithAssetLockagainst the wallet's identity, PIN-gated; identity snapshot refreshes on successShieldedTransferCoordinator.resumeFundPlatformShieldedTransferCoordinator.resumeAssetLockconsumeInvitationVoucherstaysfalseon the generic surface, so a shared invitation voucher can never be silently consumed.New
AssetLockRecoveryServicedispatches by funding type;TxDetailModel.stuckAssetLockRetryderives the button from the same liveShieldedTxLookupinfo that renders the status row (statuses 0–3 only — consumed and restored-from-chain locks never show it). The await spans the whole recovery under a progress HUD (worst case includes the IS/CL wait), and the rows re-derive afterward either way — even a failed retry can have advanced the lock, and the status row should say so.Testing
Clean
dashpaybuild; installed on the QA simulator and live-tested against the 2 DASH top-up stuck at "Broadcasting": the retry correctly found the built-never-locked lock and re-broadcast it to 2/3 peers (first successful send of that tx this session — the launch-time auto catch-up had failed on an SPV startup race).The test also uncovered why that particular lock can never complete: its input was already spent on-chain at height 1510203 (a coin-selection race during the restored wallet's history scan), so peers silently drop the tx — no retry can fix it. Follow-ups filed on the platform side: (a) the launch catch-up racing SPV startup, (b) quarantining freshly-scanned UTXOs during restore catch-up + a typed permanently-invalid error from
resume_asset_lockso this button can offer discarding such a lock instead.🤖 Generated with Claude Code
Summary by CodeRabbit