Skip to content
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

fix: virtual price rounding error #19

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

AlbertoCentonze
Copy link
Collaborator

@AlbertoCentonze AlbertoCentonze commented Feb 3, 2025

In tweak_price the calculation of the virtual price can be off by 1 due a combination of isqrt + division rounding down.

In particular this line would revert:

assert virtual_price > old_virtual_price # dev: virtual price decreased

Because of this line lacking a + 1 previously:

xcp: uint256 = isqrt(xp[0] * xp[1])
virtual_price = 10**18 * xcp // total_supply + 1

While this issue doesn't seem to have cause issues onchain (probably it doesn't happen when pool is used as intended), this still create issues in testing and could potentially prevent people from withdrawing liquidity/exchanging. For this reason we increase the value of virtual_price by 1e-18 that can be considered a safe negligible amount that can hardly be misused given virtual price has 18 decimals of precision.


This is part 2 of 3 in a stack made with GitButler:

@AlbertoCentonze AlbertoCentonze force-pushed the vp-rounding-fix branch 2 times, most recently from 1242fde to 2d03f1c Compare February 4, 2025 12:28
@AlbertoCentonze AlbertoCentonze changed the title vp-rounding-fix fix: virtual price rounding error Feb 4, 2025
Base automatically changed from bump-vyper to main February 4, 2025 14:46
@AlbertoCentonze AlbertoCentonze marked this pull request as ready for review February 4, 2025 14:55
given the fix in f2ca1be it's now possible to have broader fuzzing in stateful testing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant