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

Should SubstrateType more consistent with RustTypeMarker ? #36

Open
yjhmelody opened this issue Dec 14, 2020 · 1 comment
Open

Should SubstrateType more consistent with RustTypeMarker ? #36

yjhmelody opened this issue Dec 14, 2020 · 1 comment
Labels
difficulty-easy enhancement New feature or request good first issue Good for newcomers

Comments

@yjhmelody
Copy link
Contributor

yjhmelody commented Dec 14, 2020

I noticed that SubstrateType is a stateful version of RustTypeMarker. And I think them should keep similiar as possible.

I think the following common types should be defined in a new enum type.

pub enum SubstrateType {
	/// 512-bit hash type
	H512(primitives::H512),
	/// 256-bit hash type
	H256(primitives::H256),

	/// Recursive Call Type
	Call(Vec<(String, SubstrateType)>),
	/// Era
	Era(runtime_primitives::generic::Era),

	/// Vote
	#[serde(with = "RemoteVote")]
	GenericVote(pallet_democracy::Vote),
 ...

And SubstrateType should be:

pub enum SubstrateType<T> {
     Common(T),
 ...

Other people can compose their own implementations more easily.
And can ensure that the core changes as little as possible

How about your thinking?

@insipx insipx added enhancement New feature or request good first issue Good for newcomers difficulty-easy labels Dec 14, 2020
@insipx
Copy link
Contributor

insipx commented Dec 14, 2020

Yup, SubstrateType is a stateful/Output version of RustTypeMarker (RustTypeMarker + bytestring in, SubstrateType out)
Sounds like a good addition that would make the types more clear.

In addition to the types you listed, the Data, Address and SignedExtra would also belong on this Common enum since they don't have a direct correspondent in RustTypeMarker

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty-easy enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants