test(platform-wallet): integration test framework + first transfer test#3549
test(platform-wallet): integration test framework + first transfer test#3549Claudius-Maginificent wants to merge 59 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>
|
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>
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.
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.
|
✅ Review complete (commit 921833f) |
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>
…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>
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.
| let inputs: BTreeMap<PlatformAddress, Credits> = wallet | ||
| .platform() | ||
| .addresses_with_balances() | ||
| .await | ||
| .into_iter() | ||
| .filter(|(_, b)| *b > 0) | ||
| .collect(); | ||
| if inputs.is_empty() { | ||
| return Ok(()); | ||
| } |
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.
| /// 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; |
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).
| 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())?; |
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`.
| /// 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) | ||
| } |
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`).
| /// 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(); |
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']
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Default, Serialize, Deserialize)] | ||
| pub enum EntryStatus { | ||
| #[default] | ||
| Active, | ||
| Sweeping, | ||
| Failed, | ||
| } |
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
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.
| let inputs: BTreeMap<PlatformAddress, Credits> = wallet | ||
| .platform() | ||
| .addresses_with_balances() | ||
| .await | ||
| .into_iter() | ||
| .filter(|(_, b)| *b > 0) | ||
| .collect(); | ||
| if inputs.is_empty() { | ||
| return Ok(()); | ||
| } |
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
| 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).
| 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" | ||
| ); | ||
| } |
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.
| /// 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) | ||
| } |
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.
| 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(()) | ||
| } |
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']
| /// 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(); |
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']
| /// 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) | ||
| } |
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
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Test-only PR adding an e2e harness for rs-platform-wallet plus a small production surface (auto_select_inputs fix, simple-signer derive feature, two pub-visibility bumps). One blocking issue: the live testnet e2e test had its #[ignore] removed but the CI workflow runs platform-wallet --all-features with no env wiring or filter, so it will panic in every CI run. Several smaller architecture / robustness concerns in the framework and unused public-API surface.
Reviewed commit: aad27c5
🔴 1 blocking | 🟡 5 suggestion(s) | 💬 3 nitpick(s)
1 additional finding
💬 nitpick: Inconsistent invariant guarding: debug_assert + runtime check here, debug_assert only in sibling helper
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs (lines 343-360)
select_inputs_deduct_from_input is private and its only caller (auto_select_inputs) has already pattern-matched the strategy before dispatching here. The function still re-checks the same invariant twice — a debug_assert! (343-350) followed by a runtime if !matches!(...) (351-360) returning an error string referencing an internal function name. The companion select_inputs_reduce_output (570-574) keeps only the debug_assert!. Pick one pattern for private invariant guards and apply it consistently.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/tests/e2e/cases/transfer.rs`:
- [BLOCKING] lines 62-63: Live testnet e2e test will panic in CI: #[ignore] removed but workflow runs platform-wallet --all-features with no env wiring or test filter
`transfer_between_two_platform_addresses` is no longer `#[ignore]`. `.github/workflows/tests-rs-workspace.yml` (lines 144-171 and 308-335) runs `cargo nextest --package platform-wallet --all-features --locked` with only an `-E 'not test(~shield)'` filter — no env wiring for `PLATFORM_WALLET_E2E_BANK_MNEMONIC`, no exclusion of the `e2e` test binary, and no `offline-testing`-style feature gate on platform-wallet. Without the env var, `Config::from_env()` returns `FrameworkError::Bank("PLATFORM_WALLET_E2E_BANK_MNEMONIC not set ...")`, `setup().await.expect("e2e setup failed")` panics, and CI fails on every run. Either restore `#[ignore]` until the workflow is updated, or land the workflow change (filter + env wiring) in this PR.
In `packages/rs-platform-wallet/tests/e2e/framework/cleanup.rs`:
- [SUGGESTION] lines 217-223: sweep_platform_addresses includes dust inputs the protocol will reject
`sweep_platform_addresses` collects every address with `balance > 0` and feeds the full map into `InputSelection::Explicit`. The DPP `address_funds_transfer_transition/v0/state_transition_validation.rs:159` rejects any input `< min_input_amount`. The wallet-level gates at lines 113-114 and 154-155 only check `total >= min_input_amount`, not per-address balance, so a wallet with one spendable address plus any sub-minimum dust address (easy to produce once `ReduceOutput(0)` leaves remainders or future tests do partial spends) will fail teardown forever, leaving the registry entry behind. The current single test happens to leave both addresses well above min, but the framework is meant to generalize. Filter individual balances against `min_input_amount` instead of relying on the total gate.
In `packages/rs-sdk/src/platform/transition.rs`:
- [SUGGESTION] line 3: address_inputs promoted to pub with no external consumer
`address_inputs` (and `fetch_inputs_with_nonce` / `nonce_inc`) flipped from `pub(crate)` to `pub`, but every caller in the workspace is still inside rs-sdk itself (`transfer_address_funds.rs`, `address_credit_withdrawal.rs`, `top_up_identity_from_addresses.rs`, `put_identity.rs`, `shield.rs`). The e2e framework added in this PR does not call either function. The signatures expose internal SDK types (`dpp::AddressNonce`, `drive_proof_verifier::types::AddressInfos`, `BTreeMap<PlatformAddress, ...>`) and once `pub`, downgrading is a breaking change. Either land a justified external consumer alongside the visibility bump or keep these `pub(crate)`.
In `packages/simple-signer/src/signer.rs`:
- [SUGGESTION] lines 197-241: from_seed_for_identity is unused in this PR and has a misleading contract
`from_seed_for_identity` populates `self.address_private_keys` (keyed on pubkey-hash, used by `Signer<PlatformAddress>` at lines 379-385) but does not populate `self.private_keys: BTreeMap<IdentityPublicKey, [u8; 32]>`. Per the impl at lines 247-258, `Signer<IdentityPublicKey>::sign` reads from `private_keys` only, so the returned signer cannot satisfy that trait despite the function name. The doc-comment honestly admits callers must additionally call `add_identity_public_key`, but the e2e framework only uses `from_seed_for_platform_address_account`; nothing in the PR consumes `from_seed_for_identity`. Either drop it until a real consumer lands or rename to reflect that it populates the address-signing path (e.g. `from_seed_for_identity_authentication_addresses`) so future callers don't expect a turnkey `Signer<IdentityPublicKey>`.
In `packages/rs-platform-wallet/tests/e2e/framework/sdk.rs`:
- [SUGGESTION] lines 35-38: FrameworkError::NotImplemented misused as a generic error envelope; underlying cause is dropped
`SdkBuilder::build` (and several sibling sites in sdk.rs and spv.rs) wrap a real runtime failure in `FrameworkError::NotImplemented`, whose `Display` reads "e2e framework not yet implemented: ...". The actual error is logged at error-level then discarded. Operators reading test output will see a misleading "not implemented" message when SDK construction in fact failed at runtime, and downstream `Result` matching cannot recover the cause. Add a dedicated `Sdk(String)` (and `Spv(String)`) variant or carry the source via `#[source] Box<dyn Error + Send + Sync>` so the chain survives.
In `packages/rs-platform-wallet/tests/e2e/framework/registry.rs`:
- [SUGGESTION] lines 103-132: Registry mutates in-memory state before the JSON write succeeds
`insert`, `remove`, and `set_status` all lock, mutate `self.state`, clone the snapshot, drop the lock, and only then call `atomic_write_json`. If the write fails, the method returns `Err` but the in-memory map has already changed. That violates the module's own "persist before returning" contract: an `insert` failure leaves an in-memory orphan with no disk record (next-run sweep won't see it), and a `remove` failure forgets the entry in memory while the disk entry persists. Build the snapshot first, persist it, then swap it into `self.state` only after the write succeeds.
| let inputs: BTreeMap<PlatformAddress, Credits> = wallet | ||
| .platform() | ||
| .addresses_with_balances() | ||
| .await | ||
| .into_iter() | ||
| .filter(|(_, b)| *b > 0) | ||
| .collect(); |
There was a problem hiding this comment.
🟡 Suggestion: sweep_platform_addresses includes dust inputs the protocol will reject
sweep_platform_addresses collects every address with balance > 0 and feeds the full map into InputSelection::Explicit. The DPP address_funds_transfer_transition/v0/state_transition_validation.rs:159 rejects any input < min_input_amount. The wallet-level gates at lines 113-114 and 154-155 only check total >= min_input_amount, not per-address balance, so a wallet with one spendable address plus any sub-minimum dust address (easy to produce once ReduceOutput(0) leaves remainders or future tests do partial spends) will fail teardown forever, leaving the registry entry behind. The current single test happens to leave both addresses well above min, but the framework is meant to generalize. Filter individual balances against min_input_amount instead of relying on the total gate.
💡 Suggested change
| let inputs: BTreeMap<PlatformAddress, Credits> = wallet | |
| .platform() | |
| .addresses_with_balances() | |
| .await | |
| .into_iter() | |
| .filter(|(_, b)| *b > 0) | |
| .collect(); | |
| let min_input = PlatformVersion::latest() | |
| .dpp | |
| .state_transitions | |
| .address_funds | |
| .min_input_amount; | |
| let inputs: BTreeMap<PlatformAddress, Credits> = wallet | |
| .platform() | |
| .addresses_with_balances() | |
| .await | |
| .into_iter() | |
| .filter(|(_, b)| *b >= min_input) | |
| .collect(); |
source: ['codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/tests/e2e/framework/cleanup.rs`:
- [SUGGESTION] lines 217-223: sweep_platform_addresses includes dust inputs the protocol will reject
`sweep_platform_addresses` collects every address with `balance > 0` and feeds the full map into `InputSelection::Explicit`. The DPP `address_funds_transfer_transition/v0/state_transition_validation.rs:159` rejects any input `< min_input_amount`. The wallet-level gates at lines 113-114 and 154-155 only check `total >= min_input_amount`, not per-address balance, so a wallet with one spendable address plus any sub-minimum dust address (easy to produce once `ReduceOutput(0)` leaves remainders or future tests do partial spends) will fail teardown forever, leaving the registry entry behind. The current single test happens to leave both addresses well above min, but the framework is meant to generalize. Filter individual balances against `min_input_amount` instead of relying on the total gate.
| //! State transitions used to put changed objects to the Dash Platform. | ||
| pub mod address_credit_withdrawal; | ||
| pub(crate) mod address_inputs; | ||
| pub mod address_inputs; |
There was a problem hiding this comment.
🟡 Suggestion: address_inputs promoted to pub with no external consumer
address_inputs (and fetch_inputs_with_nonce / nonce_inc) flipped from pub(crate) to pub, but every caller in the workspace is still inside rs-sdk itself (transfer_address_funds.rs, address_credit_withdrawal.rs, top_up_identity_from_addresses.rs, put_identity.rs, shield.rs). The e2e framework added in this PR does not call either function. The signatures expose internal SDK types (dpp::AddressNonce, drive_proof_verifier::types::AddressInfos, BTreeMap<PlatformAddress, ...>) and once pub, downgrading is a breaking change. Either land a justified external consumer alongside the visibility bump or keep these pub(crate).
💡 Suggested change
| pub mod address_inputs; | |
| pub(crate) mod address_inputs; |
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk/src/platform/transition.rs`:
- [SUGGESTION] line 3: address_inputs promoted to pub with no external consumer
`address_inputs` (and `fetch_inputs_with_nonce` / `nonce_inc`) flipped from `pub(crate)` to `pub`, but every caller in the workspace is still inside rs-sdk itself (`transfer_address_funds.rs`, `address_credit_withdrawal.rs`, `top_up_identity_from_addresses.rs`, `put_identity.rs`, `shield.rs`). The e2e framework added in this PR does not call either function. The signatures expose internal SDK types (`dpp::AddressNonce`, `drive_proof_verifier::types::AddressInfos`, `BTreeMap<PlatformAddress, ...>`) and once `pub`, downgrading is a breaking change. Either land a justified external consumer alongside the visibility bump or keep these `pub(crate)`.
| /// 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) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: from_seed_for_identity is unused in this PR and has a misleading contract
from_seed_for_identity populates self.address_private_keys (keyed on pubkey-hash, used by Signer<PlatformAddress> at lines 379-385) but does not populate self.private_keys: BTreeMap<IdentityPublicKey, [u8; 32]>. Per the impl at lines 247-258, Signer<IdentityPublicKey>::sign reads from private_keys only, so the returned signer cannot satisfy that trait despite the function name. The doc-comment honestly admits callers must additionally call add_identity_public_key, but the e2e framework only uses from_seed_for_platform_address_account; nothing in the PR consumes from_seed_for_identity. Either drop it until a real consumer lands or rename to reflect that it populates the address-signing path (e.g. from_seed_for_identity_authentication_addresses) so future callers don't expect a turnkey Signer<IdentityPublicKey>.
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/simple-signer/src/signer.rs`:
- [SUGGESTION] lines 197-241: from_seed_for_identity is unused in this PR and has a misleading contract
`from_seed_for_identity` populates `self.address_private_keys` (keyed on pubkey-hash, used by `Signer<PlatformAddress>` at lines 379-385) but does not populate `self.private_keys: BTreeMap<IdentityPublicKey, [u8; 32]>`. Per the impl at lines 247-258, `Signer<IdentityPublicKey>::sign` reads from `private_keys` only, so the returned signer cannot satisfy that trait despite the function name. The doc-comment honestly admits callers must additionally call `add_identity_public_key`, but the e2e framework only uses `from_seed_for_platform_address_account`; nothing in the PR consumes `from_seed_for_identity`. Either drop it until a real consumer lands or rename to reflect that it populates the address-signing path (e.g. `from_seed_for_identity_authentication_addresses`) so future callers don't expect a turnkey `Signer<IdentityPublicKey>`.
| const DEFAULT_ACCOUNT_INDEX: u32 = 0; | ||
| const DEFAULT_KEY_CLASS: u32 = 0; |
There was a problem hiding this comment.
💬 Nitpick: DEFAULT_ACCOUNT_INDEX/DEFAULT_KEY_CLASS in mod.rs duplicate wallet_factory's pinned spec without sharing the drift guard
wallet_factory.rs pins DEFAULT_PLATFORM_PAYMENT_ACCOUNT_SPEC from PlatformPaymentAccountSpec::default() and exports DEFAULT_ACCOUNT_INDEX_PUB / DEFAULT_KEY_CLASS_PUB with a drift test. mod.rs:40-41 declares its own DEFAULT_ACCOUNT_INDEX = 0; DEFAULT_KEY_CLASS = 0; and feeds them into make_platform_signer. If PlatformPaymentAccountSpec::default() ever drifts, TestWallet::create (uses WalletAccountCreationOptions::Default) would track the new value while make_platform_signer would still derive 0/0 keys — signer/wallet drift without firing the existing test. Re-export from wallet_factory so there's one source of truth.
source: ['claude']
| fn atomic_write_json( | ||
| path: &Path, | ||
| state: &HashMap<WalletSeedHash, RegistryEntry>, | ||
| ) -> FrameworkResult<()> { | ||
| use std::io::Write; | ||
|
|
||
| let on_disk = encode_keys(state); | ||
| let bytes = serde_json::to_vec_pretty(&on_disk).map_err(|err| { | ||
| FrameworkError::Io(format!("serialising registry to {}: {err}", path.display())) | ||
| })?; | ||
| let parent = path.parent().ok_or_else(|| { | ||
| FrameworkError::Io(format!( | ||
| "registry path {} has no parent directory", | ||
| path.display() | ||
| )) | ||
| })?; | ||
| fs::create_dir_all(parent) | ||
| .map_err(|err| FrameworkError::Io(format!("creating {}: {err}", parent.display())))?; | ||
|
|
||
| // Same-filesystem temp file is required for atomic rename; | ||
| // `persist` (not `persist_noclobber`) overwrites cross-platform. | ||
| let mut tmp = tempfile::NamedTempFile::new_in(parent).map_err(|err| { | ||
| FrameworkError::Io(format!("creating temp file in {}: {err}", parent.display())) | ||
| })?; | ||
| tmp.write_all(&bytes).map_err(|err| { | ||
| FrameworkError::Io(format!("writing temp file {}: {err}", tmp.path().display())) | ||
| })?; | ||
| tmp.as_file_mut().flush().map_err(|err| { | ||
| FrameworkError::Io(format!( | ||
| "flushing temp file {}: {err}", | ||
| tmp.path().display() | ||
| )) | ||
| })?; | ||
| tmp.persist(path).map_err(|err| { | ||
| FrameworkError::Io(format!("persisting temp file -> {}: {err}", path.display())) | ||
| })?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Test wallet seeds persisted to default-permissioned JSON under shared TMPDIR
atomic_write_json writes the registry (containing 64-byte hex seeds for every fresh test wallet) under ${TMPDIR}/dash-platform-wallet-e2e/... with default umask permissions. On Linux/macOS that's typically /tmp and world-readable. On a multi-user runner or shared dev host, a co-located unprivileged user could read seeds between setup and teardown and drain the testnet credits. Impact is bounded (testnet credits, narrow window, operators self-select), but defense-in-depth is cheap: chmod the registry file to 0600 and the slot dir to 0700, or default the workdir base to ${HOME}/.cache/dash-platform-wallet-e2e.
source: ['claude']
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ve test, framework polish Restores `#[ignore]` on `transfer_between_two_platform_addresses` so a stock `cargo test -p platform-wallet --all-features` (the workflow runs heavy nextest with no env wiring) stays green; live runs now opt in via `cargo test -- --ignored`. Adds dedicated `Sdk(String)` / `Spv(String)` variants to `FrameworkError` and routes `SdkBuilder::build` / `TrustedHttpContextProvider` / DAPI parse / SPV storage / mn-list-sync failures through them so the underlying error string survives instead of being swallowed by `NotImplemented(&'static str)`. Plumbs the slot-locked workdir into `spv::start_spv` / `build_client_config` so the deferred SPV runtime tracks the cross-process slot lock instead of sharing `<workdir_base>/spv-data`. Reorders `Registry` mutators (insert / remove / set_status) to persist the JSON snapshot before swapping into `self.state` — a failed write now leaves both memory and disk on the prior state, restoring the module's "persist before returning" contract. README + transfer.rs doc comments updated to reflect the gated default. Addresses thepastaclaw findings on PR #3549: - 03f92b9df0f8 / 0f93a68e9734 / fb5e6b538a41 (BLOCKING — #[ignore] gate) - 5ed6efab6c58 / 06120f3487d4 (Sdk/Spv variants) - 113e838341f5 (slot-locked SPV workdir) - 41049103cb71 (registry persist-before-mutate) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… from PA + identity branches Sources merged: - PR #3571 commits (0186413, 3edff9e, 237f228, 8484d0b): Status standardisation across all non-Found PA entries; PA-003/006/006b assertion tightening; PA-005 trimmed to 4-round implementation with QA-007/008 rationale; PA-004b/004c/009 status pinning. - Trillian local commit fbf268063d (PR #3571 conceptual, local-only): New spec entries Harness-ID-1, ID-001b, ID-003b; Found-020 (PA-001b spec/impl drift pin); extended PA-001 implementation note re #3040 fee ceiling; PA-004c assertion rewrite (Skipped → registry-removed); PA-005b BLOCKED status with production API gap reasoning. - PR #3578 commit 43c24ed: ID-001/002/003/005 upgraded from generic Wave A stubs to Pass entries with test-file citations; ID-004/006 upgraded with detailed deferral reasoning; ID-001c/005b/006b upgraded with P2-deferred rationale. Major additions: - Harness-ID-1 (P0): sweep_identities teardown regression pin. - ID-001b (P1): setup_with_n_identities multi-identity helper spec. - ID-003b (P2): concurrent identity-to-identity transfer nonce serialisation. - ID-004/006/001c/005b/006b: STUB pinnings with deferral reasoning from #3578. - ID-001/002/003/005: Pass status with test-file citations from #3578. - Found-020: PA-001b spec/impl drift documentation. - PA-003/006/006b: assertion tightening. - PA-005: 4-round implementation alignment, rationale for 16→4 reduction. - Status field standardisation across all non-Found PA entries. Counts updated: P0=8, P1=17, P2=53, DEFERRED=1 (79 total). Judgment calls: - PA section: preferred #3571+Trillian over #3578 (more recent and comprehensive — #3578's PA section did not include Status fields or implementation notes). - ID Status strings: preferred #3578's per-test citations for Pass entries over Trillian's generic "Wave A" stubs; merged both (Trillian's new entries + 3578's specific test references) rather than picking one side. - PA-005 scenario: kept 4-round version (#3571) over 16-round (#3578 base) as it reflects the implemented state documented on #3571. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
All previously reported blockers around the live test gate, sweep dust filtering, error propagation, and SPV workdir scoping are resolved at this head. Two correctness issues remain in the new multi-identity helpers — setup_with_n_identities waits for and spends a gross funding amount the bank transfer never delivers, and MultiIdentitySetupGuard::teardown permanently leaks registered-identity credits because sweep_identities is a no-op while the registry entry is dropped. The rest are quality polish: dead error/cancel-token surface, a public Credits field that knowingly misrepresents the chain-time fee, a misleading simple-signer constructor, and rs-sdk nonce internals widened to pub for a single test consumer.
Reviewed commit: 74b1ed7
🔴 1 blocking | 🟡 5 suggestion(s) | 💬 4 nitpick(s)
1 additional finding
🟡 suggestion: Low-level nonce helpers were promoted to the public SDK surface for a single cross-crate test consumer
packages/rs-sdk/src/platform/transition/address_inputs.rs (lines 12-40)
pub mod address_inputs; (transition.rs:3) plus pub fn fetch_inputs_with_nonce and pub fn nonce_inc widen what was previously pub(crate) plumbing into a permanent public-API stability commitment for rs-sdk. The only out-of-crate consumer is transfer_capturing_st_bytes in the e2e framework's wallet_factory.rs:265-280, which uses these in a fetch→increment→sign→broadcast capture for replay-safety tests. The signatures expose internal types (AddressNonce, AddressInfos, BTreeMap<PlatformAddress, ...>); nonce_inc in particular is a footgun outside the strict flow it presumes — a naive consumer that calls it twice or uses the result without broadcasting will desync the address nonce. Prefer a higher-level public helper on the SDK (e.g. build_replayable_transfer_bytes) that owns the fetch/increment/serialize sequence, and revert these to pub(crate). If a test-only export is acceptable, keep them pub(crate) plus #[cfg(any(test, feature = "test-helpers"))] rather than ship them on the stable surface.
🤖 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/framework/mod.rs`:
- [BLOCKING] lines 206-223: `setup_with_n_identities` waits for and spends a gross funding amount that `bank.fund_address` never delivers
`bank.fund_address(&funding_addr, funding_per)` in `bank.rs:162-182` calls `transfer(...)` with `default_fee_strategy()` = `[ReduceOutput(0)]` (`wallet_factory.rs:417-419`), so the chain-time fee is taken out of output #0 — the recipient address receives `funding_per - fee`, not `funding_per`. The next call, `wait_for_balance(&base.test_wallet, &funding_addr, funding_per, 60s)` (`wait.rs:67-124`), polls for `current >= expected`, so on any non-zero-fee network it can never satisfy and will always hit the `Cleanup` timeout. Even relaxing that wait, `register_identity_from_addresses(funding_addr, funding_per, ...)` then asks `register_from_addresses` to consume `funding_per` from `funding_addr`, and the SDK's `fetch_inputs_with_nonce` (rs-sdk `address_inputs.rs:103-115`) returns `AddressNotEnoughFundsError` when `available < required`. Net effect: as currently written, this helper cannot complete against a real network. Fix by either (a) waiting for the actual delivered amount (e.g. compute the post-fee target from `cs.fee` returned by `fund_address`, or use a `<= funding_per` floor that accounts for the static min fee) and registering with that same amount, or (b) topping up by `funding_per + max_expected_fee` and registering with `funding_per`.
- [SUGGESTION] lines 242-263: `MultiIdentitySetupGuard::teardown` guarantees identity credits are leaked once `setup_with_n_identities` is exercised
`MultiIdentitySetupGuard::teardown` simply forwards to `SetupGuard::teardown`, whose cleanup path (`cleanup.rs:146-188`) calls `sweep_identities(...)` and then `registry.remove(&test_wallet.id())`. But `sweep_identities` in `cleanup.rs:336-338` is a stub `Ok(())` (still pending Task #15 / the `#identity-sweep` TODO), so identity-held credits are never recovered. Once the registry entry is removed there is no recovery trail either, so the next process startup's `sweep_orphans` cannot reclaim them. For a framework whose primary safety property is panic-safe fund recovery, shipping a public setup path that spends bank credits to register identities and then permanently forgets the resulting balance is a contract violation — and it compounds across every test using the helper. Either (a) keep the registry entry (don't remove it) when identities still hold balances so the orphan sweep can pick them up later, (b) block teardown with a clear `FrameworkError::Cleanup` until `sweep_identities` is implemented, or (c) gate `setup_with_n_identities` behind a feature flag / `#[ignore]` until identity sweep lands.
In `packages/rs-platform-wallet/tests/e2e/framework/wait.rs`:
- [SUGGESTION] lines 76-122: `wait_for_balance`'s `Notified` future is captured but not registered before the sync — defeats the documented invariant
The doc claims "The `Notified` future is captured BEFORE the sync to avoid dropping a notification that fires mid-sync." That contract is not honored: `let notified = test_wallet.wait_hub().notified(); tokio::pin!(notified);` only constructs and pins the future — Tokio's `Notify::notified` does not register the waiter until first poll (or via `Notified::enable()`). The first poll happens at `tokio::time::timeout(cap, notified.as_mut()).await` AFTER `sync_balances` returns, so any `notify_waiters()` fired by `WaitEventHub` during the sync (`on_sync_event`, `on_wallet_event`, `on_platform_address_sync_completed`) is silently lost. Worst-case wake delay is bounded by `BACKSTOP_WAKE_INTERVAL` (2s), so this is a polish issue rather than a hang risk — but it means `wait_for_balance` is effectively a 2s polling loop on busy chains, not the event-driven helper it documents itself as. Calling `notified.as_mut().enable()` immediately after the `pin!` would honor the docstring's claim.
In `packages/simple-signer/src/signer.rs`:
- [SUGGESTION] lines 197-241: `from_seed_for_identity` is misleadingly named — the resulting `SimpleSigner` cannot sign as a `Signer<IdentityPublicKey>`
`from_seed_for_identity` populates only `address_private_keys` (the 20-byte-keyed map used by `Signer<PlatformAddress>::sign`), but the `Signer<IdentityPublicKey>` impl on `SimpleSigner` (lines 245-291) reads exclusively from `private_keys` / `private_keys_in_creation`. The constructor's name therefore promises an identity signer that the trait impl cannot deliver. The doc hand-waves with "callers must additionally register `IdentityPublicKey` records via `add_identity_public_key`," but `add_identity_public_key` requires the raw private-key bytes that this constructor never exposes. The new e2e harness only works because `SeedBackedIdentitySigner` (`tests/e2e/framework/signer.rs`) bypasses the impl and reaches into `address_private_keys` directly. A downstream caller who reads the name and instantiates this constructor reasonably assumes the returned `SimpleSigner` can sign identity keys — it cannot. Either rename to reflect the actual side effect (e.g. `from_seed_for_identity_authentication_addresses`) or also insert into `private_keys` so the trait impl works out of the box.
In `packages/rs-platform-wallet/src/changeset/changeset.rs`:
- [SUGGESTION] lines 583-645: `PlatformAddressChangeSet::fee` is a public `Credits` field that knowingly misrepresents the on-chain fee, and `Merge` silently sums it
`pub fee: Credits` stores `AddressFundsTransferTransition::estimate_min_fee(...)`, which the accessor's own doc admits is "NOT the actual on-chain fee" — the static `state_transition_min_fees` floor (~6.5M for 1in/1out) is far below the real chain-time debit (~14.94M observed; see #3040). Because the field is public and shares its type with real credit values, callers reaching for `cs.fee` (rather than the doc-laden `estimated_min_fee()` accessor) get the unfiltered footgun. `Merge::merge` then does `self.fee = self.fee.saturating_add(other.fee)`, so a merged changeset's reported fee is "the sum across operations of a number that already lied for one operation," compounding the misrepresentation. Pick one of: (a) wrap in a newtype like `EstimatedMinFee(Credits)` so the type system surfaces the caveat at every call site, (b) rename the field to `static_min_fee_estimate` so accidental consumers can't confuse it with paid fees, or (c) keep it `pub(crate)` until #3040 lands and the value can mean what callers naturally expect.
In `packages/rs-sdk/src/platform/transition/address_inputs.rs`:
- [SUGGESTION] lines 12-40: Low-level nonce helpers were promoted to the public SDK surface for a single cross-crate test consumer
`pub mod address_inputs;` (transition.rs:3) plus `pub fn fetch_inputs_with_nonce` and `pub fn nonce_inc` widen what was previously `pub(crate)` plumbing into a permanent public-API stability commitment for `rs-sdk`. The only out-of-crate consumer is `transfer_capturing_st_bytes` in the e2e framework's `wallet_factory.rs:265-280`, which uses these in a fetch→increment→sign→broadcast capture for replay-safety tests. The signatures expose internal types (`AddressNonce`, `AddressInfos`, `BTreeMap<PlatformAddress, ...>`); `nonce_inc` in particular is a footgun outside the strict flow it presumes — a naive consumer that calls it twice or uses the result without broadcasting will desync the address nonce. Prefer a higher-level public helper on the SDK (e.g. `build_replayable_transfer_bytes`) that owns the fetch/increment/serialize sequence, and revert these to `pub(crate)`. If a test-only export is acceptable, keep them `pub(crate)` plus `#[cfg(any(test, feature = "test-helpers"))]` rather than ship them on the stable surface.
| for identity_index in 0..n { | ||
| let funding_addr = base.test_wallet.next_unused_address().await?; | ||
| base.ctx | ||
| .bank() | ||
| .fund_address(&funding_addr, funding_per) | ||
| .await?; | ||
| wait_for_balance( | ||
| &base.test_wallet, | ||
| &funding_addr, | ||
| funding_per, | ||
| Duration::from_secs(60), | ||
| ) | ||
| .await?; | ||
|
|
||
| let registered = base | ||
| .test_wallet | ||
| .register_identity_from_addresses(funding_addr, funding_per, identity_index) | ||
| .await?; |
There was a problem hiding this comment.
🔴 Blocking: setup_with_n_identities waits for and spends a gross funding amount that bank.fund_address never delivers
bank.fund_address(&funding_addr, funding_per) in bank.rs:162-182 calls transfer(...) with default_fee_strategy() = [ReduceOutput(0)] (wallet_factory.rs:417-419), so the chain-time fee is taken out of output #0 — the recipient address receives funding_per - fee, not funding_per. The next call, wait_for_balance(&base.test_wallet, &funding_addr, funding_per, 60s) (wait.rs:67-124), polls for current >= expected, so on any non-zero-fee network it can never satisfy and will always hit the Cleanup timeout. Even relaxing that wait, register_identity_from_addresses(funding_addr, funding_per, ...) then asks register_from_addresses to consume funding_per from funding_addr, and the SDK's fetch_inputs_with_nonce (rs-sdk address_inputs.rs:103-115) returns AddressNotEnoughFundsError when available < required. Net effect: as currently written, this helper cannot complete against a real network. Fix by either (a) waiting for the actual delivered amount (e.g. compute the post-fee target from cs.fee returned by fund_address, or use a <= funding_per floor that accounts for the static min fee) and registering with that same amount, or (b) topping up by funding_per + max_expected_fee and registering with funding_per.
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/mod.rs`:
- [BLOCKING] lines 206-223: `setup_with_n_identities` waits for and spends a gross funding amount that `bank.fund_address` never delivers
`bank.fund_address(&funding_addr, funding_per)` in `bank.rs:162-182` calls `transfer(...)` with `default_fee_strategy()` = `[ReduceOutput(0)]` (`wallet_factory.rs:417-419`), so the chain-time fee is taken out of output #0 — the recipient address receives `funding_per - fee`, not `funding_per`. The next call, `wait_for_balance(&base.test_wallet, &funding_addr, funding_per, 60s)` (`wait.rs:67-124`), polls for `current >= expected`, so on any non-zero-fee network it can never satisfy and will always hit the `Cleanup` timeout. Even relaxing that wait, `register_identity_from_addresses(funding_addr, funding_per, ...)` then asks `register_from_addresses` to consume `funding_per` from `funding_addr`, and the SDK's `fetch_inputs_with_nonce` (rs-sdk `address_inputs.rs:103-115`) returns `AddressNotEnoughFundsError` when `available < required`. Net effect: as currently written, this helper cannot complete against a real network. Fix by either (a) waiting for the actual delivered amount (e.g. compute the post-fee target from `cs.fee` returned by `fund_address`, or use a `<= funding_per` floor that accounts for the static min fee) and registering with that same amount, or (b) topping up by `funding_per + max_expected_fee` and registering with `funding_per`.
| /// Calling [`MultiIdentitySetupGuard::teardown`] consumes the guard | ||
| /// and forwards to the inner [`SetupGuard::teardown`], which sweeps | ||
| /// platform-address balances. Identity-credit cleanup is deferred to | ||
| /// a follow-up PR — see the `#identity-sweep` TODO in | ||
| /// [`cleanup::sweep_identities`]. Until then, every identity | ||
| /// registered here keeps its post-registration credit balance. | ||
| pub struct MultiIdentitySetupGuard { | ||
| /// Inner single-wallet guard. Holds the [`E2eContext`] and the | ||
| /// shared [`wallet_factory::TestWallet`] every identity is | ||
| /// derived from. | ||
| pub base: SetupGuard, | ||
| /// Identities registered during setup, ordered by DIP-9 index | ||
| /// `0..n`. | ||
| pub identities: Vec<wallet_factory::RegisteredIdentity>, | ||
| } | ||
|
|
||
| impl MultiIdentitySetupGuard { | ||
| /// Forward to the inner [`SetupGuard::teardown`]. | ||
| pub async fn teardown(self) -> FrameworkResult<()> { | ||
| self.base.teardown().await | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: MultiIdentitySetupGuard::teardown guarantees identity credits are leaked once setup_with_n_identities is exercised
MultiIdentitySetupGuard::teardown simply forwards to SetupGuard::teardown, whose cleanup path (cleanup.rs:146-188) calls sweep_identities(...) and then registry.remove(&test_wallet.id()). But sweep_identities in cleanup.rs:336-338 is a stub Ok(()) (still pending Task #15 / the #identity-sweep TODO), so identity-held credits are never recovered. Once the registry entry is removed there is no recovery trail either, so the next process startup's sweep_orphans cannot reclaim them. For a framework whose primary safety property is panic-safe fund recovery, shipping a public setup path that spends bank credits to register identities and then permanently forgets the resulting balance is a contract violation — and it compounds across every test using the helper. Either (a) keep the registry entry (don't remove it) when identities still hold balances so the orphan sweep can pick them up later, (b) block teardown with a clear FrameworkError::Cleanup until sweep_identities is implemented, or (c) gate setup_with_n_identities behind a feature flag / #[ignore] until identity sweep lands.
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/mod.rs`:
- [SUGGESTION] lines 242-263: `MultiIdentitySetupGuard::teardown` guarantees identity credits are leaked once `setup_with_n_identities` is exercised
`MultiIdentitySetupGuard::teardown` simply forwards to `SetupGuard::teardown`, whose cleanup path (`cleanup.rs:146-188`) calls `sweep_identities(...)` and then `registry.remove(&test_wallet.id())`. But `sweep_identities` in `cleanup.rs:336-338` is a stub `Ok(())` (still pending Task #15 / the `#identity-sweep` TODO), so identity-held credits are never recovered. Once the registry entry is removed there is no recovery trail either, so the next process startup's `sweep_orphans` cannot reclaim them. For a framework whose primary safety property is panic-safe fund recovery, shipping a public setup path that spends bank credits to register identities and then permanently forgets the resulting balance is a contract violation — and it compounds across every test using the helper. Either (a) keep the registry entry (don't remove it) when identities still hold balances so the orphan sweep can pick them up later, (b) block teardown with a clear `FrameworkError::Cleanup` until `sweep_identities` is implemented, or (c) gate `setup_with_n_identities` behind a feature flag / `#[ignore]` until identity sweep lands.
| loop { | ||
| // Capture `Notified` BEFORE the sync so a notification | ||
| // arriving mid-sync isn't lost; pin + `as_mut()` lets us | ||
| // re-await the same future across timeouts. | ||
| let notified = test_wallet.wait_hub().notified(); | ||
| tokio::pin!(notified); | ||
|
|
||
| match test_wallet.sync_balances().await { | ||
| Ok(()) => { | ||
| let balances = test_wallet.balances().await; | ||
| let current = balances.get(addr).copied().unwrap_or(0); | ||
| if current >= expected { | ||
| tracing::info!( | ||
| target: "platform_wallet::e2e::wait", | ||
| addr = ?addr, | ||
| observed = current, | ||
| elapsed = ?start.elapsed(), | ||
| "balance reached target" | ||
| ); | ||
| return Ok(()); | ||
| } | ||
| tracing::debug!( | ||
| target: "platform_wallet::e2e::wait", | ||
| addr = ?addr, | ||
| current, | ||
| expected, | ||
| "balance below target; waiting on event hub" | ||
| ); | ||
| } | ||
| Err(err) => tracing::debug!( | ||
| target: "platform_wallet::e2e::wait", | ||
| error = %err, | ||
| "sync_balances during wait_for_balance failed; retrying" | ||
| ), | ||
| } | ||
|
|
||
| let remaining = deadline.saturating_duration_since(Instant::now()); | ||
| if remaining.is_zero() { | ||
| return Err(FrameworkError::Cleanup(format!( | ||
| "wait_for_balance timed out after {timeout:?} \ | ||
| (addr={addr:?} expected={expected})" | ||
| ))); | ||
| } | ||
| // Backstop wake on idle chains; real activity wakes us | ||
| // earlier via the `Notified` future. | ||
| let cap = std::cmp::min(remaining, BACKSTOP_WAKE_INTERVAL); | ||
| let _ = tokio::time::timeout(cap, notified.as_mut()).await; |
There was a problem hiding this comment.
🟡 Suggestion: wait_for_balance's Notified future is captured but not registered before the sync — defeats the documented invariant
The doc claims "The Notified future is captured BEFORE the sync to avoid dropping a notification that fires mid-sync." That contract is not honored: let notified = test_wallet.wait_hub().notified(); tokio::pin!(notified); only constructs and pins the future — Tokio's Notify::notified does not register the waiter until first poll (or via Notified::enable()). The first poll happens at tokio::time::timeout(cap, notified.as_mut()).await AFTER sync_balances returns, so any notify_waiters() fired by WaitEventHub during the sync (on_sync_event, on_wallet_event, on_platform_address_sync_completed) is silently lost. Worst-case wake delay is bounded by BACKSTOP_WAKE_INTERVAL (2s), so this is a polish issue rather than a hang risk — but it means wait_for_balance is effectively a 2s polling loop on busy chains, not the event-driven helper it documents itself as. Calling notified.as_mut().enable() immediately after the pin! would honor the docstring's claim.
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/wait.rs`:
- [SUGGESTION] lines 76-122: `wait_for_balance`'s `Notified` future is captured but not registered before the sync — defeats the documented invariant
The doc claims "The `Notified` future is captured BEFORE the sync to avoid dropping a notification that fires mid-sync." That contract is not honored: `let notified = test_wallet.wait_hub().notified(); tokio::pin!(notified);` only constructs and pins the future — Tokio's `Notify::notified` does not register the waiter until first poll (or via `Notified::enable()`). The first poll happens at `tokio::time::timeout(cap, notified.as_mut()).await` AFTER `sync_balances` returns, so any `notify_waiters()` fired by `WaitEventHub` during the sync (`on_sync_event`, `on_wallet_event`, `on_platform_address_sync_completed`) is silently lost. Worst-case wake delay is bounded by `BACKSTOP_WAKE_INTERVAL` (2s), so this is a polish issue rather than a hang risk — but it means `wait_for_balance` is effectively a 2s polling loop on busy chains, not the event-driven helper it documents itself as. Calling `notified.as_mut().enable()` immediately after the `pin!` would honor the docstring's claim.
| /// 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) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: from_seed_for_identity is misleadingly named — the resulting SimpleSigner cannot sign as a Signer<IdentityPublicKey>
from_seed_for_identity populates only address_private_keys (the 20-byte-keyed map used by Signer<PlatformAddress>::sign), but the Signer<IdentityPublicKey> impl on SimpleSigner (lines 245-291) reads exclusively from private_keys / private_keys_in_creation. The constructor's name therefore promises an identity signer that the trait impl cannot deliver. The doc hand-waves with "callers must additionally register IdentityPublicKey records via add_identity_public_key," but add_identity_public_key requires the raw private-key bytes that this constructor never exposes. The new e2e harness only works because SeedBackedIdentitySigner (tests/e2e/framework/signer.rs) bypasses the impl and reaches into address_private_keys directly. A downstream caller who reads the name and instantiates this constructor reasonably assumes the returned SimpleSigner can sign identity keys — it cannot. Either rename to reflect the actual side effect (e.g. from_seed_for_identity_authentication_addresses) or also insert into private_keys so the trait impl works out of the box.
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 — the resulting `SimpleSigner` cannot sign as a `Signer<IdentityPublicKey>`
`from_seed_for_identity` populates only `address_private_keys` (the 20-byte-keyed map used by `Signer<PlatformAddress>::sign`), but the `Signer<IdentityPublicKey>` impl on `SimpleSigner` (lines 245-291) reads exclusively from `private_keys` / `private_keys_in_creation`. The constructor's name therefore promises an identity signer that the trait impl cannot deliver. The doc hand-waves with "callers must additionally register `IdentityPublicKey` records via `add_identity_public_key`," but `add_identity_public_key` requires the raw private-key bytes that this constructor never exposes. The new e2e harness only works because `SeedBackedIdentitySigner` (`tests/e2e/framework/signer.rs`) bypasses the impl and reaches into `address_private_keys` directly. A downstream caller who reads the name and instantiates this constructor reasonably assumes the returned `SimpleSigner` can sign identity keys — it cannot. Either rename to reflect the actual side effect (e.g. `from_seed_for_identity_authentication_addresses`) or also insert into `private_keys` so the trait impl works out of the box.
| @@ -606,13 +636,20 @@ impl Merge for PlatformAddressChangeSet { | |||
| .map_or(r, |existing| existing.max(r)), | |||
| ); | |||
| } | |||
| // Fee: append-sum via `saturating_add`. Sync-only merges | |||
| // (`fee == 0`) are a no-op so a transfer's recorded fee | |||
| // survives untouched; merging two transfer changesets sums | |||
| // the per-operation fees so the merged total reflects the | |||
| // "total fee paid across operations in this batch" intent. | |||
| self.fee = self.fee.saturating_add(other.fee); | |||
| } | |||
There was a problem hiding this comment.
🟡 Suggestion: PlatformAddressChangeSet::fee is a public Credits field that knowingly misrepresents the on-chain fee, and Merge silently sums it
pub fee: Credits stores AddressFundsTransferTransition::estimate_min_fee(...), which the accessor's own doc admits is "NOT the actual on-chain fee" — the static state_transition_min_fees floor (~6.5M for 1in/1out) is far below the real chain-time debit (~14.94M observed; see #3040). Because the field is public and shares its type with real credit values, callers reaching for cs.fee (rather than the doc-laden estimated_min_fee() accessor) get the unfiltered footgun. Merge::merge then does self.fee = self.fee.saturating_add(other.fee), so a merged changeset's reported fee is "the sum across operations of a number that already lied for one operation," compounding the misrepresentation. Pick one of: (a) wrap in a newtype like EstimatedMinFee(Credits) so the type system surfaces the caveat at every call site, (b) rename the field to static_min_fee_estimate so accidental consumers can't confuse it with paid fees, or (c) keep it pub(crate) until #3040 lands and the value can mean what callers naturally expect.
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/src/changeset/changeset.rs`:
- [SUGGESTION] lines 583-645: `PlatformAddressChangeSet::fee` is a public `Credits` field that knowingly misrepresents the on-chain fee, and `Merge` silently sums it
`pub fee: Credits` stores `AddressFundsTransferTransition::estimate_min_fee(...)`, which the accessor's own doc admits is "NOT the actual on-chain fee" — the static `state_transition_min_fees` floor (~6.5M for 1in/1out) is far below the real chain-time debit (~14.94M observed; see #3040). Because the field is public and shares its type with real credit values, callers reaching for `cs.fee` (rather than the doc-laden `estimated_min_fee()` accessor) get the unfiltered footgun. `Merge::merge` then does `self.fee = self.fee.saturating_add(other.fee)`, so a merged changeset's reported fee is "the sum across operations of a number that already lied for one operation," compounding the misrepresentation. Pick one of: (a) wrap in a newtype like `EstimatedMinFee(Credits)` so the type system surfaces the caveat at every call site, (b) rename the field to `static_min_fee_estimate` so accidental consumers can't confuse it with paid fees, or (c) keep it `pub(crate)` until #3040 lands and the value can mean what callers naturally expect.
| /// DIP-17 default account / key-class for clear-funds platform | ||
| /// payments. Matches `WalletAccountCreationOptions::Default`. | ||
| const DEFAULT_ACCOUNT_INDEX: u32 = 0; | ||
| const DEFAULT_KEY_CLASS: u32 = 0; | ||
|
|
||
| /// Build a [`SimpleSigner`] populated with the DIP-17 platform-payment | ||
| /// gap window for `seed_bytes` on `network`. Pins to | ||
| /// `account=0`/`key_class=0` to match | ||
| /// `WalletAccountCreationOptions::Default`. `SimpleSigner` already | ||
| /// implements `Signer<PlatformAddress>` directly, so callers can pass | ||
| /// the returned value straight to `PlatformAddressWallet::transfer`. | ||
| pub(super) fn make_platform_signer( | ||
| seed_bytes: &[u8; 64], | ||
| network: Network, | ||
| ) -> FrameworkResult<SimpleSigner> { | ||
| SimpleSigner::from_seed_for_platform_address_account( | ||
| seed_bytes, | ||
| network, | ||
| DEFAULT_ACCOUNT_INDEX, | ||
| DEFAULT_KEY_CLASS, | ||
| DIP17_GAP_LIMIT, | ||
| ) | ||
| .map_err(|err| FrameworkError::Wallet(format!("simple-signer: {err}"))) | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: make_platform_signer duplicates the DIP-17 default account/key-class constants instead of reusing the framework's tested source of truth
framework/mod.rs declares its own DEFAULT_ACCOUNT_INDEX = 0 / DEFAULT_KEY_CLASS = 0 and uses them in make_platform_signer, while framework/wallet_factory.rs:43-50 already exposes DEFAULT_ACCOUNT_INDEX_PUB / DEFAULT_KEY_CLASS_PUB derived from DEFAULT_PLATFORM_PAYMENT_ACCOUNT_SPEC and a drift test (wallet_factory.rs:651-653) asserting they match PlatformPaymentAccountSpec::default(). If the upstream default changes, every wallet-creation and transfer path that imports the _PUB constants will track the new value, but make_platform_signer will keep deriving keys for 0/0 — and the existing drift test won't catch the mismatch because it doesn't reference the local copies here. Reuse the exported constants from wallet_factory to keep signer derivation and wallet construction on a single source of truth.
💡 Suggested change
| /// DIP-17 default account / key-class for clear-funds platform | |
| /// payments. Matches `WalletAccountCreationOptions::Default`. | |
| const DEFAULT_ACCOUNT_INDEX: u32 = 0; | |
| const DEFAULT_KEY_CLASS: u32 = 0; | |
| /// Build a [`SimpleSigner`] populated with the DIP-17 platform-payment | |
| /// gap window for `seed_bytes` on `network`. Pins to | |
| /// `account=0`/`key_class=0` to match | |
| /// `WalletAccountCreationOptions::Default`. `SimpleSigner` already | |
| /// implements `Signer<PlatformAddress>` directly, so callers can pass | |
| /// the returned value straight to `PlatformAddressWallet::transfer`. | |
| pub(super) fn make_platform_signer( | |
| seed_bytes: &[u8; 64], | |
| network: Network, | |
| ) -> FrameworkResult<SimpleSigner> { | |
| SimpleSigner::from_seed_for_platform_address_account( | |
| seed_bytes, | |
| network, | |
| DEFAULT_ACCOUNT_INDEX, | |
| DEFAULT_KEY_CLASS, | |
| DIP17_GAP_LIMIT, | |
| ) | |
| .map_err(|err| FrameworkError::Wallet(format!("simple-signer: {err}"))) | |
| } | |
| use key_wallet::gap_limit::DIP17_GAP_LIMIT; | |
| use key_wallet::Network; | |
| use simple_signer::signer::SimpleSigner; | |
| use self::wallet_factory::{DEFAULT_ACCOUNT_INDEX_PUB, DEFAULT_KEY_CLASS_PUB}; | |
| /// Build a [`SimpleSigner`] populated with the DIP-17 platform-payment | |
| /// gap window for `seed_bytes` on `network`. Pins to | |
| /// `account=0`/`key_class=0` to match | |
| /// `WalletAccountCreationOptions::Default`. `SimpleSigner` already | |
| /// implements `Signer<PlatformAddress>` directly, so callers can pass | |
| /// the returned value straight to `PlatformAddressWallet::transfer`. | |
| pub(super) fn make_platform_signer( | |
| seed_bytes: &[u8; 64], | |
| network: Network, | |
| ) -> FrameworkResult<SimpleSigner> { | |
| SimpleSigner::from_seed_for_platform_address_account( | |
| seed_bytes, | |
| network, | |
| DEFAULT_ACCOUNT_INDEX_PUB, | |
| DEFAULT_KEY_CLASS_PUB, | |
| DIP17_GAP_LIMIT, | |
| ) | |
| .map_err(|err| FrameworkError::Wallet(format!("simple-signer: {err}"))) | |
| } |
source: ['codex']
| /// Placeholder returned by paths that surface an underlying | ||
| /// error through tracing; the static string names the call site. | ||
| #[error("e2e framework not yet implemented: {0}")] | ||
| NotImplemented(&'static str), |
There was a problem hiding this comment.
💬 Nitpick: FrameworkError::NotImplemented is dead — every prior call site migrated to typed Sdk / Spv variants
NotImplemented(&'static str) was previously the catch-all envelope in framework/sdk.rs and framework/spv.rs for SDK construction and SPV runtime failures. Both files now use FrameworkError::Sdk(String) / Spv(String), and a repo-wide grep confirms no producer or consumer of NotImplemented remains in the workspace. Carrying a dead variant lowers the signal in the error enum and tempts the next contributor to reach for it again as a generic placeholder when SPV (Task #15) re-lands. Drop it; if a real "feature deferred" path appears later, prefer a named typed variant.
source: ['claude', 'codex']
| /// 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(); |
There was a problem hiding this comment.
💬 Nitpick: E2eContext::cancel_token is constructed and exposed but never observed
cancel_token is created at line 109, exposed via the cancel_token() accessor at line 96, and the doc claims it backs "graceful shutdown" of background helpers. A repo-wide grep across packages/rs-platform-wallet/tests/ shows no .cancel(), no .cancelled(), and no tokio::select! arm referencing it — wait_for_balance, wait_for_identity_balance, wait_for_dpns_name_visible, the deferred SPV blocks, and the test bodies all ignore it. The token is dead state with a forward-looking accessor that tempts misuse (a future contributor may assume it actually halts background work). Either drop the field until shutdown wiring lands (Task #15) or wire one observer in (e.g., a tokio::select! arm in wait::wait_for_balance) so the documented behavior actually fires.
source: ['claude', 'codex']
| /// Read the current incremental-sync watermark from the unified | ||
| /// platform-address provider. | ||
| /// | ||
| /// Returns `None` when the provider hasn't been initialised yet | ||
| /// (no [`Self::initialize`] call) or when the provider has no stored | ||
| /// watermark (whether restored via [`Self::apply_sync_state`] or | ||
| /// produced by a previous sync). The value is monotonic non-decreasing | ||
| /// across [`Self::sync_balances`](super::sync) calls against the | ||
| /// same chain — a later sync can only advance the watermark, never | ||
| /// roll it back. A zero-valued watermark is reported as `None` to | ||
| /// match the "no stored watermark" convention used elsewhere in | ||
| /// the wallet (see [`Self::apply_sync_state`]). | ||
| pub async fn sync_watermark(&self) -> Option<u64> { | ||
| let guard = self.provider.read().await; | ||
| let raw = guard.as_ref().map(|p| p.last_known_recent_block())?; | ||
| if raw == 0 { | ||
| None | ||
| } else { | ||
| Some(raw) | ||
| } | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: sync_watermark collapses "block 0" and "no watermark stored" into the same None
sync_watermark returns None whenever last_known_recent_block() == 0. The doc claim that this matches the "no stored watermark" convention is reasonable on freshly-restored mainnet/testnet wallets, but breaks down for any chain where block 0 is a legitimate genesis-aligned watermark (devnet/regtest, or any test fixture that explicitly sets last_known_recent_block to 0 during setup). After a sync that legitimately lands at height 0, the next call would still report "never synced." If the underlying intent is "distinguish unset from set," make the storage layer return Option<u64> end-to-end so the boundary stays honest. As-is, the zero-collapse is a quiet correctness footgun for non-mainnet chains.
source: ['claude']
…A-001b] (#3579) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…-platform-wallet-e2e
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
All carry-forward findings validated at HEAD 921833f. The prior-blocking setup_with_n_identities funding mismatch is fixed via the bank's [DeductFromInput(0)] strategy. Remaining items are suggestions and nitpicks: a latent identity-credit teardown leak (sweep_identities is a no-op stub but the registry entry is still removed), a wait_for_balance Notify waiter that's never enabled before sync, several public API surface concerns (from_seed_for_identity, PlatformAddressChangeSet::fee, rs-sdk nonce helpers), and a few dead-code/duplication nits in the e2e harness.
Reviewed commit: 921833f
🟡 5 suggestion(s) | 💬 4 nitpick(s)
1 additional finding
🟡 suggestion: Low-level nonce helpers were promoted to the public rs-sdk surface for a single cross-crate test consumer
packages/rs-sdk/src/platform/transition/address_inputs.rs (lines 12-40)
Confirmed at HEAD. pub mod address_inputs (transition.rs:3) plus pub fn fetch_inputs_with_nonce (address_inputs.rs:12) and pub fn nonce_inc (address_inputs.rs:34) widen what was previously pub(crate) plumbing into a permanent public-API stability commitment for rs-sdk. They are used internally by put_identity / transfer_address_funds / shield / top_up_identity_from_addresses / address_credit_withdrawal, but pub(crate) would suffice for those. The only out-of-crate consumer is transfer_capturing_st_bytes in tests/e2e/framework/wallet_factory.rs:265-280 (a fetch→increment→sign→broadcast capture for replay tests). The signatures expose internal types (AddressNonce, AddressInfos, BTreeMap<PlatformAddress, ...>); nonce_inc is a footgun outside the strict flow it presumes — incrementing twice or without broadcasting desyncs the address nonce. Prefer a higher-level public helper (e.g. build_replayable_transfer_bytes) that owns the fetch/increment/serialize sequence and revert these to pub(crate). If a test-only export is acceptable, gate them behind #[cfg(any(test, feature = "test-helpers"))].
🤖 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/framework/cleanup.rs`:
- [SUGGESTION] lines 171-188: MultiIdentitySetupGuard::teardown silently burns identity credits — sweep_identities is a no-op while the registry entry is unconditionally removed
Confirmed at HEAD. `setup_with_n_identities` (framework/mod.rs:190-237) registers identities funded from the bank, but the teardown path forwards to `cleanup::teardown_one`, which calls `sweep_identities` (cleanup.rs:336-338, an explicit `Ok(())` stub gated by `#identity-sweep`) and then unconditionally drops the registry entry at cleanup.rs:178 (`registry.remove(&test_wallet.id())?`). Once the registry entry is gone, the next process startup's `sweep_orphans` cannot reconstruct the wallet, so the registered identity's balance is permanently lost. The framework's top-line safety property is panic-safe fund recovery; exposing a public setup helper that funds identities and then guarantees the resulting balances are forgotten violates that contract. Currently latent (no test in the PR calls `setup_with_n_identities`), but the helper is `pub` and TEST_SPEC.md schedules ID-* tests on it. Pick one of: (a) keep the registry entry while `sweep_identities` is a stub so the orphan sweep retains a recovery trail; (b) return `FrameworkError::Cleanup` from `teardown_one` until identity sweep is implemented; or (c) gate `setup_with_n_identities` behind a feature flag until identity sweep lands.
In `packages/rs-platform-wallet/tests/e2e/framework/wait.rs`:
- [SUGGESTION] lines 76-122: wait_for_balance's Notified future is constructed but never registered before sync_balances runs
Confirmed at HEAD. The doc on lines 64-65/77 promises the `Notified` future is captured BEFORE the sync to avoid losing notifications fired mid-sync, but `let notified = test_wallet.wait_hub().notified(); tokio::pin!(notified);` only constructs and pins the future. Tokio's `Notify::notified` does not register the waiter on the hub until the first poll (or via `Notified::enable()`), and that first poll happens at `tokio::time::timeout(cap, notified.as_mut()).await` AFTER `sync_balances` returns. Notifications fired by `WaitEventHub` during the sync are silently dropped. `BACKSTOP_WAKE_INTERVAL = 2s` bounds the wake delay, so this is a polish issue rather than a hang risk — but it means `wait_for_balance` is effectively a 2s polling loop on busy chains, not the event-driven helper its docstring claims. Calling `notified.as_mut().enable()` immediately after the `pin!` would register the waiter eagerly and honor the documented invariant.
In `packages/simple-signer/src/signer.rs`:
- [SUGGESTION] lines 197-241: from_seed_for_identity is misleadingly named — the resulting SimpleSigner cannot satisfy Signer<IdentityPublicKey>
Confirmed at HEAD (behind the `derive` feature). `from_seed_for_identity` populates only `address_private_keys` keyed by `ripemd160_sha256(pubkey)` (lines 235-238), but the `Signer<IdentityPublicKey>` impl on `SimpleSigner` (lines 245-291) reads exclusively from `private_keys` / `private_keys_in_creation`. The constructor's name promises an identity signer the trait impl cannot deliver. The doc hand-waves with "callers must additionally register `IdentityPublicKey` records via `add_identity_public_key`" — but that requires raw private-key bytes the constructor never re-exposes. The new e2e harness only works because `SeedBackedIdentitySigner` bypasses the trait impl and reaches into `address_private_keys` directly. A downstream caller who reads the name and instantiates this constructor reasonably assumes the returned `SimpleSigner` can sign identity keys — it cannot. Either rename to reflect the actual side effect (e.g. `from_seed_for_identity_authentication_keys_by_pkh`) or also insert into `private_keys` so the trait impl is honest.
In `packages/rs-platform-wallet/src/changeset/changeset.rs`:
- [SUGGESTION] lines 583-645: PlatformAddressChangeSet::fee is a public Credits field whose own doc admits it does not represent the on-chain fee, and Merge silently sums it
Confirmed at HEAD. `pub fee: Credits` (line 589) stores `AddressFundsTransferTransition::estimate_min_fee(...)`, which `estimated_min_fee()` (lines 592-611) explicitly calls out as "NOT the actual on-chain fee" and "NOT adjusted by the `fee_strategy`" — the static `state_transition_min_fees` floor (~6.5M for 1in/1out) is far below the chain-time debit (~14.94M observed; see issue #3040). Because the field is `pub` and shares its type with real credit values, callers reaching for `cs.fee` rather than the doc-laden accessor get the unfiltered footgun. `Merge::merge` (line 644) does `self.fee = self.fee.saturating_add(other.fee)`, so a merged changeset's reported fee is a sum of values that already lied per-operation. Pick one of: (a) wrap in a newtype like `EstimatedMinFee(Credits)` so the type system surfaces the caveat at every call site; (b) rename to `static_min_fee_estimate` so accidental consumers can't confuse it with paid fees; or (c) keep it `pub(crate)` until #3040 lands.
In `packages/rs-sdk/src/platform/transition/address_inputs.rs`:
- [SUGGESTION] lines 12-40: Low-level nonce helpers were promoted to the public rs-sdk surface for a single cross-crate test consumer
Confirmed at HEAD. `pub mod address_inputs` (transition.rs:3) plus `pub fn fetch_inputs_with_nonce` (address_inputs.rs:12) and `pub fn nonce_inc` (address_inputs.rs:34) widen what was previously `pub(crate)` plumbing into a permanent public-API stability commitment for `rs-sdk`. They are used internally by put_identity / transfer_address_funds / shield / top_up_identity_from_addresses / address_credit_withdrawal, but `pub(crate)` would suffice for those. The only out-of-crate consumer is `transfer_capturing_st_bytes` in `tests/e2e/framework/wallet_factory.rs:265-280` (a fetch→increment→sign→broadcast capture for replay tests). The signatures expose internal types (`AddressNonce`, `AddressInfos`, `BTreeMap<PlatformAddress, ...>`); `nonce_inc` is a footgun outside the strict flow it presumes — incrementing twice or without broadcasting desyncs the address nonce. Prefer a higher-level public helper (e.g. `build_replayable_transfer_bytes`) that owns the fetch/increment/serialize sequence and revert these to `pub(crate)`. If a test-only export is acceptable, gate them behind `#[cfg(any(test, feature = "test-helpers"))]`.
| 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(()) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: MultiIdentitySetupGuard::teardown silently burns identity credits — sweep_identities is a no-op while the registry entry is unconditionally removed
Confirmed at HEAD. setup_with_n_identities (framework/mod.rs:190-237) registers identities funded from the bank, but the teardown path forwards to cleanup::teardown_one, which calls sweep_identities (cleanup.rs:336-338, an explicit Ok(()) stub gated by #identity-sweep) and then unconditionally drops the registry entry at cleanup.rs:178 (registry.remove(&test_wallet.id())?). Once the registry entry is gone, the next process startup's sweep_orphans cannot reconstruct the wallet, so the registered identity's balance is permanently lost. The framework's top-line safety property is panic-safe fund recovery; exposing a public setup helper that funds identities and then guarantees the resulting balances are forgotten violates that contract. Currently latent (no test in the PR calls setup_with_n_identities), but the helper is pub and TEST_SPEC.md schedules ID-* tests on it. Pick one of: (a) keep the registry entry while sweep_identities is a stub so the orphan sweep retains a recovery trail; (b) return FrameworkError::Cleanup from teardown_one until identity sweep is implemented; or (c) gate setup_with_n_identities behind a feature flag until identity sweep lands.
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`:
- [SUGGESTION] lines 171-188: MultiIdentitySetupGuard::teardown silently burns identity credits — sweep_identities is a no-op while the registry entry is unconditionally removed
Confirmed at HEAD. `setup_with_n_identities` (framework/mod.rs:190-237) registers identities funded from the bank, but the teardown path forwards to `cleanup::teardown_one`, which calls `sweep_identities` (cleanup.rs:336-338, an explicit `Ok(())` stub gated by `#identity-sweep`) and then unconditionally drops the registry entry at cleanup.rs:178 (`registry.remove(&test_wallet.id())?`). Once the registry entry is gone, the next process startup's `sweep_orphans` cannot reconstruct the wallet, so the registered identity's balance is permanently lost. The framework's top-line safety property is panic-safe fund recovery; exposing a public setup helper that funds identities and then guarantees the resulting balances are forgotten violates that contract. Currently latent (no test in the PR calls `setup_with_n_identities`), but the helper is `pub` and TEST_SPEC.md schedules ID-* tests on it. Pick one of: (a) keep the registry entry while `sweep_identities` is a stub so the orphan sweep retains a recovery trail; (b) return `FrameworkError::Cleanup` from `teardown_one` until identity sweep is implemented; or (c) gate `setup_with_n_identities` behind a feature flag until identity sweep lands.
| loop { | ||
| // Capture `Notified` BEFORE the sync so a notification | ||
| // arriving mid-sync isn't lost; pin + `as_mut()` lets us | ||
| // re-await the same future across timeouts. | ||
| let notified = test_wallet.wait_hub().notified(); | ||
| tokio::pin!(notified); | ||
|
|
||
| match test_wallet.sync_balances().await { | ||
| Ok(()) => { | ||
| let balances = test_wallet.balances().await; | ||
| let current = balances.get(addr).copied().unwrap_or(0); | ||
| if current >= expected { | ||
| tracing::info!( | ||
| target: "platform_wallet::e2e::wait", | ||
| addr = ?addr, | ||
| observed = current, | ||
| elapsed = ?start.elapsed(), | ||
| "balance reached target" | ||
| ); | ||
| return Ok(()); | ||
| } | ||
| tracing::debug!( | ||
| target: "platform_wallet::e2e::wait", | ||
| addr = ?addr, | ||
| current, | ||
| expected, | ||
| "balance below target; waiting on event hub" | ||
| ); | ||
| } | ||
| Err(err) => tracing::debug!( | ||
| target: "platform_wallet::e2e::wait", | ||
| error = %err, | ||
| "sync_balances during wait_for_balance failed; retrying" | ||
| ), | ||
| } | ||
|
|
||
| let remaining = deadline.saturating_duration_since(Instant::now()); | ||
| if remaining.is_zero() { | ||
| return Err(FrameworkError::Cleanup(format!( | ||
| "wait_for_balance timed out after {timeout:?} \ | ||
| (addr={addr:?} expected={expected})" | ||
| ))); | ||
| } | ||
| // Backstop wake on idle chains; real activity wakes us | ||
| // earlier via the `Notified` future. | ||
| let cap = std::cmp::min(remaining, BACKSTOP_WAKE_INTERVAL); | ||
| let _ = tokio::time::timeout(cap, notified.as_mut()).await; |
There was a problem hiding this comment.
🟡 Suggestion: wait_for_balance's Notified future is constructed but never registered before sync_balances runs
Confirmed at HEAD. The doc on lines 64-65/77 promises the Notified future is captured BEFORE the sync to avoid losing notifications fired mid-sync, but let notified = test_wallet.wait_hub().notified(); tokio::pin!(notified); only constructs and pins the future. Tokio's Notify::notified does not register the waiter on the hub until the first poll (or via Notified::enable()), and that first poll happens at tokio::time::timeout(cap, notified.as_mut()).await AFTER sync_balances returns. Notifications fired by WaitEventHub during the sync are silently dropped. BACKSTOP_WAKE_INTERVAL = 2s bounds the wake delay, so this is a polish issue rather than a hang risk — but it means wait_for_balance is effectively a 2s polling loop on busy chains, not the event-driven helper its docstring claims. Calling notified.as_mut().enable() immediately after the pin! would register the waiter eagerly and honor the documented invariant.
💡 Suggested change
| loop { | |
| // Capture `Notified` BEFORE the sync so a notification | |
| // arriving mid-sync isn't lost; pin + `as_mut()` lets us | |
| // re-await the same future across timeouts. | |
| let notified = test_wallet.wait_hub().notified(); | |
| tokio::pin!(notified); | |
| match test_wallet.sync_balances().await { | |
| Ok(()) => { | |
| let balances = test_wallet.balances().await; | |
| let current = balances.get(addr).copied().unwrap_or(0); | |
| if current >= expected { | |
| tracing::info!( | |
| target: "platform_wallet::e2e::wait", | |
| addr = ?addr, | |
| observed = current, | |
| elapsed = ?start.elapsed(), | |
| "balance reached target" | |
| ); | |
| return Ok(()); | |
| } | |
| tracing::debug!( | |
| target: "platform_wallet::e2e::wait", | |
| addr = ?addr, | |
| current, | |
| expected, | |
| "balance below target; waiting on event hub" | |
| ); | |
| } | |
| Err(err) => tracing::debug!( | |
| target: "platform_wallet::e2e::wait", | |
| error = %err, | |
| "sync_balances during wait_for_balance failed; retrying" | |
| ), | |
| } | |
| let remaining = deadline.saturating_duration_since(Instant::now()); | |
| if remaining.is_zero() { | |
| return Err(FrameworkError::Cleanup(format!( | |
| "wait_for_balance timed out after {timeout:?} \ | |
| (addr={addr:?} expected={expected})" | |
| ))); | |
| } | |
| // Backstop wake on idle chains; real activity wakes us | |
| // earlier via the `Notified` future. | |
| let cap = std::cmp::min(remaining, BACKSTOP_WAKE_INTERVAL); | |
| let _ = tokio::time::timeout(cap, notified.as_mut()).await; | |
| let notified = test_wallet.wait_hub().notified(); | |
| tokio::pin!(notified); | |
| notified.as_mut().enable(); |
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/wait.rs`:
- [SUGGESTION] lines 76-122: wait_for_balance's Notified future is constructed but never registered before sync_balances runs
Confirmed at HEAD. The doc on lines 64-65/77 promises the `Notified` future is captured BEFORE the sync to avoid losing notifications fired mid-sync, but `let notified = test_wallet.wait_hub().notified(); tokio::pin!(notified);` only constructs and pins the future. Tokio's `Notify::notified` does not register the waiter on the hub until the first poll (or via `Notified::enable()`), and that first poll happens at `tokio::time::timeout(cap, notified.as_mut()).await` AFTER `sync_balances` returns. Notifications fired by `WaitEventHub` during the sync are silently dropped. `BACKSTOP_WAKE_INTERVAL = 2s` bounds the wake delay, so this is a polish issue rather than a hang risk — but it means `wait_for_balance` is effectively a 2s polling loop on busy chains, not the event-driven helper its docstring claims. Calling `notified.as_mut().enable()` immediately after the `pin!` would register the waiter eagerly and honor the documented invariant.
| /// 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) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: from_seed_for_identity is misleadingly named — the resulting SimpleSigner cannot satisfy Signer
Confirmed at HEAD (behind the derive feature). from_seed_for_identity populates only address_private_keys keyed by ripemd160_sha256(pubkey) (lines 235-238), but the Signer<IdentityPublicKey> impl on SimpleSigner (lines 245-291) reads exclusively from private_keys / private_keys_in_creation. The constructor's name promises an identity signer the trait impl cannot deliver. The doc hand-waves with "callers must additionally register IdentityPublicKey records via add_identity_public_key" — but that requires raw private-key bytes the constructor never re-exposes. The new e2e harness only works because SeedBackedIdentitySigner bypasses the trait impl and reaches into address_private_keys directly. A downstream caller who reads the name and instantiates this constructor reasonably assumes the returned SimpleSigner can sign identity keys — it cannot. Either rename to reflect the actual side effect (e.g. from_seed_for_identity_authentication_keys_by_pkh) or also insert into private_keys so the trait impl is honest.
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 — the resulting SimpleSigner cannot satisfy Signer<IdentityPublicKey>
Confirmed at HEAD (behind the `derive` feature). `from_seed_for_identity` populates only `address_private_keys` keyed by `ripemd160_sha256(pubkey)` (lines 235-238), but the `Signer<IdentityPublicKey>` impl on `SimpleSigner` (lines 245-291) reads exclusively from `private_keys` / `private_keys_in_creation`. The constructor's name promises an identity signer the trait impl cannot deliver. The doc hand-waves with "callers must additionally register `IdentityPublicKey` records via `add_identity_public_key`" — but that requires raw private-key bytes the constructor never re-exposes. The new e2e harness only works because `SeedBackedIdentitySigner` bypasses the trait impl and reaches into `address_private_keys` directly. A downstream caller who reads the name and instantiates this constructor reasonably assumes the returned `SimpleSigner` can sign identity keys — it cannot. Either rename to reflect the actual side effect (e.g. `from_seed_for_identity_authentication_keys_by_pkh`) or also insert into `private_keys` so the trait impl is honest.
| @@ -606,13 +636,20 @@ impl Merge for PlatformAddressChangeSet { | |||
| .map_or(r, |existing| existing.max(r)), | |||
| ); | |||
| } | |||
| // Fee: append-sum via `saturating_add`. Sync-only merges | |||
| // (`fee == 0`) are a no-op so a transfer's recorded fee | |||
| // survives untouched; merging two transfer changesets sums | |||
| // the per-operation fees so the merged total reflects the | |||
| // "total fee paid across operations in this batch" intent. | |||
| self.fee = self.fee.saturating_add(other.fee); | |||
| } | |||
There was a problem hiding this comment.
🟡 Suggestion: PlatformAddressChangeSet::fee is a public Credits field whose own doc admits it does not represent the on-chain fee, and Merge silently sums it
Confirmed at HEAD. pub fee: Credits (line 589) stores AddressFundsTransferTransition::estimate_min_fee(...), which estimated_min_fee() (lines 592-611) explicitly calls out as "NOT the actual on-chain fee" and "NOT adjusted by the fee_strategy" — the static state_transition_min_fees floor (~6.5M for 1in/1out) is far below the chain-time debit (~14.94M observed; see issue #3040). Because the field is pub and shares its type with real credit values, callers reaching for cs.fee rather than the doc-laden accessor get the unfiltered footgun. Merge::merge (line 644) does self.fee = self.fee.saturating_add(other.fee), so a merged changeset's reported fee is a sum of values that already lied per-operation. Pick one of: (a) wrap in a newtype like EstimatedMinFee(Credits) so the type system surfaces the caveat at every call site; (b) rename to static_min_fee_estimate so accidental consumers can't confuse it with paid fees; or (c) keep it pub(crate) until #3040 lands.
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/src/changeset/changeset.rs`:
- [SUGGESTION] lines 583-645: PlatformAddressChangeSet::fee is a public Credits field whose own doc admits it does not represent the on-chain fee, and Merge silently sums it
Confirmed at HEAD. `pub fee: Credits` (line 589) stores `AddressFundsTransferTransition::estimate_min_fee(...)`, which `estimated_min_fee()` (lines 592-611) explicitly calls out as "NOT the actual on-chain fee" and "NOT adjusted by the `fee_strategy`" — the static `state_transition_min_fees` floor (~6.5M for 1in/1out) is far below the chain-time debit (~14.94M observed; see issue #3040). Because the field is `pub` and shares its type with real credit values, callers reaching for `cs.fee` rather than the doc-laden accessor get the unfiltered footgun. `Merge::merge` (line 644) does `self.fee = self.fee.saturating_add(other.fee)`, so a merged changeset's reported fee is a sum of values that already lied per-operation. Pick one of: (a) wrap in a newtype like `EstimatedMinFee(Credits)` so the type system surfaces the caveat at every call site; (b) rename to `static_min_fee_estimate` so accidental consumers can't confuse it with paid fees; or (c) keep it `pub(crate)` until #3040 lands.
| /// 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(); |
There was a problem hiding this comment.
💬 Nitpick: E2eContext::cancel_token is constructed and exposed but never observed
Confirmed at HEAD. cancel_token is created at harness.rs:109, exposed via cancel_token() at harness.rs:96, and the doc claims it backs "graceful shutdown" of background helpers. A grep across packages/rs-platform-wallet/tests/e2e/ shows no .cancel(), no .cancelled(), and no tokio::select! arm referencing it — wait_for_balance, wait_for_identity_balance, wait_for_dpns_name_visible, the deferred SPV blocks, and the test bodies all ignore it. (The spv_sync.rs matches use a different local cancel token.) The token is dead state with a forward-looking accessor that tempts misuse — a future contributor may assume it actually halts background work. Either drop the field until shutdown wiring lands (Task #15) or wire one observer in (e.g. a tokio::select! arm in wait::wait_for_balance).
source: ['claude', 'codex']
| /// DIP-17 default account / key-class for clear-funds platform | ||
| /// payments. Matches `WalletAccountCreationOptions::Default`. | ||
| const DEFAULT_ACCOUNT_INDEX: u32 = 0; | ||
| const DEFAULT_KEY_CLASS: u32 = 0; | ||
|
|
||
| /// Build a [`SimpleSigner`] populated with the DIP-17 platform-payment | ||
| /// gap window for `seed_bytes` on `network`. Pins to | ||
| /// `account=0`/`key_class=0` to match | ||
| /// `WalletAccountCreationOptions::Default`. `SimpleSigner` already | ||
| /// implements `Signer<PlatformAddress>` directly, so callers can pass | ||
| /// the returned value straight to `PlatformAddressWallet::transfer`. | ||
| pub(super) fn make_platform_signer( | ||
| seed_bytes: &[u8; 64], | ||
| network: Network, | ||
| ) -> FrameworkResult<SimpleSigner> { | ||
| SimpleSigner::from_seed_for_platform_address_account( | ||
| seed_bytes, | ||
| network, | ||
| DEFAULT_ACCOUNT_INDEX, | ||
| DEFAULT_KEY_CLASS, | ||
| DIP17_GAP_LIMIT, | ||
| ) | ||
| .map_err(|err| FrameworkError::Wallet(format!("simple-signer: {err}"))) | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: make_platform_signer duplicates the DIP-17 default account/key-class constants instead of reusing the framework's tested source of truth
Confirmed at HEAD. framework/mod.rs:41-42 declares its own DEFAULT_ACCOUNT_INDEX = 0 / DEFAULT_KEY_CLASS = 0 and uses them in make_platform_signer, while framework/wallet_factory.rs:49-50 already exposes DEFAULT_ACCOUNT_INDEX_PUB / DEFAULT_KEY_CLASS_PUB derived from DEFAULT_PLATFORM_PAYMENT_ACCOUNT_SPEC with a drift test (wallet_factory.rs:666-667) asserting they match PlatformPaymentAccountSpec::default(). If the upstream default ever changes, every wallet-creation and transfer path that imports the _PUB constants will track the new value, but make_platform_signer will keep deriving keys for 0/0 — and the drift test won't catch the mismatch because it doesn't reference the local copies. Reuse the exported constants to keep signer derivation and wallet construction on a single source of truth.
source: ['claude']
| /// Placeholder returned by paths that surface an underlying | ||
| /// error through tracing; the static string names the call site. | ||
| #[error("e2e framework not yet implemented: {0}")] | ||
| NotImplemented(&'static str), |
There was a problem hiding this comment.
💬 Nitpick: FrameworkError::NotImplemented is dead — no producers anywhere in the workspace
Confirmed at HEAD. A repo-wide grep for NotImplemented in packages/rs-platform-wallet/ returns only the variant declaration on this line — every prior call site migrated to typed Sdk / Spv / Cleanup variants. Carrying a dead variant lowers signal in the error enum and tempts the next contributor to reach for it again as a generic placeholder when SPV (Task #15) re-lands. Drop it; if a real "feature deferred" path appears later, prefer a named typed variant.
source: ['claude']
| pub async fn sync_watermark(&self) -> Option<u64> { | ||
| let guard = self.provider.read().await; | ||
| let raw = guard.as_ref().map(|p| p.last_known_recent_block())?; | ||
| if raw == 0 { | ||
| None | ||
| } else { | ||
| Some(raw) | ||
| } | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: sync_watermark collapses "block 0" and "no watermark stored" into the same None
Confirmed at HEAD. sync_watermark returns None whenever last_known_recent_block() == 0 (lines 272-276). The doc claim that this matches the "no stored watermark" convention is reasonable on freshly-restored mainnet/testnet wallets, but breaks down for any chain where block 0 is a legitimate genesis-aligned watermark (devnet/regtest, or any test fixture that explicitly sets last_known_recent_block to 0 during setup). After a sync that legitimately lands at height 0, the next call would still report "never synced." PA-007 in the PR's TEST_SPEC.md motivates this getter for monotonicity assertions; on regtest those assertions could be misled by the collapse. If the underlying intent is "distinguish unset from set," make the storage layer return Option<u64> end-to-end so the boundary stays honest.
source: ['claude', 'codex']
lklimek
removed
the
ready for final review
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