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

EIP-7549 gRPC (part 1) #14055

Merged
merged 18 commits into from
May 31, 2024
Merged

EIP-7549 gRPC (part 1) #14055

merged 18 commits into from
May 31, 2024

Conversation

rkapka
Copy link
Contributor

@rkapka rkapka commented May 29, 2024

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.

@rkapka rkapka added Ready For Review A pull request ready for code review Electra electra hardfork labels May 29, 2024
@rkapka rkapka requested a review from a team as a code owner May 29, 2024 02:23
@rkapka rkapka added the API Api related tasks label May 29, 2024
@@ -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 {
Copy link
Contributor

@james-prysm james-prysm May 29, 2024

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?

Copy link
Contributor Author

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](
Copy link
Contributor

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) {
Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

@james-prysm james-prysm left a 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.

@rkapka rkapka added this pull request to the merge queue May 31, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks May 31, 2024
@rkapka rkapka added this pull request to the merge queue May 31, 2024
Merged via the queue into develop with commit 968e82b May 31, 2024
16 of 17 checks passed
@rkapka rkapka deleted the eip-7549-grpc1 branch May 31, 2024 18:03
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Api related tasks Electra electra hardfork Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants