Migrate to Weights V2#905
Conversation
akhilesharora
added
crcl-runtime
akhilesharora
force-pushed
the
parachain
branch
from
bc3a515 to
6bac6ba
Compare
mustermeiszer
left a comment
mustermeiszer
left a comment
There was a problem hiding this comment.
One unwrap() and castings neeed to be handled. Also, I think the MaxBound is to small
|
|
||
| let fee = base_fee | ||
| .checked_mul(&pallet_fees::BalanceOf::<T>::from(multiplier)) | ||
| .checked_mul(&pallet_fees::BalanceOf::<T>::from(multiplier as u8)) |
There was a problem hiding this comment.
Why is the as u8 needed now? I would like to use a try_into().ok_or(ArithmethicError::Overflow) here, as u8 is really small to cast to.
There was a problem hiding this comment.
Ah sure, well while testing it was complaining about the types. But indeed, I was thinking along the same lines as u8 is really small to cast to.
| .ok_or(ArithmeticError::Overflow) | ||
| }) | ||
| .and_then(|put_into_bucket| Ok(T::BlockNumber::from(put_into_bucket))) | ||
| .and_then(|put_into_bucket| Ok(T::BlockNumber::from(put_into_bucket as u8))) |
| let val: BoundedVec<u8, T::MaxBound> = | ||
| (child::root(&key, StateVersion::V0)).try_into().unwrap(); |
There was a problem hiding this comment.
unwrap() should be handled
|
|
||
| // Parameterize anchors pallet | ||
| parameter_types! { | ||
| pub const MaxBound: u32 = 100; |
There was a problem hiding this comment.
I think the child root is a H256, so 256 bytes. The bound should allow this size if I am not mistaken. WDYT? @branan
| type WeightInfo: WeightInfo; | ||
|
|
||
| #[pallet::constant] | ||
| type MaxBound: Get<u32>; |
There was a problem hiding this comment.
Maybe this name can be renamed to something more relevant for the runtime configurator in order to know what is exactly this bound for. Something like MaxRootOfChildTrieSize.
Because our Vec<u8> is storing the child root (docs say that the hashing algorithm is defined by the Block), seems like its size is something known. I mean, instead of passing a new argument, could we extract it from the current hashing algorithm used?
There was a problem hiding this comment.
@mustermeiszer I just see your comment below regarding the size, what do you think about this? Could it be possible?
There was a problem hiding this comment.
Thinking more about it, I think something like the following should work as a bound type for BoundedVec.
ConstU32<sp_std::mem::size_of(T::Hash)>There was a problem hiding this comment.
Love the idea, but is is defined not by T::Hash, but by the type Hash of trait Header as the externalities hasher define this hash type. The Output is currently the same: sp_core::H256. But just to be sure.
|
Closed in favor of #980 |
3 participants
As @offerijns already mentioned this issue, storage migrations from Vec to BoundedVec for few of the remaining pallets as per Weights V2.