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

Assets: can_decrease/increase for destroying asset is not successful #3286

Merged
merged 11 commits into from
Jul 7, 2024
16 changes: 16 additions & 0 deletions prdoc/pr_3286.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json

title: "Assets: can_decrease/increase for destroying asset is not successful"

doc:
- audience: Runtime Dev
description: |
Functions `can_decrease` and `can_increase` do not return successful consequence results
for assets undergoing destruction; instead, they return the `UnknownAsset` consequence variant.
This update aligns their behavior with similar functions, such as `reducible_balance`,
`increase_balance`, `decrease_balance`, and `burn`, which return an `AssetNotLive` error
for assets in the process of being destroyed.

crates:
- name: pallet-assets
6 changes: 6 additions & 0 deletions substrate/frame/assets/src/functions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,9 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
Some(details) => details,
None => return DepositConsequence::UnknownAsset,
};
if details.status == AssetStatus::Destroying {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd use matches! instead of ==. Same for usage below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what's the benefit? with == I follow the style within the function

Copy link
Contributor

Choose a reason for hiding this comment

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

Pattern matching is the standard way of comparing enums and it doesn't require PartialEq/Eq, but in this case it's not a big deal.

return DepositConsequence::UnknownAsset
}
if increase_supply && details.supply.checked_add(&amount).is_none() {
return DepositConsequence::Overflow
}
Expand Down Expand Up @@ -175,6 +178,9 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
if details.status == AssetStatus::Frozen {
return Frozen
}
if details.status == AssetStatus::Destroying {
return UnknownAsset
}
if amount.is_zero() {
return Success
}
Expand Down
35 changes: 34 additions & 1 deletion substrate/frame/assets/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,11 @@ use crate::{mock::*, Error};
use frame_support::{
assert_noop, assert_ok,
dispatch::GetDispatchInfo,
traits::{fungibles::InspectEnumerable, tokens::Preservation::Protect, Currency},
traits::{
fungibles::InspectEnumerable,
tokens::{Preservation::Protect, Provenance},
Currency,
},
};
use pallet_balances::Error as BalancesError;
use sp_io::storage;
Expand Down Expand Up @@ -1778,6 +1782,35 @@ fn asset_destroy_refund_existence_deposit() {
});
}

#[test]
fn increasing_or_decreasing_destroying_asset_should_not_work() {
new_test_ext().execute_with(|| {
use frame_support::traits::fungibles::Inspect;

let admin = 1;
let admin_origin = RuntimeOrigin::signed(admin);

assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, admin, true, 1));
assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100));
assert_eq!(Assets::balance(0, 1), 100);

assert_eq!(Assets::can_deposit(0, &1, 10, Provenance::Extant), DepositConsequence::Success);
assert_eq!(Assets::can_withdraw(0, &1, 10), WithdrawConsequence::<_>::Success);
assert_eq!(Assets::can_increase(0, &1, 10, false), DepositConsequence::Success);
assert_eq!(Assets::can_decrease(0, &1, 10, false), WithdrawConsequence::<_>::Success);

assert_ok!(Assets::start_destroy(admin_origin, 0));

assert_eq!(
Assets::can_deposit(0, &1, 10, Provenance::Extant),
DepositConsequence::UnknownAsset
);
assert_eq!(Assets::can_withdraw(0, &1, 10), WithdrawConsequence::<_>::UnknownAsset);
assert_eq!(Assets::can_increase(0, &1, 10, false), DepositConsequence::UnknownAsset);
assert_eq!(Assets::can_decrease(0, &1, 10, false), WithdrawConsequence::<_>::UnknownAsset);
});
}

#[test]
fn asset_id_cannot_be_reused() {
new_test_ext().execute_with(|| {
Expand Down
Loading