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 index to activationStatus and MultipleValidatorStatus response #6026

Merged
merged 17 commits into from May 29, 2020

Conversation

0xKiwi
Copy link
Contributor

@0xKiwi 0xKiwi commented May 28, 2020

What type of PR is this?
API Improvement

What does this PR do? Why is it needed?
This PR modifies the activationStatus and multipleValidatorStatus to also return the indice, for the validator to log out the indices where there is room for them.

status.go was also a bit messy, so I've reordered some parameters and removed some unneeded parameters.

@0xKiwi 0xKiwi requested a review from a team as a code owner May 28, 2020 17:40
@codecov
Copy link

codecov bot commented May 28, 2020

Codecov Report

Merging #6026 into master will decrease coverage by 0.02%.
The diff coverage is 63.73%.

@@            Coverage Diff             @@
##           master    #6026      +/-   ##
==========================================
- Coverage   59.57%   59.54%   -0.03%     
==========================================
  Files         320      320              
  Lines       26970    27112     +142     
==========================================
+ Hits        16067    16145      +78     
- Misses       8713     8775      +62     
- Partials     2190     2192       +2     

@0xKiwi 0xKiwi added the Ready For Review A pull request ready for code review label May 28, 2020
ctx, span := trace.StartSpan(ctx, "validatorServer.validatorStatus")
defer span.End()

// Using farFutureEpoch as the default value for index, in case the validators index cannot be determined.
Copy link
Member

Choose a reason for hiding this comment

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

It's confusing to use farFutureEpoch as default value for index. One's unit is Epoch and one's unit is ValidatorIndex

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, is there a better way we could do this, maybe through a boolean ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, That would require changing the protobufs as well no? Not sure how to go about this...

Copy link
Member

Choose a reason for hiding this comment

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

headState, err := vs.HeadFetcher.HeadState(ctx)
if err != nil {
return nil, status.Error(codes.Internal, "Could not get head state")
}
return vs.validatorStatus(ctx, req.PublicKey, headState), nil
vStatus, _ := vs.validatorStatus(ctx, headState, req.PublicKey)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unhandled error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is the return indice, the function doesn't return an error.

ctx, span := trace.StartSpan(ctx, "validatorServer.validatorStatus")
defer span.End()

// Using farFutureEpoch as the default value for index, in case the validators index cannot be determined.
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, is there a better way we could do this, maybe through a boolean ?

@rauljordan rauljordan merged commit d35531c into master May 29, 2020
@delete-merged-branch delete-merged-branch bot deleted the add-indices-resp branch May 29, 2020 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants