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

Add ETF support for portfolio allocation command #2143

Merged
merged 53 commits into from
Sep 9, 2022
Merged

Conversation

montezdesousa
Copy link
Contributor

@montezdesousa montezdesousa commented Jul 23, 2022

Description

This PR adds ETF support to the alloc command in the portfolio menu. This means that users that do not provide any country, sector or region for their ETF trades will still be able to see their allocations.

The method is similar on what was used to fetch company data for stocks. In this case the allocation_model module will automatically search for the ETF sector/country/region weights and assign the proportional value to the portfolio allocations.

Additional changes:

  • Added ISINs as primary identifier for ETF/STOCKS data
  • Fix print_rich_table bug on decimal places for np.float64 format
  • Refactor assets allocation logic
  • Pre-load SPY as a default benchmark so that user is not blocked to use commands after loading, it still can be changed with bench. It's similar to what was done with risk free rate.
  • Fix bug when two trades of same stock occur in the same day
  • Reformat show output and allow export
  • Fix bug when alloc category empty
  • Fix bug when optional fields empty

Example:

Portfolio X holdings, %:
ETF ABC: 1000$, 50%
STOCK XYZ: 1000$, 50%

Suppose ETF ABC has 50% stocks in US and 50% in Brazil, so it will contribute 500$ to the US allocation and 500$ to Brazil.
Suppose STOCK XYZ is from US.

Final country allocs, %:
US: 1500$, 75%
Brazil: 500$, 25%

  • Summary of the change / bug fix.
  • Relevant motivation and context.

How has this been tested?

  • Please describe the tests that you ran to verify your changes.
  • Provide instructions so we can reproduce.
  • Please also list any relevant details for your test configuration.

Others

  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • My code passes all the checks pylint, flake8, black, ... To speed up development you should run pre-commit install.
  • New and existing unit tests pass locally with my changes. You can test this locally using pytest tests/....

@montezdesousa montezdesousa requested a review from JerBouma July 23, 2022 00:06
@montezdesousa montezdesousa added the enhancement Enhancement label Jul 23, 2022
@JerBouma
Copy link
Contributor

JerBouma commented Jul 23, 2022

I can not test this because it requires me to set 2FA (which I haven't enabled). Furthermore, the process of getting this code is far too difficult for the user (see: https://github.com/Chavithra/degiro-connector#36-how-to-find-your--totp_secret_key-)

The following needs to be done (can you look into this @Chavithra):

  • Set username and password via keys for DEGIRO
  • Set username, password and 2FA via keys for DEGIRO without needing to do anything with a QR code and similar. It shouldn't be complicated.
  • I shouldn't need to rely on and .env file or config_terminal.py file to make this work. That's not user friendly.

image

@Chavithra
Copy link
Contributor

Hi @JerBouma,

are you suggesting letting the user manually adding 2FA password each time they use the Terminal ?

Thanks

@JerBouma
Copy link
Contributor

JerBouma commented Aug 2, 2022

Hi @Chavithra,

That sounds good. You gotta try and make it accessible for people that just know how to navigate the terminal. We can not ask them to edit some file or extract some code from an URL. Therefore the ideal solution here would be:

  1. Let them set-up their DEGIRO account directly from keys (without the option to provide the 2FA here)
  2. When they wish to log-in into the DEGIRO menu we have inside the portfolio menu, if they have 2FA enabled they input the 2FA key in here. Otherwise, simply running login should suffice.
  3. They can now use the DEGIRO functionalities.

@montezdesousa
Copy link
Contributor Author

Hi @Chavithra,

That sounds good. You gotta try and make it accessible for people that just know how to navigate the terminal. We can not ask them to edit some file or extract some code from an URL. Therefore the ideal solution here would be:

  1. Let them set-up their DEGIRO account directly from keys (without the option to provide the 2FA here)
  2. When they wish to log-in into the DEGIRO menu we have inside the portfolio menu, if they have 2FA enabled they input the 2FA key in here. Otherwise, simply running login should suffice.
  3. They can now use the DEGIRO functionalities.

ok done with help from @Chavithra

@montezdesousa montezdesousa marked this pull request as ready for review August 2, 2022 14:34
@JerBouma
Copy link
Contributor

JerBouma commented Sep 9, 2022

There are still some things off. As you can see the return calculation we do is not the same as on DEGIRO. It is fine that it differs a little but this is too much. Furthermore, it is impossible that the benchmark has such poor return given that my current portfolio consists primarily of S&P500 and World (thus the benchmark is closely related). Could you look into this? @montezdesousa

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants