Fix tokens comparison in Payment for temREDUNDANT error #5172
Conversation
|
This patch fixes the test failure 👍 |
| { | ||
| 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.
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
src/test/app/MPToken_test.cpp
Outdated
|
|
||
| { | ||
| testcase("Test Asset from/to Json"); | ||
| Json::Value jv; |
There was a problem hiding this comment.
nit: I think it would be more thorough to test more than just an empty json value here
include/xrpl/protocol/Asset.h
Outdated
| /** Return true if both asset's issue (Issue or MPTIssue) | ||
| * are the same and hold the same token (currency or MPTID). | ||
| * Otherwise return false. |
There was a problem hiding this comment.
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.
| /** 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. |
|
Also, I notice the linux CI builds are failing. |
Thanks, I'm investigating. |
kennyzlei
requested review from
ximinez,
kennyzlei and
thejohnfreeman
and removed request for
kennyzlei
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #5172 +/- ##
=========================================
+ Coverage 77.5% 77.7% +0.1%
=========================================
Files 777 779 +2
Lines 65833 65985 +152
Branches 8182 8152 -30
=========================================
+ Hits 51031 51238 +207
+ Misses 14802 14747 -55
... and 15 files with indirect coverage changes |
gregtatcam
added
the
Passed
* Checks only Currency or MPT Issuance ID part of the Asset object. * Resolves temREDUNDANT regression detected in testing.
High Level Overview of Change
Fix semantics of tokens comparison in the payment transactor when checking for temREDUNDANT.
This check prevents an account sending tokens to self.
Current code checks for equal tokens (currency or MPTID) AND the issuers. It should only check
if currency or MPTID are equal.
Type of Change
.gitignore, formatting, dropping support for older tooling)