Repository navigation
feat(platform-wallet): contact requests carry the sender's per-contact shielded address - #5901
QuantumExplorer wants to merge 6 commits into
Conversation
…ntact-address-send
`ContactRequestInput` takes an optional 43-byte shielded payment address. The SDK encrypts it under the request's shared key with an IV of its own and writes it as `encryptedShieldedAddress`. It refuses the address where the network's contact requests have no such property, which they gain with DashPay v3 at protocol version 15. The wallet's write helper passes the address through; the wallet does not supply one yet. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
…tact-address-send
…ntact-address-send
…tact shielded address When a public contact request is sent, the wallet derives the shielded payment address the sending identity gives that contact and hands it to the SDK, which encrypts it into the request. The address is the one the identity's shielded account derives for this request alone, so every contact gets a different one. The request carries an address only when the identity's shielded account was prepared in this session, which proves with the seed that the identity holds the account's slot, is still the account of that slot, has its viewing keys bound, and the connected network's contact requests have the property. It goes without one otherwise. The send is refused if the address would be one already given to another contact. The identity's placement is held from the derivation until the request is signed. The identity wallet, which sends contact requests, holds no shielded keys, so it shares the platform wallet's viewing keys and prepared accounts through `ContactShieldedAddresses`. 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
 |
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-11-12T22:50:30.827Z |
|
🔍 Review in progress — actively reviewing now (commit e7965fd) · triage: critical |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the findings against d83cf98: three blocking issues remain in lifecycle lock ordering, preparation invalidation, and SDK capability discovery, alongside three non-blocking suggestions. This was a static review; Kotlin SDK build/tests remain pending, completed checks pass, and the two newer base commits do not resolve these issues or introduce merge conflicts. The independently confirmed proposal-fee estimate defect comes from the separately merged #5897 and is recorded as a non-blocking follow-up.
🔴 3 blocking | 🟡 3 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: platform-versioning); 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 a large, cross-cutting change that introduces shielded-address derivation and encryption, alters key-handling and contact-request serialization, and adds protocol-version-gated 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— 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 (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/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:1074-1077: Preserve lifecycle-before-placement ordering through signing
The placement read guard remains held until line 1146, but signing first awaits `GatedSigner::hold_gate_after_signing`, which acquires the generation lifecycle read guard. This reverses `send_shielded_tip`'s lifecycle → placement order. Tokio's write-preferring locks make a permanent cycle possible: the contact send holds placement while awaiting its signer; discovery queues a placement writer; a tip send holds lifecycle and queues its placement reader behind discovery; removal queues the lifecycle writer; then the contact signature returns and its lifecycle reader queues behind removal. Completing the signer and network responses cannot release this cycle. Release placement before acquiring lifecycle, then reacquire placement and revalidate the derived account in canonical order before accepting/publishing the signature, or otherwise enforce one acquisition order without recursively acquiring lifecycle. Add a deterministic contention test covering these four operations.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:1074-1077: Expose whether the wallet included a shielded address
The wallet deliberately sends without an address when preparation, account ownership, bound keys, or network support is absent, but these cases produce neither a structured diagnostic nor an inclusion indicator in the wallet's returned/persisted `ContactRequest`. Consequently, a successful wallet send does not tell the host whether the new feature was used, which makes missing preparation and compatibility downgrades difficult to diagnose. Preserve the optional-field behavior, but expose inclusion status or log the specific omission reason. This concern is at the wallet API boundary: the SDK's `SendContactRequestResult` does include the submitted document and can be inspected directly.
In `packages/rs-platform-wallet/src/wallet/shielded/contact_address.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/contact_address.rs:48-62: Keep the preparation and selected viewing keys in one valid generation
`standing_account()` checks the coordinator generation only at this instant; neither the preparation nor its coordinator is retained while the viewing-key read awaits. Clear can complete after that check, invalidate the preparation, and unregister the receiving account, while this method still returns keys and the send proceeds through contract fetching and signing with an address. A Clear followed by a rebind before the key read can also make the old seed-proven preparation authorize keys installed by the new binding. The placement lock excludes identity moves, not Clear or viewing-key installation, and there is no later preparation check. Carry a preparation/key lease tied to the same coordinator generation and seed-proven account through the final signing/publication checks, and serialize it appropriately with Clear and rebinding. Add suspended-send tests for Clear and Clear-plus-rebind; the existing sequential Clear test does not cover this window. The newer base's operation-admission mechanism is relevant, but this send path does not acquire it.
In `packages/rs-sdk/src/platform/dashpay/contact_request.rs`:
- [BLOCKING] packages/rs-sdk/src/platform/dashpay/contact_request.rs:468-479: Discover network capability before resolving the address-capable contract
A fresh unpinned mainnet/testnet SDK starts at protocol version 13. With `RecipientIdentity::Identity` and local ECDH/xpub callbacks, no proved network read occurs before `fetch_dashpay_contract()`. That helper accepts the context provider's contract at `self.version()`, so the new property check rejects `shielded_address: Some(...)` even when the connected network runs protocol version 15. Signing cannot reach its nonce query to discover the newer version because document creation already failed. The wallet's explicit contract fetch masks this for wallet callers, but direct SDK callers remain broken. Establish the protocol requirement or fetch the authoritative network contract inside `create_contact_request` before resolving the schema when an address is supplied, leaving the legacy `None` path unchanged. Test the public creation API from the default protocol floor with supplied identities, and inspect/decrypt the resulting property; also cover `None` and a genuinely unsupported network.
- [SUGGESTION] packages/rs-sdk/src/platform/dashpay/contact_request.rs:111-114: Record the public Rust input change as breaking
Adding an `Option` field to this publicly constructible struct still breaks downstream struct literals. The PR description correctly identifies the migration, but the title lacks the breaking-change marker required by the repository's PR template, and the Unreleased changelog has no retained migration note. Mark the PR title as breaking and document that existing callers must add `shielded_address: None` to retain their behavior. This is a Rust source-compatibility change, not a reason to allocate a PlatformVersion generation.
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:79-85: Include established contacts in the issued-index collision check
`sent_contact_requests()` contains pending outgoing requests. Once a relationship establishes, its outgoing request is held in `established_contacts()[...].outgoing_request`, so its issued index disappears from this comparison set. Both `DashPayState::outgoing_request()` and `prior_sent_account_reference()` already account for these two storage locations. Although an 87-bit collision is extraordinarily unlikely, the advertised refusal currently excludes established relationships. Include their outgoing references when collecting comparison candidates, and test candidate collection independently of the hash so coverage does not require finding an actual collision.
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.
- Proposal surcharge is missing from returned fee estimates — The proposal-fee change from separately merged #5897 supplies
additional_fixed_fee_cost, but paid-event validation adds it only to balance admission, not the returnedFeeResult. CheckTx uses that result directly forgas_wanted, omitting 20,000,000,000 credits per proposal even though execution adds the surcharge. This code was imported unchanged through the v5.1-dev merge and is independent of contact-address sending.- Follow-up: Open a focused follow-up PR for #5897 in the shared fee-validation/estimation implementation, with protocol-appropriate versioning and CheckTx coverage proving the fixed component is reported and charged exactly once.
| let placement = self.identity_placement.read().await; | ||
| let shielded_address = self | ||
| .contact_shielded_address(sender_identity_id, recipient_identity_id, account_reference) | ||
| .await?; |
There was a problem hiding this comment.
🔴 Blocking: Preserve lifecycle-before-placement ordering through signing
The placement read guard remains held until line 1146, but signing first awaits GatedSigner::hold_gate_after_signing, which acquires the generation lifecycle read guard. This reverses send_shielded_tip's lifecycle → placement order. Tokio's write-preferring locks make a permanent cycle possible: the contact send holds placement while awaiting its signer; discovery queues a placement writer; a tip send holds lifecycle and queues its placement reader behind discovery; removal queues the lifecycle writer; then the contact signature returns and its lifecycle reader queues behind removal. Completing the signer and network responses cannot release this cycle. Release placement before acquiring lifecycle, then reacquire placement and revalidate the derived account in canonical order before accepting/publishing the signature, or otherwise enforce one acquisition order without recursively acquiring lifecycle. Add a deterministic contention test covering these four operations.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor)
| let standing = self | ||
| .prepared_accounts | ||
| .lock() | ||
| .unwrap_or_else(PoisonError::into_inner) | ||
| .get(identity_id) | ||
| .and_then(PreparedTipAccount::standing_account); | ||
| if standing != Some(account) { | ||
| return None; | ||
| } | ||
| self.viewing_keys | ||
| .read() | ||
| .await | ||
| .as_ref()? | ||
| .get(&account) | ||
| .cloned() |
There was a problem hiding this comment.
🔴 Blocking: Keep the preparation and selected viewing keys in one valid generation
standing_account() checks the coordinator generation only at this instant; neither the preparation nor its coordinator is retained while the viewing-key read awaits. Clear can complete after that check, invalidate the preparation, and unregister the receiving account, while this method still returns keys and the send proceeds through contract fetching and signing with an address. A Clear followed by a rebind before the key read can also make the old seed-proven preparation authorize keys installed by the new binding. The placement lock excludes identity moves, not Clear or viewing-key installation, and there is no later preparation check. Carry a preparation/key lease tied to the same coordinator generation and seed-proven account through the final signing/publication checks, and serialize it appropriately with Clear and rebinding. Add suspended-send tests for Clear and Clear-plus-rebind; the existing sequential Clear test does not cover this window. The newer base's operation-admission mechanism is relevant, but this send path does not acquire it.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor)
| // Encrypt the shielded address if provided, under an IV of its own | ||
| let encrypted_shielded_address = input | ||
| .shielded_address | ||
| .map(|address| { | ||
| encrypted_shielded_address( | ||
| contact_request_document_type, | ||
| &shared_key, | ||
| &address, | ||
| &mut rng, | ||
| ) | ||
| }) | ||
| .transpose()?; |
There was a problem hiding this comment.
🔴 Blocking: Discover network capability before resolving the address-capable contract
A fresh unpinned mainnet/testnet SDK starts at protocol version 13. With RecipientIdentity::Identity and local ECDH/xpub callbacks, no proved network read occurs before fetch_dashpay_contract(). That helper accepts the context provider's contract at self.version(), so the new property check rejects shielded_address: Some(...) even when the connected network runs protocol version 15. Signing cannot reach its nonce query to discover the newer version because document creation already failed. The wallet's explicit contract fetch masks this for wallet callers, but direct SDK callers remain broken. Establish the protocol requirement or fetch the authoritative network contract inside create_contact_request before resolving the schema when an address is supplied, leaving the legacy None path unchanged. Test the public creation API from the default protocol floor with supplied identities, and inspect/decrypt the resulting property; also cover None and a genuinely unsupported network.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor)
| let others: Vec<(Identifier, u32)> = managed | ||
| .dashpay() | ||
| .sent_contact_requests() | ||
| .values() | ||
| .filter(|request| request.recipient_id != *to_user_id) | ||
| .map(|request| (request.recipient_id, request.account_reference)) | ||
| .collect(); |
There was a problem hiding this comment.
🟡 Suggestion: Include established contacts in the issued-index collision check
sent_contact_requests() contains pending outgoing requests. Once a relationship establishes, its outgoing request is held in established_contacts()[...].outgoing_request, so its issued index disappears from this comparison set. Both DashPayState::outgoing_request() and prior_sent_account_reference() already account for these two storage locations. Although an 87-bit collision is extraordinarily unlikely, the advertised refusal currently excludes established relationships. Include their outgoing references when collecting comparison candidates, and test candidate collection independently of the hash so coverage does not require finding an actual collision.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general)
| let placement = self.identity_placement.read().await; | ||
| let shielded_address = self | ||
| .contact_shielded_address(sender_identity_id, recipient_identity_id, account_reference) | ||
| .await?; |
There was a problem hiding this comment.
🟡 Suggestion: Expose whether the wallet included a shielded address
The wallet deliberately sends without an address when preparation, account ownership, bound keys, or network support is absent, but these cases produce neither a structured diagnostic nor an inclusion indicator in the wallet's returned/persisted ContactRequest. Consequently, a successful wallet send does not tell the host whether the new feature was used, which makes missing preparation and compatibility downgrades difficult to diagnose. Preserve the optional-field behavior, but expose inclusion status or log the specific omission reason. This concern is at the wallet API boundary: the SDK's SendContactRequestResult does include the submitted document and can be inspected directly.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)
| /// The sender's shielded payment address for this contact: 43 raw bytes, the address | ||
| /// the sender's wallet derives for this request alone. The SDK encrypts it as | ||
| /// `encryptedShieldedAddress`. `None` sends the request without one. | ||
| pub shielded_address: Option<[u8; SHIELDED_ADDRESS_LEN]>, |
There was a problem hiding this comment.
🟡 Suggestion: Record the public Rust input change as breaking
Adding an Option field to this publicly constructible struct still breaks downstream struct literals. The PR description correctly identifies the migration, but the title lacks the breaking-change marker required by the repository's PR template, and the Unreleased changelog has no retained migration note. Mark the PR title as breaking and document that existing callers must add shielded_address: None to retain their behavior. This is a Rust source-compatibility change, not a reason to allocate a PlatformVersion generation.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor)
…orm into claude/dashpay-contact-address-send
Basic explanation
What this does: When the wallet sends a DashPay contact request, it now includes a private (shielded) payment address made for that one contact, encrypted so that only the contact can read it. The wallet works the address out itself, from the contact request and the sending identity's shielded account. The host app does not choose it.
Value: A contact can pay the sender privately, and the sender can tell which contact a payment was meant to come from, because every contact gets a different address. Two devices restored from one seed derive the same address for the same request, with nothing to store or keep in step. Until now nothing in the wallet filled the contract field that #5892 added.
Risks: Low to medium. Sending a contact request changes in three ways. It makes one more network read (the DashPay contract) when the identity's shielded account is ready. It holds the identity placement lock from deriving the address until the request is signed. And a request goes out without an address when the account was not prepared in this session, without telling the host app yet. This pull request is stacked on #5288 and cannot merge before it. Recipients do nothing with the address yet; reading it is a later change.
Issue being fixed or feature implemented
The DashPay shielded payment addresses DIP (dashpay/dips#195) has a wallet give each contact a different shielded address, carried in the contact request as
encryptedShieldedAddress. The contract field is in #5892 and the derivation rule in #5893, both merged. Nothing sends an address yet.Base branch. The address comes from the identity shielded account, which #5288 adds, so this is based on
feat/dashpay-shielded-tips. That branch does not have #5893 and #5897 yet, so until it mergesv5.1-devthe diff here also shows those two. The changes of this pull request are the last two commits: about 630 lines inrs-sdk,rs-sdk-cxxandrs-platform-wallet.What was done?
SDK (
packages/rs-sdk/src/platform/dashpay/contact_request.rs)ContactRequestInputtakes an optionalshielded_address, 43 raw bytes.create_contact_requestencrypts it under the request's shared key, with an IV of its own, and writes it asencryptedShieldedAddress.contactRequesttype has no such property, which it gains with DashPay v3 at protocol version 15.Wallet (
packages/rs-platform-wallet)IdentityWallet::contact_shielded_addressderives the address for a request from its sender, recipient and account reference, withAccountViewingKeys::contact_address.send_claimed_contact_requestcalls it after the account reference is known and passes the address to the SDK.prepare_shielded_identity_address), which proves with the seed that the identity holds the account's slot;ContactShieldedAddresses.Example, a contact request from an identity whose shielded account was prepared, on a network at protocol version 15:
The same request from an identity whose account was not prepared, or on a network before protocol version 15, is sent as before.
How Has This Been Tested?
New tests:
rs-sdk,contact_request.rs: the field encrypts to 64 bytes that decrypt back, with a different IV each time, and is refused against a protocol version 14 contract.rs-platform-wallet,shielded/contact_address_tests.rs: the derived address is the one the identity's own account derives, the same for the same request and different for another contact or another account reference; no address before the account is prepared, after the preparation is cleared, on a network without the property, or for an identity the wallet does not hold.Run locally:
cargo test -p dash-sdk --lib dashpay::contact_request(16 passed)cargo test -p platform-wallet --features shielded --lib -- contact_address shielded::keys(17 passed)cargo test -p platform-wallet --features shielded --lib -- contact_request viewing_key_bind tips::(251 passed)cargo clippy -p platform-wallet --features shielded --lib --testscargo checkofplatform-walletwith and without theshieldedfeature, and ofdash-sdk-cxxNot tested: the collision refusal, since a collision cannot be produced, and a full send against a network.
Breaking Changes
No consensus change.
Rust source compatibility:
dash_sdk'sContactRequestInputgains the fieldshielded_address, so code that builds it with a struct literal must set it (Nonekeeps today's behaviour).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