State Variable Change Across Upgrade

Hi folks -- I am curious about understanding the risks of a state variable change across upgrades. I realize the short answer is "don't do this", but I wanted to dig a bit deeper to understand if this is true in all cases, as well as note a case where I was able to do this inadvertently without being warned.

The quick version is that I realized my choice of string for a couple of variables within a struct was a poor choice and that I really want bytes32. The variables are in a struct and they are in an abstract base contract (I roughly mirrored how OZ extensions are modeled).

After some testing both on Hardhat and Ropsten, I found that I was able to upgrade from the initial string version to bytes32 without any warnings/force flags. On Ropsten I could see that any previous data stored in the struct was zeroed out, but could successfully store subsequent data in the now modified struct.

So here is the specific question: if a struct as noted above (with strings, in an abstract base contract) has never been used to store anything, is it still absolutely a bad idea to continue with the upgrade and store bytes32 instead of string in the struct going forward?

I presume the answer is still yes, bad idea, but wanted to dig a bit since I wasn't flagged on this upgrade. Also curiosity around what's happening under the hood with respect to storage during an upgrade.

My gapping is probably wrong below, trying to get something abbreviated; the contract calling this base would of course be modified when calling string memory for each variable it wants to modify in the struct tobytes32.

:1234: baseV1.sol

pragma solidity ^0.8.0;

import "@openzeppelin/contracts-upgradeable/token/ERC721/ERC721Upgradeable.sol";
import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

abstract contract ERC721SomethingUpgradeable is Initializable, ERC721Upgradeable {
    function __ERC721Something_init() internal initializer {
        __Context_init_unchained();
        __ERC165_init_unchained();
        __ERC721Something_init_unchained();
    }

    function __ERC721Something_init_unchained() internal initializer {
    }
    using StringsUpgradeable for uint256;    

    struct SomeThing { 
       string randomHash;
       string anotherHash;
    }

    address public _miscAddress;

    mapping(uint256 => SomeThing) private _things;

    function _setThing(uint256 tokenId, string memory randomHash, string memory anotherHash) internal virtual {
        require(_exists(tokenId), "Thing set for nonexistant token");
        _things[tokenId].randomHash = randomHash;
        _things[tokenId].anotherHash = anotherHash;
    }

    uint256[47] private __gap;
}

:1234: baseV2.sol

pragma solidity ^0.8.0;

import "@openzeppelin/contracts-upgradeable/token/ERC721/ERC721Upgradeable.sol";
import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

abstract contract ERC721SomethingUpgradeable is Initializable, ERC721Upgradeable {
    function __ERC721Something_init() internal initializer {
        __Context_init_unchained();
        __ERC165_init_unchained();
        __ERC721Something_init_unchained();
    }

    function __ERC721Something_init_unchained() internal initializer {
    }
    using StringsUpgradeable for uint256;    

    struct SomeThing { 
       bytes32 randomHash;
       bytes32 anotherHash;
    }

    address public _miscAddress;

    mapping(uint256 => SomeThing) private _things;

    function _setThing(uint256 tokenId, bytes32 randomHash, bytes32 anotherHash) internal virtual {
        require(_exists(tokenId), "Thing set for nonexistant token");
        _things[tokenId].randomHash = randomHash;
        _things[tokenId].anotherHash = anotherHash;
    }

    uint256[47] private __gap;
}

:computer: Environment

Hardhat, 0.8.4, OZ 4.3.1

Ah, seems as though I may have found the answer: How to use a struct in an upgradable contract and ZeppelinOS proxy pattern and structs - #6 by spalladino.

Also shedding some more light on this: Upgrades with Peace of Mind: "Structs" Edition, Can you rename variables in a deployed upgradeable contract? - #2 by abcoathup.

I'm getting the sense that the safest means of upgrading would be to add the bytes32 variables after the string ones and leave them intact.

Welcome to the community @ccamrobertson, and sorry we weren't able to reply earlier.

What version of our tools were you using when you were able to upgrade from string to bytes32? This should not be allowed. Using the latest version of our Upgrades plugin for Hardhat, I get the following error:

contracts/Box.sol:17: Upgraded `_things` to an incompatible type
  - In mapping(uint256 => struct BoxV2.SomeThing)
    - Bad upgrade from struct BoxV1.SomeThing to struct BoxV2.SomeThing
  - In struct BoxV2.SomeThing
    - Upgraded `randomHash` to an incompatible type
      - Bad upgrade from string to bytes32
    - Upgraded `anotherHash` to an incompatible type
      - Bad upgrade from string to bytes32

Our layout compatibility checks are conservative. We disallow some things that do not necessarily cause issues if done carefully.

This is an example of that. You can do it carefully and it would be ok. But our recommendation is like you said to always append and make sure you never overwrite previous data.

1 Like

@frangio thanks for the reply here and clarification. I suspect that there is something in how I am testing the upgrades (e.g. incorrectly) that means I missed the warning.

Still very new to upgradeable contracts, I really appreciate the follow up. I went the conservative route and just added the variables below the old ones.

1 Like