-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
AMM Unit tests: rounding down of equal asset deposit LPToken calculation #4982
Merged
Merged
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
e734403
AMM Unit tests: rounding down of equal asset deposit LPToken calculation
ckeshava 7542e72
simplify the numbers used in unit test
ckeshava 2db3500
fix clang format
ckeshava 16a6515
Enable all unit tests
ckeshava bf1712a
Merge branch 'develop' of https://github.com/XRPLF/rippled into rd
ckeshava 015c5cd
Merge branch 'rd' of https://github.com/ckeshava/rippled into rd
ckeshava 0d357c9
Rectify numbers to trigger rounding down
ckeshava 31b8971
remove an inaccurate comment
ckeshava 5302b30
explanatory comment to indicate the rounding down of LPTokens and amo…
ckeshava 13244e8
[FOLD] include an assert check to validate rounding down behavior
ckeshava 7e9f708
Merge branch 'develop' into rd
ximinez 8f7dbfb
Merge branch 'develop' into rd
ximinez File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks howard.
@seelabs I have added a test case and asserts on line 1155 to show the rounding down behavior