Skip to content

feat(platform-wallet): persist DashPay payment history through the persister callback - #4326

Merged
QuantumExplorer merged 18 commits into
v4.2-devfrom
feat/dashpay-payments-persister-callback
Aug 6, 2026
Merged

feat(platform-wallet): persist DashPay payment history through the persister callback#4326
QuantumExplorer merged 18 commits into
v4.2-devfrom
feat/dashpay-payments-persister-callback

Conversation

@QuantumExplorer

@QuantumExplorer QuantumExplorer commented Aug 6, 2026

Copy link
Copy Markdown
Member

Issue being fixed or feature implemented

DashPay payment entries were the one piece of DashPay state that bypassed the persister callback loop. PlatformWalletChangeSet carries payment rows on every store() round (the IdentityEntry.dashpay_payments snapshot that record_dashpay_payment emits, plus the merged dashpay_payments_overlay), but rs-platform-wallet-ffi never projected either — so on iOS store() returned Ok while persisting nothing for payments. That silently defeated the rollback invariant in record_dashpay_payment, which carefully keeps memory == persisted precisely because a dropped Sent entry + memo has no on-chain recovery.

The only durable path was PlatformWalletManager.refreshDashPayPayments (FFI getter → SwiftData upsert), whose sole caller is ContactDetailView.onAppear — i.e. durability depended on the user visiting a screen. Affected writers:

  • live sends from send_payment — the Sent entry and the user's memo were lost permanently if the app was killed before the refresh ran;
  • the pending→confirmed flips from the confirm sweeps;
  • the sent-payment reconstruction sweep — its entries evaporated each relaunch and were re-derived (functional but wasteful).

What was done?

Projected payments through a real persister callback so they persist event-driven like contact requests and profiles. The getter/refresh path is kept as a belt-and-suspenders reconciler. (The alternative — calling refreshDashPayPayments after dashpay_sync — was rejected: it leaves the UI layer deciding when state becomes durable, inverting the Swift-SDK rule that Swift persists, loads, and bridges but does not decide.)

