refactor(platform-wallet): share provider-key account reconstruction - #4587
Merged
lklimek merged 14 commits intoSep 15, 2026
Merged
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (16)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughChangesProvider account rebuild
Wallet core maintenance
Swift SDK updates
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Refactor Sequence Diagram(s)sequenceDiagram
participant WalletRestore
participant rebuild_provider_key_account
participant AccountCollection
WalletRestore->>rebuild_provider_key_account: provide wallet ID, account type, and provider extended public key
rebuild_provider_key_account->>AccountCollection: create and insert watch-only BLS or EdDSA account
AccountCollection-->>WalletRestore: return success or typed rebuild error
Merge Risk: ⚪ Minimal · up to The changed assertions preserve the established indentation and no actionable current-head risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
…y_account consumers platform-wallet-storage and platform-wallet-ffi both call into platform-wallet::changeset::provider_key_account, whose contents are gated #[cfg(any(feature = "bls", feature = "eddsa"))]. Both crates only picked this up via platform-wallet's inherited default features (default = ["bls", "eddsa"]), so a platform-wallet built with default-features = false would break both consumers' compile instead of cleanly dropping the gated module. Declare the requirement explicitly on each crate's platform-wallet dependency. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…h platform-wallet's rebuild tests sqlite::provider_accounts::tests and platform_wallet::changeset::provider_key_account::tests each carried a byte-identical copy of provider_key_test_wallet. Promote the platform-wallet copy to a pub fn gated #[cfg(any(test, feature = "test-utils"))], and have platform-wallet-storage's tests pull it in via a platform-wallet dev-dependency with the test-utils feature, instead of keeping a second copy that can drift. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…let-ffi-persister-codes-and-fixes # Conflicts: # packages/rs-platform-wallet-ffi/src/persistence.rs
…d refactor Move contact scan registration, Swift funding selection, and CI keychain behavior to the stacked fix PR. Keep provider account reconstruction, unchanged helper extraction, documentation, and characterization tests. Co-Authored-By: Codex <noreply@openai.com>
lklimek
changed the title
4 tasks
lklimek
marked this pull request as ready for review
September 14, 2026 14:09
lklimek
requested review from
HashEngineering,
ZocoLini,
lklimek,
llbartekll and
romchornyi
as code owners
September 14, 2026 14:09
Collaborator
|
✅ Final review complete — no blockers (commit 8e2f880) · triage: critical |
5 tasks
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The PR cleanly centralizes provider-key account reconstruction without changing persistence behavior or the FFI ABI. Two minor documentation and test-maintenance issues remain: the exported FFI documentation still describes restored wallets as watch-only, and new restore tests duplicate a shared fixture instead of using it.
💬 2 nitpick(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
💬 Nitpick: Correct the exported load documentation to describe restored wallets as external-signable
packages/rs-platform-wallet-ffi/src/manager.rs:646-647
This comment is exported verbatim into the generated C header because cbindgen.toml enables documentation = true. It still says that restored wallets are reconstructed as watch-only, but the load path constructs them with Wallet::new_external_signable(...) and restores signing through the host signer surface. The same stale wording was corrected at sibling documentation sites in this PR, so leaving this copy unchanged makes the public FFI documentation contradict the actual behavior.
/// reconstructs each wallet as **external-signable** via its stored root +
/// per-account xpubs, and registers them inside the manager. Does not
source: glm-5.3-flash (phase1-reviewer: general, ffi-engineer, rust-quality)
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: ffi-engineer); reviewer 3: glm-5.3-flash (agent: phase1-reviewer, role: rust-quality); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
criticalbygpt-6-astra(effort low) — The substantial cross-backend refactor changes provider-key account reconstruction and restoration in wallet persistence, directly involving cryptographic key handling and persisted wallet state across FFI and SQLite paths. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort max); agentphase1-reviewer,glm-5.3-flash— ffi-engineer (completed, effort max); agentphase1-reviewer,glm-5.3-flash— rust-quality (completed, effort max); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 85% left, weekly 93% left; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 11% left, 5h 100% left) - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/manager.rs`:
- [NITPICK] packages/rs-platform-wallet-ffi/src/manager.rs:646-647: Correct the exported load documentation to describe restored wallets as external-signable
This comment is exported verbatim into the generated C header because `cbindgen.toml` enables `documentation = true`. It still says that restored wallets are reconstructed as watch-only, but the load path constructs them with `Wallet::new_external_signable(...)` and restores signing through the host signer surface. The same stale wording was corrected at sibling documentation sites in this PR, so leaving this copy unchanged makes the public FFI documentation contradict the actual behavior.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [NITPICK] packages/rs-platform-wallet-ffi/src/persistence.rs:9122-9126: Use the shared provider-key wallet fixture in the new FFI restore tests
The new tests inline the same `Wallet::from_seed_bytes(..., WalletAccountCreationOptions::Default)` construction that the shared `provider_key_test_wallet()` fixture was introduced to centralize. The fixture pins the options that guarantee both provider accounts exist; using it here and in the corresponding SQLite tests prevents these assumptions from drifting independently when upstream wallet defaults change.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- platform-wallet fails to compile under partial bls/eddsa feature sets — The platform-wallet crate itself still has an existing partial-feature compilation failure because
AccountType,BLSAccount, andEdDSAAccountimports are not cfg-gated consistently with their consumers. The referenced file is byte-identical at the PR base and head, so this is not caused by this PR, but it is a concrete follow-up for consumers that need curve-specific feature combinations.- Follow-up: Track separately: cfg-gate the imports and add partial-feature platform-wallet checks to CI.
Comment on lines
+9122
to
+9126
| let wallet = Wallet::from_seed_bytes( | ||
| [0x42; 64], | ||
| Network::Testnet, | ||
| key_wallet::wallet::initialization::WalletAccountCreationOptions::Default, | ||
| ) |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: Use the shared provider-key wallet fixture in the new FFI restore tests
The new tests inline the same Wallet::from_seed_bytes(..., WalletAccountCreationOptions::Default) construction that the shared provider_key_test_wallet() fixture was introduced to centralize. The fixture pins the options that guarantee both provider accounts exist; using it here and in the corresponding SQLite tests prevents these assumptions from drifting independently when upstream wallet defaults change.
Suggested change
| let wallet = Wallet::from_seed_bytes( | |
| [0x42; 64], | |
| Network::Testnet, | |
| key_wallet::wallet::initialization::WalletAccountCreationOptions::Default, | |
| ) | |
| let wallet = | |
| platform_wallet::changeset::provider_key_account::provider_key_test_wallet(); |
source: glm-5.3-flash (phase1-reviewer: general, ffi-engineer, rust-quality)
llbartekll
approved these changes
Sep 15, 2026
llbartekll
left a comment
llbartekll
left a comment
Contributor
There was a problem hiding this comment.
LGTM. Pure refactor: the SQLite-side rebuild_provider_key_account moves into platform-wallet::changeset::provider_key_account unchanged, and the FFI load path now calls it with its original error messages preserved. Verified the new match arms carry the same bls/eddsa gates as the ProviderKeyExtendedPubKey variants, so it stays exhaustive under any feature combo, and the explicit feature declarations on the storage/FFI deps close the default-features = false hole. Round-trip and curve-mismatch tests now cover both backends. CI clippy + nextest (--all-features) green on 8e2f880.
lklimek
deleted the
feat/platform-wallet-ffi-persister-codes-and-fixes
branch
September 15, 2026 07:24
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
TL;DR: Share provider-key account reconstruction between SQLite storage and FFI without changing wallet behavior.
What was done?
Detailed discussion
Both persistence backends now call the same BLS/EdDSA account-rebuild helper in
platform-wallet. Constructor behavior, account insertion, backend-specific error mappings, and the base branch's bounded decoding are preserved.This PR also extracts the unchanged wall-clock-seconds helper, clarifies existing behavior in comments, and retains characterization tests and a fee-independent test fixture. It does not change contact-account registration, scan coverage, transaction funding selection, or CI keychain handling; those behavior fixes belong to the follow-up stacked PR #4740.
How Has This Been Tested?
cargo +1.98 fmt --check -p platform-wallet -p platform-wallet-storage -p platform-wallet-ffi: passed.cargo clippy -p platform-wallet -p platform-wallet-storage -p platform-wallet-ffi --all-targets --all-features --locked -- --no-deps -D warnings: passed (CI flags).--all-features: passed.1.98; the merged base pins1.98.1. Swift assertions were inspected but not run: no macOS toolchain.Breaking Changes
None. No public FFI ABI or persisted-format changes.
Checklist
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
Bug Fixes
Documentation