Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 10 commits into from
Oct 30, 2023
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7e5b31d
Fix OfferCreate with tfFillOrKill if offer is better than open offer …
gregtatcam Sep 8, 2023
a98ebed
[FOLD] Tweaks to Offer unit tests
scottschurr Sep 12, 2023
c7c6e96
[FOLD] Address reviewers feedback.
gregtatcam Oct 18, 2023
3859760
[FOLD] Fix initialization for fillOrKill feature.
gregtatcam Oct 19, 2023
2834d46
[FOLD] Change OfferCrossing enumerators to a lower case letter
gregtatcam Oct 19, 2023
7a7c9cc
Merge branch 'develop' into fix-fillorkill
gregtatcam Oct 23, 2023
404b1b4
[FOLD] Resolve merge conflicts
gregtatcam Oct 23, 2023
0a1aad8
Merge branch 'develop' into fix-fillorkill
gregtatcam Oct 24, 2023
41b5c61
Merge branch 'develop' into fix-fillorkill
intelliot Oct 24, 2023
7040e9a
Merge branch 'develop' into fix-fillorkill
intelliot Oct 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 41 additions & 3 deletions src/ripple/app/paths/impl/StrandFlow.h
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
Expand Up @@ -817,6 +817,35 @@ flow(
JLOG(j.trace()) << "Total flow: in: " << to_string(actualIn)
<< " out: " << to_string(actualOut);

/* flowCross doesn't handle offer crossing with tfFillOrKill flag correctly.
* 1. If tfFillOrKill is set then the owner must receive the full
* TakerPays. We reverse pays and gets because during crossing
* we are taking, therefore the owner must deliver the full TakerPays and
* the entire TakerGets doesn't have to be spent.
* Pre-fixFillOrKill amendment code fails if the entire TakerGets
* is not spent. fixFillOrKill addresses this issue.
* 2. If tfSell is also set then the owner must spend the entire TakerGets
* even if it means obtaining more than TakerPays. Since the pays and gets
* are reversed, the owner must send the entire TakerGets.
*/
bool const fixFillOrKill =
baseView.rules().enabled(fixFillOrKillOnFlowCross);
// tfSell is handled by setting the deliver amount to max
auto const sell = [&]() -> bool {
if constexpr (std::is_same_v<TOutAmt, XRPAmount>)
{
static auto max = XRPAmount{STAmount::cMaxNative};
return outReq == max;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
else if constexpr (std::is_same_v<TOutAmt, IOUAmount>)
{
static auto max =
IOUAmount{STAmount::cMaxValue / 2, STAmount::cMaxOffset};
return outReq == max;
}
return false;
}();

if (actualOut != outReq)
{
if (actualOut > outReq)
Expand All @@ -827,8 +856,13 @@ flow(
if (!partialPayment)
{
// If we're offerCrossing a !partialPayment, then we're
// handling tfFillOrKill. That case is handled below; not here.
if (!offerCrossing)
// handling tfFillOrKill.
// Pre-fixFillOrKill amendment:
// That case is handled below; not here.
// fixFillOrKill amendment:
// That case is handled here if tfSell is also not set; i.e,
// case 1.
if (!offerCrossing || (fixFillOrKill && !sell))
return {
tecPATH_PARTIAL,
actualIn,
Expand All @@ -840,11 +874,15 @@ flow(
return {tecPATH_DRY, std::move(ofrsToRmOnFail)};
}
}
if (offerCrossing && !partialPayment)
if (offerCrossing && (!partialPayment && (!fixFillOrKill || sell)))
{
// If we're offer crossing and partialPayment is *not* true, then
// we're handling a FillOrKill offer. In this case remainingIn must
// be zero (all funds must be consumed) or else we kill the offer.
// Pre-fixFillOrKill amendment:
// Handles both cases 1. and 2.
// fixFillOrKill amendment:
// Handles 2. 1. is handled above and falls through for tfSell.
assert(remainingIn);
if (remainingIn && *remainingIn != beast::zero)
return {
Expand Down
3 changes: 2 additions & 1 deletion src/ripple/protocol/Feature.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ namespace detail {
// Feature.cpp. Because it's only used to reserve storage, and determine how
// large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than
// the actual number of amendments. A LogicError on startup will verify this.
static constexpr std::size_t numFeatures = 61;
static constexpr std::size_t numFeatures = 62;

/** Amendments that this server supports and the default voting behavior.
Whether they are enabled depends on the Rules defined in the validated
Expand Down Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


} // namespace ripple

Expand Down
1 change: 1 addition & 0 deletions src/ripple/protocol/impl/Feature.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,7 @@ REGISTER_FIX (fixNFTokenRemint, Supported::yes, VoteBehavior::De
REGISTER_FIX (fixReducedOffersV1, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FEATURE(Clawback, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FEATURE(AMM, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FIX(fixFillOrKillOnFlowCross, Supported::yes, VoteBehavior::DefaultNo);

// The following amendments are obsolete, but must remain supported
// because they could potentially get enabled.
Expand Down
198 changes: 197 additions & 1 deletion src/test/app/Offer_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5081,6 +5081,196 @@ class Offer_test : public beast::unit_test::suite
pass();
}

void
testFillOrKill(FeatureBitset features)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

{
testcase("fixFillOrKillOnFlowCross");
using namespace jtx;
Env env(*this, features);
Account const issuer("issuer");
Account const maker("maker");
Account const taker("taker");
auto const USD = issuer["USD"];
auto const EUR = issuer["EUR"];

env.fund(XRP(1'000), issuer);
env.fund(XRP(1'000), maker, taker);
env.close();

env.trust(USD(1'000), maker, taker);
env.trust(EUR(1'000), maker, taker);
scottschurr marked this conversation as resolved.
Show resolved Hide resolved
env.close();

env(pay(issuer, maker, USD(1'000)));
env(pay(issuer, taker, USD(1'000)));
env(pay(issuer, maker, EUR(1'000)));
env.close();

auto makerUSDBalance = env.balance(maker, USD).value();
auto takerUSDBalance = env.balance(taker, USD).value();
auto makerEURBalance = env.balance(maker, EUR).value();
auto takerEURBalance = env.balance(taker, EUR).value();
auto makerXRPBalance = env.balance(maker, XRP).value();
auto takerXRPBalance = env.balance(taker, XRP).value();

// tfFillOrKill, TakerPays must be filled
{
TER const err = features[fixFillOrKillOnFlowCross] ||
!features[featureFlowCross]
? TER(tesSUCCESS)
: tecKILLED;

env(offer(maker, XRP(100), USD(100)));
env.close();

env(offer(taker, USD(100), XRP(101)),
txflags(tfFillOrKill),
ter(err));
env.close();

makerXRPBalance -= txfee(env, 1);
takerXRPBalance -= txfee(env, 1);
if (err == tesSUCCESS)
{
makerUSDBalance -= USD(100);
takerUSDBalance += USD(100);
makerXRPBalance += XRP(100).value();
takerXRPBalance -= XRP(100).value();
}
BEAST_EXPECT(expectOffers(env, taker, 0));

env(offer(maker, USD(100), XRP(100)));
env.close();

env(offer(taker, XRP(100), USD(101)),
txflags(tfFillOrKill),
ter(err));
env.close();

makerXRPBalance -= txfee(env, 1);
takerXRPBalance -= txfee(env, 1);
if (err == tesSUCCESS)
{
makerUSDBalance += USD(100);
takerUSDBalance -= USD(100);
makerXRPBalance -= XRP(100).value();
takerXRPBalance += XRP(100).value();
}
BEAST_EXPECT(expectOffers(env, taker, 0));

env(offer(maker, USD(100), EUR(100)));
env.close();

env(offer(taker, EUR(100), USD(101)),
txflags(tfFillOrKill),
ter(err));
env.close();

makerXRPBalance -= txfee(env, 1);
takerXRPBalance -= txfee(env, 1);
if (err == tesSUCCESS)
{
makerUSDBalance += USD(100);
takerUSDBalance -= USD(100);
makerEURBalance -= EUR(100);
takerEURBalance += EUR(100);
}
BEAST_EXPECT(expectOffers(env, taker, 0));
}

// tfFillOrKill + tfSell, TakerGets must be filled
{
env(offer(maker, XRP(101), USD(101)));
env.close();

env(offer(taker, USD(100), XRP(101)),
txflags(tfFillOrKill | tfSell));
env.close();

makerUSDBalance -= USD(101);
takerUSDBalance += USD(101);
makerXRPBalance += XRP(101).value() - txfee(env, 1);
takerXRPBalance -= XRP(101).value() + txfee(env, 1);
BEAST_EXPECT(expectOffers(env, taker, 0));

env(offer(maker, USD(101), XRP(101)));
env.close();

env(offer(taker, XRP(100), USD(101)),
txflags(tfFillOrKill | tfSell));
env.close();

makerUSDBalance += USD(101);
takerUSDBalance -= USD(101);
makerXRPBalance -= XRP(101).value() + txfee(env, 1);
takerXRPBalance += XRP(101).value() - txfee(env, 1);
BEAST_EXPECT(expectOffers(env, taker, 0));

env(offer(maker, USD(101), EUR(101)));
env.close();

env(offer(taker, EUR(100), USD(101)),
txflags(tfFillOrKill | tfSell));
env.close();

makerUSDBalance += USD(101);
takerUSDBalance -= USD(101);
makerEURBalance -= EUR(101);
takerEURBalance += EUR(101);
makerXRPBalance -= txfee(env, 1);
takerXRPBalance -= txfee(env, 1);
BEAST_EXPECT(expectOffers(env, taker, 0));
}

// Fail regardless of fixFillOrKillOnFlowCross amendment
for (auto const flags : {tfFillOrKill, tfFillOrKill + tfSell})
{
env(offer(maker, XRP(100), USD(100)));
env.close();

env(offer(taker, USD(100), XRP(99)),
txflags(flags),
ter(tecKILLED));
env.close();

makerXRPBalance -= txfee(env, 1);
takerXRPBalance -= txfee(env, 1);
BEAST_EXPECT(expectOffers(env, taker, 0));

env(offer(maker, USD(100), XRP(100)));
env.close();

env(offer(taker, XRP(100), USD(99)),
txflags(flags),
ter(tecKILLED));
env.close();

makerXRPBalance -= txfee(env, 1);
takerXRPBalance -= txfee(env, 1);
BEAST_EXPECT(expectOffers(env, taker, 0));

env(offer(maker, USD(100), EUR(100)));
env.close();

env(offer(taker, EUR(100), USD(99)),
txflags(flags),
ter(tecKILLED));
env.close();

makerXRPBalance -= txfee(env, 1);
takerXRPBalance -= txfee(env, 1);
BEAST_EXPECT(expectOffers(env, taker, 0));
}

BEAST_EXPECT(
env.balance(maker, USD) == makerUSDBalance &&
env.balance(taker, USD) == takerUSDBalance &&
env.balance(maker, EUR) == makerEURBalance &&
env.balance(taker, EUR) == takerEURBalance &&
env.balance(maker, XRP) == makerXRPBalance &&
env.balance(taker, XRP) == takerXRPBalance);
}

void
testAll(FeatureBitset features)
{
Expand Down Expand Up @@ -5142,6 +5332,7 @@ class Offer_test : public beast::unit_test::suite
testTicketCancelOffer(features);
testRmSmallIncreasedQOffersXRP(features);
testRmSmallIncreasedQOffersIOU(features);
testFillOrKill(features);
}

void
Expand All @@ -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{fixFillOrKillOnFlowCross};

testAll(all - takerDryOffer - immediateOfferKilled);
testAll(all - flowCross - takerDryOffer - immediateOfferKilled);
testAll(all - flowCross - immediateOfferKilled);
testAll(all - rmSmallIncreasedQOffers - immediateOfferKilled);
testAll(
all - rmSmallIncreasedQOffers - immediateOfferKilled -
fixFillOrKill);
testAll(all - fixFillOrKill);
testAll(all);

testFalseAssert();
}
};
Expand Down