Proxy contract

Support Upgrades
1

Hi guys!
I have a question with __gap in upgradeable contracts.
I have an NFT that inherits Openzeppelin's upgradeable contracts.
It has the following logic:

:1234: Code to reproduce

// SPDX-License-Identifier: GPL-3.0
pragma solidity ^0.8.0;

import "@openzeppelin/contracts-upgradeable/token/ERC721/ERC721Upgradeable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/ERC721EnumerableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/ERC721BurnableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/ERC721PausableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/access/AccessControlEnumerableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/utils/ContextUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/utils/CountersUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

contract LandV1 is
    Initializable,
    ContextUpgradeable,
    AccessControlEnumerableUpgradeable,
    ERC721EnumerableUpgradeable,
    ERC721BurnableUpgradeable,
    ERC721PausableUpgradeable
{
    using CountersUpgradeable for CountersUpgradeable.Counter;

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

    CountersUpgradeable.Counter private _tokenIdTracker;

    string private _baseTokenURI;

    uint256 public version;

    mapping(uint256 => uint256) _tokenIdToVersion;

    struct InfoV1 {
        uint256 version;
        uint256 x;
        uint256 y;
        bool positive;
    }

    // tokenId => InfoV1
    mapping(uint256 => InfoV1) _infoV1;

    function initialize(
        string memory name,
        string memory symbol,
        string memory baseTokenURI
    ) public virtual initializer {
        __Land_init(name, symbol, baseTokenURI);
    }

    function __Land_init(
        string memory name,
        string memory symbol,
        string memory baseTokenURI
    ) internal initializer {
        __Context_init_unchained();
        __ERC165_init_unchained();
        __AccessControl_init_unchained();
        __AccessControlEnumerable_init_unchained();
        __ERC721_init_unchained(name, symbol);
        __ERC721Enumerable_init_unchained();
        __ERC721Burnable_init_unchained();
        __Pausable_init_unchained();
        __ERC721Pausable_init_unchained();

        _baseTokenURI = baseTokenURI;
        version = 1;

        _setupRole(DEFAULT_ADMIN_ROLE, _msgSender());
        _setupRole(MINTER_ROLE, _msgSender());
        _setupRole(PAUSER_ROLE, _msgSender());
    }

function mint(
        address to,
        uint256 x,
        uint256 y,
        bool positive
    ) public virtual {
        require(hasRole(MINTER_ROLE, _msgSender()), "Land: must have minter role to mint");

        _infoV1[_tokenIdTracker.current()] = InfoV1(version, x, y, positive);

        _tokenIdToVersion[_tokenIdTracker.current()] = version;

        _mint(to, _tokenIdTracker.current());

        _tokenIdTracker.increment();
    }

    // other functions
    //...
    
    uint256[48] private __gap;
}

The first thing I want to ask is what do the "private __gap" variables in contracts mean? And why do they have different sizes in contracts, such as uint256[44], uint256[46], uint256[48],...?

Next, after deploying LandV1 and working for a while, I want to add an attribute to the info of the NFT, which is the landType property, I have kept the variables of LandV1 and added the variables as follows:

// SPDX-License-Identifier: GPL-3.0
pragma solidity ^0.8.0;

import "@openzeppelin/contracts-upgradeable/token/ERC721/ERC721Upgradeable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/ERC721EnumerableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/ERC721BurnableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/ERC721PausableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/access/AccessControlEnumerableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/utils/ContextUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/utils/CountersUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

contract LandV2 is
    Initializable,
    ContextUpgradeable,
    AccessControlEnumerableUpgradeable,
    ERC721EnumerableUpgradeable,
    ERC721BurnableUpgradeable,
    ERC721PausableUpgradeable
{
    using CountersUpgradeable for CountersUpgradeable.Counter;

    // ------------------------------ Common storage for all version -----------------------
    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
    bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");

    CountersUpgradeable.Counter private _tokenIdTracker;

    string private _baseTokenURI;

    uint256 public version;

    mapping(uint256 => uint256) _tokenIdToVersion;

    struct InfoV1 {
        uint256 version;
        uint256 x;
        uint256 y;
        bool positive;
    }

    // tokenId => InfoV1
    mapping(uint256 => InfoV1) _infoV1;

    //------------------------- V2 -------------------------
    enum LandType {
        AGRICULTUAL,
        CONSTRUCTION
    }

    struct InfoV2 {
        uint256 version;
        uint256 x;
        uint256 y;
        bool positive;
        LandType landType;
    }

    // tokenId => InfoV1
    mapping(uint256 => InfoV2) _infoV2;

    function upgrade() public {
        require(hasRole(DEFAULT_ADMIN_ROLE, _msgSender()), "Land: must have admin role to upgrade");
        version = version + 1;
    }

    function mint(
        address to,
        uint256 x,
        uint256 y,
        bool positive,
        LandType landType
    ) public virtual {
        require(hasRole(MINTER_ROLE, _msgSender()), "Land: must have minter role to mint");

        _infoV2[_tokenIdTracker.current()] = InfoV2(version, x, y, positive, landType);

        _tokenIdToVersion[_tokenIdTracker.current()] = version;
        _mint(to, _tokenIdTracker.current());
        _tokenIdTracker.increment();
    }

    // other functions
    //... 

    uint256[48] private __gap;
}

I want to ask if I set the variables above "__gap" like that right or should it be like this:

// SPDX-License-Identifier: GPL-3.0
pragma solidity ^0.8.0;

import "@openzeppelin/contracts-upgradeable/token/ERC721/ERC721Upgradeable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/ERC721EnumerableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/ERC721BurnableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/ERC721PausableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/access/AccessControlEnumerableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/utils/ContextUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/utils/CountersUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

contract LandV2 is
    Initializable,
    ContextUpgradeable,
    AccessControlEnumerableUpgradeable,
    ERC721EnumerableUpgradeable,
    ERC721BurnableUpgradeable,
    ERC721PausableUpgradeable
{
    using CountersUpgradeable for CountersUpgradeable.Counter;

    // ------------------------------ Common storage for all version -----------------------
    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
    bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");

    CountersUpgradeable.Counter private _tokenIdTracker;

    string private _baseTokenURI;

    uint256 public version;

    mapping(uint256 => uint256) _tokenIdToVersion;

    struct InfoV1 {
        uint256 version;
        uint256 x;
        uint256 y;
        bool positive;
    }

    // tokenId => InfoV1
    mapping(uint256 => InfoV1) _infoV1;

    uint256[48] private __gap;

    //------------------------- V2 -------------------------
    enum LandType {
        AGRICULTUAL,
        CONSTRUCTION
    }

    struct InfoV2 {
        uint256 version;
        uint256 x;
        uint256 y;
        bool positive;
        LandType landType;
    }

    // tokenId => InfoV1
    mapping(uint256 => InfoV2) _infoV2;

    function upgrade() public {
        require(hasRole(DEFAULT_ADMIN_ROLE, _msgSender()), "Land: must have admin role to upgrade");
        version = version + 1;
    }

    function mint(
        address to,
        uint256 x,
        uint256 y,
        bool positive,
        LandType landType
    ) public virtual {
        require(hasRole(MINTER_ROLE, _msgSender()), "Land: must have minter role to mint");

        _infoV2[_tokenIdTracker.current()] = InfoV2(version, x, y, positive, landType);

        _tokenIdToVersion[_tokenIdTracker.current()] = version;
        _mint(to, _tokenIdTracker.current());
        _tokenIdTracker.increment();
    }

    // other functions
    //... 

    uint256[48] private __gap; // Does this place need to be declared in case there is a LandV3?

:computer: Environment

type or paste code here
2

When progarmming, there are usually some coding conventions that we should try and comply with e.g. having all state variables at the beginning so that's what the purpose of __gap. It allows you to add more state variables without fear that the other variables are "shifted down".

So ideally it should be..

In V1

state variables for v1
__gap
logic for v1

In V2

state variables for v1
state variables for v2
__gap (if any remains)
logic for v1
logic for v2

Just be careful when you are trying to add more state variables for v2 that you don't add more than whatever is allocated to __gap.