Add EpochRewards sysvar type #163
Conversation
| pub const HASH_BYTES: usize = 32; | ||
|
|
||
| #[derive(Default, Debug, Clone)] | ||
| pub struct Hash([u8; HASH_BYTES]); | ||
|
|
||
| impl Hash { | ||
| pub fn to_bytes(self) -> [u8; HASH_BYTES] { | ||
| self.0 | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not an expert in how things are organized in Pinocchio, but it seems strange to define the blockhash type inside sysvars::epoch_rewards.
There was a problem hiding this comment.
Maybe I should just remove it completely and change parent_blockhash: Hash to parent_blockhash: [u8; 32] ?
There was a problem hiding this comment.
Having a dedicated type for the hashes seems quite reasonable.
Not sure what is the state of the Pinocchio coverage here.
There should be a Hash type at some point, I would think.
| #[derive(Default, Debug, Clone)] | ||
| pub struct Hash([u8; HASH_BYTES]); |
There was a problem hiding this comment.
Should this also be repr(C) if it is used inside a repr(C) struct?
There was a problem hiding this comment.
Either repr(transparent) or repr(C) would work here, I think.
repr(transparent) is more reasonable if the internal state is also public.
This way, the user can see what layout is being used in the docs, for example.
So, maybe this:
| #[derive(Default, Debug, Clone)] | |
| pub struct Hash([u8; HASH_BYTES]); | |
| #[derive(Default, Debug, Clone)] | |
| #[repr(transparent)] | |
| pub struct Hash(pub [u8; HASH_BYTES]); |
I think any 32 bytes are a valid hash. So it seems fine to expose [u8; HASH_BYTES] I guess?
The SDK folks will probably want to look at this change as well.
@joncinque You are the expert here, right?
Or you can probably get someone with the SDK expertise to double-check the approach here.
There was a problem hiding this comment.
We avoided exposing the hash bytes publicly for the base hash type in the sdk, which makes we wary of exposing it here: https://github.com/anza-xyz/solana-sdk/blob/965e2a8a63bcc730ee6c001a0e66e3314bb924d6/hash/src/lib.rs#L53
We should definitely go with repr(transparent) though.
@febo do you have any opinions about a generic hash type for pinocchio? Did you want to create newtype, a type alias, or eventually use solana_hash::Hash? I'm hoping we can go with the last option, but we might have to remove the solana_sanitize dependency first.
There was a problem hiding this comment.
@LStan I think it will be better to mimic the SDK type here – so avoid exposing the hash bytes:
[repr(transparent)]
pub struct Hash(pub(crate) [u8; HASH_BYTES]);We eventually will use the SDK type so it will be easier to "swap" the type if they follow the same implementation.
4 participants
This PR adds EpochRewards sysvar type. I copied it from solana-sdk. Not sure about
total_pointsbecause it isu128. It will work withget, but iffrom_account_infois implemented it can cause UB. Probably it should be changed to [u8; 16].