From a61a88ea8104aa48a503dc5543d4738ed395be48 Mon Sep 17 00:00:00 2001 From: Rome Reginelli Date: Fri, 1 Sep 2023 13:44:48 -0700 Subject: [PATCH 1/3] AMMBid: use tecINTERNAL for 'impossible' errors (#4674) Modify two error cases in AMMBid transactor to return `tecINTERNAL` to more clearly indicate that these errors should not be possible unless operating in unforeseen circumstances. It likely indicates a bug. The log level has been updated to `fatal()` since it indicates a (potentially network-wide) unexpected condition when either of these errors occurs. Details: The two specific transaction error cases changed are: - `tecAMM_BALANCE` - In this case, this error (total LP Tokens outstanding is lower than the amount to be burned for the bid) is a subset of the case where the user doesn't have enough LP Tokens to pay for the bid. When this case is reached, the bidder's LP Tokens balance has already been checked first. The user's LP Tokens should always be a subset of total LP Tokens issued, so this should be impossible. - `tecINSUFFICIENT_PAYMENT` - In this case, the amount to be refunded as a result of the bid is greater than the price paid for the auction slot. This should never occur unless something is wrong with the math for calculating the refund amount. Both error cases in question are "defense in depth" measures meant to protect against making things worse if the code has already reached a state that is supposed to be impossible, likely due to a bug elsewhere. Such "shouldn't ever occur" checks should use an error code that categorically indicates a larger problem. This is similar to how `tecINVARIANT_FAILED` is a warning sign that something went wrong and likely could've been worse, but since there isn't an Invariant Check applying here, `tecINTERNAL` is the appropriate error code. This is "debatably" a transaction processing change since it could hypothetically change how transactions are processed if there's a bug we don't know about. --- src/ripple/app/tx/impl/AMMBid.cpp | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/ripple/app/tx/impl/AMMBid.cpp b/src/ripple/app/tx/impl/AMMBid.cpp index d059f88c76c..822e72203a6 100644 --- a/src/ripple/app/tx/impl/AMMBid.cpp +++ b/src/ripple/app/tx/impl/AMMBid.cpp @@ -223,9 +223,11 @@ applyBid( lptAMMBalance, toSTAmount(lptAMMBalance.issue(), burn), false); if (saBurn >= lptAMMBalance) { - JLOG(ctx_.journal.debug()) - << "AMM Bid: invalid burn " << burn << " " << lptAMMBalance; - return tecAMM_BALANCE; + // This error case should never occur. + JLOG(ctx_.journal.fatal()) + << "AMM Bid: LP Token burn exceeds AMM balance " << burn << " " + << lptAMMBalance; + return tecINTERNAL; } auto res = redeemIOU(sb, account_, saBurn, lpTokens.issue(), ctx_.journal); @@ -316,9 +318,10 @@ applyBid( auto const refund = fractionRemaining * pricePurchased; if (refund > *payPrice) { - JLOG(ctx_.journal.debug()) - << "AMM Bid: invalid refund " << refund << " " << *payPrice; - return {tecINSUFFICIENT_PAYMENT, false}; + // This error case should never occur. + JLOG(ctx_.journal.fatal()) << "AMM Bid: refund exceeds payPrice " + << refund << " " << *payPrice; + return {tecINTERNAL, false}; } res = accountSend( sb, From b014b79d88baefe546c3113099581c6c8ee29df6 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Fri, 1 Sep 2023 18:50:58 -0400 Subject: [PATCH 2/3] amm_info: fetch by amm account id; add AMM object entry (#4682) - Update amm_info to fetch AMM by amm account id. - This is an additional way to retrieve an AMM object. - Alternatively, AMM can still be fetched by the asset pair as well. - Add owner directory entry for AMM object. Context: - Add back the AMM object directory entry, which was deleted by #4626. - This fixes `account_objects` for `amm` type. --- src/ripple/app/misc/impl/AMMUtils.cpp | 19 ++- src/ripple/app/tx/impl/AMMCreate.cpp | 14 ++ src/ripple/app/tx/impl/DeleteAccount.cpp | 6 +- src/ripple/ledger/View.h | 12 +- src/ripple/ledger/impl/View.cpp | 24 ++-- src/ripple/protocol/impl/LedgerFormats.cpp | 1 + src/ripple/protocol/jss.h | 1 + src/ripple/rpc/handlers/AMMInfo.cpp | 113 +++++++++++----- src/test/jtx/AMM.h | 8 +- src/test/jtx/impl/AMM.cpp | 42 +++--- src/test/rpc/AMMInfo_test.cpp | 143 +++++++++++++++------ src/test/rpc/AccountObjects_test.cpp | 78 ++++++++++- 12 files changed, 344 insertions(+), 117 deletions(-) diff --git a/src/ripple/app/misc/impl/AMMUtils.cpp b/src/ripple/app/misc/impl/AMMUtils.cpp index dcb403296c1..1d787dbe4ca 100644 --- a/src/ripple/app/misc/impl/AMMUtils.cpp +++ b/src/ripple/app/misc/impl/AMMUtils.cpp @@ -200,14 +200,17 @@ deleteAMMTrustLines( keylet::ownerDir(ammAccountID), [&](LedgerEntryType nodeType, uint256 const&, - std::shared_ptr& sleItem) -> TER { + std::shared_ptr& sleItem) -> std::pair { + // Skip AMM + if (nodeType == LedgerEntryType::ltAMM) + return {tesSUCCESS, SkipEntry::Yes}; // Should only have the trustlines if (nodeType != LedgerEntryType::ltRIPPLE_STATE) { JLOG(j.error()) << "deleteAMMTrustLines: deleting non-trustline " << nodeType; - return tecINTERNAL; + return {tecINTERNAL, SkipEntry::No}; } // Trustlines must have zero balance @@ -216,10 +219,12 @@ deleteAMMTrustLines( JLOG(j.error()) << "deleteAMMTrustLines: deleting trustline with " "non-zero balance."; - return tecINTERNAL; + return {tecINTERNAL, SkipEntry::No}; } - return deleteAMMTrustLine(sb, sleItem, ammAccountID, j); + return { + deleteAMMTrustLine(sb, sleItem, ammAccountID, j), + SkipEntry::No}; }, j, maxTrustlinesToDelete); @@ -255,6 +260,12 @@ deleteAMMAccount( return ter; auto const ownerDirKeylet = keylet::ownerDir(ammAccountID); + if (!sb.dirRemove( + ownerDirKeylet, (*ammSle)[sfOwnerNode], ammSle->key(), false)) + { + JLOG(j.error()) << "deleteAMMAccount: failed to remove dir link"; + return tecINTERNAL; + } if (sb.exists(ownerDirKeylet) && !sb.emptyDirDelete(ownerDirKeylet)) { JLOG(j.error()) << "deleteAMMAccount: cannot delete root dir node of " diff --git a/src/ripple/app/tx/impl/AMMCreate.cpp b/src/ripple/app/tx/impl/AMMCreate.cpp index 0c6874953a3..55b1126fcd0 100644 --- a/src/ripple/app/tx/impl/AMMCreate.cpp +++ b/src/ripple/app/tx/impl/AMMCreate.cpp @@ -279,6 +279,20 @@ applyCreate( // AMM creator gets the auction slot and the voting slot. initializeFeeAuctionVote( ctx_.view(), ammSle, account_, lptIss, ctx_.tx[sfTradingFee]); + + // Add owner directory to link the root account and AMM object. + if (auto const page = sb.dirInsert( + keylet::ownerDir(*ammAccount), + ammSle->key(), + describeOwnerDir(*ammAccount))) + { + ammSle->setFieldU64(sfOwnerNode, *page); + } + else + { + JLOG(j_.debug()) << "AMM Instance: failed to insert owner dir"; + return {tecDIR_FULL, false}; + } sb.insert(ammSle); // Send LPT to LP. diff --git a/src/ripple/app/tx/impl/DeleteAccount.cpp b/src/ripple/app/tx/impl/DeleteAccount.cpp index eeffc66d382..67545723a5f 100644 --- a/src/ripple/app/tx/impl/DeleteAccount.cpp +++ b/src/ripple/app/tx/impl/DeleteAccount.cpp @@ -298,19 +298,19 @@ DeleteAccount::doApply() ownerDirKeylet, [&](LedgerEntryType nodeType, uint256 const& dirEntry, - std::shared_ptr& sleItem) -> TER { + std::shared_ptr& sleItem) -> std::pair { if (auto deleter = nonObligationDeleter(nodeType)) { TER const result{ deleter(ctx_.app, view(), account_, dirEntry, sleItem, j_)}; - return result; + return {result, SkipEntry::No}; } assert(!"Undeletable entry should be found in preclaim."); JLOG(j_.error()) << "DeleteAccount undeletable item not " "found in preclaim."; - return tecHAS_OBLIGATIONS; + return {tecHAS_OBLIGATIONS, SkipEntry::No}; }, ctx_.journal); if (ter != tesSUCCESS) diff --git a/src/ripple/ledger/View.h b/src/ripple/ledger/View.h index 2c8d2354e0c..5680114a79c 100644 --- a/src/ripple/ledger/View.h +++ b/src/ripple/ledger/View.h @@ -43,6 +43,7 @@ namespace ripple { enum class WaiveTransferFee : bool { No = false, Yes }; +enum class SkipEntry : bool { No = false, Yes }; //------------------------------------------------------------------------------ // @@ -458,6 +459,14 @@ transferXRP( [[nodiscard]] TER requireAuth(ReadView const& view, Issue const& issue, AccountID const& account); +/** Deleter function prototype. Returns the status of the entry deletion + * (if should not be skipped) and if the entry should be skipped. The status + * is always tesSUCCESS if the entry should be skipped. + */ +using EntryDeleter = std::function( + LedgerEntryType, + uint256 const&, + std::shared_ptr&)>; /** Cleanup owner directory entries on account delete. * Used for a regular and AMM accounts deletion. The caller * has to provide the deleter function, which handles details of @@ -469,8 +478,7 @@ requireAuth(ReadView const& view, Issue const& issue, AccountID const& account); cleanupOnAccountDelete( ApplyView& view, Keylet const& ownerDirKeylet, - std::function&)> - deleter, + EntryDeleter const& deleter, beast::Journal j, std::optional maxNodesToDelete = std::nullopt); diff --git a/src/ripple/ledger/impl/View.cpp b/src/ripple/ledger/impl/View.cpp index 75fd35782b6..5050e8764e9 100644 --- a/src/ripple/ledger/impl/View.cpp +++ b/src/ripple/ledger/impl/View.cpp @@ -1531,8 +1531,7 @@ TER cleanupOnAccountDelete( ApplyView& view, Keylet const& ownerDirKeylet, - std::function&)> - deleter, + EntryDeleter const& deleter, beast::Journal j, std::optional maxNodesToDelete) { @@ -1567,8 +1566,8 @@ cleanupOnAccountDelete( // Deleter handles the details of specific account-owned object // deletion - if (auto const ter = deleter(nodeType, dirEntry, sleItem); - ter != tesSUCCESS) + auto const [ter, skipEntry] = deleter(nodeType, dirEntry, sleItem); + if (ter != tesSUCCESS) return ter; // dirFirst() and dirNext() are like iterators with exposed @@ -1580,21 +1579,22 @@ cleanupOnAccountDelete( // "iterator state" is invalid. // // 1. During the process of getting an entry from the - // directory uDirEntry was incremented from 0 to 1. + // directory uDirEntry was incremented from 'it' to 'it'+1. // - // 2. We then deleted the entry at index 0, which means the - // entry that was at 1 has now moved to 0. + // 2. We then deleted the entry at index 'it', which means the + // entry that was at 'it'+1 has now moved to 'it'. // - // 3. So we verify that uDirEntry is indeed 1. Then we jam it - // back to zero to "un-invalidate" the iterator. - assert(uDirEntry == 1); - if (uDirEntry != 1) + // 3. So we verify that uDirEntry is indeed 'it'+1. Then we jam it + // back to 'it' to "un-invalidate" the iterator. + assert(uDirEntry >= 1); + if (uDirEntry == 0) { JLOG(j.error()) << "DeleteAccount iterator re-validation failed."; return tefBAD_LEDGER; } - uDirEntry = 0; + if (skipEntry == SkipEntry::No) + uDirEntry--; } while ( dirNext(view, ownerDirKeylet.key, sleDirNode, uDirEntry, dirEntry)); diff --git a/src/ripple/protocol/impl/LedgerFormats.cpp b/src/ripple/protocol/impl/LedgerFormats.cpp index 9192513457a..d9e7ca178c0 100644 --- a/src/ripple/protocol/impl/LedgerFormats.cpp +++ b/src/ripple/protocol/impl/LedgerFormats.cpp @@ -279,6 +279,7 @@ LedgerFormats::LedgerFormats() {sfLPTokenBalance, soeREQUIRED}, {sfAsset, soeREQUIRED}, {sfAsset2, soeREQUIRED}, + {sfOwnerNode, soeREQUIRED}, }, commonFields); diff --git a/src/ripple/protocol/jss.h b/src/ripple/protocol/jss.h index a27a564e112..567e63699e2 100644 --- a/src/ripple/protocol/jss.h +++ b/src/ripple/protocol/jss.h @@ -160,6 +160,7 @@ JSS(alternatives); // out: PathRequest, RipplePathFind JSS(amendment_blocked); // out: NetworkOPs JSS(amendments); // in: AccountObjects, out: NetworkOPs JSS(amm); // out: amm_info +JSS(amm_account); // in: amm_info JSS(amount); // out: AccountChannels, amm_info JSS(amount2); // out: amm_info JSS(api_version); // in: many, out: Version diff --git a/src/ripple/rpc/handlers/AMMInfo.cpp b/src/ripple/rpc/handlers/AMMInfo.cpp index bcac9da171b..11e124afb44 100644 --- a/src/ripple/rpc/handlers/AMMInfo.cpp +++ b/src/ripple/rpc/handlers/AMMInfo.cpp @@ -74,51 +74,96 @@ doAMMInfo(RPC::JsonContext& context) { auto const& params(context.params); Json::Value result; - std::optional accountID; - - Issue issue1; - Issue issue2; - - if (!params.isMember(jss::asset) || !params.isMember(jss::asset2)) - { - RPC::inject_error(rpcINVALID_PARAMS, result); - return result; - } - - if (auto const i = getIssue(params[jss::asset], context.j); !i) - { - RPC::inject_error(i.error(), result); - return result; - } - else - issue1 = *i; - if (auto const i = getIssue(params[jss::asset2], context.j); !i) - { - RPC::inject_error(i.error(), result); - return result; - } - else - issue2 = *i; std::shared_ptr ledger; result = RPC::lookupLedger(ledger, context); if (!ledger) return result; - if (params.isMember(jss::account)) + struct ValuesFromContextParams { - accountID = getAccount(params[jss::account], result); - if (!accountID || !ledger->read(keylet::account(*accountID))) + std::optional accountID; + Issue issue1; + Issue issue2; + std::shared_ptr amm; + }; + + auto getValuesFromContextParams = + [&]() -> Expected { + std::optional accountID; + std::optional issue1; + std::optional issue2; + std::optional ammID; + + if ((params.isMember(jss::asset) != params.isMember(jss::asset2)) || + (params.isMember(jss::asset) == params.isMember(jss::amm_account))) + return Unexpected(rpcINVALID_PARAMS); + + if (params.isMember(jss::asset)) + { + if (auto const i = getIssue(params[jss::asset], context.j)) + issue1 = *i; + else + return Unexpected(i.error()); + } + + if (params.isMember(jss::asset2)) { - RPC::inject_error(rpcACT_MALFORMED, result); - return result; + if (auto const i = getIssue(params[jss::asset2], context.j)) + issue2 = *i; + else + return Unexpected(i.error()); } + + if (params.isMember(jss::amm_account)) + { + auto const id = getAccount(params[jss::amm_account], result); + if (!id) + return Unexpected(rpcACT_MALFORMED); + auto const sle = ledger->read(keylet::account(*id)); + if (!sle) + return Unexpected(rpcACT_MALFORMED); + ammID = sle->getFieldH256(sfAMMID); + } + + assert( + (issue1.has_value() == issue2.has_value()) && + (issue1.has_value() != ammID.has_value())); + + if (params.isMember(jss::account)) + { + accountID = getAccount(params[jss::account], result); + if (!accountID || !ledger->read(keylet::account(*accountID))) + return Unexpected(rpcACT_MALFORMED); + } + + auto const ammKeylet = [&]() { + if (issue1 && issue2) + return keylet::amm(*issue1, *issue2); + assert(ammID); + return keylet::amm(*ammID); + }(); + auto const amm = ledger->read(ammKeylet); + if (!amm) + return Unexpected(rpcACT_NOT_FOUND); + if (!issue1 && !issue2) + { + issue1 = (*amm)[sfAsset]; + issue2 = (*amm)[sfAsset2]; + } + + return ValuesFromContextParams{ + accountID, *issue1, *issue2, std::move(amm)}; + }; + + auto const r = getValuesFromContextParams(); + if (!r) + { + RPC::inject_error(r.error(), result); + return result; } - auto const ammKeylet = keylet::amm(issue1, issue2); - auto const amm = ledger->read(ammKeylet); - if (!amm) - return rpcError(rpcACT_NOT_FOUND); + auto const& [accountID, issue1, issue2, amm] = *r; auto const ammAccountID = amm->getAccountID(sfAccount); diff --git a/src/test/jtx/AMM.h b/src/test/jtx/AMM.h index 3cf06bfe401..c7c6f3b8477 100644 --- a/src/test/jtx/AMM.h +++ b/src/test/jtx/AMM.h @@ -105,7 +105,10 @@ class AMM ammRpcInfo( std::optional const& account = std::nullopt, std::optional const& ledgerIndex = std::nullopt, - std::optional> tokens = std::nullopt) const; + std::optional issue1 = std::nullopt, + std::optional issue2 = std::nullopt, + std::optional const& ammAccount = std::nullopt, + bool ignoreParams = false) const; /** Verify the AMM balances. */ @@ -150,7 +153,8 @@ class AMM STAmount const& asset2, IOUAmount const& balance, std::optional const& account = std::nullopt, - std::optional const& ledger_index = std::nullopt) const; + std::optional const& ledger_index = std::nullopt, + std::optional const& ammAccount = std::nullopt) const; [[nodiscard]] bool ammExists() const; diff --git a/src/test/jtx/impl/AMM.cpp b/src/test/jtx/impl/AMM.cpp index c09d496f439..dee1cb1bf5b 100644 --- a/src/test/jtx/impl/AMM.cpp +++ b/src/test/jtx/impl/AMM.cpp @@ -136,26 +136,36 @@ Json::Value AMM::ammRpcInfo( std::optional const& account, std::optional const& ledgerIndex, - std::optional> tokens) const + std::optional issue1, + std::optional issue2, + std::optional const& ammAccount, + bool ignoreParams) const { Json::Value jv; if (account) jv[jss::account] = to_string(*account); if (ledgerIndex) jv[jss::ledger_index] = *ledgerIndex; - if (tokens) + if (!ignoreParams) { - jv[jss::asset] = - STIssue(sfAsset, tokens->first).getJson(JsonOptions::none); - jv[jss::asset2] = - STIssue(sfAsset2, tokens->second).getJson(JsonOptions::none); - } - else - { - jv[jss::asset] = - STIssue(sfAsset, asset1_.issue()).getJson(JsonOptions::none); - jv[jss::asset2] = - STIssue(sfAsset2, asset2_.issue()).getJson(JsonOptions::none); + if (issue1 || issue2) + { + if (issue1) + jv[jss::asset] = + STIssue(sfAsset, *issue1).getJson(JsonOptions::none); + if (issue2) + jv[jss::asset2] = + STIssue(sfAsset2, *issue2).getJson(JsonOptions::none); + } + else if (!ammAccount) + { + jv[jss::asset] = + STIssue(sfAsset, asset1_.issue()).getJson(JsonOptions::none); + jv[jss::asset2] = + STIssue(sfAsset2, asset2_.issue()).getJson(JsonOptions::none); + } + if (ammAccount) + jv[jss::amm_account] = to_string(*ammAccount); } auto jr = env_.rpc("json", "amm_info", to_string(jv)); if (jr.isObject() && jr.isMember(jss::result) && @@ -292,9 +302,11 @@ AMM::expectAmmRpcInfo( STAmount const& asset2, IOUAmount const& balance, std::optional const& account, - std::optional const& ledger_index) const + std::optional const& ledger_index, + std::optional const& ammAccount) const { - auto const jv = ammRpcInfo(account, ledger_index); + auto const jv = ammRpcInfo( + account, ledger_index, std::nullopt, std::nullopt, ammAccount); return expectAmmInfo(asset1, asset2, balance, jv); } diff --git a/src/test/rpc/AMMInfo_test.cpp b/src/test/rpc/AMMInfo_test.cpp index 94795f48574..1d9642539a1 100644 --- a/src/test/rpc/AMMInfo_test.cpp +++ b/src/test/rpc/AMMInfo_test.cpp @@ -42,7 +42,7 @@ class AMMInfo_test : public jtx::AMMTestBase Account const gw("gw"); auto const USD = gw["USD"]; auto const jv = - ammAlice.ammRpcInfo({}, {}, {{USD.issue(), USD.issue()}}); + ammAlice.ammRpcInfo({}, {}, USD.issue(), USD.issue()); BEAST_EXPECT(jv[jss::error_message] == "Account not found."); }); @@ -52,6 +52,40 @@ class AMMInfo_test : public jtx::AMMTestBase auto const jv = ammAlice.ammRpcInfo(bogie.id()); BEAST_EXPECT(jv[jss::error_message] == "Account malformed."); }); + + // Invalid parameters + testAMM([&](AMM& ammAlice, Env&) { + std::vector, + std::optional, + std::optional, + bool>> + vals = { + {xrpIssue(), std::nullopt, std::nullopt, false}, + {std::nullopt, USD.issue(), std::nullopt, false}, + {xrpIssue(), std::nullopt, ammAlice.ammAccount(), false}, + {std::nullopt, USD.issue(), ammAlice.ammAccount(), false}, + {xrpIssue(), USD.issue(), ammAlice.ammAccount(), false}, + {std::nullopt, std::nullopt, std::nullopt, true}}; + for (auto const& [iss1, iss2, acct, ignoreParams] : vals) + { + auto const jv = ammAlice.ammRpcInfo( + std::nullopt, std::nullopt, iss1, iss2, acct, ignoreParams); + BEAST_EXPECT(jv[jss::error_message] == "Invalid parameters."); + } + }); + + // Invalid AMM account id + testAMM([&](AMM& ammAlice, Env&) { + Account bogie("bogie"); + auto const jv = ammAlice.ammRpcInfo( + std::nullopt, + std::nullopt, + std::nullopt, + std::nullopt, + bogie.id()); + BEAST_EXPECT(jv[jss::error_message] == "Account malformed."); + }); } void @@ -63,6 +97,13 @@ class AMMInfo_test : public jtx::AMMTestBase testAMM([&](AMM& ammAlice, Env&) { BEAST_EXPECT(ammAlice.expectAmmRpcInfo( XRP(10000), USD(10000), IOUAmount{10000000, 0})); + BEAST_EXPECT(ammAlice.expectAmmRpcInfo( + XRP(10000), + USD(10000), + IOUAmount{10000000, 0}, + std::nullopt, + std::nullopt, + ammAlice.ammAccount())); }); } @@ -91,53 +132,71 @@ class AMMInfo_test : public jtx::AMMTestBase env.fund(XRP(1000), bob, ed, bill); ammAlice.bid(alice, 100, std::nullopt, {carol, bob, ed, bill}); BEAST_EXPECT(ammAlice.expectAmmRpcInfo( - XRP(80000), USD(80000), IOUAmount{79994400})); - std::unordered_set authAccounts = { - carol.human(), bob.human(), ed.human(), bill.human()}; - auto const ammInfo = ammAlice.ammRpcInfo(); - auto const& amm = ammInfo[jss::amm]; - try + XRP(80000), + USD(80000), + IOUAmount{79994400}, + std::nullopt, + std::nullopt, + ammAlice.ammAccount())); + for (auto i = 0; i < 2; ++i) { - // votes - auto const voteSlots = amm[jss::vote_slots]; - for (std::uint8_t i = 0; i < 8; ++i) + std::unordered_set authAccounts = { + carol.human(), bob.human(), ed.human(), bill.human()}; + auto const ammInfo = i ? ammAlice.ammRpcInfo() + : ammAlice.ammRpcInfo( + std::nullopt, + std::nullopt, + std::nullopt, + std::nullopt, + ammAlice.ammAccount()); + auto const& amm = ammInfo[jss::amm]; + try { - if (!BEAST_EXPECT( - votes[voteSlots[i][jss::account].asString()] == - voteSlots[i][jss::trading_fee].asUInt() && - voteSlots[i][jss::vote_weight].asUInt() == 12500)) + // votes + auto const voteSlots = amm[jss::vote_slots]; + auto votesCopy = votes; + for (std::uint8_t i = 0; i < 8; ++i) + { + if (!BEAST_EXPECT( + votes[voteSlots[i][jss::account].asString()] == + voteSlots[i][jss::trading_fee].asUInt() && + voteSlots[i][jss::vote_weight].asUInt() == + 12500)) + return; + votes.erase(voteSlots[i][jss::account].asString()); + } + if (!BEAST_EXPECT(votes.empty())) return; - votes.erase(voteSlots[i][jss::account].asString()); - } - if (!BEAST_EXPECT(votes.empty())) - return; - - // bid - auto const auctionSlot = amm[jss::auction_slot]; - for (std::uint8_t i = 0; i < 4; ++i) - { - if (!BEAST_EXPECT(authAccounts.contains( + votes = votesCopy; + + // bid + auto const auctionSlot = amm[jss::auction_slot]; + for (std::uint8_t i = 0; i < 4; ++i) + { + if (!BEAST_EXPECT(authAccounts.contains( + auctionSlot[jss::auth_accounts][i][jss::account] + .asString()))) + return; + authAccounts.erase( auctionSlot[jss::auth_accounts][i][jss::account] - .asString()))) + .asString()); + } + if (!BEAST_EXPECT(authAccounts.empty())) return; - authAccounts.erase( - auctionSlot[jss::auth_accounts][i][jss::account] - .asString()); + BEAST_EXPECT( + auctionSlot[jss::account].asString() == alice.human() && + auctionSlot[jss::discounted_fee].asUInt() == 17 && + auctionSlot[jss::price][jss::value].asString() == + "5600" && + auctionSlot[jss::price][jss::currency].asString() == + to_string(ammAlice.lptIssue().currency) && + auctionSlot[jss::price][jss::issuer].asString() == + to_string(ammAlice.lptIssue().account)); + } + catch (std::exception const& e) + { + fail(e.what(), __FILE__, __LINE__); } - if (!BEAST_EXPECT(authAccounts.empty())) - return; - BEAST_EXPECT( - auctionSlot[jss::account].asString() == alice.human() && - auctionSlot[jss::discounted_fee].asUInt() == 17 && - auctionSlot[jss::price][jss::value].asString() == "5600" && - auctionSlot[jss::price][jss::currency].asString() == - to_string(ammAlice.lptIssue().currency) && - auctionSlot[jss::price][jss::issuer].asString() == - to_string(ammAlice.lptIssue().account)); - } - catch (std::exception const& e) - { - fail(e.what(), __FILE__, __LINE__); } }); } diff --git a/src/test/rpc/AccountObjects_test.cpp b/src/test/rpc/AccountObjects_test.cpp index 7de5b73671e..90d4e54684f 100644 --- a/src/test/rpc/AccountObjects_test.cpp +++ b/src/test/rpc/AccountObjects_test.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include @@ -552,10 +553,19 @@ class AccountObjects_test : public beast::unit_test::suite Env env(*this); // Make a lambda we can use to get "account_objects" easily. - auto acct_objs = [&env](Account const& acct, char const* type) { + auto acct_objs = [&env]( + AccountID const& acct, + std::optional const& type, + std::optional limit = std::nullopt, + std::optional marker = std::nullopt) { Json::Value params; - params[jss::account] = acct.human(); - params[jss::type] = type; + params[jss::account] = to_string(acct); + if (type) + params[jss::type] = *type; + if (limit) + params[jss::limit] = *limit; + if (marker) + params[jss::marker] = *marker; params[jss::ledger_index] = "validated"; return env.rpc("json", "account_objects", to_string(params)); }; @@ -585,6 +595,7 @@ class AccountObjects_test : public beast::unit_test::suite BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::signer_list), 0)); BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::state), 0)); BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::ticket), 0)); + BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::amm), 0)); // gw mints an NFT so we can find it. uint256 const nftID{token::getNextID(env, gw, 0u, tfTransferable)}; @@ -782,6 +793,67 @@ class AccountObjects_test : public beast::unit_test::suite BEAST_EXPECT(aobjs[0u]["LedgerEntryType"] == jss::Escrow); } } + { + // Make a lambda to get the types + auto getTypes = [&](Json::Value const& resp, + std::vector& typesOut) { + auto const objs = resp[jss::result][jss::account_objects]; + for (auto const& obj : resp[jss::result][jss::account_objects]) + typesOut.push_back( + obj[sfLedgerEntryType.fieldName].asString()); + std::sort(typesOut.begin(), typesOut.end()); + }; + // Make a lambda we can use to check the number of fetched + // account objects and their ledger type + auto expectObjects = + [&](Json::Value const& resp, + std::vector const& types) -> bool { + if (!acct_objs_is_size(resp, types.size())) + return false; + std::vector typesOut; + getTypes(resp, typesOut); + return types == typesOut; + }; + // Find AMM objects + AMM amm(env, gw, XRP(1'000), USD(1'000)); + amm.deposit(alice, USD(1)); + // AMM account has 4 objects: AMM object and 3 trustlines + auto const lines = getAccountLines(env, amm.ammAccount()); + BEAST_EXPECT(lines[jss::lines].size() == 3); + // request AMM only, doesn't depend on the limit + BEAST_EXPECT( + acct_objs_is_size(acct_objs(amm.ammAccount(), jss::amm), 1)); + // request first two objects + auto resp = acct_objs(amm.ammAccount(), std::nullopt, 2); + std::vector typesOut; + getTypes(resp, typesOut); + // request next two objects + resp = acct_objs( + amm.ammAccount(), + std::nullopt, + 10, + resp[jss::result][jss::marker].asString()); + getTypes(resp, typesOut); + BEAST_EXPECT( + (typesOut == + std::vector{ + jss::AMM.c_str(), + jss::RippleState.c_str(), + jss::RippleState.c_str(), + jss::RippleState.c_str()})); + // filter by state: there are three trustlines + resp = acct_objs(amm.ammAccount(), jss::state, 10); + BEAST_EXPECT(expectObjects( + resp, + {jss::RippleState.c_str(), + jss::RippleState.c_str(), + jss::RippleState.c_str()})); + // AMM account doesn't own offers + BEAST_EXPECT( + acct_objs_is_size(acct_objs(amm.ammAccount(), jss::offer), 0)); + // gw account doesn't own AMM object + BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::amm), 0)); + } // Run up the number of directory entries so gw has two // directory nodes. From 9d4d8c22d97cb83c9e8115d4e47a27463d03a221 Mon Sep 17 00:00:00 2001 From: Elliot Lee Date: Fri, 1 Sep 2023 15:53:10 -0700 Subject: [PATCH 3/3] Set version to 1.12.0-rc4 --- src/ripple/protocol/impl/BuildInfo.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ripple/protocol/impl/BuildInfo.cpp b/src/ripple/protocol/impl/BuildInfo.cpp index af122ee3845..722d2636430 100644 --- a/src/ripple/protocol/impl/BuildInfo.cpp +++ b/src/ripple/protocol/impl/BuildInfo.cpp @@ -33,7 +33,7 @@ namespace BuildInfo { // and follow the format described at http://semver.org/ //------------------------------------------------------------------------------ // clang-format off -char const* const versionString = "1.12.0-rc3" +char const* const versionString = "1.12.0-rc4" // clang-format on #if defined(DEBUG) || defined(SANITIZER)