-
Notifications
You must be signed in to change notification settings - Fork 607
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
Gamm stableswap improvements #3839
Gamm stableswap improvements #3839
Conversation
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.
LGTM, thanks for the cleanup!
LGTM! I had previously suggested backporting this. Even though this looks like it should be fine, changing to no backport just to be abundandantly cautious with the division by zero change. (I'm pretty sure it should be impossible to get a zero in current code) |
* Stableswap: README pseudocode fixes * Stableswap: Binary search code improvement * Stableswap: minor code improvements * Stableswap: minor code improvements 2 * Binary search: Check potential division by zero (cherry picked from commit 2ac5d35) # Conflicts: # x/gamm/pool-models/stableswap/util_test.go
* Gamm stableswap improvements (#3839) * Stableswap: README pseudocode fixes * Stableswap: Binary search code improvement * Stableswap: minor code improvements * Stableswap: minor code improvements 2 * Binary search: Check potential division by zero (cherry picked from commit 2ac5d35) # Conflicts: # x/gamm/pool-models/stableswap/util_test.go * Fix conflict * Update osmomath go mod Co-authored-by: Aleksandar Ljahović <[email protected]> Co-authored-by: Dev Ojha <[email protected]>
What is the purpose of the change
This PR is created during the Informal Systems audit, after analysis of the existing specification and code inspection
Auditing is performed on commit hash: 7374795
This PR contains minor Stableswap code improvements within the GAMM module noticed during code inspection with the aim of finding issues.
Brief Changelog
getLiquidityIndexMap
andGetScalingFactorByLiquidityIndex
functions are replaced by theGetScalingFactorByDenom
function to get the appropriate scaling factor faster and avoid creating a map. While implementing the new function, we didn't want to change logic, so it doesn't return an error or panic if no scaling factor is found.applyScalingFactorMultiplier
function is protected against unwanted overflow in case multiplier > 1BinarySearchBigDec
,BinarySearch
(osmoutils/binary_search.go) andjoinPoolSharesInternal
(stableswap/amm.go) functions.iterKCalculator
function that always returned nil.Compare
andCompareBigDec
functions.Testing and Verifying
GetScalingFactorByDenom
function.Documentation and Release Note
Unreleased
section inCHANGELOG.md
? no