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

HTTP validator API: beacon and account endpoints #13191

Merged
merged 28 commits into from
Dec 1, 2023

Conversation

james-prysm
Copy link
Contributor

@james-prysm james-prysm commented Nov 14, 2023

What type of PR is this?

Other

What does this PR do? Why is it needed?

migrates web endpoints from gRPC to REST validator.

auth endpoint

  • /v2/validator/initialize GET

validator endpoints

  • /v2/validator/accounts GET
  • /v2/validator/accounts/backup POST
  • /v2/validator/accounts/voluntary-exit POST

beacon endpoints

  • /v2/validator/beacon/status GET

  • /v2/validator/beacon/participation GET removed

  • /v2/validator/beacon/summary GET

  • /v2/validator/beacon/validators GET

  • /v2/validator/beacon/balances GET

  • /v2/validator/beacon/queue GET removed

  • /v2/validator/beacon/peers GET

  • Updates webui site_data to v2.0.5

Cleanup

  • Remove old prysm remote keymanager RPC endpoints ( not used )

Tested with updated web ui

Which issues(s) does this PR fix?

part of #13041

BLOCKED BY WEBUI RELEASE AND PR prysmaticlabs/prysm-web-ui#249

Other notes for review

@james-prysm james-prysm added Blocked Blocked by research or external factors API Api related tasks Cleanup Code health! labels Nov 14, 2023

forward_RemoteSigner_Sign_0 = runtime.ForwardResponseMessage
)
package ignore
Copy link
Contributor

@saolyn saolyn Dec 1, 2023

Choose a reason for hiding this comment

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

Can this file be deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm i'm not totally sure, we have a few other ones that are like this as well.

Copy link
Member

Choose a reason for hiding this comment

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

🔥 delete it 🔥

Where did it come from? If running the pb update script put it here, then leave it, I guess. We don't want to manually undo things that the scripts are doing. If anything, we would want to understand why this is getting put here then fix it at the root of the problem

rauljordan
rauljordan previously approved these changes Dec 1, 2023
Copy link
Contributor

@rauljordan rauljordan left a comment

Choose a reason for hiding this comment

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

This is an excellent piece of work! Very comprehensive PR and the diff was very easy to read. Nice job on getting this one completed

beacon-chain/rpc/eth/shared/structs_validator.go Outdated Show resolved Hide resolved
httputil.HandleError(w, "Prysm Wallet not initialized. Please create a new wallet.", http.StatusServiceUnavailable)
return
}
pageSize := r.URL.Query().Get("page_size")
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to do this in this PR, but one easier way to make this function more testable is to have the beginning parse out all the stuff from the URL queries and request into some pure Go struct and then have an inner function that takes in all that validated data. This way, you can write your tests using more pure business logic instead of having to mock requests and urls, and separate test out the validation of inputs

@rauljordan rauljordan removed the Blocked Blocked by research or external factors label Dec 1, 2023
Co-authored-by: Raul Jordan <raul@prysmaticlabs.com>

// Handler serves web requests from the bundled site data.
// DEPRECATED: Prysm Web UI and associated endpoints will be fully removed in a future hard fork.
Copy link
Member

Choose a reason for hiding this comment

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

Probably want to leave this deprecation?

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 can add it back

// List of 48 byte, BLS12-381 validating public keys.
repeated bytes validating_public_keys = 2;
}

// SignRequest is a message type used by a keymanager
// as part of Prysm's accounts v2 implementation.
message SignRequest {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like you can also delete SignRequest and SignResponse

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's still used in the code right now even though the remote is not. you can do a search on the type

Copy link
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

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

LGTM -- Tested the web UI locally on this branch. Checked that the UI loads, displays some information about my test validators. I did not check many functions of the UI

@prylabs-bulldozer prylabs-bulldozer bot merged commit 394bd17 into develop Dec 1, 2023
16 of 17 checks passed
@prylabs-bulldozer prylabs-bulldozer bot deleted the http-validator-beacon branch December 1, 2023 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Api related tasks Cleanup Code health!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants