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

Improve client compatibility #41

Merged
merged 5 commits into from
Apr 28, 2023
Merged

Conversation

TobiWo
Copy link
Owner

@TobiWo TobiWo commented Apr 23, 2023

/release

Summary

Currently eth-duties only supports Teku and Lighthouse due to the fact the requests were not implemented following the official beacon api spec. This PR will add compatibility for:

  • Nimbus
  • Lodestar
  • Prysm

Note on Lodestar

I needed to add a specific change how requests are sent because Lodestar does not support URI percent-encoding currently. This was reported as bug in the Lodestar github repository. If it will be added/fixed, I will adapt eth-duties as well (back to usage of percent-encoding for all requests).

Update on Lodestar specifics

It turns out that Lodestar is the only client implementing percent-encoding correctly or using the right library for handling requests. The , is a reserved delimiting character which therefore should not be encoded. Fur further details please follow the discussion in the aforementioned issue in the Lodestar repository.

Closes #32

@TobiWo TobiWo added the enhancement New feature or request label Apr 23, 2023
duties/constants/program.py Outdated Show resolved Hide resolved
@TobiWo TobiWo force-pushed the feature/improve-client-compatibility branch from 9745bc3 to d9e45e4 Compare April 24, 2023 17:15
@TobiWo
Copy link
Owner Author

TobiWo commented Apr 24, 2023

@nflaig Thanks for reviewing. I'm currently waiting for @LuisNaldo7 to finish the review on #40 . If this is done and I rebased the changes into this branch I would ask you to approve this PR which will then trigger a new release 🙌 .

@nflaig
Copy link
Contributor

nflaig commented Apr 24, 2023

@TobiWo sounds good, will give it a final review after rebase

LuisNaldo7
LuisNaldo7 previously approved these changes Apr 26, 2023
@TobiWo TobiWo force-pushed the feature/improve-client-compatibility branch from d9e45e4 to 3baa408 Compare April 28, 2023 07:04
@TobiWo TobiWo marked this pull request as ready for review April 28, 2023 07:04
@TobiWo
Copy link
Owner Author

TobiWo commented Apr 28, 2023

@nflaig Sorry for the delay. This PR is now ready for review.

nflaig
nflaig previously approved these changes Apr 28, 2023
Copy link
Contributor

@nflaig nflaig left a comment

Choose a reason for hiding this comment

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

lgtm

duties/protocol/request.py Outdated Show resolved Hide resolved
@TobiWo
Copy link
Owner Author

TobiWo commented Apr 28, 2023

@nflaig Unfortunately a review is only valid when you have write access. Therefore, if you like, I would add you as collaborator so you are able to do valid reviews in the future. Don't know if you have time for that?

@TobiWo TobiWo merged commit f381527 into main Apr 28, 2023
@TobiWo TobiWo deleted the feature/improve-client-compatibility branch April 28, 2023 19:05
@nflaig
Copy link
Contributor

nflaig commented Apr 28, 2023

@TobiWo Thanks for the offer but I won't be able to find the time right now to familiarize myself with the code base to do proper reviews. That being said, feel free to just tag me on any PRs related to Lodestar or any other CL as well if you feel like my feedback would be useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add compatibility for prysm, nimbus and lodestar
3 participants