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

How to represent BitFlags? #119

Open
ascjones opened this issue Jul 26, 2021 · 4 comments
Open

How to represent BitFlags? #119

ascjones opened this issue Jul 26, 2021 · 4 comments

Comments

@ascjones
Copy link
Contributor

ascjones commented Jul 26, 2021

In substrate we have the following type:

#[repr(u64)]
#[derive(Encode, Decode, Clone, Copy, PartialEq, Eq, BitFlags, RuntimeDebug, TypeInfo)]
pub enum IdentityField {
	Display = 0b0000000000000000000000000000000000000000000000000000000000000001,
	Legal = 0b0000000000000000000000000000000000000000000000000000000000000010,
	Web = 0b0000000000000000000000000000000000000000000000000000000000000100,
	Riot = 0b0000000000000000000000000000000000000000000000000000000000001000,
	Email = 0b0000000000000000000000000000000000000000000000000000000000010000,
	PgpFingerprint = 0b0000000000000000000000000000000000000000000000000000000000100000,
	Image = 0b0000000000000000000000000000000000000000000000000000000001000000,
	Twitter = 0b0000000000000000000000000000000000000000000000000000000010000000,
}

It is wrapped in a BitFlags in the following wrapper type to allow custom encoding/decoding.

/// Wrapper type for `BitFlags<IdentityField>` that implements `Codec`.
#[derive(Clone, Copy, PartialEq, Default, RuntimeDebug)]
pub struct IdentityFields(pub(crate) BitFlags<IdentityField>);

Question is how to represent this with scale_info: it is encoded as a raw u64 but we need to also have access to the IdentityField enum definition which maps the discriminants reduced to u8 indices.

A temporary workaround is to provide the following TypeInfo implementation

impl TypeInfo for IdentityFields {
	type Identity = Self;

	fn type_info() -> Type {
		Type::builder()
			.path(Path::new("BitFlags", module_path!()))
			.type_params(vec![ TypeParameter::new("T", Some(meta_type::<IdentityField>())) ])
			.composite(
			Fields::unnamed()
				.field(|f| f.ty::<u64>().type_name("IdentityField")),
		)
	}
}

See how it represents BitFlags<IdentityField> including the IdentityField as a type parameter, and the field itself as type u64.

However this requires some string matching for downstream type generators, so a preferable solution might be to add fist class support for a BitFlags like TypeDef variant.

On the other we might not want to be adding a new TypeDef variant for each corner case, be curious to see what others think.

/cc @jacogr

@ascjones
Copy link
Contributor Author

Another possibility is to capture the #[repr(u64)] and encode that into an optional repr field in TypeDefVariant

@gui1117
Copy link
Contributor

gui1117 commented Jul 27, 2021

sidenote: the Encode/Decode implementation on IdentityField is wrong and should be removed paritytech/substrate#9445

@ascjones
Copy link
Contributor Author

ascjones commented Jul 27, 2021

Then should I reconsider the removal of discriminant: u64 in #112?

@gui1117
Copy link
Contributor

gui1117 commented Jul 27, 2021

if the only use of discriminant is bitflag maybe it is better to have dedicated syntax.

But otherwise it is true that the discriminant gives useful information in this specific case.

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

2 participants