Erase PhantomData fields #111
Conversation
|
Do I understand correctly that this PR filters out |
Precisely |
There was a problem hiding this comment.
Full circle then! :)
@jacogr is right of course but the reason we had PhantomData here was that we decided to put the onus on the devs to #[scale(skip)] such types, so they should never appear in metadata (and thus not create issues for different languages) if we're disciplined.
That said I don't disagree removing them.
| @@ -91,7 +91,7 @@ impl IntoPortable for TypeDefComposite { | |||
|
|
|||
| impl TypeDefComposite { | |||
| /// Creates a new struct definition with named fields. | |||
| pub fn new<I>(fields: I) -> Self | |||
| pub(crate) fn new<I>(fields: I) -> Self | |||
There was a problem hiding this comment.
So users can't instantiate with any fields containing a PhantomData. They are filtered out in the builder, and this would allow circumventing the builder.
The other option of course would be to do the phantom filtering directly in this method, but I think we should encourage the use of the builder as the way to construct Composite types.
Feedback from @jacogr while working on polkadot-js/api#3759.
He rightly points out that
PhantomDatafields are redundant from a scale encoding perspective, since nothing is actually encoded. So all type generators for different langs would have to implement a filter for them or have a bunch of useless fields. This is aRustimplementation detail leaking out into the metadata definition. This PR removes that burden and filters out fields ofPhantomDataat source.I know there has been much discussion of this in the past, but I think this solution avoids the main problem we had in the past by avoiding the macro having to syntactically identify
PhantomDatatypes.Let me know what you think @Robbepop @dvdplm