Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show correct total validator count during validator recovery #6153

Merged
merged 2 commits into from
Jul 25, 2024

Conversation

dknopik
Copy link
Contributor

@dknopik dknopik commented Jul 23, 2024

Issue Addressed

A small one I found just now: The progress indicator displayed during lighthouse account validator recover would show something like 0/4294967276 Index: 40 when count < first_index.

Proposed Changes

Simply display the count there instead of subtracting the first index.

Additional Info

None.

@chong-he chong-he added bug Something isn't working UX-and-logs labels Jul 23, 2024
Copy link
Member

@chong-he chong-he left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, nice catch!

Just a nitpick, thought it would be better to have the count as count - 1, since the count displayed starts from 0. So, when we do:

lighthouse account validator recover --first-index 4 --count 2, it will show:

0/1
1/1 

instead of:

0/2 
1/2

Edit: other than that, tested and confirmed the UX shown as intended and it's good

@dknopik
Copy link
Contributor Author

dknopik commented Jul 23, 2024

Hey @chong-he, good point. But then actually, I would argue for

1/2 
2/2

which might be more intuitive to users, as people usually index starting at 1 in non-code contexts. But that is a matter of taste I guess.

@chong-he
Copy link
Member

chong-he commented Jul 24, 2024

Good point, that would be:

 index - first_index + 1,
 count,

I have commented above as well.

I think this is good to go

chong-he

This comment was marked as duplicate.

@chong-he chong-he added the ready-for-review The code is ready for review label Jul 24, 2024
Co-authored-by: chonghe <44791194+chong-he@users.noreply.github.com>
@chong-he chong-he added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jul 24, 2024
@jimmygchen
Copy link
Member

@mergify queue

Copy link

mergify bot commented Jul 24, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at b4a7560

mergify bot added a commit that referenced this pull request Jul 24, 2024
@mergify mergify bot merged commit b4a7560 into sigp:unstable Jul 25, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready-for-merge This PR is ready to merge. UX-and-logs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants