Use diamond storage instead of gaps in upgradeable contracts

[a-d-j-i]

🧐 Motivation
Gaps are problematic and this: https://eips.ethereum.org/EIPS/eip-2535#facets-state-variables-and-diamond-storage seems to be a great idea.

📝 Details
The proposed changes look like: OpenZeppelin/openzeppelin-contracts-upgradeable#134
I think this will improve the usability of upgradeable contracts and avoid a lot of common mistakes that are hard to fix.

[frangio]

Thank you for the proposal @a-d-j-i. I think this is actually a very good idea for the upgradeable contracts, but we will need to carefully consider it for the next major release because it is a breaking change.

Even if we agree that the breaking change is warranted, we are still somewhat concerned about people unknowingly upgrading their existing contracts to an incompatible version, but I wonder if we can have the contract self-check if it comes from a previous incompatible version, by introducing a notion of layout versioning.

Can you please expand on these points:

Gaps are problematic
I think this will improve the usability of upgradeable contracts and avoid a lot of common mistakes that are hard to fix.

[a-d-j-i]

The problem are not the gaps themselves but the “technique”:

• You still depend on the inheritance order.
• It requires a lot of attention from the user, forgetting a gap or adding variables without adjusting it can result in a disaster.
• The gap itself is a line of code that a lot of people don't understand, and don’t know why it is there.

[ghost]

What is the status of this issue?

[frangio]

@byterose As I mentioned above:

we will need to carefully consider it for the next major release because it is a breaking change.

There is currently no major release planned for the near future.

[SuperGNUS]

This is being worked on externally from the OpenZeppelin community right now. I'll ask OZ for a repo to move it to their organization once finished. https://github.com/GeniusVentures/openzeppelin-contracts-diamond

The transpiler update is here: https://github.com/GeniusVentures/openzeppelin-transpiler

Need to generate public getters, fix one bug and do write some testing for it. If anyone wants to help contribute, feel free to jump in.

[frangio]

We don't really want to maintain a third variant of the repo, maintaining two is already significant workload, so if we go this route we would make it the default Upgradeable variant.

It's great that you're working on this though, and you're welcome to maintain your fork.

Are you planning to use this for your project?

[SuperGNUS]

We don't really want to maintain a third variant of the rpeo, maintaining two is already significant workload, so if we go this route we would make it the default Upgradeable variant.

It's great that you're working on this though, and you're welcome to maintain your fork.

Are you planning to use this for your project?

Yes, planning on using this for the project. It now fully transpiles/compiles OpenZeppelin v4.4.1 and I'm about to npm publish it very shortly.

[SuperGNUS]

OpenZeppelin Contracts Upgradeable Diamond

Master Branch is compiling and I published it. I really wish it could be under @OpenZeppelin organization because the documentation needs to change otherwise.

Transpiled Upgradeable OpenZeppelin Contracts using Diamond Pattern is available for OZ 4.4.1

npm published package

<- or ->

npm install @gnus.ai/contracts-upgradeable-diamond

Forked github Repo

[mudgen]

@frangio When or if OpenZeppelin changes its implementations to use diamond storage (or whatever you want to call it) I am very supportive of OpenZeppelin having its own implementation of EIP2535 Diamonds. And of course OpenZeppelin may use as desired any of the code I have written for it. There are more than 40 projects using diamonds now and more coming. A list is here: https://eip2535diamonds.substack.com/p/list-of-projects-using-eip-2535-diamonds?s=w

[a-d-j-i]

@GVTopCoder There is a way to configure the transpiler so we can make it backward compatible (like taking the storage info as input) ?

[frangio]

@a-d-j-i What kinds of backwards compatibility are you looking for?

[a-d-j-i]

I was thinking that if I can set the slots (instead of using the hash), I can use the same code to evolve a contract that was deployed with gaps. It is a little bit tricky and not a general solution.

[frangio]

@a-d-j-i In the fork by @GVTopCoder there is no way to update that. The way we're planning to adopt the diamond storage pattern in our next major is by defining internal functions like _getAccessControlStorage and you might be able to override that and return a different location, though that would not be recommended usage and 5.x will not be storage compatible with the current 4.x release.

[SuperGNUS]

I was thinking that if I can set the slots (instead of using the hash), I can use the same code to evolve a contract that was deployed with gaps. It is a little bit tricky and not a general solution.

All those contracts were generated by an updated version of OZ's transpiler. https://github.com/GeniusVentures/openzeppelin-transpiler. You could probably modify the generators and add a support library and have all the slots in a mapping (bytes32 => bytes32) slotMap in a slot library, so now a call to slotLibrary.sol

library SlotLibrary {

    struct Layout {
         mapping(bytes32 => bytes32) slotMap;
    }

    bytes32 constant SLOTLIB_POSITION = keccak256("SlotLibrary.slotmapper");

    // this function does NOT use map entry
    function layout() {
              bytes32 position = SLOTLIB_POSITION;
        assembly {
            ds.slot := position
        }
    }
    
  function getSlot(byte32 originalKeccak) returns(bytes32) {
    bytes32 newSlot = layout().slotMap[originalKeccak];
    if (uint256(newSlot)  == 0) {
      newSlot = originalKeccak;
    }
    return newSlot;
  }

  function setSlot(bytes32 slot, bytes32 newSlot) {
      layout().slotMap[slot] = newSlot;
  }
}

instead of using the the predefined SLOT

Now ERC20Storage.sol

import "SlotLibrary.sol";

library ERC20Storage {
     using SlotLibrary for SlotLibrary.Layout;

      const bytes32 originalSlot = keccak256("ERC20.storage");
      
      function layout() {
         bytes32 mapSlot = SlotLibrary.getSlot(originalSlot);
         assembly {
            ds.slot := mapSlot
        }
      }
       
       function setSlotMapping(bytes32 newSlot) {
              SlotLibrary.setSlot(originalSlot, newSlot);
       }
}

[mudgen]

@a-d-j-i In the fork by @GVTopCoder there is no way to update that. The way we're planning to adopt the diamond storage pattern in our next major is by defining internal functions like _getAccessControlStorage and you might be able to override that and return a different location, though that would not be recommended usage and 5.x will not be storage compatible with the current 4.x release.

That sounds great to me.

[zdenham]

@a-d-j-i In the fork by @GVTopCoder there is no way to update that. The way we're planning to adopt the diamond storage pattern in our next major is by defining internal functions like _getAccessControlStorage and you might be able to override that and return a different location, though that would not be recommended usage and 5.x will not be storage compatible with the current 4.x release.

Seconding this, I would love to know if diamond storage will be adopted in the next major release and if so, when that will be out--just ran into a storage conflict issue on a local contract that diamond storage would have resolved!

[frangio]

Yes, we want to adopt diamond storage for the next major release, but we don't have an estimated release date. It will most likely happen whenever Solidity 0.9 is released, and as far as we know there is no date for that either.

[jcbdev]

I guess my question @frangio and @GVTopCoder is even if this not available for a long time would it be possible that the openzeppelin official version uses the same slot names as @GVTopCoder's version? (or at least align on the naming convention now)

That way early adopters could use this version now in the knowledge that when they migrate later to an official implementation the storage is compatible and they can then swap one library for the other?

If you ended up doing an implementation that didn't match the storage slots then anyone use uses the community version is stuck on the fork.

[frangio]

Sorry, as warned in the readme for the upgradeable contracts:

There will be storage incompatibilities across major versions of this package, which makes it unsafe to upgrade a deployed contract from one major version to another, for example from 3.4.0 to 4.0.0.

This means that even if we used the same convention for storage slots, we may reorder or fully change the state variables of contracts, so it is not safe to upgrade across major versions.

[mdnorman]

I guess my question @frangio and @GVTopCoder is even if this not available for a long time would it be possible that the openzeppelin official version uses the same slot names as @GVTopCoder's version? (or at least align on the naming convention now)

One problem is that they misspelled OpenZeppelin openzepplin, so I don't think it's a good idea to keep that misspelling in the real OZ contracts. Maybe they did that on purpose, but I don't know.

[novaknole]

Hi...

What's the update about this ?

[novaknole]

@a-d-j-i

Hi Andres..

As far as I understand, what you propose really solves the problem where you don't have to think about inheritance order to not mess up the gap which is a big direction forward. Though, I think your solution doesn't fix that problem - with your solution - you can't really replace/delete variables in the struct(only append). What do you think ?

Though, that inheritance chain with gaps is madness in terms of complexity...

[mdnorman]

Though that appears true, if you decompose the base contracts enough, this isn't that big of a problem. You can leave gaps in the structs as needed.

However, if you have large base contracts (a potential problem in and of itself), you can split the storage up into multiple structs with a different location for each. That way, each can grow independently.

I did this with a Diamond facet recently, but then split up into more facets.

[novaknole]

That though doesn't change the fact that i can't remove variable from the struct neither re-order it, unless each struct only contains one variable and for each struct , we use custom hash location.

[SuperGNUS]

Hi @OpenZeppelin Team,

I want to use this library for a well-funded DAO called Gam3r/Game7.io they have grants available for initiatives. Does OZ have a grant program wherein I could maintain this and the transpiler?

Currently, I maintain the main branch of this in the Genius Ventures GitHub, but I'm just trying to figure out where to maintain the main branch and a way for me to work on EIP2585 contracts using OZ libraries permanently.

[frangio]

@GVTopCoder How can I reach out to you to discuss and understand this better?

[SuperGNUS]

Sent an email.

[therealharpaljadeja]

Is DiamondStorage still under consideration?

[SuperGNUS]

Is DiamondStorage still under consideration?

I wouldn't bet on it any time soon.

But, why not just use this?

#2964 (comment)

[srini-silks]

Is there a publicly audited version of the DiamondStorage implementation?

[zdenham]

Is there a publicly audited version of the DiamondStorage implementation?

Do you mean an implementation within OZ itself? There are several for the diamond standard itself: https://github.com/mudgen/awesome-diamonds#security-audits

Many of which use diamond storage

[mudgen]

OpenZeppelin upgradeable contracts now use Diamond Storage, so in theory they should now be compatible with EIP-2535 Diamonds.

[frangio]

Fixed in #4534 with "namespaced storage" as specified in ERC-7201. As an example, you can find the code for ERC20 here, but note that this is unreleased 5.0 code. A release candidate will be available next week.

Support for checking storage layout consistency in namespaces across upgrades is being implemented in OpenZeppelin/openzeppelin-upgrades#876.

[a-d-j-i]

Great!!!.
I have one question though: how you override _getERC20Storage() if it is private ?

[frangio]

Unfortunately these functions are not overrideable. If you need to do that you will need to fork the code.

[a-d-j-i]

What is the problem with: function _getERC20Storage() internal virtual pure returns (ERC20Storage storage $) ?

[frangio]

Internal and virtual functions make it very difficult for us to design and assess changes for backwards compatibility.