-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
EIP-7549 gRPC (part 1) #14055
EIP-7549 gRPC (part 1) #14055
Conversation
@@ -67,45 +69,107 @@ func (bs *Server) ListAttestations( | |||
return nil, status.Errorf(codes.Internal, "Could not fetch attestations: %v", err) | |||
} | |||
case *ethpb.ListAttestationsRequest_Epoch: | |||
if q.Epoch >= params.BeaconConfig().ElectraForkEpoch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this listed in the beacon api spec somewhere to have a successful response that's sort of empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is gRPC, not Beacon API
return atts, nil | ||
} | ||
|
||
func blockIndexedAttestations[T ethpb.IndexedAtt]( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice use of generics
// The server may return an empty list when no attestations match the given | ||
// filter criteria. This RPC should not return NOT_FOUND. Only one filter | ||
// criteria should be used. | ||
func (bs *Server) ListAttestationsElectra(ctx context.Context, req *ethpb.ListAttestationsRequest) (*ethpb.ListAttestationsElectraResponse, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the logic below is copied, is there a way to reduce this? or probably not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wonder if we can use the interface here or a utility function to build the response to reduce the copy paste here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's a bit duplicated but the returned types are non-compatible. I would probably have to create an interface that both return types implement and given that we are getting close to deprecating gRPC, I don't think it's worth it. There's also close to 0 chance that there will be a third similar function as I don't expect attestations to change again in the F-fork.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conditionally approving but at the ACDC there was discussions on changing this EIP further need to find where the followup issue is.
What type of PR is this?
Feature
What does this PR do? Why is it needed?
Implements the following gRPC endpoints:
ListAttestationsElectra
ListIndexedAttestationsElectra
AttestationPoolElectra
SubmitAttesterSlashingElectra
Logic shared by pre and post-Electra functions are extracted to generic helper functions.