fix: 🐛 fix a bug where fixed rate update in buckets was miscalculating

pallets/providers/src/utils.rs

@@ -1732,27 +1732,29 @@ impl<T: pallet::Config> MutateBucketsInterface for pallet::Pallet<T> {
         bucket_id: &Self::BucketId,
         delta: Self::StorageDataUnit,
     ) -> DispatchResult {
-        let (msp_id, user_id) = Buckets::<T>::try_mutate(&bucket_id, |maybe_bucket| {
+        Buckets::<T>::try_mutate(&bucket_id, |maybe_bucket| {
             let bucket = maybe_bucket.as_mut().ok_or(Error::<T>::BucketNotFound)?;
 
-            bucket.size = bucket.size.saturating_add(delta);
+            // Get the MSP ID and the user owner of the bucket
+            let msp_id = bucket
+                .msp_id
+                .ok_or(Error::<T>::BucketMustHaveMspForOperation)?;
+            let user_id = bucket.user_id.clone();
 
-            Ok::<_, DispatchError>((
-                bucket
-                    .msp_id
-                    .ok_or(Error::<T>::BucketMustHaveMspForOperation)?,
-                bucket.user_id.clone(),
-            ))
-        })?;
+            // First, try to update the fixed rate payment stream with the new rate, since
+            // this function uses the current bucket size to calculate it
+            Self::apply_delta_fixed_rate_payment_stream(
+                &msp_id,
+                bucket_id,
+                &user_id,
+                RateDeltaParam::Increase(delta),
+            )?;
 
-        Self::apply_delta_fixed_rate_payment_stream(
-            &msp_id,
-            bucket_id,
-            &user_id,
-            RateDeltaParam::Increase(delta),
-        )?;
+            // Then, if that was successful, update the bucket size
+            bucket.size = bucket.size.saturating_add(delta);
 
-        Ok(())
+            Ok::<_, DispatchError>(())
+        })
     }
 
     fn decrease_bucket_size(
@@ -1762,8 +1764,8 @@ impl<T: pallet::Config> MutateBucketsInterface for pallet::Pallet<T> {
         Buckets::<T>::try_mutate(&bucket_id, |maybe_bucket| {
             let bucket = maybe_bucket.as_mut().ok_or(Error::<T>::BucketNotFound)?;
 
-            bucket.size = bucket.size.saturating_sub(delta);
-
+            // First, try to update the fixed rate payment stream with the new rate, since
+            // this function uses the current bucket size to calculate it
             if let Some(msp_id) = bucket.msp_id {
                 Self::apply_delta_fixed_rate_payment_stream(
                     &msp_id,
@@ -1773,6 +1775,9 @@ impl<T: pallet::Config> MutateBucketsInterface for pallet::Pallet<T> {
                 )?;
             }
 
+            // Then, if that was successful, update the bucket size
+            bucket.size = bucket.size.saturating_sub(delta);
+
             Ok(())
         })
     }

test/suites/integration/msp/debt-collection.test.ts

@@ -2,6 +2,7 @@ import assert, { strictEqual } from "node:assert";
 import { describeMspNet, shUser, sleep, type EnrichedBspApi } from "../../../util";
 import { DUMMY_MSP_ID, MSP_CHARGING_PERIOD } from "../../../util/bspNet/consts";
 import type { H256 } from "@polkadot/types/interfaces";
+import type { Option } from "@polkadot/types";
 
 describeMspNet("Single MSP collecting debt", ({ before, createMspApi, it, createUserApi }) => {
   let userApi: EnrichedBspApi;
@@ -194,18 +195,18 @@ describeMspNet("Single MSP collecting debt", ({ before, createMspApi, it, create
         userApi.shConsts.NODE_INFOS.user.AddressId
       )
     ).unwrap();
+    const bucketOption: Option<H256> = userApi.createType("Option<H256>", bucketId);
     const firstFileSize = (
-      await userApi.rpc.storagehubclient.getFileMetadata(bucketId, acceptedFileKey)
+      await mspApi.rpc.storagehubclient.getFileMetadata(bucketOption, acceptedFileKey)
     )
       .unwrap()
       .file_size.toNumber();
     const unitsInGigaUnit = 1024 * 1024 * 1024;
-    let expectedPaymentStreamRate = Math.round(
+    let expectedPaymentStreamRate =
       (valueProps[0].value_prop.price_per_giga_unit_of_data_per_block.toNumber() * firstFileSize) /
         unitsInGigaUnit +
-        zeroSizeBucketFixedRate
-    );
-    strictEqual(paymentStream.rate.toNumber(), expectedPaymentStreamRate);
+      zeroSizeBucketFixedRate;
+    strictEqual(paymentStream.rate.toNumber(), Math.round(expectedPaymentStreamRate));
 
     // Seal block containing the MSP's transaction response to the storage request
     await userApi.wait.mspResponseInTxPool();
@@ -274,12 +275,12 @@ describeMspNet("Single MSP collecting debt", ({ before, createMspApi, it, create
       )
     ).unwrap();
     const secondFileSize = (
-      await userApi.rpc.storagehubclient.getFileMetadata(bucketId, fileKeys2[0])
+      await mspApi.rpc.storagehubclient.getFileMetadata(bucketOption, fileKeys2[0])
     )
       .unwrap()
       .file_size.toNumber();
     const thirdFileSize = (
-      await userApi.rpc.storagehubclient.getFileMetadata(bucketId, fileKeys2[1])
+      await mspApi.rpc.storagehubclient.getFileMetadata(bucketOption, fileKeys2[1])
     )
       .unwrap()
       .file_size.toNumber();
@@ -307,18 +308,36 @@ describeMspNet("Single MSP collecting debt", ({ before, createMspApi, it, create
     await userApi.block.seal();
 
     // Verify that the MSP was able to charge the user after the notify period.
-    const firstPaymentStreamChargedEvent = await userApi.assert.eventPresent(
+    // Get all the PaymentStreamCharged events
+    const firstPaymentStreamChargedEvents = await userApi.assert.eventMany(
       "paymentStreams",
       "PaymentStreamCharged"
     );
+
+    // Keep only the ones that belong to the MSP, by checking the Provider ID
+    const firstPaymentStreamChargedEventsFiltered = firstPaymentStreamChargedEvents.filter((e) => {
+      const event = e.event;
+      assert(userApi.events.paymentStreams.PaymentStreamCharged.is(event));
+      return event.data.providerId.eq(DUMMY_MSP_ID);
+    });
+
+    // There should be only one PaymentStreamCharged event for the MSP
     assert(
-      userApi.events.paymentStreams.PaymentStreamCharged.is(firstPaymentStreamChargedEvent.event)
+      firstPaymentStreamChargedEventsFiltered.length === 1,
+      "Expected a single PaymentStreamCharged event"
+    );
+
+    // Get it and check that the user account matches
+    const firstPaymentStreamChargedEvent = firstPaymentStreamChargedEvents[0];
+    assert(
+      userApi.events.paymentStreams.PaymentStreamCharged.is(firstPaymentStreamChargedEvent.event),
+      "Expected PaymentStreamCharged event"
     );
-    assert(firstPaymentStreamChargedEvent.event.data.providerId.eq(DUMMY_MSP_ID));
     assert(
       firstPaymentStreamChargedEvent.event.data.userAccount.eq(
         userApi.shConsts.NODE_INFOS.user.AddressId
-      )
+      ),
+      "User account does not match"
     );
 
     // Advance many MSP charging periods to charge again, but this time with a known number of