Add a new serialized type: STNumber

[thejohnfreeman]

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[bot]]

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 :warning:
src/libxrpl/protocol/STObject.cpp	0.0%	5 Missing :warning:
src/libxrpl/protocol/Serializer.cpp	87.5%	2 Missing :warning:
include/xrpl/protocol/STNumber.h	66.7%	1 Missing :warning:
src/xrpld/app/tx/detail/Payment.cpp	88.9%	1 Missing :warning:
src/xrpld/ledger/detail/CachedView.cpp	80.0%	1 Missing :warning:

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%)	:arrow_up:
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%)	:arrow_up:
src/libxrpl/protocol/Serializer.cpp	85.7% <87.5%> (-0.9%)	:arrow_down:
src/libxrpl/protocol/STObject.cpp	87.4% <0.0%> (-1.1%)	:arrow_down:
... and 1 more

... and 4 files with indirect coverage changes

[dangell7]

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.

1. Will there be a getFieldNumber(sfQuantity)? (Assuming yes)
2. 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

[thejohnfreeman]

@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.

[HowardHinnant]

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: https://github.com/HowardHinnant/rippled/commit/05ecfc6ef279475d695b065a35ae82476ba65f13

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

[gregtatcam]

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]

@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]

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

[thejohnfreeman]

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.