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 ERC-721 Consecutive Extension (ERC-2309) with constructor-only batch minting, adds BitMap and lower_lookup utilities, introduces ERC721OwnerOfTrait abstraction across ERC721 components, and fixes signature handling by removing unnecessary type conversions in vote casting and SRC9 validation paths. Changes
Sequence DiagramsequenceDiagram
participant Constructor
participant ERC721Consecutive
participant SequentialOwnership
participant SequentialBurn
participant Event as Event Emitter
Constructor->>ERC721Consecutive: mint_consecutive(to, batch_size)
ERC721Consecutive->>ERC721Consecutive: Validate constructor scope
ERC721Consecutive->>ERC721Consecutive: Compute next_consecutive_id
ERC721Consecutive->>SequentialOwnership: Store ownership anchor
ERC721Consecutive->>ERC721Consecutive: Increment balance
ERC721Consecutive->>Event: Emit ConsecutiveTransfer(from_token_id, to_token_id, from, to)
Note over ERC721Consecutive: Sequential tokens owned by 'to'
participant User
User->>ERC721Consecutive: transferFrom(from, to, token_id)
ERC721Consecutive->>SequentialBurn: Check if token burned
alt Token not burned
ERC721Consecutive->>SequentialOwnership: Resolve owner via sequential_owner_of
ERC721Consecutive->>Event: Emit Transfer event
else Token is burned
ERC721Consecutive->>ERC721Consecutive: Panic - invalid token
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/utils/src/tests/test_fuzz_checkpoint.cairo (1)
26-46: Fixpush_checkpoints: it populates a different contract state than the one being asserted on.
Right nowpush_checkpointscreates a freshContractState, sotest_upper_lookup,test_upper_lookup_recent,test_lower_lookup, andtest_get_at_positionappear to query an uninitialized state.Proposed fix
@@ #[test] #[fuzzer] fn test_upper_lookup(checkpoints: Span<Checkpoint>) { let mut mock_trace = CONTRACT_STATE(); - push_checkpoints(checkpoints); + push_checkpoints(ref mut mock_trace, checkpoints); @@ fn test_upper_lookup_recent(checkpoints: Span<Checkpoint>) { let mut mock_trace = CONTRACT_STATE(); - push_checkpoints(checkpoints); + push_checkpoints(ref mut mock_trace, checkpoints); @@ fn test_lower_lookup(checkpoints: Span<Checkpoint>) { let mut mock_trace = CONTRACT_STATE(); - push_checkpoints(checkpoints); + push_checkpoints(ref mut mock_trace, checkpoints); @@ fn test_get_at_position(checkpoints: Span<Checkpoint>) { let mut mock_trace = CONTRACT_STATE(); - push_checkpoints(checkpoints); + push_checkpoints(ref mut mock_trace, checkpoints); @@ -fn push_checkpoints(checkpoints: Span<Checkpoint>) { - let mut mock_trace = CONTRACT_STATE(); +fn push_checkpoints(ref mut mock_trace: MockTrace::ContractState, checkpoints: Span<Checkpoint>) { for point in checkpoints { mock_trace.push_checkpoint(*point.key, *point.value); } }Also applies to: 50-58, 62-72, 127-132
CHANGELOG.md (1)
10-21: Resolve CHANGELOG placeholders: replace(#)with the actual PR number(s).
These TODOs tend to get forgotten and block release notes quality.Proposed fix
@@ - `openzeppelin_utils` - - Added `lower_lookup` support to checkpoint utilities (#) <!-- TODO: add PR number --> - - Added `BitMap` struct and associated helpers to `openzeppelin_utils::structs::bitmap` (#) <!-- TODO: add PR number --> + - Added `lower_lookup` support to checkpoint utilities (#1630) + - Added `BitMap` struct and associated helpers to `openzeppelin_utils::structs::bitmap` (#1630)packages/token/src/erc721/extensions/erc721_wrapper.cairo (1)
39-48: Generic trait bound already used in related ERC721 implementations; verify if this aligns with intended API design.The
ERC721OwnerOfTraitbound is not new—it's already required in other IERC721 interface implementations (IERC721,IERC721Metadata,IERC721CamelOnly) and in theVotescomponent for ERC721. A public default implementation (ERC721OwnerOfDefaultImpl) is available and exported from the component. The test mock demonstrates successful instantiation with this bound, so the pattern is established and functional.This is a clarification of existing requirements rather than a breaking API change. However, external contracts that manually instantiate these impl blocks without including
ERC721OwnerOfDefaultImplin scope would fail to compile.packages/token/src/erc721/extensions/erc721_enumerable/erc721_enumerable.cairo (1)
44-53: Public surface change: new ERC721OwnerOfTrait bound on ERC721EnumerableComponent requires downstream updates.The bound is functionally necessary—the enumerable impl calls
_owner_of()internally (line 121). Downstream consumers manually implementing or embeddingERC721EnumerableComponentmust now satisfy+ERC721Component::ERC721OwnerOfTrait<TContractState>.Mitigation: Use the
#[with_components(...)]macro (which auto-satisfies bounds) or import the publicly exportedERC721OwnerOfDefaultImplfor a no-op implementation.
🤖 Fix all issues with AI agents
In @packages/test_common/src/mocks/erc721.cairo:
- Around line 363-409: Add the missing import for ERC721OwnerOfDefaultImpl in
the ERC721ConsecutiveMock module: include a `use
openzeppelin_token::erc721::ERC721OwnerOfDefaultImpl;` alongside the other `use`
statements in the ERC721ConsecutiveMock block so the exposed `ERC721Impl` (and
any owner-of behavior) matches the other ERC721 mocks.
In @packages/token/src/erc721/erc721.cairo:
- Around line 25-26: Remove the unused alias import
ERC721ConsecutiveInternalImpl from the top of the file: the file already uses
ERC721ConsecutiveComponent for trait bounds (e.g., in
ConsecutiveExtensionERC721OwnerOfTraitImpl) and accesses the component via
get_dep_component!, so delete the second import line that aliases InternalImpl
(ERC721ConsecutiveComponent::InternalImpl as ERC721ConsecutiveInternalImpl) to
eliminate the dead import without changing any other code or dependencies.
In @packages/token/src/erc721/extensions/erc721_consecutive.cairo:
- Around line 148-152: The docstring for the consecutive mint function is
inaccurate: when batch_size == 0 the function returns next_consecutive_id (not
the “number minted so far” if FIRST_CONSECUTIVE_ID != 0). Either update the
comment to state it returns next_consecutive_id (the next token id to be minted)
or change the return value in the function to return (next_consecutive_id -
FIRST_CONSECUTIVE_ID) to provide the actual count; reference the symbols
FIRST_CONSECUTIVE_ID and next_consecutive_id (and the mint function handling
batch_size == 0) when making the change.
- Around line 89-92: The u256_to_address function currently uses unwrap() on the
final felt-to-ContractAddress conversion which can panic and bypass your
Errors::ADDRESS_OVERFLOW handling; change the final conversion to use
try_into().expect(Errors::ADDRESS_OVERFLOW) (or equivalent error propagation)
instead of unwrap() so the same ADDRESS_OVERFLOW error is returned on overflow;
update the function body in u256_to_address to use the expect call referencing
Errors::ADDRESS_OVERFLOW.
- Around line 219-224: The sequential-burn bitmap update in the
ERC721Consecutive burn path currently only checks token_id <
self.next_consecutive_id() and so can mark individually-minted IDs below
FIRST_CONSECUTIVE_ID as sequentially burned; update the conditional in the
ERC721Consecutive burn logic (the block that calls
self.ERC721Consecutive_sequential_burn.deref().set(token_id)) to also require
token_id >= FIRST_CONSECUTIVE_ID (i.e., add an explicit lower-bound check
alongside the existing to.is_zero(), token_id <
self.next_consecutive_id().into(), and !self.is_sequentially_burned(token_id)
checks) so only tokens in the consecutive-minted range are recorded as
sequentially burned.
- Around line 136-146: The sequential_owner_of function lacks bounds checking
and can return owners for unminted ids; before calling
ERC721Consecutive_sequential_ownership.deref().lower_lookup(token_id_u64)
compute let first = Self::first_consecutive_id(self) and let next =
Self::next_consecutive_id(self) and assert that token_id_u64 >= first &&
token_id_u64 < next, using ERC721Component::Errors::INVALID_TOKEN_ID as the
assertion error; place this guard at the top of sequential_owner_of (before
u256_to_u64 or immediately after) so invalid token queries are rejected rather
than returning a later batch owner.
In @packages/token/src/tests/erc721/test_erc721_consecutive.cairo:
- Around line 79-151: Replace raw integer literals used in assertions comparing
to token.balance_of(...) with explicit .into() conversions to match the return
type; e.g., in test_balance_after_transfers_and_burns (replace
assert_eq!(token.balance_of(OTHER), 2) with 2.into()), in
test_zero_batch_size_balance_is_zero (replace 0 with 0.into()), and likewise
update any other assertions in this file (including the referenced 175-223
region) that compare token.balance_of(...) to raw integers so they use N.into().
In @packages/utils/CHANGELOG.md:
- Around line 10-15: Replace the placeholder "(#) <!-- TODO: add PR number -->"
in the Unreleased "Added" entry that mentions `lower_lookup` with the actual PR
number for the change; locate the line containing "Added `lower_lookup` support
to checkpoint utilities" and update the trailing "(#)" to the correct PR
reference (e.g., "(#1234)") so the changelog entry includes the real PR number.
🧹 Nitpick comments (9)
packages/utils/src/tests/test_fuzz_checkpoint.cairo (1)
48-58:test_lower_lookupis a good addition, but it only tests “exact key” lookups.
Consider also asserting “between keys” behavior (e.g.,lower_lookup(key - 1)returns the previous checkpoint’s value) to catch off-by-one errors.packages/utils/src/tests/test_bitmap.cairo (1)
1-155: Nice coverage: bucket boundaries, idempotence, and mixed operation sequences.
Optional: consider suffixing small literals with_u256(or annotatingindexvariables) to make the intended API type unambiguous.Also applies to: 191-212
packages/test_common/src/erc721.cairo (1)
84-114: ConsecutiveTransfer assertion matches expected indexed/non-indexed split; please verify against event ABI.
The keys/data mapping looks consistent with ERC-2309, but it’s worth confirming the exact field ordering/encoding used by the Cairo event definition andExpectedEvent.Proposed tweak (clarify keys vs data order)
let expected = ExpectedEvent::new() .key(selector!("ConsecutiveTransfer")) .key(from_token_id) - .data(to_token_id) .key(from_address) - .key(to_address); + .key(to_address) + .data(to_token_id);packages/utils/src/structs/checkpoint.cairo (1)
34-46: Binary search logic forlower_lookup/_lower_binary_lookuplooks correct (standard lower_bound).
Only nit: the doc strings say “greater or equal than” → “greater than or equal to”.Also applies to: 80-92, 179-199
packages/utils/src/structs/bitmap.cairo (3)
15-47: BitMapImpl read/modify/write is correct; consider “no-op write” optimization.
set()/unset()always write even if the bucket already has the desired bit state; optional to skip the write to reduce storage churn (esp. if callers may set the same bit repeatedly).
49-75: Prefer bit-shifts overPowfor mask creation if Cairo supports it cleanly.
2_u128.pow(...)works, but1_u128 << shift(or equivalent) is typically clearer and cheaper. If shift isn’t available/ergonomic foru128, current approach is fine.
51-58: Avoidunwrap()in_bucket_and_mask()if you can keep it provably total.The comment is correct (mod < 256), but an
unwrap()still bakes in a panic path. If there’s an idiomatic “narrowing cast” /expectwith message / or a way to compute mask fromu256directly, prefer that.packages/token/src/tests/erc721/test_erc721_consecutive.cairo (1)
32-62: Hardcoded token id 0 is fragile; derive fromfirst_consecutive_id()consistently.
Several tests assume the consecutive range starts at 0 (including event assertions). Iffirst_consecutive_id()ever changes, these tests become misleading.Suggested tweak: use
first_token_ideverywherefn test_owner_of_first_middle_last() { let batch_size = 100; let (token, _) = setup(RECIPIENT, batch_size); - let first_token_id = 0; + let first_token_id: u256 = CONSECUTIVE_STATE().first_consecutive_id(); assert_eq!(token.owner_of(first_token_id.into()), RECIPIENT); - let middle_token_id = 50; + let middle_token_id = first_token_id + 50; assert_eq!(token.owner_of(middle_token_id.into()), RECIPIENT); - let last_token_id = batch_size - 1; + let last_token_id = first_token_id + (batch_size - 1).into(); assert_eq!(token.owner_of(last_token_id.into()), RECIPIENT); }Also applies to: 107-133, 262-285
packages/token/src/erc721/erc721.cairo (1)
703-719: Consider adding zero-address validation.The
increase_balancefunction doesn't validate thataccountis non-zero. While the documentation warns this is unsafe and requires careful pairing withERC721OwnerOfTrait, allowing balance increases for the zero address could lead to accounting inconsistencies.♻️ Optional: Add zero-address check
fn increase_balance( ref self: ComponentState<TContractState>, account: ContractAddress, value: u128, ) { + assert(!account.is_zero(), Errors::INVALID_ACCOUNT); let current_balance: u256 = self.ERC721_balances.read(account); self.ERC721_balances.write(account, current_balance + value.into()); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
AGENTS.mdCHANGELOG.mdpackages/account/src/extensions/src9/src9.cairopackages/governance/src/governor/governor.cairopackages/governance/src/tests/test_timelock.cairopackages/governance/src/tests/votes/test_block_number.cairopackages/governance/src/votes/votes.cairopackages/test_common/src/erc721.cairopackages/test_common/src/mocks.cairopackages/test_common/src/mocks/bitmap.cairopackages/test_common/src/mocks/checkpoint.cairopackages/test_common/src/mocks/erc721.cairopackages/test_common/src/mocks/votes.cairopackages/token/Scarb.tomlpackages/token/src/erc721.cairopackages/token/src/erc721/erc721.cairopackages/token/src/erc721/extensions.cairopackages/token/src/erc721/extensions/erc721_consecutive.cairopackages/token/src/erc721/extensions/erc721_enumerable/erc721_enumerable.cairopackages/token/src/erc721/extensions/erc721_wrapper.cairopackages/token/src/tests/erc721.cairopackages/token/src/tests/erc721/test_erc721_consecutive.cairopackages/utils/CHANGELOG.mdpackages/utils/src/structs.cairopackages/utils/src/structs/bitmap.cairopackages/utils/src/structs/checkpoint.cairopackages/utils/src/tests.cairopackages/utils/src/tests/test_bitmap.cairopackages/utils/src/tests/test_checkpoint.cairopackages/utils/src/tests/test_fuzz_checkpoint.cairo
💤 Files with no reviewable changes (1)
- packages/governance/src/tests/votes/test_block_number.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: boostsecurity - boostsecurityio/scanner
- GitHub Check: comment-benchmark-diff
- GitHub Check: Lint and test macros
🔇 Additional comments (29)
AGENTS.md (1)
1-5: Nice lightweight contributor guideline; no issues.packages/governance/src/tests/test_timelock.cairo (1)
24-27: Import cleanup is correct.Eventis not referenced anywhere in the file, so the removal from thesnforge_stdimports is safe.packages/token/src/erc721/extensions.cairo (1)
4-6: Export wiring confirmed. The moduleerc721_consecutiveresolves correctly, and both re-exported items (DefaultConfigandERC721ConsecutiveComponent) are properly defined and actively used throughout the codebase. No issues detected.packages/utils/src/tests.cairo (1)
1-1: Test suite hook-up is correct. The module filetest_bitmap.cairoexists atpackages/utils/src/tests/test_bitmap.cairoand is properly referenced by themod test_bitmap;declaration.packages/governance/src/governor/governor.cairo (1)
625-644: Change is correct:assert_valid_signatureparameter signature matches theSpan<felt252>passed at all vote-by-sig callsites. No remaining.into()conversions found in governance vote paths.All three call sites (lines 640 and 680 in governor.cairo, plus votes.cairo:189) correctly pass
Span<felt252>directly, matching the function's parameter type in packages/utils/src/execution.cairo.packages/account/src/extensions/src9/src9.cairo (1)
101-111: The change is correct. Theassert_valid_signaturefunction expectssignature: Span<felt252>(by value), and passingsignaturedirectly matches the function signature. This is consistent across all callsites in the codebase.packages/token/src/tests/erc721.cairo (1)
1-8: LGTM: test module wired in.
Just ensurepackages/token/src/tests/erc721/test_erc721_consecutive.cairois always present/compilable under the same cfgs as this parent module (i.e., not accidentally requiring thefuzzingfeature).packages/utils/src/structs.cairo (1)
1-5: Good: bitmap module is publicly exposed andBitMapis re-exported.
Double-check this is the intended stable import path for external users (openzeppelin_utils::structs::BitMapvs...::structs::bitmap::BitMap).packages/utils/src/tests/test_checkpoint.cairo (1)
74-105: Nice boundary coverage forlower_lookup(empty, below-min, exact-hit, above-max).
These tests align well with the intended “first key ≥ search key, else 0” semantics.packages/token/src/erc721.cairo (1)
5-5: Public re-export looks fine; double-check for downstream name collisions.
Since this broadens the public surface, it’s worth ensuring no other module already exposesERC721OwnerOfDefaultImplunder the same path.packages/test_common/src/mocks/checkpoint.cairo (1)
10-11: Mocklower_lookupwiring is consistent with the other lookup helpers.Also applies to: 45-48
packages/token/src/tests/erc721/test_erc721_consecutive.cairo (2)
79-297: Nice coverage of ERC-2309 behaviors (events, boundaries, burns/transfers).
The suite hits the important edges: zero batch, out-of-range, first/middle/last, burn + transfer interactions, andConsecutiveTransferemission.
20-77: State isolation risk:CONSECUTIVE_STATE()called separately in tests may not share storage with version fromsetup().In test functions like
test_owner_of_after_burn_panics()(line 102) andtest_balance_after_transfers_and_burns()(line 88), a freshconsecutive_stateis created viaCONSECUTIVE_STATE()and thenmark_sequential_burn()is called on it. However, theerc721_statecomes fromsetup(), which created a separateconsecutive_stateinstance that was never returned. Ifcomponent_state_for_testing()doesn't guarantee shared storage across multiple calls within the same test, then the burn marking will not be visible toowner_of()checks on the token state, potentially causing false test results.Recommended fix: return
ConsecutiveStatefromsetup()and thread it through test functions to ensure both states reference the same underlying storage throughout each test.packages/test_common/src/mocks/bitmap.cairo (1)
1-36: Clean mock; delegation toBitMapTraitis straightforward.
No issues spotted—this should be handy for isolated bitmap tests.packages/test_common/src/mocks/erc721.cairo (1)
8-8: Good: importingERC721OwnerOfDefaultImplto keep mocks compatible with the new owner-of abstraction.
This matches the direction of the PR (owner_of split-out) and should prevent “missing impl” failures in tests.Also applies to: 46-46, 210-210, 278-278, 313-313
packages/token/src/erc721/extensions/erc721_consecutive.cairo (2)
69-72:is_constructor_scope()check looks correct for enforcing “constructor-only” batch minting.
231-235:DefaultConfigimplementation is clean and keeps the component ergonomic to adopt.packages/governance/src/votes/votes.cairo (2)
189-189: LGTM!Removing the unnecessary
.into()conversion is correct. Thesignatureparameter is alreadySpan<felt252>, which matches whatassert_valid_signatureexpects.
247-273: LGTM!The addition of
+ERC721Component::ERC721OwnerOfTrait<TContractState>trait bound is necessary to support the new ownership resolution abstraction introduced for the ERC721Consecutive extension. The removal of.into()on Line 272 is correct sincebalance_ofalready returnsu256.packages/token/src/erc721/erc721.cairo (6)
10-10: LGTM!Adding
Boundedimport is necessary for checking theu64::MAXboundary in the consecutive ownership logic.
112-125: LGTM!The
ERC721OwnerOfTraitabstraction is well-designed with clear documentation about the invariant that implementations must maintain. The default implementation provides a simple storage read, which is appropriate for standard ERC721 behavior.
127-157: LGTM!The consecutive ownership resolution logic is well-structured:
- Storage check first handles transferred tokens correctly
- Boundary validation (
token_id > Bounded::<u64>::MAXandtoken_id < first_consecutive_id) properly excludes tokens outside the consecutive range- Burn check via
is_sequentially_burnedprevents returning owners for burned tokens- The fallback to
sequential_owner_ofcorrectly resolves batch-minted token ownershipThe logic correctly handles edge cases where
token_idequalsfirst_consecutive_id(included in consecutive range) orBounded::<u64>::MAX(included in consecutive range).
163-171: LGTM!The
+ERC721OwnerOfTrait<TContractState>trait bound is consistently added to theERC721Impl, enabling the ownership resolution abstraction throughout the component.
768-771: LGTM!The delegation to
OwnerOf::owner_ofcleanly enables pluggable ownership resolution while maintaining the existing API surface.
934-939: LGTM!The
ERC721OwnerOfDefaultImplcorrectly provides a default implementation by leveraging the trait's default method. This allows contracts not using the consecutive extension to easily satisfy the trait bound.packages/token/Scarb.toml (1)
66-66: LGTM!Adding
ERC721ConsecutiveMockto the external contracts build list is necessary for testing the new consecutive extension.packages/test_common/src/mocks.cairo (1)
3-3: LGTM!Adding the
bitmapmodule declaration exposes the BitMap mock required for testing the consecutive extension's burned token tracking.packages/test_common/src/mocks/votes.cairo (2)
192-192: LGTM!The
ERC721OwnerOfDefaultImplimport is required to satisfy the newERC721OwnerOfTraitbound on ERC721 components. This provides standard ownership resolution for the votes mock.
249-249: LGTM!Same as above - the import provides the default ownership trait implementation for the block number votes mock.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1630 +/- ##
==========================================
+ Coverage 93.98% 94.11% +0.12%
==========================================
Files 92 94 +2
Lines 2279 2360 +81
==========================================
+ Hits 2142 2221 +79
- Misses 137 139 +2
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
…eat/add-erc721-consecutive-extension-#1579
…com:OpenZeppelin/cairo-contracts into feat/add-erc721-consecutive-extension-#1579
3 participants
Fixes #1579
PR Checklist
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.