feat: add task to let MSP charge user every X blocks #268

undercover-cactus · 2024-11-21T11:16:44Z

This PR add a NotifyPeriod event that would be triggered every X blocks. It helps notifyng the MSP task that X blocks has passed and that it can charge users for their service.

The amount of blocks (aka the msp charging frequency) is a configurable value that can be passed through the CLI.

undercover-cactus · 2024-11-21T11:22:46Z

node/src/services/builder.rs

+        if let Some(notify_period) = msp_charging_freq {
+            self.with_notify_period(notify_period.clone());
+        }
+


I am not super happy for having to do this with all different setup function. It is a bit redundant with the builder strategy.

Agreed. In fact all the functions that are not storage-layer-or-provider-type-specific (i.e. that they don't need to have a special implementation for a provider type and storage layer), I'd leave them out of this setup.

In other words, to make proper use of the builder pattern I would:

Remove the use of with_notify_period from here, and call it in service.rs (in init_sh_builder()), in case the CLI parameter was passed.

Do the same with with_max_storage_capacity, with_jump_capacity, with_retry_timeout, as they seem to be called in the same way, regarless of what the provider type and storage layer is. Well almost, in the User is different. But anyway, these tiny initialisation differences are exactly what the builder pattern is meant to be good for.

Anyway, I'd like to get @snowmead involved since he was heavily involved in this code.

undercover-cactus · 2024-11-21T11:23:50Z

node/src/tasks/msp_charge_fees.rs

+            .storage_hub_handler
+            .blockchain
+            .query_storage_provider_id(None)
+            .await?;


Shouldn't MSP task have access to their own msp id ?

It would be nicer to not have to do this, indeed.

But when starting up, the only known thing is the account - the provider id associated is known by the runtime.

undercover-cactus · 2024-11-21T15:10:12Z

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

+      }
+
+      // Verify that the MSP charged the users after the notified
+      await userApi.assert.eventPresent("paymentStreams", "PaymentStreamCharged");


We should probably verify the amount charged to make sure it is correct.

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

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

timbrinded · 2024-11-21T20:43:41Z

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

+    }
+
+    const txs = [];
+    for (let i = 0; i < source.length; i++) {


for readability this can just be for (const file of source) {

We use ´i´ as the index for the destination too. In order to use a for of loop we would need to modify a bit more.

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

timbrinded

Thanks for the updates, tests look lots better. Had some follow up comments but low priority

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

timbrinded · 2024-11-25T13:49:19Z

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

+      invariant(fileKeys2.length === 2, "Expected 2 file keys");
+
+      // Allow time for the MSP to update the local forest root
+      await sleep(3000);


Is there an associated log line to wait on?

timbrinded · 2024-11-25T13:49:48Z

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

+      await userApi.sealBlock(txs, shUser);
+
+      // Allow time for the MSP to receive and store the file from the user
+      await sleep(3000);


Might be better to wait for the log line: File upload complete

ffarall

Honestly great job! Just a handful of minor suggestions and comments.

ffarall · 2024-11-25T23:19:48Z

client/blockchain-service/src/handler.rs

+    fn check_for_notify(&self, block_number: &BlockNumber) {
+        if let Some(np) = self.notify_period {
+            if block_number % np == 0 {
+                self.emit(NotifyPeriod {});
+            }
+        }
+    }


This fits better in utils.rs

ffarall · 2024-11-25T23:23:18Z

node/src/cli.rs

@@ -109,6 +109,12 @@ pub struct ProviderConfigurations {
    /// Extrinsic retry timeout in seconds.
    #[clap(long, default_value = "60")]
    pub extrinsic_retry_timeout: u64,
+
+    /// MSP charging fees frequency.


You might want to add more info like the unit of this parameter. For example, is it in number of blocks? And if so, could be helpful to give some parameter like "Setting it to 600, with 6 seconds blocks, means charging every 1 hour".

Also, wouldn't this be a period instead of a frequency?

Yes period is better. I changed every 'freq' with 'period'.

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

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

ffarall · 2024-11-25T23:26:32Z

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

+      }
+
+      // Verify that the MSP charged the users after the notified
+      await userApi.assert.eventPresent("paymentStreams", "PaymentStreamCharged");


node/src/tasks/msp_charge_fees.rs

node/src/services/handler.rs

ffarall · 2024-11-25T23:51:13Z

node/src/tasks/msp_charge_fees.rs

+
+            match charging_result {
+                Ok(submitted_transaction) => {
+                    info!(target: LOG_TARGET, "Submitted extrinsic to charge users with debt: {}", submitted_transaction.hash());


We might want to make this a debug log.

ffarall · 2024-11-25T23:53:23Z

node/src/tasks/msp_charge_fees.rs

+            let charging_result = self
+                .storage_hub_handler
+                .blockchain
+                .send_extrinsic(call, Tip::from(0))


We may want to use watch_for_success here.

For BSP, we are not watching for success. Do we want to have it there too ?

ffarall · 2024-11-25T23:55:04Z

node/src/tasks/msp_charge_fees.rs

+use crate::tasks::{FileStorageT, MspForestStorageHandlerT, StorageHubHandler};
+
+const LOG_TARGET: &str = "msp-charge-fees-task";
+const MIN_DEBT: Balance = 0;


These constants and the growing number of configuration parameters for BSPs/MSPs makes it increasingly compelling to have a way to configure them through a configuration file (like a .yml/.toml or similar). If you feel like that's something you'd like to do, setup a task with @santikaplan.

added a period notify functionality

120b859

undercover-cactus commented Nov 21, 2024

View reviewed changes

undercover-cactus added 2 commits November 21, 2024 16:07

passing test

e18b537

fmt + lint

e8e5513

undercover-cactus commented Nov 21, 2024

View reviewed changes

undercover-cactus added 3 commits November 21, 2024 16:13

remove only true

ae0aa49

2 spaces indent

d7c3132

remove warning

d33576b

undercover-cactus requested review from ffarall, snowmead, links234 and timbrinded November 21, 2024 18:03

timbrinded reviewed Nov 21, 2024

View reviewed changes

undercover-cactus added 5 commits November 22, 2024 10:35

fix advaceToBlock

ad5dbcf

use invariant

583b1e7

fmt

e250967

remove the try and catch in the test

dde2b66

fmt

578d56c

undercover-cactus marked this pull request as ready for review November 25, 2024 12:18

undercover-cactus requested a review from timbrinded November 25, 2024 12:18

timbrinded reviewed Nov 25, 2024

View reviewed changes

ffarall requested changes Nov 25, 2024

View reviewed changes

undercover-cactus added 3 commits November 27, 2024 16:13

light modification in the test

476bc83

remove 'only:true'

9ffcc39

lint

9ba6ecb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add task to let MSP charge user every X blocks #268

feat: add task to let MSP charge user every X blocks #268

undercover-cactus commented Nov 21, 2024

undercover-cactus Nov 21, 2024

ffarall Nov 25, 2024

ffarall Nov 25, 2024

undercover-cactus Nov 21, 2024

links234 Nov 25, 2024

undercover-cactus Nov 21, 2024

ffarall Nov 25, 2024

timbrinded Nov 21, 2024

undercover-cactus Nov 22, 2024

timbrinded left a comment

timbrinded Nov 25, 2024

timbrinded Nov 25, 2024

ffarall left a comment

ffarall Nov 25, 2024

ffarall Nov 25, 2024

undercover-cactus Nov 27, 2024

ffarall Nov 25, 2024

ffarall Nov 25, 2024

ffarall Nov 25, 2024

undercover-cactus Nov 27, 2024

ffarall Nov 25, 2024

feat: add task to let MSP charge user every X blocks #268

Are you sure you want to change the base?

feat: add task to let MSP charge user every X blocks #268

Conversation

undercover-cactus commented Nov 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timbrinded left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffarall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment