Add boilerplate logic for stableswap swaps and spot price #1287

ValarDragon · 2022-04-18T21:23:47Z

Adds logic for stableswap implementing the swap and spot price interfaces. The PR is marked as draft until some test cases are added.

For contributor use:o

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

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

p0mvn · 2022-04-18T23:59:42Z

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

 func (pa Pool) CalcOutAmtGivenIn(ctx sdk.Context, tokenIn sdk.Coins, tokenOutDenom string, swapFee sdk.Dec) (tokenOut sdk.DecCoin, err error) {
 	if tokenIn.Len() != 1 {
-		return sdk.DecCoin{}, errors.New("asdf")
+		return sdk.DecCoin{}, errors.New("stableswap CalcOutAmtGivenIn: tokenIn is of wrong length")


does it make sense to format the length value in the message?

p0mvn · 2022-04-19T00:01:53Z

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

+	// we ignore the decimal component, as token out amount must round down
+	tokenOut, _ = tokenOutDec.TruncateDecimal()
+	if !tokenOut.Amount.IsPositive() {
+		return sdk.Coin{}, sdkerrors.Wrapf(types.ErrInvalidMathApprox, "token amount must be positive")


does it make sense to format what it actually was in the message?

p0mvn · 2022-04-19T00:05:13Z

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

+	p.PoolLiquidity = p.PoolLiquidity.Add(tokensIn...).Sub(tokensOut)
+	// sanity check that no new denoms were added
+	if len(p.PoolLiquidity) != l {
+		panic("applySwap changed number of tokens in pool")


q: At first, I thought that we can propagate the error up top. However, it seems that the panic here is for a reason. We don't want to let the caller handle the error if the number of tokens happens to change. Is my reasoning correct?

yeah, this should never happen. (It should have been caught by the methods that call it in this package, and not be able to be triggered from functions outside the package)

p0mvn · 2022-04-19T00:05:52Z

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

 }

 func (pa Pool) CalcInAmtGivenOut(ctx sdk.Context, tokenOut sdk.Coins, tokenInDenom string, swapFee sdk.Dec) (tokenIn sdk.DecCoin, err error) {
-	return sdk.DecCoin{}, types.ErrNotImplemented
+	if tokenOut.Len() != 1 {
+		return sdk.DecCoin{}, errors.New("stableswap CalcInAmtGivenOut: tokenOut is of wrong length")


nit: format the length if makes sense

codecov-commenter · 2022-04-19T00:17:37Z

Codecov Report

Merging #1287 (0b2da2f) into main (298ad00) will increase coverage by 0.08%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1287      +/-   ##
==========================================
+ Coverage   20.14%   20.23%   +0.08%     
==========================================
  Files         203      203              
  Lines       26824    26868      +44     
==========================================
+ Hits         5405     5437      +32     
- Misses      20412    20421       +9     
- Partials     1007     1010       +3

Impacted Files	Coverage Δ
x/gamm/pool-models/balancer/amm.go	`17.41% <0.00%> (+14.39%)`	⬆️
x/gamm/pool-models/stableswap/amm.go	`43.28% <0.00%> (-12.49%)`	⬇️
x/gamm/pool-models/stableswap/pool.go	`0.00% <0.00%> (ø)`
x/gamm/pool-models/balancer/balancer_pool.go	`64.37% <0.00%> (+1.28%)`	⬆️

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 298ad00...0b2da2f. Read the comment docs.

ValarDragon · 2022-04-21T15:34:35Z

Decision from call: Add test for this module, don't amend the more general testing Roman recently added right now

mattverse

Left some minor comments! (mostly comments for the purpose of actually understanding what's happening rather than changes requests)

mattverse · 2022-04-22T07:14:19Z

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

+	tokenInCoin, tokenInDecimal := tokenInDecCoin.TruncateDecimal()
+	// if tokenInDecimal is non-zero, we add 1 to the tokenInCoin
+	if tokenInDecimal.Amount.IsPositive() {
+		tokenInCoin.Amount.AddRaw(1)


don't we have to do tokenInCoin = tokenInCoin.Amount.AddRaw(1) ?

Also, I'm curious why we add 1 in the case of non-zeros!

ah yeah your right, good catch.

The idea here is that I want to get exactly 100 tokens out after the trade. But to do that, I have to provide say 50.82 tokens as input. Tokens are stored as integers, so before we were only requiring 50 tokens to be sent. But actually it should have been 51 when its converted to integers.

We should always round-up here for correctness!

I'm going to revert this change for Balancer, and fix it in a second PR that fixes CalcSpotPrice to be an integer API

mattverse · 2022-04-22T07:39:13Z

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

+	if len(p.PoolLiquidity) != numTokens {
+		panic("updatePoolLiquidityForSwap changed number of tokens in pool")
+	}


love this! 🚀

mattverse · 2022-04-22T07:52:11Z

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

+	// We are solving for the amount of token in, cfmm(x,y) = cfmm(x + x_in, y - y_out)
+	// x = tokenInSupply, y = tokenOutSupply, yIn = -tokenOutAmount
+	inAmtRaw := solveCfmm(tokenInSupply, tokenOutSupply, tokenOut.Amount.ToDec().Neg())


Could you explain why we use a negative value for y in?

mattverse

Core function LGTM once https://github.com/osmosis-labs/osmosis/pull/1287/files#r855833475 is fixed.

I propose we merge this to main without tests as of now, since this PR is a big blocker in gamm generalization parallelization!

Add boilerplate logic for stableswap swaps and spot price

dd05943

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

ValarDragon requested review from alexanderbez, mattverse and p0mvn April 18, 2022 21:41

p0mvn reviewed Apr 19, 2022

View reviewed changes

applySwap -> updatePoolLiquidityForSwap

60f4c35

l -> numTokens

0b2da2f

mattverse reviewed Apr 22, 2022

View reviewed changes

mattverse self-requested a review April 25, 2022 16:51

mattverse approved these changes Apr 25, 2022

View reviewed changes

Fix bug matt pointed out

c613b44

ValarDragon marked this pull request as ready for review April 25, 2022 18:47

ValarDragon and others added 2 commits April 25, 2022 14:48

Revert rounding update

3456a19

Merge branch 'main' into dev/stableswap_interface

56b87cc

ValarDragon added the A:automerge label Apr 25, 2022

mergify bot merged commit 1c7ea07 into main Apr 25, 2022

mergify bot deleted the dev/stableswap_interface branch April 25, 2022 19:52

ValarDragon mentioned this pull request Apr 25, 2022

Change Pool interface calc methods to using integers #1345

Merged

mattverse mentioned this pull request Apr 26, 2022

Add tests for stableswap calc methods and spot prices #1349

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add boilerplate logic for stableswap swaps and spot price #1287

Add boilerplate logic for stableswap swaps and spot price #1287

ValarDragon commented Apr 18, 2022

p0mvn Apr 18, 2022

p0mvn Apr 19, 2022

p0mvn Apr 19, 2022

ValarDragon Apr 19, 2022

p0mvn Apr 19, 2022

codecov-commenter commented Apr 19, 2022 •

edited

Loading

ValarDragon commented Apr 21, 2022

mattverse left a comment

mattverse Apr 22, 2022

mattverse Apr 22, 2022

ValarDragon Apr 22, 2022 •

edited

Loading

ValarDragon Apr 25, 2022

mattverse Apr 22, 2022

mattverse Apr 22, 2022

mattverse left a comment

Add boilerplate logic for stableswap swaps and spot price #1287

Add boilerplate logic for stableswap swaps and spot price #1287

Conversation

ValarDragon commented Apr 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 19, 2022 • edited Loading

Codecov Report

ValarDragon commented Apr 21, 2022

mattverse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon Apr 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattverse left a comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 19, 2022 •

edited

Loading

ValarDragon Apr 22, 2022 •

edited

Loading