vote-interface: remove v0_23_5 #479
Conversation
| #[cfg(test)] | ||
| pub(crate) fn try_convert_to_v3(self) -> Result<VoteStateV3, InstructionError> { |
There was a problem hiding this comment.
@jstarry since these are now test-only, we can probable remove them completely here.
There was a problem hiding this comment.
This change doesn't look related to the rest of the PR. Can we remove it in a follow up?
There was a problem hiding this comment.
Yeah sure, we can. That's why I left that commit out for now.
| } | ||
| // V1_14_11. substantially different layout and data from V0_23_5 | ||
| // Variant 0 is uninitialized. | ||
| 0 => Err(InstructionError::UninitializedAccount), |
There was a problem hiding this comment.
Noting that this error change will change instruction errors downstream so we should make sure Firedancer is aware when we pick up this version update.
-
If we try to deserialize an account with
[0, 0, 0, 0]prefix but it's not big enough to holdV0_23_5, we used to returnInvalidAccountDatabut now we always returnUninitializedAccount -
In vote program, if we successfully deserialized and converted from
V0_23_5toV3, some instructions don't check for initialization (but later fail with some ix error) but now we will always returnUninitializedAccount
There was a problem hiding this comment.
To me, UninitializedAccount feels like the correct error, but we could always leave it as InvalidAccountData if it's too painful to change. Tagging @mjain-jump to weigh in.
There was a problem hiding this comment.
I prefer uninitialized too, just noting that we need to let them know that it's going to change
There was a problem hiding this comment.
UninitializedAccount conceptually makes less sense to me because, the account is technically "initialized" as a v0_23_5 account, its just not valid anymore (so invalid account data makes more sense IMO). I have the same proposal here: #447
There was a problem hiding this comment.
Yes, right now the account is technically "initialized" as v0 but part of the reason for this PR is to change that notion entirely and instead handle such accounts as technically being "uninitialized" since they can only exist as a fully zeroed accounts
There was a problem hiding this comment.
Haha fair enough @mjain-jump - you got me. I did suggest either one in my review comment, though.
We know none of these accounts exist on mb, so as @jstarry said, reading an account with [0, 0, 0, 0] would be all-zeroes, but I guess the SDK could more agnostically handle serialized state.
the account is technically "initialized" as a v0_23_5 account, its just not valid anymore
At this point I could go wither way, but I think this is a solid point.
| Err(InstructionError::InvalidAccountData) | ||
| } | ||
| // Variant 0 is uninitialized. | ||
| 0 => Err(InstructionError::UninitializedAccount), |
There was a problem hiding this comment.
Same as above, this error change will change instruction errors downstream so we should make sure Firedancer is aware when we pick up this version update.
- If we try to deserialize an account with
[0, 0, 0, 0]prefix but it's not big enough to holdV0_23_5, we used to returnInvalidAccountDatabut now we always returnUninitializedAccount
| #[cfg(test)] | ||
| pub(crate) fn try_convert_to_v3(self) -> Result<VoteStateV3, InstructionError> { |
There was a problem hiding this comment.
This change doesn't look related to the rest of the PR. Can we remove it in a follow up?
| // this function returns `Ok(..)`. However, future versions may not be | ||
| // convertible to v4 without data loss, so this function returns a `Result` | ||
| // for forward compatibility. | ||
| #[cfg(any(test, all(not(target_os = "solana"), feature = "bincode")))] |
There was a problem hiding this comment.
Same here, can we change this in a follow up PR?
There was a problem hiding this comment.
This change in particular is to satisfy clippy/hack checks, since the function is only used for deserialization (non-os-solana) of a v0_23_5 underlying vote state. Removal of that variant and deserialization logic leaves this unused outside of test context.
There was a problem hiding this comment.
But it's public isn't it? I thought clippy wouldn't complain about unused public methods
There was a problem hiding this comment.
Sorry the last comment was from my phone.. Makes sense to make this change given that the vis is pub(crate) not pub as I was thinking.
There was a problem hiding this comment.
No problem. Yeah it's a clippy error if we leave the original cfg visibility in tact. In follow up, they'll just come out completely.
Removes the old vote state version
V0_23_5from the vote-interface crate. ForVoteStateVersions, replacesV0_23_5with anUninitializedvariant and treats it as such.