Arbitrary Fee Tokens

[dorothy-zbornak]

Goodbye, ZRX fees!

Who knew so much of our code relied on ZRX fees being baked in?

This PR implements ZEIP-28, which does away with ZRX fees and instead allows for arbitrary maker and taker fee tokens.

The Highlights

• The Order type now has two new fields: makerFeeAssetData and takerFeeAssetData.
• Works as expected with both fillOrder() and matchOrders().
• Because we swap assets before paying maker/taker fees, both the maker and taker can actually cover their fees with the tokens they just bought in the same transaction!
• This even works with ERC721, ERC1155 (fungible and non fungible), and MultiAsset fee tokens. Madness!

Bonus Material

• The order of transfers in fillOrder() has been change to taker -> maker first, instead of maker -> taker first. This is necessary for something like ZEIP-38.
• ERC1155 and MultiAsset assets have been well incorporated into the fillOrder() and matchOrders() tests.
• Enhanced base-contract and abi-gen-templates so that any Errors thrown by a callAsync() and sendTransactionAsync() (ganache only) will be automatically decoded into a RevertError, if possible. This means you no longer just see an opaque VM exception: revert when contract calls unexpectedly explode.
• Many (but not all) package tools used in testing the V3 contracts have also been updated as collateral damage.
• A couple tests and testing tools have been completely or nearly rewritten (such as fill_order_combinatorial_utils and match_order_tester) because they weren't robust enough to handle all the edge cases for arbitrary fees, as well as the upcoming changes to matchOrders() fee splitting.
• Added a package script to exchange-libs to automatically generate LibExchangeSelectors.sol because updating that by hand is a bunch of nopes. Just run yarn generate-exchange-selectors from the package directory.
• Web3Wrapper.toBaseUnitAmount() can now accept a number in addition to a BigNumber, which makes a couple tests a lot more compact.

Issues/Caveats

• Our Fill event grew by two new parameters, which was just enough to break the stack for emit, so I had to resort to an ugly, low-level solution.
• extensions and exchange-forwarder packages have been disabled (hence why code coverage plummeted) from the pipeline. Those packages will probably need major rework beyond the scope of this PR.
• I decoupled the fillOrder() combinatorial tests from OrderStateUtils and OrderValidationUtils. The states that were being tested didn't really warrant them because the combinatorial tests do not generate consecutive fills. We were actually missing out on testing some reverting states because OrderValidationUtils was preventing us from using fill amounts that would fail.

Testing Instructions

You can find the most relevant tests in:

• fill_order.ts
• match_orders.ts

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]

Coverage decreased (-4.7%) to 79.775% when pulling f4bcb67 on feature/contracts/3.0/arbitrary-fees into 10d3539 on 3.0.

[hysz]

Lot's of work, nicely done! Left a few minor comments.

[hysz]

Is there anything special about these values or were they just picked at random?

[dorothy-zbornak]

The values were taken from the existing ERC20 tests. I was wondering this too.

[dorothy-zbornak]

Round 2!

For convenience, here is the diff against the original PR.

Summary

• sendTransactionAsync() and awaitTransactionSuccessAsync() methods on contracts now also decode and throw rich reverts. This only works on ganache, though.
• The script to generate LibExchangeSelectors.sol has been converted to typescript. It also now includes event selectors.
• EIP712_ORDER_SCHEMA_HASH in LibOrder is now public.
• Added ERC1155 and MultiAsset proxy support to the @0x/order-utils ExchangeTransferSimulator class.
• Added setProxyAllowanceForAllAsync() to ERC1155ProxyWrapper for testing purposes.
• Added mintKnownFungibleTokensAsync(), isNonFungibleItemAsync(), isFungibleItemAsync(), getOwnerOfAsync(), getBalanceAsync() to the test Erc1155Wrapper.
• Added MultiAssetProxy, ERC1155Fungible, and ERC1155NonFungible assets to the fillOrder() tests (*).
• Added MultiAssetProxy and ERC1155 assets to the matchOrders() tests.
• fillOrder() and matchOrders() will avoid making a redundant transfer if maker/feeRecipient and takerAsset/makerFeeAsset are the same.
• The order of transfers in fillOrder() has been change to taker -> maker first, instead of maker -> taker first. This is necessary for something like ZEIP-38.

* fillOrder() combinatorial tests are disabled by default.

Adding new asset fields and asset types exponentially increased the number of combinatorial tests in exchange/test/fill_order.ts. They take around 20 minutes to run locally (:sob:), so they are skipped by default (and on CI).

You can enable them by setting the environment variable TEST_ALL=1. So doing TEST_ALL=1 yarn test will run the full suite, which you should do if you are working on anything that is a component of the Exchange contracts.

[albrow]

Adding new asset fields and asset types exponentially increased the number of combinatorial tests in exchange/test/fill_order.ts.

I've been toying with the idea of using an embedded EVM implementation (i.e. one written in TypeScript/JavaScript) for combinatorial tests to drastically improve performance. @xianny I know you've been working with ethereumjs-vm. Do you think this idea is feasible?

[hysz]

Do we have a sense of the gas overhead created by manually constructing the event payload?

[dorothy-zbornak]

I will check, but we don't really have an alternative aside from pure assembly.

[dorothy-zbornak]

It costs 4.8% more gas to emit the hard way. Let's add it to the backlog if we want to further optimize it out.

[abandeali1]

I think the alternative would be cutting fields from the event... there aren't any obvious choices though.

[dorothy-zbornak]

@hysz Figured out that if you rearranged the Fill event args, you can fit an emit on the stack!

[hysz]

If I'm reading this line correctly, the Right Maker is paying their fee to the Left Fee Recipient. But it should go to the Right Fee Recipient.

This is the intended logic:

left.makerFeePaid → paid by Left Maker to Left Fee Recipient
left.takerFeePaid → paid by Taker to to Left Fee Recipient

right.makerFeePaid → paid by Right Maker to Right Fee Recipient
right.takerFeePaid → paid by Taker to Right Fee Recipient

[dorothy-zbornak]

You're right, that is exactly what it's doing. I admit I have to work this out in my head every time I see it, but I think this behavior is correct.

We are basically collapsing the two transfers in the following else clause because the the leftMaker == leftFeeRecipient and the leftMakerFeeAssetData == leftTakerAssetData. So rightMaker can just pay the combined leftTakerAsset and leftMakerFee to leftMaker/leftFeeRecipient since it's all the same asset going to the same place.

[dorothy-zbornak]

Spoke with @hysz and he pointed out there were some other fundamental errors in this settlement optimization (for one, takers were actually paying more than their fair share). This affected both matchOrders() and fillOrders(). The combinatorial tests also weren't covering these scenarios so they slipped under the radar. Totes my bad!

I just now updated things so the only optimizations we do now are avoiding maker fee transfers if maker == feeRecipient and taker fee transfers if taker == feeRecipient. There are now also explicit tests to cover all these (and the previous) scenarios.

[dorothy-zbornak]

Combinatorial tests now take even longer though. 😫
I am definitely adding these to demo sprint (woops, already did).

[hysz]

Looks solid, let's merge!