openzeppelin-contracts
openzeppelin-contracts copied to clipboard
Feature: ERC721 Wrapper
PR Checklist
- [x] Tests
- [x] Documentation
- [x] Changelog entry
this definitely should be extended to support erc20 and erc1155
this definitely should be extended to support erc20 and erc1155
ERC20Wrapper already exists.
The case for ERC1155 might need more discussion imo.
I mean erc721 wrapper for erc20
I would add a "onERC721Received" hook, so you can wrap by doing a safeTransfer to this.
function onERC721Received(address, address from, uint256 tokenId, bytes calldata data) external returns (bytes4) {
require(msg.sender == address(underlying));
_safeMint(data.length == 0 ? from : abi.decode(data, (address)), tokenIds);
return IERC721Receiver.onERC721Received.selector;
}
I would add a "onERC721Received" hook, so you can wrap by doing a safeTransfer to this.
function onERC721Received(address, address from, uint256 tokenId, bytes calldata data) external returns (bytes4) { require(msg.sender == address(underlying)); _safeMint(data.length == 0 ? from : abi.decode(data, (address)), tokenIds); return IERC721Receiver.onERC721Received.selector; }
Not sure about this. It'll require to use transferFrom()
inside depositFor
instead of safeTransferFrom()
, otherwise _safeMint
will be called twice. We'll be exposing our users to the same reason why there's no transfer()
in the ERC721.
Also, if we decide to only keep onERC721Received
, we'll lose the batch minting allowed in depositFor
.
What do you think?
I would add a "onERC721Received" hook, so you can wrap by doing a safeTransfer to this.
function onERC721Received(address, address from, uint256 tokenId, bytes calldata data) external returns (bytes4) { require(msg.sender == address(underlying)); _safeMint(data.length == 0 ? from : abi.decode(data, (address)), tokenIds); return IERC721Receiver.onERC721Received.selector; }
Not sure about this. It'll require to use
_transfer()
insidedepositFor
instead ofsafeTransferFrom()
, otherwise_safeMint
will be called twice. We'll be exposing our users to the same reason why there's notransfer()
in the ERC721.Also, if we decide to only keep
onERC721Received
, we'll lose the batch minting allowed indepositFor
.What do you think?
If we add this hook, we can keep safeTransfer in the depositFor
function, with the right "data", and remove the _safeMint
from deposit. The mint will be handled by the hook.
function depositFor(address account, uint256[] memory tokenIds) public virtual returns (bool) {
bytes memory data = abi.encode(account);
for (uint256 i = 0; i < tokenIds.length; ++i) {
underlying.safeTransferFrom(_msgSender(), address(this), tokenIds[i], data); // This will trigger the hook
}
return true;
}
// The hook will mint
function onERC721Received(address, address from, uint256 tokenId, bytes calldata data) external returns (bytes4) {
require(msg.sender == address(underlying));
_safeMint(data.length == 0 ? from : abi.decode(data, (address)), tokenIds);
return IERC721Receiver.onERC721Received.selector;
}
OR, we can use "unsafeTransfer" in deposit, which is ok because we know the receiver is able to handle NFTs
Either option is good IMO.
I really think having the hook brings some value.
// The hook will mint function onERC721Received(address, address from, uint256 tokenId, bytes calldata data) external returns (bytes4) { require(msg.sender == address(underlying)); _safeMint(data.length == 0 ? from : abi.decode(data, (address)), tokenIds); return IERC721Receiver.onERC721Received.selector; }
I have to admit I'm a fan of this solution. So I'll go for it.
Before implementing, I wonder, is there a case for keeping _recover
? EIP-721 requires every application/wallet/holder to implement onERC721Received
so _recover
won't be necessary if this is used for a compliant ERC721.
I have to admit I'm a fan of this solution. So I'll go for it.
Before implementing, I wonder, is there a case for keeping
_recover
? EIP-721 requires every application/wallet/holder to implementonERC721Received
so_recover
won't be necessary if this is used for a compliant ERC721.
For the record, transferFrom
is standard behavior and doesn't enforce onERC721Received
check, so _recover
should be kept for general unsafe transfers.
🦋 Changeset detected
Latest commit: e6eb16de41fbde0e54a7a413cde282288f6c158d
The changes in this PR will be included in the next version bump.
This PR includes changesets to release 1 package
Name | Type |
---|---|
openzeppelin-solidity | Minor |
Not sure what this means? Click here to learn what changesets are.
Click here if you're a maintainer who wants to add another changeset to this PR