Address reviewer's feedback

* Replace MPT:pair<uint32,AccountID> with MPTID:uint192 * Remove SFieldMPT * Replated TypedFieldAmount with generic TypedFieldVariant to handle STEitherAmount * Update comments
gregtatcam · Aug 26, 2024 · db124b0 · db124b0
1 parent f39eeb1
commit db124b0
Show file tree

Hide file tree

Showing 29 changed files with 307 additions and 387 deletions.
diff --git a/include/xrpl/basics/base_uint.h b/include/xrpl/basics/base_uint.h
@@ -634,6 +634,7 @@ operator<<(std::ostream& out, base_uint<Bits, Tag> const& u)
 #ifndef __INTELLISENSE__
 static_assert(sizeof(uint128) == 128 / 8, "There should be no padding bytes");
 static_assert(sizeof(uint160) == 160 / 8, "There should be no padding bytes");
+static_assert(sizeof(uint192) == 192 / 8, "There should be no padding bytes");
 static_assert(sizeof(uint256) == 256 / 8, "There should be no padding bytes");
 #endif
 

diff --git a/include/xrpl/protocol/Indexes.h b/include/xrpl/protocol/Indexes.h
@@ -290,10 +290,7 @@ Keylet
 mptIssuance(AccountID const& issuer, std::uint32_t seq) noexcept;
 
 Keylet
-mptIssuance(uint192 const& mpt) noexcept;
-
-Keylet
-mptIssuance(ripple::MPT const& mpt) noexcept;
+mptIssuance(MPTID const& mpt) noexcept;
 
 inline Keylet
 mptIssuance(uint256 const& issuance)
@@ -302,10 +299,7 @@ mptIssuance(uint256 const& issuance)
 }
 
 Keylet
-mptoken(MPT const& issuanceID, AccountID const& holder) noexcept;
-
-Keylet
-mptoken(uint192 const& issuanceID, AccountID const& holder) noexcept;
+mptoken(MPTID const& issuanceID, AccountID const& holder) noexcept;
 
 inline Keylet
 mptoken(uint256 const& mptokenKey)
@@ -336,7 +330,7 @@ getTicketIndex(AccountID const& account, std::uint32_t uSequence);
 uint256
 getTicketIndex(AccountID const& account, SeqProxy ticketSeq);
 
-uint192
+MPTID
 getMptID(AccountID const& account, std::uint32_t sequence);
 
 }  // namespace ripple

diff --git a/include/xrpl/protocol/MPTIssue.h b/include/xrpl/protocol/MPTIssue.h
@@ -28,25 +28,17 @@ namespace ripple {
 class MPTIssue
 {
 private:
-    MPT mpt_;
+    MPTID mptID_;
 
 public:
     MPTIssue() = default;
 
-    MPTIssue(MPT const& mpt);
+    MPTIssue(MPTID const& id);
 
-    MPTIssue(uint192 const& id);
-
-    AccountID const&
+    AccountID
     getIssuer() const;
 
-    MPT const&
-    mpt() const;
-
-    MPT&
-    mpt();
-
-    uint192
+    MPTID const&
     getMptID() const;
 
     friend constexpr bool
@@ -59,27 +51,17 @@ class MPTIssue
 constexpr bool
 operator==(MPTIssue const& lhs, MPTIssue const& rhs)
 {
-    return lhs.mpt_ == rhs.mpt_;
+    return lhs.mptID_ == rhs.mptID_;
 }
 
 constexpr bool
 operator!=(MPTIssue const& lhs, MPTIssue const& rhs)
 {
-    return !(lhs.mpt_ == rhs.mpt_);
-}
-
-MPT
-getMPT(uint192 const&);
-
-inline MPT
-badMPT()
-{
-    static MPT mpt{0, AccountID{0}};
-    return mpt;
+    return !(lhs.mptID_ == rhs.mptID_);
 }
 
 inline bool
-isXRP(uint192 const&)
+isXRP(MPTID const&)
 {
     return false;
 }

diff --git a/include/xrpl/protocol/SField.h b/include/xrpl/protocol/SField.h
@@ -43,6 +43,7 @@ Some fields have a different meaning for their
 // Forwards
 class STAccount;
 class STEitherAmount;
+class STAmount;
 class STIssue;
 class STBlob;
 template <int>
@@ -56,44 +57,49 @@ class STCurrency;
 #pragma push_macro("XMACRO")
 #undef XMACRO
 
-#define XMACRO(STYPE)                             \
-    /* special types */                           \
-    STYPE(STI_UNKNOWN, -2)                        \
-    STYPE(STI_NOTPRESENT, 0)                      \
-    STYPE(STI_UINT16, 1)                          \
-                                                  \
-    /* types (common) */                          \
-    STYPE(STI_UINT32, 2)                          \
-    STYPE(STI_UINT64, 3)                          \
-    STYPE(STI_UINT128, 4)                         \
-    STYPE(STI_UINT256, 5)                         \
-    STYPE(STI_AMOUNT, 6)                          \
-    STYPE(STI_EITHER_AMOUNT, 6)                   \
-    STYPE(STI_VL, 7)                              \
-    STYPE(STI_ACCOUNT, 8)                         \
-                                                  \
-    /* 9-13 are reserved */                       \
-    STYPE(STI_OBJECT, 14)                         \
-    STYPE(STI_ARRAY, 15)                          \
-                                                  \
-    /* types (uncommon) */                        \
-    STYPE(STI_UINT8, 16)                          \
-    STYPE(STI_UINT160, 17)                        \
-    STYPE(STI_PATHSET, 18)                        \
-    STYPE(STI_VECTOR256, 19)                      \
-    STYPE(STI_UINT96, 20)                         \
-    STYPE(STI_UINT192, 21)                        \
-    STYPE(STI_UINT384, 22)                        \
-    STYPE(STI_UINT512, 23)                        \
-    STYPE(STI_ISSUE, 24)                          \
-    STYPE(STI_XCHAIN_BRIDGE, 25)                  \
-    STYPE(STI_CURRENCY, 26)                       \
-                                                  \
-    /* high-level types */                        \
-    /* cannot be serialized inside other types */ \
-    STYPE(STI_TRANSACTION, 10001)                 \
-    STYPE(STI_LEDGERENTRY, 10002)                 \
-    STYPE(STI_VALIDATION, 10003)                  \
+#define XMACRO(STYPE)                              \
+    /* special types */                            \
+    STYPE(STI_UNKNOWN, -2)                         \
+    STYPE(STI_NOTPRESENT, 0)                       \
+    STYPE(STI_UINT16, 1)                           \
+                                                   \
+    /* types (common) */                           \
+    STYPE(STI_UINT32, 2)                           \
+    STYPE(STI_UINT64, 3)                           \
+    STYPE(STI_UINT128, 4)                          \
+    STYPE(STI_UINT256, 5)                          \
+    /* Need two enumerators with the same value */ \
+    /* so that SF_AMOUNT and SF_EITHER_AMOUNT */   \
+    /* map to the same serialization id. */        \
+    /* This is an artifact of */                   \
+    /* CONSTRUCT_TYPED_SFIELD */                   \
+    STYPE(STI_AMOUNT, 6)                           \
+    STYPE(STI_EITHER_AMOUNT, 6)                    \
+    STYPE(STI_VL, 7)                               \
+    STYPE(STI_ACCOUNT, 8)                          \
+                                                   \
+    /* 9-13 are reserved */                        \
+    STYPE(STI_OBJECT, 14)                          \
+    STYPE(STI_ARRAY, 15)                           \
+                                                   \
+    /* types (uncommon) */                         \
+    STYPE(STI_UINT8, 16)                           \
+    STYPE(STI_UINT160, 17)                         \
+    STYPE(STI_PATHSET, 18)                         \
+    STYPE(STI_VECTOR256, 19)                       \
+    STYPE(STI_UINT96, 20)                          \
+    STYPE(STI_UINT192, 21)                         \
+    STYPE(STI_UINT384, 22)                         \
+    STYPE(STI_UINT512, 23)                         \
+    STYPE(STI_ISSUE, 24)                           \
+    STYPE(STI_XCHAIN_BRIDGE, 25)                   \
+    STYPE(STI_CURRENCY, 26)                        \
+                                                   \
+    /* high-level types */                         \
+    /* cannot be serialized inside other types */  \
+    STYPE(STI_TRANSACTION, 10001)                  \
+    STYPE(STI_LEDGERENTRY, 10002)                  \
+    STYPE(STI_VALIDATION, 10003)                   \
     STYPE(STI_METADATA, 10004)
 
 #pragma push_macro("TO_ENUM")
@@ -302,8 +308,6 @@ class SField
     static std::map<int, SField const*> knownCodeToField;
 };
 
-enum class SFieldMPT { None, Yes, No };
-
 /** A field with a type known at compile time. */
 template <class T>
 struct TypedField : SField
@@ -325,38 +329,49 @@ struct OptionaledField
     }
 };
 
-template <class T>
-inline OptionaledField<T>
-operator~(TypedField<T> const& f)
-{
-    return OptionaledField<T>(f);
-}
-
-// Amount fields
-
-/** A field with a type known at compile time. */
-template <SFieldMPT>
-struct TypedFieldAmount : public TypedField<STEitherAmount>
+/** A field representing a variant with a type known at compile time.
+ * First template parameter is the variant type, the second
+ * template parameter is one of its alternative types. A variant field
+ * enables STObject::operator[]() overload to return the specified
+ * alternative type. For instance, STEitherAmount is a variant of STAmount
+ * and STMPTAmount. Some Amount fields, like SFee, don't support MPT
+ * and are declared as TypedVariantField<STEitherAmount, STAmount>.
+ * Conversely, sfAmount field supports MPT and is declared as
+ * TypedVariantField<STEitherAmount>. Then tx[sfFee] always returns STAmount,
+ * while tx[sfAmount] returns STEitherAmount and the caller has to get
+ * the specific type that STEitherAmount holds.
+ */
+template <class T, class H = T>
+struct TypedVariantField : TypedField<T>
 {
     template <class... Args>
-    explicit TypedFieldAmount(private_access_tag_t pat, Args&&... args);
+    explicit TypedVariantField(
+        SField::private_access_tag_t pat,
+        Args&&... args);
 };
 
-/** Indicate std::optional field semantics. */
-template <SFieldMPT M>
-struct OptionaledFieldAmount : public OptionaledField<STEitherAmount>
+/** Indicate std::optional variant field semantics. */
+template <class T, class H = T>
+struct OptionaledVariantField : OptionaledField<T>
 {
-    explicit OptionaledFieldAmount(TypedFieldAmount<M> const& f_)
-        : OptionaledField<STEitherAmount>(f_)
+    explicit OptionaledVariantField(TypedVariantField<T, H> const& f_)
+        : OptionaledField<T>(f_)
     {
     }
 };
 
-template <SFieldMPT M>
-inline OptionaledFieldAmount<M>
-operator~(TypedFieldAmount<M> const& f)
+template <class T>
+inline OptionaledField<T>
+operator~(TypedField<T> const& f)
+{
+    return OptionaledField<T>(f);
+}
+
+template <class T, class H = T>
+inline OptionaledVariantField<T, H>
+operator~(TypedVariantField<T, H> const& f)
 {
-    return OptionaledFieldAmount<M>(f);
+    return OptionaledVariantField<T, H>(f);
 }
 
 //------------------------------------------------------------------------------
@@ -376,8 +391,8 @@ using SF_UINT384 = TypedField<STBitString<384>>;
 using SF_UINT512 = TypedField<STBitString<512>>;
 
 using SF_ACCOUNT = TypedField<STAccount>;
-using SF_AMOUNT = TypedFieldAmount<SFieldMPT::No>;
-using SF_EITHER_AMOUNT = TypedFieldAmount<SFieldMPT::Yes>;
+using SF_AMOUNT = TypedVariantField<STEitherAmount, STAmount>;
+using SF_EITHER_AMOUNT = TypedVariantField<STEitherAmount>;
 using SF_ISSUE = TypedField<STIssue>;
 using SF_CURRENCY = TypedField<STCurrency>;
 using SF_VL = TypedField<STBlob>;

diff --git a/include/xrpl/protocol/SOTemplate.h b/include/xrpl/protocol/SOTemplate.h
@@ -73,13 +73,15 @@ class SOElement
     {
         init(fieldName);
     }
-    SOElement(TypedFieldAmount<SFieldMPT::No> const& fieldName, SOEStyle style)
+    SOElement(
+        TypedVariantField<STEitherAmount, STAmount> const& fieldName,
+        SOEStyle style)
         : sField_(fieldName), style_(style), supportMpt_(soeMPTNotSupported)
     {
         init(fieldName);
     }
     SOElement(
-        TypedFieldAmount<SFieldMPT::Yes> const& fieldName,
+        TypedVariantField<STEitherAmount> const& fieldName,
         SOEStyle style,
         SOETxMPTAmount supportMpt = soeMPTNotSupported)
         : sField_(fieldName), style_(style), supportMpt_(supportMpt)

diff --git a/include/xrpl/protocol/STAmount.h b/include/xrpl/protocol/STAmount.h
@@ -32,10 +32,6 @@
 
 namespace ripple {
 
-struct int64_tag_t
-{
-};
-
 // Internal form:
 // 1: If amount is zero, then value is zero and offset is -100
 // 2: Otherwise:
@@ -102,8 +98,6 @@ class STAmount final
         bool native,
         bool negative);
 
-    STAmount(std::int64_t mantissa, int64_tag_t);
-
     explicit STAmount(std::uint64_t mantissa = 0, bool negative = false);
 
     STAmount(

diff --git a/include/xrpl/protocol/STEitherAmount.h b/include/xrpl/protocol/STEitherAmount.h
@@ -29,11 +29,6 @@ template <typename TAmnt>
 concept ValidAmountType =
     std::is_same_v<TAmnt, STAmount> || std::is_same_v<TAmnt, STMPTAmount>;
 
-// Currency or MPT issuance ID
-template <typename T>
-concept ValidAssetType =
-    std::is_same_v<T, Currency> || std::is_same_v<T, uint192>;
-
 template <typename T>
 concept EitherAmountType = std::is_same_v<T, STEitherAmount> ||
     std::is_same_v<T, std::optional<STEitherAmount>>;
@@ -100,7 +95,7 @@ class STEitherAmount : public STBase, public CountedObject<STEitherAmount>
     std::variant<STAmount, STMPTAmount>&
     getValue();
 
-    AccountID const&
+    AccountID
     getIssuer() const;
 
     bool

diff --git a/include/xrpl/protocol/STMPTAmount.h b/include/xrpl/protocol/STMPTAmount.h
@@ -66,13 +66,13 @@ class STMPTAmount final : public MPTAmount
     bool
     isDefault() const;
 
-    AccountID const&
+    AccountID
     getIssuer() const;
 
     MPTIssue const&
     issue() const;
 
-    uint192
+    MPTID const&
     getCurrency() const;
 
     void