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.

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

[CL]: Refactor and generalize pool interface #3031

Closed
p0mvn opened this issue Oct 17, 2022 · 5 comments · Fixed by #3035
Closed

[CL]: Refactor and generalize pool interface #3031

p0mvn opened this issue Oct 17, 2022 · 5 comments · Fixed by #3035
Labels
C:x/gamm Changes, features and bugs related to the gamm module. T:task ⚙️ A task belongs to a story

Comments

@p0mvn
Copy link
Member

p0mvn commented Oct 17, 2022

Background

There are several methods currently defined on the pool interface that are applicable to balancer and stableswap pools but not the concentrated liquidity pool.

Therefore, we should identify what methods should be moved out of the interface into a pool interface extension similar to:

type PoolAmountOutExtension interface {

Suggested Design

Upon initial inspection of the current pool interface methods, the following should be moved out to a separate extension:

  • GetTotalPoolLiquidity
    • According to the concentrated liquidity pool design, we aren't going to keep track of each token individually. Instead, we will keep track of $$\sqrt P$$ and $$L$$
  • PokePool
    • This method is concerned with weights that only seem to be applicable to the balancer pool

The remaining methods can be split into 3 categories:

  • needed for LP
  • needed for swapping
  • generalized getters

Acceptance Criteria

  • Pool interface is refactored and generalized
  • The listed above methods are moved to a separate interface extension
  • Balancer pool continues to function as expected
  • Stableswap pool continues to function as expected
@p0mvn p0mvn added the C:x/gamm Changes, features and bugs related to the gamm module. label Oct 17, 2022
@osmo-bot osmo-bot moved this to Needs Review 🔍 in Osmosis Chain Development Oct 17, 2022
@p0mvn p0mvn added the T:task ⚙️ A task belongs to a story label Oct 17, 2022
@ValarDragon
Copy link
Member

Concentrated liquidity at most should only share a swap interface with normal AMM's. They should not share any other methods / should otherwise have distinct interfaces.

@p0mvn
Copy link
Member Author

p0mvn commented Oct 17, 2022

Concentrated liquidity at most should only share a swap interface with normal AMM's. They should not share any other methods / should otherwise have distinct interfaces.

Agreed, the goal here is to move out any balancer and stable swap-specific methods into an interface extension and only keep swapping/joining methods.

Do you also agree about join pool no swap and exit pool methods being applicable? These also seem to be valid for adding/removing liquidity

@p0mvn
Copy link
Member Author

p0mvn commented Oct 18, 2022

@mattverse has determined that the API change for joining and exiting pool would have to be significantly different due to ticks. As a result, we are exploring the idea of implementing the concentrated liquidity pool with its own API. Likely in another module (not gamm).

That being said, I removed the PokePool method from the pool interface because it only seems to make sense for balancer pools: #3035

@p0mvn
Copy link
Member Author

p0mvn commented Oct 18, 2022

After discussing and thinking about the architecture more, we should be able to extract an AmmInterface that is implemented by balancer and stableswap pools. It should contain the methods only compatible with the amm pools.

On the other hand, PoolI would have methods that can be generalized across all of balancer, stableswap, and concentrated

The proposed refactor would look like the following:

  • PoolI
// PoolI defines an interface for pools that hold tokens.
type PoolI interface {
	proto.Message

	GetAddress() sdk.AccAddress
	String() string
	GetId() uint64
	// GetSwapFee returns the pool's swap fee, based on the current state.
	// Pools may choose to make their swap fees dependent upon state
	// (prior TWAPs, network downtime, other pool states, etc.)
	// hence Context is provided as an argument.
	GetSwapFee(ctx sdk.Context) sdk.Dec
	// GetExitFee returns the pool's exit fee, based on the current state.
	// Pools may choose to make their exit fees dependent upon state.
	GetExitFee(ctx sdk.Context) sdk.Dec
	// Returns whether the pool has swaps enabled at the moment
	IsActive(ctx sdk.Context) bool
	// GetTotalShares returns the total number of LP shares in the pool
	GetTotalShares() sdk.Int

       	// SwapOutAmtGivenIn swaps 'tokenIn' against the pool, for tokenOutDenom, with the provided swapFee charged.
	// Balance transfers are done in the keeper, but this method updates the internal pool state.
	SwapOutAmtGivenIn(ctx sdk.Context, tokenIn sdk.Coins, tokenOutDenom string, swapFee sdk.Dec) (tokenOut sdk.Coin, err error)
	// CalcOutAmtGivenIn returns how many coins SwapOutAmtGivenIn would return on these arguments.
	// This does not mutate the pool, or state.
	CalcOutAmtGivenIn(ctx sdk.Context, tokenIn sdk.Coins, tokenOutDenom string, swapFee sdk.Dec) (tokenOut sdk.Coin, err error)

	// SwapInAmtGivenOut swaps exactly enough tokensIn against the pool, to get the provided tokenOut amount out of the pool.
	// Balance transfers are done in the keeper, but this method updates the internal pool state.
	SwapInAmtGivenOut(ctx sdk.Context, tokenOut sdk.Coins, tokenInDenom string, swapFee sdk.Dec) (tokenIn sdk.Coin, err error)
	// CalcInAmtGivenOut returns how many coins SwapInAmtGivenOut would return on these arguments.
	// This does not mutate the pool, or state.
	CalcInAmtGivenOut(ctx sdk.Context, tokenOut sdk.Coins, tokenInDenom string, swapFee sdk.Dec) (tokenIn sdk.Coin, err error)


     	// Returns the spot price of the 'base asset' in terms of the 'quote asset' in the pool,
	// errors if either baseAssetDenom, or quoteAssetDenom does not exist.
	// For example, if this was a UniV2 50-50 pool, with 2 ETH, and 8000 UST
	// pool.SpotPrice(ctx, "eth", "ust") = 4000.00
	SpotPrice(ctx sdk.Context, baseAssetDenom string, quoteAssetDenom string) (sdk.Dec, error)
}
  • TraditionalAmmInterface
// TraditionalAmmInterface defines an interface for pools representing traditional AMM.
type TraditionalAmmInterface interface {
	// JoinPool joins the pool using all of the tokensIn provided.
	// The AMM swaps to the correct internal ratio should be and returns the number of shares created.
	// This function is mutative and updates the pool's internal state if there is no error.
	// It is up to pool implementation if they support LP'ing at arbitrary ratios, or a subset of ratios.
	// Pools are expected to guarantee LP'ing at the exact ratio, and single sided LP'ing.
	JoinPool(ctx sdk.Context, tokensIn sdk.Coins, swapFee sdk.Dec) (numShares sdk.Int, err error)

	// JoinPoolNoSwap joins the pool with an all-asset join using the maximum amount possible given the tokensIn provided.
	// This function is mutative and updates the pool's internal state if there is no error.
	// Pools are expected to guarantee LP'ing at the exact ratio.
	JoinPoolNoSwap(ctx sdk.Context, tokensIn sdk.Coins, swapFee sdk.Dec) (numShares sdk.Int, err error)

	// CalcJoinPoolShares returns how many LP shares JoinPool would return on these arguments.
	// This does not mutate the pool, or state.
	CalcJoinPoolShares(ctx sdk.Context, tokensIn sdk.Coins, swapFee sdk.Dec) (numShares sdk.Int, newLiquidity sdk.Coins, err error)

	// CalcJoinPoolNoSwapShares returns how many LP shares JoinPoolNoSwap would return on these arguments.
	// This does not mutate the pool, or state.
	CalcJoinPoolNoSwapShares(ctx sdk.Context, tokensIn sdk.Coins, swapFee sdk.Dec) (numShares sdk.Int, newLiquidity sdk.Coins, err error)

	// ExitPool exits #numShares LP shares from the pool, decreases its internal liquidity & LP share totals,
	// and returns the number of coins that are being returned.
	// This mutates the pool and state.
	ExitPool(ctx sdk.Context, numShares sdk.Int, exitFee sdk.Dec) (exitedCoins sdk.Coins, err error)
	// CalcExitPoolCoinsFromShares returns how many coins ExitPool would return on these arguments.
	// This does not mutate the pool, or state.
	CalcExitPoolCoinsFromShares(ctx sdk.Context, numShares sdk.Int, exitFee sdk.Dec) (exitedCoins sdk.Coins, err error)
}
  • based on initial investigations, CalcJoinPoolSharesNoSwap might be eligible for being defined on the PoolI but that can be investigated post-refactor

The benefit of such refactor and generalization is that pools would reuse the common logic for handling ids, addresses and basic swaps

@p0mvn
Copy link
Member Author

p0mvn commented Oct 18, 2022

Closed by #3038

@p0mvn p0mvn closed this as completed Oct 18, 2022
Repository owner moved this from Needs Review 🔍 to Done ✅ in Osmosis Chain Development Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/gamm Changes, features and bugs related to the gamm module. T:task ⚙️ A task belongs to a story
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants