-
Notifications
You must be signed in to change notification settings - Fork 130
Add wrapper classes for config section of genesis file #208
Changes from 5 commits
bb64a3b
69709bb
ac30102
3c7eea4
9e56c2a
10a771f
e53bbb1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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; | ||
|
||
import io.vertx.core.json.JsonObject; | ||
|
||
public class CliqueConfigOptions { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where do you see validation living then? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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(); | ||
} | ||
} |
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.
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.
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:
PantheonController.fromConfig
).GenesisConfigOptions
without 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.
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.