fix(platform-wallet): register contact accounts via add_managed_account, dedup provider-key rebuild, and cleanup - #4587
Draft
Claudius-Maginificent wants to merge 8 commits into
Conversation
…etry Persistence failures on the wallet rehydration and registration paths were flattened into `PlatformWalletError::WalletCreation(String)`, destroying the transient/fatal classification callers need and severing the `#[source]` chain. Adds typed `PersisterLoad` / `PersisterStore` / `PersisterRestore` variants carrying the `PersistenceError` (boxed for the recursive restore case) and routes every persister boundary through them. On top of that, `retry_transient` (4 attempts, 20 -> 200 ms doubling backoff) now wraps persister `store` / `flush` / `load` on the registration, startup and identity-discovery paths, so a transient `SQLITE_BUSY` no longer aborts wallet registration outright or costs the identity-scan verdict its durability (#4365). Fatal errors still fail fast. The retry re-drives a failed `store` via a bare `flush`, which `PlatformWalletPersistence::store` now documents as a backend contract. Also fixes the persister leak behind #4133: a failed `load_from_persistor` left the wallet-event adapter holding an `Arc<P>` clone, so re-opening the same path returned a spurious `AlreadyOpen` masking the real error. `load_from_persistor` now shuts the manager down on both failure paths, with a `Drop` backstop cancelling and aborting the adapter task. `record_or_persister_or_log` and `reconcile_sent_payments` stop swallowing permanent read failures as "not found": transient errors still defer to the next sweep, permanent ones propagate as `PersisterLoad` instead of stalling an unbounded poll loop with no explanation. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
`From<PersistenceError>` flattened every persistence failure into the generic `ErrorWalletOperation` (6), so hosts lost the transient/fatal classification the wallet layer now preserves. Adds `ErrorPersisterFatal = 49` and `ErrorPersisterTransient = 50`, claimed from the registry's allocation frontier, and de-flattens the conversion: `PersisterLoad` / `PersisterStore` map on `is_transient()`, `PersisterRestore` unwraps to its typed inner error. Ships the full three-layer parity the registry mandates — Rust enum with a discriminant pin test, `ERROR_CODE_REGISTRY.md` rows 47-50 with the frontier moved to 51, the Swift `PlatformWalletResultCode` / `PlatformWalletError` mirrors, and Swift raw-value pins. The variants are declared in ascending discriminant order (49 then 50), the order both enums otherwise keep; the comment records why 50 is not 48. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
…counts Contact-account registration went through `accounts.insert_funds_bearing_account`, which does not bump the wallet's `account_generation`. The running filter scan therefore never picked up the new account's addresses — a contact's incoming payments stayed invisible until something else happened to invalidate the scan. Registration now goes through `ManagedAccountOperations::add_managed_account`, and `PlatformWalletInfo` forwards `account_generation()` to the core wallet so the invalidation is observable. Tests assert generation `1` after registering both a contact and an external account. `reconcile_dashpay_rescan` no longer bails on `synced_height == 0`. A zero checkpoint already means "scan from genesis", but bailing left candidates unmarked, so once that scan advanced the very same contacts triggered a redundant funding-height rewind. Candidates are now marked as covered and the height is left alone. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
…ndex `broadcaster.rs` deleted its hardcoded 30 s `SPV_ACCEPTANCE_TIMEOUT` and now passes `None` to `broadcast_and_wait`. The local override was shorter than dash-spv's own budget, so sends were reported `MaybeSent`/uncertain while the SPV layer was still legitimately waiting. `SendTransactionView` non-platform -> platform flows use `senderAccountIndex = 0` instead of "first key-class-0 account with a positive balance": that search returned a key-class Platform-Payment account index, which was then fed to `CoreTransactionBuilder.setFunding(accountType: .bip44, ...)` — a different namespace, so core -> core sends could draw on the wrong funding account. Also in this batch: `run_tests.sh` tolerates a CI runner with no user default keychain under `set -euo pipefail`; `now_secs()` moves to `util.rs` as `pub(crate)`; the shield-input-selection regression test re-seeds off `reserve()` rather than hardcoded balances so it survives fee-schedule changes; and doc comments are corrected (`Wallet::new_watch_only` -> `new_external_signable`, `derive_spent_utxos` defaults, restore-loop skip behaviour). <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
The FFI restore path carried its own copy of the provider-key account reconstruction, which had already drifted from the SQLite backend's. Both now call `platform_wallet::changeset::rebuild_provider_key_account`, so FFI and SQLite restore provider accounts identically (-54/+30). Ordering: `rebuild_provider_key_account` ships with the wallet-storage PR's required bucket (`changeset/changeset.rs` + `changeset/mod.rs`). Until that lands on the base branch this commit does not compile — the sole error is the unresolved import. Land the storage PR first. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lklimek
force-pushed
the
feat/platform-wallet-typed-persister-errors
branch
from
September 3, 2026 11:51
acaf86b to
1f5b3d2
Compare
Base automatically changed from
feat/platform-wallet-typed-persister-errors
to
v4.2-dev
September 9, 2026 13:17
…let-ffi-persister-codes-and-fixes Base carries the squashed forms of this branch's prerequisites (#4586 typed persister errors, #3968 storage backend, #4481 SPV timeout), so the pre-squash copies on this branch resolve to the merged versions: - 16afd79 (pre-squash #4586): error.rs, manager/{load,mod,startup, wallet_lifecycle}.rs, asset_lock/sync/proof.rs, identity/network/ {discovery,payments}.rs and changeset/traits.rs take the base. The branch's "store Transient MUST buffer" doc contradicted the base's store_transient_is_reissuable contract and is dropped. - f93aa3f (codes 49/50): superseded by the base's six-code 49-54 persister block (49 = LoadTransient, not Fatal). FFI error.rs, the registry and Swift PlatformWalletResult take the base; the stale Swift 49/50 test is removed, the code-26 raw-value pin is kept. - 40e04c2 (FFI provider-rebuild dedup): rebuild_provider_key_account landed as pub(super) in platform-wallet-storage, not in platform_wallet::changeset, and the FFI crate does not depend on the storage crate, so the import cannot resolve. FFI persistence.rs keeps the base's inline rebuild; dedup needs a follow-up relocation. Kept from this branch: contact-account add_managed_account generation fix and reconcile_dashpay_rescan zero-height change, reserve-derived shield regression fixture, now_secs dedup, doc and Swift cleanups. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e merge The v4.2-dev merge took the base's permanent-read-failure sweep in reconcile_sent_payments, whose catch-all `Err(e)` records the failure and continues. The pre-squash arm that returned `PersisterLoad` immediately sat outside the conflict block and survived as dead code (unreachable pattern warning). No behavior change: the arm could never match. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… storage and ffi
Provider key-material accounts (BLS ProviderOperatorKeys / EdDSA
ProviderPlatformKeys) were rebuilt by two copies of the same logic: a
pub(super) helper private to platform-wallet-storage's sqlite module and
an inline copy in platform-wallet-ffi's build_wallet_start_state. The FFI
crate does not depend on the storage crate, so the canonical helper now
lives in platform-wallet (a dependency of both) as
platform_wallet::changeset::provider_key_account::{
rebuild_provider_key_account, ProviderAccountRebuildError}, gated on the
bls/eddsa features that make its variants exist.
No behavior change:
- storage keeps its Invalid -> AccountRecordInvalid and
Rejected -> ProviderKeyAccountEntryMismatch mapping;
- ffi keeps its bincode decode (and the unmaintained-bincode-decoder
note), and maps helper errors to byte-identical Fatal
PersistenceError::backend messages.
Characterization tests pin both call sites before the move:
build_wallet restoring/rejecting provider manifest entries (storage) and
build_wallet_start_state restoring both provider accounts from
bincode-encoded specs (ffi). The helper's own unit tests move with it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claudius-Maginificent
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR: Registers new contact-account payment accounts through
add_managed_accountso they're covered by the wallet's address scan, deduplicates the provider-key-account rebuild logic betweenplatform-wallet-storageandplatform-wallet-ffi, and carries a few small cleanups.User story
As a DashPay user, I want addresses on a newly accepted contact's payment account to be covered by the wallet's ongoing address scan, so incoming payments to that account aren't missed.
As a platform-wallet maintainer, I want provider-key-account rebuild logic to exist in exactly one place, so storage and FFI can't drift out of sync.
Scenario
Base flow
A user accepts a contact request and the wallet derives a payment account for that contact; separately, both
platform-wallet-storageandplatform-wallet-ffiindependently reconstruct provider (BLS/EdDSA) key accounts when rehydrating wallet state.Actual behavior (before this PR)
Accepting a contact request added its payment account without invalidating the wallet's filter-scan generation, so the account's addresses were never covered by the running blockchain scan. Provider-key-account rebuild logic was duplicated between the storage crate and the FFI crate.
Expected behavior
Contact-account registration now goes through
ManagedAccountOperations::add_managed_account, which bumps the wallet's scan generation so new addresses are covered — note this means accepting a contact now triggers a full-history rescan instead of a funding-height rewind; please sign off on this scan-cost trade-off explicitly. Provider-key-account rebuild now lives once, inplatform_wallet::changeset::provider_key_account, called by bothplatform-wallet-storageandplatform-wallet-ffi.Detailed discussion
What was done
contacts.rs/platform_wallet_traits.rs/payments.rs— registers new contact accounts viaadd_managed_account; removes a dead-endsynced_height == 0early return inreconcile_dashpay_rescanthat skipped marking contacts covered.packages/rs-platform-wallet/src/changeset/provider_key_account.rsexposesrebuild_provider_key_account+ProviderAccountRebuildError, called from bothrs-platform-wallet-storage/src/sqlite/provider_accounts.rs(rehydrate path) andrs-platform-wallet-ffi/src/persistence.rs(restore path). No behavior change — characterization tests written before the refactor confirm identical outcomes (error strings, error kind, and success paths) on both callers.broadcaster.rs(drop hardcoded 30s SPV timeout),SendTransactionView.swift(fix a Platform-Payment account index leaking into the BIP44 Core account namespace on core→core sends),run_tests.sh(tolerate a CI runner with no keychain),util.rs+contact_requests.rs(now_secs()dedup),platform_wallet.rsshield-regression fixture,changeset/core_bridge.rsdoc,wallet_lifecycle.rsnew_watch_only→new_external_signabledoc rename, Swift doc-only files.What changed after merging
origin/v4.2-devorigin/v4.2-devalready merged fix(platform-wallet): typed persister errors with caller-visible retry classification #4586, which owns FFI codes 49–54 (LoadTransient=49, LoadFatal=50, StoreTransient=51, StoreFatal=52, StoreConstraint=53, Restore=54) — the opposite meaning at 49 from this PR's originalErrorPersisterFatal=49. Keeping the PR's codes would have been a duplicate discriminant and a silent Swift ABI mismatch.platform_wallet::changeset::rebuild_provider_key_accountas landed by an earlier feat(platform-wallet-storage): embeddable SQLite persistence backend with seedless rehydration #3968 draft) waspub(super)-private to the storage crate. Redone from scratch post-merge in its final, working form (see above).Testing
cargo nextest run -p platform-wallet -p platform-wallet-storage -p platform-wallet-ffi: 2390 passed, 0 failed, 7 ignored — includes contact-account tests (register_contact_account_persists_account_registration,register_external_with_precomputed_shared_key_builds_account,rescan_is_a_noop_when_synced_height_is_zero,rescan_uses_min_funding_across_contacts_and_drip_feed_settles) and the new provider-key-account characterization/rebuild tests.cargo clippy -p platform-wallet -p platform-wallet-storage -p platform-wallet-ffi --all-targets: clean.cargo fmt --check: clean.xcodebuild/run_tests.shin CI.Breaking changes
None to the public FFI ABI's meaning — provider-rebuild dedup is behavior-preserving; contact-account registration change is a behavior improvement (fixes missed scan coverage) with a scan-cost trade-off flagged above for reviewer sign-off.
Checklist
Known unrelated pre-existing issue
packages/rs-drive/src/query/mod.rs:1importsDocumentPropertyTypeunconditionally, but its only use is behind#[cfg(feature = "server")]; any-D warningsbuild ofdrivewithoutserverfails onunused_imports(introduced byd6ab675124). Not touched by this PR; worth a separate small fix.Prior work
Originally split out of #3968 as part of a coordinated 4-PR split alongside #4586 (typed persister errors, now merged) and #4585 (asset-lock size gate, merged). See #3968 for the full rationale.
🤖 Generated with Claude Code