-
Notifications
You must be signed in to change notification settings - Fork 55
Render Wise transfers in the correct currency #11258
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
app/views/events/transfers.html.erb
Outdated
<td style="text-align: right;"> | ||
<% if d.is_a? Wire %> | ||
<% case d %> | ||
<% when Wire %> | ||
<%= render_money(d.local_hcb_code.amount_cents.abs) %> | ||
<% when WiseTransfer %> | ||
<% if d.currency == "USD" %> | ||
<%= render_money(d.usd_amount || d.amount) %> | ||
<% else %> | ||
<% fx_tooltip = capture do %> | ||
<% if d.usd_amount_cents %> | ||
<%= render_money(d.usd_amount_cents) %> | ||
<% elsif d.quoted_usd_amount_cents %> | ||
Quoted <%= render_money(d.quoted_usd_amount_cents) %> | ||
<% else %> | ||
No USD amount | ||
<% end %> | ||
<% end %> | ||
|
||
<span class="tooltipped tooltipped--w inline-block" aria-label="<%= fx_tooltip %>"> | ||
<%= d.amount.format(with_currency: true) %> | ||
</span> | ||
<% end %> | ||
<% else %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the pattern should match the wire pattern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my first thought but I wasn't sure d.local_hcb_code.amount_cents
would be populated.
- The only non-nullable field on
WiseTransfer
isamount_cents
which can be in any number of currencies and is populated when the transfer is created_cents
is a misnomer as it technically stores subunits (e.g. JPY 1000 would be stored as 1000 as it is non-fractional)
quoted_usd_amount_cents
(nullable) is populated after a call to the Wise API (but might change?)usd_amount_cents
(nullable) is populated by ops when the transfer is processed
As far as I can tell we don't have any validation on the presence of either (3) or (4). quoted_usd_amount_cents
is populated on creation and synced to the canonical pending transaction and subsequently replaced by usd_amount_cents
(if present).
So in theory, we should be fine relying on the canonical pending transaction but
- I would like to see stronger validations for all of the above
- We would probably want to show the original currency (maybe in a tooltip) as that's what the user entered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on validations but d.local_hcb_code.amount_cents
should always be set. I'm cool with either product direction - @YodaLightsabr got thoughts? I'd just like us to be consistent with what we do for wires.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would prefer making the USD amount more prominent, but we should show the local amount in a tooltip. I will say that wherever we're rendering the USD amount, we should use the HCB code amount because that will always match the transaction amount on the ledger.
_cents is a misnomer as it technically stores subunits (e.g. JPY 1000 would be stored as 1000 as it is non-fractional)
This is true, however I believe this is the terminology used by the money-rails
gem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
…al amount + currency
Summary of the problem
We are incorrectly rendering Wise transfer amounts in non-USD currencies as USD.
Describe your changes
Add special handling for Wise transfers: