Re: [PATCH damus] wallet: Add fiat pricing to Wallet View

0 views
Skip to first unread message

William Casarin

unread,
Apr 25, 2025, 8:34:39 PM4/25/25
to ericholguin, pat...@damus.io
On Fri, Apr 25, 2025 at 09:23:56AM -0600, ericholguin wrote:
>This PR adds the fiat price to user's wallet balance.
>Using Coinbase API to get BTC current price and the user's locale (region)
>to determine their currency.
>
>Changelog-Added: Added fiat pricing to wallet balance
>
>Signed-off-by: ericholguin <erich...@apache.org>
>
>Closes: https://github.com/damus-io/damus/pull/3001
>---

LGTM

Reviewed-by: William Casarin <jb...@jb55.com>

William Casarin

unread,
Apr 25, 2025, 8:36:26 PM4/25/25
to ericholguin, pat...@damus.io
o1 review:

From: n8n@monad
To: jb...@jb55.com
Subject: Review for PR wallet: Add fiat pricing to Wallet View #3001

https://github.com/damus-io/damus/pull/3001

Review for "wallet: Add fiat pricing to Wallet View" #3001
by ericholguin

Here are a few observations about potential issues in this PR:

1. Locale.currency force-unwrap: The code uses 'Locale.current.currency!.identifier', which could lead to a crash if the locale lacks a recognized currency. It might be safer to handle cases where this could be nil.

2. Error handling for fetch: In CoinbaseModel, if the network call fails, there's only a printed error. You may want to provide a user-facing fallback or
more robust error handling.


Reply all
Reply to author
Forward
0 new messages