-
Notifications
You must be signed in to change notification settings - Fork 795
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
Add iron HTTP server #391
Add iron HTTP server #391
Conversation
beacon_node/http_server/src/api.rs
Outdated
} | ||
|
||
fn handle_fork<T: BeaconChainTypes + 'static>(req: &mut Request) -> IronResult<Response> { | ||
// TODO: investigate unwrap - I'm _guessing_ we'll never hit it but we should check to be sure. |
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.
Yeah this should succeed so long as the persistent::Read
has been linked (which it has)
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.
Do you think it would be prudent to create a conversion for this error into some IronResult
?
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.
Perhaps some sort of 500
result.
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.
Yeah, may as well avoid taking the whole node down. Looks like IronResult::new
takes any std::Error
and a response
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.
Fixed!
); | ||
|
||
// Any request to all other endpoints is handled by the `api` module. | ||
router.any("/*", api::build_handler(beacon_chain.clone()), "api"); |
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.
Another way to separate these is to "mount" the API handler on /api/
, and the metrics handler on /metrics/
using the Mount
middleware. Then we could have URLs like /api/node/fork
, etc. Probably not worth changing for now, but would be worth thinking about once we have more APIs and we want to stabilise them
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.
Looks good to me
Issue Addressed
NA
Proposed Changes
iron
HTTP server with the following endpoints:/metrics
: Prometheus-format metrics./node/fork
: returns the current fork.BeaconChainTypes
super-type and threads it through the program.Additional Info
Change #1 is the primary goal of this PR and change #2 helped facilitate it.