Add support for `__gap`

[ZeroEkkusu]

__gaps can be processed after the layouts are overlayed and aligned.

Edit: See 'Update' at the end for easier approach! 👇

How to do this:

Let's say that we have a contract X.

This is its storage layout:

x1
x2
__gap [10]

Gaps are often used when developing upgradeable smart contracts. See OpenZeppelin's documentation.

Let's say we update X to look like this:

x1
x2
x3
x4
__gap [8]

(assuming x3 and x4 occupy one slot each)

We added two new variables (x3 and x4), and correctly shrink the gap by two (10 => 8).

However, if we run Storage Delta, these variables will be marked!

We need to identify that these variables are in the storage space previously reserved by the gap, and de-escalate the findings. I recommend taking a look at alignedOldVisualized and alignedOverlayedVisualized JSONs to think about it.

Keep in mind that there can be multiple __gaps in a contract (regex: /^__gap/).

Additionally, we need to make sure gaps have been shrunk by the right amount.

Update

There may be a straightforward way to solve this. We won't be changing layout JSONs at all.

First, we determine 'gap spaces' from the old layout JSON. For example, let's say there are gap spaces at positions 10-14 and 24-26. Thats's 2 gap spaces of sizes 4 and 2. We record this for future reference.

The first rule is to de-escalate findings for variables in a gap space. For example, 🏴 becomes 🌱, and so on. This can be done by always asking, "Is the variable in a gap space?" before we write to the report, and de-escalating if so.

The second rule is that we print an empty line to the old report for each variable that has been de-escalated. This way our reports will remain aligned.

The third rule is to make sure that the gap space is either filled completely, or contains a gap at the end, correctly shrunk based its current position and its old position. Gaps can be of any type, so we should use size to perform the check.

[kamuik16]

Hey @ZeroEkkusu!

I made some changes in the script and got output something like this.

Old Contract:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

contract Counter {
    uint256 public a;
    uint256 public b;
    uint256[48] public __gap;
}

New Contract:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

contract Counter {
    uint256 public a;
    uint256 public b;
    uint256 public c;
    uint256 public d;
    uint256 public e;
    uint256 public f;
    uint256[44] public __gap;
}

Old Report:

    0          a                            uint256
    1          b                            uint256

New Report:

    0          a                            uint256
    1          b                            uint256
🔻  2          c                            uint256
🔻  3          d                            uint256
🔻  4          e                            uint256
🔻  5          f                            uint256
🏴  6          __gap                        uint256[44]

🔻 is a new symbol I added to show that this variable is filled in the storage that was reserved by the __gap . I still have some changes to do in which I will need your help but is this something you would like to see?

One problem is __gap is not printing in the Old report and if I try to it is throwing an error, so I will need your help there. And another one is if the data type is other than the uint256 it shows an error something like this

let offset = item.offset.toString().padEnd(5, " ");
                           ^

TypeError: Cannot read properties of undefined (reading 'toString')

[ZeroEkkusu]

Maybe you can push to a branch on your fork so we can take a look?

[kamuik16]

Maybe you can push to a branch on your fork so we can take a look?

Yes I have pushed my changed to my forked repo. Here's a link to the repo: https://github.com/kamuik16/storage-delta/tree/issue8

[ZeroEkkusu]

Thanks; let me get on this tomorrow moring.

[kamuik16]

Thanks; let me get on this tomorrow moring.

One thing I realized is that I have used an array for storing gaps but there can be only one __gap variable so ignore the array logic in the code because that can be changed for a single variable instance.

[ZeroEkkusu]

It may also be useful if we create a draft PR for easier reviewing. Also, I'll be able to push something if needed that way.

I have used an array for storing gaps but there can be only one __gap

There can be more than one __gap (and usually, there is!). The reason for that is because __gaps are usually private variables.

Let's say we have contracts A, B, and C. Where A is B, C (meaning that inherits them). Let's say that each contract has a private variable __gap. That means that A will actually have 3 __gaps! You can look at the storage layout yourself if you want to confirm it.

So, your approach with an array was correct.

[kamuik16]

It may also be useful if we create a draft PR for easier reviewing. Also, I'll be able to push something if needed that way.

I have used an array for storing gaps but there can be only one __gap

There can be more than one __gap (and usually, there is!). The reason for that is because __gaps are usually private variables.

Let's say we have contracts A, B, and C. Where A is B, C (meaning that inherits them). Let's say that each contract has a private variable __gap. That means that A will actually have 3 __gaps! You can look at the storage layout yourself if you want to confirm it.

So, your approach with an array was correct.

Oh, okay. So what do you think of my approach? Also any ideas about solving the errors I am facing?

If you say I'll push my changes and create a draft PR.

[ZeroEkkusu]

i think we're on the right path:

1. instead of detecting gaps in createLayout, let's just add another step (add a new helper function that iterates over our 'old' JSON)
2. we'll probably need to change the line that uses variableFitInGap, but i need to debug it with some example first

yep, feel free to open a draft PR!

---

there's an edge case where a variable can start before a gap and 'spill' into it:

struct S {
    uint256
}

0   S
1   _gap

but if we change the struct to:

struct S {
    uint256
    bool
}

the layout now looks like this:

0   S
2   _gap

for this, i think we need to make another helper function onlyClashesWithGap that takes an item from the new layout, and the old layout. it then iterates over the old layout to check if the item from the new layout only conflicts with the gap.

we'll figure out what to do with it later.

[kamuik16]

i think we're on the right path:

1. instead of detecting gaps in createLayout, let's just add another step (add a new helper function that iterates over our 'old' JSON)
2. we'll probably need to change the line that uses variableFitInGap, but i need to debug it with some example first

yep, feel free to open a draft PR!

there's an edge case where a variable can start before a gap and 'spill' into it:

struct S {
    uint256
}

0   S
1   _gap

but if we change the struct to:

struct S {
    uint256
    bool
}

the layout now looks like this:

0   S
2   _gap

for this, i think we need to make another helper function onlyClashesWithGap that takes an item from the new layout, and the old layout. it then iterates over the old layout to check if the item from the new layout only conflicts with the gap.

we'll figure out what to do with it later.

Opened a draft PR: https://github.com/0xPolygon/storage-delta/pull/16