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

Added gaps at the end of the storage of each contract. #29

Merged
merged 2 commits into from
Oct 10, 2018
Merged

Added gaps at the end of the storage of each contract. #29

merged 2 commits into from
Oct 10, 2018

Conversation

nventuro
Copy link
Contributor

@nventuro nventuro commented Oct 8, 2018

Fixes #13.

Despite IERC721 and friends being contracts and not interfaces (due to a Solidity limitation), I kept this out of those, since they should be used as interfaces.

@nventuro nventuro added this to the v2.0 milestone Oct 8, 2018
@nventuro nventuro requested a review from spalladino October 8, 2018 17:49
@nventuro
Copy link
Contributor Author

nventuro commented Oct 8, 2018

The main reason behind 50 is that you'd need 5 in a row in the inheritance tree to go over 256, causing the PUSH1 to change to PUSH2, making the bytecode longer and deployment more expensive. If we bite the bullet and accept this (very small) extra cost, we could increase the gap size to e.g. 2000, future-proofing this implementation.

@@ -46,4 +46,6 @@ contract Initializable {
assembly { cs := extcodesize(address) }
return cs == 0;
}

uint256[50] private ______gap;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will be removing Initializable and loading it from zos-lib afterwards, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - should we add this there?

spalladino
spalladino previously approved these changes Oct 9, 2018
@spalladino
Copy link
Contributor

I'd say we are fine with 50 slots per contract :-)

@spalladino
Copy link
Contributor

spalladino commented Oct 9, 2018 via email

@nventuro
Copy link
Contributor Author

nventuro commented Oct 9, 2018

Any specific reason to not add the gap? I wouldn't discount the possibility of Initializable becoming more complex and needing more storage.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 67

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 99.758%

Totals Coverage Status
Change from base Build 66: 0.0%
Covered Lines: 577
Relevant Lines: 577

💛 - Coveralls

@spalladino
Copy link
Contributor

I was going to suggest backwards compatibility with the Initializable from 1.x, but that one used a single boolean variable, while the current one uses two. And this exactly proves your point. So, can you send a PR to zeppelinos to add the gap there as well?

@nventuro
Copy link
Contributor Author

Sure thing! Opened OpenZeppelin/openzeppelin-sdk#215

@nventuro nventuro merged commit 488c3de into OpenZeppelin:master Oct 10, 2018
@nventuro nventuro deleted the storage-slots branch October 10, 2018 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants