holding: Add multi-currency support for average costs calculations. #2153

2025-04-25T03:45:56+08:00

Joelute commented

2025-04-25 03:45:56 +08:00

(Migrated from github.com)

This PR adds multi-currency support when calculating for the cost basis

Before:	After:

Fixes: #2051.

This PR adds multi-currency support when calculating for the cost basis |Before: |After: | |---|---| |![image](https://github.com/user-attachments/assets/bdd075e0-f262-4847-9dcd-41f0e4eb3a97)|![image](https://github.com/user-attachments/assets/72cf62ff-528b-40cf-9b07-e49255265dbf) | ![image](https://github.com/user-attachments/assets/01c951d1-aa69-4ecd-a6be-8a63b243a972) Fixes: #2051.

zachgoll (Migrated from github.com) reviewed 2025-04-25 22:11:40 +08:00

zachgoll (Migrated from github.com) left a comment

Thanks for the changes here. Just a few points of feedback:

I would prefer to extend these calculations so that we have both a holding.avg_cost and holding.weighted_avg_cost. That way in the future we can provide a UI for the user to select which type of calculation they want.
Let's make sure and keep our naming super clear. The existing calculation is a "Simple Avg" while your new calculation is a "Weighted Avg". In tests and methods I think we should reflect this terminology
Since we're doing this calculation on read, it needs to be very quick (esp. for accounts with lots of holdings). I think at the very least, we need to get this output number in a single SQL statement. Ideally, I think this becomes very similar to our BalanceSheet and IncomeStatement classes, but instead, we'd call it Portfolio and do one big aggregate query that calculates all the cost basis across all holdings, then these model-specific methods simply "look up" the values needed from those aggregated results that are queried once, cached, and read many times by the various holdings.

Ultimately, I think moving to a weighted average introduces quite a bit of hidden complexity, so I think the highest impact thing we can do if we're looking for a quick improvement is integrating exchange_rates into the existing simple avg. query.

Thanks for the changes here. Just a few points of feedback: - I would prefer to _extend_ these calculations so that we have both a `holding.avg_cost` and `holding.weighted_avg_cost`. That way in the future we can provide a UI for the user to select which type of calculation they want. - Let's make sure and keep our naming super clear. The existing calculation is a "Simple Avg" while your new calculation is a "Weighted Avg". In tests and methods I think we should reflect this terminology - Since we're doing this calculation on _read_, it needs to be very quick (esp. for accounts with lots of holdings). I think at the very least, we need to get this output number in a single SQL statement. Ideally, I think this becomes very similar to our `BalanceSheet` and `IncomeStatement` classes, but instead, we'd call it `Portfolio` and do one big aggregate query that calculates all the cost basis across all holdings, then these model-specific methods simply "look up" the values needed from those aggregated results that are queried once, cached, and read many times by the various holdings. Ultimately, I think moving to a weighted average introduces quite a bit of hidden complexity, so I think the highest impact thing we can do if we're looking for a quick improvement is integrating `exchange_rates` into the existing simple avg. query.

Joelute commented

2025-04-26 12:24:07 +08:00

(Migrated from github.com)

@zachgoll Thanks for the feedback on this PR! For now, I've moved forward with just integrating exchange_rates into the existing avg_cost and remove the weighted average cost as I'm not sure where would be a good place for that sort of calculation. However I would be happy to introduce it to the code base later on :)

zachgoll (Migrated from github.com) reviewed 2025-05-02 19:46:58 +08:00

zachgoll (Migrated from github.com) left a comment

Overall changes look good now thanks! Just left one improvement we can make to the query.

app/models/holding.rb

						
				@@ -34,3 +41,4 @@

				      .average("trades.price * COALESCE(exchange_rates.rate, 1)")

				    Money.new(avg_cost || price, currency)

				  end

zachgoll (Migrated from github.com) commented

2025-05-02 19:46:39 +08:00

For clarity of the ownership here, can we use some of the existing query helpers (see entryable)?

avg_cost = account.trades.with_entry.joins( ...rest of query)

Also, check entry.date <= ? I think that should be entries.date

For clarity of the _ownership_ here, can we use some of the existing query helpers (see [entryable](https://github.com/maybe-finance/maybe/blob/main/app/models/entryable.rb))? ```rb avg_cost = account.trades.with_entry.joins( ...rest of query) ``` Also, check `entry.date <= ?` I _think_ that should be `entries.date`

Joelute (Migrated from github.com) reviewed 2025-05-05 09:02:32 +08:00

app/models/holding.rb

						
				@@ -34,3 +41,4 @@

				      .average("trades.price * COALESCE(exchange_rates.rate, 1)")

				    Money.new(avg_cost || price, currency)

				  end

Joelute (Migrated from github.com) commented

2025-05-05 09:02:32 +08:00

Updated to use with_entry and entries in the latest commit.

I think entry was ok as it was just aliased to entry instead of entries. But I've updated it to use entries instead.

Updated to use `with_entry` and `entries` in the latest commit. I think entry was ok as it was just aliased to `entry` instead of `entries`. But I've updated it to use `entries` instead.