[CL][bugfix] Refactor incentives tests and fix incentive forfeiting bug #4735

AlpinYukseloglu · 2023-03-24T18:24:05Z

Closes: #XXX

What is the purpose of the change

This PR does a small refactor of incentives tests to make them compatible with the changes in #4724. It also fixes a bug where forfeited incentives were being double-distributed.

Brief Changelog

Minor refactor incentives and position withdrawal
Fix forfeiting incentives bug and make collectIncentivesForPosition return claimed & forfeited incentives as separate values

Testing and Verifying

All changes are tested in incentives_test.go and lp_test.go

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 documented)

* Update README.md * Create check-broken-links.yml * Update check-broken-links.yml * Update check-broken-links.yml * Update check-broken-links.yml * Update check-broken-links.yml * Update check-broken-links.yml * Update check-broken-links.yml * Update README.md

Co-authored-by: AutoStake <[email protected]>

…de and integrate into e2e (#4706) * update logic add account genesis * clean unused package * golint

…#4694) * initial attempt at splitting registries. Issues with circular deps * properly split registries * added reverse prefix map * channel validation done by registries * integrating registry into xcs and fixing the tests * Printing error context on failure * full integration with unwrapping * remove replace * removed unused deps * clippy * x86 bytecode * fmt after lints * gofumpt * fix re-export * lint * added test-contract addr * added missing newline * remove recovery states * x86 bytecode * new bytecode

* added single pool query * added changelog * remove empty tests as they are invalid * removed sender and made reserved * remove and reserve unused sender * removed sender * actually remove sender * querygen * move impl to correct file * more flexible querygen * Update proto/osmosis/poolmanager/v1beta1/query.proto Co-authored-by: Matt, Park <[email protected]> * Update proto/osmosis/poolmanager/v1beta1/query.proto Co-authored-by: Matt, Park <[email protected]> * Update proto/osmosis/poolmanager/v1beta1/query.proto Co-authored-by: Matt, Park <[email protected]> * Update proto/osmosis/poolmanager/v1beta1/query.proto Co-authored-by: Matt, Park <[email protected]> * protos * added cli * fixed command parsing * single line * lint * fix cli * run proto gen * added whitelisted query * fix test for v15 --------- Co-authored-by: Matt, Park <[email protected]>

…s sdk side and integrate into e2e (#4706)" (#4727) This reverts commit a9725d6.

* init set hot routes cli command * adding remaining txs * few more tests * comments

AlpinYukseloglu · 2023-03-24T18:27:32Z

x/concentrated-liquidity/incentives_test.go

-			s.Ctx = s.Ctx.WithBlockTime(s.Ctx.BlockTime().Add(cltypes.SupportedUptimes[1] - cltypes.SupportedUptimes[0]))
-
-			_, _, _, qualifyingLiquidityUptimeOne, _, err := clKeeper.CreatePosition(s.Ctx, tc.poolId, testAddressOne, testQualifyingDepositsOne, testQualifyingDepositsOne, sdk.ZeroInt(), sdk.ZeroInt(), clPool.GetCurrentTick().Int64()-1, clPool.GetCurrentTick().Int64()+1)
+			_, _, _, qualifyingLiquidity, _, err := clKeeper.CreatePosition(s.Ctx, tc.poolId, testAddressOne, testQualifyingDepositsOne, testQualifyingDepositsOne, sdk.ZeroInt(), sdk.ZeroInt(), clPool.GetCurrentTick().Int64()-1, clPool.GetCurrentTick().Int64()+1)


Note: since createPosition calls the function under test here (updateAccumulatorsToNow) which would be triggered when we space position creation out, creating multiple positions during setup no longer makes sense for this test.

AlpinYukseloglu · 2023-03-24T18:29:13Z

x/concentrated-liquidity/incentives_test.go

@@ -3194,9 +3187,14 @@ func (s *KeeperTestSuite) TestClaimAllIncentives() {
 			initSenderBalances := s.App.BankKeeper.GetAllBalances(s.Ctx, defaultSender)
 			initPoolBalances := s.App.BankKeeper.GetAllBalances(s.Ctx, clPool.GetAddress())

+			if !tc.forfeitIncentives {


Note: to avoid a larger test refactor, I kept forfeitIncentives as a boolean test field, where it either forfeits some or all incentives. Partial incentive forfeiting is tested in LP withdrawal tests in lp_test.go

AlpinYukseloglu · 2023-03-24T18:37:04Z

x/concentrated-liquidity/incentives.go

@@ -583,7 +591,8 @@ func (k Keeper) collectIncentives(ctx sdk.Context, owner sdk.AccAddress, positio
 	}

 	// Claim all incentives for the position.
-	collectedIncentivesForPosition, err := k.claimAllIncentivesForPosition(ctx, position.PositionId)
+	// TODO: consider returning forfeited rewards as well


Created issue here: #4736

…#4725) * feat: whitelist poolmanager Pool, CL Params and ClaimableFees queries * lint * conflict

czarcas7ic

This looks good to me, I will merge this into my branch and continue iterating. Thanks!

czarcas7ic · 2023-03-25T02:18:24Z

x/concentrated-liquidity/incentives_test.go

+// Testing strategy:
+// 1. Create a position
+// 2. Let a fixed amount of time pass, enough to qualify it for some (but not all) uptimes
+// 3. Let a variable amount of time pass determined by the test case


It seems the variable amount of time in every test case is one hour, but I think we can expand this in a future iteration

* utilize new position id in position key * add position id to migration response * regen proto * modify withdraw pos in e2e * cmd order * avoid store key formatting with fmt.Sprintf * set default NextPositionId to 1 * lint * return positionId from CreatePos E2E * utilize bytes.Buffer to build keys * Revert "utilize bytes.Buffer to build keys" This reverts commit 0c1bc13. * utilize sprintf for key * remove extra positions logic * initial push * regen proto * Update go.mod * lints * fix sim * add posid to sim collectincentive * e2e test fix * lint * clean up * remove extra fields * regen proto * only use pos id for fees * update cli and e2e * fee accum updates * modify claim to use only positionId * fix functional fees test * pool to position id map * add pool store to delete position * fix conflicts from main merge * test updates * lints / cleanup * add position Id to create position response * regen proto * regen proto * switch order * regen proto * add error comment to collectFees * move key format to types * rename to positionFields * Remove unnecessary test field * revert refactor to functional fees test * define custom errors * remove duplicate events emitted * correct validatePositionUpdate inputs * Update x/concentrated-liquidity/position.go Co-authored-by: Roman <[email protected]> * update README for KV stores * other readme updates * feat: query position by id (added to stargate whitelist) (#4718) * feat(CL): position by id query (stargate whitelisted) * fix test * Update app/upgrades/v15/upgrade_test.go --------- Co-authored-by: Adam Tucker <[email protected]> * initial push * remove position age as input param * [CL][bugfix] Refactor incentives tests and fix incentive forfeiting bug (#4735) * link (#4708) * daily check for broken links (#4712) * Update README.md * Create check-broken-links.yml * Update check-broken-links.yml * Update check-broken-links.yml * Update check-broken-links.yml * Update check-broken-links.yml * Update check-broken-links.yml * Update check-broken-links.yml * Update README.md * Update AutoStake seed from .net to .com (#4711) Co-authored-by: AutoStake <[email protected]> * [tests/e2e]: simplify the logic for adding accounts to genesis sdk side and integrate into e2e (#4706) * update logic add account genesis * clean unused package * golint * XCS + Registries integration: IBC forward generation using registries (#4694) * initial attempt at splitting registries. Issues with circular deps * properly split registries * added reverse prefix map * channel validation done by registries * integrating registry into xcs and fixing the tests * Printing error context on failure * full integration with unwrapping * remove replace * removed unused deps * clippy * x86 bytecode * fmt after lints * gofumpt * fix re-export * lint * added test-contract addr * added missing newline * remove recovery states * x86 bytecode * new bytecode * added single pool query (#4549) * added single pool query * added changelog * remove empty tests as they are invalid * removed sender and made reserved * remove and reserve unused sender * removed sender * actually remove sender * querygen * move impl to correct file * more flexible querygen * Update proto/osmosis/poolmanager/v1beta1/query.proto Co-authored-by: Matt, Park <[email protected]> * Update proto/osmosis/poolmanager/v1beta1/query.proto Co-authored-by: Matt, Park <[email protected]> * Update proto/osmosis/poolmanager/v1beta1/query.proto Co-authored-by: Matt, Park <[email protected]> * Update proto/osmosis/poolmanager/v1beta1/query.proto Co-authored-by: Matt, Park <[email protected]> * protos * added cli * fixed command parsing * single line * lint * fix cli * run proto gen * added whitelisted query * fix test for v15 --------- Co-authored-by: Matt, Park <[email protected]> * Revert "[tests/e2e]: simplify the logic for adding accounts to genesis sdk side and integrate into e2e (#4706)" (#4727) This reverts commit a9725d6. * ProtoRev: Adding Txs to CLI (#4567) * init set hot routes cli command * adding remaining txs * few more tests * comments * Update upgrade_test.go (#4734) * refactor tests and fix incentive forfeiting bug * update create position tx * feat: whitelist poolmanager Pool, CL Params and ClaimableFees queries (#4725) * feat: whitelist poolmanager Pool, CL Params and ClaimableFees queries * lint * conflict * fix e2e query parsing * increase gas --------- Co-authored-by: Ruslan Akhtariev <[email protected]> Co-authored-by: Master Pi <[email protected]> Co-authored-by: AutoStake <[email protected]> Co-authored-by: AutoStake <[email protected]> Co-authored-by: Nguyen Thanh Nhan <[email protected]> Co-authored-by: Nicolas Lara <[email protected]> Co-authored-by: Matt, Park <[email protected]> Co-authored-by: David Terpay <[email protected]> Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: Roman <[email protected]> * refactor collect incentives test * remove any final references to freezing * merge main * add back ParsePositionFromBz * add comments to test * add forfeit incentives withdraw test case * add negative duration validation and test * clean up * add back multi-position test * regen proto --------- Co-authored-by: czarcas7ic <[email protected]> Co-authored-by: Roman <[email protected]> Co-authored-by: alpo <[email protected]> Co-authored-by: Ruslan Akhtariev <[email protected]> Co-authored-by: Master Pi <[email protected]> Co-authored-by: AutoStake <[email protected]> Co-authored-by: AutoStake <[email protected]> Co-authored-by: Nguyen Thanh Nhan <[email protected]> Co-authored-by: Nicolas Lara <[email protected]> Co-authored-by: Matt, Park <[email protected]> Co-authored-by: David Terpay <[email protected]>

pysel and others added 10 commits March 23, 2023 13:16

link (#4708)

067e0aa

Update AutoStake seed from .net to .com (#4711)

68f0de3

Co-authored-by: AutoStake <[email protected]>

[tests/e2e]: simplify the logic for adding accounts to genesis sdk si…

a9725d6

…de and integrate into e2e (#4706) * update logic add account genesis * clean unused package * golint

Revert "[tests/e2e]: simplify the logic for adding accounts to genesi…

afbc5b0

…s sdk side and integrate into e2e (#4706)" (#4727) This reverts commit a9725d6.

ProtoRev: Adding Txs to CLI (#4567)

243b594

* init set hot routes cli command * adding remaining txs * few more tests * comments

Update upgrade_test.go (#4734)

d099e54

refactor tests and fix incentive forfeiting bug

f5c60e2

AlpinYukseloglu added the V:state/breaking State machine breaking PR label Mar 24, 2023

github-actions bot added C:simulator Edits simulator or simulations C:x/concentrated-liquidity C:x/gamm Changes, features and bugs related to the gamm module. C:x/poolmanager C:x/superfluid labels Mar 24, 2023

AlpinYukseloglu changed the title ~~Alpo refactor incentives~~ [CL][bugfix] Refactor incentives tests and fix incentive forfeiting bug Mar 24, 2023

AlpinYukseloglu changed the base branch from adam/position-id-key-refactor to adam/remove-freeze-duration March 24, 2023 18:25

AlpinYukseloglu commented Mar 24, 2023

View reviewed changes

AlpinYukseloglu mentioned this pull request Mar 24, 2023

[CL Incentives] Make collectIncentives return forfeited amount #4736

Closed

AlpinYukseloglu commented Mar 24, 2023

View reviewed changes

update create position tx

0b87b7b

github-actions bot added C:CLI and removed C:x/gamm Changes, features and bugs related to the gamm module. C:simulator Edits simulator or simulations C:x/poolmanager C:x/superfluid labels Mar 24, 2023

p0mvn and others added 2 commits March 24, 2023 15:11

feat: whitelist poolmanager Pool, CL Params and ClaimableFees queries (…

7417c4d

…#4725) * feat: whitelist poolmanager Pool, CL Params and ClaimableFees queries * lint * conflict

fix e2e query parsing

4240946

Merge branch 'adam/fix-regex-e2e-cl' into alpo-refactor-incentives

d23d3d3

github-actions bot added C:docs Improvements or additions to documentation C:x/poolmanager T:CI labels Mar 24, 2023

increase gas

5248c4d

czarcas7ic approved these changes Mar 25, 2023

View reviewed changes

czarcas7ic merged commit 6636dcc into adam/remove-freeze-duration Mar 25, 2023

czarcas7ic deleted the alpo-refactor-incentives branch March 25, 2023 02:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CL][bugfix] Refactor incentives tests and fix incentive forfeiting bug #4735

[CL][bugfix] Refactor incentives tests and fix incentive forfeiting bug #4735

AlpinYukseloglu commented Mar 24, 2023

AlpinYukseloglu Mar 24, 2023

AlpinYukseloglu Mar 24, 2023

AlpinYukseloglu Mar 24, 2023

czarcas7ic left a comment

czarcas7ic Mar 25, 2023

[CL][bugfix] Refactor incentives tests and fix incentive forfeiting bug #4735

[CL][bugfix] Refactor incentives tests and fix incentive forfeiting bug #4735

Conversation

AlpinYukseloglu commented Mar 24, 2023

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

AlpinYukseloglu Mar 24, 2023

Choose a reason for hiding this comment

AlpinYukseloglu Mar 24, 2023

Choose a reason for hiding this comment

AlpinYukseloglu Mar 24, 2023

Choose a reason for hiding this comment

czarcas7ic left a comment

Choose a reason for hiding this comment

czarcas7ic Mar 25, 2023

Choose a reason for hiding this comment