Move MaxEncodedLen from Substrate

[coriolinus]

After further discussion, it turns out that we want the MaxEncodedLen trait and derive macro in this crate after all instead of in Substrate. This PR forms part 1 of a three-step process:

• Add MaxEncodedLen to parity-scale-codec
• Drop a new release of parity-scale-codec
• Update Substrate:
  • upgrade parity-scale-codec dependency
  • update all references from frame_support::MaxEncodedLen to codec::MaxEncodedLen
  • remove frame_support::traits::MaxEncodedLen and frame_support_procedural::MaxEncodedLen

Part of paritytech/substrate#8719.

[shawntabrizi]

are we ready to move it?

I wanted to actually have it live in substrate a bit longer while we migrate pallets to use MaxEncodedLen.

Then if everything looks good, we can move it to SCALE. Otherwise, we may have to sync and bump substrate multiple times with SCALE to make it work.

[coriolinus]

I think we're ready, on the basis that the trait itself hasn't changed pretty much since we wrote the tracking issue, and I don't anticipate it changing much in the future. Still, if you like, we can put this on hold for a while.

[bkchr]

As @shawntabrizi said, we should wait until benchmarks etc are done in Substrate. Then we can move this. Otherwise we may need to release a new major version to break the trait definition.

[Xanewok]

we should wait until benchmarks etc are done in Substrate.

Is this done and/or is this blocked on anything else?

We're looking at major releases in paritytech/substrate#9140 anyway but it'd be good if we can make it without releasing an additional crate if this will be moved into parity-scale-codec anyway.

[Xanewok]

I took the liberty to resolve the conflict via web UI, I hope you don't mind.

[coriolinus]

Of course, no problem, thanks! If we're going to move forward on this, I'll take a look in depth to ensure that nothing substantial needs updating. However, I want to ensure that there are no further reservations about moving the crates into this repo before putting further work into this PR.

…

On Mon, Jun 21, 2021 at 11:29 AM Igor Matuszewski ***@***.***> wrote: I took the liberty to resolve the conflict via web UI, I hope you don't mind. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#268 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB3V4TVDI5EBYVENA34PBBTTT4A7HANCNFSM44RJYQHQ> .

-- Wichtige Mitteilung Diese Mitteilung wurde von Parity Technologies Deutschland GmbH kommuniziert, eine im Handelsregister des Amtsgerichtes Charlottenburg unter HRB 190583 B registrierte Gesellschaft mit beschränkter Haftung (GmbH). Die Geschäftsführerin der GmbH ist Frau Dr. Jutta Steiner. Der registrierte Geschäftssitz ist Glogauer Straße 6 in 10999 Berlin, Deutschland.  Diese Mitteilung enthält Informationen welche vertraulich sind und welche eventuell die Vertraulichkeit der Rechtsberatung ("Anwaltsgeheimnis") berühren. Sie ist ausschließlich für den/die vorgesehenen Empfänger bestimmt. Wenn Sie nicht der/die beabsichtigte(n) Empfänger sind, benachrichtigen Sie bitte ***@***.*** ***@***.***> und löschen Sie diese Nachricht sofort. Unsere Datenschutzrichtlinie, einschließlich die Art und den Umfang von personenbezogenen Daten, die wir erfassen, wie wir diese Daten erfassen und verarbeiten, an wen wir sie in Bezug auf die von uns angebotenen Dienste weitergeben dürfen, sowie bestimmte Rechte und Optionen, die Sie in dieser Hinsicht haben, finden Sie unter: https://www.parity.io/privacy/ <https://www.parity.io/privacy/>

[Xanewok]

Is this a whitespace/line ending issue? I'm re-running the test suite on Linux locally and it seems to pass but CI is grumpy for whatever reason.

test tests/max_encoded_len_ui/union.rs ... mismatch

EXPECTED:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
error: Union types are not supported
 --> $DIR/union.rs:4:1
  |
4 | union Union {
  | ^^^^^

error: Union types are not supported.
 --> $DIR/union.rs:4:1
  |
4 | union Union {
  | ^^^^^
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈

ACTUAL OUTPUT:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
error: Union types are not supported.
 --> $DIR/union.rs:4:1
  |
4 | union Union {
  | ^^^^^

error: Union types are not supported
 --> $DIR/union.rs:4:1
  |
4 | union Union {
  | ^^^^^
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈

[coriolinus]

Huh, weird. I'm not sure. I'm increasingly of the opinion that the macro ui tests are just flaky and should not be required in order to merge.

[Xanewok]

Oh, I just noticed the dot reordering... that's annoying. Updated to latest stable locally and reproduced it, ran with TRYBUILD=overwrite and locally it's looking good for now.

[bkchr]

Still misses the version bumps

[Xanewok]

Wanted to do a version bump in a separate PR I can do that here as well if you want me to.

I'd be inclined to publish that as a pre-release in order to progress with the release and prune max-encoded-len from the Substrate tree and once the dust settles, we can go ahead with the full release. WDYT? I'd be happy to merge, tag and publish this if you think that's a sound idea.

[bkchr]

Yeah fine

[coriolinus]

Great! I'll ensure that this has all the updates from Substrate, then merge.

[coriolinus]

Brought in the changes from the substrate version in 9f24e9d.

Note: the substrate implementation added H160, H256, H512 to the impl_primitives list. Those are substrate types, so didn't depend on Substrate to retain that implementation. Instead, the Substrate companion which switches dependencies to this crate will also need to re-implement those derives.

[coriolinus]

@ordian yes, that's the substrate companion.

[coriolinus]

@bkchr 395dbaa rewrites the code which fetches the max_encoded_len crate entirely. It moves the code which gets the crate into utils.rs, so that in principle Encode, Decode could easily use it as well, though I haven't actually implemented it for either of those derive macros. I believe it to satisfy all of your comments above.

[bkchr]

Ty. Much simpler and way more easier to read,

[Xanewok]

Released appropriate v2.2.0-rc.1 versions that contain this change and tagged the latest master. Thanks!