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

transfer: Support transfers of different currencies between accounts. #2243

Merged
Joelute merged 2 commits from multi-currency-transfer into main 2025-06-26 04:34:18 +08:00
Conversation 17 Commits 2 Files Changed 2 +106 -2
Joelute commented 2025-05-15 09:30:20 +08:00 (Migrated from github.com)
Copy Link

Allow transfer matcher to match transfers to a different currency.

image

Fixes: #1852.

Allow transfer matcher to match transfers to a different currency. ![image](https://github.com/user-attachments/assets/a68bc0d7-f147-485f-b298-f95982c6dc71) Fixes: #1852.
❤️ 5
pbendus commented 2025-05-21 03:32:40 +08:00 (Migrated from github.com)
Copy Link

How do we upvote this lol?

How do we upvote this lol?
Shpigford commented 2025-05-21 20:50:19 +08:00 (Migrated from github.com)
Copy Link

I'll let @zachgoll ultimately weigh in on this, but adding at least a couple of tests for this will go a long way to getting it merged.

I'll let @zachgoll ultimately weigh in on this, but adding at least a couple of tests for this will go a long way to getting it merged.
👍 1
zachgoll (Migrated from github.com) reviewed 2025-05-21 22:24:45 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

@Joelute thanks for the addition here!

Per @Shpigford's comments, we'll need some concise and simple tests that clearly demonstrate that this query works and does not break any of our existing functionality for auto matching and manual matching.

We should also combine this logic into the existing transfer_match_candidates query. There's a ton of logic to reason about with this process, so keeping it centralized in a single query allows use to debug things in one spot if it were to break.

@Joelute thanks for the addition here! Per @Shpigford's comments, we'll need some concise and simple tests that clearly demonstrate that this query works and does not break any of our existing functionality for auto matching and manual matching. We should also combine this logic into the existing `transfer_match_candidates` query. There's a ton of logic to reason about with this process, so keeping it centralized in a single query allows use to debug things in one spot if it were to break.
Shpigford commented 2025-05-21 22:29:24 +08:00 (Migrated from github.com)
Copy Link

bugbot run

bugbot run
cursor[bot] commented 2025-05-21 22:29:27 +08:00 (Migrated from github.com)
Copy Link

Bug: Cross-Currency Transfer Matching Inconsistency

auto_match_transfers! still uses transfer_match_candidates (single-currency scope) even though the commit introduces and switches other call-sites to transfer_match_multi_currency_candidates. As a result, bulk/automatic matching will completely ignore cross-currency transfers while per-transaction matching now considers them, creating inconsistent behaviour and defeating the stated purpose of this change.

app/models/family/auto_transfer_matchable.rb#L59-L80

8d74954813/app/models/family/auto_transfer_matchable.rb (L59-L80)

Fix in Cursor

Was this report helpful? Give feedback by reacting with 👍 or 👎

<details open> <summary><h3>Bug: Cross-Currency Transfer Matching Inconsistency</h3></summary> auto_match_transfers! still uses transfer_match_candidates (single-currency scope) even though the commit introduces and switches other call-sites to transfer_match_multi_currency_candidates. As a result, bulk/automatic matching will completely ignore cross-currency transfers while per-transaction matching now considers them, creating inconsistent behaviour and defeating the stated purpose of this change. <p></p> <details> <summary><code>app/models/family/auto_transfer_matchable.rb#L59-L80</code></summary> https://github.com/maybe-finance/maybe/blob/8d749548138d349997b6be9926f500e7b79d0ddd/app/models/family/auto_transfer_matchable.rb#L59-L80 </details> <a href="https://cursor.com/open?data=eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6ImJ1Z2JvdC12MSJ9.eyJ0eXBlIjoiQlVHQk9UX0ZJWF9JTl9DVVJTT1IiLCJkYXRhIjp7ImJ1ZyI6ImV5SnNiMk5oZEdsdmJuTWlPbHQ3SW1acGJHVWlPaUpoY0hBdmJXOWtaV3h6TDJaaGJXbHNlUzloZFhSdlgzUnlZVzV6Wm1WeVgyMWhkR05vWVdKc1pTNXlZaUlzSW5OMFlYSjBUR2x1WlNJNk5Ua3NJbVZ1WkV4cGJtVWlPamd3TENKamIyUmxUR2x1WlhNaU9sdGRmVjBzSW1sa0lqb2lPV001TkRCbVlUWXRaR1ZpWXkwME16Rm1MV0ZrTkdZdFptTTJORFZqWXpRellqWTBJaXdpWkdWelkzSnBjSFJwYjI0aU9pSmhkWFJ2WDIxaGRHTm9YM1J5WVc1elptVnljeUVnYzNScGJHd2dkWE5sY3lCMGNtRnVjMlpsY2w5dFlYUmphRjlqWVc1a2FXUmhkR1Z6SUNoemFXNW5iR1V0WTNWeWNtVnVZM2tnYzJOdmNHVXBJR1YyWlc0Z2RHaHZkV2RvSUhSb1pTQmpiMjF0YVhRZ2FXNTBjbTlrZFdObGN5QmhibVFnYzNkcGRHTm9aWE1nYjNSb1pYSWdZMkZzYkMxemFYUmxjeUIwYnlCMGNtRnVjMlpsY2w5dFlYUmphRjl0ZFd4MGFWOWpkWEp5Wlc1amVWOWpZVzVrYVdSaGRHVnpMaUJCY3lCaElISmxjM1ZzZEN3Z1luVnNheTloZFhSdmJXRjBhV01nYldGMFkyaHBibWNnZDJsc2JDQmpiMjF3YkdWMFpXeDVJR2xuYm05eVpTQmpjbTl6Y3kxamRYSnlaVzVqZVNCMGNtRnVjMlpsY25NZ2QyaHBiR1VnY0dWeUxYUnlZVzV6WVdOMGFXOXVJRzFoZEdOb2FXNW5JRzV2ZHlCamIyNXphV1JsY25NZ2RHaGxiU3dnWTNKbFlYUnBibWNnYVc1amIyNXphWE4wWlc1MElHSmxhR0YyYVc5MWNpQmhibVFnWkdWbVpXRjBhVzVuSUhSb1pTQnpkR0YwWldRZ2NIVnljRzl6WlNCdlppQjBhR2x6SUdOb1lXNW5aUzRpTENKamIyNW1hV1JsYm1ObElqb3dMams1T1RJNU5EZzJNekF3TlRRd05UWXNJbU5oZEdWbmIzSjVJam9pVEU5SFNVTmZRbFZISW4wIiwiYnJhbmNoIjoibXVsdGktY3VycmVuY3ktdHJhbnNmZXIifSwiaWF0IjoxNzQ3ODM4MDUxLCJleHAiOjE3NDg0NDI4NTF9.RcLpUw2EqT5b_MhsMEjRV_qL8lR02sI9rhj16-FSxxPxiPIj7ySyy8W0hutZxVdRi1kEfDs4aqogZqSy1F6hR1S3ed9NHkoOo4eRBALvcxLAHbp6huBR99CEcEEK1jYfun-SwBERc5Bs1PqTKyy0t2iJN0lrbKmSX-_n22b157LzTd_PEGJ_2rA8Ars1WM5GiZWy5anxolSj9Vg16pVgJHG354xppUGMgn8qqCcNmOdI5qTRJscHhL4A31lLVrxWlLl5VGR0qEaI6oAXbolYbG0-yZsbb6aNPYfS28X73LcTVBPXvOLDZ2VXML3B02gAAHETG5Hj0mt5OtE5Wju-Lg">Fix in Cursor</a> </details> --- _Was this report helpful? Give feedback by reacting with 👍 or 👎_
Joelute commented 2025-05-25 12:46:07 +08:00 (Migrated from github.com)
Copy Link

bugbot run

bugbot run
Shpigford commented 2025-05-25 17:31:44 +08:00 (Migrated from github.com)
Copy Link

bugbot run

bugbot run
cursor[bot] (Migrated from github.com) reviewed 2025-05-25 17:35:16 +08:00
cursor[bot] (Migrated from github.com) left a comment
Copy Link

Bug: Zero Division Error in Transfer Matching

Division by zero error in the transfer matching query. The calculation ABS(inflow_candidates.amount / (outflow_candidates.amount * exchange_rates.rate)) fails if exchange_rates.rate is 0. While economically invalid, a zero rate is not prevented by the IS NOT NULL check and can exist due to data errors, causing the query to crash.

app/models/family/auto_transfer_matchable.rb#L42-L43

ae80e60bb1/app/models/family/auto_transfer_matchable.rb (L42-L43)

Fix in Cursor

Was this report helpful? Give feedback by reacting with 👍 or 👎

<details open> <summary><h3>Bug: Zero Division Error in Transfer Matching</h3></summary> Division by zero error in the transfer matching query. The calculation `ABS(inflow_candidates.amount / (outflow_candidates.amount * exchange_rates.rate))` fails if `exchange_rates.rate` is 0. While economically invalid, a zero rate is not prevented by the `IS NOT NULL` check and can exist due to data errors, causing the query to crash. <p></p> <details> <summary><code>app/models/family/auto_transfer_matchable.rb#L42-L43</code></summary> https://github.com/maybe-finance/maybe/blob/ae80e60bb1ecfc8a3c0c98bb174e97cd00e1d330/app/models/family/auto_transfer_matchable.rb#L42-L43 </details> <a href="https://cursor.com/open?data=eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6ImJ1Z2JvdC12MSJ9.eyJ0eXBlIjoiQlVHQk9UX0ZJWF9JTl9DVVJTT1IiLCJkYXRhIjp7ImJ1ZyI6ImV5SnNiMk5oZEdsdmJuTWlPbHQ3SW1acGJHVWlPaUpoY0hBdmJXOWtaV3h6TDJaaGJXbHNlUzloZFhSdlgzUnlZVzV6Wm1WeVgyMWhkR05vWVdKc1pTNXlZaUlzSW5OMFlYSjBUR2x1WlNJNk5ESXNJbVZ1WkV4cGJtVWlPalF6TENKamIyUmxUR2x1WlhNaU9sdGRmVjBzSW1sa0lqb2lORFZoTTJKaE5HUXRaRFl6TnkwMFpqUXhMV0ZqTlRJdE5ERTRaREU0TkdObE5UaGlJaXdpWkdWelkzSnBjSFJwYjI0aU9pSkVhWFpwYzJsdmJpQmllU0I2WlhKdklHVnljbTl5SUdsdUlIUm9aU0IwY21GdWMyWmxjaUJ0WVhSamFHbHVaeUJ4ZFdWeWVTNGdWR2hsSUdOaGJHTjFiR0YwYVc5dUlHQkJRbE1vYVc1bWJHOTNYMk5oYm1ScFpHRjBaWE11WVcxdmRXNTBJQzhnS0c5MWRHWnNiM2RmWTJGdVpHbGtZWFJsY3k1aGJXOTFiblFnS2lCbGVHTm9ZVzVuWlY5eVlYUmxjeTV5WVhSbEtTbGdJR1poYVd4eklHbG1JR0JsZUdOb1lXNW5aVjl5WVhSbGN5NXlZWFJsWUNCcGN5QXdMaUJYYUdsc1pTQmxZMjl1YjIxcFkyRnNiSGtnYVc1MllXeHBaQ3dnWVNCNlpYSnZJSEpoZEdVZ2FYTWdibTkwSUhCeVpYWmxiblJsWkNCaWVTQjBhR1VnWUVsVElFNVBWQ0JPVlV4TVlDQmphR1ZqYXlCaGJtUWdZMkZ1SUdWNGFYTjBJR1IxWlNCMGJ5QmtZWFJoSUdWeWNtOXljeXdnWTJGMWMybHVaeUIwYUdVZ2NYVmxjbmtnZEc4Z1kzSmhjMmd1SWl3aVkyOXVabWxrWlc1alpTSTZNQzQyTXpZME9UWTBOelF5TkRFM05EWTRMQ0pqWVhSbFoyOXllU0k2SWxCUFZFVk9WRWxCVEY5RlJFZEZYME5CVTBVaWZRIiwiYnJhbmNoIjoibXVsdGktY3VycmVuY3ktdHJhbnNmZXIifSwiaWF0IjoxNzQ4MTY1NzE1LCJleHAiOjE3NDg3NzA1MTV9.MbACfN7b2nM-Z_89HdvyXLLH94g5lFLKRdUidiUNuSWNhnzUg0QFNonR3MVSnHhJcg2UbC_d5TQZcIOYMLD145sxcfL89Qw35_UsBstQF1c1S16ZT5J-ZZE2WArzzr6Fc74fE2k4b12UiUDoGjPXMBb0uS0EIOiC5FsKXYcPxdWloT89G8Hey96amM6xpXjiUHwiclKo4Q8AdjpqEHuG3-iX-xQQ0CYjr3j5_SfEYVHEHTlZPFSUz6Wyymv4K1tLWH41WqvHZL6Rq9ztLxkZxEbf4hNyY4NR9_KM8X7KsSRFHvkWN7dUqbVlCZMbufcYRHl5SGgnGPg5K-KXACNHRg">Fix in Cursor</a> </details> --- _Was this report helpful? Give feedback by reacting with 👍 or 👎_
ragnarok22 commented 2025-06-06 00:02:06 +08:00 (Migrated from github.com)
Copy Link

Oh, this is something really helpful, I also noticed that It adds the currencies as total without conversion (in the current stable version)

For example:
image

Oh, this is something really helpful, I also noticed that It adds the currencies as total without conversion (in the current stable version) For example: ![image](https://github.com/user-attachments/assets/0efac091-249e-427f-a7fa-9560f04071e3)
str4d commented 2025-06-13 23:24:31 +08:00 (Migrated from github.com)
Copy Link

@ragnarok22 this PR is unrelated, your issue is https://github.com/maybe-finance/maybe/issues/2301.

@ragnarok22 this PR is unrelated, your issue is https://github.com/maybe-finance/maybe/issues/2301.
👍 1
pbendus commented 2025-06-16 14:47:23 +08:00 (Migrated from github.com)
Copy Link

@zachgoll @Shpigford

Could you guys review this one again please? 🙏

@zachgoll @Shpigford Could you guys review this one again please? 🙏
Shpigford commented 2025-06-16 17:48:29 +08:00 (Migrated from github.com)
Copy Link

bugbot run

bugbot run
zachgoll (Migrated from github.com) reviewed 2025-06-16 21:24:50 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

Looking a lot better! left a few suggestions, but otherwise looks good.

Looking a lot better! left a few suggestions, but otherwise looks good.
app/models/family/auto_transfer_matchable.rb
@@ -24,12 +22,26 @@ module Family::AutoTransferMatchable
rejected_transfers.inflow_transaction_id = inflow_candidates.entryable_id AND
zachgoll (Migrated from github.com) commented 2025-06-16 21:23:24 +08:00
Copy Link

I think we can avoid all of the null / 0 checking with a NULLIF:

OR (
  inflow_candidates.currency <> outflow_candidates.currency AND
  ABS(inflow_candidates.amount / NULLIF(outflow_candidates.amount * exchange_rates.rate, 0)) BETWEEN 0.95 AND 1.05
)
I think we can avoid all of the null / 0 checking with a NULLIF: ```sql OR ( inflow_candidates.currency <> outflow_candidates.currency AND ABS(inflow_candidates.amount / NULLIF(outflow_candidates.amount * exchange_rates.rate, 0)) BETWEEN 0.95 AND 1.05 ) ```
test/models/family/auto_transfer_matchable_test.rb
@@ -53,4 +98,51 @@ class Family::AutoTransferMatchableTest < ActiveSupport::TestCase
@family.auto_match_transfers!
zachgoll (Migrated from github.com) commented 2025-06-16 21:09:57 +08:00
Copy Link

I think even when the rate is 0 we should go ahead and run the standard matching logic.

A value of 0 in the exchange rates table would be considered "bad data" and I don't think it is the responsibility of the transfer matcher to handle.

So in other words, let's remove this test as I don't think it's testing the right thing.

I think even when the rate is `0` we should go ahead and run the standard matching logic. A value of `0` in the exchange rates table would be considered "bad data" and I don't think it is the responsibility of the transfer matcher to handle. So in other words, let's remove this test as I don't think it's testing the right thing.
Joelute (Migrated from github.com) reviewed 2025-06-18 04:59:06 +08:00
app/models/family/auto_transfer_matchable.rb
@@ -24,12 +22,26 @@ module Family::AutoTransferMatchable
rejected_transfers.inflow_transaction_id = inflow_candidates.entryable_id AND
Joelute (Migrated from github.com) commented 2025-06-18 04:58:00 +08:00
Copy Link

Looks cleaner than the current conditions. Updated in the latest commit.

Looks cleaner than the current conditions. Updated in the latest commit.
test/models/family/auto_transfer_matchable_test.rb
@@ -53,4 +98,51 @@ class Family::AutoTransferMatchableTest < ActiveSupport::TestCase
@family.auto_match_transfers!
Joelute (Migrated from github.com) commented 2025-06-18 04:58:46 +08:00
Copy Link

Sounds good, I've removed the testcase in the latest commit.

Sounds good, I've removed the testcase in the latest commit.
Joelute commented 2025-06-18 07:40:05 +08:00 (Migrated from github.com)
Copy Link

Hey @zachgoll, I've updated the PR based on the reviews. Please take another look.

Hey @zachgoll, I've updated the PR based on the reviews. Please take another look.
👀 1 ❤️ 1
zachgoll (Migrated from github.com) approved these changes 2025-06-26 04:33:33 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

Looks good now! Thanks for working through the feedback on this one!

Looks good now! Thanks for working through the feedback on this one!
AlexDaimler commented 2025-07-27 12:04:13 +08:00 (Migrated from github.com)
Copy Link

Hi Guys could you please let me know how to update to this? thanks.

Hi Guys could you please let me know how to update to this? thanks.
Sign in to join this conversation.
Reviewers
No Reviewers
cursor[bot]
Joelute
zachgoll
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#2243
Delete Branch "multi-currency-transfer"

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?