New Order Matching Strategy

[jalextowle]

Description

These changes implements the functionality of ZEIP 40 and the remaining functionality from ZEIP 41.

New Order Matching Strategy

The bulk of the changes in this pull request are related to the new order matching strategy that ZEIP 40 introduces.

The new order matching strategy comes with a new function that is used to expose the functionality of the new order matching strategy:

    function matchOrdersWithMaximalFill(
        LibOrder.Order memory leftOrder,
        LibOrder.Order memory rightOrder,
        bytes memory leftSignature,
        bytes memory rightSignature
    )
        public
        nonReentrant
        returns (LibFillResults.MatchedFillResults memory matchedFillResults);

The new order matching strategy allows profit to be paid out of the left maker asset, the right maker asset, or both. There are three cases in the order matching strategy:

• leftTakerAssetAmountRemaining > rightMakerAssetAmountRemaining, which indicates that the right order can be fully filled but the left order cannot. In this case, the profit will be paid out in the left maker asset, and the profit will be calculated as the spread in the left maker asset.
• rightTakerAssetAmountRemaining > rightMakerAssetAmountRemaining, which indicates that the right order can be fully filled but the left order cannot. In this case, the profit will be paid out in the right maker asset, and the profit will be calculated as the spread in the right maker asset.
• leftTakerAssetAmountRemaining > rightMakerAssetAmountRemaining && rightTakerAssetAmountRemaining <= rightMakerAssetAmountRemaining, which indicates that both orders can be fully filled. In this case, the profit will be paid out in both maker assets, and the profit will be calculated as the spread in the left maker asset and the spread in the right maker asset.

Batch Orders with Maximal Fill

A reentrant version of the function is available through the internal function _matchOrders if its argument withMaximalFill is true, which allowed a batchMatchOrdersWithMaximalFill function to be created using almost identical logic to batchMatchOrders.

Split up TestExchangeInternals

We have now reached the point where TestExchangeInternals will not deploy with all of the internal functions defined. To facilitate the process of breaking of the tests, I created a TestExchangeMath contract that only inherits the math functions of the exchange. Over time, the creation of Test contracts that only inherit from one or several mixins may become necessary if deployment costs continue to increase.

Testing instructions

yarn build:contracts && yarn test:contracts

Types of changes

• New feature (non-breaking change which adds functionality)

Checklist:

• Prefix PR title with [WIP] if necessary.
• Add tests to cover changes as needed.
• Update documentation as needed.
• Add new entries to the relevant CHANGELOG.jsons.

[dorothy-zbornak]

Solid work, man!

[hysz]

Very clean code, nice work!

There are a few edge cases I think we should explore. I believe there are ~25 cases for the original matching strategy but only a handful for the new strategy. It might be worthwhile to copy over the original tests and update the expected output.

[abandeali1]

I've only done a partial review so far (haven't reviewed batch matching or tests). I'll get the rest reviewed over the next few days!

[hysz]

We may want to add a note to the definition of BatchMatchedFillResults that there will be no fill result for orders that were not matched. Right now we label the fields as "Fill results for left/right orders", which is sort of ambiguous. A client may expect to receive fill results for all orders they pass in.

[jalextowle]

Amir and I talked about this earlier. I don’t know that we came to a conclusion on it. I’ll talk to him in the morning and see what he thinks. I’m happy to add a comment for now, and depending on what he says I’ll either create an asana task or update the PR.

[hysz]

Sounds good. For this PR, we should at least ensure there is consistent behavior across match scenarios. So we should either (i) return empty fill results for all unmatched orders, or (ii) return no empty fill results for any unmatched orders. I personally think it's okay to only return a fill result for orders that are matched.

Ex of current inconsistent behavior:

-- Scenario 1 --
leftOrders = [order_a, order_b]
rightOrders = [order_c]
* order_a and order_c are fully matched*
leftResult = [a_fill_result, (empty)]
rightResult = [c_fill_result]

-- Scenario 2 --
leftOrders = [order_a]
rightOrders = [order_c, order_d]
* order_a and order_c are fully matched*
leftResult = [a_fill_result]
rightResult = [c_fill_result]

[hysz]

Note:
Chatted with Alex offline. The array initialization was updated earlier in the PR, so this is just an issue with redundant writes. We may want to refactor this section so that we only write the left/right result once, but in terms of correct output this looks good to me 👍 .

[hysz]

Is this necessary if the left order was also filled during this iteration? I believe this would write an empty fill result for the next iteration's left order. If this is the case, then we should ensure there is a test case for this scenario.

[jalextowle]

Good point. I actually don’t know if this is a problem though since in this case the next left order wouldn’t have been filled anyways. It’s redundant, but it doesn’t seem to change any behavior since we process the orders in order and the last order will never be filled in this case. I actually think it may be cleaner to keep it the same, but I’m interested to hear your thoughts.

[hysz]

Please see comment above 🙏

[hysz]

Awesome work !! Very cleanly implemented with very thorough coverage. Let's get this merged 🥂