Hector Bulgarini SBP Milestone Review 2

[hbulgarini]

HB Substrate Builder Program - Milestone Review

TIP (ternoa improvement proposals)

Very good job documenting your specs for every pallet! Those were very useful at the time of the review.

State machine for state updates and checks.

I have detected that in a few places there is most of the time similar checks like the ones below:

	// Checks
    ensure!(nft.owner == who, Error::<T>::NotTheNFTOwner);
    ensure!(!nft.state.is_listed, Error::<T>::CannotDelegateListedNFTs);
    ensure!(!nft.state.is_rented, Error::<T>::CannotDelegateRentedNFTs);
    ensure!(!nft.state.is_syncing_secret, Error::<T>::CannotDelegateSyncingNFTs);
    ensure!(!nft.state.is_syncing_capsule, Error::<T>::CannotDelegateSyncingCapsules);
    ensure!(!nft.state.is_transmission, Error::<T>::CannotDelegateNFTsInTransmission);

or

	// Checks
    ensure!(nft.owner == who, Error::<T>::NotTheNFTOwner);
    ensure!(nft.creator == who, Error::<T>::NotTheNFTCreator);
    ensure!(!nft.state.is_listed, Error::<T>::CannotSetRoyaltyForListedNFTs);
    ensure!(!nft.state.is_delegated, Error::<T>::CannotSetRoyaltyForDelegatedNFTs);
    ensure!(!nft.state.is_rented, Error::<T>::CannotSetRoyaltyForRentedNFTs);
    ensure!(!nft.state.is_syncing_secret, Error::<T>::CannotSetRoyaltyForSyncingNFTs);
    ensure!(
    	!nft.state.is_syncing_capsule,
    	Error::<T>::CannotSetRoyaltyForSyncingCapsules
    );
    ensure!(
    	!nft.state.is_transmission,
    	Error::<T>::CannotSetRoyaltyForNFTsInTransmission
    );

I understand that checks are not strictly the same over the different functions and states of the nfts, but at least it could be one new function that might check the verifications and in case there are specific checks, they could be conducted as follow:

fn state_machine(nft: NFTData, state: NFTStateModifiers) -> NFTStateModifiers {
    /// common checks
    ensure!(nft.owner == who, Error::<T>::NotTheNFTOwner);
	ensure!(nft.creator == who, Error::<T>::NotTheNFTCreator);

    ...

    match state {
        /// Delegated
        NFTStateModifiers::Delegated => {
            ensure!(!nft.state.is_delegated, Error::<T>::CannotSetRoyaltyForDelegatedNFTs);
        }
    }

    /// 
}

(Checks and logic might not be the right one but you have the idea)

I think this might help to get all the rules in one function instead of going through different parts of the code for verifying the the state transition.

This same principle could be applied for pallet call where the logic structure seems to be the same across different pallets:

pub fn call_nft(...parameters) -> DispatchResult {
    /// mutate
    StorageItem::<T>::try_mutate(nft_id, |data| -> DispatchResult {
        /// state transition update over the nft
        data.state = state;
        Ok(().into())
    })
    ///event
    Self::deposit_event(event);
}

Anyway i would prioritize the checks over this last recommendation for calls since in a way the NFTExt in a way is defining a shared behavior over this interface. But maybe the extrinsics could be reviewed under this concept and avoid code repetition (not strictly necessary to implement, it could just work as inspiration)

.clone()

I detected that the project that are a considerable number of clone()s across the code base that can be removed.
Some methods in the traits might accept references instead of values, avoiding unnecessary clones when the functions are called.

Hardcoded weights in weights.rs

Even if the benchmarking functions were developed the weight files are still using the 10_000 hardcoded values. Once the benchmark function is executed, the resulting weight file should be replaced so it is processing the weights values from the bechmarking.

Old release.

Please take into account that the project is running a very old version of polkadot and substate. The latest release at the time of this PR is 0.9.42 and this code base is using 0.9.30.

Please follow the releases highlights in the Polkadot Release Analysis report for further details about migrations, breaking changes or new features:

https://forum.polkadot.network/tag/release-analysis

Weights v2.

Related to the previous topic when the release 0.9.38 is reached you might want to migrate to use weights v2 functions (from_parts instead of from_ref_time).

I recommend going through the following PR to migrate into Weights v2 properly:

paritytech/substrate#11637

You might also consider creating a weights template like this one to have more control over the weights process generation coming from the benchmarking.