Repository navigation
feat(platform-wallet)!: private contact requests derive the sender's per-contact shielded address - #5902
Conversation
…s per-contact shielded address A private DashPay contact request took the shielded address it carries from the host app, so giving each contact a different address depended on every host. The wallet now derives it as it does for a contact request document: `send_reserved_private_contact_request` calls `IdentityWallet::contact_shielded_address` with the request's sender, recipient and account reference, and holds the identity placement lock from the derivation until the request is signed. The `shielded_address` parameter is removed from `send_private_contact_request`, `accept_private_contact_request` and `accept_contact_request_replying_in_kind` in the wallet, from the three `platform_wallet_manager_*` C functions, from the JNI bridge, and from the Swift and Kotlin SDK methods. The address a contact gave us, read from a received request, is unchanged at every layer. The private contact request tests that need a request carrying a known address give it through a test-only hook on `contact_shielded_address`; `shielded/contact_address_tests.rs` covers what that function derives. Co-Authored-By: Claude Fable 5.2 <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 — no blockers (commit 13b83da) · triage: critical |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The wallet-side derivation and cross-layer parameter removal are consistent, and no blocking correctness or interoperability issue is present. The remaining meaningful gap is that private-send tests bypass the real prepared-account derivation; the new test override also has a guard API that can silently lose or overwrite state. Kotlin CI passed, with no failed code checks reported.
🟡 2 suggestion(s)
Approved: nothing here blocks the merge. The non-blocking findings are optional: fix them forward, in this PR or a follow-up, without waiting for another review round.
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Exercise real address derivation through a private request
packages/rs-platform-wallet/src/wallet/identity/network/private_contact_requests.rs:5377-5384
The address-carrying private-request fixtures install give_every_contact, and contact_shielded_address returns from that hook before examining the sender, recipient, account reference, prepared account, viewing keys, or network schema. The separate shielded/contact_address_tests.rs tests the derivation helper directly, but no test verifies the new private-send connection between PreparedContactRequest.account_reference, the sender's prepared account, and the request payload. A swapped sender/recipient or incorrect reference at the private call site could therefore leave both test groups passing. Add a shielded wallet-level test that prepares the sender's actual identity account, sends without the override, and compares the carried address with AccountViewingKeys::contact_address for the request tuple; a private acceptance reply should also be covered if practical to verify the reversed roles.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: rust-quality)
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: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: 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 is an intricate cross-language change to shielded address derivation and identity/key handling in private contact request signing, including placement-lock concurrency behavior. - 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— 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 (PastaClaw). How to work through them:
- Verify each finding against the code; fix only what is real, as narrowly as the finding.
- If a finding is wrong, decline it on its thread in one or two sentences; the bot answers once and stops.
- To defer one, link a tracking issue on its thread (or reply asking the bot to file one).
- Suggestions don't block the merge, but most are worth fixing in this PR; judge each on its merits.
- Keep the PR's scope; don't grow it to satisfy hardening suggestions.
- Carried items were raised in an earlier round: read the thread first, and don't re-fix what you already fixed, declined or deferred there. Disputed and deferred items are already handled and are not listed.
In `packages/rs-platform-wallet/src/wallet/identity/network/private_contact_requests.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/private_contact_requests.rs:5377-5384: Exercise real address derivation through a private request
The address-carrying private-request fixtures install `give_every_contact`, and `contact_shielded_address` returns from that hook before examining the sender, recipient, account reference, prepared account, viewing keys, or network schema. The separate `shielded/contact_address_tests.rs` tests the derivation helper directly, but no test verifies the new private-send connection between `PreparedContactRequest.account_reference`, the sender's prepared account, and the request payload. A swapped sender/recipient or incorrect reference at the private call site could therefore leave both test groups passing. Add a shielded wallet-level test that prepares the sender's actual identity account, sends without the override, and compares the carried address with `AccountViewingKeys::contact_address` for the request tuple; a private acceptance reply should also be covered if practical to verify the reversed roles.
In `packages/rs-platform-wallet/src/wallet/identity/network/contact_shielded_address.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/contact_shielded_address.rs:156-168: Make the test override guard non-discardable and nestable
The new `GivenAddress` guard unconditionally resets the thread-local value to `None` on drop. Calling `give_every_contact(address);` as a statement silently disables the hook immediately, and nested guards cause the inner guard to clear the outer guard's value. The current call sites bind the guard explicitly, so this is not a present test failure, but the helper's API permits future tests to unknowingly exercise the real `None` path or lose an enclosing override. Store the previous value in the guard and mark it `#[must_use]`.
| /// While it lives, every contact request sent from this thread carries the address. | ||
| pub(crate) struct GivenAddress(()); | ||
|
|
||
| /// Have every contact request this thread sends carry `address`. | ||
| pub(crate) fn give_every_contact(address: [u8; 43]) -> GivenAddress { | ||
| GIVEN.set(Some(address)); | ||
| GivenAddress(()) | ||
| } | ||
|
|
||
| impl Drop for GivenAddress { | ||
| fn drop(&mut self) { | ||
| GIVEN.set(None); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Make the test override guard non-discardable and nestable
The new GivenAddress guard unconditionally resets the thread-local value to None on drop. Calling give_every_contact(address); as a statement silently disables the hook immediately, and nested guards cause the inner guard to clear the outer guard's value. The current call sites bind the guard explicitly, so this is not a present test failure, but the helper's API permits future tests to unknowingly exercise the real None path or lose an enclosing override. Store the previous value in the guard and mark it #[must_use].
| /// While it lives, every contact request sent from this thread carries the address. | |
| pub(crate) struct GivenAddress(()); | |
| /// Have every contact request this thread sends carry `address`. | |
| pub(crate) fn give_every_contact(address: [u8; 43]) -> GivenAddress { | |
| GIVEN.set(Some(address)); | |
| GivenAddress(()) | |
| } | |
| impl Drop for GivenAddress { | |
| fn drop(&mut self) { | |
| GIVEN.set(None); | |
| } | |
| /// While it lives, every contact request sent from this thread carries the address. | |
| #[must_use] | |
| pub(crate) struct GivenAddress(Option<[u8; 43]>); | |
| /// Have every contact request this thread sends carry `address`. | |
| pub(crate) fn give_every_contact(address: [u8; 43]) -> GivenAddress { | |
| let previous = GIVEN.get(); | |
| GIVEN.set(Some(address)); | |
| GivenAddress(previous) | |
| } | |
| impl Drop for GivenAddress { | |
| fn drop(&mut self) { | |
| GIVEN.set(self.0); | |
| } | |
| } |
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)
Basic explanation
What this does: A private DashPay contact request can carry a private (shielded) payment address for the contact. Until now the app using the wallet had to supply that address. Now the wallet works it out itself, one address per contact, the same way it already does for ordinary contact requests in #5901. The app can no longer pass one in.
Value: An app can no longer hand the same shielded address to two contacts by mistake, which would let them see they are paying the same person and would stop the wallet from telling their payments apart. Private and ordinary contact requests now give a contact the same address for the same request, and two devices restored from one seed agree on it.
Risks: Low to medium. Three calls in the Rust wallet, the C interface, the Swift SDK and the Kotlin SDK lose a parameter, so apps that pass
shieldedAddressmust drop it. A private request now goes out without an address when the identity's shielded account was not prepared in this session, where before the app could supply one. Sending holds the identity placement lock from deriving the address until the request is signed. This pull request is stacked on #5901, which is stacked on #5288, and cannot merge before them.Issue being fixed or feature implemented
The DashPay shielded payment addresses DIP (dashpay/dips#195) requires that a wallet never gives the same shielded address to two contacts. #5901 makes the wallet derive the address for ordinary contact requests. Private contact requests still took the address from the host app, through an optional
shielded_addressparameter, so the rule depended on every host getting it right.Base branch. This is based on
claude/dashpay-contact-address-send(#5901), which adds the derivation this change calls. The changes of this pull request are its last commit.What was done?
Wallet (
packages/rs-platform-wallet)send_reserved_private_contact_requestderives the address withIdentityWallet::contact_shielded_address, from the request's sender, recipient and account reference, exactly assend_claimed_contact_requestdoes for an ordinary request. It holds the identity placement lock from the derivation until the request is signed.shielded_address: Option<[u8; 43]>parameter is removed fromsend_private_contact_request,accept_private_contact_requestandaccept_contact_request_replying_in_kind, onPlatformWalletand on the DashPay view, and from the internal functions between them.encryptedShieldedAddressproperty. Otherwise the request goes out without an address.C interface (
packages/rs-platform-wallet-ffi)platform_wallet_manager_send_private_contact_request,platform_wallet_manager_accept_private_contact_requestandplatform_wallet_manager_accept_contact_request_replying_in_kindlose theirshielded_addresspointer parameter.Swift and Kotlin SDKs, JNI (
packages/swift-sdk,packages/kotlin-sdk,packages/rs-unified-sdk-jni)sendPrivateContactRequest,acceptPrivateContactRequestandacceptContactRequestReplyingInKindlose theirshieldedAddressparameter, with their callers in the example apps.The address a contact gave us, read from a received request, is unchanged at every layer (
PrivateContactFFI.shielded_address, SwiftPrivateContactInfo.shieldedAddress, Kotlin's contact type).Example, Swift:
How Has This Been Tested?
Changed tests:
rs-platform-wallet,private_contact_requests.rs: 73 call sites drop the argument. The two fixtures that need a request carrying a known address (read, store and forget of a received address) give it through a test-only hook oncontact_shielded_address, so they still prove that what that function answers is what the request carries. What the function derives is covered byshielded/contact_address_tests.rsfrom feat(platform-wallet): contact requests carry the sender's per-contact shielded address #5901.rs-platform-wallet-ffi,private_contact_request.rs: six calls drop the null pointer.FfiSmokeTest: the three probes drop the argument. They used to be refused for a 42-byte address and are now refused for the null resolver.Run locally:
cargo test -p platform-wallet --features shielded --lib -- private_contact_request contact_address contact_request(223 passed)cargo test -p platform-wallet-ffi --features shielded --lib private_contact_request(22 passed)cargo checkofplatform-walletandplatform-wallet-ffi, each with and without theshieldedfeature, and ofrs-unified-sdk-jniwith its default featurescargo clippyonplatform-walletandplatform-wallet-ffi(--features shielded --lib --tests) and onrs-unified-sdk-jni: no warnings./gradlew :sdk:compileDebugKotlin :sdk:compileDebugAndroidTestKotlin :app:compileDebugKotlin(BUILD SUCCESSFUL)cargo fmt --all -- --checkandpython3 scripts/check_sdk_parity_manifest.pyNot tested:
rs-unified-sdk-jniwithout theshieldedfeature was not compiled.FfiSmokeTestwas compiled, not run; it needs an emulator and a rebuilt native library.Breaking Changes
No consensus change.
API: three calls lose the parameter that carried a host-chosen shielded address, in the Rust wallet (
shielded_address), the C interface (shielded_address), Swift (shieldedAddress:) and Kotlin (shieldedAddress). Callers remove the argument. These calls exist only onv5.1-devand are in no release.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