rippled icon indicating copy to clipboard operation
rippled copied to clipboard

Add featureImmediateOfferKilled for tfImmediateOrCancel offers:

Open scottschurr opened this issue 2 years ago • 2 comments

High Level Overview of Change

Fixes https://github.com/ripple/rippled/issues/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

  • [x] Amendment (Functionality waits for amendment to pass. Once passed may cause amendment blocking.)
  • [x] Breaking change (fix or feature that would cause existing functionality to not work as expected).
  • [x] 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.

scottschurr avatar May 05 '22 23:05 scottschurr

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.

mDuo13 avatar May 09 '22 23:05 mDuo13

Rebased to 1.9.2.

scottschurr avatar Aug 04 '22 21:08 scottschurr