Skip to content

Commit

Permalink
fixFillOrKill: fix offer crossing with tfFillOrKill (#4694)
Browse files Browse the repository at this point in the history
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 #4684

---------

Co-authored-by: Scott Schurr <[email protected]>
  • Loading branch information
gregtatcam and scottschurr authored Oct 30, 2023
1 parent ac02e56 commit 3b624d8
Show file tree
Hide file tree
Showing 16 changed files with 272 additions and 36 deletions.
2 changes: 1 addition & 1 deletion src/ripple/app/paths/Flow.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ flow(
bool defaultPaths,
bool partialPayment,
bool ownerPaysTransferFee,
bool offerCrossing,
OfferCrossing offerCrossing,
std::optional<Quality> const& limitQuality,
std::optional<STAmount> const& sendMax,
beast::Journal j,
Expand Down
4 changes: 2 additions & 2 deletions src/ripple/app/paths/Flow.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ struct FlowDebugInfo;
@param partialPayment If the payment cannot deliver the entire
requested amount, deliver as much as possible, given the constraints
@param ownerPaysTransferFee If true then owner, not sender, pays fee
@param offerCrossing If true then flow is executing offer crossing, not
@param offerCrossing If Yes or Sell then flow is executing offer crossing, not
payments
@param limitQuality Do not use liquidity below this quality threshold
@param sendMax Do not spend more than this amount
Expand All @@ -62,7 +62,7 @@ flow(
bool defaultPaths,
bool partialPayment,
bool ownerPaysTransferFee,
bool offerCrossing,
OfferCrossing offerCrossing,
std::optional<Quality> const& limitQuality,
std::optional<STAmount> const& sendMax,
beast::Journal j,
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/app/paths/RippleCalc.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ RippleCalc::rippleCalculate(
defaultPaths,
partialPayment,
ownerPaysTransferFee,
/* offerCrossing */ false,
OfferCrossing::no,
limitQuality,
sendMax,
j,
Expand Down
6 changes: 3 additions & 3 deletions src/ripple/app/paths/impl/PaySteps.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ toStrand(
std::optional<Issue> const& sendMaxIssue,
STPath const& path,
bool ownerPaysTransferFee,
bool offerCrossing,
OfferCrossing offerCrossing,
AMMContext& ammContext,
beast::Journal j)
{
Expand Down Expand Up @@ -475,7 +475,7 @@ toStrands(
STPathSet const& paths,
bool addDefaultPath,
bool ownerPaysTransferFee,
bool offerCrossing,
OfferCrossing offerCrossing,
AMMContext& ammContext,
beast::Journal j)
{
Expand Down Expand Up @@ -588,7 +588,7 @@ StrandContext::StrandContext(
std::optional<Quality> const& limitQuality_,
bool isLast_,
bool ownerPaysTransferFee_,
bool offerCrossing_,
OfferCrossing offerCrossing_,
bool isDefaultPath_,
std::array<boost::container::flat_set<Issue>, 2>& seenDirectIssues_,
boost::container::flat_set<Issue>& seenBookOuts_,
Expand Down
14 changes: 8 additions & 6 deletions src/ripple/app/paths/impl/Steps.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 };

inline bool
redeems(DebtDirection dir)
Expand Down Expand Up @@ -398,7 +399,7 @@ toStrand(
std::optional<Issue> const& sendMaxIssue,
STPath const& path,
bool ownerPaysTransferFee,
bool offerCrossing,
OfferCrossing offerCrossing,
AMMContext& ammContext,
beast::Journal j);

Expand Down Expand Up @@ -438,7 +439,7 @@ toStrands(
STPathSet const& paths,
bool addDefaultPath,
bool ownerPaysTransferFee,
bool offerCrossing,
OfferCrossing offerCrossing,
AMMContext& ammContext,
beast::Journal j);

Expand Down Expand Up @@ -531,9 +532,10 @@ struct StrandContext
bool const isFirst; ///< true if Step is first in Strand
bool const isLast = false; ///< true if Step is last in Strand
bool const ownerPaysTransferFee; ///< true if owner, not sender, pays fee
bool const offerCrossing; ///< true if offer crossing, not payment
bool const isDefaultPath; ///< true if Strand is default path
size_t const strandSize; ///< Length of Strand
OfferCrossing const
offerCrossing; ///< Yes/Sell if offer crossing, not payment
bool const isDefaultPath; ///< true if Strand is default path
size_t const strandSize; ///< Length of Strand
/** The previous step in the strand. Needed to check the no ripple
constraint
*/
Expand Down Expand Up @@ -563,7 +565,7 @@ struct StrandContext
std::optional<Quality> const& limitQuality_,
bool isLast_,
bool ownerPaysTransferFee_,
bool offerCrossing_,
OfferCrossing offerCrossing_,
bool isDefaultPath_,
std::array<boost::container::flat_set<Issue>, 2>&
seenDirectIssues_, ///< For detecting currency loops
Expand Down
33 changes: 29 additions & 4 deletions src/ripple/app/paths/impl/StrandFlow.h
Original file line number Diff line number Diff line change
Expand Up @@ -557,7 +557,7 @@ flow(
std::vector<Strand> const& strands,
TOutAmt const& outReq,
bool partialPayment,
bool offerCrossing,
OfferCrossing offerCrossing,
std::optional<Quality> const& limitQuality,
std::optional<STAmount> const& sendMaxST,
beast::Journal j,
Expand Down Expand Up @@ -817,6 +817,19 @@ 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 fillOrKillEnabled = baseView.rules().enabled(fixFillOrKill);

if (actualOut != outReq)
{
if (actualOut > outReq)
Expand All @@ -827,8 +840,14 @@ 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 ||
(fillOrKillEnabled && offerCrossing != OfferCrossing::sell))
return {
tecPATH_PARTIAL,
actualIn,
Expand All @@ -840,11 +859,17 @@ flow(
return {tecPATH_DRY, std::move(ofrsToRmOnFail)};
}
}
if (offerCrossing && !partialPayment)
if (offerCrossing &&
(!partialPayment &&
(!fillOrKillEnabled || offerCrossing == OfferCrossing::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
2 changes: 1 addition & 1 deletion src/ripple/app/tx/impl/CashCheck.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,7 @@ CashCheck::doApply()
true, // default path
static_cast<bool>(optDeliverMin), // partial payment
true, // owner pays transfer fee
false, // offer crossing
OfferCrossing::no,
std::nullopt,
sleCheck->getFieldAmount(sfSendMax),
viewJ);
Expand Down
4 changes: 3 additions & 1 deletion src/ripple/app/tx/impl/CreateOffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -736,8 +736,10 @@ CreateOffer::flowCross(
}
// Special handling for the tfSell flag.
STAmount deliver = takerAmount.out;
OfferCrossing offerCrossing = OfferCrossing::yes;
if (txFlags & tfSell)
{
offerCrossing = OfferCrossing::sell;
// We are selling, so we will accept *more* than the offer
// specified. Since we don't know how much they might offer,
// we allow delivery of the largest possible amount.
Expand All @@ -764,7 +766,7 @@ CreateOffer::flowCross(
true, // default path
!(txFlags & tfFillOrKill), // partial payment
true, // owner pays transfer fee
true, // offer crossing
offerCrossing,
threshold,
sendMax,
j_);
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/app/tx/impl/XChainBridge.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -505,7 +505,7 @@ transferHelper(
/*default path*/ true,
/*partial payment*/ false,
/*owner pays transfer fee*/ true,
/*offer crossing*/ false,
/*offer crossing*/ OfferCrossing::no,
/*limit quality*/ std::nullopt,
/*sendmax*/ std::nullopt,
j);
Expand Down
3 changes: 2 additions & 1 deletion src/ripple/protocol/Feature.h
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 = 64;
static constexpr std::size_t numFeatures = 65;

/** 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 @@ -351,6 +351,7 @@ extern uint256 const featureClawback;
extern uint256 const featureXChainBridge;
extern uint256 const fixDisallowIncomingV1;
extern uint256 const featureDID;
extern uint256 const fixFillOrKill;

} // namespace ripple

Expand Down
1 change: 1 addition & 0 deletions src/ripple/protocol/impl/Feature.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,7 @@ REGISTER_FEATURE(AMM, Supported::yes, VoteBehavior::De
REGISTER_FEATURE(XChainBridge, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FIX (fixDisallowIncomingV1, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FEATURE(DID, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FIX(fixFillOrKill, Supported::yes, VoteBehavior::DefaultNo);

// The following amendments are obsolete, but must remain supported
// because they could potentially get enabled.
Expand Down
2 changes: 1 addition & 1 deletion src/test/app/AMMExtended_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2103,7 +2103,7 @@ struct AMMExtended_test : public jtx::AMMTest
false,
false,
true,
false,
OfferCrossing::no,
std::nullopt,
smax,
flowJournal);
Expand Down
2 changes: 1 addition & 1 deletion src/test/app/Flow_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,7 @@ struct Flow_test : public beast::unit_test::suite
false,
false,
true,
false,
OfferCrossing::no,
std::nullopt,
smax,
flowJournal);
Expand Down
Loading

0 comments on commit 3b624d8

Please sign in to comment.