Skip to content

Cli: improve vote-account vote-authority display#95

Merged
mergify[bot] merged 4 commits intoanza-xyz:masterfrom
CriesofCarrots:cli-vote-authority
Mar 6, 2024
Merged

Cli: improve vote-account vote-authority display#95
mergify[bot] merged 4 commits intoanza-xyz:masterfrom
CriesofCarrots:cli-vote-authority

Conversation

@CriesofCarrots
Copy link
Copy Markdown

Problem

In solana vote-account output, the Vote Authority line is not clear. It returns a map of one or more numbers to address strings, eg:

Vote Authority: {2: "A1T3QdPZGRT99twji2M4vrkFRDXjp8aEbFc6GCJuviSp"}

The number is in fact an epoch, the epoch the respective authority is active. This is usually the current epoch, but in the case of a vote-authority update, it is the epoch in which the new vote authority will go into effect.

Cleaning up small, old Labs issues: solana-labs#11273

Summary of Changes

Simplify normal display:

Vote Authority: A1T3QdPZGRT99twji2M4vrkFRDXjp8aEbFc6GCJuviSp

If a new vote authority has been authorized, display its information, eg:

Vote Authority: A1T3QdPZGRT99twji2M4vrkFRDXjp8aEbFc6GCJuviSp
  New Vote Authority as of Epoch 4: EvgdtuXcKoBAMW6eL6KMiqDre1do5o2v4tinFv91H2Fo

Fixes solana-labs#11273

@CriesofCarrots
Copy link
Copy Markdown
Author

CriesofCarrots commented Mar 5, 2024

Also, a question: I think the json output of vote authorities should properly be an array (echoes this issue), ie:

"authorizedVoters": [
    {
        "epoch": "3"
        "voter": "A1T3QdPZGRT99twji2M4vrkFRDXjp8aEbFc6GCJuviSp",
    },
    {
        "epoch": "4"
        "voter": "EvgdtuXcKoBAMW6eL6KMiqDre1do5o2v4tinFv91H2Fo",
    }
]

instead of current:

"authorizedVoters": {
    "3": "A1T3QdPZGRT99twji2M4vrkFRDXjp8aEbFc6GCJuviSp",
    "4": "EvgdtuXcKoBAMW6eL6KMiqDre1do5o2v4tinFv91H2Fo"
  }

Too breaking to change? (Or maybe okay after v2 version bump?)

@CriesofCarrots CriesofCarrots requested a review from t-nelson March 5, 2024 22:55
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 6, 2024

Codecov Report

Attention: Patch coverage is 70.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 81.8%. Comparing base (661de5b) to head (393569a).
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master      #95     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         838      838             
  Lines      225940   225947      +7     
=========================================
- Hits       184937   184884     -53     
- Misses      41003    41063     +60     

t-nelson
t-nelson previously approved these changes Mar 6, 2024
Copy link
Copy Markdown

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while part of me wishes it didn't, lgtm 😅

impl fmt::Display for CliAuthorizedVoters {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "{:?}", self.authorized_voters)
if let Some((_epoch, current_authorized_voter)) = self.authorized_voters.first_key_value() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"surely we can't resolve the curre... why the hell do we do all of this superfluous work for every vote?"

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a good question!

Comment thread cli-output/src/cli_output.rs Outdated
}
if self.authorized_voters.len() > 1 {
let (epoch, upcoming_authorized_voter) =
self.authorized_voters.last_key_value().unwrap();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: prefer expect("why?")

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

@CriesofCarrots CriesofCarrots added the automerge automerge Merge this Pull Request automatically once CI passes label Mar 6, 2024
@mergify mergify Bot merged commit 3cf4834 into anza-xyz:master Mar 6, 2024
codebender828 pushed a commit to codebender828/agave that referenced this pull request Oct 3, 2024
* Simplify vote-authority display

* Add handling for new vote authority

* Add proper None handling, because unit test (shouldn't happen IRL, though)

* Unwrap->expect
OliverNChalk pushed a commit to OliverNChalk/agave that referenced this pull request Nov 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge automerge Merge this Pull Request automatically once CI passes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

solana vote-account authorized-voter display is not clear

3 participants