Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hide zero amount for binance #35

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

barryvdh
Copy link
Collaborator

Re-do #32 against correct branch.

@benmarten
Copy link
Owner

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
Copy link
Collaborator Author

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
Copy link
Contributor

gosuto-inzasheru commented Jan 18, 2018

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
Copy link
Collaborator Author

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

@gosuto-inzasheru
Copy link
Contributor

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
Copy link
Owner

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 ;)

@benmarten benmarten force-pushed the develop branch 2 times, most recently from ef42ccf to d5ab3e7 Compare January 22, 2018 08:07
@gosuto-inzasheru
Copy link
Contributor

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.

gosuto-inzasheru added a commit to gosuto-inzasheru/CryptoETF that referenced this pull request Jan 28, 2018
Repository owner deleted a comment from Arpitkandwal Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants