token-2022: Add pod-compatible mint / account / multisig types #6337
Conversation
token/program-2022/src/pod.rs
Outdated
| spl_pod::primitives::PodU64, | ||
| }; | ||
|
|
||
| /// Mint data stored as a Pod type |
There was a problem hiding this comment.
Maybe link to the original type doc?
It would also be nice to have a unit test or something that ensures the Pod structs can never diverge from the original types, perhaps by comparing packed versions?
token/program-2022/src/pod.rs
Outdated
| pub supply: PodU64, | ||
| /// Number of base 10 digits to the right of the decimal place. | ||
| pub decimals: u8, | ||
| /// Is not 0 if this structure has been initialized |
There was a problem hiding this comment.
Just "not 0"? Or "exactly 1"?
Also, don't we have a PodBool in spl-pod? Any reason not to use that?
There was a problem hiding this comment.
Yeah sure, can do!
token/program-2022/src/pod.rs
Outdated
| /// If `delegate` is `Some` then `delegated_amount` represents | ||
| /// the amount authorized by the delegate | ||
| pub delegate: PodCOption<Pubkey>, | ||
| /// The account's state, stored as u8 |
There was a problem hiding this comment.
Link to AccountState doc? Not clear what these u8s will mean.
token/program-2022/src/pod.rs
Outdated
| pub m: u8, | ||
| /// Number of valid signers | ||
| pub n: u8, | ||
| /// If not 0, this structure has been initialized |
There was a problem hiding this comment.
Ditto about non-0 and PodBool
token/program-2022/src/pod.rs
Outdated
| /// COption<T> stored as a Pod type | ||
| #[repr(C, packed)] | ||
| #[derive(Clone, Copy, Debug, Default, PartialEq, Pod, Zeroable)] | ||
| pub struct PodCOption<T: Pod> { | ||
| option: [u8; 4], | ||
| value: T, | ||
| } |
There was a problem hiding this comment.
Any value in adding this to spl-pod?
There was a problem hiding this comment.
I'd prefer to keep it here since we have the OptionalNonZero* types in spl-pod, which are more space-efficient and more strictly Pod types. I'm worried it would create confusion by having this type in there too
token/program-2022/src/pod.rs
Outdated
| } | ||
| } | ||
| impl PackedSizeOf for PodMint { | ||
| const SIZE_OF: usize = std::mem::size_of::<Self>(); |
There was a problem hiding this comment.
While this expression is simple enough, maybe use spl_pod::pod_get_packed_len::<Self>() for consistency? Don't feel too strongly, though.
There was a problem hiding this comment.
That's a good point, will do!
|
All points should be addressed now, thanks for your careful review! |
token/program-2022/src/pod.rs
Outdated
| } | ||
| impl IsInitialized for PodMint { | ||
| fn is_initialized(&self) -> bool { | ||
| self.is_initialized == PodBool::from_bool(true) |
There was a problem hiding this comment.
| self.is_initialized == PodBool::from_bool(true) | |
| self.is_initialized.into() |
Better?
It would actually be kind of cute if PodBool implemented PartialEq<bool> for PodBool so that this line could be:
self.is_initialized == true
I can't really find a way to simplify it further to just self.is_initialized, though. There's a trait for Not, but not one for Is aside from PartialEq/Eq.
There was a problem hiding this comment.
ah yeah, much better! We might need to address the PartialEq / Eq question in a follow-up PR, but your suggestion is much clearer
token/program-2022/src/pod.rs
Outdated
| } | ||
| impl IsInitialized for PodMultisig { | ||
| fn is_initialized(&self) -> bool { | ||
| self.is_initialized == PodBool::from_bool(true) |
There was a problem hiding this comment.
Here too, if you like my suggestion above.
mergify
bot
dismissed
CriesofCarrots’s stale review
Pull request has been modified.
Problem
In order to use the Pod-compatible types in #6336, we need Pod-compatible base types.
Solution
Mostly copy-paste of the types in
state.rs, but only using types that align to 1. Also introducesPodCOption<T>for aPod-compatibleCOptionThese will eventually be used for the extension tests.