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

Improvement to Fee Recipient UX #11307

Merged
merged 68 commits into from
Sep 22, 2022
Merged

Improvement to Fee Recipient UX #11307

merged 68 commits into from
Sep 22, 2022

Conversation

james-prysm
Copy link
Contributor

@james-prysm james-prysm commented Aug 24, 2022

What type of PR is this?

Feature

What does this PR do? Why is it needed?

Experimental UX improvements for fee recipient / validator registry. currently, the proposer settings update is required from the validator client either through flags --suggested-fee-recipient or --proposer-settings-file or url. this is a redundant experience for users who have set the suggested-fee-recipient on the beacon node and would like to not have to specify this again on the validator client. This pr looks to address this in the following way

  1. don't periodically call the push proposer settings function in validator client if no proposer settings/suggested fee recipient etc is called
  2. don't write warning logs when flags are not provided on validator client
  3. remove the restriction on starting validator client without a fee recipient
  4. add logs to notify the user that proposer settings/suggested fee recipient was provided.
  5. setting fee recipient will only persist after 1 epoch after calling beacon node again, if the restart happens prior to this the value will not persist.
  6. --enable-builder flag will add to the proposer settings file if used together and the proposer settings file has builder options empty.

related issues
#11322
#10720 - will not be addressed in this PR but a separate PR
gas_limit persistence through API is not addressed in this PR

note: extra UX improvements

  • changing validator index not found logs to only be debug logs

@james-prysm james-prysm self-assigned this Aug 24, 2022
@james-prysm james-prysm added UX cosmetic / user experience related Builder PR or issue that supports builder related work labels Aug 24, 2022
@james-prysm james-prysm marked this pull request as ready for review August 31, 2022 21:08
@james-prysm james-prysm requested a review from a team as a code owner August 31, 2022 21:08
@james-prysm james-prysm changed the title WIP Experimental Improvement to Fee Recipient UX Improvement to Fee Recipient UX Aug 31, 2022
@james-prysm james-prysm marked this pull request as draft August 31, 2022 22:02
james-prysm and others added 8 commits September 13, 2022 10:16
Co-authored-by: Radosław Kapka <rkapka@wp.pl>
Co-authored-by: Radosław Kapka <rkapka@wp.pl>
Co-authored-by: Radosław Kapka <rkapka@wp.pl>
Co-authored-by: Radosław Kapka <rkapka@wp.pl>
Co-authored-by: Radosław Kapka <rkapka@wp.pl>
Comment on lines 137 to 139
return &ethpb.FeeRecipientByPubKeyResponse{
FeeRecipient: params.BeaconConfig().DefaultFeeRecipient.Bytes(),
}, nil
Copy link
Member

Choose a reason for hiding this comment

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

This is rather dangerous, and we shouldn't skip out on the error and just default to burn address.
The caller should be able to handle the error with a switch statement

Copy link

Choose a reason for hiding this comment

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

This is rather dangerous, and we shouldn't skip out on the error and just default to burn address.

The caller should be able to handle the error with a switch statemen
rotsen11

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll handle this better

@james-prysm james-prysm merged commit 20e99fd into develop Sep 22, 2022
@delete-merged-branch delete-merged-branch bot deleted the validator-registry-ux branch September 22, 2022 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Builder PR or issue that supports builder related work UX cosmetic / user experience related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants