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 assertions to leaf initializers of (some) pseudo-abstract contr… #27

Merged
merged 2 commits into from Oct 15, 2018
Merged

Added assertions to leaf initializers of (some) pseudo-abstract contr… #27

merged 2 commits into from Oct 15, 2018

Conversation

nventuro
Copy link
Contributor

@nventuro nventuro commented Oct 8, 2018

Fixes #22 (partially)

MintedCrowdsale, FinalizableCrowdsale and PostDeliveryCrowdsale are also leaf contracts, but they have no initialize function: I opted for not adding an empty one with these checks to them, so coverage is not complete. The situation should be better than before this PR, though.

Test coverage will decrease until sc-forks/solidity-coverage#269 is fixed.

@nventuro nventuro added this to the v2.0 milestone Oct 8, 2018
@coveralls
Copy link

coveralls commented Oct 8, 2018

Pull Request Test Coverage Report for Build 68

  • 8 of 8 (100.0%) changed or added relevant lines in 7 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.7%) to 99.055%

Totals Coverage Status
Change from base Build 63: -0.7%
Covered Lines: 585
Relevant Lines: 585

💛 - Coveralls

@spalladino
Copy link
Contributor

I'm not sure I like coupling every derived contract to its parent's internal structure. How about adding an internal view _hasBeenInitialized() method to every crowdsale, and asserting that on the children?

@nventuro
Copy link
Contributor Author

nventuro commented Oct 8, 2018

I was going to do that at first, but wanted to keep this as unobtrusive as possible. If you think that's a good idea though, I'll gladly implement it.

Any ideas on the ones without initialize functions?

@spalladino
Copy link
Contributor

I think it'll be much more clear. As for the contracts without initialize functions, I'm fine with either skipping them, or adding a _hasBeenInitialized() { return true; } implementation.

@nventuro
Copy link
Contributor Author

@spalladino can you take another look at this? Thanks!

@nventuro nventuro merged commit 36043ec into OpenZeppelin:master Oct 15, 2018
@nventuro nventuro deleted the init-asserts branch October 15, 2018 17:32
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.

Add creation-time checks to prevent base contracts from being uninitialized
3 participants