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 PR implements the ERC721URIStorage extension, adding per-token URI storage capabilities to ERC721 tokens. Changes include a new ERC721URIStorageComponent with storage mappings and events, a new trait-based token_uri abstraction with default implementations, updates to the macro system to recognize the component, test mocks, and a comprehensive test suite. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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. |
immrsd
left a comment
There was a problem hiding this comment.
We should decide if the ERC721URIComponent should implement SRC5 and register an interface ID.
In the Solidity implementation it returns true for ERC4906_INTERFACE_ID, but there's actually no external functions to generate SRC5 id upon:
// Interface ID as defined in ERC-4906. This does not correspond to a traditional interface ID as ERC-4906 only
// defines events and does not include any external function.
bytes4 private constant ERC4906_INTERFACE_ID = bytes4(0x49064906);
We can:
- register the same ID (
0x49064906) to support off-chain integrations - avoid registering any interface IDs
- compute a different ID following SRC5 and register it in the initializer
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/macros/src/attribute/with_components/parser.rs`:
- Around line 364-372: The block handling AllowedComponents::ERC721URIStorage is
misformatted; run rustfmt (cargo fmt --all) to normalize spacing and alignment
around the hook_called declaration and the
Diagnostic::warn/warnings::ERC721_URI_STORAGE_HOOKS_MISSING push, ensuring the
code for AllowedComponents::ERC721URIStorage, hook_called, Diagnostic::warn, and
warnings.push follows project formatting conventions.
- Around line 213-221: The current check finds imports of ImmutableConfig via
immutable_config_import_re and sets immutable_config_imported, but
imported_immutable_config_implemented only checks code.contains("of
ImmutableConfig") so imports using an alias (e.g., `ImmutableConfig as Alias`)
are missed; update the logic to capture any alias name from the regex match
created in immutable_config_import_re (or add a second regex to capture `as
<Alias>`), then check for either `of ImmutableConfig` or `of <Alias>` (i.e., set
imported_immutable_config_implemented = code.contains("of ImmutableConfig") ||
code.contains(&format!("of {}", alias))). After making the change, run cargo fmt
to fix the CI formatting diffs.
In `@packages/token/src/erc721/extensions.cairo`:
- Around line 4-8: The extensions module is missing a re-export of DefaultConfig
from erc721_consecutive, breaking API consistency; update the extensions.cairo
module to publicly re-export DefaultConfig by adding a pub use for
erc721_consecutive::DefaultConfig alongside the existing
ERC721ConsecutiveComponent and ERC721URIStorageComponent exports so consumers
can import DefaultConfig from extensions.cairo rather than the submodule.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1639 +/- ##
==========================================
+ Coverage 94.11% 94.15% +0.04%
==========================================
Files 94 95 +1
Lines 2360 2380 +20
==========================================
+ Hits 2221 2241 +20
Misses 139 139
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
ericnordelo
left a comment
There was a problem hiding this comment.
Looking great @immrsd. Left two small comments
Fixes #1580
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.