chore: fix prover README typos #6426
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6426 +/- ##
============================================
- Coverage 61.72% 61.70% -0.03%
============================================
Files 553 553
Lines 57862 57858 -4
Branches 1829 1828 -1
============================================
- Hits 35717 35701 -16
- Misses 22108 22120 +12
Partials 37 37 |
Performance Report✔️ no performance regression detected Full benchmark results
|
nflaig
requested changes
Feb 14, 2024
| @@ -53,15 +53,15 @@ export const startOptions: CliCommandOptions<StartArgs> = { | |||
| }, | |||
|
|
|||
| beaconUrls: { | |||
| description: "The beacon node PRC urls for 'rest' mode.", | |||
| description: "The beacon node RPC urls for 'rest' mode.", | |||
There was a problem hiding this comment.
The beacon node exposes a REST API and does not implement RPC, this is different from execution clients.
The description also reference the mode 'rest' which doesn't even exist?
There was a problem hiding this comment.
RPC is a pretty generic term (unlike JSON-RPC), so I took it in this way. Can probably be just removed.
Mention of mode looks like a left-over from previous refactoring.
There was a problem hiding this comment.
It even says that for the bootnodes for p2p which we don't even implement? Looks like it was just copy-pasted around without much thought
There was a problem hiding this comment.
RPC is not generic term, it refers to a certain standard which obviously is not what beacon node APIs complies with. So better to remove that term here.
There was a problem hiding this comment.
I would suggest to remove the beaconBootnodes option for now in a following PR, as it's not supported (even so it shows a proper error message when used). Would help to clarify for users the available options.
matthewkeil
requested changes
Feb 15, 2024
| @@ -37,13 +37,12 @@ console.log({balance, address}); | |||
| You can also invoke the package as binary. | |||
|
|
|||
| ```bash | |||
| npm -i g @lodestar/prover | |||
| npm i -g @lodestar/prover | |||
matthewkeil
requested review from
matthewkeil
and removed request for
matthewkeil
matthewkeil
previously approved these changes
Feb 15, 2024
There was a problem hiding this comment.
grrr github. was trying to remove the request changes but didnt work. approving as just agreed with @nflaig so fixing his will also fix mine
nflaig
reviewed
Feb 15, 2024
Comment on lines
63
to
64
| beaconBootnodes: { | ||
| description: "The beacon node PRC urls for 'p2p' mode.", | ||
| description: "Urls of beacon bootnodes to connect to.", |
There was a problem hiding this comment.
I am a bit confused what is meant by beaconBootnodes, on the one hand in the tests is passes a URL
lodestar/packages/prover/test/e2e/cli/cmds/start.test.ts
Lines 33 to 34 in 42e8835
But if those are bootnodes, you would not pass a URL here but an ENR and not connect to them but use them for peer discovery.
This option also give users a false assumption that you can run the light client in p2p mode but this is not even implemented.
There was a problem hiding this comment.
Right, I agree it's confusing.
My preference would be:
- do not touch the
beaconBootnodesas part of this PR - remove the
beaconBootnodesCLI option in a follow up PR
It's pretty easy to restore this option once it's actually supporte. It also reduces potential user confusion that might see this option exist but not clearly understand that it's not supported because not yet implemented.
I let @nazarhussain shim in if he feels like it should be kept.
There was a problem hiding this comment.
I'd agree, let's remove any changes to beaconBootnodes from this PR and remove it separately to have more visibility on it
nflaig
reviewed
Feb 15, 2024
|
🎉 This PR is included in v1.16.0 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix prover README typos