openzeppelin-upgrades icon indicating copy to clipboard operation
openzeppelin-upgrades copied to clipboard

Add support of packing / unpacking structure during storage layout verification.

Open Amxx opened this issue 4 years ago • 2 comments

Let's say we have an ERC20Votes contract. This contract contains 3 consecutive slots dedicated to the voting logic:

    mapping(address => address) private _delegates;
    mapping(address => Checkpoint[]) private _checkpoints;
    Checkpoint[] private _totalSupplyCheckpoints;

This storage and the associated logic would benefit from being abstracted away, so it can be reused for ERC721 voting or ERC1155 voting (for example).

This is what this structure proposes. You'll note that, at a low level, the storage is consistent. It is however hidden in structs.

    struct Checkpoint {
        uint32 index;
        uint224 value;
    }

    struct History {
        Checkpoint[] _checkpoints;
    }

    struct Votes {
        mapping(address => address) _delegation;
        mapping(address => History) _userCheckpoints;
        History                     _totalCheckpoints;
    }

I believe that it would be a great improvement if the plugin was able to flatten the structure so that this would trigger a rename error, but not replace/delete/insert errors.

This branch include tests for such a feature

Amxx avatar Sep 28 '21 10:09 Amxx

A naive implementation would not be correct because structs don't necessarily align in storage the same way as their contents. We've seen this recently with struct Timestamp { uint64 t; }, which is not equivalent to using the uint64 directly.

If we implement support for users packing variables into structs, we need to do it using the storage layout emitted by solc (#3) because it contains detailed information about alignment.

frangio avatar Nov 29 '21 22:11 frangio

It is true that struct do force some data alignment which means that packing/unpacking is not as simple as that. However, in cases such as the one presented above, packing would be consistent.

I believe it would be indeed be interesting to refactor the storage compatibility check mechanism to use the storage layout emitted by solc. It would be more versatile (possible support for packing) and safer (be basically exploit data from the source)

Amxx avatar Nov 29 '21 22:11 Amxx