-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
122ee8b
Fix tokens comparison in Payment for temREDUNDANT error
gregtatcam d44cefd
Add equalTokens unit-test
gregtatcam be4b184
Add unit-tests to increase coverage for helper functions
gregtatcam aa5835b
Address reviewer's feedback
gregtatcam 495c6f9
Fix clang format
gregtatcam 8d0b19c
Merge branch 'develop' into fix/equal-tokens-payment
gregtatcam ff821d8
Address reviewer's feedback
gregtatcam 6ff8958
Address reviewer's feedback
gregtatcam 986cc17
Fix Linux build
gregtatcam 69f20b7
Merge branch 'develop' into fix/equal-tokens-payment
ximinez 136c2a9
Merge branch 'develop' into fix/equal-tokens-payment
ximinez File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1938,6 +1938,126 @@ 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"); | ||
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(); | ||
} | ||
ximinez marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// 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(); | ||
} | ||
ximinez marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
{ | ||
testcase("Test MPTAmount arithmetics"); | ||
MPTAmount mptAmt1{100}; | ||
MPTAmount const mptAmt2{100}; | ||
BEAST_EXPECT((mptAmt1 += mptAmt2) == MPTAmount{200}); | ||
BEAST_EXPECT((mptAmt1 -= mptAmt2) == mptAmt1); | ||
ximinez marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can we explicitly check what the contents of |
||
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(asset1 == assetFromJson(jv)); | ||
ximinez marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
public: | ||
void | ||
run() override | ||
|
@@ -1973,6 +2093,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(); | ||
} | ||
}; | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm suggesting a change to the whole string, but wanted to note the original string was one of the rare occasions you'd need the plural possessive "assets'" (apostrophe after the s). You'd need to pluralize "issues" too.