Skip to content

Conversation

@L3pereira
Copy link
Contributor

funds_needed_for_upload(params: UploadParameters<T>) created in pallet storage
I think It's more advantageous to use UploadParameters as a parameter than to separate the fields into params.

This function calculates the storage fee and the net prize an actor has to pay if he wants to update X amount of data objects.
It only works on updates, so the net deletion prize is always positive (NetDeletionPrize::<T>::Pos(b)).

net_prize = number of objects to update * DataObjectDeletionPrizeValue.
storage_fee = DataObjectPerMegabyteFee * round_up_to_nearest_integer(size of total objects in bytes / 1_048_576)

funds_needed_for_upload = net_prize + storage_fee

@vercel
Copy link

vercel bot commented Apr 29, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated
pioneer-testnet ⬜️ Ignored (Inspect) May 4, 2022 at 11:14AM (UTC)

@L3pereira L3pereira requested a review from bedeho April 29, 2022 22:38
@L3pereira L3pereira self-assigned this Apr 29, 2022
Copy link
Member

@bedeho bedeho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation is doing too much. You should not be checking and verifying lots of checks that are not relevant for fee estimation, those will be checked later as part of actual call to upload. Checking for existance of bag is OK, since we need contained informatino to do actual evaluation, but the other stuff is not.

let storage_fee = Self::calculate_data_storage_fee(new_voucher_update.objects_total_size);
//Only the case where the net deletion prize (NDP) is positive will be returned, as the
//function will be used to validate whether the actor has enough balance to update the assets.
let funds_needed_for_upload = match net_prize.add_balance(storage_fee) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't net_prize include old deletin prize? If so, wouldn't funds_needed_for_upload in fact be the new total cost, rather than cost of doing this additional upload, which is what we care about?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we call upload_data_objects this function will call try_mutating_storage_state with
BagOperationParams::Update(object_creation_list)
then

  1. we will ensure bag exists and bag has a deletion prize, that is some for dynamic bags or none for static ones.
  2. Construct objects to upload where deletion prize is the old one
  3. we compute net prize init_value + (deletion_prize * num_obj_to_create)
  4. We add net_prize to storage_fee

So net prize include deletion prize in the calculation, but that is what try_mutating_storage_state is doing to do the ensure_funds validation
I think the goal here is to separate funds validation from try_mutate_storage, right?

@L3pereira L3pereira changed the title Check price of data object bloat bonds #3607 Check price of data object bloat bonds May 3, 2022
Check price of data object bloat bonds Joystream#3607

funds_needed_for_upload Joystream#3607
@L3pereira L3pereira force-pushed the check_price_data_object_bloat_bonds_#3607 branch from 2128411 to b2e396c Compare May 4, 2022 11:13
@L3pereira
Copy link
Contributor Author

L3pereira commented May 4, 2022

Now we have a much simpler funds calculation function. This function doesn't fail.
I didn't check for bag existence, let's move that responsibility somewhere else, since the main goal of this function is calculating funds.
The signature is very simple

    fn funds_needed_for_upload(
        num_of_objs_to_upload: usize,
        objs_total_size_in_bytes: u64,
    ) -> BalanceOf<T>;

I tested for overflow saturation, but I'm thinking now, these numbers are so huge, that no quantity of memory will ever amount to that.
This function is will roundup to the next greater integer when objs_total_size_in_bytes % 1048576 (1Mb) > 0

I also move to fixtures some helper functions, that were in the middle of the unit tests.

@L3pereira L3pereira requested a review from bedeho May 4, 2022 11:37
Copy link
Member

@bedeho bedeho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very simple and correct, originally, this is what I thought we would need, something very very simple. Why were you able to simplify so much only now?

@L3pereira
Copy link
Contributor Author

I thought your goal was to separate the try_mutate in such a way we would have validation and then the mutation safe parts for the content pallet, and the funds function would have those validations, But for now, you just want a calculator.

@bedeho bedeho merged commit fb0c7cd into Joystream:audit3 May 5, 2022
@L3pereira L3pereira deleted the check_price_data_object_bloat_bonds_#3607 branch July 29, 2022 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants