Implemented a fix for the share price accuracy issue#723
Merged
jalextowle merged 7 commits intomainfrom Jan 17, 2024
Merged
Conversation
Hyperdrive Gas Benchmark
This comment was automatically generated by workflow using github-action-benchmark. |
Collaborator
50ed94f to
6834758
Compare
jalextowle
commented
Jan 16, 2024
jalextowle
commented
Jan 16, 2024
jrhea
reviewed
Jan 16, 2024
1df8210 to
9a9414c
Compare
jrhea
approved these changes
Jan 16, 2024
Contributor
jrhea
left a comment
There was a problem hiding this comment.
Lgtm. Just a nit in a comment for you to fix.
fwiw, this reduces the delta reported in this issue
from 17386 to 2 wei.
I do have a comment about what you wrote in the PR description:
One potential issue that this introduces is that the amount of shares removed from the yield source may not be exactly equal to the amount of shares removed from (or added to) the pool's share reserves.
We already had this issue where the accounting was off. The way it was before, the calculate proceeds didn't match the amount of proceeds withdrawn from the yield source. This seems to have reduced the discrepancy though.
This file contains hidden or 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
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.
Fixes: #697, #698.
This PR implements a poor man's solution to the problem that we've noticed recently where the base proceeds for longs and shorts closing at maturity are greater by several thousand wei than we'd expect them to be. After closer inspection, the underlying cause of these issues is our use of a
sharePricecomputed astotalAssets.divDown(totalShares). This conflicts with the accounting done by our yield sources where they will convert from shares to assets withshares.mulDivDown(totalAssets, totalShares).One solution to this problem would be to rip out our current use of share price and use
_convertToSharesand_convertToAssetsfunctions tailored to each yield source that could perform amulDivDownif appropriate. The problem with this kind of change is that it is very invasive and would require the reworking of essentially all of our accounting logic. This is how we should think about the problem when we get a chance to re-write Hyperdrive (perhaps in Hyperdrive V2).For now, a more expedient solution is to take the base amount implied by the amount of
_sharespassed into_withdrawas base reality and use this to calculate the amount of shares that need to be removed from the yield source to remove this base amount. Doing the accounting this way ensures that our invariants around the proceeds of matured longs and shorts aren't violated, but it comes with some quirks. One potential issue that this introduces is that the amount of shares removed from the yield source may not be exactly equal to the amount of shares removed from (or added to) the pool's share reserves. In my opinion, this is acceptable since we calculate the share reserves delta consistently using the pool's share price, and the new approach of clamping the proceeds is just an error correction done at the end of the calculation.