Add owner enumeration with packed token IDs - #14
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an owner-level enumeration feature to the Beasts ERC721 contract while keeping token IDs as packed PackableBeast payloads, and removes the Beasts-specific total_supply() API in favor of owner enumeration + balance_of.
Changes:
- Introduce a new
EnumerableComponentthat trackstoken_of_owner_by_index(owner, index)backed byfelt252storage and registers a Beasts-specific SRC5 interface ID. - Update genesis minting and tests to use packed token IDs consistently and to validate owner enumeration behavior across mint + transfers.
- Remove Beasts
total_supply()/supply_countusage and update docs to reflect owner enumeration as the supported supply-style query mechanism.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/enumerable.cairo | New owner-enumeration component storing per-owner token lists and index mappings. |
| src/lib.cairo | Wires the enumerable component into the contract (storage, hooks, initializer, SRC5 exposure) and removes supply tracking. |
| src/interfaces.cairo | Adds Beasts-specific owner-enumerable interface + SRC5 ID; removes total_supply() from IBeasts. |
| src/mint_tests.cairo | Adds tests for genesis enumeration, transfer swap-fill behavior, OOB panics, and SRC5 support; updates packed-ID expectations. |
| src/minting_coordinator.cairo | Simplifies genesis batch prep to return beast IDs instead of mint payloads; removes supply helper. |
| src/tests.cairo | Updates OpenZeppelin interface imports and loosens token URI assertions to validate JSON data URI output. |
| src/pack.cairo | Minor docstring update clarifying “compact deterministic token id”. |
| src/beast_ranking.cairo | Updates tests to stop relying on removed total_supply() and use species counts instead. |
| README.md | Documents packed token IDs + owner enumeration, and removal of Beasts total_supply(). |
| AGENTS.md | Updates contributor guidance for packed IDs, uniqueness behavior, and owner enumeration usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| component!(path: ERC2981Component, storage: erc2981, event: ERC2981Event); | ||
| component!(path: VotesComponent, storage: erc721_votes, event: ERC721VotesEvent); | ||
| component!(path: NoncesComponent, storage: nonces, event: NoncesEvent); | ||
| component!(path: EnumerableComponent, storage: erc721_enumerable, event: EnumerableEvent); |
![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 a custom owner-level enumeration component for Beasts NFTs, replacing the global total_supply() functionality. Key changes include the addition of EnumerableComponent, updates to the IBeasts interface, and ensuring genesis beasts are recorded in the minted uniqueness map to prevent duplicates. Review feedback highlighted a potential underflow risk in _remove_token_from_owner_enumeration if the underlying ERC721 component state becomes inconsistent, though it is currently considered safe due to the contract's deterministic nature.
| let last_token_index: felt252 = (erc721_component.balance_of(from) - 1) | ||
| .try_into() | ||
| .unwrap(); |
There was a problem hiding this comment.
The calculation of last_token_index using balance_of(from) - 1 is safe here because this function is only called when previous_owner is not zero, ensuring a positive balance. However, if the ERC721 component's state were somehow inconsistent, this could underflow a u256 and then fail the try_into::<felt252>() check. Given the deterministic nature of this contract, it's acceptable, but worth noting as a dependency on the underlying component's correctness.
Standard ERC721 cannot answer "which tokens does this address hold", which is the one lookup every wallet-facing client needs. Because a token ID *is* the Beast — species, affixes, tier, type and variant flags are all encoded — one enumeration call per token reconstructs a wallet's whole collection with no further chain reads. Deliberately narrower than OpenZeppelin's ERC721Enumerable, which also maintains a global token list at five storage writes per mint instead of two. A global index buys little here: token IDs are derived rather than sequential, the collection is unbounded by design, and total_supply is already tracked directly. Token IDs are stored as felt252 rather than u256. They occupy at most 116 bits, so a felt holds one whole and halves what each index entry costs. Burning is rejected rather than handled: Beasts are not burnable, and refusing it is what lets the index stay dense without a tombstone scheme. Rebased onto current main — the original branch predated the 116-bit token IDs and the Votes/Nonces removal, so the hook now attaches to a fresh ERC721HooksTrait impl rather than the old voting hook. Unlike that branch, this keeps supply_count and total_supply: they are public collection surface that marketplaces read, and the lite index does not replace them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bb29048 to
3dfae40
Compare
GPT Code Reviewlgtm |
Summary
Test Notes
Review