test(platform-wallet): e2e framework + full test suite — triage pins, Found-*/PA-* guards, fail-closed persist, Stage-2 merge#3549
Draft
Claudius-Maginificent wants to merge 483 commits into
Draft
Conversation
Contributor
📝 WalkthroughWalkthroughAn end-to-end testing framework for Changes
Sequence Diagram(s)sequenceDiagram
participant Test as E2E Test
participant Harness as E2eContext Harness
participant Registry as Wallet Registry
participant Bank as BankWallet
participant TWallet as TestWallet
participant Manager as PlatformWalletManager
participant SDK as SDK/PlatformWallet
participant Cleanup as Cleanup
Test->>Harness: init() first call
Harness->>Registry: open(test_wallets.json)
Harness->>Cleanup: sweep_orphans()
Cleanup->>Registry: list_orphans()
Cleanup->>Manager: create from orphan seed
Cleanup->>SDK: sync & drain to bank
Cleanup->>Registry: remove_orphan_entry
Harness->>Bank: load from mnemonic
Harness->>Bank: sync_balances()
Harness->>Bank: fund_address(test_addr1, credits)
Harness->>SDK: transfer via bank wallet
Test->>Test: setup() generates seed
Test->>Manager: create TestWallet
Test->>TWallet: create(seed)
Test->>TWallet: next_unused_address() → addr2
Test->>Bank: fund_address(addr2, TRANSFER_CREDITS)
Test->>SDK: transfer via bank
Test->>TWallet: wait_for_balance(addr2, expected)
TWallet->>SDK: sync_balances()
Test->>SDK: transfer(addr2 → addr1, TRANSFER_CREDITS)
SDK->>SDK: execute, compute fee
Test->>TWallet: verify balances & fee
Test->>Test: teardown()
Test->>Cleanup: teardown_one(test_wallet)
Cleanup->>TWallet: drain all addresses to bank
Cleanup->>Registry: remove_entry
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
🚥 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
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an end-to-end (wallet → SDK → broadcast) integration test harness to rs-platform-wallet and introduces the first live test case (address-funds transfer), alongside a production fix to InputSelection::Auto input selection so generated transitions satisfy protocol structure rules.
Changes:
- Added a reusable E2E framework under
packages/rs-platform-wallet/tests/e2e/(workdir slot locking, bank wallet, persistent registry, cleanup/sweep, wait hub, signer, SDK wiring). - Added the first E2E test case: transferring credits between two platform-payment addresses in a test wallet (ignored by default).
- Fixed
auto_select_inputsin production code to avoid selecting full balances as “input credits”, and added unit tests for the selection logic.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs | Fixes auto input selection; adds pure helper + unit tests for selection behavior. |
| packages/rs-platform-wallet/tests/e2e.rs | Adds the integration test crate root and module wiring for the e2e suite. |
| packages/rs-platform-wallet/tests/e2e/README.md | Operator/setup documentation for running live e2e tests. |
| packages/rs-platform-wallet/tests/e2e/cases/mod.rs | Declares e2e test modules. |
| packages/rs-platform-wallet/tests/e2e/cases/transfer.rs | First e2e test exercising funding + self-transfer + teardown. |
| packages/rs-platform-wallet/tests/e2e/framework/mod.rs | Framework public surface (setup, errors, prelude) and module layout. |
| packages/rs-platform-wallet/tests/e2e/framework/harness.rs | E2eContext singleton init: config, workdir locking, SDK, manager, bank, registry, startup sweep. |
| packages/rs-platform-wallet/tests/e2e/framework/config.rs | Env/.env configuration loader for the harness. |
| packages/rs-platform-wallet/tests/e2e/framework/sdk.rs | Constructs dash_sdk::Sdk with TrustedHttpContextProvider and DAPI address resolution. |
| packages/rs-platform-wallet/tests/e2e/framework/workdir.rs | Cross-process workdir slot selection via flock. |
| packages/rs-platform-wallet/tests/e2e/framework/panic_hook.rs | Installs panic hook to cancel background work on panic. |
| packages/rs-platform-wallet/tests/e2e/framework/wait_hub.rs | Notify-based hub bridging wallet/SPV/platform events to async waiters. |
| packages/rs-platform-wallet/tests/e2e/framework/wait.rs | Async waiting helpers (event-driven balance wait + generic polling). |
| packages/rs-platform-wallet/tests/e2e/framework/signer.rs | Seed-backed Signer<PlatformAddress> with eager DIP-17 key cache. |
| packages/rs-platform-wallet/tests/e2e/framework/wallet_factory.rs | Test wallet factory + SetupGuard (panic-safe registry-backed lifecycle). |
| packages/rs-platform-wallet/tests/e2e/framework/registry.rs | JSON-backed persistent registry for panic-safe orphan recovery. |
| packages/rs-platform-wallet/tests/e2e/framework/cleanup.rs | Startup sweep + per-test teardown draining funds back to bank. |
| packages/rs-platform-wallet/tests/e2e/framework/bank.rs | Loads and syncs a pre-funded bank wallet; serialized funding API. |
| packages/rs-platform-wallet/tests/e2e/framework/context_provider.rs | Retained (disabled) SPV-backed SDK context provider module for future re-enable. |
| packages/rs-platform-wallet/tests/e2e/framework/spv.rs | Retained (disabled) SPV startup/readiness helpers for future re-enable. |
| packages/rs-platform-wallet/Cargo.toml | Adds dev-dependencies needed by the e2e harness. |
| Cargo.lock | Locks new/updated dependencies for the added test tooling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6 tasks
8 tasks
lklimek
changed the base branch from
v3.1-dev
to
fix/rs-platform-wallet-auto-select-inputs
7 tasks
lklimek
reviewed
Apr 29, 2026
lklimek
reviewed
Apr 29, 2026
Contributor
|
@coderabbitai review all |
Contributor
|
🧠 Learnings used✅ Actions performedFull review triggered. |
![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: 9
♻️ Duplicate comments (1)
packages/rs-platform-wallet/tests/e2e/framework/cleanup.rs (1)
57-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep sub-threshold wallets recoverable.
If
0 < total <= SWEEP_DUST_THRESHOLD, both cleanup paths skipsweep_platform_addressesand still delete the registry entry. That permanently abandons the remaining credits and will slowly drain the shared bank across repeated runs. Either sweep every positive balance withReduceOutput(0)or only remove the entry once the wallet is actually empty.Also applies to: 109-121, 145-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet/tests/e2e/framework/cleanup.rs` around lines 57 - 75, The cleanup currently deletes registry entries even when 0 < total <= SWEEP_DUST_THRESHOLD, abandoning recoverable credits; update the logic in the sweep_one match branches (the block that calls registry.remove and registry.set_status) to: if the wallet balance is > 0 but <= SWEEP_DUST_THRESHOLD, call sweep_platform_addresses with ReduceOutput(0) (or otherwise perform a full sweep for any positive balance) and only call registry.remove when the wallet is actually empty; ensure failed-path still sets EntryStatus::Failed when sweep fails and that successful-path only increments swept and removes the registry entry when the post-sweep balance is zero (reference symbols: sweep_one, sweep_platform_addresses, SWEEP_DUST_THRESHOLD, ReduceOutput(0), registry.remove, registry.set_status, EntryStatus::Failed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-platform-wallet/tests/e2e/cases/transfer.rs`:
- Line 31: Rename the test function transfer_between_two_platform_addresses to
follow the convention by renaming it to
should_transfer_between_two_platform_addresses; update the async fn declaration
(and any internal references or usages of
transfer_between_two_platform_addresses) to the new name so the test name begins
with "should" while keeping the function body and attributes unchanged.
- Around line 51-79: This test performs real network calls via
s.ctx.bank().fund_address and s.test_wallet.transfer / wait_for_balance; change
it to comply with the "no network in unit/integration tests" rule by either (A)
moving this file/case to an e2e-only suite (so it runs under an e2e test runner)
or (B) refactoring to inject mocked implementations for the bank client and
wallet observer used by wait_for_balance and transfer (replace s.ctx.bank() and
any network-dependent wait_for_balance calls with test doubles that simulate
funding/transfer and observable balance updates); update references to
next_unused_address, transfer, and wait_for_balance to use the mocks or the
e2e-only harness accordingly.
In `@packages/rs-platform-wallet/tests/e2e/framework/config.rs`:
- Around line 34-50: Config currently derives Debug and will print sensitive
bank_mnemonic; replace the automatic derive with a manual impl Debug for Config
that omits or redacts bank_mnemonic (e.g., display "REDACTED" or hide its value)
and prints the other fields normally; implement Debug in the same module
referencing the struct name Config and its fields (bank_mnemonic, network,
dapi_addresses, min_bank_credits, workdir_base, trusted_context_url) so future
secret fields can also be redacted consistently.
In `@packages/rs-platform-wallet/tests/e2e/framework/registry.rs`:
- Around line 225-259: Rename the three test functions to follow the "should …"
naming convention: change missing_file_opens_empty to a descriptive name like
should_open_empty_if_file_missing, change insert_remove_round_trip_persists to
should_persist_insert_remove_round_trip, and change
corrupt_file_falls_back_to_empty to should_fall_back_to_empty_on_corrupt_file;
update the fn identifiers in
packages/rs-platform-wallet/tests/e2e/framework/registry.rs (the tests currently
named missing_file_opens_empty, insert_remove_round_trip_persists,
corrupt_file_falls_back_to_empty) and run cargo test to ensure no references
break.
In `@packages/rs-platform-wallet/tests/e2e/framework/wallet_factory.rs`:
- Around line 291-293: Rename the test function
default_spec_matches_pinned_constants to follow the repository "should …"
convention (e.g., should_default_spec_match_pinned_constants or
should_match_pinned_constants_by_default) so the test name starts with "should";
update the function declaration fn default_spec_matches_pinned_constants() to
the new name and keep the body (including PlatformPaymentAccountSpec::default())
unchanged so references and assertions remain valid.
In `@packages/rs-platform-wallet/tests/e2e/framework/workdir.rs`:
- Line 92: Rename the test function
first_call_takes_slot_zero_second_falls_through to follow the required "should
..." convention (for example
should_first_call_take_slot_zero_and_second_fall_through); update the function
identifier wherever referenced (the test declaration itself and any uses in
attributes or calls) so the Rust test name begins with "should_" and keep the
original behavior and test annotation (e.g., #[test]) unchanged.
- Around line 50-61: The current error handling in the lock acquisition loop
treats every Err(err) as a busy slot; update the branch in the function that
opens/locks `lock_file` (the block that logs "workdir slot busy, trying next")
to inspect the IO error kind: if the error indicates contention (e.g.,
would-block / ErrorKind::WouldBlock or the platform-specific WouldBlock
equivalent), keep the existing tracing::debug and continue; for any other errors
(permission, other IO), log an error and propagate/return the error instead of
retrying so real failures aren’t swallowed.
In `@packages/rs-platform-wallet/tests/e2e/README.md`:
- Around line 99-106: The fenced code blocks in the e2e README (the blocks
starting with the "Bank wallet under-funded." message and the "SetupGuard
dropped without explicit teardown — wallet <id>" message) lack language tags,
causing MD040 lint failures; update those fenced blocks to include a language
specifier (e.g., change ``` to ```text) for both occurrences (the block
containing "Bank wallet under-funded." and the later block containing
"SetupGuard dropped without explicit teardown") so the markdown linter accepts
them.
- Around line 233-235: Update the stale troubleshooting example to match the
current error shape emitted by the pick_available_workdir routine: replace the
quoted `No available workdir slots (tried 0..10)` text with the actual error
text produced by pick_available_workdir (copy exact current message/format), and
note that this occurs when all 10 workdir slots are locked so operators search
logs for the correct string; reference pick_available_workdir in the note so
maintainers can locate the implementation for future changes.
---
Duplicate comments:
In `@packages/rs-platform-wallet/tests/e2e/framework/cleanup.rs`:
- Around line 57-75: The cleanup currently deletes registry entries even when 0
< total <= SWEEP_DUST_THRESHOLD, abandoning recoverable credits; update the
logic in the sweep_one match branches (the block that calls registry.remove and
registry.set_status) to: if the wallet balance is > 0 but <=
SWEEP_DUST_THRESHOLD, call sweep_platform_addresses with ReduceOutput(0) (or
otherwise perform a full sweep for any positive balance) and only call
registry.remove when the wallet is actually empty; ensure failed-path still sets
EntryStatus::Failed when sweep fails and that successful-path only increments
swept and removes the registry entry when the post-sweep balance is zero
(reference symbols: sweep_one, sweep_platform_addresses, SWEEP_DUST_THRESHOLD,
ReduceOutput(0), registry.remove, registry.set_status, EntryStatus::Failed).
🪄 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: 0379415c-b6af-4b82-b05c-635af13cb042
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
packages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/tests/.env.examplepackages/rs-platform-wallet/tests/e2e.rspackages/rs-platform-wallet/tests/e2e/README.mdpackages/rs-platform-wallet/tests/e2e/cases/mod.rspackages/rs-platform-wallet/tests/e2e/cases/transfer.rspackages/rs-platform-wallet/tests/e2e/framework/bank.rspackages/rs-platform-wallet/tests/e2e/framework/cleanup.rspackages/rs-platform-wallet/tests/e2e/framework/config.rspackages/rs-platform-wallet/tests/e2e/framework/context_provider.rspackages/rs-platform-wallet/tests/e2e/framework/harness.rspackages/rs-platform-wallet/tests/e2e/framework/mod.rspackages/rs-platform-wallet/tests/e2e/framework/registry.rspackages/rs-platform-wallet/tests/e2e/framework/sdk.rspackages/rs-platform-wallet/tests/e2e/framework/spv.rspackages/rs-platform-wallet/tests/e2e/framework/wait.rspackages/rs-platform-wallet/tests/e2e/framework/wait_hub.rspackages/rs-platform-wallet/tests/e2e/framework/wallet_factory.rspackages/rs-platform-wallet/tests/e2e/framework/workdir.rspackages/rs-sdk/src/platform/transition.rspackages/rs-sdk/src/platform/transition/address_inputs.rspackages/simple-signer/Cargo.tomlpackages/simple-signer/src/signer.rs
Collaborator
|
✅ Review complete (commit 921833f) |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 24 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (2)
packages/rs-sdk/src/platform/transition/address_inputs.rs:39
- Now that this helper is public,
nonce + 1can overflow whennonce == u32::MAX, which will panic in debug builds and wrap in release builds. Consider usingchecked_add(1)and returning an error (or otherwise handling the overflow) so callers can't accidentally produce an invalid/wrapping nonce.
pub fn nonce_inc(
data: BTreeMap<PlatformAddress, (AddressNonce, Credits)>,
) -> BTreeMap<PlatformAddress, (AddressNonce, Credits)> {
data.into_iter()
.map(|(address, (nonce, credits))| (address, (nonce + 1, credits)))
.collect()
packages/rs-sdk/src/platform/transition/address_inputs.rs:18
fetch_inputs_with_nonceis now public but has no doc comment explaining (1) that it performs existence/balance checks and (2) that callers typically need to applynonce_incbefore building a transfer (astransfer_address_fundsdoes). Please document the intended call pattern (or provide a single public helper that returns the incremented nonces) to reduce misuse from external callers.
pub async fn fetch_inputs_with_nonce(
sdk: &Sdk,
amounts: &BTreeMap<PlatformAddress, Credits>,
) -> Result<BTreeMap<PlatformAddress, (AddressNonce, Credits)>, Error> {
if amounts.is_empty() {
return Err(Error::from(TransitionNoInputsError::new()));
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lklimek
added
the
ready for final review
This was referenced Apr 30, 2026
thepastaclaw
suggested changes
May 2, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
PR adds a substantial e2e framework for rs-platform-wallet. Four blocking issues in the cleanup/teardown lifecycle: the live test no longer carries #[ignore] (so plain cargo test fails without the bank mnemonic), the sweep helper doesn't filter sub-min_input_amount inputs (DPP rejects them), SWEEP_DUST_THRESHOLD (5M) sits below the protocol's min transfer fee (6.5M) leaving an unsweepable balance band, and positive sub-threshold balances are silently dropped from the registry. Several supporting suggestions and nitpicks around dead/misnamed API and error-context loss. Overflow: 3 valid findings dropped to fit the 10-comment budget.
Reviewed commit: ae98ccf
🔴 4 blocking | 🟡 4 suggestion(s) | 💬 2 nitpick(s)
1 additional finding
🟡 suggestion: `fetch_inputs_with_nonce` / `nonce_inc` promoted to `pub` with no caller outside rs-sdk
packages/rs-sdk/src/platform/transition/address_inputs.rs (lines 12-40)
Both functions (and the address_inputs module itself) were widened from pub(crate) to pub. A repo-wide grep finds no caller outside crate::platform::transition::* — the e2e framework in rs-platform-wallet does not import them, and rs-platform-wallet production code doesn't either. The PR description frames this as future-friendliness for the e2e framework, but that framework never lands the call. Promoting low-level internals to the SDK's public API surface without a concrete consumer is a maintenance hazard: once pub, the signatures become a stability commitment, and nonce_inc in particular is footgun-prone outside the strict fetch→increment→sign→broadcast flow. Revert to pub(crate) (or pub(super)) and widen in the same PR as the first external caller.
🤖 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/tests/e2e/cases/transfer.rs`:
- [BLOCKING] lines 30-31: Live e2e test runs by default; `cargo test` hard-fails without operator env
`transfer_between_two_platform_addresses` is no longer `#[ignore]`d (the doc comment on lines 4-7 makes this explicit). `setup()` calls `Config::from_env()` which errors if `PLATFORM_WALLET_E2E_BANK_MNEMONIC` is unset, and the test escalates that to a panic via `.expect("e2e setup failed")`. Consequence: a stock `cargo test -p platform-wallet` (or workspace-wide invocation) becomes a hard failure for any contributor or CI job without a funded testnet bank wallet. Workflow-level gating is a coordination requirement, not a guarantee. The DET precedent the framework cites keeps live-network tests behind `#[ignore]` for exactly this reason. Re-add `#[ignore]` and run live with `cargo test -- --ignored`.
In `packages/rs-platform-wallet/tests/e2e/framework/cleanup.rs`:
- [BLOCKING] lines 202-211: Sweep helper doesn't filter sub-`min_input_amount` balances; DPP rejects the transition
`sweep_platform_addresses` filters inputs by `*b > 0` only. The address-funds-transfer state-transition validation (`packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funds_transfer_transition/v0/state_transition_validation.rs:157-163`) rejects any input below `platform_version.dpp.state_transitions.address_funds.min_input_amount`. So as soon as one tracked address holds a sub-minimum balance, every sweep attempt for that wallet — both `teardown_one` and the orphan `sweep_one` — submits an invalid transition and the entry stays stuck. Mirror the production auto-selector and drop inputs below `min_input_amount` from the explicit map.
- [BLOCKING] lines 26-30: `SWEEP_DUST_THRESHOLD` (5M) is below the protocol's minimum transfer fee (6.5M)
Sweep eligibility is `total > 5_000_000`, but the minimum fee for a 1-input/1-output address transfer is `address_funds_transfer_input_cost (500_000) + address_funds_transfer_output_cost (6_000_000) = 6_500_000` credits (`packages/rs-platform-version/src/version/fee/state_transition_min_fees/v1.rs:14-15`). For balances in `(5_000_000, 6_500_000)`, both `teardown_one` and `sweep_one` will attempt a `ReduceOutput(0)` sweep that cannot cover its own fee, so those wallets get retried forever (with the registry entry repeatedly marked `Failed`) until someone tops them up manually. Raise the threshold above the protocol minimum (and ideally derive it from the platform-version constants so it stays in sync).
- [BLOCKING] lines 109-162: Positive sub-threshold balances are dropped from the registry without sweeping
When `total <= SWEEP_DUST_THRESHOLD`, `teardown_one` (lines 147-162) skips `sweep_platform_addresses` and unconditionally calls `registry.remove(...)`; the orphan path does the same indirectly — `sweep_one` returns `Ok(())` after logging "below sweep threshold; skipping" (lines 109-117), and `sweep_orphans` then removes the registry entry (lines 58-66). Any wallet that still holds a positive balance under the threshold is therefore forgotten rather than retried or aggregated, permanently stranding real testnet credits and contradicting the README's recovery guarantees. Either keep the entry tagged `Failed` so a future operator can audit, or only drop entries whose `total == 0`.
In `packages/simple-signer/src/signer.rs`:
- [SUGGESTION] lines 197-241: `from_seed_for_identity` is misleadingly named, half-functional, and unused
The new (feature-gated) constructor derives DIP-9 identity-authentication ECDSA secp256k1 keys but inserts them into `address_private_keys: BTreeMap<[u8; 20], [u8; 32]>` — the map consumed by `Signer<PlatformAddress>::sign` (line 339, keyed on the 20-byte address hash). The `Signer<IdentityPublicKey>` view that the function name implies (line 245) only consults `private_keys` / `private_keys_in_creation`, both of which remain empty after this constructor runs. The doc comment hand-waves this with "callers must additionally register `IdentityPublicKey` records" — but if the caller has to do that themselves the constructor isn't actually "for identity." A repo-wide grep confirms zero callers. Either (a) populate `private_keys` inside the constructor so identity signing works out of the box, (b) drop it until a real consumer exists, or (c) rename to reflect what it actually populates (e.g. `derive_identity_path_into_address_keys`).
In `packages/rs-platform-wallet/tests/e2e/framework/sdk.rs`:
- [SUGGESTION] lines 39-46: `FrameworkError::NotImplemented` used as a generic runtime-error wrapper, dropping the underlying error
`SdkBuilder::build()` failure here is a real runtime error, not an unimplemented-feature path, but it's mapped to `FrameworkError::NotImplemented("sdk::build_sdk — SdkBuilder::build failed (see logs)")`. The actual error `e` is only emitted via a side-effect `tracing::error!` and then discarded. Callers that pattern-match on the `Result` (or render it for CI failure summaries) see only the `&'static str`. The same pattern recurs at lines 76-84, 99-107, 117-125, and `framework/spv.rs:125-148, 215-236`. The `FrameworkError` enum already has `Wallet(String)`, `Bank(String)`, `Config(String)` for this purpose — add `Sdk(String)` / `Spv(String)` variants and propagate `e.to_string()` through the `Result`.
In `packages/rs-sdk/src/platform/transition/address_inputs.rs`:
- [SUGGESTION] lines 12-40: `fetch_inputs_with_nonce` / `nonce_inc` promoted to `pub` with no caller outside rs-sdk
Both functions (and the `address_inputs` module itself) were widened from `pub(crate)` to `pub`. A repo-wide grep finds no caller outside `crate::platform::transition::*` — the e2e framework in rs-platform-wallet does not import them, and rs-platform-wallet production code doesn't either. The PR description frames this as future-friendliness for the e2e framework, but that framework never lands the call. Promoting low-level internals to the SDK's public API surface without a concrete consumer is a maintenance hazard: once `pub`, the signatures become a stability commitment, and `nonce_inc` in particular is footgun-prone outside the strict fetch→increment→sign→broadcast flow. Revert to `pub(crate)` (or `pub(super)`) and widen in the same PR as the first external caller.
In `packages/rs-platform-wallet/tests/e2e/framework/spv.rs`:
- [SUGGESTION] lines 205-208: Retained SPV path bypasses the slot-locked workdir
`E2eContext::build` acquires a unique slot via `pick_available_workdir` and stores it in `workdir`, but `build_client_config` derives its storage path from `config.workdir_base`. If the commented-out SPV block in `harness.rs` is re-enabled (Task #15), every concurrent process will share `<base>/spv-data` instead of using the locked slot directory, defeating the cross-process isolation mechanism and creating avoidable RocksDB/SPV state contention. Because the SPV module is intentionally kept compilable for re-enablement, fix this now — pass the slot workdir into `build_client_config` so the path tracks the lock.
Comment on lines
+202
to
+211
| let inputs: BTreeMap<PlatformAddress, Credits> = wallet | ||
| .platform() | ||
| .addresses_with_balances() | ||
| .await | ||
| .into_iter() | ||
| .filter(|(_, b)| *b > 0) | ||
| .collect(); | ||
| if inputs.is_empty() { | ||
| return Ok(()); | ||
| } |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Sweep helper doesn't filter sub-min_input_amount balances; DPP rejects the transition
sweep_platform_addresses filters inputs by *b > 0 only. The address-funds-transfer state-transition validation (packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funds_transfer_transition/v0/state_transition_validation.rs:157-163) rejects any input below platform_version.dpp.state_transitions.address_funds.min_input_amount. So as soon as one tracked address holds a sub-minimum balance, every sweep attempt for that wallet — both teardown_one and the orphan sweep_one — submits an invalid transition and the entry stays stuck. Mirror the production auto-selector and drop inputs below min_input_amount from the explicit map.
source: ['codex']
🤖 Fix this 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/tests/e2e/framework/cleanup.rs`:
- [BLOCKING] lines 202-211: Sweep helper doesn't filter sub-`min_input_amount` balances; DPP rejects the transition
`sweep_platform_addresses` filters inputs by `*b > 0` only. The address-funds-transfer state-transition validation (`packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funds_transfer_transition/v0/state_transition_validation.rs:157-163`) rejects any input below `platform_version.dpp.state_transitions.address_funds.min_input_amount`. So as soon as one tracked address holds a sub-minimum balance, every sweep attempt for that wallet — both `teardown_one` and the orphan `sweep_one` — submits an invalid transition and the entry stays stuck. Mirror the production auto-selector and drop inputs below `min_input_amount` from the explicit map.
Comment on lines
+26
to
+30
| /// Minimum sweep amount: skip wallets whose total balance is below | ||
| /// this. Acts as the dust gate so sweeps don't churn the chain for | ||
| /// negligible recoveries; the fee is absorbed from the output via | ||
| /// `ReduceOutput(0)` so no fee-headroom margin is needed here. | ||
| const SWEEP_DUST_THRESHOLD: Credits = 5_000_000; |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: SWEEP_DUST_THRESHOLD (5M) is below the protocol's minimum transfer fee (6.5M)
Sweep eligibility is total > 5_000_000, but the minimum fee for a 1-input/1-output address transfer is address_funds_transfer_input_cost (500_000) + address_funds_transfer_output_cost (6_000_000) = 6_500_000 credits (packages/rs-platform-version/src/version/fee/state_transition_min_fees/v1.rs:14-15). For balances in (5_000_000, 6_500_000), both teardown_one and sweep_one will attempt a ReduceOutput(0) sweep that cannot cover its own fee, so those wallets get retried forever (with the registry entry repeatedly marked Failed) until someone tops them up manually. Raise the threshold above the protocol minimum (and ideally derive it from the platform-version constants so it stays in sync).
source: ['codex']
🤖 Fix this 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/tests/e2e/framework/cleanup.rs`:
- [BLOCKING] lines 26-30: `SWEEP_DUST_THRESHOLD` (5M) is below the protocol's minimum transfer fee (6.5M)
Sweep eligibility is `total > 5_000_000`, but the minimum fee for a 1-input/1-output address transfer is `address_funds_transfer_input_cost (500_000) + address_funds_transfer_output_cost (6_000_000) = 6_500_000` credits (`packages/rs-platform-version/src/version/fee/state_transition_min_fees/v1.rs:14-15`). For balances in `(5_000_000, 6_500_000)`, both `teardown_one` and `sweep_one` will attempt a `ReduceOutput(0)` sweep that cannot cover its own fee, so those wallets get retried forever (with the registry entry repeatedly marked `Failed`) until someone tops them up manually. Raise the threshold above the protocol minimum (and ideally derive it from the platform-version constants so it stays in sync).
Comment on lines
+109
to
+162
| if total > SWEEP_DUST_THRESHOLD { | ||
| sweep_platform_addresses(&wallet, &signer, bank.primary_receive_address()).await?; | ||
| } else { | ||
| tracing::debug!( | ||
| wallet_id = %hex::encode(hash), | ||
| total, | ||
| "orphan platform total below sweep threshold; skipping" | ||
| ); | ||
| } | ||
| sweep_identities(&wallet).await?; | ||
| sweep_core_addresses(&wallet).await?; | ||
| sweep_unused_core_asset_locks(&wallet).await?; | ||
| sweep_shielded(&wallet).await?; | ||
|
|
||
| // Best-effort manager unregister so SPV stops tracking the | ||
| // wallet's addresses on subsequent passes. | ||
| if let Err(err) = manager.remove_wallet(hash).await { | ||
| tracing::warn!( | ||
| target: "platform_wallet::e2e::cleanup", | ||
| wallet_id = %hex::encode(hash), | ||
| error = %err, | ||
| "manager unregister failed after sweep; wallet remains tracked" | ||
| ); | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Per-test teardown: drain back to bank, drop the registry entry, | ||
| /// and unregister from the manager. Best-effort — failures retain | ||
| /// the entry so the next startup's [`sweep_orphans`] retries. | ||
| pub async fn teardown_one( | ||
| manager: &Arc<PlatformWalletManager<NoPlatformPersistence>>, | ||
| bank: &BankWallet, | ||
| registry: &PersistentTestWalletRegistry, | ||
| test_wallet: &TestWallet, | ||
| ) -> FrameworkResult<()> { | ||
| test_wallet.sync_balances().await?; | ||
| let total = test_wallet.total_credits().await; | ||
| if total > SWEEP_DUST_THRESHOLD { | ||
| sweep_platform_addresses( | ||
| test_wallet.platform_wallet(), | ||
| test_wallet.address_signer(), | ||
| bank.primary_receive_address(), | ||
| ) | ||
| .await?; | ||
| } | ||
| sweep_identities(test_wallet.platform_wallet()).await?; | ||
| sweep_core_addresses(test_wallet.platform_wallet()).await?; | ||
| sweep_unused_core_asset_locks(test_wallet.platform_wallet()).await?; | ||
| sweep_shielded(test_wallet.platform_wallet()).await?; | ||
|
|
||
| // Drop the registry entry first so an unregister failure | ||
| // doesn't leak it; the wallet has no balance left to recover. | ||
| registry.remove(&test_wallet.id())?; |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Positive sub-threshold balances are dropped from the registry without sweeping
When total <= SWEEP_DUST_THRESHOLD, teardown_one (lines 147-162) skips sweep_platform_addresses and unconditionally calls registry.remove(...); the orphan path does the same indirectly — sweep_one returns Ok(()) after logging "below sweep threshold; skipping" (lines 109-117), and sweep_orphans then removes the registry entry (lines 58-66). Any wallet that still holds a positive balance under the threshold is therefore forgotten rather than retried or aggregated, permanently stranding real testnet credits and contradicting the README's recovery guarantees. Either keep the entry tagged Failed so a future operator can audit, or only drop entries whose total == 0.
source: ['claude', 'codex']
🤖 Fix this 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/tests/e2e/framework/cleanup.rs`:
- [BLOCKING] lines 109-162: Positive sub-threshold balances are dropped from the registry without sweeping
When `total <= SWEEP_DUST_THRESHOLD`, `teardown_one` (lines 147-162) skips `sweep_platform_addresses` and unconditionally calls `registry.remove(...)`; the orphan path does the same indirectly — `sweep_one` returns `Ok(())` after logging "below sweep threshold; skipping" (lines 109-117), and `sweep_orphans` then removes the registry entry (lines 58-66). Any wallet that still holds a positive balance under the threshold is therefore forgotten rather than retried or aggregated, permanently stranding real testnet credits and contradicting the README's recovery guarantees. Either keep the entry tagged `Failed` so a future operator can audit, or only drop entries whose `total == 0`.
Comment on lines
+197
to
+241
| /// Build a [`SimpleSigner`] populated with the DIP-9 identity-authentication | ||
| /// (ECDSA) gap window for `identity_index`. The returned signer holds raw | ||
| /// secp256k1 secrets keyed on `(pubkey-hash, secret)` via | ||
| /// [`Self::address_private_keys`] — callers that need a `Signer<IdentityPublicKey>` | ||
| /// view must additionally register `IdentityPublicKey` records via | ||
| /// [`Self::add_identity_public_key`] using the matching pubkey bytes. | ||
| #[cfg(feature = "derive")] | ||
| pub fn from_seed_for_identity( | ||
| seed: &[u8; 64], | ||
| network: key_wallet::Network, | ||
| identity_index: u32, | ||
| gap_limit: u32, | ||
| ) -> Result<Self, SimpleSignerError> { | ||
| use key_wallet::bip32::KeyDerivationType; | ||
| use key_wallet::wallet::root_extended_keys::RootExtendedPrivKey; | ||
| use key_wallet::DerivationPath; | ||
|
|
||
| let root_priv = RootExtendedPrivKey::new_master(seed) | ||
| .map_err(|err| SimpleSignerError::InvalidSeed(err.to_string()))?; | ||
| let root_xpriv = root_priv.to_extended_priv_key(network); | ||
|
|
||
| let secp = Secp256k1::new(); | ||
| let mut signer = Self::default(); | ||
| for key_index in 0..gap_limit { | ||
| let leaf_path = DerivationPath::identity_authentication_path( | ||
| network, | ||
| KeyDerivationType::ECDSA, | ||
| identity_index, | ||
| key_index, | ||
| ); | ||
| let xpriv = root_xpriv.derive_priv(&secp, &leaf_path).map_err(|err| { | ||
| SimpleSignerError::DerivePriv { | ||
| index: key_index, | ||
| message: err.to_string(), | ||
| } | ||
| })?; | ||
| let secret: SecretKey = xpriv.private_key; | ||
| let pubkey: PublicKey = PublicKey::from_secret_key(&secp, &secret); | ||
| let pkh = ripemd160_sha256(&pubkey.serialize()); | ||
| signer | ||
| .address_private_keys | ||
| .insert(pkh, secret.secret_bytes()); | ||
| } | ||
| Ok(signer) | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: from_seed_for_identity is misleadingly named, half-functional, and unused
The new (feature-gated) constructor derives DIP-9 identity-authentication ECDSA secp256k1 keys but inserts them into address_private_keys: BTreeMap<[u8; 20], [u8; 32]> — the map consumed by Signer<PlatformAddress>::sign (line 339, keyed on the 20-byte address hash). The Signer<IdentityPublicKey> view that the function name implies (line 245) only consults private_keys / private_keys_in_creation, both of which remain empty after this constructor runs. The doc comment hand-waves this with "callers must additionally register IdentityPublicKey records" — but if the caller has to do that themselves the constructor isn't actually "for identity." A repo-wide grep confirms zero callers. Either (a) populate private_keys inside the constructor so identity signing works out of the box, (b) drop it until a real consumer exists, or (c) rename to reflect what it actually populates (e.g. derive_identity_path_into_address_keys).
source: ['claude', 'codex']
🤖 Fix this 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/simple-signer/src/signer.rs`:
- [SUGGESTION] lines 197-241: `from_seed_for_identity` is misleadingly named, half-functional, and unused
The new (feature-gated) constructor derives DIP-9 identity-authentication ECDSA secp256k1 keys but inserts them into `address_private_keys: BTreeMap<[u8; 20], [u8; 32]>` — the map consumed by `Signer<PlatformAddress>::sign` (line 339, keyed on the 20-byte address hash). The `Signer<IdentityPublicKey>` view that the function name implies (line 245) only consults `private_keys` / `private_keys_in_creation`, both of which remain empty after this constructor runs. The doc comment hand-waves this with "callers must additionally register `IdentityPublicKey` records" — but if the caller has to do that themselves the constructor isn't actually "for identity." A repo-wide grep confirms zero callers. Either (a) populate `private_keys` inside the constructor so identity signing works out of the box, (b) drop it until a real consumer exists, or (c) rename to reflect what it actually populates (e.g. `derive_identity_path_into_address_keys`).
Comment on lines
+51
to
+109
| /// Framework-wide shutdown signal for background tasks. Not | ||
| /// tripped by individual test panics — a single failing test | ||
| /// must not cancel SPV / wait helpers for sibling tests. | ||
| pub cancel_token: CancellationToken, | ||
| /// Installed as the harness's `PlatformEventHandler`; test | ||
| /// wallets clone the `Arc` so `wait_for_balance` wakes on real | ||
| /// events instead of fixed polling. | ||
| pub wait_hub: Arc<WaitEventHub>, | ||
| } | ||
|
|
||
| impl E2eContext { | ||
| /// Lazily build (or reuse) the process-shared context. | ||
| /// Concurrent callers serialise inside `OnceCell` — exactly one | ||
| /// build runs. | ||
| pub async fn init() -> FrameworkResult<&'static Self> { | ||
| CTX.get_or_try_init(Self::build).await | ||
| } | ||
|
|
||
| pub fn sdk(&self) -> &Arc<dash_sdk::Sdk> { | ||
| &self.sdk | ||
| } | ||
|
|
||
| pub fn manager(&self) -> &Arc<PlatformWalletManager<NoPlatformPersistence>> { | ||
| &self.manager | ||
| } | ||
|
|
||
| /// Pre-funded bank wallet — the funding source for tests. | ||
| pub fn bank(&self) -> &BankWallet { | ||
| &self.bank | ||
| } | ||
|
|
||
| /// Persistent test-wallet registry — every `setup` registers, | ||
| /// every `teardown` removes its entry. | ||
| pub fn registry(&self) -> &PersistentTestWalletRegistry { | ||
| &self.registry | ||
| } | ||
|
|
||
| /// `None` while the SPV-based context provider is deferred | ||
| /// (Task #15). | ||
| pub fn spv(&self) -> Option<&Arc<SpvRuntime>> { | ||
| self.spv_runtime.as_ref() | ||
| } | ||
|
|
||
| /// Framework-shutdown signal; background helpers can `select!` | ||
| /// on it for graceful shutdown. | ||
| pub fn cancel_token(&self) -> &CancellationToken { | ||
| &self.cancel_token | ||
| } | ||
|
|
||
| pub fn wait_hub(&self) -> &Arc<WaitEventHub> { | ||
| &self.wait_hub | ||
| } | ||
|
|
||
| async fn build() -> FrameworkResult<E2eContext> { | ||
| let config = Config::from_env()?; | ||
|
|
||
| let (workdir, workdir_lock) = workdir::pick_available_workdir(&config.workdir_base)?; | ||
|
|
||
| let cancel_token = CancellationToken::new(); |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: cancel_token is constructed and exposed but never observed
E2eContext::cancel_token is created at line 109, exposed via the cancel_token() accessor at line 96, and the doc comments promise it backs "graceful shutdown" of background helpers. In practice no code in the framework or test cases ever (a) cancel()s it, or (b) select!s on it — wait_for_balance, the deferred SPV blocks, and the test bodies all ignore it. The token is dead state with a forward-looking accessor that tempts misuse. Either drop the field until shutdown wiring lands (Task #15) or add a tokio::select! arm in wait_for_balance so the documented behavior actually fires.
source: ['claude']
Comment on lines
+31
to
+37
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Default, Serialize, Deserialize)] | ||
| pub enum EntryStatus { | ||
| #[default] | ||
| Active, | ||
| Sweeping, | ||
| Failed, | ||
| } |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: EntryStatus::Sweeping is defined but never set anywhere
The doc comment promises Sweeping is "set transiently so a second process knows the wallet is already being handled." The only set_status call in the codebase is cleanup::sweep_orphans setting EntryStatus::Failed after a failed sweep — no code path ever transitions an entry to Sweeping. Either wire set_status(.., Sweeping) at the start of cleanup::sweep_one (and clear it on success/failure) so the doc claim becomes true, or drop the variant and update the doc.
source: ['claude']
thepastaclaw
suggested changes
May 2, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Two blocking issues remain: the live testnet e2e test still runs in the default cargo test path (no #[ignore]), and the cleanup sweep's per-input filter (*b > 0) admits sub-min_input_amount dust into the explicit input map, which DPP rejects with InputBelowMinimumError — making mixed-balance wallets perpetually un-sweepable. The remaining items are correctness/quality suggestions: a fee-floor mismatch in the sweep gate, error-context loss via FrameworkError::NotImplemented, dead-but-public cancel_token, the misnamed SimpleSigner::from_seed_for_identity, premature pub widening of SDK internals, and the SPV path bypassing the slot-locked workdir. Several single-source security findings were dropped as not meeting the bar.
Reviewed commit: 5515ba9
🔴 2 blocking | 🟡 5 suggestion(s) | 💬 3 nitpick(s)
1 additional finding
🟡 suggestion: `fetch_inputs_with_nonce` / `nonce_inc` promoted to `pub` with no caller outside rs-sdk
packages/rs-sdk/src/platform/transition/address_inputs.rs (lines 12-40)
pub mod address_inputs; at transition.rs:3 and pub fn fetch_inputs_with_nonce / pub fn nonce_inc widen these from pub(crate) to pub. A repo-wide grep finds callers only inside crate::platform::transition::* (address_credit_withdrawal.rs, top_up_identity_from_addresses.rs, shield.rs, transfer_address_funds.rs, put_identity.rs); the e2e framework in rs-platform-wallet does not import them, and rs-platform-wallet production code doesn't either. Once pub, the signatures become a stability commitment — nonce_inc in particular is footgun-prone outside the strict fetch→increment→sign→broadcast flow (it does not protect against double-spending the same nonce in concurrent calls). Revert to pub(crate) (or pub(super)) and widen alongside the first external caller.
🤖 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/tests/e2e/cases/transfer.rs`:
- [BLOCKING] lines 62-63: Live testnet e2e test runs by default; `cargo test` hard-fails without operator env
`transfer_between_two_platform_addresses` has no `#[ignore]` and the module docs explicitly say it "Runs by default". `setup()` calls `Config::from_env()`, which returns `FrameworkError::Bank` when `PLATFORM_WALLET_E2E_BANK_MNEMONIC` is unset (`framework/config.rs`); the test escalates that to a panic via `.expect("e2e setup failed")`. Consequence: a stock `cargo test -p platform-wallet` (or workspace-wide invocation) becomes a hard failure for any contributor or CI job without a funded testnet bank wallet, live DAPI access, and the operator `.env`. The crate's own `tests/spv_sync.rs` follows the standard convention of gating live-network tests behind `#[ignore]`. Re-add the gate so default runs stay green and live coverage is opt-in.
In `packages/rs-platform-wallet/tests/e2e/framework/cleanup.rs`:
- [BLOCKING] lines 217-226: Sweep input filter is `b > 0`; sub-`min_input_amount` inputs make mixed wallets permanently un-sweepable
The new total-balance gate at lines 114 and 155 uses `min_input_amount(version)` (good), but the per-input filter inside `sweep_platform_addresses` is still `filter(|(_, b)| *b > 0)`. DPP enforces `min_input_amount` per individual input (`packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funds_transfer_transition/v0/state_transition_validation.rs:157-167` — the loop returns `InputBelowMinimumError` for any amount below the threshold), not on the sum. So a wallet with addr_A=50M and addr_B=50K passes the total gate (50.05M >> 100K) but the broadcast fails with `InputBelowMinimumError`. `teardown_one` returns the error and `sweep_orphans` marks the entry `EntryStatus::Failed` and retries on every startup — it can never succeed without manual intervention. Mirror the production auto-selector and drop sub-`min_input_amount` inputs from the explicit map (the unsweepable dust on those addresses is the same loss already accepted by the wallet-level skip path).
- [SUGGESTION] lines 111-169: Sweep gate is keyed to `min_input_amount` (100K), not the minimum transfer fee (~6.5M)
Both `sweep_one` (line 114) and `teardown_one` (line 155) treat `min_input_amount` as the sweep gate. On current platform versions that value is `100_000`, but the static 1-input/1-output address-transfer fee floor is already `address_funds_transfer_input_cost + address_funds_transfer_output_cost = 6_500_000` (`packages/rs-platform-version/src/version/fee/state_transition_min_fees/v1.rs:14-15`), and this PR's own transfer test commentary notes real chain-time fees closer to ~15M while platform bug #3040 is open (`tests/e2e/cases/transfer.rs:24-33`). So wallets with totals in `[100k, 6.5M)` go down the sweep path even though every `ReduceOutput(0)` attempt will fail (output goes negative or below `min_output_amount`), leaving the orphan permanently in `Failed`. Gate on a fee-aware floor (e.g. the static min-fee plus a safety margin) instead of just the per-input minimum.
In `packages/simple-signer/src/signer.rs`:
- [SUGGESTION] lines 197-241: `from_seed_for_identity` is misleadingly named, half-functional, and unused
The (feature-gated) constructor derives DIP-9 identity-authentication ECDSA secp256k1 keys but inserts them into `address_private_keys: BTreeMap<[u8; 20], [u8; 32]>` — the map consumed by `Signer<PlatformAddress>::sign` (line 339, keyed on the 20-byte address hash). The `Signer<IdentityPublicKey>` impl that the function name implies (line 245) only consults `private_keys` / `private_keys_in_creation`, both of which remain empty after this constructor runs. The doc comment hand-waves this with "callers must additionally register `IdentityPublicKey` records via `add_identity_public_key`" — but if the caller has to do that themselves the constructor isn't actually "for identity." A repo-wide grep confirms zero callers outside this file. Either populate `private_keys` inside the constructor so identity signing works out of the box, drop it until a real consumer exists, or rename to reflect what it actually populates (e.g. `derive_identity_path_into_address_keys`). Beyond the API-quality issue, the dual-keystore reachability (same secret reachable via both signer pathways) is the kind of cross-purpose-key footgun worth eliminating before any production caller arrives.
In `packages/rs-platform-wallet/tests/e2e/framework/sdk.rs`:
- [SUGGESTION] lines 32-41: `FrameworkError::NotImplemented` used as a generic runtime-error wrapper, dropping the underlying error
`SdkBuilder::build()` failure is a real runtime error, not an unimplemented-feature path, but it's mapped to `FrameworkError::NotImplemented("sdk::build_sdk — SdkBuilder::build failed (see logs)")`. The actual error `e` is only emitted via a side-effect `tracing::error!` and then discarded — callers that pattern-match on the `Result` (or render it for CI failure summaries) see only the static `&str`. The same pattern recurs at lines 68-77, 100-103, 113-122 here and at `framework/spv.rs:223-226, 241-244`. The `FrameworkError` enum already has `Wallet(String)`, `Bank(String)`, `Config(String)` variants for this purpose — add `Sdk(String)` / `Spv(String)` variants and propagate `e.to_string()` so CI logs and downstream callers actually receive the underlying message.
In `packages/rs-sdk/src/platform/transition/address_inputs.rs`:
- [SUGGESTION] lines 12-40: `fetch_inputs_with_nonce` / `nonce_inc` promoted to `pub` with no caller outside rs-sdk
`pub mod address_inputs;` at `transition.rs:3` and `pub fn fetch_inputs_with_nonce` / `pub fn nonce_inc` widen these from `pub(crate)` to `pub`. A repo-wide grep finds callers only inside `crate::platform::transition::*` (`address_credit_withdrawal.rs`, `top_up_identity_from_addresses.rs`, `shield.rs`, `transfer_address_funds.rs`, `put_identity.rs`); the e2e framework in rs-platform-wallet does not import them, and rs-platform-wallet production code doesn't either. Once `pub`, the signatures become a stability commitment — `nonce_inc` in particular is footgun-prone outside the strict fetch→increment→sign→broadcast flow (it does not protect against double-spending the same nonce in concurrent calls). Revert to `pub(crate)` (or `pub(super)`) and widen alongside the first external caller.
In `packages/rs-platform-wallet/tests/e2e/framework/spv.rs`:
- [SUGGESTION] lines 210-247: Retained SPV path bypasses the slot-locked workdir
`E2eContext::build` acquires a unique slot via `pick_available_workdir` and stores it in `workdir`, but `build_client_config` derives its storage path from `config.workdir_base.join("spv-data")` (line 216). When the commented-out SPV block in `harness.rs:131-147` is re-enabled (Task #15), every concurrent process will share `<base>/spv-data` instead of using the locked slot directory, defeating the cross-process isolation mechanism and creating avoidable RocksDB/SPV state contention. Because the SPV module is intentionally kept compilable for re-enablement, fix it now — pass the slot workdir into `build_client_config` so SPV storage tracks the lock.
Comment on lines
+217
to
+226
| let inputs: BTreeMap<PlatformAddress, Credits> = wallet | ||
| .platform() | ||
| .addresses_with_balances() | ||
| .await | ||
| .into_iter() | ||
| .filter(|(_, b)| *b > 0) | ||
| .collect(); | ||
| if inputs.is_empty() { | ||
| return Ok(()); | ||
| } |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Sweep input filter is b > 0; sub-min_input_amount inputs make mixed wallets permanently un-sweepable
The new total-balance gate at lines 114 and 155 uses min_input_amount(version) (good), but the per-input filter inside sweep_platform_addresses is still filter(|(_, b)| *b > 0). DPP enforces min_input_amount per individual input (packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funds_transfer_transition/v0/state_transition_validation.rs:157-167 — the loop returns InputBelowMinimumError for any amount below the threshold), not on the sum. So a wallet with addr_A=50M and addr_B=50K passes the total gate (50.05M >> 100K) but the broadcast fails with InputBelowMinimumError. teardown_one returns the error and sweep_orphans marks the entry EntryStatus::Failed and retries on every startup — it can never succeed without manual intervention. Mirror the production auto-selector and drop sub-min_input_amount inputs from the explicit map (the unsweepable dust on those addresses is the same loss already accepted by the wallet-level skip path).
💡 Suggested change
Suggested change
| let inputs: BTreeMap<PlatformAddress, Credits> = wallet | |
| .platform() | |
| .addresses_with_balances() | |
| .await | |
| .into_iter() | |
| .filter(|(_, b)| *b > 0) | |
| .collect(); | |
| if inputs.is_empty() { | |
| return Ok(()); | |
| } | |
| let dust_gate = min_input_amount(PlatformVersion::latest()); | |
| let inputs: BTreeMap<PlatformAddress, Credits> = wallet | |
| .platform() | |
| .addresses_with_balances() | |
| .await | |
| .into_iter() | |
| .filter(|(_, b)| *b >= dust_gate) | |
| .collect(); | |
| if inputs.is_empty() { | |
| return Ok(()); | |
| } |
source: ['claude', 'codex']
🤖 Fix this 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/tests/e2e/framework/cleanup.rs`:
- [BLOCKING] lines 217-226: Sweep input filter is `b > 0`; sub-`min_input_amount` inputs make mixed wallets permanently un-sweepable
The new total-balance gate at lines 114 and 155 uses `min_input_amount(version)` (good), but the per-input filter inside `sweep_platform_addresses` is still `filter(|(_, b)| *b > 0)`. DPP enforces `min_input_amount` per individual input (`packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funds_transfer_transition/v0/state_transition_validation.rs:157-167` — the loop returns `InputBelowMinimumError` for any amount below the threshold), not on the sum. So a wallet with addr_A=50M and addr_B=50K passes the total gate (50.05M >> 100K) but the broadcast fails with `InputBelowMinimumError`. `teardown_one` returns the error and `sweep_orphans` marks the entry `EntryStatus::Failed` and retries on every startup — it can never succeed without manual intervention. Mirror the production auto-selector and drop sub-`min_input_amount` inputs from the explicit map (the unsweepable dust on those addresses is the same loss already accepted by the wallet-level skip path).
Comment on lines
+111
to
+169
| let platform_version = PlatformVersion::latest(); | ||
| let dust_gate = min_input_amount(platform_version); | ||
| let total = wallet.platform().total_credits().await; | ||
| if total >= dust_gate { | ||
| sweep_platform_addresses(&wallet, &signer, bank.primary_receive_address()).await?; | ||
| } else { | ||
| tracing::debug!( | ||
| wallet_id = %hex::encode(hash), | ||
| total, | ||
| min_input = dust_gate, | ||
| "orphan platform total below protocol min_input_amount; skipping" | ||
| ); | ||
| } | ||
| sweep_identities(&wallet).await?; | ||
| sweep_core_addresses(&wallet).await?; | ||
| sweep_unused_core_asset_locks(&wallet).await?; | ||
| sweep_shielded(&wallet).await?; | ||
|
|
||
| // Best-effort manager unregister so SPV stops tracking the | ||
| // wallet's addresses on subsequent passes. | ||
| if let Err(err) = manager.remove_wallet(hash).await { | ||
| tracing::warn!( | ||
| target: "platform_wallet::e2e::cleanup", | ||
| wallet_id = %hex::encode(hash), | ||
| error = %err, | ||
| "manager unregister failed after sweep; wallet remains tracked" | ||
| ); | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Per-test teardown: drain back to bank, drop the registry entry, | ||
| /// and unregister from the manager. Best-effort — failures retain | ||
| /// the entry so the next startup's [`sweep_orphans`] retries. | ||
| pub async fn teardown_one( | ||
| manager: &Arc<PlatformWalletManager<NoPlatformPersistence>>, | ||
| bank: &BankWallet, | ||
| registry: &PersistentTestWalletRegistry, | ||
| test_wallet: &TestWallet, | ||
| ) -> FrameworkResult<()> { | ||
| test_wallet.sync_balances().await?; | ||
| let platform_version = PlatformVersion::latest(); | ||
| let dust_gate = min_input_amount(platform_version); | ||
| let total = test_wallet.total_credits().await; | ||
| if total >= dust_gate { | ||
| sweep_platform_addresses( | ||
| test_wallet.platform_wallet(), | ||
| test_wallet.address_signer(), | ||
| bank.primary_receive_address(), | ||
| ) | ||
| .await?; | ||
| } else { | ||
| tracing::debug!( | ||
| wallet_id = %hex::encode(test_wallet.id()), | ||
| total, | ||
| min_input = dust_gate, | ||
| "test wallet total below protocol min_input_amount; skipping platform sweep" | ||
| ); | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Sweep gate is keyed to min_input_amount (100K), not the minimum transfer fee (~6.5M)
Both sweep_one (line 114) and teardown_one (line 155) treat min_input_amount as the sweep gate. On current platform versions that value is 100_000, but the static 1-input/1-output address-transfer fee floor is already address_funds_transfer_input_cost + address_funds_transfer_output_cost = 6_500_000 (packages/rs-platform-version/src/version/fee/state_transition_min_fees/v1.rs:14-15), and this PR's own transfer test commentary notes real chain-time fees closer to ~15M while platform bug #3040 is open (tests/e2e/cases/transfer.rs:24-33). So wallets with totals in [100k, 6.5M) go down the sweep path even though every ReduceOutput(0) attempt will fail (output goes negative or below min_output_amount), leaving the orphan permanently in Failed. Gate on a fee-aware floor (e.g. the static min-fee plus a safety margin) instead of just the per-input minimum.
source: ['codex']
🤖 Fix this 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/tests/e2e/framework/cleanup.rs`:
- [SUGGESTION] lines 111-169: Sweep gate is keyed to `min_input_amount` (100K), not the minimum transfer fee (~6.5M)
Both `sweep_one` (line 114) and `teardown_one` (line 155) treat `min_input_amount` as the sweep gate. On current platform versions that value is `100_000`, but the static 1-input/1-output address-transfer fee floor is already `address_funds_transfer_input_cost + address_funds_transfer_output_cost = 6_500_000` (`packages/rs-platform-version/src/version/fee/state_transition_min_fees/v1.rs:14-15`), and this PR's own transfer test commentary notes real chain-time fees closer to ~15M while platform bug #3040 is open (`tests/e2e/cases/transfer.rs:24-33`). So wallets with totals in `[100k, 6.5M)` go down the sweep path even though every `ReduceOutput(0)` attempt will fail (output goes negative or below `min_output_amount`), leaving the orphan permanently in `Failed`. Gate on a fee-aware floor (e.g. the static min-fee plus a safety margin) instead of just the per-input minimum.
Comment on lines
+197
to
+241
| /// Build a [`SimpleSigner`] populated with the DIP-9 identity-authentication | ||
| /// (ECDSA) gap window for `identity_index`. The returned signer holds raw | ||
| /// secp256k1 secrets keyed on `(pubkey-hash, secret)` via | ||
| /// [`Self::address_private_keys`] — callers that need a `Signer<IdentityPublicKey>` | ||
| /// view must additionally register `IdentityPublicKey` records via | ||
| /// [`Self::add_identity_public_key`] using the matching pubkey bytes. | ||
| #[cfg(feature = "derive")] | ||
| pub fn from_seed_for_identity( | ||
| seed: &[u8; 64], | ||
| network: key_wallet::Network, | ||
| identity_index: u32, | ||
| gap_limit: u32, | ||
| ) -> Result<Self, SimpleSignerError> { | ||
| use key_wallet::bip32::KeyDerivationType; | ||
| use key_wallet::wallet::root_extended_keys::RootExtendedPrivKey; | ||
| use key_wallet::DerivationPath; | ||
|
|
||
| let root_priv = RootExtendedPrivKey::new_master(seed) | ||
| .map_err(|err| SimpleSignerError::InvalidSeed(err.to_string()))?; | ||
| let root_xpriv = root_priv.to_extended_priv_key(network); | ||
|
|
||
| let secp = Secp256k1::new(); | ||
| let mut signer = Self::default(); | ||
| for key_index in 0..gap_limit { | ||
| let leaf_path = DerivationPath::identity_authentication_path( | ||
| network, | ||
| KeyDerivationType::ECDSA, | ||
| identity_index, | ||
| key_index, | ||
| ); | ||
| let xpriv = root_xpriv.derive_priv(&secp, &leaf_path).map_err(|err| { | ||
| SimpleSignerError::DerivePriv { | ||
| index: key_index, | ||
| message: err.to_string(), | ||
| } | ||
| })?; | ||
| let secret: SecretKey = xpriv.private_key; | ||
| let pubkey: PublicKey = PublicKey::from_secret_key(&secp, &secret); | ||
| let pkh = ripemd160_sha256(&pubkey.serialize()); | ||
| signer | ||
| .address_private_keys | ||
| .insert(pkh, secret.secret_bytes()); | ||
| } | ||
| Ok(signer) | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: from_seed_for_identity is misleadingly named, half-functional, and unused
The (feature-gated) constructor derives DIP-9 identity-authentication ECDSA secp256k1 keys but inserts them into address_private_keys: BTreeMap<[u8; 20], [u8; 32]> — the map consumed by Signer<PlatformAddress>::sign (line 339, keyed on the 20-byte address hash). The Signer<IdentityPublicKey> impl that the function name implies (line 245) only consults private_keys / private_keys_in_creation, both of which remain empty after this constructor runs. The doc comment hand-waves this with "callers must additionally register IdentityPublicKey records via add_identity_public_key" — but if the caller has to do that themselves the constructor isn't actually "for identity." A repo-wide grep confirms zero callers outside this file. Either populate private_keys inside the constructor so identity signing works out of the box, drop it until a real consumer exists, or rename to reflect what it actually populates (e.g. derive_identity_path_into_address_keys). Beyond the API-quality issue, the dual-keystore reachability (same secret reachable via both signer pathways) is the kind of cross-purpose-key footgun worth eliminating before any production caller arrives.
source: ['claude', 'codex']
🤖 Fix this 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/simple-signer/src/signer.rs`:
- [SUGGESTION] lines 197-241: `from_seed_for_identity` is misleadingly named, half-functional, and unused
The (feature-gated) constructor derives DIP-9 identity-authentication ECDSA secp256k1 keys but inserts them into `address_private_keys: BTreeMap<[u8; 20], [u8; 32]>` — the map consumed by `Signer<PlatformAddress>::sign` (line 339, keyed on the 20-byte address hash). The `Signer<IdentityPublicKey>` impl that the function name implies (line 245) only consults `private_keys` / `private_keys_in_creation`, both of which remain empty after this constructor runs. The doc comment hand-waves this with "callers must additionally register `IdentityPublicKey` records via `add_identity_public_key`" — but if the caller has to do that themselves the constructor isn't actually "for identity." A repo-wide grep confirms zero callers outside this file. Either populate `private_keys` inside the constructor so identity signing works out of the box, drop it until a real consumer exists, or rename to reflect what it actually populates (e.g. `derive_identity_path_into_address_keys`). Beyond the API-quality issue, the dual-keystore reachability (same secret reachable via both signer pathways) is the kind of cross-purpose-key footgun worth eliminating before any production caller arrives.
Comment on lines
+109
to
+187
| let signer = make_platform_signer(&seed_bytes, network)?; | ||
|
|
||
| let platform_version = PlatformVersion::latest(); | ||
| let dust_gate = min_input_amount(platform_version); | ||
| let total = wallet.platform().total_credits().await; | ||
| if total >= dust_gate { | ||
| sweep_platform_addresses(&wallet, &signer, bank.primary_receive_address()).await?; | ||
| } else { | ||
| tracing::debug!( | ||
| wallet_id = %hex::encode(hash), | ||
| total, | ||
| min_input = dust_gate, | ||
| "orphan platform total below protocol min_input_amount; skipping" | ||
| ); | ||
| } | ||
| sweep_identities(&wallet).await?; | ||
| sweep_core_addresses(&wallet).await?; | ||
| sweep_unused_core_asset_locks(&wallet).await?; | ||
| sweep_shielded(&wallet).await?; | ||
|
|
||
| // Best-effort manager unregister so SPV stops tracking the | ||
| // wallet's addresses on subsequent passes. | ||
| if let Err(err) = manager.remove_wallet(hash).await { | ||
| tracing::warn!( | ||
| target: "platform_wallet::e2e::cleanup", | ||
| wallet_id = %hex::encode(hash), | ||
| error = %err, | ||
| "manager unregister failed after sweep; wallet remains tracked" | ||
| ); | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Per-test teardown: drain back to bank, drop the registry entry, | ||
| /// and unregister from the manager. Best-effort — failures retain | ||
| /// the entry so the next startup's [`sweep_orphans`] retries. | ||
| pub async fn teardown_one( | ||
| manager: &Arc<PlatformWalletManager<NoPlatformPersistence>>, | ||
| bank: &BankWallet, | ||
| registry: &PersistentTestWalletRegistry, | ||
| test_wallet: &TestWallet, | ||
| ) -> FrameworkResult<()> { | ||
| test_wallet.sync_balances().await?; | ||
| let platform_version = PlatformVersion::latest(); | ||
| let dust_gate = min_input_amount(platform_version); | ||
| let total = test_wallet.total_credits().await; | ||
| if total >= dust_gate { | ||
| sweep_platform_addresses( | ||
| test_wallet.platform_wallet(), | ||
| test_wallet.address_signer(), | ||
| bank.primary_receive_address(), | ||
| ) | ||
| .await?; | ||
| } else { | ||
| tracing::debug!( | ||
| wallet_id = %hex::encode(test_wallet.id()), | ||
| total, | ||
| min_input = dust_gate, | ||
| "test wallet total below protocol min_input_amount; skipping platform sweep" | ||
| ); | ||
| } | ||
| sweep_identities(test_wallet.platform_wallet()).await?; | ||
| sweep_core_addresses(test_wallet.platform_wallet()).await?; | ||
| sweep_unused_core_asset_locks(test_wallet.platform_wallet()).await?; | ||
| sweep_shielded(test_wallet.platform_wallet()).await?; | ||
|
|
||
| // Drop the registry entry first so an unregister failure | ||
| // doesn't leak it; the wallet has no balance left to recover. | ||
| registry.remove(&test_wallet.id())?; | ||
| if let Err(err) = manager.remove_wallet(&test_wallet.id()).await { | ||
| tracing::warn!( | ||
| target: "platform_wallet::e2e::cleanup", | ||
| wallet_id = %hex::encode(test_wallet.id()), | ||
| error = %err, | ||
| "manager unregister failed after teardown; wallet remains tracked" | ||
| ); | ||
| } | ||
| Ok(()) | ||
| } |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: Sub-min_input_amount balances are silently dropped from the registry
When total < dust_gate, both sweep_one (lines 113-123) and teardown_one (lines 155-169) skip the sweep — sweep_orphans then treats the Ok(()) as a successful recovery and removes the entry (line 62), and teardown_one unconditionally calls registry.remove(...) (line 177). Because dust_gate is PlatformVersion::min_input_amount (currently 100K), the funds in the dropped band are protocol-unsweepable so removing the entry is defensible — but small refund / fee-dust residues then silently disappear from the registry with no audit trail. Consider keeping the entry tagged EntryStatus::Failed with a one-line note like "balance below min_input_amount" so an operator can see what was abandoned, rather than removing it.
source: ['claude', 'codex']
Comment on lines
+51
to
+109
| /// Framework-wide shutdown signal for background tasks. Not | ||
| /// tripped by individual test panics — a single failing test | ||
| /// must not cancel SPV / wait helpers for sibling tests. | ||
| pub cancel_token: CancellationToken, | ||
| /// Installed as the harness's `PlatformEventHandler`; test | ||
| /// wallets clone the `Arc` so `wait_for_balance` wakes on real | ||
| /// events instead of fixed polling. | ||
| pub wait_hub: Arc<WaitEventHub>, | ||
| } | ||
|
|
||
| impl E2eContext { | ||
| /// Lazily build (or reuse) the process-shared context. | ||
| /// Concurrent callers serialise inside `OnceCell` — exactly one | ||
| /// build runs. | ||
| pub async fn init() -> FrameworkResult<&'static Self> { | ||
| CTX.get_or_try_init(Self::build).await | ||
| } | ||
|
|
||
| pub fn sdk(&self) -> &Arc<dash_sdk::Sdk> { | ||
| &self.sdk | ||
| } | ||
|
|
||
| pub fn manager(&self) -> &Arc<PlatformWalletManager<NoPlatformPersistence>> { | ||
| &self.manager | ||
| } | ||
|
|
||
| /// Pre-funded bank wallet — the funding source for tests. | ||
| pub fn bank(&self) -> &BankWallet { | ||
| &self.bank | ||
| } | ||
|
|
||
| /// Persistent test-wallet registry — every `setup` registers, | ||
| /// every `teardown` removes its entry. | ||
| pub fn registry(&self) -> &PersistentTestWalletRegistry { | ||
| &self.registry | ||
| } | ||
|
|
||
| /// `None` while the SPV-based context provider is deferred | ||
| /// (Task #15). | ||
| pub fn spv(&self) -> Option<&Arc<SpvRuntime>> { | ||
| self.spv_runtime.as_ref() | ||
| } | ||
|
|
||
| /// Framework-shutdown signal; background helpers can `select!` | ||
| /// on it for graceful shutdown. | ||
| pub fn cancel_token(&self) -> &CancellationToken { | ||
| &self.cancel_token | ||
| } | ||
|
|
||
| pub fn wait_hub(&self) -> &Arc<WaitEventHub> { | ||
| &self.wait_hub | ||
| } | ||
|
|
||
| async fn build() -> FrameworkResult<E2eContext> { | ||
| let config = Config::from_env()?; | ||
|
|
||
| let (workdir, workdir_lock) = workdir::pick_available_workdir(&config.workdir_base)?; | ||
|
|
||
| let cancel_token = CancellationToken::new(); |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: cancel_token is constructed and exposed but never observed
E2eContext::cancel_token is created at line 109, exposed via the cancel_token() accessor at line 96, and the doc comments promise it backs "graceful shutdown" of background helpers. In practice no code in the framework or test cases ever (a) cancel()s it, or (b) select!s on it — wait_for_balance, the deferred SPV blocks, and the test bodies all ignore it. The token is dead state with a forward-looking accessor that tempts misuse. Either drop the field until shutdown wiring lands (Task #15) or add a tokio::select! arm in wait_for_balance so the documented behavior actually fires.
source: ['claude']
Comment on lines
+100
to
+120
| /// Insert (or overwrite) an entry, persisting before returning. | ||
| /// Last-write-wins on duplicate: failing the insert would risk | ||
| /// leaking the new entry, while a sweep can still recover. | ||
| pub fn insert(&self, hash: WalletSeedHash, entry: RegistryEntry) -> FrameworkResult<()> { | ||
| let snapshot = { | ||
| let mut guard = self.state.lock(); | ||
| guard.insert(hash, entry); | ||
| guard.clone() | ||
| }; | ||
| atomic_write_json(&self.path, &snapshot) | ||
| } | ||
|
|
||
| /// Remove an entry. Missing-key is OK — teardown is best-effort. | ||
| pub fn remove(&self, hash: &WalletSeedHash) -> FrameworkResult<()> { | ||
| let snapshot = { | ||
| let mut guard = self.state.lock(); | ||
| guard.remove(hash); | ||
| guard.clone() | ||
| }; | ||
| atomic_write_json(&self.path, &snapshot) | ||
| } |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: Test-wallet seeds persisted hex-plaintext to JSON without restrictive file mode
atomic_write_json writes the registry — which contains hex-encoded 64-byte BIP-39 seeds in RegistryEntry::seed_hex — via tempfile::NamedTempFile then persist, with no chmod/0600 step. Default file mode honors umask, so on a multi-user host with a permissive umask another local user could read in-flight test seeds from <workdir>/test_wallets.json. Risk is bounded: seeds are OsRng-generated, ephemeral, scoped to one test run, used only on testnet, and never the bank mnemonic; the workdir defaults to $TMPDIR/dash-platform-wallet-e2e which is typically user-private. Defense-in-depth: set mode 0600 on the temp file before persist, or document that the workdir must be on a user-private mount.
source: ['claude']
thepastaclaw
suggested changes
May 4, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Test-only PR adding an e2e harness for rs-platform-wallet plus a small production surface (auto_select_inputs fix, simple-signer derive feature, two pub-visibility bumps). One blocking issue: the live testnet e2e test had its #[ignore] removed but the CI workflow runs platform-wallet --all-features with no env wiring or filter, so it will panic in every CI run. Several smaller architecture / robustness concerns in the framework and unused public-API surface.
Reviewed commit: aad27c5
🔴 1 blocking | 🟡 5 suggestion(s) | 💬 3 nitpick(s)
1 additional finding
💬 nitpick: Inconsistent invariant guarding: debug_assert + runtime check here, debug_assert only in sibling helper
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs (lines 343-360)
select_inputs_deduct_from_input is private and its only caller (auto_select_inputs) has already pattern-matched the strategy before dispatching here. The function still re-checks the same invariant twice — a debug_assert! (343-350) followed by a runtime if !matches!(...) (351-360) returning an error string referencing an internal function name. The companion select_inputs_reduce_output (570-574) keeps only the debug_assert!. Pick one pattern for private invariant guards and apply it consistently.
🤖 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/tests/e2e/cases/transfer.rs`:
- [BLOCKING] lines 62-63: Live testnet e2e test will panic in CI: #[ignore] removed but workflow runs platform-wallet --all-features with no env wiring or test filter
`transfer_between_two_platform_addresses` is no longer `#[ignore]`. `.github/workflows/tests-rs-workspace.yml` (lines 144-171 and 308-335) runs `cargo nextest --package platform-wallet --all-features --locked` with only an `-E 'not test(~shield)'` filter — no env wiring for `PLATFORM_WALLET_E2E_BANK_MNEMONIC`, no exclusion of the `e2e` test binary, and no `offline-testing`-style feature gate on platform-wallet. Without the env var, `Config::from_env()` returns `FrameworkError::Bank("PLATFORM_WALLET_E2E_BANK_MNEMONIC not set ...")`, `setup().await.expect("e2e setup failed")` panics, and CI fails on every run. Either restore `#[ignore]` until the workflow is updated, or land the workflow change (filter + env wiring) in this PR.
In `packages/rs-platform-wallet/tests/e2e/framework/cleanup.rs`:
- [SUGGESTION] lines 217-223: sweep_platform_addresses includes dust inputs the protocol will reject
`sweep_platform_addresses` collects every address with `balance > 0` and feeds the full map into `InputSelection::Explicit`. The DPP `address_funds_transfer_transition/v0/state_transition_validation.rs:159` rejects any input `< min_input_amount`. The wallet-level gates at lines 113-114 and 154-155 only check `total >= min_input_amount`, not per-address balance, so a wallet with one spendable address plus any sub-minimum dust address (easy to produce once `ReduceOutput(0)` leaves remainders or future tests do partial spends) will fail teardown forever, leaving the registry entry behind. The current single test happens to leave both addresses well above min, but the framework is meant to generalize. Filter individual balances against `min_input_amount` instead of relying on the total gate.
In `packages/rs-sdk/src/platform/transition.rs`:
- [SUGGESTION] line 3: address_inputs promoted to pub with no external consumer
`address_inputs` (and `fetch_inputs_with_nonce` / `nonce_inc`) flipped from `pub(crate)` to `pub`, but every caller in the workspace is still inside rs-sdk itself (`transfer_address_funds.rs`, `address_credit_withdrawal.rs`, `top_up_identity_from_addresses.rs`, `put_identity.rs`, `shield.rs`). The e2e framework added in this PR does not call either function. The signatures expose internal SDK types (`dpp::AddressNonce`, `drive_proof_verifier::types::AddressInfos`, `BTreeMap<PlatformAddress, ...>`) and once `pub`, downgrading is a breaking change. Either land a justified external consumer alongside the visibility bump or keep these `pub(crate)`.
In `packages/simple-signer/src/signer.rs`:
- [SUGGESTION] lines 197-241: from_seed_for_identity is unused in this PR and has a misleading contract
`from_seed_for_identity` populates `self.address_private_keys` (keyed on pubkey-hash, used by `Signer<PlatformAddress>` at lines 379-385) but does not populate `self.private_keys: BTreeMap<IdentityPublicKey, [u8; 32]>`. Per the impl at lines 247-258, `Signer<IdentityPublicKey>::sign` reads from `private_keys` only, so the returned signer cannot satisfy that trait despite the function name. The doc-comment honestly admits callers must additionally call `add_identity_public_key`, but the e2e framework only uses `from_seed_for_platform_address_account`; nothing in the PR consumes `from_seed_for_identity`. Either drop it until a real consumer lands or rename to reflect that it populates the address-signing path (e.g. `from_seed_for_identity_authentication_addresses`) so future callers don't expect a turnkey `Signer<IdentityPublicKey>`.
In `packages/rs-platform-wallet/tests/e2e/framework/sdk.rs`:
- [SUGGESTION] lines 35-38: FrameworkError::NotImplemented misused as a generic error envelope; underlying cause is dropped
`SdkBuilder::build` (and several sibling sites in sdk.rs and spv.rs) wrap a real runtime failure in `FrameworkError::NotImplemented`, whose `Display` reads "e2e framework not yet implemented: ...". The actual error is logged at error-level then discarded. Operators reading test output will see a misleading "not implemented" message when SDK construction in fact failed at runtime, and downstream `Result` matching cannot recover the cause. Add a dedicated `Sdk(String)` (and `Spv(String)`) variant or carry the source via `#[source] Box<dyn Error + Send + Sync>` so the chain survives.
In `packages/rs-platform-wallet/tests/e2e/framework/registry.rs`:
- [SUGGESTION] lines 103-132: Registry mutates in-memory state before the JSON write succeeds
`insert`, `remove`, and `set_status` all lock, mutate `self.state`, clone the snapshot, drop the lock, and only then call `atomic_write_json`. If the write fails, the method returns `Err` but the in-memory map has already changed. That violates the module's own "persist before returning" contract: an `insert` failure leaves an in-memory orphan with no disk record (next-run sweep won't see it), and a `remove` failure forgets the entry in memory while the disk entry persists. Build the snapshot first, persist it, then swap it into `self.state` only after the write succeeds.
Comment on lines
+217
to
+223
| let inputs: BTreeMap<PlatformAddress, Credits> = wallet | ||
| .platform() | ||
| .addresses_with_balances() | ||
| .await | ||
| .into_iter() | ||
| .filter(|(_, b)| *b > 0) | ||
| .collect(); |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: sweep_platform_addresses includes dust inputs the protocol will reject
sweep_platform_addresses collects every address with balance > 0 and feeds the full map into InputSelection::Explicit. The DPP address_funds_transfer_transition/v0/state_transition_validation.rs:159 rejects any input < min_input_amount. The wallet-level gates at lines 113-114 and 154-155 only check total >= min_input_amount, not per-address balance, so a wallet with one spendable address plus any sub-minimum dust address (easy to produce once ReduceOutput(0) leaves remainders or future tests do partial spends) will fail teardown forever, leaving the registry entry behind. The current single test happens to leave both addresses well above min, but the framework is meant to generalize. Filter individual balances against min_input_amount instead of relying on the total gate.
💡 Suggested change
Suggested change
| let inputs: BTreeMap<PlatformAddress, Credits> = wallet | |
| .platform() | |
| .addresses_with_balances() | |
| .await | |
| .into_iter() | |
| .filter(|(_, b)| *b > 0) | |
| .collect(); | |
| let min_input = PlatformVersion::latest() | |
| .dpp | |
| .state_transitions | |
| .address_funds | |
| .min_input_amount; | |
| let inputs: BTreeMap<PlatformAddress, Credits> = wallet | |
| .platform() | |
| .addresses_with_balances() | |
| .await | |
| .into_iter() | |
| .filter(|(_, b)| *b >= min_input) | |
| .collect(); |
source: ['codex']
🤖 Fix this 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/tests/e2e/framework/cleanup.rs`:
- [SUGGESTION] lines 217-223: sweep_platform_addresses includes dust inputs the protocol will reject
`sweep_platform_addresses` collects every address with `balance > 0` and feeds the full map into `InputSelection::Explicit`. The DPP `address_funds_transfer_transition/v0/state_transition_validation.rs:159` rejects any input `< min_input_amount`. The wallet-level gates at lines 113-114 and 154-155 only check `total >= min_input_amount`, not per-address balance, so a wallet with one spendable address plus any sub-minimum dust address (easy to produce once `ReduceOutput(0)` leaves remainders or future tests do partial spends) will fail teardown forever, leaving the registry entry behind. The current single test happens to leave both addresses well above min, but the framework is meant to generalize. Filter individual balances against `min_input_amount` instead of relying on the total gate.
| //! State transitions used to put changed objects to the Dash Platform. | ||
| pub mod address_credit_withdrawal; | ||
| pub(crate) mod address_inputs; | ||
| pub mod address_inputs; |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: address_inputs promoted to pub with no external consumer
address_inputs (and fetch_inputs_with_nonce / nonce_inc) flipped from pub(crate) to pub, but every caller in the workspace is still inside rs-sdk itself (transfer_address_funds.rs, address_credit_withdrawal.rs, top_up_identity_from_addresses.rs, put_identity.rs, shield.rs). The e2e framework added in this PR does not call either function. The signatures expose internal SDK types (dpp::AddressNonce, drive_proof_verifier::types::AddressInfos, BTreeMap<PlatformAddress, ...>) and once pub, downgrading is a breaking change. Either land a justified external consumer alongside the visibility bump or keep these pub(crate).
💡 Suggested change
Suggested change
| pub mod address_inputs; | |
| pub(crate) mod address_inputs; |
source: ['claude']
🤖 Fix this 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-sdk/src/platform/transition.rs`:
- [SUGGESTION] line 3: address_inputs promoted to pub with no external consumer
`address_inputs` (and `fetch_inputs_with_nonce` / `nonce_inc`) flipped from `pub(crate)` to `pub`, but every caller in the workspace is still inside rs-sdk itself (`transfer_address_funds.rs`, `address_credit_withdrawal.rs`, `top_up_identity_from_addresses.rs`, `put_identity.rs`, `shield.rs`). The e2e framework added in this PR does not call either function. The signatures expose internal SDK types (`dpp::AddressNonce`, `drive_proof_verifier::types::AddressInfos`, `BTreeMap<PlatformAddress, ...>`) and once `pub`, downgrading is a breaking change. Either land a justified external consumer alongside the visibility bump or keep these `pub(crate)`.
Comment on lines
+197
to
+241
| /// Build a [`SimpleSigner`] populated with the DIP-9 identity-authentication | ||
| /// (ECDSA) gap window for `identity_index`. The returned signer holds raw | ||
| /// secp256k1 secrets keyed on `(pubkey-hash, secret)` via | ||
| /// [`Self::address_private_keys`] — callers that need a `Signer<IdentityPublicKey>` | ||
| /// view must additionally register `IdentityPublicKey` records via | ||
| /// [`Self::add_identity_public_key`] using the matching pubkey bytes. | ||
| #[cfg(feature = "derive")] | ||
| pub fn from_seed_for_identity( | ||
| seed: &[u8; 64], | ||
| network: key_wallet::Network, | ||
| identity_index: u32, | ||
| gap_limit: u32, | ||
| ) -> Result<Self, SimpleSignerError> { | ||
| use key_wallet::bip32::KeyDerivationType; | ||
| use key_wallet::wallet::root_extended_keys::RootExtendedPrivKey; | ||
| use key_wallet::DerivationPath; | ||
|
|
||
| let root_priv = RootExtendedPrivKey::new_master(seed) | ||
| .map_err(|err| SimpleSignerError::InvalidSeed(err.to_string()))?; | ||
| let root_xpriv = root_priv.to_extended_priv_key(network); | ||
|
|
||
| let secp = Secp256k1::new(); | ||
| let mut signer = Self::default(); | ||
| for key_index in 0..gap_limit { | ||
| let leaf_path = DerivationPath::identity_authentication_path( | ||
| network, | ||
| KeyDerivationType::ECDSA, | ||
| identity_index, | ||
| key_index, | ||
| ); | ||
| let xpriv = root_xpriv.derive_priv(&secp, &leaf_path).map_err(|err| { | ||
| SimpleSignerError::DerivePriv { | ||
| index: key_index, | ||
| message: err.to_string(), | ||
| } | ||
| })?; | ||
| let secret: SecretKey = xpriv.private_key; | ||
| let pubkey: PublicKey = PublicKey::from_secret_key(&secp, &secret); | ||
| let pkh = ripemd160_sha256(&pubkey.serialize()); | ||
| signer | ||
| .address_private_keys | ||
| .insert(pkh, secret.secret_bytes()); | ||
| } | ||
| Ok(signer) | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: from_seed_for_identity is unused in this PR and has a misleading contract
from_seed_for_identity populates self.address_private_keys (keyed on pubkey-hash, used by Signer<PlatformAddress> at lines 379-385) but does not populate self.private_keys: BTreeMap<IdentityPublicKey, [u8; 32]>. Per the impl at lines 247-258, Signer<IdentityPublicKey>::sign reads from private_keys only, so the returned signer cannot satisfy that trait despite the function name. The doc-comment honestly admits callers must additionally call add_identity_public_key, but the e2e framework only uses from_seed_for_platform_address_account; nothing in the PR consumes from_seed_for_identity. Either drop it until a real consumer lands or rename to reflect that it populates the address-signing path (e.g. from_seed_for_identity_authentication_addresses) so future callers don't expect a turnkey Signer<IdentityPublicKey>.
source: ['claude']
🤖 Fix this 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/simple-signer/src/signer.rs`:
- [SUGGESTION] lines 197-241: from_seed_for_identity is unused in this PR and has a misleading contract
`from_seed_for_identity` populates `self.address_private_keys` (keyed on pubkey-hash, used by `Signer<PlatformAddress>` at lines 379-385) but does not populate `self.private_keys: BTreeMap<IdentityPublicKey, [u8; 32]>`. Per the impl at lines 247-258, `Signer<IdentityPublicKey>::sign` reads from `private_keys` only, so the returned signer cannot satisfy that trait despite the function name. The doc-comment honestly admits callers must additionally call `add_identity_public_key`, but the e2e framework only uses `from_seed_for_platform_address_account`; nothing in the PR consumes `from_seed_for_identity`. Either drop it until a real consumer lands or rename to reflect that it populates the address-signing path (e.g. `from_seed_for_identity_authentication_addresses`) so future callers don't expect a turnkey `Signer<IdentityPublicKey>`.
Comment on lines
+40
to
+41
| const DEFAULT_ACCOUNT_INDEX: u32 = 0; | ||
| const DEFAULT_KEY_CLASS: u32 = 0; |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: DEFAULT_ACCOUNT_INDEX/DEFAULT_KEY_CLASS in mod.rs duplicate wallet_factory's pinned spec without sharing the drift guard
wallet_factory.rs pins DEFAULT_PLATFORM_PAYMENT_ACCOUNT_SPEC from PlatformPaymentAccountSpec::default() and exports DEFAULT_ACCOUNT_INDEX_PUB / DEFAULT_KEY_CLASS_PUB with a drift test. mod.rs:40-41 declares its own DEFAULT_ACCOUNT_INDEX = 0; DEFAULT_KEY_CLASS = 0; and feeds them into make_platform_signer. If PlatformPaymentAccountSpec::default() ever drifts, TestWallet::create (uses WalletAccountCreationOptions::Default) would track the new value while make_platform_signer would still derive 0/0 keys — signer/wallet drift without firing the existing test. Re-export from wallet_factory so there's one source of truth.
source: ['claude']
Comment on lines
+152
to
+189
| fn atomic_write_json( | ||
| path: &Path, | ||
| state: &HashMap<WalletSeedHash, RegistryEntry>, | ||
| ) -> FrameworkResult<()> { | ||
| use std::io::Write; | ||
|
|
||
| let on_disk = encode_keys(state); | ||
| let bytes = serde_json::to_vec_pretty(&on_disk).map_err(|err| { | ||
| FrameworkError::Io(format!("serialising registry to {}: {err}", path.display())) | ||
| })?; | ||
| let parent = path.parent().ok_or_else(|| { | ||
| FrameworkError::Io(format!( | ||
| "registry path {} has no parent directory", | ||
| path.display() | ||
| )) | ||
| })?; | ||
| fs::create_dir_all(parent) | ||
| .map_err(|err| FrameworkError::Io(format!("creating {}: {err}", parent.display())))?; | ||
|
|
||
| // Same-filesystem temp file is required for atomic rename; | ||
| // `persist` (not `persist_noclobber`) overwrites cross-platform. | ||
| let mut tmp = tempfile::NamedTempFile::new_in(parent).map_err(|err| { | ||
| FrameworkError::Io(format!("creating temp file in {}: {err}", parent.display())) | ||
| })?; | ||
| tmp.write_all(&bytes).map_err(|err| { | ||
| FrameworkError::Io(format!("writing temp file {}: {err}", tmp.path().display())) | ||
| })?; | ||
| tmp.as_file_mut().flush().map_err(|err| { | ||
| FrameworkError::Io(format!( | ||
| "flushing temp file {}: {err}", | ||
| tmp.path().display() | ||
| )) | ||
| })?; | ||
| tmp.persist(path).map_err(|err| { | ||
| FrameworkError::Io(format!("persisting temp file -> {}: {err}", path.display())) | ||
| })?; | ||
| Ok(()) | ||
| } |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: Test wallet seeds persisted to default-permissioned JSON under shared TMPDIR
atomic_write_json writes the registry (containing 64-byte hex seeds for every fresh test wallet) under ${TMPDIR}/dash-platform-wallet-e2e/... with default umask permissions. On Linux/macOS that's typically /tmp and world-readable. On a multi-user runner or shared dev host, a co-located unprivileged user could read seeds between setup and teardown and drain the testnet credits. Impact is bounded (testnet credits, narrow window, operators self-select), but defense-in-depth is cheap: chmod the registry file to 0600 and the slot dir to 0700, or default the workdir base to ${HOME}/.cache/dash-platform-wallet-e2e.
source: ['claude']
5 tasks
…ash-spv 0.45 v4.1-dev authoritative for divergence. Resolves the 15 post-merge compile errors (14 drive-abci + 1 platform-wallet) plus the 3 broadcast.rs warnings. drive-abci (14 errors, E0599/E0609/E0560): c88264e77 reverts #808, removing the nested Core-23 platform-address API (platform_p2p_address/http_address accessors, legacy_* + addresses fields). The PR had adapted rs-drive-abci masternode code to that API; git auto-merged those PR-only edits without a conflict, so they carried into a c88264e77 tree. Reverted all 11 diverging rs-drive-abci files to v4.1-dev (now byte-identical) — the divergence was purely #808 adaptation, incl. #[cfg(test)] DMNState fixtures (invisible to `cargo check --workspace`, but broken under `cargo test`) and the PR's Core-23 validator test, which is inseparable from the reverted feature. platform-wallet spv/runtime.rs (E0308): dash-spv 0.45 migrated BlockHeaderStorage::store_headers to &[HashedBlockHeader]; convert the devnet genesis Header via .into() (BlockHeader is Copy; header reused after). platform-wallet shielded/operations.rs (E0061, shielded feature): v4.1-dev's extract_spends_and_anchor takes (sdk, store, notes); build_unshield_st passed only (store, notes). Add the in-scope sdk arg. broadcast.rs warnings: after taking v4.1-dev's broadcaster conflict side, the inline post-broadcast path is gone — drop its now-unused import (TransactionContext, WalletTransactionChecker) and the outer xpub binding (kept the in-block xpub used for change-address derivation). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-dev reconcile seam v4.1-dev replaced the PR's standalone build_transfer_persistence_entries (transfer's post-broadcast ledger-update builder) with the shared reconcile_address_infos seam. The merge dropped the production fn but kept the PR's test_utils wrapper + the found_024 e2e regression test that drove it, leaving a dangling reference. The V27-007 foreign-address behavior the pin guarded is now structurally enforced: foreign addresses never resolve through the provider bijection (nor the live-derived-pool fallback), so they are never reconciled into the local ledger. Remove the orphaned test_utils module, the found_024 test file, and its cases/mod.rs registration; annotate the Found-024 TEST_SPEC entries RETIRED with the reason. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The e2e integration target (feature-gated, so invisible to `cargo check --workspace`) had 16 compile errors against v4.1-dev's APIs. All mechanical adaptations, no test-logic change: - register_from_addresses / top_up_from_addresses / transfer_credits_to_addresses_with_external_signer now return `(Identity, AddressInfos)` / `(AddressInfos, u64)` (v4.1-dev's Found-005 fix — SDK-returned address balances are no longer discarded). Destructure at the call sites and keep the identity / balance; the harness discards the extra AddressInfos, matching its prior single-value behavior. Sites: bank_identity.rs, wallet_factory.rs, bank_rebalance.rs, id_002, id_005. - dashcore 0.45 dropped `Address::network()`; check the sink's network via `as_unchecked().is_valid_for_network(net)`. Site: cr_004. - shielded_fund_from_asset_lock gained a trailing `cl_wait: Option<Duration>` (9 args); pass `None`. Site: sh_018. `cargo test -p platform-wallet --features e2e --no-run` now compiles clean (0 errors); found_026_tests + both wallet.rs test modules build. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…omment Run rustfmt over the merge/adaptation edits (registration.rs COMBINE line wrap; cr_004 assert wrap). Update the cr_004 comment to describe the current check — Address<NetworkChecked> has no network getter, so validate via as_unchecked().is_valid_for_network — instead of the removed `==` note. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The bank fund planner sized its Core→Platform top-up (E5) from `bank_plan::snapshot_balances()`, which reads `core_balance_confirmed()` immediately after the bootstrap self-fund. That value is a lock-free atomic fed asynchronously by the SPV WalletEvent → BalanceUpdateHandler task; it lags the awaited self-fund (which resolves on a separate notify path) and can read a stale ~0, sizing E5 to nothing and failing the run with "Bank under-funded" even though the SPV gate already saw the full confirmed balance. Poll the atomic back to its pre-bootstrap baseline (minus the self-fund's max Core outlay) before the snapshot, reusing the existing `wait_for_bank_funded` gate — the same poll every other core-balance consumer already uses. Make `bootstrap_lock_duff` a `const fn` so the outlay bound reuses the single sizing rule. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The prior single-threshold poll cleared on the stale pre-spend Core balance (the async wallet_task had not yet drained the self-fund's WalletEvents), so `core_balance_confirmed()` dipped toward ~0 right after the poll returned and the fund-planner snapshot read the dip — E5 stayed sized to nothing. `bank.core_balance_confirmed()` (the WalletBalance atomic) is exactly what `wait_for_bank_funded` already polls and exactly what the planner reads — same signal, not the SPV scan view. The real defect is that atomic being NON-MONOTONIC while the wallet_task drains. Replace the threshold poll with an inline bounded loop over the same atomic that breaks only once the value is at/above `converge_min` AND unchanged since the previous read, and log every iteration (observed vs target, settled) so a rerun shows whether the atomic settles or oscillates forever. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
57 tasks
Switches the rust-dashcore pin across all 8 crates from dev head c88264e77 to PR #836 (fix/asset-lock-finality, 0f448598), which closes rust-dashcore#832: - asset-lock coin selection only spends confirmed / InstantSend-locked coins (opt-in require_final_inputs) - a self-send change output counts as confirmed only when every parent is final, matching Bitcoin Core IsTrusted semantics Fixes the #832 asset-lock finality timeouts (cr_*/id_* env reds). Note: independent of the e2e bank-funding shortfall, which is an operator funding requirement, not a code defect. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BqMDY7ZZNwSvdPgBrJQQNq
…i_thread flavor The deep SDK GroveDB proof/quorum verification recursion overflows libtest's ~2MB thread stack, and the prior current-thread stack_size fix deadlocked tk_001's concurrent work in a shared test group. Add an optional multi_thread flavor to the stack_size proc-macro that builds a multi-threaded tokio runtime with .thread_stack_size(EXPR) so worker threads share the big-stack budget, and re-annotate tk_001 to use it with 12 workers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…wallet-e2e # Conflicts: # Cargo.lock # Cargo.toml # packages/rs-platform-wallet/src/error.rs # packages/rs-platform-wallet/src/manager/mod.rs
`register_wallet` downgrades a wallet to external-signable (drops the in-process root key) as a security posture, so the cleanup sweep's resident-derive discovery walk failed with "External signable wallet has no private key" — identities went undiscovered and their credits leaked instead of returning to the bank, slowly draining the e2e bank. Derive the master xprv from the seed already in scope and probe via `load_identity_by_index_from_master`, as loading.rs documents. Phase-2 sweep already used the external-signer path; only discovery was wrong. Validated: id_001 in a workdir with orphaned wallets now discovers and drains identities (3 drained, 0 discovery failures), recovering ~8 tDASH of previously stranded funds. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BqMDY7ZZNwSvdPgBrJQQNq
Deep dashcore::sml quorum verification overflows the default 2 MiB libtest thread stack (observed: tk_001b_token_transfer_zero_rejected SIGABRT during a funded e2e run, aborting the whole test binary). Mirror the exact value CI already sets in tests-rs-wallet.yml / tests-rs-workspace.yml (4194304 = 4 MiB) so local `cargo test` matches CI instead of the 2 MiB default. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BqMDY7ZZNwSvdPgBrJQQNq
This was referenced Jul 6, 2026
Formalizes the post-#820 dash-spv gap-window rescan residual (filed upstream as rust-dashcore#846) as a tracked P2 e2e finding, distinct from the diagnostic-only §3 reproduction. Pinned crate-level in repro/pr3549-rdc; modeled by found_coinjoin_gap_limit_sync::sim_tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BqMDY7ZZNwSvdPgBrJQQNq
…on external-signable wallet Empirical hypothesis test for Found-031. The finding claimed IdentityTopUp asset-lock top-up is UNREACHABLE on a post-registration wallet because register_wallet's downgrade_to_external_signable() drops the root seed before the top-up account can be provisioned. Confirmed as a USAGE ERROR, not a defect. The id_002b helper provisioned the account with add_account(.., None), which re-derives from the (now-absent) resident key and fails with "External signable wallet has no private key". The whole asset-lock build+consume path is signer-driven (build_asset_lock_with_signer + external core_signer); the account is only used for the public next_address(account_xpub, false) derivation. So the account must be provisioned watch-only via Some(xpub), exactly as the production DashPay contact path already does. - Derive the IdentityTopUp account xpub from the wallet seed's master (master.derive_priv(account_path).neuter()) — byte-identical to the None branch's own derivation — and provision via add_account(.., Some(xpub)). - Correct Step 7: the top-up flow is remove-on-success (consume_asset_lock removes the tracked entry), so the speculative !top_up_locks.is_empty() assertion was wrong. Mirror CR-003's loose "any lingering lock must be final". Result: id_002b passes — identity balance credited 100M→100.09B credits by the asset-lock top-up. Throwaway experiment; do not merge as-is. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BqMDY7ZZNwSvdPgBrJQQNq
…ing + remove-on-success assertions
Apply the id_002b Found-031 fix to al_001's own copy of add_identity_topup_account:
provision each of the N IdentityTopUp accounts with a seed-derived Some(xpub)
instead of add_account(.., None), which fails on the post-registration
external-signable wallet ("External signable wallet has no private key"). Thread
seed + network into the helper (hoisted the existing declarations above the
provisioning loop and reused them).
Correct Step 5 for remove-on-success: top_up_identity_with_funding calls
consume_asset_lock on Platform accept, which removes the tracked entry
(tracking.rs). So after N successful concurrent top-ups the registry is drained —
the old assert_eq!(top_up_locks.len(), N) plus the tracked-lock-based
txid-distinctness and input-disjointness checks can never hold on the happy path.
Since top_up_identity_with_funding returns only the new balance (no txid), those
properties can't be re-derived post-success; they are enforced upstream by
OutpointReservations (a collision/double-spend fails a build → task Err) and thus
surface in Step 4. Reachability under concurrency is proven by every task
returning Ok (Step 4) + every identity balance increasing (Step 6). Kept the
loose "any lingering lock must be final" check (mirrors id_002b / CR-003) and
dropped the now-unused HashSet import.
Build-check: cargo test -p platform-wallet --features e2e --test e2e --no-run — OK.
Full green run deferred (shared bank depleted). Throwaway experiment; do not merge as-is.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BqMDY7ZZNwSvdPgBrJQQNq
…ndings Found-031 reframed from P1 defect to CONFIRMED USAGE ERROR: no production code provisions an IdentityTopUp account; the None-based test helper needed Some(xpub) instead (mirrors the DashPay contact path). ID-002b now carries the fix and passes GREEN (identity credited 100,000,000 -> 100,091,967,120 credits). AL-001's precondition on this branch remains unfixed and still fails as originally pinned -- documented as such, not overclaimed as green. Cross-links the in-progress key-wallet actionable-error hardening (fix/add-account-actionable-error). Adds filed-issue cross-references: Found-013 -> platform#4028, Found-032 -> platform#4029, Found-021/022 -> rust-dashcore#763/#764 (crate-level repros + posted comments), Found-034 already linked to rust-dashcore#846 (verified). Found-025: notes the draft-audit's "fixed by platform#3650" claim, hedged as not independently re-verified this session; pin left in place. Found-027 reframed as a test-scaffold limitation (InMemoryShieldedStore is test-only, zero production instantiations) rather than a product defect; not filed. Found-026 checked against the PA-008b enqueue-timing framing -- already correct, left unchanged. Adds two changelog entries for the v4.1-dev merge (rust-dashcore pinned to 647fa98, RUST_MIN_STACK=4194304 added to match CI) and the post-merge full rerun (155 passed / 26 failed / 6 ignored, clean completion, 0 discovery failures). Recomputes the priority-count line from scratch: it had drifted far out of sync with the live tables (SH-*, PA-3040, Harness-* rows accreted without a matching recompute) -- corrected from P0:10/P1:32/P2:65/108-total to the accurate P0:17/P1:53/P2:70/141-total (109 baseline Quick-index rows + 32 Found-bug-pin rows). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BqMDY7ZZNwSvdPgBrJQQNq
…13/032 Cross-reference the repro/pr3549-platform-a crate-level repro tests (found_013_recover_asset_lock_blocking_swallows_persist_error, found_032_empty_delta_must_not_regress_recent_block_watermark) alongside the already-added dashpay/platform issue links, matching the level of detail already given to the rust-dashcore-side Found-021/022 cross-refs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BqMDY7ZZNwSvdPgBrJQQNq
…ged Some(xpub) fix al_001's Some(xpub) provisioning fix is now on this branch (b0b658a); update the AL-001 matrix row + detail from "fix drafted-but-unmerged" to "applied, build-clean, on-chain green run deferred (bank depleted)", with the indirect concurrency-invariant enforcement note. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BqMDY7ZZNwSvdPgBrJQQNq
…master xprv The bank wallet is external-signable (keyless) after registration, so every bank-identity load via the resident path `load_identity_by_index(bank_identity.identity_index)` failed with "External signable wallet has no private key", blocking the bank's Core->Platform self-heal at harness setup (transfer-key provision / drain / core-refill all skipped). Add a `bank_master` helper (`ExtendedPrivKey::new_master(bank.network(), bank.seed_bytes())`) and route all four load sites through `load_identity_by_index_from_master(.., &master)` — the same seed-derived master path `cleanup::sweep_identities_with_seed` and the id_002b/al_001 fixes use: - provision_transfer_key_if_missing (was line 248) - drain_bank_identity_to_addresses (was line 322) - refill_core_from_platform_if_below_threshold (was line 462) - top_up_identity_from_platform / E3 (was line 706) Best-effort sites (first three) classify a master-derive failure as a skip in their existing style; E3 propagates it as FrameworkError::Bank. Build-check: cargo test -p platform-wallet --features e2e --test e2e --no-run — OK. Live verification needs a rebalance-triggering setup run (bank-dependent; deferred while the shared bank is depleted). Throwaway experiment; do not merge as-is. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BqMDY7ZZNwSvdPgBrJQQNq
… master xpriv register_wallet unconditionally calls downgrade_to_external_signable(), stripping the resident root/private key so the wallet is keyless before the "best-effort identity discovery" step runs. That step used identity().sync(), which derives via the resident private-key path — so on the now-keyless wallet it ALWAYS failed with "External signable wallet has no private key" and logged a WARN. The auto-hydrate-every-identity feature was therefore dead for every registered wallet, not merely noisy. Capture the BIP-32 master xpriv from the wallet BEFORE the downgrade (WalletType::Mnemonic/Seed/ExtendedPrivKey → to_extended_priv_key(network); ExternalSignable/WatchOnly → None). When present, discovery now runs via discover_from_master(..) — the master-derivation path that reproduces the resident key hashes. A wallet that arrives already keyless (watch-only restore) has nothing to derive, so discovery is skipped at debug level with a pointer to discover_from_master(..), rather than emitting a WARN. Best-effort semantics are unchanged: discovery failure is logged, never blocks or rolls back registration. sync()/discover()/discover_from_master() signatures are untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… supply burn tk_009's three post-destroy reads (peer balance, total supply, frozen balance) were bare one-shot fetches. The SDK round-robins across DAPI replicas, so a read landing on a replica that hasn't applied the destroy transition yet returns stale pre-destroy state and reds an otherwise-green run. - Add wait_for_token_supply (framework/tokens.rs), the supply-side companion to wait_for_token_balance. Gates on exact equality, not >=: a burn DROPS total supply, so the stale pre-burn value a lagging replica serves is larger than the target and a >= gate would clear on it. - Replace the three tk_009 reads with consecutive-success polling on CHAIN_CONFIRMED_CONSECUTIVE_SUCCESSES: wait_for_token_supply for the supply decrement, and inline wait_for_token_predicate == 0 gates for the balance / frozen zero-reads (the tk_011 idiom for exact-match reads). - drive-abci: test_token_destroy_frozen_funds_success asserted the destroyed balance == 0 but never the supply side. Add a pre/post total-supply snapshot asserting supply drops by exactly the burned 5000, closing the deterministic single-node coverage gap for the same invariant. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The `owner_credits_post` fetch feeding the `post < pre` fee assertion was the one bare one-shot post-destroy read left after the prior tk_009 propagation fix. The fee debit settles on whichever DAPI replica served the destroy broadcast; a bare fetch can round-robin onto a sibling still serving the pre-debit balance and fire `post < pre` on a stale `== pre` value. Convert it to a consecutive-success poll (wait_for_token_predicate on CHAIN_CONFIRMED_CONSECUTIVE_SUCCESSES) gating on `credits < owner_credits_pre` — a stale `== pre` read fails the gate and keeps waiting, and a timeout still reds if credits never drop. The trailing `post < pre` fee assertion is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…A-001/101/102) Three test-robustness fixes; no product code touched. QA-001 — captured_master_reproduces_resident_key_hashes (register_wallet test): the assertion compared two master-path derives (captured vs canonical), which is tautological — same pure fn on inputs already proven equal, so it could never catch a master-vs-resident divergence the name claims to guard. Now compares the captured master's MASTER-key hash against the RESIDENT-wallet derive (derive_identity_auth_key_hash) across networks/indices, mirroring the resident-vs-master equivalence in identity::network::loading. Widens that helper's re-export from pub(super) to pub(crate) (matching the fn's own visibility) so the manager-side test can reach it. QA-101 — drive-abci test_token_destroy_frozen_funds_success: the supply assert compared two Options, passing VACUOUSLY if both were None (wrong id / dropped supply tree / fixture drift) — masking the exact catastrophe it guards. Unwrap both fetches to concrete u64 first, then assert_eq!(post, pre - 5000). QA-102 — tk_009 supply_pre_destroy: a bare one-shot token_supply_of set the post-destroy target, so a stale pre-read (replica lagging the mint) made the target unreachable and false-red the downstream poll. Base supply is DEFAULT_BASE_SUPPLY (0) and the pre-freeze transfer doesn't change supply, so the post-mint total is deterministically MINT_TO_OWNER; streak-confirm it via wait_for_token_supply(MINT_TO_OWNER). Still reds on timeout if it never settles. Drops the now-unused token_supply_of import. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…very fix + tk_009 de-flake Add a 2026-07-06 changelog entry covering two same-day fixes: register_wallet's built-in identity discovery (3e175d2, previously dead for every wallet due to deriving via the post-downgrade resident-key path) and tk_009's destroy-frozen-funds flake (980b54c, c83fb0d, 64a1b0c — root-caused to a stale round-robin DAPI replica read, not a supply-accounting bug). Retroactively annotate the prior "open question" changelog entry as resolved (existing doc convention). Update the TK-009 detail section with the root cause and fix. Add an explicit note next to the Found-bug pins accounting explaining why the discovery fix does not get its own Found-NNN pin (distinct from, but adjacent to, the retired Found-031). No priority/status counts changed — independently recomputed against the live tables (109 Quick-index + 32 Found-bug rows, matching the printed totals). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ti-thread runtime (#3535) The default single-arg `#[stack_size(EXPR)]` drove async bodies on a tokio current-thread runtime. That runtime has one thread, so a nested sync-over-async bridge in the body — another `block_on`, or `tokio::task::block_in_place` — deadlocks (the single thread is parked in the outer `block_on`) or, for `block_in_place`, panics ("can call blocking only when running on the multi-threaded runtime"). Default async bodies to a multi-threaded runtime with two worker threads, each sized to the requested stack via `thread_stack_size`, so the deep-stack budget extends to spawned tasks and nested bridges no longer wedge. The outer big-stack `std::thread` still guards the root future's synchronous recursion; the sync branch is unchanged. Two workers is the minimum that lets `block_in_place` hand off without spawning tokio's unbounded num_cpus default under `cargo test` concurrency. The opt-in `multi_thread, worker_threads = N` knob is retained and still overrides the default of two (e.g. tk_001's 12), so its call site is unchanged; the `multi_thread` keyword is now redundant-but-accepted for async. Adds a `tests/` regression putting a single-arg `#[stack_size]` on an async body doing `block_in_place`: RED (panic) with the old current-thread default, GREEN after the flip. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… on #[stack_size] Async `#[stack_size]` bodies always run on a multi-thread runtime, so the `worker_threads = N` override is meaningful on its own. Drop the parser guard that required `multi_thread` before `worker_threads`; the `multi_thread` token stays accepted as an optional legacy no-op so existing call sites (e.g. platform-wallet tk_001's `multi_thread, worker_threads = 12`) parse unchanged. Codegen is untouched — `worker_threads` was already `unwrap_or(2)`. Unify the async-only validation: since both `worker_threads` and `multi_thread` configure the async runtime (a sync body builds none), using either on a sync `#[stack_size]` function is now one clear compile error instead of a silent no-op. This replaces the former `multi_thread`-only guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
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.
Dependency status — ✅ resolved (previously blocked on rust-dashcore #808)
No longer merge-blocked on the temporary #808 pin. Merging
origin/v4.1-devbrought the Core-23 DIP-0018 platform-address API upstream (#4021 / #4024 / #4019 —platform_p2p_address()/platform_http_address()accessors +legacy_*fallbacks), so drive-abci compiles cleanly against the shared base. rust-dashcore is now pinned to647fa98=devheadc88264e77(DIP-0018 + account-level reservation API) + #836417d61da(asset-lock finality fix). Base branch:v4.1-dev.Recent updates (2026-07-07 — #3535 stack_size fix + worker_threads feature + Found-035)
stack_sizemacro nested-runtime deadlock FIXED. Async#[stack_size]tests defaulted to a tokio current-thread runtime, which deadlocks (or panics onblock_in_place) when the test body performs a nested sync-over-async bridge. The default async runtime now builds a multi-thread runtime (2 workers, each stack-sized viathread_stack_size), so nested bridges no longer wedge; the outer big-stackstd::threadstill guards the root future. The opt-inworker_threads = Nknob is retained and tk_001 stays byte-identical (keeps 12 workers). Repro test RED→GREEN. All 29 drive-abci#[stack_size]sites flip current-thread→multi-thread — verified safe by inspection (zero task-spawning /block_in_place/ thread-local constructs;block_onruns the root future on the calling thread regardless of flavor; drive-abci production usesblock_in_placezero times). (42029c3c53)stack_sizeargument grammar made coherent.#[stack_size(EXPR, worker_threads = N)]now works standalone (was a compile error withoutmulti_thread);multi_threadis an accepted optional legacy token; a unified guard rejects either arg on a sync fn (no silent no-op). New tests assert 2/3/N worker counts. (64612455ef)register_walletdiscovery fix (a genuine production defect, distinct from the retired usage-error Found-031) now has a permanent P2 / FIXED pin in TEST_SPEC with an explicit non-conflation cross-reference. (22f0e799a0)Recent updates (2026-07-06 — register_wallet discovery fix + tk_009 de-flake)
register_walletunconditionally downgrades the wallet to external-signable, so its best-effortidentity().sync()used the resident-key path and could never derive — the recovery-hydrate feature was dead for every wallet and each registration emitted a misleadingExternal signable wallet has no private keyWARN. Now captures the master xpriv before the downgrade and routes through the existingdiscover_from_master(...)(byte-identical derivation — verified the master-derived identity key-hashes match the resident derive), with adebug-level skip for genuinely keyless watch-only wallets. Master is captured pre-downgrade, borrowed (not cloned) into discovery, and zeroizes on drop. (3e175d2c31)total supply must decreasered was a stale round-robin DAPI replica, not a supply-accounting bug:destroy_frozen_fundsatomically decrements total supply (verified in rs-drivetoken_burn_operations_v0). All four post-destroy reads plus the pre-destroy snapshot now use propagation-tolerant consecutive-success polling with exact-match-or-red-on-timeout gates (newwait_for_token_supplyhelper; exact==gate, since a burn lowers supply and a>=gate would clear on the stale higher value). (980b54c9e9,c83fb0d64d)test_token_destroy_frozen_funds_successnow asserts the exact total-supply decrement (was balance-only) — a deterministic single-node guard for the "destroy burns supply" invariant (runs green). (980b54c9e9)None==Nonesupply assert, an unpolled pre-snapshot) all folded in. (64a1b0cf11)Recent updates (v4.1-dev merge + Found-031 correction + findings)
origin/v4.1-dev(merge0b1f1b0590) — resolves the fix(ci): fix release workflow syntax error #808 saga;cargo check --workspaceand the e2e test build are both green.register_wallet'sdowngrade_to_external_signable()leaves the in-memory wallet keyless, so provisioning an IdentityTopUp account viaadd_account(_, None)fails "External signable wallet has no private key". Fix: provision via the master-derived pathadd_account(_, Some(xpub))(seed-derived master xpub, mirrorscontacts.rs) — applied toid_002b(fda0478f05) andal_001(b0b658a436); the cleanup identity-sweep already usesload_identity_by_index_from_master(c99e6e4404). Proven on-chain (id_002b top-up: identity 100M → 100.09B credits). The former P1 "Found-031 production bug" TEST_SPEC entry is reframed to retired/usage-error. Upstream ergonomic follow-up on rust-dashcore branchfix/add-account-actionable-error(typed actionable error on keylessadd_account(_, None)).repro/pr3549-platform-a,repro/pr3549-rdc. (found-025 reported fixed by fix(sdk): address-sync no longer silently discards balance changes for post-snapshot addresses (Found-025) #3650; found-026 / found-027 verified not real, not filed.)RUST_MIN_STACKin.cargo/config.toml(259ae1d105) — mirrors CI's 4 MiB so deepdashcore::smlquorum-verification recursion no longer overflows the default 2 MiB libtest thread stack. (tk_001 keeps its own 16 MiB#[stack_size]for the deeper transfer-proof path.)Why this PR exists
The rs-platform-wallet crate lacked an end-to-end test framework and suite capable of running against a live Dash Platform testnet. This PR delivers that framework — including bank-wallet funding mechanics, orphan-wallet cleanup, a multi-phase fund planner, proof-verified balance checks, and a full corpus of Found-/PA-/AL-/CR-/TK-* regression guards — plus the correctness fixes surfaced by running it against paloma (the shared devnet).
What was done
Foundation & Stage-2 merge
#3549 ← #3554) — delivers feat: identity registration with asset-lock proofs #3634/Found-008 + the v3.1-dev advance onto the e2e branch.PlatformWalletError::PersistedAfterOnChainSuccessenforced at the 5 post-on-chain-success persistence sites — roll back in-memory state + propagate a typed, non-conflatable error instead of log-and-continue.LockNotifyHandler::notify_waiters()drops lock events arriving inwait_for_proof's check/await gap (concurrent asset-lock builds stall on FinalityTimeout) #3641 — confirmed NOT regressed by Stage-2, corroborated four independent ways.found_008retired (F-A) — was a misconceived pin; AL-001 is the genuine Found-008 guard.GetDocumentsV0/V1 versioned encoder + transport wiring +Fetch::Querytrait refactor (three landed pieces, backported to v3.1-dev as fix(rs-sdk,drive-abci): SDK emits incompatible getDocuments wire against pre-v3.1 networks #3699):SdkBuilder::with_initial_versionadditive helper.sdk.version()throughexecute_transportso live wallets against v3.0 testnet emit the correct wire format.Fetch::Query(rich) vsFetch::Request(wire) associated types; removes ad-hocAny-downcast.Harness robustness (validated on paloma)
Orphan-sweep idempotency —
sweep_orphanshandlesWalletAlreadyExistsgracefully: when a wallet was re-registered by SPV persistent state across a process restart, the sweep retrieves the existing handle viaget_wallet()instead of failing.Funding minimums raised (config.rs):
EXPECTED_TOKEN_SUITE_FLOOR: 50B → 88.8B credits — reflects observed per-suite consumption; prevents mid-run TK-suite exhaustion.DEFAULT_MIN_BANK_CREDITS: 500M → 550B credits (5.5 DASH) — calibrated from measured gross demand (~4.75 DASH) via the new FundingLedger; guarantees the bank always meets the 88.8B floor so TK token tests run instead of silently skipping assertions.Stale-balance phantom-reject + spend-cache injection (bank.rs, bank_plan.rs, harness.rs):
sync_balances(None)at startup can land on a lagging DAPI replica and return 0 credits for the bank's Platform address even when the real balance is ~225B. Two complementary fixes are in place for the net final state:AddressInfo::fetchbalance is only adopted as the authoritative floor if a second independent fetch confirms it. This avoids promoting a single stale-replica reading to authoritative.address_balancesis also seeded with that value sofund_addresscan actually spend it. Without this, the gate-path saw the adopted balance but the spend path still read a stale 0 (gate-vs-spend split-brain).Together these ensure the bank reliably spends its real on-chain balance end-to-end.
rs-dapi-client rate-limit stop-gap reverted — the 5 s-cooldown patch was reverted out of this PR (
569877147b);rs-dapi-clienton this branch is byte-identical to the v3.1-dev baseline. The proper fix (classifyResourceExhaustedas retryable → rotate to a different node instead of banning) ships separately as PR fix(sdk): ban rate-limited node for Envoy-advertised reset window #3951.FundingLedger metrics
New per-type funding accounting printed at teardown via
SetupGuard::Drop:PLATFORM_WALLET_E2E_FUNDING_REPORT=1env flag.Client-defect pins: Found-031 / Found-032 / Found-033
Three new documented client-defect pin groups recorded in TEST_SPEC.md (red-by-design):
sync_balancesincremental-delta gap — doesn't refresh chain-confirmed balances or advance the watermark on an empty delta (same root cause the spend-cache injection compensates for)Test-quality fixes
denominationinstead offunding_credits; corrected.Merge
origin/v3.1-dev merged in; build-clean; no Cargo manifest drift.
Testing
cargo check -p platform-wallet -p simple-signer→ clean.cargo test -p platform-wallet --no-run→ clean (all e2e bins link).cargo test -p dash-sdk --features mocks,offline-testing --lib→ 133 passed.cargo test -p dash-sdk --features mocks,offline-testing --tests→ 127 passed (incl. V0/V1 wire-shape + dispatch_by_sdk_pv).cargo test -p drive-abci --lib query→ 585 passed.cargo test -p platform-version→ 5 passed.cargo test -p rs-dapi-client→ 116 passed.Full paloma e2e run (raised per-IP rate limit): 170 pass / 11 fail
The 11 failures break down as:
found_021,found_022,sh_006Funding held end-to-end; ban-cascade = 0.
Breaking changes
None. All changes are additive (new trait methods with default impls, new struct fields with defaults, new public constants).
Checklist
cargo checkclean)-D warnings)cargo fmtapplied🤖 Co-authored by Claudius the Magnificent AI Agent