Exchange Protocol Fees

[jalextowle]

Description

This PR adds protocol fees to the Exchange contract, implementing part of ZEIP-42. Several things had to happen for this to occur.

Refundable

A new contract was added to @0x:contracts-utils called Refundable. Refundable is a contract with no dependencies that introduces two new modifiers and a state variable called shouldNotRefund. Both modifiers are designed to respect the refundFinalBalance state variable in that they will only refund if shouldNotRefund is false.

• disableRefundUntilEnd: This modifier will set shouldNotRefund to true, call the function that it modifies, and then refund all of the ether in the contract to the sender. This will disable all nested functions that use disableRefundUntilEnd and refundFinalBalance, and this modifier will itself be disabled if shouldNotRefund is true.
• refundFinalBalance: This modifier will call the function that it is modifying and then refund all of the ether in the contract to the sender. This will be disabled if shouldNotRefund is true.

Payable Functions

Almost every function in the exchange had to be made payable. Some notable exceptions include

• cancelOrder
• batchCancelOrders
• cancelOrdersUpTo

Since these functions are now payable, they all were also modified by refundFinalBalance or disableRefundUntilEnd. The functions that were given refundFinalBalance have no risk of calling a function that also uses refundFinalBalance and experiencing strange behavior. Similarly, functions whose behavior would be affected by refunds in nested calls are modified by disableRefundUntilEnd.

Protocol Fees

Protocol fees were added to the internal functions _fillOrder and _matchOrders, so every function that calls these functions or has the potential to call them will potentially pay protocol fees. fillOrder protocol fees are half of the price of matchOrders protocol fees, and these fees will be paid on a per-fill basis. For example, a call to batchFillOrders with k orders that are filled will pay k * protocolFeePerFill, where protocolFeePerFill = exchange.protocolFeeMultiplier * tx.gasprice.

New state that was added to the exchange (like protocolFeeMultiplier from the above example), including:

• protocolFeeMultiplier: This is the number that will be multiplied to tx.gasprice to calculate the protocol fee of a single fill.
• protocolFeeCollector: This is the contract that will receive protocolFee payments.

Both of these state variables are protected by the onlyOwner modifier, and will thus not be mutable by normal addresses.

Glorious Solidity Tests

One of my favorite parts of writing this PR was discovering that writing tests in Solidity is actually surprisingly nice. The tests that I wrote are in the form:

function testSomething(SomethingContract testContract, ...)
    external
{
    testContract.something(...); // Call something with args

    verifySomething(...); // Make assertions about the call to something
}

The assertions that are made are generally made by leveraging contract inheritance's ability to override important internal functions. These overridden functions are made to change state in ways that indicate internal behavior.

Both Protocol Fees and Refundable received good Solidity test treatment that runs quickly and makes strong assertions about their behavior.

Testing instructions

yarn build:contracts && yarn test:contracts && yarn lint:contracts (or ycall for those with my .bash_profile sourced :))

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.

[dorothy-zbornak]

Still 🤔 about stuff.

[dorothy-zbornak]

🙏

[dorothy-zbornak]

Was this the discrepancy?

[jalextowle]

Yep! I guess when you chain something like:

uint256 someValue = someValue1
    .safeAdd(someValue2)
    .safeSub(someValue3.safeAdd(someValue4))

the someValue3.safeAdd(someValue4) expression is the first thing to get executed. I guess this isn't a huge surprise since this does follow PEMDAS, but it's still pretty weird. We had a case that was somewhat similar to this, which is why the safeSub needs be evaluated first.

[dorothy-zbornak]

I wonder if it would be better to just pass tx.gasprice into this function to keep it pure if we want to go down the route of converting reference functions to ethjsvm. It also creates parity with the reference functions as is.

[jalextowle]

It ended up being more efficient to just entirely exclude the calculation from this function, since it could end up being inaccurate if a protocolFeeCollector is unregistered. I'd be interested to hear your thoughts on this.

[dorothy-zbornak]

Still true?

[jalextowle]

Nope, I took your suggestion when I changed it back, as a bonus.

[dorothy-zbornak]

Same as in calculateFillResults (ethjsvm).

[abandeali1]

Nit: we can save some gas by setting address(this).balance to a stack variable and then checking balance - valuePaid >= protocolFee here. The ADDRESS opcode costs 200 gas if I recall.

[jalextowle]

Turns out that ADDRESS opcode only costs 2 gas as of now. With this in mind, the use of ADDRESS will actually be cheaper here. It's specified as Gbase in the Ethereum Yellow Paper and in the Jello Paper (and Gbase = 2 gas in both). Is this going to change soon?

[dorothy-zbornak]

Reordering worked?

[jalextowle]

Yep! Amir figured it out. The heuristic is that you use all of the fields from order together and do the same with fillResults. It also seems that maintaining the order of parameters in the event reduces stack usage.

[dorothy-zbornak]

Still true?

[dorothy-zbornak]

Really awesome stuff here, and obviously a lot of work. Just a few nits and concerns.

[coveralls]

Coverage remained the same at 75.804% when pulling 2c1393f on feature/3.0/exchange/protocol-fees into a9857fa on 3.0.

[dorothy-zbornak]

🔥

[dorothy-zbornak]

👍 👍 👍