feat(platform-wallet): SPV cancel_background/identity_ids accessors + FFI no-selectable-inputs error mapping#3651
Conversation
Carve-out of clusters D, E, K from #3549 (e2e framework PR) as an independent production change on v3.1-dev. D — SPV runtime accessors: SpvRuntime::cancel_background() (sync cancellation-token fire for panic-hook data-dir-lock release) and SpvRuntime::event_manager(); IdentityManager::identity_ids() snapshot. E — FFI error mapping: dedicated ErrorNoSelectableInputs=14 (and reserved ErrorArithmeticOverflow=13); NoSpendableInputs / OnlyOutputAddressesFunded / OnlyDustInputs now map to the dedicated FFI code instead of flattening to ErrorUnknown. K — DAPI dispatch trace log: one tracing::trace! emitting the resolved endpoint/method. Pure observability. E's match arm consumes three PlatformWalletError variants that live in the still-open #3554 (OnlyOutputAddressesFunded, OnlyDustInputs) and #3585 (NoSpendableInputs). Per maintainer decision this PR is independent on v3.1-dev and copies those variant definitions verbatim so it compiles standalone; duplicate definitions are expected and resolved at merge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR extends wallet error handling with three new input selection failure types, maps them through FFI bindings with dedicated error codes, adds a background task cancellation method to SpvRuntime, provides an identity identifier accessor, and enhances DAPI request dispatch tracing. ChangesWallet Infrastructure Enhancements
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
✅ Review complete (commit 66a1a64) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I verified the flagged changes against b8ccad8d277fb46882466adecd958af0ccd126c2. The FFI error remapping is real and the added tests are directionally correct, but the new SpvRuntime::cancel_background() API does not satisfy the restart-after-panic contract described in its own docs and is not safe as a panic-hook escape hatch. One additional test-coverage gap remains in the new FFI error-code contract.
Reviewed commit: b8ccad8
🔴 1 blocking | 🟡 1 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/spv/runtime.rs`:
- [BLOCKING] lines 182-200: `cancel_background()` is not a reliable panic-hook recovery path
This method is documented as the synchronous escape hatch for panic-hook contexts that need the dash-spv data-dir lock released before the next init attempt, but the implementation only removes the stored token and calls `token.cancel()`. The actual teardown still happens later on the spawned task's async path after `run()` leaves the `select!` and reaches `self.stop().await`, so a caller can return from `cancel_background()` and race straight into a new `start()` while the old client still owns the lock. On top of that, this panic-path API acquires `background_cancel` with `.expect("background_cancel poisoned")`, which will panic again if the mutex was poisoned by an earlier unwind. As written, the method neither guarantees lock release before re-init nor stays usable in the failure mode it was added to handle.
In `packages/rs-platform-wallet-ffi/src/error.rs`:
- [SUGGESTION] lines 405-444: The new FFI test does not cover all three variants mapped to `ErrorNoSelectableInputs`
The production mapping at lines 176-180 explicitly groups `NoSpendableInputs`, `OnlyOutputAddressesFunded`, and `OnlyDustInputs` under `ErrorNoSelectableInputs`, and the enum docs say callers distinguish the cause from the preserved message text. The new test only constructs `NoSpendableInputs`, so the two payload-heavy variants with their own `Display` output are still unpinned. A future edit could break one of those user-visible messages or accidentally remap one variant while this test still passes.
…(superseded by #3549) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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-ffi/src/error.rs`:
- Around line 82-90: The doc comment for ErrorNoSelectableInputs has the
parentheticals for NoSpendableInputs and OnlyOutputAddressesFunded reversed;
update the parentheses so NoSpendableInputs reads "(account has nothing
spendable)" and OnlyOutputAddressesFunded reads "(every funded address is also a
destination)" while leaving OnlyDustInputs and the rest of the description
unchanged (refer to ErrorNoSelectableInputs, NoSpendableInputs,
OnlyOutputAddressesFunded, OnlyDustInputs to locate the text).
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ed43d77-2176-4f5c-b362-9473fbb82092
📒 Files selected for processing (5)
packages/rs-dapi-client/src/dapi_client.rspackages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/spv/runtime.rspackages/rs-platform-wallet/src/wallet/identity/state/manager/accessors.rs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3651 +/- ##
============================================
- Coverage 88.06% 88.05% -0.01%
============================================
Files 2521 2521
Lines 308995 308781 -214
============================================
- Hits 272122 271908 -214
Misses 36873 36873
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I verified the reported issues against 66a1a64abd500691d97b49a5197b9eccd41acc0c. Three findings hold: the new SPV panic-path cancellation API does not provide the synchronous recovery its docs promise, the new Rust-side FFI code is not mirrored through Swift, and the added Rust test does not cover all three variants that now share the dedicated FFI code.
Reviewed commit: 66a1a64
🔴 1 blocking | 🟡 2 suggestion(s)
1 additional finding
🟡 suggestion: Swift no longer mirrors the Rust FFI result-code enum, so `ErrorNoSelectableInputs` is downgraded to `.errorUnknown`
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift (lines 7-58)
Rust now exports ErrorArithmeticOverflow = 13 and ErrorNoSelectableInputs = 14, and the FFI conversion returns ErrorNoSelectableInputs for NoSpendableInputs, OnlyOutputAddressesFunded, and OnlyDustInputs. The Swift mirror still stops at errorUtf8Conversion = 12 and falls through to default for any newer value, so code 14 is converted to .errorUnknown before PlatformWalletError(result:) builds the thrown error. The message string still survives, but the dedicated ABI code added in this PR does not survive the Rust-to-Swift boundary.
🤖 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/spv/runtime.rs`:
- [BLOCKING] lines 182-201: `cancel_background()` cannot guarantee the lock-release behavior its panic-hook docs promise
This method only takes the stored token and calls `cancel()`. It does not wait for the spawned `run()` task to leave the `select!`, reach `self.stop().await`, and release the underlying SPV client and storage lock. That means a panic-hook caller can return from `cancel_background()` and immediately race into a new `start()` while the old task still owns the data-dir lock, which is the exact scenario the doc comment says this API handles. The same panic-path API also uses `.lock().expect("background_cancel poisoned")`, so if the mutex was poisoned by the unwind that triggered the hook, the recovery path panics again instead of cancelling anything.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift`:
- [SUGGESTION] lines 7-58: Swift no longer mirrors the Rust FFI result-code enum, so `ErrorNoSelectableInputs` is downgraded to `.errorUnknown`
Rust now exports `ErrorArithmeticOverflow = 13` and `ErrorNoSelectableInputs = 14`, and the FFI conversion returns `ErrorNoSelectableInputs` for `NoSpendableInputs`, `OnlyOutputAddressesFunded`, and `OnlyDustInputs`. The Swift mirror still stops at `errorUtf8Conversion = 12` and falls through to `default` for any newer value, so code 14 is converted to `.errorUnknown` before `PlatformWalletError(result:)` builds the thrown error. The message string still survives, but the dedicated ABI code added in this PR does not survive the Rust-to-Swift boundary.
In `packages/rs-platform-wallet-ffi/src/error.rs`:
- [SUGGESTION] lines 405-434: The new FFI test covers only one of the three variants mapped to `ErrorNoSelectableInputs`
`From<PlatformWalletError>` maps `NoSpendableInputs`, `OnlyOutputAddressesFunded`, and `OnlyDustInputs` to the same FFI code, but `no_selectable_inputs_maps_to_dedicated_code()` exercises only `NoSpendableInputs`. The other two variants carry different payloads and different `Display` text, so a later refactor could break one of those mappings or its user-visible message while this test still passes. The dedicated-code contract added in this PR should be pinned for all three variants.
Issue being fixed or feature implemented
Independent carve-out of three small production clusters (D, E, K) extracted from #3549 (the e2e-framework PR), so they can land on
v3.1-devwithout waiting for the large e2e suite to be ready. No companion issue — this PR is the unit of work tracking.D — SPV runtime + identity accessors (
spv/runtime.rs,wallet/identity/state/manager/accessors.rs)SpvRuntime::cancel_background()— synchronously fires the backgroundrun()task's cancellation token so a sync context (e.g. astd::panic::set_hookcallback) can release the dash-spv data-dir lock before the next init attempt without blocking the panicking thread. Idempotent.IdentityManager::identity_ids()— snapshot of every managed identity'sIdentifieracross both buckets (order unspecified).SpvRuntime::event_manager()accessor originally part of this cluster has been removed (commit66a1a64abd) — it is superseded by its deletion in test(rs-platform-wallet): e2e suite, Found-025 fix + triage pins #3549 and is no longer needed here.E — FFI error mapping (
rs-platform-wallet-ffi/src/error.rs)ErrorArithmeticOverflow = 13(reserved, ABI placeholder — currently unused) andErrorNoSelectableInputs = 14.NoSpendableInputs/OnlyOutputAddressesFunded/OnlyDustInputsnow map to the dedicatedErrorNoSelectableInputscode instead of flattening toErrorUnknown; the typedDisplayrendering survives in the result message so callers can distinguish the underlying cause. Catch-allErrorUnknownretained for unmapped variants.no_selectable_inputs_maps_to_dedicated_code) and the catch-all fall-through (unmapped_variants_fall_through_to_unknown).E (supporting) — copied error variants (
rs-platform-wallet/src/error.rs)NoSpendableInputs,OnlyOutputAddressesFunded,OnlyDustInputsplus their imports — definitions byte-identical to the canonical fix(platform-wallet): auto_select_inputs honors Σ inputs == Σ outputs #3554/fix: case-insensitive .dash + atomic state on broadcast failure #3585 sources, copied so D+E+K compiles standalone.K — DAPI dispatch trace log (
rs-dapi-client/src/dapi_client.rs)tracing::trace!emitting the resolved DAPI endpoint/method/request type. Pure observability, no behavior change.What was done?
Carved clusters D, E, K out of #3549 as an independent production change targeting
v3.1-dev.Removed scope vs. the original carve-out:
SpvRuntime::event_manager()was dropped (commit66a1a64abd) because #3549 deletes that accessor outright — keeping it here would be dead/superseded surface. The retained D accessors arecancel_background()andidentity_ids().This PR is intentionally independent on
v3.1-dev(not stacked on #3554/#3585) per an explicit operator directive given during the carve-out session — "Create an independent PR on top of v3.1-dev and copy these new errors there. We'll handle conflicts during merge." The commit-message wording "Per maintainer decision this PR is independent on v3.1-dev" is inaccurate and is corrected here at the PR-narrative level: there was no dashpay/platform maintainer decision; the true provenance is the in-session operator directive quoted above. History was deliberately not rewritten so the immutable commit record stays intact.The error-enum variants
NoSpendableInputs(from #3585),OnlyOutputAddressesFundedandOnlyDustInputs(from #3554) are copied verbatim and deliberately duplicated so D+E+K compiles standalone. These definitions WILL collide with #3554/#3585 at merge time — this is expected and accepted; the conflicts are resolved at merge (the definitions are identical, so resolution is mechanical). A human maintainer should explicitly ratify this deliberate-duplication strategy before this PR is merged.How Has This Been Tested?
cargo build -p platform-wallet-ffi -p platform-wallet -p rs-dapi-client— clean.cargo clippy -p platform-wallet-ffi -p platform-wallet -p rs-dapi-client— zero diagnostics on changed code (only pre-existing workspaceCargo.tomlprofile warnings, present on untouched base).cargo test -p platform-wallet-ffi --lib error— all passed (incl.no_selectable_inputs_maps_to_dedicated_code,unmapped_variants_fall_through_to_unknown).rustfmt --edition 2021on each touched file.Breaking Changes
None. Additive: new error variants, new FFI result codes (existing codes unchanged, new codes 13/14), new public accessors, one trace log. The
event_manager()accessor removal has no external effect — the accessor never shipped onv3.1-dev.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
🤖 Co-authored by Claudius the Magnificent AI Agent