Implement Basic Dutch Auction #1225

dekz · 2018-11-07T20:37:01Z

Description

Quip doc here for further details.

A simple matchOrders in a extension contract that validates the correct amount given the current block timestamp. sellOrder is the smallest possible amount for the asset up for sale. That is the sellOrder.takerAssetAmount is the final amount at the end of the auction. The contract guarantees the incoming buyOrder is for an greater than or equal to the calculated currentAmount. The currentAmount is calculated from the auctionBeginTimeSeconds, auctionBeginAmount, sellOrder.takerAssetAmount aka auctionEndAmount and sellOrder.expiryTimeSeconds aka auctionEndTimeSeconds. currentAmount travels linearly downwards from the auctionBeginAmount to the auctionEndAmount.

Match Orders guarantees the seller gets the minimum reserve amount (sellOrder.takerAssetAmount). When matched the delta between currentAmount and sellOrder.takerAssetAmount is transferred to the seller. Any further excess is returned to the buyer (the delta between buyOrder.makerAssetAmount and currentAmount. This means the buyer can send in any amount safely and be assured they will get the exact price given the current block timestamp.

Testing instructions

Types of changes

Checklist:

Prefix PR title with [WIP] if necessary.
Prefix PR title with bracketed package name(s) corresponding to the changed package(s). For example: [sol-cov] Fixed bug.
Add tests to cover changes as needed.
Update documentation as needed.
Add new entries to the relevant CHANGELOG.jsons.

coveralls · 2018-11-07T20:44:28Z

Coverage increased (+0.02%) to 85.285% when pulling bb4b516 on feature/contracts/dutch-auction into fc3641b on development.

hysz · 2018-11-27T00:47:09Z

packages/contracts/contracts/extensions/DutchAuction/DutchAuction.sol

+    IExchange internal EXCHANGE;
+
+    struct AuctionDetails {
+        uint256 beginTimeSeconds;    // Auction begin time in seconds: sellOrder.makerAssetData


Nit - specify these are unix timestamps.

hysz · 2018-11-27T00:48:28Z

packages/contracts/test/extensions/dutch_auction.ts

+const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper);
+const DECIMALS_DEFAULT = 18;
+
+describe.only(ContractName.DutchAuction, () => {


Note to nix the only before merge.

hysz · 2018-11-27T00:51:16Z

packages/contracts/test/extensions/dutch_auction.ts

+    let buyerOrderFactory: OrderFactory;
+    let erc20Wrapper: ERC20Wrapper;
+    let erc20Balances: ERC20BalancesByOwner;
+    let tenMinutesInSeconds: number;


Nit - some of these could be const like const tenMinutesInSeconds = 10 * 60

hysz · 2018-11-27T00:56:34Z

packages/contracts/test/extensions/dutch_auction.ts

+            expect(auctionDetails.beginAmount).to.be.bignumber.equal(auctionBeginAmount);
+        });
+        it('should be be worth the end price at the end of the auction', async () => {
+            auctionBeginTimeSeconds = new BigNumber(currentBlockTimestamp - 1000);


Nit - could be good to set 1000 and 100 to variables for clarity.

I'll re-use tenMinutesInSeconds

hysz · 2018-11-27T01:05:28Z

packages/contracts/contracts/extensions/DutchAuction/DutchAuction.sol

+            uint256 buyerExcessAmount = buyOrder.makerAssetAmount-auctionDetails.currentAmount;
+            uint256 sellerExcessAmount = leftMakerAssetSpreadAmount-buyerExcessAmount;
+            bytes memory assetData = sellOrder.takerAssetData;
+            address token = assetData.readAddress(16);


Nit - include an offset table for reference. Example.

hysz · 2018-11-27T01:08:12Z

packages/contracts/contracts/extensions/DutchAuction/DutchAuction.sol

+        uint256 leftMakerAssetSpreadAmount = matchedFillResults.leftMakerAssetSpreadAmount;
+        if (leftMakerAssetSpreadAmount > 0) {
+            // Calculate the excess from the buy order. This can occur if the buyer sends in a higher
+            // amount than the calculated current amount


Should probably use safeMath here or include a comment about why it's definitely not needed.

Agreed with all of the SafeMath comments.

hysz · 2018-11-27T01:12:33Z

packages/contracts/contracts/extensions/DutchAuction/DutchAuction.sol

+        returns (AuctionDetails memory auctionDetails)
+    {
+        uint256 makerAssetDataLength = order.makerAssetData.length;
+        // We assume auctionBeginTimeSeconds and auctionBeginAmount are appended to the makerAssetData


Same note as above - should include an offset table for reference. Example.
Could probably just include this once in here then reference it above.

hysz · 2018-11-27T01:13:21Z

packages/contracts/contracts/extensions/DutchAuction/DutchAuction.sol

+        // We assume auctionBeginTimeSeconds and auctionBeginAmount are appended to the makerAssetData
+        uint256 auctionBeginTimeSeconds = order.makerAssetData.readUint256(makerAssetDataLength-64);
+        uint256 auctionBeginAmount = order.makerAssetData.readUint256(makerAssetDataLength-32);
+        require(order.expirationTimeSeconds > auctionBeginTimeSeconds, "INVALID_BEGIN_TIME");


Same note about safeMath in this function.

hysz · 2018-11-27T01:15:16Z

packages/contracts/contracts/extensions/DutchAuction/DutchAuction.sol

+        EXCHANGE = IExchange(_exchange);
+    }
+
+    /// @dev Matches the buy and sell orders at an amount given the following: the current block time, the auction


abandeali1 · 2018-11-27T02:00:00Z

packages/contracts/contracts/extensions/DutchAuction/DutchAuction.sol

+    {
+        AuctionDetails memory auctionDetails = getAuctionDetails(sellOrder);
+        // Ensure the auction has not yet started
+        require(auctionDetails.currentTimeSeconds >= auctionDetails.beginTimeSeconds, "AUCTION_NOT_STARTED");


Let's put the revert reasons on a newline.

abandeali1 · 2018-11-27T02:01:25Z

packages/contracts/contracts/extensions/DutchAuction/DutchAuction.sol

+            buySignature,
+            sellSignature
+        );
+        // Return any spread to the seller


This comment is outdated.

abandeali1 · 2018-11-27T02:09:41Z

packages/contracts/contracts/extensions/DutchAuction/DutchAuction.sol

+        uint256 leftMakerAssetSpreadAmount = matchedFillResults.leftMakerAssetSpreadAmount;
+        if (leftMakerAssetSpreadAmount > 0) {
+            // Calculate the excess from the buy order. This can occur if the buyer sends in a higher
+            // amount than the calculated current amount


Agreed with all of the SafeMath comments.

abandeali1 · 2018-11-27T02:11:28Z

packages/contracts/contracts/extensions/DutchAuction/DutchAuction.sol

+            bytes memory assetData = sellOrder.takerAssetData;
+            address token = assetData.readAddress(16);
+            if (sellerExcessAmount > 0) {
+                address makerAddress = sellOrder.makerAddress;


We probably don't need to declare makerAddress here since it isn't being reused.

abandeali1 · 2018-11-27T02:11:38Z

packages/contracts/contracts/extensions/DutchAuction/DutchAuction.sol

+                IERC20Token(token).transfer(makerAddress, sellerExcessAmount);
+            }
+            if (buyerExcessAmount > 0) {
+                address takerAddress = buyOrder.makerAddress;


Same as above.

abandeali1 · 2018-11-27T15:57:06Z

packages/contracts/contracts/extensions/DutchAuction/DutchAuction.sol

+        // |          | -32    | 32      |   2. auction begin begin amount     |
+        // ERC20 asset data length is 4+32, 64 for auction details results in min length if 100
+        require(makerAssetDataLength > 10, "INVALID_ASSET_DATA");
+        uint256 auctionBeginTimeSeconds = order.makerAssetData.readUint256(makerAssetDataLength-64);


Spaces before/after operators please :) makerAssetDataLength - 64

abandeali1

LGTM after fixing merge conflicts.

abandeali1 · 2018-12-03T20:20:20Z

packages/contracts/contracts/extensions/DutchAuction/DutchAuction.sol

+        auctionDetails.currentTimeSeconds = timestamp;
+
+        uint256 remainingDurationSeconds = order.expirationTimeSeconds-timestamp;
+        uint256 currentAmount = minAmount + (remainingDurationSeconds*amountDelta/auctionDurationSeconds);


Nit: add spaces in between arithmetic operators

dekz requested a review from albrow as a code owner November 7, 2018 20:37

dekz force-pushed the feature/contracts/dutch-auction branch from 10cf655 to 287a83b Compare November 7, 2018 20:54

dekz force-pushed the feature/contracts/dutch-auction branch 3 times, most recently from f7e260a to 06d80cc Compare November 16, 2018 09:45

dekz requested review from abandeali1 and hysz November 16, 2018 10:56

dekz added the contracts label Nov 16, 2018

dekz force-pushed the feature/contracts/dutch-auction branch from cee5bd0 to ea4b21f Compare November 22, 2018 04:41

LogvinovLeon assigned dekz Nov 26, 2018

hysz reviewed Nov 27, 2018

View reviewed changes

dekz force-pushed the feature/contracts/dutch-auction branch from 06dab7c to 630aa70 Compare November 27, 2018 04:46

abandeali1 suggested changes Nov 27, 2018

View reviewed changes

dekz force-pushed the feature/contracts/dutch-auction branch from 521e9c1 to 0d814ee Compare November 30, 2018 02:00

probot-autolabeler bot added the @0x/types label Nov 30, 2018

dekz force-pushed the feature/contracts/dutch-auction branch from 420f71c to 271a07a Compare November 30, 2018 02:27

dekz changed the title ~~[WIP] Implement Basic Dutch Auction~~ Implement Basic Dutch Auction Nov 30, 2018

dekz force-pushed the feature/contracts/dutch-auction branch 2 times, most recently from e9b30fe to bb4b516 Compare November 30, 2018 05:39

abandeali1 approved these changes Dec 3, 2018

View reviewed changes

abandeali1 reviewed Dec 3, 2018

View reviewed changes

dekz force-pushed the feature/contracts/dutch-auction branch from 61aa012 to 907b00d Compare December 3, 2018 22:12

dekz added 7 commits December 4, 2018 09:13

feat: Dutch Auction

da8c27c

chore: time movement for geth

98d9a9c

chore: Rename price to amount. Encode parameters in makerAssetData

cb86016

chore: taker address and zrx fee test

7934d77

chore: update documentation

54aaee8

chore: Return excess to buyer

a5eb1bc

chore: Work around for Ganache timestamp bug

afa24aa

dekz added 4 commits December 4, 2018 09:13

chore: Clean up documentation

6b1fea6

chore: Use SafeMath where possible

a882a39

chore: Add RevertReasons for DutchAuction. Test revert reasons

0bffc3d

chore: SafeMath and if statement in getAuctionDetails

247266b

dekz force-pushed the feature/contracts/dutch-auction branch from 907b00d to 247266b Compare December 3, 2018 22:14

chore: Move to new structure

098a531

dekz merged commit a1e985a into development Dec 4, 2018

dekz deleted the feature/contracts/dutch-auction branch December 4, 2018 01:44

adamlaska mentioned this pull request Nov 5, 2022

[Snyk] Fix for 1 vulnerabilities adamlaska/0x-monorepo#184

Open

adamlaska mentioned this pull request Dec 25, 2022

[Snyk] Fix for 1 vulnerabilities adamlaska/0x-monorepo#189

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Basic Dutch Auction #1225

Implement Basic Dutch Auction #1225

dekz commented Nov 7, 2018 •

edited

Loading

coveralls commented Nov 7, 2018 •

edited

Loading

hysz Nov 27, 2018

hysz Nov 27, 2018

hysz Nov 27, 2018

hysz Nov 27, 2018

dekz Nov 27, 2018

hysz Nov 27, 2018

hysz Nov 27, 2018

abandeali1 Nov 27, 2018

hysz Nov 27, 2018

hysz Nov 27, 2018

hysz Nov 27, 2018

abandeali1 Nov 27, 2018

abandeali1 Nov 27, 2018

abandeali1 Nov 27, 2018

abandeali1 Nov 27, 2018

abandeali1 Nov 27, 2018

abandeali1 Nov 27, 2018

abandeali1 left a comment

abandeali1 Dec 3, 2018

Implement Basic Dutch Auction #1225

Implement Basic Dutch Auction #1225

Conversation

dekz commented Nov 7, 2018 • edited Loading

Description

Testing instructions

Types of changes

Checklist:

coveralls commented Nov 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abandeali1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dekz commented Nov 7, 2018 •

edited

Loading

coveralls commented Nov 7, 2018 •

edited

Loading