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

Handle #[codec(index = $int)] on regular enums #79

Closed
dvdplm opened this issue Mar 22, 2021 · 0 comments · Fixed by #80
Closed

Handle #[codec(index = $int)] on regular enums #79

dvdplm opened this issue Mar 22, 2021 · 0 comments · Fixed by #80

Comments

@dvdplm
Copy link
Contributor

dvdplm commented Mar 22, 2021

PR #44 handles the index attribute on C-like enums, but does not address regular enums with some variants marked #[codec(index = $int)].

All enum variants have a discriminant. C-style enums allow explicitly setting the discriminant. Regular enums do not.
The #[codec(index = 123)] attribute sets the index byte for the variant it decorates when the enum is SCALE encoded.

"index" and "discriminant" do not mean the same thing, but the current implementation conflates them somewhat and the default "index" for a variant is, in order of priority, the #[codec(index = …)] attribute, the explicitly set discriminant, and lastly the implicit index from the order of appearance.

I believe parity-scale-codec is slightly broken today, in that the following will lead to bad things happening when encoding/decoding:

#[derive(Encode)]
enum Foo {
	A,
	#[codec(index = 2)]
	B = 8,
	C
}

The Variant struct in scale-info has a discriminant member, an Option<u64>. Today it is None for all non C-style enums.

Picking the right solution here can be a bit tricky. We can use the current discriminant field to store the index. This is a questionable solution though, as we perpetuate the mixing of index and discriminant. Open to alternative ideas.

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

Successfully merging a pull request may close this issue.

1 participant