feat(module): improve param validation (#43)

This commit changes how we validate module params, making it impossible
to create invalid addresses, names, or repeated names when updating an
existing module. The checks are now in one place.

pallets/subspace/src/global.rs

@@ -321,6 +321,6 @@ impl<T: Config> Pallet<T> {
     }
 
     pub fn rm_from_whitelist(module_key: &T::AccountId) {
-        LegitWhitelist::<T>::remove(&module_key);
+        LegitWhitelist::<T>::remove(module_key);
     }
 }

pallets/subspace/src/lib.rs

@@ -59,6 +59,7 @@ pub mod pallet {
     use frame_support::{pallet_prelude::*, traits::Currency};
     use frame_system::pallet_prelude::*;
 
+    use module::ModuleChangeset;
     use sp_arithmetic::per_things::Percent;
     pub use sp_std::{vec, vec::Vec};
 
@@ -894,10 +895,8 @@ pub mod pallet {
     // Errors inform users that something went wrong.
     #[pallet::error]
     pub enum Error<T> {
-        ModuleNameAlreadyExists, // --- Thrown when a module name already exists.
-        NetworkDoesNotExist,     // --- Thrown when the network does not exist.
+        NetworkDoesNotExist, // --- Thrown when the network does not exist.
         TooFewVotesForNewProposal,
-        ModuleAddressTooLong,
         NetworkExist, // --- Thrown when the network already exist.
         InvalidIpType, /* ---- Thrown when the user tries to serve an module which
                        * is not of type	4 (IPv4) or 6 (IPv6). */
@@ -952,11 +951,7 @@ pub mod pallet {
         BalanceNotAdded,
         StakeNotRemoved,
         SubnetNameNotExists,
-        ModuleNameTooLong, /* --- Thrown when the user tries to register a module name that is
-                            * too long. */
         KeyAlreadyRegistered, //
-        ModuleNameDoesNotExist, /* --- Thrown when the user tries to remove a module name that
-                               * does not exist. */
         EmptyKeys,
         NotNominator, /* --- Thrown when the user tries to set the nominator and is not the
                        * nominator */
@@ -1002,6 +997,20 @@ pub mod pallet {
         InvalidMaxBurn,
         InvalidTargetRegistrationsPerInterval,
 
+        // Modules
+        /// The module name is too long.
+        ModuleNameTooLong,
+        /// The module name is invalid. It has to be a UTF-8 encoded string.
+        InvalidModuleName,
+        /// The address is too long.
+        ModuleAddressTooLong,
+        /// The module address is invalid.
+        InvalidModuleAddress,
+        /// The module name does not exist in the subnet.
+        ModuleNameDoesNotExist,
+        /// A module with this name already exists in the subnet.
+        ModuleNameAlreadyExists,
+
         // VOTING
         ProposalDoesNotExist,
         VotingPowerIsZero,
@@ -1076,7 +1085,9 @@ pub mod pallet {
                 for (uid_usize, (key, name, address, weights)) in
                     self.modules[subnet_idx].iter().enumerate()
                 {
-                    self::Pallet::<T>::append_module(netuid, key, name.clone(), address.clone());
+                    let changeset = ModuleChangeset::new(name.clone(), address.clone());
+                    self::Pallet::<T>::append_module(netuid, key, changeset)
+                        .expect("genesis modules are valid");
                     Weights::<T>::insert(netuid, uid_usize as u16, weights);
                 }
             }
@@ -1192,17 +1203,10 @@ pub mod pallet {
             let key = ensure_signed(origin.clone())?;
             ensure!(Self::is_registered(netuid, &key), Error::<T>::NotRegistered);
             let uid: u16 = Self::get_uid_for_key(netuid, &key);
-            let mut params = Self::module_params(netuid, uid);
-            params.name = name;
-            params.address = address;
-            if let Some(delegation_fee) = delegation_fee {
-                ensure!(
-                    delegation_fee >= Self::get_floor_delegation_fee(),
-                    Error::<T>::InvalidMinDelegationFee
-                );
-                params.delegation_fee = delegation_fee;
-            }
-            Self::do_update_module(origin, netuid, params)
+            let params = Self::module_params(netuid, uid);
+
+            let changeset = ModuleChangeset::update(&params, name, address, delegation_fee);
+            Self::do_update_module(origin, netuid, changeset)
         }
 
         #[pallet::weight((Weight::zero(), DispatchClass::Normal, Pays::No))]

pallets/subspace/src/module.rs

@@ -24,36 +24,88 @@ pub struct ModuleInfo<T: Config> {
     stats: ModuleStats<T>,
 }
 
+#[derive(Debug)]
+pub struct ModuleChangeset {
+    pub name: Option<Vec<u8>>,
+    pub address: Option<Vec<u8>>,
+    pub delegation_fee: Option<Percent>,
+}
+
+impl ModuleChangeset {
+    pub fn new(name: Vec<u8>, address: Vec<u8>) -> Self {
+        Self {
+            name: Some(name),
+            address: Some(address),
+            delegation_fee: None,
+        }
+    }
+
+    pub fn update<T: Config>(
+        params: &ModuleParams<T>,
+        name: Vec<u8>,
+        address: Vec<u8>,
+        delegation_fee: Option<Percent>,
+    ) -> Self {
+        Self {
+            name: (name != params.name).then_some(name),
+            address: (address != params.address).then_some(address),
+            delegation_fee,
+        }
+    }
+
+    /// Checks whether the module params are valid. Name and address must be non-empty and below the
+    /// max name length allowed.
+    pub fn apply<T: Config>(
+        self,
+        netuid: u16,
+        key: T::AccountId,
+        uid: u16,
+    ) -> Result<(), sp_runtime::DispatchError> {
+        let max = MaxNameLength::<T>::get() as usize;
+
+        if let Some(name) = self.name {
+            ensure!(!name.is_empty(), Error::<T>::InvalidModuleName);
+            ensure!(name.len() <= max, Error::<T>::ModuleNameTooLong);
+            core::str::from_utf8(&name).map_err(|_| Error::<T>::InvalidModuleName)?;
+            ensure!(
+                !Pallet::<T>::does_module_name_exist(netuid, &name),
+                Error::<T>::ModuleNameAlreadyExists
+            );
+
+            Name::<T>::insert(netuid, uid, name);
+        }
+
+        if let Some(addr) = self.address {
+            ensure!(!addr.is_empty(), Error::<T>::InvalidModuleAddress);
+            ensure!(addr.len() <= max, Error::<T>::ModuleAddressTooLong);
+            core::str::from_utf8(&addr).map_err(|_| Error::<T>::InvalidModuleAddress)?;
+
+            Address::<T>::insert(netuid, uid, addr);
+        }
+
+        if let Some(fee) = self.delegation_fee {
+            let floor = Pallet::<T>::get_floor_delegation_fee();
+            ensure!(fee >= floor, Error::<T>::InvalidMinDelegationFee);
+
+            DelegationFee::<T>::insert(netuid, key, fee);
+        }
+
+        Ok(())
+    }
+}
+
 impl<T: Config> Pallet<T> {
     pub fn do_update_module(
         origin: T::RuntimeOrigin,
         netuid: u16,
-        params: ModuleParams<T>,
+        changeset: ModuleChangeset,
     ) -> DispatchResult {
-        // --- 1. We check the callers (key) signature.
+        // 1. We check the callers (key) signature.
         let key = ensure_signed(origin)?;
         let uid: u16 = Self::get_uid_for_key(netuid, &key);
-        Self::check_module_params(netuid, &params)?;
-        Self::set_module_params(netuid, uid, params);
-        // --- 8. Return is successful dispatch.
-        Ok(())
-    }
 
-    /// Checks whether the module params are valid. Name and address must be non-empty and below the
-    /// max name length allowed.
-    pub fn check_module_params(_netuid: u16, params: &ModuleParams<T>) -> DispatchResult {
-        let max_name_length = MaxNameLength::<T>::get() as usize;
-
-        assert!(!params.name.is_empty());
-        ensure!(
-            params.name.len() <= max_name_length,
-            Error::<T>::ModuleNameTooLong
-        );
-        assert!(!params.address.is_empty());
-        ensure!(
-            params.address.len() <= max_name_length,
-            Error::<T>::ModuleAddressTooLong
-        );
+        // 2. Apply the changeset
+        changeset.apply::<T>(netuid, key, uid)?;
 
         Ok(())
     }
@@ -67,12 +119,6 @@ impl<T: Config> Pallet<T> {
         }
     }
 
-    pub fn set_module_params(netuid: u16, uid: u16, module_params: ModuleParams<T>) {
-        Self::set_module_name(netuid, uid, module_params.name);
-        Self::set_module_address(netuid, uid, module_params.address);
-        Self::set_module_delegation_fee(netuid, uid, module_params.delegation_fee);
-    }
-
     pub fn get_module_address(netuid: u16, uid: u16) -> Vec<u8> {
         Address::<T>::get(netuid, uid)
     }
@@ -190,31 +236,37 @@ impl<T: Config> Pallet<T> {
     }
 
     // Appends the uid to the network (without increasing stake).
-    pub fn append_module(netuid: u16, key: &T::AccountId, name: Vec<u8>, address: Vec<u8>) -> u16 {
+    pub fn append_module(
+        netuid: u16,
+        key: &T::AccountId,
+        changeset: ModuleChangeset,
+    ) -> Result<u16, sp_runtime::DispatchError> {
         // 1. Get the next uid. This is always equal to subnetwork_n.
         let uid: u16 = Self::get_subnet_n(netuid);
         let block_number = Self::get_current_block_number();
+
         log::debug!("append_module( netuid: {netuid:?} | uid: {key:?} | new_key: {uid:?})");
-        // 3. Expand with new position.
-        Emission::<T>::append(netuid, 0);
-        Incentive::<T>::append(netuid, 0);
-        Dividends::<T>::append(netuid, 0);
-        LastUpdate::<T>::append(netuid, block_number);
 
-        // 4. Insert new account information.
+        // 2. Apply the changeset
+        changeset.apply::<T>(netuid, key.clone(), uid)?;
+
+        // 3. Insert new account information.
         Keys::<T>::insert(netuid, uid, key); // Make key - uid association.
         Uids::<T>::insert(netuid, key, uid); // Make uid - key association.
         RegistrationBlock::<T>::insert(netuid, uid, block_number); // Fill block at registration.
-        Name::<T>::insert(netuid, uid, name); // Fill module namespace.
-        Address::<T>::insert(netuid, uid, address); // Fill module info.
-        DelegationFee::<T>::insert(netuid, key, DelegationFee::<T>::get(netuid, key)); // Make uid - key association.
 
-        N::<T>::insert(netuid, N::<T>::get(netuid) + 1); // Increase the number of modules in the network.
+        N::<T>::mutate(netuid, |n| *n += 1); // Increase the number of modules in the network.
+
+        // 4. Expand with new position.
+        Emission::<T>::append(netuid, 0);
+        Incentive::<T>::append(netuid, 0);
+        Dividends::<T>::append(netuid, 0);
+        LastUpdate::<T>::append(netuid, block_number);
 
         // increase the stake of the new key
         Self::increase_stake(netuid, key, key, 0);
 
-        uid
+        Ok(uid)
     }
 
     pub fn get_module_stats(netuid: u16, uid: u16) -> ModuleStats<T> {

pallets/subspace/src/registration.rs

@@ -1,3 +1,5 @@
+use crate::module::ModuleChangeset;
+
 use super::*;
 
 use frame_support::pallet_prelude::DispatchResult;
@@ -104,24 +106,19 @@ impl<T: Config> Pallet<T> {
             Error::<T>::NotEnoughStakeToRegister
         );
 
-        // --- 6. Ensure the module key is not already registered,
-        // and namespace is not already taken.
+        // --- 6. Ensure the module key is not already registered.
         ensure!(
             !Self::key_registered(netuid, &module_key),
             Error::<T>::KeyAlreadyRegistered
         );
 
-        ensure!(
-            !Self::does_module_name_exist(netuid, name.clone()),
-            Error::<T>::NameAlreadyRegistered
-        );
-
         // --- 7. Check if we are exceeding the max allowed modules per network.
         // If we do deregister slot.
         Self::check_module_limits(netuid);
 
         // --- 8. Register the module.
-        let uid: u16 = Self::append_module(netuid, &module_key, name, address);
+        let changeset = ModuleChangeset::new(name, address);
+        let uid: u16 = Self::append_module(netuid, &module_key, changeset)?;
 
         // --- 9. Add the stake to the module, now that it is registered on the network.
         Self::do_add_stake(origin, netuid, module_key.clone(), stake)?;

pallets/subspace/src/subnet.rs

@@ -280,9 +280,9 @@ impl<T: Config> Pallet<T> {
     }
 
     // TODO: see if we can optimize this further
-    pub fn does_module_name_exist(netuid: u16, name: Vec<u8>) -> bool {
+    pub fn does_module_name_exist(netuid: u16, name: &[u8]) -> bool {
         <Name<T> as IterableStorageDoubleMap<u16, u16, Vec<u8>>>::iter_prefix(netuid)
-            .any(|(_uid, _name)| _name == name)
+            .any(|(_, existing)| existing == name)
     }
 
     pub fn is_subnet_founder(netuid: u16, key: &T::AccountId) -> bool {
@@ -616,37 +616,18 @@ impl<T: Config> Pallet<T> {
         let uid = Self::get_uid_for_key(netuid, key);
         Self::get_emission_for_uid(netuid, uid)
     }
+
     pub fn get_emission_for_uid(netuid: u16, uid: u16) -> u64 {
-        let vec = Emission::<T>::get(netuid);
-        if (uid as usize) < vec.len() {
-            vec[uid as usize]
-        } else {
-            0
-        }
+        Emission::<T>::get(netuid).get(uid as usize).copied().unwrap_or_default()
     }
     pub fn get_incentive_for_uid(netuid: u16, uid: u16) -> u16 {
-        let vec = Incentive::<T>::get(netuid);
-        if (uid as usize) < vec.len() {
-            vec[uid as usize]
-        } else {
-            0
-        }
+        Incentive::<T>::get(netuid).get(uid as usize).copied().unwrap_or_default()
     }
     pub fn get_dividends_for_uid(netuid: u16, uid: u16) -> u16 {
-        let vec = Dividends::<T>::get(netuid);
-        if (uid as usize) < vec.len() {
-            vec[uid as usize]
-        } else {
-            0
-        }
+        Dividends::<T>::get(netuid).get(uid as usize).copied().unwrap_or_default()
     }
     pub fn get_last_update_for_uid(netuid: u16, uid: u16) -> u64 {
-        let vec = LastUpdate::<T>::get(netuid);
-        if (uid as usize) < vec.len() {
-            vec[uid as usize]
-        } else {
-            0
-        }
+        LastUpdate::<T>::get(netuid).get(uid as usize).copied().unwrap_or_default()
     }
 
     pub fn get_global_max_allowed_subnets() -> u16 {