daroczig / binancer

An R client to the Public Rest API for Binance.
https://daroczig.github.io/binancer
53 stars 57 forks source link

extension to other Binance API calls #9

Closed madsurgeon closed 5 years ago

madsurgeon commented 5 years ago

Dear Gergely,

I have extended binancer to all other Binance API calls, except historicalTrades, since the latter gives me always the error "-2014 API-key format invalid." I could not figure out the reason, therefore I have left out that one API call.

I did not touch huobi.R since I am only interested in Binance. Since I have changed query in utils.R, huobi.R is probably going to be broken now.

A few comments to the changes:

DESCRIPTION: I have set the version number to a development version according to Hadley Wickham's suggestions. The period at the end of the description line satisfies R CMD check.

zzz.R For some reason checking the arguments in both binance_query and query throws an error. Therefore I have commented out the line for query, but left it in, since logically it seems to be appropriate.

utils.R match.arg(method): see above res returns as a httr object now, since we need the headers in binance_query

binance.R The changes in the sections are for RStudio, so it shows them properly. The other changes should be comprehensible. If not, I am happy to explain.

On a general note:

The test in query in utils.R seems not to work properly (I haven't changed that one). I often get first the error: Error in UseMethod("headers") : no applicable method for 'headers' applied to an object of class "c('simpleError', 'error', 'condition')" This error disappears when I run the API request a second time. Clearly, this should be caught in the query function.

Also, I haven't changed the order of functions and parameters. What do you think, wouldn't it be easier to work with if the order was the same as in the official API documentation?

I am looking forward to your comments :-)

Best regards, David

daroczig commented 5 years ago

This is awesome, thank you very much :+1:

I try to review and test this weekend.

madsurgeon commented 5 years ago

I have fixed a few bugs since I did the PR. Do I need to do another one or you can just take the newest version from my side?

-----"Gergely Daróczi" notifications@github.com wrote: ----- To: "daroczig/binancer" binancer@noreply.github.com From: "Gergely Daróczi" notifications@github.com Date: 08/01/2019 15:43 Cc: "David Andel" andel@ifi.uzh.ch, "Author" author@noreply.github.com Subject: Re: [daroczig/binancer] extension to other Binance API calls (#9)

This is awesome, thank you very much 👍 I try to review and test this weekend. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

daroczig commented 5 years ago

@madsurgeon pls push your updates to the same branch to be added to this very same PR :+1:

madsurgeon commented 5 years ago

I have hit a bug in httr it seems. Whenever requesting a fromId = 10^n with n >= 5 RCurl seems to send something wrong to Binance.

This is an example which works:

as.data.table(content(GET("https://api.binance.com", path = 'api/v1/aggTrades', query = list(symbol = 'BNBUSDT', limit = 1, fromId = 10000), config = config()))) V1 1: 10000 2: 1.61900000 3: 100.00000000 4: 11670 5: 11670 6: 1.510649e+12 7: FALSE 8: TRUE

BUT, when we use fromId = 100000:

as.data.table(content(GET("https://api.binance.com", path = 'api/v1/aggTrades', query = list(symbol = 'BNBUSDT', limit = 1, fromId = 100000), config = config()))) code msg 1: -1100 Illegal characters found in parameter 'fromId'; legal range is '^[0-9]{1,20}$'.

And this is NOT Binance's fault, since it works, when we send the fromId as a string:

as.data.table(content(GET("https://api.binance.com", path = 'api/v1/aggTrades', query = list(symbol = 'BNBUSDT', limit = 1, fromId = '100000'), config = config()))) V1 1: 100000 2: 3.53110000 3: 53.94000000 4: 111833 5: 111833 6: 1.513347e+12 7: TRUE 8: TRUE

The same happens with 1000000... Interactively, we can send fromId as a string from binance_ticks so that this problem does not occur, but I didn't find a way to work around this issue with a variable. My solution then is to check for fromId %% 100000 == 0 (it occurs also with 200000 etc.) and use fromId - 1 in this case.

madsurgeon commented 5 years ago

Thank you for your support and additions! I have updated the DESCRIPTION file and merged the branches.

daroczig commented 5 years ago

awesome, thanks :bowing_man:

madsurgeon commented 5 years ago

For updates of the API like now https://github.com/binance-exchange/binance-official-api-docs/blob/master/CHANGELOG.md can I just update my branch again and then file a new PR?

daroczig commented 5 years ago

sure, thank you!