From 4ab5951c25428bec8242937bbeea24189ce997c5 Mon Sep 17 00:00:00 2001 From: Michael Zhu Date: Tue, 28 Jan 2020 16:52:26 -0800 Subject: [PATCH] Add comments --- contracts/broker/contracts/src/Broker.sol | 19 ++++++++++ .../contracts/src/interfaces/IBroker.sol | 19 ++++++++++ .../src/interfaces/IGodsUnchained.sol | 4 +++ .../src/interfaces/IPropertyValidator.sol | 3 ++ .../src/validators/GodsUnchainedValidator.sol | 3 ++ contracts/broker/src/gods_unchained_utils.ts | 11 ++++++ .../integrations/test/broker/broker_test.ts | 35 +++++++++---------- 7 files changed, 76 insertions(+), 18 deletions(-) diff --git a/contracts/broker/contracts/src/Broker.sol b/contracts/broker/contracts/src/Broker.sol index 1fa86a849e..6cbd9357af 100644 --- a/contracts/broker/contracts/src/Broker.sol +++ b/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, diff --git a/contracts/broker/contracts/src/interfaces/IBroker.sol b/contracts/broker/contracts/src/interfaces/IBroker.sol index d3ef01648a..ed9e9a77ee 100644 --- a/contracts/broker/contracts/src/interfaces/IBroker.sol +++ b/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, diff --git a/contracts/broker/contracts/src/interfaces/IGodsUnchained.sol b/contracts/broker/contracts/src/interfaces/IGodsUnchained.sol index ccf4e06785..b8276b787a 100644 --- a/contracts/broker/contracts/src/interfaces/IGodsUnchained.sol +++ b/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 diff --git a/contracts/broker/contracts/src/interfaces/IPropertyValidator.sol b/contracts/broker/contracts/src/interfaces/IPropertyValidator.sol index acfb3f6373..d8e6e07d2d 100644 --- a/contracts/broker/contracts/src/interfaces/IPropertyValidator.sol +++ b/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 diff --git a/contracts/broker/contracts/src/validators/GodsUnchainedValidator.sol b/contracts/broker/contracts/src/validators/GodsUnchainedValidator.sol index a643cf8dff..9f986af2ea 100644 --- a/contracts/broker/contracts/src/validators/GodsUnchainedValidator.sol +++ b/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 diff --git a/contracts/broker/src/gods_unchained_utils.ts b/contracts/broker/src/gods_unchained_utils.ts index 18341ceada..3094df186b 100644 --- a/contracts/broker/src/gods_unchained_utils.ts +++ b/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, diff --git a/contracts/integrations/test/broker/broker_test.ts b/contracts/integrations/test/broker/broker_test.ts index d0b7b75fa2..fe7bc632f1 100644 --- a/contracts/integrations/test/broker/broker_test.ts +++ b/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( - [erc721AssetData[0]], - [order], - [new BigNumber(1)], - receipt, - ); + const expectedBalances = simulateBrokerFills([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,