From c7e0da5b8ce743f22b78782776ea548310235904 Mon Sep 17 00:00:00 2001 From: John Freeman Date: Wed, 27 Mar 2024 12:06:32 -0500 Subject: [PATCH 01/24] Add coverage --- Builds/CMake/RippledCore.cmake | 1 + src/ripple/app/tx/impl/AMMWithdraw.cpp | 12 +- src/ripple/protocol/STIssue.h | 6 - src/ripple/protocol/impl/STIssue.cpp | 12 -- src/test/app/AMMExtended_test.cpp | 3 + src/test/app/AMMWithdraw_test.cpp | 183 +++++++++++++++++++++++++ src/test/app/AMM_test.cpp | 10 +- src/test/jtx/AMM.h | 2 +- 8 files changed, 202 insertions(+), 27 deletions(-) create mode 100644 src/test/app/AMMWithdraw_test.cpp diff --git a/Builds/CMake/RippledCore.cmake b/Builds/CMake/RippledCore.cmake index f826c428135..712d01f33f7 100644 --- a/Builds/CMake/RippledCore.cmake +++ b/Builds/CMake/RippledCore.cmake @@ -806,6 +806,7 @@ if (tests) src/test/app/AMM_test.cpp src/test/app/AMMCalc_test.cpp src/test/app/AMMExtended_test.cpp + src/test/app/AMMWithdraw_test.cpp src/test/app/Check_test.cpp src/test/app/Clawback_test.cpp src/test/app/CrossingLimits_test.cpp diff --git a/src/ripple/app/tx/impl/AMMWithdraw.cpp b/src/ripple/app/tx/impl/AMMWithdraw.cpp index 58fc6b549ce..839c3b433aa 100644 --- a/src/ripple/app/tx/impl/AMMWithdraw.cpp +++ b/src/ripple/app/tx/impl/AMMWithdraw.cpp @@ -201,7 +201,7 @@ AMMWithdraw::preclaim(PreclaimContext const& ctx) { JLOG(ctx.j.debug()) << "AMM Withdraw: reserves or tokens balance is zero."; - return tecINTERNAL; + return tecINTERNAL; // LCOV_EXCL_LINE } auto const ammAccountID = ammSle->getAccountID(sfAccount); @@ -300,11 +300,11 @@ AMMWithdraw::applyGuts(Sandbox& sb) auto const ePrice = ctx_.tx[~sfEPrice]; auto ammSle = sb.peek(keylet::amm(ctx_.tx[sfAsset], ctx_.tx[sfAsset2])); if (!ammSle) - return {tecINTERNAL, false}; + return {tecINTERNAL, false}; // LCOV_EXCL_LINE auto const ammAccountID = (*ammSle)[sfAccount]; auto const accountSle = sb.read(keylet::account(ammAccountID)); if (!accountSle) - return {tecINTERNAL, false}; + return {tecINTERNAL, false}; // LCOV_EXCL_LINE auto const lpTokens = ammLPHolds(ctx_.view(), *ammSle, ctx_.tx[sfAccount], ctx_.journal); auto const lpTokensWithdraw = @@ -372,8 +372,10 @@ AMMWithdraw::applyGuts(Sandbox& sb) *lpTokensWithdraw, tfee); // should not happen. + // LCOV_EXCL_START JLOG(j_.error()) << "AMM Withdraw: invalid options."; return std::make_pair(tecINTERNAL, STAmount{}); + // LCOV_EXCL_STOP }(); if (result != tesSUCCESS) @@ -431,7 +433,7 @@ AMMWithdraw::withdraw( auto const ammSle = ctx_.view().read(keylet::amm(ctx_.tx[sfAsset], ctx_.tx[sfAsset2])); if (!ammSle) - return {tecINTERNAL, STAmount{}}; + return {tecINTERNAL, STAmount{}}; // LCOV_EXCL_LINE auto const lpTokens = ammLPHolds(view, *ammSle, account_, ctx_.journal); auto const expected = ammHolds( view, @@ -612,12 +614,14 @@ AMMWithdraw::equalWithdrawTokens( lpTokensWithdraw, tfee); } + // LCOV_EXCL_START catch (std::exception const& e) { JLOG(j_.error()) << "AMMWithdraw::equalWithdrawTokens exception " << e.what(); } return {tecINTERNAL, STAmount{}}; + // LCOV_EXCL_STOP } /** All assets withdrawal with the constraints on the maximum amount diff --git a/src/ripple/protocol/STIssue.h b/src/ripple/protocol/STIssue.h index 38ca136e017..223798a8911 100644 --- a/src/ripple/protocol/STIssue.h +++ b/src/ripple/protocol/STIssue.h @@ -56,9 +56,6 @@ class STIssue final : public STBase, CountedObject SerializedTypeID getSType() const override; - std::string - getText() const override; - Json::Value getJson(JsonOptions) const override; void @@ -71,9 +68,6 @@ class STIssue final : public STBase, CountedObject isDefault() const override; private: - static std::unique_ptr - construct(SerialIter&, SField const& name); - STBase* copy(std::size_t n, void* buf) const override; STBase* diff --git a/src/ripple/protocol/impl/STIssue.cpp b/src/ripple/protocol/impl/STIssue.cpp index 878a5b4c71b..356af438659 100644 --- a/src/ripple/protocol/impl/STIssue.cpp +++ b/src/ripple/protocol/impl/STIssue.cpp @@ -65,12 +65,6 @@ STIssue::getSType() const return STI_ISSUE; } -std::string -STIssue::getText() const -{ - return issue_.getText(); -} - Json::Value STIssue::getJson(JsonOptions) const { return to_json(issue_); @@ -97,12 +91,6 @@ STIssue::isDefault() const return issue_ == xrpIssue(); } -std::unique_ptr -STIssue::construct(SerialIter& sit, SField const& name) -{ - return std::make_unique(sit, name); -} - STBase* STIssue::copy(std::size_t n, void* buf) const { diff --git a/src/test/app/AMMExtended_test.cpp b/src/test/app/AMMExtended_test.cpp index 8c7cbce92f4..7b2f512543b 100644 --- a/src/test/app/AMMExtended_test.cpp +++ b/src/test/app/AMMExtended_test.cpp @@ -42,6 +42,9 @@ namespace ripple { namespace test { +/** + * Tests of AMM that use offers too. + */ struct AMMExtended_test : public jtx::AMMTest { private: diff --git a/src/test/app/AMMWithdraw_test.cpp b/src/test/app/AMMWithdraw_test.cpp new file mode 100644 index 00000000000..2c6d318acb6 --- /dev/null +++ b/src/test/app/AMMWithdraw_test.cpp @@ -0,0 +1,183 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2023 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include + +namespace ripple { +namespace test { + +struct AMMWithdraw_test : public jtx::AMMTest +{ + void + testDisabled() + { + using namespace jtx; + + // Each testAMM() call makes 27 tests. + testAMM([&](AMM& ammAlice, Env& env) { + env.disableFeature(featureAMM); + WithdrawArg args{.err = ter(temDISABLED)}; + ammAlice.withdraw(args); + }); + } + + void + testMalformed() + { + using namespace jtx; + + // { + // Env env{*this}; + // env.fund(XRP(30'000), alice); + // Json::Value jv; + // jv[jss::Account] = alice.human(); + // jv[jss::TransactionType] = jss::AMMWithdraw; + // env(jv, fee(drops(-10)), ter(temBAD_FEE)); + // } + + testAMM([&](AMM& ammAlice, Env& env) { + WithdrawArg args{ + .flags = tfSingleAsset, + .err = ter(temMALFORMED), + }; + ammAlice.withdraw(args); + }); + + testAMM([&](AMM& ammAlice, Env& env) { + WithdrawArg args{ + .flags = tfOneAssetLPToken, + .err = ter(temMALFORMED), + }; + ammAlice.withdraw(args); + }); + + testAMM([&](AMM& ammAlice, Env& env) { + WithdrawArg args{ + .flags = tfLimitLPToken, + .err = ter(temMALFORMED), + }; + ammAlice.withdraw(args); + }); + + testAMM([&](AMM& ammAlice, Env& env) { + WithdrawArg args{ + .asset1Out = XRP(100), + .asset2Out = XRP(100), + .err = ter(temBAD_AMM_TOKENS), + }; + ammAlice.withdraw(args); + }); + + testAMM([&](AMM& ammAlice, Env& env) { + WithdrawArg args{ + .asset1Out = XRP(100), + .asset2Out = BAD(100), + .err = ter(temBAD_CURRENCY), + }; + ammAlice.withdraw(args); + }); + + testAMM([&](AMM& ammAlice, Env& env) { + Json::Value jv; + jv[jss::TransactionType] = jss::AMMWithdraw; + jv[jss::Flags] = tfLimitLPToken; + jv[jss::Account] = alice.human(); + ammAlice.setTokens(jv); + XRP(100).value().setJson(jv[jss::Amount]); + USD(100).value().setJson(jv[jss::EPrice]); + env(jv, ter(temBAD_AMM_TOKENS)); + }); + } + + void + testOther() + { + using namespace jtx; + + testAMM( + [&](AMM& ammAlice, Env& env) { + WithdrawArg args{ + .asset1Out = XRP(100), + .err = ter(tecAMM_BALANCE), + }; + ammAlice.withdraw(args); + }, + {{XRP(99), USD(99)}}); + + testAMM( + [&](AMM& ammAlice, Env& env) { + WithdrawArg args{ + .asset1Out = USD(100), + .err = ter(tecAMM_BALANCE), + }; + ammAlice.withdraw(args); + }, + {{XRP(99), USD(99)}}); + + { + Env env{*this}; + env.fund(XRP(30'000), gw, alice, bob); + env.close(); + env(fset(gw, asfRequireAuth)); + env(trust(alice, gw["USD"](30'000), 0)); + env(trust(gw, alice["USD"](0), tfSetfAuth)); + // Bob trusts Gateway to owe him USD... + env(trust(bob, gw["USD"](30'000), 0)); + // ...but Gateway does not authorize Bob to hold its USD. + env.close(); + env(pay(gw, alice, USD(10'000))); + env.close(); + AMM ammAlice(env, alice, XRP(10'000), USD(10'000)); + WithdrawArg args{ + .account = bob, + .asset1Out = USD(100), + .err = ter(tecNO_AUTH), + }; + ammAlice.withdraw(args); + } + } + + void + run() override + { + testDisabled(); + testMalformed(); + testOther(); + } +}; + +BEAST_DEFINE_TESTSUITE_PRIO(AMMWithdraw, app, ripple, 1); + +} // namespace test +} // namespace ripple diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index b6828fab773..426673fccbe 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -37,6 +37,10 @@ namespace ripple { namespace test { +/** + * Basic tests of AMM that do not use offers. + * Tests incorporating offers are in `AMMExtended_test`. + */ struct AMM_test : public jtx::AMMTest { private: @@ -81,10 +85,8 @@ struct AMM_test : public jtx::AMMTest env.fund(XRP(30'000), gw, alice); env.close(); env(fset(gw, asfRequireAuth)); - env.close(); - env.trust(USD(30'000), alice); - env.close(); - env(trust(gw, alice["USD"](30'000)), txflags(tfSetfAuth)); + env(trust(alice, gw["USD"](30'000), 0)); + env(trust(gw, alice["USD"](0), tfSetfAuth)); env.close(); env(pay(gw, alice, USD(10'000))); env.close(); diff --git a/src/test/jtx/AMM.h b/src/test/jtx/AMM.h index 4c6e8d78a4e..570e7ffee29 100644 --- a/src/test/jtx/AMM.h +++ b/src/test/jtx/AMM.h @@ -391,12 +391,12 @@ class AMM return ammID_; } -private: void setTokens( Json::Value& jv, std::optional> const& assets = std::nullopt); +private: AccountID create( std::uint32_t tfee = 0, From c210ad4e5a9fa9c8a794f0870d58cd690dbda87d Mon Sep 17 00:00:00 2001 From: Howard Hinnant Date: Thu, 28 Mar 2024 16:43:50 -0400 Subject: [PATCH 02/24] Suppress warning: unused variable 'index' --- src/test/protocol/MultiApiJson_test.cpp | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/test/protocol/MultiApiJson_test.cpp b/src/test/protocol/MultiApiJson_test.cpp index a7d809bccbe..efd50af0a57 100644 --- a/src/test/protocol/MultiApiJson_test.cpp +++ b/src/test/protocol/MultiApiJson_test.cpp @@ -52,12 +52,6 @@ struct MultiApiJson_test : beast::unit_test::suite return obj1; } - constexpr static auto index = - [](unsigned int v) constexpr noexcept -> std::size_t - { - return v - 1; - }; - template constexpr static auto valid = [](unsigned int v) constexpr noexcept -> bool { From 779b381c8cc7f2dd18821b6fc95a3458aa83f36b Mon Sep 17 00:00:00 2001 From: Howard Hinnant Date: Thu, 28 Mar 2024 16:44:19 -0400 Subject: [PATCH 03/24] Remove unused code. --- src/ripple/app/paths/AMMOffer.h | 3 --- src/ripple/app/paths/impl/AMMOffer.cpp | 7 ------- 2 files changed, 10 deletions(-) diff --git a/src/ripple/app/paths/AMMOffer.h b/src/ripple/app/paths/AMMOffer.h index 10e6017dd96..4a805319013 100644 --- a/src/ripple/app/paths/AMMOffer.h +++ b/src/ripple/app/paths/AMMOffer.h @@ -75,9 +75,6 @@ class AMMOffer Issue const& issueIn() const; - Issue const& - issueOut() const; - AccountID const& owner() const; diff --git a/src/ripple/app/paths/impl/AMMOffer.cpp b/src/ripple/app/paths/impl/AMMOffer.cpp index 10b75b78565..d6431ee6db8 100644 --- a/src/ripple/app/paths/impl/AMMOffer.cpp +++ b/src/ripple/app/paths/impl/AMMOffer.cpp @@ -44,13 +44,6 @@ AMMOffer::issueIn() const return ammLiquidity_.issueIn(); } -template -Issue const& -AMMOffer::issueOut() const -{ - return ammLiquidity_.issueOut(); -} - template AccountID const& AMMOffer::owner() const From c688be6e2f568250fd1c521ba09423c2edf27481 Mon Sep 17 00:00:00 2001 From: Mark Travis Date: Thu, 28 Mar 2024 11:31:43 -0700 Subject: [PATCH 04/24] Tests for AMMBid::preflight() --- src/test/app/AMM_test.cpp | 49 +++++++++++++++++++++++++++++++++++++++ src/test/jtx/AMM.h | 26 ++++++++++----------- src/test/jtx/Env.h | 7 ++++++ src/test/jtx/impl/AMM.cpp | 33 +++++++++++++------------- src/test/jtx/impl/Env.cpp | 26 +++++++++++++++++++++ 5 files changed, 111 insertions(+), 30 deletions(-) diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 426673fccbe..8462d1189a1 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -2824,6 +2825,54 @@ struct AMM_test : public jtx::AMMTest STAmount{USD, UINT64_C(1'010'10090898081), -11}, IOUAmount{1'004'487'562112089, -9})); } + + // preflight tests + { + Env env(*this); + fund(env, gw, {alice, bob}, XRP(2'000), {USD(2'000)}); + AMM amm(env, gw, XRP(1'000), USD(1'010), false, 1'000); + Json::Value tx = amm.bidJson(alice, 500); + + { + auto jtx = env.jt( + tx, + seq(1), + fee(10)); + env.app().config().features.erase(featureAMM); + PreflightContext pfctx(env.app(), *jtx.stx, + env.current()->rules(), tapNONE, env.journal); + auto pf = AMMBid::preflight(pfctx); + BEAST_EXPECT(pf == temDISABLED); + env.app().config().features.insert(featureAMM); + } + + { + auto jtx = env.jt( + tx, + seq(1), + fee(10)); + jtx.jv["TxnSignature"] = "deadbeef"; + jtx.stx = env.ust(jtx); + PreflightContext pfctx(env.app(), *jtx.stx, + env.current()->rules(), tapNONE, env.journal); + auto pf = AMMBid::preflight(pfctx); + BEAST_EXPECT(pf != tesSUCCESS); + } + + { + auto jtx = env.jt( + tx, + seq(1), + fee(10)); + jtx.jv["Asset2"]["currency"] = "XRP"; + jtx.jv["Asset2"].removeMember("issuer"); + jtx.stx = env.ust(jtx); + PreflightContext pfctx(env.app(), *jtx.stx, + env.current()->rules(), tapNONE, env.journal); + auto pf = AMMBid::preflight(pfctx); + BEAST_EXPECT(pf == temBAD_AMM_TOKENS); + } + } } void diff --git a/src/test/jtx/AMM.h b/src/test/jtx/AMM.h index 570e7ffee29..b1c82b54e83 100644 --- a/src/test/jtx/AMM.h +++ b/src/test/jtx/AMM.h @@ -106,18 +106,6 @@ struct VoteArg std::optional err = std::nullopt; }; -struct BidArg -{ - std::optional account = std::nullopt; - std::optional> bidMin = std::nullopt; - std::optional> bidMax = std::nullopt; - std::vector authAccounts = {}; - std::optional flags = std::nullopt; - std::optional seq = std::nullopt; - std::optional> assets = std::nullopt; - std::optional err = std::nullopt; -}; - /** Convenience class to test AMM functionality. */ class AMM @@ -329,8 +317,18 @@ class AMM std::optional> const& assets = std::nullopt, std::optional const& ter = std::nullopt); - void - bid(BidArg const& arg); + // Return the JSON for a bid, but don't execute it. + Json::Value + bidJson(std::optional const& account, + std::optional> const& bidMin = + std::nullopt, + std::optional> const& bidMax = + std::nullopt, + std::vector const& authAccounts = {}, + std::optional const& flags = std::nullopt, + std::optional const& seq = std::nullopt, + std::optional> const& assets = std::nullopt, + std::optional const& ter = std::nullopt); AccountID const& ammAccount() const diff --git a/src/test/jtx/Env.h b/src/test/jtx/Env.h index 72cac29040a..595cf1a1bec 100644 --- a/src/test/jtx/Env.h +++ b/src/test/jtx/Env.h @@ -661,6 +661,13 @@ class Env } /** @} */ + /** Create a STTx from a JTx without sanitizing + Use to inject bogus values into test transactions by first + editing the JSON. + */ + std::shared_ptr + ust(JTx const& jt); + protected: int trace_ = 0; TestStopwatch stopwatch_; diff --git a/src/test/jtx/impl/AMM.cpp b/src/test/jtx/impl/AMM.cpp index 2d5ce90d306..e606018090f 100644 --- a/src/test/jtx/impl/AMM.cpp +++ b/src/test/jtx/impl/AMM.cpp @@ -666,9 +666,24 @@ AMM::bid( std::optional const& seq, std::optional> const& assets, std::optional const& ter) +{ + submit(bidJson(account, bidMin, bidMax, authAccounts, flags, seq, assets, + ter), seq, ter); +} + +Json::Value +AMM::bidJson( + std::optional const& account, + std::optional> const& bidMin, + std::optional> const& bidMax, + std::vector const& authAccounts, + std::optional const& flags, + std::optional const& seq, + std::optional> const& assets, + std::optional const& ter) { if (auto const amm = - env_.current()->read(keylet::amm(asset1_.issue(), asset2_.issue()))) + env_.current()->read(keylet::amm(asset1_.issue(), asset2_.issue()))) { assert( !env_.current()->rules().enabled(fixInnerObjTemplate) || @@ -724,21 +739,7 @@ AMM::bid( jv[jss::TransactionType] = jss::AMMBid; if (fee_ != 0) jv[jss::Fee] = std::to_string(fee_); - submit(jv, seq, ter); -} - -void -AMM::bid(BidArg const& arg) -{ - return bid( - arg.account, - arg.bidMin, - arg.bidMax, - arg.authAccounts, - arg.flags, - arg.seq, - arg.assets, - arg.err); + return jv; } void diff --git a/src/test/jtx/impl/Env.cpp b/src/test/jtx/impl/Env.cpp index 6496f7df1d2..17d2d511846 100644 --- a/src/test/jtx/impl/Env.cpp +++ b/src/test/jtx/impl/Env.cpp @@ -472,6 +472,32 @@ Env::st(JTx const& jt) return nullptr; } +std::shared_ptr +Env::ust(JTx const& jt) +{ + // The parse must succeed, since we + // generated the JSON ourselves. + std::optional obj; + try + { + obj = jtx::parse(jt.jv); + } + catch (jtx::parse_error const&) + { + test.log << "Exception: parse_error\n" << pretty(jt.jv) << std::endl; + Rethrow(); + } + + try + { + return std::make_shared(std::move(*obj)); + } + catch (std::exception const&) + { + } + return nullptr; +} + Json::Value Env::do_rpc( unsigned apiVersion, From f6b1f68d755d9e8d68a66948027e5f4e8b5cf4a4 Mon Sep 17 00:00:00 2001 From: Mark Travis Date: Thu, 28 Mar 2024 18:07:17 -0700 Subject: [PATCH 05/24] Test comparison of issued assets to cover STIssue.h. --- src/test/app/AMM_test.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 8462d1189a1..30dde10a4ed 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -120,6 +120,12 @@ struct AMM_test : public jtx::AMMTest }, std::nullopt, 1'000); + + // Make sure asset comparison works. + BEAST_EXPECT(STIssue(sfAsset, STAmount(XRP(2'000)).issue()) == + STIssue(sfAsset, STAmount(XRP(2'000)).issue())); + BEAST_EXPECT(STIssue(sfAsset, STAmount(XRP(2'000)).issue()) != + STIssue(sfAsset, STAmount({USD(2'000)}).issue())); } void From a8c0884b075737f0fa76a4c7618aa38dae6e0ff5 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Mon, 25 Mar 2024 22:34:34 +0000 Subject: [PATCH 06/24] Increase unit tests coverage --- src/test/app/AMM_test.cpp | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 30dde10a4ed..465a89a702b 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -3750,6 +3750,32 @@ struct AMM_test : public jtx::AMMTest Env env{*this, feature}; fund(env, gw, {alice}, {USD(1'000)}, Fund::All); AMM amm(env, alice, XRP(1'000), USD(1'000), ter(temDISABLED)); + amm.bid( + alice, + 1000, + std::nullopt, + {}, + std::nullopt, + std::nullopt, + std::nullopt, + ter(temMALFORMED)); + amm.vote( + std::nullopt, + 100, + std::nullopt, + std::nullopt, + std::nullopt, + ter(temDISABLED)); + amm.withdraw( + alice, 100, std::nullopt, std::nullopt, ter(temMALFORMED)); + amm.deposit( + alice, + USD(100), + std::nullopt, + std::nullopt, + std::nullopt, + ter(temDISABLED)); + amm.ammDelete(alice, ter(temDISABLED)); } } @@ -4448,6 +4474,9 @@ struct AMM_test : public jtx::AMMTest amm.ammDelete(alice); BEAST_EXPECT(!amm.ammExists()); BEAST_EXPECT(!env.le(keylet::ownerDir(amm.ammAccount()))); + + // Try redundant delete + amm.ammDelete(alice, ter(terNO_AMM)); } } From 48ce3994e737cf06e61061f012c3828776522d02 Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Tue, 26 Mar 2024 18:08:10 -0400 Subject: [PATCH 07/24] update test coverage for AMMDeposit --- src/ripple/app/misc/impl/AMMUtils.cpp | 14 ++-- src/ripple/app/tx/impl/AMMDeposit.cpp | 41 ++++++---- src/test/app/AMM_test.cpp | 107 +++++++++++++++++++++++++- 3 files changed, 142 insertions(+), 20 deletions(-) diff --git a/src/ripple/app/misc/impl/AMMUtils.cpp b/src/ripple/app/misc/impl/AMMUtils.cpp index d766bc508b6..95a716230b9 100644 --- a/src/ripple/app/misc/impl/AMMUtils.cpp +++ b/src/ripple/app/misc/impl/AMMUtils.cpp @@ -213,7 +213,8 @@ deleteAMMTrustLines( JLOG(j.error()) << "deleteAMMTrustLines: deleting non-trustline " << nodeType; - return {tecINTERNAL, SkipEntry::No}; + return {tecINTERNAL, SkipEntry::No}; // LCOV_EXCL_LINE exclude + // from test coverage } // Trustlines must have zero balance @@ -222,7 +223,8 @@ deleteAMMTrustLines( JLOG(j.error()) << "deleteAMMTrustLines: deleting trustline with " "non-zero balance."; - return {tecINTERNAL, SkipEntry::No}; + return {tecINTERNAL, SkipEntry::No}; // LCOV_EXCL_LINE exclude + // from test coverage } return { @@ -245,7 +247,7 @@ deleteAMMAccount( { JLOG(j.error()) << "deleteAMMAccount: AMM object does not exist " << asset << " " << asset2; - return tecINTERNAL; + return tecINTERNAL; // LCOV_EXCL_LINE exclude from test coverage } auto const ammAccountID = (*ammSle)[sfAccount]; @@ -254,7 +256,7 @@ deleteAMMAccount( { JLOG(j.error()) << "deleteAMMAccount: AMM account does not exist " << to_string(ammAccountID); - return tecINTERNAL; + return tecINTERNAL; // LCOV_EXCL_LINE exclude from test coverage } if (auto const ter = @@ -267,13 +269,13 @@ deleteAMMAccount( ownerDirKeylet, (*ammSle)[sfOwnerNode], ammSle->key(), false)) { JLOG(j.error()) << "deleteAMMAccount: failed to remove dir link"; - return tecINTERNAL; + return tecINTERNAL; // LCOV_EXCL_LINE exclude from test coverage } if (sb.exists(ownerDirKeylet) && !sb.emptyDirDelete(ownerDirKeylet)) { JLOG(j.error()) << "deleteAMMAccount: cannot delete root dir node of " << toBase58(ammAccountID); - return tecINTERNAL; + return tecINTERNAL; // LCOV_EXCL_LINE exclude from test coverage } sb.erase(ammSle); diff --git a/src/ripple/app/tx/impl/AMMDeposit.cpp b/src/ripple/app/tx/impl/AMMDeposit.cpp index 5ea49f5445c..7bbd8750cf7 100644 --- a/src/ripple/app/tx/impl/AMMDeposit.cpp +++ b/src/ripple/app/tx/impl/AMMDeposit.cpp @@ -194,8 +194,9 @@ AMMDeposit::preclaim(PreclaimContext const& ctx) return tecAMM_NOT_EMPTY; if (amountBalance != beast::zero || amount2Balance != beast::zero) { - JLOG(ctx.j.debug()) << "AMM Deposit: tokens balance is not zero."; - return tecINTERNAL; + JLOG(ctx.j.debug()) + << "AMM Deposit: tokens balance is not zero."; // LCOV_EXCL_LINE + return tecINTERNAL; // LCOV_EXCL_LINE exclude from test coverage } } else @@ -205,9 +206,10 @@ AMMDeposit::preclaim(PreclaimContext const& ctx) if (amountBalance <= beast::zero || amount2Balance <= beast::zero || lptAMMBalance < beast::zero) { - JLOG(ctx.j.debug()) - << "AMM Deposit: reserves or tokens balance is zero."; - return tecINTERNAL; + JLOG(ctx.j.debug()) // LCOV_EXCL_LINE exclude from test coverage + << "AMM Deposit: reserves or " // LCOV_EXCL_LINE + << "tokens balance is zero."; // LCOV_EXCL_LINE + return tecINTERNAL; // LCOV_EXCL_LINE exclude from test coverage } } @@ -254,10 +256,17 @@ AMMDeposit::preclaim(PreclaimContext const& ctx) if (auto const ter = requireAuth(ctx.view, amount->issue(), accountID)) { - JLOG(ctx.j.debug()) - << "AMM Deposit: account is not authorized, " - << amount->issue(); - return ter; + JLOG(ctx.j.debug()) // LCOV_EXCL_LINE exclude from test + // coverage + << "AMM Deposit: account is not " + "authorized, " // LCOV_EXCL_LINE + // exclude + // from test + // coverage + << amount->issue(); // LCOV_EXCL_LINE + // exclude from test + // coverage + return ter; // LCOV_EXCL_LINE exclude from test coverage } // AMM account or currency frozen if (isFrozen(ctx.view, ammAccountID, amount->issue())) @@ -339,7 +348,8 @@ AMMDeposit::applyGuts(Sandbox& sb) auto const lpTokensDeposit = ctx_.tx[~sfLPTokenOut]; auto ammSle = sb.peek(keylet::amm(ctx_.tx[sfAsset], ctx_.tx[sfAsset2])); if (!ammSle) - return {tecINTERNAL, false}; + return { + tecINTERNAL, false}; // LCOV_EXCL_LINE exclude from test coverage auto const ammAccountID = (*ammSle)[sfAccount]; auto const expected = ammHolds( @@ -421,8 +431,12 @@ AMMDeposit::applyGuts(Sandbox& sb) lptAMMBalance.issue(), tfee); // should not happen. - JLOG(j_.error()) << "AMM Deposit: invalid options."; - return std::make_pair(tecINTERNAL, STAmount{}); + JLOG(j_.error()) + << "AMM Deposit: invalid options."; // LCOV_EXCL_LINE exclude from + // test coverage + return std::make_pair( + tecINTERNAL, + STAmount{}); // LCOV_EXCL_LINE exclude from test coverage }(); if (result == tesSUCCESS) @@ -624,7 +638,8 @@ AMMDeposit::equalDepositTokens( JLOG(j_.error()) << "AMMDeposit::equalDepositTokens exception " << e.what(); } - return {tecINTERNAL, STAmount{}}; + return { + tecINTERNAL, STAmount{}}; // LCOV_EXCL_LINE exclude from test coverage } /** Proportional deposit of pool assets with the constraints on the maximum diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 465a89a702b..086ed909651 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -608,7 +608,12 @@ struct AMM_test : public jtx::AMMTest USD(100), STAmount{USD, 1, -1}, std::nullopt}, - }; + {tfTwoAssetIfEmpty | tfLPToken, + std::nullopt, + XRP(100), + USD(100), + STAmount{USD, 1, -1}, + std::nullopt}}; for (auto const& it : invalidOptions) { ammAlice.deposit( @@ -624,6 +629,19 @@ struct AMM_test : public jtx::AMMTest ter(temMALFORMED)); } + { + // bad preflight1 + Json::Value jv = Json::objectValue; + jv[jss::Account] = alice.human(); + jv[jss::TransactionType] = jss::AMMDeposit; + jv[jss::Asset] = + STIssue(sfAsset, XRP).getJson(JsonOptions::none); + jv[jss::Asset2] = + STIssue(sfAsset, USD).getJson(JsonOptions::none); + jv[jss::Fee] = "-1"; + env(jv, ter(temBAD_FEE)); + } + // Invalid tokens ammAlice.deposit( alice, 0, std::nullopt, std::nullopt, ter(temBAD_AMM_TOKENS)); @@ -634,6 +652,33 @@ struct AMM_test : public jtx::AMMTest std::nullopt, ter(temBAD_AMM_TOKENS)); + { + Json::Value jv = Json::objectValue; + jv[jss::Account] = alice.human(); + jv[jss::TransactionType] = jss::AMMDeposit; + jv[jss::Asset] = + STIssue(sfAsset, XRP).getJson(JsonOptions::none); + jv[jss::Asset2] = + STIssue(sfAsset, USD).getJson(JsonOptions::none); + jv[jss::LPTokenOut] = + USD(100).value().getJson(JsonOptions::none); + jv[jss::Flags] = tfLPToken; + env(jv, ter(temBAD_AMM_TOKENS)); + } + + // Invalid trading fee + ammAlice.deposit( + carol, + std::nullopt, + XRP(200), + USD(200), + std::nullopt, + tfTwoAssetIfEmpty, + std::nullopt, + std::nullopt, + 10'000, + ter(temBAD_FEE)); + // Invalid tokens - bogus currency { auto const iss1 = Issue{Currency(0xabc), gw.id()}; @@ -830,6 +875,13 @@ struct AMM_test : public jtx::AMMTest ter(tecFROZEN)); ammAlice.deposit( carol, 1'000'000, std::nullopt, std::nullopt, ter(tecFROZEN)); + ammAlice.deposit( + carol, + XRP(100), + USD(100), + std::nullopt, + std::nullopt, + ter(tecFROZEN)); }); // Individually frozen (AMM) account @@ -867,6 +919,50 @@ struct AMM_test : public jtx::AMMTest ter(tecFROZEN)); }); + // Individually frozen (AMM) account with IOU/IOU AMM + testAMM( + [&](AMM& ammAlice, Env& env) { + env(trust(gw, carol["USD"](0), tfSetFreeze)); + env(trust(gw, carol["BTC"](0), tfSetFreeze)); + env.close(); + ammAlice.deposit( + carol, + 1'000'000, + std::nullopt, + std::nullopt, + ter(tecFROZEN)); + ammAlice.deposit( + carol, + USD(100), + std::nullopt, + std::nullopt, + std::nullopt, + ter(tecFROZEN)); + env(trust(gw, carol["USD"](0), tfClearFreeze)); + // Individually frozen AMM + env(trust( + gw, + STAmount{ + Issue{gw["USD"].currency, ammAlice.ammAccount()}, 0}, + tfSetFreeze)); + env.close(); + // Can deposit non-frozen token + ammAlice.deposit( + carol, + 1'000'000, + std::nullopt, + std::nullopt, + ter(tecFROZEN)); + ammAlice.deposit( + carol, + USD(100), + BTC(0.01), + std::nullopt, + std::nullopt, + ter(tecFROZEN)); + }, + {{USD(20'000), BTC(0.5)}}); + // Insufficient XRP balance testAMM([&](AMM& ammAlice, Env& env) { env.fund(XRP(1'000), bob); @@ -956,6 +1052,15 @@ struct AMM_test : public jtx::AMMTest std::nullopt, std::nullopt, ter(tecINSUF_RESERVE_LINE)); + + env(offer(carol, XRP(100), USD(103))); + ammAlice.deposit( + carol, + USD(100), + std::nullopt, + std::nullopt, + std::nullopt, + ter(tecINSUF_RESERVE_LINE)); } // Insufficient reserve, IOU/IOU From faad55f43d745da498dba2b1802678e4ef87bd00 Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Wed, 27 Mar 2024 16:53:15 -0400 Subject: [PATCH 08/24] remove unreachable lines from codecov --- src/ripple/app/misc/impl/AMMUtils.cpp | 40 +++++++++++++------ src/ripple/app/tx/impl/AMMDeposit.cpp | 55 ++++++++++++--------------- 2 files changed, 54 insertions(+), 41 deletions(-) diff --git a/src/ripple/app/misc/impl/AMMUtils.cpp b/src/ripple/app/misc/impl/AMMUtils.cpp index 95a716230b9..138dd80dd1a 100644 --- a/src/ripple/app/misc/impl/AMMUtils.cpp +++ b/src/ripple/app/misc/impl/AMMUtils.cpp @@ -60,9 +60,12 @@ ammHolds( *optIssue2, std::make_optional(std::make_pair(issue1, issue2)))) { + // This error can only be hit if the AMM is corrupted + // LCOV_EXCL_START JLOG(j.debug()) << "ammHolds: Invalid optIssue1 or optIssue2 " << *optIssue1 << " " << *optIssue2; return std::nullopt; + // LCOV_EXCL_STOP } return std::make_optional(std::make_pair(*optIssue1, *optIssue2)); } @@ -74,9 +77,12 @@ ammHolds( return std::make_optional(std::make_pair(issue1, issue2)); else if (checkIssue == issue2) return std::make_optional(std::make_pair(issue2, issue1)); + // This error can only be hit if the AMM is corrupted + // LCOV_EXCL_START JLOG(j.debug()) << "ammHolds: Invalid " << label << " " << checkIssue; return std::nullopt; + // LCOV_EXCL_STOP }; if (optIssue1) { @@ -84,7 +90,9 @@ ammHolds( } else if (optIssue2) { - return singleIssue(*optIssue2, "optIssue2"); + // This line cannot be hit since you cannot have Amount2 + // without Amount + return singleIssue(*optIssue2, "optIssue2"); // LCOV_EXCL_LINE } return std::make_optional(std::make_pair(issue1, issue2)); }(); @@ -210,21 +218,23 @@ deleteAMMTrustLines( // Should only have the trustlines if (nodeType != LedgerEntryType::ltRIPPLE_STATE) { + // LCOV_EXCL_START JLOG(j.error()) << "deleteAMMTrustLines: deleting non-trustline " << nodeType; - return {tecINTERNAL, SkipEntry::No}; // LCOV_EXCL_LINE exclude - // from test coverage + return {tecINTERNAL, SkipEntry::No}; + // LCOV_EXCL_STOP } // Trustlines must have zero balance if (sleItem->getFieldAmount(sfBalance) != beast::zero) { + // LCOV_EXCL_START JLOG(j.error()) << "deleteAMMTrustLines: deleting trustline with " "non-zero balance."; - return {tecINTERNAL, SkipEntry::No}; // LCOV_EXCL_LINE exclude - // from test coverage + return {tecINTERNAL, SkipEntry::No}; + // LCOV_EXCL_STOP } return { @@ -245,18 +255,22 @@ deleteAMMAccount( auto ammSle = sb.peek(keylet::amm(asset, asset2)); if (!ammSle) { + // LCOV_EXCL_START JLOG(j.error()) << "deleteAMMAccount: AMM object does not exist " << asset << " " << asset2; - return tecINTERNAL; // LCOV_EXCL_LINE exclude from test coverage + return tecINTERNAL; + // LCOV_EXCL_STOP } auto const ammAccountID = (*ammSle)[sfAccount]; auto sleAMMRoot = sb.peek(keylet::account(ammAccountID)); if (!sleAMMRoot) { + // LCOV_EXCL_START JLOG(j.error()) << "deleteAMMAccount: AMM account does not exist " << to_string(ammAccountID); - return tecINTERNAL; // LCOV_EXCL_LINE exclude from test coverage + return tecINTERNAL; + // LCOV_EXCL_STOP } if (auto const ter = @@ -268,14 +282,18 @@ deleteAMMAccount( if (!sb.dirRemove( ownerDirKeylet, (*ammSle)[sfOwnerNode], ammSle->key(), false)) { + // LCOV_EXCL_START JLOG(j.error()) << "deleteAMMAccount: failed to remove dir link"; - return tecINTERNAL; // LCOV_EXCL_LINE exclude from test coverage + return tecINTERNAL; + // LCOV_EXCL_STOP } if (sb.exists(ownerDirKeylet) && !sb.emptyDirDelete(ownerDirKeylet)) { + // LCOV_EXCL_START JLOG(j.error()) << "deleteAMMAccount: cannot delete root dir node of " << toBase58(ammAccountID); - return tecINTERNAL; // LCOV_EXCL_LINE exclude from test coverage + return tecINTERNAL; + // LCOV_EXCL_STOP } sb.erase(ammSle); @@ -324,11 +342,11 @@ initializeFeeAuctionVote( if (tfee != 0) ammSle->setFieldU16(sfTradingFee, tfee); else if (ammSle->isFieldPresent(sfTradingFee)) - ammSle->makeFieldAbsent(sfTradingFee); + ammSle->makeFieldAbsent(sfTradingFee); // LCOV_EXCL_LINE never hit if (auto const dfee = tfee / AUCTION_SLOT_DISCOUNTED_FEE_FRACTION) auctionSlot.setFieldU16(sfDiscountedFee, dfee); else if (auctionSlot.isFieldPresent(sfDiscountedFee)) - auctionSlot.makeFieldAbsent(sfDiscountedFee); + auctionSlot.makeFieldAbsent(sfDiscountedFee); // LCOV_EXCL_LINE not hit } } // namespace ripple diff --git a/src/ripple/app/tx/impl/AMMDeposit.cpp b/src/ripple/app/tx/impl/AMMDeposit.cpp index 7bbd8750cf7..74cba160edc 100644 --- a/src/ripple/app/tx/impl/AMMDeposit.cpp +++ b/src/ripple/app/tx/impl/AMMDeposit.cpp @@ -186,7 +186,7 @@ AMMDeposit::preclaim(PreclaimContext const& ctx) FreezeHandling::fhIGNORE_FREEZE, ctx.j); if (!expected) - return expected.error(); + return expected.error(); // LCOV_EXCL_LINE auto const [amountBalance, amount2Balance, lptAMMBalance] = *expected; if (ctx.tx.getFlags() & tfTwoAssetIfEmpty) { @@ -194,9 +194,10 @@ AMMDeposit::preclaim(PreclaimContext const& ctx) return tecAMM_NOT_EMPTY; if (amountBalance != beast::zero || amount2Balance != beast::zero) { - JLOG(ctx.j.debug()) - << "AMM Deposit: tokens balance is not zero."; // LCOV_EXCL_LINE - return tecINTERNAL; // LCOV_EXCL_LINE exclude from test coverage + // LCOV_EXCL_START + JLOG(ctx.j.debug()) << "AMM Deposit: tokens balance is not zero."; + return tecINTERNAL; + // LCOV_EXCL_STOP } } else @@ -206,10 +207,11 @@ AMMDeposit::preclaim(PreclaimContext const& ctx) if (amountBalance <= beast::zero || amount2Balance <= beast::zero || lptAMMBalance < beast::zero) { - JLOG(ctx.j.debug()) // LCOV_EXCL_LINE exclude from test coverage - << "AMM Deposit: reserves or " // LCOV_EXCL_LINE - << "tokens balance is zero."; // LCOV_EXCL_LINE - return tecINTERNAL; // LCOV_EXCL_LINE exclude from test coverage + // LCOV_EXCL_START + JLOG(ctx.j.debug()) + << "AMM Deposit: reserves or tokens balance is zero."; + return tecINTERNAL; + // LCOV_EXCL_STOP } } @@ -256,17 +258,12 @@ AMMDeposit::preclaim(PreclaimContext const& ctx) if (auto const ter = requireAuth(ctx.view, amount->issue(), accountID)) { - JLOG(ctx.j.debug()) // LCOV_EXCL_LINE exclude from test - // coverage - << "AMM Deposit: account is not " - "authorized, " // LCOV_EXCL_LINE - // exclude - // from test - // coverage - << amount->issue(); // LCOV_EXCL_LINE - // exclude from test - // coverage - return ter; // LCOV_EXCL_LINE exclude from test coverage + // LCOV_EXCL_START + JLOG(ctx.j.debug()) + << "AMM Deposit: account is not authorized, " + << amount->issue(); + return ter; + // LCOV_EXCL_STOP } // AMM account or currency frozen if (isFrozen(ctx.view, ammAccountID, amount->issue())) @@ -348,8 +345,7 @@ AMMDeposit::applyGuts(Sandbox& sb) auto const lpTokensDeposit = ctx_.tx[~sfLPTokenOut]; auto ammSle = sb.peek(keylet::amm(ctx_.tx[sfAsset], ctx_.tx[sfAsset2])); if (!ammSle) - return { - tecINTERNAL, false}; // LCOV_EXCL_LINE exclude from test coverage + return {tecINTERNAL, false}; // LCOV_EXCL_LINE auto const ammAccountID = (*ammSle)[sfAccount]; auto const expected = ammHolds( @@ -360,7 +356,7 @@ AMMDeposit::applyGuts(Sandbox& sb) FreezeHandling::fhZERO_IF_FROZEN, ctx_.journal); if (!expected) - return {expected.error(), false}; + return {expected.error(), false}; // LCOV_EXCL_LINE auto const [amountBalance, amount2Balance, lptAMMBalance] = *expected; auto const tfee = (lptAMMBalance == beast::zero) ? ctx_.tx[~sfTradingFee].value_or(0) @@ -431,12 +427,10 @@ AMMDeposit::applyGuts(Sandbox& sb) lptAMMBalance.issue(), tfee); // should not happen. - JLOG(j_.error()) - << "AMM Deposit: invalid options."; // LCOV_EXCL_LINE exclude from - // test coverage - return std::make_pair( - tecINTERNAL, - STAmount{}); // LCOV_EXCL_LINE exclude from test coverage + // LCOV_EXCL_START + JLOG(j_.error()) << "AMM Deposit: invalid options."; + return std::make_pair(tecINTERNAL, STAmount{}); + // LCOV_EXCL_STOP }(); if (result == tesSUCCESS) @@ -635,11 +629,12 @@ AMMDeposit::equalDepositTokens( } catch (std::exception const& e) { + // LCOV_EXCL_START JLOG(j_.error()) << "AMMDeposit::equalDepositTokens exception " << e.what(); + return {tecINTERNAL, STAmount{}}; + // LCOV_EXCL_STOP } - return { - tecINTERNAL, STAmount{}}; // LCOV_EXCL_LINE exclude from test coverage } /** Proportional deposit of pool assets with the constraints on the maximum From 8975ddf5df3a8defbeb7526a11459d9e13e40a63 Mon Sep 17 00:00:00 2001 From: John Freeman Date: Fri, 29 Mar 2024 09:00:30 -0500 Subject: [PATCH 09/24] Fix formatting --- src/test/app/AMM_test.cpp | 49 +++++++++++++++++++++------------------ src/test/jtx/AMM.h | 7 +++--- src/test/jtx/impl/AMM.cpp | 8 ++++--- 3 files changed, 36 insertions(+), 28 deletions(-) diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 086ed909651..4cfda969800 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -122,10 +122,12 @@ struct AMM_test : public jtx::AMMTest 1'000); // Make sure asset comparison works. - BEAST_EXPECT(STIssue(sfAsset, STAmount(XRP(2'000)).issue()) == - STIssue(sfAsset, STAmount(XRP(2'000)).issue())); - BEAST_EXPECT(STIssue(sfAsset, STAmount(XRP(2'000)).issue()) != - STIssue(sfAsset, STAmount({USD(2'000)}).issue())); + BEAST_EXPECT( + STIssue(sfAsset, STAmount(XRP(2'000)).issue()) == + STIssue(sfAsset, STAmount(XRP(2'000)).issue())); + BEAST_EXPECT( + STIssue(sfAsset, STAmount(XRP(2'000)).issue()) != + STIssue(sfAsset, STAmount({USD(2'000)}).issue())); } void @@ -2945,41 +2947,44 @@ struct AMM_test : public jtx::AMMTest Json::Value tx = amm.bidJson(alice, 500); { - auto jtx = env.jt( - tx, - seq(1), - fee(10)); + auto jtx = env.jt(tx, seq(1), fee(10)); env.app().config().features.erase(featureAMM); - PreflightContext pfctx(env.app(), *jtx.stx, - env.current()->rules(), tapNONE, env.journal); + PreflightContext pfctx( + env.app(), + *jtx.stx, + env.current()->rules(), + tapNONE, + env.journal); auto pf = AMMBid::preflight(pfctx); BEAST_EXPECT(pf == temDISABLED); env.app().config().features.insert(featureAMM); } { - auto jtx = env.jt( - tx, - seq(1), - fee(10)); + auto jtx = env.jt(tx, seq(1), fee(10)); jtx.jv["TxnSignature"] = "deadbeef"; jtx.stx = env.ust(jtx); - PreflightContext pfctx(env.app(), *jtx.stx, - env.current()->rules(), tapNONE, env.journal); + PreflightContext pfctx( + env.app(), + *jtx.stx, + env.current()->rules(), + tapNONE, + env.journal); auto pf = AMMBid::preflight(pfctx); BEAST_EXPECT(pf != tesSUCCESS); } { - auto jtx = env.jt( - tx, - seq(1), - fee(10)); + auto jtx = env.jt(tx, seq(1), fee(10)); jtx.jv["Asset2"]["currency"] = "XRP"; jtx.jv["Asset2"].removeMember("issuer"); jtx.stx = env.ust(jtx); - PreflightContext pfctx(env.app(), *jtx.stx, - env.current()->rules(), tapNONE, env.journal); + PreflightContext pfctx( + env.app(), + *jtx.stx, + env.current()->rules(), + tapNONE, + env.journal); auto pf = AMMBid::preflight(pfctx); BEAST_EXPECT(pf == temBAD_AMM_TOKENS); } diff --git a/src/test/jtx/AMM.h b/src/test/jtx/AMM.h index b1c82b54e83..e592206d0ed 100644 --- a/src/test/jtx/AMM.h +++ b/src/test/jtx/AMM.h @@ -319,11 +319,12 @@ class AMM // Return the JSON for a bid, but don't execute it. Json::Value - bidJson(std::optional const& account, + bidJson( + std::optional const& account, std::optional> const& bidMin = - std::nullopt, + std::nullopt, std::optional> const& bidMax = - std::nullopt, + std::nullopt, std::vector const& authAccounts = {}, std::optional const& flags = std::nullopt, std::optional const& seq = std::nullopt, diff --git a/src/test/jtx/impl/AMM.cpp b/src/test/jtx/impl/AMM.cpp index e606018090f..54521a87a2c 100644 --- a/src/test/jtx/impl/AMM.cpp +++ b/src/test/jtx/impl/AMM.cpp @@ -667,8 +667,10 @@ AMM::bid( std::optional> const& assets, std::optional const& ter) { - submit(bidJson(account, bidMin, bidMax, authAccounts, flags, seq, assets, - ter), seq, ter); + submit( + bidJson(account, bidMin, bidMax, authAccounts, flags, seq, assets, ter), + seq, + ter); } Json::Value @@ -683,7 +685,7 @@ AMM::bidJson( std::optional const& ter) { if (auto const amm = - env_.current()->read(keylet::amm(asset1_.issue(), asset2_.issue()))) + env_.current()->read(keylet::amm(asset1_.issue(), asset2_.issue()))) { assert( !env_.current()->rules().enabled(fixInnerObjTemplate) || From 7f151fa2ca69a64e3395fce4361efe78fb55e374 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Tue, 2 Apr 2024 20:23:30 +0100 Subject: [PATCH 10/24] Disable wandalen/wretry for codecov upload --- .github/workflows/nix.yml | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/.github/workflows/nix.yml b/.github/workflows/nix.yml index e213599fc14..4c6df5787f3 100644 --- a/.github/workflows/nix.yml +++ b/.github/workflows/nix.yml @@ -207,13 +207,11 @@ jobs: path: coverage.xml retention-days: 30 - name: upload coverage report - uses: wandalen/wretry.action@v1.3.0 - with: - action: codecov/codecov-action@v4 - with: | - files: coverage.xml - fail_ci_if_error: true - verbose: true - token: ${{ secrets.CODECOV_TOKEN }} - attempt_limit: 5 - attempt_delay: 35000 # in milliseconds + uses: codecov/codecov-action@v4.1.1 + with: | + file: coverage.xml + fail_ci_if_error: true + disable_search: true + verbose: true + os: linux + token: ${{ secrets.CODECOV_TOKEN }} From 87c8acd48948b3e175c678d0f5374c716904a4ef Mon Sep 17 00:00:00 2001 From: Chenna Keshava Date: Wed, 3 Apr 2024 10:55:09 -0700 Subject: [PATCH 11/24] AMMBid unit test: burn all LP tokens --- src/test/app/AMM_test.cpp | 98 ++++++++++++++++++++++++++++++--------- 1 file changed, 75 insertions(+), 23 deletions(-) diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 4cfda969800..0f127da6383 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -2410,6 +2410,58 @@ struct AMM_test : public jtx::AMMTest using namespace jtx; using namespace std::chrono; + // burn all the LPTokens through a AMMBid transaction + { + Env env(*this); + fund(env, gw, {alice}, XRP(2'000), {USD(2'000)}); + AMM amm(env, gw, XRP(1'000), USD(1'000), false, 1'000); + + // auction slot is owned by the creator of the AMM i.e. gw + BEAST_EXPECT(amm.expectAuctionSlot(100, 0, IOUAmount{0})); + + // gw attempts to burn all her LPTokens through a bid transaction + // this transaction fails because AMMBid transaction can not burn + // all the outstanding LPTokens + amm.bid(BidArg{ + .account = gw, + .bidMin = 1'000'000, + .err = ter(tecAMM_INVALID_TOKENS)}); + } + + // burn all the LPTokens through a AMMBid transaction + { + Env env(*this); + fund(env, gw, {alice}, XRP(2'000), {USD(2'000)}); + AMM amm(env, gw, XRP(1'000), USD(1'000), false, 1'000); + + // auction slot is owned by the creator of the AMM i.e. gw + BEAST_EXPECT(amm.expectAuctionSlot(100, 0, IOUAmount{0})); + + // gw burns all but one of her LPTokens through a bid transaction + // this transaction suceeds because the bid price is less than + // the total outstanding LPToken balance + amm.bid(BidArg{ + .account = gw, + .bidMin = STAmount{amm.lptIssue(), UINT64_C(999'999)}, + .err = ter(tesSUCCESS)}); + + // gw must posses the auction slot + BEAST_EXPECT(amm.expectAuctionSlot(100, 0, IOUAmount{999'999})); + + // 999'999 tokens are burned, only 1 LPToken is in vogue + BEAST_EXPECT( + amm.expectBalances(XRP(1'000), USD(1'000), IOUAmount{1})); + + // gw posses only one LPToken in her balance + BEAST_EXPECT(Number{amm.getLPTokensBalance(gw)} == 1); + + // gw attempts to burn the last of her LPTokens in an AMMBid + // transaction. This transaction fails because it would burn all + // the remaining LPTokens + amm.bid(BidArg{ + .account = gw, .bidMin = 1, .err = ter(tecAMM_INVALID_TOKENS)}); + } + testAMM([&](AMM& ammAlice, Env& env) { // Invalid flags ammAlice.bid( @@ -5088,30 +5140,30 @@ struct AMM_test : public jtx::AMMTest void testCore() { - testInvalidInstance(); - testInstanceCreate(); - testInvalidDeposit(); - testDeposit(); - testInvalidWithdraw(); - testWithdraw(); - testInvalidFeeVote(); - testFeeVote(); + // testInvalidInstance(); + // testInstanceCreate(); + // testInvalidDeposit(); + // testDeposit(); + // testInvalidWithdraw(); + // testWithdraw(); + // testInvalidFeeVote(); + // testFeeVote(); testInvalidBid(); - testBid(); - testInvalidAMMPayment(); - testBasicPaymentEngine(); - testAMMTokens(); - testAmendment(); - testFlags(); - testRippling(); - testAMMAndCLOB(); - testTradingFee(); - testAdjustedTokens(); - testAutoDelete(); - testClawback(); - testAMMID(); - testSelection(); - testFixDefaultInnerObj(); + // testBid(); + // testInvalidAMMPayment(); + // testBasicPaymentEngine(); + // testAMMTokens(); + // testAmendment(); + // testFlags(); + // testRippling(); + // testAMMAndCLOB(); + // testTradingFee(); + // testAdjustedTokens(); + // testAutoDelete(); + // testClawback(); + // testAMMID(); + // testSelection(); + // testFixDefaultInnerObj(); } void From 51eaed5c4f2fa0e6cd8f052ae67d6b749b827c1a Mon Sep 17 00:00:00 2001 From: John Freeman Date: Wed, 3 Apr 2024 18:16:07 -0500 Subject: [PATCH 12/24] Edit comments --- src/ripple/app/misc/impl/AMMUtils.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/ripple/app/misc/impl/AMMUtils.cpp b/src/ripple/app/misc/impl/AMMUtils.cpp index 138dd80dd1a..cf4da2c5d85 100644 --- a/src/ripple/app/misc/impl/AMMUtils.cpp +++ b/src/ripple/app/misc/impl/AMMUtils.cpp @@ -77,7 +77,7 @@ ammHolds( return std::make_optional(std::make_pair(issue1, issue2)); else if (checkIssue == issue2) return std::make_optional(std::make_pair(issue2, issue1)); - // This error can only be hit if the AMM is corrupted + // Unreachable unless AMM corrupted. // LCOV_EXCL_START JLOG(j.debug()) << "ammHolds: Invalid " << label << " " << checkIssue; @@ -90,8 +90,7 @@ ammHolds( } else if (optIssue2) { - // This line cannot be hit since you cannot have Amount2 - // without Amount + // Cannot have Amount2 without Amount. return singleIssue(*optIssue2, "optIssue2"); // LCOV_EXCL_LINE } return std::make_optional(std::make_pair(issue1, issue2)); @@ -342,11 +341,11 @@ initializeFeeAuctionVote( if (tfee != 0) ammSle->setFieldU16(sfTradingFee, tfee); else if (ammSle->isFieldPresent(sfTradingFee)) - ammSle->makeFieldAbsent(sfTradingFee); // LCOV_EXCL_LINE never hit + ammSle->makeFieldAbsent(sfTradingFee); // LCOV_EXCL_LINE if (auto const dfee = tfee / AUCTION_SLOT_DISCOUNTED_FEE_FRACTION) auctionSlot.setFieldU16(sfDiscountedFee, dfee); else if (auctionSlot.isFieldPresent(sfDiscountedFee)) - auctionSlot.makeFieldAbsent(sfDiscountedFee); // LCOV_EXCL_LINE not hit + auctionSlot.makeFieldAbsent(sfDiscountedFee); // LCOV_EXCL_LINE } } // namespace ripple From 61d007e99fe3c5320918090a91ab2b8a595da74e Mon Sep 17 00:00:00 2001 From: John Freeman Date: Thu, 4 Apr 2024 13:36:28 -0500 Subject: [PATCH 13/24] Consolidate disabled amendment tests --- src/test/app/AMMWithdraw_test.cpp | 14 -------------- src/test/app/AMM_test.cpp | 12 +++--------- 2 files changed, 3 insertions(+), 23 deletions(-) diff --git a/src/test/app/AMMWithdraw_test.cpp b/src/test/app/AMMWithdraw_test.cpp index 2c6d318acb6..6bb30d46b33 100644 --- a/src/test/app/AMMWithdraw_test.cpp +++ b/src/test/app/AMMWithdraw_test.cpp @@ -39,19 +39,6 @@ namespace test { struct AMMWithdraw_test : public jtx::AMMTest { - void - testDisabled() - { - using namespace jtx; - - // Each testAMM() call makes 27 tests. - testAMM([&](AMM& ammAlice, Env& env) { - env.disableFeature(featureAMM); - WithdrawArg args{.err = ter(temDISABLED)}; - ammAlice.withdraw(args); - }); - } - void testMalformed() { @@ -171,7 +158,6 @@ struct AMMWithdraw_test : public jtx::AMMTest void run() override { - testDisabled(); testMalformed(); testOther(); } diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 4cfda969800..2836b77f5a7 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -3876,15 +3876,9 @@ struct AMM_test : public jtx::AMMTest std::nullopt, std::nullopt, ter(temDISABLED)); - amm.withdraw( - alice, 100, std::nullopt, std::nullopt, ter(temMALFORMED)); - amm.deposit( - alice, - USD(100), - std::nullopt, - std::nullopt, - std::nullopt, - ter(temDISABLED)); + amm.withdraw({.tokens = 100, .err = ter(temMALFORMED)}); + amm.withdraw({.err = ter(temDISABLED)}); + amm.deposit({.asset1In = USD(100), .err = ter(temDISABLED)}); amm.ammDelete(alice, ter(temDISABLED)); } } From a504e86dc6bfa4734aa7af57a04b032e4ea92a1b Mon Sep 17 00:00:00 2001 From: John Freeman Date: Thu, 4 Apr 2024 14:01:00 -0500 Subject: [PATCH 14/24] Convert most bid tests to use BidArg --- src/test/app/AMMExtended_test.cpp | 2 +- src/test/app/AMM_test.cpp | 348 +++++++++++------------------- src/test/jtx/AMM.h | 23 +- src/test/jtx/impl/AMM.cpp | 39 ++-- src/test/rpc/AMMInfo_test.cpp | 2 +- 5 files changed, 161 insertions(+), 253 deletions(-) diff --git a/src/test/app/AMMExtended_test.cpp b/src/test/app/AMMExtended_test.cpp index 7b2f512543b..d27a36bab96 100644 --- a/src/test/app/AMMExtended_test.cpp +++ b/src/test/app/AMMExtended_test.cpp @@ -3644,7 +3644,7 @@ struct AMMExtended_test : public jtx::AMMTest ammAlice.vote({}, 1'000); BEAST_EXPECT(ammAlice.expectTradingFee(1'000)); - ammAlice.bid(alice, 100); + env(ammAlice.bid({ .account = alice, .bidMin = 100 }), msig(becky, bogie)); BEAST_EXPECT(ammAlice.expectAuctionSlot(100, 0, IOUAmount{4'000})); // 4000 tokens burnt BEAST_EXPECT(ammAlice.expectBalances( diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 2836b77f5a7..f8f7a5dc20d 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -2412,141 +2412,84 @@ struct AMM_test : public jtx::AMMTest testAMM([&](AMM& ammAlice, Env& env) { // Invalid flags - ammAlice.bid( - carol, - 0, - std::nullopt, - {}, - tfWithdrawAll, - std::nullopt, - std::nullopt, - ter(temINVALID_FLAG)); + env(ammAlice.bid({ + .account = carol, + .bidMin = 0, + .flags = tfWithdrawAll, + }), ter(temINVALID_FLAG)); ammAlice.deposit(carol, 1'000'000); // Invalid Bid price <= 0 for (auto bid : {0, -100}) { - ammAlice.bid( - carol, - bid, - std::nullopt, - {}, - std::nullopt, - std::nullopt, - std::nullopt, - ter(temBAD_AMOUNT)); - ammAlice.bid( - carol, - std::nullopt, - bid, - {}, - std::nullopt, - std::nullopt, - std::nullopt, - ter(temBAD_AMOUNT)); + env(ammAlice.bid({ + .account = carol, + .bidMin = bid, + }), ter(temBAD_AMOUNT)); + env(ammAlice.bid({ + .account = carol, + .bidMax = bid, + }), ter(temBAD_AMOUNT)); } // Invlaid Min/Max combination - ammAlice.bid( - carol, - 200, - 100, - {}, - std::nullopt, - std::nullopt, - std::nullopt, - ter(tecAMM_INVALID_TOKENS)); + env(ammAlice.bid({ + .account = carol, + .bidMin = 200, + .bidMax = 100, + }), ter(tecAMM_INVALID_TOKENS)); // Invalid Account Account bad("bad"); env.memoize(bad); - ammAlice.bid( - bad, - std::nullopt, - 100, - {}, - std::nullopt, - seq(1), - std::nullopt, - ter(terNO_ACCOUNT)); + env(ammAlice.bid({ + .account = bad, + .bidMax = 100, + }), seq(1), ter(terNO_ACCOUNT)); // Account is not LP Account const dan("dan"); env.fund(XRP(1'000), dan); - ammAlice.bid( - dan, - 100, - std::nullopt, - {}, - std::nullopt, - std::nullopt, - std::nullopt, - ter(tecAMM_INVALID_TOKENS)); - ammAlice.bid( - dan, - std::nullopt, - std::nullopt, - {}, - std::nullopt, - std::nullopt, - std::nullopt, - ter(tecAMM_INVALID_TOKENS)); + env(ammAlice.bid({ + .account = dan, + .bidMin = 100, + }), ter(tecAMM_INVALID_TOKENS)); + env(ammAlice.bid({ + .account = dan, + }), ter(tecAMM_INVALID_TOKENS)); // Auth account is invalid. - ammAlice.bid( - carol, - 100, - std::nullopt, - {bob}, - std::nullopt, - std::nullopt, - std::nullopt, - ter(terNO_ACCOUNT)); + env(ammAlice.bid({ + .account = carol, + .bidMin = 100, + .authAccounts = {bob}, + }), ter(terNO_ACCOUNT)); // Invalid Assets - ammAlice.bid( - alice, - std::nullopt, - 100, - {}, - std::nullopt, - std::nullopt, - {{USD, GBP}}, - ter(terNO_AMM)); + env(ammAlice.bid({ + .account = alice, + .bidMax = 100, + .assets = {{USD, GBP}}, + }), ter(terNO_AMM)); // Invalid Min/Max issue - ammAlice.bid( - alice, - std::nullopt, - STAmount{USD, 100}, - {}, - std::nullopt, - std::nullopt, - std::nullopt, - ter(temBAD_AMM_TOKENS)); - ammAlice.bid( - alice, - STAmount{USD, 100}, - std::nullopt, - {}, - std::nullopt, - std::nullopt, - std::nullopt, - ter(temBAD_AMM_TOKENS)); + env(ammAlice.bid({ + .account = alice, + .bidMax = STAmount{USD, 100}, + }), ter(temBAD_AMM_TOKENS)); + env(ammAlice.bid({ + .account = alice, + .bidMin = STAmount{USD, 100}, + }), ter(temBAD_AMM_TOKENS)); }); // Invalid AMM testAMM([&](AMM& ammAlice, Env& env) { ammAlice.withdrawAll(alice); - ammAlice.bid( - alice, - std::nullopt, - 100, - {}, - std::nullopt, - std::nullopt, - std::nullopt, - ter(terNO_AMM)); + env(ammAlice.bid({ + .account = alice, + .bidMax = 100, + }), ter(terNO_AMM)); }); // More than four Auth accounts. @@ -2558,15 +2501,11 @@ struct AMM_test : public jtx::AMMTest env.fund(XRP(1'000), bob, ed, bill, scott, james); env.close(); ammAlice.deposit(carol, 1'000'000); - ammAlice.bid( - carol, - 100, - std::nullopt, - {bob, ed, bill, scott, james}, - std::nullopt, - std::nullopt, - std::nullopt, - ter(temMALFORMED)); + env(ammAlice.bid({ + .account = carol, + .bidMin = 100, + .authAccounts = {bob, ed, bill, scott, james}, + }), ter(temMALFORMED)); }); // Bid price exceeds LP owned tokens @@ -2574,36 +2513,23 @@ struct AMM_test : public jtx::AMMTest fund(env, gw, {bob}, XRP(1'000), {USD(100)}, Fund::Acct); ammAlice.deposit(carol, 1'000'000); ammAlice.deposit(bob, 10); - ammAlice.bid( - carol, - 1'000'001, - std::nullopt, - {}, - std::nullopt, - std::nullopt, - std::nullopt, - ter(tecAMM_INVALID_TOKENS)); - ammAlice.bid( - carol, - std::nullopt, - 1'000'001, - {}, - std::nullopt, - std::nullopt, - std::nullopt, - ter(tecAMM_INVALID_TOKENS)); - ammAlice.bid(carol, 1'000); + env(ammAlice.bid({ + .account = carol, + .bidMin = 1'000'001, + }), ter(tecAMM_INVALID_TOKENS)); + env(ammAlice.bid({ + .account = carol, + .bidMax = 1'000'001, + }), ter(tecAMM_INVALID_TOKENS)); + env(ammAlice.bid({ + .account = carol, + .bidMin = 1'000, + })); BEAST_EXPECT(ammAlice.expectAuctionSlot(0, 0, IOUAmount{1'000})); // Slot purchase price is more than 1000 but bob only has 10 tokens - ammAlice.bid( - bob, - std::nullopt, - std::nullopt, - {}, - std::nullopt, - std::nullopt, - std::nullopt, - ter(tecAMM_INVALID_TOKENS)); + env(ammAlice.bid({ + .account = bob, + }), ter(tecAMM_INVALID_TOKENS)); }); // Bid all tokens, still own the slot @@ -2616,18 +2542,13 @@ struct AMM_test : public jtx::AMMTest env.trust(STAmount{lpIssue, 50}, bob); env(pay(gw, alice, STAmount{lpIssue, 100})); env(pay(gw, bob, STAmount{lpIssue, 50})); - amm.bid(alice, 100); + env(amm.bid({ .account = alice, .bidMin = 100 })); // Alice doesn't have any more tokens, but // she still owns the slot. - amm.bid( - bob, - std::nullopt, - 50, - {}, - std::nullopt, - std::nullopt, - std::nullopt, - ter(tecAMM_FAILED)); + env(amm.bid({ + .account = bob, + .bidMax = 50, + }), ter(tecAMM_FAILED)); } } @@ -2643,7 +2564,7 @@ struct AMM_test : public jtx::AMMTest // Bid 110 tokens. Pay bidMin. testAMM([&](AMM& ammAlice, Env& env) { ammAlice.deposit(carol, 1'000'000); - ammAlice.bid(carol, 110); + env(ammAlice.bid({ .account = carol, .bidMin = 110 })); BEAST_EXPECT(ammAlice.expectAuctionSlot(0, 0, IOUAmount{110})); // 110 tokens are burned. BEAST_EXPECT(ammAlice.expectBalances( @@ -2654,12 +2575,12 @@ struct AMM_test : public jtx::AMMTest testAMM([&](AMM& ammAlice, Env& env) { ammAlice.deposit(carol, 1'000'000); // Bid exactly 110. Pay 110 because the pay price is < 110. - ammAlice.bid(carol, 110, 110); + env(ammAlice.bid({ .account = carol, .bidMin = 110, .bidMax = 110 })); BEAST_EXPECT(ammAlice.expectAuctionSlot(0, 0, IOUAmount{110})); BEAST_EXPECT(ammAlice.expectBalances( XRP(11'000), USD(11'000), IOUAmount{10'999'890})); // Bid exactly 180-200. Pay 180 because the pay price is < 180. - ammAlice.bid(alice, 180, 200); + env(ammAlice.bid({ .account = alice, .bidMin = 180, .bidMax = 200 })); BEAST_EXPECT(ammAlice.expectAuctionSlot(0, 0, IOUAmount{180})); BEAST_EXPECT(ammAlice.expectBalances( XRP(11'000), USD(11'000), IOUAmount{10'999'814'5, -1})); @@ -2669,43 +2590,34 @@ struct AMM_test : public jtx::AMMTest testAMM([&](AMM& ammAlice, Env& env) { ammAlice.deposit(carol, 1'000'000); // Bid, pay bidMin. - ammAlice.bid(carol, 110); + env(ammAlice.bid({ .account = carol, .bidMin = 110 })); BEAST_EXPECT(ammAlice.expectAuctionSlot(0, 0, IOUAmount{110})); fund(env, gw, {bob}, {USD(10'000)}, Fund::Acct); ammAlice.deposit(bob, 1'000'000); // Bid, pay the computed price. - ammAlice.bid(bob); + env(ammAlice.bid({ .account = bob })); BEAST_EXPECT(ammAlice.expectAuctionSlot(0, 0, IOUAmount(1155, -1))); // Bid bidMax fails because the computed price is higher. - ammAlice.bid( - carol, - std::nullopt, - 120, - {}, - std::nullopt, - std::nullopt, - std::nullopt, - ter(tecAMM_FAILED)); + env(ammAlice.bid({ + .account = carol, + .bidMax = 120, + }), ter(tecAMM_FAILED)); // Bid MaxSlotPrice succeeds - pay computed price - ammAlice.bid(carol, std::nullopt, 600); + env(ammAlice.bid({ .account = carol, .bidMax = 600 })); BEAST_EXPECT( ammAlice.expectAuctionSlot(0, 0, IOUAmount{121'275, -3})); // Bid Min/MaxSlotPrice fails because the computed price is not in // range - ammAlice.bid( - carol, - 10, - 100, - {}, - std::nullopt, - std::nullopt, - std::nullopt, - ter(tecAMM_FAILED)); + env(ammAlice.bid({ + .account = carol, + .bidMin = 10, + .bidMax = 100, + }), ter(tecAMM_FAILED)); // Bid Min/MaxSlotPrice succeeds - pay computed price - ammAlice.bid(carol, 100, 600); + env(ammAlice.bid({ .account = carol, .bidMin = 100, .bidMax = 600 })); BEAST_EXPECT( ammAlice.expectAuctionSlot(0, 0, IOUAmount{127'33875, -5})); }); @@ -2766,7 +2678,11 @@ struct AMM_test : public jtx::AMMTest ammAlice.deposit(carol, 500'000); ammAlice.deposit(dan, 500'000); auto ammTokens = ammAlice.getLPTokensBalance(); - ammAlice.bid(carol, 120, std::nullopt, {bob, ed}); + env(ammAlice.bid({ + .account = carol, + .bidMin = 120, + .authAccounts = {bob, ed}, + })); auto const slotPrice = IOUAmount{5'200}; ammTokens -= slotPrice; BEAST_EXPECT(ammAlice.expectAuctionSlot(100, 0, slotPrice)); @@ -2876,10 +2792,10 @@ struct AMM_test : public jtx::AMMTest 1'000); // Bid tiny amount - testAMM([&](AMM& ammAlice, Env&) { + testAMM([&](AMM& ammAlice, Env& env) { // Bid a tiny amount auto const tiny = Number{STAmount::cMinValue, STAmount::cMinOffset}; - ammAlice.bid(alice, IOUAmount{tiny}); + env(ammAlice.bid({ .account = alice, .bidMin = IOUAmount{tiny} })); // Auction slot purchase price is equal to the tiny amount // since the minSlotPrice is 0 with no trading fee. BEAST_EXPECT(ammAlice.expectAuctionSlot(0, 0, IOUAmount{tiny})); @@ -2887,8 +2803,10 @@ struct AMM_test : public jtx::AMMTest BEAST_EXPECT(ammAlice.expectBalances( XRP(10'000), USD(10'000), ammAlice.tokens())); // Bid the tiny amount - ammAlice.bid( - alice, IOUAmount{STAmount::cMinValue, STAmount::cMinOffset}); + env(ammAlice.bid({ + .account = alice, + .bidMin = IOUAmount{STAmount::cMinValue, STAmount::cMinOffset}, + })); // Pay slightly higher price BEAST_EXPECT(ammAlice.expectAuctionSlot( 0, 0, IOUAmount{tiny * Number{105, -2}})); @@ -2899,14 +2817,22 @@ struct AMM_test : public jtx::AMMTest // Reset auth account testAMM([&](AMM& ammAlice, Env& env) { - ammAlice.bid(alice, IOUAmount{100}, std::nullopt, {carol}); + env(ammAlice.bid({ + .account = alice, + .bidMin = IOUAmount{100}, + .authAccounts = {carol}, + })); BEAST_EXPECT(ammAlice.expectAuctionSlot({carol})); - ammAlice.bid(alice, IOUAmount{100}); + env(ammAlice.bid({ .account = alice, .bidMin = IOUAmount{100} })); BEAST_EXPECT(ammAlice.expectAuctionSlot({})); Account bob("bob"); Account dan("dan"); fund(env, {bob, dan}, XRP(1'000)); - ammAlice.bid(alice, IOUAmount{100}, std::nullopt, {bob, dan}); + env(ammAlice.bid({ + .account = alice, + .bidMin = IOUAmount{100}, + .authAccounts = {bob, dan}, + })); BEAST_EXPECT(ammAlice.expectAuctionSlot({bob, dan})); }); @@ -2921,7 +2847,7 @@ struct AMM_test : public jtx::AMMTest env(pay(gw, alice, STAmount{lpIssue, 500})); env(pay(gw, bob, STAmount{lpIssue, 50})); // Alice doesn't have anymore lp tokens - amm.bid(alice, 500); + env(amm.bid({ .account = alice, .bidMin = 500 })); BEAST_EXPECT(amm.expectAuctionSlot(100, 0, IOUAmount{500})); BEAST_EXPECT(expectLine(env, alice, STAmount{lpIssue, 0})); // But trades with the discounted fee since she still owns the slot. @@ -2944,7 +2870,7 @@ struct AMM_test : public jtx::AMMTest Env env(*this); fund(env, gw, {alice, bob}, XRP(2'000), {USD(2'000)}); AMM amm(env, gw, XRP(1'000), USD(1'010), false, 1'000); - Json::Value tx = amm.bidJson(alice, 500); + Json::Value tx = amm.bid({ .account = alice, .bidMin = 500 }); { auto jtx = env.jt(tx, seq(1), fee(10)); @@ -3768,7 +3694,7 @@ struct AMM_test : public jtx::AMMTest ammAlice.vote(carol, 0); BEAST_EXPECT(ammAlice.expectTradingFee(0)); // Carol bids - ammAlice.bid(carol, 100); + env(ammAlice.bid({ .account = carol, .bidMin = 100 })); BEAST_EXPECT(ammAlice.expectLPTokens(carol, IOUAmount{4'999'900})); BEAST_EXPECT(ammAlice.expectAuctionSlot(0, 0, IOUAmount{100})); BEAST_EXPECT(accountBalance(env, carol) == "22499999960"); @@ -3860,25 +3786,14 @@ struct AMM_test : public jtx::AMMTest Env env{*this, feature}; fund(env, gw, {alice}, {USD(1'000)}, Fund::All); AMM amm(env, alice, XRP(1'000), USD(1'000), ter(temDISABLED)); - amm.bid( - alice, - 1000, - std::nullopt, - {}, - std::nullopt, - std::nullopt, - std::nullopt, - ter(temMALFORMED)); - amm.vote( - std::nullopt, - 100, - std::nullopt, - std::nullopt, - std::nullopt, - ter(temDISABLED)); - amm.withdraw({.tokens = 100, .err = ter(temMALFORMED)}); - amm.withdraw({.err = ter(temDISABLED)}); - amm.deposit({.asset1In = USD(100), .err = ter(temDISABLED)}); + + env(amm.bid({.bidMax = 1000 }), ter(temMALFORMED)); + env(amm.bid(BidArg{}), ter(temDISABLED)); + amm.vote(VoteArg{.tfee = 100, .err = ter(temDISABLED)}); + amm.withdraw(WithdrawArg{.tokens = 100, .err = ter(temMALFORMED)}); + amm.withdraw(WithdrawArg{.err = ter(temDISABLED)}); + amm.deposit( + DepositArg{.asset1In = USD(100), .err = ter(temDISABLED)}); amm.ammDelete(alice, ter(temDISABLED)); } } @@ -4499,15 +4414,10 @@ struct AMM_test : public jtx::AMMTest // Bid,Vote,Deposit,Withdraw,SetTrust failing with // tecAMM_EMPTY. Deposit succeeds with tfTwoAssetIfEmpty option. - amm.bid( - alice, - 1000, - std::nullopt, - {}, - std::nullopt, - std::nullopt, - std::nullopt, - ter(tecAMM_EMPTY)); + env(amm.bid({ + .account = alice, + .bidMin = 1000, + }), ter(tecAMM_EMPTY)); amm.vote( std::nullopt, 100, diff --git a/src/test/jtx/AMM.h b/src/test/jtx/AMM.h index e592206d0ed..2dea37135f5 100644 --- a/src/test/jtx/AMM.h +++ b/src/test/jtx/AMM.h @@ -106,6 +106,16 @@ struct VoteArg std::optional err = std::nullopt; }; +struct BidArg +{ + std::optional account = std::nullopt; + std::optional> bidMin = std::nullopt; + std::optional> bidMax = std::nullopt; + std::vector authAccounts = {}; + std::optional flags = std::nullopt; + std::optional> assets = std::nullopt; +}; + /** Convenience class to test AMM functionality. */ class AMM @@ -317,19 +327,8 @@ class AMM std::optional> const& assets = std::nullopt, std::optional const& ter = std::nullopt); - // Return the JSON for a bid, but don't execute it. Json::Value - bidJson( - std::optional const& account, - std::optional> const& bidMin = - std::nullopt, - std::optional> const& bidMax = - std::nullopt, - std::vector const& authAccounts = {}, - std::optional const& flags = std::nullopt, - std::optional const& seq = std::nullopt, - std::optional> const& assets = std::nullopt, - std::optional const& ter = std::nullopt); + bid(BidArg const& arg); AccountID const& ammAccount() const diff --git a/src/test/jtx/impl/AMM.cpp b/src/test/jtx/impl/AMM.cpp index 54521a87a2c..f06d4f9dedd 100644 --- a/src/test/jtx/impl/AMM.cpp +++ b/src/test/jtx/impl/AMM.cpp @@ -668,21 +668,20 @@ AMM::bid( std::optional const& ter) { submit( - bidJson(account, bidMin, bidMax, authAccounts, flags, seq, assets, ter), + bid({ + .account = account, + .bidMin = bidMin, + .bidMax = bidMax, + .authAccounts = authAccounts, + .flags = flags, + .assets = assets, + }), seq, ter); } Json::Value -AMM::bidJson( - std::optional const& account, - std::optional> const& bidMin, - std::optional> const& bidMax, - std::vector const& authAccounts, - std::optional const& flags, - std::optional const& seq, - std::optional> const& assets, - std::optional const& ter) +AMM::bid(BidArg const& arg) { if (auto const amm = env_.current()->read(keylet::amm(asset1_.issue(), asset2_.issue()))) @@ -701,8 +700,8 @@ AMM::bidJson( bidMax_ = std::nullopt; Json::Value jv; - jv[jss::Account] = account ? account->human() : creatorAccount_.human(); - setTokens(jv, assets); + jv[jss::Account] = arg.account ? arg.account->human() : creatorAccount_.human(); + setTokens(jv, arg.assets); auto getBid = [&](auto const& bid) { if (std::holds_alternative(bid)) return STAmount{lptIssue_, std::get(bid)}; @@ -711,22 +710,22 @@ AMM::bidJson( else return std::get(bid); }; - if (bidMin) + if (arg.bidMin) { - STAmount saTokens = getBid(*bidMin); + STAmount saTokens = getBid(*arg.bidMin); saTokens.setJson(jv[jss::BidMin]); bidMin_ = saTokens.iou(); } - if (bidMax) + if (arg.bidMax) { - STAmount saTokens = getBid(*bidMax); + STAmount saTokens = getBid(*arg.bidMax); saTokens.setJson(jv[jss::BidMax]); bidMax_ = saTokens.iou(); } - if (authAccounts.size() > 0) + if (arg.authAccounts.size() > 0) { Json::Value accounts(Json::arrayValue); - for (auto const& account : authAccounts) + for (auto const& account : arg.authAccounts) { Json::Value acct; Json::Value authAcct; @@ -736,8 +735,8 @@ AMM::bidJson( } jv[jss::AuthAccounts] = accounts; } - if (flags) - jv[jss::Flags] = *flags; + if (arg.flags) + jv[jss::Flags] = *arg.flags; jv[jss::TransactionType] = jss::AMMBid; if (fee_ != 0) jv[jss::Fee] = std::to_string(fee_); diff --git a/src/test/rpc/AMMInfo_test.cpp b/src/test/rpc/AMMInfo_test.cpp index 1d9642539a1..7b688af3bd4 100644 --- a/src/test/rpc/AMMInfo_test.cpp +++ b/src/test/rpc/AMMInfo_test.cpp @@ -130,7 +130,7 @@ class AMMInfo_test : public jtx::AMMTestBase Account ed("ed"); Account bill("bill"); env.fund(XRP(1000), bob, ed, bill); - ammAlice.bid(alice, 100, std::nullopt, {carol, bob, ed, bill}); + env(ammAlice.bid({ .bidMin = 100, .authAccounts = {carol, bob, ed, bill} })); BEAST_EXPECT(ammAlice.expectAmmRpcInfo( XRP(80000), USD(80000), From 43a251c3f4b8d98623a0f723ef806180915a39ed Mon Sep 17 00:00:00 2001 From: John Freeman Date: Thu, 4 Apr 2024 17:19:07 -0500 Subject: [PATCH 15/24] Try to convert last bid tests --- src/test/app/AMM_test.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index f8f7a5dc20d..8ed2f9b082e 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -2632,23 +2632,23 @@ struct AMM_test : public jtx::AMMTest XRP(12'000), USD(12'000), IOUAmount{12'000'000, 0})); // Initial state. Pay bidMin. - ammAlice.bid(carol, 110); + env(ammAlice.bid({ .account = carol, .bidMin = 110 })); BEAST_EXPECT(ammAlice.expectAuctionSlot(0, 0, IOUAmount{110})); // 1st Interval after close, price for 0th interval. - ammAlice.bid(bob); + env(ammAlice.bid({ .account = bob })); env.close(seconds(AUCTION_SLOT_INTERVAL_DURATION + 1)); BEAST_EXPECT( ammAlice.expectAuctionSlot(0, 1, IOUAmount{1'155, -1})); // 10th Interval after close, price for 1st interval. - ammAlice.bid(carol); + env(ammAlice.bid({ .account = carol })); env.close(seconds(10 * AUCTION_SLOT_INTERVAL_DURATION + 1)); BEAST_EXPECT( ammAlice.expectAuctionSlot(0, 10, IOUAmount{121'275, -3})); // 20th Interval (expired) after close, price for 10th interval. - ammAlice.bid(bob); + env(ammAlice.bid({ .account = bob })); env.close(seconds( AUCTION_SLOT_TIME_INTERVALS * AUCTION_SLOT_INTERVAL_DURATION + 1)); @@ -2656,7 +2656,7 @@ struct AMM_test : public jtx::AMMTest 0, std::nullopt, IOUAmount{127'33875, -5})); // 0 Interval. - ammAlice.bid(carol, 110); + env(ammAlice.bid({ .account = carol, .bidMin = 110 })); BEAST_EXPECT( ammAlice.expectAuctionSlot(0, std::nullopt, IOUAmount{110})); // ~321.09 tokens burnt on bidding fees. From 2088454db53948b734171e310de2b7968f97050d Mon Sep 17 00:00:00 2001 From: John Freeman Date: Thu, 4 Apr 2024 17:22:46 -0500 Subject: [PATCH 16/24] Format code --- src/test/app/AMMExtended_test.cpp | 3 +- src/test/app/AMM_test.cpp | 187 +++++++++++++++++------------- src/test/jtx/impl/AMM.cpp | 3 +- src/test/rpc/AMMInfo_test.cpp | 3 +- 4 files changed, 110 insertions(+), 86 deletions(-) diff --git a/src/test/app/AMMExtended_test.cpp b/src/test/app/AMMExtended_test.cpp index d27a36bab96..d462f68bb4b 100644 --- a/src/test/app/AMMExtended_test.cpp +++ b/src/test/app/AMMExtended_test.cpp @@ -3644,7 +3644,8 @@ struct AMMExtended_test : public jtx::AMMTest ammAlice.vote({}, 1'000); BEAST_EXPECT(ammAlice.expectTradingFee(1'000)); - env(ammAlice.bid({ .account = alice, .bidMin = 100 }), msig(becky, bogie)); + env(ammAlice.bid({.account = alice, .bidMin = 100}), + msig(becky, bogie)); BEAST_EXPECT(ammAlice.expectAuctionSlot(100, 0, IOUAmount{4'000})); // 4000 tokens burnt BEAST_EXPECT(ammAlice.expectBalances( diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 8ed2f9b082e..f0142968853 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -2413,83 +2413,96 @@ struct AMM_test : public jtx::AMMTest testAMM([&](AMM& ammAlice, Env& env) { // Invalid flags env(ammAlice.bid({ - .account = carol, - .bidMin = 0, - .flags = tfWithdrawAll, - }), ter(temINVALID_FLAG)); + .account = carol, + .bidMin = 0, + .flags = tfWithdrawAll, + }), + ter(temINVALID_FLAG)); ammAlice.deposit(carol, 1'000'000); // Invalid Bid price <= 0 for (auto bid : {0, -100}) { env(ammAlice.bid({ - .account = carol, - .bidMin = bid, - }), ter(temBAD_AMOUNT)); + .account = carol, + .bidMin = bid, + }), + ter(temBAD_AMOUNT)); env(ammAlice.bid({ - .account = carol, - .bidMax = bid, - }), ter(temBAD_AMOUNT)); + .account = carol, + .bidMax = bid, + }), + ter(temBAD_AMOUNT)); } // Invlaid Min/Max combination env(ammAlice.bid({ - .account = carol, - .bidMin = 200, - .bidMax = 100, - }), ter(tecAMM_INVALID_TOKENS)); + .account = carol, + .bidMin = 200, + .bidMax = 100, + }), + ter(tecAMM_INVALID_TOKENS)); // Invalid Account Account bad("bad"); env.memoize(bad); env(ammAlice.bid({ - .account = bad, - .bidMax = 100, - }), seq(1), ter(terNO_ACCOUNT)); + .account = bad, + .bidMax = 100, + }), + seq(1), + ter(terNO_ACCOUNT)); // Account is not LP Account const dan("dan"); env.fund(XRP(1'000), dan); env(ammAlice.bid({ - .account = dan, - .bidMin = 100, - }), ter(tecAMM_INVALID_TOKENS)); + .account = dan, + .bidMin = 100, + }), + ter(tecAMM_INVALID_TOKENS)); env(ammAlice.bid({ - .account = dan, - }), ter(tecAMM_INVALID_TOKENS)); + .account = dan, + }), + ter(tecAMM_INVALID_TOKENS)); // Auth account is invalid. env(ammAlice.bid({ - .account = carol, - .bidMin = 100, - .authAccounts = {bob}, - }), ter(terNO_ACCOUNT)); + .account = carol, + .bidMin = 100, + .authAccounts = {bob}, + }), + ter(terNO_ACCOUNT)); // Invalid Assets env(ammAlice.bid({ - .account = alice, - .bidMax = 100, - .assets = {{USD, GBP}}, - }), ter(terNO_AMM)); + .account = alice, + .bidMax = 100, + .assets = {{USD, GBP}}, + }), + ter(terNO_AMM)); // Invalid Min/Max issue env(ammAlice.bid({ - .account = alice, - .bidMax = STAmount{USD, 100}, - }), ter(temBAD_AMM_TOKENS)); + .account = alice, + .bidMax = STAmount{USD, 100}, + }), + ter(temBAD_AMM_TOKENS)); env(ammAlice.bid({ - .account = alice, - .bidMin = STAmount{USD, 100}, - }), ter(temBAD_AMM_TOKENS)); + .account = alice, + .bidMin = STAmount{USD, 100}, + }), + ter(temBAD_AMM_TOKENS)); }); // Invalid AMM testAMM([&](AMM& ammAlice, Env& env) { ammAlice.withdrawAll(alice); env(ammAlice.bid({ - .account = alice, - .bidMax = 100, - }), ter(terNO_AMM)); + .account = alice, + .bidMax = 100, + }), + ter(terNO_AMM)); }); // More than four Auth accounts. @@ -2502,10 +2515,11 @@ struct AMM_test : public jtx::AMMTest env.close(); ammAlice.deposit(carol, 1'000'000); env(ammAlice.bid({ - .account = carol, - .bidMin = 100, - .authAccounts = {bob, ed, bill, scott, james}, - }), ter(temMALFORMED)); + .account = carol, + .bidMin = 100, + .authAccounts = {bob, ed, bill, scott, james}, + }), + ter(temMALFORMED)); }); // Bid price exceeds LP owned tokens @@ -2514,13 +2528,15 @@ struct AMM_test : public jtx::AMMTest ammAlice.deposit(carol, 1'000'000); ammAlice.deposit(bob, 10); env(ammAlice.bid({ - .account = carol, - .bidMin = 1'000'001, - }), ter(tecAMM_INVALID_TOKENS)); + .account = carol, + .bidMin = 1'000'001, + }), + ter(tecAMM_INVALID_TOKENS)); env(ammAlice.bid({ - .account = carol, - .bidMax = 1'000'001, - }), ter(tecAMM_INVALID_TOKENS)); + .account = carol, + .bidMax = 1'000'001, + }), + ter(tecAMM_INVALID_TOKENS)); env(ammAlice.bid({ .account = carol, .bidMin = 1'000, @@ -2528,8 +2544,9 @@ struct AMM_test : public jtx::AMMTest BEAST_EXPECT(ammAlice.expectAuctionSlot(0, 0, IOUAmount{1'000})); // Slot purchase price is more than 1000 but bob only has 10 tokens env(ammAlice.bid({ - .account = bob, - }), ter(tecAMM_INVALID_TOKENS)); + .account = bob, + }), + ter(tecAMM_INVALID_TOKENS)); }); // Bid all tokens, still own the slot @@ -2542,13 +2559,14 @@ struct AMM_test : public jtx::AMMTest env.trust(STAmount{lpIssue, 50}, bob); env(pay(gw, alice, STAmount{lpIssue, 100})); env(pay(gw, bob, STAmount{lpIssue, 50})); - env(amm.bid({ .account = alice, .bidMin = 100 })); + env(amm.bid({.account = alice, .bidMin = 100})); // Alice doesn't have any more tokens, but // she still owns the slot. env(amm.bid({ - .account = bob, - .bidMax = 50, - }), ter(tecAMM_FAILED)); + .account = bob, + .bidMax = 50, + }), + ter(tecAMM_FAILED)); } } @@ -2564,7 +2582,7 @@ struct AMM_test : public jtx::AMMTest // Bid 110 tokens. Pay bidMin. testAMM([&](AMM& ammAlice, Env& env) { ammAlice.deposit(carol, 1'000'000); - env(ammAlice.bid({ .account = carol, .bidMin = 110 })); + env(ammAlice.bid({.account = carol, .bidMin = 110})); BEAST_EXPECT(ammAlice.expectAuctionSlot(0, 0, IOUAmount{110})); // 110 tokens are burned. BEAST_EXPECT(ammAlice.expectBalances( @@ -2575,12 +2593,12 @@ struct AMM_test : public jtx::AMMTest testAMM([&](AMM& ammAlice, Env& env) { ammAlice.deposit(carol, 1'000'000); // Bid exactly 110. Pay 110 because the pay price is < 110. - env(ammAlice.bid({ .account = carol, .bidMin = 110, .bidMax = 110 })); + env(ammAlice.bid({.account = carol, .bidMin = 110, .bidMax = 110})); BEAST_EXPECT(ammAlice.expectAuctionSlot(0, 0, IOUAmount{110})); BEAST_EXPECT(ammAlice.expectBalances( XRP(11'000), USD(11'000), IOUAmount{10'999'890})); // Bid exactly 180-200. Pay 180 because the pay price is < 180. - env(ammAlice.bid({ .account = alice, .bidMin = 180, .bidMax = 200 })); + env(ammAlice.bid({.account = alice, .bidMin = 180, .bidMax = 200})); BEAST_EXPECT(ammAlice.expectAuctionSlot(0, 0, IOUAmount{180})); BEAST_EXPECT(ammAlice.expectBalances( XRP(11'000), USD(11'000), IOUAmount{10'999'814'5, -1})); @@ -2590,34 +2608,36 @@ struct AMM_test : public jtx::AMMTest testAMM([&](AMM& ammAlice, Env& env) { ammAlice.deposit(carol, 1'000'000); // Bid, pay bidMin. - env(ammAlice.bid({ .account = carol, .bidMin = 110 })); + env(ammAlice.bid({.account = carol, .bidMin = 110})); BEAST_EXPECT(ammAlice.expectAuctionSlot(0, 0, IOUAmount{110})); fund(env, gw, {bob}, {USD(10'000)}, Fund::Acct); ammAlice.deposit(bob, 1'000'000); // Bid, pay the computed price. - env(ammAlice.bid({ .account = bob })); + env(ammAlice.bid({.account = bob})); BEAST_EXPECT(ammAlice.expectAuctionSlot(0, 0, IOUAmount(1155, -1))); // Bid bidMax fails because the computed price is higher. env(ammAlice.bid({ - .account = carol, - .bidMax = 120, - }), ter(tecAMM_FAILED)); + .account = carol, + .bidMax = 120, + }), + ter(tecAMM_FAILED)); // Bid MaxSlotPrice succeeds - pay computed price - env(ammAlice.bid({ .account = carol, .bidMax = 600 })); + env(ammAlice.bid({.account = carol, .bidMax = 600})); BEAST_EXPECT( ammAlice.expectAuctionSlot(0, 0, IOUAmount{121'275, -3})); // Bid Min/MaxSlotPrice fails because the computed price is not in // range env(ammAlice.bid({ - .account = carol, - .bidMin = 10, - .bidMax = 100, - }), ter(tecAMM_FAILED)); + .account = carol, + .bidMin = 10, + .bidMax = 100, + }), + ter(tecAMM_FAILED)); // Bid Min/MaxSlotPrice succeeds - pay computed price - env(ammAlice.bid({ .account = carol, .bidMin = 100, .bidMax = 600 })); + env(ammAlice.bid({.account = carol, .bidMin = 100, .bidMax = 600})); BEAST_EXPECT( ammAlice.expectAuctionSlot(0, 0, IOUAmount{127'33875, -5})); }); @@ -2632,23 +2652,23 @@ struct AMM_test : public jtx::AMMTest XRP(12'000), USD(12'000), IOUAmount{12'000'000, 0})); // Initial state. Pay bidMin. - env(ammAlice.bid({ .account = carol, .bidMin = 110 })); + env(ammAlice.bid({.account = carol, .bidMin = 110})); BEAST_EXPECT(ammAlice.expectAuctionSlot(0, 0, IOUAmount{110})); // 1st Interval after close, price for 0th interval. - env(ammAlice.bid({ .account = bob })); + env(ammAlice.bid({.account = bob})); env.close(seconds(AUCTION_SLOT_INTERVAL_DURATION + 1)); BEAST_EXPECT( ammAlice.expectAuctionSlot(0, 1, IOUAmount{1'155, -1})); // 10th Interval after close, price for 1st interval. - env(ammAlice.bid({ .account = carol })); + env(ammAlice.bid({.account = carol})); env.close(seconds(10 * AUCTION_SLOT_INTERVAL_DURATION + 1)); BEAST_EXPECT( ammAlice.expectAuctionSlot(0, 10, IOUAmount{121'275, -3})); // 20th Interval (expired) after close, price for 10th interval. - env(ammAlice.bid({ .account = bob })); + env(ammAlice.bid({.account = bob})); env.close(seconds( AUCTION_SLOT_TIME_INTERVALS * AUCTION_SLOT_INTERVAL_DURATION + 1)); @@ -2656,7 +2676,7 @@ struct AMM_test : public jtx::AMMTest 0, std::nullopt, IOUAmount{127'33875, -5})); // 0 Interval. - env(ammAlice.bid({ .account = carol, .bidMin = 110 })); + env(ammAlice.bid({.account = carol, .bidMin = 110})); BEAST_EXPECT( ammAlice.expectAuctionSlot(0, std::nullopt, IOUAmount{110})); // ~321.09 tokens burnt on bidding fees. @@ -2795,7 +2815,7 @@ struct AMM_test : public jtx::AMMTest testAMM([&](AMM& ammAlice, Env& env) { // Bid a tiny amount auto const tiny = Number{STAmount::cMinValue, STAmount::cMinOffset}; - env(ammAlice.bid({ .account = alice, .bidMin = IOUAmount{tiny} })); + env(ammAlice.bid({.account = alice, .bidMin = IOUAmount{tiny}})); // Auction slot purchase price is equal to the tiny amount // since the minSlotPrice is 0 with no trading fee. BEAST_EXPECT(ammAlice.expectAuctionSlot(0, 0, IOUAmount{tiny})); @@ -2823,7 +2843,7 @@ struct AMM_test : public jtx::AMMTest .authAccounts = {carol}, })); BEAST_EXPECT(ammAlice.expectAuctionSlot({carol})); - env(ammAlice.bid({ .account = alice, .bidMin = IOUAmount{100} })); + env(ammAlice.bid({.account = alice, .bidMin = IOUAmount{100}})); BEAST_EXPECT(ammAlice.expectAuctionSlot({})); Account bob("bob"); Account dan("dan"); @@ -2847,7 +2867,7 @@ struct AMM_test : public jtx::AMMTest env(pay(gw, alice, STAmount{lpIssue, 500})); env(pay(gw, bob, STAmount{lpIssue, 50})); // Alice doesn't have anymore lp tokens - env(amm.bid({ .account = alice, .bidMin = 500 })); + env(amm.bid({.account = alice, .bidMin = 500})); BEAST_EXPECT(amm.expectAuctionSlot(100, 0, IOUAmount{500})); BEAST_EXPECT(expectLine(env, alice, STAmount{lpIssue, 0})); // But trades with the discounted fee since she still owns the slot. @@ -2870,7 +2890,7 @@ struct AMM_test : public jtx::AMMTest Env env(*this); fund(env, gw, {alice, bob}, XRP(2'000), {USD(2'000)}); AMM amm(env, gw, XRP(1'000), USD(1'010), false, 1'000); - Json::Value tx = amm.bid({ .account = alice, .bidMin = 500 }); + Json::Value tx = amm.bid({.account = alice, .bidMin = 500}); { auto jtx = env.jt(tx, seq(1), fee(10)); @@ -3694,7 +3714,7 @@ struct AMM_test : public jtx::AMMTest ammAlice.vote(carol, 0); BEAST_EXPECT(ammAlice.expectTradingFee(0)); // Carol bids - env(ammAlice.bid({ .account = carol, .bidMin = 100 })); + env(ammAlice.bid({.account = carol, .bidMin = 100})); BEAST_EXPECT(ammAlice.expectLPTokens(carol, IOUAmount{4'999'900})); BEAST_EXPECT(ammAlice.expectAuctionSlot(0, 0, IOUAmount{100})); BEAST_EXPECT(accountBalance(env, carol) == "22499999960"); @@ -3787,7 +3807,7 @@ struct AMM_test : public jtx::AMMTest fund(env, gw, {alice}, {USD(1'000)}, Fund::All); AMM amm(env, alice, XRP(1'000), USD(1'000), ter(temDISABLED)); - env(amm.bid({.bidMax = 1000 }), ter(temMALFORMED)); + env(amm.bid({.bidMax = 1000}), ter(temMALFORMED)); env(amm.bid(BidArg{}), ter(temDISABLED)); amm.vote(VoteArg{.tfee = 100, .err = ter(temDISABLED)}); amm.withdraw(WithdrawArg{.tokens = 100, .err = ter(temMALFORMED)}); @@ -4415,9 +4435,10 @@ struct AMM_test : public jtx::AMMTest // Bid,Vote,Deposit,Withdraw,SetTrust failing with // tecAMM_EMPTY. Deposit succeeds with tfTwoAssetIfEmpty option. env(amm.bid({ - .account = alice, - .bidMin = 1000, - }), ter(tecAMM_EMPTY)); + .account = alice, + .bidMin = 1000, + }), + ter(tecAMM_EMPTY)); amm.vote( std::nullopt, 100, diff --git a/src/test/jtx/impl/AMM.cpp b/src/test/jtx/impl/AMM.cpp index f06d4f9dedd..71b271b7bbc 100644 --- a/src/test/jtx/impl/AMM.cpp +++ b/src/test/jtx/impl/AMM.cpp @@ -700,7 +700,8 @@ AMM::bid(BidArg const& arg) bidMax_ = std::nullopt; Json::Value jv; - jv[jss::Account] = arg.account ? arg.account->human() : creatorAccount_.human(); + jv[jss::Account] = + arg.account ? arg.account->human() : creatorAccount_.human(); setTokens(jv, arg.assets); auto getBid = [&](auto const& bid) { if (std::holds_alternative(bid)) diff --git a/src/test/rpc/AMMInfo_test.cpp b/src/test/rpc/AMMInfo_test.cpp index 7b688af3bd4..cfcb672c032 100644 --- a/src/test/rpc/AMMInfo_test.cpp +++ b/src/test/rpc/AMMInfo_test.cpp @@ -130,7 +130,8 @@ class AMMInfo_test : public jtx::AMMTestBase Account ed("ed"); Account bill("bill"); env.fund(XRP(1000), bob, ed, bill); - env(ammAlice.bid({ .bidMin = 100, .authAccounts = {carol, bob, ed, bill} })); + env(ammAlice.bid( + {.bidMin = 100, .authAccounts = {carol, bob, ed, bill}})); BEAST_EXPECT(ammAlice.expectAmmRpcInfo( XRP(80000), USD(80000), From 242b7ecc7ba3d8160ac073c3e43317b8dd72d4fe Mon Sep 17 00:00:00 2001 From: John Freeman Date: Fri, 5 Apr 2024 12:08:02 -0500 Subject: [PATCH 17/24] Finish bid tests --- src/test/app/AMMExtended_test.cpp | 7 ++++--- src/test/app/AMM_test.cpp | 4 ++-- src/test/jtx/Env.h | 7 ++++--- src/test/jtx/multisign.h | 5 ++++- 4 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/test/app/AMMExtended_test.cpp b/src/test/app/AMMExtended_test.cpp index d462f68bb4b..5ab46e94784 100644 --- a/src/test/app/AMMExtended_test.cpp +++ b/src/test/app/AMMExtended_test.cpp @@ -3617,6 +3617,8 @@ struct AMMExtended_test : public jtx::AMMTest int const signerListOwners{features[featureMultiSignReserve] ? 2 : 5}; env.require(owners(alice, signerListOwners + 0)); + const msig ms{becky, bogie}; + // Multisign all AMM transactions AMM ammAlice( env, @@ -3628,7 +3630,7 @@ struct AMMExtended_test : public jtx::AMMTest ammCrtFee(env).drops(), std::nullopt, std::nullopt, - msig(becky, bogie), + ms, ter(tesSUCCESS)); BEAST_EXPECT(ammAlice.expectBalances( XRP(10'000), USD(10'000), ammAlice.tokens())); @@ -3644,8 +3646,7 @@ struct AMMExtended_test : public jtx::AMMTest ammAlice.vote({}, 1'000); BEAST_EXPECT(ammAlice.expectTradingFee(1'000)); - env(ammAlice.bid({.account = alice, .bidMin = 100}), - msig(becky, bogie)); + env(ammAlice.bid({.account = alice, .bidMin = 100}), ms).close(); BEAST_EXPECT(ammAlice.expectAuctionSlot(100, 0, IOUAmount{4'000})); // 4000 tokens burnt BEAST_EXPECT(ammAlice.expectBalances( diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index f0142968853..8f113696be1 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -2652,7 +2652,7 @@ struct AMM_test : public jtx::AMMTest XRP(12'000), USD(12'000), IOUAmount{12'000'000, 0})); // Initial state. Pay bidMin. - env(ammAlice.bid({.account = carol, .bidMin = 110})); + env(ammAlice.bid({.account = carol, .bidMin = 110})).close(); BEAST_EXPECT(ammAlice.expectAuctionSlot(0, 0, IOUAmount{110})); // 1st Interval after close, price for 0th interval. @@ -2676,7 +2676,7 @@ struct AMM_test : public jtx::AMMTest 0, std::nullopt, IOUAmount{127'33875, -5})); // 0 Interval. - env(ammAlice.bid({.account = carol, .bidMin = 110})); + env(ammAlice.bid({.account = carol, .bidMin = 110})).close(); BEAST_EXPECT( ammAlice.expectAuctionSlot(0, std::nullopt, IOUAmount{110})); // ~321.09 tokens burnt on bidding fees. diff --git a/src/test/jtx/Env.h b/src/test/jtx/Env.h index 595cf1a1bec..69640179b11 100644 --- a/src/test/jtx/Env.h +++ b/src/test/jtx/Env.h @@ -521,17 +521,18 @@ class Env /** Apply funclets and submit. */ /** @{ */ template - void + Env& apply(JsonValue&& jv, FN const&... fN) { submit(jt(std::forward(jv), fN...)); + return *this; } template - void + Env& operator()(JsonValue&& jv, FN const&... fN) { - apply(std::forward(jv), fN...); + return apply(std::forward(jv), fN...); } /** @} */ diff --git a/src/test/jtx/multisign.h b/src/test/jtx/multisign.h index ab9996e415d..0617ea2fcbb 100644 --- a/src/test/jtx/multisign.h +++ b/src/test/jtx/multisign.h @@ -20,6 +20,7 @@ #ifndef RIPPLE_TEST_JTX_MULTISIGN_H_INCLUDED #define RIPPLE_TEST_JTX_MULTISIGN_H_INCLUDED +#include #include #include #include @@ -99,7 +100,9 @@ class msig msig(std::vector signers_); template - explicit msig(AccountType&& a0, Accounts&&... aN) + requires std::convertible_to explicit msig( + AccountType&& a0, + Accounts&&... aN) : msig{std::vector{ std::forward(a0), std::forward(aN)...}} From 1fa15813de4ab048caa87051d94f1541cbd387b2 Mon Sep 17 00:00:00 2001 From: John Freeman Date: Fri, 5 Apr 2024 13:36:24 -0500 Subject: [PATCH 18/24] Merge burn tests and eliminate non-BidArg overload --- src/test/app/AMM_test.cpp | 15 ++++++++------- src/test/jtx/AMM.h | 12 ------------ src/test/jtx/impl/AMM.cpp | 24 ------------------------ 3 files changed, 8 insertions(+), 43 deletions(-) diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 88a020ac526..3fe97ea360d 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -2422,10 +2422,10 @@ struct AMM_test : public jtx::AMMTest // gw attempts to burn all her LPTokens through a bid transaction // this transaction fails because AMMBid transaction can not burn // all the outstanding LPTokens - amm.bid(BidArg{ + env(amm.bid({ .account = gw, .bidMin = 1'000'000, - .err = ter(tecAMM_INVALID_TOKENS)}); + }), ter(tecAMM_INVALID_TOKENS)); } // burn all the LPTokens through a AMMBid transaction @@ -2440,10 +2440,10 @@ struct AMM_test : public jtx::AMMTest // gw burns all but one of her LPTokens through a bid transaction // this transaction suceeds because the bid price is less than // the total outstanding LPToken balance - amm.bid(BidArg{ + env(amm.bid({ .account = gw, .bidMin = STAmount{amm.lptIssue(), UINT64_C(999'999)}, - .err = ter(tesSUCCESS)}); + }), ter(tesSUCCESS)).close(); // gw must posses the auction slot BEAST_EXPECT(amm.expectAuctionSlot(100, 0, IOUAmount{999'999})); @@ -2458,8 +2458,9 @@ struct AMM_test : public jtx::AMMTest // gw attempts to burn the last of her LPTokens in an AMMBid // transaction. This transaction fails because it would burn all // the remaining LPTokens - amm.bid(BidArg{ - .account = gw, .bidMin = 1, .err = ter(tecAMM_INVALID_TOKENS)}); + env(amm.bid({ + .account = gw, .bidMin = 1, + }), ter(tecAMM_INVALID_TOKENS)); } testAMM([&](AMM& ammAlice, Env& env) { @@ -3860,7 +3861,7 @@ struct AMM_test : public jtx::AMMTest AMM amm(env, alice, XRP(1'000), USD(1'000), ter(temDISABLED)); env(amm.bid({.bidMax = 1000}), ter(temMALFORMED)); - env(amm.bid(BidArg{}), ter(temDISABLED)); + env(amm.bid({}), ter(temDISABLED)); amm.vote(VoteArg{.tfee = 100, .err = ter(temDISABLED)}); amm.withdraw(WithdrawArg{.tokens = 100, .err = ter(temMALFORMED)}); amm.withdraw(WithdrawArg{.err = ter(temDISABLED)}); diff --git a/src/test/jtx/AMM.h b/src/test/jtx/AMM.h index 2dea37135f5..b98065a077e 100644 --- a/src/test/jtx/AMM.h +++ b/src/test/jtx/AMM.h @@ -315,18 +315,6 @@ class AMM void vote(VoteArg const& arg); - void - bid(std::optional const& account, - std::optional> const& bidMin = - std::nullopt, - std::optional> const& bidMax = - std::nullopt, - std::vector const& authAccounts = {}, - std::optional const& flags = std::nullopt, - std::optional const& seq = std::nullopt, - std::optional> const& assets = std::nullopt, - std::optional const& ter = std::nullopt); - Json::Value bid(BidArg const& arg); diff --git a/src/test/jtx/impl/AMM.cpp b/src/test/jtx/impl/AMM.cpp index 71b271b7bbc..eddb9691693 100644 --- a/src/test/jtx/impl/AMM.cpp +++ b/src/test/jtx/impl/AMM.cpp @@ -656,30 +656,6 @@ AMM::vote(VoteArg const& arg) return vote(arg.account, arg.tfee, arg.flags, arg.seq, arg.assets, arg.err); } -void -AMM::bid( - std::optional const& account, - std::optional> const& bidMin, - std::optional> const& bidMax, - std::vector const& authAccounts, - std::optional const& flags, - std::optional const& seq, - std::optional> const& assets, - std::optional const& ter) -{ - submit( - bid({ - .account = account, - .bidMin = bidMin, - .bidMax = bidMax, - .authAccounts = authAccounts, - .flags = flags, - .assets = assets, - }), - seq, - ter); -} - Json::Value AMM::bid(BidArg const& arg) { From c6128d1388c629179933482998e29f73a0c5cdfb Mon Sep 17 00:00:00 2001 From: John Freeman Date: Fri, 5 Apr 2024 13:59:23 -0500 Subject: [PATCH 19/24] Fix formatting --- src/test/app/AMM_test.cpp | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 3fe97ea360d..2b236ee18c4 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -2423,9 +2423,10 @@ struct AMM_test : public jtx::AMMTest // this transaction fails because AMMBid transaction can not burn // all the outstanding LPTokens env(amm.bid({ - .account = gw, - .bidMin = 1'000'000, - }), ter(tecAMM_INVALID_TOKENS)); + .account = gw, + .bidMin = 1'000'000, + }), + ter(tecAMM_INVALID_TOKENS)); } // burn all the LPTokens through a AMMBid transaction @@ -2441,9 +2442,11 @@ struct AMM_test : public jtx::AMMTest // this transaction suceeds because the bid price is less than // the total outstanding LPToken balance env(amm.bid({ - .account = gw, - .bidMin = STAmount{amm.lptIssue(), UINT64_C(999'999)}, - }), ter(tesSUCCESS)).close(); + .account = gw, + .bidMin = STAmount{amm.lptIssue(), UINT64_C(999'999)}, + }), + ter(tesSUCCESS)) + .close(); // gw must posses the auction slot BEAST_EXPECT(amm.expectAuctionSlot(100, 0, IOUAmount{999'999})); @@ -2459,8 +2462,10 @@ struct AMM_test : public jtx::AMMTest // transaction. This transaction fails because it would burn all // the remaining LPTokens env(amm.bid({ - .account = gw, .bidMin = 1, - }), ter(tecAMM_INVALID_TOKENS)); + .account = gw, + .bidMin = 1, + }), + ter(tecAMM_INVALID_TOKENS)); } testAMM([&](AMM& ammAlice, Env& env) { From fcb4bdb809187be8d404ce76040b08dad86720eb Mon Sep 17 00:00:00 2001 From: John Freeman Date: Thu, 11 Apr 2024 14:41:18 -0500 Subject: [PATCH 20/24] Address comments --- Builds/CMake/RippledCore.cmake | 1 - src/test/app/AMMWithdraw_test.cpp | 169 ------------------------------ src/test/app/AMM_test.cpp | 164 +++++++++++++++++++++++------ 3 files changed, 133 insertions(+), 201 deletions(-) delete mode 100644 src/test/app/AMMWithdraw_test.cpp diff --git a/Builds/CMake/RippledCore.cmake b/Builds/CMake/RippledCore.cmake index 712d01f33f7..f826c428135 100644 --- a/Builds/CMake/RippledCore.cmake +++ b/Builds/CMake/RippledCore.cmake @@ -806,7 +806,6 @@ if (tests) src/test/app/AMM_test.cpp src/test/app/AMMCalc_test.cpp src/test/app/AMMExtended_test.cpp - src/test/app/AMMWithdraw_test.cpp src/test/app/Check_test.cpp src/test/app/Clawback_test.cpp src/test/app/CrossingLimits_test.cpp diff --git a/src/test/app/AMMWithdraw_test.cpp b/src/test/app/AMMWithdraw_test.cpp deleted file mode 100644 index 6bb30d46b33..00000000000 --- a/src/test/app/AMMWithdraw_test.cpp +++ /dev/null @@ -1,169 +0,0 @@ -//------------------------------------------------------------------------------ -/* - This file is part of rippled: https://github.com/ripple/rippled - Copyright (c) 2023 Ripple Labs Inc. - - Permission to use, copy, modify, and/or distribute this software for any - purpose with or without fee is hereby granted, provided that the above - copyright notice and this permission notice appear in all copies. - - THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES - WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF - MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR - ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES - WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN - ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF - OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. -*/ -//============================================================================== -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include - -#include -#include -#include - -namespace ripple { -namespace test { - -struct AMMWithdraw_test : public jtx::AMMTest -{ - void - testMalformed() - { - using namespace jtx; - - // { - // Env env{*this}; - // env.fund(XRP(30'000), alice); - // Json::Value jv; - // jv[jss::Account] = alice.human(); - // jv[jss::TransactionType] = jss::AMMWithdraw; - // env(jv, fee(drops(-10)), ter(temBAD_FEE)); - // } - - testAMM([&](AMM& ammAlice, Env& env) { - WithdrawArg args{ - .flags = tfSingleAsset, - .err = ter(temMALFORMED), - }; - ammAlice.withdraw(args); - }); - - testAMM([&](AMM& ammAlice, Env& env) { - WithdrawArg args{ - .flags = tfOneAssetLPToken, - .err = ter(temMALFORMED), - }; - ammAlice.withdraw(args); - }); - - testAMM([&](AMM& ammAlice, Env& env) { - WithdrawArg args{ - .flags = tfLimitLPToken, - .err = ter(temMALFORMED), - }; - ammAlice.withdraw(args); - }); - - testAMM([&](AMM& ammAlice, Env& env) { - WithdrawArg args{ - .asset1Out = XRP(100), - .asset2Out = XRP(100), - .err = ter(temBAD_AMM_TOKENS), - }; - ammAlice.withdraw(args); - }); - - testAMM([&](AMM& ammAlice, Env& env) { - WithdrawArg args{ - .asset1Out = XRP(100), - .asset2Out = BAD(100), - .err = ter(temBAD_CURRENCY), - }; - ammAlice.withdraw(args); - }); - - testAMM([&](AMM& ammAlice, Env& env) { - Json::Value jv; - jv[jss::TransactionType] = jss::AMMWithdraw; - jv[jss::Flags] = tfLimitLPToken; - jv[jss::Account] = alice.human(); - ammAlice.setTokens(jv); - XRP(100).value().setJson(jv[jss::Amount]); - USD(100).value().setJson(jv[jss::EPrice]); - env(jv, ter(temBAD_AMM_TOKENS)); - }); - } - - void - testOther() - { - using namespace jtx; - - testAMM( - [&](AMM& ammAlice, Env& env) { - WithdrawArg args{ - .asset1Out = XRP(100), - .err = ter(tecAMM_BALANCE), - }; - ammAlice.withdraw(args); - }, - {{XRP(99), USD(99)}}); - - testAMM( - [&](AMM& ammAlice, Env& env) { - WithdrawArg args{ - .asset1Out = USD(100), - .err = ter(tecAMM_BALANCE), - }; - ammAlice.withdraw(args); - }, - {{XRP(99), USD(99)}}); - - { - Env env{*this}; - env.fund(XRP(30'000), gw, alice, bob); - env.close(); - env(fset(gw, asfRequireAuth)); - env(trust(alice, gw["USD"](30'000), 0)); - env(trust(gw, alice["USD"](0), tfSetfAuth)); - // Bob trusts Gateway to owe him USD... - env(trust(bob, gw["USD"](30'000), 0)); - // ...but Gateway does not authorize Bob to hold its USD. - env.close(); - env(pay(gw, alice, USD(10'000))); - env.close(); - AMM ammAlice(env, alice, XRP(10'000), USD(10'000)); - WithdrawArg args{ - .account = bob, - .asset1Out = USD(100), - .err = ter(tecNO_AUTH), - }; - ammAlice.withdraw(args); - } - } - - void - run() override - { - testMalformed(); - testOther(); - } -}; - -BEAST_DEFINE_TESTSUITE_PRIO(AMMWithdraw, app, ripple, 1); - -} // namespace test -} // namespace ripple diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 2b236ee18c4..016a0cde201 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -948,7 +948,7 @@ struct AMM_test : public jtx::AMMTest Issue{gw["USD"].currency, ammAlice.ammAccount()}, 0}, tfSetFreeze)); env.close(); - // Can deposit non-frozen token + // Cannot deposit non-frozen token ammAlice.deposit( carol, 1'000'000, @@ -1475,6 +1475,48 @@ struct AMM_test : public jtx::AMMTest using namespace jtx; + testAMM( + [&](AMM& ammAlice, Env& env) { + WithdrawArg args{ + .asset1Out = XRP(100), + .err = ter(tecAMM_BALANCE), + }; + ammAlice.withdraw(args); + }, + {{XRP(99), USD(99)}}); + + testAMM( + [&](AMM& ammAlice, Env& env) { + WithdrawArg args{ + .asset1Out = USD(100), + .err = ter(tecAMM_BALANCE), + }; + ammAlice.withdraw(args); + }, + {{XRP(99), USD(99)}}); + + { + Env env{*this}; + env.fund(XRP(30'000), gw, alice, bob); + env.close(); + env(fset(gw, asfRequireAuth)); + env(trust(alice, gw["USD"](30'000), 0)); + env(trust(gw, alice["USD"](0), tfSetfAuth)); + // Bob trusts Gateway to owe him USD... + env(trust(bob, gw["USD"](30'000), 0)); + // ...but Gateway does not authorize Bob to hold its USD. + env.close(); + env(pay(gw, alice, USD(10'000))); + env.close(); + AMM ammAlice(env, alice, XRP(10'000), USD(10'000)); + WithdrawArg args{ + .account = bob, + .asset1Out = USD(100), + .err = ter(tecNO_AUTH), + }; + ammAlice.withdraw(args); + } + testAMM([&](AMM& ammAlice, Env& env) { // Invalid flags ammAlice.withdraw( @@ -2438,7 +2480,7 @@ struct AMM_test : public jtx::AMMTest // auction slot is owned by the creator of the AMM i.e. gw BEAST_EXPECT(amm.expectAuctionSlot(100, 0, IOUAmount{0})); - // gw burns all but one of her LPTokens through a bid transaction + // gw burns all but one of its LPTokens through a bid transaction // this transaction suceeds because the bid price is less than // the total outstanding LPToken balance env(amm.bid({ @@ -2448,17 +2490,17 @@ struct AMM_test : public jtx::AMMTest ter(tesSUCCESS)) .close(); - // gw must posses the auction slot + // gw must own the auction slot BEAST_EXPECT(amm.expectAuctionSlot(100, 0, IOUAmount{999'999})); - // 999'999 tokens are burned, only 1 LPToken is in vogue + // 999'999 tokens are burned, only 1 LPToken is owned by gw BEAST_EXPECT( amm.expectBalances(XRP(1'000), USD(1'000), IOUAmount{1})); - // gw posses only one LPToken in her balance + // gw owns only 1 LPToken in its balance BEAST_EXPECT(Number{amm.getLPTokensBalance(gw)} == 1); - // gw attempts to burn the last of her LPTokens in an AMMBid + // gw attempts to burn the last of its LPTokens in an AMMBid // transaction. This transaction fails because it would burn all // the remaining LPTokens env(amm.bid({ @@ -5069,38 +5111,98 @@ struct AMM_test : public jtx::AMMTest } void - testCore() + testMalformed() + { + using namespace jtx; + + testAMM([&](AMM& ammAlice, Env& env) { + WithdrawArg args{ + .flags = tfSingleAsset, + .err = ter(temMALFORMED), + }; + ammAlice.withdraw(args); + }); + + testAMM([&](AMM& ammAlice, Env& env) { + WithdrawArg args{ + .flags = tfOneAssetLPToken, + .err = ter(temMALFORMED), + }; + ammAlice.withdraw(args); + }); + + testAMM([&](AMM& ammAlice, Env& env) { + WithdrawArg args{ + .flags = tfLimitLPToken, + .err = ter(temMALFORMED), + }; + ammAlice.withdraw(args); + }); + + testAMM([&](AMM& ammAlice, Env& env) { + WithdrawArg args{ + .asset1Out = XRP(100), + .asset2Out = XRP(100), + .err = ter(temBAD_AMM_TOKENS), + }; + ammAlice.withdraw(args); + }); + + testAMM([&](AMM& ammAlice, Env& env) { + WithdrawArg args{ + .asset1Out = XRP(100), + .asset2Out = BAD(100), + .err = ter(temBAD_CURRENCY), + }; + ammAlice.withdraw(args); + }); + + testAMM([&](AMM& ammAlice, Env& env) { + Json::Value jv; + jv[jss::TransactionType] = jss::AMMWithdraw; + jv[jss::Flags] = tfLimitLPToken; + jv[jss::Account] = alice.human(); + ammAlice.setTokens(jv); + XRP(100).value().setJson(jv[jss::Amount]); + USD(100).value().setJson(jv[jss::EPrice]); + env(jv, ter(temBAD_AMM_TOKENS)); + }); + } + + void + testAll() { - // testInvalidInstance(); - // testInstanceCreate(); - // testInvalidDeposit(); - // testDeposit(); - // testInvalidWithdraw(); - // testWithdraw(); - // testInvalidFeeVote(); - // testFeeVote(); + testInvalidInstance(); + testInstanceCreate(); + testInvalidDeposit(); + testDeposit(); + testInvalidWithdraw(); + testWithdraw(); + testInvalidFeeVote(); + testFeeVote(); testInvalidBid(); - // testBid(); - // testInvalidAMMPayment(); - // testBasicPaymentEngine(); - // testAMMTokens(); - // testAmendment(); - // testFlags(); - // testRippling(); - // testAMMAndCLOB(); - // testTradingFee(); - // testAdjustedTokens(); - // testAutoDelete(); - // testClawback(); - // testAMMID(); - // testSelection(); - // testFixDefaultInnerObj(); + testBid(); + testInvalidAMMPayment(); + testBasicPaymentEngine(); + testAMMTokens(); + testAmendment(); + testFlags(); + testRippling(); + testAMMAndCLOB(); + testTradingFee(); + testAdjustedTokens(); + testAutoDelete(); + testClawback(); + testAMMID(); + testSelection(); + testFixDefaultInnerObj(); + testMalformed(); } void run() override { - testCore(); + testAll(); } }; From 7ffdb33cc0103b4a5da935210b47e2a53de20340 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Wed, 3 Apr 2024 19:15:58 +0100 Subject: [PATCH 21/24] Codecov coverage reporting fixes --- .codecov.yml | 10 ++++++++++ .github/workflows/nix.yml | 12 ++++++++---- Builds/CMake/CodeCoverage.cmake | 21 +++++++++++++++++---- 3 files changed, 35 insertions(+), 8 deletions(-) diff --git a/.codecov.yml b/.codecov.yml index 191144aae16..ad57398627d 100644 --- a/.codecov.yml +++ b/.codecov.yml @@ -4,3 +4,13 @@ coverage: default: target: 60% threshold: 2% + +parsers: + cobertura: + partials_as_hits: true + handle_missing_conditions : true + +ignore: + - "src/test/" + - "src/ripple/beast/test/" + - "src/ripple/beast/unit_test/" diff --git a/.github/workflows/nix.yml b/.github/workflows/nix.yml index 916bd389e85..da61963b3f2 100644 --- a/.github/workflows/nix.yml +++ b/.github/workflows/nix.yml @@ -179,6 +179,8 @@ jobs: run: | mkdir -p ~/.conan tar -xzf conan.tar -C ~/.conan + - name: install gcovr + run: pip install "gcovr>=7,<8" - name: check environment run: | echo ${PATH} | tr ':' '\n' @@ -207,7 +209,7 @@ jobs: -DCMAKE_CXX_FLAGS="-O0" -DCMAKE_C_FLAGS="-O0" cmake-target: coverage - - name: build + - name: move coverage report shell: bash run: | mv "${build_dir}/coverage.xml" ./ @@ -218,13 +220,15 @@ jobs: path: coverage.xml retention-days: 30 - name: upload coverage report - uses: wandalen/wretry.action@v1.3.0 + uses: wandalen/wretry.action@v1.4.10 with: - action: codecov/codecov-action@v4 + action: codecov/codecov-action@v4.3.0 with: | files: coverage.xml fail_ci_if_error: true + disable_search: true verbose: true + plugin: noop token: ${{ secrets.CODECOV_TOKEN }} attempt_limit: 5 - attempt_delay: 35000 # in milliseconds + attempt_delay: 210000 # in milliseconds diff --git a/Builds/CMake/CodeCoverage.cmake b/Builds/CMake/CodeCoverage.cmake index d2af481d8a3..323303c92dc 100644 --- a/Builds/CMake/CodeCoverage.cmake +++ b/Builds/CMake/CodeCoverage.cmake @@ -95,6 +95,9 @@ # - replace both functions setup_target_for_coverage_gcovr_* with a single setup_target_for_coverage_gcovr # - add support for all gcovr output formats # +# 2024-04-03, Bronek Kozicki +# - add support for output formats: jacoco, clover, lcov +# # USAGE: # # 1. Copy this file into your cmake modules path. @@ -256,10 +259,10 @@ endif() # BASE_DIRECTORY "../" # Base directory for report # # (defaults to PROJECT_SOURCE_DIR) # FORMAT "cobertura" # Output format, one of: -# # xml cobertura sonarqube json-summary -# # json-details coveralls csv txt -# # html-single html-nested html-details -# # (xml is an alias to cobertura; +# # xml cobertura sonarqube jacoco clover +# # json-summary json-details coveralls csv +# # txt html-single html-nested html-details +# # lcov (xml is an alias to cobertura; # # if no format is set, defaults to xml) # EXCLUDE "src/dir1/*" "src/dir2/*" # Patterns to exclude (can be relative # # to BASE_DIRECTORY, with CMake 3.4+) @@ -308,6 +311,8 @@ function(setup_target_for_coverage_gcovr) set(GCOVR_OUTPUT_FILE ${Coverage_NAME}.txt) elseif(Coverage_FORMAT STREQUAL "csv") set(GCOVR_OUTPUT_FILE ${Coverage_NAME}.csv) + elseif(Coverage_FORMAT STREQUAL "lcov") + set(GCOVR_OUTPUT_FILE ${Coverage_NAME}.lcov) else() set(GCOVR_OUTPUT_FILE ${Coverage_NAME}.xml) endif() @@ -320,6 +325,14 @@ function(setup_target_for_coverage_gcovr) set(Coverage_FORMAT cobertura) # overwrite xml elseif(Coverage_FORMAT STREQUAL "sonarqube") list(APPEND GCOVR_ADDITIONAL_ARGS --sonarqube "${GCOVR_OUTPUT_FILE}" ) + elseif(Coverage_FORMAT STREQUAL "jacoco") + list(APPEND GCOVR_ADDITIONAL_ARGS --jacoco "${GCOVR_OUTPUT_FILE}" ) + list(APPEND GCOVR_ADDITIONAL_ARGS --jacoco-pretty ) + elseif(Coverage_FORMAT STREQUAL "clover") + list(APPEND GCOVR_ADDITIONAL_ARGS --clover "${GCOVR_OUTPUT_FILE}" ) + list(APPEND GCOVR_ADDITIONAL_ARGS --clover-pretty ) + elseif(Coverage_FORMAT STREQUAL "lcov") + list(APPEND GCOVR_ADDITIONAL_ARGS --lcov "${GCOVR_OUTPUT_FILE}" ) elseif(Coverage_FORMAT STREQUAL "json-summary") list(APPEND GCOVR_ADDITIONAL_ARGS --json-summary "${GCOVR_OUTPUT_FILE}" ) list(APPEND GCOVR_ADDITIONAL_ARGS --json-summary-pretty) From 803033e70299412286d37b5b8cc563a3eef0fdd1 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Fri, 12 Apr 2024 17:23:40 +0100 Subject: [PATCH 22/24] Enable github annotations --- .codecov.yml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/.codecov.yml b/.codecov.yml index ad57398627d..9d86ca7d01b 100644 --- a/.codecov.yml +++ b/.codecov.yml @@ -1,15 +1,33 @@ +codecov: + require_ci_to_pass: true + +comment: + behavior: default + layout: reach,diff,flags,tree,reach + show_carryforward_flags: false + coverage: status: project: default: target: 60% threshold: 2% + patch: + default: + target: auto + threshold: 2% + changes: false + +github_checks: + annotations: true parsers: cobertura: partials_as_hits: true handle_missing_conditions : true +slack_app: false + ignore: - "src/test/" - "src/ripple/beast/test/" From 0a308affcc22999ca6eab196cafdf6034ba00fcb Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Fri, 12 Apr 2024 18:02:35 +0100 Subject: [PATCH 23/24] Set expected range and codecov rounding --- .codecov.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.codecov.yml b/.codecov.yml index 9d86ca7d01b..3e6f09d58ae 100644 --- a/.codecov.yml +++ b/.codecov.yml @@ -7,6 +7,9 @@ comment: show_carryforward_flags: false coverage: + range: "60..80" + precision: 1 + round: nearest status: project: default: From 3fedf64f947263382113aff022022ed408950d26 Mon Sep 17 00:00:00 2001 From: John Freeman Date: Thu, 18 Apr 2024 12:07:25 -0500 Subject: [PATCH 24/24] Forgot return type specifier --- src/test/app/AMM_test.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 166f3473c54..26a4ba1b4c7 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -5169,6 +5169,7 @@ struct AMM_test : public jtx::AMMTest }); } + void testFixOverflowOffer() { using namespace jtx;