-
Notifications
You must be signed in to change notification settings - Fork 64
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
add BorshSchema for PhantomData, BTreeMap and BTreeSet #93
Conversation
|
||
// Because it's a zero-sized marker, its type parameter doesn't need to be | ||
// included in the schema and so it's not bound to `BorshSchema` | ||
impl<T> BorshSchema for PhantomData<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the fence about this one, love to hear other's thoughts.
Seems like a field like this should be ignored from the schema rather than inserting this. Not sure if implementing this would cause issues with maintenance if we ever wanted to remove.
Also, wonder if it could be nil
as ()
is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @austinabell, I don't see any value in exposing Rust type-system artifacts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that fields like this should be ignored, but I don't think that's a good reason not to implement BorshSchema
as that limits deriving schema on composed data structures that do contain these fields. I'm not a big fan of using "nil" and I think ideally the BorshSchema
trait and BorshSchemaContainer
should have the declaration optional, so that None
represents empty bytes, but that's of course a breaking change, so if you prefer to stick to "nil" for now, I'd be happy to go for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a valid point, I mean I assume that perhaps BorshSchema
should be 1:1 with borsh impls, which does contain PhantomData
, but I'm not sure if that is a guarantee we want/have with schema?
As for using nil vs having a specific case for PhantomData
, I'm not sure this adds value in any way. Given that for borsh (and even serde) this is just (de)serialized as nil, when would someone care if something was a PhantomData or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a valid point, I mean I assume that perhaps BorshSchema should be 1:1 with borsh impls, which does contain PhantomData, but I'm not sure if that is a guarantee we want/have with schema?
Yeah, I think that's a reasonable thing to do
As for using nil vs having a specific case for PhantomData, I'm not sure this adds value in any way. Given that for borsh (and even serde) this is just (de)serialized as nil, when would someone care if something was a PhantomData or not?
True, I don't think it does add any value and if anything it really should be hidden from schema because it is a language specific construct. Let's change this to "nil" then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Borsh schema exposes field names and with nil type you will still see the unnecessary field name in the schema. However, thinking about it now, it might be confusing if we skip some fields out of blue even if they are virtual, so maybe we should just recommend using borsh(skip) explicitly and then I would consider NOT implementing BorshSchema for PhantomData
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the purposes of ABI having a nil-type field would result into dead bytes. Not a huge deal though, since it would be very small anyways.
Also, FWIW serde has impls for PhantomData
and you can even find a few old issues where people complained that they could not derive serde traits because there are no PhantomData
impls (before they were added).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@itegulov this issue is about schema, not serialization/deserialization. Borsh ser/de just like serde implements traits for PhantomData, however, BorshSchema was left not implemented for PhantomData and now I argue that it should not be, thus forcing users explicitly skip the field from the schema
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frol Oh yeah right I got confused for a second, ignore the second part of my comment!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why there would be asymmetry there. If it's implemented for Borsh, why not include it in the schema? By the same logic, should we not remove impl BorshSchema for ()
?
Co-authored-by: Austin Abell <austinabell8@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All seems reasonable to me. Sorry for the delayed review
@frol I'm not sure if this was switched to draft on purpose, but I switched it back |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think there is no harm with having BorshSchema implemented to PhantomData, so let's merge it and call it a day.
This is a follow-up to #83.
closes #84