From 122ee8ba9c6f7ad0913f285b8eec245713fe6a69 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Thu, 31 Oct 2024 10:31:17 -0400 Subject: [PATCH 1/8] Fix tokens comparison in Payment for temREDUNDANT error --- include/xrpl/protocol/Asset.h | 27 +++++++++++++++++++++++++++ src/xrpld/app/tx/detail/Payment.cpp | 2 +- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/include/xrpl/protocol/Asset.h b/include/xrpl/protocol/Asset.h index bfb72ab61fc..54c030d7d8a 100644 --- a/include/xrpl/protocol/Asset.h +++ b/include/xrpl/protocol/Asset.h @@ -97,6 +97,13 @@ class Asset friend constexpr bool operator==(Currency const& lhs, Asset const& rhs); + + /** Return true if both asset's issue (Issue or MPTIssue) + * are the same and hold the same token (currency or MPTID). + * Otherwise return false. + */ + friend constexpr bool + equalTokens(Asset const& lhs, Asset const& rhs); }; template @@ -157,6 +164,26 @@ operator==(Currency const& lhs, Asset const& rhs) return rhs.holds() && rhs.get().currency == lhs; } +constexpr bool +equalTokens(Asset const& lhs, Asset const& rhs) +{ + return std::visit( + [&]( + TLhs const& issLhs, TRhs const& issRhs) { + if constexpr ( + std::is_same_v && std::is_same_v) + return issLhs.currency == issRhs.currency; + else if constexpr ( + std::is_same_v && + std::is_same_v) + return issLhs.getMptID() == issRhs.getMptID(); + else + return false; + }, + lhs.issue_, + rhs.issue_); +} + inline bool isXRP(Asset const& asset) { diff --git a/src/xrpld/app/tx/detail/Payment.cpp b/src/xrpld/app/tx/detail/Payment.cpp index 25ec119d6ae..77c8d015d1e 100644 --- a/src/xrpld/app/tx/detail/Payment.cpp +++ b/src/xrpld/app/tx/detail/Payment.cpp @@ -145,7 +145,7 @@ Payment::preflight(PreflightContext const& ctx) JLOG(j.trace()) << "Malformed transaction: " << "Bad currency."; return temBAD_CURRENCY; } - if (account == dstAccountID && srcAsset == dstAsset && !hasPaths) + if (account == dstAccountID && equalTokens(srcAsset, dstAsset) && !hasPaths) { // You're signing yourself a payment. // If hasPaths is true, you might be trying some arbitrage. From d44cefd7b4ef18fae06cc106edece0f382d3561a Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Thu, 31 Oct 2024 12:48:36 -0400 Subject: [PATCH 2/8] Add equalTokens unit-test --- src/test/app/MPToken_test.cpp | 46 +++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/src/test/app/MPToken_test.cpp b/src/test/app/MPToken_test.cpp index fa888faea17..1057704c34b 100644 --- a/src/test/app/MPToken_test.cpp +++ b/src/test/app/MPToken_test.cpp @@ -1938,6 +1938,49 @@ class MPToken_test : public beast::unit_test::suite } } + void + testTokensEquality() + { + using namespace test::jtx; + testcase("Tokens Equality"); + Currency const cur1{to_currency("CU1")}; + Currency const cur2{to_currency("CU2")}; + Account const gw1{"gw1"}; + Account const gw2{"gw2"}; + MPTID const mpt1 = makeMptID(1, gw1); + MPTID const mpt1a = makeMptID(1, gw1); + MPTID const mpt2 = makeMptID(1, gw2); + MPTID const mpt3 = makeMptID(2, gw2); + Asset const assetCur1Gw1{Issue{cur1, gw1}}; + Asset const assetCur1Gw1a{Issue{cur1, gw1}}; + Asset const assetCur2Gw1{Issue{cur2, gw1}}; + Asset const assetCur2Gw2{Issue{cur2, gw2}}; + Asset const assetMpt1Gw1{mpt1}; + Asset const assetMpt1Gw1a{mpt1a}; + Asset const assetMpt1Gw2{mpt2}; + Asset const assetMpt2Gw2{mpt3}; + + // Assets holding Issue + // Currencies are equal regardless of the issuer + BEAST_EXPECT(equalTokens(assetCur1Gw1, assetCur1Gw1a)); + BEAST_EXPECT(equalTokens(assetCur2Gw1, assetCur2Gw2)); + // Currencies are different regardless of whether the issuers + // are the same or not + BEAST_EXPECT(!equalTokens(assetCur1Gw1, assetCur2Gw1)); + BEAST_EXPECT(!equalTokens(assetCur1Gw1, assetCur2Gw2)); + + // Assets holding MPTIssue + // MPTIDs are the same if the sequence and the issuer are the same + BEAST_EXPECT(equalTokens(assetMpt1Gw1, assetMpt1Gw1a)); + // MPTIDs are different if sequence and the issuer don't match + BEAST_EXPECT(!equalTokens(assetMpt1Gw1, assetMpt1Gw2)); + BEAST_EXPECT(!equalTokens(assetMpt1Gw2, assetMpt2Gw2)); + + // Assets holding Issue and MPTIssue + BEAST_EXPECT(!equalTokens(assetCur1Gw1, assetMpt1Gw1)); + BEAST_EXPECT(!equalTokens(assetMpt2Gw2, assetCur2Gw2)); + } + public: void run() override @@ -1973,6 +2016,9 @@ class MPToken_test : public beast::unit_test::suite // Test parsed MPTokenIssuanceID in API response metadata testTxJsonMetaFields(all); + + // Test tokens equality + testTokensEquality(); } }; From be4b18462865ceedbd8b7b3a78d9e760cb052231 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Thu, 31 Oct 2024 19:19:44 -0400 Subject: [PATCH 3/8] Add unit-tests to increase coverage for helper functions --- src/test/app/MPToken_test.cpp | 78 +++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/src/test/app/MPToken_test.cpp b/src/test/app/MPToken_test.cpp index 1057704c34b..214f08450eb 100644 --- a/src/test/app/MPToken_test.cpp +++ b/src/test/app/MPToken_test.cpp @@ -1981,6 +1981,81 @@ class MPToken_test : public beast::unit_test::suite BEAST_EXPECT(!equalTokens(assetMpt2Gw2, assetCur2Gw2)); } + void + testHelperFunctions() + { + using namespace test::jtx; + Account const gw{"gw"}; + Asset const asset1{makeMptID(1, gw)}; + Asset const asset2{makeMptID(2, gw)}; + Asset const asset3{makeMptID(3, gw)}; + STAmount const amt1{asset1, 100}; + STAmount const amt2{asset2, 100}; + STAmount const amt3{asset3, 10'000}; + + { + testcase("Test STAmount MPT arithmetics"); + STAmount res = multiply(amt1, amt2, asset3); + BEAST_EXPECT(res == amt3); + + res = mulRound(amt1, amt2, asset3, true); + BEAST_EXPECT(res == amt3); + + res = mulRoundStrict(amt1, amt2, asset3, true); + BEAST_EXPECT(res == amt3); + + // overflow, any value > 3037000499ull + STAmount mptOverflow{asset2, 3037000500ull}; + try + { + res = multiply(mptOverflow, mptOverflow, asset3); + fail("should throw runtime exception 1"); + } + catch (std::runtime_error const&) + { + pass(); + } + // overflow, (v1 >> 32) * v2 > 2147483648ull + mptOverflow = STAmount{asset2, 2147483648ull}; + try + { + res = multiply( + STAmount{asset1, (2ull << 32) + 2}, mptOverflow, asset3); + fail("should throw runtime exception 2"); + } + catch (std::runtime_error const&) + { + pass(); + } + } + + { + testcase("Test MPTAmount arithmetics"); + MPTAmount mptAmt1{100}; + MPTAmount const mptAmt2{100}; + BEAST_EXPECT((mptAmt1 += mptAmt2) == MPTAmount{200}); + BEAST_EXPECT((mptAmt1 -= mptAmt2) == mptAmt1); + BEAST_EXPECT(mptAmt1 == mptAmt2); + BEAST_EXPECT(mptAmt1 == 100); + BEAST_EXPECT(MPTAmount::minPositiveAmount() == MPTAmount{1}); + } + + { + testcase("Test MPTIssue from/to Json"); + MPTIssue const issue1{asset1.get()}; + Json::Value const jv = to_json(issue1); + MPTIssue const issue2 = mptIssueFromJson(jv); + BEAST_EXPECT(issue1 == issue2); + } + + { + testcase("Test Asset from/to Json"); + Json::Value jv; + asset1.setJson(jv); + BEAST_EXPECT(asset1 == assetFromJson(jv)); + } + } + public: void run() override @@ -2019,6 +2094,9 @@ class MPToken_test : public beast::unit_test::suite // Test tokens equality testTokensEquality(); + + // Test helpers + testHelperFunctions(); } }; From aa5835b4627b968a417e060a15a49dbc3fea6f3a Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Fri, 1 Nov 2024 08:28:21 -0400 Subject: [PATCH 4/8] Address reviewer's feedback --- include/xrpl/protocol/Asset.h | 3 +++ src/libxrpl/protocol/Asset.cpp | 10 ++++++++++ src/test/app/MPToken_test.cpp | 10 ++++++---- 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/include/xrpl/protocol/Asset.h b/include/xrpl/protocol/Asset.h index 54c030d7d8a..9cefd986eca 100644 --- a/include/xrpl/protocol/Asset.h +++ b/include/xrpl/protocol/Asset.h @@ -199,6 +199,9 @@ validJSONAsset(Json::Value const& jv); Asset assetFromJson(Json::Value const& jv); +Json::Value +to_json(Asset const& asset); + } // namespace ripple #endif // RIPPLE_PROTOCOL_ASSET_H_INCLUDED diff --git a/src/libxrpl/protocol/Asset.cpp b/src/libxrpl/protocol/Asset.cpp index 67323f8614b..daca2f665df 100644 --- a/src/libxrpl/protocol/Asset.cpp +++ b/src/libxrpl/protocol/Asset.cpp @@ -70,4 +70,14 @@ assetFromJson(Json::Value const& v) return mptIssueFromJson(v); } +Json::Value +to_json(Asset const& asset) +{ + return std::visit( + [&](auto const& issue) { + return to_json(issue); + }, + asset.value()); +} + } // namespace ripple diff --git a/src/test/app/MPToken_test.cpp b/src/test/app/MPToken_test.cpp index 214f08450eb..f89e31cfe18 100644 --- a/src/test/app/MPToken_test.cpp +++ b/src/test/app/MPToken_test.cpp @@ -2044,14 +2044,16 @@ class MPToken_test : public beast::unit_test::suite testcase("Test MPTIssue from/to Json"); MPTIssue const issue1{asset1.get()}; Json::Value const jv = to_json(issue1); - MPTIssue const issue2 = mptIssueFromJson(jv); - BEAST_EXPECT(issue1 == issue2); + BEAST_EXPECT( + jv[jss::mpt_issuance_id] == to_string(asset1.get())); + BEAST_EXPECT(issue1 == mptIssueFromJson(jv)); } { testcase("Test Asset from/to Json"); - Json::Value jv; - asset1.setJson(jv); + Json::Value const jv = to_json(asset1); + BEAST_EXPECT( + jv[jss::mpt_issuance_id] == to_string(asset1.get())); BEAST_EXPECT(asset1 == assetFromJson(jv)); } } From 495c6f988747d3f7a90e06424383ee7b9b51e4cd Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Fri, 1 Nov 2024 09:07:19 -0400 Subject: [PATCH 5/8] Fix clang format --- src/libxrpl/protocol/Asset.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/libxrpl/protocol/Asset.cpp b/src/libxrpl/protocol/Asset.cpp index daca2f665df..5a496352840 100644 --- a/src/libxrpl/protocol/Asset.cpp +++ b/src/libxrpl/protocol/Asset.cpp @@ -74,10 +74,7 @@ Json::Value to_json(Asset const& asset) { return std::visit( - [&](auto const& issue) { - return to_json(issue); - }, - asset.value()); + [&](auto const& issue) { return to_json(issue); }, asset.value()); } } // namespace ripple From ff821d8042cb128c9382caa497c43dff8d2c7c48 Mon Sep 17 00:00:00 2001 From: gregtatcam Date: Mon, 4 Nov 2024 14:07:43 -0500 Subject: [PATCH 6/8] Address reviewer's feedback --- include/xrpl/protocol/Asset.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/include/xrpl/protocol/Asset.h b/include/xrpl/protocol/Asset.h index 9cefd986eca..2cccc28bd41 100644 --- a/include/xrpl/protocol/Asset.h +++ b/include/xrpl/protocol/Asset.h @@ -98,9 +98,8 @@ class Asset friend constexpr bool operator==(Currency const& lhs, Asset const& rhs); - /** Return true if both asset's issue (Issue or MPTIssue) - * are the same and hold the same token (currency or MPTID). - * Otherwise return false. + /** Return true if both assets refer to the same currency (regardless of + * issuer) or MPT issuance. Otherwise return false. */ friend constexpr bool equalTokens(Asset const& lhs, Asset const& rhs); From 6ff89585a07543357cfabc951d270edc404c4b1b Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Tue, 5 Nov 2024 12:18:00 -0500 Subject: [PATCH 7/8] Address reviewer's feedback --- src/test/app/MPToken_test.cpp | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/test/app/MPToken_test.cpp b/src/test/app/MPToken_test.cpp index f89e31cfe18..375ac019898 100644 --- a/src/test/app/MPToken_test.cpp +++ b/src/test/app/MPToken_test.cpp @@ -1995,6 +1995,7 @@ class MPToken_test : public beast::unit_test::suite { testcase("Test STAmount MPT arithmetics"); + using namespace std::string_literals; STAmount res = multiply(amt1, amt2, asset3); BEAST_EXPECT(res == amt3); @@ -2011,9 +2012,9 @@ class MPToken_test : public beast::unit_test::suite res = multiply(mptOverflow, mptOverflow, asset3); fail("should throw runtime exception 1"); } - catch (std::runtime_error const&) + catch (std::runtime_error const& e) { - pass(); + BEAST_EXPECTS(e.what() == "MPT value overflow"s, e.what()); } // overflow, (v1 >> 32) * v2 > 2147483648ull mptOverflow = STAmount{asset2, 2147483648ull}; @@ -2023,9 +2024,9 @@ class MPToken_test : public beast::unit_test::suite STAmount{asset1, (2ull << 32) + 2}, mptOverflow, asset3); fail("should throw runtime exception 2"); } - catch (std::runtime_error const&) + catch (std::runtime_error const& e) { - pass(); + BEAST_EXPECTS(e.what() == "MPT value overflow"s, e.what()); } } @@ -2034,6 +2035,7 @@ class MPToken_test : public beast::unit_test::suite MPTAmount mptAmt1{100}; MPTAmount const mptAmt2{100}; BEAST_EXPECT((mptAmt1 += mptAmt2) == MPTAmount{200}); + BEAST_EXPECT(mptAmt1 == 200); BEAST_EXPECT((mptAmt1 -= mptAmt2) == mptAmt1); BEAST_EXPECT(mptAmt1 == mptAmt2); BEAST_EXPECT(mptAmt1 == 100); @@ -2054,6 +2056,10 @@ class MPToken_test : public beast::unit_test::suite Json::Value const jv = to_json(asset1); BEAST_EXPECT( jv[jss::mpt_issuance_id] == to_string(asset1.get())); + BEAST_EXPECT( + to_string(jv) == + "{\"mpt_issuance_id\":" + "\"00000001A407AF5856CCF3C42619DAA925813FC955C72983\"}"); BEAST_EXPECT(asset1 == assetFromJson(jv)); } } From 986cc17fc45f106cbb6aad5150fd17262d8bee9e Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Tue, 5 Nov 2024 14:03:09 -0500 Subject: [PATCH 8/8] Fix Linux build --- src/test/app/MPToken_test.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/test/app/MPToken_test.cpp b/src/test/app/MPToken_test.cpp index 375ac019898..bef2c6e42a3 100644 --- a/src/test/app/MPToken_test.cpp +++ b/src/test/app/MPToken_test.cpp @@ -2006,7 +2006,7 @@ class MPToken_test : public beast::unit_test::suite BEAST_EXPECT(res == amt3); // overflow, any value > 3037000499ull - STAmount mptOverflow{asset2, 3037000500ull}; + STAmount mptOverflow{asset2, UINT64_C(3037000500)}; try { res = multiply(mptOverflow, mptOverflow, asset3); @@ -2017,11 +2017,11 @@ class MPToken_test : public beast::unit_test::suite BEAST_EXPECTS(e.what() == "MPT value overflow"s, e.what()); } // overflow, (v1 >> 32) * v2 > 2147483648ull - mptOverflow = STAmount{asset2, 2147483648ull}; + mptOverflow = STAmount{asset2, UINT64_C(2147483648)}; + uint64_t const mantissa = (2ull << 32) + 2; try { - res = multiply( - STAmount{asset1, (2ull << 32) + 2}, mptOverflow, asset3); + res = multiply(STAmount{asset1, mantissa}, mptOverflow, asset3); fail("should throw runtime exception 2"); } catch (std::runtime_error const& e)