Upgradeable NFT Smart Contract

Can someone advise me on how to make the Openzeppelin NFT ERC721 upgradeable?

I currently have a constructor with the following attributes:

constructor() public ERC721("      ", "      ") Pausable() { 
    _baseUri = "";
    _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
    _setupRole(PAUSER_ROLE, msg.sender);
    _setupRole(MINTER_ROLE, msg.sender);
    _setupRole(BURNER_ROLE, msg.sender);
    _setupRole(OPERATOR_ROLE, msg.sender);
  }

Do i need remove the constructor and set the above commands inside an "initializer" function?

Thanks

Yes, and maybe you can have a look at this docs at the same time: Writing Upgradeable Contracts - OpenZeppelin Docs

Right, thanks. So something like this would suffice?

After importing "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"

contract ERC721 is Initializable

function initialize() public initializer {
    _baseUri = "";
    _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
    _setupRole(PAUSER_ROLE, msg.sender);
    _setupRole(MINTER_ROLE, msg.sender);
    _setupRole(BURNER_ROLE, msg.sender);
    _setupRole(OPERATOR_ROLE, msg.sender);
  }

Would this work?

Emmm, you can have a look at the new modifier onlyInitializing: https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/releases/tag/v4.4.1

OK great, does this include importing the source file ?

And what version of Solidity is compatible with this?

"@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

Just so you are aware, my Solidity compiler does not recognise this command.

CompileError: DeclarationError: Identifier not found or not unique.
  --> project:/contracts/ERC721CarbonAsset.sol:26:32:
   |
26 | function initialize() internal onlyInitializing {
   |                                ^^^^^^^^^^^^^^^^

Is there a reason why?

I think v4.4.1 and above.

I am currently using v4.8.0, so i shouldn't have any problems right?

yet it still wont recognise the command...?

Sorry, managed to sort it now. Compiler recognises the command! Thanks @Skyge

Hi, can you kindly tell me if the below contract overall is correct? Was i right to comment out the Constructor?

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


import "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/ERC721URIStorageUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

contract ERC721 is Initializable, ERC721URIStorageUpgradeable, PausableUpgradeable, AccessControlUpgradeable {
  
  bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");
  bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
  bytes32 public constant BURNER_ROLE = keccak256("BURNER_ROLE");
  bytes32 public constant OPERATOR_ROLE = keccak256("OPERATOR_ROLE");
  
  // Base URI
  string private _baseUri;
  address _forwarder;
  
  mapping(uint256 => string) private _digests;
  mapping(uint256 => string) private _infoRoots;
  
  // Addresses under operator control
  mapping(address => bool) private _operatorEnabled;
  
  // constructor() public ERC721("", "") Pausable() { 

function initialize() onlyInitializing internal {
  
    _baseUri = "";
    _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
    _setupRole(PAUSER_ROLE, msg.sender);
    _setupRole(MINTER_ROLE, msg.sender);
    _setupRole(BURNER_ROLE, msg.sender);
    _setupRole(OPERATOR_ROLE, msg.sender);
  }
  
  function pause() external onlyRole(PAUSER_ROLE) {
    _pause();
  }

  function unpause() external onlyRole(PAUSER_ROLE) {
    _unpause();
  }

  /**
   * @dev See {ERC20-_beforeTokenTransfer}.
   * Taken from ERC20Pausable
   *
   * Requirements:
   *
   * - the contract must not be paused.
   */
  function _beforeTokenTransfer(address from, address to, uint256 amount, uint256 batchSize) internal virtual override  {
    super._beforeTokenTransfer(from, to, amount, batchSize);
    require(!paused(), "ERC721Pausable: token transfer while paused");
  }
  
  function mint(address to, uint256 tokenId, string memory tokenUri, string memory _digest) public onlyRole(MINTER_ROLE) {
    _mint(to, tokenId);
    _setTokenURI(tokenId, tokenUri);
    _digests[tokenId] = _digest;
  }

  function burn(uint256 tokenId) public onlyRole(BURNER_ROLE) {
    _burn(tokenId);
  }
  
  function setBaseURI(string memory uri) external onlyRole(OPERATOR_ROLE) {
    _baseUri = uri;
  }

  /**
   * @dev Base URI for computing {tokenURI}. If set, the resulting URI for each
   * token will be the concatenation of the `baseURI` and the `tokenId`. Empty
   * by default, can be overriden in child contracts.
   */
  function _baseURI() internal view virtual override returns (string memory) {
    return _baseUri;
  }

  function infoRoot(uint256 tokenId) external view virtual returns (string memory) {
    require(_exists(tokenId), "ERC721URIStorage: URI query for nonexistent token");

    string memory _infoRoot = _infoRoots[tokenId];

    // If there is no infoRoot set, return an empty string.
    if (bytes(_infoRoot).length == 0) {
      return "";
    }

    return _infoRoot;
  }

  function setInfoRoot(uint256 tokenId, string memory _infoRoot) external onlyRole(OPERATOR_ROLE) whenNotPaused() {
    require(_exists(tokenId), "ERC721URIStorage: URI set of nonexistent token");
    _infoRoots[tokenId] = _infoRoot;
  }
  
  function digest(uint256 tokenId) external view virtual returns (string memory) {
    require(_exists(tokenId), "ERC721URIStorage: URI query for nonexistent token");

    string memory _digest = _digests[tokenId];

    // If there is no digest set, return an empty string.
    if (bytes(_digest).length == 0) {
      return "";
    }

    return _digest;
  }
  
  function setdigest(uint256 tokenId, string memory _digest) external onlyRole(OPERATOR_ROLE) whenNotPaused() {
    require(_exists(tokenId), "ERC721URIStorage: URI set of nonexistent token");
    _digests[tokenId] = _digest;
  }

  // Operator initiatiated token transfer
  function operatorTransfer(address recipient, uint256 tokenId) external onlyRole(OPERATOR_ROLE) whenNotPaused() returns (bool) {
    address owner = ownerOf(tokenId);
    require(isOperatorControlled(owner), "ERC721: sender not under operator control");
    // Reset appoval
    _approve(msg.sender, tokenId);
    transferFrom(owner, recipient, tokenId);
    return true;
  }
  
  // Address owner can enable their address for operator control
  // Default state is operator disabled
  function enableOperatorControl() external whenNotPaused() returns (bool) {
    require(msgSender() != address(0), "ERC20: owner is a zero address");
    require(!isOperatorControlled(msgSender()), "ERC20: owner already under operator control");
    _operatorEnabled[msgSender()] = true;
    return true;
  }
  
  // Operator role can remove operator control from an address
  function disableOperatorControl(address owner) external onlyRole(OPERATOR_ROLE) whenNotPaused() returns (bool) {
    require(owner != address(0), "ERC721: owner is a zero address");
    require(isOperatorControlled(owner), "ERC721: owner not under operator control");
    _operatorEnabled[owner] = false;
    return true;
  }
  
  function isOperatorControlled(address owner) public view returns (bool) {
    require(owner != address(0), "ERC721: owner is a zero address");
    return _operatorEnabled[owner];
  }

  function supportsInterface(bytes4 interfaceId) public view virtual override(ERC721Upgradeable, AccessControlUpgradeable) returns (bool) {
      return super.supportsInterface(interfaceId);
  }

  function msgSender() internal view returns(address sender) {
    if(msg.sender == _forwarder) {
      bytes memory array = msg.data;
      uint256 index = msg.data.length;
      assembly {
          // Load the 32 bytes word from memory with the address on the lower 20 bytes, and mask those.
          sender := and(mload(add(array, index)), 0xffffffffffffffffffffffffffffffffffffffff)
      }
    } else {
        sender = msg.sender;
    }
    return sender;
  }

  function setForwarder(address forwarder) external onlyRole(OPERATOR_ROLE) returns (bool) {
    _forwarder = forwarder;
    return true;
  }
  
  function getForwarder() external view returns (address) {
    return _forwarder;
  }
}

Why do you need upgradable contracts what are the goals here ?

Ohhh, my fault, it is recommended to use the modifier initializer.

You need to write some test cases before deploying contracts, I think no one can guarantee a contract is absolutely safe.

Well it’s simply to ensure that if we want to add new features, fix bugs or mitigate attacks, we can do so through the proxy pattern.

Sorry, where do I need to place that?

Yes of course that’s the plan

function initialize() initializer {}

Don’t you need to use onlyInitializing to mitigate security? Doesn’t that ensure that it can only be called once and also not from other outside contracts/malicious actors?

I am sorry for misleading you, for the final deployed contract, just use initializer