-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Expand Number to support the full integer range #6025
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: ximinez/lending-XLS-66-ongoing
Are you sure you want to change the base?
Expand Number to support the full integer range #6025
Conversation
- Update the conversion points between Number and *Amount & STNumber. - Tests probably don't pass.
- Track down and fix edge cases. - Some refactoring and renaming for clarity and simplicity
5cd623f to
4abb6d9
Compare
- Field will be absent in RPC results instead of returning 0.
…number-simple * upstream/develop: chore: Clean up incorrect comments (6031) refactor: Retire MultiSignReserve and ExpandedSignerList amendments (5981)
- Update tests. Unfinished. - TODO: Finish Number tests. Use both modes for STNumber tests. Move mantissa_scale into MantissaRange.
- Fix cross-compiler build issues
b1079c5 to
9310991
Compare
- Nothing really needed to be changed in the tests, but I added a couple of test cases for the min and max int64.
b17225c to
c9ad49f
Compare
- Added test cases min int64. - Updated numberFromJson range checking to use the larger range available from Number.
- Default Number outside of transaction processing to be "large" so RPC will work.
470c9c3 to
248d267
Compare
src/xrpld/app/tx/detail/LoanPay.cpp
Outdated
| { | ||
| // LCOV_EXCL_START | ||
| [[maybe_unused]] | ||
| auto const available = *assetsAvailableProxy; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are available and total needed? They are not used.
| sleBroker->at(sfCoverAvailable) -= clawAmount; | ||
| view().update(sleBroker); | ||
|
|
||
| associateAsset(*sleBroker, vaultAsset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I keep on thinking if there is a way to make this association table driven. There is already a new field property defined sMD_NeedsAsset. What if instead adding this property in sfields, it's added in ledger_entries. Then we just need a way to point it to the associated asset, which requires accessing up to two linked objects right now. What if we add an integral flag to each related ledger entry (Vault, LoanBroker, Loan) to indicate if an asset is integral? Then in STObject::applyTemplate(), which iterates over all fields , for any STNumber field with sMD_NeedsAsset, we can set the integral flag. Does it make sense to explore this approach?
451b474 to
565e62f
Compare
…nto ximinez/lending-number-simple * XRPLF/ximinez/lending-XLS-66-ongoing: Fix Overpayment ValueChange calculation in Lending Protocol (6114) Ensure vault asset cap is not exceeded (6124) Disallow pseudo accounts to be Destination for LoanBrokerCoverWithdraw (6106) Check permissions in LoanSet and LoanPay (6108) Fix some minor bugs in Lending Protocol (6101) Fix LoanBrokerSet debtMaximum limits (6116)
- Prompted by review feedback from @gregtatcam. - Should be pretty thorough. - Tweaked how the MantissaRange class is instantiated, and added a couple more static_asserts.
8c64508 to
16ab78d
Compare
gregtatcam
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks for adding extensive comments to the Number and MantissaRange classes.
Please add some documentation to STTakesAsset.h explaining why it's needed and how to use it. I'll approve once those are in!
* Make `maxRep` public. * Document why template normalize takes min/max params. * Add a check to normalizeToRange to ensure there are no signed/unsigned conversion issues. * static_assert that maxRep is >= maxMPTokenAmount and INITIAL_XRP at their definition points. If something ever changes so that it's not, we'll know immediately.
The updated class documentation now explicitly mentions that the limits for I've added a couple of |
- Remove redundant "else" because of early return. - Add a concept "Integral64" to use in the template parameter for normalizeToRange. - Add documentation to STTakesAsset, associateAsset, and add explanation for how `STNumber` uses it. - Assert that `numberToJson` is always outside of a Transaction context. - Remove copy of `InitialFibSeqPct` and the assert in `Number::operator*=` that prompted me to add it. - Add amendment gating to `STAmount::operator=`
Supersedes #6000.
High Level Overview of Change
Context of Change
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)