ProjectOpenSea / operator-filter-registry

MIT License
312 stars 93 forks source link

restrict via _beforeTokenTransfer? #41

Closed ph101pp closed 1 year ago

ph101pp commented 1 year ago

The example implementations for ERC721 and ERC1155 are restricting safeTransferFrom and safeBatchTransferFrom like this:

    function safeTransferFrom(address from, address to, uint256 tokenId, uint256 amount, bytes memory data)
        public
        override
        onlyAllowedOperator(from)
    {
        super.safeTransferFrom(from, to, tokenId, amount, data);
    }

    function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data
    ) public virtual override onlyAllowedOperator(from) {
        super.safeBatchTransferFrom(from, to, ids, amounts, data);
    }

is there a reason to not restrict via _beforeTokenTransfer?

    function _beforeTokenTransfer(
        address operator,
        address from,
        address to,
        uint[] memory ids,
        uint[] memory amounts,
        bytes memory data
    ) internal virtual override onlyAllowedOperator(from) {
        super._beforeTokenTransfer(operator, from, to, ids, amounts, data);
    }
web3jt commented 1 year ago

seems it will works well too, and save more gas? if u have logic in the _beforeTokenTransfer

nidhhoggr commented 1 year ago

It seems the downside of using _beforeTokenTransfer instead is that _beforeTokenTransfer is also called when minting, thus unnecessarily increasing gas fees on mints.

dievardump commented 1 year ago

_beforeTokenTransfer is not part of EIP-721, it's only on some implementations of it

On top of that, in most of those implementations, if you are already in _beforeTokenTransfer, it means you already used quite some more gas to come there than if applied on a modifier

web3jt commented 1 year ago

It seems the downside of using _beforeTokenTransfer instead is that _beforeTokenTransfer is also called when minting, thus unnecessarily increasing gas fees on mints.

Oh, yes, watse when minting, tkx for ur explain

operatorfilterer commented 1 year ago

As mentioned in this thread, _beforeTokenTransfer is not part of the EIP-721 specification, so the examples do not use them.

Developers are free to implement registry logic however they'd like, and can ensure compliance with the included validation tests.

nidhhoggr commented 1 year ago

@operatorfilterer EIP-721 conformity is irrelevant because your example includes an import for the OpenZeppelin ERC721 which DOES implement _beforeTokenTransfer.

import {ERC721} from "openzeppelin-contracts/token/ERC721/ERC721.sol";

As such the real issue is that _beforeTokenTransfer is called on Mint.