feat(platform-wallet): reconstruct sent DashPay payments from tx history - #4300
feat(platform-wallet): reconstruct sent DashPay payments from tx history#4300romchornyi wants to merge 8 commits into
Conversation
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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesSent-payment history recovery
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DashPaySync
participant DashPayView
participant WalletPersister
participant SwiftPersistence
participant PaymentStore
DashPaySync->>DashPayView: reconcile_sent_payments_from_tx_history()
DashPayView->>WalletPersister: list_wallet_core_txids()
WalletPersister->>SwiftPersistence: enumerate wallet transaction IDs
SwiftPersistence-->>WalletPersister: transaction IDs and funding flags
WalletPersister-->>DashPayView: decoded transaction records
DashPayView->>PaymentStore: write missing Sent entries
DashPayView-->>DashPaySync: result or logged failure
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: 6
🧹 Nitpick comments (3)
packages/rs-platform-wallet/src/wallet/identity/network/payments.rs (3)
3274-3331: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the read-error and write-failure retry paths.
The tests cover the success and idempotency branches. Two branches decide whether recovery retries after a failure, and both are currently untested:
had_read_errorset by aget_core_tx_recorderror at lines 304-312, which must leave the guard unstamped so the next sweep retries.write_failed_forpopulated by arecord_dashpay_paymentfailure at lines 372-379, which must leave that contact's guard unstamped.A regression in either branch silently converts a transient failure into permanently missing sent history for the launch, with no assertion to catch it.
RecordStorePersisterneeds a failure toggle, in the shape of the existingToggleFailPersister.Do you want me to write these two tests?
🤖 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 `@packages/rs-platform-wallet/src/wallet/identity/network/payments.rs` around lines 3274 - 3331, Add tests for read-error and write-failure retry behavior alongside reconcile_sent_payments_from_tx_history_skips_repeat_empty_sweeps. Extend RecordStorePersister with a failure toggle matching ToggleFailPersister, then verify get_core_tx_record errors leave the reconciliation guard unstamped and cause the next sweep to retry, while record_dashpay_payment failures leave only the affected contact guard unstamped and retry that contact on the next sweep.
600-611: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse this helper in
reconcile_sent_payments.Lines 465-466 inline the same finality test:
record.is_confirmed() || matches!(record.context, TransactionContext::InstantSend(_)). Two copies of the definition of "final" can diverge. Call the helper from both sites.♻️ Proposed refactor at lines 465-466
- let is_final = record.is_confirmed() - || matches!(record.context, TransactionContext::InstantSend(_)); - if !is_final { + if sent_payment_status_for_record(&record) != PaymentStatus::Confirmed { continue; }🤖 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 `@packages/rs-platform-wallet/src/wallet/identity/network/payments.rs` around lines 600 - 611, Update reconcile_sent_payments to call sent_payment_status_for_record for the finality decision instead of duplicating the record.is_confirmed() || InstantSend check, while preserving the existing confirmed and pending behavior. Remove any now-unneeded TransactionContext usage from that call site.
290-333: 🚀 Performance & Scalability | 🔵 TrivialConsider a batched record read if wallet transaction counts grow.
The scan issues one
get_core_tx_recordFFI call per wallet transaction. Each call crosses the C ABI and runs a SwiftData fetch on the host's serial queue. The per-launch guard bounds how often the full scan runs, so the current cost is one pass per launch. On a wallet with thousands of transactions that pass becomes a single long stall on the sync task, and the host queue is blocked for its duration.If transaction counts grow, a batched enumeration that returns records (not just txids) would collapse the round trips. No change is needed for the current volumes.
🤖 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 `@packages/rs-platform-wallet/src/wallet/identity/network/payments.rs` around lines 290 - 333, The scan in reconcile_sent_payments_from_tx_history currently does one get_core_tx_record lookup per txid, which can stall the sync task when wallet histories grow. Update the tx-history reconciliation path to prefer a batched record enumeration from the persister that returns records directly, and reuse that in place of the per-txid loop while keeping the existing txid-only fallback behavior for current volumes. Preserve the current totals, records_read, outputs_scanned, and had_read_error accounting around the new batch path so the reconciliation result stays unchanged.
🤖 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 `@packages/rs-platform-wallet-ffi/src/persistence.rs`:
- Around line 2816-2821: In the txid-buffer handling around get_core_tx_record,
replace the ineffective raw.len comparison with a pre-slice overflow check that
rejects count when count multiplied by 32 cannot be represented safely,
including the isize::MAX bound required by from_raw_parts. Perform this
validation before constructing the slice, and add a SAFETY comment documenting
the pointer, length, and allocation assumptions consistent with the existing
get_core_tx_record pattern.
- Around line 630-657: Move on_list_wallet_core_txids_fn and
on_list_wallet_core_txids_free_fn to positions after
PersistenceCallbacks.release_fn, preserving the existing order and offsets of
all prior fields for older host bindings. Keep their signatures and callback
behavior unchanged.
In `@packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:
- Around line 383-395: Update the recovery guard in
payments::reconcile_sent_payments_from_tx_history so the stamp-and-return path
does not run on an empty txid enumeration from list_wallet_core_txids. Treat a
successful but zero-length txid result as inconclusive, like a read error, and
keep retrying instead of marking every eligible contact in eligible_contacts as
attempted. Preserve the existing write_failed_for filtering and
managed_identity_mut update flow for real recovery runs, and adjust
reconcile_sent_payments_from_tx_history_skips_repeat_empty_sweeps to reflect the
new empty-enumeration retry behavior.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- Around line 7512-7517: The txid enumeration callback must distinguish failures
from an empty transaction list. In PlatformWalletPersistenceHandler.swift at
lines 7512-7517, return a non-zero status when walletIdPtr, outTxids, or
outCount is nil, and when the fetch handler reports failure. At lines 5781-5791,
replace try? with do/catch, log fetch errors, and propagate an errored flag to
the shim instead of converting failures to an empty result.
- Around line 7512-7517: Update the guard handling required arguments in the
transaction enumeration method to return a non-zero failure code when context,
walletIdPtr, outTxids, or outCount is nil. Preserve the successful zero-result
behavior for valid arguments so Rust distinguishes host wiring failures and
retries reconciliation.
- Around line 34-36: Guard the faulted wallet relationship access inside
PlatformWalletPersistenceHandler’s involvedAccounts predicate so
`wallet.walletId` is only read after safely confirming the relationship is
available, matching the existing defensive pattern used later in the same file.
Update the `walletCoreTxids` filtering path around the
`transaction.involvedAccounts.contains` check to avoid crashing when a persisted
`PersistentAccount` row has an inconsistent or unloaded `wallet` relationship,
and preserve the current true/false matching behavior for valid rows.
---
Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:
- Around line 3274-3331: Add tests for read-error and write-failure retry
behavior alongside
reconcile_sent_payments_from_tx_history_skips_repeat_empty_sweeps. Extend
RecordStorePersister with a failure toggle matching ToggleFailPersister, then
verify get_core_tx_record errors leave the reconciliation guard unstamped and
cause the next sweep to retry, while record_dashpay_payment failures leave only
the affected contact guard unstamped and retry that contact on the next sweep.
- Around line 600-611: Update reconcile_sent_payments to call
sent_payment_status_for_record for the finality decision instead of duplicating
the record.is_confirmed() || InstantSend check, while preserving the existing
confirmed and pending behavior. Remove any now-unneeded TransactionContext usage
from that call site.
- Around line 290-333: The scan in reconcile_sent_payments_from_tx_history
currently does one get_core_tx_record lookup per txid, which can stall the sync
task when wallet histories grow. Update the tx-history reconciliation path to
prefer a batched record enumeration from the persister that returns records
directly, and reuse that in place of the per-txid loop while keeping the
existing txid-only fallback behavior for current volumes. Preserve the current
totals, records_read, outputs_scanned, and had_read_error accounting around the
new batch path so the reconciliation result stays unchanged.
🪄 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: 0e27ff56-9d16-4913-bcf8-cdb07e4b2efc
📒 Files selected for processing (8)
packages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet/src/changeset/traits.rspackages/rs-platform-wallet/src/manager/dashpay_sync.rspackages/rs-platform-wallet/src/wallet/identity/network/payments.rspackages/rs-platform-wallet/src/wallet/identity/state/managed_identity/dashpay.rspackages/rs-platform-wallet/src/wallet/identity/state/managed_identity/mod.rspackages/rs-platform-wallet/src/wallet/persister.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift
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.
|
Pushed 188b486 addressing both CI failures and all five review findings. CI
Review
|
…payment-reconstruction # Conflicts: # packages/rs-unified-sdk-jni/src/persistence.rs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4300 +/- ##
============================================
- Coverage 87.61% 87.61% -0.01%
============================================
Files 2704 2704
Lines 345206 345182 -24
============================================
- Hits 302446 302417 -29
- Misses 42760 42765 +5
🚀 New features to boost your workflow:
|
`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.
|
⛔ Blockers found — Opus deferred (commit 2d352b9) |
`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.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The implementation adds useful sent-payment recovery coverage, but verification confirms five in-scope blockers. The new FFI slots break the existing vtable ABI, while the reconstruction can fabricate sent payments or permanently omit records after partial reads, partial writes, and restores involving more than 20 payment addresses.
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)
🔴 5 blocking
🤖 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/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:683-714: Appending callbacks does not preserve the FFI vtable ABI
Appending fields preserves existing offsets but not binary compatibility for a struct passed through `*const PersistenceCallbacks`. `platform_wallet_manager_create_impl` copies the value with `std::ptr::read(persistence)`, so the new library reads the full 24-slot or 40-slot struct. A host compiled against the previous 22-slot or 38-slot definition allocated only the old extent, making manager creation read beyond that object before it can determine that the new callbacks are unset. Adjacent memory can then be interpreted as callback pointers and invoked. Preserve the old struct size and expose these hooks through a size/version-negotiated v2 structure, a separate extension structure, or a new creation entry point.
In `packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:258-268: One successful write prevents retrying another failed payment
This contact-level guard treats one existing `Sent` entry as proof that the contact's entire outgoing history is complete. If transaction A is persisted successfully and transaction B fails, `write_failed_for` correctly avoids setting the completion marker, but the next sweep skips the contact because A now exists. A restart also restores A and continues skipping B, so the failed entry is permanently stranded despite the retry log. Eligibility must use a genuine completion marker or retry state; the per-txid `contains_key` check later in the loop already provides idempotence for successfully recorded entries.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:306-309: Unavailable listed transactions are treated as a completed scan
A txid returned by `list_wallet_core_txids` followed by `Ok(None)` is not a conclusive absence. The FFI implementation maps callback failures, missing or empty transaction bytes, decode failures, unknown contexts, and every InstantSend record to `Ok(None)`; Swift can also enumerate placeholder rows whose bytes are populated later. This arm leaves `had_read_error` false, so a nonempty enumeration stamps every eligible contact as completed and prevents later sweeps from reconsidering the unavailable record. This also exposes the broader contract mismatch: `get_core_tx_record` documents that `transaction` may be a placeholder, but this reconstruction requires a complete decoded output list. Treat unavailable listed records as an incomplete scan or introduce a dedicated history API that guarantees a decoded transaction.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:270-277: Freshly rebuilt accounts only search the first 20 payment addresses
The candidate index contains only scripts currently materialized in the external account pool. `ManagedCoreFundsAccount::from_account` initializes a `DashpayExternalAccount` with 20 addresses, and live sends derive index 20 and above only after prior addresses have been marked used. After a seed restore that rebuilds the contact account without its historical pool state, persisted or rescanned transaction history can contain payments to later indices while the candidate set contains only indices 0 through 19. The sweep then records the early matches and marks the contact complete, permanently omitting later payments. Derive a sufficient historical range from the contact xpub or restore/reconstruct the pool's used range before setting the completion marker.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:5805-5807: Incoming and third-party transactions can be recorded as sent payments
`walletOwnsTransaction` includes transactions through wallet outputs and `involvedAccounts`, not only transactions spending wallet-owned inputs. A `DashpayExternalAccount` is watch-only but is matched whenever an output pays one of the contact's derived addresses, so an unrelated third-party transaction paying that contact is persisted as wallet-involved. An incoming transaction that pays both this wallet and the contact is also included. The Rust sweep then assumes every matching contact output was funded by this wallet and records it as `Sent`, producing false payment history. Enumeration or the returned record must carry reliable per-wallet outgoing ownership, and reconstruction must reject transactions without a wallet-owned input.
| /// Enumerate the persisted Core txids that belong to `wallet_id`. | ||
| /// | ||
| /// Appended at the END so the struct layout stays stable — a host | ||
| /// built against the previous vtable keeps working, it simply never | ||
| /// sets these two slots. | ||
| /// | ||
| /// Used by DashPay sent-payment reconstruction to walk the local | ||
| /// transaction history without requiring the optional in-memory | ||
| /// `transactions()` map to retain finalized records. | ||
| /// | ||
| /// Output contract: | ||
| /// - Set `*out_txids` to a contiguous buffer of `32 * *out_count` | ||
| /// bytes, one raw-wire txid per 32-byte chunk, and `*out_count` | ||
| /// to the number of txids returned. | ||
| /// - Set `*out_txids = null` and `*out_count = 0` when no rows | ||
| /// exist for the wallet. | ||
| /// - Return `0` on success; non-zero values are treated as backend | ||
| /// failures by the Rust side. | ||
| pub on_list_wallet_core_txids_fn: Option< | ||
| unsafe extern "C" fn( | ||
| context: *mut c_void, | ||
| wallet_id: *const u8, | ||
| out_txids: *mut *const u8, | ||
| out_count: *mut usize, | ||
| ) -> i32, | ||
| >, | ||
| /// Paired free callback for the txid buffer returned by | ||
| /// [`Self::on_list_wallet_core_txids_fn`]. Rust invokes this with | ||
| /// the same pointer and txid count, exactly once per successful | ||
| /// hit. | ||
| pub on_list_wallet_core_txids_free_fn: | ||
| Option<unsafe extern "C" fn(context: *mut c_void, txids: *const u8, count: usize)>, |
There was a problem hiding this comment.
🔴 Blocking: Appending callbacks does not preserve the FFI vtable ABI
Appending fields preserves existing offsets but not binary compatibility for a struct passed through *const PersistenceCallbacks. platform_wallet_manager_create_impl copies the value with std::ptr::read(persistence), so the new library reads the full 24-slot or 40-slot struct. A host compiled against the previous 22-slot or 38-slot definition allocated only the old extent, making manager creation read beyond that object before it can determine that the new callbacks are unset. Adjacent memory can then be interpreted as callback pointers and invoked. Preserve the old struct size and expose these hooks through a size/version-negotiated v2 structure, a separate extension structure, or a new creation entry point.
source: ['codex']
There was a problem hiding this comment.
Resolved in e3b193c — Appending callbacks does not preserve the FFI vtable ABI 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.
| // Only a `Sent` entry proves this contact's outgoing history is | ||
| // already present. `reconcile_incoming_payments` runs first and | ||
| // records `Received` entries, so testing for "any payment with | ||
| // this contact" hid every send to a contact we had also | ||
| // received from — the incoming pass filled the map, this guard | ||
| // read it as done, and the sends were never reconstructed. | ||
| if managed.dashpay().payments.values().any(|payment| { | ||
| payment.counterparty_id == contact | ||
| && payment.direction == PaymentDirection::Sent | ||
| }) { | ||
| continue; |
There was a problem hiding this comment.
🔴 Blocking: One successful write prevents retrying another failed payment
This contact-level guard treats one existing Sent entry as proof that the contact's entire outgoing history is complete. If transaction A is persisted successfully and transaction B fails, write_failed_for correctly avoids setting the completion marker, but the next sweep skips the contact because A now exists. A restart also restores A and continues skipping B, so the failed entry is permanently stranded despite the retry log. Eligibility must use a genuine completion marker or retry state; the per-txid contains_key check later in the loop already provides idempotence for successfully recorded entries.
source: ['codex']
There was a problem hiding this comment.
Resolved in e3b193c — One successful write prevents retrying another failed payment 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.
| let record = match self.persister.get_core_tx_record(&txid) { | ||
| Ok(Some(record)) => record, | ||
| Ok(None) => continue, | ||
| Err(e) => { |
There was a problem hiding this comment.
🔴 Blocking: Unavailable listed transactions are treated as a completed scan
A txid returned by list_wallet_core_txids followed by Ok(None) is not a conclusive absence. The FFI implementation maps callback failures, missing or empty transaction bytes, decode failures, unknown contexts, and every InstantSend record to Ok(None); Swift can also enumerate placeholder rows whose bytes are populated later. This arm leaves had_read_error false, so a nonempty enumeration stamps every eligible contact as completed and prevents later sweeps from reconsidering the unavailable record. This also exposes the broader contract mismatch: get_core_tx_record documents that transaction may be a placeholder, but this reconstruction requires a complete decoded output list. Treat unavailable listed records as an incomplete scan or introduce a dedicated history API that guarantees a decoded transaction.
| let record = match self.persister.get_core_tx_record(&txid) { | |
| Ok(Some(record)) => record, | |
| Ok(None) => continue, | |
| Err(e) => { | |
| Ok(None) => { | |
| had_read_error = true; | |
| tracing::warn!( | |
| %txid, | |
| "reconcile_sent_payments_from_tx_history: listed tx record is unavailable; will retry next sweep" | |
| ); | |
| continue; | |
| }, |
source: ['codex']
There was a problem hiding this comment.
Resolved in e3b193c — Unavailable listed transactions are treated as a completed scan 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.
| let txids = rows.compactMap { tx in | ||
| Self.walletOwnsTransaction(walletId: walletId, transaction: tx) ? tx.txid : nil | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Incoming and third-party transactions can be recorded as sent payments
walletOwnsTransaction includes transactions through wallet outputs and involvedAccounts, not only transactions spending wallet-owned inputs. A DashpayExternalAccount is watch-only but is matched whenever an output pays one of the contact's derived addresses, so an unrelated third-party transaction paying that contact is persisted as wallet-involved. An incoming transaction that pays both this wallet and the contact is also included. The Rust sweep then assumes every matching contact output was funded by this wallet and records it as Sent, producing false payment history. Enumeration or the returned record must carry reliable per-wallet outgoing ownership, and reconstruction must reject transactions without a wallet-owned input.
source: ['codex']
There was a problem hiding this comment.
Resolved in e3b193c — Incoming and third-party transactions can be recorded as sent payments 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.
| let pools = account.managed_account_type().address_pools(); | ||
| let Some(pool) = pools.first() else { | ||
| continue; | ||
| }; | ||
| eligible.push((owner, contact)); | ||
| for address_info in pool.addresses.values() { | ||
| out.entry(address_info.script_pubkey.clone()) | ||
| .or_insert((owner, contact)); |
There was a problem hiding this comment.
🔴 Blocking: Freshly rebuilt accounts only search the first 20 payment addresses
The candidate index contains only scripts currently materialized in the external account pool. ManagedCoreFundsAccount::from_account initializes a DashpayExternalAccount with 20 addresses, and live sends derive index 20 and above only after prior addresses have been marked used. After a seed restore that rebuilds the contact account without its historical pool state, persisted or rescanned transaction history can contain payments to later indices while the candidate set contains only indices 0 through 19. The sweep then records the early matches and marks the contact complete, permanently omitting later payments. Derive a sufficient historical range from the contact xpub or restore/reconstruct the pool's used range before setting the completion marker.
source: ['codex']
There was a problem hiding this comment.
Resolved in e3b193c — Freshly rebuilt accounts only search the first 20 payment addresses 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.
|
Notes from the Android side — we hit the same class of problem, so two things that may be useful here. No action needed from us: the 1. Blocker: We ran into exactly this. Matching a transaction because it pays a contact's watch-only derived address is true for any payer, so an incoming payment (or a third party paying that same contact) matches too. What worked for us was to stop asking "does this tx touch the contact's address" and instead compute a signed net per party, explicitly excluding the contact's watch-only external account from both sides of the sum. On Android that's the DashPay external account (type 13): its TXOs are deliberately excluded from both the sent and received sums, with an address-level fallback for rows where the account id is NULL. Direction then falls out of the sign of the net rather than out of address membership, so an incoming payment can't be misfiled as Reference, if it helps: 2. ABI caveat on the vtable extension
Android is immune — One design note, offered only as a data point: because Android recomputes attribution from the transaction store on every read (checking both the sending and receiving friend key chains), it has no persisted-cache asymmetry to lose on restore — the cache is derived, not a source of truth. That's a different trade-off from the |
On the vtable ABI@thepastaclaw @bfoss765 — you both flagged this and the facts aren't in dispute. Two things worth putting on the record. This is a property of the struct, not of this change. The same growth happened twice already — Who is exposed today. Android builds Recommendation: accept the growth here, consistent with the two prior appends, and track versioned negotiation separately so it covers every future slot at once rather than just these two. A size/version-negotiated struct or a new creation entry point changes the contract for every host, which is its own review, not a rider on a DashPay payment-history fix. If you'd rather hold this PR until that structure exists, that's a fine call too — but then the enumeration callback should be designed together with the per-transaction ownership data the sweep needs (the Either way I'd like it to be a decision rather than an omission, so I'm leaving the call to maintainers. Meanwhile the two "concluded more than the evidence supports" findings are fixed in 82dd151, and I'm working the two correctness blockers that don't depend on this: false |
|
Thanks for making the tradeoff explicit. I agree the underlying extensibility flaw predates this PR, but this change still creates a new incompatible size boundary: a host built against the current header can be over-read when paired with the new library. The prior appends are precedent for the pattern, not evidence that another append is safe. From my review perspective this remains blocking unless a Platform maintainer explicitly confirms that lockstep XCFramework/header/app builds are the supported ABI contract and accepts no cross-version compatibility, or the callback surface gains size/version negotiation (ideally together with the ownership data now being reworked). The PR head is still |
…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>
|
Pushed e3b193c addressing the review blockers. Partial writes stranded a sibling payment (payments.rs:258-268) — Fixed. Eligibility no longer consults the payment map at all; it is gated only by the per-launch
Only the first 20 addresses searched after restore (payments.rs:270-277) — Fixed. The sweep now clones the contact pool, and extends it from the contact xpub ( Incoming/third-party transactions recorded as sent (PlatformWalletPersistenceHandler.swift:5805-5807) — Fixed. The (new-in-this-PR) enumeration callback now returns a per-txid flags buffer alongside the txids; bit Appending callbacks breaks the FFI vtable ABI (persistence.rs:683-714) — Not changed, deliberately. There is no supported deployment where a host compiled against the old 22/38-slot struct loads this library: every consumer of Also applied CodeRabbit's suggestions: the read-error/write-failure retry paths are now covered by the two tests above, and Verified: |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-platform-wallet-ffi/src/persistence.rs (1)
2829-2841: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDefine buffer ownership when the callback returns non-zero.
_txid_guardis constructed before therccheck, so Rust invokeson_list_wallet_core_txids_free_fneven when the callback reported failure. The sibling contracts differ:on_load_wallet_list_fndocuments "on failure Rust does not call the free callback", andloadbuilds itsLoadGuardonly after therccheck (lines 2099-2110). The new doc at lines 719-722 says "exactly once per successful hit", which contradicts the code.A host that writes both buffers and then returns non-zero, and frees them itself, would double-free. The in-repo Swift shim leaves
outTxidsnil on every failure path, so it is unaffected today.Pick one semantic and make the code and the doc agree. Either move the guard construction after the
rccheck, or state in the callback doc that Rust frees whatever pointers the host wrote, including on failure.Also note the narrower gap in the guard itself: it keys on
self.txids.is_null(), so a host that sets onlyflags_ptrleaks the flags buffer.🛡️ Proposed fix: construct the guard after the status check
- let _txid_guard = TxidBytesGuard { - txids: txids_ptr, - flags: flags_ptr, - count, - free_fn: self.callbacks.on_list_wallet_core_txids_free_fn, - ctx: self.callbacks.context, - }; - if rc != 0 { return Err(PersistenceError::backend(format!( "on_list_wallet_core_txids_fn returned non-zero status {rc}" ))); } + let _txid_guard = TxidBytesGuard { + txids: txids_ptr, + flags: flags_ptr, + count, + free_fn: self.callbacks.on_list_wallet_core_txids_free_fn, + ctx: self.callbacks.context, + };🤖 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 `@packages/rs-platform-wallet-ffi/src/persistence.rs` around lines 2829 - 2841, Move the TxidBytesGuard construction in the callback handling flow to after the rc != 0 error check, matching the ownership contract that Rust does not invoke the free callback on failure. Update the associated callback documentation to state that buffers are freed exactly once only for successful results, and adjust TxidBytesGuard cleanup to release flags_ptr independently when txids_ptr is null so a flags-only allocation cannot leak.
🤖 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 `@packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:
- Around line 413-442: Set incomplete_scan before the range-walk loop for each
window whose key_source cannot derive, including empty pools and pools with no
observed matches. Preserve the existing warning and once-per-window behavior,
then remove the now-redundant !window.key_source.can_derive() branch inside the
loop while leaving derivable pools’ range-walk behavior unchanged.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- Around line 5805-5813: Update walletFundedTransaction to resolve each TXO’s
wallet ID through its linked account wallet when txo.walletId is empty, matching
the fallback used by loadWalletList. Compare the resolved ID with walletId while
preserving the existing external-account exclusion and current behavior for
populated walletId values.
---
Outside diff comments:
In `@packages/rs-platform-wallet-ffi/src/persistence.rs`:
- Around line 2829-2841: Move the TxidBytesGuard construction in the callback
handling flow to after the rc != 0 error check, matching the ownership contract
that Rust does not invoke the free callback on failure. Update the associated
callback documentation to state that buffers are freed exactly once only for
successful results, and adjust TxidBytesGuard cleanup to release flags_ptr
independently when txids_ptr is null so a flags-only allocation cannot leak.
🪄 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: 4bb94c2a-f618-457b-bfaa-99fb7866fbe4
📒 Files selected for processing (6)
packages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet/src/changeset/mod.rspackages/rs-platform-wallet/src/changeset/traits.rspackages/rs-platform-wallet/src/wallet/identity/network/payments.rspackages/rs-platform-wallet/src/wallet/persister.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift
|
Note for reviewers: the "Kotlin SDK build + tests" failure on this run is pre-existing v4.2-dev breakage, not this PR — #4015 renamed a Swift test without updating the SDK parity manifest, so the check fails on every PR against the current base. Fix is up in #4304 (CI green); once it merges, re-running the Kotlin job here will clear it. Everything else on this PR's latest commit is green. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Carried-forward prior findings: none remain actionable—four are fixed, and the FFI ABI concern is intentionally deferred under the maintainer-confirmed lockstep deployment contract. Latest delta: two blocking recovery gaps remain around partially populated transaction history and legacy TXO wallet attribution, plus one FFI ownership suggestion; the missing-xpub report is refuted by the supported account-pairing invariant, and all 11 focused Rust reconstruction tests pass but do not cover the retained edge cases.
Source: reviewers gpt-5.6-sol (general, rust-quality, ffi-engineer); verifier gpt-5.6-sol; openclaw-agent coordinator is orchestration-only.
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)
🔴 2 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/rs-platform-wallet/src/wallet/identity/network/payments.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:531-553: A partially populated transaction table is treated as complete
The completion condition distinguishes an empty enumeration from a completed scan, but any nonempty prefix is still treated as conclusive. The code and its regression test explicitly acknowledge that `dashpay_sync()` can run while the host is repopulating transaction history after restore. If one historical row exists when this snapshot is fetched but later rows have not arrived yet, every listed record can be read successfully and `txid_count > 0` causes the contact to be marked attempted. Rows added after that snapshot are then ignored for the rest of the process because the in-memory guard suppresses future sweeps. Completion needs an explicit host/core-history completeness signal, a transaction-history generation or high-water mark, or another mechanism that cannot certify a snapshot while restoration is still adding rows.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:5805-5812: Legacy TXOs are classified as unfunded after restore
`PersistentTxo.walletId` defaults to empty for rows migrated from before the denormalized field existed, and `loadWalletList` already resolves those rows through `txo.account.wallet.walletId`. `walletFundedTransaction` instead compares only the raw denormalized field, so a real spend of an untouched legacy TXO is reported as not wallet-funded. Rust skips that transaction and can still stamp the contact as swept, leaving its sent payment absent for the process lifetime. The earlier `walletOwnsTransaction` helper has the same raw-field-only checks for outputs, inputs, and pending inputs, so a transaction composed entirely of legacy TXOs can be filtered out before the funding classifier runs. Use one shared wallet-resolution helper in both ownership selection and funding classification: prefer a populated `txo.walletId`, otherwise resolve through the linked account wallet, while continuing to exclude DashPay external account type 13 from wallet-funded inputs.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:2822-2841: Align txid-buffer cleanup with callback failure ownership
The callback documentation says Rust invokes the paired free callback exactly once for a successful result, but `TxidBytesGuard` is installed before `rc` is checked, so a non-null buffer returned with a failure status is also freed. That conflicts with a host following the documented success-only ownership transfer and can double-free if the host retains responsibility for failure-path allocations. The guard also runs only when `txids` is non-null, leaking a flags-only allocation from a malformed successful callback. The in-repository Swift callback returns errors before allocating, so this is not a current supported-host crash, but the new FFI contract should be internally consistent: check `rc` before installing the guard and invoke the free callback when either successful output pointer is non-null.
| // An enumeration that came back empty proves nothing: after a restore | ||
| // the recurring `dashpay_sync()` can fire before the host has finished | ||
| // repopulating its transaction table, and a zero-txid sweep is | ||
| // indistinguishable from a wallet that genuinely has nothing to | ||
| // reconstruct. Stamping the guard there would end recovery for the | ||
| // rest of the process — the exact symptom this pass exists to fix. | ||
| // Retrying costs one enumeration per sweep, without the per-record | ||
| // reads, until the wallet actually has transactions. The same logic | ||
| // gates on `incomplete_scan`: a pass that could not read every | ||
| // wallet-funded record (or could not derive a contact's historical | ||
| // address range) has not proven anything about the records it missed. | ||
| if !incomplete_scan && txid_count > 0 { | ||
| for window in &windows { | ||
| if write_failed_for.contains(&(window.owner, window.contact)) { | ||
| continue; | ||
| } | ||
| let Some(managed) = info.identity_manager.managed_identity_mut(&window.owner) | ||
| else { | ||
| continue; | ||
| }; | ||
| managed | ||
| .dashpay_sent_payment_reconcile_attempted_mut() | ||
| .insert(window.contact); |
There was a problem hiding this comment.
🔴 Blocking: A partially populated transaction table is treated as complete
The completion condition distinguishes an empty enumeration from a completed scan, but any nonempty prefix is still treated as conclusive. The code and its regression test explicitly acknowledge that dashpay_sync() can run while the host is repopulating transaction history after restore. If one historical row exists when this snapshot is fetched but later rows have not arrived yet, every listed record can be read successfully and txid_count > 0 causes the contact to be marked attempted. Rows added after that snapshot are then ignored for the rest of the process because the in-memory guard suppresses future sweeps. Completion needs an explicit host/core-history completeness signal, a transaction-history generation or high-water mark, or another mechanism that cannot certify a snapshot while restoration is still adding rows.
source: ['codex']
There was a problem hiding this comment.
Resolved in 54758c2 — A partially populated transaction table is treated as complete 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.
| static func walletFundedTransaction( | ||
| walletId: Data, | ||
| transaction: PersistentTransaction | ||
| ) -> Bool { | ||
| transaction.inputs.contains { txo in | ||
| txo.walletId == walletId | ||
| && txo.account.map { $0.accountType != dashpayExternalAccountTypeTag } ?? true | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Legacy TXOs are classified as unfunded after restore
PersistentTxo.walletId defaults to empty for rows migrated from before the denormalized field existed, and loadWalletList already resolves those rows through txo.account.wallet.walletId. walletFundedTransaction instead compares only the raw denormalized field, so a real spend of an untouched legacy TXO is reported as not wallet-funded. Rust skips that transaction and can still stamp the contact as swept, leaving its sent payment absent for the process lifetime. The earlier walletOwnsTransaction helper has the same raw-field-only checks for outputs, inputs, and pending inputs, so a transaction composed entirely of legacy TXOs can be filtered out before the funding classifier runs. Use one shared wallet-resolution helper in both ownership selection and funding classification: prefer a populated txo.walletId, otherwise resolve through the linked account wallet, while continuing to exclude DashPay external account type 13 from wallet-funded inputs.
source: ['codex', 'coderabbit']
There was a problem hiding this comment.
Resolved in 54758c2 — Legacy TXOs are classified as unfunded after restore 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.
| impl Drop for TxidBytesGuard { | ||
| fn drop(&mut self) { | ||
| if let (Some(free), false) = (self.free_fn, self.txids.is_null()) { | ||
| unsafe { free(self.ctx, self.txids, self.flags, self.count) }; | ||
| } | ||
| } | ||
| } | ||
| let _txid_guard = TxidBytesGuard { | ||
| txids: txids_ptr, | ||
| flags: flags_ptr, | ||
| count, | ||
| free_fn: self.callbacks.on_list_wallet_core_txids_free_fn, | ||
| ctx: self.callbacks.context, | ||
| }; | ||
|
|
||
| if rc != 0 { | ||
| return Err(PersistenceError::backend(format!( | ||
| "on_list_wallet_core_txids_fn returned non-zero status {rc}" | ||
| ))); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Align txid-buffer cleanup with callback failure ownership
The callback documentation says Rust invokes the paired free callback exactly once for a successful result, but TxidBytesGuard is installed before rc is checked, so a non-null buffer returned with a failure status is also freed. That conflicts with a host following the documented success-only ownership transfer and can double-free if the host retains responsibility for failure-path allocations. The guard also runs only when txids is non-null, leaking a flags-only allocation from a malformed successful callback. The in-repository Swift callback returns errors before allocating, so this is not a current supported-host crash, but the new FFI contract should be internally consistent: check rc before installing the guard and invoke the free callback when either successful output pointer is non-null.
| impl Drop for TxidBytesGuard { | |
| fn drop(&mut self) { | |
| if let (Some(free), false) = (self.free_fn, self.txids.is_null()) { | |
| unsafe { free(self.ctx, self.txids, self.flags, self.count) }; | |
| } | |
| } | |
| } | |
| let _txid_guard = TxidBytesGuard { | |
| txids: txids_ptr, | |
| flags: flags_ptr, | |
| count, | |
| free_fn: self.callbacks.on_list_wallet_core_txids_free_fn, | |
| ctx: self.callbacks.context, | |
| }; | |
| if rc != 0 { | |
| return Err(PersistenceError::backend(format!( | |
| "on_list_wallet_core_txids_fn returned non-zero status {rc}" | |
| ))); | |
| } | |
| impl Drop for TxidBytesGuard { | |
| fn drop(&mut self) { | |
| if let (Some(free), true) = ( | |
| self.free_fn, | |
| !self.txids.is_null() || !self.flags.is_null(), | |
| ) { | |
| unsafe { free(self.ctx, self.txids, self.flags, self.count) }; | |
| } | |
| } | |
| } | |
| if rc != 0 { | |
| return Err(PersistenceError::backend(format!( | |
| "on_list_wallet_core_txids_fn returned non-zero status {rc}" | |
| ))); | |
| } | |
| let _txid_guard = TxidBytesGuard { | |
| txids: txids_ptr, | |
| flags: flags_ptr, | |
| count, | |
| free_fn: self.callbacks.on_list_wallet_core_txids_free_fn, | |
| ctx: self.callbacks.context, | |
| }; |
source: ['codex']
There was a problem hiding this comment.
Resolved in 54758c2 — Align txid-buffer cleanup with callback failure ownership 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.
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.
|
Pushed 54758c2 addressing the three findings on A partially populated transaction table was treated as complete — fixed. Distinguishing empty from non-empty enumeration wasn't enough: any non-empty prefix still certified the snapshot, and the sweep runs on a timer, so one early row is sufficient to stamp the guard while the backfill is still delivering the rest.
Legacy TXOs classified as unfunded — fixed. Added one shared Txid-buffer cleanup vs failure ownership — fixed, and thanks for catching it: that guard placement was mine. It now goes up after the Missing-xpub branch — agreed with your refutation; left as is. Verified: Note the Kotlin job here is still the pre-existing |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (1)
30-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the repository indentation width.
These changed Swift lines use 4-space indentation. The repository EditorConfig requires 2-space indentation. Reindent the changed lines.
As per coding guidelines,
**/*requires 2-space indentation.Also applies to: 1308-1309, 5832-5837
🤖 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 `@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift` around lines 30 - 74, Reindent the changed Swift code to use the repository’s required 2-space indentation instead of 4 spaces. Apply this consistently to the shown resolvedWalletId and walletOwnsTransaction code and the additional changed sections around lines 1308-1309 and 5832-5837, without altering behavior.Source: Coding guidelines
🤖 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.
Nitpick comments:
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- Around line 30-74: Reindent the changed Swift code to use the repository’s
required 2-space indentation instead of 4 spaces. Apply this consistently to the
shown resolvedWalletId and walletOwnsTransaction code and the additional changed
sections around lines 1308-1309 and 5832-5837, without altering behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 13d7dc8b-cc3c-40e4-a0bd-76f82827e921
📒 Files selected for processing (5)
packages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet/src/wallet/identity/network/payments.rspackages/rs-platform-wallet/src/wallet/identity/state/managed_identity/dashpay.rspackages/rs-platform-wallet/src/wallet/identity/state/managed_identity/mod.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/mod.rs
- packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/dashpay.rs
- packages/rs-platform-wallet-ffi/src/persistence.rs
- packages/rs-platform-wallet/src/wallet/identity/network/payments.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
One carried-forward blocking issue remains: sent-payment reconstruction can certify a partial transaction snapshot before the rescan coordinator arms its completeness target. The latest delta correctly fixes legacy TXO attribution and FFI buffer ownership, and no separate latest-delta defect survives verification. The FFI ownership test request is useful hardening but does not identify a current production defect.
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 additional finding(s) omitted (not in diff).
🤖 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/rs-platform-wallet/src/manager/dashpay_sync.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/dashpay_sync.rs:467-489: A partially populated transaction table is treated as complete
`sync_wallet_dashpay` invokes `reconcile_sent_payments_from_tx_history()` before `reconcile_dashpay_rescan()`, which is the only production path that writes `rescan_backfill_target`. On the first pass after contact discovery or restoration, the target is therefore still `None`; any non-empty current transaction prefix can pass the completion condition at `payments.rs:562-580` and stamp `sent_payment_reconcile_attempted`. The subsequent rescan rewinds the wallet and arms the target too late, because later passes exclude that contact at `payments.rs:284-290` before consulting the target. The `synced_height == 0` early return at `payments.rs:111-115` leaves the same gap during an initial genesis scan: once the first transaction appears, a partial scan can be certified without any target. The regression test at `payments.rs:3555-3646` manually preloads `Some(1000)` and therefore does not exercise the production coordinator ordering. Establish a conclusive settled-history signal before reconstruction, including for an initial scan, and add a test that runs the actual coordinator sequence.
5 participants
Issue being fixed or feature implemented
Received DashPay payments already recover after a restore-from-seed:
reconcile_incoming_paymentswalksdashpay_receival_accounts, which persist along with their UTXOs. The sending direction had no equivalent.The result is that a restored wallet shows "No payments with this contact yet" for every contact it had paid. The transactions are on chain and in local history — they simply are not attributed to the contact. The counterparty's wallet, meanwhile, still lists them all, so the data is provably recoverable.
The asymmetry is called out in this file's own comment (
payments.rs:207-208).What was done?
reconcile_sent_payments_from_tx_historywalks the wallet's persisted core transactions, matches outputs against the addresses derived from each contact'sDashpayExternalAccount, and records oneSententry per(owner, contact, txid). It runs as a step ofdashpay_sync(), afterreconcile_incoming_paymentsso the entry with real ground truth wins. Local-only, no network round-trips, idempotent — an existing entry for a txid is never overwritten.Three details worth a reviewer's attention:
Matching reads
record.transaction.output, notrecord.output_details. Records handed back byget_core_tx_recordare rebuilt from the host's raw transaction bytes, so onlytransaction,txidandcontextcarry real data — the details vec is always empty (rs-platform-wallet-ffi/src/persistence.rs). A version of this that readoutput_detailspassed every unit test and matched nothing on device. Comparing script pubkeys rather than rendered addresses also sidesteps address-encoding differences.The skip-guard is direction-aware. A contact is skipped once it has a
Sententry, or once swept this launch. Testing "any payment with this contact" would hide the outgoing history of every contact we had also received from, because the incoming reconcile runs first and fills the map.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. A relaunch retries once for still-empty contacts, which is far cheaper than re-scanning persisted history every 15s forever.
Enumerating the wallet's transactions needs a new persistence hook,
list_wallet_core_txids, defaulting to an empty list so existing persisters keep compiling. The Swift implementation packs only well-formed 32-byte txids and reports how many it packed — the earlier shape could hand Rust an uninitialized slot as a txid.How Has This Been Tested?
cargo test -p platform-wallet— 519 passed, 0 failed.Two of those are regression tests for the defects above, and both were verified to fail without their fix:
reconcile_sent_payments_from_tx_history_matches_without_output_details— builds the record shape the FFI actually returns (output_detailscleared).reconcile_sent_payments_from_tx_history_reconstructs_for_contact_with_received_history— a contact with prior incoming history still gets its sends reconstructed.End to end on an iOS device (testnet), wallet restored from seed with two established contacts:
Six
Sententries recorded, amounts and dates matching the counterparty wallet, and they survive relaunch. Steady-state passes afterwards report no eligible contacts, i.e. no repeated full scans. The receiving side was checked in the same run and is unaffected.Needs the companion app change (dashwallet-ios
fix/bug-28-dashpay-payment-history) to drain the deferred contact-crypto queue — without it a restored wallet has no external accounts for this pass to match against.Breaking Changes
None.
list_wallet_core_txidsis a defaulted trait method.Checklist:
Summary by CodeRabbit
New Features
Bug Fixes