fixInnerObjTemplate: Add STObject constructor to explicitly set inner object template

src/ripple/app/misc/impl/AMMUtils.cpp

@@ -287,17 +287,18 @@ initializeFeeAuctionVote(
     Issue const& lptIssue,
     std::uint16_t tfee)
 {
+    bool fixInnerObj(view.rules().enabled(fixInnerObjTemplate));
     // AMM creator gets the voting slot.
     STArray voteSlots;
-    STObject voteEntry{sfVoteEntry};
+    STObject voteEntry{sfVoteEntry, fixInnerObj};
     if (tfee != 0)
         voteEntry.setFieldU16(sfTradingFee, tfee);
     voteEntry.setFieldU32(sfVoteWeight, VOTE_WEIGHT_SCALE_FACTOR);
     voteEntry.setAccountID(sfAccount, account);
     voteSlots.push_back(voteEntry);
     ammSle->setFieldArray(sfVoteSlots, voteSlots);
     // AMM creator gets the auction slot for free.
-    auto& auctionSlot = ammSle->peekFieldObject(sfAuctionSlot);
+    STObject auctionSlot{sfAuctionSlot, fixInnerObj};
     auctionSlot.setAccountID(sfAccount, account);
     // current + sec in 24h
     auto const expiration = std::chrono::duration_cast<std::chrono::seconds>(
@@ -315,6 +316,7 @@ initializeFeeAuctionVote(
         auctionSlot.setFieldU16(sfDiscountedFee, dfee);
     else if (auctionSlot.isFieldPresent(sfDiscountedFee))
         auctionSlot.makeFieldAbsent(sfDiscountedFee);
+    ammSle->set(&auctionSlot);
 }
 
 }  // namespace ripple

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

@@ -104,6 +104,7 @@ applyVote(
     Number den{0};
     // Account already has vote entry
     bool foundAccount = false;
+    bool const fixInnerObj{ctx_.view().rules().enabled(fixInnerObjTemplate)};
     // Iterate over the current vote entries and update each entry
     // per current total tokens balance and each LP tokens balance.
     // Find the entry with the least tokens and whether the account
@@ -119,7 +120,7 @@ applyVote(
             continue;
         }
         auto feeVal = entry[sfTradingFee];
-        STObject newEntry{sfVoteEntry};
+        STObject newEntry{sfVoteEntry, fixInnerObj};
         // The account already has the vote entry.
         if (account == account_)
         {
@@ -158,7 +159,7 @@ applyVote(
     {
         auto update = [&](std::optional<std::uint8_t> const& minPos =
                               std::nullopt) {
-            STObject newEntry{sfVoteEntry};
+            STObject newEntry{sfVoteEntry, fixInnerObj};
             if (feeNew != 0)
                 newEntry.setFieldU16(sfTradingFee, feeNew);
             newEntry.setFieldU32(

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 = 65;
+static constexpr std::size_t numFeatures = 66;
 
 /** Amendments that this server supports and the default voting behavior.
    Whether they are enabled depends on the Rules defined in the validated
@@ -352,6 +352,7 @@ extern uint256 const featureXChainBridge;
 extern uint256 const fixDisallowIncomingV1;
 extern uint256 const featureDID;
 extern uint256 const fixFillOrKill;
+extern uint256 const fixInnerObjTemplate;
 
 }  // namespace ripple
 

src/ripple/protocol/SOTemplate.h

@@ -35,6 +35,11 @@ enum SOEStyle {
     soeREQUIRED = 0,  // required
     soeOPTIONAL = 1,  // optional, may be present with default value
     soeDEFAULT = 2,   // optional, if present, must not have default value
+                      // inner objects with the default fields have to
+                      // explicitly initialize SOTemplate by having
+                      // fixInnerObjTemplateEnabled set to true in STObject
+                      // constructor if fixInnerObjTemplate amendment
+                      // is enabled
 };
 
 //------------------------------------------------------------------------------

src/ripple/protocol/STObject.h

@@ -101,6 +101,7 @@ class STObject : public STBase, public CountedObject<STObject>
     STObject(SerialIter& sit, SField const& name, int depth = 0);
     STObject(SerialIter&& sit, SField const& name);
     explicit STObject(SField const& name);
+    STObject(SField const& name, bool fixInnerObjTemplateEnabled);
 
     iterator
     begin() const;

src/ripple/protocol/impl/Feature.cpp

@@ -459,6 +459,7 @@ REGISTER_FEATURE(XChainBridge,                  Supported::yes, VoteBehavior::De
 REGISTER_FIX    (fixDisallowIncomingV1,         Supported::yes, VoteBehavior::DefaultNo);
 REGISTER_FEATURE(DID,                           Supported::yes, VoteBehavior::DefaultNo);
 REGISTER_FIX(fixFillOrKill,                     Supported::yes, VoteBehavior::DefaultNo);
+REGISTER_FIX(fixInnerObjTemplate,               Supported::yes, VoteBehavior::DefaultNo);
 
 // The following amendments are obsolete, but must remain supported
 // because they could potentially get enabled.

src/ripple/protocol/impl/STObject.cpp

@@ -35,6 +35,18 @@ STObject::STObject(SField const& name) : STBase(name), mType(nullptr)
 {
 }
 
+STObject::STObject(SField const& name, bool fixInnerObjTemplateEnabled)
+    : STBase(name), mType(nullptr)
+{
+    if (fixInnerObjTemplateEnabled)
+    {
+        SOTemplate const* elements =
+            InnerObjectFormats::getInstance().findSOTemplateBySField(name);
+        if (elements)
+            set(*elements);
+    }
+}
+
 STObject::STObject(SOTemplate const& type, SField const& name) : STBase(name)
 {
     set(type);
@@ -629,6 +641,12 @@ STObject::getFieldArray(SField const& field) const
 
 void
 STObject::set(std::unique_ptr<STBase> v)
+{
+    set(v.get());
+}
+
+void
+STObject::set(STBase* v)
 {
     auto const i = getFieldIndex(v->getFName());
     if (i != -1)

src/test/app/AMM_test.cpp

@@ -4789,6 +4789,110 @@ struct AMM_test : public jtx::AMMTest
         }
     }
 
+    void
+    testFixDefaultInnerObj()
+    {
+        using namespace jtx;
+        FeatureBitset const all{supported_amendments()};
+
+        auto test = [&](FeatureBitset features,
+                        TER const& err1,
+                        TER const& err2,
+                        TER const& err3,
+                        TER const& err4,
+                        std::uint16_t tfee,
+                        bool closeLedger,
+                        std::optional<std::uint16_t> extra = std::nullopt) {
+            Env env(*this, features);
+            fund(env, gw, {alice}, XRP(1'000), {USD(10)});
+            AMM amm(
+                env,
+                gw,
+                XRP(10),
+                USD(10),
+                {.tfee = tfee, .close = closeLedger});
+            amm.deposit(alice, USD(10), XRP(10));
+            amm.vote(
+                alice,
+                tfee,
+                std::nullopt,
+                std::nullopt,
+                std::nullopt,
+                ter(err1));
+            amm.withdraw(gw, USD(1), std::nullopt, std::nullopt, ter(err2));
+            // with the amendment disabled and ledger not closed,
+            // second vote succeeds if the first vote sets the trading fee
+            // to non-zero; if the first vote sets the trading fee to >0 && <9
+            // then the second withdraw succeeds if the second vote sets
+            // the trading fee so that the discounted fee is non-zero
+            amm.vote(
+                alice, 20, std::nullopt, std::nullopt, std::nullopt, ter(err3));
+            amm.withdraw(gw, USD(2), std::nullopt, std::nullopt, ter(err4));
+        };
+
+        // ledger is closed after each transaction, vote/withdraw don't fail
+        // regardless whether the amendment is enabled or not
+        test(all, tesSUCCESS, tesSUCCESS, tesSUCCESS, tesSUCCESS, 0, true);
+        test(
+            all - fixInnerObjTemplate,
+            tesSUCCESS,
+            tesSUCCESS,
+            tesSUCCESS,
+            tesSUCCESS,
+            0,
+            true);
+        // ledger is not closed after each transaction
+        // vote/withdraw don't fail if the amendment is enabled
+        test(all, tesSUCCESS, tesSUCCESS, tesSUCCESS, tesSUCCESS, 0, false);
+        // vote/withdraw fail if the amendment is not enabled
+        // second vote/withdraw still fail: second vote fails because
+        // the initial trading fee is 0, consequently second withdraw fails
+        // because the second vote fails
+        test(
+            all - fixInnerObjTemplate,
+            tefEXCEPTION,
+            tefEXCEPTION,
+            tefEXCEPTION,
+            tefEXCEPTION,
+            0,
+            false);
+        // if non-zero trading/discounted fee then vote/withdraw
+        // don't fail whether the ledger is closed or not and
+        // the amendment is enabled or not
+        test(all, tesSUCCESS, tesSUCCESS, tesSUCCESS, tesSUCCESS, 10, true);
+        test(
+            all - fixInnerObjTemplate,
+            tesSUCCESS,
+            tesSUCCESS,
+            tesSUCCESS,
+            tesSUCCESS,
+            10,
+            true);
+        test(all, tesSUCCESS, tesSUCCESS, tesSUCCESS, tesSUCCESS, 10, false);
+        test(
+            all - fixInnerObjTemplate,
+            tesSUCCESS,
+            tesSUCCESS,
+            tesSUCCESS,
+            tesSUCCESS,
+            10,
+            false);
+        // non-zero trading fee but discounted fee is 0, vote doesn't fail
+        // but withdraw fails
+        test(all, tesSUCCESS, tesSUCCESS, tesSUCCESS, tesSUCCESS, 9, false);
+        // second vote sets the trading fee to non-zero, consequently
+        // second withdraw doesn't fail even if the amendment is not
+        // enabled and the ledger is not closed
+        test(
+            all - fixInnerObjTemplate,
+            tesSUCCESS,
+            tefEXCEPTION,
+            tesSUCCESS,
+            tesSUCCESS,
+            9,
+            false);
+    }
+
     void
     testCore()
     {
@@ -4815,6 +4919,7 @@ struct AMM_test : public jtx::AMMTest
         testClawback();
         testAMMID();
         testSelection();
+        testFixDefaultInnerObj();
     }
 
     void

src/test/jtx/AMM.h

@@ -57,6 +57,18 @@ class LPToken
     }
 };
 
+struct CreateArg
+{
+    bool log = false;
+    std::uint16_t tfee = 0;
+    std::uint32_t fee = 0;
+    std::optional<std::uint32_t> flags = std::nullopt;
+    std::optional<jtx::seq> seq = std::nullopt;
+    std::optional<jtx::msig> ms = std::nullopt;
+    std::optional<ter> err = std::nullopt;
+    bool close = true;
+};
+
 /** Convenience class to test AMM functionality.
  */
 class AMM
@@ -91,13 +103,20 @@ class AMM
         std::optional<std::uint32_t> flags = std::nullopt,
         std::optional<jtx::seq> seq = std::nullopt,
         std::optional<jtx::msig> ms = std::nullopt,
-        std::optional<ter> const& ter = std::nullopt);
+        std::optional<ter> const& ter = std::nullopt,
+        bool close = true);
     AMM(Env& env,
         Account const& account,
         STAmount const& asset1,
         STAmount const& asset2,
         ter const& ter,
-        bool log = false);
+        bool log = false,
+        bool close = true);
+    AMM(Env& env,
+        Account const& account,
+        STAmount const& asset1,
+        STAmount const& asset2,
+        CreateArg const& arg);
 
     /** Send amm_info RPC command
      */
@@ -200,14 +219,15 @@ class AMM
     IOUAmount
     withdrawAll(
         std::optional<Account> const& account,
-        std::optional<STAmount> const& asset1OutDetails = std::nullopt)
+        std::optional<STAmount> const& asset1OutDetails = std::nullopt,
+        std::optional<ter> const& ter = std::nullopt)
     {
         return withdraw(
             account,
             std::nullopt,
             asset1OutDetails,
             asset1OutDetails ? tfOneAssetWithdrawAll : tfWithdrawAll,
-            std::nullopt);
+            ter);
     }
 
     IOUAmount

src/test/jtx/impl/AMM.cpp

@@ -57,15 +57,16 @@ AMM::AMM(
     std::optional<std::uint32_t> flags,
     std::optional<jtx::seq> seq,
     std::optional<jtx::msig> ms,
-    std::optional<ter> const& ter)
+    std::optional<ter> const& ter,
+    bool close)
     : env_(env)
     , creatorAccount_(account)
     , asset1_(asset1)
     , asset2_(asset2)
     , ammID_(keylet::amm(asset1_.issue(), asset2_.issue()).key)
     , initialLPTokens_(initialTokens(asset1, asset2))
     , log_(log)
-    , doClose_(true)
+    , doClose_(close)
     , lastPurchasePrice_(0)
     , bidMin_()
     , bidMax_()
@@ -85,7 +86,8 @@ AMM::AMM(
     STAmount const& asset1,
     STAmount const& asset2,
     ter const& ter,
-    bool log)
+    bool log,
+    bool close)
     : AMM(env,
           account,
           asset1,
@@ -96,7 +98,29 @@ AMM::AMM(
           std::nullopt,
           std::nullopt,
           std::nullopt,
-          ter)
+          ter,
+          close)
+{
+}
+
+AMM::AMM(
+    Env& env,
+    Account const& account,
+    STAmount const& asset1,
+    STAmount const& asset2,
+    CreateArg const& arg)
+    : AMM(env,
+          account,
+          asset1,
+          asset2,
+          arg.log,
+          arg.tfee,
+          arg.fee,
+          arg.flags,
+          arg.seq,
+          arg.ms,
+          arg.err,
+          arg.close)
 {
 }