Skip to content

fix(#461): add regression tests for verify_with_votes overflow handling - #729

Open
OpensrcLord wants to merge 5 commits into
Iris-IV:mainfrom
OpensrcLord:fix/461-verify-votes-overflow-check
Open

fix(#461): add regression tests for verify_with_votes overflow handling#729
OpensrcLord wants to merge 5 commits into
Iris-IV:mainfrom
OpensrcLord:fix/461-verify-votes-overflow-check

Conversation

@OpensrcLord

Copy link
Copy Markdown

Summary

Fixes #461

The checked-arithmetic fix for verify_with_votes was already in place — both aggregations use checked_add(...).ok_or(Error::Overflow)?:

  • src/voting.rsapprove_votes + reject_votes (vote-count total)
  • src/voting.rsapprove_weight + reject_weight (token-weighted total)

This PR adds regression tests to lock in that behavior so aggregation returns Error::Overflow instead of silently wrapping on u32/i128 overflow:

  • test_verify_with_votes_count_overflow_returns_overflow — forces approve_votes + reject_votes past u32::MAX
  • test_verify_with_votes_weight_overflow_returns_overflow — forces approve_weight + reject_weight past i128::MAX

Test plan

  • cargo test --features testutils (CI runs this)

… handling

The checked_add guards in verify_with_votes are already in place; these
tests lock in the behavior so vote-count and token-weight aggregation
return Error::Overflow instead of silently wrapping on i128/u32 overflow.
@drips-wave

drips-wave Bot commented Jul 31, 2026

Copy link
Copy Markdown

@OpensrcLord 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! 🚀

Learn more about application limits

PR Iris-IV#618 accidentally added a draft snippet as src/campaigns.rs, which
duplicates the real src/campaigns/mod.rs. Rust (E0761) and rustfmt both
reject resolving `mod campaigns` to two files, breaking every CI run.
The category-cap feature it sketches is implemented in src/admin.rs.
PR Iris-IV#618 left src/admin.rs and src/lib.rs failing `cargo fmt --all -- --check`
(import ordering, missing trailing newlines). Format them so the fmt CI gate
passes; also collapse the storage import in the new test file.
PR Iris-IV#618 added duplicate #[contract] ProofOfHeartContract blocks to
src/lib.rs and src/admin.rs alongside the real ProofOfHeart contractimpl.
They reference symbols that do not exist (campaign.tags, DataKey::Campaigns,
DataKey::CategoryMaxGoalCap, Self::verify_admin) and duplicate the soroban
generated modules, breaking the build (E0252/E0428/E0433/E0609) for every
PR against main. Remove them so the crate compiles again.
Iris-IV#510 changed the campaign_metadata_updated event payload from (new_title,
new_description) to (old_title, old_description, new_title, new_description)
for indexers. Two tests in test_campaign_update still unpacked the old
2-tuple, causing UnexpectedSize host errors. Update them to the 4-tuple.
@davidmaronio

Copy link
Copy Markdown
Contributor

solid regression coverage. both tests directly seed the aggregate counters via env.as_contract to force the exact overflow paths (u32 vote count and i128 weight) and assert Error::Overflow comes back instead of a wrap, which is exactly what #461 asked to lock in. CI is green, and honestly documenting that the checked-add fix already existed is the right call rather than inventing code churn. small notes:

  1. src/tests/test_campaign_update.rs:100 - in the second updated test you assert only payload.2 and payload.3; the first test asserts all four fields. add the payload.0/payload.1 "Title V2"/"Description V2" assertions for consistency, or note why the previous values don't matter there.
  2. this pr also deletes the orphaned ProofOfHeartContract blocks in admin.rs/lib.rs/campaigns.rs, the same cleanup carried by fix: guarantee wasm build reproducibility via dockerized make target #724 and feat: add emergency pause multi-sig endpoint #725. that's what got you green, but it means whichever of these lands first forces rebases on the rest. maintainers should probably land one cleanup pr quickly; nothing for you to change other than expect a rebase.

gate: branch is DIRTY against main, so please resolve conflicts and rebase; after that this is good to merge from my side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] verify_with_votes uses unchecked addition for vote totals — overflow possible with high-balance tokens

3 participants