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 ProposerIndex() to BlindedBlocks #58

Merged
merged 4 commits into from
Jul 23, 2023

Conversation

GalRogozinski
Copy link
Contributor

@GalRogozinski GalRogozinski commented Jun 18, 2023

Description

Added missing method

@GalRogozinski
Copy link
Contributor Author

@mcdee will you review?
Thanks in adavnce :-)

@GalRogozinski
Copy link
Contributor Author

solved conflicts

btw @mcdee, I noticed you added a RandaoReveal() to api/versionedblindedbeaconblock.go but not to spec/versionedbeaconblock.go. Is this on purpose?

@mcdee
Copy link
Contributor

mcdee commented Jul 19, 2023

Apologies for the delay on this. I've been considering it, and my concern is that the interface is a subset of a beacon block rather than actually a beacon block. So, for example, there is no ExecutionPayload() method which is something that a user seeing a BeaconBlock would reasonably expect to be able to obtain.

In general it's better to have interfaces with fewer functions, ideally 1. Would it make sense to have interfaces as:

type BeaconBlockSlotProvider interface {
  Slot() (phase0.Slot, error)
}

type BeaconBlockProposerIndexProvider interface {
  ProposerIndex() (phase0.ValidatorIndex,error)
}

...

as that would allow a full range of interfaces that could cover both blinded and unblinded beacon blocks.

@GalRogozinski
Copy link
Contributor Author

I changed the description and title of the PR accordingly

@GalRogozinski GalRogozinski changed the title add BeaconBlock interface add ProposerIndex() to BlindedBlocks Jul 23, 2023
@mcdee mcdee merged commit b0e0e2c into attestantio:master Jul 23, 2023
2 checks passed
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