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 PausableCrowdsale contract #832

Merged
merged 32 commits into from
Dec 14, 2018
Merged

Added PausableCrowdsale contract #832

merged 32 commits into from
Dec 14, 2018

Conversation

TalAter
Copy link
Contributor

@TalAter TalAter commented Mar 22, 2018

🚀 Description

Introduces a new contract, the PausableCrowdsale.

PausableCrowdsale extends the standard Crowdsale contract, making it pausable. This is a useful failsafe mechanism used by many crowdsales, but isn't yet offered by zeppelin-solidity.

  • While paused, attempts to buy tokens using either buyTokens() or simple transactions will revert.
  • The crowdsale can only be paused and unpaused by owner.

🦆🦆🦆🦆🦆🦆

  • 📘 I've reviewed the OpenZeppelin Contributor Guidelines
  • ✅ I've added tests where applicable to test my new functionality.
  • 📖 I've made sure that my contracts are well-documented.
  • 🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:all:fix).

come-maiz
come-maiz previously approved these changes Jun 15, 2018
Copy link
Contributor

@come-maiz come-maiz left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @TalAter. This makes sense to me, because just pausing the token doesn't cover all the possible scenarios of crowdsales.
Let's wait for the second review by a fellow maintainer to confirm.

@TalAter
Copy link
Contributor Author

TalAter commented Jun 15, 2018

Thank you @ElOpio

By the way, this code is currently used in an active Crowdsale that has already raised tens of thousands of Ethers. Plus the contract was audited by Matthew Di Ferrante. So that could count as a 3rd and 4th indicator once we have a second 😉

@TalAter
Copy link
Contributor Author

TalAter commented Jul 8, 2018

Small update: our crowdsale which used this contract successfully closed last week raising over 40,000 Ether. No issues were found in this trial by fire.

Copy link

@amateescu amateescu left a comment

Choose a reason for hiding this comment

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

We should change the inheritance order to is Pausable, Crowdsale.

@TalAter
Copy link
Contributor Author

TalAter commented Sep 2, 2018

Nice catch @amateescu
Updated the code. Thank you 👍

@TalAter
Copy link
Contributor Author

TalAter commented Sep 2, 2018

@ElOpio could you re-approve this after the change done in 35e799c ?

Thank you.

@amateescu
Copy link

@TalAter looking at the Travis CI failures, I think the test coverage from this PR needs to be updated.

@TalAter
Copy link
Contributor Author

TalAter commented Sep 2, 2018

Brought this branch up to speed with the latest changes done to HEAD.

  • Merged all the latest changes from HEAD.
  • Broke a line in class to comply with new solium linting max line length.
  • Updated a Mock class I added to use the new constructor syntax.
  • Updated tests to comply with new past-tense event names (introduced in Rename events to past-tense #1181)
  • Updated tests to remove chai-as-promised and calls to should.be.fullfilled (changed in Remove chai-as-promised #1116 )
  • Changed import to require in test (babel was removed in Remove Babel #1074)

@frangio
Copy link
Contributor

frangio commented Sep 4, 2018

Great work @TalAter! We're wrapping up the 2.0 release candidate this week so I'll get back to this in a few days.

@frangio frangio added this to the v2.1 milestone Sep 4, 2018
@frangio frangio self-assigned this Sep 4, 2018
@frangio frangio modified the milestones: Test helpers, v2.1 Nov 20, 2018
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Nice work @TalAter! I merged master into your branch in order to trigger an updated Travis run.

Left some comments. Can you also re-indent your files to 4 spaces? (We made that change recently in #1508.) Thanks!

contracts/crowdsale/PausableCrowdsale.sol Outdated Show resolved Hide resolved
contracts/crowdsale/PausableCrowdsale.sol Outdated Show resolved Hide resolved
test/crowdsale/PausableCrowdsale.test.js Outdated Show resolved Hide resolved
test/crowdsale/PausableCrowdsale.test.js Outdated Show resolved Hide resolved
test/crowdsale/PausableCrowdsale.test.js Outdated Show resolved Hide resolved
@frangio
Copy link
Contributor

frangio commented Dec 11, 2018

Since we want to merge this for v2.1, I implemented the changes myself. (Sorry @TalAter!)

@nventuro can you review this now?

@frangio frangio requested a review from nventuro December 11, 2018 21:52
frangio
frangio previously approved these changes Dec 11, 2018
@frangio frangio dismissed their stale review December 11, 2018 22:47

We need a third pair of eyes.

@nventuro nventuro added feature New contracts, functions, or helpers. contracts Smart contract code. labels Dec 12, 2018
contracts/crowdsale/PausableCrowdsale.sol Outdated Show resolved Hide resolved
contracts/crowdsale/PausableCrowdsale.sol Outdated Show resolved Hide resolved
contracts/mocks/PausableCrowdsaleImpl.sol Outdated Show resolved Hide resolved
test/crowdsale/PausableCrowdsale.test.js Outdated Show resolved Hide resolved
test/crowdsale/PausableCrowdsale.test.js Outdated Show resolved Hide resolved
test/crowdsale/PausableCrowdsale.test.js Outdated Show resolved Hide resolved
test/crowdsale/PausableCrowdsale.test.js Outdated Show resolved Hide resolved
test/crowdsale/PausableCrowdsale.test.js Outdated Show resolved Hide resolved
test/crowdsale/PausableCrowdsale.test.js Outdated Show resolved Hide resolved
test/crowdsale/PausableCrowdsale.test.js Outdated Show resolved Hide resolved
@frangio frangio requested a review from nventuro December 12, 2018 20:35
@frangio frangio requested a review from nventuro December 12, 2018 21:31
@frangio frangio requested a review from nventuro December 13, 2018 13:49
@frangio frangio merged commit 8056433 into OpenZeppelin:master Dec 14, 2018
@TalAter TalAter deleted the feature/pausable-crowdsale branch December 16, 2018 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contracts Smart contract code. feature New contracts, functions, or helpers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants