search

coinbase / coinbase-pro-trading-toolkit

DEPRECATED — The Coinbase Pro trading toolkit
Apache License 2.0
870 stars 239 forks source link

Paper Trading capability #236

Closed jleonelion closed 6 years ago

jleonelion commented 6 years ago

this commit closes #135

rmm5t commented 6 years ago

There is very likely a lot of great work here. However, there are several, if not a half dozen different pull requests wrapped up in this one PR.

Relevant Reading Material: https://medium.com/@fagnerbrack/one-pull-request-one-concern-e84a27dfe9f1

jleonelion commented 6 years ago

@rmm5t feel free to work with @DannyHinshaw and handle this however you'd like. My prior PRs were rejected and I am including changes to satisfy the changes requested by the rejectors. I never thought it would take so much effort to give away work. All in all, I'm happy to share what I have done, but not getting paid enough to justify the effort required to get these contributions into the mainline project.

rmm5t commented 6 years ago

Hi @jleonelion, I understand the frustrations. Just trying to help guide you towards an easier path of getting your hard work accepted (on any open source project). This is a good read as well: https://www.igvita.com/2011/12/19/dont-push-your-pull-requests/

DannyHinshaw commented 6 years ago

@jleonelion I totally get the frustration. I really appreciate all the time and continued support for getting this feature working. I think we're on the home stretch, and as far as I know, once we address the order methods renaming mentioned by @blair (which I also asked about on this most recent feature pull) I should be able to finish this up for us, any maintenance related stuff to get this accepted etc... and speaking of...

@rmm5t thanks for the feedback. It would be very helpful to get more specific feedback from you as well. Not trying to skate by necessarily, but at this point it'd be great to know exactly what needs done to make this acceptable to get merged. The articles are good reads, but I wasn't able to extrapolate exactly what needs applied to this PR. I'm still sort of a pleb with git, but I'm willing to get my hands dirty, just need to know what needs done.

DannyHinshaw commented 6 years ago

@blair thank for your feedback. I believe I have now fixed everything you requested.

The latest commits aim to addresses the following:

• Revert the Trader class to it's previous state. The renaming of it's public methods for placing/cancelling orders would likely be unacceptable to push to the main branch (for anyone using those methods in the wild). This is solved this by using the executeMessage method directly when using the paper-trader. • Small syntactic/sematic fixes requested by @blair • Remove paper-exchange tests that remain pending when mocha has finished.

DannyHinshaw commented 6 years ago

@blair do you have permissions to review/merge this, or know who does? Not sure who's best to tag

DannyHinshaw commented 6 years ago

@fb55 could we get a review on this when you have the time?

fb55 commented 6 years ago

Very hard to review this as it includes a lot of unrelated changes. I would very much prefer to have this be split into several PRs.

Also having some descriptions why certain changes were made would be nice. Eg. why do we include an S3 stream logger?

fb55 commented 6 years ago

Generally I like the feature and would love to see it be part of the codebase, but the current state is not great for a review.

DannyHinshaw commented 6 years ago

~@fb55 thanks for the review. I will try to get this cleaned up...~

Edit: see below

DannyHinshaw commented 6 years ago

@fb55 please re-review at your leisure. I have manually gone through and removed anything extraneous or unrelated to the paper-trader feature specifically.

DannyHinshaw commented 6 years ago

@fb55 , please close this PR without merging. I'm going to create a feature branch instead of rewriting over all of jleon's changes on master, rebase and work out a few more kinks. I'll submit a new PR when I'm finished and you can re-review then.

fb55 commented 6 years ago

Sounds good — thanks a lot for looking into this!