feat(distribution): exact O(1) payouts in token units, and fix the O(n^2) basis-point path - #120
feat(distribution): exact O(1) payouts in token units, and fix the O(n^2) basis-point path#120starknetdev wants to merge 10 commits into
Conversation
Both weighted distributions normalize a position's raw weight against the
sum of every position's weight. That sum was rebuilt inside every single
share computation, so `calculate_total` — and with it `calculate_dust` and
the winner's `calculate_share_with_dust` — re-derived the whole vector once
per position: O(n^2) work, with an O(n^2) count of `FixedTrait::pow` calls
for Exponential.
Payout index 1 pays that cost on every claim, because it is the only index
that collects dust. Winners were therefore the most expensive claim in a
tournament by an order of magnitude, scaling quadratically with paid places.
Hoist the weights into a vector built once per call, and read shares,
totals and dust off it. Single-share cost is unchanged in complexity (still
O(n) — the normalization sum is inherent); the winner's path drops from
O(n^2) to O(n).
Values are unchanged. The weight expressions and their accumulation order
are preserved exactly, so the fixed-point results are bit-identical; this
was verified by sweeping the old and new implementations against each other
across every payout index of every size 1..=12 for Linear (weights 10/25/7),
Exponential (weights 10/15/25/100) and Uniform, at full and partial
available_share, plus the zero-payouts and out-of-range edges. The
zero-payout quirk where index 1 collects the entire available_share as dust
is deliberately preserved — Budokan can reach it with an empty leaderboard.
Adds `test_share_regression` pinning the exact basis points (ranges would
not catch a few-bps drift) and `test_gas_benchmark` making the cost curve
visible.
l2_gas, winner's share incl. dust:
Exponential w10, 10 places 10,535,820 -> 1,691,510 (6.2x)
Exponential w10, 25 places 56,570,220 -> 4,019,060 (14.1x)
Exponential w10, 50 places 214,644,220 -> 7,898,310 (27.2x)
Exponential w15, 10 places 77,452,060 -> 7,245,380 (10.7x)
Exponential w15, 25 places 459,709,180 -> 18,940,140 (24.3x)
Linear w10, 10 places 8,436,360 -> 1,426,910 (5.9x)
Linear w10, 50 places 169,699,960 -> 6,577,310 (25.8x)
Non-winner positions are unchanged (within noise), as are Uniform and
Custom, which never normalized against a weight sum.
End to end, Budokan's `test_exponential_100_distribution_with_10_positions`
(create + 10 entries + 10 claims) drops from ~71.6M to ~52.9M l2_gas, -26%.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
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:
📝 WalkthroughWalkthroughChangesThe distribution system adds geometric and tiered variants, exact token-unit payouts, reusable weighted-share calculations, expanded packed storage, indexed custom-share access, and regression and benchmark coverage. Distribution payout and storage support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant EntryFeeStore
participant Distribution
participant calculate_payout
participant PrizeStoreTrait
EntryFeeStore->>Distribution: reconstruct geometric or tiered configuration
Distribution->>calculate_payout: provide distribution and payout parameters
calculate_payout->>calculate_payout: validate and normalize integer weights
calculate_payout-->>Distribution: return exact token-unit payout
PrizeStoreTrait-->>Distribution: return indexed custom share when requested
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Codex Review - General Engineering ReviewReview process failed to complete. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Codex Review - Cairo/Starknet Contract ReviewReview process failed to complete. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/utilities/src/distribution/tests/test_share_regression.cairo (1)
70-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd function-level documentation consistently. Section headings do not document each test’s specific purpose and expected behavior.
packages/utilities/src/distribution/tests/test_share_regression.cairo#L70-L110: document both linear exact-share fixtures.packages/utilities/src/distribution/tests/test_gas_benchmark.cairo#L34-L147: document each benchmark’s measured shape and rationale.As per coding guidelines, “Every function must include clear explanation of what it does and why.”
🤖 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 `@packages/utilities/src/distribution/tests/test_share_regression.cairo` around lines 70 - 110, Add function-level documentation to both exact-share tests, test_linear_w10_n5_exact_shares and test_linear_w25_n4_exact_shares, describing the linear distribution fixture, expected shares, total, and dust behavior. Also document every benchmark function in packages/utilities/src/distribution/tests/test_gas_benchmark.cairo (lines 34-147), explaining its measured workload shape and rationale; do not rely on section headings as substitutes for per-function documentation.Source: Coding guidelines
packages/utilities/src/distribution/calculator.cairo (2)
156-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfusing variable name:
denominator_fpshadows the concept ofdenominator.In the
Exponentialbranch,denominator_fpactually holdsn(the fixed-point total payout count used as the division base for each position's ratio), which is conceptually unrelated to the outerdenominatoraccumulator (the weight-sum this function returns). Reusing the word "denominator" for two different values in the same function is confusing for future maintainers touching this formula.♻️ Suggested rename
let weight_fp = FixedTrait::new((weight.into() * ONE) / 10, false); let n_u64: u64 = n.into(); - let denominator_fp = FixedTrait::new_unscaled(n_u64, false); + let n_fp = FixedTrait::new_unscaled(n_u64, false); let mut p: u32 = 1; loop { if p > n { break; } let pi: u64 = (p - 1).into(); let num_fp = FixedTrait::new_unscaled(n_u64 - pi, false); - let base_fp = num_fp / denominator_fp; + let base_fp = num_fp / n_fp; let w = base_fp.pow(weight_fp); denominator = denominator + w; weights.append(w); p += 1; }As per path instructions, "Use descriptive variable names (e.g.,
liquidity_unlock_timestampnott)."🤖 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 `@packages/utilities/src/distribution/calculator.cairo` around lines 156 - 209, Rename the Exponential branch’s denominator_fp variable to a descriptive name representing the fixed-point total payout count used as the ratio’s division base, and update its use in the base_fp calculation; leave the outer denominator accumulator unchanged.Source: Path instructions
156-243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd structured
# Arguments/# Returnsdocs to the new private helpers.
weight_vectorhas a substantial explanatory comment but no formal parameter/return documentation;share_atandsum_sharesonly have one-line descriptions. Every other function in this file (calculate_share,calculate_total,calculate_share_with_dust) documents arguments and return values explicitly.As per path instructions, "Every function must include clear explanation of what it does and why, parameter descriptions with types and constraints, return value documentation, and example usage when appropriate."
🤖 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 `@packages/utilities/src/distribution/calculator.cairo` around lines 156 - 243, Add complete documentation comments to the private helpers weight_vector, share_at, and sum_shares, including # Arguments entries with types and constraints and a # Returns entry describing each returned value and relevant edge-case behavior. Preserve their existing explanatory descriptions and document the 1-indexed payout_index, zero/out-of-range handling, normalization denominator, and truncation behavior.Source: Path instructions
🤖 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 `@packages/utilities/src/distribution/tests/test_gas_benchmark.cairo`:
- Around line 34-39: Make bench_baseline_overhead a harness-only baseline by
removing the Distribution::Uniform setup, calculate_share invocation, and
assertion. Leave the test body empty or otherwise trivial without performing any
distribution-related work.
In `@packages/utilities/src/distribution/tests/test_share_regression.cairo`:
- Around line 27-153: The regression suite only validates full BASIS_POINTS
availability and lacks coverage for weighted boundary inputs. Extend the tests
around calculate_share, calculate_total, and calculate_share_with_dust with
locked fixtures using partial available_share values, then add fuzz/property
coverage for valid weights and payout counts, including zero-place behavior and
exact dust closure to the requested available share.
---
Nitpick comments:
In `@packages/utilities/src/distribution/calculator.cairo`:
- Around line 156-209: Rename the Exponential branch’s denominator_fp variable
to a descriptive name representing the fixed-point total payout count used as
the ratio’s division base, and update its use in the base_fp calculation; leave
the outer denominator accumulator unchanged.
- Around line 156-243: Add complete documentation comments to the private
helpers weight_vector, share_at, and sum_shares, including # Arguments entries
with types and constraints and a # Returns entry describing each returned value
and relevant edge-case behavior. Preserve their existing explanatory
descriptions and document the 1-indexed payout_index, zero/out-of-range
handling, normalization denominator, and truncation behavior.
In `@packages/utilities/src/distribution/tests/test_share_regression.cairo`:
- Around line 70-110: Add function-level documentation to both exact-share
tests, test_linear_w10_n5_exact_shares and test_linear_w25_n4_exact_shares,
describing the linear distribution fixture, expected shares, total, and dust
behavior. Also document every benchmark function in
packages/utilities/src/distribution/tests/test_gas_benchmark.cairo (lines
34-147), explaining its measured workload shape and rationale; do not rely on
section headings as substitutes for per-function documentation.
🪄 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 Plus
Run ID: 314dbdd5-8826-4b4d-bd91-855c3daa59f6
📒 Files selected for processing (5)
packages/utilities/src/distribution.cairopackages/utilities/src/distribution/calculator.cairopackages/utilities/src/distribution/tests.cairopackages/utilities/src/distribution/tests/test_gas_benchmark.cairopackages/utilities/src/distribution/tests/test_share_regression.cairo
| #[test] | ||
| fn bench_baseline_overhead() { | ||
| let dist = Distribution::Uniform; | ||
| let share = calculate_share(dist, 1, 1, BASIS_POINTS); | ||
| assert!(share == BASIS_POINTS, "single uniform payout takes everything"); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Make the baseline harness-only.
bench_baseline_overhead calls calculate_share, so subtracting it removes Uniform-distribution work as well as test-harness overhead. Use an empty or trivial test body with no distribution call.
🤖 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 `@packages/utilities/src/distribution/tests/test_gas_benchmark.cairo` around
lines 34 - 39, Make bench_baseline_overhead a harness-only baseline by removing
the Distribution::Uniform setup, calculate_share invocation, and assertion.
Leave the test body empty or otherwise trivial without performing any
distribution-related work.
| fn test_exponential_w10_n10_exact_shares() { | ||
| let dist = Distribution::Exponential(10); | ||
| let expected = array![1818_u16, 1636, 1454, 1272, 1090, 909, 727, 545, 363, 181]; | ||
|
|
||
| let mut p: u32 = 1; | ||
| for e in expected { | ||
| assert!( | ||
| calculate_share(dist, p, 10, BASIS_POINTS) == e, | ||
| "exp w10 n10 position {} share changed", | ||
| p, | ||
| ); | ||
| p += 1; | ||
| } | ||
|
|
||
| // Truncation leaves 5 bps unallocated; the winner absorbs it. | ||
| assert!(calculate_total(dist, 10, BASIS_POINTS) == 9995, "exp w10 n10 total"); | ||
| assert!( | ||
| calculate_share_with_dust(dist, 1, 10, BASIS_POINTS) == 1823, "exp w10 n10 winner + dust", | ||
| ); | ||
| } | ||
|
|
||
| /// Fractional weight — exercises the `exp(y * ln(x))` branch of `pow`. | ||
| #[test] | ||
| fn test_exponential_w15_n5_exact_shares() { | ||
| let dist = Distribution::Exponential(15); | ||
| let expected = array![3964_u16, 2836, 1842, 1002, 354]; | ||
|
|
||
| let mut p: u32 = 1; | ||
| for e in expected { | ||
| assert!( | ||
| calculate_share(dist, p, 5, BASIS_POINTS) == e, | ||
| "exp w15 n5 position {} share changed", | ||
| p, | ||
| ); | ||
| p += 1; | ||
| } | ||
|
|
||
| assert!(calculate_total(dist, 5, BASIS_POINTS) == 9998, "exp w15 n5 total"); | ||
| assert!( | ||
| calculate_share_with_dust(dist, 1, 5, BASIS_POINTS) == 3966, "exp w15 n5 winner + dust", | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_linear_w10_n5_exact_shares() { | ||
| let dist = Distribution::Linear(10); | ||
| let expected = array![3333_u16, 2666, 1999, 1333, 666]; | ||
|
|
||
| let mut p: u32 = 1; | ||
| for e in expected { | ||
| assert!( | ||
| calculate_share(dist, p, 5, BASIS_POINTS) == e, | ||
| "linear w10 n5 position {} share changed", | ||
| p, | ||
| ); | ||
| p += 1; | ||
| } | ||
|
|
||
| assert!(calculate_total(dist, 5, BASIS_POINTS) == 9997, "linear w10 n5 total"); | ||
| assert!( | ||
| calculate_share_with_dust(dist, 1, 5, BASIS_POINTS) == 3336, "linear w10 n5 winner + dust", | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_linear_w25_n4_exact_shares() { | ||
| let dist = Distribution::Linear(25); | ||
| let expected = array![4473_u16, 3157, 1842, 526]; | ||
|
|
||
| let mut p: u32 = 1; | ||
| for e in expected { | ||
| assert!( | ||
| calculate_share(dist, p, 4, BASIS_POINTS) == e, | ||
| "linear w25 n4 position {} share changed", | ||
| p, | ||
| ); | ||
| p += 1; | ||
| } | ||
|
|
||
| assert!(calculate_total(dist, 4, BASIS_POINTS) == 9998, "linear w25 n4 total"); | ||
| assert!( | ||
| calculate_share_with_dust(dist, 1, 4, BASIS_POINTS) == 4475, "linear w25 n4 winner + dust", | ||
| ); | ||
| } | ||
|
|
||
| /// Dust is only ever added to payout index 1, and always closes the gap to | ||
| /// `available_share` exactly. | ||
| #[test] | ||
| fn test_dust_closes_the_gap_exactly() { | ||
| let dists = array![ | ||
| Distribution::Linear(10), Distribution::Linear(25), Distribution::Exponential(10), | ||
| Distribution::Exponential(15), | ||
| ]; | ||
|
|
||
| for dist in dists { | ||
| let mut n: u32 = 1; | ||
| while n <= 8 { | ||
| let mut paid: u16 = calculate_share_with_dust(dist, 1, n, BASIS_POINTS); | ||
| let mut p: u32 = 2; | ||
| while p <= n { | ||
| paid += calculate_share_with_dust(dist, p, n, BASIS_POINTS); | ||
| p += 1; | ||
| } | ||
| assert!(paid == BASIS_POINTS, "shares + dust must total 100% at n={}", n); | ||
| n += 1; | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| /// A zero-position distribution hands the whole share to payout index 1 as | ||
| /// dust. Preserved deliberately from the pre-hoist implementation — callers | ||
| /// (Budokan's `_claim_distributed_prize`) can reach this with an empty | ||
| /// leaderboard and no configured `distribution_count`. | ||
| #[test] | ||
| fn test_zero_payouts_gives_everything_to_index_one_as_dust() { | ||
| let dists = array![Distribution::Linear(10), Distribution::Exponential(10)]; | ||
| for dist in dists { | ||
| assert!( | ||
| calculate_share(dist, 1, 0, BASIS_POINTS) == 0, "no share when there are no places", | ||
| ); | ||
| assert!(calculate_total(dist, 0, BASIS_POINTS) == 0, "no total when there are no places"); | ||
| assert!( | ||
| calculate_share_with_dust(dist, 1, 0, BASIS_POINTS) == BASIS_POINTS, | ||
| "index 1 absorbs the full share as dust", | ||
| ); | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files matching target:"
fd -a 'test_share_regression\.cairo|distribution' . | sed 's#^\./##' | head -100
echo
echo "Target file size and content outline:"
if [ -f packages/utilities/src/distribution/tests/test_share_regression.cairo ]; then
wc -l packages/utilities/src/distribution/tests/test_share_regression.cairo
ast-grep outline packages/utilities/src/distribution/tests/test_share_regression.cairo --view compact || true
echo
echo "Relevant file section:"
cat -n packages/utilities/src/distribution/tests/test_share_regression.cairo | sed -n '1,220p'
else
echo "target file not found"
fi
echo
echo "Search for partial available_share tests/fixtures/dust/coverage:"
rg -n --hidden "available_share|BASIS_POINTS|fuzz|test_fuzz|fn fuzz|partial|coverage|coverage|dust|weighted|calculate_share_with_dust|test_dust|calculate_total|Exponential|Linear" packages/utilities -S | sed -n '1,220p'Repository: Provable-Games/game-components
Length of output: 35499
Cover partial availability and fuzz weighted boundaries.
The regression title documents full/partial available_share, but every assertion in this suite still uses BASIS_POINTS, so partial-share regressions remain unprotected. Add locked partial-share fixtures and fuzz valid weighted inputs, including zero-place behavior and dust closure.
🤖 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 `@packages/utilities/src/distribution/tests/test_share_regression.cairo` around
lines 27 - 153, The regression suite only validates full BASIS_POINTS
availability and lacks coverage for weighted boundary inputs. Extend the tests
around calculate_share, calculate_total, and calculate_share_with_dust with
locked fixtures using partial available_share values, then add fuzz/property
coverage for valid weights and payout counts, including zero-place behavior and
exact dust closure to the requested available share.
Source: Coding guidelines
…iling
The hoist trades `pow` calls for an `Array<Fixed>` the per-position
implementation never allocated. Paths that were not quadratic get that
allocation for nothing, so measure where it stops paying instead of
assuming it always does.
Adds benchmarks at n = 1/2/3 (where n^2 and n are the same order), at
n = 100/200/1000 for non-winner positions (allocation against no saving),
and at n = 500/1000 for winners, plus a Custom n=1000 comparison point.
Measured, l2_gas:
Winner (pos 1) before after ratio
n=1 486,780 294,980 1.7x
n=2 952,540 450,150 2.1x
n=3 1,581,000 605,320 2.6x
n=500 20,448,538,720 77,724,810 263x
n=1000 did not complete 155,309,810 -
Non-winner (pos n) before after delta
n=3 407,990 373,370 -8.5%
n=10 exp 977,440 951,220 -2.7%
n=10 linear 786,580 686,620 -12.7%
n=50 4,231,440 4,253,220 +0.5%
n=100 8,298,940 8,380,720 +1.0%
n=200 exp 16,433,940 16,635,720 +1.2%
n=1000 exp 81,513,940 82,675,720 +1.4%
So the allocation is never the dominant term. The winner path wins at
every size including n=1. Exponential non-winners cross over around
n=25 and pay at most +1.4% at n=1000 — set against 6x-263x on the path
that actually blocked claims. Linear non-winners stay ahead throughout
(the old code computed its weight n+1 times, the vector computes it n).
Also documents a precision ceiling found while benchmarking, which is a
property of the u16 basis-point representation and predates this change
(values are bit-identical before and after): a position whose normalized
weight falls below 1/10000 of the pool truncates to a 0 bps share, and
Budokan asserts `prize_amount > 0`, so that position can never be
claimed. Last place hits zero at n=141 for both Linear(10) and
Exponential(10), and at n=20 for the steeper Exponential(25). The
n200/n1000 tail benchmarks assert bounds rather than non-zero for this
reason.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Allocation crossover — measured, not assumedFlagged in review that the new Winner path (payout index 1) — wins at every size
No small- Non-winner path (payout index n) — where allocation is pure overhead
Exponential non-winners cross over around n=25 and top out at +1.4% at n=1000. That is the entire downside, set against 6×–263× on the path that was actually making claims expensive. Linear non-winners stay ahead at every size, because the old code computed each weight n+1 times where the vector computes it n. Scaling to 1000+ placesCost is now linear at ~155k l2_gas per position (n=500 → 77.7M, n=1000 → 155.3M, exactly 2×). The old implementation could not compute a 1000-place winner's share at all within 4e9 steps. But gas is not the binding constraint at that size. Shares are u16 basis points, so a position whose normalized weight falls below 1/10000 of the pool truncates to 0 — and Budokan asserts
This predates this PR — values are bit-identical before and after — but it means a weighted prize with more than ~140 places (or ~20 for a steep exponential) already has unclaimable tail positions today. Worth a validation at |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Codex Review - General Engineering ReviewReview process failed to complete. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Codex Review - Cairo/Starknet Contract ReviewReview process failed to complete. |
100 places is a shape we want to support, so pin what it costs and what
it costs with the curve precomputed off-chain instead.
l2_gas, 100 places Exponential w10 Custom
winner (pos 1) 15,656,810 4,740,820
other (pos n) 8,380,720 1,097,550
Every position pays O(n) because each claim rebuilds the weight vector,
so paying out all 100 costs ~845M l2_gas of share math (15.7M + 99 x
8.4M) versus ~113M for Custom — before any transfer or storage cost.
Custom needs 7 packed felt slots for 100 shares and does no `pow` at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Codex Review - General Engineering ReviewReview process failed to complete. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Codex Review - Cairo/Starknet Contract ReviewReview process failed to complete. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/utilities/src/distribution/tests/test_gas_benchmark.cairo (1)
130-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument each added benchmark function.
The group headings explain categories, but the individual benchmark functions lack the required explanation of their specific shape and purpose. Add concise Cairo doc comments where the function name alone is insufficient.
As per coding guidelines, “Every function must include clear explanation of what it does and why.”
Also applies to: 272-303
🤖 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 `@packages/utilities/src/distribution/tests/test_gas_benchmark.cairo` around lines 130 - 225, The added benchmark functions in the distribution benchmark suite lack function-level documentation. Add concise Cairo doc comments to each benchmark, including the exponential and linear tail cases, the large-count ceiling cases, and the custom-distribution comparison, describing the tested shape and its purpose; retain the existing group headings and assertions.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 `@packages/utilities/src/distribution/tests/test_gas_benchmark.cairo`:
- Around line 217-223: Separate fixture construction from the metered
distribution calculation in the benchmark tests. At
packages/utilities/src/distribution/tests/test_gas_benchmark.cairo lines
217-223, isolate or subtract the 1,000-entry Custom-share setup; apply the same
treatment to the 100-entry winner fixture at lines 281-287 and the 100-entry
tail fixture at lines 294-300. Alternatively, explicitly label these benchmarks
as measuring construction plus calculation rather than the claimed
calculation-only path.
---
Nitpick comments:
In `@packages/utilities/src/distribution/tests/test_gas_benchmark.cairo`:
- Around line 130-225: The added benchmark functions in the distribution
benchmark suite lack function-level documentation. Add concise Cairo doc
comments to each benchmark, including the exponential and linear tail cases, the
large-count ceiling cases, and the custom-distribution comparison, describing
the tested shape and its purpose; retain the existing group headings and
assertions.
🪄 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 Plus
Run ID: d6fc96ab-2b79-422f-aa2c-36c0eb967c9e
📒 Files selected for processing (1)
packages/utilities/src/distribution/tests/test_gas_benchmark.cairo
| let mut shares: Array<u16> = array![]; | ||
| let mut i: u32 = 0; | ||
| while i < 1000 { | ||
| shares.append(10); | ||
| i += 1; | ||
| } | ||
| let dist = Distribution::Custom(shares.span()); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Separate Custom input construction from calculation gas.
These tests build the entire Array<u16> inside the metered body, so their reported cost includes O(n) fixture allocation rather than just the claimed O(1) Custom share path. Measure/subtract equivalent setup cost, or label these as end-to-end construction-plus-calculation benchmarks.
packages/utilities/src/distribution/tests/test_gas_benchmark.cairo#L217-L223: isolate the 1,000-entry Custom-share construction cost.packages/utilities/src/distribution/tests/test_gas_benchmark.cairo#L281-L287: isolate the 100-entry winner fixture construction cost.packages/utilities/src/distribution/tests/test_gas_benchmark.cairo#L294-L300: isolate the 100-entry tail fixture construction cost.
📍 Affects 1 file
packages/utilities/src/distribution/tests/test_gas_benchmark.cairo#L217-L223(this comment)packages/utilities/src/distribution/tests/test_gas_benchmark.cairo#L281-L287packages/utilities/src/distribution/tests/test_gas_benchmark.cairo#L294-L300
🤖 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 `@packages/utilities/src/distribution/tests/test_gas_benchmark.cairo` around
lines 217 - 223, Separate fixture construction from the metered distribution
calculation in the benchmark tests. At
packages/utilities/src/distribution/tests/test_gas_benchmark.cairo lines
217-223, isolate or subtract the 1,000-entry Custom-share setup; apply the same
treatment to the 100-entry winner fixture at lines 281-287 and the 100-entry
tail fixture at lines 294-300. Alternatively, explicitly label these benchmarks
as measuring construction plus calculation rather than the claimed
calculation-only path.
Adds `distribution::payout`, computing a position's payout directly:
payout(p) = total_amount * W(p) / sum(W)
in u256 integer arithmetic, where W is an exact integer weight and sum(W)
is closed form. The basis-point path in `calculator` stays as-is for
prizes already escrowed against it.
Three problems go away at once.
**Cost stops depending on the size of the field.** No loop, no allocation,
no Cubit `pow`. Measured l2_gas, winner's payout:
places calculator payout
10 1,691,510 228,550
100 15,656,810 228,550
1000 155,309,810 228,550
Identical at every size, and identical for the last position as for the
first (k=3 costs 367,150; Linear 273,630 — the shape of the weight, not
the size of the field). Distributing 100 places drops from ~845M l2_gas
of share math to ~23M.
**Positions stop being unclaimable.** A u16 basis point is 1/10000 of the
pool, so a steep curve over a large field gives late positions exactly 0
— and Budokan asserts `prize_amount > 0`, so those players cannot claim
at all. Measured: Exponential(2.5) loses the tail from 20 places,
Linear(1.0) from 141. In token units the floor is 1 wei;
`test_no_zero_payouts_where_basis_points_die` pins a 100-place k=3 curve
that starves under basis points and pays everyone here.
**Dust stops being load-bearing.** Basis-point truncation strands up to
n/10000 of the pool — 0.5% of a 100-place prize, silently redirected to
first place. Here the remainder is under n wei, so there is nothing worth
redistributing, and payout index 1 no longer has to sum every position to
find it. That is what makes the winner O(1) like everyone else.
Weights (any common scale cancels, so they stay small integers):
Linear(w) W(p) = 10 + (n-p)*w sum = 10n + w*n(n-1)/2
Exponential(w) W(p) = (n-p+1)^k sum = Faulhaber(k, n)
Uniform W(p) = 1 sum = n
Custom(shares) W(p) = shares[p-1] sum = sum(shares)
`Exponential` is a power law rather than a true exponential — W(p) is
polynomial in the position. That is what makes this possible: a power sum
has an exact integer closed form, where geometric decay would need r^n in
rationals and overflows u256 past a few dozen places.
Fractional exponents (1.5, 2.5) have no closed-form power sum.
`supports_exact_payout` reports them, and `calculate_payout` panics rather
than quietly substituting a different curve; those distributions stay on
the basis-point path. Integer exponents are capped at 5 — a k=3 curve
over 100 places already pays first place a million times last.
Curve values are unchanged: `test_matches_the_basis_point_curve` shows a
10000-unit pool reproduces `calculate_share` to within the 1 bps the
fixed-point implementation itself loses to rounding.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Codex Review - General Engineering ReviewReview process failed to complete. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Codex Review - Cairo/Starknet Contract ReviewReview process failed to complete. |
starknetdev
changed the title
Brings the branch up to date with v1.1.11 and, more usefully, onto the org-shared AI review workflow adopted in #122 — this branch predates it and was still running the retired hand-rolled review jobs.
GPT Code ReviewReview execution failed (exit code 1). The Codex CLI log was not posted because it can contain the full review prompt. |
Claude Code ReviewI have enough to write the review. Let me confirm the [HIGH] packages/metagame/src/entry_fee/entry_fee_store.cairo:252 - Distribution::Geometric((a, b)) => {
assert!(*a <= 255 && *b <= 255, "Distribution: geometric ratio terms must fit one byte");
(DIST_TYPE_GEOMETRIC, *a * 256 + *b, 0_u16, 0_u16)
},and likewise for [HIGH] packages/metagame/src/prize/structs.cairo:148 - Same unvalidated [LOW] packages/metagame/src/prize/structs.cairo:47 - The Note on the geometric overflow envelope: |
Addresses the [MEDIUM] finding from the Claude review on #120. `payout_weight` yields 0 for a position past `total_payouts`, but `payout_weight_sum` summed the entire shares array. The two parameters are independent — a caller may pay fewer places than the stored curve describes — so whenever `total_payouts < shares.len()` the denominator carried weight for positions that never get paid. Every payout came out proportionally short and the difference was stranded: the truncated tail's full weight, not the sub-unit rounding the module documents. With shares [5000, 3000, 2000] paying 2 places from a 1000 pool, that was 500 + 300 = 800 paid of 1000, with 200 stuck. Now 625 + 375 = 1000. Not reachable from Budokan, which validates `shares.len() == distribution_count` and passes that same count as `total_payouts` — but this is a library function and the invariant belongs here, not in the caller. Also documents the `Exponential` overflow envelope on `calculate_payout` ([LOW] from the same review). The largest intermediate is `total_amount * n^k`; against a 10^27 pool the ceiling is k=18 at 100 places, k=15 at 1,000 and k=13 at 10,000. `MAX_EXACT_EXPONENT` is 5, so every accepted curve is far inside it — the bound only matters if that cap is ever raised. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/utilities/src/distribution/payout.cairo (1)
99-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDefine the error messages as constants.
The panic message at line 99 and the assertion message at line 195 are inline literals. The coding guidelines require descriptive error constants. Extract the static text into module-level constants and keep only the formatted value inline.
As per coding guidelines: "All error messages must be implemented as descriptive constants".
Also applies to: 193-196
🤖 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 `@packages/utilities/src/distribution/payout.cairo` at line 99, Extract the static panic and assertion message text in the payout logic into descriptive module-level constants, then update the panic near the exponent range check and the assertion around the later validation to reference those constants while keeping only the formatted value inline.Source: Coding guidelines
packages/utilities/src/distribution/tests/test_payout.cairo (1)
37-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd fuzz coverage for payout conservation.
test_payouts_never_exceed_the_poolonly exercises fixedDistributions andn <= 40. Since edge cases must be fuzzed, add a#[test] #[fuzzer]function parameterized by fuzzable parameters, then assertspaid <= POOLandPOOL - paid < total_payouts.into().🤖 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 `@packages/utilities/src/distribution/tests/test_payout.cairo` around lines 37 - 52, Add a fuzz-enabled test alongside test_payouts_never_exceed_the_pool, parameterized with fuzzable Distribution and payout-count inputs, and call sum_payouts with the fuzzed values and POOL. Assert both conservation conditions: paid <= POOL and POOL - paid < total_payouts.into(), using the fuzzed payout count rather than the fixed n loop.Source: Coding guidelines
packages/utilities/src/distribution/tests/test_gas_benchmark.cairo (3)
317-321: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd function-level documentation to the new payout benchmarks.
The new
bench_payout_*functions have no function-level explanation. Document each benchmark’s purpose. State that these tests take no parameters and return no value.As per coding guidelines: “Every function must include clear explanation of what it does and why, parameter descriptions with types and constraints, return value documentation, and example usage when appropriate.”
Also applies to: 323-327, 329-333, 335-339, 341-345, 347-351, 353-357
🤖 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 `@packages/utilities/src/distribution/tests/test_gas_benchmark.cairo` around lines 317 - 321, Add function-level documentation to each new bench_payout_* benchmark, explaining its purpose and why it exercises the corresponding payout scenario. Document that these functions take no parameters and return no value, including relevant constraints or example usage where appropriate, without changing benchmark behavior.Source: Coding guidelines
318-320: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a descriptive name for the payout result.
Each new benchmark stores the token-unit result in
p. Rename it topayout_amountso its unit and meaning are clear.As per coding guidelines: “Use descriptive variable names (e.g., liquidity_unlock_timestamp not t).”
Also applies to: 324-326, 330-332, 336-338, 342-344, 348-350, 354-356
🤖 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 `@packages/utilities/src/distribution/tests/test_gas_benchmark.cairo` around lines 318 - 320, Rename the payout result variable from p to payout_amount in bench_payout_exp_k1_n10_pos1 and all additionally referenced benchmark blocks, updating each assertion to use the descriptive name while preserving the existing calculations and checks.Source: Coding guidelines
5-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the module statement about value assertions.
Lines 5-8 say that these tests assert nothing about values. The file asserts
share > 0,share <= BASIS_POINTS, andp > 0. State that the tests use coarse sanity assertions and that the correctness suite owns exact value checks.🤖 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 `@packages/utilities/src/distribution/tests/test_gas_benchmark.cairo` around lines 5 - 8, Update the module-level comment in the gas benchmark tests to state that they use coarse sanity assertions, including basic bounds or positivity checks, while the correctness suite in calculator.cairo owns exact value validation. Keep the existing explanation of benchmark purpose and compared gas figures unchanged.
🤖 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 `@packages/utilities/src/distribution/payout.cairo`:
- Around line 115-128: Validate the distribution at the start of both public
helpers, payout_weight and payout_weight_sum, by applying supports_exact_payout
before performing weight conversion or exponentiation. Reject unsupported
exponential weights such as 0 and 15 rather than silently truncating or entering
the power-sum panic path, while preserving the existing calculations for
supported distributions.
- Around line 164-174: Update the Custom branch of payout_weight_sum to sum only
the first total_payouts entries of shares, preventing weights for unpaid
positions from entering the denominator. Review calculate_payout’s handling of
mismatched shares.len() and total_payouts, asserting equality if mismatches are
invalid, and extend test_custom_uses_its_explicit_shares with cases where
total_payouts is smaller and larger than shares.len().
---
Nitpick comments:
In `@packages/utilities/src/distribution/payout.cairo`:
- Line 99: Extract the static panic and assertion message text in the payout
logic into descriptive module-level constants, then update the panic near the
exponent range check and the assertion around the later validation to reference
those constants while keeping only the formatted value inline.
In `@packages/utilities/src/distribution/tests/test_gas_benchmark.cairo`:
- Around line 317-321: Add function-level documentation to each new
bench_payout_* benchmark, explaining its purpose and why it exercises the
corresponding payout scenario. Document that these functions take no parameters
and return no value, including relevant constraints or example usage where
appropriate, without changing benchmark behavior.
- Around line 318-320: Rename the payout result variable from p to payout_amount
in bench_payout_exp_k1_n10_pos1 and all additionally referenced benchmark
blocks, updating each assertion to use the descriptive name while preserving the
existing calculations and checks.
- Around line 5-8: Update the module-level comment in the gas benchmark tests to
state that they use coarse sanity assertions, including basic bounds or
positivity checks, while the correctness suite in calculator.cairo owns exact
value validation. Keep the existing explanation of benchmark purpose and
compared gas figures unchanged.
In `@packages/utilities/src/distribution/tests/test_payout.cairo`:
- Around line 37-52: Add a fuzz-enabled test alongside
test_payouts_never_exceed_the_pool, parameterized with fuzzable Distribution and
payout-count inputs, and call sum_payouts with the fuzzed values and POOL.
Assert both conservation conditions: paid <= POOL and POOL - paid <
total_payouts.into(), using the fuzzed payout count rather than the fixed n
loop.
🪄 Autofix
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 Plus
Run ID: 153939c8-4a20-4c61-ba41-54d7f6c91ae6
📒 Files selected for processing (5)
packages/utilities/src/distribution.cairopackages/utilities/src/distribution/payout.cairopackages/utilities/src/distribution/tests.cairopackages/utilities/src/distribution/tests/test_gas_benchmark.cairopackages/utilities/src/distribution/tests/test_payout.cairo
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/utilities/src/distribution/tests.cairo
Addresses the [LOW] finding from the Claude review on #120. `payout_weight` and `payout_weight_sum` were `pub` but do no `supports_exact_payout` gating, and `Exponential` derives `k = weight / 10` by integer truncation. An external caller passing `Exponential(15)` would have silently received the k=1 curve instead of the k=1.5 one — a different distribution, no panic. `calculate_payout` guards correctly, so making it the only public entry point closes the hole rather than duplicating the assert. Tests live in the same crate and are unaffected. Also corrects the module doc table, which still described `Custom`'s denominator as the whole shares array after the previous commit bounded it by the paid-place count. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…field size
Prototype for review, not a commitment. `Exponential` is a power law, so its
winner share thins as roughly (k+1)/n: 45% over 10 places, 5.8% over 100, and
no weight fixes that. A headline first prize on a large field needs geometric
decay, which the current set cannot express at any setting.
`Geometric(a, b)` gives each place `b / a` of the one above:
W(p) = a^(n-p) * b^(p-1) sum = (a^n - b^n) / (a - b)
Exact in integers — no fixed point — and the winner takes about `1 - b/a`
regardless of n:
ratio (10,7), first place: 30.9% at n=10, 30.0% at n=39
Exponential k=5, first place: 45.3% at n=10, 5.8% at n=100
Three things the prototype surfaced that are worth knowing before committing
to the enum change:
1. **The packed storage only carries one u16 per distribution.** Both
`entry_fee_store` and `prize/structs` pack `(u8 tag, u16 param)`, so a
two-term ratio does not fit. Rather than widen the layout — a storage
change for every consumer — `a` and `b` are capped at 255 and packed as
`a * 256 + b`. That is still every ratio with terms under 256, far more
precision than a decay setting needs, and the layout is untouched.
2. **Reach is bounded, and tightens as the ratio gets finer.** The heaviest
weight is `a^(n-1)`; holding it under 2^128 keeps `pool * W(1)` inside u256
for any u128 pool, with no pool-size caveat. That gives 129 places at (2,1),
81 at (3,2), 46 at (7,5), 39 at (10,7). `calculate_payout` asserts it rather
than overflowing, and `max_geometric_payouts` lets a caller reject at
creation. A host wanting more places picks a coarser ratio for the same
decay.
3. **It is not as cheap as the power law.** Its exponent scales with the field
instead of being capped at MAX_EXACT_EXPONENT, so it costs O(log n) large
u256 multiplications. `int_pow` is now exponentiation by squaring, which
took a 39-place winner from 6.9M to 3.4M l2_gas — still ~15x the power
law's ~230k, flat across position. Against Custom at the same reach that is
a bargain; against Exponential it is a real cost for a shape you cannot
otherwise have.
`calculator` (the basis-point path) panics for Geometric rather than
approximating it — a geometric tail dies under basis points even worse than a
steep power law, so there is no honest bps form to offer.
The enum variant is appended, not inserted: Serde indices are positional, and
inserting earlier would reinterpret every stored and indexed distribution.
snforge: 301 passed. The 2 failures are the pre-existing local step-limit ones
on the basis-point comparison tests, which pass in CI.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 (2)
packages/utilities/src/distribution/tests/test_payout.cairo (2)
212-230: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest the zero-exponent rejection.
supports_exact_payouthas a separatew != 0guard inpackages/utilities/src/distribution/payout.cairolines 121-125. These tests cover fractional and excessive exponents, but notDistribution::Exponential(0). Add an assertion for rejection and acalculate_payoutpanic test.🤖 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 `@packages/utilities/src/distribution/tests/test_payout.cairo` around lines 212 - 230, Extend the exponent validation tests around supports_exact_payout and calculate_payout to cover Distribution::Exponential(0). Assert that supports_exact_payout rejects the zero exponent, and add a should_panic test confirming calculate_payout uses the expected rejection message.
187-200: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse an independent Faulhaber reference.
Line 197 sums
payout_weight, which is production logic. This only verifies thatpayout_weight_sumagrees withpayout_weight. A shared rank or exponent defect can pass this test. Accumulate(n - p + 1)^kwith test-local repeated multiplication before comparison.🤖 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 `@packages/utilities/src/distribution/tests/test_payout.cairo` around lines 187 - 200, Update test_power_sums_match_direct_summation to use an independent Faulhaber-style reference instead of summing payout_weight inside the test. Keep the outer loops over k and n, but build reference by computing (n - p + 1)^k with test-local repeated multiplication for each p, then compare that independent total against payout_weight_sum. Do not rely on payout_weight in this test so the check remains valid even if production ranking/exponent logic is wrong.
🧹 Nitpick comments (2)
packages/utilities/src/distribution/payout.cairo (1)
99-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete documentation for the new calculation functions.
The new documentation explains formulas, but it does not consistently document parameter types and constraints or return values. Add
# Argumentsand# Returnssections. Add examples for public APIs where useful.
packages/utilities/src/distribution/payout.cairo#L99-L135: documentmax_geometric_payoutsandsupports_exact_payout.packages/utilities/src/distribution/payout.cairo#L141-L178: documentpower_sumandint_pow.packages/utilities/src/distribution/payout.cairo#L183-L282: documentpayout_weightandpayout_weight_sum.packages/utilities/src/distribution/payout.cairo#L314-L341: completecalculate_payoutparameter and return documentation.packages/utilities/src/distribution/calculator.cairo#L148-L254: documentweight_vector,share_at, andsum_shares.As per coding guidelines, every function must include purpose, parameter types and constraints, return-value documentation, and example usage when appropriate.
🤖 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 `@packages/utilities/src/distribution/payout.cairo` around lines 99 - 135, Complete the documentation for max_geometric_payouts and supports_exact_payout in packages/utilities/src/distribution/payout.cairo lines 99-135 by adding purpose, typed parameter constraints, return-value descriptions, and useful examples; apply the same documentation requirements to power_sum and int_pow in packages/utilities/src/distribution/payout.cairo lines 141-178, payout_weight and payout_weight_sum in lines 183-282, and complete calculate_payout’s parameter and return documentation in lines 314-341. Also document weight_vector, share_at, and sum_shares in packages/utilities/src/distribution/calculator.cairo lines 148-254 with purpose, parameter types and constraints, return values, and examples where appropriate.Source: Coding guidelines
packages/utilities/src/distribution/tests/test_payout.cairo (1)
283-369: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument each new geometric test function.
The section comment does not document each test invariant or boundary. Add
///documentation to every new geometric test and benchmark function. State the tested condition, expected result, and reason for the boundary.As per coding guidelines, “Every function must include clear explanation of what it does and why.”
🤖 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 `@packages/utilities/src/distribution/tests/test_payout.cairo` around lines 283 - 369, Add /// documentation to each new geometric test and benchmark function in the shown section: test_geometric_ratio_holds_between_adjacent_positions, test_geometric_winner_share_is_independent_of_field_size, test_geometric_conserves_the_pool, test_max_geometric_payouts_matches_the_documented_reach, test_geometric_beyond_reach_is_refused_not_overflowed, test_geometric_shape_guards, bench_payout_geometric_n39_pos1, and bench_payout_geometric_n39_pos_last. Describe each function’s condition, expected outcome, and the rationale for its boundary or invariant.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 `@packages/metagame/src/prize/structs.cairo`:
- Line 27: Update test_fuzz_realistic_payout_types to treat
PAYOUT_TYPE_GEOMETRIC as valid by expanding the generated payout-type domain
from 0–4 to 0–5. Add an API-level packing/unpacking round-trip that covers
Distribution::Geometric((10, 7)), preserving the existing round-trip coverage
for other distribution variants.
In `@packages/utilities/src/distribution/structs.cairo`:
- Around line 18-21: Update the packed-layout documentation near
DIST_TYPE_GEOMETRIC to include type 4 as the geometric distribution and describe
dist_param as storing a * 256 + b for it; restrict the zero-value statement to
the remaining distribution types.
In `@packages/utilities/src/distribution/tests/test_payout.cairo`:
- Around line 304-311: Rename
test_geometric_winner_share_is_independent_of_field_size to reflect finite-field
normalization depending on field size, and update the comment above the
calculations to state that the winner share approaches 1 - b/a as n increases
rather than claiming independence. Keep the existing assertions and payout
calculations unchanged.
- Around line 283-355: Add #[fuzzer] tests alongside
test_geometric_conserves_the_pool covering valid a,b ratios accepted by
supports_exact_payout, field sizes at and around max_geometric_payouts(a),
payout indices including first, last, and boundary values, and varied pool
sizes. For every valid generated input, verify payouts do not exceed the pool
and total shortfall remains below n units, while preserving existing fixed-case
tests and confirming snforge test --coverage passes without regression.
---
Outside diff comments:
In `@packages/utilities/src/distribution/tests/test_payout.cairo`:
- Around line 212-230: Extend the exponent validation tests around
supports_exact_payout and calculate_payout to cover
Distribution::Exponential(0). Assert that supports_exact_payout rejects the zero
exponent, and add a should_panic test confirming calculate_payout uses the
expected rejection message.
- Around line 187-200: Update test_power_sums_match_direct_summation to use an
independent Faulhaber-style reference instead of summing payout_weight inside
the test. Keep the outer loops over k and n, but build reference by computing (n
- p + 1)^k with test-local repeated multiplication for each p, then compare that
independent total against payout_weight_sum. Do not rely on payout_weight in
this test so the check remains valid even if production ranking/exponent logic
is wrong.
---
Nitpick comments:
In `@packages/utilities/src/distribution/payout.cairo`:
- Around line 99-135: Complete the documentation for max_geometric_payouts and
supports_exact_payout in packages/utilities/src/distribution/payout.cairo lines
99-135 by adding purpose, typed parameter constraints, return-value
descriptions, and useful examples; apply the same documentation requirements to
power_sum and int_pow in packages/utilities/src/distribution/payout.cairo lines
141-178, payout_weight and payout_weight_sum in lines 183-282, and complete
calculate_payout’s parameter and return documentation in lines 314-341. Also
document weight_vector, share_at, and sum_shares in
packages/utilities/src/distribution/calculator.cairo lines 148-254 with purpose,
parameter types and constraints, return values, and examples where appropriate.
In `@packages/utilities/src/distribution/tests/test_payout.cairo`:
- Around line 283-369: Add /// documentation to each new geometric test and
benchmark function in the shown section:
test_geometric_ratio_holds_between_adjacent_positions,
test_geometric_winner_share_is_independent_of_field_size,
test_geometric_conserves_the_pool,
test_max_geometric_payouts_matches_the_documented_reach,
test_geometric_beyond_reach_is_refused_not_overflowed,
test_geometric_shape_guards, bench_payout_geometric_n39_pos1, and
bench_payout_geometric_n39_pos_last. Describe each function’s condition,
expected outcome, and the rationale for its boundary or invariant.
🪄 Autofix
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 Plus
Run ID: af6dece9-62c0-4b32-928a-203f334da5d7
📒 Files selected for processing (7)
packages/interfaces/src/distribution.cairopackages/metagame/src/entry_fee/entry_fee_store.cairopackages/metagame/src/prize/structs.cairopackages/utilities/src/distribution/calculator.cairopackages/utilities/src/distribution/payout.cairopackages/utilities/src/distribution/structs.cairopackages/utilities/src/distribution/tests/test_payout.cairo
| pub const PAYOUT_TYPE_EXPONENTIAL: u8 = 2; | ||
| pub const PAYOUT_TYPE_UNIFORM: u8 = 3; | ||
| pub const PAYOUT_TYPE_CUSTOM: u8 = 4; | ||
| pub const PAYOUT_TYPE_GEOMETRIC: u8 = 5; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise the new payout type in the packing tests.
test_fuzz_realistic_payout_types treats only values 0-4 as valid and uses % 5. It never exercises PAYOUT_TYPE_GEOMETRIC == 5. Include type 5 in the fuzz domain and add an API-level round-trip for Distribution::Geometric((10, 7)).
🤖 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 `@packages/metagame/src/prize/structs.cairo` at line 27, Update
test_fuzz_realistic_payout_types to treat PAYOUT_TYPE_GEOMETRIC as valid by
expanding the generated payout-type domain from 0–4 to 0–5. Add an API-level
packing/unpacking round-trip that covers Distribution::Geometric((10, 7)),
preserving the existing round-trip coverage for other distribution variants.
| fn test_geometric_ratio_holds_between_adjacent_positions() { | ||
| let dist = Distribution::Geometric((10, 7)); | ||
| let pool: u256 = 1_000_000_000_000_000_000; | ||
| let first = calculate_payout(dist, 1, 10, pool); | ||
| let second = calculate_payout(dist, 2, 10, pool); | ||
| assert!(first == 308720592627384808, "winner share, got {}", first); | ||
| assert!(second == 216104414839169366, "runner-up share, got {}", second); | ||
| // Each place takes 7/10 of the one above. Both sides are independently | ||
| // truncated from the exact ratio, so they can differ by a unit — comparing | ||
| // `second` against `first * 7 / 10` re-truncates an already-truncated | ||
| // value and is off by one here. | ||
| let expected = first * 7 / 10; | ||
| let drift = if second > expected { | ||
| second - expected | ||
| } else { | ||
| expected - second | ||
| }; | ||
| assert!(drift <= 1, "adjacent ratio is b/a to within a unit, drifted {}", drift); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_geometric_winner_share_is_independent_of_field_size() { | ||
| let dist = Distribution::Geometric((10, 7)); | ||
| let pool: u256 = 1_000_000_000_000_000_000; | ||
| // 1 - b/a = 30%, whether the field is 10 places or 39. | ||
| let small = calculate_payout(dist, 1, 10, pool); | ||
| let large = calculate_payout(dist, 1, 39, pool); | ||
| assert!(small > 308_000_000_000_000_000 && small < 309_000_000_000_000_000, "~30.9% at n=10"); | ||
| assert!(large > 299_000_000_000_000_000 && large < 301_000_000_000_000_000, "~30.0% at n=39"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_geometric_conserves_the_pool() { | ||
| let dist = Distribution::Geometric((3, 2)); | ||
| let pool: u256 = 1_000_000_000_000_000_000; | ||
| let n: u32 = 50; | ||
| let mut total: u256 = 0; | ||
| let mut p: u32 = 1; | ||
| while p <= n { | ||
| let amount = calculate_payout(dist, p, n, pool); | ||
| assert!(amount > 0, "position {} must be payable", p); | ||
| total += amount; | ||
| p += 1; | ||
| } | ||
| assert!(total <= pool, "never overpays"); | ||
| assert!(pool - total < n.into(), "shortfall under one unit per position"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_max_geometric_payouts_matches_the_documented_reach() { | ||
| assert!(max_geometric_payouts(2) == 129, "50% decay reaches 129 places"); | ||
| assert!(max_geometric_payouts(3) == 81, "2/3 decay reaches 81"); | ||
| assert!(max_geometric_payouts(7) == 46, "5/7 decay reaches 46"); | ||
| assert!(max_geometric_payouts(10) == 39, "7/10 decay reaches 39"); | ||
| } | ||
|
|
||
| #[test] | ||
| #[should_panic(expected: "geometric ratio reaches at most 39 places")] | ||
| fn test_geometric_beyond_reach_is_refused_not_overflowed() { | ||
| let dist = Distribution::Geometric((10, 7)); | ||
| calculate_payout(dist, 1, 40, 1_000_000_000_000_000_000); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_geometric_shape_guards() { | ||
| // b must be under a (otherwise the curve is flat or inverted), non-zero, | ||
| // and both must survive the single-u16 packed param slot. | ||
| assert!(supports_exact_payout(Distribution::Geometric((10, 7))), "valid ratio"); | ||
| assert!(!supports_exact_payout(Distribution::Geometric((7, 10))), "inverted"); | ||
| assert!(!supports_exact_payout(Distribution::Geometric((10, 10))), "flat"); | ||
| assert!(!supports_exact_payout(Distribution::Geometric((10, 0))), "zero tail"); | ||
| assert!(!supports_exact_payout(Distribution::Geometric((256, 7))), "exceeds the u8 slot"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the repository's existing Starknet Foundry fuzz-test conventions.
rg -n -C 3 --glob '*.cairo' '#\[.*fuzz|fuzz' packages
# Find configured coverage commands or tooling.
rg -n -C 3 --glob 'Scarb.toml' 'snforge|coverage' .Repository: Provable-Games/game-components
Length of output: 50387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Target coverage references"
rg -n --glob 'Scarb.toml' 'coverage' --glob 'test_payout.cairo' .
echo
echo "## test_payout outline/size"
wc -l packages/utilities/src/distribution/tests/test_payout.cairo 2>/dev/null || true
ast-grep outline packages/utilities/src/distribution/tests/test_payout.cairo --view compact 2>/dev/null | sed -n '1,120p' || true
echo
echo "## Distribution module relevant symbols"
rg -n --glob '*.cairo' 'fn (calculate_payout|supports_exact_payout|max_geometric_payouts)|enum Distribution|struct Distribution' packages/utilities/src/distribution packages/utilities/src 2>/dev/null | sed -n '1,220p'Repository: Provable-Games/game-components
Length of output: 198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## target file"
wc -l packages/utilities/src/distribution/tests/test_payout.cairo 2>/dev/null || true
sed -n '1,120p' packages/utilities/src/distribution/tests/test_payout.cairo 2>/dev/null || true
echo
echo "## distribution sources"
fd -a -i 'distribution|payout' packages 2>/dev/null | sed -n '1,120p'
rg -n --glob '*.cairo' 'calculate_payout|supports_exact_payout|max_geometric_payouts|Geometric' packages/utilities 2>/dev/null | sed -n '1,220p' || true
echo
echo "## Scarb files"
fd -a 'Scarb.toml' . 2>/dev/null | sed -n '1,80p'
for f in $(fd 'Scarb.toml' . | sed -n '1,20p'); do
echo "--- $f"
sed -n '1,220p' "$f"
done
echo
echo "## coverage mentions"
rg -n -i --glob '!node_modules/**' 'coverage|snforge|fuzz' packages README* .github 2>/dev/null | sed -n '1,220p' || trueRepository: Provable-Games/game-components
Length of output: 47946
Add fuzz coverage for geometric payout boundaries.
The geometric tests use only fixed (10, 7)/(3, 2) ratios and field sizes. Add #[fuzzer] properties that exercise valid (a, b) pairs bounded by supports_exact_payout, max_geometric_payouts(a) boundaries, payout index edge cases, and pool sizes. Verify non-overpayment and the n-unit shortfall bound for each valid input, then ensure snforge test --coverage records the required coverage without regression.
🤖 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 `@packages/utilities/src/distribution/tests/test_payout.cairo` around lines 283
- 355, Add #[fuzzer] tests alongside test_geometric_conserves_the_pool covering
valid a,b ratios accepted by supports_exact_payout, field sizes at and around
max_geometric_payouts(a), payout indices including first, last, and boundary
values, and varied pool sizes. For every valid generated input, verify payouts
do not exceed the pool and total shortfall remains below n units, while
preserving existing fixed-case tests and confirming snforge test --coverage
passes without regression.
Source: Coding guidelines
| fn test_geometric_winner_share_is_independent_of_field_size() { | ||
| let dist = Distribution::Geometric((10, 7)); | ||
| let pool: u256 = 1_000_000_000_000_000_000; | ||
| // 1 - b/a = 30%, whether the field is 10 places or 39. | ||
| let small = calculate_payout(dist, 1, 10, pool); | ||
| let large = calculate_payout(dist, 1, 39, pool); | ||
| assert!(small > 308_000_000_000_000_000 && small < 309_000_000_000_000_000, "~30.9% at n=10"); | ||
| assert!(large > 299_000_000_000_000_000 && large < 301_000_000_000_000_000, "~30.0% at n=39"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the finite-field geometric invariant.
The function name and Line 307 claim field-size independence. The assertions expect different winner shares for 10 and 39 places. Finite geometric normalization depends on n and approaches 1 - b/a as n grows. Rename the test and correct the comment.
🤖 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 `@packages/utilities/src/distribution/tests/test_payout.cairo` around lines 304
- 311, Rename test_geometric_winner_share_is_independent_of_field_size to
reflect finite-field normalization depending on field size, and update the
comment above the calculations to state that the winner share approaches 1 - b/a
as n increases rather than claiming independence. Keep the existing assertions
and payout calculations unchanged.
…e curve
`get_prize` eagerly called `get_custom_shares`, rebuilding the entire shares
array from packed storage as part of reconstructing the record. That runs on
every claim, and a claim settles exactly one position — so a 200-place Custom
prize paid O(count/15) storage reads plus 200 unpack steps, 200 times over,
to use one number.
The entry-fee store has never done this: `get_entry_fee` returns Custom with
an empty span and claims read a single share through `get_custom_share_at`.
This brings the prize side in line.
- `get_prize` reports the Custom *shape* with an empty span
- new `get_custom_share_at(prize_id, position)` — one storage read at any
position, mirroring the entry-fee accessor
- view surfaces call `get_custom_shares` explicitly, as before
No prefix sums or precomputed totals are needed: shares are validated at
creation to number `distribution_count` and sum to exactly BASIS_POINTS, so
the denominator is the constant BASIS_POINTS and there is nothing to sum.
(Running totals would only be needed to pay the top n of a *longer* stored
curve, which is a separate capability.)
Measured in budokan, marginal cost of one distributed-prize claim:
places before after saving
10 4,574,327 3,001,323 1.5x
50 11,294,147 3,001,323 3.8x
100 19,638,887 3,001,323 6.5x
200 36,401,747 3,001,323 12.1x
Flat, and now marginally cheaper than Exponential (3,174,237), which computes
a power where this reads a slot.
snforge: 437 passed. The round-trip test now asserts the new contract — that
`get_prize` does not rebuild the curve, and that any position reads correctly
through the accessor.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The variant that makes a very large field expressive. A single curve cannot:
anything steep enough to give first place a real share rounds its tail to
nothing, and anything flat enough to pay the tail gives first place nothing.
Over 10,000 places the best any single curve here can do for first place is
~0.06% (Exponential k=5).
`Tiered { head_ratio, head_count, head_share_bps }`: a geometric head over the
first `head_count` places takes `head_share_bps` of the pool; every remaining
place splits the rest evenly. The flagship configuration — top 39 on (10, 7)
taking 80% — over 10,000 places:
1st 240000218290681776 (24.00% of a 1e18 pool)
2nd 168000152803477243 (70% of 1st, exactly the decay)
39th 311843831108
40th..10000th 20078305391024 each (0.002%, all payable)
residue 9,955 wei — under one unit per position
Both tiers are exact integer maths, and both are O(1): the head reuses
Geometric's closed form, the tail is one division. Measured: 3.50M l2_gas for
the winner, 2.31M for a tail place, flat across the field. The geometric reach
bound applies to `head_count`, not the field, which is exactly why the head
can stay steep on a field a hundred times larger.
Guards, refused at the boundary rather than misbehaving inside it:
- ratio rules are Geometric's (a > b > 0, a <= 255 for the packed param slot)
- 0 < head_share_bps < BASIS_POINTS — at either end one tier becomes an
unclaimable zero, and the single-curve variants cover those shapes
- head_count within max_geometric_payouts(a); paid places strictly beyond the
head
Storage: `PackedDistribution` and `PackedERC20Data` gain two u16 param slots
(head_count, head_share_bps), appended above the existing bit layouts so
previously packed values unpack unchanged. 88 and 216 bits respectively, both
comfortably inside felt252.
The weight helpers panic on Tiered: it is two pools, not one weight family,
and `calculate_payout` settles it before they are reached. The basis-point
calculator refuses it like Geometric — a tail place's true share is far under
a basis point by construction, so there is no honest bps form.
snforge: utilities 309 passed (the 2 failures are the pre-existing local
step-limit pair, green in CI), metagame 438 passed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/utilities/src/distribution/structs.cairo (1)
66-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftDocument the changed function contracts.
The changed functions explain implementation details, but they do not consistently document parameter types and constraints, return values, and usage examples where useful.
packages/utilities/src/distribution/structs.cairo#L66-L86: Document packing inputs, field ranges, unpacked output, and legacy-layout compatibility.packages/metagame/src/entry_fee/entry_fee_store.cairo#L101-L124: Document packed ratio decoding, valid type tags, and the returned distribution/count contract.packages/metagame/src/entry_fee/entry_fee_store.cairo#L243-L273: Document encoding constraints and the storage fields written for each distribution.packages/metagame/src/prize/structs.cairo#L125-L230: Document ERC20 distribution serialization inputs, outputs, and Tiered field mapping.packages/utilities/src/distribution/payout.cairo#L99-L393: Document each helper and public payout API parameter, constraints, return value, and a usage example where appropriate.As per coding guidelines,
Every function must include clear explanation of what it does and why, parameter descriptions with types and constraints, return value documentation, and example usage when appropriate.🤖 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 `@packages/utilities/src/distribution/structs.cairo` around lines 66 - 86, Document the changed function contracts across packages/utilities/src/distribution/structs.cairo:66-86, packages/metagame/src/entry_fee/entry_fee_store.cairo:101-124, packages/metagame/src/entry_fee/entry_fee_store.cairo:243-273, packages/metagame/src/prize/structs.cairo:125-230, and packages/utilities/src/distribution/payout.cairo:99-393. Add Cairo documentation for each affected function’s purpose, typed parameters and constraints, return values, and appropriate usage examples; specifically cover PackedDistribution field ranges and legacy-layout compatibility, packed-ratio tags and distribution/count results, encoding and storage-field behavior, ERC20 serialization and Tiered mappings, and every payout helper and public API contract.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 `@packages/metagame/src/prize/prize_store.cairo`:
- Around line 78-90: Update packages/metagame/src/prize/prize_store.cairo:78-90
so PrizeComponentImpl::get_prize returns the fully reconstructed custom share
span through get_token_record, while claim paths use get_custom_share_at for the
indexed share. Update
packages/metagame/src/prize/tests/test_prize_store.cairo:401-411 to assert that
IPrize.get_prize exposes all stored custom shares.
- Around line 119-124: Update get_custom_share_at to validate that position is
within 1..=count before subtracting one, reject invalid positions without
performing a packed lookup, and restrict computed slot indices to supported
slots. In packages/metagame/src/prize/tests/test_prize_store.cairo:395-399, add
coverage for position zero, positions above count, empty prizes, and
slot-boundary positions.
In `@packages/metagame/src/prize/structs.cairo`:
- Around line 60-64: Update the storage-layout documentation adjacent to the
high-word construction in the prize struct encoding to describe the widened
216-bit representation with an 88-bit high word. Document param2 as occupying
bits 56–71 and param3 as occupying bits 72–87, keeping the existing arithmetic
layout unchanged.
---
Nitpick comments:
In `@packages/utilities/src/distribution/structs.cairo`:
- Around line 66-86: Document the changed function contracts across
packages/utilities/src/distribution/structs.cairo:66-86,
packages/metagame/src/entry_fee/entry_fee_store.cairo:101-124,
packages/metagame/src/entry_fee/entry_fee_store.cairo:243-273,
packages/metagame/src/prize/structs.cairo:125-230, and
packages/utilities/src/distribution/payout.cairo:99-393. Add Cairo documentation
for each affected function’s purpose, typed parameters and constraints, return
values, and appropriate usage examples; specifically cover PackedDistribution
field ranges and legacy-layout compatibility, packed-ratio tags and
distribution/count results, encoding and storage-field behavior, ERC20
serialization and Tiered mappings, and every payout helper and public API
contract.
🪄 Autofix
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 Plus
Run ID: 3a9130de-1859-49cd-b651-af39a5c43914
📒 Files selected for processing (11)
packages/interfaces/src/distribution.cairopackages/metagame/src/entry_fee/entry_fee_store.cairopackages/metagame/src/prize/prize_component.cairopackages/metagame/src/prize/prize_store.cairopackages/metagame/src/prize/structs.cairopackages/metagame/src/prize/tests/mocks/prize_mock.cairopackages/metagame/src/prize/tests/test_prize_store.cairopackages/utilities/src/distribution/calculator.cairopackages/utilities/src/distribution/payout.cairopackages/utilities/src/distribution/structs.cairopackages/utilities/src/distribution/tests/test_payout.cairo
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/utilities/src/distribution/calculator.cairo
| // Return the shape with an empty span | ||
| // rather than rebuilding the curve. | ||
| // Loading it is O(count/15) storage | ||
| // reads on a path that runs on every | ||
| // claim, and a claim needs exactly one | ||
| // share — `get_custom_share_at`. | ||
| // View surfaces call | ||
| // `get_custom_shares` explicitly. | ||
| // Mirrors the entry-fee store, which | ||
| // has always done this. | ||
| Option::Some( | ||
| game_components_utilities::distribution::structs::Distribution::Custom( | ||
| shares.span(), | ||
| array![].span(), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'get_prize\(|get_token_record\(|Distribution::Custom|_get_custom_share_at|get_custom_share_at|get_custom_shares' \
packages --glob '*.cairo'Repository: Provable-Games/game-components
Length of output: 50387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== prize interface excerpt =="
sed -n '80,105p' packages/interfaces/src/prize.cairo
echo "== prize_store getter excerpt =="
sed -n '60,115p' packages/metagame/src/prize/prize_store.cairo
sed -n '190,225p' packages/metagame/src/prize/prize_store.cairo
echo "== prize_component/prize wrapper candidates =="
fd -a 'prize.*\.cairo|prize_impl.*\.cairo|prize_store.*\.cairo' packages/metagame/src 2>/dev/null || true
rg -n --glob '*.cairo' 'get_prize|to_token_record|PrizeStoreTrait|IPrize|IPrizeImpl|impl .*Prize' packages/metagame/src packages/interfaces/src
echo "== tests excerpt around custom get_prize =="
sed -n '380,415p' packages/metagame/src/prize/tests/test_prize_store.cairo
echo "== all get_prize references outside distributions =="
rg -n --glob '*.cairo' 'get_prize\(' packages --glob '!packages/utilities/src/distribution/**'Repository: Provable-Games/game-components
Length of output: 23459
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== prize_component implementation excerpt =="
sed -n '212,275p' packages/metagame/src/prize/prize_component.cairo
sed -n '530,558p' packages/metagame/src/prize/prize_component.cairo
echo "== structs to_token_record and custom packing excerpts =="
sed -n '125,155p' packages/metagame/src/prize/structs.cairo
sed -n '198,220p' packages/metagame/src/prize/structs.cairo
echo "== external call references to IPrize/get_prize =="
rg -n --glob '*.cairo' 'IPrizeDispatcher|IPrizeDispatcherTrait|get_prize\(' packages --glob '!packages/metagame/src/prize/**'
echo "== broader references to custom distribution shape expectation =="
rg -n --glob '*.cairo' 'get_prize.*Custom|Custom.*get_prize|reread\.len|get_prize must not rebuild|get_token_record|get_custom_shares|get_custom_share_at' packages --glob '!packages/utilities/src/distribution/**'Repository: Provable-Games/game-components
Length of output: 13998
Preserve the full custom share span on IPrize.get_prize.
PrizeComponentImpl::get_prize returns the result of get_token_record, which converts every stored Distribution::Custom to Custom(array![].span()). The public IPrize view therefore exposes an incomplete PrizeRecord. Keep get_prize returning the reconstructed custom shares, and have claim paths call the indexed share helper instead.
📍 Affects 2 files
packages/metagame/src/prize/prize_store.cairo#L78-L90(this comment)packages/metagame/src/prize/tests/test_prize_store.cairo#L401-L411
🤖 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 `@packages/metagame/src/prize/prize_store.cairo` around lines 78 - 90, Update
packages/metagame/src/prize/prize_store.cairo:78-90 so
PrizeComponentImpl::get_prize returns the fully reconstructed custom share span
through get_token_record, while claim paths use get_custom_share_at for the
indexed share. Update
packages/metagame/src/prize/tests/test_prize_store.cairo:401-411 to assert that
IPrize.get_prize exposes all stored custom shares.
| fn get_custom_share_at(self: @T, prize_id: u64, position: u32) -> u16 { | ||
| let index: u32 = position - 1; | ||
| let slot_index: u8 = (index / CUSTOM_SHARES_PER_SLOT.into()).try_into().unwrap(); | ||
| let index_in_slot: u8 = (index % CUSTOM_SHARES_PER_SLOT.into()).try_into().unwrap(); | ||
| Store::get_custom_shares_packed(self, prize_id, slot_index).get_share(index_in_slot) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'get_custom_share_at|_get_custom_share_at|PrizeType::Distributed|set_prize_claimed|payout' \
packages --glob '*.cairo'Repository: Provable-Games/game-components
Length of output: 50387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
fd -a 'prize_store.cail|test_prize_store\.cairo|prize_component\.cairo|prize.*\.cairo' packages/metagame/src/prize packages/metagame/src 2>/dev/null | sed 's#^\./##'
echo
echo "prize_store outline:"
ast-grep outline packages/metagame/src/prize/prize_store.cairo --view expanded || true
echo
echo "Relevant store implementation:"
sed -n '1,190p' packages/metagame/src/prize/prize_store.cairo | cat -n
echo
echo "Relevant tests around custom share:"
sed -n '330,420p' packages/metagame/src/prize/tests/test_prize_store.cairo | cat -n
echo
echo "Claim/payout references in prize files only:"
rg -n -C 10 'set_prize_claimed|payout_prize|claim|Custom|get_custom_share_at|get_custom_shares|positions' packages/metagame/src/prize --glob '*.cairo'Repository: Provable-Games/game-components
Length of output: 50387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Packed share behavior probe:"
python3 - <<'PY'
# Mirrors the documented packed-share arithmetic in packages/metagame/src/prize/prize_store.cairo.
CUSTOM_SHARES_PER_SLOT = 15
positions = [0, 1, 15, 16, 17, 18, 384, 385]
count = 17
for position in positions:
index = position - 1
if index < 0:
print(f"position {position}: underflows u32; no direct slot conversion")
continue
slot_index_u8 = (index // CUSTOM_SHARES_PER_SLOT)
index_in_slot_u8 = (index % CUSTOM_SHARES_PER_SLOT)
readable = 0 <= slot_index_u8 <= 255 and 0 <= index_in_slot_u8 <= 14
status = "readable slot if slot exists" if readable else f"u8 conversion failure ({slot_index_u8=})"
print(f"position {position}: zindex={index}, slot={slot_index_u8}, offset={index_in_slot_u8}, status={status}, in_stored_range={0 < position <= count}")
PY
echo
echo "Error constants and custom share tests:"
rg -n -C 4 'Prize:|Custom share|get_custom_share_at|position + 1|position - 1|CUSTOM_SHARE|SHARES_PER_SLOT' packages/metagame/src/prize --glob '*.cairo'
echo
echo "Read claim resolution in prize component around custom shares:"
rg -n -C 12 'get_custom_share_at|get_payout_position|set_prize_claimed|claim|Distributed|pout' packages/metagame/src/prize/prize_component.cairo | sed -n '1,260p'Repository: Provable-Games/game-components
Length of output: 50386
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Determine claim-related code shape around get_custom_share_at callers"
rg -n 'get_custom_share_at|_get_custom_share_at|calculate_share|payout_prize|set_prize_claimed|get_claims|Claimable|claim_position|position' packages/metagame/src/prize/prize_component.cairo
echo
sed -n '1,340p' packages/metagame/src/prize/prize_component.cairo | cat -n
echo
sed -n '340,660p' packages/metagame/src/prize/prize_component.cairo | cat -n
echo
echo "Interface and claim type definitions in package interfaces"
fd -a 'prize.*\.cairo|claim.*\.cairo' packages packages/metagame 2>/dev/null | sed -n '1,200p'
rg -n 'iprize|IPRIZE|claim_prize|claims|ClaimType|position' packages --glob '*.cairo' | sed -n '1,260p'Repository: Provable-Games/game-components
Length of output: 50386
Reject out-of-range custom shares before packed lookup.
position - 1 can underflow for position = 0, and values above count read beyond the configured distribution. Validate 1 <= position <= count before subtraction, keep the packed lookup limited to supported slot indices, and add coverages for invalid custom-share positions including empty prizes and slot boundaries.
📍 Affects 2 files
packages/metagame/src/prize/prize_store.cairo#L119-L124(this comment)packages/metagame/src/prize/tests/test_prize_store.cairo#L395-L399
🤖 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 `@packages/metagame/src/prize/prize_store.cairo` around lines 119 - 124, Update
get_custom_share_at to validate that position is within 1..=count before
subtracting one, reject invalid positions without performing a packed lookup,
and restrict computed slot indices to supported slots. In
packages/metagame/src/prize/tests/test_prize_store.cairo:395-399, add coverage
for position zero, positions above count, empty prizes, and slot-boundary
positions.
Source: Coding guidelines
| let high: u128 = value.payout_type.into() | ||
| + value.param.into() * 0x100_u128 // shift 8 | ||
| + value.count.into() * 0x1000000_u128; // shift 24 | ||
| + value.count.into() * 0x1000000_u128 // shift 24 | ||
| + value.param2.into() * 0x100000000000000_u128 // shift 56 | ||
| + value.param3.into() * 0x1000000000000000000_u128; // shift 72 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the widened high-word layout documentation.
The implementation now stores 216 bits with an 88-bit high word. The comments still describe 184 bits and a 56-bit high word. Document param2 at bits 56-71 and param3 at bits 72-87. Incorrect storage-layout documentation can cause a later decoder or migration to use invalid offsets.
🤖 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 `@packages/metagame/src/prize/structs.cairo` around lines 60 - 64, Update the
storage-layout documentation adjacent to the high-word construction in the prize
struct encoding to describe the widened 216-bit representation with an 88-bit
high word. Document param2 as occupying bits 56–71 and param3 as occupying bits
72–87, keeping the existing arithmetic layout unchanged.
1 participant
Two commits, in order of importance. The second is what you'd build on; the first is what keeps existing escrowed prizes safe in the meantime.
1.
distribution::payout— exact payouts, O(1)The basis-point path computes a rational number in 32.32 fixed point via Cubit
pow/exp/ln, then truncates it into a u16. Both steps are avoidable. This computes the payout directly:in u256 integer arithmetic, where
Wis an exact integer weight andsum(W)is closed form:W(p)sum(W)Linear(w)10 + (n-p)*w10n + w*n(n-1)/2Exponential(w)(n-p+1)^ksum_{j=1..n} j^k(Faulhaber)Uniform1nCustom(shares)shares[p-1]sum(shares)Any common scale cancels in the ratio, so the weights stay small integers. Three problems go away at once.
Cost stops depending on the size of the field
l2_gasfor the winner's payout:calculator(bps)payout(exact)Identical at every size, and identical for the last position as for the first. Cost tracks the shape of the weight, not the size of the field: k=3 is 367,150, Linear 273,630. Distributing all 100 places drops from ~845M l2_gas of share math to ~23M — with no calldata and no storage, unlike precomputing the curve into
Custom.Positions stop being unclaimable
A u16 basis point is 1/10000 of the pool, so a steep curve over a large field pays late positions exactly 0 — and Budokan asserts
prize_amount > 0, so those players cannot claim at all. Measured first-dead-position:In token units the floor is 1 wei.
test_no_zero_payouts_where_basis_points_diepins a 100-place k=3 curve that starves under basis points and pays every position here, and asserts the premise so it can't silently stop testing anything.Dust stops being load-bearing
Basis-point truncation strands up to
n/10000of the pool — 0.5% of a 100-place prize, silently redirected to first place. Here the remainder is undernwei, so there is nothing worth redistributing, and payout index 1 no longer has to sum every position to find it. That is what makes the winner O(1) like everyone else, rather than the most expensive claim in the tournament.Scope and safety
calculatoris untouched by this commit; prizes already escrowed against basis-point math keep computing exactly as before. Adopting this is a per-prize decision in Budokan, not a global switch.test_matches_the_basis_point_curveshows a 10000-unit pool reproducescalculate_shareto within the 1 bps that the fixed-point implementation itself loses to rounding.supports_exact_payoutreports this andcalculate_payoutpanics rather than quietly substituting a different curve. Integer exponents are capped at 5.On the curve shape
Exponentialis a power law, not an exponential —W(p)is polynomial in the position,(n-p+1)^k, notr^p. That is precisely what makes this work: a power sum has an exact integer closed form, where geometric decay needsr^nin rationals and overflows u256 past a few dozen places. The cheapest curve to compute on-chain is the one already in use.Worth knowing when choosing
k: for a power law the winner's share is approximately(k+1)/n. Over 100 places that is ~2% at k=1 and ~6% at k=5 — the family is inherently flat at the top for large fields, and no weight fixes that. If a headline first prize matters, that wants a geometric curve or an explicitCustomarray, not a steeperk.2.
calculator— the O(n^2) basis-point pathBoth weighted distributions normalize a position's weight against the sum of every position's weight, and that sum was rebuilt inside every single share computation.
calculate_total,calculate_dustand the winner'scalculate_share_with_dusttherefore re-derived the whole vector once per position.Payout index 1 is the only index that collects dust, so winners paid it on every claim. Hoisting the weight vector takes that path from O(n^2) to O(n):
Values are bit-identical, verified by sweeping the old and new implementations against each other across every payout index of every size 1..=12 for Linear (10/25/7), Exponential (10/15/25/100) and Uniform, at full and partial
available_share, plus the zero-payouts and out-of-range edges. That matters for deployment: payouts are recomputed from stored config at claim time, so a partially-claimed tournament would over- or under-pay its remainder if the values moved.The allocation this introduces was measured rather than assumed — it costs at most +1.4% on the one path that was never quadratic (exponential non-winners at n=1000), against 6x-263x on the path that was. See the crossover comment.
One behaviour is deliberately preserved: with
total_payouts == 0, payout index 1 collects the entireavailable_shareas dust. Budokan reaches that state with an empty leaderboard and no configureddistribution_count, so changing it here would be a silent payout change. It now has a named test.Tests
test_payout— conservation (never overpay; shortfall under n units), no dead positions, monotonicity, curve equivalence, exact hand-checkable weights, every Faulhaber branch against direct summation, and the guard behaviour.test_share_regression— pins exact basis points for the legacy path. The existing suite asserts ranges, which would not catch a few-bps drift.test_gas_benchmark— the cost curves above, including the allocation crossover and the flat payout line.snforge testinpackages/utilities: 294 passed, 0 failed.scarb buildclean across the workspace,scarb fmt -wapplied.Not in this PR
calculate_payoutin_claim_distributed_prize/_claim_entry_fee_position— separate repo, and it needs a versioned prize type so escrowed prizes keep their original math.add_prizerejecting a distribution whose tail rounds to zero. Worth doing regardless as a guard for prizes that stay on the basis-point path.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests