feat(entry-fee): move distribution state to EntryFeeComponent#104
feat(entry-fee): move distribution state to EntryFeeComponent#104starknetdev merged 1 commit intomainfrom
Conversation
Makes `Distribution::Custom(Span<u16>)` first-class end-to-end by teaching the entry-fee component to persist the full distribution config (shape + paid-places count + custom shares) alongside existing fee state. Previously every metagame consumer had to roll its own storage to keep Custom shares around — Budokan did, but no one else could reuse it. Key changes: - Promote CustomShares (15 u16 per felt252) + share-packing math from metagame/prize to utilities/distribution/packed_shares so both prize and entry-fee paths can share the packing primitive. - Add PackedDistribution (dist_type + dist_param + positions) to utilities/distribution with a StorePacking impl. - Extend EntryFeeConfig in interfaces with `distribution: Option<Distribution>` and `distribution_count: u32`. set_entry_fee_config now persists the distribution (type tag + scalar params + Custom shares out-of-band), and get_entry_fee rehydrates it. - Expose _get_custom_share_at on the component for O(1) single-slot reads at claim sites that only need one share. - get_entry_fee returns Custom with an empty shares span to skip the O(N/15) reconstruction on every call; callers that need the full array (UI views) call _get_distribution_shares(id, count) explicitly. This is a breaking interface change to EntryFeeConfig — existing consumers must add `distribution: Option::None, distribution_count: 0` to their struct literals (done for all in-repo call sites in this commit). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis pull request extends the entry fee configuration system to support flexible payout distributions by introducing a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @starknetdev's task —— View job Claude Review - General Engineering Reviewrun=24828344952 attempt=1 sha=a2a98800167365a19de3ee21c0cf579f16a3ecb6 scope=general Task List
Review ResultsAfter examining the targeted diff with All 15 modified files in this PR are Cairo contract code within the The changes are exclusively:
No issues found in CI/CD configuration, documentation, tooling, or project infrastructure. Summary: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFO |
Codex Review - General Engineering ReviewReview process failed to complete. |
|
Claude finished @starknetdev's task —— View job Claude Review - Cairo/Starknet Contract Reviewrun=24828344952 attempt=1 sha=a2a98800167365a19de3ee21c0cf579f16a3ecb6 scope=packages Security & Cairo ReviewAfter reviewing this significant feature addition, I found no actionable security or code quality issues within the PR scope. The implementation demonstrates excellent engineering practices: Security Analysis ✅
Cairo Idioms ✅
Testing Quality ✅
Gas Optimization ✅
Implementation Quality ✅
Summary: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFO This is exemplary Cairo smart contract engineering. The feature adds substantial functionality while maintaining security, gas efficiency, and code quality standards. |
Codex Review - Cairo/Starknet Contract ReviewReview process failed to complete. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a position-based payout distribution system for entry fees, supporting Linear, Exponential, Uniform, and Custom distribution types. It introduces a gas-efficient storage mechanism that packs multiple 16-bit shares into single storage slots to optimize storage operations. Feedback identifies a fragile implicit assumption in distribution type handling, a potential panic when casting large share counts to 8-bit integers, and a possible underflow in 1-indexed position calculations.
| } else { | ||
| // DIST_TYPE_CUSTOM — return with empty shares span. Loading the | ||
| // full array is O(N/15) storage reads and is only needed for UI | ||
| // views; claim paths read a single share via `_get_custom_share_at` | ||
| // (O(1)). The strict-sum validator (Σshares == available_share | ||
| // at creation) means no dust needs to be tracked. Callers that | ||
| // need the full array (e.g. view functions) call | ||
| // `get_distribution_shares` explicitly, passing | ||
| // `packed_dist.positions` as the count. | ||
| (Option::Some(Distribution::Custom(array![].span())), packed_dist.positions) | ||
| }; |
There was a problem hiding this comment.
The else block here implicitly assumes that any distribution type other than LINEAR, EXPONENTIAL, or UNIFORM is a CUSTOM distribution. This is fragile and could lead to unexpected behavior if new distribution types are added or if storage data is corrupted. It is safer to explicitly check for DIST_TYPE_CUSTOM and handle an unknown type as an error or a default case.
} else if packed_dist.dist_type == DIST_TYPE_CUSTOM {
// DIST_TYPE_CUSTOM — return with empty shares span. Loading the
// full array is O(N/15) storage reads and is only needed for UI
// views; claim paths read a single share via `_get_custom_share_at`
// (O(1)). The strict-sum validator (Σshares == available_share
// at creation) means no dust needs to be tracked. Callers that
// need the full array (e.g. view functions) call
// `get_distribution_shares` explicitly, passing
// `packed_dist.positions` as the count.
(Option::Some(Distribution::Custom(array![].span())), packed_dist.positions)
} else {
(Option::None, 0_u32)
};
|
|
||
| let mut i: u32 = 0; | ||
| for share in shares { | ||
| let slot_index: u8 = (i / DISTRIBUTION_SHARES_PER_SLOT.into()).try_into().unwrap(); |
There was a problem hiding this comment.
The slot_index is cast to u8, but it is calculated from i which is u32. Since DISTRIBUTION_SHARES_PER_SLOT is 15, if the number of shares exceeds 255 * 15 = 3825, this try_into().unwrap() will panic. While 3825 shares is a large number, positions in PackedDistribution is a u32, suggesting a much larger theoretical limit. You should either enforce a maximum share count at the API level or increase the slot_index type to u32 (and update the storage map key accordingly).
| } | ||
|
|
||
| fn get_custom_share_at(self: @T, context_id: u64, position: u32) -> u16 { | ||
| let (slot_index, index_in_slot) = calculate_slot_position(position - 1); |
There was a problem hiding this comment.
If position is 0, position - 1 will underflow because position is an unsigned integer (u32). Although the documentation states that position is 1-indexed, it is safer to add an explicit assertion to prevent unexpected panics or incorrect storage reads.
fn get_custom_share_at(self: @T, context_id: u64, position: u32) -> u16 {
assert!(position > 0, "Position must be 1-indexed");
let (slot_index, index_in_slot) = calculate_slot_position(position - 1);
self.get_distribution_shares_packed(context_id, slot_index).get_share(index_in_slot)
}
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: 4
🧹 Nitpick comments (2)
packages/utilities/src/distribution/packed_shares.cairo (1)
137-153: Silent truncation of inputs longer thanSHARES_PER_SLOT.
from_arraydrops every share past index 14 without signaling to the caller. Today the only production packer (EntryFeeStoreImpl::store_distribution_shares) does its own multi-slot loop and never calls this helper with more than 15 shares, so there's no bug. But this helper ispub, and future callers reaching for "pack an arbitrary-length Custom shares array" viafrom_arraywill silently lose data. Consider asserting the precondition explicitly so misuse fails loudly.♻️ Proposed change
fn from_array(shares: Span<u16>) -> CustomShares { let mut packed = Self::new(); - let len: u32 = if shares.len() > SHARES_PER_SLOT.into() { - SHARES_PER_SLOT.into() - } else { - shares.len() - }; + assert!(shares.len() <= SHARES_PER_SLOT.into(), "shares exceeds SHARES_PER_SLOT"); + let len: u32 = shares.len(); let mut i: u32 = 0; while i < len { packed.set_share(i.try_into().unwrap(), *shares.at(i)); i += 1; } packed }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utilities/src/distribution/packed_shares.cairo` around lines 137 - 153, from_array currently silently truncates inputs longer than SHARES_PER_SLOT; add an explicit precondition check at the start of the function (referencing from_array, CustomShares, and SHARES_PER_SLOT) that fails loudly when shares.len() > SHARES_PER_SLOT (e.g., assert or return an error/panic), so callers cannot accidentally lose data, and update the function comment to state the precondition.packages/metagame/src/entry_fee/entry_fee_store.cairo (1)
220-248: Reconfiguration leaves stale packed distribution slots.On a second call to
set_entry_fee_configfor the samecontext_id, this path only overwrites as many slots as the newsharesarray occupies and only updatesPackedDistribution. Stale trailing slots from a longer previousCustomconfig (or from switchingCustom → Linear/Exponential/Uniform/None) remain inEntryFee_distribution_shares_packed. Today this is not a correctness bug because all reads (get_distribution_shares,get_custom_share_at) are bounded by the freshly writtenpositions, so stale data is never exposed — but it is wasted gas on future rewrites and a minor hazard if a future reader ever uses anything other thanpositionsas the upper bound.Worth either (a) documenting that
set_entry_fee_configis intended to be effectively write-once per context, or (b) zeroing the slots in range[new_slots_used, calculate_slots_needed(old_positions))before/after writing the new shares. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/metagame/src/entry_fee/entry_fee_store.cairo` around lines 220 - 248, The current set_entry_fee_config path updates PackedDistribution (via set_distribution) and writes the new shares via store_distribution_shares but does not clear trailing packed slots when a prior Custom config had more entries, leaving stale data in EntryFee_distribution_shares_packed; update set_entry_fee_config to: read the old PackedDistribution.positions (or call get_distribution for context_id) to compute old_slots_needed, compute new_slots_needed from the new config (PackedDistribution.positions), then after calling store_distribution_shares (or before) write zeros to any packed slots in the range [new_slots_needed, old_slots_needed) to ensure no stale trailing slots remain; reference functions/structs to change: set_entry_fee_config, set_distribution, store_distribution_shares, PackedDistribution.positions, and the storage key EntryFee_distribution_shares_packed (used by get_distribution_shares/get_custom_share_at) when implementing the zeroing loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/metagame/src/entry_fee/entry_fee_component.cairo`:
- Around line 52-62: The code currently can panic when packing custom
distribution shares because store_distribution_shares uses u8 slot indices (15
shares per slot => max 255*15 = 3,825 shares) but set_entry_fee_config does not
validate PackedDistribution.positions; add an explicit bounds check in
set_entry_fee_config that reads the positions count from
PackedDistribution.positions and asserts (or returns an error) if positions >
3825 before calling store_distribution_shares, so
EntryFee_distribution_shares_packed never receives an out-of-range slot index
and we avoid try_into().unwrap() panics.
In `@packages/metagame/src/entry_fee/entry_fee_store.cairo`:
- Around line 360-363: The function get_custom_share_at currently does position
- 1 on a u32 which underflows when position == 0 and lacks an upper bound check;
update get_custom_share_at to assert position >= 1 before subtracting and also
assert position <= self.get_distribution(context_id).positions to prevent
out-of-range reads, then compute (slot_index, index_in_slot) =
calculate_slot_position(position - 1) and proceed as before; reference the
get_custom_share_at method and the get_distribution(context_id).positions field
(and ensure the same guard is applied on any wrapper like _get_custom_share_at
if present).
- Around line 94-115: The final catch-all else currently treats any unknown
packed_dist.dist_type as Distribution::Custom; change that to explicitly check
for DIST_TYPE_CUSTOM (returning Option::Some(Distribution::Custom(...)),
packed_dist.positions) and make the final fallback assert/revert with a clear
error (e.g., "Unknown distribution type") so corrupted/future tags don't
silently become Custom; update the nearby comment about the (DIST_TYPE_LINEAR,
0, 0) sentinel to state it intentionally collapses to None (Linear(0) is treated
as no distribution) rather than claiming it distinguishes the two cases.
In `@packages/metagame/src/entry_fee/tests/test_distribution_shares.cairo`:
- Around line 23-28: Rename the test test_distribution_shares_empty_by_default
to something that matches the actual assertion (e.g.,
test_count_zero_returns_empty) and update any references; additionally add a new
test that uses deploy_mock and calls get_distribution_shares with a nonzero
count (e.g., count = 3) for an unset context_id to assert it returns an array of
zeroed entries (packed slots read as felt252(0) → zero u16s), or alternatively
add a has_distribution sentinel check if you prefer that API change; target the
functions get_distribution_shares and deploy_mock when making these changes.
---
Nitpick comments:
In `@packages/metagame/src/entry_fee/entry_fee_store.cairo`:
- Around line 220-248: The current set_entry_fee_config path updates
PackedDistribution (via set_distribution) and writes the new shares via
store_distribution_shares but does not clear trailing packed slots when a prior
Custom config had more entries, leaving stale data in
EntryFee_distribution_shares_packed; update set_entry_fee_config to: read the
old PackedDistribution.positions (or call get_distribution for context_id) to
compute old_slots_needed, compute new_slots_needed from the new config
(PackedDistribution.positions), then after calling store_distribution_shares (or
before) write zeros to any packed slots in the range [new_slots_needed,
old_slots_needed) to ensure no stale trailing slots remain; reference
functions/structs to change: set_entry_fee_config, set_distribution,
store_distribution_shares, PackedDistribution.positions, and the storage key
EntryFee_distribution_shares_packed (used by
get_distribution_shares/get_custom_share_at) when implementing the zeroing loop.
In `@packages/utilities/src/distribution/packed_shares.cairo`:
- Around line 137-153: from_array currently silently truncates inputs longer
than SHARES_PER_SLOT; add an explicit precondition check at the start of the
function (referencing from_array, CustomShares, and SHARES_PER_SLOT) that fails
loudly when shares.len() > SHARES_PER_SLOT (e.g., assert or return an
error/panic), so callers cannot accidentally lose data, and update the function
comment to state the precondition.
🪄 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
Run ID: bd1cfc59-73c3-47ca-9639-d48e2a4019a0
📒 Files selected for processing (15)
packages/interfaces/src/entry_fee.cairopackages/metagame/src/entry_fee/entry_fee_component.cairopackages/metagame/src/entry_fee/entry_fee_store.cairopackages/metagame/src/entry_fee/store.cairopackages/metagame/src/entry_fee/tests.cairopackages/metagame/src/entry_fee/tests/mocks/entry_fee_mock.cairopackages/metagame/src/entry_fee/tests/test_claim_types.cairopackages/metagame/src/entry_fee/tests/test_distribution_shares.cairopackages/metagame/src/entry_fee/tests/test_entry_fee_store.cairopackages/metagame/src/entry_fee/tests/test_storage_gas.cairopackages/metagame/src/prize.cairopackages/metagame/src/prize/structs.cairopackages/utilities/src/distribution.cairopackages/utilities/src/distribution/packed_shares.cairopackages/utilities/src/distribution/structs.cairo
💤 Files with no reviewable changes (1)
- packages/metagame/src/prize.cairo
| /// Packed custom distribution shares per context: (context_id, slot_index) -> CustomShares | ||
| /// Each slot packs up to 15 u16 shares (16 bits each = 240 bits per felt252). | ||
| /// Unset unless the context uses a `Distribution::Custom` payout split. | ||
| /// The number of shares is the `positions` field of the sibling | ||
| /// `EntryFee_distribution` entry (they are written atomically in | ||
| /// `set_entry_fee_config`) — no separate count is stored. | ||
| EntryFee_distribution_shares_packed: Map<(u64, u8), CustomShares>, | ||
| /// Distribution shape + paid-places count packed into a single | ||
| /// felt252 (see `PackedDistribution`). Zero-valued when no | ||
| /// distribution is configured for the context. | ||
| EntryFee_distribution: Map<u64, PackedDistribution>, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C3 '\bslot(_index)?\s*:\s*u8|idx\s*/\s*15|SHARES_PER_SLOT' --type=cairoRepository: Provable-Games/game-components
Length of output: 103
🏁 Script executed:
#!/bin/bash
# Search for slot_index, SHARES_PER_SLOT, and related constants without file type flag
rg -n 'slot_index|SHARES_PER_SLOT|distribution_shares_packed' --include='*.cairo'Repository: Provable-Games/game-components
Length of output: 515
🏁 Script executed:
#!/bin/bash
# Find the entry_fee_component.cairo file and read lines around the storage definition
find . -name 'entry_fee_component.cairo' -type fRepository: Provable-Games/game-components
Length of output: 133
🏁 Script executed:
#!/bin/bash
# Look for PackedDistribution struct definition and positions field
rg -A 5 'struct PackedDistribution' --include='*.cairo'Repository: Provable-Games/game-components
Length of output: 515
🏁 Script executed:
#!/bin/bash
# Search for store_distribution_shares and get_distribution_shares implementations
rg -B 2 -A 10 'fn.*distribution_shares|store_distribution_shares|get_distribution_shares' --include='*.cairo'Repository: Provable-Games/game-components
Length of output: 515
🏁 Script executed:
#!/bin/bash
cat -n ./packages/metagame/src/entry_fee/entry_fee_component.cairo | head -150Repository: Provable-Games/game-components
Length of output: 7939
🏁 Script executed:
#!/bin/bash
# Search without --include flag
rg 'SHARES_PER_SLOT|slot_index.*u8|distribution_shares_packed'Repository: Provable-Games/game-components
Length of output: 8701
🏁 Script executed:
#!/bin/bash
# Search for PackedDistribution definition
rg -A 10 'struct PackedDistribution'Repository: Provable-Games/game-components
Length of output: 1041
🏁 Script executed:
#!/bin/bash
# Search for store_distribution_shares function
rg -B 3 -A 15 'fn.*store_distribution_shares\|fn.*get_distribution_shares'Repository: Provable-Games/game-components
Length of output: 56
🏁 Script executed:
#!/bin/bash
cat -n ./packages/metagame/src/entry_fee/entry_fee_store.cairo | grep -A 20 -B 5 "store_distribution_shares\|DISTRIBUTION_SHARES_PER_SLOT"Repository: Provable-Games/game-components
Length of output: 6825
🏁 Script executed:
#!/bin/bash
# Check what DISTRIBUTION_SHARES_PER_SLOT is aliased to
rg -B 2 -A 2 'DISTRIBUTION_SHARES_PER_SLOT'Repository: Provable-Games/game-components
Length of output: 1870
🏁 Script executed:
#!/bin/bash
# Verify the actual calculation in store_distribution_shares
sed -n '1,500p' ./packages/metagame/src/entry_fee/entry_fee_store.cairo | tail -200Repository: Provable-Games/game-components
Length of output: 8132
🏁 Script executed:
#!/bin/bash
# Look for set_entry_fee_config to see if there's validation
rg -B 3 -A 30 'fn set_entry_fee_config'Repository: Provable-Games/game-components
Length of output: 7208
🏁 Script executed:
#!/bin/bash
# Verify the exact behavior of try_into().unwrap() by looking at CustomShares implementation
rg -B 5 -A 20 'pub fn get_share\|pub fn set_share' packages/utilities/src/distribution/packed_shares.cairoRepository: Provable-Games/game-components
Length of output: 56
🏁 Script executed:
#!/bin/bash
# Check if there's any validation of share count elsewhere
rg 'SHARES_PER_SLOT\|3825\|shares.len()\s*>' packages/metagame/src/entry_fee/Repository: Provable-Games/game-components
Length of output: 56
🏁 Script executed:
#!/bin/bash
# Check for any tests related to store_distribution_shares with large arrays
rg -B 3 -A 10 'store_distribution_shares' packages/metagame/src/entry_fee/tests/Repository: Provable-Games/game-components
Length of output: 9720
🏁 Script executed:
#!/bin/bash
# Look for any constants or comments about distribution share limits
rg -i 'limit\|max\|capacity' packages/metagame/src/entry_fee/ | grep -i 'share\|distribution'Repository: Provable-Games/game-components
Length of output: 56
🏁 Script executed:
#!/bin/bash
# Confirm the try_into behavior - check if the division result could exceed u8
python3 << 'EOF'
# Maximum value that can be stored in u8
MAX_U8 = 255
SHARES_PER_SLOT = 15
MAX_SHARES = MAX_U8 * SHARES_PER_SLOT
print(f"Max shares before overflow: {MAX_SHARES}")
print(f"At share index {MAX_SHARES}: slot_index = {MAX_SHARES // SHARES_PER_SLOT} (fits in u8)")
print(f"At share index {MAX_SHARES + 1}: slot_index = {(MAX_SHARES + 1) // SHARES_PER_SLOT} (exceeds u8, would panic)")
EOFRepository: Provable-Games/game-components
Length of output: 221
Add bounds check for oversized custom distribution shares.
store_distribution_shares packs shares 15 per slot using u8 slot indices, capping capacity at 255 × 15 = 3,825 shares. PackedDistribution.positions is u32 and can far exceed this limit. While try_into().unwrap() will panic when slot index overflows u8, there is no explicit bounds validation in set_entry_fee_config before storing. Add an assertion to reject distributions with more than 3,825 shares rather than relying on implicit panic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/metagame/src/entry_fee/entry_fee_component.cairo` around lines 52 -
62, The code currently can panic when packing custom distribution shares because
store_distribution_shares uses u8 slot indices (15 shares per slot => max 255*15
= 3,825 shares) but set_entry_fee_config does not validate
PackedDistribution.positions; add an explicit bounds check in
set_entry_fee_config that reads the positions count from
PackedDistribution.positions and asserts (or returns an error) if positions >
3825 before calling store_distribution_shares, so
EntryFee_distribution_shares_packed never receives an out-of-range slot index
and we avoid try_into().unwrap() panics.
| let packed_dist = self.get_distribution(context_id); | ||
| let (distribution, distribution_count) = if packed_dist.dist_type == DIST_TYPE_LINEAR | ||
| && packed_dist.dist_param == 0 | ||
| && packed_dist.positions == 0 { | ||
| (Option::None, 0_u32) | ||
| } else if packed_dist.dist_type == DIST_TYPE_LINEAR { | ||
| (Option::Some(Distribution::Linear(packed_dist.dist_param)), packed_dist.positions) | ||
| } else if packed_dist.dist_type == DIST_TYPE_EXPONENTIAL { | ||
| (Option::Some(Distribution::Exponential(packed_dist.dist_param)), packed_dist.positions) | ||
| } else if packed_dist.dist_type == DIST_TYPE_UNIFORM { | ||
| (Option::Some(Distribution::Uniform), packed_dist.positions) | ||
| } else { | ||
| // DIST_TYPE_CUSTOM — return with empty shares span. Loading the | ||
| // full array is O(N/15) storage reads and is only needed for UI | ||
| // views; claim paths read a single share via `_get_custom_share_at` | ||
| // (O(1)). The strict-sum validator (Σshares == available_share | ||
| // at creation) means no dust needs to be tracked. Callers that | ||
| // need the full array (e.g. view functions) call | ||
| // `get_distribution_shares` explicitly, passing | ||
| // `packed_dist.positions` as the count. | ||
| (Option::Some(Distribution::Custom(array![].span())), packed_dist.positions) | ||
| }; |
There was a problem hiding this comment.
Unknown dist_type values silently become Custom.
The else at line 105 catches DIST_TYPE_CUSTOM but also every other value (4..=255). If storage is ever corrupted or written by a future component version using a new tag, the config will round-trip as a Custom distribution with positions coming from the (possibly garbage) packed field, which is a quietly wrong state for consumers. A matchless else-if chain is fine, but the final arm should be asserted against DIST_TYPE_CUSTOM explicitly rather than acting as a catch-all:
🛡️ Suggested tightening
- } else {
+ } else if packed_dist.dist_type == DIST_TYPE_CUSTOM {
// DIST_TYPE_CUSTOM — return with empty shares span. ...
(Option::Some(Distribution::Custom(array![].span())), packed_dist.positions)
- };
+ } else {
+ panic!("invalid dist_type in storage")
+ };Separately, the comment on lines 90-93 says the (LINEAR, 0, 0) sentinel lets consumers "distinguish 'no distribution configured' from 'Linear with weight 0'" — but the code actually collapses Linear(0) + distribution_count == 0 into None, so those two states are indistinguishable on read. Consider adjusting the comment to reflect that this is an intentional collapse (since Linear(0) is a no-op distribution) rather than a distinction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/metagame/src/entry_fee/entry_fee_store.cairo` around lines 94 - 115,
The final catch-all else currently treats any unknown packed_dist.dist_type as
Distribution::Custom; change that to explicitly check for DIST_TYPE_CUSTOM
(returning Option::Some(Distribution::Custom(...)), packed_dist.positions) and
make the final fallback assert/revert with a clear error (e.g., "Unknown
distribution type") so corrupted/future tags don't silently become Custom;
update the nearby comment about the (DIST_TYPE_LINEAR, 0, 0) sentinel to state
it intentionally collapses to None (Linear(0) is treated as no distribution)
rather than claiming it distinguishes the two cases.
| fn get_custom_share_at(self: @T, context_id: u64, position: u32) -> u16 { | ||
| let (slot_index, index_in_slot) = calculate_slot_position(position - 1); | ||
| self.get_distribution_shares_packed(context_id, slot_index).get_share(index_in_slot) | ||
| } |
There was a problem hiding this comment.
Position underflow / missing bounds guard.
position - 1 on u32 panics with an underflow error when a caller passes position == 0, which produces an opaque revert rather than a clean error. The doc-comment on the trait (line 55-60) explicitly tells callers "1..=positions" but no runtime guard is enforced here or on an upper bound. Given this is also reached via _get_custom_share_at on claim paths, a single explicit assert is cheap insurance:
🛡️ Proposed guard
fn get_custom_share_at(self: `@T`, context_id: u64, position: u32) -> u16 {
+ assert!(position > 0, "position must be 1-indexed");
let (slot_index, index_in_slot) = calculate_slot_position(position - 1);
self.get_distribution_shares_packed(context_id, slot_index).get_share(index_in_slot)
}Optionally also bounds-check position <= get_distribution(context_id).positions to avoid silently reading zeros for out-of-range positions (which a downstream payout calc could then treat as a legitimate 0-share).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/metagame/src/entry_fee/entry_fee_store.cairo` around lines 360 -
363, The function get_custom_share_at currently does position - 1 on a u32 which
underflows when position == 0 and lacks an upper bound check; update
get_custom_share_at to assert position >= 1 before subtracting and also assert
position <= self.get_distribution(context_id).positions to prevent out-of-range
reads, then compute (slot_index, index_in_slot) =
calculate_slot_position(position - 1) and proceed as before; reference the
get_custom_share_at method and the get_distribution(context_id).positions field
(and ensure the same guard is applied on any wrapper like _get_custom_share_at
if present).
| #[test] | ||
| fn test_distribution_shares_empty_by_default() { | ||
| let mock = deploy_mock(); | ||
| let shares = mock.get_distribution_shares(42, 0); | ||
| assert!(shares.len() == 0, "Default shares should be empty"); | ||
| } |
There was a problem hiding this comment.
Test name overstates what it validates.
This only covers the count == 0 early-return path inside get_distribution_shares; it never actually reads packed-share storage for an unconfigured context_id. Reading a fresh context with count > 0 would return a count-length array of zeros (because unset slots read as felt252(0) → all-zero u16s), which is a distinct behavior worth exercising if you want a genuine "default" guarantee. Consider renaming to test_count_zero_returns_empty and/or adding a separate test that asserts the zero-array behavior (or add a has_distribution sentinel and assert emptiness) for an unset context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/metagame/src/entry_fee/tests/test_distribution_shares.cairo` around
lines 23 - 28, Rename the test test_distribution_shares_empty_by_default to
something that matches the actual assertion (e.g.,
test_count_zero_returns_empty) and update any references; additionally add a new
test that uses deploy_mock and calls get_distribution_shares with a nonzero
count (e.g., count = 3) for an unset context_id to assert it returns an array of
zeroed entries (packed slots read as felt252(0) → zero u16s), or alternatively
add a has_distribution sentinel check if you prefer that API change; target the
functions get_distribution_shares and deploy_mock when making these changes.
1 participant
Summary
Distribution::Custom(Span<u16>)work end-to-end inEntryFeeComponent— previously accepted at the API but silently dropped on write and rehydrated as an empty span.CustomShares+ share-packing math out ofmetagame::prizeintoutilities::distribution::packed_sharesso entry-fee and prize paths share the same packing primitive (15 u16 per felt252).PackedDistribution(dist_type + dist_param + positions) withStorePackingtoutilities::distributionand a companionEntryFee_distributionmap +EntryFee_distribution_shares_packedout-of-band storage on the component.EntryFeeConfigininterfaces::entry_feewithdistribution: Option<Distribution>anddistribution_count: u32.set_entry_fee_confignow persists the full distribution (including Custom shares) andget_entry_feerehydrates it._get_custom_share_at(context_id, position)for O(1) claim paths that only need one share.get_entry_feereturnsCustom(empty_span)by design so the generic path isn't forced to do the O(N/15) reconstruction — UI views call_get_distribution_shares(id, count)explicitly.Breaking change
EntryFeeConfiggained two required fields. Existing consumers need to adddistribution: Option::None, distribution_count: 0to every struct literal. All in-repo call sites are updated in this PR (test helpers + mocks + existing test suites).Test plan
scarb buildclean across utilities, interfaces, metagame.snforge testfull metagame suite: 415 passing (includes 5 newtest_distribution_sharesround-trip tests covering empty default, single-slot, full-15-share, 50-share multi-slot, and per-context isolation).snforge testutilities suite: 236 passing (includes moved packed-shares tests).CustomSharesviapub useso consumers importing fromcrate::prize::structskeep working.Downstream
Budokan PR that consumes this lives at (link to be added) — it swaps Budokan's own
Map<u64, PackedDistribution>storage for the component-owned version and uses the new fast path. Tagging a release against this branch before merging the Budokan PR.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests