Support storage layout gaps

[frangio]

We often recommend including a storage "gap" in storage layout in order to safely add storage variables when using (multiple) inheritance. We use this pattern ourselves in OpenZeppelin Contracts Upgradeable. However, if a slot in the gap is actually used for a new variable later, the plugins will flag this as an error. We should detect gaps through the conventional __gap name and treat them specially for layout compatibility: allow replacing gap slots with new variables in an upgrade.

[aquiladev]

Is there estimation for the feature? We need it

[Amxx]

@aquiladev unfortunately there isn't. I'll see if I can bump it, as I believe it would be valuable ... but I don't want to make any promise I can't keep.

[itsermin]

@aquiladev unfortunately there isn't. I'll see if I can bump it, as I believe it would be valuable ... but I don't want to make any promise I can't keep.

This is quite limiting unfortunately for automating upgrades

[aquiladev]

@itsermin so far I see next options:

1. Hack storage layout history in order to run upgrade (the option hard to automate)
2. Use storage slots, in order to avoid storage layout change
3. Wait for _gap support in the tool (change storage layout only through append)

[roshanr95]

It would be great if you could prefix match ____gap instead of exact. Allows for adding multiple gaps as well as giving them more meaningful names.

[sullof]

Solving this issue is quite important.

First, you suggest an approach that does not work as intended.

Second, the entire assumption is that you can write upgradeable contracts, but if the storage cannot be upgraded with new variables, the entire castle crashes, forcing to move towards other solutions for upgradeability.

Long time is passed since the issue has been open. Is there any news?

[ericglau]

@sullof Thank you for your feedback. We have recently started working on this.

[sullof]

BTW, I wonder if you are thinking of supporting this:
https://github.com/GeniusVentures/openzeppelin-contracts-diamond
I made some experiments and it works quite well, solving the issue with the changes in the storage.
Still not perfect, but much better.

[ericglau]

BTW, I wonder if you are thinking of supporting this: https://github.com/GeniusVentures/openzeppelin-contracts-diamond I made some experiments and it works quite well, solving the issue with the changes in the storage. Still not perfect, but much better.

@sullof
For OpenZeppelin Contracts, that is being considered for the next major release. See OpenZeppelin/openzeppelin-contracts#2964.
For Upgrades Plugins, there is a related issue #459

[sullof]

Thanks. When do you plan the next major release?

[ericglau]

@sullof It would come after a Solidity version that introduces breaking changes is released.

[sullof]

That makes sense, thanks.

Since our contracts are audited and ready to go, we will deploy the contracts based on current version.
Do you still plan to fix the issue with gaps, i.e., to support the current version in the future?

[ericglau]

@sullof We still plan to fix this issue, and plan to continue support using the upgrades plugins with the current version of Contracts (4.x).

[frangio]

Please note that when using the diamond storage pattern, storage layout is currently not checked at all for incompatibilities. A future version will recognize the pattern and compare the structs between versions.

---

Starting this week we are actively working on adding support for gaps. We are currently analyzing the best way to modify the layout comparison algorithm to work with gaps.

The latest idea is that we should preprocess layouts to remove gap variables and leave in actual gaps, then let the levenshtein algorithm recognize the variables that are inserted in the middle. Insertions are currently considered errors though, so we need to change something else to accept them while continuing to reject "bad" insertions. An idea here is that we could work in two modes: the current mode where insertions and deletions are disallowed, and an alternate mode that is used if we have precise storage layout information (slots and offsets) in which we rely solely on 'layoutchange' errors and allow insertions, deletions, etc.

[sullof]

Thanks. I am sticking with the version 4 and not using Diamond, since you will continue to support V4.

I think that whichever solution you choose, it is very important that you support the __gap array approach. We all followed your recommendation and expect that inserting a variable while reducing the length of the array works without errors.

Maybe you can just allow to ignore the error and proceed anyway. Everyone tests upgrades on testnet before doing the same on mainnet.

[frangio]

Errors can be ignored with the use of the unsafeSkipStorageCheck flag. This is not recommended, and we expect to support gaps out of the box in the near future.

[mudgen]

It would be good if this feature included detecting storage changes for upgrades that use diamond storage. Related issue here: #459

[frangio]

@mudgen These are separate features that have to be tackled in different ways.

[mistersingh179]

Errors can be ignored with the use of the unsafeSkipStorageCheck flag. This is not recommended, and we expect to support gaps out of the box in the near future.

@frangio will it be possible to do unsafeSkipStorageCheck: true only for the 2 lines where new variable is added and the __gap is reduced in size? this way the plugin can still check the rest of the contract for storage conflict issues.

[ericglau]

will it be possible to do unsafeSkipStorageCheck: true only for the 2 lines where new variable is added and the __gap is reduced in size? this way the plugin can still check the rest of the contract for storage conflict issues.

@mistersingh179 There are a set of checks that can be skipped for specific lines (documented here and here), but those do not cover the gaps scenario.

When this issue is completed, gaps should be supported (without needing docstring comments).

[frangio]

Sharing my proposed design notes for this feature.

We currently have a levenshtein module that computes a minimum cost edit distance between two storage layouts. A levenshtein edit distance consists of a list of insertion, deletion, and substitution operations that convert one layout into the other. My proposal is to generalize this to a custom edit distance with operations designed for our domain, with something like a "gap shrink" operation. Then, using the levenshtein module (which really stops being about levenshtein and becomes a generalized Wagner-Fischer implementation) we compute the minimum edit distance with the custom operations. That edit distance may include some insertion operations, i.e. storage variables added in the middle of the layout, which we would normally disallow, but we can now allow them if they are followed by a matching gap shrink operation of the right size.

Furthermore, we should tune the costs of operations to make sure that the minimum cost edit distance is the one that would include the operations we're looking for. This should be simply a matter of giving lower cost to operations that we want to prioritize, e.g. the gap shrink should be a low cost operation.

[adaki2004]

Hello Guys,
Any updates from your side ? We also in a situation where would like to automate deployments, but this prevents us from exchanging the ___gap.

[ericglau]

This is now available in the latest version of @openzeppelin/upgrades-core which is used by the plugins. To update, run npm install @openzeppelin/upgrades-core@latest

For details on storage gaps, see docs.

[roshanr95]

Can we do a prefix match instead of exact match please? Allows for adding multiple gaps as well as giving them more meaningful names.

[frangio]

@roshanr95 Please open an issue!

[roshanr95]

Done, thanks. #657

[itdream-dev]

Hi @frangio , @roshanr95 , I'm still getting the issue when upgrading the proxy by just adding a new state variable and reducing size of gap.

New variables should be placed after all existing inherited variables
Upgraded __gap to an incompatible type

• Bad storage gap resize from 46 to 45
  Size decrease must match with corresponding variable inserts
  Could you please help me how to solve it?

[frangio]

@skyclean906 Please open a new issue and share the code if possible.

[itdream-dev]

Yes @frangio, I opened a new issue here.
#698