Skip to content

[CHORE] BlockchainConfig as a case class #674

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

Merged
merged 5 commits into from
Sep 17, 2020

Conversation

ntallar
Copy link

@ntallar ntallar commented Sep 16, 2020

Description

Changes BlockchainConfig from being a trait to a case class.

As seen on a couple of examples here, this will:

  • Allow us to use the default copy to only change the parts of the configuration on tests that applies to each
  • Follow our current convention of having all our configurations as case classes

@ntallar ntallar marked this pull request as ready for review September 16, 2020 13:26
@ntallar ntallar mentioned this pull request Sep 16, 2020
3 tasks
@@ -43,6 +43,8 @@ object ScenarioSetup {

abstract class ScenarioSetup(_vm: VMImpl, scenario: BlockchainScenario) {

import BlockchainTestConfig._
Copy link
Contributor

Choose a reason for hiding this comment

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

is this needed?

Copy link
Author

Choose a reason for hiding this comment

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

It is, as baseBlockchainConfig and withSkippedPoWValidationBlockchainConfig functions use the BlockchainConfig's defined there

@@ -172,43 +172,43 @@ class EtcHandshakerSpec extends FlatSpec with Matchers {
override val appStateStorage: AppStateStorage = TestSetup.this.storagesInstance.storages.appStateStorage
}

override lazy val blockchainConfig = new BlockchainConfig {
override lazy val blockchainConfig = BlockchainConfig (
Copy link
Contributor

@mirkoAlic mirkoAlic Sep 16, 2020

Choose a reason for hiding this comment

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

(minor) I guess here you could use the blockchainConfig.copy given EphemBlockchainTestSetup has access to default instance.

Copy link
Contributor

@mirkoAlic mirkoAlic left a comment

Choose a reason for hiding this comment

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

LGTM!

@mirkoAlic mirkoAlic removed the request for review from KonradStaniec September 16, 2020 19:19
@mirkoAlic mirkoAlic merged commit 022a5ae into develop Sep 17, 2020
@mirkoAlic mirkoAlic deleted the chore/blockchainconfig-as-caseclass branch September 17, 2020 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants