-
Notifications
You must be signed in to change notification settings - Fork 84
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
core/validatorapi: handle HTTP methods #2866
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -86,142 +86,172 @@ func NewRouter(ctx context.Context, h Handler, eth2Cl eth2wrap.Client) (*mux.Rou | |||||||||
Name string | ||||||||||
Path string | ||||||||||
Handler handlerFunc | ||||||||||
Methods []string | ||||||||||
}{ | ||||||||||
{ | ||||||||||
Name: "attester_duties", | ||||||||||
Path: "/eth/v1/validator/duties/attester/{epoch}", | ||||||||||
Handler: attesterDuties(h), | ||||||||||
Methods: []string{http.MethodPost}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
Name: "proposer_duties", | ||||||||||
Path: "/eth/v1/validator/duties/proposer/{epoch}", | ||||||||||
Handler: proposerDuties(h), | ||||||||||
Methods: []string{http.MethodGet}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
Name: "sync_committee_duties", | ||||||||||
Path: "/eth/v1/validator/duties/sync/{epoch}", | ||||||||||
Handler: syncCommitteeDuties(h), | ||||||||||
Methods: []string{http.MethodPost}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
Name: "attestation_data", | ||||||||||
Path: "/eth/v1/validator/attestation_data", | ||||||||||
Handler: attestationData(h), | ||||||||||
Methods: []string{http.MethodGet}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
Name: "submit_attestations", | ||||||||||
Path: "/eth/v1/beacon/pool/attestations", | ||||||||||
Handler: submitAttestations(h), | ||||||||||
Methods: []string{http.MethodPost}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
Name: "get_validators", | ||||||||||
Path: "/eth/v1/beacon/states/{state_id}/validators", | ||||||||||
Handler: getValidators(h), | ||||||||||
Methods: []string{http.MethodPost, http.MethodGet}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
Name: "get_validator", | ||||||||||
Path: "/eth/v1/beacon/states/{state_id}/validators/{validator_id}", | ||||||||||
Handler: getValidator(h), | ||||||||||
Methods: []string{http.MethodGet}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
Name: "propose_block", | ||||||||||
Path: "/eth/v2/validator/blocks/{slot}", | ||||||||||
Handler: proposeBlock(h), | ||||||||||
Methods: []string{http.MethodGet}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
Name: "submit_proposal_v1", | ||||||||||
Path: "/eth/v1/beacon/blocks", | ||||||||||
Handler: submitProposal(h), | ||||||||||
Methods: []string{http.MethodPost}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
Name: "submit_proposal_v2", | ||||||||||
Path: "/eth/v2/beacon/blocks", | ||||||||||
Handler: submitProposal(h), | ||||||||||
Methods: []string{http.MethodPost}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
Name: "propose_blinded_block", | ||||||||||
Path: "/eth/v1/validator/blinded_blocks/{slot}", | ||||||||||
Handler: proposeBlindedBlock(h), | ||||||||||
Methods: []string{http.MethodGet}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
Name: "submit_blinded_block_v1", | ||||||||||
Path: "/eth/v1/beacon/blinded_blocks", | ||||||||||
Handler: submitBlindedBlock(h), | ||||||||||
Methods: []string{http.MethodPost}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
Name: "submit_blinded_block_v2", | ||||||||||
Path: "/eth/v2/beacon/blinded_blocks", | ||||||||||
Handler: submitBlindedBlock(h), | ||||||||||
Methods: []string{http.MethodPost}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
Name: "submit_validator_registration", | ||||||||||
Path: "/eth/v1/validator/register_validator", | ||||||||||
Handler: submitValidatorRegistrations(h), | ||||||||||
Methods: []string{http.MethodPost}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
Name: "submit_voluntary_exit", | ||||||||||
Path: "/eth/v1/beacon/pool/voluntary_exits", | ||||||||||
Handler: submitExit(h), | ||||||||||
Methods: []string{http.MethodPost}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
Name: "teku_proposer_config", | ||||||||||
Path: "/teku_proposer_config", | ||||||||||
Handler: proposerConfig(h), | ||||||||||
Methods: []string{http.MethodGet}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
Name: "proposer_config", | ||||||||||
Path: "/proposer_config", | ||||||||||
Handler: proposerConfig(h), | ||||||||||
Methods: []string{http.MethodGet}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
Name: "aggregate_beacon_committee_selections", | ||||||||||
Path: "/eth/v1/validator/beacon_committee_selections", | ||||||||||
Handler: aggregateBeaconCommitteeSelections(h), | ||||||||||
Methods: []string{http.MethodPost}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
Name: "aggregate_attestation", | ||||||||||
Path: "/eth/v1/validator/aggregate_attestation", | ||||||||||
Handler: aggregateAttestation(h), | ||||||||||
Methods: []string{http.MethodGet}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
Name: "submit_aggregate_and_proofs", | ||||||||||
Path: "/eth/v1/validator/aggregate_and_proofs", | ||||||||||
Handler: submitAggregateAttestations(h), | ||||||||||
Methods: []string{http.MethodPost}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
Name: "submit_sync_committee_messages", | ||||||||||
Path: "/eth/v1/beacon/pool/sync_committees", | ||||||||||
Handler: submitSyncCommitteeMessages(h), | ||||||||||
Methods: []string{http.MethodPost}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
Name: "sync_committee_contribution", | ||||||||||
Path: "/eth/v1/validator/sync_committee_contribution", | ||||||||||
Handler: syncCommitteeContribution(h), | ||||||||||
Methods: []string{http.MethodGet}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
Name: "submit_contribution_and_proofs", | ||||||||||
Path: "/eth/v1/validator/contribution_and_proofs", | ||||||||||
Handler: submitContributionAndProofs(h), | ||||||||||
Methods: []string{http.MethodPost}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
Name: "submit_proposal_preparations", | ||||||||||
Path: "/eth/v1/validator/prepare_beacon_proposer", | ||||||||||
Handler: submitProposalPreparations(), | ||||||||||
Methods: []string{http.MethodPost}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
Name: "aggregate_sync_committee_selections", | ||||||||||
Path: "/eth/v1/validator/sync_committee_selections", | ||||||||||
Handler: aggregateSyncCommitteeSelections(h), | ||||||||||
Methods: []string{http.MethodPost}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
Name: "node_version", | ||||||||||
Path: "/eth/v1/node/version", | ||||||||||
Handler: nodeVersion(h), | ||||||||||
Methods: []string{http.MethodGet}, | ||||||||||
}, | ||||||||||
} | ||||||||||
|
||||||||||
r := mux.NewRouter() | ||||||||||
for _, e := range endpoints { | ||||||||||
r.Handle(e.Path, wrap(e.Name, e.Handler)) | ||||||||||
handler := r.Handle(e.Path, wrap(e.Name, e.Handler)) | ||||||||||
if len(e.Methods) != 0 { | ||||||||||
handler.Methods(e.Methods...) | ||||||||||
} | ||||||||||
Comment on lines
+252
to
+254
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
i wonder if this could work? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sadly no: package main
import (
"net/http"
"github.com/gorilla/mux"
)
func main() {
r := mux.NewRouter()
r.HandleFunc("/test", func(w http.ResponseWriter, r *http.Request) {
w.Write([]byte(r.Method))
}).Methods()
http.ListenAndServe(":9999", r)
} This demo simulates the case in which If you |
||||||||||
} | ||||||||||
|
||||||||||
// Everything else is proxied | ||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -121,7 +121,7 @@ func TestRawRouter(t *testing.T) { | |
handler := testHandler{} | ||
|
||
callback := func(ctx context.Context, baseURL string) { | ||
res, err := http.Get(baseURL + "/eth/v1/validator/duties/attester/not_a_number") | ||
res, err := http.Post(baseURL+"/eth/v1/validator/duties/attester/not_a_number", "application/json", bytes.NewReader([]byte("{}"))) | ||
require.NoError(t, err) | ||
|
||
var errRes errorResponse | ||
|
@@ -140,7 +140,7 @@ func TestRawRouter(t *testing.T) { | |
handler := testHandler{} | ||
|
||
callback := func(ctx context.Context, baseURL string) { | ||
res, err := http.Post(baseURL+"/eth/v2/validator/blocks/123", "", nil) | ||
res, err := http.Get(baseURL + "/eth/v2/validator/blocks/123") | ||
require.NoError(t, err) | ||
|
||
var errRes errorResponse | ||
|
@@ -159,7 +159,7 @@ func TestRawRouter(t *testing.T) { | |
handler := testHandler{} | ||
|
||
callback := func(ctx context.Context, baseURL string) { | ||
res, err := http.Post(baseURL+"/eth/v2/validator/blocks/123?randao_reveal=0x0000", "", nil) | ||
res, err := http.Get(baseURL + "/eth/v2/validator/blocks/123?randao_reveal=0x0000") | ||
require.NoError(t, err) | ||
|
||
var errRes errorResponse | ||
|
@@ -185,7 +185,7 @@ func TestRawRouter(t *testing.T) { | |
|
||
callback := func(ctx context.Context, baseURL string) { | ||
randao := testutil.RandomEth2Signature().String() | ||
res, err := http.Post(baseURL+"/eth/v2/validator/blocks/123?randao_reveal="+randao, "", nil) | ||
res, err := http.Get(baseURL + "/eth/v2/validator/blocks/123?randao_reveal=" + randao) | ||
require.NoError(t, err) | ||
|
||
var okResp struct{ Data json.RawMessage } | ||
|
@@ -201,7 +201,7 @@ func TestRawRouter(t *testing.T) { | |
handler := testHandler{} | ||
|
||
callback := func(ctx context.Context, baseURL string) { | ||
res, err := http.Get(baseURL + "/eth/v1/validator/duties/attester/1") | ||
res, err := http.Post(baseURL+"/eth/v1/validator/duties/attester/1", "application/json", bytes.NewReader([]byte(""))) | ||
require.NoError(t, err) | ||
|
||
var errRes errorResponse | ||
|
@@ -576,6 +576,57 @@ func TestRouter(t *testing.T) { | |
"dependent_root": dependentRoot, | ||
} | ||
|
||
t.Run("wrong http method", func(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there any endpoint that doesn't have a list of accepted methods? In that case, I think a test case for such an endpoint could make sense But, I believe you have added either GET or a POST to all the endpoints There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct, we must always specify the list of supported methods for each VAPI endpoint. |
||
ctx := context.Background() | ||
|
||
h := testHandler{} | ||
|
||
proxy := httptest.NewServer(h.newBeaconHandler(t)) | ||
defer proxy.Close() | ||
|
||
r, err := NewRouter(ctx, h, testBeaconAddr{addr: proxy.URL}) | ||
require.NoError(t, err) | ||
|
||
server := httptest.NewServer(r) | ||
defer server.Close() | ||
|
||
endpointURL := fmt.Sprintf("%s/eth/v1/node/version", server.URL) | ||
|
||
// node_version is a GET-only endpoint, we expect it to fail | ||
resp, err := http.Post( | ||
endpointURL, | ||
"application/json", | ||
bytes.NewReader([]byte("{}")), | ||
) | ||
|
||
require.NoError(t, err) | ||
|
||
require.Equal( | ||
t, | ||
http.StatusNotFound, | ||
resp.StatusCode, | ||
) | ||
|
||
// use the right http method and expect a response, and status code 200 | ||
resp, err = http.Get(endpointURL) | ||
require.NoError(t, err) | ||
|
||
require.Equal( | ||
t, | ||
http.StatusOK, | ||
resp.StatusCode, | ||
) | ||
|
||
data, err := io.ReadAll(resp.Body) | ||
require.NoError(t, err) | ||
|
||
defer func() { | ||
_ = resp.Body.Close() | ||
}() | ||
|
||
require.NotEmpty(t, data) | ||
}) | ||
|
||
t.Run("attesterduty", func(t *testing.T) { | ||
handler := testHandler{ | ||
AttesterDutiesFunc: func(ctx context.Context, opts *eth2api.AttesterDutiesOpts) (*eth2api.Response[[]*eth2v1.AttesterDuty], error) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why a slice rather than a string? In this case, we will need to write a single handler which will handle all the http statuses. But if specify just one method per endpoint then we can separate handlers for each method which might be more clean than the current approach. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally when designing APIs like this I found that one might want to handle the same endpoint in different HTTP methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference,
gorilla/mux
does this as well, using a variadic argument forhandler.Methods
.