-
Notifications
You must be signed in to change notification settings - Fork 218
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
Properly fill genesis_data
defaults
#215
Properly fill genesis_data
defaults
#215
Conversation
1cff0fc
to
665e946
Compare
if genesis_data.get("config"): | ||
try: | ||
genesis_data_config_filled = GenesisDataConfig(**genesis_data["config"]) | ||
except ValidationError as e: |
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.
Do you think it's worth adding a test for hitting the error conditions?
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.
yes! added.
99bfa6c
to
68ee2b2
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.
lgtm 👍🏼... just left a small nit on a validation method from a previous PR. Take or leave.
geth/utils/validation.py
Outdated
@@ -135,13 +145,37 @@ def validate_genesis_data(genesis_data: GenesisDataTypedDict) -> bool: | |||
raise PyGethValueError( | |||
f"error while validating genesis_data config field: {e}" | |||
) | |||
|
|||
return True |
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.
nit: I think we can probably leave this out and return None
in the signature. Generally I don't expect validation methods to return anything, except raise if appropriate.
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.
fixed! makes sense
03a8386
to
294691c
Compare
What was wrong?
Filling
geth_kwargs
defaults by just creating a model from the dict works fine because its only member that is a dict (suffix_kwargs
) defaults toNone
.genesis_data["config"]
does have default values, so just creating aGenesisData
model from the dict does not fill the sub-dict properly.How was it fixed?
Added a
fill_default_genesis_data
function and test.Todo:
Cute Animal Picture