Staking rewards distribution #269
Conversation
nanocryk
changed the title
|
Master coverage: 72.48% |
|
Can we add description to the PR? it helps devrel categorize important/non-breaking prs etc |
|
|
||
| let rewards_per_share = rewards | ||
| .0 | ||
| .checked_div(&supply) |
There was a problem hiding this comment.
Here it is important that our rewards never surpass the supply of shares right? how do we control that?
There was a problem hiding this comment.
It is the opposite, it should be at least equal to the supply of shares to distribute 1 unit per share.
That's why T::InitialManualClaimShareValue should be big enough so that the supply is somewhat low and allow rewards to be properly distributed.
| manual_claim_rewards: delegators_manual_rewards, | ||
| }); | ||
|
|
||
| Ok(candidate_manual_rewards) |
There was a problem hiding this comment.
this function is very well documented and is easy to follow, congrats!
I'm working on the inflation mechanism in the branch elois-pallet-rewards, when it's advanced enough I'll open the PR against |
| @@ -96,3 +99,7 @@ pub mod well_known_keys { | |||
| pub const SESSION_INDEX: &[u8] = | |||
| &hex_literal::hex!["cec5070d609dd3497f72bde07fc96ba072763800a36a99fdfc7c10f6415f6ee6"]; | |||
| } | |||
|
|
|||
| pub trait DistributeRewards<AccountId, Balance> { | |||
There was a problem hiding this comment.
Even if trivial, lets try to add some doc to the trait too
Yep I think is fine, we can merge this as is and work on integration tests once your PR is ready |
|
Also ideally, we should benchmark the reward distribution so that we can at least report the utilized weight |
Co-authored-by: tmpolaczyk <[email protected]>
pallets/pooled-staking/src/pools.rs
Outdated
| /// AutoCompounding shares. This can lead to some rounding, which will be | ||
| /// absorbed in the ManualRewards distribution, which simply consist of | ||
| /// transfering the funds to the candidate account. | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
| #[allow(dead_code)] |
|
Not sure if intended, but this test passes: #[test]
fn candidate_only_no_stake() {
// Rewarding a candidate that does not have any stake works
ExtBuilder::default().build().execute_with(|| {
test_distribution(
&[],
RewardRequest {
collator: ACCOUNT_CANDIDATE_1,
rewards: 1_000_000,
},
&[],
Distribution {
collator_auto: 0,
collator_manual: 1_000_000, // 100% of rewards
delegators_auto: 0,
delegators_manual: 0, // 0% of rewards
},
)
});
}You can add that test if that's intended, or add a check that the candidate has enough shares if this should be an error. Also these is the case where the candidate does not have any shares but some delegator does, not sure what should happen in that case: Expand#[test]
fn delegator_only_candidate_zero() {
// Rewarding a candidate that does not have any stake works
ExtBuilder::default().build().execute_with(|| {
test_distribution(
&[Delegation {
candidate: ACCOUNT_CANDIDATE_1,
delegator: ACCOUNT_DELEGATOR_1,
pool: TargetPool::ManualRewards,
stake: 250_000_000,
}],
RewardRequest {
collator: ACCOUNT_CANDIDATE_1,
rewards: 1_000_000,
},
&[],
Distribution {
collator_auto: 0,
collator_manual: 200_000, // 20% of rewards
delegators_auto: 0,
delegators_manual: 800_000, // 80% of rewards
},
)
});
}Also another related test case that results in a division by zero: Expand#[test]
fn delegator_only_candidate_no_stake_auto_compounding() {
// Rewarding a candidate that does not have any stake, while some delegator has stake for that candidate
ExtBuilder::default().build().execute_with(|| {
test_distribution(
&[Delegation {
candidate: ACCOUNT_CANDIDATE_1,
delegator: ACCOUNT_DELEGATOR_1,
pool: TargetPool::AutoCompounding,
stake: 250_000_000,
}],
RewardRequest {
collator: ACCOUNT_CANDIDATE_1,
rewards: 1_000_000,
},
&[],
Distribution {
collator_auto: 0,
collator_manual: 200_000, // 20% of rewards
delegators_auto: 0,
delegators_manual: 800_000, // 80% of rewards
},
)
});
} |
Add staking reward distribution to the Auto Compounding and Manual Rewards pools.
The function is not called automatically and must be called by another pallet to manage the inflation and other details.