frontend: enable top-up flow.#4166
Conversation
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR adds a Lightning wallet top-up feature to the frontend. It introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TopUpForm
participant LightningTopUp
participant accountApi
participant TopUpConfirm
participant Keystore
User->>TopUpForm: enter amount, select source account
TopUpForm->>LightningTopUp: onReview()
LightningTopUp->>accountApi: proposeTx(sourceAccount, amount)
accountApi-->>LightningTopUp: proposal result
LightningTopUp->>TopUpConfirm: render(proposal, sourceAccount)
User->>TopUpConfirm: confirm
LightningTopUp->>Keystore: connectKeystore(rootFingerprint)
LightningTopUp->>accountApi: sendTx(sourceAccount, note)
accountApi-->>LightningTopUp: success/aborted/error
LightningTopUp-->>User: render TopUpSuccess/TopUpAborted/TopUpForm
Related Issues: None referenced in the provided context. Related PRs: None referenced in the provided context. Suggested labels: frontend, feature, lightning Suggested reviewers: benma, Beerosagos Poem ✨ 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 `@backend/lightning/topup.go`:
- Around line 92-99: The function getTopUpSourceAccount must guard against a nil
sourceAccount returned by getAccountFromCode before calling
sourceAccount.Coin(); add an explicit nil check after calling getAccountFromCode
(and return a suitable error, e.g., accountErrors.ErrInvalidAddress or wrap the
original err when err==nil) so you never dereference sourceAccount.Coin() on a
nil pointer; update the getTopUpSourceAccount control flow to handle
(sourceAccount == nil) and then proceed to compare sourceAccount.Coin().Code()
with coin.CodeBTC.
In `@frontends/web/src/routes/lightning/topup/topup.tsx`:
- Around line 365-398: The handleReview flow allows concurrent postTopUpSend
calls; add a local "isSubmitting" (or reuse state like "step") guard to return
early if a send is already in flight, set it true immediately before calling
postTopUpSend and set it false in all exit paths (catch, success, aborted, and
error branches) so duplicate clicks are ignored; update the UI trigger (the
button that calls handleReview) to be disabled when this flag is true. Apply the
same guard and state-reset logic to the other send handler at the second
occurrence (the block referenced at lines 514-518) so both send flows prevent
duplicate submissions.
🪄 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: 14024729-8a19-4680-bba3-3275521ac0f3
📒 Files selected for processing (15)
backend/handlers/handlers.gobackend/lightning/handlers.gobackend/lightning/lightning.gobackend/lightning/topup.gobackend/lightning/topup_test.gofrontends/web/src/api/lightning.tsfrontends/web/src/components/dropdown/dropdown.tsxfrontends/web/src/locales/en/app.jsonfrontends/web/src/routes/account/send/feetargets.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/topup/topup.module.cssfrontends/web/src/routes/lightning/topup/topup.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-2
branch
4 times, most recently
from
853b432 to
76f2799
Compare
|
@CodeRabbit review |
✅ Action performedReview finished.
|
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)
backend/lightning/topup_test.go (1)
1-137: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider adding test coverage for
Deactivateclearing the boarding address.While the existing tests cover the main top-up flows, there's no test verifying that
Lightning.Deactivate()clears the cached boarding address (viaclearTopUpBoardingAddress()atlightning.go:146).Adding a test would ensure this cleanup behavior remains correct across refactorings.
🧪 Suggested test
+func TestDeactivateClearsBoardingAddress(t *testing.T) { + lightning := makeTestLightning() + lightning.topUpBoardingAddress = "bc1qtopupaddress" + + // Assume Deactivate is callable in test context + // You may need to adjust based on test setup requirements + err := lightning.Deactivate() + + require.NoError(t, err) + require.Empty(t, lightning.topUpBoardingAddress) +}🤖 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 `@backend/lightning/topup_test.go` around lines 1 - 137, Add a unit test that verifies Lightning.Deactivate() clears the cached boarding address: create a Lightning via makeTestLightning(), set lightning.topUpBoardingAddress to a non-empty value, call lightning.Deactivate(), then assert lightning.topUpBoardingAddress is empty (this exercises clearTopUpBoardingAddress()). Place the test alongside existing topup tests and use require to fail on unexpected results.
🤖 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/topup.go`:
- Around line 160-174: The cached topUpBoardingAddress is intentionally
session-scoped and not cleared by topUpSend; update the code comments and/or
function docs to state that topUpBoardingAddress is reused across multiple
top-ups until explicitly cleared by Lightning.Deactivate (and not by topUpSend
or clearTopUpBoardingAddress on success), referencing the topUpSend function,
the clearTopUpBoardingAddress helper, Lightning.Deactivate, and the existing
tests TestTopUpSendKeepsCachedBoardingAddress and
TestTopUpProposalUsesCachedBoardingAddress to make the intended lifecycle
explicit.
---
Outside diff comments:
In `@backend/lightning/topup_test.go`:
- Around line 1-137: Add a unit test that verifies Lightning.Deactivate() clears
the cached boarding address: create a Lightning via makeTestLightning(), set
lightning.topUpBoardingAddress to a non-empty value, call
lightning.Deactivate(), then assert lightning.topUpBoardingAddress is empty
(this exercises clearTopUpBoardingAddress()). Place the test alongside existing
topup tests and use require to fail on unexpected results.
🪄 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: 188da02e-427a-4c98-b068-809b85d38ecb
📒 Files selected for processing (15)
backend/handlers/handlers.gobackend/lightning/handlers.gobackend/lightning/lightning.gobackend/lightning/topup.gobackend/lightning/topup_test.gofrontends/web/src/api/lightning.tsfrontends/web/src/components/dropdown/dropdown.tsxfrontends/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/topup/topup.module.cssfrontends/web/src/routes/lightning/topup/topup.tsxfrontends/web/src/routes/router.tsx
bznein
force-pushed
the
top-up-2
branch
6 times, most recently
from
9f7b713 to
c8e5ae0
Compare
|
@CodeRabbit review |
✅ Action performedReview finished.
|
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 `@backend/lightning/topup_test.go`:
- Line 20: In the test setup for topup_test.go where RateUpdater is being
initialized, replace the hardcoded string "/dev/null" with os.DevNull to ensure
cross-platform compatibility. The os.DevNull constant automatically resolves to
the correct null device path for the operating system the tests are running on
(e.g., "/dev/null" on Unix-like systems and "nul" on Windows). This change will
prevent test failures on Windows runners.
In `@frontends/web/src/routes/lightning/topup/topup.tsx`:
- Around line 293-319: The connectKeystore() call should be extracted outside of
the try/catch block to follow the early-return pattern outlined in the coding
guidelines. First, call connectKeystore() and immediately return early if it
fails (checking connectResult.success). After this early-return guard, then
establish the try/catch block that wraps only the postTopUpSend() call and its
result handling (including the setStep() calls for 'confirming', 'success',
'aborted', 'form' states and setSendError() calls). This way, connectKeystore()
failures use simple early-return logic while postTopUpSend() failures are
handled by the catch block.
🪄 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: 1f26ba0c-57c4-4fd6-abc2-54c842ad874f
📒 Files selected for processing (19)
backend/accounts/tx_responses.gobackend/coins/btc/handlers/handlers.gobackend/handlers/handlers.gobackend/lightning/handlers.gobackend/lightning/lightning.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/lightning/components/action-buttons.module.cssfrontends/web/src/routes/lightning/components/action-buttons.tsxfrontends/web/src/routes/lightning/lightning.tsxfrontends/web/src/routes/lightning/topup/topup-confirm.tsxfrontends/web/src/routes/lightning/topup/topup-form.tsxfrontends/web/src/routes/lightning/topup/topup-result.tsxfrontends/web/src/routes/lightning/topup/topup.module.cssfrontends/web/src/routes/lightning/topup/topup.tsxfrontends/web/src/routes/router.tsx
💤 Files with no reviewable changes (1)
- frontends/web/src/routes/lightning/lightning.tsx
|
|
||
| func topUpTestAccountConfig() *accounts.AccountConfig { | ||
| return &accounts.AccountConfig{ | ||
| RateUpdater: rates.NewRateUpdater(nil, "/dev/null"), |
There was a problem hiding this comment.
Use a cross-platform null device in test setup.
Line 20 hardcodes "/dev/null", which is Unix-specific and can break go test on Windows runners. Use os.DevNull instead.
Proposed fix
import (
+ "os"
"testing"
@@
func topUpTestAccountConfig() *accounts.AccountConfig {
return &accounts.AccountConfig{
- RateUpdater: rates.NewRateUpdater(nil, "/dev/null"),
+ RateUpdater: rates.NewRateUpdater(nil, os.DevNull),
}
}🤖 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 `@backend/lightning/topup_test.go` at line 20, In the test setup for
topup_test.go where RateUpdater is being initialized, replace the hardcoded
string "/dev/null" with os.DevNull to ensure cross-platform compatibility. The
os.DevNull constant automatically resolves to the correct null device path for
the operating system the tests are running on (e.g., "/dev/null" on Unix-like
systems and "nul" on Windows). This change will prevent test failures on Windows
runners.
|
@Beerosagos this is ready for review; I know the PR is quite big but it's not really easy to slim it down even more |
|
Did a quick test and found a couple of bugs:
I wonder if it wouldn't be simpler to grey out the Top up button if no keystore is connected. wdyt? cc @jadzeidan |
Beerosagos
left a comment
Beerosagos
left a comment
There was a problem hiding this comment.
I think we should show the incoming payment in the transaction list if possible.
Can you have a look at this? https://sdk-doc-spark.breez.technology/guide/receive_payment.html#bitcoin
We should probably manage the right events to handle incoming top ups
| {t('generic.send')} | ||
| </span> | ||
| )} | ||
| <Link key="topUp" to={'/lightning/topup'} className={style.topUp}> |
There was a problem hiding this comment.
I think this should go after receive
|
cc @thisconnect or @shonsirsha for frontend code 😇 |
|
I moved this back to draft for a bit to:
Will ping people when ready for review :) |
@Beerosagos a grey button is simpler but imo not ideal since you don't know why it is greyed out. I think it makes sense to keep as is but change the message "No Bitcoin accounts active. Please activate a Bitcoin account to top-up." With a CTA to manage accounts. |
btw we can address this in a separate PR as well. Don't want to blow things up too much 🙏 |
The CTA to manage accounts only works if there are no bitcoin accounts active (but a keystore connected); in other cases we should either prompt the user to connect or simply redirect to the homepage where we already tell users to connect the BitBox |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
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/topup/topup.tsx`:
- Around line 343-344: The empty-state CTA in TopUp is driven by the coarse
hasAccounts flag, which can send disconnected users with existing accounts into
the broken manage-accounts flow. Update the TopUp render path that returns
TopUpNoBitcoinAccounts to pass connection state (or determine the CTA inside
TopUp) using the existing connection/keystore signals so disconnected users are
routed through the shared connect flow instead of /settings/manage-accounts.
- Around line 30-44: The Lightning balance fetch in the top-up screen only runs
once on mount, so a failed initial `getLightningBalance()` call leaves the
balance stuck as undefined after the user connects. Update the `useEffect` in
`topup.tsx` to re-run when the keystore connection state changes, or trigger the
fetch from `connectKeystore()` after a successful connect, so `setBalance` gets
called again once `mounted.current` is still true.
🪄 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: 0db3ff69-39d3-45f3-90fc-59dc46fbda25
📒 Files selected for processing (12)
backend/lightning/payments_test.gofrontends/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/topup/topup-confirm.tsxfrontends/web/src/routes/lightning/topup/topup-form.tsxfrontends/web/src/routes/lightning/topup/topup-result.tsxfrontends/web/src/routes/lightning/topup/topup.module.cssfrontends/web/src/routes/lightning/topup/topup.tsxfrontends/web/src/routes/router.tsx
Allow users to top up their lightning wallet directly in the app, by adding a new page that allows a user to select a source BTC account, an amount, and create the tx.
shonsirsha
left a comment
shonsirsha
left a comment
There was a problem hiding this comment.
I see that it's still in draft, but just commenting what Im seeing rn:)
| setFiatAmount(''); | ||
| return; | ||
| } | ||
| const data = await convertToCurrency({ |
There was a problem hiding this comment.
Hm..
could we somehow ignore stale fiat conversion responses here?
Feels like an edge case but im thinking something like this can happen since its all async
Type A => request A starts.
Type B => request B starts.
B returns first => UI is correct for B.
A returns later => stale A overwrites B. UI is wrong.
| {sourceAccount && ( | ||
| <Column> | ||
| <FeeTargets | ||
| accountCode={sourceAccount.code} |
There was a problem hiding this comment.
FeeTargets calls useLoad w/o accountCode in the deps - so it only loads for the first account that it sees.
Here tho, I see that we're using GroupedAccountSelector to change the source account.
Pretty sure it'll re-render FeeTargets on account change, but that useLoad will still have stale value, I think. Because the useEffect inside it doesn't rerun unless deps change.
4 participants
Allow users to top up their lightning wallet directly in the app, by adding a new page that allows a user to select a source BTC account, an amount, and create the tx.
Before asking for reviews, here is a check list of the most common things you might need to consider: