feat(platform-wallet): dip-13 dashpay invitations#4041
Open
shumkov wants to merge 38 commits into
Open
Conversation
DashPay invitations (DIP-13 sub-feature 3'): inviter funds a one-time asset-lock voucher and shares a self-contained dashpay://invite link; the invitee registers their own identity from the imported voucher key and optionally sends a contact request back. Reviewed by 3 adversarial spec agents (feasibility/security/scope) + 4 research streams; owner-synced decisions folded: InstantSend proof (short IS-scoped expiry for staleness), opt-in-both-ends contact bootstrap, proper wallet-persister persistence. Core claim mechanic confirmed against code (put_to_platform_with_private_key); seedless path-gated voucher-key export is the one net-new critical piece. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
crypto/invitation.rs: encode/parse the dashpay://invite?data= link and a fail-fast validate_claimable pre-submit check. Self-contained versioned binary payload (voucher key + embedded InstantSend AssetLockProof + advisory expiry + optional inviter contact-bootstrap info), base58 in the URI. - Hand-rolled LE wire format (no dependency on the crate's optional serde feature); the embedded AssetLockProof rides on the always-available dpp::bincode encoding. - Bounds the base58 input before decode + a hard decoded-byte cap (anti-DoS, spec §8 Finding 5); rejects trailing bytes, bad version, truncation. - validate_claimable: rejects a past-expiry link, a non-InstantSend proof, and a voucher key that doesn't control the funded credit output (fail-fast UX over an opaque consensus reject). - Debug for ParsedInvitation redacts the voucher key. 13/13 unit tests green (round-trip, malformed rejections, validation). Spec slice 1 / spike S4. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The seedless voucher-key export the create-invitation flow needs (feasibility review's one blocker): production wallets are external-signable at steady state, so the voucher key can only come from the Keychain resolver, not a resident Wallet. - rs-sdk-ffi: MnemonicResolverCoreSigner::export_invitation_private_key, gated to the EXACT DIP-13 invitation path 9'/coin'/5'/3'/idx'. Deliberately stricter than the feature check alone — feature 5' is shared with the user's own identity-auth (5'/0'), registration-funding (5'/1'), and top-up (5'/2') keys, so a looser gate would be a key-exfiltration hole. Mirrors the sanctioned export_auto_accept_private_key exception. - ContactCryptoProvider gains export_invitation_private_key (trait + FFI glue + test doubles). Negative test pins the gate: exports 5'/3', rejects 5'/0', 5'/1', 5'/2', 16', wrong purpose, and wrong length. Spec slice 2 / spike S2. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
network/invitation.rs on IdentityWallet: - create_invitation: funds a one-time asset-lock voucher at the DIP-13 invitation account (InstantSend proof, owner decision), exports the path-gated voucher key, and packages a dashpay://invite link. Amount capped in Rust (MAX_INVITATION_DUFFS) so a leaked link's blast radius is bounded below the UI. - claim_invitation: registers a NEW invitee identity funded by the imported voucher — ordinary registration whose asset-lock signature uses the imported raw voucher key via the SDK's put_to_platform_with_private_key, wrapped in the CL-height-too-low retry. Bypasses the wallet's AssetLockFunding machinery (the invitee owns neither the lock's inputs nor its tracking). Best-effort IdentityManager bookkeeping mirrors register_identity_with_funding. Contact-bootstrap is deliberately separate: on success the UI asks the invitee whether to establish contact with the sender, then calls the existing contact-request path. Compile-verified against the real SDK APIs; runtime e2e rides testnet funding. Spec slice 3. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements DashPay DIP-13 invitations across Rust wallet, storage, FFI, Swift SDK, and ExampleApp layers. It adds bounded invitation links, voucher-key gating, create/claim flows, persistence callbacks and models, deep-link handling, invitation UI, and specifications with QA coverage. ChangesDashPay Invitations (DIP-13)
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Inviter
participant IdentityWallet
participant Platform
participant InviteeApp
participant SwiftData
Inviter->>IdentityWallet: create_invitation
IdentityWallet->>Platform: fund and broadcast asset-lock
IdentityWallet-->>Inviter: dashpay://invite URI
IdentityWallet->>SwiftData: persist sent invitation
InviteeApp->>IdentityWallet: parse and validate invitation
InviteeApp->>IdentityWallet: claim_invitation
IdentityWallet->>Platform: submit identity registration
Platform-->>InviteeApp: registered identity
InviteeApp->>Platform: send optional contactRequest
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Two adversarial reviews (correctness + blockchain-security) rated the core ship-worthy (export gate sound, codec panic-free, consensus linkage correct). Folding the findings: - H1 (high): create_invitation now rejects a ChainLock proof. create_funded_ asset_lock_proof falls back to Chain if IS doesn't propagate in 300s, and the invitee's validate_claimable accepts only Instant — so a slow-IS create would otherwise emit a link the invitee silently rejects (dead voucher). Now errors clearly; the funding lock stays reclaimable. - H1b: dropped the dead CL-height (10506) retry in claim_invitation — it only helps ChainLock proofs, which the claim path never carries. Direct submit. - LOW-1 (key hygiene): zeroize the encode buffer + decoded parse bytes; Drop on ParsedInvitation scrubs the voucher scalar (mirrors the resolver's key hygiene). - LOW-2: expiry_unix==0 guard + MAX_INVITATION_TTL_SECS (FFI clamp). Spec §8 reframed: the leaked-link bound is the Rust amount cap + reclaim, NOT the advisory expiry (a leaked-link finder ignores the UI's expiry check). 13/13 codec tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
TEST_PLAN.md §4.10 DashPay: DP-12 (create invitation), DP-13 (claim), DP-14 (two-wallet invite→claim e2e, the acceptance gate), DP-15 (reject malformed / reused / expired). Marked 🔌 (not-wired) until the SwiftUI screens land; entry points + a11y ids described so they flip to ✅ when the UI ships. Indexes + multiwallet tag updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
shumkov
changed the title
C-ABI entry points wrapping the platform-wallet invitation flows (authored via the swift-rust-ffi-engineer agent, verified + integrated): - platform_wallet_create_invitation(wallet, amount_duffs, funding_account_index, inviter_identity_id?, inviter_username?, now_unix, core_signer_handle, out_uri, out_outpoint): derives expiry = now + MAX_INVITATION_TTL_SECS, builds the InviterInfo only when inviter_identity_id is non-null, and drives one MnemonicResolverCoreSigner as both the asset-lock signer and (wrapped) the ContactCryptoProvider that exports the voucher key. Rejects now_unix == 0. - platform_wallet_claim_invitation(wallet, uri, identity_index, identity_pubkeys, count, signer_handle, now_unix, out_identity_id, out_identity_handle): parses the link, then registers the invitee identity via claim_invitation. Out-params get FFI-safe sentinels before any fallible work. 6 marshalling-guard tests green (null pointers, bad URI, missing username, zero now, unknown wallet). No identity signer on create (pure voucher creation needs only the core signer). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…itation Completes review LOW-1: secp256k1 SecretKey has no Drop-zeroize, so wipe the exported voucher key with non_secure_erase() once it lives in the (secret) URI. Pairs with the ParsedInvitation Drop scrub on the claim side. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…edPlatformWallet Swift wrappers over the invitation FFI (authored via the swift-rust-ffi-engineer agent; verified compiling + linking): - createInvitation(amountDuffs:fundingAccount:inviterIdentityId:inviterUsername: nowUnix:) async throws -> String — returns the dashpay://invite link (secret). - claimInvitation(uri:identityIndex:identityPubkeys:signer:nowUnix:) async throws -> ManagedIdentity — registers the invitee identity from the imported voucher. Follows the established KeychainSigner + MnemonicResolver handoff + async + PlatformWalletResult.check() idiom. SwiftExampleApp xcodebuild green (0 errors, iPhone 17 / arm64 — the arm64-sim xcframework; the generic destination's x86_64 link slice is not produced by build_ios.sh --target sim). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…half) Proper wallet-persister integration for the "Sent invitations" status list + (future) reclaim — the Rust half of the persistence split (Swift SwiftData model + InvitationsView is the counterpart). - changeset: InvitationChangeSet + InvitationEntry + InvitationStatus (Created/Claimed/Reclaimed); optional `invitations` field on PlatformWalletChangeSet (clean-additive — all construction sites use ..Default). Merge = last-write-wins by outpoint. apply_changeset drops it (persistence-only; no in-memory replay in v1). - storage: V003 `invitations` table (all-primitive columns, no lifecycle blob) + schema::invitations::apply/read + persister dispatch. NO secret column — the voucher key is re-derived from funding_index (secrets_scan guardrail green). - create_invitation queues an InvitationChangeSet (status=Created, funding_index from the derivation path, created_at = expiry - TTL); best-effort so a persist failure never fails the already-valid link. platform-wallet 407 + platform-wallet-storage 130 tests green (incl. a new apply→read→upsert→remove round-trip); fmt + clippy --all-features clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add CreateInvitationSheet — amount entry + a "send a contact request back to me" toggle → ManagedPlatformWallet.createInvitation → share sheet + QR of the dashpay://invite link — reached from a new "Invite a friend" action in DashPayProfileView. The link embeds the one-time voucher key (a bearer credential), so it is never logged and the Copy action writes to a local-only pasteboard so it is not mirrored across devices via Universal Clipboard. Funds the asset lock from BIP44 standard account 0; expiry is derived Rust-side from the passed nowUnix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
…rapper Add platform_wallet_parse_invitation(uri, out_preview) -> InvitationPreviewFFI: a read-only decode of a dashpay://invite link (no wallet handle, no network, no claim) surfacing structurally_valid / is_instant / has_inviter / inviter_id / inviter_username / amount_duffs / expiry_unix. A malformed link is a clean invalid preview (structurally_valid=false), not an error, so the claim UI can render an "invalid link" state; the clock-relative "expired" check stays in Swift. Wraps the existing crypto parse_invitation_uri. The claim sheet needs this to show the invite (amount/sender/expiry) before claiming and to drive the post-claim "establish contact with <sender>?" prompt (claimInvitation returns only the bare identity; the invite parser is otherwise Rust-internal). Adds ManagedPlatformWallet.parseInvitation(uri:) -> InvitationPreview and 2 marshaling tests (null uri, malformed→invalid-preview). 8/8 invitation FFI tests green; framework + SwiftExampleApp build green (iPhone 17, arm64). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
ClaimInvitationSheet (reached from a new "gift" toolbar action in DashPayTabView): paste a dashpay://invite link → parseInvitation preview (amount / sender / expiry / validity) → claim. Claiming registers a NEW identity for the invitee funded by the imported voucher — ordinary identity registration (master + auth keys via prePersistIdentityKeysForRegistration, plus a DashPay enc/dec pair so the new identity can send the contact request back) — then, if the link carried an inviter, prompts "Add <sender>?" and sends a normal contact request via the shipped sendContactRequest path. The wallet is resolved from the active identity's wallet, falling back to the first loaded wallet on the network, so a fresh invitee with no identity yet can still claim. The DashPay enc/dec key derivation is factored into a shared IdentityRegistrationKeys.makeDashpayKeyPair (a mirror of CreateIdentityView.makeDashpayKeyPair — kept in sync; a follow-up should unify them) rather than duplicated inline. SwiftExampleApp xcodebuild green (iPhone 17, arm64). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
Register the dashpay URL scheme (Info.plist CFBundleURLTypes) and add .onOpenURL on the app WindowGroup: opening a dashpay://invite link routes to the DashPay tab and hands the URL to AppUIState.pendingInviteURL. DashPayTabView observes it, pre-fills the claim sheet (ClaimInvitationSheet initialURI), and clears the pending URL. The link embeds a bearer voucher key, so it is not logged in the handler. SwiftExampleApp xcodebuild green (iPhone 17, arm64). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
shumkov
requested review from
QuantumExplorer,
ZocoLini,
lklimek and
llbartekll
as code owners
Collaborator
|
🕓 Ready for review — 1 ahead in queue (commit 139a478) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #4041 +/- ##
==========================================
Coverage 87.19% 87.19%
==========================================
Files 2646 2647 +1
Lines 328582 328719 +137
==========================================
+ Hits 286503 286630 +127
- Misses 42079 42089 +10
🚀 New features to boost your workflow:
|
thepastaclaw
suggested changes
Jul 8, 2026
Collaborator
There was a problem hiding this comment.
Code Review
Source: Codex gpt-5.5 reviewers for general, security-auditor, rust-quality, and ffi-engineer; Codex gpt-5.5 verifier. Degraded: Claude Opus reviewer lanes and Claude Opus verifier failed due extra-usage quota (resets July 10, 2026 at 8:00 AM America/Chicago). Policy gate: review_policy.enforce_backport_prereq_policy applied with no backport-prereq changes.
At the target SHA, the invitation core and FFI paths are present, but the public claim surface does not expose the inviter metadata required by the PR's documented contact-bootstrap flow. I also confirmed the new raw invitation-key export gate does not enforce the fully hardened path shape it documents, and the claim path leaves an extra voucher private-key copy unwiped.
🔴 1 blocking | 🟡 2 suggestion(s)
🤖 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/invitation.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/invitation.rs:244-253: Expose inviter metadata for the contact-bootstrap flow
The invitation codec preserves optional `InviterInfo`, but `platform_wallet_claim_invitation` parses the URI and then only writes the new identity id and managed-identity handle. The Swift wrapper `claimInvitation` returns only `ManagedIdentity`, and there is no parse/preview API in this target SHA that exposes `invitation.inviter` to Swift. The PR documents that an invitee should be prompted to establish contact and then send a reciprocal contact request to the inviter identity; without exposing the inviter identity id and username, that flow is not implementable without duplicating the private Rust URI codec in Swift.
In `packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs`:
- [SUGGESTION] packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs:395-399: Enforce hardened invitation export paths
`export_invitation_private_key` documents the accepted path as `m/9'/coin'/5'/3'/funding_index'`, but the gate only checks the length plus components 0, 2, and 3. Paths such as `m/9'/1'/5'/3'/0` still pass and export a non-hardened child private key. The current create flow appears to pass a builder-generated hardened path, but this function is the advertised raw-scalar export boundary; it should reject non-hardened coin and funding-index components so the gate matches the documented one-time voucher scope.
In `packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs:268-294: Wipe the voucher PrivateKey copy in the claim path
`ParsedInvitation` wipes `voucher_key` on drop, but this block copies that secret into `dashcore::PrivateKey`. That wrapper is `Copy + Clone + Debug`, contains a public `SecretKey` field, and has no `Drop` implementation, so the claim path leaves an additional bearer-voucher scalar copy in memory after both success and error returns. This PR explicitly treats the voucher key as bearer money and scrubs the exported scalar in the create path; the claim path should give the `PrivateKey` copy the same treatment.
…ader The compile-time guard tc_p1_003_prepare_cached_in_writers scans every schema writer source for bare .prepare( calls, exempting only the read-only SELECTs enumerated in READ_ONLY_PREPARE_ALLOWED. The new test-gated invitations::read_all reader uses conn.prepare like every other per-table reader, so it needs its allow-list entry. Test would have caught this in CI: the guard was the sole workspace-test failure on this branch (11942 passed, 1 failed) - red before this entry, green after. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs (1)
268-294: 🔒 Security & Privacy | 🟠 MajorVoucher
PrivateKeycopy is not scrubbed in the claim path.This was previously flagged:
ParsedInvitationwipesvoucher_keyon drop, butPrivateKey::newat line 271 copies the scalar into aCopy + Clonewrapper with noDropimplementation. The create path scrubs the exported scalar at line 198, but the claim path leaves an additional bearer-voucher copy in memory after both success and error returns. The suggestedWipingPrivateKeywrapper from the prior review has not been applied.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs` around lines 268 - 294, The claim path in the identity invitation flow leaves a non-wiped copy of the voucher secret in memory because `PrivateKey::new` is used directly in `invitation.rs` without any scrubbing wrapper. Update the claim flow around `voucher_priv` and `put_to_platform_and_wait_for_response_with_private_key` to use the same `WipingPrivateKey` approach as the create path, or otherwise ensure the scalar is zeroized on drop after success and error. Keep the fix localized to the invitation claim handling in `ParsedInvitation`/`PrivateKey::new` usage so the voucher key is not retained after use.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/dashpay/DIP15_INVITATIONS_SPEC.md`:
- Around line 376-404: Update §6 to match the implemented invitation URI format
in crypto/invitation.rs: describe the hand-rolled little-endian binary encoding
instead of serde/bincode, correct the wire field order to version, voucher_key,
expiry_unix, inviter, asset_lock, and align the API docs with the actual
encode_invitation_uri and parse_invitation_uri signatures and return types. Use
the existing InvitationPayloadV0, encode_invitation_uri, and
parse_invitation_uri symbols as the source of truth so the spec reflects the
as-built implementation.
- Around line 125-133: Update the create_invitation API spec to match the
implementation by removing the explicit invitation_index parameter from the
create_invitation signature and documenting that create_funded_asset_lock_proof
auto-selects the next unused funding index through its builder; keep the rest of
the parameters in create_invitation aligned with the implementation and
reference the create_invitation and create_funded_asset_lock_proof symbols so
the signature and behavior stay consistent.
In `@packages/rs-platform-wallet-ffi/src/invitation.rs`:
- Around line 245-266: The claim_invitation FFI entrypoint currently allows a
zero now_unix, which can make validate_claimable treat expired invitations as
valid; add the same non-zero time check used by create_invitation before
invoking validate_claimable. Update platform_wallet_claim_invitation to reject
now_unix == 0 with an invalid-parameter result, keeping the existing pointer and
identity_pubkeys_count guards intact.
In `@packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs`:
- Around line 204-207: The silent fallback to 0 for funding_index in
invitation.rs can make voucher key recovery impossible if the derivation path
shape is unexpected. Update the logic around the path.as_ref().last() match in
the invitation funding index derivation to emit a warning when the path is empty
or ends in a non-Hardened child, while still preserving the current fallback
behavior if needed. Use the existing invitation/voucher derivation code path to
add a loud signal tied to funding_index so unexpected DIP-13 path shapes are
visible during debugging and recovery.
In `@packages/swift-sdk/SwiftExampleApp/Info.plist`:
- Around line 33-47: The DashPay invitation deep link flow currently uses the
hijackable custom scheme registered in SwiftExampleApp’s Info.plist via
CFBundleURLTypes, which can expose the voucher private key. Replace this
scheme-based claim entry point with a Universal Links setup (HTTPS link backed
by an AASA file) or another verified handoff mechanism, and update the invite
handling path that currently targets dashpay://invite so the app routes claims
through the new secure entry point.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift`:
- Around line 189-207: The current sheet presentation in DashPayTabView uses a
boolean, so a second invite URL won’t recreate ClaimInvitationSheet when it is
already open. Replace the showClaimInvitation flag with an identifiable
presentation state (for example a claim request/item) and drive the sheet with
.sheet(item:) so each pendingInviteURL change constructs a fresh
ClaimInvitationSheet with the new initial URI. Update the onChange handler for
appUIState.pendingInviteURL to assign a new request object each time, and keep
ClaimInvitationSheet’s initialization path tied to that item so its internal uri
state is seeded for every incoming invite.
---
Duplicate comments:
In `@packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs`:
- Around line 268-294: The claim path in the identity invitation flow leaves a
non-wiped copy of the voucher secret in memory because `PrivateKey::new` is used
directly in `invitation.rs` without any scrubbing wrapper. Update the claim flow
around `voucher_priv` and
`put_to_platform_and_wait_for_response_with_private_key` to use the same
`WipingPrivateKey` approach as the create path, or otherwise ensure the scalar
is zeroized on drop after success and error. Keep the fix localized to the
invitation claim handling in `ParsedInvitation`/`PrivateKey::new` usage so the
voucher key is not retained after use.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bdc56fe1-5052-483c-aa66-65ce9fd8da1c
📒 Files selected for processing (28)
docs/dashpay/DIP15_INVITATIONS_SPEC.mdpackages/rs-platform-wallet-ffi/src/dashpay.rspackages/rs-platform-wallet-ffi/src/invitation.rspackages/rs-platform-wallet-ffi/src/lib.rspackages/rs-platform-wallet-storage/migrations/V003__invitations.rspackages/rs-platform-wallet-storage/src/sqlite/persister.rspackages/rs-platform-wallet-storage/src/sqlite/schema/invitations.rspackages/rs-platform-wallet-storage/src/sqlite/schema/mod.rspackages/rs-platform-wallet-storage/tests/sqlite_compile_time.rspackages/rs-platform-wallet/src/changeset/changeset.rspackages/rs-platform-wallet/src/changeset/mod.rspackages/rs-platform-wallet/src/wallet/apply.rspackages/rs-platform-wallet/src/wallet/identity/crypto/invitation.rspackages/rs-platform-wallet/src/wallet/identity/crypto/mod.rspackages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rspackages/rs-platform-wallet/src/wallet/identity/network/invitation.rspackages/rs-platform-wallet/src/wallet/identity/network/mod.rspackages/rs-platform-wallet/src/wallet/identity/network/payments.rspackages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swiftpackages/swift-sdk/SwiftExampleApp/Info.plistpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/IdentityRegistrationKeys.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ClaimInvitationSheet.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/CreateInvitationSheet.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayProfileView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swiftpackages/swift-sdk/SwiftExampleApp/TEST_PLAN.md
Cold-launch fix (HIGH): .onOpenURL sets AppUIState.pendingInviteURL during scene connection, but on a cold launch ContentView shows "Initializing…" until bootstrap finishes, so DashPayTabView doesn't exist yet and its .onChange(of: pendingInviteURL) baselines to the already-set value and never fires — the claim sheet never appears (the common "tap invite link from Messages" path) and the bearer URL lingers in @published. Add an .onAppear consumer alongside the existing .onChange (shared consumePendingInviteURL()); the nil guard makes the second call a no-op, so no double-present. Also: gate .onOpenURL on url.host == "invite" so unrelated dashpay:// links don't yank the user to the claim sheet; add an accessibilityLabel to the icon-only "gift" claim toolbar button; mark the decorative invitation QR image accessibilityHidden. SwiftExampleApp xcodebuild green (iPhone 17, arm64). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
The voucher-key export gate bound only the fixed purpose/feature/sub-feature components (9'/*/5'/3'/*), leaving coin_type and funding_index unconstrained, so a path like m/9'/1'/5'/3'/0 (non-hardened tail) still exported a child key. Every component of a real invitation-funding path is hardened; enforcing that on the raw-scalar export boundary keeps the runtime check exactly as strict as its documented scope. Not a live exfil hole (the whole 9'/*/5'/3'/* subtree is voucher-only), but defense-in-depth at the advertised export seam. Extends the negative gate test with the two now-rejected non-hardened cases (both pass the old gate and export, fail the new one) and drops a non-timeless spec-finding tag from the test doc. Addresses review: thepastaclaw 'Enforce hardened invitation export paths' + rust-quality reviewer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
Key hygiene (voucher is bearer money — scrub every copy): - Scrub the exported voucher scalar on the encode-error path too, not just on success: on an encode failure the key never legitimately left the device, so a lingering copy is the worst kind. (security-auditor LOW-1) - Wrap the claim-path voucher PrivateKey in a WipingPrivateKey RAII guard so the imported scalar is erased on every exit; dashcore::PrivateKey has no Drop-zeroize. (thepastaclaw + CodeRabbit Major + security-auditor LOW-2) Defensive consistency: - Reject a zero now_unix in validate_claimable: otherwise now(0) > expiry is false and an expired link looks claimable. Mirrors the create-side non-zero timestamp guard; unit-tested (validate_rejects_zero_clock — Ok on the old code, Err now). (rust-quality + correctness + CodeRabbit) - Skip the local invitation record with a warning when the funding path has an unexpected non-hardened tail, instead of persisting funding_index=0 — a wrong index would re-derive the wrong voucher key on reclaim. (3 reviewers) - Publish FFI-safe sentinels for the claim out-params before any fallible work, matching the create/parse siblings. (correctness) - Document the InvitationChangeSet merge insert-vs-tombstone hazard, parity with IdentityChangeSet. (rust-quality) Also drops non-timeless spec/review tags from comments per the repo's comment-timeliness policy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
…ation - §4.1 create_invitation signature: drop the explicit invitation_index param (the asset-lock builder auto-selects the next unused funding index) and add the crypto_provider param; note the fully-hardened export path. - §6 envelope: describe the hand-rolled little-endian binary encoding (not serde/bincode), correct the wire field order (asset_lock is last, length- prefixed: version, voucher_key, expiry_unix, inviter, asset_lock), and fix the encode_invitation_uri / parse_invitation_uri signatures. - New §6.1 documenting the bearer-credential / custom-scheme limitation and the Universal Links production follow-up (loss bounded by MAX_INVITATION_DUFFS). Addresses CodeRabbit: create_invitation signature drift, §6 envelope drift, and the Info.plist custom-scheme security note. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
A second dashpay://invite link arriving while the claim sheet is already open was dropped: ClaimInvitationSheet seeds its `uri` @State once from initialURI at init, so setting showClaimInvitation=true when already true is a no-op for .sheet(isPresented:) and the new URI never reaches the presented sheet (repro: tap "gift" to open, then a link arrives → ignored). Drive the claim sheet via .sheet(item:) keyed on an Identifiable `ClaimInvite` (fresh id per invocation): assigning a new value re-presents the sheet — even one already open — re-seeded with the new URI. Also clear the bearer pendingInviteURL immediately in consumePendingInviteURL (don't linger a secret in @published), and only present when a wallet is loaded. CodeRabbit finding (DashPayTabView). SwiftExampleApp xcodebuild green (iPhone 17, arm64). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
shumkov
changed the title
…start The IdentityInvitation / IdentityRegistration / IdentityTopUp / asset-lock top-up funding accounts fund credit outputs that live only in an asset-lock special-tx payload; the on-chain output is an OP_RETURN burn, so these addresses never appear as UTXOs and SPV can never rediscover their used indices. The build path marked the funding index used only in memory, and the load path dropped these accounts' persisted pools (`_ => None`), so `funding_index` reset to 0 on every app restart. For IdentityInvitation that reused the EXPORTED one-time voucher key across invitations — one leaked link could then claim every same-key invite (asset-lock proofs are public on-chain). Fix (platform-only, reuses the existing account_address_pools persist/restore that already works for funds accounts): - build.rs: after a build marks the funding index used, snapshot the asset-lock funding accounts' address pools into the account_address_pools changeset. - persistence.rs restore routing: route asset-lock account pools back to their managed account (via ManagedAccountRefMut) instead of dropping them, so restore_address_pool rebuilds highest_used → next_unused is monotonic. Fixes all asset-lock funding accounts uniformly (registration/topup share the reset but are harmless there — their keys never leave the device). Compiles clean; existing tests green (15 asset_lock build + 146 platform-wallet-ffi). Follow-up (next commit): red->green regression test pinning index monotonicity across a persist->restore round-trip + multi-agent review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
Regression test for the voucher-key-reuse fix: a build must persist the
IdentityInvitation account's address-pool snapshot with the used funding index,
so funding_index survives a restart instead of resetting to 0 and reusing the
exported one-time voucher key.
Red->green verified: with self.persist_asset_lock_account_pools() disabled the
test FAILS ('a build must persist the IdentityInvitation account's pool...');
with it enabled it PASSES.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
Fold multi-agent review of the funding-index-persistence fix: - Security-gate the IdentityInvitation path (rust-quality H1 + security-auditor): persist_asset_lock_account_pools now returns Result, and create_funded_asset_lock_proof aborts BEFORE broadcast if the invitation's funding-index persist fails. The voucher key is exported into a bearer link, so broadcasting an invitation whose used-index wasn't durably recorded would re-open the reuse window on the next restart; failing before broadcast is harmless (no tx on the wire). Other asset-lock accounts keep their keys on-device, so they stay best-effort. - Document the concurrent-build residual (security-auditor Finding 1): the snapshot re-locks after the build lock drops, so per-wallet asset-lock builds must be serialized (the UI creates invitations one-at-a-time); self-healing otherwise. - Correct the now-inaccurate restore warn: 'no matching funds account' -> 'no matching managed account' (the routed accounts are keys-accounts too). Reviews confirmed the gap-limit-past-window case is handled (next_unused scans by highest_generated, not gap_limit) and routing covers all six asset-lock types. fmt + clippy --all-features clean; asset_lock tests green (16). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs/dashpay/DIP15_INVITATIONS_SPEC.md (4)
194-195: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the Swift persistence scope with the PR objectives.
The spec requires a
PersistentInvitationSwiftData model, an FFI listing API, and anInvitationsView, but the PR objectives state that the Swift persistence bridge and sent-invitations list remain out of scope. Mark these items as deferred or remove them from the v1 contract to avoid misleading implementation and QA work.Also applies to: 225-225, 248-250
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/dashpay/DIP15_INVITATIONS_SPEC.md` around lines 194 - 195, Update the DIP15 invitation specification to remove or explicitly defer the SwiftData PersistentInvitation model, FFI listing API, and InvitationsView sent-invitations list from the v1 contract, including the corresponding sections around the referenced requirements, so the documented scope matches the PR objectives.
238-247: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPass
establishContactthroughclaimInvitation.The FFI contract requires
establish_contact, and the UI is expected to pass the user’s answer, but the Swift wrapper signature only acceptsuriandidentityIndex. Add an explicit Boolean parameter and forward it so contact bootstrap remains opt-in.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/dashpay/DIP15_INVITATIONS_SPEC.md` around lines 238 - 247, Update claimInvitation to accept an explicit establishContact Boolean alongside uri and identityIndex, then forward it to the underlying FFI call as establish_contact. Update all callers, including the claim UI flow, to pass the user’s opt-in answer so contact establishment remains optional.
203-205: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winExpose inviter display metadata consistently through FFI.
InviterInfoand the envelope includedisplay_name, butplatform_wallet_create_invitationonly accepts the identity ID and username. Add the missing parameter or document the default/omission explicitly; otherwise the FFI path cannot produce the documented inviter payload.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/dashpay/DIP15_INVITATIONS_SPEC.md` around lines 203 - 205, The FFI signature for platform_wallet_create_invitation omits the inviter display_name required by InviterInfo and the invitation envelope. Add a nullable display-name parameter consistently across the FFI declaration, implementation, and callers, or explicitly document and implement the default omission so the generated payload matches the documented schema.
234-243: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winThread the contact-bootstrap opt-in through the Swift create API.
The UI checkbox is documented as controlling optional inviter metadata, but
createInvitation(amountDuffs:fundingAccount:expiry:)has no opt-in or inviter argument. The Swift wrapper therefore cannot forward the user’s choice to the FFI/Rust layers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/dashpay/DIP15_INVITATIONS_SPEC.md` around lines 234 - 243, The Swift create-invitation API must accept and propagate the contact-bootstrap opt-in. Update ManagedPlatformWallet.createInvitation and its Controller/Coordinator flow to accept an optional inviter/contact-request setting, then forward it through the Swift FFI bindings to the Rust invitation creation layer, allowing the DashPay UI checkbox to control whether inviter metadata is included.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/dashpay/DIP15_INVITATIONS_SPEC.md`:
- Around line 381-391: Specify the wire-format code block as plain text by
changing its opening fence to ```text in the relevant DIP15 invitation
specification section, while leaving the block contents unchanged.
---
Outside diff comments:
In `@docs/dashpay/DIP15_INVITATIONS_SPEC.md`:
- Around line 194-195: Update the DIP15 invitation specification to remove or
explicitly defer the SwiftData PersistentInvitation model, FFI listing API, and
InvitationsView sent-invitations list from the v1 contract, including the
corresponding sections around the referenced requirements, so the documented
scope matches the PR objectives.
- Around line 238-247: Update claimInvitation to accept an explicit
establishContact Boolean alongside uri and identityIndex, then forward it to the
underlying FFI call as establish_contact. Update all callers, including the
claim UI flow, to pass the user’s opt-in answer so contact establishment remains
optional.
- Around line 203-205: The FFI signature for platform_wallet_create_invitation
omits the inviter display_name required by InviterInfo and the invitation
envelope. Add a nullable display-name parameter consistently across the FFI
declaration, implementation, and callers, or explicitly document and implement
the default omission so the generated payload matches the documented schema.
- Around line 234-243: The Swift create-invitation API must accept and propagate
the contact-bootstrap opt-in. Update ManagedPlatformWallet.createInvitation and
its Controller/Coordinator flow to accept an optional inviter/contact-request
setting, then forward it through the Swift FFI bindings to the Rust invitation
creation layer, allowing the DashPay UI checkbox to control whether inviter
metadata is included.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a105f30-7c36-46a8-aaf9-3de6c80c4687
📒 Files selected for processing (11)
docs/dashpay/DIP15_INVITATIONS_SPEC.mdpackages/rs-platform-wallet-ffi/src/invitation.rspackages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet/src/changeset/changeset.rspackages/rs-platform-wallet/src/wallet/asset_lock/build.rspackages/rs-platform-wallet/src/wallet/identity/crypto/invitation.rspackages/rs-platform-wallet/src/wallet/identity/network/invitation.rspackages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rspackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/CreateInvitationSheet.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swift
- packages/rs-platform-wallet/src/changeset/changeset.rs
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift
- packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs
- packages/rs-platform-wallet/src/wallet/identity/crypto/invitation.rs
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/CreateInvitationSheet.swift
- packages/rs-platform-wallet-ffi/src/invitation.rs
- packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs
Comment on lines
+381
to
+391
| ``` | ||
| wire = version:u8 // = 0 | ||
| ‖ voucher_key:[u8; 32] // one-time ECDSA private key (secret; zeroized) | ||
| ‖ expiry_unix:u32(LE) // ADVISORY, IS-scoped (§5.1); not consensus | ||
| ‖ inviter_present:u8 // 0 = none, 1 = InviterInfo follows | ||
| [ identity_id:[u8; 32] | ||
| ‖ username:len-prefixed // DPNS name (whom the invitee's contactRequest targets) | ||
| ‖ display_present:u8 [ display_name:len-prefixed ] ] | ||
| ‖ asset_lock:len-prefixed // InstantSend proof (§5.1) — embeds tx + islock; LAST, length-prefixed | ||
| // NO auto-accept dapk — v1 sends a normal contactRequest, invitee-confirmed (§2) | ||
| ``` |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specify a language for the wire-format code block.
Add a text or equivalent fence language to satisfy the repository’s Markdown lint configuration.
Suggested fix
-```
+```text
wire = version:u8📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ``` | |
| wire = version:u8 // = 0 | |
| ‖ voucher_key:[u8; 32] // one-time ECDSA private key (secret; zeroized) | |
| ‖ expiry_unix:u32(LE) // ADVISORY, IS-scoped (§5.1); not consensus | |
| ‖ inviter_present:u8 // 0 = none, 1 = InviterInfo follows | |
| [ identity_id:[u8; 32] | |
| ‖ username:len-prefixed // DPNS name (whom the invitee's contactRequest targets) | |
| ‖ display_present:u8 [ display_name:len-prefixed ] ] | |
| ‖ asset_lock:len-prefixed // InstantSend proof (§5.1) — embeds tx + islock; LAST, length-prefixed | |
| // NO auto-accept dapk — v1 sends a normal contactRequest, invitee-confirmed (§2) | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 381-381: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/dashpay/DIP15_INVITATIONS_SPEC.md` around lines 381 - 391, Specify the
wire-format code block as plain text by changing its opening fence to ```text in
the relevant DIP15 invitation specification section, while leaving the block
contents unchanged.
Source: Linters/SAST tools
…tion reclaim) Adds platform_wallet_topup_identity_with_existing_asset_lock_signer — tops up an EXISTING identity by consuming an already-tracked asset lock (IdentityTopUp via AssetLockFunding::FromExistingAssetLock), the net-new primitive for the inviter's 'reclaim into my existing identity' path. Mirrors the existing resume-register FFI; register-into-a-NEW-identity reclaim reuses that resume FFI unchanged. Design note: the adversarial reclaim review recommended an IMPORT-based reclaim because the resume/FromExistingAssetLock path looked the credit address up in the funding pool, which reset past funding_index>=5 on restart. The voucher-key-reuse fix (this branch) now persists+restores that pool, so the resume path works across restart again — and it is simpler than import-based (it re-derives the voucher key + acquires the proof internally via the inviter's own signer, no export/import). So reclaim is resume-based: register reuses the resume FFI, topup uses this new sister FFI. Reclaim recovers value as Platform credits (the L1 DASH is an OP_RETURN burn). Compiles clean (cargo check platform-wallet-ffi --all-features); fmt clean. Follow-up: marshaling unit test (mirrors the proven resume FFI) + Swift reclaim UI (ffi-swift) + DP-17/18/19 testnet. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
…tations bridge) Rust half of the 'Sent invitations' persistence bridge. Adds the FFI seam that forwards InvitationChangeSet out of FFIPersister to the host so the app can show sent invitations (the create/reclaim flows already emit the changeset; FFIPersister was silently dropping the invitations sub-field). - invitation_persistence.rs: InvitationEntryFFI (all-POD #[repr(C)]: out_point, funding_index, amount_duffs, expiry_unix, created_at_secs, has_inviter, status) + build_invitation_entries (no owned buffers / no storage Vec, unlike the asset-lock template) + wildcard-free status_to_u8. Unit tests pin the field round-trip + the 0/1/2 status discriminants. - persistence.rs: on_persist_invitations_fn appended at the END of PersistenceCallbacks (stable layout) + Default None; fired in FFIPersister::store next to the asset-lock block with the same non-empty guard + null-when-empty pointers. A non-zero return flips the round to rollback (round-global). Swift half (PersistentInvitation @model + shim + InvitationsView) is ffi-swift's lane, gated on a build_ios.sh header regen. Compiles clean; fmt + clippy clean; 2 unit tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
…ard) Adds the crate-standard compile-time layout assertion (const _: [u8; 64] = [0u8; size_of::<InvitationEntryFFI>()]) that every other *EntryFFI persistence struct carries. Enforces the reorder/padding invariant the struct's doc already warns about, so a future field change that shifts the layout is a compile error instead of a silent ABI desync against the Swift-imported struct. Verified: cargo check green (size == 64). Folds the sole should-fix from the rust-quality review of the invitation bridge + reclaim-topup FFIs (both otherwise ship-able, no correctness defects). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
The design spec for the 'Sent invitations' persistence bridge (FFI→SwiftData) and voucher reclaim, produced via research → 3-agent review → owner sync. Covers the InvitationEntryFFI push-callback bridge (§3-§5), the resume-based reclaim (§8: FromExistingAssetLock topup/register, recovers value as credits), and the failure-mode table (T1 outpoint-key seam, T3 round-global return, threading). Includes the rawOutPoint companion column agreed for commit 1 so commit-2 reclaim reads the raw outpoint without a decode/migration. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
Surface each created invitation into SwiftData + a "Sent invitations" list. Mirrors the asset_locks push-callback path, simpler (InvitationEntry is all-POD). - PersistentInvitation @model (outPointHex @unique T1 key + rawOutPoint:Data 36B companion for reclaim, walletId #Index, fundingIndexRaw/amountDuffs/expiryUnix/ createdAtSecs/hasInviter/statusRaw) + registered in DashModelContainer.modelTypes - InvitationEntrySnapshot + persistInvitations handler + persistInvitationsCallback shim (deep-copy POD; encodeOutPoint key on BOTH the upsert and removal paths — the T1 seam; always-return-0 since the callback return is round-global; inline-on-onQueue, no save(); walletId set on both insert+update branches) + cb.on_persist_invitations_fn wired in makeCallbacks() - InvitationsView (@query walletId, sort createdAtSecs desc, amount + status badge with an .unknown fallback, expiry) + a "Sent invitations" nav link in DashPayTabView - InvitationPersistenceTests: create → status-change (upsert-in-place, no dup) → removal (row deleted) — pins the T1 upsert-key ↔ removal-key seam before reclaim ships its removal path - TEST_PLAN DP-16 No secret column (voucher re-derived from funding_index); no Rust→Swift rehydrate (SwiftData is the UI source). build_ios.sh header regen + SwiftExampleApp xcodebuild green (iPhone 17, arm64). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/dashpay/INVITATIONS_PERSISTENCE_SWIFT_SPEC.md`:
- Around line 189-191: Update the T1 seam-test statement in the invitation
persistence specification to match the resolved scope: either mark the
create→reclaim/removal→row-deleted assertion as required for this release, or
explicitly move reclaim-specific coverage into a separate follow-up section;
remove the stale “v1 non-goal” wording.
- Around line 96-101: The invitation persistence path must not silently lose
SwiftData failures. In the FFI callback that invokes handler.persistInvitations,
replace bare try? handling with durable error telemetry and a recovery or
retry/reconciliation trigger before considering the round handled; preserve the
required round-global return semantics while ensuring failed invitations can be
retried or re-emitted.
- Around line 264-280: The reclaim failure handling must distinguish a prior
successful reclaim from an invitee claim; update the failure-modes guidance for
the Swift reclaim flow to persist a pending-reclaim marker before the external
consume call, then reconcile AlreadyConsumed using that marker and clear or
finalize it appropriately. Only mark PersistentInvitation.statusRaw as Claimed
when the outcome is attributable to another party, while retries after a crash
following successful reclaim must become Reclaimed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1091fbcd-4f44-450b-b963-16b5f6841832
📒 Files selected for processing (12)
docs/dashpay/INVITATIONS_PERSISTENCE_SWIFT_SPEC.mdpackages/rs-platform-wallet-ffi/src/identity_registration_funded_with_signer.rspackages/rs-platform-wallet-ffi/src/invitation_persistence.rspackages/rs-platform-wallet-ffi/src/lib.rspackages/rs-platform-wallet-ffi/src/persistence.rspackages/swift-sdk/Sources/SwiftDashSDK/Persistence/DashModelContainer.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentInvitation.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/InvitationsView.swiftpackages/swift-sdk/SwiftExampleApp/TEST_PLAN.mdpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/InvitationPersistenceTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift
Comment on lines
+96
to
+101
| `Unmanaged.fromOpaque(context).takeUnretainedValue()`, **deep-copy every FFI row into owned | ||
| Swift values before returning** (Rust frees the buffers on return), build | ||
| `[InvitationEntrySnapshot]` + `[Data]` removals, call `handler.persistInvitations(...)`. | ||
| **Consume the cbindgen-regenerated `InvitationEntryFFI`** from the header — never a hand-written | ||
| Swift mirror. Mirror the template's error discipline: wrap in `try?`, **always return 0** (see | ||
| §4 T3 — the return is round-global; a failed invitation write must NOT abort the whole round). |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not silently discard invitation persistence failures.
try? plus an unconditional 0 return makes store() succeed even when SwiftData persistence fails. Because the design is push-only with no Rust→Swift rehydrate path, that invitation can disappear from the UI permanently until another changeset happens to re-emit it. Add a retry/reconciliation mechanism or at least durable error telemetry and a recovery trigger before treating the round as successful.
Also applies to: 163-168
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/dashpay/INVITATIONS_PERSISTENCE_SWIFT_SPEC.md` around lines 96 - 101,
The invitation persistence path must not silently lose SwiftData failures. In
the FFI callback that invokes handler.persistInvitations, replace bare try?
handling with durable error telemetry and a recovery or retry/reconciliation
trigger before considering the round handled; preserve the required round-global
return semantics while ensuring failed invitations can be retried or re-emitted.
Comment on lines
+189
to
+191
| - **T1 seam test:** even though reclaim is a v1 non-goal, add a create→(simulated | ||
| reclaim/removal)→row-deleted assertion so the upsert-key ↔ removal-key form is exercised before | ||
| reclaim ships. Otherwise the seam ships untested and bites when reclaim lands. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the stale reclaim test-scope statement.
Lines [208-218] resolve reclaim as in-scope for this PR, but this section still calls reclaim a “v1 non-goal.” Mark the create→reclaim→row-deleted test as required for this release, or move reclaim-specific testing to a clearly separate follow-up.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/dashpay/INVITATIONS_PERSISTENCE_SWIFT_SPEC.md` around lines 189 - 191,
Update the T1 seam-test statement in the invitation persistence specification to
match the resolved scope: either mark the create→reclaim/removal→row-deleted
assertion as required for this release, or explicitly move reclaim-specific
coverage into a separate follow-up section; remove the stale “v1 non-goal”
wording.
Comment on lines
+264
to
+280
| - **Swift:** a "Reclaim" action on each `Created` sent-invitations row → a small sheet: **"Recover | ||
| this invitation's value as credits"** with the target choice (pick an existing identity to top | ||
| up, or "register a new identity"). On success, **the Swift side flips its own | ||
| `PersistentInvitation.statusRaw` to `Reclaimed`** (SwiftData is the UI source — no Rust re-emit | ||
| needed; the display-half persistence bridge is only for the create-time push). Copy is explicit: | ||
| "recovered as identity credits", never "DASH returned". | ||
|
|
||
| ### 8.4 Failure modes (reclaim-specific) | ||
|
|
||
| | Risk | Mitigation | | ||
| |---|---| | ||
| | **Consume race** — invitee claims the same voucher at the same moment | No L1 double-spend (no shared UTXO). Platform records consumed outpoints and deterministically rejects the second consume with `IdentityAssetLockTransactionOutPointAlreadyConsumed` (`verify_is_not_spent/v0/mod.rs:37-55`). Loser wastes a small ST fee, no funds lost. On that error the Swift side sets the row to `Claimed` (someone claimed it) and shows a benign "already claimed by your friend" message. | | ||
| | **User expects L1 DASH back** | UI copy says "recover as identity credits"; the reclaim sheet states the value returns as credits, not spendable DASH. | | ||
| | **Reclaim after app restart** (the common case — inviter reclaims days later) | Works: `FromExistingAssetLock` resumes the tracked lock; if the in-memory IS proof was lost on restart it falls back to SPV re-derivation (slower, still correct). **Verify** the SQLite `asset_locks` load re-attaches the proof (spike open-item #1) — perf only, not correctness. | | ||
| | **No expiry gate** | Reclaim is allowed anytime (protocol has no timelock; expiry is advisory). Product choice whether to nudge "wait until expiry"; default: allow immediately, since an invitee can claim past expiry anyway. | | ||
| | **Partial consumption remainder** | Invitation amounts are small and a single consume takes the whole value; verify the topup consumes the full voucher (no stranded remainder) — spike open-item #3. | | ||
| | **Status lifecycle now has a real emitter** | `Reclaimed`/`Claimed` are written by the Swift UI on the local row (not through the Rust changeset), so the `InvitationChangeSet::merge` insert-vs-tombstone hazard (§4-adjacent) stays latent — create is still the only Rust emitter. | |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not classify every AlreadyConsumed result as Claimed.
If reclaim succeeds and the app crashes before updating PersistentInvitation.statusRaw, a retry returns AlreadyConsumed; the current mitigation then marks the row Claimed, misreporting an invitation that this inviter reclaimed. Persist a pending reclaim marker before the external call and reconcile AlreadyConsumed outcomes, or otherwise distinguish “claimed by another party” from “previous reclaim succeeded.”
🧰 Tools
🪛 LanguageTool
[grammar] ~279-~279: Ensure spelling is correct
Context: ...nsume takes the whole value; verify the topup consumes the full voucher (no stranded ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/dashpay/INVITATIONS_PERSISTENCE_SWIFT_SPEC.md` around lines 264 - 280,
The reclaim failure handling must distinguish a prior successful reclaim from an
invitee claim; update the failure-modes guidance for the Swift reclaim flow to
persist a pending-reclaim marker before the external consume call, then
reconcile AlreadyConsumed using that marker and clear or finalize it
appropriately. Only mark PersistentInvitation.statusRaw as Claimed when the
outcome is attributable to another party, while retries after a crash following
successful reclaim must become Reclaimed.
The funded DP-16 sim run caught a regression: my earlier defense-in-depth fold (0b51001, from thepastaclaw's suggestion) required coin_type (comps[1]) and funding_index (comps[4]) to be Hardened in export_invitation_private_key — but real invitation funding paths use a NON-hardened coin_type, so every create now failed with 'path is not an invitation-funding path' (BIP-32 derivation failed). Revert to binding only the fixed 9'/*/5'/3'/* shape (purpose/feature/sub-feature) — the original gate that the DP-14 funded e2e proved works and that the security-auditor review explicitly blessed ('coin and funding_index are intentionally unconstrained and safe: the whole 9'/*/5'/3'/* subtree is invitation-vouchers-only'). Removes the two non-hardened negative-test cases that pinned the wrong (too-strict) behavior. Test would have caught this in CI only with a funded integration run; the unit-gate test passes either way. Invitation create was fully broken before this. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
…ing tail create_invitation extracted the funding index only when the derivation path's tail was Hardened, but the IdentityInvitation funding address is drawn from the account's address pool, so the tail is a NORMAL (non-hardened) index. Every real invitation therefore hit the skip branch: a valid dashpay:// link was returned but no InvitationEntry was persisted, so the "Sent invitations" list stayed empty and reclaim had no local record. Same assumed-hardened bug class as the voucher-export key gate. Accept the index from either the Hardened or Normal tail variant (256-bit and empty tails still skip — never a u32 funding index, and never produced for an address-pool path). Extract the decision into a pure funding_index_from_path helper so it is unit-testable. The stored index is display metadata only; reclaim resumes by outpoint (FromExistingAssetLock), so a missing/wrong index never affects key recovery. Regression: funding_index_extracted_from_non_hardened_tail would have caught this in CI — None before the fix, Some(42) after. Found by the funded on-device DP-16 run (0 ZPERSISTENTINVITATION rows before, 1 after). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
PersistentInvitation was registered in DashModelContainer.modelTypes but had no Storage Explorer coverage, so check-storage-explorer.sh failed CI. Add a top-level modelRow + per-wallet count, a walletId-scoped list view (mirroring AssetLockStorageListView, since invitations are scoped by wallet not network), and a read-only detail view dumping every persisted column. No secret column exists to show — the one-time voucher key is never stored. The coverage gate itself is the red->green proof: check-storage-explorer.sh was RED (PersistentInvitation missing from all three explorer files) and is now GREEN (32/32 model types covered). No unit test — these are read-only SwiftUI dump views with no business logic. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs (1)
370-411: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDoc comment path notation inconsistent with the relaxed gate.
The header doc at line 371 shows
m/9'/coin_type'/5'/3'/funding_index'(primes implying hardened), but the code now deliberately accepts non-hardenedcoin_typeandfunding_index. The inline comment at lines 395-401 explains the relaxation, but the header is misleading for anyone reading the contract without scrolling to the inline rationale.📝 Proposed doc fix
/// Export the raw DIP-13 invitation-funding private scalar at `path` - /// (`m/9'/coin_type'/5'/3'/funding_index'`) — the second deliberate raw-key + /// (`m/9'/coin_type/5'/3'/funding_index`) — the second deliberate raw-key /// export from this signer. An invitation hands the one-time voucher key to /// the invitee so they can register their own identity from the funded asset /// lock, so this key must leave the signer (a scoped, documented bearer /// credential like the auto-accept `dapk`). /// /// Gated to the **exact** invitation sub-feature: `path` MUST have 5 /// components with `9'` purpose, `5'` identity feature, and `3'` invitation - /// sub-feature. This is deliberately stricter than checking the feature - /// alone — feature `5'` is shared with identity authentication (`5'/0'`), - /// registration funding (`5'/1'`), and top-up (`5'/2'`), so a looser gate - /// could be repurposed to exfiltrate the user's own identity keys. Returns - /// the 32-byte scalar `Zeroizing`-wrapped. + /// sub-feature. `coin_type` (comps[1]) and `funding_index` (comps[4]) are + /// intentionally unconstrained (hardened or non-hardened) — the whole + /// `9'/*/5'/3'/*` subtree is vouchers-only, and the user's own keys live + /// at `5'/0'`, `5'/1'`, `5'/2'` (all excluded by `comps[3] == 3'`). + /// Returns the 32-byte scalar `Zeroizing`-wrapped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs` around lines 370 - 411, Update the doc comment for export_invitation_private_key to use path notation that does not imply coin_type and funding_index are hardened, matching the relaxed validation in the method. Preserve the documented fixed hardened components (9', 5', and 3') and clarify that coin_type and funding_index may use either hardening form.
🧹 Nitpick comments (1)
packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs (1)
764-805: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a non-hardened acceptance test case.
The test's valid path
m/9'/1'/5'/3'/0'is fully hardened, so it doesn't exercise the relaxation this PR introduces. A non-hardened path likem/9'/1/5'/3'/0would fail under the old strict gate but should now succeed — adding it as a second positive case would pin the intended behavior.✅ Suggested additional positive case
// A well-formed invitation-funding path exports its 32-byte scalar. let invitation = DerivationPath::from_str("m/9'/1'/5'/3'/0'").expect("valid path"); let scalar = signer .export_invitation_private_key(&invitation) .expect("a well-formed invitation path exports its scalar"); assert_ne!(*scalar, [0u8; 32], "exported scalar must be non-zero"); + + // A non-hardened coin_type / funding_index path is also accepted — + // the gate binds only the fixed purpose / feature / sub-feature. + let non_hardened = DerivationPath::from_str("m/9'/1/5'/3'/0").expect("valid path"); + let scalar2 = signer + .export_invitation_private_key(&non_hardened) + .expect("a non-hardened invitation path exports its scalar"); + assert_ne!(*scalar2, [0u8; 32], "exported scalar must be non-zero");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs` around lines 764 - 805, Add a second positive case in export_invitation_private_key_gates_to_the_invitation_path using a mixed-hardenedness invitation path such as m/9'/1/5'/3'/0, and assert that export_invitation_private_key succeeds and returns a non-zero scalar. Keep the existing fully hardened case and rejection cases unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs`:
- Around line 370-411: Update the doc comment for export_invitation_private_key
to use path notation that does not imply coin_type and funding_index are
hardened, matching the relaxed validation in the method. Preserve the documented
fixed hardened components (9', 5', and 3') and clarify that coin_type and
funding_index may use either hardening form.
---
Nitpick comments:
In `@packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs`:
- Around line 764-805: Add a second positive case in
export_invitation_private_key_gates_to_the_invitation_path using a
mixed-hardenedness invitation path such as m/9'/1/5'/3'/0, and assert that
export_invitation_private_key succeeds and returns a non-zero scalar. Keep the
existing fully hardened case and rejection cases unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 884ad22e-f9d7-4e70-87a0-6fe3780fe28d
📒 Files selected for processing (5)
packages/rs-platform-wallet/src/wallet/identity/network/invitation.rspackages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rspackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageExplorerView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageModelListViews.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs
`pub use invitation_persistence::*;` was inserted after `asset_lock_persistence` rather than its sorted position next to `pub use invitation::*;`, so `cargo fmt --check --all` failed under rustfmt 1.8.0. It only surfaced intermittently because self-hosted CI runners carry different rustfmt versions — the reorder-enforcing one flagged it, the other did not. No behavior change (re-export order is cosmetic), so no test is added; `cargo fmt --check --all` in CI is the guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
Collaborator
There was a problem hiding this comment.
Code Review
Carried-Forward Prior Findings
Three prior findings remain valid at HEAD 7b371fe and are carried forward: (a) platform_wallet_parse_invitation returns has_inviter=true with a null inviter_username for interior-NUL usernames, violating its own documented ABI invariant (invitation.rs:407-417, all four Claude lenses converge, benign nitpick); (b) untrusted dashpay://invite inviter metadata drives display and the reciprocal contact request with no DPNS name↔id binding (ClaimInvitationSheet.swift:217-224, low-severity social-engineering suggestion); (c) the claim reads trimmedURI inside the Task while the field stays editable — verified not exploitable (synchronous read before the first await), retained as a coherence nitpick. Prior-1 (expose inviter metadata) and prior-3 (wipe voucher PrivateKey) are FIXED; prior-2 (hardened export gate) is INTENTIONALLY DEFERRED (deliberately reverted in 0b12d2d because real voucher funding tails are non-hardened, and comps[3]==3' already isolates vouchers from user keys).
New Findings In Latest Delta
The 9a6188c..7b371fe delta (export-gate revert + Sent-invitations Swift/FFI persistence + funding_index_from_path) introduced no new material defects. One pre-existing coupling smell surfaced under cumulative scope: the persisted invitation record back-computes created_at_secs from expiry_unix (network/invitation.rs:247), which is only correct because the FFI clamps expiry to now + TTL; a non-FFI caller with a custom expiry would persist a garbage timestamp. All items are suggestions/nitpicks — no blockers.
🟡 2 suggestion(s) | 💬 2 nitpick(s)
🤖 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/ClaimInvitationSheet.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ClaimInvitationSheet.swift:217-224: Carried-forward: verify inviter metadata resolves to the embedded id before contact bootstrap
The `dashpay://invite` link is untrusted bearer data. `preview.inviterUsername` and `preview.inviterId` are copied verbatim from it (rs-platform-wallet-ffi/src/invitation.rs:407-417), rendered as the sender (`From <name>`, line 151-153), and used as the contact-request recipient (`prompt.inviterId`, sendContact line 242-246) with no check that the advertised DPNS username actually resolves to that identity id. A malicious inviter can fund a valid voucher that displays a trusted user's name while embedding the attacker's identity id; after claiming, the invitee taps Add and unknowingly establishes a reciprocal contact with the attacker. Impact is a social-engineering / contact-misdirection vector — opt-in and non-fatal (the request is re-sendable, no direct fund loss), which is why this is a suggestion, not a blocker. Resolve the DPNS name for `inviterId` on Platform and require it to match the advertised username before presenting it as the sender or sending the bootstrap request; otherwise show the raw identity id and label the name as unverified link-supplied metadata. Per packages/swift-sdk/CLAUDE.md this binding belongs in the platform-wallet claim/preview layer so every UI inherits it, not in the Swift view.
In `packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs:247: created_at_secs is back-computed from expiry, coupling the persisted record to a caller assumption the library doesn't enforce
The persisted invitation record derives its creation time as `created_at_secs: expiry_unix.saturating_sub(MAX_INVITATION_TTL_SECS)`. This is only correct when `expiry_unix == now + MAX_INVITATION_TTL_SECS` — which the FFI enforces (rs-platform-wallet-ffi/src/invitation.rs:116) but `create_invitation` does NOT: it takes an arbitrary `expiry_unix: u32` and its own doc (line 154-155) says the caller is responsible for clamping. Any non-FFI caller (a future direct library consumer or a test) passing a custom expiry persists a garbage `created_at` (a short expiry yields `created_at=0` via saturating_sub, i.e. a 1970 timestamp). The value is display-only metadata for the Sent-invitations list, so this is not a correctness hazard in the shipped FFI path, but reverse-deriving a value the FFI already holds (`now_unix`) from an unrelated field is fragile — it silently breaks if the TTL derivation changes. Thread an explicit `now_unix`/`created_at_secs` into `create_invitation` rather than reconstructing it from expiry.
Prior Findings Reconciliation
- expose-inviter-metadata-contact-bootstrap: FIXED — current FFI exposes inviter metadata and Swift consumes it; the resolved thread reply is corroborated by HEAD.
- enforce-hardened-invitation-export-paths: INTENTIONALLY DEFERRED — the strict gate was deliberately reverted because real voucher funding tails are non-hardened; the invitation subfeature gate still isolates voucher paths.
- wipe-voucher-privatekey-copy: FIXED — WipingPrivateKey erases the claim-path copy on every exit; the resolved thread reply is corroborated by HEAD.
- claim-same-uri-previewed: STILL VALID — carried forward as a non-exploitable coherence nitpick.
- verify-inviter-metadata-before-bootstrap: STILL VALID — carried forward as a contact-misdirection suggestion.
- preview-inviter-fields-nul-names: STILL VALID — carried forward as a benign FFI-contract nitpick.
Source: reviewers — general: opus (ok), gpt-5.6-sol (failed); security-auditor: opus (ok), gpt-5.6-sol (failed); rust-quality: opus (ok), gpt-5.6-sol (failed); ffi-engineer: opus (ok), gpt-5.6-sol (failed). Verifier: opus (ok).
Comment on lines
+217
to
+224
| if preview.hasInviter, | ||
| let inviterId = preview.inviterId, | ||
| let username = preview.inviterUsername { | ||
| contactPrompt = ContactPrompt( | ||
| newIdentityId: newIdentityId, | ||
| inviterId: inviterId, | ||
| username: username | ||
| ) |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Carried-forward: verify inviter metadata resolves to the embedded id before contact bootstrap
The dashpay://invite link is untrusted bearer data. preview.inviterUsername and preview.inviterId are copied verbatim from it (rs-platform-wallet-ffi/src/invitation.rs:407-417), rendered as the sender (From <name>, line 151-153), and used as the contact-request recipient (prompt.inviterId, sendContact line 242-246) with no check that the advertised DPNS username actually resolves to that identity id. A malicious inviter can fund a valid voucher that displays a trusted user's name while embedding the attacker's identity id; after claiming, the invitee taps Add and unknowingly establishes a reciprocal contact with the attacker. Impact is a social-engineering / contact-misdirection vector — opt-in and non-fatal (the request is re-sendable, no direct fund loss), which is why this is a suggestion, not a blocker. Resolve the DPNS name for inviterId on Platform and require it to match the advertised username before presenting it as the sender or sending the bootstrap request; otherwise show the raw identity id and label the name as unverified link-supplied metadata. Per packages/swift-sdk/CLAUDE.md this binding belongs in the platform-wallet claim/preview layer so every UI inherits it, not in the Swift view.
source: ['claude']
Collaborator
There was a problem hiding this comment.
Still valid at commit 4e3a9b06 — the latest push only reorders a re-export and does not add inviter name↔identity verification. Carried forward into the current cumulative review.
Comment on lines
+407
to
+417
| let (has_inviter, inviter_id, inviter_username) = match parsed.inviter.as_ref() { | ||
| Some(info) => { | ||
| // An interior NUL can't occur in a decoded UTF-8 DPNS label, but fall | ||
| // back to a null username rather than fail the whole preview. | ||
| let username = std::ffi::CString::new(info.username.clone()) | ||
| .map(|c| c.into_raw()) | ||
| .unwrap_or(std::ptr::null_mut()); | ||
| (true, info.identity_id, username) | ||
| } | ||
| None => (false, [0u8; 32], std::ptr::null_mut()), | ||
| }; |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: Carried-forward: preview violates its own has_inviter/username invariant on interior-NUL usernames
InvitationPreviewFFI documents inviter_username as null only when has_inviter is false (lines 335-339), but this block sets has_inviter=true and returns a null inviter_username when CString::new(info.username) fails. The decoder's Reader::string (packages/rs-platform-wallet/src/wallet/identity/crypto/invitation.rs) validates only UTF-8, and 0x00 is valid UTF-8 in a length-prefixed field, so a hostile bearer link can reach this branch — the inline comment's claim that an interior NUL 'can't occur' is not enforced. This is not a soundness or memory-safety issue: the shipped Swift consumer guards on if p.hasInviter, let name = p.inviterUsername (ClaimInvitationSheet.swift:151,217-219) and safely drops the contact prompt. The only effect is that a maliciously-crafted NUL username silently loses its contact-bootstrap prompt for an otherwise structurally-valid link. Dropping the inviter to has_inviter=false on this failure keeps the FFI boundary from emitting a state its own ABI docs forbid.
Suggested change
| let (has_inviter, inviter_id, inviter_username) = match parsed.inviter.as_ref() { | |
| Some(info) => { | |
| // An interior NUL can't occur in a decoded UTF-8 DPNS label, but fall | |
| // back to a null username rather than fail the whole preview. | |
| let username = std::ffi::CString::new(info.username.clone()) | |
| .map(|c| c.into_raw()) | |
| .unwrap_or(std::ptr::null_mut()); | |
| (true, info.identity_id, username) | |
| } | |
| None => (false, [0u8; 32], std::ptr::null_mut()), | |
| }; | |
| let (has_inviter, inviter_id, inviter_username) = match parsed.inviter.as_ref() { | |
| Some(info) => { | |
| // A crafted link can embed an interior NUL (valid UTF-8, length- | |
| // prefixed) in the username. CString::new then fails; rather than | |
| // emit has_inviter=true with a null username (breaking the ABI | |
| // contract that username is null only when has_inviter is false), | |
| // drop the inviter metadata so the two fields stay consistent. | |
| match std::ffi::CString::new(info.username.clone()) { | |
| Ok(c) => (true, info.identity_id, c.into_raw()), | |
| Err(_) => (false, [0u8; 32], std::ptr::null_mut()), | |
| } | |
| } | |
| None => (false, [0u8; 32], std::ptr::null_mut()), | |
| }; |
source: ['claude']
Collaborator
There was a problem hiding this comment.
Still valid at commit 4e3a9b06 — the latest push only reorders a re-export and does not change interior-NUL handling in the invitation preview FFI. Carried forward into the current cumulative review.
| funding_index, | ||
| amount_duffs, | ||
| expiry_unix, | ||
| created_at_secs: expiry_unix.saturating_sub(MAX_INVITATION_TTL_SECS), |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: created_at_secs is back-computed from expiry, coupling the persisted record to a caller assumption the library doesn't enforce
The persisted invitation record derives its creation time as created_at_secs: expiry_unix.saturating_sub(MAX_INVITATION_TTL_SECS). This is only correct when expiry_unix == now + MAX_INVITATION_TTL_SECS — which the FFI enforces (rs-platform-wallet-ffi/src/invitation.rs:116) but create_invitation does NOT: it takes an arbitrary expiry_unix: u32 and its own doc (line 154-155) says the caller is responsible for clamping. Any non-FFI caller (a future direct library consumer or a test) passing a custom expiry persists a garbage created_at (a short expiry yields created_at=0 via saturating_sub, i.e. a 1970 timestamp). The value is display-only metadata for the Sent-invitations list, so this is not a correctness hazard in the shipped FFI path, but reverse-deriving a value the FFI already holds (now_unix) from an unrelated field is fragile — it silently breaks if the TTL derivation changes. Thread an explicit now_unix/created_at_secs into create_invitation rather than reconstructing it from expiry.
source: ['claude']
Comment on lines
+175
to
+211
| private func claim() { | ||
| guard canClaim, !isClaiming, let preview else { return } | ||
| isClaiming = true | ||
| errorMessage = nil | ||
| Task { @MainActor in | ||
| defer { isClaiming = false } | ||
| do { | ||
| guard let wallet = walletManager.wallet(for: walletId) else { | ||
| errorMessage = "No wallet loaded." | ||
| return | ||
| } | ||
| let signer = KeychainSigner(modelContainer: modelContext.container) | ||
| let identityIndex = nextUnusedIdentityIndex() | ||
|
|
||
| // Register the invitee's own keys exactly like a normal | ||
| // registration: master + auth keys, plus a DashPay enc/dec pair | ||
| // so the new identity can send the contact request back. | ||
| var keys = try wallet.prePersistIdentityKeysForRegistration( | ||
| identityIndex: identityIndex, | ||
| keyCount: Self.authKeyCount, | ||
| network: network | ||
| ) | ||
| keys.append(contentsOf: try IdentityRegistrationKeys.makeDashpayKeyPair( | ||
| managedWallet: wallet, | ||
| walletId: walletId, | ||
| identityIndex: identityIndex, | ||
| firstKeyId: Self.authKeyCount, | ||
| network: network | ||
| )) | ||
|
|
||
| let managed = try await wallet.claimInvitation( | ||
| uri: trimmedURI, | ||
| identityIndex: identityIndex, | ||
| identityPubkeys: keys, | ||
| signer: signer, | ||
| nowUnix: UInt32(Date().timeIntervalSince1970) | ||
| ) |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: Carried-forward: claim reads live trimmedURI instead of the previewed value (coherence only, not exploitable)
claim() captures preview at line 176 for the post-claim contact prompt but reads trimmedURI fresh at line 206, and the URI TextField is not .disabled(isClaiming). In principle the submitted link could diverge from the previewed one. In practice trimmedURI is evaluated synchronously before the first await suspension in the @MainActor Task (the preceding key-derivation calls at lines 192/197 are non-async), so no user text edit can interleave between preview capture and claim submission within that run, and Rust re-parses and re-validates the submitted URI before any funds move. No fund-loss or authorization impact — retained only as a coherence nitpick: capturing the trimmed URI into an immutable local at submit time and disabling the field while claiming would make the preview↔claim pairing explicit.
source: ['claude']
Collaborator
There was a problem hiding this comment.
Still valid at commit 4e3a9b06 as a coherence-only nitpick — the latest push only reorders a re-export and does not change the claim sheet. Carried forward into the current cumulative review.
Collaborator
There was a problem hiding this comment.
Code Review
Prior Findings Reconciliation
All four findings from 7b371fe7 are STILL VALID at 4e3a9b06; the one-line latest delta touched none of their code:
- suggestion — inviter DPNS metadata is used for contact bootstrap without name↔identity verification
- suggestion —
created_at_secsis back-computed from expiry under a caller assumption the library does not enforce - nitpick — interior-NUL usernames can produce
has_inviter=truewith a null username - nitpick — claim reads live
trimmedURIrather than an immutable copy of the previewed value (coherence-only, not exploitable)
Carried-Forward Prior Findings
All four remain in the verified finding set and in their existing inline threads. No blockers were found.
New Findings In Latest Delta
None. 7b371fe7..4e3a9b06 only sorts the invitation_persistence re-export in rs-platform-wallet-ffi/src/lib.rs.
🟡 2 suggestion(s) | 💬 2 nitpick(s)
🤖 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/ClaimInvitationSheet.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ClaimInvitationSheet.swift:217-224: Inviter DPNS name is displayed and used for contact bootstrap without verifying it resolves to the embedded identity id
The `dashpay://invite` link is untrusted bearer data. `parse_invitation` copies `inviter_id` and `inviter_username` verbatim from the payload (rs-platform-wallet-ffi/src/invitation.rs:407-417); the claim UI renders the username as the sender (`From <name>`, line 151-153) and, on opt-in, sends a contact request to the embedded `inviterId` (claim line 217-224 -> sendContact line 242-246) with no check that the advertised DPNS username actually resolves to that identity id. A malicious inviter can fund a valid voucher that displays a trusted user's DPNS name while embedding the attacker's own identity id; after claiming, the invitee taps Add and unknowingly establishes a reciprocal contact with the attacker. Impact is a social-engineering / contact-misdirection vector — opt-in on both ends and non-fatal (the request is re-sendable, no direct fund loss), which is why this is a suggestion, not a blocker. Per packages/swift-sdk/CLAUDE.md this name->id binding belongs in the platform-wallet claim/preview layer (resolve the DPNS name for `inviterId` on Platform and require it to match the advertised username) so every UI inherits it; otherwise show the raw identity id and label the name as unverified link-supplied metadata. Verified STILL VALID at head 4e3a9b06 — the claim path still trusts the raw payload fields.
In `packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs:247: created_at_secs is back-computed from expiry, coupling the persisted record to a caller assumption the library doesn't enforce
The persisted invitation record derives its creation time as `created_at_secs: expiry_unix.saturating_sub(MAX_INVITATION_TTL_SECS)` (line 247). This is only correct when `expiry_unix == now + MAX_INVITATION_TTL_SECS` — which the FFI enforces (rs-platform-wallet-ffi/src/invitation.rs:116) but `create_invitation` itself does NOT: it takes an arbitrary `expiry_unix: u32` (line 166) and its own doc (lines 154-155) explicitly says the caller is responsible for clamping. A non-FFI caller (a future direct library consumer or a test) passing a custom short expiry persists a garbage `created_at` — a short expiry yields 0 via saturating_sub (a 1970 timestamp). The value is display-only metadata for the Sent-invitations list, so this is not a hazard in the shipped FFI path, but reverse-deriving a value the FFI already holds (`now_unix`) from an unrelated field is fragile and silently breaks if the TTL derivation changes. Thread an explicit `now_unix`/`created_at_secs` into `create_invitation` rather than reconstructing it from expiry. Verified STILL VALID at head 4e3a9b06.
Carried-forward findings remain in their existing inline threads; this exact-SHA cumulative review records their current status without duplicating inline comments.
Source: reviewers — opus (general, security-auditor, rust-quality, ffi-engineer); gpt-5.6-sol (general, security-auditor, rust-quality, ffi-engineer; failed before review because the ACP refresh token is revoked). Verifier — opus.
Resolve the base advance (11 commits, incl. #4072 provider operator/node keys + reveal address private keys) that left the PR CONFLICTING on GitHub. The one real conflict was in changeset/mod.rs, where both branches extended the `pub use changeset::{…}` re-export block; kept both sides — our InvitationChangeSet/InvitationEntry/InvitationStatus and the base's ProviderKeyAccountEntry/ProviderKeyExtendedPubKey/ProviderPlatformNodePubKey. Verified post-merge: cargo fmt --check --all clean; platform-wallet and platform-wallet-ffi compile; the invitation funding-index regression tests pass (4/4). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift (1)
2023-2027: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject username-only inviter metadata.
When
inviterUsernameis supplied without an identity ID, Rust ignores it and creates a voucher without contact bootstrap. Require both values or neither.Proposed fix
- if inviterIdentityId != nil && inviterUsername == nil { + if (inviterIdentityId == nil) != (inviterUsername == nil) { throw PlatformWalletError.invalidParameter( - "inviterUsername is required when inviterIdentityId is provided" + "inviterIdentityId and inviterUsername must be provided together" ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift` around lines 2023 - 2027, Update the inviter metadata validation in the surrounding platform-wallet method to reject the inverse case as well: if inviterUsername is provided while inviterIdentityId is nil, throw PlatformWalletError.invalidParameter. Require inviterIdentityId and inviterUsername to be supplied together or both omitted, preserving the existing error handling style and message clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`:
- Around line 893-923: Move mnemonic resolution out of Swift by changing the
Rust FFI APIs used by coreAddressPrivateKey, providerKeyAtIndex, and
createInvitation to accept the wallet storage/seed inputs needed for Rust-side
resolution rather than a MnemonicResolver handle. Remove Swift-side
MnemonicResolver construction, lifetime management, and resolver.handle passing,
leaving these methods as thin argument marshaling and result/free handling
wrappers.
---
Outside diff comments:
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`:
- Around line 2023-2027: Update the inviter metadata validation in the
surrounding platform-wallet method to reject the inverse case as well: if
inviterUsername is provided while inviterIdentityId is nil, throw
PlatformWalletError.invalidParameter. Require inviterIdentityId and
inviterUsername to be supplied together or both omitted, preserving the existing
error handling style and message clarity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71ce6bb8-98a8-435c-ac8d-2757bd40c514
📒 Files selected for processing (9)
packages/rs-platform-wallet-ffi/src/lib.rspackages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet/src/changeset/changeset.rspackages/rs-platform-wallet/src/changeset/mod.rspackages/rs-platform-wallet/src/wallet/apply.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swiftpackages/swift-sdk/SwiftExampleApp/TEST_PLAN.md
✅ Files skipped from review due to trivial changes (3)
- packages/rs-platform-wallet/src/changeset/mod.rs
- packages/rs-platform-wallet/src/wallet/apply.rs
- packages/swift-sdk/SwiftExampleApp/TEST_PLAN.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/rs-platform-wallet-ffi/src/lib.rs
- packages/rs-platform-wallet/src/changeset/changeset.rs
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift
- packages/rs-platform-wallet-ffi/src/persistence.rs
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift (1)
2023-2027: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject username-only inviter metadata.
When
inviterUsernameis supplied without an identity ID, Rust ignores it and creates a voucher without contact bootstrap. Require both values or neither.Proposed fix
- if inviterIdentityId != nil && inviterUsername == nil { + if (inviterIdentityId == nil) != (inviterUsername == nil) { throw PlatformWalletError.invalidParameter( - "inviterUsername is required when inviterIdentityId is provided" + "inviterIdentityId and inviterUsername must be provided together" ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift` around lines 2023 - 2027, Update the inviter metadata validation in the surrounding platform-wallet method to reject the inverse case as well: if inviterUsername is provided while inviterIdentityId is nil, throw PlatformWalletError.invalidParameter. Require inviterIdentityId and inviterUsername to be supplied together or both omitted, preserving the existing error handling style and message clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`:
- Around line 893-923: Move mnemonic resolution out of Swift by changing the
Rust FFI APIs used by coreAddressPrivateKey, providerKeyAtIndex, and
createInvitation to accept the wallet storage/seed inputs needed for Rust-side
resolution rather than a MnemonicResolver handle. Remove Swift-side
MnemonicResolver construction, lifetime management, and resolver.handle passing,
leaving these methods as thin argument marshaling and result/free handling
wrappers.
---
Outside diff comments:
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`:
- Around line 2023-2027: Update the inviter metadata validation in the
surrounding platform-wallet method to reject the inverse case as well: if
inviterUsername is provided while inviterIdentityId is nil, throw
PlatformWalletError.invalidParameter. Require inviterIdentityId and
inviterUsername to be supplied together or both omitted, preserving the existing
error handling style and message clarity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71ce6bb8-98a8-435c-ac8d-2757bd40c514
📒 Files selected for processing (9)
packages/rs-platform-wallet-ffi/src/lib.rspackages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet/src/changeset/changeset.rspackages/rs-platform-wallet/src/changeset/mod.rspackages/rs-platform-wallet/src/wallet/apply.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swiftpackages/swift-sdk/SwiftExampleApp/TEST_PLAN.md
✅ Files skipped from review due to trivial changes (3)
- packages/rs-platform-wallet/src/changeset/mod.rs
- packages/rs-platform-wallet/src/wallet/apply.rs
- packages/swift-sdk/SwiftExampleApp/TEST_PLAN.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/rs-platform-wallet-ffi/src/lib.rs
- packages/rs-platform-wallet/src/changeset/changeset.rs
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift
- packages/rs-platform-wallet-ffi/src/persistence.rs
🛑 Comments failed to post (1)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift (1)
893-923: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail FILE='packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift' wc -l "$FILE" sed -n '860,940p' "$FILE" printf '\n---\n' sed -n '980,1060p' "$FILE" printf '\n---\n' sed -n '2000,2075p' "$FILE" printf '\n---\n' rg -n "MnemonicResolver|platform_wallet_address_private_key|previewIdentityRegistrationKeys|platform_wallet_.*private_key|claim|parse" "$FILE"Repository: dashpay/platform
Length of output: 14777
Keep mnemonic resolution out of Swift.
These wrappers still create
MnemonicResolverin Swift and pass it into Rust FFI, so mnemonic lookup stays in the Swift/Keychain layer and secrets cross the boundary. Move that resolver flow intoplatform-walletand keep Swift as a thin marshaling layer; the same pattern is repeated inproviderKeyAtIndexandcreateInvitation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift` around lines 893 - 923, Move mnemonic resolution out of Swift by changing the Rust FFI APIs used by coreAddressPrivateKey, providerKeyAtIndex, and createInvitation to accept the wallet storage/seed inputs needed for Rust-side resolution rather than a MnemonicResolver handle. Remove Swift-side MnemonicResolver construction, lifetime management, and resolver.handle passing, leaving these methods as thin argument marshaling and result/free handling wrappers.Source: Coding guidelines
Adds the inviter-side reclaim flow (DIP-13 commit 2). An unclaimed voucher can be consumed into a Platform identity of the inviter's own, recovering the value as identity credits — the on-chain DASH is an OP_RETURN burn, so there is nothing to spend back on L1; reclaim is mechanically "claim your own invitation". - ManagedPlatformWallet.topUpIdentityWithExistingAssetLock: net-new Swift wrapper over platform_wallet_topup_identity_with_existing_asset_lock_signer (top up an existing identity from the tracked voucher lock). Mirrors resumeIdentityWithAssetLock's handle/resolver/out-param marshaling; no KeychainSigner (a top-up creates no keys) — only the wallet's own MnemonicResolver, which re-derives the voucher key. - ReclaimInvitationSheet: pick a target at reclaim time — top up an existing identity, or register a new one funded by the voucher (reusing resumeIdentityWithAssetLock). Copy states the value returns as identity credits, never spendable Dash. - InvitationsView: a Reclaim swipe action on Created rows presents the sheet; on success the local row flips to Reclaimed (SwiftData is the UI source, no Rust re-emit). On the deterministic already-consumed rejection (someone claimed it first) the row flips to Claimed with a neutral "already claimed" message — the claimant is intentionally not named. - TEST_PLAN: DP-17 (reclaim top-up), DP-18 (reclaim register-new), DP-19 (reclaim-vs-claim race → already-consumed) in Section 4.10. The T1 upsert-key <-> removal-key seam is already unit-pinned by InvitationPersistenceTests.testUpsertThenStatusChangeThenRemovalRoundTrips. Builds clean: build_ios.sh --target sim + SwiftExampleApp xcodebuild both rc=0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Carried-Forward Prior Findings
All four prior findings from 4e3a9b0 are independently re-verified against current HEAD 30ab23f and remain STILL VALID: (1) prior-inviter-metadata — ClaimInvitationSheet.swift:217-224 still builds the ContactPrompt from unverified bearer-link inviter_id/inviter_username; (2) prior-created-at — network/invitation.rs:247 still back-computes created_at_secs via saturating_sub while create_invitation (line 166) accepts an unclamped expiry; (3) prior-nul-username — ffi/invitation.rs:407-417 still emits has_inviter=true with a null username on CString failure, contradicting its ABI doc at 337-339/372-373; (4) prior-uri-coherence — claim() at 175-232 still submits live trimmedURI (line 206) rather than the previewed value. All four GitHub threads are unresolved with no author reply.
New Findings In Latest Delta
None. The literal 4e3a9b0..30ab23f delta is a v4.1-dev merge spanning 73 files (provider operator/node keys #4072, address private-key reveal, SPV peers, token pricing) that touches no invitation file; the invitation codec/FFI/claim/persistence code is byte-identical. No new in-scope defects. Two suggestions and two nitpicks carried forward; no blockers, so COMMENT.
Source: reviewers opus (general, security-auditor, rust-quality, ffi-engineer); gpt-5.6-sol (all four lanes failed before review because the ACP refresh token was revoked); verifier opus.
4 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
A funded testnet run showed that an invitation locking the previous UI default of 0.0005 DASH (50,000,000 credits) can be neither claimed nor reclaimed: creating an identity — which the invitee's claim does, and which a register-target reclaim does — needs ~0.00228 DASH in credits, and even a top-up-target reclaim needs slightly more than the voucher held. The state transition is rejected with IdentityAssetLockTransactionOutPointNotEnoughBalanceError, so a sub-threshold voucher produces a dead invitation. Add MIN_INVITATION_DUFFS (0.003 DASH) and reject amounts below it at creation, mirroring the existing MAX_INVITATION_DUFFS cap. Enforcing the floor at create time fixes both the claim and reclaim paths at once, since an under-funded voucher can no longer be minted. The floor sits above the identity-registration minimum with margin so the new identity keeps a small usable starting balance. No unit test: this mirrors the untested MAX_INVITATION_DUFFS range guard, and the binding evidence is the funded run that surfaced the floor; a follow-up funded e2e confirms a viable-amount invitation is claimable and reclaimable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
A voucher locking the old UI default 0.0005 DASH (50,000,000 credits) could be neither claimed nor reclaimed: creating an identity (the invitee's claim, and the register-target reclaim) needs ~228,000,000 credits, and even a top-up-target reclaim needs slightly more than the voucher held — surfacing as IdentityAssetLockTransactionOutPointNotEnoughBalanceError. - Raise the create default from 0.0005 to 0.005 DASH (comfortably above the ~0.00228 DASH registration floor, leaving the invitee a usable balance). - Enforce a 0.003 DASH minimum (mirrors the new Rust MIN_INVITATION_DUFFS = 300,000 duffs). Rust remains the source of truth and rejects a sub-min amount at create; this is UX parity so the submit gate + hint reject it first. Placeholder and range copy updated; the existing MAX (0.01) mirror is unchanged. Builds clean: build_ios.sh --target sim + SwiftExampleApp xcodebuild rc=0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
…ay + test The reclaim sheet flips a row to Claimed and shows a benign "already claimed" message when the consume is rejected as already-consumed. The SDK surfaces a consensus error as "SDK error: Protocol error: <consensus Display verbatim>", so the canonical Display of IdentityAssetLockTransactionOutPointAlreadyConsumed — "…already completely used" — appears verbatim. - Narrow isAlreadyConsumed to match ONLY "already completely used". Drop the broad "already consumed" / "alreadyconsumed" phrases: they never occur in the real Display and only widened false-positive risk (misclassifying an unrelated failure — e.g. the not-enough-credits error, which shares the "Asset lock transaction …" prefix — as benign, wrongly flipping a live invitation to Claimed). A typed FFI result code is the robust long-term fix. - Refactor the check into a pure static seam isAlreadyConsumed(message:). - Add ReclaimInvitationClassifierTests: the real already-consumed Display classifies true; the real not-enough-credits error and a transport error classify false; the dropped broad phrases classify false. Pins the false-positive safety and guards against Display wording drift. Builds clean + tests pass: build_ios.sh + SwiftExampleApp xcodebuild + the new test all rc=0 (5/5 cases pass). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
The reclaim runs in a fire-and-forget Task while a ProgressView replaces the Reclaim button. Cancel stayed tappable throughout, so dismissing the sheet mid-reclaim let the Task keep running and mutate the row's status + save after the sheet was gone, and re-opening could launch an overlapping reclaim. Gate Cancel on the in-flight flag, mirroring the submit button. No funds were ever at risk (a second consume is deterministically rejected on-chain); this closes the stale-write/overlap window. Builds clean: build_ios.sh --target sim + SwiftExampleApp xcodebuild rc=0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
DashPay invitations (DIP-13 sub-feature
3') — onboard a friend who has no Dash. An existing user pre-funds a one-time asset-lock voucher and shares adashpay://invitelink; the invitee registers their own new identity funded by that voucher (no L1 Dash required) and optionally becomes a contact. Tracked as the "NEXT" item in the DashPay backlog (#4020);SPEC.mdMilestone 5 asked for this design pass.Design + review + owner-sync are captured in
docs/dashpay/DIP15_INVITATIONS_SPEC.md(four research streams + three adversarial spec reviews folded; §14 records resolutions).What was done?
Rust core (this PR so far, all compile-clean + unit-tested):
crypto/invitation.rs) — encode/parse the self-contained versioneddashpay://invite?data=payload (voucher key + embedded InstantSendAssetLockProof+ advisory expiry + optional inviter info), plusvalidate_claimablefail-fast pre-submit checks. Hand-rolled bounded binary format (no dependency on the optionalserdefeature). 13 unit tests.MnemonicResolverCoreSigner::export_invitation_private_key) — the seedless raw-key export the create flow needs, gated to the exact9'/coin'/5'/3'/idx'path so it can never export the user's own auth/registration/top-up keys. Negative gate test.network/invitation.rs) —create_invitation(funds the voucher, exports the key, builds the link, Rust-enforced amount cap) andclaim_invitation(registers the invitee identity from the imported voucher via the SDK'sput_to_platform_with_private_key, wrapped in the CL-height retry).Key decisions (owner-synced): InstantSend proof (short IS-scoped advisory expiry for staleness) · contact-bootstrap opt-in on both ends (inviter checkbox + invitee "establish contact?" prompt, reusing the existing contact-request path) · proper wallet-persister persistence.
How Has This Been Tested?
Rust unit tests green: codec round-trip + every malformed rejection (13), export path-gate negative test,
validate_claimable(expiry / non-Instant / voucher-not-controlling-output).852 Rust tests green (platform-wallet 407, platform-wallet-ffi 144, rs-sdk-ffi 301),
fmt --check --all+ targetedclippy --all-featuresclean.SwiftExampleApp xcodebuild green (iPhone 17 / arm64) — Swift wrappers compile + link.
Runtime UI/FFI e2e on the iPhone 17 simulator (verified live): app launches on testnet; DashPay tab reachable; the claim entry point (gift) is present with NO identity (the fresh-invitee path); the claim sheet opens; and the parse-preview FFI works live — a malformed
dashpay://invitelink renders "This link isn't a valid invitation." (exercisesparse_invitation_uri+validate_claimable+InvitationPreviewFFI).Full funded round-trip (DP-14) — DONE, on-chain verified on testnet. Driven end-to-end through the app on the iPhone 17 simulator with real testnet DASH:
GEw4gdo3qCLqNtPyuby4wDBrNUpwucT4qAeAUYbDDSRu(+ DPNSinvitere2e070801.dash+ DashPay profile).dashpay://invitelink (wallet balance dropped exactly 0.005 + fee).parse_invitation+InvitationPreviewFFI).C9tc8ef7TREaL4QoqnNZaMychs6za3aLDsxC96TkHpNQ, outer-signed by the imported voucher key.On-chain confirmation via
testnet.platform-explorer: the invitee identity exists withtotalTopUpsAmount = 500000000credits (= the 0.005 DASH voucher) andtotalDocuments = 1(the contact-request bootstrap). Every layer verified: Rust unit + FFI marshaling + Swift xcodebuild + live UI + real testnet chain state.Breaking Changes
None. Reuses existing on-chain artifacts (AssetLock, IdentityCreate, contactRequest); adds one new trait method (
ContactCryptoProvider::export_invitation_private_key) and new FFI, no changes to existing behavior.Checklist:
Remaining before ready-for-merge (WIP — draft)
platform_wallet_create_invitation/_claim_invitation(committedf59edf62c1, 6/6 tests)ManagedPlatformWallet(committed4d39bdcf5c; SwiftExampleApp xcodebuild green, iPhone 17/arm64)CreateInvitationSheet, xcodebuild green) —8fd0613fe05967661ec0) + parse-preview FFI (bd60102eb0)dashpay://invitedeep-link (4e0dfbedef)invitationstable + wire create_invitation) —2b2a8712b0GEw4gdo3…→ 0.005 DASH voucher → inviteeC9tc8ef7…;totalTopUps=500000000credits,totalDocuments=1contact request)SwiftExampleApp/TEST_PLAN.md§4.10) —6a730a9b04🤖 Generated with Claude Code
https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
Follow-ups (out of scope for this PR, tracked)
invitationstable + changeset). Remaining:PersistentInvitationSwiftData model + FFI→SwiftData bridge for the newinvitationsPlatformWalletChangeSetoverlay (mirroringasset_locks/dashpay_payments_overlay) + anInvitationsView. Deferred deliberately — the bridge is a data-integrity-sensitive path that deserves fresh context + its ownbuild_ios.shwindow rather than a rushed land; nothing is lost functionally (the core create→claim→contact flow is complete and the Rust half already persists).Full funded testnet round-trip (DP-14).DONE — on-chain verified (see "How Has This Been Tested?");TEST_PLAN.md§4.10.Summary by CodeRabbit
dashpay://invitedeep-link handling and a “Sent invitations” list powered by new invitation persistence (plus persistence test coverage).