dougdellolio / coinbasepro-csharp

The unofficial .NET/C# client library for the Coinbase Pro/GDAX API
MIT License
192 stars 90 forks source link

Handling Exponential Notation in Depth Websocket Payloads #160

Closed mchandschuh closed 5 years ago

mchandschuh commented 5 years ago

While receiving depth data via websocket, I've recently been experiencing serializing failures due to coinbase sending data in exponential format. The below example pertains to the new coin but I think I experienced this once before the new coin was released. Here's the full content body as intercepted via JsonConfig.DeserializeObject where it throws a JsonReaderException. For brevity I've trimmed the body down quite a bit:

{
  "product_id": "ZRX-BTC",
  "type": "snapshot",
  "bids": [
    [
      "5.1e-7",
      "150000"
    ],
    [
      "5e-7",
      "10000"
    ],
    [
      "1e-7",
      "15288"
    ],
    [
      "5e-8",
      "8000"
    ],
    [
      "3e-8",
      "150000"
    ],
    [
      "2e-8",
      "150000"
    ],
    [
      "1e-8",
      "600000"
    ]
  ],
  "asks": [

  ]
}
mchandschuh commented 5 years ago

While this is certainly not an ideal solution to this issue (I'm pretty sure coinbase shouldn't be sending exponential notation), it is able to read the data from the socket with the exponential notation. I tried a few other things, but didn't spend too much time looking for an ideal solution. For anyone else experiencing this issue, perform the following steps:

  1. Go to ./CoinbasePro/Shared/Utilities/JsonConfig.cs
  2. Add the following class definition. I added this as a private nested class, but really it just needs to be accessible in the JsonConfig.SerializerSettings object initializer as we'll see in the next step.

    private class DecimalJsonConverter : JsonConverter
    {
    public override bool CanRead => true;
    public override bool CanWrite => false;
    
    public override bool CanConvert(Type objectType)
    {
        return objectType == typeof(decimal);
    }
    
    public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer)
    {
        throw new NotImplementedException();
    }
    
    public override object ReadJson(JsonReader reader, Type objectType, object existingValue, JsonSerializer serializer)
    {
        var value = reader.Value as string;
        if (string.IsNullOrEmpty(value))
        {
            return null;
        }
    
        try
        {
            return decimal.Parse(value);
        }
        catch
        {
            return (decimal) double.Parse(value);
        }
    }
    }
  3. Use the DecimalJsonConverter in JsonConfig.SerializerSettings in the Converters property:
    private static JsonSerializerSettings SerializerSettings { get; } = new JsonSerializerSettings
    {
    FloatParseHandling = FloatParseHandling.Decimal,
    NullValueHandling = NullValueHandling.Ignore,
    ContractResolver = new DefaultContractResolver
    {
        NamingStrategy = new SnakeCaseNamingStrategy()
    },
    Converters = new List<JsonConverter>
    {
        new DecimalJsonConverter()
    },
    Error = delegate(object sender, ErrorEventArgs args)
    {
        if (args.CurrentObject == args.ErrorContext.OriginalObject)
        {
            Log.Error("Json serialization error {@OriginalObject} {@Member} {@ErrorMessage}",
                args.ErrorContext.OriginalObject,
                args.ErrorContext.Member,
                args.ErrorContext.Error.Message);
        }
    }
    };

This will at least get things running for now until we can either come up with a more permanent solution or coinbase fixes their websocket to stop sending exponential notation. I'm not familiar with the Machine.Specifications unit testing framework, so I didn't take a stab at writing tests for this.

dougdellolio commented 5 years ago

@mchandschuh thanks for reporting this. I am seeing this locally now too. I have sent a ticket to their support team to get more information around this to see if it is a data issue on their end that can be fixed or it is something we should account for here. They usually respond quickly for API support requests so I'll keep you posted. Otherwise, if we don't hear back soon we should go ahead and add the fix in.

mchandschuh commented 5 years ago

Here's a commit with the above described changes: https://github.com/1Konto/coinbasepro-csharp/commit/f8fd0ccb46642320f4de78ecd9403cae421c07ba

dougdellolio commented 5 years ago

fyi support responded 16 days ago to my request with this:

... I have filed a bug report, and requested our engineering team to investigate further. Thank you for bearing with us in the meantime

I haven't received a response since so I just sent a followup this morning. In the meantime I am going to add your fix in

bthtrade commented 5 years ago

Thank you for highlighting this JsonConfig file I've never heard about before. In the converter, I personally use:

    public override object ReadJson(JsonReader reader, Type objectType, object existingValue, JsonSerializer serializer)
    {
        var value = reader.Value as string;
        if (string.IsNullOrEmpty(value))
        {
            return null;
        }
        return decimal.Parse(value, NumberStyles.Number | NumberStyles.AllowExponent);
    }
dougdellolio commented 5 years ago

closing. this is fixed with v1.0.25. let me know if you see any other issues