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

constructor modifier #3196

Closed
lorenzb opened this issue Nov 13, 2017 · 12 comments
Closed

constructor modifier #3196

lorenzb opened this issue Nov 13, 2017 · 12 comments

Comments

@lorenzb
Copy link

lorenzb commented Nov 13, 2017

Feature request: Add constructor modifier

Problem

Solidity currently has no special syntax to distinguish a constructor from a function. A constructor is simply a function whose name equals that of the contract.

This introduces a dependence between the name of the contract and the constructor that is easily overlooked when refactoring code, and can have disastrous consequences: If the contract is renamed, but the constructor isn't, the constructor becomes a public function callable by anyone. This allows the state of the contract to be reset or modified by an adversary. There are examples of such a vulnerability in real-world contracts, e.g. in the case of Rubixi.
I have also encountered this issue during smart contract audits.

Suggested solution

I suggest the following path for fixing this issue:

In a first phase, solidity should introduce a new constructor function modifier. The compiler should emit a warning when it encounters a constructor that does not carry the modifier. It should emit an error if it encounters a function that isn't a constructor but that carries the modifier. Here are some examples showing the desired behavior:

No warning

contract Foo {
   function Foo() constructor {
   }
}

Warning

contract Foo {
   function Foo() {
   }
}

Error

contract Bar {
   function Foo() constructor {
   }
}

In a second phase, use of the constructor modifier would be made mandatory, perhaps as part of the 0.5.0 release. Having a constructor without the constructor modifier would result in an error instead of a warning.

Acknowledgements

I would like to thank Florian Tramèr (@ftramer) and Phil Daian (@pdaian) for comments & feedback.

@axic
Copy link
Member

axic commented Nov 13, 2017

If anything, I'd rather prefer constructor Foo() then adding a modifier, because:

  • function suggests the constructor can be executed multiple times (as function in every other case means that) and
  • that having a modifier in today's language semantics would make someone look for the modifier body

@lorenzb
Copy link
Author

lorenzb commented Nov 13, 2017

@axic: My proposal above was meant to fix the issue with as little changes to the language as possible. I completely agree with you: If we can change the syntactic structure of the language, then having constructor Foo() {...} or even just constructor () {...} would definitely be nicer!

@lorenzb
Copy link
Author

lorenzb commented Nov 13, 2017

We could also go the C++/Java/C# route and just have Foo () {...}.

Any option that ensures that renaming a contract will not accidentally turn a constructor into a function is good for me.

@federicobond
Copy link
Contributor

I like constructor() { ... }. We can do it in a backward-compatible way to ease migration and the result looks familiar to JavaScript developers.

As an aside, not sure if your preferred linter does this, but if it doesn't I would highly recommend writing a rule to ban all UpperCamelCase functions that are not the constructor. This, together with the rule for enforcing UpperCamelCase contract names should prevent you from making this particular mistake.

@axic
Copy link
Member

axic commented Nov 13, 2017

Adding a built in "modifier" called constructor would not be a breaking change due to horrible shadowing rules, but in my opinion it would be a bad decision in language design, especially if we wanted to move to a different syntax in 0.5.0 :)

@chriseth
Copy link
Contributor

I also prefer constructor(...) {}.

@3esmit
Copy link
Contributor

3esmit commented Nov 15, 2017

I strongly agree with this, I've many times changed the contract name and forgotten to change the constructor name, but as I was using a full IDE with solc-lint it just remembered "hey this function is not using camelCase".

This should also be linked to #3185 where the LibraryContract should deploy the constructor method, instead of being simply run at creation and not stored.

@chriseth
Copy link
Contributor

This should not be too hard to do once we have the keyword. Anyone opposing to using constructor(...){}? What is the advantage of constructor Foo() {...} ?

@lorenzb
Copy link
Author

lorenzb commented Feb 15, 2018

I am not aware of any technical advantage of constructor Foo() {...} over constructor(...){}. I believe that this is mainly a matter of aesthetics.

Having said that, I personally prefer the D.R.Y. constructor(...){} syntax.

@chriseth chriseth changed the title Feature request: Add constructor modifier constructor modifier Mar 1, 2018
ekpyron added a commit that referenced this issue Mar 2, 2018
@ekpyron ekpyron self-assigned this Mar 2, 2018
erak pushed a commit that referenced this issue Mar 2, 2018
erak pushed a commit that referenced this issue Mar 2, 2018
ekpyron added a commit that referenced this issue Mar 2, 2018
ekpyron added a commit that referenced this issue Mar 2, 2018
@axic
Copy link
Member

axic commented Mar 4, 2018

What is the advantage of constructor Foo() {...} ?

The original proposal suggested function Foo() constructor {}, over that I quickly suggested to not use the modifier and have a keyword and had the example constructor Foo() {}, but nobody (including me) ever championed for keeping the contract name there :)

ekpyron added a commit that referenced this issue Mar 5, 2018
ekpyron added a commit that referenced this issue Mar 5, 2018
WIP: Tests are not adjusted yet.

Remove misleading comment implying that unnamed functions are always anonymous functions, resp. fallback functions.

Constructors with the new syntax are unnamed functions as well.
ekpyron added a commit that referenced this issue Mar 5, 2018
WIP: Tests are not adjusted yet.

Remove misleading comment implying that unnamed functions are always anonymous functions, resp. fallback functions.

Constructors with the new syntax are unnamed functions as well.
ekpyron added a commit that referenced this issue Mar 5, 2018
WIP: Tests are not adjusted yet.

Remove misleading comment implying that unnamed functions are always anonymous functions, resp. fallback functions.

Constructors with the new syntax are unnamed functions as well.
ekpyron added a commit that referenced this issue Mar 5, 2018
WIP: Tests are not adjusted yet.

Remove misleading comment implying that unnamed functions are always anonymous functions, resp. fallback functions.

Constructors with the new syntax are unnamed functions as well.
ekpyron added a commit that referenced this issue Mar 6, 2018
ekpyron added a commit that referenced this issue Mar 6, 2018
ekpyron added a commit that referenced this issue Mar 6, 2018
ekpyron added a commit that referenced this issue Mar 6, 2018
ekpyron added a commit that referenced this issue Mar 6, 2018
ekpyron added a commit that referenced this issue Mar 8, 2018
ekpyron added a commit that referenced this issue Mar 9, 2018
@axic axic closed this as completed in #3635 Apr 3, 2018
@JRSmith-7
Copy link

how can i input an address on constructor? I have added the function to my code to create tokens and now i want to be able to input a central mint address on the constructor's parameters

@axic
Copy link
Member

axic commented Nov 15, 2018

Better to ask the question on https://ethereum.stackexchange.com/ and be a bit more verbose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants