Cancelling an already cancelled order shouldn't consume all gas

[jorisbontje]

Currently we throw if you try to cancel an order that is already cancelled. This will cause all the 1M gas to be consumed. Instead we should just return instead of throw and make this a NOOP / save lots of gas / trees.

[nmushegian]

I disagree. The habit of using returns when the thing which your function is named didn't happen is too dangerous to be worth it IMO. That's on the EVM designers for not including a free exception op.

p.s. Did you know throw is implemented as an "invalid jump" (which like all exceptions is handled gracefully at the EVM level and then repackaged into confusing semantics at the solidity level)

[nmushegian]

pps see ethereum/EIPs#140

[jorisbontje]

While I appreciate your principled position, the EVM designers aren't paying the gas cost for this. It is our users that will have to pay instead. In this specific case it is not about return -ing when no cancel -lation happened, but when it already happened; we can specifically make sure that the specified order_id is/was a valid one to not confuse this with trying to cancel an order that did never exist.

[rainbreak]

We already use the boolean return for buy, so this would be consistent. We already have the boolean return for cancel, it just isn't exercised (just true or throw).

In general and certainly for bigger contracts I'd agree that throw is the better pattern, as it reverts all state changes.

@jorisbontje: would you also want to return false in the case of offer.owner != msg.sender? Similarly on buy, if the offer is inactive?

[jorisbontje]

@rainbeam yes, preferably; especially during mass order cancellation (when the market is closed) it is likely that cancel will be triggered concurrently.

[nmushegian]

These arguments make sense to me. I have been using tryAction naming scheme where a safe-reverting non-throwing version is useful, what do you think?

[rainbreak]

Yeah, for a bigger codebase. There's only 3 public functions here though so not sure it's worth it. But then maybe it could be seen in the context of the larger ecosystem. Whichever way it's addressed we're still just working around limitations of the current language. We need better tooling for localised state rollback. Nice EIP btw.