Add mutative, non mutative method for `PoolI` #1147

mattverse · 2022-03-25T09:45:29Z

Closes: #1142

Description

This PR separates out mutative methods for joinPool, exitPool and swap functions for PoolI.

Further things to address in the future: changing the existing methods(ex. SwapExactAmountIn in swap.go ) so that they use the mutative methods.

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

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

ValarDragon · 2022-03-25T15:18:10Z

Ah, I think we need the PR to delete ApplySwap, and then have everything use the Swap methods. Otherwise we don't get the API safety we wanted!

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

mattverse · 2022-03-28T06:52:07Z

Yeah agreed that we need to remove ApplySwap, was planning to make a second PR but seems that we could do it all in this PR!

codecov-commenter · 2022-03-28T11:22:38Z

Codecov Report

Merging #1147 (0b9b915) into main (d143d83) will decrease coverage by 0.24%.
The diff coverage is 8.19%.

@@            Coverage Diff             @@
##             main    #1147      +/-   ##
==========================================
- Coverage   20.21%   19.97%   -0.25%     
==========================================
  Files         199      199              
  Lines       25062    25459     +397     
==========================================
+ Hits         5066     5085      +19     
- Misses      19069    19445     +376     
- Partials      927      929       +2

Impacted Files	Coverage Δ
osmomath/math.go	`64.44% <ø> (ø)`
store/node.go	`66.94% <ø> (ø)`
v043_temp/address/hash.go	`80.00% <ø> (ø)`
x/claim/abci.go	`0.00% <ø> (ø)`
x/claim/client/cli/query.go	`34.45% <ø> (ø)`
x/claim/client/cli/tx.go	`0.00% <ø> (ø)`
x/claim/handler.go	`0.00% <ø> (ø)`
x/claim/keeper/claim.go	`64.33% <ø> (ø)`
x/claim/keeper/grpc_query.go	`0.00% <ø> (ø)`
x/claim/keeper/hooks.go	`28.00% <ø> (ø)`
... and 130 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 a508900...0b9b915. Read the comment docs.

alexanderbez

🚀

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

alexanderbez · 2022-03-28T18:17:01Z

x/gamm/types/pool.go

@@ -28,21 +28,22 @@ type PoolI interface {
 	GetTotalShares() sdk.Int

 	CalcOutAmtGivenIn(ctx sdk.Context, tokenIn sdk.Coins, tokenOutDenom string, swapFee sdk.Dec) (tokenOut sdk.DecCoin, err error)
-	CalcInAmtGivenOut(ctx sdk.Context, tokenOut sdk.Coins, tokenInDenom string, swapFee sdk.Dec) (tokenIn sdk.DecCoin, err error)
+	SwapOutAmtGivenIn(ctx sdk.Context, tokenIn sdk.Coins, tokenOutDenom string, swapFee sdk.Dec) (tokenOut sdk.Coin, err error)


nit: not that is has to be done in these PRs, but I believe that these methods are the meat n bones of the AMM state machine, or are at least a large part of it. Being so, I think each of these methods in the interface here should be very very well documented with godocs.

Created #1183 to track

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

ValarDragon · 2022-03-29T00:54:41Z

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

-			newShares, err := p.singleAssetJoin(coin, swapFee)
+			newShares, err := p.calcSingleAssetJoin(coin, swapFee)
 			if err != nil {
-				return sdk.ZeroInt(), err
+				return sdk.ZeroInt(), sdk.NewCoins(), err
 			}
-			p.updateLiquidity(newShares, sdk.Coins{coin})
+			newLiquidity = newLiquidity.Add(coin)


So these single asset joins actual need the liquidity updated in order to work :/
This is because the pool reserves and therefore correct amounts out change at each of these steps.

Because the JoinPool and ExitPool methods have a lot of internal steps pool liquidity updates for correctness, I'd suggest doing this in one of two ways:

Keeping JoinPool and ExitPool as before, but then make the calc methods do a deep copy of the struct. (Probably easier) I don't know if deepcopy / clone methods are going to be straight forward to make, or we'd have to make a function called "clonePoolAssets" to do it.

Making a common method, that does all of this logic as before, but with pool assets passed in as an argument rather than read off the pool. (It can mutate this pool assets) Then make the calc method use a cloned pool assets struct, and the join pool use the 'real' pool assets struct.

Gotcha! How is this passing tests right now tho :/

Uhhhh that's a very good question.

On first thought, maybe it's that we previously didn't support lping at arbitrary ratios, and therefore dont have tests for this? (Which is then concerning/ my bad for not adding tests in the refactoring)

mattverse · 2022-03-29T12:48:07Z

So I took the approach where we keep the original structure, but pass in poolAssets, poolShares, as a parameter for calculating calcSingleAssetJoin. This way we can use the updated values of poolAssets and poolShares without mutating the original values, then mutate the original value only when joiningpool

p0mvn

LGTM

p0mvn · 2022-03-31T16:55:51Z

x/gamm/types/pool.go


 	SpotPrice(ctx sdk.Context, baseAssetDenom string, quoteAssetDenom string) (sdk.Dec, error)

 	// JoinPool joins the pool, and uses all of the tokensIn provided.
 	// The AMM swaps to whatever the ratio should be and returns the number of shares created.
 	// Internally the pool updates its count for the number of shares in this function.
 	// If the function errors, or should not be mutative, then state must be reverted after this call.
+	CalcJoinPoolShares(ctx sdk.Context, tokensIn sdk.Coins, swapFee sdk.Dec) (numShares sdk.Int, newLiquidity sdk.Coins, err error)


nit: seems that the comment above applies to JoinPool

yeah we would have to go through the comments for PoolI. Many of them seemed to be like temporary comments for @ValarDragon . I'll leave it to @ValarDragon to go through / organize the final struct for PoolI: doesn't have to be in this PR

p0mvn · 2022-03-31T17:15:52Z

x/gamm/types/pool.go

@@ -28,21 +28,22 @@ type PoolI interface {
 	GetTotalShares() sdk.Int

 	CalcOutAmtGivenIn(ctx sdk.Context, tokenIn sdk.Coins, tokenOutDenom string, swapFee sdk.Dec) (tokenOut sdk.DecCoin, err error)
-	CalcInAmtGivenOut(ctx sdk.Context, tokenOut sdk.Coins, tokenInDenom string, swapFee sdk.Dec) (tokenIn sdk.DecCoin, err error)
+	SwapOutAmtGivenIn(ctx sdk.Context, tokenIn sdk.Coins, tokenOutDenom string, swapFee sdk.Dec) (tokenOut sdk.Coin, err error)


Created #1183 to track

ValarDragon

LGTM! Can you reslolve conflicts then we can merge?

mattverse · 2022-04-05T08:54:55Z

@ValarDragon Sweet! Can you review the resolved conflicts?

Add mutative, non mutative method for PoolI

18acaca

ValarDragon reviewed Mar 25, 2022

View reviewed changes

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

ValarDragon reviewed Mar 25, 2022

View reviewed changes

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

ValarDragon reviewed Mar 25, 2022

View reviewed changes

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

ValarDragon requested a review from mconcat March 27, 2022 17:39

Remove ApplySwap and its usage

0b9b915

alexanderbez approved these changes Mar 28, 2022

View reviewed changes

ValarDragon requested changes Mar 29, 2022

View reviewed changes

Add internal steps for liquidity update before swap calc

a68928e

mattverse requested a review from ValarDragon March 31, 2022 08:47

This was referenced Mar 31, 2022

[x/gamm] Document PoolI interface in x/gamm/types/pool.go #1183

Closed

[x/gamm] Re-implement ExitSwapExternAmountOut generically #1130

Closed

p0mvn approved these changes Mar 31, 2022

View reviewed changes

ValarDragon approved these changes Apr 5, 2022

View reviewed changes

merge main

c274e44

ValarDragon merged commit 5013cd4 into main Apr 5, 2022

ValarDragon deleted the mattverse/gamm-non-mutative-methods branch April 5, 2022 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mutative, non mutative method for `PoolI` #1147

Add mutative, non mutative method for `PoolI` #1147

mattverse commented Mar 25, 2022 •

edited by alexanderbez

Loading

ValarDragon commented Mar 25, 2022 •

edited

Loading

mattverse commented Mar 28, 2022

codecov-commenter commented Mar 28, 2022

alexanderbez left a comment

alexanderbez Mar 28, 2022

p0mvn Mar 31, 2022

ValarDragon Mar 29, 2022

mattverse Mar 29, 2022

ValarDragon Mar 29, 2022

mattverse commented Mar 29, 2022

p0mvn left a comment

p0mvn Mar 31, 2022

mattverse Mar 31, 2022

p0mvn Mar 31, 2022

ValarDragon left a comment •

edited

Loading

mattverse commented Apr 5, 2022

Add mutative, non mutative method for PoolI #1147

Add mutative, non mutative method for PoolI #1147

Conversation

mattverse commented Mar 25, 2022 • edited by alexanderbez Loading

Description

ValarDragon commented Mar 25, 2022 • edited Loading

mattverse commented Mar 28, 2022

codecov-commenter commented Mar 28, 2022

Codecov Report

alexanderbez left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattverse commented Mar 29, 2022

p0mvn left a 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

ValarDragon left a comment • edited Loading

Choose a reason for hiding this comment

mattverse commented Apr 5, 2022

Add mutative, non mutative method for `PoolI` #1147

Add mutative, non mutative method for `PoolI` #1147

mattverse commented Mar 25, 2022 •

edited by alexanderbez

Loading

ValarDragon commented Mar 25, 2022 •

edited

Loading

ValarDragon left a comment •

edited

Loading