Skip to content
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.

Sign up for GitHub

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

Add mutative, non mutative method for PoolI #1147

Merged
merged 4 commits into from
Apr 5, 2022
Merged

Add mutative, non mutative method for PoolI #1147

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
18acaca
Add mutative, non mutative method for PoolI
mattverse Mar 25, 2022
0b9b915
Remove ApplySwap and its usage
mattverse Mar 28, 2022
a68928e
Add internal steps for liquidity update before swap calc
mattverse Mar 29, 2022
c274e44
merge main
mattverse Apr 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 83 additions & 20 deletions x/gamm/pool-models/balancer/amm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ import (
"errors"

sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/osmosis-labs/osmosis/v7/osmomath"
"github.com/osmosis-labs/osmosis/v7/x/gamm/types"
)

// solveConstantFunctionInvariant solves the constant function of an AMM
Expand Down Expand Up @@ -61,7 +63,28 @@ func (p Pool) CalcOutAmtGivenIn(
return sdk.NewDecCoinFromDec(tokenOutDenom, tokenAmountOut), nil
}

// calcInAmtGivenOut calculates token to be provided, fee added,
// SwapOutAmtGivenIn is a mutative method for CalcOutAmtGivenIn, which includes the actual swap
mattverse marked this conversation as resolved.
Show resolved Hide resolved
func (p Pool) SwapOutAmtGivenIn(
mattverse marked this conversation as resolved.
Show resolved Hide resolved
ctx sdk.Context, tokensIn sdk.Coins, tokenOutDenom string, swapFee sdk.Dec) (
mattverse marked this conversation as resolved.
Show resolved Hide resolved
tokenOut sdk.Coin, err error,
) {
tokenOutDecCoin, err := p.CalcOutAmtGivenIn(ctx, tokensIn, tokenOutDenom, swapFee)
if err != nil {
return sdk.Coin{}, err
}
tokenOutCoin, _ := tokenOutDecCoin.TruncateDecimal()
if tokenOutCoin.Amount.LTE(sdk.ZeroInt()) {
mattverse marked this conversation as resolved.
Show resolved Hide resolved
return sdk.Coin{}, sdkerrors.Wrapf(types.ErrInvalidMathApprox, "token amount is zero or negative")
mattverse marked this conversation as resolved.
Show resolved Hide resolved
}

err = p.ApplySwap(ctx, tokensIn, sdk.Coins{tokenOutCoin})
if err != nil {
return sdk.Coin{}, err
}
return tokenOutCoin, nil
}

// CalcInAmtGivenOut calculates token to be provided, fee added,
// given the swapped out amount, using solveConstantFunctionInvariant.
func (p Pool) CalcInAmtGivenOut(
ctx sdk.Context, tokensOut sdk.Coins, tokenInDenom string, swapFee sdk.Dec) (
Expand All @@ -88,6 +111,27 @@ func (p Pool) CalcInAmtGivenOut(
return sdk.NewDecCoinFromDec(tokenInDenom, tokenAmountInBeforeFee), nil
}

// SwapInAmtGivenOut is a mutative method for CalcOutAmtGivenIn, which includes the actual swap
mattverse marked this conversation as resolved.
Show resolved Hide resolved
func (p Pool) SwapInAmtGivenOut(
mattverse marked this conversation as resolved.
Show resolved Hide resolved
ctx sdk.Context, tokensOut sdk.Coins, tokenInDenom string, swapFee sdk.Dec) (
tokenIn sdk.Coin, err error,
) {
tokenInDecCoin, err := p.CalcInAmtGivenOut(ctx, tokensOut, tokenInDenom, swapFee)
if err != nil {
return sdk.Coin{}, sdkerrors.Wrapf(types.ErrInvalidMathApprox, "token amount is zero or negative")
}
tokenInCoin, _ := tokenInDecCoin.TruncateDecimal()
if tokenInCoin.Amount.LTE(sdk.ZeroInt()) {
mattverse marked this conversation as resolved.
Show resolved Hide resolved
return sdk.Coin{}, sdkerrors.Wrapf(types.ErrInvalidMathApprox, "token amount is zero or negative")
mattverse marked this conversation as resolved.
Show resolved Hide resolved
}

err = p.ApplySwap(ctx, sdk.Coins{tokenInCoin}, tokensOut)
if err != nil {
return sdk.Coin{}, err
}
return tokenInCoin, nil
}

// ApplySwap.
func (p *Pool) ApplySwap(ctx sdk.Context, tokensIn sdk.Coins, tokensOut sdk.Coins) error {
mattverse marked this conversation as resolved.
Show resolved Hide resolved
// Also ensures that len(tokensIn) = 1 = len(tokensOut)
Expand Down Expand Up @@ -160,7 +204,7 @@ func calcPoolOutGivenSingleIn(
}

// calcPoolOutGivenSingleIn - balance pAo.
func (p *Pool) singleAssetJoin(tokenIn sdk.Coin, swapFee sdk.Dec) (numShares sdk.Int, err error) {
func (p *Pool) calcSingleAssetJoin(tokenIn sdk.Coin, swapFee sdk.Dec) (numShares sdk.Int, err error) {
tokenInPoolAsset, err := p.GetPoolAsset(tokenIn.Denom)
if err != nil {
return sdk.ZeroInt(), err
Expand Down Expand Up @@ -222,35 +266,62 @@ func (p *Pool) maximalExactRatioJoin(tokensIn sdk.Coins) (numShares sdk.Int, rem
}

func (p *Pool) JoinPool(_ctx sdk.Context, tokensIn sdk.Coins, swapFee sdk.Dec) (numShares sdk.Int, err error) {
numShares, newLiquidity, err := p.CalcJoinPoolShares(_ctx, tokensIn, swapFee)
if err != nil {
return sdk.Int{}, err
}
p.updateLiquidity(numShares, newLiquidity)
return numShares, nil
}

func (p *Pool) CalcJoinPoolShares(_ctx sdk.Context, tokensIn sdk.Coins, swapFee sdk.Dec) (numShares sdk.Int, newLiquidity sdk.Coins, err error) {
if tokensIn.Len() == 1 {
numShares, err = p.singleAssetJoin(tokensIn[0], swapFee)
p.updateLiquidity(numShares, tokensIn)
return numShares, err
numShares, err = p.calcSingleAssetJoin(tokensIn[0], swapFee)
newLiquidity = tokensIn
return numShares, newLiquidity, err
} else if tokensIn.Len() != p.NumAssets() {
return sdk.ZeroInt(), errors.New(
return sdk.ZeroInt(), sdk.NewCoins(), errors.New(
"balancer pool only supports LP'ing with one asset, or all assets in pool")
}
// Add all exact coins we can (no swap)
numShares, remCoins, err := p.maximalExactRatioJoin(tokensIn)
if err != nil {
return sdk.ZeroInt(), err
return sdk.ZeroInt(), sdk.NewCoins(), err
}
p.updateLiquidity(numShares, tokensIn.Sub(remCoins))
newLiquidity = tokensIn.Sub(remCoins)
// if there are coins that couldn't be perfectly joined, do single asset joins for each of them.
if !remCoins.Empty() {
for _, coin := range remCoins {
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

numShares = numShares.Add(newShares)
}
}
return numShares, nil
return numShares, newLiquidity, nil
}

func (p *Pool) ExitPool(ctx sdk.Context, exitingShares sdk.Int, exitFee sdk.Dec) (exitedCoins sdk.Coins, err error) {
exitedCoins, err = p.CalcExitPoolShares(ctx, exitingShares, exitFee)
if err != nil {
return sdk.Coins{}, err
}

balances := p.GetTotalPoolLiquidity(ctx).Sub(exitedCoins)
err = p.UpdatePoolAssetBalances(balances)
if err != nil {
return sdk.Coins{}, err
}

totalShares := p.GetTotalShares()
p.TotalShares = sdk.NewCoin(p.TotalShares.Denom, totalShares.Sub(exitingShares))

return exitedCoins, nil
}

func (p *Pool) CalcExitPoolShares(ctx sdk.Context, exitingShares sdk.Int, exitFee sdk.Dec) (exitedCoins sdk.Coins, err error) {
totalShares := p.GetTotalShares()
if exitingShares.GTE(totalShares) {
return sdk.Coins{}, errors.New(("too many shares out"))
Expand All @@ -274,14 +345,6 @@ func (p *Pool) ExitPool(ctx sdk.Context, exitingShares sdk.Int, exitFee sdk.Dec)
continue
}
exitedCoins = exitedCoins.Add(sdk.NewCoin(asset.Denom, exitAmt))
// update pool assets for this exit amount.
newAmt := asset.Amount.Sub(exitAmt)
err = p.UpdatePoolAssetBalance(sdk.NewCoin(asset.Denom, newAmt))
if err != nil {
return sdk.Coins{}, err
}
}

p.TotalShares = sdk.NewCoin(p.TotalShares.Denom, totalShares.Sub(exitingShares))
return exitedCoins, nil
}
6 changes: 6 additions & 0 deletions x/gamm/types/pool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,10 @@ type PoolI interface {
GetTotalShares() sdk.Int

CalcOutAmtGivenIn(ctx sdk.Context, tokenIn sdk.Coins, tokenOutDenom string, swapFee sdk.Dec) (tokenOut sdk.DecCoin, err error)
SwapOutAmtGivenIn(ctx sdk.Context, tokenIn sdk.Coins, tokenOutDenom string, swapFee sdk.Dec) (tokenOut sdk.Coin, err error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #1183 to track


CalcInAmtGivenOut(ctx sdk.Context, tokenOut sdk.Coins, tokenInDenom string, swapFee sdk.Dec) (tokenIn sdk.DecCoin, err error)
SwapInAmtGivenOut(ctx sdk.Context, tokenOut sdk.Coins, tokenInDenom string, swapFee sdk.Dec) (tokenIn sdk.Coin, err error)

// TODO: Ensure this can only be called via gamm
// TODO: Think through the API guarantees this is providing in conjunction with the caller being
Expand All @@ -41,8 +44,11 @@ type PoolI interface {
// 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: seems that the comment above applies to JoinPool

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

JoinPool(ctx sdk.Context, tokensIn sdk.Coins, swapFee sdk.Dec) (numShares sdk.Int, err error)

ExitPool(ctx sdk.Context, numShares sdk.Int, exitFee sdk.Dec) (exitedCoins sdk.Coins, err error)
CalcExitPoolShares(ctx sdk.Context, numShares sdk.Int, exitFee sdk.Dec) (exitedCoins sdk.Coins, err error)
}

func NewPoolAddress(poolId uint64) sdk.AccAddress {
Expand Down