Extract functions and methods from CalcJoinPoolShares; unit tests #1721

p0mvn · 2022-06-08T23:46:03Z

Closes: #XXX

What is the purpose of the change

Extract functions and methods in CalcJoinPoolShares to be able to unit test each one separately. Add more comments explaining steps in detail. Add unit tests for the newly extracted functions.

Functions that were created:

getPoolAssetsByDenom
updateIntermediaryPoolAssets
calcJoinMultipleSingleAssetTokensIn

Additionally, added calcSingleAssetJoin test cases with various inputs. For each test case, verified the expected value with Wolfram. Most values are mismatched by an insignificant number that is checked with multiplicative ErrTolerance of "0.0000001"

Spotted that there was a bug with ErrTolerance related to when the additive difference is nil so made a quick fix

Testing and Verifying

This change added tests and can be verified as follows:

go test -timeout 30s -run ^TestGetPoolAssetsByDenom$ github.com/osmosis-labs/osmosis/v7/x/gamm/pool-models/balancer
go test -timeout 30s -run ^TestUpdateIntermediaryPoolAssets$ github.com/osmosis-labs/osmosis/v7/x/gamm/pool-models/balancer
go test -timeout 30s -run ^TestCalcSingleAssetJoin$ github.com/osmosis-labs/osmosis/v7/x/gamm/pool-models/balancer
go test -timeout 30s -run ^TestCalcSingleAssetJoin$ github.com/osmosis-labs/osmosis/v7/x/gamm/pool-models/balancer
go test -timeout 30s -run ^TestCalcJoinSingleAssetTokensIn$ github.com/osmosis-labs/osmosis/v7/x/gamm/pool-models/balancer

Documentation and Release Note

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

osmoutils/binary_search.go

x/gamm/pool-models/balancer/amm.go

p0mvn · 2022-06-09T03:47:52Z

x/gamm/pool-models/balancer/amm.go

+		numShares.Add(newNumSharesFromRemaining)
+		newLiquidity = newLiquidity.Add(newLiquidityFromRemaining...)


Unit tests for CalcJoinPoolShares validating that these updates happen should be added in a separate PR

stackman27

Thank you for the PR @p0mvn. Left few notes, really curious whether the calculation for multi-asset is accurate as i was trying to figure that out for hours today 😅

Thank you for all the human readable equation + wolfram too. Really helpful!

x/gamm/pool-models/balancer/amm.go

x/gamm/pool-models/balancer/amm_test.go

stackman27 · 2022-06-09T06:01:46Z

x/gamm/pool-models/balancer/amm_test.go

+				},
+			},
+			tokensIn:     sdk.NewCoins(sdk.NewInt64Coin("uosmo", 50_000), sdk.NewInt64Coin("uatom", 50_000)),
+			expectShares: sdk.NewInt(2_499_999_968_750 * 2),


Are you sure you can add both shares to calculate total shares for two asset deposit ?

cc @AlpinYukseloglu @alexanderbez

As per my comment above, https://github.com/osmosis-labs/osmosis/pull/1721/files#r893463274

here, we can add shares because these are essentially done by single asset joining each token.

Seems reasonable to me.

I'm confused as to why expectShares is double what is in the previous test when the total liquidity isn't being doubled. From the balancer whitepaper:

An “all-asset” deposit has to follow the distribution of existing assets in the pool. If the deposit contains 10%
of each of the assets already in the pool, then the Value Function will increase by 10% and the depositor will
be minted 10% of the current outstanding pool token supply.

Also as per my comment above, it's not clear to me why we're implementing/testing a case where we single asset join two assets into a 2-asset pool when MaximalExactRatioJoin would do the trick with no swap fees (and would be the intended/expected behavior when a pool is joined) – am I missing something @p0mvn?

I'm confused as to why expectShares is double what is in the previous test when the total liquidity isn't being doubled

The logic was the following - here we are adding 2 tokens in that have equal weights and amounts in, so the liquidity would double. In the previous case, there was only one token in.

However, it seems that these should still not be equal because after the first join we would have more shares created for one of the tokens, right? As a result, the second join would have fewer shares returned than the first one. Seems that this test is passing because of the error tolerance

Also as per my comment above, it's not clear to me why we're implementing/testing a case where we single asset join two assets into a 2-asset pool when MaximalExactRatioJoin would do the trick with no swap fees (and would be the intended/expected behavior when a pool is joined) – am I missing something

The goal was to make the function be independent of what the caller might do. I think the suggested check is useful but should belong to the caller of this function. Let me know what you think

I'm confused as to why expectShares is double what is in the previous test when the total liquidity isn't being doubled

The logic was the following - here we are adding 2 tokens in that have equal weights and amounts in, so the liquidity would double. In the previous case, there was only one token in.

However, it seems that these should still not be equal because after the first join we would have more shares created for one of the tokens, right? As a result, the second join would have fewer shares returned than the first one. Seems that this test is passing because of the error tolerance

This is interesting – I wonder if we can set up a case where this falls outside of error tolerance just for proper coverage? Or would it always pass due to error tolerance?

Also as per my comment above, it's not clear to me why we're implementing/testing a case where we single asset join two assets into a 2-asset pool when MaximalExactRatioJoin would do the trick with no swap fees (and would be the intended/expected behavior when a pool is joined) – am I missing something

The goal was to make the function be independent of what the caller might do. I think the suggested check is useful but should belong to the caller of this function. Let me know what you think

I hear you on this, agreed!

x/gamm/pool-models/balancer/amm_test.go

x/gamm/pool-models/balancer/amm.go

AlpinYukseloglu · 2022-06-10T01:23:18Z

x/gamm/pool-models/balancer/amm.go

+// calcJoinSingleAssetTokensIn attempts to calculate single
+// asset join for all tokensIn given totalShares in pool,
+// poolAssetsByDenom and swapFee. totalShares is the number
+// of shares in pool before beginnning to join any of the tokensIn.
+//
+// Returns totalNewShares and totalNewLiquidity from joining all tokensIn
+// by mimicking individually single asset joining each.
+// or error if fails to calculate join for any of the tokensIn.
+func (p *Pool) calcJoinSingleAssetTokensIn(tokensIn sdk.Coins, totalShares sdk.Int, poolAssetsByDenom map[string]PoolAsset, swapFee sdk.Dec) (sdk.Int, sdk.Coins, error) {


Am I correct in saying that this function assumes len(tokensIn) < len(poolAssets) i.e. that the number of tokens in tokensIn is less than the total number of assets/denoms in the pool since otherwise, we would run MaximalExactRatioJoin instead of running calcSingleAssetJoin on each asset?

If so, it might be worth enforcing this with a check at the beginning of the function so that we only have one valid way of doing a multi-asset join: MaximalExactRatioJoin following by calcJoinSingleAssetTokensIn on the remaining assets

While I agree that this check is useful, I think that we should have it in the caller of this function, not inside this function itself.

A function should work independently of its caller. Otherwise, we would add an invariant that might restrict its use in other cases.

I will investigate this further

What do you think about having this check here:
f6071c9

A function should work independently of its caller. Otherwise, we would add an invariant that might restrict its use in other cases.

Generally agree with this - probably a good idea to leave it as it is to not break abstraction

What do you think about having this check here: f6071c9

This addresses a separate but important point & lets us have an error case we can use for testing. Great addition

x/gamm/pool-models/balancer/amm_test.go

AlpinYukseloglu · 2022-06-10T01:41:58Z

x/gamm/pool-models/balancer/amm_test.go

+				},
+			},
+			tokensIn:     sdk.NewCoins(sdk.NewInt64Coin("uosmo", 50_000), sdk.NewInt64Coin("uatom", 50_000)),
+			expectShares: sdk.NewInt(2_499_999_968_750 * 2),


I'm confused as to why expectShares is double what is in the previous test when the total liquidity isn't being doubled. From the balancer whitepaper:

An “all-asset” deposit has to follow the distribution of existing assets in the pool. If the deposit contains 10%
of each of the assets already in the pool, then the Value Function will increase by 10% and the depositor will
be minted 10% of the current outstanding pool token supply.

Also as per my comment above, it's not clear to me why we're implementing/testing a case where we single asset join two assets into a 2-asset pool when MaximalExactRatioJoin would do the trick with no swap fees (and would be the intended/expected behavior when a pool is joined) – am I missing something @p0mvn?

AlpinYukseloglu · 2022-06-10T01:43:39Z

x/gamm/pool-models/balancer/amm_test.go

+				},
+			},
+			tokensIn:     sdk.NewCoins(sdk.NewInt64Coin("uosmo", 50_000), sdk.NewInt64Coin("uatom", 50_000)),
+			expectShares: sdk.NewInt(2_487_500_000_000 * 2),


Same question as my comment above

Will resolve this once figure out an appropriate path forward

x/gamm/pool-models/balancer/amm_test.go

…setJoin

ValarDragon · 2022-06-10T05:44:21Z

x/gamm/pool-models/balancer/amm.go

-	// update liquidity for accurate calcSingleAssetJoin calculation
-	newLiquidity = tokensIn.Sub(remCoins)
-	for _, coin := range newLiquidity {
-		poolAsset := poolAssetsByDenom[coin.Denom]
-		poolAsset.Token.Amount = poolAssetsByDenom[coin.Denom].Token.Amount.Add(coin.Amount)
-		poolAssetsByDenom[coin.Denom] = poolAsset
-	}


Comment for posterity, these lines were moved to line 306

* Improve comments / abstractions * Add eror * Add swap fee note * Update x/gamm/pool-models/balancer/amm.go Co-authored-by: Aleksandr Bezobchuk <[email protected]> * More comments Co-authored-by: Aleksandr Bezobchuk <[email protected]>

ValarDragon · 2022-06-10T21:32:44Z

LGTM to merge after the last few commits

) * Fix insufficient total share creation * More correctx * small refactor to expose methods for unit tests * fix problem with newLiquidity, better names and more comments * fix typo * add TestCalcSingleAssetJoin * add test case to TestCalcSingleAssetJoin * add test case to TestCalcSingleAssetJoin * add another Wolfram test case to TestCalcSingleAssetJoin * another test case for TestCalcSingleAssetJoin * remove errors from TestCalcSingleAssetJoin * add TestCalcJoinMultipleSingleAssetTokensIn * add complex case for TestCalcJoinMultipleSingleAssetTokensIn * add assetions for liquidity * add no tokens in TestCalcJoinMultipleSingleAssetTokensIn * update newLiquidity * remove else clause * currTotalShares * rename to CalcJoinSingleAssetTokensIn * refactor shares logic in calcJoinSingleAssetTokensIn * fix comment in test * update numShares * use context from parameter * Update x/gamm/pool-models/balancer/amm_test.go Co-authored-by: Matt, Park <[email protected]> * Update x/gamm/pool-models/balancer/amm_test.go Co-authored-by: Matt, Park <[email protected]> * rename remainingTokensIn * remove TestCalcSingleAssetJoin * copy godoc for CalcJoinPoolShares from pool interface * Update x/gamm/pool-models/balancer/amm.go Co-authored-by: Aleksandr Bezobchuk <[email protected]> * move getPoolAssetsByDenom function and test to another PR * simplify to use recently merged GetPoolAssetsByDenom function * remove redundant time * typo * update name to updatePoolAssetsLiquidity * add validation for giving tokenIn that is not in pool to calcSingleAssetJoin * specify that balancer equation is modified with swapFeeRatio added * Fix merge conflict I introduced * Improve comments / abstractions (#1761) * Improve comments / abstractions * Add eror * Add swap fee note * Update x/gamm/pool-models/balancer/amm.go Co-authored-by: Aleksandr Bezobchuk <[email protected]> * More comments Co-authored-by: Aleksandr Bezobchuk <[email protected]> * mild test cleanup Co-authored-by: Dev Ojha <[email protected]> Co-authored-by: Matt, Park <[email protected]> Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: Dev Ojha <[email protected]> (cherry picked from commit 9432b92)

) (#1766) * Fix insufficient total share creation * More correctx * small refactor to expose methods for unit tests * fix problem with newLiquidity, better names and more comments * fix typo * add TestCalcSingleAssetJoin * add test case to TestCalcSingleAssetJoin * add test case to TestCalcSingleAssetJoin * add another Wolfram test case to TestCalcSingleAssetJoin * another test case for TestCalcSingleAssetJoin * remove errors from TestCalcSingleAssetJoin * add TestCalcJoinMultipleSingleAssetTokensIn * add complex case for TestCalcJoinMultipleSingleAssetTokensIn * add assetions for liquidity * add no tokens in TestCalcJoinMultipleSingleAssetTokensIn * update newLiquidity * remove else clause * currTotalShares * rename to CalcJoinSingleAssetTokensIn * refactor shares logic in calcJoinSingleAssetTokensIn * fix comment in test * update numShares * use context from parameter * Update x/gamm/pool-models/balancer/amm_test.go Co-authored-by: Matt, Park <[email protected]> * Update x/gamm/pool-models/balancer/amm_test.go Co-authored-by: Matt, Park <[email protected]> * rename remainingTokensIn * remove TestCalcSingleAssetJoin * copy godoc for CalcJoinPoolShares from pool interface * Update x/gamm/pool-models/balancer/amm.go Co-authored-by: Aleksandr Bezobchuk <[email protected]> * move getPoolAssetsByDenom function and test to another PR * simplify to use recently merged GetPoolAssetsByDenom function * remove redundant time * typo * update name to updatePoolAssetsLiquidity * add validation for giving tokenIn that is not in pool to calcSingleAssetJoin * specify that balancer equation is modified with swapFeeRatio added * Fix merge conflict I introduced * Improve comments / abstractions (#1761) * Improve comments / abstractions * Add eror * Add swap fee note * Update x/gamm/pool-models/balancer/amm.go Co-authored-by: Aleksandr Bezobchuk <[email protected]> * More comments Co-authored-by: Aleksandr Bezobchuk <[email protected]> * mild test cleanup Co-authored-by: Dev Ojha <[email protected]> Co-authored-by: Matt, Park <[email protected]> Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: Dev Ojha <[email protected]> (cherry picked from commit 9432b92) Co-authored-by: Roman <[email protected]>

ValarDragon and others added 3 commits June 8, 2022 11:55

Fix insufficient total share creation

57b3a0b

More correctx

73f12d5

small refactor to expose methods for unit tests

9f59d21

github-actions bot added the C:x/gamm Changes, features and bugs related to the gamm module. label Jun 8, 2022

fix problem with newLiquidity, better names and more comments

930df15

p0mvn mentioned this pull request Jun 9, 2022

Fix second source of (minor) over-LP shares #1716

Merged

fix typo

fe0eaa9

p0mvn changed the title ~~small refactor to expose methods for unit tests~~ Extract functions and methods from CalcJoinPoolShares; unit tests Jun 9, 2022

p0mvn added 6 commits June 8, 2022 21:06

add TestCalcSingleAssetJoin

37f7ffb

add test case to TestCalcSingleAssetJoin

efae99b

add test case to TestCalcSingleAssetJoin

f9dc755

add another Wolfram test case to TestCalcSingleAssetJoin

81c7dfd

another test case for TestCalcSingleAssetJoin

ddc2f5d

remove errors from TestCalcSingleAssetJoin

dd6ffd7

p0mvn mentioned this pull request Jun 9, 2022

chore: CalcJoinPoolShares unit tests with single token validated by Wolfram #1718

Closed

p0mvn added 4 commits June 8, 2022 22:11

add TestCalcJoinMultipleSingleAssetTokensIn

9a683b6

add complex case for TestCalcJoinMultipleSingleAssetTokensIn

35bc16e

add assetions for liquidity

4d2c14b

add no tokens in TestCalcJoinMultipleSingleAssetTokensIn

b6767cb

p0mvn marked this pull request as ready for review June 9, 2022 02:55

p0mvn requested a review from a team June 9, 2022 02:55

alexanderbez reviewed Jun 9, 2022

View reviewed changes

osmoutils/binary_search.go Outdated Show resolved Hide resolved

x/gamm/pool-models/balancer/amm.go Outdated Show resolved Hide resolved

x/gamm/pool-models/balancer/amm.go Outdated Show resolved Hide resolved

p0mvn added 4 commits June 8, 2022 23:42

update newLiquidity

70b1f88

remove else clause

b94494f

currTotalShares

62b39d8

rename to CalcJoinSingleAssetTokensIn

5091e3f

p0mvn commented Jun 9, 2022

View reviewed changes

p0mvn mentioned this pull request Jun 9, 2022

Validate updates to numShares and newLiquidity in CalcJoinPoolShares #1722

Closed

stackman27 reviewed Jun 9, 2022

View reviewed changes

refactor shares logic in calcJoinSingleAssetTokensIn

dbefea1

ValarDragon reviewed Jun 9, 2022

View reviewed changes

x/gamm/pool-models/balancer/amm_test.go Outdated Show resolved Hide resolved

p0mvn mentioned this pull request Jun 9, 2022

test: extract getPoolAssetsByDenom function from CalcJoinPoolShares and unit test #1738

Merged

move getPoolAssetsByDenom function and test to another PR

65ec784

mattverse self-requested a review June 9, 2022 23:15

mattverse approved these changes Jun 9, 2022

View reviewed changes

p0mvn added 2 commits June 9, 2022 19:29

Merge branch 'main' into roman/second-bug-unit-tests

fbe0590

simplify to use recently merged GetPoolAssetsByDenom function

1aac808

AlpinYukseloglu reviewed Jun 10, 2022

View reviewed changes

p0mvn added 5 commits June 9, 2022 22:15

remove redundant time

00ee003

typo

ddc01f6

update name to updatePoolAssetsLiquidity

111c174

add validation for giving tokenIn that is not in pool to calcSingleAs…

63334c8

…setJoin

specify that balancer equation is modified with swapFeeRatio added

eccdaf1

ValarDragon reviewed Jun 10, 2022

View reviewed changes

AlpinYukseloglu mentioned this pull request Jun 10, 2022

Make separate methods for single vs multi tokensIn calc. #1745

Closed

Merge branch 'main' into roman/second-bug-unit-tests

d645ed4

p0mvn mentioned this pull request Jun 10, 2022

test: ensure tokenIn exists in pool for calcSingleAssetJoin #1751

Merged

ValarDragon and others added 5 commits June 10, 2022 13:23

Merge branch 'main' into roman/second-bug-unit-tests

79c1cb8

Fix merge conflict I introduced

2c19b8f

Improve comments / abstractions (#1761)

f59ec70

* Improve comments / abstractions * Add eror * Add swap fee note * Update x/gamm/pool-models/balancer/amm.go Co-authored-by: Aleksandr Bezobchuk <[email protected]> * More comments Co-authored-by: Aleksandr Bezobchuk <[email protected]>

mild test cleanup

00d9d0f

Merge branch 'main' into roman/second-bug-unit-tests

bfcfb60

ValarDragon merged commit 9432b92 into main Jun 10, 2022

ValarDragon deleted the roman/second-bug-unit-tests branch June 10, 2022 23:25

mergify bot mentioned this pull request Jun 10, 2022

Extract functions and methods from CalcJoinPoolShares; unit tests (backport #1721) #1766

Merged

This was referenced Jun 11, 2022

Create TestCalcJoinSingleAssetTokensIn test case to fall outside of the error tolerance #1773

Open

Add the check suggested in linked discussion #1774

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract functions and methods from CalcJoinPoolShares; unit tests #1721

Extract functions and methods from CalcJoinPoolShares; unit tests #1721

p0mvn commented Jun 8, 2022 •

edited

Loading

p0mvn Jun 9, 2022

p0mvn Jun 9, 2022

stackman27 left a comment

stackman27 Jun 9, 2022 •

edited

Loading

p0mvn Jun 9, 2022

alexanderbez Jun 9, 2022

AlpinYukseloglu Jun 10, 2022 •

edited

Loading

p0mvn Jun 10, 2022

p0mvn Jun 10, 2022

AlpinYukseloglu Jun 10, 2022 •

edited

Loading

AlpinYukseloglu Jun 10, 2022

AlpinYukseloglu Jun 10, 2022

p0mvn Jun 10, 2022

p0mvn Jun 10, 2022

AlpinYukseloglu Jun 10, 2022

AlpinYukseloglu Jun 10, 2022 •

edited

Loading

AlpinYukseloglu Jun 10, 2022 •

edited

Loading

p0mvn Jun 10, 2022

ValarDragon Jun 10, 2022

ValarDragon commented Jun 10, 2022

		numShares.Add(newNumSharesFromRemaining)
		newLiquidity = newLiquidity.Add(newLiquidityFromRemaining...)

Extract functions and methods from CalcJoinPoolShares; unit tests #1721

Extract functions and methods from CalcJoinPoolShares; unit tests #1721

Conversation

p0mvn commented Jun 8, 2022 • edited Loading

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stackman27 left a comment

Choose a reason for hiding this comment

stackman27 Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlpinYukseloglu Jun 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlpinYukseloglu Jun 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlpinYukseloglu Jun 10, 2022 • edited Loading

Choose a reason for hiding this comment

AlpinYukseloglu Jun 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon commented Jun 10, 2022

p0mvn commented Jun 8, 2022 •

edited

Loading

stackman27 Jun 9, 2022 •

edited

Loading

AlpinYukseloglu Jun 10, 2022 •

edited

Loading

AlpinYukseloglu Jun 10, 2022 •

edited

Loading

AlpinYukseloglu Jun 10, 2022 •

edited

Loading

AlpinYukseloglu Jun 10, 2022 •

edited

Loading