Perbill and other should be decode only valid instance.

[gui1117]

Currenctly decoding a 2 billion value doesn't return error

Perbill::decode(encode(2*billion)).unwrap().deconstruct() == 2*billion

I think as the doc state

A fixed point representation of a number between in the range [0, 1].

We must actually return error if value is more than one billion.

(and same for other perthings)

cc @kianenigma

[gui1117]

If we look at the size of the compact encoding of perthings here is the result:

Percent:
0-63: 1 byte
64-100 2 byte
so performance are worse for 36% of values

PerU16:
0-63: 1 byte
64-16_383: 2 byte
16_383-65_535 (24%-100%): 4 byte
so performance are worse for 75% of values

Permill:
0-63: 1 byte
64-16_383: 2 byte
16_383-Million (1.6%-100%): 4 byte
so performance are improved only for 1.6% smallest values

Perbill:
0-63: 1 byte
64-16_383: 2 byte
16_383-billion (0.0016%-100%): 4 byte
so performance are improved only for 0.0016% smallest values

Perquinntill:
0-63: 1 byte
64-16_383: 2 byte
16_383-1_073_741_823: 4 byte
...
Quinntil/10-Quinntill (10%-100%): 9 byte
so performance is worse for 90% of values (by one byte though).

Considering these result I think the compact representation for perthing doesn't really make sense. My opinion is to remove it.
Though it would be great to have codec supporting having a compact which is subset of basics integer types.

[kianenigma]

How does compact encoding exactly work, for example can you elaborate more on this comment:

PerU16:
0-63: 1 bit
64-16_383: 2bit
16_383-65_535 (24%-100%): 4bit
so performance are worse for 75% of values

Also bringing this issue generally up as it seem to have been forgotten but it is important to resolve.

[gui1117]

ah I wrote bit but it is byte.

• the integer 0 to the interger 63 is encoded into one byte,
• the integer 64 to 16_383 is encoded into 2 byte,
• the integer 16_383 to 65_535 is encoded in 4 byte,

the general doc is https://substrate.dev/docs/en/knowledgebase/advanced/codec

The issue needs a new major version from parity-scale-codec, though we thought would be good to include paritytech/parity-scale-codec#208
At some point having a default skip implementation that just decode the value should be enough, this would allow to implement the optimization later if wanted.

[gui1117]

at the same time we could remove compact encoding of perthings for substrate 2.0 ?

[kianenigma]

Okay now it is clear.

But as far as I can see if just create a percent and encode it, you will use the fixe-width encoding. Only if you wrap it inside a compact struct etc. we will use the compact encoding.

I think this is perfectly fine. The user should be aware of this and make sure they use compact only it makes sense.

---

Another note is: #6720

Here we manually wrapped u32 and u16 values in compact. But based on your info, for u16 it is most often not really worth being encoded as compact. The PR overall did improve the size of the solution struct, but that was probably most often because of u32 being optimised.

[gui1117]

indeed the only worry for me is when used in dispatchable like this:

decl_module! {
....
    fn transfert_some(origin, #[compact] p: Perbill) { ... }
}

Here such code allow the caller to give a Perbill which is more than 1.

[kianenigma]

indeed the only worry for me is when used in dispatchable like this:

decl_module! {

....

    fn transfert_some(origin, #[compact] p: Perbill) { ... }

}

Here such code allow the caller to give a Perbill which is more than 1.

Yes I think the best and only thing to do is to document this in Perbill and all the others that they may bot be worth being used with #[compact]. Not much more that we can do about it.

[xlc]

indeed the only worry for me is when used in dispatchable like this:

decl_module! {
....
    fn transfert_some(origin, #[compact] p: Perbill) { ... }
}

Here such code allow the caller to give a Perbill which is more than 1.

The decode can still fail and make the transaction invalid.

[gui1117]

The decode can still fail and make the transaction invalid.

with current parity scale codec it doesn't seems doable.

Another idea could be to just convert to max when decoding a compact encoding of a value beyond the maximum allowed.
I.e. we implement CompactAs trait by hand like this:

impl CompactAs for Perbill {
	type As = u64;
	fn encode_as(&self) -> &u64{
		self.deconstruct()
	}
	fn decode_from(x: u64) -> Perbill {
		Perbill { x.max(BILLION) }
	}
}
                                              
impl From<Compact<Perbill>> for Perbill {
	fn from(x: Compact<Perbill>) -> Perbill {
		Perbill(x.deconstruct().max(BILLION))
	}
}

There is still an issue if user do Compact::<Perbill>::decode(..).0 because the resulting Perbill can be more than 1.
I think this is hacky and would be in favor of removing Compact encoding altogether.

(If user still want to use this specific encoding, then we can implement Encode/Decode on a new struct CompactPerbill and implement From/Into.)

[kianenigma]

I thought this issue is just about the efficiency of compact usage, I forgot the initial point that decoding can be corrupt.

I am sure that we don't max(BILLION) on every operation of Perbill and such, only in from and new and such contractors. Thus, a Perbill with internal value of more than billion is absolutely garbage. If there is any way to create it, which seemingly there is, it should be fixed asap, with either implementing CompactAs or any other way.

Will you work on this yourself? else I can probably have a go later this week.

[gui1117]

ah I wrote:

There is still an issue if user do Compact::<Perbill>::decode(..).0 because the resulting Perbill can be more than 1.
I think this is hacky and would be in favor of removing Compact encoding altogether.

But its wrong, if we implement the CompactAs manually then Decode implementation is:

impl<T> Decode for Compact<T>
where
	T: CompactAs,
	Compact<T::As>: Decode,
{
	fn decode<I: Input>(input: &mut I) -> Result<Self, Error> {
		Compact::<T::As>::decode(input)
			.map(|x| Compact(<T as CompactAs>::decode_from(x.0)))
	}
}

So Compact::<Perbill>::decode(..).0 is a valid Perbill

Then I think manually implementation CompactAs is a good solution, I can do, it should be quick