Skip to content
This repository has been archived by the owner on Jul 1, 2021. It is now read-only.

Move get_chain_config from TrinityConfig to Eth1AppConfig #771

Merged
merged 2 commits into from Jul 9, 2019
Merged

Move get_chain_config from TrinityConfig to Eth1AppConfig #771

merged 2 commits into from Jul 9, 2019

Conversation

cburgdorf
Copy link
Contributor

@cburgdorf cburgdorf commented Jul 8, 2019 •
edited

What was wrong?

  1. The TrinityConfig contains a get_chain_config API that is only meant to be used with the eth1 client. (Notice that the BeaconAppConfig does already contain a similar API for the specifically for the beacon chain)

  2. The name ChainConfig should be changed to Eth1ChainConfig to be aligned with the BeaconChainConfig type and doesn't spark wrong assumptions.

How was it fixed?

  1. Moved the get_chain_config API from the TrinityConfig to the Eth1AppConfig

  2. Renamed the ChainConfig type to Eth1ChainConfig

To-Do

  • Clean up commit history

Cute Animal Picture

put a cute animal picture link inside the parentheses

@cburgdorf cburgdorf force-pushed the christoph/refactor/get_chain_config branch from 4090702 to eea1191 Compare July 8, 2019 14:34
@cburgdorf cburgdorf force-pushed the christoph/refactor/get_chain_config branch from eea1191 to 7aa6b52 Compare July 8, 2019 14:41
@cburgdorf cburgdorf changed the title Christoph/refactor/get chain config Move get_chain_config from TrinityConfig to Eth1AppConfig Jul 8, 2019
@cburgdorf cburgdorf requested a review from carver July 8, 2019 15:13
@carver
Copy link
Contributor

carver commented Jul 8, 2019

Woah, what the 💩 is that animal?

carver
carver approved these changes Jul 8, 2019
View reviewed changes
Copy link
Contributor

@carver carver left a comment

Choose a reason for hiding this comment

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

LGTM

@cburgdorf
Copy link
Contributor Author

Hahaha, that is a Saiga antelope

@cburgdorf cburgdorf merged commit d4161cd into ethereum:master Jul 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@carver carver carver approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@cburgdorf @carver