Atomic Order Matching #572

hysz · 2018-05-05T01:12:46Z

Description

Atomic Order Matching - Work in Progress.

Match two complementary orders that have a positive spread. Each order is filled at their respective price point. However, the calculations are carried out as though the orders are both being filled at the right order's price point. The profit made by the left order goes to the taker (who matched the two orders).

This PR is based on two earlier PR's by Remco:

In this PR:

Refactoring of exchange core
Atomic order matching mixin
Lots of test cases for atomic order matching: ERC20 / ERC721
Several detailed comments inline
Rebased against Solidity v0.4.23

Todo:

I think we have a good test set for the order matching. With the refactoring in this PR, we could write more granular test cases for the exchange core.

Motivation and Context

The exchange core functions were getting really long. It was good to refactor/split these up. The refactor was also necessary to write a clean implementation of order matching, since there's a lot of overlap between functions.

Atomic order matching is useful for arbitrage and exchanges that take profit from the spread.

How Has This Been Tested?

Test cases were created for new functionality. We did not change any existing functionality. Tests still pass.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

Change requires a change to the documentation.
Added tests to cover my changes.
Added new entries to the relevant CHANGELOG.jsons.
Labeled this PR with the 'WIP' label if it is a work in progress.
Labeled this PR with the labels corresponding to the changed package.

abandeali1

Good work! Going to have to give this another pass tomorrow.

abandeali1 · 2018-05-07T01:40:56Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol

+    /// @return status Status of order. Statuses are defined in the LibStatus.Status struct.
+    /// @return orderHash Keccak-256 EIP712 hash of the order.
+    /// @return filledAmount Amount of order that has been filled.
+    function getOrderInfo(Order memory order)
        public


This should probably be internal.

I originally set it up this way so that it could be used by our tests -- the atomic order matching tests make extensive use of it. Is there any harm in keeping it public?

^ Could also be used by the forwarding contract. It should probably also be moved into LibOrder. Then we could use the [ContractName]Test pattern you used in the Solidity 0.4.23 PR.

abandeali1 · 2018-05-07T01:41:23Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol

+        returns (
+            uint8 orderStatus,
+            bytes32 orderHash,
+            uint256 filledAmount)


Let's rename this takerAssetFilledAmount for consistency.

abandeali1 · 2018-05-07T02:03:22Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol

+    {
+        // Ensure order status is not invalid
+        if (orderStatus == uint8(Status.INVALID)) {
+            emit ExchangeStatus(uint8(orderStatus), orderHash);


revert reverts all state, including events, so this patten won't work. Instead, let's just use a require with a reason.

👍 I'll replace with require(..., [message]) after rebasing against v0.4.23

abandeali1 · 2018-05-07T02:04:44Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol

+    /// @param takerAssetFillAmount Desired amount of order to fill by taker.
+    /// @return status Return status of calculating fill amounts. Returns Status.SUCCESS on success.
+    /// @return fillResults Amounts filled and fees paid by maker and taker.
+    function calculateFillAmounts(


I'd rename this to calculateFillResults.

abandeali1 · 2018-05-07T02:04:59Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol

+            FillResults memory fillResults)
+    {
+        // Fill Amount must be greater than 0
+        if(takerAssetFillAmount <= 0) {


Missing a space after if here.

abandeali1 · 2018-05-07T02:17:58Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol

+                status = uint8(Status.ROUNDING_ERROR_TOO_LARGE);
+                return (status, matchedFillOrderAmounts);
+            }
+        } else {


There's a lot of duplicate logic in this else block. Can we break this out into another function?

Good call 👍

abandeali1 · 2018-05-07T02:19:16Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinSettlement.sol

+        Order memory leftOrder,
+        Order memory rightOrder,
+        MatchedOrderFillAmounts memory matchedFillOrderAmounts,
+        address taker)


takerAddress

abandeali1 · 2018-05-07T02:21:30Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol

+        bytes rightSignature)
+        public
+        returns (
+            MatchedOrderFillAmounts memory matchedFillOrderAmounts)


Can we put this on the same line as returns?

abandeali1 · 2018-05-07T02:21:45Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol

+    /// @param rightSignature Proof that order was created by the right maker.
+    /// @return leftFillResults Amounts filled and fees paid by maker and taker of left order.
+    /// @return leftFillResults Amounts filled and fees paid by maker and taker of right order.
+    function matchOrders(


Nice! This function is very readable/easy to understand.

abandeali1 · 2018-05-07T02:24:23Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol

+            MatchedOrderFillAmounts memory matchedFillOrderAmounts)
+    {
+        // Get left status
+        OrderInfo memory leftOrderInfo;


Is this a struct because of the stack variable limitations?

Yeah =\ ... I left a note on the struct definition.

abandeali1 · 2018-05-11T18:20:24Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol

@@ -48,64 +51,108 @@ contract MixinExchangeCore is
    // Orders with a salt less than their maker's epoch are considered cancelled
    mapping (address => uint256) public makerEpoch;

-    /// @dev Cancels all orders reated by sender with a salt less than or equal to the specified salt value.
-    /// @param salt Orders created with a salt less or equal to this value will be cancelled.
-    function cancelOrdersUpTo(uint256 salt)


The order for functions within our contracts should be external, public, and then internal. WIthin each visibility section, they should be ordered by how frequently we think they will be used.

abandeali1 · 2018-05-11T18:20:44Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol

+    /// @return status Status of order. Statuses are defined in the LibStatus.Status struct.
+    /// @return orderHash Keccak-256 EIP712 hash of the order.
+    /// @return takerAssetFilledAmount Amount of order that has been filled.
+    function getOrderInfo(Order memory order)


Let's make this internal.

Knowing the amount remaining to be filled is needed in the forwarding contract. Is there an alternative? In the mean time I have been keeping getUnavailable around in my PR.

@dekz - I chatted offline with @abandeali1 about this & we decided to keep it public. Hoping it can be useful in the forwarding contract, for getting amount to fill & order state.

Yep either this or calculateFillResults would be ideal. If it was calculateFillResults it requires getOrderInfo, but would save also save us duplicating this calculation (and and edge cases) in the forwarding contract.

I'd be in favor of making calculateFillResults public if it can simplify the forwarding contract. @abandeali1 Thoughts?

abandeali1 · 2018-05-11T18:31:46Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol

-        bytes32 orderHash = getOrderHash(order);
+        // Compute the order hash and fetch filled amount
+        orderHash = getOrderHash(order);
+        takerAssetFilledAmount = filled[orderHash];


We should make this check right before we actually need it. For the sake of efficiency, the checks should be ordered:

makerAssetAmount

takerAssetAmount

block.timestamp

cancelled

makerEpoch
(For all of the above, takerAssetFilledAmount will be 0)

takerAssetFilledAmount (don't actually check the filled mapping until right before this check)

abandeali1 · 2018-05-11T18:37:43Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol

+    /// @param takerAddress Address of order taker.
+    /// @param takerAssetFillAmount Desired amount of order to fill by taker.
+    function validateFillOrderContextOrRevert(
+        Order memory order,


Let's group these params by type.

abandeali1 · 2018-05-11T18:39:05Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol

+    /// @param signature Proof that the orders was created by its maker.
+    /// @param takerAddress Address of order taker.
+    /// @param takerAssetFillAmount Desired amount of order to fill by taker.
+    function validateFillOrderContextOrRevert(


I think this name is a little confusing. Maybe just validateFillOrRevert?

abandeali1 · 2018-05-11T19:29:29Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol

+            rightOrderFilledAmount,
+            rightOrderAmountToFill);
+        if (status != uint8(Status.SUCCESS)) {
+            return (status, matchedFillResults);


Hmm, if the left status is successful but the right is not, matchedFillResults will be inaccurate here.

abandeali1 · 2018-05-11T19:31:10Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol

+        ) = getOrderInfo(rightOrder);
+        if (rightOrderInfo.orderStatus != uint8(Status.ORDER_FILLABLE)) {
+            emit ExchangeStatus(uint8(rightOrderInfo.orderStatus), rightOrderInfo.orderHash);
+            return matchedFillResults;


Same comment as above - matchedFillResults can be inaccurate here.

abandeali1 · 2018-05-11T19:36:56Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol

+        // Update exchange state
+        updateFilledState(
+            leftOrder,
+            rightOrder.makerAddress,


We should just use takerAddress for both updateFilledState calls.

abandeali1 · 2018-05-11T19:53:24Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinSettlement.sol

+
+        // leftOrder.MakerAsset == rightOrder.TakerAsset
+        // Taker should be left with a positive balance (the spread)
+        dispatchTransferFrom(


Currently, transfers look like:
leftMaker => taker
taker => rightMaker
rightMaker => leftMaker
The spread is always coming from the leftMaker.

However, we never want the taker to actually take custody of the tokens (other than the spread). So what would be more desirable would be:
leftMaker => rightMaker
rightMaker => leftMaker
leftMaker => taker

I agree, this feels cleaner 👍

abandeali1 · 2018-05-11T19:54:46Z

packages/contracts/src/contracts/current/protocol/Exchange/mixins/MExchangeCore.sol

+        returns (
+            uint8 orderStatus,
+            bytes32 orderHash,
+            uint256 takerAssetFilledAmount);


Ending parenthesis should be on a newline when we have multiple line returns.

fabioberger · 2018-05-15T08:29:55Z

packages/contracts/src/contracts/current/protocol/Exchange/Exchange.sol

        MixinTransactions()
+        MixinAssetProxyDispatcher()
+        MixinWrapperFunctions()


Can we perhaps order these alphabetically?

So there actually is (now) a method to this ordering: contracts are instantiated in the order they are inherited. This makes it easy to update by simply copying the inheritance list to the constructor list. It also makes it easy to verify that all constructors are being called.

I've added a comment above the constructor to clarify this.

(also worth noting that the inheritance list must be ordered by least-to-most derived)

fabioberger · 2018-05-15T08:33:27Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol

-        if (cancelled[orderHash]) {
-            emit ExchangeError(uint8(Errors.ORDER_CANCELLED), orderHash);
+        // Either our context is valid or we revert
+        validateFillOrRevert(


This method is analogous to an assertion, do we want to rename it to assertValidFill?

fabioberger · 2018-05-15T08:56:08Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol

+            order.takerAssetAmount,
+            order.makerAssetAmount))
+        {
+            status = uint8(Status.ROUNDING_ERROR_TOO_LARGE);


Or perhaps don't set it to fillResults.takerAssetFilledAmount until before returning below?

fabioberger · 2018-05-15T09:01:10Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol

+    ///      Throws if order is invalid or sender does not have permission to cancel.
+    /// @param order Order to cancel. Order must be Status.FILLABLE.
+    /// @return True if the order state changed to cancelled.
+    ///         False if the order was order was in a valid, but


Wording: if the order was order was

fabioberger · 2018-05-15T09:02:53Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol

+        (orderStatus, orderHash, ) = getOrderInfo(order);
+
+        // Validate context
+        validateCancelOrRevert(order, orderStatus, orderHash);


assertValidCancel

fabioberger · 2018-05-15T11:58:42Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol

+            matchedFillResults.right
+        );
+
+        // Return results


Think we can remove this comment.

fabioberger · 2018-05-15T12:09:06Z

packages/contracts/src/utils/exchange_wrapper.ts

@@ -225,6 +225,30 @@ export class ExchangeWrapper {
        const filledAmount = new BigNumber(await this._exchange.filled.callAsync(orderHashHex));
        return filledAmount;
    }
+    public async getOrderInfoAsync(
+        signedOrder: SignedOrder,
+    ): Promise<[number /* orderStatus */, string /* orderHash */, BigNumber /* orderTakerAssetAmountFilled */]> {


Do these comments show up in-line in text editors? If not, why don't we marshal the array into an object with named parameters? I don't think keeping the exact same interface as the contract is important here.

Commented above about this contract function returning an OrderInfo struct, if it did it would also be typed in the generated contract wrappers and would bubble up here.

fabioberger · 2018-05-15T12:13:09Z

packages/contracts/test/utils/match_order_tester.ts

+    private static _calculateExpectedBalances(
+        signedOrderLeft: SignedOrder,
+        signedOrderRight: SignedOrder,
+        feeTokenAddress: string,


Can't the feeTokenAddress to retrieved from the SignedOrder?

Negative; the fee token address is not currently included the order struct.

Ah yep, you're right. Isn't it always ZRX address? Why not hard-code it?

Actually yeah that's a good point, we could just pass it into the constructor once rather than to each call. (Updated!)

fabioberger · 2018-05-15T12:21:10Z

packages/contracts/test/utils/match_order_tester.ts

+        expect(expectedBalancesMatchRealBalances).to.be.true();
+        return [newERC20BalancesByOwner, newERC721TokenIdsByOwner];
+    }
+


Nit: Omit newline

fabioberger · 2018-05-15T12:22:54Z

packages/contracts/test/utils/match_order_tester.ts

+        signedOrderLeft: SignedOrder,
+        signedOrderRight: SignedOrder,
+        expectedLeftOrderFillAmoutBeforeMatch: BigNumber,
+        expectedRightOrderFillAmoutBeforeMatch: BigNumber,


Not sure I fully understand this variable name. So it's the expected fill amount but before the match?

Yeah that is a confusing name... I've updated to simply orderFilledAmountLeft and orderFilledAmountRight. These seem more sensible and align with the naming scheme in the smart contracts.

dekz · 2018-05-14T08:47:52Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol

    /// @dev Fills the input order.
    /// @param order Order struct containing order specifications.
-    /// @param takerAssetFillAmount Desired amount of takerAsset to sell.
+    /// @param takerAssetFillAmount Desired amount of takerToken to sell.


This should be takerAsset, maybe from the older PR?

dekz · 2018-05-16T05:33:31Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol

+        Order memory order,
+        address takerAddress,
+        bytes32 orderHash,
+        uint256 takerAssetFilledAmount,


I think this naming confused me a bit, though the above comment does help.
How about orderFilledAmount to differentiate fillResults.takerAssetFilledAmount, that seems to match OrderInfo.

Hmm yeah that is confusing. I've updated the wording here and elsewhere for consistency.

dekz · 2018-05-16T05:39:05Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol

-
-        // Validate the order
+        // Ensure order is valid
+        // An order can only be cancelled if it is Status.FILLABLE;


We don't check balances in the contract so it should still be FILLABLE even if the funds were moved.

For context @hysz in 0x.js we often determine the state based off two or more inputs, state(order,maker,taker). So we do get the scenario where the order is fillable, but the maker/taker make it unfillable due to insufficient funds.

dekz · 2018-05-16T05:48:29Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol

+    /// @return status Status of order. Statuses are defined in the LibStatus.Status struct.
+    /// @return orderHash Keccak-256 EIP712 hash of the order.
+    /// @return takerAssetFilledAmount Amount of order that has been filled.
+    function getOrderInfo(Order memory order)


Yep either this or calculateFillResults would be ideal. If it was calculateFillResults it requires getOrderInfo, but would save also save us duplicating this calculation (and and edge cases) in the forwarding contract.

dekz · 2018-05-16T05:50:52Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol

+        bytes leftSignature,
+        bytes rightSignature
+    )
+        public


I think this can be external

Solidity does not yet support structs as parameters for external functions.
See Solidity Issues #3199 and #1603. I've added a todo in the comments.

ohh mannnnn

dekz · 2018-05-16T07:23:09Z

packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSettlement.sol

+        LibOrder.Order memory leftOrder,
+        LibOrder.Order memory rightOrder,
+        LibFillResults.MatchedFillResults memory matchedFillResults,
+        address takerAddress


Can we refactor this to have a similar parameter order to settleOrder, so takerAddress before fillResults?

dekz · 2018-05-16T07:24:00Z

packages/contracts/src/utils/asset_proxy_utils.ts

    encodeERC20ProxyData(tokenAddress: string): string {
        const encodedAssetProxyId = assetProxyUtils.encodeAssetProxyId(AssetProxyId.ERC20);
        const encodedAddress = assetProxyUtils.encodeAddress(tokenAddress);
        const encodedMetadata = Buffer.concat([encodedAssetProxyId, encodedAddress]);
        const encodedMetadataHex = ethUtil.bufferToHex(encodedMetadata);
        return encodedMetadataHex;
    },
+    decodeERC20ProxyData(proxyData: string): string /* tokenAddress */ {


I would like if this had a type which includes the asset proxy Id. And also include a decodeProxyData which returns id, token address (optional) and data (optional). Forwarding wrapper currently switches on the asset id and has to pull it out.

I've updated decodeERC20ProxyData & decodeERC721ProxyData accordingly. Also see the newly created function decodeProxyData, which uses the generalized form you requested.

dekz · 2018-05-16T07:28:42Z

packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IExchangeCore.sol

+    function getOrderInfo(LibOrder.Order memory order)
+        public
+        view
+        returns (


Is there a reason this doesn't return an OrderInfo struct?

That's a good idea. This struct was originally created as a workaround for the stack limit. But yeah, it totally makes sense to just return it rather than reinterpret the output.

dekz · 2018-05-16T07:29:59Z

packages/contracts/src/utils/exchange_wrapper.ts

@@ -225,6 +225,30 @@ export class ExchangeWrapper {
        const filledAmount = new BigNumber(await this._exchange.filled.callAsync(orderHashHex));
        return filledAmount;
    }
+    public async getOrderInfoAsync(
+        signedOrder: SignedOrder,
+    ): Promise<[number /* orderStatus */, string /* orderHash */, BigNumber /* orderTakerAssetAmountFilled */]> {


Commented above about this contract function returning an OrderInfo struct, if it did it would also be typed in the generated contract wrappers and would bubble up here.

dekz · 2018-05-16T07:31:23Z

packages/contracts/src/utils/exchange_wrapper.ts

+            { from },
+        );
+        const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash);
+        tx.logs = _.filter(tx.logs, log => log.address === this._exchange.address);


this isn't needed as it is performed in _getTxWithDecodedExchangeLogsAsync

abandeali1

Couple small changes, but this looks soooo clean overall.

abandeali1 · 2018-05-17T20:15:16Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol

-        if (cancelled[orderHash]) {
-            emit ExchangeError(uint8(Errors.ORDER_CANCELLED), orderHash);
+        // Compute proportional fill amounts
+        uint8 status;


In 0.4.24, we can do (uint8 status, fillResult) = ...
Might just be worth updating to the newest version so that we don't have to adjust the styling again later.

abandeali1 · 2018-05-17T21:00:20Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol

+            takerAssetFillAmount
+        );
+        if (status != uint8(Status.SUCCESS)) {
+            emit ExchangeStatus(uint8(status), orderInfo.orderHash);
            return fillResults;


Idea I just thought of - in order to be explicit here, maybe it's worth returning a null fillResults struct.

Would need something like this (until Solidity adds support for constant structs):

function getNullFillResults() internal returns (FillResults memory) { // returns zeroed out FillResults instance return FillResults({ ... }); }

abandeali1 · 2018-05-17T21:29:08Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol

+        internal
+        returns (bool stateUpdated)
+    {
+        // Ensure order is fillable (otherwise cancelling does nothing)


Couple reasons why we have this:

Cheaper to not update state for an invalid cancel.

Could see external contracts using the filled/cancelled mappings as on-chain proofs down the line, and having fake cancels could potentially complicate that.

Don't think it's really a big deal either way, but I'm in favor of leaving it in since it is a bit more efficient.

abandeali1 · 2018-05-18T03:55:32Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol

+        );
+
+        // Compute proportional fill amounts
+        uint8 status;


In Solidity 0.4.24, we can write (uint8 status, fillResults) = ...

Might as well upgrade so we don't have to change this later.

Looks like this only works if we are declaring all variables simultaneously in the tuple.

abandeali1 · 2018-05-18T04:13:37Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol

+
+        // Fetch filled amount and validate order availability
+        orderInfo.orderFilledAmount = filled[orderInfo.orderHash];
+        if (orderInfo.orderFilledAmount >= order.takerAssetAmount) {


Is there ever a case where orderInfo.orderFilledAmount > order.takerAssetAmount?

In theory there should not be. But in practice there could be a rounding error amount.

abandeali1 · 2018-05-18T04:32:44Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol

+
+    /// @dev Validates matched fill results. Succeeds or throws.
+    /// @param matchedFillResults Amounts to fill and fees to pay by maker and taker of matched orders.
+    function assertValidMatch(MatchedFillResults memory matchedFillResults)


I'd prefer not to overload this function. How about assertValidMatchOrders and assertValidMatchResults?

abandeali1 · 2018-05-18T04:39:05Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol

+        }
+
+        // Calculate fill results for left order
+        uint8 status;


Combine these lines with Solidity 0.4.24

Looks like this only works if we are declaring all variables simultaneously in the tuple.

abandeali1 · 2018-05-18T04:41:22Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol

+            matchedFillResults.right.takerAssetFilledAmount,
+            matchedFillResults.takerFillAmount
+        );
+        require(


What miscalculation needs to happen in order for this to evaluate to false? Is this something we expect to ever hit, or are we just being extra safe?

If we expect this to sometimes be hit, we should add tests for that scenario.

Just a sanity check. And it's actually covered by the subsequent rounding error test. I'll remove this.

abandeali1 · 2018-05-18T04:42:58Z

packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IMatchOrders.sol

+    function matchOrders(
+        LibOrder.Order memory leftOrder,
+        LibOrder.Order memory rightOrder,
+        bytes leftSignature,


bytes memory again

abandeali1 · 2018-05-18T04:44:38Z

packages/contracts/src/contracts/current/protocol/Exchange/libs/LibOrder.sol

+        // Keccak-256 EIP712 hash of the order
+        bytes32 orderHash;
+        // Amount of order that has been filled
+        uint256 orderFilledAmount;


I know it's verbose, but I prefer orderTakerAssetFilledAmount here.

…ty check to order matching calculations

…bottom

…ws pattern of fillOrder more closely.

…ults. I like this better than just logging an error and failing silently.

…stinguish between fill results and order state

…change inherits Mixins)

…, which should be useful for the forwarding contract code.

…nstant, we dont need to pass it in on each call.

…sed by the Forwarding Contract.

dekz · 2018-05-21T20:26:21Z

packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol

+    function assertValidCancel(
+        Order memory order,
+        uint8 orderStatus,
+        bytes32 orderHash


assertValidFill takes in the taker address, thoughts on passing it via parameters for consistency in assertValidCancel?

That would be cleaner. It would also make the function pure. assertValidFill is also pure, except for calling isValidSignature.

…ppear to be used (it was from an old PR I merged in).

hysz requested a review from abandeali1 May 5, 2018 01:12

hysz requested review from BMillman19, dekz, fabioberger, LogvinovLeon and recmo as code owners May 5, 2018 01:12

abandeali1 suggested changes May 7, 2018

View reviewed changes

hysz force-pushed the feature/contracts/atomicMatching branch from 6d754f8 to 68f6816 Compare May 10, 2018 21:43

hysz changed the title ~~[WIP] Atomic Order Matching~~ Atomic Order Matching May 11, 2018

hysz assigned abandeali1, dekz and fabioberger May 11, 2018

abandeali1 suggested changes May 11, 2018

View reviewed changes

hysz force-pushed the feature/contracts/atomicMatching branch 2 times, most recently from d8e0153 to aca55fb Compare May 12, 2018 21:04

fabioberger suggested changes May 15, 2018

View reviewed changes

dekz suggested changes May 16, 2018

View reviewed changes

fabioberger approved these changes May 17, 2018

View reviewed changes

abandeali1 suggested changes May 18, 2018

View reviewed changes

hysz force-pushed the feature/contracts/atomicMatching branch from 0aa81e4 to cc16e2c Compare May 18, 2018 18:39

hysz added 10 commits May 18, 2018 17:01

Atomic Order Matching - Smart Contracts.

a4c821e

Atomic Order Matching - Tests

9b1015b

Removed isRoundingError from mixin header

68fa7ae

Updated remaining contracts to v0.4.23

f378406

Added require reasons to MixinMatchOrders and cleaned up some comments.

fa75703

Style changes to atomic order matching

5735095

Reordered fund transfers for matched orders, plus added an extra sani…

1dd7688

…ty check to order matching calculations

Changes based on style guidelines put out by Amir

bb73963

Reordered functions in mixin exchange core -- getOrderInfo is at the …

12d8c23

…bottom

Token -> Asset in fillOrder spec

3e6e7fb

hysz added 19 commits May 18, 2018 17:01

Removed redundant status checks in matchOrders. Saves gas, plus follo…

061facd

…ws pattern of fillOrder more closely.

Throw if the left or right orders do not compute the correct fill res…

9308732

…ults. I like this better than just logging an error and failing silently.

Renamed "validate" functions to "assert" in mixin match.

71483e2

Comments for readability in exchange core and mixin match orders

80114ed

Renamed takerAssetFilledAmount to orderFilledAmount to more easily di…

c79f350

…stinguish between fill results and order state

IExchange inherits from all other interfaces (in the same order as Ex…

ce177ae

…change inherits Mixins)

Updated order of settleMatchedOrders to align with settleOrder

c8f65a1

The OrderInfo struct is now returned by the getOrderInfo function

636dae6

Removed redundant log decode call

28bb112

Style improvements to the match orders typescript test class

e748e18

Cleaned up interface for decoding proxy data. Added a general decoder…

2eb5819

…, which should be useful for the forwarding contract code.

Wording of calculateMatchedFillResults

80285a3

Moved feeTokenAddress to MatchOrderTester constructor. Since it is co…

dbbd32d

…nstant, we dont need to pass it in on each call.

Changed calculateFillResults to public visibility so that it can be u…

bb74789

…sed by the Forwarding Contract.

Rebased against v2-prototype

8c1ae35

Upgraded to Solidity 0.4.24

89abd76

Style improvements to order matching

d13c08c

orderFilledAmount -> orderTakerAssetFilledAmount

b5bcfc8

Added getNullFillResults

f4ebbfa

hysz force-pushed the feature/contracts/atomicMatching branch from 8b4565a to f4ebbfa Compare May 19, 2018 00:08

abandeali1 approved these changes May 19, 2018

View reviewed changes

dekz approved these changes May 21, 2018

View reviewed changes

hysz added 2 commits May 21, 2018 13:54

Ran prettier

d839028

Removed debug line from chai_setup. Its failing linter and does not a…

11b6d29

…ppear to be used (it was from an old PR I merged in).

hysz merged commit d460c0e into v2-prototype May 21, 2018

recmo mentioned this pull request May 28, 2018

[WIP] Match and Batch fill #521

Closed

9 tasks

hysz deleted the feature/contracts/atomicMatching branch March 16, 2019 04:36

Atomic Order Matching #572

Atomic Order Matching #572

Conversation

hysz commented May 5, 2018 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

abandeali1 left a 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

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

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

dekz May 16, 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

abandeali1 left a 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

hysz commented May 5, 2018 •

edited

Loading

dekz May 16, 2018 •

edited

Loading