Add ERC721 Wrapper

[ernestognw]

Fixes LIB-647

PR Checklist

• Tests
• Documentation
• Changelog entry

[TrejGun]

this definitely should be extended to support erc20 and erc1155

[ernestognw]

this definitely should be extended to support erc20 and erc1155

ERC20Wrapper already exists.

The case for ERC1155 might need more discussion imo.

[TrejGun]

I mean erc721 wrapper for erc20

[Amxx]

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;
}

[ernestognw]

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?

[Amxx]

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() 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?

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.

[Amxx]

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;
}

[Amxx]

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.

[ernestognw]

// 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.

[ernestognw]

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.

For the record, transferFrom is standard behavior and doesn't enforce onERC721Received check, so _recover should be kept for general unsafe transfers.

[frangio]

LGTM

[Amxx]

Here is your second approval :