Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tokens comparison in Payment for temREDUNDANT error #5172

Merged
merged 11 commits into from
Nov 6, 2024
29 changes: 29 additions & 0 deletions include/xrpl/protocol/Asset.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
ximinez marked this conversation as resolved.
Show resolved Hide resolved
};

template <ValidIssueType TIss>
Expand Down Expand Up @@ -157,6 +163,26 @@ operator==(Currency const& lhs, Asset const& rhs)
return rhs.holds<Issue>() && rhs.get<Issue>().currency == lhs;
}

constexpr bool
equalTokens(Asset const& lhs, Asset const& rhs)
{
return std::visit(
[&]<typename TLhs, typename TRhs>(
TLhs const& issLhs, TRhs const& issRhs) {
if constexpr (
std::is_same_v<TLhs, Issue> && std::is_same_v<TRhs, Issue>)
return issLhs.currency == issRhs.currency;
else if constexpr (
std::is_same_v<TLhs, MPTIssue> &&
std::is_same_v<TRhs, MPTIssue>)
return issLhs.getMptID() == issRhs.getMptID();
else
return false;
},
lhs.issue_,
rhs.issue_);
}

inline bool
isXRP(Asset const& asset)
{
Expand All @@ -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
7 changes: 7 additions & 0 deletions src/libxrpl/protocol/Asset.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
132 changes: 132 additions & 0 deletions src/test/app/MPToken_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<MPTIssue>()};
Json::Value const jv = to_json(issue1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we explicitly check what the contents of jv are here? Otherwise we just have confidence that to_json and mptIssueFromJson are inverse functions without knowing if the json jv was properly created

BEAST_EXPECT(
jv[jss::mpt_issuance_id] == to_string(asset1.get<MPTIssue>()));
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<MPTIssue>()));
BEAST_EXPECT(
to_string(jv) ==
"{\"mpt_issuance_id\":"
"\"00000001A407AF5856CCF3C42619DAA925813FC955C72983\"}");
BEAST_EXPECT(asset1 == assetFromJson(jv));
}
}

public:
void
run() override
Expand Down Expand Up @@ -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();
}
};

Expand Down
2 changes: 1 addition & 1 deletion src/xrpld/app/tx/detail/Payment.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Loading