Add new state variable to base contract for upgradeable contracts

[ExeciN]

I’m trying to do this right from the beginning and at the same time reduce the potential hair pulling in the future.

I worked a bit with upgradable contracts with oz-cli’s help but so far I have some issues that prevent me from going on mainnet.

Lets assume I have those contracts:

// pseudocode

contract ERC721PresetMinterPauserAutoIdUpgradeSafe is Initializable {
    // logic
    uint256[50] private __gap;
}

contract BaseContractEnhancements is Initializable, ERC721PresetMinterPauserAutoIdUpgradeSafe {
    // more logic
    uint256[50] private __enhancementGap;
}

contract MyContractA is Initializable, BaseContractEnhancements {
    function initialize(string memory contractName) public initializer {
        BaseContractEnhancements.initialize("some arguments for BaseContractEnhancements", contractName);
    }
}

contract MyContractB is Initializable, BaseContractEnhancements {
    function initialize(string memory contractName) public initializer {
        BaseContractEnhancements.initialize("some different arguments for BaseContractEnhancements", contractName);
    }
}

contract MyContractC is Initializable, BaseContractEnhancements {
    function initialize(string memory contractName) public initializer {
        BaseContractEnhancements.initialize("some different arguments for BaseContractEnhancements", contractName);
    }
}

Basically I made some enhancements to the base contract and finally created 3 contracts (MyContractA, MyContractB and MyContractC) with different initializers.

If I want to introduce a new variable (a bytes32 for example) for all 3 contracts along with a getter and a setter function, how would that work without pasting the same code in those 3 contracts?

Can I include the bytes32 at the end of BaseContractEnhancements and reduce the __enhancementGap array size by one?

[abcoathup]

Hi @ExeciN,

Welcome to the community

@ExeciN

Can I include the bytes32 at the end of BaseContractEnhancements and reduce the __enhancementGap array size by one?

Yes, that is the right approach.

A workaround for this is to declare unused variables on base contracts that you may want to extend in the future, as a means of "reserving" those slots. Note that this trick does not involve increased gas usage.
From: https://docs.openzeppelin.com/upgrades/2.8/writing-upgradeable#modifying-your-contracts

If you are adding new state you will need a function to initialize that state, and protect it with a custom guard to ensure that it can only be called once.

I recommend trying out your approach to become familiar with the ins and outs.

Note: When I tried it, the OpenZeppelin CLI didn't recognise that a change had been made if I only changed the base contract.

MyBase.sol

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

abstract contract MyBase {
    // Reserved storage space to allow for layout changes in the future.
    uint256[50] private ______gap;
}

MyContract.sol

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

import "./MyBase.sol";

contract MyContract is MyBase {
    uint256 private _someValue;
    
    function getSomeValue() public view returns(uint256) {
        return _someValue;
    }
}

MyBase.sol (updated)

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

abstract contract MyBase {
    // Reserved storage space to allow for layout changes in the future.
    uint256[49] private ______gap;

    uint256 private _newValue;
}

[ExeciN]

Hi @abcoathup, tanks for the inputs!

@abcoathup

Note: When I tried it, the OpenZeppelin CLI didn’t recognise that a change had been made if I only changed the base contract.

This is one of the issues I'm facing. Is there any workaround for that?
Right now I'm thinking of 2 ways

• Introduce a new initializer function on MyContract that would call the new initializer function on MyBase
• Have a uint256 private version = 0; line in the child contracts that I would bump just so that openzeppelin-cli would see it as a change. This var wont really be used anywhere.

Another issue is that openzeppelin-cli claims that one or more contracts have validation errors when trying to upgrade. Using --force also doesn't work.

In any case, after today's npmjs.org outage, I managed to write a small test case that can be deployed with openzeppelin-cli https://gofile.io/d/M13NnK however, just like I explained above, you can't really successfully upgrade the child contracts.

[spalladino]

@abcoathup

Note: When I tried it, the OpenZeppelin CLI didn’t recognise that a change had been made if I only changed the base contract.

The CLI detects if a contract has changed based on its compiled bytecode, not its source. If you make a change to the source code, it will recompile the contracts, and check if the resulting bytecode has changed to decide if an upgrade is in order or not.

In the example you shared, modifying variable declarations that are unused do not cause the compiled bytecode to change, so the CLI decides no upgrade is needed. Try using the _newValue in a method and see if it works!

---

@ExeciN I haven't checked the file you uploaded, but could you confirm if the same is happening for you?

[abcoathup]

Hi @spalladino & @ExeciN,

Apologies, my error with the example that was too simple.
The CLI does indeed detect when the bytecode changes with a new state variable added and being used in a function.

I updated the example below:

MyContract.sol

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

import "./MyBase.sol";

contract MyContract is MyBase {
    uint256 private _someValue;
    
    function getSomeValue() public view returns(uint256) {
        return _someValue;
    }
}

MyBase.sol

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

abstract contract MyBase {
    // Reserved storage space to allow for layout changes in the future.
    uint256[50] private ______gap;
}

MyBase.sol upgraded

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

abstract contract MyBase {
    // Reserved storage space to allow for layout changes in the future.
    uint256[49] private ______gap;

    uint256 private _newValue;

    function getNewValue() public view returns(uint256) {
        return _newValue;
    }
}

Upgrade

Upgrade errors on new variable being added to base contract, so needs to be upgraded using --force

$ npx oz upgrade --force
? Pick a network development
? Which instances would you like to upgrade? Choose by address
? Pick an instance to upgrade MyContract at 0xCfEB869F69431e42cdB54A4F4f105C19C080A601
Nothing to compile, all contracts are up to date.
- Variable '______gap' in contract MyBase was changed from uint256[50] to uint256[49] in contracts/MyBase.sol:1. Avoid changing types of existing variables.
- New variable 'uint256 _newValue' was inserted in contract MyBase in contracts/MyBase.sol:1. You should only add new variables at the end of your contract.
See https://docs.openzeppelin.com/upgrades/2.6//writing-upgradeable#modifying-your-contracts for more info.
✓ Contract MyContract deployed
All implementations have been deployed
✓ Instance upgraded at 0xCfEB869F69431e42cdB54A4F4f105C19C080A601. Transaction receipt: 0x69eb8d8edf51216452a86da15ebdd16ff30e08e7ace8c6fcd41af7d04b24dd34
✓ Instance at 0xCfEB869F69431e42cdB54A4F4f105C19C080A601 upgraded

[ExeciN]

I tried a more complex example without any success.

MyContract.sol

pragma solidity ^0.6.0;

import "@openzeppelin/contracts-ethereum-package/contracts/Initializable.sol";
import "./MyBase.sol";

contract MyContract is Initializable, MyBase {

    function initialize(string memory baseURI) public initializer {
        MyBase.initializeMyBaseV1(
            "MyContractCoin",
            "MCC",
            baseURI
        );
    }

    uint256 private _someValue;
    
    function getSomeValue() public view returns(uint256) {
        return _someValue;
    }
    
    function setSomeValue(uint256 value) private {
        _someValue = value;
    }
}

MyBase.sol

pragma solidity ^0.6.0;

import "@openzeppelin/contracts-ethereum-package/contracts/Initializable.sol";
import "@openzeppelin/contracts-ethereum-package/contracts/presets/ERC721PresetMinterPauserAutoId.sol";

contract MyBase is Initializable, ERC721PresetMinterPauserAutoIdUpgradeSafe {

    uint256 private _versions = 0;
    bytes32 public constant TEST1_ROLE = keccak256("TEST1_ROLE");

    /**
    * @dev Grants `DEFAULT_ADMIN_ROLE` and `MINTER_ROLE`to the account that
    * deploys the contract.
    *
    * Token URIs will be autogenerated based on `baseURI` and their token IDs.
    * See {ERC721-tokenURI}.
    */

    function initializeMyBaseV1(string memory name, string memory symbol, string memory baseURI) public initializer {
        super.initialize(name, symbol, baseURI);
        _setupRole(TEST1_ROLE, _msgSender());
        _versions += 1;
    }

    /**
    * @dev Creates a new token for `to`. Its token ID will be automatically
    * assigned (and available on the emitted {Transfer} event), and the token
    * URI autogenerated based on the base URI passed at construction.
    *
    * See {ERC721-_mint}.
    *
    * Requirements:
    *
    * - the caller must have the `MINTER_ROLE`.
    */
    function mint(address to, uint256 tokenId) public {
        require(hasRole(MINTER_ROLE, _msgSender()), "MyBase: must have minter role to mint");
        _mint(to, tokenId);
    }
    
    function getVersions() public view returns (uint256) {
        return _versions;
    }

    uint256[50] private ______gap;

    // === upgrade to V2 ===
    // bytes32 private _TEST2_ROLE;

    // function TEST2_ROLE() public view returns (bytes32) {
    //     return _TEST2_ROLE;
    // }

    // function initializeMyBaseV2() public initializer {
    //     _TEST2_ROLE = keccak256("TEST2_ROLE");
    //     _setupRole(_TEST2_ROLE, _msgSender());
    //     _versions += 2;
    // }
    // === update ______gap length before deployment ===

    // === upgrade to V3 ===
    // bytes32 private _TEST3_ROLE;

    // function TEST3_ROLE() public view returns (bytes32) {
    //     return _TEST3_ROLE;
    // }

    // function initializeMyBaseV3() public initializer {
    //     _TEST3_ROLE = keccak256("TEST3_ROLE");
    //     _setupRole(_TEST3_ROLE, _msgSender());
    //     _versions += 4;
    // }
    // === update ______gap length before deployment ===
}

MyBase.sol upgraded

pragma solidity ^0.6.0;

import "@openzeppelin/contracts-ethereum-package/contracts/Initializable.sol";
import "@openzeppelin/contracts-ethereum-package/contracts/presets/ERC721PresetMinterPauserAutoId.sol";

contract MyBase is Initializable, ERC721PresetMinterPauserAutoIdUpgradeSafe {

    uint256 private _versions = 0;
    bytes32 public constant TEST1_ROLE = keccak256("TEST1_ROLE");

    /**
    * @dev Grants `DEFAULT_ADMIN_ROLE` and `MINTER_ROLE`to the account that
    * deploys the contract.
    *
    * Token URIs will be autogenerated based on `baseURI` and their token IDs.
    * See {ERC721-tokenURI}.
    */

    function initializeMyBaseV1(string memory name, string memory symbol, string memory baseURI) public initializer {
        super.initialize(name, symbol, baseURI);
        _setupRole(TEST1_ROLE, _msgSender());
        _versions += 1;
    }

    /**
    * @dev Creates a new token for `to`. Its token ID will be automatically
    * assigned (and available on the emitted {Transfer} event), and the token
    * URI autogenerated based on the base URI passed at construction.
    *
    * See {ERC721-_mint}.
    *
    * Requirements:
    *
    * - the caller must have the `MINTER_ROLE`.
    */
    function mint(address to, uint256 tokenId) public {
        require(hasRole(MINTER_ROLE, _msgSender()), "MyBase: must have minter role to mint");
        _mint(to, tokenId);
    }
    
    function getVersions() public view returns (uint256) {
        return _versions;
    }

    uint256[49] private ______gap;

    // === upgrade to V2 ===
    bytes32 private _TEST2_ROLE;

    function TEST2_ROLE() public view returns (bytes32) {
        return _TEST2_ROLE;
    }

    function initializeMyBaseV2() public initializer {
        _TEST2_ROLE = keccak256("TEST2_ROLE");
        _setupRole(_TEST2_ROLE, _msgSender());
        _versions += 2;
    }
    // === update ______gap length before deployment ===

    // === upgrade to V3 ===
    // bytes32 private _TEST3_ROLE;

    // function TEST3_ROLE() public view returns (bytes32) {
    //     return _TEST3_ROLE;
    // }

    // function initializeMyBaseV3() public initializer {
    //     _TEST3_ROLE = keccak256("TEST3_ROLE");
    //     _setupRole(_TEST3_ROLE, _msgSender());
    //     _versions += 4;
    // }
    // === update ______gap length before deployment ===
}

Upgrade

In this asciinema I started with uint256[49] private ______gap; so I then decreased the length to 48 instead.

The upgrade process resulted in some transactions to be reverted as seen in the node logs:

Served eth_estimateGas                   conn=172.17.0.1:51534 reqid=19 t=12.064661ms err="execution reverted"
Served eth_estimateGas                   conn=172.17.0.1:51550 reqid=20 t=10.75354ms  err="execution reverted"
Served eth_estimateGas                   conn=172.17.0.1:51558 reqid=21 t=9.623944ms  err="execution reverted"
Served eth_estimateGas                   conn=172.17.0.1:51570 reqid=22 t=12.451764ms err="execution reverted"

I’m not sure what exactly goes wrong.

[abcoathup]

Hi @ExeciN,

When upgrading we can’t use the initializer modifier, so need to create a custom guard to ensure that the upgrade initialization function is only called once. See the following example:

Error Upgrading smart contracts

Hi @ethicraul, We can’t use the initializer modifier for initializing state in an upgrade, if it was already used for initializing state in the original version. I assume this why the upgrade is reverting. Instead we need to create a custom guard to ensure that any upgrade function is only called once. If you already have a version you could do a check something like the following: function upgradeToV2() public { require(_version < 2, "MyContract: Already upgraded to version 2")…