Add marketBuy/SellOrdersFillOrKill() to Exchange #2075

dorothy-zbornak · 2019-08-16T22:08:32Z

Description

This PR introduces two new functions to the Exchange's MixinWrapperFunctions, which is the subject of ZEIP-50:

marketSellOrdersFillOrKill()
- After calling marketSellOrdersNoThrow(), ensures that takerAssetFilledAmount > takerAssetFillAmount.
marketBuyOrdersFillOrKill()
- After calling marketBuyOrdersNoThrow(), ensures that makerAssetFilledAmount > makerAssetFillAmount.

Other Changes

Re-introducing the `noThrow()` suffix

In PR #2042, the marketBuyOrders() and marketSellOrders() functions were replaced by marketBuyOrdersNoThrow() and marketSellOrdersNoThrow(), assuming the formers' names.

This was probably a bad idea because the function signatures were identical, meaning it was very possible that a dapp would call those functions expecting the old behavior.

So I've added the noThrow() suffixes back to those functions in this PR.

`RevertError` types can now take array fields

The new marketBuy/SellOrdersFillOrKill() functions needed to throw a rich revert that holds the order hashes of all the orders passed. So now this is a thing.

Added a `fillAmount` field to existing `IncompleteFillError` rich revert

Made sense.

Use `abi.decode()` in `LibTransactionDecoder`

LibTransactionDecoder relied heavily on LibBytes for decoding individual error parameters. This worked fine for simple types, but LibBytes doesn't support reading a bytes32[] (which I needed), so I figured we might as well just be consistent just use abi.decode() throughout that contract.

Added an `UNLIMITED_CONTRACT_SIZE` environment variable, `@0x/contracts-dev-utils` tests now run with it.

This environment variable is picked up by @0x/contracts-test-utils and creates the default web3Wrapper instance with allowUnlimitedContractSize enabled if set to true.

With the addition of the new methods, the DevUtils contract is no longer deployable under normal conditions because it is > 24KB. The UNLIMITED_CONTRACT_SIZE is a temporary fix for our tests, but we need to re-architecture this contract to be smaller/deployable inevitably.

Testing instructions

You know the drill, so DRILL IT.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)

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.

coveralls · 2019-08-17T00:09:45Z

Coverage remained the same at 75.804% when pulling e9a4b07 on feature/3.0/exchange/market-fill-or-kill into 0fad6a6 on 3.0.

jalextowle

A few nits, but looks great. Great job on getting solid test coverage! I'll approve once the unlimited contract size issue gets addressed, and once you've had a change to respond to the nits.

jalextowle · 2019-08-19T21:58:37Z

packages/utils/test/revert_error_test.ts

+            const strings = ['foo', 'bar'];
+            const revert1 = new ArrayRevertError(strings);
+            const revert2 = new ArrayRevertError(strings);
+            expect(revert1.equals(revert2)).to.be.true();


Style question: Is there a reason to use expect(revert1.equals(revert2)).to.be.false() instead of expect(revert1).to.be.deep.eq(revert2)?

Yeah, there's a semantic difference, but it doesn't really matter in this case. equals() will ignore missing fields.

Good to know 👍

jalextowle · 2019-08-19T21:59:10Z

packages/utils/src/revert_error.ts

@@ -9,7 +9,7 @@ import { BigNumber } from './configured_bignumber';

 // tslint:disable: max-classes-per-file

-type ArgTypes = string | BigNumber | number | boolean;
+type ArgTypes = string | BigNumber | number | boolean | BigNumber[] | string[] | number[] | boolean[];


jalextowle · 2019-08-19T22:05:50Z

packages/utils/src/revert_error.ts

@@ -388,14 +388,16 @@ function declarationToAbi(declaration: string): RevertErrorAbi {
    const [name, args] = m.slice(1);
    const argList: string[] = _.filter(args.split(','));
    const argData: DataItem[] = _.map(argList, (a: string) => {
-        m = /^\s*([_a-z][a-z0-9_]*)\s+([_a-z][a-z0-9_]*)\s*$/i.exec(a);
+        m = /^\s*(([_a-z][a-z0-9_]*)(\[\d*\])*)\s+([_a-z][a-z0-9_]*)\s*$/i.exec(a);


Nit: Could you add a comment to this regex? It's getting a bit long, and it is not immediately obvious that this regex just matches an identifier, possibly ending with an array bracket, separated from another identifier by whitespace?

jalextowle · 2019-08-19T22:08:53Z

packages/utils/src/revert_error.ts

        return normalizeBytes(lhs as string) === normalizeBytes(rhs as string);
    } else if (type === 'string') {
        return lhs === rhs;
+    } else if (/\[\d*\]$/.test(type)) {


Very clean! Might be good to add a comment on the larger regexs, but they aren't too bad yet.

abandeali1

I have few comments on the Solidity that I think would simplify things. Will review the tests tomorrow!

abandeali1 · 2019-08-19T17:44:56Z

contracts/exchange/contracts/src/MixinWrapperFunctions.sol

+        uint256 ordersLength = orders.length;
+        orderHashes = new bytes32[](ordersLength);
+        for (uint256 i = 0; i != ordersLength; i++) {
+            orderHashes[i] = orders[i].getTypedDataHash(EIP712_EXCHANGE_DOMAIN_HASH);


We can set EIP712_EXCHANGE_DOMAIN_HASH to a stack variable to save a bunch of sloads.

I'd be in favor of just removing this completely though. The order hashes here aren't really relevant to why the fill failed, and they can all be easily calculated since we have all of the orders already.

abandeali1 · 2019-08-19T17:46:58Z

contracts/exchange/contracts/src/MixinWrapperFunctions.sol

@@ -308,9 +360,25 @@ contract MixinWrapperFunctions is
        );
        if (fillResults.takerAssetFilledAmount != takerAssetFillAmount) {
            LibRichErrors.rrevert(LibExchangeRichErrors.IncompleteFillError(
+                takerAssetFillAmount,


It feels like the filled amount is probably more useful than the fill amount, since we already know the fill amount when the function is called.

abandeali1 · 2019-08-20T00:54:15Z

contracts/exchange-libs/contracts/src/LibExchangeRichErrors.sol

@@ -584,7 +609,38 @@ library LibExchangeRichErrors {
    {
        return abi.encodeWithSelector(
            INCOMPLETE_FILL_ERROR_SELECTOR,
+            takerAssetFillAmount,


The takerAssetFilledAmount feels more useful than the takerAssetFillAmount that gets passed into fillOrKillOrder, since we already know what that is when we make the function call.

That being said, I kind of like passing in both amounts (expectedFillAmount and actualFillAmount).

abandeali1 · 2019-08-20T00:56:23Z

contracts/exchange-libs/contracts/src/LibExchangeRichErrors.sol

+        return abi.encodeWithSelector(
+            INCOMPLETE_MARKET_SELL_ERROR_SELECTOR,
+            takerAssetFillAmount,
+            orderHashes


Are these orderHashes really that useful? If we add an errorCode param to this, we can consolidate all 3 fillOrKill errors into a single rich revert reason. Something like:

function IncompleteFillError( FillOrKillErrorCode errorCode, uint256 actualFillAmount, uint256 expectedFillAmount )

abandeali1 · 2019-08-20T01:01:46Z

contracts/exchange/contracts/src/MixinWrapperFunctions.sol

            // Attempt to sell the remaining amount of takerAsset
            LibFillResults.FillResults memory singleFillResults = fillOrderNoThrow(
                orders[i],
                remainingTakerAssetFillAmount,
                signatures[i]
            );

+            // Restore the original `makerAssetData` so we're non-destructive.
+            orders[i].makerAssetData = originalMakerAssetData;


Hmm, this change doesn't feel super necessary to me since this isn't intended to be a library or used in external contracts. We also do something similar in matchOrders, right? Is there anywhere where we actually reuse the in-memory orders after calling these functions?

It was to preserve the order hashes in the event of a rich revert, but we won't need it anymore if we aren't returning order hashes.

jalextowle

LGTM

abandeali1

Nice work on this! Love the new testing format.

hysz

Looks great! Just one minor change now that we've merged development into 3.0.

hysz · 2019-08-21T03:02:00Z

contracts/exchange/test/utils/exchange_wrapper.ts

+        from: string,
+        opts: { takerAssetFillAmount: BigNumber; gas?: number },
+    ): Promise<TransactionReceiptWithDecodedLogs> {
+        const txHash = await this._exchange.marketSellOrdersFillOrKill.sendTransactionAsync(


If we use awaitTransactionSuccessAsync here then we don't need the log decoder :)

hysz · 2019-08-21T03:02:12Z

contracts/exchange/test/utils/exchange_wrapper.ts

+        from: string,
+        opts: { makerAssetFillAmount: BigNumber; gas?: number },
+    ): Promise<TransactionReceiptWithDecodedLogs> {
+        const txHash = await this._exchange.marketBuyOrdersFillOrKill.sendTransactionAsync(


…lError` type. `@0x/order-utils`: Add `IncompleteMarketSellError` and `IncompleteMarketBuyError` `RevertError` types.

…de()` instead of `LibBytes`.

…now return sane values.

…ers` back to `marketSellOrdersNoThrow` and `marketBuyOrdersNoThrow`. `@0x/contracts-exchange`: Introduce new `marketSellOrdersFillOrKill` and `marketBuyOrdersFillOrKill` functions. `@0x/contracts-exchange`: Add new rich error types: `IncompleteMarketBuyError` and `IncompleteMarketSellError`. `@0x/contracts-exchange`: Use `abi.decode()` in `LibExchangeRichErrorDecoder` over `LibBytes`.

…g a ganache provider.

…ketBuy/SellOrdersFillOrKill` to `LibTransactionDecoder`.

…rorDecoder`.

… work with `marketBuy/SellFillOrKill()` functions.

…OT be destructive to the orders. `@0x/contracts-exchange`: Fix wrapper unit tests to use the actual order hash algorithm, since it can't be overridden anymore.

…onfigurable via `Web3Config`. `@0x/dev-utils`: Add `UnlimitedContractSize` to `EnvVars`.

…wUnlimitedContractSize` if `UNLIMITED_CONTRACT_SIZE` environment variable is set.

…MITED_CONTRACT_SIZE=true` environment variable.

…llError`.

…IncompleteFillError`.

…` rich error. `@0x/contracts-exchange`: Allow `marketSell/BuyOrdersNoThrow` to be destructive to orders again.

…r.ts`.

…from `MixinWrapperFUnctions`.

…st class in favor of `awaitTransactionSuccessAsync`.

dorothy-zbornak requested review from hysz, abandeali1 and jalextowle August 16, 2019 22:08

dorothy-zbornak requested a review from fabioberger as a code owner August 16, 2019 22:08

jalextowle reviewed Aug 19, 2019

View reviewed changes

abandeali1 suggested changes Aug 20, 2019

View reviewed changes

dorothy-zbornak force-pushed the feature/3.0/exchange/market-fill-or-kill branch from 4e7289f to 2e51fac Compare August 20, 2019 17:07

jalextowle approved these changes Aug 20, 2019

View reviewed changes

abandeali1 approved these changes Aug 20, 2019

View reviewed changes

hysz approved these changes Aug 21, 2019

View reviewed changes

dorothy-zbornak added 19 commits August 21, 2019 10:59

@0x/utils: Allow for array types in RevertError types.

a3f6160

@0x/order-utils: Add takerAssetFillAmount field to `IncompleteFil…

fb6e8a4

…lError` type. `@0x/order-utils`: Add `IncompleteMarketSellError` and `IncompleteMarketBuyError` `RevertError` types.

@0x/contracts-exchange-libs: Add new revert types and use `abi.deco…

d8372f7

…de()` instead of `LibBytes`.

@0x/contracts-exchange: Overridden functions in ReentrancyTester …

1698519

…now return sane values.

@0x/contracts-exchange`: Update changelog.

fd2a240

@0x/dev-utils: Set allowUnlimitedContractSize option when creatin…

740913f

…g a ganache provider.

@0x/contracts-dev-utils: Add marketBuy/SellOrdersNoThrow and `mar…

89f1d54

…ketBuy/SellOrdersFillOrKill` to `LibTransactionDecoder`.

@0x/contracts-exchange: Squelch linter errors in `LibExchangeRichEr…

a261362

…rorDecoder`.

Ran prettier

fb38867

@0x/contracts-exchange: Tweak exchangeDataEncoder test utility to…

748566b

… work with `marketBuy/SellFillOrKill()` functions.

Update CHANGELOGs with PR numbers.

8c06d66

@0x/contracts-exchange: Update marketBuy/SellOrdersNoThrow() to N…

34d075c

…OT be destructive to the orders. `@0x/contracts-exchange`: Fix wrapper unit tests to use the actual order hash algorithm, since it can't be overridden anymore.

ran prettier

0c7f09b

@0x/contracts-exchange: Fix failing wrapper.ts test.

edef3bc

@0x/dev-utils: Make ganache's allowUnlimitedCOntractSize option c…

0c53e2f

…onfigurable via `Web3Config`. `@0x/dev-utils`: Add `UnlimitedContractSize` to `EnvVars`.

@0x/contracts-test-utils: web3Wrapper is created with `shouldAllo…

6b8bc55

…wUnlimitedContractSize` if `UNLIMITED_CONTRACT_SIZE` environment variable is set.

@0x/contracts-dev-utils: run_mocha package script runs with `UNLI…

43f38d0

…MITED_CONTRACT_SIZE=true` environment variable.

@0x/order-utils: Consolidate FillOrKill errors into `IncompleteFi…

c659477

…llError`.

dorothy-zbornak added 9 commits August 21, 2019 10:59

@0x/contracts-exchange-libs: Consolidate FillOrKill errors into `…

75a4d12

…IncompleteFillError`.

@0x/contracts-exchange: Switch to consolidated `IncompleteFillError…

e4475c0

…` rich error. `@0x/contracts-exchange`: Allow `marketSell/BuyOrdersNoThrow` to be destructive to orders again.

@0x/order-utils: Update CHANGELOG and run prettier.

c3bff31

@0x/contracts-exchange-libs: Update CHANGELOG.

34f6fac

@0x/contracts-exchange: Update CHANGELOG and run prettier.

a497ddf

@0x/order-utils: Add comments to regex parsing code in `revert_erro…

1463046

…r.ts`.

@0x/utils: Ran prettier.

c344625

@0x/contracts-exchange: Remove unused _getOrderHashes() function …

abf076f

…from `MixinWrapperFUnctions`.

@0x/contracts-exchange: Remove log decoding in ExchangeWrapper te…

e9a4b07

…st class in favor of `awaitTransactionSuccessAsync`.

dorothy-zbornak force-pushed the feature/3.0/exchange/market-fill-or-kill branch from 65a9821 to e9a4b07 Compare August 21, 2019 14:59

dorothy-zbornak merged commit 7407890 into 3.0 Aug 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add marketBuy/SellOrdersFillOrKill() to Exchange #2075

Add marketBuy/SellOrdersFillOrKill() to Exchange #2075

dorothy-zbornak commented Aug 16, 2019 •

edited

Loading

coveralls commented Aug 17, 2019 •

edited

Loading

jalextowle left a comment •

edited

Loading

jalextowle Aug 19, 2019

dorothy-zbornak Aug 20, 2019

jalextowle Aug 20, 2019

jalextowle Aug 19, 2019

jalextowle Aug 19, 2019

jalextowle Aug 19, 2019

abandeali1 left a comment

abandeali1 Aug 19, 2019

abandeali1 Aug 20, 2019

abandeali1 Aug 19, 2019

abandeali1 Aug 20, 2019

abandeali1 Aug 20, 2019

abandeali1 Aug 20, 2019

dorothy-zbornak Aug 20, 2019

jalextowle left a comment

abandeali1 left a comment

hysz left a comment

hysz Aug 21, 2019

hysz Aug 21, 2019

Add marketBuy/SellOrdersFillOrKill() to Exchange #2075

Add marketBuy/SellOrdersFillOrKill() to Exchange #2075

Conversation

dorothy-zbornak commented Aug 16, 2019 • edited Loading

Description

Other Changes

Re-introducing the noThrow() suffix

RevertError types can now take array fields

Added a fillAmount field to existing IncompleteFillError rich revert

Use abi.decode() in LibTransactionDecoder

Added an UNLIMITED_CONTRACT_SIZE environment variable, @0x/contracts-dev-utils tests now run with it.

Testing instructions

Types of changes

Checklist:

coveralls commented Aug 17, 2019 • edited Loading

jalextowle left a comment • 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

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

jalextowle left a comment

Choose a reason for hiding this comment

abandeali1 left a comment

Choose a reason for hiding this comment

hysz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorothy-zbornak commented Aug 16, 2019 •

edited

Loading

Re-introducing the `noThrow()` suffix

`RevertError` types can now take array fields

Added a `fillAmount` field to existing `IncompleteFillError` rich revert

Use `abi.decode()` in `LibTransactionDecoder`

Added an `UNLIMITED_CONTRACT_SIZE` environment variable, `@0x/contracts-dev-utils` tests now run with it.

coveralls commented Aug 17, 2019 •

edited

Loading

jalextowle left a comment •

edited

Loading