Flattening deposit snapshot path config options #7869
Conversation
lucassaldanha
changed the title
| public Optional<String> getCheckpointSyncDepositSnapshotUrl() { | ||
| return checkpointSyncDepositSnapshotUrl; | ||
| } |
There was a problem hiding this comment.
This method is not being used atm. It will be relevant in my follow-up PR.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| import tech.pegasys.teku.spec.TestSpecFactory; | ||
| import tech.pegasys.teku.spec.networks.Eth2Network; | ||
|
|
||
| class PowchainConfigurationTest { |
Check notice
Code scanning / CodeQL
Unused classes and interfaces
There was a problem hiding this comment.
I think it could be not the best way to mix two options parsing in DepositOptions. We'd better set all input from appropriate options to configuration. Validate and throw in validate() part of builder and set as many fields as we need for decision in final configuration. It will look better and easier to maintain and analyze. Also we could add DepositSnapshot inner class in PowChainConfiguration for better readability, so the options could be accessed like tekuConfig.powchain().depositSnapshot().enabled() or tekuConfig.powchain().depositSnapshot().bundledPath()
|
|
||
| private boolean parseDepositSnapshotEnabled() { | ||
| return depositSnapshotEnabled; | ||
| if (parseResult.hasMatchedOption("--Xdeposit-snapshot")) { |
There was a problem hiding this comment.
I think the logic of lines starting from here could be implemented without using low level picocli api. We should have values set here already
| final CommandLine.ParseResult parseResult = commandSpec.commandLine().getParseResult(); | ||
|
|
||
| final String checkpointSyncUrlCliOption = "--checkpoint-sync-url"; | ||
| final boolean checkpointSyncUrlSet = parseResult.hasMatchedOption(checkpointSyncUrlCliOption); |
There was a problem hiding this comment.
I don't like this way of using data from other options module. Is it possible to implement the same logic more casual way by checking options here and there in TekuConfiguration.java? As initialState could be set not only by "--checkpoint-sync-url" we could add the property checkpointSyncUrlSet as boolean to `Eth2NetworkConfiguration to clarify this case.
| b.useMissingDepositEventLogging(useMissingDepositEventLogging); | ||
| b.customDepositSnapshotPath(depositSnapshotPath); | ||
| b.depositSnapshotEnabled(depositSnapshotEnabled); | ||
| if (checkpointSyncUrlSet) { |
There was a problem hiding this comment.
It could be very difficult to maintain and follow when foreign option is checked and set here.
lucassaldanha
force-pushed
the
teku-7715
branch
from
dc5bc38 to
2e2b69e
Compare
PR Description
This PR is part 1 of the deposit snapshots changes support for checkpoint sync url.
In this PR, we are breaking the deposit snapshot path option into 3 separate options:
At this point, we are not changing our existing behaviour:
default behaviour: using the bundled deposit tree snapshot by default
when using
--Xdeposit-snapshot: don't use the bundled version and use the path specified on--Xdeposit-snapshotFixed Issue(s)
related to #7715
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog