test(rs-platform-wallet/e2e): implement PA P0 — multi-output, partial-fund, sweep-back#3571
Draft
Claudius-Maginificent wants to merge 16 commits intofeat/rs-platform-wallet-e2e-casesfrom
Draft
Conversation
…platform-address cases)
Adds the three P0 platform-address scenarios from
`tests/e2e/TEST_SPEC.md` §3:
* **PA-001 — Multi-output platform-address transfer** — bank funds
`addr_1`; the wallet self-transfers two outputs to {addr_2, addr_3}
in one transition. Pinned contract: under `[ReduceOutput(0)]` the
lex-larger output arrives at gross exactly while the lex-smaller
absorbs the chain-time fee. `Σ inputs == Σ outputs` validated
against `addr_1`'s residual change.
* **PA-002 — Partial-fund + change handling** — bank funds `addr_1`;
wallet self-transfers a fraction to `addr_2`. Asserts the change
shape (`addr_1` retains `funded − bank_fee − transfer_gross`) per
the invariant fixed in `aaf8be74ee` and `9ea9e7033c`. Replaces
the prior `transfer.rs` happy-path test with the spec-aligned
PA-002 case.
* **PA-004 — Sweep-back** — bank funds `addr_1`; teardown sweeps to
the bank's primary receive address; assertion is the registry
contract (entry removed iff sweep transition landed). Cross-test
bank-balance accounting is documented as out of scope — other
tests' sweeps interleave on the shared bank within this test's
window — and would belong to a separate aggregate self-test.
`next_unused_address` parks the cursor until each derived address
is observed used (PA-005 invariant), so PA-001 needs a marker
"prep" transfer to mark `addr_2` used before deriving `addr_3`.
The output amounts on every PA case sit well above the empirical
chain-time fee ceiling (~15M for 1in/1out, ~20M for 1in/2out) to
dodge platform #3040 — see PA-002's `#3040` note.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Important Review skippedDraft detected. 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:
✨ 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 |
…regression detection Replaces the previous loose fee upper bounds (`< OUTPUT_A_CREDITS` / `< TRANSFER_CREDITS`) — which spanned the entire valid empirical fee range and would let a 50%+ fee regression slip through silently — with named regression-guard ceilings. PA-001 multi-output (1in/2out): multi_fee < OUTPUT_A_CREDITS (50M) -> multi_fee < MULTI_FEE_CEILING (30M) PA-002 partial-fund (1in/1out, two transitions): transfer_fee < TRANSFER_CREDITS (50M) -> transfer_fee < TRANSFER_FEE_CEILING (25M) (new) bank_fee < BANK_FEE_CEILING (25M) The ceilings sit at ~50-67% headroom over the empirical chain-time fees at write-time (~15M for 1in/1out, ~20M for 1in/2out per platform #3040's gap analysis). A failure of these bounds means either (a) the protocol fee schedule shifted significantly — bump the constant deliberately — or (b) a fee-explosion regression has landed on either the wallet or dpp side, which is exactly what a tight bound exists to surface. Per the test-writing principle (memory `91562d6b-...`): tests detect bugs; loose bounds spanning the entire valid range hide regressions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ain balance check Adds a second assertion to PA-004: re-derives a fresh PlatformWallet from the captured seed bytes, syncs it, and asserts addr_1's on-chain balance is zero after teardown. Why: the registry-entry-removed assertion alone is not sufficient. A regression in `cleanup::teardown_one` could remove the registry entry without actually broadcasting the sweep transition (e.g. the order-of-operations swap on commit `60f7850ab0` style mistake), and the test would silently pass while credits leaked. The re-derived wallet bypasses the now-gone TestWallet's cached state, so the balance check sees only on-chain truth. Re-derivation pattern matches `cleanup::sweep_one`'s orphan-recovery path (capture seed → `create_wallet_from_seed_bytes` → initialise → sync). After the assertion the test best-effort removes the re-derived wallet from the manager so subsequent cases don't see it; failure is non-fatal (the wallet has zero balance and no remaining work). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…le on demand) New test `pa_3040_reduce_output_chain_time_fee_exceeds_static_estimate` in `cases/pa_3040_bug_pin.rs` reproduces platform issue #3040 directly: self-transfer of 8M credits with default `[ReduceOutput(0)]`. 8M sits inside the bug zone — above the wallet's static `estimate_min_fee` ceiling (~6.5M for 1in/1out) so Phase 4 selection passes, but below Drive's chain-time fee (~15M) so the broadcast fails with `AddressesNotEnoughFundsError`. The assertion is on the error string containing `"AddressesNotEnoughFundsError"`. When #3040 is fixed (tightening estimate_min_fee, widening auto-select to reserve fee headroom for ReduceOutput, or some hybrid) this test should be either deleted or inverted. The `Ok(_)` arm panics with a self-explanatory message so the next reader knows what to do. Marked `#[ignore]` so the failure doesn't break green CI; runnable on demand with `cargo test -- --ignored`. The failure IS the proof; the test names the bug precisely. Per the test-writing principle (memory `91562d6b-...`): tests detect bugs; bug-fixing belongs in a separate PR with its own review. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…fy inverted semantics
The original substring `"AddressesNotEnoughFundsError"` is the Rust
enum name; it never appears in the SDK error chain that bubbles up to
the test. The actual user-facing message comes from the type's
`Display` impl: `"Insufficient combined address balances: total
available is less than required {required_balance}"`.
Repointed `EXPECTED_ERROR_SUBSTRING` to that distinctive prefix so the
assertion matches the actual bug shape (verified with a live testnet
run: error reads `"...Insufficient combined address balances: total
available is less than required 14939900"`, exactly the chain-time-fee
mismatch #3040 surfaces).
Also clarified the docstring: the test is INVERTED — it passes while
the bug exists (the expected error proves the bug is reproducible), and
fails when #3040 is fixed (the `Ok(_)` arm panics with a self-
explanatory message). The developer who fixes #3040 must delete the
test or invert the assertion. Documented the inversion explicitly so
future readers don't misread "test passes" as "wallet works correctly
on this path".
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…batch 1) Pins three core platform-address contracts per TEST_SPEC.md §3: - PA-003 fee scaling: fee_5 > fee_1 (more outputs ⇒ more bytes ⇒ bigger fee) and fee_5 < 5*fee_1 (sub-linear — outputs share inputs/headers). A regression in the fee strategy that starts charging per-output linearly fails this test deterministically. - PA-005 address rotation: cursor parks on repeated next_unused calls until observed-used; advances after funding+sync; no collisions across the rotation sequence. Regression bound on the cursor invariants the address provider depends on. - PA-006 replay safety: a state-transition built and signed with the same (input, nonce) tuple as a previously-broadcasted ST is rejected on the second submission. Pins the protocol-level nonce / replay-protection contract; without this a "spam-click" UX could double-debit the source address. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…A-010 (P1 batch 2) Pins four more PA contracts per TEST_SPEC.md §3: - PA-002b zero-change exact-equality: at the boundary bal_input == Σ outputs, addr_1 must end at EXACTLY 0 — no 1-credit residual from a regression in the change-output predicate. Pins the Σ inputs == Σ outputs invariant the wallet just shipped fixes for (aaf8be7, 9ea9e70). - PA-007 sync watermark idempotency: three back-to-back sync_balances calls all succeed; watermark is monotonic non-decreasing; cached balances are byte-equal across calls (catches double-counting on re-sync). - PA-008 concurrent funding: three concurrent fund_address calls into three distinct addresses on one wallet all succeed without nonce collisions or lost funding. Pins the FUNDING_MUTEX guarantee. - PA-010 bank starvation: #[ignore]'d with a documented harness gap. Needs Bank::with_balance_for_test constructor and typed BankError::Underfunded variant — out of scope for the test-only crate per task constraints. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implements all 10 P2 platform-address cases per TEST_SPEC.md §3. Runnable with tight assertions: - PA-001c zero-credit single-output: pins the boundary contract (rejected vs accepted-as-fee-only) using whichever shape the wallet implements. - PA-004c sweep zero balance: never-funded wallet's teardown must return Ok and drop the registry entry without broadcast. - PA-006b concurrent identical broadcasts: two parallel broadcasts of the SAME signed ST bytes must resolve to exactly one accepted transition. Adds minimal build_transfer_st_bytes harness helper (build only, no parallel production broadcast). - PA-007b concurrent sync_balances: two concurrent syncs both succeed; cached balance is byte-equal across the pair (no double-counting); watermark monotonic. - PA-008b two wallets × six concurrent funders: cross-wallet FUNDING_MUTEX serialisation; all six addresses observe their funded amounts. Ignored (harness gap clearly documented in module docs): - PA-001b change-address branch — output_change_address is not a parameter of the production transfer signature. - PA-004b sweep dust boundary — needs exact-balance setup helper (no mechanism to land balance at dust_gate ± 1). - PA-005b gap-limit triplet — next_unused_receive_address parks; no force-derive helper for N>1 unused. - PA-008c observable mutex serialisation — needs FUNDING_MUTEX instrumentation hook. - PA-009 min_input_amount boundary — same exact-balance gap as PA-004b. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…s' into feat/rs-platform-wallet-e2e-cases-pa
…red = bug present) The original PA-3040 was inverted (asserted the failure shape; passed while the bug existed; failed when fixed) and `#[ignore]`'d. Per the project test-writing principle (memory `91562d6b`), tests that hit a bug should fail; the failure IS the proof. The previous structure hid the failure behind `#[ignore]` and inverted the contract. Rewrites the test in the standard direction: - Asserts the contract: 1in/1out transfer with `output[0] > estimate_min_fee` must succeed; addr_2 must receive `OUTPUT_CREDITS − chain_time_fee`; Σ inputs == Σ outputs invariant. - Today (#3040 unfixed): `transfer()` succeeds at the wallet layer but Drive rejects the broadcast — `.expect("self-transfer")` panics, the test fails. The red is the proof that #3040 still exists. - After fix: transfer succeeds, post-conditions verified, test passes. Removes `#[ignore]`. Renames `pa_3040_..._exceeds_static_estimate` to `..._must_not_exceed_static_estimate` to align the test name with the contract being asserted. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…anup dust gate Harness-only extensions to support PA-008c, PA-004b, and PA-009: - bank.rs: record per-call (seq, entry_ns, exit_ns) for every FUNDING_MUTEX acquisition into a capped ring buffer; expose BankWallet::funding_mutex_history() as a drain accessor. Entry sample is taken AFTER lock().await resolves and exit BEFORE the guard drops, so recorded windows are strict subsets of the actual hold time. - cleanup.rs: add cleanup_dust_gate(version) — public mirror of the private min_input_amount() helper so boundary tests can pin the active platform version's gate without shadowing the constant. No production-side changes; all wiring lives under tests/e2e/framework/. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Three test-only cases that previously deferred on imagined harness
gaps. Now runnable against testnet (gated by the same environment
variables as the rest of the e2e suite).
PA-004b — Sweep dust-threshold boundary (below-gate sub-case)
Pins that `cleanup::teardown_one` SKIPS the platform-address
sweep when wallet total < `min_input_amount(version)`. Setup uses
Option A from the brief: bank-fund 50M, partial-drain to leave
exactly TARGET_RESIDUAL (1_000) on addr_1 via the post-fix
`Σ inputs == Σ outputs` invariant (auto-select with
[ReduceOutput(0)] charges chain-time fee against output[0], not
the residual). Asserts: teardown returns Ok, registry cleared,
on-chain addr_1 still holds TARGET_RESIDUAL (no broadcast).
AT/JUST-ABOVE sub-cases left unimplemented: at the active gate
(100_000 credits) the sweep transition's chain-time fee
(~15_000_000) exceeds the available balance, so the AT/+1
sub-cases collapse onto "broadcast attempted, broadcast failed"
and would leave a permanently-stuck testnet orphan with no
recovery path. PA-004 already pins the well-above-fee path with
100M; the BELOW-gate path is what distinguishes PA-004b.
PA-009 — `min_input_amount` cleanup gate tracks PlatformVersion
Same trim pattern as PA-004b but the unique assertion is a
version-source pin: the cleanup gate value equals
PlatformVersion::latest().dpp.state_transitions.address_funds
.min_input_amount, and the gate is positive. A future refactor
that hardcodes the gate would still pass PA-004 / PA-004b but
must fail this assertion.
PA-008c — Observable serialisation of FUNDING_MUTEX
Spawns three concurrent bank.fund_address tasks and drains the
per-call (seq, entry_ns, exit_ns) history recorded by the new
harness instrumentation in framework/bank.rs. Asserts:
1. Exactly three entries (cardinality matches fan-in).
2. seq is strictly monotonic (atomic counter integrity).
3. Sorted by seq, prev.exit_ns <= next.entry_ns for every
consecutive pair — the mutex critical sections are
pairwise non-overlapping. A silent removal of FUNDING_MUTEX
would surface here; PA-008 / PA-008b would still pass.
4. Each window is well-formed (exit_ns >= entry_ns).
Also includes a fmt-pass touchup in framework/bank.rs from the
prior harness-extension commit.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tubs and pin PA-001b drift Replaces the discursive #[ignore] reasons on the three PA cases that genuinely need feature work or harness refactors with crisp status-tagged messages that point a reviewer at TEST_SPEC.md's **Status** field. Bodies stay as panic!() with a single sentence naming the unmet dependency. - PA-001b — BLOCKED: feature missing in production (no output_change_address parameter on PlatformAddressWallet::transfer). - PA-005b — BLOCKED: needs production API (next_unused_receive_addresses(count) wrapping AddressPool::next_unused_multiple). The 21-funding-round workaround is feasible but ~10 min/sub-case runtime. - PA-010 — BLOCKED: needs harness refactor (per-test bank instance OR injectable balance override on the singleton, plus typed BankError::Underfunded). Adds Found-020 to TEST_SPEC.md documenting the PA-001b spec/impl drift as a documentation contract violation: the spec describes a parameter the production API does not expose. Found-020 lists two valid resolutions (spec realignment OR production extension) so the entry can be retired in either direction. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…c entry
Adds a `**Status**:` line directly under `**Priority**:` for every
PA / ID / TK / CR / CT / DPNS / DP / CN / Harness case in
TEST_SPEC.md. Found-NNN entries are exempt — their implicit status
is "PIN — failing test would prove the bug" and a redundant Status
line would be misleading.
Controlled vocabulary used:
- IMPLEMENTED — passing : test runs and passes on testnet
(all PA P0 + most P1 + several P2 cases).
- BLOCKED — feature missing in production: <description>
(PA-001b — see Found-020).
- BLOCKED — needs production API: <signature>
(PA-005b).
- BLOCKED — needs harness refactor: <description>
(PA-010, PA-013, CR-001..003, CN-002).
- STUB — placeholder for follow-up PR (everything else: PA-011..014,
ID-*, TK-*, CT-*, DPNS-*, DP-*, CN-001, Harness-*).
A reviewer can now scan the spec and immediately see which cases
are real and which are aspirational, without having to cross-
reference cases/mod.rs against TEST_SPEC.md headings.
Found-020 documents the PA-001b spec/impl drift discovered while
auditing the deferred cases for this PR; PA-001b's status references
it.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ument PA spec drift Marvin's spec-deviation audit pass over the 21 PA tests. Three test files gain stricter assertions; the spec gains caveats where current tests cannot honour the documented contract on the existing harness. Test changes: - PA-003: add the spec-documented `fee_5 - fee_1` regression bound. The literal 1_000_000 ceiling fires under platform issue #3040 (chain-time fees ~5-10M above static estimate); pin a 25M ceiling so the regression-guard intent (reject linear-in-output-count fee schedules) survives until #3040 is resolved. - PA-006: tighten the replay-rejection assertion from `is_err()` to a stale-nonce / already-exists / InvalidIdentityNonce / duplicate class match (typed `Error::AlreadyExists` first, fallback to keyword match on the rendered display / debug strings). Loose `is_err` would let any error type slip past — a transport timeout would have passed silently. - PA-006b: same tightening on the losing-future branch — the OK count was pinned but the failure class was not. Spec changes (TEST_SPEC.md): - PA-001: note that the live test pins fee < 30M (not the spec's 5M) while #3040 is unresolved; the 5M ceiling is restored once `calculate_min_required_fee` reflects chain-time reality. - PA-004c: document that the spec's `Skipped` registry status doesn't exist (`EntryStatus` is `Active | Failed`); the no-broadcast counter / bank-nonce-consumption hook also isn't wired. Test pins `Ok + registry-removed`, the strongest contract the harness offers. - PA-007: flag that the negative variant ("disconnect from DAPI, expect typed network error") is uncovered today — needs per-test SDK with a swappable DAPI URL, not feasible against the current shared-singleton Sdk. Verification: - cargo check -p platform-wallet --tests: PASS - cargo clippy -p platform-wallet --tests --all-features -- -D warnings: PASS - cargo fmt -p platform-wallet --check: PASS No production code touched; no e2e tests executed (no testnet bank in this env). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tation QA-007 (HIGH): the spec called for 16 funding rounds; the implementation runs 4 (chain RTT × 16 ≈ 8 min exceeds P1 runtime budget). Accept the compromise. Status field and Scenario/Assertions updated to reflect 4 rounds and 5 pairwise-distinct addresses. Rationale notes that sustained rotation through the full DIP-17 gap window is untested at this tier. QA-008 (MEDIUM): struck the assertion `signer.cached_key_count() >= 17` from the Assertions list. SimpleSigner exposes no such accessor; the reference was to SeedBackedIdentitySigner, an unrelated type. Updated "Harness extensions required" to "none" accordingly. Findings from Marvin-QA spec-deviation audit (commit 4007b20cad). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Implements the three P0 Platform-Address (PA) test cases from
packages/rs-platform-wallet/tests/e2e/TEST_SPEC.md§3, the highest-prioritytier in the spec's P0/P1/P2 scheme. Builds on PR #3563's spec scaffolding;
PR #3549 (the harness) and PR #3554 (auto-select-inputs) ship the framework
and protocol-side fixes the cases exercise.
The earlier
cases/transfer.rshappy-path test had drifted from the spec(it covered roughly half of PA-001 and most of PA-002 in one place). This
PR replaces it with three spec-aligned test files, one per case.
What was done?
cases/pa_001_multi_output.rscases/pa_002_partial_fund.rscases/pa_004_sweep_back.rsPinned contracts:
[ReduceOutput(0)]the lex-larger output arrives at gross exactly while the lex-smaller absorbs the chain-time fee.Σ inputs == Σ outputsvalidated againstaddr_1's residual change.aaf8be74eeand9ea9e7033c—addr_1retainsfunded − bank_fee − transfer_grossafter a partial-balance transfer.cleanup::teardown_oneonly removes the entry on a successful sweep, so aNonepost-teardown implies the on-chain transition landed. Cross-test bank-balance accounting is documented as out of scope (other tests' sweeps interleave on the shared bank within this test's window); an aggregate "bank drain across a run" probe would belong to a separate harness self-test.Implementation notes
next_unused_addressparks the cursor until each derived address is observed used (the PA-005 invariant). PA-001 needs three distinct addresses, so it inserts a "prep" 1in/1out transfer to markaddr_2used before derivingaddr_3.#3040comment block. Each case calls out the bumped values and why.should_prefix); thepa_NNN_*prefix gives a one-glance cross-ref to the spec.How Has This Been Tested?
Live testnet run (operator-provisioned bank wallet):
pa_001_multi_output_transferpa_002_partial_fund_changepa_004_sweep_back_drains_to_bankcargo check -p platform-wallet --tests,cargo clippy -p platform-wallet --tests --all-features -- -D warnings, andcargo fmt -p platform-wallet --checkall clean.Breaking Changes
None. The replaced
cases/transfer.rshad no external consumers.Checklist:
For repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent