Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DNM: incentive module proto #1514

Closed
wants to merge 50 commits into from
Closed

DNM: incentive module proto #1514

wants to merge 50 commits into from

Conversation

toteki
Copy link
Member

@toteki toteki commented Oct 23, 2022

Marking as DO NOT MERGE so I can break it up into smaller PRs

This PR contains the first step in the incentive module

Progress:

  • Proto files
  • Proto gen
  • Implement Msg interfaces

Design of gov proposals:

I have created two options for funding incentive programs:

  • one that uses a community fund or known address to fund a program the moment it passes governance
  • another that creates the program and sets its total rewards via governance, but requires a basic account to fund it after passing or it won't start.

Two step process (basic account)

If doing external incentives, we may want to let the source of the funding use a basic account. In that case, MsgGovCreateProgram (and governance vote) is used to set the program's max_rewards but NOT fund it.

Then, a MsgSponsorProgram can be used by the basic account, which must fund the exact amount specified in max_rewards or the program will not start / activate.

One step process (community fund)

For ordinary programs that use the community fund, or instances where external incentives are already waiting at a known address, a MsgCreateAndSponsorProgram is used to do it all in one step.


Author Checklist

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • added appropriate labels to the PR
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

@toteki toteki mentioned this pull request Oct 26, 2022
10 tasks
@toteki toteki changed the title DNM - Incentive module step 1 feat: incentive module proto Oct 27, 2022
@toteki toteki marked this pull request as ready for review October 27, 2022 18:13
@toteki toteki requested a review from a team as a code owner October 27, 2022 18:13
@toteki
Copy link
Member Author

toteki commented Oct 27, 2022

Marking this R4R - since adding the proto files without hooking up module to the app does not mess with the build in main / release branch, it should be safe to merge.

Copy link
Member

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • please follow the 0.46 module organization: types should be generated directly into the root package of the module, together with codec, errors and keys.
  • don't use yaml tags - they are deprecated
  • don't use paramtypes.ParamSet - new gov mechanism should be used instead when possible.
  • tx.go with sdk.Msg implementations should be renamed to msg.go

@codecov-commenter
Copy link

codecov-commenter commented Nov 16, 2022

Codecov Report

Merging #1514 (6e8e0fd) into main (bcd3e5a) will increase coverage by 0.15%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1514      +/-   ##
==========================================
+ Coverage   55.47%   55.63%   +0.15%     
==========================================
  Files          68       68              
  Lines        6725     6706      -19     
==========================================
  Hits         3731     3731              
+ Misses       2717     2698      -19     
  Partials      277      277              
Impacted Files Coverage Δ
x/leverage/types/msgs.go 6.52% <0.00%> (+1.90%) ⬆️

@toteki
Copy link
Member Author

toteki commented Nov 20, 2022

Switched all block height (unbondings, durations, program ends) to unix time.

Also split up where the incentive programs are stored in state and queried, so it's easier to validate them (different values should be zero/nonzero at different points in the program's lifecycle (upcoming, ongoing, completed).

Additionally, only the completed programs query has pagination - that's where programs will accumulate (the upcoming and ongoing sets will always have a limited amount of programs in them)

Copy link
Member

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Usually, the best is to keep the first PR only proto oriented + generated files. This way we firstly deal with proto, without messing with validation etc... But the validation is already here, so we can go with that. So let's make the review in mulitple stage, here I firstly review the proto.
  1. Let's don't use params - it's officially deprecated. See my comment how to move forward with that.
  2. Let's add a create_program_fee param to avoid a spam (maybe we can discuss it in notion). This will be paid in Umee.
  3. Please create a notion page to specify functionality (not a design doc). Specifically:
    • how the programs will work with uTokens?
    • Can there be multiple programs for the same uToken with overlapping period?
    • Do we support auto compounding (now, or v2?)?
    • Can programs be extended or resumed?
    • Can we partially fund a program?
    • Maybe we should support a scenario when community fund will only partially sponsor a program? In that case we would need to add ratio parameter to MsgGovCreateAndSponsorProgram

Comment on lines +29 to +30
// BondAmount tracks the amount of coins of one uToken denomination bonded to a
// given reward tier by a single account.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// BondAmount tracks the amount of coins of one uToken denomination bonded to a
// given reward tier by a single account.
// Bond tracks the user rewards per tire and uToken coin.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can simplify the name, otherwise we would need to call it UserTireBond or UserTireCoinBond

rpc UpcomingIncentivePrograms(QueryUpcomingIncentivePrograms)
returns (QueryUpcomingIncentiveProgramsResponse) {
option (google.api.http).get = "/umee/incentive/v1/incentive_programs/upcoming";
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think UI will usually need all programs, or both ongoing and upcoming. So, instead of having 3 queries, we can have either:

  1. 2 queries: past and ongoing+future (with a flag future [default = false]
  2. 1 query: pas + ongoing + future (with flag past [default = false])

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frontend will probably just need current, and it's easier to have them stored separately in state since it reduces iterations when looking for just one type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. So you are moving the program between storage prefixes?
  2. My question was more related to the query, why having 3 queries if we can only have 1 or 2? That would reduce and simplify the code.
  3. Let's verify with the frontend. My gut feeling is that they will like to show both present and future.

rpc BeginUnbonding(MsgBeginUnbonding) returns (MsgBeginUnbondingResponse);

// Sponsor defines a permissionless method for sponsoring an upcoming, not yet funded incentive program.
rpc Sponsor(MsgSponsor) returns (MsgSponsorResponse);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think w should enable partial sponsoring, when 2 separate sponsors will like to sponsor a program. Or do it in v2.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial sponsoring creates an edge case, where if a program is not fully sponsored by the time it starts, the module needs to either

  • remember the address of any sponsor and refund the tokens
  • start the program with actual rewards != governance agreed rewards

Currently, a program is either not funded, or fully funded, so such scenarios are prevented

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but maybe we need to support a scenario when, for example we have 2 sponsors?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something that could be useful in the future, but probably not near-term. Possible use case would be ecosystems with multiple tokens like Comdex. Maybe they want to incentivize borrowing their CMST stablecoin with CMDX, HARBOR, etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I was thinking about similar scenario. Other would be if there are two independent accounts which would sponsor only if the proposal passes (Eg: Umee DAO and Osmosis DAO).

// GovCreateProgram is used by governance proposals to create (but not fund) an incentive program.
rpc GovCreateProgram(MsgGovCreateProgram) returns (MsgGovCreateProgramResponse);

// GovCreateAndSponsorProgram is used by governance proposals to create and fund an incentive program.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// GovCreateAndSponsorProgram is used by governance proposals to create and fund an incentive program.
// GovCreateAndSponsorProgram is used by governance proposals to create an incentive program
// and sponsor it from the Community Fund.

// MsgGovCreateAndSponsorProgram is used by governance to create an incentive program
// and fund it in the same transaction. The sponsor address should be the community
// fund or an agreed upon account - program reward funds will be taken from that address
// when the incentive program passes governance.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no sponsor parameter in this message. Let's just write that "when passed, the program is fully sponsored by the Community Fund".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, I'll need to hardcode (or add to Params) a community fund address.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: sponsor parameter is currently below the highlighted comment - it is the last field

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You check the Community Fund address during the execution. It's not a param, because it doesn't change.

toteki and others added 6 commits November 21, 2022 07:53
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
@toteki
Copy link
Member Author

toteki commented Nov 21, 2022

Please create a notion page to [...]

Most of there questions are already answered in the design document. I'll copypaste the answers here but I think they'll eventually end up in a spec

how the programs will work with uTokens?

Users lock and unlock `uTokens` they have enabled as collateral in `x/leverage` by sumbitting `x/incentive` message types.

Locked uTokens remain in the `x/leverage` module account.

Locking of funds is independent of active incentive programs, and can even be done in their absence.

and

Locked funds must be collateral-enabled uTokens. Locked funds will not leave their original place in the leverage module account. Locking will prevent withdrawal and collateral-disabling (but not liquidation) of the locked uTokens until they are successfully unlocked.

Can there be multiple programs for the same uToken with overlapping period?

Incentive programs are created by governance. No message types to alter or halt incentive programs once voted on are planned, and any number of incentive programs should be capable of being active simultaneously regardless of parameters, including overlapping dates and denominations.

Do we support auto compounding (now, or v2?)?

no - not mentioned in any design

Can programs be extended or resumed?

Incentive programs are created by governance. No message types to alter or halt incentive programs once voted on are planned, and any number of incentive programs should be capable of being active simultaneously regardless of parameters, including overlapping dates and denominations.

Can we partially fund a program?

currently we're requiring full funding to prevent partial -> refund edge cases.

Maybe we should support a scenario when community fund will only partially sponsor a program? In that case we would need to add ratio parameter to MsgGovCreateAndSponsorProgram

Not planned

@toteki toteki changed the title feat: incentive module proto DNM: incentive module proto Nov 30, 2022
@toteki toteki mentioned this pull request Nov 30, 2022
17 tasks
@toteki toteki marked this pull request as draft December 1, 2022 18:59
@stale
Copy link

stale bot commented Dec 31, 2022

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the S:Stale label Dec 31, 2022
@stale stale bot closed this Jan 8, 2023
@toteki toteki deleted the adam/incentive-a branch April 27, 2023 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants