Address reviewer feedback

include/xrpl/basics/MPTAmount.h

@@ -88,9 +88,6 @@ class MPTAmount : private boost::totally_ordered<MPTAmount>,
     constexpr value_type
     value() const;
 
-    friend std::istream&
-    operator>>(std::istream& s, MPTAmount& val);
-
     static MPTAmount
     minPositiveAmount();
 };

include/xrpl/protocol/Asset.h

@@ -30,11 +30,10 @@ template <typename TIss>
 concept ValidIssueType =
     std::is_same_v<TIss, Issue> || std::is_same_v<TIss, MPTIssue>;
 
-/* Asset is a variant of Issue (XRP and IOU) and MPTIssue (MPT).
- * It enables handling of different issues when either one is expected.
- * For instance, it extends STAmount class to support either issue
- * in a general way. It handles specifics such arithmetics and serialization
- * depending on specific issue type held by Asset.
+/* Asset is an abstraction of three different issue types: XRP, IOU, MPT.
+ * For historical reasons, two issue types XRP and IOU are wrapped in Issue
+ * type. Asset replaces Issue where any issue type is expected. For instance,
+ * STAmount replaces Issue with Asset to represent any issue amount.
  */
 class Asset
 {
@@ -45,15 +44,27 @@ class Asset
 public:
     Asset() = default;
 
-    Asset(Issue const& issue);
+    Asset(Issue const& issue) : issue_(issue)
+    {
+    }
 
-    Asset(MPTIssue const& mptIssue);
+    Asset(MPTIssue const& mptIssue) : issue_(mptIssue)
+    {
+    }
 
-    Asset(MPTID const& issuanceID);
+    Asset(MPTID const& issuanceID) : issue_(MPTIssue{issuanceID})
+    {
+    }
 
-    explicit operator Issue() const;
+    explicit operator Issue() const
+    {
+        return get<Issue>();
+    }
 
-    explicit operator MPTIssue() const;
+    explicit operator MPTIssue() const
+    {
+        return get<MPTIssue>();
+    }
 
     AccountID const&
     getIssuer() const;
@@ -79,6 +90,12 @@ class Asset
     void
     setJson(Json::Value& jv) const;
 
+    bool
+    native() const
+    {
+        return holds<Issue>() && get<Issue>().native();
+    }
+
     friend constexpr bool
     operator==(Asset const& lhs, Asset const& rhs);
 
@@ -141,7 +158,7 @@ operator!=(Asset const& lhs, Asset const& rhs)
 inline bool
 isXRP(Asset const& asset)
 {
-    return asset.holds<Issue>() && isXRP(asset.get<Issue>());
+    return asset.native();
 }
 
 std::string

include/xrpl/protocol/Indexes.h

@@ -288,7 +288,7 @@ Keylet
 oracle(AccountID const& account, std::uint32_t const& documentID) noexcept;
 
 Keylet
-mptIssuance(AccountID const& issuer, std::uint32_t seq) noexcept;
+mptIssuance(std::uint32_t seq, AccountID const& issuer) noexcept;
 
 Keylet
 mptIssuance(MPTID const& issuanceID) noexcept;
@@ -303,9 +303,9 @@ Keylet
 mptoken(MPTID const& issuanceID, AccountID const& holder) noexcept;
 
 inline Keylet
-mptoken(uint256 const& issuanceKey)
+mptoken(uint256 const& mptokenKey)
 {
-    return {ltMPTOKEN, issuanceKey};
+    return {ltMPTOKEN, mptokenKey};
 }
 
 Keylet
@@ -352,7 +352,7 @@ std::array<keyletDesc<AccountID const&>, 6> const directAccountKeylets{
      {&keylet::did, jss::DID, true}}};
 
 MPTID
-makeMptID(AccountID const& account, std::uint32_t sequence);
+makeMptID(std::uint32_t sequence, AccountID const& account);
 
 }  // namespace ripple
 

include/xrpl/protocol/Issue.h

@@ -55,6 +55,9 @@ class Issue
 
     void
     setJson(Json::Value& jv) const;
+
+    bool
+    native() const;
 };
 
 bool
@@ -126,7 +129,7 @@ noIssue()
 inline bool
 isXRP(Issue const& issue)
 {
-    return issue == xrpIssue();
+    return issue.native();
 }
 
 }  // namespace ripple

include/xrpl/protocol/MPTIssue.h

@@ -25,11 +25,9 @@
 
 namespace ripple {
 
-/* MPTIssue represents a Multi Purpose Token (MPT) and enables handling of
- * either Issue or MPTIssue tokens by Asset and STAmount. MPT is identified
- * by MPTID, which is a 192-bit concatenation of a 32-bit account sequence
- * number at the time of MPT creation and a 160-bit account id.
- * The sequence number is stored in big endian order.
+/* Adapt MPTID to provide the same interface as Issue. Enables using static
+ * polymorphism by Asset and other classes. MPTID is a 192-bit concatenation
+ * of a 32-bit account sequence and a 160-bit account id.
  */
 class MPTIssue
 {
@@ -58,6 +56,12 @@ class MPTIssue
 
     friend constexpr bool
     operator!=(MPTIssue const& lhs, MPTIssue const& rhs);
+
+    bool
+    native() const
+    {
+        return false;
+    }
 };
 
 constexpr bool

include/xrpl/protocol/STAmount.h

@@ -447,7 +447,7 @@ STAmount::exponent() const noexcept
 inline bool
 STAmount::native() const noexcept
 {
-    return isXRP(mAsset);
+    return mAsset.native();
 }
 
 template <ValidIssueType TIss>
@@ -679,7 +679,7 @@ getRate(STAmount const& offerOut, STAmount const& offerIn);
 inline bool
 isXRP(STAmount const& amount)
 {
-    return isXRP(amount.asset());
+    return amount.native();
 }
 
 // Since `canonicalize` does not have access to a ledger, this is needed to put

src/libxrpl/protocol/Asset.cpp

@@ -23,59 +23,31 @@
 
 namespace ripple {
 
-Asset::Asset(Issue const& issue) : issue_(issue)
-{
-}
-
-Asset::Asset(MPTIssue const& mptIssue) : issue_(mptIssue)
-{
-}
-
-Asset::Asset(MPTID const& issuanceID) : issue_(MPTIssue{issuanceID})
-{
-}
-
-Asset::operator Issue() const
-{
-    return get<Issue>();
-}
-
-Asset::operator MPTIssue() const
-{
-    return get<MPTIssue>();
-}
-
 AccountID const&
 Asset::getIssuer() const
 {
-    if (holds<Issue>())
-        return get<Issue>().getIssuer();
-    return get<MPTIssue>().getIssuer();
+    return std::visit(
+        [&](auto&& issue) -> AccountID const& { return issue.getIssuer(); },
+        issue_);
 }
 
 std::string
 Asset::getText() const
 {
-    if (holds<Issue>())
-        return get<Issue>().getText();
-    return get<MPTIssue>().getText();
+    return std::visit([&](auto&& issue) { return issue.getText(); }, issue_);
 }
 
 void
 Asset::setJson(Json::Value& jv) const
 {
-    if (holds<Issue>())
-        get<Issue>().setJson(jv);
-    else
-        get<MPTIssue>().setJson(jv);
+    std::visit([&](auto&& issue) { issue.setJson(jv); }, issue_);
 }
 
 std::string
 to_string(Asset const& asset)
 {
-    if (asset.holds<Issue>())
-        return to_string(asset.get<Issue>());
-    return to_string(asset.get<MPTIssue>());
+    return std::visit(
+        [&](auto const& issue) { return to_string(issue); }, asset.value());
 }
 
 bool

src/libxrpl/protocol/Indexes.cpp

@@ -138,7 +138,7 @@ getTicketIndex(AccountID const& account, SeqProxy ticketSeq)
 }
 
 MPTID
-makeMptID(AccountID const& account, std::uint32_t sequence)
+makeMptID(std::uint32_t sequence, AccountID const& account)
 {
     MPTID u;
     sequence = boost::endian::native_to_big(sequence);
@@ -464,9 +464,9 @@ oracle(AccountID const& account, std::uint32_t const& documentID) noexcept
 }
 
 Keylet
-mptIssuance(AccountID const& issuer, std::uint32_t seq) noexcept
+mptIssuance(std::uint32_t seq, AccountID const& issuer) noexcept
 {
-    return mptIssuance(makeMptID(issuer, seq));
+    return mptIssuance(makeMptID(seq, issuer));
 }
 
 Keylet

src/libxrpl/protocol/Issue.cpp

@@ -57,6 +57,12 @@ Issue::setJson(Json::Value& jv) const
         jv[jss::issuer] = toBase58(account);
 }
 
+bool
+Issue::native() const
+{
+    return *this == xrpIssue();
+}
+
 bool
 isConsistent(Issue const& ac)
 {

src/libxrpl/protocol/STAmount.cpp

@@ -839,7 +839,7 @@ amountFromString(Asset const& asset, std::string const& amount)
     bool negative = (match[1].matched && (match[1] == "-"));
 
     // Can't specify XRP or MPT using fractional representation
-    if ((isXRP(asset) || asset.holds<MPTIssue>()) && match[3].matched)
+    if ((asset.native() || asset.holds<MPTIssue>()) && match[3].matched)
         Throw<std::runtime_error>(
             "XRP and MPT must be specified as integral amount.");
 
@@ -962,7 +962,7 @@ amountFromJson(SField const& name, Json::Value const& v)
             if (!issuer.isString() ||
                 !to_issuer(issue.account, issuer.asString()))
                 Throw<std::runtime_error>("invalid issuer");
-            if (isXRP(issue))
+            if (issue.native())
                 Throw<std::runtime_error>("invalid issuer");
             asset = issue;
         }
@@ -1182,7 +1182,7 @@ multiply(STAmount const& v1, STAmount const& v2, Asset const& asset)
     if (v1 == beast::zero || v2 == beast::zero)
         return STAmount(asset);
 
-    if (v1.native() && v2.native() && isXRP(asset))
+    if (v1.native() && v2.native() && asset.native())
     {
         std::uint64_t const minV = std::min(getSNValue(v1), getSNValue(v2));
         std::uint64_t const maxV = std::max(getSNValue(v1), getSNValue(v2));
@@ -1386,7 +1386,7 @@ mulRoundImpl(
     if (v1 == beast::zero || v2 == beast::zero)
         return {asset};
 
-    bool const xrp = isXRP(asset);
+    bool const xrp = asset.native();
 
     if (v1.native() && v2.native() && xrp)
     {
@@ -1557,7 +1557,7 @@ divRoundImpl(
 
     if (resultNegative != roundUp)
         canonicalizeRound(
-            isXRP(asset) || asset.holds<MPTIssue>(), amount, offset, roundUp);
+            asset.native() || asset.holds<MPTIssue>(), amount, offset, roundUp);
 
     STAmount result = [&]() {
         // If appropriate, tell Number the rounding mode we are using.
@@ -1571,7 +1571,7 @@ divRoundImpl(
 
     if (roundUp && !resultNegative && !result)
     {
-        if (isXRP(asset) || asset.holds<MPTIssue>())
+        if (asset.native() || asset.holds<MPTIssue>())
         {
             // return the smallest value above zero
             amount = 1;