Fix: EscrowTokenV1 (#5571)

* resolves an accounting inconsistency in MPT escrows where transfer fees were not properly handled when unlocking escrowed tokens.

include/xrpl/protocol/detail/features.macro

@@ -32,6 +32,7 @@
 // If you add an amendment here, then do not forget to increment `numFeatures`
 // in include/xrpl/protocol/Feature.h.
 
+XRPL_FIX    (TokenEscrowV1,              Supported::yes, VoteBehavior::DefaultNo)
 XRPL_FIX    (DelegateV1_1,               Supported::no,  VoteBehavior::DefaultNo)
 XRPL_FIX    (PriceOracleOrder,           Supported::no,  VoteBehavior::DefaultNo)
 XRPL_FIX    (MPTDeliveredAmount,         Supported::no,  VoteBehavior::DefaultNo)

src/test/app/EscrowToken_test.cpp

@@ -3501,6 +3501,10 @@ struct EscrowToken_test : public beast::unit_test::suite
             BEAST_EXPECT(
                 transferRate.value == std::uint32_t(1'000'000'000 * 1.25));
 
+            BEAST_EXPECT(mptEscrowed(env, alice, MPT) == 125);
+            BEAST_EXPECT(issuerMPTEscrowed(env, MPT) == 125);
+            BEAST_EXPECT(env.balance(gw, MPT) == MPT(20'000));
+
             // bob can finish escrow
             env(escrow::finish(bob, alice, seq1),
                 escrow::condition(escrow::cb1),
@@ -3510,6 +3514,15 @@ struct EscrowToken_test : public beast::unit_test::suite
 
             BEAST_EXPECT(env.balance(alice, MPT) == preAlice - delta);
             BEAST_EXPECT(env.balance(bob, MPT) == MPT(10'100));
+
+            auto const escrowedWithFix =
+                env.current()->rules().enabled(fixTokenEscrowV1) ? 0 : 25;
+            auto const outstandingWithFix =
+                env.current()->rules().enabled(fixTokenEscrowV1) ? MPT(19'975)
+                                                                 : MPT(20'000);
+            BEAST_EXPECT(mptEscrowed(env, alice, MPT) == escrowedWithFix);
+            BEAST_EXPECT(issuerMPTEscrowed(env, MPT) == escrowedWithFix);
+            BEAST_EXPECT(env.balance(gw, MPT) == outstandingWithFix);
         }
 
         // test locked rate: cancel
@@ -3554,6 +3567,60 @@ struct EscrowToken_test : public beast::unit_test::suite
 
             BEAST_EXPECT(env.balance(alice, MPT) == preAlice);
             BEAST_EXPECT(env.balance(bob, MPT) == preBob);
+            BEAST_EXPECT(env.balance(gw, MPT) == MPT(20'000));
+            BEAST_EXPECT(mptEscrowed(env, alice, MPT) == 0);
+            BEAST_EXPECT(issuerMPTEscrowed(env, MPT) == 0);
+        }
+
+        // test locked rate: issuer is destination
+        {
+            Env env{*this, features};
+            auto const baseFee = env.current()->fees().base;
+            auto const alice = Account("alice");
+            auto const bob = Account("bob");
+            auto const gw = Account("gw");
+
+            MPTTester mptGw(env, gw, {.holders = {alice, bob}});
+            mptGw.create(
+                {.transferFee = 25000,
+                 .ownerCount = 1,
+                 .holderCount = 0,
+                 .flags = tfMPTCanEscrow | tfMPTCanTransfer});
+            mptGw.authorize({.account = alice});
+            mptGw.authorize({.account = bob});
+            auto const MPT = mptGw["MPT"];
+            env(pay(gw, alice, MPT(10'000)));
+            env(pay(gw, bob, MPT(10'000)));
+            env.close();
+
+            // alice can create escrow w/ xfer rate
+            auto const preAlice = env.balance(alice, MPT);
+            auto const seq1 = env.seq(alice);
+            auto const delta = MPT(125);
+            env(escrow::create(alice, gw, MPT(125)),
+                escrow::condition(escrow::cb1),
+                escrow::finish_time(env.now() + 1s),
+                fee(baseFee * 150));
+            env.close();
+            auto const transferRate = escrow::rate(env, alice, seq1);
+            BEAST_EXPECT(
+                transferRate.value == std::uint32_t(1'000'000'000 * 1.25));
+
+            BEAST_EXPECT(mptEscrowed(env, alice, MPT) == 125);
+            BEAST_EXPECT(issuerMPTEscrowed(env, MPT) == 125);
+            BEAST_EXPECT(env.balance(gw, MPT) == MPT(20'000));
+
+            // bob can finish escrow
+            env(escrow::finish(gw, alice, seq1),
+                escrow::condition(escrow::cb1),
+                escrow::fulfillment(escrow::fb1),
+                fee(baseFee * 150));
+            env.close();
+
+            BEAST_EXPECT(env.balance(alice, MPT) == preAlice - delta);
+            BEAST_EXPECT(mptEscrowed(env, alice, MPT) == 0);
+            BEAST_EXPECT(issuerMPTEscrowed(env, MPT) == 0);
+            BEAST_EXPECT(env.balance(gw, MPT) == MPT(19'875));
         }
     }
 
@@ -3878,6 +3945,7 @@ public:
         FeatureBitset const all{testable_amendments()};
         testIOUWithFeats(all);
         testMPTWithFeats(all);
+        testMPTWithFeats(all - fixTokenEscrowV1);
     }
 };
 

src/xrpld/app/tx/detail/Escrow.cpp

@@ -1007,8 +1007,13 @@ escrowUnlockApplyHelper<MPTIssue>(
         // compute balance to transfer
         finalAmt = amount.value() - xferFee;
     }
-
+    return rippleUnlockEscrowMPT(
-    return rippleUnlockEscrowMPT(view, sender, receiver, finalAmt, journal);
+        view,
+        sender,
+        receiver,
+        finalAmt,
+        view.rules().enabled(fixTokenEscrowV1) ? amount : finalAmt,
+        journal);
 }
 
 TER

src/xrpld/ledger/View.h

@@ -719,7 +719,8 @@ rippleUnlockEscrowMPT(
     ApplyView& view,
     AccountID const& uGrantorID,
     AccountID const& uGranteeID,
-    STAmount const& saAmount,
+    STAmount const& netAmount,
+    STAmount const& grossAmount,
     beast::Journal j);
 
 /** Calls static accountSendIOU if saAmount represents Issue.

src/xrpld/ledger/detail/View.cpp

@@ -3006,11 +3006,17 @@ rippleUnlockEscrowMPT(
     ApplyView& view,
     AccountID const& sender,
     AccountID const& receiver,
-    STAmount const& amount,
+    STAmount const& netAmount,
+    STAmount const& grossAmount,
     beast::Journal j)
 {
-    auto const issuer = amount.getIssuer();
+    if (!view.rules().enabled(fixTokenEscrowV1))
-    auto const mptIssue = amount.get<MPTIssue>();
+        XRPL_ASSERT(
+            netAmount == grossAmount,
+            "ripple::rippleUnlockEscrowMPT : netAmount == grossAmount");
+
+    auto const& issuer = netAmount.getIssuer();
+    auto const& mptIssue = netAmount.get<MPTIssue>();
     auto const mptID = keylet::mptIssuance(mptIssue.getMptID());
     auto sleIssuance = view.peek(mptID);
     if (!sleIssuance)
@@ -3031,7 +3037,7 @@ rippleUnlockEscrowMPT(
         }  // LCOV_EXCL_STOP
 
         auto const locked = sleIssuance->getFieldU64(sfLockedAmount);
-        auto const redeem = amount.mpt().value();
+        auto const redeem = grossAmount.mpt().value();
 
         // Underflow check for subtraction
         if (!canSubtract(
@@ -3064,7 +3070,7 @@ rippleUnlockEscrowMPT(
         }  // LCOV_EXCL_STOP
 
         auto current = sle->getFieldU64(sfMPTAmount);
-        auto delta = amount.mpt().value();
+        auto delta = netAmount.mpt().value();
 
         // Overflow check for addition
         if (!canAdd(STAmount(mptIssue, current), STAmount(mptIssue, delta)))
@@ -3082,7 +3088,7 @@ rippleUnlockEscrowMPT(
     {
         // Decrease the Issuance OutstandingAmount
         auto const outstanding = sleIssuance->getFieldU64(sfOutstandingAmount);
-        auto const redeem = amount.mpt().value();
+        auto const redeem = netAmount.mpt().value();
 
         // Underflow check for subtraction
         if (!canSubtract(
@@ -3126,7 +3132,7 @@ rippleUnlockEscrowMPT(
         }  // LCOV_EXCL_STOP
 
         auto const locked = sle->getFieldU64(sfLockedAmount);
-        auto const delta = amount.mpt().value();
+        auto const delta = grossAmount.mpt().value();
 
         // Underflow check for subtraction
         if (!canSubtract(STAmount(mptIssue, locked), STAmount(mptIssue, delta)))
@@ -3144,6 +3150,28 @@ rippleUnlockEscrowMPT(
             sle->setFieldU64(sfLockedAmount, newLocked);
         view.update(sle);
     }
+
+    // Note: The gross amount is the amount that was locked, the net
+    // amount is the amount that is being unlocked. The difference is the fee
+    // that was charged for the transfer. If this difference is greater than
+    // zero, we need to update the outstanding amount.
+    auto const diff = grossAmount.mpt().value() - netAmount.mpt().value();
+    if (diff != 0)
+    {
+        auto const outstanding = sleIssuance->getFieldU64(sfOutstandingAmount);
+        // Underflow check for subtraction
+        if (!canSubtract(
+                STAmount(mptIssue, outstanding), STAmount(mptIssue, diff)))
+        {  // LCOV_EXCL_START
+            JLOG(j.error())
+                << "rippleUnlockEscrowMPT: insufficient outstanding amount for "
+                << mptIssue.getMptID() << ": " << outstanding << " < " << diff;
+            return tecINTERNAL;
+        }  // LCOV_EXCL_STOP
+
+        sleIssuance->setFieldU64(sfOutstandingAmount, outstanding - diff);
+        view.update(sleIssuance);
+    }
     return tesSUCCESS;
 }