Is it safe to change storage variables visibility + remove initialize function in an upgrade?

Archive SDK
1

Hi,
I’m working on an upgrade for a deployed contract.
currently I have a deployed upgradable contract like this

contract A is Initializable, Ownable {
  address private foo;
  ...
  address public lastVariable;

  function initialize() public initializer  {
    Ownable.initialize(msg.sender);
    ...
  }

  ...
}

now I would like to do 3 things with the new upgrade:

  1. Remove the old initialize method (given that it has already been initialized, to save on bytecode size) and add a new one for new storage variables for the new instance
  2. add some new storage variables
  3. decouple the contract storage from the contract

so I was wondering is something like this is correct or if could potentially break something (npx oz upgrade does not raise errors), specifically when changing visibility from private to internal for the storage decoupling

contract AStorage {
  address internal foo; // <- this must be updated to internal
  ...
  address public lastVariable;
}

contract AStorageV2 is AStorage {
  address public newVar;
}

contract A is Initializable, Ownable, AStorageV2 {
  function manualInitialize() external onlyOwner {
    ... // do init stuff for this new instance
  }
  ...
}

Thanks

1 Like
2

Hi @bugduino,

When upgrading we can’t change the order nor the type of state variables (https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable#modifying-your-contracts).

Removing functions is fine. You need to keep the inheritance of Initializable as it contains a state variable.

Changing a state variable visibility shouldn’t be an issue as you aren’t changing the order or the type.
The same goes for decoupling storage, though you need to watch out for the order that you extend the contracts, so you are adding AStorage last and keeping the order of the state variables when the contract is linearized. (Solidity Diamond Inheritance)

I recommend thorough testing. The OpenZeppelin Upgrades Plugins can be a good way to do this and I have an example showing testing the upgrade:

With your upgrade function manualInitialize you are protecting it with onlyOwner. You may want to include logic to ensure it is only called once, such as checking the state of a variable, otherwise the owner should call this multiple times.

1 Like
3

@abcoathup thanks for the answer. I don’t want to change the order of the variables nor the type of the variables just to be clear, I know that storage layout will become corrupted so that’s a no-no :slight_smile:

Ok for the Initializable (because It will change the order of the variables) and yes my point was to being sure that by adding AStorage at the end was a safe operation, considering that AStorage does not inherit anything from other contracts should be ok, but need to better check linearization; for the manualInitialize yes I have some code which prevents a double initialization.

Btw for sure makes sense to test if everything is working fine and thanks for the links!

1 Like