Data Preservers Profiles #551
Conversation
nanocryk
added
a0-pleasereview
|
hey @nanocryk please make the CI pass while we review this |
|
Can we include a migration creating a profile for all existing bootnodes? I feel this would reduce the complexity in the follow-up PRs as you already have the correct data in storage |
Coverage Report@@ Coverage Diff @@
## master jeremy-data-preservers-profiles +/- ##
===================================================================
+ Coverage 69.33% 69.38% +0.05%
Files 221 221
+ Lines 38618 38883 +265
===================================================================
+ Hits 26775 26978 +203
+ Misses 11843 11905 +62
|
We'll add it in the PR that'll add assignements and will effectively replace |
| ); | ||
|
|
||
| // Update deposit | ||
| let new_deposit = T::ProfileDeposit::profile_deposit(&profile)?; |
There was a problem hiding this comment.
Why does the deposit depend on the profile instead of being fixed to the worst case?
There was a problem hiding this comment.
Both can work. You can also impl this trait ignoring the profile and returning a const worst case cost :) This approach in the pallet is more versatile.
There was a problem hiding this comment.
I like that we adjust it to whatever profile is being created tbh. It's definitely not the same to create a profile where you inject 100 blacklisted para-ids than one where you insert none
There was a problem hiding this comment.
Yeah but what is the max difference going to be? 1000 bytes? How expensive is a deposit for that? My point is that I don't like to be handling dust on every profile update.
There was a problem hiding this comment.
how are you handling dust? you are just either releasing some amount or holding more tokens. And it's the user paying for it..
There was a problem hiding this comment.
I mean that the difference in deposit is so small that it doesn't matter, but I'm just guessing, maybe deposits are more expensive than I think.
pallets/data-preservers/src/lib.rs
Outdated
| #[scale_info(skip_type_params(T))] | ||
| pub struct Profile<T: Config> { | ||
| pub url: BoundedVec<u8, T::MaxBootNodeUrlLen>, | ||
| pub limited_to_para_ids: Option<BoundedVec<ParaId, T::MaxParaIdsVecLen>>, |
There was a problem hiding this comment.
It would also be interesting to allow blacklisting some para ids, I imagine the case of a chain that is very active so this data preserver doesn't want to handle all the load.
There was a problem hiding this comment.
so you want to have the choice between whitelisting and blacklisting?
There was a problem hiding this comment.
I guess it's not a bad idea and it does not cost much to add it
There was a problem hiding this comment.
Dont forget to update the deposit as we have new bytes now
There was a problem hiding this comment.
Deposit is computed from size so it should already take into account the change.
| fn profile_deposit(profile: &Profile) -> Result<Balance, DispatchErrorWithPostInfo>; | ||
| } | ||
|
|
||
| pub struct BytesProfileDeposit<BaseCost, ByteCost>(PhantomData<(BaseCost, ByteCost)>); |
There was a problem hiding this comment.
I think this is not a bad idea to be honest, but it's the first time that I see it in the pallet haha. I like it but since it's a configuration option maybe we can move it to a different file?
There was a problem hiding this comment.
Also let's add some comments to it
| @@ -55,5 +76,209 @@ mod benchmarks { | |||
| assert_eq!(Pallet::<T>::boot_nodes(para_id), boot_nodes); | |||
| } | |||
|
|
|||
| impl_benchmark_test_suite!(Pallet, crate::mock::new_test_ext(), crate::mock::Test); | |||
| #[benchmark] | |||
| fn create_profile(x: Linear<1, 200>, y: Linear<1, 10>) { | |||
There was a problem hiding this comment.
According to the weights, none of the benchmarks depend on x or y, that's great!
There was a problem hiding this comment.
In the runtime weigts it seems to use x and y but with very small multipliers. Should we simplify and remove those parameters?
There was a problem hiding this comment.
I would leave it, or maybe use one param which is "profile size", but it's fine
Allow users to register profile for their data provider nodes. Profiles can be forcefully managed by origins configured in the runtime.
Later PRs will replace the current
BootNodesstorage with assignements from chain managers, both in a forced/free manner and a paid manner (which will add in profiles the payment plan required by the provider).