-
-
Notifications
You must be signed in to change notification settings - Fork 291
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 gnosis preset issue #5081
Fix gnosis preset issue #5081
Conversation
Use getters to load network data, this allows to add additional logic (as it is done for dev) and also only loads the network data into memory if it is actually used.
675ba55
to
dbd6583
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me!
break; | ||
case PresetName.minimal: | ||
chainConfig = minimalChainConfig; | ||
break; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
* Fix loading presets other than mainnet and minimal Use getters to load network data, this allows to add additional logic (as it is done for dev) and also only loads the network data into memory if it is actually used. * Revert "Fix loading presets other than mainnet and minimal" This reverts commit dbd6583. * Add gnosis preset to dev command
🎉 This PR is included in v1.6.0 🎉 |
Motivation
Issue reported here https://discord.com/channels/593655374469660673/743858262864167062/1069661074556719175
In
v1.4.0
it is not possible to use any preset other thanmainnet
andminimal
because thedev.ts
is always executed which checks for those and otherwise throws an error.lodestar/packages/cli/src/networks/dev.ts
Lines 5 to 14 in be9b9a0
Description
Use getters to load network data, this allows to add additional logic (as it is done for dev) and also only loads the network data into memory if it is actually used.Add gnosis preset to dev command