Fee Revenue Decay Rate set to per topic's epoch rather than global fixed constant #529
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
relyt29
requested review from
xmariachi,
jmdkastro,
fernandofcampos,
kpeluso,
guilherme-brandao and
RedBird96
relyt29
added a commit
that referenced
this pull request
Aug 17, 2024
## Purpose of Changes and their Description #528 and others overwite the protobuf in the v2 folder of the emissions module. this is the incorrect way to perform software upgrades. Future changes to protobufs must go in their own v3 folder and the v2 must stay pinned to v0.3.0 This PR reverts the changes and introduces a new v3 folder for working with. ## Link(s) to Ticket(s) or Issue(s) resolved by this PR No written linear tasks. ## Are these changes tested and documented? Covered by existing unit tests. ## Still Left Todo Migrations for the new parameters in #528. I already wrote migration code in #525 and #529 so I recommend either basing off of those or waiting for at a miniumum, #525 to be merged.
Base automatically changed from
tyler/proto-2052-apply-parameter-tunings2
to
dev
August 17, 2024 12:38
relyt29
changed the title
|
this will wait until after 532, 533 are merged |
|
The latest Buf updates on your PR. Results from workflow Buf Linter / buf (pull_request).
|
relyt29
force-pushed
the
tyler/proto-2150-update-effective-revenue-decay-rate
branch
from
78d9700 to
62a41f1
Compare
relyt29
changed the title
| @@ -156,6 +156,9 @@ type Keeper struct { | |||
| // map of (topic) -> unfulfilled nonces | |||
| unfulfilledReputerNonces collections.Map[TopicId, types.ReputerRequestNonces] | |||
|
|
|||
| // map of (topic) -> last dripped block | |||
| lastDripBlock collections.Map[TopicId, BlockHeight] | |||
There was a problem hiding this comment.
Do we need a migration for this?
There was a problem hiding this comment.
so I added it to x/emissions/keeper/genesis.go InitGenesis and ExportGenesis functions, but as far as I can tell... no? because we would expect it to be initialized to zero, so there is no action for a migration function to take?
if you think otherwise I am all ears
guilherme-brandao
approved these changes
Aug 21, 2024
| @@ -1345,6 +1367,25 @@ func (k *Keeper) ExportGenesis(ctx context.Context) (*types.GenesisState, error) | |||
| return nil, errors.Wrap(err, "failed to get previous percentage reward to staked reputers") | |||
| } | |||
|
|
|||
| // openWorkerWindows | |||
| openWorkerWindows := make([]*types.BlockHeightAndTopicIds, 0) | |||
There was a problem hiding this comment.
Do we really need to create a specific structure just for the import/export? Is it the only way?
There was a problem hiding this comment.
I don't really understand the import export, but based on my partial understanding yes, this is what we have to do
relyt29
deleted the
tyler/proto-2150-update-effective-revenue-decay-rate
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose of Changes and their Description
Currently, we use a global parameter to drip revenue to the same extent across all topics, regardless of their epoch length. This is somewhat unfair, as topics of larger epochs then get dripped comparatively less than those with shorter epoch lengths.
@jmdkastro proposes a more appropriate way of updating effective revenue, that actually takes the topic epoch length into consideration (through calculation of "epochs per week"). Relevant text from the whitepaper:
The quantity
C_{t,i}decays each epoch by subtracting an amount∆ C_{t,i} = N_{epochs,w} * C_{t,i}(whereN_{epochs,w}is the number of epochs per week) and thus captures recent revenue.This PR modifies the keeper.go
DripTopicFeeRevenuefunction to do the following: identify if we have dripped yet this epoch. If we have not, then calculate the number of blocksPerWeek based on the blocksPerMonth, get the epochs per block from the topic, then do the math to calculate how much to decay for this epoch. Then it overwrites the fee revenue for this topic, and marks this epoch as having been decayed.This change add a new data structure to the keeper called
lastDripBlock. This change also deletes the global emissions module parameterTopicFeeRevenueDecayRateLink(s) to Ticket(s) or Issue(s) resolved by this PR
PROTO-2150
RES-302
Are these changes tested and documented?
Our existing unit tests were adapted to fit this new change, as some of them had broke from the different algorithms.