implement BoundedEncodedLen #8720

coriolinus · 2021-05-04T08:57:35Z

Work moved from paritytech/parity-scale-codec#267

In support of #8719

bkchr · 2021-05-04T09:04:58Z

frame/support/src/traits/bounded_encoded_length.rs

+	};
+}
+
+impl_tuples!(A, B, C, D, E, F, G, H, I, J, K, L, M, N, O, P, Q, R);


Please use impl_for_tuples for this

Ok: 0204e2f.

frame/support/src/traits/bounded_encoded_length.rs

bkchr · 2021-05-04T09:25:52Z

frame/support/src/traits/bounded_encoded_length.rs

+		$(
+			impl BoundedEncodedLen for $t {
+				fn max_encoded_len() -> usize {
+					// a compact encoding of a primitive can take 1 byte more than its actual size


But a primitive will never be compact encoded automatically?

You would need to implement the trait for Compact<T>

Good point. d249f57

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

…ate into prgn-boundedencodedlen

kianenigma · 2021-05-04T12:22:49Z

frame/support/src/storage/bounded_vec.rs

+	S: Get<u32>,
+	BoundedVec<T, S>: Encode,
+{
+	fn max_encoded_len() -> usize {


the least significant comment: pity not to see SomeTrait { fn some_trait() }, why not trait MaxEncodedLen?

No reason really; I like MaxEncodedLen because it's shorter and agrees with the trait's function. af0b814

kianenigma · 2021-05-04T12:24:20Z

frame/support/src/traits/bounded_encoded_length.rs

+	T: BoundedEncodedLen,
+{
+	// The compact encoding of a type usually requires fewer bytes, but can occupy 1 additional
+	// byte in the worst case.


I'd be curious to know exactly which case takes 1 extra byte?

In general, our Compact encoding uses two bits to indicate how many bytes follow. Therefore, we need an extra byte for all cases where T::count_ones() > T::BIT_WIDTH - 2. u8 example.

2^14 takes 4 bytes to be encoded in a compact way whereas it takes only 2 bytes to be encoded as a regular u16

kianenigma

LGTM

frame/support/src/traits/bounded_encoded_length.rs

Turns out that u16 doesn't play nicely with the pattern; those values take two extra bytes, where all other cases take one. :(

gui1117 · 2021-05-04T13:11:31Z

frame/support/src/traits/bounded_encoded_length.rs

+	u64 => 9;
+	// https://github.com/paritytech/parity-scale-codec/blob/f0341dabb01aa9ff0548558abb6dcc5c31c669a1/src/compact.rs#L413
+	u128 => 17;
+);


we could add a test that ensures Compact(u8::max_value()).encode().len() is correctly 2 bytes etc... but anyway values are good to me.

Yes, agree. 8b8576d.

coriolinus · 2021-05-04T13:28:35Z

bot merge

ghost · 2021-05-04T13:28:40Z

Waiting for commit status.

athei · 2021-05-05T08:48:52Z

I like this. It should be feasible to write a derive macro in order to have this for composite data structures, right?

coriolinus · 2021-05-05T08:56:11Z

Yes, I'm working on the derive macro right now.

…

On Wed, May 5, 2021 at 10:49 AM Alexander Theißen ***@***.***> wrote: I like this. It should be feasible to write a derive macro in order to have this for composite data structures, right? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#8720 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB3V4TQCHDOZMM6KOTZDEBTTMEBATANCNFSM44CJRZJA> .

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

* implement BoundedEncodedLen * update header * update imports * use impl_for_tuples instead of a custom macro * remove redundant where clause Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * impl for Compact<T> * impl BoundedEncodedLen for BoundedVec (paritytech#8727) * impl BoundedEncodedLen for bool * explicitly implement BoundedEncodedLen for each Compact form Turns out that u16 doesn't play nicely with the pattern; those values take two extra bytes, where all other cases take one. :( * rename BoundedEncodedLen -> MaxEncodedLen * add tests of compact encoded length Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

implement BoundedEncodedLen

dff9c4a

coriolinus requested review from shawntabrizi and bkchr May 4, 2021 08:57

coriolinus self-assigned this May 4, 2021

github-actions bot added the A0-please_review Pull request needs code review. label May 4, 2021

coriolinus mentioned this pull request May 4, 2021

Implement BoundedEncodedLength trait and implementations on primitives paritytech/parity-scale-codec#267

Closed

coriolinus added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels May 4, 2021

coriolinus mentioned this pull request May 4, 2021

MaxEncodedLen tracking issue #8719

Closed

20 tasks

coriolinus added the D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. label May 4, 2021

update header

10d65a7

bkchr reviewed May 4, 2021

View reviewed changes

coriolinus added 2 commits May 4, 2021 11:05

update imports

5a8f30a

use impl_for_tuples instead of a custom macro

0204e2f

bkchr reviewed May 4, 2021

View reviewed changes

coriolinus and others added 3 commits May 4, 2021 11:31

remove redundant where clause

3703843

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

impl for Compact<T>

d249f57

Merge branch 'prgn-boundedencodedlen' of github.com:paritytech/substr…

91c31c5

…ate into prgn-boundedencodedlen

github-actions bot added the A7-needspolkadotpr label May 4, 2021

coriolinus removed the A7-needspolkadotpr label May 4, 2021

impl BoundedEncodedLen for BoundedVec (#8727)

13f22c0

kianenigma reviewed May 4, 2021

View reviewed changes

kianenigma approved these changes May 4, 2021

View reviewed changes

gui1117 reviewed May 4, 2021

View reviewed changes

frame/support/src/traits/bounded_encoded_length.rs Outdated Show resolved Hide resolved

frame/support/src/traits/bounded_encoded_length.rs Outdated Show resolved Hide resolved

coriolinus added 2 commits May 4, 2021 14:38

impl BoundedEncodedLen for bool

7d472bd

explicitly implement BoundedEncodedLen for each Compact form

f18183a

Turns out that u16 doesn't play nicely with the pattern; those values take two extra bytes, where all other cases take one. :(

gui1117 reviewed May 4, 2021

View reviewed changes

gui1117 approved these changes May 4, 2021

View reviewed changes

rename BoundedEncodedLen -> MaxEncodedLen

af0b814

add tests of compact encoded length

8b8576d

ghost merged commit 6bfdaba into master May 4, 2021

ghost deleted the prgn-boundedencodedlen branch May 4, 2021 13:57

athei mentioned this pull request Jun 8, 2021

contracts: Remove weight pre charging #8976

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement BoundedEncodedLen #8720

implement BoundedEncodedLen #8720

coriolinus commented May 4, 2021

bkchr May 4, 2021

coriolinus May 4, 2021

bkchr May 4, 2021

coriolinus May 4, 2021

kianenigma May 4, 2021

coriolinus May 4, 2021

kianenigma May 4, 2021

coriolinus May 4, 2021

gui1117 May 4, 2021

kianenigma left a comment

gui1117 May 4, 2021 •

edited

Loading

coriolinus May 4, 2021

coriolinus commented May 4, 2021

ghost commented May 4, 2021

athei commented May 5, 2021

coriolinus commented May 5, 2021 via email

implement BoundedEncodedLen #8720

implement BoundedEncodedLen #8720

Conversation

coriolinus commented May 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

gui1117 May 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coriolinus commented May 4, 2021

ghost commented May 4, 2021

athei commented May 5, 2021

coriolinus commented May 5, 2021 via email

gui1117 May 4, 2021 •

edited

Loading