fix long_trade_needed_given_reserves and reserves_given_rate_ignoring_exposure
#180
Conversation
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
dpaiton
changed the title
long_trade_needed_given_reserves and reserves_given_rate_ignoring_exposure
dpaiton
force-pushed
the
dpaiton/targeted-long-fix
branch
2 times, most recently
from
94bae7c to
1e31fd5
Compare
ryangoree
approved these changes
Jul 22, 2024
dpaiton
force-pushed
the
dpaiton/targeted-long-fix
branch
2 times, most recently
from
828d34a to
60050f3
Compare
dpaiton
force-pushed
the
dpaiton/targeted-long-fix
branch
from
95e96fb to
f838c22
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Resolved Issues
working towards #21
Description
In #167 I wrote a test for
long_trade_needed_given_reservesandreserves_given_rate_ignoring_exposure, which we will need to translate to the short variety for the targeted short flow. For the tests to pass I had to set fees, zeta, and exposure to zero. This was because the test itself used other components that required this, although the function of interest (long_trade_needed_given_reserves) does not have the same requirements.In this PR I rewrote the test to be simpler and more direct, and found that the function was not passing with any sort of reasonable tolerance. I figured out the bug (we were not accounting for fees correctly) and fixed it.
Part of the fix for
long_trade_needed_given_reservesincluded adding a sanity check on reserve levels, which revealed a bug inreserves_given_rate_ignoring_exposure, which was returning invalid reserve amounts due to non-zero exposure or zeta. Adding zeta to the equation fixed that problem, so all tests are passing again.I had to increase the tolerance for the targeted long test. I'm not entirely sure why, but I think it's still reasonable so I will not investigate it further at this time.