Add featureImmediateOfferKilled for tfImmediateOrCancel offers:

[scottschurr]

High Level Overview of Change

Fixes #4115

Context of Change

@mDuo13 noted:

It's confusing and unintuitive to see an "immediate or cancel" transaction treated as "successful" when it did nothing because the "or cancel" part applied. A different result code makes it apparent at a glance what happened.

The approach taken to this fix reuses the tecKILLED code. I hesitated on this because the descriptive text associated with tecKILLED needs to change when applied in this new context. And that description change could not easily be protected by the amendment. However @mDuo13 pointed out:

... no amendment is necessary for the description change. Per https://xrpl.org/transaction-results.html#immediate-response

So this gave us the leash to make make the change minimally invasive. Even though, admittedly, it requires an amendment.

For me, the strongest motivation for this change is as follows (also from @mDuo13):

A major benefit of this change is that you could write code to assume that, if the code is tesSUCCESS (and the transaction is post-amendment) then there will be balance changes of some kind in the metadata, and if the code is tecKILLED then you don't have to look at the metadata to know that no balance change occurred (other than burning the tx cost of course).

Type of Change

• Amendment (Functionality waits for amendment to pass. Once passed may cause amendment blocking.)
• Breaking change (fix or feature that would cause existing functionality to not work as expected).
• Tests (You added tests for code that already exists, or your new feature included in this PR).

Before / After

Before: an OfferCreate transaction with the tfImmediateOrCancel flag set that causes no funds to move returns tesSUCCESS.

After: an OfferCreate transaction with the tfImmediateOrCancel flag set that causes no funds to move returns tecKILLED.

[mDuo13]

26 lines, including tests? Now that's impressive work. I know that keeping it simple is harder than it looks, and usually involves unseen work that's discarded on the way. So, thanks for the elegant solution.

[seelabs]

Nice job on this. LGTM 👍

[nbougalis]

Veto. @scottschurr: this breaks the "Immediate or Cancel" semantics. See comment below.

[nbougalis]

This is broken: it basically changes the semantics of Immediate or Cancel to be those of Fill or Kill.

A "Fill or Kill" order means: "if the offer can't be entirely filled by crossing against existing offers, abort. Never place an order on the order book."

An "Immediate or Cancel" order, on the other hand, means: "fill as much as you can by crossing against existing offers. If the offer isn't fully filled, that's fine, cancel the rest and never put an offer on the book."

So returning false from here is a bug. It should return true because you want to apply the executed crossings, but you don't want to go any further (i.e. to place the offer on the book).

Given the difference between FoK and IoC, I think that tecKILLED is the wrong error code here; it should be tecPARTIALLY_FILLED or somesuch.

[scottschurr]

@nbougais, are you sure this is broken? Note the check for !crossed in the conditional. So we should return {tecKILLED, false} only in the case where no funds are transferred. If any funds were transferred, then we continue to return {tesSUCCESS, true} as before. At least that's what I read crossed as representing.

What am I missing?

[scottschurr]

Rebased to 1.9.2.