chore: Clean up documentation

2 files changed, 58 insertions, 30 deletions

diff --git a/packages/contracts/contracts/extensions/DutchAuction/DutchAuction.sol b/packages/contracts/contracts/extensions/DutchAuction/DutchAuction.sol
index dea836da9..7115e5bd5 100644
--- a/packages/contracts/contracts/extensions/DutchAuction/DutchAuction.sol
+++ b/packages/contracts/contracts/extensions/DutchAuction/DutchAuction.sol
@@ -32,8 +32,8 @@ contract DutchAuction {
     IExchange internal EXCHANGE;
 
     struct AuctionDetails {
-        uint256 beginTimeSeconds;    // Auction begin time in seconds: sellOrder.makerAssetData
-        uint256 endTimeSeconds;      // Auction end time in seconds: sellOrder.expiryTimeSeconds
+        uint256 beginTimeSeconds;    // Auction begin unix timestamp: sellOrder.makerAssetData
+        uint256 endTimeSeconds;      // Auction end unix timestamp: sellOrder.expiryTimeSeconds
         uint256 beginAmount;         // Auction begin amount: sellOrder.makerAssetData
         uint256 endAmount;           // Auction end amount: sellOrder.takerAssetAmount
         uint256 currentAmount;       // Calculated amount given block.timestamp
@@ -80,8 +80,6 @@ contract DutchAuction {
         require(auctionDetails.currentTimeSeconds >= auctionDetails.beginTimeSeconds, "AUCTION_NOT_STARTED");
         // Ensure the auction has not expired. This will fail later in 0x but we can save gas by failing early
         require(sellOrder.expirationTimeSeconds > auctionDetails.currentTimeSeconds, "AUCTION_EXPIRED");
-        // Ensure the auction goes from high to low
-        require(auctionDetails.beginAmount > auctionDetails.endAmount, "INVALID_AMOUNT");
         // Validate the buyer amount is greater than the current auction amount
         require(buyOrder.makerAssetAmount >= auctionDetails.currentAmount, "INVALID_AMOUNT");
         // Match orders, maximally filling `buyOrder`
@@ -91,19 +89,34 @@ contract DutchAuction {
             buySignature,
             sellSignature
         );
-        // Return any spread to the seller
+        // The difference in sellOrder.takerAssetAmount and current amount is given as spread to the matcher
+        // This may include additional spread from the buyOrder.makerAssetAmount and the currentAmount.
+        // e.g currentAmount is 30, sellOrder.takerAssetAmount is 10 and buyOrder.makerAssetamount is 40.
+        // 10 (40-30) is returned to the buyer, 20 (30-10) sent to the seller and 10 has previously
+        // been transferred to the seller during matchOrders
         uint256 leftMakerAssetSpreadAmount = matchedFillResults.leftMakerAssetSpreadAmount;
         if (leftMakerAssetSpreadAmount > 0) {
+            // ERC20 Asset data itself is encoded as follows:
+            //
+            // | Area     | Offset | Length  | Contents                            |
+            // |----------|--------|---------|-------------------------------------|
+            // | Header   | 0      | 4       | function selector                   |
+            // | Params   |        | 1 * 32  | function parameters:                |
+            // |          | 4      | 12 + 20 |   1. token address                  |
+            bytes memory assetData = sellOrder.takerAssetData;
+            address token = assetData.readAddress(16);
             // Calculate the excess from the buy order. This can occur if the buyer sends in a higher
             // amount than the calculated current amount
             uint256 buyerExcessAmount = buyOrder.makerAssetAmount-auctionDetails.currentAmount;
             uint256 sellerExcessAmount = leftMakerAssetSpreadAmount-buyerExcessAmount;
-            bytes memory assetData = sellOrder.takerAssetData;
-            address token = assetData.readAddress(16);
+            // Return the difference between auctionDetails.currentAmount and sellOrder.takerAssetAmount
+            // to the seller
             if (sellerExcessAmount > 0) {
                 address makerAddress = sellOrder.makerAddress;
                 IERC20Token(token).transfer(makerAddress, sellerExcessAmount);
             }
+            // Return the difference between buyOrder.makerAssetAmount and auctionDetails.currentAmount
+            // to the buyer
             if (buyerExcessAmount > 0) {
                 address takerAddress = buyOrder.makerAddress;
                 IERC20Token(token).transfer(takerAddress, buyerExcessAmount);
@@ -122,12 +135,26 @@ contract DutchAuction {
         returns (AuctionDetails memory auctionDetails)
     {
         uint256 makerAssetDataLength = order.makerAssetData.length;
-        // We assume auctionBeginTimeSeconds and auctionBeginAmount are appended to the makerAssetData
+        // It is unknown the encoded data of makerAssetData, we assume the last 64 bytes
+        // are the Auction Details encoding.
+        // Auction Details is encoded as follows:
+        //
+        // | Area     | Offset | Length  | Contents                            |
+        // |----------|--------|---------|-------------------------------------|
+        // | Params   |        | 2 * 32  | parameters:                         |
+        // |          | -64    | 32      |   1. auction begin unix timestamp   |
+        // |          | -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);
         uint256 auctionBeginAmount = order.makerAssetData.readUint256(makerAssetDataLength-32);
+        // Ensure the auction has a valid begin time
         require(order.expirationTimeSeconds > auctionBeginTimeSeconds, "INVALID_BEGIN_TIME");
         uint256 auctionDurationSeconds = order.expirationTimeSeconds-auctionBeginTimeSeconds;
+        // Ensure the auction goes from high to low
         uint256 minAmount = order.takerAssetAmount;
+        require(auctionBeginAmount > minAmount, "INVALID_AMOUNT");
+        uint256 amountDelta = auctionBeginAmount-minAmount;
         // solhint-disable-next-line not-rely-on-time
         uint256 timestamp = block.timestamp;
         auctionDetails.beginTimeSeconds = auctionBeginTimeSeconds;
@@ -137,8 +164,9 @@ contract DutchAuction {
         auctionDetails.currentTimeSeconds = timestamp;
 
         uint256 remainingDurationSeconds = order.expirationTimeSeconds-timestamp;
-        uint256 amountDelta = auctionBeginAmount-minAmount;
         uint256 currentAmount = minAmount + (remainingDurationSeconds*amountDelta/auctionDurationSeconds);
+        // Check the bounds where we SafeMath was avoivded so the auction details can be queried prior
+        // and after the auction time.
         // If the auction has not yet begun the current amount is the auctionBeginAmount
         currentAmount = timestamp < auctionBeginTimeSeconds ? auctionBeginAmount : currentAmount;
         // If the auction has ended the current amount is the minAmount
diff --git a/packages/contracts/test/extensions/dutch_auction.ts b/packages/contracts/test/extensions/dutch_auction.ts
index ae614cbd8..b3408e27d 100644
--- a/packages/contracts/test/extensions/dutch_auction.ts
+++ b/packages/contracts/test/extensions/dutch_auction.ts
@@ -6,6 +6,7 @@ import { Web3Wrapper } from '@0x/web3-wrapper';
 import * as chai from 'chai';
 import ethAbi = require('ethereumjs-abi');
 import * as ethUtil from 'ethereumjs-util';
+import * as _ from 'lodash';
 
 import { DummyERC20TokenContract } from '../../generated-wrappers/dummy_erc20_token';
 import { DummyERC721TokenContract } from '../../generated-wrappers/dummy_erc721_token';
@@ -29,7 +30,7 @@ const expect = chai.expect;
 const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper);
 const DECIMALS_DEFAULT = 18;
 
-describe.only(ContractName.DutchAuction, () => {
+describe(ContractName.DutchAuction, () => {
     let makerAddress: string;
     let owner: string;
     let takerAddress: string;
@@ -46,7 +47,6 @@ describe.only(ContractName.DutchAuction, () => {
     let buyerOrderFactory: OrderFactory;
     let erc20Wrapper: ERC20Wrapper;
     let erc20Balances: ERC20BalancesByOwner;
-    let tenMinutesInSeconds: number;
     let currentBlockTimestamp: number;
     let auctionBeginTimeSeconds: BigNumber;
     let auctionEndTimeSeconds: BigNumber;
@@ -55,6 +55,8 @@ describe.only(ContractName.DutchAuction, () => {
     let sellOrder: SignedOrder;
     let buyOrder: SignedOrder;
     let erc721MakerAssetIds: BigNumber[];
+    const tenMinutesInSeconds = 10 * 60;
+
     async function increaseTimeAsync(): Promise<void> {
         const timestampBefore = await getLatestBlockTimestampAsync();
         await web3Wrapper.increaseTimeAsync(5);
@@ -62,10 +64,6 @@ describe.only(ContractName.DutchAuction, () => {
         // HACK send some transactions when a time increase isn't supported
         if (timestampAfter === timestampBefore) {
             await web3Wrapper.sendTransactionAsync({ to: makerAddress, from: makerAddress, value: new BigNumber(1) });
-            await web3Wrapper.sendTransactionAsync({ to: makerAddress, from: makerAddress, value: new BigNumber(1) });
-            await web3Wrapper.sendTransactionAsync({ to: makerAddress, from: makerAddress, value: new BigNumber(1) });
-            await web3Wrapper.sendTransactionAsync({ to: makerAddress, from: makerAddress, value: new BigNumber(1) });
-            await web3Wrapper.sendTransactionAsync({ to: makerAddress, from: makerAddress, value: new BigNumber(1) });
         }
     }
 
@@ -159,7 +157,6 @@ describe.only(ContractName.DutchAuction, () => {
         web3Wrapper.abiDecoder.addABI(zrxToken.abi);
         erc20Wrapper.addTokenOwnerAddress(dutchAuctionContract.address);
 
-        tenMinutesInSeconds = 10 * 60;
         currentBlockTimestamp = await getLatestBlockTimestampAsync();
         // Default auction begins 10 minutes ago
         auctionBeginTimeSeconds = new BigNumber(currentBlockTimestamp).minus(tenMinutesInSeconds);
@@ -229,8 +226,8 @@ describe.only(ContractName.DutchAuction, () => {
             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);
-            auctionEndTimeSeconds = new BigNumber(currentBlockTimestamp - 100);
+            auctionBeginTimeSeconds = new BigNumber(currentBlockTimestamp - tenMinutesInSeconds * 2);
+            auctionEndTimeSeconds = new BigNumber(currentBlockTimestamp - tenMinutesInSeconds);
             sellOrder = await sellerOrderFactory.newSignedOrderAsync({
                 makerAssetData: extendMakerAssetData(
                     assetDataUtils.encodeERC20AssetData(defaultMakerAssetAddress),
@@ -244,9 +241,9 @@ describe.only(ContractName.DutchAuction, () => {
             expect(auctionDetails.beginAmount).to.be.bignumber.equal(auctionBeginAmount);
         });
         it('should match orders at current amount and send excess to buyer', async () => {
-            const auctionDetails = await dutchAuctionContract.getAuctionDetails.callAsync(sellOrder);
+            const beforeAuctionDetails = await dutchAuctionContract.getAuctionDetails.callAsync(sellOrder);
             buyOrder = await buyerOrderFactory.newSignedOrderAsync({
-                makerAssetAmount: auctionDetails.currentAmount.times(2),
+                makerAssetAmount: beforeAuctionDetails.currentAmount.times(2),
             });
             await web3Wrapper.awaitTransactionSuccessAsync(
                 await dutchAuctionContract.matchOrders.sendTransactionAsync(
@@ -259,6 +256,7 @@ describe.only(ContractName.DutchAuction, () => {
                     },
                 ),
             );
+            const afterAuctionDetails = await dutchAuctionContract.getAuctionDetails.callAsync(sellOrder);
             const newBalances = await erc20Wrapper.getBalancesAsync();
             expect(newBalances[dutchAuctionContract.address][wethContract.address]).to.be.bignumber.equal(
                 constants.ZERO_AMOUNT,
@@ -267,13 +265,13 @@ describe.only(ContractName.DutchAuction, () => {
             // between multiple calls to the same block. Which can move the amount in our case
             // ref: https://github.com/trufflesuite/ganache-core/issues/111
             expect(newBalances[makerAddress][wethContract.address]).to.be.bignumber.gte(
-                erc20Balances[makerAddress][wethContract.address].plus(auctionDetails.currentAmount),
+                erc20Balances[makerAddress][wethContract.address].plus(afterAuctionDetails.currentAmount),
             );
-            expect(newBalances[takerAddress][wethContract.address]).to.be.bignumber.equal(
-                erc20Balances[takerAddress][wethContract.address].minus(auctionDetails.currentAmount),
+            expect(newBalances[takerAddress][wethContract.address]).to.be.bignumber.gte(
+                erc20Balances[takerAddress][wethContract.address].minus(afterAuctionDetails.currentAmount),
             );
         });
-        it('should have valid getAuctionDetails at a block in the future', async () => {
+        it('should have valid getAuctionDetails at some block in the future', async () => {
             let auctionDetails = await dutchAuctionContract.getAuctionDetails.callAsync(sellOrder);
             const beforeAmount = auctionDetails.currentAmount;
             await increaseTimeAsync();
@@ -291,6 +289,8 @@ describe.only(ContractName.DutchAuction, () => {
                 sellOrder.signature,
                 {
                     from: takerAddress,
+                    // HACK geth seems to miscalculate the gas required intermittently
+                    gas: 400000,
                 },
             );
             await web3Wrapper.awaitTransactionSuccessAsync(txHash);
@@ -303,7 +303,6 @@ describe.only(ContractName.DutchAuction, () => {
             sellOrder = await sellerOrderFactory.newSignedOrderAsync({
                 makerFee: new BigNumber(1),
             });
-            const auctionDetails = await dutchAuctionContract.getAuctionDetails.callAsync(sellOrder);
             const txHash = await dutchAuctionContract.matchOrders.sendTransactionAsync(
                 buyOrder,
                 sellOrder,
@@ -314,9 +313,10 @@ describe.only(ContractName.DutchAuction, () => {
                 },
             );
             await web3Wrapper.awaitTransactionSuccessAsync(txHash);
+            const afterAuctionDetails = await dutchAuctionContract.getAuctionDetails.callAsync(sellOrder);
             const newBalances = await erc20Wrapper.getBalancesAsync();
             expect(newBalances[makerAddress][wethContract.address]).to.be.bignumber.gte(
-                erc20Balances[makerAddress][wethContract.address].plus(auctionDetails.currentAmount),
+                erc20Balances[makerAddress][wethContract.address].plus(afterAuctionDetails.currentAmount),
             );
             expect(newBalances[feeRecipientAddress][zrxToken.address]).to.be.bignumber.equal(
                 erc20Balances[feeRecipientAddress][zrxToken.address].plus(sellOrder.makerFee),
@@ -326,7 +326,6 @@ describe.only(ContractName.DutchAuction, () => {
             buyOrder = await buyerOrderFactory.newSignedOrderAsync({
                 makerFee: new BigNumber(1),
             });
-            const auctionDetails = await dutchAuctionContract.getAuctionDetails.callAsync(sellOrder);
             const txHash = await dutchAuctionContract.matchOrders.sendTransactionAsync(
                 buyOrder,
                 sellOrder,
@@ -338,8 +337,9 @@ describe.only(ContractName.DutchAuction, () => {
             );
             await web3Wrapper.awaitTransactionSuccessAsync(txHash);
             const newBalances = await erc20Wrapper.getBalancesAsync();
+            const afterAuctionDetails = await dutchAuctionContract.getAuctionDetails.callAsync(sellOrder);
             expect(newBalances[makerAddress][wethContract.address]).to.be.bignumber.gte(
-                erc20Balances[makerAddress][wethContract.address].plus(auctionDetails.currentAmount),
+                erc20Balances[makerAddress][wethContract.address].plus(afterAuctionDetails.currentAmount),
             );
             expect(newBalances[feeRecipientAddress][zrxToken.address]).to.be.bignumber.equal(
                 erc20Balances[feeRecipientAddress][zrxToken.address].plus(buyOrder.makerFee),
@@ -432,7 +432,6 @@ describe.only(ContractName.DutchAuction, () => {
                     takerAssetAmount: new BigNumber(1),
                     takerAssetData: sellOrder.makerAssetData,
                 });
-                const auctionDetails = await dutchAuctionContract.getAuctionDetails.callAsync(sellOrder);
                 await web3Wrapper.awaitTransactionSuccessAsync(
                     await dutchAuctionContract.matchOrders.sendTransactionAsync(
                         buyOrder,
@@ -444,12 +443,13 @@ describe.only(ContractName.DutchAuction, () => {
                         },
                     ),
                 );
+                const afterAuctionDetails = await dutchAuctionContract.getAuctionDetails.callAsync(sellOrder);
                 const newBalances = await erc20Wrapper.getBalancesAsync();
                 // HACK gte used here due to a bug in ganache where the timestamp can change
                 // between multiple calls to the same block. Which can move the amount in our case
                 // ref: https://github.com/trufflesuite/ganache-core/issues/111
                 expect(newBalances[makerAddress][wethContract.address]).to.be.bignumber.gte(
-                    erc20Balances[makerAddress][wethContract.address].plus(auctionDetails.currentAmount),
+                    erc20Balances[makerAddress][wethContract.address].plus(afterAuctionDetails.currentAmount),
                 );
                 const newOwner = await erc721Token.ownerOf.callAsync(makerAssetId);
                 expect(newOwner).to.be.bignumber.equal(takerAddress);