Fix weight accounting

The problem is slightly different from the previously raised issue due to the deletion of instance pallets. Currently, the problem lies in having two tuple handlers (OnIdtyChange and Event). The weight of the function calling these handlers is the sum of each implementation. If these functions are not called in hooks, it will be possible to take the worst possible path as an approximation. However, some are called in on_initialize, and the worst case is close to 7% of a block, approximately 10 times the average path.

• Add weight return in OnIdtyChange and Event handlers.
• Account for the weight where these handlers are called.
• Replace all +=, -= by saturating_add and saturating_sub for consistency.
• Closes #163 (closed).
• Closes #203 (closed)

Note: Some handlers are difficult to benchmark using a benchmark function, so the accounting is performed by counting reads and writes, inspired by the staking pallet from Substrate.

Closes #167 (closed)

pallets/duniter-wot/src/lib.rs

@@ -104,28 +104,34 @@ pub mod pallet {
     }
 }
 
-// implement identity call checks
+/// Implementing identity call allowance check for the pallet.
 impl<AccountId, T: Config> pallet_identity::traits::CheckIdtyCallAllowed<T> for Pallet<T>
 where
     T: frame_system::Config<AccountId = AccountId> + pallet_membership::Config,
 {
-    // identity creation checks
+    /// Checks if identity creation is allowed.
+    /// This implementation checks the following:
+    ///
+    /// - Whether the identity has the right to create an identity.
+    /// - Whether the issuer can emit a certification.
+    /// - Whether the issuer respect creation period.
     fn check_create_identity(creator: IdtyIndex) -> Result<(), DispatchError> {
         let cert_meta = pallet_certification::Pallet::<T>::idty_cert_meta(creator);
-        // perform all checks
-        // 1. check that identity has the right to create an identity
-        // identity can be member with 5 certifications and still not reach identity creation threshold which could be higher (6, 7...)
+
+        // 1. Check that the identity has the right to create an identity
+        // Identity can be a member with 5 certifications and still not reach the identity creation threshold, which could be higher (6, 7...)
         ensure!(
             cert_meta.received_count >= T::MinCertForCreateIdtyRight::get(),
             Error::<T>::NotEnoughReceivedCertsToCreateIdty
         );
-        // 2. check that issuer can emit one more certification
-        // (this is only a partial check)
+
+        // 2. Check that the issuer can emit one more certification (partial check)
         ensure!(
             cert_meta.issued_count < T::MaxByIssuer::get(),
             Error::<T>::MaxEmittedCertsReached
         );
-        // 3. check that issuer respects certification creation period
+
+        // 3. Check that the issuer respects certification creation period
         ensure!(
             cert_meta.next_issuable_on <= frame_system::pallet::Pallet::<T>::block_number(),
             Error::<T>::IdtyCreationPeriodNotRespected
@@ -134,16 +140,18 @@ where
     }
 }
 
-// implement cert call checks
+/// Implementing certification allowance check for the pallet.
 impl<T: Config> pallet_certification::traits::CheckCertAllowed<IdtyIndex> for Pallet<T> {
-    // check the following:
-    // - issuer has identity
-    // - issuer identity is member
-    // - receiver has identity
-    // - receiver identity is confirmed and not revoked
+    /// Checks if certification is allowed.
+    /// This implementation checks the following:
+    ///
+    /// - Whether the issuer has an identity.
+    /// - Whether the issuer's identity is a member.
+    /// - Whether the receiver has an identity.
+    /// - Whether the receiver's identity is confirmed and not revoked.
     fn check_cert_allowed(issuer: IdtyIndex, receiver: IdtyIndex) -> Result<(), DispatchError> {
-        // issuer checks
-        // ensure issuer is member
+        // Issuer checks
+        // Ensure issuer is a member
         let issuer_data =
             pallet_identity::Pallet::<T>::identity(issuer).ok_or(Error::<T>::IdtyNotFound)?;
         ensure!(
@@ -151,8 +159,8 @@ impl<T: Config> pallet_certification::traits::CheckCertAllowed<IdtyIndex> for Pa
             Error::<T>::IssuerNotMember
         );
 
-        // receiver checks
-        // ensure receiver identity is confirmed and not revoked
+        // Receiver checks
+        // Ensure receiver identity is confirmed and not revoked
         let receiver_data =
             pallet_identity::Pallet::<T>::identity(receiver).ok_or(Error::<T>::IdtyNotFound)?;
         ensure!(
@@ -165,10 +173,14 @@ impl<T: Config> pallet_certification::traits::CheckCertAllowed<IdtyIndex> for Pa
     }
 }
 
-// implement membership call checks
+/// Implementing membership operation checks for the pallet.
 impl<T: Config> sp_membership::traits::CheckMembershipOpAllowed<IdtyIndex> for Pallet<T> {
+    /// This implementation checks the following:
+    ///
+    /// - Whether the identity's status is unvalidated or not a member.
+    /// - The count of certifications associated with the identity.
     fn check_add_membership(idty_index: IdtyIndex) -> Result<(), DispatchError> {
-        // check identity status
+        // Check identity status
         let idty_value =
             pallet_identity::Pallet::<T>::identity(idty_index).ok_or(Error::<T>::IdtyNotFound)?;
         ensure!(
@@ -176,60 +188,65 @@ impl<T: Config> sp_membership::traits::CheckMembershipOpAllowed<IdtyIndex> for P
                 || idty_value.status == IdtyStatus::NotMember,
             Error::<T>::TargetStatusInvalid
         );
-        // check cert count
+
+        // Check certificate count
         check_cert_count::<T>(idty_index)?;
         Ok(())
     }
 
-    // membership renewal is only possible when identity is member (otherwise it should claim again)
+    /// This implementation checks the following:
+    ///
+    /// - Whether the identity's status is member.
+    ///
+    /// Note: There is no need to check certification count since losing certifications makes membership expire.
+    /// Membership renewal is only possible when identity is member.
     fn check_renew_membership(idty_index: IdtyIndex) -> Result<(), DispatchError> {
-        // check identity status
         let idty_value =
             pallet_identity::Pallet::<T>::identity(idty_index).ok_or(Error::<T>::IdtyNotFound)?;
         ensure!(
             idty_value.status == IdtyStatus::Member,
             Error::<T>::TargetStatusInvalid
         );
-        // no need to check certification count since loosing certifications make membership expire
         Ok(())
     }
 }
 
-// implement membership event handler
+/// Implementing membership event handling for the pallet.
 impl<T: Config> sp_membership::traits::OnNewMembership<IdtyIndex> for Pallet<T>
 where
     T: pallet_membership::Config,
 {
+    /// This implementation notifies the identity pallet when a main membership is acquired.
+    /// It is only used on the first membership acquisition.
     fn on_created(idty_index: &IdtyIndex) {
-        // when main membership is acquired, tell identity
-        // (only used on first membership acquiry)
         pallet_identity::Pallet::<T>::membership_added(*idty_index);
     }
 
     fn on_renewed(_idty_index: &IdtyIndex) {}
 }
 
-// implement membership event handler
+/// Implementing membership removal event handling for the pallet.
 impl<T: Config> sp_membership::traits::OnRemoveMembership<IdtyIndex> for Pallet<T>
 where
     T: pallet_membership::Config,
 {
+    /// This implementation notifies the identity pallet when a main membership is lost.
     fn on_removed(idty_index: &IdtyIndex) -> Weight {
-        // when main membership is lost, tell identity
         pallet_identity::Pallet::<T>::membership_removed(*idty_index)
     }
 }
 
-// implement identity event handler
+/// Implementing the identity event handler for the pallet.
 impl<T: Config> pallet_identity::traits::OnNewIdty<T> for Pallet<T> {
+    /// This implementation adds a certificate when a new identity is created.
     fn on_created(idty_index: &IdtyIndex, creator: &IdtyIndex) {
-        // identity just has been created, a cert must be added
         <pallet_certification::Pallet<T>>::do_add_cert_checked(*creator, *idty_index, true);
     }
 }
 
+/// Implementing identity removal event handling for the pallet.
 impl<T: Config> pallet_identity::traits::OnRemoveIdty<T> for Pallet<T> {
-    // Remove membership and certificates.
+    /// This implementation removes both membership and certificates associated with the identity.
     fn on_removed(idty_index: &IdtyIndex) -> Weight {
         let mut weight = Self::on_revoked(idty_index);
         weight = weight.saturating_add(
@@ -238,7 +255,7 @@ impl<T: Config> pallet_identity::traits::OnRemoveIdty<T> for Pallet<T> {
         weight
     }
 
-    // Remove membership only.
+    /// This implementation removes membership only.
     fn on_revoked(idty_index: &IdtyIndex) -> Weight {
         let mut weight = Weight::zero();
         weight = weight.saturating_add(<pallet_membership::Pallet<T>>::do_remove_membership(
@@ -249,9 +266,10 @@ impl<T: Config> pallet_identity::traits::OnRemoveIdty<T> for Pallet<T> {
     }
 }
 
-// implement certification event handlers
-// new cert handler
+/// Implementing the certification event handler for the pallet.
 impl<T: Config> pallet_certification::traits::OnNewcert<IdtyIndex> for Pallet<T> {
+    /// This implementation checks if the receiver has received enough certificates to be able to issue certificates,
+    /// and applies the first issuable if the condition is met.
     fn on_new_cert(
         _issuer: IdtyIndex,
         _issuer_issued_count: u32,
@@ -264,8 +282,10 @@ impl<T: Config> pallet_certification::traits::OnNewcert<IdtyIndex> for Pallet<T>
     }
 }
 
-// remove cert handler
+/// Implementing the certification removal event handler for the pallet.
 impl<T: Config> pallet_certification::traits::OnRemovedCert<IdtyIndex> for Pallet<T> {
+    /// This implementation checks if the receiver has received fewer certificates than required for membership,
+    /// and if so, and the receiver is a member, it expires the receiver's membership.
     fn on_removed_cert(
         _issuer: IdtyIndex,
         _issuer_issued_count: u32,
@@ -276,7 +296,7 @@ impl<T: Config> pallet_certification::traits::OnRemovedCert<IdtyIndex> for Palle
         if receiver_received_count < T::MinCertForMembership::get()
             && pallet_membership::Pallet::<T>::is_member(&receiver)
         {
-            // expire receiver membership
+            // Expire receiver membership
             <pallet_membership::Pallet<T>>::do_remove_membership(
                 receiver,
                 MembershipRemovalReason::NotEnoughCerts,
@@ -285,10 +305,15 @@ impl<T: Config> pallet_certification::traits::OnRemovedCert<IdtyIndex> for Palle
     }
 }
 
-/// valid distance status handler
+/// Implementing the valid distance status event handler for the pallet.
 impl<T: Config + pallet_distance::Config> pallet_distance::traits::OnValidDistanceStatus<T>
     for Pallet<T>
 {
+    /// This implementation handles different scenarios based on the identity's status:
+    ///
+    /// - For `Unconfirmed` or `Revoked` identities, no action is taken.
+    /// - For `Unvalidated` or `NotMember` identities, an attempt is made to add membership.
+    /// - For `Member` identities, an attempt is made to renew membership.
     fn on_valid_distance_status(idty_index: IdtyIndex) {
         if let Some(identity) = pallet_identity::Identities::<T>::get(idty_index) {
             match identity.status {
@@ -339,15 +364,19 @@ impl<T: Config + pallet_distance::Config> pallet_distance::traits::OnValidDistan
     }
 }
 
-/// distance evaluation request allowed check
+/// Implementing the request distance evaluation check for the pallet.
 impl<T: Config + pallet_distance::Config> pallet_distance::traits::CheckRequestDistanceEvaluation<T>
     for Pallet<T>
 {
+    /// This implementation performs the following checks:
+    ///
+    /// - Membership renewal anti-spam check: Ensures that membership renewal requests respect the anti-spam period.
+    /// - Certificate count check: Ensures that the identity has a sufficient number of certificates.
     fn check_request_distance_evaluation(idty_index: IdtyIndex) -> Result<(), DispatchError> {
-        // check membership renewal antispam
+        // Check membership renewal anti-spam
         let maybe_membership_data = pallet_membership::Pallet::<T>::membership(idty_index);
         if let Some(membership_data) = maybe_membership_data {
-            // if membership data exists, this is for a renewal, apply antispam
+            // If membership data exists, this is for a renewal, apply anti-spam
             ensure!(
                 // current_block > expiration block - membership period + renewal period
                 membership_data.expire_on
@@ -357,13 +386,13 @@ impl<T: Config + pallet_distance::Config> pallet_distance::traits::CheckRequestD
                 Error::<T>::MembershipRenewalPeriodNotRespected
             );
         };
-        // check cert count
+        // Check certificate count
         check_cert_count::<T>(idty_index)?;
         Ok(())
     }
 }
 
-/// check certification count
+/// Checks the certificate count for an identity.
 fn check_cert_count<T: Config>(idty_index: IdtyIndex) -> Result<(), DispatchError> {
     let idty_cert_meta = pallet_certification::Pallet::<T>::idty_cert_meta(idty_index);
     ensure!(