feat: add property-based testing and fuzzing for escrow & governance …#134
Merged
anonfedora merged 8 commits intoanonfedora:masterfrom Apr 2, 2026
Merged
Conversation
…contracts - Integrate cargo-fuzz with libfuzzer targets for EscrowManager and Governance - Add proptest suites for unit-level property checks on both contracts - Define EscrowManager invariant: total assets == sum of active escrows - Define Governance invariants: vote tallies, double-vote rejection, quorum/majority - Add cargo alias: 'cargo test-fuzz' runs all property tests - Add dedicated CI workflow running 1M+ fuzzing iterations per target - Update existing CI to include proptest in contract checks
|
@GazzyLee Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Owner
|
Fix failing checks, please @GazzyLee |
- Collapse short imports to single-line in proptest_escrow.rs and proptest_governance.rs (rustfmt)
- Collapse short function args to single-line in proptest_escrow.rs (rustfmt)
- Fix HostError(Auth, ExistingValue) in fuzz_governance_voting.rs by:
- Writing config/total_power directly to storage instead of calling
update_config/set_total_voting_power (which trigger admin.require_auth()
conflicting with mock_all_auths + as_contract)
- Splitting single large as_contract block into separate frames for
init, proposal creation, voting, and invariant checks
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds property-based tests and libfuzzer targets for escrow and governance, CI workflows to run proptest and cargo-fuzz, a fuzz crate and cargo alias, workspace/config tweaks, expanded test-only visibility via Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Workflow
participant Runner as Proptest / Fuzzer Runner
participant Env as Soroban Env
participant Contracts as Contracts (mocks & targets)
participant Storage as Contract Storage
CI->>Runner: trigger proptest / cargo-fuzz job
Runner->>Env: initialize test environment
Env->>Contracts: deploy mocks and target contracts
loop per operation / test case
Runner->>Contracts: invoke operation (create/refund/vote/advance)
Contracts->>Storage: read/update persistent state
Contracts-->>Runner: return result
Runner->>Runner: compute expected invariants
Runner->>Storage: read stored values to verify
alt invariant violated or crash
Runner->>CI: report failure / upload artifacts
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
contracts/.cargo/config.toml (1)
1-2: Consider adding--test-threads=1for consistency with CI.The CI workflows (
.github/workflows/ci.ymland.github/workflows/fuzz.yml) run these tests with--test-threads=1. Without this flag, the local alias might exhibit different behavior if tests have shared state or resource contention.♻️ Suggested change
[alias] -test-fuzz = "test --features testutils -- --include-ignored fuzz" +test-fuzz = "test --features testutils -- --include-ignored fuzz --test-threads=1"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/.cargo/config.toml` around lines 1 - 2, The test-fuzz cargo alias currently runs "test --features testutils -- --include-ignored fuzz" without forcing single-threaded execution; update the alias (test-fuzz) to append the test runner flag --test-threads=1 so local runs match CI behavior and avoid differing behavior from parallel test threads..github/workflows/fuzz.yml (1)
33-36: Thewasm32-unknown-unknowntarget may be unnecessary for proptest.The proptest job runs native Rust tests, not WASM compilation. Including this target adds installation overhead without benefit. Consider removing it from this job.
♻️ Remove unnecessary target
- name: Install Rust uses: dtolnay/rust-toolchain@stable - with: - targets: wasm32-unknown-unknown🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/fuzz.yml around lines 33 - 36, The workflow currently adds the unnecessary wasm target "wasm32-unknown-unknown" when using the dtolnay/rust-toolchain action in the proptest job; remove the targets: wasm32-unknown-unknown entry (or the entire with: block if no targets are needed) from the "Install Rust" step that uses dtolnay/rust-toolchain@stable so the job installs only the default native toolchain and avoids extra installation overhead..github/workflows/ci.yml (1)
110-111: Consider settingPROPTEST_CASESenvironment variable for consistency.The dedicated fuzz workflow (
.github/workflows/fuzz.yml) setsPROPTEST_CASES: 10000for the proptest runs. Without this variable here, the default proptest case count (typically 256) will be used, which provides less coverage than the dedicated fuzzing workflow.If this is intentional to keep CI faster while the dedicated workflow provides thorough testing, consider adding a comment to clarify.
♻️ Optional: Add PROPTEST_CASES for more thorough CI coverage
- name: Run Property-Based Tests (proptest) - run: cargo test --features testutils -- --include-ignored fuzz --test-threads=1 + run: cargo test --features testutils -- --include-ignored fuzz --test-threads=1 + env: + PROPTEST_CASES: 1000 # Lighter than fuzz.yml's 10000 for faster CI🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 110 - 111, The proptest step "Run Property-Based Tests (proptest)" currently invokes `cargo test --features testutils -- --include-ignored fuzz --test-threads=1` without setting PROPTEST_CASES, so it uses the default (lower) case count; set the environment variable PROPTEST_CASES to the same value used in the fuzz workflow (e.g., `PROPTEST_CASES: 10000`) for consistency, or if the lower count is intentional to speed CI, add a short comment next to the "Run Property-Based Tests (proptest)" step explaining that the reduced case count is deliberate and that the dedicated `fuzz.yml` workflow runs the full 10000 cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/fuzz.yml:
- Around line 78-82: Generate and commit the missing lockfile for the fuzz
workspace: run cargo generate-lockfile (or cargo build) inside the
contracts/fuzz workspace to produce contracts/fuzz/Cargo.lock, add and commit
that file so the GitHub Actions cache key referencing contracts/fuzz/Cargo.lock
becomes stable and cache invalidation works when fuzzer dependencies change;
ensure the committed file is included in the repo before relying on the cache
key in .github/workflows/fuzz.yml.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 110-111: The proptest step "Run Property-Based Tests (proptest)"
currently invokes `cargo test --features testutils -- --include-ignored fuzz
--test-threads=1` without setting PROPTEST_CASES, so it uses the default (lower)
case count; set the environment variable PROPTEST_CASES to the same value used
in the fuzz workflow (e.g., `PROPTEST_CASES: 10000`) for consistency, or if the
lower count is intentional to speed CI, add a short comment next to the "Run
Property-Based Tests (proptest)" step explaining that the reduced case count is
deliberate and that the dedicated `fuzz.yml` workflow runs the full 10000 cases.
In @.github/workflows/fuzz.yml:
- Around line 33-36: The workflow currently adds the unnecessary wasm target
"wasm32-unknown-unknown" when using the dtolnay/rust-toolchain action in the
proptest job; remove the targets: wasm32-unknown-unknown entry (or the entire
with: block if no targets are needed) from the "Install Rust" step that uses
dtolnay/rust-toolchain@stable so the job installs only the default native
toolchain and avoids extra installation overhead.
In `@contracts/.cargo/config.toml`:
- Around line 1-2: The test-fuzz cargo alias currently runs "test --features
testutils -- --include-ignored fuzz" without forcing single-threaded execution;
update the alias (test-fuzz) to append the test runner flag --test-threads=1 so
local runs match CI behavior and avoid differing behavior from parallel test
threads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e6370835-366b-4efe-b767-f3cb82634abe
📒 Files selected for processing (12)
.github/workflows/ci.yml.github/workflows/fuzz.ymlcontracts/.cargo/config.tomlcontracts/Cargo.tomlcontracts/escrow-manager/Cargo.tomlcontracts/escrow-manager/tests/proptest_escrow.rscontracts/fuzz/Cargo.tomlcontracts/fuzz/fuzz_targets/fuzz_escrow_invariants.rscontracts/fuzz/fuzz_targets/fuzz_governance_voting.rscontracts/governance/Cargo.tomlcontracts/governance/src/lib.rscontracts/governance/tests/proptest_governance.rs
Owner
|
Fix failing CI before EOD @Gazzy-Lee |
The #[cfg(any(test, feature = testutils))] attribute on set_voting_power inside the main #[contractimpl] block caused the Soroban proc macro to generate dispatch code referencing __set_voting_power even when the method was cfg-gated out, resulting in: - E0433 (unresolved module) in non-test builds - E0061/E0063 in fuzz and proptest builds Moving it to its own #[cfg]-gated #[contractimpl] block ensures the entire block (including macro-generated dispatch) is conditionally compiled.
…based-testing-fuzzing # Conflicts: # contracts/governance/src/lib.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/governance/src/lib.rs`:
- Around line 917-924: The set_voting_power method in impl Governance currently
allows unauthenticated callers to mutate storage (key built with
symbol_short!("vp")), which is dangerous when feature flags change; add an
explicit authorization check at the top of pub fn set_voting_power(env: Env,
voter: Address, power: i128) so only an authorized principal can call it (e.g.,
require the contract owner/admin or call the existing auth helper such as
env.require_auth(owner_address) or Governance::require_admin(&env) if present),
then proceed to set the storage; ensure the check uses the same types
(Address/Env) as in the function and returns/aborts on unauthorized access.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b20c6549-0578-4392-9fa6-3bee4a9af0e1
📒 Files selected for processing (1)
contracts/governance/src/lib.rs
Comment on lines
+917
to
+924
| #[cfg(any(test, feature = "testutils"))] | ||
| #[contractimpl] | ||
| impl Governance { | ||
| /// Set voting power for testing | ||
| pub fn set_voting_power(env: Env, voter: Address, power: i128) { | ||
| let key = (symbol_short!("vp"), voter); | ||
| env.storage().persistent().set(&key, &power); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that `testutils` is not enabled in deploy/release paths and is limited to test/fuzz usage.
set -euo pipefail
echo "== Feature definitions and references =="
rg -n --iglob '*.toml' '\btestutils\b|^\[features\]|^default\s*='
echo
echo "== CI/workflow references to feature flags =="
rg -n --iglob '*.yml' --iglob '*.yaml' -- '--features\s+testutils|--all-features|cargo\s+\+?nightly|soroban\s+contract\s+(build|deploy)|cargo\s+(build|run|install)'
echo
echo "== Direct calls to set_voting_power =="
rg -n '\bset_voting_power\s*\(' contracts/governance/src/lib.rsRepository: anonfedora/stellovault
Length of output: 2516
🏁 Script executed:
cat -n contracts/governance/Cargo.toml | head -30Repository: anonfedora/stellovault
Length of output: 783
🏁 Script executed:
rg -n '\bset_voting_power\b' --type rustRepository: anonfedora/stellovault
Length of output: 7048
🏁 Script executed:
fd -name 'Makefile' -o -name '.github' -type dRepository: anonfedora/stellovault
Length of output: 296
🏁 Script executed:
find . -type f \( -name '*.yml' -o -name '*.yaml' -o -name 'Makefile' \) | head -20Repository: anonfedora/stellovault
Length of output: 204
🏁 Script executed:
cat -n .github/workflows/ci.yml | head -80Repository: anonfedora/stellovault
Length of output: 2451
🏁 Script executed:
cat -n .github/workflows/fuzz.yml | head -50Repository: anonfedora/stellovault
Length of output: 1757
🏁 Script executed:
sed -n '915,930p' contracts/governance/src/lib.rsRepository: anonfedora/stellovault
Length of output: 613
🏁 Script executed:
sed -n '917,930p' contracts/governance/src/lib.rsRepository: anonfedora/stellovault
Length of output: 532
🏁 Script executed:
sed -n '80,150p' .github/workflows/ci.ymlRepository: anonfedora/stellovault
Length of output: 1028
🏁 Script executed:
rg -n 'deploy|release|wasm|build' .github/workflows/ci.yml --context 2Repository: anonfedora/stellovault
Length of output: 782
Add authentication to set_voting_power to harden against feature flag misconfiguration.
The function at lines 921–923 allows unauthenticated state mutation. While testutils is correctly absent from the release build configuration, expanding from #[cfg(test)] to any(test, feature = "testutils") creates a feature-flag drift risk—if a developer accidentally builds with --all-features or if feature flags are misconfigured in a deploy step, voting power can be rewritten by any caller.
🔒 Suggested hardening
#[cfg(any(test, feature = "testutils"))]
#[contractimpl]
impl Governance {
/// Set voting power for testing
- pub fn set_voting_power(env: Env, voter: Address, power: i128) {
+ pub fn set_voting_power(env: Env, voter: Address, power: i128) -> Result<(), ContractError> {
+ let admin: Address = env
+ .storage()
+ .instance()
+ .get(&symbol_short!("admin"))
+ .ok_or(ContractError::Unauthorized)?;
+ admin.require_auth();
+ if power < 0 {
+ return Err(ContractError::InvalidValue);
+ }
let key = (symbol_short!("vp"), voter);
env.storage().persistent().set(&key, &power);
+ Ok(())
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[cfg(any(test, feature = "testutils"))] | |
| #[contractimpl] | |
| impl Governance { | |
| /// Set voting power for testing | |
| pub fn set_voting_power(env: Env, voter: Address, power: i128) { | |
| let key = (symbol_short!("vp"), voter); | |
| env.storage().persistent().set(&key, &power); | |
| } | |
| #[cfg(any(test, feature = "testutils"))] | |
| #[contractimpl] | |
| impl Governance { | |
| /// Set voting power for testing | |
| pub fn set_voting_power(env: Env, voter: Address, power: i128) -> Result<(), ContractError> { | |
| let admin: Address = env | |
| .storage() | |
| .instance() | |
| .get(&symbol_short!("admin")) | |
| .ok_or(ContractError::Unauthorized)?; | |
| admin.require_auth(); | |
| if power < 0 { | |
| return Err(ContractError::InvalidValue); | |
| } | |
| let key = (symbol_short!("vp"), voter); | |
| env.storage().persistent().set(&key, &power); | |
| Ok(()) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/governance/src/lib.rs` around lines 917 - 924, The set_voting_power
method in impl Governance currently allows unauthenticated callers to mutate
storage (key built with symbol_short!("vp")), which is dangerous when feature
flags change; add an explicit authorization check at the top of pub fn
set_voting_power(env: Env, voter: Address, power: i128) so only an authorized
principal can call it (e.g., require the contract owner/admin or call the
existing auth helper such as env.require_auth(owner_address) or
Governance::require_admin(&env) if present), then proceed to set the storage;
ensure the check uses the same types (Address/Env) as in the function and
returns/aborts on unauthorized access.
- Gate proptest_governance.rs with #![cfg(feature = testutils)] so it only compiles when testutils is enabled (fixes cargo test without --features testutils) - Add missing 5th argument (num_votes) to all Governance::cast_vote calls in proptest and fuzz targets to match updated API signature - Add missing tally_period field to GovernanceConfig initializations - Set voter balances to power*power to account for quadratic voting sqrt in get_voting_power - Remove unused imports (Symbol, ProposalStatus)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
contracts/governance/tests/proptest_governance.rs (3)
221-222: Add a comment explaining thepower * powervoting power pattern.The pattern of setting
voting_power = power * powerwhile casting a vote withpowerappears throughout the tests but isn't documented. This seems intentional to satisfy some contract invariant (perhapsvoting_power >= vote_amount²), but readers will find it confusing without explanation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/governance/tests/proptest_governance.rs` around lines 221 - 222, Add a short inline comment next to the Governance::set_voting_power(...) call explaining why we set voting_power = power * power before calling Governance::cast_vote(...): state that tests intentionally grant squared voting power to satisfy the contract invariant (e.g., voting_power must be >= vote_amount * vote_amount or similar square-based check) so casting a vote with amount `power` succeeds; mention the invariant name if known and reference the two functions (Governance::set_voting_power and Governance::cast_vote) so future readers understand the relationship.
160-164: Inconsistent coverage: grace period test doesn't include negative values.The
liq_thresholdandliq_penaltytests usei128::MIN..ranges to include negative values, butgrace_periodonly tests0i128..300i128. For consistency and completeness, consider including negative values.♻️ Suggested fix
fn prop_grace_period_validation(value in prop_oneof![ (300i128..=86400i128), - (0i128..300i128), + (i128::MIN..300i128), (86401i128..1_000_000i128), ]) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/governance/tests/proptest_governance.rs` around lines 160 - 164, The property test function prop_grace_period_validation currently omits negative numbers in its prop_oneof set; update its prop_oneof to include a negative range (e.g., i128::MIN..0i128 or a bounded negative range similar to liq_threshold/liq_penalty tests) so the grace period validation covers negative values as well; modify the prop_oneof inside prop_grace_period_validation to add that negative branch while keeping the existing valid and too-large ranges.
196-197: Consider generating equal-length vectors directly.The
powersandsupportsvectors are generated independently with potentially different lengths, requiring themin()at line 214. Using a single length generator would be cleaner.♻️ Suggested fix
fn prop_vote_tallies_consistent( - powers in proptest::collection::vec(1i128..100_000i128, 1..8), - supports in proptest::collection::vec(proptest::bool::ANY, 1..8), + num_voters in 1usize..8, + powers in proptest::collection::vec(1i128..100_000i128, 8), + supports in proptest::collection::vec(proptest::bool::ANY, 8), ) {Then use
num_votersas the loop bound instead ofcount.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/governance/tests/proptest_governance.rs` around lines 196 - 197, The two vectors powers and supports are generated independently which can yield different lengths and forces using min(); instead generate a single size variable (e.g., num_voters) with proptest::num::usize::ANY or a range (1..8) and use that size for both proptest::collection::vec calls so powers and supports share the same length, then iterate up to num_voters (replace uses of count/min with num_voters) when building votes; update references to functions/variables powers, supports, num_voters, and the proptest::collection::vec calls accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/governance/tests/proptest_governance.rs`:
- Around line 383-391: The test currently only asserts that execute_proposal
fails when quorum/majority aren't met but never asserts success when they are
met; update the test after the existing if-block to assert exec_result.is_ok()
(and optionally inspect the returned result) when total_votes >= quorum_needed
&& for_power >= majority_needed so the test fails if execute_proposal always
returns an error; reference the local variables total_votes, quorum_needed,
for_power, majority_needed and the exec_result/execute_proposal call when adding
the positive assertion and any additional checks (e.g., proposal state or
returned value) to verify successful execution.
---
Nitpick comments:
In `@contracts/governance/tests/proptest_governance.rs`:
- Around line 221-222: Add a short inline comment next to the
Governance::set_voting_power(...) call explaining why we set voting_power =
power * power before calling Governance::cast_vote(...): state that tests
intentionally grant squared voting power to satisfy the contract invariant
(e.g., voting_power must be >= vote_amount * vote_amount or similar square-based
check) so casting a vote with amount `power` succeeds; mention the invariant
name if known and reference the two functions (Governance::set_voting_power and
Governance::cast_vote) so future readers understand the relationship.
- Around line 160-164: The property test function prop_grace_period_validation
currently omits negative numbers in its prop_oneof set; update its prop_oneof to
include a negative range (e.g., i128::MIN..0i128 or a bounded negative range
similar to liq_threshold/liq_penalty tests) so the grace period validation
covers negative values as well; modify the prop_oneof inside
prop_grace_period_validation to add that negative branch while keeping the
existing valid and too-large ranges.
- Around line 196-197: The two vectors powers and supports are generated
independently which can yield different lengths and forces using min(); instead
generate a single size variable (e.g., num_voters) with
proptest::num::usize::ANY or a range (1..8) and use that size for both
proptest::collection::vec calls so powers and supports share the same length,
then iterate up to num_voters (replace uses of count/min with num_voters) when
building votes; update references to functions/variables powers, supports,
num_voters, and the proptest::collection::vec calls accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 38e45443-eddb-4530-99b7-40225e0b59eb
📒 Files selected for processing (2)
contracts/fuzz/fuzz_targets/fuzz_governance_voting.rscontracts/governance/tests/proptest_governance.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/fuzz/fuzz_targets/fuzz_governance_voting.rs
Comment on lines
+383
to
+391
| if total_votes < quorum_needed || for_power < majority_needed { | ||
| prop_assert!( | ||
| exec_result.is_err(), | ||
| "Should fail: votes={}, quorum_needed={}, for={}, majority_needed={}", | ||
| total_votes, quorum_needed, for_power, majority_needed | ||
| ); | ||
| } | ||
| // If quorum and majority are met, success is valid | ||
| Ok(()) |
There was a problem hiding this comment.
Incomplete test coverage: missing assertion for successful execution.
The test verifies that execution fails when quorum/majority aren't met, but doesn't verify that execution succeeds when they ARE met. The comment on line 390 acknowledges this gap without addressing it. This means the test would pass even if execute_proposal always returned an error.
🐛 Suggested fix to verify both cases
if total_votes < quorum_needed || for_power < majority_needed {
prop_assert!(
exec_result.is_err(),
"Should fail: votes={}, quorum_needed={}, for={}, majority_needed={}",
total_votes, quorum_needed, for_power, majority_needed
);
+ } else {
+ prop_assert!(
+ exec_result.is_ok(),
+ "Should succeed: votes={}, quorum_needed={}, for={}, majority_needed={}",
+ total_votes, quorum_needed, for_power, majority_needed
+ );
}
- // If quorum and majority are met, success is valid
Ok(())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if total_votes < quorum_needed || for_power < majority_needed { | |
| prop_assert!( | |
| exec_result.is_err(), | |
| "Should fail: votes={}, quorum_needed={}, for={}, majority_needed={}", | |
| total_votes, quorum_needed, for_power, majority_needed | |
| ); | |
| } | |
| // If quorum and majority are met, success is valid | |
| Ok(()) | |
| if total_votes < quorum_needed || for_power < majority_needed { | |
| prop_assert!( | |
| exec_result.is_err(), | |
| "Should fail: votes={}, quorum_needed={}, for={}, majority_needed={}", | |
| total_votes, quorum_needed, for_power, majority_needed | |
| ); | |
| } else { | |
| prop_assert!( | |
| exec_result.is_ok(), | |
| "Should succeed: votes={}, quorum_needed={}, for={}, majority_needed={}", | |
| total_votes, quorum_needed, for_power, majority_needed | |
| ); | |
| } | |
| Ok(()) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/governance/tests/proptest_governance.rs` around lines 383 - 391,
The test currently only asserts that execute_proposal fails when quorum/majority
aren't met but never asserts success when they are met; update the test after
the existing if-block to assert exec_result.is_ok() (and optionally inspect the
returned result) when total_votes >= quorum_needed && for_power >=
majority_needed so the test fails if execute_proposal always returns an error;
reference the local variables total_votes, quorum_needed, for_power,
majority_needed and the exec_result/execute_proposal call when adding the
positive assertion and any additional checks (e.g., proposal state or returned
value) to verify successful execution.
Owner
|
Fix buzzing CI, please |
…Error(Auth, ExistingValue)
…governance_voting
anonfedora
approved these changes
Apr 2, 2026
4 tasks
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
3 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.
Summary
Introduces property-based testing (proptest) and fuzzing (cargo-fuzz/libfuzzer) for the
escrow-managerandgovernancesmart contracts to systematically discover edge cases and verify critical invariants.Problem
The existing test suite uses hand-written example-based tests, which only cover anticipated scenarios. Edge cases in escrow balance accounting, governance voting arithmetic, and parameter validation may go undetected.
Solution
Cargo-fuzz targets (
contracts/fuzz/) — libfuzzer-powered harnesses that generate random sequences of operations and verify invariants after every step:fuzz_escrow_invariants— Generates random Create/Refund/AdvanceTime operation sequences and asserts: "Total assets in contract must always equal the sum of active escrow amounts."fuzz_governance_voting— Randomizes voter count, voting power, quorum/majority thresholds, and asserts: vote tallies match individual powers, double-voting is always rejected, and proposals cannot execute without quorum + majority.Proptest suites (
contracts/*/tests/) — unit-level property checks with thousands of generated inputs:Changes
contracts/.cargo/config.tomlcargo test-fuzzaliascontracts/Cargo.tomlexclude = ["fuzz"]to isolate fuzz cratecontracts/escrow-manager/Cargo.tomlrlibcrate type +proptestdev-depcontracts/governance/Cargo.tomlrlibcrate type +proptestdev-depcontracts/governance/src/lib.rsset_voting_powergated ontestutilsfeature (was#[cfg(test)]only)contracts/fuzz/Cargo.tomlcontracts/fuzz/fuzz_targets/fuzz_escrow_invariants.rscontracts/fuzz/fuzz_targets/fuzz_governance_voting.rscontracts/escrow-manager/tests/proptest_escrow.rscontracts/governance/tests/proptest_governance.rs.github/workflows/fuzz.yml.github/workflows/ci.ymlHow to Run
Acceptance Criteria
cargo test-fuzzLabels
infrastructure,testing,priority: highCloses #129
Summary by CodeRabbit