feat: bump metagame_extensions to v0.1.3 (per-context bannable)#103
feat: bump metagame_extensions to v0.1.3 (per-context bannable)#103starknetdev merged 1 commit intomainfrom
Conversation
Adapts mocks and tests to the new IEntryRequirementExtension interface: - registration_only() removed; replaced with bannable(context_id) -> bool - Mock constructors no longer take registration_only parameter - Test deploy helpers updated accordingly Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe pull request updates the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Codex Review - General Engineering ReviewReview process failed to complete. |
|
Claude finished @starknetdev's task —— View job Claude Review - General Engineering Reviewrun=24693726927 attempt=1 sha=3850f52fb1c5afb397e2a725d4f8c303884b2479 scope=general Todo List
Review FindingsAfter analyzing the targeted diff outside the Change Analysis:
Infrastructure Impact Assessment: No actionable findings inside the scoped diff. The dependency version bump in Summary: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFO |
|
Claude finished @starknetdev's task —— View job Claude Review - Cairo/Starknet Contract Reviewrun=24693726927 attempt=1 sha=3850f52fb1c5afb397e2a725d4f8c303884b2479 scope=packages Todo List
Review Findings[MEDIUM] packages/test_common/src/mocks/mock_entry_validator.cairo:47 - Missing setter function for bannable storage [MEDIUM] packages/test_common/src/mocks/mock_rejecting_entry_validator.cairo:47 - Missing setter function for bannable storage [MEDIUM] packages/metagame/src/entry_requirement/tests/mocks/*.cairo - All mock files missing setter functions [LOW] packages/metagame/src/entry_requirement/tests/test_entry_requirement_component.cairo - Missing test coverage for bannable functionality [INFO] All files - Proper Cairo idiom usage [INFO] All files - Constructor signature change is correct Summary: 0 CRITICAL, 0 HIGH, 3 MEDIUM, 1 LOW, 2 INFOKey Issue: The interface migration is incomplete. While the storage and function signatures were updated correctly, the mock contracts lack the ability to actually configure the Recommendation: Add setter functions to all mock contracts and create test cases that verify per-context bannable behavior works correctly. |
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 updates the metagame_extensions_interfaces dependency to v0.1.3 and refactors several entry validator mocks. The registration_only boolean has been replaced with a bannable storage map across multiple mock contracts, and the corresponding constructors and test deployment functions have been updated. Feedback focuses on the fact that the new bannable storage maps are never written to, causing them to always return false. This is inconsistent with the intended behavior of mocks like the rejecting validator, which should return true for bannable. It is recommended to remove the unused storage variables and return constant values directly to simplify the code.
| bannable: Map<u64, bool>, | ||
| #[substorage(v0)] |
There was a problem hiding this comment.
The bannable map is added to storage but never written to, meaning it will always return false for any context_id. In this specific mock, should_ban returns true (line 67), which is inconsistent with a non-bannable validator. Since this is a mock that always rejects and supports banning, it should return true for bannable. You can simplify this by removing the storage variable and returning a constant in the function, which also avoids the need for the new Map and StorageMapReadAccess imports.
#[substorage(v0)]
| self.bannable.read(context_id) | ||
| } |
There was a problem hiding this comment.
This should return true to be consistent with the mock's purpose and its should_ban implementation. Returning false (the default for an uninitialized map) might cause the system to skip the ban check entirely.
true
}
| bannable: Map<u64, bool>, | ||
| #[substorage(v0)] |
There was a problem hiding this comment.
The bannable map is uninitialized and never written to. For a mock with fixed behavior, it's better to remove this from storage and return a constant in the implementation.
#[substorage(v0)]
| self.bannable.read(context_id) | ||
| } |
There was a problem hiding this comment.
This mock's should_ban returns true (line 67), so bannable should also return true to be consistent. Returning false (the default for an uninitialized map) might cause the system to skip the ban check entirely.
true
}
| bannable: Map<u64, bool>, | ||
| #[substorage(v0)] |
There was a problem hiding this comment.
The bannable map is added to storage but never written to. Since this mock is intended to always return a fixed value (false), you can simplify it by removing the storage variable and returning false directly in the function.
#[substorage(v0)]
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/metagame/src/entry_requirement/tests/mocks/rejecting_entry_validator_mock.cairo (1)
22-48:⚠️ Potential issue | 🟠 MajorAdd a setter to allow the bannable state to be configured in the rejecting mock.
The
bannablemap is declared but never written, causingbannable(context_id)to always returnfalsewhileshould_ban()always returnstrue. This creates a logical inconsistency where tests gating the ban path behind abannablecheck won't exercise rejection. AddStorageMapWriteAccessto imports and provide a mock setter or initialization path to configure this state during test setup.Example: add a setter function
use starknet::storage::{ - Map, StorageMapReadAccess, StoragePointerReadAccess, StoragePointerWriteAccess, + Map, StorageMapReadAccess, StorageMapWriteAccess, StoragePointerReadAccess, + StoragePointerWriteAccess, }; @@ -36,6 +36,11 @@ pub mod RejectingEntryValidatorMock { #[constructor] fn constructor(ref self: ContractState, owner: ContractAddress) { self.owner_address.write(owner); self.src5.register_interface(IENTRY_REQUIREMENT_EXTENSION_ID); } + + #[external(v0)] + fn set_bannable(ref self: ContractState, context_id: u64, value: bool) { + self.bannable.write(context_id, value); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/metagame/src/entry_requirement/tests/mocks/rejecting_entry_validator_mock.cairo` around lines 22 - 48, The rejecting_entry_validator_mock defines a read-only bannable: Map<u64, bool> but never writes to it, so bannable(context_id) always returns false; add StorageMapWriteAccess to the imports and implement a setter (e.g., set_bannable or set_bannable_state) on the mock contract that writes into the bannable map so tests can configure bannable per context_id; update the mock's ABI/impl (EntryValidatorImpl/IEntryRequirementExtension) to expose the setter and ensure tests call it during setup so should_ban() and bannable(context_id) behave consistently.packages/test_common/src/mocks/mock_rejecting_entry_validator.cairo (1)
22-48:⚠️ Potential issue | 🟠 MajorAdd a setter to configure
bannablefor test scenarios.The
bannablemap is never written, sobannable(context_id)always returnsfalseregardless of context. Althoughshould_ban()returnstrue, tests cannot exercise per-context bannable rejection behavior. Add a test-only setter to make the mock configurable.Suggested implementation
use starknet::storage::{ - Map, StorageMapReadAccess, StoragePointerReadAccess, StoragePointerWriteAccess, + Map, StorageMapReadAccess, StorageMapWriteAccess, StoragePointerReadAccess, + StoragePointerWriteAccess, }; @@ #[constructor] fn constructor(ref self: ContractState, owner: ContractAddress) { self.owner_address.write(owner); self.src5.register_interface(IENTRY_REQUIREMENT_EXTENSION_ID); } + + #[external(v0)] + fn set_bannable(ref self: ContractState, context_id: u64, value: bool) { + self.bannable.write(context_id, value); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/test_common/src/mocks/mock_rejecting_entry_validator.cairo` around lines 22 - 48, The bannable Map<u64, bool> is never written so bannable(context_id) always returns false; add a test-only setter method (e.g., fn set_bannable(self: `@ContractState`, context_id: u64, value: bool)) on the EntryValidatorImpl or as an extra #[external] test helper that writes to self.bannable.write(context_id, value) so tests can configure per-context bannable values; reference the bannable field, the existing bannable(self: `@ContractState`, context_id: u64) accessor, and add the new setter to the same impl block so test code can toggle rejection behavior.
🧹 Nitpick comments (1)
packages/metagame/src/entry_requirement/tests/test_entry_requirement_component.cairo (1)
406-421: Add coverage for per-contextbannablebehavior.The helper updates keep deployments compiling, but the new v0.1.3 behavior isn’t asserted: no test configures different
bannablevalues for differentcontext_ids or verifies the ban path. Please add a focused test once the mocks expose a way to setbannable(context_id).As per coding guidelines,
**/*test*.cairo: “Achieve and maintain 90% line coverage for tests; edge cases must be fuzzed” and “Track test coverage; coverage must not decrease after changes.”Also applies to: 627-634
🤖 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 406 - 421, The tests lack coverage for per-context bannable behavior; add a new focused test that uses the entry validator mocks (targeting deploy_entry_validator_mock and deploy_rejecting_entry_validator_mock) to set different bannable(context_id) values for at least two context IDs, exercise both the accept and ban paths (e.g., attempt entry where bannable=false and where bannable=true leading to ban), and assert expected outcomes (success vs. ban event/state) and any expected error messages; if the current mocks do not expose a setter for bannable(context_id), extend the mocks to provide a method to set bannable per context and then use that in the new test so the behavior for v0.1.3 is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@packages/metagame/src/entry_requirement/tests/mocks/rejecting_entry_validator_mock.cairo`:
- Around line 22-48: The rejecting_entry_validator_mock defines a read-only
bannable: Map<u64, bool> but never writes to it, so bannable(context_id) always
returns false; add StorageMapWriteAccess to the imports and implement a setter
(e.g., set_bannable or set_bannable_state) on the mock contract that writes into
the bannable map so tests can configure bannable per context_id; update the
mock's ABI/impl (EntryValidatorImpl/IEntryRequirementExtension) to expose the
setter and ensure tests call it during setup so should_ban() and
bannable(context_id) behave consistently.
In `@packages/test_common/src/mocks/mock_rejecting_entry_validator.cairo`:
- Around line 22-48: The bannable Map<u64, bool> is never written so
bannable(context_id) always returns false; add a test-only setter method (e.g.,
fn set_bannable(self: `@ContractState`, context_id: u64, value: bool)) on the
EntryValidatorImpl or as an extra #[external] test helper that writes to
self.bannable.write(context_id, value) so tests can configure per-context
bannable values; reference the bannable field, the existing bannable(self:
`@ContractState`, context_id: u64) accessor, and add the new setter to the same
impl block so test code can toggle rejection behavior.
---
Nitpick comments:
In
`@packages/metagame/src/entry_requirement/tests/test_entry_requirement_component.cairo`:
- Around line 406-421: The tests lack coverage for per-context bannable
behavior; add a new focused test that uses the entry validator mocks (targeting
deploy_entry_validator_mock and deploy_rejecting_entry_validator_mock) to set
different bannable(context_id) values for at least two context IDs, exercise
both the accept and ban paths (e.g., attempt entry where bannable=false and
where bannable=true leading to ban), and assert expected outcomes (success vs.
ban event/state) and any expected error messages; if the current mocks do not
expose a setter for bannable(context_id), extend the mocks to provide a method
to set bannable per context and then use that in the new test so the behavior
for v0.1.3 is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 184518ad-5e94-4f49-a640-3af619c379a7
📒 Files selected for processing (7)
Scarb.tomlpackages/metagame/src/entry_requirement/tests/mocks/accepting_limited_entry_validator_mock.cairopackages/metagame/src/entry_requirement/tests/mocks/entry_validator_mock.cairopackages/metagame/src/entry_requirement/tests/mocks/rejecting_entry_validator_mock.cairopackages/metagame/src/entry_requirement/tests/test_entry_requirement_component.cairopackages/test_common/src/mocks/mock_entry_validator.cairopackages/test_common/src/mocks/mock_rejecting_entry_validator.cairo
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
Summary
metagame_extensions_interfacesfrom v0.1.2 to v0.1.3bannable(context_id)interface (formerlyregistration_only())Context
metagame_extensions v0.1.3 made the bannable flag per-context rather than a global validator setting (Provable-Games/metagame_extensions#17). This requires interface implementers to update their mock signatures.
Changes
registration_only: boolstorage →bannable: Map<u64, bool>, removed from constructorregistration_onlyfrom deploy helpers and call sitesTest plan
scarb buildpassessnforge test --workspacepasses🤖 Generated with Claude Code
Summary by CodeRabbit
Dependencies
Updates