Skip to content

Different currency support#27

Draft
ktecho wants to merge 4 commits into
MarkT1065:mainfrom
ktecho:add_currency
Draft

Different currency support#27
ktecho wants to merge 4 commits into
MarkT1065:mainfrom
ktecho:add_currency

Conversation

@ktecho
Copy link
Copy Markdown
Contributor

@ktecho ktecho commented Jan 1, 2026

(This is not yet ready)

This adds currency support to Symbols (USD by default), and shows correct info and currency symbol for each position (stock, option, dividend, ...).

We still need to:

  • add a configuration screen to choose the default currency of the portfolio
  • add the logic to get the exchange rate for currencies different than the one of the portfolio
  • do the changes so totals are calculated using the corresponding rate

dividend REAL DEFAULT 0.0,
ex_dividend_date DATE,
pe_ratio REAL,
currency TEXT DEFAULT 'USD',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is the best place for currency. This is the Symbol table for a ticker like MSFT.

In the current schema, I think each individual thing being tracked (an option, long_position, option, or dividend can be in a specific currency. The default currency for the account can be in preferences.

This assumes 1 account can have trades in more than 1 currency.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This assumes 1 account can have trades in more than 1 currency

Yes, that was the idea. My account is in EUR. I trade stocks in EUR and USD.

I put the currency in the symbol table because once a symbol (MSFT) is in a currency, all you can do with it is in that currency. So if MSFT is in USD (it is), stocks, dividends and options are all expressed in USD.

On the other hand I also have VOW, which trades in EUR. All you can do with it is in that currency. Dividends, buy/sells and options are all expressed in EUR.

So I think it doesn't make sense to put the currency in each movement, if all the movements for that stock will be in the same currency, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@markturansky Did you have the time to give a thought to this?

I think it doesn't make sense to add the currency to all the tables, when you can only add it to the symbol table and have it applied to all the other entities automatically.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for the follow-up.

Yes, I think your argument makes sense. It's part of the metadata of the symbol. Another example might be Exchange.

@MarkT1065
Copy link
Copy Markdown
Owner

@ktecho would you mind rebasing? I merged a few fixes and improvements, such as improvements to make including make test

@MarkT1065
Copy link
Copy Markdown
Owner

this is Claude's review. It's correct about needing to address the schema change via the new migration system. It has other good suggestions, too. WDYT?

● PR #27 Analysis: Different Currency Support

  Status: OPEN (Draft - not yet ready per PR description)Author: Luis Miguel (@ktecho)Branch: add_currency → main

  Summary

  Adds multi-currency support to Wheeler, allowing tracking of stocks, options, dividends, and treasuries in different currencies. Currently supports 23 currencies with proper symbols and formatting.

  Key Changes

  1. Database Schema (internal/database/schema.sql, db.go)
  - Adds currency TEXT DEFAULT 'USD' column to symbols table
  - Migration logic in db.go to add column to existing databases

  2. New Currency Module (internal/models/currencies.go)
  - 23 supported currencies (USD, EUR, GBP, JPY, CHF, CAD, AUD, etc.)
  - Symbol positioning logic (before/after number: $100 vs 100€)
  - Formatting functions: FormatCurrency(), FormatCurrencyWithDecimals()

  3. Models Updated (symbol.go, option.go)
  - Symbol struct gains Currency field
  - Option struct gains Currency field
  - All queries updated to JOIN with symbols table to fetch currency
  - Update() methods accept currency parameter

  4. Web Layer (server.go, templates/)
  - Template functions: formatCurrencyFor, formatCurrencyWithDecimalsFor
  - New /setup route for portfolio base currency configuration
  - Updated all templates: options.html, dividends.html, symbol.html, etc.
  - JavaScript module currency.js for client-side formatting

  5. UI Changes
  - Symbol modal includes currency selector
  - All amounts display with correct currency symbol
  - New setup page for configuring portfolio base currency

  Issues & Concerns

  Critical

  1. Migration System Conflict: This PR adds ad-hoc migration logic in db.go:77-89 that checks for currency column existence. This conflicts with the new file-based migration system we just implemented. The currency column should be added via a proper migration file in
  internal/database/migrations/.
  2. Test Failures Expected: Adding Currency field to Symbol and Option structs will break existing tests that don't expect this field:
    - internal/models/index_test.go - Option comparisons
    - internal/models/metric_test.go - Symbol/option queries
    - Any other tests creating or comparing these structs
  3. Missing Foreign Key Validation: The symbols.currency column has no CHECK constraint to validate against the supported currency list. Invalid currency codes could be inserted.

  Design

  4. Inconsistent Currency Sourcing: Options get currency from JOINing with symbols table, but there's no handling if a symbol doesn't exist or has NULL currency. The COALESCE(s.currency, 'USD') helps but indicates a deeper design issue.
  5. No Exchange Rate Logic: PR description explicitly notes exchange rate conversion is NOT implemented yet. This means:
    - Multi-currency portfolios will show incorrect totals
    - Charts will add values from different currencies directly
    - P&L calculations will be wrong across currencies
  6. Template Function Complexity: The new template functions (formatCurrencyFor) do extensive type switching. This logic should be in Go code, not template helpers.

  Code Quality

  7. Unused Currency Colors: currencyColors mapping in currency.js:28-51 is defined but never used in the PR.
  8. Breaking API Change: The symbolService.Update() signature changed to require a currency parameter. This breaks internal/polygon/service.go:71 where it passes the existing currency, but any external callers would break.
  9. SQL Query Performance: All option queries now JOIN with symbols table. For large datasets, this adds overhead. Consider denormalizing currency onto options table.

  Recommendations

  Before Merging:

  1. Convert to file-based migration: Remove ad-hoc migration logic from db.go, create internal/database/migrations/20260101_add_currency_support.sql
  2. Add CHECK constraint: Validate currency codes against supported list in database schema
  3. Fix all tests: Update test expectations for new Currency field
  4. Run make test: Verify all tests pass before merge
  5. Add integration test: Test multi-currency symbols and ensure currency propagates correctly
  6. Document limitations: Clearly document that exchange rate conversion is NOT supported yet

  Future Work (per PR description):

  - Exchange rate API integration
  - Portfolio base currency configuration (partially done)
  - Currency conversion for totals/charts

  Test Plan Needed

  - Create symbol with non-USD currency
  - Verify currency displays correctly in all views
  - Verify options inherit currency from symbol
  - Test migration on existing database
  - Verify all unit tests pass (make test)
  - Test setup page currency configuration
  - Verify currency formatting (before/after symbol positioning)

  Overall Assessment: Good foundation for multi-currency support, but needs migration system alignment and comprehensive testing before merge. The draft status is appropriate.

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.

3 participants