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

Domains staking: Epoch transition and Pending unlocks #1657

Merged
merged 13 commits into from
Jul 18, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
19 changes: 16 additions & 3 deletions crates/pallet-domains/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ mod pallet {
use crate::{calculate_tx_range, BalanceOf, FreezeIdentifier, NominatorId};
use codec::FullCodec;
use frame_support::pallet_prelude::{StorageMap, *};
use frame_support::traits::fungible::{InspectFreeze, MutateFreeze};
use frame_support::traits::fungible::{InspectFreeze, Mutate, MutateFreeze};
use frame_support::weights::Weight;
use frame_support::{Identity, PalletError};
use frame_system::pallet_prelude::*;
Expand Down Expand Up @@ -136,7 +136,9 @@ mod pallet {
type DomainRuntimeUpgradeDelay: Get<Self::BlockNumber>;

/// Currency type used by the domains for staking and other currency related stuff.
type Currency: MutateFreeze<Self::AccountId> + InspectFreeze<Self::AccountId>;
type Currency: Mutate<Self::AccountId>
+ MutateFreeze<Self::AccountId>
+ InspectFreeze<Self::AccountId>;

/// Type representing the shares in the staking protocol.
type Share: Parameter
Expand Down Expand Up @@ -275,7 +277,18 @@ mod pallet {
/// Stored here temporarily until domain epoch is complete.
#[pallet::storage]
pub(super) type PendingOperatorDeregistrations<T: Config> =
StorageValue<_, Vec<OperatorId>, OptionQuery>;
StorageMap<_, Identity, DomainId, Vec<OperatorId>, OptionQuery>;

#[pallet::storage]
pub(super) type PendingOperatorUnlocks<T: Config> = StorageDoubleMap<
_,
Identity,
DomainId,
Identity,
T::DomainNumber,
Vec<OperatorId>,
OptionQuery,
>;

/// Stores the next domain id.
#[pallet::storage]
Expand Down
4 changes: 2 additions & 2 deletions crates/pallet-domains/src/staking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ pub(crate) fn do_deregister_operator<T: Config>(
},
)?;

PendingOperatorDeregistrations::<T>::append(operator_id);
PendingOperatorDeregistrations::<T>::append(operator.current_domain_id, operator_id);

Ok(())
})
Expand Down Expand Up @@ -667,7 +667,7 @@ mod tests {
let operator = Operators::<Test>::get(operator_id).unwrap();
assert!(operator.is_frozen);

assert!(PendingOperatorDeregistrations::<Test>::get()
assert!(PendingOperatorDeregistrations::<Test>::get(domain_id)
.unwrap()
.contains(&operator_id));

Expand Down
236 changes: 216 additions & 20 deletions crates/pallet-domains/src/staking_epoch.rs
Original file line number Diff line number Diff line change
@@ -1,16 +1,28 @@
//! Staking epoch transition for domain
// TODO: remove once pieces are connected.
#![allow(dead_code)]

use crate::pallet::{DomainStakingSummary, Operators, PendingOperatorSwitches};
use crate::Config;
use crate::pallet::{
DomainStakingSummary, Nominators, Operators, PendingOperatorSwitches, PendingOperatorUnlocks,
};
use crate::{Config, FreezeIdentifier, NominatorId};
use frame_support::log::error;
use frame_support::traits::fungible::{InspectFreeze, Mutate, MutateFreeze};
use sp_domains::{DomainId, OperatorId};
use sp_runtime::traits::{CheckedAdd, CheckedSub, Zero};
use sp_runtime::Perbill;
use sp_std::vec::Vec;

#[derive(Debug)]
enum Error {
MissingOperator,
OperatorFrozen,
MissingDomainStakeSummary,
BalanceOverflow,
BalanceUnderflow,
ShareUnderflow,
RemoveLock,
MintBalance,
}

/// Add all the switched operators to new domain.
Expand Down Expand Up @@ -45,21 +57,128 @@ fn switch_operator<T: Config>(operator_id: OperatorId) -> Result<(), Error> {
})
}

