Restore lost test coverage, clear warning backlog, gate CI on clippy + fmt - #441
Merged
Merged
Conversation
CI was green, but only because it never checked most of this. Coverage gaps ------------- Four public contract functions had zero test call sites on main: get_reputation, cancel_admin_transfer_proposal, set_interest_yield_consent and admin_override_streaming_release. admin_accrue_yield had two. They were not new. Commit 8716d10 (Jul 25) rewrote test.rs and cut it from 143 test functions to 86 -- 3,481 deleted lines -- while its message said it added 29 tests. Most of that was restored by later work; 16 tests never were, and their snapshots are still sitting in test_snapshots/ with no test behind them. Adds 31 tests across five files, reusing the original names so the orphan snapshots reconnect: reputation_tests 4 cancel_admin_transfer_tests 5 admin_accrue_yield_tests 7 interest_yield_consent_tests 7 admin_override_streaming_release_tests 8 These cover the paths that move money or change who controls the escrow: streaming release splitting a disputed milestone pro-rata and refunding the remainder, yield accrual rejecting overflow rather than wrapping the accumulator negative, consent enforcing the 10_000 bps sum and the lock, and cancellation clearing the pending-transfer block. A silently disabled test ------------------------ test_tax_withholding_deductions_zero_balance_fails had lost its #[test] attribute in a merge and had stopped running. Restored; it passes. Warnings -------- 61 compiler warnings and 21 clippy warnings, all accumulated debris from the merge campaign -- unused imports and variables, assert_eq! against literal bools. Now zero of each. One clippy suggestion was NOT taken. clippy::redundant_closure_call wants the reentrancy guards' `let result = (|| { … })();` flattened to a plain block. That is not equivalent: inside a block, `?` and `return` exit the enclosing function and skip the lock release below, leaving the escrow permanently locked. cargo clippy --fix applied it to four guards before I caught it; reverted, with a crate-level allow explaining why. CI -- The contract workflow ran only tests and the wasm build, which is why none of the above failed it. Added cargo fmt --check and cargo clippy -D warnings, plus the toolchain components they need. 708 tests pass, clippy clean under -D warnings, fmt clean, wasm builds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The dual-signature enforcement in set_interest_yield_consent is not directly validated by tests (current coverage uses mock_all_auths()), leaving a key guard susceptible to regression.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR strengthens the milestone-escrow contract’s safety net by restoring previously lost unit-test coverage (including reconnecting orphaned snapshot files), eliminating warning backlogs, and updating CI to gate merges on cargo fmt and cargo clippy (with warnings denied).
Changes:
- Added new unit-test suites for previously untested public contract endpoints (reputation, admin-transfer cancellation, yield accrual, yield-consent, and disputed streaming release) and reconnected snapshot artifacts.
- Cleaned up warnings/clippy findings across existing test suites and added a crate-level
clippy::redundant_closure_callallow with rationale for reentrancy-guard correctness. - Updated GitHub Actions CI to install
clippy/rustfmtand runcargo fmt --checkpluscargo clippy -- -D warningsbefore tests.
File summaries
| File | Description |
|---|---|
| contracts/milestone-escrow/test_snapshots/reputation_tests/test_reputation_not_incremented_on_failed_release.1.json | Adds/restores snapshot for reputation failure-path test. |
| contracts/milestone-escrow/test_snapshots/interest_yield_consent_tests/test_escrow_interest_yield_consent_uninitialized_rejected.1.json | Adds/restores snapshot for uninitialized yield-consent guard. |
| contracts/milestone-escrow/test_snapshots/interest_yield_consent_tests/test_escrow_interest_yield_consent_non_admin_fails.1.json | Adds/restores snapshot for yield-consent unauthorized guard. |
| contracts/milestone-escrow/test_snapshots/cancel_admin_transfer_tests/test_cancel_admin_transfer_uninitialized_rejected.1.json | Adds/restores snapshot for uninitialized admin-transfer cancellation. |
| contracts/milestone-escrow/test_snapshots/cancel_admin_transfer_tests/test_cancel_admin_transfer_no_pending_fails.1.json | Adds/restores snapshot for “no pending transfer” cancellation guard. |
| contracts/milestone-escrow/test_snapshots/admin_override_streaming_release_tests/test_admin_override_streaming_release_requires_disputed_status.1.json | Adds/restores snapshot for disputed-status guard on streaming release override. |
| contracts/milestone-escrow/test_snapshots/admin_accrue_yield_tests/test_admin_accrue_yield_uninitialized_rejected.1.json | Adds/restores snapshot for uninitialized yield-accrual guard. |
| contracts/milestone-escrow/test_snapshots/admin_accrue_yield_tests/test_admin_accrue_yield_non_admin_fails.1.json | Adds/restores snapshot for yield-accrual unauthorized guard. |
| contracts/milestone-escrow/test_snapshots/admin_accrue_yield_tests/test_admin_accrue_yield_invalid_milestone_fails.1.json | Adds/restores snapshot for yield-accrual invalid-milestone guard. |
| contracts/milestone-escrow/src/test.rs | Re-wires/extends the main test harness module list and performs formatting/lint cleanup; restores a missing #[test]. |
| contracts/milestone-escrow/src/test_emergency_pause.rs | Cleans up unused bindings to address warnings in emergency-pause tests. |
| contracts/milestone-escrow/src/reputation_tests.rs | Adds a dedicated suite covering get_reputation behavior and release-path bookkeeping. |
| contracts/milestone-escrow/src/multisig_split_refund_tests.rs | Formatting/lint cleanup (line wrapping and assertions) to satisfy clippy/fmt. |
| contracts/milestone-escrow/src/multisig_admin_override_refund_tests.rs | Lint cleanup for unused bindings in multisig override refund tests. |
| contracts/milestone-escrow/src/milestone_time_extensions_tests.rs | Refactors formatting and loops for clippy/fmt compliance. |
| contracts/milestone-escrow/src/lib.rs | Adds crate-level clippy allow with rationale; applies formatting fixes; registers new test modules under cfg(test). |
| contracts/milestone-escrow/src/interest_yield_consent_tests.rs | Adds a new test suite for set_interest_yield_consent guards and state/event reconciliation. |
| contracts/milestone-escrow/src/execute_admin_transfer_tests.rs | Formatting/lint cleanup and minor refactors in admin-transfer execution tests. |
| contracts/milestone-escrow/src/emergency_pause_test.rs | Removes unused imports and tightens assertions/formatting for clippy/fmt. |
| contracts/milestone-escrow/src/emergency_pause_admin_override_tests.rs | Formatting and assertion cleanup; removes stray leading whitespace line. |
| contracts/milestone-escrow/src/cancel_escrow_test.rs | Removes unused imports and adjusts formatting/assertions for lint cleanliness. |
| contracts/milestone-escrow/src/cancel_admin_transfer_tests.rs | Adds a new test suite for cancel_admin_transfer_proposal including event and state checks. |
| contracts/milestone-escrow/src/admin_tax_withholding_guards_tests.rs | Minor refactors to avoid unused parameters and satisfy formatting/lints. |
| contracts/milestone-escrow/src/admin_set_yield_rate_tests.rs | Removes redundant import, improves formatting, and fixes unused-binding warnings. |
| contracts/milestone-escrow/src/admin_pause_escrow_tests.rs | Formatting cleanup and removal of unused imports to satisfy clippy. |
| contracts/milestone-escrow/src/admin_override_streaming_release_tests.rs | Adds a new suite for disputed streaming-release override behavior (balances/status/event/guards). |
| contracts/milestone-escrow/src/admin_override_refund_tests.rs | Import ordering cleanup for lint compliance. |
| contracts/milestone-escrow/src/admin_override_cancel_tests.rs | Removes unnecessary imports and applies formatting fixes. |
| contracts/milestone-escrow/src/admin_accrue_yield_tests.rs | Adds a new suite for admin_accrue_yield including overflow and guard behavior. |
| .github/workflows/ci.yml | Installs clippy/rustfmt and gates CI on fmt + clippy (warnings denied) before running tests. |
Review details
- Files reviewed: 38/52 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+29
to
+33
| let env = Env::default(); | ||
| env.mock_all_auths(); | ||
|
|
||
| let (client_addr, freelancer_addr, _, admin_addr, _, _, client) = | ||
| setup_funded_escrow(&env, vec![&env, 1_000_i128]); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CI was green, but only because it never checked most of this.
Coverage gaps
Four public contract functions had zero test call sites on
main:get_reputation,cancel_admin_transfer_proposal,set_interest_yield_consent,admin_override_streaming_release.admin_accrue_yieldhad two.They weren't new. Commit
8716d10(Jul 25) rewrotetest.rsand cut it from 143 test functions to 86 — 3,481 deleted lines — while its message said it added 29 tests. Later work restored most of it; 16 tests never came back, and their snapshots are still intest_snapshots/with no test behind them.Adds 31 tests, reusing the original names so the orphan snapshots reconnect:
reputation_testscancel_admin_transfer_testsadmin_accrue_yield_testsinterest_yield_consent_testsadmin_override_streaming_release_testsThese cover the paths that move money or change who controls the escrow: streaming release splitting a disputed milestone pro-rata, yield accrual rejecting overflow rather than wrapping the accumulator negative, consent enforcing the 10,000 bps sum and the lock, cancellation clearing the pending-transfer block.
A silently disabled test
test_tax_withholding_deductions_zero_balance_failshad lost its#[test]attribute in a merge and had stopped running. Restored — it passes.Warnings
61 compiler warnings and 21 clippy warnings, all debris from the merge campaign. Now zero of each.
One clippy suggestion was deliberately not taken.
clippy::redundant_closure_callwants the reentrancy guards'let result = (|| { … })();flattened to a plain block. That is not equivalent: inside a block,?andreturnexit the enclosing function and skip the lock release below, leaving the escrow permanently locked.cargo clippy --fixapplied it to four guards before I caught it; reverted, with a crate-levelallowexplaining why.CI
The workflow ran only tests and the wasm build — which is why none of the above failed it. Added
cargo fmt --checkandcargo clippy -- -D warnings.708 tests pass, clippy clean under
-D warnings, fmt clean, wasm builds.🤖 Generated with Claude Code