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

Autoswap accounting fixes #NTRN-385 #685

Merged
merged 21 commits into from
Oct 15, 2024
Merged

Autoswap accounting fixes #NTRN-385 #685

merged 21 commits into from
Oct 15, 2024

Conversation

jcompagni10
Copy link
Contributor

@jcompagni10 jcompagni10 commented Aug 20, 2024

This PR fixes 4 issues to related to how autoswap shares are calculated. For broader context see the autoswap spec: https://www.notion.so/hadron/1f17de6d999f4ace9f322262dde11a93?v=5bca99093f134baf9688c9bf12f1aa1c&p=7aca1c1dd1c54ae38202792b79cfe54e&pm=s

Issue 1:
The price (price1To0Center) used for the calculation was incorrect.
Issue 2:
Autoswap shares were being issued at full value instead of being discounted by the normal share value calculation (existingShares / poolValue)
Issue 2.5:
The share value calculation must include the autoswap fee as part of the share value.
Issue 3:
Autoswap calculation does not take into account prices and handles 1 sides pools incorrectly, forcing autoswap amount to be too high. See: https://www.notion.so/Autoswap-Spec-ca5f35a4cd5b4dbf9ae27e0454ddd445?pvs=4#12032ea59b0e802c925efae10c3ca85f

Because of these issues, it is possible that an incorrect number of shares were issued for pool deposits. As of this moment, there are no pools that are affected. But out of caution, there is an upgrade that will withdraw any potentially affected pools.

@jcompagni10 jcompagni10 marked this pull request as ready for review August 22, 2024 20:40
@pr0n00gler
Copy link
Collaborator

Is it ready for review? Cause the description says otherwise

@jcompagni10
Copy link
Contributor Author

Is it ready for review? Cause the description says otherwise

Yes, ready for review

@jcompagni10
Copy link
Contributor Author

x/dex/keeper/pool.go Show resolved Hide resolved
x/dex/migrations/v5/store.go Outdated Show resolved Hide resolved
@jcompagni10
Copy link
Contributor Author

@jcompagni10
Copy link
Contributor Author

pr0n00gler
pr0n00gler previously approved these changes Oct 11, 2024
@jcompagni10
Copy link
Contributor Author

@pr0n00gler pr0n00gler merged commit 32d2e8c into main Oct 15, 2024
7 checks passed
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.

2 participants