From b22cbc4cf4759ca5c96868c8589c09b98a6f2439 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Wed, 6 Nov 2024 11:20:30 -0500 Subject: [PATCH] Fix token comparison in Payment (#5172) * Checks only Currency or MPT Issuance ID part of the Asset object. * Resolves temREDUNDANT regression detected in testing. --- include/xrpl/protocol/Asset.h | 29 ++++++ src/libxrpl/protocol/Asset.cpp | 7 ++ src/test/app/MPToken_test.cpp | 132 ++++++++++++++++++++++++++++ src/xrpld/app/tx/detail/Payment.cpp | 2 +- 4 files changed, 169 insertions(+), 1 deletion(-) diff --git a/include/xrpl/protocol/Asset.h b/include/xrpl/protocol/Asset.h index bfb72ab61fc..2cccc28bd41 100644 --- a/include/xrpl/protocol/Asset.h +++ b/include/xrpl/protocol/Asset.h @@ -97,6 +97,12 @@ class Asset friend constexpr bool operator==(Currency const& lhs, Asset const& rhs); + + /** 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); }; template @@ -157,6 +163,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) { @@ -172,6 +198,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..5a496352840 100644 --- a/src/libxrpl/protocol/Asset.cpp +++ b/src/libxrpl/protocol/Asset.cpp @@ -70,4 +70,11 @@ 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 0f92212288d..69c5d90111c 100644 --- a/src/test/app/MPToken_test.cpp +++ b/src/test/app/MPToken_test.cpp @@ -1950,6 +1950,132 @@ 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)); + } + + 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"); + using namespace std::string_literals; + 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, UINT64_C(3037000500)}; + try + { + res = multiply(mptOverflow, mptOverflow, asset3); + fail("should throw runtime exception 1"); + } + catch (std::runtime_error const& e) + { + BEAST_EXPECTS(e.what() == "MPT value overflow"s, e.what()); + } + // overflow, (v1 >> 32) * v2 > 2147483648ull + mptOverflow = STAmount{asset2, UINT64_C(2147483648)}; + uint64_t const mantissa = (2ull << 32) + 2; + try + { + res = multiply(STAmount{asset1, mantissa}, mptOverflow, asset3); + fail("should throw runtime exception 2"); + } + catch (std::runtime_error const& e) + { + BEAST_EXPECTS(e.what() == "MPT value overflow"s, e.what()); + } + } + + { + testcase("Test MPTAmount arithmetics"); + 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); + BEAST_EXPECT(MPTAmount::minPositiveAmount() == MPTAmount{1}); + } + + { + testcase("Test MPTIssue from/to Json"); + MPTIssue const issue1{asset1.get()}; + Json::Value const jv = to_json(issue1); + BEAST_EXPECT( + jv[jss::mpt_issuance_id] == to_string(asset1.get())); + BEAST_EXPECT(issue1 == mptIssueFromJson(jv)); + } + + { + testcase("Test Asset from/to Json"); + 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)); + } + } + public: void run() override @@ -1985,6 +2111,12 @@ class MPToken_test : public beast::unit_test::suite // Test parsed MPTokenIssuanceID in API response metadata testTxJsonMetaFields(all); + + // Test tokens equality + testTokensEquality(); + + // Test helpers + testHelperFunctions(); } }; 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.