`fixFillOrKill`: fix OfferCreate with tfFillOrKill if offer is better than open offer rate (Amendment) #4694

gregtatcam · 2023-09-08T13:07:12Z

High Level Overview of Change

This PR addresses #4684 issue. This is the payment engine breaking change and requires an amendment.

Context of Change

flowCross amendment introduces an issue where the offer crossing with tfFillOrKill flag set and tfSell not set fails to cross an offer with a better rate. For instance, given two offers:
(takerPays/takerGets)
XRP(100)/USD(100)
USD(100)/XRP(101) has tfFillOrKill set
The second offer fails to cross but it should succeed because the taker can buy USD(100) for XRP(100) instead of XRP(101). XRPL reference for the offer create with tfFillOrKill states:

...the owner must receive the full TakerPays amount; if the tfSell flag is enabled, the owner must be able to spend the entire TakerGets amount instead.

Type of Change

[ 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

The amendment code added in StrandFlow.cpp:flow() handles tfFillOrKill and tfFillOrKill+tfSell as two separate cases, whereas the original code handled both as one case.

Test Plan

Added testFillOrKill() to Offer_test.

…rate

scottschurr

I really like both the fix and the unit test that demonstrates the problem. I left a few comments on the unit test, but they are all at your discretion. If you would like to pick up all of the unit test suggestions then you can, if you would like, cherry-pick them from the top-most commit here: https://github.com/scottschurr/rippled/commits/gregt-fix-fillorkill

Very nicely done. Thanks for fixing my bug! 👍

scottschurr · 2023-09-12T18:07:29Z

src/ripple/app/paths/impl/StrandFlow.h

This fix is elegant! Thanks for figuring out the problem and fixing it.

Thanks for reviewing and the suggestions. I added/pushed your commit.

src/test/app/Offer_test.cpp

scottschurr · 2023-09-12T22:07:30Z

src/test/app/Offer_test.cpp

@@ -5081,6 +5081,157 @@ class Offer_test : public beast::unit_test::suite
        pass();
    }

+    void
+    testFillOrKill(FeatureBitset features)


Thanks very much for adding this test. I especially like the way it demonstrates identical behavior before flowCross and after flowCross but with your fix. Nicely done!

scottschurr

Still 👍!

intelliot · 2023-10-04T04:33:38Z

@seelabs : will you review this?

intelliot · 2023-10-11T18:10:57Z

note: seelabs plans to review (eventually :)

seelabs · 2023-10-13T15:10:35Z

src/ripple/app/paths/impl/StrandFlow.h

+        if constexpr (std::is_same_v<TOutAmt, XRPAmount>)
+        {
+            static auto max = XRPAmount{STAmount::cMaxNative};
+            return outReq == max;


We can talk about this, but I don't love this solution. It's using a special value of output request to change behavior. Any payment that uses this value will get this special behavior, even payments that aren't offers and don't have the flag set. It also means if the implementation changes so we no long set these max amounts this code breaks.

I can't argue that this is a great solution, but that's a problem with the FlowCross implementation, not with this fix. @gregtatcam is using the only indication that FlowCross sends down through the payment engine that the tfSell flag is set:

https://github.com/XRPLF/rippled/blob/develop/src/ripple/app/tx/impl/CreateOffer.cpp#L739-L755

But, yes, there is also an offerCrossing flag that is passed as well. We could add that to the sell detection. Maybe like this...

// FlowCross handles tfSell by setting the deliver amount to max auto const sell = [&]() -> bool { if (offerCrossing) { if constexpr (std::is_same_v<TOutAmt, XRPAmount>) { static const auto max = XRPAmount{STAmount::cMaxNative}; return outReq == max; } else if constexpr (std::is_same_v<TOutAmt, IOUAmount>) { static const auto max = IOUAmount{STAmount::cMaxValue / 2, STAmount::cMaxOffset}; return outReq >= max; } } return false; }();

That's still not great, but it makes sure that the sell flag is only set when offer crossing.

Thanks @scottschurr, it is better but as you said not great. I wanted to minimize the payment engine impact and not change the arguments to flow(). If it's ok to change the args then the offerCrossing can be changed from bool to optional<uint32>, which if seated contains the offer crossing flags and if not seated then it's not offer crossing. The semantics of the offerCrossing flag remains the same but it does provide more information. The only thing I don't like is that partialPayment flag carries a duplicate info (it's set to !(txFlags & tfFillOrKill)) but it's probably ok. @seelabs does this work?

