fix(wallet): restored internal transfers show their route and amount - #939
Conversation
After a wipe & recover, PersistentAssetLock rows don't survive, so asset-lock funding txs rendered "Internal Transfer - 0 DASH"; shielded Sent rows showed no destination at all. ShieldedTxLookup now reconstructs missing locks from the persisted raw tx bytes (real locked amount; full Core->Platform treatment when the credit outputs pay own persisted Platform addresses; an explicit unknown-destination sentinel otherwise), and shielded Sent entries decode their 43-byte Orchard counterparty to the DIP-0018 bech32m form for the row pill and the detail sheet's From/To rows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 10 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 Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe wallet reconstructs missing asset-lock records from raw transactions during restore, uses parsed amounts in transaction displays, and decodes shielded send counterparties for destination-aware activity details and routes. ChangesAsset-lock restoration
Shielded send display
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant SnapshotRefresh
participant PlatformAddressSyncCoordinator
participant RawTransactionInspector
participant ShieldedTxLookup
SnapshotRefresh->>PlatformAddressSyncCoordinator: refresh snapshot
PlatformAddressSyncCoordinator->>ShieldedTxLookup: find existing asset-lock entries
PlatformAddressSyncCoordinator->>RawTransactionInspector: parse missing raw asset-lock transactions
RawTransactionInspector-->>PlatformAddressSyncCoordinator: return validated outputs and outpoint index
PlatformAddressSyncCoordinator->>ShieldedTxLookup: merge reconstructed records
sequenceDiagram
participant ShieldedActivity
participant HomeViewModel
participant ShieldedActivityHistory
ShieldedActivity->>HomeViewModel: load sent activity
HomeViewModel->>HomeViewModel: decode Orchard counterparty and classify ownership
HomeViewModel-->>ShieldedActivityHistory: provide destination and external status
ShieldedActivityHistory->>ShieldedActivityHistory: render destination and route
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/Models/Transactions/Model/RawTransactionInspector.swift`:
- Around line 230-242: Update assetLockCreditOutputs(payload:) to validate
scriptLength can be represented as an Int before converting it, using
Int(exactly:) or an equivalent bound check. Return nil for oversized lengths and
only pass the validated Int to reader.readBytes.
In `@DashWallet/Sources/UI/Home/Views/HomeViewModel.swift`:
- Around line 1563-1575: Update the sent-item handling in fetchShieldedActivity
so isExternalDestination reflects wallet ownership rather than address decoding
success. Resolve the destination against persisted self-transfer/ownership data
or the active wallet’s Orchard-address lookup, while preserving the decoded
destinationAddress for display; mark only non-owned destinations as external.
In `@DashWallet/Sources/UI/Home/Views/ShieldedActivityHistory.swift`:
- Line 230: Update the route documentation near the .sent branch to state that
undecodable sent entries remain route-less when destinationAddress is absent,
replacing the incorrect “External address” fallback description.
🪄 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: 686a3df1-b131-4dad-b67c-dd59810d0740
📒 Files selected for processing (5)
DashWallet/Sources/Infrastructure/SwiftDashSDK/PlatformAddressSyncCoordinator.swiftDashWallet/Sources/Models/Transactions/Model/RawTransactionInspector.swiftDashWallet/Sources/Models/Transactions/Model/Transaction.swiftDashWallet/Sources/UI/Home/Views/HomeViewModel.swiftDashWallet/Sources/UI/Home/Views/ShieldedActivityHistory.swift
…ools Every asset-lock credit output pays a one-time address derived from a purpose-specific funding account (identity registration/top-up/ invitation, Platform address top-up, shielded top-up), and those address pools survive a restore as PersistentCoreAddress rows. Match the parsed credit outputs against them to recover the exact funding type, so a restored Transparent->Shielded transfer renders the full Core->Shielded treatment instead of a generic unknown-destination "Internal Transfer". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- ByteReader gains a UInt64-length readBytes that rejects lengths above Int.max instead of trapping on the Int(_:) conversion; all CompactSize script/payload reads go through it. - A shielded Sent destination is marked external only when provably not the wallet's own: an intra-wallet transfer leaves a Received row under the same entry id, and the wallet's default Orchard address is checked directly. - internalMoveRoute doc: undecodable Sent counterparties stay route-less. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 `@DashWallet/Sources/UI/Home/Views/HomeViewModel.swift`:
- Around line 1442-1445: Update the ownership lookup for ownShieldedRaw43 to use
the walletId captured by hostHandles() instead of reading wallet.walletId from
the later SwiftDashSDKHost.shared.wallet access, while preserving the existing
manager lookup and shieldedDefaultAddress call.
🪄 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: ce5ab4d5-e57c-4ab1-9376-861eadaf007c
📒 Files selected for processing (3)
DashWallet/Sources/Models/Transactions/Model/RawTransactionInspector.swiftDashWallet/Sources/UI/Home/Views/HomeViewModel.swiftDashWallet/Sources/UI/Home/Views/ShieldedActivityHistory.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- DashWallet/Sources/UI/Home/Views/ShieldedActivityHistory.swift
- DashWallet/Sources/Models/Transactions/Model/RawTransactionInspector.swift
The default-Orchard-address lookup re-read the host's active wallet on a later main hop; a wallet switch between the two reads could compare against the wrong wallet's address. Use the walletId the activity rows were fetched with. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…sset-lock reconstruction (#943) * feat(shielded): chain-order the Date-unknown band; trust SDK asset-lock reconstruction Consumes platform #4342 (SDK-side tracked-asset-lock reconstruction + minNotePosition on scan-derived shielded entries): - The trailing "Date unknown" history band (PR #940) now orders by exact on-chain sequence: ShieldedActivityItem carries the SDK's minNotePosition (smallest note commitment-tree position, nil when the row predates the field), TransactionListDataItem exposes it as chainOrderKey, and the pre-grouping sort tiebreaks equal dates by it descending — identical order on every device that restores the seed. - The app-side restore fallback (PR #939's addReconstructedLocks raw-tx reparse) is reduced to a coverage diagnostic: SDK reconstruction now writes store-backed PersistentAssetLock rows for every historical funding tx (verified 9/9 on a restored testnet wallet — identity, platform, and shielded types all classify), and store rows always won over the fallback anyway. Any asset-lock tx with no row is logged as a reconstruction gap. The reconstructed* sentinels and Transaction.reconstructedLockAmountDuffs go with it. - statusRaw 5 (RecoveredFromChain) is mapped explicitly: dedup prefers consumed (4) over recovered (5), and the tx detail sheet shows "Restored — completion unknown" instead of falling through. Note the SDK currently delivers reconstructed rows at 1/3, not 5 — the false- pending consequence and the enrichment gap are SDK-side, tracked for a platform follow-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(dashpay): registration recovery includes RecoveredFromChain locks With platform #4347, a registration asset lock stranded by an app kill whose block chain-locks before relaunch surfaces as statusRaw 5 (RecoveredFromChain) instead of staying in the 0…3 window the recovery lookup filtered on — the stranded payment would have stopped being offered for recovery. Treat everything but the Consumed (4) tombstone as unfinished: a 5-lock with no identity is a genuinely incomplete registration, and the SDK resume path explicitly supports consuming it. Completed registrations never resume from this lock — both call sites probe the identity (local row, then the Platform slot, reconciling the lock to Consumed) first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Issue being fixed or feature implemented
On a restored (wipe & recover / second same-seed device) wallet, internal transfers lose their identity in the history:
ShieldedTxLookup, which mirrors the SDK'sPersistentAssetLockstore — and those rows are live-recorded at execution and do not survive a restore. On a freshly restored mainnet wallet, two July asset locks (0.4 and 0.302 DASH) both showed as zero-amount "Internal Transfer".Sentrows hide their destination. A shielded → shielded send's row pill and detail sheet said only "Sent / Type: Shielded" — the recipient address (which the SDK records as the 43-byte raw Orchard counterparty, live or OVK-recovered by the restore scan) was displayed nowhere, making an old send indistinguishable from an internal transfer.Both were found on a restored wallet whose only shielded history row was a scan-derived
Sent −0.01with no visible destination.What was done?
Asset-lock reconstruction (
ShieldedTxLookup.refresh()): after building the snapshot fromPersistentAssetLockrows, every persistedAssetLocktransaction with no store row gets a reconstructed entry parsed from its raw bytes (consensus truth — parser shared with the raw-tx inspector, whose credit-output parsing was promoted into reusableRawTransactionInspector.assetLockCreditOutputs/p2pkhKeyHashhelpers).The destination is recovered exactly: every credit output pays a one-time address derived from a purpose-specific funding account (identity registration / top-up / invitation, Platform address top-up, shielded top-up — account types 2…7), and those pools survive a restore as
PersistentCoreAddressrows. Matching the credit outputs against the active wallet's persisted funding-account addresses yields the exactFundingType(0…5), so a restored Transparent → Shielded funding renders the full existing Core → Shielded treatment ("Internal Transfer" with the shield route icon,From: Transparent balance/To: Shielded balanceon the detail sheet), Platform top-ups render the Core → Platform treatment, and identity fundings render their named titles. Credit outputs that match no pool (or mix pools) fall back to areconstructedUnknownFundingTypesentinel: the row keeps its generic "Internal Transfer" title but still shows the real locked amount instead of 0.Reconstructed entries carry
statusRaw = -1(outside both the pending window 1…3 and consumed 4), so they never render "Pending", never claim a completion status (lockStatusTextmaps -1 to no Status row), and never feed a recovery resume. Store-backed rows always win over reconstruction.Shielded Sent destination (
fetchShieldedActivity+ShieldedActivityItem):Sententries decode their 43-byte counterparty to the DIP-0018 bech32m display form (0x10type byte + raw address, same encoding as the Receive screen). The row pill shows the shortened address (matching the external unshield/withdrawal treatment) and the detail sheet gainsFrom: Your Shielded balance/To: <copyable address>rows. A Sent row whose counterparty didn't decode keeps the current route-less presentation rather than claiming a destination.What was deliberately NOT done (SDK follow-ups)
The restore scan currently collapses shielded history poorly: on the test wallet it reconstructed the entire multi-transaction history into a single
Sententry whose cmx list aggregates all nine actions across several transactions, attached one nullifier from a different transaction, recorded no fee, and stamped every note and the activity row with the scan-tip block height (411495 on one device, 412108 on the other, for the same on-chain history). Own-shield (Shield/ShieldFromAssetLock) entries were not reconstructed at all, so a restored wallet shows no "Transparent → Shielded" rows for past shields. That is rs-platform-wallet / swift-sdk scan-reconstruction territory and needs a cross-repo fix; this PR fixes what is honestly derivable app-side.How Has This Been Tested?
Manual, on a restored mainnet wallet (fresh iPhone 16 Pro simulator, wallet recovered from seed — the exact state that exhibits both bugs):
m/9'/5'/5'/5'/0) and renders the Core → Shielded route, the 0.4 lock matched the Platform address top-up account (m/9'/5'/5'/4'/0) and renders Core → Platform; no "Pending" pill on either.Sent −0.01row pill shows the shorteneddash1…recipient; its detail sheet shows From/To with the full copyable address.Clean
dashpaybuild verified. (The unit-test target is pre-existing broken; not run.)Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes