refactor(platform-wallet): remove dead code in rs-platform-wallet (16 audit entries) - #4637
Open
llbartekll wants to merge 16 commits into
Open
refactor(platform-wallet): remove dead code in rs-platform-wallet (16 audit entries)#4637llbartekll wants to merge 16 commits into
llbartekll wants to merge 16 commits into
Conversation
…eStrategyByAddress `transfer_with_change_address` and the whole address-keyed fee-strategy machinery (`FeeStrategyByAddress`, `FeeStrategyStepByAddress`, `FeeStrategyResolveError`) had no consumer — not in rs-platform-wallet-ffi, swift-sdk, kotlin-sdk or rs-unified-sdk-jni. The FFI entry point `platform_address_wallet_transfer` only ever calls `transfer`. Going with them: `validate_change_address`, `augment_outputs_with_change`, `checked_sum_credits`, the `ChangeBelowMinimumOutput` error variant (never mapped in ffi/error.rs), and ~480 lines of tests that covered only this path. `InputSumOverflow` stays — platform_wallet.rs still uses it. The `transfer` doc loses its "When to use this vs transfer_with_change_address" section. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ncoming_dashpay_address variants The three public variants (`match_incoming_dashpay_address`, `_blocking`, `try_match_incoming_dashpay_address`) had no call site — not from Rust, and not through the FFI, Swift or Kotlin. One of them (`_blocking`) panics when called from a tokio context. The only live consumer was `payments.rs`, reaching the private `match_in_collection` helper through an artificial `DashPayView::<SpvBroadcaster>::` turbofish. That helper becomes a plain free function `match_receival_address` in payments.rs and returns a pair of identifiers instead of a struct — `DashpayAddressMatch`'s `address_index` field had no reader. `DashpayAddressMatch` goes along with its re-exports in types/dashpay/mod.rs, types/mod.rs and identity/mod.rs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
No consumer in ffi, jni, Swift or Kotlin — verified by grepping all of packages/: - `ManagedIdentity::accept_incoming_request` — a second contact-establishment path that bypasses the persist-before-commit rule the live path enforces (`add_sent_contact_request` / `add_incoming_contact_request`). Called only by its own three tests. - `ManagedIdentity::remove_incoming_contact_request` — tests only; production rejects through `ignore_sender`. The `test_reject_contact_request` integration test moves onto `ignore_sender`. - `state/contacts.rs`: `add_established_contact`, `remove_established_contact` and `established_contact` were hiding under an `#[allow(dead_code)]` spanning the whole impl block. `established_contact_mut`, the one in use, stays. - The `EstablishedContact` setters (`set_alias`, `clear_alias`, `set_note`, `clear_note`, `hide`, `unhide`, `add_accepted_account`, `remove_accepted_account`) — production writes the fields directly via `set_contact_metadata`. Tests that still carry their weight (metadata preservation on re-establish, account_reference rotation) build their fixture by assigning fields. - `ContactRequest::is_outgoing` / `is_incoming` — own tests only. The `is_outgoing` *fields* in the FFI/JNI/Swift persistence layer are a separate thing and are left untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tigial re-export shim Two related cleanups in the same file. 1. `PrivateKeyData` and `KeyStorage` were never constructed or read — `PrivateKeyData::` returns zero hits across all of packages/, including rs-platform-wallet-ffi. The doc claimed "the IdentityKeysChangeSet apply path constructs one per replay, the FFI key-preview path uses one internally"; `apply_identity_key_entry` touches neither. `key_storage.rs` is renamed to `identity_status.rs` — it now holds only `IdentityStatus` and `DpnsNameInfo`. 2. `state::managed_identity` re-exported `block_time` and `key_storage` under the old path so "external users can still reach them" — a comment describing a past move, not the current layout. Ten sites in the tree (including rs-platform-wallet-ffi/src/memory_explorer.rs) reached the types through the shim instead of `crate::wallet::identity::types::*`. All retargeted, shim removed. Import-path changes only — no C ABI impact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…aster None of these had a call site in packages/ — not from Rust, and not through the FFI, Swift or Kotlin: - `CoreWallet::broadcast_transaction` — one hit in the entire repo, its own definition. - `CoreWallet::broadcast_transaction_releasing_reservation` and its only callee `reservations::broadcast_releasing_on_rejection`, together with two tests (`broadcast_releases_reservation_on_rejection`, `broadcast_keeps_reservation_on_ambiguous_failure`) and the `build_signed_tx` helper that existed solely for them. The finalized-handle tests already cover those properties. - `DapiBroadcaster` — 45 lines of a second trait implementation, never instantiated. `PlatformWallet.core` is `CoreWallet<SpvBroadcaster>`, and the FFI/JNI use `SpvBroadcaster` exclusively. Eight comment sites justified the pending-spend fence design by "the DapiBroadcaster path"; rewritten to describe the shape (a broadcaster returning before mempool injection) rather than naming a type that no longer exists. - `PlatformEventManager::add_handler` — every `PlatformEventManager::new` site passes the full handler list up front. - `is_instant_lock_timeout` — a one-line `matches!` whose only consumer was its own test; production matches `FinalityTimeout` directly. The `signed_payment_registry` doc stops calling the removed method "the regular send path". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tError variants `WalletLocked`, `NoPrimaryIdentity`, `NoWalletsConfigured`, `DashpayReceivingAccountAlreadyExists`, `DashpayExternalAccountAlreadyExists` and `AssetLockExpired` had no constructor anywhere in packages/ — no match arm, no FFI mapping, no Swift/Kotlin mirror. `From<PlatformWalletError>` in rs-platform-wallet-ffi routed all six through its `_` arm, so host-visible codes do not change. Dead variants widened the public enum and implied concepts this crate does not have (a wallet lock, a "primary identity"). The two DashPay variants carried four fields nobody filled. The `key_wallet::Network` import existed only for those two. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nce wrapper
`network/top_up.rs` is 67 lines: a 10-line module doc explaining that the file
"just hosts the convenience wrapper `top_up_identity`", 20 lines of argument
docs, and a body forwarding to
`top_up_identity_with_funding(id, AssetLockFunding::FromWalletBalance{..}, ..)`.
`top_up_identity` has no call site in packages/. Both FFI entry points
(`identity_top_up.rs`, `identity_registration_funded_with_signer.rs`) call
`top_up_identity_with_funding` directly.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Public API kept alive solely by `examples/basic_usage.rs`. The method uses `tokio::sync::RwLock::blocking_read` and — as its own doc says — panics when called from an async context, so it doubled the accessor surface with a footgun variant. The FFI (`rs-platform-wallet-ffi/src/asset_lock/manager.rs`) and Swift/Kotlin reach the async `list_tracked_locks` under `block_on`. The example already runs inside a runtime, so it moves to `.await`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nd BlockTime::{new,is_older_than}
`managed_identity/sync.rs` (62 lines: `needs_balance_update`, `needs_keys_sync`,
`update_keys_sync_block_time`, `update_balance_block_time`) together with
`BlockTime::new` and `BlockTime::is_older_than` existed only to test each other
— the only hits outside their own tests were inside a `#[test]` block in
wallet/apply.rs. Production sets the fields directly
(rs-platform-wallet-ffi/src/managed_identity.rs:
`identity.last_updated_balance_block_time = Some(owned)`).
`is_older_than` computed `(current_timestamp - self.timestamp) > max_age_millis`
— a debug panic, or a wrap in release, if a stored block timestamp ran ahead of
the caller's clock. If a freshness check is ever wanted, the right shape is a
single `age_millis(now)` built on `saturating_sub` at the point of use.
`round_trip_block_time_updates` stays — it covers changeset replay for fields
that are still live — and builds its fixture with a struct literal instead of
the removed setters.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
For each name, a grep across all of packages/ (Rust, Swift, Kotlin, tests, examples) returns only the definition: - `refresh_dpns_names` (loading.rs) — its wholesale-replace semantics are already covered by the existing `sync_dpns_names`. - `load_identity_by_dpns_name` (loading.rs) — the only non-definition hit is a comment in PlatformWalletPersistenceHandler.swift. - `register_name_with_signer` (dpns.rs) — returned `dash_sdk::Error` unlike every sibling; production uses `register_name_with_external_signer`. - `dpns_domain_states_for_identity` (dpns_marketplace.rs) along with its limit/pagination loop. `dpns_domain_states_page` stays — marketplace sync uses it. - `wallet_manager_read` / `wallet_manager_write` / `try_wallet_manager_write` (identity_handle.rs) — these leaked an RwLock guard outside the crate. - `derive_identity_key_bytes` (identity_handle.rs). The FFI crate calls 14 `IdentityWallet` methods and none of the above; kotlin-sdk and rs-unified-sdk-jni do not reference them at all. `refresh_identity_with_signer` from the same audit entry is KEPT — its doc explicitly names an out-of-repo consumer (dash-evo-tool's `QualifiedIdentity`). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…helpers from the public API All of these were exported from the crate root with no consumer in the FFI, Swift, Kotlin, or in tests outside their own module. They advertised a seed-resident code path the seedless design deliberately removed. Removed outright: - `derive_contact_payment_addresses` (the batch wrapper) and its test, - `DEFAULT_CONTACT_GAP_LIMIT` and its test, - the re-exports in crypto/mod.rs, identity/mod.rs and lib.rs. Gated behind `#[cfg(test)] pub(crate)` rather than deleted, because they pin the correctness of code that is still live: - `derive_contact_payment_address` — the pin that `reconstruct_contact_xpub` yields an equivalent key (production derives contact addresses through key-wallet's `AccountType::DashpayReceivingFunds` pool), - `generate_auto_accept_proof`, `verify_auto_accept_proof`, `derive_auto_accept_private_key` — coverage for the proof scheme; production goes through `provider.export_auto_accept_private_key` + `verify_auto_accept_proof_with_pubkey`. Their own docs already said "Kept for owner-side tests / a self-check". `ContactRequestValidation::new()` from the same audit entry is KEPT — verification found 9 call sites in validation.rs, two of them on the production path (`validate_sender_key`, `validate_recipient_key`). The audit classed "self-referenced" as dead code; it is not the same thing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nResultOwned trait `operations::shield` described itself as a "self-shield front for `shield_to`, preserving the pre-recipient signature for existing callers" — there are no such callers. The only producer, platform_wallet.rs, calls `shield_to` directly; beyond that the name appeared only in test-file prose (retargeted to `shield_to`). `trait SelectionResultOwned` had one implementation and one requirement, doing `refs.into_iter().cloned().collect()` in two places. Replaced by a free function `own_selection` that says so directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- `MultiSyncNotesResult::per_account_for` — definition only; its doc said it feeds "the legacy per-wallet SyncNotesResult shape", which coordinator.rs builds inline. - `_unused_payment_address` — a decoy under `#[allow(dead_code)]` claiming to suppress a warning for an `address` field that does not exist (`PaymentAddress` is used by `RecoveredOutgoing.recipient`). - `ShieldedStore::get_activity_ids` with both implementations and `SubwalletState::activity_ids` — the scan deriver takes `existing_cmxs: BTreeMap<cmx, id>` and the coordinator uses `get_activity(*id, 0, usize::MAX)`. The test assertion now counts distinct ids on the fetched page. - The default `ShieldedStore::witness` implementation — its only callers were two file_store tests; production goes through `witness_at_depth`. Those tests move to `witness_at_depth(pos, 0)`. - `marked_positions` and `checkpoints` on `InMemoryShieldedStore` — pushed and cleared, never read. They made the in-memory tree look more capable than it is (it cannot produce a witness). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…masternode helper visibility `SpvRuntime::get_quorum_public_key` and `SpvRuntime::update_config` have no call site in rs-platform-wallet, rs-platform-wallet-ffi, rs-unified-sdk-jni, kotlin-sdk or swift-sdk. (Beware appearances: `get_quorum_public_key` has 62 hits across 26 files in the workspace, but those are the `ContextProvider` trait method in entirely different crates — only the inherent method on `SpvRuntime` is removed here. `tests/spv_sync.rs` implements that trait; it does not call the method.) Each carried its own error mapping and locking code to keep consistent with the live paths. Visibility narrowing, no consumers outside the crate: - `wallet_masternode_index_blocking` → private (called only by `masternode_locator_blocking`, a few lines below), - `registration_from_transaction` → `pub(crate)` (used only within tracked.rs), - `find_in_summaries`, `locate_in_summaries` and `parse_locator_input` leave the `pub use` list in masternode/mod.rs; they remain reachable inside the crate through their own modules. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
llbartekll
requested review from
QuantumExplorer,
ZocoLini,
lklimek and
shumkov
as code owners
September 9, 2026 12:01
Contributor
|
Warning Review limit reachedNext included review available in 32 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
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 (3)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe pull request narrows the platform-wallet public API, removes obsolete identity, broadcasting, transfer, shielded-storage, and runtime paths, relocates identity status types, and updates internal callers and documentation. ChangesPlatform wallet API cleanup
Priority: ⬇️ Low Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: ⚪ Minimal · up to This change removes obsolete wallet APIs and updates remaining internal call paths. No concrete current-head merge 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 |
5 tasks
Collaborator
|
✅ Final review complete — no blockers (commit 150e7de) · triage: critical · Phase 2 only (queue backlog) |
llbartekll
changed the title
llbartekll
force-pushed
the
refactor/wallet-dead-code-rust-core
branch
from
September 9, 2026 12:14
cf6f880 to
6810971
Compare
…removed WalletLocked variant The `v4.2-dev` merge brought in #4586 (`22055ec8ab`), whose `persister_error_constructors_are_not_interchangeable` test picks `PlatformWalletError::WalletLocked` as an arbitrary sample variant to prove `from_restore_failure` preserves the concrete inner error through boxing. This branch removes `WalletLocked` as never-constructed, so the merge is textually clean but does not compile — `wallet_lifecycle.rs` references a variant `error.rs` no longer defines. `WalletLocked` still has no constructor anywhere; the test only needed *some* variant. It now uses `SpvAlreadyRunning`, a unit variant production actually raises (`spv/runtime.rs:160`). The property under test is unchanged. Verified: `cargo check -p platform-wallet -p platform-wallet-ffi --all-targets`, `cargo test -p platform-wallet` (983 passed) and `cargo fmt --all -- --check` are all green on the merge commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
thepastaclaw
reviewed
Sep 9, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
The PR’s dead-code removals are consistent with the stated scope, and I found no blocking correctness, security, consensus, or integration issues. One non-blocking performance cleanup remains: after dynamic handler registration was removed, the event handler collection is immutable but still incurs an atomic snapshot load for every event dispatch.
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
Review provenance
- Triage:
criticalbygpt-6-astra(effort low) — Despite being dead-code removal, this is a broad 58-file Rust wallet refactor deleting payment, identity/contact, broadcasting, shielded, synchronization, and cryptographic pathways, with potential side effects across funds, networking, persistence, and security-sensitive behavior. - Phase 1 reviewers: not run (skipped for throughput: 24 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer
🟡 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/events.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/events.rs:111-113: Simplify the now-immutable event-handler collection
Removing `add_handler` removed the only writer to `handlers`: the collection is initialized in `new` and is never replaced. Keeping `ArcSwap` therefore adds an atomic load and snapshot-guard lifetime to every wallet, network, sync, and progress event without providing mutation protection. Store a plain `Vec<Arc<dyn PlatformEventHandler>>` and iterate it by shared reference instead. `PlatformEventHandler` inherits the thread-safety requirements of `EventHandler`, and `PlatformEventManager` is already shared through `Arc`, so concurrent read-only dispatch remains safe while avoiding unnecessary synchronization.
Comment on lines
111
to
113
| /// Read path (every event): one atomic pointer load, then iterate. | ||
| /// Write path (add_handler): clone Vec + atomic swap — rare, not on SPV hot path. | ||
| pub struct PlatformEventManager { | ||
| handlers: ArcSwap<Vec<Arc<dyn PlatformEventHandler>>>, |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Simplify the now-immutable event-handler collection
Removing add_handler removed the only writer to handlers: the collection is initialized in new and is never replaced. Keeping ArcSwap therefore adds an atomic load and snapshot-guard lifetime to every wallet, network, sync, and progress event without providing mutation protection. Store a plain Vec<Arc<dyn PlatformEventHandler>> and iterate it by shared reference instead. PlatformEventHandler inherits the thread-safety requirements of EventHandler, and PlatformEventManager is already shared through Arc, so concurrent read-only dispatch remains safe while avoiding unnecessary synchronization.
source: ['claude']
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
3 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
The 2026-09-08 audit (
review/wallet-swift-slop-audit:WALLET_SWIFT_SLOP_REVIEW.md) raised 61dead-codeentries. Each was a reviewer's claim, not an established fact — this PR lands only the ones that survived independent verification.Scope: rust-core, Effort S, Risk low — 16 entries, one commit per entry.
What was done?
Deletions in
rs-platform-wallet(plus import-path retargeting inrs-platform-wallet-ffi, with no C ABI impact):transfer_with_change_addressand the wholeFeeStrategyByAddressmachinery,validate_change_address,augment_outputs_with_change,checked_sum_credits, theChangeBelowMinimumOutputerror variantaccept_incoming_request,remove_incoming_contact_request, the#[allow(dead_code)]-hidden impl block instate/contacts.rs, theEstablishedContactsetters,ContactRequest::{is_outgoing,is_incoming}derive_contact_payment_addresses,DEFAULT_CONTACT_GAP_LIMIT; auto-accept helpers gated behind#[cfg(test)]DashpayAddressMatchand the threematch_incoming_dashpay_address*variants;match_in_collectionbecomes a free functionIdentityWalletmethodsPrivateKeyData/KeyStorage; the vestigialstate::managed_identity::{block_time,key_storage}re-export shimDapiBroadcaster,add_handler,is_instant_lock_timeoutPlatformWalletErrorvariantsoperations::shield,trait SelectionResultOwnedper_account_for,get_activity_ids, thewitnessdefault, write-only fields)list_tracked_locks_blockingmanaged_identity/sync.rs,BlockTime::{new,is_older_than}network/top_up.rsSpvRuntime::{get_quorum_public_key,update_config}plus visibility narrowing on masternode helpersTotal: 58 files, 2,844 deletions, 221 insertions.
Where this departs from the audit
Two audit claims did not hold up and were rejected:
ContactRequestValidation::new()— the audit classed it as dead ("only self-referenced"). It has 9 call sites invalidation.rs, two of them on the production path (validate_sender_key,validate_recipient_key). Removing it broke the build; restored.#[cfg(test)]rather than deleted (derive_contact_payment_address,generate_auto_accept_proof,verify_auto_accept_proof+derive_auto_accept_private_key). They leave the public API as the audit intended, but they pin the correctness of code that is live —reconstruct_contact_xpuband the auto-accept proof scheme. Deleting them outright would have dropped real coverage.Note for reviewers: the audit's line numbers have drifted against
v4.2-dev(e.g.SpvRuntime::update_configaudit 583 → actually 628). Everything was located by symbol, never by line.How Has This Been Tested?
All green at the branch tip:
cargo check --workspace✅cargo clippy -p platform-wallet -p platform-wallet-ffi --all-features --all-targets✅ (the only two warnings — an unused import inrs-driveanddefault-featuresinrs-dpp/Cargo.toml— are pre-existing and present on the base branch)cargo test -p platform-wallet— 955 passed, 0 failedcargo test -p platform-wallet-ffi— 365 passed, 0 failedTests that only exercised the removed code go with it. Tests that check something real but merely built their fixture through the removed setters (metadata preservation on re-establish,
account_referencerotation,BlockTimechangeset round-trip) were kept and rewritten to assign fields directly.Breaking Changes
No C ABI change. The removed symbols are public in the
platform-walletcrate, but were verified to have no consumer anywhere in the repo (FFI, JNI, Swift, Kotlin, tests, examples).refresh_identity_with_signerfrom entry rust-core-099 was deliberately kept — its doc comment explicitly names an out-of-repo consumer (dash-evo-tool'sQualifiedIdentity).Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Changes
shield_toflow.API Updates