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.

Sign up for GitHub

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

Jump to bottom

Standard Liveness Endpoint #4853

Merged
merged 8 commits into from
Nov 30, 2023
Merged

Standard Liveness Endpoint #4853

merged 8 commits into from
Nov 30, 2023

Conversation

Gua00va
Copy link
Contributor

@Gua00va Gua00va commented Oct 17, 2023 •
edited
Loading

Issue Addressed

Solves #4522

Proposed Changes

Validator Client now uses standard liveness endpoint /eth/v1/validator/liveness/{epoch} implemented here #4343

For reference : #4539

@Gua00va Gua00va marked this pull request as ready for review October 17, 2023 07:20
@jimmygchen jimmygchen added val-client Relates to the validator client binary ready-for-review The code is ready for review backwards-incompat Backwards-incompatible API change v4.6.0 ETA Q1 2024 labels Oct 17, 2023
@Gua00va
Copy link
Contributor Author

Gua00va commented Oct 18, 2023

@jimmygchen, Not sure why the test is failing. I haven't made any changes there.

@jimmygchen
Copy link
Member

Hi @Gua00va
It was a compilation error unrelated to your change and fixed in #4836.

@Gua00va
Copy link
Contributor Author

Gua00va commented Oct 18, 2023

Should work now!!!

jimmygchen
jimmygchen reviewed Oct 18, 2023
View reviewed changes
common/eth2/src/lib.rs Outdated Show resolved Hide resolved
@jimmygchen jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Oct 18, 2023
jimmygchen
jimmygchen requested changes Oct 18, 2023
View reviewed changes
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

Hi @Gua00va
Thanks for the PR! I've added some comments :)
I think we need to update the doppelganger_serivce to work with the standard response StandardLivenessResponseData.

@Gua00va
Copy link
Contributor Author

Gua00va commented Oct 19, 2023

Hi @Gua00va Thanks for the PR! I've added some comments :) I think we need to update the doppelganger_serivce to work with the standard response StandardLivenessResponseData.

@jimmygchen, I have made some changes. Please have a look.

@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Oct 19, 2023
jimmygchen
jimmygchen requested changes Nov 10, 2023
View reviewed changes
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

Hi @Gua00va sorry about the delay - I've added some more comments, mostly around efficiency but the overall changes look good to me!

@jimmygchen jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Nov 10, 2023
@Gua00va
Copy link
Contributor Author

Gua00va commented Nov 16, 2023

Hi @Gua00va sorry about the delay - I've added some more comments, mostly around efficiency but the overall changes look good to me!

@jimmygchen, I have made the required changes. Please have a look.

@jimmygchen jimmygchen removed the waiting-on-author The reviewer has suggested changes and awaits thier implementation. label Nov 16, 2023
jimmygchen
jimmygchen approved these changes Nov 16, 2023
View reviewed changes
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for your work and patience ☺️ @Gua00va
I've just triggered a CI run now.

@jimmygchen jimmygchen added the ready-for-merge This PR is ready to merge. label Nov 16, 2023
@Gua00va
Copy link
Contributor Author

Gua00va commented Nov 30, 2023

@jimmygchen, This failure doesn't seem to be due to my changes.

@jimmygchen
Copy link
Member

Yep I can confirm, thanks @Gua00va !

@jimmygchen jimmygchen merged commit 44aaf13 into sigp:unstable Nov 30, 2023
23 of 24 checks passed
@jimmygchen jimmygchen mentioned this pull request Dec 4, 2023
Closed
@chenrui333 chenrui333 mentioned this pull request Jan 28, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimmygchen jimmygchen jimmygchen approved these changes

Assignees
No one assigned
Labels
backwards-incompat Backwards-incompatible API change ready-for-merge This PR is ready to merge. v4.6.0 ETA Q1 2024 val-client Relates to the validator client binary
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Gua00va @jimmygchen