feat: scaling factors validation in create stableswap pool, small refactor and unit tests #1459

p0mvn · 2022-05-10T04:44:02Z

Closes: #1392

What is the purpose of the change

Implemented scaling factors for CreateStableswapPool, NewStableswapPool, along with ValidateBasic validation.

Refactored to make it more testable and align the design closer to balancer.

All changed logic is covered by unit tests.

Brief Changelog

Testing and Verifying

This change introduces a scaling factors to CreateSta

includes a minor refactor to introduce PoolAssets in stableswap pool to make the design more aligned with the balancer and more testable.

This change added tests and can be verified as follows:

go test github.com/osmosis-labs/osmosis/v7/x/gamm/pool-models/stableswap

Documentation and Release Note

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

…ith the balancer

codecov-commenter · 2022-05-10T04:48:53Z

Codecov Report

Merging #1459 (c6f7ba2) into main (f56fbe5) will decrease coverage by 0.18%.
The diff coverage is 23.74%.

@@            Coverage Diff             @@
##             main    #1459      +/-   ##
==========================================
- Coverage   19.82%   19.64%   -0.19%     
==========================================
  Files         202      230      +28     
  Lines       27685    31617    +3932     
==========================================
+ Hits         5489     6211     +722     
- Misses      21175    24281    +3106     
- Partials     1021     1125     +104

Impacted Files	Coverage Δ
x/epochs/client/cli/query.go	`0.00% <ø> (ø)`
x/gamm/client/cli/query.go	`36.74% <0.00%> (-0.36%)`	⬇️
x/gamm/pool-models/balancer/balancer_pool.go	`64.37% <0.00%> (ø)`
x/gamm/pool-models/stableswap/tx.pb.go	`0.77% <0.00%> (ø)`
x/gamm/types/pool.go	`0.00% <ø> (ø)`
x/incentives/client/cli/query.go	`0.00% <ø> (ø)`
x/incentives/keeper/distribute.go	`61.00% <0.00%> (ø)`
x/incentives/keeper/hooks.go	`50.00% <0.00%> (ø)`
x/incentives/module.go	`51.11% <0.00%> (+1.11%)`	⬆️
x/lockup/client/cli/query.go	`51.99% <0.00%> (-3.00%)`	⬇️
... and 60 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a8685a...c6f7ba2. Read the comment docs.

…sis into roman/scaling-factors2

p0mvn · 2022-05-10T07:50:42Z

x/gamm/pool-models/stableswap/msgs.go

+
+func ValidatePoolAssets(poolAssets []PoolAsset) error {
+	// validation for pool initial liquidity
+	// TODO: expand this check to accommodate multi-asset pools for stableswap


this was copied over, what are the plans for this?

p0mvn · 2022-05-10T07:54:04Z

x/gamm/pool-models/stableswap/pool.go

@@ -154,7 +139,9 @@ func (pa *Pool) SwapOutAmtGivenIn(ctx sdk.Context, tokenIn sdk.Coins, tokenOutDe
 		return sdk.Coin{}, err
 	}

-	pa.updatePoolLiquidityForSwap(tokenIn, sdk.NewCoins(tokenOut))
+	if err := pa.updatePoolLiquidityForSwap(tokenIn, sdk.NewCoins(tokenOut)); err != nil {
+		panic(err)


this was changed to make it more testable in unit tests

p0mvn · 2022-05-10T07:54:49Z

x/gamm/pool-models/stableswap/pool.go

+// Returns a pool asset, and its index. If err != nil, then the index will be valid.
+// CONTRACT: pool must be created with NewStableSwapPool so that the pool assets are sorted
+// by denom. Otherwise, the behavior is undefined.
+func (pa Pool) getPoolAssetAndIndex(denom string) (int, PoolAsset, error) {


this can be later generalized with balancer

ValarDragon · 2022-05-10T09:14:18Z

I think the non-pool asset design was better tbh, and Balancer is what needs changing in the future

p0mvn · 2022-05-10T16:31:44Z

I think the non-pool asset design was better tbh, and Balancer is what needs changing in the future

Why do you think so?

I find it more brittle to rely on the order of 2 separate slices to be in sync. If 2 slices need to maintain the same order, they should be grouped and merged into one. Otherwise, a small update to the ordering of one of the slices in the future can bring them out of sync and might cause problems when debugging.

Additionally, while setting up unit tests, I found this abstraction to be easier to configure and assert on its results.

These were the 2 primary motivators for this change

Please let me know what you think

alexanderbez · 2022-05-10T17:39:58Z

proto/osmosis/gamm/pool-models/stableswap/stableswap_pool.proto

+      [ (gogoproto.moretags) = "yaml:\"token\"", (gogoproto.nullable) = false ];
+  // used for normalizing values for calculation amognst assets with different
+  // precisions
+  string scalingFactor = 2 [


Suggested change

string scalingFactor = 2 [

string scaling_factor = 2 [

alexanderbez · 2022-05-10T17:40:06Z

proto/osmosis/gamm/pool-models/stableswap/stableswap_pool.proto

-    (gogoproto.moretags) = "yaml:\"stableswap_scaling_factor\"",
-    (gogoproto.nullable) = false
-  ];
+  repeated osmosis.gamm.stableswap.v1beta1.PoolAsset poolAssets = 6


Suggested change

repeated osmosis.gamm.stableswap.v1beta1.PoolAsset poolAssets = 6

repeated osmosis.gamm.stableswap.v1beta1.PoolAsset pool_assets = 6

p0mvn · 2022-05-19T15:28:28Z

@ValarDragon any updates on this?

mattverse · 2022-05-25T14:14:28Z

Since this PR is starting to have blockers on it, we should decide direction for this on call

github-actions · 2022-07-08T00:02:25Z

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!

mattverse · 2022-07-08T08:52:37Z

Stale bot is indeed doing its work! Nice :)

github-actions · 2022-07-24T00:02:07Z

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!

p0mvn added 4 commits May 9, 2022 17:46

introduce PoolAssets in the stableswap pool to make more consistent w…

2e6aa31

…ith the balancer

implement TestStableswapCreatePool

de5eac4

add TestStableswapGetTotalPoolLiquidity unit test

890cb71

stableswap pool validation; getScaledPoolAmt; unit tests

57ac345

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

p0mvn and others added 8 commits May 10, 2022 00:57

TestStableswapGetDeScaledPoolAmt unit test

028a672

implement TestStableswapGetPoolAssetAndIndex

307cb72

implement TestStableswapUpdatePoolLiquidityForSwap

a447331

pool asset validation in message; TestStableSwapPoolAssetValidate

4046428

extract constants for denom names in tests

ec65196

Merge branch 'main' into roman/scaling-factors2

683e66f

update stableswap proto

85b7dab

Merge branch 'roman/scaling-factors2' of github.com:osmosis-labs/osmo…

e82ae8d

…sis into roman/scaling-factors2

p0mvn commented May 10, 2022

View reviewed changes

improve pool comments

f02cff4

p0mvn marked this pull request as ready for review May 10, 2022 08:05

p0mvn requested a review from a team May 10, 2022 08:05

alexanderbez reviewed May 10, 2022

View reviewed changes

p0mvn added 2 commits May 12, 2022 15:23

Merge branch 'main' into roman/scaling-factors2

9c5dcca

update proto definitions

c6f7ba2

stackman27 mentioned this pull request May 19, 2022

refactor: stableswap inverse tests to be consistent with the stableswap pool #1470

Closed

stackman27 mentioned this pull request May 22, 2022

Modified stableswap amm.test to accomodate Scaling Factor #1560

Closed

mattverse mentioned this pull request Jun 2, 2022

[x/gamm][StableSwap] #1451

Closed

55 tasks

p0mvn mentioned this pull request Jun 27, 2022

[x/gamm][StableSwap] Implement Scaling Factors to CreateStableswapPool #1392

Closed

github-actions bot added the Stale label Jul 8, 2022

github-actions bot removed the Stale label Jul 9, 2022

github-actions bot added the Stale label Jul 24, 2022

github-actions bot closed this Jul 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: scaling factors validation in create stableswap pool, small refactor and unit tests #1459

feat: scaling factors validation in create stableswap pool, small refactor and unit tests #1459

p0mvn commented May 10, 2022 •

edited

Loading

codecov-commenter commented May 10, 2022 •

edited

Loading

p0mvn May 10, 2022

p0mvn May 10, 2022

p0mvn May 10, 2022 •

edited

Loading

ValarDragon commented May 10, 2022

p0mvn commented May 10, 2022

alexanderbez May 10, 2022

alexanderbez May 10, 2022

p0mvn commented May 19, 2022

mattverse commented May 25, 2022

github-actions bot commented Jul 8, 2022

mattverse commented Jul 8, 2022

github-actions bot commented Jul 24, 2022

	repeated osmosis.gamm.stableswap.v1beta1.PoolAsset poolAssets = 6
	repeated osmosis.gamm.stableswap.v1beta1.PoolAsset pool_assets = 6

feat: scaling factors validation in create stableswap pool, small refactor and unit tests #1459

feat: scaling factors validation in create stableswap pool, small refactor and unit tests #1459

Conversation

p0mvn commented May 10, 2022 • edited Loading

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

codecov-commenter commented May 10, 2022 • edited Loading

Codecov Report

p0mvn May 10, 2022

Choose a reason for hiding this comment

p0mvn May 10, 2022

Choose a reason for hiding this comment

p0mvn May 10, 2022 • edited Loading

Choose a reason for hiding this comment

ValarDragon commented May 10, 2022

p0mvn commented May 10, 2022

alexanderbez May 10, 2022

Choose a reason for hiding this comment

alexanderbez May 10, 2022

Choose a reason for hiding this comment

p0mvn commented May 19, 2022

mattverse commented May 25, 2022

github-actions bot commented Jul 8, 2022

mattverse commented Jul 8, 2022

github-actions bot commented Jul 24, 2022

p0mvn commented May 10, 2022 •

edited

Loading

codecov-commenter commented May 10, 2022 •

edited

Loading

p0mvn May 10, 2022 •

edited

Loading