frontend: improve insufficient funds error in lightning wallet#4142
frontend: improve insufficient funds error in lightning wallet#4142strmci wants to merge 1 commit into
Conversation
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR unifies payment error handling and refactors custom Lightning invoice amount flows. The backend adds error normalization via ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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 |
Beerosagos
left a comment
Beerosagos
left a comment
There was a problem hiding this comment.
I think we should manually check the total debit against the available balance inside PreparePayment. This way, we don't rely on SDK validation, and we can error earlier.
Please also consider a frontend validation for the case when the invoice has no amount and the user has to input it. 🙏
strmci
force-pushed
the
lightning_insufficient_funds
branch
from
85c2485 to
046f5e9
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/lightning/handlers.go`:
- Line 117: The AvailableSat field assignment currently calls
balance.Available().BigInt().Uint64() unguarded; change it to first obtain b :=
balance.Available().BigInt(), then if b.Sign() > 0 && b.IsUint64() use
b.Uint64() else set AvailableSat to 0 to avoid negative or non-representable
wraparound; update the assignment site in handlers.go where AvailableSat is set
(referencing balance.Available(), BigInt(), IsUint64()) so clients never receive
wrapped/invalid amounts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 71892e58-08ab-405e-931d-6ca55258993c
📒 Files selected for processing (9)
backend/lightning/handlers.gobackend/lightning/payments.gobackend/lightning/payments_test.gofrontends/web/src/api/lightning.tsfrontends/web/src/locales/en/app.jsonfrontends/web/src/routes/lightning/send/components/edit-invoice-step.module.cssfrontends/web/src/routes/lightning/send/components/edit-invoice-step.tsxfrontends/web/src/routes/lightning/send/lightning-send-context.test.tsfrontends/web/src/routes/lightning/send/lightning-send-context.tsx
|
I added the check to I also checked the Rabbit review comment, but I’m not sure it’s necessary to add. PTAL thanks 🙏 |
Beerosagos
left a comment
Beerosagos
left a comment
There was a problem hiding this comment.
The more I look into this, the more I think we should load the fee asynchronously in the edit-invoice step and skip the confirmation step entirely (for invoices without an amount).
We could call the preparePayment endpoint while the user types in the amount and load the fee in the same view. This avoids the need to check for balance errors in the frontend, keep the context cleaner and so on.
I asked codex to recap the suggested changes:
Suggest moving validation to preparePayment() instead of extending the balance API or duplicating balance checks in frontend/backend helpers. For variable-amount Lightning invoices, trigger preparePayment() once the entered amount is
valid, ideally debounced while typing or on blur. Use that result as the authoritative source for both fee display and insufficient-funds detection, since it already reflects the real quote and failure mode.
Recommend removing the availableSat addition, the extra backend checkPaymentBalance() after prepare, and the frontend amount > balance heuristic. Those add API surface and duplicate logic, but still do not model the real constraint
(amount + fee). The current string-based insufficient-funds fallback should stay only if the SDK gives no typed alternative, and it should be treated as a narrow temporary shim.
For UX, suggest storing the current amount plus the latest prepare state in the send form. Show fee and total immediately when prepare succeeds, and show the returned lightningInsufficientFunds error inline when it fails. That would let
the user accept the quote directly and potentially skip the separate confirm step for custom-amount invoices. If implemented, make sure prepares are debounced and stale responses cannot overwrite newer input.
WDYT?
strmci
force-pushed
the
lightning_insufficient_funds
branch
from
046f5e9 to
af0b237
Compare
strmci
changed the title
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontends/web/src/api/lightning.ts`:
- Around line 4-6: Change the relative imports in this module to use the project
alias `@/` instead of `./`; replace "./account" with "`@/api/account`" for
AccountCode, TAmountWithConversions, TBalance, TTransactionStatus and replace
"./subscribe" with "`@/api/subscribe`" for TSubscriptionCallback, TUnsubscribe and
subscribeEndpoint so the module follows the frontend import convention.
In `@frontends/web/src/routes/lightning/send/components/invoice-details.tsx`:
- Around line 75-78: Rename the props type TPaymentFeeDetailsProps to TProps in
this component: change the type declaration from TPaymentFeeDetailsProps to
TProps and update all references (the component's props annotation and any other
uses) accordingly within invoice-details.tsx so the component signature (e.g.,
the React function/FC and its parameter typing) uses TProps to comply with the
frontend naming convention.
In `@frontends/web/src/routes/lightning/send/lightning-send-context.test.ts`:
- Line 3: Replace the deep relative import "../../../../__mocks__/i18n" with the
frontend path alias import "`@/__mocks__/i18n`" in the test file so the mock
import uses the project alias instead of leaving the src/routes tree; update the
import statement in lightning-send-context.test.ts accordingly.
- Around line 64-66: The inline props type for the wrapper component should be
replaced with a named alias TProps and used in the wrapper signature; declare
type TProps = { children: ReactNode } and update the wrapper definition to use
TProps (keeping the createElement(LightningSendProvider, null, children) body)
so the props type follows the "TProps" naming guideline and still references
LightningSendProvider and ReactNode.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9355c143-bf28-4ced-bcde-ae6649251721
📒 Files selected for processing (10)
backend/lightning/handlers.gobackend/lightning/payments.gobackend/lightning/payments_test.gofrontends/web/src/api/lightning.tsfrontends/web/src/locales/en/app.jsonfrontends/web/src/routes/lightning/send/components/confirm-step.tsxfrontends/web/src/routes/lightning/send/components/edit-invoice-step.tsxfrontends/web/src/routes/lightning/send/components/invoice-details.tsxfrontends/web/src/routes/lightning/send/lightning-send-context.test.tsfrontends/web/src/routes/lightning/send/lightning-send-context.tsx
💤 Files with no reviewable changes (1)
- frontends/web/src/routes/lightning/send/components/confirm-step.tsx
strmci
force-pushed
the
lightning_insufficient_funds
branch
2 times, most recently
from
850014f to
130a17b
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/lightning/handlers.go`:
- Around line 152-154: The current conditional in the handler uses
jsonBody.AmountSat != nil which prevents returning the structured
insufficient-funds response with fee for fixed-amount invoices; update the
conditional in the block that checks errLightningInsufficientFunds to remove the
AmountSat != nil check so that when fee != nil and errp.Cause(err) ==
errLightningInsufficientFunds you return responseDto{Success: false, ErrorCode:
string(errLightningInsufficientFunds), Data: fee}; if this behavior was
intentionally limited to custom-amount flows instead, add a concise comment
above the check explaining that design decision rather than silently leaving the
guard in place (refer to jsonBody.AmountSat, errLightningInsufficientFunds,
responseDto and errorResponse).
In `@frontends/web/src/routes/lightning/send/components/invoice-details.tsx`:
- Around line 83-95: The total can stay as a skeleton when fiat conversion fails
because convertedTotalDebitAmount is undefined but totalWithFiat remains true;
change the logic in invoice-details.tsx so totalDebitAmount falls back to
satsAmount(quote?.totalDebitSat) when convertedTotalDebitAmount is undefined and
only enable fiat rendering when conversion data exists — i.e., compute
totalDebitAmount = convertedTotalDebitAmount ?? satsAmount(quote?.totalDebitSat)
and pass showFiat={totalWithFiat && Boolean(convertedTotalDebitAmount)} to
AmountValue (functions/variables: useInvoiceAmount, convertedTotalDebitAmount,
totalWithFiat, quote?.totalDebitSat, satsAmount, AmountValue).
In `@frontends/web/src/routes/lightning/send/lightning-send-context.tsx`:
- Around line 122-123: The current error branch unsafely casts TSdkError.data to
TPreparePaymentResponse and stores it in customPrepareState.quote; add a runtime
type guard that validates TSdkError.data has the expected
TPreparePaymentResponse shape (e.g., required fields used later like fee,
amountSat, or paymentRequest) before assigning it to quote, otherwise set quote
to undefined. Update the block around the quote assignment (the quote variable
and the setCustomPrepareState call) to perform this check so only validated
quote objects are stored and downstream fee rendering is safe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9fb9f0f1-ff1f-41c1-8d53-3dbc4c6a3dc5
📒 Files selected for processing (11)
backend/lightning/handlers.gobackend/lightning/payments.gobackend/lightning/payments_test.gofrontends/web/src/api/lightning.tsfrontends/web/src/locales/en/app.jsonfrontends/web/src/routes/lightning/send/components/confirm-step.tsxfrontends/web/src/routes/lightning/send/components/edit-invoice-step.tsxfrontends/web/src/routes/lightning/send/components/invoice-details.tsxfrontends/web/src/routes/lightning/send/lightning-send-context.test.tsfrontends/web/src/routes/lightning/send/lightning-send-context.tsxfrontends/web/src/routes/lightning/send/send.module.css
💤 Files with no reviewable changes (1)
- frontends/web/src/routes/lightning/send/components/confirm-step.tsx
- prepare custom-amount invoices while editing the amount - show fee, total, and insufficient-funds errors in the edit step - validate prepared payments against the available balance - map Lightning insufficient-funds SDK errors to an app error code - map already-used Lightning invoice SDK errors to an app error code
strmci
force-pushed
the
lightning_insufficient_funds
branch
from
130a17b to
b377434
Compare
@Beerosagos yes, makes sense. |
2 participants
Before asking for reviews, here is a check list of the most common things you might need to consider: