-
Notifications
You must be signed in to change notification settings - Fork 990
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
Event callbacks for Network and Chain Events #2598
Event callbacks for Network and Chain Events #2598
Conversation
servers/src/common/hooks.rs
Outdated
fn parse_url(value: &Option<String>) -> Option<hyper::Uri> { | ||
match value { | ||
Some(url) => { | ||
let uri: hyper::Uri = url.parse().unwrap(); |
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.
Please no unwrap
Some(url) => { | ||
let uri: hyper::Uri = url.parse().unwrap(); | ||
let scheme = uri.scheme_part().map(|s| s.as_str()); | ||
if scheme != Some("http") { |
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.
What about https?
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 TLS an extra crate is needed https://hyper.rs/guides/client/configuration/ . Not sure if it is worth it as I expect this to be used mostly with internal apis and the info it broadcasts is public.
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.
The problem is that a consumer service could be https-only. Btw grin api client supports it already, so the crate is already here.
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.
ok then, will add https support
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.
In general the caller should be aware of the backpressure it is creating, if the client is taking too long to process the requests it could result in a DOS for the client. It is best practice, and it is not easy to implement but I thought it was worth mentioning. I don't expect that it will be implemented in this first cut.
}); | ||
|
||
let handle = self.runtime.executor(); | ||
handle.spawn(future); |
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.
I'm assuming this is non-blocking?
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.
Yes tokio is async. By default it spawns 1 worker thread per virtual core per reactor.
let future = self | ||
.client | ||
.request(req) | ||
.map(|_res| {}) |
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.
add a trace or debug showing result? could be useful for debugging
HeaderValue::from_static("application/json"), | ||
); | ||
|
||
let future = self |
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.
configurable timeout?
@mcdallas do you plan to add docs in this PR, separate PR, or no plans to add docs? A HOWTO in the wiki would be nice. |
@Kargakis yes I will add a section in the wiki when/if it gets merged |
How should we consider this? Being a new feature, it seems complete enough to merge and then improve upon. But if the improvements are forthcoming, then it's likely better to wait. |
f34bced
to
8ddd70d
Compare
Rebased on latest. @ignopeverell I am on the road and probably wont be able to finish it this week. If you want to merge I can send the rest next week on a separate PR |
@Kargakis I documented the new feature here: https://github.com/mimblewimble/docs/wiki/Event-hooks |
@mcdallas thanks! |
This allows for hooks on the following events:
How to use:
for webhooks, add the urls to your config (http only)
Each time an event triggers, a POST request will be made to the urls configured.
To add your own custom callbacks edit
servers/src/common/hooks.rs
, implements the corresponding trait and add your hook to the init functionTODO:
Closes #2321