Equality operation for new symbols is not working.

[sguryev]

Hi there,

I have faced one new interesting issue. I'm having a kind of local cache of active symbols. And I have a problem with Symbol to string comparing FOR NEW SYMBOLS. There are 3 new symbols for the GNT asset.

Look at the screen:

So for the new symbol GNTBTC I have to call .ToString() apart from the known ETHBTC.
I think it can be related to the string interning maybe. But I was not able to find the difference between ETHBTC and GNTBTC.

Also there is the problem with the Symbol.Cache related to this issue:

There are 4 symbols related to BCH asset:

And it looks like removing part of the syncing is not working during cache updating.

Binance/src/Binance/Symbol.cs

Line 839 in a2d4362

if (!symbols.Contains(symbol))

[sguryev]

Adding code for copy-paste:

var symbols = (await binanceApi.GetSymbolsAsync(cancellationToken).ConfigureAwait(false)).ToArray();

var hasETHBTC = symbols.Any(s => s == "ETHBTC");
var hasGNTBTC = symbols.Any(s => s == "GNTBTC");

var equalsETHBTC = symbols.Any(s => s.Equals("ETHBTC"));
var equalsGNTBTC = symbols.Any(s => s.Equals("GNTBTC"));

var hasETHBTC1 = symbols.Any(s => s.ToString() == "ETHBTC");
var hasGNTBTC1 = symbols.Any(s => s.ToString() == "GNTBTC");

var equalsETHBTC1 = symbols.Any(s => s.ToString().Equals("ETHBTC"));
var equalsGNTBTC1 = symbols.Any(s => s.ToString().Equals("GNTBTC"));

var symbolGNTBTC = symbols[302];

var b = symbolGNTBTC == "GNTBTC";
var b1 = symbolGNTBTC.ToString() == "GNTBTC";

var a = symbols.Where(s => !Symbol.Cache.ContainsKey(s)).ToArray();
var a1 = symbols.Where(s => !Symbol.Cache.ContainsKey(s.ToString())).ToArray();

var strings = Symbol.Cache.Keys.Where(k => symbols.All(s => s != k)).ToArray();
var strings1 = Symbol.Cache.Keys.Where(k => symbols.All(s => s.ToString() != k)).ToArray();

await Symbol.UpdateCacheAsync(binanceApi);

var a2 = symbols.Where(s => !Symbol.Cache.ContainsKey(s)).ToArray();
var a3 = symbols.Where(s => !Symbol.Cache.ContainsKey(s.ToString())).ToArray();

var strings2 = Symbol.Cache.Keys.Where(k => symbols.All(s => s != k)).ToArray();
var strings3 = Symbol.Cache.Keys.Where(k => symbols.All(s => s.ToString() != k)).ToArray();

[sguryev]

Oh man.

Implicit string conversion is working based on the Cache

Binance/src/Binance/Symbol.cs

Line 349 in a2d4362

public static implicit operator Symbol(string s)

So I have to update cache first and then make comparing. So if i have the new symbols now - comparing symbol with string is not working as expected. It's a really confusing. string=>Symbol implicit conversion wins and compiler says nothing. But it works on top of the Cache which can be outdated. I suggest to add the property Name (or just leave .ToString() approach) and remove the string=>Symbol implicit conversion

[sonvister]

@sguryev, yes, the string-to-Symbol conversion uses the Cache.

I have removed that implicit conversion and moved the functionality to Symbol.Get() (the Cache can be accessed directly too). The Symbol-to-string implicit conversion and comparisons should work as expected now for new and cached Symbols.

[sonvister]

Also, I considered embedding the Cache updating within the GetSymbolsAsync() call and leaving the string-to-Symbol implicit conversion. But that didn't cover Symbols instantiated directly (with new Symbol(...)).

Let me know what you think and/or your results if you can test the code before I create 0.2.0-beta4. Thanks.

[sguryev]

Good, thank you!. Give me some time please

[sguryev]

I have removed that implicit conversion and moved the functionality to Symbol.Get()

Great!

The Symbol-to-string implicit conversion and comparisons should work as expected now for new and cached Symbols

Great. I'm curious why it used string-2-Symbol conversion instead of Symbol-2-string. I was not able to find the reason. I though about the order and tried string == Symbol and Symbol == string and string-2-Symbol won in both cases.

I considered embedding the Cache updating within the GetSymbolsAsync() call and leaving the string-to-Symbol implicit conversion.

I thought about automatic updating as well. But string-2-Symbol implicit conversion implementation is too confusing. Especially for the direct instantiating (storing the whole Symbol in some 3rd party store and then operating with them seems pretty possible case). Do you use caching for something else except string-2-Symbol conversion? Actually I don't consider it as pretty useful helper. I use IDistributedCache which is under full my control and can use Memory\Redis\SQL\etc. sources. Also Binance has added LOOM coin (3 new symbols) today again so your hardcoded cache is outdated again. Do you really want to take part in the race? (especially if it doesn't cover offline operations with string-2-Symbol conversion?)

[sguryev]

So short answers are:

1. Great changes
2. I don't like the idea with Cache automatic updating because it doesn't cover the new offline cases
3. Let's consider removing the Caching completely.

btw, did you review the ConcurrentDictionary for the cache purposes?

[sonvister]

As for the runtime's choice of Symbol over string for implicit conversion target type I am guessing it is because Symbol is the most specific target type.

The Symbol cache is there as a convenience for anyone referencing all symbols repeatedly since that information is relatively static and slow to query. I don't see any reason to remove that especially since that information is still required if using ClientOrder validation.

Having an abstraction around the Symbol cache query and updating, perhaps an ISymbolCache interface would be better. Then a default implementation would use the dictionary and an alternate implementation as an IDistributedCache adapter could be created in your case.

Also, it was never a goal or requirement to create a new release for every new symbol added by Binance (others have brought this up and I have that stated in the documentation too). The static assets and symbols were just for convenience/reference. However, looking at them again, it would be better if the static properties also referenced the updated cache instances....

I may have considered ConcurrentDictionary before, but looking again at the update process, I would still choose to lock/unlock the dictionary once rather than multiple times with each access.

So, I'll look into:

• Creating symbol/asset cache abstraction(s) and default implementation(s).
• Modifying static assets/symbols to reference updated cache instances.

[sguryev]

If Cache is required I would rather leave it empty by default. And throw the exception using ClientOrder. Otherwise it looks false-positive (we have cache, it has some symbols, but it's outdated and needs to be refreshed from the beginning at the step number zero)

[sonvister]

The static assets and symbols (I don't plan to remove those) should have something to reference and with these latest changes they will be referencing the statically initialized cache values. Unless constantly polled and updated (then why use a cache) some of the cached data will at some point will become outdated, and dealing with stale data is inevitable. It has been recommended to update the cache at program startup and at intervals. It may as well start out the best that it can be (so, I don't like the idea of leaving it empty with all null static definitions). If there is old data (at any time), then the user will get an exception eventually anyway when placing an order (unavoidable, so why duplicate with more exceptions).

[sonvister]

Let me know what you think of these changes. I really hope this doesn't cause issues... like with issue #60.

[sguryev]

I have left a minor comment-suggestion in a commit. Feel free to approve it or ignore. The rest is great!

[sonvister]

I've refactored to use Set() and Clear() and reduced interfaces to IObjectCache<T>.

[sguryev]

Looks really great!

[sonvister]

0.2.0-beta4 released.