Add ERC721Wrapper #1624
Add ERC721Wrapper #1624
Conversation
WalkthroughThis PR introduces ERC20 and ERC721 wrapper extensions enabling tokens to wrap underlying assets. It adds public interfaces for wrapper operations, component implementations with deposit/withdraw functionality, test mocks, and comprehensive test suites for both wrapper types. A minor signature handling adjustment is also made to the ERC20 permit flow. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Wrapper as ERC20Wrapper
participant Underlying as Underlying ERC20
rect rgb(200, 220, 240)
Note over User,Underlying: Deposit Flow
User->>User: approve(wrapper, amount)
User->>Wrapper: deposit_for(receiver, amount)
activate Wrapper
Wrapper->>Wrapper: validate sender, receiver
Wrapper->>Underlying: transferFrom(user, wrapper, amount)
Underlying->>Underlying: transfer logic
Wrapper->>Wrapper: mint(receiver, amount)
Wrapper->>Wrapper: emit Deposit event
deactivate Wrapper
end
rect rgb(220, 240, 200)
Note over User,Underlying: Withdraw Flow
User->>Wrapper: withdraw_to(receiver, amount)
activate Wrapper
Wrapper->>Wrapper: validate receiver
Wrapper->>Wrapper: burn(user, amount)
Wrapper->>Underlying: transfer(receiver, amount)
Underlying->>Underlying: transfer logic
Wrapper->>Wrapper: emit Withdraw event
deactivate Wrapper
end
sequenceDiagram
participant User
participant Wrapper as ERC721Wrapper
participant Underlying as Underlying ERC721
rect rgb(200, 220, 240)
Note over User,Underlying: Deposit Flow (Safe Transfer)
User->>Underlying: safeTransferFrom(user, wrapper, tokenId)
Underlying->>Wrapper: onERC721Received(user, user, tokenId, data)
activate Wrapper
Wrapper->>Wrapper: validate sender is wrapper
Wrapper->>Wrapper: mint wrapped(user, tokenId)
Wrapper->>Wrapper: emit implicit transfer event
deactivate Wrapper
end
rect rgb(220, 240, 200)
Note over User,Underlying: Withdraw Flow
User->>Wrapper: withdraw_to(receiver, tokenIds)
activate Wrapper
loop for each tokenId
Wrapper->>Wrapper: burn wrapped(user, tokenId)
Wrapper->>Underlying: transferFrom(wrapper, receiver, tokenId)
end
deactivate Wrapper
end
rect rgb(240, 220, 200)
Note over User,Underlying: Recover Flow
Wrapper->>Underlying: check ownership of tokenId
Wrapper->>Wrapper: mint wrapped(account, tokenId)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
…eat/add-erc721-wrapper-#1581
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
packages/token/src/tests/erc721/test_erc721_wrapper.cairo (2)
117-163: Missing test for deposit_for reverting when receiver is wrapper.The ERC721 wrapper tests don't include a test case for
deposit_forreverting when the receiver is the wrapper contract itself. The ERC20 wrapper has this test (deposit_for_reverts_when_receiver_is_wrapper), and the ERC721 implementation should have parity.🔎 Suggested test
#[test] #[should_panic(expected: 'ERC721: invalid receiver')] fn deposit_for_reverts_when_receiver_is_wrapper() { let (underlying, wrapper) = setup_wrapped(); let owner = setup_account(); mint_underlying(underlying.contract_address, owner, TOKEN_ID); start_cheat_caller_address(underlying.contract_address, owner); underlying.approve(wrapper.contract_address, TOKEN_ID); stop_cheat_caller_address(underlying.contract_address); start_cheat_caller_address(wrapper.contract_address, owner); wrapper.deposit_for(wrapper.contract_address, array![TOKEN_ID].span()); }
170-213: Consider adding a multi-token withdraw test.For symmetry with
deposit_for_handles_multiple_tokens, consider adding a test that validates withdrawing multiple tokens in a single call works correctly.packages/token/src/erc721/extensions/erc721_wrapper.cairo (1)
156-169: Consider checking if wrapped token already exists.The
recoverfunction mints a wrapped token for an underlying token owned by the wrapper. However, if the wrapped token was already minted (e.g., viadeposit_fororon_erc721_received), callingrecoverwould attempt to mint a duplicate token ID, which should fail. This is the correct behavior, but consider adding a comment to clarify this.
📜 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 (16)
CHANGELOG.mdScarb.tomlpackages/interfaces/src/token/erc20.cairopackages/interfaces/src/token/erc721.cairopackages/test_common/src/mocks/erc20.cairopackages/test_common/src/mocks/erc721.cairopackages/token/Scarb.tomlpackages/token/src/erc20/erc20.cairopackages/token/src/erc20/extensions.cairopackages/token/src/erc20/extensions/erc20_wrapper.cairopackages/token/src/erc721/extensions.cairopackages/token/src/erc721/extensions/erc721_wrapper.cairopackages/token/src/tests/erc20.cairopackages/token/src/tests/erc20/test_erc20_wrapper.cairopackages/token/src/tests/erc721.cairopackages/token/src/tests/erc721/test_erc721_wrapper.cairo
🔇 Additional comments (19)
packages/token/Scarb.toml (1)
54-55: LGTM!The addition of wrapper mock contracts to the test build configuration properly supports the new ERC20 and ERC721 wrapper functionality.
Also applies to: 64-65
packages/interfaces/src/token/erc20.cairo (1)
171-181: LGTM!The IERC20Wrapper interface follows standard wrapper patterns with clear method signatures for underlying token access, deposits, and withdrawals.
packages/token/src/tests/erc20.cairo (1)
3-3: LGTM!The test module integration for ERC20 wrapper is properly added.
packages/token/src/erc721/extensions.cairo (1)
2-2: LGTM!The ERC721 wrapper extension module is properly declared and the component is correctly re-exported, following the same pattern as ERC721EnumerableComponent.
Also applies to: 4-4
packages/token/src/tests/erc721.cairo (1)
4-4: LGTM!The test module integration for ERC721 wrapper is properly added, consistent with the ERC20 wrapper test pattern.
packages/token/src/erc20/extensions.cairo (1)
1-1: LGTM!The ERC20 wrapper extension module is properly declared, following the same pattern as the existing erc4626 module.
packages/token/src/erc20/erc20.cairo (1)
378-378: The signature handling change is correct. Theassert_valid_signaturefunction inpackages/utils/src/execution.cairoexpectssignature: Span<felt252>directly, which matches what is passed from the permit function. The conversion via.into()occurs internally withinassert_valid_signaturewhen calling the dispatcher, not at the call site. Passingsignaturedirectly is intentional and fully compatible.packages/interfaces/src/token/erc721.cairo (2)
131-142: Well-designed interface for stateful receiver.The
IERC721ReceiverMuttrait withref selfappropriately enables state mutations duringon_erc721_received, which is required for the wrapper to mint tokens when receiving underlying tokens via safe transfer.
198-203: LGTM!The
IERC721Wrapperinterface correctly mirrors the ERC20 wrapper pattern withSpan<u256>for batch token operations, which is appropriate for NFTs where each token is unique.packages/token/src/tests/erc721/test_erc721_wrapper.cairo (1)
84-97: LGTM!Good coverage of initializer behavior including SRC5 interface registration verification.
packages/test_common/src/mocks/erc721.cairo (1)
273-304: LGTM!
ERC721MintableMockis a clean implementation for test purposes.packages/test_common/src/mocks/erc20.cairo (1)
335-383: LGTM!The
ERC20WrapperMockcorrectly composes both components with proper event wiring using#[flat]for bothERC20EventandERC20WrapperEvent.packages/token/src/tests/erc20/test_erc20_wrapper.cairo (3)
56-76: LGTM!Good initializer tests covering valid case, zero address, and self-reference scenarios with proper error message assertions.
160-216: Excellent recover test coverage.The tests thoroughly cover:
- Recovering excess underlying tokens
- No recovery when balance equals supply
- Panic when underlying balance is below total supply (undercollateralized)
222-253: LGTM!The spy helpers follow the established pattern and correctly validate event emission with proper key/data separation.
packages/token/src/erc20/extensions/erc20_wrapper.cairo (3)
85-104: LGTM!The
deposit_forimplementation is correct with proper validation:
- Prevents the wrapper from calling itself as sender
- Prevents minting to the wrapper itself
- Asserts transfer success before minting
106-124: LGTM!The
withdraw_tocorrectly burns before transferring, which is the CEI (Checks-Effects-Interactions) pattern—burn first (effect), then transfer (interaction).
39-59: LGTM!Events are well-structured with indexed keys for
sender/callerandreceiver, enabling efficient off-chain filtering.packages/token/src/erc721/extensions/erc721_wrapper.cairo (1)
39-101: No changes needed. The ERC721Wrapper implementation aligns with the OpenZeppelin standard. Unlike ERC20Wrapper, ERC721Wrapper intentionally does not emit custom Deposit/Withdraw events — instead, it relies on the standard Transfer events emitted by both the underlying and wrapped tokens to track deposit/withdraw operations. This design difference is intentional and consistent with the Solidity ERC721Wrapper implementation.Likely an incorrect or invalid review comment.
| ### Added | ||
|
|
||
| - AccessControlDefaultAdminRules (#1567): | ||
| - `MAXIMUM_DEFAULT_ADMIN_TRANSFER_DELAY` constant exposed in the component ImmutableConfig. | ||
| - `maximum_default_admin_transfer_delay` getter to the `IAccessControlDefaultAdminRules` interface. | ||
| - `execute_single_call`, `execute_calls` and `assert_valid_signature` to openzeppelin_utils::execution (#1531) | ||
| - MetaTransactionV0 preset, interface, and dispatchers (#1512) | ||
| - Embeddable `ERC2981AdminAccessControlDefaultAdminRulesImpl` implementation providing admin functions for a `ERC2981` token based on `AccessControlDefaultAdminRules` component (#1516) | ||
| - `ERC6372Clock` interface to `openzeppelin_utils::contract_clock` (#1417) | ||
| - AccessControlDefaultAdminRules interface and component (#1432) | ||
|
|
||
| ### Changed | ||
|
|
||
| - Bump scarb to 2.13.1 (#1565) | ||
| - Decoupled openzeppelin_utils version from the umbrella package (#1531) | ||
| - Removed openzeppelin_governance dependency on openzeppelin_account (#1531) | ||
| - Removed openzeppelin_token dependency on openzeppelin_account (#1531) | ||
| - `GovernorComponent` and its extensions support voting tokens that follow the `ERC6372` clock standard (#1417) | ||
|
|
||
| ### Changed (Breaking) | ||
|
|
||
| - `GovernorComponent` proposal state resolution at snapshot timepoint changed from Active to Pending (#1606) | ||
| - Removed `execute_single_call`, `execute_calls` and `assert_valid_signature` from openzeppelin_account::utils (#1531) | ||
| - `VotesComponent` now supports customizable clock mechanisms via `ERC6372Clock`, enabling alternative clock sources (#1417) | ||
| - New `IERC20Wrapper` interface in `openzeppelin_interfaces::token::erc20` for ERC20 wrappers (#1617) | ||
| - New `ERC20WrapperComponent` providing ERC20 wrapping and unwrapping functionality (#1617) | ||
|
|
There was a problem hiding this comment.
Missing ERC721 Wrapper changelog entries.
The CHANGELOG only documents the ERC20Wrapper additions, but this PR also introduces:
IERC721WrapperinterfaceERC721WrapperComponentIERC721ReceiverMutinterface
These should be added to maintain accurate documentation.
🔎 Suggested additions
### Added
- New `IERC20Wrapper` interface in `openzeppelin_interfaces::token::erc20` for ERC20 wrappers (#1617)
- New `ERC20WrapperComponent` providing ERC20 wrapping and unwrapping functionality (#1617)
+- New `IERC721Wrapper` interface in `openzeppelin_interfaces::token::erc721` for ERC721 wrappers (#1624)
+- New `ERC721WrapperComponent` providing ERC721 wrapping and unwrapping functionality (#1624)
+- New `IERC721ReceiverMut` interface for stateful ERC721 receiver implementations (#1624)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### Added | |
| - AccessControlDefaultAdminRules (#1567): | |
| - `MAXIMUM_DEFAULT_ADMIN_TRANSFER_DELAY` constant exposed in the component ImmutableConfig. | |
| - `maximum_default_admin_transfer_delay` getter to the `IAccessControlDefaultAdminRules` interface. | |
| - `execute_single_call`, `execute_calls` and `assert_valid_signature` to openzeppelin_utils::execution (#1531) | |
| - MetaTransactionV0 preset, interface, and dispatchers (#1512) | |
| - Embeddable `ERC2981AdminAccessControlDefaultAdminRulesImpl` implementation providing admin functions for a `ERC2981` token based on `AccessControlDefaultAdminRules` component (#1516) | |
| - `ERC6372Clock` interface to `openzeppelin_utils::contract_clock` (#1417) | |
| - AccessControlDefaultAdminRules interface and component (#1432) | |
| ### Changed | |
| - Bump scarb to 2.13.1 (#1565) | |
| - Decoupled openzeppelin_utils version from the umbrella package (#1531) | |
| - Removed openzeppelin_governance dependency on openzeppelin_account (#1531) | |
| - Removed openzeppelin_token dependency on openzeppelin_account (#1531) | |
| - `GovernorComponent` and its extensions support voting tokens that follow the `ERC6372` clock standard (#1417) | |
| ### Changed (Breaking) | |
| - `GovernorComponent` proposal state resolution at snapshot timepoint changed from Active to Pending (#1606) | |
| - Removed `execute_single_call`, `execute_calls` and `assert_valid_signature` from openzeppelin_account::utils (#1531) | |
| - `VotesComponent` now supports customizable clock mechanisms via `ERC6372Clock`, enabling alternative clock sources (#1417) | |
| - New `IERC20Wrapper` interface in `openzeppelin_interfaces::token::erc20` for ERC20 wrappers (#1617) | |
| - New `ERC20WrapperComponent` providing ERC20 wrapping and unwrapping functionality (#1617) | |
| ### Added | |
| - New `IERC20Wrapper` interface in `openzeppelin_interfaces::token::erc20` for ERC20 wrappers (#1617) | |
| - New `ERC20WrapperComponent` providing ERC20 wrapping and unwrapping functionality (#1617) | |
| - New `IERC721Wrapper` interface in `openzeppelin_interfaces::token::erc721` for ERC721 wrappers (#1624) | |
| - New `ERC721WrapperComponent` providing ERC721 wrapping and unwrapping functionality (#1624) | |
| - New `IERC721ReceiverMut` interface for stateful ERC721 receiver implementations (#1624) |
🤖 Prompt for AI Agents
In CHANGELOG.md around lines 12 to 16, the changelog only lists ERC20 wrapper
additions but omits the ERC721-related items introduced by the PR; add matching
entries under the "Added" section for `IERC721Wrapper` in
`openzeppelin_interfaces::token::erc721`, `ERC721WrapperComponent` providing
wrapping/unwrapping functionality, and `IERC721ReceiverMut` interface, using the
same terse bullet style and referencing the PR number (e.g., #1617) to keep
format and attribution consistent.
| #[event] | ||
| #[derive(Drop, starknet::Event)] | ||
| enum Event { | ||
| #[flat] | ||
| ERC721WrapperEvent: ERC721WrapperComponent::Event, | ||
| } |
There was a problem hiding this comment.
Missing ERC721 events in Event enum.
The ERC721WrapperMock Event enum only includes ERC721WrapperEvent but not the ERC721Component::Event. This means ERC721 events (Transfer, Approval) won't be properly emitted from this contract.
🔎 Suggested fix
#[event]
#[derive(Drop, starknet::Event)]
enum Event {
+ #[flat]
+ ERC721Event: ERC721Component::Event,
#[flat]
ERC721WrapperEvent: ERC721WrapperComponent::Event,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[event] | |
| #[derive(Drop, starknet::Event)] | |
| enum Event { | |
| #[flat] | |
| ERC721WrapperEvent: ERC721WrapperComponent::Event, | |
| } | |
| #[event] | |
| #[derive(Drop, starknet::Event)] | |
| enum Event { | |
| #[flat] | |
| ERC721Event: ERC721Component::Event, | |
| #[flat] | |
| ERC721WrapperEvent: ERC721WrapperComponent::Event, | |
| } |
🤖 Prompt for AI Agents
In packages/test_common/src/mocks/erc721.cairo around lines 334 to 339, the
Event enum only wraps ERC721WrapperComponent::Event and omits the
ERC721Component::Event, so standard ERC721 events (Transfer, Approval) won't be
emitted; add a second #[flat] enum variant for ERC721Component::Event (e.g.
ERC721Event: ERC721Component::Event) alongside the existing ERC721WrapperEvent,
keeping the #[event] and #[derive(Drop, starknet::Event)] attributes so both
component events are flattened and emitted correctly.
| fn recover(ref self: ComponentState<TContractState>, account: ContractAddress) -> u256 { | ||
| let underlying = self.underlying(); | ||
| let this = starknet::get_contract_address(); | ||
|
|
||
| // get underlying balance held by this contract | ||
| let token = IERC20Dispatcher { contract_address: underlying }; | ||
| let underlying_balance = token.balance_of(this); | ||
|
|
||
| // get current total supply of wrapped tokens | ||
| let mut erc20_component = get_dep_component_mut!(ref self, ERC20); | ||
| let total_supply = erc20_component.total_supply(); | ||
|
|
||
| // calculate recoverable amount | ||
| let value = underlying_balance - total_supply; | ||
|
|
||
| // mint recoverable amount to specified account | ||
| erc20_component.mint(account, value); | ||
| value |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd erc20_wrapper.cairo --type fRepository: OpenZeppelin/cairo-contracts
Length of output: 183
🏁 Script executed:
cat -n packages/token/src/erc20/extensions/erc20_wrapper.cairo | head -180Repository: OpenZeppelin/cairo-contracts
Length of output: 8121
🌐 Web query:
Solidity ERC20Wrapper recover function implementation undercollateralized
💡 Result:
Short answer
- OpenZeppelin's ERC20Wrapper._recover mints the positive difference between the underlying token balance held by the wrapper and the wrapper's totalSupply (i.e. mint underlying.balanceOf(address(this)) - totalSupply()). If the wrapper is undercollateralized (totalSupply > underlying balance) you must guard against underflow — otherwise the subtraction will revert in Solidity >=0.8. See OpenZeppelin docs and source. [1][2]
Minimal safe implementation
- Behaviour you usually want: only mint when the wrapper holds more underlying than totalSupply; otherwise do nothing and return 0.
Example (Solidity >=0.8):
function _recover(address account) internal virtual returns (uint256) {
uint256 balance = IERC20(_underlying).balanceOf(address(this));
uint256 supply = totalSupply();
if (balance <= supply) return 0;
uint256 value = balance - supply;
_mint(account, value);
return value;
}
Why
- OpenZeppelin's reference implementation computes the raw difference (balance - totalSupply) — it assumes you call _recover only when there is excess. If called while undercollateralized the plain subtraction will revert under Solidity's checked-arithmetic. Guarding with an if prevents revert and makes behavior explicit. [1][2]
References
- OpenZeppelin ERC20Wrapper docs / API (description of _recover). [1]
- Example/source listing of _recover computing balance - totalSupply(). [2]
Would you like me to produce a complete contract example that exposes recover with access control and tests for over- and under-collateralized cases?
🏁 Script executed:
cat -n packages/token/src/tests/erc20/test_erc20_wrapper.cairo | grep -A 20 "recover"Repository: OpenZeppelin/cairo-contracts
Length of output: 4086
Align underflow handling in recover with OpenZeppelin's safe pattern.
The calculation underlying_balance - total_supply at line 163 panics when undercollateralized (confirmed by test recover_reverts_when_underlying_below_total_supply). While this is acknowledged in the WARNING comment at lines 12-16, OpenZeppelin's Solidity ERC20Wrapper implementation safely returns 0 instead of panicking.
Recommend adopting the safe pattern (Option 1) to prevent unintended reverts:
// calculate recoverable amount
- let value = underlying_balance - total_supply;
+ if underlying_balance <= total_supply {
+ return 0;
+ }
+ let value = underlying_balance - total_supply;This allows recover to be safely called in any state without forcing external access control to prevent panics. Update the corresponding test to reflect this new behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn recover(ref self: ComponentState<TContractState>, account: ContractAddress) -> u256 { | |
| let underlying = self.underlying(); | |
| let this = starknet::get_contract_address(); | |
| // get underlying balance held by this contract | |
| let token = IERC20Dispatcher { contract_address: underlying }; | |
| let underlying_balance = token.balance_of(this); | |
| // get current total supply of wrapped tokens | |
| let mut erc20_component = get_dep_component_mut!(ref self, ERC20); | |
| let total_supply = erc20_component.total_supply(); | |
| // calculate recoverable amount | |
| let value = underlying_balance - total_supply; | |
| // mint recoverable amount to specified account | |
| erc20_component.mint(account, value); | |
| value | |
| fn recover(ref self: ComponentState<TContractState>, account: ContractAddress) -> u256 { | |
| let underlying = self.underlying(); | |
| let this = starknet::get_contract_address(); | |
| // get underlying balance held by this contract | |
| let token = IERC20Dispatcher { contract_address: underlying }; | |
| let underlying_balance = token.balance_of(this); | |
| // get current total supply of wrapped tokens | |
| let mut erc20_component = get_dep_component_mut!(ref self, ERC20); | |
| let total_supply = erc20_component.total_supply(); | |
| // calculate recoverable amount | |
| if underlying_balance <= total_supply { | |
| return 0; | |
| } | |
| let value = underlying_balance - total_supply; | |
| // mint recoverable amount to specified account | |
| erc20_component.mint(account, value); | |
| value | |
| } |
🤖 Prompt for AI Agents
In packages/token/src/erc20/extensions/erc20_wrapper.cairo around lines 150 to
167, the subtraction underlying_balance - total_supply can underflow and panic
when underlying < total_supply; change the logic to compute a recoverable value
that is zero when undercollateralized (e.g., if underlying_balance >
total_supply then value = underlying_balance - total_supply else value = 0),
only mint when value > 0, and return value; also update the test
recover_reverts_when_underlying_below_total_supply to expect a zero return (and
no revert) instead of a panic.
| fn deposit_for( | ||
| ref self: ComponentState<TContractState>, | ||
| receiver: ContractAddress, | ||
| token_ids: Span<u256>, | ||
| ) -> bool { | ||
| let caller = starknet::get_caller_address(); | ||
| let this = starknet::get_contract_address(); | ||
| let underlying = self.underlying(); | ||
| let token = IERC721Dispatcher { contract_address: underlying }; | ||
| let mut erc721_component = get_dep_component_mut!(ref self, ERC721); | ||
| let data = array![].span(); | ||
|
|
||
| for token_id in token_ids { | ||
| // This is an "unsafe" transfer that doesn't call any hook on the receiver. With | ||
| // underlying() being trusted (by design of this contract) and no other contracts | ||
| // expected to be called from there, reentrancy should be safe. | ||
| token.transfer_from(caller, this, *token_id); | ||
| erc721_component.safe_mint(receiver, *token_id, data); | ||
| } | ||
| true | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the ERC721Component's safe_mint implementation
find . -type f -name "*.cairo" | xargs grep -l "fn safe_mint" | head -5Repository: OpenZeppelin/cairo-contracts
Length of output: 112
🏁 Script executed:
# Let's also locate the erc721_wrapper file to understand the context better
find . -type f -path "*erc721*wrapper*" -name "*.cairo"Repository: OpenZeppelin/cairo-contracts
Length of output: 191
🏁 Script executed:
# Search for ERC20 wrapper to see the explicit validation mentioned
find . -type f -path "*erc20*wrapper*" -name "*.cairo"Repository: OpenZeppelin/cairo-contracts
Length of output: 187
🏁 Script executed:
# Get the safe_mint implementation from ERC721Component
cat ./packages/token/src/erc721/erc721.cairo | grep -A 20 "fn safe_mint"Repository: OpenZeppelin/cairo-contracts
Length of output: 748
🏁 Script executed:
# Get the ERC20 wrapper to see the explicit validation approach
cat ./packages/token/src/erc20/extensions/erc20_wrapper.cairo | head -100Repository: OpenZeppelin/cairo-contracts
Length of output: 4092
🏁 Script executed:
# Also check the full erc721_wrapper to understand the context
cat ./packages/token/src/erc721/extensions/erc721_wrapper.cairoRepository: OpenZeppelin/cairo-contracts
Length of output: 7635
Add explicit receiver validation in deposit_for for consistency with ERC20Wrapper.
The ERC20 wrapper explicitly checks assert(receiver != this, Errors::INVALID_RECEIVER), but this check is missing in ERC721Wrapper's deposit_for. The safe_mint function does not validate the receiver address—it only invokes the ERC721 receiver hook, which would succeed if the receiver is the wrapper itself (since it implements IERC721ReceiverMut). Add an explicit check: assert(receiver != this, Errors::INVALID_RECEIVER) to prevent accidental minting to the wrapper and maintain consistency with the ERC20 implementation.
🤖 Prompt for AI Agents
In packages/token/src/erc721/extensions/erc721_wrapper.cairo around lines 55 to
75, add the same explicit receiver validation used in the ERC20 wrapper:
assert(receiver != this, Errors::INVALID_RECEIVER). The fix: after computing
`this = starknet::get_contract_address()` (and before performing any
transfers/mints), insert the assertion to prevent minting to the wrapper itself,
keeping behavior consistent with ERC20Wrapper and avoiding accidental
self-minting.
| // TODO!: Should mint to from or operator? | ||
| erc721_component.safe_mint(from, token_id, data); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Resolve TODO: Clarify mint recipient.
The TODO comment questions whether to mint to from or operator. Based on the Solidity reference implementation (ERC721Wrapper.sol), the token should be minted to from (the original owner who initiated the transfer), which is the current behavior. This TODO should be resolved.
🔎 Suggested resolution
let mut erc721_component = get_dep_component_mut!(ref self, ERC721);
- // TODO!: Should mint to from or operator?
+ // Mint to `from` (the original owner) as per ERC721Wrapper specification.
erc721_component.safe_mint(from, token_id, data);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // TODO!: Should mint to from or operator? | |
| erc721_component.safe_mint(from, token_id, data); | |
| // Mint to `from` (the original owner) as per ERC721Wrapper specification. | |
| erc721_component.safe_mint(from, token_id, data); |
🤖 Prompt for AI Agents
In packages/token/src/erc721/extensions/erc721_wrapper.cairo around lines 124 to
125, the TODO questioning whether to mint to `from` or `operator` should be
resolved: keep the call minting to `from` (matching the Solidity ERC721Wrapper
reference), remove the TODO, and replace it with a concise clarifying comment
stating that the token is intentionally minted to `from` per the
ERC721Wrapper.sol behavior.
Scarb.toml
Outdated
| assert_macros = "2.13.1" | ||
| starknet = "2.13.1" | ||
| snforge_std = "0.54.1" | ||
| snforge_std = "0.52.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd Scarb.tomlRepository: OpenZeppelin/cairo-contracts
Length of output: 544
🌐 Web query:
snforge_std 0.52.0 0.53.0 Cairo 2.13.1 compatibility
💡 Result:
Short answer: snforge_std 0.52.0 is not compatible with Cairo 2.13.1 — it is listed as requiring Cairo <= 2.11.4. For 0.53.0 there is no public package listing showing explicit 2.13 compatibility; a newer snforge_std release (0.54.1) exists (upgrade likely required) or else you must use a Cairo <=2.11.4 build. [1][2][3]
Sources:
- snforge_std_deprecated 0.52.0 metadata (Cairo version <=2.11.4). [1]
- Scarb / Cairo v2.13.1 release notes. [2]
- snforge_std package listing showing newer 0.54.1 release. [3]
🏁 Script executed:
head -60 Scarb.tomlRepository: OpenZeppelin/cairo-contracts
Length of output: 1781
snforge_std 0.52.0 is incompatible with Cairo 2.13.1.
snforge_std 0.52.0 requires Cairo <= 2.11.4. Since this project uses Cairo 2.13.1, the current version must be upgraded to 0.54.1 or later. The downgrade to 0.52.0 will cause build and runtime failures.
🤖 Prompt for AI Agents
In Scarb.toml around line 48, the pinned dependency snforge_std = "0.52.0" is
incompatible with the project's Cairo 2.13.1; update the snforge_std version to
0.54.1 or later (replace the 0.52.0 entry) so it supports Cairo 2.13.1, then run
your build/update (scarb build or scarb update) to verify the dependency
resolution and rebuild to confirm the issue is resolved.
🧪 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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1624 +/- ##
==========================================
+ Coverage 92.75% 92.91% +0.15%
==========================================
Files 85 87 +2
Lines 2279 2358 +79
==========================================
+ Hits 2114 2191 +77
- Misses 165 167 +2
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
bidzyyys
left a comment
bidzyyys
left a comment
There was a problem hiding this comment.
Looks good besides ERC20Wrapper included here.
|
|
||
| ### Added | ||
|
|
||
| - New `IERC20Wrapper` interface in `openzeppelin_interfaces::token::erc20` for ERC20 wrappers (#1617) |
There was a problem hiding this comment.
IMHO ERC20Wrapper should not be part of this PR.
There was a problem hiding this comment.
You are right, I used as it made it easier to implement similar patterns, but forgot to decouple them before pushing. I will create a different PR since this comes from the erc20wrapper branch.
| assert(caller == self.underlying(), Errors::UNSUPPORTED_TOKEN); | ||
|
|
||
| let mut erc721_component = get_dep_component_mut!(ref self, ERC721); | ||
| // TODO!: Should mint to from or operator? |
There was a problem hiding this comment.
It should mint to from, based on Solidity reference.
There was a problem hiding this comment.
I saw that, but I feel it might make more sense to mint to the operator, since it has the approval already. Minting to from doesn't add (afaics) any advantage, and it makes the flow of an operator more tricky, since it has to first transfer the token to itself, and then wrap to receive the token.
|
Closing in favor of #1625 |
Fixes #1581
PR Checklist
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.