Coinbase API V2/V3 Updates #818
Conversation
Removed unneeded parsing from ExchangeAPIExtensions
|
@O-Mutt would you like to give this PR a review? |
| private const string ADVFILL = "advanced_trade_fill"; | ||
| private const string CURRENCY = "currency"; | ||
| private const string PRODUCTID = "product_id"; | ||
| private const string PRODUCTS = "products"; | ||
| private const string PRICEBOOK = "pricebook"; | ||
| private const string PRICEBOOKS = "pricebooks"; | ||
| private const string ASKS = "asks"; | ||
| private const string BIDS = "bids"; | ||
| private const string PRICE = "price"; | ||
| private const string AMOUNT = "amount"; | ||
| private const string VALUE = "value"; | ||
| private const string SIZE = "size"; | ||
| private const string CURSOR = "cursor"; | ||
| private const string TYPE = "type"; | ||
| private const string SUBSCRIBE = "subscribe"; | ||
| private const string MARKETTRADES = "market_trades"; | ||
| private const string TICKER = "ticker"; | ||
| private const string EVENTS = "events"; | ||
| private const string LEVEL2 = "level2"; | ||
| private const string PRICELEVEL = "price_level"; | ||
| private const string SIDE = "side"; | ||
| private const string BUY = "buy"; |
There was a problem hiding this comment.
[nit] I think i would have liked to see this split out into a new file, at minimum.
There was a problem hiding this comment.
Ahh, thank you for taking a look at this. It is much appreciated.
Please note: I’m still integrating the CoinbaseAPI with my own software, so there are more changes I’m implementing – as I find errors and what not. (Most egregious are some of the returns from the sockets coming back as undocumented types – which causes parsing fails.) I also found I do need legacy Transactions in my app, so I’m updating the Parse Transaction for V2.
Here what you are seeing is my own idiosyncratic coding style. I hate reusing strings in code. I don’t like the bloat that creates. (This is why you see me using string.Empty vs “”.) The constants are simply strings that are reused in the same code file. Of course I thought about putting these in a static class and in a separate file, I just didn’t due to time-constraint and after deleting the Model folders, separate files were unnecessary. (I didn’t anticipate the Test and Console Apps needing these, as I never compiled those Apps for my own use.)
|
|
||
| private enum PaginationType { None, V2, V3, V3Cursor} | ||
| private PaginationType pagination = PaginationType.None; | ||
| private string cursorNext; |
There was a problem hiding this comment.
[minor] This seems like a catch all that may be better suited for passing a value around or you are depending on statefulness of this class... at least that is my understanding
There was a problem hiding this comment.
No can do. The original V2 allowed pagination forward and backward, and the last API had both cursorPrev and cursorNext being captured on every call in the Response Headers by overriding the ProcessResponse. V3 changed all that, with the pagination info coming back only on certain calls in the Content (with two different formats.) Hence the capture on State Change. I didn’t like having to parse every single call to see if pagination is included, so I setup a pagination state for those calls that needed it and defaulted to none. This still requires double serialization on some calls, but you might note that the Base response code FILTERS this returned data and only returns a single response type – striping the pagination token(s). I took every precaution not to alter the base code too much… so, this. I see no other way without breaking all other Exchange APIs (which I’m not willing to do.)
| if (string.IsNullOrEmpty(cursorNext)) break; | ||
| token = await MakeJsonRequestAsync<JToken>(startURL + cursorNext); | ||
| } | ||
| pagination = PaginationType.None; |
There was a problem hiding this comment.
[minor] This seems odd, shouldn't we store the previous value here and then reset that value back to pagination after the v3cursor is processed instead of .None?
| async (_socket) => | ||
| {"base_size", order.Amount.ToStringInvariant() }, | ||
| {"limit_price", order.Price.ToStringInvariant() }, | ||
| {"end_time", ((DateTimeOffset)order.ExtraParameters["gtd_timestamp"].ToDateTimeInvariant()).ToUnixTimeSeconds().ToString() }, // This is a bit convoluted? Is this the right format? |
There was a problem hiding this comment.
FWIW this is the format used to delta epoch. i.e. number of seconds since epoch (jan 1 '70 UTC)
There was a problem hiding this comment.
Yes, I know. What I don’t know is if the param is supposed to be a string or a long (or if it matters). And this was a note to myself to check if there was a better way of creating this without all the casting/conversions.
| {"limit_price", order.Price.ToStringInvariant() }, | ||
| {"stop_price", order.StopPrice.ToStringInvariant() }, | ||
| {"post_only", order.ExtraParameters.TryGetValueOrDefault( "post_only", "false") } | ||
| //{"stop_direction", "UNKNOWN_STOP_DIRECTION" } // set stop direction? |
There was a problem hiding this comment.
[nit] // TODO what is the plan with this comment?
There was a problem hiding this comment.
As I mentioned in previous comments, this call isn’t completely documented on the Coinbase V3 site. They don’t tell you what the Payload should look like, or what parameters are required/optional. In fact, I pieced these together based on their response examples from their docs (which I know can’t be right – these are.) I haven’t successfully tested a Stop Trade and don’t know what is actually required/returned – I’m guessing. So, this still needs work – and I could use help testing.
| ); | ||
| List<ExchangeOrderResult> orders = new List<ExchangeOrderResult>(); | ||
| // Max return count is 1000 with no pagination available | ||
| JToken array = await MakeJsonRequestAsync<JToken>("/orders/historical/batch?order_status=OPEN" + marketSymbol == null || marketSymbol == string.Empty ? string.Empty : "&product_id=" + marketSymbol ); |
There was a problem hiding this comment.
[nit]
| JToken array = await MakeJsonRequestAsync<JToken>("/orders/historical/batch?order_status=OPEN" + marketSymbol == null || marketSymbol == string.Empty ? string.Empty : "&product_id=" + marketSymbol ); | |
| const params = String.IsNullOrEmpty(marketSymbol) ? string.Empty : $"&product_id={marketSymbol}"; | |
| JToken array = await MakeJsonRequestAsync<JToken>($"/orders/historical/batch?order_status=OPEN{params}"); |
| { | ||
| string message = msg.ToStringFromUTF8(); | ||
| var book = new ExchangeOrderBook(); | ||
| return await GetAmounts(false); |
There was a problem hiding this comment.
[nit] Generally speaking, I find boolean params to be ineloquent and inexpressive, by nature. Not telling you anything about why they are being used or what the outcome is of the param being passed. I tend to avoid them. Again, this is a nit and not explicit need to change
There was a problem hiding this comment.
I don’t disagree – this was from the previous API and I just carried it forward for consistency.
| if (startDate != null) trades = trades.Where(t => t.Timestamp >= startDate); | ||
| if (endDate != null) trades = trades.Where(t => t.Timestamp <= endDate);; | ||
| if (limit != null) trades = trades.Take((int)limit); |
There was a problem hiding this comment.
[minor] Shouldn't this take into account the start and end?
e.g.
| if (startDate != null) trades = trades.Where(t => t.Timestamp >= startDate); | |
| if (endDate != null) trades = trades.Where(t => t.Timestamp <= endDate);; | |
| if (limit != null) trades = trades.Take((int)limit); | |
| if (startDate != null && endDate == null) trades = trades.Where(t => t.Timestamp >= startDate); | |
| if (endDate != null && startDate == null) trades = trades.Where(t => t.Timestamp <= endDate); | |
| if (startDate != null && endDate != null) { | |
| trades = trades.Where(t => t.Timestamp >= startDate && t.Timestamp <= endDate); | |
| } | |
| if (limit != null) trades = trades.Take((int)limit); |
There was a problem hiding this comment.
Won’t make any difference. We’re talking about a max of 100 records here. It’s unfortunate that Coinbase doesn’t provide an historical trades endpoint. I kept looking on both the V2 and V3, as well as past sockets. I could have just left the call blank (API exception), but implemented this mostly as a placeholder. It’s worthless. Do with it what you will.
There was a problem hiding this comment.
With all due respect, it definitely would make a difference. If I only want trades for today to do some logic on (cost analysis for the day) and I send in a start and end and it returns 100 entries less than my end date I would probably get way more than expected.
| var trades = await OnGetRecentTradesAsync(marketSymbol.ToUpperInvariant()); | ||
|
|
||
| if (startDate != null) trades = trades.Where(t => t.Timestamp >= startDate); | ||
| if (endDate != null) trades = trades.Where(t => t.Timestamp <= endDate);; |
There was a problem hiding this comment.
| if (endDate != null) trades = trades.Where(t => t.Timestamp <= endDate);; | |
| if (endDate != null) trades = trades.Where(t => t.Timestamp <= endDate); |
| { | ||
| // Limit is required but maxed at 100 with no pagination available | ||
| limit = (limit == null || limit < 1 || limit > 100) ? 100 : (int)limit; | ||
| JToken trades = await MakeJsonRequestAsync<JToken>("/products/" + marketSymbol.ToUpperInvariant() + "/ticker?limit=" + limit); |
There was a problem hiding this comment.
[nit] IIRC this lib uses MarketSymbol as a type and sets up the marketsymbol in a way that always makes it match the exchange you are working with. I'd have to dig on how that worked but i thought that was already done
This reverts commit f7a8889.
Completed OrderParsing Fully Test PlaceOrder Additional Error Checking on WebSockets Removed Unused functions Separated constants into second file - creating partial class General Code Cleanup
|
I've updated the source and applied Matt's suggestions (thanks again, Matt). All calls were tested for parameters and return values, and I even ran all sockets overnight to test for leaks, etc. Everything works on my side, so I have to bow out of this side-project. Use this, don't use it, it's up to you, but as far as I can tell, this would be the first library to successfully incorporate the new Coinbase Advance Trade API. (and it wasn't easy). I won't be updating the ExchangeSharpTest app, and this no longer compiles. You'll have to do that update, as well as the console app. Cheers, Bob |
There was a problem hiding this comment.
Thank you for the excellent work @BobDeCuir. I will merge these changes now to allow @O-Mutt to test and base his own PRs on if needed. Once @O-Mutt is ready, I will push a new NuGet for wider testing.
I did some minor testing, aside from the fairly lacking logging setup in the class (there are literally no logs) I was able to use it to place successful orders. I'd be happy to push this out wider |
|
Great, I will work on updating the CI and pushing a new release. |
This API now uses Coinbase Advanced Trade V2/V3.
If you are using legacy API keys from previous Coinbase versions they must be upgraded to Advanced Trade on the Coinbase site.
These keys must be set before using the Coinbase API (sorry).