Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eternal storage #381

Closed
wants to merge 2 commits into from
Closed

Conversation

SylTi
Copy link
Contributor

@SylTi SylTi commented Aug 18, 2017

Let's discuss this ;)

@frangio
Copy link
Contributor

frangio commented Aug 28, 2017

This is an interesting approach. In my opinion, though, it tries to be too general purpose, and kind of reimplements the EVM storage mechanism, which is already a mapping of keys to values. Upgrading a contract to one that shares the same storage variables (in the same order), and maybe adds new ones, should serve the same purpose for upgradeability.

@SylTi
Copy link
Contributor Author

SylTi commented Aug 28, 2017

@frangio are you talking about something like
Contract A { struct MyType { uint nb; } mapping<address, MyType> map; function addToMap(uint); } contract B { address storage; function addSomething(uint val) { A(storage).addToMap(val) } ?

If that's the case, how do you upgrade MyType in that scenario?

Or are you referring to something like this https://blog.zeppelin.solutions/proxy-libraries-in-solidity-79fbe4b970fd ?

@frangio
Copy link
Contributor

frangio commented Aug 29, 2017

No, I'm thinking of something lower level.

contract MyContractV1 {
    uint256 x;
    function foo() returns (uint256) {
        return x;
    }
}

which if it is accessed behind a delegating proxy can be safely upgraded to something like

contract MyContractV2 is MyContractV1 {
    uint256 y;
    function bar() returns (uint256) {
        return x + y;
    }
}

Using inheritance is just a shorthand way of getting the variables to line up.

Sorry for the lack of detail, I can clarify further if needed.

@SylTi
Copy link
Contributor Author

SylTi commented Sep 2, 2017

I see what you mean.
But wouldn't that be unmaintainable (& cost a lot more gas) in the long run?

contract MyContractV42 is MyContractV41 {
    uint256 y;
    function bar() returns (uint256) {
        return x + y;
    }
}

@frangio
Copy link
Contributor

frangio commented Sep 2, 2017

Yes, using inheritance would not be very maintainable. It should definitely not be the long term strategy. I think with the right tooling this approach to upgradeability can be very maintainable and safe to use.

What gas costs are you referring to? I'm pretty sure the gas costs remain constant from upgrade to upgrade. The inheritance chain would be resolved and optimized by the compiler.

@SylTi
Copy link
Contributor Author

SylTi commented Sep 3, 2017

I was thinking more about the gas cost when setting information.
If you need to call setA, setB, setC as external call to other contracts it will be more expensive than calling a single set on a single external contract

@theethernaut theethernaut added review feature New contracts, functions, or helpers. and removed review labels Jan 2, 2018
@theethernaut
Copy link
Contributor

@frangio @SylTi did you arrive to any conclussions on this?
If you did, consider closing this issue and opening a new one with the specification of the implementation.

@theethernaut theethernaut changed the title [Feature]eternal storage Eternal storage Jan 5, 2018
@SylTi
Copy link
Contributor Author

SylTi commented Jan 11, 2018

I don't think there is any kind of conclusion to be reached? I think we both have valid points. In the end it's just a matter if you want to merge this in OZ or not :)

@AugustoL
Copy link
Contributor

AugustoL commented Feb 6, 2018

@SylTi what do you think about using an EternalStorage contract like:

contract EternalStorage {

  mapping(bytes32 => uint256) internal uintStorage;
  mapping(bytes32 => string) internal stringStorage;
  mapping(bytes32 => address) internal addressStorage;
  mapping(bytes32 => bytes) internal bytesStorage;
  mapping(bytes32 => bool) internal boolStorage;
  mapping(bytes32 => int256) internal intStorage;

}

If we have them all as internal variables is enough, then we can leave as responsibility of the dev to provide the get functions.

@SylTi
Copy link
Contributor Author

SylTi commented Mar 23, 2018

I'm not sure of why you would want an eternal storage without gets? How do you use the data stored?

@AugustoL
Copy link
Contributor

@SylTi what about using this implementation ethereum/EIPs#930 ?
There is an example of BasicToken using ES.

@frangio
Copy link
Contributor

frangio commented Apr 17, 2018

Closing for now because we don't see this being a part of OpenZeppelin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New contracts, functions, or helpers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants