Add a new serialized type: STNumber #5121

thejohnfreeman · 2024-09-06T20:01:09Z

This type should let objects and transactions contain multiple fields for quantities of XRP, IOU, or MPT without duplicating information about the "issue" (represented by STIssue). It is a straightforward serialization of our internal Number type that uniformly represents those quantities.

In this change, STNumber is unused anywhere except a couple example tests. I wanted to get a gut-check on the implementation before going much further. Happy to take suggestions for more tests too.

codecov · 2024-09-06T20:23:01Z

Codecov Report

Attention: Patch coverage is 80.00000% with 18 lines in your changes missing coverage. Please review.

Project coverage is 77.9%. Comparing base (0ec17b6) to head (f419c18).
Report is 2 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/libxrpl/protocol/STNumber.cpp	76.5%	8 Missing ⚠️
src/libxrpl/protocol/STObject.cpp	0.0%	5 Missing ⚠️
src/libxrpl/protocol/Serializer.cpp	87.5%	2 Missing ⚠️
include/xrpl/protocol/STNumber.h	66.7%	1 Missing ⚠️
src/xrpld/app/tx/detail/Payment.cpp	88.9%	1 Missing ⚠️
src/xrpld/ledger/detail/CachedView.cpp	80.0%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5121     +/-   ##
=========================================
- Coverage     77.9%   77.9%   -0.0%     
=========================================
  Files          782     784      +2     
  Lines        66622   66680     +58     
  Branches      8136    8138      +2     
=========================================
+ Hits         51902   51942     +40     
- Misses       14720   14738     +18

Files with missing lines	Coverage Δ
include/xrpl/basics/Number.h	`100.0% <ø> (ø)`
include/xrpl/protocol/SField.h	`100.0% <ø> (ø)`
include/xrpl/protocol/STObject.h	`92.9% <ø> (ø)`
include/xrpl/protocol/Serializer.h	`94.8% <100.0%> (+1.0%)`	⬆️
src/libxrpl/protocol/STVar.cpp	`97.3% <ø> (ø)`
include/xrpl/protocol/STNumber.h	`66.7% <66.7%> (ø)`
src/xrpld/app/tx/detail/Payment.cpp	`91.1% <88.9%> (ø)`
src/xrpld/ledger/detail/CachedView.cpp	`94.4% <80.0%> (+5.3%)`	⬆️
src/libxrpl/protocol/Serializer.cpp	`85.7% <87.5%> (-0.9%)`	⬇️
src/libxrpl/protocol/STObject.cpp	`87.4% <0.0%> (-1.1%)`	⬇️
... and 1 more

... and 4 files with indirect coverage changes

dangell7 · 2024-09-09T08:58:02Z

I like this. I had all sorts of fun when doing sfQuantity on the options proposal. I think I just ended up using an i32. Some things I would be curious about.

Will there be a getFieldNumber(sfQuantity)? (Assuming yes)
Also how can one multiply / divide / add / subtract an STNumber with an STAmount. That might be good to show in tests if its not straight forward. For example I have 100 USD and I want to multiply by 100.

I ended up with STAmount const totalValue = STAmount(strikePrice.issue(), (strikePrice.mantissa() * quantity)); but maybe I took a wrong turn.

Thank you

src/libxrpl/protocol/Serializer.cpp

include/xrpl/protocol/STNumber.h

src/test/protocol/STNumber_test.cpp

thejohnfreeman · 2024-09-24T15:39:34Z

@dangell7 I added getFieldNumber. I generally add methods as-needed, and the getField* methods seem incomplete (there is no getFieldIssue, for example). Let me know if there are others you want.

I added a test to demonstrate multiplication. The general pattern is to convert everything to Number, compute your arithmetic, and then reconstruct final types like STAmount. We can add arithmetic helpers for those final types where it makes sense. I'm a little disappointed that conversion from STAmount to Number must first pass through constructing either an XRPAmount or an IOUAmount, though I guess it is inlined.

src/libxrpl/protocol/Serializer.cpp

gregtatcam · 2024-10-10T15:18:51Z

src/libxrpl/protocol/Serializer.cpp

@@ -71,6 +72,32 @@ Serializer::add64(std::uint64_t i)
    return ret;
 }

+int
+Serializer::addi32(std::int32_t i)
+{


Consider adding a private common (template?) function for 32 and 64 bit serialization. They are practically the same except for the most significant byte handling.

include/xrpl/protocol/STNumber.h

gregtatcam · 2024-10-10T15:21:37Z

include/xrpl/protocol/STNumber.h

+#include <xrpl/protocol/STBase.h>
+
+namespace ripple {
+


Please add a brief description of the class.

include/xrpl/protocol/STNumber.h

gregtatcam · 2024-10-10T16:08:07Z

src/test/protocol/Serializer_test.cpp

+    void
+    run() override
+    {
+        std::initializer_list<std::int64_t> const values = {


Does it also make sense to add int32 tests?

gregtatcam · 2024-10-10T16:09:02Z

src/test/protocol/STNumber_test.cpp

+            BEAST_EXPECT(stnum.value() == Number{0});
+        }
+
+        std::initializer_list<std::int64_t> const values = {


Should add int32 tests?

gregtatcam · 2024-10-10T16:14:34Z

src/test/protocol/STNumber_test.cpp

+            IOUAmount totalValue{iouValue * factor};
+            STAmount const totalAmount{totalValue, strikePrice.issue()};
+            BEAST_EXPECT(totalAmount == Number{10'000});
+        }


Should we test arithmetics with some large/small fractional values to make sure STAmount and STNumber produce the same results?

I welcome all tests written by others, but I don't want to spend my time designing those tests right now.

src/libxrpl/protocol/STNumber.cpp

gregtatcam

👍 LGTM
I left a couple of minor comments to consider.

gregtatcam · 2024-10-15T23:28:07Z

include/xrpl/protocol/Serializer.h

-    add32(std::uint32_t i);  // ledger indexes, account sequence, timestamps
+
+    template <typename T>
+    requires(sizeof(T) == sizeof(std::uint32_t)) int add32(T i)


Wouldn't something like this be more accurate?

requires(std::is_same_v<std::make_unsigned_t<T>, std::uint32_t>)

gregtatcam · 2024-10-15T23:28:10Z

include/xrpl/protocol/Serializer.h

-    add64(std::uint64_t i);  // native currency amounts
+
+    template <typename T>
+    requires(sizeof(T) == sizeof(std::uint64_t)) int add64(T i)


Similarly to above:

requires(std::is_same_v<std::make_unsigned_t<T>, std::uint64_t>)

HowardHinnant · 2024-10-24T18:24:32Z

One thing that bothers me is the ability to destruct a STNumber via a reference or pointer to a Number. This would call Number's non-virtual destructor and not call STNumber's destructor.

That error can be prevented at compile-time with this refactor: HowardHinnant@05ecfc6

Only very lightly tested. So if accepted, this does need further scrutiny.

gregtatcam · 2024-10-24T18:51:26Z

One thing that bothers me is the ability to destruct a STNumber via a reference or pointer to a Number. This would call Number's non-virtual destructor and not call STNumber's destructor.

That error can be prevented at compile-time with this refactor: HowardHinnant@05ecfc6

Only very lightly tested. So if accepted, this does need further scrutiny.

@HowardHinnant Why not adding a virtual destructor? If STNumber is a subclass of Number then the math just works.

HowardHinnant · 2024-10-25T02:47:20Z

@HowardHinnant Why not adding a virtual destructor? If STNumber is a subclass of Number then the math just works.

The math also just works with the way I refactored it. Note that I did not have to change any use cases of STNumber, in code or in tests.

Adding a virtual destructor to Number adds an expense to every client of Number whether they use it or not. My refactor adds no expense to Number at all. It does not touch the declaration or implementation of Number. That makes it a cleaner separation of duties. And the resulting API that clients see is identical, minus the ability to implicitly convert STNumber* to Number* (and STNumber& to Number&), which is both unneeded and undesirable.

ximinez · 2024-11-19T21:00:18Z

@thejohnfreeman Could you provide a commit message for when this is squashed and merged?

thejohnfreeman · 2024-11-19T22:14:53Z

Add a new serialized type: STNumber (#5121)

`STNumber` lets objects and transactions contain multiple fields for
quantities of XRP, IOU, or MPT without duplicating information about the
"issue" (represented by `STIssue`). It is a straightforward serialization of
the `Number` type that uniformly represents those quantities.

`STNumber` lets objects and transactions contain multiple fields for quantities of XRP, IOU, or MPT without duplicating information about the "issue" (represented by `STIssue`). It is a straightforward serialization of the `Number` type that uniformly represents those quantities. --------- Co-authored-by: John Freeman <jfreeman08@gmail.com> Co-authored-by: Howard Hinnant <howard.hinnant@gmail.com>

thejohnfreeman requested review from gregtatcam and HowardHinnant September 6, 2024 20:01

gregtatcam reviewed Sep 9, 2024

View reviewed changes

src/libxrpl/protocol/Serializer.cpp Outdated Show resolved Hide resolved

include/xrpl/protocol/STNumber.h Outdated Show resolved Hide resolved

include/xrpl/protocol/STNumber.h Outdated Show resolved Hide resolved

src/test/protocol/STNumber_test.cpp Show resolved Hide resolved

mDuo13 mentioned this pull request Sep 19, 2024

Change UInt64-Base10 ripple/opensource.ripple.com#99

Merged

thejohnfreeman mentioned this pull request Sep 24, 2024

feature: Single Asset Vault #5147

Closed

gregtatcam reviewed Oct 10, 2024

View reviewed changes

gregtatcam approved these changes Oct 15, 2024

View reviewed changes

thejohnfreeman mentioned this pull request Oct 17, 2024

[feature] Single Asset Vault thejohnfreeman/rippled#10

Draft

HowardHinnant approved these changes Nov 5, 2024

View reviewed changes

thejohnfreeman added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Nov 5, 2024

ximinez added the Blocked label Nov 12, 2024

intelliot removed the Blocked label Nov 25, 2024

XRPLF deleted a comment from ximinez Nov 25, 2024

intelliot force-pushed the stnumber branch from 9f3b082 to f419c18 Compare November 25, 2024 23:03

intelliot merged commit f419c18 into XRPLF:develop Nov 25, 2024
18 checks passed

thejohnfreeman deleted the stnumber branch November 26, 2024 00:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a new serialized type: STNumber #5121

Add a new serialized type: STNumber #5121

thejohnfreeman commented Sep 6, 2024 •

edited

Loading

codecov bot commented Sep 6, 2024 •

edited

Loading

dangell7 commented Sep 9, 2024

thejohnfreeman commented Sep 24, 2024

gregtatcam Oct 10, 2024

gregtatcam Oct 10, 2024

gregtatcam Oct 10, 2024

gregtatcam Oct 10, 2024

gregtatcam Oct 10, 2024

thejohnfreeman Oct 15, 2024

gregtatcam left a comment

gregtatcam Oct 15, 2024

gregtatcam Oct 15, 2024

HowardHinnant commented Oct 24, 2024

gregtatcam commented Oct 24, 2024

HowardHinnant commented Oct 25, 2024

ximinez commented Nov 19, 2024

thejohnfreeman commented Nov 19, 2024

		#include <xrpl/protocol/STBase.h>

		namespace ripple {

Add a new serialized type: STNumber #5121

Add a new serialized type: STNumber #5121

Conversation

thejohnfreeman commented Sep 6, 2024 • edited Loading

codecov bot commented Sep 6, 2024 • edited Loading

Codecov Report

dangell7 commented Sep 9, 2024

thejohnfreeman commented Sep 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

gregtatcam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HowardHinnant commented Oct 24, 2024

gregtatcam commented Oct 24, 2024

HowardHinnant commented Oct 25, 2024

ximinez commented Nov 19, 2024

thejohnfreeman commented Nov 19, 2024

thejohnfreeman commented Sep 6, 2024 •

edited

Loading

codecov bot commented Sep 6, 2024 •

edited

Loading