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 validator voluntary exit endpoint #58

Merged
merged 5 commits into from
Mar 30, 2023

Conversation

jimmygchen
Copy link
Contributor

Addresses #57.

I've incorporated inputs from the discussions in the above issue and the R&D #api channel.

@jimmygchen jimmygchen marked this pull request as draft March 24, 2023 04:26
@jimmygchen
Copy link
Contributor Author

Converting back to draft as I just realised that I need to update the flows and use-cases docs.

@jimmygchen
Copy link
Contributor Author

I've just went through the docs and not sure if extra info is need for this endpoint - I think the description and spec of the endpoint seems adequate, but please let me know otherwise!

Copy link

@rkapka rkapka left a comment

Choose a reason for hiding this comment

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

Copying from Discord:

I don't really see the purpose of the confirmation query parameter. In CLI apps we usually issue a warning after the user initiates an operation, and confirming is a second step. The warning is there to allow the user to back out from the operation. The semantics of an API query parameter are different. We only have a single step, so you either go through with the operation when including the query param or you don't when not including it - the param doesn't add any value. I don't ever see a reason for anyone to invoke the endpoint without the query parameter because it will just fail (which, again, doesn't work that way in a CLI app). In my opinion it is redundant.

@dapplion
Copy link
Collaborator

Agree with the reasoning above, I would drop confirm param too

@jimmygchen
Copy link
Contributor Author

jimmygchen commented Mar 28, 2023

Agree with the reasoning above, I would drop confirm param too

Thanks @dapplion - there's some discussions on this topic in the #api channel on R&D, and it looks like the consensus so far is to drop the query param. The alternative proposal is to change the API, so that it returns a SignedVoluntaryObject and let the user send it to the Beacon API, making it a two step process. This would prevent a simple curl from triggering an irreversible operation, and also gives us flexibility to prepare an exit message ahead of time.

If there's no strong objections I'll update this PR shortly.

@jimmygchen
Copy link
Contributor Author

@dapplion and Adrian also raised that web3signer implements keymanager API but can't fetch the current epoch like VC does, so we'll have to make epoch a mandatory param.

@jimmygchen
Copy link
Contributor Author

Updated the endpoint based on discussion in api channel:

  • Return SignedVoluntaryExit object in response
  • Remove confirm query parameter
  • Make epoch query parameter mandary

@jimmygchen
Copy link
Contributor Author

jimmygchen commented Mar 29, 2023

Discussion from Discord (R&D, #api) - given that remote signers already supports signing voluntary exits, and does not implement the full keymanger API, this endpoint doesn't seem necessary for remote signer.

I've made the epoch query parameter optional, as validator client has knowledge of the current epoch and can set this as default. Also updated the tag description to explicitly mention sections that are "Validator Client only" - Fee recipient, Gas Limit & Voluntary exits.

keymanager-oapi.yaml Outdated Show resolved Hide resolved
keymanager-oapi.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@rolfyone rolfyone left a comment

Choose a reason for hiding this comment

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

LGTM

@rolfyone rolfyone merged commit ee334a4 into ethereum:master Mar 30, 2023
@jimmygchen jimmygchen deleted the val-voluntary-exit branch March 30, 2023 01:29
bors bot pushed a commit to sigp/lighthouse that referenced this pull request Apr 3, 2023
## Issue Addressed

Addresses #4117 

## Proposed Changes

See ethereum/keymanager-APIs#58 for proposed API specification.

## TODO

- [x] ~~Add submission to BN~~ 
  - removed, see discussion in [keymanager API](ethereum/keymanager-APIs#58)
- [x] ~~Add flag to allow voluntary exit via the API~~ 
  - no longer needed now the VC doesn't submit exit directly
- [x] ~~Additional verification / checks, e.g. if validator on same network as BN~~ 
  - to be done on client side
- [x] ~~Potentially wait for the message to propagate and return some exit information in the response~~ 
  - not required
- [x] Update http tests
- [x] ~~Update lighthouse book~~ 
  - not required if this endpoint makes it to the standard keymanager API

Co-authored-by: Paul Hauner <[email protected]>
Co-authored-by: Jimmy Chen <[email protected]>
ghost pushed a commit to oone-world/lighthouse that referenced this pull request Jul 13, 2023
## Issue Addressed

Addresses sigp#4117 

## Proposed Changes

See ethereum/keymanager-APIs#58 for proposed API specification.

## TODO

- [x] ~~Add submission to BN~~ 
  - removed, see discussion in [keymanager API](ethereum/keymanager-APIs#58)
- [x] ~~Add flag to allow voluntary exit via the API~~ 
  - no longer needed now the VC doesn't submit exit directly
- [x] ~~Additional verification / checks, e.g. if validator on same network as BN~~ 
  - to be done on client side
- [x] ~~Potentially wait for the message to propagate and return some exit information in the response~~ 
  - not required
- [x] Update http tests
- [x] ~~Update lighthouse book~~ 
  - not required if this endpoint makes it to the standard keymanager API

Co-authored-by: Paul Hauner <[email protected]>
Co-authored-by: Jimmy Chen <[email protected]>
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
## Issue Addressed

Addresses sigp#4117 

## Proposed Changes

See ethereum/keymanager-APIs#58 for proposed API specification.

## TODO

- [x] ~~Add submission to BN~~ 
  - removed, see discussion in [keymanager API](ethereum/keymanager-APIs#58)
- [x] ~~Add flag to allow voluntary exit via the API~~ 
  - no longer needed now the VC doesn't submit exit directly
- [x] ~~Additional verification / checks, e.g. if validator on same network as BN~~ 
  - to be done on client side
- [x] ~~Potentially wait for the message to propagate and return some exit information in the response~~ 
  - not required
- [x] Update http tests
- [x] ~~Update lighthouse book~~ 
  - not required if this endpoint makes it to the standard keymanager API

Co-authored-by: Paul Hauner <[email protected]>
Co-authored-by: Jimmy Chen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants