Use STEitherAmount as a variant of STAmount and STMPTAmount #30

gregtatcam · 2024-07-24T18:25:48Z

Introduce STEitherAmount to represent STAmount and STMPTAmount as a variant. Accessing specific amount field requires a call to get().
Add amendment guards to all transactions with STI_AMOUNT field.
Extend SOElement with enum SOESupportMPT field. TxFormats::TxFormats() is used to declare amount fields supporting both STAmount and STMPTAmount. This provides an additional run-time type-safety.

and SF_EITHER_AMOUNT (support MPT). Add TypedFieldAmount to declare Amount fields. Extend STObject with getters/setters for SF_AMOUNT fields.

…ount

shawnxie999 · 2024-08-08T19:54:40Z

include/xrpl/protocol/STEitherAmount.h

+get(auto&& amount)
+{
+    using TAmnt = std::decay_t<decltype(amount)>;
+    if constexpr (std::is_same_v<TAmnt, STEitherAmount>)


is this to support get<STEitherAmount>? I don't think it is being used anywhere

It's used to get either STAmount or STMPTAmount. See the concept ValidAmountType.

shawnxie999 · 2024-08-08T19:57:34Z

include/xrpl/protocol/STEitherAmount.h

+    if constexpr (std::is_same_v<TAmnt, STEitherAmount>)
+    {
+        if constexpr (std::is_lvalue_reference_v<decltype(amount)>)
+            return amount.template get<T>();


would this call the either one of two get()?:

template <ValidAmountType T> T const& STEitherAmount::get() const { if (std::holds_alternative<T>(amount_)) return std::get<T>(amount_); Throw<std::logic_error>("Invalid STEitherAmount conversion"); } template <ValidAmountType T> T& STEitherAmount::get() { if (std::holds_alternative<T>(amount_)) return std::get<T>(amount_); Throw<std::logic_error>("Invalid STEitherAmount conversion");

It seems like both of them would throw.

One is const and the other one is not.

And, yes one of them is going to be called and will throw if the requested type doesn't match the type that the variant holds.

shawnxie999 · 2024-08-09T17:54:16Z

include/xrpl/protocol/STEitherAmount.h

+        },
+        lhs.getValue(),
+        rhs.getValue());
+    if (lhs.isIssue() == rhs.isIssue())


is this code reachable?

Thanks. It's not reachable. I was debating what version to use and forgot to remove it.

to provide one single check for all tx. Update unit-tests. Fix payment for unauthorized holder

to have separate functions for MPT/IOU handling since there is little overlap between them. Remove the check for DeletableAccounts

shawnxie999 · 2024-08-16T15:47:21Z

src/libxrpl/basics/MPTAmount.cpp

+MPTAmount&
+MPTAmount::operator+=(MPTAmount const& other)
+{
+    value_ += other.value();


does it ever have to check overflow here?

nevermind, its checked in the constructor

shawnxie999 · 2024-08-19T18:47:30Z

src/test/protocol/STTx_test.cpp

@@ -1344,22 +1344,22 @@ class STTx_test : public beast::unit_test::suite
            0x73, 0x00, 0x81, 0x14, 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00,
            0x00, 0x65, 0x24, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
            0xe5, 0xfe, 0xf3, 0xe7, 0xe5, 0x65, 0x24, 0x00, 0x00, 0x00, 0x00,
-            0x20, 0x1e, 0x00, 0x6f, 0x00, 0x00, 0x20, 0x1f, 0x03, 0xf6, 0x00,


what's changing this for?

I took this change from Howard's MPT branch. One of the tests fails without it.

shawnxie999 · 2024-08-20T13:55:16Z

include/xrpl/protocol/SField.h

@@ -520,7 +551,7 @@ extern SF_UINT256 const sfHookNamespace;
 extern SF_UINT256 const sfHookSetTxnID;

 // currency amount (common)
-extern SF_AMOUNT const sfAmount;
+extern SF_EITHER_AMOUNT const sfAmount;


why wouldn't we need to change the other Amount types like sfBalance to SF_EITHER_AMOUNT?

SF_EITHER_AMOUNT is used to indicate that this field can be either STAmount or STMPTAmount. SF_AMOUNT indicates that the field can be only STAmount. Only sfAmount supports MPT in the first release. What this actually does is this: when you access sfAmount as tx[sfAmount] then it returns STEitherAmount and you have explicitly get the amount with get<>(). If you access sfBalance as tx[sfBalance] then it returns STAmount.

Yes I understand that, but don't we already have something like this inside the TxFormat.cpp?
{sfAmount, soeREQUIRED, soeMPTSupported},

Or i guess this one is compile-type check where as the SOElement is runtime.

TxFormat defines a run-time check, which is done in invalidMPTAmountInTx(). SF_AMOUNT and SF_EITHER_AMOUNT declare different types for the fields that don't support and support MPT. These types provide overload for STObject::operator[](), which returns either STAmount or STEitherAmount depending on the field type. This is compile time. For instance, this is going to fail for sfAmount: STAmount const amount = tx[sfAmount]becausesfAmount` supports MPT.

seelabs

I think the approach is fine. 👍 from me. I did not do a detailed review at all, I'm restricted myself to high-level considerations. However, if I did notice anything while I looked at the code I made a note and left a comment. That doesn't mean the comment is important, they are just things I noticed and wanted to point out.

seelabs · 2024-08-22T17:07:57Z

include/xrpl/protocol/MPTIssue.h

+getMPT(uint192 const&);
+
+inline MPT
+badMPT()


It would be better if we didn't need a sentinel value for a bad mpt. Is this really needed?

seelabs · 2024-08-22T17:09:29Z

include/xrpl/protocol/SField.h

@@ -68,6 +68,7 @@ class STCurrency;
    STYPE(STI_UINT128, 4)                         \
    STYPE(STI_UINT256, 5)                         \
    STYPE(STI_AMOUNT, 6)                          \
+    STYPE(STI_EITHER_AMOUNT, 6)                   \


I see this duplicates st amount, and I think I know why it's done. Could we remove STI_AMOUNT?

seelabs · 2024-08-22T17:14:21Z

include/xrpl/protocol/SField.h

@@ -346,7 +376,8 @@ using SF_UINT384 = TypedField<STBitString<384>>;
 using SF_UINT512 = TypedField<STBitString<512>>;

 using SF_ACCOUNT = TypedField<STAccount>;
-using SF_AMOUNT = TypedField<STAmount>;
+using SF_AMOUNT = TypedFieldAmount<SFieldMPT::No>;
+using SF_EITHER_AMOUNT = TypedFieldAmount<SFieldMPT::Yes>;


This seems misleading to me. The template parameter controls whether the type can contain an mpt, but it doesn't necessarily need to hold an mpt - it can still hold an stamount. Maybe rename "Yes" to something else? MaybeMPT or somesuch?

seelabs · 2024-08-22T17:15:30Z

include/xrpl/protocol/STAmount.h

-concept AssetType = std::is_same_v<A, Issue> || std::is_same_v<A, MPT> ||
-    std::is_same_v<A, Asset> || std::is_convertible_v<A, Issue> ||
-    std::is_convertible_v<A, Asset>;
+struct int64_tag_t


Consider narrowing the scope of this declaration.

seelabs · 2024-08-22T17:17:16Z

include/xrpl/protocol/STAmount.h

@@ -49,15 +46,15 @@ concept AssetType = std::is_same_v<A, Issue> || std::is_same_v<A, MPT> ||
 // Wire form:
 // High 8 bits are (offset+142), legal range is, 80 to 22 inclusive
 // Low 56 bits are value, legal range is 10^15 to (10^16 - 1) inclusive
-class STAmount final : public STBase, public CountedObject<STAmount>
+class STAmount final


Since this no longer inherits from STBase consider renaming it.

seelabs · 2024-08-22T17:18:28Z

include/xrpl/protocol/STAmount.h

-        std::uint64_t mantissa = 0,
-        int exponent = 0,
-        bool negative = false);
+    STAmount(std::int64_t mantissa, int64_tag_t);


This needs a comment. I know the existing code is way undercommented, but new code should be better documented. (for this and other changes in this changeset, but this is the first one that caught my eye)

seelabs · 2024-08-22T17:20:06Z

include/xrpl/protocol/STObject.h

@@ -53,12 +54,15 @@ throwFieldNotFound(SField const& field)

 class STObject : public STBase, public CountedObject<STObject>
 {
+    template <typename T, SFieldMPT M>
+    using ValueType = std::conditional<M == SFieldMPT::No, STAmount, T>::type;


It would be better if STObject didn't know anything about STAmount.

seelabs · 2024-08-22T17:24:21Z

include/xrpl/protocol/UintTypes.h

-public:
-    explicit MPTTag() = default;
-};
-


I'll leave a comment here, but around line 58 there's the following code:

/** MPT is a 192-bit hash representing MPTID. */ using MPT = std::pair<std::uint32_t, AccountID>;

For better type safety, consider making a class for MPT. I'm also not sure this is the best place for this declaration.

Also, around line 69 is the following declaration:

MPT const& noMPT();

It would be better if we didn't need these sentinal classes. What is the purpose of this? I don't see it used.

* Replace MPT:pair<uint32,AccountID> with MPTID:uint192 * Remove SFieldMPT * Replated TypedFieldAmount with generic TypedFieldVariant to handle STEitherAmount * Update comments

gregtatcam force-pushed the feature/mpt-v1-eitheramount branch 3 times, most recently from 02213e0 to 8cba0c3 Compare July 27, 2024 01:34

Use STEitherAmount as a variant of STAmount and MPTAmount

c2a7281

gregtatcam force-pushed the feature/mpt-v1-eitheramount branch 7 times, most recently from f1364c9 to 5386172 Compare August 1, 2024 17:15

gregtatcam force-pushed the feature/mpt-v1-eitheramount branch 3 times, most recently from 1b12d9d to 0126da7 Compare August 6, 2024 16:06

gregtatcam added 3 commits August 6, 2024 14:47

Split AMOUNT fields into two types SF_AMOUNT (don't support MPT)

e557a89

and SF_EITHER_AMOUNT (support MPT). Add TypedFieldAmount to declare Amount fields. Extend STObject with getters/setters for SF_AMOUNT fields.

Remove STBase and CountedObject inheritance from STAmount and STMPTAm…

4f6ddfe

…ount

Consolidate errors. Remove redundant checks.

749a80a

gregtatcam force-pushed the feature/mpt-v1-eitheramount branch from 0126da7 to 749a80a Compare August 6, 2024 18:49

shawnxie999 reviewed Aug 8, 2024

View reviewed changes

shawnxie999 reviewed Aug 9, 2024

View reviewed changes

gregtatcam force-pushed the feature/mpt-v1-eitheramount branch from 189362e to 4b2e103 Compare August 13, 2024 15:42

Add invalid MPT amount check in STTx::passesLocalChecks()

5b77635

to provide one single check for all tx. Update unit-tests. Fix payment for unauthorized holder

gregtatcam force-pushed the feature/mpt-v1-eitheramount branch from 4b2e103 to 5b77635 Compare August 13, 2024 15:45

Fix serialization. Extend unit-tests.

39e8906

gregtatcam force-pushed the feature/mpt-v1-eitheramount branch from 9b317f6 to 39e8906 Compare August 14, 2024 18:45

Refactor Payment and Clawback transactors

927abd5

to have separate functions for MPT/IOU handling since there is little overlap between them. Remove the check for DeletableAccounts

gregtatcam force-pushed the feature/mpt-v1-eitheramount branch from fe8e9d2 to 927abd5 Compare August 15, 2024 19:37

shawnxie999 reviewed Aug 16, 2024

View reviewed changes

shawnxie999 reviewed Aug 19, 2024

View reviewed changes

shawnxie999 reviewed Aug 20, 2024

View reviewed changes

Fix Linux build

f39eeb1

seelabs approved these changes Aug 22, 2024

View reviewed changes

gregtatcam added 5 commits August 26, 2024 09:46

Address reviewer's feedback

db124b0

* Replace MPT:pair<uint32,AccountID> with MPTID:uint192 * Remove SFieldMPT * Replated TypedFieldAmount with generic TypedFieldVariant to handle STEitherAmount * Update comments

Merge with latest develop

8ebf5a9

Fix clang-format

496f600

Resolve conflicts with mpt-v1

8298f81

Fix clang-format

632373b

gregtatcam merged commit d2925bd into feature/mpt-v1 Aug 26, 2024
7 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use STEitherAmount as a variant of STAmount and STMPTAmount #30

Use STEitherAmount as a variant of STAmount and STMPTAmount #30

gregtatcam commented Jul 24, 2024

shawnxie999 Aug 8, 2024

gregtatcam Aug 8, 2024

shawnxie999 Aug 8, 2024

gregtatcam Aug 8, 2024

gregtatcam Aug 8, 2024

shawnxie999 Aug 9, 2024

gregtatcam Aug 9, 2024

shawnxie999 Aug 16, 2024

shawnxie999 Aug 16, 2024

shawnxie999 Aug 19, 2024

gregtatcam Aug 19, 2024

shawnxie999 Aug 20, 2024

gregtatcam Aug 20, 2024

shawnxie999 Aug 20, 2024

gregtatcam Aug 20, 2024

seelabs left a comment

seelabs Aug 22, 2024

seelabs Aug 22, 2024

seelabs Aug 22, 2024

seelabs Aug 22, 2024

seelabs Aug 22, 2024

seelabs Aug 22, 2024

seelabs Aug 22, 2024

seelabs Aug 22, 2024

Use STEitherAmount as a variant of STAmount and STMPTAmount #30

Use STEitherAmount as a variant of STAmount and STMPTAmount #30

Conversation

gregtatcam commented Jul 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment