1

I wrote an NFT smart contract based on OpenZeppelin documentation, I request you to review this smart contract and tell me if it is safe, complete and in terms of writing is clean and professional.

And also, in general, is suitable for an NFT game that only has three objects for NFT?

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

// Import necessary OpenZeppelin libraries for ERC721 functionality
import "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import "@openzeppelin/contracts/token/ERC721/extensions/ERC721Enumerable.sol";
import "@openzeppelin/contracts/token/ERC721/extensions/ERC721URIStorage.sol";
import "@openzeppelin/contracts/token/ERC721/extensions/ERC721Burnable.sol";
import "@openzeppelin/contracts/security/Pausable.sol";
import "@openzeppelin/contracts/access/AccessControl.sol";
import "@openzeppelin/contracts/utils/Counters.sol";

/**
 * @title NftExam
 * @dev The NftExam contract represents an ERC721-compliant non-fungible token (NFT).
 */
contract NftExam is ERC721, ERC721Enumerable, ERC721URIStorage, Pausable, AccessControl, ERC721Burnable {
    using Counters for Counters.Counter;
    Counters.Counter private _tokenIdCounter;

    bytes32 public constant ACCESS_CONTROLLER_ROLE = keccak256("ACCESS_CONTROLLER_ROLE");
    bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");

    /**
     * @dev Initializes the contract with the name "NftExam" and symbol "NEX".
     */
    constructor() ERC721("NftExam", "NEX") {
        _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
        _grantRole(PAUSER_ROLE, msg.sender);
    }

    /**
     * @dev Pauses all token transfers. Can only be called by an account with the PAUSER_ROLE.
     */
    function pause() public onlyRole(PAUSER_ROLE) {
        _pause();
    }

    /**
     * @dev Unpauses token transfers. Can only be called by an account with the PAUSER_ROLE.
     */
    function unpause() public onlyRole(PAUSER_ROLE) {
        _unpause();
    }

    /**
     * @dev Safely mints a new NFT with the given URI and assigns it to the specified address.
     * @param to The address to which the NFT will be assigned.
     * @param uri The URI for the token's metadata.
     */
    function safeMint(address to, string memory uri) public {
        uint256 tokenId = _tokenIdCounter.current();
        _tokenIdCounter.increment();
        _safeMint(to, tokenId);
        _setTokenURI(tokenId, uri);
    }

    /**
     * @dev Grants a role to an account, restricted to ACCESS_CONTROLLER_ROLE holders.
     * @param role The role to grant.
     * @param account The address to which the role will be granted.
     */
    function grantRole(string memory role , address account)
        public
        onlyRole(ACCESS_CONTROLLER_ROLE)
    {
        bytes32 roleHash = keccak256(bytes(role));

        require(!hasRole(roleHash , account) , "Role already assigned to the account !");
        _grantRole(roleHash , account);
    }

    /**
     * @dev Revokes a role from an account, restricted to ACCESS_CONTROLLER_ROLE holders.
     * @param role The role to revoke.
     * @param account The address from which the role will be revoked.
     */
    function revokeRole(string memory role , address account)
        public
        onlyRole(ACCESS_CONTROLLER_ROLE)
    {
        bytes32 roleHash = keccak256(bytes(role));
        
        require(hasRole(roleHash , account) , "Role not assigned to the account !");
        _revokeRole(roleHash , account);
    }

    /**
     * @dev Hook function called before any token transfer.
     * @param from The address from which the tokens are transferred.
     * @param to The address to which the tokens are transferred.
     * @param tokenId The ID of the token being transferred.
     * @param batchSize The number of tokens being transferred.
     */
    function _beforeTokenTransfer(address from, address to, uint256 tokenId, uint256 batchSize)
        internal
        whenNotPaused
        override(ERC721, ERC721Enumerable)
    {
        super._beforeTokenTransfer(from, to, tokenId, batchSize);
    }

    // The following functions are overrides required by Solidity.

    /**
     * @dev Burns (destroys) the token with the given ID.
     * @param tokenId The ID of the token to be burned.
     */
    function _burn(uint256 tokenId) internal override(ERC721, ERC721URIStorage) {
        super._burn(tokenId);
    }

    /**
     * @dev Returns the token URI for the given token ID.
     * @param tokenId The ID of the token.
     * @return A string representing the token's URI.
     */
    function tokenURI(uint256 tokenId)
        public
        view
        override(ERC721, ERC721URIStorage)
        returns (string memory)
    {
        return super.tokenURI(tokenId);
    }

    /**
     * @dev Checks if a given interface is supported by this contract.
     * @param interfaceId The interface identifier.
     * @return True if the interface is supported, false otherwise.
     */
    function supportsInterface(bytes4 interfaceId)
        public
        view
        override(ERC721, ERC721Enumerable, ERC721URIStorage, AccessControl)
        returns (bool)
    {
        return super.supportsInterface(interfaceId);
    }
}

1 Answer 1

1

there are some minor improvements that you can make to optimize your code and enhance its functionality :

  • In the constructor, you can also grant the ACCESS_CONTROLLER_ROLE to yourself, so that you can manage other roles later.
  • In the safeMint function, you can add a modifier to restrict who can mint new tokens. For example, you can use onlyRole(ACCESS_CONTROLLER_ROLE) or onlyOwner (if you inherit from Ownable) to limit the minting privilege to yourself or other authorized accounts.
  • In the _beforeTokenTransfer hook function, you have a parameter called batchSize, but you are not using it anywhere in the function body. You can either remove it or use it for some logic, such as checking the maximum number of tokens that can be transferred at once.
  • You can also implement some additional features that are common in NFT games, such as:
    • A minting fee that is paid by the minter to the contract owner or a beneficiary address.
    • A royalty fee that is paid by the buyer to the original creator of the token or a beneficiary address every time the token is sold on a secondary market.
    • A tokenURI function that returns a dynamic URI based on some logic or data stored on-chain, rather than a static URI stored in a mapping.
1
  • 1
    Thank you very much for your help, I will definitely follow the points you mentioned
    – Ali
    Commented Sep 30, 2023 at 5:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.