From a4ee2b8b13124f780444cfacaffbbdb04050082f Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Mon, 29 Jan 2024 11:14:38 -0500 Subject: [PATCH 01/10] Add STObject constr option to explicitly set inner obj template --- src/ripple/app/misc/impl/AMMUtils.cpp | 6 +- src/ripple/app/tx/impl/AMMVote.cpp | 5 +- src/ripple/protocol/Feature.h | 3 +- src/ripple/protocol/SOTemplate.h | 5 ++ src/ripple/protocol/STObject.h | 1 + src/ripple/protocol/impl/Feature.cpp | 1 + src/ripple/protocol/impl/STObject.cpp | 18 +++++ src/test/app/AMM_test.cpp | 105 ++++++++++++++++++++++++++ src/test/jtx/AMM.h | 28 ++++++- src/test/jtx/impl/AMM.cpp | 32 +++++++- 10 files changed, 191 insertions(+), 13 deletions(-) diff --git a/src/ripple/app/misc/impl/AMMUtils.cpp b/src/ripple/app/misc/impl/AMMUtils.cpp index 1d787dbe4ca..b6a43c66d2e 100644 --- a/src/ripple/app/misc/impl/AMMUtils.cpp +++ b/src/ripple/app/misc/impl/AMMUtils.cpp @@ -287,9 +287,10 @@ 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); @@ -297,7 +298,7 @@ initializeFeeAuctionVote( 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( @@ -315,6 +316,7 @@ initializeFeeAuctionVote( auctionSlot.setFieldU16(sfDiscountedFee, dfee); else if (auctionSlot.isFieldPresent(sfDiscountedFee)) auctionSlot.makeFieldAbsent(sfDiscountedFee); + ammSle->set(&auctionSlot); } } // namespace ripple diff --git a/src/ripple/app/tx/impl/AMMVote.cpp b/src/ripple/app/tx/impl/AMMVote.cpp index ff0598aaa40..9888a7feeb0 100644 --- a/src/ripple/app/tx/impl/AMMVote.cpp +++ b/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 const& minPos = std::nullopt) { - STObject newEntry{sfVoteEntry}; + STObject newEntry{sfVoteEntry, fixInnerObj}; if (feeNew != 0) newEntry.setFieldU16(sfTradingFee, feeNew); newEntry.setFieldU32( diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index 3bdfcb15c59..5105bd62367 100644 --- a/src/ripple/protocol/Feature.h +++ b/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 diff --git a/src/ripple/protocol/SOTemplate.h b/src/ripple/protocol/SOTemplate.h index 56dcf4492f7..5ab69282f96 100644 --- a/src/ripple/protocol/SOTemplate.h +++ b/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 }; //------------------------------------------------------------------------------ diff --git a/src/ripple/protocol/STObject.h b/src/ripple/protocol/STObject.h index 3e3862bf6c8..24eadbf51fc 100644 --- a/src/ripple/protocol/STObject.h +++ b/src/ripple/protocol/STObject.h @@ -101,6 +101,7 @@ class STObject : public STBase, public CountedObject 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; diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index 25033d4336e..e87c43fe7e0 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/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. diff --git a/src/ripple/protocol/impl/STObject.cpp b/src/ripple/protocol/impl/STObject.cpp index 5bafbcfce54..b054faf45c4 100644 --- a/src/ripple/protocol/impl/STObject.cpp +++ b/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 v) +{ + set(v.get()); +} + +void +STObject::set(STBase* v) { auto const i = getFieldIndex(v->getFName()); if (i != -1) diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index b0e5106f9eb..af057ef40cb 100644 --- a/src/test/app/AMM_test.cpp +++ b/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 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 diff --git a/src/test/jtx/AMM.h b/src/test/jtx/AMM.h index c7c6f3b8477..156e4a87726 100644 --- a/src/test/jtx/AMM.h +++ b/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 flags = std::nullopt; + std::optional seq = std::nullopt; + std::optional ms = std::nullopt; + std::optional ter = std::nullopt; + bool close = true; +}; + /** Convenience class to test AMM functionality. */ class AMM @@ -91,13 +103,20 @@ class AMM std::optional flags = std::nullopt, std::optional seq = std::nullopt, std::optional ms = std::nullopt, - std::optional const& ter = std::nullopt); + std::optional 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 const& account, - std::optional const& asset1OutDetails = std::nullopt) + std::optional const& asset1OutDetails = std::nullopt, + std::optional const& ter = std::nullopt) { return withdraw( account, std::nullopt, asset1OutDetails, asset1OutDetails ? tfOneAssetWithdrawAll : tfWithdrawAll, - std::nullopt); + ter); } IOUAmount diff --git a/src/test/jtx/impl/AMM.cpp b/src/test/jtx/impl/AMM.cpp index dee1cb1bf5b..109e8fec56f 100644 --- a/src/test/jtx/impl/AMM.cpp +++ b/src/test/jtx/impl/AMM.cpp @@ -57,7 +57,8 @@ AMM::AMM( std::optional flags, std::optional seq, std::optional ms, - std::optional const& ter) + std::optional const& ter, + bool close) : env_(env) , creatorAccount_(account) , asset1_(asset1) @@ -65,7 +66,7 @@ AMM::AMM( , 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.ter, + arg.close) { } From 4a9af0ab33be4cb900f0d6586acb07a76241c1e9 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Mon, 29 Jan 2024 19:13:58 -0500 Subject: [PATCH 02/10] [FOLD] Fix linux compile --- src/test/jtx/AMM.h | 2 +- src/test/jtx/impl/AMM.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/jtx/AMM.h b/src/test/jtx/AMM.h index 156e4a87726..34554ef2710 100644 --- a/src/test/jtx/AMM.h +++ b/src/test/jtx/AMM.h @@ -65,7 +65,7 @@ struct CreateArg std::optional flags = std::nullopt; std::optional seq = std::nullopt; std::optional ms = std::nullopt; - std::optional ter = std::nullopt; + std::optional err = std::nullopt; bool close = true; }; diff --git a/src/test/jtx/impl/AMM.cpp b/src/test/jtx/impl/AMM.cpp index 109e8fec56f..760ba7ecf17 100644 --- a/src/test/jtx/impl/AMM.cpp +++ b/src/test/jtx/impl/AMM.cpp @@ -119,7 +119,7 @@ AMM::AMM( arg.flags, arg.seq, arg.ms, - arg.ter, + arg.err, arg.close) { } From 51fc0028f2d6ce94b9ad754311ca81c2a016f9f9 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Thu, 1 Feb 2024 09:53:52 -0500 Subject: [PATCH 03/10] [FOLD] Construct inner objects with STObject::makeInnerObject() --- src/ripple/app/misc/impl/AMMUtils.cpp | 6 +++--- src/ripple/app/tx/impl/AMMVote.cpp | 6 +++--- src/ripple/protocol/SOTemplate.h | 7 ++----- src/ripple/protocol/STObject.h | 5 ++++- src/ripple/protocol/impl/STObject.cpp | 28 +++++++++++++++------------ 5 files changed, 28 insertions(+), 24 deletions(-) diff --git a/src/ripple/app/misc/impl/AMMUtils.cpp b/src/ripple/app/misc/impl/AMMUtils.cpp index b6a43c66d2e..807ca071eec 100644 --- a/src/ripple/app/misc/impl/AMMUtils.cpp +++ b/src/ripple/app/misc/impl/AMMUtils.cpp @@ -287,10 +287,10 @@ initializeFeeAuctionVote( Issue const& lptIssue, std::uint16_t tfee) { - bool fixInnerObj(view.rules().enabled(fixInnerObjTemplate)); + auto const& rules = view.rules(); // AMM creator gets the voting slot. STArray voteSlots; - STObject voteEntry{sfVoteEntry, fixInnerObj}; + STObject voteEntry = STObject::makeInnerObject(sfVoteEntry, rules); if (tfee != 0) voteEntry.setFieldU16(sfTradingFee, tfee); voteEntry.setFieldU32(sfVoteWeight, VOTE_WEIGHT_SCALE_FACTOR); @@ -298,7 +298,7 @@ initializeFeeAuctionVote( voteSlots.push_back(voteEntry); ammSle->setFieldArray(sfVoteSlots, voteSlots); // AMM creator gets the auction slot for free. - STObject auctionSlot{sfAuctionSlot, fixInnerObj}; + STObject auctionSlot = STObject::makeInnerObject(sfAuctionSlot, rules); auctionSlot.setAccountID(sfAccount, account); // current + sec in 24h auto const expiration = std::chrono::duration_cast( diff --git a/src/ripple/app/tx/impl/AMMVote.cpp b/src/ripple/app/tx/impl/AMMVote.cpp index 9888a7feeb0..d55af6ec482 100644 --- a/src/ripple/app/tx/impl/AMMVote.cpp +++ b/src/ripple/app/tx/impl/AMMVote.cpp @@ -104,7 +104,7 @@ applyVote( Number den{0}; // Account already has vote entry bool foundAccount = false; - bool const fixInnerObj{ctx_.view().rules().enabled(fixInnerObjTemplate)}; + auto const& rules = ctx_.view().rules(); // 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 @@ -120,7 +120,7 @@ applyVote( continue; } auto feeVal = entry[sfTradingFee]; - STObject newEntry{sfVoteEntry, fixInnerObj}; + STObject newEntry = STObject::makeInnerObject(sfVoteEntry, rules); // The account already has the vote entry. if (account == account_) { @@ -159,7 +159,7 @@ applyVote( { auto update = [&](std::optional const& minPos = std::nullopt) { - STObject newEntry{sfVoteEntry, fixInnerObj}; + STObject newEntry = STObject::makeInnerObject(sfVoteEntry, rules); if (feeNew != 0) newEntry.setFieldU16(sfTradingFee, feeNew); newEntry.setFieldU32( diff --git a/src/ripple/protocol/SOTemplate.h b/src/ripple/protocol/SOTemplate.h index 5ab69282f96..609c2d2c80b 100644 --- a/src/ripple/protocol/SOTemplate.h +++ b/src/ripple/protocol/SOTemplate.h @@ -35,11 +35,8 @@ 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 + // inner object with the default fields has to be + // constructed with STObject::makeInnerObject() }; //------------------------------------------------------------------------------ diff --git a/src/ripple/protocol/STObject.h b/src/ripple/protocol/STObject.h index 24eadbf51fc..4bf1786cdfb 100644 --- a/src/ripple/protocol/STObject.h +++ b/src/ripple/protocol/STObject.h @@ -43,6 +43,7 @@ namespace ripple { class STArray; +class Rules; inline void throwFieldNotFound(SField const& field) @@ -101,7 +102,9 @@ class STObject : public STBase, public CountedObject 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); + + static STObject + makeInnerObject(SField const& name, Rules const& rules); iterator begin() const; diff --git a/src/ripple/protocol/impl/STObject.cpp b/src/ripple/protocol/impl/STObject.cpp index b054faf45c4..f31f2826a6d 100644 --- a/src/ripple/protocol/impl/STObject.cpp +++ b/src/ripple/protocol/impl/STObject.cpp @@ -19,6 +19,8 @@ #include #include +#include +#include #include #include #include @@ -35,18 +37,6 @@ 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); @@ -69,6 +59,20 @@ STObject::STObject(SerialIter& sit, SField const& name, int depth) noexcept( set(sit, depth); } +STObject +STObject::makeInnerObject(SField const& name, Rules const& rules) +{ + STObject obj{name}; + if (rules.enabled(fixInnerObjTemplate)) + { + SOTemplate const* elements = + InnerObjectFormats::getInstance().findSOTemplateBySField(name); + if (elements) + obj.set(*elements); + } + return obj; +} + STBase* STObject::copy(std::size_t n, void* buf) const { From 51eabc490378334e44fb933ffc752c0d95105051 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Thu, 1 Feb 2024 09:56:54 -0500 Subject: [PATCH 04/10] [FOLD] Fix clang format --- src/ripple/protocol/impl/STObject.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ripple/protocol/impl/STObject.cpp b/src/ripple/protocol/impl/STObject.cpp index f31f2826a6d..3719b1544d8 100644 --- a/src/ripple/protocol/impl/STObject.cpp +++ b/src/ripple/protocol/impl/STObject.cpp @@ -18,8 +18,8 @@ //============================================================================== #include -#include #include +#include #include #include #include From 292772c5c1868e789d33b25adc7c3963d3c4cca1 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Thu, 1 Feb 2024 13:47:39 -0500 Subject: [PATCH 05/10] [FOLD] Address reviewer's feedback * Add comment to InnerObjectFormats * Use if initializer in makeInnerObject() * Add assert to STObject::set() --- src/ripple/protocol/impl/InnerObjectFormats.cpp | 3 +++ src/ripple/protocol/impl/STObject.cpp | 6 +++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/ripple/protocol/impl/InnerObjectFormats.cpp b/src/ripple/protocol/impl/InnerObjectFormats.cpp index 58f4392f536..4350ea180d2 100644 --- a/src/ripple/protocol/impl/InnerObjectFormats.cpp +++ b/src/ripple/protocol/impl/InnerObjectFormats.cpp @@ -25,6 +25,9 @@ namespace ripple { InnerObjectFormats::InnerObjectFormats() { + // inner objects with the default fields have to be + // constructed with STObject::makeInnerObject() + add(sfSignerEntry.jsonName.c_str(), sfSignerEntry.getCode(), { diff --git a/src/ripple/protocol/impl/STObject.cpp b/src/ripple/protocol/impl/STObject.cpp index 3719b1544d8..1778908c489 100644 --- a/src/ripple/protocol/impl/STObject.cpp +++ b/src/ripple/protocol/impl/STObject.cpp @@ -65,9 +65,8 @@ STObject::makeInnerObject(SField const& name, Rules const& rules) STObject obj{name}; if (rules.enabled(fixInnerObjTemplate)) { - SOTemplate const* elements = - InnerObjectFormats::getInstance().findSOTemplateBySField(name); - if (elements) + if (SOTemplate const* elements = + InnerObjectFormats::getInstance().findSOTemplateBySField(name)) obj.set(*elements); } return obj; @@ -652,6 +651,7 @@ STObject::set(std::unique_ptr v) void STObject::set(STBase* v) { + assert(v); auto const i = getFieldIndex(v->getFName()); if (i != -1) { From 034b98e500e48fad1bd8a35f82d0d9338d3f11a6 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Fri, 2 Feb 2024 10:40:22 -0500 Subject: [PATCH 06/10] [FOLD] Handle AuthAccounts on empty state deposit; update unit-tests --- src/ripple/app/misc/impl/AMMUtils.cpp | 11 ++++ src/test/app/AMM_test.cpp | 73 +++++++++++++++++++++++---- src/test/jtx/AMM.h | 61 ++++++++++++++++++++++ src/test/jtx/impl/AMM.cpp | 55 +++++++++++++++++++- 4 files changed, 188 insertions(+), 12 deletions(-) diff --git a/src/ripple/app/misc/impl/AMMUtils.cpp b/src/ripple/app/misc/impl/AMMUtils.cpp index 807ca071eec..0c5752bda03 100644 --- a/src/ripple/app/misc/impl/AMMUtils.cpp +++ b/src/ripple/app/misc/impl/AMMUtils.cpp @@ -299,6 +299,17 @@ initializeFeeAuctionVote( ammSle->setFieldArray(sfVoteSlots, voteSlots); // AMM creator gets the auction slot for free. STObject auctionSlot = STObject::makeInnerObject(sfAuctionSlot, rules); + if (!rules.enabled(fixInnerObjTemplate) && + ammSle->isFieldPresent(sfAuctionSlot)) + { + auto const& origAuctionSlot = ammSle->peekFieldObject(sfAuctionSlot); + // this is executed on deposit from empty state. + // pre-amendment code re-uses AuthAccounts + // post-amendment code resets AuthAccounts + if (origAuctionSlot.isFieldPresent(sfAuthAccounts)) + auctionSlot.setFieldArray( + sfAuthAccounts, origAuctionSlot.getFieldArray(sfAuthAccounts)); + } auctionSlot.setAccountID(sfAccount, account); // current + sec in 24h auto const expiration = std::chrono::duration_cast( diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index af057ef40cb..90fa7134564 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -4792,6 +4792,7 @@ struct AMM_test : public jtx::AMMTest void testFixDefaultInnerObj() { + testcase("Fix Default Inner Object"); using namespace jtx; FeatureBitset const all{supported_amendments()}; @@ -4812,22 +4813,17 @@ struct AMM_test : public jtx::AMMTest 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)); + amm.vote(VoteArg{.account = alice, .tfee = tfee, .err = ter(err1)}); + amm.withdraw(WithdrawArg{ + .account = gw, .asset1Out = USD(1), .err = 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)); + amm.vote(VoteArg{.account = alice, .tfee = 20, .err = ter(err3)}); + amm.withdraw(WithdrawArg{ + .account = gw, .asset1Out = USD(2), .err = ter(err4)}); }; // ledger is closed after each transaction, vote/withdraw don't fail @@ -4891,6 +4887,61 @@ struct AMM_test : public jtx::AMMTest tesSUCCESS, 9, false); + + auto testDepositOnEmpty = [&](FeatureBitset features, TER const& err) { + Env env( + *this, + envconfig([](std::unique_ptr cfg) { + cfg->FEES.reference_fee = XRPAmount(1); + return cfg; + }), + features); + fund(env, gw, {alice, bob}, XRP(20'000), {USD(10'000)}); + AMM amm(env, gw, XRP(10'000), USD(10'000)); + for (auto i = 0; i < maxDeletableAMMTrustLines + 10; ++i) + { + Account const a{std::to_string(i)}; + env.fund(XRP(1'000), a); + env(trust(a, STAmount{amm.lptIssue(), 10'000})); + env.close(); + } + amm.bid(BidArg{.authAccounts = {bob, alice}}); + // The trustlines are partially deleted, + // AMM is set to an empty state. + amm.withdrawAll(gw); + amm.setClose(false); + amm.deposit(DepositArg{ + .account = alice, + .asset1In = XRP(10'000), + .asset2In = USD(10'000), + .flags = tfTwoAssetIfEmpty, + .tfee = 0}); + // AuthAccounts is reset post-amendment + if (features[fixInnerObjTemplate]) + BEAST_EXPECT(amm.expectAuctionSlot({})); + // AuthAccounts is re-used pre-amendment + else + BEAST_EXPECT(amm.expectAuctionSlot({bob, alice})); + // repeat some fail/success scenarios + amm.vote(VoteArg{.account = alice, .tfee = 0, .err = ter(err)}); + amm.withdraw(WithdrawArg{ + .account = alice, .asset1Out = USD(1), .err = ter(err)}); + amm.vote(VoteArg{.account = alice, .tfee = 20, .err = ter(err)}); + amm.withdraw(WithdrawArg{ + .account = alice, .asset1Out = USD(2), .err = ter(err)}); + env.close(); + amm.vote(VoteArg{.account = alice, .tfee = 0}); + amm.vote(VoteArg{.account = alice, .tfee = 0, .err = ter(err)}); + amm.withdraw(WithdrawArg{.account = alice, .asset1Out = USD(3)}); + amm.deposit(DepositArg{ + .account = bob, .asset1In = XRP(100), .asset2In = USD(100)}); + amm.bid(BidArg{.account = bob}); + amm.withdraw(WithdrawArg{.account = bob, .asset1Out = USD(4)}); + amm.vote(VoteArg{.account = bob, .tfee = 10, .err = ter(err)}); + }; + + testDepositOnEmpty(all, tesSUCCESS); + testDepositOnEmpty(all - fixInnerObjTemplate, tefEXCEPTION); } void diff --git a/src/test/jtx/AMM.h b/src/test/jtx/AMM.h index 34554ef2710..4c6e8d78a4e 100644 --- a/src/test/jtx/AMM.h +++ b/src/test/jtx/AMM.h @@ -69,6 +69,55 @@ struct CreateArg bool close = true; }; +struct DepositArg +{ + std::optional account = std::nullopt; + std::optional tokens = std::nullopt; + std::optional asset1In = std::nullopt; + std::optional asset2In = std::nullopt; + std::optional maxEP = std::nullopt; + std::optional flags = std::nullopt; + std::optional> assets = std::nullopt; + std::optional seq = std::nullopt; + std::optional tfee = std::nullopt; + std::optional err = std::nullopt; +}; + +struct WithdrawArg +{ + std::optional account = std::nullopt; + std::optional tokens = std::nullopt; + std::optional asset1Out = std::nullopt; + std::optional asset2Out = std::nullopt; + std::optional maxEP = std::nullopt; + std::optional flags = std::nullopt; + std::optional> assets = std::nullopt; + std::optional seq = std::nullopt; + std::optional err = std::nullopt; +}; + +struct VoteArg +{ + std::optional account = std::nullopt; + std::uint32_t tfee = 0; + std::optional flags = std::nullopt; + std::optional seq = std::nullopt; + std::optional> assets = std::nullopt; + std::optional err = std::nullopt; +}; + +struct BidArg +{ + std::optional account = std::nullopt; + std::optional> bidMin = std::nullopt; + std::optional> bidMax = std::nullopt; + std::vector authAccounts = {}; + std::optional flags = std::nullopt; + std::optional seq = std::nullopt; + std::optional> assets = std::nullopt; + std::optional err = std::nullopt; +}; + /** Convenience class to test AMM functionality. */ class AMM @@ -208,6 +257,9 @@ class AMM std::optional const& tfee = std::nullopt, std::optional const& ter = std::nullopt); + IOUAmount + deposit(DepositArg const& arg); + IOUAmount withdraw( std::optional const& account, @@ -250,6 +302,9 @@ class AMM std::optional const& seq, std::optional const& ter = std::nullopt); + IOUAmount + withdraw(WithdrawArg const& arg); + void vote( std::optional const& account, @@ -259,6 +314,9 @@ class AMM std::optional> const& assets = std::nullopt, std::optional const& ter = std::nullopt); + void + vote(VoteArg const& arg); + void bid(std::optional const& account, std::optional> const& bidMin = @@ -271,6 +329,9 @@ class AMM std::optional> const& assets = std::nullopt, std::optional const& ter = std::nullopt); + void + bid(BidArg const& arg); + AccountID const& ammAccount() const { diff --git a/src/test/jtx/impl/AMM.cpp b/src/test/jtx/impl/AMM.cpp index 760ba7ecf17..26ec26b7b5d 100644 --- a/src/test/jtx/impl/AMM.cpp +++ b/src/test/jtx/impl/AMM.cpp @@ -494,6 +494,22 @@ AMM::deposit( return deposit(account, jv, assets, seq, ter); } +IOUAmount +AMM::deposit(DepositArg const& arg) +{ + return deposit( + arg.account, + arg.tokens, + arg.asset1In, + arg.asset2In, + arg.maxEP, + arg.flags, + arg.assets, + arg.seq, + arg.tfee, + arg.err); +} + IOUAmount AMM::withdraw( std::optional const& account, @@ -598,6 +614,21 @@ AMM::withdraw( return withdraw(account, jv, seq, assets, ter); } +IOUAmount +AMM::withdraw(WithdrawArg const& arg) +{ + return withdraw( + arg.account, + arg.tokens, + arg.asset1Out, + arg.asset2Out, + arg.maxEP, + arg.flags, + arg.assets, + arg.seq, + arg.err); +} + void AMM::vote( std::optional const& account, @@ -619,6 +650,12 @@ AMM::vote( submit(jv, seq, ter); } +void +AMM::vote(VoteArg const& arg) +{ + return vote(arg.account, arg.tfee, arg.flags, arg.seq, arg.assets, arg.err); +} + void AMM::bid( std::optional const& account, @@ -687,6 +724,20 @@ AMM::bid( submit(jv, seq, ter); } +void +AMM::bid(BidArg const& arg) +{ + return bid( + arg.account, + arg.bidMin, + arg.bidMax, + arg.authAccounts, + arg.flags, + arg.seq, + arg.assets, + arg.err); +} + void AMM::submit( Json::Value const& jv, @@ -729,7 +780,9 @@ AMM::expectAuctionSlot(auto&& cb) const static_cast(amm->peekAtField(sfAuctionSlot)); if (auctionSlot.isFieldPresent(sfAccount)) { - auto const slotFee = auctionSlot[sfDiscountedFee]; + // this could fail in pre-fixInnerObjTemplate test if one + // the fail-cases. access as optional to avoid the failure + auto const slotFee = auctionSlot[~sfDiscountedFee].value_or(0); auto const slotInterval = ammAuctionTimeSlot( env_.app().timeKeeper().now().time_since_epoch().count(), auctionSlot); From 87a287986892d2219b6683ae40b5b56bc8043817 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Mon, 5 Feb 2024 19:31:26 -0500 Subject: [PATCH 07/10] [FOLD] Revert AuthAccounts change and other fixes * Create AuctionSlot on AMMCreate and update on AMMDeposit * Add assert to check for AuctionSlot --- src/ripple/app/misc/impl/AMMUtils.cpp | 21 +++++----- src/ripple/app/tx/impl/AMMBid.cpp | 10 ++++- src/ripple/app/tx/impl/AMMVote.cpp | 5 +++ src/ripple/rpc/handlers/AMMInfo.cpp | 4 ++ src/test/app/AMM_test.cpp | 55 --------------------------- src/test/jtx/impl/AMM.cpp | 37 +++++++++++------- 6 files changed, 51 insertions(+), 81 deletions(-) diff --git a/src/ripple/app/misc/impl/AMMUtils.cpp b/src/ripple/app/misc/impl/AMMUtils.cpp index 0c5752bda03..4f6971896ea 100644 --- a/src/ripple/app/misc/impl/AMMUtils.cpp +++ b/src/ripple/app/misc/impl/AMMUtils.cpp @@ -138,6 +138,10 @@ std::uint16_t getTradingFee(ReadView const& view, SLE const& ammSle, AccountID const& account) { using namespace std::chrono; + // should not happen + assert( + !view.rules().enabled(fixInnerObjTemplate) || + ammSle.isFieldPresent(sfAuctionSlot)); if (ammSle.isFieldPresent(sfAuctionSlot)) { auto const& auctionSlot = @@ -298,18 +302,14 @@ initializeFeeAuctionVote( voteSlots.push_back(voteEntry); ammSle->setFieldArray(sfVoteSlots, voteSlots); // AMM creator gets the auction slot for free. - STObject auctionSlot = STObject::makeInnerObject(sfAuctionSlot, rules); - if (!rules.enabled(fixInnerObjTemplate) && - ammSle->isFieldPresent(sfAuctionSlot)) + // AuctionSlot is created on AMMCreate and updated on AMMDeposit + // when AMM is in an empty state + if (!ammSle->isFieldPresent(sfAuctionSlot)) { - auto const& origAuctionSlot = ammSle->peekFieldObject(sfAuctionSlot); - // this is executed on deposit from empty state. - // pre-amendment code re-uses AuthAccounts - // post-amendment code resets AuthAccounts - if (origAuctionSlot.isFieldPresent(sfAuthAccounts)) - auctionSlot.setFieldArray( - sfAuthAccounts, origAuctionSlot.getFieldArray(sfAuthAccounts)); + STObject auctionSlot = STObject::makeInnerObject(sfAuctionSlot, rules); + ammSle->set(&auctionSlot); } + STObject& auctionSlot = ammSle->peekFieldObject(sfAuctionSlot); auctionSlot.setAccountID(sfAccount, account); // current + sec in 24h auto const expiration = std::chrono::duration_cast( @@ -327,7 +327,6 @@ initializeFeeAuctionVote( auctionSlot.setFieldU16(sfDiscountedFee, dfee); else if (auctionSlot.isFieldPresent(sfDiscountedFee)) auctionSlot.makeFieldAbsent(sfDiscountedFee); - ammSle->set(&auctionSlot); } } // namespace ripple diff --git a/src/ripple/app/tx/impl/AMMBid.cpp b/src/ripple/app/tx/impl/AMMBid.cpp index 822e72203a6..c8c3c144f90 100644 --- a/src/ripple/app/tx/impl/AMMBid.cpp +++ b/src/ripple/app/tx/impl/AMMBid.cpp @@ -173,8 +173,16 @@ applyBid( return {tecINTERNAL, false}; STAmount const lptAMMBalance = (*ammSle)[sfLPTokenBalance]; auto const lpTokens = ammLPHolds(sb, *ammSle, account_, ctx_.journal); + // should not happen + assert( + !ctx_.view().rules().enabled(fixInnerObjTemplate) || + ammSle->isFieldPresent(sfAuctionSlot)); if (!ammSle->isFieldPresent(sfAuctionSlot)) - ammSle->makeFieldPresent(sfAuctionSlot); + { + STObject auctionSlot = + STObject::makeInnerObject(sfAuctionSlot, ctx_.view().rules()); + ammSle->set(&auctionSlot); + } auto& auctionSlot = ammSle->peekFieldObject(sfAuctionSlot); auto const current = duration_cast( diff --git a/src/ripple/app/tx/impl/AMMVote.cpp b/src/ripple/app/tx/impl/AMMVote.cpp index d55af6ec482..a36413bbc4a 100644 --- a/src/ripple/app/tx/impl/AMMVote.cpp +++ b/src/ripple/app/tx/impl/AMMVote.cpp @@ -200,6 +200,11 @@ applyVote( } } + // should not happen + assert( + !ctx_.view().rules().enabled(fixInnerObjTemplate) || + ammSle->isFieldPresent(sfAuctionSlot)); + // Update the vote entries and the trading/discounted fee. ammSle->setFieldArray(sfVoteSlots, updatedVoteSlots); if (auto const fee = static_cast(num / den)) diff --git a/src/ripple/rpc/handlers/AMMInfo.cpp b/src/ripple/rpc/handlers/AMMInfo.cpp index a1be636cafd..12b3dbd407d 100644 --- a/src/ripple/rpc/handlers/AMMInfo.cpp +++ b/src/ripple/rpc/handlers/AMMInfo.cpp @@ -200,6 +200,10 @@ doAMMInfo(RPC::JsonContext& context) } if (voteSlots.size() > 0) ammResult[jss::vote_slots] = std::move(voteSlots); + // should not happen + assert( + !ledger->rules().enabled(fixInnerObjTemplate) || + amm->isFieldPresent(sfAuctionSlot)); if (amm->isFieldPresent(sfAuctionSlot)) { auto const& auctionSlot = diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 90fa7134564..b6828fab773 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -4887,61 +4887,6 @@ struct AMM_test : public jtx::AMMTest tesSUCCESS, 9, false); - - auto testDepositOnEmpty = [&](FeatureBitset features, TER const& err) { - Env env( - *this, - envconfig([](std::unique_ptr cfg) { - cfg->FEES.reference_fee = XRPAmount(1); - return cfg; - }), - features); - fund(env, gw, {alice, bob}, XRP(20'000), {USD(10'000)}); - AMM amm(env, gw, XRP(10'000), USD(10'000)); - for (auto i = 0; i < maxDeletableAMMTrustLines + 10; ++i) - { - Account const a{std::to_string(i)}; - env.fund(XRP(1'000), a); - env(trust(a, STAmount{amm.lptIssue(), 10'000})); - env.close(); - } - amm.bid(BidArg{.authAccounts = {bob, alice}}); - // The trustlines are partially deleted, - // AMM is set to an empty state. - amm.withdrawAll(gw); - amm.setClose(false); - amm.deposit(DepositArg{ - .account = alice, - .asset1In = XRP(10'000), - .asset2In = USD(10'000), - .flags = tfTwoAssetIfEmpty, - .tfee = 0}); - // AuthAccounts is reset post-amendment - if (features[fixInnerObjTemplate]) - BEAST_EXPECT(amm.expectAuctionSlot({})); - // AuthAccounts is re-used pre-amendment - else - BEAST_EXPECT(amm.expectAuctionSlot({bob, alice})); - // repeat some fail/success scenarios - amm.vote(VoteArg{.account = alice, .tfee = 0, .err = ter(err)}); - amm.withdraw(WithdrawArg{ - .account = alice, .asset1Out = USD(1), .err = ter(err)}); - amm.vote(VoteArg{.account = alice, .tfee = 20, .err = ter(err)}); - amm.withdraw(WithdrawArg{ - .account = alice, .asset1Out = USD(2), .err = ter(err)}); - env.close(); - amm.vote(VoteArg{.account = alice, .tfee = 0}); - amm.vote(VoteArg{.account = alice, .tfee = 0, .err = ter(err)}); - amm.withdraw(WithdrawArg{.account = alice, .asset1Out = USD(3)}); - amm.deposit(DepositArg{ - .account = bob, .asset1In = XRP(100), .asset2In = USD(100)}); - amm.bid(BidArg{.account = bob}); - amm.withdraw(WithdrawArg{.account = bob, .asset1Out = USD(4)}); - amm.vote(VoteArg{.account = bob, .tfee = 10, .err = ter(err)}); - }; - - testDepositOnEmpty(all, tesSUCCESS); - testDepositOnEmpty(all - fixInnerObjTemplate, tefEXCEPTION); } void diff --git a/src/test/jtx/impl/AMM.cpp b/src/test/jtx/impl/AMM.cpp index 26ec26b7b5d..5088e51d0b9 100644 --- a/src/test/jtx/impl/AMM.cpp +++ b/src/test/jtx/impl/AMM.cpp @@ -670,6 +670,9 @@ AMM::bid( if (auto const amm = env_.current()->read(keylet::amm(asset1_.issue(), asset2_.issue()))) { + assert( + !env_.current()->rules().enabled(fixInnerObjTemplate) || + amm->isFieldPresent(sfAuctionSlot)); if (amm->isFieldPresent(sfAuctionSlot)) { auto const& auctionSlot = @@ -773,22 +776,28 @@ bool AMM::expectAuctionSlot(auto&& cb) const { if (auto const amm = - env_.current()->read(keylet::amm(asset1_.issue(), asset2_.issue())); - amm && amm->isFieldPresent(sfAuctionSlot)) + env_.current()->read(keylet::amm(asset1_.issue(), asset2_.issue()))) { - auto const& auctionSlot = - static_cast(amm->peekAtField(sfAuctionSlot)); - if (auctionSlot.isFieldPresent(sfAccount)) + assert( + !env_.current()->rules().enabled(fixInnerObjTemplate) || + amm->isFieldPresent(sfAuctionSlot)); + if (amm->isFieldPresent(sfAuctionSlot)) { - // this could fail in pre-fixInnerObjTemplate test if one - // the fail-cases. access as optional to avoid the failure - auto const slotFee = auctionSlot[~sfDiscountedFee].value_or(0); - auto const slotInterval = ammAuctionTimeSlot( - env_.app().timeKeeper().now().time_since_epoch().count(), - auctionSlot); - auto const slotPrice = auctionSlot[sfPrice].iou(); - auto const authAccounts = auctionSlot.getFieldArray(sfAuthAccounts); - return cb(slotFee, slotInterval, slotPrice, authAccounts); + auto const& auctionSlot = + static_cast(amm->peekAtField(sfAuctionSlot)); + if (auctionSlot.isFieldPresent(sfAccount)) + { + // this could fail in pre-fixInnerObjTemplate test if one + // the fail-cases. access as optional to avoid the failure + auto const slotFee = auctionSlot[~sfDiscountedFee].value_or(0); + auto const slotInterval = ammAuctionTimeSlot( + env_.app().timeKeeper().now().time_since_epoch().count(), + auctionSlot); + auto const slotPrice = auctionSlot[sfPrice].iou(); + auto const authAccounts = + auctionSlot.getFieldArray(sfAuthAccounts); + return cb(slotFee, slotInterval, slotPrice, authAccounts); + } } } return false; From 4e6f21045e915cc694b0520170abfbb2ae5d1ecc Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Tue, 6 Feb 2024 19:16:31 -0500 Subject: [PATCH 08/10] [FOLD] Address @seelabs feedback * Remove comments * Refactor amendment changed code --- src/ripple/app/misc/impl/AMMUtils.cpp | 23 ++++++++++++++++------- src/ripple/app/tx/impl/AMMBid.cpp | 18 ++++++++++-------- src/ripple/app/tx/impl/AMMVote.cpp | 1 - src/ripple/rpc/handlers/AMMInfo.cpp | 1 - 4 files changed, 26 insertions(+), 17 deletions(-) diff --git a/src/ripple/app/misc/impl/AMMUtils.cpp b/src/ripple/app/misc/impl/AMMUtils.cpp index 4f6971896ea..2511240bfcd 100644 --- a/src/ripple/app/misc/impl/AMMUtils.cpp +++ b/src/ripple/app/misc/impl/AMMUtils.cpp @@ -138,7 +138,6 @@ std::uint16_t getTradingFee(ReadView const& view, SLE const& ammSle, AccountID const& account) { using namespace std::chrono; - // should not happen assert( !view.rules().enabled(fixInnerObjTemplate) || ammSle.isFieldPresent(sfAuctionSlot)); @@ -304,12 +303,22 @@ initializeFeeAuctionVote( // AMM creator gets the auction slot for free. // AuctionSlot is created on AMMCreate and updated on AMMDeposit // when AMM is in an empty state - if (!ammSle->isFieldPresent(sfAuctionSlot)) - { - STObject auctionSlot = STObject::makeInnerObject(sfAuctionSlot, rules); - ammSle->set(&auctionSlot); - } - STObject& auctionSlot = ammSle->peekFieldObject(sfAuctionSlot); + STObject& auctionSlot = [&]() -> STObject& { + if (!rules.enabled(fixInnerObjTemplate)) + { + return ammSle->peekFieldObject(sfAuctionSlot); + } + else + { + if (!ammSle->isFieldPresent(sfAuctionSlot)) + { + STObject auctionSlot = + STObject::makeInnerObject(sfAuctionSlot, rules); + ammSle->set(&auctionSlot); + } + return ammSle->peekFieldObject(sfAuctionSlot); + } + }(); auctionSlot.setAccountID(sfAccount, account); // current + sec in 24h auto const expiration = std::chrono::duration_cast( diff --git a/src/ripple/app/tx/impl/AMMBid.cpp b/src/ripple/app/tx/impl/AMMBid.cpp index c8c3c144f90..e49c378ceeb 100644 --- a/src/ripple/app/tx/impl/AMMBid.cpp +++ b/src/ripple/app/tx/impl/AMMBid.cpp @@ -173,15 +173,17 @@ applyBid( return {tecINTERNAL, false}; STAmount const lptAMMBalance = (*ammSle)[sfLPTokenBalance]; auto const lpTokens = ammLPHolds(sb, *ammSle, account_, ctx_.journal); - // should not happen - assert( - !ctx_.view().rules().enabled(fixInnerObjTemplate) || - ammSle->isFieldPresent(sfAuctionSlot)); - if (!ammSle->isFieldPresent(sfAuctionSlot)) + auto const& rules = ctx_.view().rules(); + if (!rules.enabled(fixInnerObjTemplate)) { - STObject auctionSlot = - STObject::makeInnerObject(sfAuctionSlot, ctx_.view().rules()); - ammSle->set(&auctionSlot); + if (!ammSle->isFieldPresent(sfAuctionSlot)) + ammSle->makeFieldPresent(sfAuctionSlot); + } + else + { + assert(ammSle->isFieldPresent(sfAuctionSlot)); + if (!ammSle->isFieldPresent(sfAuctionSlot)) + return {tecINTERNAL, false}; } auto& auctionSlot = ammSle->peekFieldObject(sfAuctionSlot); auto const current = diff --git a/src/ripple/app/tx/impl/AMMVote.cpp b/src/ripple/app/tx/impl/AMMVote.cpp index a36413bbc4a..d908a93c383 100644 --- a/src/ripple/app/tx/impl/AMMVote.cpp +++ b/src/ripple/app/tx/impl/AMMVote.cpp @@ -200,7 +200,6 @@ applyVote( } } - // should not happen assert( !ctx_.view().rules().enabled(fixInnerObjTemplate) || ammSle->isFieldPresent(sfAuctionSlot)); diff --git a/src/ripple/rpc/handlers/AMMInfo.cpp b/src/ripple/rpc/handlers/AMMInfo.cpp index 12b3dbd407d..c6711fa7b82 100644 --- a/src/ripple/rpc/handlers/AMMInfo.cpp +++ b/src/ripple/rpc/handlers/AMMInfo.cpp @@ -200,7 +200,6 @@ doAMMInfo(RPC::JsonContext& context) } if (voteSlots.size() > 0) ammResult[jss::vote_slots] = std::move(voteSlots); - // should not happen assert( !ledger->rules().enabled(fixInnerObjTemplate) || amm->isFieldPresent(sfAuctionSlot)); From 0013fde684e167ee967e0d36c87e829c4c5d19c3 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Tue, 6 Feb 2024 21:24:51 -0500 Subject: [PATCH 09/10] [FOLD] Address @seelabs feedback * Change STObject::set() arg to rvalue ref --- src/ripple/app/misc/impl/AMMUtils.cpp | 2 +- src/ripple/protocol/STObject.h | 2 +- src/ripple/protocol/impl/STObject.cpp | 11 +++++------ 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/ripple/app/misc/impl/AMMUtils.cpp b/src/ripple/app/misc/impl/AMMUtils.cpp index 2511240bfcd..0aead02db88 100644 --- a/src/ripple/app/misc/impl/AMMUtils.cpp +++ b/src/ripple/app/misc/impl/AMMUtils.cpp @@ -314,7 +314,7 @@ initializeFeeAuctionVote( { STObject auctionSlot = STObject::makeInnerObject(sfAuctionSlot, rules); - ammSle->set(&auctionSlot); + ammSle->set(std::move(auctionSlot)); } return ammSle->peekFieldObject(sfAuctionSlot); } diff --git a/src/ripple/protocol/STObject.h b/src/ripple/protocol/STObject.h index 4bf1786cdfb..5476cd01198 100644 --- a/src/ripple/protocol/STObject.h +++ b/src/ripple/protocol/STObject.h @@ -343,7 +343,7 @@ class STObject : public STBase, public CountedObject set(std::unique_ptr v); void - set(STBase* v); + set(STBase&& v); void setFieldU8(SField const& field, unsigned char); diff --git a/src/ripple/protocol/impl/STObject.cpp b/src/ripple/protocol/impl/STObject.cpp index 1778908c489..7c546a2568e 100644 --- a/src/ripple/protocol/impl/STObject.cpp +++ b/src/ripple/protocol/impl/STObject.cpp @@ -645,23 +645,22 @@ STObject::getFieldArray(SField const& field) const void STObject::set(std::unique_ptr v) { - set(v.get()); + set(std::move(*v.get())); } void -STObject::set(STBase* v) +STObject::set(STBase&& v) { - assert(v); - auto const i = getFieldIndex(v->getFName()); + auto const i = getFieldIndex(v.getFName()); if (i != -1) { - v_[i] = std::move(*v); + v_[i] = std::move(v); } else { if (!isFree()) Throw("missing field in templated STObject"); - v_.emplace_back(std::move(*v)); + v_.emplace_back(std::move(v)); } } From 98d0a1964df14a10a7f8523721aceab4d8a06c2f Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Wed, 7 Feb 2024 13:04:15 -0500 Subject: [PATCH 10/10] [FOLD] Address @ximinez feedback * Refactor auction slot amendment creation on initialize * Update a comment in amm test utils --- src/ripple/app/misc/impl/AMMUtils.cpp | 23 +++++++---------------- src/test/jtx/impl/AMM.cpp | 6 ++++-- 2 files changed, 11 insertions(+), 18 deletions(-) diff --git a/src/ripple/app/misc/impl/AMMUtils.cpp b/src/ripple/app/misc/impl/AMMUtils.cpp index 0aead02db88..d766bc508b6 100644 --- a/src/ripple/app/misc/impl/AMMUtils.cpp +++ b/src/ripple/app/misc/impl/AMMUtils.cpp @@ -303,22 +303,13 @@ initializeFeeAuctionVote( // AMM creator gets the auction slot for free. // AuctionSlot is created on AMMCreate and updated on AMMDeposit // when AMM is in an empty state - STObject& auctionSlot = [&]() -> STObject& { - if (!rules.enabled(fixInnerObjTemplate)) - { - return ammSle->peekFieldObject(sfAuctionSlot); - } - else - { - if (!ammSle->isFieldPresent(sfAuctionSlot)) - { - STObject auctionSlot = - STObject::makeInnerObject(sfAuctionSlot, rules); - ammSle->set(std::move(auctionSlot)); - } - return ammSle->peekFieldObject(sfAuctionSlot); - } - }(); + if (rules.enabled(fixInnerObjTemplate) && + !ammSle->isFieldPresent(sfAuctionSlot)) + { + STObject auctionSlot = STObject::makeInnerObject(sfAuctionSlot, rules); + ammSle->set(std::move(auctionSlot)); + } + STObject& auctionSlot = ammSle->peekFieldObject(sfAuctionSlot); auctionSlot.setAccountID(sfAccount, account); // current + sec in 24h auto const expiration = std::chrono::duration_cast( diff --git a/src/test/jtx/impl/AMM.cpp b/src/test/jtx/impl/AMM.cpp index 5088e51d0b9..2d5ce90d306 100644 --- a/src/test/jtx/impl/AMM.cpp +++ b/src/test/jtx/impl/AMM.cpp @@ -787,8 +787,10 @@ AMM::expectAuctionSlot(auto&& cb) const static_cast(amm->peekAtField(sfAuctionSlot)); if (auctionSlot.isFieldPresent(sfAccount)) { - // this could fail in pre-fixInnerObjTemplate test if one - // the fail-cases. access as optional to avoid the failure + // This could fail in pre-fixInnerObjTemplate tests + // if the submitted transactions recreate one of + // the failure scenarios. Access as optional + // to avoid the failure. auto const slotFee = auctionSlot[~sfDiscountedFee].value_or(0); auto const slotInterval = ammAuctionTimeSlot( env_.app().timeKeeper().now().time_since_epoch().count(),