-
Notifications
You must be signed in to change notification settings - Fork 575
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
Feature: RPC HTTP Adapter #3630
Conversation
fix some minor linter errors as part of cleanup
2779b8b
to
c10ee46
Compare
const error = ErrorUtils.renderError(e) | ||
this.logger.debug(`Error in HTTP adapter: ${error}`) | ||
res.writeHead(500) | ||
res.end(JSON.stringify({ 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.
This is a non RPC uniform error. We should return a uniform RPC error so they don't have to handle this specially or consumers will not be happy.
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.
Yea good suggestion. modified to handle more of our error types
Summary
This PR proposes to add
HttpAdapter
toironfish/src/rpc/adapters
. It is gated byenableRpcHttp
, which defaults tofalse
.To simplify things, everything will be a
POST
request.Testing Plan
WIP, unit tests + manual testing
Breaking Change
Is this a breaking change? If yes, add notes below on why this is breaking and
what additional work is required, if any.