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

Partial crowdfund support #51

Closed
wants to merge 21 commits into from
Closed

Partial crowdfund support #51

wants to merge 21 commits into from

Conversation

onggunhao
Copy link
Contributor

@onggunhao onggunhao commented Jul 19, 2019

  • Add principalDisbursed to terms contract
  • Keep track of totalAmountRaised in crowdLoan
  • startLoan in crowdloan should use totalAmountRaised as principalDisbursed
  • rename interestRate to monthlyInterestRate for clarity

@onggunhao onggunhao changed the title WIP: Principal requested Partial crowdfund support Jul 22, 2019
@@ -142,31 +169,13 @@ contract Crowdloan is Initializable, ICrowdloan, ReentrancyGuard {
);

if (termsContract.getLoanStatus() < TermsContractLib.LoanStatus.REPAYMENT_CYCLE) {
termsContract.startLoan();
termsContract.startRepaymentCycle(totalCrowdfunded); // TODO(Dan): change this to be the amount actually raised
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be
termsContract.startRepaymentCycle(_getPrincipalToken().balanceOf(address(this))),
which simply takes the balance at this point in time of the crowdloan contract.
This also removes the need to track totalCrowdfunded within the fund and refund functions.

Copy link
Contributor Author

@onggunhao onggunhao Jul 22, 2019

Choose a reason for hiding this comment

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

@adibas03 Ah yes I thought about that. That approach might create a few problems:

  1. Someone does a native ERC20 transfer to the smart contract, such that _getPrincipalToken().balanceOf(address(this)) is > than principalRequested

  2. In TermsContract, we have a require that checks that principalDisbursed is not more than principalRequested

  3. If that happens, the funds are locked and can't be withdrawn

I think I can resolve it by removing totalCrowdfunded, and instead just passing in the smaller of balanceOf(address(this)) and principalRequested into the startRepaymentCycle function

Copy link
Contributor Author

@onggunhao onggunhao Jul 22, 2019

Choose a reason for hiding this comment

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

Changes made in f1aba44

Tied to issue #52

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we are to use a ceiling, it means we have to implement a cap for the withdrawal function as well, which makes withdrawing in bits more annoying, and would be better we have a simple withdraw function that takes the lesser of principalRequested and balanceOf(address(this)) and transfers the said amount, after which it locks withdrawal

Copy link
Contributor Author

@onggunhao onggunhao Jul 23, 2019

Choose a reason for hiding this comment

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

Ah just saw your message in the other post - makes sense. I'll implement the donations functionality today

#52

@adibas03
Copy link
Collaborator

@onggunhao Is this PR ready?

@adibas03
Copy link
Collaborator

moved to #54

@adibas03 adibas03 closed this Jul 25, 2019
@adibas03 adibas03 deleted the principal-requested branch July 26, 2019 20:16
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.

3 participants