-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add canonical "scale" computation to Number #6246
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: tapanito/lending-vault-invariant
Are you sure you want to change the base?
Conversation
- 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.
3bff525 to
7ab9709
Compare
|
I don’t believe this approach addresses the underlying issue. After tracing the execution and considering the edge cases, there are three main reasons why this fix falls short. In Trace Example:
When we call STAmount const ret{asset, value};
// Value converts from (Exp: -15) to (Exp: -12)
// Example: 4999509900990000000, -15 -> 4999509900990000, -12
if (ret.integral()) return ret;
return roundToScale(ret, scale);
// We are passing an Exp -12 value to be rounded to scale -12.
// No rounding occurs, but normalization returns the original Exp -15 representation.Even though we passed a As you rightfully pointed out in my PR, we cannot blindly round to the smallest scale:
This creates an artificial equality that shouldn't exist. Rounding to the lowest common scale is too destructive for invariant checks. Instead of trying to force the exponents to match via rounding, we should implement an epsilon difference tolerance. This acknowledges that while the numbers are represented differently in memory, they are "equal enough" within a defined precision threshold. |
|
@ximinez, I added the unit test to my branch, then locally merged by branch into yours causing the invariant violation unit-test to fail. |
Ah! You're taking the scale of the deltas. You need to take the scale of the original values - the before and afters that were used to compute the deltas. Those tell you how many digits of the resulting delta are actually valid for comparison. The problem is that in the current implementation, you don't know all of the relevant values until well after you've computed all the deltas. Right now, |
… into ximinez/number-scale * XRPLF/tapanito/lending-vault-invariant: flyby change removing unused includes addreses review comments adds invariant test
… into ximinez/number-scale * XRPLF/tapanito/lending-vault-invariant: refactors vault invariant to use relative distance Limit reply size on `TMGetObjectByHash` queries (6110) ci: remove 'master' branch as a trigger (6234) Improve ledger_entry lookups for fee, amendments, NUNL, and hashes (5644)
|
Hi @ximinez , VaultClawback unit tests are failing with on some instances. |
| if (sign != 0) | ||
| deltas_[key] = balanceDelta * sign; | ||
| { | ||
| balanceDelta.delta *= sign; |
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.
Since we're initializing the DeltaInfo with a scale that is outside of a valid range, we should add an assertion here to ensure that scale was initialized.
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.
Since we're initializing the DeltaInfo with a scale that is outside of a valid range, we should add an assertion here to ensure that scale was initialized.
I might change scale to optional.
Co-authored-by: Vito Tumas <5780819+Tapanito@users.noreply.github.com>
High Level Overview of Change
Introduces a member function to
Numberfor scale computations, so we can be sure it's done the same everywhere.Context of Change
#6217
Type of Change