Merge branch 'develop' into ximinez/online-delete-gaps

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    (PriceOracleOrder,           Supported::no,  VoteBehavior::DefaultNo)
 XRPL_FIX    (MPTDeliveredAmount,         Supported::no,  VoteBehavior::DefaultNo)
 XRPL_FIX    (AMMClawbackRounding,        Supported::no,  VoteBehavior::DefaultNo)
 XRPL_FEATURE(TokenEscrow,                Supported::yes, VoteBehavior::DefaultNo)
@@ -39,7 +40,7 @@ XRPL_FIX    (EnforceNFTokenTrustlineV2,  Supported::yes, VoteBehavior::DefaultNo
 XRPL_FIX    (AMMv1_3,                    Supported::yes, VoteBehavior::DefaultNo)
 XRPL_FEATURE(PermissionedDEX,            Supported::yes, VoteBehavior::DefaultNo)
 XRPL_FEATURE(Batch,                      Supported::yes, VoteBehavior::DefaultNo)
-XRPL_FEATURE(SingleAssetVault,           Supported::no, VoteBehavior::DefaultNo)
+XRPL_FEATURE(SingleAssetVault,           Supported::no,  VoteBehavior::DefaultNo)
 XRPL_FEATURE(PermissionDelegation,       Supported::yes, VoteBehavior::DefaultNo)
 XRPL_FIX    (PayChanCancelAfter,         Supported::yes, VoteBehavior::DefaultNo)
 // Check flags in Credential transactions

src/test/app/Oracle_test.cpp

@@ -678,6 +678,61 @@ private:
             oracle.set(
                 UpdateArg{.series = {{"XRP", "USD", 742, 2}}, .fee = baseFee});
         }
