test(coverage): add 9 test modules for contract test coverage push#54
Open
Josue19-08 wants to merge 2 commits into
Open
test(coverage): add 9 test modules for contract test coverage push#54Josue19-08 wants to merge 2 commits into
Josue19-08 wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThe PR rewrites and adds test modules for events and profile contract coverage. It adds new event tests for token whitelist, cancel refunds, fee math, grant behavior, and narrows bounty and hackathon coverage. It also adds profile earnings and reputation tests and trims credits coverage. ChangesEvents contract tests
Profile contract tests
Estimated review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 8
🧹 Nitpick comments (1)
contracts/events/src/tests/cancel_refund.rs (1)
207-277: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMisleading name/header and dead scratch comments in this test.
This test actually exercises the ProRataPartners branch, not a boundary case:
- The section header (Lines 207-209) says
boundary: remaining == non_owner_total, but the realized state isremaining = 800 < non_owner_total = 1000, i.e. strict pro-rata.- The name
cancel_at_boundary_pays_partners_full_no_owner_residualis inaccurate — partners receive the pro-rated 400 each (Lines 274-275), not their full 500 contribution.- Lines 213-214 and 240-255 are a large block of contradictory, self-correcting scratch reasoning that doesn't describe the final test and should be removed.
Rename to reflect the ProRata path and replace the scratch block with a single accurate comment of the realized math.
♻️ Suggested cleanup
-// ============================================================ -// ProRataPartners branch (boundary: remaining == non_owner_total) -// ============================================================ - -#[test] -fn cancel_at_boundary_pays_partners_full_no_owner_residual() { - // 60/40 split; pay position 1 (60%); remaining = 40% of escrow. - // With partner pool == remaining, no owner residual. +// ============================================================ +// ProRataPartners branch (remaining < non_owner_total) +// ============================================================ + +#[test] +fn cancel_prorata_splits_remaining_across_partners_no_owner_residual() { + // escrow = 2000 (owner 1000 + p1 500 + p2 500); select_winners pays + // pos1 60% (1200) leaving remaining = 800. non_owner_total = 1000. + // 800 < 1000 -> ProRata: each partner gets 500 * 800 / 1000 = 400; owner = 0.Also drop the stale reasoning block at Lines 240-255.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/events/src/tests/cancel_refund.rs` around lines 207 - 277, This test is mislabeled and contains stale scratch reasoning: the scenario in cancel_at_boundary_pays_partners_full_no_owner_residual actually exercises the ProRataPartners path, not the boundary/full-partner case. Rename the test and header to reflect the realized math in drive_cancel and ctx.events.select_winners, and replace the contradictory comment block with one short accurate explanation of the final state (remaining < non_owner_total, so each partner gets a pro-rated share and owner gets nothing). Remove the dead self-correcting comments so the test only documents the final intended behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/events/src/tests/bounty_pillar.rs`:
- Around line 244-267: The single-recipient payout test in
select_winners_pays_recipient_and_updates_profile only checks the applicant’s
balance change and misses the required fee-account balance delta assertion.
Update this test to also verify the fee_account balance change after
ctx.events.select_winners, using the existing Ctx.fee_account and the token
client, so the payout split path covers both recipient and fee balances as
required.
In `@contracts/events/src/tests/cancel_refund.rs`:
- Around line 132-153: The refund/sweep payout tests currently assert only
recipient and owner balance deltas, but they do not verify that the fee account
remains unchanged, so a fee-skimming regression would be missed. Update the
payout-affecting tests in cancel_refund.rs, especially
full_partner_then_residual_pays_partners_and_owner,
paged_cancel_processes_in_batches, and the cancel_at_boundary_* cases, to
capture the fee account balance from Ctx before and after drive_cancel and
assert its delta is zero alongside the existing balance checks.
In `@contracts/events/src/tests/escrow_fee_math.rs`:
- Around line 199-217: Update the payout-path tests in escrow_fee_math.rs to
assert fee-account balance deltas alongside recipient balances. In
select_winners_single_100_percent_drains_escrow and the other payout split tests
referenced by the review, capture the fee account balance before and after
calling Events::select_winners and verify it does not change for these hackathon
winner-release paths, while still checking the recipient payout and
remaining_escrow/status outcomes. Use the existing setup(), token::Client, and
ctx.events.select_winners flows so the assertions cover both the
single-position, multi-position, and sweep cases consistently.
In `@contracts/events/src/tests/grant_pillar.rs`:
- Around line 154-169: The payout-split tests are missing assertions for the
fee-account balance delta even though they already verify recipient deltas.
Update the grant_pillar tests in the
claim_milestone_pays_fixed_per_milestone_amount,
claim_milestone_last_sweeps_rounding_residue, and
two_winner_grant_each_claims_their_share paths to also read ctx.fee_account via
token::Client and assert the fee balance change for each split case. Keep the
existing recipient and escrow assertions, and make sure every single-position,
multi-position, and sweep payout test covers both recipient and fee-account
balance changes.
In `@contracts/events/src/tests/hackathon_pillar.rs`:
- Around line 199-220: Add fee-account balance assertions to the payout tests
that currently only verify recipient balances. In the test function
select_winners_single_pays_full_budget_and_updates_profile, and in the
multi-recipient test mentioned in the review, check the fee_account balance
delta alongside the recipient delta after select_winners so the deposit fee
collection is covered. Use the existing setup helpers and identifiers like
ctx.events.select_winners, ctx.fee_account, and token::Client::new to locate the
payout assertions, and make the same coverage consistent with the sweep-path
test.
In `@contracts/events/src/tests/token_whitelist.rs`:
- Around line 117-136: The unsupported-token test only checks that create_event
fails, which is too weak. Update the assertions in
create_event_with_unsupported_token_reverts and the other try_create_event test
to match the specific Error::TokenNotSupported result instead of using is_err().
Use the existing try_create_event call and assert against the exact rejection
reason so the token whitelist behavior is enforced precisely.
- Around line 24-27: Add negative auth tests for the admin-gated whitelist
mutation paths in token_whitelist.rs, because setup() uses env.mock_all_auths()
and only verifies auth requests. Update the existing register/deregister test
cases and add explicit unauthorized-call checks around the whitelist mutation
entry points so non-admin callers are asserted to fail, using the relevant test
helpers and mutation methods in this module. Focus on proving rejection behavior
for both registration and deregistration rather than only successful admin
flows.
In `@contracts/profile/src/tests/earnings.rs`:
- Around line 139-152: The test register_earnings_requires_events_contract_auth
currently only verifies the success path under mock_all_auths and does not
assert the unauthorized failure it claims to cover. Update the earnings test in
register_earnings_requires_events_contract_auth to include a
try_register_earnings call that is rejected when the caller is not the
configured events contract, and assert that the auth check fails before the
state update; if you intend to keep only the success case, rename the test to
match its actual behavior.
---
Nitpick comments:
In `@contracts/events/src/tests/cancel_refund.rs`:
- Around line 207-277: This test is mislabeled and contains stale scratch
reasoning: the scenario in
cancel_at_boundary_pays_partners_full_no_owner_residual actually exercises the
ProRataPartners path, not the boundary/full-partner case. Rename the test and
header to reflect the realized math in drive_cancel and
ctx.events.select_winners, and replace the contradictory comment block with one
short accurate explanation of the final state (remaining < non_owner_total, so
each partner gets a pro-rated share and owner gets nothing). Remove the dead
self-correcting comments so the test only documents the final intended behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7e26aeca-6dbd-4fd3-904a-54286b7092f6
📒 Files selected for processing (11)
contracts/events/src/tests/bounty_pillar.rscontracts/events/src/tests/cancel_refund.rscontracts/events/src/tests/escrow_fee_math.rscontracts/events/src/tests/grant_pillar.rscontracts/events/src/tests/hackathon_pillar.rscontracts/events/src/tests/mod.rscontracts/events/src/tests/token_whitelist.rscontracts/profile/src/tests/credits.rscontracts/profile/src/tests/earnings.rscontracts/profile/src/tests/mod.rscontracts/profile/src/tests/reputation.rs
Josue19-08
added a commit
to Josue19-08/boundless-contract
that referenced
this pull request
Jun 24, 2026
- Add fee-account balance delta assertions to all payout split paths (bounty, hackathon, escrow_fee_math, grant single/sweep/multi-winner) - Add fee-account delta=0 assertions to cancel refund payout tests (full_partner_then_residual, paged_cancel, prorata) - Rename cancel_at_boundary test to cancel_prorata_splits_remaining; remove stale scratch comments, fix section header - Change is_err() to assert_eq!(err, Error::TokenNotSupported) in token whitelist create_event rejection tests - Add register_without_admin_auth_reverts and deregister_without_admin_auth_reverts negative auth tests - Rename register_earnings_requires_events_contract_auth and add register_earnings_without_events_contract_auth_reverts with a real impostor-auth rejection check
Author
|
All CodeRabbit findings addressed in the latest commit (eb378fc):
209 tests passing (147 events + 62 profile), zero warnings. |
Collaborator
|
@Josue19-08 please fix conflict |
…coverage push Implements the full test coverage matrix from issue boundlessfi#25: events: - token_whitelist: register/deregister token support + create_event gate - cancel_refund: paged cancel flow (OwnerOnly, FullPartnerThenResidual, ProRata, pagination, all error variants) - escrow_fee_math: fee override, waiver, add_funds rate snapshot, MAX_FEE_BPS guard, single/split payout math - grant_pillar: Multi release required, fixed-split milestone math, last-milestone dust sweep, credit/rep side-effects, all errors - bounty_pillar: Single release required, apply/withdraw/submit gate, credit charge and 50% refund, select_winners pay+profile, all errors - hackathon_pillar: Single+deadline required, open submit model, multi-recipient distribution, cancel refund, all errors profile: - credits: bootstrap, spend/earn/refund/admin_grant, all error variants - reputation: bump/slash/admin_slash, saturation, all error variants - earnings: register, zero/negative guard, multi-user/token tracking, i128::MAX saturation, op replay guard 203 tests passing (146 events + 57 profile). Snapshots regenerated. Closes boundlessfi#25
- Add fee-account balance delta assertions to all payout split paths (bounty, hackathon, escrow_fee_math, grant single/sweep/multi-winner) - Add fee-account delta=0 assertions to cancel refund payout tests (full_partner_then_residual, paged_cancel, prorata) - Rename cancel_at_boundary test to cancel_prorata_splits_remaining; remove stale scratch comments, fix section header - Change is_err() to assert_eq!(err, Error::TokenNotSupported) in token whitelist create_event rejection tests - Add register_without_admin_auth_reverts and deregister_without_admin_auth_reverts negative auth tests - Rename register_earnings_requires_events_contract_auth and add register_earnings_without_events_contract_auth_reverts with a real impostor-auth rejection check
Josue19-08
force-pushed
the
feat/issue-25-contract-test-coverage-push
branch
from
eb378fc to
b442ec3
Compare
Author
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/events/src/tests/escrow_fee_math.rs`:
- Around line 191-192: The test in the events escrow fee math case only checks
that try_create_event fails, which is too broad for the MAX_FEE_BPS path. Update
the assertion around ctx.events.try_create_event so it matches the specific
typed Error::InvalidFeeBps from the contract instead of using a generic is_err
check, keeping the validation focused on the intended failure in
try_create_event.
- Around line 127-149: The add_funds_uses_event_override_not_global_default test
only checks the payer and fee-account deltas, so it can miss an incorrect escrow
credit. Update the test to also verify the escrow balance/remaining_escrow
changes by the full amount, asserting it moves from budget to budget + amount
after ctx.events.add_funds is called. Use the existing
add_funds_uses_event_override_not_global_default, create_hackathon, and
ctx.events.add_funds symbols to locate the right assertions.
- Around line 199-215: The payout-path tests in the escrow fee math suite only
assert the winner’s final balance, so update the affected cases in
select_winners_single_100_percent_drains_escrow and the other payout tests to
capture recipient balances before and after the call and assert the delta
explicitly. Use the existing token client and balance checks around
ctx.events.select_winners to compare before/after values for each winner, while
keeping the fee_account delta assertions alongside them. This should be applied
consistently across the single-position, multi-position, and sweep scenarios so
the tests verify both recipient and fee-account balance deltas.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bef14ae6-7176-4eea-a83f-5193180dad02
📒 Files selected for processing (11)
contracts/events/src/tests/bounty_pillar.rscontracts/events/src/tests/cancel_refund.rscontracts/events/src/tests/escrow_fee_math.rscontracts/events/src/tests/grant_pillar.rscontracts/events/src/tests/hackathon_pillar.rscontracts/events/src/tests/mod.rscontracts/events/src/tests/token_whitelist.rscontracts/profile/src/tests/credits.rscontracts/profile/src/tests/earnings.rscontracts/profile/src/tests/mod.rscontracts/profile/src/tests/reputation.rs
💤 Files with no reviewable changes (4)
- contracts/profile/src/tests/mod.rs
- contracts/profile/src/tests/earnings.rs
- contracts/profile/src/tests/credits.rs
- contracts/profile/src/tests/reputation.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- contracts/events/src/tests/mod.rs
- contracts/events/src/tests/token_whitelist.rs
- contracts/events/src/tests/cancel_refund.rs
- contracts/events/src/tests/grant_pillar.rs
- contracts/events/src/tests/bounty_pillar.rs
- contracts/events/src/tests/hackathon_pillar.rs
Comment on lines
127
to
149
| #[test] | ||
| fn partial_position_fill_leaves_residual_escrow() { | ||
| // Fill only 1 of 2 positions → event stays Active with residual escrow. | ||
| fn add_funds_uses_event_override_not_global_default() { | ||
| let ctx = setup(); | ||
| let budget = 10_000_0000000_i128; | ||
| let override_bps: u32 = 50; | ||
| let id = create_hackathon(&ctx, budget, Some(override_bps)); | ||
|
|
||
| let mut dist = Map::new(&ctx.env); | ||
| dist.set(1, 60); | ||
| dist.set(2, 40); | ||
| let id = create_hackathon_with_dist(&ctx, dist); | ||
|
|
||
| let w1 = Address::generate(&ctx.env); | ||
| let winners = soroban_sdk::vec![ | ||
| &ctx.env, | ||
| WinnerSpec { recipient: w1.clone(), position: 1, credit_earn: 10, reputation_bump: 50 }, | ||
| ]; | ||
| let op = BytesN::random(&ctx.env); | ||
| ctx.events.select_winners(&id, &winners, &op); | ||
|
|
||
| let token = token::Client::new(&ctx.env, &ctx.token_addr); | ||
| assert_eq!(token.balance(&w1), TOTAL_BUDGET * 60 / 100); | ||
|
|
||
| let event = ctx.events.get_event(&id); | ||
| assert_eq!(event.remaining_escrow, TOTAL_BUDGET - TOTAL_BUDGET * 60 / 100); | ||
| } | ||
|
|
||
| // ============================================================ | ||
| // release math: partner contributions enlarge the payout pool | ||
| // ============================================================ | ||
|
|
||
| #[test] | ||
| fn partner_funds_grow_winner_payout() { | ||
| let ctx = setup(); | ||
| let id = create_hackathon(&ctx); | ||
| ctx.events.set_fee_bps(&500); | ||
|
|
||
| let partner = Address::generate(&ctx.env); | ||
| let contrib = 500_0000000_i128; | ||
| let fee = contrib * FEE_BPS as i128 / 10_000; | ||
| fund(&ctx, &partner, contrib + fee); | ||
| let op_add = BytesN::random(&ctx.env); | ||
| ctx.events.add_funds(&id, &partner, &contrib, &op_add); | ||
|
|
||
| let event = ctx.events.get_event(&id); | ||
| let escrow_at_select = event.remaining_escrow; | ||
| assert_eq!(escrow_at_select, TOTAL_BUDGET + contrib); | ||
|
|
||
| let winner = Address::generate(&ctx.env); | ||
| let winners = soroban_sdk::vec![ | ||
| &ctx.env, | ||
| WinnerSpec { recipient: winner.clone(), position: 1, credit_earn: 10, reputation_bump: 50 }, | ||
| ]; | ||
| let op = BytesN::random(&ctx.env); | ||
| ctx.events.select_winners(&id, &winners, &op); | ||
|
|
||
| let token = token::Client::new(&ctx.env, &ctx.token_addr); | ||
| assert_eq!(token.balance(&winner), escrow_at_select); | ||
| } | ||
|
|
||
| // ============================================================ | ||
| // release math: claim_milestone (Grant, fixed split) | ||
| // ============================================================ | ||
|
|
||
| #[test] | ||
| fn grant_milestone_pays_floored_per_milestone() { | ||
| let ctx = setup(); | ||
| let milestones = 3_u32; | ||
| let id = create_grant(&ctx, milestones); | ||
|
|
||
| let recipient = Address::generate(&ctx.env); | ||
| let winners = soroban_sdk::vec![ | ||
| &ctx.env, | ||
| WinnerSpec { recipient: recipient.clone(), position: 1, credit_earn: 10, reputation_bump: 50 }, | ||
| ]; | ||
| let op_sel = BytesN::random(&ctx.env); | ||
| ctx.events.select_winners(&id, &winners, &op_sel); | ||
| let amount = 1_000_0000000_i128; | ||
| let expected_fee = amount * override_bps as i128 / 10_000; | ||
| ctx.token_admin.mint(&partner, &(amount + expected_fee + 1_000_0000000)); | ||
|
|
||
| let token = token::Client::new(&ctx.env, &ctx.token_addr); | ||
| let partner_before = token.balance(&partner); | ||
| let fee_before = token.balance(&ctx.fee_account); | ||
|
|
||
| // total_share = TOTAL_BUDGET * 100 / 100 = TOTAL_BUDGET | ||
| // per_milestone_floored = TOTAL_BUDGET / 3 = 333_3333333 (floor) | ||
| let per_milestone = TOTAL_BUDGET / milestones as i128; | ||
|
|
||
| // Milestone 0 | ||
| let op_m0 = BytesN::random(&ctx.env); | ||
| ctx.events.claim_milestone(&id, &recipient, &0, &10, &50, &op_m0); | ||
| assert_eq!(token.balance(&recipient), per_milestone); | ||
|
|
||
| // Milestone 1 | ||
| let op_m1 = BytesN::random(&ctx.env); | ||
| ctx.events.claim_milestone(&id, &recipient, &1, &10, &50, &op_m1); | ||
| assert_eq!(token.balance(&recipient), per_milestone * 2); | ||
|
|
||
| // Milestone 2 (last): sweep — pays total_share - already_paid | ||
| let op_m2 = BytesN::random(&ctx.env); | ||
| ctx.events.claim_milestone(&id, &recipient, &2, &10, &50, &op_m2); | ||
| assert_eq!( | ||
| token.balance(&recipient), | ||
| TOTAL_BUDGET, | ||
| "last milestone sweep must pay entire remaining share" | ||
| ); | ||
|
|
||
| let event = ctx.events.get_event(&id); | ||
| assert_eq!(event.remaining_escrow, 0); | ||
| } | ||
|
|
||
| #[test] | ||
| fn grant_milestone_double_claim_rejected() { | ||
| let ctx = setup(); | ||
| let id = create_grant(&ctx, 2); | ||
|
|
||
| let recipient = Address::generate(&ctx.env); | ||
| let winners = soroban_sdk::vec![ | ||
| &ctx.env, | ||
| WinnerSpec { recipient: recipient.clone(), position: 1, credit_earn: 10, reputation_bump: 50 }, | ||
| ]; | ||
| let op_sel = BytesN::random(&ctx.env); | ||
| ctx.events.select_winners(&id, &winners, &op_sel); | ||
|
|
||
| let op_m0 = BytesN::random(&ctx.env); | ||
| ctx.events.claim_milestone(&id, &recipient, &0, &10, &50, &op_m0); | ||
|
|
||
| // Same milestone again → MilestoneAlreadyClaimed | ||
| let op_m0_dup = BytesN::random(&ctx.env); | ||
| let res = ctx.events.try_claim_milestone(&id, &recipient, &0, &10, &50, &op_m0_dup); | ||
| assert!(res.is_err(), "double claim_milestone must revert"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn grant_milestone_out_of_range_rejected() { | ||
| let ctx = setup(); | ||
| let milestones = 2_u32; | ||
| let id = create_grant(&ctx, milestones); | ||
|
|
||
| let recipient = Address::generate(&ctx.env); | ||
| let winners = soroban_sdk::vec![ | ||
| &ctx.env, | ||
| WinnerSpec { recipient: recipient.clone(), position: 1, credit_earn: 10, reputation_bump: 50 }, | ||
| ]; | ||
| let op_sel = BytesN::random(&ctx.env); | ||
| ctx.events.select_winners(&id, &winners, &op_sel); | ||
| ctx.events.add_funds(&id, &partner, &amount, &BytesN::random(&ctx.env)); | ||
|
|
||
| // Milestone 2 is out-of-range for Multi(2) (valid: 0, 1) | ||
| let op = BytesN::random(&ctx.env); | ||
| let res = ctx.events.try_claim_milestone(&id, &recipient, &2, &10, &50, &op); | ||
| assert!(res.is_err(), "milestone >= total_milestones must revert"); | ||
| assert_eq!(partner_before - token.balance(&partner), amount + expected_fee); | ||
| assert_eq!(token.balance(&ctx.fee_account) - fee_before, expected_fee); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the escrow delta in the override add_funds test.
This verifies the payer charge and fee-account credit, but not that the event actually receives the full amount. A bug that charges amount + expected_fee correctly while crediting less than amount to remaining_escrow would still pass here. Add an assertion that the escrow moves from budget to budget + amount.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/events/src/tests/escrow_fee_math.rs` around lines 127 - 149, The
add_funds_uses_event_override_not_global_default test only checks the payer and
fee-account deltas, so it can miss an incorrect escrow credit. Update the test
to also verify the escrow balance/remaining_escrow changes by the full amount,
asserting it moves from budget to budget + amount after ctx.events.add_funds is
called. Use the existing add_funds_uses_event_override_not_global_default,
create_hackathon, and ctx.events.add_funds symbols to locate the right
assertions.
Comment on lines
+191
to
+192
| let res = ctx.events.try_create_event(¶ms, &BytesN::random(&ctx.env)); | ||
| assert!(res.is_err()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the specific contract error here.
assert!(res.is_err()) will pass for any failure on this path, so it does not prove the MAX_FEE_BPS validation is what fired. Compare the result against the typed Error::InvalidFeeBps instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/events/src/tests/escrow_fee_math.rs` around lines 191 - 192, The
test in the events escrow fee math case only checks that try_create_event fails,
which is too broad for the MAX_FEE_BPS path. Update the assertion around
ctx.events.try_create_event so it matches the specific typed
Error::InvalidFeeBps from the contract instead of using a generic is_err check,
keeping the validation focused on the intended failure in try_create_event.
Comment on lines
199
to
+215
| #[test] | ||
| fn select_winners_on_nonexistent_event_reverts() { | ||
| let ctx = setup(); | ||
| let winner = Address::generate(&ctx.env); | ||
| let winners = soroban_sdk::vec![ | ||
| &ctx.env, | ||
| WinnerSpec { recipient: winner.clone(), position: 1, credit_earn: 10, reputation_bump: 50 }, | ||
| ]; | ||
| let op = BytesN::random(&ctx.env); | ||
| let res = ctx.events.try_select_winners(&999_u64, &winners, &op); | ||
| assert!(res.is_err()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn select_winners_duplicate_position_reverts() { | ||
| let ctx = setup(); | ||
| let mut dist = Map::new(&ctx.env); | ||
| dist.set(1, 50); | ||
| dist.set(2, 50); | ||
| let id = create_hackathon_with_dist(&ctx, dist); | ||
|
|
||
| let w1 = Address::generate(&ctx.env); | ||
| let w2 = Address::generate(&ctx.env); | ||
| let winners = soroban_sdk::vec![ | ||
| &ctx.env, | ||
| WinnerSpec { recipient: w1.clone(), position: 1, credit_earn: 10, reputation_bump: 50 }, | ||
| WinnerSpec { recipient: w2.clone(), position: 1, credit_earn: 10, reputation_bump: 30 }, | ||
| ]; | ||
| let op = BytesN::random(&ctx.env); | ||
| let res = ctx.events.try_select_winners(&id, &winners, &op); | ||
| assert!(res.is_err(), "duplicate winner position must revert"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn select_winners_invalid_position_reverts() { | ||
| fn select_winners_single_100_percent_drains_escrow() { | ||
| let ctx = setup(); | ||
| let id = create_hackathon(&ctx); // dist has only position 1 | ||
| let budget = 10_000_0000000_i128; | ||
| let id = create_hackathon(&ctx, budget, None); | ||
|
|
||
| let w = Address::generate(&ctx.env); | ||
| let winners = soroban_sdk::vec![ | ||
| &ctx.env, | ||
| WinnerSpec { recipient: w.clone(), position: 99, credit_earn: 10, reputation_bump: 50 }, | ||
| ]; | ||
| let op = BytesN::random(&ctx.env); | ||
| let res = ctx.events.try_select_winners(&id, &winners, &op); | ||
| assert!(res.is_err(), "position not in distribution must revert"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn select_winners_empty_list_reverts() { | ||
| let ctx = setup(); | ||
| let id = create_hackathon(&ctx); | ||
|
|
||
| let winners = soroban_sdk::vec![&ctx.env]; | ||
| let op = BytesN::random(&ctx.env); | ||
| let res = ctx.events.try_select_winners(&id, &winners, &op); | ||
| assert!(res.is_err(), "empty winners list must revert"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn select_winners_twice_reverts() { | ||
| let ctx = setup(); | ||
| let id = create_hackathon(&ctx); | ||
|
|
||
| let w = Address::generate(&ctx.env); | ||
| let token = token::Client::new(&ctx.env, &ctx.token_addr); | ||
| let fee_before = token.balance(&ctx.fee_account); | ||
| let winner = Address::generate(&ctx.env); | ||
| let winners = soroban_sdk::vec![ | ||
| &ctx.env, | ||
| WinnerSpec { recipient: w.clone(), position: 1, credit_earn: 10, reputation_bump: 50 }, | ||
| WinnerSpec { recipient: winner.clone(), position: 1, credit_earn: 0, reputation_bump: 0 }, | ||
| ]; | ||
| ctx.events.select_winners(&id, &winners, &BytesN::random(&ctx.env)); | ||
|
|
||
| let op1 = BytesN::random(&ctx.env); | ||
| ctx.events.select_winners(&id, &winners, &op1); | ||
|
|
||
| // Second selection on the same event → WinnersAlreadySelected | ||
| let op2 = BytesN::random(&ctx.env); | ||
| let res = ctx.events.try_select_winners(&id, &winners, &op2); | ||
| assert!(res.is_err(), "second select_winners on same event must revert"); | ||
| assert_eq!(token.balance(&winner), budget); | ||
| assert_eq!(token.balance(&ctx.fee_account) - fee_before, 0); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Track recipient balance deltas explicitly in the payout-path tests.
These cases now cover the fee-account side, but the recipient side still asserts terminal balances on fresh addresses rather than balance deltas. Recording before/after balances for winners makes the payout invariant explicit and keeps the tests correct if setup ever starts pre-funding recipients. As per coding guidelines, “Every payout split path (single-position, multi-position, and sweep) must have tests that assert both the recipient balance delta and the fee account balance delta.”
Also applies to: 225-263, 270-293
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/events/src/tests/escrow_fee_math.rs` around lines 199 - 215, The
payout-path tests in the escrow fee math suite only assert the winner’s final
balance, so update the affected cases in
select_winners_single_100_percent_drains_escrow and the other payout tests to
capture recipient balances before and after the call and assert the delta
explicitly. Use the existing token client and balance checks around
ctx.events.select_winners to compare before/after values for each winner, while
keeping the fee_account delta assertions alongside them. This should be applied
consistently across the single-position, multi-position, and sweep scenarios so
the tests verify both recipient and fee-account balance deltas.
Source: Coding guidelines
Benjtalkshow
requested changes
Jun 26, 2026
There was a problem hiding this comment.
This PR is described as adding coverage, but it actually drops the suite from 226 tests to 209. It deletes 62 tests from five files that are already merged and adds 45 in new modules, so the net is a loss of 17.
The deleted tests matter. They cover things like the MAX_FEE_BPS guard (L4), the M1 escrow snapshot fix, the cross-contract access control checks, and the recipient and fee account payout assertions we're required to keep. None of that is redundant.
The new modules are good though. cancel_refund, grant_pillar, token_whitelist, and earnings all add real value and should land.
So my suggestion: revert the changes to the five merged files, rebase onto current testnet, and keep this PR additive only. That should put the suite around 271 tests.
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.
Description
Implements the full test coverage matrix described in #25. Adds 6 new test modules for
boundless-eventsand 2 forboundless-profile(bounty_pillar and hackathon_pillar were already merged via #46 and #47; credits via #48).Changes
boundless-events:
token_whitelist—register_supported_token,deregister_supported_token,create_eventrejection of unsupported/deregistered tokenscancel_refund— paged cancel flow:OwnerOnly,FullPartnerThenResidual,ProRata, pagination across batches, all error variantsescrow_fee_math— default fee, per-event override,add_fundsrate snapshot, waiver (0 bps),MAX_FEE_BPSguard, single/60-40 payout math, admin fee updategrant_pillar—Multirelease required, fixed-split milestone math, last-milestone dust sweep, credit/rep side-effects, all error variantsboundless-profile:
earnings—register_earnings, zero/negative guard, multi-user/token tracking,i128::MAXsaturation, op replay guardreputation—bump/slash/admin_slash, saturation, all error variants + op replay + authResult: 206 tests passing (145 events + 61 profile), zero warnings.
Closes
Closes #25
Notes
soroban_sdk::Vecshadowsstd::vec::Vecin test scope — array literals are used instead of.collect()to avoid the name collision.Summary by CodeRabbit