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

build: dutch auction and factory #36

Merged
merged 24 commits into from
Feb 23, 2024
Merged

build: dutch auction and factory #36

merged 24 commits into from
Feb 23, 2024

Conversation

Schlagonia
Copy link
Collaborator

@Schlagonia Schlagonia commented Jan 25, 2024

Copy link
Collaborator

@fp-crypto fp-crypto left a 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

src/Auctions/AuctionFactory.sol Outdated Show resolved Hide resolved
src/Auctions/Auction.sol Outdated Show resolved Hide resolved
src/Auctions/Auction.sol Show resolved Hide resolved
src/Auctions/Auction.sol Show resolved Hide resolved
src/Auctions/Auction.sol Show resolved Hide resolved
Comment on lines +82 to +90
/// @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;

Copy link
Collaborator

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

Copy link
Collaborator Author

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

src/Auctions/Auction.sol Outdated Show resolved Hide resolved
src/Auctions/Auction.sol Outdated Show resolved Hide resolved
src/Auctions/Auction.sol Outdated Show resolved Hide resolved
src/Auctions/Auction.sol Outdated Show resolved Hide resolved
* @param _from The token that was being sold.
*/
function _disableAuction(address _from) internal virtual {
Auction(auction).disable(_from);
Copy link
Collaborator

Choose a reason for hiding this comment

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

require auction != zero address?

Copy link
Collaborator Author

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

Copy link

@0xkorin 0xkorin left a 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.

src/Auctions/Auction.sol Outdated Show resolved Hide resolved
src/Auctions/Auction.sol Outdated Show resolved Hide resolved
@Schlagonia Schlagonia changed the title Auction factory build: dutch auction and factory Feb 22, 2024
@@ -137,10 +137,14 @@ contract Auction is Governance, ReentrancyGuard {
require(_auctionLength < _auctionCooldown, "cooldown");
require(_startingPrice != 0, "starting price");

// Cannot have more than 18 decimals.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice

@@ -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");
Copy link

@0xkorin 0xkorin Feb 22, 2024

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch

@Schlagonia Schlagonia merged commit d4c3961 into master Feb 23, 2024
3 checks passed
@Schlagonia Schlagonia deleted the auction_factory branch February 23, 2024 01:17
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