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.

Sign up for GitHub

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

Jump to bottom

Implement partial refunds in CappedCrowdsale. #499

Closed
wants to merge 7 commits into from
Closed

Implement partial refunds in CappedCrowdsale. #499

wants to merge 7 commits into from

Conversation

alcuadrado
Copy link

@alcuadrado alcuadrado commented Oct 12, 2017 •
edited
Loading

This PR modifies the behaviour of CappedCrowdsale when a transaction whose value is larger than the amount needed to reach the cap. Now CappedCrowdsale accepts just enough to reach the cap, and refunds the rest.

@alcuadrado
Add CappedCrowdsale's capReached method.
43e06a1
@alcuadrado
Change CappedCrowdsale's validPurchase logic.
6bc0708 
Make purchases valid if the cap hasn't been reach, despite its value.
@alcuadrado
Abstract Crowdsale#buyTokens' implementation.
339a6ac 
It was moved to an internal method that also receives the amount of wei
to be spent on the purchase.
@alcuadrado
Update CappedCrowdsale#validPuchase.
feeafdf
@alcuadrado
Add a parameter to Crowdsale#forwardFunds.
f785ffa 
A parameter was added in order to have way to control how much is forwarded
to the wallet.
@alcuadrado
Add partial refund logc to CappedCrowdsale.
da6fb8b 
This changes the behaviour when a transaction's value plus the previous
raised amount of wei exceeds the cap. We now accept just enough wei to
reach de cap, and refund the rest.
@alcuadrado
Update CappedCrowdsale's tests.
cc74368
@alcuadrado alcuadrado force-pushed the feature/capped-crowdsale-last-tx-partial-refund branch from 9c9563e to cc74368 Compare October 12, 2017 18:24
@martriay
Copy link
Contributor

martriay commented Oct 13, 2017 •
edited
Loading

@alcuadrado Interesting! What about a sender who only wants the exact amount that she intended to buy?

@eternauta1337
Copy link
Contributor

eternauta1337 commented Jan 5, 2018 •
edited
Loading

@alcuadrado ping?

@alcuadrado
Copy link
Author

@martriay The semantic of this operation is changed to "buy tokens up-to X wei".

There are multiple reasons why I think this change is a good thing:

  1. I think it's more aligned with what most users want to do, specially the ones trying to buy the last few tokens.

  2. This makes the crowdsale's operations simpler, as there's no need to try and send an exact amount of wei to close it.

  3. Depending on the client being used to buy the tokens, closing the crowdsale ASAP can save the buyer's gas, as the clients' UI can be updated earlier to reflect the fact that no purchase will be accepted.

@eternauta1337
Copy link
Contributor

@alcuadrado I love the idea but I don't like the implementation very much =/
Besides, this PR is out of sync with the refactor we are doing to crowdsales #744. It would be awesome if you could revisit this and implement in CappedCrowdsale.

@alcuadrado
Copy link
Author

@ajsantander I don't like this implementation either. I think it would be better to close this PR and reimplement it after #744 gets merged, as that PR it's pretty much an overhaul of everything crowdsale.

@frangio frangio removed the backlog label Jun 15, 2018
@nventuro
Copy link
Contributor

Closing as stale. It is however still true that the last purchasers will need to not go over the cap to prevent triggering a revert.

@nventuro nventuro closed this Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@alcuadrado @martriay @eternauta1337 @nventuro @frangio @facuspagnuolo