Address AI review

This commit is contained in:
Gregory Tsipenyuk
2026-04-09 17:25:17 -04:00
parent 63f270f00e
commit 77df21b485
2 changed files with 71 additions and 13 deletions

View File

@@ -414,8 +414,8 @@ ValidMPTTransfer::finalize(
if (txnType == ttAMM_CLAWBACK)
return true;
// DEX transactions (cross-currency payments, offer creates) are subject to
// the MPTCanTrade flag in addition to the standard transfer rules.
// DEX transactions (AMM[Create,Deposit,Withdraw], cross-currency payments, offer creates) are
// subject to the MPTCanTrade flag in addition to the standard transfer rules.
// A payment is only DEX if it is a cross-currency payment.
auto const isDEX = [&tx, &txnType] {
if (txnType == ttPAYMENT)
@@ -425,10 +425,12 @@ ValidMPTTransfer::finalize(
auto const amount = tx[sfAmount];
return tx[~sfSendMax].value_or(amount).asset() != amount.asset();
}
return txnType == ttOFFER_CREATE;
return txnType == ttAMM_CREATE || txnType == ttAMM_DEPOSIT || txnType == ttAMM_WITHDRAW ||
txnType == ttOFFER_CREATE;
}();
// Only enforce once MPTokensV2 is enabled to preserve consensus with non-V2 nodes.
// Log invariant failure error even if MPTokensV2 is disabled.
auto const enforce = !view.rules().enabled(featureMPTokensV2);
for (auto const& [mptID, values] : amount_)
@@ -447,16 +449,10 @@ ValidMPTTransfer::finalize(
for (auto const& [account, value] : values)
{
// Check once: if any involved account is frozen, the whole
// issuance transfer is considered frozen.
if (!frozen && isFrozen(view, account, MPTIssue{mptID}))
{
frozen = true;
}
// Classify each account as a sender or receiver based on whether
// their MPTAmount decreased or increased.
if (value.amtBefore.has_value() && value.amtAfter.has_value() &&
*value.amtBefore != *value.amtAfter)
// Classify each account as a sender or receiver based on whether their MPTAmount
// decreased or increased. Count new MPToken holders (no amtBefore) as receivers.
if (value.amtAfter.has_value() &&
(!value.amtBefore.has_value() || *value.amtBefore != *value.amtAfter))
{
if (*value.amtAfter > *value.amtBefore)
{
@@ -467,6 +463,13 @@ ValidMPTTransfer::finalize(
++senders;
}
}
// Check once: if any involved account is frozen, the whole
// issuance transfer is considered frozen. Only need to check for
// frozen if there is a transfer of funds.
if (!frozen && isFrozen(view, account, MPTIssue{mptID}))
{
frozen = true;
}
}
// A transfer between holders has occurred (senders > 0 && receivers > 0).
// Fail if the issuance is frozen, does not permit transfers, or — for

View File

@@ -12,6 +12,7 @@
#include <xrpl/basics/base_uint.h>
#include <xrpl/beast/utility/Zero.h>
#include <xrpl/ledger/helpers/AMMHelpers.h>
#include <xrpl/ledger/helpers/MPTokenHelpers.h>
#include <xrpl/ledger/helpers/TokenHelpers.h>
#include <xrpl/protocol/Feature.h>
#include <xrpl/protocol/TER.h>
@@ -6625,6 +6626,57 @@ class MPToken_test : public beast::unit_test::suite
BEAST_EXPECT(carolOwnersAfterWithdraw == carolOwnersBeforeWithdraw + 1);
}
void
testTradeAndTransfer()
{
using namespace jtx;
testcase("Trade and Transfer");
// Verify canMPTTradeAndTransfer validates the flags when from == to and from != to
Account const gw{"gw"};
Account const alice{"alice"};
Account const carol{"carol"};
Env env(*this);
env.fund(XRP(1'000), gw, alice, carol);
MPTTester mpt(
{.env = env,
.issuer = gw,
.holders = {alice, carol},
.pay = 100,
.flags = MPTDEXFlags,
.mutableFlags = tmfMPTCanMutateCanTransfer | tmfMPTCanMutateCanTrade});
// Both flags are enabled
BEAST_EXPECT(isTesSuccess(canMPTTradeAndTransfer(*env.current(), mpt, gw, gw)));
BEAST_EXPECT(isTesSuccess(canMPTTradeAndTransfer(*env.current(), mpt, gw, alice)));
BEAST_EXPECT(isTesSuccess(canMPTTradeAndTransfer(*env.current(), mpt, alice, alice)));
BEAST_EXPECT(isTesSuccess(canMPTTradeAndTransfer(*env.current(), mpt, alice, carol)));
// MPTCanTrade is disabled
mpt.set({.mutableFlags = tmfMPTClearCanTrade});
BEAST_EXPECT(canMPTTradeAndTransfer(*env.current(), mpt, gw, gw) == tecNO_PERMISSION);
BEAST_EXPECT(canMPTTradeAndTransfer(*env.current(), mpt, gw, alice) == tecNO_PERMISSION);
BEAST_EXPECT(canMPTTradeAndTransfer(*env.current(), mpt, alice, alice) == tecNO_PERMISSION);
BEAST_EXPECT(canMPTTradeAndTransfer(*env.current(), mpt, alice, carol) == tecNO_PERMISSION);
// MPTCanTransfer is disabled
mpt.set({.mutableFlags = tmfMPTSetCanTrade});
mpt.set({.mutableFlags = tmfMPTClearCanTransfer});
BEAST_EXPECT(isTesSuccess(canMPTTradeAndTransfer(*env.current(), mpt, gw, gw)));
BEAST_EXPECT(isTesSuccess(canMPTTradeAndTransfer(*env.current(), mpt, gw, alice)));
BEAST_EXPECT(canMPTTradeAndTransfer(*env.current(), mpt, alice, alice) == tecNO_AUTH);
BEAST_EXPECT(canMPTTradeAndTransfer(*env.current(), mpt, alice, carol) == tecNO_AUTH);
// Both flags are disabled
mpt.set({.mutableFlags = tmfMPTClearCanTrade});
BEAST_EXPECT(canMPTTradeAndTransfer(*env.current(), mpt, gw, gw) == tecNO_PERMISSION);
BEAST_EXPECT(canMPTTradeAndTransfer(*env.current(), mpt, gw, alice) == tecNO_PERMISSION);
BEAST_EXPECT(canMPTTradeAndTransfer(*env.current(), mpt, alice, alice) == tecNO_PERMISSION);
BEAST_EXPECT(canMPTTradeAndTransfer(*env.current(), mpt, alice, carol) == tecNO_PERMISSION);
}
public:
void
run() override
@@ -6732,6 +6784,9 @@ public:
// Test AMM
testBasicAMM(all);
// Test Trade/Transfer
testTradeAndTransfer();
// Fixes
testFixDoubleOwnerCount(all);
}