Skip to content

Conversation

@wischli
Copy link
Contributor

@wischli wischli commented Jul 20, 2022

fixes KILTProtocol/ticket#1930

Follow-up PRs

TLDR Description

  • Refactors staking payouts to not happen automatically
    • Instead, a counter (similar to the Substrate staking pallet participation points) is incremented for each authored block
    • The counter can be consumed and converted into rewards
  • Adds extrinsic claim_rewards which enables any signed origin to trigger a payout for themselves

Lengthy Description

The proposed solution goes more into the direction of Substrate staking. Every time a collator authors a block, their rewards counter is incremented. The counter can be consumed to increment accumulated rewards. The rewards need to be actively claimed by the new claim_rewards_for extrinsic.

There are a lot of events which automatically consume the RewardCounter and increment the accumulated rewards since the last actively requested payout:

  • Collator changes stake (more, less)
  • A delegator changes stake (more, less)
  • A delegator removes delegation
  • A delegator is replaced
  • A collator leaves
  • The inflation configuration changes

Background

The main reason for this refactor is that the current solution is not highly scalable as it inherently emits a Rewarded event for each address that gets rewarded. For the current configuration of delegators, this is rather unproblematic.

Moreover, we currently enforce the taxable Rewarded event onto all stakers and it occurs very frequently. Therefore, it is quite a challenge to summarize all such events.

TODO

  • More thorough description
  • Add claiming extrinsic
  • Refactor yearly inflation adjustment
  • Unit tests
  • Benchmarks

Extrinsics

  • set_inflation
  • force_remove_candidate
  • init_leave_candidates
  • candidate_stake_more
  • candidate_stake_less
  • join_delegators
  • delegate_another_candidate
  • leave_delegators
  • revoke_delegation
  • delegator_stake_more
  • delegator_stake_less

Checklist:

  • I have verified that the code works
    • No panics! (checked arithmetic ops, no indexing array[3] use get(3), ...)
  • I have verified that the code is easy to understand
    • If not, I have left a well-balanced amount of inline comments
  • I have left the code in a better state
  • I have documented the changes (where applicable)

Copy link
Contributor

@ntn-x2 ntn-x2 left a comment

Choose a reason for hiding this comment

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

It will probably look much easier to follow and understand once refactored, so we are on the right path 🙂


// TODO: Benchmark, unit tests, docs
#[pallet::weight(0)]
pub fn claim_rewards_for(origin: OriginFor<T>, target: <T::Lookup as StaticLookup>::Source) -> DispatchResult {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any specific reason why we would allow anyone to trigger a payout for anyone else? I still don't understand what would be the use case for this, if I want to have control over when I would claim my rewards. If I want to delegate, I could set up a proxy,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO, the main reason would be to cleanup storage and consume tiny rewards that have not been claimed by potentially exited users. I understand that's a very unlikely scenario and each user has an incentive to claim their rewards.

Moreover, in frame staking this is also possible by triggering a payout for a validator which rewards their nominators as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am completely fine changing this and would be interested in @weichweich 's opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think Parity does that because otherwise users would just forget and keep piling up stuff, I think. Which would be fine in our case if that is the assumption. But of course the downside is that I can't simply decide to trigger my reward at any time, because someone else inside my delegation pool might do it for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's first start with the more restrictive version. We might be able to change that later if we feel the need for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By more restrictive you are referring to Antonio's proposal, e.g. origins can only claim for themselves?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wischli wischli mentioned this pull request Aug 1, 2022
8 tasks
wischli and others added 6 commits August 1, 2022 18:16
* tests: 80%

* feat: rm delegate_another_candidate and more

* docs: remove deprecated weight

* Apply suggestions from code review

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

Co-authored-by: Antonio <[email protected]>
@wischli wischli marked this pull request as ready for review August 8, 2022 12:09
@wischli wischli requested review from ntn-x2 and weichweich August 8, 2022 12:10
Copy link
Contributor

@ntn-x2 ntn-x2 left a comment

Choose a reason for hiding this comment

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

I have one main question about the rewards for delegators. It seems to me they are incentivised to jump onboard right before some other event is supposed to happen (like a stake change or an inflation change). Is my understanding correct? Is that by design? Or am I missing something?

Copy link
Contributor

@ntn-x2 ntn-x2 left a comment

Choose a reason for hiding this comment

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

LGTM!

@ntn-x2 ntn-x2 force-pushed the wf/1930-staking-refactor branch from b98a254 to 02eb407 Compare September 20, 2022 12:28
@wischli wischli added this to the 1.9.0 milestone Oct 5, 2022
@wischli
Copy link
Contributor Author

wischli commented Oct 31, 2022

@weichweich Please review when you have the time, thanks!

Copy link
Contributor

@weichweich weichweich left a comment

Choose a reason for hiding this comment

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

Looks good. I would say we can merge it as it is. I just have a few naming suggestions and the RewardCounter is a bit hard to understand. Maybe my suggestion could make this part a bit easier to understand?

The yearly inflation config update is moved from on_initialize to a call that can be called by anyone? Is there a reason for that?

@wischli wischli requested a review from weichweich November 2, 2022 11:23
@weichweich weichweich modified the milestones: 1.9.0, 1.8.0 Nov 3, 2022
Copy link
Contributor

@weichweich weichweich left a comment

Choose a reason for hiding this comment

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

LGTM very beautiful PR. Love the links to the commits. Nice comments.

@wischli wischli enabled auto-merge (squash) November 3, 2022 06:52
@wischli wischli merged commit e7aafd0 into develop Nov 3, 2022
@wischli wischli deleted the wf/1930-staking-refactor branch November 3, 2022 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants