Beneficiary#113
Conversation
Dinonard
left a comment
Dinonard
left a comment
There was a problem hiding this comment.
Great work, that was super fast 😃
I've left some comments/questions and would like to hear your opinion.
| /// | ||
| /// The dispatch origin is the current beneficiary. | ||
| #[pallet::weight(10_000)] | ||
| pub fn update_rewards_beneficiary( |
There was a problem hiding this comment.
Perhaps delegate_rewards_beneficiary?
Since this cannot be used by the staker, so it's more clear.
| T::WeightInfo::claim_staker_without_restake() | ||
| }) | ||
| .into()) | ||
| Ok(Some(weight_info).into()) |
There was a problem hiding this comment.
Is this a personal preference or was the approach before incorrect?
There was a problem hiding this comment.
It's a personal preference, trying to make return logic simple and default based.
|
|
||
| T::Currency::resolve_creating(&staker, reward_imbalance); | ||
| // Withdraw reward funds from the dapps staking pot | ||
| let reward_imbalance = T::Currency::withdraw( |
There was a problem hiding this comment.
What if this fails at this point?
What state do we leave the chain in?
There was a problem hiding this comment.
The failed call will not affect the chain state, since the call by default is transactional.
There was a problem hiding this comment.
You're right, after paritytech/substrate#11431.
| let beneficiary = RewardsBeneficiary::<T>::get(&staker, &contract_id); | ||
| let mut weight_info = T::WeightInfo::claim_staker_without_restake(); | ||
|
|
||
| if beneficiary.is_none() && should_restake_reward { |
There was a problem hiding this comment.
I'm not fond of argument bloat, but perhaps the added check would be better suited inside fn should_restake_reward?
| T::Currency::resolve_creating(&reward_account, reward_imbalance); | ||
| Self::update_staker_info(&staker, &contract_id, staker_info); | ||
| Self::deposit_event(Event::<T>::Reward(staker, contract_id, era, staker_reward)); | ||
| Self::deposit_event(Event::<T>::Reward(reward_account, contract_id, era, staker_reward)); |
There was a problem hiding this comment.
Perhaps this is confusing a bit - Reward event would be read as X staked on Y and received Z amount of reward for era N.
Right now, beneficiary could be an account that's not even part of dapps-staking, so it'd be even more confusing.
Could you share your opinion on this?
Is there an alternative solution you could propose?
There was a problem hiding this comment.
Current Reward event reads as someone staked on Y and its beneficiary is X, X received Z amount of reward for era N.
Adding an extract field called staker makes sense for easily index on staker for reward events.
For UX, most users don't need to bother this beneficiary settings, by default the restake should just works fine. User only need to care about beneficiary if they are making a staking pool for others, delegate and beneficiary will make it clear in this case.
Instead of adding an extra storage, make the reward destination to include beneficiary seems more straight forward, but require more thorough design and changes.
There was a problem hiding this comment.
Sure, I like the scenarios, even though this is just a dummy-pre-screening-task feature :)
Differentiating when stakers receive rewards and when it's just via a reward beneficiary is important if we want to be aware how much someone earned as a staker. With the modified event, we cannot tell whether the user received rewards as a beneficiary or as a nomination reward.
And love the proposal about reward destination 👍 😄 , was hoping for it!
| }) | ||
| } | ||
|
|
||
| #[test] |
2 participants
Pull Request Summary
Check list