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

Adds Presale app module #52

Open
wants to merge 18 commits into
base: v0.2
from

Conversation

@ajsantander
Copy link

commented Aug 8, 2019

Standalone project: https://github.com/aragonone/fbc

This PR adapts the standalone Presale app so that it acts as a module module-presale within the Fundraising apps.

@osarrouy osarrouy self-requested a review Aug 9, 2019

apps/module-presale/contracts/Presale.sol Outdated Show resolved Hide resolved
apps/module-presale/contracts/Presale.sol Outdated Show resolved Hide resolved
}
}

function _calculateExchangeRate() private {

This comment has been minimized.

Copy link
@osarrouy

osarrouy Aug 9, 2019

Contributor

I'm not sure to understand the rationale behind that formula ... Should the price per token be set at at fixed rate [independently of the BancorFormula parameters ?]

This comment has been minimized.

Copy link
@ajsantander

ajsantander Aug 9, 2019

Author

@lkngtn would you like to comment on this?

This comment has been minimized.

Copy link
@lkngtn

lkngtn Aug 9, 2019

Collaborator

The rationale is that the pre-sale contract is used to bootstrap the bonding curve / market maker with initial collateral. I don't think it makes much sense for this to be tightly integrated with the market maker itself, it should be self-contained within the pre-sale logic.

The idea is to take some parameters as inputs that are intuitive from a user perspective thinking about how much they initially need to raise, how much runway they need, whether there is an initial upfront cost that needs to be released immediate, etc in order to get a project "off the ground".

The exchange rate offered by the pre-sale is fixed based on these parameters and intended to ensure if the pre-sale is successful that we raise a specific amount of funds and have a specific amount of tokens issued when we initialize the curve so that we can have a deterministic starting price and that we know the relative value of the collateral pool and tap rate that we start with.

This comment has been minimized.

Copy link
@ajsantander

ajsantander Aug 13, 2019

Author

@osarrouy do you validate this?

@lkngtn lkngtn referenced this pull request Aug 9, 2019
7 of 10 tasks complete
@sohkai
Copy link

left a comment

Couple of small reminders / configuration notes, and then some thoughts on the contract itself, the most important being #52 (comment).

apps/module-presale/.solcover.js Show resolved Hide resolved
apps/module-presale/manifest.json Outdated Show resolved Hide resolved
apps/module-presale/package.json Outdated Show resolved Hide resolved
apps/module-presale/package.json Outdated Show resolved Hide resolved
apps/module-presale/package.json Outdated Show resolved Hide resolved
uint64 _fundingPeriod,
address _fundraisingPool,
address _beneficiaryAddress,
uint256 _percentFundingForBenefiriary

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 13, 2019

Perhaps it would be useful to allow a user to initialize the app with a pre-determined start date, and offer functionality to change that start date (rather than asking them to send a tx to start the fundraising)?

I imagine most fundraising instances would happen on pre-determined, well-communicated timestamps.

This comment has been minimized.

Copy link
@lkngtn

lkngtn Aug 13, 2019

Collaborator

I like this, it does seem like it would be nice to be able to plan the start time and would make communications easier. 👍

This comment has been minimized.

Copy link
@ajsantander

ajsantander Aug 13, 2019

Author

Yep, this sounds like a very good idea. Confirming with @osarrouy and @lkngtn

This comment has been minimized.

Copy link
@osarrouy

osarrouy Aug 13, 2019

Contributor

Yep. That sounds good to me: a more trustless setup :)

This comment has been minimized.

Copy link
@ajsantander

ajsantander Aug 13, 2019

Author

Ok, I'll implement the change, but not right away, since it's not trivial.

This comment has been minimized.

Copy link
@ajsantander

ajsantander Aug 14, 2019

Author

It's implemented. Unfortunately, we've lost the SaleStarted event, but I believe that the value of this feature is worth it.

apps/module-presale/contracts/Presale.sol Outdated Show resolved Hide resolved
apps/module-presale/contracts/Presale.sol Outdated Show resolved Hide resolved
apps/module-presale/contracts/Presale.sol Outdated Show resolved Hide resolved
apps/module-presale/contracts/Presale.sol Outdated Show resolved Hide resolved
@ajsantander

This comment has been minimized.

Copy link
Author

commented Aug 13, 2019

Thx for the review @sohkai!! All suggestions reviewed, except for the auto start feature. That one is a bit more involved. I'll tackle it later.

@sohkai

This comment has been minimized.

Copy link

commented Aug 15, 2019

@ajsantander The changes look good to me; I'll leave the rest to you and Olivier :)!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.