feat(gamm): JoinSwapExactAmountIn and ExitSwapShareAmountIn queries

[pysel]

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

• Does this pull request introduce a new feature or user-facing behavior changes? yes
• Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? yes
• How is the feature or change documented? changelog

[pysel]

@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?

[georgemc98]

@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 QueryExitPool and I'm debating if we even need the QueryExitSwapShareAmountIn now.

[georgemc98]

@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?

Could I push my QueryExitPool message into your branch?

[pysel]

@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?

Could I push my QueryExitPool message into your branch?

Sure

[pysel]

@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 QueryExitPool and I'm debating if we even need the QueryExitSwapShareAmountIn now.

So what do you think, we leave it or remove it for good?

[georgemc98]

@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 QueryExitPool and I'm debating if we even need the QueryExitSwapShareAmountIn now.

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 ExitPool and JoinPool. Your JoinSwapExactAmountIn query is identical to my JoinPool query. I think it would be better to name it JoinPool for simplicity's sake. I also recommend that QueryJoinPoolResponse returns both the shares and tokenIn, which is the coins accepted by the gamm module. I cannot push to your branch, so could you allow me access please?

[pysel]

@georgemc98 invited you as a collaborator, please accept

[p0mvn]

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

[pysel]

@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!

[georgemc98]

@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 TestCalcJoinPoolShares today.

[pysel]

@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 TestCalcJoinPoolShares today.

Thank you very much! I will finish TestCalcExitPoolCoinsFromShares as soon as I can

[georgemc98]

@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 TestCalcJoinPoolShares today.

Thank you very much! I will finish TestCalcExitPoolCoinsFromShares as soon as I can

Sounds good, I pushed my CalcExitPoolCoinsFromShares test earlier

[pacmanifold]

@p0mvn @alexanderbez what is the process to determine whether these queries can be added to the stargate whitelist or not?

[alexanderbez]

@p0mvn @alexanderbez what is the process to determine whether these queries can be added to the stargate whitelist or not?

AFAIK, they have to be deterministic and constant in gas usage.

[georgemc98]

@p0mvn @alexanderbez what is the process to determine whether these queries can be added to the stargate whitelist or not?

AFAIK, they have to be deterministic and constant in gas usage.

@alexanderbez Is there anything I should do on my end to initiate these queries being added to the StargateQuery whitelist?

[pacmanifold]

@p0mvn @alexanderbez what is the process to determine whether these queries can be added to the stargate whitelist or not?

AFAIK, they have to be deterministic and constant in gas usage.

@alexanderbez Is there anything I should do on my end to initiate these queries being added to the StargateQuery whitelist?

@p0mvn @alexanderbez any update on this?

[georgemc98]

@alexanderbez @RusAkh I'm seeing there is a merge conflict now in the x/gamm/types/errors.go file. Would you like me to resolve it, or will the Osmosis team fix it?

[alexanderbez]

Can you resolve the conflicts?

[georgemc98]

Can you resolve the conflicts?

Yes, happy to.

[apollo-sturdy]

@alexanderbez @p0mvn Should whitelisting of these queries be done in this PR or do we need to create a separate PR for that? Have you guys reviewed the determinism and whatever else is needed to whitelist it?

[p0mvn]

@alexanderbez @p0mvn Should whitelisting of these queries be done in this PR or do we need to create a separate PR for that? Have you guys reviewed the determinism and whatever else is needed to whitelist it?

This looks good to me to whitelist. However, let's do a separate PR to get more eyes from the rest of the team.

[pysel]

@p0mvn changes addressed:

• now takes a []string with denoms and returns sdk.Coins

[georgemc98]

@alexanderbez @p0mvn Should whitelisting of these queries be done in this PR or do we need to create a separate PR for that? Have you guys reviewed the determinism and whatever else is needed to whitelist it?

This looks good to me to whitelist. However, let's do a separate PR to get more eyes from the rest of the team.

Excellent. I made a new branch based off this branch and created the PR here: https://github.com/osmosis-labs/osmosis/pull/3217. Please let me know if there is anything else i need to do to get them whitelisted.

[apollo-sturdy]

@georgemc98 @p0mvn @RusAkh I noticed that in JoinPoolNoSwap it first does:

neededLpLiquidity, err := getMaximalNoSwapLPAmount(ctx, pool, shareOutAmount)

before passing this result to:

sharesOut, err = pool.JoinPoolNoSwap(ctx, neededLpLiquidity, pool.GetSwapFee(ctx))

which internally calls CalcJoinPoolNoSwapShares which this new query also calls.

getMaximalNoSwapLPAmount seems to calculate the reverse of CalcJoinPoolNoSwapShares, so in theory the use case of being able to use this query to get sharesOut and pass this value to JoinPoolNoSwap should work, but have we verified that there are no rounding issues or similar that would lead to issues?

For example, let's say I have some amounts of the assets in a pool, and use this new query QueryCalcJoinPoolSharesRequest to receive a share_out_amount and a tokens_out (should maybe be called tokens_used...?). Now I call MsgJoinPool with arguments ShareOutAmount = share_out_amount returned from the query and TokenInMaxs = tokens_out returned from the query, will this work without erroring due to rounding differences between CalcJoinPoolNoSwapShares and getMaximalNoSwapLPAmount?

[apollo-sturdy]

For example, let's say I have some amounts of the assets in a pool, and use this new query QueryCalcJoinPoolSharesRequest to receive a share_out_amount and a tokens_out (should maybe be called tokens_used...?). Now I call MsgJoinPool with arguments ShareOutAmount = share_out_amount returned from the query and TokenInMaxs = tokens_out returned from the query, will this work without erroring due to rounding differences between CalcJoinPoolNoSwapShares and getMaximalNoSwapLPAmount?

To expand on this, the reason we would want to pass the exact amounts returned from the query to MsgJoinPool is so that we can be sure that exactly this amount of tokens is drawn from our account, so that we can then do a single sided join with the remaining amount. Of course it would be great if this functionality was supported out of the box, but @ValarDragon recommended that we implement it at the smart contract level (https://github.com/osmosis-labs/osmosis/issues/2994#issuecomment-1289240340), so for that to work this query needs to be an exact simulation.

[pysel]

hello @p0mvn. Are you sure we need to update sdk version in go.mod in this PR?

[p0mvn]

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

[p0mvn]

I'm going to merge this. CalcJoinPoolNoSwapShares can be addressed in a follow-up PR