Skip to content
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

Add beacon chain helper functions #1339

Closed
hwwhww opened this issue Oct 1, 2018 · 7 comments
Closed

Add beacon chain helper functions #1339

hwwhww opened this issue Oct 1, 2018 · 7 comments
Assignees
Labels

Comments

@hwwhww
Copy link
Contributor

hwwhww commented Oct 1, 2018

What is wrong?

Part of #930

  • The helper functions in beacon_chain are all pure functions with a config: Dict[string, Any] parameter.
  • Some helpers functions are like ~90% similar to the spec as a reference implementation. But to fit Py-EVM paradigm, we may have to refactor a lot.

How can it be fixed

It may take multiple steps:

  1. Move all pure functions to eth/beacon/helpers.py - I have a commit here: hwwhww@5724a7a
  2. Introduce eth.beacon.db.chain.BeaconChainDB
  3. Introduce eth.beacon.state_machines.base.BaseStateMachine(Configurable)
  4. Introduce eth.beacon.chains.chain.BeaconChain
  5. Tear down helpers: move some of them to eth.beacon.state_machine.base.BaseStateMachine or being static functions / properties in eth.beacon.types.* classes.

cc @djrtwo @pipermerriam

@hwwhww hwwhww added the eth2.0 label Oct 1, 2018
@pipermerriam
Copy link
Member

  • The config object can probably be updated to be a typing.NamedTuple for stronger type safety.
  • Modifying the helper functions to be non mutative looks like something that will go relatively quickly.
  • Writing tests for all of these is probably the largest task (take care to not waste time writing exhaustive tests on things that are likely to change).

@pipermerriam
Copy link
Member

Also, ideally, rather than passing a config object around through these various helper functions, it would probably be better to just explicitly pass the specific configuration values as individual parameters.

@djrtwo
Copy link
Contributor

djrtwo commented Oct 1, 2018

config in this context can be seen as "global" parameters for the particular instantiation of the chain/fork. These would be constants probably defined in beacon.state_machine.forks.fork_name.constants, right?

edit: and agreed on passing individual config params as needed

@pipermerriam
Copy link
Member

On second thought, I'm curious if we can get away with the same pattern that the existing code uses, placing these in a constants file and importing them as needed. For parameters that end up changing for various forks, we can place them in eth.beacon.forks.fork_name.constants (as danny suggested)

@hwwhww
Copy link
Contributor Author

hwwhww commented Oct 1, 2018

Yes, that makes sense. We will have Configurable StateMachine to handle these chain configurations.

@djrtwo
Copy link
Contributor

djrtwo commented Oct 1, 2018

Yeah, I agree the import is the way to go on these

This was referenced Oct 1, 2018
@hwwhww hwwhww self-assigned this Oct 4, 2018
hwwhww added a commit to hwwhww/py-evm that referenced this issue Oct 4, 2018
hwwhww added a commit to hwwhww/py-evm that referenced this issue Oct 4, 2018
hwwhww added a commit to hwwhww/py-evm that referenced this issue Oct 4, 2018
hwwhww added a commit to hwwhww/py-evm that referenced this issue Oct 6, 2018
hwwhww added a commit to hwwhww/py-evm that referenced this issue Oct 6, 2018
This was referenced Oct 8, 2018
@hwwhww
Copy link
Contributor Author

hwwhww commented Oct 9, 2018

closed via #1341 and #1381

@hwwhww hwwhww closed this as completed Oct 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants