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

Remove get_state_root from state transition doc #1015

Merged
merged 4 commits into from
May 1, 2019
Merged

Conversation

JustinDrake
Copy link
Collaborator

@JustinDrake JustinDrake commented May 1, 2019

Remove get_state_root from the state transition function spec because it is not used by the state transition function. (Unused code affects spec testability and conciseness. Also note that get_state_root is slot-based which is inconsistent with its get_block_root counterpart.)

The get_state_root helper is used by the test suite and is now defined there. If useful to validators beyond the test suite, it may make sense to define get_state_root in the validator doc.

JustinDrake and others added 3 commits May 1, 2019 09:24
Remove `get_state_root` from the state transition function spec because it is not used by the state transition function.
Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

LGTM! (I moved it to test_libs/pyspec/tests/helpers.py)

@JustinDrake JustinDrake added general:presentation Presentation (as opposed to content) scope:CI/tests/pyspec labels May 1, 2019
@djrtwo
Copy link
Contributor

djrtwo commented May 1, 2019

Generally agree except with the "Also note that get_state_root is slot-based which is inconsistent with its get_block_root counterpart" argument. We store state roots per slot in state so makes sense to give slot granular access :)

@djrtwo djrtwo changed the base branch from dev to master May 1, 2019 23:04
@djrtwo djrtwo merged commit 715dc32 into master May 1, 2019
@hwwhww hwwhww deleted the JustinDrake-patch-11 branch May 9, 2019 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
general:presentation Presentation (as opposed to content) scope:CI/tests/pyspec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants