AMM Unit tests: rounding down of equal asset deposit LPToken calculation #4982
Conversation
include reference from XLS-30d. Validate the expected deposit sums against XLS-30d spec
src/test/app/AMM_test.cpp
Outdated
| // testClawback(); | ||
| // testAMMID(); | ||
| // testSelection(); | ||
| // testFixDefaultInnerObj(); |
There was a problem hiding this comment.
Why comment out all of the tests?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4982 +/- ##
=======================================
Coverage 70.9% 70.9%
=======================================
Files 796 796
Lines 66727 66727
Branches 10981 10976 -5
=======================================
+ Hits 47333 47338 +5
+ Misses 19394 19389 -5 see 2 files with indirect coverage changes |
|
This looks good to me. But I was hoping it would hit these lines: https://app.codecov.io/gh/XRPLF/rippled/pull/4982/blob/src/ripple/app/misc/impl/AMMHelpers.cpp#L171. And I don't have a good explanation as to why it didn't. Nevertheless, this looks like a good addition to me. |
|
@HowardHinnant I have updated the numbers so that the execution hits the necessary lines. I expected |
seelabs
requested review from
gregtatcam and
seelabs
and removed request for
gregtatcam
| BEAST_EXPECT(expectLedgerEntryRoot(env, carol, XRP(30'000) - | ||
| depositXRP - txfee(env, 1))); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Maybe my brain isn't working today, but I'm not following how this tests rounding down. Where would the calculation differ if we rounded to nearest, say? Can you add some clarifying comments to help me follow this test?
There was a problem hiding this comment.
The values computed by AMMHelpers.cpp:adjustLPTokens would be different, if we used the nearest rounding mode. I have tagged that line above ^
There was a problem hiding this comment.
I'm still not sure how the test is showing this difference. I think what I'm looking for is a comment in the test that explains how the test proves that the rounding is working as we intended. The test may be doing that, but I'm not sure how. I think I need a comment to explain how the test works.
There was a problem hiding this comment.
Fwiw, I checked the test coverage of AMMHelpers.cpp before and after this PR and this PR improves the coverage in this file, specifically in adjustAmountsByLPTokens.
There was a problem hiding this comment.
thanks howard.
@seelabs I have added a test case and asserts on line 1155 to show the rounding down behavior
| // digits, it is possible that lpTokensActual is rounded down. This is to | ||
| // ensure that we don't create/burn more LPTokens (depending on | ||
| // whether it's AMMDeposit or AMMWithdraw transaction) than what the trader | ||
| // asked for. |
There was a problem hiding this comment.
@seelabs This is one of the points in the codebase where LPTokens are rounded down. Check the function above adjustLPTokens.
Consequently, lpTokensActual < lpTokens by a very small margin.
| BEAST_EXPECT(ammAlice.expectBalances( | ||
| XRP(10'000) + depositXRP, | ||
| USD(10'000) + depositUSD, | ||
| IOUAmount{1, 7} + newLPTokens)); |
There was a problem hiding this comment.
I don't understand how lines 1156 and 1177 can be true at the same. I'm guessing it's due to the loss of precision in the last few digits.
newLPTokens == deltaLPTokens (numerically)
initLPTokens + newLPTokens = finalLPTokens, but finalLPToken - initLPToken < deltaLPTokens
seelabs
added
the
Passed
|
Hello, the commit message could be: |
…#4982) * Specifically, test using tfLPToken flag
High Level Overview of Change
This PR exercises a specific portion of the AMMHelpers.cpp file. It triggers a downward rounding of LPTokens in
adjustLPTokensfunction in theAMMHelpers.cppfile.Context of Change
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)No impact on performance. No change to APIs