Skip to content
This repository has been archived by the owner on Feb 15, 2022. It is now read-only.

Rest API Support for zenbot #797

Merged
merged 9 commits into from
Dec 4, 2017
Merged

Rest API Support for zenbot #797

merged 9 commits into from
Dec 4, 2017

Conversation

abduegal
Copy link
Contributor

@abduegal abduegal commented Dec 4, 2017

Hi guys,

Here is the code, that enables zenbot to have a REST API.
I tried to keep the code extendable, by allowing zenbot to add multiple outputs (one of them is this REST API). I have other outputs available like storing the trade in AWS dynamoDB.
I also think it would be a good idea to refactor the stdout reporting part of zenbot, so that would also become an output.

Please let me know if you have any questions.

@storm1er
Copy link

storm1er commented Dec 4, 2017

Can you give us an exemple about how to use your output extension ? =)
The same question goes for the REST API. (README.md appending ?)

This PR feels like "+16843214 up" ... You could split this in some PR next time ^^'

Copy link
Owner

@DeviaVir DeviaVir left a comment

Choose a reason for hiding this comment

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

I'm not sure if lib/output.js is the right place for this, but have no other suggestions at this time. I'll let others take a look and am interested in their feedback

let run = function(reporter, tradeObject) {
if (!reporter.port || reporter.port === 0) {
random_port({from: 20000}, function(port) {
startServer(port, tradeObject)
Copy link
Owner

Choose a reason for hiding this comment

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

try/catch if the port doesn't work, and we try a different one automatically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay I will do that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the random_port function already searches for a free port, so I dont think this is neccesary anymore. The function also tests explicitly if the port is available through a try / catch mechanism

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, sweet! Thanks for digging that up.

let startServer = function(port, tradeObject) {
tradeObject.port = port

app.get('/tradedata', function (req, res) {
Copy link
Owner

Choose a reason for hiding this comment

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

/trades?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed, I'll change that

})

app.listen(port)
tradeObject.url = require('ip').address() + ':' + port + '/tradeData'
Copy link
Owner

Choose a reason for hiding this comment

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

/trades?

@abduegal
Copy link
Contributor Author

abduegal commented Dec 4, 2017

@storm1er the output extension works the same as the notifiers, exchanges and strategies extensions. For that reason I did not add any manual for it.

With regards to the rest api. The config-sample.js should be self explanatory. I will add the available url and path names in a readme.

Also, I do not know how I can split this pull request even further, all it contains is REST api support for zenbot.

Copy link
Owner

@DeviaVir DeviaVir left a comment

Choose a reason for hiding this comment

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

👍 LGTM now. Thanks for this great addition!

@DeviaVir DeviaVir merged commit 4aadfd1 into DeviaVir:master Dec 4, 2017
@albabosh
Copy link

albabosh commented Dec 6, 2017

Seems that REST API requires two ports. If I put a single port in the config the app crashes a start. Could you give me a working config example ?

@storm1er
Copy link

storm1er commented Dec 7, 2017

@albabosh I don't have any issues with this PR,
Can you explain what's your problem ?

@albabosh
Copy link

albabosh commented Dec 7, 2017

If I enable REST API in conf.js & set c.output.api.port = 20002 it crashes a start (seems that express tries to bind twice to one port). I I set c.output.api.port = 0 zenbot starts well and binds to 2 random ports. I need to specify those two ports in confg (not random)

@storm1er
Copy link

storm1er commented Dec 8, 2017

hmm That's weird ... I didn't need have this problem ... Didn't need to manually set port anyway ^^'
Try to look what they said in review, probably a good start

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants