Removes without_storage_info macro from pallets
#1122
Conversation
mustermeiszer
added
the
D0-ready
|
Just started this, but adding max encode len derivation is more cumbersome and time consuming than I thought. Leaving this here for the future. Me or others ^^ |
I was thinking about picking this up as it affects all pallets which helps me to get an overview. I also did this last year elsewhere. Definitely cumbersome 🫠 WDYT @mustermeiszer ? |
|
@mustermeiszer Not sure if you want to take a quick look. Can't request a review as its your PR :) Apart from two custom impls (see PR description), mainly migrating Vecs. |
|
@wischli i will only have time for this tomorrow. So if you wanna merge this up today, rather ping someone else additionally. |
No worries, it can wait until EOW for sure. I imagine you have bigger fish to fry. |
There was a problem hiding this comment.
@wischli Looks good overall but two mains points:
-
Let's not abbreviate
MaxEncodedLengthin the code comments asMELsince it's not a thing and thee's no need to introduce that abbreviation -
I don't think the custom impl for
XcmDomainis correct because it just sums2as the max_encoded_length of theVersionedMultiLocationbecause of it having two variants (V1andV2) when this value can hold an arbitrarily long value.
There was a problem hiding this comment.
Re: Regarding custom impl of MaxEncodedLen. Need feedback from @wischli and @NunoAlexandre here.
pallets/connectors/src/routers.rs
Outdated
| xcm::v1::MultiLocation::max_encoded_len() | ||
| // From the two enum variants of `VersionedMultiLocation | ||
| .saturating_add(2) | ||
| // The ethereum xcm call index (default bound) | ||
| .saturating_add(BoundedVec::< | ||
| u8, | ||
| ConstU32<{ xcm_primitives::MAX_ETHEREUM_XCM_INPUT_SIZE }>, | ||
| >::max_encoded_len()) | ||
| // The contract address (default bound) | ||
| .saturating_add(H160::max_encoded_len()) | ||
| // The fee currency (custom bound) | ||
| .saturating_add(cfg_types::tokens::CurrencyId::max_encoded_len()) |
There was a problem hiding this comment.
TBH, I have no clue how the MaxEncodedLen is derived. For an enum I would guess, assuming an enum has less then 256 variants, it would be 1 bytes + size of largest enum variant?
As the differences between v0 and v1 are basically
v1::MultiLocationbeing a struct holding au8and essentiallyv0::MultiLocationv1::Junctionremoving the named variant (Parent) and all other items being equal
I would say the implementation is correct. WDYT @NunoAlexandre?
The only thing I don't understand is why we need to add 2, wouldn't it be 1 as in 1 byte for indicating the variant number in the codec?
There was a problem hiding this comment.
Maybe I don't understand MaxEncodedLength but I thought its point was to capture the max length that a value of a specific type could take; If that is correct, then a VersionedMultiLocation will not be at most 2 bytes, it can in fact be arbitrarily long and that's probably why MaxEncodedLength is not implemented for that type.
There was a problem hiding this comment.
The only thing I don't understand is why we need to add 2, wouldn't it be 1 as in 1 byte for indicating the variant number in the codec?
Yes, you are right. I mixed that up with something else. Validated this with a small example:
#[derive(codec::Encode, codec::MaxEncodedLen)]
pub enum TestEnum<T: MaxEncodedLen> {
A(T),
B(T),
C(T),
D(T),
E(T),
}
#[test]
assert_eq!(TestEnum::<u32>::max_encoded_len(), u32::max_encoded_len() + 1);Maybe I don't understand
MaxEncodedLengthbut I thought its point was to capture the max length that a value of a specific type could take; If that is correct, then aVersionedMultiLocationwill not be at most 2 bytes, it can in fact be arbitrarily long and that's probably whyMaxEncodedLengthis not implemented for that type.
MaxEncodingLen aims to provide an estimate to the maximum encoded size of an arbitrary storage. So even though a u128 could have length 1, its max encoded length would be 17. Some more context can be found in the initial PR which introduced it.
Unfortunately, VersionedMultiLocation does not impl MaxEncodedLen.
pub enum VersionedMultiLocation {
V0(v0::MultiLocation),
V1(v1::MultiLocation),
}Luckily, v1::MultiLocation does! Since v1 is v0 with an extra field as pointed out above, v1 should be fine for the MaxEncodedLen plus the encoding size of an enum which is one.
Therefore, it should be correct to use v1::Multilocation::max_encoding_len().saturating_add(1) for VersionedMultiLocation::max_encoded_len().
There was a problem hiding this comment.
Oh I see, now I get it 🙏 thanks! Looks good then :)
Description
In order to migrate to frame weights v2 we need to get rid of the
without_storage_infomacro on our pallets. In order to do this, we must implementMaxEncodedLenon ALL storage items. This PR accomplishes that goal.As a result, once weights v2 are supported, the PoV size can be estimated by using the max encoded lengths (:= MEL) of changed storage items. Luckily, this conversion does not require a migration as long as
BoundedVecmax length is below the current length of its counterpart vector. This is the case for us!Closes #1121
Changes and Descriptions
MaxEncodedLen- mostly deriving it automaticallyPoolChanges, I opened an ORML upstream PR. In the future, we can remove that custom impl then.Vecs in any storage to theirBoundedVecvariants.MaxTranchesseems sufficient for all newBoundedVecType of change
Please delete options that are not relevant.
Checklist:
- [ ] I have made corresponding changes to the documentation- [ ] I have added tests that prove my fix is effective or that my feature works~~ - [ ] Any dependent changes have been merged and published in downstream modules~~
mainbranch