feat(events): add claimed winners tracking and winner update helper in storage#62
feat(events): add claimed winners tracking and winner update helper in storage#62syed-ghufran-hassan wants to merge 1 commit into
Conversation
…n storage - Add DataKey::ClaimedWinnersCount to track per‑event claim progress - Implement set_claimed_winners_count / get_claimed_winners_count - Add update_winner_paid to mark a specific winner as paid - Add set_winner_at helper to overwrite a winner entry at a given index These support the new pull‑claim payout model for bounties/competitions.
📝 WalkthroughWalkthroughThe winner selection cap in the events contract was raised from 50 to 500, and single-release winner selection was changed to defer payouts: winners are stored unpaid with a ChangesDeferred Prize Claiming
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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)
645-661: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift500-winner cap exceeds Soroban tx limits. At 500 winners,
select_winnersdoes ~124k duplicate comparisons and, in the Single path, writes ~1,000 ledger entries (winner rows + counter updates) plus 500 contract events. That is far above the current ~100-write transaction cap, so this path will revert before reaching the new limit. Lower the cap or split selection/persistence across multiple transactions.🤖 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 645 - 661, The new 500-winner limit in select_winners exceeds Soroban transaction write and execution limits, so the single-transaction winner selection flow will still revert. Update the cap or redesign the select_winners / winner persistence path to batch or split writes across multiple transactions, and review the duplicate-check loop over winners plus the ledger-writing logic in the Single path to keep it within current limits.
🧹 Nitpick comments (2)
contracts/events/src/event_ops.rs (2)
810-817: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove leftover developer musings.
These comments are stream-of-consciousness notes ("Option 1: store winner index in a map? ... For simplicity, we'll remove and re-append?") that don't describe the final implementation and add noise. Replace with a one-line description of
update_winner_paid.♻️ Suggested cleanup
- // --- Mark this winner as paid (update the stored Winner record) --- - // We need to update the specific winner row. Since we don't have an index, - // we can find it again and overwrite. But we already have the record. - // Better: we can store winners as a vector and update in place by scanning again. - // For simplicity, we'll remove and re‑append? That would change order. - // Option 1: store winner index in a map? Or just update by scanning and replacing. - // We'll implement a helper: update_winner_paid(env, event_id, recipient, now) - // that scans and sets paid_at. + // Mark this winner as paid by scanning for the unpaid record and setting paid_at. storage::update_winner_paid(env, event_id, &recipient, env.ledger().timestamp());🤖 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 810 - 817, Remove the leftover planning/musings in the winner-paid section and replace them with a single concise comment that describes the final behavior of update_winner_paid; reference the helper name update_winner_paid and keep only the implementation summary (scan the stored winners and mark the matching record as paid_at), without any option brainstorming or alternative approaches.
826-829: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueUse
>=for the completion check.If
new_claimedever overshootstotal_winners(e.g., a future change adds extra winner rows or a recount), the strict==would silently skip completion, leaving the eventActiveindefinitely.>=is more robust.🛡️ Proposed change
- if new_claimed == total_winners { + if new_claimed >= total_winners { event.status = EventStatus::Completed; }🤖 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 826 - 829, The completion check in the event claim flow uses a strict equality against storage::winner_count, which can leave EventStatus::Completed unset if new_claimed overshoots the total. Update the logic in the relevant event_ops claim/completion path to treat any value at or above total_winners as complete by changing the comparison to a non-strict check. Keep the fix localized around the event.status assignment so the EventStatus transition still happens in the same place.
🤖 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`:
- Line 25: The winner-selection cap is currently hardcoded as a module constant,
but it needs to be configurable per event instead. Move MAX_WINNERS_PER_SELECT
into EventRecord or the relevant variant payload, and update any selection logic
in event_ops to read the cap from the event data rather than from a global
constant. Ensure the existing winner-selection path uses the per-event value
everywhere that references this cap.
- Line 784: The `winner_record.ok_or(Error::WinnerNotFound)?` path in
`event_ops::` references a non-existent error variant, so update the error
handling to use a valid winner-related error. Either add `WinnerNotFound` to the
`Error` enum in `errors.rs` and wire it through the existing error conversions,
or replace it with an existing variant such as `InvalidWinnerPosition` or
`SubmissionNotFound` where appropriate.
- Around line 751-757: The claim_prize flow currently trusts the caller-provided
reputation_bump, letting the recipient influence their own reputation. Update
claim_prize to derive the reputation delta from trusted event state or other
server-controlled data before calling profile.bump_reputation, and avoid
forwarding the raw input directly. Use the claim_prize function and the
profile.bump_reputation call site to locate and replace the untrusted value
handling.
---
Outside diff comments:
In `@contracts/events/src/event_ops.rs`:
- Around line 645-661: The new 500-winner limit in select_winners exceeds
Soroban transaction write and execution limits, so the single-transaction winner
selection flow will still revert. Update the cap or redesign the select_winners
/ winner persistence path to batch or split writes across multiple transactions,
and review the duplicate-check loop over winners plus the ledger-writing logic
in the Single path to keep it within current limits.
---
Nitpick comments:
In `@contracts/events/src/event_ops.rs`:
- Around line 810-817: Remove the leftover planning/musings in the winner-paid
section and replace them with a single concise comment that describes the final
behavior of update_winner_paid; reference the helper name update_winner_paid and
keep only the implementation summary (scan the stored winners and mark the
matching record as paid_at), without any option brainstorming or alternative
approaches.
- Around line 826-829: The completion check in the event claim flow uses a
strict equality against storage::winner_count, which can leave
EventStatus::Completed unset if new_claimed overshoots the total. Update the
logic in the relevant event_ops claim/completion path to treat any value at or
above total_winners as complete by changing the comparison to a non-strict
check. Keep the fix localized around the event.status assignment so the
EventStatus transition still happens in the same place.
🪄 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: e9f12424-9890-43b3-9565-4c12a283cd3e
📒 Files selected for processing (1)
contracts/events/src/event_ops.rs
|
|
||
| const MAX_TITLE_LEN: u32 = 120; | ||
| const MAX_WINNERS_PER_SELECT: u32 = 50; | ||
| const MAX_WINNERS_PER_SELECT: u32 = 500; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Winner-selection cap should be per-event, not a global constant.
MAX_WINNERS_PER_SELECT is a per-program cap that sales may want to vary; per guidelines it should live on EventRecord/its variant payload rather than a module constant. The 10× increase (50→500) also amplifies the downstream cost flagged below.
As per coding guidelines: "anything that sales may vary per program (such as fees, windows, or caps) must live on EventRecord or its variant payload, not in module constants."
🤖 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` at line 25, The winner-selection cap is
currently hardcoded as a module constant, but it needs to be configurable per
event instead. Move MAX_WINNERS_PER_SELECT into EventRecord or the relevant
variant payload, and update any selection logic in event_ops to read the cap
from the event data rather than from a global constant. Ensure the existing
winner-selection path uses the per-event value everywhere that references this
cap.
Source: Coding guidelines
| pub fn claim_prize( | ||
| env: &Env, | ||
| event_id: u64, | ||
| recipient: Address, | ||
| reputation_bump: u32, | ||
| op_id: BytesN<32>, | ||
| ) -> Result<(), Error> { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and inspect the surrounding implementation.
ast-grep outline contracts/events/src/event_ops.rs --view expanded || true
echo '--- claim_prize context ---'
sed -n '740,830p' contracts/events/src/event_ops.rs
echo '--- search for reputation bump usage ---'
rg -n "reputation_bump|bump_reputation|claim_prize" contracts/events/src contracts/profile/src contracts -g '*.rs'Repository: boundlessfi/boundless-contract
Length of output: 21111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- profile bump_reputation implementation ---'
sed -n '1,240p' contracts/profile/src/lib.rs
echo '--- winner selection input/types ---'
sed -n '150,230p' contracts/events/src/types.rs
echo '--- grant claim flow for comparison ---'
sed -n '1,240p' contracts/events/src/grant.rsRepository: boundlessfi/boundless-contract
Length of output: 17838
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- select_winners context ---'
sed -n '608,750p' contracts/events/src/event_ops.rs
echo '--- profile reputation bump logic ---'
sed -n '1,240p' contracts/profile/src/reputation.rs
echo '--- storage lookups for winner data ---'
rg -n "WinnerSpec|winner_distribution|reputation_bump|append_winner|set_winner|winner_count|winner_at" contracts/events/src -g '*.rs'Repository: boundlessfi/boundless-contract
Length of output: 26083
Recipient controls their own reputation_bump.
claim_prize forwards the caller-supplied value straight into profile.bump_reputation, so the recipient can self-award arbitrary reputation. Derive the delta from trusted event state 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/event_ops.rs` around lines 751 - 757, The claim_prize
flow currently trusts the caller-provided reputation_bump, letting the recipient
influence their own reputation. Update claim_prize to derive the reputation
delta from trusted event state or other server-controlled data before calling
profile.bump_reputation, and avoid forwarding the raw input directly. Use the
claim_prize function and the profile.bump_reputation call site to locate and
replace the untrusted value handling.
| } | ||
| } | ||
| } | ||
| let winner = winner_record.ok_or(Error::WinnerNotFound)?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP '\bWinnerNotFound\b' contracts/events/src/errors.rs || echo "WinnerNotFound NOT found in errors.rs"Repository: boundlessfi/boundless-contract
Length of output: 206
🏁 Script executed:
#!/bin/bash
sed -n '1,220p' contracts/events/src/errors.rs
printf '\n---\n'
rg -n 'NotFound|Winner|Submission|Position' contracts/events/src/errors.rs contracts/events/src/event_ops.rsRepository: boundlessfi/boundless-contract
Length of output: 7082
Error::WinnerNotFound does not exist. Add the missing variant in contracts/events/src/errors.rs or return an existing winner-related error such as Error::InvalidWinnerPosition/Error::SubmissionNotFound.
🤖 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` at line 784, The
`winner_record.ok_or(Error::WinnerNotFound)?` path in `event_ops::` references a
non-existent error variant, so update the error handling to use a valid
winner-related error. Either add `WinnerNotFound` to the `Error` enum in
`errors.rs` and wire it through the existing error conversions, or replace it
with an existing variant such as `InvalidWinnerPosition` or `SubmissionNotFound`
where appropriate.
1 participant
These support the new pull‑claim payout model for bounties/competitions.
This is interlinked to #61
Summary by CodeRabbit
New Features
Bug Fixes