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 9 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
105 changes: 105 additions & 0 deletions apis/fee_recipient.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
get:
operationId: ListFeeRecipients
summary: List Fee Recipient.
description: |
List the validator public key to eth address mappings for fee recipient feature as well as any default eth address mappings.
The beacon node may have its own default fee recipient not listed here.

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
responses:
"200":
description: Success response
content:
application/json:
schema:
title: ListFeeRecipientsResponse
type: object
required: [data]
properties:
data:
type: array
items:
$ref: "../keymanager-oapi.yaml#/components/schemas/FeeRecipientMap"
default_fee_recipient_address:
$ref: "../keymanager-oapi.yaml#/components/schemas/Ethaddress"
"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
requestBody:
content:
application/json:
schema:
type: object
required: [pubkeys, ethaddresses]
properties:
pubkeys:
type: array
description: hexadecimal validator public keys.
items:
$ref: "../keymanager-oapi.yaml#/components/schemas/Pubkey"
ethaddresses:
type: array
description: hexadecimal eth addresses that the validator public key should be associated to, the length should match .
items:
$ref: "../keymanager-oapi.yaml#/components/schemas/Ethaddress"
Copy link
Collaborator

Choose a reason for hiding this comment

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

this really is just being unkind to ourselves. we should be creating an object so that we dont have 2 large disassociated arrays.

{
 "pubkey": "0x...",
 "fee_recipient": "0x..."
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

actually, i think this would mean we're just using the fee recipient map defined here as a ref

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's fair I was following the model on the other APIs that ended up getting approved. I do think it should be called fee recipient and then have ethaddress as the second property. other thoughts?

Copy link

@g11tech g11tech Apr 14, 2022

Choose a reason for hiding this comment

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

I agree, the array of this object is better, but then one will also need to qualify that last entry wins in case there are repeats for a pubkey. Also should there be a signature field for validating the the fee_recipient request is actually authorized. Might be crucial for multi party single validator setup if thats a thing.

Copy link

Choose a reason for hiding this comment

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

Another point: one special value of fee_recipient could be default deleting any set entry against this validaor which is akin to delete as highlighted by @rolfyone later in the comments

Copy link
Collaborator

Choose a reason for hiding this comment

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

if we do batch, then yes the last one wins. I think an individual call might be cleaner...

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 also think an individual call is cleaner though I am still wondering how important it is to implement right now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

please take a look at the updated version

responses:
"200":
description: Success response
content:
application/json:
schema:
title: SetFeeRecipientResponse
type: object
required: [data]
properties:
data:
type: array
description: Status result of each `request.pubkeys` with same length and order of `request.pubkeys`
items:
type: object
required: [status]
properties:
Copy link

Choose a reason for hiding this comment

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

can we also return what is now the current update value set against it (irrespective of the status)

Copy link
Collaborator Author

@james-prysm james-prysm Apr 14, 2022

Choose a reason for hiding this comment

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

I guess it would be stated in the message, but how would you like to see it? to be honest I was just following how the previous APIs were implemented haha.

Copy link

Choose a reason for hiding this comment

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

return property as fee_recipient 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added 🤔

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.
Copy link

Choose a reason for hiding this comment

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

or validator not registered

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What did you mean by this one? I think you can set the fee recipient mapping even if the validator doesn't exist in the validator/ it's not active.

Copy link

Choose a reason for hiding this comment

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

umm... returning error would be more prudent, or if not error then another enum value stating validator missing or something ? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Still not sure on this. as long as it's valid format shouldn't it just be set? 🤔 even if you don't have the keys currently or if those aren't real keys, the validator itself will check when the keys become active.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we really want to take advantage of HTTP return codes...
200 - updated, 202, updated (no content reuturned)
404 - not found
400 - error

404 is a particularly useful response on the POST to a specific address

so if the fee_recipient is set, and we don't have a body to return (don't think we should need one) then the 'most appropriate' response is really 202 with no body.
If the entity wasn't found to update, then 404, and if we tried to update it but there were errors, then a 400 with a mesage would be ideal, because you can provide feedback in the message about the nature of the failure.

enum:
- set
- updated
- error
example: set
message:
type: string
description: error message if status == error
"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"
7 changes: 4 additions & 3 deletions flows/client-specific/README.md
Original file line number Diff line number Diff line change
@@ -1,23 +1,24 @@
# Prysm Specific Flows
Prysm (https://github.com/prysmaticlabs/prysm) at the time of writing is one of the consensus layer clients implementing the Keymanager APIs.

There are some Prysm specific quirks that this document would like to make users aware of.

### Prysm Default Validator API Address
`http://127.0.0.1:7500/`

Prysm by default runs on the above address when running with `--web` flag. please look at our documentation [here](https://docs.prylabs.network/docs/prysm-usage/web-interface) for more information. Prysm includes its own native UI implementation as well.


## prysm version 2.0.7

1. Authentication: Prysm requires a bearer token for using the validator APIs, this token is also used for Prysm's native UI. For more information on where to find this token please look at the Prysm Docs.

4. CORS: Prysm has CORS protection with ports 3000 (typically REACT), 7500, and 4200 (typically Angular). Please use the appropriate Prysm flag to enable other ports. Please look at the Prysm Docs for more information.
2. CORS: Prysm has CORS protection with ports 3000 (typically REACT), 7500, and 4200 (typically Angular). Please use the appropriate Prysm flag to enable other ports. Please look at the Prysm Docs for more information.

3. Prysm Wallet & keymanager Type: Prysm uses something called a Prysm wallet (not related to hardware wallet) to manage the validator keys inside the Prysm client. All users of Prysm must create a wallet with a specific keymanager type in order to import or delete validator keys. The following errors will be thrown if requirements are not met:
- invalid wallet: either the wallet itself is corrupted, missing, or unable to be opened.
- invalid keymanager type: the keymanager type when creating the wallet isn't usable with the keymanager APIs. Only the `local` keymanager type is allowed to be used for Keymanager APIs referring to the type where users are storing the validators in their local Prysm instance instead of using a remote signer.
- validator service cannot get keymanager: The Keymanager API is called too soon after wallet creation.

4. API middleware and Internal RPC: Prysm doesn't natively use REST and uses gRPC internally instead for APIs, that means Prysm is translating HTTP REST calls into RPC calls and vice versa. This can cause certain types of issues that although unintended, can happen.
- Error code or error message unexpected: because our RPC codes are translated to HTTP errors there may be unexpected response codes in which case please report to the Prysm Team
- Golang primitives default values: Golang (the programming language Prysm is written in) will default certain primatives by default (`string` as `""` instead of `null`, `boolean` as `false` instead of `null`, etc) we try to account for as many as we can with tests but please let the team know if Prysm behaves differently from other clients.
- Golang primitives default values: Golang (the programming language Prysm is written in) will default certain primatives by default (`string` as `""` instead of `null`, `boolean` as `false` instead of `null`, etc) we try to account for as many as we can with tests but please let the team know if Prysm behaves differently from other clients.
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:
$ref: './apis/fee_recipient.yaml'
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 the question here will come down to what we actually want to do when consuming the api.

  • if i had 2000 validators or more loaded, do i really want to list all of the fee recipients for all of the validators?
  • when I'm updating a fee recipient, am i likely to want to update all, or a small number (like 1)

for this api, I would be very tempted to start off by offering the ability to get / post to 1 specific validator in the first instance, and see if a get all / list all is likely to be actually required.

for example, if we were to provide /eth/v1/feerecipient/{pubkey} and and a get will fetch the recipient for that pubkey, a POST will update it, then that may be a very useful way to interact, especially for UI's I would expect...

Again - it depends on how it's going to be used. Not having a UI, this seems to make the most sense in the first instance.
If we do go down the route of a larger array of data, then we should at least be using feerecipients plural, because it's not acting on a single. It would likely need to be able to accept a query parameter or a body to get a subset on the GET. I'd be very interested to know why it's needed though.
Potentially we'd also want a DELETE to remove an overridden fee recipient for one (or many). otherwise the POST would need to accept an empty string or something to indicate we're removing that and just want to use the default fee recipient, which seems hacky.

Copy link
Collaborator

Choose a reason for hiding this comment

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

the other thing a specific endpoint gives us is the ability to return 404...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good points let me brew over it for the evening, going to see if others had any thoughts on this too.

Copy link

Choose a reason for hiding this comment

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

a specific get is a good idea and it could also be a post endpoint accepting array of pubkeys, also it should indicate that if the fee_recipient registered against it is a default or overridden.

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 would be very tempted to start off by offering the ability to get / post to 1 specific validator in the first instance and see if a get all / list all is likely to be actually required.

thinking back on this I feel like the reverse would be used more often with something like client migration 🤔

If we do go down the route of a larger array of data, then we should at least be using feerecipients plural, because it's not acting on a single. It would likely need to be able to accept a query parameter or a body to get a subset on the GET.

That's probably true. I think this will take longer to implement than the single use one haha so maybe you're right and we should just go with that....

Potentially we'd also want a DELETE to remove an overridden fee recipient for one (or many).
I guess most clients don't persist this 🤔 so it'll get reset on validator restart, they can update it again with whatever they need and also have a default, not sure if it's needed for mvp.

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 most of the time people will have a default fee recipient and be updating smaller numbers (probably 1), or they'll have large numbers and be basically wanting all/most to a default... they're the 2 main use-cases i can see... I think the latter is more likely to use a config based setup than keymanager management.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok i updated based on the individual get/post. hope I'm doing this correctly 😓 . ready for re-review.


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'
FeeRecipientMap:
$ref: './types/fee_recipient_map.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_map.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:
Copy link
Collaborator

Choose a reason for hiding this comment

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

call this by its function not its type - fee_recipient

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

manage_fee_recipient? 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

im not sure why not fee_recipient?

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 think i called the api the same thing 😓 so i was thinking it should be different but I guess they are in different folders

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: './eth_address.yaml'