Repository navigation
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 848 commits into
Draft
Claudius-Maginificent wants to merge 848 commits into
Claudius-Maginificent wants to merge 848 commits into
Conversation
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueWalkthrough
Pre-merge checks:
 |
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Description Check | |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title identifies the platform-wallet E2E framework, which is a main change in the pull request. It is overly long and includes several secondary terms, but it remains related and understandable. |
✨ Finishing Touches
-
📝 Generate docstrings
-
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
-
- Create PR with unit tests
- Commit unit tests in branch
feat/rs-platform-wallet-e2e
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
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.
4 of 6 tasks
7 of 8 tasks
lklimek
changed the base branch from
v3.1-dev
to
fix/rs-platform-wallet-auto-select-inputs
April 28, 2026 07:17
6 of 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 not started yet because this PR is a draft.
Commit f8c380c. Normal review starts when eligible; priority review starts as soon as a slot is available. |
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']
4 of 5 tasks
…leness and retry checks Read address infos with zero height tolerance through the existing per-method freshness criteria, so a node one block behind fails as stale and the regular retry/ban path moves on. Treat an address-nonce rejection whose provided nonce is ahead of the node's expected nonce as retryable through the existing broadcast retry hook, resending the same signed bytes; behind/equal nonces stay terminal. Co-Authored-By: Codex <noreply@openai.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019H9THphMnuQyh9oPY13SoE
…ed transfers in e2e Add SH-037..SH-042: identity -> own pool, identity -> foreign wallet pool, platform address -> foreign wallet pool, pool -> existing identity top-up, cross-wallet shielded transfer, and Core asset lock -> foreign wallet pool. Each case asserts exact receiver credit and sender debit within fee bounds, funded and swept through the existing framework. Co-Authored-By: Codex <noreply@openai.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019H9THphMnuQyh9oPY13SoE
…igration rows Pin the complete historical decoder in a storage-owned crate, including leaf serde and input-key encoding. Add immutable pending-context fixtures, type-safe materialization gates, and logged full rescans for invalid birth heights under Recovery while preserving Strict failures. Document the unreleased V020 recreation requirement and extend rewind boundary coverage. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Codex GPT-6 <noreply@openai.com>
…e drift Delete rejected transaction bodies by persisted keys while retaining evidence belonging to surviving logical transactions. Restore protected claims cleared by the delete trigger and document the physical-key map. Add a regression that fails when survivor outputs or claims are removed. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Codex GPT-6 <noreply@openai.com>
… syncs A shared sync from a concurrently running case scans every bound subwallet and could decrypt SH-004's note before its zero-before-sync assertion. Give SH-004 a private file-backed coordinator/store so the precondition holds under parallel execution. Co-Authored-By: Codex <noreply@openai.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019H9THphMnuQyh9oPY13SoE
…le fixtures Remove the pinned V019 codec crate and use the live record decoder and input-index writer. Preserve pending and mined fixture bytes, strengthen live decoding and migration checks, and correct the 5.0 writer provenance. Recovery rewinds invalid birth heights without overwriting corrupt wallet metadata, preserving loader diagnostics. Keep temporary V020 compatibility notes in PR material rather than the schema reference. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Codex GPT-6 <noreply@openai.com>
Cascades #5150's review fixes (f72313e: V020 migration body, frozen V019/V020 codecs, core_spend_authorization recovery). Conflict resolution: - SCHEMA.md: #5150's revised V020 row, this branch's V021 row kept. - sqlite_schema_pinning.rs: EXPECTED_SQL_FINGERPRINT recomputed for the combined migration set (#5150's revised V020 body + V021): a39aaef2c424545841cc4ee8062c4bde36ab80f9f439123545babc322e8259a9. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d lock history SH-002 polls (bounded) for the shielded change note before asserting it, since it can land a few seconds after the first sync. SH-042 reads the full tracked-lock history, because the unused-lock view excludes consumed locks by design. The harness no longer seeds fresh shielded accounts' watermarks from the local tree size, which can be ahead of what the account scanned. Co-Authored-By: Codex <noreply@openai.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019H9THphMnuQyh9oPY13SoE
The SDK's freshness checks ban nodes one block behind; with the production 60 s base ban (growing exponentially) a concurrent case run on the small testnet node pool loses nodes for minutes. Rebuild the harness SDK on an address list with a 5 s base ban period. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019H9THphMnuQyh9oPY13SoE
…ncurrent shielded syncs Two syncs over the same shielded store snapshot the tree size under a read lock and append under a later write lock, so both append the same commitments (8 leaves instead of 4 cold, 6 instead of 4 resumed). Observed on testnet as witness roots matching no Platform-recorded anchor. Co-Authored-By: Codex <noreply@openai.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019H9THphMnuQyh9oPY13SoE
…hotting sync gates Hold the write guard from the tree-size/watermark snapshot through the streamed append, so a concurrent sync cannot append between them and make this pass re-append commitments already in the tree. The stream already ran under this guard; SDK fetches never touch the store. Co-Authored-By: Codex <noreply@openai.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019H9THphMnuQyh9oPY13SoE
6 of 13 tasks
…ify rebuild RED: a duplicated tree returns Ok instead of rejecting the sync (3 passed, 1 failed). Typed-error and rebuild contracts additionally fail compilation until their APIs exist. Fixtures are offline and deterministic. Co-Authored-By: OpenAI Codex <noreply@openai.com> Co-Authored-By: Codex <noreply@openai.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019H9THphMnuQyh9oPY13SoE
… and add explicit rebuild Require a response at least as fresh as the tree's recorded height bound before diagnosing excess leaves. Persist the bound before corresponding appends in the same SQLite database; skip stale or unknown provenance and never infer a height from another response's speculative count. Expose explicit tree-only rebuild while retaining notes, nullifiers and spent status. Use indexed nullifier lookups during receipt reconciliation. Keep spend-time classification deferred and add no FFI or automatic repair. BREAKING CHANGE: exhaustive matches on PlatformWalletError must handle ShieldedTreeInconsistent. Existing method signatures are unchanged. Validation: 224 offline shielded tests; one-block-behind regression fails without freshness comparison and passes with it. Clippy all-targets / all-features with --no-deps -D warnings, cargo fmt --all, git diff --check. Co-Authored-By: OpenAI Codex <noreply@openai.com> Co-Authored-By: Codex <noreply@openai.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019H9THphMnuQyh9oPY13SoE
…mitment tree The harness sync helper discarded the coordinator's per-wallet outcomes, so a corrupted persisted tree surfaced only as unrelated timeouts and anchor errors. Fail loudly on ShieldedTreeInconsistent and point at the tree file to keep as evidence; the harness never rebuilds it, because the state indicates a wallet bug. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019H9THphMnuQyh9oPY13SoE
…rving notes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019H9THphMnuQyh9oPY13SoE
…ving notes Transfer, unshield, withdraw and identity top-up shared no zero-amount guard, so a zero request waited on the store lock held by a concurrent sync and then failed with an unrelated note-selection error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019H9THphMnuQyh9oPY13SoE
The shielded e2e suite had several ways to turn a product failure into a PASS. This makes each SH case assert the property it claims, or say plainly what it does not cover. Harness - ShieldedHandle::sync uses the typed try_sync result: tree inconsistency is matched by variant and panics, and any other sync error fails. Polling waits retry recoverable errors and report the last one on timeout. - assert_adv_rejected requires a typed consensus error with an attack-specific code. Transport errors, SDK proof/readback errors and admitted-but-unobserved timeouts now fail instead of passing. - Added NoProofProver (panics if proving is reached) and wait_for_unspent_notes. Removed the no-op TamperingProver. Cases - SH-009: funds a real note and asserts the exact zero-amount error for shield, transfer, unshield and withdraw. No proof may be built, no note may be reserved, and there is no wall-clock ceiling. Expected RED until the zero-amount guard PR is merged in. - SH-010: both concurrent spends must succeed and both originals must be spent (disjoint selection). The balance is read only after both change notes are scanned. - SH-020: the loser must be rejected with 40901. A timeout counts only on chain evidence of no effect. The residual is checked once the change note is scanned. - SH-023: real fee-floor probe. Overwrites unshielding_amount with floor - 1 and expects 40904, because the min-fee gate runs before proof verification. - SH-027: notes are funded so the deserialize and cmx paths are actually reached. SH-030: the recipient is a checksum-valid mainnet address. SH-021: replays after a real coordinator restart from persisted state. SH-033: build failures now fail. SH-035: checks the note set and sweeps afterwards. SH-036: the skip floor matches this case's own budget, and the replay goes through the build-against-notes seam. - SH-007/011/019: assert preconditions and change-scan results instead of discarding them. SH-019 asserts the second withdrawal and keeps the L1 payout TODO. - Codes follow Drive's CheckTx order (structure, min fee, state, proof). TEST_SPEC status rows and sections now match the code. Fee bounds stay fee-tolerant by design. No exact-fee tightening. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019H9THphMnuQyh9oPY13SoE
…allet-e2e Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019H9THphMnuQyh9oPY13SoE
… fees The SH-036 skip gate and funding amount used a fixed 1B headroom. They now use the real requirement: the denomination, plus the client's versioned shield fee reserve (shield_fee_reserve_credits), plus the predicted 2-action shield fee. The E2E-SKIP warning reports required vs available with the breakdown. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019H9THphMnuQyh9oPY13SoE
…keys query tests The two QuerySettings literals added in #5491 predate the verified_protocol_version field, so `cargo test -p dash-sdk --lib` no longer compiled. Set it to 0 like the crate's other test literals. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019H9THphMnuQyh9oPY13SoE
…f has no boundary Extract the Phase 2 decision of incremental_catch_up into the pure needs_compacted_phase and test it with real, decodable recent proofs. Mock expectations return an empty proof, whose decode error takes the "run compacted" arm and hides the bug. Reproduces testnet SH-040: the trunk checkpoint predates a funding in a since-compacted block, and the RangeFrom recent proof holds a later block but no boundary, so the compacted phase is skipped and the credit is lost. The no-boundary test fails until the fix lands. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019H9THphMnuQyh9oPY13SoE
…no boundary Phase 2 of incremental catch-up read "no boundary in the recent proof" as "recent tree empty, nothing to compact" and skipped the compacted query. A RangeFrom proof (every first sync of a fresh wallet) does not carry its start key as a boundary, and a RangeAfter start key that was compacted away leaves none either. Either way, a credit that landed after the checkpoint in a since-compacted block was lost until the next full rescan (about 7 days by default). Seen on testnet as a 2.22e9-credit funding the wallet reported as 0. Run the compacted phase whenever the proof carries no boundary. The per-operation height pin in apply_op keeps compacted operations at or below the checkpoint, and recent blocks already inside a compacted range, from being counted twice. Cost: one extra compacted RPC in that case. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019H9THphMnuQyh9oPY13SoE
…allet-e2e Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019H9THphMnuQyh9oPY13SoE
This branch has not been deployed
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 — rust-dashcore
aca0202(same as base)The rust-dashcore workspace deps use the base
v5.1-devpinaca020263990a86a15cc1d97ae2cbf965d488db5(dev, includes rust-dashcore#1082 and #1106); the former side-branch pin18f7f3eis gone. This PR still carries #5150 (fix/pr-5126) by merge and follows its fate.Recent updates (2026-10-23 — v5.1-dev + #5150 re-merge, review follow-ups)
v5.1-dev(4545b88cb5) and fix(platform-wallet)!: restore Core spending state and repair persisted accounting #5150 (fix/pr-5126atb1d19ac07d). Storage, changeset, migrations and Swift persistence are identical to fix(platform-wallet)!: restore Core spending state and repair persisted accounting #5150. The Core accounting migration is now V020 (renumbered from V021 to keep versions contiguous; see test(platform-wallet-storage): require unique, contiguous migration versions #5553). The PR'sSpvRuntimestart/stop additions are dropped in favour of base's lifecycle (start_and_spawn,stop_with+ShutdownIncompleteretry inframework/spv.rs). BasePlatformWalletManager::newis kept;with_handlersis used only by the harness. The secp256k1 0.33 API changes from chore(platform)!: update rust-dashcore (incl secp256k1 0.33) #5307/refactor!: migrate Platform cryptography to rust-dashcore dev (aca02026) #5514 are applied to the harness and signer.sdk.version()) instead ofPlatformVersion::latest(); offline tests keeplatest().shield_from_asset_lockbroadcast API (operations::shield_from_asset_lock,PlatformWallet::shielded_shield_from_asset_lock).build_shield_from_asset_lock_st/build_unshield_staretest-utilsonly; SH-035 broadcasts through the SDK.accessors.rsandplatform_wallet.rsmatch base.Persistencerefusal fromshielded_wallet_drain_operation_status. The drain stays out of e2e reach until SQLite attestsWALLET_RESTORE(feat(wallet-storage)!: restore complete Core wallet snapshots from SQLite #5210) andASSET_LOCK_OPERATION_IDS(fix(platform-wallet): retain drain operation history through funding conflicts #5528) and the harnessHONOUREDmask follows; recorded as a gap in TEST_SPEC.RUST_MIN_STACKhas one source (.cargo/config.toml, forced); the workflow copies are removed.dash-platform-macrosandsimple-signerare added to the nextest package list, with newsimple-signerderive unit tests.#[stack_size]grammar isworker_threads = Nonly (the never-shippedmulti_threadflag is gone);SimpleSignerErrorcarries typed#[source]errors; duplicateciboriumdev-dep removed; Found-021 docs narrowed after fix(platform-wallet): store the InstantSend lock of a send on its record #5393;derive_identity_auth_key_hashback topub(super).None of these changes edit a shipped
vN.Recent updates (2026-10-01 — v5.1-dev + #5150 merge, asset-lock accounting checks, harness fixes)
v5.1-devand fix(platform-wallet)!: restore Core spending state and repair persisted accounting #5150 (fix/pr-5126atbe7f8ecd4c). On conflicts, upstream won for the identity network error handling (PersistedAfterOnChainSuccessremoved); the e2e-only pieces stayed: theshielded_shield_from_asset_lockwrapper, thespv/runtime.rsstart/stop split andPlatformWalletManager::newtaking a handler list.keep-finalized-transactionsmoved under thee2efeature.Internalwithnet_amount == -(lock + fee), live and after reloading the SQLite copy. CR-004 checks a plain send isOutgoingwith-(amount + fee).direction must be Internal …, got Outgoing) and passes after it. CR-003/CR-005 pass on both trees because their locks keep change.framework/harness_persister.rs) replacesNoPlatformPersistence: it mirrors every changeset into a private SQLite store (0700 directory) and attests only what that store does —ATOMIC_CHANGESETSandSHIELDED_VIEWING_KEYS— sobind_shieldedworks and SH-018/SH-021 pass again. A store failure is returned, not swallowed. Theplatform-wallet-storagedev-dependency enablesshielded.71117b3472and its parent merge): CR-003, CR-005, CR-006, SH-018, SH-021 pass. No double-spend, conflict or missing-inputs rejection appeared in any run.Recent updates (2026-07-27 — PA-005c stale-doc correction)
d692f2de3f). While checking whether rust-dashcore#818's reservation lifecycle had actually been exercised by the last e2e run, audited the verification ledger directly rather than trusting the doc: a full-suite run on 2026-07-25 14:09–14:33 UTC (189 tests, 148 passed / 41 failed, zerotrailing bytesversion-skew errors — that blocker had already cleared) showspa_005c_receive_address_reservation_lifecycle ... ok. No commit had updatedTEST_SPEC.mdto reflect it, so it still read "compile-only / not yet live-verified." Fixed the status and recorded the run's evidence (ledger key, log path, counts).b309d640aa's message has no matching ledger record. That commit's own Codex session was briefed to run only the isolatedtk_001btest ("do NOT run the full suite... a full validation run will happen separately afterward"), and its tool output shows exactly that one test executing. The number doesn't match the real 2026-07-25 148/41 run either. The stack-size fix itself remains independently verified via the isolatedtk_001bpass — only the specific 145/44 full-suite figure is unbacked.Recent updates (2026-07-25 — protocol-version auto-detect fix + stack-size right-sizing)
Fixed a client/testnet proof-format version skew, root-caused via the SDK's own version-status query. After merging
origin/v4.1-dev(4df718077090, pulls in rs-drive#4224 "version the compacted address-balance proof wire format"), the e2e harness's ownE2eContext::init()started failing 4-for-4 across three live testnet nodes withcannot decode GroveDBProof: UnexpectedVariant{...}— reproducing even in release mode with default stack, ruling out stack size. Queried all three nodes directly viaFetchUnproved/EvoNodeStatus(bypasses proof verification): all three report identical chain state (block 467950, matching app-hash) and drive protocolcurrent=latest=next_epoch=13, ruling out a partial-rollout or version-mismatch explanation at the network level. Root cause:#4224retroactively defaulted every protocol version ≤12 (including the harness's deliberately-lowPV_10seed) to a new simplified proof format, while only v13 kept the double-proof envelope the live network actually sends — and the harness pinned its SDK viawith_version(), which disables auto-detection entirely (despite comments claiming otherwise) and leftSdk::refresh_protocol_version()unreachable. Fixed (041c49ea67) by switching towith_initial_version()(seeds without pinning) plus an eagerrefresh_protocol_version()call right after SDK construction, so the client ratchets to the network's real active version via a cheapgetEpochsInfoquery before the version-sensitive wallet call. Verified against all three live testnet nodes.Root-caused and right-sized the
RUST_MIN_STACKstack-overflow fix, correcting an earlier wrong diagnosis. The 2026-07-06 entry below attributedtk_001b's SIGABRT to "deepdashcore::smlquorum-verification recursion" and bumped the workspace/CI stack to a precautionary, unmeasured 32 MiB. Static disassembly profiling of the SML code path found it entirely iterative (no recursion), using ~40-50KB of stack and completing ~72s before the crash — the real cause is debug-build async-frame inflation in the e2e harness's own setup helpers (tokio-shared-rt's shared runtime polls its root future viaRuntime::block_onon the libtest caller thread, not a sized tokio worker thread). Confirmed test-harness-only, not a production rs-sdk risk (matches an independent priordash-evo-toolfinding for the same bug class). Empirically validated 8 MiB is sufficient — the original crashing test passes cleanly in isolation, and the full 189-test suite runs with zero stack-overflow/SIGABRT signatures (145 passed / 44 failed, all pre-existing documented shielded-pool/finding-pin failures). Right-sized.cargo/config.tomland the three CI workflows from 32 MiB down to 8 MiB with a corrected comment (b309d640aa), and removed a same-day defensive rs-sdk low-stackWARN(added, then determined unnecessary, in the same investigation) since the risk it warned about is confined to this test-harness threading pattern.Recent updates (2026-07-23 — PR#818 reservation-lifecycle wiring + full e2e rerun)
23e6256937) — it turned out platform-wallet wasn't using it at all:next_unused_receive_addressmarked handed-out addressesUsedimmediately (next_unused_with_info+mark_index_used), never touchingAddressState::Reserved, sorelease_reservation/sweep_expired_reservations/reserved_countwere unreachable dead surface. Now usesAddressPool::next_unused_and_reserve; added idempotentrelease_receive_reservation; rebaselined every call site that assumed the old immediate-Usedsemantics (Found-026 unit tests,wallet_factory.rs,pa_002/pa_005b). New e2e case PA-005c (8-way concurrent handout viaBarrier+JoinSet, release/double-release/reissue) added and registered inTEST_SPEC.md. TDD'd (red confirmed against old code), compile + in-process unit/sim verified —found_026_tests3/3,wallet_factory::tests4/4,found_coinjoin_gap_limit_sync::sim_tests5/5.cfc2a9f8cf, 188 tests, ~50min): 100 passed / 88 failed, down from the 2026-07-06 baseline's 155/26. 83 of the 88 new failures are one systemic, non-regression root cause: a client/server proof-format version skew fromrs-drivefix(drive): bind and bound proof decoding #4165 (merged intov4.1-devthe day before this run) that the live testnet drive-abci fleet hasn't redeployed to match — every fresh test wallet'ssetup()hits"compacted address balance proof contains trailing bytes". Confirmed deterministic across multiple DAPI nodes; confirmed not caused by this branch's merge work (app_handlers Vec refactor,join_spv_taskswap, rust-dashcore pin switch — none touch that code path). Remaining 5 failures all match previously-documented behavior, no new regressions. PA-005c's own live run hits the identical block atsetup()before reaching its own logic — the new reservation code is externally blocked, not defective. Full report: seeTEST_SPEC.mdchangelog and/data/tmp/marvin-pr3549-e2e-report.md.Recent updates (2026-07-23 — v4.1-dev re-merge + rust-dashcore branch-tracking pin)
origin/v4.1-dev(mergedf6b84c3a2) — 486 commits / 173 files ours vs 332 commits / 870 files theirs since merge-base; 23 overlapping files, 4 real conflicts (Cargo.toml,Cargo.lock,manager/mod.rs,spv/runtime.rs).manager/mod.rs: kept this branch's multi-handlerVecrefactor but switcheddashpay_payment_handlerfrom move toArc::clone, since theirs added ashutdown()quiesce step needing the Arc alive afterward — a naive "take ours" resolution would've been a use-after-move compile error.spv/runtime.rs: took theirs'join_spv_taskhelper (awaits confirmed task termination) over this branch's fire-and-forget abort.revtobranch = "dash-evo-tool"— resolved as part of theCargo.tomlconflict (both sides had competingrev=pins);Cargo.lockregenerated viacargo updateagainst the branch's current tip (18c68d4c).[features] test-utilskey inrs-platform-wallet/Cargo.toml(both branches independently added it at different line offsets — merged the two meanings into one);manager/accessors.rsandmanager/load.rstest code (added by origin) constructingPlatformWalletManager::new()with a bare handler instead of theVecthis branch's refactor now requires; andtests/e2e/framework/signer.rs'sSeedBackedCoreSignernot implementingExtendedPubKeySigner, a bound origin tightened oncreate_funded_asset_lock_proof(18 compile errors insh_035_replayed_asset_lock_proof.rs) — implemented the trait by re-deriving the xpriv at the requested path and converting viaExtendedPubKey::from_priv.tk_013,tk_014,tokens.rs) with the tagged-variant action-taker JSON shape —{"$type": "contractOwner"}/{"$type": "group", "position": N}— replacing the pre-tagged-variant"ContractOwner"string /{"Group": N}shorthand.cargo check -p platform-wallet/-p platform-wallet-ffi(plain) and-p platform-wallet --tests --features e2eall green, ledger-recorded.Recent updates (2026-07-20 — v4.1-dev merge + rust-dashcore #818 repin + AddressState migration)
origin/v4.1-dev(merge6583c82c68) — this branch was 483 commits / 657 files behind (protocol v13, DIP-13 DashPay invitations, provider-key persistence, DPP JSON/Value conversion unification, shielded-wallet work, and more). 4 real conflicts (wallet_lifecycle.rs,spv/mod.rs,spv/runtime.rs,wallet/core/broadcast.rs); the rest auto-merged clean.647fa98→6bb0c5f2(PR fix(rs-dpp): Remove overflows #818's head). The merge itself already advanced the pin to19690d31(v4.1-dev's own dependency bump); the repin closed the remaining single-commit gap to fix(rs-dpp): Remove overflows #818 (d47f3a262e).AddressInfo.used/generated_at/used_at→AddressStateenum migration. PR#818 replaces the three separate fields with a singlestate: AddressState(Available/Reserved{at}/Used). Migrated every call site, production and test (d47f3a262e).CoreWallet::send_to_addressesand itsOutpointReservationsguard. v4.1-dev never had this function; minimizing platform-wallet's production diff against v4.1-dev took priority over keeping it alongside upstream'sbroadcast_transaction_releasing_reservation. Same-UTXO concurrent-selection race protection is not lost — upstream'sTransactionBuilder/ReservationSetreserves selected inputs before signing and releases them on rejection, and the e2ecore_send_from_accounthelper holds the wallet write lock through the whole select-build-sign step. e2e callers (bank.rs,al_001,cr_004) adapted to the splitbuild_signed+broadcast_transaction_releasing_reservationflow (d47f3a262e).invitation.rstest passing a bare handler wherePlatformWalletManager::newnow wants aVec(d47f3a262e); and anOrchardKeySet/AccountViewingKeystype mismatch plus 29 stale call sites missing aseed: &[u8]parameter (introduced byaba6af2420) across the--features e2e-gated shielded test suite — that feature flag exists specifically so contributors without a funded testnet bank wallet stay green, which is also why this had gone uncaught (a98a52bf0f).platform-walletlib-test binary and thee2e-feature test binary both compile and pass; clippy-D warningsclean forplatform-walletandplatform-wallet-ffi, with and without--features e2e.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.cargo test -p platform-wallet --test e2e --features e2e --no-run→ clean (full shielded + non-shielded e2e suite compiles after the v4.1-dev merge fallout fixes above).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