Add wrapper classes for config section of genesis file #208
Conversation
…y in multiple places. Fix discrepancy in how CliqueProtocolSchedule and CliquePantheonController loaded the block period configuration.
…fig as optionals instead of providing the default.
|
|
||
| import io.vertx.core.json.JsonObject; | ||
|
|
||
| public class CliqueConfigOptions { |
There was a problem hiding this comment.
Could/should this have a base interface, then this is the "Json" version of it?
If you did that, the default values would live in the base class.
There was a problem hiding this comment.
Seems overly complicated to be honest. I don't see any real need to support multiple genesis config formats now and it would be easy to extract a base class or interface if we ever do want to, so I would apply You Ain't Gonna Need It here and just keep things simple.
|
|
||
| public static final CliqueConfigOptions DEFAULT = new CliqueConfigOptions(new JsonObject()); | ||
|
|
||
| private static final long DEFAULT_EPOCH_LENGTH = 30_000; |
There was a problem hiding this comment.
tbh - I really hate default values for this - people should configure their network to actually work - not just get lucky that things came out well.
These defaults would need to become part of the clique spec to ensure unconfigured clients would interact correctly ...
There was a problem hiding this comment.
I don't mean this to sound defensive, but I didn't introduce those defaults, you did. I just moved them. :)
In general though I think it makes a lot of sense to have defaults for these config values and it seems like pretty common behaviour in other clients. The spec does explicitly recommend 30000 block epoch length.
There was a problem hiding this comment.
Yeah, I put them in because other people did ... the defaults do now form part of our spec, maybe not a bad thing.
| private final JsonObject cliqueConfigRoot; | ||
|
|
||
| public CliqueConfigOptions(final JsonObject cliqueConfigRoot) { | ||
| this.cliqueConfigRoot = cliqueConfigRoot; |
There was a problem hiding this comment.
nit/ymmv: is it worth pre-parsing the config such that if its bad, we fail at startup rather than when we go to use the config?
There was a problem hiding this comment.
I don't think these config objects have the primary responsibility for validation - they do some incidental validation in terms of "must be a number" but even then a lot of the time they just return a String and leave parsing to the caller.
The values from these config objects are actually used during startup anyway and it would be a smell to me if the config objects continued to live on past startup.
There was a problem hiding this comment.
Where do you see validation living then?
There was a problem hiding this comment.
Right now, the same place it currently does - at the point of usage. I can see some benefit in having specific validation for config objects but I think that would be a separate validator object that uses these config objects to extract the data to validate. I think this is a big enough PR without attempting to rework validation at the same time.
There was a problem hiding this comment.
Happy to revisit it later in another PR. My thoughts to carry forward are I think the validation should come earlier than it's point of usage so whatever is being used can be treated as untainted.
| } | ||
|
|
||
| public boolean isEthHash() { | ||
| return configRoot.containsKey("ethash"); |
There was a problem hiding this comment.
is there a reason this doesn't do the same as IBFT/clique with regard a class constant?
There was a problem hiding this comment.
That string was only referenced once so locality of reference was very useful, but will move it for consistency since it is very much like the IBFT and Clique keys so should follow the same pattern.
|
|
||
| public static final IbftConfigOptions DEFAULT = new IbftConfigOptions(new JsonObject()); | ||
|
|
||
| private static final long DEFAULT_EPOCH_LENGTH = 30_000; |
There was a problem hiding this comment.
same comments as for clique.
|
|
||
| @Test | ||
| public void shouldGetEpochLengthFromConfig() { | ||
| final CliqueConfigOptions config = fromConfigOptions(singletonMap("epochLength", 10_000)); |
There was a problem hiding this comment.
So - does this imply you an create a valid clique config options,even if period is missing?
There was a problem hiding this comment.
Yes, if it's not specified it would use the default. But mostly it just means CliqueConfigOptions isn't doing validation and I'm taking advantage of that to keep the test as simple as possible.
| * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations under the License. | ||
| */ | ||
| package tech.pegasys.pantheon.config; |
There was a problem hiding this comment.
should this be in consensus.clique? Having the config class knowing about the requirements of all modules and the modules being dependent on config seems to be a step in the wrong direction with regards to the modular goals of the application.
There was a problem hiding this comment.
That was my initial inclination but two things persuaded me otherwise:
- Knowledge of the specific consensus algorithms that are available and how they fit into the config file is already outside of the specific consensus modules (see
PantheonController.fromConfig). - If we try to model the config as separate domains with each consensus implementation having it's config objects in separate modules, there's no way to expose those separate config objects from the top level
GenesisConfigOptionswithout also exposing the inner workings of the config which is what we were trying to encapsulate in the first place.
There was a problem hiding this comment.
Those don't quite persuade me, but not going to block the improvement. Can revisit it later if the config starts getting in the way of the architecture requirements.
PR description
Introduce classes to wrap the config section of a genesis file instead of accessing it directly in multiple places. This provides a centralised place for accessing the config and makes it simple to test that the config was loaded correctly.
Fixes discrepancy in how CliqueProtocolSchedule and CliquePantheonController loaded the block period configuration. They were looking for different keys with different default values for what should be the same configuration item.