feat(platform-wallet): own the DashPay startup ordering instead of each client - #4359
Conversation
First piece of moving the DashPay startup ordering out of the clients. A contact's DIP-15 addresses come from its contact account, and an address that is not watched when the filter scan passes its funding height yields no transaction — so the bring-up order decides whether a restored wallet has contact payment history, and the client has to be able to hold Core SPV back until the addresses exist. That is a policy decision, so it belongs here rather than reimplemented per client. iOS had it in Swift; Android would have had to write it again. This commit is the classification alone — no I/O, no SDK — so the rule that already regressed once in a client is pinned by tests before the async body exists: a scan that came back definitively empty settles as `NoIdentity` and is never retried, while a scan that never reached Platform reports `PartialNoIdentity` and is. platform#4352 is what made those two distinguishable at all; this is the first consumer of that distinction. `StartupTally` mirrors `ScanTally` in discovery.rs deliberately: the counters live in a type the tests can drive through the same methods production uses, so a later miswiring fails a test instead of shipping.
`start_wallet_subsystems` brings one wallet's DashPay state up in dependency order so the caller can start Core SPV against a complete contact-address set, instead of scanning first and repairing after. Three steps, each a precondition of the next: 1. Identity. Skipped entirely when one is already known locally, so a warm launch costs no network round trip. Otherwise a scan, retried ONLY on `IdentityDiscoveryIncomplete` — a scan that returned has an answer from Platform, and an empty answer is a proof of absence that rescanning cannot overturn. 2. One contact-request pass, so the deferred builds exist to drain. Log-and-continue: a prior session may have queued work this call can still finish. 3. The drain. This is the step whose absence made the ordering pointless elsewhere — after a sync pass the contact accounts still do not exist, because the unattended sweep holds no signer and can only enqueue. Without it SPV would start with nothing extra to watch. Never returns an error except `WalletNotFound`. An unreachable Platform, a failed sync and an unfinished drain are all reported in the outcome, because failing loudly would trade a data gap for a wallet with no balance — the worse of the two. The budget is a parameter defaulting to 20s. Key material stays per-call: the master xpriv and contact-crypto provider are borrowed for this call only, matching what `discover_from_master` and the drain already require. Making the recurring sweep resolve its own signer would remove the need for any of this, but it would turn a narrowly-scoped Keychain capability into a standing one, and the auto-accept half needs a full identity signer — a security-posture change, not a refactor.
One call a host makes at wallet load, immediately before starting Core SPV, replacing the step sequencer each client would otherwise hand-roll. `run_on_big_stack_thread` rather than `block_on_worker`: the manager is only reachable as a `&PlatformWalletManager` borrowed from the handle store, so the future cannot satisfy `block_on_worker`'s `'static` bound. The scoped thread also supplies the 8 MB stack the GroveDB proof verification inside discovery needs — the same reason discovery's own FFI avoids the calling thread's stack. The master xpriv is resolved once up front and erased with `non_secure_erase` before every return path; `ExtendedPrivKey` has no `Drop`, so this is the same explicit hygiene `platform_wallet_discover_identities` already performs. Both handles follow the established per-call contract — borrowed for the duration, never retained — and the identity signer stays nullable, where null skips the DIP-15 auto-accept pass exactly as it does for `platform_wallet_drain_pending_contact_crypto`. `budget_secs = 0` means the crate default, deliberately not "unbounded": this call gates Core SPV, so it must always terminate. Only an invalid handle or unknown wallet id return an error — every partial outcome is reported in the out-struct so the host can start Core SPV regardless. Adds `wallet_network_blocking` on the manager: the network is needed to build the key material before the sequence can run, and a caller holding a manager handle has no wallet handle to read it from.
`startWalletSubsystems(wallet:budget:gapLimit:)` — one awaitable call a host makes before `startSpv`, replacing the step sequencer each client would otherwise hand-roll. The ordering, the retry policy and the budget all stay Rust-side; this marshals arguments and maps the outcome. `WalletStartupStatus` carries `identityIsSettled`, false only for `partialNoIdentity`, so a caller never has to re-derive which outcomes are worth repeating. An unrecognised discriminant decodes to that same case: the conservative reading, since it is the one status that does not claim the identity question is answered. The resolver and identity signer are built per call and pinned with `withExtendedLifetime` — Rust borrows and never retains them, the same contract `unlockWalletFromKeychain` and `discoverIdentities` use. `ensureConfigured`, `modelContainer` and `signerNetwork` go from private to internal so this extension reuses them instead of duplicating the gate and the signer construction in a second file.
|
✅ Final review complete — no blockers (commit 58a43aa) |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesAdds ordered wallet startup across the Rust manager, Rust FFI, and Swift SDK. The flow resolves or discovers identity, synchronizes contact requests, drains deferred contact-account work, and returns structured startup outcomes. Wallet startup orchestration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SwiftDashSDK
participant RustFFI
participant PlatformWalletManager
participant IdentityDiscovery
participant ContactRequestSync
SwiftDashSDK->>RustFFI: Start wallet subsystems
RustFFI->>PlatformWalletManager: Pass startup options and signing state
PlatformWalletManager->>IdentityDiscovery: Discover or load local identity
PlatformWalletManager->>ContactRequestSync: Synchronize contact requests and drain deferred work
ContactRequestSync-->>PlatformWalletManager: Return startup counts and status
PlatformWalletManager-->>RustFFI: Return WalletStartupOutcome
RustFFI-->>SwiftDashSDK: Map and return outcome
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/rs-platform-wallet/src/manager/startup.rs (1)
409-418: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider a distinct signal for a non-network discovery failure.
This arm handles a wallet or persistence failure, then calls
record_discovery_gave_up, which setsdiscovery_unreachable.status()then returnsPartialNoIdentity, andidentity_is_settled()returnsfalse. The client reads that as "we never reached Platform" and schedules another scan, although the comment states the failure will not fix itself on a retry.Adding a terminal-failure flag to
StartupTallykeeps the retry advice correct for this case. If you keep the current mapping, state onPartialNoIdentitythat it also covers a local failure, so a client does not treat it as purely a reachability signal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/manager/startup.rs` around lines 409 - 418, Distinguish terminal local discovery failures from unreachable discovery in StartupTally: update the Err(e) branch of startup discovery to record a terminal-failure state instead of only calling record_discovery_gave_up, and make status() and identity_is_settled() expose that state so clients do not schedule another scan. If retaining PartialNoIdentity, document and handle its local-failure meaning so it is not treated as purely a reachability signal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet-ffi/src/wallet_startup.rs`:
- Around line 156-161: Update start_wallet_subsystems to select a resident-key
contact-crypto provider when mnemonic_resolver_handle is null, instead of always
calling resolver_contact_crypto_provider. Preserve
resolver_contact_crypto_provider for non-null resolver handles, and ensure the
selected provider supports draining pending contact crypto for resident-key
wallets.
In `@packages/rs-platform-wallet/src/manager/startup.rs`:
- Around line 308-357: Enforce the remaining startup budget around
sync_contact_requests, drain_pending_contact_crypto, drain_auto_accepts, and
pending_contact_crypto_count using tokio::time::timeout, preserving
log-and-continue behavior when the sync or drain operations time out. Record
record_drain(0, pending_before) for a timed-out drain and avoid unbounded
follow-up calls; update the PartialAccountsPending documentation so it no longer
implies that pending work proves the budget expired.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerStartup.swift`:
- Line 119: Guard the budget conversion in the startup flow around budgetSecs so
negative, NaN, and infinite values cannot reach the UInt64 initializer; clamp or
otherwise normalize them to a safe finite non-negative value. Preserve the FFI
contract while ensuring any positive budget below 0.5 seconds does not round to
zero and unintentionally select the 20-second default.
---
Nitpick comments:
In `@packages/rs-platform-wallet/src/manager/startup.rs`:
- Around line 409-418: Distinguish terminal local discovery failures from
unreachable discovery in StartupTally: update the Err(e) branch of startup
discovery to record a terminal-failure state instead of only calling
record_discovery_gave_up, and make status() and identity_is_settled() expose
that state so clients do not schedule another scan. If retaining
PartialNoIdentity, document and handle its local-failure meaning so it is not
treated as purely a reachability signal.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 43b150f5-0097-4d41-a515-a106d9d4469a
📒 Files selected for processing (7)
packages/rs-platform-wallet-ffi/src/lib.rspackages/rs-platform-wallet-ffi/src/wallet_startup.rspackages/rs-platform-wallet/src/manager/accessors.rspackages/rs-platform-wallet/src/manager/mod.rspackages/rs-platform-wallet/src/manager/startup.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerStartup.swift
… no provider Three findings from review on #4359. **The budget bounded only discovery.** `sync_contact_requests` and both drains ran without a deadline, so a stalled DAPI endpoint could hold the call — and therefore Core SPV, which this gates — well past `budget`, the exact outcome the module docs promise to avoid. Every network step now runs through `within_budget`. Abandoning one is safe by construction: work a drain did not finish stays queued and the next signer-present action retries it. The queue-length read stays unbounded on purpose — it is local, and its answer matters most precisely when the steps above ran out of time. **A resident-key wallet drained into a provider that could not work.** The provider is resolver-backed, so with a null resolver every crypto operation fails with `NullHandle`: the drain reported zero while leaving the queue untouched, which reads as "nothing to do" rather than "could not try". `contact_crypto` is now `Option`, the FFI passes `None` when there is no resolver, and the sequence skips the drain and reports the real pending count. **The Swift budget conversion could trap.** `UInt64(x.rounded())` traps on a negative, NaN or infinite value, reachable whenever the budget comes out of arithmetic — now rejected with a typed error. A positive budget under half a second also rounded to `0`, which the FFI reads as "use the default", handing the caller the longest budget where they asked for the shortest; clamped to one second instead. Also corrects the `PartialAccountsPending` doc, which claimed the budget had expired. A non-zero pending count no longer implies that — the drain may have failed on some entries, or been skipped for want of a provider. Three `within_budget` tests on a paused clock cover the gap that let the first finding through.
|
One CodeRabbit review-body finding still appears valid on Please distinguish this terminal local-discovery failure from an unreachable Platform (or explicitly change/document the status contract) so clients do not schedule a futile rescan. |
…reachable Platform Review follow-up on #4359. The non-network `Err` arm called `record_discovery_gave_up()`, which maps to `PartialNoIdentity` — the one status that asks the client to scan again. Its own comment says the opposite: a wallet-manager or persistence fault "will not fix itself on the next attempt". Clients were being sent on a rescan guaranteed to hit the same fault. New terminal `DiscoveryFailed` status for that case. It outranks unreachability in the classification, since both leave the identity question open but only this one settles whether asking again is worth it. That distinction did not fit the existing predicate, so there are now two, and they are deliberately not inverses: - `discovery_worth_retrying()` — true only for `PartialNoIdentity`. Answers "should I scan again?" - `identity_is_settled()` — false for `PartialNoIdentity` AND `DiscoveryFailed`. Answers "do we know?" `DiscoveryFailed` is the case that needs both: unanswered, yet not worth retrying. Collapsing them into one predicate is what produced the bug. FFI discriminant 4, appended so existing values are untouched.
Follows the SDK gaining a terminal `discoveryFailed` status (dashpay/platform#4359): a wallet or persistence fault, as opposed to an unreachable Platform. Logged at error rather than warning because it is the one outcome here that nothing will clear on its own — not this session, not the next launch.
|
Good catch — fixed in 235620b, and you are right that it contradicted its own comment. The non-network That case did not fit the single predicate, which is what let the bug through, so there are now two — deliberately not inverses:
FFI discriminant appended as 4, existing values untouched. The iOS caller handles it at error level (dashpay/dashwallet-ios#961). |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The ordered startup flow is directionally sound, and the previously reported sync/drain timeout issue is fixed at the exact head. Six in-scope correctness issues remain: readiness can be overstated, discovery can exceed the deadline, extreme budgets can crash at both language boundaries, and two startup paths can misclassify or prematurely fail.
Source: reviewer backend model gpt-5.6-sol (general, rust-quality, and ffi-engineer lanes); final verifier backend model gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol was orchestration-only and is not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 6 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/manager/startup.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/startup.rs:363-365: A failed contact sync can still be classified as Ready
The tally records a successful sync whenever `sync_contact_requests` returns `Ok`, but that method catches received and sent fetch failures per identity and ultimately returns `Ok(all_requests)` even if Platform was unreachable for every identity. Explicit errors and timeouts leave `dashpay_sync_ran` false, but `StartupTally::status()` does not inspect that field either. If the existing deferred queue is empty, all three cases become `Ready`, even though no complete contact-document pass occurred and undiscovered requests may require account builds. This contradicts the `Ready` contract and can start SPV without the contact addresses this API exists to prepare. Return a completion summary from the sync operation and add a partial classification whenever any identity's contact fetch did not complete.
- [SUGGESTION] packages/rs-platform-wallet/src/manager/startup.rs:478-482: Identity discovery is not bounded by the startup deadline
Each discovery attempt is awaited directly instead of through `within_budget`. One attempt can perform up to `gap_limit` sequential Platform fetches and DPNS lookups, so it can continue well beyond `opts.budget`; after a backoff consumes all remaining time, the loop can also start another unbounded attempt with an already-expired deadline. This breaks the documented whole-sequence ceiling and can keep the synchronous FFI caller from starting Core SPV during a Platform outage. Bound the entire discovery future by the remaining deadline and stop immediately on timeout. Because discovery persists sightings incrementally, re-check wallet-local identities before reporting `PartialNoIdentity` after cancellation.
- [SUGGESTION] packages/rs-platform-wallet/src/manager/startup.rs:329-330: An oversized budget can abort the FFI host
`WalletStartupOptions::budget` is unrestricted, and the new C entry point converts every nonzero `u64` directly to `Duration`. `Instant + Duration` panics when the resulting instant is not representable, including sufficiently large valid `budget_secs` values. `run_on_big_stack_thread` then re-panics through `join().expect(...)`; because the caller is an `extern "C"` function, this can abort the host process. Construct the deadline with `checked_add` and return `InvalidParameter` for an unsupported duration, then update the method's error documentation accordingly.
- [SUGGESTION] packages/rs-platform-wallet/src/manager/startup.rs:484-490: An empty discovery return can misclassify a concurrently inserted identity
`IdentityWallet::discover` returns only identities newly inserted by that invocation, while its internal scan separately treats already-managed identity sightings as trustworthy. Two concurrent startup calls can both pass the initial local check; after one inserts the identity, the other sees it on Platform but finds it already managed, returns an empty vector, and records proven absence here. That caller then skips contact sync and reports `NoIdentity` even though the wallet now owns the identity. Re-check local state whenever discovery returns an empty vector before classifying the result as a proof of absence.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerStartup.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerStartup.swift:145-150: A large finite Swift budget traps during UInt64 conversion
The validation rejects negative and non-finite values, but a finite `TimeInterval` larger than the `UInt64` range still reaches the trapping `UInt64` initializer. For example, `Double.greatestFiniteMagnitude` crashes the caller instead of producing `PlatformWalletError.invalidParameter`. Use the failable exact conversion after rounding. Rust still needs its own independent deadline validation because many representable `UInt64` values cannot be added to an `Instant`.
In `packages/rs-platform-wallet-ffi/src/wallet_startup.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/wallet_startup.rs:146-155: Eager mnemonic resolution bypasses the startup outcome contract
The FFI resolves the master xpriv before Rust checks whether the wallet already has an identity locally. A Keychain race, access denial, missing item, or malformed mnemonic therefore returns an immediate FFI error even on a warm launch that requires no discovery and may have no deferred crypto work. This contradicts both the FFI and Swift documentation that only configuration, malformed-ID, invalid-handle, and unknown-wallet failures throw; sync and drain failures are supposed to become structured startup outcomes so Core SPV can proceed. Defer master resolution until discovery is actually required, while retaining the resolver-backed provider for drain operations that genuinely need it, or translate resolver failure into a structured partial outcome.
All from review on #4359. **Discovery ran outside the budget.** The previous round bounded the sync pass and both drains but left the scans themselves unbounded — and one scan walks up to `gap_limit` indices, each a Platform fetch plus a DPNS lookup. A Platform outage could hold Core SPV well past the ceiling this call advertises. Each attempt now goes through `within_budget`. **An absurd budget could abort the host.** `Instant + Duration` panics when the sum is unrepresentable, and the FFI thread wrapper re-raises a panic as an abort, so a large `budget_secs` took the process down instead of returning. Built with `checked_add`, `InvalidParameter` on overflow. Swift had the mirror problem one layer up: a finite `TimeInterval` can still be outside `UInt64`, so the conversion now uses `UInt64(exactly:)` — the two validations are independent because plenty of representable `UInt64` seconds still cannot be added to an `Instant`. **An empty scan is not always proof of absence.** `discover` reports only identities THAT call inserted, so two concurrent bring-ups can race: the second sees the identity on Platform, finds it already managed, returns empty, and would have recorded proven absence for a wallet that demonstrably owns one. Local state is now consulted before classifying an empty return, and again after an abandoned scan, since sightings persist incrementally. **A failed contact pass could still report `Ready`.** An empty queue only means "nothing left to build" if a pass actually completed; without one there may be undiscovered requests whose builds were never enqueued. `Ready` now requires `dashpay_sync_ran`. **Eager mnemonic resolution broke the error contract.** The FFI resolved the master xpriv before Rust checked for a local identity, so a Keychain hiccup failed a warm launch that needed no scan at all — while the docs promise only handle and wallet-id problems throw. Resolution failure is now a warning; if a scan does turn out to be needed it fails without key material and surfaces as `DiscoveryFailed`, which is the structured outcome for precisely this. Partial on the first point: `sync_contact_requests` swallows per-identity fetch failures internally and returns `Ok` regardless, so a pass where every identity failed still counts as ran. Fixing that means changing its signature to report completion, which is a wider change than this PR should carry — noted on the thread.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-platform-wallet/src/manager/startup.rs (1)
380-382:⚠️ Potential issue | 🟠 MajorDo not treat every
Okresult as a completed contact pass.
sync_contact_requestscan returnOkafter it catches per-identity fetch failures. Line 382 then records a completed pass, and an empty pending queue can produceReadyeven when contact requests were not fully synchronized. Return completion information from the sync operation, or retain a partial state when any identity fetch fails.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/manager/startup.rs` around lines 380 - 382, Update the startup contact-request sync flow around sync_contact_requests and tally.record_sync_ran so an Ok result is not automatically treated as a completed pass. Propagate completion status or retain a partial state when any per-identity fetch fails, and only record a completed sync when all contact requests were successfully synchronized, preventing an empty queue from producing Ready prematurely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet-ffi/src/wallet_startup.rs`:
- Around line 146-169: Move the resolve_master_from_resolver call out of the
initial startup setup and into the discovery path after start_wallet_subsystems
determines that scanning is required. Keep master absent for warm launches with
known local identity, and propagate resolver failures through the structured
DiscoveryFailed startup status rather than only logging a warning.
In `@packages/rs-platform-wallet/src/manager/startup.rs`:
- Around line 341-346: Document the unsupported startup-budget error in both
public API contracts: update the # Errors documentation associated with the
startup manager flow around checked_add in
packages/rs-platform-wallet/src/manager/startup.rs lines 341-346, and update the
Throws documentation for PlatformWalletManagerStartup in
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerStartup.swift
lines 150-161 to state that Rust may reject a representable but unsupported
budget; no code behavior changes are required.
---
Outside diff comments:
In `@packages/rs-platform-wallet/src/manager/startup.rs`:
- Around line 380-382: Update the startup contact-request sync flow around
sync_contact_requests and tally.record_sync_ran so an Ok result is not
automatically treated as a completed pass. Propagate completion status or retain
a partial state when any per-identity fetch fails, and only record a completed
sync when all contact requests were successfully synchronized, preventing an
empty queue from producing Ready prematurely.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dd2d457c-0dd8-4966-8f5f-a6edfe7a1510
📒 Files selected for processing (3)
packages/rs-platform-wallet-ffi/src/wallet_startup.rspackages/rs-platform-wallet/src/manager/startup.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerStartup.swift
…ot use Two follow-ups on #4359. The previous commit stopped a resolver failure from failing the call, but left the resolution itself happening unconditionally — so a warm launch still paid for a Keychain round trip whose result it would never use, and spent that time outside the budget it was about to be measured against. Key material is only ever needed for a scan, and a wallet with an identity on file does not run one. `has_local_identity_blocking` answers that from a single lock read, and the resolve is skipped when the answer is yes. Also documents the second error this call can now return. `checked_add` made `InvalidParameter` reachable for an unrepresentable budget, while the Rust, FFI and Swift docs all still promised that only missing-wallet and configuration problems throw. All three now say the same thing: errors are about the request, outcomes are about the run.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-platform-wallet/src/manager/accessors.rs (1)
467-474: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid allocating all identity IDs for a presence check.
wallet_identity_ids(wallet_id)clones every local identity ID into a newVecbefore checking whether it is empty. Use the existing wallet-scoped iterator and stop after the first item.Proposed change
- !info - .identity_manager - .wallet_identity_ids(wallet_id) - .is_empty() + info.identity_manager + .wallet_managed_identities(wallet_id) + .next() + .is_some()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/manager/accessors.rs` around lines 467 - 474, Update has_local_identity_blocking to use the existing wallet-scoped identity iterator instead of wallet_identity_ids, and determine presence by checking whether the iterator yields a first item. Preserve the current wallet lookup and boolean behavior while avoiding allocation of all identity IDs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/rs-platform-wallet/src/manager/accessors.rs`:
- Around line 467-474: Update has_local_identity_blocking to use the existing
wallet-scoped identity iterator instead of wallet_identity_ids, and determine
presence by checking whether the iterator yields a first item. Preserve the
current wallet lookup and boolean behavior while avoiding allocation of all
identity IDs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ea9033b-6a19-4a32-b4e1-021df1dcc0ad
📒 Files selected for processing (4)
packages/rs-platform-wallet-ffi/src/wallet_startup.rspackages/rs-platform-wallet/src/manager/accessors.rspackages/rs-platform-wallet/src/manager/startup.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerStartup.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerStartup.swift
- packages/rs-platform-wallet-ffi/src/wallet_startup.rs
- packages/rs-platform-wallet/src/manager/startup.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The latest commits correctly bound the Rust network operations, prevent deadline overflow, handle concurrent discovery insertion, and skip unnecessary warm-launch key resolution. Four in-scope correctness issues remain: readiness still accepts internally incomplete contact synchronization, partial identity scans can suppress all later discovery, transient key-material failures become terminal outcomes, and cold key resolution remains outside the documented whole-sequence budget. Source: reviewer backend model gpt-5.6-sol (general, rust-quality, and ffi-engineer lanes); final verifier backend model gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol was orchestration-only and is not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 3 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/manager/startup.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/startup.rs:360-364: The warm-launch shortcut can strand identities hidden by a partial scan
A locally persisted identity does not prove that the preceding gap-limit scan completed. Discovery deliberately returns success after any sighting even when later probes failed (`ScanTally::is_trustworthy`), and its warning states that an identity at a failed index may remain missing until the next scan. This shortcut prevents that next startup scan: after the first identity is persisted, every later launch skips discovery and only synchronizes identities already known locally. The timeout path has the same effect when a sighting was persisted before cancellation. Persist scan-completion state, or otherwise distinguish a complete gap-limit scan from a partial scan, so restored multi-identity wallets remain discoverable on later launches.
- [SUGGESTION] packages/rs-platform-wallet/src/manager/startup.rs:385-387: A failed contact sync can still be classified as Ready
(existing thread: https://github.com/dashpay/platform/pull/4359#discussion_r3752448291)
`Some(Ok(requests))` does not prove that the contact pass completed. `sync_contact_requests` catches received-fetch failures at `contact_requests.rs:1114-1127` and sent-fetch failures at `1135-1149`, continues processing, and ultimately returns `Ok(all_requests)` at line 1417. If Platform is unreachable for every identity, this arm still records `dashpay_sync_ran`; an empty deferred queue then produces `Ready` even though no contact requests were discovered or enqueued. Because `Ready` is the new API's promise that contact-derived addresses are prepared before SPV starts, propagate per-identity and per-direction completion information from `sync_contact_requests` and record a completed startup pass only when every required fetch and ingest succeeded.
In `packages/rs-platform-wallet-ffi/src/wallet_startup.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/wallet_startup.rs:163-175: Transient key-material lookup failures become terminal discovery failures
Every resolver error is discarded and converted to `master = None`. For an external-signable wallet requiring a cold scan, that selects resident-key discovery, which fails because the in-process wallet intentionally has no private key; startup then classifies the error as `DiscoveryFailed`, whose `discovery_worth_retrying()` result is false. The Swift preflight has the same loss of information: `WalletStorage.hasMnemonic` returns false for every non-success Security status, and line 130 consequently passes a null resolver for both a genuinely absent mnemonic and transient protected-data or Keychain failures. Preserve an unavailable/error state across both checks and expose it as retryable, rather than routing recoverable Keychain failures through the terminal local-discovery status.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/wallet_startup.rs:163-177: Cold-start key resolution is outside the startup budget
`resolve_master_from_resolver` synchronously invokes the Swift-owned Keychain resolver and performs mnemonic parsing, seed derivation, and master-key construction before `start_wallet_subsystems` creates its timer and deadline at `startup.rs:341-346`. Cold-start resolver latency is therefore neither deducted from `opts.budget` nor included in the exported `elapsed_ms`; a blocked callback can also hold the synchronous FFI call beyond the API's stated whole-sequence ceiling. Start deadline accounting before key resolution and include the callback in the bounded orchestration, passing only the remaining duration to subsequent steps. If key resolution is intentionally outside the budget, narrow the Rust, C, and Swift contracts instead of describing the budget and elapsed value as covering the whole sequence.
| // 1. Local identities first. A warm launch must not pay for a network | ||
| // scan it does not need. | ||
| if let Some(known) = self.local_identity_id(wallet_id).await { | ||
| tally.record_local_identity(known); | ||
| } else { |
There was a problem hiding this comment.
🟡 Suggestion: The warm-launch shortcut can strand identities hidden by a partial scan
A locally persisted identity does not prove that the preceding gap-limit scan completed. Discovery deliberately returns success after any sighting even when later probes failed (ScanTally::is_trustworthy), and its warning states that an identity at a failed index may remain missing until the next scan. This shortcut prevents that next startup scan: after the first identity is persisted, every later launch skips discovery and only synchronizes identities already known locally. The timeout path has the same effect when a sighting was persisted before cancellation. Persist scan-completion state, or otherwise distinguish a complete gap-limit scan from a partial scan, so restored multi-identity wallets remain discoverable on later launches.
source: ['codex']
There was a problem hiding this comment.
Confirmed, and it is a regression from this PR rather than a pre-existing property — before the warm-launch shortcut this call always scanned. Filed as #4365 rather than fixed here; reasoning below.
The chain is exactly as you describe, and the budget-expiry path shares it: within_budget returns None, local state is consulted, and a sighting persisted before cancellation is recorded as a warm launch. Worth adding that on iOS the only automatic fallback has the identical gate — DWCurrentUserIdentityInfo.recoverIfNeeded scans only when there are no local identities at all — so the sole remedy today is the manual Settings → Identities → Discover button.
I looked for a fix that does not need new persisted state, and there isn't one:
- Inferring it from the local index set catches the wrong case. A hole below the maximum known index (identities at 0 and 2, nothing at 1) is visible through
IdentityEntry::identity_index. The case that matters is not: found at 0, failed at 1, identity really at 1 leaves{0}— identical to what a clean scan of a single-identity wallet leaves. - Reusing existing storage doesn't work either.
sqlite/schema/blob.rsis a codec, not a KV table, andWalletMetadataEntryis emitted only at registration and requiresnetwork+wallet_group_id, neither of which a scan has to hand.
So it needs a column plus a refinery V002 — only V001 exists today — which is a schema-version bump, and a database migrated forward is refused by older app binaries. That is a release-coordination decision, not something to fold into a startup-ordering PR.
The other way to close it is to drop the shortcut and scan unconditionally. That costs ~5 probes (IDENTITY_GAP_LIMIT), each a Platform fetch plus a DPNS lookup, plus a Keychain round trip, before every Core SPV start — which is the cost the previous review round asked to remove.
Leaving the shortcut in with the gap tracked in #4365. ScanTally already knows failed_probes at the point it makes the verdict, so the fix is small once there is somewhere to record it.
There was a problem hiding this comment.
Resolved in 482997e — The warm-launch shortcut can strand identities hidden by a partial scan no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| let mut master = if mnemonic_resolver_handle.is_null() || !needs_scan_key { | ||
| None | ||
| } else { | ||
| match resolve_master_from_resolver(mnemonic_resolver_handle, &wid, network) { | ||
| Ok(master) => Some(master), | ||
| Err(_) => { | ||
| tracing::warn!( | ||
| wallet_id = %hex::encode(wid), | ||
| "startup: could not resolve the wallet mnemonic; continuing without \ | ||
| scan key material" | ||
| ); | ||
| None | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Transient key-material lookup failures become terminal discovery failures
Every resolver error is discarded and converted to master = None. For an external-signable wallet requiring a cold scan, that selects resident-key discovery, which fails because the in-process wallet intentionally has no private key; startup then classifies the error as DiscoveryFailed, whose discovery_worth_retrying() result is false. The Swift preflight has the same loss of information: WalletStorage.hasMnemonic returns false for every non-success Security status, and line 130 consequently passes a null resolver for both a genuinely absent mnemonic and transient protected-data or Keychain failures. Preserve an unavailable/error state across both checks and expose it as retryable, rather than routing recoverable Keychain failures through the terminal local-discovery status.
source: ['codex']
There was a problem hiding this comment.
Resolved in 482997e — Transient key-material lookup failures become terminal discovery failures no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| let mut master = if mnemonic_resolver_handle.is_null() || !needs_scan_key { | ||
| None | ||
| } else { | ||
| match resolve_master_from_resolver(mnemonic_resolver_handle, &wid, network) { | ||
| Ok(master) => Some(master), | ||
| Err(_) => { | ||
| tracing::warn!( | ||
| wallet_id = %hex::encode(wid), | ||
| "startup: could not resolve the wallet mnemonic; continuing without \ | ||
| scan key material" | ||
| ); | ||
| None | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
🟡 Suggestion: Cold-start key resolution is outside the startup budget
resolve_master_from_resolver synchronously invokes the Swift-owned Keychain resolver and performs mnemonic parsing, seed derivation, and master-key construction before start_wallet_subsystems creates its timer and deadline at startup.rs:341-346. Cold-start resolver latency is therefore neither deducted from opts.budget nor included in the exported elapsed_ms; a blocked callback can also hold the synchronous FFI call beyond the API's stated whole-sequence ceiling. Start deadline accounting before key resolution and include the callback in the bounded orchestration, passing only the remaining duration to subsequent steps. If key resolution is intentionally outside the budget, narrow the Rust, C, and Swift contracts instead of describing the budget and elapsed value as covering the whole sequence.
source: ['codex']
There was a problem hiding this comment.
Resolved in 482997e — Cold-start key resolution is outside the startup budget no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Reviewed the full diff, plus the drain/discovery paths it calls into. The direction is right — this ordering is exactly the kind of policy that belongs in the Rust library, and the StartupTally split with the empty-vs-unreachable pin is well done (manager::startup tests: 14 passed locally). One must-fix, one structural should-fix, and a few notes.
Must fix: the drains are not cancel-safe, and this PR is their first caller that cancels them
within_budget is tokio::time::timeout, i.e. drop-the-future on expiry. Every pre-existing caller of the two drains awaits them to completion (platform_wallet_drain_pending_contact_crypto, the payments.rs call sites) — the wrap at startup.rs#L419-L440 introduces cancellation as new surface.
Both drains accumulate their dequeue list in a local cleared vec and apply it once, after the loop (contact_requests.rs#L2179 for the provider drain, #L2396 for auto-accepts), while per-entry side effects (register_contact_account, mark_contact_channel_broken, the reciprocal send) commit as they go. So a budget expiry at entry 14 of 14:
- 13 accounts really exist, but all 14 entries stay queued;
drainedreports0(the.unwrap_or(0)), and the unboundedpending_contact_crypto_count()returns the full pre-drain count — so the outcome saysdrained=0, pending=14when the truth is 13 built, 1 remaining.
I did verify the damage stops there: accept_contact_request_with_external_signer checks already_established / already_reciprocated before broadcasting (contact_requests.rs#L2752-L2764), so the next launch's re-drain resolves those entries locally — no duplicate state transition, no unique-index rejection, no credit spend. And the provider-only drain skips AutoAccept entries, so drained + accepted doesn't double-count. The queue is at-least-once by design and survives this.
What doesn't survive is the outcome struct — the thing this PR exists to return — which is wrong exactly and only in the case the budget fires. The module doc's "entries it did not complete stay queued" is true but incomplete: entries it did complete also stay queued. Fix belongs in contact_requests.rs: clear each entry as it lands (or flush in small batches), rather than one apply at the end. That makes the drains honestly abandonable and the counters truthful.
Should fix: needs_scan_key re-implements the library's scan gate in the FFI, and mislabels the failure it can cause
wallet_startup.rs#L148-L177 predicts "key material is only needed if the library will scan, and it won't scan when a local identity exists." That same rule lives in the library at startup.rs#L360-L374 — two copies in two crates with nothing tying them together, and has_local_identity_blocking was added solely so the FFI can make the prediction. The JNI bridge will need a third copy. If the library ever changes when it scans (rescan-after-incomplete, a force option, deeper identity indices), the FFI silently supplies master = None and the external-signable path breaks.
The sharper consequence is the swallow at L168: a resolver failure becomes master = None + a warn, the library then takes the resident-wallet derive, which for an external-signable wallet errors ("External signable wallet has no private key") — not IdentityDiscoveryIncomplete — so the tally records a local, terminal failure and the client gets DiscoveryFailed. But the realistic causes of a resolver failure at launch are a locked device, a cancelled biometric prompt, or the Keychain not being available yet — all transient. DiscoveryFailed is documented, in both Rust and Swift, as "another scan will not answer it," and the Swift caller sees discoveryWorthRetrying == false and identityIsSettled == false: no correct action remains.
Suggestion: make the key source lazy — start_wallet_subsystems takes a closure/provider returning Result<ExtendedPrivKey, _>, invoked only on the branch that actually scans. Then the FFI drops to pure marshalling, both new accessors can go away, Android inherits the behaviour, and the library sees the resolver error directly and can classify it honestly (a retryable key-material-unavailable outcome rather than terminal DiscoveryFailed).
Related, worth a doc line either way: the resolver currently runs before Instant::now(), so the Keychain round trip sits outside the budget the FFI doc promises — and note that even moved inside, tokio::time::timeout cannot interrupt a synchronous SecItemCopyMatching, so a biometric prompt blocks past the deadline regardless.
Notes (non-blocking)
- The only production
ContactCryptoProviderlives in the FFI crate.platform-walletships the trait with test-only impls, so a non-FFI consumer can never drain, and this call can't self-supply a provider even for a resident-key wallet (it reportsPartialAccountsPendingforever). Pre-existing, not introduced here — andSeedCryptoProvideris already the exact shape a resident-wallet provider would take, if someone wants the follow-up. - Swift invents a budget floor. Because
0is the "use default" sentinel, Swift needsmax(1, converted)so a 0.4s request doesn't silently become 20s — a policy with no Rust-side home that the JNI bridge will reinvent differently. Milliseconds across the boundary, or an explicit use-default flag, removes the rule. PartialAccountsPendingcollapses three causes the library can tell apart (no provider / budget expiry / per-entry failures) — its own doc admits it. A reason field would keep the FFI'sprovider = Nonedecision visible downstream.- The manager handle store's read guard is now held for up to
budget. Thatparking_lotlock is write-preferring: a teardown arriving mid-startup queues every other manager FFI call behind it for the remainder. Same pattern asplatform_address_sync, but this is the first hold with a user-configurable duration. - Warm launch still performs one attribute-only
SecItemCopyMatchinginhasMnemonic— cheap and promptless, so the skip achieves what matters; just noting the last commit message slightly overstates it.
Verified by reading source rather than assumed: the discovery error-classification chain, both drains' clear-at-end structure and their full caller set, the accept-path idempotency guards, the VTableSigner cast (matches the four existing call sites), and that headers are cbindgen-generated. Ran cargo test -p platform-wallet --lib manager::startup (14/14). Did not build the Swift side.
…meout Both from review on #4359. **The drains are not cancel-safe, and this call was their first caller that cancels them.** Both commit per-entry side effects as they go — `register_contact_account`, `mark_contact_channel_broken`, the reciprocal send — while accumulating the dequeue list in a local vec applied once after the loop. `within_budget` is `tokio::time::timeout`, which drops the future, so a budget expiry at entry 14 of 14 left 13 accounts that really exist with all 14 entries still queued, and reported `drained=0, pending=14`. The queue is at-least-once, so nothing was corrupted; what was wrong is the outcome struct this call exists to return, wrong exactly when the budget fires. Fixed by threading the deadline into the loop rather than wrapping it: `drain_pending_contact_crypto_until` / `drain_auto_accepts_until`, with the existing methods as `None`-deadline wrappers so the six unbounded call sites are untouched. The loop stops between entries, and within an entry only the reads that precede its first commit are bounded — the xpub derive, the contact fetch, the ECDH, the proof-key derive. Every commit still runs to completion, `cleared` always describes work that landed, and the `.unwrap_or(0)` pair at the call site is gone rather than corrected. Clearing each entry as it lands was the other option, and costs more than it looks: `FlushMode::Immediate` means one sqlite transaction per `store()`, so a 350-entry queue would pay hundreds of blocking commits inside the pre-SPV path this whole sequence exists to keep short. The value-aware `retain_drained_by_snapshot` apply block is also left untouched this way. `register_external_contact_account` now documents why it must never be wrapped in a timeout: unlike its sibling it persists *before* acquiring the write lock, so a future dropped in that window leaves an account on disk that no in-memory collection knows about until the next drain re-registers it. Not reachable from this change — a landmine for the next one that wraps by analogy. **A key that could not be read was reported as a key that does not exist.** A resolver failure became `master = None`, the library took the resident-wallet derive, and an external-signable wallet failed it with "no private key" — classified terminal `DiscoveryFailed`. But the realistic causes at launch are a locked device or a denied Keychain read, all transient, and `DiscoveryFailed` tells the client not to try again. `ScanKey::{Resident, Master, Unavailable}` spells the third case out, and Swift stops collapsing it: `WalletStorage.mnemonicAvailability` returns `present` / `absent` / `unavailable(OSStatus)`, and only a definitive `absent` means watch-only. cargo test -p platform-wallet --lib # 635 passed (4 new) cargo test -p platform-wallet-ffi --lib # 261 passed cargo clippy + cargo fmt --check # clean
…he scan key lazily Both as review asked for them on #4359. **The dequeue now lands with the work.** Each drain applied its whole dequeue list after the loop while committing per-entry side effects as it went, so any stop — the budget, or a caller that drops the future — left completed work still queued and reported as zero. Both drains now flush one entry's removal before starting the next, through `flush_drained_contact_crypto` (value-aware, unchanged semantics) and `flush_cleared_auto_accepts` (key-retain plus the verify-failure marks, which have to land with their entry or the sweep re-queues a proof that was already rejected). At most one entry's bookkeeping is ever in flight, and that is inherent — there is no transaction spanning a Platform round trip and a sqlite write. The deadline-driven early exit stays alongside it. Per-entry clearing alone would leave `drained` unknowable whenever an outer timeout fires, because the return value dies with the dropped future; ending the loop between entries is what keeps the returned count meaningful. Together the queue is right even if a future caller wraps a drain in a timeout anyway, which is exactly the mistake this call made. **The FFI stopped predicting when a scan happens.** It asked `has_local_identity_blocking` and resolved the master xpriv up front, which is the library's rule reimplemented in a second crate, with the JNI bridge due to be the third — and a resolver failure there became `master = None`, then a resident-key derive, then terminal `DiscoveryFailed` for what is usually a locked device. `start_wallet_subsystems` now takes `Option<ScanKeyResolver>` — a closure invoked at most once, only on the branch that scans. The FFI drops to marshalling (handle carried as a `usize` to keep the closure `Send + Sync`), `has_local_identity_blocking` is deleted, and the library erases the key it resolved. A resolver error is classified retryable rather than terminal, and a warm launch touches no Keychain at all rather than relying on the caller to have predicted that. `wallet_network_blocking` stays: the contact-crypto provider is still built caller-side and genuinely needs the network, so it is not a prediction of library policy the way the identity gate was. cargo test -p platform-wallet --lib # 635 passed cargo test -p platform-wallet-ffi --lib # 261 passed cargo clippy --all-targets + cargo fmt --check # clean
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/WalletStorage.swift (1)
138-147: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe existing doc comment is now attached to the wrong declaration.
Lines 138-141 document a "cheap existence check used by signer preflight paths" that "does not materialize the mnemonic bytes". That text was written for
hasMnemonic(for:). The new declarations were inserted between the comment and that method, so the paragraph now prefixesMnemonicAvailability, andhasMnemonic(for:)at Line 178 has no documentation.📝 Proposed fix to reattach the documentation
- /// Cheap existence check used by signer preflight paths. - /// - /// Unlike `retrieveMnemonic(...)`, this does not materialize the - /// mnemonic bytes into Swift heap objects. /// Three-way answer to "can this wallet's mnemonic be read right now?".Then add the moved paragraph above
hasMnemonic(for:)at Line 178.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/WalletStorage.swift` around lines 138 - 147, Move the existing cheap existence-check documentation from above MnemonicAvailability to directly above hasMnemonic(for:). Keep MnemonicAvailability documented only by its enum-specific comments, and preserve the full hasMnemonic(for:) behavior description without changing implementation.
🧹 Nitpick comments (2)
packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/WalletStorage.swift (1)
178-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLet
hasMnemonic(for:)delegate tomnemonicAvailability(for:).Both methods now build the same query dictionary and call
SecItemCopyMatching. Delegation removes the duplicate query and keeps the two answers in agreement if the query changes.♻️ Proposed refactor
public func hasMnemonic(for walletId: Data) -> Bool { - let account = perWalletMnemonicAccount(for: walletId) - let query: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: keychainService, - kSecAttrAccount as String: account, - kSecMatchLimit as String: kSecMatchLimitOne, - kSecReturnAttributes as String: true - ] - var result: AnyObject? - let status = SecItemCopyMatching(query as CFDictionary, &result) - return status == errSecSuccess + mnemonicAvailability(for: walletId) == .present }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/WalletStorage.swift` around lines 178 - 190, Update hasMnemonic(for:) to delegate directly to mnemonicAvailability(for:) and return its result, removing the duplicated Keychain query construction and SecItemCopyMatching call while preserving the existing boolean behavior.packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs (1)
1932-1939: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider amortizing the per-entry persistence round trip.
The flush now runs once per completed entry. Each flush takes the wallet-manager write lock and issues one
persister.storecall. A restore that queues two ops per contact therefore performs O(contacts) small writes where the previous code performed one.The accuracy gain is the point, so keep the per-entry boundary. If restore latency becomes a problem, flush on a small batch threshold instead, and keep the flush immediately before the budget check so a stop still strands at most one batch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs` around lines 1932 - 1939, Adjust the restore loop around flush_drained_contact_crypto so persistence remains at the per-entry accuracy boundary while amortizing writes with a small batch threshold. Flush when the cleared queue reaches that threshold, keep the flush immediately before the budget check, and ensure stopping strands no more than one batch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- Line 2604: Update flush_cleared_auto_accepts to process verify_failed
independently of the cleared list: dequeue and mark verification failures
whenever verify_failed is non-empty, even when cleared is empty. Keep changeset
persistence and the owner retain loop guarded by !cleared.is_empty(), while
preserving the existing handling for cleared entries.
---
Outside diff comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/WalletStorage.swift`:
- Around line 138-147: Move the existing cheap existence-check documentation
from above MnemonicAvailability to directly above hasMnemonic(for:). Keep
MnemonicAvailability documented only by its enum-specific comments, and preserve
the full hasMnemonic(for:) behavior description without changing implementation.
---
Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- Around line 1932-1939: Adjust the restore loop around
flush_drained_contact_crypto so persistence remains at the per-entry accuracy
boundary while amortizing writes with a small batch threshold. Flush when the
cleared queue reaches that threshold, keep the flush immediately before the
budget check, and ensure stopping strands no more than one batch.
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/WalletStorage.swift`:
- Around line 178-190: Update hasMnemonic(for:) to delegate directly to
mnemonicAvailability(for:) and return its result, removing the duplicated
Keychain query construction and SecItemCopyMatching call while preserving the
existing boolean behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 913bc13a-bfcf-4739-a1f4-c0c0ea2508e4
📒 Files selected for processing (7)
packages/rs-platform-wallet-ffi/src/wallet_startup.rspackages/rs-platform-wallet/src/manager/accessors.rspackages/rs-platform-wallet/src/manager/startup.rspackages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rspackages/rs-platform-wallet/src/wallet/identity/network/contacts.rspackages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/WalletStorage.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerStartup.swift
💤 Files with no reviewable changes (1)
- packages/rs-platform-wallet/src/manager/accessors.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerStartup.swift
From CodeRabbit on #4359. Splitting the end-of-drain apply block into a per-entry flush carried its outer `if !cleared.is_empty()` guard over the verify-failure marking too. The caller advances both cursors after the helper returns, so a call with marks and no dequeue would drop those marks for good — the next sweep would then re-queue a proof already proven bad. Unreachable today, since every `verify_failed` push is paired with a `cleared` push, but it is a trap the previous shape did not have: there the two always arrived together by construction. Each list is now applied on its own, and only the dequeue is persisted — the marks are in-memory by design, retried once per launch. Also on the Swift side: `mnemonicAvailability` was inserted between `hasMnemonic`'s doc comment and `hasMnemonic` itself, leaving the "cheap existence check" paragraph documenting the enum. Reattached, and `hasMnemonic` now delegates rather than repeating the query, so the two answers cannot drift apart.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4359 +/- ##
=========================================
Coverage 87.80% 87.80%
=========================================
Files 2641 2641
Lines 336510 336510
=========================================
Hits 295468 295468
Misses 41042 41042
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The centralized startup ordering is directionally sound, and the latest commits improve lazy key resolution, deadline-aware draining, and cancellation-safe queue bookkeeping. Seven in-scope suggestions remain: incomplete contact or identity discovery can still be presented as settled, synchronous callbacks can overrun the advertised budget, resolved key material is not erased on cancellation, resolver faults lose their permanence classification, and the real orchestration wiring remains untested.
Source: reviewer backends gpt-5.6-sol (general), gpt-5.6-sol (rust-quality), and gpt-5.6-sol (ffi-engineer); final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol was orchestration-only and is not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 4 suggestion(s)
1 additional finding(s) omitted (not in diff).
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/manager/startup.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/startup.rs:534-633: Resolved scan key is not erased when the startup future is cancelled
The resolver transfers an owned `ExtendedPrivKey` into this async function, but erasure occurs only in explicit code after the discovery block finishes. If a Rust caller drops or cancels `start_wallet_subsystems` while discovery is awaiting Platform, the future is dropped and lines 629-633 never run. `ExtendedPrivKey` has no erasing `Drop`, as documented by `resolve_master_from_resolver`, so its private scalar remains in freed future memory despite this API's per-call key-material contract. Store the resolved key in an RAII guard whose `Drop` calls `non_secure_erase`, covering completion, cancellation, and unwinding.
- [SUGGESTION] packages/rs-platform-wallet/src/manager/startup.rs:637-837: The ordered orchestration is not exercised by its tests
Every test in this module exercises `StartupTally`, outcome conversion, or the standalone `within_budget` helper; none constructs a manager or invokes `start_wallet_subsystems`. The PR's central behavior—lazy resolver invocation, identity/contact/drain ordering, retry wiring, deadline propagation, and translation of real operation results into the tally—can therefore regress while these tests remain green. Add a manager-level test with a mock SDK and recording `ScanKeyResolver` and `ContactCryptoProvider` implementations that invokes the public method and asserts the observed order plus representative timeout and failure paths.
- [SUGGESTION] packages/rs-platform-wallet/src/manager/startup.rs:417-425: A failed contact sync can still be classified as Ready
(existing thread: https://github.com/dashpay/platform/pull/4359#discussion_r3752448291)
`Some(Ok(requests))` does not prove that the contact pass completed. `sync_contact_requests` catches received-fetch failures at `contact_requests.rs:1150-1163`, sent-fetch failures at `1171-1185`, and per-direction persistence failures around `1299-1353`, then returns `Ok(all_requests)` at line 1453. This arm consequently records `dashpay_sync_ran` even when required directions were skipped. If the deferred queue is empty, the outcome becomes `Ready` despite never discovering or ingesting requests whose account builds should precede SPV startup. Return a typed completion summary from `sync_contact_requests` and record a completed startup pass only when every required fetch and ingest succeeded.
- [SUGGESTION] packages/rs-platform-wallet/src/manager/startup.rs:392-405: The warm-launch shortcut can strand identities hidden by a partial scan
(existing thread: https://github.com/dashpay/platform/pull/4359#discussion_r3752862202)
A locally persisted identity does not prove that the preceding gap-limit scan completed. `ScanTally::is_trustworthy()` returns success after any identity sighting even if later probes failed, and discovery explicitly warns that an identity at a failed index may remain missing until a later scan. The timeout path also accepts an identity persisted before cancellation. Once either path leaves one local identity, this shortcut skips every subsequent startup scan, so another seed identity at a failed or unvisited index can remain undiscovered indefinitely. Persist scan-completion state separately from discovered identities, or retain a rescan requirement whenever the preceding scan was partial.
In `packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:2571-2575: Auto-accept can run past the whole startup budget
The deadline bounds proof-key derivation and stops the drain between entries, but once an entry reaches this call, `accept_contact_request_with_external_signer` is awaited without a deadline. That operation can fetch or derive account data, invoke signer/provider callbacks, broadcast a reciprocal state transition, and register accounts. The FFI signer alone permits an asynchronous completion to take up to 300 seconds, far beyond the default 20-second startup budget. A valid in-flight accept can therefore hold the synchronous startup call long after its documented ceiling. Thread the deadline through cancel-safe pre-commit stages and define safe post-broadcast completion behavior, or document that a started auto-accept entry is exempt from the ceiling and that the budget is only a stop-between-entries target.
In `packages/rs-platform-wallet-ffi/src/wallet_startup.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/wallet_startup.rs:158-176: Resolver error flattening makes permanent key faults retryable
The callback adapter converts every failure from `resolve_master_from_resolver` into the same `PlatformWalletError::KeyDerivation`, and startup maps every resolver error to retryable `PartialNoIdentity`. That is appropriate for a locked device or temporary Keychain outage, but the lower layer also reports invalid UTF-8, invalid BIP-39 phrases, invalid callback lengths, an oversized buffer, and master-key construction failures. Those persistent local or callback-contract faults cannot be repaired by another discovery attempt, yet Swift receives `discoveryWorthRetrying == true` and may repeatedly schedule one. Preserve a typed temporary-unavailable versus malformed/permanent distinction across the resolver adapter and classify only the former as retryable.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/wallet_startup.rs:158-177: Cold-start key resolution is outside the startup budget
(existing thread: https://github.com/dashpay/platform/pull/4359#discussion_r3752862221)
The Rust timer now starts before the lazy resolver runs, so resolver latency is included in `elapsed_ms` and deducted from later steps. However, `resolve()` remains a synchronous callback with no interruptible deadline boundary: a delayed Keychain query, mnemonic derivation, or custom resolver holds the synchronous C call until it returns. Swift's `mnemonicAvailability` Keychain lookup also runs before entering the timed Rust call. This still contradicts the Rust and Swift contracts describing `budget` as a ceiling for the whole sequence and the FFI contract saying the call must always terminate. Make key acquisition genuinely asynchronous/cancellable with callback ownership that can outlive an abandoned startup, or narrow all three public contracts to state that synchronous host key access is measured but not bounded.
| let mut master = match scan_key { | ||
| None => None, | ||
| Some(resolve) => match resolve() { | ||
| Ok(master) => Some(master), | ||
| Err(e) => { | ||
| tracing::warn!( | ||
| error = %e, | ||
| "startup: scan key material unavailable; deferring discovery to a \ | ||
| later start" | ||
| ); | ||
| tally.record_unreachable(); | ||
| tally.record_discovery_gave_up(); | ||
| return; | ||
| } | ||
| }, | ||
| }; | ||
|
|
||
| let opts = IdentityDiscoveryOptions { | ||
| start_index: Some(0), | ||
| gap_limit: gap_limit.unwrap_or(IdentityDiscoveryOptions::default().gap_limit), | ||
| }; | ||
|
|
||
| // Scoped so every exit path — including the early returns inside the | ||
| // loop — lands on the erase below. `return` leaves this block, not the | ||
| // function, so the tally bookkeeping keeps its existing shape. | ||
| async { | ||
| for (attempt, backoff) in DISCOVERY_BACKOFF.iter().map(Some).chain([None]).enumerate() { | ||
| // A single scan walks up to `gap_limit` indices, each a Platform | ||
| // fetch plus a DPNS lookup, so it needs the same ceiling the other | ||
| // steps have — otherwise one attempt can outlast the whole budget | ||
| // this call promises. | ||
| let attempt_future = async { | ||
| match master.as_ref() { | ||
| Some(master) => identity_wallet.discover_from_master(opts, master).await, | ||
| None => identity_wallet.discover(opts).await, | ||
| } | ||
| }; | ||
| let Some(result) = within_budget(deadline, attempt_future).await else { | ||
| // Sightings persist incrementally, so an abandoned scan may | ||
| // still have folded an identity in before it was cut off. | ||
| if let Some(known) = self.local_identity_id(wallet_id).await { | ||
| tally.record_local_identity(known); | ||
| return; | ||
| } | ||
| break; | ||
| }; | ||
|
|
||
| match result { | ||
| Ok(found) => { | ||
| match found.first() { | ||
| Some(identity) => tally.record_discovered(identity.id()), | ||
| // An empty return is not proof on its own: `discover` | ||
| // reports only identities THIS call inserted, so a | ||
| // concurrent startup that inserted one first leaves us | ||
| // seeing it as already-managed and returning nothing. | ||
| // Consult local state before calling it absence. | ||
| None => match self.local_identity_id(wallet_id).await { | ||
| Some(known) => tally.record_discovered(known), | ||
| None => tally.record_proven_absent(), | ||
| }, | ||
| } | ||
| return; | ||
| } | ||
| Err(PlatformWalletError::IdentityDiscoveryIncomplete { .. }) => { | ||
| tally.record_unreachable(); | ||
| } | ||
| Err(e) => { | ||
| // Not a reachability question — a wallet/persistence | ||
| // failure will not fix itself on the next attempt, so this | ||
| // is recorded as terminal rather than as "try again". | ||
| tracing::warn!( | ||
| error = %e, | ||
| "startup: identity discovery failed for a non-network reason" | ||
| ); | ||
| tally.record_discovery_failed_locally(); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| let Some(backoff) = backoff else { break }; | ||
| let remaining = deadline.saturating_duration_since(Instant::now()); | ||
| if remaining.is_zero() { | ||
| break; | ||
| } | ||
| tracing::info!( | ||
| attempt = attempt + 1, | ||
| "startup: identity discovery could not reach Platform; retrying" | ||
| ); | ||
| tokio::time::sleep((*backoff).min(remaining)).await; | ||
| } | ||
|
|
||
| tally.record_discovery_gave_up(); | ||
| } | ||
| .await; | ||
|
|
||
| // The key lives no longer than the scan that needed it. `ExtendedPrivKey` | ||
| // has no `Drop`, so this is the only thing that clears it. | ||
| if let Some(master) = master.as_mut() { | ||
| master.private_key.non_secure_erase(); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Resolved scan key is not erased when the startup future is cancelled
The resolver transfers an owned ExtendedPrivKey into this async function, but erasure occurs only in explicit code after the discovery block finishes. If a Rust caller drops or cancels start_wallet_subsystems while discovery is awaiting Platform, the future is dropped and lines 629-633 never run. ExtendedPrivKey has no erasing Drop, as documented by resolve_master_from_resolver, so its private scalar remains in freed future memory despite this API's per-call key-material contract. Store the resolved key in an RAII guard whose Drop calls non_secure_erase, covering completion, cancellation, and unwinding.
source: ['codex']
There was a problem hiding this comment.
Resolved in 58a43aa — Resolved scan key is not erased when the startup future is cancelled no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
There was a problem hiding this comment.
Fixed in 58a43aa — and you are right that moving the erase into the library is what created the window. In the FFI it sat after a run_on_big_stack_thread join, so it ran on every path out of that scope; as an explicit call after the discovery loop it runs on every path the loop takes, and on none at all if the whole future is dropped mid-await.
The key is now owned by a ScanKeyGuard that erases in Drop, so erasure follows the value's lifetime rather than a code path. That also let the scan loop go back to its plain shape — the async block around it existed only to funnel the loop's early returns onto the explicit call.
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| fn identity() -> Identifier { | ||
| Identifier::from([7u8; 32]) | ||
| } | ||
|
|
||
| /// The regression this type exists to prevent, and the one that already | ||
| /// shipped once in a client: a scan that came back definitively empty must | ||
| /// settle as `NoIdentity`, never as something to retry. | ||
| #[test] | ||
| fn proven_absence_settles_and_is_not_retryable() { | ||
| let mut tally = StartupTally::default(); | ||
| tally.record_proven_absent(); | ||
|
|
||
| assert_eq!(tally.status(), WalletStartupStatus::NoIdentity); | ||
| assert!( | ||
| tally.status().identity_is_settled(), | ||
| "a proof of absence is an answer; retrying it cannot change it" | ||
| ); | ||
| } | ||
|
|
||
| /// The opposite case, and the reason the distinction is expressible at all | ||
| /// (platform#4352): never reaching Platform is not evidence of absence. | ||
| #[test] | ||
| fn unreachable_discovery_is_not_settled() { | ||
| let mut tally = StartupTally::default(); | ||
| tally.record_unreachable(); | ||
| tally.record_unreachable(); | ||
| tally.record_discovery_gave_up(); | ||
|
|
||
| assert_eq!(tally.status(), WalletStartupStatus::PartialNoIdentity); | ||
| assert!(!tally.status().identity_is_settled()); | ||
| assert_eq!(tally.discovery_attempts, 2); | ||
| } | ||
|
|
||
| /// A local discovery fault is terminal, unlike an unreachable Platform. | ||
| /// The branch that produces it says the failure will not fix itself, so | ||
| /// reporting it as retryable would send clients on a futile rescan. | ||
| #[test] | ||
| fn a_local_discovery_failure_is_terminal() { | ||
| let mut tally = StartupTally::default(); | ||
| tally.record_discovery_failed_locally(); | ||
|
|
||
| assert_eq!(tally.status(), WalletStartupStatus::DiscoveryFailed); | ||
| assert!( | ||
| !tally.status().discovery_worth_retrying(), | ||
| "the same local fault will still be there next time" | ||
| ); | ||
| assert!( | ||
| !tally.status().identity_is_settled(), | ||
| "terminal is not the same as answered — we still do not know" | ||
| ); | ||
| } | ||
|
|
||
| /// Both leave the identity question open, but only one is worth asking | ||
| /// again. Keeping that asymmetry visible is the point of the two methods. | ||
| #[test] | ||
| fn only_an_unreachable_platform_is_worth_retrying() { | ||
| let mut unreachable = StartupTally::default(); | ||
| unreachable.record_unreachable(); | ||
| unreachable.record_discovery_gave_up(); | ||
| assert!(unreachable.status().discovery_worth_retrying()); | ||
|
|
||
| for terminal in [ | ||
| WalletStartupStatus::Ready, | ||
| WalletStartupStatus::NoIdentity, | ||
| WalletStartupStatus::PartialAccountsPending, | ||
| WalletStartupStatus::DiscoveryFailed, | ||
| ] { | ||
| assert!( | ||
| !terminal.discovery_worth_retrying(), | ||
| "{terminal:?} must not ask the client to rescan" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /// An unreachable Platform outranks a clean drain: the later steps ran | ||
| /// against state we know to be incomplete. | ||
| #[test] | ||
| fn unreachable_discovery_outranks_a_finished_drain() { | ||
| let mut tally = StartupTally::default(); | ||
| tally.record_unreachable(); | ||
| tally.record_discovery_gave_up(); | ||
| tally.record_drain(0, 0); | ||
|
|
||
| assert_eq!(tally.status(), WalletStartupStatus::PartialNoIdentity); | ||
| } | ||
|
|
||
| #[test] | ||
| fn identity_found_and_drained_is_ready() { | ||
| let mut tally = StartupTally::default(); | ||
| tally.record_discovered(identity()); | ||
| tally.record_sync_ran(); | ||
| tally.record_drain(4, 0); | ||
|
|
||
| assert_eq!(tally.status(), WalletStartupStatus::Ready); | ||
| assert_eq!(tally.discovery_attempts, 1); | ||
| } | ||
|
|
||
| /// A warm launch: the identity was already on file, so no scan ran at all. | ||
| #[test] | ||
| fn local_identity_needs_no_discovery_attempt() { | ||
| let mut tally = StartupTally::default(); | ||
| tally.record_local_identity(identity()); | ||
| tally.record_sync_ran(); | ||
| tally.record_drain(0, 0); | ||
|
|
||
| assert_eq!(tally.status(), WalletStartupStatus::Ready); | ||
| assert_eq!( | ||
| tally.discovery_attempts, 0, | ||
| "a known identity must not cost a network scan" | ||
| ); | ||
| } | ||
|
|
||
| /// An empty drain queue is not evidence of readiness on its own. Without a | ||
| /// completed contact pass there may be requests nobody has looked at, whose | ||
| /// account builds were therefore never enqueued — reporting `Ready` would | ||
| /// promise addresses this call never prepared. | ||
| #[test] | ||
| fn an_empty_queue_without_a_contact_pass_is_not_ready() { | ||
| let mut tally = StartupTally::default(); | ||
| tally.record_discovered(identity()); | ||
| tally.record_drain(0, 0); | ||
|
|
||
| assert!(!tally.dashpay_sync_ran); | ||
| assert_eq!(tally.status(), WalletStartupStatus::PartialAccountsPending); | ||
| } | ||
|
|
||
| #[test] | ||
| fn queued_builds_report_as_pending() { | ||
| let mut tally = StartupTally::default(); | ||
| tally.record_discovered(identity()); | ||
| tally.record_sync_ran(); | ||
| tally.record_drain(2, 3); | ||
|
|
||
| assert_eq!(tally.status(), WalletStartupStatus::PartialAccountsPending); | ||
| assert!( | ||
| tally.status().identity_is_settled(), | ||
| "the identity question is answered even though the drain is not done" | ||
| ); | ||
| } | ||
|
|
||
| /// `has_identity` gates the sync and drain steps, so it must not be fooled | ||
| /// by a proven absence. | ||
| #[test] | ||
| fn proven_absence_has_no_identity_to_sync_for() { | ||
| let mut tally = StartupTally::default(); | ||
| tally.record_proven_absent(); | ||
|
|
||
| assert!(!tally.has_identity()); | ||
| } | ||
|
|
||
| /// Every network step is abandonable, so `within_budget` must return | ||
| /// `None` rather than run a future past the deadline. This is the guard for | ||
| /// the gap review found: bounding only the discovery retries let a stalled | ||
| /// sync or drain hold Core SPV well past `budget`. | ||
| #[tokio::test(start_paused = true)] | ||
| async fn within_budget_abandons_a_step_that_outlasts_the_deadline() { | ||
| let deadline = Instant::now() + Duration::from_secs(2); | ||
| let slow = async { | ||
| tokio::time::sleep(Duration::from_secs(30)).await; | ||
| "finished" | ||
| }; | ||
| assert_eq!(within_budget(deadline, slow).await, None); | ||
| } | ||
|
|
||
| #[tokio::test(start_paused = true)] | ||
| async fn within_budget_returns_a_step_that_fits() { | ||
| let deadline = Instant::now() + Duration::from_secs(10); | ||
| let quick = async { | ||
| tokio::time::sleep(Duration::from_secs(1)).await; | ||
| "finished" | ||
| }; | ||
| assert_eq!(within_budget(deadline, quick).await, Some("finished")); | ||
| } | ||
|
|
||
| /// A deadline already in the past must not start the step at all. | ||
| #[tokio::test(start_paused = true)] | ||
| async fn within_budget_skips_once_the_deadline_has_passed() { | ||
| let deadline = Instant::now(); | ||
| tokio::time::sleep(Duration::from_secs(1)).await; | ||
| assert_eq!(within_budget(deadline, async { "ran" }).await, None); | ||
| } | ||
|
|
||
| #[test] | ||
| fn outcome_carries_the_tally_through() { | ||
| let mut tally = StartupTally::default(); | ||
| tally.record_discovered(identity()); | ||
| tally.record_sync_ran(); | ||
| tally.record_drain(1, 0); | ||
|
|
||
| let outcome = tally.into_outcome(Duration::from_secs(3)); | ||
| assert_eq!(outcome.status, WalletStartupStatus::Ready); | ||
| assert_eq!(outcome.identity_id, Some(identity())); | ||
| assert!(outcome.dashpay_sync_ran); | ||
| assert_eq!(outcome.contact_accounts_drained, 1); | ||
| assert_eq!(outcome.elapsed, Duration::from_secs(3)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: The ordered orchestration is not exercised by its tests
Every test in this module exercises StartupTally, outcome conversion, or the standalone within_budget helper; none constructs a manager or invokes start_wallet_subsystems. The PR's central behavior—lazy resolver invocation, identity/contact/drain ordering, retry wiring, deadline propagation, and translation of real operation results into the tally—can therefore regress while these tests remain green. Add a manager-level test with a mock SDK and recording ScanKeyResolver and ContactCryptoProvider implementations that invokes the public method and asserts the observed order plus representative timeout and failure paths.
source: ['codex']
There was a problem hiding this comment.
Accurate, and it is called out as a known gap in the PR description rather than an oversight — every test here drives StartupTally, the outcome conversion, or within_budget, and none constructs a manager.
What it would take, having looked: a mock SDK via SdkBuilder::new_mock(), a registered wallet on a SqlitePersister (the __test-helpers feature has wallet_meta::ensure_exists for exactly this), and a fake ContactCryptoProvider — SeedCryptoProvider in contact_requests.rs is already the right shape for that half. The piece I could not confirm is the one that matters most for this module: whether the mock SDK can be made to fail an Identity::fetch in a way that surfaces as IdentityDiscoveryIncomplete, which is what the retry wiring and the empty-vs-unreachable distinction turn on. Without that, a manager-level test would cover the ordering but not the branch that has already regressed once.
I have taken the parts that are unit-testable without it: the deadline helper the drains use, and now the resolver's retry classification (4 tests, fake vtable). The orchestration itself I would rather see covered by a real integration test than by a mock deep enough to be its own fiction — filing that separately rather than half-doing it here.
| let resolver_addr = mnemonic_resolver_handle as usize; | ||
| let resolve_scan_key = move || { | ||
| resolve_master_from_resolver(resolver_addr as *mut MnemonicResolverHandle, &wid, network) | ||
| .map_err(|e| { | ||
| tracing::warn!( | ||
| wallet_id = %hex::encode(wid), | ||
| code = ?e.code, | ||
| "startup: could not resolve the wallet mnemonic for the identity scan" | ||
| ); | ||
| // Carried as an error the library can classify, not as an FFI | ||
| // throw: the documented contract is that only handle and | ||
| // wallet-id problems throw, and a locked device is neither. | ||
| platform_wallet::error::PlatformWalletError::KeyDerivation(format!( | ||
| "mnemonic resolver failed ({:?})", | ||
| e.code | ||
| )) | ||
| }) | ||
| }; | ||
| let scan_key = (!mnemonic_resolver_handle.is_null()) |
There was a problem hiding this comment.
🟡 Suggestion: Resolver error flattening makes permanent key faults retryable
The callback adapter converts every failure from resolve_master_from_resolver into the same PlatformWalletError::KeyDerivation, and startup maps every resolver error to retryable PartialNoIdentity. That is appropriate for a locked device or temporary Keychain outage, but the lower layer also reports invalid UTF-8, invalid BIP-39 phrases, invalid callback lengths, an oversized buffer, and master-key construction failures. Those persistent local or callback-contract faults cannot be repaired by another discovery attempt, yet Swift receives discoveryWorthRetrying == true and may repeatedly schedule one. Preserve a typed temporary-unavailable versus malformed/permanent distinction across the resolver adapter and classify only the former as retryable.
source: ['codex']
There was a problem hiding this comment.
Resolved in 58a43aa — Resolver error flattening makes permanent key faults retryable no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
There was a problem hiding this comment.
Fixed in 58a43aa, taking the classification you asked for.
The blocker was that the FFI result codes cannot express it: NOT_FOUND, the Keychain-error bucket, an impossible length, a phrase in no wordlist and a failed master-key build all land on ErrorWalletOperation, so nothing downstream can tell them apart without reading the message. The distinction now travels with the failure — ResolveFailureKind::{Unavailable, Permanent} on a ResolveFailure, produced at each failure site by _classified variants of the two resolver helpers. The existing helpers are thin wrappers that drop the kind, so the other ~10 call sites are untouched.
On the library side that becomes ScanKeyError::{Unavailable, Invalid}: unavailable keeps the retryable path, invalid records a local discovery failure and settles as terminal DiscoveryFailed.
One deliberate departure from your list: NOT_FOUND is classified retryable, not permanent. The host filters watch-only wallets before calling (mnemonicAvailability == .absent → no resolver at all), so reaching NOT_FOUND means the item was expected and was not there — a wipe/restore race — rather than proof this wallet has no seed.
4 tests drive each cause through a fake resolver vtable. The mapping is one that reads fine while behaving wrong in opposite directions, so it is worth pinning.
…ilure's permanence Both from review on #4359. **The erase was on a code path, and this call invites cancellation.** Moving erasure from the FFI into the library left it as an explicit `non_secure_erase` after the discovery loop — reached by every return the loop makes, and by none at all if the caller drops the whole `start_wallet_subsystems` future while a scan is awaiting Platform. `ExtendedPrivKey` has no erasing `Drop`, so the scalar would stay in the freed future. It is now owned by a `ScanKeyGuard` that erases on drop, which also lets the scan loop go back to its plain shape: the `async` block existed only to funnel returns onto the explicit call. **A corrupt phrase was reported as something the next launch might fix.** Every resolver failure became one `KeyDerivation`, and startup maps any resolver error to retryable `PartialNoIdentity`. Right for a locked device; wrong for invalid UTF-8, a phrase in no BIP-39 wordlist, an impossible buffer length, or a seed that yields no master key — none of which a retry edits. The FFI result codes cannot carry this: every one of those causes lands on `ErrorWalletOperation`. So the classification is captured where it is produced — `ResolveFailureKind::{Unavailable, Permanent}` on a `ResolveFailure`, returned by `_classified` variants of the two resolver helpers. The existing helpers become thin wrappers that drop the kind, so the ~10 other call sites are untouched. The library side is `ScanKeyError::{Unavailable, Invalid}`: unavailable keeps the retryable path, invalid records a local discovery failure and settles as terminal `DiscoveryFailed`. `NOT_FOUND` is deliberately retryable rather than permanent: the host filters watch-only wallets before calling, so reaching it means the item was expected and was not there — a wipe/restore race, not proof the wallet has no seed. 4 tests drive the classification through a fake resolver vtable, one per cause. Getting the mapping backwards is exactly the kind of thing that reads fine and behaves wrong, in opposite directions. cargo test -p platform-wallet --lib # 635 passed cargo test -p platform-wallet-ffi --lib # 265 passed (4 new) cargo clippy --all-targets + cargo fmt --check # clean
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The centralized startup orchestration is directionally sound, and the latest commits correctly add lazy scan-key resolution, lifetime-based key erasure, typed resolver-failure classification, and incremental deadline-aware drains. Four in-scope suggestions remain: incomplete contact synchronization can still produce Ready, synchronous key resolution and a started auto-accept can overrun the advertised whole-sequence budget, and the production orchestration is not directly tested.
Source: Reviewer backend model gpt-5.6-sol (general, rust-quality, and ffi-engineer lanes); final verifier backend model gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/network/contact_requests.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:2571-2574: Auto-accept can run past the whole startup budget
The drain checks the deadline between entries and bounds proof-key derivation, but after a proof verifies it awaits `accept_contact_request_with_external_signer` without a deadline. That operation can derive account data, invoke provider and signer callbacks, broadcast the reciprocal state transition, fetch the contact identity, and register both accounts. The FFI signer's asynchronous completion timeout alone is 300 seconds, far beyond the default 20-second startup budget, so one started auto-accept can keep the startup call blocked long after its documented ceiling. Thread the deadline through cancellation-safe pre-commit work and define the required post-broadcast completion behavior, or document consistently that started auto-accept entries are exempt and the budget is only checked between entries.
In `packages/rs-platform-wallet/src/manager/startup.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/startup.rs:467-469: A failed contact sync can still be classified as Ready
(existing thread: https://github.com/dashpay/platform/pull/4359#discussion_r3752448291)
This records a completed contact pass for every `Ok` from `sync_contact_requests`, but that method deliberately converts several incomplete outcomes into `Ok(all_requests)`. It skips an identity after a received-fetch failure at `contact_requests.rs:1150-1163`, ingests only the received side after a sent-fetch failure at 1171-1185, and catches per-direction persistence failures around 1293-1353. The SDK pagination helper also returns `Ok` after its 5,000-document per-sweep cap even when more documents remain. If no deferred entries are currently queued, `StartupTally::status` therefore returns `Ready` even though requests whose accounts must precede SPV may not have been fetched or persisted. Return a typed synchronization summary carrying per-identity, per-direction, persistence, and pagination-exhaustion state, and call `record_sync_ran` only when every required direction completed.
- [SUGGESTION] packages/rs-platform-wallet/src/manager/startup.rs:685-885: The ordered orchestration is not exercised by its tests
(existing thread: https://github.com/dashpay/platform/pull/4359#discussion_r3753883576)
The tests exercise `StartupTally`, outcome conversion, and the standalone `within_budget` helper, but none constructs a `PlatformWalletManager` or invokes `start_wallet_subsystems`. Consequently, the PR's central behavior—lazy resolver invocation, identity/contact/drain ordering, retry wiring, deadline propagation, cancellation cleanup, and translation of real operation results into the tally—can be miswired while every test in this module remains green. Add a manager-level test using a mock SDK and recording `ScanKeyResolver` and `ContactCryptoProvider` implementations, asserting the observed order and representative timeout, resolver-failure, and warm-launch paths.
In `packages/rs-platform-wallet-ffi/src/wallet_startup.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/wallet_startup.rs:161-167: Cold-start key resolution is outside the startup budget
(existing thread: https://github.com/dashpay/platform/pull/4359#discussion_r3752862221)
The manager timer now starts before this lazy resolver is invoked, so successful resolver latency is included in `elapsed_ms` and deducted from later steps. However, `resolve()` remains a synchronous host callback and cannot be interrupted at the deadline: a delayed Keychain query, mnemonic derivation, biometric flow, or custom resolver can hold the synchronous C entry point indefinitely. Swift also performs `mnemonicAvailability(for:)` synchronously before entering the timed Rust call. This conflicts with the Rust and Swift contracts describing `budget` as a ceiling for the whole sequence and the FFI documentation stating that the call must always terminate. Make host key acquisition asynchronous and cancellable with callback ownership that can survive abandoning startup, or narrow all public contracts to state that synchronous host key access is measured where applicable but not bounded.
4 participants
Issue being fixed or feature implemented
A DashPay contact's DIP-15 payment addresses are derived from its contact account, and an address the wallet is not watching when the compact-filter scan passes its funding height produces no transaction at all. So the order in which a wallet brings identity, contacts and contact accounts up decides whether a restored wallet has contact payment history, and a client has to be able to hold Core SPV back until those addresses exist.
That ordering is a policy decision, and it had no home in the SDK. iOS implemented it in Swift (dashpay/dashwallet-ios#961) — a hand-rolled step sequencer with its own wall-clock budget and two poll loops. Review rejected it on the grounds that
packages/swift-sdk/CLAUDE.mdstates outright:This is that helper. Android would otherwise have had to reimplement the same sequence, and the identity-retry half of it had already regressed once while it lived client-side.
The step that is easy to miss. After a DashPay sync pass the contact accounts still do not exist: the recurring sweep runs unattended and holds no signer, so it cannot derive the receiving xpub or run the ECDH for a contact's external account — it only enqueues the work (
enqueue_deferred_contact_crypto). The accounts come into being when a signer-present drain runs. A sequence that stopped after the sync pass would be correctly ordered and still start SPV with nothing extra to watch. Field evidence from the iOS build before this: 350Deferred DashPay account build: enqueuedlines, 0 drains, andpayments projection — 0 row(s)on a wallet with 7 established contacts.What was done?
manager/startup.rs—PlatformWalletManager::start_wallet_subsystems:IdentityDiscoveryIncomplete. A scan that returned has an answer from Platform, and an empty answer is a proof of absence that rescanning cannot overturn — the distinction fix(platform-wallet): report an unanswered identity scan as incomplete, not empty #4352 made expressible, and this is its first consumer.drain_pending_contact_crypto, plusdrain_auto_acceptswhen an identity signer was supplied.Returns
Erronly forWalletNotFound. An unreachable Platform, a failed sync pass and an unfinished drain are all reported inWalletStartupOutcome, because failing loudly would trade a data gap for a wallet with no balance. Budget is a parameter, defaulting to 20s.StartupTallyholds the counters and the classification, mirroringScanTallyindiscovery.rs: the tests drive the same methods production does, so a miswiring fails a test instead of shipping.Key material stays per-call. The master xpriv and contact-crypto provider are borrowed for this call only, matching what
discover_from_masterand the drain already require.platform-wallet-ffi—platform_wallet_manager_start_wallet_subsystems+WalletStartupOutcomeFFI. Usesrun_on_big_stack_threadrather thanblock_on_worker: the manager is only reachable as a&PlatformWalletManagerborrowed from the handle store, so the future cannot satisfy the'staticbound. The scoped thread also supplies the 8 MB stack GroveDB proof verification needs. The master xpriv is erased withnon_secure_erasebefore every return path.swift-sdk—PlatformWalletManager.startWalletSubsystems(wallet:budget:gapLimit:), plusWalletStartupStatus.identityIsSettledso a caller never re-derives which outcomes are worth repeating.Considered and rejected
Making the unattended sweep resolve its own signer, which would have removed the need for any client sequencing at all.
MnemonicResolveris a Swift-owned vtable pinned to a single synchronous FFI call, anddrain_auto_acceptsneeds a fullSigner<IdentityPublicKey>, not just an ECDH/xpub derive. Turning either into a standing capability driven by a 15s background loop is a security-posture change, not a refactor.How Has This Been Tested?
8 unit tests in
manager::startup::tests, no I/O. The first pins the rule that already regressed once client-side: a scan that came back definitively empty settles asNoIdentityand is never retried, while one that never reached Platform reportsPartialNoIdentityand is. Others cover the warm-launch path costing zero scans, queued builds surfacing asPartialAccountsPending, and an unreachable Platform outranking a finished drain.End-to-end on a testnet restore (iOS 26.5 simulator, wallet with 7 established contacts), driven through the Swift wrapper:
reconcile_sent_payments_from_tx_historythen matched 18 txids and the payment projection went from 0 to 21 rows.no identity for this seed) and SPV starts immediately — no added launch latency where there is nothing to prepare.Not covered: the async body itself has no integration test. Driving it needs a mock SDK plus a registered wallet, and injecting a failing
Identity::fetchto exercise theIdentityDiscoveryIncompletebranch — I did not confirmSdkBuilder::new_mock()supports that.SeedCryptoProvider(contact_requests.rs) is a ready-made fixture for the provider half if someone picks this up.Breaking Changes
None — purely additive. No existing caller changes; the separate
startPlatformAddressSync/bindShielded/startDashPaySync/ DPNS entry points are untouched. Those were deliberately not subsumed: none has a "must exist before the first filter set" ordering requirement, and folding cheap idempotent flag-flips into a budget-bounded call would widen its blast radius for no benefit.Two notes for reviewers:
ensureConfigured,modelContainerandsignerNetworkon the SwiftPlatformWalletManagergo fromprivatetointernal, so the new extension reuses them rather than duplicating the gate and the signer construction.budgetof added latency before Core SPV starts. It self-skips when there is nothing to prepare, and on expiry falls through to exactly the previous behaviour.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit