Removes `without_storage_info` macro from pallets by mustermeiszer · Pull Request #1122 · centrifuge/centrifuge-chain

mustermeiszer · 2022-12-06T12:10:03Z

Description

In order to migrate to frame weights v2 we need to get rid of the without_storage_info macro on our pallets. In order to do this, we must implement MaxEncodedLen on 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 BoundedVec max length is below the current length of its counterpart vector. This is the case for us!

Closes #1121

Changes and Descriptions

Remove macro on all pallets
Adapt storage types to implement MaxEncodedLen - mostly deriving it automatically
- Two manual implementations were necessary due to missing MEL impls for three imported enums: pool_system::pool_types::PoolChanges and connectors::routers::XcmDomain
- For PoolChanges, I opened an ORML upstream PR. In the future, we can remove that custom impl then.
Convert all Vecs in any storage to their BoundedVec variants.
- Turns out that bounding their length by MaxTranches seems sufficient for all new BoundedVec

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)

Checklist:

I have added Rust doc comments to structs, enums, traits and functions
~~- [ ] I have made corresponding changes to the documentation~~
I have performed a self-review of my code
My changes generate no new warnings (AFAICT)
~~- [ ] I have added tests that prove my fix is effective or that my feature works~~
New and existing unit tests pass locally with my changes
~~ - [ ] Any dependent changes have been merged and published in downstream modules~~
I rebased on the latest main branch

mustermeiszer · 2022-12-06T12:10:56Z

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 ^^

wischli · 2023-01-04T08:28:47Z

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 ?

…rage-info

wischli · 2023-01-10T14:29:44Z

@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.

mustermeiszer

Looks all straight forward. Thanks for picking this up.

I still need to take a look tomorrow at the custom impl in the connectors router.

Looks good, but am on the phone and can not look at the deps.

mustermeiszer · 2023-01-11T14:23:43Z

@wischli i will only have time for this tomorrow. So if you wanna merge this up today, rather ping someone else additionally.

wischli · 2023-01-11T14:25:26Z

@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.

NunoAlexandre

@wischli Looks good overall but two mains points:

Let's not abbreviate MaxEncodedLength in the code comments as MEL since it's not a thing and thee's no need to introduce that abbreviation
I don't think the custom impl for XcmDomain is correct because it just sums 2 as the max_encoded_length of the VersionedMultiLocation because of it having two variants (V1 and V2) when this value can hold an arbitrarily long value.

…rage-info

mustermeiszer

Re: Regarding custom impl of MaxEncodedLen. Need feedback from @wischli and @NunoAlexandre here.

mustermeiszer · 2023-01-12T08:18:01Z

+		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())


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::MultiLocation being a struct holding a u8 and essentially v0::MultiLocation

v1::Junction removing 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?

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.

Help me understand 😆 🙏

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 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.

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().

Oh I see, now I get it 🙏 thanks! Looks good then :)

NunoAlexandre

Sweet 🍯 solid work and thanks for the clarifications

WIP: Removed macro, fixing max encoded len info missing

d0ecc24

mustermeiszer added the D0-ready Pull request can be merged without special precaution and notification. label Dec 6, 2022

Merge branch 'main' into feature/remove-without-storage-info

54a3975

wip: derive MaxEncodedLen

6b350a5

wischli self-assigned this Jan 5, 2023

wischli added 6 commits January 6, 2023 15:28

feat: bound pool-system

9619e9b

fix: make it compile for default features

c759b76

tests: fix

cfb546b

tests: more minor fixes

b0db81e

Merge remote-tracking branch 'origin' into feature/remove-without-sto…

aa226aa

…rage-info

docs: improve comments

6e89ec1

wischli marked this pull request as ready for review January 10, 2023 12:48

docs: add upstream PR

c03454d

wischli requested review from NunoAlexandre, hieronx and mikiquantum January 10, 2023 14:20

mustermeiszer requested a review from branan January 11, 2023 14:19

mustermeiszer commented Jan 11, 2023

View reviewed changes

NunoAlexandre reviewed Jan 11, 2023

View reviewed changes

Comment thread pallets/connectors/src/routers.rs Outdated

Comment thread pallets/connectors/src/routers.rs Outdated

Comment thread pallets/connectors/src/routers.rs Outdated

wischli added 4 commits January 11, 2023 19:15

docs: improve for custom mel bound derivation

839c04c

docs: add missing max nav age comment

c6d955f

Merge remote-tracking branch 'origin' into feature/remove-without-sto…

9f174ce

…rage-info

fix: registry mock mel

5dfef10

mustermeiszer commented Jan 12, 2023

View reviewed changes

fix: final max encoded len for XcmDomain

921d7d7

wischli requested a review from NunoAlexandre January 12, 2023 11:05

NunoAlexandre approved these changes Jan 12, 2023

View reviewed changes

wischli merged commit c73c7d0 into main Jan 12, 2023

NunoAlexandre deleted the feature/remove-without-storage-info branch January 12, 2023 13:56

Uh oh!

Conversation

mustermeiszer commented Dec 6, 2022 • edited by wischli Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes and Descriptions

Type of change

Checklist:

Uh oh!

mustermeiszer commented Dec 6, 2022

Uh oh!

wischli commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wischli commented Jan 10, 2023

Uh oh!

mustermeiszer left a comment

Choose a reason for hiding this comment

Uh oh!

mustermeiszer commented Jan 11, 2023

Uh oh!

wischli commented Jan 11, 2023

Uh oh!

NunoAlexandre left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mustermeiszer left a comment

Choose a reason for hiding this comment

Uh oh!

mustermeiszer Jan 12, 2023

Choose a reason for hiding this comment

Uh oh!

NunoAlexandre Jan 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wischli Jan 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NunoAlexandre Jan 12, 2023

Choose a reason for hiding this comment

Uh oh!

NunoAlexandre left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mustermeiszer commented Dec 6, 2022 •

edited by wischli

Loading

wischli commented Jan 4, 2023 •

edited

Loading

NunoAlexandre left a comment •

edited

Loading

NunoAlexandre Jan 12, 2023 •

edited

Loading

wischli Jan 12, 2023 •

edited

Loading