I added enum OfferCrossing {No = 0, Yes, Sell} and pass it as the offerCrossing flag. This eliminates the check for the special value.

Looks good. Thanks for making that change, I appreciate it.

seelabs · 2023-10-13T15:32:01Z

src/ripple/protocol/Feature.h

@@ -348,6 +348,7 @@ extern uint256 const fixNonFungibleTokensV1_2;
 extern uint256 const fixNFTokenRemint;
 extern uint256 const fixReducedOffersV1;
 extern uint256 const featureClawback;
+extern uint256 const fixFillOrKillOnFlowCross;


I probably wouldn't mention "FlowCross". Just fixOfferFillOrKill. There's only one offer crossing implementation, there's no need to mention it by name.

intelliot · 2023-10-16T18:20:38Z

Given there are requested changes awaiting review by @gregtatcam , this PR is currently targeting a 2024 release. If there is more urgency to it, please comment here to make the case. Thanks!

scottschurr

First, your solution of changing the bool to an enum is terrific! It clarifies a bunch of code by changing a nondescript bool into something that has useful descriptive names. Great choice!

I did leave two comments. You need to fix the name of a local in Offer_test::run(). You can also consider fixing the names of your new enumerators so they match our code base conventions.

But other than those two things I really like this approach. Thanks!

scottschurr · 2023-10-19T20:17:22Z

src/test/app/Offer_test.cpp

@@ -5153,12 +5344,17 @@ class Offer_test : public beast::unit_test::suite
        FeatureBitset const takerDryOffer{fixTakerDryOfferRemoval};
        FeatureBitset const rmSmallIncreasedQOffers{fixRmSmallIncreasedQOffers};
        FeatureBitset const immediateOfferKilled{featureImmediateOfferKilled};
+        FeatureBitset const fixFillOrKill{fixFillOrKill};


There's a naming problem here. You have two distinct values with the same name. Just pick a different name for the local variable.

Thanks, it's fixed in this commit 3859760.

scottschurr · 2023-10-19T21:28:14Z

src/ripple/app/paths/impl/Steps.h

@@ -39,6 +39,7 @@ class AMMContext;
 enum class DebtDirection { issues, redeems };
 enum class QualityDirection { in, out };
 enum class StrandDirection { forward, reverse };
+enum OfferCrossing { No = 0, Yes = 1, Sell = 2 };


In this code base the enumerators typically start with a lower case letter. See the three preceding enums.

scottschurr

👍

seelabs

👍

intelliot · 2023-10-23T03:23:11Z

@gregtatcam This branch has conflicts that must be resolved

The conflicts look simple but I'll let you take care of them, preferably by adding a merge commit. (As we've been doing, this branch will be squashed to a single commit when it lands in develop)

intelliot · 2023-10-24T21:26:36Z

@gregtatcam - I am not sure you merged with latest develop (if this repo is upstream then maybe do something like git fetch upstream and git merge upstream/develop) - there are still a couple small conflicts (i.e. for featureDID)

at your convenience, please resolve

gregtatcam · 2023-10-24T23:04:51Z

@gregtatcam - I am not sure you merged with latest develop (if this repo is upstream then maybe do something like git fetch upstream and git merge upstream/develop) - there are still a couple small conflicts (i.e. for featureDID)

at your convenience, please resolve

Updated.

intelliot · 2023-10-24T23:17:04Z

@seelabs or @scottschurr - could you have a quick re-review to confirm that the merge looks OK?

scottschurr · 2023-10-24T23:54:55Z

