I'm opening this issue as a discussion thread.

Looks like the Bitcoin fork will happen in the next future, not far from now. There will be different names for the Bitcoin Unlimited and Bitcoin Core across different exchanges. Luckily for us look like the 3 exchanges we are currently supporting will maintain the BTC (or XBT) for Bitcoin Core (the current bitcoin) and will use a different name for Bitcoin Unlimited. For me, with the information I currently have, that means we could just continue using the current convention and create new pairs and currency with the new name for the Bitcoin Unlimited.

What do you think about this?

block_or_continue is waiting for the time passed between the last request and the current one. It's not waiting until the threshold is met.

Since different exchanges offer different types of data, what is the least common functions we want to implement in all the exchanges integrated?

In other words, how do the trait ExchangeApi has to be?

Hello,

I was trying to call returnChartData of the Poloniex API. But guessing from the output of my code, there seems to be a parsing error.

Interactively in the browser it would be the following request, which executes fine:
https://poloniex.com/public?command=returnChartData&currencyPair=BTC_STR&start=1527265203&end=1528993203&period=300

My Code:
``
let credentials = PoloniexCreds::new("test", "api_key", "api_secret");

let mut poloniex_api = PoloniexApi::new(credentials).unwrap();

let chartdata = poloniex_api.return_chart_data("BTC_STR", "1527265203", "1528993203", "300");

println!("{:?}", chartdata);
``

Output:
Err(Error(BadParse, State { next_error: None, backtrace: None }))

This should be more safe to use (wrong input from the users, Creds used for the wrong exchange...)

I would love to contribute in making this client use hyper on tokio with futures use. How much are you willing to make breaking API changes?

Issue

In the current state the config file look like:

     {
         "account_bitstamp": {
             "exchange"   : "bitstamp",
             "api_key"    : "1234567890ABCDEF1234567890ABCDEF",
             "api_secret" : "1234567890ABCDEF1234567890ABCDEF",
             "customer_id": "123456"
         }
     }

• The key object account_bitstamp not very practical and the exchange field is useless.
• We need to ask the key used for each config object:

pub fn new_from_file(config_name: &str, path: PathBuf) -> /...

• We can't load automatically a bunch of config in one time (as we don't know the keys used)

Solution

We force the key format to market name and we remove the useless exchange field:

     {
         "bitstamp": { // mandatory format
             "api_key"    : "1234567890ABCDEF1234567890ABCDEF",
             "api_secret" : "1234567890ABCDEF1234567890ABCDEF",
             "customer_id": "123456"
         }
     }

• We simplify the new_from_file proto and made it generic:

pub fn new_from_file(path: PathBuf) -> /...

//allow some cool future features like

let clients = Coinnect::new_clients_from_file("./config")
for client in clients {
  client.ticker().unwrap()
}

The time library is deprecated and has precision errors

As I see you only use SHA-2 and HMAC, so you don't need the whole rust-crypto (moreover it's not developed anymore). Instead you can use crates from RustCrypto organization which is a modular rework of rust-crypto. In addition to smaller size, those crates are written in completely pure Rust.

Ad I understand it the Coinnect client is used for create a generic way to handle client, so the main goal for this feature is to manage several clients at the same time. But for now the Coinnect struct can handle only one Exchange at a time so if we need to make a action on several Exchanges we must create several Coinnect client, which lead to something like this:

fn main() {
    let markets = vec!{Exchange::Bitstamp, Exchange::Kraken, Exchange::Poloniex}

    let clients = Vec::new();

    for market in markets {
        clients.push(Coinnect::new(market, "key", "secret", None).unwrap());
    }

    for client in clients {
        println!("res: {}", client.ticker());
    }
}

This method have some issues:

• The user must handle his own list of clients
• We can't improve speed by making all the call asynchonous and parallel
• The clients doesn't share the same http client

I would propose to modify the Coinnect API to handle several clients, something like this:

fn main() {
    let markets = vec!{Exchange::Bitstamp, Exchange::Kraken, Exchange::Poloniex}

    let clients = Coinnect::new();
    // or simply Coinnect::new_from_file()

    for market in markets {
        // EDIT: little typo fix here.
        clients.add(market, "key", "secret", None).unwrap())
    }

    let mut res;

    res = clients.ticker()
        .from(Exchange::Bitstamps)
        .from(Exchange::Kraken)
        .exec()
        .unwrap();

    res = clients.ticker()
        .from_all()
        .exec()
        .unwrap();
}

• Only one client (and http client)
• Requests can be parallel
• Avoid code for clients
• Wrap all the clients into a simple struct (no more Box<ExchangeAPI for the user)

I'd like to open the discussion about the approach to all the new APIs that implement websockets. More and more exchanges are going into the path of a continuous stream of data, instead of the pull approach of the classic APIs.

Would it be a useful feature to implement them? Is it even possible? Can the websockets work together within the same API as the classic exchanges?
So, what should we do about them?

While trying to use the library I am running into requirement failures

error: failed to select a version for the requirement hmac = "^0.3"``

How can I resolve this?

I tried placing an order with this code:

api.buy_market(Pair::BTC_USD, BigDecimal::from_str("0.001")

I get this error back from Bitstamp:

{"reason": String("Missing amount POST parameter"), "status": String("error")}

I looked at the code and I can see that buy_market calls private_query passing a params map containing the amount but the amount is not propagated in the actual HTTP request. It needs to be sent as a POST parameter.

Hello,

After some research, I decided to give a try to Futures to come up with a simple async API. The main idea is to keep everything very straightforward for the user (the event loop is created by the API).

The idea originates from here : #9

The source code is :

#![feature(conservative_impl_trait)]

extern crate futures;
extern crate hyper;
extern crate tokio_core;
extern crate serde_json;

use futures::Future;
use futures::future;
use futures::stream::Stream;

use hyper::Client;
use serde_json::Value;


struct Api {
    core: tokio_core::reactor::Core,
}

impl Api {
    pub fn new() -> Api {
        Api {
            core: tokio_core::reactor::Core::new().unwrap(),
        }
    }

    pub fn time(&self) -> impl Future<Item = Value, Error = hyper::Error> {
        let url = "http://date.jsontest.com";
    
        let uri = url.parse::<hyper::Uri>().unwrap();

        let handle = self.core.handle();
        let client = Client::new(&handle);

        client.get(uri).and_then(|res| {
            res.body().concat2().and_then(move |body| {
                let v: Value = serde_json::from_slice(&body).unwrap();
                future::ok((v))
            })
        })
    }
}

fn main() {
    // create the API and the Tokio core
    let mut api = Api::new();

    // BENCHMARK THE SYNC VERSION
    let work_1 = api.time(); // create the handle, the client and send request inside a Future
    let res = api.core.run(work_1).unwrap();
    let time_1 = res["milliseconds_since_epoch"].as_f64().unwrap();

    let work_2 = api.time(); 
    let res = api.core.run(work_2).unwrap();
    let time_2 = res["milliseconds_since_epoch"].as_f64().unwrap();

    println!("\nSync version takes {} milliseconds to execute.", time_2 - time_1);

    // BENCHMARK THE ASYNC VERSION
    let work_async = api.time().join(api.time());
    let (res_1, res_2) = api.core.run(work_async).unwrap();
    let time_1 = res_1["milliseconds_since_epoch"].as_f64().unwrap();
    let time_2 = res_2["milliseconds_since_epoch"].as_f64().unwrap();

    println!("\nAsync version takes {} milliseconds to execute.\n", (time_2 - time_1).abs());
}

Cargo.toml :

[dependencies]
futures = "0.1"
hyper = "0.11"
tokio-core = "0.1"
serde_json = "1.0.5"

The code below returns:

Sync version takes 198 milliseconds to execute.

Async version takes 5 milliseconds to execute.

What do you think? Since it's a huge rewrite it would be nice to have some feedbacks :)

I find it confusing when using the library that these 2 types are not in the types mod. It also forces the user to import other 2 mods instead of just types.

I also understand they are different from the rest of types implemented in the types mod.

So I'd like to ask you about moving them. What are your thoughts?

Looks like the main Price type is an alias for f64. As we know, using floats for currency is a bad idea due to precision issues.

It seems like this crate is the upcoming standard: https://crates.io/crates/bigdecimal -- based on some discussion I've seen and the fact that Diesel is using it. It's been oddly quiet for a while though.

Actually, not all return value from Poloniex is a JSON object, for now, return_order_book is an exception. I make a change to these methods to return Result<T>, which delegates the choice to the user that he can decide what type the result value is. Commit.

Poloniex supports several additional order types like FillOrKill. I have an implementation here, which introduces additional parameters to buy, sell, and move_order. Not sure if it works for you.

Issue

For now:

• Everybody push or merge directly on master
• There is no lib versionning
• The documentation is manually updated

Proposed Solution

Put some rules.

Here my propositions:

• Everybody make PR
• Every PR target a "dev" branch
• When the "dev" branch is ready -> merge on master with a tag and release a cargo version with the same tag + automatically create the new doc (I can find/create the script if needed)
• Make every merge in --non-fast-forward ( available in @hugues31 options) for more readability.

It has been deprecated: announcement.

Is there any functionality you would require from another library before this would be possible?

Build log says :

error[E0460]: found possibly newer version of crate `idna` which `hyper` depends on
  --> .cargo/registry/src/github.com-1ecc6299db9ec823/coinnect-0.5.3/src/lib.rs:33:1
   |
33 | extern crate hyper;
   | ^^^^^^^^^^^^^^^^^^^
   |
   = note: perhaps that crate needs to be recompiled?
   = note: crate `idna` path #1: /home/cratesfyi/cratesfyi/debug/deps/libidna-e17fe1ecab30d084.rlib
   = note: crate `idna` path #2: /home/cratesfyi/cratesfyi/debug/deps/libidna-61e19868261f71ed.rlib
   = note: crate `hyper` path #1: /home/cratesfyi/cratesfyi/debug/deps/libhyper-103a835b55757827.rlib

error: aborting due to previous error

And I have no idea on how to solve that

Hello all,
Since the add of fn for_each<F>(self, f: F) we need Rust version 1.21 and above for Coinnect to compile.
We then need to change the travis file config in order to fix this. I may send a PR later in the day.

Issue

Each client can have its proper need for initialization and connection. The current way is to put everything in arguments but it will quickly become very heavy.

Example:

BitstampApi::new(/*Hashmap<&str, &str>*/);
Kraken::new(&str, &str);
FuturAPI::new(....)

The other issue is for Coinnect which must handle all the differences. For now it need to take all the arguments, even the useless:

Coinnect::new(Exchange::Poloniex, "key", "secret", None /*useless*/);

This method is not sustainable, what is the next market need another specific argument? And the next one? etc...

Proposed solution

Create a generic Credential which handle all the specific parsing for each market config and an interface returing a Hashmap<&str, &str>:

let polo_creds = Poloniex::Credentials::new("key", "secret")
Coinnect::new(Exchange::Poloniex, polo_creds)
// or
let polo_creds_2 = Poloniex::Credentials::from_file("./creds");
Exchanges::Poloniex::new(polo_creds);

The markets new method could look like:

fn new(creds: T): where T: Credential {
  let data: Hashmap<&str, &str> = creds.retrieve();
  let secret = data["secret"]
 // ...
}

@hugues31 What do you think about creating OpenAPI specifications for exchanges and generating client code?

Hi,

I see that for now the error handling is hand made. It's cool but it have a lot of missing features:

• It not generic
• It lost many logs
• It heavy to add a new error

Without a proper error handling system you can't correctly return all the errors which lead to all the unnecessary unwrap call. If you don't return the errors now you soon will be locked with errors that you can't return to the client because the API doesn't allow it, so in the current state a can't use the lib.

A would like to propose you to move to a new system with the error_chain package and improve the error handling.

It's an heavy operation and I lead to a lot's of breaking changes so I forked the project and I will make you a PR soon in case you are interested.

With the idea in mind of gathering feedback from our users, I would like to propose to the community to ask for inclusions of new exchanges in the library. Something as simple as:

What exchange/s would you like to include in Coinnect and why ?

What do you think @hugues31 ?

In some cases, I'd like to call these in a burst mode, I don't want to be blocked by the API internally...