search

OpenZeppelin / openzeppelin-contracts-upgradeable

Upgradeable variant of OpenZeppelin Contracts, meant for use in upgradeable contracts.
https://openzeppelin.com/contracts
MIT License
1k stars 436 forks source link

introduce storage gap to 'Initializable' contract #164

Closed ChaoticWalrus closed 2 years ago

ChaoticWalrus commented 2 years ago

In keeping with the other contracts in this repository, the 'Initializable' contract should have a storage gap of length 49, to reflect the single storage slot currently taken by the two defined variables. Without this storage gap, future changes to the 'Initializable' contract may cause shifts in storage slots for variables defined downstream in the inheritance pattern. I note as well that 'Initializable' should properly be named 'InitializableUpgradeable', although changing this would touch more files.

Amxx commented 2 years ago

Hello @ChaoticWalrus

No, Initializable should NOT have a gap. It never had, and this change would break the storage compatibility of literally all the upgradeable contracts.

Initializable is a specific contract, and it is on purpose that it is not transpilled (not renamed & no gap added)

ChaoticWalrus commented 2 years ago

It's not my fault OpenZeppelin didn't plan ahead on this. As-is, this means the storage layout of the 'Initializable' contract cannot change. If the storage gap is truly necessary for contracts like 'Ownable' or 'Pausable', then the same reasoning applies to 'Initializable'. OZ has already seen fit to change the storage of 'Initializable' (albeit changing a bool to a uint8, so no actual byte changes); regardless, "we don't plan on changing it" is not a good answer. I realize you have legacy support in mind -- perhaps create InitializableUpgradeable as a separate contract if you want to support full contract stack upgradeability in the future then.

frangio commented 2 years ago

It's not my fault OpenZeppelin didn't plan ahead on this.

Of course it's not your fault. :slightly_smiling_face:

The storage of Initializable was changed from bool to uint8 precisely because it doesn't affect storage layout. This is a completely different discussion because it changes storage layout.

Initializable actually used to have a gap a few major versions ago. The fact that it doesn't have a storage gap now was an oversight that was identified when it was already "too late" in terms of backwards compatibility.

Without this storage gap, future changes to the 'Initializable' contract may cause shifts in storage slots for variables defined downstream in the inheritance pattern.

We consider every change with the lens of storage layout compatibility. This will not happen accidentally, and there are no planned changes that will affect storage layout.