Add comments

contracts/broker/contracts/src/Broker.sol

@@ -60,6 +60,11 @@ contract Broker is
         payable
     {} // solhint-disable-line no-empty-blocks
 
+    /// @dev The Broker implements the ERC1155 transfer function to be compatible with the ERC1155 asset proxy
+    /// @param from Since the Broker serves as the taker of the order, this should equal `address(this)`
+    /// @param to This should be the maker of the order.
+    /// @param amounts Should be an array of just one `uint256`, specifying the amount of the brokered assets to transfer.
+    /// @param data Encodes the validator contract address and any auxiliary data it needs for property validation.
     function safeBatchTransferFrom(
         address from,
         address to,
@@ -121,6 +126,13 @@ contract Broker is
         }
     }
 
+    /// @dev Fills a single property-based order by the given amount using the given assets.
+    /// @param brokeredAssets Assets specified by the taker to be used to fill the order.
+    /// @param order The property-based order to fill.
+    /// @param takerAssetFillAmount The amount to fill the order by.
+    /// @param signature The maker's signature of the given order.
+    /// @param fillFunctionSelector The selector for either `fillOrder` or `fillOrKillOrder`.
+    /// @return fillResults Amounts filled and fees paid by the maker and taker.
     function brokerTrade(
         bytes[] memory brokeredAssets,
         LibOrder.Order memory order,
@@ -175,6 +187,13 @@ contract Broker is
         return fillResults;
     }
 
+    /// @dev Fills multiple property-based orders by the given amounts using the given assets.
+    /// @param brokeredAssets Assets specified by the taker to be used to fill the orders.
+    /// @param orders The property-based orders to fill.
+    /// @param takerAssetFillAmounts The amounts to fill the orders by.
+    /// @param signatures The makers' signatures for the given orders.
+    /// @param batchFillFunctionSelector The selector for either `batchFillOrders`, `batchFillOrKillOrders`, or `batchFillOrdersNoThrow`.
+    /// @return fillResults Amounts filled and fees paid by the makers and taker.
     function batchBrokerTrade(
         bytes[] memory brokeredAssets,
         LibOrder.Order[] memory orders,

contracts/broker/contracts/src/interfaces/IBroker.sol

@@ -26,6 +26,13 @@ import "@0x/contracts-exchange-libs/contracts/src/LibOrder.sol";
 // solhint-disable space-after-comma
 interface IBroker {
 
+    /// @dev Fills a single property-based order by the given amount using the given assets.
+    /// @param brokeredAssets Assets specified by the taker to be used to fill the order.
+    /// @param order The property-based order to fill.
+    /// @param takerAssetFillAmount The amount to fill the order by.
+    /// @param signature The maker's signature of the given order.
+    /// @param fillFunctionSelector The selector for either `fillOrder` or `fillOrKillOrder`.
+    /// @return fillResults Amounts filled and fees paid by the maker and taker.
     function brokerTrade(
         bytes[] calldata brokeredAssets,
         LibOrder.Order calldata order,
@@ -37,6 +44,13 @@ interface IBroker {
         payable
         returns (LibFillResults.FillResults memory fillResults);
 
+    /// @dev Fills multiple property-based orders by the given amounts using the given assets.
+    /// @param brokeredAssets Assets specified by the taker to be used to fill the orders.
+    /// @param orders The property-based orders to fill.
+    /// @param takerAssetFillAmounts The amounts to fill the orders by.
+    /// @param signatures The makers' signatures for the given orders.
+    /// @param batchFillFunctionSelector The selector for either `batchFillOrders`, `batchFillOrKillOrders`, or `batchFillOrdersNoThrow`.
+    /// @return fillResults Amounts filled and fees paid by the makers and taker.
     function batchBrokerTrade(
         bytes[] calldata brokeredAssets,
         LibOrder.Order[] calldata orders,
@@ -48,6 +62,11 @@ interface IBroker {
         payable
         returns (LibFillResults.FillResults[] memory fillResults);
 
+    /// @dev The Broker implements the ERC1155 transfer function to be compatible with the ERC1155 asset proxy
+    /// @param from Since the Broker serves as the taker of the order, this should equal `address(this)`
+    /// @param to This should be the maker of the order.
+    /// @param amounts Should be an array of just one `uint256`, specifying the amount of the brokered assets to transfer.
+    /// @param data Encodes the validator contract address and any auxiliary data it needs for property validation.
     function safeBatchTransferFrom(
         address from,
         address to,

contracts/broker/contracts/src/interfaces/IGodsUnchained.sol

@@ -22,6 +22,10 @@ pragma experimental ABIEncoderV2;
 
 interface IGodsUnchained {
 
+    /// @dev Returns the proto and quality for a particular card given its token id
+    /// @param tokenId The id of the card to query.
+    /// @return proto The proto of the given card.
+    /// @return quality The quality of the given card
     function getDetails(uint256 tokenId)
         external
         view

contracts/broker/contracts/src/interfaces/IPropertyValidator.sol

@@ -22,6 +22,9 @@ pragma experimental ABIEncoderV2;
 
 interface IPropertyValidator {
 
+    /// @dev Checks that the given asset data satisfies the properties encoded in `propertyData`.
+    /// @param assetData The encoded asset to check.
+    /// @param propertyData Encoded properties or auxiliary data needed to perform the check.
     function checkBrokerAsset(
         bytes calldata assetData,
         bytes calldata propertyData

contracts/broker/contracts/src/validators/GodsUnchainedValidator.sol

@@ -37,6 +37,9 @@ contract GodsUnchainedValidator is
         GODS_UNCHAINED = IGodsUnchained(_godsUnchained);
     }
 
+    /// @dev Checks that the given card (encoded as assetData) has the proto and quality encoded in `propertyData`.
+    /// @param assetData The card (encoded as ERC721 assetData) to check.
+    /// @param propertyData Encoded proto and quality that the card is expected to have.
     function checkBrokerAsset(
         bytes calldata assetData,
         bytes calldata propertyData

contracts/broker/src/gods_unchained_utils.ts

@@ -2,12 +2,23 @@ import { assetDataUtils } from '@0x/order-utils';
 import { AbiEncoder, BigNumber } from '@0x/utils';
 
 export const godsUnchainedUtils = {
+    /**
+     * Encodes the given proto and quality into the bytes format expected by the GodsUnchainedValidator.
+     */
     encodePropertyData(proto: BigNumber, quality: BigNumber): string {
         return AbiEncoder.create([{ name: 'proto', type: 'uint16' }, { name: 'quality', type: 'uint8' }]).encode({
             proto,
             quality,
         });
     },
+    /**
+     * Encodes the given proto and quality into ERC1155 asset data to be used as the takerAssetData
+     * of a property-based GodsUnchained order. Must also provide the address of the Broker and
+     * the GodsUnchainedValidator contract. The optional bundleSize parameter specifies how many
+     * cards are expected for each "unit" of the takerAssetAmount. For example, If the
+     * takerAssetAmount is 3 and the bundleSize is 2, the taker must provide 2, 4, or 6 cards
+     * with the given proto and quality to fill the order. If an odd number is provided, the fill fails.
+     */
     encodeBrokerAssetData(
         brokerAddress: string,
         validatorAddress: string,

contracts/integrations/test/broker/broker_test.ts

@@ -82,7 +82,7 @@ blockchainTests.resets('Broker <> Gods Unchained integration tests', env => {
             feeRecipientAddress: constants.NULL_ADDRESS,
             makerAssetData: assetDataUtils.encodeERC20AssetData(makerToken.address),
             takerAssetData,
-            takerAssetAmount: new BigNumber(2),
+            takerAssetAmount: new BigNumber(2), // buy 2 cards
             makerFeeAssetData: constants.NULL_BYTES,
             takerFeeAssetData: constants.NULL_BYTES,
             makerFee: constants.ZERO_AMOUNT,
@@ -113,7 +113,7 @@ blockchainTests.resets('Broker <> Gods Unchained integration tests', env => {
             StakingProxy: deployment.staking.stakingProxy.address,
         };
         const tokenContracts = {
-            erc20: { makerToken, WETH: deployment.tokens.weth },
+            erc20: { makerToken },
             erc721: { GodsUnchained: new DummyERC721TokenContract(godsUnchained.address, env.provider) },
         };
         const tokenIds = {
@@ -163,6 +163,7 @@ blockchainTests.resets('Broker <> Gods Unchained integration tests', env => {
         let order: SignedOrder;
 
         before(async () => {
+            // The first two cards will satisfy the maker-specified proto and quality
             await godsUnchained
                 .setTokenProperties(godsUnchainedTokenIds[0], makerSpecifiedProto, makerSpecifiedQuality)
                 .awaitTransactionSuccessAsync();
@@ -256,9 +257,9 @@ blockchainTests.resets('Broker <> Gods Unchained integration tests', env => {
             it(`${fnName} with too few assets`, async () => {
                 const tx = broker
                     .brokerTrade(
-                        [erc721AssetData[0]],
+                        [erc721AssetData[0]], // One asset provided
                         order,
-                        new BigNumber(2),
+                        new BigNumber(2), // But two are required for the fill
                         order.signature,
                         deployment.exchange.getSelector(fnName),
                     )
@@ -301,7 +302,7 @@ blockchainTests.resets('Broker <> Gods Unchained integration tests', env => {
                     });
 
                 const expectedBalances = simulateBrokerFills(
-                    [erc721AssetData[0], erc721AssetData[1]],
+                    [erc721AssetData[0], erc721AssetData[1]], // 3rd card isn't transferred
                     [order],
                     [new BigNumber(2)],
                     receipt,
@@ -337,15 +338,10 @@ blockchainTests.resets('Broker <> Gods Unchained integration tests', env => {
                 )
                 .awaitTransactionSuccessAsync({
                     from: taker.address,
-                    value: DeploymentManager.protocolFee.plus(1),
+                    value: DeploymentManager.protocolFee.plus(1), // 1 wei gets refunded
                     gasPrice: DeploymentManager.gasPrice,
                 });
-            const expectedBalances = simulateBrokerFills(
+            const expectedBalances = simulateBrokerFills([erc721AssetData[0]], [order], [new BigNumber(1)], receipt);
-                [erc721AssetData[0]],
-                [order],
-                [new BigNumber(1)],
-                receipt,
-            );
             await balanceStore.updateBalancesAsync();
             balanceStore.assertEquals(expectedBalances);
         });
@@ -355,17 +351,20 @@ blockchainTests.resets('Broker <> Gods Unchained integration tests', env => {
         let orders: SignedOrder[];
 
         before(async () => {
+            // Two orders specifying different protos/qualities
             const firstOrderProto = makerSpecifiedProto;
             const firstOrderQuality = makerSpecifiedQuality;
             const secondOrderProto = new BigNumber(42);
             const secondOrderQuality = new BigNumber(7);
 
+            // First two cards satisfy the proto/quality of the first order
             await godsUnchained
                 .setTokenProperties(godsUnchainedTokenIds[0], firstOrderProto, firstOrderQuality)
                 .awaitTransactionSuccessAsync();
             await godsUnchained
                 .setTokenProperties(godsUnchainedTokenIds[1], firstOrderProto, firstOrderQuality)
                 .awaitTransactionSuccessAsync();
+            // Next two cards satisfy the proto/quality of the second order
             await godsUnchained
                 .setTokenProperties(godsUnchainedTokenIds[2], secondOrderProto, secondOrderQuality)
                 .awaitTransactionSuccessAsync();
@@ -421,7 +420,7 @@ blockchainTests.resets('Broker <> Gods Unchained integration tests', env => {
             it(`${fnName} with two orders, one valid asset each`, async () => {
                 const receipt = await broker
                     .batchBrokerTrade(
-                        [erc721AssetData[0], erc721AssetData[2]],
+                        [erc721AssetData[0], erc721AssetData[2]], // valid for 1st order, valid for 2nd
                         orders,
                         [new BigNumber(1), new BigNumber(1)],
                         [orders[0].signature, orders[1].signature],
@@ -482,7 +481,7 @@ blockchainTests.resets('Broker <> Gods Unchained integration tests', env => {
                     });
 
                 const expectedBalances = simulateBrokerFills(
-                    erc721AssetData.slice(0, 4),
+                    erc721AssetData.slice(0, 4), // 5th card isn't transferred
                     orders,
                     [new BigNumber(2), new BigNumber(2)],
                     receipt,
@@ -494,7 +493,7 @@ blockchainTests.resets('Broker <> Gods Unchained integration tests', env => {
         it(`batchFillOrders reverts on invalid asset`, async () => {
             const tx = broker
                 .batchBrokerTrade(
-                    [...erc721AssetData.slice(0, 3), erc721AssetData[4]],
+                    [...erc721AssetData.slice(0, 3), erc721AssetData[4]], // Last card isn't valid for 2nd order
                     orders,
                     [new BigNumber(2), new BigNumber(2)],
                     [orders[0].signature, orders[1].signature],
@@ -510,7 +509,7 @@ blockchainTests.resets('Broker <> Gods Unchained integration tests', env => {
         it(`batchFillOrKillOrders reverts on invalid asset`, async () => {
             const tx = broker
                 .batchBrokerTrade(
-                    [...erc721AssetData.slice(0, 3), erc721AssetData[4]],
+                    [...erc721AssetData.slice(0, 3), erc721AssetData[4]], // Last card isn't valid for 2nd order
                     orders,
                     [new BigNumber(2), new BigNumber(2)],
                     [orders[0].signature, orders[1].signature],
@@ -526,7 +525,7 @@ blockchainTests.resets('Broker <> Gods Unchained integration tests', env => {
         it(`batchFillOrdersNoThrow catches revert on invalid asset`, async () => {
             const receipt = await broker
                 .batchBrokerTrade(
-                    [...erc721AssetData.slice(0, 3), erc721AssetData[4]],
+                    [...erc721AssetData.slice(0, 3), erc721AssetData[4]], // Last card isn't valid for 2nd order
                     orders,
                     [new BigNumber(2), new BigNumber(2)],
                     [orders[0].signature, orders[1].signature],
@@ -538,7 +537,7 @@ blockchainTests.resets('Broker <> Gods Unchained integration tests', env => {
                     gasPrice: DeploymentManager.gasPrice,
                 });
             const expectedBalances = simulateBrokerFills(
-                erc721AssetData.slice(0, 2),
+                erc721AssetData.slice(0, 2), // First order gets filled
                 [orders[0]],
                 [new BigNumber(2)],
                 receipt,