Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Tracable defensive errors #11532

Merged
merged 8 commits into from
Jun 3, 2022
Merged

Tracable defensive errors #11532

Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b0f739d
Tracable defensive errors
Szegoo May 26, 2022
898ef83
small fixes
Szegoo May 30, 2022
9881141
fix
Szegoo May 31, 2022
923237d
refactored
Szegoo Jun 1, 2022
e9da3e2
switched to defensive_ok_or
Szegoo Jun 2, 2022
241284d
Remove unnecessary type annotations and conversions
KiChjang Jun 3, 2022
11da9e5
cargo fmt
KiChjang Jun 3, 2022
59a4651
Fixes
KiChjang Jun 3, 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
44 changes: 33 additions & 11 deletions frame/nomination-pools/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1329,13 +1329,31 @@ pub mod pallet {
MetadataExceedsMaxLen,
/// Some error occurred that should never happen. This should be reported to the
/// maintainers.
DefensiveError,
Defensive(DefensiveError),
/// Not enough points. Ty unbonding less.
NotEnoughPointsToUnbond,
/// Partial unbonding now allowed permissionlessly.
PartialUnbondNotAllowedPermissionlessly,
}

#[derive(Encode, Decode, PartialEq, TypeInfo, frame_support::PalletError)]
pub enum DefensiveError {
/// There isn't enough space in the unbond pool.
NotEnoughSpaceInUnbondPool,
KiChjang marked this conversation as resolved.
Show resolved Hide resolved
/// A (bonded) pool id does not exist.
PoolNotFound,
/// A reward pool does not exist. In all cases this is a system logic error.
RewardPoolNotFound,
/// A sub pool does not exist.
SubPoolsNotFound,
}

impl<T> From<DefensiveError> for Error<T> {
fn from(e: DefensiveError) -> Error<T> {
Error::<T>::Defensive(e)
}
}

#[pallet::call]
impl<T: Config> Pallet<T> {
/// Stake funds with a pool. The amount to bond is transferred from the member to the
Expand Down Expand Up @@ -1366,8 +1384,10 @@ pub mod pallet {

// We just need its total earnings at this point in time, but we don't need to write it
// because we are not adjusting its points (all other values can calculated virtual).
let reward_pool = RewardPool::<T>::get_and_update(pool_id)
.defensive_ok_or_else(|| Error::<T>::RewardPoolNotFound)?;
let reward_pool =
RewardPool::<T>::get_and_update(pool_id).defensive_ok_or_else(|| -> Error<T> {
DefensiveError::RewardPoolNotFound.into()
})?;

bonded_pool.try_inc_members()?;
let points_issued = bonded_pool.try_bond_funds(&who, amount, BondType::Later)?;
Expand Down Expand Up @@ -1534,14 +1554,16 @@ pub mod pallet {
.try_insert(unbond_era, UnbondPool::default())
// The above call to `maybe_merge_pools` should ensure there is
// always enough space to insert.
.defensive_map_err(|_| Error::<T>::DefensiveError)?;
.defensive_map_err(|_| -> Error<T> {
DefensiveError::NotEnoughSpaceInUnbondPool.into()
})?;
}

sub_pools
.with_era
.get_mut(&unbond_era)
// The above check ensures the pool exists.
.defensive_ok_or_else(|| Error::<T>::DefensiveError)?
.defensive_ok_or_else(|| -> Error<T> { DefensiveError::PoolNotFound.into() })?
.issue(unbonding_balance);

Self::deposit_event(Event::<T>::Unbonded {
Expand Down Expand Up @@ -1613,9 +1635,9 @@ pub mod pallet {
let current_era = T::StakingInterface::current_era();

let bonded_pool = BondedPool::<T>::get(member.pool_id)
.defensive_ok_or_else(|| Error::<T>::PoolNotFound)?;
.defensive_ok_or_else(|| -> Error<T> { DefensiveError::PoolNotFound.into() })?;
Copy link
Member

Choose a reason for hiding this comment

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

You can shorten these. Same with the other defensive calls:

Suggested change
.defensive_ok_or_else(|| -> Error<T> { DefensiveError::PoolNotFound.into() })?;
.defensive_ok_or::<Error<T>>(DefensiveError::PoolNotFound.into())?;

Not sure what @kianenigma preferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ggwpez The defensive_* functions take two generics, so I will stick with the first approach if there isn't a reason against it.

Copy link
Member

Choose a reason for hiding this comment

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

Yea the second one can be inferred with _ but whatever.

Copy link
Contributor

Choose a reason for hiding this comment

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

There actually is a cost for creating a closure every time a defensive error is encountered, and in this situation, the cost of it is higher than simply creating a DefensiveError. Is there a good reason other than saving typing to use a closure here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KiChjang I have changed the way DefensiveErrors are created. I hope the new way is better. It still has a closure, but it is a bit cleaner. It has to have a closure, otherwise the second generic can't be inferred.

Copy link
Member

Choose a reason for hiding this comment

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

You can just use defensive_ok_or instead of defensive_ok_or_else, then you dont need a closure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KiChjang I have fixed this issue, could you review now?

let mut sub_pools = SubPoolsStorage::<T>::get(member.pool_id)
.defensive_ok_or_else(|| Error::<T>::SubPoolsNotFound)?;
.defensive_ok_or_else(|| -> Error<T> { DefensiveError::SubPoolsNotFound.into() })?;

bonded_pool.ok_to_withdraw_unbonded_with(
&caller,
Expand Down Expand Up @@ -2033,10 +2055,10 @@ impl<T: Config> Pallet<T> {
who: &T::AccountId,
) -> Result<(PoolMember<T>, BondedPool<T>, RewardPool<T>), Error<T>> {
let member = PoolMembers::<T>::get(&who).ok_or(Error::<T>::PoolMemberNotFound)?;
let bonded_pool =
BondedPool::<T>::get(member.pool_id).defensive_ok_or(Error::<T>::PoolNotFound)?;
let reward_pool =
RewardPools::<T>::get(member.pool_id).defensive_ok_or(Error::<T>::PoolNotFound)?;
let bonded_pool = BondedPool::<T>::get(member.pool_id)
.defensive_ok_or::<Error<T>>(DefensiveError::PoolNotFound.into())?;
let reward_pool = RewardPools::<T>::get(member.pool_id)
.defensive_ok_or::<Error<T>>(DefensiveError::PoolNotFound.into())?;
Ok((member, bonded_pool, reward_pool))
}

Expand Down