Skip to content

Commit

Permalink
fix lottery withdraw issue (#1331)
Browse files Browse the repository at this point in the history 
* fix lottery withdraw issue

Signed-off-by: zqhxuyuan <zqhxuyuan@gmail.com>

* fmt

Signed-off-by: zqhxuyuan <zqhxuyuan@gmail.com>

* more test

Signed-off-by: zqhxuyuan <zqhxuyuan@gmail.com>

* fmt

Signed-off-by: zqhxuyuan <zqhxuyuan@gmail.com>

---------

Signed-off-by: zqhxuyuan <zqhxuyuan@gmail.com>
Co-authored-by: ferrell-code <charlie@manta.network>
  • Loading branch information
@zqhxuyuan @ferrell-code
zqhxuyuan and ferrell-code authored May 9, 2024
1 parent edf4f97 commit 5e9f453
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 19 deletions.
2 changes: 2 additions & 0 deletions pallets/pallet-lottery/src/staking/deposit_strategies.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,11 @@ pub(super) fn reactivate_bottom_collators<T: Config>(
.expect("is active collator, therefore it has collator info. qed");

// If collator not exist in delegatorState(PotAccount).delegations, ignore
// delegations has other active collator to stake.
if let Some(state) =
pallet_parachain_staking::Pallet::<T>::delegator_state(crate::Pallet::<T>::account_id())
{
log::debug!("delegator PotAccount has state.");
let mut is_kick = true;
for x in &state.delegations.0 {
if x.owner == collator {
Expand Down
21 changes: 19 additions & 2 deletions pallets/pallet-lottery/src/staking/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,9 +209,26 @@ impl<T: Config> Pallet<T> {
if withdrawal_eligible_collators.is_empty() {
return vec![];
}
let mut eligible_collators: Vec<_> = collators_we_are_unstaking_from;
if let Some(state) =
pallet_parachain_staking::Pallet::<T>::delegator_state(crate::Pallet::<T>::account_id())
{
let owners: Vec<_> = state
.delegations
.0
.iter()
.cloned()
.map(|uc| uc.owner)
.collect();
eligible_collators = withdrawal_eligible_collators
.iter()
.filter(|account| owners.contains(account))
.cloned()
.collect();
}
// first concern: If there are inactive collators we are staked with, prefer these
let (mut collators, balance_unstaked) = withdraw_strategies::unstake_inactive_collators::<T>(
&withdrawal_eligible_collators,
&eligible_collators,
remaining_balance,
);
withdrawals.append(&mut collators);
Expand All @@ -222,7 +239,7 @@ impl<T: Config> Pallet<T> {
// If we have balance to withdraw left over, we have to unstake some healthy collator.
// Unstake starting from the highest overallocated collator ( since that yields the lowest APY ) going down until request is satisfied
let (mut collators, balance_unstaked) = withdraw_strategies::unstake_least_apy_collators::<T>(
&withdrawal_eligible_collators
&eligible_collators
.into_iter()
.filter(|collator| !withdrawals.contains(collator))
.collect(),
Expand Down
73 changes: 56 additions & 17 deletions pallets/pallet-lottery/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1062,7 +1062,7 @@ fn delegator_kicked_not_in_state_cannot_deposit() {
assert_ok!(Lottery::deposit(Origin::signed(ALICE), 150_000_000 * UNIT));
assert_eq!(crate::StakedCollators::<Test>::iter().count(), 1);

// Check PotAmount state, exist!
// Check PotAccount state, exist!
let pot_state1 = ParachainStaking::delegator_state(crate::Pallet::<Test>::account_id()).expect("just delegated => exists");
assert_eq!(pot_state1.delegations.0[0].owner, BOB);
assert_eq!(pot_state1.delegations.0[0].amount, 150_000_000 * UNIT);
Expand All @@ -1089,12 +1089,12 @@ fn delegator_kicked_not_in_state_cannot_deposit() {
assert_eq!(charlie_state.delegations.0[0].owner, BOB);
assert_eq!(charlie_state.delegations.0[0].amount, 160_000_000 * UNIT);

// Check PotAmount state, not exist!
// Check PotAccount state, not exist!
let _pot_state2 = ParachainStaking::delegator_state(crate::Pallet::<Test>::account_id());
assert!(_pot_state2.is_none());

// Delegator: Pot Account is not exist on DelegatorState.
// Now Pot account is kicked, try to deposit, failed!
// Delegator: PotAccount is not exist on DelegatorState.
// Now PotAccount is kicked, try to deposit, failed!
// In this case, we should delegate instead bond more.
assert_noop!(
Lottery::deposit(Origin::signed(ALICE), 150_000_000 * UNIT),
Expand Down Expand Up @@ -1195,7 +1195,7 @@ fn delegator_kicked_when_reactivate_bottom_should_ignored() {
assert_ok!(Lottery::deposit(Origin::signed(ALICE), 51_000_000 * UNIT));
assert_eq!(crate::StakedCollators::<Test>::iter().count(), 2);

// Pot Account delegate to two collators.
// PotAccount delegate to two collators.
let pot_state1 = ParachainStaking::delegator_state(crate::Pallet::<Test>::account_id())
.expect("just delegated => exists");
assert_eq!(pot_state1.delegations.len(), 2);
Expand Down Expand Up @@ -1237,8 +1237,8 @@ fn delegator_kicked_when_reactivate_bottom_should_ignored() {
9,
0
));
// Pot Account now is kicked from collator BOB's delegator list.
// But Pot Account is still on the delegator list of CHARLIE.
// PotAccount now is kicked from collator BOB's delegator list.
// But PotAccount is still on the delegator list of CHARLIE.
let pot_state2 = ParachainStaking::delegator_state(crate::Pallet::<Test>::account_id())
.expect("just delegated => exists");
assert_eq!(pot_state2.delegations.len(), 1);
Expand Down Expand Up @@ -1298,7 +1298,6 @@ fn delegator_kicked_when_reactivate_bottom_should_ignored() {

#[test]
fn delegator_unstaking_then_kicked_should_ignored() {
let reserve = 10_000 * UNIT;
let balance = 500_000_000 * UNIT;
ExtBuilder::default()
.with_balances(vec![
Expand Down Expand Up @@ -1326,7 +1325,7 @@ fn delegator_unstaking_then_kicked_should_ignored() {
(DELEGATOR3, CHARLIE, 60_000_000 * UNIT),
(DELEGATOR4, CHARLIE, 50_000_000 * UNIT),
])
.with_funded_lottery_account(reserve)
.with_funded_lottery_account(balance)
.build()
.execute_with(|| {
assert_ok!(Lottery::deposit(Origin::signed(ALICE), 51_000_000 * UNIT));
Expand All @@ -1343,7 +1342,7 @@ fn delegator_unstaking_then_kicked_should_ignored() {
assert_eq!(crate::StakedCollators::<Test>::iter().count(), 2);
assert_eq!(crate::UnstakingCollators::<Test>::get().len(), 1);

// Pot Account delegate to two collators.
// PotAccount delegate to two collators.
let pot_state1 = ParachainStaking::delegator_state(crate::Pallet::<Test>::account_id())
.expect("just delegated => exists");
assert_eq!(pot_state1.delegations.len(), 2);
Expand Down Expand Up @@ -1392,6 +1391,7 @@ fn delegator_unstaking_then_kicked_should_ignored() {
assert_eq!(pot_state2.delegations.0[0].amount, 51_000_000 * UNIT);

assert_eq!(crate::StakedCollators::<Test>::iter().count(), 2);
assert_eq!(crate::UnstakingCollators::<Test>::get().len(), 1);

let collator1_state = ParachainStaking::candidate_info(BOB).unwrap();
let collator2_state = ParachainStaking::candidate_info(CHARLIE).unwrap();
Expand All @@ -1412,29 +1412,68 @@ fn delegator_unstaking_then_kicked_should_ignored() {
50_000_000 * UNIT
);

// Check PotAmount state, exist!
// Check PotAccount state, exist!
let _pot_state2 =
ParachainStaking::delegator_state(crate::Pallet::<Test>::account_id())
.expect("just delegated => exists");
assert_eq!(_pot_state2.delegations.len(), 1);
assert_eq!(_pot_state2.delegations.0[0].owner, CHARLIE);
assert_eq!(_pot_state2.delegations.0[0].amount, 51_000_000 * UNIT);

// Staked to BOB was already kicked Pot Account.
// Staked to BOB was already kicked PotAccount.
// And staked to CHARLIE is unstaking, now there're no collators to deposit.
// If we choose random one which is BOB, then delegator_bond_more has error.
assert_noop!(
Lottery::deposit(Origin::signed(ALICE), 53_000_000 * UNIT),
pallet_parachain_staking::Error::<Test>::DelegationDNE
);

// Check PotAmount state, exist!
// Check PotAccount state, exist!
let _pot_state3 =
ParachainStaking::delegator_state(crate::Pallet::<Test>::account_id())
.expect("just delegated => exists");
assert_eq!(_pot_state3.delegations.len(), 1);
assert_eq!(_pot_state3.delegations.0[0].owner, CHARLIE);
assert_eq!(_pot_state3.delegations.0[0].amount, 51_000_000 * UNIT);

assert_eq!(crate::StakedCollators::<Test>::iter().count(), 2);
assert_eq!(crate::UnstakingCollators::<Test>::get().len(), 1);

// All collator not eligible, withdraw failed.
assert_noop!(
Lottery::request_withdraw(Origin::signed(ALICE), 51_000_000 * UNIT),
sp_runtime::DispatchError::Other(
"FATAL: Didn't unstake the full requested balance (or more)"
)
);

// draw lottery to make unstaking collator removed.
pallet_parachain_staking::AwardedPts::<Test>::insert(2, BOB, 20);
pallet_parachain_staking::AwardedPts::<Test>::insert(2, CHARLIE, 20);
roll_to_round_begin(3);
assert_ok!(Lottery::process_matured_withdrawals(RawOrigin::Root.into()));

assert_eq!(crate::StakedCollators::<Test>::iter().count(), 1);
assert_eq!(crate::UnstakingCollators::<Test>::get().len(), 0);
assert_eq!(crate::StakedCollators::<Test>::get(BOB), 51_000_000 * UNIT);

// DelegatorState of PotAccount is empty
let _pot_state4 =
ParachainStaking::delegator_state(crate::Pallet::<Test>::account_id());
assert!(_pot_state4.is_none());

// The only one staked collator BOB is not exist in parachain staking DelegatorState.
assert_noop!(
Lottery::request_withdraw(Origin::signed(ALICE), 51_000_000 * UNIT),
sp_runtime::DispatchError::Other(
"FATAL: Didn't unstake the full requested balance (or more)"
)
);

assert_noop!(
Lottery::deposit(Origin::signed(ALICE), 53_000_000 * UNIT),
pallet_parachain_staking::Error::<Test>::DelegatorDNE
);
});
}

Expand Down Expand Up @@ -1485,7 +1524,7 @@ fn delegator_unstaking_kicked_same_collator_should_ignored() {
assert_eq!(crate::StakedCollators::<Test>::iter().count(), 2);
assert_eq!(crate::UnstakingCollators::<Test>::get().len(), 1);

// Pot Account delegate to two collators.
// PotAccount delegate to two collators.
let pot_state1 = ParachainStaking::delegator_state(crate::Pallet::<Test>::account_id())
.expect("just delegated => exists");
assert_eq!(pot_state1.delegations.len(), 2);
Expand Down Expand Up @@ -1539,7 +1578,7 @@ fn delegator_unstaking_kicked_same_collator_should_ignored() {
let pot_state2 = ParachainStaking::delegator_state(crate::Pallet::<Test>::account_id())
.expect("just delegated => exists");
assert_eq!(pot_state2.delegations.len(), 1);
// We can ensure that Pot Account now only staked to Charlie, but can't ensure the staked amount.
// We can ensure that PotAccount now only staked to Charlie, but can't ensure the staked amount.
assert_eq!(pot_state2.delegations.0[0].owner, CHARLIE);
if first_deposit_staked_to_bob {
assert_eq!(pot_state2.delegations.0[0].amount, 51_000_000 * UNIT);
Expand Down Expand Up @@ -1589,7 +1628,7 @@ fn delegator_unstaking_kicked_same_collator_should_ignored() {
);
}

// Check PotAmount state, exist!
// Check PotAccount state, exist!
let _pot_state2 =
ParachainStaking::delegator_state(crate::Pallet::<Test>::account_id())
.expect("just delegated => exists");
Expand All @@ -1610,7 +1649,7 @@ fn delegator_unstaking_kicked_same_collator_should_ignored() {
);
}

// Check PotAmount state, exist!
// Check PotAccount state, exist!
let _pot_state3 =
ParachainStaking::delegator_state(crate::Pallet::<Test>::account_id())
.expect("just delegated => exists");
Expand Down

0 comments on commit 5e9f453

Please sign in to comment.