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.

Sign up for GitHub

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

Jump to bottom

Fix gnosis preset issue #5081

Merged
merged 3 commits into from
Feb 1, 2023
Merged

Fix gnosis preset issue #5081

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dbd6583
Fix loading presets other than mainnet and minimal
nflaig Jan 30, 2023
583f81f
Revert "Fix loading presets other than mainnet and minimal"
nflaig Jan 31, 2023
4c2144b
Add gnosis preset to dev command
nflaig Jan 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/cli/src/networks/dev.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import {gnosisChainConfig} from "@lodestar/config/networks";
import {minimalChainConfig, mainnetChainConfig} from "@lodestar/config/presets";
import {ACTIVE_PRESET, PresetName} from "@lodestar/params";

Expand All @@ -9,6 +10,9 @@ switch (ACTIVE_PRESET) {
case PresetName.minimal:
chainConfig = minimalChainConfig;
break;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding a network specific getNetworkData, I think we can just add another case for PresetName.gnosis. (And perhaps remove the default case)

Copy link
Member

Choose a reason for hiding this comment

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

(We probably want to support any preset for the dev command anyways)

Copy link
Member Author

@nflaig nflaig Jan 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The main reason for adding a function to get the network data is that we only want to execute this switch-case statement (or any other network specific code for that matter) if the network is dev. If we don't use a function this will always be executed which is unnecessary/unintuitive.

Copy link
Member Author

@nflaig nflaig Jan 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think we can just add another case for PresetName.gnosis

lets do this for now as it is less overall changes to get this fix out asap, I still think it is not a good that we execute code related to dev command if we run other commands such as beacon

Copy link
Member

Choose a reason for hiding this comment

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

Agree that its not great to be running this code, just looking for the smaller diff for quick fix.

Also looked into turning the network imports into an await import but that also is a big change since getNetworkData and some downstream consumers are all sync functions

Copy link
Member Author

Choose a reason for hiding this comment

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

Also looked into turning the network imports into an await import

yeah that was also the initial solution I wanted to use until I figured that the downstream call chain just never ends and had like 30+ changes, I still have most of those changes stashed if we want to apply this solution later

case PresetName.gnosis:
chainConfig = gnosisChainConfig;
break;
default:
throw Error(`Preset ${ACTIVE_PRESET} not supported with dev command`);
}
Expand Down