diff --git a/src/ripple/app/misc/impl/AMMUtils.cpp b/src/ripple/app/misc/impl/AMMUtils.cpp index 1d787dbe4ca..d766bc508b6 100644 --- a/src/ripple/app/misc/impl/AMMUtils.cpp +++ b/src/ripple/app/misc/impl/AMMUtils.cpp @@ -138,6 +138,9 @@ std::uint16_t getTradingFee(ReadView const& view, SLE const& ammSle, AccountID const& account) { using namespace std::chrono; + assert( + !view.rules().enabled(fixInnerObjTemplate) || + ammSle.isFieldPresent(sfAuctionSlot)); if (ammSle.isFieldPresent(sfAuctionSlot)) { auto const& auctionSlot = @@ -287,9 +290,10 @@ initializeFeeAuctionVote( Issue const& lptIssue, std::uint16_t tfee) { + auto const& rules = view.rules(); // AMM creator gets the voting slot. STArray voteSlots; - STObject voteEntry{sfVoteEntry}; + STObject voteEntry = STObject::makeInnerObject(sfVoteEntry, rules); if (tfee != 0) voteEntry.setFieldU16(sfTradingFee, tfee); voteEntry.setFieldU32(sfVoteWeight, VOTE_WEIGHT_SCALE_FACTOR); @@ -297,7 +301,15 @@ initializeFeeAuctionVote( voteSlots.push_back(voteEntry); ammSle->setFieldArray(sfVoteSlots, voteSlots); // AMM creator gets the auction slot for free. - auto& auctionSlot = ammSle->peekFieldObject(sfAuctionSlot); + // AuctionSlot is created on AMMCreate and updated on AMMDeposit + // when AMM is in an empty state + 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/ripple/app/tx/impl/AMMBid.cpp b/src/ripple/app/tx/impl/AMMBid.cpp index 822e72203a6..e49c378ceeb 100644 --- a/src/ripple/app/tx/impl/AMMBid.cpp +++ b/src/ripple/app/tx/impl/AMMBid.cpp @@ -173,8 +173,18 @@ applyBid( return {tecINTERNAL, false}; STAmount const lptAMMBalance = (*ammSle)[sfLPTokenBalance]; auto const lpTokens = ammLPHolds(sb, *ammSle, account_, ctx_.journal); - if (!ammSle->isFieldPresent(sfAuctionSlot)) - ammSle->makeFieldPresent(sfAuctionSlot); + auto const& rules = ctx_.view().rules(); + if (!rules.enabled(fixInnerObjTemplate)) + { + 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 = duration_cast( diff --git a/src/ripple/app/tx/impl/AMMVote.cpp b/src/ripple/app/tx/impl/AMMVote.cpp index ff0598aaa40..d908a93c383 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; + 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 @@ -119,7 +120,7 @@ applyVote( continue; } auto feeVal = entry[sfTradingFee]; - STObject newEntry{sfVoteEntry}; + STObject newEntry = STObject::makeInnerObject(sfVoteEntry, rules); // 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 = STObject::makeInnerObject(sfVoteEntry, rules); if (feeNew != 0) newEntry.setFieldU16(sfTradingFee, feeNew); newEntry.setFieldU32( @@ -199,6 +200,10 @@ applyVote( } } + 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/protocol/Feature.h b/src/ripple/protocol/Feature.h index d8ce6dc6280..8e6483b1dbd 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 = 66; +static constexpr std::size_t numFeatures = 67; /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated @@ -353,6 +353,7 @@ extern uint256 const fixDisallowIncomingV1; extern uint256 const featureDID; extern uint256 const fixFillOrKill; extern uint256 const fixNFTokenReserve; +extern uint256 const fixInnerObjTemplate; } // namespace ripple diff --git a/src/ripple/protocol/SOTemplate.h b/src/ripple/protocol/SOTemplate.h index 56dcf4492f7..609c2d2c80b 100644 --- a/src/ripple/protocol/SOTemplate.h +++ b/src/ripple/protocol/SOTemplate.h @@ -35,6 +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 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 3e3862bf6c8..5476cd01198 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) @@ -102,6 +103,9 @@ class STObject : public STBase, public CountedObject STObject(SerialIter&& sit, SField const& name); explicit STObject(SField const& name); + static STObject + makeInnerObject(SField const& name, Rules const& rules); + iterator begin() const; @@ -339,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/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index e7cd72fb866..ab36983edd7 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -460,6 +460,7 @@ REGISTER_FIX (fixDisallowIncomingV1, Supported::yes, VoteBehavior::De REGISTER_FEATURE(DID, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX(fixFillOrKill, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixNFTokenReserve, 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/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 5bafbcfce54..7c546a2568e 100644 --- a/src/ripple/protocol/impl/STObject.cpp +++ b/src/ripple/protocol/impl/STObject.cpp @@ -18,7 +18,9 @@ //============================================================================== #include +#include #include +#include #include #include #include @@ -57,6 +59,19 @@ 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)) + { + if (SOTemplate const* elements = + InnerObjectFormats::getInstance().findSOTemplateBySField(name)) + obj.set(*elements); + } + return obj; +} + STBase* STObject::copy(std::size_t n, void* buf) const { @@ -630,16 +645,22 @@ STObject::getFieldArray(SField const& field) const void STObject::set(std::unique_ptr v) { - auto const i = getFieldIndex(v->getFName()); + set(std::move(*v.get())); +} + +void +STObject::set(STBase&& v) +{ + 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)); } } diff --git a/src/ripple/rpc/handlers/AMMInfo.cpp b/src/ripple/rpc/handlers/AMMInfo.cpp index a1be636cafd..c6711fa7b82 100644 --- a/src/ripple/rpc/handlers/AMMInfo.cpp +++ b/src/ripple/rpc/handlers/AMMInfo.cpp @@ -200,6 +200,9 @@ doAMMInfo(RPC::JsonContext& context) } if (voteSlots.size() > 0) ammResult[jss::vote_slots] = std::move(voteSlots); + 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 b0e5106f9eb..b6828fab773 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -4789,6 +4789,106 @@ struct AMM_test : public jtx::AMMTest } } + void + testFixDefaultInnerObj() + { + testcase("Fix Default Inner Object"); + 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(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(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 + // 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 +4915,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..4c6e8d78a4e 100644 --- a/src/test/jtx/AMM.h +++ b/src/test/jtx/AMM.h @@ -57,6 +57,67 @@ 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 err = std::nullopt; + 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 @@ -91,13 +152,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 */ @@ -189,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, @@ -200,14 +271,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 @@ -230,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, @@ -239,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 = @@ -251,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 dee1cb1bf5b..2d5ce90d306 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.err, + arg.close) { } @@ -470,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, @@ -574,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, @@ -595,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, @@ -609,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 = @@ -663,6 +727,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, @@ -698,20 +776,30 @@ 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)) { - auto const slotFee = auctionSlot[sfDiscountedFee]; - 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 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(), + auctionSlot); + auto const slotPrice = auctionSlot[sfPrice].iou(); + auto const authAccounts = + auctionSlot.getFieldArray(sfAuthAccounts); + return cb(slotFee, slotInterval, slotPrice, authAccounts); + } } } return false;