paritytech / parity-scale-codec

Lightweight, efficient, binary serialization and deserialization codec
Apache License 2.0
247 stars 95 forks source link

Update MaxEncodedLen derive macro #512

Closed pgherveou closed 1 year ago

pgherveou commented 1 year ago

508 fixed the generated code for MaxEncoded derive trait

#[codec(compact)]
foo: u64

// compile to
// old:  u64::max_encoded_len()
// new: <::codec::Compact<u64> as ::codec::MaxEncodedLen>::max_encoded_len()

This is all fine but cause some compilation errors when this field is generic, since Rust can not tell that Compact<T> implements MaxEncodedLen when T implements it, without extra constraints

#[codec(compact)]
deposit: BalanceOf<T>,

// 53 |         pub deposit: BalanceOf<T, OldCurrency>,
//    |                      ^^^^^^^^^ the trait `parity_scale_codec::MaxEncodedLen` is not implemented for `parity_scale_codec::Compact<<OldCurrency as Currency<<T as frame_system::Config>::AccountId>>::Balance>`
koute commented 1 year ago

Also, a side note only tangentially related to this PR - when reviewing this code I've noticed that a compact u16's maximum encoded length is apparently 4 bytes!? That is strange, because for all of the other types it only takes at most one extra byte over the size of the type itself, and IIRC the bare minimum to represent a u16 in a space-efficient varint encoding should be 3 bytes (which can actually encode a number up to 21 bits, [edit]although, okay, this assumes that the encodings for different types are compatible and that the maximum size is 64-bit; with different constraints the maximum number of bits is going to be different[/edit]).

But apparently we kinda screwed this up, and that's how we encode u16s:

assert_eq!(Compact(u16::MAX).encode().len(), 4); // Doesn't panic!

Kinda unfortunate that we waste space here.

pgherveou commented 1 year ago

Wait, why is this adding a new function max_compact_encoded_len instead of fixing the impl of the max_encoded_len for the Compact wrapper?

the impl that we fixed previously #508 is not wrong, but it forces us to add extra constraint on the type when used with generics.

See https://github.com/paritytech/parity-scale-codec/pull/512#discussion_r1318437939

If you have a better solution, I'll take it.

bkchr commented 1 year ago

Pushed the proper solution. We add a new required trait for HasCompact::Type. But I doubt that someone implements this trait downstream.

We need to seal this trait.

pgherveou commented 1 year ago

lgtm too, checking if everything compile properly on polkadot-sdk and will merge this next and create a new release