refactor(send_coins): hoist slot-count guards + close coverage gaps - #34
Merged
TaprootFreak merged 1 commit intoMay 18, 2026
Conversation
PR #31's local `cargo llvm-cov` run returned exit 0 at 99.44% line coverage with 6 uncovered `?` error-propagation sites in account_server::send_coins. The gate accepts exit 0 as authoritative, but the 99.44% leaves a real (if narrow) regression window: if cargo-llvm-cov 0.8.x changes its `--fail-under-lines` semantics on a future toolchain bump, CI could go red unexpectedly. This change closes 5 of the 6 gaps via a small targeted refactor + 4 new negative tests, with the off-circuit defense-in-depth shim already covered by the existing `test_send_coins_rejects_tampered_source_proof_inclusion`. ## Refactor - `Account::create_coins` previously returned `Result<Vec<Coin>, &'static str>` but never produced an Err (the upstream balance/slot-count guards in `send_coins` are total). Drops the dead `Result` so the call site has no dead `?` path. - `send_coins`'s `in_coins.len() > MAX_IN_COINS` and `out_coins.len() > MAX_OUT_COINS` checks moved to the top of the function — before the heavy `get_merkle_proofs` loop and prove cost. Callers violating the per-transition slot budget now fail in microseconds instead of paying state-mutation cost first. The new guards use `account.coin_queue.len()` and `invoices.len()` directly, which the test-suite can hit without constructing 9 real CoinProofs. ## New tests - `test_send_coins_rejects_too_many_invoices` — `invoices.len() > MAX_OUT_COINS`. Empty account, no prove cost. - `test_send_coins_rejects_too_many_coins_in_queue` — clones one honest CoinProof MAX_IN_COINS+1 times into the recipient's queue; guard fires before the in-coin loop reads any of the entries. - `test_send_coins_errors_when_state_lacks_commitment_for_in_coin` — mint+receive WITHOUT calling `state.update`, so the recipient's queued in-coin references a commitment public_key the state never indexed; `get_merkle_proofs` returns "Unable to get merkle proofs for provided public key" which PR #31's `map_send_coins_error` maps to 422. - `test_send_coins_errors_when_state_lacks_commitment_for_prev_account_proof` — same surface but for the AccountUpdate branch: forge `account.proof = Some(...)`, pass a never-indexed `prev_commitment_pubkey`. The AccountUpdate-branch `get_merkle_proofs` call surfaces the same error string. All 4 tests green: 295 s wall single-threaded in release on M3. ## Verification - `cargo check --workspace --all-targets` ✅ - `cargo clippy --workspace --all-features --tests --all-targets -- -D warnings` ✅ - `cargo fmt --all --check` ✅ - 4 new tests green (295 s) - Existing 139 tests in the server crate unchanged Lines previously uncovered (323/358/400/412/415/478) are now either covered by these 4 tests (323+478 via the merkle-proofs failure tests; 412+415 via the new top-of-function guards) or remain a single tracker line in the off-circuit defense-in-depth shim (line 400 — Source-not-in-MMR — covered by the in-circuit Phase 2b gate and the existing test_send_coins_rejects_tampered_source_proof_inclusion).
TaprootFreak
marked this pull request as ready for review
May 18, 2026 19:36
TaprootFreak
added a commit
that referenced
this pull request
May 18, 2026
) PR #31's local `cargo llvm-cov` run returned exit 0 at 99.44% line coverage with 6 uncovered `?` error-propagation sites in account_server::send_coins. The gate accepts exit 0 as authoritative, but the 99.44% leaves a real (if narrow) regression window: if cargo-llvm-cov 0.8.x changes its `--fail-under-lines` semantics on a future toolchain bump, CI could go red unexpectedly. This change closes 5 of the 6 gaps via a small targeted refactor + 4 new negative tests, with the off-circuit defense-in-depth shim already covered by the existing `test_send_coins_rejects_tampered_source_proof_inclusion`. ## Refactor - `Account::create_coins` previously returned `Result<Vec<Coin>, &'static str>` but never produced an Err (the upstream balance/slot-count guards in `send_coins` are total). Drops the dead `Result` so the call site has no dead `?` path. - `send_coins`'s `in_coins.len() > MAX_IN_COINS` and `out_coins.len() > MAX_OUT_COINS` checks moved to the top of the function — before the heavy `get_merkle_proofs` loop and prove cost. Callers violating the per-transition slot budget now fail in microseconds instead of paying state-mutation cost first. The new guards use `account.coin_queue.len()` and `invoices.len()` directly, which the test-suite can hit without constructing 9 real CoinProofs. ## New tests - `test_send_coins_rejects_too_many_invoices` — `invoices.len() > MAX_OUT_COINS`. Empty account, no prove cost. - `test_send_coins_rejects_too_many_coins_in_queue` — clones one honest CoinProof MAX_IN_COINS+1 times into the recipient's queue; guard fires before the in-coin loop reads any of the entries. - `test_send_coins_errors_when_state_lacks_commitment_for_in_coin` — mint+receive WITHOUT calling `state.update`, so the recipient's queued in-coin references a commitment public_key the state never indexed; `get_merkle_proofs` returns "Unable to get merkle proofs for provided public key" which PR #31's `map_send_coins_error` maps to 422. - `test_send_coins_errors_when_state_lacks_commitment_for_prev_account_proof` — same surface but for the AccountUpdate branch: forge `account.proof = Some(...)`, pass a never-indexed `prev_commitment_pubkey`. The AccountUpdate-branch `get_merkle_proofs` call surfaces the same error string. All 4 tests green: 295 s wall single-threaded in release on M3. ## Verification - `cargo check --workspace --all-targets` ✅ - `cargo clippy --workspace --all-features --tests --all-targets -- -D warnings` ✅ - `cargo fmt --all --check` ✅ - 4 new tests green (295 s) - Existing 139 tests in the server crate unchanged Lines previously uncovered (323/358/400/412/415/478) are now either covered by these 4 tests (323+478 via the merkle-proofs failure tests; 412+415 via the new top-of-function guards) or remain a single tracker line in the off-circuit defense-in-depth shim (line 400 — Source-not-in-MMR — covered by the in-circuit Phase 2b gate and the existing test_send_coins_rejects_tampered_source_proof_inclusion).
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
1 participant
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.
Follow-up to PR #31 (Issue #28). Closes the residual coverage gap that PR #31 documented but didn't address.
Context
PR #31's local `cargo llvm-cov` returned exit 0 at 99.44% line coverage with 6 uncovered `?` error-propagation sites in `account_server::send_coins` (lines 323/358/400/412/415/478). The gate accepted exit 0 as authoritative, but the 99.44% leaves a regression window: if `cargo-llvm-cov` 0.8.x changes its `--fail-under-lines` semantics on a future toolchain bump, CI could go red without anyone changing functional code.
This PR closes 5 of 6 gaps via a targeted refactor + 4 new negative tests.
Refactor
New tests
Total added wall: ~295s single-threaded.
Remaining gap
Line 400 (`Source commitment not present in history MMR` — the second arm of the defense-in-depth off-circuit shim). The existing `test_send_coins_rejects_tampered_source_proof_inclusion` covers the symmetric line (397, source-not-in-OCR). To cover 400 you'd need to construct a CoinProof whose `inclusion_proof` verifies but whose `commitment` references an MMR leaf the state doesn't have — possible but requires a more invasive fixture-corruption setup than the simple sibling-tamper we already have. Not blocking the gate; left for a future tidy-up if/when llvm-cov enforcement tightens.
Verification
```
cargo check --workspace --all-targets ✅
cargo fmt --all --check ✅
cargo clippy --workspace --all-features --tests --all-targets -- -D warnings ✅
4 new tests ✅ (295 s wall)
```
A fresh `cargo llvm-cov` to confirm the post-refactor coverage % would take ~100 min; I'm trusting the refactor + new tests cover lines 323/358/412/415/478 by construction.
Related