search

benmarten / CryptoETF

Portfolio Overview Tool for Your CryptoCoin Capitalization-Weighted Portfolio.
Apache License 2.0
396 stars 77 forks source link

Hide zero amount for binance #35

Open barryvdh opened 6 years ago

barryvdh commented 6 years ago

Re-do #32 against correct branch.

benmarten commented 6 years ago

I'm actually not sure if we should do this, in this manner... Right now I intended to add all coins that are coming from the exchanges, for the purpose of keeping track where to buy them, even if you hold 0. The main logic is here: https://github.com/benmarten/CryptoETF/blob/master/src/model/Coin.js#L93 Maybe we can add a setting to hide the exchanges that have a 0 holding? What do you think?

barryvdh commented 6 years ago

I'm not sure, but Binance lists ALL the coins in the exchange. I just used 2 coins in binance bit it adds a lot more.

gosuto-inzasheru commented 6 years ago

Thanks for this quick fix. I ran into this because even though I had set "hideMissingCoins": true, it still displayed all these 'empty' coins from Binance.

Edit: This is not how it should be fixed though. Poloniex also displays empty bags. Opened #38

barryvdh commented 6 years ago

If Poloniex also does that, shouldn't we also filter that?

gosuto-inzasheru commented 6 years ago

Yes, but I don't think it should be an exchange specific fix. It's OK to parse these empty bags (for example so that you at least know what coins are available on the exchange), it is just a matter of not displaying them. Like I state in #38, coins with a value anywhere between 0 and minValueBtc display anyway despite this setting. The fix is deeper in the code but I just forked today and am not very much at home in a Node.js environment so I'm still figuring things out.

benmarten commented 6 years ago

Yes, both of you are correct. I think we should address this at a higher level. I suggest we create something like a Strategy class, that abstracts the different portfolio strategies... Examples could be manualStrategy, marketCapStrategy, equalStrategy. Based on that we can offer multiple configurations on targetAllocation, showing/hiding coins etc... I can prob. work on a proposal over next weekend... Happy to hear what you guys think ;)

gosuto-inzasheru commented 6 years ago

Yes this is a logical next step! And perfect solution to this issue. Right now the only strategy known is to allocate based on market cap. Of course it will show you a lot of empty coins because your allocation of 0 is not enough and you need to buy in order to reflect market cap.

How I solve this when manually setting allocation is by not adding coins to the portfolio that have an allocation of 0 (if (coin && (coin.getRelativeMarketCapRecommended() > 0)) {this.addCoin(coin)} in Portfolio.js, not commited yet).

Your strategy class sounds like a great formal way of dealing with this mess and I would love to see and work with your proposal.