State Variable Change Across Upgrade

Support Upgrades
1

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

2

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.

3

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
4

@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
5

I came across this post while searching for a similar issue I am having and I was wondering if this is also the same conservative checks for compatibility at play here. I have a mapping variable I would like to change its type from

mapping(address => address) public providers;

to

mapping(address => uint) public providers;

however, I am getting:

Error: New storage layout is incompatible

Upgraded 'providers' to an incompatible type
  - In mapping(address => uint256)
    - Bad upgrade from address to uint256

Would it cause this error because of the same conservative checks? address would be allocated to a single storage slot, no?

I am using upgradable-contracts@4.4.1 and solc@0.8.11.

6

@hasandogu Is your contract already deployed? What is going to happen to addresses that are already in the providers map, once your code starts assuming that integers are stored in that location?

Unless the mapping has never been used, it sounds like the upgrade would cause a bug.

7

@frangio The contract is deployed yes.

My assumption was that since address is 20 bytes, two addresses wouldn't be sharing a storage slot and that each address in the map would have already been padded to 32 bytes. (https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html#mappings-and-dynamic-arrays)

Switching from address to uint for the mapping, the content of the storage slots allocated for each address in the providers map would just stay as is, just the storage slot would continue to be interpreted as uint. Wouldn't it be the case?

The reason we want to convert to uint is so that we can encode additional data to the most significant 96 bits (0 is default).

8

In that case it would be fine to perform that upgrade!

Ideally, you'd upgrade to a struct that is like struct { uint96 flags; address account; }. This is not currently allowed by the plugins (will be in the future), so you'd have to work around the storage checks for now. That is particularly hard to do right now because you have to edit the JSON files manually, since there is no flag to ignore the storage checks. We also plan to add this flag! In the meantime, upgrading to uint will require simpler changes in the JSON file to make it compatible.

1 Like
9

Right, the struct would make it a whole lot better.
I'll proceed with upgrades by updating JSON files manually.
Thank you for your help @frangio.

1 Like