[WIP] Lightning top-up flow#4162
Closed
bznein wants to merge 5 commits into
Closed
Conversation
bznein
force-pushed
the
top-up-flow
branch
3 times, most recently
from
22a4b36 to
ede1800
Compare
Collaborator
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds a Lightning top-up feature: backend endpoint /api/lightning/top-up/info with account discovery and tests; frontend API types and getTopUpInfo; TopUp route with hooks (boarding address, balance, source-account), a useTopUpDraft hook, TopUpForm/EmptyState UI and styles, routing at /lightning/top-up; ActionButtons wiring to fetch top-up info and optionally connect a keystore before navigating; refactors send proposal logic into useTxProposal and small SendResult API surface extensions; and removes the boarding-address display on the Lightning page. ChangesLightning top-up feature
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/routes/lightning/components/action-buttons.tsx`:
- Around line 20-30: The handleTopUpClick handler currently performs a local
keystore connection flow using connectAnyKeystore; replace this by delegating to
the shared keystore flow: either simply navigate to '/lightning/top-up' (so the
page-level KeystoreConnectPrompt handles prompting) or call the shared
connectKeystore() API and let the KeystoreConnectPrompt handle UI, removing
local connection logic in handleTopUpClick and any use of connectAnyKeystore;
update references in handleTopUpClick (and the similar code at lines ~46-47) to
use navigate('/lightning/top-up') or connectKeystore() + KeystoreConnectPrompt
instead of the local connectAnyKeystore flow.
In `@frontends/web/src/routes/lightning/top-up.tsx`:
- Around line 255-276: When a proposal fails the component never clears the
loading flag, so add calls to setIsUpdatingProposal(false) when handling failed
or completed proposals: after assigning proposePromise from
accountApi.proposeTx(...) keep the existing comparison checks against
lastProposal.current and ensure that in the catch block you call
setIsUpdatingProposal(false) when (proposePromise === lastProposal.current), and
likewise in the finally block when clearing lastProposal.current also call
setIsUpdatingProposal(false) if (proposePromise === lastProposal.current);
reference setIsUpdatingProposal, proposeTimeout, lastProposal,
accountApi.proposeTx, and the existing try/catch/finally around proposePromise.
🪄 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: ac5fcff1-5d08-4ac9-9ada-2441b527fa1e
📒 Files selected for processing (8)
frontends/web/src/locales/en/app.jsonfrontends/web/src/routes/account/send/components/result.tsxfrontends/web/src/routes/lightning/components/action-buttons.module.cssfrontends/web/src/routes/lightning/components/action-buttons.tsxfrontends/web/src/routes/lightning/lightning.tsxfrontends/web/src/routes/lightning/top-up.module.cssfrontends/web/src/routes/lightning/top-up.tsxfrontends/web/src/routes/router.tsx
bznein
force-pushed
the
top-up-flow
branch
2 times, most recently
from
9c54add to
3cfe0e4
Compare
Collaborator
Author
|
@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/routes/account/send/use-tx-proposal.ts`:
- Around line 97-108: The hook currently captures txInput before the debounce
window, causing stale values to be proposed; fix by moving the call to
getValidTxInputData() inside the proposeTimeout callback (and re-check
accountCode there), ensure you clear any existing proposeTimeout before
scheduling a new setTimeout, and only call accountApi.proposeTx(accountCode,
txInput) with the freshly read txInput; keep existing behavior around
clearOnInvalidInput -> clearProposal() and setIsUpdatingProposal(true) but base
those actions on the validated input read inside the timeout.
- Around line 114-123: The catch block for proposePromise (variables:
proposePromise, lastProposal.current) currently clears validity and logs but
doesn't set a user-visible error after setErrorHandling({}) was previously
called; update the catch to (1) set an explicit user-facing error via
setErrorHandling({ message: String(error) }) or a structured error object so the
UI explains why send is disabled, (2) clear or remove the stale proposal data
(e.g., clear lastProposal.current and any proposal state tied to it) so the UI
doesn't keep showing the old proposal, and (3) keep the existing setValid(false)
and setIsUpdatingProposal(false) only when proposePromise ===
lastProposal.current to avoid race conditions; ensure the finally block still
resets lastProposal.current and setIsUpdatingProposal(false) conditionally to
avoid leaving inconsistent state.
In `@frontends/web/src/routes/lightning/top-up.tsx`:
- Around line 125-153: The effect that calls
connectKeystore(accountToConnect.keystore.rootFingerprint) doesn't handle
promise rejection, so connectPromptPending can remain true; update the effect to
catch errors from connectKeystore and in both then and catch ensure
setConnectPromptPending(false) is called (guarded by mounted.current), and on
rejection clear connectPromptedFingerprint if appropriate; apply the same
pattern to the other call site around lines 348-350 so neither path leaves the
spinner stuck.
- Around line 125-153: Replace the page-local keystore connection state machine
(remove connectPromptPending, connectPromptedFingerprint, accountNeedsConnect
and the second useEffect that calls connectKeystore) and instead invoke the
shared keystore flow: call the existing connectKeystore(rootFingerprint) only
via the shared KeystoreConnectPrompt component used elsewhere; render
KeystoreConnectPrompt when accountToConnect (from sourceAccount ||
preferredBitcoinAccount) exists and !accountToConnect.keystore.connected,
passing accountToConnect.keystore.rootFingerprint and any required
callbacks/props, and rely on that component to handle prompting/pending/errors
rather than managing local state in this file.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1619ade3-0345-42ac-8bec-8a893909f79c
📒 Files selected for processing (10)
frontends/web/src/locales/en/app.jsonfrontends/web/src/routes/account/send/components/result.tsxfrontends/web/src/routes/account/send/send.tsxfrontends/web/src/routes/account/send/use-tx-proposal.tsfrontends/web/src/routes/lightning/components/action-buttons.module.cssfrontends/web/src/routes/lightning/components/action-buttons.tsxfrontends/web/src/routes/lightning/lightning.tsxfrontends/web/src/routes/lightning/top-up.module.cssfrontends/web/src/routes/lightning/top-up.tsxfrontends/web/src/routes/router.tsx
💤 Files with no reviewable changes (1)
- frontends/web/src/routes/lightning/lightning.tsx
✅ Files skipped from review due to trivial changes (2)
- frontends/web/src/routes/lightning/components/action-buttons.module.css
- frontends/web/src/routes/lightning/components/action-buttons.tsx
Collaborator
Author
|
@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/routes/account/send/components/result.tsx`:
- Around line 124-133: The secondary "New transaction" button is rendered even
when onContinue is undefined, producing a no-op action; change the rendering so
the secondary Button inside ViewButtons is only rendered when onContinue is
provided (e.g., wrap the secondary Button with a conditional check on
onContinue) so that ViewButtons only contains actionable controls when
showSuccessActions is true and onContinue exists; reference the existing symbols
showSuccessActions, ViewButtons, Button, onContinue, navigate, and donePath to
locate and update the JSX accordingly.
In `@frontends/web/src/routes/lightning/top-up-form.tsx`:
- Around line 141-143: The current JSX renders "-" while lightningBalance is
undefined; change the conditional in the TopUp form so that lightningBalance ===
undefined returns nothing (or a Skeleton component) to represent loading, and
only render AmountWithUnit when lightningBalance is a defined object (and keep
"-" only for an explicit null/empty state if needed). Update the conditional
around lightningBalance and the AmountWithUnit usage so undefined maps to a
loading render (e.g., null or <Skeleton />) and a defined value maps to
<AmountWithUnit maxDecimals={9} amount={lightningBalance.available} />.
In `@frontends/web/src/routes/lightning/top-up-hooks.ts`:
- Around line 110-115: The current call to connectKeystore(...) only clears
setConnectPromptPending(true) on success, so if connectKeystore rejects the UI
stays stuck; update the logic around
connectKeystore(accountToConnect.keystore.rootFingerprint) (or the function that
triggers it) to use a finally path (either .catch/.finally chain or async/await
with try { await connectKeystore(...) } finally { if (mounted.current)
setConnectPromptPending(false) }) so setConnectPromptPending(false) always runs
regardless of success or failure and still guards with mounted.current before
updating state.
- Around line 105-116: The useEffect currently calls connectKeystore(...)
automatically when accountNeedsConnect/accountToConnect change; instead remove
the connectKeystore call from this state-only hook (keep only setting
setConnectPromptedFingerprint and setConnectPromptPending based on
accountNeedsConnect/accountToConnect/mounted) and wire the actual
connectKeystore(...) invocation to an explicit user action handler (e.g., the
send/review click flow) so the device connection happens only in response to
user intent; update callers to invoke
connectKeystore(accountToConnect.keystore.rootFingerprint) from the action path
and follow the explicit-action patterns used in the sign-message, widget, and
pocket flows.
🪄 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: 54e981c2-f3dd-47b2-97a3-a4714f8c0573
📒 Files selected for processing (13)
frontends/web/src/locales/en/app.jsonfrontends/web/src/routes/account/send/components/result.tsxfrontends/web/src/routes/account/send/send.tsxfrontends/web/src/routes/account/send/use-tx-proposal.tsfrontends/web/src/routes/lightning/components/action-buttons.module.cssfrontends/web/src/routes/lightning/components/action-buttons.tsxfrontends/web/src/routes/lightning/lightning.tsxfrontends/web/src/routes/lightning/top-up-draft.tsfrontends/web/src/routes/lightning/top-up-form.tsxfrontends/web/src/routes/lightning/top-up-hooks.tsfrontends/web/src/routes/lightning/top-up.module.cssfrontends/web/src/routes/lightning/top-up.tsxfrontends/web/src/routes/router.tsx
💤 Files with no reviewable changes (1)
- frontends/web/src/routes/lightning/lightning.tsx
bznein
force-pushed
the
top-up-flow
branch
2 times, most recently
from
583a556 to
657da26
Compare
Regular send and Lightning top up both need the same transaction proposal lifecycle: debounce user edits, ignore stale proposals, expose fee and amount details, and convert proposed amounts to fiat. Keep that behavior in one hook so future fixes to proposal loading or error handling apply to both flows instead of being copied between screens.
Lightning top up reuses the send result component but does not offer a follow-up transaction action. Rendering the secondary button without an onContinue handler creates a visible no-op control. Only render the secondary action when a caller provides onContinue, while leaving the normal send result behavior unchanged.
The top up screen needs Lightning-specific data and Bitcoin account selection before it can build a transaction proposal. Keeping this in the route mixed data loading with screen flow. Move boarding address loading, Lightning balance loading, source account selection, and draft state into focused hooks. The hooks keep the route small while still using the existing send proposal API.
The top up UI is separate from route orchestration so the flow can be reviewed in smaller pieces. It still needs the familiar send controls because top up is an on-chain Bitcoin send to a Spark boarding address. Build the form from existing send components for fiat input, fee targets, confirmation, and send result. This keeps the approval path consistent with regular Bitcoin sends.
bznein
force-pushed
the
top-up-flow
branch
5 times, most recently
from
8ce3772 to
66c8093
Compare
Collaborator
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
frontends/web/src/routes/account/send/use-tx-proposal.ts (2)
97-108:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse fresh tx input inside the debounce callback.
txInputis captured at Line 97, then proposed 400ms later at Line 108. If the form updates during that window, proposals can be generated from stale input.Suggested fix
- const txInput = getValidTxInputData(); - if (!txInput || !accountCode) { + if (!accountCode) { if (clearOnInvalidInput) { clearProposal(); } return; } setIsUpdatingProposal(true); proposeTimeout.current = setTimeout(async () => { + const txInput = getValidTxInputData(); + if (!txInput) { + setIsUpdatingProposal(false); + return; + } let proposePromise: Promise<accountApi.TTxProposalResult> | null = null; try { proposePromise = accountApi.proposeTx(accountCode, txInput);🤖 Prompt for 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. In `@frontends/web/src/routes/account/send/use-tx-proposal.ts` around lines 97 - 108, The debounce callback currently uses the txInput captured before setTimeout, which can be stale; update the callback in proposeTimeout.current to call getValidTxInputData() (and re-check accountCode) inside the async function just before calling accountApi.proposeTx so the proposal uses the latest form state; preserve the existing behavior for clearOnInvalidInput by calling clearProposal() and returning if the fresh input is invalid, and keep setIsUpdatingProposal(true)/false and proposePromise handling as-is.
114-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate proposal failures to UI state instead of only logging.
On failures (Line 114+), the hook logs and sets
validfalse, but does not setsetErrorHandling(...)or clear stale proposal data. This can leave outdated proposal values visible with no actionable error message.Suggested fix
} catch (error) { if (proposePromise === lastProposal.current) { + setErrorHandling({ message: String(error) }); setValid(false); + setProposedAmount(undefined); + setProposedTotal(undefined); + setRecipientDisplayAddress(''); setIsUpdatingProposal(false); console.error('Failed to propose transaction:', error); } } finally {🤖 Prompt for 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. In `@frontends/web/src/routes/account/send/use-tx-proposal.ts` around lines 114 - 123, The catch/finally currently only logs and toggles setValid/setIsUpdatingProposal; modify the error path for proposePromise/lastProposal.current so when a proposal fails you call setErrorHandling(...) with the error (or a structured message), clear stale proposal data by resetting lastProposal.current and any proposal state values (e.g., the proposal object/inputs maintained by this hook), and ensure setIsUpdatingProposal(false) is always called; specifically update the catch block that checks (proposePromise === lastProposal.current) to invoke setErrorHandling(error), clear the stored proposal state and then in finally ensure lastProposal.current is nulled and setIsUpdatingProposal(false) so the UI shows the error and no stale proposal remains.
🤖 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.
Duplicate comments:
In `@frontends/web/src/routes/account/send/use-tx-proposal.ts`:
- Around line 97-108: The debounce callback currently uses the txInput captured
before setTimeout, which can be stale; update the callback in
proposeTimeout.current to call getValidTxInputData() (and re-check accountCode)
inside the async function just before calling accountApi.proposeTx so the
proposal uses the latest form state; preserve the existing behavior for
clearOnInvalidInput by calling clearProposal() and returning if the fresh input
is invalid, and keep setIsUpdatingProposal(true)/false and proposePromise
handling as-is.
- Around line 114-123: The catch/finally currently only logs and toggles
setValid/setIsUpdatingProposal; modify the error path for
proposePromise/lastProposal.current so when a proposal fails you call
setErrorHandling(...) with the error (or a structured message), clear stale
proposal data by resetting lastProposal.current and any proposal state values
(e.g., the proposal object/inputs maintained by this hook), and ensure
setIsUpdatingProposal(false) is always called; specifically update the catch
block that checks (proposePromise === lastProposal.current) to invoke
setErrorHandling(error), clear the stored proposal state and then in finally
ensure lastProposal.current is nulled and setIsUpdatingProposal(false) so the UI
shows the error and no stale proposal remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 286a209d-8f4a-48a0-89c8-aa18abfeff51
📒 Files selected for processing (18)
backend/handlers/handlers.gobackend/lightning_topup.gobackend/lightning_topup_test.gofrontends/web/src/api/lightning.tsfrontends/web/src/locales/en/app.jsonfrontends/web/src/routes/account/send/components/result.tsxfrontends/web/src/routes/account/send/send.tsxfrontends/web/src/routes/account/send/use-tx-proposal.tsfrontends/web/src/routes/lightning/components/action-buttons.module.cssfrontends/web/src/routes/lightning/components/action-buttons.test.tsxfrontends/web/src/routes/lightning/components/action-buttons.tsxfrontends/web/src/routes/lightning/lightning.tsxfrontends/web/src/routes/lightning/top-up-draft.tsfrontends/web/src/routes/lightning/top-up-form.tsxfrontends/web/src/routes/lightning/top-up-hooks.tsfrontends/web/src/routes/lightning/top-up.module.cssfrontends/web/src/routes/lightning/top-up.tsxfrontends/web/src/routes/router.tsx
💤 Files with no reviewable changes (1)
- frontends/web/src/routes/lightning/lightning.tsx
Collaborator
Author
|
@Beerosagos this turned otu to be quite big and with a lot of frontend changes. The overall diff is ready for review, it looks bigger than it actually is because I moved stuff (shared code for example between send and top-up). WOuld you be willing to test this first and see if the general code approach seems ok to you? Just before I try to clean it up more |
Users need a way to fund their Lightning wallet from an active Bitcoin account in the app. The flow should not send them to add an account when a BitBox is disconnected or an existing account only needs to be enabled. Add the route and action entry point, connect the BitBox from the explicit top up action, prefer the Bitcoin account tied to Lightning, and send users to account management when no active Bitcoin account is available.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before asking for reviews, here is a check list of the most common things you might need to consider: