feat(registration): add token reverse-index for O(1) lookups#107
feat(registration): add token reverse-index for O(1) lookups#107starknetdev merged 5 commits intomainfrom
Conversation
Stores token_id -> context_id and token_id -> entry_id alongside the forward (context_id, entry_id) -> token_id index, written atomically in set_entry. Exposes get_token_context, get_entry_id_for_token, and get_entry_by_token on IRegistration so consumers don't need to maintain their own duplicate maps (as budokan and bokendo currently do). Tokens are globally unique within a component instance, so a single felt252 key is sufficient and strictly more storage-efficient than keying by (context_id, token_id). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 47 minutes and 59 seconds. ⌛ 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 (3)
📝 WalkthroughWalkthroughThe pull request extends the registration component with reverse-lookup functionality by adding mappings from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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=24961154822 attempt=1 sha=b23d5e34a47469d4a9d108c4702b0a9e2da42ea7 scope=general Todo List
Review ResultsNo issues found. The git diff command with the specified scope ( All modified files are within the packages directory:
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 implements reverse lookups for the registration system, allowing O(1) mapping from token IDs to context and entry IDs. Changes include updates to the IRegistration interface, the addition of reverse index storage in RegistrationComponent, and corresponding logic in RegistrationStore. Review feedback identifies a data inconsistency risk in set_entry where existing reverse mappings are not cleared when an entry is overwritten, and points out that get_entry_by_token does not panic for unregistered tokens as specified in the documentation.
| self.set_token_context(*registration.game_token_id, *registration.context_id); | ||
| self.set_token_entry_id(*registration.game_token_id, *registration.entry_id); |
There was a problem hiding this comment.
The current implementation of set_entry does not clear the reverse index for the previous token_id when an entry is overwritten. If a registration at (context_id, entry_id) is updated with a new token_id, the old token_id will still point to this entry in the reverse index, leading to data inconsistency. You should fetch the existing token_id for the given coordinates and, if it exists and is different from the new one, clear its reverse mapping.
| fn get_entry_by_token(self: @T, token_id: felt252) -> Registration { | ||
| let context_id = self.get_token_context(token_id); | ||
| let entry_id = self.get_token_entry_id(token_id); | ||
| self.get_entry(context_id, entry_id) | ||
| } |
There was a problem hiding this comment.
The implementation of get_entry_by_token does not fulfill the requirement stated in the trait documentation ("panics if unregistered"). If the token is not found, it currently returns a zeroed Registration struct (or whatever is at context_id: 0, entry_id: 0). Additionally, adding a safety check to ensure the forward mapping matches the reverse mapping is recommended to handle potential index inconsistencies.
fn get_entry_by_token(self: @T, token_id: felt252) -> Registration {
let context_id = self.get_token_context(token_id);
let entry_id = self.get_token_entry_id(token_id);
let entry = self.get_entry(context_id, entry_id);
assert(entry.game_token_id == token_id, 'Registration: Token not found');
entry
}
Adds _get_token_context, _get_entry_id_for_token, and _get_entry_by_token on RegistrationInternalImpl so contracts can call them from internal code via self.registration._get_token_context(...) — matching the pattern used by existing internal helpers like _get_entry and _get_entry_count. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Claude finished @starknetdev's task —— View job Claude Review - General Engineering Reviewrun=24961211786 attempt=1 sha=4beb1daf511bfb89308beb57ab96ece103d9d963 scope=general Todo List
Review ResultsNo issues found. The git diff command with the specified scope ( All modified files are within the packages directory:
Summary: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFO |
Codex Review - General Engineering ReviewReview process failed to complete. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/metagame/src/registration/registration_store.cairo (1)
39-51:⚠️ Potential issue | 🟠 MajorStale reverse-index when an entry is overwritten with a different
game_token_id.
set_entryunconditionally writes the reverse mappings keyed on the newgame_token_id, but does not clear the reverse mapping for the previousgame_token_idat the same(context_id, entry_id)slot. After:set_entry(ctx=10, entry=1, token=A) // reverse[A] = (10,1) set_entry(ctx=10, entry=1, token=B) // reverse[B] = (10,1); reverse[A] still = (10,1)
get_entry_by_token(A)now returns the entry that actually belongs to tokenB, silently misattributing data. Your existingtest_overwrite_entryexercises this exact path but does not assert anything about the reverse index.The PR description states tokens are "globally unique within a component instance," which makes overwrites unlikely in production — but
set_entryis also used as the only write path, so any caller that re-registers the same slot will trip this. Worth either:
- Reading the prior
game_token_idand zeroing its reverse entries before writing the new ones, or- Adding an
assert!(prior_token_id == 0 || prior_token_id == new_token_id, ...)to make overwrites with a different token explicit.🔧 Suggested guard / cleanup
fn set_entry(ref self: T, registration: `@Registration`) { RegistrationValidationImpl::assert_valid_token_id(*registration.game_token_id); + let prior_token_id = self + .get_token_id(*registration.context_id, *registration.entry_id); + if prior_token_id != 0 && prior_token_id != *registration.game_token_id { + // Clear stale reverse index for the previous token at this slot + self.set_token_context(prior_token_id, 0); + self.set_token_entry_id(prior_token_id, 0); + } self .set_token_id( *registration.context_id, *registration.entry_id, *registration.game_token_id, );Also consider extending
test_overwrite_entryto assert thatget_entry_by_token(old_token)no longer points at the overwritten slot.
🧹 Nitpick comments (2)
packages/interfaces/src/registration.cairo (1)
30-31: Document the unregistered-token behavior ofget_entry_by_token.The two preceding methods explicitly state "0 if not registered", but
get_entry_by_tokenonly says "Get the full registration entry for a token". Per the implementation inpackages/metagame/src/registration/registration_store.cairo, an unknown token returns a zero-valuedRegistration(not a panic, not anOption), which integrators consuming this interface (e.g., budokan, bokendo) need to know to avoid mistaking a zero record for a real one.📝 Suggested doc update
- /// Get the full registration entry for a token + /// Get the full registration entry for a token. Returns a zero-valued + /// `Registration` (all fields 0/false) when the token is not registered; + /// callers that need to distinguish should check `game_token_id != 0` or + /// call `get_token_context`/`get_entry_id_for_token` first. fn get_entry_by_token(self: `@TState`, token_id: felt252) -> Registration;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/interfaces/src/registration.cairo` around lines 30 - 31, Update the docstring for get_entry_by_token to clearly state its unregistered-token behavior: when token_id is unknown the method returns a zero-valued Registration (all fields zero), not a panic or Option; mirror the explicit "0 if not registered" clarity used in the preceding methods so callers (e.g., consumers of Registration) can detect and handle a zero Registration appropriately.packages/metagame/src/registration/tests/test_registration_component.cairo (1)
383-442: Consider two additional test cases to lock in semantics.The added tests cover the documented paths well. Two gaps worth filling so future refactors can't quietly regress behavior the interface contract relies on:
- Unregistered-token full lookup —
test_token_reverse_lookups_default_zerochecks the two scalar getters but notget_entry_by_token(0xDEAD). Asserting it returns an all-zeroRegistration(or a panic, depending on which direction you take the doc fix inregistration_store.cairo) pins down the contract for downstream consumers.- Reverse-index after overwrite —
test_overwrite_entryswapsgame_token_idfrom0x7777→0x8888at the same slot but doesn't check whatget_entry_by_token(0x7777)returns afterwards. This is the assertion that surfaces the stale-reverse-index issue flagged onregistration_store.cairo::set_entry.🧪 Sketch
#[test] fn test_get_entry_by_token_unregistered_returns_zero() { let mock = deploy_mock(); let r = mock.get_entry_by_token(0xDEAD); assert!(r.context_id == 0 && r.entry_id == 0 && r.game_token_id == 0, "should be zero"); } #[test] fn test_overwrite_entry_clears_old_token_reverse_index() { let mock = deploy_mock(); mock.set_entry(make_registration(10, 1, 0x7777, false, false)); mock.set_entry(make_registration(10, 1, 0x8888, false, false)); // Old token should no longer resolve to this slot assert!(mock.get_token_context(0x7777) == 0, "old token should be cleared"); assert!(mock.get_entry_id_for_token(0x7777) == 0, "old entry_id should be cleared"); }As per coding guidelines: "edge cases must be fuzzed" and "coverage must not decrease after changes".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/metagame/src/registration/tests/test_registration_component.cairo` around lines 383 - 442, Add two tests to lock behavior: (1) a test that calls mock.get_entry_by_token(0xDEAD) on a fresh deploy_mock() and asserts the returned Registration struct fields (context_id, entry_id, game_token_id) are all zero to pin the unregistered-token contract (use get_entry_by_token). (2) a test that sets an entry with game_token_id 0x7777 then sets the same slot with game_token_id 0x8888 and asserts mock.get_token_context(0x7777) and mock.get_entry_id_for_token(0x7777) are zero to ensure set_entry clears the old reverse-index (use set_entry and make_registration for the entries). Ensure both tests use deploy_mock(), make_registration, set_entry, get_entry_by_token, get_token_context, and get_entry_id_for_token.
🤖 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/registration/registration_store.cairo`:
- Around line 27-28: The doc for get_entry_by_token is wrong: the implementation
(via get_entry returning Registration with zeros) and
IRegistration::get_entry_by_token use "0 if not registered" semantics, so either
update the comment on get_entry_by_token to state it returns a zero-valued
Registration when token_id is unregistered (matching get_entry and tests like
test_token_reverse_lookups_default_zero and the IRegistration spec), or change
the implementation to assert a missing registration (e.g., assert
entry.context_id != 0 or entry.entry_id != 0 with a clear message) so it truly
panics; reference the get_entry_by_token declaration, the get_entry
implementation, the Registration struct, and increment_entry_count (which makes
entry_id start at 1) when making your change.
---
Nitpick comments:
In `@packages/interfaces/src/registration.cairo`:
- Around line 30-31: Update the docstring for get_entry_by_token to clearly
state its unregistered-token behavior: when token_id is unknown the method
returns a zero-valued Registration (all fields zero), not a panic or Option;
mirror the explicit "0 if not registered" clarity used in the preceding methods
so callers (e.g., consumers of Registration) can detect and handle a zero
Registration appropriately.
In `@packages/metagame/src/registration/tests/test_registration_component.cairo`:
- Around line 383-442: Add two tests to lock behavior: (1) a test that calls
mock.get_entry_by_token(0xDEAD) on a fresh deploy_mock() and asserts the
returned Registration struct fields (context_id, entry_id, game_token_id) are
all zero to pin the unregistered-token contract (use get_entry_by_token). (2) a
test that sets an entry with game_token_id 0x7777 then sets the same slot with
game_token_id 0x8888 and asserts mock.get_token_context(0x7777) and
mock.get_entry_id_for_token(0x7777) are zero to ensure set_entry clears the old
reverse-index (use set_entry and make_registration for the entries). Ensure both
tests use deploy_mock(), make_registration, set_entry, get_entry_by_token,
get_token_context, and get_entry_id_for_token.
🪄 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: 2e120e74-fd9e-419f-9a8a-aa64cad1e226
📒 Files selected for processing (5)
packages/interfaces/src/registration.cairopackages/metagame/src/registration/registration_component.cairopackages/metagame/src/registration/registration_store.cairopackages/metagame/src/registration/store.cairopackages/metagame/src/registration/tests/test_registration_component.cairo
Move get_token_context, get_entry_id_for_token, and get_entry_by_token off the public IRegistration trait. Consumers that embed RegistrationImpl were paying ABI dispatch overhead for these even when unused. Keep the reverse lookups available via the internal trait (_get_token_context, _get_entry_id_for_token) and expose them on the test mock contract as `#[external(v0)]` so tests can still call them. Drops the unused get_entry_by_token convenience composite — neither budokan nor bokendo's migration needed it (they always know their context_id from the call shape and only need entry_id resolution). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Claude finished @starknetdev's task —— View job Claude Review - General Engineering Reviewrun=24961901376 attempt=1 sha=0f8b10bb5e7661f147784eeb85f046f7640f4386 scope=general Todo List
Review ResultsNo issues found. The git diff command with the specified scope ( All modified files are within the packages directory:
Summary: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFO |
Codex Review - General Engineering ReviewReview process failed to complete. |
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Claude finished @starknetdev's task —— View job Claude Review - General Engineering Reviewrun=24963191871 attempt=1 sha=c3ccbee4591b691ebafcdf74727fc410a3bb651f scope=general Todo List
Review ResultsNo issues found. The git diff command with the specified scope ( All modified files are within the packages directory:
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=24963191871 attempt=1 sha=c3ccbee4591b691ebafcdf74727fc410a3bb651f scope=packages Todo List
Review FindingsCRITICAL Issues[CRITICAL] packages/metagame/src/registration/registration_store.cairo:47-48 - Stale reverse-index on entry overwrite fn set_entry(ref self: T, registration: @Registration) {
RegistrationValidationImpl::assert_valid_token_id(*registration.game_token_id);
// Clear previous token's reverse index if overwriting
let prior_token_id = self.get_token_id(*registration.context_id, *registration.entry_id);
if prior_token_id != 0 && prior_token_id != *registration.game_token_id {
self.set_token_context(prior_token_id, 0);
self.set_token_entry_id(prior_token_id, 0);
}
// Set new entry
self.set_token_id(*registration.context_id, *registration.entry_id, *registration.game_token_id);
let flags = RegistrationOperationsImpl::pack_flags(*registration.has_submitted, *registration.is_banned);
self.set_flags(*registration.context_id, *registration.entry_id, flags);
self.set_token_context(*registration.game_token_id, *registration.context_id);
self.set_token_entry_id(*registration.game_token_id, *registration.entry_id);
}HIGH Issues[HIGH] packages/interfaces/src/registration.cairo:missing - Interface not updated with new methods /// Get the context ID that owns a token (0 if not registered)
fn get_token_context(self: @TState, token_id: felt252) -> u64;
/// Get the entry ID for a token within its context (0 if not registered)
fn get_entry_id_for_token(self: @TState, token_id: felt252) -> u32;
/// Get the full registration entry for a token. Returns zero-valued Registration
/// (all fields 0/false) if the token is not registered.
fn get_entry_by_token(self: @TState, token_id: felt252) -> Registration;[HIGH] packages/metagame/src/registration/registration_component.cairo:104-126 - IRegistration implementation missing new methods fn get_token_context(self: @ComponentState<TContractState>, token_id: felt252) -> u64 {
Store::get_token_context(self, token_id)
}
fn get_entry_id_for_token(self: @ComponentState<TContractState>, token_id: felt252) -> u32 {
Store::get_token_entry_id(self, token_id)
}
fn get_entry_by_token(self: @ComponentState<TContractState>, token_id: felt252) -> Registration {
let context_id = Store::get_token_context(self, token_id);
let entry_id = Store::get_token_entry_id(self, token_id);
self.get_entry(context_id, entry_id)
}MEDIUM Issues[MEDIUM] packages/metagame/src/registration/registration_store.cairo:missing - Missing get_entry_by_token implementation /// Reverse lookup: get the full registration for a token. Returns zero-valued
/// Registration (all fields 0/false) if the token is not registered.
fn get_entry_by_token(self: @T, token_id: felt252) -> Registration;
// Implementation:
fn get_entry_by_token(self: @T, token_id: felt252) -> Registration {
let context_id = self.get_token_context(token_id);
let entry_id = self.get_token_entry_id(token_id);
self.get_entry(context_id, entry_id)
}MEDIUM Issues (Testing)[MEDIUM] packages/metagame/src/registration/tests/test_registration_component.cairo:274-290 - Missing test for reverse-index cleanup on overwrite #[test]
fn test_overwrite_entry() {
// ... existing code ...
// Verify old token reverse index is cleared
assert!(mock.get_token_context(0x7777) == 0, "old token context should be cleared");
assert!(mock.get_entry_id_for_token(0x7777) == 0, "old token entry_id should be cleared");
}LOW Issues[LOW] packages/metagame/src/registration/tests/test_registration_component.cairo:383-416 - Missing test for unregistered token full lookup Architecture ReviewStorage Layout: ✅ Efficient use of single-felt keys instead of tuples SummarySummary: 1 CRITICAL, 2 HIGH, 2 MEDIUM, 1 LOW, 0 INFO The core reverse-index implementation is sound, but critical data integrity and API completeness issues prevent this from being production-ready. The stale reverse-index bug could cause silent data corruption in downstream consumers. The missing interface methods break the promised public API. |
Codex Review - Cairo/Starknet Contract ReviewReview process failed to complete. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/metagame/src/registration/tests/test_registration_component.cairo (1)
274-290: Add reverse-lookup assertions to the overwrite scenario.
test_overwrite_entryis the natural place to pin down the reverse-index behavior on overwrite — currently the new tests only cover fresh inserts and disjoint tokens, which is exactly the case where the stale-mapping issue (see the component-level comment) does not surface. Whatever the intended semantics end up being, please encode them here so the contract is locked in by tests.♻️ Example assertions to add after the second
set_entrylet retrieved = mock.get_entry(context_id, entry_id); assert!(retrieved.game_token_id == 0x8888, "game_token_id should be overwritten"); assert!(retrieved.has_submitted, "has_submitted should be overwritten"); assert!(retrieved.is_banned, "is_banned should be overwritten"); + + // New token resolves to the slot. + assert!(mock.get_token_context(0x8888) == context_id, "new token context"); + assert!(mock.get_entry_id_for_token(0x8888) == entry_id, "new token entry_id"); + // Previous token must no longer claim this slot. + assert!(mock.get_token_context(0x7777) == 0, "old token context should be cleared"); + assert!(mock.get_entry_id_for_token(0x7777) == 0, "old token entry_id should be cleared");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/metagame/src/registration/tests/test_registration_component.cairo` around lines 274 - 290, In test_overwrite_entry, after the second mock.set_entry(reg2) add assertions that verify the reverse-index was updated: call the contract's reverse-lookup helper used elsewhere in tests (the mock method that fetches a registration/context by game_token_id) to assert that the new token 0x8888 maps to the updated entry (matching retrieved) and that the old token 0x7777 no longer maps to the entry (returns empty/zero/default or a not-found result); ensure you use the same mock.reverse-lookup method used in other tests so the stale-mapping behavior on overwrite is explicitly asserted.
🤖 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/registration/registration_component.cairo`:
- Around line 80-82: get_token_context currently returns 0 for "not registered",
but context_id == 0 is not reserved so a real context 0 would be
indistinguishable; fix by making the component contract reserve 0 as invalid or
make the API explicit: either assert context_id != 0 in the mutators that write
Registration_token_context (e.g., set_entry and increment_entry_count) so no
stored context can be 0, or change get_token_context to return Option<u64> (None
for not-registered) and update callers accordingly; reference
Registration_token_context, get_token_context, set_entry, and
increment_entry_count when applying the change.
- Around line 33-36: set_entry currently overwrites the slot's token_id without
clearing the previous token's reverse mappings, leaving stale entries in
Registration_token_context and Registration_token_entry_id; update set_entry in
registration_store.cairo to read the previous token via get_token_id(context_id,
entry_id), and if prev_token != 0 and prev_token != incoming game_token_id, call
set_token_context(prev_token, 0) and set_token_entry_id(prev_token, 0) before
writing the new token (via set_token_id) and the new reverse mappings
(set_token_context and set_token_entry_id); alternatively, if you choose not to
clear, add documentation to the reverse-index invariants and ensure callers
re-verify the slot contents.
---
Nitpick comments:
In `@packages/metagame/src/registration/tests/test_registration_component.cairo`:
- Around line 274-290: In test_overwrite_entry, after the second
mock.set_entry(reg2) add assertions that verify the reverse-index was updated:
call the contract's reverse-lookup helper used elsewhere in tests (the mock
method that fetches a registration/context by game_token_id) to assert that the
new token 0x8888 maps to the updated entry (matching retrieved) and that the old
token 0x7777 no longer maps to the entry (returns empty/zero/default or a
not-found result); ensure you use the same mock.reverse-lookup method used in
other tests so the stale-mapping behavior on overwrite is explicitly asserted.
🪄 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: 4dff6dea-d240-42c0-abd4-c2299b7fd5ce
📒 Files selected for processing (5)
packages/metagame/src/registration/registration_component.cairopackages/metagame/src/registration/registration_store.cairopackages/metagame/src/registration/store.cairopackages/metagame/src/registration/tests/mocks/registration_mock.cairopackages/metagame/src/registration/tests/test_registration_component.cairo
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/metagame/src/registration/registration_store.cairo
- packages/metagame/src/registration/store.cairo
| fn get_token_context(self: @ComponentState<TContractState>, token_id: felt252) -> u64 { | ||
| self.Registration_token_context.entry(token_id).read() | ||
| } |
There was a problem hiding this comment.
Sentinel ambiguity: 0 for "not registered" collides with context_id == 0.
The doc on Lines 12–13 and the implementation here treat 0 as "not registered". entry_id == 0 is safe (IDs start at 1 via increment_entry_count), but nothing in the surrounding code reserves context_id == 0. If a caller ever uses context 0, get_token_context becomes indistinguishable from the unregistered state. Either reserve 0 explicitly in the component contract (assert context_id != 0 in set_entry/increment_entry_count) or change the API to return Option<u64>.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/metagame/src/registration/registration_component.cairo` around lines
80 - 82, get_token_context currently returns 0 for "not registered", but
context_id == 0 is not reserved so a real context 0 would be indistinguishable;
fix by making the component contract reserve 0 as invalid or make the API
explicit: either assert context_id != 0 in the mutators that write
Registration_token_context (e.g., set_entry and increment_entry_count) so no
stored context can be 0, or change get_token_context to return Option<u64> (None
for not-registered) and update callers accordingly; reference
Registration_token_context, get_token_context, set_entry, and
increment_entry_count when applying the change.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
set_entry now reads the token currently stored at (context_id, entry_id) before writing. If a different token previously held the slot, its reverse-index entries (Registration_token_context and Registration_token_entry_id) are zeroed so the displaced token no longer claims the slot via _get_token_context / _get_entry_id_for_token. Adds reverse-lookup assertions to test_overwrite_entry pinning the new behavior: the incoming token resolves to the slot, the displaced token's reverse maps return 0. Documents the existing context_id >= 1 convention on the component storage docstring (the 0 sentinel for "not registered" is consistent with Registration_entry_counts and matches downstream consumer identifier ranges in budokan and bokendo). Addresses review feedback on PR #107 from coderabbitai (major) and gemini-code-assist (high). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex Review - General Engineering ReviewReview process failed to complete. |
|
Claude finished @starknetdev's task —— View job Claude Review - General Engineering Reviewrun=24963437590 attempt=1 sha=8bac9705653acf0cf5c6fdb060cac6c75241d2a0 scope=general Todo List
Review ResultsNo issues found. The git diff command with the specified scope ( All modified files are within the packages directory:
Summary: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFO |
|
Pushed SRC5 interface ID question (asked offline): No update needed. Stale reverse-index on overwrite (coderabbitai 🟠 Major / gemini-code-assist high) — FIXED in
|
|
Claude finished @starknetdev's task —— View job Claude Review - Cairo/Starknet Contract Reviewrun=24963437590 attempt=1 sha=8bac9705653acf0cf5c6fdb060cac6c75241d2a0 scope=packages Todo List
Review FindingsThe implementation correctly adds reverse-index functionality to allow O(1) token → entry lookups. The stale reverse-index cleanup logic in Security: ✅ No vulnerabilities found. Access control preserved, no arithmetic issues, proper validation maintained. Cairo Idioms: ✅ Proper component patterns, storage isolation, trait implementations follow established conventions. Testing: ✅ Comprehensive test coverage including reverse lookup edge cases, overwrite scenarios, and isolation tests. All critical paths tested. Gas Optimization: ✅ Efficient storage layout with single-felt252 keys, minimal storage overhead (+2 SSTOREs per registration). Code Quality: ✅ Clear documentation, proper error handling, consistent with existing codebase patterns. Architecture ReviewStorage Layout: ✅ Efficient use of single-felt keys instead of tuples SummarySummary: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFO No issues found. The implementation is production-ready with solid security properties, comprehensive test coverage, and efficient gas usage. |
Codex Review - Cairo/Starknet Contract ReviewReview process failed to complete. |
…#108) * refactor(registration)!: key flags by token_id, drop entry_id reverse Re-key Registration_flags from Map<(u64, u32), u8> to Map<felt252, u8>. Each token belongs to exactly one (context_id, entry_id) slot within a component instance, so flags can live directly under the token. This collapses the two-step `token -> entry_id -> flags` lookup that PR #107 introduced into a single `token -> flags` lookup. Storage: - DROP Registration_token_entry_id (no longer needed) - REKEY Registration_flags to Map<felt252, u8> - Registration_token_context kept (drives has_context) - Registration_token_ids kept (forward enumeration) Public IRegistration trait (BREAKING): - Renames is_entry_banned(context_id, entry_id) -> is_token_banned(token_id) - Other public methods unchanged Internal trait: - DROP _get_entry_id_for_token, mark_entry_submitted, ban_entry - ADD mark_token_submitted, ban_token, _is_token_submitted, _is_token_banned - _get_token_context preserved - The new _is_token_submitted lets bokendo's _is_token_completed shrink from 3 storage hops to 1. set_entry simplifies: only one reverse map (token_context) to write, and the stale-clear-on-overwrite logic only zeros that one map. get_entry now performs a forward-map lookup followed by token-keyed flag read (2 SLOADs at enumeration time, acceptable for a rare path). Net effect for consumers: 1 fewer SSTORE per registration, 1 fewer SLOAD per submit/ban/refund hot path, less component bytecode embedded into each consumer. This is a breaking interface change — bump to v1.2.0 on tag. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * perf(registration): pack token state into a single storage slot Combine context_id (u64) and the two flag bits into a single packed felt252 stored under one Map<felt252, felt252>, replacing the prior pair of Registration_flags + Registration_token_context maps. Bit layout (low → high): bits 0..63: context_id bit 64: has_submitted bit 65: is_banned Net effect: - set_entry: 3 SSTOREs → 2 SSTOREs (token_id + token_state). - Overwrite-clear: zeroing the packed slot atomically clears both context and flags for the displaced token. - Combined queries (e.g. bokendo's _is_token_completed) collapse to a single SLOAD via per-field unpack helpers. Per-field unpack helpers `unpack_token_context_id`, `unpack_token_has_submitted`, `unpack_token_is_banned` mirror the pattern used by entry_fee/structs.cairo — callers reading a single field skip the full struct rebuild. Store trait surface shrinks: dropped get/set_flags and get/set_token_context, replaced by get/set_token_state_raw -> felt252. The bridge in registration_store.cairo absorbs the pack/unpack. 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
RegistrationComponentso consumers can resolve token → entry without iterating:Registration_token_context: Map<felt252, u64>andRegistration_token_entry_id: Map<felt252, u32>.RegistrationStoreImpl::set_entry— no new write paths or component-level API changes for existing callers.IRegistration:get_token_context,get_entry_id_for_token, and the convenienceget_entry_by_token(composes both reverse reads with the existing forwardget_entry).Map<felt252, …>) rather than the budokan-style tuple — tokens are globally unique within a component instance, so the second key element is redundant. Strictly more storage-efficient.Why
Budokan and bokendo each duplicate this state today (
token_to_entry/token_context_idandtoken_entry/token_questrespectively). Lifting it into the component lets both contracts drop their copies, and any future consumer gets the same lookup behavior for free.Cost is +2 SSTOREs per registration, which is exactly what those consumers are paying today — net-zero gas, single source of truth.
Test plan
snforge test test_registration— 24/24 pass (20 existing + 4 new: default-zero, after-set roundtrip, flag-update reflection, multi-token isolation)snforge testfor the metagame package — 419/419 passbudokanandbokendobuilds remain green (additive interface change; no breakage)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests