search

ropensci / software-review

rOpenSci Software Peer Review.
290 stars 104 forks source link

Submission for R Package ccex #109

Closed kaneplusplus closed 5 years ago

kaneplusplus commented 7 years ago

Summary

Package ccex is an R implementation of the REST interface used by the C-Cex crypto-currency exchange. It provides functions for endpoints supported by the exchange. This includes the ability to retrieve price, volume, and order book information as well as the ability to trade crypto-currencies.

Package: ccex
Type: Package
Title: Client for the C-Cex Crypto-currency Exchange
Version: 0.1.0
Date: 2017-04-05
Authors@R: c(person("Michael", "kane", role = c("aut", "cph", "cre"), email = "kaneplusplus@gmail.com"))
URL: https://github.com/kaneplusplus/ccex
BugReports: https://github.com/kaneplusplus/ccex/issues
Description: A client for the C-Cex crypo-currency exchange \url{https://c-cex.com} including the ability to query trade data, manage account balances, and place orders.
License: LGPL-2
Imports: httr, openssl, stringr
Suggests: covr, testthat, knitr, rmarkdown, threejs, tidyverse, igraph, quantmod
VignetteBuilder: knitr
RoxygenNote: 6.0.1

(https://github.com/kaneplusplus/ccex)[https://github.com/kaneplusplus/ccex]

Target audience is finance researchers and traders interested in crypo-currencies.

No package provide an interface to the (C-Cex cryptocurrency exchange)[https://c-cex.com/] meant for research, which includes the ability to trade. Package (Rbitcoin)[https://github.com/jangorecki/Rbitcoin] is an "end-to-end trading engine in R" for currency trading with Bitcoin. It is not clear of other crypto-currencies are included. Package (IBroker)[https://github.com/joshuaulrich/IBrokers] is an API for the (Interactive Broker)[https://www.interactivebrokers.com/en/home.php] trading platform. It provides the ability to trade stocks and currencies, not crypto-currencies.

Requirements

Confirm each of the following by checking the box. This package:

Publication options

Detail

The devtools::check() command succeeds with no warnings.

No exceptions

No.

Joshua Ulrich: joshuaulrich Whit Armstrong: armstrtw

kaneplusplus commented 7 years ago

Taylor Arnold: statsmaths may also be able to provide a review.

noamross commented 7 years ago

Thank you for your submission, @kaneplusplus! You may be interested to know that an early rOpenSci contribution was https://github.com/ropensci/RCryptsy, though that service has since closed and the package removed from CRAN.

Editor checks:

Editor comments

I noted I needed the github/development version of threejs to build the vignette. You may wish to add this to Remotes: in your DESCRIPTION file, but in any case you'll want a CRAN-stable version if you are submitting to CRAN.

Here is output from goodpractice::gp(). As the package passes R CMD CHECK with no errors, warnings, or notes, these won't hold up assigning reviewers. Please address them during the review, though. 100% coverage isn't required but reviewers can use this to examine whether tests cover adequate use-cases. 

── GP ccex ─────────────────────────────────────

It is good practice to

  ✖ write unit tests for all functions, and all package code in general. 86%
    of code lines are covered by test cases.

    R/ccex.r:583:NA
    R/ccex.r:584:NA
    R/ccex.r:585:NA
    R/ccex.r:588:NA
    R/ccex.r:589:NA
    ... and 16 more lines

  ✖ omit "Date" in DESCRIPTION. It is not required and it gets invalid quite
    often. A build date will be added to the package when you perform `R CMD
    build` on it.
  ✖ use '<-' for assignment instead of '='. '<-' is the standard, and R users
    and developers are used it and it is easier to read your code for them if
    you use '<-'.

    R/ccex.r:61:15
    R/ccex.r:65:16
    R/ccex.r:95:7
    R/ccex.r:119:8
    R/ccex.r:120:8
    ... and 128 more lines

  ✖ avoid long code lines, it is bad for readability. Also, many people prefer
    editor windows that are about 80 characters wide. Try make your lines
    shorter than 80 characters

    R/ccex.r:240:1
    R/ccex.r:318:1
    R/ccex.r:342:1
    R/ccex.r:391:1
    R/ccex.r:432:1
    ... and 2 more lines

  ✖ avoid 1:length(...), 1:nrow(...), 1:ncol(...), 1:NROW(...) and 1:NCOL(...)
    expressions. They are error prone and result 1:0 if the expression on the
    right hand side is zero. Use seq_len() or seq_along() instead.

    R/ccex.r:283:12
    R/ccex.r:284:15
    R/ccex.r:583:14
    R/ccex.r:584:17
    R/ccex.r:628:14
    ... and 3 more lines

──────────────────────────────────────────

Reviewers: @statsmaths @kylehamilton Due date: 2017-05-11

noamross commented 7 years ago

Reviewers: @statsmaths @kylehamilton Due date: HOLDING

statsmaths commented 7 years ago

I ran into issues with the package that seem to be related to a problem with the ccex API. Opened an issue at https://github.com/kaneplusplus/ccex/issues/1 and @kaneplusplus is looking into it.

noamross commented 7 years ago

Thanks for the update, @statsmaths and @kaneplusplus.

noamross commented 7 years ago

Hello, @kaneplusplus, I just want to double check on the status of this review. Are we waiting for the CCEX API to update before moving forward? If so, I'll pause the clock until you let us know so that @kylehamilton and @statsmaths can plan for when to do their reviews.

kaneplusplus commented 7 years ago

Hi @noamross. The C-cex exchange was DDos'ed and the exchange's cloud provider (CloudFlare) has limited traffic to the browser. The package and documentation are complete but the reviewers can't use the package to query the exchange until the limitation is lifted.

The exchanges support has verified that the limitation will be lifted "soon" but they have not given any idea how soon. I've been checking daily to see if it has been lifted.

I'm sorry about the inconvenience and I'm happy to handle this however you want.

noamross commented 7 years ago

No worries, @kaneplusplus. I've put the "holding" tag on this. When you let us know that the API is back up I'll check with @statsmaths and @kylehamilton that they're still available, recruit new reviewer(s) if not, and give a new 3-week review deadline.

kaneplusplus commented 7 years ago

Got it. Thanks very much.

kaneplusplus commented 7 years ago

@noamross it looks like traffic is still being limited to C-cex. Could we have the same reviewers look at my bittrex package instead? The difference is the Bittrex crypto-currency exchange hasn't been limiting traffic for the last few weeks. The goal of the package is the same and the interface is almost the same.

noamross commented 7 years ago

Sorry that the API has been shut down in the middle of all this, @kaneplusplus. Could you submut bitrex as a separate issue and I'll process it within a day or two? @statsmaths and @kylehamilton, if so, would you be OK serving as reviewers for that?

kaneplusplus commented 7 years ago

Sure, no problem. Thanks.

sckott commented 5 years ago

@noamross i imagine we can close this?