refactor the vote state with union type#1232
Conversation
dnut
force-pushed
the
refactor-vote-state-with-union
branch
from
efc3c35 to
e362462
Compare
hamza-syndica
force-pushed
the
refactor-vote-state-with-union
branch
from
8838642 to
8297063
Compare
src/runtime/program/vote/state.zig
Outdated
| pub fn votes(self: *const VoteState) *const std.ArrayListUnmanaged(LandedVote) { | ||
| return switch (self.*) { | ||
| .v3 => |*s| &s.votes, | ||
| .v4 => |*s| &s.votes, | ||
| }; | ||
| } |
There was a problem hiding this comment.
There's not much sense in returning a constant referenco the arraylist.
| pub fn votes(self: *const VoteState) *const std.ArrayListUnmanaged(LandedVote) { | |
| return switch (self.*) { | |
| .v3 => |*s| &s.votes, | |
| .v4 => |*s| &s.votes, | |
| }; | |
| } | |
| pub fn votes(self: *const VoteState) []const LandedVote { | |
| return switch (self.*) { | |
| inline .v3, .v4 => |s| s.votes.items, | |
| }; | |
| } |
src/runtime/program/vote/state.zig
Outdated
| pub fn epochCreditsList(self: *const VoteState) *const std.ArrayListUnmanaged(EpochCredit) { | ||
| return switch (self.*) { | ||
| .v3 => |*s| &s.epoch_credits, | ||
| .v4 => |*s| &s.epoch_credits, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Ditto
| pub fn epochCreditsList(self: *const VoteState) *const std.ArrayListUnmanaged(EpochCredit) { | |
| return switch (self.*) { | |
| .v3 => |*s| &s.epoch_credits, | |
| .v4 => |*s| &s.epoch_credits, | |
| }; | |
| } | |
| pub fn epochCreditsList(self: *const VoteState) []const EpochCredit { | |
| return switch (self.*) { | |
| inline .v3, .v4 => |s| s.epoch_credits.items, | |
| }; | |
| } |
| pub fn epochCredits(self: *const VoteState) u64 { | ||
| return switch (self.*) { | ||
| inline else => |*s| s.epochCredits(), | ||
| }; | ||
| } |
There was a problem hiding this comment.
I would use inline .v3, .v4 instead of inline else. Expresses intent more clearly.
There was a problem hiding this comment.
when would you use inline else?
i think the intent is clear either way. there's an expectation for all the types to have this method. if not, it will fail to compile. what's the problem?
src/runtime/program/vote/state.zig
Outdated
| pub fn isV4(self: VoteState) bool { | ||
| return self == .v4; | ||
| } | ||
|
|
||
| pub fn isV3(self: VoteState) bool { | ||
| return self == .v3; | ||
| } |
There was a problem hiding this comment.
These seem somewhat pointless compared to the caller just writing vote_state == .<tag>.
src/runtime/program/vote/state.zig
Outdated
| pub fn asV3(self: *VoteState) ?*VoteStateV3 { | ||
| return switch (self.*) { | ||
| .v3 => |*s| s, | ||
| .v4 => null, | ||
| }; | ||
| } | ||
|
|
||
| /// Access the underlying VoteStateV4, if this is a V4 state. | ||
| pub fn asV4(self: *VoteState) ?*VoteStateV4 { | ||
| return switch (self.*) { | ||
| .v3 => null, | ||
| .v4 => |*s| s, | ||
| }; | ||
| } | ||
|
|
||
| pub fn asV3Const(self: *const VoteState) ?*const VoteStateV3 { | ||
| return switch (self.*) { | ||
| .v3 => |*s| s, | ||
| .v4 => null, | ||
| }; | ||
| } | ||
|
|
||
| pub fn asV4Const(self: *const VoteState) ?*const VoteStateV4 { | ||
| return switch (self.*) { | ||
| .v3 => null, | ||
| .v4 => |*s| s, | ||
| }; | ||
| } |
There was a problem hiding this comment.
I don't see the benefit of these methods over just directly switching over the vote state.
src/runtime/program/vote/state.zig
Outdated
| pub fn initV3( | ||
| allocator: Allocator, | ||
| node_pubkey: Pubkey, | ||
| authorized_voter: Pubkey, | ||
| withdrawer: Pubkey, | ||
| commission_val: u8, | ||
| voter_epoch: Epoch, | ||
| ) Allocator.Error!VoteState { | ||
| return .{ .v3 = try VoteStateV3.init( | ||
| allocator, | ||
| node_pubkey, | ||
| authorized_voter, | ||
| withdrawer, | ||
| commission_val, | ||
| voter_epoch, | ||
| ) }; | ||
| } | ||
|
|
||
| pub fn initV4( | ||
| allocator: Allocator, | ||
| node_pubkey: Pubkey, | ||
| authorized_voter: Pubkey, | ||
| withdrawer: Pubkey, | ||
| commission_pct: u8, | ||
| voter_epoch: Epoch, | ||
| vote_pubkey: Pubkey, | ||
| ) Allocator.Error!VoteState { | ||
| return .{ .v4 = try VoteStateV4.init( | ||
| allocator, | ||
| node_pubkey, | ||
| authorized_voter, | ||
| withdrawer, | ||
| commission_pct, | ||
| voter_epoch, | ||
| vote_pubkey, | ||
| ) }; | ||
| } |
There was a problem hiding this comment.
These initialization methods don't seem very purposeful. The caller can just as easily write .{ .v4 = try .init(all, the, same, parameters) }. You even bypass these just a hundred or so lines prior, so that you can initialize the payload directly with managed memory.
src/runtime/program/stake/lib.zig
Outdated
| var vote_state_to_deinit: ?*const anyerror!VoteStateVersions = &vote_state; | ||
| if (vote_state_to_deinit) |fallible| if (fallible.*) |*vs| vs.deinit(allocator) else |_| {}; |
There was a problem hiding this comment.
Is this missing a defer? Or otherwise, what's going on here.
src/runtime/program/vote/execute.zig
Outdated
| if (std.meta.isError(account.setDataLength( | ||
| allocator, | ||
| resize_delta, | ||
| VoteStateV4.MAX_VOTE_STATE_SIZE, | ||
| ))) { | ||
| return InstructionError.AccountNotRentExempt; | ||
| } |
There was a problem hiding this comment.
I highly advise advise against using std.meta in general unless there is no better alternative:
| if (std.meta.isError(account.setDataLength( | |
| allocator, | |
| resize_delta, | |
| VoteStateV4.MAX_VOTE_STATE_SIZE, | |
| ))) { | |
| return InstructionError.AccountNotRentExempt; | |
| } | |
| try account.setDataLength( | |
| allocator, | |
| resize_delta, | |
| VoteStateV4.MAX_VOTE_STATE_SIZE, | |
| ); |
I would have suggested catch return error.AccountNotRentExempt;, but I noticed that this function returns Allocator.Error || InstructionError, which is identical to the enclosing function's error set. Is there any reason not to propagate it directly?
src/runtime/program/vote/execute.zig
Outdated
| if (account.constAccountData().len < VoteStateV3.MAX_VOTE_STATE_SIZE and | ||
| (!rent.isExempt(account.account.lamports, VoteStateV3.MAX_VOTE_STATE_SIZE) or | ||
| std.meta.isError(account.setDataLength( | ||
| allocator, | ||
| resize_delta, | ||
| VoteStateV3.MAX_VOTE_STATE_SIZE, | ||
| )))) | ||
| { |
There was a problem hiding this comment.
This is a bad idea, the isError will treat error.OutOfMemory equally to an InstructionError.
|
@InKryption thanks for all of your comments. I have resolved your concerns. |
5 participants
Refactored VoteState to use a union type, extracted into a dedicated
state.zigmodule. Updated all consumers across consensus, replay, rewards, and runtime to use the new type.Changes
src/runtime/program/vote/state.zig(around ~1000 lines) with union-based vote state representationvote/execute.zigto use the new state types