Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

Add wrapper classes for config section of genesis file #208

Merged
merged 7 commits into from
Nov 1, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
36 changes: 36 additions & 0 deletions config/build.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* Copyright 2018 ConsenSys AG.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
* 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.
*/

apply plugin: 'java-library'

jar {
baseName 'pantheon-config'
manifest {
attributes('Implementation-Title': baseName,
'Implementation-Version': project.version)
}
}

dependencies {
implementation 'com.fasterxml.jackson.core:jackson-databind'
implementation 'io.vertx:vertx-core'
implementation 'org.apache.logging.log4j:log4j-api'

runtime 'org.apache.logging.log4j:log4j-core'

testImplementation project(':testutil')

testImplementation 'org.assertj:assertj-core'
testImplementation 'org.mockito:mockito-core'
testImplementation 'junit:junit'
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* Copyright 2018 ConsenSys AG.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
* 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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my initial inclination but two things persuaded me otherwise:

  1. 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).
  2. 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 GenesisConfigOptions without also exposing the inner workings of the config which is what we were trying to encapsulate in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


import io.vertx.core.json.JsonObject;

public class CliqueConfigOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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 ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I put them in because other people did ... the defaults do now form part of our spec, maybe not a bad thing.

private static final int DEFAULT_BLOCK_PERIOD_SECONDS = 15;

private final JsonObject cliqueConfigRoot;

public CliqueConfigOptions(final JsonObject cliqueConfigRoot) {
this.cliqueConfigRoot = cliqueConfigRoot;
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Where do you see validation living then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 long getEpochLength() {
return cliqueConfigRoot.getLong("epochLength", DEFAULT_EPOCH_LENGTH);
}

public int getBlockPeriodSeconds() {
return cliqueConfigRoot.getInteger("blockPeriodSeconds", DEFAULT_BLOCK_PERIOD_SECONDS);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
/*
* Copyright 2018 ConsenSys AG.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
* 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;

import java.util.OptionalInt;
import java.util.OptionalLong;

import io.vertx.core.json.JsonObject;

public class GenesisConfigOptions {

private static final String ETHASH_CONFIG_KEY = "ethash";
private static final String IBFT_CONFIG_KEY = "ibft";
private static final String CLIQUE_CONFIG_KEY = "clique";
private final JsonObject configRoot;

private GenesisConfigOptions(final JsonObject configRoot) {
this.configRoot = configRoot != null ? configRoot : new JsonObject();
}

public static GenesisConfigOptions fromGenesisConfig(final String genesisConfig) {
return fromGenesisConfig(new JsonObject(genesisConfig));
}

public static GenesisConfigOptions fromGenesisConfig(final JsonObject genesisConfig) {
return new GenesisConfigOptions(genesisConfig.getJsonObject("config"));
}

public boolean isEthHash() {
return configRoot.containsKey(ETHASH_CONFIG_KEY);
}

public boolean isIbft() {
return configRoot.containsKey(IBFT_CONFIG_KEY);
}

public boolean isClique() {
return configRoot.containsKey(CLIQUE_CONFIG_KEY);
}

public IbftConfigOptions getIbftConfigOptions() {
return isIbft()
? new IbftConfigOptions(configRoot.getJsonObject(IBFT_CONFIG_KEY))
: IbftConfigOptions.DEFAULT;
}

public CliqueConfigOptions getCliqueConfigOptions() {
return isClique()
? new CliqueConfigOptions(configRoot.getJsonObject(CLIQUE_CONFIG_KEY))
: CliqueConfigOptions.DEFAULT;
}

public OptionalLong getHomesteadBlockNumber() {
return getOptionalLong("homesteadBlock");
}

public OptionalLong getDaoForkBlock() {
return getOptionalLong("daoForkBlock");
}

public OptionalLong getTangerineWhistleBlockNumber() {
return getOptionalLong("eip150Block");
}

public OptionalLong getSpuriousDragonBlockNumber() {
return getOptionalLong("eip158Block");
}

public OptionalLong getByzantiumBlockNumber() {
return getOptionalLong("byzantiumBlock");
}

public OptionalLong getConstantinopleBlockNumber() {
return getOptionalLong("constantinopleBlock");
}

public OptionalInt getChainId() {
return configRoot.containsKey("chainId")
? OptionalInt.of(configRoot.getInteger("chainId"))
: OptionalInt.empty();
}

private OptionalLong getOptionalLong(final String key) {
return configRoot.containsKey(key)
? OptionalLong.of(configRoot.getLong(key))
: OptionalLong.empty();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/*
* Copyright 2018 ConsenSys AG.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
* 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;

import io.vertx.core.json.JsonObject;

public class IbftConfigOptions {

public static final IbftConfigOptions DEFAULT = new IbftConfigOptions(new JsonObject());

private static final long DEFAULT_EPOCH_LENGTH = 30_000;
Copy link
Contributor

Choose a reason for hiding this comment

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

same comments as for clique.

private static final int DEFAULT_BLOCK_PERIOD_SECONDS = 1;
private static final int DEFAULT_ROUND_EXPIRY_MILLISECONDS = 10000;

private final JsonObject ibftConfigRoot;

public IbftConfigOptions(final JsonObject ibftConfigRoot) {
this.ibftConfigRoot = ibftConfigRoot;
}

public long getEpochLength() {
return ibftConfigRoot.getLong("epochLength", DEFAULT_EPOCH_LENGTH);
}

public int getBlockPeriodSeconds() {
return ibftConfigRoot.getInteger("blockPeriodSeconds", DEFAULT_BLOCK_PERIOD_SECONDS);
}

public int getRequestTimeoutMillis() {
return ibftConfigRoot.getInteger("requestTimeout", DEFAULT_ROUND_EXPIRY_MILLISECONDS);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/*
* Copyright 2018 ConsenSys AG.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
* 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;

import static java.util.Collections.emptyMap;
import static java.util.Collections.singletonMap;
import static org.assertj.core.api.Assertions.assertThat;

import java.util.Map;

import io.vertx.core.json.JsonObject;
import org.junit.Test;

public class CliqueConfigOptionsTest {

private static final long EXPECTED_DEFAULT_EPOCH_LENGTH = 30_000;
private static final int EXPECTED_DEFAULT_BLOCK_PERIOD = 15;

@Test
public void shouldGetEpochLengthFromConfig() {
final CliqueConfigOptions config = fromConfigOptions(singletonMap("epochLength", 10_000));
Copy link
Contributor

Choose a reason for hiding this comment

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

So - does this imply you an create a valid clique config options,even if period is missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

assertThat(config.getEpochLength()).isEqualTo(10_000);
}

@Test
public void shouldFallbackToDefaultEpochLength() {
final CliqueConfigOptions config = fromConfigOptions(emptyMap());
assertThat(config.getEpochLength()).isEqualTo(EXPECTED_DEFAULT_EPOCH_LENGTH);
}

@Test
public void shouldGetDefaultEpochLengthFromDefaultConfig() {
assertThat(CliqueConfigOptions.DEFAULT.getEpochLength())
.isEqualTo(EXPECTED_DEFAULT_EPOCH_LENGTH);
}

@Test
public void shouldGetBlockPeriodFromConfig() {
final CliqueConfigOptions config = fromConfigOptions(singletonMap("blockPeriodSeconds", 5));
assertThat(config.getBlockPeriodSeconds()).isEqualTo(5);
}

@Test
public void shouldFallbackToDefaultBlockPeriod() {
final CliqueConfigOptions config = fromConfigOptions(emptyMap());
assertThat(config.getBlockPeriodSeconds()).isEqualTo(EXPECTED_DEFAULT_BLOCK_PERIOD);
}

@Test
public void shouldGetDefaultBlockPeriodFromDefaultConfig() {
assertThat(CliqueConfigOptions.DEFAULT.getBlockPeriodSeconds())
.isEqualTo(EXPECTED_DEFAULT_BLOCK_PERIOD);
}

private CliqueConfigOptions fromConfigOptions(final Map<String, Object> cliqueConfigOptions) {
return GenesisConfigOptions.fromGenesisConfig(
new JsonObject(singletonMap("config", singletonMap("clique", cliqueConfigOptions))))
.getCliqueConfigOptions();
}
}
Loading