Conversation
grod220
force-pushed
the
pod-option-updates
branch
from
345ff64 to
f61cfb4
Compare
| #[cfg(feature = "borsh")] | ||
| use borsh::{BorshDeserialize, BorshSchema, BorshSerialize}; |
There was a problem hiding this comment.
OptionalNonZeroPubkey had borsh support fyi. Though, not sure if this serialization method is still blessed or if we should remove it in this breaking change. Noticed about half of the primitives have it and half don't.
There was a problem hiding this comment.
we probably needed it for one of the token-2022 extensions, so we'd be in a tough position if it got removed
grod220
force-pushed
the
pod-option-updates
branch
from
f61cfb4 to
93e8a0c
Compare
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
Just a few comments to make this a bit more general
| #[cfg(feature = "borsh")] | ||
| use borsh::{BorshDeserialize, BorshSchema, BorshSerialize}; |
There was a problem hiding this comment.
we probably needed it for one of the token-2022 extensions, so we'd be in a tough position if it got removed
| #[cfg_attr( | ||
| feature = "borsh", | ||
| derive(BorshDeserialize, BorshSerialize, BorshSchema) | ||
| )] |
There was a problem hiding this comment.
Maybe I'm just having a brain fart, but does this only work if T: BorshSerialize + BorshDeserialize + BorshSchema? Will it fail to compile if T doesn't implement those and the borsh feature is enabled in the crate?
We'll probably need to implement each trait conditionally
| #[cfg(feature = "serde")] | ||
| impl Serialize for PodOption<Address> { |
There was a problem hiding this comment.
How about implementing this generically for all T: Serialize?
|
|
||
| #[cfg(feature = "serde")] | ||
| impl<'de> Deserialize<'de> for PodOption<Address> { | ||
| fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> |
There was a problem hiding this comment.
Same with this, let's implement it generically using https://docs.rs/serde_core/latest/src/serde_core/de/impls.rs.html#916-934 as inspiration 😄
| assert_eq!(def, None.try_into().unwrap()); | ||
| } | ||
|
|
||
| #[cfg(feature = "borsh")] |
There was a problem hiding this comment.
Let's also enable the borsh feature in spl-pod to be sure that these tests run, since I don't see the feature being enabled on main currently
2 participants
Part of #175
spl-podcurrently has two ways to represent optional addresses:optional_keys::OptionalNonZeroPubkeyoption::PodOption<Address>This PR reduces this duplication and aligns on
PodOption<Address>and extends PodOption to fully cover what was deleted in optional_keys:PodOption<Address>