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

Adding rewards per block with premint #50

Closed
wants to merge 1 commit into from
Closed

Conversation

mart1n-xyz
Copy link

Description

Main changes:

  • currentRewardIndex() now uses rewardsPerBlockand block.number to determine the current rewardIndexbut does not update state. Hence, balances update in real time without a need for a transaction.
  • updateRewardIndex() updates state with the same logic.
  • setRewardsPerBlock() is an ownable function that changes rewardsPerBlock until rewardEndBlock, it calls REWARD_TOKEN.mint() for the amount need for this period given the two parameters. So, there should be always sufficient rewards ready in the contract.

Checklist

Ensure you completed all of the steps below before submitting your pull request:

  • Added natspec comments?
  • Ran pnpm adorno?
  • Ran pnpm verify?

@mart1n-xyz
Copy link
Author

This relates to the debate on total supply and some implementation details of the desirable UX. I wanted to show that

  1. we can have a system that updates balances in real time without the need for a transaction / state update while reward tokens are readily available in the contract (alternative is they can be minted on the go before any state update - actually may be a more elegant approach)
  2. the sacrifices to produce this (setting rewardsPerBlock ahead) are actually practical - one can preset the contract for a desirable period where the total rewards distributed can be result of "some formula" that is a function of, say, the total supply in the previous period (as Cyp suggested in the call). I'd say it is not practical to change setup or send rewards to the contract every block.
  3. with this system, one can determine the minted supply either based on staking contract stake and compute the other 2/3 allocations or, the other way, use arbitrary number to be minted and distributed via staking. So, anything goes. Obviously, this is incompatible with XP=MP

rewardIndex += (newRewards * SCALE_FACTOR) / totalStaked;
accountedRewards += newRewards;
if (block.number > rewardEndBlock) {
applicableBlocks = rewardEndBlock - lastRewardBlock;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is problematic because rewardEndBlock is not set during construction. Meaning, even when setRewardsPerBlock is called the first time, rewardEndBlock will be 0 if I'm not mistaken.

Copy link
Author

Choose a reason for hiding this comment

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

Good point, gotta admit I kind of neglected the initiation and edge cases 😅

// Update the rewards per block and reward end block
rewardsPerBlock = _rewardsPerBlock;
rewardEndBlock = block.number + _durationInBlocks;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, so here we're saying, we have an admin that is able to set some reward per block for some duration in blocks.
This allows for issuance of rewards "in the future" because there's a fixed amount of rewards being available for claim per block.

This is opposed to prior this change, where, at any point in time one would update the reward index with whatever rewards are in regardless of how many blocks have passed.

To achieve the same result, one would have to send REWARD_TOKEN to the staking contract on a per block basis, so that rewardIndex would be updatable on a per block basis with the newest rewards as well.

I think your proposed change is not bad
What I don't like that much is that the duration unit is blocks. Blocks time may very, so for the same segment of time, there could be different rewards because a different number of blocks has been processed.

I think it'd be more predictable to have a solution that uses actual time as duration instead.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, so here we're saying, we have an admin that is able to set some reward per block for some duration in blocks.

Or you can have epochs and rewards that arrived in the past one determine the current rate of rewards - and you do not need an admin.

What I don't like that much is that the duration unit is blocks.

Just wanted to show a minimal example and this was the easiest way to do it. In terms of UX, it does make sense to use time.

@mart1n-xyz
Copy link
Author

If setting rewardsPerBlock in advance is not desirable, the contract can have epoch: any rewards that arrived in previous epoch determine the rewardsPerBlock in the current epoch (past_rewards/blocks_in_epoch).

}

uint256 newRewards = applicableBlocks * rewardsPerBlock;
Copy link
Author

Choose a reason for hiding this comment

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

If one wants to eliminate the REWARD_TOKEN premint, this is where a mint call for newRewards can be instead. Then, rewards are minted as state is updated and not in advance.

@0x-r4bbit
Copy link
Collaborator

Thanks once again @mart1n-xyz for you contribution here. I'm closing this in favor of #74 which adds a similar functionality but it's more complete.

@0x-r4bbit 0x-r4bbit closed this Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants