-
Notifications
You must be signed in to change notification settings - Fork 969
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
Address Review of v0.12 #6179
Address Review of v0.12 #6179
Conversation
@@ -211,19 +217,22 @@ func (vs *Server) SubscribeCommitteeSubnets(ctx context.Context, req *ethpb.Comm | |||
return uint64(len(vals)), nil | |||
} | |||
|
|||
// run first request | |||
// request the head validator indices of |
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.
Why are the comments formatted like this?
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.
ok changed
@@ -195,11 +198,14 @@ func (vs *Server) ProposeAttestation(ctx context.Context, att *ethpb.Attestation | |||
|
|||
// SubscribeCommitteeSubnets subscribes to the committee ID subnet given subscribe request. | |||
func (vs *Server) SubscribeCommitteeSubnets(ctx context.Context, req *ethpb.CommitteeSubnetsSubscribeRequest) (*ptypes.Empty, error) { | |||
ctx, span := trace.StartSpan(ctx, "AttesterServer.SubscribeCommitteeSubnets") | |||
defer span.End() | |||
|
|||
if len(req.Slots) != len(req.CommitteeIds) && len(req.CommitteeIds) != len(req.IsAggregator) { | |||
return nil, status.Error(codes.InvalidArgument, "request fields are not the same length") | |||
} | |||
if len(req.Slots) == 0 { |
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.
what happen during genesis when attesters request for slot 0?
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.
req.Slots will still have len 1 in this case
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.
ah yes, i was thinking the actual slot number. my b!
…o tidy-up-comments
…into v0.12Review
What type of PR is this?
Review Cleanup
What does this PR do? Why is it needed?
Fixes some issues from review of #5614
Which issues(s) does this PR fix?
N.A
Other notes for review
N.A