Pallet assets improvements (paritytech#13543)

Author: Dinonard

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(

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(())
 		}

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())
 	}
 }
 

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
+		);
 	});
 }