refactor: remove all_tokens enumeration from EnumerableComponent#91
Conversation
Remove global token enumeration (total_supply, token_by_index) to save 2 storage writes per mint. Only owner-based enumeration remains via IEnumerableOwner (token_of_owner_by_index, all_tokens_of_owner). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR modifies the ERC721 enumerable extension to replace the standard Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
|
Claude finished @starknetdev's task —— View job Claude Review - General Engineering ReviewTodo List
run=24041056419 attempt=1 sha=53627afe700503ce24f28ba8c7dfcc8a38bcbed7 scope=general Review ResultsNo issues found. This PR contains changes only within the
All changes are limited to Cairo contract code within the packages directory, which falls outside the scope of this general engineering review. Summary: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFO |
|
Claude finished @starknetdev's task —— View job Claude Review - Cairo/Starknet Contract Reviewrun=24041056419 attempt=1 sha=53627afe700503ce24f28ba8c7dfcc8a38bcbed7 scope=packages Todo List
Review Findings[MEDIUM] packages/embeddable_game_standard/src/token/extensions/enumerable/enumerable.cairo:114-116 - Potential underflow in removal logic [LOW] packages/embeddable_game_standard/src/token/extensions/enumerable/enumerable.cairo:76 - Conversion could panic [LOW] packages/embeddable_game_standard/src/token/extensions/enumerable/enumerable.cairo:50 - Conversion could panic [INFO] packages/embeddable_game_standard/src/token/tests/test_enumerable.cairo:175 - Test coverage reduced Positive Observations✅ Security: Burn protection is correctly maintained with Interface ID VerificationThe new interface ID follows established patterns: /// EFS: token_of_owner_by_index(ContractAddress,(u128,u128))->(u128,u128)
pub const IENUMERABLE_OWNER_ID: felt252 = 0x312c74a3a4f7aaf9aa3e80ddea171f958139ef0c3dbea524e0763682b7d57dd;The EFS signature correctly represents Test Coverage AssessmentThe test removals are appropriate given the functionality removal. Tests properly cover:
Missing: Direct storage verification for edge cases around empty balances. Summary1 MEDIUM potential underflow requiring attention The refactor successfully achieves its gas optimization goals while maintaining security properties. The main concern is the potential underflow in the removal enumeration logic that should be addressed for robustness. |
Codex Review - General Engineering ReviewNo issues found. |
![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 EnumerableComponent to optimize gas usage by removing global token enumeration and focusing on owner-based enumeration. It introduces a new IEnumerableOwner interface and updates the component's storage and logic accordingly. Feedback identifies that the IENUMERABLE_OWNER_ID does not follow the SRC5 standard for interface identification, which requires the ID to be the XOR of the function selectors.
| pub const IENUMERABLE_OWNER_ID: felt252 = | ||
| 0x312c74a3a4f7aaf9aa3e80ddea171f958139ef0c3dbea524e0763682b7d57dd; |
There was a problem hiding this comment.
The interface ID 0x312c74a3a4f7aaf9aa3e80ddea171f958139ef0c3dbea524e0763682b7d57dd does not follow the SRC5 (and ERC165) standard, which defines the interface ID as the XOR of all function selectors in the trait. For a single-function trait like IEnumerableOwner, the ID should be exactly the selector of token_of_owner_by_index. The current value seems to be the hash of the trait name IEnumerableOwner, which will break standard-compliant interface discovery.
pub const IENUMERABLE_OWNER_ID: felt252 =
0x02e30a6aa3d393d8d117333e03280b639194efc4480276dad4d23d53f2f9131;
Codex Review - Cairo/Starknet Contract ReviewNo issues found. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/embeddable_game_standard/src/token/tests/test_enumerable.cairo (2)
61-69: Assert that the legacy enumerable ID is no longer advertised.This only proves
IENUMERABLE_OWNER_IDis present. Add a negative assertion for the oldIERC721Enumerableinterface ID too, otherwise a regression could still report support for removed selectors viasupports_interface.As per coding guidelines "Use SRC5 interface discovery (
supports_interface) for cross-contract capability detection".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/embeddable_game_standard/src/token/tests/test_enumerable.cairo` around lines 61 - 69, The test_initializer currently only asserts the new IENUMERABLE_OWNER_ID and ISRC5_ID via mock_state.supports_interface; add a negative assertion to ensure the legacy IERC721 enumerable interface ID is not advertised (e.g., assert!(!mock_state.supports_interface(IERC721_ENUMERABLE_ID))); update the test function test_initializer in the same file (using COMPONENT_STATE(), CONTRACT_STATE(), and supports_interface) to include this negative check so regressions cannot reintroduce the old IERC721Enumerable selectors.
174-192: Add coverage for the self-transfer no-op path.Line 78 in
packages/embeddable_game_standard/src/token/extensions/enumerable/enumerable.caironow skips both remove/add whenprevious_owner == to, but none of these tests exercise that branch. A regression there would silently reorder or duplicate owner slots.🧪 Suggested regression test
+#[test] +fn test_before_update_when_transfer_to_same_owner() { + let (mut state, _) = setup(); + + state.before_update(OWNER(), TOKEN_1); + + assert_owned_tokens_list_after_update( + OWNER(), array![TOKEN_1, TOKEN_2, TOKEN_3].span(), + ); +}As per coding guidelines "Achieve and maintain 90% line coverage for tests; edge cases must be fuzzed".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/embeddable_game_standard/src/token/tests/test_enumerable.cairo` around lines 174 - 192, Add a test that covers the self-transfer no-op branch by calling state.before_update with to equal to the token's current owner so neither remove nor add runs; in test_enumerable.cairo create a test (e.g., test_before_update_self_transfer_noop) that sets up state, captures the owned lists, calls state.before_update(OWNER(), TOKEN_2) where OWNER() is the current owner, and then asserts the owner's and recipient's owned token lists remain exactly unchanged, verifying the branch guarded by the previous_owner == to check in enumerable.cairo is exercised.packages/embeddable_game_standard/src/token/extensions/enumerable/interface.cairo (1)
7-10: Document the owner-only ABI contract.
IEnumerableOwnernow replaces the standard enumerable interface, buttoken_of_owner_by_indexhas no docstring describing the owner-only scope, parameter constraints, or return semantics. Add function-level docs here so integrators do not assume fullIERC721Enumerablebehavior.As per coding guidelines "Every function must include clear explanation of what it does and why, parameter descriptions with types and constraints, return value documentation, and example usage when appropriate".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/embeddable_game_standard/src/token/extensions/enumerable/interface.cairo` around lines 7 - 10, Add a detailed function-level docstring for the IEnumerableOwner trait's token_of_owner_by_index method: state that IEnumerableOwner is an owner-only ABI (not full IERC721Enumerable), describe token_of_owner_by_index(self: `@TState`, owner: ContractAddress, index: u256) -> u256 semantics (index is zero-based and must be < owner balance, index and returned token id are u256, owner must be a valid ContractAddress/non-zero), specify failure behavior (e.g., revert or error if index out of bounds or owner has no tokens), clarify that the return value is the token ID owned by owner at that index, and add a short example of usage showing how to iterate owner tokens by incrementing index until out-of-bounds error or balance limit; attach this docstring directly above the token_of_owner_by_index declaration in the IEnumerableOwner trait.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/embeddable_game_standard/src/token/extensions/enumerable/interface.cairo`:
- Around line 7-10: Add a detailed function-level docstring for the
IEnumerableOwner trait's token_of_owner_by_index method: state that
IEnumerableOwner is an owner-only ABI (not full IERC721Enumerable), describe
token_of_owner_by_index(self: `@TState`, owner: ContractAddress, index: u256) ->
u256 semantics (index is zero-based and must be < owner balance, index and
returned token id are u256, owner must be a valid ContractAddress/non-zero),
specify failure behavior (e.g., revert or error if index out of bounds or owner
has no tokens), clarify that the return value is the token ID owned by owner at
that index, and add a short example of usage showing how to iterate owner tokens
by incrementing index until out-of-bounds error or balance limit; attach this
docstring directly above the token_of_owner_by_index declaration in the
IEnumerableOwner trait.
In `@packages/embeddable_game_standard/src/token/tests/test_enumerable.cairo`:
- Around line 61-69: The test_initializer currently only asserts the new
IENUMERABLE_OWNER_ID and ISRC5_ID via mock_state.supports_interface; add a
negative assertion to ensure the legacy IERC721 enumerable interface ID is not
advertised (e.g.,
assert!(!mock_state.supports_interface(IERC721_ENUMERABLE_ID))); update the test
function test_initializer in the same file (using COMPONENT_STATE(),
CONTRACT_STATE(), and supports_interface) to include this negative check so
regressions cannot reintroduce the old IERC721Enumerable selectors.
- Around line 174-192: Add a test that covers the self-transfer no-op branch by
calling state.before_update with to equal to the token's current owner so
neither remove nor add runs; in test_enumerable.cairo create a test (e.g.,
test_before_update_self_transfer_noop) that sets up state, captures the owned
lists, calls state.before_update(OWNER(), TOKEN_2) where OWNER() is the current
owner, and then asserts the owner's and recipient's owned token lists remain
exactly unchanged, verifying the branch guarded by the previous_owner == to
check in enumerable.cairo is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e6690c5f-7bfe-4e0d-bcf9-5cbbf4789990
📒 Files selected for processing (4)
packages/embeddable_game_standard/src/token/extensions/enumerable/enumerable.cairopackages/embeddable_game_standard/src/token/extensions/enumerable/interface.cairopackages/embeddable_game_standard/src/token/tests/test_enumerable.cairopackages/interfaces/src/token/enumerable.cairo
💤 Files with no reviewable changes (1)
- packages/interfaces/src/token/enumerable.cairo
1 participant
Summary
total_supply,token_by_index,Enumerable_all_tokens,Enumerable_all_tokens_len) fromEnumerableComponentIERC721Enumerableinterface with customIEnumerableOwner(onlytoken_of_owner_by_index)IENUMERABLE_OWNER_IDpackages/interfaces/src/token/enumerable.cairo(was comment-only, not declared as a module)Motivation
Saves 2 storage writes per mint (
Enumerable_all_tokensmap write +Enumerable_all_tokens_lenincrement). On-chain owner-based enumeration is more valuable than global enumeration, which can be served by an indexer/API.Breaking changes
total_supply()andtoken_by_index()no longer exist on contracts usingEnumerableComponentIERC721_ENUMERABLE_IDtoIENUMERABLE_OWNER_IDIERC721EnumerableDispatcherfortoken_of_owner_by_indexshould switch toIEnumerableOwnerDispatcherTest plan
snforge test enumerable)🤖 Generated with Claude Code
Summary by CodeRabbit
total_supplyand global token-by-index queries are no longer available.