Fee Recipient API #24

james-prysm · 2022-04-12T17:54:23Z

Post Merge ( Bellatrix release) , stakers will be able to set an eth address to their corresponding validator key to receive gas fees when proposing a block. Fee recipient mapping could be standard across all validating public keys or specific per validating key.

Validator clients should provide a standard API to update and view the fee recipient settings so stakers who have a gui only setup ( dappnode/avado ) and stakers who want to update fee recipients at run time can do so.

This PR proposes one possible implementation of this standard API.

related to #23

rolfyone · 2022-04-13T21:15:02Z

keymanager-oapi.yaml

+  /eth/v1/feerecipient:
+    $ref:  './apis/fee_recipient.yaml'


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.

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

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

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.

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.

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.

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

rolfyone · 2022-04-13T21:18:43Z

apis/fee_recipient.yaml

+    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) 


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.

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.

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.

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?

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.

rolfyone · 2022-04-13T21:21:05Z

apis/fee_recipient.yaml

+            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"


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..." }

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

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?

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.

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

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

I also think an individual call is cleaner though I am still wondering how important it is to implement right now.

please take a look at the updated version

rolfyone · 2022-04-13T21:22:43Z

types/fee_recipient_map.yaml

+properties:
+  pubkey:
+    $ref: './public_key.yaml'
+  ethaddress:


call this by its function not its type - fee_recipient

manage_fee_recipient? 😅

im not sure why not fee_recipient?

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

g11tech · 2022-04-14T06:15:46Z

apis/fee_recipient.yaml

+                      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.


or validator not registered

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.

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

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.

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.

g11tech · 2022-04-14T06:20:23Z

apis/fee_recipient.yaml

+                items:
+                  type: object
+                  required: [status]
+                  properties:


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

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.

return property as fee_recipient 🤔

rolfyone · 2022-04-16T20:33:24Z

apis/fee_recipient.yaml

+                    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


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.

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

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

apis/fee_recipient.yaml

rolfyone · 2022-04-16T20:50:13Z

apis/fee_recipient.yaml

+    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) 


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

rolfyone · 2022-04-19T03:24:02Z

apis/fee_recipient.yaml

+    "200":
+      description: ok or updated
+      content:
+        application/json:
+          schema:
+            title: SetFeeRecipientResponse
+            type: string
+            example: "updated"


so i think we go with 202, description "successfully updated", and no content.
The 200 then wont be used for the POST. I've said this in a couple of comments but highlighting the actual code for review here (sorry if it sounds like I'm nagging, but this is arguably the best actual place to comment for it)

got it, appreciate the comments. I just updated it, how does it look now?

rolfyone · 2022-04-21T07:16:11Z

keymanager-oapi.yaml

@@ -52,8 +56,12 @@ components:
  schemas:
    Pubkey:
      $ref: './types/public_key.yaml'
+    Ethaddress:


EthAddress - missed capital ~A

rolfyone · 2022-04-21T07:55:00Z

I was talking to the team and someone pointed out having it more rest like changes the path a bit but makes things more flexible, and more rest-like, so it sounds good...

This may look like /eth/v1/validator/{pubkey}/feerecipient for example... Then building out other fields becomes 'simple'... also becomes possible to build out a /eth/v1/validator/{pubkey} maybe at some point that allows patches...

Overall I think we're close to a model here, but we may be kind to ourselves by going this route and maybe leveraging that model down the line...

james-prysm · 2022-04-21T19:32:24Z

Overall I think we're close to a model here, but we may be kind to ourselves by going this route and maybe leveraging that model down the line...

Cool I think this is fair and a good idea for down the road, maybe we can migrate to that one once we have some more APIs for the specific validator key !

rolfyone · 2022-04-22T04:09:09Z

keymanager-oapi.yaml

@@ -41,6 +43,8 @@ paths:
    $ref:  './apis/local_keystores.yaml'
  /eth/v1/remotekeys:
    $ref:  './apis/remote_keystores.yaml'
+  /eth/v1/feerecipient/{pubkey}:


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

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

rolfyone

LGTM overall, I think changing the path before implementing is probably a good plan, it seems logical...
marking approved because i think we're in a good place overall.

rolfyone · 2022-04-22T14:22:42Z

Thanks for your patience James, looks good, happy for you to merge ;)

james-prysm and others added 9 commits February 15, 2022 15:14

prysm flow

c9487c3

prysm specific info

6c4e813

remove DS_Store

166e8ae

removed...

fd1e33c

initial commit

ffaa666

Merge branch 'master' into fee-recipient

422d2eb

fixing tags

749917b

fixing linting

160a15d

adding warning that API works only post bellatrix

08b323b

james-prysm requested review from rolfyone, dapplion and michaelsproul April 13, 2022 00:29

rolfyone reviewed Apr 13, 2022

View reviewed changes

g11tech reviewed Apr 14, 2022

View reviewed changes

james-prysm added 7 commits April 14, 2022 10:30

updating based on some review comments

c058c12

fixing linting tabbing

447137a

adjusting functions based on comment feedback

82580b3

updating description

d9bbe54

removing unneeded file

4500f6f

Merge branch 'master' into fee-recipient

4f9eb94

deleted the wrong file adding it back in and deleting the correct one

aa10025

rolfyone reviewed Apr 16, 2022

View reviewed changes

apis/fee_recipient.yaml Show resolved Hide resolved

rolfyone reviewed Apr 16, 2022

View reviewed changes

apis/fee_recipient.yaml Show resolved Hide resolved

rolfyone reviewed Apr 16, 2022

View reviewed changes

pablomendezroyo mentioned this pull request Apr 18, 2022

Track Eth2 clients dappnode/DAppNode#389

Closed

updating based on review comments

9b3f95c

rolfyone reviewed Apr 19, 2022

View reviewed changes

updating based on review comments

c094bd1

james-prysm requested a review from rolfyone April 20, 2022 15:31

james-prysm marked this pull request as ready for review April 21, 2022 02:36

rolfyone reviewed Apr 21, 2022

View reviewed changes

updating based on comments

2063d06

james-prysm requested a review from rolfyone April 22, 2022 04:05

rolfyone reviewed Apr 22, 2022

View reviewed changes

rolfyone approved these changes Apr 22, 2022

View reviewed changes

adjusting path

c32e9aa

james-prysm merged commit 28c18e1 into master Apr 22, 2022

james-prysm deleted the fee-recipient branch April 22, 2022 16:05

paulhauner mentioned this pull request May 9, 2022

Implement feerecipient API for keymanager (VC) sigp/lighthouse#3173

Closed

Fee Recipient API #24

Fee Recipient API #24

Conversation

james-prysm commented Apr 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g11tech Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-prysm Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rolfyone commented Apr 21, 2022

james-prysm commented Apr 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rolfyone left a comment

Choose a reason for hiding this comment

rolfyone commented Apr 22, 2022

g11tech Apr 14, 2022 •

edited

Loading

james-prysm Apr 14, 2022 •

edited

Loading