Move to futures

[crackcomm]

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?

[hugues31]

I have to say that futures is not something I'm confortable with (I don't really understand the benefit) but I am open to all contributions.
Actually, the exposed API is fairly simple to use (2 lines to create an API and get a price), it would have to remain so. I would like to see what would this example (or any other practical example) look like if you use futures ? At least just for a quick preview :)

[ainestal]

Same as hugues31, but I'd love to see the use case and an example

…

Sent from my iPhone

On 15 Apr 2017, at 14:45, hugues31 ***@***.***> wrote: I have to say that futures is not something I'm confortable with (I don't really understand the benefit) but I am open to all contributions. Actually, the exposed API is fairly simple to use (2 lines to create an API and get a price), it would have to remain so. I would like to see what would this example (or any other practical example) look like if you use futures ? At least just for a quick preview :) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[crackcomm]

For quick overview see hyper example client.

[ainestal]

What are the advantages of this approach? I fail to understand what we gain in return for the increased complexity

…

On 16 Apr 2017, at 01:02, Łukasz Kurowski ***@***.***> wrote: For quick interview see hyper example client. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[hugues31]

Yes, same here. For me, futures could be used by the lib's users in their apps if they need to. I don't see the point of using futures internally but maybe I'm missing something.

[crackcomm]

It's advantages are that we can make simultaneous requests in one thread, easily select on first response, collect responses or use events like deadlines etc.

In example of Poloniex client, it will be required to change two functions – public_query and private_query.
Function return argument would change from Result<Map<String, Value>, error::Error> to Future<Item=Map<String, Value>, Error=error::Error> (favorably Self::Future).

If you want to know more about futures you can visit tokio.rs.

[hugues31]

So if I understand it right, the Poloniex example was previously (=currently) :

let mut my_api = PoloniexApi::new("api_key", "api_secret");
let list_coins = my_api.return_ticker().unwrap();

and with Futures, simply becomes:

let mut my_api = PoloniexApi::new("api_key", "api_secret");
let list_coins = my_api.return_ticker().wait().unwrap();

Thus, lets the user implements more easily a timeout mechanism for example if I'm correct. The best of it is we can make parallel requests easily (syntax is incorrect but the idea is here..) :

let mut polo = PoloniexApi::new("api_key", "api_secret");
let mut krak = KrakenApi::new("api_key", "api_secret");
let price_polo_future = polo.return_ticker();
let price_krak_future = krak.return_ticker();
let average_price = price_krak_future.select(price_polo_future).map(|(price1, price2)| (price1+price2) / 2.0);

Where with the current synchronous implementation we had to wait the first request to be done to send the next one.
If this is correct that could definitely be a great feature 👍 The only problem I see is that we have to change (mostly) all functions signatures since they return Result<Map<String, Value>, error::Error>. I also have no idea how to reimplement the Coinnect generic API, but maybe there is no problem.

Anyway, thanks for your time and your help

[ainestal]

It looks like a good feature to have. Who is going to implement it?

(I can do it if nobody else volunteers)

[hugues31]

I will not try until next week, so you can maybe send a PR if you need that feature now :)

[ainestal]

I don't really need it for now. I was asking just to continue developing the library.

I'll give it a try this week.

[crackcomm]

Here is a working sample for Poloniex based on your previous code: https://gist.github.com/crackcomm/5fbf16d27acdda5d9af3f6e835690c90

[hugues31]

Thanks for your input! I'll look into it tomorrow

[hugues31]

After a few thoughts, I think Futures add a lot of boilerplate code (in its most trivial implementation through). I don't have enough knowledge to modify what you have linked. Is it possible to include the tokio_core inside the public_query and private_query functions in some way? So the user exposed API remains as simple as I have imagined it above.

[ainestal]

I also tried with the same result as @hugues31

[crackcomm]

extern crate pretty_env_logger;
extern crate futures;
extern crate tokio_core;
extern crate hyper;
extern crate hyper_tls;
extern crate coinnect;

use std::env;

use futures::Future;

use coinnect::{Auth};
use coinnect::poloniex;

fn main() {
    pretty_env_logger::init().unwrap();

    let mut core = ::tokio_core::reactor::Core::new().unwrap();
    let handle = core.handle();
    let http = ::hyper::Client::configure()
        .connector(::hyper_tls::HttpsConnector::new(1, &handle))
        .build(&handle);

    let mut client = poloniex::Client::new(http, Auth {
        key: "".to_owned(),
        secret: "".to_owned(),
    });

    core.run(client.balances().map(|res| {
        println!("res: {:?}", res);
        ()
    })).unwrap();
}

[crackcomm]

I think this change should wait anyway for you to gain confidence in it.

[Peltoche]

Yop,

I will probably make a try for creating a wrapper around the lib. I will not change the old lib, only create a new one wrapping the old one without any logic.

It seem the best if we want to keep the core of the lib synchronous but for me an asynchronous core will be also a great idea. (Maybe the subject to a new Issue?)

[crackcomm]

I personally think the core should be asynchronous because it can be used synchronously and it doesn't work like that vice-versa. Interesting project: futures-await.

[hugues31]

Is there a way to make this possible (as I stated above):

let mut my_api = PoloniexApi::new("api_key", "api_secret").unwrap();
let list_coins = my_api.return_ticker().wait().unwrap();

So that return_ticker() returns a Future and wait() should... wait until response is received.
The idea is to keep the API very straightforward so we can use it both in a synchronous or asynchronous way.

[crackcomm]

It is possible.

[Peltoche]

In order to be completely useful I think #19 should be handle first. Can you make some feedbacks in order to validate (or not)?

[crackcomm]

I uploaded some of my code to github maybe you will be able to learn or use some of it. Feel free to steal it.

[Peltoche]

As promise I start to work seriously on it:

Issue

For now each API create his own http client during his initialization. With Futures it seem a very bad idea (if not impossible). Everyone should (must?) use the same http client.

Proposed Solution

I think we will need to give the http client to each Exchange so everyone run on the same Future::Core loop, so there will be a new breaking change.

Example:

impl BitstampApi {
   // Before
    pub fn new<C: Credentials>(creds: C) -> Result<BitstampApi> {}
  // After
   pub fn new<C: Credentials>(http: hyper::Client<HttpsConnector>, creds: C) -> // ....

Someone see a workaround or a no go?

[crackcomm]

It is surely more efficient.
If not that, you would have to pass a tokio-core handle, additionaly HttpsConnector creates additional thread(/s) for accepting connections.

[Peltoche]

I have just finish a first working version for one route here if you want to make some feedback.

I start a complete rewrite base on the work above if it's ok for you.

PS: Thanks @crackcomm for you code, it was very helpful!

[ainestal]

That's a huge change! Is it possible to split this change in smaller changes that we can TDD?

About the implementation, I think we shouldn't finish here:
let mut my_api = PoloniexApi::new(http, creds).unwrap();

We should use the generic api and then include new connections to exchanges within the same object, something like:

let mut my_api = Coinnect::new();
my_api.new_connection(Exchange::Poloniex, creds);

let ticker = my_api.ticker(Exchange::Poloniex, ETC_BTC);

By doing that we encapsulate the specific exchange initialization in the Coinnect api and the user wouldn't be forced to initialize the handler or any other specifics.
At the same time coinnect feels like an api capable of interacting with different exchanges instead of a collection of apis with similar characteristics.

[hugues31]

@ainestal maybe instead of :

let ticker = my_api.ticker(Exchange::Poloniex, ETC_BTC);

we could use :

let ticker = my_api.from("my_poloniex_account_name").ticker(ETC_BTC);

The Creds store an account name so we could use it to identify the exchange, etc.. This way all methods (ticker, etc) do not have the Exchange parameter and we could do something like this:

let balance_1 = my_api.from("polo_account_1").balances()
let balance_2 = my_api.from("polo_account_2").balances()

[ainestal]

@hugues31 In both approaches you have to provide a reference to the exchange you want, the operation you want to perform and some extra information, usually the pair. For me they are very similar. I don't have a strong opinion here, I prefer the first one, looks more explicit to me, but I'm fine with any of them.

[Peltoche]

I totally agree with you guys! The async stuff is a pain-in-the-ass code to write and I don't think the users want to be bothered by it, so we should wrap it around an synchronous API with possibility to retrieve Futures.

@hugues31 in you example:

let balance_1 = my_api.from("polo_account_1").balances()
let balance_2 = my_api.from("polo_account_2").balances()

We can easily make these two call asynchronous which allow great performance improvement.

So I redirect you on #19 where I proposed some ideas for a wrapper which look like:

fn main() {
    let clients = Coinnect::new();

    for cred in creds {
        clients.add(creds).unwrap())
    }

    // or simply Coinnect::new_from_file()

    let mut res;

   // run the two ticker request in parallel 
    res = clients.ticker()
        .from("my_bitstamp_account_name")
        .from("my_kraken_account_name")
        .exec()
        .unwrap();

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

[hugues31]

@Peltoche I like this idea. So exec() is just a wrapper for sending all requests asynchronously based on selected exchanges but this function remains synchronous for the user so it keeps simple. And we could always implement a function that returns the Future(s) associated with the exec() function.

[Peltoche]

Yep, the ticker() create a query-builder which can be dynamically composed by adding some rules (from() by example) then run by exec().

[ainestal]

I like the idea. I specially like the "let's not annoy our users" approach.

[crackcomm]

@Peltoche Thanks, I learned from your implementation too.
@ainestal @hugues31 We also need account selection.
Market name doesn't have to be specified, it's already embed in account struct, that's a good thing.

Method from in rust should be reserved for From::from and from in general is for conversion between types.
Name of the method should be closer to with_account.

[crackcomm]

I don't necessarily like the idea of multiple request builder like that. I think it should be a simple client which then can be used to process requests in parallel.