with_components support for ERC721Consecutive#1660
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request adds support for the ERC721Consecutive component extension to the macro system by introducing a new AllowedComponents variant, wiring it through deserialization and component registry, adding validation logic for required hooks, and including comprehensive tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/macros/src/attribute/with_components/components.rs`:
- Around line 218-225: The ERC721Consecutive ComponentInfo entry uses a path
that omits the erc721_consecutive submodule, causing immutable-config validation
to generate a bad DefaultConfig import; update the ComponentInfo for
AllowedComponents::ERC721Consecutive (the entry with name
"ERC721ConsecutiveComponent" and path currently
"openzeppelin_token::erc721::extensions::ERC721ConsecutiveComponent") so its
path includes the erc721_consecutive submodule (so the immutable-config logic
will resolve DefaultConfig as
"openzeppelin_token::erc721::extensions::erc721_consecutive::DefaultConfig") —
adjust the `path` string in that ComponentInfo accordingly.
In `@packages/macros/src/attribute/with_components/parser.rs`:
- Around line 153-160: The hard-fail check in uses_erc721_enumerable /
uses_erc721_consecutive is too broad because body_code.contains(...) matches
comments/strings; change the secondary check to detect real component usage
patterns instead (e.g., look for the component! macro or concrete symbol tokens)
rather than raw substrings: update the condition that currently uses
body_code.contains("ERC721Enumerable") / body_code.contains("ERC721Consecutive")
to a scoped pattern match (for example a regex or simple string scan that
recognizes "component!(" with an ERC721... identifier, or better, inspect the
parsed tokens/AST for Ident/component! invocation) so only true component usage
flips the flag while leaving components_info.iter().any(|c| matches!(c.kind(),
AllowedComponents::...)) as is; reference functions/variables:
uses_erc721_enumerable, uses_erc721_consecutive, components_info,
AllowedComponents::ERC721Enumerable, AllowedComponents::ERC721Consecutive, and
body_code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55fc55f1-970f-4b9b-9268-af50ed388d49
⛔ Files ignored due to path filters (5)
packages/macros/src/tests/snapshots/openzeppelin_macros__tests__test_with_components__cannot_use_erc721_consecutive_with_manual_erc721_enumerable.snapis excluded by!**/*.snappackages/macros/src/tests/snapshots/openzeppelin_macros__tests__test_with_components__cannot_use_erc721_enumerable_with_manual_erc721_consecutive.snapis excluded by!**/*.snappackages/macros/src/tests/snapshots/openzeppelin_macros__tests__test_with_components__with_erc721_consecutive.snapis excluded by!**/*.snappackages/macros/src/tests/snapshots/openzeppelin_macros__tests__test_with_components__with_erc721_consecutive_custom_immutable_config.snapis excluded by!**/*.snappackages/macros/src/tests/snapshots/openzeppelin_macros__tests__test_with_components__with_erc721_consecutive_no_config.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
packages/macros/src/attribute/with_components/components.rspackages/macros/src/attribute/with_components/diagnostics.rspackages/macros/src/attribute/with_components/parser.rspackages/macros/src/tests/test_with_components.rs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1660 +/- ##
==========================================
+ Coverage 94.01% 94.18% +0.16%
==========================================
Files 96 96
Lines 2391 2391
==========================================
+ Hits 2248 2252 +4
+ Misses 143 139 -4 see 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
🧪 Cairo Contract Size Benchmark DiffBYTECODE SIZE (felts) (limit: 81,920 felts)No changes in felts. SIERRA CONTRACT CLASS SIZE (bytes) (limit: 4,089,446 bytes)
This comment was generated automatically from benchmark diffs. |
bidzyyys
left a comment
There was a problem hiding this comment.
Looks good to me, but make sure to resolve AI review comments.
Summary by CodeRabbit
New Features
Validation & Warnings
Tests