fix(platform-wallet)!: dashpay sync correctness, mobile interop, payments + DashPay tab (M1+M2)#3841
fix(platform-wallet)!: dashpay sync correctness, mobile interop, payments + DashPay tab (M1+M2)#3841shumkov wants to merge 23 commits into
Conversation
…earch Seven-agent reviewed spec for completing the full DashPay flow (sync, contact requests, payments, profiles) in the platform wallet + SwiftExampleApp: protocol reference (DIP-9/11/13/14/15), per-layer implementation inventory, 15 prioritized gaps (G1-G15), 5-milestone work plan, Swift UI design with normative interaction states, and a two-tier test plan aligned with the unmerged e2e framework (PR #3549). Backed by 6 source-cited research files, including the cross-client interop desk-check and an on-chain census of all 368 testnet contactRequest documents. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ompact xpub, key-purpose interop Three fixes to the rs-sdk/platform-encryption contact-request layer, each pinned red-to-green: 1. Entropy mismatch (consensus rejection). send_contact_request generated fresh entropy for broadcast while the document id was derived from the creation entropy; drive-abci recomputes the id from the broadcast entropy and rejected EVERY send with InvalidDocumentTransitionIdError. ContactRequestResult now carries the creation entropy and send reuses it. Test: contact_request_result_entropy_derives_returned_id (red: field inexpressible pre-fix; green after). 2. DIP-15 69-byte compact xpub wire format. We encrypted the 107-byte DIP-14 ExtendedPubKey::encode() form (failing our own 96-byte ciphertext check); DIP-15 and both reference mobile clients use fingerprint||chaincode||pubkey = 69 bytes. New compact_xpub_bytes/parse_compact_xpub codec in platform-encryption; the get_extended_public_key callback contract is now the 69-byte compact, validated before encryption. Test: test_encrypt_compact_xpub_is_exactly_96_bytes (+ round-trip and wrong-length rejection). 3. Key-purpose alignment with on-chain reality. Verified against all 368 testnet contactRequests: the dominant mobile cohort references an ENCRYPTION key for BOTH indices (mobile identities carry no DECRYPTION key). The recipient-key assertion now accepts DECRYPTION or ENCRYPTION. Test: recipient_key_purpose_accepts_decryption_and_encryption (red on DECRYPTION-only predicate; green after). BREAKING: the SDK-side get_extended_public_key callback must now return the 69-byte DIP-15 compact form (rs-sdk-ffi C ABI unchanged; caller doc contract tightened). Also enables dashcore/rand in platform-encryption dev-deps — the crate's tests previously failed to compile at all. dash-sdk: 139 lib tests green (mocks,offline-testing); platform-encryption 7/7; rs-sdk-ffi check clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…blish/reconcile, account rebuild Milestone 1 of docs/dashpay/SPEC.md. Makes DashPay sync actually converge to a payable state, recurring, and restore-safe. Each behavior pinned red-to-green (see SPEC.md Part 5 M1 DONE notes for the full test list). - Recurring sync (G12): new DashPaySyncManager (modeled on PlatformAddressSyncManager) drives dashpay_sync() per wallet on the shared cadence/cancel/quiesce machinery — iterating the wallets map, NOT the token registry (which skips zero-token identities). Per-identity log-and-continue pushed into sync_contact_requests. Test: recurring_pass_syncs_every_wallet_including_zero_token_identities. - Establish via sync (G1a): the ingest guard dropped reciprocal requests whose sender we had already sent to — the offline-accept scenario could never establish. Guard relaxed; reciprocals now flow into auto-establish. - Sent-side reconcile (G13): sync now ingests our own on-platform sent requests (idempotent, metadata-preserving merge — naive re-establish wiped alias/note every sweep), and Accept adopts an existing reciprocal instead of re-broadcasting into the unique-index rejection that permanently bricked Accept after restore-from-seed. - Account rebuild sweep (G1b): every established contact missing accounts gets validate-key-indices -> decrypt -> register external account, plus the DashpayReceivingFunds account (previously only created on fresh send, so restore-from-seed left incoming payments invisible). Candidates collected under the write guard, registered after guard drop (tokio RwLock is non-reentrant). - Failure policy (G1c): transient failures retry next sweep; permanent decrypt/parse failures set the new EstablishedContact.payment_channel_broken flag (persisted; FFI accessor added) and stop retrying. Purpose-validation mismatches only log-and-skip. - Reject tombstone (G5 stage 1): rejected requests are tombstoned by (owner, sender, accountReference) — never bare sender, so a rotated request with a bumped accountReference still gets through. New rejected_contact_requests table + ContactChangeSet.rejected. - Receive-side compact xpub (G14): register_external_contact_account parses the 69-byte DIP-15 compact and reconstructs the contact xpub (address-equality pinned by reconstructed_xpub_derives_identical_addresses); legacy 78/107 fallback kept. - Key-purpose envelope (G15, verified on-chain): send prefers the recipient's DECRYPTION key and falls back to ENCRYPTION (mobile identities have no DECRYPTION key); validate_contact_request gains a recipient purpose gate (AUTHENTICATION was silently accepted before) and a purpose_mismatch classification. - Testability seam (G11): DashPaySdkWriter object-safe trait over the SDK write paths; fetch paths use the SDK's built-in mock. platform-wallet: 196 lib + 8 integration tests green (was 170); storage + FFI checks clean; FFI ABI extended by one accessor (established_contact_is_payment_channel_broken). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
shumkov
requested review from
QuantumExplorer,
ZocoLini,
lklimek and
llbartekll
as code owners
|
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 9441316) |
![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
| `autoAcceptProof` (optional, 38–102 bytes) lets a recipient pre-authorize automatic acceptance. | ||
| DIP-0015 defines a **separate** derivation path for the auto-accept proof keys: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Specify code-fence languages for markdown lint compliance.
Please add explicit language tags to these fenced blocks (at least text where no specific language applies).
Also applies to: 194-194, 245-245, 289-289, 418-418, 455-455
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 131-131: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/dashpay/research/01-dip-spec.md` at 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.
Source: Linters/SAST tools
|
|
||
| `test_dashpay_vector_1..4` derive against real DashPay-shaped paths, e.g.: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Missing language tags on fenced code blocks.
Add language identifiers to keep markdownlint clean and preserve proper syntax highlighting.
Also applies to: 394-394
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 232-232: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/dashpay/research/02-rust-dashcore-keywallet.md` at 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.
Source: Linters/SAST tools
|
|
||
| `ContentView.swift` is the root. A 5-tab `TabView` (enum `RootTab`): | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced block.
This block should declare a language (or text) to satisfy markdownlint.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 47-47: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/dashpay/research/05-swift-app.md` at 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.
Source: Linters/SAST tools
| | # | Item | Verdict | | ||
| |---|------|---------| | ||
| | 1 | encryptedPublicKey plaintext layout | **FAIL** — ours is a 107-byte DIP-14 serialization; spec + both reference clients use the 69-byte compact (`fingerprint(4) ‖ chainCode(32) ‖ pubKey(33)`). Our current send path cannot even produce a valid document (128-byte ciphertext vs the contract's hard 96). Our receive path rejects reference-client payloads. | | ||
| | 2 | ECDH shared-key derivation | **PASS** — all three stacks compute libsecp256k1-style `SHA256((y[31]&0x1|0x2) ‖ x)`. | |
There was a problem hiding this comment.
Fix table column mismatch in the verdict summary.
The row includes an extra column relative to the 3-column header, which can drop content in renderers/linters. Align all rows to the header shape.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 24-24: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 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 `@docs/dashpay/research/06-interop-desk-check.md` at 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.
Source: Linters/SAST tools
| bundle: **`https://testnet.platform-explorer.pshenmic.dev`** (routes in | ||
| `pshenmic/platform-explorer` `packages/api/src/routes.js`). Endpoints used: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Specify the language for the fenced block.
Add a language tag (http, text, etc.) to satisfy markdownlint and improve readability.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 366-366: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/dashpay/research/06-interop-desk-check.md` at 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.
Source: Linters/SAST tools
| if let Ok(mut guard) = this.background_cancel.lock() { | ||
| *guard = None; | ||
| } |
There was a problem hiding this comment.
Prevent old-loop cleanup from clearing a newer loop’s cancel token
Line 221 clears background_cancel unconditionally. If stop() (Line 238) is followed by a quick start(), the old thread can exit later and overwrite the new token with None, leaving the new loop untracked and potentially un-stoppable.
Suggested fix
pub struct DashPaySyncManager {
wallets: Arc<RwLock<BTreeMap<WalletId, Arc<PlatformWallet>>>>,
- background_cancel: StdMutex<Option<CancellationToken>>,
+ background_cancel: StdMutex<Option<(u64, CancellationToken)>>,
+ run_id: AtomicU64,
interval_secs: AtomicU64,
is_syncing: AtomicBool,
quiescing: AtomicBool,
last_sync_unix: AtomicU64,
}
impl DashPaySyncManager {
pub fn new(wallets: Arc<RwLock<BTreeMap<WalletId, Arc<PlatformWallet>>>>) -> Self {
Self {
wallets,
background_cancel: StdMutex::new(None),
+ run_id: AtomicU64::new(1),
interval_secs: AtomicU64::new(DEFAULT_SYNC_INTERVAL_SECS),
is_syncing: AtomicBool::new(false),
quiescing: AtomicBool::new(false),
last_sync_unix: AtomicU64::new(0),
}
}
pub fn start(self: Arc<Self>) {
let mut guard = self.background_cancel.lock().expect("bg_cancel poisoned");
if guard.is_some() {
return;
}
let cancel = CancellationToken::new();
- *guard = Some(cancel.clone());
+ let run_id = self.run_id.fetch_add(1, Ordering::Relaxed);
+ *guard = Some((run_id, cancel.clone()));
drop(guard);
let handle = tokio::runtime::Handle::current();
let this = self;
std::thread::Builder::new()
.name("dashpay-sync".into())
.spawn(move || {
handle.block_on(async move {
@@
- if let Ok(mut guard) = this.background_cancel.lock() {
- *guard = None;
+ if let Ok(mut guard) = this.background_cancel.lock() {
+ if guard.as_ref().map(|(id, _)| *id) == Some(run_id) {
+ *guard = None;
+ }
}
});
})
.expect("failed to spawn dashpay-sync thread");
}
pub fn stop(&self) {
- if let Some(token) = self
+ if let Some((_, token)) = self
.background_cancel
.lock()
.expect("bg_cancel poisoned")
.take()
{
token.cancel();
}
}
}Also applies to: 238-245
🤖 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/manager/dashpay_sync.rs` around lines 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.
| fn parse_contact_request_doc( | ||
| doc: &dpp::document::Document, | ||
| sender_id: Identifier, | ||
| recipient_id: Identifier, | ||
| ) -> Option<ContactRequest> { | ||
| let props = doc.properties(); | ||
| let sender_key_index = props | ||
| .get("senderKeyIndex") | ||
| .and_then(|v: &Value| v.to_integer::<u32>().ok()); | ||
| let recipient_key_index = props | ||
| .get("recipientKeyIndex") | ||
| .and_then(|v: &Value| v.to_integer::<u32>().ok()); | ||
| let account_reference = props | ||
| .get("accountReference") | ||
| .and_then(|v: &Value| v.to_integer::<u32>().ok()); | ||
| let encrypted_public_key = props | ||
| .get("encryptedPublicKey") | ||
| .and_then(|v: &Value| v.as_bytes()) | ||
| .cloned(); | ||
|
|
||
| match ( | ||
| sender_key_index, | ||
| recipient_key_index, | ||
| account_reference, | ||
| encrypted_public_key, | ||
| ) { | ||
| (Some(ski), Some(rki), Some(ar), Some(epk)) => Some(ContactRequest::new( | ||
| sender_id, | ||
| recipient_id, | ||
| ski, | ||
| rki, | ||
| ar, | ||
| epk, | ||
| doc.created_at_core_block_height().unwrap_or(0), | ||
| doc.created_at().unwrap_or(0), | ||
| )), | ||
| _ => { | ||
| tracing::warn!( | ||
| sender = %sender_id, | ||
| recipient = %recipient_id, | ||
| "Skipping contact request document: missing required field" | ||
| ); | ||
| None | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Parse our own sent `contactRequest` document into a [`ContactRequest`] | ||
| /// (owner is us, recipient is `toUserId`). | ||
| fn parse_sent_contact_request_doc( | ||
| doc: &dpp::document::Document, | ||
| owner_id: Identifier, | ||
| recipient_id: Identifier, | ||
| ) -> Option<ContactRequest> { | ||
| // Same field set as the received side; the only difference is which | ||
| // identity is owner vs recipient. | ||
| Self::parse_contact_request_doc(doc, owner_id, recipient_id) | ||
| } |
There was a problem hiding this comment.
Sync ingest is dropping optional contact-request payloads.
parse_contact_request_doc() only preserves the required fields, so both the received-side ingest and the sent-side reconcile path lose encryptedAccountLabel and autoAcceptProof even though ContactRequest carries them and sent_contact_requests() parses them later in this file. After a restore/sync sweep those values become None, which breaks metadata round-tripping and any auto-accept behavior that depends on the proof.
🤖 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 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.
| 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.rejected | ||
| .insert((owner_id, *sender_id, account_reference), tombstone); | ||
| cs |
There was a problem hiding this comment.
Persist the incoming-removal when recording a rejection tombstone.
At Line 116, the method removes the incoming request from in-memory state, but the returned changeset (Line 127-Line 130) only contains rejected. On replay, rejected does not remove incoming entries, so rejected requests can reappear after restore.
Proposed fix
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 removed_incoming = self.incoming_contact_requests.remove(sender_id).is_some();
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();
+ if removed_incoming {
+ cs.removed_incoming.insert(ReceivedContactRequestKey {
+ owner_id,
+ sender_id: *sender_id,
+ });
+ }
cs.rejected
.insert((owner_id, *sender_id, account_reference), tombstone);
cs
}🤖 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/state/managed_identity/contact_requests.rs`
around lines 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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3841 +/- ##
=============================================
+ Coverage 70.73% 87.22% +16.49%
=============================================
Files 20 2641 +2621
Lines 2788 328569 +325781
=============================================
+ Hits 1972 286597 +284625
- Misses 816 41972 +41156
🚀 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; }`).
| note TEXT, | ||
| is_hidden INTEGER, | ||
| accepted_accounts BLOB, | ||
| payment_channel_broken INTEGER, | ||
| updated_at INTEGER NOT NULL DEFAULT (unixepoch()), | ||
| PRIMARY KEY (wallet_id, owner_id, contact_id), | ||
| FOREIGN KEY (wallet_id) REFERENCES wallet_metadata(wallet_id) ON DELETE CASCADE | ||
| ); | ||
|
|
||
| -- Rejected-request tombstone (G5 stage 1). Keyed by | ||
| -- `(wallet_id, owner_id, sender_id, account_reference)` — NOT bare | ||
| -- sender id — so a once-rejected sender can still re-request via a | ||
| -- bumped accountReference (the DIP-15 rotation mechanism), while a | ||
| -- replay of the exact same immutable request stays suppressed. The | ||
| -- optional `document_id` records the rejected document's id for audit / | ||
| -- exact-match suppression. The sync ingest path consults this table | ||
| -- before re-ingesting a received contactRequest. | ||
| CREATE TABLE rejected_contact_requests ( | ||
| wallet_id BLOB NOT NULL, | ||
| owner_id BLOB NOT NULL, | ||
| sender_id BLOB NOT NULL, | ||
| account_reference INTEGER NOT NULL, | ||
| document_id BLOB, | ||
| rejected_at INTEGER NOT NULL DEFAULT (unixepoch()), | ||
| PRIMARY KEY (wallet_id, owner_id, sender_id, account_reference), | ||
| FOREIGN KEY (wallet_id) REFERENCES wallet_metadata(wallet_id) ON DELETE CASCADE | ||
| ); | ||
|
|
There was a problem hiding this comment.
🔴 Blocking: 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 da9d3fe / 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.
source: ['claude']
There was a problem hiding this comment.
Resolved in this update — V001 migration edited in-place violates append-only policy and breaks upgrade from v4.0.0-beta.4 no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| 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.rejected | ||
| .insert((owner_id, *sender_id, account_reference), tombstone); | ||
| cs | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: 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 DELETEs 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.
| 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.rejected | |
| .insert((owner_id, *sender_id, account_reference), tombstone); | |
| cs | |
| } | |
| 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 |
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in this update — record_rejected_contact_request removes incoming in memory but does not emit removed_incoming no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| if let Err(e) = self | ||
| .register_external_contact_account( | ||
| identity_id, | ||
| &contact_id, | ||
| &candidate.encrypted_public_key, | ||
| candidate.our_decryption_key_index, | ||
| candidate.contact_encryption_key_index, | ||
| ) | ||
| .await | ||
| { | ||
| tracing::warn!( | ||
| identity = %identity_id, | ||
| contact = %contact_id, | ||
| error = %e, | ||
| "Failed to register DashPay external account; marking payment channel broken (permanent)" | ||
| ); | ||
| self.mark_contact_channel_broken(identity_id, &contact_id) | ||
| .await; |
There was a problem hiding this comment.
🔴 Blocking: 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).
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Transient identity fetch failures inside account-build are marked as permanent no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| self.is_valid = false; | ||
| } | ||
|
|
||
| /// Add a key-PURPOSE error: sets `is_valid = false` AND flags | ||
| /// `purpose_mismatch` so callers can downgrade this to a non-permanent | ||
| /// skip rather than a permanent broken-channel mark (G15). | ||
| pub fn add_purpose_error(&mut self, error: String) { | ||
| self.errors.push(error); | ||
| self.is_valid = false; | ||
| self.purpose_mismatch = true; | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: 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.
| self.is_valid = false; | |
| } | |
| /// Add a key-PURPOSE error: sets `is_valid = false` AND flags | |
| /// `purpose_mismatch` so callers can downgrade this to a non-permanent | |
| /// skip rather than a permanent broken-channel mark (G15). | |
| pub fn add_purpose_error(&mut self, error: String) { | |
| self.errors.push(error); | |
| self.is_valid = false; | |
| self.purpose_mismatch = true; | |
| } | |
| pub fn add_error(&mut self, error: String) { | |
| self.errors.push(error); | |
| self.is_valid = false; | |
| self.purpose_mismatch = false; | |
| } | |
| /// Add a key-PURPOSE error: sets `is_valid = false` AND flags | |
| /// `purpose_mismatch` so callers can downgrade this to a non-permanent | |
| /// skip rather than a permanent broken-channel mark (G15). | |
| 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; | |
| } |
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — purpose_mismatch is set even when a non-purpose hard error is also present no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| // G1a: do NOT skip just because the sender is in | ||
| // `sent_contact_requests` — that is the reciprocal we | ||
| // need to let through to auto-establish. Skip only when | ||
| // already tracked as incoming or established (true | ||
| // dedup), or suppressed by a rejected-request tombstone. | ||
| if managed.incoming_contact_requests.contains_key(&sender_id) | ||
| || managed.established_contacts.contains_key(&sender_id) | ||
| { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: 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.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Superseding requests from established contacts are skipped — payment_channel_broken recovery is unreachable no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| pub fn start(self: Arc<Self>) { | ||
| let mut guard = self.background_cancel.lock().expect("bg_cancel poisoned"); | ||
| if guard.is_some() { | ||
| return; | ||
| } | ||
| let cancel = CancellationToken::new(); | ||
| *guard = Some(cancel.clone()); | ||
| drop(guard); | ||
|
|
||
| let handle = tokio::runtime::Handle::current(); | ||
| let this = self; | ||
| std::thread::Builder::new() | ||
| .name("dashpay-sync".into()) | ||
| .spawn(move || { | ||
| handle.block_on(async move { | ||
| loop { | ||
| if cancel.is_cancelled() { | ||
| break; | ||
| } | ||
|
|
||
| this.sync_now().await; | ||
|
|
||
| let interval = this.interval(); | ||
| tokio::select! { | ||
| _ = tokio::time::sleep(interval) => {} | ||
| _ = cancel.cancelled() => break, | ||
| } | ||
| } | ||
|
|
||
| if let Ok(mut guard) = this.background_cancel.lock() { | ||
| *guard = None; | ||
| } | ||
| }); | ||
| }) | ||
| .expect("failed to spawn dashpay-sync thread"); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: 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; }).
source: ['claude']
There was a problem hiding this comment.
Resolved in 4e0bffa — stop() followed quickly by start() can let the old thread null out the new cancel token no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…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
| // Same guard as the other app-facing writers | ||
| // (`setWalletName`, …): a refresh landing while a Rust | ||
| // persister round is open must ride that round's | ||
| // endChangeset commit/rollback instead of flushing the | ||
| // half-applied round early. | ||
| if !self.inChangeset { try? backgroundContext.save() } | ||
| } |
There was a problem hiding this comment.
Surface save() failures in DashPay payment persistence.
persistDashpayPayments currently swallows backgroundContext.save() failures via try?, which can silently drop payment-history updates without telemetry or caller visibility.
Suggested fix
- if !self.inChangeset { try? backgroundContext.save() }
+ if !self.inChangeset {
+ do {
+ try backgroundContext.save()
+ } catch {
+ NSLog(
+ "[persistor:swift] persistDashpayPayments save failed: %@",
+ String(describing: error)
+ )
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Same guard as the other app-facing writers | |
| // (`setWalletName`, …): a refresh landing while a Rust | |
| // persister round is open must ride that round's | |
| // endChangeset commit/rollback instead of flushing the | |
| // half-applied round early. | |
| if !self.inChangeset { try? backgroundContext.save() } | |
| } | |
| // Same guard as the other app-facing writers | |
| // (`setWalletName`, …): a refresh landing while a Rust | |
| // persister round is open must ride that round's | |
| // endChangeset commit/rollback instead of flushing the | |
| // half-applied round early. | |
| if !self.inChangeset { | |
| do { | |
| try backgroundContext.save() | |
| } catch { | |
| NSLog( | |
| "[persistor:swift] persistDashpayPayments save failed: %@", | |
| String(describing: error) | |
| ) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`
around lines 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.
| /// §6.4 optimistic overlay for *send*: contact ids whose request | ||
| /// was just broadcast but whose outgoing row hasn't landed via | ||
| /// the persister yet. Rendered as synthetic "Pending" rows in the | ||
| /// Outgoing section; pruned there when the query catches up or a | ||
| /// sync pass completes. | ||
| @State private var optimisticSentIds: Set<Data> = [] |
There was a problem hiding this comment.
Reset identity-scoped UI state when the active identity changes.
optimisticSentIds and ownProfile survive a picker switch, and loadOwnProfileFromCache() explicitly keeps the previous profile on a read failure. That means switching from identity A to B before the persister/cache catches up can render A's pending request overlay or profile header under B. Clear those fields at the start of the activeIdentity task, or key them by owner identity instead of storing one tab-wide value.
Also applies to: 44-48, 169-177, 389-402
🤖 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 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.
| Section("Avatar URL") { | ||
| TextField("https://…", text: $avatarUrl) | ||
| .keyboardType(.URL) | ||
| .textInputAutocapitalization(.never) | ||
| .autocorrectionDisabled() | ||
| .accessibilityIdentifier("dashpay.profile.avatarUrl") | ||
| Text("Paste an HTTPS image URL. SHA-256 + dHash " + | ||
| "are computed client-side when you save — see " + | ||
| "DIP-15.") |
There was a problem hiding this comment.
Enforce the HTTPS-only avatar rule in code.
The form tells users to paste an HTTPS avatar URL, but fetchAvatarBytes currently accepts any parseable URL. That lets http: URLs be fetched in cleartext and admits schemes this downloader should never touch. Reject anything whose scheme is not https before building the request.
Suggested fix
private func fetchAvatarBytes(urlString: String) async throws -> Data {
- guard let url = URL(string: urlString) else {
+ guard let url = URL(string: urlString),
+ url.scheme?.lowercased() == "https" else {
throw DashPayProfileEditorError.avatarFetchFailed("Invalid URL: \(urlString)")
}
var request = URLRequest(url: url)
request.timeoutInterval = Self.avatarFetchTimeoutAlso applies to: 1390-1410
🤖 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/IdentityDetailView.swift`
around lines 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.
| let refresh = app.buttons | ||
| .matching(identifier: Identifier.refreshButton).firstMatch | ||
| XCTAssertTrue( | ||
| refresh.exists, | ||
| "dashpay.refresh toolbar button must exist alongside the segment." | ||
| ) |
There was a problem hiding this comment.
Wait for the refresh button instead of checking .exists immediately.
After the DashPay segment appears, the toolbar can still land a moment later on slower simulators. refresh.exists makes this smoke test flaky; use waitForExistence(timeout:) here like the other assertions.
Suggested fix
let refresh = app.buttons
.matching(identifier: Identifier.refreshButton).firstMatch
XCTAssertTrue(
- refresh.exists,
+ refresh.waitForExistence(timeout: 10),
"dashpay.refresh toolbar button must exist alongside the segment."
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let refresh = app.buttons | |
| .matching(identifier: Identifier.refreshButton).firstMatch | |
| XCTAssertTrue( | |
| refresh.exists, | |
| "dashpay.refresh toolbar button must exist alongside the segment." | |
| ) | |
| let refresh = app.buttons | |
| .matching(identifier: Identifier.refreshButton).firstMatch | |
| XCTAssertTrue( | |
| refresh.waitForExistence(timeout: 10), | |
| "dashpay.refresh toolbar button must exist alongside the segment." | |
| ) |
🤖 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/SwiftExampleAppUITests/DashPayTabUITests.swift`
around lines 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.
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
| private var orderedAccounts: [PersistentAccount] { | ||
| accounts.sorted { lhs, rhs in | ||
| let lhsKey = AccountListView.sortKey(for: lhs) | ||
| let rhsKey = AccountListView.sortKey(for: rhs) | ||
| return lhsKey < rhsKey | ||
| } | ||
| accounts | ||
| .filter { $0.accountType != 12 && $0.accountType != 13 } | ||
| .sorted { lhs, rhs in | ||
| let lhsKey = AccountListView.sortKey(for: lhs) | ||
| let rhsKey = AccountListView.sortKey(for: rhs) | ||
| return lhsKey < rhsKey | ||
| } |
There was a problem hiding this comment.
Keep the empty-state checks in sync with the filtered list.
After filtering out account types 12/13 here, the later accounts.isEmpty / !accounts.isEmpty branches still use the unfiltered query. A wallet that only has DashPay friendship accounts will now render a blank list instead of the existing "No Accounts" state. Use orderedAccounts.isEmpty for the visible-account checks, or compute a dedicated visibleAccounts collection and key both branches off that.
🤖 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/Core/Views/AccountListView.swift`
around lines 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).
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.
| fn select_recipient_key_index(recipient_identity: &Identity) -> Result<u32, PlatformWalletError> { | ||
| // Prefer a DECRYPTION key. | ||
| if let Some((id, _)) = recipient_identity.public_keys().iter().find(|(_, k)| { | ||
| k.purpose() == Purpose::DECRYPTION && k.key_type() == KeyType::ECDSA_SECP256K1 | ||
| }) { | ||
| return Ok(*id); | ||
| } | ||
| // Fall back to an ENCRYPTION key (mobile cohort). | ||
| if let Some((id, _)) = recipient_identity.public_keys().iter().find(|(_, k)| { | ||
| k.purpose() == Purpose::ENCRYPTION && k.key_type() == KeyType::ECDSA_SECP256K1 | ||
| }) { | ||
| return Ok(*id); | ||
| } | ||
| Err(PlatformWalletError::InvalidIdentityData( | ||
| "Recipient identity has no ECDSA_SECP256K1 DECRYPTION or ENCRYPTION key".to_string(), | ||
| )) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: 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.
| fn select_recipient_key_index(recipient_identity: &Identity) -> Result<u32, PlatformWalletError> { | |
| // Prefer a DECRYPTION key. | |
| if let Some((id, _)) = recipient_identity.public_keys().iter().find(|(_, k)| { | |
| k.purpose() == Purpose::DECRYPTION && k.key_type() == KeyType::ECDSA_SECP256K1 | |
| }) { | |
| return Ok(*id); | |
| } | |
| // Fall back to an ENCRYPTION key (mobile cohort). | |
| if let Some((id, _)) = recipient_identity.public_keys().iter().find(|(_, k)| { | |
| k.purpose() == Purpose::ENCRYPTION && k.key_type() == KeyType::ECDSA_SECP256K1 | |
| }) { | |
| return Ok(*id); | |
| } | |
| Err(PlatformWalletError::InvalidIdentityData( | |
| "Recipient identity has no ECDSA_SECP256K1 DECRYPTION or ENCRYPTION key".to_string(), | |
| )) | |
| } | |
| if let Some((id, _)) = recipient_identity.public_keys().iter().find(|(_, k)| { | |
| k.purpose() == Purpose::DECRYPTION | |
| && k.key_type() == KeyType::ECDSA_SECP256K1 | |
| && k.disabled_at().is_none() | |
| }) { | |
| return Ok(*id); | |
| } | |
| if let Some((id, _)) = recipient_identity.public_keys().iter().find(|(_, k)| { | |
| k.purpose() == Purpose::ENCRYPTION | |
| && k.key_type() == KeyType::ECDSA_SECP256K1 | |
| && k.disabled_at().is_none() | |
| }) { | |
| return Ok(*id); | |
| } | |
| Err(PlatformWalletError::InvalidIdentityData( | |
| "Recipient identity has no usable ECDSA_SECP256K1 DECRYPTION or ENCRYPTION key".to_string(), | |
| )) |
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in this update — select_recipient_key_index returns disabled keys no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| #[repr(C)] | ||
| #[derive(Debug, Clone, Copy)] | ||
| pub struct DashpayPaymentFFI { | ||
| /// The other identity in this payment (`counterparty_id`). Whether | ||
| /// they are the sender or the receiver is encoded in `direction`. | ||
| pub counterparty_id: [u8; 32], | ||
| /// Amount in duffs. Always positive; `direction` carries the sign. | ||
| pub amount_duffs: u64, | ||
| /// Payment direction from the owner's perspective. | ||
| pub direction: DashpayPaymentDirectionFFI, | ||
| /// Core-chain status. | ||
| pub status: DashpayPaymentStatusFFI, | ||
| /// NUL-terminated transaction id (hex), the `dashpay_payments` map | ||
| /// key. Always non-null. Owned — released by | ||
| /// [`dashpay_payment_array_free`]. | ||
| pub txid: *mut c_char, | ||
| /// NUL-terminated sender memo, or null when the source `Option` was | ||
| /// `None`. Owned — released by [`dashpay_payment_array_free`]. | ||
| pub memo: *mut c_char, | ||
| } |
There was a problem hiding this comment.
*🟡 Suggestion: 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.
| #[repr(C)] | |
| #[derive(Debug, Clone, Copy)] | |
| pub struct DashpayPaymentFFI { | |
| /// The other identity in this payment (`counterparty_id`). Whether | |
| /// they are the sender or the receiver is encoded in `direction`. | |
| pub counterparty_id: [u8; 32], | |
| /// Amount in duffs. Always positive; `direction` carries the sign. | |
| pub amount_duffs: u64, | |
| /// Payment direction from the owner's perspective. | |
| pub direction: DashpayPaymentDirectionFFI, | |
| /// Core-chain status. | |
| pub status: DashpayPaymentStatusFFI, | |
| /// NUL-terminated transaction id (hex), the `dashpay_payments` map | |
| /// key. Always non-null. Owned — released by | |
| /// [`dashpay_payment_array_free`]. | |
| pub txid: *mut c_char, | |
| /// NUL-terminated sender memo, or null when the source `Option` was | |
| /// `None`. Owned — released by [`dashpay_payment_array_free`]. | |
| pub memo: *mut c_char, | |
| } | |
| #[repr(C)] | |
| #[derive(Debug)] | |
| pub struct DashpayPaymentFFI { | |
| pub counterparty_id: [u8; 32], | |
| pub amount_duffs: u64, | |
| pub direction: DashpayPaymentDirectionFFI, | |
| pub status: DashpayPaymentStatusFFI, | |
| pub txid: *mut c_char, | |
| pub memo: *mut c_char, | |
| } |
source: ['claude']
There was a problem hiding this comment.
Resolved in this update — DashpayPaymentFFI derives Clone, Copy despite owning *mut c_char strings reclaimed by dashpay_payment_array_free no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
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.)
| 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.rejected | ||
| .insert((owner_id, *sender_id, account_reference), tombstone); | ||
| cs | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: 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.
| 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.rejected | |
| .insert((owner_id, *sender_id, account_reference), tombstone); | |
| cs | |
| } | |
| 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 |
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in 440ffca — record_rejected_contact_request drops incoming in memory but never persists the deletion no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
There was a problem hiding this comment.
Resolved in 440ffca — record_rejected_contact_request drops incoming in memory but never persists the deletion no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| if let Err(e) = self | ||
| .register_external_contact_account( | ||
| identity_id, | ||
| &contact_id, | ||
| &candidate.encrypted_public_key, | ||
| candidate.our_decryption_key_index, | ||
| candidate.contact_encryption_key_index, | ||
| ) | ||
| .await | ||
| { | ||
| tracing::warn!( | ||
| identity = %identity_id, | ||
| contact = %contact_id, | ||
| error = %e, | ||
| "Failed to register DashPay external account; marking payment channel broken (permanent)" | ||
| ); | ||
| self.mark_contact_channel_broken(identity_id, &contact_id) | ||
| .await; | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: 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).
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in 440ffca — Transient DAPI failures inside register_external_contact_account are classified as permanent no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| // G1a: do NOT skip just because the sender is in | ||
| // `sent_contact_requests` — that is the reciprocal we | ||
| // need to let through to auto-establish. Skip only when | ||
| // already tracked as incoming or established (true | ||
| // dedup), or suppressed by a rejected-request tombstone. | ||
| if managed.incoming_contact_requests.contains_key(&sender_id) | ||
| || managed.established_contacts.contains_key(&sender_id) | ||
| { | ||
| continue; | ||
| } | ||
| }; | ||
| let account_reference = match props | ||
| .get("accountReference") | ||
| .and_then(|v: &Value| v.to_integer::<u32>().ok()) | ||
| { | ||
| Some(v) => v, | ||
| None => { | ||
| tracing::warn!( | ||
| // G5 stage 1: a rejected request (same sender + | ||
| // accountReference) must not be resurrected. A rotated | ||
| // request (bumped accountReference) is NOT suppressed. | ||
| if managed.is_request_rejected(&sender_id, contact_request.account_reference) { | ||
| tracing::debug!( | ||
| sender = %sender_id, | ||
| recipient = %identity_id, | ||
| "Skipping contact request document: missing accountReference" | ||
| account_reference = contact_request.account_reference, | ||
| "Skipping rejected contact request (tombstoned); doc {doc_id}" | ||
| ); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: 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.
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in 440ffca — Established-contact ingest skip makes payment_channel_broken recovery unreachable no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| pub fn start(self: Arc<Self>) { | ||
| let mut guard = self.background_cancel.lock().expect("bg_cancel poisoned"); | ||
| if guard.is_some() { | ||
| return; | ||
| } | ||
| let cancel = CancellationToken::new(); | ||
| *guard = Some(cancel.clone()); | ||
| drop(guard); | ||
|
|
||
| let handle = tokio::runtime::Handle::current(); | ||
| let this = self; | ||
| std::thread::Builder::new() | ||
| .name("dashpay-sync".into()) | ||
| // DashPay sync verifies GroveDB *document-query* proofs | ||
| // (contactRequest / profile fetches), whose recursive | ||
| // `verify_layer_proof_v1` descent overflows the platform | ||
| // default thread stack (SIGBUS on the stack guard, observed | ||
| // on-device 2026-06-12). The sibling sync threads survive on | ||
| // the default only because their proofs are shallower; match | ||
| // the FFI worker convention (`runtime.rs` WORKER_STACK_BYTES) | ||
| // since `Handle::block_on` polls the future on THIS thread. | ||
| .stack_size(8 * 1024 * 1024) | ||
| .spawn(move || { | ||
| handle.block_on(async move { | ||
| loop { | ||
| if cancel.is_cancelled() { | ||
| break; | ||
| } | ||
|
|
||
| this.sync_now().await; | ||
|
|
||
| let interval = this.interval(); | ||
| tokio::select! { | ||
| _ = tokio::time::sleep(interval) => {} | ||
| _ = cancel.cancelled() => break, | ||
| } | ||
| } | ||
|
|
||
| if let Ok(mut guard) = this.background_cancel.lock() { | ||
| *guard = None; | ||
| } | ||
| }); | ||
| }) | ||
| .expect("failed to spawn dashpay-sync thread"); |
There was a problem hiding this comment.
🔴 Blocking: 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 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.)
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in this update — DashPaySyncManager thread cleanup unconditionally clears the cancel token — stop/start race enables use-after-free across FFI no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…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.
| let account_reference = { | ||
| let secret = ecdh_private_key.secret_bytes(); | ||
| let previous_version = { | ||
| let wm = self.wallet_manager.read().await; | ||
| wm.get_wallet_info(&self.wallet_id) | ||
| .and_then(|info| info.identity_manager.managed_identity(sender_identity_id)) | ||
| .and_then(|managed| managed.sent_contact_requests.get(recipient_identity_id)) | ||
| .map(|prior| { | ||
| crate::wallet::identity::crypto::dip14::unmask_account_reference( | ||
| prior.account_reference, | ||
| &secret, | ||
| &xpub_bytes, | ||
| ) | ||
| .0 | ||
| }) | ||
| }; | ||
| let version = match previous_version { | ||
| // 4-bit field; saturate rather than wrap so a 16th | ||
| // rotation fails loudly at the unique index instead of | ||
| // silently colliding with version 0. | ||
| Some(v) if v >= 15 => { | ||
| tracing::warn!( | ||
| recipient = %recipient_identity_id, | ||
| "accountReference rotation version saturated at 15" | ||
| ); | ||
| 15 | ||
| } | ||
| Some(v) => v + 1, | ||
| None => 0, | ||
| }; | ||
| crate::wallet::identity::crypto::dip14::calculate_account_reference( | ||
| &secret, | ||
| &xpub_bytes, | ||
| account_index, | ||
| version, | ||
| ) | ||
| }; |
There was a problem hiding this comment.
🔴 Blocking: 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).
| let account_reference = { | |
| let secret = ecdh_private_key.secret_bytes(); | |
| let previous_version = { | |
| let wm = self.wallet_manager.read().await; | |
| wm.get_wallet_info(&self.wallet_id) | |
| .and_then(|info| info.identity_manager.managed_identity(sender_identity_id)) | |
| .and_then(|managed| managed.sent_contact_requests.get(recipient_identity_id)) | |
| .map(|prior| { | |
| crate::wallet::identity::crypto::dip14::unmask_account_reference( | |
| prior.account_reference, | |
| &secret, | |
| &xpub_bytes, | |
| ) | |
| .0 | |
| }) | |
| }; | |
| let version = match previous_version { | |
| // 4-bit field; saturate rather than wrap so a 16th | |
| // rotation fails loudly at the unique index instead of | |
| // silently colliding with version 0. | |
| Some(v) if v >= 15 => { | |
| tracing::warn!( | |
| recipient = %recipient_identity_id, | |
| "accountReference rotation version saturated at 15" | |
| ); | |
| 15 | |
| } | |
| Some(v) => v + 1, | |
| None => 0, | |
| }; | |
| crate::wallet::identity::crypto::dip14::calculate_account_reference( | |
| &secret, | |
| &xpub_bytes, | |
| account_index, | |
| version, | |
| ) | |
| }; | |
| let account_reference = { | |
| let secret = ecdh_private_key.secret_bytes(); | |
| let previous_version = { | |
| let wm = self.wallet_manager.read().await; | |
| wm.get_wallet_info(&self.wallet_id) | |
| .and_then(|info| info.identity_manager.managed_identity(sender_identity_id)) | |
| .and_then(|managed| { | |
| managed.sent_contact_requests.get(recipient_identity_id).or_else(|| { | |
| managed | |
| .established_contacts | |
| .get(recipient_identity_id) | |
| .map(|contact| &contact.outgoing_request) | |
| }) | |
| }) | |
| .map(|prior| { | |
| crate::wallet::identity::crypto::dip14::unmask_account_reference( | |
| prior.account_reference, | |
| &secret, | |
| &xpub_bytes, | |
| ) | |
| .0 | |
| }) | |
| }; |
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in this update — Send-side rotation version lookup ignores established_contacts — re-send after auto-establishment reuses version=0 and collides on the unique index no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| let tracked_reference = managed | ||
| .incoming_contact_requests | ||
| .get(&sender_id) | ||
| .map(|r| r.account_reference) | ||
| .or_else(|| { | ||
| managed | ||
| .established_contacts | ||
| .get(&sender_id) | ||
| .map(|c| c.incoming_request.account_reference) | ||
| }); | ||
| if tracked_reference == Some(contact_request.account_reference) { | ||
| continue; | ||
| } | ||
| }; | ||
| let account_reference = match props | ||
| .get("accountReference") | ||
| .and_then(|v: &Value| v.to_integer::<u32>().ok()) | ||
| { | ||
| Some(v) => v, | ||
| None => { | ||
| tracing::warn!( | ||
| // G5 stage 1: a rejected request (same sender + | ||
| // accountReference) must not be resurrected. A rotated | ||
| // request (bumped accountReference) is NOT suppressed. | ||
| if managed.is_request_rejected(&sender_id, contact_request.account_reference) { | ||
| tracing::debug!( | ||
| sender = %sender_id, | ||
| recipient = %identity_id, | ||
| "Skipping contact request document: missing accountReference" | ||
| account_reference = contact_request.account_reference, | ||
| "Skipping rejected contact request (tombstoned); doc {doc_id}" | ||
| ); | ||
| continue; | ||
| } | ||
| }; | ||
| let encrypted_public_key = match props | ||
| .get("encryptedPublicKey") | ||
| .and_then(|v: &Value| v.as_bytes()) | ||
| .cloned() | ||
| { | ||
| Some(v) => v, | ||
| None => { | ||
| tracing::warn!( | ||
| sender = %sender_id, | ||
| recipient = %identity_id, | ||
| "Skipping contact request document: missing encryptedPublicKey" | ||
| ); | ||
|
|
||
| if tracked_reference.is_some() { | ||
| // Rotation: supersede the tracked request. When an | ||
| // established contact was re-keyed, queue the stale | ||
| // external account for teardown so the build sweep | ||
| // below re-registers it from the new xpub. | ||
| if managed | ||
| .apply_rotated_incoming_request(contact_request.clone(), &self.persister) | ||
| { | ||
| rotated_contacts.push(sender_id); | ||
| } | ||
| all_requests.push(contact_request); | ||
| continue; |
There was a problem hiding this comment.
🔴 Blocking: 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.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Historical contactRequest documents replay as fresh rotations every sync sweep no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| fn select_recipient_key_index(recipient_identity: &Identity) -> Result<u32, PlatformWalletError> { | ||
| // Prefer a DECRYPTION key. | ||
| if let Some((id, _)) = recipient_identity.public_keys().iter().find(|(_, k)| { | ||
| k.purpose() == Purpose::DECRYPTION && k.key_type() == KeyType::ECDSA_SECP256K1 | ||
| }) { | ||
| return Ok(*id); | ||
| } | ||
| // Fall back to an ENCRYPTION key (mobile cohort). | ||
| if let Some((id, _)) = recipient_identity.public_keys().iter().find(|(_, k)| { | ||
| k.purpose() == Purpose::ENCRYPTION && k.key_type() == KeyType::ECDSA_SECP256K1 | ||
| }) { | ||
| return Ok(*id); | ||
| } | ||
| Err(PlatformWalletError::InvalidIdentityData( | ||
| "Recipient identity has no ECDSA_SECP256K1 DECRYPTION or ENCRYPTION key".to_string(), | ||
| )) | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: 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.
| fn select_recipient_key_index(recipient_identity: &Identity) -> Result<u32, PlatformWalletError> { | |
| // Prefer a DECRYPTION key. | |
| if let Some((id, _)) = recipient_identity.public_keys().iter().find(|(_, k)| { | |
| k.purpose() == Purpose::DECRYPTION && k.key_type() == KeyType::ECDSA_SECP256K1 | |
| }) { | |
| return Ok(*id); | |
| } | |
| // Fall back to an ENCRYPTION key (mobile cohort). | |
| if let Some((id, _)) = recipient_identity.public_keys().iter().find(|(_, k)| { | |
| k.purpose() == Purpose::ENCRYPTION && k.key_type() == KeyType::ECDSA_SECP256K1 | |
| }) { | |
| return Ok(*id); | |
| } | |
| Err(PlatformWalletError::InvalidIdentityData( | |
| "Recipient identity has no ECDSA_SECP256K1 DECRYPTION or ENCRYPTION key".to_string(), | |
| )) | |
| } | |
| fn select_recipient_key_index(recipient_identity: &Identity) -> Result<u32, PlatformWalletError> { | |
| if let Some((id, _)) = recipient_identity.public_keys().iter().find(|(_, k)| { | |
| k.purpose() == Purpose::DECRYPTION | |
| && k.key_type() == KeyType::ECDSA_SECP256K1 | |
| && k.disabled_at().is_none() | |
| }) { | |
| return Ok(*id); | |
| } | |
| if let Some((id, _)) = recipient_identity.public_keys().iter().find(|(_, k)| { | |
| k.purpose() == Purpose::ENCRYPTION | |
| && k.key_type() == KeyType::ECDSA_SECP256K1 | |
| && k.disabled_at().is_none() | |
| }) { | |
| return Ok(*id); | |
| } | |
| Err(PlatformWalletError::InvalidIdentityData( | |
| "Recipient identity has no usable ECDSA_SECP256K1 DECRYPTION or ENCRYPTION key".to_string(), | |
| )) | |
| } |
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in 440ffca — select_recipient_key_index returns disabled (revoked) keys — DIP-15 compact xpub encrypted to a key whose private half may be compromised no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
Network layer over the part-1 crypto core: - fetch_decrypted_contact_infos: query the owner's contactInfo docs (with the load-bearing ORDER BY $updatedAt — drive proves absence for bare secondary-index equality, same trap as the contact-request queries), derive each doc's keys from its own rootEncryptionKeyIndex/derivationEncryptionKeyIndex, decrypt encToUserId to find which contact it belongs to. The contact↔doc mapping is deliberately stateless — restore-from-seed recovers alias/note/hidden entirely from chain. - sync_contact_infos: step 3 of the recurring dashpay_sync pass; applies decrypted metadata onto established contacts through the new ManagedIdentity::set_contact_metadata (no-op when unchanged so recurring passes don't spam the persister). - set_contact_info_with_external_signer: local state first (works offline), then publish create-or-update through the put_document seam. Enforces DIP-15's privacy rule: with <2 established contacts the network write is deferred (logged), local state still lands. Fresh docs take the next sequential derivation index; updates reuse the existing doc's index + bump its revision. - FFI: platform_wallet_set_dashpay_contact_info_with_signer (same vtable-signer shape as the profile write). Follow-ups (part 3): persist alias/note/hidden across the FFI persister into SwiftData (ContactRequestFFI + Swift model columns), switch ContactDetailView off the UserDefaults meta store, and seam-level tests for the sync/publish paths via the recording SdkWriter. 226/226 lib tests green.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs (1)
109-130:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlso emit
removed_incomingwith the rejection tombstone.Line 116 removes the incoming request from in-memory state, but the returned
ContactChangeSetonly carriesrejected. The reject path inpackages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rspersists this delta as-is, so restore/replay has no persisted signal to delete the old incoming request and can resurrect the rejected request after reload. Add the matchingReceivedContactRequestKeytocs.removed_incomingwhen the map entry is removed.Proposed fix
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 removed_incoming = self.incoming_contact_requests.remove(sender_id).is_some(); 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(); + if removed_incoming { + cs.removed_incoming.insert(ReceivedContactRequestKey { + owner_id, + sender_id: *sender_id, + }); + } cs.rejected .insert((owner_id, *sender_id, account_reference), tombstone); cs }🤖 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/state/managed_identity/contact_requests.rs` around lines 109 - 130, In record_rejected_contact_request, after removing the entry from incoming_contact_requests you must also record that removal in the returned ContactChangeSet so the delta persists deletion; build a ReceivedContactRequestKey (owner_id, *sender_id, account_reference) and insert it into cs.removed_incoming before returning. This ensures the rejection tombstone is added to cs.rejected and the corresponding incoming entry is emitted via cs.removed_incoming for proper replay; locate record_rejected_contact_request, incoming_contact_requests, ContactChangeSet and Removed/removed_incoming to implement the insertion.
🤖 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/07-contactinfo-conventions.md`:
- Around line 25-28: The fenced code block showing the derivation paths lacks a
language tag; update the triple-backtick fence surrounding the lines starting
with "encToUserId key:" and "privateData key:" to include a plain text specifier
(e.g., ```text or ```plaintext) so the block is lint-compliant and renders
correctly.
In `@packages/rs-platform-wallet-ffi/src/contact_info.rs`:
- Around line 51-56: The code currently converts signer_handle to usize and back
to a pointer which can lose pointer provenance; instead capture a typed pointer
to VTableSigner before the worker hop and then unsafely dereference it inside
the closure. Concretely, in the block that calls
PLATFORM_WALLET_STORAGE.with_item and block_on_worker, replace the
signer_addr/usize roundtrip with a typed pointer (e.g., signer_ptr of type
*const VTableSigner derived from signer_handle) and inside the async closure
obtain the signer with unsafe { &*signer_ptr } when creating the &VTableSigner
used by the worker; leave block_on_worker and wallet access unchanged. Ensure
the symbol names referenced are signer_handle, signer_ptr, VTableSigner,
PLATFORM_WALLET_STORAGE.with_item, and block_on_worker.
---
Duplicate comments:
In
`@packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:
- Around line 109-130: In record_rejected_contact_request, after removing the
entry from incoming_contact_requests you must also record that removal in the
returned ContactChangeSet so the delta persists deletion; build a
ReceivedContactRequestKey (owner_id, *sender_id, account_reference) and insert
it into cs.removed_incoming before returning. This ensures the rejection
tombstone is added to cs.rejected and the corresponding incoming entry is
emitted via cs.removed_incoming for proper replay; locate
record_rejected_contact_request, incoming_contact_requests, ContactChangeSet and
Removed/removed_incoming to implement the insertion.
🪄 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: b106d317-7b25-48d6-884b-90c7c13f2b57
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
docs/dashpay/SPEC.mddocs/dashpay/research/07-contactinfo-conventions.mdpackages/rs-platform-encryption/src/lib.rspackages/rs-platform-wallet-ffi/src/contact_info.rspackages/rs-platform-wallet-ffi/src/dashpay_profile.rspackages/rs-platform-wallet-ffi/src/lib.rspackages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/src/lib.rspackages/rs-platform-wallet/src/wallet/identity/crypto/contact_info.rspackages/rs-platform-wallet/src/wallet/identity/crypto/dip14.rspackages/rs-platform-wallet/src/wallet/identity/crypto/mod.rspackages/rs-platform-wallet/src/wallet/identity/mod.rspackages/rs-platform-wallet/src/wallet/identity/network/contact_info.rspackages/rs-platform-wallet/src/wallet/identity/network/contact_requests.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/state/managed_identity/contact_requests.rspackages/rs-platform-wallet/tests/contact_workflow_tests.rs
✅ Files skipped from review due to trivial changes (2)
- packages/rs-platform-wallet/Cargo.toml
- docs/dashpay/SPEC.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/rs-platform-wallet-ffi/src/lib.rs
- packages/rs-platform-wallet/src/wallet/identity/network/mod.rs
- packages/rs-platform-wallet/src/lib.rs
- packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs
| ``` | ||
| encToUserId key: rootEncryptionKey / 65536' / derivationEncryptionKeyIndex' (2^16) | ||
| privateData key: rootEncryptionKey / 65537' / derivationEncryptionKeyIndex' (2^16 + 1) | ||
| ``` |
There was a problem hiding this comment.
Add a language specifier to the fenced code block.
The code block showing the derivation paths lacks a language identifier. Adding text or plaintext would satisfy linting and improve rendering.
-```
+```text
encToUserId key: rootEncryptionKey / 65536' / derivationEncryptionKeyIndex' (2^16)
privateData key: rootEncryptionKey / 65537' / derivationEncryptionKeyIndex' (2^16 + 1)
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @docs/dashpay/research/07-contactinfo-conventions.md around lines 25 - 28,
The fenced code block showing the derivation paths lacks a language tag; update
the triple-backtick fence surrounding the lines starting with "encToUserId key:"
and "privateData key:" to include a plain text specifier (e.g., ```text or
Source: Linters/SAST tools
| let signer_addr = signer_handle as usize; | ||
|
|
||
| let option = PLATFORM_WALLET_STORAGE.with_item(wallet_handle, move |wallet| { | ||
| let identity_wallet = wallet.identity().clone(); | ||
| block_on_worker(async move { | ||
| let signer: &VTableSigner = &*(signer_addr as *const VTableSigner); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Signer type definitions and thread-safety markers =="
fd -t f . packages/rs-sdk-ffi/src | xargs rg -n -C3 'SignerHandle|VTableSigner|unsafe impl .*Send|unsafe impl .*Sync|impl .*Send|impl .*Sync'
echo
echo "== FFI call sites that move signer pointers into async workers =="
rg -n -C3 'signer_addr as usize|as \*const VTableSigner|block_on_worker' packages/rs-platform-wallet-ffi/srcRepository: dashpay/platform
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate VTableSigner / SignerHandle definitions =="
fd -t f "signer*" packages/rs-sdk-ffi/src | head -n 50
echo "---"
rg -n "struct VTableSigner|VTableSigner::|impl .*VTableSigner|type VTableSigner" packages/rs-sdk-ffi/src | head -n 200
echo "---"
rg -n "SignerHandle" packages/rs-sdk-ffi/src | head -n 200
echo
echo "== Check for explicit Send/Sync impls for signer-related types =="
rg -n --hidden "unsafe impl .*Send|unsafe impl .*Sync|impl .*Send|impl .*Sync" packages/rs-sdk-ffi/src/sign* packages/rs-sdk-ffi/src/*sign* 2>/dev/null || true
rg -n --hidden "Send.*VTableSigner|Sync.*VTableSigner|Send.*SignerHandle|Sync.*SignerHandle" packages/rs-sdk-ffi/src || true
echo
echo "== Inspect contact_info.rs around lines 40-90 =="
sed -n '35,95p' packages/rs-platform-wallet-ffi/src/contact_info.rs
echo
echo "== Find block_on_worker usage that touches signer_addr / VTableSigner in wallet-ffi =="
rg -n --context 2 "signer_addr as usize|as \\*const VTableSigner|block_on_worker" packages/rs-platform-wallet-ffi/src/contact_info.rs
echo
echo "== Look for same pointer pattern in dashpay_profile.rs =="
rg -n --context 3 "signer_addr as usize|VTableSigner|\\*const VTableSigner|block_on_worker" packages/rs-platform-wallet-ffi/src/dashpay_profile.rsRepository: dashpay/platform
Length of output: 30571
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find block_on_worker definition =="
fd -t f "block_on_worker" packages/rs-platform-wallet-ffi/src | head -n 50
rg -n "fn block_on_worker|block_on_worker<|block_on_worker\(" packages/rs-platform-wallet-ffi/src | head -n 50
echo
echo "== Show runtime module around block_on_worker =="
fd -t f "runtime" packages/rs-platform-wallet-ffi/src | head -n 50
# print likely file(s)
fd -t f "runtime" packages/rs-platform-wallet-ffi/src | xargs -I{} bash -lc 'echo "---- {}"; rg -n "block_on_worker" "{}" || true' 2>/dev/null || true
# fallback: just cat small likely candidates
for f in $(rg -l "block_on_worker" packages/rs-platform-wallet-ffi/src | head -n 20); do
echo "---- $f"
sed -n '1,200p' "$f"
doneRepository: dashpay/platform
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== runtime.rs: block_on_worker implementation =="
sed -n '1,140p' packages/rs-platform-wallet-ffi/src/runtime.rs
echo
echo "== contact_info.rs: whole function (context) =="
sed -n '1,140p' packages/rs-platform-wallet-ffi/src/contact_info.rsRepository: dashpay/platform
Length of output: 5215
Fix signer pointer provenance loss in contact_info worker hop
rs_sdk_ffi::signer::VTableSigner is explicitly unsafe impl Send + Sync, and block_on_worker spawns the async work onto a Tokio worker but waits for it to finish, so the signer being used off-thread is the intended/allowed model. The real problem in lines 51-56 is the signer_handle as usize → as *const VTableSigner roundtrip, which can drop pointer provenance; capture a typed pointer instead (e.g., let signer_ptr = signer_handle as *const VTableSigner; then let signer = unsafe { &*signer_ptr };).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rs-platform-wallet-ffi/src/contact_info.rs` around lines 51 - 56,
The code currently converts signer_handle to usize and back to a pointer which
can lose pointer provenance; instead capture a typed pointer to VTableSigner
before the worker hop and then unsafely dereference it inside the closure.
Concretely, in the block that calls PLATFORM_WALLET_STORAGE.with_item and
block_on_worker, replace the signer_addr/usize roundtrip with a typed pointer
(e.g., signer_ptr of type *const VTableSigner derived from signer_handle) and
inside the async closure obtain the signer with unsafe { &*signer_ptr } when
creating the &VTableSigner used by the worker; leave block_on_worker and wallet
access unchanged. Ensure the symbol names referenced are signer_handle,
signer_ptr, VTableSigner, PLATFORM_WALLET_STORAGE.with_item, and
block_on_worker.
…part 3) Pipeline: ContactRequestFFI gains alias/note/is_hidden (established rows only, replicated onto both directions like the broken flag; CString lifecycle owned by free_contact_requests_ffi; layout asserts updated 152→176) → Swift persistence handler copies them onto three new additive PersistentDashpayContactRequest columns (lightweight migration) → ContactDetailView + ContactsView read them reactively off the @query rows and write through the new ManagedPlatformWallet.setDashPayContactInfo (KeychainSigner, same vtable shape as the profile write). "This device only" labels replaced; the UserDefaults meta store now only keeps the add-time DPNS hint. Verified on-sim: alias save → FFI → Rust set_contact_metadata → persister → both SwiftData rows carry the alias; the DIP-15 privacy gate correctly deferred the document publish at 1 established contact ("publish deferred ... local state updated"). KNOWN GAP (fix follows): in the deferred-publish window the metadata does NOT survive an app relaunch — contacts are not restored from local persistence at load (they re-derive from chain via the sync sweep), so the re-establish round writes alias=None back over the rows. Once the contactInfo doc publishes (≥2 contacts), relaunch durability comes from chain. Next commit restores contacts (incl. metadata) from SwiftData at load, which also makes contacts visible on offline launches.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
All 10 prior findings against 440ffca are STILL VALID at HEAD 3f707c2 — verified directly in the worktree. The new contactInfo delta (commits d78bf31 + 3f707c2) does not touch any of the prior hotspots, so the prior blockers carry forward unchanged. Two additional issues surfaced in the new contactInfo code (pagination cap of 100 docs driving both sync and derivation_index allocation; ENCRYPTION key selection without disabled_at check on the owner's own keys at contact_info.rs:313-320). The pagination issue is folded into the budget as it overlaps with derivation-index correctness; the contact_info disabled_at gap is folded into finding #10 since it is the same root-cause pattern. Budget filled with 10 blockers; 2 suggestion-level findings (DashpayPaymentFFI Copy/Clone, contactInfo pagination cap) dropped due to budget but recorded in out_of_scope_findings.
🔴 2 blocking
2 additional finding(s) omitted (not in diff).
8 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 tombstones are written but never restored on rehydration
Verified at HEAD: `managed_identity_from_entry` hard-codes `rejected_contact_requests: Default::default()` at line 214. The writer (INSERT INTO rejected_contact_requests) and the V001 row exist, but no `load_state` reader for the new table exists, and `apply_changeset` only handles live deltas — restored state is always empty after restart. The recurring DashPay loop's tombstone check at `network/contact_requests.rs:457` therefore never fires after a restart, so a rejected request is resurrected on the first sweep. Add a per-wallet `load_state` reader for `rejected_contact_requests` and route its output into the rehydrated `ManagedIdentity.rejected_contact_requests`.
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
Verified at HEAD lines 3024-3034: 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 3005-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 — a confidentiality regression for users handing off or disposing devices.
…ad (M3, part 4)
Closes the part-3 relaunch-durability gap: contacts were never
restored from local persistence — they re-derived from chain on the
first sync sweep, so (a) the Contacts UI was empty on offline
launches and (b) the re-establish round emitted alias=None over the
SwiftData rows, wiping contactInfo metadata during the DIP-15
deferred-publish window.
- IdentityRestoreEntryFFI gains a contacts array (reuses the
persist-side ContactRequestFFI shape; rows are load-allocation-
owned — Rust's persist-side destructor never runs on them).
- Swift assembles the per-identity PersistentDashpayContactRequest
rows into the load callback (payloads on scalarBuffers, metadata
strings on cStringBuffers, array on the new contactArrays list).
- Rust folds them into ManagedIdentity at load: direction pairs →
EstablishedContact (with alias/note/hidden + broken flag), single
rows → pending sent/incoming maps. Direct map inserts, no
persister rounds.
Verified on-sim: set alias → relaunch → SwiftData rows still carry
it AND the contact list renders it ("B, Bestie") after the first
sweep — the re-establish path now preserves metadata from the
restored in-memory contact.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified the checked-out worktree at 8037a47. All 10 prior blocking findings remain valid and in scope for this PR; no prior finding was resolved. CodeRabbit supplied no inline findings, and one lower-priority in-scope suggestion was omitted because the 10 blocking issues fill the comment budget.
🔴 10 blocking
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:167-180: Send-side rotation ignores established contacts
The resend path still derives `previous_version` only from `managed.sent_contact_requests`. Once reciprocal sync establishes the contact, that sent request is moved to `EstablishedContact.outgoing_request`, so a later send to the same recipient falls back to version 0. With the same sender, recipient, xpub, and account index, that can reproduce the original masked `accountReference` and collide with the DashPay contract unique index instead of creating a broadcastable rotation. The new load path also restores established contacts without repopulating `sent_contact_requests`, so a resend after relaunch hits the same state immediately.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:441-478: Historical requests replay as rotations
The receive-side rotation guard still compares only the incoming `accountReference` against the currently tracked reference. Because `fetch_received_contact_requests(identity_id, None)` is unfiltered and contactRequest documents are immutable, older v0 documents can be seen after a newer rotated document has advanced local state. Reprocessing the stale document as a rotation can replace the tracked request, tear down the external account, and then flip forward again on a later document or sweep; rotation acceptance needs a monotonic freshness check such as created time or decoded version, not bare reference inequality.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:806-826: Transient account-registration failures become permanent
`build_contact_accounts` still treats every `register_external_contact_account` error as permanent and marks `payment_channel_broken`. That callee performs additional network-backed identity/decryption work, so a transient DAPI or fetch failure can be converted into durable local broken-channel state. Future sweeps then skip the contact until the other party sends a superseding request; only genuinely non-recoverable decrypt, decode, missing-key, or key-type failures should take the permanent-broken path.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:292-308: Disabled encryption keys are still selectable
`select_recipient_key_index` still chooses the first DECRYPTION or ENCRYPTION ECDSA_SECP256K1 key without checking `disabled_at`, while receive-side validation rejects disabled keys. A sender can therefore encrypt the DIP-15 compact xpub to a key the recipient has revoked on-chain, which is the signal that the private half may be compromised. The same missing disabled-key filter remains in the contactInfo root encryption key selector at `contact_info.rs:313-320`, allowing owner-private alias/note/hidden metadata to be published under a revoked encryption key id.
In `packages/rs-platform-wallet-storage/migrations/V001__initial.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/migrations/V001__initial.rs:178-212: V001 was edited in place
`contacts.payment_channel_broken` and the `rejected_contact_requests` table are still added directly to V001, and there is no V002 migration. V001 already shipped, so existing v4.0.0-beta.4 databases will either fail refinery checksum validation or treat the old V001 as already applied and never create the new column/table. Runtime writes to the new broken-channel flag or rejected-request tombstone table then fail on upgraded wallets; these schema additions need an append-only migration.
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-130: Reject does not persist the incoming-row deletion
`record_rejected_contact_request` removes the incoming request from memory, but the returned `ContactChangeSet` only populates `cs.rejected`. The storage writer inserts rejected tombstones separately and only deletes live incoming rows when `cs.removed_incoming` is populated, so the stale `state='received'` contact row remains persisted. On the next load or contact restore, that stale row can reappear as a visible incoming request even though the user rejected it.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:3311-3417: Rejected tombstones are not restored
The new `restore_dashpay_contacts` bridge restores sent, incoming, and established contact rows, but `IdentityRestoreEntryFFI` has no rejected-tombstone input and this function never repopulates `ManagedIdentity.rejected_contact_requests`. SwiftData likewise treats rejected snapshots as delete-only visible-row operations. After relaunch, Rust has no suppression entry for the immutable rejected contactRequest, while stale incoming rows can still be restored or fetched again, so a rejected request can reappear despite the user's action.
In `packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs:50-79: purpose_mismatch survives hard validation errors
The type documents `purpose_mismatch` as true only when a key-purpose mismatch is the sole invalidity reason, and the sync code uses that flag to choose retry versus permanent broken-channel handling. However, `add_error` does not clear the flag, `add_purpose_error` sets it unconditionally, and `merge` ORs it. A request with both a hard key error and a purpose error can therefore be retried forever instead of being classified as permanently invalid.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:3031-3047: Wallet deletion omits DashPay payment children
The SwiftData deletion workaround pre-deletes identity children with non-optional inverse relationships, but it still deletes only DPNS names, DashPay profile, and contact requests. `PersistentDashpayPayment.owner` is also non-optional and is reached through `PersistentIdentity.dashpayPayments`, so a wallet with DashPay payment history can hit the same SwiftData inverse-removal fatal during identity deletion. That aborts the wipe before the wallet row is removed and can leave payment metadata such as memo, counterparty id, amount, and txid on disk.
In `packages/rs-platform-wallet/src/manager/dashpay_sync.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/dashpay_sync.rs:230-232: DashPay sync cleanup can clear a newer cancel token
The DashPay sync worker still unconditionally sets `background_cancel` to `None` when its loop exits. If `stop()` cancels token A, `start()` installs token B, and the old thread exits afterward, the old cleanup erases B while the new loop is still running. Later `stop()` or shutdown cannot find the live token, so FFI callback context can be freed while an untracked DashPay thread continues calling through Swift-owned callback pointers. The identity and shielded sync managers already use generation guards to avoid this race.
| let previous_version = { | ||
| let wm = self.wallet_manager.read().await; | ||
| wm.get_wallet_info(&self.wallet_id) | ||
| .and_then(|info| info.identity_manager.managed_identity(sender_identity_id)) | ||
| .and_then(|managed| managed.sent_contact_requests.get(recipient_identity_id)) | ||
| .map(|prior| { | ||
| crate::wallet::identity::crypto::dip14::unmask_account_reference( | ||
| prior.account_reference, | ||
| &secret, | ||
| &xpub_bytes, | ||
| ) | ||
| .0 | ||
| }) | ||
| }; |
There was a problem hiding this comment.
🔴 Blocking: Send-side rotation ignores established contacts
The resend path still derives previous_version only from managed.sent_contact_requests. Once reciprocal sync establishes the contact, that sent request is moved to EstablishedContact.outgoing_request, so a later send to the same recipient falls back to version 0. With the same sender, recipient, xpub, and account index, that can reproduce the original masked accountReference and collide with the DashPay contract unique index instead of creating a broadcastable rotation. The new load path also restores established contacts without repopulating sent_contact_requests, so a resend after relaunch hits the same state immediately.
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in a9599b6 — Send-side rotation ignores established contacts no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| let tracked_reference = managed | ||
| .incoming_contact_requests | ||
| .get(&sender_id) | ||
| .map(|r| r.account_reference) | ||
| .or_else(|| { | ||
| managed | ||
| .established_contacts | ||
| .get(&sender_id) | ||
| .map(|c| c.incoming_request.account_reference) | ||
| }); | ||
| if tracked_reference == Some(contact_request.account_reference) { | ||
| continue; | ||
| } | ||
| }; | ||
| let account_reference = match props | ||
| .get("accountReference") | ||
| .and_then(|v: &Value| v.to_integer::<u32>().ok()) | ||
| { | ||
| Some(v) => v, | ||
| None => { | ||
| tracing::warn!( | ||
| // G5 stage 1: a rejected request (same sender + | ||
| // accountReference) must not be resurrected. A rotated | ||
| // request (bumped accountReference) is NOT suppressed. | ||
| if managed.is_request_rejected(&sender_id, contact_request.account_reference) { | ||
| tracing::debug!( | ||
| sender = %sender_id, | ||
| recipient = %identity_id, | ||
| "Skipping contact request document: missing accountReference" | ||
| account_reference = contact_request.account_reference, | ||
| "Skipping rejected contact request (tombstoned); doc {doc_id}" | ||
| ); | ||
| continue; | ||
| } | ||
| }; | ||
| let encrypted_public_key = match props | ||
| .get("encryptedPublicKey") | ||
| .and_then(|v: &Value| v.as_bytes()) | ||
| .cloned() | ||
| { | ||
| Some(v) => v, | ||
| None => { | ||
| tracing::warn!( | ||
| sender = %sender_id, | ||
| recipient = %identity_id, | ||
| "Skipping contact request document: missing encryptedPublicKey" | ||
| ); | ||
|
|
||
| if tracked_reference.is_some() { | ||
| // Rotation: supersede the tracked request. When an | ||
| // established contact was re-keyed, queue the stale | ||
| // external account for teardown so the build sweep | ||
| // below re-registers it from the new xpub. | ||
| if managed | ||
| .apply_rotated_incoming_request(contact_request.clone(), &self.persister) | ||
| { | ||
| rotated_contacts.push(sender_id); | ||
| } | ||
| all_requests.push(contact_request); | ||
| continue; |
There was a problem hiding this comment.
🔴 Blocking: Historical requests replay as rotations
The receive-side rotation guard still compares only the incoming accountReference against the currently tracked reference. Because fetch_received_contact_requests(identity_id, None) is unfiltered and contactRequest documents are immutable, older v0 documents can be seen after a newer rotated document has advanced local state. Reprocessing the stale document as a rotation can replace the tracked request, tear down the external account, and then flip forward again on a later document or sweep; rotation acceptance needs a monotonic freshness check such as created time or decoded version, not bare reference inequality.
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in a9599b6 — Historical requests replay as rotations no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| note TEXT, | ||
| is_hidden INTEGER, | ||
| accepted_accounts BLOB, | ||
| payment_channel_broken INTEGER, | ||
| updated_at INTEGER NOT NULL DEFAULT (unixepoch()), | ||
| PRIMARY KEY (wallet_id, owner_id, contact_id), | ||
| FOREIGN KEY (wallet_id) REFERENCES wallet_metadata(wallet_id) ON DELETE CASCADE | ||
| ); | ||
|
|
||
| -- Rejected-request tombstone (G5 stage 1). Keyed by | ||
| -- `(wallet_id, owner_id, sender_id, account_reference)` — NOT bare | ||
| -- sender id — so a once-rejected sender can still re-request via a | ||
| -- bumped accountReference (the DIP-15 rotation mechanism), while a | ||
| -- replay of the exact same immutable request stays suppressed. The | ||
| -- optional `document_id` records the rejected document's id for audit / | ||
| -- exact-match suppression. The sync ingest path consults this table | ||
| -- before re-ingesting a received contactRequest. | ||
| CREATE TABLE rejected_contact_requests ( | ||
| wallet_id BLOB NOT NULL, | ||
| owner_id BLOB NOT NULL, | ||
| sender_id BLOB NOT NULL, | ||
| account_reference INTEGER NOT NULL, | ||
| document_id BLOB, | ||
| rejected_at INTEGER NOT NULL DEFAULT (unixepoch()), | ||
| PRIMARY KEY (wallet_id, owner_id, sender_id, account_reference), | ||
| FOREIGN KEY (wallet_id) REFERENCES wallet_metadata(wallet_id) ON DELETE CASCADE | ||
| ); |
There was a problem hiding this comment.
🔴 Blocking: V001 was edited in place
contacts.payment_channel_broken and the rejected_contact_requests table are still added directly to V001, and there is no V002 migration. V001 already shipped, so existing v4.0.0-beta.4 databases will either fail refinery checksum validation or treat the old V001 as already applied and never create the new column/table. Runtime writes to the new broken-channel flag or rejected-request tombstone table then fail on upgraded wallets; these schema additions need an append-only migration.
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in this update — V001 was edited in place no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| 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.rejected | ||
| .insert((owner_id, *sender_id, account_reference), tombstone); | ||
| cs |
There was a problem hiding this comment.
🔴 Blocking: Reject does not persist the incoming-row deletion
record_rejected_contact_request removes the incoming request from memory, but the returned ContactChangeSet only populates cs.rejected. The storage writer inserts rejected tombstones separately and only deletes live incoming rows when cs.removed_incoming is populated, so the stale state='received' contact row remains persisted. On the next load or contact restore, that stale row can reappear as a visible incoming request even though the user rejected it.
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in this update — Reject does not persist the incoming-row deletion no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| unsafe fn restore_dashpay_contacts( | ||
| spec: &IdentityRestoreEntryFFI, | ||
| owner_id: &Identifier, | ||
| managed: &mut ManagedIdentity, | ||
| ) { | ||
| use platform_wallet::{ContactRequest, EstablishedContact}; | ||
|
|
||
| if spec.contacts.is_null() || spec.contacts_count == 0 { | ||
| return; | ||
| } | ||
| let rows = slice::from_raw_parts(spec.contacts, spec.contacts_count); | ||
|
|
||
| /// Per-contact accumulator while pairing the direction rows. | ||
| #[derive(Default)] | ||
| struct PairAccumulator { | ||
| outgoing: Option<ContactRequest>, | ||
| incoming: Option<ContactRequest>, | ||
| payment_channel_broken: bool, | ||
| alias: Option<String>, | ||
| note: Option<String>, | ||
| is_hidden: bool, | ||
| } | ||
|
|
||
| let opt_string = |ptr: *const std::os::raw::c_char| -> Option<String> { | ||
| if ptr.is_null() { | ||
| None | ||
| } else { | ||
| Some( | ||
| std::ffi::CStr::from_ptr(ptr) | ||
| .to_string_lossy() | ||
| .into_owned(), | ||
| ) | ||
| } | ||
| }; | ||
| let opt_bytes = |ptr: *const u8, len: usize| -> Option<Vec<u8>> { | ||
| if ptr.is_null() || len == 0 { | ||
| None | ||
| } else { | ||
| Some(slice::from_raw_parts(ptr, len).to_vec()) | ||
| } | ||
| }; | ||
|
|
||
| let mut by_contact: BTreeMap<[u8; 32], PairAccumulator> = BTreeMap::new(); | ||
| for row in rows { | ||
| let contact_id = Identifier::from(row.contact_id); | ||
| let (sender_id, recipient_id) = if row.is_outgoing { | ||
| (*owner_id, contact_id) | ||
| } else { | ||
| (contact_id, *owner_id) | ||
| }; | ||
| let mut request = ContactRequest::new( | ||
| sender_id, | ||
| recipient_id, | ||
| row.sender_key_index, | ||
| row.recipient_key_index, | ||
| row.account_reference, | ||
| opt_bytes(row.encrypted_public_key, row.encrypted_public_key_len) | ||
| .unwrap_or_default(), | ||
| row.core_height_created_at, | ||
| row.created_at, | ||
| ); | ||
| request.encrypted_account_label = | ||
| opt_bytes(row.encrypted_account_label, row.encrypted_account_label_len); | ||
| request.auto_accept_proof = opt_bytes(row.auto_accept_proof, row.auto_accept_proof_len); | ||
|
|
||
| let acc = by_contact.entry(row.contact_id).or_default(); | ||
| if row.is_outgoing { | ||
| acc.outgoing = Some(request); | ||
| } else { | ||
| acc.incoming = Some(request); | ||
| } | ||
| // Relationship-level properties are replicated onto both rows | ||
| // by the persist projection; OR / first-non-null is the safe | ||
| // re-fold. | ||
| acc.payment_channel_broken |= row.payment_channel_broken; | ||
| acc.is_hidden |= row.is_hidden; | ||
| if acc.alias.is_none() { | ||
| acc.alias = opt_string(row.alias); | ||
| } | ||
| if acc.note.is_none() { | ||
| acc.note = opt_string(row.note); | ||
| } | ||
| } | ||
|
|
||
| for (contact_id_bytes, acc) in by_contact { | ||
| let contact_id = Identifier::from(contact_id_bytes); | ||
| match (acc.outgoing, acc.incoming) { | ||
| (Some(outgoing), Some(incoming)) => { | ||
| let mut contact = EstablishedContact::new(contact_id, outgoing, incoming); | ||
| contact.alias = acc.alias; | ||
| contact.note = acc.note; | ||
| contact.is_hidden = acc.is_hidden; | ||
| contact.payment_channel_broken = acc.payment_channel_broken; | ||
| managed.established_contacts.insert(contact_id, contact); | ||
| } | ||
| (Some(outgoing), None) => { | ||
| managed.sent_contact_requests.insert(contact_id, outgoing); | ||
| } | ||
| (None, Some(incoming)) => { | ||
| managed | ||
| .incoming_contact_requests | ||
| .insert(contact_id, incoming); | ||
| } | ||
| (None, None) => unreachable!("accumulator entries always hold at least one row"), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Rejected tombstones are not restored
The new restore_dashpay_contacts bridge restores sent, incoming, and established contact rows, but IdentityRestoreEntryFFI has no rejected-tombstone input and this function never repopulates ManagedIdentity.rejected_contact_requests. SwiftData likewise treats rejected snapshots as delete-only visible-row operations. After relaunch, Rust has no suppression entry for the immutable rejected contactRequest, while stale incoming rows can still be restored or fetched again, so a rejected request can reappear despite the user's action.
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in this update — Rejected tombstones are not restored no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| // (3) Register the external (sending) account — decrypt + ECDH. A | ||
| // decrypt/decode failure is PERMANENT. | ||
| if let Err(e) = self | ||
| .register_external_contact_account( | ||
| identity_id, | ||
| &contact_id, | ||
| &candidate.encrypted_public_key, | ||
| candidate.our_decryption_key_index, | ||
| candidate.contact_encryption_key_index, | ||
| ) | ||
| .await | ||
| { | ||
| tracing::warn!( | ||
| identity = %identity_id, | ||
| contact = %contact_id, | ||
| error = %e, | ||
| "Failed to register DashPay external account; marking payment channel broken (permanent)" | ||
| ); | ||
| self.mark_contact_channel_broken(identity_id, &contact_id) | ||
| .await; | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Transient account-registration failures become permanent
build_contact_accounts still treats every register_external_contact_account error as permanent and marks payment_channel_broken. That callee performs additional network-backed identity/decryption work, so a transient DAPI or fetch failure can be converted into durable local broken-channel state. Future sweeps then skip the contact until the other party sends a superseding request; only genuinely non-recoverable decrypt, decode, missing-key, or key-type failures should take the permanent-broken path.
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in a9599b6 — Transient account-registration failures become permanent no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| if !other.is_valid { | ||
| self.is_valid = false; | ||
| } | ||
| if other.purpose_mismatch { | ||
| self.purpose_mismatch = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: purpose_mismatch survives hard validation errors
The type documents purpose_mismatch as true only when a key-purpose mismatch is the sole invalidity reason, and the sync code uses that flag to choose retry versus permanent broken-channel handling. However, add_error does not clear the flag, add_purpose_error sets it unconditionally, and merge ORs it. A request with both a hard key error and a purpose error can therefore be retried forever instead of being classified as permanently invalid.
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in this update — purpose_mismatch survives hard validation errors no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| if let Ok(mut guard) = this.background_cancel.lock() { | ||
| *guard = None; | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: DashPay sync cleanup can clear a newer cancel token
The DashPay sync worker still unconditionally sets background_cancel to None when its loop exits. If stop() cancels token A, start() installs token B, and the old thread exits afterward, the old cleanup erases B while the new loop is still running. Later stop() or shutdown cannot find the live token, so FFI callback context can be freed while an untracked DashPay thread continues calling through Swift-owned callback pointers. The identity and shielded sync managers already use generation guards to avoid this race.
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in this update — DashPay sync cleanup can clear a newer cancel token no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| fn select_recipient_key_index(recipient_identity: &Identity) -> Result<u32, PlatformWalletError> { | ||
| // Prefer a DECRYPTION key. | ||
| if let Some((id, _)) = recipient_identity.public_keys().iter().find(|(_, k)| { | ||
| k.purpose() == Purpose::DECRYPTION && k.key_type() == KeyType::ECDSA_SECP256K1 | ||
| }) { | ||
| return Ok(*id); | ||
| } | ||
| // Fall back to an ENCRYPTION key (mobile cohort). | ||
| if let Some((id, _)) = recipient_identity.public_keys().iter().find(|(_, k)| { | ||
| k.purpose() == Purpose::ENCRYPTION && k.key_type() == KeyType::ECDSA_SECP256K1 | ||
| }) { | ||
| return Ok(*id); | ||
| } | ||
| Err(PlatformWalletError::InvalidIdentityData( | ||
| "Recipient identity has no ECDSA_SECP256K1 DECRYPTION or ENCRYPTION key".to_string(), | ||
| )) | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Disabled encryption keys are still selectable
select_recipient_key_index still chooses the first DECRYPTION or ENCRYPTION ECDSA_SECP256K1 key without checking disabled_at, while receive-side validation rejects disabled keys. A sender can therefore encrypt the DIP-15 compact xpub to a key the recipient has revoked on-chain, which is the signal that the private half may be compromised. The same missing disabled-key filter remains in the contactInfo root encryption key selector at contact_info.rs:313-320, allowing owner-private alias/note/hidden metadata to be published under a revoked encryption key id.
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in a9599b6 — Disabled encryption keys are still selectable no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "a35c2efe8eb96a98082ae78acfdcbf0d5672c1628268cdc2f4642481647a0911"
)Xcode manual integration:
|
…on, G8 real ciphertext, G9 contract cache - G6: the cfg(not(dashpay-contract)) fallback contract id held the DPNS id — corrected to the deployed DashPay id (latent foot-gun; dead code in default builds). - G7 (send half): the selected sender/recipient key pair now gates through validate_contact_request BEFORE any ECDH or broadcast — same validator the receive/accept paths use since M1. Warnings are logged; hard failures abort the send. Auto-accept stays deliberately dormant: it activates with M5 invitations behind the verify-gate acceptance criterion. - G8: the local sent ContactRequest now stores the real 96-byte encryptedPublicKey off the broadcast document instead of a zero placeholder, so the persisted/SwiftData row matches Platform. - G9: process-wide OnceLock cache for the bundled DashPay contract — replaces five per-call load_system_data_contract re-parses across the profile/contactInfo paths. 226/226 lib tests green.
…19 done G4 scoping found the M3 ECDH-only hook insufficient for watch-only: the friendship/receiving xpub derivations are hardened (seed-bound) on both send and accept, so true watch-only needs xpub-derivation hooks too (or one combined derive-DashPay-context hook). Recorded so the future slice starts from the real constraint, not the incomplete M3 note. Task 20 (live cross-client e2e) marked blocked-external with the manual-run note.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at af53ae0 (incremental over 8037a47). The latest delta — G6 fallback contract id, G7 pre-send key-pair validator, G8 real ciphertext capture, G9 process-wide contract cache — is small and well-scoped, and introduces no new blockers. G7 mitigates the worst-case security half of prior finding #10 on the send path (a disabled recipient key is now caught before ECDH/broadcast), but the underlying selectors still don't filter disabled_at and the contactInfo rootEncryptionKeyIndex selector is still ungated. The remaining nine prior blockers are unchanged at HEAD. Verified the send-side rotation lookup, the rejected-request changeset, the V001 in-place edits (no V002 exists), and the unconditional DashPay sync cancel-token clear by reading the worktree at HEAD. Recommending REQUEST_CHANGES.
🔴 4 blocking
1 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/migrations/V001__initial.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/migrations/V001__initial.rs:178-212: V001 migration edited in place — refinery upgrade from v4.0.0-beta.4 will not pick up new column / table
Verified at HEAD: `contacts.payment_channel_broken` (line 189) and `CREATE TABLE rejected_contact_requests` (lines 203-212) are added directly into V001, and no V002 migration exists anywhere under `packages/rs-platform-wallet-storage/migrations/` (globbed `V*.rs` returns only V001). V001 (without these additions) shipped in v4.0.0-beta.4, and the storage README enforces append-only migrations because refinery checksums each migration in `refinery_schema_history`. Against an existing beta.4 DB this either aborts on divergent checksum or silently skips V001 as already-applied — neither the new column nor the new table will exist. The first runtime write touching `payment_channel_broken` (`mark_contact_channel_broken`) or `INSERT INTO rejected_contact_requests` (G5 tombstone) then fails at SQLite, disabling the PR's payment-channel-broken safety state and the G5 rejection tombstone on every upgraded wallet. Add a V002 migration: `ALTER TABLE contacts ADD COLUMN payment_channel_broken INTEGER` plus a separate `CREATE TABLE rejected_contact_requests (...)`.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:3311-3417: Rejected tombstones are written but never restored on rehydration
`restore_dashpay_contacts` restores sent, incoming, and established rows from the FFI restore spec, but `IdentityRestoreEntryFFI` exposes no rejected-tombstone field — there is no C struct or count/pointer pair for tombstones in the FFI surface at all. The Swift side persists rejected snapshots as write-only deletes on visible rows, and the SQLite reconstruction helper defaults `rejected_contact_requests` to empty. After relaunch, Rust has no suppression entry for the immutable rejected contactRequest, and the sync-loop tombstone check at contact_requests.rs:500 never fires — a rejected request resurfaces on the first sweep. Because the on-platform document is immutable, wiping the tombstone on restart defeats the user's explicit reject and re-exposes them to suppression-bypass by an unwanted sender. Add a per-wallet load reader for `rejected_contact_requests`, surface the rows via a new `rejected_tombstones` field on `IdentityRestoreEntryFFI`, and rehydrate `ManagedIdentity.rejected_contact_requests` from that input.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:3037-3048: 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 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 — a confidentiality regression for users handing off or disposing devices.
In `packages/rs-platform-wallet/src/wallet/identity/network/contact_info.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_info.rs:301-308: Disabled encryption keys are still selectable — contactInfo rootEncryptionKeyIndex and contactRequest selector both miss disabled_at
G7's pre-send validator at `contact_requests.rs:127-138` now gates the contactRequest send path against `validate_contact_request`, which checks `disabled_at`, closing the worst-case 'encrypt DIP-15 compact xpub to a revoked recipient key' confidentiality path. Two residual gaps remain in this PR's scope: (1) `contact_info.rs:301-308` still picks the owner's first ECDSA_SECP256K1 ENCRYPTION key for `rootEncryptionKeyIndex` with no `disabled_at` filter and no equivalent validator gate. The published contactInfo doc references that key id, and if an attacker has the revoked root private material the public id + derivation index lets them derive the same contactInfo AES keys and decrypt the owner's private alias/note/hidden metadata — a real confidentiality exposure on the contactInfo path. (2) `contact_requests.rs:335-351` (`select_recipient_key_index`) still picks the first matching key without `disabled_at`; when a recipient has [disabled DECRYPTION + live ENCRYPTION], the new G7 gate now hard-fails the send instead of falling back to the live ENCRYPTION key — a behavioral regression introduced by this delta. Add `&& k.disabled_at().is_none()` to both selectors so contactInfo is safe and the contactRequest send path picks the live key cleanly.
| note TEXT, | ||
| is_hidden INTEGER, | ||
| accepted_accounts BLOB, | ||
| payment_channel_broken INTEGER, | ||
| updated_at INTEGER NOT NULL DEFAULT (unixepoch()), | ||
| PRIMARY KEY (wallet_id, owner_id, contact_id), | ||
| FOREIGN KEY (wallet_id) REFERENCES wallet_metadata(wallet_id) ON DELETE CASCADE | ||
| ); | ||
|
|
||
| -- Rejected-request tombstone (G5 stage 1). Keyed by | ||
| -- `(wallet_id, owner_id, sender_id, account_reference)` — NOT bare | ||
| -- sender id — so a once-rejected sender can still re-request via a | ||
| -- bumped accountReference (the DIP-15 rotation mechanism), while a | ||
| -- replay of the exact same immutable request stays suppressed. The | ||
| -- optional `document_id` records the rejected document's id for audit / | ||
| -- exact-match suppression. The sync ingest path consults this table | ||
| -- before re-ingesting a received contactRequest. | ||
| CREATE TABLE rejected_contact_requests ( | ||
| wallet_id BLOB NOT NULL, | ||
| owner_id BLOB NOT NULL, | ||
| sender_id BLOB NOT NULL, | ||
| account_reference INTEGER NOT NULL, | ||
| document_id BLOB, | ||
| rejected_at INTEGER NOT NULL DEFAULT (unixepoch()), | ||
| PRIMARY KEY (wallet_id, owner_id, sender_id, account_reference), | ||
| FOREIGN KEY (wallet_id) REFERENCES wallet_metadata(wallet_id) ON DELETE CASCADE | ||
| ); |
There was a problem hiding this comment.
🔴 Blocking: V001 migration edited in place — refinery upgrade from v4.0.0-beta.4 will not pick up new column / table
Verified at HEAD: contacts.payment_channel_broken (line 189) and CREATE TABLE rejected_contact_requests (lines 203-212) are added directly into V001, and no V002 migration exists anywhere under packages/rs-platform-wallet-storage/migrations/ (globbed V*.rs returns only V001). V001 (without these additions) shipped in v4.0.0-beta.4, and the storage README enforces append-only migrations because refinery checksums each migration in refinery_schema_history. Against an existing beta.4 DB this either aborts on divergent checksum or silently skips V001 as already-applied — neither the new column nor the new table will exist. The first runtime write touching payment_channel_broken (mark_contact_channel_broken) or INSERT INTO rejected_contact_requests (G5 tombstone) then fails at SQLite, disabling the PR's payment-channel-broken safety state and the G5 rejection tombstone on every upgraded wallet. Add a V002 migration: ALTER TABLE contacts ADD COLUMN payment_channel_broken INTEGER plus a separate CREATE TABLE rejected_contact_requests (...).
source: ['claude', 'codex']
| unsafe fn restore_dashpay_contacts( | ||
| spec: &IdentityRestoreEntryFFI, | ||
| owner_id: &Identifier, | ||
| managed: &mut ManagedIdentity, | ||
| ) { | ||
| use platform_wallet::{ContactRequest, EstablishedContact}; | ||
|
|
||
| if spec.contacts.is_null() || spec.contacts_count == 0 { | ||
| return; | ||
| } | ||
| let rows = slice::from_raw_parts(spec.contacts, spec.contacts_count); | ||
|
|
||
| /// Per-contact accumulator while pairing the direction rows. | ||
| #[derive(Default)] | ||
| struct PairAccumulator { | ||
| outgoing: Option<ContactRequest>, | ||
| incoming: Option<ContactRequest>, | ||
| payment_channel_broken: bool, | ||
| alias: Option<String>, | ||
| note: Option<String>, | ||
| is_hidden: bool, | ||
| } | ||
|
|
||
| let opt_string = |ptr: *const std::os::raw::c_char| -> Option<String> { | ||
| if ptr.is_null() { | ||
| None | ||
| } else { | ||
| Some( | ||
| std::ffi::CStr::from_ptr(ptr) | ||
| .to_string_lossy() | ||
| .into_owned(), | ||
| ) | ||
| } | ||
| }; | ||
| let opt_bytes = |ptr: *const u8, len: usize| -> Option<Vec<u8>> { | ||
| if ptr.is_null() || len == 0 { | ||
| None | ||
| } else { | ||
| Some(slice::from_raw_parts(ptr, len).to_vec()) | ||
| } | ||
| }; | ||
|
|
||
| let mut by_contact: BTreeMap<[u8; 32], PairAccumulator> = BTreeMap::new(); | ||
| for row in rows { | ||
| let contact_id = Identifier::from(row.contact_id); | ||
| let (sender_id, recipient_id) = if row.is_outgoing { | ||
| (*owner_id, contact_id) | ||
| } else { | ||
| (contact_id, *owner_id) | ||
| }; | ||
| let mut request = ContactRequest::new( | ||
| sender_id, | ||
| recipient_id, | ||
| row.sender_key_index, | ||
| row.recipient_key_index, | ||
| row.account_reference, | ||
| opt_bytes(row.encrypted_public_key, row.encrypted_public_key_len) | ||
| .unwrap_or_default(), | ||
| row.core_height_created_at, | ||
| row.created_at, | ||
| ); | ||
| request.encrypted_account_label = | ||
| opt_bytes(row.encrypted_account_label, row.encrypted_account_label_len); | ||
| request.auto_accept_proof = opt_bytes(row.auto_accept_proof, row.auto_accept_proof_len); | ||
|
|
||
| let acc = by_contact.entry(row.contact_id).or_default(); | ||
| if row.is_outgoing { | ||
| acc.outgoing = Some(request); | ||
| } else { | ||
| acc.incoming = Some(request); | ||
| } | ||
| // Relationship-level properties are replicated onto both rows | ||
| // by the persist projection; OR / first-non-null is the safe | ||
| // re-fold. | ||
| acc.payment_channel_broken |= row.payment_channel_broken; | ||
| acc.is_hidden |= row.is_hidden; | ||
| if acc.alias.is_none() { | ||
| acc.alias = opt_string(row.alias); | ||
| } | ||
| if acc.note.is_none() { | ||
| acc.note = opt_string(row.note); | ||
| } | ||
| } | ||
|
|
||
| for (contact_id_bytes, acc) in by_contact { | ||
| let contact_id = Identifier::from(contact_id_bytes); | ||
| match (acc.outgoing, acc.incoming) { | ||
| (Some(outgoing), Some(incoming)) => { | ||
| let mut contact = EstablishedContact::new(contact_id, outgoing, incoming); | ||
| contact.alias = acc.alias; | ||
| contact.note = acc.note; | ||
| contact.is_hidden = acc.is_hidden; | ||
| contact.payment_channel_broken = acc.payment_channel_broken; | ||
| managed.established_contacts.insert(contact_id, contact); | ||
| } | ||
| (Some(outgoing), None) => { | ||
| managed.sent_contact_requests.insert(contact_id, outgoing); | ||
| } | ||
| (None, Some(incoming)) => { | ||
| managed | ||
| .incoming_contact_requests | ||
| .insert(contact_id, incoming); | ||
| } | ||
| (None, None) => unreachable!("accumulator entries always hold at least one row"), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Rejected tombstones are written but never restored on rehydration
restore_dashpay_contacts restores sent, incoming, and established rows from the FFI restore spec, but IdentityRestoreEntryFFI exposes no rejected-tombstone field — there is no C struct or count/pointer pair for tombstones in the FFI surface at all. The Swift side persists rejected snapshots as write-only deletes on visible rows, and the SQLite reconstruction helper defaults rejected_contact_requests to empty. After relaunch, Rust has no suppression entry for the immutable rejected contactRequest, and the sync-loop tombstone check at contact_requests.rs:500 never fires — a rejected request resurfaces on the first sweep. Because the on-platform document is immutable, wiping the tombstone on restart defeats the user's explicit reject and re-exposes them to suppression-bypass by an unwanted sender. Add a per-wallet load reader for rejected_contact_requests, surface the rows via a new rejected_tombstones field on IdentityRestoreEntryFFI, and rehydrate ManagedIdentity.rejected_contact_requests from that input.
source: ['claude', 'codex']
| let root_key_id = managed | ||
| .identity | ||
| .public_keys() | ||
| .iter() | ||
| .find(|(_, k)| { | ||
| k.purpose() == Purpose::ENCRYPTION && k.key_type() == KeyType::ECDSA_SECP256K1 | ||
| }) | ||
| .map(|(_, k)| k.id()); |
There was a problem hiding this comment.
🔴 Blocking: Disabled encryption keys are still selectable — contactInfo rootEncryptionKeyIndex and contactRequest selector both miss disabled_at
G7's pre-send validator at contact_requests.rs:127-138 now gates the contactRequest send path against validate_contact_request, which checks disabled_at, closing the worst-case 'encrypt DIP-15 compact xpub to a revoked recipient key' confidentiality path. Two residual gaps remain in this PR's scope: (1) contact_info.rs:301-308 still picks the owner's first ECDSA_SECP256K1 ENCRYPTION key for rootEncryptionKeyIndex with no disabled_at filter and no equivalent validator gate. The published contactInfo doc references that key id, and if an attacker has the revoked root private material the public id + derivation index lets them derive the same contactInfo AES keys and decrypt the owner's private alias/note/hidden metadata — a real confidentiality exposure on the contactInfo path. (2) contact_requests.rs:335-351 (select_recipient_key_index) still picks the first matching key without disabled_at; when a recipient has [disabled DECRYPTION + live ENCRYPTION], the new G7 gate now hard-fails the send instead of falling back to the live ENCRYPTION key — a behavioral regression introduced by this delta. Add && k.disabled_at().is_none() to both selectors so contactInfo is safe and the contactRequest send path picks the live key cleanly.
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in this update — Disabled encryption keys are still selectable — contactInfo rootEncryptionKeyIndex and contactRequest selector both miss disabled_at no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
A send from identity A ghosted as an outgoing "Pending" row under identity B after a picker switch — optimisticSentIds is per-identity state and now resets on selection change. Observed live during the rotation UAT (2026-06-13).
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at 9441316. Latest delta is documentation (SPEC.md UAT round 2) plus a per-identity reset of optimisticSentIds in DashPayTabView — no Rust, FFI, or schema changes. All 10 prior blocking findings independently re-verified at HEAD against the worktree and remain valid; both Claude and Codex converge on every one of them. Recommending REQUEST_CHANGES.
🔴 1 blocking
1 additional finding(s) omitted (not in diff).
9 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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:3037-3048: Wallet deletion PHASE 1 omits PersistentDashpayPayment children — same fatal pattern as contactRequests
Verified at HEAD lines 3037-3047: 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 (populated from `DashpayPaymentFFI` rows). The surrounding comment (lines 3013-3036) 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 — a confidentiality regression for users handing off or disposing devices.
2 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