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

[Merged by Bors] - Add remotekey API support #3162

Closed
wants to merge 10 commits into from

Conversation

tthebst
Copy link
Contributor

@tthebst tthebst commented Apr 18, 2022

Issue Addressed

#3068

Proposed Changes

Adds support for remote key API.

Additional Info

Needed to add is_local_keystore argument to delete_definition_and_keystore to know if we want to delete local or remote key. Previously this wasn't necessary because remotekeys(web3signers) could be deleted.

@tthebst tthebst changed the title Tim/add remote keys Add remotekey API support Apr 18, 2022
@michaelsproul michaelsproul self-requested a review May 5, 2022 07:52
@michaelsproul michaelsproul added the ready-for-review The code is ready for review label May 5, 2022
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

This is absolutely fantastic, thank you so much for taking the time to implement it ❤️

I think we're super close to being able to merge. There's just one minor change required to the list keys API, and some nitpicks.

Thanks again!

validator_client/src/http_api/create_validator.rs Outdated Show resolved Hide resolved
validator_client/src/http_api/mod.rs Outdated Show resolved Hide resolved
validator_client/src/http_api/remotekeys.rs Outdated Show resolved Hide resolved
validator_client/src/http_api/remotekeys.rs Outdated Show resolved Hide resolved
common/eth2/src/lighthouse_vc/std_types.rs Outdated Show resolved Hide resolved
validator_client/src/http_api/remotekeys.rs Show resolved Hide resolved
validator_client/src/http_api/tests.rs Outdated Show resolved Hide resolved
validator_client/src/http_api/tests/keystores.rs Outdated Show resolved Hide resolved
@michaelsproul michaelsproul 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 May 6, 2022
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Latest changes look great! There are just two things we need to do before merging:

  1. Remove the // DO NOT MERGE section
  2. Satisfy Clippy: https://github.com/sigp/lighthouse/runs/6343319556?check_suite_focus=true

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels May 9, 2022
@michaelsproul
Copy link
Member

LFG! 🚀

bors r+

bors bot pushed a commit that referenced this pull request May 9, 2022
## Issue Addressed

#3068

## Proposed Changes

Adds support for remote key API.

## Additional Info

Needed to add `is_local_keystore`  argument to `delete_definition_and_keystore` to know if we want to delete local or remote key. Previously this wasn't necessary because remotekeys(web3signers) could be deleted.
@bors bors bot changed the title Add remotekey API support [Merged by Bors] - Add remotekey API support May 9, 2022
@bors bors bot closed this May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants