Ohkthx / cbadv-rs

Coinbase Advanced API, written in Rust.
https://crates.io/crates/cbadv
MIT License
5 stars 6 forks source link

Missing field in struct Candle #2

Closed py-chemist closed 9 months ago

py-chemist commented 9 months ago

Hello,

The "product_id" field is missing in the Candle struct.

Thank you.

P.S Thanks for such a great library!

Ohkthx commented 9 months ago

Thank you for the kind words and bringing this up. I base the structs mostly off of the expected result from a successful API request. When checking the Get Product Candles documentation, the successful request should return the following (as of 30 Sept. 2023):

image

Candle Struct for reference.

Even if the product_id was an expected attribute, I would still be hesitant to add it to the Candle struct. The reasons for this are:

My personal use case for this project sometimes holds millions of Candles in memory. Having the product_id populated for each candle would unnecessarily increase the footprint for me. I am open to hear of your use case, and I am excited you are using this project!

py-chemist commented 9 months ago

Thank you for your detailed explanation and sorry for not providing a background of my case.

I need to use websocket stream to get updates of the candles as quick as possible. Currently, I am getting updates via websocket for ~ 285 tickers/product_ids and having the product id is very important to me. If you know how to identify the incoming websocket data without product_id, please let me know. Just in case, the structure of the ws response for the "candles" channel, which includes product_id, can be found here.

P.S I am very new to Rust and hope that your crate will help me to understand this programming language better. Also, at some point, I am planning to migrate my trading bot/bots from Python to Rust using cbadv-rs.

Ohkthx commented 9 months ago

P.S I am very new to Rust and hope that your crate will help me to understand this programming language better. Also, at some point, I am planning to migrate my trading bot/bots from Python to Rust using cbadv-rs.

I also moved to Rust recently and this was my first larger project to get my feet wet. The reason I created this was to specifically move my bots using the depreciated CoinBase Pro API in both Python and NodeJS.

I see what you mean, you're talking about the WebSocket variant of candles. That was an oversight on my part. When I added 4d722e6f6cf5952fdee9b26f700bc8704bdf3886, the CandleChannel did not exist yet. When I updated WebSocket channels to add CandleChannel, I assumed that the Candle struct it provided was the same structure. For your use case, I agree 100% that you need to have that property on candles to help identify what data is coming in.

As far as implementation, I am thinking we add an additional struct specifically for the WebSocket variant that way both parties win. We get the expected outputs returned from both WebSockets and REST clients. I'm thinking WebSocketCandle as a struct name, with a function to create a normal Candle from any WebSocketCandle such as to_candle().

How do you feel about that? I am open to additional input or suggestions.

py-chemist commented 9 months ago

That sounds good to me but the final decision is up to you since you know the project much better.

Thanks for working on that.

Ohkthx commented 9 months ago

If you don't mind, I would like to know more about your use case. Are you storing these candles in a database? Are you performing technical analysis? You don't have to go into details, more curious so I can make a better decision.

One alternative is deviating slightly from the expected output of the WebSocket where the returned struct is:

struct WebSocketCandle {
    product_id: String,
    data: Candle,
}

Original discussed implementation:

struct WebSocketCandle {
    product_id: String,
    start: u64,
    low: f64,
    high: f64,
    open: f64,
    close: f64,
    volume: f64,
}

Benefits of the alternative I see is that it will keep the underlying Candle struct included so that you can access its properties, but access the raw candle itself without calling a function. ie. WebSocketCandle.data.close to access the close data. If you wanted the raw candle, WebSocketCandle.data

That sounds good to me but the final decision is up to you since you know the project much better.

haha, although true, I am not building and maintaining this for just me! There's several inconsistencies in the current CoinBase Advanced API between their implementation and documentation. On top of that, it is still subject to large changes we will have to adapt to. So getting things setup in a way that feels better for us is most important.

py-chemist commented 9 months ago

Sure I can share my case. Currently ( I am talking about my python bot) I store, preprocess and apply TA and trade on websocket data in memory using Polars. It's all done in one script but I think that I'll separate data gathering and cleaning from TA and trades in two scripts. In that case, I'll need to save websocket data in a database. There are many types of databases and I haven't done any research yet which db is better to handle time series data.

I like the alternative option. It's short and there is no need to create a separate Candle struct for websocket data. Let's go with

struct WebSocketCandle {
    product_id: String,
    data: Candle,
}

Thanks.

Ohkthx commented 9 months ago

Alright, in a5da310be743b2a07b54b2fc9685f717f909bc2c I added the CandleUpdate struct:

struct CandleUpdate {
    product_id: String,
    data: Candle,
}

I opted for that name, because other updates from the WebSocket follow the same pattern ItemNameUpdate. Thank you again for bringing this to my attention and helping make this crate better! For now it sits as cbadv = "1.1.9-beta.1" and I will publish it as a non-beta version in the coming days assuming there are no issue.

If you're interested in TA, there are several TA crates for Rust! I created my own called tatk and it hosted on crates.io as well. I wrote it because I found there to be slight issues with some of the more popular ones, feel free to contribute or check it out.

I'll keep this open until I publish it as non-beta, let me know if all works out!

py-chemist commented 9 months ago

Thank you very much for acting so fast. I really appreciate that! I'll test the updated code and let you know how it goes.

I'll definitely check your crate takt out since I'll need a TA crate/s in Rust at some point.

You have several crates related to trading, it seems like you are planning to "hack" the crypto world! Great job.

One last thing, may I contact you if I have questions (not necessarily issues) related to cbadv-rs? If yes, is github a good option or you prefer another way to be contacted?

Thank you again for your efforts!

Ohkthx commented 9 months ago

I'll definitely check your crate takt out since I'll need a TA crate/s in Rust at some point.

I would appreciate it, especially second eyes on all of the algorithms implemented.

You have several crates related to trading, it seems like you are planning to "hack" the crypto world! Great job.

It ended up building a suite of tools / crates, including a Discord Webhook crate for pushing notifications on trades ands news too. As more requirements come up, you can bet that I will be expanding.

One last thing, may I contact you if I have questions (not necessarily issues) related to cbadv-rs? If yes, is github a good option or you prefer another way to be contacted?

GitHub works for me, feel free to message me.

py-chemist commented 9 months ago

It looks like it's working fine. I get the updated candles with the product_id property.

Thanks for updating the code quickly!

Ohkthx commented 9 months ago

Updated crate to version 1.1.9 with the changes, thanks again for the input!