test(platform-wallet): integration test framework + first transfer test#3549
Open
Claudius-Maginificent wants to merge 54 commits intofix/rs-platform-wallet-auto-select-inputsfrom
Open
test(platform-wallet): integration test framework + first transfer test#3549Claudius-Maginificent wants to merge 54 commits intofix/rs-platform-wallet-auto-select-inputsfrom
Claudius-Maginificent wants to merge 54 commits intofix/rs-platform-wallet-auto-select-inputsfrom
Conversation
…cessors Two small public-API additions feeding the upcoming e2e harness: - `PlatformAddressWallet::address_derivation_info(addr)` returns the DIP-17 `(account_index, key_class, key_index)` for an address owned by the wallet, exposed via a new `AddressDerivationInfo` struct. Lets external `Signer<PlatformAddress>` impls re-derive the matching ECDSA private key from the seed without poking at internal locks. - `PlatformAddressChangeSet::fee_paid()` returns the credits burned by the transfer that produced the changeset, computed as `inputs_consumed - outputs_credited` at construction time. A new `fee_paid: Credits` field on the changeset retains the value; `Merge::merge` accumulates it (saturating-add) and `is_empty` considers it. Sync-only changesets keep `fee_paid == 0`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Operator guide for the rs-platform-wallet integration test framework: env vars, bank pre-funding, multi-process slot isolation, panic-safe cleanup via JSON registry, troubleshooting, and architecture quick reference. Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
Empty/stub modules + dev-deps so Wave 3 agents can fill in disjoint
files without re-shuffling layout. Every public surface returns
`FrameworkError::NotImplemented` and is documented to point at the
wave that will wire it.
Layout (`tests/e2e/`):
- framework/{mod,config,harness,workdir,panic_hook,wait,persistence}
- cases/mod.rs (empty, ready for Wave 4 `pub mod transfer;`)
`tests/e2e.rs` uses `#[path = ...]` on the top-level `cases` /
`framework` mods because the integration-test crate root would
otherwise resolve submodules under `tests/` rather than
`tests/e2e/`.
Cargo.toml dev-deps added: tokio-shared-rt, tempfile, dotenvy,
bip39, fs2, simple-signer (path), parking_lot, tokio-util with `rt`
feature for `CancellationToken`. async-trait + serde_json already
in `[dependencies]` and visible to tests.
`cargo check --tests`, `cargo clippy --tests -- -D warnings`, and
`cargo fmt` are all clean.
Co-Authored-By: Claudius <noreply@anthropic.com>
…cleanup
Five framework modules plus the FrameworkError surface upgrade.
- `signer.rs` — `SeedBackedPlatformAddressSigner` impls
`Signer<PlatformAddress>` by looking up DIP-17 coords via
`address_derivation_info` (Wave 1 accessor) and walking
`m/9'/coin'/17'/account'/key_class'/index` against the wallet's
root key. ECDSA secrets cached in a `parking_lot::Mutex` so the
critical section stays sync.
- `bank.rs` — `BankWallet::load` parses the BIP-39 mnemonic,
registers the wallet via the manager, runs a single BLAST sync,
captures the primary receive address for log breadcrumbs, and
PANICS with an actionable message if the balance falls below
`Config::min_bank_credits`. `fund_address` serialises through a
static `tokio::sync::Mutex` so concurrent in-process funding
calls don't race nonces.
- `wallet_factory.rs` — `TestWallet` factory + `SetupGuard` with
a Drop-warning panic-safety fallback. Default account/key-class
match `WalletAccountCreationOptions::Default` (account 0,
key_class 0).
- `registry.rs` — `PersistentTestWalletRegistry` JSON-backed under
`<workdir>/test_wallets.json`. In-memory keys are `[u8; 32]`;
on-disk keys are hex-encoded (JSON requires string keys).
Atomic write-temp + rename. Corrupt files fall back to empty
with a `tracing::warn!`. Unit tests cover round-trip + corrupt-
file recovery.
- `cleanup.rs` — `sweep_orphans` (startup) reconstructs every
registry entry, syncs, drains above-dust balances back to the
bank's primary receive address. `teardown_one` is the per-test
variant called by `SetupGuard::teardown`. Best-effort: failures
log + retain the registry entry so the next startup retries.
`framework/mod.rs` grows the `FrameworkError` enum with `Io` /
`Wallet` / `Bank` / `Cleanup` variants and registers the five new
submodules. The Wave 2 placeholder `SetupGuard` is replaced with
`pub use wallet_factory::SetupGuard;` so test authors and the
`prelude` re-export both resolve to the real type.
`SetupGuard::teardown` itself is intentionally still a stub
returning `NotImplemented` — Wave 4 wires it to
`E2eContext::{manager, bank, registry}()` accessors that don't
exist yet (those land alongside Wave 3b's SDK/SPV/ContextProvider
work). Concrete implementation lives in `cleanup::teardown_one`,
which takes the resources explicitly, so Wave 4 just threads them
through.
Cargo.toml dev-deps add `serde = { version = "1", features = [
"derive"] }` for the registry's JSON shape.
`cargo check --tests`, `cargo clippy --tests -- -D warnings`,
`cargo fmt`, and the in-binary registry unit tests (3/3) all pass.
Co-Authored-By: Claudius <noreply@anthropic.com>
Wave 3b of the rs-platform-wallet e2e harness — the network half: - `framework/sdk.rs`: `build_sdk` constructs an `Arc<dash_sdk::Sdk>` for the configured network (testnet default; devnet/regtest/local aliases). DAPI addresses come from `Config::dapi_addresses` or, for testnet, the same hard-coded peer list used in `tests/spv_sync.rs`. Bootstraps with `NoopContextProvider` so harness init can build the SDK before SPV is ready; harness then live-swaps the provider via `Sdk::set_context_provider` (backed by `ArcSwap`, so no rebuild needed — Risk #3 from the plan resolved). - `framework/spv.rs`: `start_spv` spawns the SPV runtime in the background under the workdir slot's storage path with full validation + bloom-filter mempool tracking + testnet P2P seed peers. `wait_for_mn_list_synced` polls `SpvRuntime::sync_progress` until the masternode-list manager reports `SyncState::Synced`, with a configurable timeout. - `framework/context_provider.rs`: `SpvContextProvider` wraps `Arc<SpvRuntime>` and bridges async->sync quorum-key lookups via `tokio::task::block_in_place` + `Handle::block_on`. Module docs flag the multi-thread runtime requirement. Data contracts and token configurations defer to SDK network fetch (`Ok(None)`); the activation height is a documented placeholder pending a `SpvRuntime` accessor (`TODO(Wave5)`). `mod.rs` gets the three `pub mod` declarations needed to compile the new files; no other wiring touched (Wave 4 owns the integration into `setup`/`E2eContext`). Errors temporarily flow through the existing `FrameworkError::NotImplemented` static-string variant with real diagnostic detail logged via `tracing::error!` — Wave 4 adds richer variants when it reconciles bank/registry/cleanup wiring. cargo check + clippy (-D warnings) + fmt all clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… test
Promotes every Wave-2 / Wave-3 stub to production wiring and lands
the first end-to-end test case under cases/transfer.rs.
Framework integration (`framework/`):
- `harness.rs` — full `E2eContext::init` body. `OnceCell`-backed
process singleton runs Config -> workdir slot -> panic hook ->
SDK build -> manager construction -> SPV start ->
wait_for_mn_list_synced -> live `set_context_provider` swap
to `SpvContextProvider` -> bank load -> registry open ->
startup `cleanup::sweep_orphans`. Adds `manager()` / `bank()` /
`registry()` / `spv()` / `cancel_token()` accessors. Internal
`NoopEventHandler` satisfies `PlatformEventHandler`.
- `mod.rs::setup` — wires the `OnceCell` init + `TestWallet`
creation + registry insert. Registry write happens BEFORE
returning the guard so a panic mid-test still surfaces to the
next process startup's sweep.
- `wallet_factory::SetupGuard::teardown` — now forwards to
`cleanup::teardown_one(ctx.manager(), ctx.bank(),
ctx.registry(), &test_wallet)`. Sets `teardown_called = true`
on success so `Drop` doesn't emit a spurious warning.
- `config::Config::from_env` — real `dotenvy` + env-var loader
with documented defaults. New `DEFAULT_MIN_BANK_CREDITS` const
pulled out of the `Default` impl.
- `workdir::pick_available_workdir` — real `flock` slot-fallback
loop with `MAX_SLOTS = 10`. Slot 0 IS `<base>`; higher slots
are `<base>-N`. Includes a unit test that demonstrates the
fall-through behaviour with a held lock.
- `panic_hook::install` — installs the cancellation hook (via
`take_hook` + `set_hook`) idempotently; preserves the
previously-installed hook so test output isn't suppressed.
- `wait::{wait_for, wait_for_balance}` — real poll-with-timeout
loop on a 500ms interval. `wait_for_balance` runs a fresh
`sync_balances` each round and treats sync errors as transient
(debug-log + retry).
- `bank.rs` — adds `BankWallet::network()` accessor used by
`harness::build` and `cleanup::sweep_orphans`.
Test case (`cases/transfer.rs`):
- `transfer_between_two_platform_addresses` — `#[ignore]`-gated
`#[tokio_shared_rt::test(shared)]`. Bank funds `addr_1` with
50_000_000 credits; test wallet self-transfers 10_000_000 to
`addr_2`; asserts both balances against `cs.fee_paid()` (the
Wave-1 accessor); explicit `s.teardown()` sweeps remaining
funds back to the bank.
Verification (no live testnet):
- `cargo check --tests -p platform-wallet` OK
- `cargo clippy --tests -p platform-wallet -- -D warnings` OK
- `cargo fmt -p platform-wallet` OK
- `cargo test -p platform-wallet --test e2e` 4/4 passed,
1 ignored (the network-bound transfer test)
- `cargo test -p platform-wallet --test e2e -- --ignored --list`
prints exactly one test
- `cargo test -p platform-wallet --lib` 110/110
Live testnet run is the operator's job: pre-fund the bank wallet
named by PLATFORM_WALLET_E2E_BANK_MNEMONIC and run
`cargo test --test e2e -- --ignored --nocapture`.
Co-Authored-By: Claudius <noreply@anthropic.com>
- Note in `cases/transfer.rs` that the live happy-path run is pending operator bank pre-funding; QA could not exercise it in this branch. - Note in `cases/transfer.rs` and the README's new "Status" section that `tokio_shared_rt::test(shared)` defaults to a current-thread runtime, under which `SpvContextProvider::block_in_place` panics. DET's precedent uses `flavor = "multi_thread", worker_threads = 12` for exactly this reason — follow-up Bilby pass should align this test attribute (or rework the bridge to be channel-based). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…anic, log cleanup unregister errors
QA-001 (HIGH, blocked ship):
- `cases/transfer.rs` — change test attribute from
`#[tokio_shared_rt::test(shared)]` to
`#[tokio_shared_rt::test(shared, flavor = "multi_thread", worker_threads = 12)]`.
`tokio_shared_rt::test` defaults to a current-thread runtime
under which `SpvContextProvider`'s `block_in_place` bridge
panics with "can call blocking only when running on the
multi-threaded runtime". Mirrors the
`dash-evo-tool/tests/backend-e2e/` precedent. Drops the
Wave-5 TODO that flagged the missing flavor; the live-run TODO
stays put until an operator-funded testnet bank lands.
- `tests/e2e/README.md` — canonical-pattern example updated to
match. Status section rewritten to note the resolution.
Obsolete "Note (QA Wave 5)" callout slimmed down to a
forward-looking "runtime flavor is non-optional" reminder.
QA-004 (LOW):
- `tests/e2e/README.md` — drop `mut` from `let mut s = setup()`.
`SetupGuard::teardown` consumes `self`; the binding is moved
on call and never mutated. Matches `cases/transfer.rs:74`.
QA-005 (LOW):
- `framework/bank.rs` — under-funded panic now matches the
README's friendlier multi-line format and prints the bech32m
address (`PlatformAddress::to_bech32m_string(network)`,
e.g. `tdash1q...` on testnet) instead of the `Debug`
`P2pkh([1, 2, ...])` form. Operators see the same shape in
the README's "Bank pre-funding" section and at runtime.
QA-009 (LOW):
- `framework/cleanup.rs` — replace three `let _ =
manager.remove_wallet(...)` sites (sweep-dust path, post-sweep,
teardown) with `if let Err(err) = ... { tracing::warn!(...) }`.
Failures previously silent now surface in CI logs so operators
can spot leaked manager state (e.g. SPV still tracking a
wallet's addresses on subsequent passes).
Verification:
- `cargo check --tests -p platform-wallet` OK
- `cargo clippy --tests -p platform-wallet -- -D warnings` OK
- `cargo fmt -p platform-wallet` OK
- `cargo test -p platform-wallet --test e2e` 4/4 + 1 ignored
- `cargo test -p platform-wallet --test e2e -- --ignored --list` shows
`transfer_between_two_platform_addresses`
Live-testnet verification still owned by operator with a
pre-funded `PLATFORM_WALLET_E2E_BANK_MNEMONIC` — see the
remaining `TODO(qa-wave5)` at the top of `cases/transfer.rs`.
Deferred per team-lead's brief:
- QA-002 (plan doc drift) — memcan lesson at wrap.
- QA-003 (hard-coded sweep fee) — design discussion.
- QA-006 (dead persistence.rs stub) — next maintenance pass.
- QA-007 (permissive can_sign_with) — current behaviour is
acceptable for current tests.
- QA-008 (placeholder activation height) — TODO comment is
sufficient until Core feature tests need it.
Co-Authored-By: Claudius <noreply@anthropic.com>
Course correction on QA-001 (per team-lead): replace the raw
`tokio::task::block_in_place + Handle::current().block_on`
bridge in `framework/context_provider.rs::get_quorum_public_key`
with the runtime-flavor-agnostic `dash_async::block_on` from the
workspace `dash-async` crate. The helper handles three scenarios:
- No active tokio runtime: creates a temporary current-thread
runtime for the call.
- Current-thread runtime (the `tokio_shared_rt::test` default):
spawns a dedicated OS thread with a sync_channel bridge —
sidesteps the `block_in_place` panic Marvin reproduced live.
- Multi-thread runtime: uses `block_in_place + spawn`, the
optimal path.
With this in place the e2e harness works on every tokio flavor;
the `flavor = "multi_thread"` attribute in `cases/transfer.rs`
(landed in the prior wave-6 commit) is now defense-in-depth +
parity with `dash-evo-tool/tests/backend-e2e/`, no longer
load-bearing for correctness.
`Cargo.toml` dev-deps gain `dash-async = { path =
"../rs-dash-async" }`. Module docs in
`framework/context_provider.rs` rewritten to document the new
bridge and the per-flavor handling.
Verification:
- `cargo check --tests -p platform-wallet` OK
- `cargo clippy --tests -p platform-wallet -- -D warnings` OK
- `cargo fmt -p platform-wallet` OK
- `cargo test -p platform-wallet --test e2e` 4/4 + 1 ignored
Co-Authored-By: Claudius <noreply@anthropic.com>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAn end-to-end testing framework for Changes
Sequence Diagram(s)sequenceDiagram
participant Test as E2E Test
participant Harness as E2eContext Harness
participant Registry as Wallet Registry
participant Bank as BankWallet
participant TWallet as TestWallet
participant Manager as PlatformWalletManager
participant SDK as SDK/PlatformWallet
participant Cleanup as Cleanup
Test->>Harness: init() first call
Harness->>Registry: open(test_wallets.json)
Harness->>Cleanup: sweep_orphans()
Cleanup->>Registry: list_orphans()
Cleanup->>Manager: create from orphan seed
Cleanup->>SDK: sync & drain to bank
Cleanup->>Registry: remove_orphan_entry
Harness->>Bank: load from mnemonic
Harness->>Bank: sync_balances()
Harness->>Bank: fund_address(test_addr1, credits)
Harness->>SDK: transfer via bank wallet
Test->>Test: setup() generates seed
Test->>Manager: create TestWallet
Test->>TWallet: create(seed)
Test->>TWallet: next_unused_address() → addr2
Test->>Bank: fund_address(addr2, TRANSFER_CREDITS)
Test->>SDK: transfer via bank
Test->>TWallet: wait_for_balance(addr2, expected)
TWallet->>SDK: sync_balances()
Test->>SDK: transfer(addr2 → addr1, TRANSFER_CREDITS)
SDK->>SDK: execute, compute fee
Test->>TWallet: verify balances & fee
Test->>Test: teardown()
Test->>Cleanup: teardown_one(test_wallet)
Cleanup->>TWallet: drain all addresses to bank
Cleanup->>Registry: remove_entry
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lklimek
changed the title
…funded
Live testnet run surfaced an assertion failure:
assertion `left != right` failed: wallet must hand out two distinct addresses
left: P2pkh([78, 100, 160, ...])
right: P2pkh([78, 100, 160, ...])
`PlatformAddressWallet::next_unused_receive_address` advances the
HD-pool cursor only when an address is observed as used (i.e. an
inbound balance is seen during sync). Calling it twice
back-to-back BEFORE any sync therefore returns the same address —
the cursor hasn't moved.
Reorder `cases/transfer.rs` so addr_2 is derived AFTER
`wait_for_balance(&test_wallet, &addr_1, ...)` lands. The BLAST
sync inside `wait_for_balance` marks addr_1 used; the next
derivation then lands on a fresh slot. Side benefit: the test now
also exercises the wallet's "observe inbound funds + advance
address pool" property as a happy-path invariant.
Updated the per-step comments and the module-level "Flow"
docstring to match the new ordering. The `assert_ne!` message
now reads "wallet must hand out a fresh address once addr_1 is
observed used" — phrasing matches the post-fix invariant.
Also refreshed an outdated comment block above the
`#[tokio_shared_rt::test(...)]` attribute. Wave 7 swapped the
SpvContextProvider bridge to `dash_async::block_on`, so the
multi-thread flavor is no longer load-bearing for correctness;
it stays for parity with `dash-evo-tool/tests/backend-e2e/` and
because it gives the optimal `block_in_place + spawn` path.
Verification (offline):
- `cargo check --tests -p platform-wallet` OK
- `cargo clippy --tests -p platform-wallet -- -D warnings` OK
- `cargo fmt -p platform-wallet` OK
- `cargo test -p platform-wallet --test e2e` 4/4 + 1 ignored
Live retest pending Claudius.
Co-Authored-By: Claudius <noreply@anthropic.com>
…ount
Live e2e bank funding surfaced an upstream wallet bug:
bank.fund_address: Wallet("SDK error: Protocol error: \
Input and output credits must be equal: \
inputs=499985086740, outputs=50000000")
`auto_select_inputs` in `wallet/platform_addresses/transfer.rs`
was inserting each selected address with its FULL balance as the
input's `Credits` value, then returning as soon as accumulated
covered `output + fee`. With a bank holding ~500B credits and a
50M output, the SDK got
`inputs = {bank: 499_985_086_740}, outputs = {target: 50_000_000}`
and the protocol rejected because address-funds-transfer enforces
`Σ inputs.credits == Σ outputs.credits` (verified at
`rs-dpp/.../address_funds_transfer_transition/v0/state_transition_validation.rs`).
The protocol's actual semantics (confirmed by the on-chain test
`rs-drive-abci/.../address_funds_transfer/tests.rs::test_input_balance_decreased_correctly`,
asserting `new_balance == initial_balance - transfer_amount - fee`):
- `inputs[addr].credits` = consumed amount from `addr`
- `outputs[addr]` = credited amount to `addr`
- `Σ inputs.credits == Σ outputs.credits` (strict equality)
- Fee is deducted from the targeted input address's REMAINING
balance (post-consumption), per `AddressFundsFeeStrategy`
(`DeductFromInput(0)` reduces the *remaining balance* by the
fee — never the inputs map's `Credits` value)
Fix: extract the selection loop into a pure module-scope helper
`select_inputs(candidates, outputs, total_output, fee_strategy,
platform_version)` that:
1. Walks candidates in DIP-17 order, tentatively adding each at
its full balance to drive the per-iteration fee estimate.
2. Stops when `accumulated >= total_output + estimated_fee` (the
accumulated balance must be enough to also pay the fee from
the last input's remaining balance).
3. Trims the LAST included input down to
`total_output - prior_accumulated` so
`Σ inputs.credits == total_output`.
4. If the trim is 0 (corner case where prior inputs alone
covered total_output but didn't leave enough margin for the
fee margin), drops the last address — the fee gets paid out
of the preceding input's remaining-balance margin instead of
forcing a `min_input_amount` violation.
Side benefits of the refactor:
- The pure helper is unit-testable without constructing a full
`PlatformWalletManager` + `PlatformAddressWallet`. Four new
tests in `auto_select_tests` cover the fix:
- `single_input_oversized_balance_trims_to_output_amount`
— the regression test for the Wave-8 live failure
(bank with way more than needed). Asserts
`selected[addr] == total_output` (NOT full balance and
NOT total_output + fee, the latter being a common
misconception).
- `two_input_selection_trims_only_the_last`
— trims only the last input when two are needed; first
consumed in full, second trimmed to bring sum to
`total_output`.
- `insufficient_balance_errors`
— descriptive error path.
- `no_candidates_errors`
— empty input set returns error rather than panicking.
- The full per-`PlatformAddressWallet` async method
`auto_select_inputs` now just gathers `(address, balance)`
candidates and calls `select_inputs`, which keeps the
testability win without changing public API.
Doc note in `auto_select_inputs_for_withdrawal` clarifying the
asymmetry: withdrawal validates `Σ inputs > output_amount`
(strictly greater, surplus = fee), so its drain-everything
strategy is correct by design — NOT the same bug as the transfer
selector. No code change there.
Verification:
- `cargo check --tests -p platform-wallet` OK
- `cargo clippy --tests -p platform-wallet -- -D warnings` OK
- `cargo fmt -p platform-wallet` OK
- `cargo test -p platform-wallet --lib` 114/114 (110 existing + 4 new)
Live e2e retest pending.
Co-Authored-By: Claudius <noreply@anthropic.com>
…ormEventHandler Replace the harness's NoopEventHandler with a shared `WaitEventHub` that implements `PlatformEventHandler`. The hub fans SPV sync, network, wallet, and platform-address-sync events out to a `tokio::sync::Notify`. Test wallets clone an `Arc<WaitEventHub>` from the `E2eContext` and `wait_for_balance` now awaits on the hub instead of polling on a fixed 500ms interval. The loop captures a `Notified` future BEFORE running `sync_balances`, so notifications arriving mid-sync aren't dropped. A `BACKSTOP_WAKE_INTERVAL` of 2s caps each await, keeping forward progress on idle-chain / no-peer scenarios where no events fire. The generic `wait_for` helper is unchanged — it stays polling-based for conditions outside the event hub's reach. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rb in e2e framework
User wants the production surface as close to upstream v3.1-dev as
possible — only real bug fixes, no test-only convenience accessors.
This commit reverts every Wave 1 / 4 / 6 production-code change
EXCEPT Wave 9's `auto_select_inputs` trim (which is a real upstream
bug fix) and absorbs the dependency on those reverted accessors
inside the e2e test framework.
Reverted in production code (now identical to origin/v3.1-dev):
- `PlatformAddressChangeSet::fee_paid` field, accessor,
`Merge::merge` accumulator, and `is_empty` branch
(`src/changeset/changeset.rs`).
- `fee_paid` capture / computation at construction
(`src/wallet/platform_addresses/transfer.rs`'s `transfer`
method body — auto_select trim KEPT).
- `PlatformAddressWallet::address_derivation_info` accessor and
the new `AddressDerivationInfo` struct
(`src/wallet/platform_addresses/wallet.rs`).
- Supporting `lookup_p2pkh` helper on `PlatformPaymentAddressProvider`
(`src/wallet/platform_addresses/provider.rs`).
- Re-exports of `AddressDerivationInfo` from
`src/wallet/platform_addresses/mod.rs`, `src/wallet/mod.rs`,
`src/lib.rs`.
- Doc-comment block on `auto_select_inputs_for_withdrawal`
explaining the protocol asymmetry — useful, but additive
production-code change beyond the Wave-9 trim, so reverted to
match the team-lead's "ONLY Wave 9's auto_select_inputs trim"
gate.
Kept in production code:
- Wave 9's `auto_select_inputs` trim in
`src/wallet/platform_addresses/transfer.rs` (real upstream bug
fix discovered via the live e2e run; trims the last selected
input down to the consumed amount so `Σ inputs.credits ==
Σ outputs.credits` holds. Includes the pure `select_inputs`
helper + 4 unit tests.).
Test-framework absorbs the dependency:
`tests/e2e/framework/signer.rs` — completely rewritten:
- `SeedBackedPlatformAddressSigner::new(&seed_bytes, network)`
(and `new_with_gap` for explicit gap-window control) eagerly
pre-derives every clear-funds platform-payment key in
`0..gap_limit` (default 20) via the DIP-17 path
`m/9'/coin_type'/17'/0'/0'/index`, computes each address
(RIPEMD160(SHA256(compressed pubkey))), and stores `[u8; 32]`
ECDSA secrets keyed by the 20-byte address hash.
- `sign(addr, data)` → synchronous `HashMap` lookup → SimpleSigner-
shape `dashcore::signer::sign`. No async wallet round-trip on
the hot path.
- `can_sign_with(addr)` is now a HONEST cache check (resolves
Marvin's QA-007 deferred finding as a side effect — no more
permissive `true` for any P2PKH).
`tests/e2e/framework/bank.rs` — `BankWallet::load` now derives
the 64-byte seed from the BIP-39 mnemonic via `Mnemonic::to_seed("")`
and passes it to the seed-backed signer constructor.
`tests/e2e/framework/wallet_factory.rs` — `TestWallet::create`
already had `seed_bytes: [u8; 64]` in its signature; threading it
into the new signer constructor was a one-line swap.
`tests/e2e/framework/cleanup.rs` — `sweep_one` already parses
`seed_bytes` from the registry's `seed_hex`; passes them into the
new signer constructor.
`tests/e2e/cases/transfer.rs` — fee assertion switches from
`cs.fee_paid()` to balance-delta derivation
(`fee = FUNDING_CREDITS - received - remaining`), with
`assert!(fee > 0)` and `assert!(fee < TRANSFER_CREDITS)` bounding
plausibility. The `cs` binding is dropped (transfer's return value
is no longer needed for assertions). A debug `tracing::info!` log
records the observed fee for operator visibility.
`tests/e2e/README.md` — canonical example updated to match the
balance-delta fee derivation.
`book/src/sdk/wallet.md` — verified clean, no references to
`fee_paid` / `address_derivation_info` to remove.
Verification:
- `cargo check -p platform-wallet --tests` OK
- `cargo clippy -p platform-wallet --tests -- -D warnings` OK
- `cargo fmt -p platform-wallet` OK
- `cargo test -p platform-wallet --test e2e` 4 passed + 1 ignored
- `cargo test -p platform-wallet --test e2e -- --ignored --list`
shows `transfer_between_two_platform_addresses`
- `git diff origin/v3.1-dev -- src/` ONLY
`transfer.rs` (Wave 9's auto_select_inputs trim — 269+/42-)
- `cargo test -p platform-wallet --lib` pre-revert
the lib added 4 auto_select_tests; those are still in transfer.rs
and pass (114 lib tests total)
Live retest pending Claudius — with the new seed-backed signer the
test should now (a) produce a working bank signer (50M funding
transfer), (b) produce a working test-wallet signer (10M
self-transfer), (c) derive the fee from observed balances and
pass the new bound assertions.
Resolves: QA-007 (`can_sign_with` honesty) as a side benefit.
Co-Authored-By: Claudius <noreply@anthropic.com>
Live e2e runs against testnet were timing out at 180s in
`framework::spv::wait_for_mn_list_synced`. Investigation:
- The wait predicate (`MasternodesProgress::state() == Synced`) is
correct — the dash-spv `MasternodesManager` reaches `Synced` at
the end of `verify_and_complete()` once the QRInfo + non-rotating
quorum verification pipeline drains. New blocks after that drive
incremental updates *while staying in `Synced`*, so the predicate
is reachable on a live network.
- DET's `wait_for_spv_running` checks the `SpvStatus::Running`
flag set after `SyncEvent::SyncComplete` fires — same underlying
signal, just exposed via app-level state.
- The `tests/spv_sync.rs` integration test uses a 600s timeout for
the same cold-cache scenario; the 180s `SPV_READY_TIMEOUT` baked
into the harness was simply too short for ~1.4M+ headers + ~3.6M
filters + a full QRInfo round-trip on a fresh cache.
Root cause classification: (b) — predicate correct, timeout too short.
Fix, scoped to `framework/spv.rs` only:
- Lift the effective timeout to `timeout.max(600s)` via a
`COLD_CACHE_TIMEOUT_FLOOR` constant. Larger caller-supplied
timeouts still pass through unchanged.
- Drop the polling interval to 500ms so the wait reacts faster
once mn-list flips to `Synced`.
- Emit `info`-level pipeline snapshots every 30s (and once on
timeout) summarising headers / filter-headers / filters /
masternodes state, current and target heights — so future
cold-run hangs are debuggable from default logs.
- Track `(state, height)` together for the per-change debug log
so `WaitForEvents → WaitingForConnections → Syncing → Synced`
transitions are visible even when current_height stays at 0.
Production code is untouched (Wave 11 territory). No new dependency
on `WaitEventHub` — the existing 500ms poll is responsive enough now
that the timeout floor is realistic.
Co-Authored-By: Claudius <noreply@anthropic.com>
…ctivation-height assumption QA-006 — delete the Wave-2 `persistence.rs` stub: - The module shipped in Wave 2 as a placeholder for a `TestPersister` wrapper that Wave 3 was meant to fill in. Wave 4 wired `NoPlatformPersistence` directly inside `harness::E2eContext::build` instead, leaving the stub orphaned. Marvin's QA pass flagged it as dead-code in QA-006; this commit drops it. - `tests/e2e/framework/persistence.rs` removed. - `pub mod persistence;` declaration in `framework/mod.rs` removed alongside its prelude bullet in the module-level docs. No callers to update — confirmed via `grep -rn "TestPersister\|persistence::" tests/e2e/` returning zero hits before / after. QA-008 — document the testnet-only activation-height assumption: - `framework/context_provider.rs`: replace the `TODO(Wave5)` placeholder block on the activation-height constant with explicit rustdoc explaining WHY hard-coding `0` is safe-by-position for the e2e framework's testnet-only scope (mn_rr activation on testnet is past every height the platform-address transfer flow exercises; the verification path that consumes this value never compares against an unactivated quorum). Constant renamed `PLACEHOLDER_ACTIVATION_HEIGHT` → `PLATFORM_ACTIVATION_HEIGHT_TESTNET_SAFE` so the assumption shows up at the use-site too. Forward-looking pointer for future tests retained: surface the real value via `SpvRuntime` and wire it through if a Core / mainnet path needs it. - `cases/transfer.rs`: new `# Testnet assumption` section in the module-level `//!` docs flags that the test depends on the hard-coded activation height being safe-by-position, with a pointer back to the rationale in the `context_provider` constant docs. Verification (offline): - `cargo check -p platform-wallet --tests` OK - `cargo clippy -p platform-wallet --tests -- -D warnings` OK - `cargo fmt -p platform-wallet` OK - `cargo test -p platform-wallet --test e2e -- --ignored --list` shows `transfer_between_two_platform_addresses` - 4 files touched: 3 modified (`mod.rs`, `context_provider.rs`, `cases/transfer.rs`), 1 deleted (`persistence.rs`). No production-code (`src/`) changes — the diff against `origin/v3.1-dev -- packages/rs-platform-wallet/src/` remains exactly the Wave 9 `auto_select_inputs` trim in `transfer.rs`, no other production-code drift. Co-Authored-By: Claudius <noreply@anthropic.com>
…PV until stable Swap the e2e harness's context-provider strategy from the local SPV runtime to `rs-sdk-trusted-context-provider::TrustedHttpContextProvider`, which answers quorum public-key lookups over a trusted HTTP endpoint (testnet/mainnet defaults baked into the crate). This delivers fast and reliable testnet runs while the SPV cold-start path stabilizes (Task #15). Cargo.toml dev-deps gain `rs-sdk-trusted-context-provider = { path = "../rs-sdk-trusted-context-provider" }`. `framework/sdk.rs`: - `build_sdk` now installs `TrustedHttpContextProvider` directly via `SdkBuilder::with_context_provider`. No more `NoopContextProvider` placeholder + later `set_context_provider` swap. - New helper `build_trusted_context_provider` honours the optional `Config::trusted_context_url` override (`PLATFORM_WALLET_E2E_TRUSTED_CONTEXT_URL` env var) and falls back to the network-builtin URL via `TrustedHttpContextProvider::new`. Cache size: 256 entries (LRU; the provider only allocates on a miss). - `NoopContextProvider` impl removed (no longer needed). `framework/config.rs`: - `trusted_context_url: Option<String>` field added with `None` default. - `vars::TRUSTED_CONTEXT_URL` constant added. - `from_env` parses the new env var with whitespace-trim and empty-string filter. `framework/harness.rs`: - SPV start + readiness wait + ctx-provider live-swap blocks COMMENTED OUT — not deleted — with a clear marker block showing exactly what to uncomment when SPV stabilises (the `SPV_READY_TIMEOUT` const, the `spv` / `context_provider` imports, the `start_spv` / `wait_for_mn_list_synced` / `set_context_provider` calls). - `E2eContext::spv_runtime` field changed from `Arc<SpvRuntime>` to `Option<Arc<SpvRuntime>>` (current default `None`). Keeps the field shape so future Core-feature tests don't churn signatures when SPV returns; the `spv()` accessor returns `Option<&Arc<SpvRuntime>>` accordingly. - Module-level `//!` docs rewritten to reflect the new init order (no SPV step) plus a "SPV-based context provider — currently disabled" section. `framework/spv.rs`, `framework/context_provider.rs`: - Top-level `//! NOTE` headers added flagging the modules as currently disabled in favour of `TrustedHttpContextProvider`, with a pointer to harness.rs's commented-out wiring and the Task #15 re-enablement path. - Modules stay compilable; the framework's existing `#![allow(dead_code)]` (mod.rs:35) covers the unused symbols without per-item annotations. `tests/e2e/README.md`: - New "Context provider" section explaining the `TrustedHttpContextProvider` default and the `PLATFORM_WALLET_E2E_TRUSTED_CONTEXT_URL` override (with a ready-to-paste shell example). - New "Deferred" section listing SPV-based context provider (Task #15) with a pointer to the harness.rs commented block. - "Future Core support" section updated: when Task #15 lands, `SpvRuntime` will run for the test process lifetime and `SpvContextProvider` will be live-swapped after mn-list sync; the public test API stays unchanged. - Env-var table gains `PLATFORM_WALLET_E2E_TRUSTED_CONTEXT_URL` row. Verification (offline): - `cargo check -p platform-wallet --tests` OK - `cargo clippy -p platform-wallet --tests -- -D warnings` OK - `cargo fmt -p platform-wallet` OK - `cargo test -p platform-wallet --test e2e` 4 passed + 1 ignored - `cargo test -p platform-wallet --test e2e -- --ignored --list` shows `transfer_between_two_platform_addresses` Production-code diff against `origin/v3.1-dev` is unchanged (still exclusively Wave 9's `auto_select_inputs` trim in `transfer.rs`); this commit only touches dev-deps + e2e framework files + the e2e README. Live retest pending Claudius. With the trusted HTTP provider in place the harness should reach the bank load + balance check in seconds rather than the 95s cold-start SPV took, and the test body should run the full bank → fund → wait → transfer → assert → teardown loop. Co-Authored-By: Claudius <noreply@anthropic.com>
… teardown Live testnet retest surfaced QA-003 from latent to manifest: teardown: Insufficient balance: available 40448500 credits, required 46448500 (outputs 39448500 + estimated fee 7000000) The old `SWEEP_FEE_ESTIMATE = 1_000_000` was wildly under the real testnet fee. Observed in early 2026: - 1-input → 1-output: ~9.55M credits - 2-input → 1-output: ~7.00M credits Bump `SWEEP_FEE_ESTIMATE` from 1M to 15M, comfortably covering 1-3 input scenarios. Bump `SWEEP_DUST_THRESHOLD` proportionally from 1M to 5M so the minimum-worth-sweeping total (`dust + fee = 20M`) recovers at least 5M net of fees rather than the implausible 1M of the old constants. Constant docs strengthened to: - Spell out the observed testnet fee bracket so future operators can sanity-check the value when retuning. - Cross-reference QA-003 (Marvin's deferred finding) and the long-term plan: lift the wallet's existing `transfer::estimate_fee_for_inputs` to a small public helper and call it from cleanup.rs, so the estimate stays accurate across protocol-version bumps. Tracked as a follow-up; until then bump the constant when testnet fee observations move beyond ~10M. No other behavior change. Build / clippy / fmt / test discovery all clean. Verification (offline): - `cargo check -p platform-wallet --tests` OK - `cargo clippy -p platform-wallet --tests -- -D warnings` OK - `cargo fmt -p platform-wallet` OK Live retest pending Claudius. The teardown sweep should now have the right margin to succeed in a single transition; combined with Wave 14's TrustedHttp provider, a full happy-path run is within reach. Co-Authored-By: Claudius <noreply@anthropic.com>
…ass auto_select trim
QA-003 kept biting because the e2e cleanup paths had two
fee estimators that disagreed:
1. `cleanup.rs` computed `outputs = total_balance - SWEEP_FEE_ESTIMATE`
(15M margin, set in Wave 15) and called `transfer` with
`InputSelection::Auto`.
2. `auto_select_inputs` in `transfer.rs` (Wave 9) trimmed the last
selected input down to `total_output - prior_accumulated`,
computing required = `total_output + estimate_fee_for_inputs(...)`.
`estimate_fee_for_inputs` reflects the protocol's
`AddressFundsTransferTransition::estimate_min_fee` (~5M for a
1→1 testnet transition, far less than the harness's 15M
`SWEEP_FEE_ESTIMATE`).
When the caller's `total_output` was constructed from
`SWEEP_FEE_ESTIMATE` but `auto_select` did its own (smaller)
fee estimate, the resulting `Σ inputs` carried the auto-select
estimate's leftover instead of the harness's, and the protocol's
strict `Σ inputs == Σ outputs` check rejected the transition.
Live observation: `inputs=30522500, outputs=25522500` — 5M off
(auto_select's estimate, not the SWEEP_FEE_ESTIMATE).
Fix: introduce `cleanup::drain_to_bank(&wallet, &signer, &bank_addr)`
that uses `InputSelection::Explicit` so no trimming happens. The
helper:
1. Snapshots non-zero balances for the wallet's default account.
2. Skips the sweep if total <= dust + fee_estimate (same gate as
before).
3. Picks the LARGEST-balance address as the fee bearer (its
remaining balance after consumption must cover the on-chain
fee, so largest is the safest pick).
4. Builds `inputs_map`: every address contributes its full
balance EXCEPT the fee bearer, which contributes
`balance - SWEEP_FEE_ESTIMATE` so 15M stays at the fee
bearer as the on-chain fee margin.
5. Computes the fee bearer's index in BTreeMap iteration order
so `DeductFromInput(N)` targets the right input. (BTreeMap is
sorted by `PlatformAddress`'s natural Ord, which matches what
`deduct_fee_from_outputs_or_remaining_balance_of_inputs_v0`
uses to index inputs.)
6. Calls `wallet.platform().transfer(account, Explicit(inputs_map),
{bank: total_consumed}, [DeductFromInput(N)], …)`.
`Σ inputs == Σ outputs` holds by construction (both equal
`total - SWEEP_FEE_ESTIMATE`); the on-chain fee comes from the
fee bearer's remaining balance via the strategy.
Both `sweep_one` (orphan startup-sweep) and `teardown_one` (per-test
happy-path) now route through the same helper:
- `sweep_one` calls `drain_to_bank(&wallet, &signer,
bank.primary_receive_address())` against the locally
reconstructed wallet.
- `teardown_one` calls
`drain_to_bank(test_wallet.platform_wallet(),
test_wallet.address_signer(), …)` — TestWallet exposes both
via existing accessors, no new methods required.
Edge case: the helper errors with a clear message if the
fee-bearer's balance is below `SWEEP_FEE_ESTIMATE`. That only
happens when a wallet's funds are so spread out across many
small balances that no single address can cover the fee — outside
the e2e test's normal distribution (max two addresses per test).
Verification (offline):
- `cargo check -p platform-wallet --tests` OK
- `cargo clippy -p platform-wallet --tests -- -D warnings` OK
- `cargo fmt -p platform-wallet` OK
- `cargo test -p platform-wallet --test e2e` 4 passed + 1 ignored
- `cargo test -p platform-wallet --lib auto_select_tests` 4/4 (Wave 9
unit tests still pass — `auto_select_inputs` itself unchanged;
the cleanup paths simply don't go through it anymore)
No production-code (`src/`) changes — production diff vs
`origin/v3.1-dev` remains exactly Wave 9's `auto_select_inputs`
trim. Cleanup-only fix in the test framework.
Live retest pending Claudius. Both teardown and startup-sweep
should now succeed: SDK gets matched `Σ inputs == Σ outputs`
maps with explicit DeductFromInput targeting the fee bearer.
Co-Authored-By: Claudius <noreply@anthropic.com>
…nput sweeps Live testnet retest after Wave 16's structural fix (Explicit input selection + DeductFromInput targeting the largest-balance fee-bearer) cleared the protocol's `Σ inputs == Σ outputs` mismatch but tripped a fresh fee-margin failure on 2-input sweeps. Observed protocol fees scale with input count: - 1-input → 1-output: ~9.55M credits (single-address transfer) - 2-input → 1-output: ~20.9M credits (Wave 17 live-observed) - 3-input → 1-output: ~30M credits (projected linear scaling) Each additional input adds witness + signature bytes that the fee schedule charges for. Wave 15's 15M margin covered only 1-input sweeps; the typical e2e teardown has 2 owned addresses (addr_1 with bank-funded balance + addr_2 from the self-transfer) and the 2-input fee blew past the 15M reserved. Bump `SWEEP_FEE_ESTIMATE` from 15M to 30M, which covers up to 3 inputs comfortably and exceeds the e2e test's normal distribution. The doc-comment on the constant is rewritten to spell out the observed / projected fee curve so future operators can sanity-check the value when retuning. `SWEEP_DUST_THRESHOLD` stays at 5M — the minimum-worth-sweeping total moves to `dust + fee = 35M` (recovers at least 5M net of fees). Wallets whose largest single address has < 30M can't be swept in a single transition and will sit in the persistent registry until topped up; the existing `drain_to_bank` short-circuits cleanly with a clear error in that case rather than silently leaking dust. Acceptable trade-off for the test scope. The long-term fix remains unchanged from Wave 15's note: lift `transfer::PlatformAddressWallet::estimate_fee_for_inputs` to a small public helper and call it from `cleanup.rs` so the estimate stays accurate across protocol-version bumps. Tracked as a follow-up to Marvin's QA-003. Verification (offline): - `cargo check -p platform-wallet --tests` OK - `cargo clippy -p platform-wallet --tests -- -D warnings` OK - `cargo fmt -p platform-wallet` OK No production-code (`src/`) changes; production diff vs `origin/v3.1-dev` remains exactly Wave 9's `auto_select_inputs` trim. Constant-only fix in `tests/e2e/framework/cleanup.rs`. Live retest pending Claudius. With Waves 14 (TrustedHttp), 16 (Explicit selection), and 17 (multi-input fee margin) in place both teardown_one and sweep_orphans should clear all fee gates on the typical 2-address e2e wallet. Co-Authored-By: Claudius <noreply@anthropic.com>
…ore live test
Two ergonomic improvements making the e2e test the same kind of
"set up `.env` once and run `cargo test`" experience as the rest
of the workspace's integration-test harnesses.
1. **`.env` loading** mirrors the convention used by
`packages/rs-sdk/tests/fetch/config.rs:95-98` and
`packages/rs-sdk-ffi/tests/integration_tests/config.rs:76-78`.
`framework/config.rs::Config::from_env` now anchors `.env` at
`${CARGO_MANIFEST_DIR}/tests/.env` via `dotenvy::from_path`
instead of falling through to `dotenvy::dotenv()`'s CWD walk.
The path is deterministic regardless of the shell's CWD;
missing `.env` is silently OK (process env vars stay the
source of truth); a malformed file logs a `tracing::warn!`
pointing at the offending path.
Operator template lives at
`packages/rs-platform-wallet/tests/.env.example` — copy it to
`tests/.env` and fill in `PLATFORM_WALLET_E2E_BANK_MNEMONIC`.
The template documents every supported env var (network,
DAPI overrides, min-credits, workdir base, trusted-context
URL, RUST_LOG) with the same defaults the framework uses,
commented out so the template is a working starting point.
2. **`#[ignore]` removed** from
`cases::transfer::transfer_between_two_platform_addresses`.
The test now runs by default once `tests/.env` is in place;
`cargo test --test e2e -- --nocapture` is the canonical
command. If `PLATFORM_WALLET_E2E_BANK_MNEMONIC` is unset or
the bank is under-funded, the existing harness panics with
the actionable bank-under-funded message (Wave 6's polish)
naming the bank's primary receive address — the failure is
operator-actionable, not silent. CI gating happens at the
workflow level, not via `#[ignore]`.
`tests/e2e/README.md` updated:
- "Tests run by default" + a one-paragraph operator-error
story (panic with primary receive address) replaces the old
"all tests carry `#[ignore]`" wording.
- Configuration section names the canonical `tests/.env`
location and the `tests/.env.example` template; spells out
the rs-sdk parity. `cp tests/.env.example tests/.env` snippet
shows the one-time setup.
- Every `cargo test … --ignored …` invocation in the README
drops the `--ignored` flag (4 sites).
- The canonical-pattern example test attribute drops its
`#[ignore = …]` line.
Stale comment block at the top of `cases/transfer.rs` that
referenced Marvin's wave-5 "live happy-path run pending operator
bank pre-funding" TODO is removed — the operator setup now
lives in the README + `.env.example`, and the test no longer
needs the breadcrumb.
Verification (offline):
- `cargo check -p platform-wallet --tests` OK
- `cargo clippy -p platform-wallet --tests -- -D warnings` OK
- `cargo fmt -p platform-wallet` OK
- `cargo test -p platform-wallet --test e2e -- --list`
shows `cases::transfer::transfer_between_two_platform_addresses: test`
WITHOUT the `(ignored, ...)` annotation.
No production-code (`src/`) changes; the diff against
`origin/v3.1-dev -- src/` remains exactly Wave 9's
`auto_select_inputs` trim. Wave 18 touches:
- `tests/.env.example` (new)
- `tests/e2e/cases/transfer.rs` (drop `#[ignore]` + stale TODO)
- `tests/e2e/framework/config.rs` (rs-sdk-style `.env` loader)
- `tests/e2e/README.md` (operator-facing wording)
The workspace `.gitignore` already covers `.env` anywhere, so
the operator's mnemonic stays uncommitted by default. After
the operator moves their existing `.env` from
`/home/ubuntu/platform/.env` to
`/home/ubuntu/platform/packages/rs-platform-wallet/tests/.env`,
`cargo test --test e2e -- --nocapture` should run end-to-end.
Co-Authored-By: Claudius <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an end-to-end (wallet → SDK → broadcast) integration test harness to rs-platform-wallet and introduces the first live test case (address-funds transfer), alongside a production fix to InputSelection::Auto input selection so generated transitions satisfy protocol structure rules.
Changes:
- Added a reusable E2E framework under
packages/rs-platform-wallet/tests/e2e/(workdir slot locking, bank wallet, persistent registry, cleanup/sweep, wait hub, signer, SDK wiring). - Added the first E2E test case: transferring credits between two platform-payment addresses in a test wallet (ignored by default).
- Fixed
auto_select_inputsin production code to avoid selecting full balances as “input credits”, and added unit tests for the selection logic.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs | Fixes auto input selection; adds pure helper + unit tests for selection behavior. |
| packages/rs-platform-wallet/tests/e2e.rs | Adds the integration test crate root and module wiring for the e2e suite. |
| packages/rs-platform-wallet/tests/e2e/README.md | Operator/setup documentation for running live e2e tests. |
| packages/rs-platform-wallet/tests/e2e/cases/mod.rs | Declares e2e test modules. |
| packages/rs-platform-wallet/tests/e2e/cases/transfer.rs | First e2e test exercising funding + self-transfer + teardown. |
| packages/rs-platform-wallet/tests/e2e/framework/mod.rs | Framework public surface (setup, errors, prelude) and module layout. |
| packages/rs-platform-wallet/tests/e2e/framework/harness.rs | E2eContext singleton init: config, workdir locking, SDK, manager, bank, registry, startup sweep. |
| packages/rs-platform-wallet/tests/e2e/framework/config.rs | Env/.env configuration loader for the harness. |
| packages/rs-platform-wallet/tests/e2e/framework/sdk.rs | Constructs dash_sdk::Sdk with TrustedHttpContextProvider and DAPI address resolution. |
| packages/rs-platform-wallet/tests/e2e/framework/workdir.rs | Cross-process workdir slot selection via flock. |
| packages/rs-platform-wallet/tests/e2e/framework/panic_hook.rs | Installs panic hook to cancel background work on panic. |
| packages/rs-platform-wallet/tests/e2e/framework/wait_hub.rs | Notify-based hub bridging wallet/SPV/platform events to async waiters. |
| packages/rs-platform-wallet/tests/e2e/framework/wait.rs | Async waiting helpers (event-driven balance wait + generic polling). |
| packages/rs-platform-wallet/tests/e2e/framework/signer.rs | Seed-backed Signer<PlatformAddress> with eager DIP-17 key cache. |
| packages/rs-platform-wallet/tests/e2e/framework/wallet_factory.rs | Test wallet factory + SetupGuard (panic-safe registry-backed lifecycle). |
| packages/rs-platform-wallet/tests/e2e/framework/registry.rs | JSON-backed persistent registry for panic-safe orphan recovery. |
| packages/rs-platform-wallet/tests/e2e/framework/cleanup.rs | Startup sweep + per-test teardown draining funds back to bank. |
| packages/rs-platform-wallet/tests/e2e/framework/bank.rs | Loads and syncs a pre-funded bank wallet; serialized funding API. |
| packages/rs-platform-wallet/tests/e2e/framework/context_provider.rs | Retained (disabled) SPV-backed SDK context provider module for future re-enable. |
| packages/rs-platform-wallet/tests/e2e/framework/spv.rs | Retained (disabled) SPV startup/readiness helpers for future re-enable. |
| packages/rs-platform-wallet/Cargo.toml | Adds dev-dependencies needed by the e2e harness. |
| Cargo.lock | Locks new/updated dependencies for the added test tooling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6 tasks
Triages the seven inline comments left by `copilot-pull-request-reviewer`: * `auto_select_inputs` now keeps Σ inputs == total_output even when the tail candidate was added only to satisfy the per-input fee margin. The previous trim path dropped the last input but left earlier inputs at full balance, allowing Σ inputs > total_output and tripping the protocol's `Σ inputs == Σ outputs` invariant. Selection state moved to a `Vec` so the result is built front-to-back from insertion order, with a regression test (`fee_only_tail_input_does_not_inflate_input_sum`). * `registry.rs` `atomic_write_json` now persists via `tempfile::NamedTempFile::persist`, which uses `MoveFileEx` with `MOVEFILE_REPLACE_EXISTING` on Windows (cross-platform overwrite), and the module / fn docs match the actual no-fsync behavior. * Stale "Wave 2 skeleton" / "live run not yet executed" / "15M fee estimate" / "multi-thread MUST" notes updated in `e2e.rs`, `tests/e2e/README.md`, and `tests/e2e/framework/cleanup.rs` to match Wave-7+ reality (`TrustedHttpContextProvider` default, runtime-flavor-agnostic `dash_async::block_on`, 30M sweep margin, successful live testnet run). Live happy-path test passes in 20s; unit tests (5/5 for select_inputs, 4/4 for the e2e harness modules) green.
8 tasks
…-platform-wallet-e2e
…tication_path
`AccountType::IdentityAuthenticationEcdsa { identity_index }` was
removed from `key-wallet` between revs `4c8bec3` and `ea33cbc8`.
Replaced with the new top-level `DerivationPath::identity_authentication_path(
network, KeyDerivationType::ECDSA, identity_index, key_index)` API
(`key-wallet/src/bip32.rs:1115`), which bakes both identity_index and
key_index into the path directly — `key_index` becomes the loop
variable instead of an external `extend([leaf])` step.
…ectly PR #3549 dedup re-audit (PROJ-001): the local DEFAULT_GAP_LIMIT = 20 shadows key_wallet's canonical pub const DIP17_GAP_LIMIT (rust-dashcore ea33cbc8: key-wallet/src/gap_limit.rs:26). Drop the local constant and import the upstream one — drift here would silently de-sync the harness from key-wallet's own gap policy.
…gh PlatformWallet PR #3549 dedup re-audit (PROJ-002): derive_platform_address_at_index was running BIP-32 manually from raw seed bytes. The bank already holds a PlatformWallet whose Wallet::derive_public_key (key-wallet/src/wallet/ helper.rs:763) does the same thing, so the parallel-derivation surface was redundant. Take the existing &Arc<PlatformWallet>, call .state().await.wallet().derive_public_key(&path), hash the result. Drops the bip39 seed-bytes consumer (the seed bytes are still derived once for SeedBackedPlatformAddressSigner::new four lines below). Net removes RootExtendedPrivKey, Secp256k1, PublicKey imports from bank.rs.
…r wrapper `framework/signer.rs` was a 78-line do-nothing shell around `SimpleSigner::from_seed_for_platform_address_account`: - the `Signer<PlatformAddress>` trait impl just delegated to inner; - `SimpleSigner` already implements that trait directly (`packages/simple-signer/src/signer.rs:338`); - `cached_key_count` and `new_with_gap` had zero callers outside the module; - the only added value was pinning `account=0`/`key_class=0`, which collapses to four lines of construction code. Replace with `framework::make_platform_signer(seed_bytes, network) -> SimpleSigner` next to the `FrameworkError`/`FrameworkResult` types in `mod.rs`. The three call sites (`bank.rs`, `wallet_factory.rs`, `cleanup.rs`) now hold `SimpleSigner` directly and pass it straight to `PlatformAddressWallet::transfer`. `TestWallet::address_signer()` returns `&SimpleSigner` for the same reason.
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 GateCommit:
|
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.
…-platform-wallet-e2e
lklimek
added
the
ready for final review
…rant transfer fixture
* `transfer.rs` — funding/transfer fixtures handle `[ReduceOutput(0)]`
fee deduction via post-fee floors and split-fee assertions
(bank_fee + transfer_fee), so the test no longer asserts the gross
amount lands intact. Module doc points at the actual error path
(`FrameworkError::Bank` for missing mnemonic, panic for under-funded
bank) per Copilot's `transfer.rs:7` note.
* `config.rs` — replace `derive(Debug)` with a manual impl that
redacts `bank_mnemonic` so a stray `{config:?}` log or panic
backtrace can't leak the shared funding seed (CodeRabbit `config.rs:50`).
* `workdir.rs` — match `ErrorKind::WouldBlock` as slot-busy and
propagate every other IO error as `FrameworkError::Io`, instead of
swallowing them all as "slot busy" (CodeRabbit `workdir.rs:50`).
* `registry.rs` — drop the never-set `EntryStatus::Sweeping` variant +
doc references; the per-slot workdir lock already serialises the
only writer, so no transient cross-process state is required
(Copilot `registry.rs:35`, `cleanup.rs:75`).
* `cleanup.rs` — replace the hardcoded `SWEEP_DUST_THRESHOLD` constant
with the protocol's `min_input_amount` from `PlatformVersion`, so
the sweep gate stays in lock-step with whatever `address_funds`
validation requires.
* `wait_hub.rs` — fix stale `platform_address_sync` import path; the
module moved to `manager::platform_address_sync` in PR #3564 and
is re-exported at the crate root.
* `README.md` — fenced-code-block language tags (MD040), corrected
workdir-exhausted error string, first-run timing reflects
`TrustedHttpContextProvider` default (no SPV in critical path),
troubleshooting note rescoped, teardown step list no longer claims
to wait for the bank to observe credits.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…network-seeds Replaces the hardcoded `TESTNET_DAPI_ADDRESSES` list in `framework/sdk.rs` with a `default_address_list_for_network` helper that mirrors PR #3533's upstream `default_address_list_for_network` byte-for-byte: pulls `dash_network_seeds::evo_seeds(network)`, filters seeds with a `platform_http_port`, and constructs DAPI URLs from the seed IPs. Once PR #3533 (`feat(sdk): source mainnet/testnet bootstrap from dash-network-seeds`) lands in `v3.1-dev` and exposes `SdkBuilder::new_testnet()` properly (currently `unimplemented!()` on this branch's base), the helper collapses into a single `SdkBuilder::new_testnet()` call with no behavioural delta. `framework/spv.rs::seed_p2p_peers` follows suit: testnet peer IPs come from `dash_network_seeds::evo_seeds(Testnet)` when the operator hasn't supplied an explicit DAPI list. Also drops the dead `TESTNET_DAPI_ADDRESSES` re-import. Adds `dash-network-seeds` as a dev-dependency, pinned to the same rust-dashcore rev as the workspace `dashcore` to keep all sibling crates in lock-step. Resolves the `sdk.rs:41` review thread. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Apr 30, 2026
…3040 Bumps `FUNDING_CREDITS` 50M -> 100M and `TRANSFER_CREDITS` 10M -> 50M (plus matching floors) so `output[0]` comfortably exceeds Drive's chain-time fee. Issue #3040 (`calculate_min_required_fee` is too low) causes `[ReduceOutput(0)]` selections with small `output[0]` to fail at chain time despite passing the static-fee check. Picking output amounts well above the empirical chain-time ceiling sidesteps the bug until the dpp-layer fix lands. Bumps `DEFAULT_MIN_BANK_CREDITS` 100M -> 500M to keep the bank covering several runs at the larger per-run cost (also follows DET's 5x safety-factor pattern from dash-evo-tool#513). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ct-inputs' into feat/rs-platform-wallet-e2e
…hat #3570 landed PR #3570 (backport of #3533) merged into v3.1-dev, making `SdkBuilder::new_testnet()` and `new_mainnet()` real on this branch's base. Drops the harness's local `default_address_list_for_network` helper (which had been the byte-for-byte mirror placeholder) and delegates to the upstream builders directly. Network-explicit operator overrides via `PLATFORM_WALLET_E2E_DAPI_ADDRESSES` still route through `SdkBuilder::new(...)`. Switches `dash-network-seeds` from a git-rev-pinned dev-dep to `workspace = true` — PR #3570 added the workspace entry; the dep now only serves `framework/spv.rs::seed_p2p_peers`, which the SPV runtime needs in raw `SocketAddr` form (no `SdkBuilder`-equivalent helper exists for that). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…peers from SDK address list * Adds `Config::p2p_port: Option<u16>` plus the `PLATFORM_WALLET_E2E_P2P_PORT` env var. `None` falls back to `default_p2p_port(network)` (mainnet 9999, testnet 19999); regtest / devnet require the explicit override. `effective_p2p_port` resolves the override-or-default for callers. * Drops the hardcoded `TESTNET_P2P_PORT = 19999` constant and the `Network::Testnet`-only guard in `seed_p2p_peers`. * `seed_p2p_peers` now consumes the SDK's live `AddressList` instead of forking from `dash_network_seeds::evo_seeds(network)` — same source of truth as `framework/sdk.rs`'s SDK construction, so the SPV peer list can't drift from the DAPI endpoints the SDK is actually using. IPs come from each `Address::uri().host()`; non-IP hosts (DNS targets) are left for the SPV client's discovery loop. * `start_spv` takes the address list as a new parameter; the commented-out caller in `harness.rs` updated to pass `sdk.address_list()`. * Drops `dash-network-seeds` from `[dev-dependencies]` — the workspace entry stays for other consumers, but the platform-wallet test harness no longer needs it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…truction `Config::from_env` and `Config::default` now return a fully-resolved configuration — every defaultable field carries its final value as of construction. Callers don't have to re-derive defaults at use time; the read-then-derive helpers (`parse_network`, `effective_p2p_port`) are gone. Concretely: * `Config::network: String` -> `Network`. Parsed at construction via the now-private `parse_network` helper, which still accepts the `local` alias and the empty-string testnet shorthand. * `Config::p2p_port: Option<u16>` semantics preserved (`None` only for regtest / devnet without an override) but the value is the resolved override-or-default — no further lookup required. Resolution happens in `Config::from_env` and `Default::default` via the now- private `default_p2p_port` helper. * `parse_network` and `default_p2p_port` are demoted from `pub(super)` to private — they're construction-time implementation details, not part of the cross-module API. * `effective_p2p_port` deleted entirely (callers read `config.p2p_port` directly). * `bank.rs`, `sdk.rs`, `spv.rs` updated to consume the resolved `config.network` / `config.p2p_port` instead of re-parsing. `seed_p2p_peers` drops the explicit `Network` argument since the resolved port already encodes whatever network-default came from config construction. No behaviour delta — just moves the resolution from "every call site" to "one place at boot." Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
5 tasks
thepastaclaw
suggested changes
May 2, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
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
+30
to
+31
| #[tokio_shared_rt::test(shared)] | ||
| async fn transfer_between_two_platform_addresses() { |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: 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.
💡 Suggested change
Suggested change
| #[tokio_shared_rt::test(shared)] | |
| async fn transfer_between_two_platform_addresses() { | |
| #[tokio_shared_rt::test(shared)] | |
| #[ignore = "requires PLATFORM_WALLET_E2E_BANK_MNEMONIC and live testnet access"] | |
| async fn transfer_between_two_platform_addresses() { |
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/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`.
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
+39
to
+46
| let sdk = SdkBuilder::new(address_list) | ||
| .with_network(network) | ||
| .with_context_provider(context_provider) | ||
| .build() | ||
| .map_err(|e| { | ||
| tracing::error!(target: "platform_wallet::e2e::sdk", "SdkBuilder::build failed: {e}"); | ||
| FrameworkError::NotImplemented("sdk::build_sdk — SdkBuilder::build failed (see logs)") | ||
| })?; |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: 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.
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-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`.
Comment on lines
+205
to
+208
| fn build_client_config(config: &Config) -> FrameworkResult<ClientConfig> { | ||
| let network = parse_network(&config.network)?; | ||
|
|
||
| let storage_path = config.workdir_base.join("spv-data"); |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: 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.
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/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
+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
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
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
+62
to
+63
| #[tokio_shared_rt::test(shared)] | ||
| async fn transfer_between_two_platform_addresses() { |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: 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.
💡 Suggested change
Suggested change
| #[tokio_shared_rt::test(shared)] | |
| async fn transfer_between_two_platform_addresses() { | |
| #[tokio_shared_rt::test(shared)] | |
| #[ignore = "requires PLATFORM_WALLET_E2E_BANK_MNEMONIC and live testnet access; run with `cargo test -- --ignored`"] | |
| async fn transfer_between_two_platform_addresses() { |
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/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.
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
+32
to
+41
| let sdk = builder | ||
| .with_context_provider(context_provider) | ||
| .build() | ||
| .map_err(|e| { | ||
| tracing::error!(target: "platform_wallet::e2e::sdk", "SdkBuilder::build failed: {e}"); | ||
| FrameworkError::NotImplemented("sdk::build_sdk — SdkBuilder::build failed (see logs)") | ||
| })?; | ||
|
|
||
| Ok(Arc::new(sdk)) | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: 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.
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/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.
Comment on lines
+210
to
+247
| fn build_client_config( | ||
| config: &Config, | ||
| address_list: &AddressList, | ||
| ) -> FrameworkResult<ClientConfig> { | ||
| let network = config.network; | ||
|
|
||
| let storage_path = config.workdir_base.join("spv-data"); | ||
| std::fs::create_dir_all(&storage_path).map_err(|e| { | ||
| tracing::error!( | ||
| target: "platform_wallet::e2e::spv", | ||
| "failed to create SPV storage dir {}: {e}", | ||
| storage_path.display() | ||
| ); | ||
| FrameworkError::NotImplemented( | ||
| "spv::build_client_config — failed to create SPV storage dir (see logs)", | ||
| ) | ||
| })?; | ||
|
|
||
| let mut client_config = ClientConfig::new(network) | ||
| .with_storage_path(storage_path) | ||
| .with_validation_mode(ValidationMode::Full) | ||
| .with_start_height(0) | ||
| .with_mempool_tracking(MempoolStrategy::BloomFilter); | ||
|
|
||
| seed_p2p_peers(&mut client_config, config, address_list); | ||
|
|
||
| client_config.validate().map_err(|e| { | ||
| tracing::error!( | ||
| target: "platform_wallet::e2e::spv", | ||
| "invalid SPV ClientConfig: {e}" | ||
| ); | ||
| FrameworkError::NotImplemented( | ||
| "spv::build_client_config — invalid SPV ClientConfig (see logs)", | ||
| ) | ||
| })?; | ||
|
|
||
| Ok(client_config) | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: 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.
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/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
+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']
…-platform-wallet-e2e
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.
Issue being fixed or feature implemented
rs-platform-wallethad a thin test surface (tests/spv_sync.rs,tests/contact_workflow_tests.rs,tests/thread_safety.rs) and no end-to-end harness exercising the full wallet → SDK → broadcast pipeline. This adds a generalised, future-extractable e2e test framework modelled ondash-evo-tool/tests/backend-e2e/, plus the first test case: passing credits between two platform addresses.Two notable differences from DET:
tests/e2e/(notplatform_e2e/); SPV runtime + ContextProvider modules retained (currently disabled, see below) for re-enablement when SPV stabilises.Per user direction, production code is kept as close to upstream
v3.1-devas possible — only one real bug fix lands insrc/. All test-only convenience accessors (fee_paid,address_derivation_info) were absorbed inside the test framework.What was done?
Production code (single bug fix only)
src/wallet/platform_addresses/transfer.rs—auto_select_inputswas inserting the FULL address balance as the input contribution. The protocol enforcesΣ inputs == Σ outputs(with fee strategy adjusting), so when one input had 500B credits and the requested output was 50M, the balance equation broke. Fix: trim the last selected input torequired - prior_accumulatedso the contribution map matches what the protocol expects. Includes new unit tests covering the trimming behaviour.This bug affected every caller using
InputSelection::Auto— not just our framework.Cargo.toml— dev-dependencies for the e2e framework (tokio_shared_rt,tempfile,dotenvy,bip39,fs2,serde_json,simple-signer,rs-sdk-trusted-context-provider,dash-async,parking_lot,tokio-util,async-trait).git diff origin/v3.1-dev -- packages/rs-platform-wallet/src/shows ONLY the auto_select_inputs fix.Test framework (
packages/rs-platform-wallet/tests/e2e/)framework/harness.rs—E2eContextlazy-init viaOnceCell: workdir slot lock → SDK construction →TrustedHttpContextProvider→PlatformWalletManager→ bank load → registry open → startupcleanup::sweep_orphans.framework/sdk.rs— SDK constructed withTrustedHttpContextProvider::new(network, None, cache_size). OptionalPLATFORM_WALLET_E2E_TRUSTED_CONTEXT_URLenv override. SPV-based context provider commented out and tracked for re-enablement.framework/bank.rs—BankWallet::loadparses BIP-39, syncs, panics with operator-actionable message if under-funded (includes the bank's primary receive address).framework/wallet_factory.rs—TestWallet,SetupGuard(synchronousDropthat warns and defers to startup-sweep).setup()registers wallets in the persistent registry BEFORE returning the guard so panics leave a recoverable trail.framework/signer.rs—SeedBackedPlatformAddressSignerwith eager DIP-17 key cache (derives0..GAP_LIMITkeys at construction;can_sign_withis honest, no async wallet round-trip).framework/registry.rs—PersistentTestWalletRegistry, JSON-backed at<workdir>/test_wallets.json, atomic write-temp-+-rename.framework/cleanup.rs—sweep_orphans(startup) +teardown_one(per-test). UsesInputSelection::Explicit(full_balances)to bypassauto_select_inputstrim semantics; reservesSWEEP_FEE_ESTIMATE = 30Mon a fee-bearer input (covers 1-3 input sweeps based on observed testnet fees).framework/wait_hub.rs—WaitEventHubimplementingPlatformEventHandler, lock-freetokio::sync::Notify-based event bus integration.wait_for_balanceis event-driven (no fixed polling).framework/workdir.rs—flock-based slot fallback (DET pattern, up to 10 slots) for cross-process safety.framework/context_provider.rs—SpvContextProviderusingdash_async::block_on(runtime-flavour-agnostic). Currently disabled; module retained for re-enablement.framework/spv.rs— SPV start +wait_for_mn_list_syncedwith 600s timeout floor matchingtests/spv_sync.rsprecedent. Currently disabled.cases/transfer.rs— the first test:addr_1with 50M; test wallet self-transfers 10M toaddr_2; assertions on balances; explicit teardown sweeps remaining funds back to bank.Documentation
tests/e2e/README.md— operator setup, env-var table, bank pre-funding, multi-process safety, panic-safe cleanup, troubleshooting, future Core support.How Has This Been Tested?
cargo check --tests -p platform-wallet— cleancargo clippy --tests -p platform-wallet -- -D warnings— cleancargo fmt -p platform-wallet— appliedaddr_1with 50M (2.3s confirmation)addr_1→addr_2(202ms confirmation)funded=50M received=10M remaining=30755720 fee=9244280✓startup sweep recovered orphan wallets from prior runs count=N).docs/private notes.Breaking Changes
None.
Outstanding (deferred)
SWEEP_FEE_ESTIMATE = 30Mis a constant calibrated against observed testnet fees (1-input ~9.5M, 2-input ~21M). Long-term: derive fromtransfer::estimate_fee_for_inputs(currently private) — needs a small public helper.Checklist
For repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
Release Notes
New Features
Tests
Documentation