Repository navigation
adds support to GBP, JPY and CNY - #463
Conversation
Deploying wallet-mutinynet with
|
| Latest commit: |
3049354
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://eb1f0bff.arkade-wallet.pages.dev |
| Branch Preview URL: | https://add-more-fiat-currencies.arkade-wallet.pages.dev |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds JPY, GBP, and CNY fiat support, exposes a new fiatDecimals() provider function, and replaces hardcoded 2-decimal fiat formatting with dynamic precision across UI, provider, formatter, and settings. Minor styling additions: Changes
Sequence Diagram(s)mermaid UI->>FProv: read fiatDecimals(), toFiat(), fromFiat() Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔍 Review —
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/fiat.ts (1)
3-10:⚠️ Potential issue | 🟡 MinorAdd fallback values to match the interface type declaration.
The
FiatPricesinterface declares all fields asnumber, but the API response mapping uses optional chaining (json.JPY?.last). While blockchain.info's ticker API does return JPY, GBP, and CNY data, the code lacks defensive handling for partial API responses. If any currency is missing, the field will beundefined, creating a type mismatch. This could causeNaNresults in downstream arithmetic operations.Apply fallback values to ensure type safety:
🛡️ Proposed fix
return { eur: json.EUR?.last, usd: json.USD?.last, chf: json.CHF?.last, - jpy: json.JPY?.last, - gbp: json.GBP?.last, - cny: json.CNY?.last, + jpy: json.JPY?.last ?? 0, + gbp: json.GBP?.last ?? 0, + cny: json.CNY?.last ?? 0, }Also applies to: 16-23
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/fiat.ts` around lines 3 - 10, The FiatPrices interface requires all fields to be numbers but the mapping uses optional chaining (e.g., json.JPY?.last) which can produce undefined; update the code that constructs the FiatPrices object to coerce each currency value to a number with a safe fallback (e.g., use "json.JPY?.last ?? 0" or "Number(json.JPY?.last ?? 0)") for eur, usd, chf, jpy, gbp, and cny so every property on the FiatPrices object is a numeric value and matches the interface (refer to the FiatPrices type and the place where the API response is mapped into that object).
🧹 Nitpick comments (3)
src/screens/Wallet/Send/Form.tsx (1)
330-332: Input field decimal handling is hardcoded to 2 for fiat currencies.While the display formatting now correctly uses
fiatDecimals(), the input field text value formatting still uses hardcoded2decimal places for fiat (lines 331, 427, 515). For JPY and CNY users, this means the input field will show values like "1234.00" while the display shows "1,234".Consider using
fiatDecimals()for consistency:♻️ Suggested refactor
// Line 331-332 - const maximumFractionDigits = useFiat ? 2 : 0 + const maximumFractionDigits = useFiat ? fiatDecimals() : 0 setTextValue(prettyNumber(value, maximumFractionDigits, false)) // Line 427 - setTextValue(useFiat ? prettyNumber(toFiat(sats), 2, false) : prettyNumber(sats, 0, false)) + setTextValue(useFiat ? prettyNumber(toFiat(sats), fiatDecimals(), false) : prettyNumber(sats, 0, false)) // Line 513-515 - const maximumFractionDigits = useFiat ? 2 : 0 + const maximumFractionDigits = useFiat ? fiatDecimals() : 0Also applies to: 427-427, 513-515
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/Wallet/Send/Form.tsx` around lines 330 - 332, The input-field formatting uses a hardcoded 2 decimals for fiat; update the code that computes maximumFractionDigits (where useFiat is checked before calling setTextValue) to call fiatDecimals() instead of using 2 so input text and display formatting match; specifically change the logic around useFiat/toFiat/maximumFractionDigits/prettyNumber before invoking setTextValue in the Send Form (the same pattern appears in the blocks that set textValue at the uses around the prettyNumber calls) to use fiatDecimals() when useFiat is true.src/components/Keyboard.tsx (1)
61-71: Inconsistent decimal handling ingetMaxDecimals()for fiat input.The function returns a hardcoded
2for fiat mode, but with JPY/CNY (zero-decimal currencies), users can type decimal values (e.g., "1234.56") that will then be displayed without decimals. This creates a confusing UX where input precision differs from display precision.Consider using
fiatDecimals()here as well:♻️ Proposed fix
const getMaxDecimals = () => { switch (inputMode) { case 'asset': return asset?.decimals ?? 0 case 'fiat': - return 2 + return fiatDecimals() case 'sats': default: return 0 } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Keyboard.tsx` around lines 61 - 71, getMaxDecimals currently hardcodes 2 for the 'fiat' inputMode which mismatches display precision for zero-decimal currencies; update the 'fiat' case in getMaxDecimals to call fiatDecimals(...) instead of returning 2, passing the current fiat currency/locale variable used elsewhere (e.g., fiatCurrency or selectedFiat) so the function returns the correct decimal count for currencies like JPY/CNY; ensure getMaxDecimals still falls back to a sensible default (0 or 2) if fiatDecimals cannot determine precision.src/providers/fiat.tsx (1)
31-36: Pre-existing: Division by zero when price feed fails.If
getPriceFeed()fails or returns incomplete data, prices remain at0(fromemptyFiatPrices), causingDecimal.div(fiat, 0)to returnInfinity. This affects all currencies including the new ones. The fallback at line 70 switches to sats display, but there's a window where conversions could produceInfinity.This is a pre-existing concern, but worth noting as scope expands to more currencies.
🛡️ Optional: Guard against zero prices
-const fromJPY = (fiat = 0) => toSatoshis(Decimal.div(fiat, fiatPrices.current.jpy).toNumber()) -const fromGBP = (fiat = 0) => toSatoshis(Decimal.div(fiat, fiatPrices.current.gbp).toNumber()) -const fromCNY = (fiat = 0) => toSatoshis(Decimal.div(fiat, fiatPrices.current.cny).toNumber()) +const fromJPY = (fiat = 0) => fiatPrices.current.jpy ? toSatoshis(Decimal.div(fiat, fiatPrices.current.jpy).toNumber()) : 0 +const fromGBP = (fiat = 0) => fiatPrices.current.gbp ? toSatoshis(Decimal.div(fiat, fiatPrices.current.gbp).toNumber()) : 0 +const fromCNY = (fiat = 0) => fiatPrices.current.cny ? toSatoshis(Decimal.div(fiat, fiatPrices.current.cny).toNumber()) : 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/fiat.tsx` around lines 31 - 36, The conversion helpers (fromEUR, fromUSD, fromCHF, fromJPY, fromGBP, fromCNY) can divide by zero when fiatPrices.current.* is 0; update each helper to check the corresponding fiatPrices.current.<currency> is a positive number before calling Decimal.div and toSatoshis, and return a safe fallback (e.g., 0 sats or null) when the price is missing/<=0; ensure the same guard is applied consistently across the fromEUR/fromUSD/fromCHF/fromJPY/fromGBP/fromCNY functions and consider centralizing the check into a small helper (e.g., safeFromFiat) that takes the price key and fiat amount and only performs Decimal.div when price > 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/providers/fiat.tsx`:
- Around line 61-63: The fiatDecimals helper incorrectly treats Fiats.CNY as
zero-decimal; update the logic in fiatDecimals so that only Fiats.JPY returns 0
and Fiats.CNY returns 2 (i.e., change the condition that checks [Fiats.JPY,
Fiats.CNY] to exclude Fiats.CNY), referencing the fiatDecimals function and the
Fiats enum and config.fiat to locate the change; ensure the function returns 2
for CNY per ISO 4217.
---
Outside diff comments:
In `@src/lib/fiat.ts`:
- Around line 3-10: The FiatPrices interface requires all fields to be numbers
but the mapping uses optional chaining (e.g., json.JPY?.last) which can produce
undefined; update the code that constructs the FiatPrices object to coerce each
currency value to a number with a safe fallback (e.g., use "json.JPY?.last ?? 0"
or "Number(json.JPY?.last ?? 0)") for eur, usd, chf, jpy, gbp, and cny so every
property on the FiatPrices object is a numeric value and matches the interface
(refer to the FiatPrices type and the place where the API response is mapped
into that object).
---
Nitpick comments:
In `@src/components/Keyboard.tsx`:
- Around line 61-71: getMaxDecimals currently hardcodes 2 for the 'fiat'
inputMode which mismatches display precision for zero-decimal currencies; update
the 'fiat' case in getMaxDecimals to call fiatDecimals(...) instead of returning
2, passing the current fiat currency/locale variable used elsewhere (e.g.,
fiatCurrency or selectedFiat) so the function returns the correct decimal count
for currencies like JPY/CNY; ensure getMaxDecimals still falls back to a
sensible default (0 or 2) if fiatDecimals cannot determine precision.
In `@src/providers/fiat.tsx`:
- Around line 31-36: The conversion helpers (fromEUR, fromUSD, fromCHF, fromJPY,
fromGBP, fromCNY) can divide by zero when fiatPrices.current.* is 0; update each
helper to check the corresponding fiatPrices.current.<currency> is a positive
number before calling Decimal.div and toSatoshis, and return a safe fallback
(e.g., 0 sats or null) when the price is missing/<=0; ensure the same guard is
applied consistently across the fromEUR/fromUSD/fromCHF/fromJPY/fromGBP/fromCNY
functions and consider centralizing the check into a small helper (e.g.,
safeFromFiat) that takes the price key and fiat amount and only performs
Decimal.div when price > 0.
In `@src/screens/Wallet/Send/Form.tsx`:
- Around line 330-332: The input-field formatting uses a hardcoded 2 decimals
for fiat; update the code that computes maximumFractionDigits (where useFiat is
checked before calling setTextValue) to call fiatDecimals() instead of using 2
so input text and display formatting match; specifically change the logic around
useFiat/toFiat/maximumFractionDigits/prettyNumber before invoking setTextValue
in the Send Form (the same pattern appears in the blocks that set textValue at
the uses around the prettyNumber calls) to use fiatDecimals() when useFiat is
true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3a12ce9-3585-4a9b-9a6a-e3ac6dea66f9
📒 Files selected for processing (16)
src/components/Balance.tsxsrc/components/InputAmount.tsxsrc/components/Keyboard.tsxsrc/components/TransactionsList.tsxsrc/lib/fiat.tssrc/lib/format.tssrc/lib/types.tssrc/providers/fiat.tsxsrc/screens/Apps/Assets/Detail.tsxsrc/screens/Apps/Assets/MintSuccess.tsxsrc/screens/Settings/Fiat.tsxsrc/screens/Wallet/Notes/Success.tsxsrc/screens/Wallet/Receive/Amount.tsxsrc/screens/Wallet/Receive/Success.tsxsrc/screens/Wallet/Send/Form.tsxsrc/screens/Wallet/Send/Success.tsx
Deploying wallet-bitcoin with
|
| Latest commit: |
3049354
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ca32f6f7.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://add-more-fiat-currencies.wallet-bitcoin.pages.dev |
|
Review (Arkana) — Clean PR. Currency additions are consistent, and the Observations:
LGTM for the fiat logic. The input validation nit (point 2) and division-by-zero (point 3) are worth tracking but non-blocking. |
🔍 Arkana Review — New commits (prevent division by zero + fix Keyboard with JPY)Scope: Two new commits since last review — division-by-zero guard on ✅ Division by zero fix (
|
ghost
mentioned this pull request
* adds support to JPY and CNY * fix: Per ISO 4217 standards, CNY (Chinese Yuan) has a minor unit exponent of 2 * prevent division by zero * fix Keyboard with JPY
ghost
mentioned this pull request
@tiero please review
closes #451
Big change because JPY and CNY are non decimal currencies, and all our code assumed 2 decimal places for all fiat currencies.
Summary by CodeRabbit
New Features
UX / Formatting
Style