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

Broader support for online update of securities #1908

Conversation

MrMarvin
Copy link
Contributor

@MrMarvin MrMarvin commented Dec 24, 2020

Broader support for online update of securities

Some types of investment vehicles receive updates to their base information and attributes. Especially funds tend to get re balanced on their constituents (often), change their TER according to market shifts (occasionally) or even switch the index they tracks (rarely).
Currently there is no extendable way to automate the process of updating those information on a security in Portfolio-Performance.

Use of etf-data.com

While I personally would prefer to get those pieces of information from the funds respective vendors directly, this has been proven difficult. A lack of consistent public APIs from their side forces us to rely on third party aggregation services. Those vary in accuracy and consistency unfortunately.
The suggested https://etf-data.com/ is such an independent API provider. It offers both free (without any additional authentication or registration) and paid access. I've personally used their service over the last few days without issues on the free plan. Hoping this will stay available for the foreseeable future 🤞. One downside is that it currently focuses on ETFs only.

Features

  1. Search and import from etf-data.com
  2. Showing the SearchProvider's name on the search results table
  3. Import and register additional attributes
  4. Link existing Securities to ETF-Data.com, much like it currently works with Portfolio-Report.net
  5. (Automatic) Updates of Security attributes

image

Design decisions:

  1. Focus on extensibility, always have another potential data provider / API in mind.
  2. Any Security can have one of many online updates sources. Currently we only support Portfolio-Report.net.
    2.1. The existing SecuritySearchProvider should be in the call chain to do update as well, introducing this interface to also abstract away for updates in SyncOnlineSecuritiesJob.
    2.2. The onlineId field should be used to indicate which online updater to use, if any.
  3. Initial (search) and recurring (update) online searches can set or change core information on a security (such as ISIN or name), as well as set or update its attributes.
    2.1 If an attribute that is received via online sources is not currently configured in the clientSettings list of Security attributes, it will be added and therefore appear in the security's edit view / wizard. A user may change the display naming of the attribute at any point.

Future work

Importing/update Taxonomies such as Region and Sector distribution, potentially guarded by if the user has Regions (MSCI) and/or Industries (GICS) enabled

@MrMarvin MrMarvin changed the title Add etf-data.com search provider Broader support for online update of securities Dec 25, 2020
@buchen
Copy link
Member

buchen commented Dec 27, 2020

Hi @MrMarvin,

thanks for taking the time for your contribution. 😄

I check the terms and conditions of etf-data.com. The terms are actually about the subscription case. If the users subscribes, then all looks okay. The "The client shall not use, store, or access any ZuriAnalytics’ Data & Services, ETF Data API or Vattly after the termination of this Agreement" is something that the user could implement by deleting the securities and/or attributes. There is almost nothing writing about the free tier. It says "Nevertheless, it should always be enough for someone just exploring the API and doing small individual research." where one could see PP as small individual research. Anyway, as you added the name "PortfolioPerformance" to the header, it would be fairly easy to track usage back to this project.

Do you happen to have contact with those folks?

The code itself looks okay. I cannot test it at the moment because the SSL certificate of eft-data has expired. That is a change I can fairly quickly merge.

About the change - maybe we should not hit the eft-data server if we know the search term is not a ISIN. Just to keep the free limit safe. We could use a simply regex to see if it is an ISIN.

If an attribute that is received via online sources is not currently configured in the clientSettings list of Security attributes, it will be added and therefore appear in the security's edit view / wizard. A user may change the display naming of the attribute at any point.

I am not sure that is the case at the moment. The attributes that search provider currently sets are simply lost. Unfortunately, the design of the attributes is not really intuitive nor is it good (but hard to change because it is out).

Today, it works like this:

  • In the global settings, the user can create attributes with a pre-configured setup
  • This creates AttributeTypes which are stored in the ClientSettings class
  • The AttributeType as an id (String) which then is the key into the map stored with each security, account, or portfolio
  • Unless it is one of the default attributes, the id is a generated UUID
  • Because the user can delete any existing attributes, we cannot make any assumption as to which attributes exist

In short, there is not yet a solution to dynamically add new attributes which have a particular meaning.
Somehow we would need to extend to have a "external mapping" field which tells the update to fill in the values into this attribute.

Then the next point: I like the regions and sector information for each product. For this, I would like to see a Taxonomy maintained. Then all ETFs end up in one taxonomy and can be compared evaluated accordingly. But also for this, I do not yet have any logic available. Again, the update would need to know which taxonomy to use sectors and regions.

etf-data just needs the ISIN, nothing special such as an onlineId. Do you think the user should "mark" the securities somehow for which PP then retrieves additional attributes?

What do you think about syncing into the taxonomy?

@MrMarvin
Copy link
Contributor Author

MrMarvin commented Dec 28, 2020

