Add ERC721Wrapper #1625
Add ERC721Wrapper #1625
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 an ERC721 Wrapper extension enabling wrapping and unwrapping of existing ERC721 tokens. It introduces the IERC721Wrapper interface, a core ERC721WrapperComponent with deposit/withdraw functionality, test mock contracts, and comprehensive test coverage for deposit, withdrawal, safe transfer, and recovery flows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Wrapper as ERC721Wrapper<br/>Contract
participant Underlying as Underlying<br/>ERC721
participant Storage
rect rgba(200, 220, 255, 0.3)
Note over User,Storage: deposit_for(receiver, token_ids)
User->>Underlying: approve(wrapper_address, token_id)
User->>Wrapper: deposit_for(receiver, [token_id])
Wrapper->>Underlying: transferFrom(user, wrapper, token_id)
Underlying->>Storage: Update owner to wrapper
Underlying-->>Wrapper: Transfer complete
Wrapper->>Wrapper: mint(receiver, token_id)
Storage->>Storage: Store wrapped token ownership
Wrapper-->>User: Return true
end
sequenceDiagram
participant User
participant Wrapper as ERC721Wrapper<br/>Contract
participant Underlying as Underlying<br/>ERC721
participant Storage
rect rgba(220, 200, 255, 0.3)
Note over User,Storage: withdraw_to(receiver, token_ids)
User->>Wrapper: withdraw_to(receiver, [token_id])
Wrapper->>Wrapper: burn(token_id)
Storage->>Storage: Remove wrapped token
Wrapper->>Underlying: transferFrom(wrapper, receiver, token_id)
Underlying->>Storage: Update owner to receiver
Underlying-->>Wrapper: Transfer complete
Wrapper-->>User: Return true
end
sequenceDiagram
participant Sender
participant Underlying as Underlying<br/>ERC721
participant Wrapper as ERC721Wrapper<br/>Contract
participant Storage
rect rgba(255, 220, 200, 0.3)
Note over Sender,Storage: on_erc721_received (safe transfer)<br/>from underlying token
Sender->>Underlying: safeTransferFrom(from, wrapper, token_id)
Underlying->>Wrapper: on_erc721_received(operator, from,<br/>token_id, data)
Wrapper->>Wrapper: Verify caller is underlying
Wrapper->>Wrapper: mint(from, token_id)
Storage->>Storage: Store wrapped token ownership
Wrapper-->>Underlying: Return IERC721_RECEIVER_ID
Underlying-->>Sender: Transfer complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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: 2
🧹 Nitpick comments (2)
packages/token/src/tests/erc721/test_erc721_wrapper.cairo (2)
237-246: Consider adding expected panic message for clarity.The test uses
#[should_panic]without specifying the expected message. Addingexpected: 'Wrapper: unsupported token'would make the test more precise and serve as documentation.🔎 Proposed fix
#[test] -#[should_panic] +#[should_panic(expected: 'Wrapper: unsupported token')] fn on_erc721_received_reverts_for_unsupported_token() {
99-111: Consider adding expected panic messages for initializer tests.Similar to other tests in this file, adding expected panic messages would improve test precision and documentation.
🔎 Proposed fix
#[test] -#[should_panic] +#[should_panic(expected: 'Wrapper: invalid underlying')] fn initializer_reverts_on_zero_underlying() { let mut state = COMPONENT_STATE(); state.initializer(ZERO); } #[test] -#[should_panic] +#[should_panic(expected: 'Wrapper: invalid underlying')] fn initializer_reverts_on_self_underlying() { let mut state = COMPONENT_STATE(); state.initializer(test_address()); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.mdpackages/interfaces/src/token/erc721.cairopackages/test_common/src/mocks/erc721.cairopackages/token/Scarb.tomlpackages/token/src/erc721/extensions.cairopackages/token/src/erc721/extensions/erc721_wrapper.cairopackages/token/src/tests/erc721.cairopackages/token/src/tests/erc721/test_erc721_wrapper.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 Cairo
- GitHub Check: Lint and test macros
🔇 Additional comments (18)
CHANGELOG.md (1)
14-15: Verify the PR reference number.The changelog entries reference PR #1624, but this PR appears to be #1625. Please verify the correct PR number should be used here.
packages/token/Scarb.toml (1)
62-63: LGTM!The new mock contracts are correctly added to the test build configuration, aligning with the mock definitions for the ERC721 wrapper tests.
packages/token/src/tests/erc721.cairo (1)
4-4: LGTM!The new test module is correctly added to the test suite.
packages/token/src/erc721/extensions.cairo (1)
3-5: LGTM!The new wrapper module and component are correctly exposed following the existing pattern for
ERC721EnumerableComponent.packages/interfaces/src/token/erc721.cairo (2)
131-142: LGTM!The
IERC721ReceiverMutinterface correctly usesref selfto allow state mutations during token reception, which is necessary for the wrapper to mint wrapped tokens upon receiving underlying tokens.
198-203: LGTM!The
IERC721Wrapperinterface follows the Solidity reference implementation with clear methods for querying the underlying token and performing deposit/withdraw operations.packages/token/src/tests/erc721/test_erc721_wrapper.cairo (4)
84-97: LGTM!The initializer tests properly verify that the underlying address is stored, and that the wrapper registers support for
IERC721_RECEIVER_IDandISRC5_IDinterfaces.
117-164: LGTM!The
deposit_fortests comprehensively verify single and multiple token deposits, including ownership transfers, event emissions, and balance updates.
170-213: LGTM!The
withdraw_totests properly verify successful withdrawals and unauthorized caller rejection.
252-282: LGTM!The recover tests properly verify the recovery mechanism for tokens transferred directly (without using
safe_transfer_from).packages/test_common/src/mocks/erc721.cairo (3)
263-271: LGTM!The test interfaces
IERC721MintableandIERC721WrapperRecovererare appropriately scoped for testing purposes.
273-304: LGTM!The
ERC721MintableMockprovides a clean, minimal implementation for the underlying ERC721 token needed in wrapper tests.
328-339: LGTM!The storage and event declarations correctly integrate the
ERC721WrapperComponentsubstorage and events.packages/token/src/erc721/extensions/erc721_wrapper.cairo (5)
155-169: Consider edge case inrecoverfunction.If the wrapped token with
token_idhas already been minted (e.g., viadeposit_foror a previousrecovercall), thesafe_mintwill panic. This is likely the intended behavior since it prevents double-wrapping, but consider adding a comment clarifying this or checking existence first for a more descriptive error.
67-73: Acknowledge reentrancy safety design.The comment explains why the unsafe
transfer_fromis acceptable here. The reasoning is sound: the underlying token is trusted by design, and no external untrusted contracts are called.
90-98: LGTM!The
withdraw_toflow correctly burns the wrapped token first (with auth check viaupdate), then safely transfers the underlying token to the receiver.
145-153: LGTM!The initializer correctly validates the underlying address (non-zero and not self) and registers the
IERC721_RECEIVER_IDinterface for safe transfer support.
5-10: LGTM!Good documentation explaining the wrapper's purpose and use cases, particularly the governance token example.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1625 +/- ##
==========================================
+ Coverage 92.89% 93.02% +0.12%
==========================================
Files 86 87 +1
Lines 2323 2364 +41
==========================================
+ Hits 2158 2199 +41
Misses 165 165
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
immrsd
left a comment
immrsd
left a comment
There was a problem hiding this comment.
LGTM! We should also support ERC721Wrapper in macros (and ERC20Wrapper as well)
…pelin/cairo-contracts into feat/add-erc721-wrapper-v2-#1581
Fixes #1581
PR Checklist
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.