From efd5d98e109dbbf3921541087b8a98f4f7b17837 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dino=20Pa=C4=8Dandi?=
<3002868+Dinonard@users.noreply.github.com>
Date: Mon, 13 Mar 2023 16:44:47 +0100
Subject: [PATCH] Pallet assets improvements (#13543)
---
substrate/frame/assets/src/functions.rs | 4 +--
substrate/frame/assets/src/lib.rs | 13 ++++---
substrate/frame/assets/src/mock.rs | 40 ++++++++++++++++++---
substrate/frame/assets/src/tests.rs | 46 ++++++++++++++++++++-----
4 files changed, 85 insertions(+), 18 deletions(-)
diff --git a/substrate/frame/assets/src/functions.rs b/substrate/frame/assets/src/functions.rs
index f6e027472f1..47657ff2674 100644
--- a/substrate/frame/assets/src/functions.rs
+++ b/substrate/frame/assets/src/functions.rs
@@ -661,8 +661,8 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
status: AssetStatus::Live,
},
);
+ ensure!(T::CallbackHandle::created(&id, &owner).is_ok(), Error::<T, I>::CallbackFailed);
Self::deposit_event(Event::ForceCreated { asset_id: id, owner: owner.clone() });
- T::CallbackHandle::created(&id, &owner);
Ok(())
}
@@ -752,7 +752,6 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
approvals_destroyed: removed_approvals as u32,
approvals_remaining: details.approvals as u32,
});
- T::CallbackHandle::destroyed(&id);
Ok(())
})?;
Ok(removed_approvals)
@@ -767,6 +766,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
ensure!(details.status == AssetStatus::Destroying, Error::<T, I>::IncorrectStatus);
ensure!(details.accounts == 0, Error::<T, I>::InUse);
ensure!(details.approvals == 0, Error::<T, I>::InUse);
+ ensure!(T::CallbackHandle::destroyed(&id).is_ok(), Error::<T, I>::CallbackFailed);
let metadata = Metadata::<T, I>::take(&id);
T::Currency::unreserve(
diff --git a/substrate/frame/assets/src/lib.rs b/substrate/frame/assets/src/lib.rs
index bb0e8ce8b37..b36a5cabd37 100644
--- a/substrate/frame/assets/src/lib.rs
+++ b/substrate/frame/assets/src/lib.rs
@@ -178,10 +178,14 @@ const LOG_TARGET: &str = "runtime::assets";
/// Trait with callbacks that are executed after successfull asset creation or destruction.
pub trait AssetsCallback<AssetId, AccountId> {
/// Indicates that asset with `id` was successfully created by the `owner`
- fn created(_id: &AssetId, _owner: &AccountId) {}
+ fn created(_id: &AssetId, _owner: &AccountId) -> Result<(), ()> {
+ Ok(())
+ }
/// Indicates that asset with `id` has just been destroyed
- fn destroyed(_id: &AssetId) {}
+ fn destroyed(_id: &AssetId) -> Result<(), ()> {
+ Ok(())
+ }
}
/// Empty implementation in case no callbacks are required.
@@ -560,6 +564,8 @@ pub mod pallet {
IncorrectStatus,
/// The asset should be frozen before the given operation.
NotFrozen,
+ /// Callback action resulted in error
+ CallbackFailed,
}
#[pallet::call]
@@ -618,13 +624,12 @@ pub mod pallet {
status: AssetStatus::Live,
},
);
-
+ ensure!(T::CallbackHandle::created(&id, &owner).is_ok(), Error::<T, I>::CallbackFailed);
Self::deposit_event(Event::Created {
asset_id: id,
creator: owner.clone(),
owner: admin,
});
- T::CallbackHandle::created(&id, &owner);
Ok(())
}
diff --git a/substrate/frame/assets/src/mock.rs b/substrate/frame/assets/src/mock.rs
index 14795b10a7e..3f456a7de3e 100644
--- a/substrate/frame/assets/src/mock.rs
+++ b/substrate/frame/assets/src/mock.rs
@@ -91,12 +91,44 @@ impl pallet_balances::Config for Test {
pub struct AssetsCallbackHandle;
impl AssetsCallback<AssetId, AccountId> for AssetsCallbackHandle {
- fn created(_id: &AssetId, _owner: &AccountId) {
- storage::set(b"asset_created", &().encode());
+ fn created(_id: &AssetId, _owner: &AccountId) -> Result<(), ()> {
+ if Self::should_err() {
+ Err(())
+ } else {
+ storage::set(Self::CREATED.as_bytes(), &().encode());
+ Ok(())
+ }
}
- fn destroyed(_id: &AssetId) {
- storage::set(b"asset_destroyed", &().encode());
+ fn destroyed(_id: &AssetId) -> Result<(), ()> {
+ if Self::should_err() {
+ Err(())
+ } else {
+ storage::set(Self::DESTROYED.as_bytes(), &().encode());
+ Ok(())
+ }
+ }
+}
+
+impl AssetsCallbackHandle {
+ pub const CREATED: &'static str = "asset_created";
+ pub const DESTROYED: &'static str = "asset_destroyed";
+
+ const RETURN_ERROR: &'static str = "return_error";
+
+ // Configures `Self` to return `Ok` when callbacks are invoked
+ pub fn set_return_ok() {
+ storage::clear(Self::RETURN_ERROR.as_bytes());
+ }
+
+ // Configures `Self` to return `Err` when callbacks are invoked
+ pub fn set_return_error() {
+ storage::set(Self::RETURN_ERROR.as_bytes(), &().encode());
+ }
+
+ // If `true`, callback should return `Err`, `Ok` otherwise.
+ fn should_err() -> bool {
+ storage::exists(Self::RETURN_ERROR.as_bytes())
}
}
diff --git a/substrate/frame/assets/src/tests.rs b/substrate/frame/assets/src/tests.rs
index a2788f7f4a9..bc61810a76a 100644
--- a/substrate/frame/assets/src/tests.rs
+++ b/substrate/frame/assets/src/tests.rs
@@ -1261,28 +1261,58 @@ fn querying_roles_should_work() {
#[test]
fn normal_asset_create_and_destroy_callbacks_should_work() {
new_test_ext().execute_with(|| {
- assert!(storage::get(b"asset_created").is_none());
- assert!(storage::get(b"asset_destroyed").is_none());
+ assert!(storage::get(AssetsCallbackHandle::CREATED.as_bytes()).is_none());
+ assert!(storage::get(AssetsCallbackHandle::DESTROYED.as_bytes()).is_none());
Balances::make_free_balance_be(&1, 100);
assert_ok!(Assets::create(RuntimeOrigin::signed(1), 0, 1, 1));
- assert!(storage::get(b"asset_created").is_some());
- assert!(storage::get(b"asset_destroyed").is_none());
+ assert!(storage::get(AssetsCallbackHandle::CREATED.as_bytes()).is_some());
+ assert!(storage::get(AssetsCallbackHandle::DESTROYED.as_bytes()).is_none());
assert_ok!(Assets::start_destroy(RuntimeOrigin::signed(1), 0));
assert_ok!(Assets::destroy_accounts(RuntimeOrigin::signed(1), 0));
assert_ok!(Assets::destroy_approvals(RuntimeOrigin::signed(1), 0));
+ // Callback still hasn't been invoked
+ assert!(storage::get(AssetsCallbackHandle::DESTROYED.as_bytes()).is_none());
+
assert_ok!(Assets::finish_destroy(RuntimeOrigin::signed(1), 0));
- assert!(storage::get(b"asset_destroyed").is_some());
+ assert!(storage::get(AssetsCallbackHandle::DESTROYED.as_bytes()).is_some());
});
}
#[test]
fn root_asset_create_should_work() {
new_test_ext().execute_with(|| {
- assert!(storage::get(b"asset_created").is_none());
+ assert!(storage::get(AssetsCallbackHandle::CREATED.as_bytes()).is_none());
assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, true, 1));
- assert!(storage::get(b"asset_created").is_some());
- assert!(storage::get(b"asset_destroyed").is_none());
+ assert!(storage::get(AssetsCallbackHandle::CREATED.as_bytes()).is_some());
+ assert!(storage::get(AssetsCallbackHandle::DESTROYED.as_bytes()).is_none());
+ });
+}
+
+#[test]
+fn asset_create_and_destroy_is_reverted_if_callback_fails() {
+ new_test_ext().execute_with(|| {
+ // Asset creation fails due to callback failure
+ AssetsCallbackHandle::set_return_error();
+ Balances::make_free_balance_be(&1, 100);
+ assert_noop!(
+ Assets::create(RuntimeOrigin::signed(1), 0, 1, 1),
+ Error::<Test>::CallbackFailed
+ );
+
+ // Callback succeeds, so asset creation succeeds
+ AssetsCallbackHandle::set_return_ok();
+ assert_ok!(Assets::create(RuntimeOrigin::signed(1), 0, 1, 1));
+
+ // Asset destroy should fail due to callback failure
+ AssetsCallbackHandle::set_return_error();
+ assert_ok!(Assets::start_destroy(RuntimeOrigin::signed(1), 0));
+ assert_ok!(Assets::destroy_accounts(RuntimeOrigin::signed(1), 0));
+ assert_ok!(Assets::destroy_approvals(RuntimeOrigin::signed(1), 0));
+ assert_noop!(
+ Assets::finish_destroy(RuntimeOrigin::signed(1), 0),
+ Error::<Test>::CallbackFailed
+ );
});
}
--
GitLab