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

Commit

Permalink
Remove IncRefError, DecRefError and StoredMapError (#9384)
Browse files Browse the repository at this point in the history
All of them are a subset of `DispatchError` anyway, no need to have
special errors IMHO.
  • Loading branch information
bkchr authored Jul 20, 2021
1 parent 04f6f68 commit 03d4d0d
Show file tree
Hide file tree
Showing 9 changed files with 45 additions and 98 deletions.
6 changes: 3 additions & 3 deletions frame/assets/src/impl_stored_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ impl<T: Config<I>, I: 'static> StoredMap<(T::AssetId, T::AccountId), T::Extra> f
}
}

fn try_mutate_exists<R, E: From<StoredMapError>>(
fn try_mutate_exists<R, E: From<DispatchError>>(
id_who: &(T::AssetId, T::AccountId),
f: impl FnOnce(&mut Option<T::Extra>) -> Result<R, E>,
) -> Result<R, E> {
Expand All @@ -46,11 +46,11 @@ impl<T: Config<I>, I: 'static> StoredMap<(T::AssetId, T::AccountId), T::Extra> f
if let Some(ref mut account) = maybe_account {
account.extra = extra;
} else {
Err(StoredMapError::NoProviders)?;
Err(DispatchError::NoProviders)?;
}
} else {
// They want to delete it. Let this pass if the item never existed anyway.
ensure!(maybe_account.is_none(), StoredMapError::ConsumerRemaining);
ensure!(maybe_account.is_none(), DispatchError::ConsumerRemaining);
}
Ok(r)
})
Expand Down
5 changes: 1 addition & 4 deletions frame/assets/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,10 +141,7 @@ pub use types::*;
use sp_std::{prelude::*, borrow::Borrow, convert::TryInto};
use sp_runtime::{
TokenError, ArithmeticError,
traits::{
AtLeast32BitUnsigned, Zero, StaticLookup, Saturating, CheckedSub, CheckedAdd, Bounded,
StoredMapError,
}
traits::{AtLeast32BitUnsigned, Zero, StaticLookup, Saturating, CheckedSub, CheckedAdd, Bounded}
};
use codec::HasCompact;
use frame_support::{ensure, dispatch::{DispatchError, DispatchResult}};
Expand Down
12 changes: 6 additions & 6 deletions frame/balances/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ use sp_runtime::{
RuntimeDebug, DispatchResult, DispatchError, ArithmeticError,
traits::{
Zero, AtLeast32BitUnsigned, StaticLookup, CheckedAdd, CheckedSub,
MaybeSerializeDeserialize, Saturating, Bounded, StoredMapError,
MaybeSerializeDeserialize, Saturating, Bounded,
},
};
use frame_system as system;
Expand Down Expand Up @@ -830,8 +830,8 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
pub fn mutate_account<R>(
who: &T::AccountId,
f: impl FnOnce(&mut AccountData<T::Balance>) -> R,
) -> Result<R, StoredMapError> {
Self::try_mutate_account(who, |a, _| -> Result<R, StoredMapError> { Ok(f(a)) })
) -> Result<R, DispatchError> {
Self::try_mutate_account(who, |a, _| -> Result<R, DispatchError> { Ok(f(a)) })
}

/// Mutate an account to some new value, or delete it entirely with `None`. Will enforce
Expand All @@ -843,7 +843,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
///
/// NOTE: LOW-LEVEL: This will not attempt to maintain total issuance. It is expected that
/// the caller will do this.
fn try_mutate_account<R, E: From<StoredMapError>>(
fn try_mutate_account<R, E: From<DispatchError>>(
who: &T::AccountId,
f: impl FnOnce(&mut AccountData<T::Balance>, bool) -> Result<R, E>,
) -> Result<R, E> {
Expand All @@ -867,7 +867,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
///
/// NOTE: LOW-LEVEL: This will not attempt to maintain total issuance. It is expected that
/// the caller will do this.
fn try_mutate_account_with_dust<R, E: From<StoredMapError>>(
fn try_mutate_account_with_dust<R, E: From<DispatchError>>(
who: &T::AccountId,
f: impl FnOnce(&mut AccountData<T::Balance>, bool) -> Result<R, E>,
) -> Result<(R, DustCleaner<T, I>), E> {
Expand Down Expand Up @@ -1449,7 +1449,7 @@ impl<T: Config<I>, I: 'static> Currency<T::AccountId> for Pallet<T, I> where

for attempt in 0..2 {
match Self::try_mutate_account(who,
|account, _is_new| -> Result<(Self::NegativeImbalance, Self::Balance), StoredMapError> {
|account, _is_new| -> Result<(Self::NegativeImbalance, Self::Balance), DispatchError> {
// Best value is the most amount we can slash following liveness rules.
let best_value = match attempt {
// First attempt we try to slash the full amount, and see if liveness issues happen.
Expand Down
6 changes: 3 additions & 3 deletions frame/support/src/traits/misc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

//! Smaller traits used in FRAME which don't need their own file.

use sp_runtime::traits::{StoredMapError, Block as BlockT};
use sp_runtime::{traits::Block as BlockT, DispatchError};
use sp_arithmetic::traits::AtLeast32Bit;
use crate::dispatch::Parameter;

Expand Down Expand Up @@ -157,10 +157,10 @@ pub trait OnKilledAccount<AccountId> {
/// A simple, generic one-parameter event notifier/handler.
pub trait HandleLifetime<T> {
/// An account was created.
fn created(_t: &T) -> Result<(), StoredMapError> { Ok(()) }
fn created(_t: &T) -> Result<(), DispatchError> { Ok(()) }

/// An account was killed.
fn killed(_t: &T) -> Result<(), StoredMapError> { Ok(()) }
fn killed(_t: &T) -> Result<(), DispatchError> { Ok(()) }
}

impl<T> HandleLifetime<T> for () {}
Expand Down
24 changes: 12 additions & 12 deletions frame/support/src/traits/stored_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
//! Traits and associated datatypes for managing abstract stored values.

use codec::FullCodec;
use sp_runtime::traits::StoredMapError;
use sp_runtime::DispatchError;
use crate::storage::StorageMap;
use crate::traits::misc::HandleLifetime;

Expand All @@ -31,15 +31,15 @@ pub trait StoredMap<K, T: Default> {

/// Maybe mutate the item only if an `Ok` value is returned from `f`. Do nothing if an `Err` is
/// returned. It is removed or reset to default value if it has been mutated to `None`
fn try_mutate_exists<R, E: From<StoredMapError>>(
fn try_mutate_exists<R, E: From<DispatchError>>(
k: &K,
f: impl FnOnce(&mut Option<T>) -> Result<R, E>,
) -> Result<R, E>;

// Everything past here has a default implementation.

/// Mutate the item.
fn mutate<R>(k: &K, f: impl FnOnce(&mut T) -> R) -> Result<R, StoredMapError> {
fn mutate<R>(k: &K, f: impl FnOnce(&mut T) -> R) -> Result<R, DispatchError> {
Self::mutate_exists(k, |maybe_account| match maybe_account {
Some(ref mut account) => f(account),
x @ None => {
Expand All @@ -57,15 +57,15 @@ pub trait StoredMap<K, T: Default> {
fn mutate_exists<R>(
k: &K,
f: impl FnOnce(&mut Option<T>) -> R,
) -> Result<R, StoredMapError> {
Self::try_mutate_exists(k, |x| -> Result<R, StoredMapError> { Ok(f(x)) })
) -> Result<R, DispatchError> {
Self::try_mutate_exists(k, |x| -> Result<R, DispatchError> { Ok(f(x)) })
}

/// Set the item to something new.
fn insert(k: &K, t: T) -> Result<(), StoredMapError> { Self::mutate(k, |i| *i = t) }
fn insert(k: &K, t: T) -> Result<(), DispatchError> { Self::mutate(k, |i| *i = t) }

/// Remove the item or otherwise replace it with its default value; we don't care which.
fn remove(k: &K) -> Result<(), StoredMapError> { Self::mutate_exists(k, |x| *x = None) }
fn remove(k: &K) -> Result<(), DispatchError> { Self::mutate_exists(k, |x| *x = None) }
}

/// A shim for placing around a storage item in order to use it as a `StoredValue`. Ideally this
Expand All @@ -87,27 +87,27 @@ impl<
T: FullCodec + Default,
> StoredMap<K, T> for StorageMapShim<S, L, K, T> {
fn get(k: &K) -> T { S::get(k) }
fn insert(k: &K, t: T) -> Result<(), StoredMapError> {
fn insert(k: &K, t: T) -> Result<(), DispatchError> {
if !S::contains_key(&k) {
L::created(k)?;
}
S::insert(k, t);
Ok(())
}
fn remove(k: &K) -> Result<(), StoredMapError> {
fn remove(k: &K) -> Result<(), DispatchError> {
if S::contains_key(&k) {
L::killed(&k)?;
S::remove(k);
}
Ok(())
}
fn mutate<R>(k: &K, f: impl FnOnce(&mut T) -> R) -> Result<R, StoredMapError> {
fn mutate<R>(k: &K, f: impl FnOnce(&mut T) -> R) -> Result<R, DispatchError> {
if !S::contains_key(&k) {
L::created(k)?;
}
Ok(S::mutate(k, f))
}
fn mutate_exists<R>(k: &K, f: impl FnOnce(&mut Option<T>) -> R) -> Result<R, StoredMapError> {
fn mutate_exists<R>(k: &K, f: impl FnOnce(&mut Option<T>) -> R) -> Result<R, DispatchError> {
S::try_mutate_exists(k, |maybe_value| {
let existed = maybe_value.is_some();
let r = f(maybe_value);
Expand All @@ -121,7 +121,7 @@ impl<
Ok(r)
})
}
fn try_mutate_exists<R, E: From<StoredMapError>>(
fn try_mutate_exists<R, E: From<DispatchError>>(
k: &K,
f: impl FnOnce(&mut Option<T>) -> Result<R, E>,
) -> Result<R, E> {
Expand Down
54 changes: 16 additions & 38 deletions frame/system/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ use sp_runtime::{
self, CheckEqual, AtLeast32Bit, Zero, Lookup, LookupError,
SimpleBitOps, Hash, Member, MaybeDisplay, BadOrigin,
MaybeSerializeDeserialize, MaybeMallocSizeOf, StaticLookup, One, Bounded,
Dispatchable, AtLeast32BitUnsigned, Saturating, StoredMapError, BlockNumberProvider,
Dispatchable, AtLeast32BitUnsigned, Saturating, BlockNumberProvider,
},
};

Expand Down Expand Up @@ -1025,20 +1025,6 @@ pub enum DecRefStatus {
Exists,
}

/// Some resultant status relevant to decrementing a provider reference.
#[derive(Eq, PartialEq, RuntimeDebug)]
pub enum DecRefError {
/// Account cannot have the last provider reference removed while there is a consumer.
ConsumerRemaining,
}

/// Some resultant status relevant to incrementing a consumer reference.
#[derive(Eq, PartialEq, RuntimeDebug)]
pub enum IncRefError {
/// Account cannot introduce a consumer while there are no providers.
NoProviders,
}

impl<T: Config> Pallet<T> {
pub fn account_exists(who: &T::AccountId) -> bool {
Account::<T>::contains_key(who)
Expand Down Expand Up @@ -1085,7 +1071,7 @@ impl<T: Config> Pallet<T> {
/// Decrement the provider reference counter on an account.
///
/// This *MUST* only be done once for every time you called `inc_providers` on `who`.
pub fn dec_providers(who: &T::AccountId) -> Result<DecRefStatus, DecRefError> {
pub fn dec_providers(who: &T::AccountId) -> Result<DecRefStatus, DispatchError> {
Account::<T>::try_mutate_exists(who, |maybe_account| {
if let Some(mut account) = maybe_account.take() {
if account.providers == 0 {
Expand All @@ -1105,7 +1091,7 @@ impl<T: Config> Pallet<T> {
}
(1, c, _) if c > 0 => {
// Cannot remove last provider if there are consumers.
Err(DecRefError::ConsumerRemaining)
Err(DispatchError::ConsumerRemaining)
}
(x, _, _) => {
// Account will continue to exist as there is either > 1 provider or
Expand Down Expand Up @@ -1191,12 +1177,12 @@ impl<T: Config> Pallet<T> {
/// Increment the reference counter on an account.
///
/// The account `who`'s `providers` must be non-zero or this will return an error.
pub fn inc_consumers(who: &T::AccountId) -> Result<(), IncRefError> {
pub fn inc_consumers(who: &T::AccountId) -> Result<(), DispatchError> {
Account::<T>::try_mutate(who, |a| if a.providers > 0 {
a.consumers = a.consumers.saturating_add(1);
Ok(())
} else {
Err(IncRefError::NoProviders)
Err(DispatchError::NoProviders)
})
}

Expand Down Expand Up @@ -1559,27 +1545,23 @@ impl<T: Config> Pallet<T> {
/// Event handler which registers a provider when created.
pub struct Provider<T>(PhantomData<T>);
impl<T: Config> HandleLifetime<T::AccountId> for Provider<T> {
fn created(t: &T::AccountId) -> Result<(), StoredMapError> {
fn created(t: &T::AccountId) -> Result<(), DispatchError> {
Pallet::<T>::inc_providers(t);
Ok(())
}
fn killed(t: &T::AccountId) -> Result<(), StoredMapError> {
Pallet::<T>::dec_providers(t)
.map(|_| ())
.or_else(|e| match e {
DecRefError::ConsumerRemaining => Err(StoredMapError::ConsumerRemaining),
})
fn killed(t: &T::AccountId) -> Result<(), DispatchError> {
Pallet::<T>::dec_providers(t).map(|_| ())
}
}

/// Event handler which registers a self-sufficient when created.
pub struct SelfSufficient<T>(PhantomData<T>);
impl<T: Config> HandleLifetime<T::AccountId> for SelfSufficient<T> {
fn created(t: &T::AccountId) -> Result<(), StoredMapError> {
fn created(t: &T::AccountId) -> Result<(), DispatchError> {
Pallet::<T>::inc_sufficients(t);
Ok(())
}
fn killed(t: &T::AccountId) -> Result<(), StoredMapError> {
fn killed(t: &T::AccountId) -> Result<(), DispatchError> {
Pallet::<T>::dec_sufficients(t);
Ok(())
}
Expand All @@ -1588,13 +1570,10 @@ impl<T: Config> HandleLifetime<T::AccountId> for SelfSufficient<T> {
/// Event handler which registers a consumer when created.
pub struct Consumer<T>(PhantomData<T>);
impl<T: Config> HandleLifetime<T::AccountId> for Consumer<T> {
fn created(t: &T::AccountId) -> Result<(), StoredMapError> {
fn created(t: &T::AccountId) -> Result<(), DispatchError> {
Pallet::<T>::inc_consumers(t)
.map_err(|e| match e {
IncRefError::NoProviders => StoredMapError::NoProviders
})
}
fn killed(t: &T::AccountId) -> Result<(), StoredMapError> {
fn killed(t: &T::AccountId) -> Result<(), DispatchError> {
Pallet::<T>::dec_consumers(t);
Ok(())
}
Expand Down Expand Up @@ -1623,7 +1602,7 @@ impl<T: Config> StoredMap<T::AccountId, T::AccountData> for Pallet<T> {
Account::<T>::get(k).data
}

fn try_mutate_exists<R, E: From<StoredMapError>>(
fn try_mutate_exists<R, E: From<DispatchError>>(
k: &T::AccountId,
f: impl FnOnce(&mut Option<T::AccountData>) -> Result<R, E>,
) -> Result<R, E> {
Expand All @@ -1635,10 +1614,9 @@ impl<T: Config> StoredMap<T::AccountId, T::AccountData> for Pallet<T> {
if !was_providing && is_providing {
Self::inc_providers(k);
} else if was_providing && !is_providing {
match Self::dec_providers(k) {
Err(DecRefError::ConsumerRemaining) => Err(StoredMapError::ConsumerRemaining)?,
Ok(DecRefStatus::Reaped) => return Ok(result),
Ok(DecRefStatus::Exists) => {
match Self::dec_providers(k)? {
DecRefStatus::Reaped => return Ok(result),
DecRefStatus::Exists => {
// Update value as normal...
}
}
Expand Down
8 changes: 4 additions & 4 deletions frame/system/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,18 +121,18 @@ fn sufficient_cannot_support_consumer() {
assert_eq!(System::inc_sufficients(&0), IncRefStatus::Created);
System::inc_account_nonce(&0);
assert_eq!(System::account_nonce(&0), 1);
assert_noop!(System::inc_consumers(&0), IncRefError::NoProviders);
assert_noop!(System::inc_consumers(&0), DispatchError::NoProviders);

assert_eq!(System::inc_providers(&0), IncRefStatus::Existed);
assert_ok!(System::inc_consumers(&0));
assert_noop!(System::dec_providers(&0), DecRefError::ConsumerRemaining);
assert_noop!(System::dec_providers(&0), DispatchError::ConsumerRemaining);
});
}

#[test]
fn provider_required_to_support_consumer() {
new_test_ext().execute_with(|| {
assert_noop!(System::inc_consumers(&0), IncRefError::NoProviders);
assert_noop!(System::inc_consumers(&0), DispatchError::NoProviders);

assert_eq!(System::inc_providers(&0), IncRefStatus::Created);
System::inc_account_nonce(&0);
Expand All @@ -143,7 +143,7 @@ fn provider_required_to_support_consumer() {
assert_eq!(System::account_nonce(&0), 1);

assert_ok!(System::inc_consumers(&0));
assert_noop!(System::dec_providers(&0), DecRefError::ConsumerRemaining);
assert_noop!(System::dec_providers(&0), DispatchError::ConsumerRemaining);

System::dec_consumers(&0);
assert_eq!(System::dec_providers(&0).unwrap(), DecRefStatus::Reaped);
Expand Down
9 changes: 0 additions & 9 deletions primitives/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -516,15 +516,6 @@ impl From<crate::traits::BadOrigin> for DispatchError {
}
}

impl From<crate::traits::StoredMapError> for DispatchError {
fn from(e: crate::traits::StoredMapError) -> Self {
match e {
crate::traits::StoredMapError::ConsumerRemaining => Self::ConsumerRemaining,
crate::traits::StoredMapError::NoProviders => Self::NoProviders,
}
}
}

/// Description of what went wrong when trying to complete an operation on a token.
#[derive(Eq, PartialEq, Clone, Copy, Encode, Decode, Debug)]
#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]
Expand Down
19 changes: 0 additions & 19 deletions primitives/runtime/src/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,25 +152,6 @@ impl From<BadOrigin> for &'static str {
}
}

/// Error that can be returned by our impl of `StoredMap`.
#[derive(Encode, Decode, RuntimeDebug)]
pub enum StoredMapError {
/// Attempt to create map value when it is a consumer and there are no providers in place.
NoProviders,
/// Attempt to anull/remove value when it is the last provider and there is still at
/// least one consumer left.
ConsumerRemaining,
}

impl From<StoredMapError> for &'static str {
fn from(e: StoredMapError) -> &'static str {
match e {
StoredMapError::NoProviders => "No providers",
StoredMapError::ConsumerRemaining => "Consumer remaining",
}
}
}

/// An error that indicates that a lookup failed.
#[derive(Encode, Decode, RuntimeDebug)]
pub struct LookupError;
Expand Down

0 comments on commit 03d4d0d

Please sign in to comment.