Skip to content
This repository was archived by the owner on Sep 28, 2023. It is now read-only.

Beneficiary #113

Closed
kaichaosun wants to merge 3 commits into from
Closed

Beneficiary #113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4e47849
Delegate staking rewards to a beneficiary.
kaichaosun Nov 9, 2022
665064a
adding tests
kaichaosun Nov 9, 2022
90efe8f
fix claim_staker.
kaichaosun Nov 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion frame/dapps-staking/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@
//! - `maintenance_mode` - enables or disables pallet maintenance mode
//! - `set_reward_destination` - sets reward destination for the staker rewards
//! - `set_contract_stake_info` - root-only call to set storage value (used for fixing corrupted data)
//! - `set_rewards_beneficiary` - set the beneficary of a staker's rewards
//! - `remove_rewards_beneficiary` - remove the beneficary of a staker's rewards
//! - `update_rewards_beneficiary` - called by the beneficary and update the beneficary of a staker's rewards to a new account
//!
//! User is encouraged to refer to specific function implementations for more comprehensive documentation.
//!
Expand Down Expand Up @@ -484,7 +487,7 @@ where
}
}

/// Instruction on how to handle reward payout for stakers.
/// Instruction on how to handle reward payout for stakers when beneficiary is not set.
/// In order to make staking more competitive, majority of stakers will want to
/// automatically restake anything they earn.
#[derive(Encode, Decode, Clone, Copy, PartialEq, Eq, RuntimeDebug, TypeInfo)]
Expand Down
141 changes: 120 additions & 21 deletions frame/dapps-staking/src/pallet/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,19 @@ pub mod pallet {
ValueQuery,
>;

/// Beneficiary of staking rewards on perticular contract.
/// `(staker, contract_id) -> beneficiary_account_id`
#[pallet::storage]
#[pallet::getter(fn staking_beneficiary)]
pub type RewardsBeneficiary<T: Config> = StorageDoubleMap<
_,
Twox64Concat,
T::AccountId,
Blake2_128Concat,
T::SmartContract,
T::AccountId,
>;

/// Stores the current pallet storage version.
#[pallet::storage]
#[pallet::getter(fn storage_version)]
Expand Down Expand Up @@ -233,6 +246,12 @@ pub mod pallet {
BalanceOf<T>,
T::SmartContract,
),
/// Staking rewards beneficiary is set
BeneficiarySet(T::AccountId, T::SmartContract, T::AccountId),
/// Staking rewards beneficiary is removed
BeneficiaryRemoved(T::AccountId, T::SmartContract),
/// Staking rewards beneficiary is updated
BeneficiaryUpdated(T::AccountId, T::SmartContract, T::AccountId),
}

#[pallet::error]
Expand Down Expand Up @@ -291,6 +310,12 @@ pub mod pallet {
NotActiveStaker,
/// Transfering nomination to the same contract
NominationTransferToSameContract,
/// There is no beneficiary set for the staker per contract
BeneficiaryNotSet,
/// Not allow non-beneficiary to update
UpdateBeneficiaryNotAllowed,
/// Beneficiary used is not valid
InvalidBeneficiary,
}

#[pallet::hooks]
Expand Down Expand Up @@ -709,8 +734,10 @@ pub mod pallet {
/// Claim earned staker rewards for the oldest unclaimed era.
/// In order to claim multiple eras, this call has to be called multiple times.
///
/// The rewards are always added to the staker's free balance (account) but depending on the reward destination configuration,
/// they might be immediately re-staked.
/// When [`RewardsBeneficiary`] is set, the rewards are added to the beneficiary's free balance and unlocked.
/// When RewardsBeneficiary is not set, the rewards are added to,
/// - staker's free balance but locked with [`RewardDestination`] is StakeBalance
/// - staker's free balance and unlocked with [`RewardDestination`] is FreeBalance
#[pallet::weight(T::WeightInfo::claim_staker_with_restake().max(T::WeightInfo::claim_staker_without_restake()))]
pub fn claim_staker(
origin: OriginFor<T>,
Expand Down Expand Up @@ -751,7 +778,10 @@ pub mod pallet {
staker_info.latest_staked_value(),
);

if should_restake_reward {
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 {
Copy link
Collaborator

@Dinonard Dinonard Nov 14, 2022

Choose a reason for hiding this comment

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

I'm not fond of argument bloat, but perhaps the added check would be better suited inside fn should_restake_reward?

Copy link
Author

@kaichaosun kaichaosun Nov 14, 2022

Choose a reason for hiding this comment

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

Sounds good!

staker_info
.stake(current_era, staker_reward)
.map_err(|_| Error::<T>::UnexpectedStakeInfoEra)?;
Expand All @@ -762,17 +792,7 @@ pub mod pallet {
staker_info.len() <= T::MaxEraStakeValues::get(),
Error::<T>::TooManyEraStakeValues
);
}

// Withdraw reward funds from the dapps staking pot
let reward_imbalance = T::Currency::withdraw(
&Self::account_id(),
staker_reward,
WithdrawReasons::TRANSFER,
ExistenceRequirement::AllowDeath,
)?;

if should_restake_reward {
ledger.locked = ledger.locked.saturating_add(staker_reward);
Self::update_ledger(&staker, ledger);

Expand All @@ -795,18 +815,24 @@ pub mod pallet {
contract_id.clone(),
staker_reward,
));

weight_info = T::WeightInfo::claim_staker_with_restake();
}

T::Currency::resolve_creating(&staker, reward_imbalance);
// Withdraw reward funds from the dapps staking pot
let reward_imbalance = T::Currency::withdraw(
Copy link
Collaborator

@Dinonard Dinonard Nov 14, 2022

Choose a reason for hiding this comment

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

What if this fails at this point?
What state do we leave the chain in?

Copy link
Author

@kaichaosun kaichaosun Nov 14, 2022

Choose a reason for hiding this comment

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

The failed call will not affect the chain state, since the call by default is transactional.

Copy link
Collaborator

@Dinonard Dinonard Nov 14, 2022

Choose a reason for hiding this comment

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

You're right, after paritytech/substrate#11431.

&Self::account_id(),
staker_reward,
WithdrawReasons::TRANSFER,
ExistenceRequirement::AllowDeath,
)?;

let reward_account = beneficiary.clone().unwrap_or(staker.clone());
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));
Copy link
Collaborator

@Dinonard Dinonard Nov 14, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Author

@kaichaosun kaichaosun Nov 14, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@Dinonard Dinonard Nov 14, 2022

Choose a reason for hiding this comment

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

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!


Ok(Some(if should_restake_reward {
T::WeightInfo::claim_staker_with_restake()
} else {
T::WeightInfo::claim_staker_without_restake()
})
.into())
Ok(Some(weight_info).into())
Copy link
Collaborator

@Dinonard Dinonard Nov 14, 2022

Choose a reason for hiding this comment

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

Is this a personal preference or was the approach before incorrect?

Copy link
Author

@kaichaosun kaichaosun Nov 14, 2022

Choose a reason for hiding this comment

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

It's a personal preference, trying to make return logic simple and default based.

}

/// Claim earned dapp rewards for the specified era.
Expand Down Expand Up @@ -961,6 +987,79 @@ pub mod pallet {
Ok(().into())
}

/// Sets the beneficiary of staking rewards.
///
/// The dispatch origin is staker.
#[pallet::weight(10_000)]
pub fn set_rewards_beneficiary(
origin: OriginFor<T>,
contract_id: T::SmartContract,
beneficiary: T::AccountId,
) -> DispatchResultWithPostInfo {
Self::ensure_pallet_enabled()?;
let staker = ensure_signed(origin)?;

ensure!(
Self::is_active(&contract_id),
Error::<T>::NotOperatedContract
);
ensure!(beneficiary != staker, Error::<T>::InvalidBeneficiary);

RewardsBeneficiary::<T>::insert(&staker, &contract_id, &beneficiary);

Self::deposit_event(Event::<T>::BeneficiarySet(staker, contract_id, beneficiary));
Ok(().into())
}

