gavin/maybe
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 28 Wiki Activity

perf(sync): Preload and cache exchange rate data when syncing. #2404

Closed
Joelute wants to merge 1 commits from preload-exchange-rate into main
pull from: preload-exchange-rate
merge into: gavin:main
Conversation 9 Commits 1 Files Changed 6 +226 -44
Joelute commented 2025-06-23 10:04:32 +08:00 (Migrated from github.com)
Copy Link

When we are performing transaction syncs, we are looking up for the exchange rate of the transaction if the currency of the transaction is different from the account currency. This look up is performed on every transactions that is being synced, which results in a N+1 query.

This change loads and caches the exchange rates when performing syncs.

When we are performing transaction syncs, we are looking up for the exchange rate of the transaction if the currency of the transaction is different from the account currency. This look up is performed on every transactions that is being synced, which results in a `N+1` query. This change loads and caches the exchange rates when performing syncs.
zachgoll (Migrated from github.com) requested changes 2025-06-23 22:37:51 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

I agree with your diagnosis here and like the overall direction you're taking this with the exchange_rates method in the sync_cache.

That said, I don't think cached_exchange_to belongs in the Money class. That class should not be aware of caching, and if we need these bulk loaded rates, I'd rather just encapsulate the "find rate from cache" logic directly in our sync_cache.rb class.

Also, I'm thinking we should remove QueryAssertions as this adds complexity and makes our tests brittle by asserting things that aren't directly relevant to the subject under test. Rails already ships with a helper for this, but I still think we ought to be very sparse with our usage of it and let our APM tools notify us of these performance issues.

I agree with your diagnosis here and like the overall direction you're taking this with the `exchange_rates` method in the `sync_cache`. That said, I don't think `cached_exchange_to` belongs in the `Money` class. That class should not be aware of caching, and if we need these bulk loaded rates, I'd rather just encapsulate the "find rate from cache" logic directly in our `sync_cache.rb` class. Also, I'm thinking we should remove `QueryAssertions` as this adds complexity and makes our tests brittle by asserting things that aren't directly relevant to the subject under test. Rails already ships with [a helper for this](https://api.rubyonrails.org/v7.2.1/classes/ActiveRecord/Assertions/QueryAssertions.html#method-i-assert_queries_count), but I still think we ought to be very sparse with our usage of it and let our APM tools notify us of these performance issues.
Joelute commented 2025-06-26 04:36:09 +08:00 (Migrated from github.com)
Copy Link

Hi Zach, I've updated the PR to:

  • Rename cached_exchange_to to find_rate_by_cache
  • Moved the find_rate_by_cache logic over to sync_cache.rb
  • Removed my custom test case helper in favour for the built in helper that Rails has

Please do take a look and give it another review!

Hi Zach, I've updated the PR to: - Rename `cached_exchange_to` to `find_rate_by_cache` - Moved the `find_rate_by_cache` logic over to `sync_cache.rb` - Removed my custom test case helper in favour for the built in helper that Rails has Please do take a look and give it another review!
zachgoll (Migrated from github.com) reviewed 2025-06-26 04:39:59 +08:00
app/models/balance/sync_cache.rb
@@ -16,2 +16,4 @@
end
def find_rate_by_cache(amount_money, to_currency, date: Date.current, fallback_rate: 1)
raise TypeError unless amount_money.respond_to?(:amount) && amount_money.respond_to?(:currency)
zachgoll (Migrated from github.com) commented 2025-06-26 04:38:55 +08:00
Copy Link

I think this method needs another look. A few issues I'm seeing at first glance:

  • Errors not handled
  • Inconsistent return values
  • Over-fetching rates (we don't need to find_or_fetch_rate if we're preloading to a cache already)
I think this method needs another look. A few issues I'm seeing at first glance: - Errors not handled - Inconsistent return values - Over-fetching rates (we don't need to `find_or_fetch_rate` if we're preloading to a cache already)
test/models/balance/forward_calculator_test.rb
zachgoll (Migrated from github.com) commented 2025-06-26 04:39:56 +08:00
Copy Link

We probably should not be using this assert_queries_count at such a broad level. This test is brittle because if we make any change to ForwardCalculator (highly possible in the future), the number of queries may change and we'd have to come back and update all these tests.

We probably should not be using this `assert_queries_count` at such a broad level. This test is brittle because if we make any change to `ForwardCalculator` (highly possible in the future), the number of queries may change and we'd have to come back and update all these tests.
Joelute (Migrated from github.com) reviewed 2025-06-27 11:10:11 +08:00
app/models/balance/sync_cache.rb
@@ -16,2 +16,4 @@
end
def find_rate_by_cache(amount_money, to_currency, date: Date.current, fallback_rate: 1)
raise TypeError unless amount_money.respond_to?(:amount) && amount_money.respond_to?(:currency)
Joelute (Migrated from github.com) commented 2025-06-27 08:05:32 +08:00
Copy Link

Hmm, I'm confused on the "inconsistent return values part". entry is passed as a Money object and it is returned if it doesn't need any currency conversions. Then we are creating a new Money object from the currency conversion. In both cases we are returning a consistent return type of Money.

I would love to know your opinion on this.

Hmm, I'm confused on the "inconsistent return values part". `entry` is passed as a Money object and it is returned if it doesn't need any currency conversions. Then we are creating a new `Money` object from the currency conversion. In both cases we are returning a consistent return type of `Money`. I would love to know your opinion on this.
test/models/balance/forward_calculator_test.rb
Joelute (Migrated from github.com) commented 2025-06-27 08:12:55 +08:00
Copy Link

I agree that using assert_queries_count at such a broad level is quite brittle and it is subjected to changes, but I argue that it would help in putting an emphasis in considering performance when we are making changes to ForwardCalculator. Avoiding the need to add unnecessary queries to the database.

I'll remove the use of using assert_queries_count at a broad level for now.

I agree that using `assert_queries_count` at such a broad level is quite brittle and it is subjected to changes, but I argue that it would help in putting an emphasis in considering performance when we are making changes to `ForwardCalculator`. Avoiding the need to add unnecessary queries to the database. I'll remove the use of using `assert_queries_count` at a broad level for now.
zachgoll (Migrated from github.com) reviewed 2025-06-27 23:01:20 +08:00
app/models/balance/sync_cache.rb
@@ -16,2 +16,4 @@
end
def find_rate_by_cache(amount_money, to_currency, date: Date.current, fallback_rate: 1)
raise TypeError unless amount_money.respond_to?(:amount) && amount_money.respond_to?(:currency)
zachgoll (Migrated from github.com) commented 2025-06-27 22:58:19 +08:00
Copy Link

Ahh, I see. I was thrown off by the naming of entry. In the app, Entry is a first class concept, so we should not be naming this param entry unless it truly represents an Entry object. I'd probably just call this param money or amount_money to be clear.

Ahh, I see. I was thrown off by the naming of `entry`. In the app, `Entry` is a first class concept, so we should not be naming this param `entry` unless it truly represents an `Entry` object. I'd probably just call this param `money` or `amount_money` to be clear.
test/models/balance/forward_calculator_test.rb
zachgoll (Migrated from github.com) commented 2025-06-27 23:01:14 +08:00
Copy Link

I agree that thinking about performance is important, but there are a lot of reasons why we might need to add another query / remove a query from the calculator, and we don't want to have to constantly update our test assertions every time we make a change.

The test suite should be testing the outcome rather than the implementation. If we were testing a Query Object, I could definitely see the use case for assert_queries_count. This is just too high of a level to be using it I think.

I agree that thinking about performance is important, but there are a lot of reasons why we might need to add another query / remove a query from the calculator, and we don't want to have to constantly update our test assertions every time we make a change. The test suite should be testing the _outcome_ rather than the _implementation_. If we were testing a Query Object, I could definitely see the use case for `assert_queries_count`. This is just too high of a level to be using it I think.
Joelute (Migrated from github.com) reviewed 2025-07-05 11:59:02 +08:00
test/models/balance/forward_calculator_test.rb
Joelute (Migrated from github.com) commented 2025-07-05 11:59:02 +08:00
Copy Link

I've just pushed a couple testcases aimed at testing find_rates_by_cache and performing more query assertions there instead.

I've just pushed a couple testcases aimed at testing `find_rates_by_cache` and performing more query assertions there instead.
Joelute (Migrated from github.com) reviewed 2025-07-05 12:04:29 +08:00
app/models/balance/sync_cache.rb
@@ -16,2 +16,4 @@
end
def find_rate_by_cache(amount_money, to_currency, date: Date.current, fallback_rate: 1)
raise TypeError unless amount_money.respond_to?(:amount) && amount_money.respond_to?(:currency)
Joelute (Migrated from github.com) commented 2025-07-05 12:04:29 +08:00
Copy Link

I've updated the param names from:

  • entry -> money
  • other_currency -> to_currency
    Hopefully this reduces the confusions around the names and makes the code more readable.

In the event that we don't have the exchange rates in our cache, we want to be able to fetch for that exchange rate and load it for use. Hence, fetch_rate

I've updated the param names from: - `entry` -> `money` - `other_currency` -> `to_currency` Hopefully this reduces the confusions around the names and makes the code more readable. In the event that we don't have the exchange rates in our cache, we want to be able to fetch for that exchange rate and load it for use. Hence, `fetch_rate`
Joelute commented 2025-07-05 12:12:53 +08:00 (Migrated from github.com)
Copy Link

Hi @zachgoll, I've just pushed some code to provide a clearer naming, better error handling, and new test cases for all the changes. Please take a look.

Hi @zachgoll, I've just pushed some code to provide a clearer naming, better error handling, and new test cases for all the changes. Please take a look.
jjmata commented 2025-07-26 01:00:47 +08:00 (Migrated from github.com)
Copy Link

This repo is no longer maintained. Want to submit your PR here instead, @Joelute? 🙏

This repo is [no longer maintained](https://github.com/maybe-finance/maybe/blob/main/README.md?plain=1#L15). Want to submit your PR [here](https://github.com/we-promise/sure) instead, @Joelute? 🙏

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
zachgoll
Joelute
Labels
Clear labels
📆 Planned
Not ready to be worked on yet, but planned for future
 💻 Self Hosted only
Issues pertaining to self-hosted versions of Maybe
 ⚙️ Config
ℹ️ Needs info
Further information is requested
 📱 Mobile
Bugs exclusively happening on mobile devices
 1️⃣ High Priority
High priority issues will be worked on next
 🚨 Urgent
Issues that are urgent for app functionality
 3️⃣ Low priority
Contributions accepted, but Maybe team will not be working on this in the near term
 2️⃣ Medium Priority
Community contributions accepted, Maybe team only works on if there are no high priority items open
 ⚡ Performance
Performance issues to address
 🎨 UI / UX
Issues related to UI / UX interactions and design.
 💎 Bounty
💰 Rewarded
📊 Data issue
Issues where the app functionally works, but the data is missing or inaccurate.
 🙋 Bounty claim
$1.5K
Community
This is a great issue for community members to work on
 dependencies
Pull requests that update a dependency file
 Design
Hosted only
Hosted app only
 Required self hoster data migration
Self hosters must run data migrations to resolve this issue
 ruby
Pull requests that update Ruby code
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
gavin
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: gavin/maybe#2404
Delete Branch "preload-exchange-rate"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?