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

V0 -> V1 POC review #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

V0 -> V1 POC review #1

wants to merge 1 commit into from

Conversation

0age
Copy link
Contributor

@0age 0age commented Mar 13, 2020

No description provided.

uint256 daiAmount
) external returns (uint256 backstopTokensMinted) {
require(
_status == Status.ACCEPTING_DEPOSITS,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would personally simplify and try to only prevent deposits when auctions are over.

uint256 daiBalance = _DAI.balanceOf(address(this));

// Combine Dai locked in auctions with the balance on the contract.
uint256 combinedDai = daiLockedInAuctions.add(daiBalance);
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to make sure that it's not possible to have a small amount of dai locked in auction that can't be withdrawn, otherwise this call might always end up throwing where daiReeemed end up always > than daiBalance).

require(
_status == Status.ACCEPTING_DEPOSITS,
"Cannot activate again after a prior activation."
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Someone could freeze deposit by spamming activate().

Plus once activated, how do we re-enable deposits? I only see it in Constructor, so looks like the system could permanently prevent further deposits once we have at least 50k DAI.

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.

2 participants