search

ethereum / solidity

Solidity, the Smart Contract Programming Language
https://soliditylang.org
GNU General Public License v3.0
23.14k stars 5.74k forks source link

Allow specifying storage locations #597

Open pipermerriam opened 8 years ago

pipermerriam commented 8 years ago

Inline assembly has now made fully up upgradable contracts possible. One of the main hangups with this is that the storage locations have to stay the same across upgrades. Would it be possible to introduce support for specifying the storage locations for storage variables?

VoR0220 commented 8 years ago

not so!

VoR0220 commented 8 years ago

See Nick Johnson's Library on upgradeability :)

https://gist.github.com/Arachnid/4ca9da48d51e23e5cfe0f0e14dd6318f#file-upgradeable-sol

chriseth commented 8 years ago

Especially with contract upgrades in mind, wouldn't it be better to copy the storage layout and "disable" unused state variables by e.g. prefixing them? Otherwise I don't see how you would practically verify that the storage layout is consistent between upgrades.

pipermerriam commented 8 years ago

Is there documentation on how storage layout is determined?

On Wed, May 25, 2016, 1:54 AM chriseth notifications@github.com wrote:

Especially with contract upgrades in mind, wouldn't it be better to copy the storage layout and "disable" unused state variables by e.g. prefixing them? Otherwise I don't see how you would practically verify that the storage layout is consistent between upgrades.

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/ethereum/solidity/issues/597#issuecomment-221499635

chriseth commented 8 years ago

http://solidity.readthedocs.io/en/latest/miscellaneous.html#layout-of-state-variables-in-storage

pipermerriam commented 8 years ago

Ok, so after reading up on storage layouts...

contract MyContractV1 {
    uint a;
    bytse32 b;
}

In this example, a should be stored in slot 0 and b in slot 1.

Now, consider I upgrade it to the following.

contract MyContractV2 {
    int c;
    uint a;
    bytes32 b;
}

This would end up with c stored in slot 0, a in 1, and b in 2 which would break things.

So, instead, I propose being able to do the following.

contract MyContractV2 {
    int c;
    uint a @ 0x0;
    bytes32 b @ 0x1;
}

The solidity compiler would see that a and b are designated for storage slots 0 and 1 respectively, and would then place c at the next available location, slot 2.

Does that make sense? Is this possible?

axic commented 8 years ago

I was looking for a complementary/similar feature: the ability to disable packing. (i.e. currently if two storage parameters are each < 256 bits and together they fit into one slot, they are packed together.)

Ultimately the compiler could optimise the packing based on the frequency of changes to one ore more variables within.

With your suggestion this is a given, each marked variable gets its own slot. I would use a different markup though:

storage(0) int a;
storage(1) bytes32 b;
pipermerriam commented 8 years ago

I would use a different markup though

the @ was just the first thing that came to mind. I like storage(...) better.

chriseth commented 8 years ago

I think the tradeoff between introducing errors and decreasing readability is much better when just adding int c at the end. If you want, you can also use inheritance (let the upgraded contract inherit from the old contract).

VoR0220 commented 8 years ago

^ 👍 for the inheritance structure...it overall is cheaper and more cost effective to do it that way. I envision a lot of modularity around dapps in the future in regards to storage to better handle updates and save gas.

axic commented 6 years ago

This came up again as a discussion with @federicobond and I think a good middle ground could be to have an annotation (as proposed in https://github.com/ethereum/solidity/issues/597#issuecomment-221611370), but instead of marking a storage slot, it would rather have a string literal as a key, which is hashed to produce a 256-bit key for storage.

This would be more expensive (due to the fact of using 32-byte long constants and one couldn't combine multiple variables into a single slot), but might be justified for some.

When this annotation is missing, it would default to the current behaviour.

For syntax I propose:

int256 a storage_key("this_is_my_variable");
bytes32 b storage_key("and_this_too");
pipermerriam commented 6 years ago

@axic

  1. I like the hashed key approach
  2. reasoning for not allowing specific slot to be specified? foot gun?
chriseth commented 6 years ago

I really don't think solidity should have such low-level impact on the storage location. If you want to dislocate storage variables, why not use structs or a mapping to structs?

gnidan commented 6 years ago

One more possible other solution:

contract MyContract {
  storage("some-collection") {
    uint foo;
    uint bar;
  }

  storage("other-collection") {
    mapping (uint => bool) qux;
    MyStruct baz;
  }
}

The advantage of this is that contracts could define blocks of variables that are colocated in storage, but providing gaps, to extend structs later, etc.

spalladino commented 6 years ago

Just throwing this as an idea: given that this need arises from avoiding clashes when working with upgradeability, wouldn't it make sense to just avoid clashing by storing all variables in a hashed location, similar to how a mapping works? We could either store all variables from the same contract/struct together (the hash being a contract identifier, and variables are stored at offsets of that hash), or all individual variables in sparse hashed locations.

The issue remains on how to generate an identifier for a contact, to ensure there are no clashes between different contracts, but that identifier is more robust than a simple name. Maybe requiring a special constant with a random value for every contract that will use this approach, similar to old Java's serialVersionUID?

axic commented 4 years ago

There was also a lengthy related discussion in #4017.

ekpyron commented 4 years ago

This came up again with #7891 .

If we want to expose really general control we need three components:

Natural restrictions would apply (violating would result in compile time errors):

I would suggest to make all such specifiers optional. Variables without specifiers before any variables with specifiers will be assigned slots as before. For variables without specifiers after any variables with specifiers there are two options:

For the purpose of inheritance: locations are assigned just as if it was one flat contract containing all variables in the order of C3 linearization.

Example (we can always decide on a different syntax):

contract A {
  uint256 a; // will occuply full slot 0
  // slots 1 and 2 will remain unused
  storage{slot: 3, offset: 0, reserved: 32} bool b; // will occupy full slot 3

  storage{slot: 4, offset: 1} bool c; // will occupy the second byte in slot 4
  storage{slot: 4, offset: 0} bool d; // will occupy the first byte in slot 4
  storage{slot: 4, offset: 16} uint128 d; // will occupy the second half of slot 4

  uint128 e; // will occupy the first half of slot 5

  storage{slot: 5, offset: 16} uint128 f; // will occupy the second half of slot 5

  storage{slot: 6, offset: 0} bool g; // will occupy first byte in slot 6
  bool h; // will occupy second byte in slot 6
  storage{slot: 6, offset: 2, reserved: 2} bool i; // will occupy third byte in slot 6
  bool j; // will occupy fifth byte in slot 6
  storage{slot: 6, offset: 16, reserved: 48} uint128 k; // will occupy second half of slot 6
  // slot 7 will remain unused
  uint128 l; // will use the first half of slot 8
}

An alternative notation-wise would be to merge slot and offset into a single byte offset that is then split into slot = byteOffset/32 and offset = byteOffset%32 (to which the same restrictions would apply). A copy of the example above using this notation:

contract A {
  uint256 a; // will occuply full slot 0
  // slots 1 and 2 will remain unused
  storage{offset: 96, reserved: 32} bool b; // will occupy full slot 3

  storage{offset: 129} bool c; // will occupy the second byte in slot 4
  storage{offset: 128} bool d; // will occupy the first byte in slot 4
  storage{offset: 144} uint128 d; // will occupy the second half of slot 4

  uint128 e; // will occupy the first half of slot 5

  storage{offset: 160} uint128 f; // will occupy the second half of slot 5

  storage{offset: 192} bool g; // will occupy first byte in slot 6
  bool h; // will occupy second byte in slot 6
  storage{offset: 194, reserved: 2} bool i; // will occupy third byte in slot 6
  bool j; // will occupy fifth byte in slot 6
  storage{offset: 208, reserved: 48} uint128 k; // will occupy second half of slot 6
  // slot 7 will remain unused
  uint128 l; // will use the first half of slot 8
}
ekpyron commented 4 years ago

Another alternative would be to require specifying the location for all variables, if the location is specified for any variable.

Also we could at a later point allow compile time evaluated expressions in the specifier, i.e.:

storage{slot: keccak256("some_key")} uint256 some_key;

Although we'd need to consider that one could construct those to specifically collide with some mapping key, so this would be dangerous.

Although that's also true for choosing some specific value for slot: that happens to be the location of some mapping element.

chriseth commented 4 years ago

Maybe we should gather some data about how this feature would be used. One use is avoiding clashes during upgrades, another is having more efficient use of storage by combining small variables in a certain way. I think just providing full flexibility all the time might not be the way to go as it is too easy to get wrong. So it could already be enough to only allow hashed locations and another way to specify which variables to combine (without specifying the offset exactly) or when to insert "start a new slot here".

ekpyron commented 4 years ago

What can "go wrong"? Or in particular, what can go wrong that we can't easily detect at compile time? I'd argue that it makes more sense to provide a general solution and, if deemed necessary, restrict it to simple cases (as in restrict to some particular kinds of values for slot, etc. - e.g. restricting to only supporting "start a new slot here" would be to require slot to be the "current slot" plus one and require offset to be zero).

That way we can always extend the very same solution to support more cases, instead of needing breaking changes and new language features...

spalladino commented 4 years ago

One use is avoiding clashes during upgrades

For the sake of upgrades, it'd seem that the only requirement is to be able to assign an immutable id to a variable, which should be deterministically mapped to a slot (like the storage{slot: keccak256("some_key")} proposed above). It's not really important where in the storage the variable is kept.

As for EIP2330 linked above, the requirements are pretty much the same. As long as there is a deterministic process for calculating the storage slot, the actual slot can then be just exposed in the ABI for any consumers.

KaiRo-at commented 4 years ago

To implement contract following EIP 1822 and/or EIP 1967, the capability to define a specific slot would be required. Right now, this needs to be done via inline assembly...

junderw commented 4 years ago

After reading this issue, and thinking about the issue, this is my proposed solution.

  1. add the keyword deterministic
  2. it can only be used with declared storage variables
  3. at compile time the slot used for uint256 deterministic myNumber; is keccak256('myNumber')
  4. basically, similar to immutable which doesn't use storage, deterministic variables will be removed when calculating the sequential slots of each variable, the keccak256 is calculated at compile time and used for all instances.
  5. upgradable contracts can now rely on "this variable name will always be a specific storage slot"
  6. If it sees the same variable name in multiple inherited contracts with mixed deterministic states it should throw an error at compile time saying "can't use deterministically declared variable non-deterministically"

It seems simple enough to remove complexity but accomplish some of the major goals of this thread.

However, it seems like this thread has grown with a list of reasons and use cases which can only be satisfied by increasingly complex low level access which is difficult to implement without adding a foot cannon.

Edit: Thinking about it more, it might be sufficient to commit only to the variable name, as someone using this feature would probably make their variable names more descriptive address deterministic openZepplinProxyImplementation; which should probably throw an error if an inherited contract tries to use uint256 deterministic openZepplinProxyImplementation; which could be another foot cannon.

Edit2: I have a low-deploy-gas-cost proxy contract that I optimized the bytecode for, and it would be great to use single byte storage slots (ie. 0xff) without needing to create 255 dummies... I just tried upping my storage slot for the proxy to PUSH32 with a random hash instead of the 0 I'm using right now, it bumped my deploy cost from 80k to 100k (since I have 1 PUSH in the deploy code and 2 PUSHes of it in the contract code.)... so I would definitely also enjoy the ability to specify an arbitrary value for the slot as well... that said, my use case is extremely niche so I understand not accommodating it.

axic commented 4 years ago

Copying the suggestion from @dominicletz from #7593:

A new keyword fixed(@N) is proposed that can be used to define fixed slot position in interfaces.

interface ContractAddressMap {
   public fixed(@5) mapping(bytes32=>address) addr;
}
SchoofsKelvin commented 3 years ago

Here's my spin on specifying storage slots, based on existing EIPs that could benefit from this:

This is partly with my (although limited) experience working with Solidity and proxy contracts, the feedback in this issue and the mentioned EIPs. It's both an attempt at covering as much of the use cases and requirements, while also seeing if there's any (planned) progress on this issue. I've noticed now that I'm rereading it that this is quite a big comment, hopefully that isn't too much of an issue.

Example 1 (simple proxy)

Example showing how it behaves on its own and how it interacts with non-specified slots:

contract SomeProxy {
    uint256 private someVar1; // storage slot 0x0 (in this case starting at 0x0 because there are no inherited fields)
    uint256 private someVar2 = 5; // storage slot 0x1

    uint256 public something1 at(0xAA112233) = 99; // storage slot 0xAA112233

    // This would produce an error/warning because it uses the same storage slot as `something1`
    uint256 public something2 at(0xAA112233) = 123; // storage slot 0xAA112233

    uint256 public someVar3; // storage slot 0x3 (right behind `someVar2`, thus ignoring `something1` and `something2`)

    bytes32 constant IMPLEMENTATION_SLOT = bytes32(uint256(keccak256('eip1967.proxy.implementation')) - 1);
    // ^ 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc 

    // Not specifying the offset, so can use the syntactic sugar version without the brackets
    address private _implementation at IMPLEMENTATION_SLOT; // storage slot 0x360894...

    // ....
}

_the _implementation part is based on the storage slots specified in EIP-1967_

Example 2 (diamond storage)

This could also work in interfaces, which helps a lot with diamond storage (EIP-2535), related to #9551. In this example:

// OldFacet.sol
bytes32 constant OLD_FACET_STORAGE_SLOT = keccak256("diamond.storage.OldFacet");

struct OldFacetStorage {
    // ...
}

contract OldFacet {
    // Directly registers the storage field/slot here, e.g. an externally developed or pre-Storage.sol contract
    OldFacetStorage storage at OLD_FACET_STORAGE_SLOT;
    // ....
}

// Storage.sol
import "./OldFacet.sol";
import "./NewFacet.sol";

interface DiamondStorages {
    OldFacetStorage oldFacetStorage at OLD_FACET_STORAGE_SLOT;
    NewFacetStorage newFacetStorage at NEW_FACET_STORAGE_SLOT;
    // ....
}

// NewFacet.sol
import "./DiamondStorages";

bytes32 constant NEW_FACET_STORAGE_SLOT = keccak256("diamond.storage.NewFacet");

struct NewFacetStorage {
    // ...
}

// Makes use of Storage.sol instead of specifying the storage field/slot for every (known/used) faucet's storage
contract NewFacet is DiamondStorages {
    // Optional "aliasing storage fields" which can be handy for shortening long names to shorter ones
    NewFacetStorage private s referencing(newFacetStorage);

    function banana() public returns (uint256) {
        // Using OldFacet's storage as declared in `DiamondStorages` in `Storage.sol`
        // Using NewFacet's storage as declared by `s`, using the same slot as `newFacetStorage` in `DiamondStorages`
        return oldFacetStorage.apple + s.durian;
    }
}

Since all the used/known diamond storages are all defined and known at compile-time, the compiler could (as part of later feature requests) check whether any storage slots overlap each other.

Example for #7593

Example of how this would benefit #7593 should non-integer values for N be allowed:

interface ContractAddressMap {
    public mapping(bytes32=>address) addr at keccak256('ENS.addr');
    // or different versions of their syntax that allows non-integer `N` values:
    public fixed(@keccak256('ENS.addr')) mapping(bytes32=>address) addr; // keep `@` to differentiate from e.g. `map_v1`
    public fixed(keccak256('ENS.addr')) mapping(bytes32=>address) addr; // get rid of `@`, force 1st argument to be location
    public fixed('ENS.addr') mapping(bytes32=>address) addr; // assuming using a direct string implies `keccak256`
}

contract Resolver is ContractAddressMap {
    // ...
}

Mind that this also more or less solves their issue regarding conflicts, as compared to using fixed(@5), it's a lot harder to have clashes when using e.g. keccak256('ENS.addr'). Suggesting these kind of modifications to the EIP-2330 draft seems doable and beneficial for everyone.

Expected improvement from #3157

Should #3157 be implemented, this can help a lot with readability with complex keys later on, e.g. for our first example:


function proxy_slot(string key) public pure returns(bytes32) {
    return bytes32(uint256(keccak256(abi.encodePacked('eip1967.proxy.'), key)) - 1);
}

contract SomeProxy {
    // Points to the slot at `bytes32(uint256(keccak256('eip1967.proxy.implementation')) - 1)`
    // aka `0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc`
    address private _implementation at proxy_slot("implementation");
}

Deterministic storage slots

This feature request is purely to use statically assigned storage slots, as specified by those EIPs or used in the diamond (storage) pattern. The whole "deterministic storage slot" idea is also interesting, but more of a separate feature from (or on top of) specifying specific storage locations. It's worth mentioning that the latter could still be relatively easily done should (the static part of) this feature be implemented:

contract DeterministicFields {
    address private _owner at keccak256('_owner');

    // or again, if we have #3157 and support pure functions in the contract itself during compile-time:
    function storage_slot(string key) internal pure returns(bytes32) {
        return bytes32(uint256(keccak256(abi.encodePacked('DeterministicFields.'), key)) - 1);
    }
    address private _admin at storage_slot('_admin');
    // ^ Would use storage slot `keccak256('DeterministicFields._admin')`, making it less likely to clash
    // with storage slots from other contracts using an `_admin` field (e.g. in base contracts or other facets in a diamond)
}
cameel commented 3 years ago

In https://github.com/ethereum/solidity/issues/8353#issuecomment-932474378 I have described another solution, similar to what @gnidan and @spalladino suggested above (see https://github.com/ethereum/solidity/issues/597#issuecomment-406582568, https://github.com/ethereum/solidity/issues/597#issuecomment-418798622).

In short, you can already use a mapping of structs to give your contract a set of variables at a storage location deterministically computed from some key:

contract MyContract {
    struct State {
        uint variable1;
        address variable2;
    }

    mapping (string => State) states;

    function f() public returns (uint) {
        return states["MyContract"].variable1;
    }
}

We could just add a bit of syntax sugar on top of that and have a simple solution that might be good enough in most cases. I.e. this would generate bytecode identical to the above:

contract mapping MyContract {
    uint variable1;
    address variable2;

    function f() public returns (uint) {
        return variable1;
    }
}

Since it would be a new contract variety, it would be fully backwards-compatible.

github-actions[bot] commented 1 year ago

This issue has been marked as stale due to inactivity for the last 90 days. It will be automatically closed in 7 days.

ekpyron commented 1 year ago

We have multiple issues along this (see e.g. the list in https://github.com/ethereum/solidity/issues/13466#issuecomment-1419319182) - I'm marking this one here unstale and as must have eventually, since it's the most generic and most likely version to be implemented eventually.

yoavw commented 1 year ago

This issue has become more critical with the growing popularity of account abstraction, so I would like to resume this discussion.

Account abstraction adds a new challenge that, to the best of my knowledge, no other contract has faced before.

The user owns the account, typically a proxy, and may occasionally wish to switch to a different implementation as the needs change. E.g. switch between Safe and Argent. Unlike other proxy situations where new implementations use a compatible storage layout, or at least the new implementation is aware of the old one and can perform a conversion, now we're dealing with implementations that are not aware of each other.

Currently, most account implementations start at storage slot zero. When switching implementations, the user may inadvertently end up with things like shadow signers, e.g. this Safe which seems to have one valid signer, but in fact has 20 invisible signers who can actually transact in the Safe.

There is no safe way for users to migrate between account implementations, unless each account uses a different storage base. I would like to propose a pragma for doing so:

pragma storage_base <value>;

When present, the compiler calculates keccak(value) and uses it as a hardcoded offset for storage access. A typical value would be ..

The pragma only sets the offset of next items, and following items are incremented by one, as usual. Therefore if the pragma appears multiple times, each pragma forms a "logical group" of member fields.

A buggy/malicious account would still be able to bypass it and access other storage offsets, but it'll require asm code and therefore attract auditors' attention. Pure solidity code will only access storage determined by the storage_base value.

hrkrshnn commented 1 year ago

@yoavw Typically what people do to get this is to add a state variable uint[1000] junk.

// Inherit this for leaving out the first 100 slots
abstract contract storageBase {
    uint[100] junk;
}

Would that be a reasonable way to achieve the same?

yoavw commented 1 year ago

Would that be a reasonable way to achieve the same?

I'm not sure, since the storage base has to be at a random location in the storage space, typically keccak(project_name) to avoid conflicts between different account implementations. solc would complain if contracts allocate an array half the size of the storage space.

Another issue is that a contract may need to access the storage base of another contract. If they allocate these slots for an array, they won't be able to. At least two cases require this:

  1. Modules. Some accounts are modular and a module sometimes need to access the storage of the account's core implementation. The modules use different storage bases to avoid conflicts but if they use an array they won't be able to access lower slots. Suppose the core account implementation starts at S1 and a module starts at S2, with S1 < S2. The module will have the S1 space inside its junk array and won't be able to access these variables.

With the pragma I suggested, it would be accessed like this:

contract SafeStorage {
  #pragma storagebase "safe.core"
  uint threshold;
  mapping (address=>address) owners;

  #pragma storagebase "safe.modules.niftymodule"
  mapping (address=>address) someModuleMapping;
  uint someModuleVariable;
}
  1. Migration function. Suppose the user is switching from SimpleAccount to Safe, which use different storage bases. The transaction that changes the implementation will have to import signer from the SimpleAccount storage base and add it to the Safe storage base as a signer. If SimpleAccount's storage base is lower than Safe's, then Safe won't be able to perform the migration.

With the pragma I suggested, a Safe migration module could be something like this:

contract MigrateSimpleAccountToSafe is OwnerManager {
  #pragma storagebase "safe.core"
  uint threshold;
  mapping (address=>address) owners;

  #pragma storagebase "simpleaccount"
  address public simpleAccountOwner;

  function migrate() public {
    addOwnerWithThreshold(simpleAccountOwner, 1);
    simpleAccountOwner = address(0);
  }
}
ekpyron commented 1 year ago

Pragmas are a mechanism for modifying file-based compiler settings - I wouldn't consider them a good match for this purpose. My preferred approach would still be to have specific syntax for specifying the storage location of any specific storage slot, which would result in the following slots to continue using storage after the previous slot as usual, until the next slot has specified a specific storage location.

So in your example something like

contract MigrateSimpleAccountToSafe is OwnerManager {
  storage{slot: keccak256("safe.core")} uint threshold;
  mapping (address=>address) owners;

  storage{slot: keccak256("simpleaccount")} address public simpleAccountOwner;

  function migrate() public {
    addOwnerWithThreshold(simpleAccountOwner, 1);
    simpleAccountOwner = address(0);
  }
}

(potentially with different syntax, but with regular language keywords decorating the slots instead of pragmas)

yoavw commented 1 year ago

Pragmas are a mechanism for modifying file-based compiler settings

It's mostly a file-based property and should affect everything in that file. Except for rare cases like a migration contract or a module accessing the account's core storage, the pragma is set at the beginning of the file and affects all variables defined in that file.

We could also have a syntax for variable-level setting, but I think we need a way to do it for the entire file. Otherwise we risk missing some variable and it gets based at slot 0 rather than the account's storage base. None of the variables should be based at zero if the implementation might change.

In the above example, threshold is based at keccak256("safe.core"), but wouldn't owners be based at zero rather than keccak256("safe.core")+1? If it's based at zero, it would conflict with any account implementation that also happens to have something at zero due to making the same mistake?

The intent we'd like to signal to the compiler is "from here on, variables start at slot S and grow from there". If pragma is not the appropriate way to signal something like this, what would be the right way?

ekpyron commented 1 year ago

It's not a file-based property at all - it's a property at most of a contract, but really of its state variables - pragmas are definitely the wrong tool for this. I mean, even in your example, it's a property that changes more than once within one contract in a file :-).

In the above example, threshold is based at keccak256("safe.core"), but wouldn't owners be based at zero rather than keccak256("safe.core")+1? If it's based at zero, it would conflict with any account implementation that also happens to have something at zero due to making the same mistake?

In my last comment I said "specifying the storage location of any specific storage slot, which would result in the following slots to continue using storage after the previous slot as usual" - storage slots are generally not assigned globally but in relation to the last slot (since there may be packing involved). So whenever we'd specify the specific slot for any state variables, the next variable would continue continuously from there, and in particular in the example owners would live at keccak256("safe.core")+1. That's the only sane choice, I'd argue, since interleaving different continuous storage regions would be horribly confusing.

The intent we'd like to signal to the compiler is "from here on, variables start at slot S and grow from there". If pragma is not the appropriate way to signal something like this, what would be the right way?

I'd argue that the right way to do this is specifying the slot directly for the first variable that's supposed to live at slot S - and then let the compiler append the next slot after the previous one like it always does.

yoavw commented 1 year ago

ok, no pragma then :)

Let's go with the syntax you're suggesting.

One case I'm unsure of, is inheritance. A pragma would have been placed before the contract definition, affecting everything that comes after it. With the above syntax, how would the following be handled?

abstract contract Ownable {  // OpenZeppelin Ownable.sol - imported library, not modified locally.
  address private _owner;
  ...
}

contract SomeAccount is Ownable {
  storage{slot: keccak256("someaccount")} mapping(address => address) internal modules;
  ...
}

My understanding is that Ownable._owner gets allocated before SomeAccount.modules. Wouldn't _owner end up in slot 0 then?

wranai commented 1 year ago

Over 7 (SEVEN!) years into this thread about something obviously useful and desirable, what legitimate arguments go against just rolling with something simple, stupid, and safe:

contract Contract {
    mapping (address=>uint256) stuff at keccak256('stuff');
    // ...
}

or

contract Contract {
    mapping (address=>uint256) stuff @ keccak256('stuff');
    // ...
}

Not having this feature was a design flaw right from the beginning, probably due to the mistaken belief held at the time that "code is law, code is immutable," which allowed for abstracting storage address away since nobody would look at it again.

yoavw commented 1 year ago

Over 7 (SEVEN!) years into this thread about something obviously useful and desirable, what legitimate arguments go against just rolling with something simple, stupid, and safe

Yes please! Something simple and stupid is better than nothing at all. We need this.

As for this specific syntax, it'll be a bit error-prone since the developer needs to manually handle the storage counter or use different strings for each variable:

contract Contract {
    mapping(address=>uint256) mapping1 at keccak256('stuff');
    uint256 array1[10] at keccak256('stuff')+1;
    uint256 num at keccak('stuff')+11;
    // ...
}

or 

contract Contract {
    mapping(address=>uint256) mapping1 at keccak256('Contract.mapping');
    uint256 array1[10] at keccak256('Contract.array1');
    uint256 num at keccak('Contract.num');
    // ...
}

I think being able to set the storage base, and then all subsequent declarations continue from the new base, is more readable and less error prone. But at this point anything would be better than waiting another 7 years.

zemse commented 9 months ago

I saw the talk by openzeppelin at solidity summit. They have mentioned that their v5 uses storage locations as here.

Problem: this does not include the info in the storage interface in the solc output.

It would be great if solidity supports specifying the storage locations so that the storage interface will contain all these values so that tools can utilize this info. Is there any active issue regarding this?

yoavw commented 9 months ago

It would be great if solidity supports specifying the storage locations so that the storage interface will contain all these values so that tools can utilize this info.

Right. It should be a compiler level feature, not a hack by the contract itself.

Is there any active issue regarding this?

This issue is still open :) but it's been here since 2016. Can we increase its priority somehow?

frangio commented 9 months ago

@zemse asks if there is an open issue discussing the problem that struct types as used in ERC-7201 will be ignored by solc when producing the storageLayout compiler output. This is because the struct type is not used in any state variable that solc is aware of, the type is only used as the type of a storage pointer returned by a function.

I've discussed this with the Solidity team in the past on Gitter, but did not open an issue. I do think it should be fixed. We had to implement a pretty complex workaround because of it, for the storage safety checks implemented in OpenZeppelin Upgrades.

lucemans commented 4 months ago

EIP-7702 was here.

yoavw commented 4 months ago

Happy 8th birthday to this issue! 🎉🥳

And more seriously, with EIP-7702 this becomes critical. EOAs will be delegating to different contracts, all of which may write to storage slots in the EOA itself. Conflicts are pretty much guaranteed if we don't make it easy to use different storage bases.

cameel commented 4 months ago

We discussed this with @yoavw on the design call today. The biggest hangup here is really that none of the solutions proposed so far seem to both be simple and address all the relevant use cases. One that would be the most appealing is letting the user specify an offset for a particular variable and have all the variables defined after that just follow it. The problem is that contract inheritance severely limits its usefulness since the variables of the base contracts are laid out first.

Still, we're aware that EIP-7702 is making the issue more pressing and we need to address it quickly. We decided that we're going to introduce a simple solution that will at least address this particular use case without conflicting with possible extensions in the future.

We'll make it possible to specify a base storage offset for a contract as a whole (including its inheritance hierarchy). For now it will only be possible to specify it for the most derived contract, but we may consider relaxing that restriction if there are good reasons for it. The offset may be specified indirectly (e.g. be a hash of some identifier, which is the common way to do it), but must still be computable at compilation time.

Optionally, we may consider including a mechanism for moving the variables of a specific contract out of that flow, to give them an independent location, which might mesh better with upgradeable contracts. But then there's the question if the location should be completely fixed or depend on the base offset?

That's the general outline, but we still need a concrete syntax proposal.

We're also interested in feedback on that solution. Is there anything it's missing in the context of EIP-7702?

frangio commented 4 months ago

Decoupling storage location from inheritance linearization is important. What is the main challenge to going in that direction rather than just adding a global offset?

yoavw commented 4 months ago

https://eips.ethereum.org/EIPS/eip-7201

GregTheGreek commented 4 months ago

In the case of 7702, wouldn't it make more sense to set it globally at the contract level? For example instead of declaring the pragma on a per variable scope, we can set it globally

storagePragma

contract Contract {}

Where the contract code is keccak hashed, and thus every variable downstream is namespaced based on the contract that is currently deployed?

yoavw commented 4 months ago

In the case of 7702, wouldn't it make more sense to set it globally at the contract level?

Yes. I believe that's what the solidity team has in mind. Set it globally in the most-inherited contract.

PowerStream3604 commented 4 months ago

@cameel @yoavw really glad to see this is making progress. I think this will be a big help for the EIP 7702 and for AA wallet implementations in general.

Curious to know if we have a draft or idea on how the syntax will work for setting the base storage slot from the latest comment?

Regarding the below comment:

We'll make it possible to specify a base storage offset for a contract as a whole (including its inheritance hierarchy)

Does it imply that we'll use the final contract's base storage as a whole? keccak256("ModularAccount") in this case or would the storage be allocated in each contract's declared base storage slot?

basestorage{slot: keccak256("Storage")} // Just assuming a possible syntax
contract Storage {
    address entryPoint;
    // ...
}

basestorage{slot: keccak256("ModularAccount")} // Just assuming a possible syntax
contract ModularAccount is Storage {
    mapping (bytes4=>address) selectorTomodules;
    // ...
}
nlordell commented 4 months ago

We discussed this with @yoavw on the design call today. The biggest hangup here is really that none of the solutions proposed so far seem to both be simple and address all the relevant use cases.

Is there a summary of what those proposals are? This GitHub issue seems to mostly describe proposals around specifying slots for specific fields, but not necessarily what the currently considered proposals for setting a storage namespace for entire contracts in the context of 7702.

Some random thoughts (although I don't have a concrete syntax proposal).

  1. You could argue that base classes should be "storage namespace" agnostic, however derived contracts may want to have storage of a base class also part of its namespace. One possible syntax could be: 
    contract Foo is Bar{storage: 1337} {
}
  2. If you want to be very opinionated, then you could just (AFAIU, this is what @yoavw suggested in the past): 
    pragma storage namespace "MyNamespace";

    And just assign slots starting at keccak256("foo") - 1 (instead of 0) and "grow downwards" in order to prevent issues around storage slots being at locations with known preimages (if that is a concern). This does, however, get a bit tricky with inheritance (what happens if two files use pragmas to different namespaces?) and I don't know if there are some ugly edge cases that I didn't think about.

  3. Same as 2, but make it a compiler flag. This removes some ambiguity with inheritance and makes the storage namespace only apply to single compilation unit without introducing any additional language requirements at the downside of making it less obvious from code.
nlordell commented 4 months ago

As a follow up suggestion, another possible solution is to have something like:

storage namespace Foo is Bar {
   slot: 1337
}

Which would output bytecode for a "contract" named Foo whose implementation is Bar with different field storage slot assignment as described above (where the field that is assigned storage slot 0 is instead assigned storage slot 1337, etc.). This eliminates ambiguity around what happens when base and derived contracts start specifying this, since the "storage namepace" is not a contract that can be inherited from.

All syntax suggestions are just placeholders