-
Notifications
You must be signed in to change notification settings - Fork 608
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
feat(gamm): CalcJoinPoolShares and CalcExitPoolCoinsFromShares queries #2972
Conversation
@georgemc98 I want to clarify one thing about the second query. Currently it works by equally splitting shares among all tokens in pool and then returns respectful amount of tokens of specific denom. My question is: should it work like this or should it transform all shares just into one specific denom? |
This is more difficult than the other queries. The query estimation will be inaccurate since the liquidity is never taken out before the other token in the pool is swapped for the request token denom. This is what we have so far: https://github.com/apollodao/osmosis/tree/dev/pool-shares-rpc-queries I have a query for |
Could I push my |
Sure |
So what do you think, we leave it or remove it for good? |
I recommend that we add two endpoints, JoinPool and ExitPool. JoinPool can take a single asset, or both assets. Check out my branch where I implement |
@georgemc98 invited you as a collaborator, please accept |
x/gamm/keeper/grpc_query.go
Outdated
@@ -140,6 +140,65 @@ func (q Querier) PoolType(ctx context.Context, req *types.QueryPoolTypeRequest) | |||
}, err | |||
} | |||
|
|||
// JoinSwapExactAmountIn queries the amount of shares you get by providing specific amount of tokens | |||
func (q Querier) JoinSwapExactAmountIn(ctx context.Context, req *types.QueryJoinSwapExactAmountInRequest) (*types.QueryJoinSwapExactAmountInResponse, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we call this CalcJoinPoolShares
?
x/gamm/keeper/grpc_query.go
Outdated
}, nil | ||
} | ||
|
||
func (q Querier) ExitSwapShareAmountIn(ctx context.Context, req *types.QueryExitSwapShareAmountInRequest) (*types.QueryExitSwapShareAmountInResponse, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
godoc please
x/gamm/keeper/grpc_query.go
Outdated
|
||
totalSharesAmount := pool.GetTotalShares() | ||
if req.ShareInAmount.GTE(totalSharesAmount) || req.ShareInAmount.LTE(sdk.ZeroInt()) { | ||
return &types.QueryExitSwapShareAmountInResponse{TokenOutAmount: 0}, sdkerrors.Wrapf(types.ErrInvalidMathApprox, "share ratio is zero or negative") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please return nil
x/gamm/keeper/grpc_query.go
Outdated
|
||
exitCoins, err := pool.CalcExitPoolCoinsFromShares(sdkCtx, *req.ShareInAmount, exitFee) | ||
if err != nil { | ||
return &types.QueryExitSwapShareAmountInResponse{TokenOutAmount: 0}, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
x/gamm/keeper/grpc_query.go
Outdated
sdkCtx := sdk.UnwrapSDKContext(ctx) | ||
pool, err := q.Keeper.GetPoolAndPoke(sdkCtx, req.PoolId) | ||
if err != nil { | ||
return &types.QueryExitSwapShareAmountInResponse{TokenOutAmount: 0}, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nil please
x/gamm/keeper/grpc_query.go
Outdated
return &types.QueryExitSwapShareAmountInResponse{TokenOutAmount: coin.Amount.Uint64()}, nil | ||
} | ||
} | ||
return &types.QueryExitSwapShareAmountInResponse{}, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we return error if denom is not found?
x/gamm/keeper/grpc_query_test.go
Outdated
TokenOutDenom: "foo", | ||
ShareInAmount: &defaultShares, | ||
}) | ||
suite.Require().NotNil(out) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add table driven tests to cover all edge cases
x/gamm/keeper/grpc_query_test.go
Outdated
suite.Require().NotNil(out) | ||
suite.Require().NoError(err) | ||
} | ||
func (suite *KeeperTestSuite) TestJoinSwapExactAmountIn() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func (suite *KeeperTestSuite) TestJoinSwapExactAmountIn() { | |
func (suite *KeeperTestSuite) TestJoinSwapExactAmountIn() { |
x/gamm/keeper/grpc_query_test.go
Outdated
TokensIn: sdk.NewCoins(sdk.NewCoin("foo", sdk.NewInt(500))), | ||
}) | ||
suite.Require().NotNil(out) | ||
suite.Require().NoError(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Table driven tests with edge cases please
x/gamm/keeper/grpc_query.go
Outdated
}, nil | ||
} | ||
|
||
func (q Querier) ExitSwapShareAmountIn(ctx context.Context, req *types.QueryExitSwapShareAmountInRequest) (*types.QueryExitSwapShareAmountInResponse, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renaming to CalcExitPoolCoinsFromShares
The discussed approach of having fewer queries that take more inputs makes sense. I would like to suggest considering naming queries w/o "Join/Exit" but with "Calc". We've been trying to use the former abstraction to imply mutations. Since queries are non-mutative, "Calc" abstraction is preferable |
@georgemc98 I fixed some things right now but not finished. basically what is left is to finish with tests. If you decide to work on this issue please feel free to push to my branch (recall that I added you to collaborators) thank you! |
I will add table driven tests for |
Thank you very much! I will finish TestCalcExitPoolCoinsFromShares as soon as I can |
Sounds good, I pushed my |
@p0mvn changes addressed:
|
Excellent. I made a new branch based off this branch and created the PR here: #3217. Please let me know if there is anything else i need to do to get them whitelisted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me after discussing the filtering
x/gamm/keeper/grpc_query.go
Outdated
}, nil | ||
} | ||
|
||
// CalcExitPoolCoinsFromShares queries the amount of tokens you get by exiting specific amouint of shares |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// CalcExitPoolCoinsFromShares queries the amount of tokens you get by exiting specific amouint of shares | |
// CalcExitPoolCoinsFromShares queries the number of tokens you get by exiting a specific amount of shares. | |
// Only returns the coins requested by `TokensOutDenoms`. If `TokensOutDenoms` is empty, nothing is returned | |
// despite potentially having exiting coins. |
x/gamm/keeper/grpc_query.go
Outdated
} | ||
|
||
coinsOut := sdk.NewCoins() | ||
for _, req_denom := range req.TokensOutDenoms { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm slightly confused about the purpose of TokensOutDenoms
for filtering. Is this a requirement for this query?
Why can't we let clients filter out the denoms that they want? I think that filtering out by this input here is likely to lead to erroneous use where clients provide only one TokensOutDenoms
while there are more existing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, let me remove that parameter and remove this for loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm slightly confused about the purpose of
TokensOutDenoms
for filtering. Is this a requirement for this query?Why can't we let clients filter out the denoms that they want? I think that filtering out by this input here is likely to lead to erroneous use where clients provide only one
TokensOutDenoms
while there are more existing
I've removed this parameter and fixed the typos that you also suggested.
@georgemc98 @p0mvn @rusakh neededLpLiquidity, err := getMaximalNoSwapLPAmount(ctx, pool, shareOutAmount) before passing this result to: sharesOut, err = pool.JoinPoolNoSwap(ctx, neededLpLiquidity, pool.GetSwapFee(ctx)) which internally calls
For example, let's say I have some amounts of the assets in a pool, and use this new query |
To expand on this, the reason we would want to pass the exact amounts returned from the query to |
return nil, err | ||
} | ||
|
||
numShares, newLiquidity, err := pool.CalcJoinPoolShares(sdkCtx, req.TokensIn, pool.GetSwapFee(sdkCtx)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apollo-sturdy if you are planning to pass the result of this query to JoinPoolNoSwap
, I suggest using CalcJoinPoolNoSwap
method here instead.
We should also then rename this query to CalcJoinPoolNoSwapShares
.
The difference between CalcJoinPoolShares
and CalcJoinPoolNoSwapShares
is that the latter calculates the number of shares that we get from the given tokens without performing a swap.
On the other hand, CalcJoinPoolShares
first joins without a swap, and then single asset joins any remaining coins. As a result, it is important to use the right calc method here.
I'm unaware of how you are planning to use the query and only reviewing for soundness as it is, not the integrations. Please let me know if there are other planned uses that we haven't discussed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see now looking closer at CalcJoinPoolShares
. Well in that case we would need two queries. As per the original issue #2956 what we actually want is the ability to simulate each type of liquidity provision (MsgJoinPool
and MsgJoinSwapExternAmountIn
). These are the only use cases I'm aware of or can think of anyone wanting.
It looks like for MsgJoinSwapExternAmountIn
it only really does CalcJoinPoolShares
, so the current query that this PR introduces should work as an exact simulation of MsgJoinSwapExternAmountIn
?
For MsgJoinPool
as mentioned it seems to do more than just CalcJoinPoolNoSwapShares
. Would you be able to help with introducing an exact simulation query for this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For MsgJoinPool as mentioned it seems to do more than just CalcJoinPoolNoSwapShares. Would you be able to help with introducing an exact simulation query for this function?
I think we should be able to do the following:
- create a new query
CalcJoinPoolNoSwapShares
similar to the one introduced in this PR - add this logic to get maximal no swap lp amounts
- call pool's
CalcJoinPoolNoSwapShares
- add table-driven unit tests
- ideally, add a functional test that makes sure that this new query behaves exactly the same as
JoinPoolNoSwap
- I think this can be also tested contract side so is optional
@rusakh would you be interested in making a follow-up PR with this new query?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I would like to work on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. Please feel free to make a follow-up issue and start working on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a problem in getMaximalNoSwapLPAmount
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totalSharesAmount := pool.GetTotalShares()
// shareRatio is the desired number of shares, divided by the total number of
// shares currently in the pool. It is intended to be used in scenarios where you want
shareRatio := shareOutAmount.ToDec().QuoInt(totalSharesAmount)
if shareRatio.LTE(sdk.ZeroDec()) || shareRatio.GT(sdk.OneDec()) {
return sdk.Coins{}, sdkerrors.Wrapf(types.ErrInvalidMathApprox, "share ratio is zero or negative")
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It only checks if share ratio is LTE 0, I think it should also check if it is bigger than one, because in case it is >= 1, you are trying to get more shares than there is available. Wdyt @p0mvn @apollo-sturdy ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It only checks if share ratio is LTE 0, I think it should also check if it is bigger than one, because in case it is >= 1, you are trying to get more shares than there is available. Wdyt @p0mvn @apollo-sturdy ?
Why should you not be able to provide more liquidity than is already in the pool? The amount of "shares available" is infinite, since they are minted upon liquidity provision?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It only checks if share ratio is LTE 0, I think it should also check if it is bigger than one, because in case it is >= 1, you are trying to get more shares than there is available. Wdyt @p0mvn @apollo-sturdy ?
Why should you not be able to provide more liquidity than is already in the pool? The amount of "shares available" is infinite, since they are minted upon liquidity provision?
indeed, thank you for clarification
hello @p0mvn. Are you sure we need to update sdk version in go.mod in this PR? |
Yes, that should be fine. We use a replace directive for cosmos-sdk so the true version is determined by that. Thanks for checking though |
I'm going to merge this. |
#2972) * JoinSwapExactAmountIn query * ExitSwapShareAmountIn query * JoinSwapExactAmountIn, ExitSwapShareAmountIn queries * JoinSwapExactAmountIn, ExitSwapShareAmountIn queries * JoinSwapExactAmountIn, ExitSwapShareAmountIn queries * JoinSwapExactAmountIn, ExitSwapShareAmountIn queries * JoinSwapExactAmountIn, ExitSwapShareAmountIn queries * JoinSwapExactAmountIn, ExitSwapShareAmountIn queries * save * test: finish CalcJoinPoolShares query test * tests for CalcExitPoolCoinsFromShares * tests for CalcExitPoolCoinsFromShares * test: add additional exit coin tests * Update x/gamm/keeper/grpc_query.go Co-authored-by: Aleksandr Bezobchuk <[email protected]> * Update x/gamm/keeper/grpc_query.go Co-authored-by: Aleksandr Bezobchuk <[email protected]> * Update x/gamm/keeper/grpc_query.go Co-authored-by: Aleksandr Bezobchuk <[email protected]> * queries * docs fix * chore: generate protobuf files * feat: remove TokensOutDenoms, fix typos * Update x/gamm/keeper/grpc_query_test.go Co-authored-by: Ruslan Akhtariev <[email protected]> Co-authored-by: George McAskill <[email protected]> Co-authored-by: Roman <[email protected]> Co-authored-by: Aleksandr Bezobchuk <[email protected]> (cherry picked from commit 94534da) # Conflicts: # CHANGELOG.md
} | ||
|
||
sdkCtx := sdk.UnwrapSDKContext(ctx) | ||
pool, err := q.Keeper.getPoolForSwap(sdkCtx, req.PoolId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For balancer pools, this method may call PokePool()
which updates pool weights. Is this acceptable for a stargate query?
CC: @ValarDragon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is fine, because:
- this won't get saved
- Even if it did, cosmwasm would revert the result
#2972) * JoinSwapExactAmountIn query * ExitSwapShareAmountIn query * JoinSwapExactAmountIn, ExitSwapShareAmountIn queries * JoinSwapExactAmountIn, ExitSwapShareAmountIn queries * JoinSwapExactAmountIn, ExitSwapShareAmountIn queries * JoinSwapExactAmountIn, ExitSwapShareAmountIn queries * JoinSwapExactAmountIn, ExitSwapShareAmountIn queries * JoinSwapExactAmountIn, ExitSwapShareAmountIn queries * save * test: finish CalcJoinPoolShares query test * tests for CalcExitPoolCoinsFromShares * tests for CalcExitPoolCoinsFromShares * test: add additional exit coin tests * Update x/gamm/keeper/grpc_query.go Co-authored-by: Aleksandr Bezobchuk <[email protected]> * Update x/gamm/keeper/grpc_query.go Co-authored-by: Aleksandr Bezobchuk <[email protected]> * Update x/gamm/keeper/grpc_query.go Co-authored-by: Aleksandr Bezobchuk <[email protected]> * queries * docs fix * chore: generate protobuf files * feat: remove TokensOutDenoms, fix typos * Update x/gamm/keeper/grpc_query_test.go Co-authored-by: Ruslan Akhtariev <[email protected]> Co-authored-by: George McAskill <[email protected]> Co-authored-by: Roman <[email protected]> Co-authored-by: Aleksandr Bezobchuk <[email protected]> (cherry picked from commit 94534da) # Conflicts: # CHANGELOG.md # x/gamm/keeper/grpc_query.go # x/gamm/keeper/grpc_query_test.go # x/gamm/types/query.pb.go
Closes: #2956
What is the purpose of the change
Adds two queries:
1 JoinSwapExactAmountIn allows you to query the amount of shares you get by providing X tokens
2 ExitSwapShareAmountIn allows you to query the amount of tokens you get when exiting pool with X shares
Testing and Verifying
added two tests
Documentation and Release Note
Unreleased
section inCHANGELOG.md
? yes