From dccf3f49ef35bedef76dd93e14fd071e34cd7f58 Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Mon, 29 Apr 2024 15:44:20 -0700 Subject: [PATCH 01/11] Update list of maintainers: (#4984) I am resigning from my role as maintainer of the `rippled` codebase. Please update repository permissions accordingly, prior to merging this pull request. Thanks to everyone who has contributed, especially those whom I had the opportunity to closely collaborate with. --- CONTRIBUTING.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b27a7dfd9bb..ed4656651cf 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -200,7 +200,6 @@ existing maintainer without a vote. * [JoelKatz](https://github.com/JoelKatz) (Ripple) * [manojsdoshi](https://github.com/manojsdoshi) (Ripple) * [n3tc4t](https://github.com/n3tc4t) (XRPL Labs) -* [Nik Bougalis](https://github.com/nbougalis) * [nixer89](https://github.com/nixer89) (XRP Ledger Foundation) * [RichardAH](https://github.com/RichardAH) (XRPL Labs + XRP Ledger Foundation) * [seelabs](https://github.com/seelabs) (Ripple) From 5aa1106ba1fe25ee26dae34feeeed2136ba21bd7 Mon Sep 17 00:00:00 2001 From: seelabs Date: Wed, 1 May 2024 11:40:06 -0400 Subject: [PATCH 02/11] Remove flow assert: (#5009) Rounding in the payment engine is causing an assert to sometimes fire with "dust" amounts. This is causing issues when running debug builds of rippled. This issue will be addressed, but the assert is no longer serving its purpose. --- src/ripple/app/paths/impl/StrandFlow.h | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/ripple/app/paths/impl/StrandFlow.h b/src/ripple/app/paths/impl/StrandFlow.h index 7817251560f..2df496d8311 100644 --- a/src/ripple/app/paths/impl/StrandFlow.h +++ b/src/ripple/app/paths/impl/StrandFlow.h @@ -834,7 +834,13 @@ flow( { if (actualOut > outReq) { - assert(0); + // Rounding in the payment engine is causing this assert to + // sometimes fire with "dust" amounts. This is causing issues when + // running debug builds of rippled. While this issue still needs to + // be resolved, the assert is causing more harm than good at this + // point. + // assert(0); + return {tefEXCEPTION, std::move(ofrsToRmOnFail)}; } if (!partialPayment) From 76128051c08b4038455edfa5f5acc9a1d31e8ba5 Mon Sep 17 00:00:00 2001 From: John Freeman Date: Thu, 2 May 2024 10:14:59 -0500 Subject: [PATCH 03/11] Add missing includes (#5011) --- src/ripple/conditions/impl/utils.h | 2 ++ src/ripple/consensus/LedgerTrie.h | 1 + src/test/csf/Histogram.h | 2 ++ src/test/csf/Tx.h | 1 + src/test/jtx/JTx.h | 1 + src/test/rpc/GRPCTestClientBase.h | 1 + 6 files changed, 8 insertions(+) diff --git a/src/ripple/conditions/impl/utils.h b/src/ripple/conditions/impl/utils.h index 75a23725e06..e9ab1770a18 100644 --- a/src/ripple/conditions/impl/utils.h +++ b/src/ripple/conditions/impl/utils.h @@ -20,6 +20,8 @@ #ifndef RIPPLE_CONDITIONS_UTILS_H #define RIPPLE_CONDITIONS_UTILS_H +#include +#include #include #include #include diff --git a/src/ripple/consensus/LedgerTrie.h b/src/ripple/consensus/LedgerTrie.h index 0bb902ef1cb..12cd0d3cbbc 100644 --- a/src/ripple/consensus/LedgerTrie.h +++ b/src/ripple/consensus/LedgerTrie.h @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include diff --git a/src/test/csf/Histogram.h b/src/test/csf/Histogram.h index 9d385cc1f5d..2df1844a21d 100644 --- a/src/test/csf/Histogram.h +++ b/src/test/csf/Histogram.h @@ -20,7 +20,9 @@ #define RIPPLE_TEST_CSF_HISTOGRAM_H_INCLUDED #include +#include #include +#include #include namespace ripple { diff --git a/src/test/csf/Tx.h b/src/test/csf/Tx.h index d271338141a..7f3645a59e6 100644 --- a/src/test/csf/Tx.h +++ b/src/test/csf/Tx.h @@ -24,6 +24,7 @@ #include #include #include +#include #include #include diff --git a/src/test/jtx/JTx.h b/src/test/jtx/JTx.h index 06b68dda336..2eeb6ccf8f4 100644 --- a/src/test/jtx/JTx.h +++ b/src/test/jtx/JTx.h @@ -21,6 +21,7 @@ #define RIPPLE_TEST_JTX_JTX_H_INCLUDED #include +#include #include #include #include diff --git a/src/test/rpc/GRPCTestClientBase.h b/src/test/rpc/GRPCTestClientBase.h index a2cf8b686e8..a5c613f55e2 100644 --- a/src/test/rpc/GRPCTestClientBase.h +++ b/src/test/rpc/GRPCTestClientBase.h @@ -20,6 +20,7 @@ #ifndef RIPPLED_GRPCTESTCLIENTBASE_H #define RIPPLED_GRPCTESTCLIENTBASE_H +#include #include #include From f6509495731b396ce33d96778e8df0366a8200bf Mon Sep 17 00:00:00 2001 From: Michael Legleux Date: Thu, 2 May 2024 12:44:42 -0700 Subject: [PATCH 04/11] Add external directory to Conan recipe's exports (#5006) --- conanfile.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/conanfile.py b/conanfile.py index 1d4259777cf..e61728039fc 100644 --- a/conanfile.py +++ b/conanfile.py @@ -116,7 +116,7 @@ def requirements(self): self.requires('rocksdb/6.29.5') exports_sources = ( - 'CMakeLists.txt', 'Builds/*', 'bin/getRippledInfo', 'src/*', 'cfg/*' + 'CMakeLists.txt', 'Builds/*', 'bin/getRippledInfo', 'src/*', 'cfg/*', 'external/*' ) def layout(self): From f4da2e31d93e921f1742a6c7c2120409cba4810e Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Thu, 9 May 2024 15:17:16 -0400 Subject: [PATCH 05/11] Price Oracle: validate input parameters and extend test coverage: (#5013) * Price Oracle: validate input parameters and extend test coverage: Validate trim, time_threshold, document_id are valid Int, UInt, or string convertible to UInt. Validate base_asset and quote_asset are valid currency. Update error codes. Extend Oracle and GetAggregatePrice unit-tests. Denote unreachable coverage code. * Set one-line LCOV_EXCL_LINE * Move ledger_entry tests to LedgerRPC_test.cpp * Add constants for "None" * Fix LedgerRPC test --------- Co-authored-by: Scott Determan --- src/ripple/app/tx/impl/DeleteOracle.cpp | 12 +- src/ripple/app/tx/impl/SetOracle.cpp | 13 +- src/ripple/rpc/handlers/GetAggregatePrice.cpp | 68 ++++++-- src/ripple/rpc/handlers/LedgerEntry.cpp | 2 +- src/test/app/Oracle_test.cpp | 119 +++++++------ src/test/jtx/Oracle.h | 66 ++++--- src/test/jtx/impl/Oracle.cpp | 164 ++++++++++++++---- src/test/rpc/GetAggregatePrice_test.cpp | 101 +++++++++-- src/test/rpc/LedgerRPC_test.cpp | 84 +++++++++ 9 files changed, 472 insertions(+), 157 deletions(-) diff --git a/src/ripple/app/tx/impl/DeleteOracle.cpp b/src/ripple/app/tx/impl/DeleteOracle.cpp index dfaecc384d4..9331daee97d 100644 --- a/src/ripple/app/tx/impl/DeleteOracle.cpp +++ b/src/ripple/app/tx/impl/DeleteOracle.cpp @@ -48,7 +48,7 @@ TER DeleteOracle::preclaim(PreclaimContext const& ctx) { if (!ctx.view.exists(keylet::account(ctx.tx.getAccountID(sfAccount)))) - return terNO_ACCOUNT; + return terNO_ACCOUNT; // LCOV_EXCL_LINE if (auto const sle = ctx.view.read(keylet::oracle( ctx.tx.getAccountID(sfAccount), ctx.tx[sfOracleDocumentID])); @@ -60,8 +60,10 @@ DeleteOracle::preclaim(PreclaimContext const& ctx) else if (ctx.tx.getAccountID(sfAccount) != sle->getAccountID(sfOwner)) { // this can't happen because of the above check + // LCOV_EXCL_START JLOG(ctx.j.debug()) << "Oracle Delete: invalid account."; return tecINTERNAL; + // LCOV_EXCL_STOP } return tesSUCCESS; } @@ -74,18 +76,20 @@ DeleteOracle::deleteOracle( beast::Journal j) { if (!sle) - return tesSUCCESS; + return tecINTERNAL; // LCOV_EXCL_LINE if (!view.dirRemove( keylet::ownerDir(account), (*sle)[sfOwnerNode], sle->key(), true)) { + // LCOV_EXCL_START JLOG(j.fatal()) << "Unable to delete Oracle from owner."; return tefBAD_LEDGER; + // LCOV_EXCL_STOP } auto const sleOwner = view.peek(keylet::account(account)); if (!sleOwner) - return tecINTERNAL; + return tecINTERNAL; // LCOV_EXCL_LINE auto const count = sle->getFieldArray(sfPriceDataSeries).size() > 5 ? -2 : -1; @@ -104,7 +108,7 @@ DeleteOracle::doApply() keylet::oracle(account_, ctx_.tx[sfOracleDocumentID]))) return deleteOracle(ctx_.view(), sle, account_, j_); - return tecINTERNAL; + return tecINTERNAL; // LCOV_EXCL_LINE } } // namespace ripple diff --git a/src/ripple/app/tx/impl/SetOracle.cpp b/src/ripple/app/tx/impl/SetOracle.cpp index 37dc6fcd212..dcd00311475 100644 --- a/src/ripple/app/tx/impl/SetOracle.cpp +++ b/src/ripple/app/tx/impl/SetOracle.cpp @@ -74,7 +74,7 @@ SetOracle::preclaim(PreclaimContext const& ctx) auto const sleSetter = ctx.view.read(keylet::account(ctx.tx.getAccountID(sfAccount))); if (!sleSetter) - return terNO_ACCOUNT; + return terNO_ACCOUNT; // LCOV_EXCL_LINE // lastUpdateTime must be within maxLastUpdateTimeDelta seconds // of the last closed ledger @@ -88,8 +88,7 @@ SetOracle::preclaim(PreclaimContext const& ctx) std::size_t const lastUpdateTimeEpoch = lastUpdateTime - epoch_offset.count(); if (closeTime < maxLastUpdateTimeDelta) - Throw( - "Oracle: close time is less than maxLastUpdateTimeDelta"); + return tecINTERNAL; // LCOV_EXCL_LINE if (lastUpdateTimeEpoch < (closeTime - maxLastUpdateTimeDelta) || lastUpdateTimeEpoch > (closeTime + maxLastUpdateTimeDelta)) return tecINVALID_UPDATE_TIME; @@ -194,7 +193,7 @@ adjustOwnerCount(ApplyContext& ctx, int count) return true; } - return false; + return false; // LCOV_EXCL_LINE } static void @@ -274,7 +273,7 @@ SetOracle::doApply() auto const newCount = pairs.size() > 5 ? 2 : 1; auto const adjust = newCount - oldCount; if (adjust != 0 && !adjustOwnerCount(ctx_, adjust)) - return tefINTERNAL; + return tefINTERNAL; // LCOV_EXCL_LINE ctx_.view().update(sle); } @@ -295,13 +294,13 @@ SetOracle::doApply() auto page = ctx_.view().dirInsert( keylet::ownerDir(account_), sle->key(), describeOwnerDir(account_)); if (!page) - return tecDIR_FULL; + return tecDIR_FULL; // LCOV_EXCL_LINE (*sle)[sfOwnerNode] = *page; auto const count = series.size() > 5 ? 2 : 1; if (!adjustOwnerCount(ctx_, count)) - return tefINTERNAL; + return tefINTERNAL; // LCOV_EXCL_LINE ctx_.view().insert(sle); } diff --git a/src/ripple/rpc/handlers/GetAggregatePrice.cpp b/src/ripple/rpc/handlers/GetAggregatePrice.cpp index 5490cc4fcff..3554ba90c96 100644 --- a/src/ripple/rpc/handlers/GetAggregatePrice.cpp +++ b/src/ripple/rpc/handlers/GetAggregatePrice.cpp @@ -85,10 +85,11 @@ iteratePriceData( auto const ledger = context.ledgerMaster.getLedgerBySeq(prevSeq); if (!ledger) - return; + return; // LCOV_EXCL_LINE meta = ledger->txRead(prevTx).second; + prevChain = chain; for (STObject const& node : meta->getFieldArray(sfAffectedNodes)) { if (node.getFieldU16(sfLedgerEntryType) != ltORACLE) @@ -96,7 +97,6 @@ iteratePriceData( continue; } - prevChain = chain; chain = &node; isNew = node.isFieldPresent(sfNewFields); // if a meta is for the new and this is the first @@ -170,21 +170,49 @@ doGetAggregatePrice(RPC::JsonContext& context) if (!params.isMember(jss::quote_asset)) return RPC::missing_field_error(jss::quote_asset); + // Lambda to validate uint type + // support positive int, uint, and a number represented as a string + auto validUInt = [](Json::Value const& params, + Json::StaticString const& field) { + auto const& jv = params[field]; + std::uint32_t v; + return jv.isUInt() || (jv.isInt() && jv.asInt() >= 0) || + (jv.isString() && beast::lexicalCastChecked(v, jv.asString())); + }; + // Lambda to get `trim` and `time_threshold` fields. If the field // is not included in the input then a default value is returned. - auto getField = [¶ms]( + auto getField = [¶ms, &validUInt]( Json::StaticString const& field, unsigned int def = 0) -> std::variant { if (params.isMember(field)) { - if (!params[field].isConvertibleTo(Json::ValueType::uintValue)) - return rpcORACLE_MALFORMED; + if (!validUInt(params, field)) + return rpcINVALID_PARAMS; return params[field].asUInt(); } return def; }; + // Lambda to get `base_asset` and `quote_asset`. The values have + // to conform to the Currency type. + auto getCurrency = + [¶ms](SField const& sField, Json::StaticString const& field) + -> std::variant { + try + { + if (params[field].asString().empty()) + return rpcINVALID_PARAMS; + currencyFromJson(sField, params[field]); + return params[field]; + } + catch (...) + { + return rpcINVALID_PARAMS; + } + }; + auto const trim = getField(jss::trim); if (std::holds_alternative(trim)) { @@ -206,8 +234,18 @@ doGetAggregatePrice(RPC::JsonContext& context) return result; } - auto const& baseAsset = params[jss::base_asset]; - auto const& quoteAsset = params[jss::quote_asset]; + auto const baseAsset = getCurrency(sfBaseAsset, jss::base_asset); + if (std::holds_alternative(baseAsset)) + { + RPC::inject_error(std::get(baseAsset), result); + return result; + } + auto const quoteAsset = getCurrency(sfQuoteAsset, jss::quote_asset); + if (std::holds_alternative(quoteAsset)) + { + RPC::inject_error(std::get(quoteAsset), result); + return result; + } // Collect the dataset into bimap keyed by lastUpdateTime and // STAmount (Number is int64 and price is uint64) @@ -220,8 +258,7 @@ doGetAggregatePrice(RPC::JsonContext& context) RPC::inject_error(rpcORACLE_MALFORMED, result); return result; } - auto const documentID = oracle[jss::oracle_document_id].isConvertibleTo( - Json::ValueType::uintValue) + auto const documentID = validUInt(oracle, jss::oracle_document_id) ? std::make_optional(oracle[jss::oracle_document_id].asUInt()) : std::nullopt; auto const account = @@ -235,7 +272,7 @@ doGetAggregatePrice(RPC::JsonContext& context) std::shared_ptr ledger; result = RPC::lookupLedger(ledger, context); if (!ledger) - return result; + return result; // LCOV_EXCL_LINE auto const sle = ledger->read(keylet::oracle(*account, *documentID)); iteratePriceData(context, sle, [&](STObject const& node) { @@ -246,9 +283,9 @@ doGetAggregatePrice(RPC::JsonContext& context) series.end(), [&](STObject const& o) -> bool { return o.getFieldCurrency(sfBaseAsset).getText() == - baseAsset && + std::get(baseAsset) && o.getFieldCurrency(sfQuoteAsset).getText() == - quoteAsset && + std::get(quoteAsset) && o.isFieldPresent(sfAssetPrice); }); iter != series.end()) @@ -287,10 +324,15 @@ doGetAggregatePrice(RPC::JsonContext& context) prices.left.erase( prices.left.upper_bound(upperBound), prices.left.end()); + // At least one element should remain since upperBound is either + // equal to oldestTime or is less than latestTime, in which case + // the data is deleted between the oldestTime and upperBound. if (prices.empty()) { - RPC::inject_error(rpcOBJECT_NOT_FOUND, result); + // LCOV_EXCL_START + RPC::inject_error(rpcINTERNAL, result); return result; + // LCOV_EXCL_STOP } } result[jss::time] = latestTime; diff --git a/src/ripple/rpc/handlers/LedgerEntry.cpp b/src/ripple/rpc/handlers/LedgerEntry.cpp index dfbc32e606a..8985a880824 100644 --- a/src/ripple/rpc/handlers/LedgerEntry.cpp +++ b/src/ripple/rpc/handlers/LedgerEntry.cpp @@ -624,7 +624,7 @@ doLedgerEntry(RPC::JsonContext& context) auto const& oracle = context.params[jss::oracle]; auto const documentID = [&]() -> std::optional { auto const& id = oracle[jss::oracle_document_id]; - if (id.isConvertibleTo(Json::ValueType::uintValue)) + if (id.isUInt() || (id.isInt() && id.asInt() >= 0)) return std::make_optional(id.asUInt()); else if (id.isString()) { diff --git a/src/test/app/Oracle_test.cpp b/src/test/app/Oracle_test.cpp index f5488c793a1..16a72de70fd 100644 --- a/src/test/app/Oracle_test.cpp +++ b/src/test/app/Oracle_test.cpp @@ -163,7 +163,7 @@ struct Oracle_test : public beast::unit_test::suite env.fund(XRP(1'000), owner); Oracle oracle(env, {.owner = owner}, false); - // Symbol class or provider not included on create + // Asset class or provider not included on create oracle.set(CreateArg{ .assetClass = std::nullopt, .provider = "provider", @@ -174,7 +174,7 @@ struct Oracle_test : public beast::unit_test::suite .uri = "URI", .err = ter(temMALFORMED)}); - // Symbol class or provider are included on update + // Asset class or provider are included on update // and don't match the current values oracle.set(CreateArg{}); BEAST_EXPECT(oracle.exists()); @@ -194,7 +194,7 @@ struct Oracle_test : public beast::unit_test::suite Oracle oracle(env, {.owner = owner}, false); // Fields too long - // Symbol class + // Asset class std::string assetClass(17, '0'); oracle.set( CreateArg{.assetClass = assetClass, .err = ter(temMALFORMED)}); @@ -203,6 +203,13 @@ struct Oracle_test : public beast::unit_test::suite oracle.set(CreateArg{.provider = large, .err = ter(temMALFORMED)}); // URI oracle.set(CreateArg{.uri = large, .err = ter(temMALFORMED)}); + // Empty field + // Asset class + oracle.set(CreateArg{.assetClass = "", .err = ter(temMALFORMED)}); + // provider + oracle.set(CreateArg{.provider = "", .err = ter(temMALFORMED)}); + // URI + oracle.set(CreateArg{.uri = "", .err = ter(temMALFORMED)}); } { @@ -224,6 +231,12 @@ struct Oracle_test : public beast::unit_test::suite // Invalid update time using namespace std::chrono; Env env(*this); + auto closeTime = [&]() { + return duration_cast( + env.current()->info().closeTime.time_since_epoch() - + 10'000s) + .count(); + }; env.fund(XRP(1'000), owner); Oracle oracle(env, {.owner = owner}); BEAST_EXPECT(oracle.exists()); @@ -231,20 +244,25 @@ struct Oracle_test : public beast::unit_test::suite // Less than the last close time - 300s oracle.set(UpdateArg{ .series = {{"XRP", "USD", 740, 1}}, - .lastUpdateTime = testStartTime.count() + 400 - 301, + .lastUpdateTime = static_cast(closeTime() - 301), .err = ter(tecINVALID_UPDATE_TIME)}); // Greater than last close time + 300s oracle.set(UpdateArg{ .series = {{"XRP", "USD", 740, 1}}, - .lastUpdateTime = testStartTime.count() + 400 + 301, + .lastUpdateTime = static_cast(closeTime() + 311), .err = ter(tecINVALID_UPDATE_TIME)}); oracle.set(UpdateArg{.series = {{"XRP", "USD", 740, 1}}}); - BEAST_EXPECT( - oracle.expectLastUpdateTime(testStartTime.count() + 450)); + BEAST_EXPECT(oracle.expectLastUpdateTime( + static_cast(testStartTime.count() + 450))); // Less than the previous lastUpdateTime oracle.set(UpdateArg{ .series = {{"XRP", "USD", 740, 1}}, - .lastUpdateTime = testStartTime.count() + 449, + .lastUpdateTime = static_cast(449), + .err = ter(tecINVALID_UPDATE_TIME)}); + // Less than the epoch time + oracle.set(UpdateArg{ + .series = {{"XRP", "USD", 740, 1}}, + .lastUpdateTime = static_cast(epoch_offset.count() - 1), .err = ter(tecINVALID_UPDATE_TIME)}); } @@ -284,6 +302,38 @@ struct Oracle_test : public beast::unit_test::suite .series = {{"USD", "BTC", 740, maxPriceScale + 1}}, .err = ter(temMALFORMED)}); } + + { + // Updating token pair to add and delete + Env env(*this); + env.fund(XRP(1'000), owner); + Oracle oracle(env, {.owner = owner}); + oracle.set(UpdateArg{ + .series = + {{"XRP", "EUR", std::nullopt, std::nullopt}, + {"XRP", "EUR", 740, 1}}, + .err = ter(temMALFORMED)}); + // Delete token pair that doesn't exist in this oracle + oracle.set(UpdateArg{ + .series = {{"XRP", "EUR", std::nullopt, std::nullopt}}, + .err = ter(tecTOKEN_PAIR_NOT_FOUND)}); + // Delete token pair in oracle, which is not in the ledger + oracle.set(UpdateArg{ + .documentID = 10, + .series = {{"XRP", "EUR", std::nullopt, std::nullopt}}, + .err = ter(temMALFORMED)}); + } + + { + // Bad fee + Env env(*this); + env.fund(XRP(1'000), owner); + Oracle oracle( + env, {.owner = owner, .fee = -1, .err = ter(temBAD_FEE)}); + Oracle oracle1(env, {.owner = owner}); + oracle.set( + UpdateArg{.owner = owner, .fee = -1, .err = ter(temBAD_FEE)}); + } } void @@ -356,13 +406,19 @@ struct Oracle_test : public beast::unit_test::suite {.owner = bad, .seq = seq(1), .err = ter(terNO_ACCOUNT)}); } - // Invalid Sequence + // Invalid DocumentID oracle.remove({.documentID = 2, .err = ter(tecNO_ENTRY)}); // Invalid owner Account const invalid("invalid"); env.fund(XRP(1'000), invalid); oracle.remove({.owner = invalid, .err = ter(tecNO_ENTRY)}); + + // Invalid flags + oracle.remove({.flags = tfSellNFToken, .err = ter(temINVALID_FLAG)}); + + // Bad fee + oracle.remove({.fee = -1, .err = ter(temBAD_FEE)}); } void @@ -622,50 +678,6 @@ struct Oracle_test : public beast::unit_test::suite } } - void - testLedgerEntry() - { - testcase("Ledger Entry"); - using namespace jtx; - - Env env(*this); - std::vector accounts; - std::vector oracles; - for (int i = 0; i < 10; ++i) - { - Account const owner(std::string("owner") + std::to_string(i)); - env.fund(XRP(1'000), owner); - // different accounts can have the same asset pair - Oracle oracle(env, {.owner = owner, .documentID = i}); - accounts.push_back(owner.id()); - oracles.push_back(oracle.documentID()); - // same account can have different asset pair - Oracle oracle1(env, {.owner = owner, .documentID = i + 10}); - accounts.push_back(owner.id()); - oracles.push_back(oracle1.documentID()); - } - for (int i = 0; i < accounts.size(); ++i) - { - auto const jv = [&]() { - // document id is uint32 - if (i % 2) - return Oracle::ledgerEntry(env, accounts[i], oracles[i]); - // document id is string - return Oracle::ledgerEntry( - env, accounts[i], std::to_string(oracles[i])); - }(); - try - { - BEAST_EXPECT( - jv[jss::node][jss::Owner] == to_string(accounts[i])); - } - catch (...) - { - fail(); - } - } - } - public: void run() override @@ -683,7 +695,6 @@ struct Oracle_test : public beast::unit_test::suite all - featureMultiSignReserve - featureExpandedSignerList, all - featureExpandedSignerList}) testMultisig(features); - testLedgerEntry(); } }; diff --git a/src/test/jtx/Oracle.h b/src/test/jtx/Oracle.h index f6fdbbff34a..ac46b32d092 100644 --- a/src/test/jtx/Oracle.h +++ b/src/test/jtx/Oracle.h @@ -28,6 +28,28 @@ namespace test { namespace jtx { namespace oracle { +using AnyValue = std::variant; +using OraclesData = + std::vector, std::optional>>; + +// Special string value, which is converted to unquoted string in the string +// passed to rpc. +constexpr char const* NoneTag = "%None%"; +constexpr char const* UnquotedNone = "None"; +constexpr char const* NonePattern = "\"%None%\""; + +std::uint32_t +asUInt(AnyValue const& v); + +bool +validDocumentID(AnyValue const& v); + +void +toJson(Json::Value& jv, AnyValue const& v); + +void +toJsonHex(Json::Value& jv, AnyValue const& v); + // base asset, quote asset, price, scale using DataSeries = std::vector owner = std::nullopt; - std::optional documentID = 1; + std::optional documentID = 1; DataSeries series = {{"XRP", "USD", 740, 1}}; - std::optional assetClass = "currency"; - std::optional provider = "provider"; - std::optional uri = "URI"; - std::optional lastUpdateTime = std::nullopt; + std::optional assetClass = "currency"; + std::optional provider = "provider"; + std::optional uri = "URI"; + std::optional lastUpdateTime = std::nullopt; std::uint32_t flags = 0; std::optional msig = std::nullopt; std::optional seq = std::nullopt; - std::uint32_t fee = 10; + int fee = 10; std::optional err = std::nullopt; bool close = false; }; @@ -57,26 +79,27 @@ struct CreateArg struct UpdateArg { std::optional owner = std::nullopt; - std::optional documentID = std::nullopt; + std::optional documentID = std::nullopt; DataSeries series = {}; - std::optional assetClass = std::nullopt; - std::optional provider = std::nullopt; - std::optional uri = "URI"; - std::optional lastUpdateTime = std::nullopt; + std::optional assetClass = std::nullopt; + std::optional provider = std::nullopt; + std::optional uri = "URI"; + std::optional lastUpdateTime = std::nullopt; std::uint32_t flags = 0; std::optional msig = std::nullopt; std::optional seq = std::nullopt; - std::uint32_t fee = 10; + int fee = 10; std::optional err = std::nullopt; }; struct RemoveArg { std::optional const& owner = std::nullopt; - std::optional const& documentID = std::nullopt; + std::optional const& documentID = std::nullopt; + std::uint32_t flags = 0; std::optional const& msig = std::nullopt; std::optional seq = std::nullopt; - std::uint32_t fee = 10; + int fee = 10; std::optional const& err = std::nullopt; }; @@ -123,12 +146,11 @@ class Oracle static Json::Value aggregatePrice( Env& env, - std::optional const& baseAsset, - std::optional const& quoteAsset, - std::optional>> const& - oracles = std::nullopt, - std::optional const& trim = std::nullopt, - std::optional const& timeTreshold = std::nullopt); + std::optional const& baseAsset, + std::optional const& quoteAsset, + std::optional const& oracles = std::nullopt, + std::optional const& trim = std::nullopt, + std::optional const& timeTreshold = std::nullopt); std::uint32_t documentID() const @@ -154,8 +176,8 @@ class Oracle static Json::Value ledgerEntry( Env& env, - AccountID const& account, - std::variant const& documentID, + std::optional> const& account, + std::optional const& documentID, std::optional const& index = std::nullopt); Json::Value diff --git a/src/test/jtx/impl/Oracle.cpp b/src/test/jtx/impl/Oracle.cpp index 95da59952a0..34249a61228 100644 --- a/src/test/jtx/impl/Oracle.cpp +++ b/src/test/jtx/impl/Oracle.cpp @@ -22,6 +22,7 @@ #include #include +#include #include @@ -43,8 +44,8 @@ Oracle::Oracle(Env& env, CreateArg const& arg, bool submit) env_.close(now + testStartTime - epoch_offset); if (arg.owner) owner_ = *arg.owner; - if (arg.documentID) - documentID_ = *arg.documentID; + if (arg.documentID && validDocumentID(*arg.documentID)) + documentID_ = asUInt(*arg.documentID); if (submit) set(arg); } @@ -55,13 +56,15 @@ Oracle::remove(RemoveArg const& arg) Json::Value jv; jv[jss::TransactionType] = jss::OracleDelete; jv[jss::Account] = to_string(arg.owner.value_or(owner_)); - jv[jss::OracleDocumentID] = arg.documentID.value_or(documentID_); + toJson(jv[jss::OracleDocumentID], arg.documentID.value_or(documentID_)); if (Oracle::fee != 0) jv[jss::Fee] = std::to_string(Oracle::fee); else if (arg.fee != 0) jv[jss::Fee] = std::to_string(arg.fee); else jv[jss::Fee] = std::to_string(env_.current()->fees().increment.drops()); + if (arg.flags != 0) + jv[jss::Flags] = arg.flags; submit(jv, arg.msig, arg.seq, arg.err); } @@ -142,12 +145,11 @@ Oracle::expectLastUpdateTime(std::uint32_t lastUpdateTime) const Json::Value Oracle::aggregatePrice( Env& env, - std::optional const& baseAsset, - std::optional const& quoteAsset, - std::optional>> const& - oracles, - std::optional const& trim, - std::optional const& timeThreshold) + std::optional const& baseAsset, + std::optional const& quoteAsset, + std::optional const& oracles, + std::optional const& trim, + std::optional const& timeThreshold) { Json::Value jv; Json::Value jvOracles(Json::arrayValue); @@ -156,26 +158,34 @@ Oracle::aggregatePrice( for (auto const& id : *oracles) { Json::Value oracle; - oracle[jss::account] = to_string(id.first.id()); - oracle[jss::oracle_document_id] = id.second; + if (id.first) + oracle[jss::account] = to_string((*id.first).id()); + if (id.second) + toJson(oracle[jss::oracle_document_id], *id.second); jvOracles.append(oracle); } jv[jss::oracles] = jvOracles; } if (trim) - jv[jss::trim] = *trim; + toJson(jv[jss::trim], *trim); if (baseAsset) - jv[jss::base_asset] = *baseAsset; + toJson(jv[jss::base_asset], *baseAsset); if (quoteAsset) - jv[jss::quote_asset] = *quoteAsset; + toJson(jv[jss::quote_asset], *quoteAsset); if (timeThreshold) - jv[jss::time_threshold] = *timeThreshold; - - auto jr = env.rpc("json", "get_aggregate_price", to_string(jv)); + toJson(jv[jss::time_threshold], *timeThreshold); + // Convert "%None%" to None + auto str = to_string(jv); + str = boost::regex_replace(str, boost::regex(NonePattern), UnquotedNone); + auto jr = env.rpc("json", "get_aggregate_price", str); - if (jr.isObject() && jr.isMember(jss::result) && - jr[jss::result].isMember(jss::status)) - return jr[jss::result]; + if (jr.isObject()) + { + if (jr.isMember(jss::result) && jr[jss::result].isMember(jss::status)) + return jr[jss::result]; + else if (jr.isMember(jss::error)) + return jr; + } return Json::nullValue; } @@ -186,17 +196,24 @@ Oracle::set(UpdateArg const& arg) Json::Value jv; if (arg.owner) owner_ = *arg.owner; - if (arg.documentID) - documentID_ = *arg.documentID; + if (arg.documentID && + std::holds_alternative(*arg.documentID)) + { + documentID_ = std::get(*arg.documentID); + jv[jss::OracleDocumentID] = documentID_; + } + else if (arg.documentID) + toJson(jv[jss::OracleDocumentID], *arg.documentID); + else + jv[jss::OracleDocumentID] = documentID_; jv[jss::TransactionType] = jss::OracleSet; jv[jss::Account] = to_string(owner_); - jv[jss::OracleDocumentID] = documentID_; if (arg.assetClass) - jv[jss::AssetClass] = strHex(*arg.assetClass); + toJsonHex(jv[jss::AssetClass], *arg.assetClass); if (arg.provider) - jv[jss::Provider] = strHex(*arg.provider); + toJsonHex(jv[jss::Provider], *arg.provider); if (arg.uri) - jv[jss::URI] = strHex(*arg.uri); + toJsonHex(jv[jss::URI], *arg.uri); if (arg.flags != 0) jv[jss::Flags] = arg.flags; if (Oracle::fee != 0) @@ -207,8 +224,14 @@ Oracle::set(UpdateArg const& arg) jv[jss::Fee] = std::to_string(env_.current()->fees().increment.drops()); // lastUpdateTime if provided is offset from testStartTime if (arg.lastUpdateTime) - jv[jss::LastUpdateTime] = - to_string(testStartTime.count() + *arg.lastUpdateTime); + { + if (std::holds_alternative(*arg.lastUpdateTime)) + jv[jss::LastUpdateTime] = to_string( + testStartTime.count() + + std::get(*arg.lastUpdateTime)); + else + toJson(jv[jss::LastUpdateTime], *arg.lastUpdateTime); + } else jv[jss::LastUpdateTime] = to_string( duration_cast( @@ -263,18 +286,22 @@ Oracle::set(CreateArg const& arg) Json::Value Oracle::ledgerEntry( Env& env, - AccountID const& account, - std::variant const& documentID, + std::optional> const& account, + std::optional const& documentID, std::optional const& index) { Json::Value jvParams; - jvParams[jss::oracle][jss::account] = to_string(account); - if (std::holds_alternative(documentID)) - jvParams[jss::oracle][jss::oracle_document_id] = - std::get(documentID); - else - jvParams[jss::oracle][jss::oracle_document_id] = - std::get(documentID); + if (account) + { + if (std::holds_alternative(*account)) + jvParams[jss::oracle][jss::account] = + to_string(std::get(*account)); + else + jvParams[jss::oracle][jss::account] = + std::get(*account); + } + if (documentID) + toJson(jvParams[jss::oracle][jss::oracle_document_id], *documentID); if (index) { std::uint32_t i; @@ -283,7 +310,68 @@ Oracle::ledgerEntry( else jvParams[jss::oracle][jss::ledger_index] = *index; } - return env.rpc("json", "ledger_entry", to_string(jvParams))[jss::result]; + // Convert "%None%" to None + auto str = to_string(jvParams); + str = boost::regex_replace(str, boost::regex(NonePattern), UnquotedNone); + auto jr = env.rpc("json", "ledger_entry", str); + + if (jr.isObject()) + { + if (jr.isMember(jss::result) && jr[jss::result].isMember(jss::status)) + return jr[jss::result]; + else if (jr.isMember(jss::error)) + return jr; + } + return Json::nullValue; +} + +void +toJson(Json::Value& jv, AnyValue const& v) +{ + std::visit([&](auto&& arg) { jv = arg; }, v); +} + +void +toJsonHex(Json::Value& jv, AnyValue const& v) +{ + std::visit( + [&](T&& arg) { + if constexpr (std::is_same_v) + { + if (arg.starts_with("##")) + jv = arg.substr(2); + else + jv = strHex(arg); + } + else + jv = arg; + }, + v); +} + +std::uint32_t +asUInt(AnyValue const& v) +{ + Json::Value jv; + toJson(jv, v); + return jv.asUInt(); +} + +bool +validDocumentID(AnyValue const& v) +{ + try + { + Json::Value jv; + toJson(jv, v); + jv.asUInt(); + jv.isNumeric(); + return true; + } + catch (...) + { + } + return false; } } // namespace oracle diff --git a/src/test/rpc/GetAggregatePrice_test.cpp b/src/test/rpc/GetAggregatePrice_test.cpp index 4c45b7b9d2b..1fb263bbc55 100644 --- a/src/test/rpc/GetAggregatePrice_test.cpp +++ b/src/test/rpc/GetAggregatePrice_test.cpp @@ -35,10 +35,9 @@ class GetAggregatePrice_test : public beast::unit_test::suite { testcase("Errors"); using namespace jtx; - using Oracles = std::vector>; Account const owner{"owner"}; Account const some{"some"}; - static Oracles oracles = {{owner, 1}}; + static OraclesData oracles = {{owner, 1}}; { Env env(*this); @@ -55,6 +54,32 @@ class GetAggregatePrice_test : public beast::unit_test::suite ret[jss::error_message].asString() == "Missing field 'quote_asset'."); + // invalid base_asset, quote_asset + std::vector invalidAsset = { + NoneTag, + 1, + -1, + 1.2, + "", + "invalid", + "a", + "ab", + "A", + "AB", + "ABCD", + "010101", + "012345678901234567890123456789012345678", + "012345678901234567890123456789012345678G"}; + for (auto const& v : invalidAsset) + { + ret = Oracle::aggregatePrice(env, "USD", v, oracles); + BEAST_EXPECT(ret[jss::error].asString() == "invalidParams"); + ret = Oracle::aggregatePrice(env, v, "USD", oracles); + BEAST_EXPECT(ret[jss::error].asString() == "invalidParams"); + ret = Oracle::aggregatePrice(env, v, v, oracles); + BEAST_EXPECT(ret[jss::error].asString() == "invalidParams"); + } + // missing oracles array ret = Oracle::aggregatePrice(env, "XRP", "USD"); BEAST_EXPECT( @@ -62,16 +87,39 @@ class GetAggregatePrice_test : public beast::unit_test::suite "Missing field 'oracles'."); // empty oracles array - ret = Oracle::aggregatePrice(env, "XRP", "USD", Oracles{}); + ret = Oracle::aggregatePrice(env, "XRP", "USD", OraclesData{}); BEAST_EXPECT(ret[jss::error].asString() == "oracleMalformed"); + // no token pairs found + ret = Oracle::aggregatePrice(env, "YAN", "USD", oracles); + BEAST_EXPECT(ret[jss::error].asString() == "objectNotFound"); + // invalid oracle document id + // id doesn't exist ret = Oracle::aggregatePrice(env, "XRP", "USD", {{{owner, 2}}}); BEAST_EXPECT(ret[jss::error].asString() == "objectNotFound"); + // invalid values + std::vector invalidDocument = { + NoneTag, 1.2, -1, "", "none", "1.2"}; + for (auto const& v : invalidDocument) + { + ret = Oracle::aggregatePrice(env, "XRP", "USD", {{{owner, v}}}); + Json::Value jv; + toJson(jv, v); + BEAST_EXPECT(ret[jss::error].asString() == "invalidParams"); + } + // missing document id + ret = Oracle::aggregatePrice( + env, "XRP", "USD", {{{owner, std::nullopt}}}); + BEAST_EXPECT(ret[jss::error].asString() == "oracleMalformed"); // invalid owner ret = Oracle::aggregatePrice(env, "XRP", "USD", {{{some, 1}}}); BEAST_EXPECT(ret[jss::error].asString() == "objectNotFound"); + // missing account + ret = Oracle::aggregatePrice( + env, "XRP", "USD", {{{std::nullopt, 1}}}); + BEAST_EXPECT(ret[jss::error].asString() == "oracleMalformed"); // oracles have wrong asset pair env.fund(XRP(1'000), owner); @@ -82,18 +130,35 @@ class GetAggregatePrice_test : public beast::unit_test::suite BEAST_EXPECT(ret[jss::error].asString() == "objectNotFound"); // invalid trim value - ret = Oracle::aggregatePrice( - env, "XRP", "USD", {{{owner, oracle.documentID()}}}, 0); - BEAST_EXPECT(ret[jss::error].asString() == "invalidParams"); - ret = Oracle::aggregatePrice( - env, "XRP", "USD", {{{owner, oracle.documentID()}}}, 26); - BEAST_EXPECT(ret[jss::error].asString() == "invalidParams"); + std::vector invalidTrim = { + NoneTag, 0, 26, -1, 1.2, "", "none", "1.2"}; + for (auto const& v : invalidTrim) + { + ret = Oracle::aggregatePrice( + env, "XRP", "USD", {{{owner, oracle.documentID()}}}, v); + BEAST_EXPECT(ret[jss::error].asString() == "invalidParams"); + } + + // invalid time threshold value + std::vector invalidTime = { + NoneTag, -1, 1.2, "", "none", "1.2"}; + for (auto const& v : invalidTime) + { + ret = Oracle::aggregatePrice( + env, + "XRP", + "USD", + {{{owner, oracle.documentID()}}}, + std::nullopt, + v); + BEAST_EXPECT(ret[jss::error].asString() == "invalidParams"); + } } // too many oracles { Env env(*this); - std::vector> oracles; + OraclesData oracles; for (int i = 0; i < 201; ++i) { Account const owner(std::to_string(i)); @@ -132,7 +197,7 @@ class GetAggregatePrice_test : public beast::unit_test::suite // or time threshold { Env env(*this); - std::vector> oracles; + OraclesData oracles; prep(env, oracles); // entire and trimmed stats auto ret = Oracle::aggregatePrice(env, "XRP", "USD", oracles); @@ -148,7 +213,7 @@ class GetAggregatePrice_test : public beast::unit_test::suite // Aggregate data set includes all price oracle instances { Env env(*this); - std::vector> oracles; + OraclesData oracles; prep(env, oracles); // entire and trimmed stats auto ret = @@ -171,14 +236,14 @@ class GetAggregatePrice_test : public beast::unit_test::suite // updated ledgers { Env env(*this); - std::vector> oracles; + OraclesData oracles; prep(env, oracles); for (int i = 0; i < 3; ++i) { Oracle oracle( env, {.owner = oracles[i].first, - .documentID = oracles[i].second}, + .documentID = asUInt(*oracles[i].second)}, false); // push XRP/USD by more than three ledgers, so this price // oracle is not included in the dataset @@ -191,7 +256,7 @@ class GetAggregatePrice_test : public beast::unit_test::suite Oracle oracle( env, {.owner = oracles[i].first, - .documentID = oracles[i].second}, + .documentID = asUInt(*oracles[i].second)}, false); // push XRP/USD by two ledgers, so this price // is included in the dataset @@ -201,7 +266,7 @@ class GetAggregatePrice_test : public beast::unit_test::suite // entire and trimmed stats auto ret = - Oracle::aggregatePrice(env, "XRP", "USD", oracles, 20, 200); + Oracle::aggregatePrice(env, "XRP", "USD", oracles, 20, "200"); BEAST_EXPECT(ret[jss::entire_set][jss::mean] == "74.6"); BEAST_EXPECT(ret[jss::entire_set][jss::size].asUInt() == 7); BEAST_EXPECT( @@ -219,14 +284,14 @@ class GetAggregatePrice_test : public beast::unit_test::suite // Reduced data set because of the time threshold { Env env(*this); - std::vector> oracles; + OraclesData oracles; prep(env, oracles); for (int i = 0; i < oracles.size(); ++i) { Oracle oracle( env, {.owner = oracles[i].first, - .documentID = oracles[i].second}, + .documentID = asUInt(*oracles[i].second)}, false); // push XRP/USD by two ledgers, so this price // is included in the dataset diff --git a/src/test/rpc/LedgerRPC_test.cpp b/src/test/rpc/LedgerRPC_test.cpp index 877b8ef2a02..79f244c4711 100644 --- a/src/test/rpc/LedgerRPC_test.cpp +++ b/src/test/rpc/LedgerRPC_test.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -2279,6 +2280,87 @@ class LedgerRPC_test : public beast::unit_test::suite } } + void + testInvalidOracleLedgerEntry() + { + testcase("Invalid Oracle Ledger Entry"); + using namespace ripple::test::jtx; + using namespace ripple::test::jtx::oracle; + + Env env(*this); + Account const owner("owner"); + env.fund(XRP(1'000), owner); + Oracle oracle(env, {.owner = owner}); + + // Malformed document id + auto res = Oracle::ledgerEntry(env, owner, NoneTag); + BEAST_EXPECT(res[jss::error].asString() == "invalidParams"); + std::vector invalid = {-1, 1.2, "", "Invalid"}; + for (auto const& v : invalid) + { + auto const res = Oracle::ledgerEntry(env, owner, v); + BEAST_EXPECT(res[jss::error].asString() == "malformedDocumentID"); + } + // Missing document id + res = Oracle::ledgerEntry(env, owner, std::nullopt); + BEAST_EXPECT(res[jss::error].asString() == "malformedRequest"); + + // Missing account + res = Oracle::ledgerEntry(env, std::nullopt, 1); + BEAST_EXPECT(res[jss::error].asString() == "malformedRequest"); + + // Malformed account + std::string malfAccount = to_string(owner.id()); + malfAccount.replace(10, 1, 1, '!'); + res = Oracle::ledgerEntry(env, malfAccount, 1); + BEAST_EXPECT(res[jss::error].asString() == "malformedAddress"); + } + + void + testOracleLedgerEntry() + { + testcase("Oracle Ledger Entry"); + using namespace ripple::test::jtx; + using namespace ripple::test::jtx::oracle; + + Env env(*this); + std::vector accounts; + std::vector oracles; + for (int i = 0; i < 10; ++i) + { + Account const owner(std::string("owner") + std::to_string(i)); + env.fund(XRP(1'000), owner); + // different accounts can have the same asset pair + Oracle oracle(env, {.owner = owner, .documentID = i}); + accounts.push_back(owner.id()); + oracles.push_back(oracle.documentID()); + // same account can have different asset pair + Oracle oracle1(env, {.owner = owner, .documentID = i + 10}); + accounts.push_back(owner.id()); + oracles.push_back(oracle1.documentID()); + } + for (int i = 0; i < accounts.size(); ++i) + { + auto const jv = [&]() { + // document id is uint32 + if (i % 2) + return Oracle::ledgerEntry(env, accounts[i], oracles[i]); + // document id is string + return Oracle::ledgerEntry( + env, accounts[i], std::to_string(oracles[i])); + }(); + try + { + BEAST_EXPECT( + jv[jss::node][jss::Owner] == to_string(accounts[i])); + } + catch (...) + { + fail(); + } + } + } + public: void run() override @@ -2304,6 +2386,8 @@ class LedgerRPC_test : public beast::unit_test::suite testQueue(); testLedgerAccountsOption(); testLedgerEntryDID(); + testInvalidOracleLedgerEntry(); + testOracleLedgerEntry(); forAllApiVersions(std::bind_front( &LedgerRPC_test::testLedgerEntryInvalidParams, this)); From 244ac5e024f7151128db0288ab3c2bda5401f496 Mon Sep 17 00:00:00 2001 From: Denis Angell Date: Mon, 13 May 2024 16:54:34 +0200 Subject: [PATCH 06/11] Update CONTRIBUTING.md (#4904) --- CONTRIBUTING.md | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ed4656651cf..75616eb6b4e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -197,15 +197,39 @@ existing maintainer without a vote. ## Current Maintainers +Maintainers are users with admin access to the repo. Maintainers do not typically approve or deny pull requests. + +* [intelliot](https://github.com/intelliot) (Ripple) * [JoelKatz](https://github.com/JoelKatz) (Ripple) -* [manojsdoshi](https://github.com/manojsdoshi) (Ripple) -* [n3tc4t](https://github.com/n3tc4t) (XRPL Labs) * [nixer89](https://github.com/nixer89) (XRP Ledger Foundation) -* [RichardAH](https://github.com/RichardAH) (XRPL Labs + XRP Ledger Foundation) -* [seelabs](https://github.com/seelabs) (Ripple) * [Silkjaer](https://github.com/Silkjaer) (XRP Ledger Foundation) * [WietseWind](https://github.com/WietseWind) (XRPL Labs + XRP Ledger Foundation) + +## Current Code Reviewers + +Code Reviewers are developers who have the ability to review and approve source code changes. + +* [HowardHinnant](https://github.com/HowardHinnant) (Ripple) +* [scottschurr](https://github.com/scottschurr) (Ripple) +* [seelabs](https://github.com/seelabs) (Ripple) * [Ed Hennis](https://github.com/ximinez) (Ripple) +* [mvadari](https://github.com/mvadari) (Ripple) +* [thejohnfreeman](https://github.com/thejohnfreeman) (Ripple) +* [Bronek](https://github.com/Bronek) (Ripple) +* [manojsdoshi](https://github.com/manojsdoshi) (Ripple) +* [godexsoft](https://github.com/godexsoft) (Ripple) +* [mDuo13](https://github.com/mDuo13) (Ripple) +* [ckniffen](https://github.com/ckniffen) (Ripple) +* [arihantkothari](https://github.com/arihantkothari) (Ripple) +* [pwang200](https://github.com/pwang200) (Ripple) +* [sophiax851](https://github.com/sophiax851) (Ripple) +* [shawnxie999](https://github.com/shawnxie999) (Ripple) +* [gregtatcam](https://github.com/gregtatcam) (Ripple) +* [mtrippled](https://github.com/mtrippled) (Ripple) +* [ckeshava](https://github.com/ckeshava) (Ripple) +* [nbougalis](https://github.com/nbougalis) None +* [RichardAH](https://github.com/RichardAH) (XRPL Labs + XRP Ledger Foundation) +* [dangell7](https://github.com/dangell7) (XRPL Labs) [1]: https://docs.github.com/en/get-started/quickstart/contributing-to-projects From 2705109592a56cb9800dca7664be91fc8cecc9de Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Tue, 14 May 2024 15:28:38 -0400 Subject: [PATCH 07/11] Add the fixAMMOfferRounding amendment: (#4983) * Fix AMM offer rounding and low quality LOB offer blocking AMM: A single-path AMM offer with account offer on DEX, is always generated starting with the takerPays first, which is rounded up, and then the takerGets, which is rounded down. This rounding ensures that the pool's product invariant is maintained. However, when one of the offer's side is XRP, this rounding can result in the AMM offer having a lower quality, potentially causing offer generation to fail if the quality is lower than the account's offer quality. To address this issue, the proposed fix adjusts the offer generation process to start with the XRP side first and always rounds it down. This results in a smaller offer size, improving the offer's quality. Regardless if the offer has XRP or not, the rounding is done so that the offer size is minimized. This change still ensures the product invariant, as the other generated side is the exact result of the swap-in or swap-out equations. If a liquidity can be provided by both AMM and LOB offer on offer crossing then AMM offer is generated so that it matches LOB offer quality. If LOB offer quality is less than limit quality then generated AMM offer quality is also less than limit quality and the offer doesn't cross. To address this issue, if LOB quality is better than limit quality then use LOB quality to generate AMM offer. Otherwise, don't use the quality to generate AMM offer. In this case, limitOut() function in StrandFlow limits the out amount to match strand's quality to limit quality and consume maximum AMM liquidity. --- src/ripple/app/misc/AMMHelpers.h | 329 +++++++++-- src/ripple/app/misc/impl/AMMHelpers.cpp | 15 + src/ripple/app/paths/impl/AMMLiquidity.cpp | 11 +- src/ripple/app/paths/impl/BookStep.cpp | 79 ++- src/ripple/app/tx/impl/AMMCreate.cpp | 4 +- src/ripple/basics/Number.h | 20 + src/ripple/ledger/impl/View.cpp | 2 +- src/ripple/protocol/Feature.h | 2 +- src/ripple/protocol/impl/Feature.cpp | 2 +- src/ripple/protocol/impl/STAmount.cpp | 20 - src/test/app/AMMCalc_test.cpp | 7 +- src/test/app/AMMExtended_test.cpp | 97 ++-- src/test/app/AMM_test.cpp | 620 ++++++++++++++++++--- 13 files changed, 986 insertions(+), 222 deletions(-) diff --git a/src/ripple/app/misc/AMMHelpers.h b/src/ripple/app/misc/AMMHelpers.h index 7bdaf23d69f..787bb2300a3 100644 --- a/src/ripple/app/misc/AMMHelpers.h +++ b/src/ripple/app/misc/AMMHelpers.h @@ -21,7 +21,9 @@ #define RIPPLE_APP_MISC_AMMHELPERS_H_INCLUDED #include +#include #include +#include #include #include #include @@ -35,6 +37,20 @@ namespace ripple { +namespace detail { + +Number +reduceOffer(auto const& amount) +{ + static Number const reducedOfferPct(9999, -4); + + // Make sure the result is always less than amount or zero. + NumberRoundModeGuard mg(Number::towards_zero); + return amount * reducedOfferPct; +} + +} // namespace detail + /** Calculate LP Tokens given AMM pool reserves. * @param asset1 AMM one side of the pool reserve * @param asset2 AMM another side of the pool reserve @@ -147,12 +163,165 @@ withinRelativeDistance(Amt const& calc, Amt const& req, Number const& dist) } // clang-format on -/** Finds takerPays (i) and takerGets (o) such that given pool composition - * poolGets(I) and poolPays(O): (O - o) / (I + i) = quality. - * Where takerGets is calculated as the swapAssetIn (see below). - * The above equation produces the quadratic equation: - * i^2*(1-fee) + i*I*(2-fee) + I^2 - I*O/quality, - * which is solved for i, and o is found with swapAssetIn(). +/** Solve quadratic equation to find takerGets or takerPays. Round + * to minimize the amount in order to maximize the quality. + */ +std::optional +solveQuadraticEqSmallest(Number const& a, Number const& b, Number const& c); + +/** Generate AMM offer starting with takerGets when AMM pool + * from the payment perspective is IOU(in)/XRP(out) + * Equations: + * Spot Price Quality after the offer is consumed: + * Qsp = (O - o) / (I + i) -- equation (1) + * where O is poolPays, I is poolGets, o is takerGets, i is takerPays + * Swap out: + * i = (I * o) / (O - o) * f -- equation (2) + * where f is (1 - tfee/100000), tfee is in basis points + * Effective price targetQuality: + * Qep = o / i -- equation (3) + * There are two scenarios to consider + * A) Qsp = Qep. Substitute i in (1) with (2) and solve for o + * and Qsp = targetQuality(Qt): + * o**2 + o * (I * Qt * (1 - 1 / f) - 2 * O) + O**2 - Qt * I * O = 0 + * B) Qep = Qsp. Substitute i in (3) with (2) and solve for o + * and Qep = targetQuality(Qt): + * o = O - I * Qt / f + * Since the scenario is not known a priori, both A and B are solved and + * the lowest value of o is takerGets. takerPays is calculated with + * swap out eq (2). If o is less or equal to 0 then the offer can't + * be generated. + */ +template +std::optional> +getAMMOfferStartWithTakerGets( + TAmounts const& pool, + Quality const& targetQuality, + std::uint16_t const& tfee) +{ + if (targetQuality.rate() == beast::zero) + return std::nullopt; + + NumberRoundModeGuard mg(Number::to_nearest); + auto const f = feeMult(tfee); + auto const a = 1; + auto const b = pool.in * (1 - 1 / f) / targetQuality.rate() - 2 * pool.out; + auto const c = + pool.out * pool.out - (pool.in * pool.out) / targetQuality.rate(); + + auto nTakerGets = solveQuadraticEqSmallest(a, b, c); + if (!nTakerGets || *nTakerGets <= 0) + return std::nullopt; // LCOV_EXCL_LINE + + auto const nTakerGetsConstraint = + pool.out - pool.in / (targetQuality.rate() * f); + if (nTakerGetsConstraint <= 0) + return std::nullopt; + + // Select the smallest to maximize the quality + if (nTakerGetsConstraint < *nTakerGets) + nTakerGets = nTakerGetsConstraint; + + auto getAmounts = [&pool, &tfee](Number const& nTakerGetsProposed) { + // Round downward to minimize the offer and to maximize the quality. + // This has the most impact when takerGets is XRP. + auto const takerGets = toAmount( + getIssue(pool.out), nTakerGetsProposed, Number::downward); + return TAmounts{ + swapAssetOut(pool, takerGets, tfee), takerGets}; + }; + + // Try to reduce the offer size to improve the quality. + // The quality might still not match the targetQuality for a tiny offer. + if (auto const amounts = getAmounts(*nTakerGets); + Quality{amounts} < targetQuality) + return getAmounts(detail::reduceOffer(amounts.out)); + else + return amounts; +} + +/** Generate AMM offer starting with takerPays when AMM pool + * from the payment perspective is XRP(in)/IOU(out) or IOU(in)/IOU(out). + * Equations: + * Spot Price Quality after the offer is consumed: + * Qsp = (O - o) / (I + i) -- equation (1) + * where O is poolPays, I is poolGets, o is takerGets, i is takerPays + * Swap in: + * o = (O * i * f) / (I + i * f) -- equation (2) + * where f is (1 - tfee/100000), tfee is in basis points + * Effective price quality: + * Qep = o / i -- equation (3) + * There are two scenarios to consider + * A) Qsp = Qep. Substitute o in (1) with (2) and solve for i + * and Qsp = targetQuality(Qt): + * i**2 * f + i * I * (1 + f) + I**2 - I * O / Qt = 0 + * B) Qep = Qsp. Substitute i in (3) with (2) and solve for i + * and Qep = targetQuality(Qt): + * i = O / Qt - I / f + * Since the scenario is not known a priori, both A and B are solved and + * the lowest value of i is takerPays. takerGets is calculated with + * swap in eq (2). If i is less or equal to 0 then the offer can't + * be generated. + */ +template +std::optional> +getAMMOfferStartWithTakerPays( + TAmounts const& pool, + Quality const& targetQuality, + std::uint16_t tfee) +{ + if (targetQuality.rate() == beast::zero) + return std::nullopt; + + NumberRoundModeGuard mg(Number::to_nearest); + auto const f = feeMult(tfee); + auto const& a = f; + auto const b = pool.in * (1 + f); + auto const c = + pool.in * pool.in - pool.in * pool.out * targetQuality.rate(); + + auto nTakerPays = solveQuadraticEqSmallest(a, b, c); + if (!nTakerPays || nTakerPays <= 0) + return std::nullopt; // LCOV_EXCL_LINE + + auto const nTakerPaysConstraint = + pool.out * targetQuality.rate() - pool.in / f; + if (nTakerPaysConstraint <= 0) + return std::nullopt; + + // Select the smallest to maximize the quality + if (nTakerPaysConstraint < *nTakerPays) + nTakerPays = nTakerPaysConstraint; + + auto getAmounts = [&pool, &tfee](Number const& nTakerPaysProposed) { + // Round downward to minimize the offer and to maximize the quality. + // This has the most impact when takerPays is XRP. + auto const takerPays = toAmount( + getIssue(pool.in), nTakerPaysProposed, Number::downward); + return TAmounts{ + takerPays, swapAssetIn(pool, takerPays, tfee)}; + }; + + // Try to reduce the offer size to improve the quality. + // The quality might still not match the targetQuality for a tiny offer. + if (auto const amounts = getAmounts(*nTakerPays); + Quality{amounts} < targetQuality) + return getAmounts(detail::reduceOffer(amounts.in)); + else + return amounts; +} + +/** Generate AMM offer so that either updated Spot Price Quality (SPQ) + * is equal to LOB quality (in this case AMM offer quality is + * better than LOB quality) or AMM offer is equal to LOB quality + * (in this case SPQ is better than LOB quality). + * Pre-amendment code calculates takerPays first. If takerGets is XRP, + * it is rounded down, which results in worse offer quality than + * LOB quality, and the offer might fail to generate. + * Post-amendment code calculates the XRP offer side first. The result + * is rounded down, which makes the offer quality better. + * It might not be possible to match either SPQ or AMM offer to LOB + * quality. This generally happens at higher fees. * @param pool AMM pool balances * @param quality requested quality * @param tfee trading fee in basis points @@ -163,43 +332,111 @@ std::optional> changeSpotPriceQuality( TAmounts const& pool, Quality const& quality, - std::uint16_t tfee) + std::uint16_t tfee, + Rules const& rules, + beast::Journal j) { - auto const f = feeMult(tfee); // 1 - fee - auto const& a = f; - auto const b = pool.in * (1 + f); - Number const c = pool.in * pool.in - pool.in * pool.out * quality.rate(); - if (auto const res = b * b - 4 * a * c; res < 0) + if (!rules.enabled(fixAMMv1_1)) + { + // Finds takerPays (i) and takerGets (o) such that given pool + // composition poolGets(I) and poolPays(O): (O - o) / (I + i) = quality. + // Where takerGets is calculated as the swapAssetIn (see below). + // The above equation produces the quadratic equation: + // i^2*(1-fee) + i*I*(2-fee) + I^2 - I*O/quality, + // which is solved for i, and o is found with swapAssetIn(). + auto const f = feeMult(tfee); // 1 - fee + auto const& a = f; + auto const b = pool.in * (1 + f); + Number const c = + pool.in * pool.in - pool.in * pool.out * quality.rate(); + if (auto const res = b * b - 4 * a * c; res < 0) + return std::nullopt; // LCOV_EXCL_LINE + else if (auto const nTakerPaysPropose = (-b + root2(res)) / (2 * a); + nTakerPaysPropose > 0) + { + auto const nTakerPays = [&]() { + // The fee might make the AMM offer quality less than CLOB + // quality. Therefore, AMM offer has to satisfy this constraint: + // o / i >= q. Substituting o with swapAssetIn() gives: i <= O / + // q - I / (1 - fee). + auto const nTakerPaysConstraint = + pool.out * quality.rate() - pool.in / f; + if (nTakerPaysPropose > nTakerPaysConstraint) + return nTakerPaysConstraint; + return nTakerPaysPropose; + }(); + if (nTakerPays <= 0) + { + JLOG(j.trace()) + << "changeSpotPriceQuality calc failed: " + << to_string(pool.in) << " " << to_string(pool.out) << " " + << quality << " " << tfee; + return std::nullopt; + } + auto const takerPays = + toAmount(getIssue(pool.in), nTakerPays, Number::upward); + // should not fail + if (auto const amounts = + TAmounts{ + takerPays, swapAssetIn(pool, takerPays, tfee)}; + Quality{amounts} < quality && + !withinRelativeDistance( + Quality{amounts}, quality, Number(1, -7))) + { + JLOG(j.error()) + << "changeSpotPriceQuality failed: " << to_string(pool.in) + << " " << to_string(pool.out) << " " + << " " << quality << " " << tfee << " " + << to_string(amounts.in) << " " << to_string(amounts.out); + Throw("changeSpotPriceQuality failed"); + } + else + { + JLOG(j.trace()) + << "changeSpotPriceQuality succeeded: " + << to_string(pool.in) << " " << to_string(pool.out) << " " + << " " << quality << " " << tfee << " " + << to_string(amounts.in) << " " << to_string(amounts.out); + return amounts; + } + } + JLOG(j.trace()) << "changeSpotPriceQuality calc failed: " + << to_string(pool.in) << " " << to_string(pool.out) + << " " << quality << " " << tfee; return std::nullopt; - else if (auto const nTakerPaysPropose = (-b + root2(res)) / (2 * a); - nTakerPaysPropose > 0) + } + + // Generate the offer starting with XRP side. Return seated offer amounts + // if the offer can be generated, otherwise nullopt. + auto const amounts = [&]() { + if (isXRP(getIssue(pool.out))) + return getAMMOfferStartWithTakerGets(pool, quality, tfee); + return getAMMOfferStartWithTakerPays(pool, quality, tfee); + }(); + if (!amounts) { - auto const nTakerPays = [&]() { - // The fee might make the AMM offer quality less than CLOB quality. - // Therefore, AMM offer has to satisfy this constraint: o / i >= q. - // Substituting o with swapAssetIn() gives: - // i <= O / q - I / (1 - fee). - auto const nTakerPaysConstraint = - pool.out * quality.rate() - pool.in / f; - if (nTakerPaysPropose > nTakerPaysConstraint) - return nTakerPaysConstraint; - return nTakerPaysPropose; - }(); - if (nTakerPays <= 0) - return std::nullopt; - auto const takerPays = toAmount( - getIssue(pool.in), nTakerPays, Number::rounding_mode::upward); - // should not fail - if (auto const amounts = - TAmounts{ - takerPays, swapAssetIn(pool, takerPays, tfee)}; - Quality{amounts} < quality && - !withinRelativeDistance(Quality{amounts}, quality, Number(1, -7))) - Throw("changeSpotPriceQuality failed"); - else - return amounts; + JLOG(j.trace()) << "changeSpotPrice calc failed: " << to_string(pool.in) + << " " << to_string(pool.out) << " " << quality << " " + << tfee << std::endl; + return std::nullopt; } - return std::nullopt; + + if (Quality{*amounts} < quality) + { + JLOG(j.error()) << "changeSpotPriceQuality failed: " + << to_string(pool.in) << " " << to_string(pool.out) + << " " << quality << " " << tfee << " " + << to_string(amounts->in) << " " + << to_string(amounts->out); + return std::nullopt; + } + + JLOG(j.trace()) << "changeSpotPriceQuality succeeded: " + << to_string(pool.in) << " " << to_string(pool.out) << " " + << " " << quality << " " << tfee << " " + << to_string(amounts->in) << " " << to_string(amounts->out); + + return amounts; } /** AMM pool invariant - the product (A * B) after swap in/out has to remain @@ -231,7 +468,7 @@ swapAssetIn( std::uint16_t tfee) { if (auto const& rules = getCurrentTransactionRules(); - rules && rules->enabled(fixAMMRounding)) + rules && rules->enabled(fixAMMv1_1)) { // set rounding to always favor the amm. Clip to zero. // calculate: @@ -275,8 +512,7 @@ swapAssetIn( if (swapOut.signum() < 0) return toAmount(getIssue(pool.out), 0); - return toAmount( - getIssue(pool.out), swapOut, Number::rounding_mode::downward); + return toAmount(getIssue(pool.out), swapOut, Number::downward); } else { @@ -284,7 +520,7 @@ swapAssetIn( getIssue(pool.out), pool.out - (pool.in * pool.out) / (pool.in + assetIn * feeMult(tfee)), - Number::rounding_mode::downward); + Number::downward); } } @@ -305,7 +541,7 @@ swapAssetOut( std::uint16_t tfee) { if (auto const& rules = getCurrentTransactionRules(); - rules && rules->enabled(fixAMMRounding)) + rules && rules->enabled(fixAMMv1_1)) { // set rounding to always favor the amm. Clip to zero. // calculate: @@ -349,8 +585,7 @@ swapAssetOut( if (swapIn.signum() < 0) return toAmount(getIssue(pool.in), 0); - return toAmount( - getIssue(pool.in), swapIn, Number::rounding_mode::upward); + return toAmount(getIssue(pool.in), swapIn, Number::upward); } else { @@ -358,7 +593,7 @@ swapAssetOut( getIssue(pool.in), ((pool.in * pool.out) / (pool.out - assetOut) - pool.in) / feeMult(tfee), - Number::rounding_mode::upward); + Number::upward); } } diff --git a/src/ripple/app/misc/impl/AMMHelpers.cpp b/src/ripple/app/misc/impl/AMMHelpers.cpp index 736743eaaf7..57b3d3a07d3 100644 --- a/src/ripple/app/misc/impl/AMMHelpers.cpp +++ b/src/ripple/app/misc/impl/AMMHelpers.cpp @@ -203,4 +203,19 @@ solveQuadraticEq(Number const& a, Number const& b, Number const& c) return (-b + root2(b * b - 4 * a * c)) / (2 * a); } +// Minimize takerGets or takerPays +std::optional +solveQuadraticEqSmallest(Number const& a, Number const& b, Number const& c) +{ + auto const d = b * b - 4 * a * c; + if (d < 0) + return std::nullopt; + // use numerically stable citardauq formula for quadratic equation solution + // https://people.csail.mit.edu/bkph/articles/Quadratics.pdf + if (b > 0) + return (2 * c) / (-b - root2(d)); + else + return (2 * c) / (-b + root2(d)); +} + } // namespace ripple diff --git a/src/ripple/app/paths/impl/AMMLiquidity.cpp b/src/ripple/app/paths/impl/AMMLiquidity.cpp index bcc086e23da..9ec23d08a1a 100644 --- a/src/ripple/app/paths/impl/AMMLiquidity.cpp +++ b/src/ripple/app/paths/impl/AMMLiquidity.cpp @@ -205,8 +205,8 @@ AMMLiquidity::getOffer( return maxOffer(balances, view.rules()); } else if ( - auto const amounts = - changeSpotPriceQuality(balances, *clobQuality, tradingFee_)) + auto const amounts = changeSpotPriceQuality( + balances, *clobQuality, tradingFee_, view.rules(), j_)) { return AMMOffer( *this, *amounts, balances, Quality{*amounts}); @@ -239,7 +239,12 @@ AMMLiquidity::getOffer( return offer; } - JLOG(j_.error()) << "AMMLiquidity::getOffer, failed"; + JLOG(j_.error()) << "AMMLiquidity::getOffer, failed " + << ammContext_.multiPath() << " " + << ammContext_.curIters() << " " + << (clobQuality ? clobQuality->rate() : STAmount{}) + << " " << to_string(balances.in) << " " + << to_string(balances.out); } return std::nullopt; diff --git a/src/ripple/app/paths/impl/BookStep.cpp b/src/ripple/app/paths/impl/BookStep.cpp index 358dac4c796..9d5ceea82a8 100644 --- a/src/ripple/app/paths/impl/BookStep.cpp +++ b/src/ripple/app/paths/impl/BookStep.cpp @@ -297,6 +297,14 @@ class BookPaymentStep : public BookStep> return true; } + // A payment doesn't use quality threshold (limitQuality) + // since the strand's quality doesn't directly relate to the step's quality. + std::optional + qualityThreshold(Quality const& lobQuality) const + { + return lobQuality; + } + // For a payment ofrInRate is always the same as trIn. std::uint32_t getOfrInRate(Step const*, AccountID const&, std::uint32_t trIn) const @@ -450,6 +458,25 @@ class BookOfferCrossingStep return !defaultPath_ || quality >= qualityThreshold_; } + // Return quality threshold or nullopt to use when generating AMM offer. + // AMM synthetic offer is generated to match LOB offer quality. + // If LOB tip offer quality is less than qualityThreshold + // then generated AMM offer quality is also less than qualityThreshold and + // the offer is not crossed even though AMM might generate a better quality + // offer. To address this, if qualityThreshold is greater than lobQuality + // then don't use quality to generate the AMM offer. The limit out value + // generates the maximum AMM offer in this case, which matches + // the quality threshold. This only applies to single path scenario. + // Multi-path AMM offers work the same as LOB offers. + std::optional + qualityThreshold(Quality const& lobQuality) const + { + if (this->ammLiquidity_ && !this->ammLiquidity_->multiPath() && + qualityThreshold_ > lobQuality) + return std::nullopt; + return lobQuality; + } + // For offer crossing don't pay the transfer fee if alice is paying alice. // A regular (non-offer-crossing) payment does not apply this rule. std::uint32_t @@ -758,8 +785,16 @@ BookStep::forEachOffer( }; // At any payment engine iteration, AMM offer can only be consumed once. - auto tryAMM = [&](std::optional const& quality) -> bool { - auto ammOffer = getAMMOffer(sb, quality); + auto tryAMM = [&](std::optional const& lobQuality) -> bool { + // If offer crossing then use either LOB quality or nullopt + // to prevent AMM being blocked by a lower quality LOB. + auto const qualityThreshold = [&]() -> std::optional { + if (sb.rules().enabled(fixAMMv1_1) && lobQuality) + return static_cast(this)->qualityThreshold( + *lobQuality); + return lobQuality; + }(); + auto ammOffer = getAMMOffer(sb, qualityThreshold); return !ammOffer || execOffer(*ammOffer); }; @@ -776,7 +811,7 @@ BookStep::forEachOffer( } else { - // Might have AMM offer if there is no CLOB offers. + // Might have AMM offer if there are no LOB offers. tryAMM(std::nullopt); } @@ -851,17 +886,37 @@ BookStep::tip(ReadView const& view) const // This can be simplified (and sped up) if directories are never empty. Sandbox sb(&view, tapNONE); BookTip bt(sb, book_); - auto const clobQuality = + auto const lobQuality = bt.step(j_) ? std::optional(bt.quality()) : std::nullopt; - // Don't pass in clobQuality. For one-path it returns the offer as - // the pool balances and the resulting quality is Spot Price Quality. - // For multi-path it returns the actual offer. - // AMM quality is better or no CLOB offer - if (auto const ammOffer = getAMMOffer(view, std::nullopt); ammOffer && - ((clobQuality && ammOffer->quality() > clobQuality) || !clobQuality)) + // Multi-path offer generates an offer with the quality + // calculated from the offer size and the quality is constant in this case. + // Single path offer quality changes with the offer size. Spot price quality + // (SPQ) can't be used in this case as the upper bound quality because + // even if SPQ quality is better than LOB quality, it might not be possible + // to generate AMM offer at or better quality than LOB quality. Another + // factor to consider is limit quality on offer crossing. If LOB quality + // is greater than limit quality then use LOB quality when generating AMM + // offer, otherwise don't use quality threshold when generating AMM offer. + // AMM or LOB offer, whether multi-path or single path then can be selected + // based on the best offer quality. Using the quality to generate AMM offer + // in this case also prevents the payment engine from going into multiple + // iterations to cross a LOB offer. This happens when AMM changes + // the out amount at the start of iteration to match the limitQuality + // on offer crossing but AMM can't generate the offer at this quality, + // as the result a LOB offer is partially crossed, and it might take a few + // iterations to fully cross the offer. + auto const qualityThreshold = [&]() -> std::optional { + if (view.rules().enabled(fixAMMv1_1) && lobQuality) + return static_cast(this)->qualityThreshold( + *lobQuality); + return std::nullopt; + }(); + // AMM quality is better or no LOB offer + if (auto const ammOffer = getAMMOffer(view, qualityThreshold); ammOffer && + ((lobQuality && ammOffer->quality() > lobQuality) || !lobQuality)) return ammOffer; - // CLOB quality is better or nullopt - return clobQuality; + // LOB quality is better or nullopt + return lobQuality; } template diff --git a/src/ripple/app/tx/impl/AMMCreate.cpp b/src/ripple/app/tx/impl/AMMCreate.cpp index 55b1126fcd0..cab99f1669e 100644 --- a/src/ripple/app/tx/impl/AMMCreate.cpp +++ b/src/ripple/app/tx/impl/AMMCreate.cpp @@ -252,8 +252,8 @@ applyCreate( : 1}; sleAMMRoot->setFieldU32(sfSequence, seqno); // Ignore reserves requirement, disable the master key, allow default - // rippling (AMM LPToken can be used as a token in another AMM, which must - // support payments and offer crossing), and enable deposit authorization to + // rippling (AMM LPToken can be used in payments and offer crossing but + // not as a token in another AMM), and enable deposit authorization to // prevent payments into AMM. // Note, that the trustlines created by AMM have 0 credit limit. // This prevents shifting the balance between accounts via AMM, diff --git a/src/ripple/basics/Number.h b/src/ripple/basics/Number.h index 48cea443ee3..cdc25b3b27d 100644 --- a/src/ripple/basics/Number.h +++ b/src/ripple/basics/Number.h @@ -387,6 +387,26 @@ class saveNumberRoundMode operator=(saveNumberRoundMode const&) = delete; }; +// saveNumberRoundMode doesn't do quite enough for us. What we want is a +// Number::RoundModeGuard that sets the new mode and restores the old mode +// when it leaves scope. Since Number doesn't have that facility, we'll +// build it here. +class NumberRoundModeGuard +{ + saveNumberRoundMode saved_; + +public: + explicit NumberRoundModeGuard(Number::rounding_mode mode) noexcept + : saved_{Number::setround(mode)} + { + } + + NumberRoundModeGuard(NumberRoundModeGuard const&) = delete; + + NumberRoundModeGuard& + operator=(NumberRoundModeGuard const&) = delete; +}; + } // namespace ripple #endif // RIPPLE_BASICS_NUMBER_H_INCLUDED diff --git a/src/ripple/ledger/impl/View.cpp b/src/ripple/ledger/impl/View.cpp index a68f3a51387..b71b4f39e5e 100644 --- a/src/ripple/ledger/impl/View.cpp +++ b/src/ripple/ledger/impl/View.cpp @@ -1147,7 +1147,7 @@ accountSend( beast::Journal j, WaiveTransferFee waiveFee) { - if (view.rules().enabled(fixAMMRounding)) + if (view.rules().enabled(fixAMMv1_1)) { if (saAmount < beast::zero) { diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index b8788a74de7..57cd9513eea 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -359,7 +359,7 @@ extern uint256 const featurePriceOracle; extern uint256 const fixEmptyDID; extern uint256 const fixXChainRewardRounding; extern uint256 const fixPreviousTxnID; -extern uint256 const fixAMMRounding; +extern uint256 const fixAMMv1_1; } // namespace ripple diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index 8c8ff403e3f..2dd6e361408 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -466,7 +466,7 @@ REGISTER_FEATURE(PriceOracle, Supported::yes, VoteBehavior::De REGISTER_FIX (fixEmptyDID, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixXChainRewardRounding, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixPreviousTxnID, Supported::yes, VoteBehavior::DefaultNo); -REGISTER_FIX (fixAMMRounding, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FIX (fixAMMv1_1, 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/STAmount.cpp b/src/ripple/protocol/impl/STAmount.cpp index 201bcb6b681..a02dc9e89e1 100644 --- a/src/ripple/protocol/impl/STAmount.cpp +++ b/src/ripple/protocol/impl/STAmount.cpp @@ -1367,26 +1367,6 @@ canonicalizeRoundStrict( namespace { -// saveNumberRoundMode doesn't do quite enough for us. What we want is a -// Number::RoundModeGuard that sets the new mode and restores the old mode -// when it leaves scope. Since Number doesn't have that facility, we'll -// build it here. -class NumberRoundModeGuard -{ - saveNumberRoundMode saved_; - -public: - explicit NumberRoundModeGuard(Number::rounding_mode mode) noexcept - : saved_{Number::setround(mode)} - { - } - - NumberRoundModeGuard(NumberRoundModeGuard const&) = delete; - - NumberRoundModeGuard& - operator=(NumberRoundModeGuard const&) = delete; -}; - // We need a class that has an interface similar to NumberRoundModeGuard // but does nothing. class DontAffectNumberRoundMode diff --git a/src/test/app/AMMCalc_test.cpp b/src/test/app/AMMCalc_test.cpp index cdb57c3b4c0..e230ed4d3c5 100644 --- a/src/test/app/AMMCalc_test.cpp +++ b/src/test/app/AMMCalc_test.cpp @@ -413,13 +413,18 @@ class AMMCalc_test : public beast::unit_test::suite // 10 is AMM trading fee else if (*p == "changespq") { + Env env(*this); if (auto const pool = getAmounts(++p)) { if (auto const offer = getAmounts(p)) { auto const fee = getFee(p); if (auto const ammOffer = changeSpotPriceQuality( - pool->first, Quality{offer->first}, fee); + pool->first, + Quality{offer->first}, + fee, + env.current()->rules(), + beast::Journal(beast::Journal::getNullSink())); ammOffer) std::cout << "amm offer: " << toString(ammOffer->in) diff --git a/src/test/app/AMMExtended_test.cpp b/src/test/app/AMMExtended_test.cpp index 8dd915c8d91..39abf150fa7 100644 --- a/src/test/app/AMMExtended_test.cpp +++ b/src/test/app/AMMExtended_test.cpp @@ -94,7 +94,7 @@ struct AMMExtended_test : public jtx::AMMTest sendmax(BTC(1'000)), txflags(tfPartialPayment)); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { BEAST_EXPECT(ammCarol.expectBalances( STAmount{BTC, UINT64_C(1'001'000000374812), -12}, @@ -720,7 +720,7 @@ struct AMMExtended_test : public jtx::AMMTest auto const jrr = env.rpc("json", "submit", to_string(payment)); BEAST_EXPECT(jrr[jss::result][jss::status] == "success"); BEAST_EXPECT(jrr[jss::result][jss::engine_result] == "tesSUCCESS"); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { BEAST_EXPECT(ammAlice.expectBalances( STAmount(XTS, UINT64_C(101'010101010101), -12), @@ -1291,17 +1291,34 @@ struct AMMExtended_test : public jtx::AMMTest env(offer(cam, B_BUX(30), A_BUX(30))); // AMM is consumed up to the first cam Offer quality - BEAST_EXPECT(ammCarol.expectBalances( - STAmount{A_BUX, UINT64_C(309'3541659651605), -13}, - STAmount{B_BUX, UINT64_C(320'0215509984417), -13}, - ammCarol.tokens())); - BEAST_EXPECT(expectOffers( - env, - cam, - 1, - {{Amounts{ - STAmount{B_BUX, UINT64_C(20'0215509984417), -13}, - STAmount{A_BUX, UINT64_C(20'0215509984417), -13}}}})); + if (!features[fixAMMv1_1]) + { + BEAST_EXPECT(ammCarol.expectBalances( + STAmount{A_BUX, UINT64_C(309'3541659651605), -13}, + STAmount{B_BUX, UINT64_C(320'0215509984417), -13}, + ammCarol.tokens())); + BEAST_EXPECT(expectOffers( + env, + cam, + 1, + {{Amounts{ + STAmount{B_BUX, UINT64_C(20'0215509984417), -13}, + STAmount{A_BUX, UINT64_C(20'0215509984417), -13}}}})); + } + else + { + BEAST_EXPECT(ammCarol.expectBalances( + STAmount{A_BUX, UINT64_C(309'3541659651604), -13}, + STAmount{B_BUX, UINT64_C(320'0215509984419), -13}, + ammCarol.tokens())); + BEAST_EXPECT(expectOffers( + env, + cam, + 1, + {{Amounts{ + STAmount{B_BUX, UINT64_C(20'0215509984419), -13}, + STAmount{A_BUX, UINT64_C(20'0215509984419), -13}}}})); + } } void @@ -1427,7 +1444,7 @@ struct AMMExtended_test : public jtx::AMMTest using namespace jtx; FeatureBitset const all{supported_amendments()}; testRmFundedOffer(all); - testRmFundedOffer(all - fixAMMRounding); + testRmFundedOffer(all - fixAMMv1_1); testEnforceNoRipple(all); testFillModes(all); testOfferCrossWithXRP(all); @@ -1441,28 +1458,17 @@ struct AMMExtended_test : public jtx::AMMTest testOfferCreateThenCross(all); testSellFlagExceedLimit(all); testGatewayCrossCurrency(all); - testGatewayCrossCurrency(all - fixAMMRounding); - // testPartialCross - // testXRPDirectCross - // testDirectCross + testGatewayCrossCurrency(all - fixAMMv1_1); testBridgedCross(all); - // testSellOffer testSellWithFillOrKill(all); testTransferRateOffer(all); testSelfIssueOffer(all); testBadPathAssert(all); testSellFlagBasic(all); testDirectToDirectPath(all); - // testSelfCrossLowQualityOffer - // testOfferInScaling - // testOfferInScalingWithXferRate - // testOfferThresholdWithReducedFunds - // testTinyOffer - // testSelfPayXferFeeOffer - // testSelfPayXferFeeOffer + testDirectToDirectPath(all - fixAMMv1_1); testRequireAuth(all); testMissingAuth(all); - // testRCSmoketest } void @@ -2317,7 +2323,7 @@ struct AMMExtended_test : public jtx::AMMTest txflags(tfNoRippleDirect | tfPartialPayment | tfLimitQuality)); env.close(); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { // alice buys 77.2727USD with 75.5555GBP and pays 25% tr fee // on 75.5555GBP @@ -2364,7 +2370,7 @@ struct AMMExtended_test : public jtx::AMMTest env(offer(alice, EUR(100), USD(100))); env.close(); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { // 95.2380USD is swapped in for 100EUR BEAST_EXPECT(amm.expectBalances( @@ -2417,7 +2423,7 @@ struct AMMExtended_test : public jtx::AMMTest env(pay(gw, dan, USD(1'000))); AMM ammDan(env, dan, USD(1'000), EUR(1'050)); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { // alice -> bob -> gw -> carol. $50 should have transfer fee; // $10, no fee @@ -2486,7 +2492,7 @@ struct AMMExtended_test : public jtx::AMMTest // alice buys 107.1428USD with 120GBP and pays 25% tr fee on 120GBP // 1,000 - 120*1.25 = 850GBP BEAST_EXPECT(expectLine(env, alice, GBP(850))); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { // 120GBP is swapped in for 107.1428USD BEAST_EXPECT(amm.expectBalances( @@ -2575,7 +2581,7 @@ struct AMMExtended_test : public jtx::AMMTest env.close(); BEAST_EXPECT(expectLine(env, alice, GBP(850))); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { // alice buys 107.1428EUR with 120GBP and pays 25% tr fee on // 120GBP 1,000 - 120*1.25 = 850GBP 120GBP is swapped in for @@ -2692,7 +2698,7 @@ struct AMMExtended_test : public jtx::AMMTest txflags(tfNoRippleDirect | tfPartialPayment | tfLimitQuality)); env.close(); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { // alice buys 28.125USD with 24GBP and pays 25% tr fee // on 24GBP @@ -2749,7 +2755,7 @@ struct AMMExtended_test : public jtx::AMMTest txflags(tfNoRippleDirect | tfPartialPayment | tfLimitQuality)); env.close(); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { // alice buys 70.4210EUR with 70.4210GBP via the offer // and pays 25% tr fee on 70.4210GBP @@ -2841,7 +2847,7 @@ struct AMMExtended_test : public jtx::AMMTest txflags(tfNoRippleDirect | tfPartialPayment | tfLimitQuality)); env.close(); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { // alice buys 53.3322EUR with 56.3368GBP via the amm // and pays 25% tr fee on 56.3368GBP @@ -2919,7 +2925,7 @@ struct AMMExtended_test : public jtx::AMMTest txflags(tfNoRippleDirect | tfPartialPayment | tfLimitQuality)); env.close(); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { // alice buys 53.3322EUR with 107.5308GBP // 25% on 86.0246GBP is paid in tr fee @@ -2990,7 +2996,7 @@ struct AMMExtended_test : public jtx::AMMTest txflags(tfNoRippleDirect | tfPartialPayment | tfLimitQuality)); env.close(); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { // 108.1481GBP is swapped in for 97.5935EUR BEAST_EXPECT(amm1.expectBalances( @@ -3163,8 +3169,12 @@ struct AMMExtended_test : public jtx::AMMTest // Alice offers to buy 1000 XRP for 1000 USD. She takes Bob's first // offer, removes 999 more as unfunded, then hits the step limit. env(offer(alice, USD(1'000), XRP(1'000))); - env.require( - balance(alice, STAmount{USD, UINT64_C(2'050126257867561), -15})); + if (!features[fixAMMv1_1]) + env.require(balance( + alice, STAmount{USD, UINT64_C(2'050126257867561), -15})); + else + env.require(balance( + alice, STAmount{USD, UINT64_C(2'050125257867587), -15})); env.require(owners(alice, 2)); env.require(balance(bob, USD(0))); env.require(owners(bob, 1'001)); @@ -3270,7 +3280,7 @@ struct AMMExtended_test : public jtx::AMMTest env(offer(bob, XRP(100), USD(100))); env(offer(bob, XRP(1'000), USD(100))); AMM ammDan(env, dan, XRP(1'000), USD(1'100)); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { env(pay(alice, carol, USD(10'000)), paths(XRP), @@ -4080,9 +4090,9 @@ struct AMMExtended_test : public jtx::AMMTest testBookStep(all); testBookStep(all | ownerPaysFee); testTransferRate(all | ownerPaysFee); - testTransferRate((all - fixAMMRounding) | ownerPaysFee); + testTransferRate((all - fixAMMv1_1) | ownerPaysFee); testTransferRateNoOwnerFee(all); - testTransferRateNoOwnerFee(all - fixAMMRounding); + testTransferRateNoOwnerFee(all - fixAMMv1_1); testLimitQuality(); testXRPPathLoop(); } @@ -4093,6 +4103,7 @@ struct AMMExtended_test : public jtx::AMMTest using namespace jtx; FeatureBitset const all{supported_amendments()}; testStepLimit(all); + testStepLimit(all - fixAMMv1_1); } void @@ -4101,7 +4112,7 @@ struct AMMExtended_test : public jtx::AMMTest using namespace jtx; FeatureBitset const all{supported_amendments()}; test_convert_all_of_an_asset(all); - test_convert_all_of_an_asset(all - fixAMMRounding); + test_convert_all_of_an_asset(all - fixAMMv1_1); } void diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index ecf68c9ae62..3beee97cce8 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -37,6 +37,8 @@ #include #include +#include + namespace ripple { namespace test { @@ -2955,7 +2957,7 @@ struct AMM_test : public jtx::AMMTest // alice pays ~1.011USD in fees, which is ~10 times more // than carol's fee // 100.099431529USD swapped in for 100XRP - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { BEAST_EXPECT(ammAlice.expectBalances( XRPAmount{13'000'000'668}, @@ -2984,7 +2986,7 @@ struct AMM_test : public jtx::AMMTest } // carol pays ~9.94USD in fees, which is ~10 times more in // trading fees vs discounted fee. - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { BEAST_EXPECT( env.balance(carol, USD) == @@ -3009,7 +3011,7 @@ struct AMM_test : public jtx::AMMTest // carol pays ~1.008XRP in trading fee, which is // ~10 times more than the discounted fee. // 99.815876XRP is swapped in for 100USD - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { BEAST_EXPECT(ammAlice.expectBalances( XRPAmount(13'100'824'790), @@ -3111,7 +3113,7 @@ struct AMM_test : public jtx::AMMTest IOUAmount{1'004'487'562112089, -9})); // Bob pays the full fee ~0.1USD env(pay(bob, alice, XRP(10)), path(~XRP), sendmax(USD(11))); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { BEAST_EXPECT(amm.expectBalances( XRPAmount{1'000'010'011}, @@ -3520,7 +3522,7 @@ struct AMM_test : public jtx::AMMTest XRPAmount(10'030'082'730), STAmount(EUR, UINT64_C(9'970'007498125468), -12), ammEUR_XRP.tokens())); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { BEAST_EXPECT(ammUSD_EUR.expectBalances( STAmount(USD, UINT64_C(9'970'097277662122), -12), @@ -3621,7 +3623,7 @@ struct AMM_test : public jtx::AMMTest sendmax(XRP(200)), txflags(tfPartialPayment)); env.close(); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { BEAST_EXPECT(ammAlice.expectBalances( XRP(10'100), USD(10'000), ammAlice.tokens())); @@ -3833,7 +3835,7 @@ struct AMM_test : public jtx::AMMTest path(~USD), path(~ETH, ~EUR, ~USD), sendmax(XRP(200))); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { // XRP-ETH-EUR-USD // This path provides ~26.06USD/26.2XRP @@ -3914,7 +3916,7 @@ struct AMM_test : public jtx::AMMTest path(~EUR, ~BTC, ~USD), path(~ETH, ~EUR, ~BTC, ~USD), sendmax(XRP(200))); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { // XRP-EUR-BTC-USD path provides ~17.8USD/~18.7XRP // XRP-ETH-EUR-BTC-USD path provides ~82.2USD/82.4XRP @@ -3981,7 +3983,7 @@ struct AMM_test : public jtx::AMMTest path(~XRP, ~USD), sendmax(EUR(400)), txflags(tfPartialPayment | tfNoRippleDirect)); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { // Carol gets ~29.91USD because of the AMM offers limit BEAST_EXPECT(ammAlice.expectBalances( @@ -4028,7 +4030,7 @@ struct AMM_test : public jtx::AMMTest txflags(tfPartialPayment | tfNoRippleDirect)); BEAST_EXPECT(ammAlice.expectBalances( XRPAmount{10'101'010'102}, USD(9'900), ammAlice.tokens())); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { // Carol gets ~100USD BEAST_EXPECT(expectLine( @@ -4060,17 +4062,33 @@ struct AMM_test : public jtx::AMMTest env(offer(bob, XRP(100), USD(100.001))); AMM ammAlice(env, alice, XRP(10'000), USD(10'100)); env(offer(carol, USD(100), XRP(100))); - BEAST_EXPECT(ammAlice.expectBalances( - XRPAmount{10'049'825'373}, - STAmount{USD, UINT64_C(10'049'92586949302), -11}, - ammAlice.tokens())); - BEAST_EXPECT(expectOffers( - env, - bob, - 1, - {{{XRPAmount{50'074'629}, - STAmount{USD, UINT64_C(50'07513050698), -11}}}})); - BEAST_EXPECT(expectLine(env, carol, USD(30'100))); + if (!features[fixAMMv1_1]) + { + BEAST_EXPECT(ammAlice.expectBalances( + XRPAmount{10'049'825'373}, + STAmount{USD, UINT64_C(10'049'92586949302), -11}, + ammAlice.tokens())); + BEAST_EXPECT(expectOffers( + env, + bob, + 1, + {{{XRPAmount{50'074'629}, + STAmount{USD, UINT64_C(50'07513050698), -11}}}})); + } + else + { + BEAST_EXPECT(ammAlice.expectBalances( + XRPAmount{10'049'825'372}, + STAmount{USD, UINT64_C(10'049'92587049303), -11}, + ammAlice.tokens())); + BEAST_EXPECT(expectOffers( + env, + bob, + 1, + {{{XRPAmount{50'074'628}, + STAmount{USD, UINT64_C(50'07512950697), -11}}}})); + BEAST_EXPECT(expectLine(env, carol, USD(30'100))); + } } // Individually frozen account @@ -4342,7 +4360,7 @@ struct AMM_test : public jtx::AMMTest // Execute with CLOB offer prep( [&](Env& env) { - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) env(offer( LP1, XRPAmount{18'095'133}, @@ -4352,7 +4370,7 @@ struct AMM_test : public jtx::AMMTest env(offer( LP1, XRPAmount{18'095'132}, - STAmount{TST, UINT64_C(1'68737984885387), -14}), + STAmount{TST, UINT64_C(1'68737976189735), -14}), txflags(tfPassive)); }, [&](Env& env) { @@ -4584,7 +4602,7 @@ struct AMM_test : public jtx::AMMTest {{Amounts{ STAmount{EUR, UINT64_C(5'025125628140703), -15}, STAmount{USD, UINT64_C(5'025125628140703), -15}}}})); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { BEAST_EXPECT(ammAlice.expectBalances( STAmount{USD, UINT64_C(1'004'974874371859), -12}, @@ -4624,7 +4642,7 @@ struct AMM_test : public jtx::AMMTest sendmax(EUR(15)), txflags(tfNoRippleDirect)); BEAST_EXPECT(expectLine(env, ed, USD(2'010))); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { BEAST_EXPECT(expectLine(env, bob, EUR(1'990))); BEAST_EXPECT(ammAlice.expectBalances( @@ -4661,7 +4679,7 @@ struct AMM_test : public jtx::AMMTest sendmax(EUR(15)), txflags(tfNoRippleDirect)); BEAST_EXPECT(expectLine(env, ed, USD(2'010))); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { BEAST_EXPECT(expectLine( env, @@ -4677,10 +4695,10 @@ struct AMM_test : public jtx::AMMTest BEAST_EXPECT(expectLine( env, bob, - STAmount{EUR, UINT64_C(1'989'987453007616), -12})); + STAmount{EUR, UINT64_C(1'989'987453007628), -12})); BEAST_EXPECT(ammAlice.expectBalances( USD(1'000), - STAmount{EUR, UINT64_C(1'005'012546992384), -12}, + STAmount{EUR, UINT64_C(1'005'012546992372), -12}, ammAlice.tokens())); } BEAST_EXPECT(expectOffers(env, carol, 0)); @@ -5192,35 +5210,69 @@ struct AMM_test : public jtx::AMMTest BEAST_EXPECT(!amm->expectBalances( USD(1'000), ETH(1'000), amm->tokens())); } - if (i == 2 && !features[fixAMMRounding]) + if (i == 2 && !features[fixAMMv1_1]) { if (rates.first == 1.5) { - BEAST_EXPECT(expectOffers( - env, - ed, - 1, - {{Amounts{ - STAmount{ - ETH, UINT64_C(378'6327949540823), -13}, - STAmount{ - USD, - UINT64_C(283'9745962155617), - -13}}}})); + if (!features[fixAMMv1_1]) + BEAST_EXPECT(expectOffers( + env, + ed, + 1, + {{Amounts{ + STAmount{ + ETH, + UINT64_C(378'6327949540823), + -13}, + STAmount{ + USD, + UINT64_C(283'9745962155617), + -13}}}})); + else + BEAST_EXPECT(expectOffers( + env, + ed, + 1, + {{Amounts{ + STAmount{ + ETH, + UINT64_C(378'6327949540813), + -13}, + STAmount{ + USD, + UINT64_C(283'974596215561), + -12}}}})); } else { - BEAST_EXPECT(expectOffers( - env, - ed, - 1, - {{Amounts{ - STAmount{ - ETH, UINT64_C(325'299461620749), -12}, - STAmount{ - USD, - UINT64_C(243'9745962155617), - -13}}}})); + if (!features[fixAMMv1_1]) + BEAST_EXPECT(expectOffers( + env, + ed, + 1, + {{Amounts{ + STAmount{ + ETH, + UINT64_C(325'299461620749), + -12}, + STAmount{ + USD, + UINT64_C(243'9745962155617), + -13}}}})); + else + BEAST_EXPECT(expectOffers( + env, + ed, + 1, + {{Amounts{ + STAmount{ + ETH, + UINT64_C(325'299461620748), + -12}, + STAmount{ + USD, + UINT64_C(243'974596215561), + -12}}}})); } } else if (i == 2) @@ -5292,29 +5344,71 @@ struct AMM_test : public jtx::AMMTest { if (rates.first == 1.5) { - BEAST_EXPECT(expectOffers( - env, ed, 1, {{Amounts{ETH(400), USD(250)}}})); - BEAST_EXPECT(expectOffers( - env, - alice, - 1, - {{Amounts{ - STAmount{USD, UINT64_C(40'5694150420947), -13}, - STAmount{ETH, UINT64_C(64'91106406735152), -14}, - }}})); + if (!features[fixAMMv1_1]) + { + BEAST_EXPECT(expectOffers( + env, ed, 1, {{Amounts{ETH(400), USD(250)}}})); + BEAST_EXPECT(expectOffers( + env, + alice, + 1, + {{Amounts{ + STAmount{ + USD, UINT64_C(40'5694150420947), -13}, + STAmount{ + ETH, UINT64_C(64'91106406735152), -14}, + }}})); + } + else + { + // Ed offer is partially crossed. + // The updated rounding makes limitQuality + // work if both amendments are enabled + BEAST_EXPECT(expectOffers( + env, + ed, + 1, + {{Amounts{ + STAmount{ + ETH, UINT64_C(335'0889359326475), -13}, + STAmount{ + USD, UINT64_C(209'4305849579047), -13}, + }}})); + BEAST_EXPECT(expectOffers(env, alice, 0)); + } } else { - // Ed offer is partially crossed. - BEAST_EXPECT(expectOffers( - env, - ed, - 1, - {{Amounts{ - STAmount{ETH, UINT64_C(335'0889359326485), -13}, - STAmount{USD, UINT64_C(209'4305849579053), -13}, - }}})); - BEAST_EXPECT(expectOffers(env, alice, 0)); + if (!features[fixAMMv1_1]) + { + // Ed offer is partially crossed. + BEAST_EXPECT(expectOffers( + env, + ed, + 1, + {{Amounts{ + STAmount{ + ETH, UINT64_C(335'0889359326485), -13}, + STAmount{ + USD, UINT64_C(209'4305849579053), -13}, + }}})); + BEAST_EXPECT(expectOffers(env, alice, 0)); + } + else + { + // Ed offer is partially crossed. + BEAST_EXPECT(expectOffers( + env, + ed, + 1, + {{Amounts{ + STAmount{ + ETH, UINT64_C(335'0889359326475), -13}, + STAmount{ + USD, UINT64_C(209'4305849579047), -13}, + }}})); + BEAST_EXPECT(expectOffers(env, alice, 0)); + } } } } @@ -5361,7 +5455,7 @@ struct AMM_test : public jtx::AMMTest BEAST_EXPECT(expectLine(env, bob, USD(2'100))); - if (i == 2 && !features[fixAMMRounding]) + if (i == 2 && !features[fixAMMv1_1]) { if (rates.first == 1.5) { @@ -5557,6 +5651,222 @@ struct AMM_test : public jtx::AMMTest false); } + void + testFixChangeSpotPriceQuality(FeatureBitset features) + { + testcase("Fix changeSpotPriceQuality"); + using namespace jtx; + + enum class Status { + SucceedShouldSucceedResize, // Succeed in pre-fix because + // error allowance, succeed post-fix + // because of offer resizing + FailShouldSucceed, // Fail in pre-fix due to rounding, + // succeed after fix because of XRP + // side is generated first + SucceedShouldFail, // Succeed in pre-fix, fail after fix + // due to small quality difference + Fail, // Both fail because the quality can't be matched + Succeed // Both succeed + }; + using enum Status; + auto const xrpIouAmounts10_100 = + TAmounts{XRPAmount{10}, IOUAmount{100}}; + auto const iouXrpAmounts10_100 = + TAmounts{IOUAmount{10}, XRPAmount{100}}; + // clang-format off + std::vector> tests = { + //Pool In , Pool Out, Quality , Fee, Status + {"0.001519763260828713", "1558701", Quality{5414253689393440221}, 1000, FailShouldSucceed}, + {"0.01099814367603737", "1892611", Quality{5482264816516900274}, 1000, FailShouldSucceed}, + {"0.78", "796599", Quality{5630392334958379008}, 1000, FailShouldSucceed}, + {"105439.2955578965", "49398693", Quality{5910869983721805038}, 400, FailShouldSucceed}, + {"12408293.23445213", "4340810521", Quality{5911611095910090752}, 997, FailShouldSucceed}, + {"1892611", "0.01099814367603737", Quality{6703103457950430139}, 1000, FailShouldSucceed}, + {"423028.8508101858", "3392804520", Quality{5837920340654162816}, 600, FailShouldSucceed}, + {"44565388.41001027", "73890647", Quality{6058976634606450001}, 1000, FailShouldSucceed}, + {"66831.68494832662", "16", Quality{6346111134641742975}, 0, FailShouldSucceed}, + {"675.9287302203422", "1242632304", Quality{5625960929244093294}, 300, FailShouldSucceed}, + {"7047.112186735699", "1649845866", Quality{5696855348026306945}, 504, FailShouldSucceed}, + {"840236.4402981238", "47419053", Quality{5982561601648018688}, 499, FailShouldSucceed}, + {"992715.618909774", "189445631733", Quality{5697835648288106944}, 815, SucceedShouldSucceedResize}, + {"504636667521", "185545883.9506651", Quality{6343802275337659280}, 503, SucceedShouldSucceedResize}, + {"992706.7218636649", "189447316000", Quality{5697835648288106944}, 797, SucceedShouldSucceedResize}, + {"1.068737911388205", "127860278877", Quality{5268604356368739396}, 293, SucceedShouldSucceedResize}, + {"17932506.56880419", "189308.6043676173", Quality{6206460598195440068}, 311, SucceedShouldSucceedResize}, + {"1.066379294658174", "128042251493", Quality{5268559341368739328}, 270, SucceedShouldSucceedResize}, + {"350131413924", "1576879.110907892", Quality{6487411636539049449}, 650, Fail}, + {"422093460", "2.731797662057464", Quality{6702911108534394924}, 1000, Fail}, + {"76128132223", "367172.7148422662", Quality{6487263463413514240}, 548, Fail}, + {"132701839250", "280703770.7695443", Quality{6273750681188885075}, 562, Fail}, + {"994165.7604612011", "189551302411", Quality{5697835592690668727}, 815, Fail}, + {"45053.33303227917", "86612695359", Quality{5625695218943638190}, 500, Fail}, + {"199649.077043865", "14017933007", Quality{5766034667318524880}, 324, Fail}, + {"27751824831.70903", "78896950", Quality{6272538159621630432}, 500, Fail}, + {"225.3731275781907", "156431793648", Quality{5477818047604078924}, 989, Fail}, + {"199649.077043865", "14017933007", Quality{5766036094462806309}, 324, Fail}, + {"3.590272027140361", "20677643641", Quality{5406056147042156356}, 808, Fail}, + {"1.070884664490231", "127604712776", Quality{5268620608623825741}, 293, Fail}, + {"3272.448829820197", "6275124076", Quality{5625710328924117902}, 81, Fail}, + {"0.009059512633902926", "7994028", Quality{5477511954775533172}, 1000, Fail}, + {"1", "1.0", Quality{0}, 100, Fail}, + {"1.0", "1", Quality{0}, 100, Fail}, + {"10", "10.0", Quality{xrpIouAmounts10_100}, 100, Fail}, + {"10.0", "10", Quality{iouXrpAmounts10_100}, 100, Fail}, + {"69864389131", "287631.4543025075", Quality{6487623473313516078}, 451, Succeed}, + {"4328342973", "12453825.99247381", Quality{6272522264364865181}, 997, Succeed}, + {"32347017", "7003.93031579449", Quality{6347261126087916670}, 1000, Succeed}, + {"61697206161", "36631.4583206413", Quality{6558965195382476659}, 500, Succeed}, + {"1654524979", "7028.659825511603", Quality{6487551345110052981}, 504, Succeed}, + {"88621.22277293179", "5128418948", Quality{5766347291552869205}, 380, Succeed}, + {"1892611", "0.01099814367603737", Quality{6703102780512015436}, 1000, Succeed}, + {"4542.639373338766", "24554809", Quality{5838994982188783710}, 0, Succeed}, + {"5132932546", "88542.99750172683", Quality{6419203342950054537}, 380, Succeed}, + {"78929964.1549083", "1506494795", Quality{5986890029845558688}, 589, Succeed}, + {"10096561906", "44727.72453735605", Quality{6487455290284644551}, 250, Succeed}, + {"5092.219565514988", "8768257694", Quality{5626349534958379008}, 503, Succeed}, + {"1819778294", "8305.084302902864", Quality{6487429398998540860}, 415, Succeed}, + {"6970462.633911943", "57359281", Quality{6054087899185946624}, 850, Succeed}, + {"3983448845", "2347.543644281467", Quality{6558965195382476659}, 856, Succeed}, + // This is a tiny offer 12drops/19321952e-15 it succeeds pre-amendment because of the error allowance. + // Post amendment it is resized to 11drops/17711789e-15 but the quality is still less than + // the target quality and the offer fails. + {"771493171", "1.243473020567508", Quality{6707566798038544272}, 100, SucceedShouldFail}, + }; + // clang-format on + + boost::regex rx("^\\d+$"); + boost::smatch match; + // tests that succeed should have the same amounts pre-fix and post-fix + std::vector> successAmounts; + Env env(*this, features); + auto rules = env.current()->rules(); + CurrentTransactionRulesGuard rg(rules); + for (auto const& t : tests) + { + auto getPool = [&](std::string const& v, bool isXRP) { + if (isXRP) + return amountFromString(xrpIssue(), v); + return amountFromString(noIssue(), v); + }; + auto const& quality = std::get(t); + auto const tfee = std::get(t); + auto const status = std::get(t); + auto const poolInIsXRP = + boost::regex_search(std::get<0>(t), match, rx); + auto const poolOutIsXRP = + boost::regex_search(std::get<1>(t), match, rx); + assert(!(poolInIsXRP && poolOutIsXRP)); + auto const poolIn = getPool(std::get<0>(t), poolInIsXRP); + auto const poolOut = getPool(std::get<1>(t), poolOutIsXRP); + try + { + auto const amounts = changeSpotPriceQuality( + Amounts{poolIn, poolOut}, + quality, + tfee, + env.current()->rules(), + env.journal); + if (amounts) + { + if (status == SucceedShouldSucceedResize) + { + if (!features[fixAMMv1_1]) + BEAST_EXPECT(Quality{*amounts} < quality); + else + BEAST_EXPECT(Quality{*amounts} >= quality); + } + else if (status == Succeed) + { + if (!features[fixAMMv1_1]) + BEAST_EXPECT( + Quality{*amounts} >= quality || + withinRelativeDistance( + Quality{*amounts}, quality, Number{1, -7})); + else + BEAST_EXPECT(Quality{*amounts} >= quality); + } + else if (status == FailShouldSucceed) + { + BEAST_EXPECT( + features[fixAMMv1_1] && + Quality{*amounts} >= quality); + } + else if (status == SucceedShouldFail) + { + BEAST_EXPECT( + !features[fixAMMv1_1] && + Quality{*amounts} < quality && + withinRelativeDistance( + Quality{*amounts}, quality, Number{1, -7})); + } + } + else + { + // Fails pre- and post-amendment because the quality can't + // be matched. Verify by generating a tiny offer, which + // doesn't match the quality. Exclude zero quality since + // no offer is generated in this case. + if (status == Fail && quality != Quality{0}) + { + auto tinyOffer = [&]() { + if (isXRP(poolIn)) + { + auto const takerPays = STAmount{xrpIssue(), 1}; + return Amounts{ + takerPays, + swapAssetIn( + Amounts{poolIn, poolOut}, + takerPays, + tfee)}; + } + else if (isXRP(poolOut)) + { + auto const takerGets = STAmount{xrpIssue(), 1}; + return Amounts{ + swapAssetOut( + Amounts{poolIn, poolOut}, + takerGets, + tfee), + takerGets}; + } + auto const takerPays = toAmount( + getIssue(poolIn), Number{1, -10} * poolIn); + return Amounts{ + takerPays, + swapAssetIn( + Amounts{poolIn, poolOut}, takerPays, tfee)}; + }(); + BEAST_EXPECT(Quality(tinyOffer) < quality); + } + else if (status == FailShouldSucceed) + { + BEAST_EXPECT(!features[fixAMMv1_1]); + } + else if (status == SucceedShouldFail) + { + BEAST_EXPECT(features[fixAMMv1_1]); + } + } + } + catch (std::runtime_error const& e) + { + BEAST_EXPECT( + !strcmp(e.what(), "changeSpotPriceQuality failed")); + BEAST_EXPECT( + !features[fixAMMv1_1] && status == FailShouldSucceed); + } + } + + // Test negative discriminant + { + // b**2 - 4 * a * c -> 1 * 1 - 4 * 1 * 1 = -3 + auto const res = + solveQuadraticEqSmallest(Number{1}, Number{1}, Number{1}); + BEAST_EXPECT(!res.has_value()); + } + } + void testMalformed() { @@ -5895,18 +6205,14 @@ struct AMM_test : public jtx::AMMTest txflags(tfPartialPayment)); env.close(); - auto const failUsdGH = features[fixAMMRounding] - ? input.failUsdGHr - : input.failUsdGH; - auto const failUsdBIT = features[fixAMMRounding] - ? input.failUsdBITr - : input.failUsdBIT; - auto const goodUsdGH = features[fixAMMRounding] - ? input.goodUsdGHr - : input.goodUsdGH; - auto const goodUsdBIT = features[fixAMMRounding] - ? input.goodUsdBITr - : input.goodUsdBIT; + auto const failUsdGH = + features[fixAMMv1_1] ? input.failUsdGHr : input.failUsdGH; + auto const failUsdBIT = + features[fixAMMv1_1] ? input.failUsdBITr : input.failUsdBIT; + auto const goodUsdGH = + features[fixAMMv1_1] ? input.goodUsdGHr : input.goodUsdGH; + auto const goodUsdBIT = + features[fixAMMv1_1] ? input.goodUsdBITr : input.goodUsdBIT; if (!features[fixAMMOverflowOffer]) { BEAST_EXPECT(amm.expectBalances( @@ -5977,7 +6283,135 @@ struct AMM_test : public jtx::AMMTest {{xrpPool, iouPool}}, 889, std::nullopt, - {jtx::supported_amendments() | fixAMMRounding}); + {jtx::supported_amendments() | fixAMMv1_1}); + } + + void + testFixAMMOfferBlockedByLOB(FeatureBitset features) + { + testcase("AMM Offer Blocked By LOB"); + using namespace jtx; + + // Low quality LOB offer blocks AMM liquidity + + // USD/XRP crosses AMM + { + Env env(*this, features); + + fund(env, gw, {alice, carol}, XRP(1'000'000), {USD(1'000'000)}); + // This offer blocks AMM offer in pre-amendment + env(offer(alice, XRP(1), USD(0.01))); + env.close(); + + AMM amm(env, gw, XRP(200'000), USD(100'000)); + + // The offer doesn't cross AMM in pre-amendment code + // It crosses AMM in post-amendment code + env(offer(carol, USD(0.49), XRP(1))); + env.close(); + + if (!features[fixAMMv1_1]) + { + BEAST_EXPECT(amm.expectBalances( + XRP(200'000), USD(100'000), amm.tokens())); + BEAST_EXPECT(expectOffers( + env, alice, 1, {{Amounts{XRP(1), USD(0.01)}}})); + // Carol's offer is blocked by alice's offer + BEAST_EXPECT(expectOffers( + env, carol, 1, {{Amounts{USD(0.49), XRP(1)}}})); + } + else + { + BEAST_EXPECT(amm.expectBalances( + XRPAmount(200'000'980'005), USD(99'999.51), amm.tokens())); + BEAST_EXPECT(expectOffers( + env, alice, 1, {{Amounts{XRP(1), USD(0.01)}}})); + // Carol's offer crosses AMM + BEAST_EXPECT(expectOffers(env, carol, 0)); + } + } + + // There is no blocking offer, the same AMM liquidity is consumed + // pre- and post-amendment. + { + Env env(*this, features); + + fund(env, gw, {alice, carol}, XRP(1'000'000), {USD(1'000'000)}); + // There is no blocking offer + // env(offer(alice, XRP(1), USD(0.01))); + + AMM amm(env, gw, XRP(200'000), USD(100'000)); + + // The offer crosses AMM + env(offer(carol, USD(0.49), XRP(1))); + env.close(); + + // The same result as with the blocking offer + BEAST_EXPECT(amm.expectBalances( + XRPAmount(200'000'980'005), USD(99'999.51), amm.tokens())); + // Carol's offer crosses AMM + BEAST_EXPECT(expectOffers(env, carol, 0)); + } + + // XRP/USD crosses AMM + { + Env env(*this, features); + fund(env, gw, {alice, carol, bob}, XRP(10'000), {USD(1'000)}); + + // This offer blocks AMM offer in pre-amendment + // It crosses AMM in post-amendment code + env(offer(bob, USD(1), XRPAmount(500))); + env.close(); + AMM amm(env, alice, XRP(1'000), USD(500)); + env(offer(carol, XRP(100), USD(55))); + env.close(); + if (!features[fixAMMv1_1]) + { + BEAST_EXPECT( + amm.expectBalances(XRP(1'000), USD(500), amm.tokens())); + BEAST_EXPECT(expectOffers( + env, bob, 1, {{Amounts{USD(1), XRPAmount(500)}}})); + BEAST_EXPECT(expectOffers( + env, carol, 1, {{Amounts{XRP(100), USD(55)}}})); + } + else + { + BEAST_EXPECT(amm.expectBalances( + XRPAmount(909'090'909), + STAmount{USD, UINT64_C(550'000000055), -9}, + amm.tokens())); + BEAST_EXPECT(expectOffers( + env, + carol, + 1, + {{Amounts{ + XRPAmount{9'090'909}, + STAmount{USD, 4'99999995, -8}}}})); + BEAST_EXPECT(expectOffers( + env, bob, 1, {{Amounts{USD(1), XRPAmount(500)}}})); + } + } + + // There is no blocking offer, the same AMM liquidity is consumed + // pre- and post-amendment. + { + Env env(*this, features); + fund(env, gw, {alice, carol, bob}, XRP(10'000), {USD(1'000)}); + + AMM amm(env, alice, XRP(1'000), USD(500)); + env(offer(carol, XRP(100), USD(55))); + env.close(); + BEAST_EXPECT(amm.expectBalances( + XRPAmount(909'090'909), + STAmount{USD, UINT64_C(550'000000055), -9}, + amm.tokens())); + BEAST_EXPECT(expectOffers( + env, + carol, + 1, + {{Amounts{ + XRPAmount{9'090'909}, STAmount{USD, 4'99999995, -8}}}})); + } } void @@ -5994,29 +6428,33 @@ struct AMM_test : public jtx::AMMTest testFeeVote(); testInvalidBid(); testBid(all); - testBid(all - fixAMMRounding); + testBid(all - fixAMMv1_1); testInvalidAMMPayment(); testBasicPaymentEngine(all); - testBasicPaymentEngine(all - fixAMMRounding); + testBasicPaymentEngine(all - fixAMMv1_1); testAMMTokens(); testAmendment(); testFlags(); testRippling(); testAMMAndCLOB(all); - testAMMAndCLOB(all - fixAMMRounding); + testAMMAndCLOB(all - fixAMMv1_1); testTradingFee(all); - testTradingFee(all - fixAMMRounding); + testTradingFee(all - fixAMMv1_1); testAdjustedTokens(); testAutoDelete(); testClawback(); testAMMID(); testSelection(all); - testSelection(all - fixAMMRounding); + testSelection(all - fixAMMv1_1); testFixDefaultInnerObj(); testMalformed(); testFixOverflowOffer(all); - testFixOverflowOffer(all - fixAMMRounding); + testFixOverflowOffer(all - fixAMMv1_1); testSwapRounding(); + testFixChangeSpotPriceQuality(all); + testFixChangeSpotPriceQuality(all - fixAMMv1_1); + testFixAMMOfferBlockedByLOB(all); + testFixAMMOfferBlockedByLOB(all - fixAMMv1_1); } }; From 2a25f58d40229175009cc156c25cd8c91756af0a Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Sat, 4 May 2024 18:50:59 -0400 Subject: [PATCH 08/11] Fix adjustAmountsByLPTokens(): The fix is to return the actual adjusted lp tokens and amounts by the function. --- src/ripple/app/misc/impl/AMMHelpers.cpp | 33 +- src/test/app/AMMExtended_test.cpp | 20 +- src/test/app/AMM_test.cpp | 406 ++++++++++++++++-------- src/test/jtx/AMMTest.h | 2 +- src/test/jtx/impl/AMMTest.cpp | 67 ++-- 5 files changed, 338 insertions(+), 190 deletions(-) diff --git a/src/ripple/app/misc/impl/AMMHelpers.cpp b/src/ripple/app/misc/impl/AMMHelpers.cpp index 57b3d3a07d3..33669dcdc1f 100644 --- a/src/ripple/app/misc/impl/AMMHelpers.cpp +++ b/src/ripple/app/misc/impl/AMMHelpers.cpp @@ -165,6 +165,13 @@ adjustAmountsByLPTokens( if (lpTokensActual < lpTokens) { + bool const ammRoundingEnabled = [&]() { + if (auto const& rules = getCurrentTransactionRules(); + rules && rules->enabled(fixAMMRounding)) + return true; + return false; + }(); + // Equal trade if (amount2) { @@ -172,10 +179,14 @@ adjustAmountsByLPTokens( auto const amountActual = toSTAmount(amount.issue(), fr * amount); auto const amount2Actual = toSTAmount(amount2->issue(), fr * *amount2); - return std::make_tuple( - amountActual < amount ? amountActual : amount, - amount2Actual < amount2 ? amount2Actual : amount2, - lpTokensActual); + if (!ammRoundingEnabled) + return std::make_tuple( + amountActual < amount ? amountActual : amount, + amount2Actual < amount2 ? amount2Actual : amount2, + lpTokensActual); + else + return std::make_tuple( + amountActual, amount2Actual, lpTokensActual); } // Single trade @@ -183,13 +194,19 @@ adjustAmountsByLPTokens( if (isDeposit) return ammAssetIn( amountBalance, lptAMMBalance, lpTokensActual, tfee); - else + else if (!ammRoundingEnabled) return withdrawByTokens( amountBalance, lptAMMBalance, lpTokens, tfee); + else + return withdrawByTokens( + amountBalance, lptAMMBalance, lpTokensActual, tfee); }(); - return amountActual < amount - ? std::make_tuple(amountActual, std::nullopt, lpTokensActual) - : std::make_tuple(amount, std::nullopt, lpTokensActual); + if (!ammRoundingEnabled) + return amountActual < amount + ? std::make_tuple(amountActual, std::nullopt, lpTokensActual) + : std::make_tuple(amount, std::nullopt, lpTokensActual); + else + return std::make_tuple(amountActual, std::nullopt, lpTokensActual); } assert(lpTokensActual == lpTokens); diff --git a/src/test/app/AMMExtended_test.cpp b/src/test/app/AMMExtended_test.cpp index 39abf150fa7..27fb2ce14f5 100644 --- a/src/test/app/AMMExtended_test.cpp +++ b/src/test/app/AMMExtended_test.cpp @@ -233,7 +233,7 @@ struct AMMExtended_test : public jtx::AMMTest {{XRP(10'100), USD(10'000)}}, 0, std::nullopt, - tweakedFeatures); + {tweakedFeatures}); // Immediate or Cancel - cross as much as possible // and add nothing on the books. @@ -257,7 +257,7 @@ struct AMMExtended_test : public jtx::AMMTest {{XRP(10'100), USD(10'000)}}, 0, std::nullopt, - tweakedFeatures); + {tweakedFeatures}); // tfPassive -- place the offer without crossing it. testAMM( @@ -274,7 +274,7 @@ struct AMMExtended_test : public jtx::AMMTest {{XRP(10'100), USD(10'000)}}, 0, std::nullopt, - tweakedFeatures); + {tweakedFeatures}); // tfPassive -- cross only offers of better quality. testAMM( @@ -296,7 +296,7 @@ struct AMMExtended_test : public jtx::AMMTest {{XRP(11'000), USD(9'000)}}, 0, std::nullopt, - tweakedFeatures); + {tweakedFeatures}); } } @@ -430,7 +430,7 @@ struct AMMExtended_test : public jtx::AMMTest {{XRP(10'000), USD(10'000)}}, 0, std::nullopt, - features); + {features}); } void @@ -453,7 +453,7 @@ struct AMMExtended_test : public jtx::AMMTest {{XRP(10'000), USD(10'100)}}, 0, std::nullopt, - features); + {features}); } void @@ -477,7 +477,7 @@ struct AMMExtended_test : public jtx::AMMTest {{XRP(10'100), USD(10'000)}}, 0, std::nullopt, - features); + {features}); } void @@ -643,7 +643,7 @@ struct AMMExtended_test : public jtx::AMMTest {{XRP(9'900), USD(10'100)}}, 0, std::nullopt, - features); + {features}); } void @@ -952,7 +952,7 @@ struct AMMExtended_test : public jtx::AMMTest {{XRP(10'000), USD(10'100)}}, 0, std::nullopt, - features); + {features}); // Reverse the order, so the offer in the books is to sell XRP // in return for USD. @@ -973,7 +973,7 @@ struct AMMExtended_test : public jtx::AMMTest {{XRP(10'100), USD(10'000)}}, 0, std::nullopt, - features); + {features}); { // Bridged crossing. diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 3beee97cce8..e2a50d95095 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -2215,33 +2215,64 @@ struct AMM_test : public jtx::AMMTest IOUAmount{10'000'000, 0})); }); + auto const all = supported_amendments(); // Withdraw with EPrice limit. - testAMM([&](AMM& ammAlice, Env&) { - ammAlice.deposit(carol, 1'000'000); - ammAlice.withdraw(carol, USD(100), std::nullopt, IOUAmount{520, 0}); - BEAST_EXPECT( - ammAlice.expectBalances( - XRPAmount(11'000'000'000), - STAmount{USD, UINT64_C(9'372'781065088757), -12}, - IOUAmount{10'153'846'15384616, -8}) && - ammAlice.expectLPTokens( - carol, IOUAmount{153'846'15384616, -8})); - ammAlice.withdrawAll(carol); - BEAST_EXPECT(ammAlice.expectLPTokens(carol, IOUAmount{0})); - }); + testAMM( + [&](AMM& ammAlice, Env& env) { + ammAlice.deposit(carol, 1'000'000); + ammAlice.withdraw( + carol, USD(100), std::nullopt, IOUAmount{520, 0}); + if (!env.current()->rules().enabled(fixAMMv1_1)) + BEAST_EXPECT( + ammAlice.expectBalances( + XRPAmount(11'000'000'000), + STAmount{USD, UINT64_C(9'372'781065088757), -12}, + IOUAmount{10'153'846'15384616, -8}) && + ammAlice.expectLPTokens( + carol, IOUAmount{153'846'15384616, -8})); + else + BEAST_EXPECT( + ammAlice.expectBalances( + XRPAmount(11'000'000'000), + STAmount{USD, UINT64_C(9'372'781065088769), -12}, + IOUAmount{10'153'846'15384616, -8}) && + ammAlice.expectLPTokens( + carol, IOUAmount{153'846'15384616, -8})); + ammAlice.withdrawAll(carol); + BEAST_EXPECT(ammAlice.expectLPTokens(carol, IOUAmount{0})); + }, + std::nullopt, + 0, + std::nullopt, + {all, all - fixAMMv1_1}); // Withdraw with EPrice limit. AssetOut is 0. - testAMM([&](AMM& ammAlice, Env&) { - ammAlice.deposit(carol, 1'000'000); - ammAlice.withdraw(carol, USD(0), std::nullopt, IOUAmount{520, 0}); - BEAST_EXPECT( - ammAlice.expectBalances( - XRPAmount(11'000'000'000), - STAmount{USD, UINT64_C(9'372'781065088757), -12}, - IOUAmount{10'153'846'15384616, -8}) && - ammAlice.expectLPTokens( - carol, IOUAmount{153'846'15384616, -8})); - }); + testAMM( + [&](AMM& ammAlice, Env& env) { + ammAlice.deposit(carol, 1'000'000); + ammAlice.withdraw( + carol, USD(0), std::nullopt, IOUAmount{520, 0}); + if (!env.current()->rules().enabled(fixAMMv1_1)) + BEAST_EXPECT( + ammAlice.expectBalances( + XRPAmount(11'000'000'000), + STAmount{USD, UINT64_C(9'372'781065088757), -12}, + IOUAmount{10'153'846'15384616, -8}) && + ammAlice.expectLPTokens( + carol, IOUAmount{153'846'15384616, -8})); + else + BEAST_EXPECT( + ammAlice.expectBalances( + XRPAmount(11'000'000'000), + STAmount{USD, UINT64_C(9'372'781065088769), -12}, + IOUAmount{10'153'846'15384616, -8}) && + ammAlice.expectLPTokens( + carol, IOUAmount{153'846'15384616, -8})); + }, + std::nullopt, + 0, + std::nullopt, + {all, all - fixAMMv1_1}); // IOU to IOU + transfer fee { @@ -2904,20 +2935,40 @@ struct AMM_test : public jtx::AMMTest ammAlice.withdraw(ed, tokens, USD(0)); } // carol, bob, and ed pay ~0.99USD in fees. - BEAST_EXPECT( - env.balance(carol, USD) == - STAmount(USD, UINT64_C(29'499'00572620545), -11)); - BEAST_EXPECT( - env.balance(bob, USD) == - STAmount(USD, UINT64_C(18'999'00572616195), -11)); - BEAST_EXPECT( - env.balance(ed, USD) == - STAmount(USD, UINT64_C(18'999'00572611841), -11)); - // USD pool is slightly higher because of the fees. - BEAST_EXPECT(ammAlice.expectBalances( - XRP(13'000), - STAmount(USD, UINT64_C(13'002'98282151419), -11), - ammTokens)); + if (!features[fixAMMv1_1]) + { + BEAST_EXPECT( + env.balance(carol, USD) == + STAmount(USD, UINT64_C(29'499'00572620545), -11)); + BEAST_EXPECT( + env.balance(bob, USD) == + STAmount(USD, UINT64_C(18'999'00572616195), -11)); + BEAST_EXPECT( + env.balance(ed, USD) == + STAmount(USD, UINT64_C(18'999'00572611841), -11)); + // USD pool is slightly higher because of the fees. + BEAST_EXPECT(ammAlice.expectBalances( + XRP(13'000), + STAmount(USD, UINT64_C(13'002'98282151419), -11), + ammTokens)); + } + else + { + BEAST_EXPECT( + env.balance(carol, USD) == + STAmount(USD, UINT64_C(29'499'00572620544), -11)); + BEAST_EXPECT( + env.balance(bob, USD) == + STAmount(USD, UINT64_C(18'999'00572616194), -11)); + BEAST_EXPECT( + env.balance(ed, USD) == + STAmount(USD, UINT64_C(18'999'0057261184), -10)); + // USD pool is slightly higher because of the fees. + BEAST_EXPECT(ammAlice.expectBalances( + XRP(13'000), + STAmount(USD, UINT64_C(13'002'98282151422), -11), + ammTokens)); + } ammTokens = ammAlice.getLPTokensBalance(); // Trade with the fee for (int i = 0; i < 10; ++i) @@ -2928,29 +2979,62 @@ struct AMM_test : public jtx::AMMTest // dan pays ~9.94USD, which is ~10 times more in fees than // carol, bob, ed. the discounted fee is 10 times less // than the trading fee. - BEAST_EXPECT( - env.balance(dan, USD) == - STAmount(USD, UINT64_C(19'490'056722744), -9)); - // USD pool gains more in dan's fees. - BEAST_EXPECT(ammAlice.expectBalances( - XRP(13'000), - STAmount{USD, UINT64_C(13'012'92609877019), -11}, - ammTokens)); - // Discounted fee payment - ammAlice.deposit(carol, USD(100)); - ammTokens = ammAlice.getLPTokensBalance(); - BEAST_EXPECT(ammAlice.expectBalances( - XRP(13'000), - STAmount{USD, UINT64_C(13'112'92609877019), -11}, - ammTokens)); - env(pay(carol, bob, USD(100)), path(~USD), sendmax(XRP(110))); - env.close(); - // carol pays 100000 drops in fees - // 99900668XRP swapped in for 100USD - BEAST_EXPECT(ammAlice.expectBalances( - XRPAmount{13'100'000'668}, - STAmount{USD, UINT64_C(13'012'92609877019), -11}, - ammTokens)); + if (!features[fixAMMv1_1]) + { + BEAST_EXPECT( + env.balance(dan, USD) == + STAmount(USD, UINT64_C(19'490'056722744), -9)); + // USD pool gains more in dan's fees. + BEAST_EXPECT(ammAlice.expectBalances( + XRP(13'000), + STAmount{USD, UINT64_C(13'012'92609877019), -11}, + ammTokens)); + // Discounted fee payment + ammAlice.deposit(carol, USD(100)); + ammTokens = ammAlice.getLPTokensBalance(); + BEAST_EXPECT(ammAlice.expectBalances( + XRP(13'000), + STAmount{USD, UINT64_C(13'112'92609877019), -11}, + ammTokens)); + env(pay(carol, bob, USD(100)), + path(~USD), + sendmax(XRP(110))); + env.close(); + // carol pays 100000 drops in fees + // 99900668XRP swapped in for 100USD + BEAST_EXPECT(ammAlice.expectBalances( + XRPAmount{13'100'000'668}, + STAmount{USD, UINT64_C(13'012'92609877019), -11}, + ammTokens)); + } + else + { + BEAST_EXPECT( + env.balance(dan, USD) == + STAmount(USD, UINT64_C(19'490'05672274399), -11)); + // USD pool gains more in dan's fees. + BEAST_EXPECT(ammAlice.expectBalances( + XRP(13'000), + STAmount{USD, UINT64_C(13'012'92609877023), -11}, + ammTokens)); + // Discounted fee payment + ammAlice.deposit(carol, USD(100)); + ammTokens = ammAlice.getLPTokensBalance(); + BEAST_EXPECT(ammAlice.expectBalances( + XRP(13'000), + STAmount{USD, UINT64_C(13'112'92609877023), -11}, + ammTokens)); + env(pay(carol, bob, USD(100)), + path(~USD), + sendmax(XRP(110))); + env.close(); + // carol pays 100000 drops in fees + // 99900668XRP swapped in for 100USD + BEAST_EXPECT(ammAlice.expectBalances( + XRPAmount{13'100'000'668}, + STAmount{USD, UINT64_C(13'012'92609877023), -11}, + ammTokens)); + } // Payment with the trading fee env(pay(alice, carol, XRP(100)), path(~XRP), sendmax(USD(110))); env.close(); @@ -2968,16 +3052,21 @@ struct AMM_test : public jtx::AMMTest { BEAST_EXPECT(ammAlice.expectBalances( XRPAmount{13'000'000'668}, - STAmount{USD, UINT64_C(13'114'03663047265), -11}, + STAmount{USD, UINT64_C(13'114'03663047269), -11}, ammTokens)); } // Auction slot expired, no discounted fee env.close(seconds(TOTAL_TIME_SLOT_SECS + 1)); // clock is parent's based env.close(); - BEAST_EXPECT( - env.balance(carol, USD) == - STAmount(USD, UINT64_C(29'399'00572620545), -11)); + if (!features[fixAMMv1_1]) + BEAST_EXPECT( + env.balance(carol, USD) == + STAmount(USD, UINT64_C(29'399'00572620545), -11)); + else + BEAST_EXPECT( + env.balance(carol, USD) == + STAmount(USD, UINT64_C(29'399'00572620544), -11)); ammTokens = ammAlice.getLPTokensBalance(); for (int i = 0; i < 10; ++i) { @@ -3000,10 +3089,10 @@ struct AMM_test : public jtx::AMMTest { BEAST_EXPECT( env.balance(carol, USD) == - STAmount(USD, UINT64_C(29'389'06197177127), -11)); + STAmount(USD, UINT64_C(29'389'06197177124), -11)); BEAST_EXPECT(ammAlice.expectBalances( XRPAmount{13'000'000'668}, - STAmount{USD, UINT64_C(13'123'98038490683), -11}, + STAmount{USD, UINT64_C(13'123'98038490689), -11}, ammTokens)); } env(pay(carol, bob, USD(100)), path(~USD), sendmax(XRP(110))); @@ -3022,14 +3111,14 @@ struct AMM_test : public jtx::AMMTest { BEAST_EXPECT(ammAlice.expectBalances( XRPAmount(13'100'824'790), - STAmount{USD, UINT64_C(13'023'98038490683), -11}, + STAmount{USD, UINT64_C(13'023'98038490689), -11}, ammTokens)); } }, std::nullopt, 1'000, std::nullopt, - features); + {features}); // Bid tiny amount testAMM( @@ -4497,8 +4586,11 @@ struct AMM_test : public jtx::AMMTest auto const tokensFee = ammAlice.withdraw( carol, USD(100), std::nullopt, IOUAmount{520, 0}); // carol withdraws ~1,443.44USD - auto const balanceAfterWithdraw = - STAmount(USD, UINT64_C(30'443'43891402715), -11); + auto const balanceAfterWithdraw = [&]() { + if (!features[fixAMMv1_1]) + return STAmount(USD, UINT64_C(30'443'43891402715), -11); + return STAmount(USD, UINT64_C(30'443'43891402714), -11); + }(); BEAST_EXPECT(env.balance(carol, USD) == balanceAfterWithdraw); // Set to original pool size auto const deposit = balanceAfterWithdraw - USD(29'000); @@ -4507,12 +4599,22 @@ struct AMM_test : public jtx::AMMTest ammAlice.vote(alice, 0); BEAST_EXPECT(ammAlice.expectTradingFee(0)); auto const tokensNoFee = ammAlice.withdraw(carol, deposit); - BEAST_EXPECT( - env.balance(carol, USD) == - STAmount(USD, UINT64_C(30'443'43891402717), -11)); + if (!features[fixAMMv1_1]) + BEAST_EXPECT( + env.balance(carol, USD) == + STAmount(USD, UINT64_C(30'443'43891402717), -11)); + else + BEAST_EXPECT( + env.balance(carol, USD) == + STAmount(USD, UINT64_C(30'443'43891402716), -11)); // carol pays ~4008 LPTokens in fees or ~0.5% of the no-fee // LPTokens - BEAST_EXPECT(tokensNoFee == IOUAmount(746'579'80779913, -8)); + if (!features[fixAMMv1_1]) + BEAST_EXPECT( + tokensNoFee == IOUAmount(746'579'80779913, -8)); + else + BEAST_EXPECT( + tokensNoFee == IOUAmount(746'579'80779912, -8)); BEAST_EXPECT(tokensFee == IOUAmount(750'588'23529411, -8)); }, std::nullopt, @@ -4762,74 +4864,101 @@ struct AMM_test : public jtx::AMMTest } void - testAdjustedTokens() + testAdjustedTokens(FeatureBitset features) { testcase("Adjusted Deposit/Withdraw Tokens"); using namespace jtx; // Deposit/Withdraw in USD - testAMM([&](AMM& ammAlice, Env& env) { - Account const bob("bob"); - Account const ed("ed"); - Account const paul("paul"); - Account const dan("dan"); - Account const chris("chris"); - Account const simon("simon"); - Account const ben("ben"); - Account const nataly("nataly"); - fund( - env, - gw, - {bob, ed, paul, dan, chris, simon, ben, nataly}, - {USD(1'500'000)}, - Fund::Acct); - for (int i = 0; i < 10; ++i) - { - ammAlice.deposit(ben, STAmount{USD, 1, -10}); - ammAlice.withdrawAll(ben, USD(0)); - ammAlice.deposit(simon, USD(0.1)); - ammAlice.withdrawAll(simon, USD(0)); - ammAlice.deposit(chris, USD(1)); - ammAlice.withdrawAll(chris, USD(0)); - ammAlice.deposit(dan, USD(10)); - ammAlice.withdrawAll(dan, USD(0)); - ammAlice.deposit(bob, USD(100)); - ammAlice.withdrawAll(bob, USD(0)); - ammAlice.deposit(carol, USD(1'000)); - ammAlice.withdrawAll(carol, USD(0)); - ammAlice.deposit(ed, USD(10'000)); - ammAlice.withdrawAll(ed, USD(0)); - ammAlice.deposit(paul, USD(100'000)); - ammAlice.withdrawAll(paul, USD(0)); - ammAlice.deposit(nataly, USD(1'000'000)); - ammAlice.withdrawAll(nataly, USD(0)); - } - // Due to round off some accounts have a tiny gain, while - // other have a tiny loss. The last account to withdraw - // gets everything in the pool. - BEAST_EXPECT(ammAlice.expectBalances( - XRP(10'000), - STAmount{USD, UINT64_C(10'000'0000000013), -10}, - IOUAmount{10'000'000})); - BEAST_EXPECT(expectLine(env, ben, USD(1'500'000))); - BEAST_EXPECT(expectLine(env, simon, USD(1'500'000))); - BEAST_EXPECT(expectLine(env, chris, USD(1'500'000))); - BEAST_EXPECT(expectLine(env, dan, USD(1'500'000))); - BEAST_EXPECT(expectLine( - env, carol, STAmount{USD, UINT64_C(30'000'00000000001), -11})); - BEAST_EXPECT(expectLine(env, ed, USD(1'500'000))); - BEAST_EXPECT(expectLine(env, paul, USD(1'500'000))); - BEAST_EXPECT(expectLine( - env, nataly, STAmount{USD, UINT64_C(1'500'000'000000002), -9})); - ammAlice.withdrawAll(alice); - BEAST_EXPECT(!ammAlice.ammExists()); - BEAST_EXPECT(expectLine( - env, alice, STAmount{USD, UINT64_C(30'000'0000000013), -10})); - // alice XRP balance is 30,000initial - 50 ammcreate fee - - // 10drops fee - BEAST_EXPECT(accountBalance(env, alice) == "29949999990"); - }); + testAMM( + [&](AMM& ammAlice, Env& env) { + Account const bob("bob"); + Account const ed("ed"); + Account const paul("paul"); + Account const dan("dan"); + Account const chris("chris"); + Account const simon("simon"); + Account const ben("ben"); + Account const nataly("nataly"); + fund( + env, + gw, + {bob, ed, paul, dan, chris, simon, ben, nataly}, + {USD(1'500'000)}, + Fund::Acct); + for (int i = 0; i < 10; ++i) + { + ammAlice.deposit(ben, STAmount{USD, 1, -10}); + ammAlice.withdrawAll(ben, USD(0)); + ammAlice.deposit(simon, USD(0.1)); + ammAlice.withdrawAll(simon, USD(0)); + ammAlice.deposit(chris, USD(1)); + ammAlice.withdrawAll(chris, USD(0)); + ammAlice.deposit(dan, USD(10)); + ammAlice.withdrawAll(dan, USD(0)); + ammAlice.deposit(bob, USD(100)); + ammAlice.withdrawAll(bob, USD(0)); + ammAlice.deposit(carol, USD(1'000)); + ammAlice.withdrawAll(carol, USD(0)); + ammAlice.deposit(ed, USD(10'000)); + ammAlice.withdrawAll(ed, USD(0)); + ammAlice.deposit(paul, USD(100'000)); + ammAlice.withdrawAll(paul, USD(0)); + ammAlice.deposit(nataly, USD(1'000'000)); + ammAlice.withdrawAll(nataly, USD(0)); + } + // Due to round off some accounts have a tiny gain, while + // other have a tiny loss. The last account to withdraw + // gets everything in the pool. + if (!features[fixAMMv1_1]) + BEAST_EXPECT(ammAlice.expectBalances( + XRP(10'000), + STAmount{USD, UINT64_C(10'000'0000000013), -10}, + IOUAmount{10'000'000})); + else + BEAST_EXPECT(ammAlice.expectBalances( + XRP(10'000), USD(10'000), IOUAmount{10'000'000})); + BEAST_EXPECT(expectLine(env, ben, USD(1'500'000))); + BEAST_EXPECT(expectLine(env, simon, USD(1'500'000))); + BEAST_EXPECT(expectLine(env, chris, USD(1'500'000))); + BEAST_EXPECT(expectLine(env, dan, USD(1'500'000))); + if (!features[fixAMMv1_1]) + BEAST_EXPECT(expectLine( + env, + carol, + STAmount{USD, UINT64_C(30'000'00000000001), -11})); + else + BEAST_EXPECT(expectLine(env, carol, USD(30'000))); + BEAST_EXPECT(expectLine(env, ed, USD(1'500'000))); + BEAST_EXPECT(expectLine(env, paul, USD(1'500'000))); + if (!features[fixAMMv1_1]) + BEAST_EXPECT(expectLine( + env, + nataly, + STAmount{USD, UINT64_C(1'500'000'000000002), -9})); + else + BEAST_EXPECT(expectLine( + env, + nataly, + STAmount{USD, UINT64_C(1'500'000'000000005), -9})); + ammAlice.withdrawAll(alice); + BEAST_EXPECT(!ammAlice.ammExists()); + if (!features[fixAMMv1_1]) + BEAST_EXPECT(expectLine( + env, + alice, + STAmount{USD, UINT64_C(30'000'0000000013), -10})); + else + BEAST_EXPECT(expectLine(env, alice, USD(30'000))); + // alice XRP balance is 30,000initial - 50 ammcreate fee - + // 10drops fee + BEAST_EXPECT(accountBalance(env, alice) == "29949999990"); + }, + std::nullopt, + 0, + std::nullopt, + {features}); // Same as above but deposit/withdraw in XRP testAMM([&](AMM& ammAlice, Env& env) { @@ -6440,7 +6569,8 @@ struct AMM_test : public jtx::AMMTest testAMMAndCLOB(all - fixAMMv1_1); testTradingFee(all); testTradingFee(all - fixAMMv1_1); - testAdjustedTokens(); + testAdjustedTokens(all); + testAdjustedTokens(all - fixAMMv1_1); testAutoDelete(); testClawback(); testAMMID(); diff --git a/src/test/jtx/AMMTest.h b/src/test/jtx/AMMTest.h index 7831713382f..5de805a10a7 100644 --- a/src/test/jtx/AMMTest.h +++ b/src/test/jtx/AMMTest.h @@ -84,7 +84,7 @@ class AMMTestBase : public beast::unit_test::suite std::optional> const& pool = std::nullopt, std::uint16_t tfee = 0, std::optional const& ter = std::nullopt, - std::optional const& features = std::nullopt); + std::vector const& features = {supported_amendments()}); }; class AMMTest : public jtx::AMMTestBase diff --git a/src/test/jtx/impl/AMMTest.cpp b/src/test/jtx/impl/AMMTest.cpp index 2c384a7d6f4..b9dea7d1fc3 100644 --- a/src/test/jtx/impl/AMMTest.cpp +++ b/src/test/jtx/impl/AMMTest.cpp @@ -103,44 +103,45 @@ AMMTestBase::testAMM( std::optional> const& pool, std::uint16_t tfee, std::optional const& ter, - std::optional const& features) + std::vector const& vfeatures) { using namespace jtx; - auto env = [&]() { - if (features) - return Env{*this, *features}; - return Env{*this}; - }(); - auto const [asset1, asset2] = - pool ? *pool : std::make_pair(XRP(10000), USD(10000)); - auto tofund = [&](STAmount const& a) -> STAmount { - if (a.native()) - { - auto const defXRP = XRP(30000); - if (a <= defXRP) - return defXRP; - return a + XRP(1000); - } - auto const defIOU = STAmount{a.issue(), 30000}; - if (a <= defIOU) - return defIOU; - return a + STAmount{a.issue(), 1000}; - }; - auto const toFund1 = tofund(asset1); - auto const toFund2 = tofund(asset2); - BEAST_EXPECT(asset1 <= toFund1 && asset2 <= toFund2); + for (auto const& features : vfeatures) + { + Env env{*this, features}; - if (!asset1.native() && !asset2.native()) - fund(env, gw, {alice, carol}, {toFund1, toFund2}, Fund::All); - else if (asset1.native()) - fund(env, gw, {alice, carol}, toFund1, {toFund2}, Fund::All); - else if (asset2.native()) - fund(env, gw, {alice, carol}, toFund2, {toFund1}, Fund::All); + auto const [asset1, asset2] = + pool ? *pool : std::make_pair(XRP(10000), USD(10000)); + auto tofund = [&](STAmount const& a) -> STAmount { + if (a.native()) + { + auto const defXRP = XRP(30000); + if (a <= defXRP) + return defXRP; + return a + XRP(1000); + } + auto const defIOU = STAmount{a.issue(), 30000}; + if (a <= defIOU) + return defIOU; + return a + STAmount{a.issue(), 1000}; + }; + auto const toFund1 = tofund(asset1); + auto const toFund2 = tofund(asset2); + BEAST_EXPECT(asset1 <= toFund1 && asset2 <= toFund2); + + if (!asset1.native() && !asset2.native()) + fund(env, gw, {alice, carol}, {toFund1, toFund2}, Fund::All); + else if (asset1.native()) + fund(env, gw, {alice, carol}, toFund1, {toFund2}, Fund::All); + else if (asset2.native()) + fund(env, gw, {alice, carol}, toFund2, {toFund1}, Fund::All); - AMM ammAlice(env, alice, asset1, asset2, false, tfee); - BEAST_EXPECT(ammAlice.expectBalances(asset1, asset2, ammAlice.tokens())); - cb(ammAlice, env); + AMM ammAlice(env, alice, asset1, asset2, false, tfee); + BEAST_EXPECT( + ammAlice.expectBalances(asset1, asset2, ammAlice.tokens())); + cb(ammAlice, env); + } } XRPAmount From 7f6a079aa4b28fc6ce0428310c81061dbeea35a1 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Sat, 4 May 2024 17:36:49 -0400 Subject: [PATCH 09/11] Fix offer crossing via single path AMM with transfer fee: Single path AMM offer has to factor in the transfer in rate when calculating the upper bound quality and the quality function because single path AMM's offer quality is not constant. This fix factors in the transfer fee in BookStep::adjustQualityWithFees(). --- src/ripple/app/paths/impl/BookStep.cpp | 57 +++++++- src/test/app/AMM_test.cpp | 194 +++++++++++++++++++++++++ 2 files changed, 245 insertions(+), 6 deletions(-) diff --git a/src/ripple/app/paths/impl/BookStep.cpp b/src/ripple/app/paths/impl/BookStep.cpp index 9d5ceea82a8..65dc8351680 100644 --- a/src/ripple/app/paths/impl/BookStep.cpp +++ b/src/ripple/app/paths/impl/BookStep.cpp @@ -275,6 +275,7 @@ class BookPaymentStep : public BookStep> using BookStep>::BookStep; using BookStep>::qualityUpperBound; + using typename BookStep>::OfferType; // Never limit self cross quality on a payment. template