Free calls pallet #181
Free calls pallet #181
Conversation
F3Joule
requested review from
F3Joule and
siman
and removed request for
F3Joule
Co-authored-by: Vlad Proshchavaiev <[email protected]>
Co-authored-by: Vlad Proshchavaiev <[email protected]>
TarekkMA
force-pushed
the
tarekkma/free-calls
branch
from
816a306 to
d236787
Compare
| ( | ||
| total_weight, | ||
| boxed_call_info.class, | ||
| Pays::No, | ||
| ) |
There was a problem hiding this comment.
please put a note that you have a signed extension doing a pre-validation check here
pallets/free-calls/src/lib.rs
Outdated
| actual_weight = actual_weight.saturating_add(call.get_dispatch_info().weight); | ||
|
|
||
| // Dispatch the call | ||
| let result = call.dispatch(origin); |
There was a problem hiding this comment.
this doesn't make sense to me. You are just doing the same math here as you did above. you will get the same results.
what you want to do is actually get the result from let result = call.dispatch(origin); and from that get the real weight, and use that for the math
There was a problem hiding this comment.
There was a problem hiding this comment.
Right, that makes more sense. Thank you.
pallets/free-calls/src/lib.rs
Outdated
| /// Determine if `consumer` can have a free call and optionally update its window usage. | ||
| /// | ||
| /// Window usage for the caller `consumer` will only update if there is a quota and all of the | ||
| /// previous window usages doesn't exceed the defined windows config. | ||
| pub fn can_make_free_call(consumer: &T::AccountId, should_update_consumer_stats: ShouldUpdateConsumerStats) -> bool { |
There was a problem hiding this comment.
i generally do not like combining a check function with also a state modification function, but that is just up to you
There was a problem hiding this comment.
Yeah I planned to refactor this, which will make it easier for tests, and also it will be less complex.
pallets/free-calls/src/lib.rs
Outdated
| _ => return false, | ||
| }; | ||
|
|
||
| let mut windows: Vec<Window<T>> = Vec::new(); |
There was a problem hiding this comment.
you know the size of this vec will be at most WindowType or something, so you should create this vec with the known bound so it doesnt reallocate
There was a problem hiding this comment.
Ideally you use bounded vecs everywhere for this kind of thing
There was a problem hiding this comment.
Now all vecs are bounded to the size of the passed in config.
/// A `BoundedVec` that can hold a list of `ConsumerStats` objects bounded by the size of WindowConfigs.
pub type ConsumerStatsVec<T> = BoundedVec<ConsumerStats<<T as frame_system::Config>::BlockNumber>, WindowsConfigSize<T>>;
/// Retrieves the size of `T::WindowsConfig` to be used for `BoundedVec` declaration.
pub struct WindowsConfigSize<T: Config>(PhantomData<T>);
impl<T: Config> Default for WindowsConfigSize<T> {
fn default() -> Self {
Self(PhantomData)
}
}
impl<T: Config> Get<u32> for WindowsConfigSize<T> {
fn get() -> u32 {
T::WindowsConfig::get().len().try_into().unwrap()
}
}
pallets/free-calls/src/lib.rs
Outdated
| WindowType, | ||
| ConsumerStats<T::BlockNumber>, | ||
| >; |
There was a problem hiding this comment.
using a double map here does not seem as efficient as a bounded vec.
from what i read, you are updating all the windows each time a tx is going though, so instead of modifying one storage item, you are modifying N storage items.
TarekkMA
force-pushed
the
tarekkma/free-calls
branch
from
23a2c6e to
955c734
Compare
TarekkMA
force-pushed
the
tarekkma/free-calls
branch
from
955c734 to
9979464
Compare
|
This PR is obsolete, it's now split into 4 PRs
|
Rename branch from #177