fix(platform-wallet): report an unanswered identity scan as incomplete, not empty - #4352
Merged
Merged
Conversation
…e, not empty `discover_inner` counted a failed `Identity::fetch` as a gap-limit miss, identical to a genuine "no identity at this index". With `IDENTITY_GAP_LIMIT = 5`, a few seconds of network trouble ended the scan with `Ok(vec![])` — which at the FFI boundary is indistinguishable from "this seed owns no identity". That is not a rare shape. Every probe verifies its GroveDB proof against quorum keys from a single HTTPS endpoint whose LRU cache is cold on each process start, so all five DAPI retries and all six index probes fail together when it is slow. dashwallet runs the scan once, immediately after restore-from-seed, and records an empty success as final for the session: the restored identity — and with it every DashPay contact and all contact payment history — stayed hidden until the app was relaunched. Count unanswered probes apart from real misses. They still advance the miss counter so a scan terminates when Platform is unreachable, but a scan that ends with nothing found and at least one unanswered probe now returns `IdentityDiscoveryIncomplete` instead of an empty success. Callers retry on error and cache an empty success, so this alone lets a transient failure heal on the next attempt. A scan that did find something keeps returning its identities — they are already persisted — with a warning that a failed index may still hide one.
…inction Name the rule the previous commit introduced and cover it. The whole defect was that two different endings — "no identity exists" and "no answer came back" — shared one counter and one return value, so the condition deserves to be a named predicate with the regression written down next to it rather than an inline `&&` nobody can grep for.
romchornyi
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
August 9, 2026 21:39
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughIdentity discovery now records failed Platform probes separately from empty results. Empty scans with unanswered probes return ChangesIdentity discovery reliability
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant IdentityDiscovery
participant Platform
Caller->>IdentityDiscovery: start identity scan
IdentityDiscovery->>Platform: probe identity indices
Platform-->>IdentityDiscovery: responses or failed probes
IdentityDiscovery->>Caller: persisted discoveries or IdentityDiscoveryIncomplete
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
5 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/discovery.rs`:
- Around line 342-353: Track a separate found_identity flag in the discovery
scan, setting it for every successful Ok(Some(identity)) probe regardless of
whether is_new adds to discovered. Update scan_result_is_trustworthy and all
corresponding result paths to use found_identity while continuing to return
discovered unchanged. Add a regression test covering a known identity followed
by a failed probe, verifying the scan is not marked incomplete.
🪄 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: a9050b06-6ed8-4f5d-b76b-7073f5e6359e
📒 Files selected for processing (2)
packages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/wallet/identity/network/discovery.rs
Collaborator
|
✅ Final review complete — no blockers (commit 3876477) |
romchornyi
force-pushed
the
fix/identity-discovery-error-vs-miss
branch
from
August 9, 2026 21:45
c1cf523 to
13067c9
Compare
…dentities `scan_result_is_trustworthy` was fed `discovered.len()`, but `discovered` collects only identities the manager did not already track — the push is guarded by `is_new`. A rescan from index 0 therefore re-confirms a known identity without adding anything to that list, so a single failed probe at a later index made the scan report itself incomplete even though Platform had plainly answered. That is the app's "Find identities" command: it always scans from 0, and for a wallet whose identity is already known it would have started returning an error instead of "no new identities were found". Count every index Platform answered with an identity and judge by that. Regression test asserts both directions — the sighting count passes where the `discovered` length would have failed. Reported by CodeRabbit on #4352.
thepastaclaw
reviewed
Aug 9, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The scan now correctly distinguishes unanswered probes from confirmed misses and uses all identity sightings, including already-managed identities, when deciding whether a result is trustworthy. Two non-blocking improvements remain: preserve the underlying SDK error for Rust callers and test the production bookkeeping rather than only its final predicate.
Source: reviewers codex-general (gpt-5.6-sol) and codex-rust-quality (gpt-5.6-sol); final verifier codex (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) - 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)
🟡 2 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/error.rs:68-69: Preserve the typed SDK error as the incomplete scan's source
Converting the last `dash_sdk::Error` to a `String` discards its variant and prevents Rust callers from inspecting the underlying failure or using its retry classification where applicable. This matters because the new error's contract tells callers to retry, while fetch failures can also represent non-transient configuration, protocol, or proof failures. Store the last error as a boxed `dash_sdk::Error` marked with `#[source]`; `thiserror` will continue rendering it for the existing FFI message while preserving structured diagnostics for Rust callers.
In `packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs:846-878: Exercise the production scan bookkeeping instead of only its predicate
These tests manually supply `identities_seen` and `failed_probes`, so they verify the predicate but not the `discover_inner` bookkeeping that feeds it. They would still pass if production stopped incrementing `failed_probes`, counted only newly discovered identities, or passed `discovered.len()` again—the exact wiring defect fixed by the final commit. The crate already enables `dash-sdk`'s `mocks` feature for tests; drive the real scan with mocked found, missing, and failed fetches, or extract the bookkeeping into a state object that the scan and tests both exercise.
…an drive Addresses both suggestions on #4352. **Keep the probe failure typed.** `IdentityDiscoveryIncomplete` carried the last error as an already-rendered `String`, so a Rust caller could only parse the message back. The variant now holds `Box<dash_sdk::Error>` as `#[source]`: `thiserror` still renders it into the message the FFI boundary logs, and callers get the variant. This matters because the error's contract is "retry", while a probe can also fail on configuration, protocol or proof errors that retrying will not fix — classifying that needs the typed error. **Test the bookkeeping, not just the verdict.** The counters were locals in `discover_inner` and the tests asserted on a bare predicate, so they kept passing no matter what the scan fed it — including the exact `discovered.len()` wiring defect the previous commit fixed. The counters and the verdict move into `ScanTally`, which the scan and the tests now both drive: `run_scan` replays a sequence of probe outcomes through the same methods and the same gap-limit loop condition production uses. Verified by mutation: dropping `identities_seen += 1` fails three tests, dropping `failed_probes += 1` fails three others. Under the old tests both mutations passed. Adds coverage for the gap-limit behaviour that had none: a sighting resets the miss run, unanswered probes still terminate the scan, and the error carries its source.
thepastaclaw
approved these changes
Aug 10, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The current head correctly distinguishes unanswered identity probes from confirmed misses, preserves termination during network failure, and judges reachability using every identity sighting rather than only newly discovered identities. Both prior suggestions are fixed: the incomplete-scan error retains the typed SDK failure as its source, and production plus tests now share the same ScanTally bookkeeping methods; no in-scope findings remain.
Source: reviewers gpt-5.6-sol (general) and gpt-5.6-sol (rust-quality); final verifier 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) - 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)
QuantumExplorer
approved these changes
Aug 10, 2026
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
A gap-limit identity scan has three possible endings — it found identities, it confirmed there are none, or it never got an answer — but only reported two. In
discover_inner, a failedIdentity::fetchincremented the sameconsecutive_missescounter as a genuinely empty index:With
IDENTITY_GAP_LIMIT = 5, a scan that reached nobody at all returnedOk(vec![])— at the FFI boundary indistinguishable from "this seed owns no identity".That failure shape is correlated, not rare. Every probe verifies its GroveDB proof against quorum keys that
TrustedHttpContextProviderpulls from a single HTTPS endpoint (quorums.<network>.networks.dash.org) whose LRU cache is cold on each process start, so all fivers-dapi-clientretries and all six index probes fail together when it is slow or unreachable.The consequence lands downstream: dashwallet-ios runs this scan once, during runtime start immediately after restore-from-seed, and caches an empty success as final for the session. A few seconds of network trouble hid an already-registered identity — and with it every DashPay contact and all contact payment history — until the app was relaunched.
What was done?
packages/rs-platform-wallet/src/wallet/identity/network/discovery.rsfailed_probes,last_probe_error) separately from real misses. They still advanceconsecutive_misses, so a scan against an unreachable Platform still terminates.scan_result_is_trustworthy(discovered, failed_probes): emptiness is only trustworthy when every probe was answered; a scan that found something is trustworthy either way.tracing::warn!that a failed index may hide one until the next scan.packages/rs-platform-wallet/src/error.rsPlatformWalletError::IdentityDiscoveryIncomplete { start_index, probed, failed_probes, last_error }. It falls through the FFI's existing_ => ErrorUnknownarm, so no FFI mapping change is needed — callers that retry onErrand cache onOknow do the right thing without any client change.No signature or ABI change:
discover_identitiesstill returnsResult<Vec<Identity>, PlatformWalletError>.How Has This Been Tested?
Three unit tests added to the existing
discovery::testsmodule, covering the distinction the bug collapsed:empty_scan_with_unanswered_probes_is_not_trustworthy— the regression itselfempty_scan_with_every_probe_answered_is_trustworthyscan_that_found_something_is_trustworthy_despite_failuresdiscover_inneritself needs a live SDK and wallet manager, so the decision it makes is tested through the extracted predicate rather than by mocking the scan — consistent with the pure-function tests already in that module.Not yet verified on-device. The failure is timing-dependent by nature; confirming the end-to-end fix needs a testnet restore-from-seed against a slow or blocked quorum endpoint, together with the matching dashwallet-ios change (dashpay/dashwallet-ios#950), which also stops caching an empty scan as final.
Breaking Changes
None to the API surface. One behavioural change worth calling out: a caller that previously received
Ok(vec![])from a scan that could not reach Platform now receivesErr(IdentityDiscoveryIncomplete). That is the point of the change — callers must be able to tell "none exist" from "we don't know" — but any caller treating everyErras fatal rather than retryable should be checked.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit