Add enforcement of valid integer range to Number #6000
Conversation
ximinez
force-pushed
the
ximinez/lending-number
branch
5 times, most recently
from
805fbc1 to
96fe08c
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #6000 +/- ##
========================================
Coverage 78.6% 78.6%
========================================
Files 818 818
Lines 68953 69105 +152
Branches 8243 8253 +10
========================================
+ Hits 54171 54299 +128
- Misses 14782 14806 +24
... and 5 files with indirect coverage changes 🚀 New features to boost your workflow:
|
ximinez
force-pushed
the
ximinez/lending-number
branch
from
96fe08c to
bd196c7
Compare
ximinez
changed the title
- Change the Number::maxIntValue to all 9's. - Add integral() to Asset (copied from Lending) - Add toNumber() functions to STAmount, MPTAmount, XRPAmount to allow explicit conversions with enforcement options. - Add optional Number::EnforceInteger options to STAmount and STNumber ctors, conversions, etc. IOUs are never checked. - Update Vault transactors, and helper functions, to check restrictions. - Fix and add Vault tests.
ximinez
force-pushed
the
ximinez/lending-number
branch
from
bd196c7 to
0175dd7
Compare
- "valid" means the value is <= Number::maxIntValue, which has been changed to maxMantissa / 100. A valid number could get bigger and be ok - such as when paying late interest on a loan. - "representable" means the value is <= Number::maxMantissa. An unrepresentable number WILL be rounded or truncated. - Adds a fourth level of enforcement: "compatible". It is used for converting XRP to Number (for AMM), and when doing explicit checks. - "weak" will now throw if the number is unrepresentable.
- Turns out that "Proxy::operator->" is not a safe substitute for "Proxy::value()." if the field is not required. The implementation is different such that "operator->" will return a null ptr if the field is not present. This includes default fields with a value of zero!
|
In description, "Before / After" section is missing |
Bronek
left a comment
Bronek
left a comment
There was a problem hiding this comment.
This is very extensive change; I wonder if a simpler alternative is possible ?
include/xrpl/protocol/STAmount.h
Outdated
| // used during automatic conversions to Number. If not set, the default | ||
| // behavior is used. It can also be overridden when coverting by using | ||
| // toNumber(). | ||
| std::optional<Number::EnforceInteger> enforceConversion_; |
There was a problem hiding this comment.
this seems to be the same semantics as simpler:
Number::EnforceInteger enforceConversion_ = Number::EnforceInteger::none;There was a problem hiding this comment.
this seems to be the same semantics as simpler:
Number::EnforceInteger enforceConversion_ = Number::EnforceInteger::none;
It is not, because of the default behavior in STAmount::operator Number(). If enforceConversion_ is unseated, then the default behavior is used - XRP becomes compatible, MPT becomes strong, and IOUs are left alone. If it is set, even if set to none, that conversion is used regardless of the asset type. (See also STAmount::toNumber.)
You could think of the unseated optional as a fifth level of checking called, default, but that doesn't make sense for Number.
include/xrpl/protocol/STAmount.h
Outdated
| enforceConversion_ = enforce; | ||
| if (!enforce) | ||
| { | ||
| // Use the default conversion behavior |
There was a problem hiding this comment.
I don't understand why we need this block. The operator Number does not seem to be doing any useful work when enforceConversion_ is empty (or none)
There was a problem hiding this comment.
I don't understand why we need this block. The
operator Numberdoes not seem to be doing any useful work whenenforceConversion_is empty (ornone)
If the default conversion uses an option that can throw, and the converted Number is invalid, that conversion will throw. Otherwise the result is unneeded. There may be a better way to do it.
| .amount = d.asset(10)}); | ||
| env(tx, ter{tecPRECISION_LOSS}); | ||
| env.close(); | ||
| }); |
There was a problem hiding this comment.
I guess this test means that vaultMaximumIOUScale in Protocol.h should be cut down, perhaps to 15 ? Which will require futher changes in Vault_test.cpp
There was a problem hiding this comment.
I guess this test means that
vaultMaximumIOUScaleinProtocol.hshould be cut down, perhaps to 15 ? Which will require futher changes inVault_test.cpp
Perhaps. The only downside to changing it is that when we have a fuller solution that can represent everything, we'll need to have another value that is amendment-gated. Not a deal breaker, but something to consider. What do you think?
There was a problem hiding this comment.
I think it's fair to make this change to 15 now, and let future amendments worry about themselves
kennyzlei
requested review from
shawnxie999
and removed request for
gregtatcam
| constexpr static rep maxMantissa = minMantissa * 10 - 1; | ||
| static_assert(maxMantissa == 9'999'999'999'999'999LL); | ||
|
|
||
| constexpr static rep maxIntValue = maxMantissa / 100; |
There was a problem hiding this comment.
how was this number chosen? could we have chosen a larger value?
There was a problem hiding this comment.
how was this number chosen? could we have chosen a larger value?
There's a discussion in the comments in https://ripplelabs.atlassian.net/browse/RIPD-4045.
Since that's not public, the tl;dr is that I started with maxMantissa / 10, which would give the STNumber fields in Vault room to grow if they get, for example, extra interest from a special case payment in a Loan. I was concerned that wouldn't be enough overhead. The answer was
Let’s try to go with a safer, conservative limit as in one that reduces the truncation as much as possible
This is intended to be a temporary solution to get the feature available without a major rewrite. A fuller solution should be available in a version or two which allows the entire range of valid numbers.
Updated. |
- Changed the EnforceInteger enum into a bool. - Removed enforcement by throw. - Essentially got rid of the "weak" and "strong" options. - Removed integer options from STAmount. Since there's no throwing, there's no need to override the default.
- STAmount::clear() needs to use 0 exponent for all integral types. - Change vaultMaximumIOUScale to 13. - Fix an dereferenced unseated optional.
|
Some Vault unit-tests should be updated because |
src/libxrpl/basics/Number.cpp
Outdated
| { | ||
| // The strictest setting prevails | ||
| if (!isInteger_) | ||
| isInteger_ = y.isInteger_; |
There was a problem hiding this comment.
This doesn't seem right. Consider this test:
Number i{100, true};
Number a{1, -10, false};
Number b = a + i;
std::cout << i << " " << a << " " << b << " " << b.isInteger() << std::endl;
It outputs:
100 0.0000000001 100.0000000001 1
'b' is integer but it's actually not.
There was a problem hiding this comment.
This doesn't seem right. Consider this test:
[...]
'b' is integer but it's actually not.
This is completely intentional, and is why there are no asserts or throws related to the flag.
What I wrote in Number.h is
// isInteger_ is not serialized, transmitted, or used in
// calculations in any way. It is used only for internal validation
// of integer types. It is a one-way switch. Once it's on, it stays
// on.
I should probably also add that it's transmissible, or "sticky", so that any interaction with an integer will become an integer. The only thing that can strip the integer flag from a Number is converting or assigning it to something other than a Number.
The reason is that the flag does not affect any operations, and can be completely ignored if it's not needed. If it is needed, it would be much worse to strip it off than it would be to add it unnecessarily.
To get a better view of what I'm talking about, it's probably better to look at how the flag is used: The Number::valid() and Number::representable() functions, and the STAmount::validNumber() and STAmount::representableNumber() functions which wrap them.
git grep -w -e "representable()" -e "valid()" -e "validNumber()" -e "representableNumber()"
Notice how they are only called from tests, and specific transactors doing specific tests.
- The limit has been reduced to 13, so 18 is now considered malformed. - Make the test more robust so it doesn't segfault if the vault creation fails.
Two things:
|
- Anything above 13 is _nearly_ unusable, but there might be some edge use cases where it is. - Added unit tests to show this.
- Overflow Number -> int64 if it doesn't fit in the mantissa range. - Only enabled if at least one of the SingleAssetVault or LendingProtocol amendments are enabled. - Will throw the overflow error if the value is larger than maxMantissa. Current behavior is to throw if the value is larger than max int64_t value.
ximinez
force-pushed
the
ximinez/lending-number
branch
from
7698fdc to
82553c2
Compare
- Document why STAmount to Number conversions work in canonicalize(). - Update some unit tests
- Amendment gated on SAV and LP
ximinez
removed
the
DraftRunCI
High Level Overview of Change
Adds a mechanism to check whether an integer
Numbercan be safely and accurately represented..Context of Change
Numberuses anint64internally, which has a maximum value approximately9e18. However, all of it's precision math is done with values below1e16. If an integer value larger than that is converted toNumber, some of the least-significant digits will be dropped. This is unacceptable for situations that need the exact values that integers provide.Fortunately, the only features where this imprecision will matter are either unsupported (
SingleAssetVault) or pending merge (LendingProtocol). Before the introduction of these features, onlyAMMusedNumbers, and it used them to calculate rates, qualities, etc. Exact values are not needed for that.Type of Change
Before / After
Numbermust be less than1e14. This is 1/100th of the computation range so that certain computed fields can go over this value if necessary.Numberas holding an integer. This has no functional implications toNumber, and integerNumberscan still store values well beyond the valid range for integers. Transactions and other functionality can check whether aNumberis valid as part of their validation and transaction processing.Numberis designated as an integer, it can not be undesignated. Any interactions and operations (e.g. assignment, addition) involving any integers will return integer results.XRPAmounts andMPTAmounts converted toNumbers, including viaSTAmount, will be designated as integers.SingleAssetVaulttransactions and operations to check values at the important steps to ensure no precision is lost.Future Tasks
Even though this PR is based on
develop, it is a pre-requisite for #5270, Lending Protocol implementation, which will also need to be updated to use these fields. Other pending PRs that may have consequences from this include #5285, MPT on DEX.