Add a virtual pure function that can be overriden to customize the storage location

[Amxx]

TODO:

• Is there an issue with the /// @custom:storage-location erc7201:${id} annotation being invalid if the function is overriden ? How do we deal with that ?

[ernestognw]

imo the NatSpec of the ${namespace}Location function should note the instructions of overriding. For example:

/// @dev Consider updating the struct's storage location annotation
/// See https://docs.openzeppelin.com/upgrades-plugins/writing-upgradeable#namespaced-storage-layout
function C3StorageLocation() ... virtual {}

Also, I'm pretty sure we can detect overrides to these functions in upgrades plugins, cc @ericglau

[ericglau]

The /// @dev Consider updating the struct's storage location annotation comment makes sense if the user is able to modify the source code of the inherited contract where the struct is defined. By doing so, this would still conform to ERC-7201.

However, if they are inheriting from a contracts library, they may not be able to modify the parent source code's struct annotation. There is no direct link between the struct and the storage location function, so in order to support this, we could introduce further annotations or heuristics. However, this would be outside the scope (and perhaps an extension) of ERC-7201.

Consider the following example from the tests:

        /// @custom:storage-location erc7201:openzeppelin.storage.C2␊
        struct C2Storage {␊
            // a␊
            uint x;␊
            uint z;␊
    ␊
            string s1;␊
        }␊
    ␊
        /// @dev Consider updating the struct's storage location annotation␊
        /// See https://docs.openzeppelin.com/upgrades-plugins/writing-upgradeable#namespaced-storage-layout␊
        function _C2StorageLocation() internal pure virtual returns (bytes32) {␊
            // keccak256(abi.encode(uint256(keccak256("openzeppelin.storage.C2")) - 1)) & ~bytes32(uint256(0xff))␊
            return 0xf05a05e0e3d15983ea921cad031aaea3040e9d631039045748753d29c5d24800;␊
        }␊
    ␊
        function _getC2Storage() private pure returns (C2Storage storage $) {␊
            bytes32 slot = _C2StorageLocation();␊
            assembly {␊
                $.slot := slot␊
            }␊
        }␊

From the AST's perspective, _getC2Storage references the C2Storage struct and the _C2StorageLocation function.
However, C2Storage and _C2StorageLocation do not reference each other.
If a user overrides _C2StorageLocation, simply inspecting the overriding function in the AST would not tell us that the C2Storage struct's location is overridden.

Two considerations:

• Do we need a way for the child contract's overriding function to formally declare that it is using a specific namespace id (e.g. an additional annotation with erc7201:my.custom.location)? Otherwise, the only way to figure out which slot it is using is by examining its return value, and that does not tell us the string id itself.
• Do we need a way to formally declare that a function is overriding a specific namespace, or should we use heuristics based on the pattern in the example above?

[Amxx]

Do we need a way for the child contract's overriding function to formally declare that it is using a specific namespace id (e.g. an additional annotation with erc7201:my.custom.location)? Otherwise, the only way to figure out which slot it is using is by examining its return value, and that does not tell us the string id itself.

I'd say yes.

IMO the user should not modify the source code of the upgradeable contracts, nor should he redefine a structure. I think there should be a way for the user to add an anotation that overrides the /// @custom:storage-location erc7201:openzeppelin.storage.C2 information that is attached to the struct.