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

Fix pregenesis countdown msg wording #7477

Merged

Conversation

bidlocode
Copy link
Contributor

@bidlocode bidlocode commented Oct 9, 2020

Fixes #7476

Pre genesis countdown log messages might be confusing:

Time till minimum genesis: <time>	## time till eth1 threshold
<time> until chain genesis 		## time till MIN_GENESIS_TIME

@bidlocode bidlocode requested a review from a team as a code owner October 9, 2020 13:45
"Extra validators needed": valNeeded,
"Time till minimum genesis": fmt.Sprintf("%s", time.Duration(secondsLeft)*time.Second),
"Extra validators needed": valNeeded,
"Time till eth1 threshold": fmt.Sprintf("%s", time.Duration(secondsLeft)*time.Second),
Copy link
Member

Choose a reason for hiding this comment

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

Eth1 threshold is not a time. I don't quite understand this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two countdowns, and the first countdown with 'minimum genesis' was confusing for users(because of MIN_GENESIS_TIME).

@prestonvanloon
Copy link
Member

Needs docs approval on the language. cc: @har00ga

I personally find this more confusing because min genesis time is the nomenclature that we have been using since the beginning and other teams are using as well.

@bidlocode
Copy link
Contributor Author

bidlocode commented Oct 10, 2020

But that's the thing that MIN_GENESIS_TIME from config is different from what the first countdown is about. Am I wrong on that?

@prestonvanloon
Copy link
Member

But that's the thing that MIN_GENESIS_TIME from config is different from what the first countdown is about. Am I wrong on that?

The "seconds left" counts to the minimum genesis time or the actual genesis time of the chain, whichever is greater.

@prestonvanloon
Copy link
Member

I see how it could be confusing when secondsLeft counts to the actual genesis time rather than the min genesis time. Perhaps we can rename to remove "minimum". I.e. "Time till genesis".

@har00ga thoughts?

@bidlocode
Copy link
Contributor Author

bidlocode commented Oct 10, 2020

to remove "minimum". I.e. "Time till genesis".

👍

@nisdas
Copy link
Member

nisdas commented Oct 10, 2020

Agree with @bidlocode the message is confusing. Although Time till Genesis or Until Chain Genesis isn't very correct either as you could interprete it as chainstart in both situations. How about Generating genesis state in 'time' ?

@prestonvanloon
Copy link
Member

Sounds good to me @nisdas

@stale
Copy link

stale bot commented Oct 22, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale There hasn't been any activity here in some time... label Oct 22, 2020
@rkapka
Copy link
Contributor

rkapka commented Oct 22, 2020

@bidlocode Hey, are you planning on completing this PR? It was been marked as stale by the bot.

@codecov
Copy link

codecov bot commented Oct 23, 2020

Codecov Report

Merging #7477 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7477      +/-   ##
==========================================
- Coverage   61.88%   61.88%   -0.01%     
==========================================
  Files         422      422              
  Lines       29714    29721       +7     
==========================================
+ Hits        18389    18392       +3     
+ Misses       8404     8403       -1     
- Partials     2921     2926       +5     

@prylabs-bulldozer prylabs-bulldozer bot merged commit cdbbf66 into prysmaticlabs:master Oct 23, 2020
@bidlocode bidlocode deleted the fix-pregenesis-logs-wording branch October 23, 2020 05:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale There hasn't been any activity here in some time...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pre genesis countdown msgs might be confusing
5 participants