docs and fixes

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

pallets/ah-migrator/src/multisig.rs

@@ -49,6 +49,7 @@ impl<T: Config> Pallet<T> {
 				log::error!(target: LOG_TARGET, "Failed to unreserve deposit for multisig {} missing {:?}, details: {:?}", multisig.creator.to_ss58check(), missing, multisig.details);
 			}
 		}
+
 		Ok(())
 	}
 }

pallets/rc-migrator/src/accounts.rs

@@ -74,6 +74,7 @@ use crate::{types::*, *};
 use frame_support::{traits::tokens::IdAmount, weights::WeightMeter};
 use frame_system::Account as SystemAccount;
 use pallet_balances::{AccountData, BalanceLock};
+use sp_runtime::traits::Zero;
 
 /// Account type meant to transfer data between RC and AH.
 #[derive(Encode, Decode, Clone, PartialEq, Eq, RuntimeDebug, TypeInfo)]
@@ -160,7 +161,6 @@ impl<T: Config> Pallet<T> {
 	///
 	/// Returns `None` when there are no accounts present.
 	pub fn first_account(_weight: &mut WeightMeter) -> Result<Option<T::AccountId>, Error<T>> {
-		();
 		Ok(SystemAccount::<T>::iter_keys().next())
 	}
 	// TODO: Currently, we use `debug_assert!` for basic test checks against a production snapshot.
@@ -205,9 +205,9 @@ impl<T: Config> Pallet<T> {
 				Ok(None) => continue,
 				Ok(Some(ah_account)) => package.push(ah_account),
 				// Not enough weight, lets try again in the next block since we made some progress.
-				Err(Error::OutOfWeight) if package.len() > 0 => break Some(who.clone()),
+				Err(Error::OutOfWeight) if !package.is_empty() => break Some(who.clone()),
 				// Not enough weight and was unable to make progress, bad.
-				Err(Error::OutOfWeight) if package.len() == 0 => {
+				Err(Error::OutOfWeight) if package.is_empty() => {
 					defensive!("Not enough weight to migrate a single account");
 					return Err(Error::OutOfWeight);
 				},
@@ -298,6 +298,18 @@ impl<T: Config> Pallet<T> {
 
 		let account_data: AccountData<T::Balance> = account_info.data.clone();
 
+		if account_data.free.is_zero() &&
+			account_data.reserved.is_zero() &&
+			account_data.frozen.is_zero()
+		{
+			if account_info.nonce.is_zero() {
+				log::warn!(target: LOG_TARGET, "Possible system account detected '{}'", who.to_ss58check());
+			} else {
+				log::warn!(target: LOG_TARGET, "Weird account detected '{}'", who.to_ss58check());
+			}
+			return Ok(None);
+		}
+
 		let freezes: Vec<IdAmount<T::FreezeIdentifier, T::Balance>> =
 			pallet_balances::Freezes::<T>::get(&who).into();
 

pallets/rc-migrator/src/multisig.md

@@ -0,0 +1,23 @@
+## Pallet Multisig
+
+The issue with the `multisig` pallet is that every Multisig is scoped to a specific call hash. It is
+not possible to just create a Multisig between Alice and Bob - it must always be scoped to a
+specific call hash. A Multisig is only valid for its specific call hash.
+
+Now, migrating call hashes from the relay to AH is dangerous. The preimage data of that hash either
+does not decode anymore (best case) or decodes to something else (worse case). We can therefore  not
+migrate the pure state of the `multisig` pallet. The only thing that goes amiss are previous
+approvals on a specific call hash by the Multisig members.
+
+One thing to consider is that Multisigs are constructed from account IDs. In order to allow the same
+Multisigs to be re-created, it is paramount to keep all account IDs that were accessible on the
+relay still accessible, hence: https://github.com/polkadot-fellows/runtimes/issues/526. Otherwise it
+could happen that a Multisig cannot be re-created and loses funds to its associated accounts.
+
+Note: I considered an XCM where the call is sent back to the relay to execute instead of executing
+on AH. This would allow to migrate Multisigs, but we either need to create a new pallet for this or
+change the existing one. Both probably not worth it for us now.
+### Actionable
+
+The only thing that we should do is to unlock the deposits on the AH since they were migrated to AH
+with the account state.

pallets/rc-migrator/src/multisig.rs

@@ -123,6 +123,7 @@ impl<T: Config> PalletMigration for MultisigMigrator<T> {
 
 		let maybe_last_key = loop {
 			log::debug!("Migrating multisigs from {:?}", last_key);
+
 			let kv = iter.next();
 			let Some((k1, k2, multisig)) = kv else {
 				break None;