Add MPTIssue to STIssue #5200
Add MPTIssue to STIssue #5200
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5200 +/- ##
=======================================
Coverage 77.9% 77.9%
=======================================
Files 783 783
Lines 66677 66705 +28
Branches 8108 8105 -3
=======================================
+ Hits 51921 51959 +38
+ Misses 14756 14746 -10
... and 5 files with indirect coverage changes |
include/xrpl/protocol/Asset.h
Outdated
| @@ -157,6 +167,21 @@ operator!=(Asset const& lhs, Asset const& rhs) | |||
| return !(lhs == rhs); | |||
| } | |||
|
|
|||
| constexpr bool | |||
| operator<(Asset const& lhs, Asset const& rhs) | |||
There was a problem hiding this comment.
Can you pls replace this with operator<=> ? It could look like this
friend constexpr auto
operator<=>(Asset const& lhs, Asset const& rhs)
{
return std::visit(
[&]<typename TLhs, typename TRhs>(
TLhs const& issLhs, TRhs const& issRhs) -> std::weak_ordering {
if constexpr (std::is_same_v<TLhs, TRhs>)
return issLhs <=> issRhs;
else
return lhs.issue_.index() <=> rhs.issue_.index();
},
lhs.issue_,
rhs.issue_);
}There was a problem hiding this comment.
It would be great if we could give std::strong_ordering to operator<=>(Issue const& lhs, Issue const& rhs) but I wouldn't want that change in this PR even if it is correct one (and I am not assuming that it is)
There was a problem hiding this comment.
Sure, will do. I actually added it in MPT-V2 (still a long way from release).
include/xrpl/protocol/STIssue.h
Outdated
| bool | ||
| STIssue::holds() const | ||
| { | ||
| return std::holds_alternative<TIss>(asset_.value()); |
There was a problem hiding this comment.
This should be return asset_.holds<TIss>();
| return std::weak_ordering::greater; | ||
| else | ||
| return std::weak_ordering::less; | ||
| }, |
There was a problem hiding this comment.
this is correct as long as we have exactly these two types i.e. Issue, MPTIssue inside issue_. As soon as that changes it will be no longer correct, meaning that this code is brittle. This is why I would prefer index comparison instead.
include/xrpl/protocol/STIssue.h
Outdated
| @@ -116,19 +144,19 @@ operator!=(STIssue const& lhs, STIssue const& rhs) | |||
| inline bool | |||
| operator<(STIssue const& lhs, STIssue const& rhs) | |||
There was a problem hiding this comment.
could we replace this with friend std::weak_ordering operator<=> as well ? Also, we should not need operator!= anymore
include/xrpl/protocol/STIssue.h
Outdated
| } | ||
|
|
||
| inline bool | ||
| operator<(STIssue const& lhs, Issue const& rhs) | ||
| operator<(STIssue const& lhs, Asset const& rhs) |
There was a problem hiding this comment.
also replace with friend std::weak_ordering operator<=>
| @@ -89,7 +132,7 @@ STIssue::isEquivalent(const STBase& t) const | |||
| bool | |||
| STIssue::isDefault() const | |||
| { | |||
| return issue_ == xrpIssue(); | |||
| return holds<Issue>() && asset_.get<Issue>() == xrpIssue(); | |||
Co-authored-by: Bronek Kozicki <brok@incorrekt.com>
There was a problem hiding this comment.
All good, just please try to expand unit tests a little, as per https://app.codecov.io/gh/XRPLF/rippled/pull/5200
gregtatcam
force-pushed
the
feature/mpt-stissue
branch
from
631ec0f to
0e8c062
Compare
src/test/protocol/STIssue_test.cpp
Outdated
gregtatcam
added
the
Passed
High Level Overview of Change
Replace
IssueinSTIssuewithAsset.In this change,
STIssuewithMPTIssueis unused anywhere except in MPT tests.It is intended for use in
Vaultand in transactions withSTIssuefields onceMPT is integrated into DEX.
Type of Change
.gitignore, formatting, dropping support for older tooling)Test Plan
Extend MPTTest to test that
AMMClawback,MMDeposit,AMMWithdraw,AMMDelete,AMMVotetransactions don't supportMPTIssueinAssetandAsset2.