feat: complete dashpay in platform wallet and swift example app#3841
Open
shumkov wants to merge 238 commits into
Open
feat: complete dashpay in platform wallet and swift example app#3841shumkov wants to merge 238 commits into
shumkov wants to merge 238 commits into
Conversation
…earch Seven-agent reviewed spec for completing the full DashPay flow (sync, contact requests, payments, profiles) in the platform wallet + SwiftExampleApp: protocol reference (DIP-9/11/13/14/15), per-layer implementation inventory, 15 prioritized gaps (G1-G15), 5-milestone work plan, Swift UI design with normative interaction states, and a two-tier test plan aligned with the unmerged e2e framework (PR #3549). Backed by 6 source-cited research files, including the cross-client interop desk-check and an on-chain census of all 368 testnet contactRequest documents. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ompact xpub, key-purpose interop Three fixes to the rs-sdk/platform-encryption contact-request layer, each pinned red-to-green: 1. Entropy mismatch (consensus rejection). send_contact_request generated fresh entropy for broadcast while the document id was derived from the creation entropy; drive-abci recomputes the id from the broadcast entropy and rejected EVERY send with InvalidDocumentTransitionIdError. ContactRequestResult now carries the creation entropy and send reuses it. Test: contact_request_result_entropy_derives_returned_id (red: field inexpressible pre-fix; green after). 2. DIP-15 69-byte compact xpub wire format. We encrypted the 107-byte DIP-14 ExtendedPubKey::encode() form (failing our own 96-byte ciphertext check); DIP-15 and both reference mobile clients use fingerprint||chaincode||pubkey = 69 bytes. New compact_xpub_bytes/parse_compact_xpub codec in platform-encryption; the get_extended_public_key callback contract is now the 69-byte compact, validated before encryption. Test: test_encrypt_compact_xpub_is_exactly_96_bytes (+ round-trip and wrong-length rejection). 3. Key-purpose alignment with on-chain reality. Verified against all 368 testnet contactRequests: the dominant mobile cohort references an ENCRYPTION key for BOTH indices (mobile identities carry no DECRYPTION key). The recipient-key assertion now accepts DECRYPTION or ENCRYPTION. Test: recipient_key_purpose_accepts_decryption_and_encryption (red on DECRYPTION-only predicate; green after). BREAKING: the SDK-side get_extended_public_key callback must now return the 69-byte DIP-15 compact form (rs-sdk-ffi C ABI unchanged; caller doc contract tightened). Also enables dashcore/rand in platform-encryption dev-deps — the crate's tests previously failed to compile at all. dash-sdk: 139 lib tests green (mocks,offline-testing); platform-encryption 7/7; rs-sdk-ffi check clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…blish/reconcile, account rebuild Milestone 1 of docs/dashpay/SPEC.md. Makes DashPay sync actually converge to a payable state, recurring, and restore-safe. Each behavior pinned red-to-green (see SPEC.md Part 5 M1 DONE notes for the full test list). - Recurring sync (G12): new DashPaySyncManager (modeled on PlatformAddressSyncManager) drives dashpay_sync() per wallet on the shared cadence/cancel/quiesce machinery — iterating the wallets map, NOT the token registry (which skips zero-token identities). Per-identity log-and-continue pushed into sync_contact_requests. Test: recurring_pass_syncs_every_wallet_including_zero_token_identities. - Establish via sync (G1a): the ingest guard dropped reciprocal requests whose sender we had already sent to — the offline-accept scenario could never establish. Guard relaxed; reciprocals now flow into auto-establish. - Sent-side reconcile (G13): sync now ingests our own on-platform sent requests (idempotent, metadata-preserving merge — naive re-establish wiped alias/note every sweep), and Accept adopts an existing reciprocal instead of re-broadcasting into the unique-index rejection that permanently bricked Accept after restore-from-seed. - Account rebuild sweep (G1b): every established contact missing accounts gets validate-key-indices -> decrypt -> register external account, plus the DashpayReceivingFunds account (previously only created on fresh send, so restore-from-seed left incoming payments invisible). Candidates collected under the write guard, registered after guard drop (tokio RwLock is non-reentrant). - Failure policy (G1c): transient failures retry next sweep; permanent decrypt/parse failures set the new EstablishedContact.payment_channel_broken flag (persisted; FFI accessor added) and stop retrying. Purpose-validation mismatches only log-and-skip. - Reject tombstone (G5 stage 1): rejected requests are tombstoned by (owner, sender, accountReference) — never bare sender, so a rotated request with a bumped accountReference still gets through. New rejected_contact_requests table + ContactChangeSet.rejected. - Receive-side compact xpub (G14): register_external_contact_account parses the 69-byte DIP-15 compact and reconstructs the contact xpub (address-equality pinned by reconstructed_xpub_derives_identical_addresses); legacy 78/107 fallback kept. - Key-purpose envelope (G15, verified on-chain): send prefers the recipient's DECRYPTION key and falls back to ENCRYPTION (mobile identities have no DECRYPTION key); validate_contact_request gains a recipient purpose gate (AUTHENTICATION was silently accepted before) and a purpose_mismatch classification. - Testability seam (G11): DashPaySdkWriter object-safe trait over the SDK write paths; fetch paths use the SDK's built-in mock. platform-wallet: 196 lib + 8 integration tests green (was 170); storage + FFI checks clean; FFI ABI extended by one accessor (established_contact_is_payment_channel_broken). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
shumkov
requested review from
QuantumExplorer,
ZocoLini,
lklimek and
llbartekll
as code owners
Contributor
|
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:
📝 WalkthroughWalkthroughAdds DashPay SPEC/research docs and implements DIP-15 compact-xpub handling, tightened key-purpose validation, rejected-request tombstones and payment-channel-broken tracking, SDK writer seam, recurring DashPay sync manager, incoming-payment recording/reconciliation, FFI extensions (payments/sync/persistence/seed attach), SwiftData models, and SwiftExampleApp UI and tests. ChangesDashPay Spec & Research
Crypto & SDK
Validation, State & Storage
FFI & Persistence
SDK writer seam & wallet integration
Contact flow refactor
Payments, reconciliation & event bridge
Recurring sync manager
Swift SDK and Example App
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
|
✅ Review complete (commit 5ba83b3) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
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/src/wallet/identity/network/contact_requests.rs (1)
806-875:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe accept-adopt check is only local, not platform-aware.
already_reciprocatedis derived from localsent_contact_requests/established_contacts, but the sync code above explicitly allows "received loaded, sent fetch failed" by logging and continuing. In that state the reciprocal already exists on Platform whilealready_reciprocatedis still false here, so this path retries the same(ownerId, toUserId, accountReference)write and gets the unique-index rejection instead of adopting. This needs a platform check here, or a duplicate-send fallback that switches to the adopt path.🤖 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/contact_requests.rs` around lines 806 - 875, The local-only already_reciprocated check (variable already_reciprocated) can be stale; change the flow so before attempting send_contact_request_with_external_signer you either (A) perform a platform check for an existing reciprocal contact request/relationship (use whatever network client/query you have for checking platform contact requests for (ownerId,toUserId,accountReference)) and set already_reciprocated accordingly, or (B) keep the existing local check but add a duplicate-send fallback: catch the unique-index conflict/error returned by send_contact_request_with_external_signer and, on that specific error, log that the reciprocal exists on Platform and run the adopt path (call register_contact_account(&our_identity_id, &sender_id, 0) and treat as success). Reference already_reciprocated, send_contact_request_with_external_signer, and register_contact_account when implementing either fix.
🤖 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/dashpay/research/01-dip-spec.md`:
- Line 131: Several fenced code blocks use plain ``` without a language tag;
update each triple-backtick fence in the document (e.g., the blocks currently
shown as ``` at the indicated locations) to include an explicit language token
(for non-code or prose use `text`, or a specific language like `json`, `bash`,
`markdown` where applicable) so the markdown linter passes; search for all
occurrences of ``` (including the ones noted around 131, 194, 245, 289, 418,
455) and replace them with ```text or the appropriate language identifier.
In `@docs/dashpay/research/02-rust-dashcore-keywallet.md`:
- Line 232: The markdown contains fenced code blocks without language tags;
update the offending triple-backtick fences to include the appropriate language
identifier (e.g., ```rust, ```bash, or ```text) for the code snippets so
markdownlint passes and syntax highlighting works—locate the plain ``` fences in
the document (the blocks referenced in the review) and replace them with
language-tagged fences.
In `@docs/dashpay/research/05-swift-app.md`:
- Line 47: The fenced code block currently uses a bare triple-backtick fence
(```); add a language tag (e.g., ```swift or ```text) immediately after the
opening backticks to satisfy markdownlint and enable proper syntax highlighting
for that block.
In `@docs/dashpay/research/06-interop-desk-check.md`:
- Line 366: The fenced code block uses plain ``` without a language tag; update
the opening fence to include an appropriate language identifier (for example
`http`, `text`, or `bash`) so markdownlint is satisfied and readability
improves—locate the triple-backtick fenced block in the document and add the
language tag immediately after the opening ``` fence.
- Line 24: The table row contains an extra leading column ("2") so it has four
columns while the table header defines three; remove the extra column in the row
that contains "2" (the row with "ECDH shared-key derivation" and the
libsecp256k1 SHA256 expression) so the row matches the 3-column header layout,
keeping the description "ECDH shared-key derivation" and the verdict "**PASS** —
all three stacks compute libsecp256k1-style `SHA256((y[31]&0x1|0x2) ‖ x)`" as
the remaining columns.
In `@docs/dashpay/SPEC.md`:
- Line 111: Several fenced code blocks in SPEC.md are missing language
identifiers; update each triple-backtick fence (``` ) at the noted examples so
they include an appropriate language tag (e.g., change ``` to ```text, ```rust,
or ```swift as appropriate) to satisfy markdownlint and enable correct syntax
highlighting; search for the bare ``` occurrences (including the ones referenced
near the examples) and replace them with language-tagged fences, ensuring
opening and closing fences remain paired.
In `@packages/rs-platform-wallet/src/manager/dashpay_sync.rs`:
- Around line 221-223: The background loop cleanup currently unconditionally
sets this.background_cancel to None (in the block near start()), which can
overwrite a newer token if stop() and start() race; change the logic so the
background thread only clears background_cancel if the stored cancel token it
captured at spawn time still matches the current token in this.background_cancel
(i.e., capture the Arc/ID of the cancel handle when spawning and
compare-before-clearing); apply the same compare-and-clear pattern in the stop()
/ thread-exit cleanup (references: this.background_cancel, start(), stop()) so a
late-exiting old loop cannot null out a replacement token.
In `@packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- Around line 103-118: The sender/recipient key selection currently using
sender_identity.public_keys().iter().find(...) (checking Purpose::ENCRYPTION and
KeyType::ECDSA_SECP256K1) can pick a disabled/rotated key; update the logic to
only consider active/enabled keys (e.g., filter by .enabled() or reuse the
existing enabled-key selection utility used for signing) so
sender_encryption_key and recipient_key_index (the call to
select_recipient_key_index should be updated similarly or replaced) always
reference the current active ENCRYPTION/DECRYPTION ECDSA_SECP256K1 key; ensure
you still call .map(...).ok_or_else(...) and preserve error type
PlatformWalletError::InvalidIdentityData when no active key is found.
- Around line 516-556: collect_account_build_candidates currently skips contacts
when info.core_wallet.accounts.dashpay_external_accounts.contains_key(&key) is
true, which prevents retries if register_contact_account previously failed after
inserting an external entry; remove that gating so contacts with an
incoming_request (incoming.encrypted_public_key and key indices) are always
returned as AccountBuildCandidate (unless payment_channel_broken) to allow
build_contact_accounts -> register_contact_account to retry; specifically, in
collect_account_build_candidates remove or change the has_external
check/continue and rely on contact.incoming_request and payment_channel_broken
to decide inclusion (keep AccountBuildCandidate fields: contact_id,
encrypted_public_key, our_decryption_key_index, contact_encryption_key_index).
- Around line 452-509: parse_contact_request_doc currently only extracts
required fields and drops optional fields encryptedAccountLabel and
autoAcceptProof, causing restores to lose these values; update
parse_contact_request_doc (and thus parse_sent_contact_request_doc which calls
it) to also read props.get("encryptedAccountLabel").and_then(|v: &Value|
v.as_str()).map(|s| s.to_owned()) and props.get("autoAcceptProof").and_then(|v:
&Value| v.as_bytes()).cloned() (or appropriate conversions) and pass them into
ContactRequest::new (or the appropriate constructor/factory) so the
ContactRequest created preserves encryptedAccountLabel and autoAcceptProof
during ingest/reconcile. Ensure the match arm pattern includes these Option
values and the fallback logging remains unchanged.
In
`@packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:
- Around line 116-130: The code removes an incoming request from
self.incoming_contact_requests but the returned ContactChangeSet only records
cs.rejected, so on replay the incoming entry isn't removed; update the change
set returned by the function to also include the incoming-removal for (owner_id,
*sender_id, account_reference) (i.e., add the corresponding removal entry to the
ContactChangeSet alongside cs.rejected) so that replay will delete the
incoming_contact_requests entry when applying the rejection tombstone.
---
Outside diff comments:
In `@packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- Around line 806-875: The local-only already_reciprocated check (variable
already_reciprocated) can be stale; change the flow so before attempting
send_contact_request_with_external_signer you either (A) perform a platform
check for an existing reciprocal contact request/relationship (use whatever
network client/query you have for checking platform contact requests for
(ownerId,toUserId,accountReference)) and set already_reciprocated accordingly,
or (B) keep the existing local check but add a duplicate-send fallback: catch
the unique-index conflict/error returned by
send_contact_request_with_external_signer and, on that specific error, log that
the reciprocal exists on Platform and run the adopt path (call
register_contact_account(&our_identity_id, &sender_id, 0) and treat as success).
Reference already_reciprocated, send_contact_request_with_external_signer, and
register_contact_account when implementing either fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 596c3a94-3c49-4cc0-869e-b392a37c181e
📒 Files selected for processing (38)
docs/dashpay/SPEC.mddocs/dashpay/research/01-dip-spec.mddocs/dashpay/research/02-rust-dashcore-keywallet.mddocs/dashpay/research/03-rs-platform-wallet.mddocs/dashpay/research/04-sdk-and-contract.mddocs/dashpay/research/05-swift-app.mddocs/dashpay/research/06-interop-desk-check.mdpackages/rs-platform-encryption/Cargo.tomlpackages/rs-platform-encryption/src/lib.rspackages/rs-platform-wallet-ffi/src/established_contact.rspackages/rs-platform-wallet-storage/migrations/V001__initial.rspackages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rspackages/rs-platform-wallet-storage/src/sqlite/schema/identities.rspackages/rs-platform-wallet-storage/tests/sqlite_load_reconstruction.rspackages/rs-platform-wallet/src/changeset/changeset.rspackages/rs-platform-wallet/src/changeset/mod.rspackages/rs-platform-wallet/src/lib.rspackages/rs-platform-wallet/src/manager/accessors.rspackages/rs-platform-wallet/src/manager/dashpay_sync.rspackages/rs-platform-wallet/src/manager/mod.rspackages/rs-platform-wallet/src/wallet/apply.rspackages/rs-platform-wallet/src/wallet/identity/crypto/dip14.rspackages/rs-platform-wallet/src/wallet/identity/crypto/validation.rspackages/rs-platform-wallet/src/wallet/identity/network/account_labels.rspackages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rspackages/rs-platform-wallet/src/wallet/identity/network/contacts.rspackages/rs-platform-wallet/src/wallet/identity/network/dashpay_sync.rspackages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rspackages/rs-platform-wallet/src/wallet/identity/network/mod.rspackages/rs-platform-wallet/src/wallet/identity/network/profile.rspackages/rs-platform-wallet/src/wallet/identity/network/sdk_writer.rspackages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rspackages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rspackages/rs-platform-wallet/src/wallet/identity/state/managed_identity/mod.rspackages/rs-platform-wallet/src/wallet/identity/types/dashpay/established_contact.rspackages/rs-platform-wallet/src/wallet/platform_wallet.rspackages/rs-sdk-ffi/src/dashpay/contact_request.rspackages/rs-sdk/src/platform/dashpay/contact_request.rs
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3841 +/- ##
============================================
+ Coverage 87.17% 87.20% +0.03%
============================================
Files 2629 2638 +9
Lines 327221 328005 +784
============================================
+ Hits 285265 286047 +782
- Misses 41956 41958 +2
🚀 New features to boost your workflow:
|
thepastaclaw
suggested changes
Jun 11, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
M1 of the DashPay completion plan: the SDK entropy / DIP-15 compact xpub / key-purpose interop fixes are correct, but six in-scope correctness issues block merge. The most concerning is editing V001 in-place (violates the documented append-only migration policy and bricks DB rehydration for the v4.0.0-beta.4 cohort). Additional blockers: the reject path emits an incomplete ChangeSet (no removed_incoming); the new rejected_contact_requests table is written but never read; transient identity fetches in register_external_contact_account are misclassified as permanent and brick the channel; validation.purpose_mismatch is set even when a hard error is also present, masking permanent failures as retryable; and the sync sweep skips superseding requests from established contacts, making the documented payment_channel_broken recovery path unreachable.
🔴 6 blocking | 🟡 2 suggestion(s)
2 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-storage/migrations/V001__initial.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/migrations/V001__initial.rs:186-213: V001 migration edited in-place violates append-only policy and breaks upgrade from v4.0.0-beta.4
This PR adds `contacts.payment_channel_broken` and a new `rejected_contact_requests` table by editing V001 directly. V001 (without these additions) was already shipped in `v4.0.0-beta.4` (commit da9d3fe84e / schema confirmed via `git show`), and `packages/rs-platform-wallet-storage/README.md:106` explicitly states migrations are append-only and applied by refinery on every `open`. refinery checksums each migration in `refinery_schema_history`; against an existing v4.0.0-beta.4 DB it will either abort with a divergent-checksum error or silently skip V001 (already applied) — in which case neither the new table nor the new column is ever created, and the first runtime write in `contacts.rs:240` (`INSERT INTO rejected_contact_requests …`) or `contacts.rs:194-212` (`payment_channel_broken` column) fails at the SQLite layer. `tc029_migration_fingerprint_stable` does not catch this because it only checks self-stability, not a pinned hash. Add `V002__dashpay_reject_and_broken_channel.rs` doing `ALTER TABLE contacts ADD COLUMN payment_channel_broken INTEGER` and `CREATE TABLE rejected_contact_requests (…)`; the loader at `contacts.rs::load_state` already tolerates NULL `payment_channel_broken`, so a default-less ALTER is compatible.
In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs:109-131: `record_rejected_contact_request` removes incoming in memory but does not emit `removed_incoming`
The function calls `self.incoming_contact_requests.remove(sender_id)` and returns a `ContactChangeSet` populated only with `cs.rejected`. The unified-`contacts`-table writer at `rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:182-193` only `DELETE`s when `cs.removed_incoming` is non-empty, so the previously persisted state='received' row (with the `incoming_request` blob) stays in SQLite. Once `persister.load()` (TODO at `sqlite/persister.rs:909`) is wired up, the unified contacts reader rebuckets that row as an incoming request, `apply_changeset` re-inserts it into `incoming_contact_requests`, and the FFI surfaces the explicitly-rejected request back to the UI. The persisted delta is also internally inconsistent with the in-memory mutation — a delta-persistence invariant violation. The in-memory `rejected_tombstone_round_trips_and_respects_account_reference` test does not catch this because it round-trips via `apply_changeset`, not the SQLite reader. Fix by also inserting a matching `ReceivedContactRequestKey { owner_id, sender_id: *sender_id }` into `cs.removed_incoming`.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:206-222: `rejected_contact_requests` is written but never read — tombstones lost across restart
The PR adds a writer (`contacts.rs:240`), a migration row (`V001__initial.rs:203`), and an `apply_changeset` branch that restores `ManagedIdentity.rejected_contact_requests` from `cs.rejected`. But `managed_identity_from_entry` hard-codes `rejected_contact_requests: Default::default()` (line 214), and grep confirms no `load_state` reader for the new table. Once `persister.load()` (TODO at `sqlite/persister.rs:909`) is wired up, the in-memory tombstone map is always empty after restart even though SQLite holds the rows. `is_request_rejected` then returns `false`, the sweep's tombstone-skip in `network/contact_requests.rs:396-404` does not fire, and the recurring DashPay loop (G12) resurrects every rejected request on the first sweep — exactly the M1 failure mode the SPEC.md cites as the reason G5 must land with G12. Add a per-wallet `load_state` for `rejected_contact_requests` and route its output into the `ContactChangeSet.rejected` synthesized during rehydration.
In `packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:711-728: Transient identity fetch failures inside account-build are marked as permanent
`build_contact_accounts` treats any error from `register_external_contact_account` as permanent and calls `mark_contact_channel_broken`. But `register_external_contact_account` (`network/contacts.rs:400-407`) performs another `Identity::fetch` for the same contact and wraps the DAPI/network error as `PlatformWalletError::InvalidIdentityData`. A transient DAPI hiccup after validation therefore permanently disables the payment channel; subsequent sweeps skip the contact via the `payment_channel_broken` filter at line 530, and recovery only fires if a superseding contactRequest happens to arrive — contradicting the policy in the docstring at lines 573-578 ("Transient (identity fetch / network): logged, left for the next sweep to retry. The broken flag stays clear."). The fix is to perform the contact-identity fetch and treat its failure as transient *before* calling `register_external_contact_account` (mirroring the existing fetch at lines 631-655) and to scope the permanent-broken classification to genuinely non-recoverable failures (decrypt/decode, missing-key, key-type mismatch).
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:383-392: Superseding requests from established contacts are skipped — `payment_channel_broken` recovery is unreachable
The received-side ingest drops every doc whose sender is already in `established_contacts` before consulting `accountReference`. `EstablishedContact::reestablish_preserving_metadata` exists precisely to clear `payment_channel_broken = false` when a fresh request flows in (see `types/dashpay/established_contact.rs:84-104`), and `collect_account_build_candidates` documents the recovery contract at lines 528-529 ("never retry a permanently-broken channel — wait for a superseding request (which clears the flag on re-establish)"). But there is no path that reaches `reestablish_preserving_metadata` for an already-established sender from the sync sweep — `add_incoming_contact_request` is only called for new senders here, and the send-side guard at `state/managed_identity/contact_requests.rs:46-48` similarly returns early for established contacts. Net effect: once `payment_channel_broken` is set, it stays set forever. Either (a) detect a superseding incoming request (new `accountReference` for the same sender) and route it through the reestablish path, or (b) change the broken-channel policy so the next sweep can retry under controlled conditions.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:5733-5749: `select_recipient_key_index` returns disabled keys
The send-side recipient-key selector iterates `recipient_identity.public_keys()` and returns the first key whose purpose is DECRYPTION (then ENCRYPTION) and whose type is ECDSA_SECP256K1, with no `disabled_at` check. `validate_contact_request` in `crypto/validation.rs` does gate on disabled keys, so if a preferred DECRYPTION key has been rotated/disabled this selector returns it anyway and the broadcast fails downstream with an opaque error instead of falling through to a usable ENCRYPTION key on the same identity. Add `&& k.disabled_at().is_none()` to both branches so selection is consistent with validation.
In `packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs:50-62: `purpose_mismatch` is set even when a non-purpose hard error is also present
The docstring at lines 19-29 contracts `purpose_mismatch` as `true` *only* when the sole reason for invalidity is a key-purpose mismatch — it is what tells `build_contact_accounts` at `network/contact_requests.rs:689` to treat the failure as a non-permanent skip instead of marking the channel permanently broken. The implementation does not preserve that invariant: `add_purpose_error` unconditionally sets `purpose_mismatch = true`, and `add_error` never clears it. A request whose key has both a wrong key type (hard, permanent error) and a wrong purpose ends up with `is_valid = false, purpose_mismatch = true`, and the caller skips + retries forever instead of marking broken. The mobile testnet census makes this rare in practice today, but the classifier is the load-bearing primitive the recovery policy is built on — fix `add_error` to clear the flag, and fix `add_purpose_error` to only set it if no prior hard error was recorded.
In `packages/rs-platform-wallet/src/manager/dashpay_sync.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/dashpay_sync.rs:192-227: `stop()` followed quickly by `start()` can let the old thread null out the new cancel token
`stop()` takes the current `background_cancel`, sets it to `None`, then cancels the old token. The spawned thread exits its loop on cancellation and at lines 221-223 re-acquires the guard and writes `*guard = None`. If `start()` runs between `stop()` and the old thread's cleanup block, the old thread's final clear will overwrite the new token just installed by `start()` — leaving `background_cancel` empty while a fresh sync thread is still running, so a subsequent `stop()`/`quiesce()` will be a no-op against that running thread. The normal shutdown path (`quiesce` waits for in-flight passes) does not hit this, but bare `stop()`/`start()` races can. Fix by capturing the token at spawn time and only clearing the guard if it still holds that same token (`if matches!(*guard, Some(t) if Arc::ptr_eq(...)) { *guard = None; }`).
…hout order-by Two devnet-UAT fixes on the rs-sdk side: - contact_request_queries: add explicit `ORDER BY $createdAt` to both fetch_received/fetch_sent queries. Drive answers a bare secondary-index equality (toUserId / $ownerId) with a verified proof of ABSENCE even when matching documents exist — isolated live against devnet with a host-side probe (equality-only: 0 docs; with order-by: found). The order-by binds the query to the (field, $createdAt) index so results return. Worth a platform issue: drive should reject the under-specified query instead of proving absence. - rs-sdk-ffi: 8MB tokio worker stacks. GroveDB document-query proof verification (verify_layer_proof_v1) recurses deep enough to overflow the platform-default stack (SIGBUS on the stack guard, observed on-device). No test: requires a live drive node answering proofs; pinned by the on-device UAT flow (docs/dashpay/SPEC.md Part 7 e2e plan covers it once PR #3549 lands).
…lock
Devnet UAT (2026-06-12) showed the receiver's payment history was
always empty ("Payments (0)") and friendship-account UTXOs were
silently dropped on every relaunch. Three root causes, all fixed:
1. Incoming payments were never recorded: the old
try_record_incoming_payment had ZERO callers. Replaced with
record_incoming_dashpay_payments wired into the wallet-event
adapter (core_bridge) — every TransactionDetected output paying a
DashpayReceivingFunds address now records a Received PaymentEntry
on the owning managed identity, idempotent per txid.
2. No recovery for missed/restored payments: new
reconcile_incoming_payments() derives missing Received entries
from the receival accounts' UTXO sets; runs as a local-only third
step of dashpay_sync() each sweep. Never clobbers an existing
txid entry (e.g. the sender's own Sent record when both
identities share a wallet).
3. DashPay account registrations were in-memory only:
register_contact_account / register_external_contact_account now
persist an AccountRegistrationEntry + initial pool snapshot (same
round shape as wallet creation), emitted BEFORE the in-memory
inserts. Without this the accounts vanished on relaunch and the
UTXO restore dropped their rows (load: dropped_no_account=2
observed live). register_contact_account also gains the missing
early-exit and now mirrors the restored shape into the immutable
wallet.accounts collection.
Tests (red->green demonstrated against the unfixed code):
- register_contact_account_persists_account_registration: FAILED
before (no store round), passes after.
- reconcile_records_received_payments_from_receival_utxos: FAILED
before (stub recorded 0), passes after; also pins idempotency.
- reconcile_does_not_clobber_existing_entry_for_same_txid.
204/204 platform-wallet lib tests green.
Also: attach_wallet_seed manager API + FFI
(platform_wallet_manager_attach_wallet_seed_from_mnemonic) — wallets
rehydrate external-signable after relaunch with the mnemonic still
in the host keychain; this upgrades them in place (idempotent,
SeedMismatch-guarded, BIP44-0 xpub-equality fallback for
pre-network-scoped wallet ids). dashpay-sync loop thread gets an
8MB stack (GroveDB proof recursion SIGBUS, observed on-device).
…payment history
SPEC Part 6 ("nice UI") + M2 tasks 7-11, verified end-to-end on a
devnet: profile create, add contact by id, request/accept,
established contacts, send 0.01 DASH with txid in sender history,
received payments on the recipient's side across relaunches.
FFI (rs-platform-wallet-ffi):
- dashpay_sync.rs: 7 platform_wallet_manager_dashpay_sync_* symbols
(start/stop/sync_now/is_syncing/is_running/interval get+set);
sync_now runs via block_on_worker (8MB worker — GroveDB proof
recursion overflows the caller thread's stack).
- dashpay_payment.rs: managed_identity_get_dashpay_payments getter.
- Persister callback arity 8→10: payment_channel_broken +
contact-request rejection tombstones now cross the boundary.
Swift SDK:
- PersistentDashpayPayment model + persistence bridge;
PersistentDashpayContactRequest gains rejection fields;
PersistentIdentity payment relationship.
- PlatformWalletManagerDashPaySync: start/stop/refresh +
@published dashPaySyncIsSyncing (1 Hz poll, sibling convention).
- Keychain unlock hook in loadFromPersistor: re-attaches the wallet
seed via attach_wallet_seed so rehydrated wallets can sign.
SwiftExampleApp:
- New DashPay root tab (Views/DashPay/, 7 views): identity picker
with @AppStorage persistence, profile header + editor, contacts +
requests segments (incoming accept/reject, outgoing pending),
add-contact (DPNS search + identity-id modes), contact detail
(payments history, local alias/note/hide), send sheet. All §6.4
interaction states; dashpay.* accessibility ids throughout.
- Contacts consolidated into the DashPay tab: legacy FriendsView
(917 lines) deleted; IdentityDetailView's DashPay section now
deep-links to the tab with the identity pre-selected (root tab
selection moved to AppUIState). SendDashPayPaymentSheet +
DashPayContact moved to Views/DashPay/.
- AddContactView guards partial base58 input (<32-byte decode
crashed the FFI identifier precondition).
Tests: DashPayPersistenceTests (15 — persister bridge, tombstone
rotation-survival, payments), DashPayTabUITests (smoke).
Marks M2 + the receiver-side payment path as live-verified (2026-06-12, devnet): account registrations now persisted, incoming payments recorded live + reconciled after restore. Notes the drive query-absence behaviour (equality without order-by proves absence) referenced from the rs-sdk fix.
…detail Contacts live in the DashPay tab now — the redirect row added during the consolidation was an extra menu item with no unique function. The identity screen keeps only identity-owned concerns (keys, DPNS, balance, profile).
Three placement fixes from UI review: - Sync page gains a "DashPay Sync Status" section (spinner while a pass is in flight, relative last-sync stamp from the FFI, Recurring/Stopped state, Sync Now) — the recurring DashPay loop was previously invisible there. - DashPay tab shows "Received from contacts" under the profile header: the active identity's DashpayReceivingFunds balances, read from the same lock-free account-balance call the wallet list uses. - Wallets account list hides the DashPay friendship accounts (tags 12/13): per-contact protocol plumbing that would bloat the list as contacts grow, and external accounts watch the contact's addresses (not our funds). Totals are unaffected — receiving funds already roll into Core Balance (verified live: 9.39698657 = BIP44 9.37698657 + 0.02 received); the Storage Explorer still lists the raw rows. Verified on-sim: sync section shows "Last sync: 5 secs / Recurring"; DashPay tab shows 0.02000000 DASH received; no DashPay rows remain in the Wallets account list.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/swift-sdk/SwiftTests/SwiftDashSDKTests/DashPayPersistenceTests.swift (1)
421-465: ⚡ Quick winAlso assert the payment rows roll back in this atomicity test.
The doc comment says a mid-round
persistDashpayPaymentswrite must ride the open changeset and roll back with it, but the test only checksPersistentDashpayContactRequest. If payment persistence starts auto-saving again, this still passes. Add aPersistentDashpayPaymentfetch before and afterendChangeset(..., success: false)so the regression is pinned end-to-end.🤖 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/SwiftTests/SwiftDashSDKTests/DashPayPersistenceTests.swift` around lines 421 - 465, In testPaymentRefreshDoesNotCommitAnOpenChangesetRound, add assertions that verify the payment row staged by persistDashpayPayments is not visible mid-round and is rolled back after endChangeset(..., success: false); specifically, call the existing payment-fetch helper (or add/rename a fetch function for PersistentDashpayPayment rows) to assert count == 0 immediately after the mid-round persist and again after handler.endChangeset(..., success: false), mirroring the contact-row assertions so the test verifies payment atomicity as well as contact atomicity.
🤖 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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- Around line 1919-1925: persistDashpayPayments is swallowing failures from
backgroundContext.save() via try?, which can silently drop payment-history
updates; change the save to propagate or log errors instead of ignoring them:
replace the try? backgroundContext.save() with a throwing or do/catch path
inside persistDashpayPayments that captures the thrown error from
backgroundContext.save(), records telemetry/logging (or rethrows to the caller)
with context (e.g., include which payment batch or wallet ID), and preserve the
existing inChangeset check (if !self.inChangeset) so the save still only runs
when appropriate; update callers or function signature as needed to handle
propagated errors or ensure telemetry is emitted in the catch.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift`:
- Around line 35-40: The optimisticSentIds and ownProfile state are
identity-scoped but currently persist across identity switches; update the
activeIdentity handling (the Task that observes activeIdentity) to reset
identity-scoped UI state at the start of the task: clear optimisticSentIds and
set ownProfile to nil (or otherwise remove cached profile) before loading;
alternatively refactor optimisticSentIds and ownProfile to be keyed by owner
identity (e.g., a dictionary keyed by activeIdentity.id) and read/write via that
key, and ensure loadOwnProfileFromCache() does not retain the previous profile
on read failure for the new identity but returns nil so the UI doesn’t show the
old identity’s data.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift`:
- Around line 1235-1243: The avatar downloader currently accepts any parseable
URL, allowing non-HTTPS schemes; update fetchAvatarBytes to explicitly validate
the URL scheme and reject anything not exactly "https" before creating the
URLRequest, returning an error (or nil) for non-https inputs; locate
fetchAvatarBytes (and the analogous implementation referenced around lines
1390-1410) and add a guard that checks url.scheme?.lowercased() == "https" and
fails early with a clear error to prevent http or other schemes from being
fetched.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/DashPayTabUITests.swift`:
- Around line 92-97: Replace the immediate existence check on the toolbar
refresh button with a timed wait to avoid flakes: locate the `refresh` query
using `Identifier.refreshButton` in DashPayTabUITests (variable `refresh`) and
change the assertion to call `refresh.waitForExistence(timeout: ...)` instead of
checking `refresh.exists`, keeping the same failure message; choose a reasonable
timeout (e.g., 1–5s) consistent with other tests.
---
Nitpick comments:
In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKTests/DashPayPersistenceTests.swift`:
- Around line 421-465: In testPaymentRefreshDoesNotCommitAnOpenChangesetRound,
add assertions that verify the payment row staged by persistDashpayPayments is
not visible mid-round and is rolled back after endChangeset(..., success:
false); specifically, call the existing payment-fetch helper (or add/rename a
fetch function for PersistentDashpayPayment rows) to assert count == 0
immediately after the mid-round persist and again after
handler.endChangeset(..., success: false), mirroring the contact-row assertions
so the test verifies payment atomicity as well as contact atomicity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c0b4a7c-c449-41c7-bd16-7979ff30c777
📒 Files selected for processing (42)
docs/dashpay/SPEC.mdpackages/rs-platform-wallet-ffi/src/contact_persistence.rspackages/rs-platform-wallet-ffi/src/dashpay_payment.rspackages/rs-platform-wallet-ffi/src/dashpay_sync.rspackages/rs-platform-wallet-ffi/src/lib.rspackages/rs-platform-wallet-ffi/src/manager.rspackages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet/src/changeset/core_bridge.rspackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/manager/attach_seed.rspackages/rs-platform-wallet/src/manager/dashpay_sync.rspackages/rs-platform-wallet/src/manager/mod.rspackages/rs-platform-wallet/src/wallet/identity/network/contacts.rspackages/rs-platform-wallet/src/wallet/identity/network/dashpay_sync.rspackages/rs-platform-wallet/src/wallet/identity/network/mod.rspackages/rs-platform-wallet/src/wallet/identity/network/payments.rspackages/rs-sdk-ffi/src/sdk.rspackages/rs-sdk/src/platform/dashpay/contact_request_queries.rspackages/swift-sdk/Sources/SwiftDashSDK/Persistence/DashModelContainer.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentDashpayContactRequest.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentDashpayPayment.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentIdentity.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/DashPayPayment.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedIdentity.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerDashPaySync.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/ContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/AddContactView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactsView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayContactMeta.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayProfileView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/SendDashPayPaymentSheet.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/DashPayTabUITests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/DashPayPersistenceTests.swift
💤 Files with no reviewable changes (1)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swift
✅ Files skipped from review due to trivial changes (3)
- packages/rs-platform-wallet-ffi/src/lib.rs
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/AddContactView.swift
- docs/dashpay/SPEC.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rs-platform-wallet/src/wallet/identity/network/mod.rs
- packages/rs-platform-wallet/src/manager/mod.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift (1)
23-26:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winScope the persisted active-identity key by network.
dashpay.activeIdentityIdis shared across every network, so selecting an identity on testnet/devnet overwrites the remembered choice for mainnet too. When the user switches back,activeIdentityfalls back to the first eligible identity instead of restoring the last selection on that network.🤖 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/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift` around lines 23 - 26, The persisted AppStorage key stored in DashPayTabView (`@AppStorage("dashpay.activeIdentityId") private var storedIdentityId`) is global across networks; change it to be network-scoped by deriving the key from the current network identifier (e.g., include network.rawValue or chainId) so each network has its own stored key. Update DashPayTabView to compute the AppStorage key at runtime (or use a computed property / wrapper that returns "dashpay.activeIdentityId.\(networkId)") using the view’s network/environment value and ensure storedIdentityId is read/written through that network-scoped key so switching networks preserves separate selections.
♻️ Duplicate comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift (1)
169-177:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset identity-scoped state before loading the next identity.
optimisticSentIdsandownProfilestill survive an identity switch, andloadOwnProfileFromCache()explicitly keeps the previous profile on a read failure. That can render identity A's pending-request overlay or profile header under identity B until the cache catches up. Clear those fields at the start of the.task(id:)block, and don't retain the previousownProfilein the failure path for the new identity.Also applies to: 420-433
🤖 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/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift` around lines 169 - 177, The task block keyed by .task(id: activeIdentity?.identityId) is not resetting identity-scoped state: clear optimisticSentIds and ownProfile immediately at the top of that task before calling loadOwnProfileFromCache() and walletManager.dashPaySyncNow(); and update loadOwnProfileFromCache() so that on a cache read failure for the new identity it does not retain the previous ownProfile (set ownProfile to nil or replace with an empty/default value) instead of keeping the old profile. Ensure the reset refers to the existing properties optimisticSentIds and ownProfile and the loadOwnProfileFromCache() function so the UI doesn't show the previous identity while the new identity's cache is loaded.
🤖 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/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swift`:
- Around line 35-42: The visible-empty-state logic is still checking the raw
accounts collection instead of the filtered/sorted list, causing the UI to hide
the "No Accounts" state when only accountType 12/13 are present; update the
empty-state checks to use orderedAccounts.isEmpty (or introduce a
visibleAccounts computed collection that filters out accountType 12/13 and reuse
it everywhere) and replace any usages of accounts.isEmpty / !accounts.isEmpty in
AccountListView with checks against that filtered collection so the UI matches
the displayed list (keep AccountListView.sortKey as the sorting helper).
---
Outside diff comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift`:
- Around line 23-26: The persisted AppStorage key stored in DashPayTabView
(`@AppStorage("dashpay.activeIdentityId") private var storedIdentityId`) is
global across networks; change it to be network-scoped by deriving the key from
the current network identifier (e.g., include network.rawValue or chainId) so
each network has its own stored key. Update DashPayTabView to compute the
AppStorage key at runtime (or use a computed property / wrapper that returns
"dashpay.activeIdentityId.\(networkId)") using the view’s network/environment
value and ensure storedIdentityId is read/written through that network-scoped
key so switching networks preserves separate selections.
---
Duplicate comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift`:
- Around line 169-177: The task block keyed by .task(id:
activeIdentity?.identityId) is not resetting identity-scoped state: clear
optimisticSentIds and ownProfile immediately at the top of that task before
calling loadOwnProfileFromCache() and walletManager.dashPaySyncNow(); and update
loadOwnProfileFromCache() so that on a cache read failure for the new identity
it does not retain the previous ownProfile (set ownProfile to nil or replace
with an empty/default value) instead of keeping the old profile. Ensure the
reset refers to the existing properties optimisticSentIds and ownProfile and the
loadOwnProfileFromCache() function so the UI doesn't show the previous identity
while the new identity's cache is loaded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5cf1916b-35bc-47ca-bb9d-48b3d9493945
📒 Files selected for processing (4)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift
💤 Files with no reviewable changes (1)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift
thepastaclaw
suggested changes
Jun 12, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
All 8 prior findings against 9f770b8 remain STILL VALID at a51606d — verified directly against the worktree (V001 unchanged, record_rejected_contact_request still omits removed_incoming, no reader for rejected_contact_requests, transient identity fetch still permanently breaks channels, purpose_mismatch still sticky, established-contact ingest still skips superseding requests, dashpay_sync cleanup still clobbers cancel token unconditionally, select_recipient_key_index still ignores disabled_at). The M2 delta also introduced one new blocker: Swift wallet deletion does not pre-delete the newly added PersistentDashpayPayment children whose owner inverse is non-optional, mirroring the contact-request pattern that the surrounding comment explicitly calls out as fatal. One FFI suggestion is worth flagging: the new DashpayPaymentFFI derives Copy despite owning two *mut c_char allocations reclaimed by dashpay_payment_array_free. Overflow: 1 valid suggestion dropped (register_external_contact_account persist outside write lock — conf 0.55).
🔴 2 blocking | 🟡 2 suggestion(s)
2 additional finding(s) omitted (not in diff).
6 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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-storage/src/sqlite/schema/identities.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:206-222: rejected_contact_requests is written but never read — tombstones lost across restart
Verified at HEAD: managed_identity_from_entry still hard-codes rejected_contact_requests: Default::default() at line 214. The writer at contacts.rs:240 (INSERT INTO rejected_contact_requests) and migration row at V001:203 exist, but there is no load_state reader for the new table and apply_changeset only handles live deltas — restored state is always empty. The recurring DashPay loop's tombstone-skip at network/contact_requests.rs:396-404 never fires after a restart, so a rejected contact request is resurrected on the first sweep. Add a per-wallet load_state for rejected_contact_requests and route its output into the ContactChangeSet.rejected synthesized during rehydration.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:2980-2996: Wallet deletion omits new DashPay payment children before deleting identities — same fatal pattern as contact requests
Verified: PersistentDashpayPayment.owner is declared as non-optional (`public var owner: PersistentIdentity`), and the new cascade relationship was added on PersistentIdentity.dashpayPayments in the M2 delta. The PHASE 1 pre-delete loop at lines 2986-2996 iterates dpnsNames, dashpayProfile, and contactRequests — but not dashpayPayments. The surrounding comment (lines 2962-2978) explicitly states this phase exists because SwiftData fatals during save() when it must null out a non-optional inverse on a child processed in the same delete batch. dashpayPayments has the exact same shape as contactRequests, so a wallet with persisted DashPay payments can crash or fail to wipe cleanly when deleted. Add a `for payment in Array(identity.dashpayPayments) { backgroundContext.delete(payment) }` loop alongside the existing pre-deletion.
In `packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:245-261: select_recipient_key_index returns disabled keys
Verified at HEAD lines 245-261: the selector iterates recipient_identity.public_keys() and returns the first DECRYPTION (then ENCRYPTION) ECDSA_SECP256K1 key with no disabled_at check. validate_contact_request in crypto/validation.rs does gate on disabled keys, so a recipient with a disabled preferred DECRYPTION key gets returned anyway and the broadcast fails downstream with an opaque error instead of falling through to a usable ENCRYPTION key on the same identity. Beyond reliability, on the send-side this also means the wallet could encrypt the DIP-15 compact xpub to a revoked key whose private half may be compromised. Add `&& k.disabled_at().is_none()` to both branches so selection matches validation.
In `packages/rs-platform-wallet-ffi/src/dashpay_payment.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay_payment.rs:89-108: DashpayPaymentFFI derives Clone, Copy despite owning *mut c_char strings reclaimed by dashpay_payment_array_free
Verified at HEAD lines 89-108: DashpayPaymentFFI carries two heap-owned C strings (txid, memo — produced via CString::into_raw in cstring_or_null and reclaimed via CString::from_raw in dashpay_payment_array_free), yet the struct is `#[derive(Debug, Clone, Copy)]`. With Copy the compiler will silently shallow-duplicate the struct on any by-value rebinding inside this crate, and a subsequent free walk on the array (or a stray from_raw on the duplicate) would double-free the txid/memo allocations across the FFI boundary. Today's call sites are sound — the struct is built once, moved into Vec → Box<[T]> → Box::into_raw, and reclaimed exactly once — but the Copy derive removes the borrow-checker guardrail that normally prevents this class of bug at refactor time. Sibling FFI types in this crate that own heap pointers (ContactRequestFFI, WalletChangeSetFFI) deliberately omit Copy for exactly this reason. Drop Copy (and Clone if unneeded) on this struct. The cross-boundary contract is unchanged — Swift consumes by raw pointer.
Conflict: identity_handle.rs — both sides appended a test module (ours: ecdh_key_derivation_tests; upstream: master-derive tests from the rescan fix). Kept both; 221/221 platform-wallet lib tests green on the merged tree. Also folds in a build fix the merged tree needs: upstream CreateIdentityView's funding-source footer (string concatenation with an embedded ternary) exceeds the Swift type-checker budget on Xcode here — hoisted into a static helper, no copy change.
shumkov
changed the title
The explorer-coverage CI guard caught the M2 model addition: every SwiftData model needs an explorer row + list view + detail view. Adds the "DashPay Payments" section (network-scoped count, newest first, full-column read-only detail), mirroring the contact-request views. check-storage-explorer.sh: 28/28 covered.
…3, M3) Send side: - contact requests now carry the DIP-15 masked accountReference instead of a hardcoded 0: (version << 28) | (ASK28 ^ account). With the contract's unique index (ownerId, toUserId, accountReference), the constant 0 meant a superseding request after key rotation could never broadcast (duplicate-unique rejection) — the version bump is what makes re-keying possible. - Re-sending to a recipient with a tracked prior request unmasks the prior version and bumps it (saturating at the 4-bit max with a warning). Crypto helper fixes (research/06 §3 found both axes wrong): - HMAC input is now the 69-byte DIP-15 compact xpub (both reference clients agree), not the 107-byte DIP-14 encode(). - ASK28 extraction matches iOS dash-shared-core: digest bytes [28..32] big-endian >> 4. The reference clients disagree with each other here (Android: bytes [0..4] LE) — recipients must disregard the field per DIP-15, so the binding consumer is our own round-trip; we follow the Rust reference implementation and flag the divergence for a DIP clarification. - New unmask_account_reference recovers (version, account) for the sender. Receive side (DIP-15 "sender rotated their addresses"): - Sync ingest dedups by (sender, accountReference) instead of bare sender id: a known sender with a NEW reference is a rotation request and passes the guard (the old guard silently dropped it). - apply_rotated_incoming_request supersedes the tracked request (last-write-wins per pair; simultaneous multi-account rides acceptedAccounts later), clears payment_channel_broken — the recovery the flag's contract promises — and the sync pass tears down the stale external account so the build sweep re-registers it from the rotated xpub. Tests: ASK28 byte-order pin (fails on the old head-of-digest read), mask/unmask round-trip across version/account ranges, rotation re-key + broken-flag clear + pending-replace + stranger no-op. 223/223 lib + 9/9 workflow green.
Shared-secret-only callback on the existing host-signer table; the identity private key never crosses the ABI. EcdhProvider routing stays internal to platform-wallet so M4's implementation lands without wallet-API churn. One hook covers both send-side and decrypt-side ECDH.
thepastaclaw
suggested changes
Jun 12, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Re-verified all 10 prior findings against worktree HEAD aabc21e; every one is STILL VALID — none of the three Swift example-app commits or the v3.1-dev merge touched the dashpay-correctness Rust/Swift hotspots. Carrying forward 7 blockers (append-only V001 violation, rejected-request persistence asymmetry, missing tombstone reader, transient-as-permanent channel breakage, sticky purpose_mismatch, unreachable broken-channel recovery, SwiftData wallet-deletion miss on the new payments cascade) and 3 suggestions (sync stop/start cleanup race, send-side disabled-key selection, Copy on FFI-owned C-strings). REQUEST_CHANGES.
🔴 7 blocking
3 additional finding(s) omitted (not in diff).
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/state/managed_identity/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs:109-131: record_rejected_contact_request drops incoming in memory but never persists the deletion
Verified at HEAD lines 109-131: line 116 calls `self.incoming_contact_requests.remove(sender_id)` but the returned `ContactChangeSet` (lines 127-129) only populates `cs.rejected`. The unified contacts-table writer in `rs-platform-wallet-storage/src/sqlite/schema/contacts.rs` only DELETEs incoming rows when `cs.removed_incoming` is non-empty, so the persisted `state='received'` row with its stale `incoming_request` blob stays in SQLite. Once `persister.load()` is wired up, the contacts reader re-buckets that row as an incoming request and `apply_changeset` re-inserts it — the explicitly-rejected request reappears in the UI. The in-memory mutation and the persisted delta are internally inconsistent. Emit a matching `ReceivedContactRequestKey { owner_id, sender_id: *sender_id }` into `cs.removed_incoming` alongside `cs.rejected`.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:206-222: rejected_contact_requests tombstones are written but never restored on rehydration
Verified at HEAD: `managed_identity_from_entry` still hard-codes `rejected_contact_requests: Default::default()` at line 214. The writer at `contacts.rs:240` (`INSERT INTO rejected_contact_requests`) and the migration row at `V001:203` exist, but no `load_state` reader for the new table exists, and `apply_changeset` only handles live deltas — restored state is always empty. The recurring DashPay loop's tombstone-skip at `network/contact_requests.rs:396-404` therefore never fires after a restart, so a rejected contact request is resurrected on the first sweep and surfaces to the user again. Security framing: the on-platform document is immutable, so the local tombstone is the ONLY thing that suppresses a spammer's repeated contact request — a wipe-on-restart defeats the user's explicit reject. Add a per-wallet `load_state` reader for `rejected_contact_requests` and route its output into the `ContactChangeSet.rejected` synthesized during rehydration.
In `packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:711-729: Transient DAPI failures inside register_external_contact_account are classified as permanent
Verified at HEAD lines 711-729: `build_contact_accounts` treats ANY error from `register_external_contact_account` as permanent and calls `mark_contact_channel_broken`. `register_external_contact_account` performs a fresh `Identity::fetch` internally and wraps DAPI/network failures as `PlatformWalletError::InvalidIdentityData`. A single transient DAPI hiccup therefore permanently disables the payment channel; subsequent sweeps skip the contact via the `payment_channel_broken` filter, and recovery only fires if a superseding contactRequest arrives — but the established-contact ingest skip at line 389 makes that path unreachable. Combined, a transient network event bricks a channel forever, and a malicious or unreliable DAPI endpoint becomes a persistent availability attack against payments to a specific contact. Fix by either passing the pre-fetched identity into registration or scoping permanent-broken classification to genuinely non-recoverable failures (decrypt/decode, missing-key, key-type mismatch).
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:383-404: Established-contact ingest skip makes payment_channel_broken recovery unreachable
Verified at HEAD lines 383-404: the received-side ingest drops every doc whose sender is already in `established_contacts` (line 389) BEFORE consulting `accountReference`. `EstablishedContact::reestablish_preserving_metadata` exists precisely to clear `payment_channel_broken` when a fresh request flows in, and `collect_account_build_candidates` documents the recovery contract ("never retry a permanently-broken channel — wait for a superseding request which clears the flag on re-establish"). But no path reaches `reestablish_preserving_metadata` for an already-established sender from the sync sweep — `add_incoming_contact_request` is only called for new senders here, and the send-side guard at `state/managed_identity/contact_requests.rs:46-48` also returns early for established contacts. Once `payment_channel_broken` is set, it stays set forever. Either detect a new `accountReference` for the same sender and route through the reestablish path, or change the broken-channel policy to permit controlled retry.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:245-261: select_recipient_key_index returns disabled (revoked) keys — DIP-15 compact xpub encrypted to a key whose private half may be compromised
Verified at HEAD lines 245-261: the selector iterates `recipient_identity.public_keys()` and returns the first DECRYPTION (then ENCRYPTION) ECDSA_SECP256K1 key with no `disabled_at` check. `validate_contact_request` in `crypto/validation.rs` does gate on `disabled_at`, so the asymmetry creates both a reliability bug (an opaque downstream broadcast failure instead of falling through to a usable key) AND a real confidentiality exposure: on send, the wallet encrypts the 69-byte DIP-15 compact xpub (fingerprint‖chaincode‖pubkey — combined with `accountReference` lets the holder derive every receiving address on that account) to a key the recipient has explicitly revoked. Identity-key revocation is the on-platform mechanism for declaring "the private half of this key may be compromised". Add `&& k.disabled_at().is_none()` to both branches so selection matches validation. (Promoted from suggestion to blocking on the security-auditor confidentiality analysis.)
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:3018-3035: Wallet deletion PHASE 1 omits PersistentDashpayPayment children — same fatal pattern as contactRequests
Verified at HEAD: PHASE 1 (lines 3024-3034) iterates `dpnsNames`, `dashpayProfile`, and `contactRequests` but NOT `dashpayPayments`. `PersistentDashpayPayment.owner` is non-optional (`PersistentDashpayPayment.swift:98`), and `PersistentIdentity.dashpayPayments` is the cascading inverse added in this PR. The surrounding comment (lines 3018-3023) explicitly states this phase exists because SwiftData fatals during `save()` when it must null out a non-optional inverse on a child processed in the same batch — exactly the shape of the new payments relationship. A wallet with persisted DashPay payment history will hit the SwiftData fatal at PHASE 2 (`save()` after `delete(identity)`), aborting before the wallet row is removed. The user's belief that they wiped DashPay data is wrong, and plaintext memo + counterparty id + amount + txid rows remain on disk. Add a `for payment in Array(identity.dashpayPayments) { backgroundContext.delete(payment) }` loop alongside the existing pre-deletions.
In `packages/rs-platform-wallet/src/manager/dashpay_sync.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/dashpay_sync.rs:192-235: DashPaySyncManager thread cleanup unconditionally clears the cancel token — stop/start race enables use-after-free across FFI
Verified at HEAD lines 192-235: the spawned thread's cleanup at lines 230-232 writes `*guard = None` on loop exit regardless of which token the slot currently holds. If `stop()` cancels the old token and `start()` installs a fresh token before the old thread reaches cleanup, the old thread clears the NEW token — `background_cancel` is empty while a fresh sync thread is still running. `stop()`/`quiesce()` then become a no-op against that running thread. In this PR's Swift integration the persister and DashPay event callbacks close over an UnsafePointer<Context> allocated on the Swift side; calling `dashpay_sync_manager_destroy` (or the wallet manager's drop) after the visible token was cleared frees that context while the surviving thread continues invoking the callbacks against the freed pointer — a concrete use-after-free crossing the C ABI, reachable through normal start/stop/destroy controls (toggling tabs, login/logout) and widened by attacker-influenced network timing. Capture the spawned token and only clear the slot if it still holds the same token (`Arc::ptr_eq`), mirroring `ShieldedSyncManager`'s generation guard. (Upgraded from suggestion to blocking based on the security-auditor and codex-security cross-checks of the destroy/UAF path.)
…earch Decisive: no reference client (DashSync-iOS, dashj, dash-shared-core) ever implemented contactInfo — our implementation sets the de-facto convention. Adopts: DIP-15 child derivation (root/65536'+65537'/idx'), AES-256-ECB encToUserId, IV-prepended AES-256-CBC privateData, CBOR array [aliasName, note, displayHidden] per the deployed schema (which contradicts DIP-15 prose — table included), ≥2-contacts publish gate.
… part 1) The crypto core for DashPay contactInfo documents, following the conventions recorded in docs/dashpay/research/07 (no reference client ever implemented this doc type — this sets the de-facto wire format): - platform-encryption: AES-256-ECB encrypt/decrypt for the 32-byte encToUserId (two raw blocks, no IV/padding — DIP-15's own ECB soundness argument: the plaintext is a SHA-256 output and the key is single-purpose), plus IV-prepended AES-256-CBC helpers for privateData. Tests pin the ECB property (identical blocks encrypt identically) so a CBC-with-zero-IV regression can't slip in. - platform-wallet crypto/contact_info.rs: DIP-15 key derivation (rootEncryptionKey / 65536' / index' for encToUserId, / 65537' / index' for privateData — hardened children of the identity's registered ENCRYPTION key path), CBOR codec for the deployed schema's array shape [aliasName, note, displayHidden] with a 4th ignored padding element lifting tiny payloads to the schema's 48-byte ciphertext floor. Tests: key-derivation determinism + domain separation, CBOR round-trip incl. all-absent payload, full derive→encrypt→decrypt round-trip with schema bounds check.
thepastaclaw
suggested changes
Jun 12, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Reconciliation against HEAD 440ffca: prior finding #6 (established-contact ingest skip) is FIXED by the new rotation path. Nine prior findings remain STILL VALID and the new G3 delta introduces two additional blockers — the send-side rotation-version lookup ignores established_contacts (forcing version=0 collisions after auto-establishment) and the receive-side rotation handler replays immutable historical requests as fresh rotations on every sweep, churning the external account. Total: 10 in-scope blockers. Overflow: 1 suggestion (DashpayPaymentFFI Copy) dropped due to budget.
🔴 5 blocking
2 additional finding(s) omitted (not in diff).
5 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:165-201: Send-side rotation version lookup ignores established_contacts — re-send after auto-establishment reuses version=0 and collides on the unique index
The new G3 rotation logic computes `previous_version` only from `managed.sent_contact_requests.get(recipient_identity_id)` (line 171). But establishment (`add_incoming_contact_request` line 175 and `apply_established_contact` line 372 in state/managed_identity/contact_requests.rs) explicitly removes the entry from `sent_contact_requests` and parks the prior outgoing request on `EstablishedContact.outgoing_request`. Once the reciprocal arrives and a sweep auto-establishes the pair, the next `send_contact_request` to that recipient sees `previous_version = None` and falls back to `version = 0`. With deterministic xpub/ECDH for the same (sender, recipient) and unchanged `account_index`, the PRF reproduces the same masked `account_reference` as the original sent request. The contract's unique index `($ownerId, toUserId, accountReference)` rejects the broadcast — the exact failure mode G3 was added to prevent. Fall back to `established_contacts[recipient].outgoing_request` (taking the max of both versions if both are present).
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:441-478: Historical contactRequest documents replay as fresh rotations every sync sweep
The rotation guard at line 451 only compares the incoming reference against the currently tracked reference (incoming map or established contact). contactRequest documents are immutable, and `fetch_received_contact_requests(identity_id, None)` (line 370) is unfiltered, so every sweep returns both the original v=0 and any rotated v=N documents. Within a single sweep, ingesting v=0 against an already-tracked v=N flips the established contact back to v=0 and queues a teardown (lines 472-478, then 517-528), and the next document in the same iteration flips it forward again. Across sweeps the same churn replays — the external account is torn down and rebuilt on every cycle, generating wasted DAPI traffic. Worse, if the freshest document falls outside the eventual paginated window (post-M3 growth), the contact can regress to the stale xpub. Compare by `created_at`/version monotonicity, not bare reference inequality: only apply rotation when the incoming request strictly supersedes the tracked one.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:292-308: select_recipient_key_index returns disabled (revoked) keys — DIP-15 compact xpub encrypted to a key whose private half may be compromised
Verified at HEAD lines 292-308: the selector iterates `recipient_identity.public_keys()` and returns the first DECRYPTION (then ENCRYPTION) ECDSA_SECP256K1 key with no `disabled_at` check. `validate_contact_request` does gate on `disabled_at`, so the send/receive interop rules are asymmetric. On send, the 69-byte DIP-15 compact xpub (fingerprint‖chaincode‖pubkey — combined with `accountReference` lets the holder derive every receiving address on that account) is encrypted to a key the recipient has explicitly revoked. Identity-key revocation is the on-platform mechanism for declaring 'this key's private half may be compromised'. Add `&& k.disabled_at().is_none()` to both branches so selection matches validation.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:206-222: rejected_contact_requests tombstones are written but never restored on rehydration
`managed_identity_from_entry` still hard-codes `rejected_contact_requests: Default::default()` at line 214. The writer at `sqlite/schema/contacts.rs:240` (INSERT INTO rejected_contact_requests) and the V001 row exist, but there is no `load_state` reader for the new table, and `apply_changeset` only handles live deltas — restored state is always empty after restart. The recurring DashPay loop's tombstone-skip at `network/contact_requests.rs:457` therefore never fires after restart, so a rejected request is resurrected on the first sweep. Because the on-platform document is immutable, wiping the tombstone on restart defeats the user's explicit reject. Add a per-wallet `load_state` reader for `rejected_contact_requests` and route its output into the rehydration changeset / ManagedIdentity field.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:3024-3034: Wallet deletion PHASE 1 omits PersistentDashpayPayment children — same fatal pattern as contactRequests
PHASE 1 iterates `dpnsNames`, `dashpayProfile`, and `contactRequests` but NOT `dashpayPayments`. `PersistentDashpayPayment.owner` is non-optional and `PersistentIdentity.dashpayPayments` is the cascading inverse added in this PR. The surrounding comment (lines 3008-3023) explicitly documents that this phase exists because SwiftData fatals during `save()` when it must null out a non-optional inverse on a child processed in the same batch — exactly the shape of the new payments relationship. A wallet with persisted DashPay payment history will hit the SwiftData fatal at PHASE 2 `save()`, aborting before the wallet row is removed. The user believes their data was wiped; plaintext memo + counterparty id + amount + txid rows remain on disk. Pre-delete `identity.dashpayPayments` alongside the other children.
thepastaclaw
suggested changes
Jun 25, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Corrective review action for commit ce1604d3: the verifier selected REQUEST_CHANGES because all 9 findings from the prior 1b247fe8 review are still valid at this head. The latest delta is only a cosmetic 3-line deletion in DashPayTabView.swift, so there are no new delta findings.
Carried-forward blockers that still require changes:
packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs:set_contact_metadatastill mutates the liveEstablishedContactbefore persistence, so a store failure makes the retry path short-circuit.packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift:submitDashPayProfileWithSignerstill uses a bare_ = signerkeepalive instead ofwithExtendedLifetimearound the synchronous FFI call.
The seven prior suggestion-level findings were also revalidated as still valid: the two FFI out-param zero-init gaps, record_dashpay_payment in-memory-before-persist, unbounded dapk base58 decode, flattened SDK writer errors, missing SQLite persistence for contact_account_label, and the contact-request page-budget/rewind-window livelock risk. CodeRabbit had 0 inline findings, so no reactions were needed.
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Incremental review of ce1604d..0ff4cd7: only delta is a 7-line Swift UI fix giving DpnsSearchResult a composite Identifiable id — pure SwiftUI plumbing, no new defects introduced. All 9 prior findings reconciled and verified STILL VALID against HEAD: 2 blockers (set_contact_metadata mutates before persist defeating retry hook; submitDashPayProfileWithSigner uses bare _ = signer instead of withExtendedLifetime) and 7 suggestions (two FFI out-pointers not zero-initialised, in-memory-before-persist for record_dashpay_payment, unbounded base58 on QR/URI ingress, SDK error flattening in send_contact_request, missing contact_account_label SQLite column, sweep-budget × rewind-window doc-comment gap). All carried forward verbatim.
🟡 2 suggestion(s)
2 additional finding(s) omitted (not in diff).
7 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/established_contact.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/established_contact.rs:156-169: established_contact_get_note does not zero-init *out_note before fallible work
Carried forward — STILL VALID at HEAD 0ff4cd74. Sibling `established_contact_get_alias` (L107-108) writes `*out_alias = std::ptr::null_mut();` immediately after `check_ptr!`, before any fallible work. `established_contact_get_note` does `check_ptr!(out_note)` at L160 then immediately runs a `with_item` lookup, two `unwrap_option_or_return!`s, and `CString::new` before assigning `*out_note` at L167. Any early return inside that chain leaves `*out_note` at whatever pointer the caller pre-stamped — a use-after-free or double-free risk for any C/Swift caller that frees the out-param on error. Mirror the alias getter's pre-zero.
In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay_profile.rs:326-327: platform_wallet_create_or_update_dashpay_profile_with_signer does not zero-init *out_profile before fallible work
Carried forward — STILL VALID at HEAD 0ff4cd74. After `check_ptr!(out_profile)` (L326) and `check_ptr!(signer_handle)` (L327), the function runs `read_identifier`, three `decode_opt_c_str` calls, a raw-slice copy, the wallet `with_item` lookup, the signer vtable dereference, and the full `block_on_worker` broadcast (L329-364) before finally writing `*out_profile` at L367. Every error path leaves `*out_profile` holding caller-supplied stale bytes; `DashPayProfileFFI` owns three heap C-string pointers that callers free with `dashpay_profile_ffi_free`. The reader entry points in this same file already pre-zero with `DashPayProfileFFI::empty()` exactly to avoid this hazard. A C caller reusing an out-buffer would double-free. Add `*out_profile = DashPayProfileFFI::empty();` right after the `check_ptr!`s.
thepastaclaw
suggested changes
Jun 25, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Incremental review of ce1604d..0ff4cd7: only delta is a 7-line Swift UI fix giving DpnsSearchResult a composite Identifiable id — pure SwiftUI plumbing, no new defects introduced. All 9 prior findings reconciled and verified STILL VALID against HEAD: 2 blockers (set_contact_metadata mutates before persist defeating retry hook; submitDashPayProfileWithSigner uses bare _ = signer instead of withExtendedLifetime) and 7 suggestions (two FFI out-pointers not zero-initialised, in-memory-before-persist for record_dashpay_payment, unbounded base58 on QR/URI ingress, SDK error flattening in send_contact_request, missing contact_account_label SQLite column, sweep-budget × rewind-window doc-comment gap). All carried forward verbatim.
2 blocking | 6 suggestion(s) | 1 nitpick(s)
Corrective exact-SHA review: the first posting for this SHA was downgraded to COMMENT after inline dedupe even though the verifier returned REQUEST_CHANGES. This body-only review preserves the verified action without re-opening duplicate inline threads.
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.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:287-313: set_contact_metadata mutates in-memory state before persisting; equality short-circuit defeats the retry hook
Carried forward — STILL VALID at HEAD 0ff4cd74 (L299-301 unchanged). The function assigns `alias`, `note`, and `is_hidden` on the live `&mut EstablishedContact` before `persister.store(cs.into())?` runs at L311. The method's own doc-comment at L276-280 contracts callers to surface `Err` so the `ContactInfoDecrypt` queue entry stays in place for retry. That retry hook is dead: on the next drain the equality guard at L291-297 sees alias/note/hidden already match `metadata` and short-circuits to `Ok(true)` without re-calling the persister. After process restart the in-memory mutation is lost while the persisted row is stale — and a privacy-sensitive `is_hidden = true` flip can silently revert to visible. Persist first against a clone, then commit to the live contact only on `Ok`.
- [BLOCKING] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`:2159-2210: submitDashPayProfileWithSigner uses bare `_ = signer` keepalive instead of withExtendedLifetime
Carried forward — STILL VALID at HEAD 0ff4cd74 (L2160 still `_ = signer`, no `withExtendedLifetime` wrapper around the synchronous `platform_wallet_create_or_update_dashpay_profile_with_signer` calls at L2175/L2190). Rust dereferences `&*(signer_addr as *const VTableSigner)` for the entire synchronous `block_on_worker` (rs-platform-wallet-ffi/src/dashpay_profile.rs:341-363), and `KeychainSigner` registers `Unmanaged.passUnretained(self).toOpaque()` as its callback context — so Swift must keep the signer object alive for the full call. A bare `_ = signer` discard is not a documented optimizer lifetime barrier; under `-O` the compiler may release `signer` any time after its last use, causing a use-after-free of the signer callback context during signing. Every sibling `*WithSigner` wrapper in this same file uses `withExtendedLifetime(signer) { ... }`. Wrap the synchronous FFI invocation in `withExtendedLifetime(signer) { ... }` to match.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/established_contact.rs`:156-169: established_contact_get_note does not zero-init *out_note before fallible work
Carried forward — STILL VALID at HEAD 0ff4cd74. Sibling `established_contact_get_alias` (L107-108) writes `*out_alias = std::ptr::null_mut();` immediately after `check_ptr!`, before any fallible work. `established_contact_get_note` does `check_ptr!(out_note)` at L160 then immediately runs a `with_item` lookup, two `unwrap_option_or_return!`s, and `CString::new` before assigning `*out_note` at L167. Any early return inside that chain leaves `*out_note` at whatever pointer the caller pre-stamped — a use-after-free or double-free risk for any C/Swift caller that frees the out-param on error. Mirror the alias getter's pre-zero.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:326-327: platform_wallet_create_or_update_dashpay_profile_with_signer does not zero-init *out_profile before fallible work
Carried forward — STILL VALID at HEAD 0ff4cd74. After `check_ptr!(out_profile)` (L326) and `check_ptr!(signer_handle)` (L327), the function runs `read_identifier`, three `decode_opt_c_str` calls, a raw-slice copy, the wallet `with_item` lookup, the signer vtable dereference, and the full `block_on_worker` broadcast (L329-364) before finally writing `*out_profile` at L367. Every error path leaves `*out_profile` holding caller-supplied stale bytes; `DashPayProfileFFI` owns three heap C-string pointers that callers free with `dashpay_profile_ffi_free`. The reader entry points in this same file already pre-zero with `DashPayProfileFFI::empty()` exactly to avoid this hazard. A C caller reusing an out-buffer would double-free. Add `*out_profile = DashPayProfileFFI::empty();` right after the `check_ptr!`s.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs`:165-180: record_dashpay_payment inserts into the in-memory map before persisting; defeats caller-side contains_key retry guards
Carried forward — STILL VALID at HEAD 0ff4cd74 (L171 still inserts before the `persister.store` at L179). The reconcile sweep in `payments.rs` gates on `managed.dashpay_payments.contains_key(&txid)` and skips when true; on persist failure the in-memory entry is already committed, so the next sweep's `contains_key` returns true and the documented retry never fires until process restart. The send path is worse because the user memo has no UTXO-derived recovery. Roll back the in-memory insert on persister error so the next sweep correctly re-triggers.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/identity/crypto/auto_accept.rs`:343-368: parse_dashpay_contact_uri base58-decodes untrusted dapk with no upper bound
Carried forward — STILL VALID at HEAD 0ff4cd74 (L364 still calls `bs58::decode(&dapk).into_vec()` with no pre-length check). `parse_dashpay_contact_uri` is the QR / deep-link / paste ingress for arbitrary external input. The valid DIP-15 `key_blob` is fixed-size (`KEY_BLOB_LEN`), so the base58 envelope has a known maximum length (~52 chars for a 38-byte blob); any oversized input is invalid by construction. On a phone, a hostile QR code or pasted URI can force a multi-megabyte base58 allocation and decode round-trip before the post-decode length check rejects it. Reject `dapk.len()` above a small ceiling derived from `KEY_BLOB_LEN` before calling `bs58::decode`, then verify the decoded length equals `KEY_BLOB_LEN` as today.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/identity/network/sdk_writer.rs`:286-292: DashPaySdkWriter::send_contact_request flattens dash_sdk::Error into InvalidIdentityData
Carried forward — STILL VALID at HEAD 0ff4cd74. `DashPaySdkWriter::send_contact_request` maps every error returned by `Sdk::send_contact_request` through `PlatformWalletError::InvalidIdentityData(format!("Failed to send contact request: {e}"))`. The sibling `put_document` impl in this same file (L307-318) uses `.map_err(PlatformWalletError::Sdk)` to preserve the structured `#[from] dash_sdk::Error`. The categorization is also wrong — a broadcast/consensus rejection is not 'invalid identity data'. The stringification permanently strips the typed error from the `is_instant_lock_proof_invalid` / `as_asset_lock_proof_cl_height_too_low` matchers used elsewhere in the crate to drive CL-proof / IS-timeout retry paths. Match the sibling.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs`:199-230: SQLite established-contact rows do not persist contact_account_label
Carried forward — STILL VALID at HEAD 0ff4cd74 (INSERT/ON CONFLICT at L200-212 still lacks the column). `EstablishedContact` carries `contact_account_label: Option<String>`, and `store_contact_account_label` (network/contact_requests.rs) drives an `established` changeset specifically to persist it. The SQLite writer only stores alias / note / is_hidden / accepted_accounts / payment_channel_broken — the new column is absent from the INSERT/ON CONFLICT, the V001 schema has no column for it, and the load path explicitly reconstructs `contact_account_label: None` with a comment that the backend has no column for it. SQLite-backed wallets therefore silently lose the decrypted incoming-account label across restart and redo decrypt work the backend can never preserve. V001 has not yet shipped, so the column can be added in-place rather than via a follow-on migration.
- [NITPICK] In `packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs`:33-51: Per-sweep page budget plus 10-minute wallet rewind window can livelock under contactRequest clusters
Carried forward — STILL VALID at HEAD 0ff4cd74 (doc-comment L43-50 still names only the single-`$createdAt` cluster residual risk). The wallet caller defines `const SYNC_OVERLAP_MS: u64 = 10 * 60_000;` and rewinds the high-water by that amount on every sweep. If ≥5000 matching `contactRequest` docs cluster across multiple blocks but all inside a 10-minute window, each sweep advances `high_water` to T1 inside the cluster, then rewinds to T1−10min on the next sweep — re-reading the same oldest budgeted prefix indefinitely and starving newer requests. Either persist a `StartAfter` doc-id continuation cursor when a sweep returns a still-full budgeted page, or extend the doc-comment to call out the multi-block-within-rewind-window angle as part of the documented residual risk.
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Latest delta (0ff4cd7..71e7e03) is documentation-only — a single 439-line addition of docs/dashpay/IDENTITY_KEY_SCALAR_ELIMINATION_SPEC.md with no code changes — so no new findings emerged from this delta. All 9 prior findings against 0ff4cd7 were verified file-by-file at HEAD and remain STILL VALID; they are carried forward unchanged. Two blocking issues persist (persist-before-mutate retry-hook violation in set_contact_metadata, and a missing withExtendedLifetime around the synchronous signer FFI call).
🟡 2 suggestion(s)
2 additional finding(s) omitted (not in diff).
7 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/established_contact.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/established_contact.rs:156-169: established_contact_get_note does not zero-init *out_note before fallible work
Carried forward — STILL VALID at HEAD 71e7e030. After `check_ptr!(out_note)` at L160, three fallible operations (`with_item` returning `None` for a stale handle, the inner `Option<String>` being `None`, and `CString::new` failing on interior NUL) can early-return before `*out_note` is assigned at L167. On those failure paths the caller's out-pointer retains whatever it held pre-call. Callers that follow the standard pattern of freeing the previous owned C-string on success and reusing the variable across calls can hit use-after-free / double-free shapes. Sibling getters (e.g. `established_contact_get_alias`) write `*out = std::ptr::null_mut()` immediately after `check_ptr!` to make the error contract explicit — match that pattern here.
In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay_profile.rs:326-327: platform_wallet_create_or_update_dashpay_profile_with_signer does not zero-init *out_profile before fallible work
Carried forward — STILL VALID at HEAD 71e7e030. After `check_ptr!(out_profile)` and `check_ptr!(signer_handle)` (L326–327) the function performs identifier decoding, three optional C-string decodes, an avatar slice copy, wallet-handle resolution, signer dereference, and the async broadcast — all fallible — before finally assigning `*out_profile` at L367. On any error path `*out_profile` retains whatever the caller supplied. The Swift wrapper at ManagedPlatformWallet.swift:2206 unconditionally invokes `dashpay_profile_ffi_free(&outProfile)` via `defer`, so a stale owned-C-string layout in caller memory is reachable as double-free / use-after-free. While the current Swift caller declares the buffer fresh via `var outProfile = DashPayProfileFFI()`, the ABI contract is fragile for any other consumer. Assign `*out_profile = DashPayProfileFFI::empty()` right after `check_ptr!`.
thepastaclaw
suggested changes
Jun 25, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Latest delta (0ff4cd7..71e7e03) is documentation-only — a single 439-line addition of docs/dashpay/IDENTITY_KEY_SCALAR_ELIMINATION_SPEC.md with no code changes — so no new findings emerged from this delta. All 9 prior findings against 0ff4cd7 were verified file-by-file at HEAD and remain STILL VALID; they are carried forward unchanged. Two blocking issues persist (persist-before-mutate retry-hook violation in set_contact_metadata, and a missing withExtendedLifetime around the synchronous signer FFI call).
🔴 2 blocking | 🟡 6 suggestion(s) | 💬 1 nitpick(s)
9 additional finding(s)
blocking: set_contact_metadata mutates in-memory state before persisting; equality short-circuit defeats the retry hook
packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs (line 287)
Carried forward — STILL VALID at HEAD 71e7e03 (file unchanged in latest delta). L299–301 assign alias, note, and is_hidden onto the live &mut EstablishedContact before persister.store(cs.into())? runs at L311. The method's own doc-comment (L276–280) explicitly contracts callers to surface Err so the ContactInfoDecrypt queue entry remains for retry — but because the in-memory copy was already mutated, the next call hits the equality guard at L291–294 and returns Ok(true) without retrying. Net effect: a transient persist failure leaves the alias/note/is_hidden change in RAM only, the queue entry stays parked yet bypasses retry, and the user's edit (including the local block-flag is_hidden) is silently lost at next restart. Fix: clone the contact, build/persist the changeset against the candidate state first, and only commit to the live entry after persister.store succeeds.
if contact.alias == metadata.alias_name
&& contact.note == metadata.note
&& contact.is_hidden == metadata.display_hidden
{
// Unchanged — skip the persister round (the recurring sync
// calls this for every decrypted doc on every pass).
return Ok(true);
}
let mut updated = contact.clone();
updated.alias = metadata.alias_name;
updated.note = metadata.note;
updated.is_hidden = metadata.display_hidden;
let mut cs = ContactChangeSet::default();
cs.established.insert(
SentContactRequestKey {
owner_id,
recipient_id: *contact_id,
},
updated.clone(),
);
persister.store(cs.into())?;
*contact = updated;
Ok(true)
}
blocking: submitDashPayProfileWithSigner uses bare `_ = signer` keepalive instead of withExtendedLifetime
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift (line 2159)
Carried forward — STILL VALID at HEAD 71e7e03. The detached task at L2159 captures signer and immediately discards it via _ = signer (L2160), then makes the synchronous FFI calls at L2175/L2190 that pass signerHandle (an unmanaged pointer the Rust side dereferences as &VTableSigner in dashpay_profile.rs:353). A bare discard is not a documented optimizer lifetime barrier — the Swift optimizer is free to release the strong reference while Rust is still inside block_on_worker. If signer is the last strong reference, ARC can destroy the VTableSigner backing storage mid-FFI, producing a use-after-free at the boundary. Sibling signer-driven flows in this file already use withExtendedLifetime(signer) { ... } for exactly this reason. Wrap the FFI invocation in withExtendedLifetime(signer) to match the established pattern.
suggestion: SQLite established-contact rows do not persist contact_account_label
packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs (line 199)
Carried forward — STILL VALID at HEAD 71e7e03. The INSERT/ON CONFLICT at L199–212 lists wallet_id, owner_id, contact_id, state, outgoing_request, incoming_request, alias, note, is_hidden, accepted_accounts, payment_channel_broken — no contact_account_label column. EstablishedContact carries contact_account_label, and store_contact_account_label emits an established changeset, but the SQL silently drops it and the load path (sqlite/schema/contacts.rs:347) reconstructs None. The label survives only in-memory until process restart. Since contact_account_label is part of the BIP-44 account-derivation context for the friendship, silent loss is a state-divergence with potential loss-of-funds shape depending on how downstream code resolves label == None. Add the column to V001 and wire it through INSERT, ON CONFLICT UPDATE SET, and the load mapping.
suggestion: established_contact_get_note does not zero-init *out_note before fallible work
packages/rs-platform-wallet-ffi/src/established_contact.rs (line 156)
Carried forward — STILL VALID at HEAD 71e7e03. After check_ptr!(out_note) at L160, three fallible operations (with_item returning None for a stale handle, the inner Option<String> being None, and CString::new failing on interior NUL) can early-return before *out_note is assigned at L167. On those failure paths the caller's out-pointer retains whatever it held pre-call. Callers that follow the standard pattern of freeing the previous owned C-string on success and reusing the variable across calls can hit use-after-free / double-free shapes. Sibling getters (e.g. established_contact_get_alias) write *out = std::ptr::null_mut() immediately after check_ptr! to make the error contract explicit — match that pattern here.
/// Get the note for an established contact
#[no_mangle]
pub unsafe extern "C" fn established_contact_get_note(
contact_handle: Handle,
out_note: *mut *mut std::os::raw::c_char,
) -> PlatformWalletFFIResult {
check_ptr!(out_note);
unsafe { *out_note = std::ptr::null_mut() };
let option =
ESTABLISHED_CONTACT_STORAGE.with_item(contact_handle, |contact| contact.note.clone());
let option = unwrap_option_or_return!(option);
let note = unwrap_option_or_return!(option);
let c_str = unwrap_result_or_return!(std::ffi::CString::new(note));
unsafe { *out_note = c_str.into_raw() };
PlatformWalletFFIResult::ok()
}
suggestion: platform_wallet_create_or_update_dashpay_profile_with_signer does not zero-init *out_profile before fallible work
packages/rs-platform-wallet-ffi/src/dashpay_profile.rs (line 326)
Carried forward — STILL VALID at HEAD 71e7e03. After check_ptr!(out_profile) and check_ptr!(signer_handle) (L326–327) the function performs identifier decoding, three optional C-string decodes, an avatar slice copy, wallet-handle resolution, signer dereference, and the async broadcast — all fallible — before finally assigning *out_profile at L367. On any error path *out_profile retains whatever the caller supplied. The Swift wrapper at ManagedPlatformWallet.swift:2206 unconditionally invokes dashpay_profile_ffi_free(&outProfile) via defer, so a stale owned-C-string layout in caller memory is reachable as double-free / use-after-free. While the current Swift caller declares the buffer fresh via var outProfile = DashPayProfileFFI(), the ABI contract is fragile for any other consumer. Assign *out_profile = DashPayProfileFFI::empty() right after check_ptr!.
suggestion: record_dashpay_payment inserts into the in-memory map before persisting; defeats caller-side contains_key retry guards
packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs (line 165)
Carried forward — STILL VALID at HEAD 71e7e03. L171 inserts (tx_id, entry) into self.dashpay_payments before snapshot_changeset() and persister.store(cs.into()) at L172/L179. The reconcile-Received sweep gates retry on managed.dashpay_payments.contains_key(&txid); on persist failure the in-memory insert poisons the retry guard, so the failed payment row is never re-persisted until process restart, while the doc-comment (L173–178) advertises self-healing sweep callers can log-and-continue. Defer the in-memory insert until persister.store succeeds, or roll it back on Err.
suggestion: DashPaySdkWriter::send_contact_request flattens dash_sdk::Error into InvalidIdentityData
packages/rs-platform-wallet/src/wallet/identity/network/sdk_writer.rs (line 286)
Carried forward — STILL VALID at HEAD 71e7e03. L287–291 stringifies any dash_sdk::Error as PlatformWalletError::InvalidIdentityData(format!("Failed to send contact request: {e}")). The sibling put_document impl (just below at L294+) preserves typed PlatformWalletError::Sdk variants so callers can distinguish transient network/SDK failures (retryable) from validation failures (terminal). This stripping makes the recurring sync manager mis-classify retryable transients as permanent and replay deterministic rejections, burning credits. Use .map_err(PlatformWalletError::Sdk) like the sibling.
let xpub_bytes_clone = xpub_bytes.clone();
self.sdk
.send_contact_request(send_input, ecdh_provider, |_account_ref: u32| async move {
Ok::<Vec<u8>, dash_sdk::Error>(xpub_bytes_clone)
})
.await
.map_err(PlatformWalletError::Sdk)
}
suggestion: parse_dashpay_contact_uri base58-decodes untrusted dapk with no upper bound
packages/rs-platform-wallet/src/wallet/identity/crypto/auto_accept.rs (line 343)
Carried forward — STILL VALID at HEAD 71e7e03. L364 calls bs58::decode(&dapk).into_vec() against the raw query parameter value extracted at L356 with no length precheck. dapk arrives from untrusted sources (QR codes, deep links, paste). bs58 decode is O(n²) in input length and the post-decode length check only runs after the allocation. A maliciously crafted dash: URI (megabyte-sized base58 string) forces a quadratic-time decode plus a large transient allocation on a wallet UI thread. The valid key_blob has a small fixed maximum (DIP-15 compact-xpub = 69 bytes), so an oversized dapk is purely a DoS amplifier. Reject dapk strings above a small bound derived from KEY_BLOB_LEN before bs58::decode, then keep the post-decode length check.
nitpick: Per-sweep page budget plus 10-minute wallet rewind window can livelock under contactRequest clusters
packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs (line 33)
Carried forward — STILL VALID at HEAD 71e7e03. The doc-comment at L43–50 names only the same-$createdAt residual risk (single block over 5,000 docs — implausible), but the wallet caller rewinds the high-water cursor by ~10 minutes per sweep (SYNC_OVERLAP_MS). A hostile sender broadcasting > 5,000 fee-paid contactRequests within a 10-minute window (spanning multiple blocks) creates a starvation set: every sweep re-fetches that bucket from (high_water − 10min), exhausts the 50-page budget on the attack docs, never advances past them, and starves newer legitimate requests. Either persist a StartAfter document-id continuation cursor for full budgeted pages, or extend the doc-comment to acknowledge the multi-block-within-rewind-window starvation surface.
🤖 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.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:287-313: set_contact_metadata mutates in-memory state before persisting; equality short-circuit defeats the retry hook
Carried forward — STILL VALID at HEAD 71e7e030 (file unchanged in latest delta). L299–301 assign `alias`, `note`, and `is_hidden` onto the live `&mut EstablishedContact` *before* `persister.store(cs.into())?` runs at L311. The method's own doc-comment (L276–280) explicitly contracts callers to surface `Err` so the `ContactInfoDecrypt` queue entry remains for retry — but because the in-memory copy was already mutated, the next call hits the equality guard at L291–294 and returns `Ok(true)` without retrying. Net effect: a transient persist failure leaves the alias/note/is_hidden change in RAM only, the queue entry stays parked yet bypasses retry, and the user's edit (including the local block-flag `is_hidden`) is silently lost at next restart. Fix: clone the contact, build/persist the changeset against the candidate state first, and only commit to the live entry after `persister.store` succeeds.
- [BLOCKING] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`:2159-2210: submitDashPayProfileWithSigner uses bare `_ = signer` keepalive instead of withExtendedLifetime
Carried forward — STILL VALID at HEAD 71e7e030. The detached task at L2159 captures `signer` and immediately discards it via `_ = signer` (L2160), then makes the synchronous FFI calls at L2175/L2190 that pass `signerHandle` (an unmanaged pointer the Rust side dereferences as `&VTableSigner` in dashpay_profile.rs:353). A bare discard is not a documented optimizer lifetime barrier — the Swift optimizer is free to release the strong reference while Rust is still inside `block_on_worker`. If `signer` is the last strong reference, ARC can destroy the VTableSigner backing storage mid-FFI, producing a use-after-free at the boundary. Sibling signer-driven flows in this file already use `withExtendedLifetime(signer) { ... }` for exactly this reason. Wrap the FFI invocation in `withExtendedLifetime(signer)` to match the established pattern.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs`:199-230: SQLite established-contact rows do not persist contact_account_label
Carried forward — STILL VALID at HEAD 71e7e030. The INSERT/ON CONFLICT at L199–212 lists `wallet_id, owner_id, contact_id, state, outgoing_request, incoming_request, alias, note, is_hidden, accepted_accounts, payment_channel_broken` — no `contact_account_label` column. `EstablishedContact` carries `contact_account_label`, and `store_contact_account_label` emits an established changeset, but the SQL silently drops it and the load path (sqlite/schema/contacts.rs:347) reconstructs `None`. The label survives only in-memory until process restart. Since `contact_account_label` is part of the BIP-44 account-derivation context for the friendship, silent loss is a state-divergence with potential loss-of-funds shape depending on how downstream code resolves `label == None`. Add the column to V001 and wire it through INSERT, ON CONFLICT UPDATE SET, and the load mapping.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/established_contact.rs`:156-169: established_contact_get_note does not zero-init *out_note before fallible work
Carried forward — STILL VALID at HEAD 71e7e030. After `check_ptr!(out_note)` at L160, three fallible operations (`with_item` returning `None` for a stale handle, the inner `Option<String>` being `None`, and `CString::new` failing on interior NUL) can early-return before `*out_note` is assigned at L167. On those failure paths the caller's out-pointer retains whatever it held pre-call. Callers that follow the standard pattern of freeing the previous owned C-string on success and reusing the variable across calls can hit use-after-free / double-free shapes. Sibling getters (e.g. `established_contact_get_alias`) write `*out = std::ptr::null_mut()` immediately after `check_ptr!` to make the error contract explicit — match that pattern here.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:326-327: platform_wallet_create_or_update_dashpay_profile_with_signer does not zero-init *out_profile before fallible work
Carried forward — STILL VALID at HEAD 71e7e030. After `check_ptr!(out_profile)` and `check_ptr!(signer_handle)` (L326–327) the function performs identifier decoding, three optional C-string decodes, an avatar slice copy, wallet-handle resolution, signer dereference, and the async broadcast — all fallible — before finally assigning `*out_profile` at L367. On any error path `*out_profile` retains whatever the caller supplied. The Swift wrapper at ManagedPlatformWallet.swift:2206 unconditionally invokes `dashpay_profile_ffi_free(&outProfile)` via `defer`, so a stale owned-C-string layout in caller memory is reachable as double-free / use-after-free. While the current Swift caller declares the buffer fresh via `var outProfile = DashPayProfileFFI()`, the ABI contract is fragile for any other consumer. Assign `*out_profile = DashPayProfileFFI::empty()` right after `check_ptr!`.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs`:165-180: record_dashpay_payment inserts into the in-memory map before persisting; defeats caller-side contains_key retry guards
Carried forward — STILL VALID at HEAD 71e7e030. L171 inserts `(tx_id, entry)` into `self.dashpay_payments` before `snapshot_changeset()` and `persister.store(cs.into())` at L172/L179. The reconcile-Received sweep gates retry on `managed.dashpay_payments.contains_key(&txid)`; on persist failure the in-memory insert poisons the retry guard, so the failed payment row is never re-persisted until process restart, while the doc-comment (L173–178) advertises self-healing sweep callers can log-and-continue. Defer the in-memory insert until `persister.store` succeeds, or roll it back on `Err`.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/identity/network/sdk_writer.rs`:286-292: DashPaySdkWriter::send_contact_request flattens dash_sdk::Error into InvalidIdentityData
Carried forward — STILL VALID at HEAD 71e7e030. L287–291 stringifies any `dash_sdk::Error` as `PlatformWalletError::InvalidIdentityData(format!("Failed to send contact request: {e}"))`. The sibling `put_document` impl (just below at L294+) preserves typed `PlatformWalletError::Sdk` variants so callers can distinguish transient network/SDK failures (retryable) from validation failures (terminal). This stripping makes the recurring sync manager mis-classify retryable transients as permanent and replay deterministic rejections, burning credits. Use `.map_err(PlatformWalletError::Sdk)` like the sibling.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/identity/crypto/auto_accept.rs`:343-368: parse_dashpay_contact_uri base58-decodes untrusted dapk with no upper bound
Carried forward — STILL VALID at HEAD 71e7e030. L364 calls `bs58::decode(&dapk).into_vec()` against the raw query parameter value extracted at L356 with no length precheck. `dapk` arrives from untrusted sources (QR codes, deep links, paste). bs58 decode is O(n²) in input length and the post-decode length check only runs after the allocation. A maliciously crafted `dash:` URI (megabyte-sized base58 string) forces a quadratic-time decode plus a large transient allocation on a wallet UI thread. The valid `key_blob` has a small fixed maximum (DIP-15 compact-xpub = 69 bytes), so an oversized `dapk` is purely a DoS amplifier. Reject `dapk` strings above a small bound derived from `KEY_BLOB_LEN` before `bs58::decode`, then keep the post-decode length check.
- [NITPICK] In `packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs`:33-51: Per-sweep page budget plus 10-minute wallet rewind window can livelock under contactRequest clusters
Carried forward — STILL VALID at HEAD 71e7e030. The doc-comment at L43–50 names only the same-`$createdAt` residual risk (single block over 5,000 docs — implausible), but the wallet caller rewinds the high-water cursor by ~10 minutes per sweep (`SYNC_OVERLAP_MS`). A hostile sender broadcasting > 5,000 fee-paid `contactRequest`s within a 10-minute window (spanning multiple blocks) creates a starvation set: every sweep re-fetches that bucket from `(high_water − 10min)`, exhausts the 50-page budget on the attack docs, never advances past them, and starves newer legitimate requests. Either persist a `StartAfter` document-id continuation cursor for full budgeted pages, or extend the doc-comment to acknowledge the multi-block-within-rewind-window starvation surface.
Corrective exact-SHA review: the first posting for this SHA was downgraded to COMMENT after inline dedupe even though the verifier returned REQUEST_CHANGES. This body-only review preserves the verified action without reopening duplicate inline threads.
After a contact is established the payment channel is symmetric (each party derives the other's payment address from the xpubs exchanged at establishment), so DP-03 now requires driving A->B and B->A. The live run only exercised the forward direction (Eve->Alice); the reverse is symmetric-by-design but not yet verified — recorded as a gap in QA_TESTCASES_SPEC.md §12. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Degraded-coverage recovery review: all required ACP Claude/Codex reviewer sessions and native CLIs failed authentication, so this exact-SHA review is based on deterministic coordinator revalidation of the prior 71e7e03 findings plus inspection of the latest delta. The af9031e delta only documents that DP-03 must verify payments in both directions and introduces no new code defects; all 9 prior findings remain STILL VALID and are carried forward.
🟡 2 suggestion(s)
2 additional finding(s) omitted (not in diff).
7 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/established_contact.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/established_contact.rs:156-169: established_contact_get_note does not zero-init *out_note before fallible work
Carried forward — STILL VALID at HEAD af9031e3. After check_ptr!(out_note) at line 160, stale-handle lookup, missing note, and CString::new can all return before line 167 assigns *out_note. Callers that reuse an output variable can observe a stale pointer on failure instead of a stable null value; the sibling alias getter pattern should be mirrored here.
In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay_profile.rs:326-367: platform_wallet_create_or_update_dashpay_profile_with_signer does not zero-init *out_profile before fallible work
Carried forward — STILL VALID at HEAD af9031e3. This signer-backed profile FFI entry point still checks out_profile at line 326 but only writes *out_profile at line 367, after identifier decoding, optional string decoding, avatar copying, wallet lookup, signer dereference, and async profile submission can fail. Other profile FFI functions in this file initialize DashPayProfileFFI::empty() early, but this one does not, leaving a fragile ABI contract for callers that reuse output storage.
thepastaclaw
suggested changes
Jun 26, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Degraded-coverage recovery review: all required ACP Claude/Codex reviewer sessions and native CLIs failed authentication, so this exact-SHA review is based on deterministic coordinator revalidation of the prior 71e7e03 findings plus inspection of the latest delta. The af9031e delta only documents that DP-03 must verify payments in both directions and introduces no new code defects; all 9 prior findings remain STILL VALID and are carried forward.
🔴 2 blocking | 🟡 6 suggestion(s) | 💬 1 nitpick(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/state/managed_identity/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs:287-313: set_contact_metadata mutates in-memory state before persisting; equality short-circuit defeats the retry hook
Carried forward — STILL VALID at HEAD af9031e3. Lines 299-301 still assign alias, note, and is_hidden onto the live EstablishedContact before persister.store runs at line 311. If store fails, the caller preserves the ContactInfoDecrypt queue item for retry as the doc-comment requires, but the next attempt sees the already-mutated in-memory values at lines 291-294 and returns Ok(true) without retrying persistence. A transient write failure can therefore leave the metadata only in RAM and lose it on restart.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift:2159-2210: submitDashPayProfileWithSigner uses bare `_ = signer` keepalive instead of withExtendedLifetime
Carried forward — STILL VALID at HEAD af9031e3. The detached task still captures signer and immediately discards it with `_ = signer` at line 2160, then calls platform_wallet_create_or_update_dashpay_profile_with_signer with signerHandle at lines 2175 and 2190. That raw handle is dereferenced on the Rust side as a VTableSigner during block_on_worker. A bare discard is not a documented Swift lifetime barrier, while nearby signer-driven flows in this file use withExtendedLifetime for the full FFI call, so the last strong Swift reference can be released while Rust is still using the pointer.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:199-230: SQLite established-contact rows do not persist contact_account_label
Carried forward — STILL VALID at HEAD af9031e3. The established-contact INSERT/ON CONFLICT still writes wallet_id, owner_id, contact_id, state, outgoing_request, incoming_request, alias, note, is_hidden, accepted_accounts, and payment_channel_broken, but no contact_account_label column. The load path still reconstructs contact_account_label as None at line 347. Any stored contact_account_label survives only in memory until restart.
In `packages/rs-platform-wallet-ffi/src/established_contact.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/established_contact.rs:156-169: established_contact_get_note does not zero-init *out_note before fallible work
Carried forward — STILL VALID at HEAD af9031e3. After check_ptr!(out_note) at line 160, stale-handle lookup, missing note, and CString::new can all return before line 167 assigns *out_note. Callers that reuse an output variable can observe a stale pointer on failure instead of a stable null value; the sibling alias getter pattern should be mirrored here.
In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay_profile.rs:326-367: platform_wallet_create_or_update_dashpay_profile_with_signer does not zero-init *out_profile before fallible work
Carried forward — STILL VALID at HEAD af9031e3. This signer-backed profile FFI entry point still checks out_profile at line 326 but only writes *out_profile at line 367, after identifier decoding, optional string decoding, avatar copying, wallet lookup, signer dereference, and async profile submission can fail. Other profile FFI functions in this file initialize DashPayProfileFFI::empty() early, but this one does not, leaving a fragile ABI contract for callers that reuse output storage.
In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs:165-180: record_dashpay_payment inserts into the in-memory map before persisting; defeats caller-side contains_key retry guards
Carried forward — STILL VALID at HEAD af9031e3. The method still inserts into self.dashpay_payments at line 171 before snapshot_changeset and persister.store at line 179. If persistence fails, reconcile callers that skip already-seen txids via contains_key will not retry the failed row until restart, contradicting the self-healing sweep behavior described in the comment.
In `packages/rs-platform-wallet/src/wallet/identity/network/sdk_writer.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/sdk_writer.rs:281-291: DashPaySdkWriter::send_contact_request flattens dash_sdk::Error into InvalidIdentityData
Carried forward — STILL VALID at HEAD af9031e3. send_contact_request still maps every dash_sdk::Error into PlatformWalletError::InvalidIdentityData at lines 287-291. The sibling put_document path preserves PlatformWalletError::Sdk, so callers can distinguish retryable SDK/network failures from terminal validation failures there but not for contact-request sends.
In `packages/rs-platform-wallet/src/wallet/identity/crypto/auto_accept.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/crypto/auto_accept.rs:343-368: parse_dashpay_contact_uri base58-decodes untrusted dapk with no upper bound
Carried forward — STILL VALID at HEAD af9031e3. parse_dashpay_contact_uri still copies the raw dapk query value and calls bs58::decode(&dapk).into_vec() at lines 363-365 with no length precheck. Because dapk can come from QR/deeplink/paste input and the valid key blob is fixed-size, oversized input can force avoidable decode work and allocation before any semantic length check occurs.
Corrective exact-SHA review: carried-forward blockers were deduped from inline reposting, so this body-only review preserves the verifier's REQUEST_CHANGES action without opening duplicate inline threads.
…so the first payment after a contact accepts succeeds
DashPay payment failed in the original-sender -> accepter direction: after
Alice sends a contact request and Bob accepts, Alice's first send to Bob
failed with "No DashpayExternalAccount found". It's a drain-timing gap, not
an establishment gap — Alice's contact IS established with Bob's reciprocal
request and the RegisterExternal build IS enqueued, but it's completed only
by drain_pending_contact_crypto, which ran solely fire-and-forget on Keychain
unlock. send_payment failed the external-account lookup immediately without
draining, despite holding a signer. The accepter side worked because accept
builds the external account synchronously.
Fix: send_payment now takes a ContactCryptoProvider and drains
pending_contact_crypto before the external-account lookup (before acquiring
the wallet-manager write guard — the drain re-acquires that non-reentrant
lock). The FFI send export builds the provider from the same core_signer
handle via resolver_contact_crypto_provider; zero new FFI surface, zero
Swift change.
TDD (red->green, verified): send_payment_runs_pending_contact_crypto_drain
goes red with the drain disabled ("must drain ... before failing") and green
with the fix; send_payment_passes_external_lookup_once_account_built pins that
a pre-built external account clears the lookup. The full RegisterExternal
drain-then-send is covered by live testnet e2e (the mock SDK's single-shot
Identity::fetch makes it non-deterministic in-process).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Workflow K drives the in-app testnet faucet button by its stable accessibility id `receive.testnetFaucetButton` (~1 tDASH to the wallet's Core receive address), so e2e runs can self-fund without a manual faucet visit. Documents the Core-tab + testnet preconditions, the tap-by-AXUniqueId snippet, a balance-poll loop, and the rate-limit/web-fallback gotcha. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
suggested changes
Jun 26, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Incremental + cumulative review for bd2b4e9: the latest delta (af9031e..bd2b4e9) adds the send-payment deferred contact-crypto drain, FFI provider wiring, focused send-payment tests, and simulator-control faucet notes. I found no new latest-delta defects. All 9 prior af9031e findings are still valid at the current head and are carried forward below.
🔴 2 blocking | 🟡 6 suggestion(s) | 💬 1 nitpick(s)
2 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/wallet/identity/state/managed_identity/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs:299-311: Carried forward: set_contact_metadata still mutates memory before persistence succeeds
STILL VALID at bd2b4e91. The live EstablishedContact is still updated at lines 299-301 before persister.store runs at line 311. If the store fails, the ContactInfoDecrypt retry hook is defeated because the next call sees the in-memory alias/note/hidden values already equal and returns Ok(true) without retrying the durable write, so restart drops the metadata.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift:2159-2210: Carried forward: profile signer lifetime is still not pinned with withExtendedLifetime
STILL VALID at bd2b4e91. The detached profile submit task still uses `_ = signer` at line 2160, then passes the raw signerHandle into platform_wallet_create_or_update_dashpay_profile_with_signer at lines 2175/2190. Nearby signer-backed wrappers use withExtendedLifetime to make Swift retain the passUnretained KeychainSigner for the full synchronous FFI call; this path still lacks that lifetime barrier while Rust dereferences the signer callback context.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:199-230: Carried forward: SQLite established contacts still drop contact_account_label
STILL VALID at bd2b4e91. The established-contact INSERT/UPSERT still persists alias, note, is_hidden, accepted_accounts, and payment_channel_broken, but no contact_account_label column/value. The load path reconstructs contact_account_label as None, so decrypted account labels survive only in memory and are lost across restart.
In `packages/rs-platform-wallet-ffi/src/established_contact.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/established_contact.rs:156-169: Carried forward: established_contact_get_note still leaves out_note stale on early return
STILL VALID at bd2b4e91. After check_ptr!(out_note), stale-handle lookup, missing note, or CString::new can return before line 167 writes *out_note. Unlike safer getters that clear out params first, callers that reuse an output variable can observe or free a stale pointer on failure.
In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay_profile.rs:326-367: Carried forward: profile-with-signer FFI still does not clear out_profile before fallible work
STILL VALID at bd2b4e91. platform_wallet_create_or_update_dashpay_profile_with_signer checks out_profile at line 326 but only assigns *out_profile after all fallible identifier/string/avatar decoding, wallet lookup, signer dereference, and async profile submission succeeds. Any earlier error leaves caller-provided DashPayProfileFFI storage in its previous state.
In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs:171-179: Carried forward: record_dashpay_payment still inserts before persistence succeeds
STILL VALID at bd2b4e91. record_dashpay_payment still inserts into dashpay_payments at line 171 before snapshot_changeset and persister.store at line 179. If store fails, in-memory state is ahead of durable state and contains_key-based retry paths can be suppressed until restart; sent payment memo/status state then disappears on reload.
In `packages/rs-platform-wallet/src/wallet/identity/network/sdk_writer.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/sdk_writer.rs:287-291: Carried forward: send_contact_request still flattens SDK errors into InvalidIdentityData
STILL VALID at bd2b4e91. DashPaySdkWriter::send_contact_request still maps every dash_sdk::Error into PlatformWalletError::InvalidIdentityData with a string. The sibling put_document path preserves PlatformWalletError::Sdk, so contact-request callers still cannot distinguish retryable SDK/network failures from invalid identity data.
In `packages/rs-platform-wallet/src/wallet/identity/crypto/auto_accept.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/crypto/auto_accept.rs:363-366: Carried forward: parse_dashpay_contact_uri still base58-decodes unbounded dapk input
STILL VALID at bd2b4e91. The QR/deep-link parser still copies the untrusted dapk parameter and calls bs58::decode(&dapk).into_vec() without an encoded-length precheck. The valid blob is fixed-size, so hostile pasted/QR input can force oversized allocation and CPU before later rejection.
Comment on lines
199
to
+230
| @@ -225,6 +226,42 @@ pub fn apply( | |||
| established.note, | |||
| established.is_hidden as i64, | |||
| accepted, | |||
| established.payment_channel_broken as i64, | |||
| ])?; | |||
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Carried forward: SQLite established contacts still drop contact_account_label
STILL VALID at bd2b4e9. The established-contact INSERT/UPSERT still persists alias, note, is_hidden, accepted_accounts, and payment_channel_broken, but no contact_account_label column/value. The load path reconstructs contact_account_label as None, so decrypted account labels survive only in memory and are lost across restart.
source: ['native-general', 'native-security', 'native-rust-quality']
Comment on lines
+287
to
+291
| .map_err(|e| { | ||
| PlatformWalletError::InvalidIdentityData(format!( | ||
| "Failed to send contact request: {e}" | ||
| )) | ||
| }) |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Carried forward: send_contact_request still flattens SDK errors into InvalidIdentityData
STILL VALID at bd2b4e9. DashPaySdkWriter::send_contact_request still maps every dash_sdk::Error into PlatformWalletError::InvalidIdentityData with a string. The sibling put_document path preserves PlatformWalletError::Sdk, so contact-request callers still cannot distinguish retryable SDK/network failures from invalid identity data.
source: ['native-security', 'native-rust-quality', 'native-general']
Comment on lines
+363
to
+366
| let dapk = dapk.ok_or_else(|| invalid("contact URI missing dapk (key)"))?; | ||
| let key_blob = bs58::decode(&dapk) | ||
| .into_vec() | ||
| .map_err(|e| invalid(format!("dapk is not valid base58: {e}")))?; |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Carried forward: parse_dashpay_contact_uri still base58-decodes unbounded dapk input
STILL VALID at bd2b4e9. The QR/deep-link parser still copies the untrusted dapk parameter and calls bs58::decode(&dapk).into_vec() without an encoded-length precheck. The valid blob is fixed-size, so hostile pasted/QR input can force oversized allocation and CPU before later rejection.
source: ['native-security', 'native-rust-quality', 'native-general']
Comment on lines
+33
to
+51
| /// Per-sweep page budget. `contactRequest` documents are public and freely | ||
| /// indexable by `toUserId`, so a hostile sender can flood a target with cheap | ||
| /// throwaway requests; without a cap, every cold-start / restore sweep would | ||
| /// fetch and hold the entire spam set in memory at once. The fetch is | ||
| /// `$createdAt`-ascending and the caller's high-water cursor resumes from the | ||
| /// max `$createdAt` fetched, so capping pages spreads a large backlog across | ||
| /// sweeps oldest-first — nothing is buried or skipped, only deferred. 50 × 100 | ||
| /// = 5_000 documents per sweep, far above any legitimate pending-request count | ||
| /// (and a legit user above it just takes an extra sweep to fully ingest). | ||
| /// | ||
| /// Forward progress assumes no single `$createdAt` value holds ≥ this budget of | ||
| /// matching documents. `$createdAt` is block-granular (every doc in a block | ||
| /// shares the block time), so a same-`$createdAt` cluster is bounded by one | ||
| /// block's transaction capacity — far below 5_000 fee-paid, signed | ||
| /// `contactRequest`s. If that ever ceased to hold, the timestamp cursor could | ||
| /// not advance past such a cluster (it would re-read the same oldest 5_000 each | ||
| /// sweep); the fix would be a persisted `StartAfter` document-id continuation | ||
| /// cursor rather than the `$createdAt` high-water. | ||
| const MAX_CONTACT_REQUEST_PAGES_PER_SWEEP: u32 = 50; |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: Carried forward: page-budget docs still omit the 10-minute overlap livelock case
STILL VALID at bd2b4e9. The comment still documents same-$createdAt starvation, but not the wallet caller's repeated 10-minute rewind window. A dense multi-block cluster inside that overlap can keep every sweep spending the page budget on the same oldest prefix without reaching newer requests; a persisted document-id continuation or equivalent progress marker is still the durable fix.
source: ['native-security', 'native-rust-quality', 'native-general']
…sted profile, not the volatile cache DashPayTabView showed 'Set up your DashPay profile' for an identity that already has a profile. The prompt's else branch fires when ownProfile is nil, and ownProfile was loaded ONLY from the in-memory wallet-handle cache (getDashPayProfile()). That cache is populated lazily per-identity per-sync, so with many identities — on cold restore or right after a picker switch — it's empty for a not-yet-synced identity and the CTA showed despite a persisted profile existing. Fix: loadOwnProfileFromCache falls back to the persisted PersistentIdentity.dashpayProfile (the cascade-owned source of truth, written by the persister only after a profile is created/synced) when the live cache doesn't have it. The cache is still preferred when present. Verification: SwiftUI @State + wallet-handle + SwiftData entangled (no isolated unit seam) → verified by live reproduction, not a unit test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Incremental review at HEAD e69515f (delta af9031e..e69515f). The latest delta is small and clean: send_payment now drains the deferred contact-crypto queue via a new ContactCryptoProvider FFI thread, plus a Swift profile CTA is gated on the persisted profile rather than the volatile cache — no new defects introduced. However, ALL 9 prior findings from the af9031e review remain STILL VALID at HEAD: the cited files/lines were not touched. Carrying them forward, including 2 blocking issues (set_contact_metadata persist-after-mutate ordering, Swift signer keepalive). Codex ACP reviewers all failed authentication in this run — Codex coverage is degraded; only Claude reviewers and mandatory prior findings were used. CodeRabbit produced no inline findings.
🟡 2 suggestion(s) | 💬 1 nitpick(s)
2 additional finding(s) omitted (not in diff).
6 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/established_contact.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/established_contact.rs:156-169: established_contact_get_note does not zero-init *out_note before fallible work
Carried forward from af9031e3 — STILL VALID at HEAD e69515f4. After `check_ptr!(out_note)` at line 160, three paths return without ever assigning `*out_note`: stale handle (line 164), missing note (line 165), and `CString::new` failure on an interior NUL byte in attacker-influenced contactInfo (line 166). A C/Swift caller that reuses the same `*mut *mut c_char` slot across calls will read stale prior contents on a non-ok return and may call `established_contact_string_free` on a pointer that no longer belongs to the most recent call — a double-free or free-of-unrelated-allocation. One-line hardening: `*out_note = std::ptr::null_mut()` immediately after the null check. The same shape exists across the sibling FFI string-getters in this crate; audit them together.
In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay_profile.rs:326-367: platform_wallet_create_or_update_dashpay_profile_with_signer does not zero-init *out_profile before fallible work
Carried forward from af9031e3 — STILL VALID at HEAD e69515f4. `check_ptr!(out_profile)` is at line 326, but `*out_profile` is only written at line 367 after identifier decode (329), three opt-c-str decodes (331-333), the avatar slice copy (335-339), wallet lookup (343), signer dereference (353), and the async profile submit (354-362). Any early failure leaves `*out_profile` untouched. The current Swift wrapper happens to pre-initialize `var outProfile = DashPayProfileFFI()` so its `defer { dashpay_profile_ffi_free(&outProfile) }` is safe today, but the contract is asymmetric — any other FFI caller following 'declare-then-call' would crash on a non-ok return. Defensive fix: write a zeroed `DashPayProfileFFI::default()` to `*out_profile` immediately after `check_ptr!`. The same shape applies to the other `*_with_signer` profile entry points in this file.
Comment on lines
+33
to
+51
| /// Per-sweep page budget. `contactRequest` documents are public and freely | ||
| /// indexable by `toUserId`, so a hostile sender can flood a target with cheap | ||
| /// throwaway requests; without a cap, every cold-start / restore sweep would | ||
| /// fetch and hold the entire spam set in memory at once. The fetch is | ||
| /// `$createdAt`-ascending and the caller's high-water cursor resumes from the | ||
| /// max `$createdAt` fetched, so capping pages spreads a large backlog across | ||
| /// sweeps oldest-first — nothing is buried or skipped, only deferred. 50 × 100 | ||
| /// = 5_000 documents per sweep, far above any legitimate pending-request count | ||
| /// (and a legit user above it just takes an extra sweep to fully ingest). | ||
| /// | ||
| /// Forward progress assumes no single `$createdAt` value holds ≥ this budget of | ||
| /// matching documents. `$createdAt` is block-granular (every doc in a block | ||
| /// shares the block time), so a same-`$createdAt` cluster is bounded by one | ||
| /// block's transaction capacity — far below 5_000 fee-paid, signed | ||
| /// `contactRequest`s. If that ever ceased to hold, the timestamp cursor could | ||
| /// not advance past such a cluster (it would re-read the same oldest 5_000 each | ||
| /// sweep); the fix would be a persisted `StartAfter` document-id continuation | ||
| /// cursor rather than the `$createdAt` high-water. | ||
| const MAX_CONTACT_REQUEST_PAGES_PER_SWEEP: u32 = 50; |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: Per-sweep page budget plus wallet's 10-minute rewind window can livelock under contactRequest clusters
Carried forward from af9031e — STILL VALID at HEAD e69515f. The comment at 43-50 correctly identifies the same-$createdAt cluster bound but does not mention the wallet-side high-water rewind: the wallet rewinds its high-water by ~10 minutes on each sweep to tolerate slow indexing. A cluster spanning > page budget within that 10-minute window will be re-fetched from the same low-water mark every sweep, starving newer requests. Documenting the interaction helps future readers spot the failure mode; the structural fix (persisted StartAfter document-id cursor) is already noted.
source: ['claude']
thepastaclaw
suggested changes
Jun 26, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Incremental review at HEAD e69515f (delta af9031e..e69515f). The latest delta is small and clean: send_payment now drains the deferred contact-crypto queue via a new ContactCryptoProvider FFI thread, plus a Swift profile CTA is gated on the persisted profile rather than the volatile cache — no new defects introduced. However, ALL 9 prior findings from the af9031e review remain STILL VALID at HEAD: the cited files/lines were not touched. Carrying them forward, including 2 blocking issues (set_contact_metadata persist-after-mutate ordering, Swift signer keepalive). Codex ACP reviewers all failed authentication in this run — Codex coverage is degraded; only Claude reviewers and mandatory prior findings were used. CodeRabbit produced no inline findings.
🔴 2 blocking | 🟡 6 suggestion(s) | 💬 1 nitpick(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/state/managed_identity/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs:287-313: set_contact_metadata mutates in-memory state before persisting; equality short-circuit defeats the retry hook
Carried forward from af9031e3 — STILL VALID at HEAD e69515f4. Lines 299-301 assign `alias`, `note`, and `is_hidden` onto the live `EstablishedContact` BEFORE `persister.store(...)` at line 311. The doc comment at 274-280 explicitly states the caller must surface a persist Err so the `ContactInfoDecrypt` queue entry stays armed for retry. But on the retry pass, the equality short-circuit at 291-298 finds the in-memory contact already equal to `metadata` and returns `Ok(true)` without ever re-attempting persistence. Net effect: the on-disk row never picks up the alias/note/hidden change, even though the queue retry would otherwise have done so. Fix is to clone-and-persist-first, then commit to the live contact only on success.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift:2159-2210: submitDashPayProfileWithSigner uses bare `_ = signer` keepalive instead of withExtendedLifetime
Carried forward from af9031e3 — STILL VALID at HEAD e69515f4. The detached Task at line 2159 extracts `signerHandle = signer.handle` at line 2150 and then uses `_ = signer` at line 2160 as the only liveness barrier while Rust dereferences `signerHandle` as `*const VTableSigner` (dashpay_profile.rs:353) and invokes the signer's vtable callbacks during the async profile submission. A bare discard-assignment is NOT a documented Swift lifetime barrier — SIL/ARC optimization is free to release the captured `signer` after the discard executes since no further use exists in source. If that happens, `KeychainSigner` deinit tears down whatever the C vtable's `context` pointer references before Rust's async work completes, producing a use-after-free across the boundary. The Apple-documented primitive for guaranteeing a value outlives a call that consumes a derived raw pointer is `withExtendedLifetime`. Wrap the FFI invocation with `withExtendedLifetime(signer) { ... }`.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:199-230: SQLite established-contact rows do not persist contact_account_label
Carried forward from af9031e3 — STILL VALID at HEAD e69515f4. The INSERT/UPSERT at lines 200-212 covers `alias`, `note`, `is_hidden`, `accepted_accounts`, and `payment_channel_broken` — but never `contact_account_label`. The corresponding SELECT at 283-286 also omits the column, and load_state at line 347 explicitly reconstructs `contact_account_label: None`. The domain model writes this field via the post-ECDH receive sweep, but the value is silently dropped on restart; the next sweep then re-decrypts it (the `idempotent` short-circuit no longer fires). Either add the column + a migration and bind it in INSERT/SELECT, or document and enforce that the label is in-memory-only and stop writing it to the changeset.
In `packages/rs-platform-wallet-ffi/src/established_contact.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/established_contact.rs:156-169: established_contact_get_note does not zero-init *out_note before fallible work
Carried forward from af9031e3 — STILL VALID at HEAD e69515f4. After `check_ptr!(out_note)` at line 160, three paths return without ever assigning `*out_note`: stale handle (line 164), missing note (line 165), and `CString::new` failure on an interior NUL byte in attacker-influenced contactInfo (line 166). A C/Swift caller that reuses the same `*mut *mut c_char` slot across calls will read stale prior contents on a non-ok return and may call `established_contact_string_free` on a pointer that no longer belongs to the most recent call — a double-free or free-of-unrelated-allocation. One-line hardening: `*out_note = std::ptr::null_mut()` immediately after the null check. The same shape exists across the sibling FFI string-getters in this crate; audit them together.
In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay_profile.rs:326-367: platform_wallet_create_or_update_dashpay_profile_with_signer does not zero-init *out_profile before fallible work
Carried forward from af9031e3 — STILL VALID at HEAD e69515f4. `check_ptr!(out_profile)` is at line 326, but `*out_profile` is only written at line 367 after identifier decode (329), three opt-c-str decodes (331-333), the avatar slice copy (335-339), wallet lookup (343), signer dereference (353), and the async profile submit (354-362). Any early failure leaves `*out_profile` untouched. The current Swift wrapper happens to pre-initialize `var outProfile = DashPayProfileFFI()` so its `defer { dashpay_profile_ffi_free(&outProfile) }` is safe today, but the contract is asymmetric — any other FFI caller following 'declare-then-call' would crash on a non-ok return. Defensive fix: write a zeroed `DashPayProfileFFI::default()` to `*out_profile` immediately after `check_ptr!`. The same shape applies to the other `*_with_signer` profile entry points in this file.
In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs:165-180: record_dashpay_payment inserts into the in-memory map before persisting; defeats caller-side contains_key retry guards
Carried forward from af9031e3 — STILL VALID at HEAD e69515f4. Line 171 inserts into `dashpay_payments` before `snapshot_changeset()` at 172 and `persister.store` at 179. The doc comment correctly notes the send-path propagates Err so the UI sees the failure, but the self-healing receival recorder relies on `contains_key`-based idempotency: if the persister fails, the in-memory map already has the entry, so the next sweep skips durable retry until process restart re-loads from disk and finds the row absent. Same shape as set_contact_metadata — persist-first, then mutate.
In `packages/rs-platform-wallet/src/wallet/identity/network/sdk_writer.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/sdk_writer.rs:281-291: DashPaySdkWriter::send_contact_request flattens dash_sdk::Error into InvalidIdentityData
Carried forward from af9031e3 — STILL VALID at HEAD e69515f4. The closure at 287-291 maps every `dash_sdk::Error` (transport timeout, gRPC unavailable, consensus rejection) into `PlatformWalletError::InvalidIdentityData(format!(...))`, while sibling `put_document` at line 318 preserves the structured variant via `.map_err(PlatformWalletError::Sdk)`. The recurring sync's retry classifier cannot distinguish a transient network failure (retryable) from genuinely invalid identity data (not retryable), so transient failures get permanently classified and broken-channel tombstones may be set against errors that would clear on the next sweep. Mirror put_document's mapping.
In `packages/rs-platform-wallet/src/wallet/identity/crypto/auto_accept.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/crypto/auto_accept.rs:343-368: parse_dashpay_contact_uri base58-decodes untrusted dapk with no upper bound
Carried forward from af9031e3 — STILL VALID at HEAD e69515f4. `dapk = Some(v.to_string())` at line 356 copies the raw query value with no length precheck; `bs58::decode(&dapk).into_vec()` at line 364 runs against arbitrary input. Valid DIP-15 dapk blobs are fixed-size (≤69 bytes raw → ~94 base58 chars). An attacker-supplied URL/QR with a multi-KB `dapk=` value forces a proportional decode buffer allocation in a UI-facing path (QR scanner / deep link handler), and bs58's O(n²) decode pass can stall the UI thread under hostile input. Cheap mitigation: reject `dapk` if its encoded length exceeds ~128 chars.
In `packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs`:
- [NITPICK] packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs:33-51: Per-sweep page budget plus wallet's 10-minute rewind window can livelock under contactRequest clusters
Carried forward from af9031e3 — STILL VALID at HEAD e69515f4. The comment at 43-50 correctly identifies the same-`$createdAt` cluster bound but does not mention the wallet-side high-water rewind: the wallet rewinds its high-water by ~10 minutes on each sweep to tolerate slow indexing. A cluster spanning > page budget within that 10-minute window will be re-fetched from the same low-water mark every sweep, starving newer requests. Documenting the interaction helps future readers spot the failure mode; the structural fix (persisted `StartAfter` document-id cursor) is already noted.
Corrective exact-SHA review: the normal poster deduped carried-forward blockers from inline reposting and posted a COMMENT, so this body-only review preserves the verifier's REQUEST_CHANGES action without reopening duplicate inline threads. Erroneous reconciliation replies from the first post were deleted.
…emory so a retry isn't swallowed set_contact_metadata mutated the in-memory alias/note/hidden BEFORE persisting. On a store failure the change was left in memory, so the next retry hit the unchanged-equality short-circuit and returned Ok(true) WITHOUT persisting — permanently losing the alias/note (the exact retry hook the doc-comment promises). Addresses the thepastaclaw review thread. Fix: build the changeset from a copy carrying the new metadata, persist it, and only commit to the live contact after the store succeeds. On failure memory stays equal to the persisted state, so the retry re-persists. TDD: set_contact_metadata_retry_after_failed_persist_actually_persists is red (retry stores 0x — swallowed) before the fix, green after; existing set_contact_metadata_surfaces_persist_failure still passes (21/21 module). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ubmitDashPayProfileWithSigner The profile-submit FFI path kept the signer alive with a bare `_ = signer`, which the optimizer can drop before the synchronous platform_wallet_create_or_update_dashpay_profile_with_signer call returns. Rust holds a passUnretained ctx pointer to the KeychainSigner via signerHandle, so the call can use-after-free under -O and profile create/update can crash or fail. Wrap the FFI call in withExtendedLifetime(signer), matching every other *_with_signer wrapper in this file. Addresses the thepastaclaw review thread. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Latest delta (e69515f..90d96c1) cleanly resolves both prior blockers: set_contact_metadata now clones, persists, then commits to memory only on success (with a regression test pinning the retry path), and submitDashPayProfileWithSigner pins the signer with withExtendedLifetime around the synchronous FFI call. Carried-forward findings 3–9 are unchanged in their respective files and remain valid; none are in the latest delta's footprint but all sit in DashPay code paths central to this PR. Codex coverage was degraded across all three roles (auth failure); Claude reviewers completed all three. CodeRabbit supplied 0 inline findings.
🟡 4 suggestion(s) | 💬 1 nitpick(s)
2 additional finding(s) omitted (not in diff).
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/state/managed_identity/identity_ops.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs:165-180: record_dashpay_payment inserts into the in-memory map before persisting — defeats the same retry-guard pattern just fixed in set_contact_metadata
At line 171 the entry is inserted into `self.dashpay_payments` before `self.snapshot_changeset()` and `persister.store(...)` at 172/179. If the persist fails, the in-memory map already contains the entry, so any caller using `contains_key` as an idempotency/retry guard will skip the retry and the entry is permanently lost until process restart — exactly the anti-pattern that 56fb63e35e fixed in `set_contact_metadata` (contact_requests.rs:300-320 at HEAD, with the explanatory comment and the `set_contact_metadata_retry_after_failed_persist_actually_persists` regression test). The fix shape is identical: stage a new entry in a local, persist first, commit to `self.dashpay_payments` only on `Ok`. The doc comment on this method already promises persist-failure propagation, and `send_payment` relies on it — the in-memory side-effect happening before the surfaced error breaks that contract.
In `packages/rs-platform-wallet-ffi/src/established_contact.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/established_contact.rs:156-169: established_contact_get_note leaves *out_note unspecified on failure paths
After `check_ptr!(out_note)` at line 160, three early returns (stale handle via `unwrap_option_or_return!` at 164, missing note at 165, `CString::new` failure at 166) return a non-OK status without ever writing to `*out_note`. C/Swift callers that pass an uninitialised stack slot and inspect the out-pointer on a non-OK result will read garbage — a use-after-free / wild-pointer footgun across the FFI boundary. Defense in depth: write `*out_note = std::ptr::null_mut()` immediately after the null check, matching the pattern other accessors in this crate use.
In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay_profile.rs:326-367: platform_wallet_create_or_update_dashpay_profile_with_signer leaves *out_profile unspecified on failure paths
`*out_profile` is written only at line 367, after identifier decode, three optional C-string decodes, wallet lookup, signer dereference, and the async submit all succeed. Each prior step can early-return via `unwrap_*_or_return!`, leaving the caller's `out_profile` slot with whatever it contained on entry. The current Swift wrapper in this PR happens to pre-initialise with `DashPayProfileFFI()`, but the FFI contract still leaks uninitialised state to any other consumer (and to future Swift callers that allocate raw). Mitigation: write `*out_profile = DashPayProfileFFI::default()` immediately after the `check_ptr!` calls.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:199-230: Established-contact INSERT/UPSERT omits contact_account_label — contradicts the persist contract documented in store_contact_account_label
The INSERT for established contacts at lines 199-213 lists ten columns (state, outgoing_request, incoming_request, alias, note, is_hidden, accepted_accounts, payment_channel_broken) but never writes `contact_account_label`. The load path at line 347 reconstructs it as `None` with the comment 'this backend has no column for it, so it restores empty and re-derives on the next contact-info sweep' — i.e., intentional. But `store_contact_account_label` in `contact_requests.rs:2270-2271` explicitly documents 'Persisted via an `established` changeset entry — the same path as the broken-channel flag', and `payments.rs:2459-2473` asserts the label is read back after store. Either the storage backend needs the column (round-trip the field) or the producer's doc comment + test expectations need to be corrected to reflect the re-derive-on-restart design. Today the two layers disagree on whether the label persists, which is a fragile correctness contract to leave standing.
Comment on lines
199
to
+230
| @@ -225,6 +226,42 @@ pub fn apply( | |||
| established.note, | |||
| established.is_hidden as i64, | |||
| accepted, | |||
| established.payment_channel_broken as i64, | |||
| ])?; | |||
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Established-contact INSERT/UPSERT omits contact_account_label — contradicts the persist contract documented in store_contact_account_label
The INSERT for established contacts at lines 199-213 lists ten columns (state, outgoing_request, incoming_request, alias, note, is_hidden, accepted_accounts, payment_channel_broken) but never writes contact_account_label. The load path at line 347 reconstructs it as None with the comment 'this backend has no column for it, so it restores empty and re-derives on the next contact-info sweep' — i.e., intentional. But store_contact_account_label in contact_requests.rs:2270-2271 explicitly documents 'Persisted via an established changeset entry — the same path as the broken-channel flag', and payments.rs:2459-2473 asserts the label is read back after store. Either the storage backend needs the column (round-trip the field) or the producer's doc comment + test expectations need to be corrected to reflect the re-derive-on-restart design. Today the two layers disagree on whether the label persists, which is a fragile correctness contract to leave standing.
source: ['claude']
Comment on lines
+33
to
+51
| /// Per-sweep page budget. `contactRequest` documents are public and freely | ||
| /// indexable by `toUserId`, so a hostile sender can flood a target with cheap | ||
| /// throwaway requests; without a cap, every cold-start / restore sweep would | ||
| /// fetch and hold the entire spam set in memory at once. The fetch is | ||
| /// `$createdAt`-ascending and the caller's high-water cursor resumes from the | ||
| /// max `$createdAt` fetched, so capping pages spreads a large backlog across | ||
| /// sweeps oldest-first — nothing is buried or skipped, only deferred. 50 × 100 | ||
| /// = 5_000 documents per sweep, far above any legitimate pending-request count | ||
| /// (and a legit user above it just takes an extra sweep to fully ingest). | ||
| /// | ||
| /// Forward progress assumes no single `$createdAt` value holds ≥ this budget of | ||
| /// matching documents. `$createdAt` is block-granular (every doc in a block | ||
| /// shares the block time), so a same-`$createdAt` cluster is bounded by one | ||
| /// block's transaction capacity — far below 5_000 fee-paid, signed | ||
| /// `contactRequest`s. If that ever ceased to hold, the timestamp cursor could | ||
| /// not advance past such a cluster (it would re-read the same oldest 5_000 each | ||
| /// sweep); the fix would be a persisted `StartAfter` document-id continuation | ||
| /// cursor rather than the `$createdAt` high-water. | ||
| const MAX_CONTACT_REQUEST_PAGES_PER_SWEEP: u32 = 50; |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: Document the per-sweep page budget × 10-minute high-water rewind interaction
The comment block now (partially) calls out the same-$createdAt cluster stall, but does not mention its interaction with the wallet-side 10-minute high-water rewind: under a dense cluster of contactRequests, every cold-start sweep within the rewind window re-fetches the same cluster prefix. Low-impact (bounded by fees on the attacker side), but pairing the rewind window with the page cap in the comment would prevent a future contributor from tightening one in isolation and accidentally introducing a stall.
source: ['claude']
…lure so retries aren't swallowed record_dashpay_payment inserted the entry into memory BEFORE persisting, so on a store failure the entry was stranded in memory — a caller's contains_key retry guard then skipped the next sweep's re-attempt, permanently dropping a Sent entry + memo (no on-chain recovery). Same swallow class as the set_contact_metadata fix. Addresses the thepastaclaw review thread. Fix: roll the change back on store failure (restore the previous entry on a failed overwrite, else remove the fresh insert) so in-memory state stays equal to the persisted state and the retry re-attempts cleanly. TDD: record_dashpay_payment_rolls_back_fresh_on_persist_failure + record_dashpay_payment_restores_previous_on_failed_overwrite are red before the fix (entry stranded / overwrite kept 999 not 100), green after; 7/7 module. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nonical sim build + multi-sim install Make `bash packages/swift-sdk/build_ios.sh --target sim` the documented way to build for the simulator (don't hand-run xcodebuild/swift build), warn against --target mac for sim work, add the concurrent-build DerivedData corruption guard, and install-to-each-UDID for two-party (two-sim) testing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Latest delta (391abd4) cleanly resolves the prior record_dashpay_payment in-memory/persist divergence: the insert-then-conditional-rollback pattern is correct, with regression tests covering both fresh-insert and failed-overwrite paths. No new defects introduced in the delta. Reconciling the 7 prior dispatcher findings: 1 FIXED, 1 INTENTIONALLY DEFERRED (now documented in code), 5 STILL VALID and carried forward (4 suggestions + 1 nitpick).
🟡 2 suggestion(s) | 💬 1 nitpick(s)
2 additional finding(s) omitted (not in diff).
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/established_contact.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/established_contact.rs:156-169: established_contact_get_note leaves *out_note unspecified on failure paths
After `check_ptr!(out_note)` (which only validates non-null), three failure paths return a non-OK status without writing `*out_note`: stale handle (outer `unwrap_option_or_return!`), missing note (inner `unwrap_option_or_return!`), and `CString::new` failing on an interior NUL. The sibling `established_contact_get_alias` at line 108 correctly does `*out_alias = std::ptr::null_mut();` immediately after the null check — this function is asymmetric with that sibling, leaving the caller's `*mut *mut c_char` slot whatever it was before. Swift/C callers reading the out-pointer on a non-OK status (or passing uninitialised stack memory) will observe a stale or garbage pointer. Mirror the `_get_alias` pattern.
In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay_profile.rs:326-368: platform_wallet_create_or_update_dashpay_profile_with_signer leaves *out_profile unspecified on failure paths
`DashPayProfileFFI` is `#[repr(C)]` with three `*mut c_char` fields. `*out_profile` is only written on the happy path at line 367. Every early return — `read_identifier` failure, the three `decode_opt_c_str` calls, `with_item` returning `None` (stale handle), or the async submit failing — surfaces a non-OK status while the caller's slot retains whatever it held on entry. If a Swift caller mishandles the status or a wrapper later passes the struct into a `free_dashpay_profile`-style destructor, the result is dereferencing or freeing garbage pointers. The crate already exposes `DashPayProfileFFI::empty()` (lines 30-40) for exactly this purpose — zero-initialise the slot immediately after the `check_ptr!` calls so every failure path leaves a defined, harmless value.
Comment on lines
+33
to
+51
| /// Per-sweep page budget. `contactRequest` documents are public and freely | ||
| /// indexable by `toUserId`, so a hostile sender can flood a target with cheap | ||
| /// throwaway requests; without a cap, every cold-start / restore sweep would | ||
| /// fetch and hold the entire spam set in memory at once. The fetch is | ||
| /// `$createdAt`-ascending and the caller's high-water cursor resumes from the | ||
| /// max `$createdAt` fetched, so capping pages spreads a large backlog across | ||
| /// sweeps oldest-first — nothing is buried or skipped, only deferred. 50 × 100 | ||
| /// = 5_000 documents per sweep, far above any legitimate pending-request count | ||
| /// (and a legit user above it just takes an extra sweep to fully ingest). | ||
| /// | ||
| /// Forward progress assumes no single `$createdAt` value holds ≥ this budget of | ||
| /// matching documents. `$createdAt` is block-granular (every doc in a block | ||
| /// shares the block time), so a same-`$createdAt` cluster is bounded by one | ||
| /// block's transaction capacity — far below 5_000 fee-paid, signed | ||
| /// `contactRequest`s. If that ever ceased to hold, the timestamp cursor could | ||
| /// not advance past such a cluster (it would re-read the same oldest 5_000 each | ||
| /// sweep); the fix would be a persisted `StartAfter` document-id continuation | ||
| /// cursor rather than the `$createdAt` high-water. | ||
| const MAX_CONTACT_REQUEST_PAGES_PER_SWEEP: u32 = 50; |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: Page-budget doc does not mention the wallet-side 10-minute high-water rewind interaction
The forward-progress note covers same-$createdAt clusters (bounded by per-block tx capacity), but does not call out the interaction with the wallet caller's mandatory rewind window (SYNC_OVERLAP_MS, ~10 minutes) in contact_requests.rs. Each sweep re-issues the query at high_water − 10min, so on cold-start/restore a flood near the rewind boundary can re-enter the same window every sweep and consume the 5_000-doc budget on documents the wallet has already ingested. One sentence cross-referencing the rewind would help future maintainers tuning either side avoid an isolated regression.
source: ['claude']
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Cumulative re-review at HEAD 61d3d61. The delta since 391abd4 is purely cosmetic (rustfmt of two managed_identity test files plus a simulator-control SKILL.md doc update) and introduces no new substantive code. All 5 prior findings are STILL VALID at the current head and are carried forward verbatim. One additional new finding emerged from the FFI pass: platform_wallet_sync_contact_requests / platform_wallet_fetch_sent_contact_requests leave the repr(C) ContactRequestHandleArray out-pointer unwritten on every failure path, and because the host-side _free routine will Box::from_raw any non-null handles pointer with a nonzero count, this is strictly more dangerous than the analogous out_profile case. Suggestion-class hardening only; no blocking issues.
🟡 3 suggestion(s)
3 additional finding(s) omitted (not in diff).
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/established_contact.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/established_contact.rs:156-169: [carried-forward, STILL VALID] established_contact_get_note leaves *out_note unspecified on failure paths
STILL VALID at HEAD 61d3d61c — re-verified at established_contact.rs:156-169. After `check_ptr!(out_note)` (which only validates non-null), three failure paths return a non-OK status without writing `*out_note`: stale handle (outer `unwrap_option_or_return!`), missing note (inner `unwrap_option_or_return!`), and `CString::new` failing on an interior NUL. The sibling `established_contact_get_alias` at line 108 correctly does `*out_alias = std::ptr::null_mut();` immediately after the null check — this function is asymmetric with that sibling, leaving the caller's `*mut *mut c_char` slot whatever it was before. The new `dashpay_payment.rs::managed_identity_get_dashpay_payments` also adopts the zero-init pattern, making this inconsistency more visible. Swift/C callers reading the out-pointer on a non-OK status (or passing uninitialised stack memory) will observe a stale or garbage pointer, which becomes a free-of-garbage primitive if the wrapper later feeds it to a destructor on error paths.
In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay_profile.rs:326-368: [carried-forward, STILL VALID] platform_wallet_create_or_update_dashpay_profile_with_signer leaves *out_profile unspecified on failure paths
STILL VALID at HEAD 61d3d61c — re-verified at dashpay_profile.rs:314-369. `DashPayProfileFFI` is `#[repr(C)]` with three `*mut c_char` fields. `*out_profile` is only written on the happy path at line 367. Every early return — `read_identifier` failure, the three `decode_opt_c_str` calls, `with_item` returning `None` (stale handle), or the async submit failing — surfaces a non-OK status while the caller's slot retains whatever it held on entry. If a Swift caller mishandles the status or a wrapper later passes the struct into a `free_dashpay_profile`-style destructor, the result is dereferencing or freeing garbage pointers. The crate already exposes `DashPayProfileFFI::empty()` (lines 30-40) for exactly this purpose, and the newer `dashpay_payment` FFI in this PR demonstrates the same zero-init pattern — zero-initialise the slot immediately after the `check_ptr!` calls so every failure path leaves a defined, harmless value.
In `packages/rs-platform-wallet-ffi/src/dashpay.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay.rs:171-185: [NEW] platform_wallet_sync_contact_requests / fetch_sent leave *out_array (a repr(C) struct holding a heap pointer) unspecified on failure paths
Verified at dashpay.rs:171-185 and the sibling fetch_sent at 478-494. `ContactRequestHandleArray` is `#[repr(C)]` with `handles: *mut Handle` + `count: usize`, allocated by `from_requests` via `Box::into_raw` and freed by the host through `platform_wallet_contact_request_handle_array_free` (lines 144-158). In both entry points `check_ptr!(out_array)` only null-checks the pointer; if the async work errors or the handle lookup fails, `*out_array` is never written. The free routine only bails when `handles.is_null() || count == 0` (line 151), so a Swift caller that follows the natural `defer { platform_wallet_contact_request_handle_array_free(&array) }` pattern with uninitialised stack memory will pass garbage `handles` + nonzero `count` straight into `slice::from_raw_parts_mut` and `Box::from_raw` — a use-after-free / heap-corruption primitive worse than the `out_profile` case because the free actually dereferences the slot rather than just leaking C strings. `ContactRequestHandleArray::empty()` already exists at lines 102-109; zero-initialise the slot before any fallible work in both functions.
DashPay sync was a fixed 15s background poll, so a freshly received contact request / acceptance / payment could take up to ~15s to surface and an action you just took left the list stale until the next tick. Two UX-only changes (no Rust/FFI changes — both ride the existing set_interval + sync_now surface): - Foreground-fast / background-slow cadence. The DashPay tab drops the background loop's interval to 4s while it is on screen and restores 15s when it leaves, driven from the NavigationStack's onAppear/onDisappear so drilling into a contact detail or presenting a sheet (neither fires the stack's onDisappear) keeps the fast cadence — only a tab switch relaxes it. An idle, backgrounded app no longer sweeps every few seconds. - Post-mutation sync kick. After send-request / accept / QR-send / pay the handler fires a non-blocking dashPaySyncNow() (kickDashPaySync) so the counterparty's state and the established pair converge immediately instead of after the next poll tick. The sheet dismisses right away; the Rust manager folds an in-flight pass into a no-op via the single sync-in-progress signal. Complements the optimistic @query overlay (overlay = sender's own row, kick = the other side). Spec'd in docs/dashpay/SPEC.md §6.4 (sync-in-progress), the M2 task 9 notes, and a new dp_006b test-plan entry. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Incremental delta (61d3d61→56ff8ae3) is Swift-only — DashPay tab tunes the background poll cadence (4s foreground / 15s backgrounded) via setDashPaySyncInterval and adds a fire-and-forget kickDashPaySync after local mutations, plus SPEC.md prose. No new Rust/FFI surface, no new defects. All six prior findings remain STILL VALID and are carried forward; none are blocking.
🟡 4 suggestion(s)
3 additional finding(s) omitted (not in diff).
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/sdk_writer.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/sdk_writer.rs:287-291: send_contact_request collapses every dash_sdk::Error variant into InvalidIdentityData
Verified unchanged at HEAD. The `.map_err(|e| PlatformWalletError::InvalidIdentityData(format!("Failed to send contact request: {e}")))` closure erases the structured `dash_sdk::Error` taxonomy (network/transport, signing, broadcast rejection, consensus error, proof verification) into a single text-formatted `InvalidIdentityData` variant. The neighboring `put_document` (line 318, same trait impl) correctly uses `PlatformWalletError::Sdk(e)` to preserve variant, so this reads as an oversight rather than intentional categorization. Two concrete downstream consequences: (1) the `payment_channel_broken` policy added in this PR depends on classifying transient vs. permanent failures from machine-readable error structure; here Swift callers have to string-match. (2) The UI surfaces "invalid identity data" for what is actually a network timeout, misleading the user and complicating retry/backoff logic.
In `packages/rs-platform-wallet-ffi/src/established_contact.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/established_contact.rs:156-169: established_contact_get_note leaves *out_note unspecified on every failure path
Verified unchanged at HEAD. `check_ptr!(out_note)` guards null, then both `unwrap_option_or_return!` calls (storage miss → invalid handle; contact has no note) and the `unwrap_result_or_return!` on `CString::new` early-return without writing `*out_note`. Only the success path at line 167 assigns. A Swift/C caller that frees on every result — or that reads the out-pointer before checking the result code (a common defensive pattern for ARC bridges) — consumes uninitialized stack memory and may then `free`/`dash_sdk_string_free` a stale pointer (UB). Cheap fix: write `*out_note = std::ptr::null_mut()` immediately after `check_ptr!`.
In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay_profile.rs:326-368: platform_wallet_create_or_update_dashpay_profile_with_signer leaves *out_profile unspecified on failure
Verified unchanged at HEAD. After `check_ptr!(out_profile)` / `check_ptr!(signer_handle)`, every failure exit (`read_identifier`, the three `decode_opt_c_str` calls, the `with_item` returning None for an invalid wallet handle, the inner `Result<Profile, _>`) returns without writing `*out_profile`. Only line 367 (`*out_profile = DashPayProfileFFI::from_profile(&profile)`) writes. `DashPayProfileFFI` is a repr(C) struct that holds owning pointers (display name, public message, avatar URL, avatar bytes); a Swift caller that drops/frees the partially-filled struct on the error path will free uninitialized pointer bytes — UB / crash / double-free. Either initialize `*out_profile` to a sentinel/zero immediately after `check_ptr!`, or document loudly that the out-param is undefined on non-Success.
In `packages/rs-platform-wallet-ffi/src/dashpay.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay.rs:171-185: platform_wallet_sync_contact_requests / platform_wallet_fetch_sent_contact_requests leave *out_array unspecified on failure
Verified unchanged at HEAD for both `platform_wallet_sync_contact_requests` (171-185) and `platform_wallet_fetch_sent_contact_requests` (478-494). Both `check_ptr!(out_array)` then only write `*out_array = ContactRequestHandleArray::from_requests(list)` on the success path; every `unwrap_option_or_return!` / `unwrap_result_or_return!` failure path leaves the struct untouched. `ContactRequestHandleArray` is `{handles: *mut Handle, count: usize}` — a Swift caller that uses RAII/defer to unconditionally call `platform_wallet_contact_request_handle_array_free` after an error attempts `Box::from_raw` on a garbage pointer (UB / heap corruption). Zero-initialize to `{null, 0}` immediately after `check_ptr!`.
Comment on lines
+287
to
+291
| .map_err(|e| { | ||
| PlatformWalletError::InvalidIdentityData(format!( | ||
| "Failed to send contact request: {e}" | ||
| )) | ||
| }) |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: send_contact_request collapses every dash_sdk::Error variant into InvalidIdentityData
Verified unchanged at HEAD. The .map_err(|e| PlatformWalletError::InvalidIdentityData(format!("Failed to send contact request: {e}"))) closure erases the structured dash_sdk::Error taxonomy (network/transport, signing, broadcast rejection, consensus error, proof verification) into a single text-formatted InvalidIdentityData variant. The neighboring put_document (line 318, same trait impl) correctly uses PlatformWalletError::Sdk(e) to preserve variant, so this reads as an oversight rather than intentional categorization. Two concrete downstream consequences: (1) the payment_channel_broken policy added in this PR depends on classifying transient vs. permanent failures from machine-readable error structure; here Swift callers have to string-match. (2) The UI surfaces "invalid identity data" for what is actually a network timeout, misleading the user and complicating retry/backoff logic.
Suggested change
| .map_err(|e| { | |
| PlatformWalletError::InvalidIdentityData(format!( | |
| "Failed to send contact request: {e}" | |
| )) | |
| }) | |
| let xpub_bytes_clone = xpub_bytes.clone(); | |
| self.sdk | |
| .send_contact_request(send_input, ecdh_provider, |_account_ref: u32| async move { | |
| Ok::<Vec<u8>, dash_sdk::Error>(xpub_bytes_clone) | |
| }) | |
| .await | |
| .map_err(PlatformWalletError::Sdk) | |
| } |
source: ['claude']
Follow-up to 56ff8ae, addressing two gaps a multi-agent review of the diff surfaced: - Entering the tab didn't actually produce a fast tick. setDashPaySyncInterval only stores an atomic the loop reads on its *next* sleep (dashpay_sync.rs:157, no wakeup), so dropping to 4s left the current up-to-15s sleep running — on a plain tab re-entry (no identity change) .task(id:) doesn't re-run either, so there was no kick to mask it and the user could wait ~15s for the first refresh. Now foreground entry fires one kickDashPaySync (no-op if a pass is already in flight), so re-entry always sweeps immediately. (A Rust Notify on set_interval would shorten the in-flight sleep directly; deferred as an internal refinement — the entry kick gives the same user-visible result.) - Cadence keyed off tab visibility only, so backgrounding the app *while on the DashPay tab* left the interval stuck at 4s (TabView doesn't fire the stack's onDisappear on app background) — contradicting "an idle backgrounded app no longer sweeps every few seconds". Now gated on effective foreground = tab visible AND scenePhase == .active, recomputed from onAppear/onDisappear and onChange(of: scenePhase), acting only on transitions. Also softened the SPEC's "converge immediately" to "promptly (≤ the foreground cadence)" — the post-mutation kick no-ops against an already-in-flight pass rather than enqueuing a coalesced re-run. No automated test: this is SwiftUI-lifecycle + wall-clock timing (scenePhase / tab-visibility transitions and the leftover-sleep window), which the simulator harness can't assert deterministically — covered by the manual two-sim e2e per SPEC dp_006b. The underlying FFI set-interval/sync-now round-trip stays unit-tested Rust-side. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Latest delta (56ff8ae..fd45d03) is doc/Swift-only: SPEC.md updates plus DashPayTabView scene-phase-driven sync cadence with a one-shot foreground kick. The Swift cadence change correctly drives existing FFI (setDashPaySyncInterval / dashPaySyncNow) and introduces no new defects. No new in-scope findings in the delta. All six prior Rust/FFI findings were re-verified against the current head and remain STILL VALID at the cited line ranges — carried forward unchanged.
🟡 3 suggestion(s) | 💬 1 nitpick(s)
3 additional finding(s) omitted (not in diff).
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/established_contact.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/established_contact.rs:156-169: established_contact_get_note leaves *out_note uninitialized on every failure path (carried from prior review)
Verified at fd45d030 lines 156-169 — unchanged. After `check_ptr!(out_note)`, only the success arm writes through `out_note` (line 167). The three early-return paths — stale handle (`with_item` returns None), missing note (inner `Option<String>` None), and `CString::new` Err (note contains an interior NUL, reachable from untrusted contact metadata) — return a non-success result without ever assigning `*out_note`. A C/Swift caller following the standard pattern (uninitialized `var note: UnsafeMutablePointer<CChar>? = nil`, then unconditional inspect/free on any return code) will read undefined bytes and pass them to `cstring_free`, producing use-after-free / wild-pointer-free. The same pattern recurs across the get_alias / get_note_for_* family in this crate; consider fixing them consistently.
In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay_profile.rs:326-368: platform_wallet_create_or_update_dashpay_profile_with_signer leaves *out_profile uninitialized on failure (carried from prior review)
Verified at fd45d030 lines 326-368 — unchanged. After the two `check_ptr!` guards, every failure mode — `read_identifier` Err (329), three `decode_opt_c_str` Errs (331-333), stale wallet handle (365), or the async submit Err (366) — returns without writing `*out_profile`; only the success path at 367 assigns. `DashPayProfileFFI` is an owning repr(C) struct carrying heap pointers (display name / public message / avatar URL / avatar bytes). A Swift caller that declares a stack-allocated profile and unconditionally calls the FFI free routine on any non-OK result will free arbitrary memory. Failure paths are reachable from network (server rejection) and from caller-controlled C strings (UTF-8 decode failure), so this is not just internal-misuse exposure.
In `packages/rs-platform-wallet-ffi/src/dashpay.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay.rs:171-185: platform_wallet_sync_contact_requests / fetch_sent_contact_requests leave *out_array uninitialized on failure (carried from prior review)
Verified at fd45d030 lines 171-185 and 478-494 — both unchanged. Each function writes `*out_array = ContactRequestHandleArray::from_requests(list)` only on success. The failure paths (stale wallet handle, identifier decode error in the fetch variant, and the async sync/fetch Err) return without writing the slot. The documented free routine `platform_wallet_contact_request_handle_array_free` dereferences `{handles, count}` and iterates / `Box::from_raw`s each handle — a write-anywhere / heap-corruption primitive if the caller pre-declares the struct and frees on error (idiomatic Swift `defer { free(&arr) }`). The async error path is remotely reachable via any DAPI disruption during a wallet sync sweep.
Comment on lines
+33
to
+51
| /// Per-sweep page budget. `contactRequest` documents are public and freely | ||
| /// indexable by `toUserId`, so a hostile sender can flood a target with cheap | ||
| /// throwaway requests; without a cap, every cold-start / restore sweep would | ||
| /// fetch and hold the entire spam set in memory at once. The fetch is | ||
| /// `$createdAt`-ascending and the caller's high-water cursor resumes from the | ||
| /// max `$createdAt` fetched, so capping pages spreads a large backlog across | ||
| /// sweeps oldest-first — nothing is buried or skipped, only deferred. 50 × 100 | ||
| /// = 5_000 documents per sweep, far above any legitimate pending-request count | ||
| /// (and a legit user above it just takes an extra sweep to fully ingest). | ||
| /// | ||
| /// Forward progress assumes no single `$createdAt` value holds ≥ this budget of | ||
| /// matching documents. `$createdAt` is block-granular (every doc in a block | ||
| /// shares the block time), so a same-`$createdAt` cluster is bounded by one | ||
| /// block's transaction capacity — far below 5_000 fee-paid, signed | ||
| /// `contactRequest`s. If that ever ceased to hold, the timestamp cursor could | ||
| /// not advance past such a cluster (it would re-read the same oldest 5_000 each | ||
| /// sweep); the fix would be a persisted `StartAfter` document-id continuation | ||
| /// cursor rather than the `$createdAt` high-water. | ||
| const MAX_CONTACT_REQUEST_PAGES_PER_SWEEP: u32 = 50; |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: Per-sweep page-budget doc omits the wallet-side high-water rewind cross-reference (carried from prior review)
Verified at fd45d03 lines 33-51 — unchanged. The doc thoroughly explains the 5_000-document cap and the same-$createdAt-cluster forward-progress assumption, but does not cross-reference the wallet-side ~10-minute high-water rewind that tolerates timestamp skew across nodes and prevents edge-of-sweep gaps. A future maintainer reading only this comment could reasonably conclude the cursor is tight to max($createdAt) and remove the rewind, breaking the two halves of the contract. A one-line pointer at the wallet-side rewind locks them together.
source: ['claude']
Mirrors the "Set up your DashPay profile" CTA with a second, independent card: "Register a username". A DPNS username is the searchable handle other users type to find and add you; without one, people can't reach you by name. The copy makes the distinction explicit — the profile's display name is cosmetic and not searchable, the username is. To avoid the false-prompt bug (nagging an identity that already has a username, just not yet cached — the same class of bug we fixed for the profile prompt), the card only shows once an on-chain dpnsGetUsername check in .task confirms there's no name: a found name is persisted (card stays hidden), a definitive empty result marks the id resolved (card shows), and a thrown error (offline/transient) leaves it unresolved to retry rather than guessing. Mirrors IdentitiesView's lazy DPNS fetch. Tapping the card opens RegisterNameView; on success the Rust register path persists dpnsName, so the prompt hides reactively via @query. No automated test: this is a SwiftUI prompt gated on a network lookup + lifecycle (.task) — not deterministically assertable in the simulator harness; covered by the manual two-sim e2e. The dpnsGetUsername / register FFI paths are already exercised elsewhere. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Brings in the 2 new base commits (Swift app rebrand to "Dash Developer Pro" #3952; DAPI rate-limit-ban fix #3951). Only conflict was .claude/skills/simulator-control/SKILL.md (both sides edited it): resolved by taking the rebrand's new bundle id (org.dashfoundation.DashDeveloperPro) + its "find the .app by PRODUCT_NAME SwiftExampleApp.app, launch by bundle id" logic, while keeping this branch's multi-sim install loop. Also updated the faucet workflow's hardcoded get_app_container bundle id to match the rebrand. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
There was a problem hiding this comment.
Code Review
Latest delta (fd45d03..7b9022b) adds docs/dashpay/SPEC.md text and a DPNS-username prompt to DashPayTabView. One new in-scope defect in the Swift prompt: after registering from this sheet, the prompt does not hide because the Rust persister writes PersistentDPNSName rows, not the scalar PersistentIdentity.dpnsName/mainDpnsName that gates the prompt. All six prior findings (FFI uninitialized out-params, dash_sdk::Error collapse, unbounded base58 decode, page-budget doc nit) remain STILL VALID at HEAD at the same line ranges and are carried forward.
🟡 7 suggestion(s) | 💬 1 nitpick(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/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift:222-233: Username prompt stays visible after registering from DashPay tab
The prompt is gated by `!hasName && usernameResolvedIds.contains(identity.identityId)` where `hasName = (identity.mainDpnsName ?? identity.dpnsName).map { !$0.isEmpty } ?? false` (line 570). The author comment claims the prompt 'hides reactively' because the Rust IdentityChangeSet persists the new dpnsName, but `PlatformWalletPersistenceHandler.upsertDPNSNames` only inserts/refreshes `PersistentDPNSName` relationship rows — it never touches the scalar `PersistentIdentity.dpnsName` or `mainDpnsName` fields (search for `row.dpnsName =` / `row.mainDpnsName =` in PlatformWalletPersistenceHandler.swift returns nothing; the only writers are `PersistentIdentity.updateDpnsName` / `updateMainDpnsName`). So registering inline from this sheet leaves `hasName == false` and the identity still in `usernameResolvedIds`, and the 'Register a username' CTA remains until `.task(id: activeIdentity?.identityId)` re-fires (identity switch) or scenePhase becomes `.active` (background/foreground). Make `onRegistered` explicitly persist the new label and clear the resolved-id entry, or have it remove the identity from `usernameResolvedIds` so the prompt hides on the next render.
In `packages/rs-platform-wallet-ffi/src/established_contact.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/established_contact.rs:156-169: established_contact_get_note leaves *out_note uninitialized on every failure path
STILL VALID at HEAD 7b9022bb — lines 156-169 unchanged. After `check_ptr!(out_note)`, the function only writes through `out_note` on the success path (line 167). The three early-return paths — stale handle (`with_item` returns None), missing note (inner `Option<String>` None), and `CString::new` Err (reachable when a note contains an interior NUL) — return a non-OK `PlatformWalletFFIResult` without storing anything into `*out_note`. A C/Swift caller that follows the typical idiom of declaring an uninitialized `*mut c_char` slot and either reading/freeing it on any return code will read uninitialized memory and may invoke the string-free routine on a wild pointer. Initialize `*out_note = ptr::null_mut()` immediately after the pointer check. The same pattern appears across this crate's getter family.
In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay_profile.rs:326-368: platform_wallet_create_or_update_dashpay_profile_with_signer leaves *out_profile uninitialized on failure
STILL VALID at HEAD 7b9022bb — lines 326-368 unchanged. After the two `check_ptr!` guards, every fallible step before line 367 can early-return without initializing `*out_profile`: `read_identifier` Err (329), the three `decode_opt_c_str` Errs (331-333), `with_item` stale handle (365), or the async create/update Err (366). `DashPayProfileFFI` is an owning `repr(C)` struct carrying heap-allocated C-string pointers (display name, public message, avatar URL, avatar bytes). A Swift caller that defensively frees the struct on a non-OK result will free arbitrary stack contents. Some of these failure paths are reachable from caller-supplied C strings (UTF-8 decode failure) and from network/server rejection. Initialize the struct to its empty/default state immediately after pointer validation.
In `packages/rs-platform-wallet-ffi/src/dashpay.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay.rs:171-185: platform_wallet_sync_contact_requests leaves *out_array uninitialized on failure
STILL VALID at HEAD 7b9022bb — lines 171-185 unchanged. `check_ptr!(out_array)` only validates non-null; the `unwrap_option_or_return!(option)` (stale wallet handle) and `unwrap_result_or_return!(result)` (async sync error reachable on any DAPI disruption) paths return without writing `*out_array`. `ContactRequestHandleArray` is a `{handles: *mut Handle, count: usize}` value type that the documented free routine `platform_wallet_contact_request_handle_array_free` dereferences and iterates. A Swift caller that reuses a stack-allocated array and frees it on any return code can hand `Box::from_raw` an attacker-influenced pointer/count — a heap-corruption primitive. Zero the struct before any fallible work.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay.rs:478-494: platform_wallet_fetch_sent_contact_requests leaves *out_array uninitialized on failure
STILL VALID at HEAD 7b9022bb — lines 478-494 unchanged. Same ownership pattern as `platform_wallet_sync_contact_requests`: identifier decode (484), stale wallet handle (490), and the async fetch error (491) all return before `*out_array` is assigned. Apply the same zero-init-up-front fix so the documented array-free helper cannot be handed an uninitialized `{handles, count}` pair.
In `packages/rs-platform-wallet/src/wallet/identity/network/sdk_writer.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/sdk_writer.rs:287-291: send_contact_request collapses every dash_sdk::Error into InvalidIdentityData
STILL VALID at HEAD 7b9022bb — lines 287-291 unchanged. The closure flattens the entire `dash_sdk::Error` taxonomy (transport, signing, broadcast rejection, consensus error, proof verification, contract-bound errors) into a single `PlatformWalletError::InvalidIdentityData` formatted string. The sibling `put_document` writer in this same file correctly forwards `PlatformWalletError::Sdk(e)`. Downstream policy — including this PR series' DashPaySyncManager `payment_channel_broken` transient/permanent discriminator — cannot tell a retryable transport blip from a hard consensus rejection on the contact-request send path. Misclassification will both drive infinite retry of consensus-rejected transitions and defeat the new failure-policy code. `PlatformWalletError` already has `Sdk(#[from] dash_sdk::Error)`, so the fix is minimal.
In `packages/rs-platform-wallet/src/wallet/identity/crypto/auto_accept.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/crypto/auto_accept.rs:362-367: parse_dashpay_contact_uri base58-decodes untrusted dapk with no upper-bound length check
STILL VALID at HEAD 7b9022bb — lines 362-367 unchanged. `bs58::decode(&dapk).into_vec()` runs on attacker-controllable QR / `dash:` URI / deep-link input with no length precheck on `dapk`. The legitimate payload is a compact key blob (≤69 bytes → ~95 base58 chars). Base58 decode is O(n²) due to per-character big-int multiplication, so a 1 MB crafted `dapk` burns substantial CPU and allocates ~750 KB on the wallet's hot path — a trivial DoS against the Add-Contact / QR-scan flow on mobile. A cheap pre-decode length cap eliminates the amplification.
Carried-Forward Prior Finding
- [NITPICK] packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs:33-51: Per-sweep page-budget doc omits the wallet-side high-water rewind cross-reference
STILL VALID at HEAD 7b9022b — lines 33-51 unchanged. The doc thoroughly explains the 5_000-document cap and the same-$createdAt-cluster forward-progress assumption, but does not cross-reference the wallet-side ~10-minute high-water rewind that tolerates timestamp skew across nodes and prevents edge-of-sweep gaps. A future maintainer reading only this comment could reasonably conclude the cursor is tight tomax($createdAt)and remove the rewind, breaking the two halves of the contract. One-line pointer at the wallet-side rewind locks them together.
Repair note: this body was edited after posting so all verifier-confirmed carried-forward prior findings are visible in the top-level review, including non-diff findings that were already raised in earlier threads.
Comment on lines
+222
to
+233
| .sheet(isPresented: $showRegisterName) { | ||
| if let identity = activeIdentity { | ||
| // RegisterNameView brings its own NavigationView + | ||
| // Cancel toolbar; the Rust register path queues an | ||
| // IdentityChangeSet that persists the new dpnsName, | ||
| // so this prompt hides reactively — the onRegistered | ||
| // callback is a no-op here. | ||
| RegisterNameView(identity: identity, onRegistered: { _ in }) | ||
| .environmentObject(walletManager) | ||
| .environmentObject(appState) | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Username prompt stays visible after registering from DashPay tab
The prompt is gated by !hasName && usernameResolvedIds.contains(identity.identityId) where hasName = (identity.mainDpnsName ?? identity.dpnsName).map { !$0.isEmpty } ?? false (line 570). The author comment claims the prompt 'hides reactively' because the Rust IdentityChangeSet persists the new dpnsName, but PlatformWalletPersistenceHandler.upsertDPNSNames only inserts/refreshes PersistentDPNSName relationship rows — it never touches the scalar PersistentIdentity.dpnsName or mainDpnsName fields (search for row.dpnsName = / row.mainDpnsName = in PlatformWalletPersistenceHandler.swift returns nothing; the only writers are PersistentIdentity.updateDpnsName / updateMainDpnsName). So registering inline from this sheet leaves hasName == false and the identity still in usernameResolvedIds, and the 'Register a username' CTA remains until .task(id: activeIdentity?.identityId) re-fires (identity switch) or scenePhase becomes .active (background/foreground). Make onRegistered explicitly persist the new label and clear the resolved-id entry, or have it remove the identity from usernameResolvedIds so the prompt hides on the next render.
Suggested change
| .sheet(isPresented: $showRegisterName) { | |
| if let identity = activeIdentity { | |
| // RegisterNameView brings its own NavigationView + | |
| // Cancel toolbar; the Rust register path queues an | |
| // IdentityChangeSet that persists the new dpnsName, | |
| // so this prompt hides reactively — the onRegistered | |
| // callback is a no-op here. | |
| RegisterNameView(identity: identity, onRegistered: { _ in }) | |
| .environmentObject(walletManager) | |
| .environmentObject(appState) | |
| } | |
| } | |
| RegisterNameView(identity: identity, onRegistered: { label in | |
| PersistentIdentity.updateDpnsName( | |
| in: modelContext, | |
| identityId: identity.identityId, | |
| dpnsName: label | |
| ) | |
| usernameResolvedIds.remove(identity.identityId) | |
| try? modelContext.save() | |
| }) |
source: ['codex']
Collaborator
There was a problem hiding this comment.
Still present at 5ba83b3.
thepastaclaw
suggested changes
Jun 26, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
All eight prior findings from the 7b9022b review remain STILL VALID at HEAD 5ba83b3: the latest delta is a v3.1-dev merge (dashmate gateway config + rs-dapi-client RateLimit-Reset ban_for) plus a Swift bundle-id rebrand and does not touch any DashPay/FFI surface. No new in-scope defects in the delta itself. The dashmate schema/migration concern flagged by codex-general is intentional per the author's in-line justification (migration deliberately keyed to the rc.3 release bump) and arrived via the v3.1-dev merge, so it is recorded out-of-scope.
🔴 3 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
4 additional finding(s) omitted (not in diff).
4 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/dashpay.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/dashpay.rs:171-185: Carried forward: platform_wallet_sync_contact_requests leaves *out_array uninitialized on failure
Verified unchanged at HEAD 5ba83b31 (lines 171-185). After `check_ptr!(out_array)`, `*out_array` is only written on the success path (line 183). Both the stale-handle branch (`unwrap_option_or_return!(option)` at 181) and the async-sync error branch (`unwrap_result_or_return!(result)` at 182 — reachable on any DAPI disruption, made more frequent by the new rs-dapi-client ban_for path landed in this delta) return without storing into `*out_array`. `ContactRequestHandleArray` is a `{handles: *mut Handle, count: usize}` value type whose documented free routine `platform_wallet_contact_request_handle_array_free` dereferences `handles` and iterates `count`. A Swift caller using a stack-allocated array and unconditional cleanup-on-return hands `Box::from_raw` and a length-driven destroy loop an attacker-influenced pointer/count pair sourced from stack residue — a heap-corruption primitive triggerable by any transient sync failure. Zero the struct immediately after the pointer check.
- [BLOCKING] packages/rs-platform-wallet-ffi/src/dashpay.rs:478-494: Carried forward: platform_wallet_fetch_sent_contact_requests leaves *out_array uninitialized on failure
Verified unchanged at HEAD 5ba83b31 (lines 478-494). Same ownership-transfer pattern and same heap-corruption primitive as `platform_wallet_sync_contact_requests`: identifier decode (484), stale wallet handle (490), and async fetch error (491) all return before `*out_array` is assigned (492). Apply the same zero-init-up-front fix so the documented array-free helper cannot be handed an uninitialized `{handles, count}` pair from stack residue.
In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/dashpay_profile.rs:326-368: Carried forward: profile create-or-update leaves *out_profile uninitialized on failure
Verified unchanged at HEAD 5ba83b31 (lines 326-368). After the two `check_ptr!` guards, every fallible step before the success write at 367 can early-return without initializing `*out_profile`: `read_identifier` Err (329), the three `decode_opt_c_str` Errs (331-333), `with_item` stale handle (365), and the async create/update Err (366). `DashPayProfileFFI` is an owning `repr(C)` struct carrying heap-allocated C-string pointers (display name, public message, avatar URL, avatar bytes). A Swift caller that defensively frees the struct on a non-OK result frees arbitrary stack contents — an arbitrary-free primitive triggerable from caller-supplied non-UTF-8 strings or any normal Platform rejection. Initialize the struct to its empty/default sentinel immediately after pointer validation.
In `packages/rs-platform-wallet-ffi/src/established_contact.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/established_contact.rs:156-169: Carried forward: established_contact_get_note leaves *out_note uninitialized on every failure path
Verified unchanged at HEAD 5ba83b31 (lines 156-169). After `check_ptr!(out_note)`, `*out_note` is only written on the success path (line 167). The three early-return paths — stale handle (`with_item` None at 164), missing note (inner `Option<String>` None at 165), and `CString::new` Err when a note contains an interior NUL (166) — return without storing anything into `*out_note`. A Swift/C caller that declares an uninitialized `*mut c_char` slot and unconditionally frees or reads it on any return code will read uninitialized memory or invoke `platform_wallet_string_free` on a wild pointer. The set-note path round-trips user-supplied content, so a single NUL byte in a note triggers the failure path. Severity is lower than the (handles,count) cases above because only a single pointer is at risk.
Comment on lines
+67
to
+116
| /// Whether the recurring DashPay sync background loop is running. | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn platform_wallet_manager_dashpay_sync_is_running( | ||
| handle: Handle, | ||
| out_running: *mut bool, | ||
| ) -> PlatformWalletFFIResult { | ||
| check_ptr!(out_running); | ||
|
|
||
| let option = PLATFORM_WALLET_MANAGER_STORAGE | ||
| .with_item(handle, |manager| manager.dashpay_sync().is_running()); | ||
| let running = unwrap_option_or_return!(option); | ||
| *out_running = running; | ||
| PlatformWalletFFIResult::ok() | ||
| } | ||
|
|
||
| /// Whether a DashPay sync pass is currently in flight. | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn platform_wallet_manager_dashpay_sync_is_syncing( | ||
| handle: Handle, | ||
| out_syncing: *mut bool, | ||
| ) -> PlatformWalletFFIResult { | ||
| check_ptr!(out_syncing); | ||
|
|
||
| let option = PLATFORM_WALLET_MANAGER_STORAGE | ||
| .with_item(handle, |manager| manager.dashpay_sync().is_syncing()); | ||
| let syncing = unwrap_option_or_return!(option); | ||
| *out_syncing = syncing; | ||
| PlatformWalletFFIResult::ok() | ||
| } | ||
|
|
||
| /// Unix seconds of the last completed DashPay sync pass, or 0 if no | ||
| /// pass has ever completed. | ||
| /// | ||
| /// Unlike the identity-token coordinator this watermark is global (one | ||
| /// last-sync per manager, not per-identity), matching the | ||
| /// wallet-driven sweep. | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn platform_wallet_manager_dashpay_sync_last_sync_unix_seconds( | ||
| handle: Handle, | ||
| out_last_sync_unix: *mut u64, | ||
| ) -> PlatformWalletFFIResult { | ||
| check_ptr!(out_last_sync_unix); | ||
|
|
||
| let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(handle, |manager| { | ||
| manager.dashpay_sync().last_sync_unix_seconds() | ||
| }); | ||
| let value = unwrap_option_or_return!(option); | ||
| *out_last_sync_unix = value.unwrap_or(0); | ||
| PlatformWalletFFIResult::ok() | ||
| } |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: New dashpay-sync scalar getters repeat the uninitialized-out-param pattern
platform_wallet_manager_dashpay_sync_is_running (69-80), _is_syncing (84-95), and _last_sync_unix_seconds (104-116) each check_ptr! the out-pointer and then only write *out_* on the success path; the stale-handle branch (unwrap_option_or_return!) returns without ever assigning. Boundary impact is bounded vs. the heap-struct cases above — these are primitive bool / u64 scalars with no ownership transfer, so a Swift caller reads stale stack contents but no UB / no free-of-wild-pointer. Worth pinning so the new dashpay-sync FFI surface doesn't entrench the same anti-pattern as the established_contact / dashpay / dashpay_profile entry points. Initialize each out slot to false / 0 immediately after the pointer check.
source: ['claude']
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Milestone 1 of the DashPay completion plan (
docs/dashpay/SPEC.md, included in this PR with its research base). DashPay's contact-request flow was broken in four independent, previously-unknown ways:send_contact_requestwas rejected by consensus — the broadcast carried a document id derived from the creation entropy but fresh entropy in the transition; drive-abci recomputes the id and rejects withInvalidDocumentTransitionIdError.ExtendedPubKey::encode()form; DIP-15 and both reference mobile clients (iOS dash-shared-core, Android dashj) use the 69-byte compactfingerprint‖chaincode‖pubkey. Our send failed its own 96-byte ciphertext check; our receive couldn't parse mobile payloads.What was done?
Three logical commits:
docs(dashpay)— the 7-agent-reviewed implementation spec (protocol reference, per-layer inventory, gaps G1–G15, 5-milestone plan, Swift UI design, test plan) + 6 research files including the cross-client interop desk-check and the testnet key-purpose census.fix(sdk)!— entropy threading (ContactRequestResult.entropyreused at broadcast), the DIP-15 69-byte compact-xpub codec inplatform-encryption+ the SDK callback contract switched to it, and the recipient key-purpose assertion relaxed to DECRYPTION-or-ENCRYPTION.fix(platform-wallet)— new recurringDashPaySyncManager(iterates the wallets map, not the token registry; per-identity log-and-continue); ingest-guard relaxation + sent-side reconcile with idempotent, metadata-preserving merge; Accept adopts an existing on-platform reciprocal instead of re-broadcasting; per-sweep account rebuild (external and receiving accounts) with validate-before-ECDH, guard-drop lock ordering, and a transient/permanent failure policy (payment_channel_brokenflag, persisted + FFI accessor); rejected-request tombstone keyed(owner, sender, accountReference)so rotated requests still surface; 69-byte compact parsing on receive with address-equality pinned; key-purpose envelope aligned with on-chain reality;DashPaySdkWriterseam making the write paths testable.How Has This Been Tested?
TDD throughout — every behavioral fix has a test that was red against the unfixed code and green after (red→green evidence recorded in the SPEC.md M1 DONE notes and the three commit messages):
platform-wallet: 196 lib + 8 integration tests green (was 170 before this branch; +34 new)dash-sdk(--features mocks,offline-testing): 139 lib tests green (incl. the entropy-id and 69→96-byte pins)platform-encryption: 7/7 (the crate's test target previously failed to compile — fixed dev-deps)cargo checkclean onrs-sdk-ffi,platform-wallet-ffi,platform-wallet-storage; clippy clean on touched cratesdp_001..dp_006) is specced to ride the e2e framework in test(platform-wallet): e2e framework + full test suite — triage pins, Found-*/PA-* guards, fail-closed persist, Stage-2 merge #3549 and is explicitly not gated on this PR (SPEC.md Part 7.4)Note
CI:
Rust workspace tests / Tests (macOS)red on 3 pre-existing tests — passing locally.The macOS check fails only on three receiver-payment tests
(
register_contact_account_persists_account_registration,reconcile_records_received_payments_from_receival_utxos,reconcile_does_not_clobber_existing_entry_for_same_txid), all withExternal signable wallet has no private key.These pass locally in every configuration tested (9):
cargo test,cargo nextest(isolated and full platform-wallet suite), the CI feature set,
--all-features, theplatform-wallet-family feature unification, under
cargo llvm-covcoverage, and theexact CI package set (
drive+dpp+drive-abci+…--all-featuresunder coverage) —all on the same macOS/aarch64 as the runner. All green.
The wallet is provably
WalletType::Seed-bearing through every code path (from_seed→Seed; the manager'sinsert_walletstores it verbatim;get_walletreturns a&Wallet),yet only the CI runner reads it as
ExternalSignable. Root cause is a use-after-zeroize inthe
key-walletgit dependency:Wallethas aDropthat zeroizes itsZeroize-derivedwallet_type, so the discriminant can corrupt under a particular memory layout (UB isenvironment-dependent — it manifests on the CI runner but not locally). This is outside
this PR's code — pre-existing branch tests plus an external-dependency bug being tracked for
the key-wallet maintainers; the DashPay changes themselves are correct and green.
Breaking Changes
get_extended_public_keycallback contract forcreate_contact_request/send_contact_requestis now "return the 69-byte DIP-15 compact form" (was an encodedExtendedPubKey); validated before encryption.ContactRequestResultgains a publicentropy: Bytes32field. Thers-sdk-ffiC ABI is unchanged (caller doc contract tightened).contacts.payment_channel_brokencolumn,rejected_contact_requeststable) in the initial migration;ContactChangeSetgains arejectedfield.Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation