Composite crowdsale (WIP) #426
Conversation
|
also linked with #317
I think you didn't mean to say twice composition :) Composition seems a good fit in my opinion. |
|
Thx @SylTi for catching that, I've updated the description. |
|
Also concerning this:
A single version is a better in my opinion: easier to maintain & less error prone |
|
I really like the approach this is taking. Kudos! |
|
It would be nice to merge this before the next release! 🥇 |
|
@frangio what need's to be done to merge this in the next release? This will greatly improve the flexibility of the current Crowdsales contract in my opinion. |
|
I think we are all in the same page regarding the benefits of composition over inheritance (both here and in smart contracts in general). Besides, another strong argument for composition that was brought up is that it better fits the on-chain-libraries model that we will be following in the near future with zeppelin_os. As such, it seems a good start to begin imposing that pattern here in OZ. @jakub-wojciechowski what would be missing for this to move out of WIP? After a quick look I'd say that the largest missing feature is a PricingStrategy to go along with the DistributionStrategy. Is there anything else on your mind? |
|
Something else I'd like to discuss is how to manage the shift from the current Crowdsale to this one. Today, most crowdsale contracts in the wild inherit from Crowdsale, and would be broken by shifting to a CompositeCrowdsale. My suggestion would be:
|
|
Also, we should try out implementing known Crowdsale patterns to see how they fit with this composable model: with global cap, with per-user caps, with phases, with whitelists, with different pricing, etc. |
| import '../math/SafeMath.sol'; | ||
|
|
||
| /** | ||
| * @title FixedRateTokenDistributionStrategy |
@spalladino This shouldn't be hard if we add a PricingStrategy like you mentioned before (in combination with DistributionStrategy) |
|
Following @SylTi 's suggestion in the OpenZeppelin Slack, I've pulled Kuba's branch as |
| function getToken() constant returns(ERC20) { | ||
| return token; | ||
| } | ||
| } |
There was a problem hiding this comment.
We probably will need to dissociate ownership from minting right as we talked about in slack.
We can't ever finish the minting with the current version.
|
@shakkaist, @jakub-wojciechowski thanks for your contributions! After playing around with many flavors of the models involving inheritance and composition, we reached the solution in #739, published in v1.7.0. |
5 participants
This PR is connected to Decouple Crowdsale from MintableToken #351 issue.
Fixes #351
It is intended to drive discussion about using composition pattern in OpenZeppelin contract. The proposed changes aren't final and need updated and more testing before are ready to be merged to production code. Tests currently produce output that shows amount of gas consumed by different approaches:
It's still to decide whether we want to use an approach based on composition?
Should we decide to favour either composition or inheritance or should we maintain two separate versions to allow beginners to use simpler approach based on inheritance and offer more advanced users a better customisation via composition?
I'll be very grateful for all of the feedback from you guys.