fix(wallet): handle reported-consumed asset-lock recovery - #963
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe asset-lock resume flow maps an already-consumed lock to ChangesAsset-lock resume and status presentation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 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
🤖 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 `@DashWalletTests/PassiveWalletStateUITailTests.swift`:
- Around line 9-11: Reorder the imports at the top of
PassiveWalletStateUITailTests.swift to match the configured sorted_imports
order, keeping the same three imports and no other changes.
🪄 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: e4f508b3-cd19-4ded-ace7-164e004e035a
📒 Files selected for processing (2)
DashWallet/Sources/UI/Payments/InternalTransfer/ShieldedTransferCoordinator.swiftDashWalletTests/PassiveWalletStateUITailTests.swift
romchornyi
left a comment
romchornyi
left a comment
There was a problem hiding this comment.
idempotentAssetLockResumePhase maps any assetLockAlreadyConsumed to success, but that error also comes from the pre-existing local tombstone path (recovery.rs:345, AssetLockStatus::Consumed), where the lock was burned by an identity registration/top-up rather than by this shield — so a transfer that never landed would be reported as complete. Reachability is low and the refresh() + resync you added make the real balance surface shortly after, so either narrowing the mapping to the reconciled case or just softening the code comment works for me — as written it asserts a stronger invariant than the tombstone path guarantees. Two smaller notes: the title check fails because shielded isn't an allowed scope (wallet would do), and ShieldedTxLookup.refresh() + the projection notification were added only to the resume tail while the fresh performAssetLock success path (~line 466) probably wants the same.
cb48ff7 to
673f557
Compare
llbartekll
changed the title
|
Addressed the latest review points:
I also kept the status-5 history wording neutral ( |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/en.lproj/Localizable.strings`:
- Line 3586: Add the "Completion unknown" localization key to all 42 non-English
locale catalogs, matching the existing English entry and each catalog’s
formatting conventions, then synchronize the updated catalogs with Transifex.
In
`@DashWallet/Sources/UI/Payments/InternalTransfer/ShieldedTransferCoordinator.swift`:
- Around line 545-553: The status comments must reflect the consumed-lock
outcome across all three locations. In
DashWallet/Sources/UI/Payments/InternalTransfer/ShieldedTransferCoordinator.swift
lines 545-553, document both error sources that produce .submittedUnconfirmed;
in
DashWallet/Sources/UI/Payments/InternalTransfer/InternalTransferConfirmSheet.swift
lines 710-714, remove the claim that consumed locks surface a real SDK error;
and in DashWallet/Sources/UI/Tx/Details/Model/TxDetailModel.swift lines 503-508,
document unauthenticated consumed reports alongside restore-only status 5.
🪄 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: 60553932-38f8-410b-a612-c70451afaf4f
📒 Files selected for processing (6)
DashWallet/Sources/Infrastructure/SwiftDashSDK/PlatformAddressSyncCoordinator.swiftDashWallet/Sources/UI/Payments/InternalTransfer/InternalTransferConfirmSheet.swiftDashWallet/Sources/UI/Payments/InternalTransfer/ShieldedTransferCoordinator.swiftDashWallet/Sources/UI/Tx/Details/Model/TxDetailModel.swiftDashWallet/en.lproj/Localizable.stringsDashWalletTests/PassiveWalletStateUITailTests.swift
2 participants
Depends on dashpay/platform#4357.
Summary
assetLockAlreadyConsumedreport tosubmittedUnconfirmed, not verified successShieldedTxLookup, publish the transaction-projection notification, and schedule shielded sync after recoveryPending — tap to finishonce Rust has durably retained the lock as Core-final with Platform consumption unknownCompletion unknownfor status 5 and add a unit-level mapping regression testsorted_importsTesting
dashpaysimulator build succeeded after rebasing onto currentdevelopDashWalletTestsexecution remains locally blocked because thedashwalletscheme embeds a Watch app and the required watchOS 26.5 runtime is not installedSummary by CodeRabbit