[stableswap]: simplify CFMM solver #2697

AlpinYukseloglu · 2022-09-11T14:26:05Z

Closes: #2696

What is the purpose of the change

This PR implements a drastically simplified solver for our two-asset stableswap CFMM.

Key differences:

Fits on a single screen (core solver logic is ~5 lines)
Far fewer square root, product, and power terms
Largest power is same as previous (8)
No negative numbers in intermediate calculations
Much easier to parse visually

Brief Changelog

I derived this solver manually, but we can confirm on Desmos that it fits our curve exactly: https://www.desmos.com/calculator/wxl9ffxofw

As described in the issue, the key simplifcations leaned on the following assumptions:

For any given swap, everything except for the amount of the output asset is known & constant
The number of assets in the pool are always greater than zero (let's us use the original CFMM as part of our solver)
Finding the final asset reserve amounts is as good as finding the output amount since we can subtract the original reserves at the end

Pseudocode for new solver:

func solveCfmm(xReserve, yReserve, yIn) {
    Calculate k using xReserve and yReserve

    yReserve = yReserve + yIn
    
    Use new yReserve to solve for new xReserve (using new solver derived from xy(x^2 + y^2) = k)

    xOut = original xReserve minus resulting output

    Return xOut
}

Testing and Verifying

Basic tests pass, and the ones that failed on our original solver are around 3x closer to the target with this solver (likely due to fewer terms leading to fewer approximations).

Still working on more comprehensive tests, although we might first need to solve general precision issues around our CFMM.

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? (yes / no)
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes / no)
How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

ValarDragon · 2022-09-12T10:00:19Z

Ah great idea, there was no need to keep b as a variable in the prior equations!

Nice job, this is a big improvement ;)

x/gamm/pool-models/stableswap/amm.go

Co-authored-by: Dev Ojha <[email protected]>

…s/osmosis into simplify-cfmm-solver

AlpinYukseloglu · 2022-09-12T22:59:32Z

Recent commit cuts the exponents of the two largest terms in half (x^8 -> x^4 and k^2 -> k)

This makes meaningful progress towards #2701, but we still overflow at much smaller asset amounts than we expect to be able to accommodate (current capacity is just under $10m per asset in a 2-asset pool, and we need to be able to support $100m per asset in a 3+ asset pool)

Reference to verify new implementation: https://www.desmos.com/calculator/bx5m5wpind

The last idea/trick I have in mind is potentially dropping the +1s given the size of pools we're dealing with (maybe have two solvers and switch to the other one when x, y, or k is sufficiently large)

Ultimately we'll want to use our binary search solver, but I believe it's important to have a comprehensive and simple reference solver to benchmark against in the meantime

ValarDragon · 2022-09-14T09:11:47Z

nice, this is great! Big win!

(The largest term is x^6 still tho =p, as k = O(x^4), and we have an x^2 9 k term)

Heres an equation switch inspired by how your doing this, basically moving (x^2k)^{1/3} to instead be x (k/x)^{1/3}, which then truly lowers the largest term to x^4 as far as I can tell!

https://www.desmos.com/calculator/lww58igxe8

And actually if you unwrap k in those equations, by making k' = k/x = y(x^2 + y^2), and computing off of k' directly everywhere, you'd actually be at O(x^3) everywhere AFAICT! (This is inspired by noticing that the x^4 terms were all cancellable as they're of the form k/x or x^4 / k

AlpinYukseloglu · 2022-09-14T10:40:35Z

This is awesome! Really good point with k being O(x^4) – does this mean as long as we don't unwrap k the best we can do is O(x^4)?

In any case, what do you think about this simplification? (no massive denominators and only two unique calculations involving variables):

If we're happy with directly computing k in the way you described, this one might just leave us with an extremely simple equation that also falls within O(x^3)

Desmos: https://www.desmos.com/calculator/ktdvu7tdxv

ValarDragon · 2022-09-14T12:59:31Z

That looks great to me!

ValarDragon · 2022-09-20T10:07:57Z

x/gamm/pool-models/stableswap/amm.go

+// So we want to solve for a given addition of `b` units of y into the pool,
+// how many units `a` of x do we get out.
+// Let y' = y + b
+// we solve k = (x'y')(x'^2 + y^2) for x', using the following equation {wolfram alpha link}


Needs wolfram alpha link, or equation we test and explanation of correctness

I put a wolfram alpha link of the original equation, a desmos link for our simplified one, and direct equation + our key abstractions – let me know what you think!

ValarDragon · 2022-09-20T10:08:52Z

x/gamm/pool-models/stableswap/amm.go

+	large_term := (y4.Quo(k)).Mul(osmomath.NewBigDec(2).Quo(twentySevenRootTwo))
+	large_term2 := large_term.Mul(large_term)
+
+	// solve for new xReserve using new yReserve and old k using a solver derived from xy(x^2 + y^2) = k
+	sqrt_term, err := (osmomath.OneDec().Add(large_term2)).ApproxRoot(2)
+	if err != nil {
+		panic(err)
+	}


Lets take the equation we want to compute, write out the variable substitutions we do, then show that each of this is very clearly computing the desired variable

Took a crack at this!

