OpenZeppelin / openzeppelin-contracts

OpenZeppelin Contracts is a library for secure smart contract development.
https://openzeppelin.com/contracts
MIT License
24.92k stars 11.79k forks source link

Function beforeTokenTransfer #5146

Open Wi77iame opened 2 months ago

Wi77iame commented 2 months ago

Hello everybody,

I m new here and in Solidity and I try to learn. I have an issue with this function (_beforeTokenTransfer), here is the code :


pragma solidity ^0.8.0;

import "@openzeppelin/contracts/token/ERC721/ERC721.sol";

contract SoulboundToken is ERC721 { uint256 public nextTokenId;

mapping(uint256 => bool) public soulbound;

constructor() ERC721("SoulboundToken", "MTK") {}

function mint(address to) public {
    _mint(to, nextTokenId);
    soulbound[nextTokenId] = true;
    nextTokenId++;
}

// Override the transfer functions to prevent transferring of SBTs
function _beforeTokenTransfer(address from, address to, uint256 tokenId) internal override {
    require(from == address(0), "SBTs are non-transferable");
    super._beforeTokenTransfer(from, to, tokenId);
}

}


Remix or VScode send me an error : "Function has override specified but does not override anything"

I don't understand why and how can I fix it

Could you please help me e-and explain me what to do and what is the matter ?

Thank you very much

William

fatemehnedaee commented 2 months ago

Hi In v5.0.0-rc.0 release _beforeTokenTransfer deleted, So you can use _update instead. If you have any question, you can ask me.

Wi77iame commented 2 months ago

Thank you for this fast answer !

So here is my new code :

// SPDX-License-Identifier: MIT // Compatible with OpenZeppelin Contracts ^5.0.0 pragma solidity ^0.8.20;

import "@openzeppelin/contracts/token/ERC721/ERC721.sol"; import "@openzeppelin/contracts/token/ERC721/extensions/ERC721URIStorage.sol"; import "@openzeppelin/contracts/token/ERC721/extensions/ERC721Burnable.sol"; import "@openzeppelin/contracts/access/Ownable.sol";

contract MyToken is ERC721, ERC721URIStorage, ERC721Burnable, Ownable { uint256 private _nextTokenId;

constructor(address initialOwner)
    ERC721("MyToken", "MTK")
    Ownable(initialOwner)
{}

function safeMint(address to, string memory uri) public onlyOwner {
    uint256 tokenId = _nextTokenId++;
    _safeMint(to, tokenId);
    _setTokenURI(tokenId, uri);
}

// The following functions are overrides required by Solidity.

function tokenURI(uint256 tokenId)
    public
    view
    override(ERC721, ERC721URIStorage)
    returns (string memory)
{
    return super.tokenURI(tokenId);
}

function supportsInterface(bytes4 interfaceId)
    public
    view
    override(ERC721, ERC721URIStorage)
    returns (bool)
{
    return super.supportsInterface(interfaceId);
}

function _update(address from, address to) internal virtual { require(from == address(0) || to == address(0), "SBTs are non-transferable"); } }


Is it correct ? I have no error but this seems too easy If I understood correctly, the idea of ​​this function is to prevent the transfer of the token

fatemehnedaee commented 2 months ago

your welcome Yes, It is easy As document says :

_update(address to, uint256 tokenId, address auth) → address

Transfers tokenId from its current owner to to, or alternatively mints (or burns) if the current owner (or to) is the zero address. Returns the owner of the tokenId before the update.

The auth argument is optional. If the value passed is non 0, then this function will check that auth is either the owner of the token, or approved to operate on the token (by the owner).

ernestognw commented 2 months ago

I have no error but this seems too easy

Too fast to say it's ready but it should be that easy though. Feel free to head to our Wizard where you can build a contract with a couple of option selection. The output is always secure.

0xneves commented 2 months ago

Thank you for this fast answer !

So here is my new code :

// SPDX-License-Identifier: MIT // Compatible with OpenZeppelin Contracts ^5.0.0 pragma solidity ^0.8.20;

import "@openzeppelin/contracts/token/ERC721/ERC721.sol"; import "@openzeppelin/contracts/token/ERC721/extensions/ERC721URIStorage.sol"; import "@openzeppelin/contracts/token/ERC721/extensions/ERC721Burnable.sol"; import "@openzeppelin/contracts/access/Ownable.sol";

contract MyToken is ERC721, ERC721URIStorage, ERC721Burnable, Ownable { uint256 private _nextTokenId;

constructor(address initialOwner)
    ERC721("MyToken", "MTK")
    Ownable(initialOwner)
{}

function safeMint(address to, string memory uri) public onlyOwner {
    uint256 tokenId = _nextTokenId++;
    _safeMint(to, tokenId);
    _setTokenURI(tokenId, uri);
}

// The following functions are overrides required by Solidity.

function tokenURI(uint256 tokenId)
    public
    view
    override(ERC721, ERC721URIStorage)
    returns (string memory)
{
    return super.tokenURI(tokenId);
}

function supportsInterface(bytes4 interfaceId)
    public
    view
    override(ERC721, ERC721URIStorage)
    returns (bool)
{
    return super.supportsInterface(interfaceId);
}

function _update(address from, address to) internal virtual { require(from == address(0) || to == address(0), "SBTs are non-transferable"); } }

Is it correct ? I have no error but this seems too easy If I understood correctly, the idea of ​​this function is to prevent the transfer of the token

Hey @Wi77iame , good to know you are learning Solidity!

Your contract will not work because:

I propose you to override the _transfer function directly wich will cover for both transfer and transferFrom, while not messing with mint/burn

ernestognw commented 2 months ago

@0xneves, heads up that the _transfer function is not virtual anymore since we do recommend overriding _update instead. Considering @Wi77iame's original code:

function _beforeTokenTransfer(address from, address to, uint256 tokenId) internal override {
    require(from == address(0), "SBTs are non-transferable");
    super._beforeTokenTransfer(from, to, tokenId);
}

The correct override would be:

import {IERC721Errors} from "@openzeppelin/contracts/interfaces/draft-IERC6093.sol";

...

function _update(address to, uint256 tokenId, address auth) internal override returns (address) {
  address from = super._update(from, tokenId, auth);
  if (from != address(0)) throw IERC721Errors.ERC721InvalidSender(from); // SBTs are non-transferrable
  return from;
}

This way, it's still possible to mint, since the from will be address(0). Burning on the other side is impossible, but I feel that's implicit.

Wi77iame commented 2 months ago

Hello guys !

Thanks for your help. My project is to allow a user to mint an SBT after having entered information via a form and this information will be found on this SBT. Following your comments here is the new code :

pragma solidity ^0.8.20;

import "@openzeppelin/contracts/token/ERC721/ERC721.sol"; import "@openzeppelin/contracts/token/ERC721/extensions/ERC721URIStorage.sol"; import "@openzeppelin/contracts/token/ERC721/extensions/ERC721Burnable.sol"; import "@openzeppelin/contracts/access/Ownable.sol"; import {IERC721Errors} from "@openzeppelin/contracts/interfaces/draft-IERC6093.sol";

contract VeriBound_SBT is ERC721, ERC721URIStorage, ERC721Burnable, Ownable { uint256 private _nextTokenId;

    struct FormData {
    string name;
    string mail;
}

mapping(uint256 => FormData) public tokenFormData;

constructor(address initialOwner)
    ERC721("MyToken", "MTK")
    Ownable(initialOwner)
{}

function safeMint(
    address to, 
    string memory uri,
    string memory name,
    string memory mail

    ) 
    public onlyOwner {
    uint256 tokenId = _nextTokenId++;
    _safeMint(to, tokenId);
    _setTokenURI(tokenId, uri);
    tokenFormData[tokenId] = FormData(name, mail);
}

// The following functions are overrides required by Solidity.

 function getTokenFormData(uint256 tokenId) public view returns (FormData memory) {
    return tokenFormData[tokenId];
}

function tokenURI(uint256 tokenId)
    public
    view
    override(ERC721, ERC721URIStorage)
    returns (string memory)
{
    return super.tokenURI(tokenId);
}

function supportsInterface(bytes4 interfaceId)
    public
    view
    override(ERC721, ERC721URIStorage)
    returns (bool)
{
    return super.supportsInterface(interfaceId);
}

function _update(address to, uint256 tokenId, address auth) internal override returns (address) { address from = super._update(from, tokenId, auth); if (from != address(0)) throw IERC721Errors.ERC721InvalidSender(from); // SBTs are non-transferrable return from; } }


With this new function _update, I don't know why, Remix underlines IERC721Errors.

Before this, I wrote (without the underscore):

function update(address from, address to) internal virtual { require(from == address(0) || to == address(0), "SBTs are non-transferrable"); }

And it seemed to have work when I ran transactions.

0xneves commented 2 months ago

@0xneves, heads up that the _transfer function is not virtual anymore since we do recommend overriding _update instead. Considering @Wi77iame's original code:

function _beforeTokenTransfer(address from, address to, uint256 tokenId) internal override {
    require(from == address(0), "SBTs are non-transferable");
    super._beforeTokenTransfer(from, to, tokenId);
}

The correct override would be:

import {IERC721Errors} from "@openzeppelin/contracts/interfaces/draft-IERC6093.sol";

...

function _update(address to, uint256 tokenId, address auth) internal override returns (address) {
  address from = super._update(from, tokenId, auth);
  if (from != address(0)) throw IERC721Errors.ERC721InvalidSender(from); // SBTs are non-transferrable
  return from;
}

This way, it's still possible to mint, since the from will be address(0). Burning on the other side is impossible, but I feel that's implicit.

Nice call on reverting when it comes from a different address other than zero address. But this _update function looks different than the 0.8.20. Shouldn't it be something like this then?

import {IERC721Errors} from "@openzeppelin/contracts/interfaces/draft-IERC6093.sol";

...

function _update(address from, address to, uint256 tokenId) internal override {
    if (from != address(0)) throw IERC721Errors.ERC721InvalidSender(from); // SBTs are non-transferrable
    super._update(from, to, tokenId);
}
0xneves commented 2 months ago

With this new function _update, I don't know why, Remix underlines IERC721Errors.

Before this, I wrote (without the underscore):

function update(address from, address to) internal virtual { require(from == address(0) || to == address(0), "SBTs are non-transferrable"); }

And it seemed to have work when I ran transactions.

Make sure that you are not gonna use _burn with your require that worked. Because they directly call the _update function. When passing the address(0) as the from argument - which is what @ernestognw suggested - when calling _mint, it will work to make the token soulbounded. But if you revert when the to is also 0 then you are invalidating the possibility to call _burn.

Users can still send to a random address, but burning is a good practice to lower the total supply, therefore raising the token price on charts.

function _mint(address account, uint256 value) internal {
        if (account == address(0)) {
            revert ERC20InvalidReceiver(address(0));
        }
        _update(address(0), account, value);
    }
Wi77iame commented 2 months ago

OK I think now I understand the goal of function _update and how I have to deal with.

But Remix is going crazy with semicolon, when I ask RemixAI, it wants to put ";" at the end of // SBTs are non-transferrable but I always have an error...

MuhammadAwais-KM commented 1 week ago

With this new function _update, I don't know why, Remix underlines IERC721Errors. Before this, I wrote (without the underscore): function update(address from, address to) internal virtual { require(from == address(0) || to == address(0), "SBTs are non-transferrable"); } And it seemed to have work when I ran transactions.

Make sure that you are not gonna use _burn with your require that worked. Because they directly call the _update function. When passing the address(0) as the from argument - which is what @ernestognw suggested - when calling _mint, it will work to make the token soulbounded. But if you revert when the to is also 0 then you are invalidating the possibility to call _burn.

Users can still send to a random address, but burning is a good practice to lower the total supply, therefore raising the token price on charts.

function _mint(address account, uint256 value) internal {
        if (account == address(0)) {
            revert ERC20InvalidReceiver(address(0));
        }
        _update(address(0), account, value);
    }

I have a question can i add custom logic inside the _update function ? I am asking because if I add custom logic inside _update function then due to that custom logic my minting will be failed, As inside mint function there is function call to _update.

0xneves commented 1 week ago

Yes sir, you can use super._update(...) inside an override function and beneath add your custom logic