Repository navigation
feat(swift-sdk): send, receive and accept DashPay private contact requests - #5812
QuantumExplorer wants to merge 45 commits into
Conversation
… resolver signer `MnemonicResolverCoreSigner` gains the three key operations DashPay private contact requests (protocol version 15) need, in the style of `contact_info_seal` and `ecdh_shared_secret`: each derives the keys it needs at Rust-built paths, wipes its copies, and returns only the result. - `scan_private_document_page` sorts a page of a private document store with the recipients' secret keys and the senders' outgoing viewing keys. - `read_private_contact_requests` reads a scan's requests, checking each sender with the recipient key that opened it. Only recipient keys are derived. - `seal_private_contact_request` seals a request with the sender's encryption key. A scan or a read derives every key from one resolve of the mnemonic, not one Keychain read per key: `resolve_and_derive` is now a one-path case of the new `resolve_master`. The SDK's refusals come back as the new `MnemonicResolverSignerError::PrivateContactRequest`, keeping the `dash_sdk::Error`. Tests pin the signer to a resident wallet of the same seed: the sealed entry is byte for byte the one the SDK seals with the seed-derived key, and a page scans and reads exactly as with seed-derived scan keys. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…he FFI The resolver-backed `ContactCryptoProvider` implements the three private contact request methods by delegating to `MnemonicResolverCoreSigner`, so the keys stay in the signer. New C exports: - `platform_wallet_sync_private_contact_requests(wallet_handle, core_signer_handle, max_pages, out_requests, out_report)` runs `DashPayView::sync_private_contact_requests` and returns the requests it ingested with a `PrivateContactSyncReportFFI`. The wallet is looked up under the registry guard and the pass runs outside it. - `platform_wallet_manager_send_private_contact_request` and `platform_wallet_manager_accept_private_contact_request` (`shielded` feature) pay from an account's shielded notes, with the shielded spend contract: panics and results go through `catch_spend_panic` and `map_spend_result`, as the other note-spending exports do. - `managed_identity_get_private_contact` reads `PrivateContact`: whether requests came or went privately, and the contact's shielded address. `PrivateContactRequestNeedsPrivateReply` maps to the new result code `ErrorPrivateContactRequestNeedsPrivateReply` (68), so a host routes a privately received request to the private accept. 61 to 67 are already held by open branches; the registry records the claim and those holds. The auto-accept proof copy moved into `read_auto_accept_proof`, shared by the public and private sends. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Thin wrappers over the new platform-wallet FFI: - `ManagedPlatformWallet.syncPrivateContactRequests(maxPages:)` returns a `PrivateContactSyncResult`. - `PlatformWalletManager.sendPrivateContactRequest(...)` and `acceptPrivateContactRequest(...)` pay from the wallet's shielded notes, taking the resolver as the other shielded calls do. - `ManagedIdentity.privateContact(contactId:)` returns `PrivateContactInfo` (came privately, went privately, shielded address), or nil. `PlatformWalletResultCode.errorPrivateContactRequestNeedsPrivateReply` (68) and `PlatformWalletError.privateContactRequestNeedsPrivateReply` carry the public accept's refusal, pinned in `ErrorHandlingTests`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…hem privately - Add Contact has a "Send privately" toggle that sends the request from the wallet's shielded balance. - Accepting a request the public accept refuses as private replies privately instead, in the request list and in Add Contact's collision alert. Rust decides which; the app only reacts to the refusal. - Pull-to-refresh on the request list also reads the private store, which is what records that a request came privately. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t/ffi-swift-private-contact-requests
…er's signature Private contact requests are now signed by the sender's identity signer with one of its authentication keys, as public requests are. The resolver signer keeps only the DashPay encryption key: - `seal_private_contact_request` takes the signing key id and the 65-byte signature and passes them to the SDK seal. It signs nothing itself; the SDK refuses a signature that is not the signing key's. The encryption key, derived for the outgoing viewing key and the key check, is wiped on drop as before. - `read_private_contact_requests` is removed: checking an entry needs no secret key, so the wallet reads the scan itself. - `scan_private_document_page` takes each sender encryption key's id with its path, as `PrivateDocumentScanKeys::with_sender` now does. Tests sign with `SingleKeySigner` through `private_contact_request_signature`, as an identity signer signs data. The sealed entry is still byte for byte the one a resident wallet seals; a scanned page reads as the sender's signed request; a scan and a seal each resolve the mnemonic once; a signature by another key and a wrong encryption key path are refused, each for its own reason. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… request FFI
The private send and accept now take the identity signer the public send
and accept take, and pass it to the wallet, which signs the request with
it:
platform_wallet_manager_send_private_contact_request(handle,
wallet_id_bytes, mnemonic_resolver_handle, account,
sender_identity_id, recipient_identity_id, account_label,
auto_accept_proof, auto_accept_proof_len, shielded_address,
signer_handle, out_request_handle)
platform_wallet_manager_accept_private_contact_request(handle,
wallet_id_bytes, mnemonic_resolver_handle, account, request_handle,
shielded_address, signer_handle, out_established_handle)
`signer_handle` is a `*mut SignerHandle` (a `VTableSigner`), crossed into
the worker as an address and re-materialized there, as
`platform_wallet_send_contact_request_with_signer` does. A null signer is
`ErrorNullPointer` before the wallet is touched.
`ResolverContactCryptoProvider` follows the new provider trait: no
`read_private_contact_requests`, and the seal passes the signing key id and
signature through. `managed_identity_get_private_contact` maps the owned
`PrivateContact`; its docs say the flags now hold across restarts and only
the shielded address waits for a sync.
Code 68 (`ErrorPrivateContactRequestNeedsPrivateReply`) also covers a
public send to a contact whose requests came or went privately; its docs
and the registry row say so.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`PlatformWalletManager.sendPrivateContactRequest` and `acceptPrivateContactRequest` take `signer: KeychainSigner`, last as in `sendContactRequest` and `acceptContactRequest`, and keep it alive across the FFI call with the resolver. The docs say the private request is signed by the same identity signer, that `privateContact(contactId:)` holds across launches (only the shielded address waits for a sync), and that the public send also throws `privateContactRequestNeedsPrivateReply` for a contact whose requests came or went privately. The `ErrorHandlingTests` pin of code 68 carries the wallet's new message. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…in signer Add Contact's private send and the private-accept fallback pass the `KeychainSigner` the public send and accept already use. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
|
|
⛔ Final review complete — 1 blocking finding(s) (commit edc9dd1) · triage: critical |
…t/ffi-swift-private-contact-requests
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Static verification at 6787f32 confirms four blocking issues involving ambiguous-spend retries, post-payment persistence failures, teardown admission, and acceptance orchestration ownership. The cryptographic delegation and ABI mapping are otherwise sound; the claimed missing Rust pin for code 68 already exists. Current CI reports passing Kotlin build/tests and no failed code checks; no local builds or tests were run.
🔴 4 blocking | 🟡 4 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large cross-language change directly implements cryptographic key derivation, private-document scanning and request sealing in packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs, including scan_private_document_page and seal_private_contact_request. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/AddContactView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/AddContactView.swift:471-479: Keep unconfirmed private requests pending instead of enabling a fresh send
The private send can throw `shieldedSpendUnconfirmed`, but the generic catch treats it as an ordinary failure and the defer clears `isSending`. The native send has not recorded an outgoing contact request at this point, although its transition may already have executed or remain queued for byte-identical re-drive. With other spendable notes available, another tap can build a different transition and pay another fee; reserving the first transition's notes does not prevent this. The newly routed private accepts in this view and ContactRequestsView likewise restore their Accept controls after an ambiguous completion. Handle this outcome as pending, following the distinction already made by SendViewModel, and reconcile native spend recovery plus private-store ingestion before allowing a fresh submission. Add injected-unconfirmed coverage for both send and accept.
In `packages/rs-platform-wallet-ffi/src/private_contact_request.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/private_contact_request.rs:528-533: Preserve the committed accept outcome after a contact-store failure
This exported accept can report an ordinary failure after its shielded reply was confirmed. In the native private send, `pay(entry)` succeeds before `add_sent_contact_request`; a failure of that store is logged and swallowed. Because `add_sent_contact_request` persists before updating memory, the failed write leaves neither a sent request nor an established contact. `accept_contact_request_replying_with` then fails its final established-contact lookup with `ContactRequestNotFound`, which this export maps to a generic wallet-operation failure. Retrying before private-store sync recovers the reply can therefore broadcast and pay for another reply. Fix the native private-accept outcome contract so a confirmed payment followed by local persistence failure remains committed/recovery-required, with protection against a fresh reply. Test a failed contact store after successful payment and verify that retry does not invoke payment again.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PrivateContactRequests.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PrivateContactRequests.swift:156-162: Include private spends in teardown admission
Both new manager methods detach their native calls without entering `admitNativeOp`/`finishNativeOp`. Consequently, `deleteWallet` can pass its zero-active-operation guard while either spend is running, and shutdown does not drain these calls. The FFI retains wallet and coordinator Arcs, so removal does not cancel an operation already proving. After proof construction, `private_document_create_from_pool_with_metadata` records activity and broadcasts without a wallet-generation liveness gate; `save_activity` uses `entry(id).or_default()`, allowing removed shielded state to be recreated and persisted after the wipe. Count both Swift operations through completion, and make the native private-spend path participate in the existing generation lifecycle guard so direct native/FFI callers are also protected. Test removal and shutdown while proving is paused.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactsView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactsView.swift:249-257: Move automatic acceptance routing into platform-wallet
This helper attempts public acceptance, interprets the wallet's privacy refusal, and substitutes a shielded-funded private acceptance. That is DashPay orchestration, not argument marshalling or an explicit user-selected channel. The coding conventions assign mobile DashPay orchestration to rs-platform-wallet and require bindings to call an existing Rust operation rather than make these decisions; the motivating example explicitly includes logic in a Swift view. The Rust refusal correctly prevents public disclosure, but every other client must currently reproduce the replacement-operation policy. Add a channel-aware native acceptance operation using the wallet-owned request provenance and reciprocity state, expose it through a thin binding, and have the views supply credentials and acceptance intent. The explicitly public-only API can retain its refusal contract.
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactsView.swift:233-235: Expose failed or incompletely persisted private refreshes
This helper suppresses every private-sync error and discards the report. An unsupported pre-v15 network can be an expected case, but network failures, unavailable resolver material, and a nonzero `unpersistedIdentityCount` do not mean the request list was successfully refreshed. This is the example app's only private-store ingestion call, so these outcomes leave stale or empty results without indicating that private requests could not be checked or durably ingested. Handle the expected unsupported-network case separately and expose operational errors and incomplete persistence through refresh error/banner state.
In `packages/rs-platform-wallet-ffi/src/dashpay.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay.rs:1300-1305: Preserve the missing-mnemonic key-availability signal
When the resolver returns NOT_FOUND during private-store scanning, `MnemonicResolverSignerError::NotFound` carries the reserved key-unavailable marker at position zero. This conversion wraps it in `InvalidIdentityData`, which has no dedicated FFI mapping and therefore becomes `ErrorUnknown` (99). The existing code-31 restoration only recognizes the marker structurally inside `Sdk(Protocol(Generic(...)))`, so it cannot restore the signal from this variant. A caller consequently cannot distinguish missing signing material from an unknown internal failure. Map NotFound explicitly to a key-unavailable outcome and add an FFI mapping test; preserve the structural discriminator rather than introducing substring matching.
In `packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs`:
- [SUGGESTION] packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs:712-726: Share recipient and sender scan-key assembly
`PrivateDocumentKeyPaths::scan_keys` in platform-wallet already performs the recipient-secret and sender-outgoing-viewing-key transformation through a derivation callback. This adds another implementation in the FFI crate, with independently maintained temporary-key cleanup. The wallet still supplies the roles and paths, so this is not an incorrect key-selection finding, but the two implementations can drift as scan-key construction or erasure changes. Keep mnemonic resolution and derivation inside the resolver adapter, and extract the shared assembly into a lower Rust-library helper, such as a callback-based constructor in rs-sdk's private_document_scan module. Preserve this implementation's wipe-on-error handling when sharing it.
- [SUGGESTION] packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs:181-187: Cover the remaining documented seal-refusal inputs
The new error contract documents malformed requests and contracts without the private type, but the signer tests exercise SDK refusals only through an unrelated encryption key and another key's signature. Add malformed-request and missing-private-type cases that assert `MnemonicResolverSignerError::PrivateContactRequest` and verify that no sealed entry is returned. Validation should remain in rs-sdk; these tests would pin the adapter's forwarding and typed-error contract rather than add validation to the binding.
…t/ffi-swift-private-contact-requests
… the FFI `platform_wallet_manager_accept_contact_request_replying_in_kind(handle, wallet_id_bytes, mnemonic_resolver_handle, account, request_handle, shielded_address, signer_handle, out_established_handle)` (`shielded` feature) exposes `PlatformWallet::accept_contact_request_replying_in_kind`: the one accept a host calls. The wallet replies privately to a request that came privately with no reply yet, and with a document (or by adopting the reply already sent) otherwise, so hosts no longer catch code 68 and call the private accept themselves. It is built as `platform_wallet_manager_accept_private_contact_request` is: same arguments and null checks, `catch_spend_panic` and `map_spend_result` for the shielded spend contract, and a null-signer test. The docs of code 68 name it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`PlatformWalletManager.acceptContactRequestReplyingInKind(walletId:resolver:account:request:shieldedAddress:signer:)` is a thin wrapper over the new FFI export: the wallet decides whether the reply is private or a document. The explicitly public and private accepts stay. The file header and the code 68 docs point hosts at it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…quest is replied to The request list and Add Contact's collision alert call `acceptContactRequestReplyingInKind` once instead of trying the public accept and falling back to the private one on code 68. The `acceptIncomingContactRequest` helper that held that choice is removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the findings against head 1c07538. Wallet-owned acceptance routing fixes the previous layering issue, but four blocking issues remain: private operations bypass teardown admission, committed and ambiguous replies can enable duplicate fee-bearing requests, and public acceptance unnecessarily requires shielded configuration. This was a static review with no builds or tests run; current CI shows Kotlin build/tests passing and the review pending, with no Rust or iOS validation jobs listed.
🔴 4 blocking | 🟡 3 suggestion(s)
6 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large cross-language diff directly changes cryptographic key derivation, private-document scanning, request sealing, and signature handling in packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs, including scan_private_document_page and seal_private_contact_request. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/private_contact_request.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/private_contact_request.rs:616-623: Acquire shielded resources only when acceptance needs a private reply
The new in-kind entrypoint calls resolve_wallet_and_coordinator before the wallet chooses public reply, private reply, or adoption. That helper rejects a loaded wallet unless configure_shielded has run, so public acceptance and adoption now fail with 'shielded support not configured' even though neither needs a shielded payment. Both example-app acceptance sites have switched unconditionally to this entrypoint. Resolving the seed here also imposes spend-authority access before the wallet can determine that no payment is needed; delaying derive_spend_keyset inside the wallet does not avoid these earlier failures. Keep routing in rs-platform-wallet, but make shielded-context and spend-authority acquisition lazy behind its private-payment branch. Cover public and adopt-only acceptance without a shielded coordinator.
- [BLOCKING] packages/rs-platform-wallet-ffi/src/private_contact_request.rs:531-536: Preserve the committed accept outcome after a contact-store failure
(existing thread: https://github.com/dashpay/platform/pull/5812#discussion_r4506333080)
A confirmed private reply can reach this Err arm as an ordinary operation failure. DashPayView::send_private_contact_request awaits pay successfully, logs a failed add_sent_contact_request, and returns Ok. That identity update persists before inserting the established contact into memory, so the enclosing accept_contact_request_replying_with then fails its final lookup with ContactRequestNotFound. Both accept exports map this to ErrorWalletOperation, and the views offer acceptance again despite the first reply already having committed and consumed its fee. With no recorded outgoing request to adopt, another attempt can pay for another private entry. Preserve a committed-but-local-repair-pending outcome in rs-platform-wallet's acceptance orchestration and marshal it through both bindings; test a persister failure after successful payment without allowing a second paid reply.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PrivateContactRequests.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PrivateContactRequests.swift:158-164: Include private spends in teardown admission
(existing thread: https://github.com/dashpay/platform/pull/5812#discussion_r4506333083)
sendPrivateContactRequest, acceptPrivateContactRequest, and acceptContactRequestReplyingInKind capture the manager handle and suspend into Task.detached without calling admitNativeOp/finishNativeOp. shutdown() drains only activeNativeOpCount, and deleteWallet() uses that same count to exclude concurrent work, so either can overtake these operations. The native exports release the registry guard after cloning wallet/coordinator Arcs; those preserve object lifetime but do not stop a proof already in progress from broadcasting and writing activity after teardown or deletion. Enroll all three wrappers in balanced admission before their first suspension and retain it through native completion and result conversion. Add lifecycle coverage demonstrating that shutdown waits and deletion is refused while a private operation is active.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/AddContactView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/AddContactView.swift:471-479: Keep unconfirmed private requests pending instead of enabling a fresh send
(existing thread: https://github.com/dashpay/platform/pull/5812#discussion_r4506333077)
When this call throws shieldedSpendUnconfirmed, the generic catch only sets errorMessage and defer clears isSending, immediately enabling Send Request again. The wallet records the outgoing contact request only after pay returns confirmed metadata, so this ambiguous attempt has no contact record that prevents another submission. Shielded reservations protect the first attempt's notes, not the recipient: another call can select other available notes and append another independently sealed request, while the original may still confirm or be re-driven by sync. Both acceptance actions similarly release their in-flight guards on this outcome. Retain the unresolved contact operation and its reconciliation status in rs-platform-wallet, expose it through the bindings, and render it as pending until resolved, including after reopening or restart.
In `packages/rs-platform-wallet-ffi/src/dashpay.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay.rs:1300-1305: Preserve the missing-mnemonic key-availability signal
(existing thread: https://github.com/dashpay/platform/pull/5812#discussion_r4506333098)
MnemonicResolverSignerError::NotFound carries the reserved key-unavailable marker, but this catch-all wraps it in InvalidIdentityData. A missing mnemonic during private scanning therefore becomes ErrorUnknown rather than ErrorSigningKeyUnavailable (31); map_spend_result likewise cannot recover the classification from this wrapper. Keeping the marker somewhere in the rendered message is insufficient because both conversions deliberately recognize it only in the structured SDK ProtocolError::Generic payload. Preserve NotFound through the existing key-unavailable representation while keeping malformed mnemonic and derivation errors distinct. Add a resolver-provider-to-FFI boundary test asserting code 31, not only the signer's intermediate NotFound variant.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactsView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactsView.swift:233-235: Expose failed or incompletely persisted private refreshes
(existing thread: https://github.com/dashpay/platform/pull/5812#discussion_r4506333092)
This helper discards every thrown error and the successful PrivateContactSyncResult. Consequently, a network, proof, or unavailable-mnemonic failure looks like a completed empty refresh, and unpersistedIdentityCount > 0 is ignored even though those requests did not reach durable storage. The wallet retains affected cursors for retry, but the request view receives no indication that recovery is needed. This is the example app's private-store receiving path. Distinguish expected unsupported-network behavior from operational failures, and expose failed or incompletely persisted passes through the refresh/error state with a retry indication.
In `packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs`:
- [SUGGESTION] packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs:712-726: Share recipient and sender scan-key assembly
(existing thread: https://github.com/dashpay/platform/pull/5812#discussion_r4506333099)
This helper adds a second implementation of the scan-key assembly already exposed by rs-platform-wallet's PrivateDocumentKeyPaths::scan_keys and used by the resident provider: derive each recipient's keyed secrets and derive an outgoing viewing key for each sender. The implementations already handle temporary cleanup differently, and future construction changes must now be maintained separately for resident and resolver-backed scans. Extract the common assembly into a lower library accessible to both callers, such as an SDK builder accepting a derivation closure. Retain the resolver's single resolve_master scope and wiping guards, and do not introduce a dependency from rs-sdk-ffi back to platform-wallet. This is a shared-helper maintainability suggestion, not a claim that the current code selects incorrect keys.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Apply lifecycle admission to the existing shieldedTransfer wrapper — PlatformWalletManagerShieldedSync.swift's pre-existing shieldedTransfer also captures the manager handle and runs detached native work without admission. Correcting the new private-contact wrappers alone leaves this older spend able to outlive the manager's admission drain.
- Follow-up: A separate follow-up PR should enroll shieldedTransfer in balanced native-operation admission and test shutdown and deletion during an in-flight transfer.
…t/ffi-swift-private-contact-requests
… derive - `scan_private_document_page` builds its keys with `PrivateDocumentScanKeys::derive`, which wipes the derived keys on an error too. The signer keeps only the mnemonic resolve and the per-path derivation; its own scan-keys helper and `WipingSecretKeys` are gone. - `seal_private_contact_request` takes `platform_version` last and passes it to the SDK seal, which validates the payload with dpp. - Tests follow the SDK: signing data and seals take the platform version, and the read takes `ProvedIdentity` entries proved at the heights the scan needs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… it is one, in every build `platform_wallet_manager_accept_contact_request_replying_in_kind` no longer looks up the shielded coordinator or reads the mnemonic up front. It hands the wallet a payment that, only for a private reply, looks the coordinator up and resolves the seed on a blocking thread. A public or adopted reply therefore works on a manager without `platform_wallet_manager_configure_shielded` and reads no mnemonic for the payment (an FFI test pins both). A failure of that lookup returns the code the lookup reported. A reply for which no payment was asked keeps the codes the public accept returns; once one was asked, errors keep the shielded spend contract. The export and its JNI-facing contract exist in every build. Without the `shielded` feature it calls `DashPayView::accept_contact_request_replying_in_kind` with a payment that refuses: a request that needs a private reply fails with the new `ErrorShieldedBuildRequired` (69), and a public or adopted reply works as before. No existing code meant "shielded unsupported". Also: - `ErrorProtocolVersionNotActive` (70): the SDK's lasting `ProtocolVersionNotActive` refusal, such as the private store before protocol version 15, was `ErrorUnknown`; hosts can now stay quiet about it. A refusal that may lift soon keeps the catch-all. - A missing mnemonic while scanning or sealing the private store maps to the key-unavailable outcome (31), through the signer's position-0 marker in a `ProtocolError::Generic`, matched by variant; a test drives it through the resolver-backed provider. - The seal passes `platform_version`. - The private store sync runs on `run_on_big_stack_thread`: the SDK's identity fetch stream trips rust-lang/rust#100013 against `block_on_worker`'s `Send + 'static` bounds. Both codes are in `ERROR_CODE_REGISTRY.md`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…type codes 69 and 70 - `sendPrivateContactRequest`, `acceptPrivateContactRequest` and `acceptContactRequestReplyingInKind` are admitted with `admitNativeOp` and finished with `finishNativeOp`, so a teardown waits for them as it does for the other counted manager calls. - `PlatformWalletError.shieldedBuildRequired` (69) and `.protocolVersionNotActive` (70), with `ErrorHandlingTests` pins. - The in-kind accept's docs say a public or adopted reply needs neither shielded support nor a payment mnemonic read. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…and report private refresh failures - A private send or an in-kind accept whose result could not be confirmed (`shieldedSpendUnconfirmed`) stays pending, as `SendViewModel` treats an unconfirmed spend: Add Contact shows a Done-only notice and no longer enables Send, and the request row keeps its pending state without its buttons until the next sync settles it. - The request list's private refresh stays quiet when the network does not serve the private store yet (code 70), and shows any other failure, and requests read but not saved, in a notice above the list. - `ContactRequestSubmissionOutcome` and `PrivateContactRefreshNotice` hold those two readings, pinned by `ContactRequestOutcomesTests`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t/ffi-swift-private-contact-requests
…code and read it per contact - `PlatformWalletError::PrivateContactRequestPending` maps to the new `ErrorPrivateContactRequestPending` (71), both in the blanket conversion and in the shielded spend contract the private send and accepts use, which would otherwise flatten it to `ErrorWalletOperation`. A private request to that contact is in flight or paid and not yet read back; nothing new was paid, and a later private sync settles it. Recorded in `ERROR_CODE_REGISTRY.md`. - `managed_identity_get_private_reply_pending(identity_handle, contact_id, out_pending)` is a pure read of `DashPayState::private_reply_pending`, so hosts disable a second send or accept from wallet-owned state. It is a getter rather than a `PrivateContactFFI` field because the latter answers NotFound until a request has come or gone privately, while a first private send is already pending. - The private store sync runs on `block_on_worker` again: the SDK's identity fetch future is `Send` now, so the big-stack thread is no longer needed (`block_on_worker` already polls on the 8 MB worker stack). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…er contact - `PlatformWalletError.privateContactRequestPending` (71), with an `ErrorHandlingTests` pin. The private send, private accept and in-kind accept docs name it. - `ManagedIdentity.privateReplyPending(contactId:)` wraps `managed_identity_get_private_reply_pending`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The request rows and Add Contact's Send read whether the wallet has a private request to the contact in flight or not yet read back (`privateRepliesPending`, over `ManagedIdentity.privateReplyPending`), instead of keeping an unconfirmed accept in screen-local state. While it is set, the row shows pending without its buttons and Send is off; it clears when a private sync settles it, and it holds when the screen is left and reopened. The views re-read it on appear, after an accept or send, and after each sync pass. `privateContactRequestPending` (71) is the same outcome as an unconfirmed spend: pending, no fresh submission. `ContactRequestOutcomesTests` covers it and the row message. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t/ffi-swift-private-contact-requests
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification of head 745eb92 confirms three blocking issues: native teardown fencing remains incomplete, pending private submissions lose their protection across restart, and the collision flow can publish a reply despite the selected private-send option. Six prior findings are fixed, and the withdrawn coverage suggestion remains withdrawn. Current CI shows successful Kotlin validation and no failed code checks, but no Rust or iOS validation checks are listed; no builds or tests were run locally.
🔴 3 blocking
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, cross-language diff directly changes cryptographic key derivation, private-document scanning, sealing, and signature handling in packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs, including scan_private_document_page and seal_private_contact_request. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/private_contact_request.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/private_contact_request.rs:425-443: Include private spends in teardown admission
The Swift admission scopes protect calls through PlatformWalletManager, but these new C entry points still allow removal to overtake a private payment. After resolving the wallet and coordinator, this call enters the native payment path without acquiring its generation's payment guard or checking that the generation is current. A direct FFI caller can remove the wallet while proof construction is paused: platform_wallet_manager_remove_wallet takes the exclusive generation guard and unregisters the shielded state, but the retained spend subsequently calls record_pending_activity and broadcasts. Both shielded stores recreate the removed subwallet through save_activity's entry(id).or_default(), and the retained persister queues activity after teardown. The omission also exists in older shielded operations, but that does not fence these newly exported paths; the native portion of the prior finding remains unresolved, and no linked tracking issue supports its proposed deferral. Add generation-aware admission in rs-platform-wallet, recheck liveness under the lifecycle guard before post-proof persistence or broadcast, and cover removal while proving is paused.
- [BLOCKING] packages/rs-platform-wallet-ffi/src/private_contact_request.rs:253-258: Keep unconfirmed private requests pending instead of enabling a fresh send
The pending getter and Submitted UI prevent retries only within the current process. DashPayState::pending_private_replies is memory-only, and this getter explicitly returns false after restart. For example, a private reply can execute, fail its post-payment private-store read-back, and return code 71 without recording an outgoing request. After relaunch, the incoming request remains actionable but its pending reservation is gone; Add Contact and the request list enable another send or accept before private-store reconciliation. An ambiguous payment has the same problem even when its original transition remains durably queued in shielded_pending_spends. With other spendable notes, another attempt seals a fresh entry and pays another fee; reserving the original notes does not prevent both operations from executing. Private ingestion runs on pull-to-refresh, while dashPaySyncNow does not ingest the private store. Persist and restore the per-contact operation association in rs-platform-wallet, or gate submissions until native recovery establishes the original outcome, and add restart coverage for both paid-but-not-read and ambiguous submissions.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/AddContactView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/AddContactView.swift:569-574: Preserve the selected privacy mode through the collision flow
When Send privately is enabled and the recipient has already sent a public request, tapping Send opens the collision alert. Its Accept action reaches this call without preserving sendPrivately: native needs_private_reply is false for the public incoming request, so in-kind acceptance broadcasts a public contactRequest identifying this user as the owner and the counterparty as toUserId. The alert only asks whether to accept instead; it does not disclose that this overrides the selected privacy option and publishes a reciprocal confirmation. The counterparty's earlier unilateral request does not already disclose that confirmation. Preserve the requested privacy through a Rust-owned acceptance operation, or require explicit informed consent before sending a public reply. Add coverage for a private-send collision with a public incoming request.
…t/ffi-swift-private-contact-requests
…t/ffi-swift-private-contact-requests
…e of code 71 `ErrorPrivateContactRequestPending` (71) is now also returned when an identity's private store could not be read to its end before its first private send or accept since load: the wallet reads it first so that a reply paid for in an earlier process is adopted rather than paid for again. Nothing was paid, and a later private sync or a retry settles it. The FFI enum doc, the registry row and Swift's `privateContactRequestPending` doc say so. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ollision accept With "Send privately" on and a request already received from the recipient, the collision alert's accept called the in-kind accept. A publicly received request then got a public `contactRequest` reply, which publishes this user as its owner and the contact as `toUserId`, against the chosen option and without saying so. The accept now follows the toggle, through the pure `CollisionAccept` helper: - On, it replies privately with `acceptPrivateContactRequest`, paid from the shielded balance. The Rust private accept takes a publicly received request too. The alert's button reads "Accept privately" and its message says the reply is private and paid from the shielded balance. - Off, it is the in-kind accept, as before. The accept is refused while the wallet reports a private reply to the contact pending, as Send is. Outcomes are read as elsewhere: an unconfirmed spend or code 71 stays pending. `ContactRequestOutcomesTests` pins both toggle states. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t/ffi-swift-private-contact-requests
…t/ffi-swift-private-contact-requests
…tact-requests' into feat/ffi-swift-private-contact-requests
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the findings against head 9b24f50: ten prior findings are fixed and the withdrawn finding remains withdrawn, but restart recovery still allows a public submission before an unresolved private request is recovered. One additional non-blocking error-mapping inconsistency is confirmed. This was a static review; current CI reports Kotlin build/tests passing and the review pending, with no independent Rust or Swift check results.
🔴 1 blocking | 🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large cross-language change directly adds cryptographic and key-handling logic in mnemonic_resolver_core_signer.rs, notably scan_private_document_page and seal_private_contact_request, alongside private shielded request submission. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/shielded_send.rs:626: Preserve the lifecycle refusal code in private spend results
When removal wins before a private payment acquires its generation guard, `pay_private_contact_request` returns `PlatformWalletError::WalletRemovedBeforeSend`. The new private-payment exports pass this through `map_spend_result`, whose catch-all converts it to `ErrorWalletOperation` (6), although the blanket wallet-error conversion maps it to `NotFound` (98). The native guard prevents payment correctly, but the host loses the typed lifecycle outcome promised by this API and cannot distinguish removal from an ordinary operation failure without parsing the message. Preserve this variant alongside the pending-request variant and add a mapping regression test asserting code 98.
In `packages/rs-platform-wallet-ffi/src/private_contact_request.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/private_contact_request.rs:776-781: Keep unconfirmed private requests pending instead of enabling a fresh send
(existing thread: https://github.com/dashpay/platform/pull/5812#discussion_r4507400051)
Restart recovery protects another private payment, but this in-kind acceptance can choose a public reply before either recovery safeguard runs. Receive a public request, accept it privately through Add Contact, get spend-unconfirmed or paid-but-not-read-back code 71, then relaunch before private-store ingestion. The incoming public request survives, but the pending reservation is memory-only and the getter returns false. There is no production caller of `PlatformWallet::restore_pending_private_contact_requests`, and the app explicitly ingests the private store only on pull-to-refresh. Accept is therefore enabled again. In `DashPayView::accept_contact_request_replying_in_kind`, both `request.came_privately()` and `needs_private_reply(request)` are false, so acceptance skips the store read and sends a public reply without entering the payment closure that restores saved unconfirmed associations.
The added pre-payment restore and first-private-operation scan address private retries, but not this public branch. It can publish the user's reciprocal confirmation and charge another fee while the original private reply remains unresolved. Add Contact's default-public send has the same gap because its native privacy gate checks only currently tracked requests and reservations. Complete Rust-owned recovery/readiness before allowing public sends or selecting any acceptance reply: restore unconfirmed associations, reconcile confirmed-but-unread private requests, and fail closed when recovery cannot complete. Add restart coverage for an unresolved private reply to a public incoming request, exercising the exported in-kind accept and a public send before manual refresh, and assert that neither broadcasts.
…tact-requests' into feat/ffi-swift-private-contact-requests Takes the wallet's batch scan of the private contact request store and its PaymentStarted pay token: the in-kind accept's non-shielded refusal takes the token and never marks it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ent at NotFound `map_spend_result` sent `WalletRemovedBeforeSend` through its catch-all to `ErrorWalletOperation` (6), while every other export gives it the lifecycle refusal code `NotFound` (98). It now passes through the blanket conversion beside `PrivateContactRequestPending`, so a host can tell a removed wallet from an ordinary failure without reading the message. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…est scan state The wallet keeps where each identity reads DashPay's private contact request store from next, and its private contacts' shielded addresses, behind `PRIVATE_CONTACT_REQUEST_SCAN_STATE` (bit 14). This carries them across the FFI and stores them in SwiftData, so a relaunch resumes the private sync instead of reading the store from its start. FFI: three size-gated `PersistenceCallbacksExtension` slots, appended after the asset-lock operation id pair. The persist slot receives a round's read positions, addresses, address removals and the identities the round removed (whose scan state the host clears), after the contacts callback. The load slot returns a wallet's positions and addresses during the wallet-list load; they are applied to the identities that load restored, and a host error or a missing buffer fails the load. The capability is attested only with all three slots, the wallet-list restore callbacks and the declaration, and nothing is read from a host that does not attest it. Swift: two new models, `PersistentPrivateContactRequestsPosition` and `PersistentPrivateShieldedAddress`, in a new live schema V5; V4 is bound to its published snapshot with a lightweight V4 to V5 stage. The handler writes them inside the changeset round, purges them with the wallet, and declares the capability. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… pending `privateRepliesPending` read a failed wallet read as "nothing pending", so the request list brought back Accept and Ignore, and Add Contact enabled Send, with nothing showing the pending request had cleared. The read now returns nil on a failure, and `PrivateRepliesPendingSnapshot` keeps the last successful snapshot and leaves those controls off for every contact until a read covering it succeeds. An unmanaged identity still reads as nothing pending. A pending private request (code 71), which the first public send or accept since load can now meet, gets its own outcome: nothing was sent, so the view shows it as pending and Add Contact offers "Sync private requests", after which Send comes back for the user to try again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…in Storage Explorer `PersistentPrivateContactRequestsPosition` and `PersistentPrivateShieldedAddress` get a row, a count, a list view and a detail view, as `PersistentIdentityBalanceMetadata` does, so the explorer covers every model `DashModelContainer` registers again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…g pending `privateRepliesPending` caught a failed wallet read and returned an empty set, so Requests brought back Accept and Ignore, and Add Contact enabled Send, with nothing showing the pending request had cleared. The read now returns null on a failure, and `PrivateRepliesPendingSnapshot` keeps the last successful snapshot and leaves those controls off for every contact until a read covering it succeeds. An unmanaged identity still reads as nothing pending. A pending private request (code 71), which the first public send or accept since load can now meet, gets its own outcome: nothing was sent, so Add Contact shows it as pending and offers "Sync private requests", after which Send comes back for the user to try again. Port of the Swift change on #5812. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the findings against head 342162b: three blockers remain involving restart-time payment recovery, unchecked scan-provider binding, and incomplete persistence capability negotiation. Eleven prior findings are fixed, one remains valid, and one remains withdrawn; three additional suggestions are retained. This was static verification only: current code checks pass, while the review check remains pending.
🔴 3 blocking | 🟡 3 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large cross-language change directly implements private request signing and shielded payment flows in packages/rs-platform-wallet-ffi/src/private_contact_request.rs and changes persisted scan-state handling and SwiftData schema migration. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/private_contact_request.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/private_contact_request.rs:156-162: Verify the scan provider before advancing durable cursors
This export constructs a resolver-backed provider and immediately invokes the state-mutating scan without verifying that the resolver supplies this wallet's seed. The native scan also performs no binding check. A different valid mnemonic derives valid but unrelated keys: `PrivateDocumentScanKeys::scan_page` finds no matching requests but still advances `next_position`. The new persistence extension then saves that progress. Correcting the resolver does not trigger a rewind because `private_scan_key_fingerprint` hashes the managed identity's expected index and public keys, not the keys actually derived by the provider. Requests skipped by the wrong-seed pass can consequently remain missing across launches. Establish the checked boundary in rs-platform-wallet before scan-state mutation, reusing the existing `verify_seed_binds` mechanism rather than duplicating validation in FFI. Add a wrong-mnemonic scan followed by a correct-mnemonic scan, asserting that refusal leaves the cursor unchanged and the correct scan recovers the request.
- [BLOCKING] packages/rs-platform-wallet-ffi/src/private_contact_request.rs:776-781: Keep unconfirmed private requests pending instead of enabling a fresh send
(existing thread: https://github.com/dashpay/platform/pull/5812#discussion_r4507400051)
The bind-time restore and read-before-decision gate leave public acceptance available before pending-payment recovery. `load_from_persistor` discards the shielded snapshot, and `should_not_reply_publicly_after_a_restart_with_a_private_reply_unconfirmed` explicitly observes no pending reservation before binding. After restarting with an unresolved private reply to a public incoming request, the committed-store scan can reach its end without finding that unexecuted reply. This in-kind operation can then choose a public reciprocal, while the original private transition remains capable of executing. The getter also exports a successful false during this interval. Additionally, `install_shielded_views` logs reservation-restoration failures and returns success, leaving the same public branch unprotected after a failed restore. The hooks described in your latest reply therefore cover successful binding and already-appended entries, but not recovery readiness. Include the remaining rs-platform-wallet fix from #5810: restore or rule out durable pending associations before exposing definitive pending state or admitting public submissions, or refuse those operations until recovery succeeds. Cover acceptance before binding and after restoration failure without requiring spend authority for genuinely public-only wallets.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:1797-1803: Require contact persistence before enabling resumable private scans
The structural capability mask accepts the scan-state callback trio and wallet-list restore even when `on_persist_contacts_fn` is absent; the new `scan_state_persister` fixture explicitly constructs and accepts this combination. In that configuration, `FFIPersister::store` silently skips contact writes and returns success. Native ingestion therefore treats the requests as persisted and saves an advanced cursor through the scan-state extension. On restart, the cursor and matching fingerprint are restored without the corresponding requests, so later scans skip the entries needed to reconstruct them. This changes an ephemeral-contact host from rereading the store to durably losing requests. Require the contact-persistence callback as part of this capability's structural dependencies, and cover a declared scan-state capability with the trio present but the contact callback missing.
In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/shielded_send.rs:628-629: Preserve protocol-version refusals through the private spend mapper
The explicit private send and accept use this mapper, but their preliminary private-store read can return `PlatformWalletError::Sdk(ProtocolVersionNotActive)` before any payment. That error reaches the catch-all and becomes `ErrorWalletOperation` (6), bypassing the blanket conversion that maps a lasting refusal to code 70 and leaves a transient refusal as `ErrorUnknown`. The same refusal therefore has a typed outcome through private sync or pre-payment in-kind acceptance but a generic operation failure through the explicit private APIs. Pass this structured SDK variant through the blanket conversion and add mapping regressions for both lasting and transient refusals.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:12133-12136: Fail scan-state loads when a stored row cannot cross the ABI
These filters turn malformed persisted rows into successful absence. For example, an address row with an invalid byte length is discarded while its identity's valid cursor is returned successfully. Rust then resumes beyond the entry that supplied the address, so an ordinary private refresh cannot reconstruct the omitted state. The load boundary already propagates storage errors; unrepresentable rows should preserve that same distinction between unreadable state and an empty store. Validate all rows before allocating either output and return a load error if any row cannot be marshalled.
In `packages/rs-platform-wallet-ffi/src/contact_persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/contact_persistence.rs:229-236: Extend ABI layout guards to the new FFI structs
The existing contact structs have compile-time size and alignment assertions, but the new scan-position and shielded-address structs have none. `PrivateContactSyncReportFFI` and `PrivateContactFFI` in `private_contact_request.rs` likewise lack these guards. Extend the existing regression protection to the new ABI shapes: on the supported 64-bit targets their size/alignment pairs are 72/8, 107/1, 24/8, and 46/1 respectively. These assertions protect against accidental size or alignment changes; field-offset assertions would be needed to detect same-size field reordering.
…tact-requests' into feat/ffi-swift-private-contact-requests Takes the wallet's copy of codes 68 to 71 and `map_spend_result`'s 98, with code 71 now also mapping `PrivateContactPaymentsNotRestored`, its doc and registry row extended. One more test page literal gains `chunk_power: None`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…uest FFI structs Compile-time size, alignment and field-offset guards, as the contact structs have: the scan-position row (72/8), the shielded-address row (107/1), `PrivateContactSyncReportFFI` (24/8) and `PrivateContactFFI` (46/1), on 64-bit targets. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… scan-state capability A read position vouches for the contact requests read before it. Without `on_persist_contacts_fn`, `FFIPersister::store` skips those requests and returns success, so the advanced position was stored without them and a restart skipped the entries that carried them. The capability now also needs the contacts callback; a host with the scan-state trio and the declaration but no contacts callback is not attested and has nothing read at load. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…h map_spend_result The store read a private send or accept makes before paying can return `Sdk(ProtocolVersionNotActive)`, which the spend mapper flattened to `ErrorWalletOperation` (6). It now passes through the blanket conversion, like `PrivateContactPaymentsNotRestored`: a lasting refusal is 70, a transient one `ErrorUnknown`, as on every other path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…re by its DashPay code Each private sync pass, and the store read a send or accept makes first, now checks that the resolver's seed is the wallet's. A mismatch reached the host as `ErrorUnknown` (or `ErrorWalletOperation` through the spend mapper); it now takes `ErrorInvalidParameter`, the code every DashPay export and the seed check give it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…oss the ABI The load shim filtered out stored rows of the wrong length and handed back the rest, so an address row of 42 bytes vanished while its identity's read position was restored, and the sync resumed past the entry that carried the address. Every row is now checked before either array is allocated, and one that cannot be marshalled fails the load as an unreadable store does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… replies The private sync and `PrivateContactInfo` docs said the store cursors and shielded addresses live in memory only; they are restored where the store attests `privateContactRequestScanState`, with the full read as the fallback. `privateReplyPending` is restored too once the shielded store is configured after the load or the wallet binds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…load A wallet that bound a shielded account in an earlier process has its public contact requests refused (code 71) until Rust restores its unconfirmed private payments, which configuring the shielded store does with no keys. The app configured it only when binding, which needs the mnemonic; it now configures the same per-network path right after `loadFromPersistor`, so the bind's own call is a no-op. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e sync Code 71 does not mean the wallet holds a reservation for the contact: it is also returned, with nothing reserved, when the private store could not be read to its end before the first send or accept since load. The views dropped the refusal and read the pending state, which then found nothing and brought Send and Accept back with no notice. `PrivateRepliesPendingSnapshot` now keeps the refused contact pending, with the private sync offered, until a private sync completes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… load A wallet that bound a shielded account in an earlier process has its public contact requests refused (code 71) until Rust restores its unconfirmed private payments, which configuring the shielded store does with no keys. The app configured it only when binding, which needs the mnemonic; it now configures the same per-network path right after `loadPersistedWallets`. Port of the Swift change on #5812. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…te sync Code 71 does not mean the wallet holds a reservation for the contact: it is also returned, with nothing reserved, when the private store could not be read to its end before the first send or accept since load. Add Contact's public Send, its collision accept and the Requests rows dropped the refusal and read the pending state, which then found nothing and brought the controls back with no notice. `PrivateRepliesPendingSnapshot` now keeps the refused contact pending, with the private sync offered, until a private sync completes. Port of the Swift change on #5812. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At edc9dd1, most prior issues are fixed, but reopening a previously shielded wallet without shielded support still loses the recovery hold and can permit a public reply while an earlier private reply is unresolved. The complete private-sync path also still loses the missing-mnemonic error code. This was static verification only; current CI reports successful Kotlin build/tests and SwiftData explorer checks, with the review check pending and no failed code checks reported.
🔴 1 blocking | 🟡 1 suggestion(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This large cross-language change adds shielded-payment send and accept execution in packages/rs-platform-wallet-ffi/src/private_contact_request.rs and changes SwiftData schema migration in DashModelContainer.swift, directly touching funds movement and storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/private_contact_request.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/private_contact_request.rs:867-876: Keep unconfirmed private requests pending instead of enabling a fresh send
(existing thread: https://github.com/dashpay/platform/pull/5812#discussion_r4507400051)
The load-time recovery hold fixes the before-bind window only in shielded builds, but this new in-kind accept is exported in every build. Without `shielded`, `FFIPersister::load` omits shielded hydration, `ClientStartState` has no shielded field, and manager load never calls `hold_until_private_payments_restored`; the readiness flag therefore remains false and no pending reservation is restored.
Reopen a persisted wallet containing a public incoming request and an ambiguous private reply whose payment is saved as unconfirmed but has not executed. The preliminary private-store read cannot discover that unexecuted reply, so this branch chooses and broadcasts a public reply. Its code-69 payment closure is never reached because the chosen reply is public. The original private transition can subsequently execute or be redriven, producing both another paid request and the public reciprocal relationship the private operation avoided.
Preserve recovery-readiness and the necessary non-spending payment metadata across feature configurations in rs-platform-wallet's persistence contract. A build unable to recover an earlier private payment must fail closed rather than treat unavailable recovery state as no pending operation. Add a persisted-unconfirmed-reply fixture exercised without `shielded` and verify that public send and in-kind accept cannot broadcast.
In `packages/rs-platform-wallet-ffi/src/dashpay.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay.rs:1135: Preserve the missing-mnemonic key-availability signal
(existing thread: https://github.com/dashpay/platform/pull/5812#discussion_r4506333098)
The variant-aware conversion fixes direct page scans, but an actual private sync with a scan-eligible managed identity first calls `verify_seed_binds`, which invokes `receiving_xpub`. When the resolver returns NOT_FOUND, `extended_public_key` returns `MnemonicResolverSignerError::NotFound`, and this conversion changes it to `InvalidIdentityData`. The exported sync consequently returns ErrorUnknown (99), not ErrorSigningKeyUnavailable (31), before reaching the repaired page-scan conversion. Swift therefore cannot route this failure to key repair.
Use the shared variant-preserving mapper here. The existing regression invokes `provider.scan_private_document_page` directly and bypasses seed verification; add coverage through wallet sync or the exported sync with a populated wallet and a NOT_FOUND resolver, asserting code 31.
Basic explanation
What this does: iOS apps can now send, receive and accept DashPay private contact requests.
KeychainSignerthe app passes for public requests.Value: the private request flow from #5810, usable from iOS without the app handling any key.
Risks: low.
Issue being fixed or feature implemented
Part of anonymous DashPay contact requests for 5.1, on top of #5810.
What was done?
rs-sdk-ffi (
MnemonicResolverCoreSigner)scan_private_document_pagesorts a page with the recipients' secret keys and each sender encryption key's outgoing viewing key, all derived inside the signer from one resolve of the mnemonic.seal_private_contact_requestderives the sender's encryption key, seals the request with the identity signer's signature, and wipes the key.resolve_and_deriveis now a one-path case of the newresolve_master.MnemonicResolverSignerError::PrivateContactRequest.platform-wallet-ffi
ResolverContactCryptoProviderimplements the wallet's two private methods over that signer.platform_wallet_sync_private_contact_requests(wallet_handle, core_signer_handle, max_pages, out_requests, out_report)platform_wallet_manager_send_private_contact_request(handle, wallet_id_bytes, mnemonic_resolver_handle, account, sender_identity_id, recipient_identity_id, account_label, auto_accept_proof, auto_accept_proof_len, shielded_address, signer_handle, out_request_handle)(shieldedfeature)platform_wallet_manager_accept_private_contact_request(handle, wallet_id_bytes, mnemonic_resolver_handle, account, request_handle, shielded_address, signer_handle, out_established_handle)(shieldedfeature)managed_identity_get_private_contact(identity_handle, contact_id, out_contact)signer_handleis theSignerHandleplatform_wallet_send_contact_request_with_signertakes. The send and accept follow the shielded spend contract (catch_spend_panic,map_spend_result).ErrorPrivateContactRequestNeedsPrivateReply. It is returned when a public send or accept would publish a contact whose requests came or went privately. Recorded inERROR_CODE_REGISTRY.md.swift-sdk
ManagedPlatformWallet.syncPrivateContactRequests(maxPages:)returns aPrivateContactSyncResult.PlatformWalletManager.sendPrivateContactRequest(...)andacceptPrivateContactRequest(...)take the resolver, as the other shielded calls do, andsigner: KeychainSigner, as the public send and accept do.ManagedIdentity.privateContact(contactId:)returnsPrivateContactInfo(came privately, went privately, shielded address), or nil.PlatformWalletError.privateContactRequestNeedsPrivateReplycarries code 68.SwiftExampleApp
Changes from review
platform_wallet_manager_accept_contact_request_replying_in_kind(...)(every build) lets the wallet pick the reply: private when the request came privately and nothing was sent back yet, otherwise public or adopted. Its private reply is paid only when it is one. Swift'sacceptContactRequestReplyingInKind(...)wraps it, and SwiftExampleApp accepts only through it.KeychainSignerthe public send takes. The scan keys come from the SDK'sPrivateDocumentScanKeys::derive.managed_identity_get_private_reply_pending(identity_handle, contact_id, out_pending)reads whether a private request to a contact is still in flight. Swift wraps it asManagedIdentity.privateReplyPending(contactId:).acceptPrivateContactRequest). The alert says the reply is private and paid from the shielded balance. With the toggle off it is unchanged.NotFound).PersistenceCallbacksExtensionslots behind thePLATFORM_WALLET_PERSISTENCE_CAPABILITY_PRIVATE_CONTACT_REQUEST_SCAN_STATEcapability.NotFound(98) on the private-payment exports.ErrorShieldedBuildRequired: a private reply on a build withoutshielded.ErrorProtocolVersionNotActive: the network is below protocol version 15.ErrorPrivateContactRequestPending: a private request to that contact is in flight or not read back yet, or the identity's private store could not be read to its end before its first private operation since load. Nothing new was paid, and the host must not submit it again.ERROR_CODE_REGISTRY.md. 71 survivesmap_spend_result.How Has This Been Tested?
cargo test -p rs-sdk-ffi --lib -- mnemonic_resolver_core_signer: 20 passed, 6 of them private. The private ones:cargo test -p platform-wallet-ffi --lib -- private_contact: 5 passed; with--features shielded: 7 passed, including null signer handles.cargo clippy -p rs-sdk-ffi -p platform-wallet-ffi --all-targets -- -D warnings, and-p platform-wallet-ffi --features shielded: clean../build_ios.sh --target sim, then SwiftExampleApp: BUILD SUCCEEDED.ErrorHandlingTestsagainst that framework on an iPhone 17 Pro simulator: 65 passed, including codes 68 to 71 and every field ofPrivateContactInfo.ContactRequestOutcomesTests: 9 passed, including code 71 as pending and both states of the collision accept.cargo test -p platform-wallet-ffi --lib --features shielded -- private: 17 passed, including the pending getter (false for a fresh identity,NotFoundfor an unknown handle,ErrorNullPointerfor a null output).Breaking Changes
None. New exports, plus result codes 68 to 71.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code