From 37c377a1b6ab18055bd3c8b954c5de97aaee683b Mon Sep 17 00:00:00 2001 From: Denis Angell Date: Mon, 15 Sep 2025 16:48:47 +0200 Subject: [PATCH] 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 | 1 + src/test/app/EscrowToken_test.cpp | 68 +++++++++++++++++++++ src/xrpld/app/tx/detail/Escrow.cpp | 9 ++- src/xrpld/ledger/View.h | 3 +- src/xrpld/ledger/detail/View.cpp | 42 ++++++++++--- 5 files changed, 113 insertions(+), 10 deletions(-) diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index 9aacbbe3d9..f04e9f3641 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/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) diff --git a/src/test/app/EscrowToken_test.cpp b/src/test/app/EscrowToken_test.cpp index e81064c825..28c9a5b167 100644 --- a/src/test/app/EscrowToken_test.cpp +++ b/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); } }; diff --git a/src/xrpld/app/tx/detail/Escrow.cpp b/src/xrpld/app/tx/detail/Escrow.cpp index dd0ffac778..3b05aa0007 100644 --- a/src/xrpld/app/tx/detail/Escrow.cpp +++ b/src/xrpld/app/tx/detail/Escrow.cpp @@ -1007,8 +1007,13 @@ escrowUnlockApplyHelper( // compute balance to transfer finalAmt = amount.value() - xferFee; } - - return rippleUnlockEscrowMPT(view, sender, receiver, finalAmt, journal); + return rippleUnlockEscrowMPT( + view, + sender, + receiver, + finalAmt, + view.rules().enabled(fixTokenEscrowV1) ? amount : finalAmt, + journal); } TER diff --git a/src/xrpld/ledger/View.h b/src/xrpld/ledger/View.h index cfd3599f78..faad633e00 100644 --- a/src/xrpld/ledger/View.h +++ b/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. diff --git a/src/xrpld/ledger/detail/View.cpp b/src/xrpld/ledger/detail/View.cpp index 708d5b29f7..473efa58fb 100644 --- a/src/xrpld/ledger/detail/View.cpp +++ b/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(); - auto const mptIssue = amount.get(); + if (!view.rules().enabled(fixTokenEscrowV1)) + XRPL_ASSERT( + netAmount == grossAmount, + "ripple::rippleUnlockEscrowMPT : netAmount == grossAmount"); + + auto const& issuer = netAmount.getIssuer(); + auto const& mptIssue = netAmount.get(); 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; }