StorageSlot library : add storage slot derivation tooling and UDVT typed slot representation.

[Amxx]

First part of #4955, Provides the building block for more transient storage work (reentrancy guard, temporary approvals, ...)

Fixes #4961
Addresses parts of #4696

Note: we decided to keep normal storage support to the UDVT representation, for the sake of compliteness, and because we don't have any security concern with it.

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

🦋 Changeset detected

Latest commit: 40c4943

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[socket-security]

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/[email protected]	Transitive: filesystem	+15	3.77 MB	frangio
npm/[email protected]	environment, filesystem, network, shell Transitive: eval, unsafe	+261	229 MB	kanej

🚮 Removed packages: npm/[email protected], npm/[email protected]

View full report↗︎

[frangio]

Note: we decided to keep normal storage support to the UDVT representation, for the sake of compliteness, and because we don't have any security concern with it.

I want to elaborate on this point just a little more in case I'm able to change your minds 🙂. My concern with mixing these things is that correct use of the abstraction relies on remembering/checking at each use site that the right storage space (transient or normal) is being accessed. The wrong decision at a single use site can result in a massive bug. I would prefer it in my code if I only made this decision at the variable declaration site, and subsequent load/store operations involving this variable automatically access the correct storage space.

[Amxx]

I want to elaborate on this point just a little more in case I'm able to change your minds 🙂. My concern with mixing these things is that correct use of the abstraction relies on remembering/checking at each use site that the right storage space (transient or normal) is being accessed. The wrong decision at a single use site can result in a massive bug. I would prefer it in my code if I only made this decision at the variable declaration site, and subsequent load/store operations involving this variable automatically access the correct storage space.

That is an interresting view. Not sure I share that concern.

[ernestognw]

After reviewing I see @frangio's concerns about mixing transient and regular storage.
In my opinion, it's not a big deal but after reviewing I felt a bit lost navigating the StorageSlot library on its own.

From the Solidity file, there are ~500 lines of code only for "storage slots". I would say it's expected if users get confused with all the options in the library.

Currently, I don't have a strong opinion of how to split this, but splitting Transient from regular Storage (eg. TransientSlots vs StorageSlots) seems a good starting point.

Also side note, but, I couldn't find any plans for Solidity to add a language feature that enables transient storage in a variable. I'm wondering how much this work may overlap with such a feature if they ever release it. I'm feeling this PR is adding a bunch of features that might be as well the language responsibility.

[Amxx]

Also side note, but, I couldn't find any plans for Solidity to add a language feature that enables transient storage in a variable. I'm wondering how much this work may overlap with such a feature if they ever release it. I'm feeling this PR is adding a bunch of features that might be as well the language responsibility.

Its not clear to me how 0.8 is evolving, and at what pace. I think most of the effort is going toward the overall redesign. I agree it would be better is solidity just supported this out of the box ... but we have no clue when that will be possible, and people are activelly asking for some solution to handle transient storage.

[Amxx]

From the Solidity file, there are ~500 lines of code only for "storage slots". I would say it's expected if users get confused with all the options in the library.

SafeCast is over twice as long. I think its ok to have long file if its just many clear variation of a few simple features.

Currently, I don't have a strong opinion of how to split this, but splitting Transient from regular Storage (eg. TransientSlots vs StorageSlots) seems a good starting point.

If we do that, do we :

• remove sstore/sload from the UDVT approach?
  • If not, where do we define the UDVT types?
    • In StorageSlot.sol (with the sstore/ssload) and then import them in TransiantSlot.sol?
    • elsewhere?
• where do we put the namespace & derivation functions? Note that they "apply" to both normal and transient storage.
  • keep them in StorageSlot.sol?
  • dedicated file (SlotDerivation.sol)?

[ernestognw]

The PR is looking good but I'm still not convinced about the design. Sorry for the back and forth but after reviewing I came to the following conclusions:

• TransientSlot is not a "transient" version of StorageSlot, they do very different things.
• Even if we define the custom UDVTs in each StorageSlot and TransientSlot libraries, it shouldn't be a problem because I don't think we want to encourage using them indistinctly (i.e. it's ok that StorageSlot.Uint256SlotType is different to TransientSlot.Uint256SlotType for the compiler)
• Providing any of the StorageSlot struct types (e.g. struct AddressSlot {...}) as an argument to a function requires memory (more on this below)

Right now I'm leaning towards the following 3 libraries:

• StorageSlot: Leave it as it is right now.
• SlotDerivation: Leave it as it is right now
• TransientSlot:
  • Rename this one to TransientStorage: This reflects that the library is for actually storing.
  • We leave the types from TypedSlot here and rename them as AddressTransientType instead of AddressSlotType.
  • Similarly, we rename the functions from asAddressSlot to toTransientAddress (or asTransientAddress)

If my framing is correct, this sets apart transient storage operations from regular storage ones, and the only common thing between them is the library with the derivation functions. I wouldn't try to make both TransientStorage and StorageSlot
to use the same value types (changed my mind 😅 ), this is because assigning would work very differently depending on the type of storage:

// For regular storage
getAddressSlot().value = '0x...'

// For transient storage
slot.toTransientAddress().tstore('0x...')

Trying to use the same types we already have in StorageSlot would result in (iugh, memory):

struct AddressSlot {
    address value;
}

function tstore(TypedSlot.AddressSlotType slot, AddressSlot memory v) internal {
    address value = v.value;
    /// @solidity memory-safe-assembly
    assembly {
        tstore(slot, value)
    }
}

I see that a common interface between TransientStorage and StorageSlots is possible without using memory if we share the UDVTs, but I'd be fine if they are not compatible out of the box. Note I would add UDVTs to for sstore and sload in another PR and only if it's ever requested.

---

To be clear, this is what I am conceptualizing for the TransientStorage library:

library TransientStorage {
    type AddressTransientType is bytes32;

    function toTransientAddress(bytes32 slot) internal pure returns (AddressTransientType) {
        return AddressTransientType.wrap(slot);
    }

    function tload(AddressTransientType slot) internal view returns (address value) {
        /// @solidity memory-safe-assembly
        assembly {
            value := tload(slot)
        }
    }

    function tstore(AddressTransientType slot, address value) internal {
        /// @solidity memory-safe-assembly
        assembly {
            tstore(slot, value)
        }
    }
}

[Amxx]

Renaming TransientSlot to TransientStorage is good to me.

However, I'm not really onboard with the other proposed changes. I think its a question of vision. IMO the storage and transient storage spaces are like 2 view, with different lifecycle, of the same thing. Like 2 plans on top of one another.

Consequently, I believe that a (derived) storage location is equally valid in storage and in transiant storage. One example is the ReentrancyGuard contract, where I think the slot that is currently used with normal storage should be used exactly the same, but in the transiant storage space. This is why I initially imagined the Typed-slot UDVT would provide sload and sstore in addition to tstore and tload. I'm ok with only implementing the transient side for now, and putting that in a dedicated library (TransientStorage) ... but I would not go as far as using a transient specific UDVT.

Again, I think the UDVT represent a "location + type" that is valid in both spaces.

[ernestognw]

I agree that a location is valid for both storage and transient storage, but I guess the main difference here comes from Solidity. Given the language doesn't treat both types of storage in the same way, we're limited by the current syntax of each one. It would make sense to share an interface if both were the same at the language level, but they're only very similar in assembly.

The way I read this situation is that in order to make both share the same interface, the only way is to use assembly for both. However, the native Solidity syntax is preferred, so that sets transient storage in a different category only because it can't be used with the same language features

I'm ok with only implementing the transient side for now, and putting that in a dedicated library (TransientStorage) ... but I would not go as far as using a transient specific UDVT.

The UDVT is useful here because we don't have a way to tell the compiler what type we're using (e.g. address), so we're using the UDVT as a wrapper to circumvent this limitation. This makes it more specific to transient storage than to "slots". Wouldn't you say as<type>Slot has a name conflict with StorageSlot.<type>Slot?

[Amxx]

The UDVT is useful here because we don't have a way to tell the compiler what type we're using (e.g. address), so we're using the UDVT as a wrapper to circumvent this limitation. This makes it more specific to transient storage than to "slots". Wouldn't you say asSlot has a name conflict with StorageSlot.Slot?

The need for UDVT, as an approach, is specific to transient storage for sure, but no, I don't think the UDVT types themselves are specific to transient storage. Again they just represent a "position+type".

Yes naming can be confusing. We already have a "conflict" between AddressSlotType (UDVT) and AddressSlot (struct). Maybe we can rename as<type>Slot to something like as<type>TypedSlot ... but again I would avoid mentionning "Transient" here, because there is nothing specific to "transient" in the UDVT definition.

[Amxx]

Replaced by #4980