From 4568c969fd87cdc25a7b962b541bef53ece33a38 Mon Sep 17 00:00:00 2001 From: Tobi Demeco Date: Thu, 12 Dec 2024 18:34:05 -0300 Subject: [PATCH] fix: :bug: fix a bug where fixed rate update in buckets was miscalculating --- pallets/providers/src/utils.rs | 41 +++++++++++-------- .../integration/msp/debt-collection.test.ts | 41 ++++++++++++++----- 2 files changed, 53 insertions(+), 29 deletions(-) diff --git a/pallets/providers/src/utils.rs b/pallets/providers/src/utils.rs index 36af819b..c46feec2 100644 --- a/pallets/providers/src/utils.rs +++ b/pallets/providers/src/utils.rs @@ -1732,27 +1732,29 @@ impl MutateBucketsInterface for pallet::Pallet { bucket_id: &Self::BucketId, delta: Self::StorageDataUnit, ) -> DispatchResult { - let (msp_id, user_id) = Buckets::::try_mutate(&bucket_id, |maybe_bucket| { + Buckets::::try_mutate(&bucket_id, |maybe_bucket| { let bucket = maybe_bucket.as_mut().ok_or(Error::::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::::BucketMustHaveMspForOperation)?; + let user_id = bucket.user_id.clone(); - Ok::<_, DispatchError>(( - bucket - .msp_id - .ok_or(Error::::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 MutateBucketsInterface for pallet::Pallet { Buckets::::try_mutate(&bucket_id, |maybe_bucket| { let bucket = maybe_bucket.as_mut().ok_or(Error::::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 MutateBucketsInterface for pallet::Pallet { )?; } + // Then, if that was successful, update the bucket size + bucket.size = bucket.size.saturating_sub(delta); + Ok(()) }) } diff --git a/test/suites/integration/msp/debt-collection.test.ts b/test/suites/integration/msp/debt-collection.test.ts index 3a29dc7f..cb3230c1 100644 --- a/test/suites/integration/msp/debt-collection.test.ts +++ b/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 = userApi.createType("Option", 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