refactor(registration)!: key flags by token_id, drop entry_id reverse#108
refactor(registration)!: key flags by token_id, drop entry_id reverse#108starknetdev merged 2 commits intomainfrom
Conversation
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>
📝 WalkthroughWalkthroughThe registration system's API is refactored from entry-based operations (using context_id + entry_id pairs) to token-based operations (using token_id). Storage consolidates from per-entry flags and reverse-index maps into a single packed token state field, eliminating separate reverse-lookup methods. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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=24964205766 attempt=1 sha=dce354296029d59d935d8571eb4e755cde3be1e1 scope=general Todo List
Review ResultsNo issues found. This PR contains only changes within the
No changes were made to CI/CD configuration, documentation, tooling, or scripts outside 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=24964205766 attempt=1 sha=dce354296029d59d935d8571eb4e755cde3be1e1 scope=packages Review Progress
Security Analysis[LOW] packages/metagame/src/registration/registration_store.cairo:44-46 - Token displacement cleanup only clears context mapping Gas Optimization Verification✅ Claims validated:
API Breaking Changes Assessment✅ Breaking changes properly handled:
Test Coverage Analysis✅ Comprehensive test coverage:
Implementation Logic Review✅ Storage layout refactoring correctly implemented:
✅ Reverse index logic preserved:
Code Quality✅ Following Cairo idioms:
No issues found. Summary: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 1 LOW, 0 INFO |
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 refactors the registration system to key flags (submission and ban status) by token_id for O(1) access. Feedback suggests clearing flags upon token displacement to avoid stale data, optimizing flag reads for empty slots, and adding validation to prevent modification of the null token ID.
| if prev_token != 0 && prev_token != *registration.game_token_id { | ||
| self.set_token_context(prev_token, 0); | ||
| self.set_token_entry_id(prev_token, 0); | ||
| } |
There was a problem hiding this comment.
When a token is displaced from a slot, its context is cleared, but its flags (submission status and ban status) remain in storage keyed by the token_id. If a consumer relies on the "1-hop" check mentioned in the PR description (checking is_token_banned without verifying context), they might see stale data for unregistered tokens. Clearing the flags here ensures the 1-hop check remains safe and provides a gas refund for the storage cleanup.
if prev_token != 0 && prev_token != *registration.game_token_id {
self.set_token_context(prev_token, 0);
self.set_flags(prev_token, 0);
}
| fn get_entry(self: @T, context_id: u64, entry_id: u32) -> Registration { | ||
| let game_token_id = self.get_token_id(context_id, entry_id); | ||
| let flags = self.get_flags(context_id, entry_id); | ||
| let flags = self.get_flags(game_token_id); |
There was a problem hiding this comment.
When fetching an entry for an empty slot (game_token_id == 0), calling get_flags(0) results in an unnecessary storage read (SLOAD) that will always return 0. Since token_id 0 is reserved for empty slots and cannot be registered, we can optimize this by skipping the storage read.
let flags = if game_token_id == 0 { 0 } else { self.get_flags(game_token_id) };
| fn mark_token_submitted(ref self: T, token_id: felt252) { | ||
| let flags = self.get_flags(token_id); | ||
| self.set_flags(token_id, RegistrationOperationsImpl::set_submitted(flags)); | ||
| } |
There was a problem hiding this comment.
It is important to ensure that token_id is non-zero before marking it as submitted. If a consumer accidentally passes 0 (e.g., from an empty slot lookup), it would corrupt the flags for the "empty" state, causing all empty slots to appear as submitted in get_entry.
fn mark_token_submitted(ref self: T, token_id: felt252) {
RegistrationValidationImpl::assert_valid_token_id(token_id);
let flags = self.get_flags(token_id);
self.set_flags(token_id, RegistrationOperationsImpl::set_submitted(flags));
}
| fn ban_token(ref self: T, token_id: felt252) { | ||
| let flags = self.get_flags(token_id); | ||
| self.set_flags(token_id, RegistrationOperationsImpl::set_banned(flags)); | ||
| } |
There was a problem hiding this comment.
Similar to mark_token_submitted, we should validate that the token_id is non-zero to prevent banning the null token ID, which would affect the default state of all empty registration slots.
fn ban_token(ref self: T, token_id: felt252) {
RegistrationValidationImpl::assert_valid_token_id(token_id);
let flags = self.get_flags(token_id);
self.set_flags(token_id, RegistrationOperationsImpl::set_banned(flags));
}
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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>
|
Claude finished @starknetdev's task —— View job Claude Review - General Engineering Reviewrun=24964682974 attempt=1 sha=26fdc613f93412e982eeb1e5bd2435a583c85784 scope=general Todo List
Review ResultsNo issues found. This PR contains only changes within the
No changes were made to CI/CD configuration, documentation, tooling, or scripts outside 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=24964682974 attempt=1 sha=26fdc613f93412e982eeb1e5bd2435a583c85784 scope=packages Todo List
Security Analysis✅ No critical security vulnerabilities found The refactor correctly implements the token-keyed flag design:
Gas Optimization Verification✅ Claims validated:
API Breaking Changes Assessment✅ Breaking changes properly handled:
Test Coverage Analysis✅ Comprehensive test coverage with key addition:
Cairo Idiom Compliance✅ Follows Cairo best practices:
Storage Layout Analysis✅ Storage refactoring correctly implemented:
Code Quality Assessment✅ Implementation quality is high:
No issues found. Summary: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFO |
Codex Review - Cairo/Starknet Contract 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.
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_component.cairo (1)
135-165:⚠️ Potential issue | 🟡 MinorVerify internal callers of
mark_token_submitted/ban_tokenalways guard with registration check.These internal helpers do unconditional RMW on
Registration_token_state[token_id]. If invoked on a token that was never registered (context_id == 0), they'll persist{ context_id: 0, has_submitted/is_banned: true }. From that point on, the publicis_token_bannedwould incorrectly returntruefor an unregistered token.Since
mark_token_submittedandban_tokenare internal-only (part ofRegistrationInternalTrait), verify that all embedding contracts call them only on tokens with a valid registration context (context_id > 0). Either add an assertionassert!(state.context_id != 0, "token not registered")to both functions, or haveis_token_bannedreturnfalsewhencontext_id == 0.🤖 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 135 - 165, The internal helpers mark_token_submitted and ban_token perform unconditional RMW on Registration_token_state[token_id]; add a guard to both functions to prevent persisting state for unregistered tokens by asserting the token has a valid context_id before mutating: fetch the token's context via RegistrationStoreTrait::get_token_context(self, token_id) or existing state access and assert context_id != 0 (e.g., "token not registered") at the start of mark_token_submitted and ban_token in the ComponentState<TContractState> implementation; this ensures callers cannot accidentally mark or ban an unregistered token (alternatively, you may instead change RegistrationStoreTrait::is_token_banned to return false when context_id == 0, but prefer adding the assert in mark_token_submitted and ban_token to keep the internal-trait semantics strict).
🧹 Nitpick comments (4)
packages/metagame/src/registration/registration_store.cairo (1)
85-95: Optional: skip the unpack/pack round-trip on flag-only RMWs.
mark_token_submitted/ban_tokenonly need to OR a single bit. The unpack→mutate→pack form is readable but performs two extratry_intoround-trips per call. Since this is on the hot submit/ban path (a key motivation for the PR per the objectives), a raw-OR variant could shave a few steps:fn mark_token_submitted(ref self: T, token_id: felt252) { - let mut state = TokenStateStorePacking::unpack(self.get_token_state_raw(token_id)); - state.has_submitted = true; - self.set_token_state_raw(token_id, TokenStateStorePacking::pack(state)); + let packed: u128 = self.get_token_state_raw(token_id).try_into().unwrap(); + self.set_token_state_raw(token_id, (packed | 0x10000000000000000_u128).into()); } fn ban_token(ref self: T, token_id: felt252) { - let mut state = TokenStateStorePacking::unpack(self.get_token_state_raw(token_id)); - state.is_banned = true; - self.set_token_state_raw(token_id, TokenStateStorePacking::pack(state)); + let packed: u128 = self.get_token_state_raw(token_id).try_into().unwrap(); + self.set_token_state_raw(token_id, (packed | 0x20000000000000000_u128).into()); }If you go this route, expose
HAS_SUBMITTED_BIT/IS_BANNED_BITfromstructs.cairorather than duplicating the literals. Given the chill review preference, feel free to defer — current form is plenty readable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/metagame/src/registration/registration_store.cairo` around lines 85 - 95, mark_token_submitted and ban_token perform a full unpack/pack RMW but only need to set a single flag; replace the unpack→mutate→pack sequence in functions mark_token_submitted and ban_token with a raw bitwise-OR update on the stored packed value (read current raw via get_token_state_raw, OR with the appropriate flag bit, and write back via set_token_state_raw) to avoid try_into round-trips. Import and use the HAS_SUBMITTED_BIT and IS_BANNED_BIT constants from structs.cairo (do not duplicate literals) and ensure the OR uses the same packed layout as TokenStateStorePacking so semantics stay identical.packages/metagame/src/registration/structs.cairo (1)
76-83: Document the implicitpacked ≤ u128invariant onunpack.
value.try_into().unwrap()on the felt252 will panic if anyone ever writes a state value with bits above 127 intoRegistration_token_state. Today the only writer isTokenStateStorePacking::pack, which is bounded to u128, so this is safe. Worth a one-line comment so future contributors who want to use bits 66..127 (reserved per the layout) know they must keep the packed value within u128, otherwise existing reads will start reverting on every load.fn unpack(value: felt252) -> TokenState { + // Invariant: any writer to this slot must keep the packed value within u128. + // Reserved bits 66..127 are available; bit 128+ would break this unwrap. let packed: u128 = value.try_into().unwrap();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/metagame/src/registration/structs.cairo` around lines 76 - 83, The unpack function assumes the incoming felt252 fits into u128 and currently calls value.try_into().unwrap(), which will panic if high bits are set; add a one-line comment in or immediately above the unpack function documenting the implicit invariant "packed ≤ u128" and note that TokenStateStorePacking::pack is the current writer that enforces this bound so future contributors must preserve that constraint (or widen unpack) if they use bits above 127; reference the unpack function and TokenStateStorePacking::pack in the comment for clarity.packages/metagame/src/registration/tests/test_registration_component.cairo (1)
294-298: Refresh stale "reverse mapping" wording.The comment still references the removed reverse-index map. The assertion is correct because
set_entrynow zeroes the displaced token's full packed state (seeregistration_store.cairoline 51–56), soget_token_contextreturns 0. Suggest aligning the wording with the new mechanism:- // Previous token's reverse mapping must be cleared so it no longer claims the slot. + // Previous token's packed state must be zeroed so it no longer claims this slot. assert!(mock.get_token_context(0x7777) == 0, "old token context 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 294 - 298, Update the test comment to reflect that the displaced token's packed state is zeroed by set_entry (so get_token_context(0x7777) returns 0) rather than referring to the removed "reverse-index" map; mention set_entry and get_token_context in the comment to make the mechanism clear (e.g., "Previous token's packed state is cleared by set_entry, so get_token_context(...) returns 0").packages/metagame/src/registration/registration_component.cairo (1)
17-22: Update the "reverse index" wording — the reverse map no longer exists.The convention is correct (context_id == 0 means "not registered" via
_get_token_context), but the words "reverse index" referred to the removedRegistration_token_entry_id/Registration_token_contextmaps. With this PR the lookup is the low-64 bits of the packed token state inRegistration_token_state, so the comment is slightly misleading to a future reader./// Conventions: -/// - context_id is expected to be >= 1. `_get_token_context` treats 0 as -/// "not registered", so a caller using context_id == 0 cannot -/// distinguish a real context-zero registration from an unknown -/// token via the reverse index. entry_id is always >= 1 by -/// construction (see `increment_entry_count`). +/// - context_id is expected to be >= 1. `_get_token_context` reads the +/// low 64 bits of the packed token state and treats 0 as +/// "not registered", so a caller using context_id == 0 cannot +/// distinguish a real context-zero registration from an unknown +/// token. entry_id is always >= 1 by construction +/// (see `increment_entry_count`).🤖 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 17 - 22, The comment incorrectly refers to a "reverse index" that no longer exists; update the wording to state that `_get_token_context` treats `context_id == 0` as "not registered" and that the lookup now reads the low-64 bits of the packed token state stored in `Registration_token_state` rather than a separate reverse map. Modify the comment around `context_id`, `_get_token_context`, `entry_id`, and `increment_entry_count` to explain that `entry_id >= 1` and registration presence is determined by the low-64 bits of `Registration_token_state` (packed token state), removing any mention of `Registration_token_entry_id`/`Registration_token_context` or a "reverse index." Ensure the new phrasing is concise and accurate for future readers.
🤖 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/registration/registration_component.cairo`:
- Around line 135-165: The internal helpers mark_token_submitted and ban_token
perform unconditional RMW on Registration_token_state[token_id]; add a guard to
both functions to prevent persisting state for unregistered tokens by asserting
the token has a valid context_id before mutating: fetch the token's context via
RegistrationStoreTrait::get_token_context(self, token_id) or existing state
access and assert context_id != 0 (e.g., "token not registered") at the start of
mark_token_submitted and ban_token in the ComponentState<TContractState>
implementation; this ensures callers cannot accidentally mark or ban an
unregistered token (alternatively, you may instead change
RegistrationStoreTrait::is_token_banned to return false when context_id == 0,
but prefer adding the assert in mark_token_submitted and ban_token to keep the
internal-trait semantics strict).
---
Nitpick comments:
In `@packages/metagame/src/registration/registration_component.cairo`:
- Around line 17-22: The comment incorrectly refers to a "reverse index" that no
longer exists; update the wording to state that `_get_token_context` treats
`context_id == 0` as "not registered" and that the lookup now reads the low-64
bits of the packed token state stored in `Registration_token_state` rather than
a separate reverse map. Modify the comment around `context_id`,
`_get_token_context`, `entry_id`, and `increment_entry_count` to explain that
`entry_id >= 1` and registration presence is determined by the low-64 bits of
`Registration_token_state` (packed token state), removing any mention of
`Registration_token_entry_id`/`Registration_token_context` or a "reverse index."
Ensure the new phrasing is concise and accurate for future readers.
In `@packages/metagame/src/registration/registration_store.cairo`:
- Around line 85-95: mark_token_submitted and ban_token perform a full
unpack/pack RMW but only need to set a single flag; replace the
unpack→mutate→pack sequence in functions mark_token_submitted and ban_token with
a raw bitwise-OR update on the stored packed value (read current raw via
get_token_state_raw, OR with the appropriate flag bit, and write back via
set_token_state_raw) to avoid try_into round-trips. Import and use the
HAS_SUBMITTED_BIT and IS_BANNED_BIT constants from structs.cairo (do not
duplicate literals) and ensure the OR uses the same packed layout as
TokenStateStorePacking so semantics stay identical.
In `@packages/metagame/src/registration/structs.cairo`:
- Around line 76-83: The unpack function assumes the incoming felt252 fits into
u128 and currently calls value.try_into().unwrap(), which will panic if high
bits are set; add a one-line comment in or immediately above the unpack function
documenting the implicit invariant "packed ≤ u128" and note that
TokenStateStorePacking::pack is the current writer that enforces this bound so
future contributors must preserve that constraint (or widen unpack) if they use
bits above 127; reference the unpack function and TokenStateStorePacking::pack
in the comment for clarity.
In `@packages/metagame/src/registration/tests/test_registration_component.cairo`:
- Around line 294-298: Update the test comment to reflect that the displaced
token's packed state is zeroed by set_entry (so get_token_context(0x7777)
returns 0) rather than referring to the removed "reverse-index" map; mention
set_entry and get_token_context in the comment to make the mechanism clear
(e.g., "Previous token's packed state is cleared by set_entry, so
get_token_context(...) returns 0").
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67fcc77a-4c63-47c3-be72-55dc264e95b5
📒 Files selected for processing (7)
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/structs.cairopackages/metagame/src/registration/tests/mocks/registration_mock.cairopackages/metagame/src/registration/tests/test_registration_component.cairo
1 participant
Summary
Second pass on the registration component. PR #107 added a token → entry_id reverse index so consumers could resolve a token's slot in O(1). This PR observes that the only reason consumers need entry_id after the lookup is because `Registration_flags` is keyed by `(context_id, entry_id)`. Re-keying the flags map directly by `token_id` collapses the two-hop `token → entry_id → flags` chain into one hop and makes the entry_id reverse redundant.
Changes
Storage
Public `IRegistration` trait (BREAKING)
Internal trait
The new `_is_token_submitted` / `_is_token_banned` helpers let bokendo's `_is_token_completed` shrink from 3 storage hops to 1.
Why
Net per-consumer wins:
Indexing / data considerations
Verified by grepping consumer indexers and clients:
This affects the budokan and bokendo migrations (PRs to follow). No on-chain data migration is needed — both contracts will be redeployed fresh.
Test plan
Release
Follow-ups
🤖 Generated with Claude Code
Summary by CodeRabbit