Skip to content
This repository has been archived by the owner on Jan 24, 2022. It is now read-only.

Add a reference to contract validations in the Writing upgradeable contracts docsite section. #790

Closed
bmiller59 opened this issue Mar 16, 2019 · 3 comments
Labels
kind:enhancement Enhancement to an existing feature

Comments

@bmiller59
Copy link

As described here https://docs.zeppelinos.org/docs/writing_contracts.html, there are various ways to extend/rewrite/upgrade a contract which will cause a failure which may be noticed only once a contract is deployed. This of course could have serious consequences for contracts that are already deployed and in use.

An example of one of the criteria from the page above:

When writing new versions of your contracts, either due to new features or bugfixing, there is an additional restriction to observe: you cannot change the order in which the contract state variables are declared, nor their type.

I would propose that the zos cli and the TestHelper include a contract linter that would look for problems. This linter would also maintain and check necessary information about contract variables and constructor/initializer signatures stored in the zos*.json files. This linter could then prevent catastrophic deployments before they happen.

@jbcarpanelli jbcarpanelli added kind:documentation Write or review documentation content kind:enhancement Enhancement to an existing feature labels Mar 16, 2019
@jbcarpanelli
Copy link
Contributor

jbcarpanelli commented Mar 16, 2019

Hi @bmiller59! ZeppelinOS does include a checker, which indeed verifies the storage layout of your contracts (among other things) between deploys, including the example you provided: If you modify the order of the state variables or you change the type of one of them, and even if you introduce a new one before existing ones, the tool will gently fail to push your new contract implementation and list which storage layout issues encountered:

check
In the gif above, I added a new variable in the middle of my state variables declaration instead of at the end.

In short, the cli will take care, for instance, of letting you know when you are about to modify your storage layout, or on potentially unsafe operations such as using selfdestruct or delegatecall in a wrong way.

Anyway, it might be a good idea to add a paragraph in the documentation site explaining that even though you have to be careful while developing or modifying upgradeable contracts, zos will error when appropiate.
I'm tagging this as a documentation issue 👍

@jbcarpanelli jbcarpanelli changed the title Add a contract upgradeability linter Add a reference to contract validations in the Writing upgradeable contracts docsite section. Mar 16, 2019
@jbcarpanelli jbcarpanelli added topic:docsite and removed kind:documentation Write or review documentation content labels Mar 16, 2019
@bmiller59
Copy link
Author

bmiller59 commented Mar 17, 2019

Great! Very glad to hear this. It was foresighted to add this in already. Now I can rest easier that I won't shoot myself in the foot accidentally. :) I do recommend mentioning in the docs on the "Writing upgradeable contracts" page for clarity. Thank you.

@spalladino
Copy link
Contributor

Hopefully this won't be needed anymore after #1301

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind:enhancement Enhancement to an existing feature
Projects
None yet
Development

No branches or pull requests

3 participants