fix(platform-wallet)!: complete dashpay#3841
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
|
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
✨ 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 |
|
✅ Review complete (commit adb7b32) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3841 +/- ##
=============================================
- Coverage 71.20% 52.54% -18.66%
=============================================
Files 20 11 -9
Lines 2837 1707 -1130
=============================================
- Hits 2020 897 -1123
+ Misses 817 810 -7
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
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.
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
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
left a comment
thepastaclaw
left a comment
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
left a comment
thepastaclaw
left a comment
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
left a comment
thepastaclaw
left a comment
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.
|
Correction to my automated review for
|
A non-Rust embedder that calls dash_sdk_dashpay_create_contact_request and then submits the document via its own generic document-put couldn't recover the 32-byte entropy used to derive the document id — without it consensus rejects the create transition (it recomputes the id from the entropy). Add an inline `entropy: [u8;32]` field (POD, no separate free) populated from the already- available ContactRequestResult.entropy. (Deferred from the PR #3841 review.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
One blocking compile-break introduced by the latest delta: the SQLite contacts writer still references the removed ContactChangeSet::rejected field, breaking the default build of platform-wallet-storage. All 11 prior findings re-verified against HEAD as STILL VALID; carrying forward the top 9 by confidence (2 lowest-confidence nitpicks dropped to fit the 10-finding budget).
🔴 1 blocking | 🟡 7 suggestion(s) | 💬 2 nitpick(s)
10 additional finding(s)
blocking: SQLite contacts writer references removed ContactChangeSet::rejected field — will not compile
packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs (line 233)
The Spec 2 refactor in this PR removed ContactChangeSet::rejected and replaced it with sender-scoped ignored / unignored sets (packages/rs-platform-wallet/src/changeset/changeset.rs:619-646). The SQLite contacts writer here still reads cs.rejected.is_empty() and iterates cs.rejected.values() writing to the old rejected_contact_requests table. The sqlite feature is in the crate's default = ["sqlite", "cli", "secrets", "kv"] list (Cargo.toml:150), so the default build of platform-wallet-storage will fail to type-check against the post-delta platform-wallet API. Beyond compilation, the SQLite backend never persists ignored / unignored deltas, so any wallet running this backend would silently lose the local-only ignore state across reload — defeating Spec 2's persistence guarantee. Update this writer (and the corresponding load path and schema/migration) to store sender-scoped ignored rows with delete-on-unignore semantics matching the new changeset.
suggestion: Pending incoming requests fetch attacker-controlled avatar URLs before user consent
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift (line 119)
sync_contact_profiles populates profiles for every key in managed.incoming_contact_requests (unaccepted senders), and IncomingRequestRow feeds cachedProfile(row.contactIdentityId)?.avatarUrl into DashPayAvatarView → SwiftUI AsyncImage. Any party that publishes a dashpay.profile and sends an unsolicited contact request triggers an HTTPS request from the victim's device to an attacker-controlled host the moment the Requests view renders — leaking IP, online status, and TLS/UA fingerprint without any consent action. The new PersistentDashpayContactProfile + apply_contact_profile_rows restore makes the side channel survive across restarts and re-arm on resume. Rust-side is_valid_avatar_url only validates scheme and length, which does not address the tracking dimension. The Spec 2 local-only Ignore added in this PR helps after-the-fact mute but does not gate the pre-consent fetch. Suppress remote avatars Swift-side for unaccepted senders (initials only) until acceptance, or gate the persist FFI to only ship profiles for established contacts.
suggestion: Confirmed-absent contact profiles never delete their SwiftData row; stale data resurrects on cold start
packages/rs-platform-wallet-ffi/src/identity_persistence.rs (line 577)
allocate_contact_profile_rows continues past entries with profile == None ("Skip confirmed-absent entries — the negative cache is not persisted; it rebuilds on the next sweep"). Swift's upsertDashpayContactProfiles is append/refresh-only, and apply_contact_profile_rows rehydrates every restored row as ContactProfileEntry { profile: Some(..), .. }. When a contact removes their dashpay.profile, apply_fetched_profile flips the in-memory entry to None, but the absence is dropped from the FFI projection — the stale PersistentDashpayContactProfile row survives. On cold start the cache rehydrates as Some(stale_profile), the UI displays the old name, and the device re-issues a GET to the previously attacker-controlled avatar URL. Compounds the pre-accept avatar fetch above into a tracking channel that outlasts the on-chain profile removal. Project absences across the boundary as an explicit delete intent (mirroring the *const [u8;32] removal-set shape this PR introduced for ignored senders) so SwiftData can prune confirmed-absent rows.
suggestion: Create-only contact-request C ABI drops consensus-critical entropy
packages/rs-sdk-ffi/src/dashpay/contact_request.rs (line 181)
DashSDKContactRequestResult exposes only document_id, owner_id, and properties_json. The Rust ContactRequestResult carries 32-byte creation entropy because Drive recomputes the document id from (contract_id, owner_id, document_type_name, entropy) and rejects mismatches with InvalidDocumentTransitionIdError — the exact failure this PR fixes for the Rust-side atomic send path. dash_sdk_dashpay_create_contact_request is a create-only export across the C ABI: any non-Swift caller that creates here and broadcasts via a separate path (cold-signer / hardware-wallet flows) cannot regenerate a matching id at broadcast time, silently re-introducing the consensus rejection across the FFI boundary. Surface the 32-byte entropy on the result struct so the C contract matches the Rust contract.
/// Result of creating a contact request
#[repr(C)]
pub struct DashSDKContactRequestResult {
/// Document ID as hex string
pub document_id: *mut std::os::raw::c_char,
/// Owner ID (sender ID) as hex string
pub owner_id: *mut std::os::raw::c_char,
/// Document properties as JSON string
pub properties_json: *mut std::os::raw::c_char,
/// 32-byte creation entropy. Drive recomputes the document id from
/// (contract_id, owner_id, document_type_name, entropy); the caller
/// MUST reuse this exact entropy when later broadcasting the document
/// or the transition is rejected with InvalidDocumentTransitionIdError.
pub entropy: [u8; 32],
}
suggestion: established_contact_get_note skips the pre-null write its sibling getter performs
packages/rs-platform-wallet-ffi/src/established_contact.rs (line 156)
Sibling established_contact_get_alias writes *out_alias = std::ptr::null_mut(); at line 108 immediately after check_ptr!, so every early-return path leaves the out pointer at defined null. established_contact_get_note (lines 156-169) only calls check_ptr!(out_note) — on NotFound, None, or CString interior-NUL paths *out_note is never written. A C/Swift caller that does not pre-zero the slot reads whichever stale pointer value was there and may pass it to platform_wallet_string_free, causing invalid-free or double-free. The exported C ABI should not depend on caller hygiene; mirror the sibling pattern.
#[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);
*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: contactInfo fetch caps at 100 docs and allocates the next derivation slot from a truncated set
packages/rs-platform-wallet/src/wallet/identity/network/contact_info.rs (line 94)
fetch_decrypted_contact_infos issues one contactInfo query with limit: 100, start: None and treats the returned vector as complete. The same loop builds the per-root_index high_water map from that truncated page, and the publish path later chooses high_water + 1. Once an identity owns more than 100 contactInfo docs, the 101st+ are not applied during sync and publish can compute the next derivation index from only the first page. With 101 existing docs ordered ascending by $updatedAt, the first page contains indices 0..99, so the next write picks 100 even though index 100 already exists on-chain — the unique ($ownerId, rootEncryptionKeyIndex, derivationEncryptionKeyIndex) index then rejects the update. Drain all pages with Start::StartAfter, or query by max(derivation_index) for the relevant root index, before allocating.
suggestion: Contact-profile refresh timestamps are not durably persisted when the profile is unchanged
packages/rs-platform-wallet/src/wallet/identity/network/profile.rs (line 588)
apply_fetched_profile always refreshes checked_at_ms but returns changed only when the stored profile field differs; sync_contact_profiles calls persister.store only when any_changed is true. A re-check confirming the same present (or confirmed-absent) profile refreshes checked_at_ms in memory only — the FFI persistence callback never fires. After cold start should_fetch_profile immediately re-issues an In query for every unchanged contact: the negative-cache backoff effectively does not survive restart, amplifying the avatar-URL tracking side channel above and the cold-start fetch storm below. A failed earlier store is also not retried because later sweeps see changed = false. Return a separate needs_persist signal from apply_fetched_profile for timestamp-only refreshes.
suggestion: First-sync/cold-start retrieve-all has no per-call or total document budget; aggravated by the new un-ignore cursor rewind
packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs (line 42)
fetch_contact_requests_paginated drains every matching document with CONTACT_REQUEST_PAGE_SIZE = 100 per page but has no per-call total cap, elapsed-time budget, or caller-visible continuation. On cold start, restore-from-seed, or any after_created_at == None path, a long-lived identity with thousands of historical contact requests forces an O(N/100) proof-verified round-trip storm on every wallet open. The Spec 2 delta sharpens the trigger: unignore_sender resets high_water_received_ms to None, so any un-ignore (a UI action) drops the next sweep onto the full retrieve-all path. Combined with the recurring DashPaySyncManager tick, one oversized identity (organic growth, attacker-spammed contactRequest documents varying accountReference, or an attacker who induces repeated un-ignores) can monopolize the sync loop and starve other identities. Add a configurable total-document ceiling per sweep with a resumable cursor, or at minimum a logged warn + early break above a threshold.
nitpick: Restore/persist FFI structs (Payment, ContactProfile, ContactProfileRow) lack compile-time layout guards
packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs (line 339)
Sibling FFI structs are guarded (AccountAddressPoolFFI, ContactRequestFFI, ContactRequestRemovalFFI, the new ContactIgnoredSenderFFI, IdentityKeyEntryFFI, IdentityEntryFFI). Three #[repr(C)] structs crossing the same restore/persist boundary still lack const _: [u8; N] = [0u8; size_of::<T>()] + align_of tripwires: PaymentRestoreEntryFFI (wallet_restore_types.rs:354-368), ContactProfileRestoreEntryFFI (wallet_restore_types.rs:385-411), and the persist-side ContactProfileRowFFI (identity_persistence.rs:181-211). The parent IdentityEntryFFI guard pins the pointer field but not the inner row layout Swift indexes through. A field reorder, type widen, or padding change on either side would compile silently and shift offsets relative to the Swift mirror, potentially restoring profiles under the wrong contact id. Add the same const-assert idiom to all three structs.
nitpick: Profile-reader trio leaves out-params undefined on early-return paths
packages/rs-platform-wallet-ffi/src/dashpay_profile.rs (line 90)
managed_identity_get_dashpay_profile, platform_wallet_get_dashpay_profile, and platform_wallet_get_contact_profile only write *out_profile / *out_has_profile on the success/no-profile match arms. After check_ptr!, the early-return paths from unwrap_option_or_return! (handle not in storage → NotFound) and unwrap_result_or_return! (invalid identifier bytes → InvalidParameter) leave both out pointers indeterminate. Sibling readers (established_contact_get_alias, the dashpay_sync readers) pre-write out values right after check_ptr!. A C caller that reads *out_profile regardless of result code may interpret garbage as live pointer fields and pass them to dashpay_profile_ffi_free. Pre-write *out_profile = DashPayProfileFFI::empty() and *out_has_profile = false right after the check_ptr! calls.
🤖 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-storage/src/sqlite/schema/contacts.rs`:233-255: SQLite contacts writer references removed ContactChangeSet::rejected field — will not compile
The Spec 2 refactor in this PR removed `ContactChangeSet::rejected` and replaced it with sender-scoped `ignored` / `unignored` sets (`packages/rs-platform-wallet/src/changeset/changeset.rs:619-646`). The SQLite contacts writer here still reads `cs.rejected.is_empty()` and iterates `cs.rejected.values()` writing to the old `rejected_contact_requests` table. The `sqlite` feature is in the crate's `default = ["sqlite", "cli", "secrets", "kv"]` list (`Cargo.toml:150`), so the default build of `platform-wallet-storage` will fail to type-check against the post-delta `platform-wallet` API. Beyond compilation, the SQLite backend never persists `ignored` / `unignored` deltas, so any wallet running this backend would silently lose the local-only ignore state across reload — defeating Spec 2's persistence guarantee. Update this writer (and the corresponding load path and schema/migration) to store sender-scoped ignored rows with delete-on-unignore semantics matching the new changeset.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift`:119-129: Pending incoming requests fetch attacker-controlled avatar URLs before user consent
`sync_contact_profiles` populates profiles for every key in `managed.incoming_contact_requests` (unaccepted senders), and `IncomingRequestRow` feeds `cachedProfile(row.contactIdentityId)?.avatarUrl` into `DashPayAvatarView` → SwiftUI `AsyncImage`. Any party that publishes a `dashpay.profile` and sends an unsolicited contact request triggers an HTTPS request from the victim's device to an attacker-controlled host the moment the Requests view renders — leaking IP, online status, and TLS/UA fingerprint without any consent action. The new `PersistentDashpayContactProfile` + `apply_contact_profile_rows` restore makes the side channel survive across restarts and re-arm on resume. Rust-side `is_valid_avatar_url` only validates scheme and length, which does not address the tracking dimension. The Spec 2 local-only Ignore added in this PR helps after-the-fact mute but does not gate the pre-consent fetch. Suppress remote avatars Swift-side for unaccepted senders (initials only) until acceptance, or gate the persist FFI to only ship profiles for established contacts.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/identity_persistence.rs`:577-621: Confirmed-absent contact profiles never delete their SwiftData row; stale data resurrects on cold start
`allocate_contact_profile_rows` continues past entries with `profile == None` ("Skip confirmed-absent entries — the negative cache is not persisted; it rebuilds on the next sweep"). Swift's `upsertDashpayContactProfiles` is append/refresh-only, and `apply_contact_profile_rows` rehydrates every restored row as `ContactProfileEntry { profile: Some(..), .. }`. When a contact removes their `dashpay.profile`, `apply_fetched_profile` flips the in-memory entry to `None`, but the absence is dropped from the FFI projection — the stale `PersistentDashpayContactProfile` row survives. On cold start the cache rehydrates as `Some(stale_profile)`, the UI displays the old name, and the device re-issues a GET to the previously attacker-controlled avatar URL. Compounds the pre-accept avatar fetch above into a tracking channel that outlasts the on-chain profile removal. Project absences across the boundary as an explicit delete intent (mirroring the `*const [u8;32]` removal-set shape this PR introduced for ignored senders) so SwiftData can prune confirmed-absent rows.
- [SUGGESTION] In `packages/rs-sdk-ffi/src/dashpay/contact_request.rs`:181-188: Create-only contact-request C ABI drops consensus-critical entropy
`DashSDKContactRequestResult` exposes only `document_id`, `owner_id`, and `properties_json`. The Rust `ContactRequestResult` carries 32-byte creation entropy because Drive recomputes the document id from `(contract_id, owner_id, document_type_name, entropy)` and rejects mismatches with `InvalidDocumentTransitionIdError` — the exact failure this PR fixes for the Rust-side atomic send path. `dash_sdk_dashpay_create_contact_request` is a create-only export across the C ABI: any non-Swift caller that creates here and broadcasts via a separate path (cold-signer / hardware-wallet flows) cannot regenerate a matching id at broadcast time, silently re-introducing the consensus rejection across the FFI boundary. Surface the 32-byte entropy on the result struct so the C contract matches the Rust contract.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/established_contact.rs`:156-169: established_contact_get_note skips the pre-null write its sibling getter performs
Sibling `established_contact_get_alias` writes `*out_alias = std::ptr::null_mut();` at line 108 immediately after `check_ptr!`, so every early-return path leaves the out pointer at defined null. `established_contact_get_note` (lines 156-169) only calls `check_ptr!(out_note)` — on `NotFound`, `None`, or CString interior-NUL paths `*out_note` is never written. A C/Swift caller that does not pre-zero the slot reads whichever stale pointer value was there and may pass it to `platform_wallet_string_free`, causing invalid-free or double-free. The exported C ABI should not depend on caller hygiene; mirror the sibling pattern.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/identity/network/contact_info.rs`:94-166: contactInfo fetch caps at 100 docs and allocates the next derivation slot from a truncated set
`fetch_decrypted_contact_infos` issues one `contactInfo` query with `limit: 100, start: None` and treats the returned vector as complete. The same loop builds the per-`root_index` `high_water` map from that truncated page, and the publish path later chooses `high_water + 1`. Once an identity owns more than 100 contactInfo docs, the 101st+ are not applied during sync and publish can compute the next derivation index from only the first page. With 101 existing docs ordered ascending by `$updatedAt`, the first page contains indices 0..99, so the next write picks 100 even though index 100 already exists on-chain — the unique `($ownerId, rootEncryptionKeyIndex, derivationEncryptionKeyIndex)` index then rejects the update. Drain all pages with `Start::StartAfter`, or query by `max(derivation_index)` for the relevant root index, before allocating.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/identity/network/profile.rs`:588-725: Contact-profile refresh timestamps are not durably persisted when the profile is unchanged
`apply_fetched_profile` always refreshes `checked_at_ms` but returns `changed` only when the stored `profile` field differs; `sync_contact_profiles` calls `persister.store` only when `any_changed` is true. A re-check confirming the same present (or confirmed-absent) profile refreshes `checked_at_ms` in memory only — the FFI persistence callback never fires. After cold start `should_fetch_profile` immediately re-issues an `In` query for every unchanged contact: the negative-cache backoff effectively does not survive restart, amplifying the avatar-URL tracking side channel above and the cold-start fetch storm below. A failed earlier store is also not retried because later sweeps see `changed = false`. Return a separate `needs_persist` signal from `apply_fetched_profile` for timestamp-only refreshes.
- [SUGGESTION] In `packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs`:42-112: First-sync/cold-start retrieve-all has no per-call or total document budget; aggravated by the new un-ignore cursor rewind
`fetch_contact_requests_paginated` drains every matching document with `CONTACT_REQUEST_PAGE_SIZE = 100` per page but has no per-call total cap, elapsed-time budget, or caller-visible continuation. On cold start, restore-from-seed, or any `after_created_at == None` path, a long-lived identity with thousands of historical contact requests forces an O(N/100) proof-verified round-trip storm on every wallet open. The Spec 2 delta sharpens the trigger: `unignore_sender` resets `high_water_received_ms` to `None`, so any un-ignore (a UI action) drops the next sweep onto the full retrieve-all path. Combined with the recurring `DashPaySyncManager` tick, one oversized identity (organic growth, attacker-spammed `contactRequest` documents varying `accountReference`, or an attacker who induces repeated un-ignores) can monopolize the sync loop and starve other identities. Add a configurable total-document ceiling per sweep with a resumable cursor, or at minimum a logged warn + early break above a threshold.
- [NITPICK] In `packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs`:339-411: Restore/persist FFI structs (Payment, ContactProfile, ContactProfileRow) lack compile-time layout guards
Sibling FFI structs are guarded (`AccountAddressPoolFFI`, `ContactRequestFFI`, `ContactRequestRemovalFFI`, the new `ContactIgnoredSenderFFI`, `IdentityKeyEntryFFI`, `IdentityEntryFFI`). Three `#[repr(C)]` structs crossing the same restore/persist boundary still lack `const _: [u8; N] = [0u8; size_of::<T>()]` + `align_of` tripwires: `PaymentRestoreEntryFFI` (wallet_restore_types.rs:354-368), `ContactProfileRestoreEntryFFI` (wallet_restore_types.rs:385-411), and the persist-side `ContactProfileRowFFI` (identity_persistence.rs:181-211). The parent `IdentityEntryFFI` guard pins the pointer field but not the inner row layout Swift indexes through. A field reorder, type widen, or padding change on either side would compile silently and shift offsets relative to the Swift mirror, potentially restoring profiles under the wrong contact id. Add the same const-assert idiom to all three structs.
- [NITPICK] In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:90-185: Profile-reader trio leaves out-params undefined on early-return paths
`managed_identity_get_dashpay_profile`, `platform_wallet_get_dashpay_profile`, and `platform_wallet_get_contact_profile` only write `*out_profile` / `*out_has_profile` on the success/no-profile match arms. After `check_ptr!`, the early-return paths from `unwrap_option_or_return!` (handle not in storage → NotFound) and `unwrap_result_or_return!` (invalid identifier bytes → InvalidParameter) leave both out pointers indeterminate. Sibling readers (`established_contact_get_alias`, the dashpay_sync readers) pre-write out values right after `check_ptr!`. A C caller that reads `*out_profile` regardless of result code may interpret garbage as live pointer fields and pass them to `dashpay_profile_ffi_free`. Pre-write `*out_profile = DashPayProfileFFI::empty()` and `*out_has_profile = false` right after the `check_ptr!` calls.
Inline dry-run could not load the GitHub PR diff because this PR exceeds GitHub's 20,000-line diff limit, so I posted the same verified findings as a top-level review body.
|
Correction to my same-SHA review at nitpick: Same-$createdAt pagination boundary relies on undocumented Drive cursor secondary order
The contact-request query orders only by nitpick: Accept-adopt branch can leave a reciprocal request unestablished
When |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative verification at 1ea4d52. Two prior findings are FIXED in the latest delta: entropy on DashSDKContactRequestResult (514b32e) and contactInfo retrieve-all pagination (e757d9a). Carried forward as STILL VALID: the SQLite contacts writer still references the removed ContactChangeSet::rejected field — confirmed compile break — plus six lower-severity hardening items. One new low-severity finding from the latest delta: payments_for_contact's doc claims newest-first but BTreeMap<String,_> iteration is lexicographic by hex txid (PaymentEntry has no timestamp). Review action: REQUEST_CHANGES on the blocking compile error.
Reviewed commit: 1ea4d52
🔴 1 blocking | 🟡 6 suggestion(s) | 💬 2 nitpick(s)
Verified findings
blocking: [Carried forward — STILL VALID] SQLite contacts writer references removed ContactChangeSet::rejected — crate fails to compile
packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs (line 233)
Verified at head 1ea4d52. The Spec 2 refactor in this PR (commit 62b7ad1) replaced ContactChangeSet::rejected with sender-scoped ignored/unignored sets (packages/rs-platform-wallet/src/changeset/changeset.rs:619-666). The SQLite writer here still reads cs.rejected.is_empty() (line 233), cs.rejected.values() (line 246), and entry.account_reference / entry.document_id (lines 251-252). cargo check -p platform-wallet-storage fails with E0609 on lines 233 and 246, plus an E0282 on line 252; the workspace cannot build. The PR description advertises the rejected-request tombstone table (rejected_contact_requests, G5 stage-1 rotation defense) as a behavior, so the writer block must either be re-plumbed against the new ignored/unignored model or removed (along with the V001 migration), depending on the intended persistence semantics for local-only ignore.
suggestion: [Carried forward — STILL VALID] Pending incoming requests fetch attacker-controlled avatar URLs before user consent
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift (line 119)
Verified at head. sync_contact_profiles populates the profile cache for every key in managed.incoming_contact_requests (unaccepted senders — packages/rs-platform-wallet/src/wallet/identity/network/profile.rs:640-650), and IncomingRequestRow feeds cachedProfile(row.contactIdentityId)?.avatarUrl into DashPayAvatarView (SwiftUI AsyncImage). Any party who publishes a dashpay.profile and sends an unsolicited request causes the victim wallet to issue an outbound HTTPS GET to the attacker-controlled avatarUrl before the user has accepted them — leaking IP, online status, user-agent, and TLS fingerprint. The https:// validation added on the Rust side blocks local-scheme SSRF but does not address pre-consent fetching. Gate avatar URL fetches on established contacts only, or strip avatarUrl from pending-request rows.
suggestion: [Carried forward — STILL VALID] Confirmed-absent contact profiles never delete their SwiftData row; stale profile resurrects on cold start
packages/rs-platform-wallet-ffi/src/identity_persistence.rs (line 587)
Verified at head. allocate_contact_profile_rows continues past entries with profile == None (lines 590-592 'Skip confirmed-absent entries — the negative cache is not persisted; it rebuilds on the next sweep'). Swift's upsertDashpayContactProfiles is append/refresh-only, and apply_contact_profile_rows rehydrates restored rows as present profiles. Once a contact's profile is later deleted upstream or confirmed absent, the stale SwiftData row survives — the previously-published display name / avatar URL keeps rendering on cold start. Either persist the absent tombstone (with checked_at_ms) or emit an explicit delete row through the changeset so the SwiftData writer can DELETE.
suggestion: [Carried forward — STILL VALID] established_contact_get_note leaves *out_note undefined on early-return paths
packages/rs-platform-wallet-ffi/src/established_contact.rs (line 156)
Verified at head. Sibling established_contact_get_alias writes *out_alias = std::ptr::null_mut(); immediately after check_ptr! (line 108), so every early-return path leaves the out pointer at defined null. established_contact_get_note only calls check_ptr!(out_note) (line 160) and skips the pre-null write. Early returns from unwrap_option_or_return! (storage NotFound / contact.note == None) or unwrap_result_or_return! (CString::new failure on interior NUL) leave *out_note indeterminate. A C/Swift caller that frees or reads the slot on a non-ok return can crash or double-free. One-line fix mirrors the sibling.
#[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);
*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: [Carried forward — STILL VALID] Contact-profile `checked_at_ms` not durably persisted when profile is unchanged
packages/rs-platform-wallet/src/wallet/identity/network/profile.rs (line 588)
Verified at head. apply_fetched_profile always overwrites checked_at_ms = now_ms but returns changed only when the stored profile value differs. sync_contact_profiles calls persister.store only when any_changed is true. So a successful re-check confirming the same present or absent profile bumps the timestamp only in RAM — restart immediately re-fetches every contact (cold-start write amplification + visible UI flicker, the exact problem the negative cache exists to avoid). A failed persister.store also drops in-memory state: the next sweep skips re-emit. Persist the timestamp update unconditionally on successful re-check, or emit a 'touch' changeset when !any_changed && profile_present.
suggestion: [Carried forward — STILL VALID] Paginated retrieve-all has no per-call total or elapsed budget
packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs (line 42)
Verified at head. fetch_contact_requests_paginated drains every matching document at CONTACT_REQUEST_PAGE_SIZE = 100 per page with no per-call total cap, no elapsed-time budget, and no caller-visible continuation token. The cold-start / restore path (after_created_at == None) walks the entire history in one call. Combined with the recurring DashPaySyncManager, an identity with a long history monopolizes the sweep on each cold start; a flood of incoming requests can keep the sweep paginating across many proof-verified gRPC round trips with no opportunity to interleave the rest of the wallet's sync. Add a per-call document/time budget and a resumable cursor.
suggestion: [Carried forward — STILL VALID] Profile-reader trio leaves *out_profile / *out_has_profile undefined on early-return paths
packages/rs-platform-wallet-ffi/src/dashpay_profile.rs (line 90)
Verified at head. managed_identity_get_dashpay_profile (line 90), platform_wallet_get_dashpay_profile (line 116), and platform_wallet_get_contact_profile (line 153) only assign *out_profile / *out_has_profile in the present / no-profile match arms. After check_ptr!, early-return paths from unwrap_result_or_return!(read_identifier(..)) and unwrap_option_or_return! (storage NotFound / no managed identity / no entry) all leave the out pointers untouched. DashPayProfileFFI owns several *mut c_char; a subsequent dashpay_profile_ffi_free on a slot the caller left uninitialized frees garbage pointers. Pre-null + pre-false writes immediately after check_ptr! (matching established_contact_get_alias) close the gap.
nitpick: [Carried forward — STILL VALID] Restore/persist FFI structs (Payment, ContactProfile, ContactProfileRow) lack compile-time layout guards
packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs (line 352)
Verified at head. Sibling #[repr(C)] types crossing the same Swift↔Rust restore/persist boundary carry static-assert size/align guards: AccountAddressPoolFFI, ContactRequestFFI, ContactRequestRemovalFFI, ContactIgnoredSenderFFI, IdentityKeyEntryFFI, IdentityEntryFFI. PaymentRestoreEntryFFI, ContactProfileRestoreEntryFFI, and ContactProfileRowFFI — three new boundary types introduced by this PR — do not. A silent field reorder or padding-driven size change would compile cleanly and corrupt restore. Adding const _: [u8; N] = [0u8; std::mem::size_of::<...>()]; guards matches precedent and is essentially free.
nitpick: [New — latest delta] payments_for_contact doc claims 'newest first' but iteration is lexicographic by hex txid
packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs (line 177)
New in commit 1ea4d52. The docstring states 'newest first' (line 178), but the implementation iterates self.dashpay_payments — a BTreeMap<String, PaymentEntry> keyed by hex tx_id. PaymentEntry (packages/rs-platform-wallet/src/wallet/identity/types/dashpay/payment.rs:69-82) has no timestamp field. So the returned Vec is sorted by hex-string txid (lexicographic), not by time. The UI consumer (per-contact payment history) will display payments in essentially random order, not chronological. Either drop the 'newest first' claim from the doc, sort by an actual timestamp source (e.g. add created_at_ms to PaymentEntry or look it up from the SPV transaction store), or document that the caller is responsible for ordering. The accompanying test (payments_for_contact_filters_by_counterparty) does not assert ordering, so this would not be caught by the existing test.
_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3841 1ea4d52 --dry-run --prior-sha 023479d
STDOUT:
STDERR:
Traceback (most recent call last):
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", li), so I posted the same verified findings as a top-level review body._
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at 3d765b8. Latest delta (1ea4d52..HEAD) is comment-only spec-gate stripping with no behavioral changes — no new defects introduced. All 9 prior findings reconciled and STILL VALID against current HEAD, verified by direct file inspection: contacts.rs:233/246 still references removed ContactChangeSet::rejected (confirmed blocking compile break under default features), and the note/profile FFI getters still skip the pre-null pattern their alias siblings use. Two prior findings (entropy on DashSDKContactRequestResult and contactInfo pagination exhaustion) were already resolved in earlier commits on this branch and stay resolved.
🔴 1 blocking | 🟡 6 suggestion(s) | 💬 2 nitpick(s)
9 additional finding(s)
blocking: SQLite contacts writer references removed ContactChangeSet::rejected — crate fails to compile under default features
packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs (line 233)
Verified at HEAD: contacts.rs:233 does if !cs.rejected.is_empty() and contacts.rs:246 does for entry in cs.rejected.values(), but ContactChangeSet (changeset.rs:619-646) no longer has a rejected field — Spec 2 replaced it with sender-scoped ignored / unignored: BTreeSet<(Identifier, Identifier)>. cargo check -p platform-wallet-storage emits two E0609 (no field 'rejected') plus a follow-on E0282 at line 252 where entry.document_id can no longer be type-inferred. The sqlite feature is in this crate's default feature set, so the default workspace build is broken. Beyond the build break, even with a mechanical fix the SQLite backend would silently drop the new local-only ignored/unignored deltas across restart, defeating Spec 2's persistence guarantee on this storage path. Re-plumb the writer to insert on cs.ignored and delete on cs.unignored, repurposing or replacing the rejected_contact_requests table and load path accordingly.
suggestion: Pending incoming requests fetch attacker-controlled avatar URLs before user consent
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift (line 119)
sync_contact_profiles (profile.rs:640-650) explicitly includes managed.incoming_contact_requests.keys() in the per-owner fetch plan, populating the contact-profile cache for senders the victim has never accepted. IncomingRequestRow feeds cachedProfile(row.contactIdentityId)?.avatarUrl into DashPayAvatarView → SwiftUI AsyncImage, so rendering the Requests tab issues an HTTPS GET to the attacker-controlled host the moment the view appears — leaking IP, online state, and TLS/UA fingerprint before any consent action. With this PR's PersistentDashpayContactProfile persistence the side channel survives restarts. Rust-side is_valid_avatar_url only checks scheme + length; the new Spec 2 Ignore only mutes after first render. Either suppress remote avatars Swift-side for unaccepted senders (initials only) or strip avatarUrl from the Rust→Swift projection for non-established contacts.
suggestion: Confirmed-absent contact profiles never delete their SwiftData row; stale profile resurrects on cold start
packages/rs-platform-wallet-ffi/src/identity_persistence.rs (line 587)
allocate_contact_profile_rows explicitly continues past entries with profile == None (lines 588-592, "Skip confirmed-absent entries — the negative cache is not persisted; it rebuilds on the next sweep"). Swift's upsertDashpayContactProfiles is append/refresh-only, and the restore path rehydrates every row as ContactProfileEntry { profile: Some(..), checked_at_ms }. When a contact removes their on-chain dashpay.profile, apply_fetched_profile flips the in-memory entry to None, but the absence is never projected across the FFI — the stale PersistentDashpayContactProfile row survives. On cold start the cache rehydrates as Some(stale_profile) and the UI re-issues GETs to the previously attacker-controlled avatarUrl. This compounds the pre-consent avatar fetch above into a tracking channel that outlasts on-chain profile removal. Project absences as an explicit delete intent across the boundary (e.g. removed_contact_profile_ids: *const [u8;32], mirroring the ContactIgnoredSenderFFI removal-set shape already used in this PR).
suggestion: established_contact_get_note leaves *out_note undefined on early-return paths
packages/rs-platform-wallet-ffi/src/established_contact.rs (line 156)
Verified at HEAD: sibling established_contact_get_alias writes *out_alias = std::ptr::null_mut(); at line 108 immediately after check_ptr!, so every early return leaves the out pointer at defined null. established_contact_get_note (lines 156-169) only calls check_ptr!(out_note) and skips the pre-null write. Early returns from unwrap_option_or_return! (storage NotFound or contact.note == None) and unwrap_result_or_return! (CString interior NUL) leave *out_note indeterminate across the C ABI. A Swift/C caller that doesn't pre-zero the slot can read or pass a stale pointer to platform_wallet_string_free, risking invalid-free or double-free. The exported C ABI shouldn't depend on caller hygiene — mirror the sibling pattern.
#[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);
*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: Profile-reader trio leaves *out_profile / *out_has_profile undefined on early-return paths
packages/rs-platform-wallet-ffi/src/dashpay_profile.rs (line 90)
managed_identity_get_dashpay_profile (line 90), platform_wallet_get_dashpay_profile (line 116), and platform_wallet_get_contact_profile (line 153) only assign *out_profile / *out_has_profile inside the present/no-profile match arms. After check_ptr!, early returns from unwrap_result_or_return!(read_identifier(..)) (invalid 32-byte input) and unwrap_option_or_return! (handle NotFound, no managed identity, no entry) leave both out pointers indeterminate. DashPayProfileFFI owns several *mut c_char fields; a subsequent dashpay_profile_ffi_free on a slot the caller didn't pre-zero would free garbage pointers. Pre-write *out_profile = DashPayProfileFFI::empty() and *out_has_profile = false immediately after check_ptr!, matching the established_contact_get_alias pattern.
suggestion: Contact-profile checked_at_ms not durably persisted when profile is unchanged
packages/rs-platform-wallet/src/wallet/identity/network/profile.rs (line 588)
apply_fetched_profile always refreshes checked_at_ms = now_ms (line 599) but returns changed only when the stored profile value differs; sync_contact_profiles calls persister.store only when any_changed is true (lines 717-725). A re-check confirming the same present (or confirmed-absent) profile updates the timestamp in memory only — the FFI persistence callback never fires. After cold start, restored rows carry the stale checked_at_ms, should_fetch_profile immediately re-issues an In query for every unchanged contact, and the negative-cache backoff effectively doesn't survive restart — amplifying the avatar-URL tracking side channel and the cold-start fetch storm. A failed earlier store is also not retried because later sweeps see changed = false. Return a separate needs_persist signal from apply_fetched_profile for timestamp-only refreshes (with a coarse coalescing window if write-amp matters).
suggestion: Paginated retrieve-all has no per-call total or elapsed budget; aggravated by Spec 2 un-ignore cursor rewind
packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs (line 42)
fetch_contact_requests_paginated drains every matching document at CONTACT_REQUEST_PAGE_SIZE = 100 per page with no per-call total cap, elapsed-time budget, or caller-visible continuation. On cold start, restore-from-seed, or any after_created_at == None path it walks the entire history in one call. Spec 2 sharpens the trigger: unignore_sender resets high_water_received_ms to None, so any UI un-ignore drops the next sweep onto the full retrieve-all. Combined with the recurring DashPaySyncManager tick, one oversized identity (organic growth, attacker-spammed contactRequest documents varying accountReference, or induced repeated un-ignores) can monopolize the sync loop across the SDK gRPC boundary and starve every other identity — a cheap algorithmic-DoS surface. Add a configurable total-document ceiling per sweep with a resumable cursor, or at minimum a logged warn + early break above a threshold.
nitpick: Restore/persist FFI structs (Payment, ContactProfile, ContactProfileRow) lack compile-time layout guards
packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs (line 352)
Sibling #[repr(C)] types crossing the same Swift↔Rust restore/persist boundary carry static-assert size/align guards (AccountAddressPoolFFI, ContactRequestFFI, ContactRequestRemovalFFI, ContactIgnoredSenderFFI, IdentityKeyEntryFFI, IdentityEntryFFI). Three boundary types introduced by this PR still don't: PaymentRestoreEntryFFI (wallet_restore_types.rs:352-367), ContactProfileRestoreEntryFFI (wallet_restore_types.rs:383-410), and the persist-side ContactProfileRowFFI (identity_persistence.rs ~181-211). The parent IdentityEntryFFI guard pins the pointer field but not the inner row layout Swift indexes through. A field reorder, type widen, or padding change on either side would compile silently and shift offsets relative to the Swift mirror, potentially restoring profiles under the wrong contact id. Add const _: [u8; N] = [0u8; size_of::<T>()] + align_of tripwires to all three structs, matching precedent.
nitpick: payments_for_contact doc claims 'newest first' but iteration is lexicographic by hex txid
packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs (line 177)
Verified at HEAD: the rustdoc (line 177-180) states the result is ordered 'newest first', but the implementation iterates self.dashpay_payments — a BTreeMap<String, PaymentEntry> keyed by hex tx_id. PaymentEntry carries no timestamp field, so the returned Vec is sorted by hex-string txid (lexicographic), not by time. The per-contact payment-history UI consumer renders payments in essentially random (txid-hash) order rather than chronological. The accompanying test payments_for_contact_filters_by_counterparty doesn't assert ordering, so this wouldn't be caught. Either drop the 'newest first' claim from the docstring, add an explicit timestamp field (e.g. created_at_ms) and sort by it, or have the caller stitch ordering from the SPV transaction store.
🤖 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-storage/src/sqlite/schema/contacts.rs`:233-255: SQLite contacts writer references removed ContactChangeSet::rejected — crate fails to compile under default features
Verified at HEAD: contacts.rs:233 does `if !cs.rejected.is_empty()` and contacts.rs:246 does `for entry in cs.rejected.values()`, but `ContactChangeSet` (changeset.rs:619-646) no longer has a `rejected` field — Spec 2 replaced it with sender-scoped `ignored` / `unignored: BTreeSet<(Identifier, Identifier)>`. `cargo check -p platform-wallet-storage` emits two E0609 (`no field 'rejected'`) plus a follow-on E0282 at line 252 where `entry.document_id` can no longer be type-inferred. The `sqlite` feature is in this crate's default feature set, so the default workspace build is broken. Beyond the build break, even with a mechanical fix the SQLite backend would silently drop the new local-only ignored/unignored deltas across restart, defeating Spec 2's persistence guarantee on this storage path. Re-plumb the writer to insert on `cs.ignored` and delete on `cs.unignored`, repurposing or replacing the `rejected_contact_requests` table and load path accordingly.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift`:119-129: Pending incoming requests fetch attacker-controlled avatar URLs before user consent
`sync_contact_profiles` (profile.rs:640-650) explicitly includes `managed.incoming_contact_requests.keys()` in the per-owner fetch plan, populating the contact-profile cache for senders the victim has never accepted. `IncomingRequestRow` feeds `cachedProfile(row.contactIdentityId)?.avatarUrl` into `DashPayAvatarView` → SwiftUI `AsyncImage`, so rendering the Requests tab issues an HTTPS GET to the attacker-controlled host the moment the view appears — leaking IP, online state, and TLS/UA fingerprint before any consent action. With this PR's `PersistentDashpayContactProfile` persistence the side channel survives restarts. Rust-side `is_valid_avatar_url` only checks scheme + length; the new Spec 2 Ignore only mutes after first render. Either suppress remote avatars Swift-side for unaccepted senders (initials only) or strip `avatarUrl` from the Rust→Swift projection for non-established contacts.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/identity_persistence.rs`:587-621: Confirmed-absent contact profiles never delete their SwiftData row; stale profile resurrects on cold start
`allocate_contact_profile_rows` explicitly `continue`s past entries with `profile == None` (lines 588-592, "Skip confirmed-absent entries — the negative cache is not persisted; it rebuilds on the next sweep"). Swift's `upsertDashpayContactProfiles` is append/refresh-only, and the restore path rehydrates every row as `ContactProfileEntry { profile: Some(..), checked_at_ms }`. When a contact removes their on-chain `dashpay.profile`, `apply_fetched_profile` flips the in-memory entry to `None`, but the absence is never projected across the FFI — the stale `PersistentDashpayContactProfile` row survives. On cold start the cache rehydrates as `Some(stale_profile)` and the UI re-issues GETs to the previously attacker-controlled `avatarUrl`. This compounds the pre-consent avatar fetch above into a tracking channel that outlasts on-chain profile removal. Project absences as an explicit delete intent across the boundary (e.g. `removed_contact_profile_ids: *const [u8;32]`, mirroring the `ContactIgnoredSenderFFI` removal-set shape already used in this PR).
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/established_contact.rs`:156-169: established_contact_get_note leaves *out_note undefined on early-return paths
Verified at HEAD: sibling `established_contact_get_alias` writes `*out_alias = std::ptr::null_mut();` at line 108 immediately after `check_ptr!`, so every early return leaves the out pointer at defined null. `established_contact_get_note` (lines 156-169) only calls `check_ptr!(out_note)` and skips the pre-null write. Early returns from `unwrap_option_or_return!` (storage NotFound or `contact.note == None`) and `unwrap_result_or_return!` (CString interior NUL) leave `*out_note` indeterminate across the C ABI. A Swift/C caller that doesn't pre-zero the slot can read or pass a stale pointer to `platform_wallet_string_free`, risking invalid-free or double-free. The exported C ABI shouldn't depend on caller hygiene — mirror the sibling pattern.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:90-185: Profile-reader trio leaves *out_profile / *out_has_profile undefined on early-return paths
`managed_identity_get_dashpay_profile` (line 90), `platform_wallet_get_dashpay_profile` (line 116), and `platform_wallet_get_contact_profile` (line 153) only assign `*out_profile` / `*out_has_profile` inside the present/no-profile match arms. After `check_ptr!`, early returns from `unwrap_result_or_return!(read_identifier(..))` (invalid 32-byte input) and `unwrap_option_or_return!` (handle NotFound, no managed identity, no entry) leave both out pointers indeterminate. `DashPayProfileFFI` owns several `*mut c_char` fields; a subsequent `dashpay_profile_ffi_free` on a slot the caller didn't pre-zero would free garbage pointers. Pre-write `*out_profile = DashPayProfileFFI::empty()` and `*out_has_profile = false` immediately after `check_ptr!`, matching the `established_contact_get_alias` pattern.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/identity/network/profile.rs`:588-725: Contact-profile checked_at_ms not durably persisted when profile is unchanged
`apply_fetched_profile` always refreshes `checked_at_ms = now_ms` (line 599) but returns `changed` only when the stored profile value differs; `sync_contact_profiles` calls `persister.store` only when `any_changed` is true (lines 717-725). A re-check confirming the same present (or confirmed-absent) profile updates the timestamp in memory only — the FFI persistence callback never fires. After cold start, restored rows carry the stale `checked_at_ms`, `should_fetch_profile` immediately re-issues an `In` query for every unchanged contact, and the negative-cache backoff effectively doesn't survive restart — amplifying the avatar-URL tracking side channel and the cold-start fetch storm. A failed earlier store is also not retried because later sweeps see `changed = false`. Return a separate `needs_persist` signal from `apply_fetched_profile` for timestamp-only refreshes (with a coarse coalescing window if write-amp matters).
- [SUGGESTION] In `packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs`:42-112: Paginated retrieve-all has no per-call total or elapsed budget; aggravated by Spec 2 un-ignore cursor rewind
`fetch_contact_requests_paginated` drains every matching document at `CONTACT_REQUEST_PAGE_SIZE = 100` per page with no per-call total cap, elapsed-time budget, or caller-visible continuation. On cold start, restore-from-seed, or any `after_created_at == None` path it walks the entire history in one call. Spec 2 sharpens the trigger: `unignore_sender` resets `high_water_received_ms` to `None`, so any UI un-ignore drops the next sweep onto the full retrieve-all. Combined with the recurring `DashPaySyncManager` tick, one oversized identity (organic growth, attacker-spammed `contactRequest` documents varying `accountReference`, or induced repeated un-ignores) can monopolize the sync loop across the SDK gRPC boundary and starve every other identity — a cheap algorithmic-DoS surface. Add a configurable total-document ceiling per sweep with a resumable cursor, or at minimum a logged warn + early break above a threshold.
- [NITPICK] In `packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs`:352-411: Restore/persist FFI structs (Payment, ContactProfile, ContactProfileRow) lack compile-time layout guards
Sibling `#[repr(C)]` types crossing the same Swift↔Rust restore/persist boundary carry static-assert size/align guards (`AccountAddressPoolFFI`, `ContactRequestFFI`, `ContactRequestRemovalFFI`, `ContactIgnoredSenderFFI`, `IdentityKeyEntryFFI`, `IdentityEntryFFI`). Three boundary types introduced by this PR still don't: `PaymentRestoreEntryFFI` (wallet_restore_types.rs:352-367), `ContactProfileRestoreEntryFFI` (wallet_restore_types.rs:383-410), and the persist-side `ContactProfileRowFFI` (identity_persistence.rs ~181-211). The parent `IdentityEntryFFI` guard pins the pointer field but not the inner row layout Swift indexes through. A field reorder, type widen, or padding change on either side would compile silently and shift offsets relative to the Swift mirror, potentially restoring profiles under the wrong contact id. Add `const _: [u8; N] = [0u8; size_of::<T>()]` + `align_of` tripwires to all three structs, matching precedent.
- [NITPICK] In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs`:177-190: payments_for_contact doc claims 'newest first' but iteration is lexicographic by hex txid
Verified at HEAD: the rustdoc (line 177-180) states the result is ordered 'newest first', but the implementation iterates `self.dashpay_payments` — a `BTreeMap<String, PaymentEntry>` keyed by hex `tx_id`. `PaymentEntry` carries no timestamp field, so the returned `Vec` is sorted by hex-string txid (lexicographic), not by time. The per-contact payment-history UI consumer renders payments in essentially random (txid-hash) order rather than chronological. The accompanying test `payments_for_contact_filters_by_counterparty` doesn't assert ordering, so this wouldn't be caught. Either drop the 'newest first' claim from the docstring, add an explicit timestamp field (e.g. `created_at_ms`) and sort by it, or have the caller stitch ordering from the SPV transaction store.
Inline dry-run could not load the GitHub PR diff because this PR exceeds GitHub's 20,000-line diff limit, so I posted the same verified findings as a top-level review body.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at 182a398. The latest delta (3d765b8→HEAD) is docs-only (TODO.md links upstream rust-dashcore#813), introducing no new defects. All 9 prior findings independently re-verified against the worktree and remain valid — headlined by a blocking compile break in platform-wallet-storage (default features) where the SQLite contacts writer still references the removed ContactChangeSet::rejected field. Both Claude and Codex converge on the same 9 findings.
🔴 1 blocking | 🟡 6 suggestion(s) | 💬 2 nitpick(s)
9 additional finding(s)
blocking: SQLite contacts writer references removed ContactChangeSet::rejected — default build broken
packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs (line 233)
Verified at HEAD: lines 233 and 246 still read cs.rejected.is_empty() / cs.rejected.values() and line 252 still accesses entry.document_id. The Spec 2 refactor replaced ContactChangeSet::rejected with sender-scoped ignored / unignored: BTreeSet<(Identifier, Identifier)> (changeset.rs:619-646), and cargo check -p platform-wallet-storage fails with two E0609 (no field 'rejected' on type '&ContactChangeSet') plus a follow-on E0282 type-inference error. The sqlite feature is in the crate's default feature set, so the default workspace build is broken.
Beyond the compile break: even when mechanically patched, the SQLite persister has no insert/delete plumbing for cs.ignored / cs.unignored. The Spec 2 Ignore action is the wallet's local-only block list and the primary defense against the pre-consent avatar/sender-spam vectors. On the SQLite backend, ignore/un-ignore intents would silently fail to persist across restart — ignored senders' requests resurface on the next sweep, un-ignores are lost, and rotated accountReference values from once-rejected senders flow back into the UI. Re-plumb the writer (and load path / schema migration) for ignored / unignored with delete-on-unignore semantics, or remove the dead rejected block plus its V001 migration if the design has fully moved away from on-disk tombstones.
suggestion: Pending incoming requests fetch attacker-controlled avatar URLs before user consent
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift (line 119)
sync_contact_profiles (profile.rs:640-650) explicitly includes managed.incoming_contact_requests.keys() in the per-owner fetch plan, populating the contact-profile cache for senders the victim has never accepted. IncomingRequestRow feeds cachedProfile(row.contactIdentityId)?.avatarUrl into DashPayAvatarView → SwiftUI AsyncImage, so rendering the Requests tab issues an outbound HTTPS GET to the sender-controlled host before any consent action — leaking IP, online state, and TLS/UA fingerprint, and providing per-victim URL-token receipt confirmation. With this PR's PersistentDashpayContactProfile SwiftData persistence, the side channel survives restart. Rust-side is_valid_avatar_url only checks scheme/length; the Spec 2 local Ignore only mutes after first render. Either suppress remote avatars Swift-side for unaccepted senders (initials-only variant for the Requests tab) or strip avatarUrl from the Rust→Swift projection until the sender becomes an established contact.
suggestion: Confirmed-absent contact profiles never delete their SwiftData row; stale profile resurrects on cold start
packages/rs-platform-wallet-ffi/src/identity_persistence.rs (line 587)
Verified at HEAD: allocate_contact_profile_rows explicitly continues past entries with profile == None (lines 590-592, "Skip confirmed-absent entries — the negative cache is not persisted; it rebuilds on the next sweep"). The FFI projection carries no removal intent across the boundary, Swift's upsertDashpayContactProfiles is append/refresh-only, and the restore path rehydrates every persisted row as ContactProfileEntry { profile: Some(..), .. }. When a contact removes their on-chain dashpay.profile, apply_fetched_profile flips the in-memory entry to None, but the absence is dropped at the FFI projection — the stale PersistentDashpayContactProfile row survives on disk. On cold start the cache rehydrates as Some(stale_profile) and the UI re-issues GETs to the previously attacker-controlled avatarUrl. Compounds the pre-consent avatar fetch into a tracking channel that outlasts on-chain profile removal. Project absences as an explicit delete intent across the boundary (e.g. removed_contact_profile_ids: *const [u8;32]), mirroring the ContactIgnoredSenderFFI removal-set shape this PR already introduces.
suggestion: established_contact_get_note leaves *out_note undefined on early-return paths
packages/rs-platform-wallet-ffi/src/established_contact.rs (line 156)
Verified at HEAD: sibling established_contact_get_alias writes *out_alias = std::ptr::null_mut(); at line 108 immediately after check_ptr!, but established_contact_get_note only calls check_ptr!(out_note) (line 160) and skips the pre-null write. Early returns from unwrap_option_or_return! (storage NotFound / contact.note == None) and unwrap_result_or_return! (CString::new failure on interior NUL — reachable from on-chain note content) leave *out_note indeterminate across the C ABI. A Swift/C caller that doesn't pre-zero the slot can read a stale pointer or pass it to platform_wallet_string_free, risking invalid-free or double-free. Mirror the sibling pattern.
#[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);
*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: Profile-reader trio leaves *out_profile / *out_has_profile undefined on early-return paths
packages/rs-platform-wallet-ffi/src/dashpay_profile.rs (line 90)
Verified at HEAD: managed_identity_get_dashpay_profile (line 90), platform_wallet_get_dashpay_profile (line 116), and platform_wallet_get_contact_profile (line 153) only assign *out_profile / *out_has_profile inside the present / no-profile match arms. After check_ptr!, early returns from unwrap_result_or_return!(read_identifier(..)) (invalid 32-byte input) and unwrap_option_or_return! (handle NotFound, no managed identity, no entry) leave both out pointers indeterminate. DashPayProfileFFI owns three *mut c_char fields; a subsequent dashpay_profile_ffi_free on a slot the caller didn't pre-zero would free garbage pointers. Pre-write *out_profile = DashPayProfileFFI::empty() and *out_has_profile = false immediately after check_ptr! in each function, matching the established_contact_get_alias precedent.
suggestion: Contact-profile checked_at_ms not durably persisted when profile is unchanged
packages/rs-platform-wallet/src/wallet/identity/network/profile.rs (line 588)
apply_fetched_profile always refreshes checked_at_ms = now_ms but returns changed only when the stored profile value differs (equality compares only profile); sync_contact_profiles calls persister.store only when any_changed is true. A re-check confirming the same present or confirmed-absent profile updates the timestamp in memory only — the FFI persistence callback never fires. After cold start, restored rows carry the stale checked_at_ms, should_fetch_profile immediately re-issues an In query for every unchanged contact, and the negative-cache backoff effectively doesn't survive restart. This amplifies both the pre-consent avatar tracking side channel (restart re-issues per-victim GETs to known-stable hosts) and the cold-start fetch storm. A failed earlier store is also not retried because later sweeps see changed = false. Return a separate needs_persist signal from apply_fetched_profile for timestamp-only refreshes, with coarse coalescing if write-amp matters.
suggestion: Paginated retrieve-all has no per-call total or elapsed budget — aggravated by Spec 2 un-ignore cursor rewind
packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs (line 42)
fetch_contact_requests_paginated drains every matching document at CONTACT_REQUEST_PAGE_SIZE = 100 per page with no per-call total cap, no elapsed-time budget, and no caller-visible continuation. On cold start, restore-from-seed, or any after_created_at == None path it walks the entire history in one call. Spec 2 sharpens the trigger: unignore_sender resets high_water_received_ms to None, so any UI un-ignore drops the next sweep onto the full retrieve-all. Combined with the recurring DashPaySyncManager tick, an attacker who spams contactRequest documents (cheap on testnet, ongoing-cost on mainnet) against one of the wallet's identities can make every recurring sweep monopolize the SDK gRPC client and starve every other identity — a cheap algorithmic-DoS surface against the platform proof-verification path. Add a configurable total-document ceiling per sweep with a resumable cursor, or at minimum a logged warn + early break above a threshold.
nitpick: Restore/persist FFI structs (Payment, ContactProfile, ContactProfileRow) lack compile-time layout guards
packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs (line 352)
Sibling #[repr(C)] types crossing the same Swift↔Rust restore/persist boundary carry static-assert size/align guards (AccountAddressPoolFFI, ContactRequestFFI, ContactRequestRemovalFFI, ContactIgnoredSenderFFI, IdentityKeyEntryFFI, IdentityEntryFFI). Three boundary types introduced by this PR don't: PaymentRestoreEntryFFI (lines 352-367), ContactProfileRestoreEntryFFI (lines 383-410), and the persist-side ContactProfileRowFFI (identity_persistence.rs:180-211). The parent IdentityEntryFFI guard pins the pointer/count field but not the inner row layout Swift indexes through. A field reorder, type widen, or padding change on either side would compile silently and shift offsets relative to the Swift mirror, potentially mis-attributing restored profiles or payments to the wrong contact id / wrong owner. Add const _: [u8; N] = [0u8; std::mem::size_of::<T>()]; + align_of tripwires to all three structs, matching precedent.
nitpick: payments_for_contact doc claims 'newest first' but iteration is lexicographic by hex txid
packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs (line 177)
Verified at HEAD: the rustdoc at line 177 states the result is ordered 'newest first', but the implementation iterates self.dashpay_payments — a BTreeMap<String, PaymentEntry> keyed by hex tx_id. PaymentEntry carries no timestamp field, so the returned Vec is sorted lexicographically by hex-string txid, not by time. The per-contact payment-history UI consumer therefore renders payments in essentially random (txid-hash) order. A sender who can grind their outgoing txid can also influence display position, providing a small social-engineering surface at the UI layer. The accompanying test payments_for_contact_filters_by_counterparty does not assert ordering. Either drop the 'newest first' claim from the docstring, add an explicit timestamp field (e.g. created_at_ms) and sort by it, or have the caller stitch ordering from the SPV transaction store.
🤖 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-storage/src/sqlite/schema/contacts.rs`:233-255: SQLite contacts writer references removed ContactChangeSet::rejected — default build broken
Verified at HEAD: lines 233 and 246 still read `cs.rejected.is_empty()` / `cs.rejected.values()` and line 252 still accesses `entry.document_id`. The Spec 2 refactor replaced `ContactChangeSet::rejected` with sender-scoped `ignored` / `unignored: BTreeSet<(Identifier, Identifier)>` (changeset.rs:619-646), and `cargo check -p platform-wallet-storage` fails with two E0609 (`no field 'rejected' on type '&ContactChangeSet'`) plus a follow-on E0282 type-inference error. The `sqlite` feature is in the crate's default feature set, so the default workspace build is broken.
Beyond the compile break: even when mechanically patched, the SQLite persister has no insert/delete plumbing for `cs.ignored` / `cs.unignored`. The Spec 2 Ignore action is the wallet's local-only block list and the primary defense against the pre-consent avatar/sender-spam vectors. On the SQLite backend, ignore/un-ignore intents would silently fail to persist across restart — ignored senders' requests resurface on the next sweep, un-ignores are lost, and rotated `accountReference` values from once-rejected senders flow back into the UI. Re-plumb the writer (and load path / schema migration) for `ignored` / `unignored` with delete-on-unignore semantics, or remove the dead `rejected` block plus its V001 migration if the design has fully moved away from on-disk tombstones.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift`:119-129: Pending incoming requests fetch attacker-controlled avatar URLs before user consent
`sync_contact_profiles` (profile.rs:640-650) explicitly includes `managed.incoming_contact_requests.keys()` in the per-owner fetch plan, populating the contact-profile cache for senders the victim has never accepted. `IncomingRequestRow` feeds `cachedProfile(row.contactIdentityId)?.avatarUrl` into `DashPayAvatarView` → SwiftUI `AsyncImage`, so rendering the Requests tab issues an outbound HTTPS GET to the sender-controlled host before any consent action — leaking IP, online state, and TLS/UA fingerprint, and providing per-victim URL-token receipt confirmation. With this PR's `PersistentDashpayContactProfile` SwiftData persistence, the side channel survives restart. Rust-side `is_valid_avatar_url` only checks scheme/length; the Spec 2 local Ignore only mutes after first render. Either suppress remote avatars Swift-side for unaccepted senders (initials-only variant for the Requests tab) or strip `avatarUrl` from the Rust→Swift projection until the sender becomes an established contact.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/identity_persistence.rs`:587-621: Confirmed-absent contact profiles never delete their SwiftData row; stale profile resurrects on cold start
Verified at HEAD: `allocate_contact_profile_rows` explicitly `continue`s past entries with `profile == None` (lines 590-592, "Skip confirmed-absent entries — the negative cache is not persisted; it rebuilds on the next sweep"). The FFI projection carries no removal intent across the boundary, Swift's `upsertDashpayContactProfiles` is append/refresh-only, and the restore path rehydrates every persisted row as `ContactProfileEntry { profile: Some(..), .. }`. When a contact removes their on-chain `dashpay.profile`, `apply_fetched_profile` flips the in-memory entry to `None`, but the absence is dropped at the FFI projection — the stale `PersistentDashpayContactProfile` row survives on disk. On cold start the cache rehydrates as `Some(stale_profile)` and the UI re-issues GETs to the previously attacker-controlled `avatarUrl`. Compounds the pre-consent avatar fetch into a tracking channel that outlasts on-chain profile removal. Project absences as an explicit delete intent across the boundary (e.g. `removed_contact_profile_ids: *const [u8;32]`), mirroring the `ContactIgnoredSenderFFI` removal-set shape this PR already introduces.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/established_contact.rs`:156-169: established_contact_get_note leaves *out_note undefined on early-return paths
Verified at HEAD: sibling `established_contact_get_alias` writes `*out_alias = std::ptr::null_mut();` at line 108 immediately after `check_ptr!`, but `established_contact_get_note` only calls `check_ptr!(out_note)` (line 160) and skips the pre-null write. Early returns from `unwrap_option_or_return!` (storage NotFound / `contact.note == None`) and `unwrap_result_or_return!` (`CString::new` failure on interior NUL — reachable from on-chain note content) leave `*out_note` indeterminate across the C ABI. A Swift/C caller that doesn't pre-zero the slot can read a stale pointer or pass it to `platform_wallet_string_free`, risking invalid-free or double-free. Mirror the sibling pattern.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:90-185: Profile-reader trio leaves *out_profile / *out_has_profile undefined on early-return paths
Verified at HEAD: `managed_identity_get_dashpay_profile` (line 90), `platform_wallet_get_dashpay_profile` (line 116), and `platform_wallet_get_contact_profile` (line 153) only assign `*out_profile` / `*out_has_profile` inside the present / no-profile match arms. After `check_ptr!`, early returns from `unwrap_result_or_return!(read_identifier(..))` (invalid 32-byte input) and `unwrap_option_or_return!` (handle NotFound, no managed identity, no entry) leave both out pointers indeterminate. `DashPayProfileFFI` owns three `*mut c_char` fields; a subsequent `dashpay_profile_ffi_free` on a slot the caller didn't pre-zero would free garbage pointers. Pre-write `*out_profile = DashPayProfileFFI::empty()` and `*out_has_profile = false` immediately after `check_ptr!` in each function, matching the `established_contact_get_alias` precedent.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/identity/network/profile.rs`:588-725: Contact-profile checked_at_ms not durably persisted when profile is unchanged
`apply_fetched_profile` always refreshes `checked_at_ms = now_ms` but returns `changed` only when the stored profile value differs (equality compares only `profile`); `sync_contact_profiles` calls `persister.store` only when `any_changed` is true. A re-check confirming the same present or confirmed-absent profile updates the timestamp in memory only — the FFI persistence callback never fires. After cold start, restored rows carry the stale `checked_at_ms`, `should_fetch_profile` immediately re-issues an `In` query for every unchanged contact, and the negative-cache backoff effectively doesn't survive restart. This amplifies both the pre-consent avatar tracking side channel (restart re-issues per-victim GETs to known-stable hosts) and the cold-start fetch storm. A failed earlier store is also not retried because later sweeps see `changed = false`. Return a separate `needs_persist` signal from `apply_fetched_profile` for timestamp-only refreshes, with coarse coalescing if write-amp matters.
- [SUGGESTION] In `packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs`:42-112: Paginated retrieve-all has no per-call total or elapsed budget — aggravated by Spec 2 un-ignore cursor rewind
`fetch_contact_requests_paginated` drains every matching document at `CONTACT_REQUEST_PAGE_SIZE = 100` per page with no per-call total cap, no elapsed-time budget, and no caller-visible continuation. On cold start, restore-from-seed, or any `after_created_at == None` path it walks the entire history in one call. Spec 2 sharpens the trigger: `unignore_sender` resets `high_water_received_ms` to `None`, so any UI un-ignore drops the next sweep onto the full retrieve-all. Combined with the recurring `DashPaySyncManager` tick, an attacker who spams `contactRequest` documents (cheap on testnet, ongoing-cost on mainnet) against one of the wallet's identities can make every recurring sweep monopolize the SDK gRPC client and starve every other identity — a cheap algorithmic-DoS surface against the platform proof-verification path. Add a configurable total-document ceiling per sweep with a resumable cursor, or at minimum a logged warn + early break above a threshold.
- [NITPICK] In `packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs`:352-411: Restore/persist FFI structs (Payment, ContactProfile, ContactProfileRow) lack compile-time layout guards
Sibling `#[repr(C)]` types crossing the same Swift↔Rust restore/persist boundary carry static-assert size/align guards (`AccountAddressPoolFFI`, `ContactRequestFFI`, `ContactRequestRemovalFFI`, `ContactIgnoredSenderFFI`, `IdentityKeyEntryFFI`, `IdentityEntryFFI`). Three boundary types introduced by this PR don't: `PaymentRestoreEntryFFI` (lines 352-367), `ContactProfileRestoreEntryFFI` (lines 383-410), and the persist-side `ContactProfileRowFFI` (identity_persistence.rs:180-211). The parent `IdentityEntryFFI` guard pins the pointer/count field but not the inner row layout Swift indexes through. A field reorder, type widen, or padding change on either side would compile silently and shift offsets relative to the Swift mirror, potentially mis-attributing restored profiles or payments to the wrong contact id / wrong owner. Add `const _: [u8; N] = [0u8; std::mem::size_of::<T>()];` + `align_of` tripwires to all three structs, matching precedent.
- [NITPICK] In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs`:177-190: payments_for_contact doc claims 'newest first' but iteration is lexicographic by hex txid
Verified at HEAD: the rustdoc at line 177 states the result is ordered 'newest first', but the implementation iterates `self.dashpay_payments` — a `BTreeMap<String, PaymentEntry>` keyed by hex `tx_id`. `PaymentEntry` carries no timestamp field, so the returned `Vec` is sorted lexicographically by hex-string txid, not by time. The per-contact payment-history UI consumer therefore renders payments in essentially random (txid-hash) order. A sender who can grind their outgoing txid can also influence display position, providing a small social-engineering surface at the UI layer. The accompanying test `payments_for_contact_filters_by_counterparty` does not assert ordering. Either drop the 'newest first' claim from the docstring, add an explicit timestamp field (e.g. `created_at_ms`) and sort by it, or have the caller stitch ordering from the SPV transaction store.
Inline dry-run could not load the GitHub PR diff because this PR exceeds GitHub's 20,000-line diff limit, so I posted the same verified findings as a top-level review body.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The latest delta (6512cae) correctly fixes the prior blocking compile break in the SQLite contacts persister. However, two new in-scope issues remain at HEAD: (1) Codex correctly identified that wallet wipe's phase-1 delete loop omits the newly added contactProfiles relationship — PersistentDashpayContactProfile.owner is non-optional, so a wipe on a wallet with cached contact profiles will hit the exact SwiftData fatal phase 1 exists to avoid; (2) Claude-security-auditor identified a real reader/writer asymmetry on the new ignored_senders relational table — writes go to SQLite but the (test-gated) reader restores only from entry_blob, which ignore_sender/unignore_sender never refresh. Eight prior findings remain valid and are carried forward; the prior compile break is resolved.
🔴 1 blocking | 🟡 7 suggestion(s) | 💬 2 nitpick(s)
10 additional finding(s)
blocking: Wallet wipe omits new contactProfiles before deleting identities — SwiftData fatal on wipe
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 3471)
The phase-1 loop pre-deletes every child whose inverse to PersistentIdentity is non-optional precisely because the comment at lines 3438-3470 documents that SwiftData fatals during save() when it has to null out a non-optional inverse on a child being processed in the same batch. This PR adds PersistentIdentity.contactProfiles: [PersistentDashpayContactProfile] (PersistentIdentity.swift:150) with cascade inverse PersistentDashpayContactProfile.owner: PersistentIdentity (PersistentDashpayContactProfile.swift:109) — that owner is non-optional, matching the exact shape the comment cites for dashpayPayments and dashpayIgnoredSenders. The phase-1 loop deletes dpnsNames, dashpayProfile, contactRequests, dashpayPayments, and dashpayIgnoredSenders — but not contactProfiles. A wallet with any cached contact profile rows will therefore hit the documented fatal during phase-2 identity deletion, aborting the user-initiated wipe and leaving the new cached profile rows (with attacker-controllable avatarUrl strings) on disk. Add the same delete pass for identity.contactProfiles.
for identity in identitiesToDelete {
for name in Array(identity.dpnsNames) {
backgroundContext.delete(name)
}
if let profile = identity.dashpayProfile {
backgroundContext.delete(profile)
}
for contactProfile in Array(identity.contactProfiles) {
backgroundContext.delete(contactProfile)
}
for cr in Array(identity.contactRequests) {
backgroundContext.delete(cr)
}
for payment in Array(identity.dashpayPayments) {
backgroundContext.delete(payment)
}
for ignored in Array(identity.dashpayIgnoredSenders) {
backgroundContext.delete(ignored)
}
}
suggestion: ignored_senders SQLite writes go to a relational table the reader never consults — Spec 2 ignore will silently revert once SQLite rehydration is wired
packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs (line 214)
The delta added ignored_senders as a relational SQLite table written via contacts::apply (contacts.rs:233-267). The reader managed_identity_from_entry restores ignored_senders: entry.ignored_senders.clone() from the identity's entry_blob only. But ignore_sender/unignore_sender (state/managed_identity/contact_requests.rs:132-191) emit ONLY a ContactChangeSet — they never call snapshot_changeset and so never produce an IdentityChangeSet that would re-encode entry_blob (identities.rs:33-78 only updates excluded.entry_blob from cs.identities). After a user ignores a sender, the relational row exists, but entry_blob.ignored_senders reflects whatever the last identity snapshot captured. Once the production Wallet::from_persisted path (currently TODO at persister.rs:909-916) starts using this reader, cold restart will restore ignored_senders = {} from a stale blob and the next sweep will resurface the blocked sender's still-immutable on-chain contactRequest. Combined with the carried-forward pre-consent avatar fetch, the user's local block becomes a restart-resilient deanonymization channel against blocked senders. Either load ignored_senders from the relational table in the reader (matches the schema's (wallet_id, owner_id, sender_id) source-of-truth and removes the dual-write hazard), or also emit an IdentityChangeSet from ignore/unignore so entry_blob stays in lockstep.
suggestion: Pending incoming requests fetch attacker-controlled avatar URLs before user consent
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift (line 119)
sync_contact_profiles includes managed.incoming_contact_requests.keys() in the per-owner fetch plan (profile.rs:640-650), so any sender who publishes a dashpay.profile and sends an unsolicited contactRequest populates the contact-profile cache before the victim accepts or ignores them. IncomingRequestRow feeds cachedProfile(row.contactIdentityId)?.avatarUrl into DashPayAvatarView → SwiftUI AsyncImage, issuing an HTTPS GET to the sender-controlled host on first render — leaking IP, online state, TLS/UA fingerprint, and providing per-victim URL-token receipt confirmation. With this PR's PersistentDashpayContactProfile SwiftData persistence the side channel survives restart, and Rust-side is_valid_avatar_url only checks scheme/length. The Spec 2 Ignore only mutes AFTER first render, and (per the new ignored_senders reader asymmetry) won't even survive cold restart on the SQLite backend. Suppress remote avatars Swift-side for unaccepted senders (initials-only variant for the Requests tab) or strip avatarUrl from the Rust→Swift projection until the sender becomes an established contact.
suggestion: Confirmed-absent contact profiles cannot cross the Swift persistence boundary — stale row resurrects on cold start
packages/rs-platform-wallet-ffi/src/identity_persistence.rs (line 587)
allocate_contact_profile_rows explicitly continues past entries with profile == None (lines 590-592: "Skip confirmed-absent entries — the negative cache is not persisted; it rebuilds on the next sweep"). Swift's upsertDashpayContactProfiles is append/refresh-only, and the restore path (ContactProfileRestoreEntryFFI) rehydrates every persisted row as ContactProfileEntry { profile: Some(..) }. When a contact removes their on-chain dashpay.profile, apply_fetched_profile flips the in-memory entry to None, but absence is dropped at the FFI projection — the stale PersistentDashpayContactProfile row survives on disk. On cold start the cache rehydrates as Some(stale_profile) and the UI re-issues GETs to the previously attacker-controlled avatarUrl. This compounds the pre-consent avatar fetch into a tracking channel that outlasts on-chain profile removal. Project absences as an explicit delete intent across the boundary (e.g. removed_contact_profile_ids: *const [u8;32]), mirroring the ContactIgnoredSenderFFI removal-set shape this PR already establishes.
suggestion: established_contact_get_note leaves *out_note undefined on every early-return path
packages/rs-platform-wallet-ffi/src/established_contact.rs (line 156)
Sibling established_contact_get_alias (lines 102-117) writes *out_alias = std::ptr::null_mut(); immediately after check_ptr!, so every early return leaves the out pointer at defined null. established_contact_get_note (lines 156-169) only calls check_ptr!(out_note) and skips the pre-null write. Early returns from unwrap_option_or_return! (storage NotFound / contact.note == None) and unwrap_result_or_return! (CString::new failure on interior NUL — reachable from on-chain note content) leave *out_note indeterminate across the C ABI. A Swift/C caller that doesn't pre-zero the slot can read a stale pointer or pass it to platform_wallet_string_free, risking invalid-free or double-free. The exported C ABI must not depend on caller hygiene — mirror the sibling pattern.
#[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);
*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: DashPay profile reader trio leaves *out_profile / *out_has_profile undefined on early-return paths
packages/rs-platform-wallet-ffi/src/dashpay_profile.rs (line 90)
managed_identity_get_dashpay_profile (line 90), platform_wallet_get_dashpay_profile (line 116), and platform_wallet_get_contact_profile (line 153) only assign *out_profile / *out_has_profile inside the present/no-profile match arms. After check_ptr!, early returns from unwrap_result_or_return!(read_identifier(..)) (invalid 32-byte input) and unwrap_option_or_return! (handle NotFound / no managed identity / no entry) leave both out pointers indeterminate. DashPayProfileFFI owns multiple *mut c_char fields released by dashpay_profile_ffi_free — a subsequent free on a slot the caller didn't pre-zero would free garbage pointers (invalid-free or double-free). Pre-write *out_profile = DashPayProfileFFI::empty() and *out_has_profile = false immediately after check_ptr! in each function, matching the established_contact_get_alias precedent.
suggestion: Contact-profile checked_at_ms not durably persisted when profile is unchanged
packages/rs-platform-wallet/src/wallet/identity/network/profile.rs (line 588)
apply_fetched_profile always refreshes checked_at_ms = now_ms (line 599) but returns changed only when the stored profile value differs (the equality compares only profile, line 594). sync_contact_profiles calls persister.store only when any_changed is true (lines 717-725). A re-check confirming the same present or confirmed-absent profile updates the timestamp in memory only — the FFI persistence callback never fires. After cold start, restored rows carry the stale checked_at_ms, should_fetch_profile immediately re-issues an In query for every unchanged contact, and the negative-cache backoff effectively doesn't survive restart. This amplifies the pre-consent avatar tracking side channel (restart re-issues per-victim GETs to known-stable hosts) and creates a cold-start fetch storm. A failed earlier store is also not retried because later sweeps see changed = false. Return a separate needs_persist signal from apply_fetched_profile for timestamp-only refreshes, with coarse coalescing if write-amp matters.
suggestion: Paginated retrieve-all has no per-call total or elapsed budget — algorithmic-DoS surface against the recurring sync loop
packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs (line 42)
fetch_contact_requests_paginated drains every matching document at CONTACT_REQUEST_PAGE_SIZE = 100 per page with no per-call total cap, no elapsed-time budget, and no caller-visible continuation. On cold start, restore-from-seed, or any after_created_at == None path it walks the entire history in one call. Spec 2 sharpens the trigger: unignore_sender resets high_water_received_ms to None, so any UI un-ignore drops the next sweep onto the full retrieve-all. Combined with the recurring DashPaySyncManager, an attacker who spams contactRequest documents (varying accountReference to defeat per-request dedup) against one of the wallet's identities can make every recurring sweep monopolize the SDK gRPC client and starve every other identity's sync — a cheap algorithmic-DoS surface against the verified-proof query path. Add a configurable total-document ceiling per sweep with a resumable cursor, or at minimum a logged-warn + early-break above a threshold.
nitpick: New restore/persist FFI structs lack compile-time layout guards
packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs (line 352)
Sibling #[repr(C)] types crossing the same Swift↔Rust restore/persist boundary carry static-assert size/align guards (AccountAddressPoolFFI, ContactRequestFFI, ContactRequestRemovalFFI, ContactIgnoredSenderFFI, IdentityKeyEntryFFI, IdentityEntryFFI). Three boundary types introduced by this PR don't: PaymentRestoreEntryFFI (wallet_restore_types.rs:352-367), ContactProfileRestoreEntryFFI (wallet_restore_types.rs:383-410), and the persist-side ContactProfileRowFFI (identity_persistence.rs:180-211). The parent IdentityEntryFFI guard pins the pointer/count fields but not the inner row layout Swift indexes through. A silent field reorder, type widen, or padding change on either side would compile cleanly and shift offsets relative to the Swift mirror, potentially mis-attributing restored profiles or payments to the wrong contact id / wrong owner (cross-account leak of cached profile / payment metadata). Add const _: [u8; N] = [0u8; std::mem::size_of::<T>()]; + matching align_of tripwires to all three structs, matching precedent.
nitpick: payments_for_contact doc claims 'newest first' but iteration is lexicographic by hex txid
packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs (line 177)
The rustdoc states the result is ordered 'newest first', but the implementation iterates self.dashpay_payments — a BTreeMap<String, PaymentEntry> keyed by hex tx_id. PaymentEntry carries no timestamp field, so the returned Vec is sorted lexicographically by hex-string txid, not by time. The per-contact payment-history UI consumer therefore renders payments in essentially random (txid-hash) order. The accompanying test payments_for_contact_filters_by_counterparty does not assert ordering. Either drop the 'newest first' claim from the docstring, add an explicit timestamp field (e.g. created_at_ms) and sort by it, or have the caller stitch ordering from the SPV transaction store.
🤖 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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:3471-3487: Wallet wipe omits new contactProfiles before deleting identities — SwiftData fatal on wipe
The phase-1 loop pre-deletes every child whose inverse to `PersistentIdentity` is non-optional precisely because the comment at lines 3438-3470 documents that SwiftData fatals during `save()` when it has to null out a non-optional inverse on a child being processed in the same batch. This PR adds `PersistentIdentity.contactProfiles: [PersistentDashpayContactProfile]` (PersistentIdentity.swift:150) with cascade inverse `PersistentDashpayContactProfile.owner: PersistentIdentity` (PersistentDashpayContactProfile.swift:109) — that `owner` is non-optional, matching the exact shape the comment cites for `dashpayPayments` and `dashpayIgnoredSenders`. The phase-1 loop deletes dpnsNames, dashpayProfile, contactRequests, dashpayPayments, and dashpayIgnoredSenders — but not contactProfiles. A wallet with any cached contact profile rows will therefore hit the documented fatal during phase-2 identity deletion, aborting the user-initiated wipe and leaving the new cached profile rows (with attacker-controllable `avatarUrl` strings) on disk. Add the same delete pass for `identity.contactProfiles`.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:214-218: ignored_senders SQLite writes go to a relational table the reader never consults — Spec 2 ignore will silently revert once SQLite rehydration is wired
The delta added `ignored_senders` as a relational SQLite table written via `contacts::apply` (contacts.rs:233-267). The reader `managed_identity_from_entry` restores `ignored_senders: entry.ignored_senders.clone()` from the identity's `entry_blob` only. But `ignore_sender`/`unignore_sender` (state/managed_identity/contact_requests.rs:132-191) emit ONLY a `ContactChangeSet` — they never call `snapshot_changeset` and so never produce an `IdentityChangeSet` that would re-encode `entry_blob` (identities.rs:33-78 only updates `excluded.entry_blob` from `cs.identities`). After a user ignores a sender, the relational row exists, but `entry_blob.ignored_senders` reflects whatever the last identity snapshot captured. Once the production `Wallet::from_persisted` path (currently TODO at persister.rs:909-916) starts using this reader, cold restart will restore `ignored_senders = {}` from a stale blob and the next sweep will resurface the blocked sender's still-immutable on-chain `contactRequest`. Combined with the carried-forward pre-consent avatar fetch, the user's local block becomes a restart-resilient deanonymization channel against blocked senders. Either load `ignored_senders` from the relational table in the reader (matches the schema's `(wallet_id, owner_id, sender_id)` source-of-truth and removes the dual-write hazard), or also emit an IdentityChangeSet from ignore/unignore so `entry_blob` stays in lockstep.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift`:119-129: Pending incoming requests fetch attacker-controlled avatar URLs before user consent
`sync_contact_profiles` includes `managed.incoming_contact_requests.keys()` in the per-owner fetch plan (profile.rs:640-650), so any sender who publishes a `dashpay.profile` and sends an unsolicited `contactRequest` populates the contact-profile cache before the victim accepts or ignores them. `IncomingRequestRow` feeds `cachedProfile(row.contactIdentityId)?.avatarUrl` into `DashPayAvatarView` → SwiftUI `AsyncImage`, issuing an HTTPS GET to the sender-controlled host on first render — leaking IP, online state, TLS/UA fingerprint, and providing per-victim URL-token receipt confirmation. With this PR's `PersistentDashpayContactProfile` SwiftData persistence the side channel survives restart, and Rust-side `is_valid_avatar_url` only checks scheme/length. The Spec 2 Ignore only mutes AFTER first render, and (per the new ignored_senders reader asymmetry) won't even survive cold restart on the SQLite backend. Suppress remote avatars Swift-side for unaccepted senders (initials-only variant for the Requests tab) or strip `avatarUrl` from the Rust→Swift projection until the sender becomes an established contact.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/identity_persistence.rs`:587-621: Confirmed-absent contact profiles cannot cross the Swift persistence boundary — stale row resurrects on cold start
`allocate_contact_profile_rows` explicitly `continue`s past entries with `profile == None` (lines 590-592: "Skip confirmed-absent entries — the negative cache is not persisted; it rebuilds on the next sweep"). Swift's `upsertDashpayContactProfiles` is append/refresh-only, and the restore path (`ContactProfileRestoreEntryFFI`) rehydrates every persisted row as `ContactProfileEntry { profile: Some(..) }`. When a contact removes their on-chain `dashpay.profile`, `apply_fetched_profile` flips the in-memory entry to `None`, but absence is dropped at the FFI projection — the stale `PersistentDashpayContactProfile` row survives on disk. On cold start the cache rehydrates as `Some(stale_profile)` and the UI re-issues GETs to the previously attacker-controlled `avatarUrl`. This compounds the pre-consent avatar fetch into a tracking channel that outlasts on-chain profile removal. Project absences as an explicit delete intent across the boundary (e.g. `removed_contact_profile_ids: *const [u8;32]`), mirroring the `ContactIgnoredSenderFFI` removal-set shape this PR already establishes.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/established_contact.rs`:156-169: established_contact_get_note leaves *out_note undefined on every early-return path
Sibling `established_contact_get_alias` (lines 102-117) writes `*out_alias = std::ptr::null_mut();` immediately after `check_ptr!`, so every early return leaves the out pointer at defined null. `established_contact_get_note` (lines 156-169) only calls `check_ptr!(out_note)` and skips the pre-null write. Early returns from `unwrap_option_or_return!` (storage NotFound / `contact.note == None`) and `unwrap_result_or_return!` (`CString::new` failure on interior NUL — reachable from on-chain note content) leave `*out_note` indeterminate across the C ABI. A Swift/C caller that doesn't pre-zero the slot can read a stale pointer or pass it to `platform_wallet_string_free`, risking invalid-free or double-free. The exported C ABI must not depend on caller hygiene — mirror the sibling pattern.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:90-185: DashPay profile reader trio leaves *out_profile / *out_has_profile undefined on early-return paths
`managed_identity_get_dashpay_profile` (line 90), `platform_wallet_get_dashpay_profile` (line 116), and `platform_wallet_get_contact_profile` (line 153) only assign `*out_profile` / `*out_has_profile` inside the present/no-profile match arms. After `check_ptr!`, early returns from `unwrap_result_or_return!(read_identifier(..))` (invalid 32-byte input) and `unwrap_option_or_return!` (handle NotFound / no managed identity / no entry) leave both out pointers indeterminate. `DashPayProfileFFI` owns multiple `*mut c_char` fields released by `dashpay_profile_ffi_free` — a subsequent free on a slot the caller didn't pre-zero would free garbage pointers (invalid-free or double-free). Pre-write `*out_profile = DashPayProfileFFI::empty()` and `*out_has_profile = false` immediately after `check_ptr!` in each function, matching the `established_contact_get_alias` precedent.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/identity/network/profile.rs`:588-725: Contact-profile checked_at_ms not durably persisted when profile is unchanged
`apply_fetched_profile` always refreshes `checked_at_ms = now_ms` (line 599) but returns `changed` only when the stored profile value differs (the equality compares only `profile`, line 594). `sync_contact_profiles` calls `persister.store` only when `any_changed` is true (lines 717-725). A re-check confirming the same present or confirmed-absent profile updates the timestamp in memory only — the FFI persistence callback never fires. After cold start, restored rows carry the stale `checked_at_ms`, `should_fetch_profile` immediately re-issues an `In` query for every unchanged contact, and the negative-cache backoff effectively doesn't survive restart. This amplifies the pre-consent avatar tracking side channel (restart re-issues per-victim GETs to known-stable hosts) and creates a cold-start fetch storm. A failed earlier store is also not retried because later sweeps see `changed = false`. Return a separate `needs_persist` signal from `apply_fetched_profile` for timestamp-only refreshes, with coarse coalescing if write-amp matters.
- [SUGGESTION] In `packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs`:42-112: Paginated retrieve-all has no per-call total or elapsed budget — algorithmic-DoS surface against the recurring sync loop
`fetch_contact_requests_paginated` drains every matching document at `CONTACT_REQUEST_PAGE_SIZE = 100` per page with no per-call total cap, no elapsed-time budget, and no caller-visible continuation. On cold start, restore-from-seed, or any `after_created_at == None` path it walks the entire history in one call. Spec 2 sharpens the trigger: `unignore_sender` resets `high_water_received_ms` to `None`, so any UI un-ignore drops the next sweep onto the full retrieve-all. Combined with the recurring `DashPaySyncManager`, an attacker who spams `contactRequest` documents (varying `accountReference` to defeat per-request dedup) against one of the wallet's identities can make every recurring sweep monopolize the SDK gRPC client and starve every other identity's sync — a cheap algorithmic-DoS surface against the verified-proof query path. Add a configurable total-document ceiling per sweep with a resumable cursor, or at minimum a logged-warn + early-break above a threshold.
- [NITPICK] In `packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs`:352-411: New restore/persist FFI structs lack compile-time layout guards
Sibling `#[repr(C)]` types crossing the same Swift↔Rust restore/persist boundary carry static-assert size/align guards (`AccountAddressPoolFFI`, `ContactRequestFFI`, `ContactRequestRemovalFFI`, `ContactIgnoredSenderFFI`, `IdentityKeyEntryFFI`, `IdentityEntryFFI`). Three boundary types introduced by this PR don't: `PaymentRestoreEntryFFI` (wallet_restore_types.rs:352-367), `ContactProfileRestoreEntryFFI` (wallet_restore_types.rs:383-410), and the persist-side `ContactProfileRowFFI` (identity_persistence.rs:180-211). The parent `IdentityEntryFFI` guard pins the pointer/count fields but not the inner row layout Swift indexes through. A silent field reorder, type widen, or padding change on either side would compile cleanly and shift offsets relative to the Swift mirror, potentially mis-attributing restored profiles or payments to the wrong contact id / wrong owner (cross-account leak of cached profile / payment metadata). Add `const _: [u8; N] = [0u8; std::mem::size_of::<T>()];` + matching `align_of` tripwires to all three structs, matching precedent.
- [NITPICK] In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs`:177-190: payments_for_contact doc claims 'newest first' but iteration is lexicographic by hex txid
The rustdoc states the result is ordered 'newest first', but the implementation iterates `self.dashpay_payments` — a `BTreeMap<String, PaymentEntry>` keyed by hex `tx_id`. `PaymentEntry` carries no timestamp field, so the returned `Vec` is sorted lexicographically by hex-string txid, not by time. The per-contact payment-history UI consumer therefore renders payments in essentially random (txid-hash) order. The accompanying test `payments_for_contact_filters_by_counterparty` does not assert ordering. Either drop the 'newest first' claim from the docstring, add an explicit timestamp field (e.g. `created_at_ms`) and sort by it, or have the caller stitch ordering from the SPV transaction store.
Inline dry-run could not load the GitHub PR diff because this PR exceeds GitHub's 20,000-line diff limit, so I posted the same verified findings as a top-level review body.
Refactor the per-request reject machinery into a per-sender ignore (= block, reversible), LOCAL-ONLY (R1: syncing it via contactInfo leaks who you ignored). - Rust: ManagedIdentity.rejected_contact_requests (keyed (sender,accountReference)) -> ignored_senders: BTreeSet<Identifier>. ignore_sender / is_sender_ignored / unignore_sender (removes + resets the high-water cursor so the sender's requests re-fetch). Sync suppression checks is_sender_ignored first, suppressing ALL of an ignored sender's requests incl. rotations; established-contact rotation (apply_rotated_incoming_request) untouched. Changeset ignored/unignored + IdentityEntry.ignored_senders (union merge); removed_incoming still emitted. - FFI: ContactRequestRejectionFFI -> ContactIgnoredSenderFFI; restore_dashpay_rejected -> restore_dashpay_ignored ([u8;32] id array); platform_wallet_ignore_contact_sender + new platform_wallet_unignore_contact_sender. - Swift: PersistentDashpayRejectedRequest -> PersistentDashpayIgnoredSender; handler store/restore + wallet-wipe PHASE-1; ignoreContactSender / unignoreContactSender. UI: ContactRequestsView reject->Ignore; new IgnoredContactsView (@Query-driven, name/avatar via getContactProfile, Un-ignore). Implemented via the rust-master-engineer agent; reviewed by a correctness + FFI-memory-safety audit (both array directions verified safe, mirroring the contact_profiles precedent). Fixed the audit's one HIGH finding: a TOCTOU where a concurrent sync sweep clobbered the un-ignore cursor rewind -> added advance_if_unchanged (compare-and-advance: only advance if the cursor still equals the sweep's snapshot, so a concurrent un-ignore reset wins), unit-pinned. Plus doc-rot fixes. Verified: 273 platform-wallet + 110 FFI tests + build_ios.sh BUILD SUCCEEDED. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…I done Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A label shorter than 16 chars produced a 32-byte AES-CBC blob that fails the contract's encryptedAccountLabel 48..80-byte constraint. kotlin/dashj pad the label to >=16 chars with trailing spaces and always emit it (empty -> 16 spaces). Match that on encrypt (pad_account_label, mirrors padEnd(16,' ')) and trim the trailing spaces on decrypt — required for cross-client interop. Tests: pad matches padEnd(16); short/empty labels clear the 48-byte floor and round-trip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…vention We were trusting SharedSecret::new to compute the DIP-15 ECDH from a library comment, not bytes — a cross-impl mismatch with dashj would silently break contactRequest/contactInfo interop. Add a KAT that, for fixed keys, recomputes the shared key by hand (shared point P = a.B; key = SHA256((0x02|(P.y&1)) || P.x)) and pins both symmetry (a.B == b.A) and the exact compressed-y-prefix + x preimage layout (not x||y). Locks the byte-level assumption against a refactor or a secp256k1 swap. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tion) The contactInfo sync used limit:100, start:None — a wallet with >100 contacts silently dropped the rest (same truncation the contact-request fetch had). contactInfos are owner-scoped so it's not attackable like the request flood, but the fix is the same: drain all pages via a StartAfter document-id cursor on the ownerIdAndUpdatedAt index. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A non-Rust embedder that calls dash_sdk_dashpay_create_contact_request and then submits the document via its own generic document-put couldn't recover the 32-byte entropy used to derive the document id — without it consensus rejects the create transition (it recomputes the id from the entropy). Add an inline `entropy: [u8;32]` field (POD, no separate free) populated from the already- available ContactRequestResult.entropy. (Deferred from the PR #3841 review.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…es iOS) The "accountReference interop-break vs dashj" turns out not to be a break. An iOS-stack diff (dash-shared-core / dashsync-iOS / kotlin-platform / dash-evo-tool + DIP-15) found: - iOS dash-shared-core and our Rust are ALGEBRAICALLY IDENTICAL (iOS reverses the digest then reads LE first-4 == our BE last-4). - FOUR conventions exist (ours/iOS, Android le[0..4]>>4, dash-evo-tool/DIP-literal be[0..4]>>4) — but DIP-15 defines accountReference as a one-time-pad obfuscation recipients MUST ignore; only the original sender un-masks it. So every convention round-trips for its own sender and there is NO on-chain interop failure and no canonical value to match. Decision: keep ours (matches iOS, the most-deployed wallet → our sent requests are bit-identical to the incumbent's). Expanded the dip14 doc with the four-way split + the "recipients ignore it" rationale, extracted a testable extract_ask28, and added a KAT pinning our value (0x01c1d1e1 for ask[i]=i) and documenting the other conventions' divergent values. No masking change (we already match iOS). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add ManagedIdentity::payments_for_contact(contact) — filters dashpay_payments by counterparty_id, covering both sent and received (send_payment stamps the counterparty, so the "sent reverse lookup" the old SPV match_in_collection lacked is already on PaymentEntry). Closes the documented per-contact tx-history gap. TODO swept: marked P0 #3/#4 (sync stages), accountReference (keep-ours), encryptedAccountLabel, ECDH KAT, contactInfo pagination, rs-sdk-ffi entropy, per-contact accessor, and the iOS-stack diff DONE; key-selection AUTH fallback RESOLVED (deliberately not added — poor key separation); account' path, multi- account, send address-reuse marked blocked/deferred with reasons. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Timeless-comments cleanup across 18 DashPay source files in rs-platform-wallet / rs-platform-wallet-ffi: removed internal tracking tokens (G1a..G15 spec gates, M3 task 13 milestone labels, (P2) priority tags, "stage N") from comments and tracing log strings. Where a bare gate ID was load-bearing in the sentence, it's replaced with the plain-English concept it referenced (G4 → "host-side signing hook", G1c → "transient/permanent failure policy", G3 → "sender key-rotation", G1b → "established-contact account build", etc.) so each comment is self-contained. Comment/string-only: no function/type/field/signature/control-flow changes; build green (platform-wallet + platform-wallet-ffi), zero residual tracker tokens. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a status header: the implementable DashPay backlog is done/tested/pushed; the five remaining unchecked items are blocked on resources outside this codebase (funded devnet for integration tests + on-device UAT; a registered dashpay contract change for the encrypted profile ignored-list and query-level DoS filter) or are a deliberate privacy don't-do marker, not oversights. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…path fix The friendship-path `account' = 0'` item is no longer just "blocked cross-repo" — the upstream key-wallet fix is submitted as rust-dashcore#813 (honor the AccountType `index` instead of a hardcoded 0', red→green test, backward-compatible for account 0). Also correct the framing: it's not a counterparty-interop break (the recipient pays from the shared xpub and ignores accountReference per DIP-15) — it's a single-account limitation, the same gap as the deferred multi-account item, gated on #813. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
shumkov
force-pushed
the
feat/dashpay-m1-sync-correctness
branch
from
6512cae to
822b1ee
Compare
Make the DashPay design docs match what actually shipped on this branch: - SYNC_CORRECTNESS_SPEC (Spec 0): REVIEWED→IMPLEMENTED — both stages shipped (paginated/high-water request sync + id-keyed contact-profile cache), durably persisted to both the SQLite persister and SwiftData. - CONTACTINFO_FORMAT_SPEC (Spec 1): the §4 contactInfo reject/block field marked DEFINED-but-NOT-ADOPTED — R1 resolved ignore as local-only, so it is not carried on contactInfo; the shipped codec is alias/note/displayHidden/acceptedAccounts. - BLOCK_SPEC: PAUSED→SUPERSEDED — replaced by the implemented per-sender, reversible, local-only Ignore (Spec 2); the contactInfo cross-device route was rejected (R1); cross-device deferred to a future encrypted profile field (contract track). - SPEC.md: added a 2026-06-18 status block mapping the Part-0 gap table (G1–G15) to its resolution, plus accountReference (keep-ours) and the account-index path fix (upstream rust-dashcore#813 via dashcore bump PR #3936). Points at TODO.md as the authoritative item-by-item status. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative verification at 822b1ee. The latest delta from 6512cae is comment-only test cleanup in platform-wallet-storage migrations; no behavioral changes. All 10 prior findings independently re-verified against the worktree and remain STILL VALID. Headlined by the blocking SwiftData wipe omission of identity.contactProfiles (the in-source PHASE 1 comment explicitly enumerates the non-optional cascade-children that must be pre-deleted and PersistentDashpayContactProfile.owner is exactly such a child), plus the ignored_senders SQLite reader/writer asymmetry, the pre-consent attacker-controlled avatar fetch, and three FFI out-param hygiene defects.
🔴 1 blocking | 🟡 7 suggestion(s) | 💬 2 nitpick(s)
10 additional finding(s)
blocking: Wallet wipe omits contactProfiles before deleting identities — SwiftData fatal will abort user-initiated wipe
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 3471)
Verified at HEAD (lines 3471-3487): phase 1 deletes dpnsNames, dashpayProfile, contactRequests, dashpayPayments, and dashpayIgnoredSenders, but not contactProfiles. The in-source comment block at 3438-3470 documents exactly why this loop exists: SwiftData fatals during save() when a parent delete has to null out a non-optional inverse on a child in the same batch. This PR adds PersistentIdentity.contactProfiles (PersistentIdentity.swift:150) with cascade inverse PersistentDashpayContactProfile.owner: PersistentIdentity (PersistentDashpayContactProfile.swift:109) — that owner is non-optional, the exact shape the comment cites for the other children it does pre-delete. Any wallet whose recurring DashPay sync has cached a contact profile row (established contacts AND unsolicited senders, see profile.rs:640-650) will hit the documented fatal during phase 2's identity delete, aborting the user-initiated wipe and leaving cached PersistentDashpayContactProfile rows — including sender-controlled avatarUrl/displayName/publicMessage strings — on disk.
for identity in identitiesToDelete {
for name in Array(identity.dpnsNames) {
backgroundContext.delete(name)
}
if let profile = identity.dashpayProfile {
backgroundContext.delete(profile)
}
for contactProfile in Array(identity.contactProfiles) {
backgroundContext.delete(contactProfile)
}
for cr in Array(identity.contactRequests) {
backgroundContext.delete(cr)
}
for payment in Array(identity.dashpayPayments) {
backgroundContext.delete(payment)
}
for ignored in Array(identity.dashpayIgnoredSenders) {
backgroundContext.delete(ignored)
}
}
suggestion: ignored_senders SQLite writes go to a relational table the reader restores from entry_blob — local Ignore intent silently reverts on cold restart
packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs (line 214)
Verified at HEAD (identities.rs:218): ignored_senders: entry.ignored_senders.clone() rehydrates from the identity's serialized entry_blob (in-place comment at 214-217 confirms this). Meanwhile contacts::apply (contacts.rs:233-267) writes ignore/unignore into the relational ignored_senders table, and ignore_sender/unignore_sender (contact_requests.rs) emit only a ContactChangeSet — they never trigger an IdentityChangeSet so entry_blob.ignored_senders is not refreshed. Once the production Wallet::from_persisted path (currently TODO at persister.rs:909-916) wires this reader into production rehydration, cold restart restores ignored_senders = {} from a stale blob and the next sweep resurfaces the blocked sender's still-immutable on-chain contactRequest. Combined with the pre-consent avatar finding below, a user's local block becomes a restart-resilient deanonymization channel. Either load ignored_senders from the relational table in the reader (which is the schema's source of truth and removes the dual-write hazard), or emit an IdentityChangeSet from ignore/unignore so entry_blob stays in lockstep.
suggestion: Pending incoming requests fetch attacker-controlled avatar URLs before user consent
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift (line 119)
Verified at HEAD: sync_contact_profiles (profile.rs:640-650) explicitly includes managed.incoming_contact_requests.keys() in the per-owner fetch plan, populating the contact-profile cache for senders the victim has never accepted. IncomingRequestRow feeds cachedProfile(row.contactIdentityId)?.avatarUrl into DashPayAvatarView → SwiftUI AsyncImage, issuing an HTTPS GET to the sender-controlled host on first render — leaking IP, online state, TLS/UA fingerprint, and per-victim URL-token receipt confirmation before any consent action. With this PR's PersistentDashpayContactProfile SwiftData persistence the channel survives restart; Rust-side is_valid_avatar_url only checks scheme/length. Spec 2 Ignore mutes only AFTER first render, and (per the SQLite reader asymmetry) won't survive cold restart. Suppress remote avatars Swift-side for unaccepted senders (initials-only variant for the Requests tab), or strip avatarUrl from the Rust→Swift projection until the sender becomes an established contact.
suggestion: Confirmed-absent contact profiles cannot cross the Swift persistence boundary — stale row resurrects on cold start
packages/rs-platform-wallet-ffi/src/identity_persistence.rs (line 587)
Verified at HEAD: allocate_contact_profile_rows explicitly continues past entries with profile == None (in-comment: "Skip confirmed-absent entries — the negative cache is not persisted; it rebuilds on the next sweep"). The FFI projection carries no removal intent, Swift's upsertDashpayContactProfiles is append/refresh-only, and ContactProfileRestoreEntryFFI rehydrates every persisted row as ContactProfileEntry { profile: Some(..) }. When a contact removes their on-chain dashpay.profile, apply_fetched_profile flips the in-memory entry to None (profile.rs:594-602), but absence is dropped at the FFI projection — the stale PersistentDashpayContactProfile row survives on disk. On cold start the cache rehydrates as Some(stale_profile) and the UI re-issues GETs to the previously attacker-controlled avatarUrl. Project absences as an explicit delete intent across the boundary (e.g. removed_contact_profile_ids: *const [u8;32]), mirroring the ContactIgnoredSenderFFI removal-set shape this PR already establishes.
suggestion: established_contact_get_note leaves *out_note undefined on every early-return path
packages/rs-platform-wallet-ffi/src/established_contact.rs (line 156)
Verified at HEAD: sibling established_contact_get_alias (lines 102-117) writes *out_alias = std::ptr::null_mut(); at line 108 immediately after check_ptr!, establishing the C-ABI contract that out_* is defined-null on early returns. established_contact_get_note (lines 156-169) only calls check_ptr!(out_note) and skips the pre-null write. Early returns from unwrap_option_or_return! (storage NotFound or contact.note == None) and unwrap_result_or_return! (CString::new failure on interior NUL — reachable from on-chain note content) leave *out_note indeterminate across the C ABI. A Swift/C caller that doesn't pre-zero the slot can read a stale pointer or pass it to platform_wallet_string_free, risking invalid-free or double-free. Mirror the sibling pattern.
#[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);
*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: DashPay profile reader trio leaves *out_profile / *out_has_profile undefined on early-return paths
packages/rs-platform-wallet-ffi/src/dashpay_profile.rs (line 90)
Verified at HEAD: managed_identity_get_dashpay_profile (line 90), platform_wallet_get_dashpay_profile (line 116), and platform_wallet_get_contact_profile (line 153) only assign *out_profile / *out_has_profile inside the present/no-profile match arms. After check_ptr!, early returns from unwrap_result_or_return!(read_identifier(..)) (invalid 32-byte input at 125, 163-164) and unwrap_option_or_return! (handle NotFound, missing managed identity, missing entry at 100, 132-133, 173) leave both out pointers indeterminate. DashPayProfileFFI owns multiple *mut c_char fields released by dashpay_profile_ffi_free — a subsequent free on a slot the caller didn't pre-zero would free garbage pointers (invalid-free or double-free). Pre-write *out_profile = DashPayProfileFFI::empty() and *out_has_profile = false immediately after check_ptr! in each function, matching the established_contact_get_alias precedent (line 108).
suggestion: Contact-profile checked_at_ms not durably persisted when profile is unchanged — negative-cache backoff does not survive restart
packages/rs-platform-wallet/src/wallet/identity/network/profile.rs (line 588)
Verified at HEAD: apply_fetched_profile always refreshes checked_at_ms = now_ms (line 599) but returns changed only when the stored profile value differs (equality at 594 compares only profile). sync_contact_profiles calls persister.store only when any_changed is true (717-725). A re-check confirming the same present or confirmed-absent profile updates the timestamp in memory only — the FFI persistence callback never fires. After cold start, restored rows carry stale checked_at_ms, should_fetch_profile immediately re-issues an In query for every unchanged contact, and the negative-cache backoff effectively doesn't survive restart. This amplifies the pre-consent avatar tracking channel (restart re-issues per-victim GETs) and creates a cold-start fetch storm. A failed earlier store is also not retried because later sweeps see changed = false. Return a separate needs_persist signal from apply_fetched_profile for timestamp-only refreshes, with coarse coalescing if write-amp matters.
suggestion: Paginated retrieve-all has no per-call total or elapsed budget — algorithmic-DoS surface against the recurring sync loop
packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs (line 42)
Verified at HEAD: fetch_contact_requests_paginated drains every matching document at CONTACT_REQUEST_PAGE_SIZE = 100 per page with no per-call total cap, no elapsed-time budget, and no caller-visible continuation. On cold start, restore-from-seed, or any after_created_at == None path it walks the entire history in one call. Spec 2 sharpens the trigger: unignore_sender resets high_water_received_ms to None, so any UI un-ignore drops the next sweep onto the full retrieve-all. Combined with the recurring DashPaySyncManager tick, an attacker who spams contactRequest documents (varying accountReference to defeat per-request dedup) against one of the wallet's identities can make every recurring sweep monopolize the SDK gRPC client and starve every other identity's sync — a cheap algorithmic-DoS against the verified-proof query path. Add a configurable total-document ceiling per sweep with a resumable cursor, or at minimum a logged-warn + early-break above a threshold.
nitpick: New restore/persist FFI structs lack compile-time layout guards
packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs (line 352)
Verified at HEAD: sibling #[repr(C)] types crossing the same Swift↔Rust restore/persist boundary carry static-assert size/align guards (AccountAddressPoolFFI, ContactRequestFFI, ContactRequestRemovalFFI, ContactIgnoredSenderFFI, IdentityKeyEntryFFI, IdentityEntryFFI). Three boundary types introduced by this PR don't: PaymentRestoreEntryFFI (wallet_restore_types.rs:352-367), ContactProfileRestoreEntryFFI (wallet_restore_types.rs:383-410), and the persist-side ContactProfileRowFFI (identity_persistence.rs:180-211). The parent IdentityEntryFFI guard pins the pointer/count fields but not the inner row layout Swift indexes through. A silent field reorder, type widen, or padding change on either side would compile cleanly and shift offsets relative to the Swift mirror, potentially mis-attributing restored profiles or payments to the wrong contact id / wrong owner. Add const _: [u8; N] = [0u8; std::mem::size_of::<T>()]; + matching align_of tripwires to all three structs, matching precedent.
nitpick: payments_for_contact doc claims 'newest first' but iteration is lexicographic by hex txid
packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs (line 177)
Verified at HEAD: the rustdoc states the result is ordered 'newest first', but the implementation iterates self.dashpay_payments — a BTreeMap<String, PaymentEntry> keyed by hex tx_id. PaymentEntry carries no timestamp field, so the returned Vec is sorted lexicographically by hex-string txid, not by time. The per-contact payment-history UI consumer renders payments in essentially random (txid-hash) order. Test payments_for_contact_filters_by_counterparty does not assert ordering. A sender who can grind their outgoing txid can also influence display position, providing a small social-engineering surface at the UI layer. Either drop the 'newest first' claim from the docstring, add a created_at_ms field and sort by it, or have the caller stitch ordering from the SPV transaction store.
🤖 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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:3471-3487: Wallet wipe omits contactProfiles before deleting identities — SwiftData fatal will abort user-initiated wipe
Verified at HEAD (lines 3471-3487): phase 1 deletes dpnsNames, dashpayProfile, contactRequests, dashpayPayments, and dashpayIgnoredSenders, but not contactProfiles. The in-source comment block at 3438-3470 documents exactly why this loop exists: SwiftData fatals during save() when a parent delete has to null out a non-optional inverse on a child in the same batch. This PR adds PersistentIdentity.contactProfiles (PersistentIdentity.swift:150) with cascade inverse PersistentDashpayContactProfile.owner: PersistentIdentity (PersistentDashpayContactProfile.swift:109) — that owner is non-optional, the exact shape the comment cites for the other children it does pre-delete. Any wallet whose recurring DashPay sync has cached a contact profile row (established contacts AND unsolicited senders, see profile.rs:640-650) will hit the documented fatal during phase 2's identity delete, aborting the user-initiated wipe and leaving cached PersistentDashpayContactProfile rows — including sender-controlled avatarUrl/displayName/publicMessage strings — on disk.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:214-218: ignored_senders SQLite writes go to a relational table the reader restores from entry_blob — local Ignore intent silently reverts on cold restart
Verified at HEAD (identities.rs:218): `ignored_senders: entry.ignored_senders.clone()` rehydrates from the identity's serialized entry_blob (in-place comment at 214-217 confirms this). Meanwhile contacts::apply (contacts.rs:233-267) writes ignore/unignore into the relational ignored_senders table, and ignore_sender/unignore_sender (contact_requests.rs) emit only a ContactChangeSet — they never trigger an IdentityChangeSet so entry_blob.ignored_senders is not refreshed. Once the production Wallet::from_persisted path (currently TODO at persister.rs:909-916) wires this reader into production rehydration, cold restart restores ignored_senders = {} from a stale blob and the next sweep resurfaces the blocked sender's still-immutable on-chain contactRequest. Combined with the pre-consent avatar finding below, a user's local block becomes a restart-resilient deanonymization channel. Either load ignored_senders from the relational table in the reader (which is the schema's source of truth and removes the dual-write hazard), or emit an IdentityChangeSet from ignore/unignore so entry_blob stays in lockstep.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift`:119-129: Pending incoming requests fetch attacker-controlled avatar URLs before user consent
Verified at HEAD: sync_contact_profiles (profile.rs:640-650) explicitly includes managed.incoming_contact_requests.keys() in the per-owner fetch plan, populating the contact-profile cache for senders the victim has never accepted. IncomingRequestRow feeds cachedProfile(row.contactIdentityId)?.avatarUrl into DashPayAvatarView → SwiftUI AsyncImage, issuing an HTTPS GET to the sender-controlled host on first render — leaking IP, online state, TLS/UA fingerprint, and per-victim URL-token receipt confirmation before any consent action. With this PR's PersistentDashpayContactProfile SwiftData persistence the channel survives restart; Rust-side is_valid_avatar_url only checks scheme/length. Spec 2 Ignore mutes only AFTER first render, and (per the SQLite reader asymmetry) won't survive cold restart. Suppress remote avatars Swift-side for unaccepted senders (initials-only variant for the Requests tab), or strip avatarUrl from the Rust→Swift projection until the sender becomes an established contact.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/identity_persistence.rs`:587-621: Confirmed-absent contact profiles cannot cross the Swift persistence boundary — stale row resurrects on cold start
Verified at HEAD: allocate_contact_profile_rows explicitly continues past entries with profile == None (in-comment: "Skip confirmed-absent entries — the negative cache is not persisted; it rebuilds on the next sweep"). The FFI projection carries no removal intent, Swift's upsertDashpayContactProfiles is append/refresh-only, and ContactProfileRestoreEntryFFI rehydrates every persisted row as ContactProfileEntry { profile: Some(..) }. When a contact removes their on-chain dashpay.profile, apply_fetched_profile flips the in-memory entry to None (profile.rs:594-602), but absence is dropped at the FFI projection — the stale PersistentDashpayContactProfile row survives on disk. On cold start the cache rehydrates as Some(stale_profile) and the UI re-issues GETs to the previously attacker-controlled avatarUrl. Project absences as an explicit delete intent across the boundary (e.g. removed_contact_profile_ids: *const [u8;32]), mirroring the ContactIgnoredSenderFFI removal-set shape this PR already establishes.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/established_contact.rs`:156-169: established_contact_get_note leaves *out_note undefined on every early-return path
Verified at HEAD: sibling established_contact_get_alias (lines 102-117) writes `*out_alias = std::ptr::null_mut();` at line 108 immediately after check_ptr!, establishing the C-ABI contract that out_* is defined-null on early returns. established_contact_get_note (lines 156-169) only calls check_ptr!(out_note) and skips the pre-null write. Early returns from unwrap_option_or_return! (storage NotFound or contact.note == None) and unwrap_result_or_return! (CString::new failure on interior NUL — reachable from on-chain note content) leave *out_note indeterminate across the C ABI. A Swift/C caller that doesn't pre-zero the slot can read a stale pointer or pass it to platform_wallet_string_free, risking invalid-free or double-free. Mirror the sibling pattern.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:90-185: DashPay profile reader trio leaves *out_profile / *out_has_profile undefined on early-return paths
Verified at HEAD: managed_identity_get_dashpay_profile (line 90), platform_wallet_get_dashpay_profile (line 116), and platform_wallet_get_contact_profile (line 153) only assign *out_profile / *out_has_profile inside the present/no-profile match arms. After check_ptr!, early returns from unwrap_result_or_return!(read_identifier(..)) (invalid 32-byte input at 125, 163-164) and unwrap_option_or_return! (handle NotFound, missing managed identity, missing entry at 100, 132-133, 173) leave both out pointers indeterminate. DashPayProfileFFI owns multiple *mut c_char fields released by dashpay_profile_ffi_free — a subsequent free on a slot the caller didn't pre-zero would free garbage pointers (invalid-free or double-free). Pre-write `*out_profile = DashPayProfileFFI::empty()` and `*out_has_profile = false` immediately after check_ptr! in each function, matching the established_contact_get_alias precedent (line 108).
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/identity/network/profile.rs`:588-725: Contact-profile checked_at_ms not durably persisted when profile is unchanged — negative-cache backoff does not survive restart
Verified at HEAD: apply_fetched_profile always refreshes checked_at_ms = now_ms (line 599) but returns changed only when the stored profile value differs (equality at 594 compares only profile). sync_contact_profiles calls persister.store only when any_changed is true (717-725). A re-check confirming the same present or confirmed-absent profile updates the timestamp in memory only — the FFI persistence callback never fires. After cold start, restored rows carry stale checked_at_ms, should_fetch_profile immediately re-issues an In query for every unchanged contact, and the negative-cache backoff effectively doesn't survive restart. This amplifies the pre-consent avatar tracking channel (restart re-issues per-victim GETs) and creates a cold-start fetch storm. A failed earlier store is also not retried because later sweeps see changed = false. Return a separate needs_persist signal from apply_fetched_profile for timestamp-only refreshes, with coarse coalescing if write-amp matters.
- [SUGGESTION] In `packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs`:42-112: Paginated retrieve-all has no per-call total or elapsed budget — algorithmic-DoS surface against the recurring sync loop
Verified at HEAD: fetch_contact_requests_paginated drains every matching document at CONTACT_REQUEST_PAGE_SIZE = 100 per page with no per-call total cap, no elapsed-time budget, and no caller-visible continuation. On cold start, restore-from-seed, or any after_created_at == None path it walks the entire history in one call. Spec 2 sharpens the trigger: unignore_sender resets high_water_received_ms to None, so any UI un-ignore drops the next sweep onto the full retrieve-all. Combined with the recurring DashPaySyncManager tick, an attacker who spams contactRequest documents (varying accountReference to defeat per-request dedup) against one of the wallet's identities can make every recurring sweep monopolize the SDK gRPC client and starve every other identity's sync — a cheap algorithmic-DoS against the verified-proof query path. Add a configurable total-document ceiling per sweep with a resumable cursor, or at minimum a logged-warn + early-break above a threshold.
- [NITPICK] In `packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs`:352-411: New restore/persist FFI structs lack compile-time layout guards
Verified at HEAD: sibling #[repr(C)] types crossing the same Swift↔Rust restore/persist boundary carry static-assert size/align guards (AccountAddressPoolFFI, ContactRequestFFI, ContactRequestRemovalFFI, ContactIgnoredSenderFFI, IdentityKeyEntryFFI, IdentityEntryFFI). Three boundary types introduced by this PR don't: PaymentRestoreEntryFFI (wallet_restore_types.rs:352-367), ContactProfileRestoreEntryFFI (wallet_restore_types.rs:383-410), and the persist-side ContactProfileRowFFI (identity_persistence.rs:180-211). The parent IdentityEntryFFI guard pins the pointer/count fields but not the inner row layout Swift indexes through. A silent field reorder, type widen, or padding change on either side would compile cleanly and shift offsets relative to the Swift mirror, potentially mis-attributing restored profiles or payments to the wrong contact id / wrong owner. Add `const _: [u8; N] = [0u8; std::mem::size_of::<T>()];` + matching align_of tripwires to all three structs, matching precedent.
- [NITPICK] In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs`:177-190: payments_for_contact doc claims 'newest first' but iteration is lexicographic by hex txid
Verified at HEAD: the rustdoc states the result is ordered 'newest first', but the implementation iterates self.dashpay_payments — a BTreeMap<String, PaymentEntry> keyed by hex tx_id. PaymentEntry carries no timestamp field, so the returned Vec is sorted lexicographically by hex-string txid, not by time. The per-contact payment-history UI consumer renders payments in essentially random (txid-hash) order. Test payments_for_contact_filters_by_counterparty does not assert ordering. A sender who can grind their outgoing txid can also influence display position, providing a small social-engineering surface at the UI layer. Either drop the 'newest first' claim from the docstring, add a created_at_ms field and sort by it, or have the caller stitch ordering from the SPV transaction store.
Inline dry-run could not load the GitHub PR diff because this PR exceeds GitHub's 20,000-line diff limit, so I posted the same verified findings as a top-level review body.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at 30d5112. Latest delta from 822b1ee is docs-only (4 DashPay spec markdown files); no code changed. All 10 prior findings re-verified line-by-line against the worktree and remain valid — three independent agent pairs (general/security/FFI) converged on the same set. Headlined by the blocking SwiftData wipe omission of identity.contactProfiles: lines 3471-3487 still don't pre-delete contactProfiles, while the in-source comment at 3438-3470 explicitly enumerates the non-optional cascade-children that PHASE 1 exists to handle, and PersistentDashpayContactProfile.owner is exactly such a child. Other carryovers cover the SQLite ignored_senders reader/writer asymmetry, pre-consent attacker-controlled avatar fetch, three FFI out-param hygiene defects, the confirmed-absent FFI projection gap, a timestamp-persistence gap, an unbounded paginated retrieve-all, missing #[repr(C)] layout guards, and a docs/behavior mismatch in payments_for_contact.
🔴 1 blocking | 🟡 7 suggestion(s) | 💬 2 nitpick(s)
10 additional finding(s)
blocking: Wallet wipe omits contactProfiles — SwiftData fatal will abort user-initiated wipe and leave attacker-controlled strings on disk
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 3471)
Verified at HEAD (3471-3487). PHASE 1 deletes dpnsNames, dashpayProfile, contactRequests, dashpayPayments, and dashpayIgnoredSenders, but NOT contactProfiles. The 3438-3470 in-source comment explicitly documents that this loop exists because SwiftData fatals during save() when a parent delete must null out a non-optional inverse on a child in the same batch. This PR introduces PersistentIdentity.contactProfiles whose inverse PersistentDashpayContactProfile.owner is non-optional — the exact shape the comment cites for the other children it does pre-delete. Any wallet whose recurring DashPay sync has cached even one PersistentDashpayContactProfile row (established contacts AND unsolicited senders, since profile.rs:640-650 includes incoming-request senders) will hit the documented fatal during PHASE 2's identity delete (3493-3496), aborting a user-initiated wipe and leaving cached PersistentDashpayContactProfile rows — including sender-controlled avatarUrl / displayName / publicMessage strings — on disk. A user who wipes precisely to scrub a stalker's footprint actually retains it.
for identity in identitiesToDelete {
for name in Array(identity.dpnsNames) {
backgroundContext.delete(name)
}
if let profile = identity.dashpayProfile {
backgroundContext.delete(profile)
}
for contactProfile in Array(identity.contactProfiles) {
backgroundContext.delete(contactProfile)
}
for cr in Array(identity.contactRequests) {
backgroundContext.delete(cr)
}
for payment in Array(identity.dashpayPayments) {
backgroundContext.delete(payment)
}
for ignored in Array(identity.dashpayIgnoredSenders) {
backgroundContext.delete(ignored)
}
}
suggestion: established_contact_get_note leaves *out_note undefined on every early-return path
packages/rs-platform-wallet-ffi/src/established_contact.rs (line 156)
Verified at HEAD. Sibling established_contact_get_alias (102-117) writes *out_alias = std::ptr::null_mut(); at line 108 immediately after check_ptr!, establishing the C-ABI contract on this module that out-params are defined-null on early returns. established_contact_get_note (156-169) only calls check_ptr!(out_note) and skips the pre-null write. Early returns from unwrap_option_or_return! (storage NotFound, contact.note == None) and unwrap_result_or_return! (CString::new failure on interior NUL, reachable from on-chain note content) leave *out_note indeterminate across the ABI. A Swift/C caller that doesn't pre-zero the slot can read a stale pointer or pass it to platform_wallet_string_free, risking invalid-free or double-free. The exported C ABI must not depend on caller hygiene — mirror the sibling pattern.
#[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);
*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: DashPay profile reader trio leaves *out_profile / *out_has_profile undefined on early-return paths
packages/rs-platform-wallet-ffi/src/dashpay_profile.rs (line 90)
Verified at HEAD. managed_identity_get_dashpay_profile (90), platform_wallet_get_dashpay_profile (116), and platform_wallet_get_contact_profile (153) only assign *out_profile / *out_has_profile inside the Some/None match arms (102-110, 134-143, 174-183). After check_ptr!, early returns from unwrap_result_or_return!(read_identifier(..)) (invalid 32-byte input at 125, 163-164) and unwrap_option_or_return! (handle NotFound, missing managed identity, missing contact entry at 100, 132-133, 173) leave both out pointers indeterminate. DashPayProfileFFI owns four *mut c_char fields released by dashpay_profile_ffi_free — a subsequent free on a slot the caller didn't pre-zero would free garbage pointers (invalid-free or double-free). Pre-write *out_profile = DashPayProfileFFI::empty(); *out_has_profile = false; immediately after check_ptr! in each function, matching the established_contact_get_alias precedent at line 108.
suggestion: Confirmed-absent contact profiles cannot cross the Swift persistence boundary — stale row resurrects on cold start
packages/rs-platform-wallet-ffi/src/identity_persistence.rs (line 587)
Verified at HEAD. allocate_contact_profile_rows (587-621) explicitly continues past entries with profile == None (590-592: "Skip confirmed-absent entries — the negative cache is not persisted; it rebuilds on the next sweep"). The FFI projection carries no removal intent for absences; Swift's upsertDashpayContactProfiles is append/refresh-only; and ContactProfileRestoreEntryFFI rehydrates every persisted row as ContactProfileEntry { profile: Some(..) }. When a contact removes their on-chain dashpay.profile, apply_fetched_profile (profile.rs:594-602) flips the in-memory entry to None, but absence is dropped at the FFI projection — the stale PersistentDashpayContactProfile row survives on disk. On cold start the cache rehydrates as Some(stale_profile) and the UI re-issues GETs to the previously attacker-controlled avatarUrl. This compounds the pre-consent avatar fetch into a tracking channel that outlasts on-chain profile removal. Project absences as an explicit delete intent across the boundary (e.g. removed_contact_profile_ids: *const [u8;32] + count), mirroring the ContactIgnoredSenderFFI removal-set shape this PR already establishes on the same callback.
suggestion: Pending incoming requests fetch attacker-controlled avatar URLs before user consent
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift (line 119)
Verified at HEAD. sync_contact_profiles (profile.rs:640-650) explicitly includes managed.incoming_contact_requests.keys() in the per-owner fetch plan, populating the contact-profile cache for senders the victim has never accepted. IncomingRequestRow (119-129) feeds cachedProfile(row.contactIdentityId)?.avatarUrl into DashPayAvatarView → SwiftUI AsyncImage, issuing an outbound HTTPS GET to the sender-controlled host on first render — leaking IP, online state, TLS/UA fingerprint, and per-victim URL-token receipt confirmation before any consent action. With this PR's PersistentDashpayContactProfile SwiftData persistence the channel survives restart; Rust-side is_valid_avatar_url only validates scheme/length. Spec 2 Ignore mutes only AFTER first render, and (per the ignored_senders reader asymmetry below) won't even survive cold restart on the SQLite backend. Suppress remote avatars Swift-side for unaccepted senders (initials-only variant for the Requests tab) or strip avatarUrl from the Rust→Swift projection until the sender becomes an established contact.
suggestion: ignored_senders SQLite writes go to a relational table the reader restores from entry_blob — local Ignore intent silently reverts on cold restart
packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs (line 214)
Verified at HEAD. Line 218 rehydrates ignored_senders: entry.ignored_senders.clone() from the identity's entry_blob (214-217 comment confirms scalar-snapshot collections ride the identity blob). Meanwhile contacts::apply (contacts.rs:233-267) writes ignore/unignore into the relational ignored_senders table, and ignore_sender / unignore_sender emit only a ContactChangeSet — they never trigger an IdentityChangeSet, so entry_blob.ignored_senders is not refreshed. Once production Wallet::from_persisted (TODO at persister.rs) wires this reader into production rehydration, cold restart restores ignored_senders = {} from a stale blob and the next sweep resurfaces blocked senders' still-immutable on-chain contactRequest. Combined with the pre-consent avatar fetch, the user's local block becomes a restart-resilient deanonymization channel against blocked senders. Either load ignored_senders from the relational table in the reader (the schema's source of truth, removes the dual-write hazard), or emit an IdentityChangeSet from ignore/unignore so entry_blob stays in lockstep.
suggestion: Contact-profile checked_at_ms not durably persisted when profile is unchanged — negative-cache backoff does not survive restart
packages/rs-platform-wallet/src/wallet/identity/network/profile.rs (line 588)
Verified at HEAD. apply_fetched_profile always refreshes checked_at_ms = now_ms (599), but returns changed only when the stored profile value differs (equality at 594 compares only profile). sync_contact_profiles calls persister.store only when any_changed is true (717-725). A re-check confirming the same present or confirmed-absent profile updates the timestamp in memory only — the FFI persistence callback never fires. After cold start, restored rows carry stale checked_at_ms, should_fetch_profile immediately re-issues an In query for every unchanged contact, and the negative-cache backoff effectively doesn't survive restart. This amplifies the pre-consent avatar tracking channel (restart re-issues per-victim GETs to known-stable hosts) and creates a cold-start fetch storm. A failed earlier store is also not retried because later sweeps see changed = false. Return a separate needs_persist signal from apply_fetched_profile for timestamp-only refreshes, with coarse coalescing if write-amp matters.
suggestion: Paginated retrieve-all has no per-call total or elapsed budget — algorithmic-DoS surface against the recurring sync loop
packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs (line 42)
Verified at HEAD. fetch_contact_requests_paginated drains every matching document at CONTACT_REQUEST_PAGE_SIZE = 100 per page with no per-call total cap, no elapsed-time budget, and no caller-visible continuation. On cold start, restore-from-seed, or any after_created_at == None path it walks the entire history in one call. Spec 2 sharpens the trigger: unignore_sender resets high_water_received_ms to None, so any UI un-ignore drops the next sweep onto the full retrieve-all. Combined with the recurring DashPaySyncManager tick (15s cadence per the updated SYNC_CORRECTNESS_SPEC), an attacker who spams contactRequest documents (varying accountReference to defeat per-request dedup) against one of the wallet's identities can make every recurring sweep monopolize the SDK gRPC client and starve every other identity's sync — a cheap algorithmic-DoS against the verified-proof query path. Add a configurable total-document ceiling per sweep with a resumable cursor, or at minimum a logged-warn + early-break above a threshold.
nitpick: New restore/persist FFI structs lack compile-time layout guards
packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs (line 352)
Verified at HEAD. Sibling #[repr(C)] types crossing the same Swift↔Rust restore/persist boundary carry static-assert size/align guards: IdentityKeyEntryFFI (identity_persistence.rs:335-336), IdentityEntryFFI (identity_persistence.rs:376-377), ContactRequestFFI/ContactRequestRemovalFFI/ContactIgnoredSenderFFI (contact_persistence.rs:207-223). Three boundary types introduced by this PR don't: PaymentRestoreEntryFFI (wallet_restore_types.rs:352-367), ContactProfileRestoreEntryFFI (wallet_restore_types.rs:383-410), and the persist-side ContactProfileRowFFI (identity_persistence.rs:180-211). The parent IdentityEntryFFI guard pins the outer pointer/count fields but not the inner row layout Swift indexes through. A silent field reorder, type widen, or padding change on either side compiles cleanly and shifts offsets relative to the Swift mirror, potentially mis-attributing restored profiles or payments to the wrong contact id / wrong owner (cross-account leak of cached profile / payment metadata). Add const _: [u8; N] = [0u8; std::mem::size_of::<T>()]; + matching align_of tripwires to all three structs.
nitpick: payments_for_contact doc claims 'newest first' but iteration is lexicographic by hex txid
packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs (line 177)
Verified at HEAD. The rustdoc at line 177 states the result is ordered 'newest first', but the implementation iterates self.dashpay_payments — a BTreeMap<String, PaymentEntry> keyed by hex tx_id. PaymentEntry carries no timestamp field, so the returned Vec is sorted lexicographically by hex-string txid, not by time. The per-contact payment-history UI consumer therefore renders payments in essentially random (txid-hash) order. The accompanying test payments_for_contact_filters_by_counterparty does not assert ordering. A sender who can grind their outgoing txid can also influence display position, providing a small social-engineering surface at the UI layer. Either drop the 'newest first' claim from the docstring, add a created_at_ms field and sort by it, or have the caller stitch ordering from the SPV transaction store.
🤖 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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:3471-3487: Wallet wipe omits contactProfiles — SwiftData fatal will abort user-initiated wipe and leave attacker-controlled strings on disk
Verified at HEAD (3471-3487). PHASE 1 deletes dpnsNames, dashpayProfile, contactRequests, dashpayPayments, and dashpayIgnoredSenders, but NOT contactProfiles. The 3438-3470 in-source comment explicitly documents that this loop exists because SwiftData fatals during save() when a parent delete must null out a non-optional inverse on a child in the same batch. This PR introduces PersistentIdentity.contactProfiles whose inverse PersistentDashpayContactProfile.owner is non-optional — the exact shape the comment cites for the other children it does pre-delete. Any wallet whose recurring DashPay sync has cached even one PersistentDashpayContactProfile row (established contacts AND unsolicited senders, since profile.rs:640-650 includes incoming-request senders) will hit the documented fatal during PHASE 2's identity delete (3493-3496), aborting a user-initiated wipe and leaving cached PersistentDashpayContactProfile rows — including sender-controlled avatarUrl / displayName / publicMessage strings — on disk. A user who wipes precisely to scrub a stalker's footprint actually retains it.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/established_contact.rs`:156-169: established_contact_get_note leaves *out_note undefined on every early-return path
Verified at HEAD. Sibling established_contact_get_alias (102-117) writes `*out_alias = std::ptr::null_mut();` at line 108 immediately after check_ptr!, establishing the C-ABI contract on this module that out-params are defined-null on early returns. established_contact_get_note (156-169) only calls check_ptr!(out_note) and skips the pre-null write. Early returns from unwrap_option_or_return! (storage NotFound, contact.note == None) and unwrap_result_or_return! (CString::new failure on interior NUL, reachable from on-chain note content) leave *out_note indeterminate across the ABI. A Swift/C caller that doesn't pre-zero the slot can read a stale pointer or pass it to platform_wallet_string_free, risking invalid-free or double-free. The exported C ABI must not depend on caller hygiene — mirror the sibling pattern.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:90-185: DashPay profile reader trio leaves *out_profile / *out_has_profile undefined on early-return paths
Verified at HEAD. managed_identity_get_dashpay_profile (90), platform_wallet_get_dashpay_profile (116), and platform_wallet_get_contact_profile (153) only assign *out_profile / *out_has_profile inside the Some/None match arms (102-110, 134-143, 174-183). After check_ptr!, early returns from unwrap_result_or_return!(read_identifier(..)) (invalid 32-byte input at 125, 163-164) and unwrap_option_or_return! (handle NotFound, missing managed identity, missing contact entry at 100, 132-133, 173) leave both out pointers indeterminate. DashPayProfileFFI owns four *mut c_char fields released by dashpay_profile_ffi_free — a subsequent free on a slot the caller didn't pre-zero would free garbage pointers (invalid-free or double-free). Pre-write `*out_profile = DashPayProfileFFI::empty(); *out_has_profile = false;` immediately after check_ptr! in each function, matching the established_contact_get_alias precedent at line 108.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/identity_persistence.rs`:587-621: Confirmed-absent contact profiles cannot cross the Swift persistence boundary — stale row resurrects on cold start
Verified at HEAD. allocate_contact_profile_rows (587-621) explicitly `continue`s past entries with profile == None (590-592: "Skip confirmed-absent entries — the negative cache is not persisted; it rebuilds on the next sweep"). The FFI projection carries no removal intent for absences; Swift's upsertDashpayContactProfiles is append/refresh-only; and ContactProfileRestoreEntryFFI rehydrates every persisted row as ContactProfileEntry { profile: Some(..) }. When a contact removes their on-chain dashpay.profile, apply_fetched_profile (profile.rs:594-602) flips the in-memory entry to None, but absence is dropped at the FFI projection — the stale PersistentDashpayContactProfile row survives on disk. On cold start the cache rehydrates as Some(stale_profile) and the UI re-issues GETs to the previously attacker-controlled avatarUrl. This compounds the pre-consent avatar fetch into a tracking channel that outlasts on-chain profile removal. Project absences as an explicit delete intent across the boundary (e.g. removed_contact_profile_ids: *const [u8;32] + count), mirroring the ContactIgnoredSenderFFI removal-set shape this PR already establishes on the same callback.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift`:119-129: Pending incoming requests fetch attacker-controlled avatar URLs before user consent
Verified at HEAD. sync_contact_profiles (profile.rs:640-650) explicitly includes managed.incoming_contact_requests.keys() in the per-owner fetch plan, populating the contact-profile cache for senders the victim has never accepted. IncomingRequestRow (119-129) feeds cachedProfile(row.contactIdentityId)?.avatarUrl into DashPayAvatarView → SwiftUI AsyncImage, issuing an outbound HTTPS GET to the sender-controlled host on first render — leaking IP, online state, TLS/UA fingerprint, and per-victim URL-token receipt confirmation before any consent action. With this PR's PersistentDashpayContactProfile SwiftData persistence the channel survives restart; Rust-side is_valid_avatar_url only validates scheme/length. Spec 2 Ignore mutes only AFTER first render, and (per the ignored_senders reader asymmetry below) won't even survive cold restart on the SQLite backend. Suppress remote avatars Swift-side for unaccepted senders (initials-only variant for the Requests tab) or strip avatarUrl from the Rust→Swift projection until the sender becomes an established contact.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:214-218: ignored_senders SQLite writes go to a relational table the reader restores from entry_blob — local Ignore intent silently reverts on cold restart
Verified at HEAD. Line 218 rehydrates `ignored_senders: entry.ignored_senders.clone()` from the identity's entry_blob (214-217 comment confirms scalar-snapshot collections ride the identity blob). Meanwhile contacts::apply (contacts.rs:233-267) writes ignore/unignore into the relational ignored_senders table, and ignore_sender / unignore_sender emit only a ContactChangeSet — they never trigger an IdentityChangeSet, so entry_blob.ignored_senders is not refreshed. Once production Wallet::from_persisted (TODO at persister.rs) wires this reader into production rehydration, cold restart restores ignored_senders = {} from a stale blob and the next sweep resurfaces blocked senders' still-immutable on-chain contactRequest. Combined with the pre-consent avatar fetch, the user's local block becomes a restart-resilient deanonymization channel against blocked senders. Either load ignored_senders from the relational table in the reader (the schema's source of truth, removes the dual-write hazard), or emit an IdentityChangeSet from ignore/unignore so entry_blob stays in lockstep.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/identity/network/profile.rs`:588-725: Contact-profile checked_at_ms not durably persisted when profile is unchanged — negative-cache backoff does not survive restart
Verified at HEAD. apply_fetched_profile always refreshes checked_at_ms = now_ms (599), but returns `changed` only when the stored profile value differs (equality at 594 compares only profile). sync_contact_profiles calls persister.store only when any_changed is true (717-725). A re-check confirming the same present or confirmed-absent profile updates the timestamp in memory only — the FFI persistence callback never fires. After cold start, restored rows carry stale checked_at_ms, should_fetch_profile immediately re-issues an `In` query for every unchanged contact, and the negative-cache backoff effectively doesn't survive restart. This amplifies the pre-consent avatar tracking channel (restart re-issues per-victim GETs to known-stable hosts) and creates a cold-start fetch storm. A failed earlier store is also not retried because later sweeps see `changed = false`. Return a separate needs_persist signal from apply_fetched_profile for timestamp-only refreshes, with coarse coalescing if write-amp matters.
- [SUGGESTION] In `packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs`:42-112: Paginated retrieve-all has no per-call total or elapsed budget — algorithmic-DoS surface against the recurring sync loop
Verified at HEAD. fetch_contact_requests_paginated drains every matching document at CONTACT_REQUEST_PAGE_SIZE = 100 per page with no per-call total cap, no elapsed-time budget, and no caller-visible continuation. On cold start, restore-from-seed, or any after_created_at == None path it walks the entire history in one call. Spec 2 sharpens the trigger: unignore_sender resets high_water_received_ms to None, so any UI un-ignore drops the next sweep onto the full retrieve-all. Combined with the recurring DashPaySyncManager tick (15s cadence per the updated SYNC_CORRECTNESS_SPEC), an attacker who spams contactRequest documents (varying accountReference to defeat per-request dedup) against one of the wallet's identities can make every recurring sweep monopolize the SDK gRPC client and starve every other identity's sync — a cheap algorithmic-DoS against the verified-proof query path. Add a configurable total-document ceiling per sweep with a resumable cursor, or at minimum a logged-warn + early-break above a threshold.
- [NITPICK] In `packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs`:352-411: New restore/persist FFI structs lack compile-time layout guards
Verified at HEAD. Sibling #[repr(C)] types crossing the same Swift↔Rust restore/persist boundary carry static-assert size/align guards: IdentityKeyEntryFFI (identity_persistence.rs:335-336), IdentityEntryFFI (identity_persistence.rs:376-377), ContactRequestFFI/ContactRequestRemovalFFI/ContactIgnoredSenderFFI (contact_persistence.rs:207-223). Three boundary types introduced by this PR don't: PaymentRestoreEntryFFI (wallet_restore_types.rs:352-367), ContactProfileRestoreEntryFFI (wallet_restore_types.rs:383-410), and the persist-side ContactProfileRowFFI (identity_persistence.rs:180-211). The parent IdentityEntryFFI guard pins the outer pointer/count fields but not the inner row layout Swift indexes through. A silent field reorder, type widen, or padding change on either side compiles cleanly and shifts offsets relative to the Swift mirror, potentially mis-attributing restored profiles or payments to the wrong contact id / wrong owner (cross-account leak of cached profile / payment metadata). Add `const _: [u8; N] = [0u8; std::mem::size_of::<T>()];` + matching align_of tripwires to all three structs.
- [NITPICK] In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs`:177-190: payments_for_contact doc claims 'newest first' but iteration is lexicographic by hex txid
Verified at HEAD. The rustdoc at line 177 states the result is ordered 'newest first', but the implementation iterates self.dashpay_payments — a BTreeMap<String, PaymentEntry> keyed by hex tx_id. PaymentEntry carries no timestamp field, so the returned Vec is sorted lexicographically by hex-string txid, not by time. The per-contact payment-history UI consumer therefore renders payments in essentially random (txid-hash) order. The accompanying test payments_for_contact_filters_by_counterparty does not assert ordering. A sender who can grind their outgoing txid can also influence display position, providing a small social-engineering surface at the UI layer. Either drop the 'newest first' claim from the docstring, add a created_at_ms field and sort by it, or have the caller stitch ordering from the SPV transaction store.
Inline dry-run could not load the GitHub PR diff because this PR exceeds GitHub's 20,000-line diff limit, so I posted the same verified findings as a top-level review body.
…ork() removal Updates all 8 rust-dashcore workspace deps 981e97f1 -> ceee4a9b40 (dev HEAD), which picks up #813 (DashPay account-index derivation path) and #814 (revert of the temporary #808 Core 23 nested-masternode-address SML changes), so platform keeps the existing flat-field masternode shape and no consensus change is needed. The bump also crosses #802 (removed Address::network(), replaced Network with AddressPrefix because an address's prefix is ambiguous across testnet/devnet and legacy-regtest). Migrate the four call sites: - wasm-sdk validate_address: addr.is_valid_for_network(net) — a bool, exactly what it computed and now correct for the shared testnet/devnet prefix. - platform-wallet derivation_path_for_derived_address: the path only distinguishes mainnet (coin 5') from everything else (1'), so probe is_valid_for_network(Mainnet). - platform-wallet-ffi address rendering (2 sites): an address_display_network helper probes mainnet/testnet/regtest, decisive for the bech32m platform-payment addresses rendered here (mainnet ds / testnet+devnet tb / regtest dsrt). Migration tracked in #3939. Full workspace --all-targets compiles. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ored_senders to fixtures The Spec 2 local-only Ignore feature (4169bcf) added `contact_profiles` and `ignored_senders` to IdentityEntry but did not update these two sqlite test fixtures, so the test crate has not compiled with --all-targets since. Add the (empty) fields. Pre-existing breakage, unrelated to the rust-dashcore bump; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
These files predated a `cargo fmt` pass, so the branch's CI `fmt --check` was red independent of the dashcore bump. Pure rustfmt output; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Clears the warnings that would fail CI `clippy -D warnings`, all in pre-existing branch
DashPay code (none from the dashcore bump):
- manual_repeat_n: repeat(' ').take(n) -> repeat_n(' ', n)
- op_ref: drop the needless & on the left operand
- type_complexity: factor an OwnerContactProfiles type alias
- assertions_on_constants: move the SYNC_OVERLAP_MS static invariant into a const block
- unnecessary_get_then_check (x3): get(k).is_none() -> !contains_key(k)
Behavior-preserving; the two affected assertions are intentional invariant/regression guards.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at HEAD 928c2c5. Latest delta (rustfmt, clippy cleanup, fixtures, and a rust-dashcore Address::network() removal migration introducing the address_display_network probe) is non-substantive except for one new latent issue: the address_display_network probe collapses regtest into testnet because base58 testnet/regtest/devnet addresses share is_valid_for_network(Testnet)=true, so regtest wallets render bech32m platform addresses with the testnet 'tb' HRP instead of 'dsrt'. Nine prior findings are carried forward as STILL VALID; the SQLite ignored_senders finding is downgraded to out-of-scope because the reader is now gated to #[cfg(any(test, feature='__test-helpers'))] and no longer wired into production rehydration. The blocking SwiftData wipe omission (contactProfiles) remains the gating issue.
Reviewed commit: 928c2c5
🔴 1 blocking | 🟡 7 suggestion(s) | 💬 2 nitpick(s)
Verified findings
blocking: [Carried forward — STILL VALID] Wallet wipe omits contactProfiles — SwiftData fatal will abort user-initiated wipe and leave attacker-controlled strings on disk
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 3471)
Verified at HEAD (3471-3487). PHASE 1 deletes dpnsNames, dashpayProfile, contactRequests, dashpayPayments, and dashpayIgnoredSenders, but NOT contactProfiles. The 3438-3470 in-source comment explicitly documents that this pre-delete loop exists because SwiftData fatals during save() when a parent delete must null out a non-optional inverse on a child in the same batch. PersistentIdentity.contactProfiles (Models/PersistentIdentity.swift:150) inverses to PersistentDashpayContactProfile.owner: PersistentIdentity (non-optional), which is the exact shape the comment cites for the other children it does pre-delete. profile.rs sync_contact_profiles includes incoming_contact_requests senders in the per-owner fetch plan, so any wallet whose recurring DashPay sync has cached even one PersistentDashpayContactProfile row (established contacts and unsolicited senders) will hit the documented fatal during PHASE 2's identity delete (3493-3496), aborting a user-initiated wipe and leaving cached PersistentDashpayContactProfile rows — including sender-controlled avatarUrl / displayName / publicMessage strings — on disk. A user who wipes precisely to scrub a stalker's footprint actually retains it.
for identity in identitiesToDelete {
for name in Array(identity.dpnsNames) {
backgroundContext.delete(name)
}
if let profile = identity.dashpayProfile {
backgroundContext.delete(profile)
}
for contactProfile in Array(identity.contactProfiles) {
backgroundContext.delete(contactProfile)
}
for cr in Array(identity.contactRequests) {
backgroundContext.delete(cr)
}
for payment in Array(identity.dashpayPayments) {
backgroundContext.delete(payment)
}
for ignored in Array(identity.dashpayIgnoredSenders) {
backgroundContext.delete(ignored)
}
}
suggestion: [New in latest delta] address_display_network cannot return Regtest for base58 inputs — regtest wallets render bech32m platform addresses with the testnet 'tb' HRP
packages/rs-platform-wallet-ffi/src/persistence.rs (line 2475)
Introduced in commit a7b05f7 (Address::network() removal migration). The helper probes Mainnet, then Testnet, falling back to Regtest. Per the rust-dashcore docs cited in the bumped dependency, a single non-mainnet base58 Dash address satisfies is_valid_for_network for Testnet, Regtest, and Devnet simultaneously (the base58 prefix is shared). The input here is a base58 dashcore::Address coming from AddressInfo, so a regtest address always short-circuits at the Testnet branch and the Regtest fallback at line 2482 is unreachable for any base58 input. Net effect: regtest wallets get their bech32m platform-payment addresses rendered with HRP 'tb' instead of 'dsrt', and any tooling/operator that copies those addresses into a regtest-aware tool gets an HRP mismatch. The doc-comment (2471-2474) asserts a behavior the helper does not deliver. Either thread the originating network from the wallet config (WalletInfo carries it), or drop the regtest branch and document explicitly that platform-wallet renders regtest as 'tb'.
suggestion: [Carried forward — STILL VALID] established_contact_get_note leaves *out_note undefined on every early-return path
packages/rs-platform-wallet-ffi/src/established_contact.rs (line 156)
Verified at HEAD. Sibling established_contact_get_alias (102-117) writes *out_alias = std::ptr::null_mut(); at line 108 immediately after check_ptr!, establishing the C-ABI contract on this module that out-params are defined-null on early returns. established_contact_get_note (156-169) only calls check_ptr!(out_note) and skips the pre-null write. Early returns from unwrap_option_or_return! (storage NotFound, contact.note == None) and unwrap_result_or_return! (CString::new failure on interior NUL, reachable from on-chain note content) leave *out_note indeterminate across the ABI. A C/Swift caller that doesn't pre-zero the slot can read a stale pointer or pass it to platform_wallet_string_free, risking invalid-free or double-free. The exported C ABI must not depend on caller hygiene — mirror the sibling pattern.
#[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);
*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: [Carried forward — STILL VALID] DashPay profile reader trio leaves *out_profile / *out_has_profile undefined on early-return paths
packages/rs-platform-wallet-ffi/src/dashpay_profile.rs (line 90)
Verified at HEAD. managed_identity_get_dashpay_profile (90), platform_wallet_get_dashpay_profile (116), and platform_wallet_get_contact_profile (153) only assign *out_profile / *out_has_profile inside the Some/None match arms (101-110, 134-143, 174-183). After check_ptr!, early returns from unwrap_result_or_return!(read_identifier(..)) (invalid 32-byte input at 125, 163-164) and unwrap_option_or_return! (handle NotFound, missing managed identity, missing contact entry) leave both out pointers indeterminate. DashPayProfileFFI owns four *mut c_char fields released by dashpay_profile_ffi_free — a subsequent free on a slot the caller didn't pre-zero would free garbage pointers (invalid-free or double-free). Pre-write *out_profile = DashPayProfileFFI::empty(); *out_has_profile = false; immediately after check_ptr! in each function, matching the established_contact_get_alias precedent at line 108.
suggestion: [Carried forward — STILL VALID] Confirmed-absent contact profiles cannot cross the Swift persistence boundary — stale row resurrects on cold start
packages/rs-platform-wallet-ffi/src/identity_persistence.rs (line 587)
Verified at HEAD. allocate_contact_profile_rows explicitly continues past entries with profile == None ('Skip confirmed-absent entries — the negative cache is not persisted; it rebuilds on the next sweep'). The FFI projection carries no removal intent for absences; Swift's upsertDashpayContactProfiles is append/refresh-only; and ContactProfileRestoreEntryFFI rehydrates every persisted row as ContactProfileEntry { profile: Some(..) }. When a contact removes their on-chain dashpay.profile, apply_fetched_profile flips the in-memory entry to None, but absence is dropped at the FFI projection — the stale PersistentDashpayContactProfile row survives on disk. On cold start the cache rehydrates as Some(stale_profile) and the UI re-issues GETs to the previously attacker-controlled avatarUrl. This compounds the pre-consent avatar fetch into a tracking channel that outlasts on-chain profile removal. Project absences as an explicit delete intent across the boundary (e.g. removed_contact_profile_ids: *const [u8;32] + count), mirroring the ContactIgnoredSenderFFI removal-set shape this PR already establishes on the same callback.
suggestion: [Carried forward — STILL VALID] Pending incoming requests fetch attacker-controlled avatar URLs before user consent
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift (line 119)
Verified at HEAD. sync_contact_profiles (profile.rs) explicitly includes managed.incoming_contact_requests.keys() in the per-owner fetch plan, populating the contact-profile cache for senders the victim has never accepted. IncomingRequestRow (119-129) feeds cachedProfile(row.contactIdentityId)?.avatarUrl into DashPayAvatarView → SwiftUI AsyncImage, issuing an outbound HTTPS GET to the sender-controlled host on first render — leaking IP, online state, TLS/UA fingerprint, and per-victim URL-token receipt confirmation before any consent action. With this PR's PersistentDashpayContactProfile SwiftData persistence the channel survives restart; Rust-side is_valid_avatar_url only validates scheme/length. The Ignore action mutes only AFTER first render. Suppress remote avatars Swift-side for unaccepted senders (initials-only variant for the Requests tab) or strip avatarUrl from the Rust→Swift projection until the sender becomes an established contact.
suggestion: [Carried forward — STILL VALID] Contact-profile checked_at_ms not durably persisted when profile is unchanged — negative-cache backoff does not survive restart
packages/rs-platform-wallet/src/wallet/identity/network/profile.rs (line 588)
Verified at HEAD. apply_fetched_profile always refreshes checked_at_ms = now_ms, but returns changed only when the stored profile value differs (equality compares only profile content). sync_contact_profiles calls persister.store only when any_changed is true. A re-check confirming the same present or confirmed-absent profile updates the timestamp in memory only — the FFI persistence callback never fires. After cold start, restored rows carry stale checked_at_ms, should_fetch_profile immediately re-issues an In query for every unchanged contact, and the negative-cache backoff effectively doesn't survive restart. This amplifies the pre-consent avatar tracking channel and creates a cold-start fetch storm. A failed earlier store is also not retried because later sweeps see changed = false. Return a separate needs_persist signal from apply_fetched_profile for timestamp-only refreshes, with coarse coalescing if write-amp matters.
suggestion: [Carried forward — STILL VALID] Paginated retrieve-all has no per-call total or elapsed budget — algorithmic-DoS surface against the recurring sync loop
packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs (line 42)
Verified at HEAD. fetch_contact_requests_paginated drains every matching document at CONTACT_REQUEST_PAGE_SIZE = 100 per page with no per-call total cap, no elapsed-time budget, and no caller-visible continuation. On cold start, restore-from-seed, or any after_created_at == None path it walks the entire history in one call. unignore_sender resets high_water_received_ms to None, so any UI un-ignore drops the next sweep onto the full retrieve-all. Combined with the recurring DashPaySyncManager tick, an attacker who spams contactRequest documents (varying accountReference to defeat per-request dedup) against one of the wallet's identities can make every recurring sweep monopolize the SDK gRPC client and starve every other identity's sync. Add a configurable total-document ceiling per sweep with a resumable cursor, or at minimum a logged-warn + early-break above a threshold.
nitpick: [Carried forward — STILL VALID] New restore/persist FFI structs lack compile-time layout guards
packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs (line 352)
Verified at HEAD. Sibling #[repr(C)] types crossing the same Swift↔Rust restore/persist boundary carry static-assert size/align guards: IdentityKeyEntryFFI, IdentityEntryFFI, ContactRequestFFI / ContactRequestRemovalFFI / ContactIgnoredSenderFFI. Three boundary types introduced by this PR don't: PaymentRestoreEntryFFI (wallet_restore_types.rs:352-367), ContactProfileRestoreEntryFFI (wallet_restore_types.rs:383-410), and the persist-side ContactProfileRowFFI (identity_persistence.rs:180-211). The parent IdentityEntryFFI guard pins the outer pointer/count fields but not the inner row layout Swift indexes through. A silent field reorder, type widen, or padding change on either side compiles cleanly and shifts offsets relative to the Swift mirror, potentially mis-attributing restored profiles or payments to the wrong contact id / wrong owner (cross-account leak of cached profile / payment metadata). Add const _: [u8; N] = [0u8; std::mem::size_of::<T>()]; + matching align_of tripwires to all three structs.
nitpick: [Carried forward — STILL VALID] payments_for_contact doc claims 'newest first' but iteration is lexicographic by hex txid
packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs (line 177)
Verified at HEAD. The rustdoc at line 177 states the result is 'newest first', but the implementation iterates self.dashpay_payments — a BTreeMap<String, PaymentEntry> keyed by hex tx_id. PaymentEntry carries no timestamp field, so the returned Vec is sorted lexicographically by hex-string txid, not by time. The per-contact payment-history UI consumer therefore renders payments in essentially random (txid-hash) order. A sender who can grind their outgoing txid can also influence display position, providing a small social-engineering surface at the UI layer. Either drop the 'newest first' claim from the docstring, add a created_at_ms field and sort by it, or have the caller stitch ordering from the SPV transaction store.
_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3841 928c2c5 --dry-run --prior-sha 30d5112
STDOUT:
STDERR:
Traceback (most recent call last):
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", li), so I posted the same verified findings as a top-level review body._
On-device UAT (testnet, 2026-06-19) surfaced a bug: an imported identity cannot sign any state transition — KeychainSigner finds no PersistentPublicKey matching the public key the Rust SDK supplies, even though all 5 keys are persisted correctly (exact match to the on-chain keys, verified via SwiftData). The signer matches by publicKeyData only (not identity-scoped), so the SDK is selecting a key the wallet didn't derive/persist — a key-selection/derivation issue on the import-existing-identity path. Captured the repro, diagnosis, and the harness state for resuming via create-in-app. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified at adb7b32. The latest delta from 928c2c5 is docs-only (docs/dashpay/TODO.md adding a UAT signing bug entry); no code changed in this delta. All 10 prior findings from the 928c2c5 review were re-verified line-by-line against the worktree and remain STILL VALID. Two blocking issues: (1) the SwiftData wallet-wipe PHASE 1 loop at PlatformWalletPersistenceHandler.swift:3471-3487 still omits identity.contactProfiles despite the in-source comment block at 3438-3470 explicitly enumerating exactly this non-optional-inverse cascade-child shape, and (2) the imported-identity load path at loading.rs:253-263 only attaches a derivation breadcrumb to the MASTER probe key while DPNS/profile flows (dpns.rs:206-220, profile.rs key selection) intentionally exclude MASTER and pick the first HIGH/CRITICAL key — Swift clears privateKeyKeychainIdentifier for rows lacking derivation indices (PlatformWalletPersistenceHandler.swift:1584-1597), so post-import signing fails with the exact KeychainSigner error the TODO.md now documents. Six carried-forward suggestions plus one new rs-sdk-ffi defensive-length check; two nitpicks dropped to stay under the 10-finding budget. CodeRabbit reported zero inline findings; nothing to react to.
🔴 2 blocking | 🟡 8 suggestion(s)
10 additional finding(s)
blocking: Wallet wipe omits contactProfiles — SwiftData fatal aborts the wipe and leaves attacker-controlled DashPay strings on disk
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 3471)
Verified at HEAD adb7b32. PHASE 1 of the wallet-wipe routine pre-deletes dpnsNames, dashpayProfile, contactRequests, dashpayPayments, and dashpayIgnoredSenders, but not identity.contactProfiles. The in-source comment block at 3438-3470 documents the exact invariant this loop is built to enforce: SwiftData fatals during save() when a parent delete must null out a non-optional inverse on a child in the same save batch, so every cascade-child whose inverse is non-optional must be deleted in its own save() ahead of the parent. PersistentIdentity.contactProfiles is cascade and PersistentDashpayContactProfile.owner is non-optional — exactly that shape. profile.rs sync_contact_profiles explicitly includes managed.incoming_contact_requests.keys() in the per-owner fetch plan (lines 640-650), so any wallet whose recurring DashPay sync has cached even one PersistentDashpayContactProfile row (established contacts OR unsolicited senders) will hit the documented fatal during PHASE 2's identity delete (3493-3496), abort the user-initiated wipe, and leave cached PersistentDashpayContactProfile rows — including sender-controlled avatarUrl, displayName, and publicMessage strings — on disk. A user who wipes precisely to scrub a stalker's footprint actually retains it.
for identity in identitiesToDelete {
for name in Array(identity.dpnsNames) {
backgroundContext.delete(name)
}
if let profile = identity.dashpayProfile {
backgroundContext.delete(profile)
}
for contactProfile in Array(identity.contactProfiles) {
backgroundContext.delete(contactProfile)
}
for cr in Array(identity.contactRequests) {
backgroundContext.delete(cr)
}
for payment in Array(identity.dashpayPayments) {
backgroundContext.delete(payment)
}
for ignored in Array(identity.dashpayIgnoredSenders) {
backgroundContext.delete(ignored)
}
}
blocking: Imported identity persists derivation breadcrumb only for the master probe key — all signed state transitions fail post-import
packages/rs-platform-wallet/src/wallet/identity/network/loading.rs (line 253)
Verified at HEAD. load_identity_by_index_inner emits a derivation breadcrumb only for the single on-chain key whose hash matched the load/discovery probe, by calling managed.add_key(pub_key, Some((wallet_id, identity_index, MASTER_KEY_INDEX)), ...) at lines 259-263. The identity's other on-chain public keys are persisted via add_identity at 241-247 with no derivation indices, and Swift explicitly clears row.privateKeyKeychainIdentifier for entries whose FFI projection has no derivation indices (PlatformWalletPersistenceHandler.swift:1584-1597). Document-write flows then select the first HIGH/CRITICAL authentication key and intentionally exclude MASTER — dpns.rs:206-220 enforces [SecurityLevel::HIGH, SecurityLevel::CRITICAL] because DPP rejects MASTER on document-side state transitions, and profile.rs follows the same key-selection contract. For an imported identity whose probe matched the master slot but whose document key is a separate HIGH/CRITICAL key, the signer is asked to sign with a public key that has no stored or derivable private material, and KeychainSigner returns 'No PersistentPublicKey row matches the supplied public-key bytes' — exactly the symptom now tracked in docs/dashpay/TODO.md (Verification & hygiene → '🐛 BUG (found in UAT 2026-06-19)'). The bug is genuinely in this PR's diff and breaks DPNS register, DashPay profile set, contact requests, and payments for every imported identity; the in-app create flow works only because it derives and persists breadcrumbs for every key it registers. The load/import path needs to derive and persist breadcrumbs for each wallet-derived on-chain authentication key it may later select for signing, not just the MASTER probe key.
suggestion: established_contact_get_note leaves *out_note undefined on every early-return path
packages/rs-platform-wallet-ffi/src/established_contact.rs (line 156)
Verified at HEAD. Sibling established_contact_get_alias (102-117) writes *out_alias = std::ptr::null_mut(); at line 108 immediately after check_ptr!, establishing the module's C-ABI contract that out-params are defined-null on early returns. established_contact_get_note only calls check_ptr!(out_note) and skips the pre-null write. Early returns from unwrap_option_or_return! (storage NotFound, contact.note == None) and unwrap_result_or_return! (CString::new failure on interior NUL — reachable from on-chain note content) leave *out_note indeterminate across the ABI. A C/Swift caller that doesn't pre-zero the slot can read a stale pointer or pass it to platform_wallet_string_free, risking invalid-free or double-free. The exported C ABI must not depend on caller hygiene — mirror the sibling pattern.
#[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);
*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: Pending incoming requests fetch attacker-controlled avatar URLs before user consent
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift (line 119)
Verified at HEAD. sync_contact_profiles (profile.rs:640-650) explicitly includes managed.incoming_contact_requests.keys() in the per-owner fetch plan, populating the contact-profile cache for senders the victim has never accepted. IncomingRequestRow (119-129) feeds cachedProfile(row.contactIdentityId)?.avatarUrl into DashPayAvatarView, which uses SwiftUI AsyncImage — opening the Requests tab issues an HTTPS GET to a sender-controlled host on first render, leaking IP, online state, TLS/UA fingerprint, and per-victim URL-token receipt confirmation before any consent action. With this PR's PersistentDashpayContactProfile SwiftData persistence the channel survives restart; Rust-side is_valid_avatar_url only validates scheme/length. The Ignore action mutes only after first render. Either suppress remote avatars Swift-side for unaccepted senders (initials-only variant for the Requests tab) or strip avatarUrl from the Rust→Swift projection until the sender becomes an established contact.
suggestion: DashPay profile reader trio leaves *out_profile / *out_has_profile undefined on early-return paths
packages/rs-platform-wallet-ffi/src/dashpay_profile.rs (line 90)
Verified at HEAD. managed_identity_get_dashpay_profile (90), platform_wallet_get_dashpay_profile (116), and platform_wallet_get_contact_profile (153) only assign *out_profile / *out_has_profile inside the Some/None match arms (101-110, 134-143, 174-183). After check_ptr!, early returns from unwrap_result_or_return!(read_identifier(..)) (invalid 32-byte input at 125, 163-164) and unwrap_option_or_return! (handle NotFound, missing managed identity, missing contact entry at 100, 132-133, 173) leave both out pointers indeterminate. DashPayProfileFFI owns four *mut c_char fields released by dashpay_profile_ffi_free — a subsequent free on a slot the caller didn't pre-zero would free garbage pointers (invalid-free or double-free). Pre-write *out_profile = DashPayProfileFFI::empty(); *out_has_profile = false; immediately after check_ptr! in each function, matching the established_contact_get_alias precedent at line 108.
suggestion: Confirmed-absent contact profiles cannot cross the Swift persistence boundary — stale rows resurrect on cold start
packages/rs-platform-wallet-ffi/src/identity_persistence.rs (line 587)
Verified at HEAD. allocate_contact_profile_rows explicitly continues past entries with profile == None (comment: 'Skip confirmed-absent entries — the negative cache is not persisted; it rebuilds on the next sweep'). The FFI projection carries no removal intent for absences; Swift's upsertDashpayContactProfiles is append/refresh-only; and ContactProfileRestoreEntryFFI rehydrates every persisted row as ContactProfileEntry { profile: Some(..) }. When a contact removes their on-chain dashpay.profile, apply_fetched_profile (profile.rs:594-602) flips the in-memory entry to None, but the absence is dropped at the FFI projection — the stale PersistentDashpayContactProfile row survives on disk. On cold start the cache rehydrates as Some(stale_profile) and the UI re-issues GETs to the previously attacker-controlled avatarUrl. Project absences as an explicit delete intent across the boundary (e.g. removed_contact_profile_ids: *const [u8;32] + count), mirroring the ContactIgnoredSenderFFI removal-set shape this PR already establishes on the same callback.
suggestion: rs-sdk-ffi forms a Rust slice over caller-supplied xpub bytes before enforcing the fixed 69-byte contract
packages/rs-sdk-ffi/src/dashpay/contact_request.rs (line 250)
DashSDKContactRequestParams now documents extended_public_key as exactly the 69-byte DIP-15 compact xpub, and the underlying Rust SDK validates the length after the callback returns. The C ABI entry points, however, only reject null/zero length and then call std::slice::from_raw_parts(params.extended_public_key, params.extended_public_key_len).to_vec() (≈ lines 317-319 and again in send_contact_request ≈ 563-565) before enforcing the scalar length. A stale or buggy foreign caller can therefore make Rust read an arbitrary-length foreign buffer before the validation error path runs. Reject params.extended_public_key_len != 69 at the C ABI boundary, before forming the slice, so caller-controlled length cannot drive an out-of-bounds read.
suggestion: Contact-profile checked_at_ms not durably persisted on unchanged refresh — negative-cache backoff does not survive restart
packages/rs-platform-wallet/src/wallet/identity/network/profile.rs (line 588)
Verified at HEAD. apply_fetched_profile always refreshes checked_at_ms = now_ms but returns changed only when the stored profile value differs (equality compares only profile content). sync_contact_profiles calls persister.store only when any_changed is true. A re-check confirming the same present or confirmed-absent profile updates the timestamp in memory only — the FFI persistence callback never fires. After cold start, restored rows carry stale checked_at_ms, should_fetch_profile immediately re-issues an In query for every unchanged contact, and the negative-cache backoff effectively doesn't survive restart. This amplifies the pre-consent avatar tracking channel (restart re-issues per-victim GETs to known-stable hosts) and creates a cold-start fetch storm; a failed earlier store is also not retried because later sweeps see changed = false. Return a separate needs_persist signal from apply_fetched_profile for timestamp-only refreshes, with coarse coalescing if write-amp matters.
suggestion: Paginated retrieve-all has no per-call total or elapsed budget — algorithmic-DoS surface against the recurring sync loop
packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs (line 42)
Verified at HEAD. fetch_contact_requests_paginated drains every matching document at CONTACT_REQUEST_PAGE_SIZE per page with no per-call total cap, no elapsed-time budget, and no caller-visible continuation. On cold start, restore-from-seed, or any after_created_at == None path it walks the entire history in one call. unignore_sender resets high_water_received_ms to None, so any UI un-ignore drops the next sweep onto the full retrieve-all. Combined with the recurring DashPaySyncManager tick, an attacker who spams contactRequest documents (varying accountReference to defeat per-request dedup) against one of the wallet's identities can make every recurring sweep monopolize the SDK gRPC client and starve every other identity's sync — a cheap algorithmic-DoS against the verified-proof query path. Add a configurable total-document ceiling per sweep with a resumable cursor, or at minimum a logged-warn + early-break above a threshold.
suggestion: address_display_network cannot return Regtest for base58 inputs — regtest wallets render bech32m platform addresses with the testnet 'tb' HRP
packages/rs-platform-wallet-ffi/src/persistence.rs (line 2475)
Verified at HEAD. The helper probes Mainnet, then Testnet, falling back to Regtest. For base58 Dash addresses (the input here is the dashcore::Address from AddressInfo), testnet/regtest/devnet share the same base58 version prefix, so is_valid_for_network(Testnet) returns true for regtest input and the Regtest fallback at line 2482 is unreachable. build_core_address_entry_ffi then feeds the mis-derived network into PlatformAddress::to_bech32m_string, so regtest wallets get their bech32m platform-payment addresses rendered with HRP 'tb' instead of 'dsrt' — exactly the inverse of the doc-comment at 2471-2474. Security impact is bounded (regtest is dev-only), but operators copying these addresses into a regtest-aware tool see an HRP mismatch and may interact with the wrong network domain. Either thread the originating network from WalletInfo (which carries it) instead of probing, or amend the doc and remove the unreachable arm.
🤖 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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:3471-3487: Wallet wipe omits contactProfiles — SwiftData fatal aborts the wipe and leaves attacker-controlled DashPay strings on disk
Verified at HEAD adb7b32e. PHASE 1 of the wallet-wipe routine pre-deletes dpnsNames, dashpayProfile, contactRequests, dashpayPayments, and dashpayIgnoredSenders, but not identity.contactProfiles. The in-source comment block at 3438-3470 documents the exact invariant this loop is built to enforce: SwiftData fatals during save() when a parent delete must null out a non-optional inverse on a child in the same save batch, so every cascade-child whose inverse is non-optional must be deleted in its own save() ahead of the parent. PersistentIdentity.contactProfiles is cascade and PersistentDashpayContactProfile.owner is non-optional — exactly that shape. profile.rs sync_contact_profiles explicitly includes managed.incoming_contact_requests.keys() in the per-owner fetch plan (lines 640-650), so any wallet whose recurring DashPay sync has cached even one PersistentDashpayContactProfile row (established contacts OR unsolicited senders) will hit the documented fatal during PHASE 2's identity delete (3493-3496), abort the user-initiated wipe, and leave cached PersistentDashpayContactProfile rows — including sender-controlled avatarUrl, displayName, and publicMessage strings — on disk. A user who wipes precisely to scrub a stalker's footprint actually retains it.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/network/loading.rs`:253-263: Imported identity persists derivation breadcrumb only for the master probe key — all signed state transitions fail post-import
Verified at HEAD. load_identity_by_index_inner emits a derivation breadcrumb only for the single on-chain key whose hash matched the load/discovery probe, by calling managed.add_key(pub_key, Some((wallet_id, identity_index, MASTER_KEY_INDEX)), ...) at lines 259-263. The identity's other on-chain public keys are persisted via add_identity at 241-247 with no derivation indices, and Swift explicitly clears row.privateKeyKeychainIdentifier for entries whose FFI projection has no derivation indices (PlatformWalletPersistenceHandler.swift:1584-1597). Document-write flows then select the first HIGH/CRITICAL authentication key and intentionally exclude MASTER — dpns.rs:206-220 enforces `[SecurityLevel::HIGH, SecurityLevel::CRITICAL]` because DPP rejects MASTER on document-side state transitions, and profile.rs follows the same key-selection contract. For an imported identity whose probe matched the master slot but whose document key is a separate HIGH/CRITICAL key, the signer is asked to sign with a public key that has no stored or derivable private material, and KeychainSigner returns 'No PersistentPublicKey row matches the supplied public-key bytes' — exactly the symptom now tracked in docs/dashpay/TODO.md (Verification & hygiene → '🐛 BUG (found in UAT 2026-06-19)'). The bug is genuinely in this PR's diff and breaks DPNS register, DashPay profile set, contact requests, and payments for every imported identity; the in-app create flow works only because it derives and persists breadcrumbs for every key it registers. The load/import path needs to derive and persist breadcrumbs for each wallet-derived on-chain authentication key it may later select for signing, not just the MASTER probe key.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/established_contact.rs`:156-169: established_contact_get_note leaves *out_note undefined on every early-return path
Verified at HEAD. Sibling established_contact_get_alias (102-117) writes `*out_alias = std::ptr::null_mut();` at line 108 immediately after check_ptr!, establishing the module's C-ABI contract that out-params are defined-null on early returns. established_contact_get_note only calls check_ptr!(out_note) and skips the pre-null write. Early returns from unwrap_option_or_return! (storage NotFound, contact.note == None) and unwrap_result_or_return! (CString::new failure on interior NUL — reachable from on-chain note content) leave *out_note indeterminate across the ABI. A C/Swift caller that doesn't pre-zero the slot can read a stale pointer or pass it to platform_wallet_string_free, risking invalid-free or double-free. The exported C ABI must not depend on caller hygiene — mirror the sibling pattern.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift`:119-129: Pending incoming requests fetch attacker-controlled avatar URLs before user consent
Verified at HEAD. sync_contact_profiles (profile.rs:640-650) explicitly includes managed.incoming_contact_requests.keys() in the per-owner fetch plan, populating the contact-profile cache for senders the victim has never accepted. IncomingRequestRow (119-129) feeds cachedProfile(row.contactIdentityId)?.avatarUrl into DashPayAvatarView, which uses SwiftUI AsyncImage — opening the Requests tab issues an HTTPS GET to a sender-controlled host on first render, leaking IP, online state, TLS/UA fingerprint, and per-victim URL-token receipt confirmation before any consent action. With this PR's PersistentDashpayContactProfile SwiftData persistence the channel survives restart; Rust-side is_valid_avatar_url only validates scheme/length. The Ignore action mutes only after first render. Either suppress remote avatars Swift-side for unaccepted senders (initials-only variant for the Requests tab) or strip avatarUrl from the Rust→Swift projection until the sender becomes an established contact.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:90-185: DashPay profile reader trio leaves *out_profile / *out_has_profile undefined on early-return paths
Verified at HEAD. managed_identity_get_dashpay_profile (90), platform_wallet_get_dashpay_profile (116), and platform_wallet_get_contact_profile (153) only assign *out_profile / *out_has_profile inside the Some/None match arms (101-110, 134-143, 174-183). After check_ptr!, early returns from unwrap_result_or_return!(read_identifier(..)) (invalid 32-byte input at 125, 163-164) and unwrap_option_or_return! (handle NotFound, missing managed identity, missing contact entry at 100, 132-133, 173) leave both out pointers indeterminate. DashPayProfileFFI owns four *mut c_char fields released by dashpay_profile_ffi_free — a subsequent free on a slot the caller didn't pre-zero would free garbage pointers (invalid-free or double-free). Pre-write `*out_profile = DashPayProfileFFI::empty(); *out_has_profile = false;` immediately after check_ptr! in each function, matching the established_contact_get_alias precedent at line 108.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/identity_persistence.rs`:587-621: Confirmed-absent contact profiles cannot cross the Swift persistence boundary — stale rows resurrect on cold start
Verified at HEAD. allocate_contact_profile_rows explicitly continues past entries with profile == None (comment: 'Skip confirmed-absent entries — the negative cache is not persisted; it rebuilds on the next sweep'). The FFI projection carries no removal intent for absences; Swift's upsertDashpayContactProfiles is append/refresh-only; and ContactProfileRestoreEntryFFI rehydrates every persisted row as ContactProfileEntry { profile: Some(..) }. When a contact removes their on-chain dashpay.profile, apply_fetched_profile (profile.rs:594-602) flips the in-memory entry to None, but the absence is dropped at the FFI projection — the stale PersistentDashpayContactProfile row survives on disk. On cold start the cache rehydrates as Some(stale_profile) and the UI re-issues GETs to the previously attacker-controlled avatarUrl. Project absences as an explicit delete intent across the boundary (e.g. removed_contact_profile_ids: *const [u8;32] + count), mirroring the ContactIgnoredSenderFFI removal-set shape this PR already establishes on the same callback.
- [SUGGESTION] In `packages/rs-sdk-ffi/src/dashpay/contact_request.rs`:250-319: rs-sdk-ffi forms a Rust slice over caller-supplied xpub bytes before enforcing the fixed 69-byte contract
DashSDKContactRequestParams now documents extended_public_key as exactly the 69-byte DIP-15 compact xpub, and the underlying Rust SDK validates the length after the callback returns. The C ABI entry points, however, only reject null/zero length and then call `std::slice::from_raw_parts(params.extended_public_key, params.extended_public_key_len).to_vec()` (≈ lines 317-319 and again in send_contact_request ≈ 563-565) before enforcing the scalar length. A stale or buggy foreign caller can therefore make Rust read an arbitrary-length foreign buffer before the validation error path runs. Reject `params.extended_public_key_len != 69` at the C ABI boundary, before forming the slice, so caller-controlled length cannot drive an out-of-bounds read.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/identity/network/profile.rs`:588-725: Contact-profile checked_at_ms not durably persisted on unchanged refresh — negative-cache backoff does not survive restart
Verified at HEAD. apply_fetched_profile always refreshes checked_at_ms = now_ms but returns `changed` only when the stored profile value differs (equality compares only profile content). sync_contact_profiles calls persister.store only when any_changed is true. A re-check confirming the same present or confirmed-absent profile updates the timestamp in memory only — the FFI persistence callback never fires. After cold start, restored rows carry stale checked_at_ms, should_fetch_profile immediately re-issues an `In` query for every unchanged contact, and the negative-cache backoff effectively doesn't survive restart. This amplifies the pre-consent avatar tracking channel (restart re-issues per-victim GETs to known-stable hosts) and creates a cold-start fetch storm; a failed earlier store is also not retried because later sweeps see `changed = false`. Return a separate needs_persist signal from apply_fetched_profile for timestamp-only refreshes, with coarse coalescing if write-amp matters.
- [SUGGESTION] In `packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs`:42-112: Paginated retrieve-all has no per-call total or elapsed budget — algorithmic-DoS surface against the recurring sync loop
Verified at HEAD. fetch_contact_requests_paginated drains every matching document at CONTACT_REQUEST_PAGE_SIZE per page with no per-call total cap, no elapsed-time budget, and no caller-visible continuation. On cold start, restore-from-seed, or any after_created_at == None path it walks the entire history in one call. unignore_sender resets high_water_received_ms to None, so any UI un-ignore drops the next sweep onto the full retrieve-all. Combined with the recurring DashPaySyncManager tick, an attacker who spams contactRequest documents (varying accountReference to defeat per-request dedup) against one of the wallet's identities can make every recurring sweep monopolize the SDK gRPC client and starve every other identity's sync — a cheap algorithmic-DoS against the verified-proof query path. Add a configurable total-document ceiling per sweep with a resumable cursor, or at minimum a logged-warn + early-break above a threshold.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs`:2475-2484: address_display_network cannot return Regtest for base58 inputs — regtest wallets render bech32m platform addresses with the testnet 'tb' HRP
Verified at HEAD. The helper probes Mainnet, then Testnet, falling back to Regtest. For base58 Dash addresses (the input here is the dashcore::Address from AddressInfo), testnet/regtest/devnet share the same base58 version prefix, so is_valid_for_network(Testnet) returns true for regtest input and the Regtest fallback at line 2482 is unreachable. build_core_address_entry_ffi then feeds the mis-derived network into PlatformAddress::to_bech32m_string, so regtest wallets get their bech32m platform-payment addresses rendered with HRP 'tb' instead of 'dsrt' — exactly the inverse of the doc-comment at 2471-2474. Security impact is bounded (regtest is dev-only), but operators copying these addresses into a regtest-aware tool see an HRP mismatch and may interact with the wrong network domain. Either thread the originating network from WalletInfo (which carries it) instead of probing, or amend the doc and remove the unreachable arm.
Inline dry-run could not load the GitHub PR diff because this PR exceeds GitHub's 20,000-line diff limit, so I posted the same verified findings as a top-level review body.
3 participants
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