perf(entry-requirement): drop redundant entries_left cross-call#109
perf(entry-requirement): drop redundant entries_left cross-call#109starknetdev merged 2 commits intomainfrom
Conversation
The extension branch of update_qualification_entries dispatched a second cross-contract call into the validator's entries_left just to assert > 0. For validators with expensive view paths (Opus walks user troves end-to-end twice) this duplicated the work valid_entry already performed. The IEntryRequirementExtension contract is now: valid_entry MUST enforce both eligibility and remaining-entry quota. The framework trusts that single signal on the entry path. entries_left remains for off-chain consumers. See companion PR in metagame_extensions for matching docs and the in-tree validators that now bundle quota. Updates the three update_qualification_entries extension-path tests to assert no-op behavior (previously they asserted on the entries_left panic, which has been removed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 ReviewTodo List
run=24987620561 attempt=1 sha=4e60abc92e1fb95b8e93c605e65ea3d44c9e6cb7 scope=general Review ResultsNo issues found. The git diff shows no changes to files outside the Summary: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFO |
Codex Review - General Engineering 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 removes redundant cross-contract calls to extensions within update_qualification_entries, as extensions now manage eligibility and entry quotas internally. Corresponding tests were updated to reflect that this function is now a no-op for extension-based requirements. A suggestion was made to replace the wildcard match arm with an explicit one for EntryRequirementType::token to improve maintainability and leverage compiler checks for future enum variants.
| // call here — it would walk the same on-chain state a second time. | ||
| // See IEntryRequirementExtension docs for the contract. | ||
| }, | ||
| _ => { |
There was a problem hiding this comment.
Using an explicit match arm for EntryRequirementType::token instead of a wildcard _ is preferred for maintainability. This ensures that if new requirement types are added to the enum in the future, the compiler will require them to be explicitly handled here, preventing accidental fall-through to the internal counter logic.
EntryRequirementType::token(_) => {
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/metagame/src/entry_requirement/tests/test_entry_requirement_component.cairo (1)
641-718: Strengthen the no-op assertions and drop unused test setup.The three tests currently only verify that
update_qualification_entriesdoesn't panic on the extension path. To actually pin the no-op behavior (and catch a regression where state starts being written for extensions), assert the stored entry count remains zero after the call. Additionally, since the branch no longer calls into the validator and no longer reads the caller, thecheat_caller_addresssetup and the validator deployments don't affect the outcome — they make the tests look like they exercise more than they do.♻️ Suggested tightening
fn test_update_qualification_entries_extension_is_noop() { let mock = deploy_entry_requirement_mock(); - let caller = make_address(0x555); - let validator_addr = deploy_entry_validator_mock(caller); + let validator_addr = deploy_entry_validator_mock(make_address(0x555)); let context_id: u64 = 80; + let proof = QualificationProof::Extension(array![].span()); let req = EntryRequirement { entry_limit: 10, entry_requirement_type: EntryRequirementType::extension( ExtensionConfig { address: validator_addr, config: array![].span() }, ), }; - snforge_std::cheat_caller_address( - mock.contract_address, caller, snforge_std::CheatSpan::TargetCalls(1), - ); - mock - .update_qualification_entries( - context_id, QualificationProof::Extension(array![].span()), req, - ); + mock.update_qualification_entries(context_id, proof, req); + + // No-op: framework must not have recorded any entry for the extension path. + let entries = mock.get_qualification_entries(context_id, proof); + assert!(entries.entry_count == 0, "extension path must not write entry_count"); }Apply the same
entry_count == 0assertion to..._noop_even_when_extension_would_rejectand..._accepts_any_proof_type, and drop theircheat_caller_addresscalls as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/metagame/src/entry_requirement/tests/test_entry_requirement_component.cairo` around lines 641 - 718, The tests test_update_qualification_entries_extension_is_noop, test_update_qualification_entries_extension_noop_even_when_extension_would_reject, and test_update_qualification_entries_extension_accepts_any_proof_type only ensure no panic; strengthen them by asserting the stored entry count remains zero after calling update_qualification_entries to verify no state was written (use the test helper or contract view that reads the stored entry count for the given context_id and caller), and remove unnecessary setup in the latter two tests (drop snforge_std::cheat_caller_address and the validator deployment calls in test_update_qualification_entries_extension_noop_even_when_extension_would_reject and test_update_qualification_entries_extension_accepts_any_proof_type) since the branch is a no-op and those side effects are unused.
🤖 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_requirement/entry_requirement_store.cairo`:
- Around line 192-199: The EntryRequirement for type
EntryRequirementType::extension currently accepts and stores entry_limit but
never uses it (update_qualification_entries no-ops for extension), which is
confusing; modify set_entry_requirement to detect
EntryRequirementType::extension and either reject non-zero entry_limit (return
an error) or explicitly set entry_limit to zero, and add a short doc comment on
the EntryRequirement struct explaining that extension requirements ignore
entry_limit and must enforce quotas in their valid_entry implementations; ensure
callers still call validate_qualification before update_qualification_entries by
referencing validate_qualification and update_qualification_entries in the
comment.
---
Nitpick comments:
In
`@packages/metagame/src/entry_requirement/tests/test_entry_requirement_component.cairo`:
- Around line 641-718: The tests
test_update_qualification_entries_extension_is_noop,
test_update_qualification_entries_extension_noop_even_when_extension_would_reject,
and test_update_qualification_entries_extension_accepts_any_proof_type only
ensure no panic; strengthen them by asserting the stored entry count remains
zero after calling update_qualification_entries to verify no state was written
(use the test helper or contract view that reads the stored entry count for the
given context_id and caller), and remove unnecessary setup in the latter two
tests (drop snforge_std::cheat_caller_address and the validator deployment calls
in
test_update_qualification_entries_extension_noop_even_when_extension_would_reject
and test_update_qualification_entries_extension_accepts_any_proof_type) since
the branch is a no-op and those side effects are unused.
🪄 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: a49f5c06-f119-437b-acdc-5bdbc31e64af
📒 Files selected for processing (2)
packages/metagame/src/entry_requirement/entry_requirement_store.cairopackages/metagame/src/entry_requirement/tests/test_entry_requirement_component.cairo
| match entry_requirement.entry_requirement_type { | ||
| EntryRequirementType::extension(extension_config) => { | ||
| let extension_address = extension_config.address; | ||
| let extension_dispatcher = IEntryRequirementExtensionDispatcher { | ||
| contract_address: extension_address, | ||
| }; | ||
| let display_extension_address: felt252 = extension_address.into(); | ||
| let caller_address = get_caller_address(); | ||
| let context_owner = get_contract_address(); | ||
|
|
||
| let qualification = match qualifier { | ||
| QualificationProof::Extension(qual) => qual, | ||
| _ => panic!( | ||
| "EntryRequirement: Provided qualification proof is not of type 'Extension'", | ||
| ), | ||
| }; | ||
|
|
||
| let entries_left = extension_dispatcher | ||
| .entries_left(context_owner, context_id, caller_address, qualification); | ||
|
|
||
| match entries_left { | ||
| Option::Some(entries_left) => { | ||
| assert!( | ||
| entries_left > 0, | ||
| "EntryRequirement: No entries left according to extension {}", | ||
| display_extension_address, | ||
| ); | ||
| }, | ||
| Option::None => {}, | ||
| } | ||
| EntryRequirementType::extension(_) => { | ||
| // Extensions enforce both eligibility and remaining-entry quota inside | ||
| // their own `valid_entry` (called from `validate_qualification`). The | ||
| // framework no longer makes a redundant `entries_left` cross-contract | ||
| // call here — it would walk the same on-chain state a second time. | ||
| // See IEntryRequirementExtension docs for the contract. | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find every call site for both methods and inspect 30 lines around each
rg -nP --type=cairo -C8 '\bupdate_qualification_entries\s*\('
echo "----"
rg -nP --type=cairo -C8 '\bvalidate_qualification\s*\('Repository: Provable-Games/game-components
Length of output: 157
🏁 Script executed:
#!/bin/bash
# Find calls to both methods in .cairo files
echo "=== Searching for update_qualification_entries calls ==="
rg -g '*.cairo' -n 'update_qualification_entries' -B 8 -A 8
echo ""
echo "=== Searching for validate_qualification calls ==="
rg -g '*.cairo' -n 'validate_qualification' -B 8 -A 8
echo ""
echo "=== Checking the target file for entry_limit in Extension handling ==="
fd -e cairo | xargs grep -l 'set_entry_requirement' | head -5Repository: Provable-Games/game-components
Length of output: 50386
🏁 Script executed:
#!/bin/bash
# View the entry_requirement_store.cairo file around the key sections
echo "=== Implementation of update_qualification_entries (lines 186-207) ==="
sed -n '186,207p' packages/metagame/src/entry_requirement/entry_requirement_store.cairo
echo ""
echo "=== Full view of entry_requirement_store.cairo (key sections) ==="
sed -n '50,230p' packages/metagame/src/entry_requirement/entry_requirement_store.cairo
echo ""
echo "=== Search for set_entry_requirement function ==="
rg -g '*.cairo' -n 'fn set_entry_requirement' -A 30 --max-count=2Repository: Provable-Games/game-components
Length of output: 28138
Document that entry_limit is accepted but ignored for Extension requirements; consider rejecting non-zero values at set time for clarity.
The Extension branch of update_qualification_entries (lines 192–199) is correctly a no-op, delegating quota enforcement to the extension's valid_entry call. However, entry_limit is still stored for Extension types in set_entry_requirement (lines 81–84) and never read by the framework. This silently-ignored field can confuse integrators. Either document this contract clearly in the EntryRequirement struct or extension docs, or reject non-zero entry_limit for Extension types at set time to make the semantics unambiguous.
Additionally, the correctness of this pattern depends on callers invoking validate_qualification before update_qualification_entries on the entry path (so that the extension's valid_entry is called first to enforce quotas). Verify this invariant holds across all production call sites.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/metagame/src/entry_requirement/entry_requirement_store.cairo` around
lines 192 - 199, The EntryRequirement for type EntryRequirementType::extension
currently accepts and stores entry_limit but never uses it
(update_qualification_entries no-ops for extension), which is confusing; modify
set_entry_requirement to detect EntryRequirementType::extension and either
reject non-zero entry_limit (return an error) or explicitly set entry_limit to
zero, and add a short doc comment on the EntryRequirement struct explaining that
extension requirements ignore entry_limit and must enforce quotas in their
valid_entry implementations; ensure callers still call validate_qualification
before update_qualification_entries by referencing validate_qualification and
update_qualification_entries in the comment.
scarb fmt collapses comments inside an empty match-arm body onto the same line as the braces. Moving the explanation up to the fn doc keeps it readable and keeps the formatter happy. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Claude finished @starknetdev's task —— View job Claude Review - General Engineering ReviewTodo List
run=24987882618 attempt=1 sha=ded0faa5b8d506e869460771af401b98d30a0107 scope=general Review ResultsNo issues found. The git diff shows no changes to files outside the 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 ReviewTodo List
run=24987882618 attempt=1 sha=ded0faa5b8d506e869460771af401b98d30a0107 scope=packages Review Findings[INFO] packages/metagame/src/entry_requirement/entry_requirement_store.cairo:200 - Using wildcard pattern [LOW] packages/metagame/src/entry_requirement/entry_requirement_store.cairo:81-84 - Extension requirements accept and store [LOW] packages/metagame/src/entry_requirement/tests/test_entry_requirement_component.cairo:641-718 - Test functions only verify no panic but don't assert the no-op behavior Security Analysis✅ Access Control: Proper use of Architecture ReviewThe refactor correctly implements the performance optimization by:
Test Quality AssessmentThe test changes appropriately reflect the new no-op behavior:
Gas Optimization✅ Significant Improvement: Eliminates expensive cross-contract call on entry path Correctness VerificationThe refactor relies on the critical assumption that Summary: 0 CRITICAL, 0 HIGH, 2 LOW, 1 INFO RecommendationThe performance optimization is well-implemented and maintains security guarantees. The suggestions above are minor improvements for code clarity and test robustness. The PR can proceed after addressing the test coverage gap to prevent future regressions. |
Codex Review - Cairo/Starknet Contract ReviewReview process failed to complete. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…225) * perf(contracts): bundle quota into validate_entry in validator mocks Aligns the budokan in-tree validator mocks with the IEntryRequirementExtension contract change in metagame_extensions: valid_entry must enforce both eligibility and remaining-entry quota, since the metagame framework no longer cross-checks entries_left on the entry path. - entry_validator_mock: validate_entry now also rejects when current_entries >= entry_limit. - tournament_validator_mock: same fix. Required so the existing test_extension_gated_tournament_entry_limit_enforced and test_extension_gated_tournament_with_entry_limit cases continue to assert the right invariant after the framework drops its defensive entries_left call. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * style: scarb fmt Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(contracts): bump game_components dep to post-#109 main Refresh Scarb.lock to point game_components_* at the new main SHA that includes "perf(entry-requirement): drop redundant entries_left cross-call" (Provable-Games/game-components#109). With that change, the framework no longer dispatches entries_left into the extension during enter_tournament — quota exhaustion now surfaces as the validator's own valid_entry returning false, which the framework reports as "Invalid entry according to extension". Updates two should_panic tests that grepped the old "No entries left according to extension" message: - test_extension_gated_tournament_entry_limit_enforced - test_create_tournament_gated_by_multiple_tournaments_with_limited_entry snforge test --package budokan: 196 passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * perf(contracts): quota check before validate_entry_internal in mocks Acked review feedback on PR #225 — reorder valid_entry so the cheap storage-read quota check short-circuits before the expensive validate_entry_internal dispatch. Functionally identical on the happy path; real saving when the player is already over their per-context cap. - entry_validator_mock: quota check (one Map read) runs first; the ERC721 balance_of call only happens for players still within quota. - tournament_validator_mock: same shape; the much more expensive chain (ITournament + IRegistration + IMinigame + IERC721 + leaderboard search) is fully skipped when quota is exhausted. snforge test --package budokan: 196 passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
Drops the second cross-contract dispatch into the validator on the entry path. Previously
update_qualification_entriescalled the extension'sentries_leftpurely to assert> 0— but the extension'svalid_entry(already invoked fromvalidate_qualification) is contractually required to enforce that. For validators with expensive view paths, this was duplicating the work end-to-end.The new
IEntryRequirementExtensioncontract:valid_entryMUST enforce both eligibility AND remaining-entry quota. The framework trusts that single signal on the entry path.entries_leftremains as an off-chain view.Pairs with metagame_extensions PR that:
valid_entryrequirement.valid_entry.Test changes
Three tests in
test_entry_requirement_component.cairocovered the dispatcher behavior we just removed:extension_none_entries_leftandextension_some_entries_remainingrewritten toextension_is_noop.extension_zero_entries_left(wasshould_panic) →extension_noop_even_when_extension_would_reject.extension_wrong_proof_type(wasshould_panicon qualifier destructuring) →extension_accepts_any_proof_type.Test plan
scarb build --workspacepassessnforge test --package game_components_metagamepasses (418/418)snforge test --workspaceonce metagame_extensions sister PR lands and is tagged🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Tests