fix: keep preset amounts when fiat has no rate - #4651
TheSeydiCharyyev wants to merge 5 commits into
Conversation
With fiat as the unit and no rate for it, getRawAmountFromSats returned the 'Disabled' placeholder, so preset amounts in Send, ClinkPay and LnurlPay showed 'Disabled' in the input. Opening Send with an amount dropped it to 0, and a new amount while Send was open stayed hidden behind the placeholder. getRawAmountFromSats now returns the amount in sats with forceUnit 'sats' and the error when the fiat conversion fails. The views pin their amount inputs to sats until the screen is closed, and AmountInput shows the error under the input. The no-currency branch of getUnformattedAmount sets error too. Closes ZeusLN#4635
There was a problem hiding this comment.
Athena Review
Pinned commit: 60a2ded
anthropic:claude-opus-5: completed openai:gpt-5.6-sol: completed
4 validated findings. This is a nonblocking COMMENT review, not an approval. No findings does not establish correctness.
Limitations:
- Full source of utils/AmountUtils.ts getUnformattedAmount (return type and the exact conditions that set the 'Disabled'/error branches) is not included, so the nullability of
amountand the meaning of each error string could not be verified. - components/AmountInput.tsx props interface is only partially shown; could not verify that
hideUnitChangeButtonexists as a prop (used in Send.tsx and LnurlPay.tsx) or thatthemeColor('warning')is a defined theme key. - No list of all callers of getRawAmountFromSats was provided; call sites outside the diff that still expect a
stringreturn value could not be checked. - The test file's beforeEach/setup (which would reset the mutated
fiatStore.fiatRatesandsettingsStore.settings.fiatbetween cases) is not included, so cross-test state pollution from the added tests could not be confirmed or ruled out. - The full repository call-site search and build/test results were not supplied, so completeness of the getRawAmountFromSats return-type migration could not be verified.
|
Before and after on the Android emulator (Pixel 9 Pro, Android 16), with IDR selected so the unit is fiat with no rate. Columns: Send opened with With USD, which has a rate, the screen is the same as on master: the amount in fiat, the rate line and the unit toggle. |
|
I think when in That would resolve #3831 imho. No auto-switch but user is given an explanation for the display and can switch as they wish. |
recalculateDisplayAmount only updated state when the amount string changed. If the unit was switched to fiat without a rate while the input already showed the sats value, the sats fallback was equal to it, so the input was not pinned and later read the value as fiat.
When the unit is fiat and there is no rate, jumboText amounts such as the wallet balance and the Verify screen total showed only N/A. They now show the fiat error under the amount. Pending amounts do not repeat it, and nothing is shown while rates are still loading. Tapping the amount still switches units. Refs ZeusLN#3831
|
Added in 815d13b: large ( |
kaloudis
left a comment
There was a problem hiding this comment.
Nits from another pass over the branch, all non-blocking.
Pin the additional-output amount inputs in Send the same way as the primary input. Hide the fiat conversion row in AmountInput when fiatError is shown, so it no longer shows N/A under the warning. Route the fixed-amount branch in LnurlPay through getRawAmountFromSats instead of relying on resetUnits running first.
|
Round 2. All three of my round-1 nits are done, and the bot's two MEDIUMs are correctly dispatched. Code looks right to me. What is left is device coverage on the paths that changed after round 1. VerifiedThe The fiat-disabled rebuttal is correct. The ClinkPay rebuttal is correct. Locale claim checks out. One thing worth pulling into the description6b03c58 fixes a second, unrelated bug on the LnurlPay fixed-amount branch, and I do not think you noticed. On master, With a working rate and fiat as the active unit, that means a fixed-amount LNURL prefilled the locked input with the fiat string, so a 12,618 sat request rendered as "6.31 sats" and So my round-1 nit about the hidden coupling was not cosmetic. Please call this out in the description, since it changes what a reviewer should test and it is a user-facing prefill bug independent of #4635. Testing: what I would like before mergeThe store and util side is well covered, and the nine-test delta against master's
The additional-output inputs are fine to leave unverified given LDK Node has no onchain batching and the props are identical to the primary input you did run. Say that in the description rather than leaving the box empty. Optional
tACK on the code. Happy to merge once the two LnurlPay runs and an iOS pass are in the matrix. |
|
Same as on #4450: I cannot test iOS, there is no Mac or iPhone here, so that box stays empty on my side. Can someone with a Mac take that pass? The change on the jumbo path is the centering The rest I will do: both LnurlPay runs on Android, the description note about |
|
I checked 6b03c58 before putting it in the description, and I cannot confirm the second bug. Two of your three steps hold. The outcome does not. Holds: Does not hold: the other arm of the ternary is
So on that branch the live unit is already I ran it as well, against master's
The same holds after 6b03c58. Do you see a path where the two arms differ? The only way I can get the fiat string into the prefill is if If you still read it as a user-facing fix, name the case and I will run it on the device before I write it into the description. The rest of round 2 is unchanged and in progress: both LnurlPay runs on Android, the additional-output note, and the |
|
Both LnurlPay runs are done on Android, and I ran the same cases on master as a control, so the before and after sit side by side. Pixel 9 Pro emulator, API 36, debug build, an LND (REST) stub wallet. I checked which code was on the device each time: the branch bundle has 1. The fixed-amount branch. I cannot reproduce the second bug. master 7838154, USD active, the rate row reading This branch, same case: That is what the trace said. The So 2. The pin. Here the two differ a lot. IDR selected, unit sats, 12,618 entered and confirmed on a variable-amount LNURL, then the keypad, unit switched to IDR there, back out without confirming. master: the input reads This branch: the input stays 3. The additional outputs are tested now, not assumed. I was wrong that I could not reach them. The stand wallet is LND (REST), and With IDR and no rate, an on-chain Send with one added output: both Amount inputs are in sats, both carry 4. One thing that is not from this PR. On the fixed-amount screen with a currency that has no rate, the lines under the input read master prints the same 5. iOS. Still not possible here, no Mac and no iPhone. I will add the |
Send.tsx:220 and the LnurlPay focus handler re-derive the amount with fixedUnits sats while the active unit is still fiat without a rate. Both rely on that call returning no error. The existing fixedUnits test only covers a working rate.
There was a problem hiding this comment.
@TheSeydiCharyyev, I think the new fiatError = error assignment in the fixed-amount branch is effectively dead code.
resetUnits() runs unconditionally immediately before this branch and forces unitsStore.units to 'sats'. Since getRawAmountFromSats(minSendableSats) is called without a fixedUnits override, it always takes the sats path, so getUnformattedAmount returns without an error and error is always undefined.
This also means the change doesn’t actually achieve the goal described in 6b03c58c1 (“Route the fixed-amount branch in LnurlPay through getRawAmountFromSats instead of relying on resetUnits running first”) — resetUnits is still what determines the result.
It’s harmless today because locked is independently forced for fixed amounts, but worth revisiting since the new error handling won’t protect the input if that separate logic changes later.
There was a problem hiding this comment.
You are right. That fiatError line is dead.
resetUnits() runs just above it, on the same condition. getUnformattedAmount reads the unit live at AmountUtils.ts:120. So the call always goes to the sats path and never returns an error.
I said the same in my comment above. You can also see it on a device: with IDR the screen shows Infinity sats = 1 IDR and ≈N/A, and no warning at all. That is the dead line.
You are right about the commit message too, and I did not see this. 6b03c58 says the branch does not rely on resetUnits() any more. It still does. The call has no fixedUnits, so the live unit decides.
One fix to "harmless today". locked helps, but the main reason is another one: the payment does not read the string on the screen. stateFromProps puts minSendableSats into satAmount (LnurlPay.tsx:186). recalculateDisplayAmount writes only amount. CONFIRM sends this.state.satAmount (:695). So a wrong unit here is a display bug, not a wrong payment.
Two lines would do what the commit message says:
getRawAmountFromSats(minSendableSats, 'sats')for the stringforceUnitset tosatsfor the fixed amount case, not only whenfiatErroris set
forceUnit goes into effectiveUnits at AmountInput.tsx:189. It decides what you see and how the string is read back to sats. After that the screen does not need resetUnits(). Nothing changes today, because resetUnits() already sets sats. The dead line goes away too.
This came from a round 1 review nit, so I do not want to grow the PR on my own. If you guys say it belongs here, I push it and run the two fixed amount cases on the device again. If not, I open a new PR.
| @@ -307,6 +315,8 @@ export default class ClinkPay extends React.Component< | |||
| </Text> | |||
| <AmountInput | |||
There was a problem hiding this comment.
nit: missing hideUnitChangeButton={!!fiatError}, which the sibling Send.tsx / LnurlPay.tsx call sites both pass.
It’s currently masked because fiatError is only reachable when locked is already true (Fixed-price offers), but it’s an inconsistency that could surface if that coupling changes later.
There was a problem hiding this comment.
Checked it. You are right about the facts.
ClinkPay.tsx is the only call site without it. Send.tsx passes it in four places, LnurlPay.tsx in one.
It changes nothing today. fiatError is set only inside priceType === Fixed && price (ClinkPay.tsx:117-123), and locked is loading || priceType === Fixed (:329-333). The guard in the component is !hideUnitChangeButton && !locked (AmountInput.tsx:258), so locked alone already hides the button.
One thing you may have missed: this line was already looked at in round 2. The bot raised it, I said it was not a bug, and Evan checked it and wrote "Passing hideUnitChangeButton there would be redundant. Leaving it is fine". It is here: #4651 (comment)
Your point is not the same as the bot's. The bot said the button shows up. You say the three call sites do not match. That part is true, and it is the same shape as your other note: the code leans on a coupling instead of saying what it wants.
I am fine either way. If you guys want them to match, I add the one line in the same commit as the forceUnit change from the other thread. No behaviour change, only the same props everywhere.
|
Round 3. You are right about 6b03c58, and I was wrong. On the runs:
a9c1321 looks right. Remaining before merge: update the description (the LnurlPay runs, the additional-output run, drop the 6b03c58 claim, link the Infinity issue). I will do the iOS pass on the tACK a9c1321 on Android runs + code. Will upgrade to ACK after iOS. |
Description
Relates to issue: #4635, #3831
When fiat is the active unit and there is no rate for it:
satAmountparam) no longer reaches the amount input as the "Disabled" placeholder. The input shows the amount in sats instead, with the reason under it.jumboText) that show "N/A" now have the reason under them, for example the wallet balance and the Verify screen total. There is no auto-switch; tapping the amount still changes units. This is the approach suggested for handle N/A amounts and disable fiat currency rates while app is offline #3831.Before this PR,
getRawAmountFromSatsreturnedgetUnformattedAmount(...).amount || sats, and the placeholder is truthy, so the fallback never ran. What the user got depended on when the amount arrived (details and before screenshots in the issue):AmountInputparsed "Disabled" as 0 and passed that up, so the amount was dropped.AmountInputdid not pass anything up, so Send kept the real amount while the field showed "Disabled" and 0 sats. Proceed was on, and the Verify screen showed "≈N/A", so the amount was not visible before Slide to Pay.Changes:
getUnformattedAmount: the no-currency branch now setserrortoo, so all three fiat failures signal it the same way. Theamountplaceholder stays, becauseAmount.tsxand existing tests depend on it.getRawAmountFromSatsnow returns{ amount, forceUnit?, error? }. When the fiat conversion fails,amountis the sats value,forceUnitis'sats'anderrorhas the reason. Returning the sats value alone would be wrong, because the input would read "12618" as fiat.AmountInputtakes a newfiatErrorprop and shows it under the input, in place of the conversion rate line.forceUnit="sats", unit toggle hidden) until the screen is closed. The pin is not removed when a rate arrives later, becauseAmountInputwould then read the sats value as fiat. The global unit does not change.getUnformattedAmount(...).amountinto its input directly, instateFromPropsand inrecalculateDisplayAmount. The second commit also pins the input when the sats fallback is equal to the amount already shown.Amount: in the error branch, ajumboTextamount gets the error under it. While rates are still loading, the existing spinner is shown and no warning. Pending amounts (for example pending channel balances under the wallet balance) do not repeat the warning, because the main amount above them already shows it.What this does not cover
jumboText), for example the rows in the balance list, still show only "N/A".FiatStore.getFiatRatesstill replaces rates that were already loaded withundefinedwhen a refresh fails (from my comment in handle N/A amounts and disable fiat currency rates while app is offline #3831). With this PR the user sees why amounts show "N/A", but a failed refresh still removes working rates. That is a store change, so I left it for a separate PR.satAmountwhile the unit is fiat without a rate, and I have no noffer. They use the sameAmountInputprops as Send, which was run.Amountnow shows "N/A" instead of "Disabled", because that branch now haserror. This state should not happen in practice: the default is USD, and a migration fixes an empty value. The error text reusesgeneral.fiatRateNotAvailable, so there are no new locale strings.This pull request is categorized as a:
Checklist
yarn run tscand made sure my code compiles correctlyyarn run lintand made sure my code didn't contain any problematic patternsyarn run prettierand made sure my code is formatted correctlyyarn run testand made sure all of the tests passTesting
If you modified or added a utility file, did you add new unit tests?
In
utils/AmountUtils.test.ts: three new tests for the three fiat failure branches ofgetRawAmountFromSats(no rates, no rate for the selected currency, no currency), each checking the returned sats amount,forceUnitanderror, and thatgetSatAmountreads the amount back correctly with that unit. The five existing round-trip tests now check the new return shape, and the no-currency test ofgetUnformattedAmountcheckserror. Run against master'sAmountUtils.ts, exactly these 9 tests fail. The view and component changes have no unit tests; there are no component tests for them in the repo.I have tested this PR on the following platforms (please specify OS version and phone model/VM):
Pixel 9 Pro emulator, x86_64, Android 16. To get "fiat unit, no rate" I selected IDR, which is in the currency list but has no entry in the ZEUS rates response (BGN and KWD are missing too). Send was opened with
bitcoin:1A1zP1eP5QGefi2DMPTfTL5SLmv7DivfNa?amount=0.00012618, then the same link withamount=0.0002was sent while Send was open. Also checked: the keypad opened from the pinned input is in sats without the unit toggle, the wallet balance and the Verify screen show the warning under "≈N/A", and with USD (which has a rate) Send looks the same as on master. Screenshots are in comments below.I have tested this PR with the following types of nodes (please specify node version and API version where appropriate):
On-device
Remote
LDK Node on mainnet. The change is in amount handling in the views and does not depend on the backend.
Locales
Third Party Dependencies and Packages
yarnafter this PR is merged inpackage.jsonandyarn.lockhave been properly updatedOther:
Closes #4635
Closes #3831