+
+        for (bool const withFixOrder : {false, true})
+        {
+            // Should be same order as creation
+            Env env(
+                *this,
+                withFixOrder ? testable_amendments()
+                             : testable_amendments() - fixPriceOracleOrder);
+            auto const baseFee =
+                static_cast<int>(env.current()->fees().base.drops());
+
+            auto test = [&](Env& env, DataSeries const& series) {
+                env.fund(XRP(1'000), owner);
+                Oracle oracle(
+                    env, {.owner = owner, .series = series, .fee = baseFee});
+                BEAST_EXPECT(oracle.exists());
+                auto sle = env.le(keylet::oracle(owner, oracle.documentID()));
+                BEAST_EXPECT(
+                    sle->getFieldArray(sfPriceDataSeries).size() ==
+                    series.size());
+
+                auto const beforeQuoteAssetName1 =
+                    sle->getFieldArray(sfPriceDataSeries)[0]
+                        .getFieldCurrency(sfQuoteAsset)
+                        .getText();
+                auto const beforeQuoteAssetName2 =
+                    sle->getFieldArray(sfPriceDataSeries)[1]
+                        .getFieldCurrency(sfQuoteAsset)
+                        .getText();
+
+                oracle.set(UpdateArg{.series = series, .fee = baseFee});
+                sle = env.le(keylet::oracle(owner, oracle.documentID()));
+
+                auto const afterQuoteAssetName1 =
+                    sle->getFieldArray(sfPriceDataSeries)[0]
+                        .getFieldCurrency(sfQuoteAsset)
+                        .getText();
+                auto const afterQuoteAssetName2 =
+                    sle->getFieldArray(sfPriceDataSeries)[1]
+                        .getFieldCurrency(sfQuoteAsset)
+                        .getText();
+
+                if (env.current()->rules().enabled(fixPriceOracleOrder))
+                {
+                    BEAST_EXPECT(afterQuoteAssetName1 == beforeQuoteAssetName1);
+                    BEAST_EXPECT(afterQuoteAssetName2 == beforeQuoteAssetName2);
+                }
+                else
+                {
+                    BEAST_EXPECT(afterQuoteAssetName1 != beforeQuoteAssetName1);
+                    BEAST_EXPECT(afterQuoteAssetName2 != beforeQuoteAssetName2);
+                }
+            };
+            test(env, {{"XRP", "USD", 742, 2}, {"XRP", "EUR", 711, 2}});
+        }
     }
 
     void

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

@@ -175,126 +175,18 @@ MPTokenAuthorize::createMPToken(
     return tesSUCCESS;
 }
 
-TER
-MPTokenAuthorize::authorize(
-    ApplyView& view,
-    beast::Journal journal,
-    MPTAuthorizeArgs const& args)
-{
-    auto const sleAcct = view.peek(keylet::account(args.account));
-    if (!sleAcct)
-        return tecINTERNAL;
-
-    // If the account that submitted the tx is a holder
-    // Note: `account_` is holder's account
-    //       `holderID` is NOT used
-    if (!args.holderID)
-    {
-        // When a holder wants to unauthorize/delete a MPT, the ledger must
-        //      - delete mptokenKey from owner directory
-        //      - delete the MPToken
-        if (args.flags & tfMPTUnauthorize)
-        {
-            auto const mptokenKey =
-                keylet::mptoken(args.mptIssuanceID, args.account);
-            auto const sleMpt = view.peek(mptokenKey);
-            if (!sleMpt || (*sleMpt)[sfMPTAmount] != 0)
-                return tecINTERNAL;  // LCOV_EXCL_LINE
-
-            if (!view.dirRemove(
-                    keylet::ownerDir(args.account),
-                    (*sleMpt)[sfOwnerNode],
-                    sleMpt->key(),
-                    false))
-                return tecINTERNAL;  // LCOV_EXCL_LINE
-
-            adjustOwnerCount(view, sleAcct, -1, journal);
-
-            view.erase(sleMpt);
-            return tesSUCCESS;
-        }
-
-        // A potential holder wants to authorize/hold a mpt, the ledger must:
-        //      - add the new mptokenKey to the owner directory
-        //      - create the MPToken object for the holder
-
-        // The reserve that is required to create the MPToken. Note
-        // that although the reserve increases with every item
-        // an account owns, in the case of MPTokens we only
-        // *enforce* a reserve if the user owns more than two
-        // items. This is similar to the reserve requirements of trust lines.
-        std::uint32_t const uOwnerCount = sleAcct->getFieldU32(sfOwnerCount);
-        XRPAmount const reserveCreate(
-            (uOwnerCount < 2) ? XRPAmount(beast::zero)
-                              : view.fees().accountReserve(uOwnerCount + 1));
-
-        if (args.priorBalance < reserveCreate)
-            return tecINSUFFICIENT_RESERVE;
-
-        auto const mptokenKey =
-            keylet::mptoken(args.mptIssuanceID, args.account);
-        auto mptoken = std::make_shared<SLE>(mptokenKey);
-        if (auto ter = dirLink(view, args.account, mptoken))
-            return ter;  // LCOV_EXCL_LINE
-
-        (*mptoken)[sfAccount] = args.account;
-        (*mptoken)[sfMPTokenIssuanceID] = args.mptIssuanceID;
-        (*mptoken)[sfFlags] = 0;
-        view.insert(mptoken);
-
-        // Update owner count.
-        adjustOwnerCount(view, sleAcct, 1, journal);
-
-        return tesSUCCESS;
-    }
-
-    auto const sleMptIssuance =
-        view.read(keylet::mptIssuance(args.mptIssuanceID));
-    if (!sleMptIssuance)
-        return tecINTERNAL;
-
-    // If the account that submitted this tx is the issuer of the MPT
-    // Note: `account_` is issuer's account
-    //       `holderID` is holder's account
-    if (args.account != (*sleMptIssuance)[sfIssuer])
-        return tecINTERNAL;
-
-    auto const sleMpt =
-        view.peek(keylet::mptoken(args.mptIssuanceID, *args.holderID));
-    if (!sleMpt)
-        return tecINTERNAL;
-
-    std::uint32_t const flagsIn = sleMpt->getFieldU32(sfFlags);
-    std::uint32_t flagsOut = flagsIn;
-
-    // Issuer wants to unauthorize the holder, unset lsfMPTAuthorized on
-    // their MPToken
-    if (args.flags & tfMPTUnauthorize)
-        flagsOut &= ~lsfMPTAuthorized;
-    // Issuer wants to authorize a holder, set lsfMPTAuthorized on their
-    // MPToken
-    else
-        flagsOut |= lsfMPTAuthorized;
-
-    if (flagsIn != flagsOut)
-        sleMpt->setFieldU32(sfFlags, flagsOut);
-
-    view.update(sleMpt);
-    return tesSUCCESS;
-}
-
 TER
 MPTokenAuthorize::doApply()
 {
     auto const& tx = ctx_.tx;
-    return authorize(
+    return authorizeMPToken(
         ctx_.view(),
+        mPriorBalance,
+        tx[sfMPTokenIssuanceID],
+        account_,
         ctx_.journal,
-        {.priorBalance = mPriorBalance,
+        tx.getFlags(),
-         .mptIssuanceID = tx[sfMPTokenIssuanceID],
+        tx[~sfHolder]);
-         .account = account_,
-         .flags = tx.getFlags(),
-         .holderID = tx[~sfHolder]});
 }
 
 }  // namespace ripple

src/xrpld/app/tx/detail/MPTokenAuthorize.h

@@ -48,12 +48,6 @@ public:
     static TER
     preclaim(PreclaimContext const& ctx);
 
-    static TER
-    authorize(
-        ApplyView& view,
-        beast::Journal journal,
-        MPTAuthorizeArgs const& args);
-
     static TER
     createMPToken(
         ApplyView& view,

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

@@ -209,6 +209,17 @@ SetOracle::doApply()
 {
     auto const oracleID = keylet::oracle(account_, ctx_.tx[sfOracleDocumentID]);
 
+    auto populatePriceData = [](STObject& priceData, STObject const& entry) {
+        setPriceDataInnerObjTemplate(priceData);
+        priceData.setFieldCurrency(
+            sfBaseAsset, entry.getFieldCurrency(sfBaseAsset));
+        priceData.setFieldCurrency(
+            sfQuoteAsset, entry.getFieldCurrency(sfQuoteAsset));
+        priceData.setFieldU64(sfAssetPrice, entry.getFieldU64(sfAssetPrice));
+        if (entry.isFieldPresent(sfScale))
+            priceData.setFieldU8(sfScale, entry.getFieldU8(sfScale));
+    };
+
     if (auto sle = ctx_.view().peek(oracleID))
     {
         // update
@@ -249,15 +260,7 @@ SetOracle::doApply()
             {
                 // add a token pair with the price
                 STObject priceData{sfPriceData};
-                setPriceDataInnerObjTemplate(priceData);
+                populatePriceData(priceData, entry);
-                priceData.setFieldCurrency(
-                    sfBaseAsset, entry.getFieldCurrency(sfBaseAsset));
-                priceData.setFieldCurrency(
-                    sfQuoteAsset, entry.getFieldCurrency(sfQuoteAsset));
-                priceData.setFieldU64(
-                    sfAssetPrice, entry.getFieldU64(sfAssetPrice));
-                if (entry.isFieldPresent(sfScale))
-                    priceData.setFieldU8(sfScale, entry.getFieldU8(sfScale));
                 pairs.emplace(key, std::move(priceData));
             }
         }
@@ -285,7 +288,26 @@ SetOracle::doApply()
         sle->setFieldVL(sfProvider, ctx_.tx[sfProvider]);
         if (ctx_.tx.isFieldPresent(sfURI))
             sle->setFieldVL(sfURI, ctx_.tx[sfURI]);
-        auto const& series = ctx_.tx.getFieldArray(sfPriceDataSeries);
+
+        STArray series;
+        if (!ctx_.view().rules().enabled(fixPriceOracleOrder))
+        {
+            series = ctx_.tx.getFieldArray(sfPriceDataSeries);
+        }
+        else
+        {
+            std::map<std::pair<Currency, Currency>, STObject> pairs;
+            for (auto const& entry : ctx_.tx.getFieldArray(sfPriceDataSeries))
+            {
+                auto const key = tokenPairKey(entry);
+                STObject priceData{sfPriceData};
+                populatePriceData(priceData, entry);
+                pairs.emplace(key, std::move(priceData));
+            }
+            for (auto const& iter : pairs)
+                series.push_back(std::move(iter.second));
+        }
+
         sle->setFieldArray(sfPriceDataSeries, series);
         sle->setFieldVL(sfAssetClass, ctx_.tx[sfAssetClass]);
         sle->setFieldU32(sfLastUpdateTime, ctx_.tx[sfLastUpdateTime]);

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

@@ -210,12 +210,12 @@ VaultDeposit::doApply()
         auto sleMpt = view().read(keylet::mptoken(mptIssuanceID, account_));
         if (!sleMpt)
         {
-            if (auto const err = MPTokenAuthorize::authorize(
+            if (auto const err = authorizeMPToken(
                     view(),
-                    ctx_.journal,
+                    mPriorBalance,
-                    {.priorBalance = mPriorBalance,
+                    mptIssuanceID->value(),
-                     .mptIssuanceID = mptIssuanceID->value(),
+                    account_,
-                     .account = account_});
+                    ctx_.journal);
                 !isTesSuccess(err))
                 return err;
         }
@@ -223,15 +223,15 @@ VaultDeposit::doApply()
         // If the vault is private, set the authorized flag for the vault owner
         if (vault->isFlag(tfVaultPrivate))
         {
-            if (auto const err = MPTokenAuthorize::authorize(
+            if (auto const err = authorizeMPToken(
                     view(),
+                    mPriorBalance,              // priorBalance
+                    mptIssuanceID->value(),     // mptIssuanceID
+                    sleIssuance->at(sfIssuer),  // account
                     ctx_.journal,
-                    {
+                    {},       // flags
-                        .priorBalance = mPriorBalance,
+                    account_  // holderID
-                        .mptIssuanceID = mptIssuanceID->value(),
+                );
-                        .account = sleIssuance->at(sfIssuer),
-                        .holderID = account_,
-                    });
                 !isTesSuccess(err))
                 return err;
         }

src/xrpld/ledger/View.h

@@ -600,6 +600,16 @@ addEmptyHolding(
         asset.value());
 }
 
+[[nodiscard]] TER
+authorizeMPToken(
+    ApplyView& view,
+    XRPAmount const& priorBalance,
+    MPTID const& mptIssuanceID,
+    AccountID const& account,
+    beast::Journal journal,
+    std::uint32_t flags = 0,
+    std::optional<AccountID> holderID = std::nullopt);
+
 // VFALCO NOTE Both STAmount parameters should just
 //             be "Amount", a unit-less number.
 //

src/xrpld/ledger/detail/View.cpp

@@ -18,7 +18,6 @@
 //==============================================================================
 
 #include <xrpld/app/misc/CredentialHelpers.h>
-#include <xrpld/app/tx/detail/MPTokenAuthorize.h>
 #include <xrpld/ledger/ReadView.h>
 #include <xrpld/ledger/View.h>
 
@@ -1215,12 +1214,115 @@ addEmptyHolding(
     if (view.peek(keylet::mptoken(mptID, accountID)))
         return tecDUPLICATE;
 
-    return MPTokenAuthorize::authorize(
+    return authorizeMPToken(view, priorBalance, mptID, accountID, journal);
-        view,
+}
-        journal,
+
-        {.priorBalance = priorBalance,
+[[nodiscard]] TER
-         .mptIssuanceID = mptID,
+authorizeMPToken(
-         .account = accountID});
+    ApplyView& view,
+    XRPAmount const& priorBalance,
+    MPTID const& mptIssuanceID,
+    AccountID const& account,
+    beast::Journal journal,
+    std::uint32_t flags,
+    std::optional<AccountID> holderID)
+{
+    auto const sleAcct = view.peek(keylet::account(account));
+    if (!sleAcct)
+        return tecINTERNAL;
+
+    // If the account that submitted the tx is a holder
+    // Note: `account_` is holder's account
+    //       `holderID` is NOT used
+    if (!holderID)
+    {
+        // When a holder wants to unauthorize/delete a MPT, the ledger must
+        //      - delete mptokenKey from owner directory
+        //      - delete the MPToken
+        if (flags & tfMPTUnauthorize)
+        {
+            auto const mptokenKey = keylet::mptoken(mptIssuanceID, account);
+            auto const sleMpt = view.peek(mptokenKey);
+            if (!sleMpt || (*sleMpt)[sfMPTAmount] != 0)
+                return tecINTERNAL;  // LCOV_EXCL_LINE
+
+            if (!view.dirRemove(
+                    keylet::ownerDir(account),
+                    (*sleMpt)[sfOwnerNode],
+                    sleMpt->key(),
+                    false))
+                return tecINTERNAL;  // LCOV_EXCL_LINE
+
+            adjustOwnerCount(view, sleAcct, -1, journal);
+
+            view.erase(sleMpt);
+            return tesSUCCESS;
+        }
+
+        // A potential holder wants to authorize/hold a mpt, the ledger must:
+        //      - add the new mptokenKey to the owner directory
+        //      - create the MPToken object for the holder
+
+        // The reserve that is required to create the MPToken. Note
+        // that although the reserve increases with every item
+        // an account owns, in the case of MPTokens we only
+        // *enforce* a reserve if the user owns more than two
+        // items. This is similar to the reserve requirements of trust lines.
+        std::uint32_t const uOwnerCount = sleAcct->getFieldU32(sfOwnerCount);
+        XRPAmount const reserveCreate(
+            (uOwnerCount < 2) ? XRPAmount(beast::zero)
+                              : view.fees().accountReserve(uOwnerCount + 1));
+
+        if (priorBalance < reserveCreate)
+            return tecINSUFFICIENT_RESERVE;
+
+        auto const mptokenKey = keylet::mptoken(mptIssuanceID, account);
+        auto mptoken = std::make_shared<SLE>(mptokenKey);
+        if (auto ter = dirLink(view, account, mptoken))
+            return ter;  // LCOV_EXCL_LINE
+
+        (*mptoken)[sfAccount] = account;
+        (*mptoken)[sfMPTokenIssuanceID] = mptIssuanceID;
+        (*mptoken)[sfFlags] = 0;
+        view.insert(mptoken);
+
+        // Update owner count.
+        adjustOwnerCount(view, sleAcct, 1, journal);
+
+        return tesSUCCESS;
+    }
+
+    auto const sleMptIssuance = view.read(keylet::mptIssuance(mptIssuanceID));
+    if (!sleMptIssuance)
+        return tecINTERNAL;
+
+    // If the account that submitted this tx is the issuer of the MPT
+    // Note: `account_` is issuer's account
+    //       `holderID` is holder's account
+    if (account != (*sleMptIssuance)[sfIssuer])
+        return tecINTERNAL;
+
+    auto const sleMpt = view.peek(keylet::mptoken(mptIssuanceID, *holderID));
+    if (!sleMpt)
+        return tecINTERNAL;
+
+    std::uint32_t const flagsIn = sleMpt->getFieldU32(sfFlags);
+    std::uint32_t flagsOut = flagsIn;
+
+    // Issuer wants to unauthorize the holder, unset lsfMPTAuthorized on
+    // their MPToken
+    if (flags & tfMPTUnauthorize)
+        flagsOut &= ~lsfMPTAuthorized;
+    // Issuer wants to authorize a holder, set lsfMPTAuthorized on their
+    // MPToken
+    else
+        flagsOut |= lsfMPTAuthorized;
+
+    if (flagsIn != flagsOut)
+        sleMpt->setFieldU32(sfFlags, flagsOut);
+
+    view.update(sleMpt);
+    return tesSUCCESS;
 }
 
 TER
@@ -1418,13 +1520,14 @@ removeEmptyHolding(
     if (mptoken->at(sfMPTAmount) != 0)
         return tecHAS_OBLIGATIONS;
 
-    return MPTokenAuthorize::authorize(
+    return authorizeMPToken(
         view,
+        {},  // priorBalance
+        mptID,
+        accountID,
         journal,
-        {.priorBalance = {},
+        tfMPTUnauthorize  // flags
-         .mptIssuanceID = mptID,
+    );
-         .account = accountID,
-         .flags = tfMPTUnauthorize});
 }
 
 TER
@@ -2497,15 +2600,12 @@ enforceMPTokenAuthorization(
         XRPL_ASSERT(
             maybeDomainID.has_value() && sleToken == nullptr,
             "ripple::enforceMPTokenAuthorization : new MPToken for domain");
-        if (auto const err = MPTokenAuthorize::authorize(
+        if (auto const err = authorizeMPToken(
                 view,
-                j,
+                priorBalance,   // priorBalance
-                {
+                mptIssuanceID,  // mptIssuanceID
-                    .priorBalance = priorBalance,
+                account,        // account
-                    .mptIssuanceID = mptIssuanceID,
+                j);
-                    .account = account,
-                    .flags = 0,
-                });
             !isTesSuccess(err))
             return err;