/// Removes the beneficiary of staking rewards.
///
/// The dispatch origin is staker.
#[pallet::weight(10_000)]
pub fn remove_rewards_beneficiary(
origin: OriginFor<T>,
contract_id: T::SmartContract,
) -> DispatchResultWithPostInfo {
Self::ensure_pallet_enabled()?;
let staker = ensure_signed(origin)?;

RewardsBeneficiary::<T>::remove(&staker, &contract_id);

Self::deposit_event(Event::<T>::BeneficiaryRemoved(staker, contract_id));
Ok(().into())
}

/// Updates the beneficiary to a new account.
///
/// The dispatch origin is the current beneficiary.
#[pallet::weight(10_000)]
pub fn update_rewards_beneficiary(
Copy link
Collaborator

@Dinonard Dinonard Nov 14, 2022

Choose a reason for hiding this comment

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

Perhaps delegate_rewards_beneficiary?
Since this cannot be used by the staker, so it's more clear.

origin: OriginFor<T>,
staker: T::AccountId,
contract_id: T::SmartContract,
new_beneficiary: T::AccountId,
) -> DispatchResultWithPostInfo {
Self::ensure_pallet_enabled()?;
let sender = ensure_signed(origin)?;

ensure!(
Self::is_active(&contract_id),
Error::<T>::NotOperatedContract
);

RewardsBeneficiary::<T>::try_mutate(&staker, &contract_id, |maybe_beneficiary| -> DispatchResultWithPostInfo {
let beneficiary = maybe_beneficiary.as_mut().ok_or(Error::<T>::BeneficiaryNotSet)?;
ensure!(sender == *beneficiary, Error::<T>::UpdateBeneficiaryNotAllowed);
ensure!(new_beneficiary != staker, Error::<T>::InvalidBeneficiary);

*beneficiary = new_beneficiary.clone();

Ok(().into())
})?;

Self::deposit_event(Event::<T>::BeneficiaryUpdated(staker, contract_id, new_beneficiary));
Ok(().into())
}

/// Used to force set `ContractEraStake` storage values.
/// The purpose of this call is only for fixing one of the issues detected with dapps-staking.
///
Expand Down
112 changes: 73 additions & 39 deletions frame/dapps-staking/src/testing_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ pub(crate) struct MemorySnapshot {
staker_info: StakerInfo<Balance>,
contract_info: ContractStakeInfo<Balance>,
free_balance: Balance,
beneficiary: Option<AccountId>,
beneficiary_free_balance: Balance,
ledger: AccountLedger<Balance>,
}

Expand All @@ -27,6 +29,10 @@ impl MemorySnapshot {
contract_info: DappsStaking::contract_stake_info(contract_id, era).unwrap_or_default(),
ledger: DappsStaking::ledger(&account),
free_balance: <TestRuntime as Config>::Currency::free_balance(&account),
beneficiary: RewardsBeneficiary::<TestRuntime>::get(&account, &contract_id),
beneficiary_free_balance: RewardsBeneficiary::<TestRuntime>::get(&account, &contract_id)
.map(|acc| <TestRuntime as Config>::Currency::free_balance(&acc))
.unwrap_or_default(),
}
}

Expand All @@ -40,6 +46,8 @@ impl MemorySnapshot {
contract_info: DappsStaking::contract_stake_info(contract_id, era).unwrap_or_default(),
ledger: Default::default(),
free_balance: Default::default(),
beneficiary: None,
beneficiary_free_balance: Default::default(),
}
}
}
Expand Down Expand Up @@ -528,7 +536,7 @@ pub(crate) fn assert_claim_staker(claimer: AccountId, contract_id: &MockSmartCon
);

// check for stake event if restaking is performed
if DappsStaking::should_restake_reward(
if init_state_current_era.beneficiary.is_none() && DappsStaking::should_restake_reward(
init_state_current_era.ledger.reward_destination,
init_state_current_era.dapp_info.state,
init_state_current_era.staker_info.latest_staked_value(),
Expand All @@ -544,8 +552,9 @@ pub(crate) fn assert_claim_staker(claimer: AccountId, contract_id: &MockSmartCon
}

// last event should be Reward, regardless of restaking
let reward_account = init_state_current_era.beneficiary.unwrap_or(claimer);
System::assert_last_event(mock::Event::DappsStaking(Event::Reward(
claimer,
reward_account,
contract_id.clone(),
claim_era,
calculated_reward,
Expand Down Expand Up @@ -582,46 +591,71 @@ fn assert_restake_reward(
final_state_current_era: &MemorySnapshot,
reward: Balance,
) {
if DappsStaking::should_restake_reward(
let beneficiary = final_state_current_era.beneficiary;
let restake = DappsStaking::should_restake_reward(
init_state_current_era.ledger.reward_destination,
init_state_current_era.dapp_info.state,
init_state_current_era.staker_info.latest_staked_value(),
) {
// staked values should increase
assert_eq!(
init_state_current_era.staker_info.latest_staked_value() + reward,
final_state_current_era.staker_info.latest_staked_value()
);
assert_eq!(
init_state_current_era.era_info.staked + reward,
final_state_current_era.era_info.staked
);
assert_eq!(
init_state_current_era.era_info.locked + reward,
final_state_current_era.era_info.locked
);
assert_eq!(
init_state_current_era.contract_info.total + reward,
final_state_current_era.contract_info.total
);
} else {
// staked values should remain the same, and free balance increase
assert_eq!(
init_state_current_era.free_balance + reward,
final_state_current_era.free_balance
);
assert_eq!(
init_state_current_era.era_info.staked,
final_state_current_era.era_info.staked
);
assert_eq!(
init_state_current_era.era_info.locked,
final_state_current_era.era_info.locked
);
assert_eq!(
init_state_current_era.contract_info,
final_state_current_era.contract_info
);
);

match (beneficiary, restake) {
(Some(_), _) => {
// staked values should remain the same, and free balance increase
assert_eq!(
init_state_current_era.beneficiary_free_balance + reward,
final_state_current_era.beneficiary_free_balance
);
assert_eq!(
init_state_current_era.era_info.staked,
final_state_current_era.era_info.staked
);
assert_eq!(
init_state_current_era.era_info.locked,
final_state_current_era.era_info.locked
);
assert_eq!(
init_state_current_era.contract_info,
final_state_current_era.contract_info
);
},
(None, true) => {
// staked values should increase
assert_eq!(
init_state_current_era.staker_info.latest_staked_value() + reward,
final_state_current_era.staker_info.latest_staked_value()
);
assert_eq!(
init_state_current_era.era_info.staked + reward,
final_state_current_era.era_info.staked
);
assert_eq!(
init_state_current_era.era_info.locked + reward,
final_state_current_era.era_info.locked
);
assert_eq!(
init_state_current_era.contract_info.total + reward,
final_state_current_era.contract_info.total
);
},
(None, false) => {
// staked values should remain the same, and the staker's free balance increase
assert_eq!(
init_state_current_era.free_balance + reward,
final_state_current_era.free_balance
);
assert_eq!(
init_state_current_era.era_info.staked,
final_state_current_era.era_info.staked
);
assert_eq!(
init_state_current_era.era_info.locked,
final_state_current_era.era_info.locked
);
assert_eq!(
init_state_current_era.contract_info,
final_state_current_era.contract_info
);
}
}
}

Expand Down
Loading