Skip to content

Conversation

@Perseverance
Copy link
Contributor

I've changed the buyTokens method to get the rate from getRate function. This function can later be overridden to make crowdsales with different rate periods based on some business logic.

@Perseverance
Copy link
Contributor Author

Sorry for the messiness with the commits, we should squash them upon merge

@shrugs
Copy link
Contributor

shrugs commented Dec 31, 2017

LGTM, but since this is a breaking change we should settle on a semver strategy before merging (and it should be rebased on to master; 1.5.0 now).

cc @frangio for the semver strategy.

@Perseverance
Copy link
Contributor Author

Correct me if I am wrong, but I rebased already and saw the version is 1.4.0 before upgrading. Am I rebasing to the wrong branch?

@shrugs
Copy link
Contributor

shrugs commented Dec 31, 2017

You're totally right; we forgot to update package.json when we released 1.5.0 a few days ago. https://github.com/OpenZeppelin/zeppelin-solidity/releases

// calculate token amount to be created
uint256 tokens = weiAmount.mul(rate);
uint256 _rate = getRate();
uint256 tokens = weiAmount.mul(_rate);
Copy link

@grimsa grimsa Jan 2, 2018

Choose a reason for hiding this comment

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

Wouldn't extracting getTokenAmount(weiAmount) with default implementation of weiAmount.mul(rate) be more flexible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not really sure which one is more flexible :) I wanted to keep the code as similar to the original one

Copy link

Choose a reason for hiding this comment

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

I had two scenarios in my mind:

  1. Applying multiple price levels to a transaction, for example:
    • You have a crowdsale with a bonus period where you are minting tokens at 25% increased rate until GOAL wei are raised.
    • Let's say you have raised (GOAL - 10 wei) already and a big transaction comes sending 10k ETH. You only want to mint (10 wei * 25%) worth of bonus tokens (not 10K ETH * 25 %).
  2. Rounding problems when original rate = 1. Example:
    • If you want to implement 1 ETH = 1 Token crowdsale, you'll use a rate of 1 and a token with 18 decimal places.
    • Now if you want to issue tokens at 125% rate at first, you have a problem, as getRate result will be 1 (original rate) * 5/4, which rounds down to 1, hence, no bonus.

It is impossible to implement any of these two by just overriding getRate().

As you can always override getTokenAmount(weiAmount) and implement it as weiAmount.mul(getRate()) with a custom getRate() function, to me it seems like a superior approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, will rewrite it ASAP ;)

@Perseverance
Copy link
Contributor Author

@grimsa @shrugs Moved this pull request here for clarity purposes:
#638

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.

5 participants