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

Fix account call for freeze/lock inconsistency #1276

Merged
merged 6 commits into from
Jun 24, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
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
101 changes: 79 additions & 22 deletions pallets/dapp-staking-v3/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,8 @@ pub mod pallet {
NoExpiredEntries,
/// Force call is not allowed in production.
ForceNotAllowed,
/// Account doesn't have the freeze inconsistency
InvalidAccount, // TODO: can be removed after call `fix_account` is removed
Copy link
Member

Choose a reason for hiding this comment

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

nit: can be better named, AccountNotInconsistent ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there's the need for this. If account is inconsistent then there's noop tx paying just fee

Copy link
Member Author

Choose a reason for hiding this comment

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

@ashutoshvarma sure. Believe it or not, that was the first name I choose, but decided to go with a more generic one later xD.

@ermalkaleci
I added it to make testing more clear.
It will be removed later anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough

}

/// General information about dApp staking protocol state.
Expand Down Expand Up @@ -931,28 +933,7 @@ pub mod pallet {
Self::ensure_pallet_enabled()?;
let account = ensure_signed(origin)?;

let mut ledger = Ledger::<T>::get(&account);

let current_block = frame_system::Pallet::<T>::block_number();
let amount = ledger.claim_unlocked(current_block.saturated_into());
ensure!(amount > Zero::zero(), Error::<T>::NoUnlockedChunksToClaim);

// In case it's full unlock, account is exiting dApp staking, ensure all storage is cleaned up.
let removed_entries = if ledger.is_empty() {
let _ = StakerInfo::<T>::clear_prefix(&account, ledger.contract_stake_count, None);
ledger.contract_stake_count
} else {
0
};

Self::update_ledger(&account, ledger)?;
CurrentEraInfo::<T>::mutate(|era_info| {
era_info.unlocking_removed(amount);
});

Self::deposit_event(Event::<T>::ClaimedUnlocked { account, amount });

Ok(Some(T::WeightInfo::claim_unlocked(removed_entries)).into())
Self::internal_claim_unlocked(account)
}

#[pallet::call_index(10)]
Expand Down Expand Up @@ -1603,6 +1584,57 @@ pub mod pallet {

Ok(())
}

/// A call used to fix accounts with inconsistent state, where frozen balance is actually higher than what's available.
///
/// The approach is as simple as possible:
/// 1. Caller provides an account to fix.
/// 2. If account is eligible for the fix, all unlocking chunks are modified to be claimable immediately.
/// 3. The `claim_unlocked` call is executed using the provided account as the origin.
/// 4. All states are updated accordingly, and the account is no longer in an inconsistent state.
///
/// The benchmarked weight of the `claim_unlocked` call is used as a base, and additional overestimated weight is added.
/// Call doesn't touch any storage items that aren't already touched by the `claim_unlocked` call, hence the simplified approach.
#[pallet::call_index(100)]
#[pallet::weight(T::WeightInfo::claim_unlocked(T::MaxNumberOfStakedContracts::get()).saturating_add(Weight::from_parts(1_000_000_000, 0)))]
ermalkaleci marked this conversation as resolved.
Show resolved Hide resolved
pub fn fix_account(
origin: OriginFor<T>,
account: T::AccountId,
) -> DispatchResultWithPostInfo {
Self::ensure_pallet_enabled()?;
ensure_signed(origin)?;

let mut ledger = Ledger::<T>::get(&account);
let locked_amount_ledger = ledger.total_locked_amount();
let total_balance = T::Currency::total_balance(&account);

if locked_amount_ledger > total_balance {
// 1. Modify all unlocking chunks so they can be unlocked immediately.
let current_block: BlockNumber =
frame_system::Pallet::<T>::block_number().saturated_into();
ledger
.unlocking
.iter_mut()
.for_each(|chunk| chunk.unlock_block = current_block);
Ledger::<T>::insert(&account, ledger);

// 2. Execute the unlock call, clearing all of the unlocking chunks.
let result = Self::internal_claim_unlocked(account);

// 3. Adjust consumed weight to consume the max possible weight (as defined in the weight macro).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// 3. Adjust consumed weight to consume the max possible weight (as defined in the weight macro).
// 3. Adjust consumed weight to consume the max possible weight (as defined in `T::WeightInfo::claim_unlocked()`).

nit: internal_claim_unlocked() has no weights macro, but use weights from claim_unlocked().
I was confused at first

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean the macro above the call: T::WeightInfo::claim_unlocked() + const.

match result {
Dinonard marked this conversation as resolved.
Show resolved Hide resolved
Ok(mut info) => {
info.pays_fee = Pays::No;
info.actual_weight = None;
ermalkaleci marked this conversation as resolved.
Show resolved Hide resolved
}
Err(mut info) => info.post_info.actual_weight = None,
}
result
} else {
// The above logic is designed for a specific scenario and cannot be used otherwise.
Err(Error::<T>::InvalidAccount.into())
ermalkaleci marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

impl<T: Config> Pallet<T> {
Expand Down Expand Up @@ -2125,5 +2157,30 @@ pub mod pallet {
// It could end up being less than this weight, but this won't occur often enough to be important.
T::WeightInfo::on_idle_cleanup()
}

fn internal_claim_unlocked(account: T::AccountId) -> DispatchResultWithPostInfo {
let mut ledger = Ledger::<T>::get(&account);

let current_block = frame_system::Pallet::<T>::block_number();
let amount = ledger.claim_unlocked(current_block.saturated_into());
ensure!(amount > Zero::zero(), Error::<T>::NoUnlockedChunksToClaim);

// In case it's full unlock, account is exiting dApp staking, ensure all storage is cleaned up.
let removed_entries = if ledger.is_empty() {
let _ = StakerInfo::<T>::clear_prefix(&account, ledger.contract_stake_count, None);
ledger.contract_stake_count
} else {
0
};

Self::update_ledger(&account, ledger)?;
CurrentEraInfo::<T>::mutate(|era_info| {
era_info.unlocking_removed(amount);
});

Self::deposit_event(Event::<T>::ClaimedUnlocked { account, amount });

Ok(Some(T::WeightInfo::claim_unlocked(removed_entries)).into())
}
}
}
72 changes: 72 additions & 0 deletions pallets/dapp-staking-v3/src/test/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3302,3 +3302,75 @@ fn lock_correctly_considers_unlocking_amount() {
);
})
}

#[test]
fn fix_account_scenarios_work() {
ExtBuilder::build().execute_with(|| {
// 1. Lock some amount correctly, unstake it, try to fix it, and ensure the call fails
let (account_1, lock_1) = (1, 100);
assert_lock(account_1, lock_1);
assert_noop!(
DappStaking::fix_account(RuntimeOrigin::signed(11), account_1),
Error::<Test>::InvalidAccount
);

assert_unlock(account_1, lock_1);
assert_noop!(
DappStaking::fix_account(RuntimeOrigin::signed(11), account_1),
Error::<Test>::InvalidAccount
);

// 2. Reproduce the issue where the account has more frozen than balance
let (account_2, unlock_2) = (2, 13);
let lock_2 = Balances::total_balance(&account_2);
assert_lock(account_2, lock_2);
assert_unlock(account_2, unlock_2);

// With the fix implemented, the scenario needs to be reproduced by hand.
// Account calls `lock`, specifying the amount that is undergoing the unlocking process.
// It can be either more or less, it doesn't matter for the test or the issue.

// But first, a sanity check.
assert_noop!(
DappStaking::lock(RuntimeOrigin::signed(account_2), unlock_2),
Error::<Test>::ZeroAmount,
);

// Now reproduce the incorrect lock/freeze operation.
let mut ledger = Ledger::<Test>::get(&account_2);
ledger.add_lock_amount(unlock_2);
assert_ok!(DappStaking::update_ledger(&account_2, ledger));
use crate::CurrentEraInfo;
CurrentEraInfo::<Test>::mutate(|era_info| {
era_info.add_locked(unlock_2);
});
assert!(
Balances::free_balance(&account_2)
< Ledger::<Test>::get(&account_2).total_locked_amount(),
"Sanity check."
);

// Now fix the account
assert_ok!(DappStaking::fix_account(
RuntimeOrigin::signed(11),
account_2
));
System::assert_last_event(RuntimeEvent::DappStaking(Event::ClaimedUnlocked {
account: account_2,
amount: unlock_2,
}));

// Post-fix checks
assert_eq!(
Balances::free_balance(&account_2),
Ledger::<Test>::get(&account_2).total_locked_amount(),
"After the fix, balances should be equal."
);

// Cannot fix the same account again.
assert_noop!(
DappStaking::fix_account(RuntimeOrigin::signed(11), account_2),
Error::<Test>::InvalidAccount
);
})
}
Loading