feat(events): replace push-based select_winners with pull-model claim_prize for unlimited winners #83
feat(events): replace push-based select_winners with pull-model claim_prize for unlimited winners #83Shadow-MMN wants to merge 14 commits into
Conversation
…_prize for unlimited winners (boundlessfi#61) select_winners for ReleaseKind::Single (bounties, hackathons) previously paid every winner inline — N × (token transfer + 3 cross-contract calls) in one transaction, capped at 50 to stay under Soroban's per-tx budget. Now select_winners only records winner records. Each winner calls claim_prize() in their own transaction to receive their payout, matching the existing claim_milestone pattern already used for Grants/Crowdfunding. Changes: - select_winners Single branch: record winners without paying inline - Add claim_prize(): token release + profile calls per-winner per-tx - Add PrizeClaimed DataKey + event + storage for claim idempotency - Raise MAX_WINNERS_PER_SELECT from 50 to 5000 (pull-model is cheaper) - 180 tests pass, snapshots updated
Conflicts resolved: - event_ops.rs: take pull-model code (record-only select_winners, claim_prize) - types.rs: add PrizeClaimed DataKey variant - contributions.rs / cross_contract.rs / hackathon_pillar.rs: add claim_prize calls in tests - Strip explanatory comments per boundlessfi#81 convention
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughSingle-release winner selection now records unpaid winners instead of paying immediately. A new ChangesSingle-release pull claim flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Organizer
participant EventsContract
participant Recipient
participant Escrow
participant ProfileClient
Organizer->>EventsContract: select_winners(winners)
EventsContract-->>Organizer: unpaid winner records
Recipient->>EventsContract: claim_prize(recipient, position, op_id)
EventsContract->>Escrow: release prize amount
EventsContract->>ProfileClient: update recipient profile
EventsContract-->>Recipient: PrizeClaimed
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
|
@Shadow-MMN Please resolve conflict |
I will , gimme some time please |
Resolved snapshot file conflicts by accepting upstream deletion (snapshots are gitignored per repo convention). Upstream changes: - chore: stop tracking test snapshots; regenerate locally instead (boundlessfi#84) - test(admin): assert admin auth is actually required, not just mocked (boundlessfi#78) - fix(events): harden cancellation liveness (boundlessfi#82)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/events/src/tests/hackathon_pillar.rs (1)
377-404: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAssert recipient balance deltas for the multi-position payout path.
These generated accounts currently start at zero, but the required contract is explicitly a delta assertion. Snapshot each recipient before claiming and compare post-claim changes.
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.”
🤖 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/hackathon_pillar.rs` around lines 377 - 404, Update the multi-position payout test around the three claim_prize calls to snapshot each recipient’s token balance before claiming, then assert post-claim balance deltas equal amt_1, amt_2, and amt_3 rather than asserting absolute balances. Preserve the existing fee-account delta assertion and other payout/reputation checks.Source: Coding guidelines
🤖 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/event_ops.rs`:
- Around line 673-679: Update the winner-selection and claim flow around
select_winners and claim_prize to persist the manager-approved
WinnerSpec.reputation_bump in each claim entitlement, then derive the award from
that stored value during claiming. Remove reputation_bump from claim_prize’s
public inputs and prevent callers from supplying or overriding the approved
amount.
- Around line 748-759: Update the winner persistence flow around the per-claim
append operations to ensure get_winners can return every claim row for a full
selection, rather than truncating claims behind the 5,000-row limit. Prefer
updating each existing anchor in place with its claim data instead of appending
duplicate Winner rows, and apply the same change to the corresponding flow near
the second referenced append.
- Around line 21-29: Reduce MAX_WINNERS_PER_SELECT from 5,000 to a cap
compatible with the current table-backed implementation, unless implementing
keyed winner lookup or batched recording. Preserve the existing select_winners
and claim_prize behavior while ensuring duplicate checking, per-winner writes,
and linear claim scans remain within Soroban limits.
In `@contracts/events/src/types.rs`:
- Around line 182-183: Move the PrizeClaimed variant to the end of the DataKey
enum, preserving the existing order of all earlier variants and keeping its
fields unchanged so later storage key discriminants remain stable.
---
Outside diff comments:
In `@contracts/events/src/tests/hackathon_pillar.rs`:
- Around line 377-404: Update the multi-position payout test around the three
claim_prize calls to snapshot each recipient’s token balance before claiming,
then assert post-claim balance deltas equal amt_1, amt_2, and amt_3 rather than
asserting absolute balances. Preserve the existing fee-account delta assertion
and other payout/reputation checks.
🪄 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: c433d673-a1bf-4918-ae3c-fc673e119447
📒 Files selected for processing (12)
contracts/events/src/errors.rscontracts/events/src/event_ops.rscontracts/events/src/events.rscontracts/events/src/lib.rscontracts/events/src/storage.rscontracts/events/src/tests/bounty_pillar.rscontracts/events/src/tests/cancel_refund.rscontracts/events/src/tests/contributions.rscontracts/events/src/tests/cross_contract.rscontracts/events/src/tests/escrow_fee_math.rscontracts/events/src/tests/hackathon_pillar.rscontracts/events/src/types.rs
…_bump storage, in-place claim update, DataKey ordering - Reduce MAX_WINNERS_PER_SELECT from 5,000 to 500 (compatible with table-backed storage + linear scans in claim_prize) - Add reputation_bump field to Winner struct, stored from WinnerSpec during select_winners; remove caller-supplied param from both claim_prize and claim_milestone (manager-approved value only) - claim_prize updates anchor row in-place via set_winner_at instead of appending duplicate Winner rows; claim_milestone preserves append for multi-milestone support - Move PrizeClaimed variant to end of DataKey enum so existing key discriminants remain stable on upgrade - All 197 tests pass, wasm build clean
![almanax-ai[bot]](https://avatars.githubusercontent.com/in/934833?s=60&v=4){kind=small}
Manager could call start_cancel immediately after select_winners, draining escrow before winners call claim_prize — permanently preventing payouts. Added has_unclaimed_single_winner() guard in start_cancel that rejects cancellation when any anchor row has paid_at: None. After all winners claim (anchors updated to paid_at: Some(...)), cancellation of residual escrow still permitted.
The PrizeClaimed flag (fast-path) can TTL-expire while winner rows stay live from get_winners/winner_at reads. Added && paid_at.is_none() to the anchor scan so a claimed anchor (paid_at: Some(...)) prevents replay even if the separate flag has expired.
Two findings from Almanax review: 1. Winner deserialization on upgrade (High): reputation_bump was added as a non-optional u32, which would brick reads of old on-chain Winner entries after upgrade. Changed to Option<u32> — old entries deserialize as None (treated as 0), new entries store Some(x). 2. Linear winner scan in claim_prize (Medium): claim_prize scanned all winners to find the anchor row, approaching budget limits at higher winner counts. Added DataKey::WinnerIndex stored at selection time for O(1) lookup in claim_prize. - Winner.reputation_bump: u32 → Option<u32> - DataKey::WinnerIndex variant + storage get/set helpers - select_winners stores index + Some() reputation_bump - claim_prize uses index lookup, falls back to PrizeAlreadyClaimed if paid_at.is_some() (canonical guard) - claim_milestone uses .unwrap_or(0) on read, Some() on write
If a Single-release winner cannot authenticate (lost key, contract without signer), escrow is permanently locked — claim_prize requires recipient auth and start_cancel rejects unclaimed winners. Allow cancellation after the event deadline passes: the deadline is the natural claim window. Winners who haven't claimed by then forfeit their prize, and the manager can recover remaining escrow.
|
@0xdevcollins , we can now check this |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/events/src/event_ops.rs (1)
847-854: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestore the snapshot limit multiplier to prevent truncating milestone claims.
By reducing the snapshot limit to
MAX_WINNERS_PER_SELECT, this function will silently truncate the claim history forMulti-release events (such as Grants and Crowdfunding).Unlike
Single-release events which now update their anchor rows in-place,Multi-release events still append a new row for every claimed milestone. For example, 200 winners claiming 3 milestones generates 600 rows. Without a multiplier, the last 100 rows will be entirely excluded from this read. Restore a sufficient multiplier (e.g.,.saturating_mul(20)) to ensure the full history is returned.💡 Proposed fix
pub fn get_winners(env: &Env, event_id: u64) -> Result<Vec<Winner>, Error> { storage::get_event(env, event_id).ok_or(Error::EventNotFound)?; Ok(storage::winners_snapshot( env, event_id, - MAX_WINNERS_PER_SELECT, + MAX_WINNERS_PER_SELECT.saturating_mul(20), )) }🤖 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/event_ops.rs` around lines 847 - 854, Update get_winners to multiply MAX_WINNERS_PER_SELECT by a sufficient multiplier, such as saturating_mul(20), before passing the limit to storage::winners_snapshot, preserving the full milestone claim history for Multi-release events.
🧹 Nitpick comments (1)
contracts/events/src/grant.rs (1)
57-80: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftConsider optimizing the linear scan to prevent out-of-gas errors on late claims.
With
MAX_WINNERS_PER_SELECTat 500, aMulti-release event with several milestones can accumulate thousands of appended claim rows. Sinceclaim_milestoneperforms a linear scan over allwinner_countrows (storage::winner_atpersistent reads) to calculatealready_claimed_for_recipientandalready_paid_to_recipient, late milestone claims risk exceeding the Soroban persistent read limits (typically 2048).Consider using
WinnerIndexfor Grants to find the anchor row, and calculating previously claimed amounts mathematically (e.g.,claimed_milestones * per_milestone_amount) to avoid the O(N) persistent reads.🤖 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/grant.rs` around lines 57 - 80, Optimize the recipient lookup in claim_milestone by using the Grants WinnerIndex to locate the anchor winner instead of scanning every row from storage::winner_count through storage::winner_at. Derive already_claimed_for_recipient and already_paid_to_recipient from the claimed milestone count and per-milestone amount, while preserving the existing anchor winner’s reputation_bump and position 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.
Outside diff comments:
In `@contracts/events/src/event_ops.rs`:
- Around line 847-854: Update get_winners to multiply MAX_WINNERS_PER_SELECT by
a sufficient multiplier, such as saturating_mul(20), before passing the limit to
storage::winners_snapshot, preserving the full milestone claim history for
Multi-release events.
---
Nitpick comments:
In `@contracts/events/src/grant.rs`:
- Around line 57-80: Optimize the recipient lookup in claim_milestone by using
the Grants WinnerIndex to locate the anchor winner instead of scanning every row
from storage::winner_count through storage::winner_at. Derive
already_claimed_for_recipient and already_paid_to_recipient from the claimed
milestone count and per-milestone amount, while preserving the existing anchor
winner’s reputation_bump and position behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89529271-38dd-470d-b384-fd6bcd93c994
📒 Files selected for processing (13)
contracts/events/src/event_ops.rscontracts/events/src/grant.rscontracts/events/src/lib.rscontracts/events/src/storage.rscontracts/events/src/tests/bounty_pillar.rscontracts/events/src/tests/cancel_refund.rscontracts/events/src/tests/contributions.rscontracts/events/src/tests/cross_contract.rscontracts/events/src/tests/crowdfunding.rscontracts/events/src/tests/escrow_fee_math.rscontracts/events/src/tests/grant_pillar.rscontracts/events/src/tests/hackathon_pillar.rscontracts/events/src/types.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- contracts/events/src/tests/cancel_refund.rs
- contracts/events/src/tests/contributions.rs
- contracts/events/src/tests/bounty_pillar.rs
2 participants
Summary
This PR removes the 50-winner hard cap on bounties/competitions by replacing the push-based payout model with a pull/claim model, winners claim their prize individually via
claim_prize(), one transaction per winner, matching the same pattern already proven for Grants/Crowdfunding viaclaim_milestone().select_winnerspays all winners inlineselect_winnersonly records winnersclaim_prizeexistsclaim_prize()individuallyKey changes
event_ops.rs:select_winnersSingle branch now records winners with pre-computed amounts but defers all token transfers and profile calls toclaim_prizeevent_ops.rs: Newclaim_prize()function — per-winner pull payout with full idempotency, auth, and completion logicerrors.rs/types.rs/storage.rs/events.rs: Supporting types (PrizeClaimed), storage helpers, and eventsMAX_WINNERS_PER_SELECTraised from 50 → 5,000 since the per-winner cost inselect_winnersis now just a storage writeTesting
claim_prizeafterselect_winnersall verified
Closes #61
Summary by CodeRabbit
PrizeClaimednotification.