[CL Incentives] Implement `CreateIncentive` logic and message #4519

AlpinYukseloglu · 2023-03-06T18:21:00Z

Closes: #4400, #4183

What is the purpose of the change

This PR implements incentive creation logic and its respective message.

Brief Changelog

Implement createIncentive
Implement MsgCreateIncentive & related wiring

Testing and Verifying

New functionality is tested in incentives_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)

stackman27

LGTM, just have couple of questions

stackman27 · 2023-03-06T22:00:57Z

proto/osmosis/concentrated-liquidity/tx.proto

+  string incentive_denom = 3;
+  string incentive_amount = 4 [


just a thought, why not set this as Coin instead of denom and amount? so that later down the line we can create incentives record for many Coins

Agreed on making this a Coin, unless it was by design for a reason I am not seeing.

stackman27 · 2023-03-06T22:08:17Z

x/concentrated-liquidity/types/msgs.go

+
+func (msg MsgCreateIncentive) Route() string { return RouterKey }
+func (msg MsgCreateIncentive) Type() string  { return TypeMsgCollectIncentives }
+func (msg MsgCreateIncentive) ValidateBasic() error {


should we check if min_uptime is valid here too? basically the same logic as https://github.com/osmosis-labs/osmosis/pull/4519/files#diff-8fb13ed7a62b35b89555bdc906798a4353c391186de40eacadac4045c1e4d7bdR526-R534

stackman27

Also do we ever need to update Incentive Records? or are these also non fungible once created?

czarcas7ic

Well done :) just a few non blocking comments

czarcas7ic · 2023-03-07T23:17:19Z

proto/osmosis/concentrated-liquidity/tx.proto

+  string incentive_denom = 3;
+  string incentive_amount = 4 [


Agreed on making this a Coin, unless it was by design for a reason I am not seeing.

czarcas7ic · 2023-03-07T23:17:32Z

proto/osmosis/concentrated-liquidity/tx.proto

+  string incentive_denom = 1;
+  string incentive_amount = 2 [
+    (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Dec",
+    (gogoproto.moretags) = "yaml:\"incentive_amount\"",
+    (gogoproto.nullable) = false
+  ];


czarcas7ic · 2023-03-07T23:21:53Z

x/concentrated-liquidity/client/cli/tx.go

+		Flags:               osmocli.FlagDesc{RequiredFlags: []*flag.FlagSet{FlagSetJustPoolId()}},
+	}, &types.MsgCreateIncentive{}


Not blocking, but we have been trying to move away from mandatory flags, would be nice if we could just make poolId be part of the create incentive message

czarcas7ic · 2023-03-07T23:31:49Z

x/concentrated-liquidity/incentives_test.go

+		"existing incentive records": {
+			poolId: defaultPoolId,
+			sender: s.TestAccs[0],
+			senderBalance: sdk.NewCoins(
+				sdk.NewCoin(
+					incentiveRecordOne.IncentiveDenom,
+					incentiveRecordOne.RemainingAmount.Ceil().RoundInt(),
+				),
+			),
+			recordToSet: incentiveRecordOne,
+			existingRecords: []types.IncentiveRecord{incentiveRecordTwo, incentiveRecordThree},


Do we have a test case that tests what happens if someone creates the same incentive record twice? Does it add to the existing incentive record? I don't believe this tests that test case since I think incentiveRecordTwo and Three have different params.

czarcas7ic · 2023-03-07T23:33:09Z

x/concentrated-liquidity/types/msgs.go

+
+func (msg MsgCreateIncentive) Route() string { return RouterKey }
+func (msg MsgCreateIncentive) Type() string  { return TypeMsgCollectIncentives }
+func (msg MsgCreateIncentive) ValidateBasic() error {


p0mvn · 2023-03-08T01:42:02Z

x/concentrated-liquidity/incentives.go

+	validUptime := false
+	for _, supportedUptime := range types.SupportedUptimes {
+		if minUptime == supportedUptime {
+			validUptime = true


Suggested change

validUptime = true

validUptime = true

break

p0mvn · 2023-03-08T01:43:17Z

x/concentrated-liquidity/incentives.go

+	}
+
+	// Ensure sender has balance for incentive denom
+	incentiveCoin := sdk.NewCoin(incentiveDenom, incentiveAmount)


related to earlier comments - I think having it as Coin from the start makes more sense to me as well

p0mvn · 2023-03-08T01:45:04Z

x/concentrated-liquidity/msg_server.go

+	ctx.EventManager().EmitEvents(sdk.Events{
+		sdk.NewEvent(
+			sdk.EventTypeMessage,
+			sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory),
+			sdk.NewAttribute(sdk.AttributeKeySender, msg.Sender),
+		),
+		sdk.NewEvent(
+			types.TypeEvtCreateIncentive,
+			sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory),
+			sdk.NewAttribute(sdk.AttributeKeySender, msg.Sender),
+			sdk.NewAttribute(types.AttributeKeyPoolId, strconv.FormatUint(msg.PoolId, 10)),
+			sdk.NewAttribute(types.AttributeIncentiveDenom, msg.IncentiveDenom),
+			sdk.NewAttribute(types.AttributeIncentiveAmount, msg.IncentiveAmount.String()),
+			sdk.NewAttribute(types.AttributeIncentiveEmissionRate, msg.EmissionRate.String()),
+			sdk.NewAttribute(types.AttributeIncentiveStartTime, msg.StartTime.String()),
+			sdk.NewAttribute(types.AttributeIncentiveMinUptime, msg.MinUptime.String()),
+		),
+	})


Should we test these events being emitted?

…#4519)" This reverts commit b07e763.

…#4519)" (#4568) This reverts commit b07e763.

mergify · 2023-03-10T23:47:40Z

⚠️ The sha of the head commit of this PR conflicts with #4569. Mergify cannot evaluate rules on this PR. ⚠️

* initial push * push * untracked files * add untracked files * merge branches * add changes required for fees * lint * fix test errors * clean up * remove print lines * feat: single fee accum (#4116) * single fee accum * lint * no longer need to get all positions for fee * incentive accum initialization * lint * Update x/concentrated-liquidity/store_test.go Co-authored-by: Sishir Giri <[email protected]> * add lower level tests and make uptime accum access private * lint * add gotests for helper fn * Update test to use existing variable Co-authored-by: Roman <[email protected]> * create or update records upon position creation/update * add tests and clean up * sync position-related changes * add support for negative liquidity delta * add and test helper & set up tick initialization logic * implement accumulator updates on tick crossing and clean up proto * clean up comments * minor test cleanup * ensure accum initalization follows conventions and clean up tests * update pool proto and CL pool extension for tracking liq changes * update new field at pool initialization and add tests * comment todos * clean up naming and move empty options to global var * add clarifying comments and lint * implement IncentiveRecord proto * add comments * lint * set up wiring for proto * implement updateUptimeAccumulatorsToNow * implement and test calcAccruedIncentivesForAccum * add tests for new helpers * minor comment updates Co-authored-by: Roman <[email protected]> * accum test updates * lint * clean up diff * lint * update go.mod for e2e * clean up proto and add error message * lint * sketch out testing approach * set up keys * clean up diff * clean up comments * lint * implement set and get for incentive records * comment cleanup * minor comment and test cleanup * clean up tests and comments * [CL Incentives] Implement `setIncentiveRecords` and `getIncentiveRecords` (#4283) * set up keys * implement set and get for incentive records * comment cleanup * minor comment and test cleanup * Revert "[CL Incentives] Implement `setIncentiveRecords` and `getIncentiveRecords` (#4283)" (#4294) This reverts commit de7c146. * set incentive records in new position tests * fix merge conflicts * add check for init position record values * fix and finalize uptime accum position tests * fix go mod for e2e * clean up diff * add err check to accum update * fix rounding error * implement tick init uptime tracker logic * fix test compatibility * implement fix for global accums falling out of sync before tick init * add tests to ensure uptime trackers are initialized properly * add comment for new helper * add core logic * go mod tidy * fix conflicts * fix e2e * update go mod * fix and further test tick init logic * remove redundant values from incentive record state * cleanup from review * comment cleanup * set up cross tick tests * add further tests * test and comment cleanup * implement core logic * update osmoutils go mod and remove curTick as fn param * thorough tests for new functionality * clean up new functions for readability * clean up test comments * fix test names * lint * add godoc and further comments * update go mod for e2e * update liq change logic * update uptime accums before gettings them for tick crossing * abstract uptime init and update logic to helper * add thorough tests for initOrUpdatePositionUptime * update liquidity update logic and tests to use new helper * add todo for frozenuntil issue * lint * lint * clean up logic and add tests for helper * go mod for osmoutils e2e * move position key computation outside loop * clean up imports and go sum * fix imports * [CL Incentives][bugfix]: Replace all uses of frozenUntil with joinTime and freezeDuration * e2e passing * implement message, proto, and core logic * convert freezeduration to uint64 * implement tests for collectIncentives * add msg level tests and clean up logic/tests * add codec, cli, and simulation * claim incentives upon position deletion to clear records * comment cleanup * lint * minor test comment updates Co-authored-by: Roman <[email protected]> * godoc update Co-authored-by: Roman <[email protected]> * expand withdraw tests to better include incentives logic * minor optimizations and cleanup from code review * [CL Incentives] Implement `CreateIncentive` logic and message (#4519) * draft implementation * begin testing * add more test cases * add error catching test cases * implement createIncentive message * cli and codec * add check and related tests * add further tests and lint * add test case for existing incentive records * Revert "[CL Incentives] Implement `CreateIncentive` logic and message (#4519)" (#4568) This reverts commit b07e763. * fix conflicts --------- Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: Sishir Giri <[email protected]> Co-authored-by: Roman <[email protected]>

* initial push * push * untracked files * add untracked files * merge branches * add changes required for fees * lint * fix test errors * clean up * remove print lines * feat: single fee accum (#4116) * single fee accum * lint * no longer need to get all positions for fee * incentive accum initialization * lint * Update x/concentrated-liquidity/store_test.go Co-authored-by: Sishir Giri <[email protected]> * add lower level tests and make uptime accum access private * lint * add gotests for helper fn * Update test to use existing variable Co-authored-by: Roman <[email protected]> * create or update records upon position creation/update * add tests and clean up * sync position-related changes * add support for negative liquidity delta * add and test helper & set up tick initialization logic * implement accumulator updates on tick crossing and clean up proto * clean up comments * minor test cleanup * ensure accum initalization follows conventions and clean up tests * update pool proto and CL pool extension for tracking liq changes * update new field at pool initialization and add tests * comment todos * clean up naming and move empty options to global var * add clarifying comments and lint * implement IncentiveRecord proto * add comments * lint * set up wiring for proto * implement updateUptimeAccumulatorsToNow * implement and test calcAccruedIncentivesForAccum * add tests for new helpers * minor comment updates Co-authored-by: Roman <[email protected]> * accum test updates * lint * clean up diff * lint * update go.mod for e2e * clean up proto and add error message * lint * sketch out testing approach * set up keys * clean up diff * clean up comments * lint * implement set and get for incentive records * comment cleanup * minor comment and test cleanup * clean up tests and comments * [CL Incentives] Implement `setIncentiveRecords` and `getIncentiveRecords` (#4283) * set up keys * implement set and get for incentive records * comment cleanup * minor comment and test cleanup * Revert "[CL Incentives] Implement `setIncentiveRecords` and `getIncentiveRecords` (#4283)" (#4294) This reverts commit de7c146. * set incentive records in new position tests * fix merge conflicts * add check for init position record values * fix and finalize uptime accum position tests * fix go mod for e2e * clean up diff * add err check to accum update * fix rounding error * implement tick init uptime tracker logic * fix test compatibility * implement fix for global accums falling out of sync before tick init * add tests to ensure uptime trackers are initialized properly * add comment for new helper * add core logic * go mod tidy * fix conflicts * fix e2e * update go mod * fix and further test tick init logic * remove redundant values from incentive record state * cleanup from review * comment cleanup * set up cross tick tests * add further tests * test and comment cleanup * implement core logic * update osmoutils go mod and remove curTick as fn param * thorough tests for new functionality * clean up new functions for readability * clean up test comments * fix test names * lint * add godoc and further comments * update go mod for e2e * update liq change logic * update uptime accums before gettings them for tick crossing * abstract uptime init and update logic to helper * add thorough tests for initOrUpdatePositionUptime * update liquidity update logic and tests to use new helper * add todo for frozenuntil issue * lint * lint * clean up logic and add tests for helper * go mod for osmoutils e2e * move position key computation outside loop * clean up imports and go sum * fix imports * [CL Incentives][bugfix]: Replace all uses of frozenUntil with joinTime and freezeDuration * e2e passing * implement message, proto, and core logic * convert freezeduration to uint64 * implement tests for collectIncentives * add msg level tests and clean up logic/tests * draft implementation * begin testing * add codec, cli, and simulation * claim incentives upon position deletion to clear records * comment cleanup * add more test cases * lint * add error catching test cases * implement createIncentive message * cli and codec * add check and related tests * add further tests and lint * add test case for existing incentive records * minor test comment updates Co-authored-by: Roman <[email protected]> * godoc update Co-authored-by: Roman <[email protected]> * expand withdraw tests to better include incentives logic * minor optimizations and cleanup from code review * [CL Incentives] Implement `CreateIncentive` logic and message (#4519) * draft implementation * begin testing * add more test cases * add error catching test cases * implement createIncentive message * cli and codec * add check and related tests * add further tests and lint * add test case for existing incentive records * Revert "[CL Incentives] Implement `CreateIncentive` logic and message (#4519)" (#4568) This reverts commit b07e763. * fix conflicts --------- Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: Sishir Giri <[email protected]> Co-authored-by: Roman <[email protected]>

* initial push * push * untracked files * add untracked files * merge branches * add changes required for fees * lint * fix test errors * clean up * remove print lines * feat: single fee accum (#4116) * single fee accum * lint * no longer need to get all positions for fee * incentive accum initialization * lint * Update x/concentrated-liquidity/store_test.go Co-authored-by: Sishir Giri <[email protected]> * add lower level tests and make uptime accum access private * lint * add gotests for helper fn * Update test to use existing variable Co-authored-by: Roman <[email protected]> * create or update records upon position creation/update * add tests and clean up * sync position-related changes * add support for negative liquidity delta * add and test helper & set up tick initialization logic * implement accumulator updates on tick crossing and clean up proto * clean up comments * minor test cleanup * ensure accum initalization follows conventions and clean up tests * update pool proto and CL pool extension for tracking liq changes * update new field at pool initialization and add tests * comment todos * clean up naming and move empty options to global var * add clarifying comments and lint * implement IncentiveRecord proto * add comments * lint * set up wiring for proto * implement updateUptimeAccumulatorsToNow * implement and test calcAccruedIncentivesForAccum * add tests for new helpers * minor comment updates Co-authored-by: Roman <[email protected]> * accum test updates * lint * clean up diff * lint * update go.mod for e2e * clean up proto and add error message * lint * sketch out testing approach * set up keys * clean up diff * clean up comments * lint * implement set and get for incentive records * comment cleanup * minor comment and test cleanup * clean up tests and comments * [CL Incentives] Implement `setIncentiveRecords` and `getIncentiveRecords` (#4283) * set up keys * implement set and get for incentive records * comment cleanup * minor comment and test cleanup * Revert "[CL Incentives] Implement `setIncentiveRecords` and `getIncentiveRecords` (#4283)" (#4294) This reverts commit de7c146. * set incentive records in new position tests * fix merge conflicts * add check for init position record values * fix and finalize uptime accum position tests * fix go mod for e2e * clean up diff * add err check to accum update * fix rounding error * implement tick init uptime tracker logic * fix test compatibility * implement fix for global accums falling out of sync before tick init * add tests to ensure uptime trackers are initialized properly * add comment for new helper * add core logic * go mod tidy * fix conflicts * fix e2e * update go mod * fix and further test tick init logic * remove redundant values from incentive record state * cleanup from review * comment cleanup * set up cross tick tests * add further tests * test and comment cleanup * implement core logic * update osmoutils go mod and remove curTick as fn param * thorough tests for new functionality * clean up new functions for readability * clean up test comments * fix test names * lint * add godoc and further comments * update go mod for e2e * update liq change logic * update uptime accums before gettings them for tick crossing * abstract uptime init and update logic to helper * add thorough tests for initOrUpdatePositionUptime * update liquidity update logic and tests to use new helper * add todo for frozenuntil issue * lint * lint * clean up logic and add tests for helper * go mod for osmoutils e2e * move position key computation outside loop * clean up imports and go sum * fix imports * [CL Incentives][bugfix]: Replace all uses of frozenUntil with joinTime and freezeDuration * e2e passing * implement message, proto, and core logic * convert freezeduration to uint64 * implement tests for collectIncentives * add msg level tests and clean up logic/tests * draft implementation * begin testing * add codec, cli, and simulation * claim incentives upon position deletion to clear records * comment cleanup * add more test cases * lint * add error catching test cases * implement createIncentive message * cli and codec * add check and related tests * add further tests and lint * add test case for existing incentive records * pull relevant changes into new branch * lint * minor test comment updates Co-authored-by: Roman <[email protected]> * godoc update Co-authored-by: Roman <[email protected]> * expand withdraw tests to better include incentives logic * minor optimizations and cleanup from code review * [CL Incentives] Implement `CreateIncentive` logic and message (#4519) * draft implementation * begin testing * add more test cases * add error catching test cases * implement createIncentive message * cli and codec * add check and related tests * add further tests and lint * add test case for existing incentive records * Revert "[CL Incentives] Implement `CreateIncentive` logic and message (#4519)" (#4568) This reverts commit b07e763. * make unit tests more robust * minor comment cleanup * fix conflicts --------- Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: Sishir Giri <[email protected]> Co-authored-by: Roman <[email protected]>

AlpinYukseloglu added 9 commits March 4, 2023 15:29

draft implementation

620602b

begin testing

6e1c365

add more test cases

43573c3

add error catching test cases

fe7a39f

implement createIncentive message

efe72bc

Merge branch 'alpo-uptime-claim' into alpo-create-incentive

3abe2a8

cli and codec

580a7ca

add check and related tests

21268ba

add further tests and lint

c863344

AlpinYukseloglu added V:state/breaking State machine breaking PR C:x/concentrated-liquidity labels Mar 6, 2023

github-actions bot added the C:CLI label Mar 6, 2023

add test case for existing incentive records

a8c6d4c

AlpinYukseloglu requested review from czarcas7ic, p0mvn and stackman27 March 6, 2023 19:45

stackman27 approved these changes Mar 6, 2023

View reviewed changes

stackman27 reviewed Mar 6, 2023

View reviewed changes

czarcas7ic approved these changes Mar 7, 2023

View reviewed changes

p0mvn approved these changes Mar 8, 2023

View reviewed changes

Merge branch 'alpo-uptime-claim' into alpo-create-incentive

2acb254

AlpinYukseloglu merged commit b07e763 into alpo-uptime-claim Mar 10, 2023

AlpinYukseloglu deleted the alpo-create-incentive branch March 10, 2023 23:06

AlpinYukseloglu restored the alpo-create-incentive branch March 10, 2023 23:08

AlpinYukseloglu added a commit that referenced this pull request Mar 10, 2023

Revert "[CL Incentives] Implement CreateIncentive logic and message (…

ce4531e

…#4519)" This reverts commit b07e763.

AlpinYukseloglu mentioned this pull request Mar 10, 2023

Revert "[CL Incentives] Implement CreateIncentive logic and message" #4568

Merged

AlpinYukseloglu added a commit that referenced this pull request Mar 10, 2023

Revert "[CL Incentives] Implement CreateIncentive logic and message (…

bded5b6

…#4519)" (#4568) This reverts commit b07e763.

AlpinYukseloglu mentioned this pull request Mar 10, 2023

[CL Incentives] Implement CreateIncentive logic and message #4569

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CL Incentives] Implement `CreateIncentive` logic and message #4519

[CL Incentives] Implement `CreateIncentive` logic and message #4519

AlpinYukseloglu commented Mar 6, 2023 •

edited

Loading

stackman27 left a comment

stackman27 Mar 6, 2023 •

edited

Loading

czarcas7ic Mar 7, 2023

stackman27 Mar 6, 2023

czarcas7ic Mar 7, 2023

stackman27 left a comment

czarcas7ic left a comment

czarcas7ic Mar 7, 2023

czarcas7ic Mar 7, 2023

czarcas7ic Mar 7, 2023

czarcas7ic Mar 7, 2023

czarcas7ic Mar 7, 2023

p0mvn Mar 8, 2023

p0mvn Mar 8, 2023

p0mvn Mar 8, 2023

mergify bot commented Mar 10, 2023

		Flags: osmocli.FlagDesc{RequiredFlags: []*flag.FlagSet{FlagSetJustPoolId()}},
		}, &types.MsgCreateIncentive{}

[CL Incentives] Implement CreateIncentive logic and message #4519

[CL Incentives] Implement CreateIncentive logic and message #4519

Conversation

AlpinYukseloglu commented Mar 6, 2023 • edited Loading

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

stackman27 left a comment

Choose a reason for hiding this comment

stackman27 Mar 6, 2023 • 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

stackman27 left a comment

Choose a reason for hiding this comment

czarcas7ic left a 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

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

mergify bot commented Mar 10, 2023

[CL Incentives] Implement `CreateIncentive` logic and message #4519

[CL Incentives] Implement `CreateIncentive` logic and message #4519

AlpinYukseloglu commented Mar 6, 2023 •

edited

Loading

stackman27 Mar 6, 2023 •

edited

Loading