Repository navigation
feat(platform-wallet)!: support DashPay shielded tips with dedicated accounts - #5288
PastaPastaPasta wants to merge 15 commits into
Conversation
|
Important Review skippedToo many files! This PR contains 120 files, which is 20 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry.
 |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
|
⛔ Final review complete — 6 blocking finding(s) (commit 279719e) · triage: critical |
Live V4 adds PersistentDashpayPaymentAddresses, which sorts ahead of PersistentIdentity and renumbers it from 13 to 14. Core Data embeds the ordinal in generated column names, so the token-balance table's Z13TOKENBALANCES foreign key becomes Z14TOKENBALANCES with its values intact. validatePreservation normalized Z_<n> join-table names but not the Z<n><NAME> column form, so the V1 -> V3 -> V4 and historical V2 -> V3 -> V4 migrations were reported as removing a column (DashModelMigrationTests, CI on #5288). validatePreservation now keys columns by entity name for the cross-store comparison; a dropped or type-converted column is still rejected. layout()/evidence() keep their existing keys so recovery journals written by earlier builds still verify. Test: a renumbered copy of the dash-v1 fixture (entity inserted at 8, join-table and generated columns renamed) passes; the same copy with the column dropped or converted to VARCHAR is rejected. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the supplied findings against head 02460d6 and confirmed four issues involving identity-placement persistence, restart-safe submissions, identity-specific spending-account selection, and concurrent account binding. These are client-side defects, classified as suggestions under the supplied policy reserving blocking severity for consensus invariants. Validation was static: the supplied CI snapshot reports successful Rust workspace and Kotlin checks, skipped dedicated wallet tests, and Swift build/tests still in progress.
🟡 4 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, cross-language diff directly changes key derivation and dedicated account handling in packages/rs-platform-wallet/src/wallet/shielded/tips.rs, funds submission in packages/rs-platform-wallet-ffi/src/shielded_send.rs, and persisted-data migrations in SQLite, SwiftData, and Room. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/identity/state/manager/lifecycle.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/state/manager/lifecycle.rs:206-207: Preserve corrected identity indices through changeset merging
The new adoption path changes identity_index, but IdentityChangeSet::merge never copies that field into an existing entry. With SQLite FlushMode::Manual, a snapshot buffered at placeholder index 0 can therefore absorb the subsequent discovery snapshot at verified index 5 while retaining index 0. Both Buffer::normalize_identities and the final buffered merge use these semantics, so flush/reload restores the incorrect placement. The same stale slot also remains occupied during buffered admission checks, potentially rejecting another identity that discovery places into the freed slot.
Make identity_index follow the latest snapshot alongside wallet_id. Add a SQLite regression that buffers an old-slot snapshot, adopts the identity into its verified slot, persists the resulting snapshot, then flushes and reloads; also verify that another identity can occupy the freed slot.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ShieldedTipSubmission.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ShieldedTipSubmission.swift:28-32: Restore outstanding tip submissions before enabling another send
The submission guard and wallet map are memory-only, and each new process initializes the guard to ready. Kotlin's ShieldedTipSubmissions has the same lifecycle. Restarting after a broadcast with an unresolved outcome therefore removes the warning and permits the user to retry the same intended payment as a new transfer. A relay withholding confirmation is sufficient to create this ambiguity; process termination during broadcast also exposes it.
Rust records pending transfer activity and can redrive the original transition, but transfer reserves individual input notes rather than a payment intent. Additional spendable notes can fund the retry, allowing both transfers to execute. Persist an unresolved intent before entering the native send, restore its guard before enabling submission, and reconcile it with the original activity or transition. Add process-recreation coverage with an ambiguous original transfer and additional spendable notes; the existing reopening tests retain the same in-memory owner.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayProfileView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayProfileView.swift:39-44: Verify the identity index before selecting its tip spending account
Rust's optional index getter returns recorded metadata, not a verified identity-to-account association. The mobile restore path constructs ManagedIdentity::new(identity, spec.identity_index), so a persisted placeholder zero becomes Some(0). The existing should_refuse_tip_account_for_a_placeholder_index_until_it_is_verified regression demonstrates an identity belonging at index 5 restored at index 0 while index 0's tip account is bound.
Preparation rejects this association by checking the seed, but this balance/spending selector and Kotlin's corresponding selectors bypass that check. They can display index 0's funds as belonging to the restored identity and pass that account to the send API, which validates account keys but receives no source identity to verify. Resolve identity-specific tip accounts through a verified wallet operation, or disable these selectors until the association is verified. Extend the placeholder regression to cover balance lookup and spending-account selection, not only address preparation.
In `packages/rs-platform-wallet/src/wallet/shielded/tips.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/tips.rs:142-147: Make tip preparation an atomic augmentation of the bound account set
The account snapshot and replacement bind are separate operations. Preparation can snapshot ordinary account 0, another task can finish binding accounts 0 and 1, and preparation can then bind its stale snapshot plus the tip account. bind_shielded automatically retains discovered and persisted tip accounts, but not omitted ordinary accounts. Consequently, register_locked removes account 1 from registration and purges its live subwallet state, contradicting preparation's ordinary-account preservation guarantee.
The shield guard and coordinator install transaction serialize individual installations, not the preceding read-modify-write sequence. Serialize configuration changes across the snapshot and bind, or provide an augmentation operation that merges with the current registration inside the installation transaction. Add a deterministic interleaving regression proving that an ordinary account added during preparation remains bound.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Seed verification runs while holding the wallet-manager read guard — The guard covers bounded synchronous derivation and verification with no network call or await inside the guarded block. Writers are briefly excluded, but the finding supplies no demonstrated hot-path or material contention problem; moving this work outside the lock is an optional optimization rather than a confirmed review defect.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
| @Published private(set) var status: Status = .ready | ||
| @Published private(set) var message: String? | ||
|
|
||
| var busy: Bool { status == .sending } | ||
| var submitted: Bool { status != .ready } |
There was a problem hiding this comment.
🟡 Suggestion: Restore outstanding tip submissions before enabling another send
The submission guard and wallet map are memory-only, and each new process initializes the guard to ready. Kotlin's ShieldedTipSubmissions has the same lifecycle. Restarting after a broadcast with an unresolved outcome therefore removes the warning and permits the user to retry the same intended payment as a new transfer. A relay withholding confirmation is sufficient to create this ambiguity; process termination during broadcast also exposes it.
Rust records pending transfer activity and can redrive the original transition, but transfer reserves individual input notes rather than a payment intent. Additional spendable notes can fund the retry, allowing both transfers to execute. Persist an unresolved intent before entering the native send, restore its guard before enabling submission, and reconcile it with the original activity or transition. Add process-recreation coverage with an ambiguous original transfer and additional spendable notes; the existing reopening tests retain the same in-memory owner.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
There was a problem hiding this comment.
Not addressed in this PR; it needs a maintainer call on scope. The description has listed this thread as open since the PR was recreated. The finding holds: ShieldedTipSubmission (ShieldedTipSubmission.swift:25-32) and Kotlin's ShieldedTipSubmissions keep their guard in memory, so a relaunch after an uncertain broadcast starts in .ready. Because the native send reserves notes rather than a payment intent, a retry can pay the recipient again.
Closing that needs a durable outstanding-intent record in both example apps and a reconciliation against native activity on relaunch. That's new persistence plus recovery UI, which I'd rather keep out of this already large change and land as a follow-up with the process-recreation test you describe.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Still applies (re-reviewed at 4d92dc45): Both example apps still keep the submission guard only in memory, so relaunch after an ambiguous broadcast resets the state to ready without durable intent reconciliation.
There was a problem hiding this comment.
Tracked in #5674, together with the source-account race. Please mark it deferred. Whether this PR can merge without restart-safe submission is left to the maintainer review; the PR description has listed it as open since the PR was recreated.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Still applies (re-reviewed at 7a242efd): The submission guard remains process-local in both example apps, so relaunch after an ambiguous broadcast resets the state to Ready and permits a retry without reconciling the original payment intent.
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification confirms the original concurrent-bind race and buffered identity-index merge defect are fixed, and the reported occupied-placeholder spending case is now rejected before recipient resolution. Six client-side suggestions remain concerning restart recovery, stale source-account selection, identity replay, account preservation, and test isolation; no consensus-blocking defect was confirmed. No builds or tests were run, and the supplied exact-head CI snapshot still had most validation queued or pending.
🟡 6 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Serialize shielded_add_account with the new shielded config lock
packages/rs-platform-wallet/src/wallet/platform_wallet.rs:1154-1160
Tip preparation now holds shielded_config_lock across its account snapshot and replacement bind, but shielded_add_account writes the same shielded_keys map without participating in that lock. Preparation can snapshot [0], an addition can persist and insert ordinary account 1, and preparation can then install its stale snapshot plus the tip account, overwriting the completed insertion while leaving its viewing-key row persisted. This breaks the new preparation helper's ordinary-account preservation guarantee even though the concurrent-bind case is fixed. Acquire the same configuration lock for the complete add operation, preserving the existing rule that host callbacks run outside the key-slot guard. The add API intentionally does not register accounts with the coordinator itself, so this finding concerns loss of its in-memory insertion, not a claim that it creates a coordinator registration. Extend the interleaving coverage to race preparation with this writer.
source: muse-spark-1.3-contributor (phase1-reviewer: rust-quality)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This large, intricate diff directly changes funds movement and dedicated ZIP-32 key/account handling in packages/rs-platform-wallet/src/wallet/shielded/tips.rs and packages/rs-platform-wallet-ffi/src/shielded_send.rs, as well as persisted-data migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/DashPayTabScreen.kt`:
- [SUGGESTION] packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/DashPayTabScreen.kt:336-345: Invalidate and reverify the source account when an identity is re-slotted
The lookup keys include the wallet wrapper, manager, and identity ID, but not the identity's placement. This PR allows discovery to move that same identity from placeholder index 0 to verified index 5 without replacing those objects. If discovery completes while this composition is alive, the cached result remains `0x40000000` and is still offered as that identity's dedicated source account. A blocking discovery can continue natively after its search screen is dismissed, so returning to DashPay before it finishes can expose this interleaving. The new Rust check does not reject the stale source: `verify_tip_account_owner` succeeds when slot 0 has become empty, and also succeeds if the rightful identity 0 later occupies it. A confirmed send can thus spend the retained account-0 funds rather than the selected identity's account-5 funds. Invalidate the lookup when verified placement changes and recheck the selected source identity/account association at submission; checking only the account's current occupant is insufficient. Add a regression resolving at placeholder 0, adopting into 5, and attempting a send with the old selection. The profile-screen lookup also needs placement-change invalidation.
In `packages/rs-platform-wallet/src/wallet/identity/state/manager/lifecycle.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/state/manager/lifecycle.rs:195-207: Carry identity relocation through changeset replay
Adoption now changes bucket placement and identity coordinates, but `IdentityManager::apply_identity_entry` still updates an existing identity in place and returns without applying `wallet_id`, `identity_index`, or `location_index`. Replaying valid snapshots A@0, then adopted A@5, then newly admitted B@0 through the public `PlatformWallet::apply` or `PlatformWalletInfo::apply_changeset` API leaves A at slot 0 until B's fresh insertion overwrites it. A's state is lost and its reverse-index entry still points to slot 0, so `identity(A)` returns B. Promotion from the observed bucket likewise fails to reproduce the adoption. Normal startup reconstruction uses a different path, which limits the current impact, but the public replay API no longer reproduces the mutations introduced here. Apply relocation while preserving managed state and reverse-index consistency, reject unrelated occupied targets, and handle merged batches independently of identity-ID ordering when one entry frees another's slot. Add round-trip coverage for observed-identity promotion and A@0 → A@5 followed by B@0.
In `packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/identity/SearchWalletsForIdentitiesScreen.kt`:
- [SUGGESTION] packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/identity/SearchWalletsForIdentitiesScreen.kt:147-151: Preserve ordinary accounts during discovery-triggered rebinding
This new refresh calls the replacement bind API with its default account list `[0]`. For a wallet already bound to ordinary accounts `[0, 1]`, Rust adds discovered and persisted tip accounts but does not retain omitted ordinary account 1. `NetworkShieldedCoordinator::register_locked` consequently removes account 1's registration and purges its live notes and watermark, so discovering identities unexpectedly stops an existing ordinary account from scanning. Swift preserves a snapshot of ordinary accounts, but its snapshot and replacement call are also outside the native configuration lock. Provide a refresh operation that reads the current configuration and adds discovered tip accounts atomically under `shielded_config_lock`, and use it from discovery rather than treating `[0]` as a complete replacement configuration. Keep full bind available for intentional replacement. Cover discovery refresh starting from `[0, 1]` and verify that both ordinary registrations survive.
In `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_wallet.rs:1154-1160: Serialize shielded_add_account with the new shielded config lock
Tip preparation now holds `shielded_config_lock` across its account snapshot and replacement bind, but `shielded_add_account` writes the same `shielded_keys` map without participating in that lock. Preparation can snapshot `[0]`, an addition can persist and insert ordinary account 1, and preparation can then install its stale snapshot plus the tip account, overwriting the completed insertion while leaving its viewing-key row persisted. This breaks the new preparation helper's ordinary-account preservation guarantee even though the concurrent-bind case is fixed. Acquire the same configuration lock for the complete add operation, preserving the existing rule that host callbacks run outside the key-slot guard. The add API intentionally does not register accounts with the coordinator itself, so this finding concerns loss of its in-memory insertion, not a claim that it creates a coordinator registration. Extend the interleaving coverage to race preparation with this writer.
In `packages/rs-platform-wallet/src/wallet/shielded/tips.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/tips.rs:258-263: Isolate the preparation pause hook by wallet instance
The global pause hook is consumed using only the deterministic wallet ID. The concurrency regression, placeholder-index regression, and discovery/restoration regression all create independent testnet wallet instances from `MESSAGE_SIGNING_TEST_MNEMONIC`, so they share that ID. Under parallel execution, another test's preparation can remove the concurrency test's hook and signal `reached` while holding its own configuration lock. The intended preparation then runs without pausing, allowing the competing bind to complete inside the timeout and failing the regression despite correct production locking. Attach the hook to shared state belonging to the actual wallet instance, use an instance-specific token, or give the concurrency fixture a distinct deterministic mnemonic. The current wallet-ID key does not provide the parallel-test isolation claimed by the comment.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ShieldedTipSubmission.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ShieldedTipSubmission.swift:28-32: Restore outstanding tip submissions before enabling another send
(existing thread: https://github.com/dashpay/platform/pull/5288#discussion_r4185823786)
The shared owner preserves the submission guard across sheet dismissal, but a new process reconstructs an empty wallet map and a `.ready` submission. Kotlin has the same lifecycle. After termination during an accepted-but-unconfirmed send, reopening therefore removes the uncertainty warning and permits the user to confirm the same payment again. Native pending activity and redrive records do not prevent this: note reservations exclude particular inputs, while `reserve_unspent_notes` can select other funded notes for a fresh transfer. The redrive transaction itself is armed only after an ambiguous broadcast result returns, so process loss during the call also cannot be treated as a recovered safe retry. Persist a network/wallet-scoped unresolved intent before entering native spending, restore its warning before enabling confirmation, and reconcile native activity or require explicit acknowledgment that another submission is a separate payment. Add process-recreation tests using newly constructed owners and sufficient independent notes to fund both attempts. The PR's pending maintainer decision is not an implemented recovery boundary.
|
Re: the review's non-inline finding "Serialize shielded_add_account with the new shielded config lock" ( Confirmed and fixed in 93d4d3b. The new regression 🤖 Posted autonomously by Claude on behalf of pasta. |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At head 6c88c31, the configuration-locking, pause-hook isolation, and identity-index merge fixes are present, but three client-side suggestions remain around merged replay, queued source-account selection, and restart-safe submission state; no consensus blocker was confirmed. All nine prior findings were independently reconciled, with the acknowledged balance-provenance follow-up kept deferred. Validation was static only: the supplied CI snapshot shows Rust workspace and Kotlin checks passing, the dedicated wallet job skipped, and PR Hygiene pending; the Swift failure log confirms “database is locked” in testCaptureHistoricalV2Fixture.
🟡 3 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This large, intricate diff changes funds movement and key handling in packages/rs-platform-wallet-ffi/src/shielded_send.rs and packages/rs-platform-wallet/src/wallet/shielded/tips.rs, as well as persisted-state migrations across SQLite, SwiftData, and Room. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/apply.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/apply.rs:162-170: Carry identity relocation through changeset replay
The new ordering handles sequential relocation, but it still evaluates occupancy against transient state rather than the merged batch's final survivors. Starting with A@0, merge the changesets from moving A to 5, removing A, and inserting B@5. IdentityChangeSet::merge retains both A's snapshot and tombstone, so this partition postpones A's removal. Replay moves A to 5, skips fresh B because A occupies 5, then removes A and returns success with B missing. B's subsequent key/contact entries cannot route, and applying the batch again can produce a different result.
The relocation loop in identity/state/manager/apply.rs has another instance of the same final-state problem: A@0/B@1 ending at A@1/B@0 remains blocked and falls back to in-place updates. The public adoption API can produce those final snapshots through free slot 2, and SQLite's check_index_conflicts explicitly accepts a two-way swap, so replay can disagree with persistence even for a conflict-free final batch.
Remove all tombstoned identities before placement and exclude their snapshots. Stage surviving relocations so valid cycles can be installed while preserving managed state and reverse-index consistency. Add regressions using actual merged mutation changesets, checking replay against the final persisted placement and checking repeated application.
In `packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/ShieldedTipSheet.kt`:
- [SUGGESTION] packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/ShieldedTipSheet.kt:95-99: Invalidate and reverify the source account when an identity is re-slotted
The placement keys now refresh the account offered by the open sheet, but confirmation copies it into sendAccount before history.confirm suspends on Dispatchers.IO. Discovery can move the selected identity A from placeholder 0 to verified 5 during that suspension. Subsequent recomposition cannot change the account captured by the application-owned submission, so the native call still receives 0x40000000.
Rust's verify_tip_account_owner accepts that account if slot 0 is empty, or verifies its rightful replacement B if B now occupies slot 0. Neither case establishes that the submitted account belongs to selected identity A. These are still wallet-owned funds, and explicitly spending a retired account is legitimate; the defect is silently using that pool from an identity-specific dedicated-account choice.
Carry the expected source identity through the identity-scoped submission and verify its seed-derived association with the requested account before selecting notes. Preserve intentional retired-account spending through an explicitly account-based path. Add a regression that pauses recipient-history persistence, re-slots A, and verifies that the captured old source is rejected.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ShieldedTipSubmission.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ShieldedTipSubmission.swift:28-32: Restore outstanding tip submissions before enabling another send
(existing thread: https://github.com/dashpay/platform/pull/5288#discussion_r4185823786)
The application-owned guard survives navigation, but its status and registry are memory-only. After process termination, forWallet creates a submission in ready without consulting persisted outgoing activity or recovery state. Kotlin's registry has the same lifetime boundary, and recipient confirmation history records the destination rather than the payment outcome.
If the process exits after broadcast but before the result reaches the host, relaunch permits a fresh attempt while the first payment may already have executed or remain eligible for redrive. Native reservations protect the first payment's nullifiers, not the logical payment: operations::transfer selects other available notes without an unresolved-intent check. With another sufficient note set, both transfers can execute and pay the recipient twice. This is duplicate payment, not a consensus double-spend.
Persist an unresolved intent before entering the native send, restore a conservative reconciliation state before enabling another attempt, and clear it only after a definitive outcome. Add process-recreation coverage with an unresolved first payment and enough other notes to fund another transfer; the current tests reuse the same submission owner.
| val sendAccount = if (spendTips) tipAccount else 0 | ||
| recipient = null | ||
| submission.submit { | ||
| history.confirm(manager.network, walletId, sendUsername, selected) | ||
| manager.sendShieldedTip(walletId, sendUsername, selected, sendAmount, account = sendAccount) |
There was a problem hiding this comment.
🟡 Suggestion: Invalidate and reverify the source account when an identity is re-slotted
The placement keys now refresh the account offered by the open sheet, but confirmation copies it into sendAccount before history.confirm suspends on Dispatchers.IO. Discovery can move the selected identity A from placeholder 0 to verified 5 during that suspension. Subsequent recomposition cannot change the account captured by the application-owned submission, so the native call still receives 0x40000000.
Rust's verify_tip_account_owner accepts that account if slot 0 is empty, or verifies its rightful replacement B if B now occupies slot 0. Neither case establishes that the submitted account belongs to selected identity A. These are still wallet-owned funds, and explicitly spending a retired account is legitimate; the defect is silently using that pool from an identity-specific dedicated-account choice.
Carry the expected source identity through the identity-scoped submission and verify its seed-derived association with the requested account before selecting notes. Preserve intentional retired-account spending through an explicitly account-based path. Add a regression that pauses recipient-history persistence, re-slots A, and verifies that the captured old source is rejected.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, ffi-engineer)
There was a problem hiding this comment.
Leaving this one for the maintainer review rather than fixing it in this PR. The race is real: ShieldedTipSheet.kt:95 captures sendAccount before history.confirm suspends at line 98. A re-slot of the selected identity inside that one preferences write would send from the old slot's tip account. verify_tip_account_owner (tips.rs:169) only proves that the account is wallet-owned, not that it belongs to the identity the user picked.
The fix you describe is the right shape, but it changes the native send in every layer (Rust, C FFI, JNI, Swift, Kotlin) so that it carries the expected identity. That's a sizeable ABI change on top of an already large PR, for a window the user can only hit by running discovery during that write. No funds leave the wallet either way; the wrong pool can pay. I'd take it as a follow-up with the regression you outline.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Still applies (re-reviewed at 4d92dc45): The sheet still captures the account before the suspending confirmation write, and the native API still receives no expected source identity, so a re-slot during that write can select the old identity-specific pool.
There was a problem hiding this comment.
Tracked in #5674 with the restart-safety follow-up. Please mark it deferred; the fix needs an identity-aware native send through every layer.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Still applies (re-reviewed at 7a242efd): Placement-keyed lookup refreshes do not protect the account captured by the open sheet or the native send's pre-await owner check. A re-slot can still leave the send using an empty retired slot or another identity's now-current slot, so the selected identity must be carried through and revalidated before spending.
|
Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The client-only diff preserves proof-verified recipient resolution and addresses the reported identity-replay, account-configuration locking, and changeset-merging defects. Three non-blocking concerns remain: two source-account relocation windows and loss of unresolved-payment guards after process restart; unverified balance attribution remains an explicitly separated follow-up. Validation was static only: git diff --check passed, and the supplied exact-head CI snapshot still had Kotlin build/tests and PR Hygiene pending.
🟡 3 suggestion(s)
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This large, intricate diff changes key derivation and viewing-key binding in packages/rs-platform-wallet/src/wallet/shielded/tips.rs, funds submission in packages/rs-platform-wallet-ffi/src/shielded_send.rs, and persisted profile encoding and migrations across SQLite, SwiftData, and Room. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/ShieldedTipSheet.kt`:
- [SUGGESTION] packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/ShieldedTipSheet.kt:95-99: Invalidate and reverify the source account when an identity is re-slotted
(existing thread: https://github.com/dashpay/platform/pull/5288#discussion_r4190547321)
The source account is copied into sendAccount before history.confirm suspends on Dispatchers.IO. Discovery can move the selected identity from placeholder slot 0 to verified slot 5 during that suspension. A subsequent Room update cannot change the captured argument, and the JNI/C send signatures carry the recipient identity but no expected source identity. Rust's verify_tip_account_owner accepts the emptied slot-0 account, so with sufficient retained notes the payment can spend account 0x40000000 instead of the selected identity's account-5 pool. This is a source-selection error, not an unauthorized-spending bypass: both pools remain wallet-owned. Carry the selected source identity and confirmed account through the identity-specific submission path, reject a changed association before spending, and require renewed review. Preserve explicitly selected retired-account spending separately. Add an interleaving regression that relocates the source while confirmation-history persistence is suspended.
In `packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/DashPayTabScreen.kt`:
- [SUGGESTION] packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/DashPayTabScreen.kt:337-345: Invalidate and reverify the source account when an identity is re-slotted
(existing thread: https://github.com/dashpay/platform/pull/5288#discussion_r4187974162)
The placement key fixes eventual cache invalidation, but there is also a window before Room delivers the changed placement. Native discovery updates the identity manager first; until the corresponding row reaches composition, tipAccountKey still matches the old result and the sheet can confirm that account as the selected identity's dedicated source. Reading the current prop at confirmation does not close this window. The native ownership check validates the account's current occupant, not the selected source identity, and deliberately permits an empty retired slot. Retaining that policy is reasonable, but this identity-selected flow should not silently substitute the retired pool. Keep the Room-key fix and validate the expected source identity/account association natively at submission. Cover relocation before the corresponding Room emission, separately from relocation after the sheet captures its argument.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ShieldedTipSubmission.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ShieldedTipSubmission.swift:28-32: Restore outstanding tip submissions before enabling another send
(existing thread: https://github.com/dashpay/platform/pull/5288#discussion_r4185823786)
Every reconstructed submission starts ready, and both apps retain their per-wallet guards only in memory. Restarting after a broadcast with an unresolved outcome therefore enables another confirmation without restoring the previous sending/uncertain state. Native recovery does not make this payment-intent-safe: PendingRedrive is armed after an ambiguous result returns, and even a surviving record reserves only the original transaction's notes. reserve_unspent_notes can select other available notes for a fresh transfer, allowing both payments to execute if the user repeats the apparently unfinished tip. Persist an unresolved submission marker by network and wallet before invoking native send, restore it before enabling payment, and reconcile it with shielded activity/recovery state. Add reconstruction tests with a new submissions owner over the same durable state; the current reopening tests exercise only the existing owner. The description identifies a pending maintainer decision, but the current implementation still has this limitation.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Track verified identity placement for tip-balance attribution — The restore path constructs ManagedIdentity::new(identity, spec.identity_index), so a historical placeholder zero becomes Some(0). Swift and Kotlin balance selectors consequently can attribute retained account-0 notes to an identity actually derived at index 5. Native seed checks reject preparation and spending while that incorrect identity occupies the slot, but do not verify the displayed balance. This is the concrete balance-display remainder explicitly separated into a follow-up in the author's response, not a new finding to reopen here.
- Follow-up: Track derivation-verification provenance and expose an identity-aware tip-account resolver through the native bindings, with an unverified presentation state and placeholder-restore coverage.
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
74a473f to
4451545
Compare
|
Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The configuration-locking, identity-replay, and changeset-merging fixes are present; three client-side suggestions remain concerning source-account association, restart recovery, and profile clearing. Static validation only: git diff --check passed, and no builds or tests were run. The supplied CI snapshot shows Kotlin and Swift schema/explorer checks passing, Rust workspace tests pending, and the dedicated Rust wallet job skipped.
🟡 3 suggestion(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This large, intricate diff directly changes shielded funds submission in packages/rs-platform-wallet-ffi/src/shielded_send.rs, viewing-key and dedicated-account handling in packages/rs-platform-wallet/src/wallet/shielded/tips.rs, and persisted profile migrations across SQLite, SwiftData, and Room. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 14% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:3593-3595: Preserve the shared wallet's profile-clear semantics during persistence
ManagedIdentity::set_dashpay_profile(None) explicitly clears the profile, sync_profiles emits that snapshot after confirming absence, and IdentityChangeSet::merge preserves None with last-write-wins semantics. This adapter instead leaves the previous profile row untouched; Kotlin's onPersistIdentityUpsert does the same. Although that interpretation predates this change, this PR adds owned-profile restoration that loads the retained row back into canonical ManagedIdentity state. A successfully cleared profile can consequently reappear after restart, including its cached shielded address. Apply an absent profile snapshot by deleting the cached owner profile and, on Swift, its owner-profile payment-address sidecar. If callers need a distinct unknown/no-update state, represent it explicitly rather than interpreting the authoritative snapshot differently in each adapter. Add a persist-profile, clear-snapshot, cold-restore regression for both hosts.
In `packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/ShieldedTipSheet.kt`:
- [SUGGESTION] packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/ShieldedTipSheet.kt:95-99: Invalidate and reverify the source account when an identity is re-slotted
(existing thread: https://github.com/dashpay/platform/pull/5288#discussion_r4190547321)
Confirmation copies tipAccount into sendAccount before history.confirm suspends on Dispatchers.IO to commit preferences. Discovery can relocate the selected identity from placeholder slot 0 to verified slot 5 during that suspension. The placement-keyed lookup can subsequently refresh, but it cannot replace the value already captured by this submission. Native send_shielded_tip receives only the numeric source account, and verify_tip_account_owner accepts the old account when slot 0 is empty or contains its correctly derived owner. With available notes, the payment can therefore spend account-0 funds while the UI presents the source as the selected identity's dedicated account. Wallet ownership makes deliberate retired-account spending valid, but does not establish that this stale identity-specific selection was deliberate. Carry the expected source identity into an identity-aware native submission operation and validate its verified account association before selecting notes. Add a regression that pauses confirmation-history persistence, relocates the identity, and verifies that resuming cannot spend the obsolete source.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ShieldedTipSubmission.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ShieldedTipSubmission.swift:28-32: Restore outstanding tip submissions before enabling another send
(existing thread: https://github.com/dashpay/platform/pull/5288#discussion_r4185823786)
Application ownership preserves the guard across navigation, but the wallet dictionary and submission status are entirely process-local. After termination during broadcasting or following an uncertain result, relaunch constructs a new submission in ready state without restoring the outstanding payment. Kotlin's application-owned submissions have the same boundary. Native recovery can reconcile or redrive the original transition, but reserve_unspent_notes selects available notes without deduplicating the recipient/amount intent; a fresh submission can use other notes and pay the recipient again. Persist an outstanding intent before invoking native send, restore it into an unresolved state before enabling another submission, and reconcile it against native activity before clearing the guard. The existing reopen tests retain the same submission owner; add a reconstruction regression using a new owner after an accepted-but-unconfirmed attempt.
|
Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The client-only account policy, persistence migrations, replay fixes, and owned-profile clearing are consistent with the intended design; no consensus or dependency-layering blockers were found. Three non-blocking issues remain: source-account association can change during asynchronous confirmation, unresolved submissions are forgotten after process restart, and Kotlin's new profile deletion crosses network boundaries. This was static verification only; the supplied exact-head CI snapshot still has Kotlin validation running and other checks pending or queued.
🟡 3 suggestion(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This large, intricate diff directly changes shielded key derivation and funds isolation in packages/rs-platform-wallet/src/wallet/shielded/tips.rs, payment submission in packages/rs-platform-wallet-ffi/src/shielded_send.rs, and persisted storage migrations across SQLite, SwiftData, and Room. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 14% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:1746-1747: Scope owned-profile deletion to the snapshot network
This new absent-profile branch calls `deleteProfilesByIdentity`, whose SQL filters only by identityId. However, DashpayProfileEntity is keyed by `(networkRaw, identityId)`, the present-profile branch writes using the resolved networkRaw, and restoration reads profiles using that network-qualified key. The example app shares one DashDatabase across network managers. If the same identity ID has cached profile rows on two networks, an authoritative absence on one network deletes both rows, including the other network's profile and payment addresses. Add a network-scoped DAO deletion and use the networkRaw already resolved for this snapshot. Extend the clearing regression with a second-network profile row and assert that it survives.
In `packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/ShieldedTipSheet.kt`:
- [SUGGESTION] packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/ShieldedTipSheet.kt:95-99: Invalidate and reverify the source account when an identity is re-slotted
(existing thread: https://github.com/dashpay/platform/pull/5288#discussion_r4190547321)
The placement-keyed lookup refreshes subsequent compositions, but this submission captures only a numeric source account before `history.confirm` suspends on Dispatchers.IO. Discovery can move identity A from restored placeholder index 0 to verified index 5 during that suspension, and the resumed submission still passes account 0x40000000. Native `verify_tip_account_owner` accepts that account when slot 0 is empty, or when its correctly derived identity subsequently occupies it; the send boundary receives no selected source identity to detect that the account no longer matches A. Allowing explicitly selected retired accounts is valid, but it does not preserve this UI's promise to spend from the selected identity's dedicated account. Carry the selected source identity and expected account association into the native identity-specific send operation and reject a changed association before selecting funds, while preserving explicit retired-account spending separately. Add a regression that pauses confirmation persistence, re-slots A, and resumes the captured submission.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ShieldedTipSubmission.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ShieldedTipSubmission.swift:28-32: Restore outstanding tip submissions before enabling another send
(existing thread: https://github.com/dashpay/platform/pull/5288#discussion_r4185823786)
Both example apps keep submission guards exclusively in memory, so a new process creates an empty registry and initializes each wallet to ready. Termination after broadcast but before observing its outcome, or after an unconfirmed result, therefore removes the warning and permits an apparent retry as a new payment. Native activity/redrive records protect the original transaction, and note reservations exclude its selected inputs, but `operations::transfer` independently selects other available notes for a new call; neither mechanism deduplicates the user's payment intent. With sufficient additional notes, both transfers can execute. Persist an unresolved submission marker before entering native spending, restore it before enabling another send, and reconcile it against durable activity/recovery information or require explicit acknowledgement of the unresolved payment. Add a cold-recreation regression: the existing reopen tests retain the same application-owned guard and do not exercise process restart. The PR description's open maintainer decision does not establish an implemented recovery path or an agreed exclusion.
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The dedicated-account tip flow still has two wallet-correctness defects: source-account selection is not bound to the selected identity across asynchronous discovery/re-slotting, and unresolved submissions are forgotten after process restart. Restored placeholder indices can also misattribute another account's balance, while the FFI decoder accepts an unbounded length before copying. Rust and JS checks are queued, and Swift/Kotlin checks remain in progress; no completed changed-code failure is reported.
🔴 3 blocking | 🟡 2 suggestion(s)
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This large cross-cutting diff changes funds movement and coin selection, shielded key/account derivation, and storage migrations in files such as packages/rs-platform-wallet/src/wallet/shielded/tips.rs, packages/rs-platform-wallet-ffi/src/shielded_send.rs, and the V020 profile persistence migration. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/shielded/tips.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/tips.rs:115-153: Revalidate identity placement before publishing a prepared tip address
Preparation verifies the identity's index while holding the wallet-manager read guard, then releases that guard before acquiring `shielded_config_lock` and binding the account. Discovery can re-slot the identity in that interval, causing preparation to bind and return an address for the old index; the later profile write can publish an address that is no longer the selected identity's current dedicated account. Make placement verification and account binding one owner-level synchronized operation, or revalidate the identity's verified placement while holding the relevant placement/configuration synchronization.
In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay_profile.rs:38-40: Bound FFI address length before copying
`decode_address_update` copies `input.len` bytes before wallet validation. A malformed or buggy C/JNI caller can therefore request an arbitrarily large allocation, and values exceeding the platform's valid address sizes are copied before being rejected. Keep the exact field-specific 21-byte/43-byte semantics in `platform-wallet`, but impose a small transport-level maximum at the ABI boundary before calling `from_raw_parts`.
In `packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/ShieldedTipSheet.kt`:
- [BLOCKING] packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/ShieldedTipSheet.kt:95-100: Invalidate and reverify the source account when an identity is re-slotted
(existing thread: https://github.com/dashpay/platform/pull/5288#discussion_r4190547321)
The sheet captures the numeric tip account before the suspending history write, and the native API receives no expected source identity. In addition, `send_shielded_tip` verifies only the current occupant of that account before awaiting recipient resolution; discovery can move the selected identity during either window. If the old slot is empty, the retired account is accepted, and if another identity has been placed there, that identity's seed-derived key can satisfy the check, allowing the confirmed tip to spend from the wrong dedicated pool. Carry the selected identity or a Rust-owned source descriptor through the C/JNI/Swift/Kotlin APIs and revalidate the identity-to-account association immediately before selecting inputs.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ShieldedTipSubmission.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ShieldedTipSubmission.swift:10-32: Restore outstanding tip submissions before enabling another send
(existing thread: https://github.com/dashpay/platform/pull/5288#discussion_r4185823786)
The duplicate-send guard is stored only in the in-memory `wallets` map and each submission's status. After process death during or after an ambiguous native broadcast, a new app instance starts in `.ready` and can submit the same intended tip again; Kotlin has the same lifecycle. Rust's pending activity reservation covers the original input notes, not the payment intent, so a retry can select additional notes and both transfers can execute. Persist an unresolved tip intent before native submission, restore it on launch, and reconcile it with the original shielded activity or transition before allowing another send.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayProfileView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayProfileView.swift:39-44: Verify the identity index before selecting its tip spending account
(existing thread: https://github.com/dashpay/platform/pull/5288#discussion_r4185823798)
`getIdentityIndex()` returns persisted placement metadata, not proof that the restored identity belongs at that derivation index. A wallet-attached identity restored with the historical placeholder `Some(0)` is therefore mapped to account `0x40000000`, and the Swift and Kotlin selectors can display that account's notes as the selected identity's balance and pass it into the spending flow. The Rust send check validates the account's current wallet occupant, not the identity selected by the UI, and intentionally accepts empty retired accounts. Expose a Rust-owned verified identity-specific account lookup and keep the balance and spending action unavailable until placement has been seed-verified.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Make iOS FFI exports panic-safe under panic=abort — The new
catch_panic_to_codewrapper cannot contain panics in the workspace's iOSpanic = "abort"profiles. This is a workspace-wide limitation affecting the FFI crate's existing exports rather than a defect specific to this profile flow, and it is tracked separately.- Follow-up: Implement the tracked iOS FFI panic-safety work in #5673.
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The change introduces an unguarded Rust panic path in the new shielded-account snapshot C ABI export, where Tokio runtime and future panics can terminate native hosts instead of returning an error. The mobile tip selectors also continue to treat recorded identity indices as verified account ownership, and restore/deletion paths contain two additional state-integrity issues. CI reports no failed code check, but Rust, JS, and explorer checks remain queued while wallet tests are skipped by policy.
🔴 1 blocking | 🟡 3 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate change that modifies funds movement and coin selection, shielded key handling, native FFI boundaries, and SQLite/SwiftData/Room storage migrations, notably in packages/rs-platform-wallet/src/wallet/shielded/tips.rs and packages/rs-platform-wallet-ffi/src/shielded_send.rs. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/shielded_sync.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/shielded_sync.rs:750-769: Guard the shielded account-index snapshot C ABI entry point against Rust panics
This new extern "C" export calls `runtime().block_on` directly without the panic boundary used by the neighboring shielded query exports. A caller executing from an active Tokio runtime can trigger Tokio's nested-runtime panic, and a panic during runtime initialization or inside `get_wallet`/`shielded_account_indices` can unwind to the C ABI and abort the host. The Swift wrapper only checks the returned FFI result and cannot recover from a Rust panic. Wrap the complete export body in `catch_panic_to_code` or the shared query guard, and use the standard worker helper where its bounds permit, while preserving the existing output-pointer clearing.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:6654-6662: Validate persisted payment addresses during FFI restore
`profile_from_restore_row` trusts the host presence flags and copies all fixed-size address buffers directly into the canonical profile. A stale or corrupted SwiftData/Room row, or a mixed ABI consumer, can therefore restore an invalid transparent address or undecodable Orchard address even though profile fetches and outbound writes use the wallet validators. Apply the existing owner-level validators during restore, dropping invalid contact fields or returning a typed error for an owned profile, and cover invalid 21-byte and 43-byte restore values.
In `packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentIdentity.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentIdentity.swift:464-474: Do not swallow payment-address sidecar cleanup failures during identity deletion
The new payment-address sidecar rows are keyed by owner identity and are not deleted by SwiftData's identity relationship cascade. `PersistentIdentity.remove` ignores failures from `PersistentDashpayPaymentAddresses.removeOwned`, then deletes the identity and reports success. A fetch, migration, or storage failure can leave stale address rows behind after the identity is removed, allowing them to reappear in storage inspection or later restoration. Make sidecar cleanup part of a throwing deletion path and abort the parent deletion when it fails, or otherwise surface the failure instead of treating removal as successful.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayProfileView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayProfileView.swift:39-44: Verify the identity index before selecting its tip spending account
(existing thread: https://github.com/dashpay/platform/pull/5288#discussion_r4185823798)
`getIdentityIndex()` returns recorded placement metadata, not proof that the wallet seed currently associates this identity with that derivation slot. The Swift selector therefore maps a restored placeholder or stale index directly to a tip account and sums that account's notes as this identity's balance. Kotlin's profile and DashPay-tab selectors make the same inference from the live index lookup. Rust rejects some incorrect sends, but that does not prevent the UI from displaying another or retired account's balance or offering the wrong identity-specific source. Resolve the association through a Rust-owned seed-verifying operation, or keep the balance and source selector unavailable until placement is verified.
Out-of-scope follow-up suggestions (2)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Make shielded-tip submission restart-safe and identity-scoped — The Swift and Kotlin submission guards remain process-memory state, and the native send still accepts a source account without carrying the selected identity through the operation. A restart or a re-slot during recipient-history persistence can therefore lose the ambiguity guard or spend a wallet-owned account selected for another identity. This is a concrete lifecycle and source-account issue, but it requires the broader durable-intent and identity-scoped send redesign tracked separately.
- Follow-up: Implement durable intent/reconciliation and identity-scoped source-account validation in #5674.
- Make FFI panic containment effective for iOS panic=abort builds — The profile export and other guarded exports still cannot contain panics in the iOS profiles because the workspace sets
panic = "abort". This is a workspace-wide FFI policy issue affecting existing exports rather than a defect specific to the profile implementation in this PR.- Follow-up: Resolve the iOS FFI panic strategy and eliminate abort-reachable export panic paths under #5673.
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
github-actions
Bot
added
waiting-self-review
df1ad39 to
d56d393
Compare
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The head still contains a panic-safety defect in both new shielded-tip manager exports: wallet/coordinator and seed resolution occur before the Rust panic boundary, so calls from an active Tokio runtime can abort across the C ABI. The mobile balance selector also still trusts an unverified persisted identity index, and the Swift persistence callback can silently leave payment-address sidecars behind when its identity fetch fails. CI shows no failed code checks in the supplied snapshot, although several policy and runner checks remain pending.
🔴 1 blocking | 🟡 2 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate diff that changes shielded-wallet key handling and funds movement/coin selection while also introducing persistence migrations and extensive FFI boundaries, meeting the critical-surface bar. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/shielded_send.rs:2277-2286: Guard shielded-tip preparation and sending before manager resolution
Both new manager exports perform fallible preflight work before entering their Rust panic boundaries. `resolve_wallet_and_coordinator` calls `runtime().block_on`, so invoking `platform_wallet_manager_prepare_shielded_tip_address` from an active Tokio runtime can trigger Tokio's nested-runtime panic at lines 2277-2280; the send export has the same ordering at lines 2407-2416, and also resolves the seed before `catch_spend_panic`. That panic can unwind through the `extern "C"` boundary before JNI or Swift can translate it, aborting the host. Move wallet/coordinator and seed resolution, along with the rest of the operation-level preflight, inside `catch_query_panic`/`catch_spend_panic`, or use a resolver that performs the complete operation on a non-nested worker.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:3761-3769: Propagate failed identity fetches before deleting payment-address sidecars
The removal callback still uses `try?` when fetching the existing identity. A SwiftData fetch, migration, or storage failure is therefore treated the same as an absent row: the callback skips both the sidecar deletion and identity deletion, then returns success from the surrounding persistence operation. Rust has already removed the identity from its in-memory state, so the stale identity and published payment-address sidecar can be restored on the next launch. Let the fetch error propagate through the existing `do`/`catch` path instead of converting it to `nil`.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayProfileView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayProfileView.swift:37-44: Verify the identity index before selecting its tip spending account
(existing thread: https://github.com/dashpay/platform/pull/5288#discussion_r4185823798)
This still derives the displayed dedicated account directly from `getIdentityIndex()`, which returns recorded placement metadata rather than a seed-proven identity-to-account association. A restored identity can carry placeholder index 0 even when its derived placement is another index, so this view can attribute account `0x40000000`'s notes to the wrong identity and enable the corresponding balance/spend UI before discovery corrects the placement. The Kotlin profile screen has the same raw-index behavior at lines 84-87. Rust rejects many incorrect spends, but the balance remains misattributed and an empty retired slot is intentionally spendable; these selectors should remain unavailable until the wallet verifies the identity's derivation index.
…accounts Publish a reusable Orchard tip address on a DashPay profile and pay it by username. Wallet-generated tip addresses come from a dedicated ZIP-32 account per identity (0x40000000 + identity index), so tip viewing keys stay separate from ordinary wallet activity. Externally generated Orchard addresses can be published too. Carries the profile payment fields through the Rust wallet, SQLite storage (V019), the C FFI and JNI, SwiftData and Room (14 to 15), and the Swift and Kotlin example apps. Identity changeset replay now follows re-slotted identities, so the tip account follows its identity's verified index. BREAKING CHANGE: IdentityEntryFFI, ContactProfileRowFFI and the native profile structs gained payment address fields; C ABI consumers, the Swift xcframework and the Android JNI library must be rebuilt with the host. The Rust profile models gained fields. Migrated wallet databases cannot be opened by older SDKs. Squashed from 25 commits (previous head 74a473f) onto v5.1-dev. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Load now hydrates a wallet identity's cached profile back into Rust, so the Swift and Kotlin adapters' "absent snapshot means no update" reading let a profile deleted on Platform come back on every cold start. For a wallet-owned identity an absent snapshot now deletes the owner profile row (and the Swift payment-address sidecar). Out-of-wallet identities are never hydrated and keep the old behaviour. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Profile rows are keyed by (networkRaw, identityId) and restored by that key, so clearing one network's owned profile must not delete another network's row for the same identity id. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ntegration binary v5.1-dev builds the storage integration tests as one binary (#5397); a test file outside tests/integration/ would link the whole dependency graph again as a binary of its own. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Say what Keep and Remove mean on ProfileFields and that address bytes are sent unchecked, put the avatar test's doc comment back on its test, rename the storage tests that still named V019, and stop the V4 entity test claiming V4 changes no other entity. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…all owners The address-aware profile export ran `block_on_worker`, whose join error panics, straight under its `extern "C"` frame. It now runs under `catch_panic_to_code`, which moves from the shielded send module into `runtime` so builds without the shielded feature can use it. The Swift shielded-tip calls copied only the raw native handle into their detached tasks. `resolveShieldedTip` now keeps its wallet alive for the call, and the manager's prepare and send go through the shutdown admission gate, so teardown cannot destroy a handle a call is using. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e owners The profile FFI accepted only 21- or 43-byte addresses, a partial copy of the per-field check `validate_payment_address_update` already does; it now marshals the bytes and the wallet decides. The Swift profile write keeps its wallet alive through the detached native call, since `deinit` destroys the handle, and `clearAllData` deletes the payment-address rows, which identity deletion does not cascade to. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Both profile screens turned a failed tip-account lookup into a zero balance; they now say the balance is unavailable and keep the spend button disabled. In the Kotlin tip sheet the recipient-history write ran inside the native send's failure handling, so a failed preferences commit locked the sheet as an uncertain send; it now runs as a separate pre-send step whose failure returns the sheet to review. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ying it The wallet checks each field's exact length; the ABI now refuses anything over 64 bytes before allocating, so a bad C or JNI caller can't ask for an arbitrary copy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…its proved slot After the rebase onto #5487, placement no longer persists: load and discovery store the placed identity's snapshot with its verified keys in one changeset. Cover the adopted case, which now relies on that snapshot to carry the new wallet and index, and narrow VerifiedPlacement to the crate with the function that returns it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…shot export platform_wallet_manager_shielded_account_indices called runtime().block_on with no panic boundary, so a host calling from inside a Tokio runtime (or any panic in the lookup) unwound into the C frame and aborted. Run it under catch_query_panic, now shared from runtime.rs, and poll on a worker via block_on_worker. The outputs stay cleared on failure. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… the host profile_from_restore_row copied the host's payment-address buffers as-is, so a stale or corrupt SwiftData/Room row could restore an address a profile fetch would reject. Apply the wallet's transparent and Orchard validators on restore, as the avatar URL already is. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…an't be removed PersistentIdentity.remove swallowed the sidecar cleanup error and deleted the identity anyway, stranding address rows. It now throws without deleting. The example app saves the row deletes before touching the Keychain and rolls back on failure, so a failed removal no longer leaves a keyless identity behind. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…panic boundary The prepare and send tip exports resolved the wallet, coordinator and seed before entering their panic boundaries. Resolution blocks on the runtime, so a host calling from inside a Tokio runtime panicked across the C ABI and aborted. Prepare now resolves inside its query boundary; send resolves under a retryable preflight boundary, since nothing is spent yet, and keeps the do-not-retry contract for the spend itself. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…fetch fails The removal callback read a failed fetch as an absent row, skipping the identity and payment-address deletes while reporting success. Let the error fail the changeset. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
e1cce9f to
279719e
Compare
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the combined reviewer evidence and all 31 prior findings against head 279719e. Most prior fixes are present, but four newly introduced submission/account-association defects and two mobile ownership violations remain; tracking the defects in #5674 does not satisfy the supplied deferral policy. This was static verification only: current exact-head CI shows Rust workspace, Kotlin and JavaScript tests passing, with the Swift build pending and the dedicated wallet test job skipped.
🔴 6 blocking
4 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This large, intricate diff directly changes funds movement in rs-platform-wallet-ffi/src/shielded_send.rs, ZIP-32 account and viewing-key handling in rs-platform-wallet/src/wallet/shielded/tips.rs, and persisted-data migrations in rs-platform-wallet-storage/migrations/V020__profile_address_encoding.rs. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/tokens/ShieldedTipRecipientHistory.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/tokens/ShieldedTipRecipientHistory.kt:15-26: Keep recipient-confirmation history policy in the Rust wallet
This SDK class defines the canonical recipient-history key, decides whether a confirmed username's identity/address changed, and advances the confirmed record. Swift independently implements the same policy in `PlatformWallet/ShieldedTipRecipientHistory.swift`; the Rust wallet only resolves and rechecks the current submission. The book assigns DashPay orchestration to `rs-platform-wallet` and limits mobile SDKs to persisting Rust-emitted state, loading it for views and invoking Rust operations. Move the confirmed-recipient record and compare/confirm operations into the Rust DashPay owner, expose the change indication through the bindings, and let the hosts persist the emitted record and render the warning. Keep policy tests in Rust and storage round-trip tests in each adapter.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ShieldedTipSubmission.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ShieldedTipSubmission.swift:72-81: Expose retry disposition from the Rust tip operation
This allow-list decides when the payment guard may release and permit another submission, while Kotlin independently maintains that decision in `ShieldedTipFailure.kt`. Native `map_spend_result` already distinguishes ambiguous spends from definite failures, but each host reconstructs a retry disposition from transport error categories, including the generic wallet-operation category, rather than consuming one shared wallet-owned decision. This is payment lifecycle policy, not just error-message rendering, and the project's mobile boundary assigns such decisions to Rust. Expose the operation's retry disposition from `rs-platform-wallet`, preserve it through the ABI, and have both apps render it. Host failures known to occur before native invocation can be represented explicitly as not-started; unknown transport outcomes must remain unresolved.
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ShieldedTipSubmission.swift:25-32: Restore outstanding tip submissions before enabling another send
(existing thread: https://github.com/dashpay/platform/pull/5288#discussion_r4185823786)
Every new process constructs this guard in `.ready`; neither the Swift wallet map nor Kotlin's equivalent restores an outstanding submission. After an accepted-but-unconfirmed payment and process termination, the user can therefore repeat the intended payment as a new transfer. Native `reserve_unspent_notes` fences selected nullifiers, not the payment intent, so additional notes can fund the second transfer while the original executes or is redriven. #5674 documents the problem, but this flow introduces the duplicate-payment path, so the tracking issue does not make the deferral reasonable under the review policy. Implement durable intent state and reconciliation in `rs-platform-wallet`, persist and restore that state through the hosts, and test process recreation with an unresolved original transfer and additional spendable notes.
In `packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/ShieldedTipSheet.kt`:
- [BLOCKING] packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/ShieldedTipSheet.kt:95-100: Invalidate and reverify the source account when an identity is re-slotted
(existing thread: https://github.com/dashpay/platform/pull/5288#discussion_r4190547321)
`sendAccount` is captured before `history.confirm` suspends on Dispatchers.IO. If discovery moves selected identity A from placeholder slot 0 to verified slot 5 during that write, submission still sends from account `0x40000000`. Rust's `verify_tip_account_owner` accepts an empty slot, or verifies replacement identity B when B occupies slot 0; neither case establishes that this is A's selected dedicated account. Wallet ownership makes intentional retired-account spending valid, but does not make silently substituting that pool for A's account correct. Carry the expected source identity into the native operation and verify its association before note selection, retaining a separate explicit account-based route for retired funds. #5674 tracks this new defect but does not resolve it.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayProfileView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayProfileView.swift:39-44: Verify the identity index before selecting its tip spending account
(existing thread: https://github.com/dashpay/platform/pull/5288#discussion_r4185823798)
A successful index lookup returns recorded metadata, not a verified association. FFI restoration constructs `ManagedIdentity::new(identity, spec.identity_index)`, so an identity belonging at index 5 but restored with placeholder 0 returns `Some(0)` here and displays account 0's notes as its dedicated balance. Kotlin uses the same selection mechanism, and the native placeholder regression already constructs this state. The send-side seed check and unavailable-on-error display fix do not correct this successful-but-unverified lookup. Rust can retain placement provenance, return an unverified result after restore, and promote it after derivation-backed load, discovery or preparation; the views need not access the mnemonic on every render. Use that identity-scoped result in both hosts. The remaining misreport is introduced by this feature and cannot be closed merely by linking #5674.
In `packages/rs-platform-wallet/src/wallet/shielded/tips.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/tips.rs:139-153: Revalidate identity placement before publishing a prepared tip address
(existing thread: https://github.com/dashpay/platform/pull/5288#discussion_r4416638323)
The manager read guard ends before acquiring `shielded_config_lock`, and discovery/adoption does not acquire that configuration lock. A relocation after the verified read can therefore leave preparation persisting, binding and returning the captured former account without checking the identity's current placement. Seed verification rejects an ordinary wrong-placeholder case, but it does not make placement immutable: an identity can contain keys derived at multiple identity indices, and discovery adopts it at each matching index. The returned address also carries no association provenance for the separate profile write to validate. Coordinate placement validation with account installation in the wallet owner and reject obsolete wallet-generated publication; add an interleaving regression with a placement that changes after the initial verification. #5674 is tracking rather than a fix for this newly introduced stale-association path.
| fun hasChanged( | ||
| network: Network, walletId: ByteArray, username: String, recipient: ShieldedTipRecipient, | ||
| ): Boolean { | ||
| val previous = preferences.getString(key(network, walletId, username), null) ?: return false | ||
| return previous != snapshot(recipient) | ||
| } | ||
|
|
||
| /** Call only after explicit confirmation, before submitting the payment. */ | ||
| suspend fun confirm( | ||
| network: Network, walletId: ByteArray, username: String, recipient: ShieldedTipRecipient, | ||
| ) = withContext(Dispatchers.IO) { | ||
| check(preferences.edit().putString(key(network, walletId, username), snapshot(recipient)).commit()) { |
There was a problem hiding this comment.
🔴 Blocking: Keep recipient-confirmation history policy in the Rust wallet
This SDK class defines the canonical recipient-history key, decides whether a confirmed username's identity/address changed, and advances the confirmed record. Swift independently implements the same policy in PlatformWallet/ShieldedTipRecipientHistory.swift; the Rust wallet only resolves and rechecks the current submission. The book assigns DashPay orchestration to rs-platform-wallet and limits mobile SDKs to persisting Rust-emitted state, loading it for views and invoking Rust operations. Move the confirmed-recipient record and compare/confirm operations into the Rust DashPay owner, expose the change indication through the bindings, and let the hosts persist the emitted record and render the warning. Keep policy tests in Rust and storage round-trip tests in each adapter.
source: gpt-6.1-sol (phase2-reviewer: architecture-layering)
| static func canReviewAfterFailure(_ error: Error) -> Bool { | ||
| guard let error = error as? PlatformWalletError else { return false } | ||
| switch error { | ||
| case .nullPointer, .invalidHandle, .invalidParameter, .invalidIdentifier, .invalidNetwork, | ||
| .walletOperation, .identityNotFound, .contactNotFound, .utf8Conversion, | ||
| .noSelectableInputs, .shieldedBroadcastFailed, .shieldedNoRecordedAnchor, | ||
| .shieldedInsufficientBalance: | ||
| return true | ||
| default: | ||
| return false |
There was a problem hiding this comment.
🔴 Blocking: Expose retry disposition from the Rust tip operation
This allow-list decides when the payment guard may release and permit another submission, while Kotlin independently maintains that decision in ShieldedTipFailure.kt. Native map_spend_result already distinguishes ambiguous spends from definite failures, but each host reconstructs a retry disposition from transport error categories, including the generic wallet-operation category, rather than consuming one shared wallet-owned decision. This is payment lifecycle policy, not just error-message rendering, and the project's mobile boundary assigns such decisions to Rust. Expose the operation's retry disposition from rs-platform-wallet, preserve it through the ABI, and have both apps render it. Host failures known to occur before native invocation can be represented explicitly as not-started; unknown transport outcomes must remain unresolved.
source: gpt-6.1-sol (phase2-reviewer: architecture-layering)
Issue being fixed or feature implemented
Let a DashPay user publish a reusable shielded tip address and receive payments by username. Wallet-generated addresses use a dedicated ZIP-32 account per identity, separating tip viewing keys from ordinary wallet activity. Users can also publish an externally generated Orchard address.
This follows the transparent profile-address work in #4380 and the 43-byte format in dashpay/dips#188.
Split (2026-09-16): the contract field shipped separately in #4768 and the DPNS resolver fix in #4769; both are in
v5.1-dev. This PR now carries only the wallet, persistence, native bindings, and Swift/Kotlin example flows, and targetsv5.1-dev(milestone v5.1.0). Nothing that remains here is consensus code.What was done?
profile.shieldedAddresson DashPay v2 at position 7, 43 raw bytes including the full diversifier; consensus enforces the byte length, clients validate the Orchard decoding) landed in feat(dpp)!: dashpay profile shielded address field #4768 together with the protocol 13 to 14 upgrade tests and the fee/root fixtures. Since the 2026-10-05 merge ofv5.1-devthose files are no longer in this diff.0x40000000 + index. Preparation verifies the seed, binds viewing keys, and flushes persistence before returning an address. Identity discovery reconstructs the account without relying on the current profile; retired tip accounts keep scanning. Ordinary balance/spending choices exclude tip accounts, with explicit access to tip funds.How Has This Been Tested?
Local macOS builds and targeted validation:
StateFlow.valuecomposition finding inSyncStatusScreen.kt:215.git diff --checkpasses. Storage explorer coverage passes for all 36 models, with a negative fixture verifying inherited-model omissions are detected.GitHub skips Rust and mobile runner jobs for this fork under the existing workflow trust policy; the results above are local validation.
Not exercised: a live funded network tip transaction, Android NDK/native
.sobuild, or Android device/instrumented execution. Full network end-to-end validation remains a release validation step.Breaking Changes
Client-side only: there is no consensus or protocol change here (the protocol-14 DashPay v2 contract bytes and their fee/root baselines shipped in #4768). The client changes are breaking:
#[repr(C)]restore structsIdentityEntryFFIandContactProfileRowFFIgained the three payment addresses and their presence flags (IdentityEntryFFIgrows from 224 to 312 bytes), and the native profile/update structs widened too. Every C ABI consumer, the Swift xcframework, and the Android JNI library must be rebuilt together with the host code; mixing old and new binaries misreads these structs. The existing FFI profile setter stays as a wrapper with keep-address semantics.DashPayProfile, the profile patch types) gained fields.rs-sdk'sProfileFieldsgainedcore_payment_address,platform_payment_addressandshielded_address, andPaymentAddressUpdatenow lives indash_sdk::platform::dashpay(platform-walletre-exports it).PersistentIdentity.remove(in:identityId:)nowthrows, so a failure to remove the identity's payment-address rows is reported instead of swallowed.Checklist:
For repository code-owners and collaborators only
This pull request was created by Codex.
Fix-up (2026-09-16, maintainer push)
Merged
v4.2-dev(ffb6e53f20) into the branch and addressed the open review threads; head = merge47e9e1bc95+a637a0e2bd.v4.2-devalready ships V008 to V018. Theentry_format/profile_formatstamps nowDEFAULT 1and the migration marks the rows it finds0, so only pre-V019 rows are decoded as the legacy shape; both identity readers on the hard-delete schema dispatch on the stamp.tests/sqlite_profile_address_encoding.rswalks a V007 database through the whole chain. SCHEMA.md documents both columns.scripts/freeze_schema_models.py, FREEZES row at787cac09e7) instead of a hand-written copy;DashSchemaV5addsPersistentDashpayPaymentAddresses;dash-v5.storewritten by this build for the fixture-based hash test from feat(swift-sdk): generate the frozen SwiftData schema models, and guard them with real stores #4644.catch_query_panicaround the non-spending tip exports plus an entry-point-level panic regression test; app-ownedShieldedTipSubmissionson iOS (parity with Android); Kotlin tip account derived from the live identity index rather than Room's placeholder 0.Verified locally: storage crate suite, wallet tip/profile/bind tests (87), FFI panic tests, Drive check-tx/document/root-hash tests, Swift package persistence tests (18) and the example app build with warnings as errors, Kotlin SDK/app compile and tip unit tests.
Update (2026-10-05, maintainer push)
Merged
v5.1-dev(1ebcedb028): the #4768/#4769 copies dropped out (143 to 99 files). Persistence numbering moved to follow the base:v5.1-devships versions 12 to 14, so the profile address columns are nowMIGRATION_14_15(database version 15,15.json); migration tests walk 14 to 15 and 10 to 15.v5.1-devreplaced the development V4/V5 chain with App Store release snapshots (feat(swift-sdk)!: freeze schemas only after App Store publication #4818, fix(swift-sdk)!: restore historical V2 migration to live V3 #4910) and published schema 3.0.0. The hand-registered V4 freeze and V5 from the earlier fix-up are gone:DashSchemaV3is bound to its generated release snapshot and a new liveDashSchemaV4addsPersistentDashpayPaymentAddresses(V2 to V3 to V4, and V1 to V3 to V4). SCHEMA_RELEASES.md lists the routes.Also addressed in the remaining review threads: the tip resolver releases the wallet registry lock before its network lookups; Kotlin ties the tip account to the identity it was resolved for; Swift identity deletion paths remove payment-address rows; literal-bytes coverage for the pre-V019 profile record.
Update (2026-10-29, rebase onto
v5.1-dev03945d3aa7)The base moved under three parts of this PR:
rs-sdk(feat(sdk): move DashPay profile create, update and fetch into rs-sdk #5470). The wallet no longer builds and merges profile documents itself, so the payment addresses riders-sdk'sProfileFieldsnow:PaymentAddressUpdate(keep/set/remove) moved there,ProfileFields::overlayapplies it on create and on the read-modify-write update, and the wallet'smerge_profile_propertiesis gone (its tests moved tors-sdk). Tip resolution fetches throughSdk::get_profiles.V020__profile_address_encoding.rsand RoomMIGRATION_15_16(database version 16,16.json; migration tests walk 15 to 16 and 10 to 16). The base also introduced the same live SwiftData V4 for its asset-lock change;PersistentDashpayPaymentAddressesis added to that V4, which is still unpublished (schema-releases.jsonlists 3.0.0 only).PlatformWallet::newgained two arguments; the PR's tests pass them.16.jsonwas written from the base15.jsonplus this PR's two profile tables, with the Room identity hash computed by a script that reproduces the recorded hashes of versions 12 to 15; Gradle could not run locally, so CI regenerates and checks it.Verified locally: clippy (
-D warnings, all targets and features) ondash-sdk,platform-wallet,platform-wallet-storage,platform-wallet-ffiandrs-unified-sdk-jni; all tests of the four wallet crates (1646 wallet lib, 557 storage integration);rs-sdkDashPay tests. Swift and Kotlin were not built locally.Update (2026-10-30, rebased onto
v5.1-deveaa7602a8e)add_identity_unstored) and store it with its verified keys in one changeset. This PR'splace_verified_identityfollows that: it no longer persists on its own,add_identity_unstoredcarries the occupied-slot refusal, and an adopted identity's new wallet and index reach the store in the same changeset as its keys (covered by a new load test).Verified locally:
cargo fmt, the wallet CI clippy command (wallet crates plusrs-unified-sdk-ffi/-jni, all targets and features,-D warnings), and all tests of the three wallet crates (1712 wallet lib, 567 storage integration, 496 FFI). Swift and Kotlin were not built locally.🤖 Generated with Claude Code
PR Hygiene ·
279719e/self-reviewedkotlin-sdk(packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/di/AppContainer.kt,packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/DashPayJson.kt,packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/DashPayProfileScreen.ktand 29 more) — HashEngineeringrs-platform-wallet-ffi(packages/rs-platform-wallet-ffi/src/dashpay_profile.rs,packages/rs-platform-wallet-ffi/src/identity_persistence.rs,packages/rs-platform-wallet-ffi/src/persistence.rsand 5 more) — HashEngineering or ZocoLini or llbartekll or romchornyiwallet-storage(packages/rs-platform-wallet-storage/SCHEMA.md,packages/rs-platform-wallet-storage/migrations/V020__profile_address_encoding.rs,packages/rs-platform-wallet-storage/src/sqlite/mod.rsand 10 more) — lklimekrs-platform-wallet(packages/rs-platform-wallet/docs/SHIELDED_TIPS.md,packages/rs-platform-wallet/src/changeset/changeset.rs,packages/rs-platform-wallet/src/error.rsand 18 more) — HashEngineering or ZocoLini or llbartekll or romchornyirust-sdk(packages/rs-sdk/src/platform/dashpay/mod.rs,packages/rs-sdk/src/platform/dashpay/profile.rs) — lklimek or shumkovpackages/rs-unified-sdk-jni/src/dashpay.rs,packages/rs-unified-sdk-jni/src/funding.rs,packages/rs-unified-sdk-jni/src/persistence.rsand 2 more) — QuantumExplorer or shumkovswift-sdk(packages/swift-sdk/SCHEMA_RELEASES.md,packages/swift-sdk/Sources/SwiftDashSDK/Persistence/DashLegacyStoreSQLite.swift,packages/swift-sdk/Sources/SwiftDashSDK/Persistence/DashModelContainer.swiftand 36 more) — llbartekll or romchornyiWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.