When I attempt to build the latest version of this branch the build fails. But that failure stems from the APIv2: remove tx_history and ledger header commit (1eac4d2)

The failure is in src/test/jtx/TestHelpers.h:38...

make_vector(auto const& input) requires std::ranges::range<decltype(input)>

The compiler says

error: no member named 'range' in namespace 'std::ranges'

I suspect that the library for the version of clang I'm running does not yet have support for std::ranges::range.

My inability to compile just happened with the introduction of this commit. There are newer versions of Xcode I could upgrade to. I'm running 13.4.1, release June 22, 2022 -- about a year and a half old. There is a version 15.0.1 available, released Oct 18, 2023. Is it time to upgrade?

gregtatcam · 2023-10-24T23:59:17Z

My inability to compile just happened with the introduction of this commit. There are newer versions of Xcode I could upgrade to. I'm running 13.4.1, release June 22, 2022 -- about a year and a half old. There is a version 15.0.1 available, released Oct 18, 2023. Is it time to upgrade?

Builds for me. I have Apple clang version 15.0.0 (clang-1500.0.40.1).

scottschurr · 2023-10-24T23:59:30Z

According to this website, ranges support requires Xcode 14.3 or later: https://developer.apple.com/xcode/cpp/

scottschurr · 2023-10-25T00:01:41Z

Xcode 14.3 was released March 30, 2023 according to Wikipedia.

scottschurr · 2023-10-25T00:18:00Z

In the mean time, we'd better have @seelabs review the merge commit, since the code is not building for me at the moment.

intelliot · 2023-10-25T21:50:58Z

note: #4788 is expected to fix the error scottschurr mentioned above

intelliot · 2023-10-30T18:14:51Z

Now that #4788 has been merged, this PR can be merged once CI passes. Meanwhile, @gregtatcam - can you confirm this looks good to merge?

(If so, we can put the Passed label on it)

gregtatcam · 2023-10-30T18:41:47Z

Now that #4788 has been merged, this PR can be merged once CI passes. Meanwhile, @gregtatcam - can you confirm this looks good to merge?

(If so, we can put the Passed label on it)

It's good to merge.

Introduce the `fixFillOrKill` amendment. Fix an edge case occurring when an offer with `tfFillOrKill` set (but without `tfSell` set) fails to cross an offer with a better rate. If `tfFillOrKill` is set, then the owner must receive the full TakerPays. Without this amendment, an offer fails if the entire `TakerGets` is not spent. With this amendment, when `tfSell` is not set, the entire `TakerGets` does not have to be spent. For details about OfferCreate, see: https://xrpl.org/offercreate.html Fix XRPLF#4684 --------- Co-authored-by: Scott Schurr <scott@ripple.com>

Fix OfferCreate with tfFillOrKill if offer is better than open offer …

7e5b31d

…rate

gregtatcam requested a review from scottschurr September 8, 2023 13:07

scottschurr approved these changes Sep 12, 2023

View reviewed changes

[FOLD] Tweaks to Offer unit tests

a98ebed

intelliot requested a review from seelabs September 13, 2023 12:33

intelliot assigned seelabs Sep 13, 2023

scottschurr approved these changes Sep 13, 2023

View reviewed changes

seelabs requested changes Oct 13, 2023

View reviewed changes

seelabs reviewed Oct 13, 2023

View reviewed changes

intelliot assigned gregtatcam Oct 13, 2023

intelliot modified the milestones: 2.0, 2024 release Oct 13, 2023

intelliot added Bug Amendment labels Oct 16, 2023

gregtatcam added 2 commits October 18, 2023 09:43

[FOLD] Address reviewers feedback.

c7c6e96

[FOLD] Fix initialization for fillOrKill feature.

3859760

scottschurr requested changes Oct 19, 2023

View reviewed changes

[FOLD] Change OfferCrossing enumerators to a lower case letter

2834d46

scottschurr approved these changes Oct 20, 2023

View reviewed changes

