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

Crosslink in AttestationData #1044

Merged
merged 9 commits into from
May 8, 2019
Merged

Crosslink in AttestationData #1044

merged 9 commits into from
May 8, 2019

Conversation

JustinDrake
Copy link
Collaborator

@JustinDrake JustinDrake commented May 5, 2019

Substantive change:

  • Batch fields in AttestationData into a Crosslink
    • better encapsulation and reuse of Crosslink type
    • cleaner presentation (remove an awkward helper function, simpler code, 15 lines removed).

Cosmetic changes:

  • rename MAX_CROSSLINK_EPOCHS to MAX_EPOCHS_PER_CROSSLINK
  • avoid long lines (over 120 characters) in process_attestation

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

I like this! One suggestion before approve

I've always kind of wanted shard inside Crosslink even though they are ordered in state. Makes Crosslink more useful in other contexts.

yay

specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
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.

Since crosslink_data_root only be used in Crosslink, what do you think about renaming it to data_root? (I'm fine with both)

specs/core/0_beacon-chain.md Show resolved Hide resolved
specs/core/0_beacon-chain.md Show resolved Hide resolved
@JustinDrake
Copy link
Collaborator Author

Since crosslink_data_root only be used in Crosslink, what do you think about renaming it to data_root?

data_root is also used in the custody bit, but I think that's the right thing to do regardless :) I also think we should rename previous_crosslink_root to parent_crosslink_root. The reason is that there is a naming clash now with state.previous_crosslink_roots. For consistency this means reverting back to parent_block_root (as opposed to previous_block_root).

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! 👍

Maybe wait for more 👍 on renamings?

@djrtwo
Copy link
Contributor

djrtwo commented May 7, 2019

yeah, let me do one more pass

@djrtwo
Copy link
Contributor

djrtwo commented May 7, 2019

I cleaned up the validator guide (and fixed a couple of things there). Would appreciated a once over before we merge

@JustinDrake
Copy link
Collaborator Author

Side note: making the validator guide executable may be useful, if anything to help with testing.

@djrtwo
Copy link
Contributor

djrtwo commented May 7, 2019

Yeah, I've been thinking it would help keep things in sync for sure

@djrtwo djrtwo merged commit af2bb7d into dev May 8, 2019
@djrtwo djrtwo deleted the JustinDrake-patch-21 branch May 8, 2019 14:35
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.

3 participants