Skip to content
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

Submission for R Package bittrex #120

Closed
14 tasks done
kaneplusplus opened this issue Jun 2, 2017 · 16 comments
Closed
14 tasks done

Submission for R Package bittrex #120

kaneplusplus opened this issue Jun 2, 2017 · 16 comments

Comments

@kaneplusplus
Copy link

Summary

  • What does this package do? (explain in 50 words or less):

Package bittrex is an R implementation of the REST interface used by the Bittrex 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.

  • Paste the full DESCRIPTION file inside a code block below:
Package: bittrex
Type: Package
Title: Client for the Bitrex Exchange
Version: 0.1.0
Authors@R: c(person("Michael", "Kane", role = c("aut", "cph", "cre"), email = "kaneplusplus@gmail.com"))
URL: https://github.com/kaneplusplus/bittrex
BugReports: https://github.com/kaneplusplus/bittrex/issues
Description: A client for the Bitrex crypo-currency exchange \url{https://bittrex.com/} including the ability to query trade data, manage account balances, and place orders.
License: LGPL-2
Imports: httr, openssl
Remotes: bwlewis/rthreejs
Suggests: covr, testthat, knitr, rmarkdown, threejs, tidyverse, igraph, quantmod
VignetteBuilder: knitr
RoxygenNote: 6.0.1
  • URL for the package (the development repository, not a stylized html page):

https://github.com/kaneplusplus/bittrex

  • Please indicate which category or categories from our package fit policies this package falls under and why? (e.g., data retrieval, reproducibility. If you are unsure, we suggest you make a pre-submission inquiry.):

  • Who is the target audience?

The target audience consists of finance researchers and traders interested in crypo-currencies.

  • Are there other R packages that accomplish the same thing? If so, what is different about yours?

No package provide an interface to the Bittrex cryptocurrency exchange meant for research, which includes the ability to trade. Package 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 is an API for the Interactive Broker 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:

  • does not violate the Terms of Service of any service it interacts with.
  • has a CRAN and OSI accepted license.
  • contains a README with instructions for installing the development version.
  • includes documentation with examples for all functions.
  • contains a vignette with examples of its essential functions and uses.
  • has a test suite.
  • has continuous integration, including reporting of test coverage, using services such as Travis CI, Coeveralls and/or CodeCov.
  • I agree to abide by ROpenSci's Code of Conduct during the review process and in maintaining my package should it be accepted.

Publication options

  • Do you intend for this package to go on CRAN?
  • Do you wish to automatically submit to the Journal of Open Source Software? If so:
    • The package contains a paper.md with a high-level description in the package root or in inst/.
    • The package is deposited in a long-term repository with the DOI:
    • (Do not submit your package separately to JOSS)

Detail

  • Does R CMD check (or devtools::check()) succeed? Paste and describe any errors or warnings:

  • Does the package conform to rOpenSci packaging guidelines? Please describe any exceptions:

  • If this is a resubmission following rejection, please explain the change in circumstances:

  • If possible, please provide recommendations of reviewers - those with experience with similar packages and/or likely users of your package - and their GitHub user names:

Kyle Hamilton: @kylehamilton
Taylor Arnold: @statsmaths

@noamross
Copy link
Contributor

noamross commented Jun 4, 2017

Editor checks:

  • Fit: The package meets criteria for fit and overlap
  • Automated tests: Package has a testing suite and is tested via Travis-CI or another CI service.
  • License: The package has a CRAN or OSI accepted license
  • Repository: The repository link resolves correctly
  • Archive (JOSS only, may be post-review): The repository DOI resolves correctly
  • Version (JOSS only, may be post-review): Does the release version given match the GitHub release (v1.0.0)?

Editor comments

Thanks, @kaneplusplus. goodpractice::gp() shows a nice clean package:

── GP bittrex ───────────────────────────

It is good practice to

  ✖ 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/bittrex.r:63:16
    R/bittrex.r:67:15
    R/bittrex.r:68:7
    R/bittrex.r:69:14
    R/bittrex.r:73:13
    ... and 138 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/bittrex.r:289:1

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

Reviewers: @statsmaths
Due date: 2017-06-26

@noamross
Copy link
Contributor

noamross commented Jun 6, 2017

@statsmaths has confirmed that he can move review from ccex, thanks!

@kaneplusplus
Copy link
Author

Thanks to both of you!

@statsmaths
Copy link

statsmaths commented Jun 18, 2017

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

  • As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

  • A statement of need clearly stating problems the software is designed to solve and its target audience in README
  • Installation instructions: for the development version of package and any non-standard dependencies in README
  • Vignette(s) demonstrating major functionality that runs successfully locally
  • Function Documentation: for all exported functions in R help
  • Examples for all exported functions in R Help that run successfully locally
  • Community guidelines including contribution guidelines in the README or CONTRIBUTING, and URL, Maintainer and BugReports fields in DESCRIPTION
Paper (for packages co-submitting to JOSS)

The package contains a paper.md with:

  • A short summary describing the high-level functionality of the software
  • Authors: A list of authors with their affiliations
  • A statement of need clearly stating problems the software is designed to solve and its target audience.
  • References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

  • Installation: Installation succeeds as documented.
  • Functionality: Any functional claims of the software been confirmed.
  • Performance: Any performance claims of the software been confirmed.
  • Automated tests: Unit tests cover essential functions of the package
    and a reasonable range of inputs and conditions. All tests pass on the local machine.
  • Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Final approval (post-review)

  • The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: 2.5 hours


Review Comments

This is a very well-written client for placing API calls to Bitrex crypo-currency exchange. There is excellent coverage of both unit tests and documentation; the README file got me up and running in under 5 minutes. I have only two issues and one suggestion before publishing the package:

  • Include a maintainer entry in the DESCRIPTION file.

  • add a link to the CONDUCT.md from the README.md file; if I understand correctly, this is the preferred method from the packaging_guide.md file.

  • (suggestion) I had previously reviewed the ccex package, which ran into trouble because the API was throttled and eventually shut-down. It would be nice if this package had something along the lines of a bittrex_api_check function that returns TRUE if the API seems to be working and FALSE otherwise. This would make tracking down bug-fixes down the line easier and would be a helpful function to use when writing a script that makes use of the package.

@kaneplusplus
Copy link
Author

WRT the maintainer entry: With apologies for quoting documentation... according to Writing R Extensions "The Package’, ‘Version’, ‘License’, ‘Description’, ‘Title’, ‘Author’, and ‘Maintainer’ fields are mandatory, all other fields are optional. Fields ‘Author’ and ‘Maintainer’ can be auto-generated from ‘Authors@R’, and may be omitted if the latter is provided." If the latter is provided then the "cre" role is designated as maintainer. I'm happy to add it though, to be more explicit.

WRT link to CONDUCT.md: Thanks for pointing this out. The link has been added.

WRT bittrex_api_check: This is a great suggesion and it has been added to the package. The function returns TRUE when the GET request's status code is 200 and FALSE otherwise. By default a warning is provided with the status code if the request is not successful.

@statsmaths
Copy link

@kaneplusplus Thanks for the quick turn around on the response / fixes. The new bittrex_api_check is exactly what I had in mind. I'll leave the whole implicit/explicit Maintainer field up to the editor, but otherwise this addresses all of my concerns.

@noamross
Copy link
Contributor

noamross commented Jun 24, 2017

Package Review

(As I haven't heard from a second reviewer I went ahead and did an editor's review)

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

  • As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

  • A statement of need clearly stating problems the software is designed to solve and its target audience in README
  • Installation instructions: for the development version of package and any non-standard dependencies in README
  • Vignette(s) demonstrating major functionality that runs successfully locally
  • Function Documentation: for all exported functions in R help
  • Examples for all exported functions in R Help that run successfully locally
  • Community guidelines including contribution guidelines in the README or CONTRIBUTING, and URL, Maintainer and BugReports fields in DESCRIPTION
Paper (for packages co-submitting to JOSS)

The package contains a paper.md with:

  • A short summary describing the high-level functionality of the software
  • Authors: A list of authors with their affiliations
  • A statement of need clearly stating problems the software is designed to solve and its target audience.
  • References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

  • Installation: Installation succeeds as documented.
  • Functionality: Any functional claims of the software been confirmed.
  • Performance: Any performance claims of the software been confirmed.
  • Automated tests: Unit tests cover essential functions of the package and a reasonable range of inputs and conditions. All tests pass on the local machine.
  • Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Final approval (post-review)

  • The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: 2.5


Review Comments

This is an excellent package providing access to data for Bittrex currency market research and interaction. It has an intuitive R API, covers the Bittrex REST API completely, is easy to set up and follow best practices of REST API interaction.

Code/API

  • I appreciate the type-stable/error-safe responses of the API calling functions. I would try to extend this in a few places. For getorderbook(), I would suggest returning a single data frame with a type column with buy and sell values. (Essentially the lines in the vignette). Also, empty values (e.g., from getopenorders(), the get*histotory() functions) are returned as an empty list rather than a zero-row data frame. I would suggest returning the latter with the expected fields.
  • Running getbalance("something") when I have no currency of type "something" yields an error. Can this fail more gracefully?
  • It would he helpful to use a function prefix (such at bt_) for all your API call functions. API function calls are fairly similar to what one might expect from other financial API packages (e.g., buy()), and this helps avoid namespace clashes and enables discovery through autocompletion. For programming people use :: but for interactive use having something like bt_buy() is very helpful.

Documentation

  • For getticker(), I suggest adding to the docs that the market names are the names column from getmarkets(), and they are not case-sensitive.
  • In the getorderbook() doc - can you specify what depth means? I assumed it would be number of records but the rows are much larger than this. Also, I suggest using an example with a market that yields results. usd-btc yields INVALID_MARKET.
  • Something is left out in the getmarkethistory market parameter description.
  • When you note that "market orders are currently disabled", can you specify whether this disabled in your package or in the Bittrex API? It might also be worth it to put a date here if it's the bittrex API because this could stop being true. (e.g., "As of DATE/VERSION...")
  • I am submitting a PR with some typo corrections, including what I believe were places you accidentally wrote "C-cex" instead of "Bittrex" (Typo fixes ropensci-archive/bittrex#1)
  • For private interfaces, it would be helpful to show results in the examples of what the output should look like for successful calls / transactions. Running the examples just for testing will yield failures, as they should, so example successful output (commented out in the not-run example), will be instructive.

Other Suggestions

I consider none of these mandatory just things to consider

  • You might consider returning the output data frames as tibbles, so that if the user has tidyverse packages loaded the shorter and more informative print methods are used, but if not they will act as standard data frames. (You don't need to import tibble, just change the class and set row names. See here.)
  • POSIXct times are also more tibble() compatible. Unless POSIXlt is required for compatibility with likely used packages like xts or quantmod I suggest POSIXct as the default time storage format.
  • I would suggest breaking up the bittrex.r file into multiple files for future ease of maintainability/collaboration.

Per @statsmaths' comment, the automated generation of Maintainer: field is fine with us. I'll make a note to clarify this in our guidelines.

@kaneplusplus
Copy link
Author

@noamross Thanks very much for the pull request, comments, and suggestions. I have made all of the revisions except for one. I decided not to include the tibble class information despite liking the approach you suggested. It gives slight preference to the tidyverse over data.table. However, I have added an issue, as an enhancement, the ability to register a function that data.frames generated in the package would be passed to. This would allow users to decorate returned data.frames in whichever way they want. Is this sufficient?

If so, then I think the package is ready to go. Thanks again for the review. It was extremely helpful.

@noamross
Copy link
Contributor

Looks good! Tiny things to fix and then we'll be good to go:

  • for bt_getorderbook(), the example reflects the previous format of output (list rather than data frame)
  • Please use <- instead of = for assignment. That was in the goodpractice() output but I neglected to call it out before.
  • The vignette still uses the old function names.
  • I fixed what I think is a documentation typo in a pull request.

The tibble() idea was just a suggestion. That's fine. I commented on the issue.

@kaneplusplus
Copy link
Author

kaneplusplus commented Jul 12, 2017

@noamross Sorry, I should have caught these. The revisions have been made.

@noamross
Copy link
Contributor

Approved! Thanks @kaneplusplus for submitting and @statsmaths for your review!

To-dos:

  • Transfer the repo to the rOpenSci organization under "Settings" in your repo. I have invited you to a team that should allow you to do so.
  • Add the rOpenSci footer to the bottom of your README
[![ropensci_footer](https://ropensci.org/public_images/ropensci_footer.png)](https://ropensci.org)
  • Add the (brand new!) "Peer-reviewed" badge to your README (in the future people will add this on submittal; it updates through the stages of review):
[![](https://ropensci.org/badges/120_status.svg)](https://github.com/ropensci/onboarding/issues/120)
  • Fix any links in badges for CI and coverage to point to the ropensci URL. (we'll turn on the services on our end.)
  • Activate Zenodo watching the repo
  • Tag and create a release so as to create a Zenodo version and DOI
  • Submit to JOSS using the Zenodo DOI. We will tag it for expedited review.

Welcome aboard! We'd also love a blog post about your package, either a short-form intro to it (https://ropensci.org/tech-notes/) or long-form post with more narrative about its development. ((https://ropensci.org/blog/). Let me know if you are interested.

@kaneplusplus
Copy link
Author

@noamross I'm having some trouble with Zenodo. I'm able to log in and see most of my repositories, but I'm not able to see the bittrex repository. Did I need to register the package before transferring the repo to the organization?

@noamross
Copy link
Contributor

Hmm, I'm not sure but I just upped you to Admin on both the RO repo and team. Can you see it now?

@kaneplusplus
Copy link
Author

That did it. Thanks. I'll have the rest of the checklist in the next hour or so.

I am interested in a blog post. However, I probably won't be able to get to this until next week.

@kaneplusplus
Copy link
Author

OK, badges have been added, Zenodo is watching, a DOI has been created, and I have submitted to JOSS.

Is the blog in one of the repositories? If not, please tell me who it should be sent to?

@noamross
Copy link
Contributor

Excellent! Glad we've wrapped this up. Our community manager @stefaniebutland will be in touch about the post.

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

No branches or pull requests

4 participants