-
Notifications
You must be signed in to change notification settings - Fork 34
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
build: dutch auction and factory #36
Conversation
5e34531
to
094c5fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good, i need to do a second pass and dig into the test suite
/// @notice The amount to start the auction at. | ||
uint256 public startingPrice; | ||
|
||
/// @notice The time that each auction lasts. | ||
uint256 public auctionLength; | ||
|
||
/// @notice The minimum time to wait between auction 'kicks'. | ||
uint256 public auctionCooldown; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sad these aren't immutable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could do clones with immutables
* @param _from The token that was being sold. | ||
*/ | ||
function _disableAuction(address _from) internal virtual { | ||
Auction(auction).disable(_from); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require auction != zero address?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will revert during the auction.disable() call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just two small comments.
@@ -137,10 +137,14 @@ contract Auction is Governance, ReentrancyGuard { | |||
require(_auctionLength < _auctionCooldown, "cooldown"); | |||
require(_startingPrice != 0, "starting price"); | |||
|
|||
// Cannot have more than 18 decimals. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
src/Auctions/Auction.sol
Outdated
@@ -137,10 +137,14 @@ contract Auction is Governance, ReentrancyGuard { | |||
require(_auctionLength < _auctionCooldown, "cooldown"); | |||
require(_startingPrice != 0, "starting price"); | |||
|
|||
// Cannot have more than 18 decimals. | |||
uint256 decimals = ERC20(_want).decimals(); | |||
require(decimals <= WAD, "unsupported decimals"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be decimals <= 18
, right? or alternatively WAD / 10 ** decimals > 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch
https://hackmd.io/3ag321JBSOi0ijq0HPDoNQ?view