Hi @buchen, thanks a lot for your comments!

Do you happen to have contact with those folks?

Not yet. If this PR's direction is something we agree on pursuing, I can definitely reach out to them and clarify our usage patterns. I agree that PP should fall into "small individual research", but let's see.

I cannot test it at the moment because the SSL certificate of eft-data has expired.

Yeah, that's unfortunate. Almost ironic that after finding this service, implementing a client against their API, opening a PR stating "[third party providers] vary in accuracy and consistency unfortunately" and the next morning their service breaks with an expired certificate 😅

maybe we should not hit the eft-data server if we know the search term is not a ISIN

Yep, good point. I'll add a check before calling out to the API in the SearchProvider.

I am not sure that is the case at the moment. The attributes that search provider currently sets are simply lost.

To my understanding those attributes are not in fact lost, they continue to exist, but are just never displayed. All UI components do look up from the ClientSettings list and map to values on the security model, so they cant "know" that there are more.
My current WIP code (note: I'm not really happy with where it currently resides) adds some small-ish UI logic that forcefully adds "unknown" attributes that exists on a security to the list in ClientSettings, whenever a security edit page is opened. So this basically triggers during the import flow right after the new security is created. It adds the newly created attributes with default, non-localized names. Maybe non-UI code should have a way to access ClientSettings globally, without needing a Client reference from within the UI first.

etf-data just needs the ISIN, nothing special such as an onlineId. Do you think the user should "mark" the securities somehow for which PP then retrieves additional attributes?

I'd tried to avoid changing the model itself so far. Looking into tweaking the onlineId field to also contain a reference to what SearchProvider to use for updates. Currently it's assumed there is only Portfolio-Report.net.

What do you think about syncing into the taxonomy?

This was my initial reason to start working on security searching / importing. I believe this becomes much easier to get by once we settled on a mechanism to import attributes. It's basically the same thing, needs the same understanding of what exists on the ClientSettings etc.
I'd like to tackle the 'connect an existing Security for online updates' use case first.

How do you feel about wrapping up this PRs with the basic flow with security info and attributes and then add the Taxonomy import/update in a secondary PR on top?

@MrMarvin
Copy link
Contributor Author

Regarding "some small-ish UI logic that forcefully adds "unknown" attributes that exists on a security to the list in ClientSettings":

Please see the commit I just pushed. It's a first idea how to do that, currently only supporting the initial 'create Security after search' case, not updating or connecting an existing Secuity with an online update source.
There are two main changes:

  1. Changing Attributes to use the AttributeType objects as keys directly, instead of their (String) id. This allows retrieval of the type of an attribute without looking it up on the ClientSettings.
  2. Adding 3e1c243#diff-0e015e286363fcfe2299c4798452393821eb5227852281493c7a1bf83046070eR44-R45 that adds those new AttributeTypes to ClientSettings

@MrMarvin
Copy link
Contributor Author

MrMarvin commented Dec 28, 2020

Wanted to give an update on the state of the UX considerations for this change. Some screenshots:

Before importing

image

image

After import

image

image

image

image

image

@MrMarvin MrMarvin force-pushed the add_etf_data_com_search_provider branch from e29366e to 7389253 Compare December 29, 2020 15:34
@MrMarvin
Copy link
Contributor Author

Okay, I took another swing at linking existing Securities. User facing changes so far:

  1. Renamed the context menu action to a more generic Link to ... (or the respective translations)
  2. Added a new dialog to select which provider to link to.
  3. Provider specific dialog will now pop up on a per-provider basis. E.g. the existing URL based dialog for Portfolio-Report.net will show when selecting it's provider in the select list.

image

image

image

This all with the caveat that the 'Link to' mechanism currently has no way to add the new AttributeTypes to the ClientSettings list, as the initial import via search does. A workaround is to search and import at least one other Security from ETF-Data.com. This will populate the AttributeTypes. All other (linked) Securities will then show their attribute just fine.

@MrMarvin MrMarvin force-pushed the add_etf_data_com_search_provider branch from 8815abf to 7d2b65d Compare December 29, 2020 22:29
@MrMarvin MrMarvin marked this pull request as ready for review December 29, 2020 23:03
@buchen
Copy link
Member

buchen commented Jan 5, 2021

Hi @MrMarvin,

as I said, I like the idea of getting more attributes from such sites as etf-data.

Looking at your proposal, the first thing I am thinking about whether it makes sense to use the "link to provider" approach. For example, it should be possible to retrieve historical prices from portfolio report but at the same time retrieve attributes from etf-data (which has no historical prices to my understanding). Plus: the unique identifier on etf-data is the ISIN (which is great) so we could sync the attributes on all securities that happen to have a ISIN.

My thinking right now is:

  • Adding a item to the "Online" menu called "Sync with etf-data" which
    • creates the missing attributes (if they do not yet exist)
    • attempts to sync all securities that have an ISIN
    • for now, this requires to manually trigger the Online menu (w/ 40K users I am careful to roll out automatic updates right away)
  • The search works as you designed
    • Download and create the attributes if necessary
    • Does not link to etf-data as that is not really needed

If you don't mind, I will cherry pick your changes and re-assemble them along these lines.

I have a couple more comments on the code:

  • Changing the format of the online id should include the migration of existing files (upon opening). That is easier than keeping if/else statements in the code (see Security)
  • Why not have two properties: provider & online id? Might be easier than always having to split / join the fields
  • AttributeType equals/hashCode only on the id?
  • The changes to "Attribute" do not work this way because in the XML serialization it will throw up - also, when opening old files, the user looses all data. I have the simply String -> Object map by intention so that the XML straight forward
  • It is okay the way the attribute types are pushed into the client by SearchSecurityWizard

buchen added a commit that referenced this pull request Jan 5, 2021
- search etf-data if search string is an ISIN
- add new attributes if user chooses security
- added new attribute type filed 'source' to identify mapped fields
- update default fields

Co-authored-by: Marvin Frick <marvin.frick@shopify.com>
Co-authored-by: Andreas Buchen <andreas.buchen@gmail.com>
Issue: #1908
@buchen
Copy link
Member

buchen commented Jan 5, 2021

I have no cherry-picked your changes and extracted the "search" functionality as a first starting point. The branch is here:
https://github.com/buchen/portfolio/tree/feature_etf_data_com

The major change is that I determine the attribute based on a new "source" attribute. This should allow to attach it also to existing attributes where I cannot change the Id.

Next, I will take your sync functionality and wrap it into a menu item.

It looks already nice. Thanks for your contribution. I added you to the about text.

buchen added a commit that referenced this pull request Jan 5, 2021
Co-authored-by: Marvin Frick <marvin.frick@shopify.com>
Co-authored-by: Andreas Buchen <andreas.buchen@gmail.com>
Issue: #1908
@buchen
Copy link
Member

buchen commented Jan 5, 2021

It is possible to create the taxonomies, but I quickly exceeded my free quota...

This is the result using the "kommer.xml" sample file.

Bildschirmfoto 2021-01-05 um 21 06 49

buchen added a commit that referenced this pull request Jan 6, 2021
buchen added a commit that referenced this pull request Jan 6, 2021
@buchen
Copy link
Member

buchen commented Jan 6, 2021

Merged into master

@buchen buchen closed this Jan 6, 2021
@MrMarvin
Copy link
Contributor Author

Thank you @buchen! The cherry-picked and enhanced solution you chose is great.

I agree that the whole linking a Security to an online service i.e. portfolio-report.net seemed odd to me. I think generally I'm in in favour of a more neutral stance on online services, like we do with the multiple Quote providers. Not having a single one "own" certain or all securities. Please not that I'm missing a lot of context on the decisions made around that.

cmaoling pushed a commit to cmaoling/portfolio that referenced this pull request Jan 24, 2021
- search etf-data if search string is an ISIN
- add new attributes if user chooses security
- added new attribute type filed 'source' to identify mapped fields
- update default fields

Co-authored-by: Marvin Frick <marvin.frick@shopify.com>
Co-authored-by: Andreas Buchen <andreas.buchen@gmail.com>
Issue: portfolio-performance#1908
cmaoling pushed a commit to cmaoling/portfolio that referenced this pull request Jan 24, 2021
Co-authored-by: Marvin Frick <marvin.frick@shopify.com>
Co-authored-by: Andreas Buchen <andreas.buchen@gmail.com>
Issue: portfolio-performance#1908
cmaoling pushed a commit to cmaoling/portfolio that referenced this pull request Jan 24, 2021
cmaoling pushed a commit to cmaoling/portfolio that referenced this pull request Jan 24, 2021
Lennix pushed a commit to Lennix/portfolio that referenced this pull request Feb 22, 2021
- search etf-data if search string is an ISIN
- add new attributes if user chooses security
- added new attribute type filed 'source' to identify mapped fields
- update default fields

Co-authored-by: Marvin Frick <marvin.frick@shopify.com>
Co-authored-by: Andreas Buchen <andreas.buchen@gmail.com>
Issue: portfolio-performance#1908
Lennix pushed a commit to Lennix/portfolio that referenced this pull request Feb 22, 2021
Co-authored-by: Marvin Frick <marvin.frick@shopify.com>
Co-authored-by: Andreas Buchen <andreas.buchen@gmail.com>
Issue: portfolio-performance#1908
Lennix pushed a commit to Lennix/portfolio that referenced this pull request Feb 22, 2021
Lennix pushed a commit to Lennix/portfolio that referenced this pull request Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants