ERC-7201 base contract

[aviggiano]

🧐 Motivation

Hello

The "Namespaced Storage" pattern introduced in Contracts v5 is a great feature, and will certainly help mitigate many problems associated with storage layout changes in upgradeable contracts.

However, the general issue still exists.

Since the implementation of ERC-7201 is only available for the OpenZeppelin library, and not enforced anywhere on users' contracts, developers can still make the same mistakes by messing with inherited contracts' storage variables.

Given that the thorough implementation of "Namespaced Storage" can be a bit challenging, requiring a little bit of inline assembly to fetch and update the storage, I suggest an ERC-7201 base contract that users can inherit from and use the diamond storage pattern everywhere.

📝 Details

As a Solidity developer, I would like to have a ERC7201BaseContract that all my custom contracts extend from, with helper methods that make it easy to retrieve and update data from storage.

[ernestognw]

Hi @aviggiano!

I agree messing with the storage layout is still a risk when using upgradeable contracts. However, that can be avoided by using @openzeppelin/upgrades since they have upgrade safetiness checks that also support the namespaced storage pattern (soon to be out for Foundry users as well).

However, the general issue still exists.

Yes, ideally we would have built-in safety instead of requiring checks. That's why we decided to implement EIP-7201 in upgradeable contracts, which is where the storage layout incompatibilities might happen. Aside from that, I don't see how it might be an issue for regular contract users.

Can you provide an example of how storage layout incompatibilities are still a risk even after introducing EIP-7201?

Regarding an ERC7201 base contract implementation, seems possible to do something like:

contract ERC7201Base {
  bytes private immutable STORAGE;

  constructor(string memory name) {
    STORAGE = keccak256(abi.encode(uint256(keccak256(name)) - 1)) & ~bytes32(uint256(0xff))
  }

    function getStorage() internal pure returns (?? storage $) {
        assembly {
            $.slot := STORAGE
        }
    }
}

But, I think the storage struct should be declared outside and we already have an storage slot util.

Overall, EIP-7201 defines a storage-derivation formula, but not actually a general implementation of a namespaced contract.

[aviggiano]

Hello @ernestognw

Thank you for your response.

Aside from that, I don't see how it might be an issue for regular contract users.

The problem I see is that smart contract developers may see the v5 announcement as a solution for all their upgradeability woes, when in reality they will still need to follow the same upgradeable rules from before.

The only guarantee they have is that they don't need to worry about v5 internal updates, since each one of the OpenZeppelin extended contracts has Namespace Storage.

The problem is that users' own contracts will still not have Namespaced Storage. Maybe they will have to manage __gap by themselves, maybe they will need to manually try to avoid storage clashes, etc.

This allows for the same bugs as before to exist:

// MyToken.sol
contract MyToken is
    GreetingToken,
    HappyToken,
    Initializable,
    ERC20Upgradeable,
    OwnableUpgradeable,
    UUPSUpgradeable
{
    /// @custom:oz-upgrades-unsafe-allow constructor
    constructor() {
        _disableInitializers();
    }

    function initialize(address initialOwner) public initializer {
        __ERC20_init("MyToken", "MTK");
        __Ownable_init(initialOwner);
        __UUPSUpgradeable_init();

        happy = true;
    }

    function _authorizeUpgrade(
        address newImplementation
    ) internal override onlyOwner {}
}

// GreetingToken.sol
abstract contract GreetingToken {
    uint256 public version;

    function greet() public pure returns (string memory) {
        return "Hello World!";
    }
}

// HappyToken.sol
abstract contract HappyToken {
    bool public happy;

    function isHappy() public view returns (bool) {
        return happy;
    }
}

Now the developer updates the implementation of an inherited contract that does not follow EIP-7201

// GreetingToken.sol
abstract contract GreetingTokenV2 {
  uint256 public version;
  bool public shouldGreet;

  function greet() public returns(string memory) {
    return shouldGreet ? "Hello World!" : "Goodbye";
  }
}

And there's a storage collision between GreetingTokenV2.shouldGreet and HappyToken.sad

Full POC here

[aviggiano]

My suggestion is that by providing a ERC7201Base contract, EIP-7201 could be enforced everywhere.

I think your proposal is a good initial step, but some helper functions might still be required so that the internal intricacies of dealing with storage can be abstracted away, not sure exactly how.

For example, suppose I want to develop MyToken with deriving contracts using EIP-7201:

abstract contract GreetingTokenV2 is ERC7201Base {
  struct GreetingTokenStorage {
    uint256 version;
    bool shouldGreet
  }

  // maybe ERC7201Base uses this user-defined constant?
  bytes32 private constant STORAGE = keccak256(abi.encode(uint256(keccak256("storage.GreetingToken")) - 1)) & ~bytes32(uint256(0xff));

  function greet() private returns (bool) {
    // some magic happens here, not sure how
    // ideally, ERC7201Base abstracts away the creation of the boilerplate necessary from ERC-7201
    return _getStorage().shouldGreet ? "Hello World!" : "";
  }
}

[ernestognw]

Gotcha! Thanks for such a clear example @aviggiano.

After discussing it internally, we agree storage collisions are still something to worry about when using custom contracts and we'd like to provide a good solution.

Ideally, namespaced storage would be built in Solidity (eg. with a variable flag struct Storage namespaced("...") { ... }), but we see this very unlikely to happen in the short term. The alternative you suggest is to build an ERC7201 that abstracts away the initial boilerplate and I personally agree with the approach. However, the main issue is that the storage struct can't be abstracted away.

Having Solidity "generics" (already considered for Q3 2024) would be helpful since we could do:

contract ERC7201Base {
  bytes private immutable STORAGE;

  constructor(string memory namespace) {
    STORAGE = keccak256(abi.encode(uint256(keccak256(name)) - 1)) & ~bytes32(uint256(0xff))
  }

    function getStorage<T>() internal pure returns (T storage $) {
        assembly {
            $.slot := STORAGE
        }
    }
}

But currently, each user would need to define their own getStorage function and I don't think they should reimplement it each time. I don't see an easy workaround for that.

// maybe ERC7201Base uses this user-defined constant?
bytes32 private constant STORAGE = keccak256(abi.encode(uint256(keccak256("storage.GreetingToken")) - 1)) & ? ~bytes32(uint256(0xff));

Regarding a user-defined constant, the EIP is actually the formula. Letting the user bypass it wouldn't be a proper EIP-7201 implementation. I'd stick to the immutable variable and a constructor string memory namespace.

Note that a solution for this is not necessarily code in the library but perhaps a guide in the docs. We're still updating the documentation with the newest 5.0 changes so we can also go that way imo. To be clear, not discarding an ERC7201Base contract but I don't see how much value it can provide as an standalone contract.

[aviggiano]

Hi @ernestognw

I'm glad you agree this feature can be beneficial to smart contract developers.

About Solidity's "generics", sounds like it would help reduce the boilerplate in the implementation of ERC-7201. Meanwhile, indeed this doesn't need to be a base class included in the library, so maybe a documentation explaining how to implement this standard is enough.

[DRIVENpol]

I'm seeking advice on the most effective way to implement ERC7201 in a smart contract that already inherits from Initializable, ERC20Upgradable, and other similar contracts.

From my current understanding, to make a smart contract ERC7201 compliant, one should encapsulate all variables within a struct. Is this understanding correct? Additionally, in a scenario where there are no extra variables, is it still necessary to implement a struct, or can this step be omitted?

[ernestognw]

In reality, ERC 7201 only defines the Natspec annotation for a storage location (@custom:storage-location) and a formula to derive a slot that's guaranteed to avoid collisions in both Solidity and Vyper.

Take the following contract as an example:

contract MyContract {
  uint256 x;

  function getX() returns(uint256) {
    return x;
  }

  function setX(uint256 y) {
    x = y;
  }
}

Then apply the following steps:

1. Wrap the storage variables (i.e. x) in a struct
2. Define a namespaced id that's unique in the inheritance tree (e.g. openzeppelin.storage.MyContract)
3. Add the NatSpec annotation to your struct. (⚠️ This step is important! It allows tooling like OpenZeppelin Upgrades to look for misconfigurations (e.g. collisions due to a repeated namespaced ID in the inheritance tree))
4. Add a constant location pointer with the value obtained from the ERC 7201 formula. For openzeppelin.storage.MyContract it would be: keccak256(abi.encode(uint256(keccak256("openzeppelin.storage.MyContract")) - 1)) & ~bytes32(uint256(0xff))
5. Define a storage getter that retrieves the storage struct from the calculated storage pointer.
6. Use the storage getter for both writing and reading values to storage.

The result should be like this:

contract MyContract {
  // 1) 2) and 3)
  /// @custom:storage-location erc7201:openzeppelin.storage.MyContract
  struct MyContractStorage {
    uint256 x;
  }

  // 4)
  // keccak256(abi.encode(uint256(keccak256("openzeppelin.storage.MyContract")) - 1)) & ~bytes32(uint256(0xff))
  bytes32 MY_CONTRACT_STORAGE_LOCATION = 0x39a23e8607d7306e7523a088572477d066e708824f79fb6d8838d6f8bddff400;

  // 5)
  function _getMyContractStorage() private pure returns (MyContractStorage storage $) {
      assembly {
          $.slot := MY_CONTRACT_STORAGE_LOCATION
      }
  }

  // 6)
  function getX() public returns(uint256) {
    return _getMyContractStorage().x;
  }

  // 6)
  function setX(uint256 y) public {
    _getMyContractStorage().x = y;
  }
}

Additionally, in a scenario where there are no extra variables, is it still necessary to implement a struct, or can this step be omitted?

If there are no variables, it doesn't make sense to create a namespace, but I'm not sure if this is what you're asking. Can you elaborate on what "no extra variables" mean? If that means your contract inherit from OZ upgradeable contracts and you're not adding new variables, then yes, you can omit it.

[DRIVENpol]

Yes, this is what I was looking for "your contract inherit from OZ upgradeable contracts and you're not adding new variables". Thank you very much!

[Amxx]

Resolved (to the limits of what solidity allows) in #4975

Should be reopenned when next solidity support implementing that at a lower level in a standard library.