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

Fee Recipient API #24

Merged
merged 20 commits into from
Apr 22, 2022
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 101 additions & 0 deletions apis/fee_recipient.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
get:
operationId: ListFeeRecipient
summary: List Fee Recipient.
description: |
List the validator public key to eth address mapping for fee recipient feature on a specific public key.
The validator public key will return with the default fee recipient address if a specific one was not found.

WARNING: API only works POST MERGE(Bellatrix)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Potentially something like The fee_recipient is not used on Phase0 or Altair networks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

security:
- bearerAuth: []
tags:
- Fee Recipient
parameters:
- in: path
name: pubkey
schema:
$ref: "../keymanager-oapi.yaml#/components/schemas/Pubkey"
required: true
responses:
"200":
description: Success response
content:
application/json:
schema:
title: ListFeeRecipientResponse
type: object
required: [data]
properties:
data:
$ref: "../keymanager-oapi.yaml#/components/schemas/FeeRecipient"
"401":
rolfyone marked this conversation as resolved.
Show resolved Hide resolved
$ref: "../keymanager-oapi.yaml#/components/responses/Unauthorized"
"403":
$ref: "../keymanager-oapi.yaml#/components/responses/Forbidden"
"500":
$ref: "../keymanager-oapi.yaml#/components/responses/InternalError"

post:
operationId: SetFeeRecipient
summary: Set Fee Recipient.
description: |
Sets the validator client fee recipient mapping which will then update the beacon node.
Existing mappings for the same validator public key will be overwritten.
Specific Public keys not mapped will continue to use the default address for fee recipient in accordance to the startup of the validator client and beacon node.
Cannot specify default fee recipient address through API.

WARNING: API only works POST MERGE(Bellatrix)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we'd be far better off allowing this to happen as long as the Bellatrix milestone is enabled in config.
People everywhere would otherwise have to script updating their config, or leave it for hours potentially not correct, which I can't see as a great outcome.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think prysm has a fork config flag 🤔 unless you meant something else.
the API doesn't set the default address ( which i think would complain loudly), but those ux problems are definitely a concern.

Copy link

Choose a reason for hiding this comment

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

I think what @rolfyone is saying is the config has bellatrix epoch set to non infinity. But in my opinion there is no need to condition it to anything, a user will obviously use this api on setup which is merged or impending merge.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess the note maybe could be more for future devs doing integration testing? which I guess is a whole different UX problem lol. Do we keep the description as is then?

Copy link
Collaborator

Choose a reason for hiding this comment

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

if its future devs, we don't care.

  • what do we need now for UX to be ideal?
  • are we doing more than we need, or are we doing 'enough'?

Ultimately we're trying to make it possible to provide a 'painless' UX for people, and that's why i think that restricting to after bellatrix epoch may be against that idea.

security:
- bearerAuth: []
tags:
- Fee Recipient
parameters:
- in: path
name: pubkey
schema:
$ref: "../keymanager-oapi.yaml#/components/schemas/Pubkey"
required: true
requestBody:
content:
application/json:
schema:
$ref: '../keymanager-oapi.yaml#/components/schemas/Ethaddress'
required: true
responses:
"200":
description: Success response
content:
application/json:
schema:
title: SetFeeRecipientResponse
type: object
required: [data]
properties:
data:
type: object
required: [status]
properties:
status:
type: string
description: |
- set: new fee recipient mapping is set.
- updated: existing fee recipient mapping is replaced.
- error: Any other status different to the above: decrypting error, I/O errors, etc.
enum:
- set
- updated
- error
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think if we're doing this kind of individual interface, then that changes the response too.
202 is literally 'ok'/'updated' and no content body returned
400 with an error message thats useful, something like {data:{"code": 400, "message": "oopsie"}}

we don't need to pass back fee recipient if it's a 202, because we literally passed it in.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should it be 202 accepted or 200 ok? 🤔 i kept it as 200 right now since I wasn't sure but I just updated this

Copy link
Collaborator

Choose a reason for hiding this comment

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

no body would be 202... typically a 200 would expect to have a HTTP BODY associated

example: set
message:
type: string
description: error message if status == error
FeeRecipient:
$ref: "../keymanager-oapi.yaml#/components/schemas/FeeRecipient"
"400":
$ref: "../keymanager-oapi.yaml#/components/responses/BadRequest"
"401":
$ref: "../keymanager-oapi.yaml#/components/responses/Unauthorized"
"403":
rolfyone marked this conversation as resolved.
Show resolved Hide resolved
$ref: "../keymanager-oapi.yaml#/components/responses/Forbidden"
"500":
$ref: "../keymanager-oapi.yaml#/components/responses/InternalError"
23 changes: 0 additions & 23 deletions flows/client-specific/README.md

This file was deleted.

8 changes: 8 additions & 0 deletions keymanager-oapi.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ servers:
default: "https://public-mainnet-node.ethereum.org"

tags:
- name: Fee Recipient
description: Set of endpoints for management of fee recipient.
- name: Local Key Manager
description: Set of endpoints for key management of local keys.
- name: Remote Key Manager
Expand All @@ -41,6 +43,8 @@ paths:
$ref: './apis/local_keystores.yaml'
/eth/v1/remotekeys:
$ref: './apis/remote_keystores.yaml'
/eth/v1/feerecipient/{pubkey}:
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is the bit i think, if we just change this in the current PR to be /eth/v1/validator/{pubkey}/feerecipient I think we leave the change i was talking about easy without making people move from the one we do here...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh i misunderstood then, ok i'll follow that then and change it to under validator

$ref: './apis/fee_recipient.yaml'

components:
securitySchemes:
Expand All @@ -52,8 +56,12 @@ components:
schemas:
Pubkey:
$ref: './types/public_key.yaml'
Ethaddress:
Copy link
Collaborator

Choose a reason for hiding this comment

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

EthAddress - missed capital ~A

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

$ref: './types/eth_address.yaml'
Keystore:
$ref: './types/keystore.yaml'
FeeRecipient:
$ref: './types/fee_recipient.yaml'
SignerDefinition:
$ref: './types/signer_definition.yaml'
ImportRemoteSignerDefinition:
Expand Down
4 changes: 4 additions & 0 deletions types/eth_address.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
type: string
description: "An address on the execution (Ethereum 1) network."
example: "0xabcf8e0d4e9587369b2301d0790347320302cc09"
pattern: "^0x[a-fA-F0-9]{40}$"
7 changes: 7 additions & 0 deletions types/fee_recipient.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
type: object
required: [pubkey,ethaddress]
properties:
pubkey:
$ref: './public_key.yaml'
ethaddress:
$ref: './eth_address.yaml'