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

src/ripple/app/paths/Flow.cpp

@@ -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,

src/ripple/app/paths/Flow.h

@@ -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
@@ -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,

src/ripple/app/paths/RippleCalc.cpp

@@ -106,7 +106,7 @@ RippleCalc::rippleCalculate(
  defaultPaths,
  partialPayment,
  ownerPaysTransferFee,
- /* offerCrossing */ false,
+ OfferCrossing::No,
  limitQuality,
  sendMax,
  j,

src/ripple/app/paths/impl/PaySteps.cpp

@@ -141,7 +141,7 @@ toStrand(
  std::optional<Issue> const& sendMaxIssue,
  STPath const& path,
  bool ownerPaysTransferFee,
- bool offerCrossing,
+ OfferCrossing offerCrossing,
  AMMContext& ammContext,
  beast::Journal j)
 {
@@ -475,7 +475,7 @@ toStrands(
  STPathSet const& paths,
  bool addDefaultPath,
  bool ownerPaysTransferFee,
- bool offerCrossing,
+ OfferCrossing offerCrossing,
  AMMContext& ammContext,
  beast::Journal j)
 {
@@ -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_,

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 };
 
 inline bool
 redeems(DebtDirection dir)
@@ -398,7 +399,7 @@ toStrand(
  std::optional<Issue> const& sendMaxIssue,
  STPath const& path,
  bool ownerPaysTransferFee,
- bool offerCrossing,
+ OfferCrossing offerCrossing,
  AMMContext& ammContext,
  beast::Journal j);
 
@@ -438,7 +439,7 @@ toStrands(
  STPathSet const& paths,
  bool addDefaultPath,
  bool ownerPaysTransferFee,
- bool offerCrossing,
+ OfferCrossing offerCrossing,
  AMMContext& ammContext,
  beast::Journal j);
 
@@ -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
  */
@@ -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

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

@@ -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,
@@ -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)
@@ -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,
@@ -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 {

src/ripple/app/tx/impl/CashCheck.cpp

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

src/ripple/app/tx/impl/CreateOffer.cpp

@@ -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.
@@ -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_);

src/ripple/protocol/Feature.h

@@ -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
@@ -348,6 +348,7 @@ extern uint256 const fixNonFungibleTokensV1_2;
 extern uint256 const fixNFTokenRemint;
 extern uint256 const fixReducedOffersV1;
 extern uint256 const featureClawback;
+extern uint256 const fixFillOrKill;
 
 } // namespace ripple
 

src/ripple/protocol/impl/Feature.cpp

@@ -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(fixFillOrKill, Supported::yes, VoteBehavior::DefaultNo);
 
 // The following amendments are obsolete, but must remain supported
 // because they could potentially get enabled.

src/test/app/AMMExtended_test.cpp

@@ -2103,7 +2103,7 @@ struct AMMExtended_test : public jtx::AMMTest
  false,
  false,
  true,
- false,
+ OfferCrossing::No,
  std::nullopt,
  smax,
  flowJournal);

src/test/app/Flow_test.cpp

@@ -473,7 +473,7 @@ struct Flow_test : public beast::unit_test::suite
  false,
  false,
  true,
- false,
+ OfferCrossing::No,
  std::nullopt,
  smax,
  flowJournal);