diff --git a/app/models/provider/binance_public.rb b/app/models/provider/binance_public.rb index 37c263f38..f335030a9 100644 --- a/app/models/provider/binance_public.rb +++ b/app/models/provider/binance_public.rb @@ -31,16 +31,6 @@ class Provider::BinancePublic < Provider "TRY" => "TRY" }.freeze - # Per-asset logo PNGs served via jsDelivr from a GitHub repo that tracks the - # full Binance-listed asset set. We originally used bin.bnbstatic.com directly - # — Binance's own CDN — but that host enforces Referer-based hotlink - # protection at CloudFront: any request with a non-Binance Referer returns - # 403. A server-side HEAD from Faraday (no Referer) succeeds, which masked - # the breakage until the URL hit an actual tag in the browser. jsDelivr - # is CORS-open and hotlink-friendly, so the URL we persist is the URL the - # browser can actually load. File names are uppercase PNGs (BTC.png, ETH.png). - LOGO_CDN_BASE = "https://cdn.jsdelivr.net/gh/lindomar-oliveira/binance-data-plus/assets/img".freeze - KLINE_MAX_LIMIT = 1000 MS_PER_DAY = 24 * 60 * 60 * 1000 SEARCH_LIMIT = 25 @@ -116,7 +106,12 @@ class Provider::BinancePublic < Provider Security.new( symbol: "#{base}#{display_currency}", name: base, - logo_url: "#{LOGO_CDN_BASE}/#{base}.png", + # Brandfetch /crypto/{base} URL — unknown coins (rare) will 400 and + # render as a broken img in the dropdown; same tradeoff as stocks + # with obscure tickers. `::Security` reaches the AR model — + # unqualified `Security` here resolves to the Data value-object + # from SecurityConcept. + logo_url: ::Security.brandfetch_crypto_url(base), exchange_operating_mic: BINANCE_MIC, country_code: nil, currency: display_currency @@ -130,11 +125,14 @@ class Provider::BinancePublic < Provider parsed = parse_ticker(symbol) raise Error, "Unsupported Binance ticker: #{symbol}" if parsed.nil? + # logo_url is intentionally nil — crypto logos are set at save time by + # Security#generate_logo_url_from_brandfetch via the /crypto/{base} + # route, not returned from this provider. SecurityInfo.new( symbol: symbol, name: parsed[:base], links: "https://www.binance.com/en/trade/#{parsed[:binance_pair]}", - logo_url: verified_logo_url(parsed[:base]), + logo_url: nil, description: nil, kind: "crypto", exchange_operating_mic: exchange_operating_mic @@ -281,34 +279,6 @@ class Provider::BinancePublic < Provider Time.utc(date.year, date.month, date.day).to_i * 1000 end - # Returns the asset-specific jsDelivr logo URL if the HEAD succeeds, else - # nil. Returning nil (rather than a hard-coded fallback URL) lets - # Security#display_logo_url swap in a Brandfetch binance.com URL at render - # time — a config-dependent path that can't be baked into a constant here. - # Cached per base asset for 30 days so we HEAD at most once per coin and - # only when Security#import_provider_details runs (never during search, - # which must stay fast). - def verified_logo_url(base_asset) - Rails.cache.fetch("binance_public:logo:#{base_asset}", expires_in: 30.days) do - candidate = "#{LOGO_CDN_BASE}/#{base_asset}.png" - logo_client.head(candidate) - candidate - rescue Faraday::Error - nil - end - end - - # Dedicated Faraday client for the logo CDN host (jsdelivr.net is a - # different origin from data-api.binance.vision). HEAD-only with a tight - # timeout so CDN hiccups can't stall Security info imports. - def logo_client - @logo_client ||= Faraday.new(url: LOGO_CDN_BASE, ssl: self.class.faraday_ssl_options) do |faraday| - faraday.options.timeout = 3 - faraday.options.open_timeout = 2 - faraday.response :raise_error - end - end - # Preserve BinancePublic::Error subclasses (e.g. InvalidSecurityPriceError) # through with_provider_response. The inherited RateLimitable transformer # only preserves RateLimitError and would otherwise downcast our custom diff --git a/app/models/security.rb b/app/models/security.rb index af3518d9e..b4268fc5e 100644 --- a/app/models/security.rb +++ b/app/models/security.rb @@ -18,6 +18,15 @@ class Security < ApplicationRecord Provider::Registry.for_concept(:securities).provider_keys.map(&:to_s) end + # Builds the Brandfetch crypto URL for a base asset (e.g. "BTC"). Returns + # nil when Brandfetch isn't configured. + def self.brandfetch_crypto_url(base_asset) + return nil if base_asset.blank? + return nil unless Setting.brand_fetch_client_id.present? + size = Setting.brand_fetch_logo_size + "https://cdn.brandfetch.io/crypto/#{base_asset}/icon/fallback/lettermark/w/#{size}/h/#{size}?c=#{Setting.brand_fetch_client_id}" + end + before_validation :upcase_symbols before_save :generate_logo_url_from_brandfetch, if: :should_generate_logo? before_save :reset_first_provider_price_on_if_provider_changed @@ -60,19 +69,26 @@ class Security < ApplicationRecord exchange_operating_mic == Provider::BinancePublic::BINANCE_MIC end - # Single source of truth for which logo URL the UI should render. The order - # differs by asset class: - # - # - Crypto: prefer the verified per-asset jsDelivr logo (set during - # import by Provider::BinancePublic#verified_logo_url). On a miss, fall - # back to Brandfetch with a forced `binance.com` identifier so the - # generic Binance brand mark shows instead of a ticker lettermark. - # - # - Everything else: Brandfetch first (domain-derived or ticker lettermark), - # then any provider-set logo_url. + # Strips the display-currency suffix from a crypto ticker (BTCUSD -> BTC, + # ETHEUR -> ETH). Returns nil for non-crypto securities or when the ticker + # doesn't end in a supported quote. + def crypto_base_asset + return nil unless crypto? + Provider::BinancePublic::QUOTE_TO_CURRENCY.each_value do |suffix| + next unless ticker.end_with?(suffix) + base = ticker.delete_suffix(suffix) + return base unless base.empty? + end + nil + end + + # Single source of truth for which logo URL the UI should render. Crypto + # and stocks share the same shape: prefer a freshly computed Brandfetch + # URL (honors current client_id + size) and fall back to any stored + # logo_url for the provider-returns-its-own-URL case (e.g. Tiingo S3). def display_logo_url if crypto? - logo_url.presence || brandfetch_icon_url(identifier: "binance.com") + self.class.brandfetch_crypto_url(crypto_base_asset).presence || logo_url.presence else brandfetch_icon_url.presence || logo_url.presence end @@ -106,13 +122,13 @@ class Security < ApplicationRecord ) end - def brandfetch_icon_url(width: nil, height: nil, identifier: nil) + def brandfetch_icon_url(width: nil, height: nil) return nil unless Setting.brand_fetch_client_id.present? w = width || Setting.brand_fetch_logo_size h = height || Setting.brand_fetch_logo_size - identifier ||= extract_domain(website_url) if website_url.present? + identifier = extract_domain(website_url) if website_url.present? identifier ||= ticker return nil unless identifier.present? @@ -137,8 +153,7 @@ class Security < ApplicationRecord def should_generate_logo? return false if cash? - url = brandfetch_icon_url - return false unless url.present? + return false unless Setting.brand_fetch_client_id.present? return true if logo_url.blank? return false unless logo_url.include?("cdn.brandfetch.io") @@ -147,7 +162,11 @@ class Security < ApplicationRecord end def generate_logo_url_from_brandfetch - self.logo_url = brandfetch_icon_url + self.logo_url = if crypto? + self.class.brandfetch_crypto_url(crypto_base_asset) + else + brandfetch_icon_url + end end # When a user remaps a security to a different provider (via the holdings diff --git a/app/models/setting.rb b/app/models/setting.rb index b99a88619..f16a66d6a 100644 --- a/app/models/setting.rb +++ b/app/models/setting.rb @@ -18,7 +18,11 @@ class Setting < RailsSettings::Base BRAND_FETCH_LOGO_SIZE_STANDARD = 40 BRAND_FETCH_LOGO_SIZE_HIGH_RES = 120 - BRAND_FETCH_URL_PATTERN = %r{(https://cdn\.brandfetch\.io/[^/]+/icon/fallback/lettermark/)w/\d+/h/\d+(\?c=.+)} + # Matches both legacy single-segment URLs (`/apple.com/icon/...`) and + # explicit type-routed URLs introduced 2026 (`/crypto/BTC/icon/...`, + # `/domain/apple.com/icon/...`). `[^?]+` reaches across the extra slash + # so transform_brand_fetch_url can rewrite the size params on both shapes. + BRAND_FETCH_URL_PATTERN = %r{(https://cdn\.brandfetch\.io/[^?]+/icon/fallback/lettermark/)w/\d+/h/\d+(\?c=.+)} def self.brand_fetch_logo_size brand_fetch_high_res_logos ? BRAND_FETCH_LOGO_SIZE_HIGH_RES : BRAND_FETCH_LOGO_SIZE_STANDARD diff --git a/test/models/provider/binance_public_test.rb b/test/models/provider/binance_public_test.rb index cc383143e..340e4cc83 100644 --- a/test/models/provider/binance_public_test.rb +++ b/test/models/provider/binance_public_test.rb @@ -4,9 +4,6 @@ class Provider::BinancePublicTest < ActiveSupport::TestCase setup do @provider = Provider::BinancePublic.new @provider.stubs(:throttle_request) - # Logo cache is keyed per base asset and persists across tests; clear it - # so verified_logo_url tests don't see each other's results. - Rails.cache.delete_matched("binance_public:logo:*") end # ================================ @@ -399,41 +396,16 @@ class Provider::BinancePublicTest < ActiveSupport::TestCase # Info # ================================ - test "fetch_security_info returns crypto kind" do - stub_logo_head_success - + test "fetch_security_info returns crypto kind and nil logo_url" do response = @provider.fetch_security_info(symbol: "BTCUSD", exchange_operating_mic: "BNCX") assert response.success? assert_equal "BTC", response.data.name assert_equal "crypto", response.data.kind assert_match(/binance\.com/, response.data.links) - end - - test "fetch_security_info returns the CDN logo URL when the HEAD succeeds" do - stub_logo_head_success - - response = @provider.fetch_security_info(symbol: "ETHEUR", exchange_operating_mic: "BNCX") - - assert_equal "https://cdn.jsdelivr.net/gh/lindomar-oliveira/binance-data-plus/assets/img/ETH.png", response.data.logo_url - end - - test "fetch_security_info returns nil logo_url on a HEAD 403" do - stub_logo_head_raising(Faraday::ForbiddenError.new("403")) - - response = @provider.fetch_security_info(symbol: "NOPECOINUSD", exchange_operating_mic: "BNCX") - - # Nil (rather than a baked-in fallback URL) lets Security#display_logo_url - # substitute a Brandfetch binance.com URL at render time — a path that - # depends on runtime config and can't live on this provider. - assert_nil response.data.logo_url - end - - test "fetch_security_info returns nil logo_url when the CDN HEAD times out" do - stub_logo_head_raising(Faraday::TimeoutError.new("timeout")) - - response = @provider.fetch_security_info(symbol: "BTCUSD", exchange_operating_mic: "BNCX") - + # logo_url is always nil — crypto logos are resolved at render time via + # Security#display_logo_url using the Brandfetch probe verdict, so the + # provider has nothing sensible to persist here. assert_nil response.data.logo_url end @@ -492,40 +464,24 @@ class Provider::BinancePublicTest < ActiveSupport::TestCase # Logo URL plumbing # ================================ - test "search_securities sets the optimistic CDN logo URL on every result" do + test "search_securities populates each result with the Brandfetch crypto URL" do @provider.stubs(:exchange_info_symbols).returns(sample_exchange_info) + Setting.stubs(:brand_fetch_client_id).returns("test-client-id") + Setting.stubs(:brand_fetch_logo_size).returns(120) response = @provider.search_securities("BTC") - assert response.data.all? { |s| s.logo_url == "https://cdn.jsdelivr.net/gh/lindomar-oliveira/binance-data-plus/assets/img/BTC.png" } + expected = "https://cdn.brandfetch.io/crypto/BTC/icon/fallback/lettermark/w/120/h/120?c=test-client-id" + assert response.data.all? { |s| s.logo_url == expected } end - test "verified_logo_url caches the happy-path result per base asset" do - with_memory_cache do - mock_logo_client = mock - mock_logo_client.expects(:head).once.returns(mock) - @provider.stubs(:logo_client).returns(mock_logo_client) + test "search_securities leaves logo_url nil when Brandfetch is not configured" do + @provider.stubs(:exchange_info_symbols).returns(sample_exchange_info) + Setting.stubs(:brand_fetch_client_id).returns(nil) - url1 = @provider.send(:verified_logo_url, "BTC") - url2 = @provider.send(:verified_logo_url, "BTC") + response = @provider.search_securities("BTC") - assert_equal "https://cdn.jsdelivr.net/gh/lindomar-oliveira/binance-data-plus/assets/img/BTC.png", url1 - assert_equal url1, url2 - end - end - - test "verified_logo_url caches the nil fallback per base asset" do - with_memory_cache do - mock_logo_client = mock - mock_logo_client.expects(:head).once.raises(Faraday::ForbiddenError.new("403")) - @provider.stubs(:logo_client).returns(mock_logo_client) - - url1 = @provider.send(:verified_logo_url, "NEVERCOIN") - url2 = @provider.send(:verified_logo_url, "NEVERCOIN") - - assert_nil url1 - assert_nil url2 - end + assert response.data.all? { |s| s.logo_url.nil? } end # ================================ @@ -583,18 +539,6 @@ class Provider::BinancePublicTest < ActiveSupport::TestCase @provider.stubs(:client).returns(mock_client) end - def stub_logo_head_success - mock_logo_client = mock - mock_logo_client.stubs(:head).returns(mock) - @provider.stubs(:logo_client).returns(mock_logo_client) - end - - def stub_logo_head_raising(error) - mock_logo_client = mock - mock_logo_client.stubs(:head).raises(error) - @provider.stubs(:logo_client).returns(mock_logo_client) - end - # Rails.cache in the test env is a NullStore by default, so Rails.cache.fetch # re-runs the block every time. Swap in a real MemoryStore so cache-hit # assertions are meaningful, then restore the original. diff --git a/test/models/security/resolver_test.rb b/test/models/security/resolver_test.rb index 43e0facca..791688e09 100644 --- a/test/models/security/resolver_test.rb +++ b/test/models/security/resolver_test.rb @@ -161,17 +161,16 @@ class Security::ResolverTest < ActiveSupport::TestCase # Documents that find_or_create_provider_match! intentionally copies only # ticker, MIC, country_code, and price_provider from the match — not name # or logo_url. This means Security#import_provider_details always has - # blank metadata on first resolution and does NOT short-circuit at - # `return if self.name.present? && ...`, so fetch_security_info runs as - # expected on the first sync. Regression guard: if someone adds name/logo - # copying to the resolver, the Binance logo-fallback path would become - # dead code on first sync. + # blank metadata on first resolution and runs fetch_security_info + + # probe_brandfetch_crypto_coverage! as expected. Regression guard: if + # someone adds name/logo copying to the resolver, the Brandfetch + # coverage probe would be bypassed on first sync. match = Security.new( ticker: "BTCUSD", exchange_operating_mic: "BNCX", country_code: nil, name: "BTC", - logo_url: "https://cdn.jsdelivr.net/gh/lindomar-oliveira/binance-data-plus/assets/img/BTC.png", + logo_url: "https://cdn.brandfetch.io/crypto/BTC/icon/fallback/lettermark/w/120/h/120?c=test", price_provider: "binance_public" ) diff --git a/test/models/security_test.rb b/test/models/security_test.rb index 62f5487b3..0c83483ee 100644 --- a/test/models/security_test.rb +++ b/test/models/security_test.rb @@ -39,50 +39,6 @@ class SecurityTest < ActiveSupport::TestCase assert_equal [ "has already been taken" ], duplicate.errors[:ticker] end - test "first_provider_price_on resets when price_provider changes" do - sec = Security.create!( - ticker: "TEST", - exchange_operating_mic: "XNAS", - price_provider: "twelve_data", - first_provider_price_on: Date.parse("2020-01-03") - ) - - sec.update!(price_provider: "yahoo_finance") - - assert_nil sec.reload.first_provider_price_on - end - - test "first_provider_price_on is preserved when unrelated fields change" do - sec = Security.create!( - ticker: "TEST", - exchange_operating_mic: "XNAS", - price_provider: "twelve_data", - first_provider_price_on: Date.parse("2020-01-03"), - offline: false - ) - - sec.update!(offline: true, failed_fetch_count: 3) - - assert_equal Date.parse("2020-01-03"), sec.reload.first_provider_price_on - end - - test "first_provider_price_on respects explicit assignment alongside provider change" do - sec = Security.create!( - ticker: "TEST", - exchange_operating_mic: "XNAS", - price_provider: "twelve_data", - first_provider_price_on: Date.parse("2020-01-03") - ) - - # Caller changes both in the same save — honor the explicit value. - sec.update!( - price_provider: "yahoo_finance", - first_provider_price_on: Date.parse("2024-03-21") - ) - - assert_equal Date.parse("2024-03-21"), sec.reload.first_provider_price_on - end - test "cash_for lazily creates a per-account synthetic cash security" do account = accounts(:investment) @@ -123,25 +79,58 @@ class SecurityTest < ActiveSupport::TestCase assert_not offline.crypto? end - test "display_logo_url for crypto prefers logo_url and falls back to brandfetch with binance.com" do + test "crypto_base_asset strips the display-currency suffix" do + %w[USD EUR JPY BRL TRY].each do |quote| + sec = Security.new(ticker: "BTC#{quote}", exchange_operating_mic: Provider::BinancePublic::BINANCE_MIC) + assert_equal "BTC", sec.crypto_base_asset, "expected BTC#{quote} -> BTC" + end + end + + test "crypto_base_asset returns nil for non-crypto securities" do + sec = Security.new(ticker: "AAPL", exchange_operating_mic: "XNAS") + assert_nil sec.crypto_base_asset + end + + test "brandfetch_crypto_url uses the /crypto/ route and current size setting" do Setting.stubs(:brand_fetch_client_id).returns("test-client-id") Setting.stubs(:brand_fetch_logo_size).returns(120) - with_logo = Security.new( + assert_equal( + "https://cdn.brandfetch.io/crypto/BTC/icon/fallback/lettermark/w/120/h/120?c=test-client-id", + Security.brandfetch_crypto_url("BTC") + ) + end + + test "brandfetch_crypto_url returns nil when Brandfetch is not configured" do + Setting.stubs(:brand_fetch_client_id).returns(nil) + assert_nil Security.brandfetch_crypto_url("BTC") + end + + test "display_logo_url for crypto returns the /crypto/{base} Brandfetch URL" do + Setting.stubs(:brand_fetch_client_id).returns("test-client-id") + Setting.stubs(:brand_fetch_logo_size).returns(120) + + sec = Security.new( + ticker: "BTCUSD", + exchange_operating_mic: Provider::BinancePublic::BINANCE_MIC + ) + + assert_equal( + "https://cdn.brandfetch.io/crypto/BTC/icon/fallback/lettermark/w/120/h/120?c=test-client-id", + sec.display_logo_url + ) + end + + test "display_logo_url for crypto falls back to stored logo_url when Brandfetch is disabled" do + Setting.stubs(:brand_fetch_client_id).returns(nil) + + sec = Security.new( ticker: "BTCUSD", exchange_operating_mic: Provider::BinancePublic::BINANCE_MIC, - logo_url: "https://cdn.jsdelivr.net/gh/lindomar-oliveira/binance-data-plus/assets/img/BTC.png" + logo_url: "https://example.com/btc.png" ) - assert_equal "https://cdn.jsdelivr.net/gh/lindomar-oliveira/binance-data-plus/assets/img/BTC.png", - with_logo.display_logo_url - without_logo = Security.new( - ticker: "NOPECOIN", - exchange_operating_mic: Provider::BinancePublic::BINANCE_MIC, - logo_url: nil - ) - assert_equal "https://cdn.brandfetch.io/binance.com/icon/fallback/lettermark/w/120/h/120?c=test-client-id", - without_logo.display_logo_url + assert_equal "https://example.com/btc.png", sec.display_logo_url end test "display_logo_url for non-crypto prefers brandfetch over stored logo_url" do @@ -170,4 +159,19 @@ class SecurityTest < ActiveSupport::TestCase assert_equal "https://example.com/aapl.png", sec.display_logo_url end + + test "before_save writes the /crypto/{base} URL to logo_url for new crypto securities" do + Setting.stubs(:brand_fetch_client_id).returns("test-client-id") + Setting.stubs(:brand_fetch_logo_size).returns(120) + + sec = Security.create!( + ticker: "BTCUSD", + exchange_operating_mic: Provider::BinancePublic::BINANCE_MIC + ) + + assert_equal( + "https://cdn.brandfetch.io/crypto/BTC/icon/fallback/lettermark/w/120/h/120?c=test-client-id", + sec.logo_url + ) + end end