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)
|
|
✅ Review complete (commit 7b94eab) |
![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% 50.09% -21.11%
=============================================
Files 20 10 -10
Lines 2837 1599 -1238
=============================================
- Hits 2020 801 -1219
+ Misses 817 798 -19
🚀 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.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
All 8 prior blockers re-verified STILL VALID at 03ba3ee; the latest delta (zeroize seed at FFI boundary + ContactXpubData embedding CompactXpub) introduces one new compile-breaking defect — create_wallet_from_seed_bytes now takes &[u8; 64] but tests/spv_sync.rs and examples/basic_usage.rs (plus both shielded_sync examples) still pass by value. One suggestion-grade observation: the same commit's *seed_bytes deref at wallet_lifecycle.rs:110 materializes a non-zeroized [u8;64] stack copy that contradicts the new doc-comment invariant. Recommending REQUEST_CHANGES.
🔴 9 blocking | 🟡 1 suggestion(s)
10 additional finding(s)
blocking: V001 migration edited in place — upgrades from v4.0.0-beta.4 won't apply the new column/table
packages/rs-platform-wallet-storage/migrations/V001__initial.rs (line 178)
contacts.payment_channel_broken (line 189) and CREATE TABLE rejected_contact_requests (lines 203-212) are inlined into V001, and the migrations directory contains only V001__initial.rs — no V002. V001 without these additions already shipped in v4.0.0-beta.4. Refinery records per-migration checksums in refinery_schema_history, so against an existing beta.4 DB this either aborts on divergent checksum or skips V001 as already-applied and never creates the new column/table. The first runtime write to payment_channel_broken (mark_contact_channel_broken) or INSERT INTO rejected_contact_requests (G5 tombstone) then fails at SQLite, disabling both the payment-channel-broken safety state AND the G5 rejection tombstone on every upgraded wallet — a once-rejected, abusive sender's still-immutable on-chain contactRequest is no longer suppressed on relaunch. Add a V002 migration with ALTER TABLE contacts ADD COLUMN payment_channel_broken INTEGER plus CREATE TABLE rejected_contact_requests (...) and leave V001 byte-stable.
blocking: record_rejected_contact_request drops the incoming map entry but never persists the row deletion via the SQLite contacts writer
packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs (line 133)
Line 140 removes the entry from incoming_contact_requests, but the returned ContactChangeSet populates only cs.rejected (lines 151-153) — cs.removed_incoming is left empty. The unified SQLite contacts writer DELETEs incoming rows only when cs.removed_incoming is non-empty; the cs.rejected block only writes the tombstone table row. The persisted state='received' row with its stale incoming_request blob therefore stays in SQLite. After a restart the load path reinserts it as a live incoming entry; the sync sweep's tombstone check only suppresses on-chain re-ingest, not local rehydration. The Swift persister masks this in SwiftData, but the Rust SQLite path used in non-Swift hosts and in tests leaves the row behind, silently undoing the user's reject — and the on-platform contactRequest document is immutable, so the local tombstone + delete is the only suppression mechanism against an abusive sender. Emit a matching ReceivedContactRequestKey { owner_id, sender_id } into cs.removed_incoming alongside cs.rejected.
let owner_id = self.id();
self.incoming_contact_requests.remove(sender_id);
let tombstone = RejectedContactRequest {
owner_id,
sender_id: *sender_id,
account_reference,
document_id,
};
self.rejected_contact_requests
.insert((*sender_id, account_reference), tombstone.clone());
let mut cs = ContactChangeSet::default();
cs.removed_incoming.insert(ReceivedContactRequestKey {
owner_id,
sender_id: *sender_id,
});
cs.rejected
.insert((owner_id, *sender_id, account_reference), tombstone);
cs
blocking: Rejected tombstones are written but never restored on rehydration — FFI restore record has no rejected-tombstone field
packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs (line 281)
IdentityRestoreEntryFFI carries dpns_names, contested_dpns_names, keys, contacts, payments, but no rejected-tombstone field. The persist side writes rejected tombstones into the rejected_contact_requests SQLite table (and Swift consumes them by deleting the visible incoming row), but the reverse Swift→Rust load ABI has no slot to surface persisted tombstones — restore_dashpay_contacts reconstructs sent/incoming/established only, leaving ManagedIdentity.rejected_contact_requests empty after rehydration. The next DashPay sweep then fetches the immutable rejected contactRequest, the in-memory suppression set is empty (is_request_rejected never fires), and the request resurfaces — defeating the user's explicit reject after every relaunch. Because the on-platform document is immutable, this local tombstone is the only suppression vector against an abusive sender. Add a #[repr(C)] RejectedContactRequestFFI row type plus rejected_tombstones / rejected_tombstones_count on IdentityRestoreEntryFFI, a Rust loader that writes the rows into managed.rejected_contact_requests, and a Swift producer that surfaces persisted tombstone rows.
blocking: Transient DAPI failures inside register_external_contact_account are classified as permanent payment-channel-broken
packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs (line 913)
build_contact_accounts treats any error from register_external_contact_account as permanent and calls mark_contact_channel_broken. The callee performs an additional network-backed Identity::fetch plus decryption work, wrapping DAPI failures as PlatformWalletError::InvalidIdentityData; earlier pre-fetches do not short-circuit this second internal fetch. A single transient DAPI hiccup permanently disables the payment channel; subsequent sweeps skip the contact via the payment_channel_broken filter, and recovery requires the counterparty to send a superseding contactRequest. A flaky or malicious DAPI endpoint becomes a persistent denial-of-payment primitive against a chosen contact, also reachable by routine network flakiness. Either thread the pre-fetched contact identity into register_external_contact_account to eliminate the second fetch, or scope permanent-broken classification to genuinely non-recoverable failures (decrypt/decode, missing-key, key-type/disabled-key) by matching on the PlatformWalletError variant.
blocking: purpose_mismatch flag stays sticky when a non-purpose hard error is also present
packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs (line 49)
add_error (lines 50-53) does not clear purpose_mismatch; add_purpose_error (lines 58-62) sets it unconditionally; merge (lines 70-79) ORs the flag. The struct's documented contract is purpose_mismatch = true only when key-purpose is the SOLE reason for invalidity, and build_contact_accounts uses that flag as the load-bearing predicate for retry-forever vs mark-broken. A request whose key has both a wrong key-TYPE (hard, permanent) and a wrong purpose ends up is_valid=false, purpose_mismatch=true, so the caller retries forever instead of marking broken — the opposite of intent, and an attacker-controllable resource-amplification path where a crafted recipient identity keeps a victim's sync sweep doing repeated DAPI work indefinitely. Fix add_error to clear the flag, add_purpose_error to only set it when no prior hard error exists, and merge to track hard-error presence on both sides.
pub fn add_error(&mut self, error: String) {
self.errors.push(error);
self.is_valid = false;
self.purpose_mismatch = false;
}
pub fn add_purpose_error(&mut self, error: String) {
let already_has_hard_error = !self.errors.is_empty() && !self.purpose_mismatch;
self.errors.push(error);
self.is_valid = false;
self.purpose_mismatch = !already_has_hard_error;
}
pub fn add_warning(&mut self, warning: String) {
self.warnings.push(warning);
}
pub fn merge(&mut self, other: ContactRequestValidation) {
let self_has_hard_error = !self.errors.is_empty() && !self.purpose_mismatch;
let other_has_hard_error = !other.errors.is_empty() && !other.purpose_mismatch;
self.errors.extend(other.errors);
self.warnings.extend(other.warnings);
if !other.is_valid {
self.is_valid = false;
}
self.purpose_mismatch =
!self_has_hard_error && !other_has_hard_error && (self.purpose_mismatch || other.purpose_mismatch);
}
blocking: Wallet-delete PHASE 1 omits PersistentDashpayPayment children — SwiftData fatal guaranteed on wipes with payment history
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 3042)
PHASE 1 iterates dpnsNames, dashpayProfile, and contactRequests only — not dashpayPayments. PersistentDashpayPayment.owner is non-optional and PersistentIdentity.dashpayPayments is its cascading inverse, populated by this PR's restore_dashpay_payments path on load and on every refresh. The surrounding comment at lines 3025-3041 explicitly documents that PHASE 1 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 dashpayPayments. Before this PR the failure mode was speculative; now it is guaranteed for any wallet ever exercised through DashPay payments. The fatal aborts PHASE 2 save() before the wallet row is removed; the user believes their data was wiped while plaintext memo + counterparty id + amount + txid rows remain on disk — confidentiality regression on device handoff/disposal. Add a dashpayPayments loop alongside the existing children.
for identity in identitiesToDelete {
for name in Array(identity.dpnsNames) {
backgroundContext.delete(name)
}
if let profile = identity.dashpayProfile {
backgroundContext.delete(profile)
}
for cr in Array(identity.contactRequests) {
backgroundContext.delete(cr)
}
for payment in Array(identity.dashpayPayments) {
backgroundContext.delete(payment)
}
}
try backgroundContext.save()
blocking: DashPaySyncManager cleanup unconditionally clears the cancel-token slot — stop/start race enables use-after-free across the C ABI
packages/rs-platform-wallet/src/manager/dashpay_sync.rs (line 214)
start() installs a fresh token at line 198 and spawns a dedicated OS thread that polls inside handle.block_on. On loop exit the cleanup arm at lines 230-232 writes *guard = None UNCONDITIONALLY — it never checks whether the token currently in the slot is still the one this thread was spawned with. Race: stop() (lines 246-255) cancels token A and take()s it; start() observes the slot empty, installs fresh token B, spawns thread B; thread A — still finishing its block_on — then reaches the cleanup arm and writes *guard = None, evicting token B's handle while thread B is alive. Any subsequent stop()/quiesce() sees no token to cancel/await and treats DashPay sync as quiesced. Cross-FFI consequence: the Swift persister and DashPay event callbacks close over a Swift-allocated UnsafePointer<Context>. Once the visible token is cleared while a stale thread is alive, the host treats dashpay_sync_manager_destroy as safe and runs Context.deallocate(), while the surviving thread continues invoking PersistenceCallbacks / event-handler function pointers against the freed Swift pointer — concrete use-after-free across the C ABI, reachable by normal Swift gestures (DashPay tab toggle, login/logout, identity-picker switch). The sibling identity and shielded sync managers already use generation/Arc::ptr_eq guards; mirror that here by capturing the spawned token in the closure and only clearing the slot when it still holds the same token.
blocking: Disabled encryption keys are still selectable — contactInfo rootEncryptionKeyIndex and contactRequest recipient selector both miss disabled_at
packages/rs-platform-wallet/src/wallet/identity/network/contact_info.rs (line 347)
contact_info.rs:347-354 selects the owner's first ECDSA_SECP256K1 ENCRYPTION key for rootEncryptionKeyIndex filtering only on purpose+key_type — no disabled_at check. The published contactInfo doc references that key id, so if an attacker holds the revoked root private material the public key id + derivation index lets them derive the same contactInfo AES keys and decrypt the owner's private alias / note / hidden metadata — a confidentiality exposure the disabled_at signal exists to prevent. Separately, select_recipient_key_index in contact_requests.rs:394-410 still picks the first matching DECRYPTION/ENCRYPTION key without disabled_at; with the pre-send validator hard-failing the send when the chosen key is disabled, a recipient with [disabled DECRYPTION + live ENCRYPTION] gets sends blocked rather than falling through to the live ENCRYPTION key — behavioral denial-of-contact regression. Add && k.disabled_at().is_none() to both selectors.
blocking: create_wallet_from_seed_bytes signature change breaks tests and examples — &[u8; 64] no longer matches by-value call sites
packages/rs-platform-wallet/tests/spv_sync.rs (line 183)
The latest delta changed wallet_lifecycle.rs:100-106 to take seed_bytes: &[u8; 64], but the in-repo call sites in tests/spv_sync.rs:184 (seed_bytes from mnemonic.to_seed("") passed by value), examples/basic_usage.rs:59-61 (also by value), examples/shielded_sync.rs:241, and examples/shielded_sync_paloma.rs:235 were not updated. Rust does not auto-borrow ordinary function arguments, so cargo test/cargo check on these targets fails with a type mismatch. The production call sites in payments.rs:460, payments.rs:795, dashpay_sync.rs:425, and wallet_lifecycle.rs:670/683 need to be reviewed too. Pass &seed_bytes (or &transparent_seed) at every stale call site.
suggestion: Zeroize-seed commit's `*seed_bytes` deref re-creates a non-zeroized [u8;64] stack copy, contradicting the doc-comment invariant
packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs (line 100)
The commit's stated intent (doc comment at lines 103-105) is that create_wallet_from_seed_bytes 'never owns a non-zeroized stack copy of the master secret — the only copy is the one consumed into key-wallet's Seed below'. But line 110 calls Wallet::from_seed_bytes(*seed_bytes, network, accounts) — the *seed_bytes deref on a [u8; 64] (which is Copy) materializes a fresh byte-for-byte copy in this caller's stack frame to pass by value into from_seed_bytes. That intermediate copy is not wrapped in Zeroizing and not explicitly cleared before this function returns; whether it survives the frame depends on stack reuse. The FFI-side Zeroizing wrapper still helps (the cross-FFI copy is zeroized), but the inner-Rust copy at this *deref site is the same defense-in-depth gap the commit was trying to close. Either change Wallet::from_seed_bytes to take &[u8; 64] (zeroizing internally), or wrap the local in Zeroizing here: let seed = zeroize::Zeroizing::new(*seed_bytes); Wallet::from_seed_bytes(*seed, ...).
🤖 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/migrations/V001__initial.rs`:178-212: V001 migration edited in place — upgrades from v4.0.0-beta.4 won't apply the new column/table
`contacts.payment_channel_broken` (line 189) and `CREATE TABLE rejected_contact_requests` (lines 203-212) are inlined into V001, and the migrations directory contains only `V001__initial.rs` — no V002. V001 without these additions already shipped in v4.0.0-beta.4. Refinery records per-migration checksums in `refinery_schema_history`, so against an existing beta.4 DB this either aborts on divergent checksum or skips V001 as already-applied and never creates the new column/table. The first runtime write to `payment_channel_broken` (`mark_contact_channel_broken`) or `INSERT INTO rejected_contact_requests` (G5 tombstone) then fails at SQLite, disabling both the payment-channel-broken safety state AND the G5 rejection tombstone on every upgraded wallet — a once-rejected, abusive sender's still-immutable on-chain contactRequest is no longer suppressed on relaunch. Add a V002 migration with `ALTER TABLE contacts ADD COLUMN payment_channel_broken INTEGER` plus `CREATE TABLE rejected_contact_requests (...)` and leave V001 byte-stable.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:133-155: record_rejected_contact_request drops the incoming map entry but never persists the row deletion via the SQLite contacts writer
Line 140 removes the entry from `incoming_contact_requests`, but the returned `ContactChangeSet` populates only `cs.rejected` (lines 151-153) — `cs.removed_incoming` is left empty. The unified SQLite contacts writer DELETEs incoming rows only when `cs.removed_incoming` is non-empty; the `cs.rejected` block only writes the tombstone table row. The persisted `state='received'` row with its stale `incoming_request` blob therefore stays in SQLite. After a restart the load path reinserts it as a live incoming entry; the sync sweep's tombstone check only suppresses on-chain re-ingest, not local rehydration. The Swift persister masks this in SwiftData, but the Rust SQLite path used in non-Swift hosts and in tests leaves the row behind, silently undoing the user's reject — and the on-platform contactRequest document is immutable, so the local tombstone + delete is the only suppression mechanism against an abusive sender. Emit a matching `ReceivedContactRequestKey { owner_id, sender_id }` into `cs.removed_incoming` alongside `cs.rejected`.
- [BLOCKING] In `packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs`:281-316: Rejected tombstones are written but never restored on rehydration — FFI restore record has no rejected-tombstone field
`IdentityRestoreEntryFFI` carries `dpns_names`, `contested_dpns_names`, `keys`, `contacts`, `payments`, but no rejected-tombstone field. The persist side writes rejected tombstones into the `rejected_contact_requests` SQLite table (and Swift consumes them by deleting the visible incoming row), but the reverse Swift→Rust load ABI has no slot to surface persisted tombstones — `restore_dashpay_contacts` reconstructs sent/incoming/established only, leaving `ManagedIdentity.rejected_contact_requests` empty after rehydration. The next DashPay sweep then fetches the immutable rejected contactRequest, the in-memory suppression set is empty (`is_request_rejected` never fires), and the request resurfaces — defeating the user's explicit reject after every relaunch. Because the on-platform document is immutable, this local tombstone is the only suppression vector against an abusive sender. Add a `#[repr(C)] RejectedContactRequestFFI` row type plus `rejected_tombstones` / `rejected_tombstones_count` on `IdentityRestoreEntryFFI`, a Rust loader that writes the rows into `managed.rejected_contact_requests`, and a Swift producer that surfaces persisted tombstone rows.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:913-933: Transient DAPI failures inside register_external_contact_account are classified as permanent payment-channel-broken
`build_contact_accounts` treats any error from `register_external_contact_account` as permanent and calls `mark_contact_channel_broken`. The callee performs an additional network-backed `Identity::fetch` plus decryption work, wrapping DAPI failures as `PlatformWalletError::InvalidIdentityData`; earlier pre-fetches do not short-circuit this second internal fetch. A single transient DAPI hiccup permanently disables the payment channel; subsequent sweeps skip the contact via the `payment_channel_broken` filter, and recovery requires the counterparty to send a superseding contactRequest. A flaky or malicious DAPI endpoint becomes a persistent denial-of-payment primitive against a chosen contact, also reachable by routine network flakiness. Either thread the pre-fetched contact identity into `register_external_contact_account` to eliminate the second fetch, or scope permanent-broken classification to genuinely non-recoverable failures (decrypt/decode, missing-key, key-type/disabled-key) by matching on the `PlatformWalletError` variant.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs`:49-79: purpose_mismatch flag stays sticky when a non-purpose hard error is also present
`add_error` (lines 50-53) does not clear `purpose_mismatch`; `add_purpose_error` (lines 58-62) sets it unconditionally; `merge` (lines 70-79) ORs the flag. The struct's documented contract is `purpose_mismatch = true` only when key-purpose is the SOLE reason for invalidity, and `build_contact_accounts` uses that flag as the load-bearing predicate for retry-forever vs mark-broken. A request whose key has both a wrong key-TYPE (hard, permanent) and a wrong purpose ends up `is_valid=false, purpose_mismatch=true`, so the caller retries forever instead of marking broken — the opposite of intent, and an attacker-controllable resource-amplification path where a crafted recipient identity keeps a victim's sync sweep doing repeated DAPI work indefinitely. Fix `add_error` to clear the flag, `add_purpose_error` to only set it when no prior hard error exists, and `merge` to track hard-error presence on both sides.
- [BLOCKING] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:3042-3053: Wallet-delete PHASE 1 omits PersistentDashpayPayment children — SwiftData fatal guaranteed on wipes with payment history
PHASE 1 iterates `dpnsNames`, `dashpayProfile`, and `contactRequests` only — not `dashpayPayments`. `PersistentDashpayPayment.owner` is non-optional and `PersistentIdentity.dashpayPayments` is its cascading inverse, populated by this PR's `restore_dashpay_payments` path on load and on every refresh. The surrounding comment at lines 3025-3041 explicitly documents that PHASE 1 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 `dashpayPayments`. Before this PR the failure mode was speculative; now it is guaranteed for any wallet ever exercised through DashPay payments. The fatal aborts PHASE 2 `save()` before the wallet row is removed; the user believes their data was wiped while plaintext memo + counterparty id + amount + txid rows remain on disk — confidentiality regression on device handoff/disposal. Add a `dashpayPayments` loop alongside the existing children.
- [BLOCKING] In `packages/rs-platform-wallet/src/manager/dashpay_sync.rs`:214-235: DashPaySyncManager cleanup unconditionally clears the cancel-token slot — stop/start race enables use-after-free across the C ABI
`start()` installs a fresh token at line 198 and spawns a dedicated OS thread that polls inside `handle.block_on`. On loop exit the cleanup arm at lines 230-232 writes `*guard = None` UNCONDITIONALLY — it never checks whether the token currently in the slot is still the one this thread was spawned with. Race: `stop()` (lines 246-255) cancels token A and `take()`s it; `start()` observes the slot empty, installs fresh token B, spawns thread B; thread A — still finishing its block_on — then reaches the cleanup arm and writes `*guard = None`, evicting token B's handle while thread B is alive. Any subsequent `stop()`/`quiesce()` sees no token to cancel/await and treats DashPay sync as quiesced. Cross-FFI consequence: the Swift persister and DashPay event callbacks close over a Swift-allocated `UnsafePointer<Context>`. Once the visible token is cleared while a stale thread is alive, the host treats `dashpay_sync_manager_destroy` as safe and runs `Context.deallocate()`, while the surviving thread continues invoking `PersistenceCallbacks` / event-handler function pointers against the freed Swift pointer — concrete use-after-free across the C ABI, reachable by normal Swift gestures (DashPay tab toggle, login/logout, identity-picker switch). The sibling identity and shielded sync managers already use generation/`Arc::ptr_eq` guards; mirror that here by capturing the spawned token in the closure and only clearing the slot when it still holds the same token.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/network/contact_info.rs`:347-354: Disabled encryption keys are still selectable — contactInfo rootEncryptionKeyIndex and contactRequest recipient selector both miss disabled_at
`contact_info.rs:347-354` selects the owner's first ECDSA_SECP256K1 ENCRYPTION key for `rootEncryptionKeyIndex` filtering only on purpose+key_type — no `disabled_at` check. The published contactInfo doc references that key id, so if an attacker holds the revoked root private material the public key id + derivation index lets them derive the same contactInfo AES keys and decrypt the owner's private alias / note / hidden metadata — a confidentiality exposure the `disabled_at` signal exists to prevent. Separately, `select_recipient_key_index` in `contact_requests.rs:394-410` still picks the first matching DECRYPTION/ENCRYPTION key without `disabled_at`; with the pre-send validator hard-failing the send when the chosen key is disabled, a recipient with [disabled DECRYPTION + live ENCRYPTION] gets sends blocked rather than falling through to the live ENCRYPTION key — behavioral denial-of-contact regression. Add `&& k.disabled_at().is_none()` to both selectors.
- [BLOCKING] In `packages/rs-platform-wallet/tests/spv_sync.rs`:183-191: create_wallet_from_seed_bytes signature change breaks tests and examples — &[u8; 64] no longer matches by-value call sites
The latest delta changed `wallet_lifecycle.rs:100-106` to take `seed_bytes: &[u8; 64]`, but the in-repo call sites in `tests/spv_sync.rs:184` (`seed_bytes` from `mnemonic.to_seed("")` passed by value), `examples/basic_usage.rs:59-61` (also by value), `examples/shielded_sync.rs:241`, and `examples/shielded_sync_paloma.rs:235` were not updated. Rust does not auto-borrow ordinary function arguments, so `cargo test`/`cargo check` on these targets fails with a type mismatch. The production call sites in `payments.rs:460`, `payments.rs:795`, `dashpay_sync.rs:425`, and `wallet_lifecycle.rs:670/683` need to be reviewed too. Pass `&seed_bytes` (or `&transparent_seed`) at every stale call site.
- [SUGGESTION] In `packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:100-117: Zeroize-seed commit's `*seed_bytes` deref re-creates a non-zeroized [u8;64] stack copy, contradicting the doc-comment invariant
The commit's stated intent (doc comment at lines 103-105) is that `create_wallet_from_seed_bytes` 'never owns a non-zeroized stack copy of the master secret — the only copy is the one consumed into key-wallet's `Seed` below'. But line 110 calls `Wallet::from_seed_bytes(*seed_bytes, network, accounts)` — the `*seed_bytes` deref on a `[u8; 64]` (which is `Copy`) materializes a fresh byte-for-byte copy in this caller's stack frame to pass by value into `from_seed_bytes`. That intermediate copy is not wrapped in `Zeroizing` and not explicitly cleared before this function returns; whether it survives the frame depends on stack reuse. The FFI-side `Zeroizing` wrapper still helps (the cross-FFI copy is zeroized), but the inner-Rust copy at this `*deref` site is the same defense-in-depth gap the commit was trying to close. Either change `Wallet::from_seed_bytes` to take `&[u8; 64]` (zeroizing internally), or wrap the local in `Zeroizing` here: `let seed = zeroize::Zeroizing::new(*seed_bytes); Wallet::from_seed_bytes(*seed, ...)`.
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.
Resolves the rs-sdk-ffi/src/sdk.rs conflict in favor of v3.1-dev's BigStackRuntime::build_shared() — it sets the same 8 MiB worker stack (WORKER_STACK_SIZE) that the branch added manually to fix the on-device GroveDB-proof SIGBUS, plus a 16 MiB blocking stack, and matches the renamed Arc<BigStackRuntime> runtime type. Same fix, canonical form. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… stop/start race The background loop's exit cleanup ran an unconditional `*guard = None`. A cancel-only `stop()` (takes + cancels loop A's token while A keeps draining its in-flight pass) followed by a quick `start()` (installs loop B's token) left loop A's later cleanup nulling loop B's *live* token — after which `is_running()` lies and a shutdown `stop()`/`quiesce()` silently no-ops while loop B keeps fanning out `persister.store(...)` through a freed FFI persister context (use-after-free). Fix: bump a monotonic `loop_generation` under the `background_cancel` lock on every install, and clear the stored token on exit only when the generation still matches (`clear_cancel_if_current`) — the newest loop is the only one that may clear, and only while it is still the newest. Extracted `install_cancel`/`clear_cancel_if_current` so the race is deterministically testable (the real loop runs on an OS thread under `Handle::block_on`, whose exit can't be timed reliably). Test would have caught this in CI: regression `stale_loop_cleanup_does_not_clobber_newer_loop_token` is ✖ with the unconditional clear, ✔ with the generation guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…transient failure
build_contact_accounts marked a contact's payment channel permanently
broken (G1c) on *any* error from register_external_contact_account — but
that method did an Identity::fetch().await internally, so a transient
DAPI blip was indistinguishable from a malformed request and killed
payments to the contact forever (recovery needs a rotation that may
never come).
Two changes:
- register_external_contact_account now takes the already-fetched
&Identity (both callers fetch it for the pre-ECDH key validation), so
it performs NO network I/O — eliminating the transient-DAPI path AND a
redundant second fetch of the same identity.
- It returns a typed RegisterExternalError::{Permanent,Transient}.
build_contact_accounts marks the channel broken only on Permanent
(malformed encrypted xpub / missing key — re-deriving won't help) and
leaves it intact on Transient (persistence/insert hiccup) for the next
sweep to retry.
Test would have caught this in CI: register_external_classifies_infra_miss_as_transient
is ✖ when the failure is flattened to all-permanent, ✔ with the split;
register_external_classifies_missing_key_as_permanent pins the other arm.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tion error ContactRequestValidation::add_purpose_error set purpose_mismatch=true even when a genuinely-permanent hard error (disabled / missing / wrong-type key) was also recorded. build_contact_accounts read the bare purpose_mismatch flag to downgrade the failure to a non-permanent skip (G15) — so a request that tripped BOTH a purpose mismatch AND a hard error was retried forever instead of marking the channel broken. Track non-purpose hard errors separately (hard_error flag, set by add_error and propagated through merge) and gate the skip on a new is_purpose_only() == purpose_mismatch && !hard_error. The build path now downgrades to a skip only when the purpose mismatch is the SOLE cause. Test would have caught this in CI: purpose_mismatch_with_hard_error_is_not_purpose_only is ✖ when is_purpose_only() == purpose_mismatch (old semantics), ✔ with the hard_error guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ay ECDH select_recipient_key_index and the contactInfo root-encryption-key selector both picked the first key matching purpose+type via a bare .find() with no disabled-key gate — so a revoked key could be chosen to receive the contact's DIP-15 compact xpub (or to encrypt contactInfo), handing that material to whoever holds the compromised private half. The sibling AUTHENTICATION selector already excludes disabled keys. Add `&& k.disabled_at().is_none()` to both selectors (mirrors the validator's disabled-key check). Test would have caught this in CI: skips_disabled_decryption_key_and_falls_back_to_enabled_encryption is ✖ without the filter (selects the disabled key id 0), ✔ with it (falls back to the enabled key id 1); errors_when_only_candidate_key_is_disabled pins the no-enabled-key case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…end-only V002 V001 ships in v4.0.0-beta.4 / rc.1 / rc.2. This branch had added the contacts.payment_channel_broken column and the rejected_contact_requests table directly into V001 — but refinery records a checksum of every applied migration, so editing V001 in place breaks upgrades for any DB that already ran it: the open aborts on a checksum mismatch, or the new DDL is silently skipped because V001 is already marked applied. Revert V001 to its released form and add the two additions in an append-only V002. The SQLite writers/readers (contacts.rs) are unchanged — the column/table exist after V002 runs, before any read/write. Test would have caught this in CI: v001_database_upgrades_to_v002_schema applies only V001 (asserts the column/table are ABSENT), then upgrades and asserts they're PRESENT — ✖ if the additions sit in V001 (present at the V001 target), ✔ with them in V002. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s raw pointers) DashpayPaymentFFI owns its txid/memo heap pointers (freed by dashpay_payment_array_free), but derived Copy/Clone, so a bitwise copy or clone would duplicate the raw pointers and freeing both halves would double-free. Nothing in the crate copied or cloned it (rows are built in place), so removing the derives is a pure safety hardening with no behavior change — no test (compile-time-only, no behavior delta). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… contacts don't resurrect A rejected contact reappeared after every relaunch. The Rust suppression set (ManagedIdentity.rejected_contact_requests) is in-memory and started empty on load — the FFI load path rebuilt contacts + payments but not the tombstones — so a previously-rejected sender's still-on-platform immutable contactRequest re-ingested on the next sync sweep and the contact came back. Rust (rs-platform-wallet-ffi): - Add a `rejected` array (reusing ContactRequestRejectionFFI) to IdentityRestoreEntryFFI, and restore_dashpay_rejected / apply_rejected_rows to rehydrate rejected_contact_requests keyed by (sender, accountReference) — the same suppression key the live reject path uses, so a ROTATED (bumped accountReference) request stays un-suppressed. Swift (swift-sdk): - New PersistentDashpayRejectedRequest SwiftData model (the SwiftData analog of the SQLite rejected_contact_requests table the example app's persister doesn't have), cascade-owned by PersistentIdentity, registered in the schema. persistContacts now upserts a tombstone row alongside deleting the incoming row; the load builder projects them into the new FFI array. Also surfaces SwiftData save() failures in persistDashpayPayments (was swallowed by `try?`): a dropped payment upsert silently loses Sent history + memos — the H1 symptom this path exists to prevent — so a failure must be observable. Test would have caught this in CI: restore_rejected_rows_rebuilds_suppression_set asserts a fresh identity suppresses nothing (the resurrection state), then that restore rehydrates the tombstones while a rotated reference stays un-suppressed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…UITest wait - AccountListView: gate the empty state on orderedAccounts (the FILTERED list actually rendered), not the raw accounts query. A wallet whose only rows are DashPay friendship accounts (tags 12/13, hidden) has a non-empty accounts but empty orderedAccounts, so it showed an empty Section instead of the 'No Accounts' state. - IdentityDetailView: enforce the HTTPS-only avatar rule in code (scheme-parse, reject non-https before save) instead of relying on the helper text — the DIP-15 avatar pipeline fetches the image to hash it, and a plaintext-http URL is a privacy leak and not reliably fetchable. - DashPayTabUITests: wait for the refresh button (waitForExistence) instead of an immediate .exists check that races the segment navigation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Add 'text' language tag to five bare code fences (MD040) across the DIP-spec, keywallet, swift-app, interop-desk-check, and contactInfo research notes. - Escape the literal pipe in `SHA256((y[31]&0x1\|0x2) ‖ x)` in the interop verdict table — the unescaped `|` split the row into an extra column. 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
Three blocking carried-forward issues remain at HEAD 1734fd9 and one defense-in-depth suggestion. The latest delta (03ba3ee..HEAD) successfully lands six of the prior fixes (V001/V002 split, FFI rejected-tombstone restore, transient/permanent contact-account classification, hard_error/is_purpose_only flag separation, loop_generation cancel-token guard, disabled_at filter on both ECDH selectors). No new defects introduced by this PR's own commits — the two 'new' codex findings (validate_update_v0, BigStackRuntime) both originate in upstream PRs #3868 / #3896 that arrived via the v3.1-dev merge, not in this PR's scope.
🔴 3 blocking | 🟡 1 suggestion(s)
4 additional finding(s)
blocking: record_rejected_contact_request never emits a removed_incoming entry — Rust SQLite writer leaves the visible 'received' row in place
packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs (line 133)
Confirmed at HEAD lines 138-154: the method removes the entry from incoming_contact_requests in memory and returns a ContactChangeSet whose only populated field is cs.rejected. The unified Rust SQLite contacts writer at packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:182-193 DELETEs from the contacts table only when cs.removed_incoming is non-empty; the cs.rejected block writes only the tombstone table row. The persisted state='received' row therefore stays in SQLite, and after a restart the load path reinserts it as a live incoming entry — the sync tombstone check only suppresses on-chain re-ingest, not local rehydration. The Swift persister masks this in SwiftData via a bespoke deleteRejectedIncomingRow call (PlatformWalletPersistenceHandler.swift:1810-1822), but the Rust SQLite path used in tests and any non-Swift host has no equivalent — the two backends diverge and the anti-harassment guarantee is silently undone on each relaunch. Emit a matching ReceivedContactRequestKey { owner_id, sender_id } into cs.removed_incoming alongside cs.rejected so both persistence backends converge on row deletion.
let owner_id = self.id();
self.incoming_contact_requests.remove(sender_id);
let tombstone = RejectedContactRequest {
owner_id,
sender_id: *sender_id,
account_reference,
document_id,
};
self.rejected_contact_requests
.insert((*sender_id, account_reference), tombstone.clone());
let mut cs = ContactChangeSet::default();
cs.removed_incoming.insert(ReceivedContactRequestKey {
owner_id,
sender_id: *sender_id,
});
cs.rejected
.insert((owner_id, *sender_id, account_reference), tombstone);
cs
blocking: Wallet-delete PHASE 1 omits dashpayPayments AND the newly-added dashpayRejectedRequests — guaranteed SwiftData fatal on wipe
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 3336)
Confirmed at HEAD lines 3336-3346. PHASE 1 iterates only dpnsNames, dashpayProfile, and contactRequests. The surrounding comment block (lines 3312-3334) 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 as its parent. Two non-optional cascade children of PersistentIdentity are missing from the loop: (a) dashpayPayments — PersistentDashpayPayment.owner: PersistentIdentity is non-optional with @Relationship(deleteRule: .cascade, inverse: \PersistentDashpayPayment.owner), populated by restore_dashpay_payments (H1) on every load/refresh; and (b) the new dashpayRejectedRequests — PersistentDashpayRejectedRequest.owner: PersistentIdentity is non-optional with the same cascade inverse, populated whenever the user rejects an incoming request (G5 stage 1). Either child reaching the wipe codepath guarantees a SwiftData fatal during PHASE 2 save() before the wallet row is deleted; the user believes their data was wiped while plaintext memo, counterparty id, amount, txid, and rejection-tombstone rows remain on disk — a confidentiality regression on device handoff/disposal. This worsened in the latest delta because dashpayRejectedRequests is newly added by this PR; before, only dashpayPayments was a concern.
for identity in identitiesToDelete {
for name in Array(identity.dpnsNames) {
backgroundContext.delete(name)
}
if let profile = identity.dashpayProfile {
backgroundContext.delete(profile)
}
for cr in Array(identity.contactRequests) {
backgroundContext.delete(cr)
}
for payment in Array(identity.dashpayPayments) {
backgroundContext.delete(payment)
}
for tomb in Array(identity.dashpayRejectedRequests) {
backgroundContext.delete(tomb)
}
}
try backgroundContext.save()
blocking: create_wallet_from_seed_bytes signature change breaks tests + examples — &[u8;64] doesn't match by-value call sites
packages/rs-platform-wallet/tests/spv_sync.rs (line 183)
Confirmed at HEAD: wallet_lifecycle.rs:100-106 takes seed_bytes: &[u8; 64], but tests/spv_sync.rs:186 (visible at line 186 here), examples/basic_usage.rs:61, examples/shielded_sync.rs:243, and examples/shielded_sync_paloma.rs:243 still pass the array by value. Rust does not auto-borrow ordinary function arguments, so cargo check -p platform-wallet --tests --examples fails with error[E0308]: mismatched types ... expected '&[u8; 64]', found '[u8; 64]'. The library build succeeds, so the regression is invisible from cargo build -p platform-wallet, but cargo test -p platform-wallet and any example build are broken. Production call sites in payments.rs, dashpay_sync.rs, and the in-tree duplicate test in wallet_lifecycle.rs were updated; the test/example sites were missed. Fix is mechanical: prefix with & at each remaining site.
let platform_wallet = manager
.create_wallet_from_seed_bytes(
network,
&seed_bytes,
WalletAccountCreationOptions::Default,
None,
)
.await
.expect("Failed to create platform wallet");
suggestion: Zeroize-seed deref still materializes a non-zeroized [u8;64] stack copy, contradicting the doc-comment invariant
packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs (line 100)
Confirmed at HEAD. The doc-comment at lines 103-105 states the method 'never owns a non-zeroized stack copy of the master secret — the only copy is the one consumed into key-wallet's Seed below'. Line 110 calls Wallet::from_seed_bytes(*seed_bytes, network, accounts). Because [u8; 64] is Copy, dereferencing &[u8; 64] materializes a fresh byte-for-byte copy in this caller's stack frame to pass by value into from_seed_bytes. That intermediate copy is not wrapped in Zeroizing and not explicitly cleared before return; whether the bytes survive the frame depends on stack reuse. The FFI-side Zeroizing wrapper at rs-platform-wallet-ffi/src/manager.rs still helps for the cross-FFI copy, but the inner-Rust copy at this * deref site is the same defense-in-depth gap the commit was trying to close. Either change Wallet::from_seed_bytes upstream in key-wallet to take &[u8; 64] and zeroize internally, or wrap the local in Zeroizing here.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:133-155: record_rejected_contact_request never emits a removed_incoming entry — Rust SQLite writer leaves the visible 'received' row in place
Confirmed at HEAD lines 138-154: the method removes the entry from `incoming_contact_requests` in memory and returns a `ContactChangeSet` whose only populated field is `cs.rejected`. The unified Rust SQLite contacts writer at `packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:182-193` DELETEs from the `contacts` table only when `cs.removed_incoming` is non-empty; the `cs.rejected` block writes only the tombstone table row. The persisted `state='received'` row therefore stays in SQLite, and after a restart the load path reinserts it as a live incoming entry — the sync tombstone check only suppresses on-chain re-ingest, not local rehydration. The Swift persister masks this in SwiftData via a bespoke `deleteRejectedIncomingRow` call (PlatformWalletPersistenceHandler.swift:1810-1822), but the Rust SQLite path used in tests and any non-Swift host has no equivalent — the two backends diverge and the anti-harassment guarantee is silently undone on each relaunch. Emit a matching `ReceivedContactRequestKey { owner_id, sender_id }` into `cs.removed_incoming` alongside `cs.rejected` so both persistence backends converge on row deletion.
- [BLOCKING] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:3336-3347: Wallet-delete PHASE 1 omits dashpayPayments AND the newly-added dashpayRejectedRequests — guaranteed SwiftData fatal on wipe
Confirmed at HEAD lines 3336-3346. PHASE 1 iterates only `dpnsNames`, `dashpayProfile`, and `contactRequests`. The surrounding comment block (lines 3312-3334) 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 as its parent. Two non-optional cascade children of `PersistentIdentity` are missing from the loop: (a) `dashpayPayments` — `PersistentDashpayPayment.owner: PersistentIdentity` is non-optional with `@Relationship(deleteRule: .cascade, inverse: \PersistentDashpayPayment.owner)`, populated by `restore_dashpay_payments` (H1) on every load/refresh; and (b) the new `dashpayRejectedRequests` — `PersistentDashpayRejectedRequest.owner: PersistentIdentity` is non-optional with the same cascade inverse, populated whenever the user rejects an incoming request (G5 stage 1). Either child reaching the wipe codepath guarantees a SwiftData fatal during PHASE 2 `save()` *before* the wallet row is deleted; the user believes their data was wiped while plaintext memo, counterparty id, amount, txid, and rejection-tombstone rows remain on disk — a confidentiality regression on device handoff/disposal. This worsened in the latest delta because `dashpayRejectedRequests` is newly added by this PR; before, only `dashpayPayments` was a concern.
- [BLOCKING] In `packages/rs-platform-wallet/tests/spv_sync.rs`:183-191: create_wallet_from_seed_bytes signature change breaks tests + examples — &[u8;64] doesn't match by-value call sites
Confirmed at HEAD: `wallet_lifecycle.rs:100-106` takes `seed_bytes: &[u8; 64]`, but `tests/spv_sync.rs:186` (visible at line 186 here), `examples/basic_usage.rs:61`, `examples/shielded_sync.rs:243`, and `examples/shielded_sync_paloma.rs:243` still pass the array by value. Rust does not auto-borrow ordinary function arguments, so `cargo check -p platform-wallet --tests --examples` fails with `error[E0308]: mismatched types ... expected '&[u8; 64]', found '[u8; 64]'`. The library build succeeds, so the regression is invisible from `cargo build -p platform-wallet`, but `cargo test -p platform-wallet` and any example build are broken. Production call sites in `payments.rs`, `dashpay_sync.rs`, and the in-tree duplicate test in `wallet_lifecycle.rs` were updated; the test/example sites were missed. Fix is mechanical: prefix with `&` at each remaining site.
- [SUGGESTION] In `packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:100-117: Zeroize-seed deref still materializes a non-zeroized [u8;64] stack copy, contradicting the doc-comment invariant
Confirmed at HEAD. The doc-comment at lines 103-105 states the method 'never owns a non-zeroized stack copy of the master secret — the only copy is the one consumed into key-wallet's `Seed` below'. Line 110 calls `Wallet::from_seed_bytes(*seed_bytes, network, accounts)`. Because `[u8; 64]` is `Copy`, dereferencing `&[u8; 64]` materializes a fresh byte-for-byte copy in this caller's stack frame to pass by value into `from_seed_bytes`. That intermediate copy is not wrapped in `Zeroizing` and not explicitly cleared before return; whether the bytes survive the frame depends on stack reuse. The FFI-side `Zeroizing` wrapper at `rs-platform-wallet-ffi/src/manager.rs` still helps for the cross-FFI copy, but the inner-Rust copy at this `*` deref site is the same defense-in-depth gap the commit was trying to close. Either change `Wallet::from_seed_bytes` upstream in key-wallet to take `&[u8; 64]` and zeroize internally, or wrap the local in `Zeroizing` here.
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.
…_metadata set_contact_metadata took alias/note/is_hidden as three loose positional args (two adjacent Option<String> — easy to transpose), and both call sites had a ContactInfoPrivateData in hand that they exploded into those args. Take the struct directly instead: - The recurring-sync caller owns its decrypted doc, so it now MOVES decrypted.data in — no per-field clone. - set_contact_info_with_external_signer builds the ContactInfoPrivateData once and feeds it to BOTH the local metadata apply and the wire encode_private_data, removing a duplicate struct literal. ContactInfoPrivateData is the decrypted contactInfo payload (no padding field — that's computed at encode time), so it IS the metadata; the wire names alias_name/display_hidden map onto the domain alias/is_hidden in the one place that writes onto EstablishedContact. No behavior change; the existing contactInfo/sync suite (249 tests) covers it. 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 a7930b5. All four prior findings re-verified STILL VALID against the worktree: (1) record_rejected_contact_request never emits removed_incoming, breaking the Rust SQLite anti-harassment path; (2) Swift wallet-wipe PHASE 1 omits the new dashpayPayments + dashpayRejectedRequests cascade children — guaranteed SwiftData save() fatal that leaves plaintext payment/tombstone rows on disk; (3) create_wallet_from_seed_bytes signature change to &[u8; 64] still breaks tests/spv_sync.rs:186 and three examples (basic_usage.rs:61, shielded_sync.rs:243, shielded_sync_paloma.rs:237); (4) the *seed_bytes deref at wallet_lifecycle.rs:110 still materializes a non-zeroized stack copy that contradicts the doc-comment invariant. Codex-ffi flagged an entropy gap on the create-only contact-request FFI result that is real but lower-priority. Requesting changes.
🔴 3 blocking | 🟡 2 suggestion(s)
5 additional finding(s)
blocking: record_rejected_contact_request never emits removed_incoming — rejected requests resurrect on the Rust SQLite backend
packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs (line 140)
Verified at HEAD a7930b5 lines 140-155. The method removes the in-memory incoming_contact_requests entry (line 141) and populates only cs.rejected (lines 152-154); cs.removed_incoming is left empty. The unified Rust SQLite contacts writer in packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs only DELETEs from the contacts table when cs.removed_incoming is non-empty; the cs.rejected branch writes only the tombstone table. The persisted state='received' row therefore stays in SQLite after a reject. On next load, the contacts loader rehydrates it as a live incoming entry — the sync-side tombstone check only suppresses on-chain re-ingest, not local rehydration of an already-stored row. The Swift persister masks this in SwiftData via a bespoke deleteRejectedIncomingRow call, but the Rust SQLite backend used by tests and any non-Swift host has no equivalent, so the two persistence backends silently diverge. Because the on-platform contactRequest document is immutable, this local tombstone+delete is the only suppression vector against a rejected sender; without removed_incoming, the user's explicit reject is silently undone on every relaunch.
let owner_id = self.id();
self.incoming_contact_requests.remove(sender_id);
let tombstone = RejectedContactRequest {
owner_id,
sender_id: *sender_id,
account_reference,
document_id,
};
self.rejected_contact_requests
.insert((*sender_id, account_reference), tombstone.clone());
let mut cs = ContactChangeSet::default();
cs.removed_incoming.insert(ReceivedContactRequestKey {
owner_id,
sender_id: *sender_id,
});
cs.rejected
.insert((owner_id, *sender_id, account_reference), tombstone);
cs
blocking: Wallet-delete PHASE 1 omits dashpayPayments and the newly-added dashpayRejectedRequests — guaranteed SwiftData fatal on wipe
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 3336)
Verified at HEAD a7930b5 lines 3336-3346. The surrounding comment (3312-3334) documents that PHASE 1 exists precisely because SwiftData fatals during save() when it has to null out a non-optional inverse on a child being processed in the same batch as its parent. The loop pre-deletes only dpnsNames, dashpayProfile, and contactRequests. This PR adds two more non-optional cascade children of PersistentIdentity: (a) PersistentDashpayPayment.owner: PersistentIdentity (non-optional, populated by every load via the Rust restore path); and (b) the new PersistentDashpayRejectedRequest.owner: PersistentIdentity (non-optional, populated whenever the user rejects an incoming request). Either child reaching the wipe path guarantees a save() fatal in PHASE 2 before the wallet row is deleted — the user believes their data was wiped while plaintext memo, counterparty id, amount, txid (payments) and rejection tombstones (privacy-relevant: who the user cut off) remain on disk. Confidentiality regression on device handoff/disposal. The dashpayRejectedRequests half is uniquely introduced by this PR.
for identity in identitiesToDelete {
for name in Array(identity.dpnsNames) {
backgroundContext.delete(name)
}
if let profile = identity.dashpayProfile {
backgroundContext.delete(profile)
}
for cr in Array(identity.contactRequests) {
backgroundContext.delete(cr)
}
for payment in Array(identity.dashpayPayments) {
backgroundContext.delete(payment)
}
for tomb in Array(identity.dashpayRejectedRequests) {
backgroundContext.delete(tomb)
}
}
blocking: create_wallet_from_seed_bytes signature change breaks tests + examples — by-value [u8;64] doesn't match &[u8;64] parameter
packages/rs-platform-wallet/tests/spv_sync.rs (line 183)
Verified at HEAD a7930b5. wallet_lifecycle.rs:100-117 now takes seed_bytes: &[u8; 64], but stale call sites still pass the array by value: tests/spv_sync.rs:186 (seed_bytes), examples/basic_usage.rs:61 (seed_bytes), examples/shielded_sync.rs:243 (transparent_seed), examples/shielded_sync_paloma.rs:237 (transparent_seed). Rust does not auto-borrow ordinary function arguments, so cargo check -p platform-wallet --tests --examples fails with error[E0308]: mismatched types ... expected '&[u8; 64]', found '[u8; 64]'. The library build succeeds, so the regression is invisible to cargo build -p platform-wallet, but the test/example targets — the harness that exercises wallet lifecycle and SPV/DashPay sync paths central to this PR — are broken. Fix is mechanical: prefix & at each remaining site.
let platform_wallet = manager
.create_wallet_from_seed_bytes(
network,
&seed_bytes,
WalletAccountCreationOptions::Default,
None,
)
.await
.expect("Failed to create platform wallet");
suggestion: Seed dereference at line 110 materializes a non-zeroized [u8;64] stack copy, contradicting the doc-comment invariant
packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs (line 100)
Verified at HEAD a7930b5. The doc-comment at lines 103-105 asserts this method 'never owns a non-zeroized stack copy of the master secret — the only copy is the one consumed into key-wallet's Seed below'. Line 110 calls Wallet::from_seed_bytes(*seed_bytes, network, accounts). Because [u8; 64] is Copy, dereferencing &[u8; 64] materializes a fresh 64-byte byte-for-byte copy in this caller's stack frame to pass by value into from_seed_bytes. That intermediate copy is not wrapped in Zeroizing and is not explicitly cleared on return; whether the bytes survive depends on stack reuse. The FFI-side Zeroizing wrapper at rs-platform-wallet-ffi/src/manager.rs still covers the cross-language hop, but the very next Rust frame past the FFI boundary defeats the defense-in-depth that wrapper was added to provide. Fix: change Wallet::from_seed_bytes upstream in key-wallet to take &[u8; 64] and zeroize internally, or wrap the local in Zeroizing (let seed = Zeroizing::new(*seed_bytes); Wallet::from_seed_bytes(*seed, ...)).
suggestion: Create-only contact-request FFI result drops entropy required for a valid downstream broadcast
packages/rs-sdk-ffi/src/dashpay/contact_request.rs (line 181)
Verified at HEAD a7930b5 lines 181-188. The Rust ContactRequestResult now carries entropy: Bytes32 because consensus recomputes the document id from entropy at apply time and rejects mismatches with InvalidDocumentTransitionIdError (rs-sdk dashpay/contact_request.rs). The FFI create-only result DashSDKContactRequestResult exposes only document_id, owner_id, properties_json — no entropy field. A non-Rust embedder calling dash_sdk_dashpay_create_contact_request and then submitting the document through the generic document-put FFI has no way to recover the entropy needed to satisfy consensus, so the resulting contact request is unusable for staged create-then-broadcast flows. The in-tree Swift caller currently uses the combined create+send entry point, so the gap is dormant for this PR, but the public C ABI for the create-only path was tightened in this PR (DIP-15 69-byte plaintext + entropy-derived document id) without exposing the field that downstream callers now need. Add entropy: [u8; 32] to DashSDKContactRequestResult and document the caller contract.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:140-155: record_rejected_contact_request never emits removed_incoming — rejected requests resurrect on the Rust SQLite backend
Verified at HEAD a7930b58 lines 140-155. The method removes the in-memory `incoming_contact_requests` entry (line 141) and populates only `cs.rejected` (lines 152-154); `cs.removed_incoming` is left empty. The unified Rust SQLite contacts writer in `packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs` only DELETEs from the `contacts` table when `cs.removed_incoming` is non-empty; the `cs.rejected` branch writes only the tombstone table. The persisted `state='received'` row therefore stays in SQLite after a reject. On next load, the contacts loader rehydrates it as a live incoming entry — the sync-side tombstone check only suppresses on-chain re-ingest, not local rehydration of an already-stored row. The Swift persister masks this in SwiftData via a bespoke `deleteRejectedIncomingRow` call, but the Rust SQLite backend used by tests and any non-Swift host has no equivalent, so the two persistence backends silently diverge. Because the on-platform contactRequest document is immutable, this local tombstone+delete is the only suppression vector against a rejected sender; without `removed_incoming`, the user's explicit reject is silently undone on every relaunch.
- [BLOCKING] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:3336-3346: Wallet-delete PHASE 1 omits dashpayPayments and the newly-added dashpayRejectedRequests — guaranteed SwiftData fatal on wipe
Verified at HEAD a7930b58 lines 3336-3346. The surrounding comment (3312-3334) documents that PHASE 1 exists precisely because SwiftData fatals during `save()` when it has to null out a non-optional inverse on a child being processed in the same batch as its parent. The loop pre-deletes only `dpnsNames`, `dashpayProfile`, and `contactRequests`. This PR adds two more non-optional cascade children of `PersistentIdentity`: (a) `PersistentDashpayPayment.owner: PersistentIdentity` (non-optional, populated by every load via the Rust restore path); and (b) the new `PersistentDashpayRejectedRequest.owner: PersistentIdentity` (non-optional, populated whenever the user rejects an incoming request). Either child reaching the wipe path guarantees a `save()` fatal in PHASE 2 *before* the wallet row is deleted — the user believes their data was wiped while plaintext memo, counterparty id, amount, txid (payments) and rejection tombstones (privacy-relevant: who the user cut off) remain on disk. Confidentiality regression on device handoff/disposal. The `dashpayRejectedRequests` half is uniquely introduced by this PR.
- [BLOCKING] In `packages/rs-platform-wallet/tests/spv_sync.rs`:183-191: create_wallet_from_seed_bytes signature change breaks tests + examples — by-value [u8;64] doesn't match &[u8;64] parameter
Verified at HEAD a7930b58. `wallet_lifecycle.rs:100-117` now takes `seed_bytes: &[u8; 64]`, but stale call sites still pass the array by value: `tests/spv_sync.rs:186` (`seed_bytes`), `examples/basic_usage.rs:61` (`seed_bytes`), `examples/shielded_sync.rs:243` (`transparent_seed`), `examples/shielded_sync_paloma.rs:237` (`transparent_seed`). Rust does not auto-borrow ordinary function arguments, so `cargo check -p platform-wallet --tests --examples` fails with `error[E0308]: mismatched types ... expected '&[u8; 64]', found '[u8; 64]'`. The library build succeeds, so the regression is invisible to `cargo build -p platform-wallet`, but the test/example targets — the harness that exercises wallet lifecycle and SPV/DashPay sync paths central to this PR — are broken. Fix is mechanical: prefix `&` at each remaining site.
- [SUGGESTION] In `packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:100-117: Seed dereference at line 110 materializes a non-zeroized [u8;64] stack copy, contradicting the doc-comment invariant
Verified at HEAD a7930b58. The doc-comment at lines 103-105 asserts this method 'never owns a non-zeroized stack copy of the master secret — the only copy is the one consumed into key-wallet's `Seed` below'. Line 110 calls `Wallet::from_seed_bytes(*seed_bytes, network, accounts)`. Because `[u8; 64]` is `Copy`, dereferencing `&[u8; 64]` materializes a fresh 64-byte byte-for-byte copy in this caller's stack frame to pass by value into `from_seed_bytes`. That intermediate copy is not wrapped in `Zeroizing` and is not explicitly cleared on return; whether the bytes survive depends on stack reuse. The FFI-side `Zeroizing` wrapper at `rs-platform-wallet-ffi/src/manager.rs` still covers the cross-language hop, but the very next Rust frame past the FFI boundary defeats the defense-in-depth that wrapper was added to provide. Fix: change `Wallet::from_seed_bytes` upstream in key-wallet to take `&[u8; 64]` and zeroize internally, or wrap the local in `Zeroizing` (`let seed = Zeroizing::new(*seed_bytes); Wallet::from_seed_bytes(*seed, ...)`).
- [SUGGESTION] In `packages/rs-sdk-ffi/src/dashpay/contact_request.rs`:181-188: Create-only contact-request FFI result drops entropy required for a valid downstream broadcast
Verified at HEAD a7930b58 lines 181-188. The Rust `ContactRequestResult` now carries `entropy: Bytes32` because consensus recomputes the document id from entropy at apply time and rejects mismatches with `InvalidDocumentTransitionIdError` (rs-sdk dashpay/contact_request.rs). The FFI create-only result `DashSDKContactRequestResult` exposes only `document_id`, `owner_id`, `properties_json` — no `entropy` field. A non-Rust embedder calling `dash_sdk_dashpay_create_contact_request` and then submitting the document through the generic document-put FFI has no way to recover the entropy needed to satisfy consensus, so the resulting contact request is unusable for staged create-then-broadcast flows. The in-tree Swift caller currently uses the combined create+send entry point, so the gap is dormant for this PR, but the public C ABI for the create-only path was tightened in this PR (DIP-15 69-byte plaintext + entropy-derived document id) without exposing the field that downstream callers now need. Add `entropy: [u8; 32]` to `DashSDKContactRequestResult` and document the caller contract.
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.
…example create_wallet_from_seed_bytes takes &[u8; 64] since the seed-zeroize hardening (03ba3ee), but this example still passed the array by value (E0308). It only compiles under the `shielded` feature, which the default `cargo test -p platform-wallet --lib` doesn't build — CI's macOS shielded build caught it on the first run after the conflict cleared. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…xplorer The `check-storage-explorer.sh` CI gate requires every SwiftData model in DashModelContainer.modelTypes to be referenced in all three explorer views. The new rejected-tombstone model (added for the resurrection fix) was registered in the container but missing from the explorer, failing the gate. Add its modelRow + per-network directCount, list view, and detail view, mirroring PersistentDashpayPayment. `check-storage-explorer.sh` now passes (30/30 models) and the app builds. 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
All 5 prior findings reconciled and STILL VALID at HEAD 666bc0a. The latest delta (a7930b5..HEAD) fixed only the shielded_sync_paloma seed callsite and added read-only Storage Explorer wiring for PersistentDashpayRejectedRequest; no new defects introduced. Three blockers remain: rejected-request rehydration on the Rust SQLite backend, SwiftData wallet-wipe omission for two non-optional cascade children (one of which this PR added), and three remaining by-value seed callsites that break cargo test --tests --examples for platform-wallet.
🔴 3 blocking | 🟡 2 suggestion(s)
5 additional finding(s)
blocking: record_rejected_contact_request leaves cs.removed_incoming empty — rejected requests resurrect on the Rust SQLite backend
packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs (line 140)
Verified at HEAD 666bc0a. The method removes the in-memory incoming_contact_requests entry (line 141) and populates only cs.rejected (lines 153-154); cs.removed_incoming is never set. The Rust SQLite contacts writer at packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:182-193 DELETEs from the contacts table only when cs.removed_incoming is non-empty; the cs.rejected branch writes only the tombstone-table row. The persisted state='received' row with its stale incoming_request blob therefore stays in SQLite, and the contacts loader rehydrates it as a live incoming entry on next load. The sync-side tombstone check only suppresses on-chain re-ingest, not local rehydration of an already-persisted row. The SwiftData persister masks this via a bespoke deleteRejectedIncomingRow call, so the two backends silently diverge: any non-Swift host on the Rust SQLite backend silently un-rejects the request on every relaunch. Because the on-platform contactRequest document is immutable, this local tombstone+delete is the only suppression vector against a rejected (potentially abusive) sender.
let owner_id = self.id();
self.incoming_contact_requests.remove(sender_id);
let tombstone = RejectedContactRequest {
owner_id,
sender_id: *sender_id,
account_reference,
document_id,
};
self.rejected_contact_requests
.insert((*sender_id, account_reference), tombstone.clone());
let mut cs = ContactChangeSet::default();
cs.removed_incoming.insert(ReceivedContactRequestKey {
owner_id,
sender_id: *sender_id,
});
cs.rejected
.insert((owner_id, *sender_id, account_reference), tombstone);
cs
blocking: Wallet-delete PHASE 1 omits dashpayPayments and dashpayRejectedRequests — guaranteed SwiftData save() fatal leaves plaintext rows on disk
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 3336)
Verified at HEAD 666bc0a. The surrounding comment (lines 3312-3334) explains PHASE 1 exists because SwiftData fatals during save() when it must null a non-optional inverse on a child processed in the same batch as its parent. The loop pre-deletes only dpnsNames, dashpayProfile, and contactRequests, but PersistentIdentity has two additional non-optional cascade children: PersistentDashpayPayment.owner (verified at PersistentDashpayPayment.swift:98, populated on every load via restore_dashpay_payments) and the new PersistentDashpayRejectedRequest.owner (verified at PersistentDashpayRejectedRequest.swift:80, populated whenever a request is rejected). Either child reaching the wipe path guarantees a save() fatal in PHASE 2 before the wallet row is removed — the user believes data was wiped while plaintext payment memo/amount/txid/counterparty rows and privacy-relevant rejection tombstones remain on disk. Confidentiality regression on device handoff/disposal. This PR specifically added PersistentDashpayRejectedRequest and wired it into the Storage Explorer (StorageExplorerView.swift:58-64) without extending the wipe path, so this PR makes the gap strictly worse.
for identity in identitiesToDelete {
for name in Array(identity.dpnsNames) {
backgroundContext.delete(name)
}
if let profile = identity.dashpayProfile {
backgroundContext.delete(profile)
}
for cr in Array(identity.contactRequests) {
backgroundContext.delete(cr)
}
for payment in Array(identity.dashpayPayments) {
backgroundContext.delete(payment)
}
for tomb in Array(identity.dashpayRejectedRequests) {
backgroundContext.delete(tomb)
}
}
blocking: create_wallet_from_seed_bytes call sites in tests/spv_sync.rs and two examples still pass [u8;64] by value
packages/rs-platform-wallet/tests/spv_sync.rs (line 183)
Verified at HEAD 666bc0a. wallet_lifecycle.rs:106 requires seed_bytes: &[u8; 64]. The latest delta fixed examples/shielded_sync_paloma.rs:237 but three by-value sites remain: tests/spv_sync.rs:186 (passes seed_bytes directly), examples/basic_usage.rs:61 (passes literal seed_bytes = [0u8; 64]), and examples/shielded_sync.rs:243 (passes transparent_seed). Rust does not auto-borrow ordinary function arguments, so cargo check -p platform-wallet --tests --examples fails with error[E0308]: mismatched types ... expected '&[u8; 64]', found '[u8; 64]'. The library build succeeds, so this is invisible to cargo build -p platform-wallet, but cargo test -p platform-wallet and the named example targets are broken — disabling the SPV-sync integration harness that exercises the wallet-lifecycle paths this PR modifies. Fix is mechanical: prefix & at each remaining site.
let platform_wallet = manager
.create_wallet_from_seed_bytes(
network,
&seed_bytes,
WalletAccountCreationOptions::Default,
None,
)
.await
.expect("Failed to create platform wallet");
suggestion: *seed_bytes deref materializes a non-zeroized [u8;64] stack copy, defeating the FFI Zeroizing wrapper
packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs (line 100)
Verified at HEAD 666bc0a. The doc comment at lines 103-105 asserts this method 'never owns a non-zeroized stack copy of the master secret — the only copy is the one consumed into key-wallet's Seed below'. Line 110 calls Wallet::from_seed_bytes(*seed_bytes, network, accounts). Because [u8; 64] is Copy, dereferencing &[u8; 64] materializes a fresh 64-byte byte-for-byte copy in this caller's stack frame to pass by value into from_seed_bytes. That copy is not wrapped in Zeroizing and is not explicitly cleared on return; whether the bytes survive depends on stack reuse and the optimizer. The FFI-side Zeroizing wrapper at rs-platform-wallet-ffi/src/manager.rs covers the cross-language hop, but this Rust frame past the FFI boundary defeats the defense-in-depth that wrapper exists to provide — and the only reason a reviewer would expect the wrapper to help is the invariant this line silently violates. Fix: change Wallet::from_seed_bytes upstream in key-wallet to take &[u8; 64] and zeroize internally, or wrap locally: let seed = zeroize::Zeroizing::new(*seed_bytes); Wallet::from_seed_bytes(*seed, network, accounts).
suggestion: Create-only contact-request C ABI drops the entropy now required to satisfy consensus
packages/rs-sdk-ffi/src/dashpay/contact_request.rs (line 181)
Verified at HEAD 666bc0a. The Rust ContactRequestResult carries entropy: Bytes32 because this PR's DIP-15 entropy threading makes consensus recompute the document id from the entropy carried in the document-create transition and reject mismatches with InvalidDocumentTransitionIdError. DashSDKContactRequestResult still exposes only document_id, owner_id, properties_json — no entropy field — and dash_sdk_dashpay_create_contact_request likewise drops result.entropy on construction. A non-Rust embedder calling the create-only path and submitting through a generic document-put FFI has no way to recover the entropy required by consensus, so the staged create-then-broadcast flow is unusable across the C ABI. The in-tree Swift caller currently uses the combined create+send entry point so the gap is dormant for this PR, but the public C ABI for the create-only path was tightened in this PR (DIP-15 69-byte plaintext + entropy-derived document id) without exposing the field downstream callers now need. Fix: add entropy: [u8; 32] (or *mut u8 allocated with the existing string allocator) to DashSDKContactRequestResult, populate it at construction, and update the cbindgen header.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:140-155: record_rejected_contact_request leaves cs.removed_incoming empty — rejected requests resurrect on the Rust SQLite backend
Verified at HEAD 666bc0ae. The method removes the in-memory `incoming_contact_requests` entry (line 141) and populates only `cs.rejected` (lines 153-154); `cs.removed_incoming` is never set. The Rust SQLite contacts writer at `packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:182-193` DELETEs from the `contacts` table only when `cs.removed_incoming` is non-empty; the `cs.rejected` branch writes only the tombstone-table row. The persisted `state='received'` row with its stale `incoming_request` blob therefore stays in SQLite, and the contacts loader rehydrates it as a live incoming entry on next load. The sync-side tombstone check only suppresses on-chain re-ingest, not local rehydration of an already-persisted row. The SwiftData persister masks this via a bespoke `deleteRejectedIncomingRow` call, so the two backends silently diverge: any non-Swift host on the Rust SQLite backend silently un-rejects the request on every relaunch. Because the on-platform contactRequest document is immutable, this local tombstone+delete is the only suppression vector against a rejected (potentially abusive) sender.
- [BLOCKING] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:3336-3346: Wallet-delete PHASE 1 omits dashpayPayments and dashpayRejectedRequests — guaranteed SwiftData save() fatal leaves plaintext rows on disk
Verified at HEAD 666bc0ae. The surrounding comment (lines 3312-3334) explains PHASE 1 exists because SwiftData fatals during `save()` when it must null a non-optional inverse on a child processed in the same batch as its parent. The loop pre-deletes only `dpnsNames`, `dashpayProfile`, and `contactRequests`, but `PersistentIdentity` has two additional non-optional cascade children: `PersistentDashpayPayment.owner` (verified at PersistentDashpayPayment.swift:98, populated on every load via `restore_dashpay_payments`) and the new `PersistentDashpayRejectedRequest.owner` (verified at PersistentDashpayRejectedRequest.swift:80, populated whenever a request is rejected). Either child reaching the wipe path guarantees a `save()` fatal in PHASE 2 before the wallet row is removed — the user believes data was wiped while plaintext payment memo/amount/txid/counterparty rows and privacy-relevant rejection tombstones remain on disk. Confidentiality regression on device handoff/disposal. This PR specifically added `PersistentDashpayRejectedRequest` and wired it into the Storage Explorer (StorageExplorerView.swift:58-64) without extending the wipe path, so this PR makes the gap strictly worse.
- [BLOCKING] In `packages/rs-platform-wallet/tests/spv_sync.rs`:183-191: create_wallet_from_seed_bytes call sites in tests/spv_sync.rs and two examples still pass [u8;64] by value
Verified at HEAD 666bc0ae. `wallet_lifecycle.rs:106` requires `seed_bytes: &[u8; 64]`. The latest delta fixed `examples/shielded_sync_paloma.rs:237` but three by-value sites remain: `tests/spv_sync.rs:186` (passes `seed_bytes` directly), `examples/basic_usage.rs:61` (passes literal `seed_bytes = [0u8; 64]`), and `examples/shielded_sync.rs:243` (passes `transparent_seed`). Rust does not auto-borrow ordinary function arguments, so `cargo check -p platform-wallet --tests --examples` fails with `error[E0308]: mismatched types ... expected '&[u8; 64]', found '[u8; 64]'`. The library build succeeds, so this is invisible to `cargo build -p platform-wallet`, but `cargo test -p platform-wallet` and the named example targets are broken — disabling the SPV-sync integration harness that exercises the wallet-lifecycle paths this PR modifies. Fix is mechanical: prefix `&` at each remaining site.
- [SUGGESTION] In `packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:100-117: *seed_bytes deref materializes a non-zeroized [u8;64] stack copy, defeating the FFI Zeroizing wrapper
Verified at HEAD 666bc0ae. The doc comment at lines 103-105 asserts this method 'never owns a non-zeroized stack copy of the master secret — the only copy is the one consumed into key-wallet's `Seed` below'. Line 110 calls `Wallet::from_seed_bytes(*seed_bytes, network, accounts)`. Because `[u8; 64]` is `Copy`, dereferencing `&[u8; 64]` materializes a fresh 64-byte byte-for-byte copy in this caller's stack frame to pass by value into `from_seed_bytes`. That copy is not wrapped in `Zeroizing` and is not explicitly cleared on return; whether the bytes survive depends on stack reuse and the optimizer. The FFI-side `Zeroizing` wrapper at `rs-platform-wallet-ffi/src/manager.rs` covers the cross-language hop, but this Rust frame past the FFI boundary defeats the defense-in-depth that wrapper exists to provide — and the only reason a reviewer would expect the wrapper to help is the invariant this line silently violates. Fix: change `Wallet::from_seed_bytes` upstream in key-wallet to take `&[u8; 64]` and zeroize internally, or wrap locally: `let seed = zeroize::Zeroizing::new(*seed_bytes); Wallet::from_seed_bytes(*seed, network, accounts)`.
- [SUGGESTION] In `packages/rs-sdk-ffi/src/dashpay/contact_request.rs`:181-188: Create-only contact-request C ABI drops the entropy now required to satisfy consensus
Verified at HEAD 666bc0ae. The Rust `ContactRequestResult` carries `entropy: Bytes32` because this PR's DIP-15 entropy threading makes consensus recompute the document id from the entropy carried in the document-create transition and reject mismatches with `InvalidDocumentTransitionIdError`. `DashSDKContactRequestResult` still exposes only `document_id`, `owner_id`, `properties_json` — no `entropy` field — and `dash_sdk_dashpay_create_contact_request` likewise drops `result.entropy` on construction. A non-Rust embedder calling the create-only path and submitting through a generic document-put FFI has no way to recover the entropy required by consensus, so the staged create-then-broadcast flow is unusable across the C ABI. The in-tree Swift caller currently uses the combined create+send entry point so the gap is dormant for this PR, but the public C ABI for the create-only path was tightened in this PR (DIP-15 69-byte plaintext + entropy-derived document id) without exposing the field downstream callers now need. Fix: add `entropy: [u8; 32]` (or `*mut u8` allocated with the existing string allocator) to `DashSDKContactRequestResult`, populate it at construction, and update the cbindgen header.
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.
shumkov
changed the title
… drop dead import The create-wallet FFI boundary now takes `seed_bytes: &[u8; 64]` (zeroization hardening), but the `spv_sync` integration test still passed it by value, breaking the `platform-wallet` test build (caught only by --all-targets, not the lib-only / mocks feature run). Borrow at the call site. Also remove the now-unused `key_wallet::bip32::ExtendedPrivKey` import in the dip14 test module (dead since the contact-xpub refactor; warns under -D warnings). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolve two SwiftExampleApp conflicts introduced by the v3.1-dev GlobalSyncIndicatorOverlay perf refactor landing alongside this PR's DashPay tab: - ContentView.swift: adopt v3.1-dev's GlobalSyncIndicatorOverlay leaf (keeps the volatile walletManager.spvProgress observation out of the TabView so sync ticks don't tear down drill-downs), while keeping this PR's appUIState-backed tab-selection binding (deep views switch tabs) and the DashPay tab. appUIState is re-declared on ContentView (it only publishes on low-frequency UI events, not sync progress); selectedTab is a Binding so the overlay reads selectedTab.wrappedValue. - CreateIdentityView.swift: keep v3.1-dev's funding-footer naming (fundingSourceFooterText) — pure rename/whitespace, identical copy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…arration
Rewrites code comments that were anchored to the development moment and
would not make sense to a future reader, without changing behavior:
- Strip TDD red/green narration ("RED before task 9", "GREEN after",
"before the fix") — reframed to state the invariant / hazard the test
pins (the rationale is kept, the process bookkeeping dropped).
- Drop speculative milestone ETAs ("once M4 lands", "G4 hook lands this
later", "M2 app stored these device-locally") and milestone-task tags
("M1 task 9", "M3 task 13") — replaced with plain descriptions; bare
gap IDs (G4/G5/G15) are kept as cross-references to docs/dashpay/SPEC.md.
- Drop PR-relative phrasing ("as before this change", "Previously a…").
- Add the missing "Research date: 2026-06-10" anchor to research/03 so
its "current flow" snapshot reads correctly as a point-in-time record.
The dated SPEC.md DONE-journal entries are left as-is (a historical record
anchored to its Status (2026-06-10) header).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…hielded_sync examples The seed-by-ref change to create_wallet_from_seed_bytes (&[u8; 64]) was propagated to spv_sync but these two examples still passed the seed by value (E0308). They only compile under feature/example builds the default cargo test --lib doesn't run, so CI's macOS shielded build surfaces them. 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 incremental review at a3ca03c. The 666bc0a..HEAD delta is mostly a v3.1-dev merge plus one in-scope fix (454ae52: spv_sync.rs seed-by-ref). All five prior findings were re-verified against current source: #1 (record_rejected_contact_request omits cs.removed_incoming), #2 (wallet-wipe PHASE 1 omits dashpayPayments + the newly-added dashpayRejectedRequests), #4 (seed deref defeats zeroize), and #5 (create-only contact-request FFI drops entropy) are unchanged. #3 (seed-by-ref callsite migration) is PARTIALLY FIXED — tests/spv_sync.rs was patched, but examples/basic_usage.rs:61 and examples/shielded_sync.rs:243 still pass [u8;64] by value and break cargo check --examples. No new defects were introduced by this delta's in-scope commit. Requesting changes.
🔴 4 blocking | 🟡 2 suggestion(s)
6 additional finding(s)
blocking: record_rejected_contact_request leaves cs.removed_incoming empty — rejected requests rehydrate on the Rust SQLite backend
packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs (line 140)
Verified STILL VALID at HEAD a3ca03c (lines 140-156). The method removes the in-memory incoming entry (line 141) and populates only cs.rejected (lines 152-154); cs.removed_incoming is never set. The unified Rust SQLite contacts writer at packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs DELETEs from the contacts table only when cs.removed_incoming is non-empty; the cs.rejected branch writes only the tombstone-table row. The persisted state='received' row therefore stays in SQLite after a reject, and the contacts loader rehydrates it as a live incoming entry on next load. The sync-side tombstone check only suppresses on-chain re-ingest, not local rehydration of an already-persisted row. The SwiftData persister masks this with a bespoke deleteRejectedIncomingRow call, so the two backends silently diverge: any non-Swift host on the Rust SQLite backend silently un-rejects the request on every relaunch. Because the on-platform contactRequest document is immutable, this local tombstone+delete is the only suppression vector against a rejected (potentially harassing) sender, so the user's explicit reject is silently undone.
let owner_id = self.id();
self.incoming_contact_requests.remove(sender_id);
let tombstone = RejectedContactRequest {
owner_id,
sender_id: *sender_id,
account_reference,
document_id,
};
self.rejected_contact_requests
.insert((*sender_id, account_reference), tombstone.clone());
let mut cs = ContactChangeSet::default();
cs.removed_incoming.insert(ReceivedContactRequestKey {
owner_id,
sender_id: *sender_id,
});
cs.rejected
.insert((owner_id, *sender_id, account_reference), tombstone);
cs
blocking: Wallet-delete PHASE 1 omits dashpayPayments and dashpayRejectedRequests — SwiftData save() fatal mid-wipe leaves plaintext rows on disk
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 3336)
Verified STILL VALID at HEAD a3ca03c (lines 3336-3346). The surrounding comment (3312-3334) documents that PHASE 1 exists precisely because SwiftData fatals during save() when it must null a non-optional inverse on a child processed in the same batch as its parent. The loop pre-deletes only dpnsNames, dashpayProfile, and contactRequests. Two non-optional cascade children of PersistentIdentity are missing: (a) PersistentDashpayPayment.owner (non-optional at PersistentDashpayPayment.swift:98, populated on every load via restore_dashpay_payments) and (b) the newly-added PersistentDashpayRejectedRequest.owner (non-optional at PersistentDashpayRejectedRequest.swift:80, populated whenever the user rejects an incoming request). Either child reaching the wipe path guarantees a save() fatal in PHASE 2 before the wallet row is deleted — the user believes data was wiped while plaintext payment memo/amount/txid/counterparty rows and privacy-relevant rejection tombstones remain on disk (confidentiality regression on device handoff/disposal). This PR uniquely introduces PersistentDashpayRejectedRequest and wires it through the Storage Explorer without extending the wipe path, so the gap is strictly worsened here.
for identity in identitiesToDelete {
for name in Array(identity.dpnsNames) {
backgroundContext.delete(name)
}
if let profile = identity.dashpayProfile {
backgroundContext.delete(profile)
}
for cr in Array(identity.contactRequests) {
backgroundContext.delete(cr)
}
for payment in Array(identity.dashpayPayments) {
backgroundContext.delete(payment)
}
for tomb in Array(identity.dashpayRejectedRequests) {
backgroundContext.delete(tomb)
}
}
blocking: Two example call sites still pass [u8;64] by value to create_wallet_from_seed_bytes — `cargo check --examples` fails
packages/rs-platform-wallet/examples/basic_usage.rs (line 58)
PARTIALLY FIXED: commit 454ae52 migrated tests/spv_sync.rs to &seed_bytes, but two example call sites were not updated and remain broken at HEAD. Verified by Read: wallet_lifecycle.rs:106 requires seed_bytes: &[u8; 64], but examples/basic_usage.rs:61 still passes seed_bytes by value (confirmed at line 61) and examples/shielded_sync.rs:243 still passes transparent_seed by value (confirmed at line 243). Rust does not auto-borrow ordinary function arguments, so cargo check -p platform-wallet --examples and cargo check -p platform-wallet --example shielded_sync --features shielded both fail with error[E0308]: mismatched types ... expected '&[u8; 64]', found '[u8; 64]'. The library and test builds succeed, so this is invisible to cargo build -p platform-wallet, but the example targets are broken. Fix is mechanical at both sites: prefix with &. The shielded_sync.rs fix is required in addition to the basic_usage.rs fix shown below — both must be changed.
// Create a wallet from seed bytes
let seed_bytes = [0u8; 64]; // dummy seed for example
let wallet = manager
.create_wallet_from_seed_bytes(
Network::Testnet,
&seed_bytes,
WalletAccountCreationOptions::Default,
None,
)
.await?;
blocking: shielded_sync example passes transparent_seed by value to create_wallet_from_seed_bytes
packages/rs-platform-wallet/examples/shielded_sync.rs (line 240)
Companion to the basic_usage.rs finding above — verified at HEAD a3ca03c lines 240-248. wallet_lifecycle.rs:106 now requires seed_bytes: &[u8; 64] after the zeroize hardening, but transparent_seed is still passed by value. cargo check -p platform-wallet --example shielded_sync --features shielded fails with E0308 at line 243. The companion example shielded_sync_paloma.rs:237 was fixed in the prior delta; this one was missed. Fix is mechanical: prefix & at the call site.
let platform_wallet = manager
.create_wallet_from_seed_bytes(
network,
&transparent_seed,
WalletAccountCreationOptions::Default,
None,
)
.await
.expect("create_wallet_from_seed_bytes");
suggestion: *seed_bytes deref materializes a non-zeroized [u8;64] stack copy, contradicting the doc-comment invariant
packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs (line 100)
Verified STILL VALID at HEAD a3ca03c (lines 100-117). The doc-comment at lines 103-105 asserts this method 'never owns a non-zeroized stack copy of the master secret — the only copy is the one consumed into key-wallet's Seed below'. Line 110 calls Wallet::from_seed_bytes(*seed_bytes, network, accounts). Because [u8; 64] is Copy, dereferencing &[u8; 64] materializes a fresh 64-byte byte-for-byte copy in this caller's stack frame to pass by value into from_seed_bytes. That intermediate copy is not wrapped in Zeroizing and is not explicitly cleared on return; whether the bytes survive depends on stack reuse and the optimizer. The FFI-side Zeroizing wrapper at rs-platform-wallet-ffi/src/manager.rs covers the cross-language hop, but this very next Rust frame past the FFI boundary defeats the defense-in-depth that wrapper exists to provide. Fix: change Wallet::from_seed_bytes upstream in key-wallet to take &[u8; 64] and zeroize internally, or wrap locally with let seed = zeroize::Zeroizing::new(*seed_bytes); Wallet::from_seed_bytes(*seed, network, accounts). Exploitation requires a local memory disclosure vector (crash dumps, process-memory forensics, or another memory-safety bug), so this is defense-in-depth rather than a direct remote exploit.
suggestion: Create-only contact-request C ABI drops the entropy required by consensus to validate the document id
packages/rs-sdk-ffi/src/dashpay/contact_request.rs (line 181)
Verified STILL VALID at HEAD a3ca03c (lines 181-188). The Rust ContactRequestResult carries entropy: Bytes32 because this PR's DIP-15 entropy threading makes consensus recompute the document id from the entropy in the document-create transition and reject mismatches with InvalidDocumentTransitionIdError. DashSDKContactRequestResult still exposes only document_id, owner_id, and properties_json — no entropy field — and the create-only constructor drops result.entropy. The input contract at lines 169-176 was tightened in this PR to require the 69-byte DIP-15 compact form, but the output struct was not extended in parallel. A non-Rust embedder calling dash_sdk_dashpay_create_contact_request and submitting through a generic document-put FFI has no way to recover the entropy required by consensus, so any staged create-then-broadcast flow across the C ABI silently produces transitions consensus will reject. The in-tree Swift caller currently uses the combined create+send entry point so the gap is dormant for this PR, but it is a forward-compatibility defect introduced by this PR's tightening of the create-only path. Fix: add entropy: [u8; 32] (or *mut c_char allocated via the existing string allocator) to DashSDKContactRequestResult, populate at construction, and update the cbindgen header.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:140-156: record_rejected_contact_request leaves cs.removed_incoming empty — rejected requests rehydrate on the Rust SQLite backend
Verified STILL VALID at HEAD a3ca03c0 (lines 140-156). The method removes the in-memory incoming entry (line 141) and populates only cs.rejected (lines 152-154); cs.removed_incoming is never set. The unified Rust SQLite contacts writer at packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs DELETEs from the contacts table only when cs.removed_incoming is non-empty; the cs.rejected branch writes only the tombstone-table row. The persisted state='received' row therefore stays in SQLite after a reject, and the contacts loader rehydrates it as a live incoming entry on next load. The sync-side tombstone check only suppresses on-chain re-ingest, not local rehydration of an already-persisted row. The SwiftData persister masks this with a bespoke deleteRejectedIncomingRow call, so the two backends silently diverge: any non-Swift host on the Rust SQLite backend silently un-rejects the request on every relaunch. Because the on-platform contactRequest document is immutable, this local tombstone+delete is the only suppression vector against a rejected (potentially harassing) sender, so the user's explicit reject is silently undone.
- [BLOCKING] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:3336-3346: Wallet-delete PHASE 1 omits dashpayPayments and dashpayRejectedRequests — SwiftData save() fatal mid-wipe leaves plaintext rows on disk
Verified STILL VALID at HEAD a3ca03c0 (lines 3336-3346). The surrounding comment (3312-3334) documents that PHASE 1 exists precisely because SwiftData fatals during save() when it must null a non-optional inverse on a child processed in the same batch as its parent. The loop pre-deletes only dpnsNames, dashpayProfile, and contactRequests. Two non-optional cascade children of PersistentIdentity are missing: (a) PersistentDashpayPayment.owner (non-optional at PersistentDashpayPayment.swift:98, populated on every load via restore_dashpay_payments) and (b) the newly-added PersistentDashpayRejectedRequest.owner (non-optional at PersistentDashpayRejectedRequest.swift:80, populated whenever the user rejects an incoming request). Either child reaching the wipe path guarantees a save() fatal in PHASE 2 before the wallet row is deleted — the user believes data was wiped while plaintext payment memo/amount/txid/counterparty rows and privacy-relevant rejection tombstones remain on disk (confidentiality regression on device handoff/disposal). This PR uniquely introduces PersistentDashpayRejectedRequest and wires it through the Storage Explorer without extending the wipe path, so the gap is strictly worsened here.
- [BLOCKING] In `packages/rs-platform-wallet/examples/basic_usage.rs`:58-65: Two example call sites still pass [u8;64] by value to create_wallet_from_seed_bytes — `cargo check --examples` fails
PARTIALLY FIXED: commit 454ae524ce migrated tests/spv_sync.rs to &seed_bytes, but two example call sites were not updated and remain broken at HEAD. Verified by Read: wallet_lifecycle.rs:106 requires `seed_bytes: &[u8; 64]`, but examples/basic_usage.rs:61 still passes `seed_bytes` by value (confirmed at line 61) and examples/shielded_sync.rs:243 still passes `transparent_seed` by value (confirmed at line 243). Rust does not auto-borrow ordinary function arguments, so `cargo check -p platform-wallet --examples` and `cargo check -p platform-wallet --example shielded_sync --features shielded` both fail with `error[E0308]: mismatched types ... expected '&[u8; 64]', found '[u8; 64]'`. The library and test builds succeed, so this is invisible to `cargo build -p platform-wallet`, but the example targets are broken. Fix is mechanical at both sites: prefix with `&`. The shielded_sync.rs fix is required in addition to the basic_usage.rs fix shown below — both must be changed.
- [BLOCKING] In `packages/rs-platform-wallet/examples/shielded_sync.rs`:240-248: shielded_sync example passes transparent_seed by value to create_wallet_from_seed_bytes
Companion to the basic_usage.rs finding above — verified at HEAD a3ca03c0 lines 240-248. wallet_lifecycle.rs:106 now requires `seed_bytes: &[u8; 64]` after the zeroize hardening, but `transparent_seed` is still passed by value. `cargo check -p platform-wallet --example shielded_sync --features shielded` fails with E0308 at line 243. The companion example shielded_sync_paloma.rs:237 was fixed in the prior delta; this one was missed. Fix is mechanical: prefix `&` at the call site.
- [SUGGESTION] In `packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:100-117: *seed_bytes deref materializes a non-zeroized [u8;64] stack copy, contradicting the doc-comment invariant
Verified STILL VALID at HEAD a3ca03c0 (lines 100-117). The doc-comment at lines 103-105 asserts this method 'never owns a non-zeroized stack copy of the master secret — the only copy is the one consumed into key-wallet's `Seed` below'. Line 110 calls `Wallet::from_seed_bytes(*seed_bytes, network, accounts)`. Because [u8; 64] is Copy, dereferencing &[u8; 64] materializes a fresh 64-byte byte-for-byte copy in this caller's stack frame to pass by value into from_seed_bytes. That intermediate copy is not wrapped in Zeroizing and is not explicitly cleared on return; whether the bytes survive depends on stack reuse and the optimizer. The FFI-side Zeroizing wrapper at rs-platform-wallet-ffi/src/manager.rs covers the cross-language hop, but this very next Rust frame past the FFI boundary defeats the defense-in-depth that wrapper exists to provide. Fix: change `Wallet::from_seed_bytes` upstream in key-wallet to take &[u8; 64] and zeroize internally, or wrap locally with `let seed = zeroize::Zeroizing::new(*seed_bytes); Wallet::from_seed_bytes(*seed, network, accounts)`. Exploitation requires a local memory disclosure vector (crash dumps, process-memory forensics, or another memory-safety bug), so this is defense-in-depth rather than a direct remote exploit.
- [SUGGESTION] In `packages/rs-sdk-ffi/src/dashpay/contact_request.rs`:181-188: Create-only contact-request C ABI drops the entropy required by consensus to validate the document id
Verified STILL VALID at HEAD a3ca03c0 (lines 181-188). The Rust ContactRequestResult carries `entropy: Bytes32` because this PR's DIP-15 entropy threading makes consensus recompute the document id from the entropy in the document-create transition and reject mismatches with InvalidDocumentTransitionIdError. DashSDKContactRequestResult still exposes only document_id, owner_id, and properties_json — no entropy field — and the create-only constructor drops result.entropy. The input contract at lines 169-176 was tightened in this PR to require the 69-byte DIP-15 compact form, but the output struct was not extended in parallel. A non-Rust embedder calling dash_sdk_dashpay_create_contact_request and submitting through a generic document-put FFI has no way to recover the entropy required by consensus, so any staged create-then-broadcast flow across the C ABI silently produces transitions consensus will reject. The in-tree Swift caller currently uses the combined create+send entry point so the gap is dormant for this PR, but it is a forward-compatibility defect introduced by this PR's tightening of the create-only path. Fix: add `entropy: [u8; 32]` (or `*mut c_char` allocated via the existing string allocator) to DashSDKContactRequestResult, populate at construction, and update the cbindgen header.
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.
…schema The payment_channel_broken column + rejected_contact_requests table were added in an append-only V002 (to avoid editing a migration that ships in v4.0.0-beta.4/rc.1/rc.2). Squash them back into V001: the storage crate has no product consumers — nothing instantiates SqlitePersister or runs these migrations (the iOS app uses the SwiftData persister via the FFI callbacks; no other crate depends on it), so no real database ever applied V001. A single clean initial schema is preferable for a still-unreleased, unconsumed crate. Replace the V001→V002 upgrade-path test with v001_creates_dashpay_sync_schema, which pins that the (only) migration creates the column + tombstone table. 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 0d8f69f. The a3ca03c..HEAD delta is a single doc-only commit (docs(dashpay): make comments timeless) — no behavior, FFI surface, or signatures changed. All six prior findings were re-verified directly against the worktree and remain STILL VALID. Two persistence blockers (Rust SQLite rejected-request rehydration and SwiftData wallet-wipe PHASE 1 missing two non-optional cascade children) plus two example-target build failures (E0308 on by-value [u8;64] seed) and two suggestion-tier hardening items (seed-zeroization gap, create-only contact-request C ABI dropping consensus-required entropy) carry forward unchanged.
🔴 4 blocking | 🟡 2 suggestion(s)
6 additional finding(s)
blocking: record_rejected_contact_request leaves cs.removed_incoming empty — rejected requests rehydrate on the Rust SQLite backend
packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs (line 134)
Verified at HEAD 0d8f69f: the method removes the in-memory incoming_contact_requests entry at line 141 and populates only cs.rejected (lines 152-154); cs.removed_incoming is never set. The unified Rust SQLite contacts writer in packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs DELETEs from the contacts table only when cs.removed_incoming is non-empty; the cs.rejected branch writes only the tombstone-table row. The persisted state='received' row therefore stays in SQLite after a reject, and the contacts loader rehydrates it as a live incoming entry on next process start. The SwiftData persister masks this with a bespoke deleteRejectedIncomingRow call, so the two backends silently diverge — any non-Swift host on the Rust SQLite backend silently un-rejects the request on every relaunch. Because the on-platform contactRequest document is immutable, this local tombstone+delete is the only suppression vector against a (potentially harassing) rejected sender; the user's explicit reject is silently undone. The PR's own doc comment at lines 123-133 documents the tombstone is intended to prevent the recurring sync from resurrecting the row, but the persistence path is incomplete.
let owner_id = self.id();
self.incoming_contact_requests.remove(sender_id);
let tombstone = RejectedContactRequest {
owner_id,
sender_id: *sender_id,
account_reference,
document_id,
};
self.rejected_contact_requests
.insert((*sender_id, account_reference), tombstone.clone());
let mut cs = ContactChangeSet::default();
cs.removed_incoming.insert(ReceivedContactRequestKey {
owner_id,
sender_id: *sender_id,
});
cs.rejected
.insert((owner_id, *sender_id, account_reference), tombstone);
cs
blocking: Wallet-delete PHASE 1 omits dashpayPayments and dashpayRejectedRequests — SwiftData save() can fatal mid-wipe and leave plaintext rows on disk
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 3336)
Verified at HEAD 0d8f69f: the surrounding comment at lines 3312-3334 explicitly documents PHASE 1 exists because SwiftData fatals during save() when it must null a non-optional inverse on a child processed in the same batch as its parent, and that PHASE 1 must pre-delete every cascade child whose inverse to identity is non-optional. The loop covers only dpnsNames, dashpayProfile, and contactRequests. PersistentIdentity has two additional non-optional cascade children that this PR populates on every load: (a) PersistentDashpayPayment.owner (populated via the restore_dashpay_payments callback) and (b) the newly-added PersistentDashpayRejectedRequest.owner (populated whenever the user rejects an incoming request and surfaced in the Storage Explorer view this PR adds). If either child exists at wipe time, PHASE 2's identity delete will hit the same fatal this phase is designed to avoid, aborting before the wallet row is removed. Threat: a user-initiated wallet wipe (device sale, handoff, end-of-life) silently leaves plaintext counterparty id, memo, amount, txid (payments), and privacy-relevant rejection tombstones (the set of senders the user explicitly cut off) on disk. This PR uniquely introduces PersistentDashpayRejectedRequest and wires it into the explorer without extending the wipe path, so the gap is strictly worsened by this PR.
for identity in identitiesToDelete {
for name in Array(identity.dpnsNames) {
backgroundContext.delete(name)
}
if let profile = identity.dashpayProfile {
backgroundContext.delete(profile)
}
for cr in Array(identity.contactRequests) {
backgroundContext.delete(cr)
}
for payment in Array(identity.dashpayPayments) {
backgroundContext.delete(payment)
}
for tomb in Array(identity.dashpayRejectedRequests) {
backgroundContext.delete(tomb)
}
}
blocking: basic_usage example passes [u8;64] by value to create_wallet_from_seed_bytes — `cargo check --examples` fails
packages/rs-platform-wallet/examples/basic_usage.rs (line 58)
Verified at HEAD 0d8f69f: wallet_lifecycle.rs:106 requires seed_bytes: &[u8; 64], but line 61 still passes seed_bytes by value. Rust does not auto-borrow ordinary function arguments, so cargo check -p platform-wallet --examples fails with error[E0308]: mismatched types ... expected '&[u8; 64]', found '[u8; 64]'. Library/test builds succeed (tests/spv_sync.rs was fixed in 454ae52), so the break is invisible to cargo build -p platform-wallet, but the example target — the canonical caller-facing reference for the zeroize-hardened seed entry point — is broken. Mechanical one-character fix.
let wallet = manager
.create_wallet_from_seed_bytes(
Network::Testnet,
&seed_bytes,
WalletAccountCreationOptions::Default,
None,
)
.await?;
blocking: shielded_sync example passes transparent_seed by value to create_wallet_from_seed_bytes
packages/rs-platform-wallet/examples/shielded_sync.rs (line 240)
Verified at HEAD 0d8f69f: companion to the basic_usage.rs finding. Line 243 still passes transparent_seed by value to a signature requiring &[u8; 64]. cargo check -p platform-wallet --example shielded_sync --features shielded fails with E0308. The sibling shielded_sync_paloma.rs callsite was migrated in a prior delta; this one was missed. Mechanical fix: prefix &.
let platform_wallet = manager
.create_wallet_from_seed_bytes(
network,
&transparent_seed,
WalletAccountCreationOptions::Default,
None,
)
.await
.expect("create_wallet_from_seed_bytes");
suggestion: *seed_bytes deref materializes a non-zeroized [u8;64] stack copy, contradicting the method's documented invariant
packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs (line 100)
Verified at HEAD 0d8f69f: the doc comment at lines 103-105 explicitly asserts this method 'never owns a non-zeroized stack copy of the master secret — the only copy is the one consumed into key-wallet's Seed below'. Line 110 calls Wallet::from_seed_bytes(*seed_bytes, network, accounts). Because [u8; 64] is Copy, dereferencing &[u8; 64] materializes a fresh 64-byte copy in this stack frame to pass by value, and that intermediate copy is not wrapped in Zeroizing and is not explicitly cleared on return. The FFI-side Zeroizing wrapper in rs-platform-wallet-ffi/src/manager.rs covers the Swift/C → Rust handoff, but this very next Rust frame past the FFI boundary defeats the defense-in-depth the wrapper exists to provide — and the wrapper's only value to a reviewer is the invariant this line silently violates. Exploitation requires a local memory disclosure vector (crash dumps, process-memory forensics, sibling memory-safety bug, swap capture), so this is defense-in-depth rather than directly exploitable; flagged because the doc-comment contract is explicit. Fix: either change Wallet::from_seed_bytes upstream in key-wallet to take &[u8; 64] and zeroize internally, or wrap locally with let seed = zeroize::Zeroizing::new(*seed_bytes); Wallet::from_seed_bytes(*seed, network, accounts).
suggestion: Create-only contact-request C ABI drops the entropy consensus requires to validate the document id
packages/rs-sdk-ffi/src/dashpay/contact_request.rs (line 181)
Verified at HEAD 0d8f69f: the input contract at lines 169-176 was tightened in this PR to require the 69-byte DIP-15 compact form, and the Rust ContactRequestResult carries entropy: Bytes32 because consensus recomputes the document id from the entropy in the document-create transition and rejects mismatches with InvalidDocumentTransitionIdError. DashSDKContactRequestResult still exposes only document_id, owner_id, properties_json — no entropy field — and the create-only constructor drops result.entropy. A non-Rust embedder calling dash_sdk_dashpay_create_contact_request and submitting through a generic document-put FFI has no way to recover the entropy required by consensus, so any staged create-then-broadcast flow across the C ABI silently produces transitions consensus will reject. The in-tree Swift caller uses the combined create+send entry point so the gap is dormant for this PR, but the public C ABI was tightened by this PR without exposing the field downstream callers now need. Fix: add entropy: [u8; 32] (or *mut c_char allocated via the existing string allocator) to DashSDKContactRequestResult, populate at construction, and regenerate the cbindgen header.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:134-156: record_rejected_contact_request leaves cs.removed_incoming empty — rejected requests rehydrate on the Rust SQLite backend
Verified at HEAD 0d8f69f0: the method removes the in-memory `incoming_contact_requests` entry at line 141 and populates only `cs.rejected` (lines 152-154); `cs.removed_incoming` is never set. The unified Rust SQLite contacts writer in `packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs` DELETEs from the `contacts` table only when `cs.removed_incoming` is non-empty; the `cs.rejected` branch writes only the tombstone-table row. The persisted `state='received'` row therefore stays in SQLite after a reject, and the contacts loader rehydrates it as a live incoming entry on next process start. The SwiftData persister masks this with a bespoke `deleteRejectedIncomingRow` call, so the two backends silently diverge — any non-Swift host on the Rust SQLite backend silently un-rejects the request on every relaunch. Because the on-platform contactRequest document is immutable, this local tombstone+delete is the only suppression vector against a (potentially harassing) rejected sender; the user's explicit reject is silently undone. The PR's own doc comment at lines 123-133 documents the tombstone is intended to prevent the recurring sync from resurrecting the row, but the persistence path is incomplete.
- [BLOCKING] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:3336-3346: Wallet-delete PHASE 1 omits dashpayPayments and dashpayRejectedRequests — SwiftData save() can fatal mid-wipe and leave plaintext rows on disk
Verified at HEAD 0d8f69f0: the surrounding comment at lines 3312-3334 explicitly documents PHASE 1 exists because SwiftData fatals during `save()` when it must null a non-optional inverse on a child processed in the same batch as its parent, and that PHASE 1 must pre-delete every cascade child whose inverse to identity is non-optional. The loop covers only `dpnsNames`, `dashpayProfile`, and `contactRequests`. `PersistentIdentity` has two additional non-optional cascade children that this PR populates on every load: (a) `PersistentDashpayPayment.owner` (populated via the `restore_dashpay_payments` callback) and (b) the newly-added `PersistentDashpayRejectedRequest.owner` (populated whenever the user rejects an incoming request and surfaced in the Storage Explorer view this PR adds). If either child exists at wipe time, PHASE 2's identity delete will hit the same fatal this phase is designed to avoid, aborting before the wallet row is removed. Threat: a user-initiated wallet wipe (device sale, handoff, end-of-life) silently leaves plaintext counterparty id, memo, amount, txid (payments), and privacy-relevant rejection tombstones (the set of senders the user explicitly cut off) on disk. This PR uniquely introduces `PersistentDashpayRejectedRequest` and wires it into the explorer without extending the wipe path, so the gap is strictly worsened by this PR.
- [BLOCKING] In `packages/rs-platform-wallet/examples/basic_usage.rs`:58-65: basic_usage example passes [u8;64] by value to create_wallet_from_seed_bytes — `cargo check --examples` fails
Verified at HEAD 0d8f69f0: `wallet_lifecycle.rs:106` requires `seed_bytes: &[u8; 64]`, but line 61 still passes `seed_bytes` by value. Rust does not auto-borrow ordinary function arguments, so `cargo check -p platform-wallet --examples` fails with `error[E0308]: mismatched types ... expected '&[u8; 64]', found '[u8; 64]'`. Library/test builds succeed (tests/spv_sync.rs was fixed in 454ae524), so the break is invisible to `cargo build -p platform-wallet`, but the example target — the canonical caller-facing reference for the zeroize-hardened seed entry point — is broken. Mechanical one-character fix.
- [BLOCKING] In `packages/rs-platform-wallet/examples/shielded_sync.rs`:240-248: shielded_sync example passes transparent_seed by value to create_wallet_from_seed_bytes
Verified at HEAD 0d8f69f0: companion to the basic_usage.rs finding. Line 243 still passes `transparent_seed` by value to a signature requiring `&[u8; 64]`. `cargo check -p platform-wallet --example shielded_sync --features shielded` fails with E0308. The sibling `shielded_sync_paloma.rs` callsite was migrated in a prior delta; this one was missed. Mechanical fix: prefix `&`.
- [SUGGESTION] In `packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:100-117: *seed_bytes deref materializes a non-zeroized [u8;64] stack copy, contradicting the method's documented invariant
Verified at HEAD 0d8f69f0: the doc comment at lines 103-105 explicitly asserts this method 'never owns a non-zeroized stack copy of the master secret — the only copy is the one consumed into key-wallet's Seed below'. Line 110 calls `Wallet::from_seed_bytes(*seed_bytes, network, accounts)`. Because `[u8; 64]` is Copy, dereferencing `&[u8; 64]` materializes a fresh 64-byte copy in this stack frame to pass by value, and that intermediate copy is not wrapped in `Zeroizing` and is not explicitly cleared on return. The FFI-side `Zeroizing` wrapper in `rs-platform-wallet-ffi/src/manager.rs` covers the Swift/C → Rust handoff, but this very next Rust frame past the FFI boundary defeats the defense-in-depth the wrapper exists to provide — and the wrapper's only value to a reviewer is the invariant this line silently violates. Exploitation requires a local memory disclosure vector (crash dumps, process-memory forensics, sibling memory-safety bug, swap capture), so this is defense-in-depth rather than directly exploitable; flagged because the doc-comment contract is explicit. Fix: either change `Wallet::from_seed_bytes` upstream in key-wallet to take `&[u8; 64]` and zeroize internally, or wrap locally with `let seed = zeroize::Zeroizing::new(*seed_bytes); Wallet::from_seed_bytes(*seed, network, accounts)`.
- [SUGGESTION] In `packages/rs-sdk-ffi/src/dashpay/contact_request.rs`:181-188: Create-only contact-request C ABI drops the entropy consensus requires to validate the document id
Verified at HEAD 0d8f69f0: the input contract at lines 169-176 was tightened in this PR to require the 69-byte DIP-15 compact form, and the Rust `ContactRequestResult` carries `entropy: Bytes32` because consensus recomputes the document id from the entropy in the document-create transition and rejects mismatches with `InvalidDocumentTransitionIdError`. `DashSDKContactRequestResult` still exposes only `document_id`, `owner_id`, `properties_json` — no entropy field — and the create-only constructor drops `result.entropy`. A non-Rust embedder calling `dash_sdk_dashpay_create_contact_request` and submitting through a generic document-put FFI has no way to recover the entropy required by consensus, so any staged create-then-broadcast flow across the C ABI silently produces transitions consensus will reject. The in-tree Swift caller uses the combined create+send entry point so the gap is dormant for this PR, but the public C ABI was tightened by this PR without exposing the field downstream callers now need. Fix: add `entropy: [u8; 32]` (or `*mut c_char` allocated via the existing string allocator) to `DashSDKContactRequestResult`, populate at construction, and regenerate the cbindgen header.
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
Latest delta (bba0a39) is a 2-line mechanical fix that resolves the two example seed-by-value callsites (prior #3 and #4). All four other prior findings re-verified against the worktree and carry forward: two blocking persistence/confidentiality defects (rejected-request rehydration on Rust SQLite backend; SwiftData wallet-wipe PHASE 1 missing two non-optional cascade children) and two suggestions (non-zeroized seed stack copy contradicting documented invariant; create-only contact-request C ABI dropping consensus-required entropy). No new defects introduced.
🔴 2 blocking | 🟡 2 suggestion(s)
4 additional finding(s)
blocking: record_rejected_contact_request never sets cs.removed_incoming — rejected requests rehydrate on the Rust SQLite backend
packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs (line 134)
The method removes the in-memory incoming_contact_requests entry (line 141) and populates only cs.rejected (lines 152–154); cs.removed_incoming is never set. Verified at packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:182-193, the SQLite writer issues DELETE FROM contacts ... only when cs.removed_incoming is non-empty; the cs.rejected branch (lines 233–255) writes only the tombstone-table row. The persisted state='received' row with its stale incoming_request blob therefore stays in SQLite after a reject, and the contacts loader rehydrates it as a live incoming entry on next load. The sync-side tombstone check only suppresses on-chain re-ingest of an additional copy, not local rehydration of the row already persisted in contacts. The SwiftData persister masks this with a bespoke deleteRejectedIncomingRow call, so the two backends silently diverge: any non-Swift host on the Rust SQLite backend silently un-rejects the request on every relaunch. Because the on-platform contactRequest document is immutable, this local tombstone+delete is the only suppression vector against a rejected (potentially abusive) sender; the user's explicit reject is silently undone after restart.
pub fn record_rejected_contact_request(
&mut self,
sender_id: &Identifier,
account_reference: u32,
document_id: Option<Identifier>,
) -> ContactChangeSet {
let owner_id = self.id();
self.incoming_contact_requests.remove(sender_id);
let tombstone = RejectedContactRequest {
owner_id,
sender_id: *sender_id,
account_reference,
document_id,
};
self.rejected_contact_requests
.insert((*sender_id, account_reference), tombstone.clone());
let mut cs = ContactChangeSet::default();
cs.removed_incoming.insert(ReceivedContactRequestKey {
owner_id,
sender_id: *sender_id,
});
cs.rejected
.insert((owner_id, *sender_id, account_reference), tombstone);
cs
}
blocking: Wallet-delete PHASE 1 omits dashpayPayments and dashpayRejectedRequests — SwiftData save() can fatal mid-wipe and leave plaintext rows on disk
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 3336)
The surrounding comment (lines 3312–3334) documents that PHASE 1 exists because SwiftData fatals during save() when it must null a non-optional inverse on a child processed in the same batch as its parent, and that PHASE 1 must pre-delete every cascade child of PersistentIdentity whose inverse to identity is non-optional. The loop pre-deletes only dpnsNames, dashpayProfile, and contactRequests. Two additional non-optional cascade children populated by this PR are missing (both verified at public var owner: PersistentIdentity): PersistentDashpayPayment.owner at PersistentDashpayPayment.swift:98 (populated via the Rust persister restore_dashpay_payments callback) and the newly-added PersistentDashpayRejectedRequest.owner at PersistentDashpayRejectedRequest.swift:80 (populated whenever a request is rejected). If either child exists at wipe time, PHASE 2's identity delete hits the same fatal PHASE 1 exists to prevent, aborting before the wallet row is removed. A user-initiated wallet wipe (device sale, handoff, end-of-life) deterministically leaves plaintext counterparty id, memo, amount, txid (payments) and privacy-relevant rejection tombstones on disk. This PR uniquely introduces PersistentDashpayRejectedRequest and wires it in without extending the wipe path, so the gap is strictly worsened here.
for identity in identitiesToDelete {
for name in Array(identity.dpnsNames) {
backgroundContext.delete(name)
}
if let profile = identity.dashpayProfile {
backgroundContext.delete(profile)
}
for cr in Array(identity.contactRequests) {
backgroundContext.delete(cr)
}
for payment in Array(identity.dashpayPayments) {
backgroundContext.delete(payment)
}
for tomb in Array(identity.dashpayRejectedRequests) {
backgroundContext.delete(tomb)
}
}
suggestion: *seed_bytes deref materializes a non-zeroized [u8;64] stack copy, contradicting the doc-comment invariant
packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs (line 100)
The doc comment at lines 103–105 explicitly asserts this method 'never owns a non-zeroized stack copy of the master secret — the only copy is the one consumed into key-wallet's Seed below'. Line 110 calls Wallet::from_seed_bytes(*seed_bytes, network, accounts). Because [u8; 64] is Copy, dereferencing &[u8; 64] materializes a fresh 64-byte byte-for-byte copy in this caller's stack frame to pass by value into from_seed_bytes; that intermediate copy is not wrapped in Zeroizing and is not explicitly cleared on return. The FFI-side Zeroizing wrapper in rs-platform-wallet-ffi/src/manager.rs covers the Swift/C → Rust handoff, but this Rust frame past the FFI boundary defeats the defense-in-depth that wrapper exists to provide — and the only reason the wrapper helps a reviewer is the invariant this line silently violates. Exploitation requires a local memory-disclosure vector (crash dumps, process-memory forensics, sibling memory-safety bug, swap capture), so this is defense-in-depth rather than directly exploitable; flagged because the doc-comment contract is explicit. Fix: change Wallet::from_seed_bytes upstream in key-wallet to take &[u8; 64] and zeroize internally, or wrap locally with let seed = zeroize::Zeroizing::new(*seed_bytes); Wallet::from_seed_bytes(*seed, network, accounts).
suggestion: Create-only contact-request C ABI drops the entropy required by consensus to validate the document id
packages/rs-sdk-ffi/src/dashpay/contact_request.rs (line 181)
The Rust ContactRequestResult carries entropy: Bytes32 because this PR's DIP-15 entropy threading makes consensus recompute the document id from the entropy in the document-create transition and reject mismatches with InvalidDocumentTransitionIdError. DashSDKContactRequestResult still exposes only document_id, owner_id, properties_json (verified at lines 181–188) — no entropy field — and the constructor at lines 435–439 drops result.entropy. The input contract at lines 169–176 was tightened by this PR to require the 69-byte DIP-15 compact form, but the output struct was not extended in parallel. A non-Rust embedder calling dash_sdk_dashpay_create_contact_request and submitting through a generic document-put FFI has no way to recover the entropy required by consensus, so any staged create-then-broadcast flow across the C ABI silently produces transitions consensus will reject. The in-tree Swift caller currently uses the combined create+send entry point so the gap is dormant for this PR, but the public C ABI was tightened by this PR without exposing the field downstream callers now need. Fix: add entropy: [u8; 32] (or *mut c_char allocated via the existing string allocator) to DashSDKContactRequestResult, populate at construction, and regenerate the cbindgen header.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:134-156: record_rejected_contact_request never sets cs.removed_incoming — rejected requests rehydrate on the Rust SQLite backend
The method removes the in-memory `incoming_contact_requests` entry (line 141) and populates only `cs.rejected` (lines 152–154); `cs.removed_incoming` is never set. Verified at `packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:182-193`, the SQLite writer issues `DELETE FROM contacts ...` only when `cs.removed_incoming` is non-empty; the `cs.rejected` branch (lines 233–255) writes only the tombstone-table row. The persisted `state='received'` row with its stale `incoming_request` blob therefore stays in SQLite after a reject, and the contacts loader rehydrates it as a live incoming entry on next load. The sync-side tombstone check only suppresses on-chain re-ingest of an additional copy, not local rehydration of the row already persisted in `contacts`. The SwiftData persister masks this with a bespoke `deleteRejectedIncomingRow` call, so the two backends silently diverge: any non-Swift host on the Rust SQLite backend silently un-rejects the request on every relaunch. Because the on-platform contactRequest document is immutable, this local tombstone+delete is the only suppression vector against a rejected (potentially abusive) sender; the user's explicit reject is silently undone after restart.
- [BLOCKING] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:3336-3346: Wallet-delete PHASE 1 omits dashpayPayments and dashpayRejectedRequests — SwiftData save() can fatal mid-wipe and leave plaintext rows on disk
The surrounding comment (lines 3312–3334) documents that PHASE 1 exists because SwiftData fatals during `save()` when it must null a non-optional inverse on a child processed in the same batch as its parent, and that PHASE 1 must pre-delete every cascade child of `PersistentIdentity` whose inverse to identity is non-optional. The loop pre-deletes only `dpnsNames`, `dashpayProfile`, and `contactRequests`. Two additional non-optional cascade children populated by this PR are missing (both verified at `public var owner: PersistentIdentity`): `PersistentDashpayPayment.owner` at `PersistentDashpayPayment.swift:98` (populated via the Rust persister `restore_dashpay_payments` callback) and the newly-added `PersistentDashpayRejectedRequest.owner` at `PersistentDashpayRejectedRequest.swift:80` (populated whenever a request is rejected). If either child exists at wipe time, PHASE 2's identity delete hits the same fatal PHASE 1 exists to prevent, aborting before the wallet row is removed. A user-initiated wallet wipe (device sale, handoff, end-of-life) deterministically leaves plaintext counterparty id, memo, amount, txid (payments) and privacy-relevant rejection tombstones on disk. This PR uniquely introduces `PersistentDashpayRejectedRequest` and wires it in without extending the wipe path, so the gap is strictly worsened here.
- [SUGGESTION] In `packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:100-117: *seed_bytes deref materializes a non-zeroized [u8;64] stack copy, contradicting the doc-comment invariant
The doc comment at lines 103–105 explicitly asserts this method 'never owns a non-zeroized stack copy of the master secret — the only copy is the one consumed into key-wallet's `Seed` below'. Line 110 calls `Wallet::from_seed_bytes(*seed_bytes, network, accounts)`. Because `[u8; 64]` is `Copy`, dereferencing `&[u8; 64]` materializes a fresh 64-byte byte-for-byte copy in this caller's stack frame to pass by value into `from_seed_bytes`; that intermediate copy is not wrapped in `Zeroizing` and is not explicitly cleared on return. The FFI-side `Zeroizing` wrapper in `rs-platform-wallet-ffi/src/manager.rs` covers the Swift/C → Rust handoff, but this Rust frame past the FFI boundary defeats the defense-in-depth that wrapper exists to provide — and the only reason the wrapper helps a reviewer is the invariant this line silently violates. Exploitation requires a local memory-disclosure vector (crash dumps, process-memory forensics, sibling memory-safety bug, swap capture), so this is defense-in-depth rather than directly exploitable; flagged because the doc-comment contract is explicit. Fix: change `Wallet::from_seed_bytes` upstream in key-wallet to take `&[u8; 64]` and zeroize internally, or wrap locally with `let seed = zeroize::Zeroizing::new(*seed_bytes); Wallet::from_seed_bytes(*seed, network, accounts)`.
- [SUGGESTION] In `packages/rs-sdk-ffi/src/dashpay/contact_request.rs`:181-188: Create-only contact-request C ABI drops the entropy required by consensus to validate the document id
The Rust `ContactRequestResult` carries `entropy: Bytes32` because this PR's DIP-15 entropy threading makes consensus recompute the document id from the entropy in the document-create transition and reject mismatches with `InvalidDocumentTransitionIdError`. `DashSDKContactRequestResult` still exposes only `document_id`, `owner_id`, `properties_json` (verified at lines 181–188) — no entropy field — and the constructor at lines 435–439 drops `result.entropy`. The input contract at lines 169–176 was tightened by this PR to require the 69-byte DIP-15 compact form, but the output struct was not extended in parallel. A non-Rust embedder calling `dash_sdk_dashpay_create_contact_request` and submitting through a generic document-put FFI has no way to recover the entropy required by consensus, so any staged create-then-broadcast flow across the C ABI silently produces transitions consensus will reject. The in-tree Swift caller currently uses the combined create+send entry point so the gap is dormant for this PR, but the public C ABI was tightened by this PR without exposing the field downstream callers now need. Fix: add `entropy: [u8; 32]` (or `*mut c_char` allocated via the existing string allocator) to `DashSDKContactRequestResult`, populate at construction, and regenerate the cbindgen header.
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 incremental review at HEAD 18a3696. All four prior findings re-verified against the worktree and remain valid: two blocking persistence/confidentiality defects (rejected-request rehydration on the Rust SQLite backend; SwiftData wallet-wipe PHASE 1 missing two non-optional cascade children) plus two suggestion-tier hardening items (non-zeroized seed stack copy contradicting documented invariant; create-only contact-request C ABI dropping consensus-required entropy). Dropped Codex's new V001-squash blocker: no other crate depends on rs-platform-wallet-storage and the iOS app uses SwiftData via FFI, so the unconsumed-pre-release-crate justification in the commit holds.
🔴 2 blocking | 🟡 2 suggestion(s)
4 additional finding(s)
blocking: record_rejected_contact_request leaves cs.removed_incoming empty — rejected requests rehydrate on the Rust SQLite backend
packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs (line 134)
The method drops the in-memory incoming_contact_requests entry at line 141 and populates only cs.rejected (lines 152-154); cs.removed_incoming is never set. The unified Rust SQLite contacts writer at packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:182-193 DELETEs from the contacts table only when cs.removed_incoming is non-empty; the cs.rejected branch (lines 233-255) writes only the tombstone-table row. The persisted state='received' row therefore stays in SQLite after a reject, and the contacts loader rehydrates it as a live incoming entry on next process start. The sync-side tombstone check only suppresses on-chain re-ingest, not local rehydration of the row already persisted in contacts. The SwiftData persister masks this with a bespoke deleteRejectedIncomingRow call, so the two backends silently diverge: any non-Swift host on the Rust SQLite backend silently un-rejects the request on every relaunch. Because the on-platform contactRequest document is immutable, this local tombstone+delete is the only suppression vector against a rejected (potentially abusive) sender; the user's explicit reject is silently undone after restart. The PR's own doc-comment at lines 123-133 advertises the tombstone prevents the recurring sync from resurrecting the row, but the persistence path is incomplete.
pub fn record_rejected_contact_request(
&mut self,
sender_id: &Identifier,
account_reference: u32,
document_id: Option<Identifier>,
) -> ContactChangeSet {
let owner_id = self.id();
self.incoming_contact_requests.remove(sender_id);
let tombstone = RejectedContactRequest {
owner_id,
sender_id: *sender_id,
account_reference,
document_id,
};
self.rejected_contact_requests
.insert((*sender_id, account_reference), tombstone.clone());
let mut cs = ContactChangeSet::default();
cs.removed_incoming.insert(ReceivedContactRequestKey {
owner_id,
sender_id: *sender_id,
});
cs.rejected
.insert((owner_id, *sender_id, account_reference), tombstone);
cs
}
blocking: Wallet-delete PHASE 1 omits dashpayPayments and dashpayRejectedRequests — SwiftData save() can fatal mid-wipe and leave plaintext rows on disk
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 3336)
The surrounding comment at lines 3312-3334 documents that PHASE 1 exists precisely because SwiftData fatals during save() when it must null a non-optional inverse on a child processed in the same batch as its parent, and that PHASE 1 must pre-delete every cascade child of PersistentIdentity whose inverse to identity is non-optional. The loop covers only dpnsNames, dashpayProfile, and contactRequests. Two additional non-optional cascade children populated by this PR are missing: PersistentDashpayPayment.owner: PersistentIdentity (populated on every load via restore_dashpay_payments) and the newly-added PersistentDashpayRejectedRequest.owner: PersistentIdentity (populated whenever the user rejects an incoming request, surfaced in the Storage Explorer view this PR adds). If either child exists at wipe time, PHASE 2's identity delete hits the same fatal PHASE 1 exists to prevent, aborting before the wallet row is removed. A user-initiated wallet wipe (device sale, handoff, end-of-life) then deterministically leaves plaintext counterparty id, memo, amount, and txid (payments) plus privacy-relevant rejection tombstones (the set of senders the user explicitly cut off) on disk. This PR uniquely introduces PersistentDashpayRejectedRequest and wires it through the explorer without extending the wipe path, so the gap is strictly worsened here.
for identity in identitiesToDelete {
for name in Array(identity.dpnsNames) {
backgroundContext.delete(name)
}
if let profile = identity.dashpayProfile {
backgroundContext.delete(profile)
}
for cr in Array(identity.contactRequests) {
backgroundContext.delete(cr)
}
for payment in Array(identity.dashpayPayments) {
backgroundContext.delete(payment)
}
for tomb in Array(identity.dashpayRejectedRequests) {
backgroundContext.delete(tomb)
}
}
suggestion: *seed_bytes deref materializes a non-zeroized [u8;64] stack copy, contradicting the method's documented invariant
packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs (line 100)
The doc-comment at lines 103-105 explicitly asserts this method 'never owns a non-zeroized stack copy of the master secret — the only copy is the one consumed into key-wallet's Seed below'. Line 110 calls Wallet::from_seed_bytes(*seed_bytes, network, accounts). Because [u8; 64] is Copy, dereferencing &[u8; 64] materializes a fresh 64-byte byte-for-byte copy in this caller's stack frame to pass by value into from_seed_bytes; that intermediate copy is not wrapped in Zeroizing and is not explicitly cleared on return. The FFI-side Zeroizing wrapper in rs-platform-wallet-ffi/src/manager.rs covers the Swift/C → Rust handoff, but this Rust frame past the FFI boundary defeats the defense-in-depth the wrapper exists to provide. Exploitation requires a local memory-disclosure vector (crash dumps, process-memory forensics, sibling memory-safety bug, swap capture), so this is defense-in-depth rather than directly exploitable; flagged because the doc-comment contract is explicit. Fix: change Wallet::from_seed_bytes upstream in key-wallet to take &[u8; 64] and zeroize internally, or wrap locally with let seed = zeroize::Zeroizing::new(*seed_bytes); Wallet::from_seed_bytes(*seed, network, accounts).
suggestion: Create-only contact-request C ABI drops the entropy consensus requires to validate the document id
packages/rs-sdk-ffi/src/dashpay/contact_request.rs (line 181)
The Rust ContactRequestResult carries entropy: Bytes32 because this PR's DIP-15 entropy threading makes consensus recompute the document id from the entropy in the document-create transition and reject mismatches with InvalidDocumentTransitionIdError. DashSDKContactRequestResult still exposes only document_id, owner_id, properties_json — no entropy field — and the create-only constructor drops result.entropy. The input contract at lines 169-176 was tightened by this PR to require the 69-byte DIP-15 compact form, but the output struct was not extended in parallel. A non-Rust embedder calling dash_sdk_dashpay_create_contact_request and submitting through a generic document-put FFI has no way to recover the entropy required by consensus, so any staged create-then-broadcast flow across the C ABI silently produces transitions consensus will reject with the exact bug this PR exists to fix. The in-tree Swift caller currently uses the combined create+send entry point so the gap is dormant for this PR, but the public C ABI was tightened by this PR without exposing the field downstream callers now need. Fix: add entropy: [u8; 32] (or *mut c_char allocated via the existing string allocator) to DashSDKContactRequestResult, populate at construction, and regenerate the cbindgen header.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:134-156: record_rejected_contact_request leaves cs.removed_incoming empty — rejected requests rehydrate on the Rust SQLite backend
The method drops the in-memory `incoming_contact_requests` entry at line 141 and populates only `cs.rejected` (lines 152-154); `cs.removed_incoming` is never set. The unified Rust SQLite contacts writer at packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:182-193 DELETEs from the `contacts` table only when `cs.removed_incoming` is non-empty; the `cs.rejected` branch (lines 233-255) writes only the tombstone-table row. The persisted `state='received'` row therefore stays in SQLite after a reject, and the contacts loader rehydrates it as a live incoming entry on next process start. The sync-side tombstone check only suppresses on-chain re-ingest, not local rehydration of the row already persisted in `contacts`. The SwiftData persister masks this with a bespoke `deleteRejectedIncomingRow` call, so the two backends silently diverge: any non-Swift host on the Rust SQLite backend silently un-rejects the request on every relaunch. Because the on-platform contactRequest document is immutable, this local tombstone+delete is the only suppression vector against a rejected (potentially abusive) sender; the user's explicit reject is silently undone after restart. The PR's own doc-comment at lines 123-133 advertises the tombstone prevents the recurring sync from resurrecting the row, but the persistence path is incomplete.
- [BLOCKING] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:3336-3346: Wallet-delete PHASE 1 omits dashpayPayments and dashpayRejectedRequests — SwiftData save() can fatal mid-wipe and leave plaintext rows on disk
The surrounding comment at lines 3312-3334 documents that PHASE 1 exists precisely because SwiftData fatals during `save()` when it must null a non-optional inverse on a child processed in the same batch as its parent, and that PHASE 1 must pre-delete every cascade child of `PersistentIdentity` whose inverse to identity is non-optional. The loop covers only `dpnsNames`, `dashpayProfile`, and `contactRequests`. Two additional non-optional cascade children populated by this PR are missing: `PersistentDashpayPayment.owner: PersistentIdentity` (populated on every load via `restore_dashpay_payments`) and the newly-added `PersistentDashpayRejectedRequest.owner: PersistentIdentity` (populated whenever the user rejects an incoming request, surfaced in the Storage Explorer view this PR adds). If either child exists at wipe time, PHASE 2's identity delete hits the same fatal PHASE 1 exists to prevent, aborting before the wallet row is removed. A user-initiated wallet wipe (device sale, handoff, end-of-life) then deterministically leaves plaintext counterparty id, memo, amount, and txid (payments) plus privacy-relevant rejection tombstones (the set of senders the user explicitly cut off) on disk. This PR uniquely introduces `PersistentDashpayRejectedRequest` and wires it through the explorer without extending the wipe path, so the gap is strictly worsened here.
- [SUGGESTION] In `packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:100-117: *seed_bytes deref materializes a non-zeroized [u8;64] stack copy, contradicting the method's documented invariant
The doc-comment at lines 103-105 explicitly asserts this method 'never owns a non-zeroized stack copy of the master secret — the only copy is the one consumed into key-wallet's `Seed` below'. Line 110 calls `Wallet::from_seed_bytes(*seed_bytes, network, accounts)`. Because `[u8; 64]` is `Copy`, dereferencing `&[u8; 64]` materializes a fresh 64-byte byte-for-byte copy in this caller's stack frame to pass by value into `from_seed_bytes`; that intermediate copy is not wrapped in `Zeroizing` and is not explicitly cleared on return. The FFI-side `Zeroizing` wrapper in `rs-platform-wallet-ffi/src/manager.rs` covers the Swift/C → Rust handoff, but this Rust frame past the FFI boundary defeats the defense-in-depth the wrapper exists to provide. Exploitation requires a local memory-disclosure vector (crash dumps, process-memory forensics, sibling memory-safety bug, swap capture), so this is defense-in-depth rather than directly exploitable; flagged because the doc-comment contract is explicit. Fix: change `Wallet::from_seed_bytes` upstream in key-wallet to take `&[u8; 64]` and zeroize internally, or wrap locally with `let seed = zeroize::Zeroizing::new(*seed_bytes); Wallet::from_seed_bytes(*seed, network, accounts)`.
- [SUGGESTION] In `packages/rs-sdk-ffi/src/dashpay/contact_request.rs`:181-188: Create-only contact-request C ABI drops the entropy consensus requires to validate the document id
The Rust `ContactRequestResult` carries `entropy: Bytes32` because this PR's DIP-15 entropy threading makes consensus recompute the document id from the entropy in the document-create transition and reject mismatches with `InvalidDocumentTransitionIdError`. `DashSDKContactRequestResult` still exposes only `document_id`, `owner_id`, `properties_json` — no entropy field — and the create-only constructor drops `result.entropy`. The input contract at lines 169-176 was tightened by this PR to require the 69-byte DIP-15 compact form, but the output struct was not extended in parallel. A non-Rust embedder calling `dash_sdk_dashpay_create_contact_request` and submitting through a generic document-put FFI has no way to recover the entropy required by consensus, so any staged create-then-broadcast flow across the C ABI silently produces transitions consensus will reject with the exact bug this PR exists to fix. The in-tree Swift caller currently uses the combined create+send entry point so the gap is dormant for this PR, but the public C ABI was tightened by this PR without exposing the field downstream callers now need. Fix: add `entropy: [u8; 32]` (or `*mut c_char` allocated via the existing string allocator) to `DashSDKContactRequestResult`, populate at construction, and regenerate the cbindgen header.
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.
…ectation #3886 (2026-06-14) raised the per-network protocol floors (min_protocol_version: Mainnet=11, others=12). The build-time clamp applies them as max(DEFAULT_INITIAL_PROTOCOL_VERSION, min_protocol_version(network)), which silently invalidated this test (added earlier by #3809): it still asserted the unpinned mock SDK seeds to DEFAULT_INITIAL_PROTOCOL_VERSION (10), but new_mock() defaults to Mainnet, so the seed is now 11. v3.1-dev has been red on this test since #3886; the DashPay merge pulled the breakage in. Assert the SDK's documented post-clamp contract instead (max(DEFAULT_INITIAL_PROTOCOL_VERSION, PROTOCOL_VERSION_11) == 11). Verified locally: left:11 right:10 ✖ before, 1 passed ✔ after. 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 7b94eab. The 18a3696..HEAD delta is a single non-functional test-expectation fix in rs-sdk; no new defects introduced. All four prior findings re-verified directly against the worktree and remain valid: two blocking persistence/confidentiality bugs (Rust-SQLite rejected-request rehydration; SwiftData wallet-wipe omits two non-optional cascade children added/touched by this PR) and two suggestion-tier hardening items (seed-by-ref deref defeats FFI-side Zeroizing; create-only contact-request C ABI drops consensus-required entropy).
🔴 2 blocking | 🟡 2 suggestion(s)
4 additional finding(s)
blocking: record_rejected_contact_request leaves cs.removed_incoming empty — rejected requests rehydrate on the Rust SQLite backend
packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs (line 134)
Verified at HEAD 7b94eab: line 141 removes the in-memory incoming_contact_requests entry, lines 152–154 populate only cs.rejected, and cs.removed_incoming is never set. The unified Rust SQLite contacts writer at packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:182-186 only issues DELETE FROM contacts ... when cs.removed_incoming is non-empty; the cs.rejected branch (line 233+) only upserts the tombstone-table row. The persisted state='received' row therefore stays in SQLite after a reject, and the contacts loader rehydrates it as a live incoming entry on next process start. Because the on-platform contactRequest document is immutable, this local tombstone+delete is the only suppression vector against a (potentially abusive) rejected sender — the user's explicit reject is silently undone after restart on the Rust SQLite backend. The SwiftData persister masks this with a bespoke deleteRejectedIncomingRow call, so the two backends silently diverge and the divergence is hidden in Swift-only CI. The PR's own doc comment at lines 123–133 advertises that the tombstone prevents the recurring sync from resurrecting the row, but the persistence path is incomplete.
pub fn record_rejected_contact_request(
&mut self,
sender_id: &Identifier,
account_reference: u32,
document_id: Option<Identifier>,
) -> ContactChangeSet {
let owner_id = self.id();
self.incoming_contact_requests.remove(sender_id);
let tombstone = RejectedContactRequest {
owner_id,
sender_id: *sender_id,
account_reference,
document_id,
};
self.rejected_contact_requests
.insert((*sender_id, account_reference), tombstone.clone());
let mut cs = ContactChangeSet::default();
cs.removed_incoming.insert(ReceivedContactRequestKey {
owner_id,
sender_id: *sender_id,
});
cs.rejected
.insert((owner_id, *sender_id, account_reference), tombstone);
cs
}
blocking: Wallet-delete PHASE 1 omits dashpayPayments and dashpayRejectedRequests — SwiftData save() can fatal mid-wipe and leave plaintext rows on disk
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 3336)
Verified at HEAD 7b94eab: the surrounding comment (lines 3312–3334) explicitly states PHASE 1 exists because SwiftData fatals during save() when it must null a non-optional inverse on a child processed in the same batch as its parent, and that PHASE 1 must pre-delete every cascade child of PersistentIdentity whose inverse to identity is non-optional. The loop at 3336–3346 covers only dpnsNames, dashpayProfile, and contactRequests. Confirmed in this run: PersistentDashpayPayment.owner: PersistentIdentity (PersistentDashpayPayment.swift:98) and PersistentDashpayRejectedRequest.owner: PersistentIdentity (PersistentDashpayRejectedRequest.swift:80) are both non-optional cascade children of PersistentIdentity. If either exists at wipe time — payments are populated on every load via the Rust restore_dashpay_payments callback, and rejection tombstones are populated whenever the user rejects an incoming request and surfaced in the Storage Explorer view this PR adds — PHASE 2's identity delete hits the same fatal PHASE 1 exists to prevent, aborting before the wallet row is removed. A user-initiated wallet wipe (device sale, handoff, end-of-life) deterministically leaves plaintext counterparty id, memo, amount, and txid (payments) plus privacy-relevant rejection tombstones on disk. This PR uniquely introduces PersistentDashpayRejectedRequest and wires it through the explorer without extending the wipe path, so the gap is strictly worsened here.
for identity in identitiesToDelete {
for name in Array(identity.dpnsNames) {
backgroundContext.delete(name)
}
if let profile = identity.dashpayProfile {
backgroundContext.delete(profile)
}
for cr in Array(identity.contactRequests) {
backgroundContext.delete(cr)
}
for payment in Array(identity.dashpayPayments) {
backgroundContext.delete(payment)
}
for tomb in Array(identity.dashpayRejectedRequests) {
backgroundContext.delete(tomb)
}
}
suggestion: *seed_bytes deref materializes a non-zeroized [u8;64] stack copy, contradicting the method's documented invariant
packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs (line 100)
Verified at HEAD 7b94eab: the doc comment at lines 103–105 explicitly asserts this method 'never owns a non-zeroized stack copy of the master secret — the only copy is the one consumed into key-wallet's Seed below'. Line 110 calls Wallet::from_seed_bytes(*seed_bytes, network, accounts). Because [u8; 64] is Copy, dereferencing &[u8; 64] materializes a fresh 64-byte byte-for-byte copy in this caller's stack frame to pass by value into from_seed_bytes; that intermediate copy is not wrapped in Zeroizing and is not explicitly cleared on return. The FFI-side Zeroizing<[u8; 64]> wrapper in rs-platform-wallet-ffi/src/manager.rs scrubs the Swift/C → Rust handoff, but this Rust frame one past the FFI boundary silently leaves an unzeroed image of the master seed on the stack, defeating the only invariant that wrapper buys reviewers. Exploitation requires a local memory-disclosure vector (crash dumps, process-memory forensics, sibling memory-safety bug, swap capture), so this is defense-in-depth rather than directly exploitable; flagged because the doc-comment contract is explicit. Fix: change Wallet::from_seed_bytes upstream in key-wallet to take &[u8; 64] and zeroize internally, or wrap locally with let seed = zeroize::Zeroizing::new(*seed_bytes); Wallet::from_seed_bytes(*seed, network, accounts).
suggestion: Create-only contact-request C ABI drops the entropy consensus requires to validate the document id
packages/rs-sdk-ffi/src/dashpay/contact_request.rs (line 181)
Verified at HEAD 7b94eab: the Rust ContactRequestResult carries entropy: Bytes32 because this PR's DIP-15 entropy threading makes consensus recompute the document id from the entropy in the document-create transition and reject mismatches with InvalidDocumentTransitionIdError — the exact bug this PR exists to fix. DashSDKContactRequestResult (lines 181–188) still exposes only document_id, owner_id, properties_json, and the create-only constructor at lines 435–439 drops result.entropy. The input contract at lines 169–176 was tightened by this PR to require the 69-byte DIP-15 compact form, but the output struct was not extended in parallel. A non-Rust embedder calling dash_sdk_dashpay_create_contact_request and submitting through a generic document-put FFI has no way to recover the entropy required by consensus, so any staged create-then-broadcast flow across the C ABI silently produces transitions consensus will reject with the exact InvalidDocumentTransitionIdError this PR exists to fix. The in-tree Swift caller uses the combined create+send entry point so the gap is dormant for first-party use, but the public C ABI was tightened by this PR without exposing the field downstream callers now need. Fix: add entropy: [u8; 32] (or *mut c_char allocated via the existing string allocator) to DashSDKContactRequestResult, populate at construction, and regenerate the cbindgen header.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:134-156: record_rejected_contact_request leaves cs.removed_incoming empty — rejected requests rehydrate on the Rust SQLite backend
Verified at HEAD 7b94eabd: line 141 removes the in-memory `incoming_contact_requests` entry, lines 152–154 populate only `cs.rejected`, and `cs.removed_incoming` is never set. The unified Rust SQLite contacts writer at `packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:182-186` only issues `DELETE FROM contacts ...` when `cs.removed_incoming` is non-empty; the `cs.rejected` branch (line 233+) only upserts the tombstone-table row. The persisted `state='received'` row therefore stays in SQLite after a reject, and the contacts loader rehydrates it as a live incoming entry on next process start. Because the on-platform contactRequest document is immutable, this local tombstone+delete is the only suppression vector against a (potentially abusive) rejected sender — the user's explicit reject is silently undone after restart on the Rust SQLite backend. The SwiftData persister masks this with a bespoke `deleteRejectedIncomingRow` call, so the two backends silently diverge and the divergence is hidden in Swift-only CI. The PR's own doc comment at lines 123–133 advertises that the tombstone prevents the recurring sync from resurrecting the row, but the persistence path is incomplete.
- [BLOCKING] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:3336-3346: Wallet-delete PHASE 1 omits dashpayPayments and dashpayRejectedRequests — SwiftData save() can fatal mid-wipe and leave plaintext rows on disk
Verified at HEAD 7b94eabd: the surrounding comment (lines 3312–3334) explicitly states PHASE 1 exists because SwiftData fatals during `save()` when it must null a non-optional inverse on a child processed in the same batch as its parent, and that PHASE 1 must pre-delete every cascade child of `PersistentIdentity` whose inverse to identity is non-optional. The loop at 3336–3346 covers only `dpnsNames`, `dashpayProfile`, and `contactRequests`. Confirmed in this run: `PersistentDashpayPayment.owner: PersistentIdentity` (PersistentDashpayPayment.swift:98) and `PersistentDashpayRejectedRequest.owner: PersistentIdentity` (PersistentDashpayRejectedRequest.swift:80) are both non-optional cascade children of `PersistentIdentity`. If either exists at wipe time — payments are populated on every load via the Rust `restore_dashpay_payments` callback, and rejection tombstones are populated whenever the user rejects an incoming request and surfaced in the Storage Explorer view this PR adds — PHASE 2's identity delete hits the same fatal PHASE 1 exists to prevent, aborting before the wallet row is removed. A user-initiated wallet wipe (device sale, handoff, end-of-life) deterministically leaves plaintext counterparty id, memo, amount, and txid (payments) plus privacy-relevant rejection tombstones on disk. This PR uniquely introduces `PersistentDashpayRejectedRequest` and wires it through the explorer without extending the wipe path, so the gap is strictly worsened here.
- [SUGGESTION] In `packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:100-117: *seed_bytes deref materializes a non-zeroized [u8;64] stack copy, contradicting the method's documented invariant
Verified at HEAD 7b94eabd: the doc comment at lines 103–105 explicitly asserts this method 'never owns a non-zeroized stack copy of the master secret — the only copy is the one consumed into key-wallet's `Seed` below'. Line 110 calls `Wallet::from_seed_bytes(*seed_bytes, network, accounts)`. Because `[u8; 64]` is `Copy`, dereferencing `&[u8; 64]` materializes a fresh 64-byte byte-for-byte copy in this caller's stack frame to pass by value into `from_seed_bytes`; that intermediate copy is not wrapped in `Zeroizing` and is not explicitly cleared on return. The FFI-side `Zeroizing<[u8; 64]>` wrapper in `rs-platform-wallet-ffi/src/manager.rs` scrubs the Swift/C → Rust handoff, but this Rust frame one past the FFI boundary silently leaves an unzeroed image of the master seed on the stack, defeating the only invariant that wrapper buys reviewers. Exploitation requires a local memory-disclosure vector (crash dumps, process-memory forensics, sibling memory-safety bug, swap capture), so this is defense-in-depth rather than directly exploitable; flagged because the doc-comment contract is explicit. Fix: change `Wallet::from_seed_bytes` upstream in key-wallet to take `&[u8; 64]` and zeroize internally, or wrap locally with `let seed = zeroize::Zeroizing::new(*seed_bytes); Wallet::from_seed_bytes(*seed, network, accounts)`.
- [SUGGESTION] In `packages/rs-sdk-ffi/src/dashpay/contact_request.rs`:181-188: Create-only contact-request C ABI drops the entropy consensus requires to validate the document id
Verified at HEAD 7b94eabd: the Rust `ContactRequestResult` carries `entropy: Bytes32` because this PR's DIP-15 entropy threading makes consensus recompute the document id from the entropy in the document-create transition and reject mismatches with `InvalidDocumentTransitionIdError` — the exact bug this PR exists to fix. `DashSDKContactRequestResult` (lines 181–188) still exposes only `document_id`, `owner_id`, `properties_json`, and the create-only constructor at lines 435–439 drops `result.entropy`. The input contract at lines 169–176 was tightened by this PR to require the 69-byte DIP-15 compact form, but the output struct was not extended in parallel. A non-Rust embedder calling `dash_sdk_dashpay_create_contact_request` and submitting through a generic document-put FFI has no way to recover the entropy required by consensus, so any staged create-then-broadcast flow across the C ABI silently produces transitions consensus will reject with the exact `InvalidDocumentTransitionIdError` this PR exists to fix. The in-tree Swift caller uses the combined create+send entry point so the gap is dormant for first-party use, but the public C ABI was tightened by this PR without exposing the field downstream callers now need. Fix: add `entropy: [u8; 32]` (or `*mut c_char` allocated via the existing string allocator) to `DashSDKContactRequestResult`, populate at construction, and regenerate the cbindgen header.
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)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