pub(crate) fn do_unlock_operators<T: Config>(
domain_id: DomainId,
domain_block_number: T::DomainNumber,
) {
if let Some(operators) = PendingOperatorUnlocks::<T>::take(domain_id, domain_block_number) {
for operator_id in operators {
if let Err(err) = unlock_operator::<T>(operator_id) {
error!("Failed to unlock operator pool[{operator_id:?}]: {err:?}",)
}
}
}
}

fn unlock_operator<T: Config>(operator_id: OperatorId) -> Result<(), Error> {
Operators::<T>::try_mutate(operator_id, |maybe_operator| {
let operator = maybe_operator.as_mut().ok_or(Error::MissingOperator)?;

let mut total_shares = operator.total_shares;
let mut total_stake = operator
Comment on lines +159 to +160
Copy link
Member

Choose a reason for hiding this comment

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

We may not need to update these 2 variables in every iteration if we introduce a remaining_stake and update it instead which is also more straightforward IMHO, non-blocker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the total_stake here will act as remaining_stake at the end. As for the total_shares, not sure what you mean here. We would still need track the remaining shares to calculate the nominator's stake. Am I missing something here ?

Copy link
Member

Choose a reason for hiding this comment

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

What I mean was something like this:

        let total_shares = operator.total_shares;
        let total_stake = operator
            .current_total_stake
            .checked_add(&operator.current_epoch_rewards)
            .ok_or(TransitionError::BalanceOverflow)?;
        let mut remaining_stake = total_stake;

        let freeze_identifier = T::FreezeIdentifier::staking_freeze_id(operator_id);

        Nominators::<T>::drain_prefix(operator_id).try_for_each(|(nominator_id, nominator)| {
            let nominator_share = Perbill::from_rational(nominator.shares, total_shares);
            let nominator_staked_amount = nominator_share.mul_floor(total_stake);

            let locked_amount = T::Currency::balance_frozen(&freeze_identifier, &nominator_id);
            let amount_to_mint = nominator_staked_amount
                .checked_sub(&locked_amount)
                .unwrap_or(Zero::zero());

            // remove the lock and mint any gains
            ...

            // update pool's remaining stake
            remaining_stake = remaining_stake
                .checked_sub(&nominator_staked_amount)
                .ok_or(TransitionError::BalanceUnderflow)?;

            // remove any pending deposits for this nominator and then remove the lock
            ...

            Ok(())
        })?;

Currently, we updating total_stake and total_share and using them to calculate nominator_staked_amount at the same time, it is not straightforward to see the calculation is correct IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is incorrect. You suggestion does not account for shares that are burned. If you don't update the shares, then nominator_share will be always be incorrect.

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by burned shares? If there are 10 total shares and 2 nominators with 3 and 7 shares respectively, the nominator_share of them should be 3/10 and 7/10, right?

Copy link
Member

Choose a reason for hiding this comment

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

Rounding loss can be captured by remaining_stake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but it is not captured when calculating the nominator share. If that is possible, please give me an example here

Copy link
Member

Choose a reason for hiding this comment

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

The remaining_stake is initialized to total_stake said 100, for every nominator we deduct nominator_staked_amount (assume rounding loss is 0.1, the value will be 19.9, 29.9, and 49.9 respectively) from remaining_stake, thus after all deduction the remaining_stake left 0.3 which is the rounding loss.

Copy link
Contributor Author

@vedhavyas vedhavyas Jul 18, 2023

Choose a reason for hiding this comment

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

Yes that is what we get with the current implementation. But you are not calculating share_percentage of each nominator after every deposit Withdraw. You are assuming the share_percentage is not changing which is not true. Here we are calculating nominator's share_percentage after every withdrawal so that (total_shares_left/total_stake_remaining) are upto date instead of pre calculating them earlier to account for those small increments of percentage due to a rounding down in the previous withdrawal

Copy link
Member

Choose a reason for hiding this comment

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

You are assuming the share_percentage is not changing which is not true.

The share_percentage is not (and should) changed even in current impl, the only calculation of share is total_shares = total_shares.checked_sub((&nominator.shares)) which does not bring any rounding loss.

But I do see the difference now, in the current impl nominators can get a cut of the rounding loss. In the above example, after the first nominator is processed, the total_stake becomes 80.1 and the second nominator can withdraw 80.1 * 3/8 while my proposal is 100 * 3/10 and leave all rounding loss to treasury.

.current_total_stake
.checked_add(&operator.current_epoch_rewards)
.ok_or(Error::BalanceOverflow)?;

let nominator_ids =
Nominators::<T>::iter_key_prefix(operator_id).collect::<Vec<NominatorId<T>>>();

let freeze_identifier = T::FreezeIdentifier::staking_freeze_id(operator_id);
for nominator_id in nominator_ids {
// defensively read the the Nominator stake and then clear it once processed
if let Some(nominator) = Nominators::<T>::get(operator_id, nominator_id.clone()) {
let nominator_share = Perbill::from_rational(nominator.shares, total_shares);
let nominator_staked_amount = nominator_share * total_stake;

let locked_amount = T::Currency::balance_frozen(&freeze_identifier, &nominator_id);
let amount_to_mint = nominator_staked_amount
.checked_sub(&locked_amount)
.unwrap_or(Zero::zero());

// remove the lock and mint any gains
T::Currency::thaw(&freeze_identifier, &nominator_id)
.map_err(|_| Error::RemoveLock)?;
T::Currency::mint_into(&nominator_id, amount_to_mint)
.map_err(|_| Error::MintBalance)?;

// update pool's remaining shares and stake
total_shares = total_shares
.checked_sub(&nominator.shares)
.ok_or(Error::ShareUnderflow)?;
total_stake = total_stake
.checked_sub(&nominator_staked_amount)
.ok_or(Error::BalanceUnderflow)?;

// remove nominator
Nominators::<T>::remove(operator_id, nominator_id)
}
}

// TODO: transfer any remaining amount to treasury

// reset operator pool
operator.total_shares = Zero::zero();
vedhavyas marked this conversation as resolved.
Show resolved Hide resolved
operator.current_total_stake = Zero::zero();
operator.current_epoch_rewards = Zero::zero();

Ok(())
})
}

#[cfg(test)]
mod tests {
use crate::pallet::{
DomainStakingSummary, OperatorIdOwner, Operators, PendingOperatorSwitches,
DomainStakingSummary, Nominators, OperatorIdOwner, Operators, PendingOperatorSwitches,
PendingOperatorUnlocks,
};
use crate::staking::{Operator, StakingSummary};
use crate::staking_epoch::do_finalize_switch_operator_domain;
use crate::staking::{Nominator, Operator, StakingSummary};
use crate::staking_epoch::{do_finalize_switch_operator_domain, do_unlock_operators};
use crate::tests::{new_test_ext, Test};
use crate::{BalanceOf, Config, FreezeIdentifier as FreezeIdentifierT, NominatorId};
use frame_support::assert_ok;
use frame_support::traits::fungible::MutateFreeze;
use frame_support::traits::Currency;
use sp_core::{Pair, U256};
use sp_domains::{DomainId, OperatorPair};
use sp_domains::{DomainId, OperatorId, OperatorPair};
use sp_runtime::traits::Zero;
use subspace_runtime_primitives::SSC;

type Balances = pallet_balances::Pallet<Test>;
type Domains = crate::Pallet<Test>;
type ShareOf<T> = <T as Config>::Share;

struct RequiredStateParams {
domain_id: DomainId,
total_domain_stake: BalanceOf<Test>,
current_operators: Vec<OperatorId>,
operator_id: OperatorId,
operator_account: <Test as frame_system::Config>::AccountId,
operator: Operator<BalanceOf<Test>, ShareOf<Test>>,
}

fn create_required_state(params: RequiredStateParams) {
let RequiredStateParams {
domain_id,
total_domain_stake,
current_operators,
operator_id,
operator_account,
operator,
} = params;

DomainStakingSummary::<Test>::insert(
domain_id,
StakingSummary {
current_epoch_index: 0,
current_total_stake: total_domain_stake,
current_operators,
next_operators: vec![],
},
);

OperatorIdOwner::<Test>::insert(operator_id, operator_account);
Operators::<Test>::insert(operator_id, operator);
}

#[test]
fn finalize_operator_domain_switch() {
Expand All @@ -71,20 +190,13 @@ mod tests {

let mut ext = new_test_ext();
ext.execute_with(|| {
DomainStakingSummary::<Test>::insert(
new_domain_id,
StakingSummary {
current_epoch_index: 0,
current_total_stake: 0,
current_operators: vec![],
next_operators: vec![],
},
);

OperatorIdOwner::<Test>::insert(operator_id, operator_account);
Operators::<Test>::insert(
create_required_state(RequiredStateParams {
domain_id: new_domain_id,
total_domain_stake: 0,
current_operators: vec![],
operator_id,
Operator {
operator_account,
operator: Operator {
signing_key: pair.public(),
current_domain_id: old_domain_id,
next_domain_id: new_domain_id,
Expand All @@ -95,7 +207,7 @@ mod tests {
total_shares: Zero::zero(),
is_frozen: false,
},
);
});

PendingOperatorSwitches::<Test>::append(old_domain_id, operator_id);
do_finalize_switch_operator_domain::<Test>(old_domain_id);
Expand All @@ -109,4 +221,88 @@ mod tests {
assert!(domain_stake_summary.next_operators.contains(&operator_id));
});
}

fn unlock_operator(
nominators: Vec<(NominatorId<Test>, BalanceOf<Test>)>,
rewards: BalanceOf<Test>,
) {
let domain_id = DomainId::new(0);
let operator_account = 1;
let operator_id = 1;
let pair = OperatorPair::from_seed(&U256::from(0u32).into());
let minimum_free_balance = 10 * SSC;

let mut ext = new_test_ext();
ext.execute_with(|| {
let mut total_stake = Zero::zero();
let mut total_shares = Zero::zero();
let freeze_id = crate::tests::FreezeIdentifier::staking_freeze_id(operator_id);
for nominator in &nominators {
total_stake += nominator.1;
total_shares += nominator.1;
Balances::make_free_balance_be(&nominator.0, nominator.1 + minimum_free_balance);
assert_ok!(Balances::set_freeze(&freeze_id, &nominator.0, nominator.1));
assert_eq!(Balances::usable_balance(nominator.0), minimum_free_balance);
Nominators::<Test>::insert(
operator_id,
nominator.0,
Nominator {
shares: nominator.1,
},
)
}
create_required_state(RequiredStateParams {
domain_id,
total_domain_stake: total_stake,
current_operators: vec![],
operator_id,
operator_account,
operator: Operator {
signing_key: pair.public(),
current_domain_id: domain_id,
next_domain_id: domain_id,
minimum_nominator_stake: 10 * SSC,
nomination_tax: Default::default(),
current_total_stake: total_stake,
current_epoch_rewards: rewards,
total_shares,
is_frozen: true,
},
});

let domain_block_number = 100;
PendingOperatorUnlocks::<Test>::append(domain_id, domain_block_number, operator_id);
do_unlock_operators::<Test>(domain_id, domain_block_number);

for nominator in &nominators {
let mut required_minimum_free_balance = minimum_free_balance + nominator.1;
if rewards.is_zero() {
// subtracted 1 SSC to account for any rounding errors if there are not rewards
required_minimum_free_balance -= SSC;
}
assert_eq!(Nominators::<Test>::get(operator_id, nominator.0), None);
assert!(Balances::usable_balance(nominator.0) > required_minimum_free_balance);
}

let operator = Operators::<Test>::get(operator_id).unwrap();
assert!(operator.is_frozen);
assert_eq!(operator.total_shares, Zero::zero());
assert_eq!(operator.current_epoch_rewards, Zero::zero());
assert_eq!(operator.current_total_stake, Zero::zero());
assert_eq!(
PendingOperatorUnlocks::<Test>::get(domain_id, domain_block_number),
None
)
});
}

#[test]
fn unlock_operator_with_no_rewards() {
unlock_operator(vec![(1, 150 * SSC), (2, 50 * SSC), (3, 10 * SSC)], 0);
}

#[test]
fn unlock_operator_with_rewards() {
unlock_operator(vec![(1, 150 * SSC), (2, 50 * SSC), (3, 10 * SSC)], 20 * SSC);
}
}