chore: typesafe options and cli args #4576
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
Performance Report✔️ no performance regression detected Full benchmark results
|
|
Anyone understands what is this error? The file referenced import { BeaconRestApiServerOpts } from "./rest/index.js";
export interface IApiOptions {
maxGindicesInProof?: number;
rest?: BeaconRestApiServerOpts & {
enabled?: boolean;
};
version?: string;
}
export declare const defaultApiOptions: {
maxGindicesInProof: number;
rest: {
enabled: boolean;
port: number;
address: string;
bodyLimit: number;
cors: string;
api: (keyof import("@lodestar/api/lib").Api)[];
};
};and |
dapplion
force-pushed
the
dapplion/cli-typesafe-args
branch
from
3443b1f
to
e9dfb46
Compare
g11tech
reviewed
Oct 2, 2022
packages/cli/src/util/command.ts
Outdated
| @@ -1,6 +1,13 @@ | |||
| import {Options, Argv} from "yargs"; | |||
|
|
|||
| export type ICliCommandOptions<OwnArgs> = Required<{[key in keyof OwnArgs]: Options}>; | |||
| export type ICliCommandOptions<OwnArgs> = Required< | |||
There was a problem hiding this comment.
this is some nifty typescript! ❤️
g11tech
reviewed
Oct 2, 2022
| const address = node.opts.api.rest.address || "localhost"; | ||
| const port = node.opts.api.rest.port || 19596; | ||
| const address = node.opts.api?.rest?.address ?? "localhost"; | ||
| const port = node.opts.api?.rest?.port ?? 19596; |
There was a problem hiding this comment.
the beacon node api was being hosted at 19596 earlier
g11tech
reviewed
Oct 2, 2022
| { | ||
| db: {name: tmpDir.name}, | ||
| eth1: {enabled: false}, | ||
| api: {rest: {api: ["beacon", "config", "events", "node", "validator"], port: 19596}}, |
There was a problem hiding this comment.
this endpoint has been removed, but validator still uses it
dapplion
force-pushed
the
dapplion/cli-typesafe-args
branch
from
128fcfb
to
e3fc4a6
Compare
Merged
dapplion
force-pushed
the
dapplion/cli-typesafe-args
branch
2 times, most recently
from
1e30ffc
to
8aaf742
Compare
dapplion
changed the title
|
Split original PR into two parts:
Previous branch with all parts before rebase: https://github.com/ChainSafe/lodestar/tree/dapplion/cli-typesafe-args_BACKUP |
dapplion
force-pushed
the
dapplion/cli-typesafe-args
branch
from
8aaf742
to
ddea9f8
Compare
dapplion
force-pushed
the
dapplion/cli-typesafe-args
branch
from
ddea9f8
to
4e3bf82
Compare
nflaig
reviewed
Jun 16, 2023
nflaig
reviewed
Jun 16, 2023
|
Thank you so much @nflaig ❤️ |
|
🎉 This PR is included in v1.10.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.
Motivation
Our current approach to options and CLI arg processing is not typesafe.
Description
Problem: CLI args and options are defined such that it's not guaranteed for the arg to not be undefined.
This PR: modifies
ICliCommandOptionsto ensure that either a default is set or the option must be marked as optionalProblem: Programmatic use of BeaconNode class has bad DX since many non-essential options must be provided
This PR: Changed options definitions such that BeaconNode class can be instantiated without proving any option
Problem: BeaconNodeOptions current default merging strategy is not typesafe. Options that are typed as not optional may receive undefined
This PR: Remove option deepmerging at the CLI level, and let's each subclass to handle default option setting without nesting
Extra issues
Fix typo bad option merging, mixing retryAttempts and retryDelay
lodestar/packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts
Line 129 in 34305f3
Set maxPeers to a value that greater than targetPeers if only the latter is set
Closes #4577