Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces an ERC-1155 Supply Extension component that tracks token supplies across the ERC-1155 token standard. The implementation includes new interface definitions, a component with storage and hooks, macro support for component integration, mock implementations for testing, and comprehensive test coverage. Dependency versions are also updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Contract
participant ERC1155Component
participant ERC1155SupplyComponent
participant Storage
Contract->>ERC1155Component: transfer/mint/burn
activate ERC1155Component
ERC1155Component->>ERC1155Component: execute operation
ERC1155Component->>Contract: invoke after_update hook
deactivate ERC1155Component
activate Contract
Contract->>ERC1155SupplyComponent: after_update(from, to, token_ids, values)
deactivate Contract
activate ERC1155SupplyComponent
ERC1155SupplyComponent->>Storage: update total_supply[token_id]
ERC1155SupplyComponent->>Storage: update total_supply_all
deactivate ERC1155SupplyComponent
Contract->>ERC1155SupplyComponent: total_supply(token_id)
activate ERC1155SupplyComponent
ERC1155SupplyComponent->>Storage: read total_supply[token_id]
ERC1155SupplyComponent-->>Contract: u256 value
deactivate ERC1155SupplyComponent
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
🧪 Cairo Contract Size Benchmark DiffBYTECODE SIZE (felts) (limit: 81,920 felts)
SIERRA CONTRACT CLASS SIZE (bytes) (limit: 4,089,446 bytes)
This comment was generated automatically from benchmark diffs. |
![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
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Around line 19-21: Add a new entry under the "Added" section of the changelog
to document the ERC1155 supply extension: mention the new ERC1155SupplyComponent
and the IERC1155Supply interface (and optionally a short note about their
purpose, e.g., supply tracking for ERC‑1155 tokens) so users see these API
additions; ensure the entry follows existing changelog style and placement
conventions.
🧹 Nitpick comments (1)
packages/interfaces/src/token/erc1155.cairo (1)
45-50: Interface definition looks good.The
IERC1155Supplyinterface correctly mirrors the OpenZeppelin Solidity ERC1155Supply extension with appropriate snake_case naming.Consider defining an interface ID constant (e.g.,
IERC1155_SUPPLY_ID) similar to other interfaces in this file if consumers need to check for supply extension support via SRC5.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Scarb.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
CHANGELOG.mdScarb.tomlpackages/interfaces/src/token/erc1155.cairopackages/macros/src/attribute/with_components/components.rspackages/test_common/src/mocks/erc1155.cairopackages/testing/CHANGELOG.mdpackages/token/src/erc1155.cairopackages/token/src/erc1155/extensions.cairopackages/token/src/erc1155/extensions/erc1155_supply.cairopackages/token/src/tests/erc1155.cairopackages/token/src/tests/erc1155/test_erc1155_supply.cairo
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: ericnordelo
Repo: OpenZeppelin/cairo-contracts PR: 1630
File: packages/test_common/src/mocks/erc721.cairo:363-409
Timestamp: 2026-01-13T11:48:56.063Z
Learning: In Cairo contracts using the ERC721ConsecutiveComponent with the component! macro, the ERC721OwnerOfDefaultImpl import is not needed because ConsecutiveExtensionERC721OwnerOfTraitImpl is automatically used to provide custom owner_of logic for consecutive/batched tokens. This is different from regular ERC721 mocks that require the explicit ERC721OwnerOfDefaultImpl import.
📚 Learning: 2026-01-13T11:50:05.718Z
Learnt from: ericnordelo
Repo: OpenZeppelin/cairo-contracts PR: 1630
File: packages/token/src/erc721/erc721.cairo:25-26
Timestamp: 2026-01-13T11:50:05.718Z
Learning: In Cairo contracts, trait implementation imports must be brought into scope using an import like `use Component::InternalImpl as Alias` to enable method resolution, even if the alias name is not textually used in the code. Ensure such trait imports (e.g., for ERC721ConsecutiveComponent::InternalImpl and similar component trait imports) exist so component method calls on instances (e.g., via get_dep_component()) resolve correctly. In reviews, verify that relevant Cairo files include the necessary trait import aliases to bring implementations into scope, not just the concrete types or module paths.
Applied to files:
packages/interfaces/src/token/erc1155.cairopackages/token/src/erc1155/extensions/erc1155_supply.cairopackages/token/src/erc1155/extensions.cairopackages/test_common/src/mocks/erc1155.cairopackages/token/src/tests/erc1155/test_erc1155_supply.cairopackages/token/src/tests/erc1155.cairopackages/token/src/erc1155.cairo
📚 Learning: 2026-01-13T11:54:54.301Z
Learnt from: ericnordelo
Repo: OpenZeppelin/cairo-contracts PR: 1630
File: packages/token/src/erc721/extensions/erc721_consecutive.cairo:118-129
Timestamp: 2026-01-13T11:54:54.301Z
Learning: In Cairo 1.0 and later, all integer types (u8, u16, u32, u64, u128, u256, i8–i128) have built-in overflow and underflow protection. Arithmetic operations on these types automatically panic (revert) if they overflow or underflow, rather than wrapping. This means explicit overflow guards are not necessary for standard integer arithmetic in Cairo. Apply this broadly to Cairo code (including files like packages/token/src/erc721/extensions/erc721_consecutive.cairo); rely on language-provided overflow checks. If you need non-wrapping behavior, consider alternative logic or explicit checks only for exceptional cases, but generally avoid manual overflow guards for normal operations.
Applied to files:
packages/interfaces/src/token/erc1155.cairopackages/token/src/erc1155/extensions/erc1155_supply.cairopackages/token/src/erc1155/extensions.cairopackages/test_common/src/mocks/erc1155.cairopackages/token/src/tests/erc1155/test_erc1155_supply.cairopackages/token/src/tests/erc1155.cairopackages/token/src/erc1155.cairo
📚 Learning: 2026-01-13T11:48:56.063Z
Learnt from: ericnordelo
Repo: OpenZeppelin/cairo-contracts PR: 1630
File: packages/test_common/src/mocks/erc721.cairo:363-409
Timestamp: 2026-01-13T11:48:56.063Z
Learning: In Cairo contracts using the ERC721ConsecutiveComponent with the component! macro, the ERC721OwnerOfDefaultImpl import is not needed because ConsecutiveExtensionERC721OwnerOfTraitImpl is automatically used to provide custom owner_of logic for consecutive/batched tokens. This is different from regular ERC721 mocks that require the explicit ERC721OwnerOfDefaultImpl import.
Applied to files:
packages/token/src/erc1155/extensions/erc1155_supply.cairopackages/token/src/erc1155.cairo
📚 Learning: 2026-01-13T11:48:47.957Z
Learnt from: ericnordelo
Repo: OpenZeppelin/cairo-contracts PR: 1630
File: packages/test_common/src/mocks/erc721.cairo:363-409
Timestamp: 2026-01-13T11:48:47.957Z
Learning: When using ERC721ConsecutiveComponent with the component! macro in Cairo contracts, you do not need to import ERC721OwnerOfDefaultImpl in mocks under packages/test_common/src/mocks. The ConsecutiveExtensionERC721OwnerOfTraitImpl automatically provides the custom owner_of logic for consecutive/batched tokens, unlike regular ERC721 mocks that require the explicit ERC721OwnerOfDefaultImpl import. Apply this guidance to all Cairo mocks in the mocks directory that utilize ERC721ConsecutiveComponent.
Applied to files:
packages/test_common/src/mocks/erc1155.cairo
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: comment-benchmark-diff
- GitHub Check: Lint and test macros
- GitHub Check: Lint and test Cairo
🔇 Additional comments (17)
packages/testing/CHANGELOG.md (1)
10-12: LGTM!The changelog entry follows the established format and correctly documents the snforge version bump.
packages/token/src/erc1155/extensions.cairo (1)
1-2: LGTM!The module declaration and re-export follow the established pattern for extension components in this codebase.
Scarb.toml (1)
30-31: Version numbers appear to exceed officially released versions and require clarification.The Scarb.toml specifies Cairo 2.15.0 and Scarb 2.15.1, which are ahead of the latest officially released versions (Cairo 2.13.2 and Scarb 2.13.1 as of January 2026). Verify whether these are:
- Pre-release/development versions intentionally targeted
- Versions with documented compatibility guarantees
- Or if versions should be aligned with official releases
This needs to be confirmed before merging, as using non-standard versions could introduce stability or compatibility risks.
Likely an incorrect or invalid review comment.
packages/token/src/tests/erc1155.cairo (1)
3-3: LGTM!Test module declaration follows the existing pattern.
packages/token/src/erc1155.cairo (1)
3-3: LGTM!The extensions module and
ERC1155SupplyComponentre-export follow the established pattern for token extensions (similar to ERC721 extensions structure).Also applies to: 7-7
packages/token/src/tests/erc1155/test_erc1155_supply.cairo (3)
1-12: LGTM!Imports are correctly structured. The
ERC1155InternalImplalias import (line 11) follows the Cairo pattern where trait imports enable method resolution even if the alias isn't textually used. Based on learnings.
18-32: LGTM!Setup function correctly initializes the contract state and batch mints tokens, which properly triggers the
after_updatehook for supply tracking.
38-272: Excellent test coverage!The tests comprehensively cover all supply-tracking scenarios:
- Zero state verification
- Single and batch mint/burn operations
- Transfer operations (verifying supply unchanged)
- Partial burns
- Mixed operations
exists()semantics for never-minted and fully-burned tokensThe test logic aligns with the expected behavior of the ERC1155Supply extension per the Solidity reference implementation.
packages/macros/src/attribute/with_components/components.rs (3)
22-22: LGTM!Enum variant correctly placed in alphabetical order after
ERC1155.
61-61: LGTM!String parsing for the new component variant is correctly implemented.
227-235: LGTM!The
ComponentInfoconfiguration is correct:
has_initializer: falseis appropriate since supply tracking doesn't require initializationinternal_impls: vec!["InternalImpl"]correctly exposes theafter_updatehook- Path aligns with the actual module structure in
openzeppelin_token::erc1155::extensionspackages/test_common/src/mocks/erc1155.cairo (1)
152-200: LGTM! Well-structured mock for ERC1155Supply extension.The mock correctly wires the
after_updatehook to theERC1155SupplyComponentfor supply tracking. The pattern of only implementing the needed hook (after_update) while relying on defaults forbefore_updatefollows the extension pattern seen throughout the codebase, consistent with how other hooks are implemented (e.g.,ERC20VotesHooksImpl,ERC721VotesHooksImpl).packages/token/src/erc1155/extensions/erc1155_supply.cairo (5)
5-16: Clear and appropriate documentation.The warnings about upgrade constraints and hook requirements are well-articulated, which is essential for ensuring correct integration of this extension.
17-28: Imports are appropriate and necessary.All imported items are utilized in the implementation.
29-33: Storage structure is well-designed.The dual tracking approach (per-token and aggregate) aligns with the Solidity reference implementation and provides efficient access patterns for both query types.
35-57: Public interface implementation is correct.The read-only functions are straightforward and correctly implemented. The generic constraints properly enforce that this component can only be used with contracts implementing ERC1155Component.
75-105: Supply tracking logic is correctly implemented.The implementation properly handles:
- Mints (from=zero): Increases per-token and aggregate supply
- Burns (to=zero): Decreases per-token and aggregate supply
- Transfers (both non-zero): No supply change (neither block executes)
The optimization of accumulating values before a single aggregate write is a good pattern. Based on learnings, Cairo's built-in overflow/underflow protection will correctly revert if attempting to burn more tokens than exist.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1632 +/- ##
==========================================
+ Coverage 93.10% 93.74% +0.63%
==========================================
Files 87 88 +1
Lines 2364 2190 -174
==========================================
- Hits 2201 2053 -148
+ Misses 163 137 -26
... and 28 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| has_initializer: false, | ||
| has_immutable_config: false, | ||
| internal_impls: vec!["InternalImpl"], | ||
| }, |
There was a problem hiding this comment.
We may want to add an additional check in case ERC1155Supply is used (or any other component that has internal function that must be called in hooks), a check that ensures that the calls to these internal functions do exist in the implementing contract
There was a problem hiding this comment.
You are right. I'm going to address that in a separate PR where I also plan to enable the other components in macros (like the wrappers). I haven't gone deep into that because there's also an issue with the span matching to address in the macro implementation,
2 participants
Fixes #1574
PR Checklist
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.