-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add rounding to Vault invariants #6217
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: develop
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6217 +/- ##
=======================================
Coverage 79.4% 79.4%
=======================================
Files 839 839
Lines 71619 71633 +14
Branches 8236 8231 -5
=======================================
+ Hits 56849 56872 +23
+ Misses 14770 14761 -9
🚀 New features to boost your workflow:
|
a7e80dd to
6b5618e
Compare
6b5618e to
9235ec4
Compare
- Requires a template for STAmount and Asset. - Update tests and computeMinScale from #6217 to use scale. - Convert a few other places to use "scale" correctly.
- Requires a template for STAmount and Asset. - Update tests and computeMinScale from #6217 to use scale. - Convert a few other places to use "scale" correctly.
ximinez
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.
I created #6246 to make the some of the changes I requested below. Pretty much everything except the changes to the actual ValidVault invariant checks.
| auto const vaultDeltaAssets = | ||
| roundToAsset(vaultAsset, *maybeVaultDeltaAssets, minScale); | ||
|
|
||
| if (vaultDeltaAssets > tx[sfAmount]) |
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.
If this is using "round to nearest", it could round vaultDeltaAssets up while tx[sfAmount] doesn't, and yet still be valid. Maybe you should round both?
| if (value == beast::zero || asset.integral()) | ||
| return 0; |
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.
If the Number series values are particularly small, and has one 0, this will throw the rounding off. At an extreme, all the values in the caller could round to 0. I suggest that if value is 0, return value.exponent() (which is the minimum int value) instead. That will guarantee that the result of max_element will represent the largest non-zero exponent.
| if (value == beast::zero || asset.integral()) | ||
| return 0; | ||
|
|
||
| auto mantissa = std::abs(value.mantissa()); |
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 don't think taking the absolute value will make any difference. You're only dividing.
| // Remove trailing zeros from mantissa, adjusting scale accordingly | ||
| while (mantissa % 10 == 0) | ||
| { | ||
| mantissa /= 10; | ||
| ++scale; | ||
| } |
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 don't think this is right. Let's take an example:
- 1.00000000 is going to have a mantissa of 1e18 and an exponent of -18.
- After this loop,
mantissawill be 1, and scale will be 0, so it returns 0.
- After this loop,
- Then 1.23456789 will return -8.
max_elementwill return the 0.
Later, roundToAsset, will round both of them to 1. They would look equal when they are definitely not equal.
The canonical way to correctly get the scale is to make an STNumber, and get its exponent.
src/test/app/Invariants_test.cpp
Outdated
| { | ||
| .name = "Mixed scales", | ||
| .expectedMinScale = -2, | ||
| .values = {Number{1, -2}, Number{5, -3}, Number{3, -2}}, | ||
| }, |
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.
Another case
{
.name = "Mixed mantissa sizes",
.expectedMinScale = X,
.values = {Number{1}, Number{1234, -3}, Number{12345, -6}, Number{123, 1}},
}
6f9b206 to
c6821ab
Compare
|
@ximinez apologies for the forced push, I haven't seen your review! |
|
@ximinez as we discussed, I rewrote the Invariant to check relative distance between values within some tolerance. For IOU the tolerance is 1 * 10^-13, for MPT/XRP the tolerance is zero. The IOU tolerance is the smallest tolerance that would cause the unit test to pass. If the tolerance was -14, the unit-test would fail. |
| afterVault.assetsAvailable) | ||
| auto const assetsTotalDelta = | ||
| afterVault.assetsTotal - beforeVault.assetsTotal; | ||
| if (assetsTotalDelta != vaultDeltaAssets) |
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 is this first if needed?
This PR relaxes Vault Deposit, Withdraw and Clawback invariants for IOUs to allow minimal discrepancy between various balance changes.
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)