Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not derive encode/decode when discriminant doesn't fit u8. #283

Open
gui1117 opened this issue Jul 27, 2021 · 7 comments
Open

Do not derive encode/decode when discriminant doesn't fit u8. #283

gui1117 opened this issue Jul 27, 2021 · 7 comments

Comments

@gui1117
Copy link
Contributor

gui1117 commented Jul 27, 2021

whe the discriminant doesn't fit u8 the derivation is wrong (more precisely: unexpected)

#[derive(Encode, Decode)]
#[repr(u32)]
enum ABC {
	D = 3u32,
}
#[repr(u32)]
enum ABC { D = 3u32, }
const _: () =
    {
        #[allow(unknown_lints)]
        #[allow(rust_2018_idioms)]
        extern crate parity_scale_codec as _parity_scale_codec;
        impl _parity_scale_codec::Encode for ABC {
            fn encode_to<__CodecOutputEdqy: _parity_scale_codec::Output +
                         ?Sized>(&self,
                                 __codec_dest_edqy: &mut __CodecOutputEdqy) {
                match *self {
                    ABC::D => {
                        __codec_dest_edqy.push_byte(3u32 as
                                                        ::core::primitive::u8);
                    }
                    _ => (),
                }
            }
        }
        impl _parity_scale_codec::EncodeLike for ABC { }
    };
const _: () =
    {
        #[allow(unknown_lints)]
        #[allow(rust_2018_idioms)]
        extern crate parity_scale_codec as _parity_scale_codec;
        impl _parity_scale_codec::Decode for ABC {
            fn decode<__CodecInputEdqy: _parity_scale_codec::Input>(__codec_input_edqy:
                                                                        &mut __CodecInputEdqy)
             -> ::core::result::Result<Self, _parity_scale_codec::Error> {
                match __codec_input_edqy.read_byte().map_err(|e|
                                                                 e.chain("Could not decode `ABC`, failed to read variant byte"))?
                    {
                    __codec_x_edqy if
                    __codec_x_edqy == 3u32 as ::core::primitive::u8 => {
                        ::core::result::Result::Ok(ABC::D)
                    }
                    _ =>
                    ::core::result::Result::Err("Could not decode `ABC`, variant doesn\'t exist".into()),
                }
            }
        }
    };

I think we can use From::from instead of as maybe but it is probably a breaking change.

@ascjones
Copy link
Contributor

It's already protected against overflows, you will get the following error:

image

On a related note, do we want to be able to support derived encoding for larger repr discriminants?

@gui1117
Copy link
Contributor Author

gui1117 commented Jul 27, 2021

I tried to compile:

#[derive(Encode, Decode)]
#[repr(u32)]
enum ABC {
	D = 300_000u32,
}

I didn't get any compile error, how did you get it ?

@gui1117
Copy link
Contributor Author

gui1117 commented Jul 27, 2021

On a related note, do we want to be able to support derived encoding for larger repr discriminants?

I was thinking maybe we should reserve the index: u8::max_value() for such a case

@ascjones
Copy link
Contributor

I didn't get any compile error, how did you get it ?

I added a variant to IdentityFields https://github.com/paritytech/substrate/blob/686dd77ce530af2131402683b29974c74eb23e8f/frame/identity/src/types.rs#L243

Maybe you only get that for binary literals.

@ascjones
Copy link
Contributor

I was thinking maybe we should reserve the index: u8::max_value() for such a case

It's a good idea, though would break any existing 256 variant enums 😁

@gui1117
Copy link
Contributor Author

gui1117 commented Jul 27, 2021

yes it is breaking enums which uses the index 255, even if it contained less variants.

@bkchr
Copy link
Member

bkchr commented Jul 27, 2021

Do we see repr(u32) in the attributes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants