Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Function beforeTokenTransfer #5146

Open
Wi77iame opened this issue Aug 11, 2024 · 10 comments
Open

Function beforeTokenTransfer #5146

Wi77iame opened this issue Aug 11, 2024 · 10 comments

Comments

@Wi77iame
Copy link

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
Copy link

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
Copy link
Author

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
Copy link

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
Copy link
Member

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
Copy link
Contributor

0xneves commented Aug 20, 2024

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:

  • You are not properly overriding the _update function
  • Even if you override it... By doing that you make it impossible to mint/burn tokens, because those internal functions use _update themselves.

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

@ernestognw
Copy link
Member

@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
Copy link
Author

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
Copy link
Contributor

0xneves commented Aug 21, 2024

@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
Copy link
Contributor

0xneves commented Aug 21, 2024

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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants