Skip to content

Conversation

@frangio
Copy link
Contributor

@frangio frangio commented Jul 18, 2017

Fixes #302.

An abstract function getRate() is added to Crowdsale, which allows setting a custom rate function. The function was intentionally left public to allow querying the rate before buying.

The implementation of the current behavior is moved to FixedRate.

Throwing the idea in here to see what people think.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 83.508% when pulling 5c2e866 on frangio:feature/crowdsale-custom-rate into e2fdf09 on OpenZeppelin:master.

@maraoz
Copy link
Contributor

maraoz commented Jul 19, 2017

I like the idea, but I would maybe model it using composition vs what you did with inheritance. I also don't like the name FixedPrice for the proposed model, it should be FixedPriceCrowdsale. What do you think about having a PricingStrategy interface, like a simpler version of TokenMarketNet's?

@frangio
Copy link
Contributor Author

frangio commented Jul 19, 2017

It felt natural to use inheritance because that's how we modeled other crowdsale features (refunds, for example). This feature is a bit different, though. I'll think about it but composition like the PricingStrategy approach sounds good.

We could have CrowdsalePricing contracts with a function getRate(address investor). I can't tell if more parameters are needed.

@jakub-wojciechowski
Copy link
Contributor

I like composition approach much more than the one with inheritance.

BTW, shouldn't we rather calculate a number of tokens that are going to be bought? It will save a lot of hassle for crowdsales with decreasing bonus strategy (like first 1000 ETH 50% of bonus). If someone's donation is overlapping on two different rate zones we could simply calculate the number of tokens, instead of providing an artificial intermediary price. What do you think about it?

@frangio
Copy link
Contributor Author

frangio commented Jul 22, 2017

Great observation. I've actually seen that in the wild. Does CrowdsalePricing still sound like an appropriate name if the functionality is to calculate an amount of tokens?

@eternauta1337
Copy link
Contributor

@frangio
The problem with your suggestion is that it makes the base Crowdsale abstract (i.e. not compilable by itself).
However, your idea was considered in the crowdsale refactor #744. rate is left as is, but getCurrentRate() can be overriden to modify the behavior.

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.

7 participants