Add a new serialized type: STNumber #5121
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5121 +/- ##
=========================================
- Coverage 77.5% 77.5% -0.0%
=========================================
Files 777 779 +2
Lines 65831 65890 +59
Branches 8184 8186 +2
=========================================
+ Hits 51026 51071 +45
- Misses 14805 14819 +14
... and 4 files with indirect coverage changes |
|
I like this. I had all sorts of fun when doing
I ended up with Thank you |
|
@dangell7 I added I added a test to demonstrate multiplication. The general pattern is to convert everything to |
src/libxrpl/protocol/Serializer.cpp
Outdated
| @@ -71,6 +72,32 @@ Serializer::add64(std::uint64_t i) | |||
| return ret; | |||
| } | |||
|
|
|||
| int | |||
| Serializer::addi32(std::int32_t i) | |||
| { | |||
There was a problem hiding this comment.
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/STBase.h> | ||
|
|
||
| namespace ripple { | ||
|
|
There was a problem hiding this comment.
Please add a brief description of the class.
| void | ||
| run() override | ||
| { | ||
| std::initializer_list<std::int64_t> const values = { |
There was a problem hiding this comment.
Does it also make sense to add int32 tests?
src/test/protocol/STNumber_test.cpp
Outdated
| BEAST_EXPECT(stnum.value() == Number{0}); | ||
| } | ||
|
|
||
| std::initializer_list<std::int64_t> const values = { |
There was a problem hiding this comment.
Should add int32 tests?
| IOUAmount totalValue{iouValue * factor}; | ||
| STAmount const totalAmount{totalValue, strikePrice.issue()}; | ||
| BEAST_EXPECT(totalAmount == Number{10'000}); | ||
| } |
There was a problem hiding this comment.
Should we test arithmetics with some large/small fractional values to make sure STAmount and STNumber produce the same results?
There was a problem hiding this comment.
I welcome all tests written by others, but I don't want to spend my time designing those tests right now.
include/xrpl/protocol/Serializer.h
Outdated
| add32(std::uint32_t i); // ledger indexes, account sequence, timestamps | ||
|
|
||
| template <typename T> | ||
| requires(sizeof(T) == sizeof(std::uint32_t)) int add32(T i) |
There was a problem hiding this comment.
Wouldn't something like this be more accurate?
requires(std::is_same_v<std::make_unsigned_t<T>, std::uint32_t>)
include/xrpl/protocol/Serializer.h
Outdated
| add64(std::uint64_t i); // native currency amounts | ||
|
|
||
| template <typename T> | ||
| requires(sizeof(T) == sizeof(std::uint64_t)) int add64(T i) |
There was a problem hiding this comment.
Similarly to above:
requires(std::is_same_v<std::make_unsigned_t<T>, std::uint64_t>)
|
One thing that bothers me is the ability to destruct a 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 |
The math also just works with the way I refactored it. Note that I did not have to change any use cases of Adding a virtual destructor to |
* This prevents code trying to delete a STNumber via a pointer to the base class using a non-virtual destructor.
thejohnfreeman
added
the
Passed
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 internalNumbertype that uniformly represents those quantities.In this change,
STNumberis 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.