rs-platform-wallet-ffi

  • New on_persist_dashpay_payments_fn slot, appended at the END of PersistenceCallbacks (the repo's established vtable-growth pattern — same as the invitations slot, release_fn, and the txid-enumeration pair). Vtable pin tests updated (24→25 slots non-shielded, 40→41 with shielded) along with the terminal-field assertion.
  • Entry payload (DashpayPaymentPersistEntryFFI) mirrors the load-side PaymentRestoreEntryFFI field-for-field (raw direction/status discriminants, txid/memo C-strings) plus the leading owner identity id, so the write and restore shapes agree by construction. Persist direction needs no paired free fn — Rust owns the buffers for the duration of the call.
  • FFIPersister::store flattens both carriers — the per-identity dashpay_payments snapshots inside changeset.identities and any dashpay_payments_overlay — deduped by (owner, txid) with the overlay winning, and fires the callback once, after the identities callback (so a brand-new owner's row is staged in the same round first). Projecting only the overlay would have missed every live write: record_dashpay_payment emits an IdentityChangeSet, and the overlay only appears on merged rounds.
  • The module doc in dashpay_payment.rs ("Why a getter, not a persister callback") is rewritten: the getter-only rationale predated the restore buffer, the confirm sweeps, and reconstruction, and its premise — "the map already persists through the changeset" — was true of the desktop SQLite persister but never of FFI hosts.

rs-unified-sdk-jni

  • New slot set to None with a comment: Android derives contact attribution from transaction history on reads and doesn't consume PaymentEntry (confirmed by the Android team on the sent-payment reconstruction review), so behaviour there is unchanged.

Swift (swift-sdk)

  • PlatformWalletPersistenceHandler implements the callback by reusing the existing persistDashpayPayments upsert core (keyed (networkRaw, ownerIdentityId, txid), cascade from PersistentIdentity), refactored into a stage-only helper shared by the callback, the refresh reconciler, and a replay path.
  • The pre-existing ordering caveat (silent skip when the owner PersistentIdentity row doesn't exist yet) is handled with replay semantics instead of a drop: a group whose owner isn't resolvable mid-round parks and replays once after the round's commit; parked rows are discarded on rollback (the Rust side rolled the entries back too, so persisting them would fabricate history).
  • The relaunch read half already existed (the payments array on the identity restore buffer) — this PR closes the write half, so Sent entries + memos now survive relaunch without any UI surface appearing.

Parity manifest

  • Added persistence.dashpay_payment_history with verification ids that exist and run (two Swift unit tests, one shared Rust test); kotlin marked not-applicable with the Android-team rationale.

How Has This Been Tested?

  • New Rust test store_projects_dashpay_payments_from_identities_and_overlay: both carriers flatten into one callback fire, (owner, txid) dedup with overlay-wins (a Confirmed flip beats the snapshot's Pending row), memo and null-memo projection, and no fire on a payments-free round. Vtable pin tests pass in both feature sets.
  • New Swift tests in DashPayPaymentPersistenceTests:
    • testChangesetRoundPersistsSentEntryAndRestoreBufferRoundTripsIt — a Sent entry with a memo delivered by the callback round commits atomically and round-trips through the cold-start restore buffer, with no UI refresh involved;
    • testChangesetRoundStatusFlipRepersistsTheSameRow — pending→confirmed re-emits upsert in place, no duplicate;
    • testRowsForAnOwnerStagedLaterInTheRoundReplayAfterCommit — the parked-row replay path;
    • testFailedRoundRollsBackStagedAndParkedPaymentRows — rollback discards both staged and parked rows.
  • cargo fmt --all; cargo test -p platform-wallet --lib (568 passed); cargo test -p platform-wallet-ffi with and without --features shielded; cargo check -p rs-unified-sdk-jni; mac FFI slice rebuilt via build_ios.sh --target mac (header regenerated with the new struct + slot); swift build + full swift test (298 tests, 0 failures). The SwiftExampleApp xcodebuild step at the end of build_ios.sh fails on this machine for the known environmental reason (no iOS-simulator slice in the mac-only build); CI covers the app build.

Note on the vtable append: per the settled position from the sent-payment reconstruction review (maintainer-confirmed), lockstep XCFramework/header/app builds are the supported deployment contract — pre-release, monorepo-only consumers — so an append-at-end slot is not an ABI break for any supported consumer.

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 made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (N/A)

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added durable DashPay payment history on Swift, including restoration after app restarts.
    • Payment records retain status, direction, transaction ID, and memo updates.
    • Payments associated with identities created during a changeset are saved after completion.
  • Bug Fixes

    • Prevented failed changesets from leaving behind payment records.
  • Tests

    • Added coverage for persistence, restoration, updates, deferred identity handling, and rollback behavior.

jeanpierreroma and others added 14 commits August 5, 2026 16:43
Received DashPay payments already recover after a restore-from-seed:
`reconcile_incoming_payments` walks `dashpay_receival_accounts`, which
persist with their UTXOs. The sending direction had no equivalent, so a
restored wallet showed "No payments with this contact yet" for every
contact it had paid — the transactions were on chain and in local
history, just not attributed.

`reconcile_sent_payments_from_tx_history` closes that gap. It walks the
wallet's persisted core transactions, matches outputs against the
addresses derived from each contact's `DashpayExternalAccount`, and
records one `Sent` entry per (owner, contact, txid). Local-only, no
network round-trips, idempotent — an existing entry for a txid is never
overwritten, so the live send path and the incoming reconcile both keep
priority. It runs as a step of `dashpay_sync()` after
`reconcile_incoming_payments`.

Matching reads `record.transaction.output` and compares script pubkeys.
It deliberately does not read `record.output_details`: records handed
back by `get_core_tx_record` are rebuilt from the host's raw transaction
bytes, so only `transaction`, `txid` and `context` carry real data and
the details vec is always empty. Comparing scripts rather than rendered
addresses also sidesteps address-encoding differences.

Contacts are skipped once they have a `Sent` entry, or once swept this
launch. The direction matters: the incoming reconcile runs first, so a
"has any payment with this contact" test would have hidden the outgoing
history of every contact we had also received from. The per-launch
marker is in-memory only and is not set when a persister read or write
failed, so a transient error cannot permanently strand a contact.

Enumerating the wallet's transactions needs a new persistence hook,
`list_wallet_core_txids`, defaulting to an empty list so existing
persisters keep compiling.
CI:
- `cargo fmt` on the long test signature.
- Android's JNI vtable builds `PersistenceCallbacks` by literal, so the two
  new slots have to be named there. Left `None`, matching the existing
  `on_persist_invitations_fn` precedent: Android keeps today's behaviour
  rather than reporting a reconstruction it cannot perform.

Review:
- Append the txid callbacks after `release_fn`. The struct is a C-bound
  vtable whose trailing slots are documented as end-safe; inserting before
  them would shift the layout for hosts built against the previous header.
- Replace the tautological buffer-length check with the check that matters:
  `count * 32` must not overflow and must fit in `isize::MAX` before
  `from_raw_parts` sees it. Adds the missing SAFETY note.
- Do not stamp the per-launch guard when the enumeration came back empty.
  After a restore the recurring sweep can fire before the host has
  repopulated its transaction table, and a zero-txid answer is
  indistinguishable from "nothing to reconstruct" — stamping there ended
  recovery for the rest of the process, the exact symptom this pass exists
  to fix. `..._skips_repeat_empty_sweeps` pinned that behaviour and is
  replaced by two tests: one that an empty enumeration is retried, one that
  a conclusive scan is not repeated.
- Guard the fault-loaded `account.wallet` access in `walletOwnsTransaction`
  the way `loadWalletList` already does; this predicate runs over every
  persisted transaction row, so the exposure is wider.
- Report failures from the txid callback. A nil argument or a failed fetch
  returned 0 with an empty list, which Rust could not tell from an empty
  wallet.
…payment-reconstruction

# Conflicts:
#	packages/rs-unified-sdk-jni/src/persistence.rs
`clippy::type_complexity` is denied workspace-wide and the inline tuple
annotation tripped it. Extracting `OwnerContact` and `ContactScriptIndex`
also gives the script-pubkey keying an obvious place to be explained.
`ffi_capability_projection_has_stable_v1_layout_values` pins the callback
vtable's size and asserts the last-appended field is terminal. Both move
when a slot is added, exactly as they did for invitations and then for
`release_fn`.

The two txid callbacks sit after `release_fn`, so no previously-defined
slot changes offset — which is the property that actually matters for
hosts built against an older header, and the reason growth is only ever
safe at the end.
…ial scans

Addresses the four reconstruction blockers from review:

- Contact eligibility no longer treats one existing Sent entry as
  proof of completion. A write that fails after a sibling write
  succeeded is retried on the next sweep; per-txid dedup already
  makes re-sweeping recorded entries a no-op.

- A listed txid whose record resolves to Ok(None) now marks the scan
  incomplete. The FFI collapses backend failures, missing/undecodable
  tx bytes and pending InstantSend rows into a miss, so a miss on a
  txid the host itself enumerated means "not available yet" — the
  completion guard stays unstamped and the sweep retries.

- Candidate scripts are derived from the contact xpub over the
  historical range (matched index + gap limit, iterated to a fixed
  point) on a pool clone, instead of matching only the addresses the
  restored pool materialized. Payments past the initial gap window
  are now found after restore-from-seed.

- The txid enumeration carries a per-txid spends-wallet-input flag
  computed by the host from persisted TXO rows (inputs tracked under
  a watch-only DashPay external account do not count). Transactions
  the wallet did not fund — a third party paying the watched contact
  address, or incoming payments — are skipped without a record read
  and can no longer fabricate Sent history.

The get_core_tx_record field contract now requires the real decoded
transaction (or None); reconcile_sent_payments reuses the shared
finality helper.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three findings from the review of e3b193c.

**A partially populated transaction table was treated as complete.**
Distinguishing an empty enumeration from a completed one was not enough:
any non-empty prefix still certified the snapshot. The sweep runs on a
timer, so it can read the table while a DashPay rescan is still
delivering rows into it — one early row makes `txid_count > 0`, the
per-launch guard gets stamped, and every row arriving afterwards is
ignored for the rest of the process.

`reconcile_dashpay_rescan` now records the tip it rewound from, and the
sweep refuses to certify a contact while `synced_height` is still below
that mark. In-memory like `rescan_triggered`, and self-healing for the
same reason: a relaunch restores `synced_height` at its high-water and
re-arms the mark with the re-triggered backfill.

**Legacy TXOs were classified as unfunded.** `PersistentTxo.walletId`
is empty on rows written before the denormalized field existed, and
`loadWalletList` already resolves those through the owning account's
wallet. Both `walletOwnsTransaction` and `walletFundedTransaction`
compared the raw field, so a real spend of an untouched legacy TXO read
as "not ours" — the transaction was skipped and the contact could still
be stamped. Both now share one resolver that prefers the populated field
and falls back to the account's wallet, while still excluding the
watch-only DashPay external account from funded inputs.

**Txid-buffer ownership contradicted its own contract.** The free
callback is documented as transferring ownership on success only, but
the guard was installed before the status check, so a buffer returned
alongside a failure was freed too — a double free for any host that
cleans up its own failed allocation. The guard now goes up after the
check and fires when either output pointer is non-null, so a flags-only
allocation from a malformed success can't leak.
…flag

Two blockers from the review of de9ea9b.

**A partial transaction snapshot could be certified as complete.** The
previous guard was a boolean armed by `reconcile_dashpay_rescan`, but
`sync_wallet_dashpay` runs the reconstruction sweep *before* that
reconcile — so the first post-restore pass saw no target, took whatever
prefix of the transaction table was visible as the whole of history, and
stamped the contact. The same gap remained on an initial or forward-only
scan, where `reconcile_dashpay_rescan` arms nothing at all because there
is no rewind, yet rows keep arriving with each new block.

The marker is now the `synced_height` a sweep certified against, not a
flag. A pass is conclusive for the table as it stood at that height and
says nothing about later blocks, so a contact becomes eligible again the
moment the height advances — which is exactly when new rows can appear.
That needs no "history is settled" signal (nothing in the persistence
callbacks reports one) and removes the ordering dependency entirely: a
rewind lowers the height, so the stamp stops matching whichever reconcile
ran first. In steady state the height stops moving and so does the sweep.

`rescan_backfill_target` existed only to paper over the ordering and is
deleted.

**The derivation walk could not cross a full unused gap.** It extended
the pool only past an address it had already seen paid, so a stretch of
unused indices stopped it. Those stretches are reachable: `send_payment`
marks the chosen contact address used before `build_signed` and does not
roll that back when the build fails, so after enough failures a real
payment sits past a hole no on-chain output bridges — and a restored pool
stops short of it. The seed window is now five gap limits wide and is
derived whether or not the pool already materialized addresses, so the
walk no longer depends on an earlier match.

Both regression tests were verified to fail without their fix.
…ruction' into fix/dashpay-sent-payment-reconstruction
…e itself

Replaces the scan-height guard with a digest of the enumerated
(txid, wallet-funded) rows, and stops failed sends from consuming
payment addresses.

A chain height was never a completeness signal for the persisted
transaction table: the sweep could stamp tip H before the rescan
coordinator rewound below it (excluding the whole backfill until
H+1), the wallet-event adapter commits rows asynchronously behind
the in-memory height, mempool rows arrive with no height advance at
all, and a second height read after the persistence I/O could
certify a height the pass never inspected. Stamping the digest of
exactly the enumeration the pass scanned closes all four: any row
that lands afterwards changes the next enumeration's digest and
re-opens the affected contacts, whichever order the reconciles ran
in, and there is no second read to race. It also ends the
scan-per-block regression — an unchanged table costs later sweeps
one txid enumeration and zero record reads, and a block with no new
wallet rows re-triggers nothing.

The five-gap seed window guarded against failed sends marching the
next derivation index past any recoverable range, but nothing
bounded the march. Bound it at the source instead: a failed
build_signed now returns the consumed address to the pool (state ->
Available) before anything was persisted or broadcast, restoring
the BIP44 invariant that used indices chain within the gap limit.
The seed window stays as tolerance for histories written before
this fix.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rsister callback

DashPay payment entries were the one piece of DashPay state that
bypassed the persister callback loop: FFI hosts' store() returned Ok
while persisting nothing for payments, silently defeating
record_dashpay_payment's rollback invariant. Durability depended on
ContactDetailView.onAppear running the getter-backed refresh — a live
send's Sent entry + memo were lost permanently if the app was killed
first, confirm-sweep status flips didn't stick, and reconstruction
re-derived its entries every relaunch.

Append an on_persist_dashpay_payments_fn slot at the end of
PersistenceCallbacks (established vtable-growth pattern; pin tests now
25/41 slots). FFIPersister::store flattens BOTH payment carriers — the
per-identity dashpay_payments snapshots inside changeset.identities
(what live writes actually emit) and any merged
dashpay_payments_overlay — deduped by (owner, txid), overlay wins, and
fires after the identities callback so a new owner's row is staged in
the same round first. The entry payload mirrors PaymentRestoreEntryFFI
so the write and restore-buffer shapes agree by construction.

Swift implements the callback over the existing payment upsert core,
refactored into a stage-only helper; a group whose owner identity
isn't resolvable mid-round parks for one post-commit replay instead of
dropping (discarded on rollback — the Rust side rolled back too). The
getter/refresh path stays as a reconciler. JNI sets the slot to None:
Android derives contact attribution from transaction history on reads
and doesn't consume PaymentEntry.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@thepastaclaw

thepastaclaw commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

🕓 Ready for review — next in queue (commit 1c70311)
Queue position: 1/1

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d749d642-2f1e-444e-a0b7-27bc8393df04

📥 Commits

Reviewing files that changed from the base of the PR and between 3fe84b3 and 1c70311.

📒 Files selected for processing (1)
  • docs/sdk/sdk-parity-manifest.json
🚧 Files skipped from review as they are similar to previous changes (1)
  • docs/sdk/sdk-parity-manifest.json

📝 Walkthrough

Walkthrough

DashPay payment persistence now uses a Rust FFI callback to deliver payment overlay rows. Swift stages rows during changesets, commits them after success, restores them on startup, and removes them after rollback. Android leaves the callback unset.

Changes

DashPay payment persistence

Layer / File(s) Summary
Payment overlay and FFI row contract
packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs, packages/rs-platform-wallet-ffi/src/dashpay_payment.rs, packages/rs-platform-wallet-ffi/src/identity_persistence.rs
Payment recording adds one overlay row for each changed transaction. The FFI flattens overlay rows into callback-scoped entries and preserves payment fields and string lifetimes.
Rust persistence callback
packages/rs-platform-wallet-ffi/src/persistence.rs
FFIPersister::store emits only nonempty payment overlays after identity persistence. Callback failures mark the round unsuccessful. ABI and projection tests cover the callback contract.
Swift persistence integration
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift, packages/swift-sdk/SwiftTests/SwiftDashSDKTests/DashPayPersistenceTests.swift
Swift groups callback rows by owner, stages upserts, defers unresolved owners, commits successful changesets, restores persisted rows, and rolls back staged and parked rows. Tests cover these paths and in-place status updates.
Platform bindings and parity records
packages/rs-unified-sdk-jni/src/persistence.rs, docs/sdk/sdk-parity-manifest.json
Android sets the payment callback slot to None. The parity manifest records the shared callback symbol and the DashPay payment history capability.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant RustFFIPersister
  participant SwiftPersistenceHandler
  participant SwiftData
  participant WalletLoader
  RustFFIPersister->>SwiftPersistenceHandler: send payment overlay rows
  SwiftPersistenceHandler->>SwiftData: stage owner-scoped upserts
  SwiftPersistenceHandler->>SwiftData: commit after successful changeset
  WalletLoader->>SwiftData: load persisted payment history
  SwiftData-->>WalletLoader: restore payment rows
Loading

Possibly related PRs

  • dashpay/platform#4300: Reconstructs missing sent payments from wallet transaction history while this change adds callback-based persistence.

Suggested reviewers: shumkov, lklimek

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: persisting DashPay payment history through the platform wallet persister callback.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/dashpay-payments-persister-callback

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

@thepastaclaw

Copy link
Copy Markdown
Collaborator

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 `@docs/sdk/sdk-parity-manifest.json`:
- Around line 1042-1054: Update the manifest entry for DashPay payment
persistence by adding on_persist_dashpay_payments_fn to shared_symbols and
persistence.dashpay_payment_history.shared_apis. Because loadWalletList() does
not validate process death, change Swift restart to required, set the SDK status
to partial, and provide a reason describing the missing process-death coverage.

In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- Around line 2526-2531: Update the drop log in persistDashpayPayments to log
only the owner identity’s first eight bytes, matching the truncation convention
used by persistContacts and upsertIgnoredSender, while preserving the existing
warning context and payment count.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: da6f8a17-9d16-4137-942a-fb5d10f94c0d

📥 Commits

Reviewing files that changed from the base of the PR and between b55002f and c36d0b8.

📒 Files selected for processing (7)
  • docs/sdk/sdk-parity-manifest.json
  • packages/rs-platform-wallet-ffi/src/dashpay_payment.rs
  • packages/rs-platform-wallet-ffi/src/identity_persistence.rs
  • packages/rs-platform-wallet-ffi/src/persistence.rs
  • packages/rs-unified-sdk-jni/src/persistence.rs
  • packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift
  • packages/swift-sdk/SwiftTests/SwiftDashSDKTests/DashPayPersistenceTests.swift

Comment thread docs/sdk/sdk-parity-manifest.json Outdated
Register on_persist_dashpay_payments_fn in the parity manifest's
shared_symbols and the capability's shared_apis; downgrade the Swift
host to partial with restart required — the loadWalletList() round-trip
test exercises the write-then-restore loop in-process against an
in-memory container, which does not validate a real process death.
Truncate the parked-row drop log to the owner id's first eight bytes,
matching the file's identifier-logging convention.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@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.

Preliminary review — Codex only

The normal Rust projection and Swift staging path correctly connect DashPay payment changesets to durable storage, but the deferred-owner path performs a second commit after the atomic changeset has already committed, allowing payment loss to be reported as success. The callback also reprocesses an identity's complete payment history on every relevant snapshot, violating the persistence trait's bounded-work guidance as history grows.
Source: reviewers gpt-5.6-sol (general, rust-quality, and ffi-engineer); final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol was orchestration-only and is not reviewer evidence.

Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

  • Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — rust-quality (completed), gpt-5.6-sol — ffi-engineer (completed)
  • Verifier: gpt-5.6-sol — verifier
  • Opus: not run (deferred by blocker gate)

🔴 1 blocking | 🟡 1 suggestion(s)

🤖 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/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:1377-1384: Deferred payments are persisted after the round has already committed
  `endChangeset` first commits the round with `backgroundContext.save()` and only then calls `replayDeferredPaymentUpserts`, which stages parked rows and performs a separate save at lines 2534-2539. A termination between those saves leaves the identity and other round data durable while losing its payment rows. The replay helper also catches its own save failure and drops groups whose owners remain unresolved, after which `endChangeset` still returns `true`; the C callback therefore reports success and Rust retains the in-memory payment state even though Swift did not durably persist it. Resolve and stage deferred rows before the round's single save, failing and rolling back the whole round if an owner remains unresolved or staging fails, or execute both phases inside one actual storage transaction.

In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:1351-1378: Every identity snapshot reprocesses the complete payment history
  Each `IdentityEntry` snapshot carries the identity's complete `dashpay_payments` map, and this projection converts and synchronously sends every row to the host. Consequently, recording N payments performs 1 + 2 + ... + N row projections and host upserts, while unrelated identity mutations continue replaying all N historical rows. This work occurs while `FFIPersister::round_lock` is held and `PlatformWalletPersistence::store` commonly runs under the wallet-manager write lock; the trait explicitly requires per-call work to remain bounded. Emit an incremental payment overlay for the changed `(owner, txid)` row from payment mutators and reserve complete snapshots for bootstrap or newly created identities.

Comment thread packages/rs-platform-wallet-ffi/src/persistence.rs Outdated
Base automatically changed from fix/dashpay-sent-payment-reconstruction to v4.2-dev August 6, 2026 21:42
QuantumExplorer and others added 2 commits August 7, 2026 04:54
…ents-persister-callback

# Conflicts:
#	packages/rs-platform-wallet-ffi/src/persistence.rs
#	packages/rs-platform-wallet/src/changeset/traits.rs
#	packages/rs-platform-wallet/src/wallet/identity/network/payments.rs
#	packages/rs-platform-wallet/src/wallet/persister.rs
#	packages/rs-unified-sdk-jni/src/persistence.rs
#	packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift
…rows inside the atomic round

Address the two thepastaclaw findings:

Blocker — deferred payments persisted after the round committed:
endChangeset now stages parked payment groups BEFORE the round's
single save, so they commit in the same atomic transaction as the
owner identity; a group whose owner is still unresolvable fails the
whole round (rollback + failure to Rust) instead of committing a
lossy persist behind a success report. The post-commit replay helper
and its second save are gone.

Bounded work — every snapshot replayed the full history:
record_dashpay_payment (the single writer for every payment mutation)
now rides exactly the changed (owner, txid) row on
dashpay_payments_overlay, and FFIPersister::store projects ONLY the
overlay — never the full-map IdentityEntry snapshots — so per-round
work is bounded by the delta while the snapshot keeps serving
blob-style persisters unchanged. New platform-wallet test pins the
single-row overlay emission; the FFI test now also pins that a
snapshot-only round does not fire the callback.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 `@docs/sdk/sdk-parity-manifest.json`:
- Line 1044: Declare managed_identity_get_dashpay_payments in the shared_symbols
section of the parity manifest and map it to
packages/rs-platform-wallet-ffi/src/dashpay_payment.rs, so the existing
shared_apis entry is recognized by the validator.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 3d1eb6b5-0e2e-4cda-a42f-3d1b9050b393

📥 Commits

Reviewing files that changed from the base of the PR and between c36d0b8 and 3fe84b3.

📒 Files selected for processing (6)
  • docs/sdk/sdk-parity-manifest.json
  • packages/rs-platform-wallet-ffi/src/dashpay_payment.rs
  • packages/rs-platform-wallet-ffi/src/persistence.rs
  • packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs
  • packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift
  • packages/swift-sdk/SwiftTests/SwiftDashSDKTests/DashPayPersistenceTests.swift
🚧 Files skipped from review as they are similar to previous changes (4)
  • packages/swift-sdk/SwiftTests/SwiftDashSDKTests/DashPayPersistenceTests.swift
  • packages/rs-platform-wallet-ffi/src/persistence.rs
  • packages/rs-platform-wallet-ffi/src/dashpay_payment.rs
  • packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift

Comment thread docs/sdk/sdk-parity-manifest.json
…est's shared_symbols

managed_identity_get_dashpay_payments rode the capability's
shared_apis without a shared_symbols declaration, which the manifest
validator rejects.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@QuantumExplorer
QuantumExplorer merged commit 9840dc5 into v4.2-dev Aug 6, 2026
1 check passed
@QuantumExplorer
QuantumExplorer deleted the feat/dashpay-payments-persister-callback branch August 6, 2026 22:01
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.

3 participants