Skip to content

Commit

Permalink
Fix OfferCreate with tfFillOrKill if offer is better than open offer …
Browse files Browse the repository at this point in the history 
…rate
  • Loading branch information
@gregtatcam
gregtatcam committed Sep 8, 2023
1 parent c6f6375 commit 7e5b31d
Show file tree
Hide file tree
Showing 4 changed files with 206 additions and 9 deletions.
44 changes: 41 additions & 3 deletions src/ripple/app/paths/impl/StrandFlow.h
View file
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;
}
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
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;

} // namespace ripple

Expand Down
1 change: 1 addition & 0 deletions src/ripple/protocol/impl/Feature.cpp
View file
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
167 changes: 162 additions & 5 deletions src/test/app/Offer_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5081,6 +5081,157 @@ class Offer_test : public beast::unit_test::suite
pass();
}

void
testFillOrKill(FeatureBitset features)
{
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.trust(USD(1'000), maker, taker);
env.trust(EUR(1'000), maker, taker);

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

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();

TER const err =
features[fixFillOrKillOnFlowCross] || !features[featureFlowCross]
? TER(tesSUCCESS)
: tecKILLED;

// tfFillOrKill, TakerPays must be filled
{
env(offer(maker, XRP(100), USD(100)));
env(offer(taker, USD(100), XRP(101)),
txflags(tfFillOrKill),
ter(err));
makerXRPBalance -= txfee(env, 1);
takerXRPBalance -= txfee(env, 1);
if (tesSUCCESS == err)
{
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(offer(taker, XRP(100), USD(101)),
txflags(tfFillOrKill),
ter(err));
makerXRPBalance -= txfee(env, 1);
takerXRPBalance -= txfee(env, 1);
if (tesSUCCESS == err)
{
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(offer(taker, EUR(100), USD(101)),
txflags(tfFillOrKill),
ter(err));
makerXRPBalance -= txfee(env, 1);
takerXRPBalance -= txfee(env, 1);
if (tesSUCCESS == err)
{
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(offer(taker, USD(100), XRP(101)),
txflags(tfFillOrKill | tfSell));
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(offer(taker, XRP(100), USD(101)),
txflags(tfFillOrKill | tfSell));
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(offer(taker, EUR(100), USD(101)),
txflags(tfFillOrKill | tfSell));
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(offer(taker, USD(100), XRP(99)),
txflags(flags),
ter(tecKILLED));
makerXRPBalance -= txfee(env, 1);
takerXRPBalance -= txfee(env, 1);
BEAST_EXPECT(expectOffers(env, taker, 0));

env(offer(maker, USD(100), XRP(100)));
env(offer(taker, XRP(100), USD(99)),
txflags(flags),
ter(tecKILLED));
makerXRPBalance -= txfee(env, 1);
takerXRPBalance -= txfee(env, 1);
BEAST_EXPECT(expectOffers(env, taker, 0));

env(offer(maker, USD(100), EUR(100)));
env(offer(taker, EUR(100), USD(99)),
txflags(flags),
ter(tecKILLED));
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 +5293,7 @@ class Offer_test : public beast::unit_test::suite
testTicketCancelOffer(features);
testRmSmallIncreasedQOffersXRP(features);
testRmSmallIncreasedQOffersIOU(features);
testFillOrKill(features);
}

void
Expand All @@ -5153,12 +5305,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);
for (auto const& features : {all - fixFillOrKill, all})
{
testAll(features - takerDryOffer - immediateOfferKilled);
testAll(
features - flowCross - takerDryOffer - immediateOfferKilled);
testAll(features - flowCross - immediateOfferKilled);
testAll(features - rmSmallIncreasedQOffers - immediateOfferKilled);
testAll(features);
}
testFalseAssert();
}
};
Expand Down

0 comments on commit 7e5b31d

Please sign in to comment.