…rch (#2816) Progress towards: #2794 ## What is the purpose of the change This PR replaces our stableswap two-asset CFMM solver to use the `BigDec` binary search implementation in #2802 Note: most of the diff is tests merged from #2697 and the core binary search logic from #2802, both of which had to be merged into this branch ## Brief Changelog - Replace binary search solver logic with our `BigDec` binary search implementation ## Testing and Verifying - All two-asset CFMM test cases were set up to test this implementation in `amm_test.go` ## Documentation and Release Note - Does this pull request introduce a new feature or user-facing behavior changes? (no) - Is a relevant changelog entry added to the `Unreleased` section in `CHANGELOG.md`? (no) - How is the feature or change documented? (not documented)

ValarDragon

Nice!

LGTM to merge once conflicts are fixed

* add checks for invalid cfmm inputs * implement simplified two-asset cfmm solver * add further error and validation checks * reduce largest term's exponent from 12 to 8 * update comments to match new solver method Co-authored-by: Dev Ojha <[email protected]> * reduce largest terms from x^8 to x^4 and k^2 to k respectively * linter * linter * factor out test cases * add overflow and uneven pool tests * add thorough comments and links explaining solver math Co-authored-by: Dev Ojha <[email protected]>

AlpinYukseloglu added 2 commits September 10, 2022 14:04

add checks for invalid cfmm inputs

dba0541

implement simplified two-asset cfmm solver

8937eb8

github-actions bot added the C:x/gamm Changes, features and bugs related to the gamm module. label Sep 11, 2022

add further error and validation checks

e050381

AlpinYukseloglu mentioned this pull request Sep 12, 2022

[x/gamm][StableSwap] #1451

Closed

55 tasks

reduce largest term's exponent from 12 to 8

533b325

AlpinYukseloglu mentioned this pull request Sep 12, 2022

[stableswap]: Resolve large inaccuracies that arise from solver approximations #2702

Closed

ValarDragon reviewed Sep 12, 2022

View reviewed changes

x/gamm/pool-models/stableswap/amm.go Outdated Show resolved Hide resolved

AlpinYukseloglu and others added 4 commits September 12, 2022 12:19

update comments to match new solver method

7228e01

Co-authored-by: Dev Ojha <[email protected]>

reduce largest terms from x^8 to x^4 and k^2 to k respectively

899fdef

Merge branch 'simplify-cfmm-solver' of https://github.com/osmosis-lab…

9a159d4

…s/osmosis into simplify-cfmm-solver

linter

581eb6a

Merge branch 'main' into simplify-cfmm-solver

b245acb

AlpinYukseloglu marked this pull request as ready for review September 20, 2022 09:38

AlpinYukseloglu requested review from a team, ValarDragon and p0mvn September 20, 2022 09:38

ValarDragon reviewed Sep 20, 2022

View reviewed changes

AlpinYukseloglu requested a review from ValarDragon September 20, 2022 20:15

linter

31c2bb3

p0mvn self-assigned this Sep 20, 2022

AlpinYukseloglu mentioned this pull request Sep 21, 2022

Implement BigDec binary search in osmoutils #2802

Merged

Merge branch 'main' into simplify-cfmm-solver

0504812

AlpinYukseloglu mentioned this pull request Sep 22, 2022

[stableswap]: Switch two-asset CFMM solver to use BigDec binary search #2816

Merged

AlpinYukseloglu added 4 commits September 22, 2022 01:39

factor out test cases

07a5bad

merge main and fix conflicts

27e54bc

add overflow and uneven pool tests

23db7cf

add thorough comments and links explaining solver math

84e51b9

AlpinYukseloglu mentioned this pull request Sep 22, 2022

[stableswap] cfmm solver calculates incorrect value #2819

Closed

ValarDragon approved these changes Sep 25, 2022

View reviewed changes

AlpinYukseloglu mentioned this pull request Sep 26, 2022

[stableswap]: Combine solveCfmm and solveCfmmMulti #2847

Merged

Merge branch 'main' into simplify-cfmm-solver

b28b5f4

AlpinYukseloglu merged commit 5c5ffe6 into main Sep 26, 2022

AlpinYukseloglu deleted the simplify-cfmm-solver branch September 26, 2022 23:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[stableswap]: simplify CFMM solver #2697

[stableswap]: simplify CFMM solver #2697

AlpinYukseloglu commented Sep 11, 2022 •

edited

Loading

ValarDragon commented Sep 12, 2022 •

edited

Loading

AlpinYukseloglu commented Sep 12, 2022

ValarDragon commented Sep 14, 2022 •

edited

Loading

AlpinYukseloglu commented Sep 14, 2022 •

edited

Loading

ValarDragon commented Sep 14, 2022

ValarDragon Sep 20, 2022

AlpinYukseloglu Sep 20, 2022 •

edited

Loading

ValarDragon Sep 20, 2022

AlpinYukseloglu Sep 20, 2022 •

edited

Loading

ValarDragon left a comment •

edited

Loading

[stableswap]: simplify CFMM solver #2697

[stableswap]: simplify CFMM solver #2697

Conversation

AlpinYukseloglu commented Sep 11, 2022 • edited Loading

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

ValarDragon commented Sep 12, 2022 • edited Loading

AlpinYukseloglu commented Sep 12, 2022

ValarDragon commented Sep 14, 2022 • edited Loading

AlpinYukseloglu commented Sep 14, 2022 • edited Loading

ValarDragon commented Sep 14, 2022

ValarDragon Sep 20, 2022

Choose a reason for hiding this comment

AlpinYukseloglu Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

ValarDragon Sep 20, 2022

Choose a reason for hiding this comment

AlpinYukseloglu Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

ValarDragon left a comment • edited Loading

Choose a reason for hiding this comment

AlpinYukseloglu commented Sep 11, 2022 •

edited

Loading

ValarDragon commented Sep 12, 2022 •

edited

Loading

ValarDragon commented Sep 14, 2022 •

edited

Loading

AlpinYukseloglu commented Sep 14, 2022 •

edited

Loading

AlpinYukseloglu Sep 20, 2022 •

edited

Loading

AlpinYukseloglu Sep 20, 2022 •

edited

Loading

ValarDragon left a comment •

edited

Loading