seelabs approved these changes Oct 20, 2023

View reviewed changes

intelliot unassigned seelabs Oct 23, 2023

intelliot modified the milestones: 2024 release, 2.0 Oct 23, 2023

intelliot added the Testable label Oct 23, 2023

Merge branch 'develop' into fix-fillorkill

7a7c9cc

[FOLD] Resolve merge conflicts

404b1b4

Merge branch 'develop' into fix-fillorkill

0a1aad8

Merge branch 'develop' into fix-fillorkill

41b5c61

intelliot unassigned gregtatcam Oct 24, 2023

Merge branch 'develop' into fix-fillorkill

7040e9a

intelliot added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Oct 30, 2023

intelliot merged commit 3b624d8 into XRPLF:develop Oct 30, 2023
16 checks passed

intelliot mentioned this pull request Nov 3, 2023

Proposed 2.0.0-rc1 #4802

Closed

1 task

intelliot changed the title ~~Fix OfferCreate with tfFillOrKill if offer is better than open offer rate~~ fixFillOrKill: fix OfferCreate with tfFillOrKill if offer is better than open offer rate (Amendment) Nov 3, 2023

legleux self-assigned this Nov 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`fixFillOrKill`: fix OfferCreate with tfFillOrKill if offer is better than open offer rate (Amendment) #4694

`fixFillOrKill`: fix OfferCreate with tfFillOrKill if offer is better than open offer rate (Amendment) #4694

gregtatcam commented Sep 8, 2023

scottschurr left a comment

scottschurr Sep 12, 2023

gregtatcam Sep 13, 2023

scottschurr Sep 12, 2023

scottschurr left a comment

intelliot commented Oct 4, 2023

intelliot commented Oct 11, 2023

seelabs Oct 13, 2023

scottschurr Oct 13, 2023

gregtatcam Oct 16, 2023

gregtatcam Oct 18, 2023

seelabs Oct 20, 2023

seelabs Oct 13, 2023

intelliot commented Oct 16, 2023

scottschurr left a comment

scottschurr Oct 19, 2023

gregtatcam Oct 19, 2023

scottschurr Oct 19, 2023

scottschurr left a comment

seelabs left a comment

intelliot commented Oct 23, 2023

intelliot commented Oct 24, 2023

gregtatcam commented Oct 24, 2023

intelliot commented Oct 24, 2023

scottschurr commented Oct 24, 2023

gregtatcam commented Oct 24, 2023

scottschurr commented Oct 24, 2023

scottschurr commented Oct 25, 2023

scottschurr commented Oct 25, 2023

intelliot commented Oct 25, 2023

intelliot commented Oct 30, 2023

gregtatcam commented Oct 30, 2023

fixFillOrKill: fix OfferCreate with tfFillOrKill if offer is better than open offer rate (Amendment) #4694

fixFillOrKill: fix OfferCreate with tfFillOrKill if offer is better than open offer rate (Amendment) #4694

Conversation

gregtatcam commented Sep 8, 2023

High Level Overview of Change

Context of Change

Type of Change

Before / After

Test Plan

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

intelliot commented Oct 4, 2023

intelliot commented Oct 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

intelliot commented Oct 16, 2023

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

seelabs left a comment

Choose a reason for hiding this comment

intelliot commented Oct 23, 2023

intelliot commented Oct 24, 2023

gregtatcam commented Oct 24, 2023

intelliot commented Oct 24, 2023

scottschurr commented Oct 24, 2023

gregtatcam commented Oct 24, 2023

scottschurr commented Oct 24, 2023

scottschurr commented Oct 25, 2023

scottschurr commented Oct 25, 2023

intelliot commented Oct 25, 2023

intelliot commented Oct 30, 2023

gregtatcam commented Oct 30, 2023

`fixFillOrKill`: fix OfferCreate with tfFillOrKill if offer is better than open offer rate (Amendment) #4694

`fixFillOrKill`: fix OfferCreate with tfFillOrKill if offer is better than open offer rate (Amendment) #4694