test(platform-wallet): implement PA P0 — multi-output, partial-fund, sweep-back#3571
Conversation
|
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:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
|
✅ Review complete (commit 346d404) |
Squashed delta of feat/rs-platform-wallet-e2e-cases-pa rebased onto fix/rs-platform-wallet-arithmetic-and-sync-hardening. The src/ and framework changes from this branch's history were already present upstream via feat/rs-platform-wallet-e2e merges, so only the PA-specific test cases and supporting helpers land here as new content. Net additions: - tests/e2e/cases/pa_001..pa_010, pa_3040_bug_pin (PA test suite) - tests/e2e/framework/wallet_factory.rs build_transfer_st_bytes (no-broadcast variant for PA-006b concurrent-broadcast race) - tests/e2e/framework/bank.rs FUNDING_MUTEX instrumentation - tests/e2e/framework/cleanup.rs dust gate - TEST_SPEC.md PA Status entries + Harness-ID-1, ID-001b, ID-003b Original branch tip: fbf268063d (consolidated spec content already on PR #3549's 74b1ed7). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
lklimek
force-pushed
the
feat/rs-platform-wallet-e2e-cases-pa
branch
from
1982e9b to
346d404
Compare
lklimek
changed the base branch from
feat/rs-platform-wallet-e2e
to
fix/rs-platform-wallet-arithmetic-and-sync-hardening
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Three solid issues stand out across the PR: (1) the new setup_with_n_identities helper waits for and re-spends a gross funding amount that the bank's [ReduceOutput(0)] strategy will never actually deliver, so any future caller will time out or hit insufficient-funds; (2) pa_002's module docstring claims #[ignore] gating that the function attribute does not provide, misleading contributors; (3) teardown_one removes the registry seed entry even though sweep_identities is currently a documented no-op, leaking identity-credit recoverability across process restarts. Several smaller suggestions (lift-loop ordering in balance_explicit_inputs, changeset fee semantic drift on merge, accessor overflow, brittle substring matching, pervasive saturating_sub in tests) round out the review. No P0 production-path defects.
Reviewed commit: 1982e9b
🔴 1 blocking | 🟡 4 suggestion(s) | 💬 3 nitpick(s)
6 additional findings
🔴 blocking: `setup_with_n_identities` waits for and spends an unreachable gross funding amount
packages/rs-platform-wallet/tests/e2e/framework/mod.rs (lines 206-225)
bank.fund_address(&funding_addr, funding_per) runs through default_fee_strategy() = [ReduceOutput(0)] (wallet_factory.rs:456-458, bank.rs:271-281), so the funded address actually receives funding_per − bank_fee on chain — never the gross funding_per. The helper then calls wait_for_balance(..., funding_per, 60s) (>= expected semantics) which can never satisfy, and immediately passes the same gross funding_per into register_identity_from_addresses as the funding amount, which will likewise fail explicit-input balancing once it tries to spend funding_per from an under-funded address.
The helper has no in-PR callers, so PA-001/002/004 are not affected, but as shipped this is a latent dead-on-arrival public API. Every other PA case that funds an address pins a *_FLOOR strictly below the gross *_CREDITS (e.g. pa_002_partial_fund.rs FUNDING_FLOOR = 70_000_000 vs FUNDING_CREDITS = 100_000_000). Either wait on a post-fee floor and pass the realistic post-fee amount into registration, or have the helper reserve extra headroom in funding_per to absorb the bank's fee.
🟡 suggestion: Module docstring claims `#[ignore]` gating that the test does not have
packages/rs-platform-wallet/tests/e2e/cases/pa_002_partial_fund.rs (lines 14-28)
Lines 14-21 promise contributors that this case is #[ignore]'d so a stock cargo test -p platform-wallet (or workspace-wide invocation) stays green without a funded testnet bank, live DAPI, or operator .env. The function on line 88 only carries #[tokio_shared_rt::test(shared)] — there is no #[ignore] attribute (compare with pa_010_bank_starvation.rs:40-42 and pa_001b_change_address_branch.rs which do carry explicit #[ignore = "BLOCKED — …"]). Without bank credentials, setup().expect("e2e setup failed") panics, exactly the failure mode the docstring promises is suppressed.
PA-001 and PA-004 don't claim ignore-gating in their docstrings, so the convention in this PR is that the live PA tests run by default; the prose here is what's wrong. Either remove the misleading paragraph or add an #[ignore = "…"] attribute to actually match it.
🟡 suggestion: `teardown_one` removes registry seed even though `sweep_identities` is a no-op
packages/rs-platform-wallet/tests/e2e/framework/cleanup.rs (lines 180-196)
teardown_one calls sweep_identities(test_wallet.platform_wallet()).await? (currently Ok(()) at line 345-347) and then unconditionally registry.remove(&test_wallet.id())? (line 187). Once identity credits are tracked but not swept, dropping the seed entry from the registry strips the only off-disk path back to those credits if the process restarts before a follow-up PR implements identity sweeping. MultiIdentitySetupGuard documents the leak deliberately, but that doc covers the credit balance; the registry-side erasure means the seed itself is gone, so even a future sweep implementation can't reconcile orphans for runs that completed against the current code.
Gate the registry.remove on sweep_identities actually returning success-with-progress (e.g. an enum SweepOutcome::{Drained, Skipped}), or skip removal when identity-credit cleanup is a no-op, so the registry entry survives until real sweeping lands.
🟡 suggestion: `PlatformAddressChangeSet::estimated_min_fee` mixes per-transfer and aggregate semantics across `Merge`
packages/rs-platform-wallet/src/changeset/changeset.rs (lines 585-645)
fee is documented as the lower-bound static fee estimate for the transfer that produced this changeset, and estimated_min_fee() advertises that contract. Merge::merge (line 644) unconditionally saturating_adds other.fee, so after one merge the value no longer represents "this transfer's estimate" — it's an aggregate across operations, with no type-level signal to callers that the invariant changed. A consumer reading changeset.estimated_min_fee() after a batched merge silently gets a sum and is likely to treat it as a per-transition estimate.
Either (a) drop fee from Merge (sync-only merges already preserve the originating value when other.fee == 0, but transfer-on-transfer merges currently double-count), (b) split into a per_op_fee: Vec<Credits> so callers can ask per-operation vs aggregate explicitly, or (c) rename to accumulated_min_fee and document the aggregate semantics. The current shape is the kind of semantic drift Rust's type system is good at preventing.
🟡 suggestion: `balance_explicit_inputs` lift pass is single-shot; donor headroom can drift across iterations
packages/rs-platform-wallet/tests/e2e/framework/wallet_factory.rs (lines 530-556)
needs_lift is captured once before the loop (line 532), but the donor predicate inside the loop reads the live shares map, which earlier iterations may have already drained below min_input + deficit. With three sub-floor shares (A, B, C) and one fat donor D, the first lift drains D for A; the second iteration may then find no donor with sufficient headroom and return Err(FrameworkError::Wallet(...)), even though processing the largest deficit first (or iterating to a fixed point) would have succeeded. The unit test on lines 721-744 covers a 2-input case which dodges the ordering sensitivity. Today only auto_select_inputs calls into this helper for >2 input shapes, so the bug is latent — but if the helper grows multi-input call sites, sort needs_lift by deficit descending or iterate-to-fixed-point so the order-dependence stops being load-bearing.
💬 nitpick: `total_credit_balance` can panic on u64 overflow
packages/rs-platform-wallet/src/wallet/identity/state/manager/accessors.rs (lines 83-95)
total_credit_balance sums identity balances with Iterator::sum::<u64>() (lines 84-93) and then with the unchecked + operator on line 94. The total credit pool is far below u64::MAX in practice, but this is a read accessor invokable from FFI/UI paths where a panic from a corrupted ManagedIdentity cache would propagate as a hard crash rather than a recoverable error. Use checked_add (return Option<u64>) or saturating_add to clamp.
🤖 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-225: `setup_with_n_identities` waits for and spends an unreachable gross funding amount
`bank.fund_address(&funding_addr, funding_per)` runs through `default_fee_strategy()` = `[ReduceOutput(0)]` (wallet_factory.rs:456-458, bank.rs:271-281), so the funded address actually receives `funding_per − bank_fee` on chain — never the gross `funding_per`. The helper then calls `wait_for_balance(..., funding_per, 60s)` (`>= expected` semantics) which can never satisfy, and immediately passes the same gross `funding_per` into `register_identity_from_addresses` as the funding amount, which will likewise fail explicit-input balancing once it tries to spend `funding_per` from an under-funded address.
The helper has no in-PR callers, so PA-001/002/004 are not affected, but as shipped this is a latent dead-on-arrival public API. Every other PA case that funds an address pins a `*_FLOOR` strictly below the gross `*_CREDITS` (e.g. `pa_002_partial_fund.rs` `FUNDING_FLOOR = 70_000_000` vs `FUNDING_CREDITS = 100_000_000`). Either wait on a post-fee floor and pass the realistic post-fee amount into registration, or have the helper reserve extra headroom in `funding_per` to absorb the bank's fee.
In `packages/rs-platform-wallet/tests/e2e/cases/pa_002_partial_fund.rs`:
- [SUGGESTION] lines 14-28: Module docstring claims `#[ignore]` gating that the test does not have
Lines 14-21 promise contributors that this case is `#[ignore]`'d so a stock `cargo test -p platform-wallet` (or workspace-wide invocation) stays green without a funded testnet bank, live DAPI, or operator `.env`. The function on line 88 only carries `#[tokio_shared_rt::test(shared)]` — there is no `#[ignore]` attribute (compare with `pa_010_bank_starvation.rs:40-42` and `pa_001b_change_address_branch.rs` which do carry explicit `#[ignore = "BLOCKED — …"]`). Without bank credentials, `setup().expect("e2e setup failed")` panics, exactly the failure mode the docstring promises is suppressed.
PA-001 and PA-004 don't claim ignore-gating in their docstrings, so the convention in this PR is that the live PA tests run by default; the prose here is what's wrong. Either remove the misleading paragraph or add an `#[ignore = "…"]` attribute to actually match it.
In `packages/rs-platform-wallet/tests/e2e/framework/cleanup.rs`:
- [SUGGESTION] lines 180-196: `teardown_one` removes registry seed even though `sweep_identities` is a no-op
`teardown_one` calls `sweep_identities(test_wallet.platform_wallet()).await?` (currently `Ok(())` at line 345-347) and then unconditionally `registry.remove(&test_wallet.id())?` (line 187). Once identity credits are tracked but not swept, dropping the seed entry from the registry strips the only off-disk path back to those credits if the process restarts before a follow-up PR implements identity sweeping. `MultiIdentitySetupGuard` documents the leak deliberately, but that doc covers the *credit balance*; the registry-side erasure means the seed itself is gone, so even a future sweep implementation can't reconcile orphans for runs that completed against the current code.
Gate the `registry.remove` on `sweep_identities` actually returning success-with-progress (e.g. an enum `SweepOutcome::{Drained, Skipped}`), or skip removal when identity-credit cleanup is a no-op, so the registry entry survives until real sweeping lands.
In `packages/rs-platform-wallet/src/changeset/changeset.rs`:
- [SUGGESTION] lines 585-645: `PlatformAddressChangeSet::estimated_min_fee` mixes per-transfer and aggregate semantics across `Merge`
`fee` is documented as the lower-bound static fee estimate for *the* transfer that produced this changeset, and `estimated_min_fee()` advertises that contract. `Merge::merge` (line 644) unconditionally `saturating_add`s `other.fee`, so after one merge the value no longer represents "this transfer's estimate" — it's an aggregate across operations, with no type-level signal to callers that the invariant changed. A consumer reading `changeset.estimated_min_fee()` after a batched merge silently gets a sum and is likely to treat it as a per-transition estimate.
Either (a) drop `fee` from `Merge` (sync-only merges already preserve the originating value when `other.fee == 0`, but transfer-on-transfer merges currently double-count), (b) split into a `per_op_fee: Vec<Credits>` so callers can ask per-operation vs aggregate explicitly, or (c) rename to `accumulated_min_fee` and document the aggregate semantics. The current shape is the kind of semantic drift Rust's type system is good at preventing.
In `packages/rs-platform-wallet/tests/e2e/framework/wallet_factory.rs`:
- [SUGGESTION] lines 530-556: `balance_explicit_inputs` lift pass is single-shot; donor headroom can drift across iterations
`needs_lift` is captured once before the loop (line 532), but the donor predicate inside the loop reads the live `shares` map, which earlier iterations may have already drained below `min_input + deficit`. With three sub-floor shares (A, B, C) and one fat donor D, the first lift drains D for A; the second iteration may then find no donor with sufficient headroom and `return Err(FrameworkError::Wallet(...))`, even though processing the largest deficit first (or iterating to a fixed point) would have succeeded. The unit test on lines 721-744 covers a 2-input case which dodges the ordering sensitivity. Today only `auto_select_inputs` calls into this helper for >2 input shapes, so the bug is latent — but if the helper grows multi-input call sites, sort `needs_lift` by deficit descending or iterate-to-fixed-point so the order-dependence stops being load-bearing.
| let err_string = format!("{replay_err}").to_lowercase(); | ||
| let dbg_string = format!("{replay_err:?}").to_lowercase(); | ||
| let class_match = matches!(replay_err, dash_sdk::Error::AlreadyExists(_)) | ||
| || [ | ||
| "already exists", | ||
| "alreadyexists", | ||
| "stale nonce", | ||
| "invalididentitynonce", | ||
| "duplicate", | ||
| ] | ||
| .iter() | ||
| .any(|needle| err_string.contains(needle) || dbg_string.contains(needle)); | ||
| assert!( | ||
| class_match, | ||
| "PA-006: replay error must be of stale-nonce / already-exists class; \ | ||
| got display={replay_err}, debug={replay_err:?}" | ||
| ); |
There was a problem hiding this comment.
💬 Nitpick: Replay-rejection assertion falls back to substring matching on error display
The contract this test pins is "the second submission of the same (input, nonce) is rejected with stale-nonce / already-exists semantics". The assertion correctly tries the typed variant first (matches!(replay_err, dash_sdk::Error::AlreadyExists(_))) but ORs in substring matching on err_string / dbg_string against "already exists" | "alreadyexists" | "stale nonce" | "invalididentitynonce" | "duplicate". A future error-message refactor that changes wording (e.g. switches to "replay detected") would silently make the test pass on a different error class without anyone noticing. If multiple typed variants can fire today, enumerate them with matches!(replay_err, A | B | C) so the type system flags removals; otherwise leave a TODO and tracking issue to remove the substring fallback.
source: ['claude']
| let lo_delta = lex_lo_post.saturating_sub(lex_lo_pre); | ||
| let hi_delta = lex_hi_post.saturating_sub(lex_hi_pre); | ||
| let multi_fee = OUTPUT_A_CREDITS.saturating_sub(lo_delta); | ||
| let addr_1_drain = addr_1_pre.saturating_sub(addr_1_post); |
There was a problem hiding this comment.
💬 Nitpick: Pervasive saturating_sub/saturating_add in test deltas masks arithmetic regressions
The new test files (pa_001, pa_002, pa_002b, pa_006, pa_004) compute pre/post deltas with saturating_sub and sums with saturating_add. Production code wants overflow-tolerant arithmetic; tests want the opposite — an off-by-one that flips addr_1_post > addr_1_pre should panic, not silently produce 0 and let downstream assertions accept it. Use checked operators or plain -/+ so regressions in either direction surface as a loud failure.
source: ['claude']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
REQUEST_CHANGES. Four blocking issues: (1) 18 of the 21 newly added PA cases dropped the #[ignore] gate that the prior transfer.rs carried and that the README §Prerequisites contractually documents — stock cargo test -p platform-wallet (and any CI without PLATFORM_WALLET_E2E_BANK_MNEMONIC) will panic in setup().expect(...); (2) pa_3040_bug_pin is documented as failing-by-design until protocol issue #3040 is fixed but ships ungated, so any --ignored opt-in run also goes red; (3) the new build_transfer_st_bytes helper omits the balance_explicit_inputs call that its sibling transfer_capturing_st_bytes runs to satisfy the protocol's Σinputs==Σoutputs check, so PA-006b will fail with InputOutputBalanceMismatchError (not the stale-nonce class it asserts); (4) PA-002's headline Σ inputs == Σ outputs assertion is mathematically a tautology — expected_change simplifies algebraically to remaining, so the test pins nothing about the invariant the PR claims it pins. Two non-blocking issues: PA-008c reads a process-global mutex history without per-test scoping (only safe under --test-threads=1), and the new setup_with_n_identities helper waits for a gross balance the bank's [ReduceOutput(0)] strategy can never deliver (latent — no test calls it yet).
Reviewed commit: 346d404
🔴 4 blocking | 🟡 2 suggestion(s)
1 additional finding
🟡 suggestion: `setup_with_n_identities` waits for a gross balance the bank can never deliver
packages/rs-platform-wallet/tests/e2e/framework/mod.rs (lines 206-225)
BankWallet::fund_address (bank.rs:248-281) sends credits with default_fee_strategy() = [ReduceOutput(0)], so the recipient address receives credits − bank_fee, not the gross credits. wait_for_balance (wait.rs:67, 146) succeeds only when the observed balance is >= expected. This helper calls bank.fund_address(&funding_addr, funding_per) then wait_for_balance(&funding_addr, funding_per, ...) — the wait can never complete on any non-zero-fee network. Even if it did, register_identity_from_addresses(funding_addr, funding_per, ...) is then called with an input amount larger than the address actually owns (wallet_factory.rs:412-428 packs {funding_addr → funding_per} into the input map), which violates the address-funded registration contract.
No PA case currently uses this helper (only TEST_SPEC.md references it), so this is latent — but the helper is unusable as written the first time any test relies on it. Either pass a post-fee floor to wait_for_balance, then read the actual balance and forward THAT into register_identity_from_addresses, or change the bank funding strategy on this code path to deliver the exact gross.
💡 Suggested change
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?;
// Bank uses `[ReduceOutput(0)]`; recipient receives `funding_per − bank_fee`.
// Wait on a post-fee floor, then forward the OBSERVED balance into registration.
wait_for_balance(
&base.test_wallet,
&funding_addr,
1,
Duration::from_secs(60),
)
.await?;
base.test_wallet.sync_balances().await?;
let funded = base
.test_wallet
.balances()
.await
.get(&funding_addr)
.copied()
.ok_or_else(|| FrameworkError::Cleanup(format!(
"funding address {funding_addr:?} disappeared before identity registration"
)))?;
let registered = base
.test_wallet
.register_identity_from_addresses(funding_addr, funded, identity_index)
.await?;
identities.push(registered);
}
🤖 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/pa_002_partial_fund.rs`:
- [BLOCKING] lines 88-89: PA cases dropped `#[ignore]` gate — stock `cargo test` will panic without bank mnemonic
The prior `cases/transfer.rs` (`git show 85cfeb3885`) carried `#[ignore = "requires PLATFORM_WALLET_E2E_BANK_MNEMONIC and live testnet access; run with `cargo test -- --ignored`"]`. README §Prerequisites contractually documents this gating: "Tests are gated behind `#[ignore]` so a stock `cargo test` (or workspace-wide invocation) stays green for contributors and CI jobs that lack a funded testnet bank wallet". The new PA-002 docstring (lines 14-21) still claims the same gate, but the attribute is missing on the function. A grep across the new cases shows the gate is retained only on the BLOCKED-class tests (`pa_001b`, `pa_005b`, `pa_010`). The other 18 (`pa_001`, `pa_001c`, `pa_002`, `pa_002b`, `pa_003`, `pa_004`, `pa_004b`, `pa_004c`, `pa_005`, `pa_006`, `pa_006b`, `pa_007`, `pa_007b`, `pa_008`, `pa_008b`, `pa_008c`, `pa_009`, `pa_3040`) are now active in the default `e2e` binary and will panic at `setup().expect("e2e setup failed")` (line 98 here) when `PLATFORM_WALLET_E2E_BANK_MNEMONIC` is unset, breaking the documented contract for contributors and CI.
- [BLOCKING] lines 210-217: PA-002's `Σ inputs == Σ outputs` assertion is a tautology — pins nothing
`expected_change` simplifies algebraically to `remaining`, so `assert_eq!(remaining, expected_change)` is unconditionally true regardless of what the wallet shipped on chain.
Unfolding the test's own definitions:
- `observed_total = received + remaining`
- `total_fees = FUNDING_CREDITS − observed_total = FUNDING_CREDITS − received − remaining`
- `transfer_fee = TRANSFER_CREDITS − received`
- `bank_fee = total_fees − transfer_fee = FUNDING_CREDITS − remaining − TRANSFER_CREDITS`
- `expected_change = FUNDING_CREDITS − bank_fee − TRANSFER_CREDITS = remaining`
The assertion reduces to `assert_eq!(remaining, remaining)`. It cannot detect a regression of the Σinputs == Σoutputs invariant the PR description specifically calls out as the contract this case is meant to pin (referenced commits aaf8be74ee / 9ea9e7033c). The earlier `received < TRANSFER_CREDITS` + fee-ceiling assertions already cover everything this assertion can detect.
To actually pin the invariant, derive `expected_change` from an independent source — e.g. an upper-bound check `remaining >= FUNDING_CREDITS − BANK_FEE_CEILING − TRANSFER_CREDITS`, or cross-check `received + remaining + total_fees == FUNDING_CREDITS` with an independently-observed bank fee from a sweep, or assert `total_fees < BANK_FEE_CEILING + TRANSFER_FEE_CEILING` plus the existing per-fee bounds.
In `packages/rs-platform-wallet/tests/e2e/cases/pa_3040_bug_pin.rs`:
- [BLOCKING] lines 82-83: `pa_3040_bug_pin` is red-by-design — must be `#[ignore]`-gated or `#[should_panic]`-inverted
Module docstring (lines 35-39) explicitly states: "Today (#3040 unfixed): `transfer()` succeeds at the wallet layer (Phase 4 passes) but the broadcast is rejected by Drive with `AddressesNotEnoughFundsError`. The `.expect(\"self-transfer\")` then panics → test fails (red)." Even with the gating fix from the prior finding, an opt-in `--ignored` run will be red until protocol issue #3040 lands — the suite cannot stay green when used as a regression signal, and a real wallet-side regression in another PA case would be hidden by this perpetual red. Sibling BLOCKED tests (`pa_001b`, `pa_005b`, `pa_010`) use `#[ignore = "BLOCKED — …"]`; do the same here, or invert with `#[should_panic(expected = "AddressesNotEnoughFundsError")]` so the test will start failing-when-expected-to-pass the day #3040 is fixed and signals "time to remove this pin".
In `packages/rs-platform-wallet/tests/e2e/framework/wallet_factory.rs`:
- [BLOCKING] lines 311-338: `build_transfer_st_bytes` skips `balance_explicit_inputs` — PA-006b will fail by construction
The sibling `transfer_capturing_st_bytes` runs `balance_explicit_inputs(&inputs, &outputs, platform_version)` at line 271 specifically because `AddressFundsTransferTransition` validation rejects with `InputOutputBalanceMismatchError` unless Σinputs == Σoutputs in the encoded payload (the helper's own doc at lines 463-475, and the equality enforced in DPP state-transition validation). `build_transfer_st_bytes` forwards the caller's `inputs` map straight to `try_from_inputs_with_signer`, which is a pure builder — it does NOT rebalance.
PA-006b at `pa_006b_concurrent_broadcast.rs:88-94` passes `inputs = {addr_src: addr_src_pre}` (≈85M after bank-fee deduction) against `outputs = {addr_dst: TRANSFER_CREDITS = 50_000_000}`. The signed bytes therefore carry Σinputs (~85M) ≠ Σoutputs (50M) and both concurrent broadcasts will be rejected with the balance-mismatch error class — not the stale-nonce / `AlreadyExists` class the test asserts on (`pa_006b_concurrent_broadcast.rs:138-160`). The `ok_count == 1` assertion will see 0 successes and the `class_match` assertion will not hit any of the targeted needles. The PR's own test report shows only PA-001/002/004 ran live; PA-006b appears not to have been exercised against testnet, which would have surfaced this immediately.
In `packages/rs-platform-wallet/tests/e2e/cases/pa_008c_funding_mutex_observable.rs`:
- [SUGGESTION] lines 124-172: PA-008c's `history.len() == 3` assertion races a process-global buffer
`FUNDING_MUTEX_HISTORY` is a process-global `SyncMutex<VecDeque<FundingMutexHistoryEntry>>` (bank.rs:59) shared by every test that calls `bank.fund_address`. PA-008c drains it before its `tokio::join!` (line 126) and again after balances are observed (line 155) and asserts exactly three entries. The pre-drain only removes past entries; nothing prevents a sibling test running on another worker thread from appending entries between line 126 and line 155, which makes `assert_eq!(history.len(), 3)` nondeterministic under default `cargo test` parallelism. The PR specifies `--test-threads=1`, so the case is sound under the documented runner, but the implicit single-thread requirement is invisible to a future maintainer running with default test-threads — they will see flaky failures unrelated to the mutex contract. Either acquire a per-test serialization handle in `setup()` and drop it in `teardown()`, or tag history entries with a per-test id and filter on read so the case is correct under any runner config. At minimum, add a `compile_error!` or runtime assertion that detects parallel test threads and gates the assertion accordingly.
In `packages/rs-platform-wallet/tests/e2e/framework/mod.rs`:
- [SUGGESTION] lines 206-225: `setup_with_n_identities` waits for a gross balance the bank can never deliver
`BankWallet::fund_address` (bank.rs:248-281) sends `credits` with `default_fee_strategy()` = `[ReduceOutput(0)]`, so the recipient address receives `credits − bank_fee`, not the gross `credits`. `wait_for_balance` (wait.rs:67, 146) succeeds only when the observed balance is `>= expected`. This helper calls `bank.fund_address(&funding_addr, funding_per)` then `wait_for_balance(&funding_addr, funding_per, ...)` — the wait can never complete on any non-zero-fee network. Even if it did, `register_identity_from_addresses(funding_addr, funding_per, ...)` is then called with an input amount larger than the address actually owns (wallet_factory.rs:412-428 packs `{funding_addr → funding_per}` into the input map), which violates the address-funded registration contract.
No PA case currently uses this helper (only TEST_SPEC.md references it), so this is latent — but the helper is unusable as written the first time any test relies on it. Either pass a post-fee floor to `wait_for_balance`, then read the actual balance and forward THAT into `register_identity_from_addresses`, or change the bank funding strategy on this code path to deliver the exact gross.
| #[tokio_shared_rt::test(shared)] | ||
| #[ignore = "requires PLATFORM_WALLET_E2E_BANK_MNEMONIC and live testnet access; run with `cargo test -- --ignored`"] | ||
| async fn transfer_between_two_platform_addresses() { | ||
| async fn pa_002_partial_fund_change() { |
There was a problem hiding this comment.
🔴 Blocking: PA cases dropped #[ignore] gate — stock cargo test will panic without bank mnemonic
The prior cases/transfer.rs (git show 85cfeb3885) carried #[ignore = "requires PLATFORM_WALLET_E2E_BANK_MNEMONIC and live testnet access; run with cargo test -- --ignored"]. README §Prerequisites contractually documents this gating: "Tests are gated behind #[ignore] so a stock cargo test (or workspace-wide invocation) stays green for contributors and CI jobs that lack a funded testnet bank wallet". The new PA-002 docstring (lines 14-21) still claims the same gate, but the attribute is missing on the function. A grep across the new cases shows the gate is retained only on the BLOCKED-class tests (pa_001b, pa_005b, pa_010). The other 18 (pa_001, pa_001c, pa_002, pa_002b, pa_003, pa_004, pa_004b, pa_004c, pa_005, pa_006, pa_006b, pa_007, pa_007b, pa_008, pa_008b, pa_008c, pa_009, pa_3040) are now active in the default e2e binary and will panic at setup().expect("e2e setup failed") (line 98 here) when PLATFORM_WALLET_E2E_BANK_MNEMONIC is unset, breaking the documented contract for contributors and CI.
💡 Suggested change
| #[tokio_shared_rt::test(shared)] | |
| #[ignore = "requires PLATFORM_WALLET_E2E_BANK_MNEMONIC and live testnet access; run with `cargo test -- --ignored`"] | |
| async fn transfer_between_two_platform_addresses() { | |
| async fn pa_002_partial_fund_change() { | |
| #[tokio_shared_rt::test(shared)] | |
| #[ignore = "requires PLATFORM_WALLET_E2E_BANK_MNEMONIC and live testnet access; run with `cargo test -- --ignored`"] | |
| async fn pa_002_partial_fund_change() { |
source: ['claude-general', 'codex-general', 'claude-rust-quality']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/tests/e2e/cases/pa_002_partial_fund.rs`:
- [BLOCKING] lines 88-89: PA cases dropped `#[ignore]` gate — stock `cargo test` will panic without bank mnemonic
The prior `cases/transfer.rs` (`git show 85cfeb3885`) carried `#[ignore = "requires PLATFORM_WALLET_E2E_BANK_MNEMONIC and live testnet access; run with `cargo test -- --ignored`"]`. README §Prerequisites contractually documents this gating: "Tests are gated behind `#[ignore]` so a stock `cargo test` (or workspace-wide invocation) stays green for contributors and CI jobs that lack a funded testnet bank wallet". The new PA-002 docstring (lines 14-21) still claims the same gate, but the attribute is missing on the function. A grep across the new cases shows the gate is retained only on the BLOCKED-class tests (`pa_001b`, `pa_005b`, `pa_010`). The other 18 (`pa_001`, `pa_001c`, `pa_002`, `pa_002b`, `pa_003`, `pa_004`, `pa_004b`, `pa_004c`, `pa_005`, `pa_006`, `pa_006b`, `pa_007`, `pa_007b`, `pa_008`, `pa_008b`, `pa_008c`, `pa_009`, `pa_3040`) are now active in the default `e2e` binary and will panic at `setup().expect("e2e setup failed")` (line 98 here) when `PLATFORM_WALLET_E2E_BANK_MNEMONIC` is unset, breaking the documented contract for contributors and CI.
| #[tokio_shared_rt::test(shared)] | ||
| async fn pa_3040_reduce_output_chain_time_fee_must_not_exceed_static_estimate() { |
There was a problem hiding this comment.
🔴 Blocking: pa_3040_bug_pin is red-by-design — must be #[ignore]-gated or #[should_panic]-inverted
Module docstring (lines 35-39) explicitly states: "Today (#3040 unfixed): transfer() succeeds at the wallet layer (Phase 4 passes) but the broadcast is rejected by Drive with AddressesNotEnoughFundsError. The .expect(\"self-transfer\") then panics → test fails (red)." Even with the gating fix from the prior finding, an opt-in --ignored run will be red until protocol issue #3040 lands — the suite cannot stay green when used as a regression signal, and a real wallet-side regression in another PA case would be hidden by this perpetual red. Sibling BLOCKED tests (pa_001b, pa_005b, pa_010) use #[ignore = "BLOCKED — …"]; do the same here, or invert with #[should_panic(expected = "AddressesNotEnoughFundsError")] so the test will start failing-when-expected-to-pass the day #3040 is fixed and signals "time to remove this pin".
💡 Suggested change
| #[tokio_shared_rt::test(shared)] | |
| async fn pa_3040_reduce_output_chain_time_fee_must_not_exceed_static_estimate() { | |
| #[tokio_shared_rt::test(shared)] | |
| #[ignore = "BUG-PIN — fails-by-design until platform #3040 is fixed; run manually with `cargo test -- --ignored pa_3040`"] | |
| async fn pa_3040_reduce_output_chain_time_fee_must_not_exceed_static_estimate() { |
source: ['claude-general', 'codex-general', 'claude-rust-quality']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/tests/e2e/cases/pa_3040_bug_pin.rs`:
- [BLOCKING] lines 82-83: `pa_3040_bug_pin` is red-by-design — must be `#[ignore]`-gated or `#[should_panic]`-inverted
Module docstring (lines 35-39) explicitly states: "Today (#3040 unfixed): `transfer()` succeeds at the wallet layer (Phase 4 passes) but the broadcast is rejected by Drive with `AddressesNotEnoughFundsError`. The `.expect(\"self-transfer\")` then panics → test fails (red)." Even with the gating fix from the prior finding, an opt-in `--ignored` run will be red until protocol issue #3040 lands — the suite cannot stay green when used as a regression signal, and a real wallet-side regression in another PA case would be hidden by this perpetual red. Sibling BLOCKED tests (`pa_001b`, `pa_005b`, `pa_010`) use `#[ignore = "BLOCKED — …"]`; do the same here, or invert with `#[should_panic(expected = "AddressesNotEnoughFundsError")]` so the test will start failing-when-expected-to-pass the day #3040 is fixed and signals "time to remove this pin".
| pub async fn build_transfer_st_bytes( | ||
| &self, | ||
| outputs: BTreeMap<PlatformAddress, Credits>, | ||
| inputs: BTreeMap<PlatformAddress, Credits>, | ||
| ) -> FrameworkResult<Vec<u8>> { | ||
| use dash_sdk::platform::transition::address_inputs::{fetch_inputs_with_nonce, nonce_inc}; | ||
| use dpp::serialization::PlatformSerializable; | ||
| use dpp::state_transition::address_funds_transfer_transition::methods::AddressFundsTransferTransitionMethodsV0; | ||
| use dpp::state_transition::address_funds_transfer_transition::AddressFundsTransferTransition; | ||
|
|
||
| let inputs_with_nonce = fetch_inputs_with_nonce(self.wallet.sdk(), &inputs) | ||
| .await | ||
| .map_err(|err| FrameworkError::Wallet(format!("nonce fetch: {err}")))?; | ||
| let inputs_with_nonce = nonce_inc(inputs_with_nonce); | ||
|
|
||
| let st = AddressFundsTransferTransition::try_from_inputs_with_signer( | ||
| inputs_with_nonce, | ||
| outputs, | ||
| default_fee_strategy(), | ||
| &self.signer, | ||
| Default::default(), | ||
| PlatformVersion::latest(), | ||
| ) | ||
| .await | ||
| .map_err(|err| FrameworkError::Wallet(format!("st build: {err}")))?; | ||
| PlatformSerializable::serialize_to_bytes(&st) | ||
| .map_err(|err| FrameworkError::Wallet(format!("st serialize: {err}"))) | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: build_transfer_st_bytes skips balance_explicit_inputs — PA-006b will fail by construction
The sibling transfer_capturing_st_bytes runs balance_explicit_inputs(&inputs, &outputs, platform_version) at line 271 specifically because AddressFundsTransferTransition validation rejects with InputOutputBalanceMismatchError unless Σinputs == Σoutputs in the encoded payload (the helper's own doc at lines 463-475, and the equality enforced in DPP state-transition validation). build_transfer_st_bytes forwards the caller's inputs map straight to try_from_inputs_with_signer, which is a pure builder — it does NOT rebalance.
PA-006b at pa_006b_concurrent_broadcast.rs:88-94 passes inputs = {addr_src: addr_src_pre} (≈85M after bank-fee deduction) against outputs = {addr_dst: TRANSFER_CREDITS = 50_000_000}. The signed bytes therefore carry Σinputs (~85M) ≠ Σoutputs (50M) and both concurrent broadcasts will be rejected with the balance-mismatch error class — not the stale-nonce / AlreadyExists class the test asserts on (pa_006b_concurrent_broadcast.rs:138-160). The ok_count == 1 assertion will see 0 successes and the class_match assertion will not hit any of the targeted needles. The PR's own test report shows only PA-001/002/004 ran live; PA-006b appears not to have been exercised against testnet, which would have surfaced this immediately.
💡 Suggested change
| pub async fn build_transfer_st_bytes( | |
| &self, | |
| outputs: BTreeMap<PlatformAddress, Credits>, | |
| inputs: BTreeMap<PlatformAddress, Credits>, | |
| ) -> FrameworkResult<Vec<u8>> { | |
| use dash_sdk::platform::transition::address_inputs::{fetch_inputs_with_nonce, nonce_inc}; | |
| use dpp::serialization::PlatformSerializable; | |
| use dpp::state_transition::address_funds_transfer_transition::methods::AddressFundsTransferTransitionMethodsV0; | |
| use dpp::state_transition::address_funds_transfer_transition::AddressFundsTransferTransition; | |
| let inputs_with_nonce = fetch_inputs_with_nonce(self.wallet.sdk(), &inputs) | |
| .await | |
| .map_err(|err| FrameworkError::Wallet(format!("nonce fetch: {err}")))?; | |
| let inputs_with_nonce = nonce_inc(inputs_with_nonce); | |
| let st = AddressFundsTransferTransition::try_from_inputs_with_signer( | |
| inputs_with_nonce, | |
| outputs, | |
| default_fee_strategy(), | |
| &self.signer, | |
| Default::default(), | |
| PlatformVersion::latest(), | |
| ) | |
| .await | |
| .map_err(|err| FrameworkError::Wallet(format!("st build: {err}")))?; | |
| PlatformSerializable::serialize_to_bytes(&st) | |
| .map_err(|err| FrameworkError::Wallet(format!("st serialize: {err}"))) | |
| } | |
| pub async fn build_transfer_st_bytes( | |
| &self, | |
| outputs: BTreeMap<PlatformAddress, Credits>, | |
| inputs: BTreeMap<PlatformAddress, Credits>, | |
| ) -> FrameworkResult<Vec<u8>> { | |
| use dash_sdk::platform::transition::address_inputs::{fetch_inputs_with_nonce, nonce_inc}; | |
| use dpp::serialization::PlatformSerializable; | |
| use dpp::state_transition::address_funds_transfer_transition::methods::AddressFundsTransferTransitionMethodsV0; | |
| use dpp::state_transition::address_funds_transfer_transition::AddressFundsTransferTransition; | |
| let platform_version = PlatformVersion::latest(); | |
| let balanced_inputs = balance_explicit_inputs(&inputs, &outputs, platform_version)?; | |
| let inputs_with_nonce = fetch_inputs_with_nonce(self.wallet.sdk(), &balanced_inputs) | |
| .await | |
| .map_err(|err| FrameworkError::Wallet(format!("nonce fetch: {err}")))?; | |
| let inputs_with_nonce = nonce_inc(inputs_with_nonce); | |
| let st = AddressFundsTransferTransition::try_from_inputs_with_signer( | |
| inputs_with_nonce, | |
| outputs, | |
| default_fee_strategy(), | |
| &self.signer, | |
| Default::default(), | |
| platform_version, | |
| ) | |
| .await | |
| .map_err(|err| FrameworkError::Wallet(format!("st build: {err}")))?; | |
| PlatformSerializable::serialize_to_bytes(&st) | |
| .map_err(|err| FrameworkError::Wallet(format!("st serialize: {err}"))) | |
| } |
source: ['claude-general', 'codex-general', 'claude-rust-quality']
🤖 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/wallet_factory.rs`:
- [BLOCKING] lines 311-338: `build_transfer_st_bytes` skips `balance_explicit_inputs` — PA-006b will fail by construction
The sibling `transfer_capturing_st_bytes` runs `balance_explicit_inputs(&inputs, &outputs, platform_version)` at line 271 specifically because `AddressFundsTransferTransition` validation rejects with `InputOutputBalanceMismatchError` unless Σinputs == Σoutputs in the encoded payload (the helper's own doc at lines 463-475, and the equality enforced in DPP state-transition validation). `build_transfer_st_bytes` forwards the caller's `inputs` map straight to `try_from_inputs_with_signer`, which is a pure builder — it does NOT rebalance.
PA-006b at `pa_006b_concurrent_broadcast.rs:88-94` passes `inputs = {addr_src: addr_src_pre}` (≈85M after bank-fee deduction) against `outputs = {addr_dst: TRANSFER_CREDITS = 50_000_000}`. The signed bytes therefore carry Σinputs (~85M) ≠ Σoutputs (50M) and both concurrent broadcasts will be rejected with the balance-mismatch error class — not the stale-nonce / `AlreadyExists` class the test asserts on (`pa_006b_concurrent_broadcast.rs:138-160`). The `ok_count == 1` assertion will see 0 successes and the `class_match` assertion will not hit any of the targeted needles. The PR's own test report shows only PA-001/002/004 ran live; PA-006b appears not to have been exercised against testnet, which would have surfaced this immediately.
| let expected_change = FUNDING_CREDITS | ||
| .saturating_sub(bank_fee) | ||
| .saturating_sub(TRANSFER_CREDITS); | ||
| assert_eq!( | ||
| remaining, expected_change, | ||
| "addr_1 change must equal `FUNDING_CREDITS − bank_fee − TRANSFER_CREDITS` \ | ||
| (Σ inputs == Σ outputs invariant); expected {expected_change}, got {remaining}" | ||
| ); |
There was a problem hiding this comment.
🔴 Blocking: PA-002's Σ inputs == Σ outputs assertion is a tautology — pins nothing
expected_change simplifies algebraically to remaining, so assert_eq!(remaining, expected_change) is unconditionally true regardless of what the wallet shipped on chain.
Unfolding the test's own definitions:
observed_total = received + remainingtotal_fees = FUNDING_CREDITS − observed_total = FUNDING_CREDITS − received − remainingtransfer_fee = TRANSFER_CREDITS − receivedbank_fee = total_fees − transfer_fee = FUNDING_CREDITS − remaining − TRANSFER_CREDITSexpected_change = FUNDING_CREDITS − bank_fee − TRANSFER_CREDITS = remaining
The assertion reduces to assert_eq!(remaining, remaining). It cannot detect a regression of the Σinputs == Σoutputs invariant the PR description specifically calls out as the contract this case is meant to pin (referenced commits aaf8be7 / 9ea9e70). The earlier received < TRANSFER_CREDITS + fee-ceiling assertions already cover everything this assertion can detect.
To actually pin the invariant, derive expected_change from an independent source — e.g. an upper-bound check remaining >= FUNDING_CREDITS − BANK_FEE_CEILING − TRANSFER_CREDITS, or cross-check received + remaining + total_fees == FUNDING_CREDITS with an independently-observed bank fee from a sweep, or assert total_fees < BANK_FEE_CEILING + TRANSFER_FEE_CEILING plus the existing per-fee bounds.
source: ['claude-rust-quality']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/tests/e2e/cases/pa_002_partial_fund.rs`:
- [BLOCKING] lines 210-217: PA-002's `Σ inputs == Σ outputs` assertion is a tautology — pins nothing
`expected_change` simplifies algebraically to `remaining`, so `assert_eq!(remaining, expected_change)` is unconditionally true regardless of what the wallet shipped on chain.
Unfolding the test's own definitions:
- `observed_total = received + remaining`
- `total_fees = FUNDING_CREDITS − observed_total = FUNDING_CREDITS − received − remaining`
- `transfer_fee = TRANSFER_CREDITS − received`
- `bank_fee = total_fees − transfer_fee = FUNDING_CREDITS − remaining − TRANSFER_CREDITS`
- `expected_change = FUNDING_CREDITS − bank_fee − TRANSFER_CREDITS = remaining`
The assertion reduces to `assert_eq!(remaining, remaining)`. It cannot detect a regression of the Σinputs == Σoutputs invariant the PR description specifically calls out as the contract this case is meant to pin (referenced commits aaf8be74ee / 9ea9e7033c). The earlier `received < TRANSFER_CREDITS` + fee-ceiling assertions already cover everything this assertion can detect.
To actually pin the invariant, derive `expected_change` from an independent source — e.g. an upper-bound check `remaining >= FUNDING_CREDITS − BANK_FEE_CEILING − TRANSFER_CREDITS`, or cross-check `received + remaining + total_fees == FUNDING_CREDITS` with an independently-observed bank fee from a sweep, or assert `total_fees < BANK_FEE_CEILING + TRANSFER_FEE_CEILING` plus the existing per-fee bounds.
| // Drain whatever the markers + sibling tests recorded so the | ||
| // post-fan-in drain contains ONLY our three concurrent entries. | ||
| let _pre = s.ctx.bank().funding_mutex_history(); | ||
|
|
||
| // ---- Concurrent funds. PA-008's contract — but here we drain the | ||
| // history afterwards and assert observable serialisation. ---- | ||
| let bank = s.ctx.bank(); | ||
| let (r_a, r_b, r_c) = tokio::join!( | ||
| bank.fund_address(&addr_a, FUND_AMOUNT), | ||
| bank.fund_address(&addr_b, FUND_AMOUNT), | ||
| bank.fund_address(&addr_c, FUND_AMOUNT), | ||
| ); | ||
| r_a.expect("concurrent fund a"); | ||
| r_b.expect("concurrent fund b"); | ||
| r_c.expect("concurrent fund c"); | ||
|
|
||
| // Wait for each address to observe its concurrent fund so any | ||
| // sibling test that piggy-backs on FUNDING_MUTEX between the | ||
| // join and the drain doesn't pollute our window. wait_for_balance | ||
| // doesn't acquire FUNDING_MUTEX itself, so this is safe. | ||
| wait_for_balance(&s.test_wallet, &addr_a, FUND_FLOOR, STEP_TIMEOUT) | ||
| .await | ||
| .expect("addr_a never observed concurrent fund"); | ||
| wait_for_balance(&s.test_wallet, &addr_b, FUND_FLOOR, STEP_TIMEOUT) | ||
| .await | ||
| .expect("addr_b never observed concurrent fund"); | ||
| wait_for_balance(&s.test_wallet, &addr_c, FUND_FLOOR, STEP_TIMEOUT) | ||
| .await | ||
| .expect("addr_c never observed concurrent fund"); | ||
|
|
||
| // ---- Assertions on the drained history. ---- | ||
| let history = s.ctx.bank().funding_mutex_history(); | ||
|
|
||
| tracing::info!( | ||
| target: "platform_wallet::e2e::cases::pa_008c", | ||
| entries = ?history, | ||
| "FUNDING_MUTEX observed history" | ||
| ); | ||
|
|
||
| // (1) Cardinality: one entry per spawned future. If the harness | ||
| // has bled in extra entries from a sibling test (it shouldn't, | ||
| // because we drained after the markers), this fires deterministically. | ||
| assert_eq!( | ||
| history.len(), | ||
| 3, | ||
| "PA-008c: expected exactly 3 FUNDING_MUTEX entries from the \ | ||
| concurrent fan-in, observed {}: {history:?}", | ||
| history.len() | ||
| ); |
There was a problem hiding this comment.
🟡 Suggestion: PA-008c's history.len() == 3 assertion races a process-global buffer
FUNDING_MUTEX_HISTORY is a process-global SyncMutex<VecDeque<FundingMutexHistoryEntry>> (bank.rs:59) shared by every test that calls bank.fund_address. PA-008c drains it before its tokio::join! (line 126) and again after balances are observed (line 155) and asserts exactly three entries. The pre-drain only removes past entries; nothing prevents a sibling test running on another worker thread from appending entries between line 126 and line 155, which makes assert_eq!(history.len(), 3) nondeterministic under default cargo test parallelism. The PR specifies --test-threads=1, so the case is sound under the documented runner, but the implicit single-thread requirement is invisible to a future maintainer running with default test-threads — they will see flaky failures unrelated to the mutex contract. Either acquire a per-test serialization handle in setup() and drop it in teardown(), or tag history entries with a per-test id and filter on read so the case is correct under any runner config. At minimum, add a compile_error! or runtime assertion that detects parallel test threads and gates the assertion accordingly.
source: ['claude-general', 'codex-general', 'codex-rust-quality']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/tests/e2e/cases/pa_008c_funding_mutex_observable.rs`:
- [SUGGESTION] lines 124-172: PA-008c's `history.len() == 3` assertion races a process-global buffer
`FUNDING_MUTEX_HISTORY` is a process-global `SyncMutex<VecDeque<FundingMutexHistoryEntry>>` (bank.rs:59) shared by every test that calls `bank.fund_address`. PA-008c drains it before its `tokio::join!` (line 126) and again after balances are observed (line 155) and asserts exactly three entries. The pre-drain only removes past entries; nothing prevents a sibling test running on another worker thread from appending entries between line 126 and line 155, which makes `assert_eq!(history.len(), 3)` nondeterministic under default `cargo test` parallelism. The PR specifies `--test-threads=1`, so the case is sound under the documented runner, but the implicit single-thread requirement is invisible to a future maintainer running with default test-threads — they will see flaky failures unrelated to the mutex contract. Either acquire a per-test serialization handle in `setup()` and drop it in `teardown()`, or tag history entries with a per-test id and filter on read so the case is correct under any runner config. At minimum, add a `compile_error!` or runtime assertion that detects parallel test threads and gates the assertion accordingly.
|
merged into feature branch 'test/platform-wallet-e2e' |
3 participants
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