Forwarder Market sell specified amount or throw

[dekz]

Description

We wish to sell a specific amount or throw. This is then consistent with marketBuyOrdersWithEth and our usage of FillOrKill functions in asset-swapper.

marketSellOrdersWithEth does not throw and is a surprise to many users and inconsistent when selling DAI versus selling ETH via asset-swapper/0xApi. Generally we view fills as having funds for the takerAssetAmount and an additional source of funds for the protocolFee. Asset-swapper is no longer guaranteeing that the orders are always sorted by adjusted price (due to redundant path), so this opens up a potential issue where we partially fill some "ok" orders.

One thing to note is how ethSellAmount interacts with protocol fees. We wish to sell 50 ETH for example and not 49.9 ETH and 0.1 ETH of protocol fee. In normal conditions these are distinct (WETH and ETH in value, WETH and WETH (allowance)). This function also treats them as seperate pools, (ethSellAmount ETH and protocol fee ETH also in value). To sell 50 ETH using this function the user will need to specify ethSellAmount=50 and send value=50.1 to sell and pay protocol fee.

In the event of a successful marketSell which encountered unfillable orders, the additional protocolFees are returned, rather than sold in following orders.

TODO

• clean up as there is a bit of duplicate functionality
• use the struct everywhere for consistency?

[dorothy-zbornak]

I think we also want to safeSub(totalProtocolFeePaid)?

[dekz]

Perhaps the naming is off right now or I am missing something, but wethSellAmount in the parameter is exclusive of protocol fees. So we don't want to subtract the protocolFee as it is not included in wethSellAmount. I.e A user calls marketSellAmountWithEth with 2 ETH and provides 2.1 ETH in value. We then proceed to sell 2ETH, not 2.1 ETH. We're not using msg.value.

[dorothy-zbornak]

Ahh, OK. I forgot wethSellAmount is minus protocol fees already.

[dorothy-zbornak]

What if we also did something like this here:

if (remainingTakerAssetFillAmount > singleProtocolFee) {
    // Do not count the protocol fee as part of the fill amount.
    remainingTakerAssetFillAmount = remainingTakerAssetFillAmount.safeSub(singleProtocolFee);
} else {
    // Stop if we don't have at least enough ETH to pay another protocol fee.
    break;
}

[dorothy-zbornak]

My hunch is line 206 (.safeSub(_isV2Order(orders[i]) ? 0 : protocolFee);) in _marketSellNoThrow() is probably where I was seeing those rigo + USDC + high gas reverts, so adding something similar to that function could also fix that.

[dekz]

I can make this change on the other sell variant. See other comment.

[coveralls]

Coverage increased (+0.05%) to 79.739% when pulling 77bdb90 on feat/forwarder/market-sell-amount into bf00e67 on development.

[dorothy-zbornak]

👍 👍 👍

[dorothy-zbornak]

Ahh, OK. I forgot wethSellAmount is minus protocol fees already.

[dorothy-zbornak]

Now I'm wondering if it would be more useful to know how much we actually sold minus the protocol fee. What do you think?

[moodlezoup]

looks good to me!