Refactor: Change Pool API to have separate methods for JoinPool and JoinPoolNoSwap
#2133
Conversation
AlpinYukseloglu
added
the
C:x/gamm
| return numShares, tokensJoined, nil | ||
| } | ||
|
|
||
| // 4) Still more coins to join, so we update the effective pool state here to account for | ||
| // join that just happened. | ||
| // * We add the joined coins to our "current pool liquidity" object (poolAssetsByDenom) | ||
| // * We increment a variable for our "newTotalShares" to add in the shares that've been added. | ||
| tokensJoined = tokensIn.Sub(remainingTokensIn) |
There was a problem hiding this comment.
Context: CalcJoinPoolNoSwapShares includes this calculation and returns the resulting tokensJoined above
|
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
x/gamm/pool-models/balancer/pool.go
Outdated
| // get all 'pool assets' (aka current pool liquidity + balancer weight) | ||
| poolAssetsByDenom, err := getPoolAssetsByDenom(p.GetAllPoolAssets()) | ||
| if err != nil { | ||
| return sdk.ZeroInt(), sdk.NewCoins(), tokensIn, err |
There was a problem hiding this comment.
I'm just going to point out which branches are not covered by tests.
@AlpinYukseloglu do you mind confirming please if it is possible to introduce test cases to cover these
This is the first branch
There was a problem hiding this comment.
This one should actually not be possible as it only fails when there are duplicate assets in a pool and it's not possible to have duplicate denoms in a valid coin set. The error check is just here as a best practice.
There was a problem hiding this comment.
Is it due to using a balancer pool constructor in TestCalcJoinPoolNoSwapShares where the constructor performs this validation?
What if we avoid it using it to trigger an invalid state and achieve full coverage?
We should be able to:
balancerPool := balancer.Pool{
Address: types.NewPoolAddress(defaultPoolId).String(),
Id: defaultPoolId,
PoolParams: balancer.PoolParams{SwapFee: defaultSwapFee, ExitFee: defaultExitFee},
PoolAssets: test.poolAssets,
FuturePoolGovernor: defaultFutureGovernor,
TotalShares: sdk.NewCoin(types.GetPoolShareDenom(defaultPoolId), types.InitPoolSharesSupply),
}
Now, we can force duplicate pool assets
There was a problem hiding this comment.
Please see the most recent case I added. It is still technically a trivial test case because duplicate pool assets are caught in multiple places throughout this function and ultimately end up in a division by zero even if you comment out all of them, but it should be covered now.
There was a problem hiding this comment.
LGTM. Great work on this @AlpinYukseloglu . Thank you for maximizing test coverage.
The remaining comments are non-blocking. However, could you please double-check the following:
- the error message about "no swap joins requiring all assets in pool". I just want to ensure that this is not exclusive of "joins with swaps" because
CalcJoinPoolNoSwapSharesis called fromCalcJoinPoolShares. - the new empty
sdk.Contextbeing passed in toMaximalExactRatioJoinfromCalcJoinPoolNoSwapShares
|
|
||
| // ensure that there aren't too many or too few assets in `tokensIn` | ||
| if tokensIn.Len() != p.NumAssets() { | ||
| return sdk.ZeroInt(), sdk.NewCoins(), errors.New("no-swap joins require LP'ing with all assets in pool") |
There was a problem hiding this comment.
Just trying to understand:
Why do you emphasize "no-swap joins"?
CalcJoinPoolNoSwapShares is called from CalcJoinPoolShares which is called from regular JoinPool that does swaps. I also noticed a similar check n CalcJoinPoolShares.
My point is that it seems that this is relevant for joins with the swaps as well.
There was a problem hiding this comment.
This specific function is for no-swap joins, so the errors returned by it specify that it is a no-swap pool join (this specific line wouldn't error if this was a join with swaps)
Even though CalcJoinPoolNoSwapShares is still called in CalcJoinPoolShares, it is called in the "no-swap"/"all-asset"/"exact ratio" join component of it, and all pool join logic related to swaps is handled elsewhere in that function
There was a problem hiding this comment.
Awesome, thank you! Just wanted to make sure that this check was intentional to have on the path for regular joins as well
Co-authored-by: Roman <[email protected]>
…osmosis into separate-join-pool
… `JoinPoolNoSwap` (#2133) * split out no swap joins on pool interface and implement changes for balancer * minor refactor and comment changes for clarity * minor comment changes for review clarity * add tests for new calcs * fix merge-related issues * add changelog entry * add failure case test and make existing tests more robust * clean up validation logic * factor out common denom check and add tests for it * add additional test cases * apply new internal fx where relevant and ensure proper testing * add test case and improve test readability * add tests for edge cases and clean up existing tests * add duplicate pool edge case test * minor comment updates from code review Co-authored-by: Roman <[email protected]> * remove remainingCoins return value from internal no-swap calcs * update mocks * move ensuredenominpool test to amm tests * minor comment updates from code review Co-authored-by: Roman <[email protected]> * pass ctx through no-swap pool joins * add multi asset and edge case tests to calcnoswap Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: Roman <[email protected]> (cherry picked from commit 4de8bdf)
… `JoinPoolNoSwap` (#2133) (#2634) * split out no swap joins on pool interface and implement changes for balancer * minor refactor and comment changes for clarity * minor comment changes for review clarity * add tests for new calcs * fix merge-related issues * add changelog entry * add failure case test and make existing tests more robust * clean up validation logic * factor out common denom check and add tests for it * add additional test cases * apply new internal fx where relevant and ensure proper testing * add test case and improve test readability * add tests for edge cases and clean up existing tests * add duplicate pool edge case test * minor comment updates from code review Co-authored-by: Roman <[email protected]> * remove remainingCoins return value from internal no-swap calcs * update mocks * move ensuredenominpool test to amm tests * minor comment updates from code review Co-authored-by: Roman <[email protected]> * pass ctx through no-swap pool joins * add multi asset and edge case tests to calcnoswap Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: Roman <[email protected]> (cherry picked from commit 4de8bdf) Co-authored-by: alpo <[email protected]> Co-authored-by: alpo <[email protected]>
Closes: #2128
What is the purpose of the change
Add
JoinPoolNoSwapandCalcJoinPoolNoSwapSharesto our GAMM pool interface to have a clear and accessible flow for no-swap joins.Brief Changelog
JoinPoolNoSwapandCalcJoinPoolNoSwapSharesto GAMM pool interfaceJoinPoolNoSwapandCalcJoinPoolNoSwapSharesin balancer packageJoinPoolNoSwapin pool_service.go to the newJoinPoolNoSwapin our pool interfaceTODO:
Testing and Verifying
This change added tests and can be verified as follows:
TestCalcJoinPoolNoSwapSharesin balancer's pool_test.goDocumentation and Release Note
Unreleasedsection inCHANGELOG.md? (yes)