Add security prices provider (Synth integration) #1039

2024-08-01T08:24:19+08:00

zachgoll commented

2024-08-01 08:24:19 +08:00

(Migrated from github.com)

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

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 (Migrated from github.com) reviewed 2024-08-02 03:13:01 +08:00

test/test_helper.rb

						
				@@ -35,3 +33,3 @@

				    # Run tests in parallel with specified workers

				    parallelize(workers: :number_of_processors) unless ENV["DISABLE_PARALLELIZATION"]

				    parallelize(workers: :number_of_processors) unless ENV["DISABLE_PARALLELIZATION"] == "true"

zachgoll (Migrated from github.com) commented

2024-08-02 03:13:00 +08:00

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.

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](https://github.com/user-attachments/assets/647a0590-df47-479c-810d-c8821eb25d65)

zachgoll (Migrated from github.com) reviewed 2024-08-02 03:21:47 +08:00

db/migrate/20240731191344_change_primary_identifier_for_security.rb

						
				@@ -0,0 +4,4 @@

				    remove_column :securities, :isin, :string

				    rename_column :security_prices, :isin, :ticker

				  end

				end

zachgoll (Migrated from github.com) commented

2024-08-02 03:21:47 +08:00

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.

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.

josefarias (Migrated from github.com) reviewed 2024-08-10 10:43:56 +08:00

test/interfaces/exchange_rate_provider_interface_test.rb

josefarias (Migrated from github.com) commented

2024-08-10 10:43:51 +08:00

@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.

@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.

zachgoll (Migrated from github.com) reviewed 2024-08-12 20:35:25 +08:00

test/interfaces/exchange_rate_provider_interface_test.rb

zachgoll (Migrated from github.com) commented

2024-08-12 20:35:25 +08:00

@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

@josefarias ahh, good callout! Was this the direction you were thinking? ```rb def accounting_for_http_calls cassettes = [ { name: 'synth/exchange_rate' }, { name: 'other/exchange_rate' } ] VCR.use_cassettes(cassettes) do yield end end ```

josefarias (Migrated from github.com) reviewed 2024-08-12 21:47:25 +08:00

test/interfaces/exchange_rate_provider_interface_test.rb

josefarias (Migrated from github.com) commented

2024-08-12 21:47:25 +08:00

Yes! Something like that

Schyzophrenic (Migrated from github.com) reviewed 2024-11-02 18:42:48 +08:00

db/migrate/20240731191344_change_primary_identifier_for_security.rb

						
				@@ -0,0 +4,4 @@

				    remove_column :securities, :isin, :string

				    rename_column :security_prices, :isin, :ticker

				  end

				end

Schyzophrenic (Migrated from github.com) commented

2024-11-02 18:42:48 +08:00

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?

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?

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gavin/maybe#1039