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 security prices provider (Synth integration) #1039

Conversation

zachgoll
Copy link
Collaborator

@zachgoll zachgoll commented Aug 1, 2024

A basic implementation of security price fetching from our "default" data provider, Synth.

A lot of the code here is intentionally left with some duplication as we implement more and more Synth endpoints and incorporate more providers. Eventually we'll need to consolidate and refactor provider logic.

Major changes here include:

  • Move to using ticker rather than isin to identify a security internally
  • Add fetch_security_prices method to Synth with pagination support for fetching multiple prices at once (1 Synth credit used per "group" rather than per securities price)
  • Incorporate live prices into Holdings sync algorithm

@zachgoll zachgoll linked an issue Aug 1, 2024 that may be closed by this pull request
@zachgoll zachgoll marked this pull request as ready for review August 1, 2024 19:07
@@ -16,7 +15,6 @@
# https://github.com/ged/ruby-pg/issues/538#issuecomment-1591629049
ENV["PGGSSENCMODE"] = "disable"

require_relative "../config/environment"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, I had removed dotenv from the test environment entirely to avoid "mystery guest" values and keep test configurations transparent for new contributors. Now that we're starting to introduce more data providers which require live api keys to generate VCR cassettes, I think the .env.test strategy (as outlined in the dotenv-rails docs below) is a good compromise.

Everything after the require_relative "../config/environment" is the "default" value, while the with_env_override helper method can override these defaults on a per-test basis.

CleanShot 2024-08-01 at 15 09 09@2x

remove_column :securities, :isin, :string
rename_column :security_prices, :isin, :ticker
end
end
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is largely a simplification of how we identify securities internally. My original thinking was that isin codes would provide globally unique identification for securities and help us avoid duplicates, but after doing some research on various data providers, this is not viable.

For example, Plaid provides an isin code for each security, while Polygon (a popular stock price provider) doesn't provide an isin code at all. Instead, it provides a FIGI code, which is the new globally unique identifier standard for all types of financial instruments that is not subject to change when a company reclassifies, splits, moves exchanges, etc. (isin can change in rare cases).

While a "ticker" symbol isn't globally unique, it fits our domain model well. Each ticker represents a company + exchange pair, which results in a unique price.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main issue is that tickers are for specific companies. All ETF and active funds will have an ISIN instead.
Tickers are useful but ISIN are the standard for looking up values of an investment and are widely available on the Key Investor Information or popular web sites such as morningstar.
Without the ISIN, how will you add investments which are not companies?

@zachgoll zachgoll merged commit 453a54e into main Aug 1, 2024
4 checks passed
@zachgoll zachgoll deleted the 1036-incorporate-daily-security-prices-from-synth-into-new-holdings-view branch August 1, 2024 23:43

assert_respond_to response, :rate
assert_respond_to response, :success?
assert_respond_to response, :error
assert_respond_to response, :raw_response
end
end

private
def accounting_for_http_calls
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zachgoll great work here! In case you're wondering (you may have already figured this out) this was put in place to have a single point of extension to stub responses for all providers, since these tests will eventually be shared across them.

All good removing, and all good to never bring it back. Just realized it may not be obvious so thought I'd point it out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josefarias ahh, good callout! Was this the direction you were thinking?

def accounting_for_http_calls
  cassettes = [
   { name: 'synth/exchange_rate' },
   { name: 'other/exchange_rate' }
  ]
  
  VCR.use_cassettes(cassettes) do
    yield
  end
end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! Something like that

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.

Incorporate daily security prices from Synth into new holdings view
3 participants