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

Add HitBTC, Cryptopia and some other exchange IDs #148

Open
wants to merge 15 commits into
base: binance
Choose a base branch
from
Open

Add HitBTC, Cryptopia and some other exchange IDs #148

wants to merge 15 commits into from

Conversation

OlegVic
Copy link

@OlegVic OlegVic commented Feb 3, 2018

No description provided.

@cryptoeax
Copy link
Owner

Do you have any plans to contribute any of these exchanges back? :-) I prefer to first add a few more exchange backends before adding more. It seems kind of unfair for me to just keep adding IDs to my project for which I won't see any other code contributions...

@cryptoeax
Copy link
Owner

Nice, thank you! I will try to review this PR and also test it out in the next few days...

@cryptoeax cryptoeax changed the title Add other exchanges IDs Add HitBTC and some other exchange IDs Feb 5, 2018
@cryptoeax
Copy link
Owner

How well is HitBTC working for you currently?

@OlegVic
Copy link
Author

OlegVic commented Feb 5, 2018

Not good, many things in CCXT adapter not compatable. And i build my own branch with updated CCXT. About HitBTC you can see code in my profile

@cryptoeax
Copy link
Owner

Can you please submit a PR for the CCXTAdapter fixes you made too? This one only includes 8ad9a4b as far as I can see. Thanks!

@cryptoeax
Copy link
Owner

(I'd like to test the exchange before merging it, so having something that works would be really helpful.)

@cryptoeax
Copy link
Owner

How well does Cryptopia work? I really wish you didn't mix multiple exchange backends in the same PR. This makes it impossible to merge your PR as is without me spending weeks to test both HitBTC and Cryptopia and make sure they are both bug free. :-( Just to make sure you aren't expecting a fast turn around, each exchange being tested takes quite a long time, and keeping them in separate PRs would have helped me merge the one that is ready first.

@cryptoeax cryptoeax changed the title Add HitBTC and some other exchange IDs Add HitBTC, Cryptopia and some other exchange IDs Feb 9, 2018
@cryptoeax
Copy link
Owner

Please avoid adding any other features to this PR. If you have other changes to contribute, please spend some time to make separate PRs for them. Thanks!


}

// Changed functions!
Copy link
Owner

Choose a reason for hiding this comment

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

Why have you made these changes?

Copy link
Author

Choose a reason for hiding this comment

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

your adapter not universal! and have specific code

return null;
}
$amount_fee = round($amount * $fee, $precisions[ 'amount' ]);
print( $this->prefix() . "INFO BUY [$tradeable]:\n" );
Copy link
Owner

Choose a reason for hiding this comment

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

Can you please remove the debugging statements from your code before submitting? Thanks!

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

Successfully merging this pull request may close these issues.

2 participants