[WIP] Feat - Namespaced storage inspection: forge inspect storage-layout --eip7201

[MerkleBoy]

Motivation

It would be pretty nice to inspect the storage layout of contracts with namespaced storage; especially since this development pattern has become more and more prevalent over the years

Solution

I introduce a few Natspec custom flags that, with the help of AST artifacts, is enough to reliably detect the exact layout of an EIP7201 storage slot and its underlying, or even nested Structs.

• @custom:storage-bucket <BUCKET_ID> : to declare over the constructor of the contract implementing the namespaced storage
• @custom:storage-bucket-transient <BUCKET_ID> : same, but for transient storage slots
• @custom:storage-bucket-schema <BUCKET_ID>: to declare over a getter function that returns the Namespaced storage object
• @custom:storage-bucket-slot (opt.) <?SLOT_ID> : to be put just before a storage slot's declaration.
• @custom:storage-bucket-transient-slot (opt.) <SLOT_ID?>: same, but for transient storage

My hope is that, with enough build artifacts trickery, it would be possible to reduce the amount of natspecs comments to one per SLOT_ID; more in line with the EIP.

PR Checklist

• Added Tests
• Added Documentation
• Breaking changes

[MerkleBoy]

this solves #7662

[DaniPopes]

1. Sorry I don't quite understand your implementation; what are the @custom tags you're searching for, they're not mentioned in the ERC?
2. we are migrating all usage of solc and solang AST to solar. See:

• Tracking: Use Solar for Solidity AST #9317
  This PR would be 1/10th the size if you had used solar.

3. We have code scattered throughout the code base to handle storage layouts, and more recently PRs to fix this issue implement similar logic: Cannot find the storage slot for a public string variable #3869

[MerkleBoy]

I didn't know about Solar, that's on me

As for the custom tags, yeah, they're not part of the original EIP (though they do define one @custom:storage-location Tag in it); the ones I came up with are mostly there to make a working prototype, and talk whether it would be a good idea to add eip7201 support in a format close to this 👍

[onbjerg]

I appreciate the effort put into this, but as @DaniPopes mentioned we have most of the tools in place to do this already, and this re-implements a lot of it. For example, we already have Natspec parsers, and we also have a more robust parsing approach for user defined types like structs/enums/mappings.

This PR is also not up to spec with the EIP, which I think is important: I don't think it is super useful that we roll our own approach, even if we think it would be better, as people are already using EIP7201 as-is. As such, I would only be willing to merge something that is up to spec. The spec is also relatively simple:

• Parse the source file, looking for any user defined types that have @custom:storage-location eip7201:<offset>
• Add these to the storage layout, computing their storage layout as normal but adding the offset.

Let me know if you need pointers on how to implement this :)

[MerkleBoy]

I'll gladly remove my own parsing logic in favour of something more robust like Solar's; I've started reading the documentation, but if there are specific sections you'd recommend starting with, please let me know @onbjerg.

Regarding the Natspec flags, I think we could rearrange things and re-use the one from the EIP verbatim; however, I suspect we may still need at least one more. Let me explain:

In the EIP, the author defined @custom:storage-location <FORMULA_ID>:<NAMESPACE_ID>. For eip7201, the formula is erc7201(id: string) = keccak256(keccak256(id) - 1) & ~0xff. This works well for defining the slot used as the root of whatever goes into storage there.

But when it comes to storage layout, we’re missing context on how that slot is actually used: are we storing a single instance of a structure, or is that slot acting as the base of a mapping or an array? Maybe I’m overthinking it and we don’t need this level of detail in a storage-layout report... but I thought it would be valuable if we could.

I can’t really say the authors of the EIP were shortsighted, given how popular this storage abstraction has become in just a couple of years, but re-reading it, we have:

Preliminaries

A namespace consists of a set of ordered variables, some of which may be dynamic arrays or mappings, with its values laid out following the same rules as the default storage layout but rooted in some location that is not necessarily slot 0. A contract using namespaces to organize storage is said to use namespaced storage. [...]
link to the EIP here

I get that the authors probably didn’t want to open that can of worms, but the lack of detail on “is this namespace meant to be a single object, a mapping, or an array” makes a big difference:

• single objects start using storage slots 0-indexed from that root slot
• mappings use that slot as the base for keccak-based key-pair addressing, with the base itself reserved
• dynamic arrays use that slot to store the array’s length, and the array itself begins at keccak(base)

We can’t use the same namespace for more than one of these three cases without overwriting critical data. So to compute a valid storage layout, that context either needs to be given explicitly or inferred from compiler artifacts (which is what I was attempting to do here, a bit haphazardly).

Hopefully Solar’s parsing capabilities are strong enough that this becomes trivial. But if it's not, what do you think we should do ?

On a side-note, Certora is using a somewhat-similar method, and have created a few storage layout annotations of their own to bridge that gap.

[onbjerg]

But when it comes to storage layout, we’re missing context on how that slot is actually used: are we storing a single instance of a structure, or is that slot acting as the base of a mapping or an array? Maybe I’m overthinking it and we don’t need this level of detail in a storage-layout report... but I thought it would be valuable if we could.

What this EIP does is allow you to specify what p is when you compute storage slots. For example, the storage slot of a key k in a mapping is s = keccak256(h(k) . p) where h is a padding function that depends on the key type, and . is concatenation. Normally, p is determined by the order the storage variable is defined in, but obviously that does not work when you manually set the storage slot.

The way this trick plays out in practice is usually that you define a struct. That is to say, I don't recall anyone using a mapping directly, they'd define a wrapper struct that holds a mapping. There is (almost?) no cost to doing so.

[MerkleBoy]

The way this trick plays out in practice is usually that you define a struct. That is to say, I don't recall anyone using a mapping directly, they'd define a wrapper struct that holds a mapping. There is (almost?) no cost to doing so.

That makes a lot of sense; not only that, that means I have been using that EIP the wrong way for a while. Thank you very much

I'll keep you guys posted when the Solar version for this PR is ready