Add order deletion

[fedgiac]

Adds a function for users to get back their ETH if the order wasn't executed.

For now we send ETH back to the user using transfers. This avoids reentrancy but adds a risk of users losing funds as they might not be able to cancel an order anymore.

Note that the conversion of ETH to WETH and vice-versa will be handled in a follow-up PR.

Another unfortunate observation is that Foundry doesn't support mocking a revert from a contract.

Test plan

New unit tests.

[josojo]

Super nice!

[josojo]

Why don't you set it to NO_OWNER?

Is it because the order is still existing in the settlement contract and hence a new order placement would fail? Probably deleting the old order is too expensive?

[fedgiac]

The worse that could happen is that the order was partially filled, the user cancels it, and then another completely different user creates an order with the same parameters. Then they would "inherit" the state of filledAmount, which would mean that (part of) their ETH is lost forever. (The same would happen for a fill-or-kill order that was cancelled, which is currently possible and returns 0 ETH.)
It's not possible to just clear the state of filledAmount, so I don't see a way around this. The most similar function is invalidateOrder in the settlement contract, but this just sets filledAmount to maxUint256.

[bh2smith]

All looks good, but I noticed several conditions that could cause order deletion to fail and only one of those conditions appears to be tested... is there a plan to test the other branches of this condition. Like owner is no owner or invalid owner.

[fedgiac]

The revert conditions that are tested are:

• not an owner and not expired (testCannotDeleteValidOrdersIfNotOwner)
• deleting order with no owner (testOrderDeletionRevertsIfDeletingUninitializedOrder)
• deleting invalidated order (testOrderDeletionRevertsIfDeletingOrderTwice)
• sending eth fails (testOrderDeletionRevertsIfSendingEthFails)

These cases are the negation of each proposition that is currently joined by || that leads to a revert statement. Something that could be missing is testing all subpropositions separately, so for example all of:

• no owner + not expired + not owner
• no owner + expired + not owner
• no owner + not expired + owner
• no owner + expired + owner
• invalidated + not expired + not owner
• invalidated + expired + not owner
• invalidated + not expired + owner
• invalidated + expired + owner

This is useful to test that the logic of the revert condition was written correctly, on the other hand it seems that it might be too much and could clutter the tests.

[nlordell]

LGTM.