[CL Simulator] Fix existing simulator and test simulator flow for CollectFees, CollectIncentives #4832

stackman27 · 2023-04-04T17:55:48Z

Closes: #4753

What is the purpose of the change

Simulator: Testing flow createPool -> createPositions -> swap Randomly -> CollectFees

Create Pool with random values (could be multiple pools)
createRandom Position in the above pool (could be multiple positions)
SwapRandom amount in the above pool & position (could be across multiple pools and positions
collectFees for random positions created above and wherever swap happened

Simulator: Testing flow createPool -> createPosition -> collectIncentives

Create Pool with random values (could be multiple pools)
createRandom Position in the above pool (could be multiple positions)
Let random x amount of time pass (in simulator this could be done through the amount of blocks created)
Collect Incentives for that position (should be for created positions)

Extra features

fixed CreateIncentive wiring
added CollectIncentive simulator check
added CreateIncentive simulator check

Brief Changelog

registered CreateIncentive message and added CL module simulator test

Testing and Verifying

added sim test

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? (yes / no)
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes / no)
How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

stackman27 · 2023-04-06T01:25:49Z

proto/osmosis/concentrated-liquidity/tx.proto

@@ -15,6 +15,7 @@ service Msg {
  rpc CollectFees(MsgCollectFees) returns (MsgCollectFeesResponse);
  rpc CollectIncentives(MsgCollectIncentives)
      returns (MsgCollectIncentivesResponse);
+  rpc CreateIncentive(MsgCreateIncentive) returns (MsgCreateIncentiveResponse);


note to reviewers: idk why this was missed while creating the msg, so i added it

stackman27 · 2023-04-06T01:27:00Z

x/pool-incentives/keeper/keeper.go

@@ -113,7 +112,7 @@ func (k Keeper) CreateConcentratedLiquidityPoolGauge(ctx sdk.Context, poolId uin
 		// lockQueryType as byTime. Although we donot need this check, we still cannot pass empty struct.
 		lockuptypes.QueryCondition{
 			LockQueryType: lockuptypes.ByTime,
-			Denom:         appparams.BaseCoinUnit,
+			Denom:         sdk.DefaultBondDenom,


note to reviewers: we technically donot care about the denom since, we donot use it while creating CL gauges. so i changed it to "stake" to make simulator testing easier. We cannot leave it empty as it is a require field in CreateGauge

x/concentrated-liquidity/lp.go

czarcas7ic · 2023-04-11T20:59:50Z

x/concentrated-liquidity/simulation/sim_msgs.go

+	remainingToken1Amt := tokens[1].Amount
+	// create positions until the funds run out from positionCreator
+	for remainingToken0Amt.GT(sdk.ZeroInt()) && remainingToken1Amt.GT(sdk.ZeroInt()) {
+		//make sure the user always has tokens to create Position


Suggested change

//make sure the user always has tokens to create Position

// make sure the user always has tokens to create Position

x/concentrated-liquidity/simulation/sim_msgs.go

x/pool-incentives/keeper/keeper.go

czarcas7ic · 2023-04-11T23:55:06Z

x/concentrated-liquidity/simulation/sim_msgs.go

+	// positionCreator creates the position with pool denoms
 	// get random user address with the pool denoms
 	sender, _, senderExists := sim.SelAddrWithDenoms(ctx, poolDenoms)
 	if !senderExists {
 		return nil, fmt.Errorf("no sender with denoms %s exists", poolDenoms)
 	}

-	positions, err := k.GetUserPositions(ctx, sender.Address, 0)
+	// get a random Position
+	positions, err := k.GetUserPositions(ctx, sender.Address, clPool.GetId())
 	if err != nil {
 		return nil, fmt.Errorf("position does not exist")
 	}

-	if len(positions) == 0 {
-		return nil, fmt.Errorf("user does not have any position")
+	if len(positions) < 1 {
+		return nil, fmt.Errorf("user doesnot have any positions")
 	}


The flow should be the following:

Get a random CL pool

Utilize the PoolId to PositionId mapping. This will allow you to iterate that store and select a random position ID form that list

Use this position to attempt to withdraw

What you implemented does the following:

Gets a random address

Hopes the address has a position

^ this will result in lots of failed attempts to withdraw, while the first option will fail much less

i see, the poolId -> positionid mapping flow was def very clean!

changed!

czarcas7ic · 2023-04-24T01:39:54Z

What was the decision on this? I think I remember something about us holding off on merging until something was figured out. Would like to note whatever the blocker is here so we can track it appropriately.

p0mvn · 2023-04-24T01:43:15Z

What was the decision on this? I think I remember something about us holding off on merging until something was figured out. Would like to note whatever the blocker is here so we can track it appropriately.

As far as I'm tracking, this is good to go as long as the non-deterministic failures are figured out and resolved.

If this is still failing from time to time, I think we should get to the bottom of this prior to merge

czarcas7ic · 2023-04-24T01:52:50Z

@stackman27 can you confirm if we are still having non-determinism in the test?

stackman27 · 2023-04-24T02:25:14Z

@stackman27 can you confirm if we are still having non-determinism in the test?

@p0mvn @czarcas7ic i was waiting on #4973 to get merge. Will run some final test now and let you guys know

stackman27 · 2023-04-24T05:33:20Z

@stackman27 can you confirm if we are still having non-determinism in the test?

@p0mvn @czarcas7ic i was waiting on #4973 to get merge. Will run some final test now and let you guys know

update on this

i rebased and reran the tests. Everything seems good, haven't gotten the negative coin bug so far

… emissions

p0mvn

great work!

github-actions bot added C:app-wiring Changes to the app folder C:CLI C:simulator Edits simulator or simulations C:x/concentrated-liquidity C:x/gamm Changes, features and bugs related to the gamm module. C:x/incentives C:x/pool-incentives C:x/poolmanager labels Apr 4, 2023

stackman27 force-pushed the sis/simulator-inc-flow branch from 85c7c60 to e09401c Compare April 5, 2023 07:27

github-actions bot removed C:x/gamm Changes, features and bugs related to the gamm module. C:x/poolmanager C:CLI labels Apr 5, 2023

stackman27 force-pushed the sis/simulator-inc-flow branch from 4a0e23f to 849c7c7 Compare April 6, 2023 01:25

stackman27 commented Apr 6, 2023

View reviewed changes

stackman27 force-pushed the sis/simulator-inc-flow branch from 0e09f10 to 940e906 Compare April 6, 2023 01:28

stackman27 marked this pull request as ready for review April 6, 2023 01:29

stackman27 added the V:state/breaking State machine breaking PR label Apr 6, 2023

stackman27 requested review from czarcas7ic, p0mvn, AlpinYukseloglu and mattverse and removed request for czarcas7ic and p0mvn April 6, 2023 01:32

czarcas7ic requested changes Apr 11, 2023

View reviewed changes

stackman27 requested a review from czarcas7ic April 11, 2023 23:43

stackman27 marked this pull request as draft April 11, 2023 23:54

czarcas7ic reviewed Apr 11, 2023

View reviewed changes

stackman27 force-pushed the sis/simulator-inc-flow branch from 933f67f to de35d1a Compare April 20, 2023 01:09

stackman27 added the no-changelog label Apr 20, 2023

stackman27 force-pushed the sis/simulator-inc-flow branch from de35d1a to 94c0046 Compare April 24, 2023 04:53

github-actions bot removed the C:x/incentives label Apr 24, 2023

stackman27 requested a review from p0mvn April 25, 2023 06:58

stackman27 added 16 commits April 25, 2023 15:01

refactored gethighestliquiditypool

696e688

[CL Incentives] Implement incentives wiring from governance-triggered…

b832845

… emissions

adams comments

ef70c97

adams comments

d41d0ec

nit

cbe9940

added position id and pool id mapping

da89259

wip

aa7f2b7

fixed test

748c67b

more cleanup

6163c92

fix

c4d637f

adam comment nits

ed6d755

fixed test

b5af6de

nit

2768067

removed math/rand

aa03466

rebased

a0f23b9

reabsed

c2a7350

stackman27 force-pushed the sis/simulator-inc-flow branch from 94c0046 to c2a7350 Compare April 25, 2023 22:48

p0mvn merged commit 465f8ef into main Apr 25, 2023

p0mvn deleted the sis/simulator-inc-flow branch April 25, 2023 23:18

p0mvn reviewed Apr 25, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CL Simulator] Fix existing simulator and test simulator flow for CollectFees, CollectIncentives #4832

[CL Simulator] Fix existing simulator and test simulator flow for CollectFees, CollectIncentives #4832

stackman27 commented Apr 4, 2023 •

edited

Loading

stackman27 Apr 6, 2023 •

edited

Loading

stackman27 Apr 6, 2023

czarcas7ic Apr 11, 2023

czarcas7ic Apr 11, 2023

stackman27 Apr 12, 2023

czarcas7ic commented Apr 24, 2023 •

edited

Loading

p0mvn commented Apr 24, 2023

czarcas7ic commented Apr 24, 2023

stackman27 commented Apr 24, 2023

stackman27 commented Apr 24, 2023

p0mvn left a comment

	//make sure the user always has tokens to create Position
	// make sure the user always has tokens to create Position

[CL Simulator] Fix existing simulator and test simulator flow for CollectFees, CollectIncentives #4832

[CL Simulator] Fix existing simulator and test simulator flow for CollectFees, CollectIncentives #4832

Conversation

stackman27 commented Apr 4, 2023 • edited Loading

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

stackman27 Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

stackman27 Apr 6, 2023

Choose a reason for hiding this comment

czarcas7ic Apr 11, 2023

Choose a reason for hiding this comment

czarcas7ic Apr 11, 2023

Choose a reason for hiding this comment

stackman27 Apr 12, 2023

Choose a reason for hiding this comment

czarcas7ic commented Apr 24, 2023 • edited Loading

p0mvn commented Apr 24, 2023

czarcas7ic commented Apr 24, 2023

stackman27 commented Apr 24, 2023

stackman27 commented Apr 24, 2023

p0mvn left a comment

Choose a reason for hiding this comment

stackman27 commented Apr 4, 2023 •

edited

Loading

stackman27 Apr 6, 2023 •

edited

Loading

czarcas7ic commented Apr 24, 2023 •

edited

Loading