[x/gamm][Refactor] New AMM interface #1066

ValarDragon · 2022-03-10T01:19:25Z

Closes: #XXX

Description

(WIP) New AMM interface in preparation for implementing stableswap. This PR is intended to be reviewed commit by commit.

The order of commits right now is:

Defining new Pool interface
Make balancer pool implement new API
Comment out some files that I think we should fix in future PR
(In progress) Change keeper logic to compile
- Status:
- ExitPool
  - Keeper side done
  - Balancer side done
- Swap
  - Balancer side done
  - Keeper side done
- JoinPool
  - Keeper side needs big refactor
  - Balancer side simple, still needs to be done
  - This is the big breaking change
- CreatePool
  - TBD on what even the right structure should be. For now just get something thats maybe awkward that works.

Changes I want to still make:

Change MsgJoinSwapExternAmountIn take in Coins instead of just Coin.
Write a document detailing design decisions here. E.g.
Fix queries for pool assets
Move poolassets.go to balancer types, and have everything new just use sdk.Coins in a simpler way. (Maybe we fix balancer in a second PR w/ migration logic)
Make an interface for MsgCreatePools

It turns out that we can do most of the CFMM optimization at a higher layer of abstraction, so I no longer think its worth making more CFMM scaffolding atm. Please let me know your thoughts after taking a look as well! I think the main thing in favor of CFMM scaffolding is further simplifying the amm.go file, but I don't think its that much code duplication atm.

For contributor use:

Targeted PR against correct branch (see CONTRIBUTING.md)
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

mattverse · 2022-03-10T17:04:33Z

@ValarDragon Is my understanding correct that you need review on the first two commits on this PR and see if #1051 would be unnecessary and go with the design you proposed in this PR?

ValarDragon · 2022-03-10T17:20:53Z

Yup! That'd be great

mattverse

concept Ack!
Just out of curiosity, what's the difference between amm.go and balancer_pool.go?

mattverse · 2022-03-11T05:45:32Z

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

+func (p Pool) parsePoolAssetsByDenoms(tokenADenom, tokenBDenom string) (
+	Aasset types.PoolAsset, Basset types.PoolAsset, err error) {
+	Aasset, found1 := types.GetPoolAssetByDenom(p.PoolAssets, tokenADenom)
+	Basset, found2 := types.GetPoolAssetByDenom(p.PoolAssets, tokenBDenom)
+	if !(found1 && found2) {
+		return Aasset, Basset, errors.New("TODO: fill message here")
+	}
+	return Aasset, Basset, nil
+}
+
+func (p Pool) parsePoolAssets(tokensA sdk.Coins, tokenBDenom string) (
+	tokenA sdk.Coin, Aasset types.PoolAsset, Basset types.PoolAsset, err error) {
+	if len(tokensA) != 1 {
+		return tokenA, Aasset, Basset, errors.New("TODO: Fill message here")
+	}
+	Aasset, Basset, err = p.parsePoolAssetsByDenoms(tokensA[0].Denom, tokenBDenom)
+	return tokensA[0], Aasset, Basset, err
+}
+
+// CalcOutAmtGivenIn calculates token to be swapped out given


I love this approach where poolAssets are no longer poolI but went in balancerPoolAsset

Agreed, this separation between assets / pool is definitely the way

ValarDragon · 2022-03-11T17:00:35Z

My goal was to put all the math public functions in amm.go, and everything else moved to balancer_pool.go

- Does not update JoinPool, will be done in next commit This implements swap, and exit pool for the keeper, both using the AMM interface correctly

ValarDragon · 2022-03-20T05:24:27Z

JoinPool logic now added, with the multi-asset non-even liquidity code added (but untested). We mainly need to add tests ala #1116 for each of the perfect inverse between joinpool / exit pool. I mostly want to check via test more of the edge case around rounding for JoinPoolNoSwap happens correctly. (No extra iterations through the single asset adds)

Remaining items for this PR:

(maybe) CreatePoolMsg interface
Fixing tests
- Appears that most of them right now are due to spot price params changing. I think we just need to switch them around.

- Swap spot price args temporarily to be the old style (which should change to new one) - Spot price rounding fix (needed weight to be decimal) - Change tests to get spot price from keeper, not pool directly - Fix bug in CalcAmountOutGivenIn - Add accounting for TokenInMaxs being nil - Add breaking change note for tokenInMaxs needs 0 coins or all coins

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

x/gamm/types/pool_asset.go

mconcat

Overall it looks like a great change. Merging multiple swap methods is awesome.

mconcat · 2022-03-21T13:57:09Z

x/gamm/types/pool.go

+	GetTotalLpBalances(ctx sdk.Context) sdk.Coins
+	GetTotalShares() sdk.Int
+
+	CalcOutAmtGivenIn(ctx sdk.Context, tokenIn sdk.Coins, tokenOutDenom string, swapFee sdk.Dec) (tokenOut sdk.DecCoin, err error)


Do we need different method for InOut and OutIn? I think it would not be a hassle to implement just two methods, so it will be fine, but just wanted to make sure

The reason was because of swap fee handling and liquidity during high volatility. One goal I didn't really document anywhere was enabling assymetric swap fees from the underlying AMM. I think the future of AMM's is that they track volatility themselves, and have different swap fees (Spreads) depending on the direction the trade is going during high volatility, and change the amount of effective liquidity. Similar to real market makers!

The alternative is we make the AMM interface do that, but I think the generality may suffer a bit

x/gamm/types/pool.go

mconcat · 2022-03-21T14:00:00Z

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

+func (p Pool) parsePoolAssetsByDenoms(tokenADenom, tokenBDenom string) (
+	Aasset types.PoolAsset, Basset types.PoolAsset, err error) {
+	Aasset, found1 := types.GetPoolAssetByDenom(p.PoolAssets, tokenADenom)
+	Basset, found2 := types.GetPoolAssetByDenom(p.PoolAssets, tokenBDenom)
+	if !(found1 && found2) {
+		return Aasset, Basset, errors.New("TODO: fill message here")
+	}
+	return Aasset, Basset, nil
+}
+
+func (p Pool) parsePoolAssets(tokensA sdk.Coins, tokenBDenom string) (
+	tokenA sdk.Coin, Aasset types.PoolAsset, Basset types.PoolAsset, err error) {
+	if len(tokensA) != 1 {
+		return tokenA, Aasset, Basset, errors.New("TODO: Fill message here")
+	}
+	Aasset, Basset, err = p.parsePoolAssetsByDenoms(tokensA[0].Denom, tokenBDenom)
+	return tokensA[0], Aasset, Basset, err
+}
+
+// CalcOutAmtGivenIn calculates token to be swapped out given


Agreed, this separation between assets / pool is definitely the way

mconcat · 2022-03-21T14:17:39Z

x/gamm/types/pool.go

+	// TODO: Ensure this can only be called via gamm
+	// TODO: Think through the API guarantees this is providing in conjunction with the caller being
+	// expected to Set the pool into state as well.
+	ApplySwap(ctx sdk.Context, tokenIn sdk.Coins, tokenOut sdk.Coins) error


Great interface! Maybe add shareIn, shareOut sdk.Int to capture shareswap case?

Interesting idea. At the moment JoinPool / ExitPool be mutative. (This is because I made iterative logic in the join pool code, which would be harder if it was non mutative)

wdyt about those being mutative, vs an "apply liquidity change" method?

mconcat · 2022-03-21T14:19:28Z

proto/osmosis/gamm/v1beta1/tx.proto

@@ -24,6 +24,7 @@ service Msg {
 }

 // ===================== MsgJoinPool
+// This is really MsgJoinPoolNoSwap


Can't we rename the protobuf message name, or does it break things?

So we can rename the struct, but we can't rename the amino route :/

Renaming any amino routes has to be blocked until we get a solution for #1017

Do you think we should rename the struct but not the amino route? Is that more or less confusing?

p0mvn

Awesome work!

Left a few comments about error checks and nits.

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

x/gamm/pool-models/balancer/balancer_pool.go

codecov-commenter · 2022-03-22T04:04:03Z

Codecov Report

Merging #1066 (bb23596) into main (5b1d86f) will decrease coverage by 1.16%.
The diff coverage is 37.01%.

@@            Coverage Diff             @@
##             main    #1066      +/-   ##
==========================================
- Coverage   20.76%   19.59%   -1.17%     
==========================================
  Files         199      200       +1     
  Lines       25649    25940     +291     
==========================================
- Hits         5326     5083     -243     
- Misses      19360    19950     +590     
+ Partials      963      907      -56

Impacted Files	Coverage Δ
x/gamm/genesis.go	`71.42% <ø> (+3.42%)`	⬆️
x/gamm/keeper/invariants.go	`0.00% <ø> (-14.93%)`	⬇️
x/gamm/keeper/msg_server.go	`2.79% <0.00%> (-0.02%)`	⬇️
x/gamm/pool-models/balancer/amm.go	`0.00% <0.00%> (ø)`
x/gamm/types/pool.go	`0.00% <ø> (ø)`
x/gamm/types/pool_asset.go	`0.00% <0.00%> (ø)`
x/gamm/pool-models/balancer/balancer_pool.go	`59.51% <3.44%> (-7.76%)`	⬇️
x/gamm/keeper/pool.go	`68.00% <33.33%> (-1.68%)`	⬇️
x/superfluid/keeper/epoch.go	`67.92% <50.00%> (ø)`
x/gamm/keeper/share.go	`63.63% <61.53%> (-6.37%)`	⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b1d86f...bb23596. Read the comment docs.

ValarDragon · 2022-03-22T04:05:50Z

All comments addressed!

ValarDragon added 3 commits March 9, 2022 16:52

New Pool interface

c8d1af8

Make balancer pool implement new API

7b7caee

Comment out files to fix at the end

579a472

ValarDragon added state-machine-breaking C:x/gamm Changes, features and bugs related to the gamm module. labels Mar 10, 2022

czarcas7ic mentioned this pull request Mar 10, 2022

[Epic] GAMM module #1050

Closed

6 tasks

ValarDragon force-pushed the dev/new_amm_interface branch from e1b4187 to d6c1062 Compare March 10, 2022 22:37

mattverse reviewed Mar 11, 2022

View reviewed changes

ValarDragon force-pushed the dev/new_amm_interface branch 4 times, most recently from fdf1dbe to fe3ee9b Compare March 17, 2022 02:01

Update most of keeper for new interface

0fe0f42

- Does not update JoinPool, will be done in next commit This implements swap, and exit pool for the keeper, both using the AMM interface correctly

ValarDragon force-pushed the dev/new_amm_interface branch from fe3ee9b to 0fe0f42 Compare March 17, 2022 05:28

ValarDragon added 3 commits March 17, 2022 00:40

Fix many of the cross-module breaks

ba08af7

Finish ExitPool messages for this PR

18c4bc4

Merge branch 'main' into dev/new_amm_interface

eb7300b

ValarDragon force-pushed the dev/new_amm_interface branch from 43ba32e to ad77fc7 Compare March 19, 2022 23:55

JoinPool logic

671fa00

ValarDragon force-pushed the dev/new_amm_interface branch from ad77fc7 to 671fa00 Compare March 20, 2022 05:21

Tests compiling

e5f8250

ValarDragon force-pushed the dev/new_amm_interface branch 3 times, most recently from 4caf3b5 to 7f3238a Compare March 21, 2022 03:10

ValarDragon marked this pull request as ready for review March 21, 2022 03:10

ValarDragon force-pushed the dev/new_amm_interface branch from 7f3238a to f127eaf Compare March 21, 2022 03:24

ValarDragon added 2 commits March 20, 2022 22:35

Comment out tests for unimplemented functions, fix lint

e12a08b

Fix CLI tests

c9456bf